| Message ID | 20230714150051.637952-2-marius.cristea@microchip.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Adding support for Microchip MCP3564 ADC family | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
Hey, On Fri, Jul 14, 2023 at 06:00:50PM +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 (Microchip's > MCP3461, MCP3462, MCP3464, MCP3461R, MCP3462R, > MCP3464R, MCP3561, MCP3562, MCP3564, MCP3561R, > MCP3562R and MCP3564R analog to digital converters). > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> This looks good to me, other than the custom property, for which I can't tell if a consensus was reached on last time around. > + 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. > + Each device communication starts by a CS falling edge, followed by the > + clocking of the device address (BITS[7:6] - top two bits of COMMAND BYTE > + which is first one on the wire). On the last version, the last comment I could find on lore was https://lore.kernel.org/all/20230609184149.00002766@Huawei.com/ where Jonathan and Rob were discussing whether or not a spi-mux type of thing could work, but it does not seem to have ended conclusively. Rob or Jonathan, would you mind commenting on that? There was also a comment from Jonathan: > > + 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. The binding does now enforce the vref supply where relevant, but it sounds like you were looking more supplies to be documented Jonathan? (AVdd, DVdd etc) Thanks, Conor.
On Sat, 15 Jul 2023 11:28:03 +0100 Conor Dooley <conor@kernel.org> wrote: > Hey, > > On Fri, Jul 14, 2023 at 06:00:50PM +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 (Microchip's > > MCP3461, MCP3462, MCP3464, MCP3461R, MCP3462R, > > MCP3464R, MCP3561, MCP3562, MCP3564, MCP3561R, > > MCP3562R and MCP3564R analog to digital converters). > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > This looks good to me, other than the custom property, for which I can't > tell if a consensus was reached on last time around. > > > + 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. > > + Each device communication starts by a CS falling edge, followed by the > > + clocking of the device address (BITS[7:6] - top two bits of COMMAND BYTE > > + which is first one on the wire). > > On the last version, the last comment I could find on lore was > https://lore.kernel.org/all/20230609184149.00002766@Huawei.com/ > where Jonathan and Rob were discussing whether or not a spi-mux type of > thing could work, but it does not seem to have ended conclusively. > > Rob or Jonathan, would you mind commenting on that? Sure - as far as I'm concerned - it looks like it should be possible to do something generic, but without a prototype it's hard to be sure how fiddly that will be. +CC Mark Brown who might be able to give a more informed answer to whether such a thing would work / be easy to implement. I've no idea how common this trick is. If it's a one off, may not be worth the bother of a more generic mux like binding whether that is the more elegant solution or not. > > There was also a comment from Jonathan: > > > + 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. > > The binding does now enforce the vref supply where relevant, but it > sounds like you were looking more supplies to be documented Jonathan? > (AVdd, DVdd etc) Exactly. > > Thanks, > Conor.
On 14/07/2023 17:00, 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 ... > + > +dependencies: > + spi-cpol: [ spi-cpha ] > + spi-cpha: [ spi-cpol ] Put dependencies after patternProperties:, before required:. > + > +patternProperties: > + "^channel@([0-9]|([1-7][0-9]))$": > + $ref: adc.yaml > + type: object Missing unevaluatedProperties: false. Open other bindings and look how it is done there. > + description: Represents the external channels which are connected to the ADC. > + > + properties: > + reg: > + description: The channel number in single-ended and differential mode. > + minimum: 0 > + maximum: 79 > + > + diff-channels: true Why? Drop, unless you want to say there all other ADC properties are invalid for this type of device (device, not driver!). > + > + required: > + - reg Best regards, Krzysztof
Hey Conor, On Sat, 2023-07-15 at 11:28 +0100, Conor Dooley wrote: > Hey, > > On Fri, Jul 14, 2023 at 06:00:50PM +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 (Microchip's > > MCP3461, MCP3462, MCP3464, MCP3461R, MCP3462R, > > MCP3464R, MCP3561, MCP3562, MCP3564, MCP3561R, > > MCP3562R and MCP3564R analog to digital converters). > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > This looks good to me, other than the custom property, for which I > can't > tell if a consensus was reached on last time around. > I don't think a consensus related to "custom property" was reached last time around. I was aiming to fix all other review comments first and leave the "details" at the end. I still think is a good idea to have some custom properties that will impact only a certain range of devices and leave the user to decide/choose how to use that configuration setting (to better suite his needs). If the application doesn't recognize the property it will be configured with the default value and it should not broke anything. If we decide that we need to take out the custom properties, then I could move some of them into the Device Tree. > > + 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. > > + Each device communication starts by a CS falling edge, > > followed by the > > + clocking of the device address (BITS[7:6] - top two bits of > > COMMAND BYTE > > + which is first one on the wire). > > On the last version, the last comment I could find on lore was > https://lore.kernel.org/all/20230609184149.00002766@Huawei.com/ > where Jonathan and Rob were discussing whether or not a spi-mux type > of > thing could work, but it does not seem to have ended conclusively. > > Rob or Jonathan, would you mind commenting on that? > I think in case of a single device on bus, we could avoid using the spi-mux. > There was also a comment from Jonathan: > > > + 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. > > The binding does now enforce the vref supply where relevant, but it > sounds like you were looking more supplies to be documented Jonathan? > (AVdd, DVdd etc) > All other supply (like AVdd, DVdd etc) for this particular chip doesn't have any direct impact (way of working, resolution, any configuration setup), so I'm not sure if we need to add anything more here. > Thanks, > Conor.
Hi Krzysztof, > > + > > +patternProperties: > > + "^channel@([0-9]|([1-7][0-9]))$": > > + $ref: adc.yaml > > + type: object > > Missing unevaluatedProperties: false. > > Open other bindings and look how it is done there. > > > + description: Represents the external channels which are > > connected to the ADC. > > + > > + properties: > > + reg: > > + description: The channel number in single-ended and > > differential mode. > > + minimum: 0 > > + maximum: 79 > > + > > + diff-channels: true > > Why? Drop, unless you want to say there all other ADC properties are > invalid for this type of device (device, not driver!). > > > + > > + required: > > + - reg > > All other ADC properties are valid. Here I was trying to add some properties for each the channel (ADC channel) used by user on this ADC. The channel could be single ended (Channel to ground) or "diff-channels" where I need to know the pins/channel used. Maybe I'm missing something but I was trying to follow the binding from: Documentation/devicetree/bindings/iio/adc/adi,ad7292.yaml Best regards, Marius
On 19/07/2023 17:40, Marius.Cristea@microchip.com wrote: > Hi Krzysztof, > >>> + >>> +patternProperties: >>> + "^channel@([0-9]|([1-7][0-9]))$": >>> + $ref: adc.yaml >>> + type: object >> >> Missing unevaluatedProperties: false. >> >> Open other bindings and look how it is done there. >> >>> + description: Represents the external channels which are >>> connected to the ADC. >>> + >>> + properties: >>> + reg: >>> + description: The channel number in single-ended and >>> differential mode. >>> + minimum: 0 >>> + maximum: 79 >>> + >>> + diff-channels: true >> >> Why? Drop, unless you want to say there all other ADC properties are >> invalid for this type of device (device, not driver!). >> >>> + >>> + required: >>> + - reg >> >> > > All other ADC properties are valid. So drop what I questioned. Best regards, Krzysztof
On Tue, 18 Jul 2023 09:24:58 +0000 <Marius.Cristea@microchip.com> wrote: > Hey Conor, > > > On Sat, 2023-07-15 at 11:28 +0100, Conor Dooley wrote: > > Hey, > > > > On Fri, Jul 14, 2023 at 06:00:50PM +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 (Microchip's > > > MCP3461, MCP3462, MCP3464, MCP3461R, MCP3462R, > > > MCP3464R, MCP3561, MCP3562, MCP3564, MCP3561R, > > > MCP3562R and MCP3564R analog to digital converters). > > > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > > > This looks good to me, other than the custom property, for which I > > can't > > tell if a consensus was reached on last time around. > > > > I don't think a consensus related to "custom property" was reached > last time around. I was aiming to fix all other review comments first > and leave the "details" at the end. That's fair enough as a way to move things forward - just make sure to mention open questions in your cover letter / patch descriptions or under the --- > > I still think is a good idea to have some custom properties that will > impact only a certain range of devices and leave the user to > decide/choose how to use that configuration setting (to better suite > his needs). If the application doesn't recognize the property it will > be configured with the default value and it should not broke anything. > > If we decide that we need to take out the custom properties, then I > could move some of them into the Device Tree. > > > > + 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. > > > + Each device communication starts by a CS falling edge, > > > followed by the > > > + clocking of the device address (BITS[7:6] - top two bits of > > > COMMAND BYTE > > > + which is first one on the wire). > > > > On the last version, the last comment I could find on lore was > > https://lore.kernel.org/all/20230609184149.00002766@Huawei.com/ > > where Jonathan and Rob were discussing whether or not a spi-mux type > > of > > thing could work, but it does not seem to have ended conclusively. > > > > Rob or Jonathan, would you mind commenting on that? > > > > I think in case of a single device on bus, we could avoid using the > spi-mux. > That's a fair point. I think key here is how common this is, and I have no idea. > > > 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. > > > > The binding does now enforce the vref supply where relevant, but it > > sounds like you were looking more supplies to be documented Jonathan? > > (AVdd, DVdd etc) > > > > All other supply (like AVdd, DVdd etc) for this particular chip > doesn't have any direct impact (way of working, resolution, any > configuration setup), so I'm not sure if we need to add anything more > here. > Pretty big impact if they aren't turned on ;) Expectation is the driver will just ensure they are and it can only do that if it knows they exist. Jonathan
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..297b77eb1cb0 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3564.yaml @@ -0,0 +1,197 @@ +# 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. Datasheet can be found here: + 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 + +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: + maximum: 20000000 + + spi-cpha: true + + spi-cpol: true + + 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 + + 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). In + case of "r" devices (e. g. mcp3564r), if it does not exists the internal + reference will be used. + + 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. + Each device communication starts by a CS falling edge, followed by the + clocking of the device address (BITS[7:6] - top two bits of COMMAND BYTE + which is first one on the wire). + + "#io-channel-cells": + const: 1 + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +dependencies: + spi-cpol: [ spi-cpha ] + spi-cpha: [ spi-cpol ] + +patternProperties: + "^channel@([0-9]|([1-7][0-9]))$": + $ref: adc.yaml + type: object + description: Represents the external channels which are connected to the ADC. + + properties: + reg: + description: The channel number in single-ended and differential mode. + minimum: 0 + maximum: 79 + + diff-channels: true + + required: + - reg + +required: + - compatible + - reg + - microchip,hw-device-address + - spi-max-frequency + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + - # External vref, no internal reference + if: + properties: + compatible: + contains: + enum: + - microchip,mcp3461 + - microchip,mcp3462 + - microchip,mcp3464 + - microchip,mcp3561 + - microchip,mcp3562 + - microchip,mcp3564 + then: + required: + - vref-supply + +unevaluatedProperties: 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>; + + #address-cells = <1>; + #size-cells = <0>; + + channel@0 { + /* CH0 to AGND */ + reg = <0>; + }; + + channel@1 { + /* CH1 to AGND */ + reg = <1>; + }; + + /* diff-channels */ + channel@11 { + reg = <11>; + + /* CN0, CN1 */ + diff-channels = <0 1>; + }; + + channel@22 { + reg = <0x22>; + + /* CN1, CN2 */ + diff-channels = <1 2>; + }; + + channel@23 { + reg = <0x23>; + + /* CN1, CN3 */ + diff-channels = <1 3>; + }; + }; + }; +...