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

[Jagan Teki]

On Sat, Jan 12, 2019 at 3:44 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
>
> Hi Jagan.
>
> > On Sat, Jan 12, 2019 at 2:49 AM Sam Ravnborg <sam@xxxxxxxxxxxx> 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.

Sorry, now I found it thanks, will drop in next version.

>
> > > > +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);

I even tried this, but the difference wrt ST7701 can pass, same
command with type MIPI_DSI_DCS_SHORT_WRITE_PARAM but the default
mipi_dsi_dcs_enter_sleep_mode has length 0 so it's
MIPI_DSI_DCS_SHORT_WRITE type. ie reason I have not used.

>
> 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

I even tried this just set 0, since prepare is doing a sequence, it
good behavior to do the reverse during handoff. ie reason I just
initiated this sequence.

> And there is a requirement that it must be low for a minimum of 10 us
> which would be good to have here.

Sorry, I didn't get this requirement what is this for?

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

Yes unprepare is doing the same, it exit from sleep out mode and wait
for 120msec and do the reset.

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

Look like simple-panel.c is doing delay after reset initiated and
regulator disabled.