On Tue, 2022-07-19 at 14:59 +0000, Ibrahim Tilki wrote:
> > On Thu, 7 Jul 2022 08:31:24 +0000
> > Ibrahim Tilki <Ibrahim.Tilki@xxxxxxxxxx> wrote:
> >
> > > Adding support for max11410 24-bit, 1.9ksps delta-sigma adc which
> > > has 3 differential reference and 10 differential channel inputs.
> > > Inputs and references can be buffered internally. Inputs can also
> > > be amplified with internal PGA.
> > >
> > > Device has a digital filter that is controlled by a custom sysfs
> > > attribute.
> > > User has four options to choose from: fir50/60, fir50, fir60 and
> > > sinc4.
> > > Digital filter selection affects sampling frequency range so
> > > driver
> > > has to consider the configured filter when configuring sampling
> > > frequency.
> > >
> > > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@xxxxxxxxxx>
> > > Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@xxxxxxxxxx>
> >
> > Hi Ibrahim,
> >
> > As you probably expect, quite a bit of the feedback inline is about
> > the
> > custom sysfs attribute. I think we need to fit that more closely to
> > the current
> > filter ABI. It's not a perfect fit however, but I make some
> > suggestions inline.
> >
> > thanks,
> >
> > Jonathan
> >
>
> Hi Jonathan,
>
> Thanks for the review, I've resolved most of the items and will send
> v3 soon after
> I perform some more tests with the hardware. In the meantime I have
> some questions inline.
>
> Regards,
> Ibrahim
>
> ...
>
> > > +static int max11410_read_reg(struct max11410_state *st,
> > > + unsigned int reg,
> > > + int *val)
> > > +{
> > > + u8 data[3];
> > > + int ret;
> > > +
> > > + if (max11410_reg_size(reg) == 3) {
> > > + ret = regmap_bulk_read(st->regmap, reg, data, 3);
> >
> > Ah. There is a fun corner here. SPI bulk reads in general
> > require DMA safe buffers (basically they need to be on the heap,
> > not the
> > stack and we need to enforce that nothing else shares a cacheline
> > with them).
> > Now, last time I checked regmap happens to always end up using a
> > safe bounce
> > buffer, but it's not documented as such and there is no guarantee
> > it will continue
> > to do so. We checked this with the maintainer a while back and the
> > answer
> > was to always use DMA safe buffers with bulk accesses.
> > Whilst that might have changed, I've not heard anything about it
> > doing so.
> >
>
> So I guess having this would solve dma alignment and the leak issue
> in max11410_trigger_handler
> and the data field can be shared between?
>
> struct max11410_state {
> // ...
> struct {
> int data ____cacheline_aligned;
> s64 ts __aligned(8);
> } scan;
> };
>
Just a note on this one... You want to use 'IIO_DMA_MINALIGN' and
not ____cacheline_aligned.
https://lore.kernel.org/linux-iio/20220508175712.647246-1-jic23@xxxxxxxxxx/
- Nuno Sá
