| Message ID | 20240125103117.2622095-6-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fe04d1632cb4130fb47d18fe70ac292562a3b9c3 |
| Headers | show |
| Series | [v2,1/7] ASoC: cs42l43: Tidy up header includes | expand |
On Thu, Jan 25, 2024 at 12:31 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > Refactor the code in cs42l43_mask_to_slots() to use for_each_set_bit(). ... > +#include <linux/bits.h> > +#include <linux/find.h> Of course you can leave them, but I think we more or less _guarantee_ their inclusion by bitops.h, otherwise bitops.h will require those two in _each_ instance of use which sounds not such a clever decision. That said, I would avoid adding them here as the compiler would need to mmap() the first page of each header, check the guard and unmap, and repeat for each header. This will slow down the build for no particular reason. ... Adding nslots parameter is a good idea, but I still think the code can be refactored better (have you checked the code generation, btw? I believe my version would be better or not worse). > + for_each_set_bit(slot, &mask, BITS_PER_TYPE(mask)) { > + if (i == nslots) { > + dev_warn(priv->dev, "Too many channels in TDM mask: %lx\n", > + mask); > return; > + } > > + slots[i++] = slot; > } i = 0; for_each_set_bit(slot, &mask, CS42L43_ASP_MAX_CHANNELS) slots[i++] = slot; if (hweight_long(mask) >= CS42L43_ASP_MAX_CHANNELS) dev_warn(priv->dev, "Too many channels in TDM mask\n"); The code is simpler and behaviour is not changed.
On Thu, Jan 25, 2024 at 09:11:44PM +0200, Andy Shevchenko wrote: > On Thu, Jan 25, 2024 at 12:31 PM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > Adding nslots parameter is a good idea, but I still think the code can > be refactored better (have you checked the code generation, btw? I > believe my version would be better or not worse). > > > + for_each_set_bit(slot, &mask, BITS_PER_TYPE(mask)) { > > + if (i == nslots) { > > + dev_warn(priv->dev, "Too many channels in TDM mask: %lx\n", > > + mask); > > return; > > + } > > > > + slots[i++] = slot; > > } > > i = 0; > for_each_set_bit(slot, &mask, CS42L43_ASP_MAX_CHANNELS) > slots[i++] = slot; > > if (hweight_long(mask) >= CS42L43_ASP_MAX_CHANNELS) > dev_warn(priv->dev, "Too many channels in TDM mask\n"); > > The code is simpler and behaviour is not changed. I don't think this works, the limit here is on the number of channels not on the position of those channels. The last parameter of for_each_set_bits appears to measure against the bit position not the number of set bits. So for example 0xFC000000 would be a valid 6 channel mask, but would result in no slot positions being set in the above code. Thanks, Charles
On Mon, Jan 29, 2024 at 6:27 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > On Thu, Jan 25, 2024 at 09:11:44PM +0200, Andy Shevchenko wrote: > > On Thu, Jan 25, 2024 at 12:31 PM Charles Keepax > > <ckeepax@opensource.cirrus.com> wrote: > > Adding nslots parameter is a good idea, but I still think the code can > > be refactored better (have you checked the code generation, btw? I > > believe my version would be better or not worse). > > > > > + for_each_set_bit(slot, &mask, BITS_PER_TYPE(mask)) { > > > + if (i == nslots) { > > > + dev_warn(priv->dev, "Too many channels in TDM mask: %lx\n", > > > + mask); > > > return; > > > + } > > > > > > + slots[i++] = slot; > > > } > > > > i = 0; > > for_each_set_bit(slot, &mask, CS42L43_ASP_MAX_CHANNELS) > > slots[i++] = slot; > > > > if (hweight_long(mask) >= CS42L43_ASP_MAX_CHANNELS) > > dev_warn(priv->dev, "Too many channels in TDM mask\n"); > > > > The code is simpler and behaviour is not changed. > > I don't think this works, the limit here is on the number of > channels not on the position of those channels. The last parameter > of for_each_set_bits appears to measure against the bit position > not the number of set bits. So for example 0xFC000000 would be a > valid 6 channel mask, but would result in no slot positions being > set in the above code. Ah, indeed. Then BITS_PER_LONG is the correct approach.
diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c index 1852cb072bd0e..23e9557494afa 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, unsigned int nslots) { - 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, BITS_PER_TYPE(mask)) { + if (i == nslots) { + 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, @@ -580,8 +581,10 @@ static int cs42l43_asp_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mas rx_mask = CS42L43_DEFAULT_SLOTS; } - cs42l43_mask_to_slots(priv, tx_mask, priv->tx_slots); - cs42l43_mask_to_slots(priv, rx_mask, priv->rx_slots); + cs42l43_mask_to_slots(priv, tx_mask, priv->tx_slots, + ARRAY_SIZE(priv->tx_slots)); + cs42l43_mask_to_slots(priv, rx_mask, priv->rx_slots, + ARRAY_SIZE(priv->rx_slots)); return 0; } @@ -2098,8 +2101,10 @@ static int cs42l43_component_probe(struct snd_soc_component *component) snd_soc_component_init_regmap(component, cs42l43->regmap); - cs42l43_mask_to_slots(priv, CS42L43_DEFAULT_SLOTS, priv->tx_slots); - cs42l43_mask_to_slots(priv, CS42L43_DEFAULT_SLOTS, priv->rx_slots); + cs42l43_mask_to_slots(priv, CS42L43_DEFAULT_SLOTS, priv->tx_slots, + ARRAY_SIZE(priv->tx_slots)); + cs42l43_mask_to_slots(priv, CS42L43_DEFAULT_SLOTS, priv->rx_slots, + ARRAY_SIZE(priv->rx_slots)); priv->component = component; priv->constraint = cs42l43_constraint;
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> --- Changes since v1: - Added () after funcs in commit message - Use BITS_PER_TYPE - Pass size into cs42l43_mask_to_slots() Thanks, Charles sound/soc/codecs/cs42l43.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)