Hi Javier,
On Mon, Oct 9, 2023 at 8:36 PM Javier Martinez Canillas
<javierm@xxxxxxxxxx> 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 <javierm@xxxxxxxxxx>
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 -- geert@xxxxxxxxxxxxxx
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
