Re: [PATCH] drm/panel: auo novatek 1080p video mode panel

[Bjorn Andersson <bjorn.andersson@sonymobile.com>]

On Fri 07 Aug 09:11 PDT 2015, Rob Clark wrote:

> On Fri, Aug 7, 2015 at 9:19 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Jul 21, 2015 at 03:36:02PM -0400, Rob Clark wrote:
[..]
> >> +- compatible: should be "auo,novatek-1080p-vid"
> >
> > This looks a little generic for a compatible string. Can't we get at the
> > specific panel model number that's been used? What if AUO ever produced
> > some other Novatek panel with a 1080p resolution?
> 
> Maybe Sony or someone else can chime in?  That somewhat generic name
> was all I could get from downstream android kernel.  I'm sure there is
> a better possible name, although I have no means to find that out
> myself.
> 

We're working on it.

> > Also, what's the -vid suffix for?
> 
> the same panel seems to also work in cmd mode.. so idea was to have
> -vid and -cmd compat strings to choose which mode to operate in.
> 

An alternative would be to make it a bool property, to indicate video
mode - following how the framework is implemented.

[..]
> >> diff --git a/drivers/gpu/drm/panel/panel-auo-novatek-1080p.c b/drivers/gpu/drm/panel/panel-auo-novatek-1080p.c
> >> +static int auo_panel_init(struct auo_panel *auo)
> >> +{
> >> +     struct mipi_dsi_device *dsi = auo->dsi;
> >> +     int ret;
> >> +
> >> +     dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> >> +
> >> +     ret = mipi_dsi_generic_write(dsi, (u8[]){ 0xb0, 0x04 }, 2);
> >
> > I find this notation hard to read. Have you considered moving this into
> > some sort of table that you can loop through? Or perhaps add some
> > helpers, say, mipi_dsi_generic_writeb() and mipi_dsi_dcs_writeb() to
> > help make this more readable?
> >
> 
> Yeah, helper macro thing might be a reasonable idea.  The table option
> makes it hard to use the helpers for things that are not non-standard,
> or when you need delays, etc..
> 

I agree with you here, we don't want lists of data that the driver has
to interpret into writes (of various types) and delays.

> 
> >> +     if (ret < 0)
> >> +             return ret;
> >> +

Regards,
Bjorn