Re: [PATCH v2 6/9] iio: imu: adis: Refactor adis_initial_startup

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 10 Feb 2020 15:26:03 +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. Previously, the logic was that the self-test
> was performed in adis_initial_startup() and if that failed a reset was done
> and then a self-test was attempted again.
> 
> This change unifies the reset mechanism under the adis_initial_startup()
> call. A HW reset will be done if  GPIO is configured, or a SW reset
> otherwise. This should make sure that the chip is in a sane state for
> self-test. Once the reset is done, the self-test operation will be
> performed. If anything goes wrong with self-test, the driver should just
> bail/error-out (i.e. no second attempt). The chip would likely not be a in
> a sane state state if the self-test fails after a reset.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
A very small tweak in here..

Tweak made and patch applied to the togreg branch of iio.git and pushed out
as testing for the autobuilders to poke at it.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/Kconfig |  1 +
>  drivers/iio/imu/adis.c  | 29 ++++++++++++++++++++++-------
>  2 files changed, 23 insertions(+), 7 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
This is needed. If we don't have GPIOLIB the gpio_get
will return NULL and we'll carry on as if one wasn't supplied in the first
place which should work fine.

As a side note, you can't do depends inside something that is selected and
expect the kernel build system to notice.  If this was actually needed
you would have gotten build errors in some configurations.


>  	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 f7845a90f376..0bd6e32cf577 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>
> @@ -368,6 +369,10 @@ static int adis_self_test(struct adis *adis)
>   * __adis_initial_startup() - Device initial setup
>   * @adis: The adis device
>   *
> + * The function performs a HW reset via a reset pin that should be specified
> + * via GPIOLIB. If no pin is configured a SW reset will be performed.
> + * The RST pin for the ADIS devices should be configured as ACTIVE_LOW.
> + *
>   * Returns 0 if the device is operational, a negative error code otherwise.
>   *
>   * This function should be called early on in the device initialization sequence
> @@ -375,18 +380,28 @@ static int adis_self_test(struct adis *adis)
>   */
>  int __adis_initial_startup(struct adis *adis)
>  {
> +	const struct adis_timeout *timeouts = adis->data->timeouts;
> +	struct gpio_desc *gpio;
>  	int 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);
> +	/* 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);
> +
> +	if (gpio) {
> +		gpiod_set_value_cansleep(gpio, 1);
> +		msleep(10);
> +		/* bring device out of reset */
> +		gpiod_set_value_cansleep(gpio, 0);
> +		msleep(timeouts->reset_ms);
> +	} else {
> +		ret = __adis_reset(adis);
>  		if (ret)
> -			dev_err(&adis->spi->dev, "Second self-test failed, giving up.\n");
> +			return ret;
>  	}
>  
> -	return ret;
> +	return adis_self_test(adis);
>  }
>  EXPORT_SYMBOL_GPL(__adis_initial_startup);
>