On Fri, 20 Sep 2024 17:33:26 +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 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@xxxxxxxxxxxx>
> ---
> 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);
