Re: [PATCH v7 5/6] spi: cs42l43: Add SPI controller support

[Andy Shevchenko]

On Wed, Jan 31, 2024 at 5:41 PM Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, Jan 19, 2024 at 11:49:17AM +0000, Charles Keepax wrote:
> > On Thu, Jan 18, 2024 at 07:06:13PM +0200, andy.shevchenko@xxxxxxxxx wrote:
> > > Fri, Aug 04, 2023 at 11:46:01AM +0100, Charles Keepax kirjoitti:
> > > > +         while (buf < block) {
> > > > +                 const u8 *word = min(buf + sizeof(u32), block);
> > > > +                 int pad = (buf + sizeof(u32)) - word;
> > > > +
> > > > +                 while (buf < word) {
> > > > +                         val >>= BITS_PER_BYTE;
> > > > +                         val |= FIELD_PREP(GENMASK(31, 24), *buf);
> > > > +
> > > > +                         buf++;
> > > > +                 }
> > >
> > > Is this a reinvented way of get_unaligned_*() APIs?
> > >
> > > > +                 val >>= pad * BITS_PER_BYTE;
> > > > +
> > > > +                 regmap_write(regmap, CS42L43_TX_DATA, val);
> > > > +         }
> > >
> > > ...
> > >
> > > > +                 while (buf < word) {
> > > > +                         *buf = FIELD_GET(GENMASK(7, 0), val);
> > > > +
> > > > +                         val >>= BITS_PER_BYTE;
> > > > +                         buf++;
> > > > +                 }
> > >
> > > put_unaligned_*() ?
> > >
> >
> > Alas as it has been a while I have forgetten the exact context
> > here and this one will take a little more time. I will try to
> > find some spare time to work out if that would actual do the same
> > thing, I have a vague feeling there was something here.
> >
>
> Ok found some time to refresh my memory on this.
>
> The main issue here was as this is processing raw data for the
> SPI we shouldn't assume the data is a multiple of 4-bytes. You
> could write the code such that you used put_unaligned_le32 for
> most of the data and then special case the remainder, which would
> probably be slightly faster. But for the remainder you either end
> with the code here anyway or a special case for each of the cases
> 8,16,24 bits. So the code ends up looking much messier.
> Personally I am inclined to leave this unless performance on the
> SPI becomes a major issue.

Yes, the problem with the code is that it is a NIH existing API or patterns.
We have already in the IIO subsystem a pattern where there is a switch case
and put/get unaligned APIs per case. Perhaps this is what needs to be
factored out
for everybody.

https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ad4130.c#L472

(some shorter variants)
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ltc2497.c#L66
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ad4130.c#L472

Here is the abstraction for cameras, perhaps that's what ASoC might need.
drivers/media/v4l2-core/v4l2-cci.c.

> There is perhaps an argument for a comment in the code to explain
> this given it took me time to remember what was going on.

That's for sure.


-- 
With Best Regards,
Andy Shevchenko