Re: [PATCH 2/4] iio: imu: adis: Refactor adis_initial_startup

[Jonathan Cameron]

On Mon, 3 Feb 2020 10:31:30 +0100
Nuno Sá <noname.nuno@xxxxxxxxx> wrote:

> Hi Jonathan,
> 
> 
> On Sat, 2020-02-01 at 17:08 +0000, Jonathan Cameron wrote:
> > On Mon, 20 Jan 2020 16:20:49 +0200
> > Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
> >   
> > > From: Nuno Sá <nuno.sa@xxxxxxxxxx>
> > > 
> > > All the ADIS devices perform, at the beginning, a self test to make
> > > sure
> > > the device is in a sane state. Furthermore, some drivers also do a
> > > call
> > > to `adis_reset()` before the test which is also a good practice.
> > > This
> > > patch unifies all those operation so that, there's no need for code
> > > duplication. Furthermore, the rst pin is also checked to make sure
> > > the
> > > device is not in HW reset. On top of this, some drivers also read
> > > the
> > > device product id and compare it with the device being probed to
> > > make
> > > sure the correct device is being handled. This can also be passed
> > > to the
> > > library by introducing a variable holding the PROD_ID register of
> > > the
> > > device.
> > > 
> > > Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> > > ---
> > >  drivers/iio/imu/Kconfig      |  1 +
> > >  drivers/iio/imu/adis.c       | 63 ++++++++++++++++++++++++++----
> > > ------
> > >  include/linux/iio/imu/adis.h | 15 ++++++++-
> > >  3 files changed, 61 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
> > > index 60bb1029e759..63036cf473c7 100644
> > > --- a/drivers/iio/imu/Kconfig
> > > +++ b/drivers/iio/imu/Kconfig
> > > @@ -85,6 +85,7 @@ endmenu
> > >  
> > >  config IIO_ADIS_LIB
> > >  	tristate
> > > +	depends on GPIOLIB
> > >  	help
> > >  	  A set of IO helper functions for the Analog Devices ADIS*
> > > device family.
> > >  
> > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > > index d02b1911b0f2..1eca5271380e 100644
> > > --- a/drivers/iio/imu/adis.c
> > > +++ b/drivers/iio/imu/adis.c
> > > @@ -7,6 +7,7 @@
> > >   */
> > >  
> > >  #include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > >  #include <linux/mutex.h>
> > >  #include <linux/device.h>
> > >  #include <linux/kernel.h>
> > > @@ -365,36 +366,64 @@ static int adis_self_test(struct adis *adis)
> > >  }
> > >  
> > >  /**
> > > - * adis_inital_startup() - Performs device self-test
> > > + * __adis_initial_startup() - Device initial setup
> > >   * @adis: The adis device
> > >   *
> > > + * This functions makes sure the device is not in reset, via rst
> > > pin.
> > > + * Furthermore it performs a SW reset (only in the case we are not
> > > coming from
> > > + * reset already) and a self test. It also compares the product id
> > > with the
> > > + * device id if the prod_id_reg variable is set.
> > > + *
> > >   * Returns 0 if the device is operational, a negative error code
> > > otherwise.
> > >   *
> > >   * This function should be called early on in the device
> > > initialization sequence
> > >   * to ensure that the device is in a sane and known state and that
> > > it is usable.
> > >   */
> > > -int adis_initial_startup(struct adis *adis)
> > > +int __adis_initial_startup(struct adis *adis)
> > >  {
> > >  	int ret;
> > > -
> > > -	mutex_lock(&adis->state_lock);
> > > +	struct gpio_desc *gpio;
> > > +	const struct adis_timeout *timeouts = adis->data->timeouts;
> > > +	const char *iio_name = spi_get_device_id(adis->spi)->name;
> > > +	u16 prod_id, dev_id;
> > > +
> > > +	/* check if the device has rst pin low */
> > > +	gpio = devm_gpiod_get_optional(&adis->spi->dev, "reset",
> > > GPIOD_ASIS);
> > > +	if (IS_ERR(gpio)) {
> > > +		return PTR_ERR(gpio);  
> > 
> > Given you are returning here, no need for else to follow
> > 
> > if (gpio...
> >   
> 
> Definitely...
> 
> > > +	} else if (gpio && gpiod_get_value_cansleep(gpio)) {
> > > +		/* bring device out of reset */
> > > +		gpiod_set_value_cansleep(gpio, 0);  
> > 
> > Hmm. So is a software reset the best option if we have a hardware
> > reset
> > line but it's not currently in the reset mode?
> >   
> 
> Hmm, that's a fair question. Now that I think about it, if we do have a
> gpio we should just assume it's in reset and call
> `gpiod_set_value_cansleep`. So, I guess we could just ditch the
> `gpiod_get_value_cansleep(gpio)` part.

Not sure I agree.   For example the driver may well have been unbound
and rebound for some reason.

I would argue you should just do a set / reset cycle with appropriate sleep
in between.  If it's already set then no harm done, if it isn't you force
a hardware reset.

> 
> > > +		msleep(timeouts->reset_ms);
> > > +	} else {
> > > +		ret = __adis_reset(adis);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > >  
> > >  	ret = adis_self_test(adis);
> > > -	if (ret) {
> > > -		dev_err(&adis->spi->dev, "Self-test failed, trying
> > > reset.\n");
> > > -		__adis_reset(adis);
> > > -		ret = adis_self_test(adis);
> > > -		if (ret) {
> > > -			dev_err(&adis->spi->dev, "Second self-test
> > > failed, giving up.\n");
> > > -			goto out_unlock;
> > > -		}
> > > -	}
> > > +	if (ret)
> > > +		return ret;
> > >  
> > > -out_unlock:
> > > -	mutex_unlock(&adis->state_lock);
> > > -	return ret;
> > > +	if (!adis->data->prod_id_reg)
> > > +		return 0;
> > > +
> > > +	ret = adis_read_reg_16(adis, adis->data->prod_id_reg,
> > > &prod_id);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = sscanf(iio_name, "adis%hu\n", &dev_id);  
> > 
> > Hmm. I have a general dislike of pulling part name strings apart to
> > get
> > IDs.  It tends to break when someone comes along and adds a part with
> > new
> > branding.  Perhaps just put it in the relevant device part specific
> > structures
> > directly?
> >   
> 
> I'll admit that this to orientated to ADI devices and I basically just
> took what all the drivers were doing and placed it inside the
> library...
> 
> So, you mean passing this to each `chip_info` and then passing it to
> the library through `adis_data`?

Yes.  People don't tend to expect strings to need to take a particular form,
so pulling them apart in a library can give unexpected results...

> > > +	if (ret != 1)
> > > +		return -EINVAL;
> > > +
> > > +	if (prod_id != dev_id)
> > > +		dev_warn(&adis->spi->dev,
> > > +			 "Device ID(%u) and product ID(%u) do not
> > > match.",
> > > +			 dev_id, prod_id);
> > > +
> > > +	return 0;
> > >  }
> > > -EXPORT_SYMBOL_GPL(adis_initial_startup);
> > > +EXPORT_SYMBOL_GPL(__adis_initial_startup);
> > >  
> > >  /**
> > >   * adis_single_conversion() - Performs a single sample conversion
> > > diff --git a/include/linux/iio/imu/adis.h
> > > b/include/linux/iio/imu/adis.h
> > > index d21a013d1122..c43e7922ab32 100644
> > > --- a/include/linux/iio/imu/adis.h
> > > +++ b/include/linux/iio/imu/adis.h
> > > @@ -41,6 +41,7 @@ struct adis_timeout {
> > >   * @glob_cmd_reg: Register address of the GLOB_CMD register
> > >   * @msc_ctrl_reg: Register address of the MSC_CTRL register
> > >   * @diag_stat_reg: Register address of the DIAG_STAT register
> > > + * @prod_id_reg: Register address of the PROD_ID register
> > >   * @self_test_reg: Register address to request self test command
> > >   * @status_error_msgs: Array of error messgaes
> > >   * @status_error_mask:
> > > @@ -54,6 +55,7 @@ struct adis_data {
> > >  	unsigned int glob_cmd_reg;
> > >  	unsigned int msc_ctrl_reg;
> > >  	unsigned int diag_stat_reg;
> > > +	unsigned int prod_id_reg;
> > >  
> > >  	unsigned int self_test_mask;
> > >  	unsigned int self_test_reg;
> > > @@ -299,6 +301,7 @@ static inline int adis_read_reg_32(struct adis
> > > *adis, unsigned int reg,
> > >  
> > >  int adis_enable_irq(struct adis *adis, bool enable);
> > >  int __adis_check_status(struct adis *adis);
> > > +int __adis_initial_startup(struct adis *adis);
> > >  
> > >  static inline int adis_check_status(struct adis *adis)
> > >  {
> > > @@ -311,7 +314,17 @@ static inline int adis_check_status(struct
> > > adis *adis)
> > >  	return ret;
> > >  }
> > >  
> > > -int adis_initial_startup(struct adis *adis);
> > > +/* locked version of __adis_initial_startup() */
> > > +static inline int adis_initial_startup(struct adis *adis)
> > > +{
> > > +	int ret;
> > > +
> > > +	mutex_lock(&adis->state_lock);
> > > +	ret = __adis_initial_startup(adis);
> > > +	mutex_unlock(&adis->state_lock);
> > > +
> > > +	return ret;
> > > +}
> > >  
> > >  int adis_single_conversion(struct iio_dev *indio_dev,
> > >  	const struct iio_chan_spec *chan, unsigned int error_mask,  
> > 
> >   
>