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

[sashiko-bot@kernel.org]

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610012727.848-1-lizhi2@eswincomputing.com?part=2