Re: [PATCH v2 1/2] dt-bindings: iio: adc: adding MCP3564 ADC

[Marius.Cristea]

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.

 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.



> 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.