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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 16 Apr 2024 14:40:56 +0000
"Paller, Kim Seer" <KimSeer.Paller@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, April 14, 2024 1:10 AM
> > To: David Lechner <dlechner@baylibre.com>
> > Cc: Paller, Kim Seer <KimSeer.Paller@analog.com>; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Lars-Peter Clausen
> > <lars@metafoo.de>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> > Hennerich, Michael <Michael.Hennerich@analog.com>
> > Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml
> > 
> > [External]
> > 
> > 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 ;)  
> 
> To clarify, does this mean we should still add an optional property for it in the binding?
I think it should not be optional.  Should be fine providing a 0V fixed regulator
via DT if it is wired to 0.

Given that is a little unusual, check it works!

> 
> > > ...
> > >
> > >  
> > > > >
> > > > > * (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.  
> 
> We can consider potential future use cases as they become relevant.
> For now, we might not to include or support this functionality.

The pins should be in the DT binding, though the driver support can of course
be a future exercise.  Make them optional.

> 
> > >
> > > ...
> > >  
> > > > >  
> > > > > > +
> > > > > > +      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
> >   
>