| Message ID | 20240124165558.1876407-6-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/7] ASoC: cs42l43: Tidy up header includes | expand |
On Wed, Jan 24, 2024 at 6:56 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > Refactor the code in cs42l43_mask_to_slots to use for_each_set_bit. ..._bit() ... > #include <linux/bitops.h> > +#include <linux/bits.h> > +#include <linux/find.h> No need, it's included by bitops.h (and there is kinda guarantee for these). ... > + for_each_set_bit(slot, &mask, sizeof(mask) * BITS_PER_BYTE) { BITS_PER_TYPE() ? But actually it's unsigned long, so BITS_PER_LONG should suffice. BUT. Why not use ..._MAX_CHANNELS here directly as it was in the original loop? You might need a static_assert() that tells you it's not longer than bits in the mask, but it probably is an overkill check. ... > + if (i == CS42L43_ASP_MAX_CHANNELS) { > + dev_warn(priv->dev, "Too many channels in TDM mask: %lx\n", > + mask); This is invariant to the loop, you may check even before it (I'm writing by memory, might have made mistake(s) in the snippet): nslots = hweight_long(mask); if (nslots >= ..._MAX_CHANNELS) dev_warn(...); > return; > + }
On Thu, Jan 25, 2024 at 12:42:13AM +0200, Andy Shevchenko wrote: > On Wed, Jan 24, 2024 at 6:56 PM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > > > Refactor the code in cs42l43_mask_to_slots to use for_each_set_bit. > > ..._bit() > > ... > > > #include <linux/bitops.h> > > > +#include <linux/bits.h> > > +#include <linux/find.h> > > No need, it's included by bitops.h (and there is kinda guarantee for these). > We just moved a bunch of includes out of the cs42l43 header files to be directly included in the C files. It makes sense to be consistent if each file is going to directly include each header it uses it should do so. The header guards will weed out what is already included. > > + if (i == CS42L43_ASP_MAX_CHANNELS) { > > + dev_warn(priv->dev, "Too many channels in TDM mask: %lx\n", > > + mask); > > This is invariant to the loop, you may check even before it (I'm > writing by memory, might have made mistake(s) in the snippet): > > nslots = hweight_long(mask); > if (nslots >= ..._MAX_CHANNELS) > dev_warn(...); > > > return; > > + } > This would result in a change of behaviour, as one would need to return before the loop to ensure we didn't overflow the slots array. We could possible do something with masking the slots to ensure it only has the right number of bits set, but it is really starting to get a little micro-optimisation for something that is likely only going to run once whilst the kernel boots. Thanks, Charles
diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c index 1852cb072bd0e..05ca20d4fd622 100644 --- a/sound/soc/codecs/cs42l43.c +++ b/sound/soc/codecs/cs42l43.c @@ -6,10 +6,12 @@ // Cirrus Logic International Semiconductor Ltd. #include <linux/bitops.h> +#include <linux/bits.h> #include <linux/clk.h> #include <linux/device.h> #include <linux/err.h> #include <linux/errno.h> +#include <linux/find.h> #include <linux/gcd.h> #include <linux/irq.h> #include <linux/irqdomain.h> @@ -547,23 +549,22 @@ static int cs42l43_asp_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) return 0; } -static void cs42l43_mask_to_slots(struct cs42l43_codec *priv, unsigned int mask, int *slots) +static void cs42l43_mask_to_slots(struct cs42l43_codec *priv, + unsigned long mask, int *slots) { - int i; + int i = 0; + int slot; - for (i = 0; i < CS42L43_ASP_MAX_CHANNELS; ++i) { - int slot = ffs(mask) - 1; - - if (slot < 0) + for_each_set_bit(slot, &mask, sizeof(mask) * BITS_PER_BYTE) { + if (i == CS42L43_ASP_MAX_CHANNELS) { + dev_warn(priv->dev, "Too many channels in TDM mask: %lx\n", + mask); return; + } - slots[i] = slot; - - mask &= ~(1 << slot); + slots[i++] = slot; } - if (mask) - dev_warn(priv->dev, "Too many channels in TDM mask\n"); } static int cs42l43_asp_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
Refactor the code in cs42l43_mask_to_slots to use for_each_set_bit. Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- sound/soc/codecs/cs42l43.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)