| Message ID | 20230714150051.637952-2-marius.cristea@microchip.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Adding support for Microchip MCP3564 ADC family | expand |
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.
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 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>; + }; + }; + }; +...