Re: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

Re: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5144 bytes --]

Hey Tobias, Guenter, Jonathan,

On Fri, Aug 30, 2024 at 11:49:53AM +0000, Sperling, Tobias wrote:
> From b2e04ce5500faf274654be5284be9db4f3abefce Mon Sep 17 00:00:00 2001
> From: Tobias Sperling <tobias.sperling@softing.com>
> Date: Fri, 23 Aug 2024 12:08:33 +0200
> Subject: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8
> 
> Add documentation for the driver of ADS7128 and ADS7138 12-bit, 8-channel
> analog-to-digital converters. These ADCs have a wide operating range and
> a wide feature set. Communication is based on an I2C interface.
> The driver provides the functionality of manually reading single channels
> or sequentially reading all channels automatically.
> 
> Signed-off-by: Tobias Sperling <tobias.sperling@softing.com>
> ---
>  .../devicetree/bindings/hwmon/ti,ads71x8.yaml |  85 +++++++++++

If this is a "generic" adc, why is it going into hwmon?
I would have expected this to be in iio/adc, and use more typical adc
bindings, even if the driver is in hwmon.

Guenter/Jonathan wdyt?

>  Documentation/hwmon/ads71x8.rst               | 140 ++++++++++++++++++
>  Documentation/hwmon/index.rst                 |   1 +

And these two documents are not dt-bindings, so they should either be in
their own commit or alongside the driver. Not sure how Guenter likes
things.

>  3 files changed, 226 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ads71x8.yaml

>  create mode 100644 Documentation/hwmon/ads71x8.rst
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ads71x8.yaml b/Documentation/devicetree/bindings/hwmon/ti,ads71x8.yaml
> new file mode 100644
> index 000000000000..e422c4ebd207
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ti,ads71x8.yaml

Please make the filename match a compatible.

> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/ti,ads71x8.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments ADS7128/ADS7138 Analog to Digital Converter (ADC)
> +
> +maintainers:
> +  - None

Nice trick..

> +description: |
> +  The ADS7128 is 12-Bit, 8-Channel Sampling Analog to Digital Converter (ADC)
> +  with an I2C interface.
> +
> +  Datasheets:
> +    https://www.ti.com/product/ADS7128
> +    https://www.ti.com/product/ADS7138
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,ads7128
> +      - ti,ads7138
> +
> +  reg:
> +    maxItems: 1
> +
> +  avdd-supply:

There's also a dvdd on the ads7128.

> +    description:
> +      The regulator used as analog supply voltage as well as reference voltage.
> +
> +  ti,mode:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description: |
> +      Operation mode
> +      Mode 0 - Manual mode. A channel is only sampled when the according input
> +        in the sysfs is read.
> +      Mode 1 - Auto mode. All channels are automatically sampled sequentially.
> +        Reading an input returns the last valid sample. In this mode further
> +        features like statistics and interrupts are available.
> +    default: 0

I don't think this ti,mode property is suitable for bindings. sysfs is a
linux implementation detail, when to do sampling is an implementation
detail of your driver. Bindings are only supposed to describe properties
of the hardware, not set software policy.

> +
> +  ti,interval:
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +    description: |
> +      Only considered in mode 1!
> +      Interval in microseconds a new sample is triggered. Is set to closest
> +      possible interval, see datasheet.

For iio devices, this is usually set from userspace, not from
devicetree, because it is usually not a hardware property, but rather
something a user may want to change at runtime.

> +    default: 1
> +
> +  interrupts:
> +    description: |
> +      Only considered in mode 1!
> +      Interrupt specifier the device's ALERT pin is connected to. Level must be
> +      IRQ_TYPE_LEVEL_LOW. If not configured the digital window comparator (DWC)
> +      is not available.
> +    maxItems: 1

You've got 8 channels on the device, so I would be expecting to see
these described here, with a reference to adc.yaml, even if the only
suitable property is "label".

> +
> +required:
> +  - compatible
> +  - reg
> +  - avdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ads7138@10 {

This should just be "dac@".

> +            compatible = "ti,ads7138";
> +            reg = <0x10>;
> +            avdd-supply = <&reg_stb_3v3>;
> +            ti,mode = /bits/ 8 <1>;
> +            ti,interval = /bits/ 16 <1000>;
> +            interrupt-parent = <&gpio2>;
> +            interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
> +            status = "okay";
> +        };
> +    };
oCheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Guenter Roeck]

On 8/30/24 06:14, Conor Dooley wrote:
> Hey Tobias, Guenter, Jonathan,
> 
> On Fri, Aug 30, 2024 at 11:49:53AM +0000, Sperling, Tobias wrote:
>>  From b2e04ce5500faf274654be5284be9db4f3abefce Mon Sep 17 00:00:00 2001
>> From: Tobias Sperling <tobias.sperling@softing.com>
>> Date: Fri, 23 Aug 2024 12:08:33 +0200
>> Subject: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8
>>
>> Add documentation for the driver of ADS7128 and ADS7138 12-bit, 8-channel
>> analog-to-digital converters. These ADCs have a wide operating range and
>> a wide feature set. Communication is based on an I2C interface.
>> The driver provides the functionality of manually reading single channels
>> or sequentially reading all channels automatically.
>>
>> Signed-off-by: Tobias Sperling <tobias.sperling@softing.com>
>> ---
>>   .../devicetree/bindings/hwmon/ti,ads71x8.yaml |  85 +++++++++++
> 
> If this is a "generic" adc, why is it going into hwmon?
> I would have expected this to be in iio/adc, and use more typical adc
> bindings, even if the driver is in hwmon.
> 
> Guenter/Jonathan wdyt?
> 

Same thought here. While the chip supports limits, making it suitable for
hardware monitoring, its primary use seems to be as ADC, not as hardware
monitoring device. The hardware monitoring API isn't well suited for the
fast sample rate supported by this chip.

Guenter

Re: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Jonathan Cameron]

On Fri, 30 Aug 2024 07:30:16 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 8/30/24 06:14, Conor Dooley wrote:
> > Hey Tobias, Guenter, Jonathan,
> > 
> > On Fri, Aug 30, 2024 at 11:49:53AM +0000, Sperling, Tobias wrote:  
> >>  From b2e04ce5500faf274654be5284be9db4f3abefce Mon Sep 17 00:00:00 2001
> >> From: Tobias Sperling <tobias.sperling@softing.com>
> >> Date: Fri, 23 Aug 2024 12:08:33 +0200
> >> Subject: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8
> >>
> >> Add documentation for the driver of ADS7128 and ADS7138 12-bit, 8-channel
> >> analog-to-digital converters. These ADCs have a wide operating range and
> >> a wide feature set. Communication is based on an I2C interface.
> >> The driver provides the functionality of manually reading single channels
> >> or sequentially reading all channels automatically.
> >>
> >> Signed-off-by: Tobias Sperling <tobias.sperling@softing.com>
> >> ---
> >>   .../devicetree/bindings/hwmon/ti,ads71x8.yaml |  85 +++++++++++  
> > 
> > If this is a "generic" adc, why is it going into hwmon?
> > I would have expected this to be in iio/adc, and use more typical adc
> > bindings, even if the driver is in hwmon.
> > 
> > Guenter/Jonathan wdyt?
> >   
> 
> Same thought here. While the chip supports limits, making it suitable for
> hardware monitoring, its primary use seems to be as ADC, not as hardware
> monitoring device. The hardware monitoring API isn't well suited for the
> fast sample rate supported by this chip.

Agreed, looks like a typical IIO ADC.

If the particular board needs it for hardware monitoring we have
the bridge that should work for that (iio-hwmon).

Jonathan

> 
> Guenter
> 

AW: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Sperling, Tobias]

> On Fri, 30 Aug 2024 07:30:16 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > On 8/30/24 06:14, Conor Dooley wrote:
> > > Hey Tobias, Guenter, Jonathan,
> > >
> > > On Fri, Aug 30, 2024 at 11:49:53AM +0000, Sperling, Tobias wrote:
> > >>  From b2e04ce5500faf274654be5284be9db4f3abefce Mon Sep 17 00:00:00
> 2001
> > >> From: Tobias Sperling <tobias.sperling@softing.com>
> > >> Date: Fri, 23 Aug 2024 12:08:33 +0200
> > >> Subject: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8
> > >>
> > >> Add documentation for the driver of ADS7128 and ADS7138 12-bit, 8-channel
> > >> analog-to-digital converters. These ADCs have a wide operating range and
> > >> a wide feature set. Communication is based on an I2C interface.
> > >> The driver provides the functionality of manually reading single channels
> > >> or sequentially reading all channels automatically.
> > >>
> > >> Signed-off-by: Tobias Sperling <tobias.sperling@softing.com>
> > >> ---
> > >>   .../devicetree/bindings/hwmon/ti,ads71x8.yaml |  85 +++++++++++
> > >
> > > If this is a "generic" adc, why is it going into hwmon?
> > > I would have expected this to be in iio/adc, and use more typical adc
> > > bindings, even if the driver is in hwmon.
> > >
> > > Guenter/Jonathan wdyt?
> > >
> >
> > Same thought here. While the chip supports limits, making it suitable for
> > hardware monitoring, its primary use seems to be as ADC, not as hardware
> > monitoring device. The hardware monitoring API isn't well suited for the
> > fast sample rate supported by this chip.
> 
> Agreed, looks like a typical IIO ADC.
> 
> If the particular board needs it for hardware monitoring we have
> the bridge that should work for that (iio-hwmon).

Just some addition. In theory the chip also provides the possibility to use some
channels as GPIO making it not only work as ADC.
But yes, driver mainly implements reading of the ADC. Will try to make it an
IIO ADC device then.

> Jonathan
> 
> >
> > Guenter
> >

Regards
Tobias

Re: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Jonathan Cameron]

On Mon, 2 Sep 2024 13:04:55 +0000
"Sperling, Tobias" <Tobias.Sperling@Softing.com> wrote:

> > On Fri, 30 Aug 2024 07:30:16 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >   
> > > On 8/30/24 06:14, Conor Dooley wrote:  
> > > > Hey Tobias, Guenter, Jonathan,
> > > >
> > > > On Fri, Aug 30, 2024 at 11:49:53AM +0000, Sperling, Tobias wrote:  
> > > >>  From b2e04ce5500faf274654be5284be9db4f3abefce Mon Sep 17 00:00:00  
> > 2001  
> > > >> From: Tobias Sperling <tobias.sperling@softing.com>
> > > >> Date: Fri, 23 Aug 2024 12:08:33 +0200
> > > >> Subject: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8
> > > >>
> > > >> Add documentation for the driver of ADS7128 and ADS7138 12-bit, 8-channel
> > > >> analog-to-digital converters. These ADCs have a wide operating range and
> > > >> a wide feature set. Communication is based on an I2C interface.
> > > >> The driver provides the functionality of manually reading single channels
> > > >> or sequentially reading all channels automatically.
> > > >>
> > > >> Signed-off-by: Tobias Sperling <tobias.sperling@softing.com>
> > > >> ---
> > > >>   .../devicetree/bindings/hwmon/ti,ads71x8.yaml |  85 +++++++++++  
> > > >
> > > > If this is a "generic" adc, why is it going into hwmon?
> > > > I would have expected this to be in iio/adc, and use more typical adc
> > > > bindings, even if the driver is in hwmon.
> > > >
> > > > Guenter/Jonathan wdyt?
> > > >  
> > >
> > > Same thought here. While the chip supports limits, making it suitable for
> > > hardware monitoring, its primary use seems to be as ADC, not as hardware
> > > monitoring device. The hardware monitoring API isn't well suited for the
> > > fast sample rate supported by this chip.  
> > 
> > Agreed, looks like a typical IIO ADC.
> > 
> > If the particular board needs it for hardware monitoring we have
> > the bridge that should work for that (iio-hwmon).  
> 
> Just some addition. In theory the chip also provides the possibility to use some
> channels as GPIO making it not only work as ADC.
> But yes, driver mainly implements reading of the ADC. Will try to make it an
> IIO ADC device then.
That's fairly common.  If you want to support it then provide the gpio_chip
etc and +CC the relevant maintainers and mailing lists.

Jonathan

> 
> > Jonathan
> >   
> > >
> > > Guenter
> > >  
> 
> Regards
> Tobias

Re: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Jonathan Cameron]

> > +  ti,mode:
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    description: |
> > +      Operation mode
> > +      Mode 0 - Manual mode. A channel is only sampled when the according input
> > +        in the sysfs is read.
> > +      Mode 1 - Auto mode. All channels are automatically sampled sequentially.
> > +        Reading an input returns the last valid sample. In this mode further
> > +        features like statistics and interrupts are available.
> > +    default: 0  
> 
> I don't think this ti,mode property is suitable for bindings. sysfs is a
> linux implementation detail, when to do sampling is an implementation
> detail of your driver. Bindings are only supposed to describe properties
> of the hardware, not set software policy.

Agreed. With an IIO driver this will become a switch based on what usespace
interfaces are enabled.
So if events are on or buffered data capture, enable automode.
If just sysfs reads, then manual mode is fine.


> > +
> > +        ads7138@10 {  
> 
> This should just be "dac@".

adc :)

> 
> > +            compatible = "ti,ads7138";
> > +            reg = <0x10>;
> > +            avdd-supply = <&reg_stb_3v3>;
> > +            ti,mode = /bits/ 8 <1>;
> > +            ti,interval = /bits/ 16 <1000>;
> > +            interrupt-parent = <&gpio2>;
> > +            interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
> > +            status = "okay";
> > +        };
> > +    };  
> oCheers,
> Conor.

AW: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Sperling, Tobias]

> > > +  ti,mode:
> > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > +    description: |
> > > +      Operation mode
> > > +      Mode 0 - Manual mode. A channel is only sampled when the according
> input
> > > +        in the sysfs is read.
> > > +      Mode 1 - Auto mode. All channels are automatically sampled
> sequentially.
> > > +        Reading an input returns the last valid sample. In this mode further
> > > +        features like statistics and interrupts are available.
> > > +    default: 0
> >
> > I don't think this ti,mode property is suitable for bindings. sysfs is a
> > linux implementation detail, when to do sampling is an implementation
> > detail of your driver. Bindings are only supposed to describe properties
> > of the hardware, not set software policy.
> 
> Agreed. With an IIO driver this will become a switch based on what usespace
> interfaces are enabled.
> So if events are on or buffered data capture, enable automode.
> If just sysfs reads, then manual mode is fine.

Not quite sure if I understood you correctly. With the mode I intended to give
control about the sampling behavior.
In manual mode channels are only sampled if they are accessed/read.
In auto mode they are sampled all the time sequentially. This also offers to use
some extended features, like triggering an interrupt if a measurement crosses a
defined limit.
So the mode mainly affects the hardware behavior and just offers the possibility
to catch that in userspace, if configured accordingly, but that's not a must-have.

Anyway, did I understood it correctly, that you suggest to configure the mode
according some symbols in the kconfig and check that with #ifdef? Do you have
the specific symbol names for me or a driver as example, so I can have a look?

Thanks and regards
Tobias

Re: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Jonathan Cameron]

On Mon, 2 Sep 2024 13:24:59 +0000
"Sperling, Tobias" <Tobias.Sperling@Softing.com> wrote:

> > > > +  ti,mode:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > > +    description: |
> > > > +      Operation mode
> > > > +      Mode 0 - Manual mode. A channel is only sampled when the according  
> > input  
> > > > +        in the sysfs is read.
> > > > +      Mode 1 - Auto mode. All channels are automatically sampled  
> > sequentially.  
> > > > +        Reading an input returns the last valid sample. In this mode further
> > > > +        features like statistics and interrupts are available.
> > > > +    default: 0  
> > >
> > > I don't think this ti,mode property is suitable for bindings. sysfs is a
> > > linux implementation detail, when to do sampling is an implementation
> > > detail of your driver. Bindings are only supposed to describe properties
> > > of the hardware, not set software policy.  
> > 
> > Agreed. With an IIO driver this will become a switch based on what usespace
> > interfaces are enabled.
> > So if events are on or buffered data capture, enable automode.
> > If just sysfs reads, then manual mode is fine.  
> 
> Not quite sure if I understood you correctly. With the mode I intended to give
> control about the sampling behavior.
> In manual mode channels are only sampled if they are accessed/read.
> In auto mode they are sampled all the time sequentially. This also offers to use
> some extended features, like triggering an interrupt if a measurement crosses a
> defined limit.
> So the mode mainly affects the hardware behavior and just offers the possibility
> to catch that in userspace, if configured accordingly, but that's not a must-have.
> 
> Anyway, did I understood it correctly, that you suggest to configure the mode
> according some symbols in the kconfig and check that with #ifdef? Do you have
> the specific symbol names for me or a driver as example, so I can have a look?
No, this is not a build time of firmware config question. It is a question of
what the user is 'doing' with the device. Configure the mode according to what
userspace has enabled.

If it enables threshold detection, then turn on continuous mode.
If it enables capture of data via a chardev (so fast path) then turn on continuous
mode.  If neither of those, then run in manual mode.

Jonathan

> 
> Thanks and regards
> Tobias
> 

AW: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Sperling, Tobias]

Hi,
thanks for the feedback in general.

> >  Documentation/hwmon/ads71x8.rst               | 140 ++++++++++++++++++
> >  Documentation/hwmon/index.rst                 |   1 +
> 
> And these two documents are not dt-bindings, so they should either be in
> their own commit or alongside the driver. Not sure how Guenter likes
> things.

Ok, probably misunderstood some documentation then, that everything in
Documentation/ should be a separate commit. Would move it then alongside
the driver, if that's fine from your side.

> > +description: |
> > +  The ADS7128 is 12-Bit, 8-Channel Sampling Analog to Digital Converter (ADC)
> > +  with an I2C interface.
> > +
> > +  Datasheets:
> > +    https://www.ti.com/product/ADS7128
> > +    https://www.ti.com/product/ADS7138
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ti,ads7128
> > +      - ti,ads7138
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  avdd-supply:
> 
> There's also a dvdd on the ads7128.

Yes, but it doesn't affect anything in the driver. Does it still need to be documented then?
 
> oCheers,
> Conor.

Regards
Tobias

Re: AW: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Guenter Roeck]

On 9/2/24 05:58, Sperling, Tobias wrote:
> Hi,
> thanks for the feedback in general.
> 
>>>   Documentation/hwmon/ads71x8.rst               | 140 ++++++++++++++++++
>>>   Documentation/hwmon/index.rst                 |   1 +
>>
>> And these two documents are not dt-bindings, so they should either be in
>> their own commit or alongside the driver. Not sure how Guenter likes
>> things.
> 
> Ok, probably misunderstood some documentation then, that everything in
> Documentation/ should be a separate commit. Would move it then alongside

Sure, but it doesn't say that it should be a _single_ separate commit.

Guenter