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

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

[Alexandru Ardelean]

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>
---
 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
 	 */
-- 
2.17.1

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

[Jonathan Cameron]

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.

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
>  	 */

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

[Alexandru Ardelean]

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.
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
> >        */
>

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

[Jonathan Cameron]

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
> > >        */  
> >