Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1425579

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v6 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel Vinay Simha BN <simhavcs@gmail.com> - 2016-06-17 20:30 +0200
  Re: [PATCH v6 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel Emil Velikov <emil.l.velikov@gmail.com> - 2016-06-18 02:30 +0200
    Re: [PATCH v6 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel Vinay Simha <simhavcs@gmail.com> - 2016-06-18 09:00 +0200
      Re: [PATCH v6 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel Emil Velikov <emil.l.velikov@gmail.com> - 2016-06-19 01:50 +0200

csiph-web