| Message ID | BE1P281MB24208CB90AF549578AA5C384EF972@BE1P281MB2420.DEUP281.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] dt-bindings: hwmon: Introduce ADS71x8 | expand |
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 = <®_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.
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
On Fri, Aug 30, 2024 at 11:49:53AM +0000, Sperling, Tobias wrote: > >From b2e04ce5500faf274654be5284be9db4f3abefce Mon Sep 17 00:00:00 2001 Some junk ^^^ above. Please investigate how you send patches. > 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 And all this suggests malformed patch. > > 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 +++++++++++ > Documentation/hwmon/ads71x8.rst | 140 ++++++++++++++++++ > Documentation/hwmon/index.rst | 1 + Please run scripts/checkpatch.pl and fix reported warnings. Then please run and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. > 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 > @@ -0,0 +1,85 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > + Drop blank line > +$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 Fidn a person... otherwise why would we care? > + > +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: > + 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. Use string instead. > + default: 0 > + > + 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. User proper unit-suffix. > + default: 1 > + > + interrupts: > + description: | > + Only considered in mode 1! What is "considered"? Driver considers? This does not matter. Describe the hardware and if this is hardware related, you should have allOf:if:then restricting this. > + 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 > + > +required: > + - compatible > + - reg > + - avdd-supply > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ads7138@10 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "ti,ads7138"; > + reg = <0x10>; > + avdd-supply = <®_stb_3v3>; > + ti,mode = /bits/ 8 <1>; > + ti,interval = /bits/ 16 <1000>; > + interrupt-parent = <&gpio2>; > + interrupts = <12 IRQ_TYPE_LEVEL_LOW>; > + status = "okay"; Drop Best regards, Krzysztof
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 >
> > + 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 = <®_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.
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
> 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
> > > + 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
> On Fri, Aug 30, 2024 at 11:49:53AM +0000, Sperling, Tobias wrote: > > >From b2e04ce5500faf274654be5284be9db4f3abefce Mon Sep 17 00:00:00 > 2001 > > Some junk ^^^ above. Please investigate how you send patches. Yeah also saw this line, but of course tried to apply the patch again after sending it as mail and that worked fine. But just checked again and seems like this line can be dropped. And yes, I sent the patches manually, as we likely have some restrictions for smtp, but as I was able to apply them again it's fine I guess. > > 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 > > And all this suggests malformed patch. Why? If I drop this I'm not able to apply the patch, so I think this should be fine. > > > > 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 +++++++++++ > > Documentation/hwmon/ads71x8.rst | 140 ++++++++++++++++++ > > Documentation/hwmon/index.rst | 1 + > > Please run scripts/checkpatch.pl and fix reported warnings. Then please > run and (probably) fix more warnings. > Some warnings can be ignored, especially from --strict run, but the code > here looks like it needs a fix. Feel free to get in touch if the warning > is not clear. Had done this already before submitting the patches (at least without --strict), but only reports a warning about splitting the patch (which I got wrong here) and updating the maintainers. I guess you were about suggesting a second script to run. Which one is that? > > +$id: > https://deu01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree. > org%2Fschemas%2Fhwmon%2Fti%2Cads71x8.yaml%23&data=05%7C02%7C%7C > ff09fedbe2744394f78508dcc9881ee7%7Cfe3606fad3974238999768dcd7851f64 > %7C1%7C0%7C638606833686313557%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi > MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C > %7C%7C&sdata=vZaCpdaNzELpNNnd6wp5P9MNLQTnAmWXYD%2BNKQYCJ78% > 3D&reserved=0 > > +$schema: > https://deu01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree. > org%2Fmeta- > schemas%2Fcore.yaml%23&data=05%7C02%7C%7Cff09fedbe2744394f78508dcc > 9881ee7%7Cfe3606fad3974238999768dcd7851f64%7C1%7C0%7C63860683368 > 6326954%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu > MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=EJflznuTZYGR > BRjULwohiHk8gPF9iRusSbmF8CKkl5Q%3D&reserved=0 > > + > > +title: Texas Instruments ADS7128/ADS7138 Analog to Digital Converter (ADC) > > + > > +maintainers: > > + - None > > Fidn a person... otherwise why would we care? I know it's not nice, but we are likely not implementing further features, but OK, will add myself then. > > + default: 1 > > + > > + interrupts: > > + description: | > > + Only considered in mode 1! > > What is "considered"? Driver considers? This does not matter. Describe > the hardware and if this is hardware related, you should have > allOf:if:then restricting this. It's possible to define a mode, either manual sampling or autosampling. In the latter mode, also the hardware capabilities change, e.g. the driver is able to trigger an interrupt so defining the interrupt only makes sense in that mode. Will have a look to allOf:if:then then. > > + compatible = "ti,ads7138"; > > + reg = <0x10>; > > + avdd-supply = <®_stb_3v3>; > > + ti,mode = /bits/ 8 <1>; > > + ti,interval = /bits/ 16 <1000>; > > + interrupt-parent = <&gpio2>; > > + interrupts = <12 IRQ_TYPE_LEVEL_LOW>; > > + status = "okay"; > > Drop I guess, I shall only drop the "status" not the whole section? > Best regards, > Krzysztof Regards, Tobias
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
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 >
On 02/09/2024 15:48, Sperling, Tobias wrote: >> On Fri, Aug 30, 2024 at 11:49:53AM +0000, Sperling, Tobias wrote: >>> >From b2e04ce5500faf274654be5284be9db4f3abefce Mon Sep 17 00:00:00 >> 2001 >> >> Some junk ^^^ above. Please investigate how you send patches. > > Yeah also saw this line, but of course tried to apply the patch again after sending it > as mail and that worked fine. But just checked again and seems like this line can be > dropped. > And yes, I sent the patches manually, as we likely have some restrictions for smtp, > but as I was able to apply them again it's fine I guess. > >>> 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 >> >> And all this suggests malformed patch. > > Why? If I drop this I'm not able to apply the patch, so I think this should be fine. OK, it works with b4, but seeing duplicated subject is not expected and might not work with all tools. > >>> >>> 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 +++++++++++ >>> Documentation/hwmon/ads71x8.rst | 140 ++++++++++++++++++ >>> Documentation/hwmon/index.rst | 1 + >> >> Please run scripts/checkpatch.pl and fix reported warnings. Then please >> run and (probably) fix more warnings. >> Some warnings can be ignored, especially from --strict run, but the code >> here looks like it needs a fix. Feel free to get in touch if the warning >> is not clear. > > Had done this already before submitting the patches (at least without --strict), > but only reports a warning about splitting the patch (which I got wrong here) > and updating the maintainers. > I guess you were about suggesting a second script to run. Which one is that? Please split the patches. > >>> +$id: >> https://deu01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree. >> org%2Fschemas%2Fhwmon%2Fti%2Cads71x8.yaml%23&data=05%7C02%7C%7C >> ff09fedbe2744394f78508dcc9881ee7%7Cfe3606fad3974238999768dcd7851f64 >> %7C1%7C0%7C638606833686313557%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi >> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C >> %7C%7C&sdata=vZaCpdaNzELpNNnd6wp5P9MNLQTnAmWXYD%2BNKQYCJ78% >> 3D&reserved=0 >>> +$schema: >> https://deu01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree. >> org%2Fmeta- >> schemas%2Fcore.yaml%23&data=05%7C02%7C%7Cff09fedbe2744394f78508dcc >> 9881ee7%7Cfe3606fad3974238999768dcd7851f64%7C1%7C0%7C63860683368 >> 6326954%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu >> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=EJflznuTZYGR >> BRjULwohiHk8gPF9iRusSbmF8CKkl5Q%3D&reserved=0 >>> + >>> +title: Texas Instruments ADS7128/ADS7138 Analog to Digital Converter (ADC) >>> + >>> +maintainers: >>> + - None >> >> Fidn a person... otherwise why would we care? > > I know it's not nice, but we are likely not implementing further features, but OK, will > add myself then. > >>> + default: 1 >>> + >>> + interrupts: >>> + description: | >>> + Only considered in mode 1! >> >> What is "considered"? Driver considers? This does not matter. Describe >> the hardware and if this is hardware related, you should have >> allOf:if:then restricting this. > > It's possible to define a mode, either manual sampling or autosampling. In the > latter mode, also the hardware capabilities change, e.g. the driver is able to > trigger an interrupt so defining the interrupt only makes sense in that mode. > Will have a look to allOf:if:then then. > >>> + compatible = "ti,ads7138"; >>> + reg = <0x10>; >>> + avdd-supply = <®_stb_3v3>; >>> + ti,mode = /bits/ 8 <1>; >>> + ti,interval = /bits/ 16 <1000>; >>> + interrupt-parent = <&gpio2>; >>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>; >>> + status = "okay"; >> >> Drop > > I guess, I shall only drop the "status" not the whole section? Only status Best regards, Krzysztof
On 9/2/24 09:02, Krzysztof Kozlowski wrote: > On 02/09/2024 15:48, Sperling, Tobias wrote: >>> On Fri, Aug 30, 2024 at 11:49:53AM +0000, Sperling, Tobias wrote: >>>> >From b2e04ce5500faf274654be5284be9db4f3abefce Mon Sep 17 00:00:00 >>> 2001 >>> >>> Some junk ^^^ above. Please investigate how you send patches. >> >> Yeah also saw this line, but of course tried to apply the patch again after sending it >> as mail and that worked fine. But just checked again and seems like this line can be >> dropped. >> And yes, I sent the patches manually, as we likely have some restrictions for smtp, >> but as I was able to apply them again it's fine I guess. >> >>>> 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 >>> >>> And all this suggests malformed patch. >> >> Why? If I drop this I'm not able to apply the patch, so I think this should be fine. > > OK, it works with b4, but seeing duplicated subject is not expected and > might not work with all tools. > >> >>>> >>>> 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 +++++++++++ >>>> Documentation/hwmon/ads71x8.rst | 140 ++++++++++++++++++ >>>> Documentation/hwmon/index.rst | 1 + >>> >>> Please run scripts/checkpatch.pl and fix reported warnings. Then please >>> run and (probably) fix more warnings. >>> Some warnings can be ignored, especially from --strict run, but the code >>> here looks like it needs a fix. Feel free to get in touch if the warning >>> is not clear. >> >> Had done this already before submitting the patches (at least without --strict), >> but only reports a warning about splitting the patch (which I got wrong here) >> and updating the maintainers. >> I guess you were about suggesting a second script to run. Which one is that? > > Please split the patches. > I suspect you may have to be more specific. There need to be two patches in Documentation/, one with the bindings and another one with the driver documentation. Thanks, Guenter
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
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 @@ -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 + +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: + 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 + + 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. + 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 + +required: + - compatible + - reg + - avdd-supply + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ads7138@10 { + compatible = "ti,ads7138"; + reg = <0x10>; + avdd-supply = <®_stb_3v3>; + ti,mode = /bits/ 8 <1>; + ti,interval = /bits/ 16 <1000>; + interrupt-parent = <&gpio2>; + interrupts = <12 IRQ_TYPE_LEVEL_LOW>; + status = "okay"; + }; + }; diff --git a/Documentation/hwmon/ads71x8.rst b/Documentation/hwmon/ads71x8.rst new file mode 100644 index 000000000000..383669c1f8c5 --- /dev/null +++ b/Documentation/hwmon/ads71x8.rst @@ -0,0 +1,140 @@ +.. SPDX-License-Identifier: GPL-2.0-or-later + +Kernel driver ads71x8 +===================== + +Supported chips: + + * Texas Instruments ADS7138 + + Prefix: 'ads7128' + + Datasheet: Publicly available at the Texas Instruments website: + http://focus.ti.com/lit/ds/symlink/ads7128.pdf + + * Texas Instruments ADS7138 + + Prefix: 'ads7138' + + Datasheet: Publicly available at the Texas Instruments website: + http://focus.ti.com/lit/ds/symlink/ads7138.pdf + +Author: Tobias Sperling <tobias.sperling@softing.com> + (based on ads7828 by Steve Hardy) + +Description +----------- + +This driver implements support for the Texas Instruments ADS7128 and ADS7138, +which are 8-channel 12-bit A/D converters. + +The chip requires an external analog supply voltage AVDD which is also used as +reference voltage. If it is missing or too low, the chip won't show up as I2C +device. + +The driver can be run in different modes. In manual mode a new (averaged) sample +is created when the according input is read. + +In auto mode all channels are sampled sequentially automatically. Reading an +input returns the last valid sample. In this mode there are also further +features like statistics and the possibility to trigger an interrupt if a +voltage drops/raises below/above a specific value (DWC - Digital Window +Comparator). +The overall update time (after which all channels are updated) depends on the +number of samples, the update interval and the amount of channels (8). + + update time = samples * update_interval * 8 + +There is no reliable way to identify this chip, so the driver will not scan +some addresses to try to auto-detect it. That means that you will have to +statically declare the device in the device tree. + +sysfs-Interface +--------------- + +The following interfaces are available in all modes. + ++----------------+----+---------------------------------------------+ +| in[0-7]_input | ro | Voltage in mV sampled at channel [0-7] | ++----------------+----+---------------------------------------------+ +| samples | rw | Number of samples used for averaging 1-128. | +| | | Automatically set to closest power of 2. | ++----------------+----+---------------------------------------------+ +| calibrate | rw | Write any value greater than 0 to trigger | +| | | self-calibration. Reads as 0 if finished. | ++----------------+----+---------------------------------------------+ + +If the device is running in auto mode there are also the following interfaces. + ++------------------+----+-----------------------------------------------------+ +| in[0-7]_max | ro | Maximum value in mV that occurred at channel [0-7] | ++------------------+----+-----------------------------------------------------+ +| in[0-7]_min | ro | Minimal value in mV that occurred at channel [0-7] | ++------------------+----+-----------------------------------------------------+ +| update_interval | ro | Time in microseconds after which the next sample is | +| | | executed. | ++------------------+----+-----------------------------------------------------+ + +If the device is running in auto mode and the interrupt is configured also the +following interfaces are added. If CONFIG_SYSFS is set in the kernel +configuration it is also possible to poll the 'alrarms', see example below. + ++--------------------+----+---------------------------------------------------+ +| alarms | ro | | Contains the flags of DWC events. Once read it | +| | | is reset to 0. | +| | | | BIT0 equals the low event flag of channel 0. | +| | | | BIT7 equals the low event flag of channel 7. | +| | | | BIT8 equals the high event flag of channel 0. | +| | | | BIT15 equals the high event flag of channel 7. | ++--------------------+----+---------------------------------------------------+ +| in[0-7]_max_alarm | rw | Set high threshold in mV of DWC for channel [0-7] | ++--------------------+----+---------------------------------------------------+ +| in[0-7]_min_alarm | rw | Set low threshold in mV of DWC for channel [0-7] | ++--------------------+----+---------------------------------------------------+ + +Example +------- + +.. code:: c + + #include <stdio.h> + #include <stdlib.h> + #include <fcntl.h> + #include <sys/select.h> + #include <unistd.h> + + int main(void) + { + int retval, fd; + fd_set exceptfds; + char buf[16]; + + fd = open("/sys/class/hwmon/hwmon1/alarms", O_RDONLY); + + while (1) { + + FD_ZERO(&exceptfds); + FD_SET(fd, &exceptfds); + + /* Must be assigned to 'exceptional conditions'. For poll() use + POLLPRI. */ + retval = select(fd + 1, NULL, NULL, &exceptfds, NULL); + if (retval == -1) + perror("select()"); + else if (retval) { + /* Close and reopen is required, since it's a sysfs file */ + close(fd); + fd = open("/sys/class/hwmon/hwmon1/alarms", O_RDONLY); + retval = read(fd, buf, sizeof(buf)); + printf("Received: %.*s\n", retval,buf); + } + } + + close(fd); + exit(EXIT_SUCCESS); + } + +Notes +----- + +TODO support for GPIOs, ADC hysteresis and counts is missing yet. diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index 913c11390a45..a54df7af27ea 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -33,6 +33,7 @@ Hardware Monitoring Kernel Drivers adm1275 adm9240 adp1050 + ads71x8 ads7828 adt7410 adt7411