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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
