Re: [PATCH v3 phy-next 14/16] dt-bindings: phy: lynx-10g: initial document

[Rob Herring <robh@kernel.org>]

On Fri, Jun 05, 2026 at 11:37:37AM +0300, Vladimir Oltean wrote:
> On Wed, Jun 03, 2026 at 04:50:39PM +0100, Conor Dooley wrote:
> > On Wed, Jun 03, 2026 at 04:38:25PM +0300, Vladimir Oltean wrote:
> > > On Wed, Jun 03, 2026 at 01:33:50PM +0000, sashiko-bot@kernel.org wrote:
> > > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > > - [Medium] The `if` condition for enforcing the `big-endian` property incorrectly matches the `compatible` string array directly against an `enum`, which will always evaluate to false and cause the requirement to silently fail.
> > > > --
> > > > 
> > > > commit 8bd3982a89722b20a3686e42d4ab3b9f0711ae11
> > > > Author: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > 
> > > > dt-bindings: phy: lynx-10g: initial document
> > > > 
> > > > This commit adds the schema for the 10G Lynx SerDes used on most QorIQ and
> > > > Layerscape SoCs. It defines the valid compatible strings, PHY cell requirements,
> > > > and enforces SoC-specific constraints such as lane counts and endianness.
> > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml
> > > > > new file mode 100644
> > > > > index 0000000000000..8cf6027a75b91
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml
> > > > > +  - if:
> > > > > +      properties:
> > > > > +        compatible:
> > > > > +          enum:
> > > > > +            - fsl,ls1046a-serdes1
> > > > > +            - fsl,ls1046a-serdes2
> > > > 
> > > > [Severity: Medium]
> > > > Does this conditional correctly enforce the big-endian property? 
> > > > 
> > > > Since the compatible property is a string array in device trees, evaluating it
> > > > directly against an enum always returns false. Should this use "contains:" 
> > > > with the enum, similar to the preceding if block, so dt_binding_check 
> > > > correctly enforces the requirement?
> > > 
> > > The tests I did suggest that it does enforce the requirement.
> > 
> > Yeah, sashiko just doesn't understand how this works without "contains".
> > "contains" is only actually required for validation purposes if there's
> > fallback compatibles and you're trying to get a partial match.

Not exactly. The missing part is the schema is fixed up so the above is 
converted to:

compatible:
  minItems: 1
  maxItems: 1
  items:
     - enum:
         - fsl,ls1046a-serdes1
         - fsl,ls1046a-serdes2

If we didn't do that, the above is not what we want here, but 'contains' 
is as that's the loosest condition and works regardless of how many 
entries compatible has. That's what you should do here.

This fixup used to be a lot more widespread when we used the DT YAML 
format and everything was decoded to an an array or matrix. The fixups 
saved a lot of boilerplate. That's less true now, so maybe getting rid 
of more fixups like this case would be better.

> Yeah, I have no clear understanding of the json-schema syntax either, I
> just copied from another place where it was clear that the intention was
> to have multiple matches on nodes having a single compatible string each.
> 
> But maybe it would be good from DT bindings maintainers to teach LLMs
> where they get things wrong in this repo?
> https://github.com/masoncl/review-prompts/blob/main/kernel/subsystem/dt-bindings.md

Yes, there's a few things it consistently gets wrong. I've looked at 
this and I can go write more instructions (and fix some things that seem 
wrong), but how do I know if it really works? First, I'd be testing with 
a different LLM as that's what I have access to. Second, how do I know 
if no warning is just the indeterminate nature of LLMs? What's really 
needed is for sashiko to incorporate feedback like any other developer. 
Otherwise, it's going to be like some certain reviewers we've banned.

Rob