Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Wed, 11 Oct 2023 10:09:50 +0200 | Subject | Re: [PATCH 5/8] drm/ssd13xx: Add a per controller family functions table |
| |
Hi Javier,
On Mon, Oct 9, 2023 at 8:36 PM Javier Martinez Canillas <javierm@redhat.com> 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@redhat.com>
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@linux-m68k.org
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
| |