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

[Robert Chiras]

Hi Fabio,

Thanks for your feedback. I will handle them all, but for the pm_ops I
have some comments. See inline.

On Vi, 2019-06-21 at 10:59 -0300, Fabio Estevam wrote:
> Hi Robert,
> 
> On Thu, Jun 20, 2019 at 10:31 AM Robert Chiras <robert.chiras@xxxxxxx
> > wrote:
> 
> > 
> > +fail:
> > +ÂÂÂÂÂÂÂif (rad->reset)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value_cansleep(rad->reset, 1);
> gpiod_set_value_cansleep() can handle NULL, so no need for the if()
> check.
> 
> > 
> > +static const struct display_timing rad_default_timing = {
> > +ÂÂÂÂÂÂÂ.pixelclock = { 132000000, 132000000, 132000000 },
> Having the same information listed three times does not seem useful.
> 
> You could use a drm_display_mode structure with a single entry
> instead.
> 
> > 
> > +ÂÂÂÂÂÂÂvideomode_from_timing(&rad_default_timing, &panel->vm);
> > +
> > +ÂÂÂÂÂÂÂof_property_read_u32(np, "width-mm", &panel->width_mm);
> > +ÂÂÂÂÂÂÂof_property_read_u32(np, "height-mm", &panel->height_mm);
> > +
> > +ÂÂÂÂÂÂÂpanel->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> Since this is optional it would be better to use
> devm_gpiod_get_optional() instead.
> 
> 
> > 
> > +
> > +ÂÂÂÂÂÂÂif (IS_ERR(panel->reset))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpanel->reset = NULL;
> > +ÂÂÂÂÂÂÂelse
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value_cansleep(panel->reset, 1);
> This is not handling defer probing, so it would be better to do like
> this:
> 
> panel->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> if (IS_ERR(panel->reset))
> ÂÂÂÂÂÂreturnÂÂPTR_ERR(panel->reset);
> 
> > 
> > +ÂÂÂÂÂÂÂmemset(&bl_props, 0, sizeof(bl_props));
> > +ÂÂÂÂÂÂÂbl_props.type = BACKLIGHT_RAW;
> > +ÂÂÂÂÂÂÂbl_props.brightness = 255;
> > +ÂÂÂÂÂÂÂbl_props.max_brightness = 255;
> > +
> > +ÂÂÂÂÂÂÂpanel->backlight = devm_backlight_device_register(dev,
> > dev_name(dev),
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev, dsi,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&rad_bl_o
> > ps,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&bl_props
> > );
> Could you put more parameters into the same line?
> 
> Using 4 lines seems excessive.
> 
> > 
> > +ÂÂÂÂÂÂÂif (IS_ERR(panel->backlight)) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret = PTR_ERR(panel->backlight);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_err(dev, "Failed to register backlight (%d)\n",
> > ret);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ret;
> > +ÂÂÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂÂÂdrm_panel_init(&panel->panel);
> > +ÂÂÂÂÂÂÂpanel->panel.funcs = &rad_panel_funcs;
> > +ÂÂÂÂÂÂÂpanel->panel.dev = dev;
> > +ÂÂÂÂÂÂÂdev_set_drvdata(dev, panel);
> > +
> > +ÂÂÂÂÂÂÂret = drm_panel_add(&panel->panel);
> > +
> Unneeded blank line
> 
> > 
> > +ÂÂÂÂÂÂÂif (ret < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ret;
> > +
> > +ÂÂÂÂÂÂÂret = mipi_dsi_attach(dsi);
> > +ÂÂÂÂÂÂÂif (ret < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdrm_panel_remove(&panel->panel);
> > +
> > +ÂÂÂÂÂÂÂreturn ret;
> You did not handle the "power" regulator.
There is no need for a power regulator.
> 
> > 
> > +static int __maybe_unused 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 __maybe_unused 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);
> Why do you call devm_gpiod_get() once again?
> 
> I am having a hard time to understand the need for this
> suspend/resume hooks.
> 
> Can't you simply remove them?
The reset gpio pin is shared between the DSI display controller and
touch controller, both found on the same panel. Since, the touch driver
also acceses this gpio pin, in some cases the display can be put to
sleep, but the touch is still active. This is why, during suspend I
release the gpio resource, and I have to take it back in resume.
Without this release/acquire mechanism during suspend/resume, stress-
tests showed some failures: the resume freezes completly.