[PATCH 01/41] drm/panel: simple: Change mode for Sharp lq123p1jx31

[Doug Anderson]

Hi,

On Mon, Mar 20, 2017 at 6:59 AM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Thu, Mar 09, 2017 at 11:32:16PM -0500, Sean Paul wrote:
>> Change the mode for Sharp lq123p1jx31 panel to something more
>> rockchip-friendly such that we can use the fixed PLLs to
>> generate the pixel clock
>>
>> Cc: Chris Zhong <zyw at rock-chips.com>
>> Cc: Stéphane Marchesin <marcheu at chromium.org>
>> Signed-off-by: Sean Paul <seanpaul at chromium.org>
>> ---
>>  drivers/gpu/drm/panel/panel-simple.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> That's really not how you should be doing this. If you want to support
> this panel on more than one type of hardware, especially if they have
> different restrictions on what clocks and timings they can generate,
> the driver should specify the timings using a struct display_timing and
> drivers should use that data to generate a mode that matches their
> requirements but is still within the valid ranges specified in the .min
> and .max values.
>
> That said, in this particular case nobody seems to be using the panel
> in the upstream kernel.

Sean and I had a private conversation about this too.  Roughly summarizing:

1. We have validated with the panel manufacturer that 266.667 MHz is
valid and within spec.  The panel's datasheet itself says something
like "if you want to try other values you can, but they might not
work", so technically the only values "known" to work are those that
were in the original patch and the values here in Sean's patch.

2. In the particular case of rk3399-kevin (which uses this panel), we
actually went through several iterations before we found a mode that
not only worked with the limited PLLs but also that didn't generate
excessive noise on the digitizer (used for stylus input).  The
digitizer is (as I understand) a separate component from the panel
itself, so this restriction isn't really one on the panel but is a
reality of how this panel was used in real hardware.  I have no idea
how one expresses this board-centric view of the world.

NOTE: Point #2 actually made me check, and Sean's patch is the wrong
iteration of these changes.  Please see http://crosreview.com/381015

3. In an ideal world, even on rk3399-kevin we'd be able to choose the
252.75 MHz clock if the variable PLL on rk3399 happened to be
available (if there was no external display whose pixel clock needed
the variable PLL).  This would save a bit of power, and I believe the
252.75 MHz also avoids noise on the digitizer.  ...but trying to deal
with all this was very complicated.


That all being said: I'd personally be in favor for something like
Sean's patch to land since, as you said, there are no other current
users of the panel.  It's nice to start with something working and
hopefully we can figure out a more advanced / dynamic system sometime
in the future.


> One minor nit below...
>
>>
>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>> index 89eb0422821c..787b4d143220 100644
>> --- a/drivers/gpu/drm/panel/panel-simple.c
>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>> @@ -1598,17 +1598,18 @@ static const struct panel_desc sharp_lq101k1ly04 = {
>>  };
>>
>>  static const struct drm_display_mode sharp_lq123p1jx31_mode = {
>> -     .clock = 252750,
>> +     .clock = 266667,
>>       .hdisplay = 2400,
>>       .hsync_start = 2400 + 48,
>>       .hsync_end = 2400 + 48 + 32,
>> -     .htotal = 2400 + 48 + 32 + 80,
>> +     .htotal = 2400 + 48 + 32 + 139,

Please fold in <https://chromium-review.googlesource.com/381015> to
get noise-free timings.


>>       .vdisplay = 1600,
>>       .vsync_start = 1600 + 3,
>>       .vsync_end = 1600 + 3 + 10,
>> -     .vtotal = 1600 + 3 + 10 + 33,
>> +     .vtotal = 1600 + 3 + 10 + 84,

Here too.

>>       .vrefresh = 60,
>>       .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
>> +     .type = DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER,

IIRC this was an attempt to deal with the fact that the EDID had a
different mode than we were specifying here, but I could be wrong.



-Doug