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