diff mbox series

[3/8] drm/ssd13xx: Replace .page_height field in device info with a constant

Message ID 20231009183522.543918-4-javierm@redhat.com
State New
Headers
Series drm/solomon: Add support for the SSD132x controller family |

Commit Message

Javier Martinez Canillas Oct. 9, 2023, 6:34 p.m. UTC 
  This deemed useful to avoid hardcoding a page height and allow to support
other Solomon controller families, but dividing the screen in pages seems
to be something that is specific to the SSD130x chip family.

For example, SSD132x chip family divides the screen in segments (columns)
and common outputs (rows), so the concept of screen pages does not exist
for the SSD132x family.

Let's drop this field from the device info struct and just use a constant
SSD130X_PAGE_HEIGHT macro to define the page height. While being there,
replace hardcoded 8 values in places where it is used as the page height.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/solomon/ssd13xx.c | 37 +++++++++++++++----------------
 drivers/gpu/drm/solomon/ssd13xx.h |  1 -
 2 files changed, 18 insertions(+), 20 deletions(-)

Comments

Geert Uytterhoeven Oct. 11, 2023, 7:39 a.m. UTC | #1 
On Mon, Oct 9, 2023 at 8:36 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> This deemed useful to avoid hardcoding a page height and allow to support
> other Solomon controller families, but dividing the screen in pages seems
> to be something that is specific to the SSD130x chip family.
>
> For example, SSD132x chip family divides the screen in segments (columns)
> and common outputs (rows), so the concept of screen pages does not exist
> for the SSD132x family.
>
> Let's drop this field from the device info struct and just use a constant
> SSD130X_PAGE_HEIGHT macro to define the page height. While being there,
> replace hardcoded 8 values in places where it is used as the page height.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

> --- a/drivers/gpu/drm/solomon/ssd13xx.c
> +++ b/drivers/gpu/drm/solomon/ssd13xx.c

> @@ -465,13 +462,13 @@ static int ssd13xx_update_rect(struct ssd13xx_device *ssd13xx,
>         unsigned int width = drm_rect_width(rect);
>         unsigned int height = drm_rect_height(rect);
>         unsigned int line_length = DIV_ROUND_UP(width, 8);
> -       unsigned int page_height = ssd13xx->device_info->page_height;
> +       unsigned int page_height = SSD130X_PAGE_HEIGHT;
>         unsigned int pages = DIV_ROUND_UP(height, page_height);
>         struct drm_device *drm = &ssd13xx->drm;
>         u32 array_idx = 0;
>         int ret, i, j, k;
>
> -       drm_WARN_ONCE(drm, y % 8 != 0, "y must be aligned to screen page\n");
> +       drm_WARN_ONCE(drm, y % page_height != 0, "y must be aligned to screen page\n");
>
>         /*
>          * The screen is divided in pages, each having a height of 8
> @@ -503,27 +500,32 @@ static int ssd13xx_update_rect(struct ssd13xx_device *ssd13xx,
>          */
>
>         if (!ssd13xx->page_address_mode) {
> +               u8 page_start;
> +
>                 /* Set address range for horizontal addressing mode */
>                 ret = ssd13xx_set_col_range(ssd13xx, ssd13xx->col_offset + x, width);
>                 if (ret < 0)
>                         return ret;
>
> -               ret = ssd13xx_set_page_range(ssd13xx, ssd13xx->page_offset + y / 8, pages);
> +               page_start = ssd13xx->page_offset + y / page_height;
> +               ret = ssd13xx_set_page_range(ssd13xx, page_start, pages);
>                 if (ret < 0)
>                         return ret;
>         }
>
>         for (i = 0; i < pages; i++) {
> -               int m = 8;
> +               int m = page_height;
>
>                 /* Last page may be partial */
> -               if (8 * (y / 8 + i + 1) > ssd13xx->height)
> -                       m = ssd13xx->height % 8;
> +               if (page_height * (y / page_height + i + 1) > ssd13xx->height)
> +                       m = ssd13xx->height % page_height;
> +
>                 for (j = 0; j < width; j++) {
>                         u8 data = 0;
>
>                         for (k = 0; k < m; k++) {
> -                               u8 byte = buf[(8 * i + k) * line_length + j / 8];
> +                               u32 idx = (page_height * i + k) * line_length + j / 8;

Nit: I would use unsigned int for idx, as that matches all other
variables involved.
But given array_idx is u32, too, u32 may makes sense.

> +                               u8 byte = buf[idx];
>                                 u8 bit = (byte >> (j % 8)) & 1;
>
>                                 data |= bit << k;

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
Javier Martinez Canillas Oct. 11, 2023, 8:33 a.m. UTC | #2 
Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

Thanks a lot for your feedback.

> On Mon, Oct 9, 2023 at 8:36 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:

[...]

>>         u32 array_idx = 0;

[...]

>>
>>                         for (k = 0; k < m; k++) {
>> -                               u8 byte = buf[(8 * i + k) * line_length + j / 8];
>> +                               u32 idx = (page_height * i + k) * line_length + j / 8;
>
> Nit: I would use unsigned int for idx, as that matches all other
> variables involved.
> But given array_idx is u32, too, u32 may makes sense.
>

Yes, this function logic is mostly based on ssd1307fb_update_rect() from
drivers/video/fbdev/ssd1307fb.c and that is from where the u32 array_idx
comes from.

As you said, I used u32 for idx to be consistent with that variable type.

>> +                               u8 byte = buf[idx];
>>                                 u8 bit = (byte >> (j % 8)) & 1;
>>
>>                                 data |= bit << k;
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>

Thanks!
diff mbox series

Patch

diff --git a/drivers/gpu/drm/solomon/ssd13xx.c b/drivers/gpu/drm/solomon/ssd13xx.c
index 10a767fb614c..d29be17665b5 100644
--- a/drivers/gpu/drm/solomon/ssd13xx.c
+++ b/drivers/gpu/drm/solomon/ssd13xx.c
@@ -42,6 +42,8 @@ 
 #define DRIVER_MAJOR	1
 #define DRIVER_MINOR	0
 
+#define SSD130X_PAGE_HEIGHT 8
+
 #define SSD130X_PAGE_COL_START_LOW		0x00
 #define SSD130X_PAGE_COL_START_HIGH		0x10
 #define SSD130X_SET_ADDRESS_MODE		0x20
@@ -102,7 +104,6 @@  const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
 		.default_width = 132,
 		.default_height = 64,
 		.page_mode_only = 1,
-		.page_height = 8,
 	},
 	[SSD1305_ID] = {
 		.default_vcomh = 0x34,
@@ -110,7 +111,6 @@  const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
 		.default_dclk_frq = 7,
 		.default_width = 132,
 		.default_height = 64,
-		.page_height = 8,
 	},
 	[SSD1306_ID] = {
 		.default_vcomh = 0x20,
@@ -119,7 +119,6 @@  const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
 		.need_chargepump = 1,
 		.default_width = 128,
 		.default_height = 64,
-		.page_height = 8,
 	},
 	[SSD1307_ID] = {
 		.default_vcomh = 0x20,
@@ -128,7 +127,6 @@  const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
 		.need_pwm = 1,
 		.default_width = 128,
 		.default_height = 39,
-		.page_height = 8,
 	},
 	[SSD1309_ID] = {
 		.default_vcomh = 0x34,
@@ -136,7 +134,6 @@  const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
 		.default_dclk_frq = 10,
 		.default_width = 128,
 		.default_height = 64,
-		.page_height = 8,
 	}
 };
 EXPORT_SYMBOL_NS_GPL(ssd13xx_variants, DRM_SSD13XX);
@@ -465,13 +462,13 @@  static int ssd13xx_update_rect(struct ssd13xx_device *ssd13xx,
 	unsigned int width = drm_rect_width(rect);
 	unsigned int height = drm_rect_height(rect);
 	unsigned int line_length = DIV_ROUND_UP(width, 8);
-	unsigned int page_height = ssd13xx->device_info->page_height;
+	unsigned int page_height = SSD130X_PAGE_HEIGHT;
 	unsigned int pages = DIV_ROUND_UP(height, page_height);
 	struct drm_device *drm = &ssd13xx->drm;
 	u32 array_idx = 0;
 	int ret, i, j, k;
 
-	drm_WARN_ONCE(drm, y % 8 != 0, "y must be aligned to screen page\n");
+	drm_WARN_ONCE(drm, y % page_height != 0, "y must be aligned to screen page\n");
 
 	/*
 	 * The screen is divided in pages, each having a height of 8
@@ -503,27 +500,32 @@  static int ssd13xx_update_rect(struct ssd13xx_device *ssd13xx,
 	 */
 
 	if (!ssd13xx->page_address_mode) {
+		u8 page_start;
+
 		/* Set address range for horizontal addressing mode */
 		ret = ssd13xx_set_col_range(ssd13xx, ssd13xx->col_offset + x, width);
 		if (ret < 0)
 			return ret;
 
-		ret = ssd13xx_set_page_range(ssd13xx, ssd13xx->page_offset + y / 8, pages);
+		page_start = ssd13xx->page_offset + y / page_height;
+		ret = ssd13xx_set_page_range(ssd13xx, page_start, pages);
 		if (ret < 0)
 			return ret;
 	}
 
 	for (i = 0; i < pages; i++) {
-		int m = 8;
+		int m = page_height;
 
 		/* Last page may be partial */
-		if (8 * (y / 8 + i + 1) > ssd13xx->height)
-			m = ssd13xx->height % 8;
+		if (page_height * (y / page_height + i + 1) > ssd13xx->height)
+			m = ssd13xx->height % page_height;
+
 		for (j = 0; j < width; j++) {
 			u8 data = 0;
 
 			for (k = 0; k < m; k++) {
-				u8 byte = buf[(8 * i + k) * line_length + j / 8];
+				u32 idx = (page_height * i + k) * line_length + j / 8;
+				u8 byte = buf[idx];
 				u8 bit = (byte >> (j % 8)) & 1;
 
 				data |= bit << k;
@@ -559,8 +561,7 @@  static int ssd13xx_update_rect(struct ssd13xx_device *ssd13xx,
 
 static void ssd13xx_clear_screen(struct ssd13xx_device *ssd13xx, u8 *data_array)
 {
-	unsigned int page_height = ssd13xx->device_info->page_height;
-	unsigned int pages = DIV_ROUND_UP(ssd13xx->height, page_height);
+	unsigned int pages = DIV_ROUND_UP(ssd13xx->height, SSD130X_PAGE_HEIGHT);
 	unsigned int width = ssd13xx->width;
 	int ret, i;
 
@@ -605,14 +606,13 @@  static int ssd13xx_fb_blit_rect(struct drm_framebuffer *fb,
 				u8 *buf, u8 *data_array)
 {
 	struct ssd13xx_device *ssd13xx = drm_to_ssd13xx(fb->dev);
-	unsigned int page_height = ssd13xx->device_info->page_height;
 	struct iosys_map dst;
 	unsigned int dst_pitch;
 	int ret = 0;
 
 	/* Align y to display page boundaries */
-	rect->y1 = round_down(rect->y1, page_height);
-	rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd13xx->height);
+	rect->y1 = round_down(rect->y1, SSD130X_PAGE_HEIGHT);
+	rect->y2 = min_t(unsigned int, round_up(rect->y2, SSD130X_PAGE_HEIGHT), ssd13xx->height);
 
 	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
 
@@ -814,8 +814,7 @@  static int ssd13xx_crtc_atomic_check(struct drm_crtc *crtc,
 	struct ssd13xx_device *ssd13xx = drm_to_ssd13xx(drm);
 	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	struct ssd13xx_crtc_state *ssd13xx_state = to_ssd13xx_crtc_state(crtc_state);
-	unsigned int page_height = ssd13xx->device_info->page_height;
-	unsigned int pages = DIV_ROUND_UP(ssd13xx->height, page_height);
+	unsigned int pages = DIV_ROUND_UP(ssd13xx->height, SSD130X_PAGE_HEIGHT);
 	int ret;
 
 	ret = drm_crtc_helper_atomic_check(crtc, state);
diff --git a/drivers/gpu/drm/solomon/ssd13xx.h b/drivers/gpu/drm/solomon/ssd13xx.h
index e5abf23196b0..64283935fbc1 100644
--- a/drivers/gpu/drm/solomon/ssd13xx.h
+++ b/drivers/gpu/drm/solomon/ssd13xx.h
@@ -39,7 +39,6 @@  struct ssd13xx_deviceinfo {
 	u32 default_dclk_frq;
 	u32 default_width;
 	u32 default_height;
-	u32 page_height;
 	bool need_pwm;
 	bool need_chargepump;
 	bool page_mode_only;