[EXT] Re: [PATCH v2 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Robert Chiras]

Hi Sam,

On Mi, 2019-06-19 at 15:25 +0200, Sam Ravnborg wrote:
> On Tue, Jun 18, 2019 at 04:30:46PM +0300, Robert Chiras wrote:
> > 
> > This patch adds Raydium RM67191 TFT LCD panel driver (MIPI-DSI
> > protocol).
> > 
> > Signed-off-by: Robert Chiras <robert.chiras at nxp.com>
> Please include in the changelog a list of what was updated - like
> this:
> 
> v2:
> - added kconif symbol sorted (sam)
> - another nitpick (foo)
> - etc
> 
> In general try to namme who gave feedback to give them some credit.
Thanks. I will keep this in mind, next time
> 
> Who is maintainer for this onwards?
I can offer myself to maintain this.
> 
> > 
> > ---
> >  drivers/gpu/drm/panel/Kconfig                 |   9 +
> >  drivers/gpu/drm/panel/Makefile                |   1 +
> >  drivers/gpu/drm/panel/panel-raydium-rm67191.c | 709
> > ++++++++++++++++++++++++++
> >  3 files changed, 719 insertions(+)
> >  create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
> > 
> > +static int rad_panel_prepare(struct drm_panel *panel)
> > +{
> > +     struct rad_panel *rad = to_rad_panel(panel);
> > +
> > +     if (rad->prepared)
> > +             return 0;
> > +
> > +     if (rad->reset) {
> > +             gpiod_set_value_cansleep(rad->reset, 1);
> > +             usleep_range(3000, 5000);
> > +             gpiod_set_value_cansleep(rad->reset, 0);
> So writing a 0 will release reset.
> > 
> > +             usleep_range(18000, 20000);
> > +     }
> > +
> > +     rad->prepared = true;
> > +
> > +     return 0;
> > +}
> > +
> > +static int rad_panel_unprepare(struct drm_panel *panel)
> > +{
> > +     struct rad_panel *rad = to_rad_panel(panel);
> > +
> > +     if (!rad->prepared)
> > +             return 0;
> > +
> > +     if (rad->reset) {
> > +             gpiod_set_value_cansleep(rad->reset, 1);
> > +             usleep_range(15000, 17000);
> > +             gpiod_set_value_cansleep(rad->reset, 0);
> Looks strange that reset is released in unprepare.
> I would expect it to stay reset to minimize power etc.
Yes, it looks strange indeed. The reason here is coming from the fact
that this panel also has touch-screen that shares this reset pin with
the OLED display. While the display may be turned off, the touch driver
may need to keep the connection active with the touch controller from
this panel. This is the reason for releasing the reset pin in
unprepare. I will add this in a comment in the code, to eliminate the
confusion.
> 
> > 
> > +
> > +     ret = drm_display_info_set_bus_formats(&connector-
> > >display_info,
> > +                                            rad_bus_formats,
> > +                                            ARRAY_SIZE(rad_bus_for
> > mats));
> Other drivers has this as the last stement in their enable function.
> I did not check why, but maybe something to invest a few minutes
> into.
> Be different only if there is a reason to do so.
Ok, I will move this as the last statement. I also noticed that the
other panel drivers do not check the return of this call, like I do
here, so I will also remove this too.
> 
> > 
> > +     if (ret)
> > +             return ret;
> > +
> > +     drm_mode_probed_add(panel->connector, mode);
> > +
> > +     return 1;
> > +}
> > +
> > +
> > +#ifdef CONFIG_PM
> > +static int rad_panel_suspend(struct device *dev)
> > +{
> > +     struct rad_panel *rad = dev_get_drvdata(dev);
> > +
> > +     if (!rad->reset)
> > +             return 0;
> > +
> > +     devm_gpiod_put(dev, rad->reset);
> > +     rad->reset = NULL;
> > +
> > +     return 0;
> > +}
> > +
> > +static int rad_panel_resume(struct device *dev)
> > +{
> > +     struct rad_panel *rad = dev_get_drvdata(dev);
> > +
> > +     if (rad->reset)
> > +             return 0;
> > +
> > +     rad->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > +     if (IS_ERR(rad->reset))
> > +             rad->reset = NULL;
> > +
> > +     return PTR_ERR_OR_ZERO(rad->reset);
> > +}
> > +
> > +#endif
> Use __maybe_unused for the tw functions above.
> And loose the ifdef...
Thanks. Will apply.
> 
> > 
> > +
> > +static const struct dev_pm_ops rad_pm_ops = {
> > +     SET_RUNTIME_PM_OPS(rad_panel_suspend, rad_panel_resume, NULL)
> > +     SET_SYSTEM_SLEEP_PM_OPS(rad_panel_suspend, rad_panel_resume)
> > +};
> > +
> > +static const struct of_device_id rad_of_match[] = {
> > +     { .compatible = "raydium,rm67191", },
> > +     { }
> We often, but not always, write this as { /* sentinal */ },
Thanks. Will apply.
> 
> > 
> > +};
> > +MODULE_DEVICE_TABLE(of, rad_of_match);
> > +
> > +static struct mipi_dsi_driver rad_panel_driver = {
> > +     .driver = {
> > +             .name = "panel-raydium-rm67191",
> > +             .of_match_table = rad_of_match,
> > +             .pm     = &rad_pm_ops,
> > +     },
> > +     .probe = rad_panel_probe,
> > +     .remove = rad_panel_remove,
> > +     .shutdown = rad_panel_shutdown,
> > +};
> > +module_mipi_dsi_driver(rad_panel_driver);
> > +
> > +MODULE_AUTHOR("Robert Chiras <robert.chiras at nxp.com>");
> > +MODULE_DESCRIPTION("DRM Driver for Raydium RM67191 MIPI DSI
> > panel");
> > +MODULE_LICENSE("GPL v2");
> With the above trivialities considered/fixed:
> Reviewed-by: Sam Ravnborg <sam at ravnborg.org>
> 
>         Sam