[PATCH v4 3/3] drm/panel-fannal-c3004: Add fannal c3004 DSI panel

[Paulo Pavacic]

Hello Linus,

thank you for the comments.

čet, 15. lip 2023. u 21:55 Linus Walleij <linus.walleij at linaro.org> napisao je:
>
> Hi Paulo,
>
> thanks for your patch!
>
> Overall this looks very good.
>
> I doubt that the display controller is actually by Fannal, but I guess
> you tried to find out? We usually try to identify the underlying display
> controller so the driver can be named after it and reused for more
> display panels.

Yes, of course, the controller is ST7701S.

>
> Some minor questions/nitpicks below.
>
> On Wed, Jun 7, 2023 at 5:11 PM Paulo Pavacic <pavacic.p at gmail.com> wrote:
>
> > +static int fannal_panel_enable(struct drm_panel *panel)
> > +{
> > +       struct mipi_dsi_device *dsi = to_mipi_dsi_device(panel->dev);
> > +
> > +       mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x13);
> > +       mipi_dsi_generic_write_seq(dsi, 0xEF, 0x08);
>
> Why is everything using mipi_dsi_generic_write_seq() instead of
> mipi_dsi_dcs_write_seq()?

Okay, I will replace it.

>
> Especially here, because 0x11 is certainly a command:
>
> > +       mipi_dsi_generic_write_seq(dsi, 0x11); //MIPI_DCS_EXIT_SLEEP_MODE
>
> Instead use:
>
>     ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>     if (ret)
>             return ret;
>
>
> > +       mipi_dsi_generic_write_seq(dsi, 0x29); //MIPI_DCS_SET_DISPLAY_ON
>
> Instead use:
>
>     ret = mipi_dsi_dcs_set_display_on(dsi);
>     if (ret)
>             return ret;
>

Okay I will replace all commands with a functions found here:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_mipi_dsi.c#L995

> Yours,
> Linus Walleij

Best regards,
Paulo