Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 13 Apr 2024 11:21:55 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Sat, Apr 13, 2024 at 10:06 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 12 Apr 2024 16:23:00 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >  
> > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
> > > <kimseer.paller@analog.com> wrote:  
> > > >  
> 
> ...
> 
> > >
> > > And there is V~ on both which can be between -5.5V/-15.75V and GND, so
> > > optional v-neg-supply seems appropriate.  
> >
> > Only make it optional in the binding if the settings of the device change
> > depending on whether it is there or not.  Looks like there is an internal
> > reference, so maybe it really is optional.  
> 
> I suggested optional with the thinking that if the pin is tied to GND,
> then the property would be omitted.

We could but given VND isn't really that special in this case I think
I'd prefer a fixed voltage reg of 0V if someone does wire it like that.
(I think that works, though not sure I've tried a 0V supply ;)
> 
> 
> ...
> 
> 
> > >
> > > * (both) The MUX/MUXOUT pins look like we have an embedded pin mux, so
> > > it could mean we need #pinctrl-cells. ltc2664 would also need
> > > muxin-gpios for this.  
> > Not convinced that's the right approach - looks more like a channel
> > selector than a conventional mux or pin control. Sure that's a mux, but
> > we want a clean userspace control to let us choose a signal to measure
> > at runtime
> >
> > If you wanted to support this I'd have the binding describe optional
> > stuff to act as a consumer of an ADC channel on another device.
> > The IIO driver would then provide a bunch of input channels to allow
> > measurement of each of the signals.
> >
> > Look at io-channels etc in existing bindings for how to do that.
> >  
> 
> Right. I was thinking that this pin might be connected to something
> else external rather than the signal coming back to the SoC (or
> whatever has the SPI controller). But it makes more sense that we
> would want it as extra channels being read back by the SoC for
> diagnostics.

It might indeed.  But I think that's an exercise for the future if
it matters.  Might be a debugfs control only perhaps.

> 
> ...
> 
> > >  
> > > > +
> > > > +      patternProperties:
> > > > +        "^channel@([0-3])$":
> > > > +          $ref: '#/$defs/toggle-operation'
> > > > +          unevaluatedProperties: false
> > > > +
> > > > +          description: Channel in toggle functionality.
> > > > +
> > > > +          properties:
> > > > +            adi,output-range-microvolt:
> > > > +              description: Specify the channel output full scale range.  
> > >
> > > How would someone writing a .dts know what values to select for this
> > > property? Or is this something that should be configured at runtime
> > > instead of in the devicetree? Or should this info come from the
> > > missing voltage supplies I mentioned?  
> >
> > Sometimes this one is a wiring related choice.  Sometimes to the extent
> > that picking the wrong one from any userspace control can cause damage
> > or is at least nonsense.
> >
> > You look to be right though that the possible values here aren' fine
> > if the internal reference is used, but not the external.
> >
> > However, it's keyed off MPS pins so you can't control it if they aren't
> > tied to all high.  So I'd imagine if the board can be damaged it will
> > be hard wired.  Hence these could be controlled form userspace.
> > It's a bit fiddly though as combines scale and offset controls and
> > you can end trying to set things to an invalid combination.
> > E.g. scale set to cover 20V range and offset set to 0V
> > To get around that you have to clamp one parameter to nearest
> > possible when the other is changed.
> >  
> 
> Thanks for the explanation. It sounds like I missed something in the
> datasheet that would be helpful to call out in the description for
> this property.
Agreed - it needs more detail.

Jonathan