| Message ID | 20240710-x1e80100-crd-backlight-v1-1-eb242311a23e@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/panel: atna33xc20: Fix the Samsung ATNA45AF01 panel | expand |
Hi, On Wed, Jul 10, 2024 at 10:05 AM Stephan Gerhold <stephan.gerhold@linaro.org> wrote: > > The Samsung ATNA45AF01 panel is an AMOLED eDP panel that has backlight > control over the DP AUX channel. While it works almost correctly with the > generic "edp-panel" compatible, the backlight needs special handling to > work correctly. It is similar to the existing ATNA33XC20 panel, just with > a larger resolution and size. > > Add a new "samsung,atna45af01" compatible to describe this panel in the DT. > > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > --- > .../devicetree/bindings/display/panel/samsung,atna33xc20.yaml | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml b/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml > index 765ca155c83a..d668e8d0d296 100644 > --- a/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml > +++ b/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml > @@ -14,7 +14,11 @@ allOf: > > properties: > compatible: > - const: samsung,atna33xc20 > + enum: > + # Samsung 13.3" FHD (1920x1080 pixels) eDP AMOLED panel > + - samsung,atna33xc20 > + # Samsung 14.5" WQXGA+ (2880x1800 pixels) eDP AMOLED panel > + - samsung,atna45af01 Seems OK, but a few thoughts: 1. Is it worth renaming this file? Something like "samsung,atna-oled-panel.yaml"? I'd be interested in DT maintainer folks' opinions here. 2. In theory you could make your compatible look like this: compatible = "samsung,atna45af01", "samsung,atna33xc20" ...which would say "I have a 45af01 but if the OS doesn't have anything special to do that it would be fine to use the 33xc20 driver". That would allow device trees to work without the kernel changes and would allow you to land the DT changes in parallel with the driver changes and things would keep working. ...and, in fact, that would mean you _didn't_ need to add the new compatible string to the driver, which is nice. -Doug
On Wed, Jul 10, 2024 at 10:35:28AM -0700, Doug Anderson wrote: > On Wed, Jul 10, 2024 at 10:05 AM Stephan Gerhold > <stephan.gerhold@linaro.org> wrote: > > > > The Samsung ATNA45AF01 panel is an AMOLED eDP panel that has backlight > > control over the DP AUX channel. While it works almost correctly with the > > generic "edp-panel" compatible, the backlight needs special handling to > > work correctly. It is similar to the existing ATNA33XC20 panel, just with > > a larger resolution and size. > > > > Add a new "samsung,atna45af01" compatible to describe this panel in the DT. > > > > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > > --- > > .../devicetree/bindings/display/panel/samsung,atna33xc20.yaml | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml b/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml > > index 765ca155c83a..d668e8d0d296 100644 > > --- a/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml > > +++ b/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml > > @@ -14,7 +14,11 @@ allOf: > > > > properties: > > compatible: > > - const: samsung,atna33xc20 > > + enum: > > + # Samsung 13.3" FHD (1920x1080 pixels) eDP AMOLED panel > > + - samsung,atna33xc20 > > + # Samsung 14.5" WQXGA+ (2880x1800 pixels) eDP AMOLED panel > > + - samsung,atna45af01 > > Seems OK, but a few thoughts: > > 1. Is it worth renaming this file? Something like > "samsung,atna-oled-panel.yaml"? I'd be interested in DT maintainer > folks' opinions here. > I think examples for both approaches exist in the kernel tree, so I am also interested in the opinion of the DT maintainers here. :-) > 2. In theory you could make your compatible look like this: > > compatible = "samsung,atna45af01", "samsung,atna33xc20" > > ...which would say "I have a 45af01 but if the OS doesn't have > anything special to do that it would be fine to use the 33xc20 > driver". That would allow device trees to work without the kernel > changes and would allow you to land the DT changes in parallel with > the driver changes and things would keep working. > > ...and, in fact, that would mean you _didn't_ need to add the new > compatible string to the driver, which is nice. > Yeah, I considered this. I mentioned the reason why I decided against this in patch 2: > While ATNA45AF01 would also work with "samsung,atna33xc20" as a fallback > compatible, the original submission of the compatible in commit > 4bfe6c8f7c23 ("drm/panel-simple: Add Samsung ATNA33XC20") had the timings > and resolution hardcoded. These would not work for ATNA45AF01. Basically, it works with the current driver. But if you would run the kernel at the state of the original submission then it would behave incorrectly. This is why I considered the resolution and timings to be part of the "samsung,atna33xc20" "ABI". The new panel would not be compatible with that. I don't mind changing it, if there is consensus that we should ignore this and use the fallback compatible instead. Thanks, Stephan
Hi, On Wed, Jul 10, 2024 at 12:03 PM Stephan Gerhold <stephan.gerhold@linaro.org> wrote: > > > 2. In theory you could make your compatible look like this: > > > > compatible = "samsung,atna45af01", "samsung,atna33xc20" > > > > ...which would say "I have a 45af01 but if the OS doesn't have > > anything special to do that it would be fine to use the 33xc20 > > driver". That would allow device trees to work without the kernel > > changes and would allow you to land the DT changes in parallel with > > the driver changes and things would keep working. > > > > ...and, in fact, that would mean you _didn't_ need to add the new > > compatible string to the driver, which is nice. > > > > Yeah, I considered this. I mentioned the reason why I decided against > this in patch 2: > > > While ATNA45AF01 would also work with "samsung,atna33xc20" as a fallback > > compatible, the original submission of the compatible in commit > > 4bfe6c8f7c23 ("drm/panel-simple: Add Samsung ATNA33XC20") had the timings > > and resolution hardcoded. These would not work for ATNA45AF01. > > Basically, it works with the current driver. But if you would run the > kernel at the state of the original submission then it would behave > incorrectly. This is why I considered the resolution and timings to be > part of the "samsung,atna33xc20" "ABI". The new panel would not be > compatible with that. Ah, oops. My eyes totally glazed over the description since the patch was so simple. :-P Sorry about that. IMO I'd still prefer using the fallback compatible, but happy to hear other opinions. In the original commit things were pretty broken still (sorta like how it's broken for you using "edp-panel") and the resolution hasn't been hardcoded for a long while...
On 10/07/2024 19:04, Stephan Gerhold wrote: > The Samsung ATNA45AF01 panel is an AMOLED eDP panel that has backlight > control over the DP AUX channel. While it works almost correctly with the > generic "edp-panel" compatible, the backlight needs special handling to > work correctly. It is similar to the existing ATNA33XC20 panel, just with > a larger resolution and size. > > Add a new "samsung,atna45af01" compatible to describe this panel in the DT. > > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > --- Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 10/07/2024 19:35, Doug Anderson wrote: > Hi, > > On Wed, Jul 10, 2024 at 10:05 AM Stephan Gerhold > <stephan.gerhold@linaro.org> wrote: >> >> The Samsung ATNA45AF01 panel is an AMOLED eDP panel that has backlight >> control over the DP AUX channel. While it works almost correctly with the >> generic "edp-panel" compatible, the backlight needs special handling to >> work correctly. It is similar to the existing ATNA33XC20 panel, just with >> a larger resolution and size. >> >> Add a new "samsung,atna45af01" compatible to describe this panel in the DT. >> >> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> >> --- >> .../devicetree/bindings/display/panel/samsung,atna33xc20.yaml | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml b/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml >> index 765ca155c83a..d668e8d0d296 100644 >> --- a/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml >> +++ b/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml >> @@ -14,7 +14,11 @@ allOf: >> >> properties: >> compatible: >> - const: samsung,atna33xc20 >> + enum: >> + # Samsung 13.3" FHD (1920x1080 pixels) eDP AMOLED panel >> + - samsung,atna33xc20 >> + # Samsung 14.5" WQXGA+ (2880x1800 pixels) eDP AMOLED panel >> + - samsung,atna45af01 > > Seems OK, but a few thoughts: > > 1. Is it worth renaming this file? Something like > "samsung,atna-oled-panel.yaml"? I'd be interested in DT maintainer > folks' opinions here. No, rather keep existing name, because it should be based on compatible. Best regards, Krzysztof
On Wed, Jul 10, 2024 at 12:16:58PM -0700, Doug Anderson wrote: > On Wed, Jul 10, 2024 at 12:03 PM Stephan Gerhold > <stephan.gerhold@linaro.org> wrote: > > > > > 2. In theory you could make your compatible look like this: > > > > > > compatible = "samsung,atna45af01", "samsung,atna33xc20" > > > > > > ...which would say "I have a 45af01 but if the OS doesn't have > > > anything special to do that it would be fine to use the 33xc20 > > > driver". That would allow device trees to work without the kernel > > > changes and would allow you to land the DT changes in parallel with > > > the driver changes and things would keep working. > > > > > > ...and, in fact, that would mean you _didn't_ need to add the new > > > compatible string to the driver, which is nice. > > > > > > > Yeah, I considered this. I mentioned the reason why I decided against > > this in patch 2: > > > > > While ATNA45AF01 would also work with "samsung,atna33xc20" as a fallback > > > compatible, the original submission of the compatible in commit > > > 4bfe6c8f7c23 ("drm/panel-simple: Add Samsung ATNA33XC20") had the timings > > > and resolution hardcoded. These would not work for ATNA45AF01. > > > > Basically, it works with the current driver. But if you would run the > > kernel at the state of the original submission then it would behave > > incorrectly. This is why I considered the resolution and timings to be > > part of the "samsung,atna33xc20" "ABI". The new panel would not be > > compatible with that. > > Ah, oops. My eyes totally glazed over the description since the patch > was so simple. :-P Sorry about that. > > IMO I'd still prefer using the fallback compatible, but happy to hear > other opinions. In the original commit things were pretty broken still > (sorta like how it's broken for you using "edp-panel") and the > resolution hasn't been hardcoded for a long while... I briefly discussed this with Krzysztof on IRC yesterday and we concluded that a fallback compatible is better. If one considers just the non-discoverable part of the interface for the binding (i.e. the non-standard power sequence), then the two panels are indeed compatible. I will send a v2 with the fallback compatible on Monday. I think this can also simplify backporting the backlight fix as you mentioned, since then no driver change is required to make it work. Thanks, Stephan
diff --git a/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml b/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml index 765ca155c83a..d668e8d0d296 100644 --- a/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml +++ b/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml @@ -14,7 +14,11 @@ allOf: properties: compatible: - const: samsung,atna33xc20 + enum: + # Samsung 13.3" FHD (1920x1080 pixels) eDP AMOLED panel + - samsung,atna33xc20 + # Samsung 14.5" WQXGA+ (2880x1800 pixels) eDP AMOLED panel + - samsung,atna45af01 enable-gpios: true port: true
The Samsung ATNA45AF01 panel is an AMOLED eDP panel that has backlight control over the DP AUX channel. While it works almost correctly with the generic "edp-panel" compatible, the backlight needs special handling to work correctly. It is similar to the existing ATNA33XC20 panel, just with a larger resolution and size. Add a new "samsung,atna45af01" compatible to describe this panel in the DT. Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> --- .../devicetree/bindings/display/panel/samsung,atna33xc20.yaml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)