Re: [PATCH v5 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

[Emil Velikov]

Hi Vinay,

I belive I've spotted a few issues. If my understanding is correct,
then I'll defer to Thierry if he'd like them fixed here, or as
follow-ups.

On 16 June 2016 at 04:00, Vinay Simha BN <simhavcs@xxxxxxxxx> wrote:


> +#define PANEL_NUM_REGULATORS   3
> +
Nit: #define PANEL_NUM_REGULATORS ARRAY_SIZE(regulator_names) or just
drop the extra define and use the latter directly ?


> +static int jdi_panel_init(struct jdi_panel *jdi)
> +{


> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
Nit: The above three lines can become one - "return ret;"

> +}
> +
> +static int jdi_panel_on(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_on(dsi);
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
Ditto.


> +static int jdi_panel_disable(struct drm_panel *panel)
> +{
> +       struct jdi_panel *jdi = to_jdi_panel(panel);
> +
> +       if (!jdi->enabled)
> +               return 0;
> +
Thinking out loud:

Thierry,
Shouldn't we fold 'enabled' and 'prepared' in struct drm_panel and
tweak the helpers respectively ? Is there any specific reason for
keeping these in the drivers ?


> +       if (jdi->backlight) {
We seems to be bailing out of jdi_panel_add() when this is NULL. Thus
we can omit the check.



> +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;
> +       }
> +
> +       ret = regulator_bulk_disable(ARRAY_SIZE(jdi->supplies), jdi->supplies);
> +       if (ret < 0) {
> +               dev_err(dev, "regulator disable failed, %d\n", ret);
> +               return ret;
> +       }
> +
Since we cannot recover from most/all of the above I'm thinking if one
shouldn't drop the "return ret" lines. Same goes for jdi_panel_off().


> +       if (jdi->reset_gpio)
> +               gpiod_set_value(jdi->reset_gpio, 0);
> +
> +       if (jdi->enable_gpio)
Drop these two checks. The gpios are required, thus one should bail
out in jdi_panel_add()



> +static int jdi_panel_prepare(struct drm_panel *panel)
> +{

> +       if (jdi->reset_gpio) {
> +               gpiod_set_value(jdi->reset_gpio, 1);
> +               usleep_range(10, 20);
> +       }
> +
> +       if (jdi->enable_gpio) {
> +               gpiod_set_value(jdi->enable_gpio, 1);
> +               usleep_range(10, 20);
> +       }
> +


> +poweroff:
> +       if (jdi->reset_gpio)
> +               gpiod_set_value(jdi->reset_gpio, 0);
> +       if (jdi->enable_gpio)
> +               gpiod_set_value(jdi->enable_gpio, 0);
> +
Generic suggestion/nitpick: Please keep the teardown order the inverse
of the setup one. In here one could/should handle enable_gpio first
and then reset_gpio.

> +       return ret;
> +}
> +
> +static int jdi_panel_enable(struct drm_panel *panel)
> +{
> +       struct jdi_panel *jdi = to_jdi_panel(panel);
> +
> +       if (jdi->enabled)
> +               return 0;
> +
> +       if (jdi->backlight) {
Analogous to jdi_panel_disable - drop the check ?



> +static int jdi_panel_add(struct jdi_panel *jdi)
> +{


> +       jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +       if (IS_ERR(jdi->reset_gpio)) {
> +               dev_err(dev, "cannot get reset-gpios %ld\n",
> +                       PTR_ERR(jdi->reset_gpio));
> +               jdi->reset_gpio = NULL;
> +       } else {
> +               gpiod_direction_output(jdi->reset_gpio, 0);
> +       }
> +
> +       jdi->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> +       if (IS_ERR(jdi->enable_gpio)) {
> +               dev_err(dev, "cannot get enable-gpio %ld\n",
> +                       PTR_ERR(jdi->enable_gpio));
> +               jdi->enable_gpio = NULL;
> +       } else {
> +               gpiod_direction_output(jdi->enable_gpio, 0);
> +       }
> +
As mentioned above - since these two are required, thus we should
error out of this function. Right ?

> +       jdi->backlight = drm_panel_create_dsi_backlight(jdi->dsi);
> +       if (!jdi->backlight)
> +               return -EPROBE_DEFER;
> +
> +       drm_panel_init(&jdi->base);
> +       jdi->base.funcs = &jdi_panel_funcs;
> +       jdi->base.dev = &jdi->dsi->dev;
> +
> +       ret = drm_panel_add(&jdi->base);
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
return ret;

Regards,
Emil