Re: [PATCH V7 2/2] drm/panel: Add Sitronix ST7701 panel driver

From: Sam Ravnborg <sam@ravnborg.org>
To: Jagan Teki <jagan@amarulasolutions.com>
Cc: David Airlie <airlied@linux.ie>,
	linux-amarula@amarulasolutions.com,
	linux-kernel <linux-kernel@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Michael Trimarchi <michael@amarulasolutions.com>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH V7 2/2] drm/panel: Add Sitronix ST7701 panel driver
Date: Sat, 12 Jan 2019 11:14:09 +0100	[thread overview]
Message-ID: <20190112101409.GA14273@ravnborg.org> (raw)
In-Reply-To: <CAMty3ZArGPf8+M5GVhkzoMDhtYwT3aUS9bNbKt8=gqeb6-b82Q@mail.gmail.com>

Hi Jagan.

> On Sat, Jan 12, 2019 at 2:49 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > Hi Jagan.
> >
> > Gave this another more detailed read - triggered some additional comments.
> > Depite the comments it looks good, and this is all more or
> > less cosmetic improvements.
> 
> Thanks for the review.
> 
> >
> >         Sam
> >
> > > +struct st7701_panel_desc {
> > > +     const struct drm_display_mode *mode;
> > > +     unsigned int lanes;
> > > +     unsigned long flags;
> > > +     enum mipi_dsi_pixel_format format;
> > > +     const char *const *supply_names;
> > > +     unsigned int num_supplies;
> > > +     unsigned int panel_sleep_delay;
> > > +};
> > > +
> > > +struct st7701 {
> > > +     struct drm_panel panel;
> > > +     struct mipi_dsi_device *dsi;
> > > +     const struct st7701_panel_desc *desc;
> > > +
> > > +     struct backlight_device *backlight;
> > > +     struct regulator_bulk_data *supplies;
> > > +     unsigned int num_supplies;
> > I cannot see that num_supplies in this struct are used?
> 
> Yes it is used in the code, please check in struct st7701_panel_desc.
I have applied the patch and deleted num_supplies - now build errors.
So num_supplies in struct st7701 is not used and should be deleted.

> > > +static int st7701_prepare(struct drm_panel *panel)
> > > +{
> > > +     struct st7701 *st7701 = panel_to_st7701(panel);
> > > +     int ret;
> > > +
> > > +     gpiod_set_value(st7701->reset, 0);
> > > +     msleep(20);
> > > +
> > > +     ret = regulator_bulk_enable(st7701->desc->num_supplies,
> > > +                                 st7701->supplies);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +     msleep(20);
> > > +
> > > +     gpiod_set_value(st7701->reset, 1);
> > > +     msleep(20);
> > > +
> > > +     gpiod_set_value(st7701->reset, 0);
> > > +     msleep(30);
> > > +
> > > +     gpiod_set_value(st7701->reset, 1);
> > > +     msleep(150);
> > > +
> > > +     st7701_init_sequence(st7701);
> > > +
> > > +     return 0;
> > > +}
> > > +
> >
> > > +static int st7701_unprepare(struct drm_panel *panel)
> > > +{
> > > +     struct st7701 *st7701 = panel_to_st7701(panel);
> > > +
> > > +     ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);

Should this be MIPI_DCS_ENTER_SLEEP_MODE?

Note - you have shortcuts fot the standard commands like
in this case:

	mipi_dsi_dcs_enter_sleep_mode(st7701->dsi);

Thay could be introduced in many palces, but I also like how all the
commands follows a consistent way to be issued.
So consider this only if this was new for you.

> > > +
> > > +     msleep(st7701->sleep_delay);
> > > +
> > > +     gpiod_set_value(st7701->reset, 0);
> > > +
> > > +     gpiod_set_value(st7701->reset, 1);
> > > +
> > > +     gpiod_set_value(st7701->reset, 0);
> > No timing constrains here? In prepare there are sleeps intermixed.
> 
> Delay while doing unprare is not needed I suppose.

If the purpose is alone to reset the display then a single write '0'
should do it I think
And there is a requirement that it must be low for a minimum of 10 us
which would be good to have here.

I aslo found in chapter 9. (page 163 - second line) this statement:
"VDDA and VDDI must be powered down with minimum 120msec."

This is similar to the unprepare delay to be found in simple-panel.c
So an unprepare delay seems in order here.

> > > +static const struct st7701_panel_desc ts8550b_desc = {
> > > +     .mode = &ts8550b_mode,
> > > +     .lanes = 2,
> > > +     .flags = MIPI_DSI_MODE_VIDEO,
> > > +     .format = MIPI_DSI_FMT_RGB888,
> > > +     .supply_names = ts8550b_supply_names,
> > > +     .num_supplies = ARRAY_SIZE(ts8550b_supply_names),
> > > +     .panel_sleep_delay = 80, /* panel need extra 80ms for sleep out cmd */
> > In the only place this is used there is added 120 ms too.
> > Looks inconsistent - do we have the same delay twice?
> 
> 120ms is the one recommended by st7701 controller delay after a sleep
> out command, please check it in datasheet [1], page 188. And this 80ms
> is specific to TS8550B panel as per BSP code this doesn't have proper
> documentation but the BSP code doing this extra 80ms.

OK, thanks for the pointer to the datasheet.

> [1] http://www.startek-lcd.com/res/starteklcd/pdres/201705/20170512144242904.pdf

	Sam