Re: [PATCH 1/5] dt-bindings: display: panel: Update NewVision NV3051D compatibles

[Chris Morgan <macromorgan@hotmail.com>]

On Tue, Oct 24, 2023 at 01:27:55PM -0500, Rob Herring wrote:
> On Thu, Oct 19, 2023 at 09:50:38AM -0500, Chris Morgan wrote:
> > On Thu, Oct 19, 2023 at 11:22:19AM +0200, Krzysztof Kozlowski wrote:
> > > On 18/10/2023 18:18, Chris Morgan wrote:
> > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > > 
> > > > Update the NewVision NV3051D compatible strings by adding a new panel,
> > > > the powkiddy,rk2023-panel, and removing another entry, the
> > > > anbernic,rg353v-panel. The rg353v-panel is exactly identical to the
> > > > rg353p-panel and is not currently in use by any existing device tree.
> > > > The rk2023-panel is similar to the rg353p-panel but has slightly
> > > > different timings.
> > > 
> > > This still does not explain me why do you want to remove old panel.
> > 
> > When I originally wrote the driver I only had one piece of hardware
> > and I set the compatible string in the driver as newvision,nv3051d.
> > Unfortunately since then I've found 2 more devices in use that are
> > *just* different enough to require the driver to do things a bit
> > differently. In the case of the anbernic,rg351v-panel I need to
> > enable a new DSI flag; in the case of the powkiddy,rk2023-panel I need
> > to decrease the vertical back porch and drop the higher frequency
> > timings.
> > 
> > The best way to accomplish this was to change the strategy from having
> > a single binding in the driver of newvision,nv3051d to a binding for
> > each distinct hardware where the differences apply. 
> 
> Exactly why the DT maintainers annoyingly ask for specific compatible 
> strings which may not be used immediately.

You're not wrong. Sorry for making this difficult. I should have done
it this way from the start.

> 
> > Note that I've
> > looked at querying the DSI panel ID, but for each device the value
> > is identical (so it can't be used to differentiate the hardware sadly).
> > So the driver now has 3 different compatible strings. I could in this
> > case add a 4th compatible string of anbernic,rg353v-panel but it would
> > be identical to anbernic,rg353p-panel. For the moment we are using
> > anbernic,rg353p-panel everywhere (including the rg353v), so it makes
> > sense to drop this unused value while we can, at least to me.
> 
> Your reasoning is the compatible string is unused, so remove it. 
> 
> If there's some reasoning about how the 2 panels are the same hardware 
> or the rg353v is never going to be used or show up at some point, then 
> that would be a reason to remove.

The compatible string of 353v-panel is unused, and the hardware is
identical to the 353p-panel (so only one string is necessary). Sorry
if that wasn't clear.

Panel 1 - The original anbernic,rg353p-panel which is also
anbernic,rg353v-panel.

Panel 2 - anbernic,rg351v-panel. This is almost identical to Panel 1
except it requires an additional flag.

Panel 3 - powkiddy,rk2023-panel. This is almost identical to Panel 1
except it requires a change to the VBP timing parameter and isn't
tolerant of speeds much higher than 60hz.

The issue I had is I originally wrote the driver checking for the
newvision,nv3051d compatible string which worked fine when there was
only 1 panel type. When I added support for the 351v-panel I *should*
have changed how the compatible string was handled, but instead I
simply added a check in the probe function to look for the secondary
string of "anbernic,rg351v-panel". When the 3rd panel type of
"powkiddy,rk2023-panel" was needed I took this time to correct the
driver and do it the right way by checking for the specific
compatibles.

Thank you, and sorry for the headaches this caused you.
Chris

> 
> You could also say the rg353v is just wrong because it should have a 
> fallback compatible to rg353p and rather than fix it, just remove it 
> for now since there are no known users of it.
> 
> Rob