Message-ID: <5860310e0413038ac477b9e191c3d15029487628.camel@gmail.com>
Date: Wed, 09 Oct 2024 16:29:33 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Guillaume Stols <gstols@...libre.com>, Uwe
 Kleine-König <ukleinek@...nel.org>, Lars-Peter Clausen
 <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
 "Rafael J. Wysocki" <rafael@...nel.org>, Jonathan Corbet <corbet@....net>
Cc: linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org, 
 linux-fbdev@...r.kernel.org, linux-iio@...r.kernel.org, 
 devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
 aardelean@...libre.com,  dlechner@...libre.com, jstephan@...libre.com,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v4 4/8] iio: adc: ad7606: Add PWM support for conversion
 trigger

On Wed, 2024-10-09 at 09:19 +0000, Guillaume Stols 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@...libre.com>
> ---

Hi Guillaume,

Looks overall good, just some minor stuff

>  drivers/iio/adc/ad7606.c | 143 +++++++++++++++++++++++++++++++++++++++++++----
>  drivers/iio/adc/ad7606.h |   2 +
>  2 files changed, 135 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index 71362eafe838..5b276d087ec3 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,
>  	}
>  }
>  
> +static int ad7606_pwm_set_high(struct ad7606_state *st)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +	int ret;
> +
> +	if (!st->cnvst_pwm)
> +		return -EINVAL;
> +

Maybe consider doing the check before calling the API (for the cases that need it)?
It seems at least that in a couple of cases you actually already know that the PWM
must be here (since you check for the gpio presence)...

> +	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +	cnvst_pwm_state.enabled = true;
> +	cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period;
> +
> +	ret = pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
> +	/* sleep 2 µS to let finish the current pulse */
> +	fsleep(2);
> +
> +	return ret;
> +}
> +
> +static int ad7606_pwm_set_low(struct ad7606_state *st)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +	int ret;
> +
> +	if (!st->cnvst_pwm)
> +		return -EINVAL;
> +

Same deal...

> +	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +	cnvst_pwm_state.enabled = true;
> +	cnvst_pwm_state.duty_cycle = 0;
> +
> +	ret = pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
> +	/* sleep 2 µS to let finish the current pulse */
> +	fsleep(2);
> +
> +	return ret;
> +}
> +
> +static bool ad7606_pwm_is_swinging(struct ad7606_state *st)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +
> +	if (!st->cnvst_pwm)
> +		return false;
> +

This one also seems to be redundant? ad7606_set_sampling_freq() should be only called
from a context where the PWM exists right?

> +	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. */
> +	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;
> +	else
> +		cnvst_pwm_state.duty_cycle = 0;
> +
> +	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;
> @@ -325,6 +403,7 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>  	iio_trigger_notify_done(indio_dev->trig);
>  	/* The rising edge of the CONVST signal starts a new conversion. */
>  	gpiod_set_value(st->gpio_convst, 1);
> +	ad7606_pwm_set_high(st);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -337,7 +416,13 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev,
> unsigned int ch,
>  	const struct iio_chan_spec *chan;
>  	int ret;
>  
> -	gpiod_set_value(st->gpio_convst, 1);
> +	if (st->gpio_convst) {
> +		gpiod_set_value(st->gpio_convst, 1);
> +	} else {
> +		ret = ad7606_pwm_set_high(st);
> +		if (ret < 0)
> +			return ret;
> +	}
>  	ret = wait_for_completion_timeout(&st->completion,
>  					  msecs_to_jiffies(1000));
>  	if (!ret) {
> @@ -363,6 +448,11 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev,
> unsigned int ch,
>  	}
>  
>  error_ret:
> +	if (!st->gpio_convst) {
> +		ret = ad7606_pwm_set_low(st);
> +		if (ret < 0)
> +			return ret;
> +	}
>  	gpiod_set_value(st->gpio_convst, 0);
>  
>  	return ret;
> @@ -662,8 +752,9 @@ static int ad7606_request_gpios(struct ad7606_state *st)
>  {
>  	struct device *dev = st->dev;
>  
> -	st->gpio_convst = devm_gpiod_get(dev, "adi,conversion-start",
> -					 GPIOD_OUT_LOW);
> +	st->gpio_convst = devm_gpiod_get_optional(dev, "adi,conversion-start",
> +						  GPIOD_OUT_LOW);
> +
>  	if (IS_ERR(st->gpio_convst))
>  		return PTR_ERR(st->gpio_convst);
>  
> @@ -708,6 +799,7 @@ static irqreturn_t ad7606_interrupt(int irq, void *dev_id)
>  
>  	if (iio_buffer_enabled(indio_dev)) {
>  		gpiod_set_value(st->gpio_convst, 0);
> +		ad7606_pwm_set_low(st);
>  		iio_trigger_poll_nested(st->trig);
>  	} else {
>  		complete(&st->completion);
> @@ -732,6 +824,7 @@ static int ad7606_buffer_postenable(struct iio_dev *indio_dev)
>  	struct ad7606_state *st = iio_priv(indio_dev);
>  
>  	gpiod_set_value(st->gpio_convst, 1);
> +	ad7606_pwm_set_high(st);

error handling?
>  
>  	return 0;
>  }
> @@ -741,6 +834,7 @@ static int ad7606_buffer_predisable(struct iio_dev *indio_dev)
>  	struct ad7606_state *st = iio_priv(indio_dev);
>  
>  	gpiod_set_value(st->gpio_convst, 0);
> +	ad7606_pwm_set_low(st);
> 

error handling?

>  	return 0;
>  }
> @@ -874,6 +968,11 @@ static int ad7606_chan_scales_setup(struct iio_dev *indio_dev)
>  	return 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)
> @@ -950,6 +1049,31 @@ int ad7606_probe(struct device *dev, int irq, void __iomem
> *base_address,
>  	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);
> +
> +		/* 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;
> +
> +		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;
> +	}
>  	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
>  					  indio_dev->name,
>  					  iio_device_id(indio_dev));
> @@ -963,6 +1087,12 @@ int ad7606_probe(struct device *dev, int irq, void __iomem
> *base_address,
>  		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;
> 

The above change seems unrelated?

- Nuno Sá

Powered by blists - more mailing lists