Re: [PATCH v2 1/3] iio: adc: add max11410 adc driver

["Nuno Sá" <noname.nuno@gmail.com>]

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@analog.com> 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@analog.com>
> > > Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@analog.com>
> > 
> > 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@kernel.org/


- Nuno Sá