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

[Jonathan Cameron]

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