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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: broonie@kernel.org, lee@kernel.org, robh+dt@kernel.org,
	 krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	 linus.walleij@linaro.org, vkoul@kernel.org, lgirdwood@gmail.com,
	 yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com,
	 pierre-louis.bossart@linux.intel.com,
	alsa-devel@alsa-project.org,  patches@opensource.cirrus.com,
	devicetree@vger.kernel.org,  linux-gpio@vger.kernel.org,
	linux-spi@vger.kernel.org,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 5/6] spi: cs42l43: Add SPI controller support
Date: Wed, 31 Jan 2024 22:17:15 +0200	[thread overview]
Message-ID: <CAHp75Vda3nZn8KwqSRvKQL9oeH_PjTnDDPuQytJvTgb2_HDvBQ@mail.gmail.com> (raw)
In-Reply-To: <ZbpqPuDj/v07yZ5y@ediswmail9.ad.cirrus.com>

On Wed, Jan 31, 2024 at 5:41 PM Charles Keepax
<ckeepax@opensource.cirrus.com> 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@gmail.com 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