* [PATCH 0/2] Adding support for Microchip MCP3564 ADC family @ 2023-05-19 16:01 marius.cristea 2023-05-19 16:01 ` [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC marius.cristea 2023-05-19 16:01 ` [PATCH 2/2] iio: adc: adding support for " marius.cristea 0 siblings, 2 replies; 16+ messages in thread From: marius.cristea @ 2023-05-19 16:01 UTC (permalink / raw) To: jic23, lars, robh+dt; +Cc: linux-iio, devicetree, linux-kernel, marius.cristea From: Marius Cristea <marius.cristea@microchip.com> Adding support for Microchip family of 153.6 ksps, Low-Noise 16/24-Bit Delta-Sigma ADCs with an SPI interface. This driver covers the following part numbers: - MCP3561, MCP3562, MCP3564, MCP3561R, MCP3562R, MCP3564R, - MCP3461, MCP3462, MCP3464, MCP3461R, MCP3462R and MCP3464R. Differences related to previous patch: v0: - first version comitted to review Marius Cristea (2): dt-bindings: iio: adc: adding MCP3564 ADC iio: adc: adding support for MCP3564 ADC .../ABI/testing/sysfs-bus-iio-mcp3564 | 143 ++ .../bindings/iio/adc/microchip,mcp3564.yaml | 88 ++ MAINTAINERS | 7 + drivers/iio/adc/Kconfig | 13 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/mcp3564.c | 1395 +++++++++++++++++ 6 files changed, 1647 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-mcp3564 create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml create mode 100644 drivers/iio/adc/mcp3564.c base-commit: a508fbfed333aea05e4ac42d98803a031cda8ce8 -- 2.34.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC 2023-05-19 16:01 [PATCH 0/2] Adding support for Microchip MCP3564 ADC family marius.cristea @ 2023-05-19 16:01 ` marius.cristea 2023-05-19 18:29 ` Conor Dooley ` (2 more replies) 2023-05-19 16:01 ` [PATCH 2/2] iio: adc: adding support for " marius.cristea 1 sibling, 3 replies; 16+ messages in thread From: marius.cristea @ 2023-05-19 16:01 UTC (permalink / raw) To: jic23, lars, robh+dt; +Cc: linux-iio, devicetree, linux-kernel, marius.cristea From: Marius Cristea <marius.cristea@microchip.com> This is the device tree schema for iio driver for Microchip family of 153.6 ksps, Low-Noise 16/24-Bit Delta-Sigma ADCs with an SPI interface. Signed-off-by: Marius Cristea <marius.cristea@microchip.com> --- .../bindings/iio/adc/microchip,mcp3564.yaml | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml new file mode 100644 index 000000000000..407a125e3776 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml @@ -0,0 +1,88 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/microchip,mcp3564.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Microchip MCP346X and MCP356X ADC Family + +maintainers: + - Marius Cristea <marius.cristea@microchip.com> + +description: | + Bindings for the Microchip family of 153.6 ksps, Low-Noise 16/24-Bit + Delta-Sigma ADCs with an SPI interface. + +properties: + compatible: + enum: + - microchip,mcp3461 + - microchip,mcp3462 + - microchip,mcp3464 + - microchip,mcp3461r + - microchip,mcp3462r + - microchip,mcp3464r + - microchip,mcp3561 + - microchip,mcp3562 + - microchip,mcp3564 + - microchip,mcp3561r + - microchip,mcp3562r + - microchip,mcp3564r + + reg: + maxItems: 1 + + spi-max-frequency: true + + spi-cpha: true + + spi-cpol: true + + vref-supply: + description: + Some devices have a specific reference voltage supplied on a different + pin to the other supplies. Needed to be able to establish channel scaling + unless there is also an internal reference available (e.g. mcp3564r) + + microchip,hw-device-address: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 3 + description: + The address is set on a per-device basis by fuses in the factory, + configured on request. If not requested, the fuses are set for 0x1. + The device address is part of the device markings to avoid + potential confusion. This address is coded on two bits, so four possible + addresses are available when multiple devices are present on the same + SPI bus with only one Chip Select line for all devices. + + "#io-channel-cells": + const: 1 + +dependencies: + spi-cpol: [ spi-cpha ] + spi-cpha: [ spi-cpol ] + +required: + - compatible + - microchip,hw-device-address + +additionalProperties: false + +examples: + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "microchip,mcp3564r"; + reg = <0>; + vref-supply = <&vref_reg>; + spi-cpha; + spi-cpol; + spi-max-frequency = <10000000>; + microchip,hw-device-address = <1>; + }; + }; +... -- 2.34.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC 2023-05-19 16:01 ` [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC marius.cristea @ 2023-05-19 18:29 ` Conor Dooley 2023-05-20 15:17 ` Jonathan Cameron 2023-05-20 13:20 ` Jonathan Cameron 2023-05-24 10:37 ` Mike Looijmans 2 siblings, 1 reply; 16+ messages in thread From: Conor Dooley @ 2023-05-19 18:29 UTC (permalink / raw) To: marius.cristea; +Cc: jic23, lars, robh+dt, linux-iio, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1917 bytes --] Hey Marius, On Fri, May 19, 2023 at 07:01:44PM +0300, marius.cristea@microchip.com wrote: > From: Marius Cristea <marius.cristea@microchip.com> > > This is the device tree schema for iio driver for > Microchip family of 153.6 ksps, Low-Noise 16/24-Bit > Delta-Sigma ADCs with an SPI interface. Just one quick process bit, please try to CC all of the maintainers listed by get_maintainer.pl - you unfortunately managed to miss 2 of the 3 dt-binding maintainers :/ Perhaps you ran get_maintainer.pl using our vendor tree? > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > --- > + vref-supply: > + description: > + Some devices have a specific reference voltage supplied on a different > + pin to the other supplies. Needed to be able to establish channel scaling > + unless there is also an internal reference available (e.g. mcp3564r) Should this be marked as a required property for the non-r devices that do not have an internal reference? > + microchip,hw-device-address: Hopefully Rob or Jonathan etc can chime in as to whether a common property exists for this type of thing... > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 3 > + description: > + The address is set on a per-device basis by fuses in the factory, > + configured on request. If not requested, the fuses are set for 0x1. > + The device address is part of the device markings to avoid > + potential confusion. This address is coded on two bits, so four possible > + addresses are available when multiple devices are present on the same > + SPI bus with only one Chip Select line for all devices. ..although if it doesn't, it'd be good, I think, to add here where the property crops up in spi transfers. And if not in the description, in the commit message instead? Thanks, Conor. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC 2023-05-19 18:29 ` Conor Dooley @ 2023-05-20 15:17 ` Jonathan Cameron 2023-06-08 19:34 ` Rob Herring 0 siblings, 1 reply; 16+ messages in thread From: Jonathan Cameron @ 2023-05-20 15:17 UTC (permalink / raw) To: Conor Dooley Cc: marius.cristea, lars, robh+dt, linux-iio, devicetree, linux-kernel On Fri, 19 May 2023 19:29:15 +0100 Conor Dooley <conor@kernel.org> wrote: > Hey Marius, > > On Fri, May 19, 2023 at 07:01:44PM +0300, marius.cristea@microchip.com wrote: > > From: Marius Cristea <marius.cristea@microchip.com> > > > > This is the device tree schema for iio driver for > > Microchip family of 153.6 ksps, Low-Noise 16/24-Bit > > Delta-Sigma ADCs with an SPI interface. > > Just one quick process bit, please try to CC all of the maintainers > listed by get_maintainer.pl - you unfortunately managed to miss 2 of the > 3 dt-binding maintainers :/ Perhaps you ran get_maintainer.pl using our > vendor tree? > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > --- > > > + vref-supply: > > + description: > > + Some devices have a specific reference voltage supplied on a different > > + pin to the other supplies. Needed to be able to establish channel scaling > > + unless there is also an internal reference available (e.g. mcp3564r) > > Should this be marked as a required property for the non-r devices that > do not have an internal reference? > > > + microchip,hw-device-address: > > Hopefully Rob or Jonathan etc can chime in as to whether a common > property exists for this type of thing... > Nope. This is a new one for me - there are devices that work on a daisy chain principle but I think this one works by encoding stuff in the actual message which is unusual for SPI. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + minimum: 0 > > + maximum: 3 > > + description: > > + The address is set on a per-device basis by fuses in the factory, > > + configured on request. If not requested, the fuses are set for 0x1. > > + The device address is part of the device markings to avoid > > + potential confusion. This address is coded on two bits, so four possible > > + addresses are available when multiple devices are present on the same > > + SPI bus with only one Chip Select line for all devices. > > ..although if it doesn't, it'd be good, I think, to add here where the > property crops up in spi transfers. And if not in the description, in > the commit message instead? Agreed. Top two bits of COMMAND BYTE which is first one on the wire I think. Thanks, J > > Thanks, > Conor. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC 2023-05-20 15:17 ` Jonathan Cameron @ 2023-06-08 19:34 ` Rob Herring 2023-06-09 17:41 ` Jonathan Cameron 0 siblings, 1 reply; 16+ messages in thread From: Rob Herring @ 2023-06-08 19:34 UTC (permalink / raw) To: Jonathan Cameron Cc: Conor Dooley, marius.cristea, lars, linux-iio, devicetree, linux-kernel On Sat, May 20, 2023 at 04:17:53PM +0100, Jonathan Cameron wrote: > On Fri, 19 May 2023 19:29:15 +0100 > Conor Dooley <conor@kernel.org> wrote: > > > Hey Marius, > > > > On Fri, May 19, 2023 at 07:01:44PM +0300, marius.cristea@microchip.com wrote: > > > From: Marius Cristea <marius.cristea@microchip.com> > > > > > > This is the device tree schema for iio driver for > > > Microchip family of 153.6 ksps, Low-Noise 16/24-Bit > > > Delta-Sigma ADCs with an SPI interface. > > > > Just one quick process bit, please try to CC all of the maintainers > > listed by get_maintainer.pl - you unfortunately managed to miss 2 of the > > 3 dt-binding maintainers :/ Perhaps you ran get_maintainer.pl using our > > vendor tree? > > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > > --- > > > > > + vref-supply: > > > + description: > > > + Some devices have a specific reference voltage supplied on a different > > > + pin to the other supplies. Needed to be able to establish channel scaling > > > + unless there is also an internal reference available (e.g. mcp3564r) > > > > Should this be marked as a required property for the non-r devices that > > do not have an internal reference? > > > > > + microchip,hw-device-address: > > > > Hopefully Rob or Jonathan etc can chime in as to whether a common > > property exists for this type of thing... > > > Nope. This is a new one for me - there are devices that work on a daisy chain > principle but I think this one works by encoding stuff in the actual message > which is unusual for SPI. Not something I've seen either. > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + minimum: 0 > > > + maximum: 3 > > > + description: > > > + The address is set on a per-device basis by fuses in the factory, > > > + configured on request. If not requested, the fuses are set for 0x1. > > > + The device address is part of the device markings to avoid > > > + potential confusion. This address is coded on two bits, so four possible > > > + addresses are available when multiple devices are present on the same > > > + SPI bus with only one Chip Select line for all devices. What's this going to look like with more than one device? It would need to be incorporated into 'reg' and the unit-address to work. Something like this is spi { device0@0 { reg = <0>; microchip,hw-device-address = <0>; }; device1@0 { reg = <0>; microchip,hw-device-address = <1>; }; }; That should throw warnings because you have 2 nodes at the same address which is not good practice. I think you need a spi mux in here with the mux addresses being the microchip,hw-device-address values. Rob ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC 2023-06-08 19:34 ` Rob Herring @ 2023-06-09 17:41 ` Jonathan Cameron 0 siblings, 0 replies; 16+ messages in thread From: Jonathan Cameron @ 2023-06-09 17:41 UTC (permalink / raw) To: Rob Herring Cc: Jonathan Cameron, Conor Dooley, marius.cristea, lars, linux-iio, devicetree, linux-kernel, Chris Packham, Mark Brown On Thu, 8 Jun 2023 13:34:43 -0600 Rob Herring <robh@kernel.org> wrote: > On Sat, May 20, 2023 at 04:17:53PM +0100, Jonathan Cameron wrote: > > On Fri, 19 May 2023 19:29:15 +0100 > > Conor Dooley <conor@kernel.org> wrote: > > > > > Hey Marius, > > > > > > On Fri, May 19, 2023 at 07:01:44PM +0300, marius.cristea@microchip.com wrote: > > > > From: Marius Cristea <marius.cristea@microchip.com> > > > > > > > > This is the device tree schema for iio driver for > > > > Microchip family of 153.6 ksps, Low-Noise 16/24-Bit > > > > Delta-Sigma ADCs with an SPI interface. > > > > > > Just one quick process bit, please try to CC all of the maintainers > > > listed by get_maintainer.pl - you unfortunately managed to miss 2 of the > > > 3 dt-binding maintainers :/ Perhaps you ran get_maintainer.pl using our > > > vendor tree? > > > > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > > > --- > > > > > > > + vref-supply: > > > > + description: > > > > + Some devices have a specific reference voltage supplied on a different > > > > + pin to the other supplies. Needed to be able to establish channel scaling > > > > + unless there is also an internal reference available (e.g. mcp3564r) > > > > > > Should this be marked as a required property for the non-r devices that > > > do not have an internal reference? > > > > > > > + microchip,hw-device-address: > > > > > > Hopefully Rob or Jonathan etc can chime in as to whether a common > > > property exists for this type of thing... > > > > > Nope. This is a new one for me - there are devices that work on a daisy chain > > principle but I think this one works by encoding stuff in the actual message > > which is unusual for SPI. > > Not something I've seen either. > > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + minimum: 0 > > > > + maximum: 3 > > > > + description: > > > > + The address is set on a per-device basis by fuses in the factory, > > > > + configured on request. If not requested, the fuses are set for 0x1. > > > > + The device address is part of the device markings to avoid > > > > + potential confusion. This address is coded on two bits, so four possible > > > > + addresses are available when multiple devices are present on the same > > > > + SPI bus with only one Chip Select line for all devices. > > What's this going to look like with more than one device? It would need > to be incorporated into 'reg' and the unit-address to work. Something > like this is > > spi { > device0@0 { > reg = <0>; > microchip,hw-device-address = <0>; > }; > > device1@0 { > reg = <0>; > microchip,hw-device-address = <1>; > }; > }; > > That should throw warnings because you have 2 nodes at the same address > which is not good practice. > > I think you need a spi mux in here with the mux addresses being the > microchip,hw-device-address values. Something that looked like an spi-mux would be cute I'm not sure how easy it would be to make it work given need to modify the messages (rather sending extra ones before and after with a different chip select). It would be nice if it were somewhat generic - so binding included which bits would be replaced with the 'chip select' for the spi controller that represents the mux. Jonathan > > Rob > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC 2023-05-19 16:01 ` [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC marius.cristea 2023-05-19 18:29 ` Conor Dooley @ 2023-05-20 13:20 ` Jonathan Cameron 2023-05-24 10:37 ` Mike Looijmans 2 siblings, 0 replies; 16+ messages in thread From: Jonathan Cameron @ 2023-05-20 13:20 UTC (permalink / raw) To: marius.cristea; +Cc: lars, robh+dt, linux-iio, devicetree, linux-kernel On Fri, 19 May 2023 19:01:44 +0300 <marius.cristea@microchip.com> wrote: > From: Marius Cristea <marius.cristea@microchip.com> > > This is the device tree schema for iio driver for > Microchip family of 153.6 ksps, Low-Noise 16/24-Bit > Delta-Sigma ADCs with an SPI interface. > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> Hi Marius, A few comments inline > --- > .../bindings/iio/adc/microchip,mcp3564.yaml | 88 +++++++++++++++++++ > 1 file changed, 88 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml > > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml > new file mode 100644 > index 000000000000..407a125e3776 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml > @@ -0,0 +1,88 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/microchip,mcp3564.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip MCP346X and MCP356X ADC Family > + > +maintainers: > + - Marius Cristea <marius.cristea@microchip.com> > + > +description: | > + Bindings for the Microchip family of 153.6 ksps, Low-Noise 16/24-Bit > + Delta-Sigma ADCs with an SPI interface. > + > +properties: > + compatible: > + enum: > + - microchip,mcp3461 > + - microchip,mcp3462 > + - microchip,mcp3464 > + - microchip,mcp3461r > + - microchip,mcp3462r > + - microchip,mcp3464r > + - microchip,mcp3561 > + - microchip,mcp3562 > + - microchip,mcp3564 > + - microchip,mcp3561r > + - microchip,mcp3562r > + - microchip,mcp3564r > + > + reg: > + maxItems: 1 > + > + spi-max-frequency: true If there is a device constraint on this (normally is) then good to enforce it from the binding. Naturally wiring / host side limits may constrain it more. > + > + spi-cpha: true > + > + spi-cpol: true > + > + vref-supply: > + description: > + Some devices have a specific reference voltage supplied on a different > + pin to the other supplies. Needed to be able to establish channel scaling > + unless there is also an internal reference available (e.g. mcp3564r) > + From a quick glance at a random datasheet, looks like there additional power supplies that should be required. If this is required for some devices, I'd expect to see the binding enforce that with some required entries conditioned on the compatibles rather than as documentation. If there are devices where it isn't even optional then the binding should enforce that as well. > + microchip,hw-device-address: > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 3 > + description: > + The address is set on a per-device basis by fuses in the factory, > + configured on request. If not requested, the fuses are set for 0x1. > + The device address is part of the device markings to avoid > + potential confusion. This address is coded on two bits, so four possible > + addresses are available when multiple devices are present on the same > + SPI bus with only one Chip Select line for all devices. > + > + "#io-channel-cells": > + const: 1 > + > +dependencies: > + spi-cpol: [ spi-cpha ] > + spi-cpha: [ spi-cpol ] > + > +required: > + - compatible > + - microchip,hw-device-address Supplies should also be required (unless optional like vref seems to sometimes be for these parts - where it's required I expect the binding to enforce that.) > + > +additionalProperties: false For SPI devices, the newer way of providing standard SPI properties is: allOf: - $ref: /schemas/spi/spi-peripheral-props.yaml# unevaluatedProperties: false https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/devicetree/bindings/iio/adc/adi,ad7298.yaml#L33 for example > + > +examples: > + - | > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "microchip,mcp3564r"; > + reg = <0>; > + vref-supply = <&vref_reg>; > + spi-cpha; > + spi-cpol; > + spi-max-frequency = <10000000>; > + microchip,hw-device-address = <1>; > + }; > + }; > +... ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC 2023-05-19 16:01 ` [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC marius.cristea 2023-05-19 18:29 ` Conor Dooley 2023-05-20 13:20 ` Jonathan Cameron @ 2023-05-24 10:37 ` Mike Looijmans 2023-05-28 19:16 ` Jonathan Cameron 2 siblings, 1 reply; 16+ messages in thread From: Mike Looijmans @ 2023-05-24 10:37 UTC (permalink / raw) To: marius.cristea, jic23, lars, robh+dt; +Cc: linux-iio, devicetree, linux-kernel On 19-05-2023 18:01, marius.cristea@microchip.com wrote: > From: Marius Cristea <marius.cristea@microchip.com> > > This is the device tree schema for iio driver for > Microchip family of 153.6 ksps, Low-Noise 16/24-Bit > Delta-Sigma ADCs with an SPI interface. > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > --- > .../bindings/iio/adc/microchip,mcp3564.yaml | 88 +++++++++++++++++++ > 1 file changed, 88 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml > > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml > new file mode 100644 > index 000000000000..407a125e3776 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml > @@ -0,0 +1,88 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/microchip,mcp3564.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip MCP346X and MCP356X ADC Family > + > +maintainers: > + - Marius Cristea <marius.cristea@microchip.com> > + > +description: | > + Bindings for the Microchip family of 153.6 ksps, Low-Noise 16/24-Bit > + Delta-Sigma ADCs with an SPI interface. > + > +properties: > + compatible: > + enum: > + - microchip,mcp3461 > + - microchip,mcp3462 > + - microchip,mcp3464 > + - microchip,mcp3461r > + - microchip,mcp3462r > + - microchip,mcp3464r > + - microchip,mcp3561 > + - microchip,mcp3562 > + - microchip,mcp3564 > + - microchip,mcp3561r > + - microchip,mcp3562r > + - microchip,mcp3564r > + > + reg: > + maxItems: 1 > + > + spi-max-frequency: true > + > + spi-cpha: true > + > + spi-cpol: true > + > + vref-supply: > + description: > + Some devices have a specific reference voltage supplied on a different > + pin to the other supplies. Needed to be able to establish channel scaling > + unless there is also an internal reference available (e.g. mcp3564r) > + > + microchip,hw-device-address: > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 3 > + description: > + The address is set on a per-device basis by fuses in the factory, > + configured on request. If not requested, the fuses are set for 0x1. > + The device address is part of the device markings to avoid > + potential confusion. This address is coded on two bits, so four possible > + addresses are available when multiple devices are present on the same > + SPI bus with only one Chip Select line for all devices. > + > + "#io-channel-cells": That's a typo, "io" should be "iio", right? > + const: 1 > + > +dependencies: > + spi-cpol: [ spi-cpha ] > + spi-cpha: [ spi-cpol ] > + > +required: > + - compatible > + - microchip,hw-device-address Should be optional, as the default "1" is going to be what most people will be using. > + > +additionalProperties: false Missing properties: - Interrupt (even though the driver doesn't use it yet) - push-pull or open-drain config of interrupt pin - dvdd-supply - avdd-supply I suggest using this: + clocks: + description: + Phandle and clock identifier for external sampling clock. + If not specified, the internal crystal oscillator will be used. + maxItems: 1 + + interrupts: + description: IRQ line of the ADC + maxItems: 1 + + drive-open-drain: + description: + Whether to drive the IRQ signal as push-pull (default) or open-drain. Note + that the device requires this pin to become "high", otherwise it will stop + converting. + type: boolean > + > +examples: > + - | > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "microchip,mcp3564r"; > + reg = <0>; > + vref-supply = <&vref_reg>; > + spi-cpha; > + spi-cpol; > + spi-max-frequency = <10000000>; > + microchip,hw-device-address = <1>; > + }; > + }; > +... -- Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topic.nl W: www.topic.nl ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC 2023-05-24 10:37 ` Mike Looijmans @ 2023-05-28 19:16 ` Jonathan Cameron 0 siblings, 0 replies; 16+ messages in thread From: Jonathan Cameron @ 2023-05-28 19:16 UTC (permalink / raw) To: Mike Looijmans Cc: marius.cristea, lars, robh+dt, linux-iio, devicetree, linux-kernel On Wed, 24 May 2023 12:37:43 +0200 Mike Looijmans <mike.looijmans@topic.nl> wrote: > On 19-05-2023 18:01, marius.cristea@microchip.com wrote: > > From: Marius Cristea <marius.cristea@microchip.com> > > > > This is the device tree schema for iio driver for > > Microchip family of 153.6 ksps, Low-Noise 16/24-Bit > > Delta-Sigma ADCs with an SPI interface. > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > --- > > .../bindings/iio/adc/microchip,mcp3564.yaml | 88 +++++++++++++++++++ > > 1 file changed, 88 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml > > new file mode 100644 > > index 000000000000..407a125e3776 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml > > @@ -0,0 +1,88 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/adc/microchip,mcp3564.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Microchip MCP346X and MCP356X ADC Family > > + > > +maintainers: > > + - Marius Cristea <marius.cristea@microchip.com> > > + > > +description: | > > + Bindings for the Microchip family of 153.6 ksps, Low-Noise 16/24-Bit > > + Delta-Sigma ADCs with an SPI interface. > > + > > +properties: > > + compatible: > > + enum: > > + - microchip,mcp3461 > > + - microchip,mcp3462 > > + - microchip,mcp3464 > > + - microchip,mcp3461r > > + - microchip,mcp3462r > > + - microchip,mcp3464r > > + - microchip,mcp3561 > > + - microchip,mcp3562 > > + - microchip,mcp3564 > > + - microchip,mcp3561r > > + - microchip,mcp3562r > > + - microchip,mcp3564r > > + > > + reg: > > + maxItems: 1 > > + > > + spi-max-frequency: true > > + > > + spi-cpha: true > > + > > + spi-cpol: true > > + > > + vref-supply: > > + description: > > + Some devices have a specific reference voltage supplied on a different > > + pin to the other supplies. Needed to be able to establish channel scaling > > + unless there is also an internal reference available (e.g. mcp3564r) > > + > > + microchip,hw-device-address: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + minimum: 0 > > + maximum: 3 > > + description: > > + The address is set on a per-device basis by fuses in the factory, > > + configured on request. If not requested, the fuses are set for 0x1. > > + The device address is part of the device markings to avoid > > + potential confusion. This address is coded on two bits, so four possible > > + addresses are available when multiple devices are present on the same > > + SPI bus with only one Chip Select line for all devices. > > + > > + "#io-channel-cells": > > That's a typo, "io" should be "iio", right? Nope. I honestly can't remember the history behind that, but it's just io. Probably to do with IIO being Linux specific and this needing to be generic, but its just an educated guess. Jonathan ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] iio: adc: adding support for MCP3564 ADC 2023-05-19 16:01 [PATCH 0/2] Adding support for Microchip MCP3564 ADC family marius.cristea 2023-05-19 16:01 ` [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC marius.cristea @ 2023-05-19 16:01 ` marius.cristea 2023-05-20 15:15 ` Jonathan Cameron 2023-05-24 11:05 ` Mike Looijmans 1 sibling, 2 replies; 16+ messages in thread From: marius.cristea @ 2023-05-19 16:01 UTC (permalink / raw) To: jic23, lars, robh+dt; +Cc: linux-iio, devicetree, linux-kernel, marius.cristea [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="y", Size: 49453 bytes --] From: Marius Cristea <marius.cristea@microchip.com> This is the iio driver for Microchip family of 153.6 ksps, Low-Noise 16/24-Bit Delta-Sigma ADCs with an SPI interface. Signed-off-by: Marius Cristea <marius.cristea@microchip.com> --- .../ABI/testing/sysfs-bus-iio-mcp3564 | 143 ++ MAINTAINERS | 7 + drivers/iio/adc/Kconfig | 13 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/mcp3564.c | 1395 +++++++++++++++++ 5 files changed, 1559 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-mcp3564 create mode 100644 drivers/iio/adc/mcp3564.c diff --git a/Documentation/ABI/testing/sysfs-bus-iio-mcp3564 b/Documentation/ABI/testing/sysfs-bus-iio-mcp3564 new file mode 100644 index 000000000000..fd65995e2245 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-iio-mcp3564 @@ -0,0 +1,143 @@ +What: /sys/bus/iio/devices/iio:deviceX/calibbias +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + Offset Error Digital Calibration Code (two’s complement, + MSb first coding). The device includes a digital calibration + feature for offset and gain errors. The calibration + scheme for offset error consists of the addition of a + fixed offset value to the ADC output code + + This attribute is used to set the offset error digital + calibration code (two’s complement, MSb first coding). + The ADC includes a digital calibration feature for offset + errors. The calibration scheme for offset error consists of the + addition of a fixed offset value to the ADC output code. + +What: /sys/bus/iio/devices/iio:deviceX/calibbias_available +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + Reading returns a range with the possible values for the offset + error digital calibration register. + +What: /sys/bus/iio/devices/iio:deviceX/calibscale +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + This attribute is used to set the gain error digital calibration + code (unsigned, MSb first coding). The default value is 800000, + which provides a gain of 1x. + The ADC includes a digital calibration feature for gain errors. + The calibration scheme for gain error consists of the + multiplication of a fixed gain value to the ADC data code. + +What: /sys/bus/iio/devices/iio:deviceX/calibscale_available +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + Reading returns a range with the possible values for the gain + error digital calibration register. + +What: /sys/bus/iio/devices/iio:deviceX/boost_current +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + This attribute is used to set the biasing circuit of the + Delta-Sigma modulator. The different BOOST settings are applied + to the entire modulator circuit, including the voltage reference + buffers. + +What: /sys/bus/iio/devices/iio:deviceX/boost_current_available +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + Reading returns a list with the possible values for + the current biasing circuit of the Delta-Sigma modulator. + + "x0.5", - ADC channel has current x 0.5 + + "x0.66", - ADC channel has current x 0.66 + + "x1", - ADC channel has current x 1 (default) + + "x2" - ADC channel has current x 2 + +What: /sys/bus/iio/devices/iio:deviceX/current_bias +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + This attribute is used to set the current Source/Sink + values for sensor bias. The ADC inputs, VIN-/VIN+, feature a + selectable burnout current source, which enables open or + short-circuit detection, as well as biasing very low-current + external sensors. The bias current is sourced on the VIN+ pin of + the ADC (noninverting output of the analog multiplexer) + and sunk on the VIN- pin of the ADC (inverting output of + the analog multiplexer). Since the same current flows + at the VIN-/VIN+ pins of the ADC, it can sense the + impedance of an externally connected sensor that + would be connected between the selected inputs of the + multiplexer. + +What: /sys/bus/iio/devices/iio:deviceX/current_bias_available +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + Reading returns a list with the possible values for + the current source/sink selection values for sensor bias. + + "15_uA" - 15 uA is applied to the ADC inputs. + + "3.7_uA" - 3.7 uA is applied to the ADC inputs. + + "0.9_uA" - 0.9 uA is applied to the ADC inputs. + + "no_current" - "No current source is applied to the ADC + inputs (default)" + +What: /sys/bus/iio/devices/iio:deviceX/enable_auto_zeroing_mux +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + This attribute is used to enable the analog input multiplexer + auto-zeroing algorithm. Write '1' to enable it, write '0' to + disable it. + +What: /sys/bus/iio/devices/iio:deviceX/enable_auto_zeroing_ref +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + This attribute is used to enable the chopping algorithm for the + internal voltage reference buffer. Write '1' to enable it, + write '0' to disable it. + +What: /sys/bus/iio/devices/iio:deviceX/hardwaregain +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + This attribute is used to set the hardware applied gain factor. + It is shared across all channels. + +What: /sys/bus/iio/devices/iio:deviceX/hardwaregain_available +KernelVersion: 6.4 +Contact: linux-iio@vger.kernel.org +Description: + Reading this attribute will list all available hardware applied gain factors. + Shared across all channels. + + "0.33" - Gain is x1/3 ( x0.33 analog) + + "1" - Gain is x1 ( x8 analog) (default) + + "2" - Gain is x2 ( x2 analog) + + "4" - Gain is x4 ( x4 analog) + + "8" - Gain is x8 ( x8 analog) + + "16" - Gain is x16 (x16 analog) + + "32" - Gain is x32 (x16 analog, x2 digital) + + "64" - Gain is x64 (x16 analog, x4 digital) \ No newline at end of file diff --git a/MAINTAINERS b/MAINTAINERS index 7e0b87d5aa2e..2fd2d06fb87f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13786,6 +13786,13 @@ S: Supported F: Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt F: drivers/regulator/mcp16502.c +MICROCHIP MCP3564 ADC DRIVER +M: Marius Cristea <marius.cristea@microchip.com> +L: linux-iio@vger.kernel.org +S: Supported +F: Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml +F: drivers/iio/adc/mcp3564.c + MICROCHIP MCP3911 ADC DRIVER M: Marcus Folkesson <marcus.folkesson@gmail.com> M: Kent Gustavsson <kent@minoris.se> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index eb2b09ef5d5b..9ee85764b985 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -768,6 +768,19 @@ config MCP3422 This driver can also be built as a module. If so, the module will be called mcp3422. +config MCP3564 + tristate "Microchip Technology MCP3461/2/4/R, MCP3561/2/4/R driver" + depends on SPI + depends on IIO + help + Say yes here to build support for Microchip Technology's MCP3461, + MCP3462, MCP3464, MCP3461R, MCP3462R, MCP3464R, MCP3561, MCP3562, + MCP3564, MCP3561R, MCP3562R and MCP3564R analog to digital + converters. + + This driver can also be built as a module. If so, the module will be + called mcp3564. + config MCP3911 tristate "Microchip Technology MCP3911 driver" depends on SPI diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index e07e4a3e6237..78aed4878e86 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -70,6 +70,7 @@ obj-$(CONFIG_MAX1363) += max1363.o obj-$(CONFIG_MAX9611) += max9611.o obj-$(CONFIG_MCP320X) += mcp320x.o obj-$(CONFIG_MCP3422) += mcp3422.o +obj-$(CONFIG_MCP356X) += mcp3564.o obj-$(CONFIG_MCP3911) += mcp3911.o obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o diff --git a/drivers/iio/adc/mcp3564.c b/drivers/iio/adc/mcp3564.c new file mode 100644 index 000000000000..dfdbe4e37cd4 --- /dev/null +++ b/drivers/iio/adc/mcp3564.c @@ -0,0 +1,1395 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * IIO driver for MCP356X/MCP356XR and MCP346X/MCP346XR series ADC chip family + * + * Copyright (C) 2022-2023 Microchip Technology Inc. and its subsidiaries + * + * Author: Marius Cristea <marius.cristea@microchip.com> + * + * Datasheet for MCP3561, MCP3562, MCP3564 can be found here: + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP3561-2-4-Family-Data-Sheet-DS20006181C.pdf + * Datasheet for MCP3561R, MCP3562R, MCP3564R can be found here: + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3561_2_4R-Data-Sheet-DS200006391C.pdf + * Datasheet for MCP3461, MCP3462, MCP3464 can be found here: + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4-Two-Four-Eight-Channel-153.6-ksps-Low-Noise-16-Bit-Delta-Sigma-ADC-Data-Sheet-20006180D.pdf + * Datasheet for MCP3461R, MCP3462R, MCP3464R can be found here: + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4R-Family-Data-Sheet-DS20006404C.pdf + * + */ + +#include <linux/acpi.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/iopoll.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.h> +#include <linux/util_macros.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +#define MCP356X_ADCDATA 0x00 +#define MCP356X_CONFIG0 0x01 +#define MCP356X_CONFIG1 0x02 +#define MCP356X_CONFIG2 0x03 +#define MCP356X_CONFIG3 0x04 +#define MCP356X_IRQ 0x05 +#define MCP356X_MUX 0x06 +#define MCP356X_SCAN 0x07 +#define MCP356X_TIMER 0x08 +#define MCP356X_OFFSETCAL 0x09 +#define MCP356X_GAINCAL 0x0A +#define MCP356X_RESERVED_B 0x0B +#define MCP356X_RESERVED_C 0x0C +#define MCP356X_LOCK 0x0D +#define MCP356X_RESERVED_E 0x0E +#define MCP356X_CRCCFG 0x0F + +#define MCP356X_FAST_CMD_CTRL 0 +#define MCP356X_RD_CTRL BIT(0) +#define MCP356X_WRT_CTRL BIT(1) + +#define MCP356X_FULL_RESET_CMD GENMASK(3, 1) + +#define MCP3461_HW_ID BIT(3) +#define MCP3462_HW_ID 0x0009 +#define MCP3464_HW_ID 0x000B + +#define MCP3561_HW_ID GENMASK(3, 2) +#define MCP3562_HW_ID 0x000D +#define MCP3564_HW_ID GENMASK(3, 0) +#define MCP356X_HW_ID_MASK GENMASK(3, 0) + +#define MCP356XR_INT_VREF_MV 2400 + +/* MUX_VIN Input Selection + */ +#define MCP356X_INTERNAL_VCM GENMASK(3, 0) +#define MCP356X_TEMP_DIODE_M GENMASK(3, 1) +#define MCP356X_TEMP_DIODE_P 0b1101 +#define MCP356X_REFIN_NEG GENMASK(3, 2) +#define MCP356X_REFIN_POZ 0b1011 +#define MCP356X_RESERVED 0b1010 /* do not use */ +#define MCP356X_AVDD 0b1001 +#define MCP356X_AGND BIT(3) +#define MCP356X_CH7 GENMASK(2, 0) +#define MCP356X_CH6 GENMASK(2, 1) +#define MCP356X_CH5 0b0101 +#define MCP356X_CH4 BIT(2) +#define MCP356X_CH3 GENMASK(1, 0) +#define MCP356X_CH2 BIT(1) +#define MCP356X_CH1 BIT(0) +#define MCP356X_CH0 0 + +#define MCP356X_ADC_MODE_MASK GENMASK(1, 0) + +#define MCP356X_ADC_DEFAULT_MODE 0 +#define MCP356X_ADC_SHUTDOWN_MODE BIT(0) +#define MCP356X_ADC_STANDBY BIT(1) +#define MCP356X_ADC_CONVERSION_MODE GENMASK(1, 0) + +#define MCP356X_DATA_READY_MASK BIT(6) + +#define MCP356X_OVERSAMPLING_RATIO_32 0 +#define MCP356X_OVERSAMPLING_RATIO_64 BIT(0) +#define MCP356X_OVERSAMPLING_RATIO_128 BIT(1) +#define MCP356X_OVERSAMPLING_RATIO_256 GENMASK(1, 0) +#define MCP356X_OVERSAMPLING_RATIO_512 BIT(2) +#define MCP356X_OVERSAMPLING_RATIO_1024 0x05 +#define MCP356X_OVERSAMPLING_RATIO_2048 GENMASK(2, 1) +#define MCP356X_OVERSAMPLING_RATIO_4096 GENMASK(2, 0) +#define MCP356X_OVERSAMPLING_RATIO_8192 BIT(3) +#define MCP356X_OVERSAMPLING_RATIO_16384 0x09 +#define MCP356X_OVERSAMPLING_RATIO_20480 0x0A +#define MCP356X_OVERSAMPLING_RATIO_24576 0x0B +#define MCP356X_OVERSAMPLING_RATIO_40960 0x0C +#define MCP356X_OVERSAMPLING_RATIO_49152 0x0D +#define MCP356X_OVERSAMPLING_RATIO_81920 GENMASK(3, 1) +#define MCP356X_OVERSAMPLING_RATIO_98304 GENMASK(3, 0) + +#define MCP356X_OVERSAMPLING_RATIO_MASK GENMASK(5, 2) +#define MCP356X_OVERSAMPLING_RATIO_SHIFT 0x02 + +#define MCP356X_HARDWARE_GAIN_MASK GENMASK(5, 3) +#define MCP356X_HARDWARE_GAIN_SHIFT 0x03 +#define MCP356X_DEFAULT_HARDWARE_GAIN BIT(1) + +#define MCP356X_CS_SEL_0_0_uA 0x0 +#define MCP356X_CS_SEL_0_9_uA BIT(0) +#define MCP356X_CS_SEL_3_7_uA BIT(1) +#define MCP356X_CS_SEL_15_uA GENMASK(1, 0) + +#define MCP356X_CS_SEL_MASK GENMASK(3, 2) + +#define MCP356X_BOOST_CURRENT_x0_50 0 +#define MCP356X_BOOST_CURRENT_x0_66 BIT(0) +#define MCP356X_BOOST_CURRENT_x1_00 BIT(1) +#define MCP356X_BOOST_CURRENT_x2_00 GENMASK(1, 0) + +#define MCP356X_BOOST_CURRENT_MASK GENMASK(7, 6) + +/* Auto-Zeroing MUX Setting */ +#define MCP356X_AZ_MUX_MASK BIT(2) +/* Auto-Zeroing REF Setting */ +#define MCP356X_AZ_REF_MASK BIT(1) + +#define MCP356X_SHARED_DEVATTRS_COUNT 1 +#define MCP356X_PARTICULAR_DEVATTRS_COUNT 1 + +#define MAX_HWGAIN 64000 + +#define MCP356X_DATA_READY_TIMEOUT_MS 2000 + +enum mcp356x_ids { + mcp3461, + mcp3462, + mcp3464, + mcp3461r, + mcp3462r, + mcp3464r, + mcp3561, + mcp3562, + mcp3564, + mcp3561r, + mcp3562r, + mcp3564r, +}; + +static const unsigned int mcp356x_oversampling_avail[16] = { + [MCP356X_OVERSAMPLING_RATIO_32] = 32, + [MCP356X_OVERSAMPLING_RATIO_64] = 64, + [MCP356X_OVERSAMPLING_RATIO_128] = 128, + [MCP356X_OVERSAMPLING_RATIO_256] = 256, + [MCP356X_OVERSAMPLING_RATIO_512] = 512, + [MCP356X_OVERSAMPLING_RATIO_1024] = 1024, + [MCP356X_OVERSAMPLING_RATIO_2048] = 2048, + [MCP356X_OVERSAMPLING_RATIO_4096] = 4096, + [MCP356X_OVERSAMPLING_RATIO_8192] = 8192, + [MCP356X_OVERSAMPLING_RATIO_16384] = 16384, + [MCP356X_OVERSAMPLING_RATIO_20480] = 20480, + [MCP356X_OVERSAMPLING_RATIO_24576] = 24576, + [MCP356X_OVERSAMPLING_RATIO_40960] = 40960, + [MCP356X_OVERSAMPLING_RATIO_49152] = 49152, + [MCP356X_OVERSAMPLING_RATIO_81920] = 81920, + [MCP356X_OVERSAMPLING_RATIO_98304] = 98304 +}; + +/* + * Current Source/Sink Selection Bits for Sensor Bias (source on VIN+/sink on VIN-) + */ +static const char * const mcp356x_current_bias_avail[] = { + [MCP356X_CS_SEL_0_0_uA] = "no_current(default)", + [MCP356X_CS_SEL_0_9_uA] = "0.9_uA", + [MCP356X_CS_SEL_3_7_uA] = "3.7_uA", + [MCP356X_CS_SEL_15_uA] = "15_uA", +}; + +/* + * BOOST[1:0]: ADC Bias Current Selection + */ +static const char * const mcp356x_boost_current_avail[] = { + [MCP356X_BOOST_CURRENT_x0_50] = "x0.5", + [MCP356X_BOOST_CURRENT_x0_66] = "x0.66", + [MCP356X_BOOST_CURRENT_x1_00] = "x1_(default)", + [MCP356X_BOOST_CURRENT_x2_00] = "x2", +}; + +/* + * Calibration bias values + */ +static const int mcp356x_calib_bias[] = { + -8388608, /* min: -2^23 */ + 1, /* step: 1 */ + 8388607 /* max: 2^23 - 1 */ +}; + +/* + * Calibration scale values + * The Gain Error Calibration register (GAINCAL) is an + * unsigned 24-bit register that holds the digital gain error + * calibration value, GAINCAL which could be calculated by + * GAINCAL (V/V) = (GAINCAL[23:0])/8388608 + * The gain error calibration value range in equivalent voltage is [0; 2-2^(-23)] + */ +static const unsigned int mcp356x_calib_scale[] = { + 0, /* min: 0 */ + 1, /* step: 1/8388608 */ + 16777215 /* max: 2 - 2^(-23) */ +}; + +/* Programmable hardware gain x1/3, x1, x2, x4, x8, x16, x32, x64 */ +static const int mcp356x_hwgain_frac[] = { + 3, + 10, + 1, + 1, + 2, + 1, + 4, + 1, + 8, + 1, + 16, + 1, + 32, + 1, + 64, + 1 +}; + +static const int mcp356x_hwgain[] = { + 300, + 1000, + 2000, + 4000, + 8000, + 16000, + 32000, + 64000 +}; + +/** + * struct mcp356x_chip_info - chip specific data + * @channels: struct iio_chan_spec matching the device's capabilities + * @num_channels: number of channels + * @int_vref_uv: internal voltage reference value in microVolts + * @has_vref: Does the ADC has an internal voltage reference? + */ +struct mcp356x_chip_info { + const struct iio_chan_spec *channels; + unsigned int num_channels; + unsigned int int_vref_uv; + bool has_vref; +}; + +/** + * struct mcp356x_state - working data for a ADC device + * @chip_info: chip specific data + * @mcp356x_info: information about iio device + * @spi: SPI device structure + * @vref: The regulator device used as a voltage reference in case + * external voltage reference is used + * @vref_mv: voltage reference value in miliVolts + * @lock: mutex to prevent concurrent reads/writes + * @dev_addr: hardware device address + * @oversampling: the index inside oversampling list of the ADC + * @hwgain: the index inside hardware gain list of the ADC + * @calib_bias: calibration bias value + * @calib_scale: calibration scale value + * @current_boost_mode: the index inside current boost list of the ADC + * @current_bias_mode: the index inside current bias list of the ADC + * @auto_zeroing_mux: set if ADC auto-zeroing algorithm is enabled + * @auto_zeroing_ref: set if ADC auto-Zeroing Reference Buffer Setting is enabled + */ +struct mcp356x_state { + const struct mcp356x_chip_info *chip_info; + struct iio_info mcp356x_info; + struct spi_device *spi; + struct regulator *vref; + unsigned short vref_mv; + struct mutex lock; /*lock to prevent concurrent reads/writes */ + u8 dev_addr; + unsigned int oversampling; + unsigned int hwgain; + int calib_bias; + int calib_scale; + unsigned int current_boost_mode; + unsigned int current_bias_mode; + bool auto_zeroing_mux; + bool auto_zeroing_ref; +}; + +static inline u8 mcp356x_reg_write(u8 chip_addr, u8 reg) +{ + return ((chip_addr << 6) | (reg << 2) | MCP356X_WRT_CTRL); +} + +static inline u8 mcp356x_reg_read(u8 chip_addr, u8 reg) +{ + return ((chip_addr << 6) | (reg << 2) | MCP356X_RD_CTRL); +} + +static inline u8 mcp356x_reg_fast_cmd(u8 chip_addr, u8 cmd) +{ + return ((chip_addr << 6) | (cmd << 2)); +} + +static int mcp356x_read(struct mcp356x_state *adc, u8 reg, u32 *val, u8 len) +{ + int ret; + u8 tmp_reg; + + tmp_reg = mcp356x_reg_read(adc->dev_addr, reg); + + ret = spi_write_then_read(adc->spi, &tmp_reg, 1, val, len); + + be32_to_cpus(val); + *val >>= ((4 - len) * 8); + + return ret; +} + +static int mcp356x_write(struct mcp356x_state *adc, u8 reg, u32 val, u8 len) +{ + val |= (mcp356x_reg_write(adc->dev_addr, reg) << (len * 8)); + val <<= (3 - len) * 8; + cpu_to_be32s(&val); + + return spi_write(adc->spi, &val, len + 1); +} + +static int mcp356x_fast_cmd(struct mcp356x_state *adc, u8 fast_cmd) +{ + u8 val; + + val = mcp356x_reg_fast_cmd(adc->dev_addr, fast_cmd); + + return spi_write(adc->spi, &val, 1); +} + +static int mcp356x_update(struct mcp356x_state *adc, u8 reg, u32 mask, u32 val, + u8 len) +{ + u32 tmp; + int ret; + + ret = mcp356x_read(adc, reg, &tmp, len); + + if (ret == 0) { + val &= mask; + val |= tmp & ~mask; + ret = mcp356x_write(adc, reg, val, len); + } + + return ret; +} + +/* Custom IIO Device Attributes */ +static int mcp356x_set_current_boost_mode(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + unsigned int mode) +{ + struct mcp356x_state *adc = iio_priv(indio_dev); + int ret; + + dev_dbg(&indio_dev->dev, "%s: %d\n", __func__, mode); + + mutex_lock(&adc->lock); + ret = mcp356x_update(adc, MCP356X_CONFIG2, MCP356X_BOOST_CURRENT_MASK, + mode, 1); + + if (ret) + dev_err(&indio_dev->dev, "Failed to configure CONFIG2 register\n"); + else + adc->current_boost_mode = mode; + + mutex_unlock(&adc->lock); + + return ret; +} + +static int mcp356x_get_current_boost_mode(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan) +{ + struct mcp356x_state *adc = iio_priv(indio_dev); + + return adc->current_boost_mode; +} + +static int mcp356x_set_current_bias_mode(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + unsigned int mode) +{ + struct mcp356x_state *adc = iio_priv(indio_dev); + int ret; + + dev_dbg(&indio_dev->dev, "%s: %d\n", __func__, mode); + + mutex_lock(&adc->lock); + ret = mcp356x_update(adc, MCP356X_CONFIG0, MCP356X_CS_SEL_MASK, mode, 1); + + if (ret) + dev_err(&indio_dev->dev, "Failed to configure CONFIG0 register\n"); + else + adc->current_bias_mode = mode; + + mutex_unlock(&adc->lock); + + return ret; +} + +static int mcp356x_get_current_bias_mode(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan) +{ + struct mcp356x_state *adc = iio_priv(indio_dev); + + return adc->current_bias_mode; +} + +static const struct iio_enum mcp356x_current_boost_mode_enum = { + .items = mcp356x_boost_current_avail, + .num_items = ARRAY_SIZE(mcp356x_boost_current_avail), + .set = mcp356x_set_current_boost_mode, + .get = mcp356x_get_current_boost_mode, +}; + +static const struct iio_enum mcp356x_current_bias_mode_enum = { + .items = mcp356x_current_bias_avail, + .num_items = ARRAY_SIZE(mcp356x_current_bias_avail), + .set = mcp356x_set_current_bias_mode, + .get = mcp356x_get_current_bias_mode, +}; + +static const struct iio_chan_spec_ext_info mcp356x_ext_info[] = { + IIO_ENUM("boost_current", IIO_SHARED_BY_ALL, &mcp356x_current_boost_mode_enum), + { + .name = "boost_current_available", + .shared = IIO_SHARED_BY_ALL, + .read = iio_enum_available_read, + .private = (uintptr_t)&mcp356x_current_boost_mode_enum, + }, + IIO_ENUM("current_bias", IIO_SHARED_BY_ALL, &mcp356x_current_bias_mode_enum), + { + .name = "current_bias_available", + .shared = IIO_SHARED_BY_ALL, + .read = iio_enum_available_read, + .private = (uintptr_t)&mcp356x_current_bias_mode_enum, + }, + {} +}; + +static ssize_t mcp356x_auto_zeroing_mux_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct mcp356x_state *adc = iio_priv(indio_dev); + + return sysfs_emit(buf, "%d\n", adc->auto_zeroing_mux); +} + +static ssize_t mcp356x_auto_zeroing_mux_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct mcp356x_state *adc = iio_priv(indio_dev); + bool auto_zero; + int ret; + + ret = kstrtobool(buf, &auto_zero); + if (ret) + return ret; + + mutex_lock(&adc->lock); + ret = mcp356x_update(adc, MCP356X_CONFIG2, MCP356X_AZ_MUX_MASK, + (u32)auto_zero, 1); + + if (ret) + dev_err(&indio_dev->dev, "Failed to update CONFIG2 register\n"); + else + adc->auto_zeroing_mux = auto_zero; + + mutex_unlock(&adc->lock); + + return ret ? ret : len; +} + +static ssize_t mcp356x_auto_zeroing_ref_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct mcp356x_state *adc = iio_priv(indio_dev); + + return sysfs_emit(buf, "%d\n", adc->auto_zeroing_ref); +} + +static ssize_t mcp356x_auto_zeroing_ref_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct mcp356x_state *adc = iio_priv(indio_dev); + bool auto_zero; + int ret; + + ret = kstrtobool(buf, &auto_zero); + if (ret) + return ret; + + mutex_lock(&adc->lock); + ret = mcp356x_update(adc, MCP356X_CONFIG2, MCP356X_AZ_REF_MASK, + (u32)auto_zero, 1); + + if (ret) + dev_err(&indio_dev->dev, "Failed to update CONFIG2 register\n"); + else + adc->auto_zeroing_ref = auto_zero; + + mutex_unlock(&adc->lock); + + return ret ? ret : len; +} + +#define MCP356X_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr) + +static IIO_DEVICE_ATTR(enable_auto_zeroing_ref, 0644, + mcp356x_auto_zeroing_ref_show, + mcp356x_auto_zeroing_ref_store, 0); + +static struct attribute *mcp356x_particular_attributes[] = { + MCP356X_DEV_ATTR(enable_auto_zeroing_ref), + NULL +}; + +static IIO_DEVICE_ATTR(enable_auto_zeroing_mux, 0644, + mcp356x_auto_zeroing_mux_show, + mcp356x_auto_zeroing_mux_store, 0); + +static struct attribute *mcp356x_shared_attributes[] = { + MCP356X_DEV_ATTR(enable_auto_zeroing_mux), + NULL, +}; + +static int mcp356x_prep_custom_attributes(struct mcp356x_state *adc, + struct iio_dev *indio_dev) +{ + int i; + struct attribute **mcp356x_custom_attr; + struct attribute_group *mcp356x_group; + + mcp356x_group = devm_kzalloc(&adc->spi->dev, sizeof(*mcp356x_group), GFP_KERNEL); + + if (!mcp356x_group) + return (-ENOMEM); + + mcp356x_custom_attr = devm_kzalloc(&adc->spi->dev, (MCP356X_SHARED_DEVATTRS_COUNT + + MCP356X_PARTICULAR_DEVATTRS_COUNT + 1) * sizeof(struct attribute *), + GFP_KERNEL); + + if (!mcp356x_custom_attr) + return (-ENOMEM); + + for (i = 0; i < MCP356X_SHARED_DEVATTRS_COUNT; i++) + mcp356x_custom_attr[i] = mcp356x_shared_attributes[i]; + + if (adc->chip_info->has_vref) { + dev_dbg(&indio_dev->dev, "Setup custom attr for R variant\n"); + for (i = 0; i < MCP356X_PARTICULAR_DEVATTRS_COUNT; i++) + mcp356x_custom_attr[MCP356X_SHARED_DEVATTRS_COUNT + i] = + mcp356x_particular_attributes[i]; + } + + mcp356x_group->attrs = mcp356x_custom_attr; + adc->mcp356x_info.attrs = mcp356x_group; + + return 0; +} + +#define MCP356X_V_CHANNEL(index, addr, depth) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = (index), \ + .address = (((addr) << 4) | MCP356X_AGND), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \ + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ + BIT(IIO_CHAN_INFO_CALIBSCALE) | \ + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_CALIBSCALE) | \ + BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \ + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ + .ext_info = mcp356x_ext_info, \ + .scan_index = 0, \ + .scan_type = { \ + .sign = 's', \ + .realbits = depth, \ + .storagebits = 32, \ + .endianness = IIO_BE, \ + }, \ +} + +#define MCP356X_T_CHAN(depth) { \ + .type = IIO_TEMP, \ + .channel = 0, \ + .address = ((MCP356X_TEMP_DIODE_P << 4) | MCP356X_TEMP_DIODE_M), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \ + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ + BIT(IIO_CHAN_INFO_CALIBSCALE) | \ + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_CALIBSCALE) | \ + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ + .ext_info = mcp356x_ext_info, \ + .scan_index = 0, \ + .scan_type = { \ + .sign = 'u', \ + .realbits = depth, \ + .storagebits = 32, \ + .endianness = IIO_BE, \ + }, \ +} + +#define MCP356X_V_CHANNEL_DIFF(chan1, chan2, addr, depth) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = (chan1), \ + .channel2 = (chan2), \ + .address = (addr), \ + .differential = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \ + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ + BIT(IIO_CHAN_INFO_CALIBSCALE) | \ + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_CALIBSCALE) | \ + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ + .ext_info = mcp356x_ext_info, \ + .scan_index = 0, \ + .scan_type = { \ + .sign = 's', \ + .realbits = depth, \ + .storagebits = 32, \ + .endianness = IIO_BE, \ + }, \ +} + +#define MCP3561_CHANNELS(depth) { \ + MCP356X_V_CHANNEL(0, 0, depth), \ + MCP356X_V_CHANNEL(1, 1, depth), \ + MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth), \ + MCP356X_T_CHAN(depth) \ +} + +#define MCP3562_CHANNELS(depth) { \ + MCP356X_V_CHANNEL(0, 0, depth), \ + MCP356X_V_CHANNEL(1, 1, depth), \ + MCP356X_V_CHANNEL(2, 2, depth), \ + MCP356X_V_CHANNEL(3, 3, depth), \ + MCP356X_T_CHAN(depth), \ + MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth), \ + MCP356X_V_CHANNEL_DIFF(0, 2, 0x02, depth), \ + MCP356X_V_CHANNEL_DIFF(0, 3, 0x03, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 2, 0x12, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 3, 0x13, depth), \ + MCP356X_V_CHANNEL_DIFF(2, 3, 0x23, depth), \ + MCP356X_V_CHANNEL_DIFF(2, 0, 0x20, depth), \ + MCP356X_V_CHANNEL_DIFF(3, 0, 0x30, depth), \ + MCP356X_V_CHANNEL_DIFF(2, 1, 0x21, depth), \ + MCP356X_V_CHANNEL_DIFF(3, 1, 0x31, depth), \ + MCP356X_V_CHANNEL_DIFF(3, 2, 0x32, depth), \ +} + +#define MCP3564_CHANNELS(depth) { \ + MCP356X_V_CHANNEL(0, 0, depth), \ + MCP356X_V_CHANNEL(1, 1, depth), \ + MCP356X_V_CHANNEL(2, 2, depth), \ + MCP356X_V_CHANNEL(3, 3, depth), \ + MCP356X_V_CHANNEL(4, 4, depth), \ + MCP356X_V_CHANNEL(5, 5, depth), \ + MCP356X_V_CHANNEL(6, 6, depth), \ + MCP356X_V_CHANNEL(7, 7, depth), \ + MCP356X_T_CHAN(depth), \ + MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth), \ + MCP356X_V_CHANNEL_DIFF(0, 2, 0x02, depth), \ + MCP356X_V_CHANNEL_DIFF(0, 3, 0x03, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 2, 0x12, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 3, 0x13, depth), \ + MCP356X_V_CHANNEL_DIFF(2, 3, 0x23, depth), \ + MCP356X_V_CHANNEL_DIFF(2, 0, 0x20, depth), \ + MCP356X_V_CHANNEL_DIFF(3, 0, 0x30, depth), \ + MCP356X_V_CHANNEL_DIFF(2, 1, 0x21, depth), \ + MCP356X_V_CHANNEL_DIFF(3, 1, 0x31, depth), \ + MCP356X_V_CHANNEL_DIFF(3, 2, 0x32, depth), \ + MCP356X_V_CHANNEL_DIFF(0, 4, 0x04, depth), \ + MCP356X_V_CHANNEL_DIFF(0, 5, 0x05, depth), \ + MCP356X_V_CHANNEL_DIFF(0, 6, 0x06, depth), \ + MCP356X_V_CHANNEL_DIFF(0, 7, 0x07, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 4, 0x14, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 5, 0x15, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 6, 0x16, depth), \ + MCP356X_V_CHANNEL_DIFF(1, 7, 0x17, depth), \ + MCP356X_V_CHANNEL_DIFF(2, 4, 0x24, depth), \ + MCP356X_V_CHANNEL_DIFF(2, 5, 0x25, depth), \ + MCP356X_V_CHANNEL_DIFF(2, 6, 0x26, depth), \ + MCP356X_V_CHANNEL_DIFF(2, 7, 0x27, depth), \ + MCP356X_V_CHANNEL_DIFF(3, 4, 0x34, depth), \ + MCP356X_V_CHANNEL_DIFF(3, 5, 0x35, depth), \ + MCP356X_V_CHANNEL_DIFF(3, 6, 0x36, depth), \ + MCP356X_V_CHANNEL_DIFF(3, 7, 0x37, depth), \ + MCP356X_V_CHANNEL_DIFF(4, 5, 0x45, depth), \ + MCP356X_V_CHANNEL_DIFF(4, 6, 0x46, depth), \ + MCP356X_V_CHANNEL_DIFF(4, 7, 0x47, depth), \ + MCP356X_V_CHANNEL_DIFF(5, 6, 0x56, depth), \ + MCP356X_V_CHANNEL_DIFF(5, 7, 0x57, depth), \ + MCP356X_V_CHANNEL_DIFF(6, 7, 0x67, depth), \ + MCP356X_V_CHANNEL_DIFF(4, 0, 0x40, depth), \ + MCP356X_V_CHANNEL_DIFF(5, 0, 0x50, depth), \ + MCP356X_V_CHANNEL_DIFF(6, 0, 0x60, depth), \ + MCP356X_V_CHANNEL_DIFF(7, 0, 0x70, depth), \ + MCP356X_V_CHANNEL_DIFF(4, 1, 0x41, depth), \ + MCP356X_V_CHANNEL_DIFF(5, 1, 0x51, depth), \ + MCP356X_V_CHANNEL_DIFF(6, 1, 0x61, depth), \ + MCP356X_V_CHANNEL_DIFF(7, 1, 0x71, depth), \ + MCP356X_V_CHANNEL_DIFF(4, 2, 0x42, depth), \ + MCP356X_V_CHANNEL_DIFF(5, 2, 0x52, depth), \ + MCP356X_V_CHANNEL_DIFF(6, 2, 0x62, depth), \ + MCP356X_V_CHANNEL_DIFF(7, 2, 0x72, depth), \ + MCP356X_V_CHANNEL_DIFF(4, 3, 0x43, depth), \ + MCP356X_V_CHANNEL_DIFF(5, 3, 0x53, depth), \ + MCP356X_V_CHANNEL_DIFF(6, 3, 0x63, depth), \ + MCP356X_V_CHANNEL_DIFF(7, 3, 0x73, depth), \ + MCP356X_V_CHANNEL_DIFF(5, 4, 0x54, depth), \ + MCP356X_V_CHANNEL_DIFF(6, 4, 0x64, depth), \ + MCP356X_V_CHANNEL_DIFF(7, 4, 0x74, depth), \ + MCP356X_V_CHANNEL_DIFF(6, 5, 0x65, depth), \ + MCP356X_V_CHANNEL_DIFF(7, 5, 0x75, depth), \ + MCP356X_V_CHANNEL_DIFF(7, 6, 0x76, depth) \ +} + +static const struct iio_chan_spec mcp3461_channels[] = MCP3561_CHANNELS(16); +static const struct iio_chan_spec mcp3462_channels[] = MCP3562_CHANNELS(16); +static const struct iio_chan_spec mcp3464_channels[] = MCP3564_CHANNELS(16); +static const struct iio_chan_spec mcp3561_channels[] = MCP3561_CHANNELS(24); +static const struct iio_chan_spec mcp3562_channels[] = MCP3562_CHANNELS(24); +static const struct iio_chan_spec mcp3564_channels[] = MCP3564_CHANNELS(24); + +static const struct mcp356x_chip_info mcp356x_chip_infos_tbl[] = { + [mcp3461] = { + .channels = mcp3461_channels, + .num_channels = ARRAY_SIZE(mcp3461_channels), + .int_vref_uv = 0, + .has_vref = false + }, + [mcp3462] = { + .channels = mcp3462_channels, + .num_channels = ARRAY_SIZE(mcp3462_channels), + .int_vref_uv = 0, + .has_vref = false + }, + [mcp3464] = { + .channels = mcp3464_channels, + .num_channels = ARRAY_SIZE(mcp3464_channels), + .int_vref_uv = 0, + .has_vref = false + }, + [mcp3461r] = { + .channels = mcp3461_channels, + .num_channels = ARRAY_SIZE(mcp3461_channels), + .int_vref_uv = MCP356XR_INT_VREF_MV, + .has_vref = true + }, + [mcp3462r] = { + .channels = mcp3462_channels, + .num_channels = ARRAY_SIZE(mcp3462_channels), + .int_vref_uv = MCP356XR_INT_VREF_MV, + .has_vref = true + }, + [mcp3464r] = { + .channels = mcp3464_channels, + .num_channels = ARRAY_SIZE(mcp3464_channels), + .int_vref_uv = MCP356XR_INT_VREF_MV, + .has_vref = true + }, + [mcp3561] = { + .channels = mcp3561_channels, + .num_channels = ARRAY_SIZE(mcp3561_channels), + .int_vref_uv = 0, + .has_vref = false + }, + [mcp3562] = { + .channels = mcp3562_channels, + .num_channels = ARRAY_SIZE(mcp3562_channels), + .int_vref_uv = 0, + .has_vref = false + }, + [mcp3564] = { + .channels = mcp3564_channels, + .num_channels = ARRAY_SIZE(mcp3564_channels), + .int_vref_uv = 0, + .has_vref = false + }, + [mcp3561r] = { + .channels = mcp3561_channels, + .num_channels = ARRAY_SIZE(mcp3561_channels), + .int_vref_uv = MCP356XR_INT_VREF_MV, + .has_vref = true + }, + [mcp3562r] = { + .channels = mcp3562_channels, + .num_channels = ARRAY_SIZE(mcp3562_channels), + .int_vref_uv = MCP356XR_INT_VREF_MV, + .has_vref = true + }, + [mcp3564r] = { + .channels = mcp3564_channels, + .num_channels = ARRAY_SIZE(mcp3564_channels), + .int_vref_uv = MCP356XR_INT_VREF_MV, + .has_vref = true + }, +}; + +static int mcp356x_read_single_value(struct iio_dev *indio_dev, + struct iio_chan_spec const *channel, + int *val) +{ + struct mcp356x_state *adc = iio_priv(indio_dev); + int ret, tmp, ret_read = 0; + + /* Configure MUX register with the requested channel */ + ret = mcp356x_write(adc, MCP356X_MUX, channel->address, 1); + if (ret) { + dev_err(&indio_dev->dev, "Failed to configure MUX register\n"); + return ret; + } + + /* ADC Conversion starts by writing ADC_MODE[1:0] = 11 to CONFIG0[1:0] = */ + ret = mcp356x_update(adc, MCP356X_CONFIG0, MCP356X_ADC_MODE_MASK, + MCP356X_ADC_CONVERSION_MODE, 1); + if (ret) { + dev_err(&indio_dev->dev, + "Failed to configure CONFIG0 register\n"); + return ret; + } + + /* + * Check if the conversion is ready. If not, wait a little bit, and + * in case of timeout exit with an error. + */ + + ret = read_poll_timeout(mcp356x_read, ret_read, + ret_read || !(tmp & MCP356X_DATA_READY_MASK), + 1000, MCP356X_DATA_READY_TIMEOUT_MS * 1000, true, + adc, MCP356X_IRQ, &tmp, 1); + + /* failed to read status register */ + if (ret_read) + return ret; + + if (ret) + return -ETIMEDOUT; + + if (tmp & MCP356X_DATA_READY_MASK) + /* failing to finish conversion */ + return -EBUSY; + + ret = mcp356x_read(adc, MCP356X_ADCDATA, &tmp, 4); + if (ret) + return ret; + + *val = tmp; + + return ret; +} + +static int mcp356x_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *channel, + const int **vals, int *type, + int *length, long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: + *type = IIO_VAL_INT; + *vals = mcp356x_oversampling_avail; + *length = ARRAY_SIZE(mcp356x_oversampling_avail); + return IIO_AVAIL_LIST; + case IIO_CHAN_INFO_HARDWAREGAIN: + *type = IIO_VAL_FRACTIONAL; + *length = ARRAY_SIZE(mcp356x_hwgain_frac); + *vals = mcp356x_hwgain_frac; + return IIO_AVAIL_LIST; + case IIO_CHAN_INFO_CALIBBIAS: + *vals = mcp356x_calib_bias; + *type = IIO_VAL_INT; + return IIO_AVAIL_RANGE; + case IIO_CHAN_INFO_CALIBSCALE: + *vals = mcp356x_calib_scale; + *type = IIO_VAL_INT; + return IIO_AVAIL_RANGE; + default: + return -EINVAL; + } +} + +static int mcp356x_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *channel, + int *val, int *val2, long mask) +{ + struct mcp356x_state *adc = iio_priv(indio_dev); + int ret; + + mutex_lock(&adc->lock); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + ret = mcp356x_read_single_value(indio_dev, channel, val); + if (ret) + ret = -EINVAL; + else + ret = IIO_VAL_INT; + break; + case IIO_CHAN_INFO_SCALE: + *val = adc->vref_mv; + *val2 = channel->scan_type.realbits - 1; + ret = IIO_VAL_FRACTIONAL_LOG2; + break; + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: + *val = mcp356x_oversampling_avail[adc->oversampling]; + ret = IIO_VAL_INT; + break; + case IIO_CHAN_INFO_HARDWAREGAIN: + *val = mcp356x_hwgain_frac[2 * adc->hwgain]; + *val2 = mcp356x_hwgain_frac[(2 * adc->hwgain) + 1]; + ret = IIO_VAL_FRACTIONAL; + break; + case IIO_CHAN_INFO_CALIBBIAS: + *val = adc->calib_bias; + ret = IIO_VAL_INT; + break; + case IIO_CHAN_INFO_CALIBSCALE: + *val = adc->calib_scale; + ret = IIO_VAL_INT; + break; + default: + ret = -EINVAL; + } + + mutex_unlock(&adc->lock); + + return ret; +} + +static int mcp356x_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *channel, int val, + int val2, long mask) +{ + struct mcp356x_state *adc = iio_priv(indio_dev); + int tmp; + int ret = -EINVAL; + + mutex_lock(&adc->lock); + switch (mask) { + case IIO_CHAN_INFO_CALIBBIAS: + if (val < mcp356x_calib_bias[0] && val > mcp356x_calib_bias[2]) + goto out; + + adc->calib_bias = val; + ret = mcp356x_write(adc, MCP356X_OFFSETCAL, val, 3); + break; + case IIO_CHAN_INFO_CALIBSCALE: + if (val < mcp356x_calib_bias[0] && val > mcp356x_calib_bias[2]) + goto out; + + adc->calib_scale = val; + ret = mcp356x_write(adc, MCP356X_GAINCAL, val, 3); + break; + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: + if (val < 0) + goto out; + + adc->oversampling = find_closest(val, mcp356x_oversampling_avail, + ARRAY_SIZE(mcp356x_oversampling_avail)); + + dev_dbg(&adc->spi->dev, + "IIO_CHAN_INFO_OVERSAMPLING_RATIO index %d\n", + adc->oversampling); + + ret = mcp356x_update(adc, MCP356X_CONFIG1, MCP356X_OVERSAMPLING_RATIO_MASK, + (adc->oversampling << MCP356X_OVERSAMPLING_RATIO_SHIFT), + 1); + if (ret) + dev_err(&indio_dev->dev, + "Failed to configure CONFIG1 register\n"); + + break; + case IIO_CHAN_INFO_HARDWAREGAIN: + /* + * calculate gain from read values. + * avoid using fractional numbers so + * multiply the value with 1000. In case of x1/3 gain + * the tmp will be 300 + */ + tmp = ((val * 1000000) + val2) / 1000; + if (tmp < 1 || tmp > MAX_HWGAIN) + goto out; + + adc->hwgain = find_closest(tmp, mcp356x_hwgain, + ARRAY_SIZE(mcp356x_hwgain)); + + dev_dbg(&adc->spi->dev, + "IIO_CHAN_INFO_HARDWAREGAIN Gain:%d; index %d\n", + tmp, adc->hwgain); + + /* Update GAIN in CONFIG2[5:3] -> GAIN[2:0]*/ + ret = mcp356x_update(adc, MCP356X_CONFIG2, MCP356X_HARDWARE_GAIN_MASK, + (adc->hwgain << MCP356X_HARDWARE_GAIN_SHIFT), 1); + if (ret) + dev_err(&indio_dev->dev, + "Failed to configure CONFIG0 register\n"); + break; + } + +out: + mutex_unlock(&adc->lock); + + return ret; +} + +static int mcp356x_config(struct mcp356x_state *adc) +{ + int ret = 0; + unsigned int tmp; + + dev_dbg(&adc->spi->dev, "%s: Start config...\n", __func__); + + /* + * The address is set on a per-device basis by fuses in the factory, + * configured on request. If not requested, the fuses are set for 0x1. + * The device address is part of the device markings to avoid + * potential confusion. This address is coded on two bits, so four possible + * addresses are available when multiple devices are present on the same + * SPI bus with only one Chip Select line for all devices. + */ + device_property_read_u32(&adc->spi->dev, "microchip,hw-device-address", &tmp); + + if (tmp > 3) { + dev_err_probe(&adc->spi->dev, tmp, + "invalid device address. Must be in range 0-3.\n"); + return -EINVAL; + } + + adc->dev_addr = 0xff & tmp; + + dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr); + + ret = mcp356x_read(adc, MCP356X_RESERVED_E, &tmp, 2); + + if (ret == 0) { + switch (tmp & MCP356X_HW_ID_MASK) { + case MCP3461_HW_ID: + dev_dbg(&adc->spi->dev, "Found MCP3461 chip\n"); + break; + case MCP3462_HW_ID: + dev_dbg(&adc->spi->dev, "Found MCP3462 chip\n"); + break; + case MCP3464_HW_ID: + dev_dbg(&adc->spi->dev, "Found MCP3464 chip\n"); + break; + case MCP3561_HW_ID: + dev_dbg(&adc->spi->dev, "Found MCP3561 chip\n"); + break; + case MCP3562_HW_ID: + dev_dbg(&adc->spi->dev, "Found MCP3562 chip\n"); + break; + case MCP3564_HW_ID: + dev_dbg(&adc->spi->dev, "Found MCP3564 chip\n"); + break; + default: + dev_err_probe(&adc->spi->dev, tmp, + "Unknown chip found\n"); + return -EINVAL; + } + } else { + return ret; + } + + /* Command sequence that ensures a recovery with + * the desired settings in any cases of loss-of-power scenario. + */ + + /* Write LOCK register to 0xA5 (Write Access Password) + * Write access is allowed on the full register map. + */ + ret = mcp356x_write(adc, MCP356X_LOCK, 0x000000A5, 1); + if (ret) + return ret; + + /* Write IRQ register to 0x03 */ + /* IRQ --> IRQ Mode = Hi-Z IRQ Output --> (0b00000011). + * IRQ = 0x00000003 + */ + ret = mcp356x_write(adc, MCP356X_IRQ, 0x00000003, 1); + if (ret) + return ret; + + /* Device Full Reset Fast Command */ + ret = mcp356x_fast_cmd(adc, MCP356X_FULL_RESET_CMD); + + /* wait 1ms for the chip to restart after a full reset */ + mdelay(1); + + /* Reconfigure the ADC chip */ + + /* GAINCAL --> Disabled. + * Default value is GAINCAL = 0x00800000; which provides a gain of 1x + */ + ret = mcp356x_write(adc, MCP356X_GAINCAL, 0x00800000, 3); + if (ret) + return ret; + + adc->calib_scale = 0x00800000; + + /* OFFSETCAL --> 0 Counts of Offset Cancellation + * (Measured offset is negative). + * OFFSETCAL = 0x0 + */ + ret = mcp356x_write(adc, MCP356X_OFFSETCAL, 0x00000000, 3); + if (ret) + return ret; + + /* TIMER --> Disabled. + * TIMER = 0x00000000 + */ + ret = mcp356x_write(adc, MCP356X_TIMER, 0x00000000, 3); + if (ret) + return ret; + + /* SCAN --> Disabled. + * SCAN = 0x00000000 + */ + ret = mcp356x_write(adc, MCP356X_SCAN, 0x00000000, 3); + if (ret) + return ret; + + /* MUX --> VIN+ = CH0, VIN- = CH1 --> (0b00000001). + * MUX = 0x00000001 + */ + ret = mcp356x_write(adc, MCP356X_MUX, 0x00000001, 1); + if (ret) + return ret; + + /* IRQ --> IRQ Mode = Hi-Z IRQ Output --> (0b00000011). + * IRQ = 0x00000003 + */ + ret = mcp356x_write(adc, MCP356X_IRQ, 0x00000003, 1); + if (ret) + return ret; + + /* CONFIG3 + * Conv. Mod = One-Shot/Standby, + * FORMAT = 32-bit (right justified data): SGN extension + ADC data, + * CRC_FORMAT = 16b, CRC-COM = Disabled, + * OFFSETCAL = Enabled, GAINCAL = Enabled --> (10100011). + * CONFIG3 = 0x000000A3 + * + */ + ret = mcp356x_write(adc, MCP356X_CONFIG3, 0x000000A3, 1); + if (ret) + return ret; + + /* CONFIG2 --> BOOST = 1x, GAIN = 1x, AZ_MUX = 1 --> (0b10001101). + * CONFIG2 = 0x0000008D + */ + ret = mcp356x_write(adc, MCP356X_CONFIG2, 0x0000008D, 1); + if (ret) + return ret; + + adc->hwgain = 0x01; + adc->auto_zeroing_mux = true; + adc->auto_zeroing_ref = false; + adc->current_boost_mode = MCP356X_BOOST_CURRENT_x1_00; + + /* CONFIG1 --> AMCLK = MCLK, OSR = 98304 --> (0b00111100). + * CONFIG1 = 0x0000003C + */ + ret = mcp356x_write(adc, MCP356X_CONFIG1, 0x0000003C, 1); + if (ret) + return ret; + + adc->oversampling = 0x0F; + + if (!adc->vref) { + /* CONFIG0 --> VREF_SEL = Internal Voltage Reference 2.4v + * CLK_SEL = INTOSC w/o CLKOUT, CS_SEL = No Bias, + * ADC_MODE = Standby Mode --> (0b11100010). + * CONFIG0 = 0x000000E2 + */ + ret = mcp356x_write(adc, MCP356X_CONFIG0, 0x000000E2, 1); + + dev_dbg(&adc->spi->dev, "%s: Using internal Vref\n", + __func__); + adc->vref_mv = MCP356XR_INT_VREF_MV; + + } else { + /* CONFIG0 --> CLK_SEL = INTOSC w/o CLKOUT, CS_SEL = No Bias, + * ADC_MODE = Standby Mode --> (0b01100010). + * CONFIG0 = 0x000000E2 + */ + ret = mcp356x_write(adc, MCP356X_CONFIG0, 0x00000062, 1); + } + adc->current_bias_mode = MCP356X_CS_SEL_0_0_uA; + + return ret; +} + +static int mcp356x_probe(struct spi_device *spi) +{ + int ret, device_index; + struct iio_dev *indio_dev; + struct mcp356x_state *adc; + + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); + if (!indio_dev) { + dev_err_probe(&indio_dev->dev, PTR_ERR(indio_dev), + "Can't allocate iio device\n"); + return -ENOMEM; + } + + adc = iio_priv(indio_dev); + adc->spi = spi; + + dev_dbg(&adc->spi->dev, "%s: probe(spi = 0x%p)\n", __func__, spi); + + adc->vref = devm_regulator_get_optional(&adc->spi->dev, "vref"); + if (IS_ERR(adc->vref)) { + if (PTR_ERR(adc->vref) == -ENODEV) { + adc->vref = NULL; + dev_dbg(&adc->spi->dev, "%s: Using internal Vref\n", + __func__); + } else { + dev_err_probe(&adc->spi->dev, PTR_ERR(adc->vref), + "failed to get regulator\n"); + return PTR_ERR(adc->vref); + } + } else { + ret = regulator_enable(adc->vref); + if (ret) + return ret; + + dev_dbg(&adc->spi->dev, "%s: Using External Vref\n", + __func__); + + ret = regulator_get_voltage(adc->vref); + if (ret < 0) { + dev_err_probe(&adc->spi->dev, ret, + "Failed to read vref regulator\n"); + goto error_disable_reg; + } + + adc->vref_mv = ret / 1000; + } + + spi_set_drvdata(spi, indio_dev); + device_index = spi_get_device_id(spi)->driver_data; + adc->chip_info = &mcp356x_chip_infos_tbl[device_index]; + + adc->mcp356x_info.read_raw = mcp356x_read_raw; + adc->mcp356x_info.write_raw = mcp356x_write_raw; + adc->mcp356x_info.read_avail = mcp356x_read_avail; + + ret = mcp356x_prep_custom_attributes(adc, indio_dev); + if (ret) { + dev_err_probe(&adc->spi->dev, ret, + "Can't configure custom attributes for MCP356X device\n"); + goto error_disable_reg; + } + + indio_dev->name = spi_get_device_id(spi)->name; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->info = &adc->mcp356x_info; + + indio_dev->channels = adc->chip_info->channels; + indio_dev->num_channels = adc->chip_info->num_channels; + indio_dev->masklength = adc->chip_info->num_channels - 1; + + /* initialize the chip access mutex */ + mutex_init(&adc->lock); + + /* Do any chip specific initialization, e.g: + * read/write some registers + * enable/disable certain channels + * change the sampling rate to the requested value + */ + ret = mcp356x_config(adc); + if (ret) { + dev_err_probe(&adc->spi->dev, ret, + "Can't configure MCP356X device\n"); + goto error_disable_reg; + } + + dev_dbg(&adc->spi->dev, "%s: Vref (mV): %d\n", __func__, adc->vref_mv); + + ret = devm_iio_device_register(&spi->dev, indio_dev); + if (ret) { + dev_err_probe(&adc->spi->dev, ret, + "Can't register IIO device\n"); + goto error_disable_reg; + } + + return 0; + +error_disable_reg: + if (adc->vref) + regulator_disable(adc->vref); + + return ret; +} + +static void mcp356x_remove(struct spi_device *spi) +{ + struct iio_dev *indio_dev = spi_get_drvdata(spi); + struct mcp356x_state *adc = iio_priv(indio_dev); + + if (adc->vref) + regulator_disable(adc->vref); +} + +static const struct of_device_id mcp356x_dt_ids[] = { + { .compatible = "microchip,mcp3461" }, + { .compatible = "microchip,mcp3462" }, + { .compatible = "microchip,mcp3464" }, + { .compatible = "microchip,mcp3461r" }, + { .compatible = "microchip,mcp3462r" }, + { .compatible = "microchip,mcp3464r" }, + { .compatible = "microchip,mcp3561" }, + { .compatible = "microchip,mcp3562" }, + { .compatible = "microchip,mcp3564" }, + { .compatible = "microchip,mcp3561r" }, + { .compatible = "microchip,mcp3562r" }, + { .compatible = "microchip,mcp3564r" }, + { } +}; +MODULE_DEVICE_TABLE(of, mcp356x_dt_ids); + +static const struct spi_device_id mcp356x_id[] = { + { "mcp3461", mcp3461 }, + { "mcp3462", mcp3462 }, + { "mcp3464", mcp3464 }, + { "mcp3461r", mcp3461r }, + { "mcp3462r", mcp3462r }, + { "mcp3464r", mcp3464r }, + { "mcp3561", mcp3561 }, + { "mcp3562", mcp3562 }, + { "mcp3564", mcp3564 }, + { "mcp3561r", mcp3561r }, + { "mcp3562r", mcp3562r }, + { "mcp3564r", mcp3564r }, + { } +}; +MODULE_DEVICE_TABLE(spi, mcp356x_id); + +static struct spi_driver mcp356x_driver = { + .driver = { + .name = "mcp3564", + .of_match_table = mcp356x_dt_ids, + }, + .probe = mcp356x_probe, + .remove = mcp356x_remove, + .id_table = mcp356x_id, +}; + +module_spi_driver(mcp356x_driver); + +MODULE_AUTHOR("Marius Cristea <marius.cristea@microchip.com>"); +MODULE_DESCRIPTION("Microchip MCP346x/MCP346xR and MCP356x/MCP346xR ADCs"); +MODULE_LICENSE("GPL v2"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC 2023-05-19 16:01 ` [PATCH 2/2] iio: adc: adding support for " marius.cristea @ 2023-05-20 15:15 ` Jonathan Cameron 2023-06-22 11:32 ` Marius.Cristea 2023-05-24 11:05 ` Mike Looijmans 1 sibling, 1 reply; 16+ messages in thread From: Jonathan Cameron @ 2023-05-20 15:15 UTC (permalink / raw) To: marius.cristea; +Cc: lars, robh+dt, linux-iio, devicetree, linux-kernel On Fri, 19 May 2023 19:01:45 +0300 <marius.cristea@microchip.com> wrote: > From: Marius Cristea <marius.cristea@microchip.com> > > This is the iio driver for Microchip family of 153.6 ksps, > Low-Noise 16/24-Bit Delta-Sigma ADCs with an SPI interface. > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> Hi Marius, Comments inline, Thanks, Jonathan > --- > .../ABI/testing/sysfs-bus-iio-mcp3564 | 143 ++ > MAINTAINERS | 7 + > drivers/iio/adc/Kconfig | 13 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/mcp3564.c | 1395 +++++++++++++++++ > 5 files changed, 1559 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-mcp3564 > create mode 100644 drivers/iio/adc/mcp3564.c > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-mcp3564 b/Documentation/ABI/testing/sysfs-bus-iio-mcp3564 > new file mode 100644 > index 000000000000..fd65995e2245 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-mcp3564 Don't provide documentation for anything that is standard ABI. We can't have more than one instance of any given path + file in the documentation without breaking it. > @@ -0,0 +1,143 @@ > +What: /sys/bus/iio/devices/iio:deviceX/calibbias > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + Offset Error Digital Calibration Code (two’s complement, > + MSb first coding). The device includes a digital calibration > + feature for offset and gain errors. The calibration > + scheme for offset error consists of the addition of a > + fixed offset value to the ADC output code > + > + This attribute is used to set the offset error digital > + calibration code (two’s complement, MSb first coding). > + The ADC includes a digital calibration feature for offset > + errors. The calibration scheme for offset error consists of the > + addition of a fixed offset value to the ADC output code. > + > +What: /sys/bus/iio/devices/iio:deviceX/calibbias_available > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading returns a range with the possible values for the offset > + error digital calibration register. > + > +What: /sys/bus/iio/devices/iio:deviceX/calibscale > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + This attribute is used to set the gain error digital calibration > + code (unsigned, MSb first coding). The default value is 800000, > + which provides a gain of 1x. > + The ADC includes a digital calibration feature for gain errors. > + The calibration scheme for gain error consists of the > + multiplication of a fixed gain value to the ADC data code. > + > +What: /sys/bus/iio/devices/iio:deviceX/calibscale_available > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading returns a range with the possible values for the gain > + error digital calibration register. > + > +What: /sys/bus/iio/devices/iio:deviceX/boost_current > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + This attribute is used to set the biasing circuit of the > + Delta-Sigma modulator. The different BOOST settings are applied > + to the entire modulator circuit, including the voltage reference > + buffers. > + > +What: /sys/bus/iio/devices/iio:deviceX/boost_current_available > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading returns a list with the possible values for > + the current biasing circuit of the Delta-Sigma modulator. > + > + "x0.5", - ADC channel has current x 0.5 Keep just numbers in the attr. It should be named in a fashion that makes it apparent that it's a multiplier, not an absolute current. New ABI like this is best avoided if we can. I see from a quick glance at the datasheet that there is advice on controlling this to allow for different clock settings, but I'm not sure if that's a simple relationship or not. From https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4-Two-Four-Eight-Channel-153.6-ksps-Low-Noise-16-Bit-Delta-Sigma-ADC-Data-Sheet-20006180D.pdf figures 2-20 onwards, it looks like this effectively trades off power consumption against max frequency, so maybe we could set it automatically? > + > + "x0.66", - ADC channel has current x 0.66 > + > + "x1", - ADC channel has current x 1 (default) > + > + "x2" - ADC channel has current x 2 > + > +What: /sys/bus/iio/devices/iio:deviceX/current_bias Another bit of custom ABI with the normal problems of it being unknown to standard userspace software. You could perhaps use an output channel for this and just treat it like a current DAC, with a label that makes it's relationship to the inputs obvious. > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + This attribute is used to set the current Source/Sink > + values for sensor bias. The ADC inputs, VIN-/VIN+, feature a > + selectable burnout current source, which enables open or > + short-circuit detection, as well as biasing very low-current > + external sensors. The bias current is sourced on the VIN+ pin of > + the ADC (noninverting output of the analog multiplexer) > + and sunk on the VIN- pin of the ADC (inverting output of > + the analog multiplexer). Since the same current flows > + at the VIN-/VIN+ pins of the ADC, it can sense the > + impedance of an externally connected sensor that > + would be connected between the selected inputs of the > + multiplexer. > + > +What: /sys/bus/iio/devices/iio:deviceX/current_bias_available > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading returns a list with the possible values for > + the current source/sink selection values for sensor bias. > + > + "15_uA" - 15 uA is applied to the ADC inputs. Match to standard IIO units for a current then drop the postfix. > + > + "3.7_uA" - 3.7 uA is applied to the ADC inputs. > + > + "0.9_uA" - 0.9 uA is applied to the ADC inputs. > + > + "no_current" - "No current source is applied to the ADC > + inputs (default)" 0 would seem self explanatory for this an is easier for userspace software to deal with. Also, this doc should not include the values, merely that there is a list. > + > +What: /sys/bus/iio/devices/iio:deviceX/enable_auto_zeroing_mux > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + This attribute is used to enable the analog input multiplexer > + auto-zeroing algorithm. Write '1' to enable it, write '0' to > + disable it. If you can explain what auto zeroing is that would be great. It's not something I recall seeing in other drivers. > + > +What: /sys/bus/iio/devices/iio:deviceX/enable_auto_zeroing_ref > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + This attribute is used to enable the chopping algorithm for the > + internal voltage reference buffer. Write '1' to enable it, > + write '0' to disable it. > + > +What: /sys/bus/iio/devices/iio:deviceX/hardwaregain > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + This attribute is used to set the hardware applied gain factor. > + It is shared across all channels. This attr is pretty much only used when the gain is not directly related to the value being used. An example being a time of flight sensor where the amplitude of the measured signal doesn't directly matter as we are looking for the timing of the peak, we just need it to be big enough to measure. Otherwise scale is what you want. > + > +What: /sys/bus/iio/devices/iio:deviceX/hardwaregain_available > +KernelVersion: 6.4 > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading this attribute will list all available hardware applied gain factors. > + Shared across all channels. Docs shouldn't included this level of detail - they should say what the file information means, not what the exact values are. > + > + "0.33" - Gain is x1/3 ( x0.33 analog) > + > + "1" - Gain is x1 ( x8 analog) (default) x8? > + > + "2" - Gain is x2 ( x2 analog) > + > + "4" - Gain is x4 ( x4 analog) > + > + "8" - Gain is x8 ( x8 analog) > + > + "16" - Gain is x16 (x16 analog) > + > + "32" - Gain is x32 (x16 analog, x2 digital) > + > + "64" - Gain is x64 (x16 analog, x4 digital) > \ No newline at end of file add one ;) > diff --git a/MAINTAINERS b/MAINTAINERS > index 7e0b87d5aa2e..2fd2d06fb87f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13786,6 +13786,13 @@ S: Supported > F: Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt > F: drivers/regulator/mcp16502.c > > +MICROCHIP MCP3564 ADC DRIVER > +M: Marius Cristea <marius.cristea@microchip.com> > +L: linux-iio@vger.kernel.org > +S: Supported > +F: Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml > +F: drivers/iio/adc/mcp3564.c > + > MICROCHIP MCP3911 ADC DRIVER > M: Marcus Folkesson <marcus.folkesson@gmail.com> > M: Kent Gustavsson <kent@minoris.se> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index eb2b09ef5d5b..9ee85764b985 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -768,6 +768,19 @@ config MCP3422 > This driver can also be built as a module. If so, the module will be > called mcp3422. > > +config MCP3564 > + tristate "Microchip Technology MCP3461/2/4/R, MCP3561/2/4/R driver" > + depends on SPI > + depends on IIO > + help > + Say yes here to build support for Microchip Technology's MCP3461, > + MCP3462, MCP3464, MCP3461R, MCP3462R, MCP3464R, MCP3561, MCP3562, > + MCP3564, MCP3561R, MCP3562R and MCP3564R analog to digital > + converters. > + > + This driver can also be built as a module. If so, the module will be > + called mcp3564. > + > config MCP3911 > tristate "Microchip Technology MCP3911 driver" > depends on SPI > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index e07e4a3e6237..78aed4878e86 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -70,6 +70,7 @@ obj-$(CONFIG_MAX1363) += max1363.o > obj-$(CONFIG_MAX9611) += max9611.o > obj-$(CONFIG_MCP320X) += mcp320x.o > obj-$(CONFIG_MCP3422) += mcp3422.o > +obj-$(CONFIG_MCP356X) += mcp3564.o That won't work as config variable is MCP3564 Which is good given wildcards always go wrong in the end! > obj-$(CONFIG_MCP3911) += mcp3911.o > obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o > obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o > diff --git a/drivers/iio/adc/mcp3564.c b/drivers/iio/adc/mcp3564.c > new file mode 100644 > index 000000000000..dfdbe4e37cd4 > --- /dev/null > +++ b/drivers/iio/adc/mcp3564.c > @@ -0,0 +1,1395 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * IIO driver for MCP356X/MCP356XR and MCP346X/MCP346XR series ADC chip family > + * > + * Copyright (C) 2022-2023 Microchip Technology Inc. and its subsidiaries > + * > + * Author: Marius Cristea <marius.cristea@microchip.com> > + * > + * Datasheet for MCP3561, MCP3562, MCP3564 can be found here: > + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP3561-2-4-Family-Data-Sheet-DS20006181C.pdf > + * Datasheet for MCP3561R, MCP3562R, MCP3564R can be found here: > + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3561_2_4R-Data-Sheet-DS200006391C.pdf > + * Datasheet for MCP3461, MCP3462, MCP3464 can be found here: > + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4-Two-Four-Eight-Channel-153.6-ksps-Low-Noise-16-Bit-Delta-Sigma-ADC-Data-Sheet-20006180D.pdf > + * Datasheet for MCP3461R, MCP3462R, MCP3464R can be found here: > + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4R-Family-Data-Sheet-DS20006404C.pdf > + * > + */ > + > +#include <linux/acpi.h> > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/iopoll.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > +#include <linux/util_macros.h> > + > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > + > +#define MCP356X_ADCDATA 0x00 I'd rather not see wildcards as it implies these apply to other MCP356x devices e.g. the mcp3565 (actually they may well do for that particular part, but that's not always the case :) > +#define MCP356X_CONFIG0 0x01 > +#define MCP356X_CONFIG1 0x02 > +#define MCP356X_CONFIG2 0x03 > +#define MCP356X_CONFIG3 0x04 > +#define MCP356X_IRQ 0x05 > +#define MCP356X_MUX 0x06 > +#define MCP356X_SCAN 0x07 > +#define MCP356X_TIMER 0x08 > +#define MCP356X_OFFSETCAL 0x09 > +#define MCP356X_GAINCAL 0x0A > +#define MCP356X_RESERVED_B 0x0B > +#define MCP356X_RESERVED_C 0x0C > +#define MCP356X_LOCK 0x0D > +#define MCP356X_RESERVED_E 0x0E > +#define MCP356X_CRCCFG 0x0F > + > +#define MCP356X_FAST_CMD_CTRL 0 > +#define MCP356X_RD_CTRL BIT(0) > +#define MCP356X_WRT_CTRL BIT(1) > + > +#define MCP356X_FULL_RESET_CMD GENMASK(3, 1) > + > +#define MCP3461_HW_ID BIT(3) > +#define MCP3462_HW_ID 0x0009 > +#define MCP3464_HW_ID 0x000B > + > +#define MCP3561_HW_ID GENMASK(3, 2) > +#define MCP3562_HW_ID 0x000D > +#define MCP3564_HW_ID GENMASK(3, 0) As below. This isn't a mask, it's a numebr so GENMASK not appropriate. > +#define MCP356X_HW_ID_MASK GENMASK(3, 0) This is a mask, so here GENMASK is the right choice. > + > +#define MCP356XR_INT_VREF_MV 2400 > + > +/* MUX_VIN Input Selection > + */ > +#define MCP356X_INTERNAL_VCM GENMASK(3, 0) Only use GENMASK if it's actually a mask, not just because it happens to be some bits in a row. Just use values in that case. Same for BIT. These are just the numbers 0 to 15 with some gaps. > +#define MCP356X_TEMP_DIODE_M GENMASK(3, 1) > +#define MCP356X_TEMP_DIODE_P 0b1101 > +#define MCP356X_REFIN_NEG GENMASK(3, 2) > +#define MCP356X_REFIN_POZ 0b1011 > +#define MCP356X_RESERVED 0b1010 /* do not use */ > +#define MCP356X_AVDD 0b1001 > +#define MCP356X_AGND BIT(3) > +#define MCP356X_CH7 GENMASK(2, 0) > +#define MCP356X_CH6 GENMASK(2, 1) > +#define MCP356X_CH5 0b0101 > +#define MCP356X_CH4 BIT(2) > +#define MCP356X_CH3 GENMASK(1, 0) > +#define MCP356X_CH2 BIT(1) > +#define MCP356X_CH1 BIT(0) > +#define MCP356X_CH0 0 > + > +#define MCP356X_ADC_MODE_MASK GENMASK(1, 0) > + > +#define MCP356X_ADC_DEFAULT_MODE 0 > +#define MCP356X_ADC_SHUTDOWN_MODE BIT(0) > +#define MCP356X_ADC_STANDBY BIT(1) > +#define MCP356X_ADC_CONVERSION_MODE GENMASK(1, 0) > + > +#define MCP356X_DATA_READY_MASK BIT(6) > + > +#define MCP356X_OVERSAMPLING_RATIO_32 0 > +#define MCP356X_OVERSAMPLING_RATIO_64 BIT(0) > +#define MCP356X_OVERSAMPLING_RATIO_128 BIT(1) > +#define MCP356X_OVERSAMPLING_RATIO_256 GENMASK(1, 0) > +#define MCP356X_OVERSAMPLING_RATIO_512 BIT(2) > +#define MCP356X_OVERSAMPLING_RATIO_1024 0x05 > +#define MCP356X_OVERSAMPLING_RATIO_2048 GENMASK(2, 1) > +#define MCP356X_OVERSAMPLING_RATIO_4096 GENMASK(2, 0) > +#define MCP356X_OVERSAMPLING_RATIO_8192 BIT(3) > +#define MCP356X_OVERSAMPLING_RATIO_16384 0x09 > +#define MCP356X_OVERSAMPLING_RATIO_20480 0x0A > +#define MCP356X_OVERSAMPLING_RATIO_24576 0x0B > +#define MCP356X_OVERSAMPLING_RATIO_40960 0x0C > +#define MCP356X_OVERSAMPLING_RATIO_49152 0x0D > +#define MCP356X_OVERSAMPLING_RATIO_81920 GENMASK(3, 1) > +#define MCP356X_OVERSAMPLING_RATIO_98304 GENMASK(3, 0) > + > +#define MCP356X_OVERSAMPLING_RATIO_MASK GENMASK(5, 2) > +#define MCP356X_OVERSAMPLING_RATIO_SHIFT 0x02 > + > +#define MCP356X_HARDWARE_GAIN_MASK GENMASK(5, 3) > +#define MCP356X_HARDWARE_GAIN_SHIFT 0x03 > +#define MCP356X_DEFAULT_HARDWARE_GAIN BIT(1) > + > +#define MCP356X_CS_SEL_0_0_uA 0x0 > +#define MCP356X_CS_SEL_0_9_uA BIT(0) > +#define MCP356X_CS_SEL_3_7_uA BIT(1) > +#define MCP356X_CS_SEL_15_uA GENMASK(1, 0) > + > +#define MCP356X_CS_SEL_MASK GENMASK(3, 2) > + > +#define MCP356X_BOOST_CURRENT_x0_50 0 > +#define MCP356X_BOOST_CURRENT_x0_66 BIT(0) > +#define MCP356X_BOOST_CURRENT_x1_00 BIT(1) > +#define MCP356X_BOOST_CURRENT_x2_00 GENMASK(1, 0) > + > +#define MCP356X_BOOST_CURRENT_MASK GENMASK(7, 6) > + > +/* Auto-Zeroing MUX Setting */ > +#define MCP356X_AZ_MUX_MASK BIT(2) > +/* Auto-Zeroing REF Setting */ > +#define MCP356X_AZ_REF_MASK BIT(1) > + > +#define MCP356X_SHARED_DEVATTRS_COUNT 1 > +#define MCP356X_PARTICULAR_DEVATTRS_COUNT 1 > + > +#define MAX_HWGAIN 64000 Event his wants to be prefixed because we may end up with a similar define in a generic header at somepoint which would be confusing. > + > +#define MCP356X_DATA_READY_TIMEOUT_MS 2000 > + > +enum mcp356x_ids { > + mcp3461, > + mcp3462, > + mcp3464, > + mcp3461r, > + mcp3462r, > + mcp3464r, > + mcp3561, > + mcp3562, > + mcp3564, > + mcp3561r, > + mcp3562r, > + mcp3564r, > +}; > + > +static const unsigned int mcp356x_oversampling_avail[16] = { > + [MCP356X_OVERSAMPLING_RATIO_32] = 32, > + [MCP356X_OVERSAMPLING_RATIO_64] = 64, > + [MCP356X_OVERSAMPLING_RATIO_128] = 128, > + [MCP356X_OVERSAMPLING_RATIO_256] = 256, > + [MCP356X_OVERSAMPLING_RATIO_512] = 512, > + [MCP356X_OVERSAMPLING_RATIO_1024] = 1024, > + [MCP356X_OVERSAMPLING_RATIO_2048] = 2048, > + [MCP356X_OVERSAMPLING_RATIO_4096] = 4096, > + [MCP356X_OVERSAMPLING_RATIO_8192] = 8192, > + [MCP356X_OVERSAMPLING_RATIO_16384] = 16384, > + [MCP356X_OVERSAMPLING_RATIO_20480] = 20480, > + [MCP356X_OVERSAMPLING_RATIO_24576] = 24576, > + [MCP356X_OVERSAMPLING_RATIO_40960] = 40960, > + [MCP356X_OVERSAMPLING_RATIO_49152] = 49152, > + [MCP356X_OVERSAMPLING_RATIO_81920] = 81920, > + [MCP356X_OVERSAMPLING_RATIO_98304] = 98304 > +}; > + > +/* > + * Current Source/Sink Selection Bits for Sensor Bias (source on VIN+/sink on VIN-) > + */ > +static const char * const mcp356x_current_bias_avail[] = { > + [MCP356X_CS_SEL_0_0_uA] = "no_current(default)", > + [MCP356X_CS_SEL_0_9_uA] = "0.9_uA", > + [MCP356X_CS_SEL_3_7_uA] = "3.7_uA", > + [MCP356X_CS_SEL_15_uA] = "15_uA", > +}; > + > +/* > + * BOOST[1:0]: ADC Bias Current Selection > + */ > +static const char * const mcp356x_boost_current_avail[] = { > + [MCP356X_BOOST_CURRENT_x0_50] = "x0.5", > + [MCP356X_BOOST_CURRENT_x0_66] = "x0.66", > + [MCP356X_BOOST_CURRENT_x1_00] = "x1_(default)", > + [MCP356X_BOOST_CURRENT_x2_00] = "x2", See comments above. These should be IIO_VAL_INT_PLUS_MICRO pairs, not xX > +}; > + > +/* > + * Calibration bias values > + */ > +static const int mcp356x_calib_bias[] = { > + -8388608, /* min: -2^23 */ > + 1, /* step: 1 */ > + 8388607 /* max: 2^23 - 1 */ > +}; > + > +/* > + * Calibration scale values > + * The Gain Error Calibration register (GAINCAL) is an > + * unsigned 24-bit register that holds the digital gain error > + * calibration value, GAINCAL which could be calculated by > + * GAINCAL (V/V) = (GAINCAL[23:0])/8388608 > + * The gain error calibration value range in equivalent voltage is [0; 2-2^(-23)] > + */ > +static const unsigned int mcp356x_calib_scale[] = { > + 0, /* min: 0 */ > + 1, /* step: 1/8388608 */ > + 16777215 /* max: 2 - 2^(-23) */ > +}; > + > +/* Programmable hardware gain x1/3, x1, x2, x4, x8, x16, x32, x64 */ > +static const int mcp356x_hwgain_frac[] = { > + 3, > + 10, I think these are pairs so 3, 10, 1, 1, 1, 2, 1, 4, etc. > + 1, > + 1, > + 2, > + 1, > + 4, > + 1, > + 8, > + 1, > + 16, > + 1, > + 32, > + 1, > + 64, > + 1 > +}; > + > +static const int mcp356x_hwgain[] = { > + 300, > + 1000, > + 2000, > + 4000, > + 8000, > + 16000, > + 32000, > + 64000 > +}; > + > +/** > + * struct mcp356x_chip_info - chip specific data > + * @channels: struct iio_chan_spec matching the device's capabilities > + * @num_channels: number of channels > + * @int_vref_uv: internal voltage reference value in microVolts > + * @has_vref: Does the ADC has an internal voltage reference? > + */ > +struct mcp356x_chip_info { > + const struct iio_chan_spec *channels; > + unsigned int num_channels; > + unsigned int int_vref_uv; > + bool has_vref; > +}; > + > +/** > + * struct mcp356x_state - working data for a ADC device > + * @chip_info: chip specific data > + * @mcp356x_info: information about iio device > + * @spi: SPI device structure > + * @vref: The regulator device used as a voltage reference in case > + * external voltage reference is used > + * @vref_mv: voltage reference value in miliVolts > + * @lock: mutex to prevent concurrent reads/writes > + * @dev_addr: hardware device address > + * @oversampling: the index inside oversampling list of the ADC > + * @hwgain: the index inside hardware gain list of the ADC > + * @calib_bias: calibration bias value > + * @calib_scale: calibration scale value > + * @current_boost_mode: the index inside current boost list of the ADC > + * @current_bias_mode: the index inside current bias list of the ADC > + * @auto_zeroing_mux: set if ADC auto-zeroing algorithm is enabled > + * @auto_zeroing_ref: set if ADC auto-Zeroing Reference Buffer Setting is enabled > + */ > +struct mcp356x_state { > + const struct mcp356x_chip_info *chip_info; > + struct iio_info mcp356x_info; Shouldn't need a copy of this. The real one in iio_dev should always be available any where you need it. > + struct spi_device *spi; > + struct regulator *vref; > + unsigned short vref_mv; > + struct mutex lock; /*lock to prevent concurrent reads/writes */ The spi core bus locks prevent that. Please add more specific dos. Also fine to just have docs in the kernel-doc block above, don't need this here as well. > + u8 dev_addr; > + unsigned int oversampling; > + unsigned int hwgain; > + int calib_bias; > + int calib_scale; > + unsigned int current_boost_mode; Modes tend to be better represented as enums. Can that work here? Means the compiler can see what values are possible and complain if others are used. > + unsigned int current_bias_mode; > + bool auto_zeroing_mux; > + bool auto_zeroing_ref; > +}; > + > +static inline u8 mcp356x_reg_write(u8 chip_addr, u8 reg) > +{ > + return ((chip_addr << 6) | (reg << 2) | MCP356X_WRT_CTRL); > +} > + > +static inline u8 mcp356x_reg_read(u8 chip_addr, u8 reg) > +{ > + return ((chip_addr << 6) | (reg << 2) | MCP356X_RD_CTRL); > +} > + > +static inline u8 mcp356x_reg_fast_cmd(u8 chip_addr, u8 cmd) > +{ > + return ((chip_addr << 6) | (cmd << 2)); FIELD_PREP() + masks Also drop the extra outer brackets. > +} > + > +static int mcp356x_read(struct mcp356x_state *adc, u8 reg, u32 *val, u8 len) I'm in two minds about these read functions as they are more complex than say just having mcp356x_read_8bits() mcp356x_read_16bits() mcp356x_read_24bits() As then you a only do an endian conversion if it matters (not the 8 bit ones) and it's obvious what is going on + you can use get_unaligned_be24 for example to make it even clearer. > +{ > + int ret; > + u8 tmp_reg; > + > + tmp_reg = mcp356x_reg_read(adc->dev_addr, reg); > + > + ret = spi_write_then_read(adc->spi, &tmp_reg, 1, val, len); > + > + be32_to_cpus(val); As below, better to enforce type as __be32 > + *val >>= ((4 - len) * 8); > + > + return ret; > +} > + > +static int mcp356x_write(struct mcp356x_state *adc, u8 reg, u32 val, u8 len) > +{ > + val |= (mcp356x_reg_write(adc->dev_addr, reg) << (len * 8)); > + val <<= (3 - len) * 8; > + cpu_to_be32s(&val); Use a __be32 to store the temporary so that we can track the endian type cleanly. > + > + return spi_write(adc->spi, &val, len + 1); spi_write needs a dma safe buffer. Either allocate it on the stack, or embed it in iio_priv() taking care to mark it appropriately aligned (lots of examples of this in IIO). Or just use spi_write_then_read() with a 0 size read as that uses bounce buffers to achieve DMA safety. As above, I'd be tempted to do sized versions as there are only a few different options and they will be more readable + use unaligned puts to make it obvious what is going on. > +} > + > +static int mcp356x_fast_cmd(struct mcp356x_state *adc, u8 fast_cmd) > +{ > + u8 val; > + > + val = mcp356x_reg_fast_cmd(adc->dev_addr, fast_cmd); > + > + return spi_write(adc->spi, &val, 1); > +} > + > +static int mcp356x_update(struct mcp356x_state *adc, u8 reg, u32 mask, u32 val, > + u8 len) > +{ > + u32 tmp; > + int ret; > + > + ret = mcp356x_read(adc, reg, &tmp, len); if (ret < 0) return ret; val &= mask ... return mc356x_write(); > + > + if (ret == 0) { > + val &= mask; > + val |= tmp & ~mask; > + ret = mcp356x_write(adc, reg, val, len); > + } > + > + return ret; > +} > + > +/* Custom IIO Device Attributes */ Obvious comment, drop it. This sort of 'section' title frequently gets broken as code gets moved around. > +static int mcp356x_set_current_boost_mode(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + unsigned int mode) > +{ > + struct mcp356x_state *adc = iio_priv(indio_dev); > + int ret; > + > + dev_dbg(&indio_dev->dev, "%s: %d\n", __func__, mode); > + > + mutex_lock(&adc->lock); > + ret = mcp356x_update(adc, MCP356X_CONFIG2, MCP356X_BOOST_CURRENT_MASK, > + mode, 1); > + > + if (ret) > + dev_err(&indio_dev->dev, "Failed to configure CONFIG2 register\n"); > + else > + adc->current_boost_mode = mode; > + > + mutex_unlock(&adc->lock); > + > + return ret; > +} > + > +static int mcp356x_get_current_boost_mode(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan) > +{ > + struct mcp356x_state *adc = iio_priv(indio_dev); > + > + return adc->current_boost_mode; > +} > + > +static int mcp356x_set_current_bias_mode(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + unsigned int mode) > +{ > + struct mcp356x_state *adc = iio_priv(indio_dev); > + int ret; > + > + dev_dbg(&indio_dev->dev, "%s: %d\n", __func__, mode); > + > + mutex_lock(&adc->lock); > + ret = mcp356x_update(adc, MCP356X_CONFIG0, MCP356X_CS_SEL_MASK, mode, 1); > + > + if (ret) > + dev_err(&indio_dev->dev, "Failed to configure CONFIG0 register\n"); > + else > + adc->current_bias_mode = mode; > + > + mutex_unlock(&adc->lock); > + > + return ret; > +} > + > +static int mcp356x_get_current_bias_mode(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan) > +{ > + struct mcp356x_state *adc = iio_priv(indio_dev); > + > + return adc->current_bias_mode; > +} > + > +static const struct iio_enum mcp356x_current_boost_mode_enum = { > + .items = mcp356x_boost_current_avail, > + .num_items = ARRAY_SIZE(mcp356x_boost_current_avail), > + .set = mcp356x_set_current_boost_mode, > + .get = mcp356x_get_current_boost_mode, Not going to comment again on these. Before we focus on implementation need to deal with question of whether we want them / how useful they are to expose. > +}; > + > +static const struct iio_enum mcp356x_current_bias_mode_enum = { > + .items = mcp356x_current_bias_avail, > + .num_items = ARRAY_SIZE(mcp356x_current_bias_avail), > + .set = mcp356x_set_current_bias_mode, > + .get = mcp356x_get_current_bias_mode, > +}; > + > +static const struct iio_chan_spec_ext_info mcp356x_ext_info[] = { > + IIO_ENUM("boost_current", IIO_SHARED_BY_ALL, &mcp356x_current_boost_mode_enum), > + { > + .name = "boost_current_available", > + .shared = IIO_SHARED_BY_ALL, > + .read = iio_enum_available_read, > + .private = (uintptr_t)&mcp356x_current_boost_mode_enum, > + }, > + IIO_ENUM("current_bias", IIO_SHARED_BY_ALL, &mcp356x_current_bias_mode_enum), > + { > + .name = "current_bias_available", > + .shared = IIO_SHARED_BY_ALL, > + .read = iio_enum_available_read, > + .private = (uintptr_t)&mcp356x_current_bias_mode_enum, > + }, > + {} > +}; > + > +static ssize_t mcp356x_auto_zeroing_mux_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct mcp356x_state *adc = iio_priv(indio_dev); > + > + return sysfs_emit(buf, "%d\n", adc->auto_zeroing_mux); > +} > + > +static ssize_t mcp356x_auto_zeroing_mux_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct mcp356x_state *adc = iio_priv(indio_dev); > + bool auto_zero; > + int ret; > + > + ret = kstrtobool(buf, &auto_zero); > + if (ret) > + return ret; > + > + mutex_lock(&adc->lock); > + ret = mcp356x_update(adc, MCP356X_CONFIG2, MCP356X_AZ_MUX_MASK, > + (u32)auto_zero, 1); > + > + if (ret) > + dev_err(&indio_dev->dev, "Failed to update CONFIG2 register\n"); > + else > + adc->auto_zeroing_mux = auto_zero; > + > + mutex_unlock(&adc->lock); > + > + return ret ? ret : len; > +} > + > +static ssize_t mcp356x_auto_zeroing_ref_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct mcp356x_state *adc = iio_priv(indio_dev); > + > + return sysfs_emit(buf, "%d\n", adc->auto_zeroing_ref); > +} > + > +static ssize_t mcp356x_auto_zeroing_ref_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct mcp356x_state *adc = iio_priv(indio_dev); > + bool auto_zero; > + int ret; > + > + ret = kstrtobool(buf, &auto_zero); > + if (ret) > + return ret; > + > + mutex_lock(&adc->lock); > + ret = mcp356x_update(adc, MCP356X_CONFIG2, MCP356X_AZ_REF_MASK, > + (u32)auto_zero, 1); > + > + if (ret) > + dev_err(&indio_dev->dev, "Failed to update CONFIG2 register\n"); > + else > + adc->auto_zeroing_ref = auto_zero; > + > + mutex_unlock(&adc->lock); > + > + return ret ? ret : len; > +} > + > +#define MCP356X_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr) > + > +static IIO_DEVICE_ATTR(enable_auto_zeroing_ref, 0644, > + mcp356x_auto_zeroing_ref_show, > + mcp356x_auto_zeroing_ref_store, 0); > + > +static struct attribute *mcp356x_particular_attributes[] = { > + MCP356X_DEV_ATTR(enable_auto_zeroing_ref), > + NULL > +}; > + > +static IIO_DEVICE_ATTR(enable_auto_zeroing_mux, 0644, > + mcp356x_auto_zeroing_mux_show, > + mcp356x_auto_zeroing_mux_store, 0); > + > +static struct attribute *mcp356x_shared_attributes[] = { > + MCP356X_DEV_ATTR(enable_auto_zeroing_mux), > + NULL, No trailing comma needed on a NULL terminating element as we can't stick anything after it. > +}; > + > +static int mcp356x_prep_custom_attributes(struct mcp356x_state *adc, > + struct iio_dev *indio_dev) > +{ > + int i; > + struct attribute **mcp356x_custom_attr; > + struct attribute_group *mcp356x_group; > + > + mcp356x_group = devm_kzalloc(&adc->spi->dev, sizeof(*mcp356x_group), GFP_KERNEL); > + > + if (!mcp356x_group) > + return (-ENOMEM); return -ENOMEM; same elsewhere. > + > + mcp356x_custom_attr = devm_kzalloc(&adc->spi->dev, (MCP356X_SHARED_DEVATTRS_COUNT + > + MCP356X_PARTICULAR_DEVATTRS_COUNT + 1) * sizeof(struct attribute *), > + GFP_KERNEL); > + > + if (!mcp356x_custom_attr) > + return (-ENOMEM); > + > + for (i = 0; i < MCP356X_SHARED_DEVATTRS_COUNT; i++) > + mcp356x_custom_attr[i] = mcp356x_shared_attributes[i]; > + > + if (adc->chip_info->has_vref) { > + dev_dbg(&indio_dev->dev, "Setup custom attr for R variant\n"); > + for (i = 0; i < MCP356X_PARTICULAR_DEVATTRS_COUNT; i++) > + mcp356x_custom_attr[MCP356X_SHARED_DEVATTRS_COUNT + i] = > + mcp356x_particular_attributes[i]; > + } > + > + mcp356x_group->attrs = mcp356x_custom_attr; > + adc->mcp356x_info.attrs = mcp356x_group; This looks to me like you are picking between two fixed sets. Just define both sets as static const and pick between dpending on has_vref. return attrs and set them in the caller so you don't need the mcp356x_info element in the adc struct. > + > + return 0; > +} > + > +#define MCP356X_V_CHANNEL(index, addr, depth) { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = (index), \ > + .address = (((addr) << 4) | MCP356X_AGND), \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \ > + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ > + BIT(IIO_CHAN_INFO_CALIBSCALE) | \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_CALIBSCALE) | \ > + BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \ > + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > + .ext_info = mcp356x_ext_info, \ > + .scan_index = 0, \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = depth, \ > + .storagebits = 32, \ > + .endianness = IIO_BE, \ > + }, \ > +} > + > +#define MCP356X_T_CHAN(depth) { \ > + .type = IIO_TEMP, \ > + .channel = 0, \ > + .address = ((MCP356X_TEMP_DIODE_P << 4) | MCP356X_TEMP_DIODE_M), \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \ > + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ > + BIT(IIO_CHAN_INFO_CALIBSCALE) | \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_CALIBSCALE) | \ > + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > + .ext_info = mcp356x_ext_info, \ > + .scan_index = 0, \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = depth, \ > + .storagebits = 32, \ > + .endianness = IIO_BE, \ > + }, \ > +} > + > +#define MCP356X_V_CHANNEL_DIFF(chan1, chan2, addr, depth) { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = (chan1), \ > + .channel2 = (chan2), \ > + .address = (addr), \ > + .differential = 1, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \ > + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ > + BIT(IIO_CHAN_INFO_CALIBSCALE) | \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_CALIBSCALE) | \ > + BIT(IIO_CHAN_INFO_CALIBBIAS) | \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > + .ext_info = mcp356x_ext_info, \ > + .scan_index = 0, \ > + .scan_type = { \ Don't set any values you don't use - they can be misleading. For instance if you had implemented buffered capture (the chrdev route) then you could not have scan_index = 0 for all the instances of this. Also, you probably don't use any/all of the scan_type fields - so don't set them. > + .sign = 's', \ > + .realbits = depth, \ > + .storagebits = 32, \ > + .endianness = IIO_BE, \ > + }, \ > +} > + > +#define MCP3561_CHANNELS(depth) { \ > + MCP356X_V_CHANNEL(0, 0, depth), \ > + MCP356X_V_CHANNEL(1, 1, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth), \ > + MCP356X_T_CHAN(depth) \ > +} > + > +#define MCP3562_CHANNELS(depth) { \ > + MCP356X_V_CHANNEL(0, 0, depth), \ > + MCP356X_V_CHANNEL(1, 1, depth), \ > + MCP356X_V_CHANNEL(2, 2, depth), \ > + MCP356X_V_CHANNEL(3, 3, depth), \ > + MCP356X_T_CHAN(depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 2, 0x02, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 3, 0x03, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 2, 0x12, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 3, 0x13, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 3, 0x23, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 0, 0x20, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 0, 0x30, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 1, 0x21, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 1, 0x31, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 2, 0x32, depth), \ > +} > + > +#define MCP3564_CHANNELS(depth) { \ > + MCP356X_V_CHANNEL(0, 0, depth), \ > + MCP356X_V_CHANNEL(1, 1, depth), \ > + MCP356X_V_CHANNEL(2, 2, depth), \ > + MCP356X_V_CHANNEL(3, 3, depth), \ > + MCP356X_V_CHANNEL(4, 4, depth), \ > + MCP356X_V_CHANNEL(5, 5, depth), \ > + MCP356X_V_CHANNEL(6, 6, depth), \ > + MCP356X_V_CHANNEL(7, 7, depth), \ > + MCP356X_T_CHAN(depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 2, 0x02, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 3, 0x03, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 2, 0x12, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 3, 0x13, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 3, 0x23, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 0, 0x20, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 0, 0x30, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 1, 0x21, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 1, 0x31, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 2, 0x32, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 4, 0x04, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 5, 0x05, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 6, 0x06, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 7, 0x07, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 4, 0x14, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 5, 0x15, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 6, 0x16, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 7, 0x17, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 4, 0x24, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 5, 0x25, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 6, 0x26, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 7, 0x27, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 4, 0x34, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 5, 0x35, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 6, 0x36, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 7, 0x37, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 5, 0x45, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 6, 0x46, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 7, 0x47, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 6, 0x56, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 7, 0x57, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 7, 0x67, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 0, 0x40, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 0, 0x50, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 0, 0x60, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 0, 0x70, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 1, 0x41, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 1, 0x51, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 1, 0x61, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 1, 0x71, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 2, 0x42, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 2, 0x52, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 2, 0x62, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 2, 0x72, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 3, 0x43, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 3, 0x53, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 3, 0x63, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 3, 0x73, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 4, 0x54, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 4, 0x64, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 4, 0x74, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 5, 0x65, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 5, 0x75, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 6, 0x76, depth) \ > +} > + > +static const struct iio_chan_spec mcp3461_channels[] = MCP3561_CHANNELS(16); > +static const struct iio_chan_spec mcp3462_channels[] = MCP3562_CHANNELS(16); > +static const struct iio_chan_spec mcp3464_channels[] = MCP3564_CHANNELS(16); > +static const struct iio_chan_spec mcp3561_channels[] = MCP3561_CHANNELS(24); > +static const struct iio_chan_spec mcp3562_channels[] = MCP3562_CHANNELS(24); > +static const struct iio_chan_spec mcp3564_channels[] = MCP3564_CHANNELS(24); > + > +static const struct mcp356x_chip_info mcp356x_chip_infos_tbl[] = { > + [mcp3461] = { > + .channels = mcp3461_channels, Put the name in each of these as well as it avoids trying to extract it from the id tables which tends to be error prone as a driver evolves over time. > + .num_channels = ARRAY_SIZE(mcp3461_channels), > + .int_vref_uv = 0, > + .has_vref = false This is enough to decide whether the vref is optional. So use it for that. > + }, > + [mcp3462] = { > + .channels = mcp3462_channels, > + .num_channels = ARRAY_SIZE(mcp3462_channels), > + .int_vref_uv = 0, > + .has_vref = false > + }, > + [mcp3464] = { > + .channels = mcp3464_channels, > + .num_channels = ARRAY_SIZE(mcp3464_channels), > + .int_vref_uv = 0, > + .has_vref = false > + }, > + [mcp3461r] = { > + .channels = mcp3461_channels, > + .num_channels = ARRAY_SIZE(mcp3461_channels), > + .int_vref_uv = MCP356XR_INT_VREF_MV, > + .has_vref = true > + }, > + [mcp3462r] = { > + .channels = mcp3462_channels, > + .num_channels = ARRAY_SIZE(mcp3462_channels), > + .int_vref_uv = MCP356XR_INT_VREF_MV, > + .has_vref = true > + }, > + [mcp3464r] = { > + .channels = mcp3464_channels, > + .num_channels = ARRAY_SIZE(mcp3464_channels), > + .int_vref_uv = MCP356XR_INT_VREF_MV, > + .has_vref = true > + }, > + [mcp3561] = { > + .channels = mcp3561_channels, > + .num_channels = ARRAY_SIZE(mcp3561_channels), > + .int_vref_uv = 0, > + .has_vref = false > + }, > + [mcp3562] = { > + .channels = mcp3562_channels, > + .num_channels = ARRAY_SIZE(mcp3562_channels), > + .int_vref_uv = 0, > + .has_vref = false > + }, > + [mcp3564] = { > + .channels = mcp3564_channels, > + .num_channels = ARRAY_SIZE(mcp3564_channels), > + .int_vref_uv = 0, > + .has_vref = false > + }, > + [mcp3561r] = { > + .channels = mcp3561_channels, > + .num_channels = ARRAY_SIZE(mcp3561_channels), > + .int_vref_uv = MCP356XR_INT_VREF_MV, > + .has_vref = true > + }, > + [mcp3562r] = { > + .channels = mcp3562_channels, > + .num_channels = ARRAY_SIZE(mcp3562_channels), > + .int_vref_uv = MCP356XR_INT_VREF_MV, > + .has_vref = true > + }, > + [mcp3564r] = { > + .channels = mcp3564_channels, > + .num_channels = ARRAY_SIZE(mcp3564_channels), > + .int_vref_uv = MCP356XR_INT_VREF_MV, > + .has_vref = true > + }, > +}; > + > +static int mcp356x_read_single_value(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, > + int *val) > +{ > + struct mcp356x_state *adc = iio_priv(indio_dev); > + int ret, tmp, ret_read = 0; int ret, tmp; int reg_read = 0; That is keep the cases that set a value on separate lines from ones that don't for readability. > + > + /* Configure MUX register with the requested channel */ Obvious from code - so drop the comment. > + ret = mcp356x_write(adc, MCP356X_MUX, channel->address, 1); > + if (ret) { > + dev_err(&indio_dev->dev, "Failed to configure MUX register\n"); > + return ret; > + } > + > + /* ADC Conversion starts by writing ADC_MODE[1:0] = 11 to CONFIG0[1:0] = */ > + ret = mcp356x_update(adc, MCP356X_CONFIG0, MCP356X_ADC_MODE_MASK, > + MCP356X_ADC_CONVERSION_MODE, 1); > + if (ret) { > + dev_err(&indio_dev->dev, > + "Failed to configure CONFIG0 register\n"); > + return ret; > + } > + > + /* > + * Check if the conversion is ready. If not, wait a little bit, and > + * in case of timeout exit with an error. > + */ > + > + ret = read_poll_timeout(mcp356x_read, ret_read, > + ret_read || !(tmp & MCP356X_DATA_READY_MASK), > + 1000, MCP356X_DATA_READY_TIMEOUT_MS * 1000, true, > + adc, MCP356X_IRQ, &tmp, 1); > + > + /* failed to read status register */ > + if (ret_read) > + return ret; > + > + if (ret) > + return -ETIMEDOUT; > + > + if (tmp & MCP356X_DATA_READY_MASK) > + /* failing to finish conversion */ > + return -EBUSY; > + > + ret = mcp356x_read(adc, MCP356X_ADCDATA, &tmp, 4); > + if (ret) > + return ret; > + > + *val = tmp; > + > + return ret; > +} > + > +static int mcp356x_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, > + const int **vals, int *type, > + int *length, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + *type = IIO_VAL_INT; > + *vals = mcp356x_oversampling_avail; > + *length = ARRAY_SIZE(mcp356x_oversampling_avail); > + return IIO_AVAIL_LIST; > + case IIO_CHAN_INFO_HARDWAREGAIN: > + *type = IIO_VAL_FRACTIONAL; > + *length = ARRAY_SIZE(mcp356x_hwgain_frac); > + *vals = mcp356x_hwgain_frac; > + return IIO_AVAIL_LIST; > + case IIO_CHAN_INFO_CALIBBIAS: > + *vals = mcp356x_calib_bias; > + *type = IIO_VAL_INT; > + return IIO_AVAIL_RANGE; > + case IIO_CHAN_INFO_CALIBSCALE: > + *vals = mcp356x_calib_scale; > + *type = IIO_VAL_INT; > + return IIO_AVAIL_RANGE; > + default: > + return -EINVAL; > + } > +} > + > +static int mcp356x_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, > + int *val, int *val2, long mask) > +{ > + struct mcp356x_state *adc = iio_priv(indio_dev); > + int ret; > + > + mutex_lock(&adc->lock); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = mcp356x_read_single_value(indio_dev, channel, val); > + if (ret) > + ret = -EINVAL; > + else > + ret = IIO_VAL_INT; > + break; > + case IIO_CHAN_INFO_SCALE: > + *val = adc->vref_mv; > + *val2 = channel->scan_type.realbits - 1; > + ret = IIO_VAL_FRACTIONAL_LOG2; > + break; > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + *val = mcp356x_oversampling_avail[adc->oversampling]; > + ret = IIO_VAL_INT; > + break; > + case IIO_CHAN_INFO_HARDWAREGAIN: > + *val = mcp356x_hwgain_frac[2 * adc->hwgain]; > + *val2 = mcp356x_hwgain_frac[(2 * adc->hwgain) + 1]; > + ret = IIO_VAL_FRACTIONAL; > + break; > + case IIO_CHAN_INFO_CALIBBIAS: > + *val = adc->calib_bias; > + ret = IIO_VAL_INT; > + break; > + case IIO_CHAN_INFO_CALIBSCALE: > + *val = adc->calib_scale; > + ret = IIO_VAL_INT; > + break; > + default: > + ret = -EINVAL; > + } > + > + mutex_unlock(&adc->lock); > + > + return ret; > +} > + > +static int mcp356x_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int val, > + int val2, long mask) > +{ > + struct mcp356x_state *adc = iio_priv(indio_dev); > + int tmp; > + int ret = -EINVAL; > + > + mutex_lock(&adc->lock); > + switch (mask) { > + case IIO_CHAN_INFO_CALIBBIAS: > + if (val < mcp356x_calib_bias[0] && val > mcp356x_calib_bias[2]) > + goto out; > + > + adc->calib_bias = val; > + ret = mcp356x_write(adc, MCP356X_OFFSETCAL, val, 3); > + break; > + case IIO_CHAN_INFO_CALIBSCALE: > + if (val < mcp356x_calib_bias[0] && val > mcp356x_calib_bias[2]) > + goto out; > + > + adc->calib_scale = val; > + ret = mcp356x_write(adc, MCP356X_GAINCAL, val, 3); > + break; > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + if (val < 0) > + goto out; > + > + adc->oversampling = find_closest(val, mcp356x_oversampling_avail, > + ARRAY_SIZE(mcp356x_oversampling_avail)); > + > + dev_dbg(&adc->spi->dev, > + "IIO_CHAN_INFO_OVERSAMPLING_RATIO index %d\n", > + adc->oversampling); > + > + ret = mcp356x_update(adc, MCP356X_CONFIG1, MCP356X_OVERSAMPLING_RATIO_MASK, > + (adc->oversampling << MCP356X_OVERSAMPLING_RATIO_SHIFT), FIELD_PREP() and no need to define the shift > + 1); > + if (ret) > + dev_err(&indio_dev->dev, > + "Failed to configure CONFIG1 register\n"); > + > + break; > + case IIO_CHAN_INFO_HARDWAREGAIN: As per comments above, I'd imagine that the raw value needs to be multiplied by a factor related to the gain. Thus control this with _SCALE not _HARDWAREGAIN. > + /* > + * calculate gain from read values. > + * avoid using fractional numbers so > + * multiply the value with 1000. In case of x1/3 gain > + * the tmp will be 300 > + */ > + tmp = ((val * 1000000) + val2) / 1000; > + if (tmp < 1 || tmp > MAX_HWGAIN) > + goto out; > + > + adc->hwgain = find_closest(tmp, mcp356x_hwgain, > + ARRAY_SIZE(mcp356x_hwgain)); > + > + dev_dbg(&adc->spi->dev, > + "IIO_CHAN_INFO_HARDWAREGAIN Gain:%d; index %d\n", > + tmp, adc->hwgain); > + > + /* Update GAIN in CONFIG2[5:3] -> GAIN[2:0]*/ > + ret = mcp356x_update(adc, MCP356X_CONFIG2, MCP356X_HARDWARE_GAIN_MASK, > + (adc->hwgain << MCP356X_HARDWARE_GAIN_SHIFT), 1); FIELD_PREP() > + if (ret) > + dev_err(&indio_dev->dev, > + "Failed to configure CONFIG0 register\n"); > + break; > + } > + > +out: > + mutex_unlock(&adc->lock); > + > + return ret; > +} > + > +static int mcp356x_config(struct mcp356x_state *adc) > +{ > + int ret = 0; > + unsigned int tmp; > + > + dev_dbg(&adc->spi->dev, "%s: Start config...\n", __func__); A lot of debug prints. They are only wort keeping in a production driver if you can't get the same info from ftrace and similar. This one you can, so dro it. > + > + /* > + * The address is set on a per-device basis by fuses in the factory, > + * configured on request. If not requested, the fuses are set for 0x1. > + * The device address is part of the device markings to avoid > + * potential confusion. This address is coded on two bits, so four possible > + * addresses are available when multiple devices are present on the same > + * SPI bus with only one Chip Select line for all devices. > + */ > + device_property_read_u32(&adc->spi->dev, "microchip,hw-device-address", &tmp); > + > + if (tmp > 3) { > + dev_err_probe(&adc->spi->dev, tmp, > + "invalid device address. Must be in range 0-3.\n"); > + return -EINVAL; > + } > + > + adc->dev_addr = 0xff & tmp; > + > + dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr); > + > + ret = mcp356x_read(adc, MCP356X_RESERVED_E, &tmp, 2); > + if (ret < 0) return ret; Then no need for if (ret == 0) > + if (ret == 0) { > + switch (tmp & MCP356X_HW_ID_MASK) { > + case MCP3461_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3461 chip\n"); If detectable, then you don't need (or indeed want) to have the device specific data selected from the dt compatible. Select it based on what is actually present. However, you can warn about a missmatch if you like. > + break; > + case MCP3462_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3462 chip\n"); > + break; > + case MCP3464_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3464 chip\n"); > + break; > + case MCP3561_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3561 chip\n"); > + break; > + case MCP3562_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3562 chip\n"); > + break; > + case MCP3564_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3564 chip\n"); > + break; > + default: > + dev_err_probe(&adc->spi->dev, tmp, > + "Unknown chip found\n"); > + return -EINVAL; > + } > + } else { > + return ret; > + } > + > + /* Command sequence that ensures a recovery with Multi line comment syntax wrong (see below). > + * the desired settings in any cases of loss-of-power scenario. > + */ > + > + /* Write LOCK register to 0xA5 (Write Access Password) > + * Write access is allowed on the full register map. > + */ > + ret = mcp356x_write(adc, MCP356X_LOCK, 0x000000A5, 1); > + if (ret) > + return ret; > + > + /* Write IRQ register to 0x03 */ Two comments next to each other need combining into a single multi line comment. > + /* IRQ --> IRQ Mode = Hi-Z IRQ Output --> (0b00000011). Feels like this should be controlled from device tree as the pull up may not be present on some boards. Why set this, then reset device? > + * IRQ = 0x00000003 > + */ > + ret = mcp356x_write(adc, MCP356X_IRQ, 0x00000003, 1); > + if (ret) > + return ret; > + > + /* Device Full Reset Fast Command */ Naming is self explanatory so drop the comment. > + ret = mcp356x_fast_cmd(adc, MCP356X_FULL_RESET_CMD); > + Check ret > + /* wait 1ms for the chip to restart after a full reset */ Another obvious comment that isn't needed. > + mdelay(1); > + > + /* Reconfigure the ADC chip */ Also not needed. > + > + /* GAINCAL --> Disabled. > + * Default value is GAINCAL = 0x00800000; which provides a gain of 1x > + */ Use a define to make the magic value 0x00800000 meaning obvious. I assume it's some field of a larger register? If so FIELD_PREP() appropriate mask etc. > + ret = mcp356x_write(adc, MCP356X_GAINCAL, 0x00800000, 3); > + if (ret) > + return ret; > + > + adc->calib_scale = 0x00800000; Again, use a FIELD_PREP() to set it (bits 23-8 I think). > + > + /* OFFSETCAL --> 0 Counts of Offset Cancellation > + * (Measured offset is negative). > + * OFFSETCAL = 0x0 Value obvious from code (or should be anyway). > + */ > + ret = mcp356x_write(adc, MCP356X_OFFSETCAL, 0x00000000, 3); FIELD_PREP and appropriately defined mask. > + if (ret) > + return ret; > + > + /* TIMER --> Disabled. > + * TIMER = 0x00000000 > + */ > + ret = mcp356x_write(adc, MCP356X_TIMER, 0x00000000, 3); > + if (ret) > + return ret; > + > + /* SCAN --> Disabled. > + * SCAN = 0x00000000 > + */ > + ret = mcp356x_write(adc, MCP356X_SCAN, 0x00000000, 3); A bunch of different things in here. I'd expect to see this built from all the different fields you aren't enabling so it's easy to see the state. > + if (ret) > + return ret; > + > + /* MUX --> VIN+ = CH0, VIN- = CH1 --> (0b00000001). > + * MUX = 0x00000001 Defines and field masks should make this obvious in the code - thus not needing the comment. Same for the similar register writes that follow. There should need to be almost no comments in here because the code should make it obvious what is being set and to what value. > + */ > + ret = mcp356x_write(adc, MCP356X_MUX, 0x00000001, 1); > + if (ret) > + return ret; > + > + /* IRQ --> IRQ Mode = Hi-Z IRQ Output --> (0b00000011). > + * IRQ = 0x00000003 > + */ > + ret = mcp356x_write(adc, MCP356X_IRQ, 0x00000003, 1); > + if (ret) > + return ret; > + > + /* CONFIG3 > + * Conv. Mod = One-Shot/Standby, > + * FORMAT = 32-bit (right justified data): SGN extension + ADC data, > + * CRC_FORMAT = 16b, CRC-COM = Disabled, > + * OFFSETCAL = Enabled, GAINCAL = Enabled --> (10100011). > + * CONFIG3 = 0x000000A3 > + * > + */ > + ret = mcp356x_write(adc, MCP356X_CONFIG3, 0x000000A3, 1); > + if (ret) > + return ret; > + > + /* CONFIG2 --> BOOST = 1x, GAIN = 1x, AZ_MUX = 1 --> (0b10001101). > + * CONFIG2 = 0x0000008D > + */ > + ret = mcp356x_write(adc, MCP356X_CONFIG2, 0x0000008D, 1); > + if (ret) > + return ret; > + > + adc->hwgain = 0x01; > + adc->auto_zeroing_mux = true; > + adc->auto_zeroing_ref = false; > + adc->current_boost_mode = MCP356X_BOOST_CURRENT_x1_00; > + > + /* CONFIG1 --> AMCLK = MCLK, OSR = 98304 --> (0b00111100). > + * CONFIG1 = 0x0000003C > + */ > + ret = mcp356x_write(adc, MCP356X_CONFIG1, 0x0000003C, 1); > + if (ret) > + return ret; > + > + adc->oversampling = 0x0F; > + > + if (!adc->vref) { > + /* CONFIG0 --> VREF_SEL = Internal Voltage Reference 2.4v > + * CLK_SEL = INTOSC w/o CLKOUT, CS_SEL = No Bias, > + * ADC_MODE = Standby Mode --> (0b11100010). > + * CONFIG0 = 0x000000E2 > + */ > + ret = mcp356x_write(adc, MCP356X_CONFIG0, 0x000000E2, 1); Build the value into a local variable, then have the write after the if/else using that variable. > + > + dev_dbg(&adc->spi->dev, "%s: Using internal Vref\n", > + __func__); > + adc->vref_mv = MCP356XR_INT_VREF_MV; > + > + } else { > + /* CONFIG0 --> CLK_SEL = INTOSC w/o CLKOUT, CS_SEL = No Bias, > + * ADC_MODE = Standby Mode --> (0b01100010). > + * CONFIG0 = 0x000000E2 > + */ > + ret = mcp356x_write(adc, MCP356X_CONFIG0, 0x00000062, 1); > + } > + adc->current_bias_mode = MCP356X_CS_SEL_0_0_uA; > + > + return ret; > +} > + > +static int mcp356x_probe(struct spi_device *spi) > +{ > + int ret, device_index; > + struct iio_dev *indio_dev; > + struct mcp356x_state *adc; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); > + if (!indio_dev) { > + dev_err_probe(&indio_dev->dev, PTR_ERR(indio_dev), > + "Can't allocate iio device\n"); > + return -ENOMEM; > + } > + > + adc = iio_priv(indio_dev); > + adc->spi = spi; > + > + dev_dbg(&adc->spi->dev, "%s: probe(spi = 0x%p)\n", __func__, spi); > + > + adc->vref = devm_regulator_get_optional(&adc->spi->dev, "vref"); Is it always optional? If it's only for some parts, then enforce that by first using spi_get_device_match_data() to get appropriate pointer to a chip type specific structure, then have a flag in there that says if is optional or not. Adjust how it is retrieved to take that into account. > + if (IS_ERR(adc->vref)) { > + if (PTR_ERR(adc->vref) == -ENODEV) { > + adc->vref = NULL; > + dev_dbg(&adc->spi->dev, "%s: Using internal Vref\n", > + __func__); In binding docs you mention that not all devices have an internal reference. If the device doesn't have one we need to error out here. > + } else { > + dev_err_probe(&adc->spi->dev, PTR_ERR(adc->vref), > + "failed to get regulator\n"); > + return PTR_ERR(adc->vref); > + } > + } else { > + ret = regulator_enable(adc->vref); > + if (ret) > + return ret; Add a devm_add_action_or_reset() fall here with appropriate callback to turn off the regulator. > + > + dev_dbg(&adc->spi->dev, "%s: Using External Vref\n", > + __func__); > + > + ret = regulator_get_voltage(adc->vref); > + if (ret < 0) { > + dev_err_probe(&adc->spi->dev, ret, > + "Failed to read vref regulator\n"); > + goto error_disable_reg; > + } > + > + adc->vref_mv = ret / 1000; > + } > + > + spi_set_drvdata(spi, indio_dev); This probably won't be needed after you take the devm_add_action_or_reset() route to getting rid of remove() as suggested below. > + device_index = spi_get_device_id(spi)->driver_data; spi_get_device_match_data() and add the data as pointers to where you currently have the enum values + to the spi_device_id table. This avoids need to be careful that entries align perfectly between those two tables. However you show above that the chip type is detectable. Better to just detect it than rely on firmware telling you which one you have. > + adc->chip_info = &mcp356x_chip_infos_tbl[device_index]; > + > + adc->mcp356x_info.read_raw = mcp356x_read_raw; > + adc->mcp356x_info.write_raw = mcp356x_write_raw; > + adc->mcp356x_info.read_avail = mcp356x_read_avail; > + > + ret = mcp356x_prep_custom_attributes(adc, indio_dev); > + if (ret) { > + dev_err_probe(&adc->spi->dev, ret, > + "Can't configure custom attributes for MCP356X device\n"); > + goto error_disable_reg; > + } > + > + indio_dev->name = spi_get_device_id(spi)->name; This requires careful matching between the entries in the of and spi ID tables. I'd rather the name was stored in part specific data so your chip_infos_tbl[] entries. > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &adc->mcp356x_info; > + > + indio_dev->channels = adc->chip_info->channels; > + indio_dev->num_channels = adc->chip_info->num_channels; > + indio_dev->masklength = adc->chip_info->num_channels - 1; You should not be setting that directly. Leave it to the IIO core. > + > + /* initialize the chip access mutex */ Don't have any comments that state the obvious. The mutex scope is defined above and doesn't need repeating here. > + mutex_init(&adc->lock); > + > + /* Do any chip specific initialization, e.g: IIO multi-line comments are /* * Do any ... > + * read/write some registers > + * enable/disable certain channels > + * change the sampling rate to the requested value > + */ > + ret = mcp356x_config(adc); > + if (ret) { > + dev_err_probe(&adc->spi->dev, ret, > + "Can't configure MCP356X device\n"); > + goto error_disable_reg; > + } > + > + dev_dbg(&adc->spi->dev, "%s: Vref (mV): %d\n", __func__, adc->vref_mv); > + > + ret = devm_iio_device_register(&spi->dev, indio_dev); > + if (ret) { > + dev_err_probe(&adc->spi->dev, ret, > + "Can't register IIO device\n"); > + goto error_disable_reg; > + } > + > + return 0; > + > +error_disable_reg: > + if (adc->vref) Use devm_add_action_or_reset() and this goes away as all error caused unwinding automatically handled. > + regulator_disable(adc->vref); > + > + return ret; > +} > + > +static void mcp356x_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct mcp356x_state *adc = iio_priv(indio_dev); > + > + if (adc->vref) > + regulator_disable(adc->vref); Use a devm_add_action_or_reset() in probe() to ensure this is turned off at the right point in the right point in the sequence. Right now you turn it off before the userspace interfaces are removed leaving a race where a reading might be taken despite the reference being off. > +} > + > +static const struct of_device_id mcp356x_dt_ids[] = { > + { .compatible = "microchip,mcp3461" }, > + { .compatible = "microchip,mcp3462" }, > + { .compatible = "microchip,mcp3464" }, > + { .compatible = "microchip,mcp3461r" }, > + { .compatible = "microchip,mcp3462r" }, > + { .compatible = "microchip,mcp3464r" }, > + { .compatible = "microchip,mcp3561" }, > + { .compatible = "microchip,mcp3562" }, > + { .compatible = "microchip,mcp3564" }, > + { .compatible = "microchip,mcp3561r" }, > + { .compatible = "microchip,mcp3562r" }, > + { .compatible = "microchip,mcp3564r" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, mcp356x_dt_ids); > + > +static const struct spi_device_id mcp356x_id[] = { > + { "mcp3461", mcp3461 }, Much prefer the data here to both be replicated in the above of_device_id table and to be a pointer to a device type specific structure with information about what a particular device supports. > + { "mcp3462", mcp3462 }, > + { "mcp3464", mcp3464 }, > + { "mcp3461r", mcp3461r }, > + { "mcp3462r", mcp3462r }, > + { "mcp3464r", mcp3464r }, > + { "mcp3561", mcp3561 }, > + { "mcp3562", mcp3562 }, > + { "mcp3564", mcp3564 }, > + { "mcp3561r", mcp3561r }, > + { "mcp3562r", mcp3562r }, > + { "mcp3564r", mcp3564r }, > + { } > +}; > +MODULE_DEVICE_TABLE(spi, mcp356x_id); > + > +static struct spi_driver mcp356x_driver = { > + .driver = { > + .name = "mcp3564", > + .of_match_table = mcp356x_dt_ids, > + }, > + .probe = mcp356x_probe, > + .remove = mcp356x_remove, > + .id_table = mcp356x_id, > +}; > + > +module_spi_driver(mcp356x_driver); > + > +MODULE_AUTHOR("Marius Cristea <marius.cristea@microchip.com>"); > +MODULE_DESCRIPTION("Microchip MCP346x/MCP346xR and MCP356x/MCP346xR ADCs"); > +MODULE_LICENSE("GPL v2"); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC 2023-05-20 15:15 ` Jonathan Cameron @ 2023-06-22 11:32 ` Marius.Cristea 2023-07-02 10:28 ` Jonathan Cameron 0 siblings, 1 reply; 16+ messages in thread From: Marius.Cristea @ 2023-06-22 11:32 UTC (permalink / raw) To: jic23; +Cc: devicetree, lars, linux-iio, robh+dt, linux-kernel Hi Jonathan, Please see my comments bellow. Thanks, Marius On Sat, 2023-05-20 at 16:15 +0100, Jonathan Cameron wrote: > > + > > +What: > > /sys/bus/iio/devices/iio:deviceX/calibscale_available > > +KernelVersion: 6.4 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + Reading returns a range with the possible values for > > the gain > > + error digital calibration register. > > + > > +What: > > /sys/bus/iio/devices/iio:deviceX/boost_current > > +KernelVersion: 6.4 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + This attribute is used to set the biasing circuit of > > the > > + Delta-Sigma modulator. The different BOOST settings > > are applied > > + to the entire modulator circuit, including the > > voltage reference > > + buffers. > > + > > +What: > > /sys/bus/iio/devices/iio:deviceX/boost_current_available > > +KernelVersion: 6.4 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + Reading returns a list with the possible values for > > + the current biasing circuit of the Delta-Sigma > > modulator. > > + > > + "x0.5", - ADC channel has current x 0.5 > > Keep just numbers in the attr. It should be named in a fashion that > makes it apparent that it's a multiplier, not an absolute current. > > New ABI like this is best avoided if we can. I see from a quick > glance at the > datasheet that there is advice on controlling this to allow for > different > clock settings, but I'm not sure if that's a simple relationship or > not. > From > https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4-Two-Four-Eight-Channel-153.6-ksps-Low-Noise-16-Bit-Delta-Sigma-ADC-Data-Sheet-20006180D.pdf > figures 2-20 onwards, it looks like this effectively trades off power > consumption > against max frequency, so maybe we could set it automatically? > Here being a trade off between power consumption and bandwidth I would like it to be somehow programmable and let the user to set it. Maybe the user wants to "monitor" a channel (have low power consumption) and other channels to be benefit from a larger bandwidth. It don't think it can be set automatically. > > > + > > + "x0.66", - ADC channel has current x 0.66 > > + > > + "x1", - ADC channel has current x 1 (default) > > + > > + "x2" - ADC channel has current x 2 > > + > > +What: /sys/bus/iio/devices/iio:deviceX/current_bias > > Another bit of custom ABI with the normal problems of it being > unknown > to standard userspace software. You could perhaps use an output > channel for > this and just treat it like a current DAC, with a label that makes > it's relationship > to the inputs obvious. I could use an output channel to setup the current. Is it OK to have both ADC and "DAC" functionality in "iio/adc"? > > > +KernelVersion: 6.4 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + This attribute is used to set the current Source/Sink > > + values for sensor bias. The ADC inputs, VIN-/VIN+, > > feature a > > + selectable burnout current source, which enables open > > or > > + short-circuit detection, as well as biasing very low- > > current > > + external sensors. The bias current is sourced on the > > VIN+ pin of > > + the ADC (noninverting output of the analog > > multiplexer) > > + and sunk on the VIN- pin of the ADC (inverting output > > of > > + the analog multiplexer). Since the same current flows > > + at the VIN-/VIN+ pins of the ADC, it can sense the > > + impedance of an externally connected sensor that > > + would be connected between the selected inputs of the > > + multiplexer. > > + > > +What: > > /sys/bus/iio/devices/iio:deviceX/current_bias_available > > +KernelVersion: 6.4 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + Reading returns a list with the possible values for > > + the current source/sink selection values for sensor > > bias. > > + > > + "15_uA" - 15 uA is applied to the ADC inputs. > Match to standard IIO units for a current then drop the postfix. > > + > > + "3.7_uA" - 3.7 uA is applied to the ADC inputs. > > + > > + "0.9_uA" - 0.9 uA is applied to the ADC inputs. > > + > > + "no_current" - "No current source is applied to the > > ADC > > + inputs (default)" > > 0 would seem self explanatory for this an is easier for userspace > software > to deal with. Also, this doc should not include the values, merely > that > there is a list. > > > > + > > +What: > > /sys/bus/iio/devices/iio:deviceX/enable_auto_zeroing_mux > > +KernelVersion: 6.4 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + This attribute is used to enable the analog input > > multiplexer > > + auto-zeroing algorithm. Write '1' to enable it, write > > '0' to > > + disable it. > If you can explain what auto zeroing is that would be great. It's not > something > I recall seeing in other drivers. I will add this description: "This attribute is used to enable the chopping algorithm for the internal voltage reference buffer. This setting has no effect when external voltage reference is selected. Internal voltage reference buffer injects a certain quantity of 1/f noise into the system that can be modulated with the incoming input signals and can limit the SNR performance at higher Oversampling Ratio values (over 256). To overcome this limitation, the buffer includes an auto-zeroing algorithm that greatly reduces (cancels out) the 1/f noise and cancels the offset value of the reference buffer. As a result, the SNR of the system is not affected by this 1/f noise component of the reference buffer, even at maximum oversampling ratio values. Write '1' to enable it, write '0' to disable it." > > + > > +What: > > /sys/bus/iio/devices/iio:deviceX/enable_auto_zeroing_ref > > +KernelVersion: 6.4 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + This attribute is used to enable the chopping > > algorithm for the > > + internal voltage reference buffer. Write '1' to > > enable it, > > + write '0' to disable it. > > + > > +What: /sys/bus/iio/devices/iio:deviceX/hardwaregain > > +KernelVersion: 6.4 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + This attribute is used to set the hardware applied > > gain factor. > > + It is shared across all channels. > This attr is pretty much only used when the gain is not directly > related to the > value being used. An example being a time of flight sensor where the > amplitude > of the measured signal doesn't directly matter as we are looking for > the timing of the > peak, we just need it to be big enough to measure. Otherwise scale is > what you want. > The MCP3564 has internally an input amplifier with programmable gain. Because we can measure a difference between 2 channels, we have clients that measure the output of load cells. This output is quite small am we need to amplify it before conversion is done. I was thinking that because we name it "hardwaregain" this will be be somehow connected to a configurable gain that could be set in hardware. Is better to have a separate info strictly related to hardware amplification. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC 2023-06-22 11:32 ` Marius.Cristea @ 2023-07-02 10:28 ` Jonathan Cameron 0 siblings, 0 replies; 16+ messages in thread From: Jonathan Cameron @ 2023-07-02 10:28 UTC (permalink / raw) To: Marius.Cristea; +Cc: jic23, devicetree, lars, linux-iio, robh+dt, linux-kernel On Thu, 22 Jun 2023 11:32:11 +0000 <Marius.Cristea@microchip.com> wrote: > Hi Jonathan, > > Please see my comments bellow. > > Thanks, > Marius > > On Sat, 2023-05-20 at 16:15 +0100, Jonathan Cameron wrote: > > > + > > > +What: > > > /sys/bus/iio/devices/iio:deviceX/calibscale_available > > > +KernelVersion: 6.4 > > > +Contact: linux-iio@vger.kernel.org > > > +Description: > > > + Reading returns a range with the possible values for > > > the gain > > > + error digital calibration register. > > > + > > > +What: > > > /sys/bus/iio/devices/iio:deviceX/boost_current > > > +KernelVersion: 6.4 > > > +Contact: linux-iio@vger.kernel.org > > > +Description: > > > + This attribute is used to set the biasing circuit of > > > the > > > + Delta-Sigma modulator. The different BOOST settings > > > are applied > > > + to the entire modulator circuit, including the > > > voltage reference > > > + buffers. > > > + > > > +What: > > > /sys/bus/iio/devices/iio:deviceX/boost_current_available > > > +KernelVersion: 6.4 > > > +Contact: linux-iio@vger.kernel.org > > > +Description: > > > + Reading returns a list with the possible values for > > > + the current biasing circuit of the Delta-Sigma > > > modulator. > > > + > > > + "x0.5", - ADC channel has current x 0.5 > > > > Keep just numbers in the attr. It should be named in a fashion that > > makes it apparent that it's a multiplier, not an absolute current. > > > > New ABI like this is best avoided if we can. I see from a quick > > glance at the > > datasheet that there is advice on controlling this to allow for > > different > > clock settings, but I'm not sure if that's a simple relationship or > > not. > > From > > https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4-Two-Four-Eight-Channel-153.6-ksps-Low-Noise-16-Bit-Delta-Sigma-ADC-Data-Sheet-20006180D.pdf > > figures 2-20 onwards, it looks like this effectively trades off power > > consumption > > against max frequency, so maybe we could set it automatically? > > > > Here being a trade off between power consumption and bandwidth I would > like it to be somehow programmable and let the user to set it. Maybe > the user wants to "monitor" a channel (have low power consumption) and > other channels to be benefit from a larger bandwidth. It don't think it > can be set automatically. Hmm. Whilst it's certainly possible a user wants to do this sort of mix the trade off against extra complexity of the interface (and hence the chance that anyone actually uses it being reduced, makes me wonder if it is worth while). If you just configured this for all channels then could be done automatically from sampling_frequency control which is standard ABI. I don't really mind something custom if necessary though, but I find it very unlikely that this particular interface generalizes long term as it's a multiplier of 'something magic' - if we could make these actual currents that would be better but I have no idea if we can. > > > > > > + > > > + "x0.66", - ADC channel has current x 0.66 > > > + > > > + "x1", - ADC channel has current x 1 (default) > > > + > > > + "x2" - ADC channel has current x 2 > > > + > > > +What: /sys/bus/iio/devices/iio:deviceX/current_bias > > > > Another bit of custom ABI with the normal problems of it being > > unknown > > to standard userspace software. You could perhaps use an output > > channel for > > this and just treat it like a current DAC, with a label that makes > > it's relationship > > to the inputs obvious. > > > I could use an output channel to setup the current. Is it OK to have > both ADC and "DAC" functionality in "iio/adc"? Yes. That's fine if it's primarily an ADC - I think we have some existing examples (definitely do in other classes of sensor, but maybe not ADCs). > > > > + > > > +What: > > > /sys/bus/iio/devices/iio:deviceX/enable_auto_zeroing_mux > > > +KernelVersion: 6.4 > > > +Contact: linux-iio@vger.kernel.org > > > +Description: > > > + This attribute is used to enable the analog input > > > multiplexer > > > + auto-zeroing algorithm. Write '1' to enable it, write > > > '0' to > > > + disable it. > > If you can explain what auto zeroing is that would be great. It's not > > something > > I recall seeing in other drivers. > > I will add this description: > "This attribute is used to enable the chopping algorithm for the > internal voltage reference buffer. This setting has no effect > when external voltage reference is selected. Don't present it when that's the case an no need to talk about it in this description as a result. > Internal voltage reference buffer injects a certain quantity of > 1/f noise into the system that can be modulated with the > incoming input signals and can limit the SNR performance at > higher Oversampling Ratio values (over 256). To overcome this > limitation, the buffer includes an auto-zeroing algorithm that > greatly reduces (cancels out) the 1/f noise and cancels the > offset value of the reference buffer. As a result, the SNR of > the system is not affected by this 1/f noise component of the > reference buffer, even at maximum oversampling ratio values. > Write '1' to enable it, write '0' to disable it." Indeed a new one to me. We've had devices that modulate the input a little in the past. Dumb question though - why wouldn't you turn this on? Do we need to provide the option. > > > +What: > > > /sys/bus/iio/devices/iio:deviceX/enable_auto_zeroing_ref > > > +KernelVersion: 6.4 > > > +Contact: linux-iio@vger.kernel.org > > > +Description: > > > + This attribute is used to enable the chopping > > > algorithm for the > > > + internal voltage reference buffer. Write '1' to > > > enable it, > > > + write '0' to disable it. > > > + > > > +What: /sys/bus/iio/devices/iio:deviceX/hardwaregain > > > +KernelVersion: 6.4 > > > +Contact: linux-iio@vger.kernel.org > > > +Description: > > > + This attribute is used to set the hardware applied > > > gain factor. > > > + It is shared across all channels. > > This attr is pretty much only used when the gain is not directly > > related to the > > value being used. An example being a time of flight sensor where the > > amplitude > > of the measured signal doesn't directly matter as we are looking for > > the timing of the > > peak, we just need it to be big enough to measure. Otherwise scale is > > what you want. > > > > The MCP3564 has internally an input amplifier with programmable gain. > Because we can measure a difference between 2 channels, we have clients > that measure the output of load cells. This output is quite small am we > need to amplify it before conversion is done. > > I was thinking that because we name it "hardwaregain" this will be be > somehow connected to a configurable gain that could be set in hardware. > Is better to have a separate info strictly related to hardware > amplification. Not an interface that any standard software will control. If you can either treat it as _scale (which implies that it is relevant to the _raw calculation) or as _calibscale (which implies that it is a tweak to correct for device differences or external circuitry but should not be applied by software to the _raw value) then you will have a control that is the same as being used for any other device. Many ADCs have programmable gain - we deliberately expose that only via _scale because userspace doesn't care 'how' a gain results, just what it is. Jonathan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC 2023-05-19 16:01 ` [PATCH 2/2] iio: adc: adding support for " marius.cristea 2023-05-20 15:15 ` Jonathan Cameron @ 2023-05-24 11:05 ` Mike Looijmans 2023-05-28 19:20 ` Jonathan Cameron 1 sibling, 1 reply; 16+ messages in thread From: Mike Looijmans @ 2023-05-24 11:05 UTC (permalink / raw) To: marius.cristea, jic23, lars, robh+dt; +Cc: linux-iio, devicetree, linux-kernel On 19-05-2023 18:01, marius.cristea@microchip.com wrote: > From: Marius Cristea <marius.cristea@microchip.com> > > This is the iio driver for Microchip family of 153.6 ksps, > Low-Noise 16/24-Bit Delta-Sigma ADCs with an SPI interface. Oh nice.. I tried to submit a similar driver just yesterday, I'll comment on yours instead... > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > --- ... > + > +static inline u8 mcp356x_reg_write(u8 chip_addr, u8 reg) > +{ > + return ((chip_addr << 6) | (reg << 2) | MCP356X_WRT_CTRL); > +} > + > +static inline u8 mcp356x_reg_read(u8 chip_addr, u8 reg) > +{ > + return ((chip_addr << 6) | (reg << 2) | MCP356X_RD_CTRL); > +} How about using FIELD_PREP() here? > + > +static inline u8 mcp356x_reg_fast_cmd(u8 chip_addr, u8 cmd) > +{ > + return ((chip_addr << 6) | (cmd << 2)); > +} > + > +static int mcp356x_read(struct mcp356x_state *adc, u8 reg, u32 *val, u8 len) > +{ > + int ret; > + u8 tmp_reg; > + > + tmp_reg = mcp356x_reg_read(adc->dev_addr, reg); > + > + ret = spi_write_then_read(adc->spi, &tmp_reg, 1, val, len); I had issues with spi_write_then_read not communicating properly with the device. This may be due to the SPI controller (Xilinx IP in FPGA) though. I had to use spi_sync() to get reliable answers. Also has the added benefit of giving access to the interrupt flags. > + > + be32_to_cpus(val); > + *val >>= ((4 - len) * 8); > + > + return ret; > +} > + > +static int mcp356x_write(struct mcp356x_state *adc, u8 reg, u32 val, u8 len) > +{ > + val |= (mcp356x_reg_write(adc->dev_addr, reg) << (len * 8)); > + val <<= (3 - len) * 8; > + cpu_to_be32s(&val); > + > + return spi_write(adc->spi, &val, len + 1); > +} > + > +static int mcp356x_fast_cmd(struct mcp356x_state *adc, u8 fast_cmd) > +{ > + u8 val; > + > + val = mcp356x_reg_fast_cmd(adc->dev_addr, fast_cmd); > + > + return spi_write(adc->spi, &val, 1); > +} > + > +static int mcp356x_update(struct mcp356x_state *adc, u8 reg, u32 mask, u32 val, > + u8 len) > +{ > + u32 tmp; > + int ret; > + > + ret = mcp356x_read(adc, reg, &tmp, len); > + > + if (ret == 0) { > + val &= mask; > + val |= tmp & ~mask; > + ret = mcp356x_write(adc, reg, val, len); > + } > + > + return ret; > +} This update function is used a lot, and is just a re-implementation of "regmap" functionality. Since this driver only reads/writes single registers, I would recommend using regmap with the read/write functions above. That would also reduce the SPI traffic as regmap can cache the values for you. ... > + > +#define MCP3564_CHANNELS(depth) { \ > + MCP356X_V_CHANNEL(0, 0, depth), \ > + MCP356X_V_CHANNEL(1, 1, depth), \ > + MCP356X_V_CHANNEL(2, 2, depth), \ > + MCP356X_V_CHANNEL(3, 3, depth), \ > + MCP356X_V_CHANNEL(4, 4, depth), \ > + MCP356X_V_CHANNEL(5, 5, depth), \ > + MCP356X_V_CHANNEL(6, 6, depth), \ > + MCP356X_V_CHANNEL(7, 7, depth), \ > + MCP356X_T_CHAN(depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 2, 0x02, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 3, 0x03, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 2, 0x12, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 3, 0x13, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 3, 0x23, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 0, 0x20, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 0, 0x30, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 1, 0x21, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 1, 0x31, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 2, 0x32, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 4, 0x04, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 5, 0x05, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 6, 0x06, depth), \ > + MCP356X_V_CHANNEL_DIFF(0, 7, 0x07, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 4, 0x14, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 5, 0x15, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 6, 0x16, depth), \ > + MCP356X_V_CHANNEL_DIFF(1, 7, 0x17, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 4, 0x24, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 5, 0x25, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 6, 0x26, depth), \ > + MCP356X_V_CHANNEL_DIFF(2, 7, 0x27, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 4, 0x34, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 5, 0x35, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 6, 0x36, depth), \ > + MCP356X_V_CHANNEL_DIFF(3, 7, 0x37, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 5, 0x45, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 6, 0x46, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 7, 0x47, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 6, 0x56, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 7, 0x57, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 7, 0x67, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 0, 0x40, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 0, 0x50, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 0, 0x60, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 0, 0x70, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 1, 0x41, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 1, 0x51, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 1, 0x61, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 1, 0x71, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 2, 0x42, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 2, 0x52, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 2, 0x62, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 2, 0x72, depth), \ > + MCP356X_V_CHANNEL_DIFF(4, 3, 0x43, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 3, 0x53, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 3, 0x63, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 3, 0x73, depth), \ > + MCP356X_V_CHANNEL_DIFF(5, 4, 0x54, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 4, 0x64, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 4, 0x74, depth), \ > + MCP356X_V_CHANNEL_DIFF(6, 5, 0x65, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 5, 0x75, depth), \ > + MCP356X_V_CHANNEL_DIFF(7, 6, 0x76, depth) \ > +} Nice that the chip can do full 8x8 mux delivering 56 channels, but I don't think that will be useful to many people. Also I'd want to see buffer support added to the device, which only works for the channels as in the table in the datasheet, so only 4 differential ones. It'd be annoying to have 56 channels but only be able to contiuously read 4 specific ones. ... > + > +static int mcp356x_read_single_value(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, > + int *val) > +{ > + struct mcp356x_state *adc = iio_priv(indio_dev); > + int ret, tmp, ret_read = 0; > + > + /* Configure MUX register with the requested channel */ > + ret = mcp356x_write(adc, MCP356X_MUX, channel->address, 1); > + if (ret) { > + dev_err(&indio_dev->dev, "Failed to configure MUX register\n"); > + return ret; > + } > + > + /* ADC Conversion starts by writing ADC_MODE[1:0] = 11 to CONFIG0[1:0] = */ > + ret = mcp356x_update(adc, MCP356X_CONFIG0, MCP356X_ADC_MODE_MASK, > + MCP356X_ADC_CONVERSION_MODE, 1); > + if (ret) { > + dev_err(&indio_dev->dev, > + "Failed to configure CONFIG0 register\n"); > + return ret; > + } > + > + /* > + * Check if the conversion is ready. If not, wait a little bit, and > + * in case of timeout exit with an error. > + */ > + > + ret = read_poll_timeout(mcp356x_read, ret_read, > + ret_read || !(tmp & MCP356X_DATA_READY_MASK), > + 1000, MCP356X_DATA_READY_TIMEOUT_MS * 1000, true, > + adc, MCP356X_IRQ, &tmp, 1); I have an implementation that (optionally) uses the interrupt signal from the chip, nicer than polling. > + > + /* failed to read status register */ > + if (ret_read) > + return ret; > + > + if (ret) > + return -ETIMEDOUT; > + > + if (tmp & MCP356X_DATA_READY_MASK) > + /* failing to finish conversion */ > + return -EBUSY; > + > + ret = mcp356x_read(adc, MCP356X_ADCDATA, &tmp, 4); > + if (ret) > + return ret; > + > + *val = tmp; > + > + return ret; > +} > + > +static int mcp356x_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, > + const int **vals, int *type, > + int *length, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + *type = IIO_VAL_INT; > + *vals = mcp356x_oversampling_avail; > + *length = ARRAY_SIZE(mcp356x_oversampling_avail); > + return IIO_AVAIL_LIST; > + case IIO_CHAN_INFO_HARDWAREGAIN: Hmm, last time I tried to submit a driver with HARDWAREGAIN I got told to change it to selectable SCALE instead. See the driver I submitted yesterday for details. > + *type = IIO_VAL_FRACTIONAL; > + *length = ARRAY_SIZE(mcp356x_hwgain_frac); > + *vals = mcp356x_hwgain_frac; > + return IIO_AVAIL_LIST; > + case IIO_CHAN_INFO_CALIBBIAS: > + *vals = mcp356x_calib_bias; > + *type = IIO_VAL_INT; > + return IIO_AVAIL_RANGE; > + case IIO_CHAN_INFO_CALIBSCALE: > + *vals = mcp356x_calib_scale; > + *type = IIO_VAL_INT; > + return IIO_AVAIL_RANGE; > + default: > + return -EINVAL; > + } > +} ... > + > +static int mcp356x_config(struct mcp356x_state *adc) > +{ > + int ret = 0; > + unsigned int tmp; > + > + dev_dbg(&adc->spi->dev, "%s: Start config...\n", __func__); > + > + /* > + * The address is set on a per-device basis by fuses in the factory, > + * configured on request. If not requested, the fuses are set for 0x1. > + * The device address is part of the device markings to avoid > + * potential confusion. This address is coded on two bits, so four possible > + * addresses are available when multiple devices are present on the same > + * SPI bus with only one Chip Select line for all devices. > + */ > + device_property_read_u32(&adc->spi->dev, "microchip,hw-device-address", &tmp); > tmp may be used uninitialized here. Initialize to "1" to get a proper default value. That also makes the microchip,hw-device-address optional instead of mandatory (see my dt-bindings comment). > + if (tmp > 3) { > + dev_err_probe(&adc->spi->dev, tmp, > + "invalid device address. Must be in range 0-3.\n"); > + return -EINVAL; > + } > + > + adc->dev_addr = 0xff & tmp; > + > + dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr); > + > + ret = mcp356x_read(adc, MCP356X_RESERVED_E, &tmp, 2); > + > + if (ret == 0) { > + switch (tmp & MCP356X_HW_ID_MASK) { > + case MCP3461_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3461 chip\n"); This will never get output unless the driver is compiled in debug mode. I'd have expected this ID to be used to initialize things? > + break; > + case MCP3462_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3462 chip\n"); > + break; > + case MCP3464_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3464 chip\n"); > + break; > + case MCP3561_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3561 chip\n"); > + break; > + case MCP3562_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3562 chip\n"); > + break; > + case MCP3564_HW_ID: > + dev_dbg(&adc->spi->dev, "Found MCP3564 chip\n"); > + break; > + default: > + dev_err_probe(&adc->spi->dev, tmp, > + "Unknown chip found\n"); > + return -EINVAL; > + } > + } else { > + return ret; > + } > + > + /* Command sequence that ensures a recovery with > + * the desired settings in any cases of loss-of-power scenario. > + */ > + > + /* Write LOCK register to 0xA5 (Write Access Password) > + * Write access is allowed on the full register map. > + */ > + ret = mcp356x_write(adc, MCP356X_LOCK, 0x000000A5, 1); > + if (ret) > + return ret; > + > + /* Write IRQ register to 0x03 */ > + /* IRQ --> IRQ Mode = Hi-Z IRQ Output --> (0b00000011). > + * IRQ = 0x00000003 > + */ > + ret = mcp356x_write(adc, MCP356X_IRQ, 0x00000003, 1); > + if (ret) > + return ret; > + > + /* Device Full Reset Fast Command */ > + ret = mcp356x_fast_cmd(adc, MCP356X_FULL_RESET_CMD); > + > + /* wait 1ms for the chip to restart after a full reset */ > + mdelay(1); > + > + /* Reconfigure the ADC chip */ > + > + /* GAINCAL --> Disabled. > + * Default value is GAINCAL = 0x00800000; which provides a gain of 1x > + */ > + ret = mcp356x_write(adc, MCP356X_GAINCAL, 0x00800000, 3); > + if (ret) > + return ret; > + > + adc->calib_scale = 0x00800000; > + > + /* OFFSETCAL --> 0 Counts of Offset Cancellation > + * (Measured offset is negative). > + * OFFSETCAL = 0x0 > + */ > + ret = mcp356x_write(adc, MCP356X_OFFSETCAL, 0x00000000, 3); > + if (ret) > + return ret; > + > + /* TIMER --> Disabled. > + * TIMER = 0x00000000 > + */ > + ret = mcp356x_write(adc, MCP356X_TIMER, 0x00000000, 3); > + if (ret) > + return ret; > + > + /* SCAN --> Disabled. > + * SCAN = 0x00000000 > + */ > + ret = mcp356x_write(adc, MCP356X_SCAN, 0x00000000, 3); > + if (ret) > + return ret; > + > + /* MUX --> VIN+ = CH0, VIN- = CH1 --> (0b00000001). > + * MUX = 0x00000001 > + */ > + ret = mcp356x_write(adc, MCP356X_MUX, 0x00000001, 1); > + if (ret) > + return ret; > + > + /* IRQ --> IRQ Mode = Hi-Z IRQ Output --> (0b00000011). > + * IRQ = 0x00000003 > + */ > + ret = mcp356x_write(adc, MCP356X_IRQ, 0x00000003, 1); > + if (ret) > + return ret; > + > + /* CONFIG3 > + * Conv. Mod = One-Shot/Standby, > + * FORMAT = 32-bit (right justified data): SGN extension + ADC data, > + * CRC_FORMAT = 16b, CRC-COM = Disabled, > + * OFFSETCAL = Enabled, GAINCAL = Enabled --> (10100011). > + * CONFIG3 = 0x000000A3 > + * > + */ > + ret = mcp356x_write(adc, MCP356X_CONFIG3, 0x000000A3, 1); > + if (ret) > + return ret; > + > + /* CONFIG2 --> BOOST = 1x, GAIN = 1x, AZ_MUX = 1 --> (0b10001101). > + * CONFIG2 = 0x0000008D > + */ > + ret = mcp356x_write(adc, MCP356X_CONFIG2, 0x0000008D, 1); > + if (ret) > + return ret; > + > + adc->hwgain = 0x01; > + adc->auto_zeroing_mux = true; > + adc->auto_zeroing_ref = false; > + adc->current_boost_mode = MCP356X_BOOST_CURRENT_x1_00; > + > + /* CONFIG1 --> AMCLK = MCLK, OSR = 98304 --> (0b00111100). > + * CONFIG1 = 0x0000003C > + */ > + ret = mcp356x_write(adc, MCP356X_CONFIG1, 0x0000003C, 1); > + if (ret) > + return ret; > + > + adc->oversampling = 0x0F; > + > + if (!adc->vref) { > + /* CONFIG0 --> VREF_SEL = Internal Voltage Reference 2.4v > + * CLK_SEL = INTOSC w/o CLKOUT, CS_SEL = No Bias, > + * ADC_MODE = Standby Mode --> (0b11100010). > + * CONFIG0 = 0x000000E2 > + */ > + ret = mcp356x_write(adc, MCP356X_CONFIG0, 0x000000E2, 1); > + > + dev_dbg(&adc->spi->dev, "%s: Using internal Vref\n", > + __func__); > + adc->vref_mv = MCP356XR_INT_VREF_MV; > + > + } else { > + /* CONFIG0 --> CLK_SEL = INTOSC w/o CLKOUT, CS_SEL = No Bias, > + * ADC_MODE = Standby Mode --> (0b01100010). > + * CONFIG0 = 0x000000E2 > + */ > + ret = mcp356x_write(adc, MCP356X_CONFIG0, 0x00000062, 1); > + } > + adc->current_bias_mode = MCP356X_CS_SEL_0_0_uA; > + > + return ret; > +} > + > +static int mcp356x_probe(struct spi_device *spi) > +{ > + int ret, device_index; > + struct iio_dev *indio_dev; > + struct mcp356x_state *adc; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); > + if (!indio_dev) { > + dev_err_probe(&indio_dev->dev, PTR_ERR(indio_dev), > + "Can't allocate iio device\n"); > + return -ENOMEM; > + } > + > + adc = iio_priv(indio_dev); > + adc->spi = spi; > + > + dev_dbg(&adc->spi->dev, "%s: probe(spi = 0x%p)\n", __func__, spi); > + > + adc->vref = devm_regulator_get_optional(&adc->spi->dev, "vref"); > + if (IS_ERR(adc->vref)) { > + if (PTR_ERR(adc->vref) == -ENODEV) { > + adc->vref = NULL; > + dev_dbg(&adc->spi->dev, "%s: Using internal Vref\n", > + __func__); > + } else { > + dev_err_probe(&adc->spi->dev, PTR_ERR(adc->vref), > + "failed to get regulator\n"); > + return PTR_ERR(adc->vref); You can "return dev_err_probe(...);" directly and save a line of code > + } > + } else { > + ret = regulator_enable(adc->vref); > + if (ret) > + return ret; > + Use devm_add_action_or_reset to register a disable function. That gets rid of the gotos and the remove() callback as well. See my version of the driver for implementation example. > + dev_dbg(&adc->spi->dev, "%s: Using External Vref\n", > + __func__); > + > + ret = regulator_get_voltage(adc->vref); > + if (ret < 0) { > + dev_err_probe(&adc->spi->dev, ret, > + "Failed to read vref regulator\n"); > + goto error_disable_reg; > + } > + > + adc->vref_mv = ret / 1000; > + } > + > + spi_set_drvdata(spi, indio_dev); > + device_index = spi_get_device_id(spi)->driver_data; > + adc->chip_info = &mcp356x_chip_infos_tbl[device_index]; > + > + adc->mcp356x_info.read_raw = mcp356x_read_raw; > + adc->mcp356x_info.write_raw = mcp356x_write_raw; > + adc->mcp356x_info.read_avail = mcp356x_read_avail; > + > + ret = mcp356x_prep_custom_attributes(adc, indio_dev); > + if (ret) { > + dev_err_probe(&adc->spi->dev, ret, > + "Can't configure custom attributes for MCP356X device\n"); > + goto error_disable_reg; > + } > + > + indio_dev->name = spi_get_device_id(spi)->name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &adc->mcp356x_info; > + > + indio_dev->channels = adc->chip_info->channels; > + indio_dev->num_channels = adc->chip_info->num_channels; > + indio_dev->masklength = adc->chip_info->num_channels - 1; > + > + /* initialize the chip access mutex */ > + mutex_init(&adc->lock); > + > + /* Do any chip specific initialization, e.g: > + * read/write some registers > + * enable/disable certain channels > + * change the sampling rate to the requested value > + */ > + ret = mcp356x_config(adc); > + if (ret) { > + dev_err_probe(&adc->spi->dev, ret, > + "Can't configure MCP356X device\n"); > + goto error_disable_reg; > + } > + > + dev_dbg(&adc->spi->dev, "%s: Vref (mV): %d\n", __func__, adc->vref_mv); > + > + ret = devm_iio_device_register(&spi->dev, indio_dev); > + if (ret) { > + dev_err_probe(&adc->spi->dev, ret, > + "Can't register IIO device\n"); > + goto error_disable_reg; > + } > + > + return 0; > + > +error_disable_reg: > + if (adc->vref) > + regulator_disable(adc->vref); > + > + return ret; > +} > + > +static void mcp356x_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct mcp356x_state *adc = iio_priv(indio_dev); > + > + if (adc->vref) > + regulator_disable(adc->vref); > +} > + > +static const struct of_device_id mcp356x_dt_ids[] = { > + { .compatible = "microchip,mcp3461" }, > + { .compatible = "microchip,mcp3462" }, > + { .compatible = "microchip,mcp3464" }, > + { .compatible = "microchip,mcp3461r" }, > + { .compatible = "microchip,mcp3462r" }, > + { .compatible = "microchip,mcp3464r" }, > + { .compatible = "microchip,mcp3561" }, > + { .compatible = "microchip,mcp3562" }, > + { .compatible = "microchip,mcp3564" }, > + { .compatible = "microchip,mcp3561r" }, > + { .compatible = "microchip,mcp3562r" }, > + { .compatible = "microchip,mcp3564r" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, mcp356x_dt_ids); > + > +static const struct spi_device_id mcp356x_id[] = { > + { "mcp3461", mcp3461 }, > + { "mcp3462", mcp3462 }, > + { "mcp3464", mcp3464 }, > + { "mcp3461r", mcp3461r }, > + { "mcp3462r", mcp3462r }, > + { "mcp3464r", mcp3464r }, > + { "mcp3561", mcp3561 }, > + { "mcp3562", mcp3562 }, > + { "mcp3564", mcp3564 }, > + { "mcp3561r", mcp3561r }, > + { "mcp3562r", mcp3562r }, > + { "mcp3564r", mcp3564r }, > + { } > +}; > +MODULE_DEVICE_TABLE(spi, mcp356x_id); > + > +static struct spi_driver mcp356x_driver = { > + .driver = { > + .name = "mcp3564", > + .of_match_table = mcp356x_dt_ids, > + }, > + .probe = mcp356x_probe, > + .remove = mcp356x_remove, > + .id_table = mcp356x_id, > +}; > + > +module_spi_driver(mcp356x_driver); > + > +MODULE_AUTHOR("Marius Cristea <marius.cristea@microchip.com>"); > +MODULE_DESCRIPTION("Microchip MCP346x/MCP346xR and MCP356x/MCP346xR ADCs"); > +MODULE_LICENSE("GPL v2"); -- Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topic.nl W: www.topic.nl ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC 2023-05-24 11:05 ` Mike Looijmans @ 2023-05-28 19:20 ` Jonathan Cameron 2023-06-20 14:34 ` Mike Looijmans 0 siblings, 1 reply; 16+ messages in thread From: Jonathan Cameron @ 2023-05-28 19:20 UTC (permalink / raw) To: Mike Looijmans Cc: marius.cristea, lars, robh+dt, linux-iio, devicetree, linux-kernel > > > + > > +static inline u8 mcp356x_reg_fast_cmd(u8 chip_addr, u8 cmd) > > +{ > > + return ((chip_addr << 6) | (cmd << 2)); > > +} > > + > > +static int mcp356x_read(struct mcp356x_state *adc, u8 reg, u32 *val, u8 len) > > +{ > > + int ret; > > + u8 tmp_reg; > > + > > + tmp_reg = mcp356x_reg_read(adc->dev_addr, reg); > > + > > + ret = spi_write_then_read(adc->spi, &tmp_reg, 1, val, len); > > I had issues with spi_write_then_read not communicating properly with the > device. This may be due to the SPI controller (Xilinx IP in FPGA) though. That's a bit worrying! Shouldn't be a problem unless there is something fiddly required with the chip select or similar. > > I had to use spi_sync() to get reliable answers. Also has the added benefit of > giving access to the interrupt flags. > > > + > > +#define MCP3564_CHANNELS(depth) { \ > > + MCP356X_V_CHANNEL(0, 0, depth), \ > > + MCP356X_V_CHANNEL(1, 1, depth), \ > > + MCP356X_V_CHANNEL(2, 2, depth), \ > > + MCP356X_V_CHANNEL(3, 3, depth), \ > > + MCP356X_V_CHANNEL(4, 4, depth), \ > > + MCP356X_V_CHANNEL(5, 5, depth), \ > > + MCP356X_V_CHANNEL(6, 6, depth), \ > > + MCP356X_V_CHANNEL(7, 7, depth), \ > > + MCP356X_T_CHAN(depth), \ > > + MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth), \ > > + MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth), \ > > + MCP356X_V_CHANNEL_DIFF(0, 2, 0x02, depth), \ > > + MCP356X_V_CHANNEL_DIFF(0, 3, 0x03, depth), \ > > + MCP356X_V_CHANNEL_DIFF(1, 2, 0x12, depth), \ > > + MCP356X_V_CHANNEL_DIFF(1, 3, 0x13, depth), \ > > + MCP356X_V_CHANNEL_DIFF(2, 3, 0x23, depth), \ > > + MCP356X_V_CHANNEL_DIFF(2, 0, 0x20, depth), \ > > + MCP356X_V_CHANNEL_DIFF(3, 0, 0x30, depth), \ > > + MCP356X_V_CHANNEL_DIFF(2, 1, 0x21, depth), \ > > + MCP356X_V_CHANNEL_DIFF(3, 1, 0x31, depth), \ > > + MCP356X_V_CHANNEL_DIFF(3, 2, 0x32, depth), \ > > + MCP356X_V_CHANNEL_DIFF(0, 4, 0x04, depth), \ > > + MCP356X_V_CHANNEL_DIFF(0, 5, 0x05, depth), \ > > + MCP356X_V_CHANNEL_DIFF(0, 6, 0x06, depth), \ > > + MCP356X_V_CHANNEL_DIFF(0, 7, 0x07, depth), \ > > + MCP356X_V_CHANNEL_DIFF(1, 4, 0x14, depth), \ > > + MCP356X_V_CHANNEL_DIFF(1, 5, 0x15, depth), \ > > + MCP356X_V_CHANNEL_DIFF(1, 6, 0x16, depth), \ > > + MCP356X_V_CHANNEL_DIFF(1, 7, 0x17, depth), \ > > + MCP356X_V_CHANNEL_DIFF(2, 4, 0x24, depth), \ > > + MCP356X_V_CHANNEL_DIFF(2, 5, 0x25, depth), \ > > + MCP356X_V_CHANNEL_DIFF(2, 6, 0x26, depth), \ > > + MCP356X_V_CHANNEL_DIFF(2, 7, 0x27, depth), \ > > + MCP356X_V_CHANNEL_DIFF(3, 4, 0x34, depth), \ > > + MCP356X_V_CHANNEL_DIFF(3, 5, 0x35, depth), \ > > + MCP356X_V_CHANNEL_DIFF(3, 6, 0x36, depth), \ > > + MCP356X_V_CHANNEL_DIFF(3, 7, 0x37, depth), \ > > + MCP356X_V_CHANNEL_DIFF(4, 5, 0x45, depth), \ > > + MCP356X_V_CHANNEL_DIFF(4, 6, 0x46, depth), \ > > + MCP356X_V_CHANNEL_DIFF(4, 7, 0x47, depth), \ > > + MCP356X_V_CHANNEL_DIFF(5, 6, 0x56, depth), \ > > + MCP356X_V_CHANNEL_DIFF(5, 7, 0x57, depth), \ > > + MCP356X_V_CHANNEL_DIFF(6, 7, 0x67, depth), \ > > + MCP356X_V_CHANNEL_DIFF(4, 0, 0x40, depth), \ > > + MCP356X_V_CHANNEL_DIFF(5, 0, 0x50, depth), \ > > + MCP356X_V_CHANNEL_DIFF(6, 0, 0x60, depth), \ > > + MCP356X_V_CHANNEL_DIFF(7, 0, 0x70, depth), \ > > + MCP356X_V_CHANNEL_DIFF(4, 1, 0x41, depth), \ > > + MCP356X_V_CHANNEL_DIFF(5, 1, 0x51, depth), \ > > + MCP356X_V_CHANNEL_DIFF(6, 1, 0x61, depth), \ > > + MCP356X_V_CHANNEL_DIFF(7, 1, 0x71, depth), \ > > + MCP356X_V_CHANNEL_DIFF(4, 2, 0x42, depth), \ > > + MCP356X_V_CHANNEL_DIFF(5, 2, 0x52, depth), \ > > + MCP356X_V_CHANNEL_DIFF(6, 2, 0x62, depth), \ > > + MCP356X_V_CHANNEL_DIFF(7, 2, 0x72, depth), \ > > + MCP356X_V_CHANNEL_DIFF(4, 3, 0x43, depth), \ > > + MCP356X_V_CHANNEL_DIFF(5, 3, 0x53, depth), \ > > + MCP356X_V_CHANNEL_DIFF(6, 3, 0x63, depth), \ > > + MCP356X_V_CHANNEL_DIFF(7, 3, 0x73, depth), \ > > + MCP356X_V_CHANNEL_DIFF(5, 4, 0x54, depth), \ > > + MCP356X_V_CHANNEL_DIFF(6, 4, 0x64, depth), \ > > + MCP356X_V_CHANNEL_DIFF(7, 4, 0x74, depth), \ > > + MCP356X_V_CHANNEL_DIFF(6, 5, 0x65, depth), \ > > + MCP356X_V_CHANNEL_DIFF(7, 5, 0x75, depth), \ > > + MCP356X_V_CHANNEL_DIFF(7, 6, 0x76, depth) \ > > +} > > Nice that the chip can do full 8x8 mux delivering 56 channels, but I don't > think that will be useful to many people. > > Also I'd want to see buffer support added to the device, which only works for > the channels as in the table in the datasheet, so only 4 differential ones. > It'd be annoying to have 56 channels but only be able to contiuously read 4 > specific ones. One common solution to this is to push the channel setup to firmware as it corresponds to 'what is wired'. See the channel bindings in Documentation/devicetree/bindings/iio/adc/adc.yaml and various examples in tree. Thanks for combining efforts btw - should give us the best overall result but I feel for you when you discovered the duplication :( Jonathan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC 2023-05-28 19:20 ` Jonathan Cameron @ 2023-06-20 14:34 ` Mike Looijmans 0 siblings, 0 replies; 16+ messages in thread From: Mike Looijmans @ 2023-06-20 14:34 UTC (permalink / raw) To: Jonathan Cameron Cc: marius.cristea, lars, robh+dt, linux-iio, devicetree, linux-kernel On 28-05-2023 21:20, Jonathan Cameron wrote: >>> + >>> +static inline u8 mcp356x_reg_fast_cmd(u8 chip_addr, u8 cmd) >>> +{ >>> + return ((chip_addr << 6) | (cmd << 2)); >>> +} >>> + >>> +static int mcp356x_read(struct mcp356x_state *adc, u8 reg, u32 *val, u8 len) >>> +{ >>> + int ret; >>> + u8 tmp_reg; >>> + >>> + tmp_reg = mcp356x_reg_read(adc->dev_addr, reg); >>> + >>> + ret = spi_write_then_read(adc->spi, &tmp_reg, 1, val, len); >> I had issues with spi_write_then_read not communicating properly with the >> device. This may be due to the SPI controller (Xilinx IP in FPGA) though. > That's a bit worrying! Shouldn't be a problem unless there is something > fiddly required with the chip select or similar. I suspect it's the Xilinx SPI controller IP that's misbehaving, we saw some strange behavior of the CS line while probing with a scope. Judging from the scope images I suspect it tri-states the CS line between the transfers, instead of always driving it low or high. > >> I had to use spi_sync() to get reliable answers. Also has the added benefit of >> giving access to the interrupt flags. >>> + >>> +#define MCP3564_CHANNELS(depth) { \ >>> + MCP356X_V_CHANNEL(0, 0, depth), \ >>> + MCP356X_V_CHANNEL(1, 1, depth), \ >>> + MCP356X_V_CHANNEL(2, 2, depth), \ >>> + MCP356X_V_CHANNEL(3, 3, depth), \ >>> + MCP356X_V_CHANNEL(4, 4, depth), \ >>> + MCP356X_V_CHANNEL(5, 5, depth), \ >>> + MCP356X_V_CHANNEL(6, 6, depth), \ >>> + MCP356X_V_CHANNEL(7, 7, depth), \ >>> + MCP356X_T_CHAN(depth), \ >>> + MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(0, 2, 0x02, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(0, 3, 0x03, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(1, 2, 0x12, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(1, 3, 0x13, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(2, 3, 0x23, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(2, 0, 0x20, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(3, 0, 0x30, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(2, 1, 0x21, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(3, 1, 0x31, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(3, 2, 0x32, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(0, 4, 0x04, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(0, 5, 0x05, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(0, 6, 0x06, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(0, 7, 0x07, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(1, 4, 0x14, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(1, 5, 0x15, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(1, 6, 0x16, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(1, 7, 0x17, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(2, 4, 0x24, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(2, 5, 0x25, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(2, 6, 0x26, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(2, 7, 0x27, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(3, 4, 0x34, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(3, 5, 0x35, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(3, 6, 0x36, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(3, 7, 0x37, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(4, 5, 0x45, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(4, 6, 0x46, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(4, 7, 0x47, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(5, 6, 0x56, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(5, 7, 0x57, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(6, 7, 0x67, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(4, 0, 0x40, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(5, 0, 0x50, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(6, 0, 0x60, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(7, 0, 0x70, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(4, 1, 0x41, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(5, 1, 0x51, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(6, 1, 0x61, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(7, 1, 0x71, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(4, 2, 0x42, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(5, 2, 0x52, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(6, 2, 0x62, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(7, 2, 0x72, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(4, 3, 0x43, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(5, 3, 0x53, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(6, 3, 0x63, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(7, 3, 0x73, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(5, 4, 0x54, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(6, 4, 0x64, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(7, 4, 0x74, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(6, 5, 0x65, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(7, 5, 0x75, depth), \ >>> + MCP356X_V_CHANNEL_DIFF(7, 6, 0x76, depth) \ >>> +} >> Nice that the chip can do full 8x8 mux delivering 56 channels, but I don't >> think that will be useful to many people. >> >> Also I'd want to see buffer support added to the device, which only works for >> the channels as in the table in the datasheet, so only 4 differential ones. >> It'd be annoying to have 56 channels but only be able to contiuously read 4 >> specific ones. > One common solution to this is to push the channel setup to firmware > as it corresponds to 'what is wired'. See the channel bindings > in Documentation/devicetree/bindings/iio/adc/adc.yaml > and various examples in tree. Makes sense. Also would allow you to hide inputs that aren't connected anyway. > > Thanks for combining efforts btw - should give us the best overall > result but I feel for you when you discovered the duplication :( What worries me more is that Marius Cristea isn't responding... maybe he's on a holiday? I'd be happy to join forces on this... > > Jonathan -- Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topic.nl W: www.topic.nl ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-07-02 10:28 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-19 16:01 [PATCH 0/2] Adding support for Microchip MCP3564 ADC family marius.cristea 2023-05-19 16:01 ` [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC marius.cristea 2023-05-19 18:29 ` Conor Dooley 2023-05-20 15:17 ` Jonathan Cameron 2023-06-08 19:34 ` Rob Herring 2023-06-09 17:41 ` Jonathan Cameron 2023-05-20 13:20 ` Jonathan Cameron 2023-05-24 10:37 ` Mike Looijmans 2023-05-28 19:16 ` Jonathan Cameron 2023-05-19 16:01 ` [PATCH 2/2] iio: adc: adding support for " marius.cristea 2023-05-20 15:15 ` Jonathan Cameron 2023-06-22 11:32 ` Marius.Cristea 2023-07-02 10:28 ` Jonathan Cameron 2023-05-24 11:05 ` Mike Looijmans 2023-05-28 19:20 ` Jonathan Cameron 2023-06-20 14:34 ` Mike Looijmans
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).