Message-ID: <20200206112829.4aef53f3@archlinux>
Date:   Thu, 6 Feb 2020 11:28:29 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Sa, Nuno" <Nuno.Sa@...log.com>
Cc:     "Jonathan.Cameron@...wei.com" <Jonathan.Cameron@...wei.com>,
        "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>,
        "Bogdan, Dragos" <Dragos.Bogdan@...log.com>
Subject: Re: [PATCH 2/4] iio: imu: adis: Refactor adis_initial_startup

On Thu, 6 Feb 2020 10:19:09 +0000
"Sa, Nuno" <Nuno.Sa@...log.com> wrote:

> On Thu, 2020-02-06 at 09:45 +0000, Jonathan Cameron wrote:
> > On Wed, 5 Feb 2020 16:44:13 +0000
> > "Sa, Nuno" <Nuno.Sa@...log.com> wrote:
> >   
> > > On Wed, 2020-02-05 at 14:59 +0000, Jonathan Cameron wrote:  
> > > > On Wed, 5 Feb 2020 12:25:40 +0000
> > > > "Sa, Nuno" <Nuno.Sa@...log.com> wrote:
> > > >     
> > > > > On Mon, 2020-02-03 at 12:03 +0000, Jonathan Cameron wrote:    
> > > > > > On Mon, 3 Feb 2020 10:31:30 +0100
> > > > > > Nuno Sá <noname.nuno@...il.com> 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@...log.com> wrote:
> > > > > > > >         
> > > > > > > > > From: Nuno Sá <nuno.sa@...log.com>
> > > > > > > > > 
> > > > > > > > > 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@...log.com>
> > > > > > > > > Signed-off-by: Alexandru Ardelean <      
> > > > > > > > > alexandru.ardelean@...log.com>      
> > > > > > > > > ---
> > > > > > > > >  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.      
> > > > > 
> > > > > Yes, that is true..
> > > > >     
> > > > > > 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.      
> > > > > 
> > > > > So, As Im understanding, it comes down to what should we
> > > > > consider
> > > > > as
> > > > > default. You suggest to first do the sw reset and the check the
> > > > > gpio
> > > > > state and if needed, bring the device out of reset, right? Now
> > > > > that
> > > > > I
> > > > > think about it, I think the only reason I haven't done like
> > > > > that is
> > > > > because you might end up sleeping quite some time (sw reset +
> > > > > hw
> > > > > reset). Either way, I'm fine with both options. Not sure if
> > > > > Alex
> > > > > has
> > > > > something to add...    
> > > > 
> > > > Either reset should be good on it's own.  I would use hardware
> > > > reset
> > > > if the pin is there. If it's not, then use the software reset.
> > > > 
> > > > For hardware always set the pin explicitly to reset as that
> > > > guarantees
> > > > against any race conditions, even if something odd happens.
> > > > 
> > > > Jonathan    
> > > 
> > > Hmm, I think I'm not getting the point or maybe I failed to explain
> > > what I was doing... So, on the code in this patch, there's no HW
> > > reset.
> > > It checks the reset pin and sees if the part is in reset and, if it
> > > is,
> > > it brings it out of reset. In that case, no need for sw reset since
> > > we
> > > are coming already from reset. On the other hand, if there's no
> > > reset
> > > pin configured or the part is already powered, then I was forcing a
> > > sw
> > > reset to guarantee a sane state when starting...  
> > 
> > Agreed that is what your patch is doing.   However I'm suggesting you
> > do
> > something different.  Decide which type of reset takes precedence.
> > Normally that is hardware reset if it is wired up, but there is no
> > particular reason it can't be the software reset if they do exactly
> > the
> > same thing.
> > 
> > Plan a,
> > If you decide the software takes precedence you need to check if the
> > hardware reset is already set.  If so you need to release it and
> > carry on.
> > If hardware reset is not provided or not set then you just call the
> > software
> > reset.
> > 
> > Plan b, which is the most common one for drivers IIRC...
> > If you decide to make the hardware reset take precedence and it's
> > there, then you
> > 'always' set the pin for appropriate time to trigger a reset.  You
> > don't
> > care what state it was previously in as either it's already in reset
> > in which
> > case you are making no change, or not in which case you enter reset.
> > Note this also works if you have an output only pin and no access to
> > what
> > its current state is (typically because it was set by firmware).
> > 
> > Then you raise the gpio to take it out of reset and move on.  If the
> > reset is not provided then you fall back to the software reset.  
> 
> Ok, Now I believe I got it. If Im understanding correctly, what is
> happening right know is plan A. So, on plan B, we actually force the HW
> reset (if the pin is there) instead of just bringing it out of reset...

Yup. Exactly that.
> 
> I already spoke with Alex and we agreed he is sending v2 since he
> started this series. So, He is deciding which plan to go for :)

Cool

Jonathan

> 
> Thanks for your explanation!
> Nuno Sá
> > Jonathan
> > 
> > 
> > 
> >   
> > > > > Nuno Sá     
> > > > > > > > > +		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,        
> > > > > > > > 
> > > > > > > >         
> > > > > > 
> > > > > >       
> > > > 
> > > >     
> 

Powered by blists - more mailing lists