[PATCH v3 phy-next 14/16] dt-bindings: phy: lynx-10g: initial document
Vladimir Oltean
vladimir.oltean at nxp.com
Mon Jun 8 05:03:16 PDT 2026
On Fri, Jun 05, 2026 at 03:24:58PM -0500, Rob Herring wrote:
> 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 at 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 at 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.
Thanks for the explanation, it makes sense now.
I ran "dt-mk-schema -j Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml"
and I can see the following, which seems to check out:
"http://devicetree.org/schemas/phy/fsl,lynx-10g.yaml": {
"$filename": "/opt/linux/net-phy-upstreaming/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml",
"$id": "http://devicetree.org/schemas/phy/fsl,lynx-10g.yaml#",
"$schema": "http://devicetree.org/meta-schemas/core.yaml#",
"additionalProperties": false,
"allOf": [
{
{
"if": {
"properties": {
"compatible": {
"items": [
{
"enum": [
"fsl,ls1046a-serdes1",
"fsl,ls1046a-serdes2"
]
}
],
"maxItems": 1,
"minItems": 1,
"type": "array"
}
}
},
"then": {
"required": [
"big-endian"
]
}
}
}
]
}
I can send a v4 replacing the constraint with:
allOf:
- if:
properties:
compatible:
contains:
enum:
- fsl,ls1028a-serdes
- fsl,ls1046a-serdes1
- fsl,ls1046a-serdes2
- fsl,ls1088a-serdes1
- fsl,ls1088a-serdes2
then:
patternProperties:
"^phy@[0-7]$":
properties:
reg:
minimum: 0
maximum: 3
- if:
properties:
compatible:
contains:
enum:
- fsl,ls1046a-serdes1
- fsl,ls1046a-serdes2
then:
properties:
big-endian: true
required:
- big-endian
since you've said that we shouldn't rely on this fixup anymore. I see
you gave me a Reviewed-by and I deliberately am not adding a fallback
compatible, but I can do so anyway.
> > 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?
If you have a lot of time on your hands, the question "was this concern
considered and dismissed, or not considered?" should be able to be
answered.
In the sashiko web UI for the patch set, there is a "View Raw Log" link
for each patch:
https://sashiko.dev/#/patchset/20260603131914.503053-1-vladimir.oltean%40nxp.com
This gives you its reasoning process and its internal findings (which it
tries to dismiss again in a later stage, before reporting them):
https://sashiko.dev/#/log/67856
```json
{
"findings": [
{
"problem": "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.",
"severity": "Medium",
"severity_explanation": "In Device Tree schema validation, the `compatible` property is processed as a JSON string array. Inside an `if` block, standard JSON schema rules apply. Using `properties: compatible: enum:` evaluates whether the array itself exactly equals one of the scalar strings in the enum, which is structurally impossible. As a result, the condition will always evaluate to false, and the `big-endian` requirement for `fsl,ls1046a-serdes1` and `fsl,ls1046a-serdes2` will never be enforced by `dt_binding_check`. The check must be updated to use `properties: compatible: contains: enum:` to correctly evaluate the elements within the array, similar to the preceding `if` block.",
"preexisting": false,
"locations": [
{
"file": "Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml",
"function_or_symbol": null,
"line": 91,
"code_snippet": " - if:\n properties:\n compatible:\n enum:\n - fsl,ls1046a-serdes1\n - fsl,ls1046a-serdes2\n then:\n required:\n - big-endian",
"why_this_location_matters": "This conditional block incorrectly omits `contains:`, rendering the entire schema validation rule for the `big-endian` property ineffective."
}
]
}
]
}
```
> 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.
I don't think that process is in place right now - the learning process
from mailing list discussions is human driven and not autonomous.
Nor would it be immediately obvious how to automate it, if you consider
the risk of pollution from certain reviewers, which you've also pointed
out. But as a reviewer and maintainer, I think you are in a good
position to write your experience down in the dt-schema skill and see
how it goes from there.
More information about the linux-phy
mailing list