Re: [PATCH v4 08/13] dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode

[Marcelo Schmitt]

On 10/06, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] dt-bindings: invalid and incomplete constraints for spi-rx-bus-width
> --
> 
> commit a58d6d0037332b8cd9f5b3717ec4b6ddd8e36734
> Author: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> 
> dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode
> 
> This commit updates the AD4134 device tree bindings to document different SPI 
> connection modes. It introduces the adi,spi-mode and #daisy-chained-devices 
> properties along with corresponding constraints for different wiring 
> configurations.
> 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml
> > index ea6d7e0264190..970e7adb2405b 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml
> [ ... ]
> > @@ -141,13 +161,36 @@ required:
> >    - clocks
> >    - clock-names
> >  
> > -oneOf:
> > -  - required:
> > -      - ldoin-supply
> > -  - required:
> > -      - avdd1v8-supply
> > -      - dvdd1v8-supply
> > -      - clkvdd-supply
> > +allOf:
> > +  - oneOf:
> > +      - required:
> > +          - ldoin-supply
> > +      - required:
> > +          - avdd1v8-supply
> > +          - dvdd1v8-supply
> > +          - clkvdd-supply
> > +
> > +  - oneOf:
> > +      - required: ['adi,spi-mode', '#daisy-chained-devices']
> 
> [Severity: Low]
> Neither chain configuration includes spi-rx-bus-width in the required list.
> 
> Since JSON Schema property constraints only evaluate when the property is 
> present, omitting spi-rx-bus-width from a chain node bypasses the hardware 
> constraint. 
> 
> Should spi-rx-bus-width be added to the required lists so the SPI controller 
> doesn't silently initialize with a single receive lane?

I don't think that would necessarily be a problem.

> 
> > +        properties:
> > +          adi,spi-mode:
> > +            const: one-channel-chain
> > +          spi-rx-bus-width:
> > +            const: 1
> 
> [Severity: Low]
> Because the core SPI bindings define spi-rx-bus-width as a uint32-array, 
> evaluating a device tree value (such as <1>, parsed internally as the JSON 
> array [1]) against a scalar causes a strict type mismatch. 
> 
> Could this falsely reject valid configurations during dt_binding_check?

When spi-rx-bus-width = <1>; the property can be omitted and the pair
#daisy-chained-devices = <N>;
adi,spi-mode = "one-channel-chain";
passes dt_binding_check.

> 
> > +      - required: ['adi,spi-mode', '#daisy-chained-devices']
> > +        properties:
> > +          adi,spi-mode:
> > +            const: two-channel-chain
> > +          spi-rx-bus-width:
> > +            items:
> > +              - const: 1
> > +              - const: 1
> 
> [Severity: Low]
> This array items constraint validates the first two lanes, but it lacks bounds 
> checks. 
> 
> Does this need minItems: 2 and maxItems: 2 to properly restrict the overall 
> length of the array?

I've tried that before preparing v4 and the result was dt_binding_check hints.
'hint: "maxItems" is not needed with an "items" list'
'hint: "minItems" is only needed if less than the "items" list length'

> 
> > +      - properties:
> > +          '#daisy-chained-devices': false
> > +          adi,spi-mode:
> > +            enum:
> > +              - no-cs
> > +              - 4-wire
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791309733.git.marcelo.schmitt@xxxxxxxxxx?part=8