To allow the driver to decouple the common logic in the function callbacks
from the behaviour that is specific of a given Solomon controller family.
For this, include a chip family to the device info and add fields to the
driver-private device struct, to store the hardware buffer maximum size,
the intermediate buffer maximum size and pixel format, and a set of per
chip family function handlers.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/gpu/drm/solomon/ssd13xx.c | 88 ++++++++++++++++++++++---------
drivers/gpu/drm/solomon/ssd13xx.h | 27 ++++++++++
2 files changed, 90 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd13xx.c b/drivers/gpu/drm/solomon/ssd13xx.c
index 9747f8656636..5a426ac10c58 100644
--- a/drivers/gpu/drm/solomon/ssd13xx.c
+++ b/drivers/gpu/drm/solomon/ssd13xx.c
@@ -104,6 +104,7 @@ const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
.default_width = 132,
.default_height = 64,
.page_mode_only = 1,
+ .family_id = SSD130X_FAMILY,
},
[SSD1305_ID] = {
.default_vcomh = 0x34,
@@ -111,6 +112,7 @@ const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
.default_dclk_frq = 7,
.default_width = 132,
.default_height = 64,
+ .family_id = SSD130X_FAMILY,
},
[SSD1306_ID] = {
.default_vcomh = 0x20,
@@ -119,6 +121,7 @@ const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
.need_chargepump = 1,
.default_width = 128,
.default_height = 64,
+ .family_id = SSD130X_FAMILY,
},
[SSD1307_ID] = {
.default_vcomh = 0x20,
@@ -127,6 +130,7 @@ const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
.need_pwm = 1,
.default_width = 128,
.default_height = 39,
+ .family_id = SSD130X_FAMILY,
},
[SSD1309_ID] = {
.default_vcomh = 0x34,
@@ -134,6 +138,7 @@ const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
.default_dclk_frq = 10,
.default_width = 128,
.default_height = 64,
+ .family_id = SSD130X_FAMILY,
}
};
EXPORT_SYMBOL_NS_GPL(ssd13xx_variants, DRM_SSD13XX);
@@ -148,8 +153,6 @@ struct ssd13xx_plane_state {
struct drm_shadow_plane_state base;
/* Intermediate buffer to convert pixels from XRGB8888 to HW format */
u8 *buffer;
- /* Pixel format info for the intermediate buffer */
- const struct drm_format_info *fi;
};
static inline struct ssd13xx_crtc_state *to_ssd13xx_crtc_state(struct drm_crtc_state *state)
@@ -331,7 +334,7 @@ static void ssd13xx_power_off(struct ssd13xx_device *ssd13xx)
regulator_disable(ssd13xx->vcc_reg);
}
-static int ssd13xx_init(struct ssd13xx_device *ssd13xx)
+static int ssd130x_init(struct ssd13xx_device *ssd13xx)
{
u32 precharge, dclk, com_invdir, compins, chargepump, seg_remap;
bool scan_mode;
@@ -455,7 +458,7 @@ static int ssd13xx_init(struct ssd13xx_device *ssd13xx)
SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
}
-static int ssd13xx_update_rect(struct ssd13xx_device *ssd13xx,
+static int ssd130x_update_rect(struct ssd13xx_device *ssd13xx,
struct drm_rect *rect, u8 *buf,
u8 *data_array)
{
@@ -561,7 +564,7 @@ static int ssd13xx_update_rect(struct ssd13xx_device *ssd13xx,
return ret;
}
-static void ssd13xx_clear_screen(struct ssd13xx_device *ssd13xx, u8 *data_array)
+static void ssd130x_clear_screen(struct ssd13xx_device *ssd13xx, u8 *data_array)
{
unsigned int pages = DIV_ROUND_UP(ssd13xx->height, SSD130X_PAGE_HEIGHT);
unsigned int width = ssd13xx->width;
@@ -602,6 +605,15 @@ static void ssd13xx_clear_screen(struct ssd13xx_device *ssd13xx, u8 *data_array)
}
}
+static const struct ssd13xx_funcs ssd13xx_family_funcs[] = {
+ [SSD130X_FAMILY] = {
+ .init = ssd130x_init,
+ .update_rect = ssd130x_update_rect,
+ .clear_screen = ssd130x_clear_screen,
+ .fmt_convert = drm_fb_xrgb8888_to_mono,
+ },
+};
+
static int ssd13xx_fb_blit_rect(struct drm_framebuffer *fb,
const struct iosys_map *vmap,
struct drm_rect *rect, u8 *buf,
@@ -609,6 +621,7 @@ static int ssd13xx_fb_blit_rect(struct drm_framebuffer *fb,
u8 *data_array)
{
struct ssd13xx_device *ssd13xx = drm_to_ssd13xx(fb->dev);
+ const struct ssd13xx_funcs *ssd13xx_funcs = ssd13xx->funcs;
struct iosys_map dst;
unsigned int dst_pitch;
int ret = 0;
@@ -624,13 +637,11 @@ static int ssd13xx_fb_blit_rect(struct drm_framebuffer *fb,
return ret;
iosys_map_set_vaddr(&dst, buf);
- drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
+ ssd13xx_funcs->fmt_convert(&dst, &dst_pitch, vmap, fb, rect);
drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
- ssd13xx_update_rect(ssd13xx, rect, buf, data_array);
-
- return ret;
+ return ssd13xx_funcs->update_rect(ssd13xx, rect, buf, data_array);
}
static int ssd13xx_primary_plane_atomic_check(struct drm_plane *plane,
@@ -642,8 +653,6 @@ static int ssd13xx_primary_plane_atomic_check(struct drm_plane *plane,
struct ssd13xx_plane_state *ssd13xx_state = to_ssd13xx_plane_state(plane_state);
struct drm_crtc *crtc = plane_state->crtc;
struct drm_crtc_state *crtc_state;
- const struct drm_format_info *fi;
- unsigned int pitch;
int ret;
if (!crtc)
@@ -657,18 +666,10 @@ static int ssd13xx_primary_plane_atomic_check(struct drm_plane *plane,
if (ret)
return ret;
- fi = drm_format_info(DRM_FORMAT_R1);
- if (!fi)
- return -EINVAL;
-
- pitch = drm_format_info_min_pitch(fi, 0, ssd13xx->width);
-
- ssd13xx_state->buffer = kcalloc(pitch, ssd13xx->height, GFP_KERNEL);
+ ssd13xx_state->buffer = kzalloc(ssd13xx->buffer_size, GFP_KERNEL);
if (!ssd13xx_state->buffer)
return -ENOMEM;
- ssd13xx_state->fi = fi;
-
return 0;
}
@@ -684,6 +685,7 @@ static void ssd13xx_primary_plane_atomic_update(struct drm_plane *plane,
struct drm_framebuffer *fb = plane_state->fb;
struct drm_atomic_helper_damage_iter iter;
struct drm_device *drm = plane->dev;
+ struct ssd13xx_device *ssd13xx = drm_to_ssd13xx(drm);
struct drm_rect dst_clip;
struct drm_rect damage;
int idx;
@@ -700,7 +702,7 @@ static void ssd13xx_primary_plane_atomic_update(struct drm_plane *plane,
ssd13xx_fb_blit_rect(fb, &shadow_plane_state->data[0], &dst_clip,
ssd13xx_plane_state->buffer,
- ssd13xx_plane_state->fi,
+ ssd13xx->buffer_fi,
ssd13xx_crtc_state->data_array);
}
@@ -712,6 +714,7 @@ static void ssd13xx_primary_plane_atomic_disable(struct drm_plane *plane,
{
struct drm_device *drm = plane->dev;
struct ssd13xx_device *ssd13xx = drm_to_ssd13xx(drm);
+ const struct ssd13xx_funcs *ssd13xx_funcs = ssd13xx->funcs;
struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
struct drm_crtc_state *crtc_state;
struct ssd13xx_crtc_state *ssd13xx_crtc_state;
@@ -726,7 +729,7 @@ static void ssd13xx_primary_plane_atomic_disable(struct drm_plane *plane,
if (!drm_dev_enter(drm, &idx))
return;
- ssd13xx_clear_screen(ssd13xx, ssd13xx_crtc_state->data_array);
+ ssd13xx_funcs->clear_screen(ssd13xx, ssd13xx_crtc_state->data_array);
drm_dev_exit(idx);
}
@@ -820,14 +823,13 @@ 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 pages = DIV_ROUND_UP(ssd13xx->height, SSD130X_PAGE_HEIGHT);
int ret;
ret = drm_crtc_helper_atomic_check(crtc, state);
if (ret)
return ret;
- ssd13xx_state->data_array = kmalloc(ssd13xx->width * pages, GFP_KERNEL);
+ ssd13xx_state->data_array = kmalloc(ssd13xx->data_array_size, GFP_KERNEL);
if (!ssd13xx_state->data_array)
return -ENOMEM;
@@ -905,13 +907,14 @@ static void ssd13xx_encoder_atomic_enable(struct drm_encoder *encoder,
{
struct drm_device *drm = encoder->dev;
struct ssd13xx_device *ssd13xx = drm_to_ssd13xx(drm);
+ const struct ssd13xx_funcs *ssd13xx_funcs = ssd13xx->funcs;
int ret;
ret = ssd13xx_power_on(ssd13xx);
if (ret)
return;
- ret = ssd13xx_init(ssd13xx);
+ ret = ssd13xx_funcs->init(ssd13xx);
if (ret)
goto power_off;
@@ -1195,11 +1198,38 @@ static int ssd13xx_get_resources(struct ssd13xx_device *ssd13xx)
return 0;
}
+static int ssd13xx_set_buffer_sizes(struct ssd13xx_device *ssd13xx,
+ enum ssd13xx_family_ids family_id)
+{
+ const struct drm_format_info *fi;
+ unsigned int buffer_pitch;
+
+ switch (family_id) {
+ case SSD130X_FAMILY:
+ unsigned int pages = DIV_ROUND_UP(ssd13xx->height, SSD130X_PAGE_HEIGHT);
+
+ ssd13xx->data_array_size = ssd13xx->width * pages;
+
+ fi = drm_format_info(DRM_FORMAT_R1);
+ break;
+ }
+
+ if (!fi)
+ return -EINVAL;
+
+ buffer_pitch = drm_format_info_min_pitch(fi, 0, ssd13xx->width);
+ ssd13xx->buffer_size = buffer_pitch * ssd13xx->height;
+ ssd13xx->buffer_fi = fi;
+
+ return 0;
+}
+
struct ssd13xx_device *ssd13xx_probe(struct device *dev, struct regmap *regmap)
{
struct ssd13xx_device *ssd13xx;
struct backlight_device *bl;
struct drm_device *drm;
+ enum ssd13xx_family_ids family_id;
int ret;
ssd13xx = devm_drm_dev_alloc(dev, &ssd13xx_drm_driver,
@@ -1214,11 +1244,19 @@ struct ssd13xx_device *ssd13xx_probe(struct device *dev, struct regmap *regmap)
ssd13xx->regmap = regmap;
ssd13xx->device_info = device_get_match_data(dev);
+ family_id = ssd13xx->device_info->family_id;
+
+ ssd13xx->funcs = &ssd13xx_family_funcs[family_id];
+
if (ssd13xx->device_info->page_mode_only)
ssd13xx->page_address_mode = 1;
ssd13xx_parse_properties(ssd13xx);
+ ret = ssd13xx_set_buffer_sizes(ssd13xx, family_id);
+ if (ret)
+ return ERR_PTR(ret);
+
ret = ssd13xx_get_resources(ssd13xx);
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/solomon/ssd13xx.h b/drivers/gpu/drm/solomon/ssd13xx.h
index 64283935fbc1..e78d5ab87474 100644
--- a/drivers/gpu/drm/solomon/ssd13xx.h
+++ b/drivers/gpu/drm/solomon/ssd13xx.h
@@ -20,11 +20,17 @@
#include <drm/drm_plane_helper.h>
#include <linux/regmap.h>
+#include <linux/iosys-map.h>
#define SSD130X_DATA 0x40
#define SSD130X_COMMAND 0x80
+enum ssd13xx_family_ids {
+ SSD130X_FAMILY,
+};
+
enum ssd13xx_variants {
+ /* ssd130x family */
SH1106_ID,
SSD1305_ID,
SSD1306_ID,
@@ -39,6 +45,7 @@ struct ssd13xx_deviceinfo {
u32 default_dclk_frq;
u32 default_width;
u32 default_height;
+ enum ssd13xx_family_ids family_id;
bool need_pwm;
bool need_chargepump;
bool page_mode_only;
@@ -76,6 +83,12 @@ struct ssd13xx_device {
u32 col_offset;
u32 prechargep1;
u32 prechargep2;
+ /* HW format buffer size */
+ u32 data_array_size;
+ /* Intermediate buffer size */
+ u32 buffer_size;
+ /* Pixel format info for the intermediate buffer */
+ const struct drm_format_info *buffer_fi;
struct backlight_device *bl_dev;
struct pwm_device *pwm;
@@ -88,6 +101,20 @@ struct ssd13xx_device {
u8 col_end;
u8 page_start;
u8 page_end;
+
+ /* Chip family specific operations */
+ const struct ssd13xx_funcs *funcs;
+};
+
+struct ssd13xx_funcs {
+ int (*init)(struct ssd13xx_device *ssd130x);
+ int (*update_rect)(struct ssd13xx_device *ssd13xx, struct drm_rect *rect,
+ u8 *buf, u8 *data_array);
+ void (*clear_screen)(struct ssd13xx_device *ssd13xx,
+ u8 *data_array);
+ void (*fmt_convert)(struct iosys_map *dst, const unsigned int *dst_pitch,
+ const struct iosys_map *src, const struct drm_framebuffer *fb,
+ const struct drm_rect *clip);
};
extern const struct ssd13xx_deviceinfo ssd13xx_variants[];
--
2.41.0
Hi Javier,
On Mon, Oct 9, 2023 at 8:36 PM Javier Martinez Canillas
<[email protected]> wrote:
> To allow the driver to decouple the common logic in the function callbacks
> from the behaviour that is specific of a given Solomon controller family.
>
> For this, include a chip family to the device info and add fields to the
> driver-private device struct, to store the hardware buffer maximum size,
> the intermediate buffer maximum size and pixel format, and a set of per
> chip family function handlers.
>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
Thanks for your patch!
> --- a/drivers/gpu/drm/solomon/ssd13xx.c
> +++ b/drivers/gpu/drm/solomon/ssd13xx.c
> @@ -104,6 +104,7 @@ const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
> .default_width = 132,
> .default_height = 64,
> .page_mode_only = 1,
> + .family_id = SSD130X_FAMILY,
Why not store &ssd13xx_family_funcs[SSD130X_FAMILY]?
> },
> [SSD1305_ID] = {
> .default_vcomh = 0x34,
> @@ -602,6 +605,15 @@ static void ssd13xx_clear_screen(struct ssd13xx_device *ssd13xx, u8 *data_array)
> }
> }
>
> +static const struct ssd13xx_funcs ssd13xx_family_funcs[] = {
> + [SSD130X_FAMILY] = {
> + .init = ssd130x_init,
> + .update_rect = ssd130x_update_rect,
> + .clear_screen = ssd130x_clear_screen,
> + .fmt_convert = drm_fb_xrgb8888_to_mono,
> + },
> +};
> +
> static int ssd13xx_fb_blit_rect(struct drm_framebuffer *fb,
> const struct iosys_map *vmap,
> struct drm_rect *rect, u8 *buf,
> @@ -1195,11 +1198,38 @@ static int ssd13xx_get_resources(struct ssd13xx_device *ssd13xx)
> return 0;
> }
>
> +static int ssd13xx_set_buffer_sizes(struct ssd13xx_device *ssd13xx,
> + enum ssd13xx_family_ids family_id)
> +{
> + const struct drm_format_info *fi;
> + unsigned int buffer_pitch;
> +
> + switch (family_id) {
> + case SSD130X_FAMILY:
> + unsigned int pages = DIV_ROUND_UP(ssd13xx->height, SSD130X_PAGE_HEIGHT);
> +
> + ssd13xx->data_array_size = ssd13xx->width * pages;
> +
> + fi = drm_format_info(DRM_FORMAT_R1);
> + break;
> + }
Please don't mix ssd13xx_funcs[family_id] and switch (family_id).
The switch() could be replaced by an extra function pointer in
ssd13xx_funcs, and by storing fi in ssd13xx_funcs, too.
Note that you don't really need the full drm_format_info, the number
of bits per pixel is sufficient. But having the drm_format_info is
nice, as fmt_convert() will need it later when adding support for
fbdev emulation using R1 or R4 ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Geert Uytterhoeven <[email protected]> writes:
> Hi Javier,
>
> On Mon, Oct 9, 2023 at 8:36 PM Javier Martinez Canillas
> <[email protected]> wrote:
>> To allow the driver to decouple the common logic in the function callbacks
>> from the behaviour that is specific of a given Solomon controller family.
>>
>> For this, include a chip family to the device info and add fields to the
>> driver-private device struct, to store the hardware buffer maximum size,
>> the intermediate buffer maximum size and pixel format, and a set of per
>> chip family function handlers.
>>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>
> Thanks for your patch!
>
>> --- a/drivers/gpu/drm/solomon/ssd13xx.c
>> +++ b/drivers/gpu/drm/solomon/ssd13xx.c
>> @@ -104,6 +104,7 @@ const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
>> .default_width = 132,
>> .default_height = 64,
>> .page_mode_only = 1,
>> + .family_id = SSD130X_FAMILY,
>
> Why not store &ssd13xx_family_funcs[SSD130X_FAMILY]?
>
I could do that, yeah. Originally thought that could be useful besides the
per chip functions, but I don't think that it's used for anything else...
[...]
>> + switch (family_id) {
>> + case SSD130X_FAMILY:
>> + unsigned int pages = DIV_ROUND_UP(ssd13xx->height, SSD130X_PAGE_HEIGHT);
>> +
>> + ssd13xx->data_array_size = ssd13xx->width * pages;
>> +
>> + fi = drm_format_info(DRM_FORMAT_R1);
>> + break;
>> + }
>
> Please don't mix ssd13xx_funcs[family_id] and switch (family_id).
> The switch() could be replaced by an extra function pointer in
> ssd13xx_funcs, and by storing fi in ssd13xx_funcs, too.
>
Yes, I didn't want to store the format info in struct ssd13xx_funcs
because the idea was for that struct to only have chip operations.
But could do that. I wonder if should rename that struct thought? to
something that better denotes is more than function handlers.
> Note that you don't really need the full drm_format_info, the number
> of bits per pixel is sufficient. But having the drm_format_info is
> nice, as fmt_convert() will need it later when adding support for
> fbdev emulation using R1 or R4 ;-)
>
Exactly, thinking in your patches is why I stored the full format info :)
> Gr{oetje,eeting}s,
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat