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?
