Date:   Wed, 22 Jul 2020 14:42:11 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Alexandru Ardelean <ardeleanalex@...il.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iio: adc: ad7124: move chip ID & name on the chip_info
 table

On Wed, 22 Jul 2020 10:07:19 +0300
Alexandru Ardelean <ardeleanalex@...il.com> wrote:

> On Tue, Jul 21, 2020 at 9:41 PM Andy Shevchenko
> <andy.shevchenko@...il.com> wrote:
> >
> > On Mon, Jul 20, 2020 at 2:30 PM Jonathan Cameron <jic23@...nel.org> wrote:  
> > >
> > > On Tue, 14 Jul 2020 08:11:11 +0300
> > > Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
> > >  
> > > > This change does the following:
> > > > * removes the SPI device table in favor of the OF device table
> > > > * adds 'name' && 'chip_id' fields to chip_info
> > > > * implements chip ID & silicon revision checking; the device ID for
> > > >   AD7124-4 is 0x0, so just checking that value can be useless;
> > > >   but at least the silicon revision isn't 0, so a non-zero value can be
> > > >   used to check that "a" device is on the SPI bus; it's probably the best
> > > >   way to narrow it down to one of the 2 AD7124 chip IDs
> > > >
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>  
> > > Nice patch.  
> >
> > Doesn't this patch bring an opposite to what you are trying to clean up, i.e.
> > of_get_match_data(), of_device.h?  
> 
> A bit.
> I usually want to try to make things both OF & ACPI friendly, but I
> mostly have OF devices to test with and that creates a bit of habit to
> go for OF-first.
> I'll hopefully get to do more of these [OF & ACPI friendly] cleanups
> at some point.
> But until then, I have a list of IIO framework changes to do with
> regards to output-buffer support.
> And I noticed that if I look at trying to do too many things at once I
> end up fixing bugs like this:
> https://patchwork.kernel.org/patch/11675339/

To Andy's question, Yes it would be nice to clean this one up as well, but the driver has a
lot of of specific calls already.  I'd rather we cleaned them out in one
go at some point.  So it's not a 'bad' step as such, just a parallel
change to what we are looking to do in that work.

Jonathan


> 
> >  
> > > Applied to the togreg branch of iio.git and pushed out as testing for
> > > the auto builders to play with it.
> > >
> > > At somepoint we could think about moving this driver over to generic
> > > fw interfaces, but that is a bigger job and no particular reason to
> > > do that now (if it's even possible!)
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >  
> > > > ---
> > > >  drivers/iio/adc/ad7124.c | 73 ++++++++++++++++++++++++++++++++--------
> > > >  1 file changed, 59 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> > > > index bb53ab265b64..8dce06e9e69c 100644
> > > > --- a/drivers/iio/adc/ad7124.c
> > > > +++ b/drivers/iio/adc/ad7124.c
> > > > @@ -12,6 +12,7 @@
> > > >  #include <linux/interrupt.h>
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/of_device.h>
> > > >  #include <linux/regulator/consumer.h>
> > > >  #include <linux/spi/spi.h>
> > > >
> > > > @@ -47,6 +48,15 @@
> > > >  #define AD7124_ADC_CTRL_MODE_MSK     GENMASK(5, 2)
> > > >  #define AD7124_ADC_CTRL_MODE(x)      FIELD_PREP(AD7124_ADC_CTRL_MODE_MSK, x)
> > > >
> > > > +/* AD7124 ID */
> > > > +#define AD7124_DEVICE_ID_MSK         GENMASK(7, 4)
> > > > +#define AD7124_DEVICE_ID_GET(x)              FIELD_GET(AD7124_DEVICE_ID_MSK, x)
> > > > +#define AD7124_SILICON_REV_MSK               GENMASK(3, 0)
> > > > +#define AD7124_SILICON_REV_GET(x)    FIELD_GET(AD7124_SILICON_REV_MSK, x)
> > > > +
> > > > +#define CHIPID_AD7124_4                      0x0
> > > > +#define CHIPID_AD7124_8                      0x1
> > > > +
> > > >  /* AD7124_CHANNEL_X */
> > > >  #define AD7124_CHANNEL_EN_MSK                BIT(15)
> > > >  #define AD7124_CHANNEL_EN(x)         FIELD_PREP(AD7124_CHANNEL_EN_MSK, x)
> > > > @@ -120,6 +130,8 @@ static const char * const ad7124_ref_names[] = {
> > > >  };
> > > >
> > > >  struct ad7124_chip_info {
> > > > +     const char *name;
> > > > +     unsigned int chip_id;
> > > >       unsigned int num_inputs;
> > > >  };
> > > >
> > > > @@ -165,9 +177,13 @@ static const struct iio_chan_spec ad7124_channel_template = {
> > > >
> > > >  static struct ad7124_chip_info ad7124_chip_info_tbl[] = {
> > > >       [ID_AD7124_4] = {
> > > > +             .name = "ad7127-4",
> > > > +             .chip_id = CHIPID_AD7124_4,
> > > >               .num_inputs = 8,
> > > >       },
> > > >       [ID_AD7124_8] = {
> > > > +             .name = "ad7127-8",
> > > > +             .chip_id = CHIPID_AD7124_8,
> > > >               .num_inputs = 16,
> > > >       },
> > > >  };
> > > > @@ -503,6 +519,34 @@ static int ad7124_soft_reset(struct ad7124_state *st)
> > > >       return -EIO;
> > > >  }
> > > >
> > > > +static int ad7124_check_chip_id(struct ad7124_state *st)
> > > > +{
> > > > +     unsigned int readval, chip_id, silicon_rev;
> > > > +     int ret;
> > > > +
> > > > +     ret = ad_sd_read_reg(&st->sd, AD7124_ID, 1, &readval);
> > > > +     if (ret < 0)
> > > > +             return ret;
> > > > +
> > > > +     chip_id = AD7124_DEVICE_ID_GET(readval);
> > > > +     silicon_rev = AD7124_SILICON_REV_GET(readval);
> > > > +
> > > > +     if (chip_id != st->chip_info->chip_id) {
> > > > +             dev_err(&st->sd.spi->dev,
> > > > +                     "Chip ID mismatch: expected %u, got %u\n",
> > > > +                     st->chip_info->chip_id, chip_id);
> > > > +             return -ENODEV;
> > > > +     }
> > > > +
> > > > +     if (silicon_rev == 0) {
> > > > +             dev_err(&st->sd.spi->dev,
> > > > +                     "Silicon revision empty. Chip may not be present\n");
> > > > +             return -ENODEV;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  static int ad7124_init_channel_vref(struct ad7124_state *st,
> > > >                                   unsigned int channel_number)
> > > >  {
> > > > @@ -665,25 +709,28 @@ static int ad7124_setup(struct ad7124_state *st)
> > > >
> > > >  static int ad7124_probe(struct spi_device *spi)
> > > >  {
> > > > -     const struct spi_device_id *id;
> > > > +     const struct ad7124_chip_info *info;
> > > >       struct ad7124_state *st;
> > > >       struct iio_dev *indio_dev;
> > > >       int i, ret;
> > > >
> > > > +     info = of_device_get_match_data(&spi->dev);
> > > > +     if (!info)
> > > > +             return -ENODEV;
> > > > +
> > > >       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > > >       if (!indio_dev)
> > > >               return -ENOMEM;
> > > >
> > > >       st = iio_priv(indio_dev);
> > > >
> > > > -     id = spi_get_device_id(spi);
> > > > -     st->chip_info = &ad7124_chip_info_tbl[id->driver_data];
> > > > +     st->chip_info = info;
> > > >
> > > >       ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
> > > >
> > > >       spi_set_drvdata(spi, indio_dev);
> > > >
> > > > -     indio_dev->name = spi_get_device_id(spi)->name;
> > > > +     indio_dev->name = st->chip_info->name;
> > > >       indio_dev->modes = INDIO_DIRECT_MODE;
> > > >       indio_dev->info = &ad7124_info;
> > > >
> > > > @@ -721,6 +768,10 @@ static int ad7124_probe(struct spi_device *spi)
> > > >       if (ret < 0)
> > > >               goto error_clk_disable_unprepare;
> > > >
> > > > +     ret = ad7124_check_chip_id(st);
> > > > +     if (ret)
> > > > +             goto error_clk_disable_unprepare;
> > > > +
> > > >       ret = ad7124_setup(st);
> > > >       if (ret < 0)
> > > >               goto error_clk_disable_unprepare;
> > > > @@ -768,16 +819,11 @@ static int ad7124_remove(struct spi_device *spi)
> > > >       return 0;
> > > >  }
> > > >
> > > > -static const struct spi_device_id ad7124_id_table[] = {
> > > > -     { "ad7124-4", ID_AD7124_4 },
> > > > -     { "ad7124-8", ID_AD7124_8 },
> > > > -     {}
> > > > -};
> > > > -MODULE_DEVICE_TABLE(spi, ad7124_id_table);
> > > > -
> > > >  static const struct of_device_id ad7124_of_match[] = {
> > > > -     { .compatible = "adi,ad7124-4" },
> > > > -     { .compatible = "adi,ad7124-8" },
> > > > +     { .compatible = "adi,ad7124-4",
> > > > +             .data = &ad7124_chip_info_tbl[ID_AD7124_4], },
> > > > +     { .compatible = "adi,ad7124-8",
> > > > +             .data = &ad7124_chip_info_tbl[ID_AD7124_8], },
> > > >       { },
> > > >  };
> > > >  MODULE_DEVICE_TABLE(of, ad7124_of_match);
> > > > @@ -789,7 +835,6 @@ static struct spi_driver ad71124_driver = {
> > > >       },
> > > >       .probe = ad7124_probe,
> > > >       .remove = ad7124_remove,
> > > > -     .id_table = ad7124_id_table,
> > > >  };
> > > >  module_spi_driver(ad71124_driver);
> > > >  
> > >  
> >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko  

Powered by blists - more mailing lists