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

On Tue, 18 Jul 2023 09:24:58 +0000
[off-list ref] wrote:

Hey Conor,


On Sat, 2023-07-15 at 11:28 +0100, Conor Dooley wrote:

quoted

Hey,

On Fri, Jul 14, 2023 at 06:00:50PM +0300,
marius.cristea@microchip.com wrote:  

quoted

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.

quoted

quoted

+  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/ (local)
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.

quoted

quoted

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