Messages in this thread | | | From | Emil Velikov <> | Date | Sat, 18 Jun 2016 01:28:10 +0100 | Subject | Re: [PATCH v6 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel |
| |
Hi Vinay,
On 17 June 2016 at 19:23, Vinay Simha BN <simhavcs@gmail.com> wrote:
> v6: > * emil review comments incorporated > PANEL_NUM_REGULATORS dropped, return ret added at necessary > places, if checks dropped for backlight and gpios
Looks like some of my suggestions went below the radar. Did you miss them or I've I lost the plot somewhere ? In case of the latter don't be shy to point out :-)
> +struct jdi_panel { > + struct drm_panel base; > + struct mipi_dsi_device *dsi; > + > + struct regulator_bulk_data supplies[3]; > + struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
> +static int jdi_panel_off(struct jdi_panel *jdi) > +{ > + struct mipi_dsi_device *dsi = jdi->dsi; > + int ret; > + > + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > + > + ret = mipi_dsi_dcs_set_display_off(dsi); > + if (ret < 0) > + return ret; > + IMHO neither this function nor its caller jdi_panel_unprepare() should stop in the middle/return prematurely.
Or in other words - one should change the function return type to void and drop the returns.
> +static int jdi_panel_unprepare(struct drm_panel *panel) > +{ > + struct jdi_panel *jdi = to_jdi_panel(panel); > + struct device *dev = &jdi->dsi->dev; > + int ret; > + > + if (!jdi->prepared) > + return 0; > + > + ret = jdi_panel_off(jdi); > + if (ret) { > + dev_err(panel->dev, "failed to set panel off: %d\n", ret); > + return ret; As suggested above, drop this return.
> +static int jdi_panel_prepare(struct drm_panel *panel) > +{ > + struct jdi_panel *jdi = to_jdi_panel(panel); > + struct device *dev = &jdi->dsi->dev; > + int ret; > + > + if (jdi->prepared) > + return 0; > + > + ret = regulator_bulk_enable(ARRAY_SIZE(jdi->supplies), jdi->supplies); > + if (ret < 0) { > + dev_err(dev, "regulator enable failed, %d\n", ret); > + return ret; > + } > + > + msleep(20); > + > + if (jdi->reset_gpio) { You can drop the check.
> + gpiod_set_value(jdi->reset_gpio, 1); > + usleep_range(10, 20); > + } > + > + if (jdi->enable_gpio) { Ditto.
> + > +poweroff: > + gpiod_set_value(jdi->enable_gpio, 0); > + gpiod_set_value(jdi->reset_gpio, 0); > + Just noticed that you're missing regulator_bulk_disable() here.
Regards, Emil
| |