Re: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911

[Marcus Folkesson]

Hello Rob,

Thank you for the review.

On Wed, Jul 25, 2018 at 11:51:17AM -0600, Rob Herring wrote:
> On Tue, Jul 24, 2018 at 08:30:03PM +0200, Marcus Folkesson wrote:
> > MCP3911 is a dual channel Analog Front End (AFE) containing two
> > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
> > 
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>
> > Signed-off-by: Kent Gustavsson <kent@xxxxxxxxxx>
> > ---
> > 
> > Notes:
> >     v2:
> >     	- drop channel width
> >     	- drop `external_vref`
> >     	- replace `external-clock` with proper clock bindings
> > 
> >  .../devicetree/bindings/iio/adc/mcp3911.txt        | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> > new file mode 100644
> > index 000000000000..af5472f51a84
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> > @@ -0,0 +1,28 @@
> > +* Microchip MCP3911 Dual channel analog front end (ADC)
> > +
> > +Required properties:
> > + - compatible: Should be "microchip,mcp3911"
> > + - reg: SPI chip select number for the device
> > +
> > +Recommended properties:
> > + - spi-max-frequency: Definition as per
> > +	 Documentation/devicetree/bindings/spi/spi-bus.txt.
> > +	 Max frequency for this chip is 20MHz.
> > +
> > +Optional properties:
> > + - device-addr: Device address when multiple MCP3911 chips are present on the
> > +	same SPI bus. Valid values are 0-3. Defaults to 0.
> 
> Isn't this the reg value?
> 

The reg value defines which chip select the chip hangs on.
The chip has support to connect up to four chips on the same SPI bus,
including the same chip select signal.

The chips are separated by the device-addr as it is part of the
communication protocol.

When reading the description for device-addr, I agree that it fits well
for the reg property as well..


> > + - vref-supply: Phandle to the external reference voltage supply.
> > + - clocks: Phandle and clock identifier (see clock-names)
> > + - clock-names: "adc_clk" for the ADC (sampling) clock
> 
> Datasheet calls this clki (or mclk internally). Or just drop clock-names 
> as it is pointless 
> when there is only 1 clock. 

I will drop the clock-names, thanks!

> 
> Also DR handling as an IRQ is missing.

I have considered using the DR signal, but as we not support buffering
yet I did not see any point in using it.

From the datasheet, section 7.1 ::

	These registers [channel registers] are latched when an
	ADC read communication occurs. When a data ready
	event occurs during a read communication, the most
	current ADC data is also latched to avoid data
	corruption issues. The three bytes of each channel are
	updated synchronously at a DRCLK rate. The three
	bytes can be accessed separately if needed but are
	refreshed synchronously.

So it should be safe to read the values independently of the DR signal.
Or maybe I'm missing something.


Best regards
Marcus Folkesson

Attachment: signature.asc
Description: PGP signature