Hi Vinay,
On 17 June 2016 at 19:23, Vinay Simha BN <simhavcs@xxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
