Re: [PATCH v3 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: iio: adc: channel schema inverts adc.yaml differential semantics
- [Medium] dt-bindings: iio: adc: incomplete channel property restriction with adc.yaml
--

commit f0823504c40ee953b211fa5196cd4cbe1be6611c
Author: Chi-Wen Weng <cwweng@nuvoton.com>

dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC

This commit adds the devicetree binding schema for the Nuvoton MA35D1
Enhanced Analog-to-Digital Converter (EADC), describing its interrupts,
clocks, resets, reference supply, DMA support, and channel child nodes.

> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
> new file mode 100644
> index 0000000000000..97144a8eeb5c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml

[ ... ]

> +patternProperties:
> +  '^channel@[0-7]$':
> +    type: object
> +    $ref: /schemas/iio/adc/adc.yaml#
> +    unevaluatedProperties: false
> +    description: |
> +      External EADC channel description.
> +
> +      For a single-ended channel, single-channel specifies the external
> +      analog input number (0 through 7), while reg provides the logical IIO
> +      channel index.
> +
> +      For a differential channel, omit single-channel. The reg value selects
> +      one of the four fixed hardware differential pairs: reg 0 selects 0-4,
> +      reg 1 selects 1-5, reg 2 selects 2-6 and reg 3 selects 3-7.
> +
> +    properties:
> +      reg:
> +        maximum: 7
> +
> +      single-channel:
> +        maximum: 7
> +
> +      # Differential pairs are fixed in hardware and are selected by reg.
> +      diff-channels: false
> +
> +      bipolar: false
> +
> +      common-mode-channel: false
> +
> +      settling-time-us: false
> +
> +      oversampling-ratio: false

[Severity: Medium]
Does using unevaluatedProperties: false here leave other unsupported
properties from Documentation/devicetree/bindings/iio/adc/adc.yaml permitted
in channel@[0-7] child nodes?

Because adc.yaml is referenced with unevaluatedProperties: false and only
five properties are explicitly set to false above, the remaining properties
defined in adc.yaml (reference-sources, input-chopping, excitation-channels,
excitation-current-nanoamp, and burn-out-current-nanoamp) will still pass
schema validation.

Should patternProperties.'^channel@[0-7]$' use additionalProperties: false
instead and list only the supported properties from adc.yaml?

> +
> +    allOf:
> +      - if:
> +          not:
> +            required:
> +              - single-channel
> +        then:
> +          properties:
> +            reg:
> +              maximum: 3

[Severity: Medium]
Doesn't forbidding diff-channels and using the omission of single-channel to
select differential pairs invert the standard semantics in
Documentation/devicetree/bindings/iio/adc/adc.yaml?

In adc.yaml, properties.single-channel specifies:

  single-channel:
    $ref: /schemas/types.yaml#/definitions/uint32
    description:
      When devices combine single-ended and differential channels, allow the
      channel for a single element to be specified, independent of reg (as for
      differential channels). If this and diff-channels are not present reg
      shall be used instead.

Under adc.yaml, a child node with neither single-channel nor diff-channels
(such as channel@1 with only reg = <1> in the example) represents a
single-ended channel on input reg, whereas this schema treats it as
differential pair 1-5.

Additionally, for single-ended channels, reg (0 through 7) acts as a logical
channel index independent of the input pin selected by single-channel, while
for differential channels reg (0 through 3) is forced to act as both the
child node unit address and the hardware differential pair selector. Because
sibling devicetree nodes cannot share the same reg value, a board that uses
reg = <0> for a single-ended channel on another input (for instance,
channel@0 with reg = <0> and single-channel = <2>) cannot also describe
hardware differential pair 0-4, which requires reg = <0>.

Could differential channels use diff-channels constrained to the four valid
hardware pairs (<0 4>, <1 5>, <2 6>, and <3 7>) so that reg remains purely
the logical channel index?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030031501.130290-1-cwweng.linux@gmail.com?part=1