Re: [PATCH 6/8] iio: adc: ad7606: Add PWM support for conversion trigger

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 15 Aug 2024 12:12:00 +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.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
Hi,

A few comments inline. I'm not particularly familiar with PWM
controls though so that bit could do with more eyes.

Jonathan

> ---

> +
> +static int ad7606_pwm_set_swing(struct ad7606_state *st)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +
> +	if (!st->cnvst_pwm)
> +		return -EINVAL;
Blank line here and in similar cases where we have unrelated blocks
of code otherwise with no line between them.


> +	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;

Blank line 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;
> +}
> +
> +static int ad7606_set_sampling_freq(struct ad7606_state *st, unsigned long freq)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +	bool is_swinging = ad7606_pwm_is_swinging(st);
> +	bool is_high;
> +
> +	if (freq == 0)
> +		return -EINVAL;
> +
> +	/*Retrieve the previous state */
/* Ret

> +	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +	is_high = cnvst_pwm_state.duty_cycle == cnvst_pwm_state.period;
> +
> +	cnvst_pwm_state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> +	cnvst_pwm_state.polarity = PWM_POLARITY_NORMAL;
> +	if (is_high)
> +		cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period;
> +	else if (is_swinging)
> +		cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period / 2;
> +
> +	return pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
> +}
> +
>  static int ad7606_read_samples(struct ad7606_state *st)
>  {
>  	unsigned int num = st->chip_info->num_channels - 1;
> @@ -141,9 +213,21 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>  static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  {
>  	struct ad7606_state *st = iio_priv(indio_dev);
> +	struct pwm_state cnvst_pwm_state;
>  	int ret;
> +	bool pwm_swings = false;
>  
> -	gpiod_set_value(st->gpio_convst, 1);
> +	if (st->gpio_convst) {
> +		gpiod_set_value(st->gpio_convst, 1);
> +	} else {
> +		pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +		/* Keep the current PWM state to switch it back off if needed*/
needed */

also back on or off I guess?

> +		if (ad7606_pwm_is_swinging(st))
> +			pwm_swings = true;
> +		ret = ad7606_pwm_set_high(st);
> +		if (!ret)
> +			return ret;
> +	}
>  	ret = wait_for_completion_timeout(&st->completion,
>  					  msecs_to_jiffies(1000));
>  	if (!ret) {
> @@ -154,7 +238,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];
> -
> +	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);



> +static void ad7606_pwm_disable(void *data)
> +{
> +	pwm_disable(data);
> +}
> +
>  int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  		 const char *name, unsigned int id,
>  		 const struct ad7606_bus_ops *bops)
> @@ -635,20 +733,42 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  			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;
> -
> -	indio_dev->trig = iio_trigger_get(st->trig);
> +	/* If convst pin is not defined, setup PWM*/
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);
> +		ret = devm_add_action_or_reset(dev, ad7606_pwm_disable,

Add a comment on why we are registering devm based disabling here.
I'm not sure where matching enable is.

> +					       st->cnvst_pwm);
> +		if (ret)
> +			return ret;
>  
> +		/*
> +		 * Set the sampling rate to 2 kHz so to be sure that the interruption can be
> +		 * handled between within a single pulse.
> +		 */
> +		ret = ad7606_set_sampling_freq(st, 2 * KILO);
> +		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;
> +		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;
> +		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,
> @@ -657,13 +777,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);
>  }
>  EXPORT_SYMBOL_NS_GPL(ad7606_probe, IIO_AD7606);
> @@ -693,7 +806,6 @@ static int ad7606_resume(struct device *dev)
>  		gpiod_set_value(st->gpio_standby, 1);
>  		ad7606_reset(st);
>  	}
> -
Check for spurious noisy whitespace cleanup in patches.
it doesn't belong in a patch doing anything other that whitespace
changes.

>  	return 0;
>  }
>  
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index 0c6a88cc4695..60bac1894a91 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -38,6 +38,8 @@
>  	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>  		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>  
> +struct pwm_device		*cnvst_pwm;

I'm going to guess that this started global then some internal review
or you guessed that wasn't going anywhere and added it to the state
structure below.  Then forgot to clean this up.
Anyhow this is correctly unused, get rid of it.

> +
>  /**
>   * struct ad7606_chip_info - chip specific information
>   * @channels:		channel specification
> @@ -94,6 +96,7 @@ struct ad7606_state {
>  	const struct ad7606_bus_ops	*bops;
>  	unsigned int			range[16];
>  	unsigned int			oversampling;
> +	struct pwm_device		*cnvst_pwm;
This structure had docs that need updating.

>  	void __iomem			*base_address;
>  	bool				sw_mode_en;
>  	const unsigned int		*scale_avail;
>