| Message ID | 20240125103117.2622095-6-ckeepax@opensource.cirrus.com |
|---|---|
| State | Accepted |
| Commit | fe04d1632cb4130fb47d18fe70ac292562a3b9c3 |
| Headers | show |
| Series | None | expand |
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(-)