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

[Rob Clark]

On Mon, Aug 10, 2015 at 3:54 PM, Bjorn Andersson
<bjorn.andersson at sonymobile.com> wrote:
> On Fri 07 Aug 09:11 PDT 2015, Rob Clark wrote:
>
>> On Fri, Aug 7, 2015 at 9:19 AM, Thierry Reding <thierry.reding at 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.

Cool, thx

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

I was debating both approaches..  I think I ended up going with the
compat name so that we could, if wanted, have the two split out into
different drivers, depending on how much was in common.  Not even
really sure if that is worth worrying about or not.

BR,
-R

> [..]
>> >> 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