Re: [2/2] drm/panel: lms397kf04: Add driver for LMS397KF04

[Linus Walleij <linus.walleij@linaro.org>]

Hi Doug,

just sent out a v2 of this! Hope you can look at it.

Some comment on comments:

On Thu, Apr 29, 2021 at 10:21 PM Doug Anderson <dianders@chromium.org> wrote:

(...)
> > +#define LMS397_UNKNOWN_F8              0xf8
> > +#define LMS397_UNKNOWN_FC              0xfc
>
> I managed to dig up a copy of the DCS spec. It says that all these
> 0xb0 - 0xfc are specific to each manufacturer, so that makes sense
> that they're all defined in this file instead of somewhere common.
> ...but it also says that, at least the way they're intended to be
> used, these commands are all supposed to be used only in the factory
> and then disabled. I guess maybe the manufacturer of this panel
> ignored that and requires these things to be configured each time the
> panel is powered up?
(...)
> I'll believe you if you tell me that it's correct, but something feels
> odd to me. As I mentioned above, the MIPI DCS specs say that it's
> expected that most of the configuration that you're doing in
> lms397kf04_power_on() would happen at manufacturer time and then be
> "locked" so you don't need to do it again.
>
> I could sorta believe that maybe some panels wouldn't have any
> non-volatile storage and that would explain why you need to program
> this stuff on every enable. ...but now the above comment says that
> it's loading stuff from non-volatile memory.
>
> Are you sure that all of the magic config commands you have in
> lms397kf04_power_on() are actually needed / doing anything? Could they
> be leftover from some example code and they're not actually needed
> anymore?

Yes it is absolutely necessary.

This happens for a bit, see for example
drivers/gpu/drm/panel/panel-novatek-nt35510.c

Why manufacturers (Samsung) do this I do not know for sure.
Could be either:

1. The flash memory is an optional external component so they save
  money on the BOM like this
2. Programming the flash in production takes too much time so they
  save manufacturing cost like this
3. Time-to-market, product was stressed out the door without
  time to set up the proper flashing on the production line
4. The panel actually has some settings from production, but oops
  they are wrong and now they can't be fixed.

Case 4 corresponds to e.g. all ACPI quirks we have because
abstracting things away by giving stuff BIOS is so good in theory
and fails so much in practice because bugs.

> So I've never looked at MIPI stuff at all before. ...but, as far as I
> can tell your panel is implementing MIPI DBI Type C Option 1 (3-line
> SPI). It feels like you should be using the functions for dealing with
> MIPI DBI, like mipi_dbi_spi_init(), mipi_dbi_command(). If I'm reading
> the code correctly, I think that will have the benefit of making your
> panel more flexible in terms of the capabilities of the SPI
> controller. It seems to handle the case when the SPI controller
> doesn't support 9-bits-per-word transfers, for instance.
>
> If you have a good reason for not using the MIPI DBI functions then
> let me know and I'll look over your functions more closely.

MIPI DBI nominally requires an extra (GPIO) line called D/C
(data/command) and this display for sure does not have it.
So it would be "type c1" (no extra GPIO).

I will test and see if I can get DBI working... if I do I will
respin v2 to v3.

Yours,
Linus Walleij