| Message ID | 20231018161848.346947-2-macroalpha82@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rockchip: Add Powkiddy RK2023 | expand |
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. Best regards, Krzysztof
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. 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. Let me know if you have any more questions, thank you. Chris > > > > Best regards, > Krzysztof >
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. > 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. 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
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
diff --git a/Documentation/devicetree/bindings/display/panel/newvision,nv3051d.yaml b/Documentation/devicetree/bindings/display/panel/newvision,nv3051d.yaml index cce775a87f87..7a634fbc465e 100644 --- a/Documentation/devicetree/bindings/display/panel/newvision,nv3051d.yaml +++ b/Documentation/devicetree/bindings/display/panel/newvision,nv3051d.yaml @@ -21,7 +21,7 @@ properties: - enum: - anbernic,rg351v-panel - anbernic,rg353p-panel - - anbernic,rg353v-panel + - powkiddy,rk2023-panel - const: newvision,nv3051d reg: true