Re: [PATCH] iio: frequency: ad9523: add eeprom read/write verification

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

On Sat, 18 May 2019 22:39:09 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, May 18, 2019 at 1:01 PM Jonathan Cameron
> <jic23@jic23.retrosnub.co.uk> wrote:
> >
> > On Fri, 17 May 2019 17:19:38 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >  
> > > From: Michael Hennerich <michael.hennerich@analog.com>
> > >
> > > This change adds a basic verification of the EEPROM by writing a known
> > > value to the customer version ID register, and reading it back.
> > >
> > > This validates that the EEPROM & SPI communication are functioning
> > > properly, and the device is ready to use.
> > >
> > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> >
> > I'm going to assume that the write cycle limitations of that eeprom
> > have been taken into account here and it won't be an issue until
> > a very large number of probe and remove cycles.  There is also the
> > slightly amusing possibility of breaking a customer part if someone
> > managed to pull the power whilst you have the wrong customer ID
> > in the eeprom.  However your device and I assume there is a customer
> > who really wants this sanity check so fair enough...
> >
> > Applied to the togreg branch of iio.git and pushed out as testing for
> > the autobuilders to play with it.  
> 
> I was also a bit unsure about this patch in this form.
> But now that you've raised these points, I am now somewhat paranoid.
> 
> I guess, I'll have to go back and start a internal discussion about
> this. Maybe it makes more sense to add a device[-tree] property to
> configure this somehow, and if someone really wants this behavior,
> he/she can enable it.
> This patch was also created some time ago [before I joined Analog] so
> there is some context I may be lacking here about it.
> 
> Maybe let's drop this, and I can come back with a version that would
> not allow users to shoot-them-selves-in-the-foot without a safety
> mechanism off.
> 
> Sorry for the noise.
Cool. Dropped for now then.

Jonathan

> Alex
> 
> >
> > Thanks,
> >
> > Jonathan
> >  
> > > ---
> > >  drivers/iio/frequency/ad9523.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/iio/frequency/ad9523.c b/drivers/iio/frequency/ad9523.c
> > > index 9b9eee27176c..dd159a1237f3 100644
> > > --- a/drivers/iio/frequency/ad9523.c
> > > +++ b/drivers/iio/frequency/ad9523.c
> > > @@ -749,6 +749,30 @@ static int ad9523_reg_access(struct iio_dev *indio_dev,
> > >       return ret;
> > >  }
> > >
> > > +static int ad9523_verify_eeprom(struct iio_dev *indio_dev)
> > > +{
> > > +     int ret, id;
> > > +
> > > +     id = ad9523_read(indio_dev, AD9523_EEPROM_CUSTOMER_VERSION_ID);
> > > +     if (id < 0)
> > > +             return id;
> > > +
> > > +     ret = ad9523_write(indio_dev, AD9523_EEPROM_CUSTOMER_VERSION_ID, 0xAD95);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = ad9523_read(indio_dev, AD9523_EEPROM_CUSTOMER_VERSION_ID);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     if (ret != 0xAD95) {
> > > +             dev_err(&indio_dev->dev, "SPI Read Verify failed (0x%X)\n", ret);
> > > +             return -EIO;
> > > +     }
> > > +
> > > +     return ad9523_write(indio_dev, AD9523_EEPROM_CUSTOMER_VERSION_ID, id);
> > > +}
> > > +
> > >  static const struct iio_info ad9523_info = {
> > >       .read_raw = &ad9523_read_raw,
> > >       .write_raw = &ad9523_write_raw,
> > > @@ -780,6 +804,10 @@ static int ad9523_setup(struct iio_dev *indio_dev)
> > >       if (ret < 0)
> > >               return ret;
> > >
> > > +     ret = ad9523_verify_eeprom(indio_dev);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > >       /*
> > >        * PLL1 Setup
> > >        */  
> >