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

From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: <andy.shevchenko@gmail.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 15:41:50 +0000	[thread overview]
Message-ID: <ZbpqPuDj/v07yZ5y@ediswmail9.ad.cirrus.com> (raw)
In-Reply-To: <20240119114917.GB16899@ediswmail.ad.cirrus.com>

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.

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.

Thanks,
Charles