[PATCH 1/4] iio: imu: adis: Add self_test_reg variable

[PATCH 1/4] iio: imu: adis: Add self_test_reg variable

[Alexandru Ardelean]

From: Nuno Sá <nuno.sa@analog.com>

This patch adds a dedicated self_test_reg variable. This is also a step
to let new drivers make use of `adis_initial_startup()`. Some devices
use MSG_CTRL reg to request a self_test command while others use the
GLOB_CMD register.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/accel/adis16201.c         | 1 +
 drivers/iio/accel/adis16209.c         | 1 +
 drivers/iio/gyro/adis16136.c          | 1 +
 drivers/iio/gyro/adis16260.c          | 1 +
 drivers/iio/imu/adis.c                | 6 +++---
 drivers/iio/imu/adis16400.c           | 1 +
 drivers/iio/imu/adis16460.c           | 2 ++
 drivers/iio/imu/adis16480.c           | 3 +++
 drivers/staging/iio/accel/adis16203.c | 1 +
 drivers/staging/iio/accel/adis16240.c | 1 +
 include/linux/iio/imu/adis.h          | 2 ++
 11 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adis16201.c b/drivers/iio/accel/adis16201.c
index 0f0f27a8184e..4154e7396bbe 100644
--- a/drivers/iio/accel/adis16201.c
+++ b/drivers/iio/accel/adis16201.c
@@ -246,6 +246,7 @@ static const struct adis_data adis16201_data = {
 	.diag_stat_reg = ADIS16201_DIAG_STAT_REG,
 
 	.self_test_mask = ADIS16201_MSC_CTRL_SELF_TEST_EN,
+	.self_test_reg = ADIS16201_MSC_CTRL_REG,
 	.self_test_no_autoclear = true,
 	.timeouts = &adis16201_timeouts,
 
diff --git a/drivers/iio/accel/adis16209.c b/drivers/iio/accel/adis16209.c
index c6dbd2424e10..31d45e7c5485 100644
--- a/drivers/iio/accel/adis16209.c
+++ b/drivers/iio/accel/adis16209.c
@@ -256,6 +256,7 @@ static const struct adis_data adis16209_data = {
 	.diag_stat_reg = ADIS16209_STAT_REG,
 
 	.self_test_mask = ADIS16209_MSC_CTRL_SELF_TEST_EN,
+	.self_test_reg = ADIS16209_MSC_CTRL_REG,
 	.self_test_no_autoclear = true,
 	.timeouts = &adis16209_timeouts,
 
diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
index d5e03a406d4a..e8375d4a408f 100644
--- a/drivers/iio/gyro/adis16136.c
+++ b/drivers/iio/gyro/adis16136.c
@@ -472,6 +472,7 @@ static const struct adis_data adis16136_data = {
 	.msc_ctrl_reg = ADIS16136_REG_MSC_CTRL,
 
 	.self_test_mask = ADIS16136_MSC_CTRL_SELF_TEST,
+	.self_test_reg = ADIS16136_REG_MSC_CTRL,
 
 	.read_delay = 10,
 	.write_delay = 10,
diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c
index be09b3e5910c..9823573e811a 100644
--- a/drivers/iio/gyro/adis16260.c
+++ b/drivers/iio/gyro/adis16260.c
@@ -346,6 +346,7 @@ static const struct adis_data adis16260_data = {
 	.diag_stat_reg = ADIS16260_DIAG_STAT,
 
 	.self_test_mask = ADIS16260_MSC_CTRL_MEM_TEST,
+	.self_test_reg = ADIS16260_MSC_CTRL,
 	.timeouts = &adis16260_timeouts,
 
 	.status_error_msgs = adis1620_status_error_msgs,
diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 022bb54fb748..d02b1911b0f2 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -346,8 +346,8 @@ static int adis_self_test(struct adis *adis)
 	int ret;
 	const struct adis_timeout *timeouts = adis->data->timeouts;
 
-	ret = __adis_write_reg_16(adis, adis->data->msc_ctrl_reg,
-			adis->data->self_test_mask);
+	ret = __adis_write_reg_16(adis, adis->data->self_test_reg,
+				  adis->data->self_test_mask);
 	if (ret) {
 		dev_err(&adis->spi->dev, "Failed to initiate self test: %d\n",
 			ret);
@@ -359,7 +359,7 @@ static int adis_self_test(struct adis *adis)
 	ret = __adis_check_status(adis);
 
 	if (adis->data->self_test_no_autoclear)
-		__adis_write_reg_16(adis, adis->data->msc_ctrl_reg, 0x00);
+		__adis_write_reg_16(adis, adis->data->self_test_reg, 0x00);
 
 	return ret;
 }
diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index cfb1c19eb930..a59cd7e0c7ed 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -1126,6 +1126,7 @@ static const struct adis_data adis16400_data = {
 	.write_delay = 50,
 
 	.self_test_mask = ADIS16400_MSC_CTRL_MEM_TEST,
+	.self_test_reg = ADIS16400_MSC_CTRL,
 
 	.status_error_msgs = adis16400_status_error_msgs,
 	.status_error_mask = BIT(ADIS16400_DIAG_STAT_ZACCL_FAIL) |
diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
index 9539cfe4a259..42fa473c6d81 100644
--- a/drivers/iio/imu/adis16460.c
+++ b/drivers/iio/imu/adis16460.c
@@ -392,6 +392,8 @@ static const struct adis_timeout adis16460_timeouts = {
 static const struct adis_data adis16460_data = {
 	.diag_stat_reg = ADIS16460_REG_DIAG_STAT,
 	.glob_cmd_reg = ADIS16460_REG_GLOB_CMD,
+	.self_test_mask = BIT(2),
+	.self_test_reg = ADIS16460_REG_GLOB_CMD,
 	.has_paging = false,
 	.read_delay = 5,
 	.write_delay = 5,
diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index dac87f1001fd..e1de25f18e2e 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -1080,6 +1080,9 @@ static const struct adis_data adis16480_data = {
 	.read_delay = 5,
 	.write_delay = 5,
 
+	.self_test_mask = BIT(1),
+	.self_test_reg = ADIS16480_REG_GLOB_CMD,
+
 	.status_error_msgs = adis16480_status_error_msgs,
 	.status_error_mask = BIT(ADIS16480_DIAG_STAT_XGYRO_FAIL) |
 		BIT(ADIS16480_DIAG_STAT_YGYRO_FAIL) |
diff --git a/drivers/staging/iio/accel/adis16203.c b/drivers/staging/iio/accel/adis16203.c
index 39dfe3f7f254..fef52d9b5346 100644
--- a/drivers/staging/iio/accel/adis16203.c
+++ b/drivers/staging/iio/accel/adis16203.c
@@ -250,6 +250,7 @@ static const struct adis_data adis16203_data = {
 	.diag_stat_reg = ADIS16203_DIAG_STAT,
 
 	.self_test_mask = ADIS16203_MSC_CTRL_SELF_TEST_EN,
+	.self_test_reg = ADIS16203_MSC_CTRL,
 	.self_test_no_autoclear = true,
 	.timeouts = &adis16203_timeouts,
 
diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
index 39eb8364aa95..8bd35c6c56a1 100644
--- a/drivers/staging/iio/accel/adis16240.c
+++ b/drivers/staging/iio/accel/adis16240.c
@@ -373,6 +373,7 @@ static const struct adis_data adis16240_data = {
 	.diag_stat_reg = ADIS16240_DIAG_STAT,
 
 	.self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
+	.self_test_reg = ADIS16240_MSC_CTRL,
 	.self_test_no_autoclear = true,
 	.timeouts = &adis16240_timeouts,
 
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index d2fcf45b4cef..d21a013d1122 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
+ * @self_test_reg: Register address to request self test command
  * @status_error_msgs: Array of error messgaes
  * @status_error_mask:
  * @timeouts: Chip specific delays
@@ -55,6 +56,7 @@ struct adis_data {
 	unsigned int diag_stat_reg;
 
 	unsigned int self_test_mask;
+	unsigned int self_test_reg;
 	bool self_test_no_autoclear;
 	const struct adis_timeout *timeouts;
 
-- 
2.20.1

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

[Alexandru Ardelean]

From: Nuno Sá <nuno.sa@analog.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@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.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);
+	} else if (gpio && gpiod_get_value_cansleep(gpio)) {
+		/* bring device out of reset */
+		gpiod_set_value_cansleep(gpio, 0);
+		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);
+	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,
-- 
2.20.1

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

[Jonathan Cameron]

On Mon, 20 Jan 2020 16:20:49 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Nuno Sá <nuno.sa@analog.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@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.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,

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

[Nuno Sá]

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@analog.com> wrote:
> 
> > From: Nuno Sá <nuno.sa@analog.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@analog.com>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.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.

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

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@gmail.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@analog.com> wrote:
> >   
> > > From: Nuno Sá <nuno.sa@analog.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@analog.com>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.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.

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

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

[Sa, Nuno]

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@gmail.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@analog.com> wrote:
> > >   
> > > > From: Nuno Sá <nuno.sa@analog.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@analog.com>
> > > > Signed-off-by: Alexandru Ardelean <
> > > > alexandru.ardelean@analog.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...


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

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

[Jonathan Cameron]

On Wed, 5 Feb 2020 12:25:40 +0000
"Sa, Nuno" <Nuno.Sa@analog.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@gmail.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@analog.com> wrote:
> > > >     
> > > > > From: Nuno Sá <nuno.sa@analog.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@analog.com>
> > > > > Signed-off-by: Alexandru Ardelean <  
> > > > > alexandru.ardelean@analog.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

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

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

[Sa, Nuno]

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@analog.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@gmail.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@analog.com> wrote:
> > > > >     
> > > > > > From: Nuno Sá <nuno.sa@analog.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@analog.com>
> > > > > > Signed-off-by: Alexandru Ardelean <  
> > > > > > alexandru.ardelean@analog.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...

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

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

[Jonathan Cameron]

On Wed, 5 Feb 2020 16:44:13 +0000
"Sa, Nuno" <Nuno.Sa@analog.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@analog.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@gmail.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@analog.com> wrote:
> > > > > >       
> > > > > > > From: Nuno Sá <nuno.sa@analog.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@analog.com>
> > > > > > > Signed-off-by: Alexandru Ardelean <    
> > > > > > > alexandru.ardelean@analog.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.

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

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

[Sa, Nuno]

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@analog.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@analog.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@gmail.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@analog.com> wrote:
> > > > > > >       
> > > > > > > > From: Nuno Sá <nuno.sa@analog.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@analog.com>
> > > > > > > > Signed-off-by: Alexandru Ardelean <    
> > > > > > > > alexandru.ardelean@analog.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...

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 :)

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

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

[Jonathan Cameron]

On Thu, 6 Feb 2020 10:19:09 +0000
"Sa, Nuno" <Nuno.Sa@analog.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@analog.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@analog.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@gmail.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@analog.com> wrote:
> > > > > > > >         
> > > > > > > > > From: Nuno Sá <nuno.sa@analog.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@analog.com>
> > > > > > > > > Signed-off-by: Alexandru Ardelean <      
> > > > > > > > > alexandru.ardelean@analog.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,        
> > > > > > > > 
> > > > > > > >         
> > > > > > 
> > > > > >       
> > > > 
> > > >     
> 

[PATCH 3/4] iio: adis16480: Make use of __adis_initial_startup

[Alexandru Ardelean]

From: Nuno Sá <nuno.sa@analog.com>

All actions done in `adis16480_initial_setup()` are now done in
`__adis_initial_startup()` so, there's no need for code duplication.
Furthermore, the call to `adis16480_initial_setup()` is done before any
device configuration since the device will be reset if not already (via
rst pin). This is actually fixing a potential bug since `adis_reset()` was
being called after configuring the device which is obviously a problem.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16480.c | 55 ++++++++-----------------------------
 1 file changed, 11 insertions(+), 44 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index e1de25f18e2e..36973662a31d 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -1014,40 +1014,6 @@ static int adis16480_enable_irq(struct adis *adis, bool enable)
 	return __adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, val);
 }
 
-static int adis16480_initial_setup(struct iio_dev *indio_dev)
-{
-	struct adis16480 *st = iio_priv(indio_dev);
-	uint16_t prod_id;
-	unsigned int device_id;
-	int ret;
-
-	adis_reset(&st->adis);
-	msleep(70);
-
-	ret = adis_write_reg_16(&st->adis, ADIS16480_REG_GLOB_CMD, BIT(1));
-	if (ret)
-		return ret;
-	msleep(30);
-
-	ret = adis_check_status(&st->adis);
-	if (ret)
-		return ret;
-
-	ret = adis_read_reg_16(&st->adis, ADIS16480_REG_PROD_ID, &prod_id);
-	if (ret)
-		return ret;
-
-	ret = sscanf(indio_dev->name, "adis%u\n", &device_id);
-	if (ret != 1)
-		return -EINVAL;
-
-	if (prod_id != device_id)
-		dev_warn(&indio_dev->dev, "Device ID(%u) and product ID(%u) do not match.",
-				device_id, prod_id);
-
-	return 0;
-}
-
 #define ADIS16480_DIAG_STAT_XGYRO_FAIL 0
 #define ADIS16480_DIAG_STAT_YGYRO_FAIL 1
 #define ADIS16480_DIAG_STAT_ZGYRO_FAIL 2
@@ -1075,6 +1041,7 @@ static const char * const adis16480_status_error_msgs[] = {
 static const struct adis_data adis16480_data = {
 	.diag_stat_reg = ADIS16480_REG_DIAG_STS,
 	.glob_cmd_reg = ADIS16480_REG_GLOB_CMD,
+	.prod_id_reg = ADIS16480_REG_PROD_ID,
 	.has_paging = true,
 
 	.read_delay = 5,
@@ -1296,18 +1263,22 @@ static int adis16480_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = adis16480_config_irq_pin(spi->dev.of_node, st);
+	ret = __adis_initial_startup(&st->adis);
 	if (ret)
 		return ret;
 
+	ret = adis16480_config_irq_pin(spi->dev.of_node, st);
+	if (ret)
+		goto error_stop_device;
+
 	ret = adis16480_get_ext_clocks(st);
 	if (ret)
-		return ret;
+		goto error_stop_device;
 
 	if (!IS_ERR_OR_NULL(st->ext_clk)) {
 		ret = adis16480_ext_clk_config(st, spi->dev.of_node, true);
 		if (ret)
-			return ret;
+			goto error_stop_device;
 
 		st->clk_freq = clk_get_rate(st->ext_clk);
 		st->clk_freq *= 1000; /* micro */
@@ -1319,24 +1290,20 @@ static int adis16480_probe(struct spi_device *spi)
 	if (ret)
 		goto error_clk_disable_unprepare;
 
-	ret = adis16480_initial_setup(indio_dev);
-	if (ret)
-		goto error_cleanup_buffer;
-
 	ret = iio_device_register(indio_dev);
 	if (ret)
-		goto error_stop_device;
+		goto error_cleanup_buffer;
 
 	adis16480_debugfs_init(indio_dev);
 
 	return 0;
 
-error_stop_device:
-	adis16480_stop_device(indio_dev);
 error_cleanup_buffer:
 	adis_cleanup_buffer_and_trigger(&st->adis, indio_dev);
 error_clk_disable_unprepare:
 	clk_disable_unprepare(st->ext_clk);
+error_stop_device:
+	adis16480_stop_device(indio_dev);
 	return ret;
 }
 
-- 
2.20.1

[PATCH 4/4] iio: adis16460: Make use of __adis_initial_startup

[Alexandru Ardelean]

From: Nuno Sá <nuno.sa@analog.com>

All of the actions done in `adis16460_initial_setup()` are now done in
`__adis_initial_startup()` so, there's no need for code duplication.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16460.c | 37 ++-----------------------------------
 1 file changed, 2 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
index 42fa473c6d81..6f94e81c41eb 100644
--- a/drivers/iio/imu/adis16460.c
+++ b/drivers/iio/imu/adis16460.c
@@ -333,40 +333,6 @@ static int adis16460_enable_irq(struct adis *adis, bool enable)
 	return 0;
 }
 
-static int adis16460_initial_setup(struct iio_dev *indio_dev)
-{
-	struct adis16460 *st = iio_priv(indio_dev);
-	uint16_t prod_id;
-	unsigned int device_id;
-	int ret;
-
-	adis_reset(&st->adis);
-	msleep(222);
-
-	ret = adis_write_reg_16(&st->adis, ADIS16460_REG_GLOB_CMD, BIT(1));
-	if (ret)
-		return ret;
-	msleep(75);
-
-	ret = adis_check_status(&st->adis);
-	if (ret)
-		return ret;
-
-	ret = adis_read_reg_16(&st->adis, ADIS16460_REG_PROD_ID, &prod_id);
-	if (ret)
-		return ret;
-
-	ret = sscanf(indio_dev->name, "adis%u\n", &device_id);
-	if (ret != 1)
-		return -EINVAL;
-
-	if (prod_id != device_id)
-		dev_warn(&indio_dev->dev, "Device ID(%u) and product ID(%u) do not match.",
-				device_id, prod_id);
-
-	return 0;
-}
-
 #define ADIS16460_DIAG_STAT_IN_CLK_OOS	7
 #define ADIS16460_DIAG_STAT_FLASH_MEM	6
 #define ADIS16460_DIAG_STAT_SELF_TEST	5
@@ -392,6 +358,7 @@ static const struct adis_timeout adis16460_timeouts = {
 static const struct adis_data adis16460_data = {
 	.diag_stat_reg = ADIS16460_REG_DIAG_STAT,
 	.glob_cmd_reg = ADIS16460_REG_GLOB_CMD,
+	.prod_id_reg = ADIS16460_REG_PROD_ID,
 	.self_test_mask = BIT(2),
 	.self_test_reg = ADIS16460_REG_GLOB_CMD,
 	.has_paging = false,
@@ -441,7 +408,7 @@ static int adis16460_probe(struct spi_device *spi)
 
 	adis16460_enable_irq(&st->adis, 0);
 
-	ret = adis16460_initial_setup(indio_dev);
+	ret = __adis_initial_startup(&st->adis);
 	if (ret)
 		goto error_cleanup_buffer;
 
-- 
2.20.1