Re: Re: [PATCH net-next v8 2/6] dt-bindings: ethernet: eswin: add EIC7700 eth1 RX clock inversion variant

[李志 <lizhi2@eswincomputing.com>]

> -----Original Messages-----
> From: sashiko-bot@kernel.org
> Send time:Thursday, 11/06/2026 09:32:49
> To: lizhi2@eswincomputing.com
> Cc: robh@kernel.org, devicetree@vger.kernel.org, conor+dt@kernel.org
> Subject: Re: [PATCH net-next v8 2/6] dt-bindings: ethernet: eswin: add EIC7700 eth1 RX clock inversion variant
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Using a top-level `oneOf` partition for `compatible`-based constraints breaks schema extensibility and violates dt-schema conditional patterns.
> --
> 
> commit c1875936c193e91a73baa1d661559e819f16e710
> Author: Zhi Li <lizhi2@eswincomputing.com>
> 
> dt-bindings: ethernet: eswin: add EIC7700 eth1 RX clock inversion variant
> 
> This commit adds a new compatible string for the EIC7700 eth1 MAC variant
> to handle RX clock inversion requirements. It introduces specific
> tx-internal-delay-ps constraints for each variant using a oneOf block.
> 
> > diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> > index 4e02fedae5c67..8cb7545c56e87 100644
> > --- a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> > +++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> 
> [ ... ]
> 
> >  allOf:
> >    - $ref: snps,dwmac.yaml#
> >  
> > +oneOf:
> > +  - properties:
> > +      compatible:
> > +        contains:
> > +          const: eswin,eic7700-qos-eth
> > +      tx-internal-delay-ps:
> > +        minimum: 0
> > +        maximum: 2540
> > +        multipleOf: 20
> > +
> > +  - properties:
> > +      compatible:
> > +        contains:
> > +          const: eswin,eic7700-qos-eth-clk-inversion
> > +      tx-internal-delay-ps:
> > +        minimum: 2000
> > +        maximum: 4540
> > +        multipleOf: 20
> 
> [Severity: Medium]
> According to the device tree bindings subsystem guidelines, YAML binding 
> schemas typically use allOf with if-then blocks to apply different 
> constraints based on the compatible string.
> 
> Does placing a oneOf block at the root level create a mutually-exclusive 
> validation partition that will break schema extensibility? 
> 
> If a new compatible string is added to the enum in the future, it looks 
> like it might fail the entire oneOf block by not matching either arm, 
> causing the whole document to fail validation.
> 
> Could this be rewritten using allOf and if-then conditionals?
> 
> [ ... ]
> 
> > @@ -67,11 +89,6 @@ properties:
> >      maximum: 2540
> >      multipleOf: 20
> >  
> > -  tx-internal-delay-ps:
> > -    minimum: 0
> > -    maximum: 2540
> > -    multipleOf: 20
> > -
> >    eswin,hsp-sp-csr:
> >      description:
> >        HSP CSR is to control and get status of different high-speed peripherals
> 
> [Severity: Medium]
> Is it intended to remove tx-internal-delay-ps entirely from the main 
> properties block? 
> 
> Removing the property from the root properties block is usually considered 
> a schema anti-pattern. Should the base property definition remain here, 
> with only the range constraints being overridden in the conditional blocks 
> above?
> 
> -- 
I am planning to switch the schema from a top-level oneOf
to an allOf + if/then based approach.

The idea is to:
- define a global tx-internal-delay-ps range in properties (0–4540 ps)
- then apply per-compatible constraints using if/then:
- eswin,eic7700-qos-eth: max 2540 ps
- eswin,eic7700-qos-eth-clk-inversion: min 2000 ps

Does this align with the preferred dt-schema pattern for
modeling variant-specific constraints, compared to using oneOf?