Re: [PATCH v3 02/10] dt-bindings: display: panel: samsung,s6e3ha8: Add AMB630QY01 panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Low] dt-bindings: display: panel: samsung,s6e3ha8: unwieldy
  per-compatible conditionals
- [Low] dt-bindings: display: panel: samsung,s6e3ha8: required and allOf
  before properties
- [Low] dt-bindings: display: panel: samsung,s6e3ha8: changelog omits
  supply changes
--

commit 2e2113bfd1b86858603c7b29b0786ff6036afe10
Author: David Heidelberg <david@ixit.cz>

> dt-bindings: display: panel: samsung,s6e3ha8: Add AMB630QY01 panel
>
> The Google Pixel 3 XL (crosshatch) uses a Samsung AMB630QY01 6.3"
> 1440x2960 flexible AMOLED panel.
>
> Signed-off-by: David Heidelberg <david@ixit.cz>

[Severity: Low]
Could the commit message also describe the regulator supply changes?  Beyond
adding the samsung,s6e3ha8-amb630qy01 compatible string, the patch adds
vddpos-supply and vddneg-supply for the ELVDD and ELVSS boost regulators and
makes vddr-supply vs vddpos-supply/vddneg-supply conditionally required per
panel variant.

> diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha8.yaml b/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha8.yaml
> index ca250b1b91712..dcf347fe5ca83 100644
> --- a/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha8.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha8.yaml
> @@ -12,17 +12,45 @@ description: |
>  
>    Known panels:
>      - AMB577PX01, 5.77", Samsung Galaxy S9 (samsung,s6e3ha8-amb577px01)
> +    - AMB630QY01, 6.3", Google Pixel 3 XL (samsung,s6e3ha8-amb630qy01)
>  
>  maintainers:
>    - Dzmitry Sankouski <dsankouski@gmail.com>
>  
> +required:
> +  - compatible
> +  - reset-gpios
> +  - vdd3-supply
> +  - vci-supply
> +
>  allOf:

[Severity: Low]
As reported by Krzysztof Kozlowski on v2 [1], should the properties block
come before required and allOf?  Standard DT schema ordering places
properties first, followed by required, allOf, and unevaluatedProperties:
false, rather than listing required properties and allOf before properties
are defined.

>    - $ref: panel-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - samsung,s6e3ha8
> +              - samsung,s6e3ha8-amb577px01
> +    then:
> +      required:
> +        - vddr-supply
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,s6e3ha8-amb630qy01
> +    then:
> +      required:
> +        - vddpos-supply
> +        - vddneg-supply

[Severity: Low]
Would it be cleaner to split samsung,s6e3ha8-amb630qy01 into a separate
binding schema document instead of adding per-compatible supply conditionals
here?

samsung,s6e3ha8-amb630qy01 and samsung,s6e3ha8-amb577px01 are standalone
compatibles with no shared fallback compatible and different power supply
topologies (vddpos-supply and vddneg-supply vs vddr-supply).  Keeping both in
one schema requires per-compatible if-then blocks, and still leaves
vddr-supply allowed on samsung,s6e3ha8-amb630qy01 and vddpos-supply and
vddneg-supply allowed on samsung,s6e3ha8-amb577px01 unless further
constraints are added.

[1] https://lore.kernel.org/r/20260929-automatic-humorous-muskox-45bdfd@quoll

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-crosshatch-panel-v3-0-87dd4ec4f7cc@ixit.cz?part=2