linux-kernel - Re: [PATCH 6/7] dt-bindings: iio: adc: add adi,ad7606c-{16,18} compatible strings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+GgBR_V8r0Vz1PeKxwD6ovwHXxGM6=Z6XVd03ehokT5C3zjnQ@mail.gmail.com>
Date: Tue, 20 Aug 2024 07:51:50 +0300
From: Alexandru Ardelean <aardelean@...libre.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, jic23@...nel.org, krzk+dt@...nel.org, 
	robh@...nel.org, lars@...afoo.de, michael.hennerich@...log.com, 
	gstols@...libre.com
Subject: Re: [PATCH 6/7] dt-bindings: iio: adc: add adi,ad7606c-{16,18}
 compatible strings

On Mon, Aug 19, 2024 at 4:09 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On Mon, Aug 19, 2024 at 09:47:16AM +0300, Alexandru Ardelean wrote:
> > The driver will support the AD7606C-16 and AD7606C-18.
> > This change adds the compatible strings for these devices.
> >
> > The AD7606C-16,18 channels also support these (individually configurable)
> > types of channels:
> >  - bipolar single-ended
> >  - unipolar single-ended
> >  - bipolar differential
> >
> > This DT adds support for 'channel@X' nodes'
>
> I don't understand this sentence, suggest to drop it.

Huh?
I guess I'm developing more ADHD, where I forget to finish sentences(?)

>
>
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@...libre.com>
> > ---
> >  .../bindings/iio/adc/adi,ad7606.yaml          | 83 +++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > index 69408cae3db9..f9e177de3f8c 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > @@ -14,6 +14,8 @@ description: |
> >    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7605-4.pdf
> >    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606_7606-6_7606-4.pdf
> >    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7606B.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
> >    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf
> >
> >  properties:
> > @@ -24,6 +26,8 @@ properties:
> >        - adi,ad7606-6
> >        - adi,ad7606-8  # Referred to as AD7606 (without -8) in the datasheet
> >        - adi,ad7606b
> > +      - adi,ad7606c-16
> > +      - adi,ad7606c-18
> >        - adi,ad7616
> >
> >    reg:
> > @@ -114,6 +118,30 @@ properties:
> >        assumed that the pins are hardwired to VDD.
> >      type: boolean
> >
> > +patternProperties:
> > +  "^channel@([0-9a-f])$":
>
> [0-7]
>
> > +    type: object
> > +    $ref: adc.yaml
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        description: The channel number.
> > +        minimum: 0
> > +        maximum: 7
> > +
> > +      diff-channel:
> > +        description: Channel is bipolar differential.
>
> There is diff-channels property, why do we need one more?

Yeah, I wanted to use that.
Maybe I will try another spin at that.
The thing with "diff-channels" is that it requires 2 ints.
So,  diff-channels = <0 0>.
To use it here, a rule would need to be put in place where  "reg ==
diff-channels[0] && reg == diff-channels[1]".
That also works from my side.
Part of the reason for this patchset, was to also get some feedback
(if this is the correct direction).

>
> > +        type: boolean
> > +
> > +      bipolar:
> > +        description: |
> > +          Channel is bipolar single-ended. If 'diff-channel' is set, then
> > +          the value of this property will be ignored.
>
> Then provide here allOf:if:then which makes it false if diff-channel(s)
> is present. And then drop entire property, because you duplicate what's
> in adc.yaml.

Ack.

>
>
> > +        type: boolean
>
> Blank line.
>
> > +    required:
> > +      - reg
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -170,6 +198,21 @@ allOf:
> >          adi,conversion-start-gpios:
> >            maxItems: 1
> >
> > +  - if:
> > +      not:
> > +        properties:
> > +          compatible:
> > +            contains:
> > +              enum:
> > +                - adi,ad7606c-16
> > +                - adi,ad7606c-18
> > +    then:
> > +      patternProperties:
> > +        "^channel@([0-9a-f])$":
> > +          properties:
> > +            diff-channels: false
> > +            bipolar: true
>
> ? Drop, no clue what you want to say here. But more important, you are
> now adding channels to other variants. Split your commit between new
> device and new properties for existing devices.

Ack.
Will do that.

>
>
> Best regards,
> Krzysztof
>