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

[Fabio Estevam]

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_ops,
> +                                                         &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.

> +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?