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@xxxxxxxxxxxx> > --- 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á