On Tue, 18 Jul 2023 09:24:58 +0000 <Marius.Cristea@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx wrote: > > > From: Marius Cristea <marius.cristea@xxxxxxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxx> > > > > 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@xxxxxxxxxx/ > > 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
