On Mon, 30 Sep 2024 04:28:09 +0000
"Tinaco, Mariel" <Mariel.Tinaco@xxxxxxxxxx> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@xxxxxxxxxx>
> > Sent: Saturday, September 28, 2024 10:20 PM
> > To: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> > Cc: Tinaco, Mariel <Mariel.Tinaco@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Lars-Peter Clausen
> > <lars@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> > <krzk+dt@xxxxxxxxxx>; Hennerich, Michael
> > <Michael.Hennerich@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>;
> > Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>; Dimitri Fedrau
> > <dima.fedrau@xxxxxxxxx>; David Lechner <dlechner@xxxxxxxxxxxx>; Nuno Sá
> > <noname.nuno@xxxxxxxxx>
> > Subject: Re: [PATCH v4 2/2] iio: dac: support the ad8460 Waveform DAC
> >
> > [External]
> >
> > On Sat, 14 Sep 2024 20:21:56 +0200
> > Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:
> >
> > > Le 12/09/2024 à 11:54, Mariel Tinaco a écrit :
> > > > The AD8460 is a “bits in, power out” high voltage, high-power,
> > > > high-speed driver optimized for large output current (up to ±1 A)
> > > > and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V)
> > > > into capacitive loads.
> > > >
> > > > A digital engine implements user-configurable features: modes for
> > > > digital input, programmable supply current, and fault monitoring and
> > > > programmable protection settings for output current, output voltage,
> > > > and junction temperature. The AD8460 operates on high voltage dual
> > > > supplies up to ±55 V and a single low voltage supply of 5 V.
> > > >
> > > > Signed-off-by: Mariel Tinaco
> > > > <Mariel.Tinaco-OyLXuOCK7orQT0dZR+AlfA@xxxxxxxxxxxxxxxx>
> > > > ---
> >
> > Rather than go around again, I fixed up all the comments made and the
> > autobuilder issues then applied this.
> >
> > Diff follows. The only bit I'm not 100% sure on was your intent with the
> > temperature channel. I've made it an input but shout if I'm missing something.
> >
> > With this diff applied on top, applied to the togreg branch of iio.git which is
> > only pushed out as testing for now as I'll rebase on rc1 once available.
> >
> > Thanks,
> >
> > Jonathan
> >
>
> Hi Jonathan,
>
> Thank you for finding the time to fix up the inline comments from the
> previous rounds! I have created a patch for that but was unable to send it
> yet because I'm still clueless about the temp channel. Apologies about that
>
> About the temperature channel, it does make sense to set it as input since the
> value is read-only.
Input means that we are measuring a signal from the outside world. As you can
read the temperature it should be an input (doesn't matter that it comes from
elsewhere - in this case an ADC channel is providing the service of reading
that voltage for us).
> About the implementation of the channel, I'm wondering
> what would happen if the consumer-get-channel would throw -EPROBE_DEFER
> half the time?
It will return -EPROBE_DEFER, it then later the driver providing the ADC channel will probe
and we will go around again with it succeeding. Roughly speaking every
time a driver is successfully bound (probe finishes) the kernel tries again
for any deferred instances.
The only nasty corner is the DT supplies the channel but we don't have the
driver for the ADC built. I'd argue that is a kernel miss configuration
where the right approach is to fix that and provide the ADC driver.
> Is it not possible to skip it over since the channel is optional
> anyway? Or does this defer error from the consumer mean that the other
> configurations for the succeeding attributes will be blocked, which is why we
> have to return probe instantly?
If we don't defer then we never try again and succeed.
We could skip it but that would effectively be so unreliable we would be
better off not providing that feature at all as it will be the source
of lots of bug reports.
>
> state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460-tmp");
> if (IS_ERR(state->tmp_adc_channel)) {
> state->tmp_adc_channel = NULL;
> indio_dev->channels = ad8460_channels;
> indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> } else {
> ret = iio_get_channel_type(state->tmp_adc_channel, &temp);
> if (ret < 0)
> return ret;
>
> if (temp != IIO_TEMP)
> return dev_err_probe(dev, -EINVAL,
> "Incompatible channel type %d\n", temp);
>
> indio_dev->channels = ad8460_channels_with_tmp_adc;
> indio_dev->num_channels = ARRAY_SIZE(ad8460_channels_with_tmp_adc);
> }
>
> I also found other implementations where the type of channel is checked. Thought
> That maybe it's a good addition for security.
in this case it would be a DT bug on a very simple binding so I'm not sure we care.
It is also very unlikely to be a temperature channel given we need to read the voltage
from this chips output pin
Jonathan
>
> Thanks,
>
> Mariel
