Re: [PATCH 1/5] iio: imu: adis: Add Managed device functions

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 25 Feb 2020 13:41:48 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> This patch adds support for a managed device version of
> adis_setup_buffer_and_trigger. It works exactly as the original
> one but it calls all the devm_iio_* functions to setup an iio
> buffer and trigger. Hence we do not need to care about cleaning those
> and we do not need to support a remove() callback for every driver using
> the adis library.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

A few trivial things inline.

I'm hoping the plan here is to replace all the existing non devm versions
and remove the non devm versions?

That way we don't end up with a near identical repeated block of code.

Thanks,

Jonathan


> ---
>  drivers/iio/imu/adis_buffer.c  | 34 +++++++++++++++++++++++++++++
>  drivers/iio/imu/adis_trigger.c | 39 +++++++++++++++++++++++++++++++---
>  include/linux/iio/imu/adis.h   | 17 +++++++++++++++
>  3 files changed, 87 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index 3f4dd5c00b03..296036a01d39 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -196,7 +196,41 @@ int adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(adis_setup_buffer_and_trigger);

blank line here.

> +/**
> + * devm_adis_setup_buffer_and_trigger() - Sets up buffer and trigger for
> + *					  the managed adis device
> + * @adis: The adis device
> + * @indio_dev: The IIO device
> + * @trigger_handler: Optional trigger handler, may be NULL.
> + *
> + * Returns 0 on success, a negative error code otherwise.
> + *
> + * This function perfoms exactly the same as adis_setup_buffer_and_trigger()
> + */
> +int
> +devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
> +				   irqreturn_t (*trigger_handler)(int, void *))
> +{
> +	int ret;
> +
> +	if (!trigger_handler)
> +		trigger_handler = adis_trigger_handler;
> +
> +	ret = devm_iio_triggered_buffer_setup(&adis->spi->dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (adis->spi->irq) {
> +		ret = devm_adis_probe_trigger(adis, indio_dev);
> +		if (ret)
> +			return ret;
> +	}
>  
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_adis_setup_buffer_and_trigger);
>  /**
>   * adis_cleanup_buffer_and_trigger() - Free buffer and trigger resources
>   * @adis: The adis device.
> diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c
> index 8b9cd02c0f9f..a07dcc365c18 100644
> --- a/drivers/iio/imu/adis_trigger.c
> +++ b/drivers/iio/imu/adis_trigger.c
> @@ -27,6 +27,13 @@ static const struct iio_trigger_ops adis_trigger_ops = {
>  	.set_trigger_state = &adis_data_rdy_trigger_set_state,
>  };
>  
> +static inline void adis_trigger_setup(struct adis *adis)
> +{
> +	adis->trig->dev.parent = &adis->spi->dev;
> +	adis->trig->ops = &adis_trigger_ops;
> +	iio_trigger_set_drvdata(adis->trig, adis);
> +}
> +
>  /**
>   * adis_probe_trigger() - Sets up trigger for a adis device
>   * @adis: The adis device
> @@ -45,9 +52,7 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
>  	if (adis->trig == NULL)
>  		return -ENOMEM;
>  
> -	adis->trig->dev.parent = &adis->spi->dev;
> -	adis->trig->ops = &adis_trigger_ops;
> -	iio_trigger_set_drvdata(adis->trig, adis);
> +	adis_trigger_setup(adis);
>  
>  	ret = request_irq(adis->spi->irq,
>  			  &iio_trigger_generic_data_rdy_poll,
> @@ -72,7 +77,35 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(adis_probe_trigger);

Blank line here would help a tiny bit on readability.

> +/**
> + * devm_adis_probe_trigger() - Sets up trigger for a managed adis device
> + * @adis: The adis device
> + * @indio_dev: The IIO device
> + *
> + * Returns 0 on success or a negative error code
> + */
> +int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
> +{
> +	int ret;
>  
> +	adis->trig = devm_iio_trigger_alloc(&adis->spi->dev, "%s-dev%d",
> +					    indio_dev->name, indio_dev->id);
> +	if (!adis->trig)
> +		return -ENOMEM;
> +
> +	adis_trigger_setup(adis);
> +
> +	ret = devm_request_irq(&adis->spi->dev, adis->spi->irq,
> +			       &iio_trigger_generic_data_rdy_poll,
> +			       IRQF_TRIGGER_RISING,
> +			       indio_dev->name,
> +			       adis->trig);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_trigger_register(&adis->spi->dev, adis->trig);
> +}
> +EXPORT_SYMBOL_GPL(devm_adis_probe_trigger);
>  /**
>   * adis_remove_trigger() - Remove trigger for a adis devices
>   * @adis: The adis device
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index ac7cfd073804..741512b28aaa 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -419,11 +419,15 @@ struct adis_burst {
>  	unsigned int	extra_len;
>  };
>  
> +int
> +devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
> +				   irqreturn_t (*trigger_handler)(int, void *));
>  int adis_setup_buffer_and_trigger(struct adis *adis,
>  	struct iio_dev *indio_dev, irqreturn_t (*trigger_handler)(int, void *));
>  void adis_cleanup_buffer_and_trigger(struct adis *adis,
>  	struct iio_dev *indio_dev);
>  
> +int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev);
>  int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev);
>  void adis_remove_trigger(struct adis *adis);
>  
> @@ -432,6 +436,13 @@ int adis_update_scan_mode(struct iio_dev *indio_dev,
>  
>  #else /* CONFIG_IIO_BUFFER */
>  
> +static inline int
> +devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
> +				   irqreturn_t (*trigger_handler)(int, void *))
> +{
> +	return 0;
> +}
> +
>  static inline int adis_setup_buffer_and_trigger(struct adis *adis,
>  	struct iio_dev *indio_dev, irqreturn_t (*trigger_handler)(int, void *))
>  {
> @@ -443,6 +454,12 @@ static inline void adis_cleanup_buffer_and_trigger(struct adis *adis,
>  {
>  }
>  
> +static inline int devm_adis_probe_trigger(struct adis *adis,
> +					  struct iio_dev *indio_dev)
> +{
> +	return 0;
> +}
> +
>  static inline int adis_probe_trigger(struct adis *adis,
>  	struct iio_dev *indio_dev)
>  {