Message-ID: <20250927181341.58c106d3@jic23-huawei>
Date: Sat, 27 Sep 2025 18:13:41 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: David Lechner <dlechner@...libre.com>, Ariana Lazar 
 <ariana.lazar@...rochip.com>, Nuno Sá  <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, Rob Herring  <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley 
 <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: dac: adding support for Microchip MCP47FEB02

On Tue, 23 Sep 2025 09:21:30 +0100
Nuno Sá <noname.nuno@...il.com> wrote:

> On Mon, 2025-09-22 at 17:15 -0500, David Lechner wrote:
> > On 9/22/25 3:10 PM, Nuno Sá wrote:  
> > > Hi Ariana,
> > > 
> > > Thanks for your patches. Some initial comments from me...
> > > 
> > > On Mon, 2025-09-22 at 14:30 +0300, Ariana Lazar wrote:  
> > 
> > ...
> >   
> > > > +static IIO_DEVICE_ATTR(store_eeprom, 0200, NULL, mcp47feb02_store_eeprom,
> > > > 0);
> > > > +static struct attribute *mcp47feb02_attributes[] = {
> > > > +	&iio_dev_attr_store_eeprom.dev_attr.attr,
> > > > +	NULL,
> > > > +};
> > > > +  
> > > 
> > > Not going to argue about the ABI for now but I don't think this is a
> > > standard one? So
> > > if acceptable you need an ABI doc.

store_eeprom is existing ABI and documented in sysfs-bus-iio (2 drivers implement it from
a quick grep)


> > >   
> > Here's a random idea. (I would wait for Jonathan to weigh in first before
> > assuming it is an acceptable idea though :-p)
> > 
> > The config registers are pretty much going to be a one-time deal. So those
> > could be written to only if they need it during probe.
> > 
> > For the voltage output registers, we could add extra out_voltageY channels
> > that are the power-on output state channels. So writing to out_voltageY_raw
> > wouldn't change any real output but would just be written to EEPROM. This
> > way these voltages could be controlled independently from the real outputs
> > and it uses existing ABI.

In some devices I've come across, the eeprom write is a 'store all current settings'
rather than individual register writes. For that a set of extra channels doesn't work.

Also eeproms have very limited write cycles so you really don't want to make this
too easy to do and we want to shout it's an eeprom. 


> > 
> > In any case, it would be interesting to hear more about how this chips are
> > actually used to better understand this EEPROM feature.  
> 
> I didn't really looked at the datasheet so this can be totally wrong. But we
> have some LTC parts (mainly hwmon stuff) that are also packed with an EEPRON.
> AFAIU, the usecase in there is to have some defaults you can program in the
> chips (and there's a feature we can enable so the chip can save things into the
> eeprom automatically). Now, in those drivers we don't really support doing
> anything with the eeprom at runtime so I'm curious to see how this unfolds :)

Usecase for DACs is that on power on (usually at board power up not driver load)
they will start outputting the saved values.  That might well be part of something
fairly critical such as fan control or trip points so you don't want to wait for
the driver to load. The driver on an eeprom equiped part should not configure
anything on probe but rather just report back what was already there.

Userspace can then modify those values and 'commit' them via store_eeprom
to apply on next power cycle as well as now (in some cases only on next power cycle).

Jonathan


> 
> - Nuno Sá

Powered by blists - more mailing lists