Re: [PATCH resend] iio: dac: add support for max5522

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 2 Oct 2022 22:32:59 +0200
Angelo Dureghello <angelo.dureghello@timesys.com> wrote:

> Hi Jonathan,
> 
> thanks a lot for all the feedbacks,
> 
> On Sun, Oct 2, 2022 at 1:56 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun,  2 Oct 2022 00:12:25 +0200
> > Angelo Dureghello <angelo.dureghello@timesys.com> wrote:
> >  
> > > Add initial support for dac max5522.  
> >
> > DAC preferred for comments.
> >  
> ok, fixed
Hi Angelo,

To cut down a little on reading time, it's helpful to crop out anything
where you agree.  That way we can focus in on remaining questions with
less scrolling!

...

> > > +config MAX5522
> > > +     tristate "Maxim MAX5522 DAC driver"
> > > +     depends on SPI  
> > Hmm. We only have one instance of the pattern and that's more complex because
> > it's a driver that supports both SPI and I2C. Simpler to have (unless I'm missing
> > something!)
> >
> >         depends on SPI_MASTER
> >         select REGMAP_SPI
> >  
> Not sure i am understanding this point, device is SPI only.
> Anyway, ok, i changed as you are suggesting.

Ah I was less clear than I could have been.
You have the depends refering to SPI, and the select
based on the more specific SPI_MASTER.

There is some SPI code that will be built with SPI that is not sufficient here,
so we should depend on SPI_MASTER.

The driver only supports SPI, thus we can unconditionally select REGMAP_SPI if
we are building it at all.

I was trying to figure out where you found this pattern on assumption it was
copied from existing code and only similar example supported multiple buses
and hence needed more complex logic that wasn't relevant here.

> 
> >  
> > > +     select REGMAP_SPI if SPI_MASTER
> > > +     help
> > > +       Say Y here if you want to build a driver for the Maxinm MAX5522.  


...
> > > +struct max5522_state {
> > > +     struct regmap *regmap;
> > > +     const struct max5522_chip_info *chip_info;
> > > +     unsigned short dac_cache[2];
> > > +     unsigned int vrefin_mv;  
> >
> > In theory voltages can change and sensible userspace software will only read them
> > in a slow path anyway, so I'd just move the voltage readback into the
> > read_raw() callback and drop this cache of the value.
> >  
> Sorry, not clear. This device does not provide read operations.
> There is only write operation and DIN spi pin.

The regulator supplying the reference voltage is queried for the reference.
That is currently done at probe() then cached.  You could do it later when
calling read_raw() to get the scale - that would avoid need to cache it
within the driver and handle later reference voltage changes.

Reading scale should never be a latency sensitive path, so no need to
cache the value when we can read it directly when we need it.


> > > +
> > > +enum max5522_type {
> > > +     ID_MAX5522,
> > > +};
> > > +
> > > +static const struct max5522_chip_info max5522_chip_info_tbl[] = {  
> >
> > Unless you are going to follow this patch very soon with support for more devices,
> > I'd prefer seeing this indirection only when it becomes necessary.
> > For now, it just leads to less readable and longer code.
> >  
> idea is to follow up with MAX5523/5524/5525,
> not sure when right now, since i cannot test them, but code was ready
> for addition

Great if you do or if anyone else can test those parts and hence
help you get these upstream. (Nuno:  Not sure how combine things are between
different parts of ADI family, but are these parts you have access to?)

If they are very similar then I'd be fine taking support tested against some
unit tests or emulation.  For some of these families we only ever manage
to test a subset when developing.

Add a note to the patch description to say that these are on their way
as reasoning behind the flexibility.

> 
> > > +     [ID_MAX5522] = {
> > > +             .channels = max5522_channels,
> > > +             .num_channels = 2,
> > > +     },
> > > +};

> > > +     indio_dev->info = &max5522_info;
> > > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > > +     indio_dev->channels = max5522_channels;
> > > +     indio_dev->num_channels = ARRAY_SIZE(max5522_channels);
> > > +     indio_dev->name = id->name;  
> >
> > Hard code the name preferred as it makes it easier to be sure it's exactly what
> > we expect when reading the code and does rely on the fallback compatible matching
> > in the spi core for dt described devices.
> >  
> Ok, if possible i would keep the id table for next additions.

Keep the id table. Just don't get the name from it. In fact we have to keep the
ID table because it's used for module alias generation needed to autoload
modules.


Instead get the name from the chip_info_tbl[] so that you definitely get what
you expect independent of the probe mechanism.

Jonathan