Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> writes: > 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]? > 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