Received-SPF: pass (google.com: domain of linux-kernel+bounces-152224-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sat, 20 Apr 2024 11:13:22 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: "Paller, Kim Seer" <KimSeer.Paller@analog.com>
Cc: David Lechner <dlechner@baylibre.com>, "linux-iio@vger.kernel.org"
 <linux-iio@vger.kernel.org>, "devicetree@vger.kernel.org"
 <devicetree@vger.kernel.org>, "linux-kernel@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
Message-ID: <20240420111322.4fa026f3@jic23-huawei>
In-Reply-To: <PH0PR03MB714170F4494EA0E1B4112291F9082@PH0PR03MB7141.namprd03.prod.outlook.com>
References: <20240412032102.136071-1-kimseer.paller@analog.com>
	<20240412032102.136071-2-kimseer.paller@analog.com>
	<CAMknhBHgKK_OEcPz-5ktxj+YEkB7jHpw5owdh9HVj_qfwuVXkQ@mail.gmail.com>
	<20240413160610.4cec010b@jic23-huawei>
	<CAMknhBHMd2mK3yVoH_XjW7BapX5BTRZjUJpF=ZQrF8Mctf-NJQ@mail.gmail.com>
	<20240413181025.39d1a62e@jic23-huawei>
	<PH0PR03MB714170F4494EA0E1B4112291F9082@PH0PR03MB7141.namprd03.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

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 Cl=
ausen
> > <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
> >=20
> > [External]
> >=20
> > On Sat, 13 Apr 2024 11:21:55 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >  =20
> > > On Sat, Apr 13, 2024 at 10:06=E2=80=AFAM Jonathan Cameron <jic23@kern=
el.org> =20
> > wrote: =20
> > > >
> > > > On Fri, 12 Apr 2024 16:23:00 -0500
> > > > David Lechner <dlechner@baylibre.com> wrote:
> > > > =20
> > > > > On Thu, Apr 11, 2024 at 10:21=E2=80=AFPM Kim Seer Paller
> > > > > <kimseer.paller@analog.com> wrote: =20
> > > > > > =20
> > >
> > > ...
> > > =20
> > > > >
> > > > > And there is V~ on both which can be between -5.5V/-15.75V and GN=
D, so
> > > > > optional v-neg-supply seems appropriate. =20
> > > >
> > > > 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 in=
ternal
> > > > reference, so maybe it really is optional. =20
> > >
> > > I suggested optional with the thinking that if the pin is tied to GND,
> > > then the property would be omitted. =20
> >=20
> > 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 ;) =20
>=20
> To clarify, does this mean we should still add an optional property for i=
t in the binding?
I think it should not be optional.  Should be fine providing a 0V fixed reg=
ulator
via DT if it is wired to 0.

Given that is a little unusual, check it works!

>=20
> > > ...
> > >
> > > =20
> > > > >
> > > > > * (both) The MUX/MUXOUT pins look like we have an embedded pin mu=
x, =20
> > so =20
> > > > > it could mean we need #pinctrl-cells. ltc2664 would also need
> > > > > muxin-gpios for this. =20
> > > > 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 meas=
ure
> > > > 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.
> > > > =20
> > >
> > > 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. =20
> >=20
> > It might indeed.  But I think that's an exercise for the future if
> > it matters.  Might be a debugfs control only perhaps. =20
>=20
> 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 cour=
se
be a future exercise.  Make them optional.

>=20
> > >
> > > ...
> > > =20
> > > > > =20
> > > > > > +
> > > > > > +      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 sca=
le range. =20
> > > > >
> > > > > How would someone writing a .dts know what values to select for t=
his
> > > > > property? Or is this something that should be configured at runti=
me
> > > > > instead of in the devicetree? Or should this info come from the
> > > > > missing voltage supplies I mentioned? =20
> > > >
> > > > Sometimes this one is a wiring related choice.  Sometimes to the ex=
tent
> > > > that picking the wrong one from any userspace control can cause dam=
age
> > > > 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 ar=
en't
> > > > tied to all high.  So I'd imagine if the board can be damaged it wi=
ll
> > > > 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.
> > > > =20
> > >
> > > 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. =20
> > Agreed - it needs more detail.
> >=20
> > Jonathan
> >  =20
>=20