Hello,
On Fri, Jan 31, 2025 at 06:18:48PM +0000, Jonathan Cameron wrote:
> On Thu, 30 Jan 2025 12:08:27 +0100
> Esteban Blanc <eblanc@xxxxxxxxxxxx> wrote:
>
> > This add support for the averaging mode of AD4030 using oversampling IIO
> > attribute
> >
> > Signed-off-by: Esteban Blanc <eblanc@xxxxxxxxxxxx>
> A couple of comments inline. The one about the gpio wiggling
> is just me venting at silly hardware, so feel free to ignore that!
> Other one is a trivial 'nice to have' for formatting.
>
> Jonathan
>
> >
> > +static const int ad4030_average_modes[] = {
> > + 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384,
> > + 32768, 65536
> > +};
> Groups of 8 often best option for lists like this. Make it easy to see how
> many there are.
I personally also like adding the indexes explicitly. So I would write
this as something like:
static const int ad4030_average_modes[] = {
/* [ 0] */ 1, 2, 4, 8, 16, 32, 64, 128,
/* [ 8] */ 256, 512, 1024, 2048, 4096, 8192, 16384, 32768,
/* [16] */ 65536,
};
Or (being unsure this is long enough to make this useful) just:
/* [i] = 2 ** i for i in [0 ... 16] */
static const int ad4030_average_modes[] = {
1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384,
32768, 65536
};
> > - gpiod_set_value_cansleep(st->cnv_gpio, 1);
> > - ndelay(AD4030_TCNVH_NS);
> > - gpiod_set_value_cansleep(st->cnv_gpio, 0);
> > - ndelay(st->chip->tcyc_ns);
> > + for (i = 0; i < cnv_nb; i++) {
> > + gpiod_set_value_cansleep(st->cnv_gpio, 1);
> > + ndelay(AD4030_TCNVH_NS);
> > + gpiod_set_value_cansleep(st->cnv_gpio, 0);
> > + ndelay(st->chip->tcyc_ns);
>
> Hmm. This is a bit nasty. To actually use this in anger
> and get decent performance I guess a PWM with appropriate
> functionality to do the right length pull train is the
> way to go.
I think the imx27 PWM can be made to do that, but that's not actually
easy. (It has a FIFO for duty_cycle values of length 4 and always
consumes the next entry at the end of a period if the FIFO is non-empty,
so either you have to hit the right timing when filling the FIFO (but
then you can just do it generically for all PWMs) or you have to write
cnv_nb times the a non-zero value and then a zero. But with cnv_nb > 4
that's also non-trivial. Though it might support DMA for that, not
sure.) I'm not aware of other supported PWM hardware that has such a
feature (but I might miss one or the other). So I don't think this is
something valuable and useful to add to the hardware abstraction.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
