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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 20 Sep 2024 17:33:26 +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 conjonction with IIO backend, and for now only a sampling frequency
> of 2 kHz is available.
Spell check patch descriptions.  conjunction

Note this is a do as I say, not as I do because my spelling is terrible and
I frequently forget to check them.

A few trivial things inline.

Jonathan

> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  drivers/iio/adc/ad7606.c | 170 ++++++++++++++++++++++++++++++++++++++++-------
>  drivers/iio/adc/ad7606.h |   2 +
>  2 files changed, 148 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index 9b457472d49c..b98057138295 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -13,11 +13,13 @@
>  #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/util_macros.h>
> +#include <linux/units.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/buffer.h>
> @@ -83,6 +85,80 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int ad7606_pwm_set_high(struct ad7606_state *st)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +
> +	if (!st->cnvst_pwm)
> +		return -EINVAL;

Trivial but add a blank line here to separate the sanity check from the
guts of the function.

> +	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +	cnvst_pwm_state.enabled = true;
> +	cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period;
> +
> +	return pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
> +}
> +
> +static int ad7606_pwm_set_low(struct ad7606_state *st)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +
> +	if (!st->cnvst_pwm)
> +		return -EINVAL;

Likewise blank line here.

> +	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +	cnvst_pwm_state.enabled = true;
> +	cnvst_pwm_state.duty_cycle = 0;
> +
> +	return pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
> +}
> +
> +static int ad7606_pwm_set_swing(struct ad7606_state *st)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +
> +	if (!st->cnvst_pwm)
> +		return -EINVAL;
> +
> +	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +	cnvst_pwm_state.enabled = true;
> +	cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period / 2;
> +
> +	return pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
> +}
> +
> +static bool ad7606_pwm_is_swinging(struct ad7606_state *st)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +
> +	if (!st->cnvst_pwm)
> +		return false;
And here.

> +	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +	return cnvst_pwm_state.duty_cycle != cnvst_pwm_state.period &&
> +	       cnvst_pwm_state.duty_cycle != 0;
> +}
> @@ -130,7 +219,12 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  	ret = ad7606_read_samples(st);
>  	if (ret == 0)
>  		ret = st->data[ch];
> -

I'd keep a blank line here as this code unconnected to previous block.

> +	if (!st->gpio_convst) {
> +		if (!pwm_swings)
> +			ret = ad7606_pwm_set_low(st);
> +		else
> +			ret = ad7606_pwm_set_swing(st);
> +	}
>  error_ret:
>  	gpiod_set_value(st->gpio_convst, 0);