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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Aug 10, 2023 at 01:57:02PM +0200, Nuno Sá wrote:
> On Thu, 2023-08-10 at 12:33 +0300, Dumitru Ceclan wrote:

...

> Is ad717x_gpio_cleanup() being used anywhere? Moreover I would maybe just get rid of
> the #ifdef wrapper and just select GPIOLIB. How often will it be disabled anyways?

The agreement is that users are depend on and not selecting GPIOLIB.
Any news in these agreement terms?

...

> > +       id &= AD717X_ID_MASK;
> > +       if (id != st->info->id)
> > +               dev_warn(&st->sd.spi->dev, "Unexpected device id: %x, expected:
> > %x\n",
> > +                                           id, st->info->id);
> > +
> 
> Shouldn't we error out?

It seems a new way of thinking about unsupported CHIP ID. Dunno if hw vendors
won't ever do a dirty trick that new ID must be programmed differently and
otherwise burn hardware to a smoke...

I'm with you here, unknown chips mustn't be supported.

...

> > +                               *val = -(1 << (chan->scan_type.realbits - 1));
> 
> nit: I don't expect the driver to really be updated with more devices (it's
> like this for a long time) but the above is not very extensible... Imagine we
> add a device with 32bit channels? We would enter shady waters If I'm not
> missing anything.

Also 1 << 31 is UB in accordance with C standard.

...

> > +       st->info = device_get_match_data(&spi->dev);
> > +       if (!st->info)
> > +               return -ENODEV;
> > +
> 
> spi_get_device_match_data() (not really sure if this is still applicable
> since some work related to this is being done for i2c - and eventually in spi
> I guess).

Still applicable.

-- 
With Best Regards,
Andy Shevchenko