[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20230720193848.5b9c3b59@jic23-huawei>
Date: Thu, 20 Jul 2023 19:38:48 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: <Marius.Cristea@...rochip.com>
Cc: <conor@...nel.org>, <linux-iio@...r.kernel.org>,
<devicetree@...r.kernel.org>, <lars@...afoo.de>,
<linux-kernel@...r.kernel.org>, <conor+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <robh+dt@...nel.org>
Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: adding MCP3564 ADC
On Tue, 18 Jul 2023 09:24:58 +0000
<Marius.Cristea@...rochip.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@...rochip.com wrote:
> > > From: Marius Cristea <marius.cristea@...rochip.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@...rochip.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
Powered by blists - more mailing lists