Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 10 Aug 2023 13:57:02 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> Hi Dumitru,
> 
> Thanks for taking care of this driver which is out of tree for a long time... Some
> comments below.
Hi.

A few follow ups...


> > +static int ad717x_setup(struct iio_dev *indio_dev)
> > +{
> > +       struct ad717x_state *st = iio_priv(indio_dev);
> > +       unsigned int id;
> > +       u8 *buf;
> > +       int ret;
> > +
> > +       /* reset the serial interface */
> > +       buf = kcalloc(8, sizeof(*buf), GFP_KERNEL);
> > +       if (!buf)
> > +               return -ENOMEM;
> > +
> > +       memset(buf, 0xff, 8);
> > +       ret = spi_write(st->sd.spi, buf, 8);
> > +       kfree(buf);  
> 
> Hmm, why allocating the buffer? I guess one could argue that we'll get DMA safe
> alignment but then maybe use some define instead of the magic 8.
> 

Just use spi_write_then_read() without an read buffer as then it will use
magic bounce buffers for you within the spi subsystem.



> > +static struct spi_driver ad717x_driver = {
> > +       .driver = {
> > +               .name   = "ad717x",  
> 
> I would keep the name as we have out of tree which is ad7173.c. I'm not sure if
> there's any policy in here but I think typically you just name your driver from the
> first supported device and then extend it from there. Since here you are just adding
> more than one device at once, it would be nice if you could keep the name of the
> driver Lars originally developed...

In this case, indeed the one originally developed is a good choice.
Otherwise, pick a supported part.

Wild cards have bitten us far too many times as manufacturers love to
'use up' gaps in their ID space with parts that are either very different
or even worse look the same but have totally different interfaces...


>