Re: [PATCH v3 06/10] iio: adc: ad7606: Add PWM support for conversion trigger

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 04 Oct 2024 21:48:40 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> Until now, the conversion were triggered by setting high the GPIO
> connected to the convst pin. This commit gives the possibility to
> connect the convst pin to a PWM.
> Connecting a PWM allows to have a better control on the samplerate,
> but it must be handled with care, as it is completely decorrelated of
> the driver's busy pin handling.
> Hence it is not recommended to be used "as is" but must be exploited
> in conjunction with IIO backend, and for now only a mock functionality
> is enabled, i.e PWM never swings, but is used as a GPIO, i.e duty_cycle
> == period equals high state, duty_cycle == 0 equals low state.
> 
> This mock functionality will be disabled after the IIO backend usecase
> is introduced.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
If you reroll for other reasons a few trivial comments inline

J
> ---
>  drivers/iio/adc/ad7606.c | 164 ++++++++++++++++++++++++++++++++++++++++-------
>  drivers/iio/adc/ad7606.h |   2 +
>  2 files changed, 144 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index d1aec53e0bcf..224ffaf3dbff 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -13,10 +13,12 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/property.h>
> +#include <linux/pwm.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> +#include <linux/units.h>
>  #include <linux/util_macros.h>
>  
>  #include <linux/iio/buffer.h>
> @@ -299,6 +301,82 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
>  	}
>  }
>  


>  int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  		 const char *name, unsigned int id,
>  		 const struct ad7606_bus_ops *bops)
> @@ -951,20 +1050,48 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  	if (ret)
>  		return ret;
>  
> -	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> -					  indio_dev->name,
> -					  iio_device_id(indio_dev));
> -	if (!st->trig)
> -		return -ENOMEM;
> -
> -	st->trig->ops = &ad7606_trigger_ops;
> -	iio_trigger_set_drvdata(st->trig, indio_dev);
> -	ret = devm_iio_trigger_register(dev, st->trig);
> -	if (ret)
> -		return ret;
> +	/* If convst pin is not defined, setup PWM. */
> +	if (!st->gpio_convst) {
> +		st->cnvst_pwm = devm_pwm_get(dev, NULL);
> +		if (IS_ERR(st->cnvst_pwm))
> +			return PTR_ERR(st->cnvst_pwm);

As below. Blank line preferred here.

> +		/* The PWM is initialized at 1MHz to have a fast enough GPIO emulation. */
> +		ret = ad7606_set_sampling_freq(st, 1 * MEGA);
> +		if (ret)
> +			return ret;
>  
> -	indio_dev->trig = iio_trigger_get(st->trig);
> +		ret = ad7606_pwm_set_low(st);
> +		if (ret)
> +			return ret;
>  
> +		/*
> +		 * PWM is not disabled when sampling stops, but instead its duty cycle is set
> +		 * to 0% to be sure we have a "low" state. After we unload the driver, let's
> +		 * disable the PWM.
> +		 */
> +		ret = devm_add_action_or_reset(dev, ad7606_pwm_disable,
> +					       st->cnvst_pwm);
> +		if (ret)
> +			return ret;
> +	} else {
> +		st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +						  indio_dev->name,
> +						  iio_device_id(indio_dev));
> +		if (!st->trig)
> +			return -ENOMEM;

Blank line preferred here.  Generally it helps readability a little to
have one after any 'do something and check for errors block'.


> +		st->trig->ops = &ad7606_trigger_ops;
> +		iio_trigger_set_drvdata(st->trig, indio_dev);
> +		ret = devm_iio_trigger_register(dev, st->trig);
> +		if (ret)
> +			return ret;

It we are rerolling for any other reason add a blank line here too.
If not I might tweak whilst applying or might not bother..

> +		indio_dev->trig = iio_trigger_get(st->trig);
> +		ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +						      &iio_pollfunc_store_time,
> +						      &ad7606_trigger_handler,
> +						      &ad7606_buffer_ops);
> +		if (ret)
> +			return ret;
> +	}
>  	ret = devm_request_threaded_irq(dev, irq,
>  					NULL,
>  					&ad7606_interrupt,
> @@ -973,13 +1100,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> -					      &iio_pollfunc_store_time,
> -					      &ad7606_trigger_handler,
> -					      &ad7606_buffer_ops);
> -	if (ret)
> -		return ret;
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }