On Tue, Mar 21, 2017 at 01:38:31PM -0700, Brian Norris wrote:
> + Thierry for real
>
> On Tue, Mar 21, 2017 at 03:26:35PM -0400, Sean Paul wrote:
> > On Wed, Mar 15, 2017 at 03:19:13PM +0800, Chris Zhong wrote:
> > > Support Innolux P079ZCA 7.85" 768x1024 TFT LCD panel, it is a MIPI DSI
> > > panel.
> > >
> > > Signed-off-by: Chris Zhong <z...@rock-chips.com>
> > > ---
> > >
> > > Changes in v2:
> > > - add some error check
> > > - always use Low power mode to send commend
> > > - add comments for all the sleep
> > > - use DRM_DEV_ERROR instead of dev_err
> > >
> >
> > Minor suggestion below. Once that's cleared up, you can add:
> > Reviewed-by: Sean Paul <seanp...@chromium.org>
> >
> > Also, please add Thierry directly to your patches so he sees them.
> >
> >
> > > drivers/gpu/drm/panel/Kconfig | 11 +
> > > drivers/gpu/drm/panel/Makefile | 1 +
> > > drivers/gpu/drm/panel/panel-innolux-p079zca.c | 344
> > > ++++++++++++++++++++++++++
> > > 3 files changed, 356 insertions(+)
> > > create mode 100644 drivers/gpu/drm/panel/panel-innolux-p079zca.c
> > >
> >
> > <snip>
> > > +static int innolux_panel_prepare(struct drm_panel *panel)
> > > +{
> > > + struct innolux_panel *innolux = to_innolux_panel(panel);
> > > + int err, ret;
> > > +
> > > + if (innolux->prepared)
> > > + return 0;
> > > +
> > > + gpiod_set_value_cansleep(innolux->enable_gpio, 0);
> > > +
> > > + err = regulator_enable(innolux->supply);
> > > + if (err < 0)
> > > + return err;
> > > +
> > > + /* T2: 15ms - 1000ms */
> > > + usleep_range(15000, 16000);
> > > +
> > > + gpiod_set_value_cansleep(innolux->enable_gpio, 1);
> > > +
> > > + /* T4: 15ms - 1000ms */
> > > + usleep_range(15000, 16000);
> > > +
> > > + err = mipi_dsi_dcs_exit_sleep_mode(innolux->link);
> > > + if (err < 0) {
> > > + DRM_DEV_ERROR(panel->dev, "failed to exit sleep mode: %d\n",
> > > + err);
> > > + goto poweroff;
> > > + }
> > > +
> > > + /* T6: 120ms - 1000ms*/
> > > + msleep(120);
> > > +
> > > + err = mipi_dsi_dcs_set_display_on(innolux->link);
> > > + if (err < 0) {
> > > + DRM_DEV_ERROR(panel->dev, "failed to set display on: %d\n",
> > > + err);
> > > + goto poweroff;
> > > + }
> > > +
> > > + /* T7: 5ms */
> > > + usleep_range(5000, 6000);
> > > +
> > > + innolux->prepared = true;
> > > +
> > > + return 0;
> > > +
> > > +poweroff:
> > > + ret = regulator_disable(innolux->supply);
> > > + if (ret)
> > > + return ret;
> >
> > I don't think we should be returning this error code. If we're here, it's
> > because something else triggered err, and we should return that. Change
> > this to:
> >
> > if (regulator_disable(innolux->supply))
> > DRM_DEV_ERROR(panel->dev, "failed to disable regulator after
> > error\n");
>
> And maybe include the error code in that message still?
>
> ret = regulator_disable(innolux->supply);
> if (ret)
> DRM_DEV_ERROR(panel->dev, "failed to disable regulator after
> error (%d)\n",
> ret);
Sure, but let's not have ret beside err, each function should have one or the
other. So, perhaps s/ret/regulator_err/ or similar.
Sean
>
> Brian
>
> > > +
> > > + gpiod_set_value_cansleep(innolux->enable_gpio, 0);
> > > + return err;
> > > +}
> >
> > <snip>
> >
> > > --
> > > 2.6.3
--
Sean Paul, Software Engineer, Google / Chromium OS
