Message-ID: 
 <SJ0PR03MB62242BD5117C5B2026CCC5D191D42@SJ0PR03MB6224.namprd03.prod.outlook.com>
Date: Mon, 24 Jun 2024 04:38:33 +0000
From: "Tinaco, Mariel" <Mariel.Tinaco@...log.com>
To: Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lars-Peter
 Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Liam Girdwood
	<lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
        "Hennerich, Michael"
	<Michael.Hennerich@...log.com>,
        Marcelo Schmitt <marcelo.schmitt1@...il.com>,
        Dimitri Fedrau <dima.fedrau@...il.com>,
        Guenter Roeck <linux@...ck-us.net>
Subject: RE: [PATCH 0/2] add AD8460 DAC driver



> -----Original Message-----
> From: Jonathan Cameron <jic23@...nel.org>
> Sent: Sunday, May 12, 2024 12:22 AM
> To: David Lechner <dlechner@...libre.com>
> Cc: Tinaco, Mariel <Mariel.Tinaco@...log.com>; linux-iio@...r.kernel.org;
> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org; Lars-Peter Clausen
> <lars@...afoo.de>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski
> <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; Liam Girdwood
> <lgirdwood@...il.com>; Mark Brown <broonie@...nel.org>; Hennerich,
> Michael <Michael.Hennerich@...log.com>; Marcelo Schmitt
> <marcelo.schmitt1@...il.com>; Dimitri Fedrau <dima.fedrau@...il.com>;
> Guenter Roeck <linux@...ck-us.net>
> Subject: Re: [PATCH 0/2] add AD8460 DAC driver
> 
> [External]
> 
> On Fri, 10 May 2024 12:30:46 -0500
> David Lechner <dlechner@...libre.com> wrote:
> 
> > On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@...log.com>
> wrote:
> > >
> > > The AD8460 is a 14-bit, high power +-40V 1A, high-speed DAC, with
> > > dual digital input modes, programmable supply current and fault
> > > monitoring and protection settings for output current, output
> > > voltage and junction temperature.
> > >
> > > The fault monitoring and shutdown protection features were supported
> > > in the earlier versions of the IIO driver but was scrapped due to
> > > uncertainties if the functionalities belong to the IIO driver.
> > > However, it would be best to implement it for the device's quality
> > > of life. I'd like to know if it's better suited as a stand-alone
> > > HWMON driver.
> 
> That probably doesn't make sense.  This reply btw is based on the text here. I
> haven't yet looked in detail at the datasheet.
> 
> Some fault conditions are relatively easy to map to threshold events on an input
> channel.  The ones that are harder are things like short circuit detectors where it's
> hard to know what the actual threshold is.

Upon checking the IIO Threshold events, the fault conditions can easily be mapped.
But I had to add Current and Temperature channels

> The other place we are limited is in multiple levels for the same thing.  So
> warning and fault levels. That stuff is handled much better by hwmon where
> these things are more common.
> The issue we have is that the event encoding is a bit tight but we could see if
> there is a way to add these.

Fortunately, there weren't multiple levels of fault monitoring as checked in the
datasheet

> > >
> > > The following are the configurable and readable parameters through
> > > SPI that could be implemented on the HWMON driver:
> > >   * An enable bit to arm/protect the device on overcurrent,
> > > overvoltage or overtemperature events. The device is shut down upon
> > > detection.
> 
> We don't have a good way to handle restarting from shutdown, but perhaps we
> could use another action to signal that - maybe even going as far as saying that
> the driver should be reloaded.
> 
> As for the thresholds, they sound like the map to IIO events reasonably well.
> 
> > >   * A configurable range/threshold for voltage, current and
> > > temperature that raises alarm when exceeded while the device is
> > > armed.
> 
> That maps fine to the IIO event threshold controls.
> 
> > >   * Flags that can be polled to raise alarm upon detection of
> > > overcurrent, overvoltage or overtemperature events, and apply
> > > additional protective measures.
> The specific need to poll is awkward but you could do it easily enough in driver
> and use the IIO event stuff to signal any events detected.
> 
> 
> > >   * Programmable quiescent current (optional)
> Could probably figure out a suitable control for this, but I'm not entirely sure
> what it is :)

Thinking about it, wouldn't the raw attribute be a suitable control for this? This 
Value is relative to nominal supply current and acts as a "monotonic but nonlinear"
multiplier. A register value maps to a current level from 0 to 2 times the nominal
current supplied. I also thought that it could be hardware gain but the gain
computation wasn't explicitly indicated in the datasheet and there is not yet
"current_hardwaregain" attribute available in the ABI. So I settled with raw. I
Think there would only be an issue of we expose the "processed" attribute
Because it has a particular computation. But I'm not planning to expose the 
Processed attribute

> > >   * Thermal monitoring is done by measuring voltage on TMP pin
> > > (unlikely to be included)
>
> If you did want to, the usual trick for these is to include an optional use as a
> consumer of an IIO provider which would be a separate ADC.

I included this in my current revision, thanks for the idea. Although the optional use
Isn’t yet available in the consumer API. My question is, in case the ADC channel to read
The TMP pin is not available, should I still make the temp raw value available and
Set to 0? Or should the temp raw attribute be unavailable or unlisted completely from
IIO Info.
> > >
> >
> > Adding myself to the cc: here since I'm interested to see what
> > Jonathan (or anyone else) has to say about the fault monitoring.
> 
> Jonathan

Powered by blists - more mailing lists