Re: [PATCH v3 3/4] drm/panel: ili9882t: Break out as separate driver

[Doug Anderson]

Hi,

On Mon, Jul 3, 2023 at 6:22 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> The Starry ILI9882t-based panel should never have been part of the boe
> tv101wum driver, it is clearly based on the Ilitek ILI9882t display
> controller and if you look at the custom command sequences for the
> panel these clearly contain the signature Ilitek page switch (0xff)
> commands. The hardware has nothing in common with the other panels
> supported by this driver.
>
> Break this out into a separate driver and config symbol instead.
>
> If the placement here is out of convenience for using similar code,
> we should consider creating a helper library instead.
>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/panel/Kconfig                  |   9 +
>  drivers/gpu/drm/panel/Makefile                 |   1 +
>  drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 386 -------------
>  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c  | 739 +++++++++++++++++++++++++
>  4 files changed, 749 insertions(+), 386 deletions(-)

I have no real objection here and am happy to let others argue about
bikeshed color. I think the "panel-boe-tv101wum-nl6.c" driver ended up
becoming a dumping ground for a bunch of panels in response to Sam's
feedback originally [1].

[1] https://lore.kernel.org/all/YSPAseE6WD8dDRuz@xxxxxxxxxxxx/

...so it would be good to get Sam's feedback here.


> +/*
> + * Use this descriptor struct to describe different panels using the
> + * Ilitek ILI9882T display controller.
> + */
> +struct panel_desc {
> +       const struct drm_display_mode *modes;
> +       unsigned int bpc;
> +
> +       /**
> +        * @width_mm: width of the panel's active display area
> +        * @height_mm: height of the panel's active display area
> +        */
> +       struct {
> +               unsigned int width_mm;
> +               unsigned int height_mm;
> +       } size;
> +
> +       unsigned long mode_flags;
> +       enum mipi_dsi_pixel_format format;
> +       int (*init)(struct mipi_dsi_device *dsi);
> +       unsigned int lanes;
> +       bool discharge_on_disable;

IMO "discharge_on_disable" should be removed since the one panel
supported by this driver doesn't use it. If later we find that some
ili9882t panels need this logic then we can add it back in, but it
seems hard to believe it would use the same code.


> +       bool lp11_before_reset;

IMO "lp11_before_reset" should be removed. The one panel supported by
this driver _always_ needs lp11_before_reset. If we later find that
some ili9882t panels want different behavior then we can add it back
in. It doesn't feel like the kind of thing that would be different on
different drivers using the same chip.


> +static int ili9882t_get_modes(struct drm_panel *panel,
> +                              struct drm_connector *connector)
> +{
> +       struct ili9882t *ili = to_ili9882t(panel);
> +       const struct drm_display_mode *m = ili->desc->modes;
> +       struct drm_display_mode *mode;
> +
> +       mode = drm_mode_duplicate(connector->dev, m);
> +       if (!mode) {
> +               dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> +                       m->hdisplay, m->vdisplay, drm_mode_vrefresh(m));
> +               return -ENOMEM;
> +       }
> +
> +       mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +       drm_mode_set_name(mode);
> +       drm_mode_probed_add(connector, mode);
> +
> +       connector->display_info.width_mm = ili->desc->size.width_mm;
> +       connector->display_info.height_mm = ili->desc->size.height_mm;
> +       connector->display_info.bpc = ili->desc->bpc;
> +       /*
> +        * TODO: Remove once all drm drivers call
> +        * drm_connector_set_orientation_from_panel()
> +        */
> +       drm_connector_set_panel_orientation(connector, ili->orientation);

I'd be inclined to take the above call out and assume anyone using
this new panel has a DRM driver that's working properly...


-Doug