Re: [PATCH 2/2] iio: dac: adding support for Microchip MCP47FEB02

["Nuno Sá" <noname.nuno@gmail.com>]

On Sat, 2025-09-27 at 18:13 +0100, Jonathan Cameron wrote:
> On Tue, 23 Sep 2025 09:21:30 +0100
> Nuno Sá <noname.nuno@gmail.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)
> 

Ack. Next time I should bother in grepping the docs :)

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

Makes sense and it is a similar case of we have in hwmon given those LTC chips are
often controlling power of some fundamental blocks of the system.

- Nuno Sá
> 
> Jonathan
> 
> 
> > 
> > - Nuno Sá
>