Message-ID: <20200201170839.4ab98d8e@archlinux>
Date:   Sat, 1 Feb 2020 17:08:39 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Alexandru Ardelean <alexandru.ardelean@...log.com>
Cc:     <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devel@...verdev.osuosl.org>, <dragos.bogdan@...log.com>,
        <nuno.sa@...log.com>
Subject: Re: [PATCH 2/4] iio: imu: adis: Refactor adis_initial_startup

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

> +	} 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?

> +		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?

> +	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