On Fri, 04 Oct 2024 21:48:40 +0000
Guillaume Stols <gstols@xxxxxxxxxxxx> 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>
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);
> }
