[PATCH v2 1/7] ASoC: cs42l43: Tidy up header includes

[PATCH v2 1/7] ASoC: cs42l43: Tidy up header includes

[Charles Keepax]

Use more forward declarations, move header guards to cover other
includes, and rely less on including headers through other headers.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Shuffle forward declarations around

Thanks,
Charles

 sound/soc/codecs/cs42l43-jack.c |  5 +++++
 sound/soc/codecs/cs42l43-sdw.c  |  1 +
 sound/soc/codecs/cs42l43.c      |  8 ++++++++
 sound/soc/codecs/cs42l43.h      | 21 ++++++++++++---------
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/cs42l43-jack.c b/sound/soc/codecs/cs42l43-jack.c
index 24a598f2ed9a3..1d8d7bf0a6b0d 100644
--- a/sound/soc/codecs/cs42l43-jack.c
+++ b/sound/soc/codecs/cs42l43-jack.c
@@ -6,19 +6,24 @@
 //                         Cirrus Logic International Semiconductor Ltd.
 
 #include <linux/build_bug.h>
+#include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/irq.h>
 #include <linux/jiffies.h>
 #include <linux/mfd/cs42l43.h>
 #include <linux/mfd/cs42l43-regs.h>
+#include <linux/mutex.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/workqueue.h>
 #include <sound/control.h>
 #include <sound/jack.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc-component.h>
+#include <sound/soc-jack.h>
 #include <sound/soc.h>
 
 #include "cs42l43.h"
diff --git a/sound/soc/codecs/cs42l43-sdw.c b/sound/soc/codecs/cs42l43-sdw.c
index 388f95853b699..60c00c05da055 100644
--- a/sound/soc/codecs/cs42l43-sdw.c
+++ b/sound/soc/codecs/cs42l43-sdw.c
@@ -9,6 +9,7 @@
 #include <linux/mfd/cs42l43.h>
 #include <linux/mfd/cs42l43-regs.h>
 #include <linux/module.h>
+#include <linux/soundwire/sdw.h>
 #include <sound/pcm.h>
 #include <sound/sdw.h>
 #include <sound/soc-component.h>
diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
index 6a64681767de8..f2332f90f8337 100644
--- a/sound/soc/codecs/cs42l43.c
+++ b/sound/soc/codecs/cs42l43.c
@@ -6,17 +6,25 @@
 //                         Cirrus Logic International Semiconductor Ltd.
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/gcd.h>
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/jiffies.h>
 #include <linux/mfd/cs42l43.h>
 #include <linux/mfd/cs42l43-regs.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
 #include <linux/string.h>
+#include <linux/workqueue.h>
 #include <sound/control.h>
+#include <sound/cs42l43.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc-component.h>
diff --git a/sound/soc/codecs/cs42l43.h b/sound/soc/codecs/cs42l43.h
index 125e36861d5d5..9924c13e1eb53 100644
--- a/sound/soc/codecs/cs42l43.h
+++ b/sound/soc/codecs/cs42l43.h
@@ -6,19 +6,14 @@
  *                         Cirrus Logic International Semiconductor Ltd.
  */
 
-#include <linux/clk.h>
+#ifndef CS42L43_ASOC_INT_H
+#define CS42L43_ASOC_INT_H
+
 #include <linux/completion.h>
-#include <linux/device.h>
 #include <linux/mutex.h>
-#include <linux/regmap.h>
-#include <linux/soundwire/sdw.h>
 #include <linux/types.h>
-#include <sound/cs42l43.h>
+#include <linux/workqueue.h>
 #include <sound/pcm.h>
-#include <sound/soc-jack.h>
-
-#ifndef CS42L43_ASOC_INT_H
-#define CS42L43_ASOC_INT_H
 
 #define CS42L43_INTERNAL_SYSCLK		24576000
 #define CS42L43_DEFAULT_SLOTS		0x3F
@@ -37,6 +32,14 @@
 
 #define CS42L43_N_BUTTONS	6
 
+struct clk;
+struct device;
+
+struct snd_soc_component;
+struct snd_soc_jack;
+
+struct cs42l43;
+
 struct cs42l43_codec {
 	struct device *dev;
 	struct cs42l43 *core;
-- 
2.30.2

[PATCH v2 2/7] ASoC: cs42l43: Minor code tidy ups

[Charles Keepax]

Add some missing commas, refactor a couple small bits of code.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Move static assert

Thanks,
Charles

 sound/soc/codecs/cs42l43-jack.c | 10 +++++-----
 sound/soc/codecs/cs42l43.c      | 12 ++++--------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/sound/soc/codecs/cs42l43-jack.c b/sound/soc/codecs/cs42l43-jack.c
index 1d8d7bf0a6b0d..4f7a405b7e06a 100644
--- a/sound/soc/codecs/cs42l43-jack.c
+++ b/sound/soc/codecs/cs42l43-jack.c
@@ -29,11 +29,11 @@
 #include "cs42l43.h"
 
 static const unsigned int cs42l43_accdet_us[] = {
-	20, 100, 1000, 10000, 50000, 75000, 100000, 200000
+	20, 100, 1000, 10000, 50000, 75000, 100000, 200000,
 };
 
 static const unsigned int cs42l43_accdet_db_ms[] = {
-	0, 125, 250, 500, 750, 1000, 1250, 1500
+	0, 125, 250, 500, 750, 1000, 1250, 1500,
 };
 
 static const unsigned int cs42l43_accdet_ramp_ms[] = { 10, 40, 90, 170 };
@@ -851,6 +851,9 @@ static const char * const cs42l43_jack_text[] = {
 	"Line-In", "Microphone", "Optical",
 };
 
+static_assert(ARRAY_SIZE(cs42l43_jack_override_modes) ==
+	      ARRAY_SIZE(cs42l43_jack_text) - 1);
+
 SOC_ENUM_SINGLE_VIRT_DECL(cs42l43_jack_enum, cs42l43_jack_text);
 
 int cs42l43_jack_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)
@@ -873,9 +876,6 @@ int cs42l43_jack_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *u
 	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
 	unsigned int override = ucontrol->value.integer.value[0];
 
-	BUILD_BUG_ON(ARRAY_SIZE(cs42l43_jack_override_modes) !=
-		     ARRAY_SIZE(cs42l43_jack_text) - 1);
-
 	if (override >= e->items)
 		return -EINVAL;
 
diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
index f2332f90f8337..d418c0b0ce9aa 100644
--- a/sound/soc/codecs/cs42l43.c
+++ b/sound/soc/codecs/cs42l43.c
@@ -1059,12 +1059,10 @@ static int cs42l43_decim_get(struct snd_kcontrol *kcontrol,
 	int ret;
 
 	ret = cs42l43_shutter_get(priv, CS42L43_STATUS_MIC_SHUTTER_MUTE_SHIFT);
-	if (ret < 0)
-		return ret;
+	if (ret > 0)
+		ret = cs42l43_dapm_get_volsw(kcontrol, ucontrol);
 	else if (!ret)
 		ucontrol->value.integer.value[0] = ret;
-	else
-		ret = cs42l43_dapm_get_volsw(kcontrol, ucontrol);
 
 	return ret;
 }
@@ -1077,12 +1075,10 @@ static int cs42l43_spk_get(struct snd_kcontrol *kcontrol,
 	int ret;
 
 	ret = cs42l43_shutter_get(priv, CS42L43_STATUS_SPK_SHUTTER_MUTE_SHIFT);
-	if (ret < 0)
-		return ret;
+	if (ret > 0)
+		ret = snd_soc_get_volsw(kcontrol, ucontrol);
 	else if (!ret)
 		ucontrol->value.integer.value[0] = ret;
-	else
-		ret = snd_soc_get_volsw(kcontrol, ucontrol);
 
 	return ret;
 }
-- 
2.30.2

[PATCH v2 3/7] ASoC: cs42l43: Check error from device_property_read_u32_array()

[Charles Keepax]

Whilst reading cirrus,buttons-ohms the error from
device_property_read_u32_array() is not checked, whilst there is a
preceding device_property_count_u32() which is checked the property
read can still fail. Add the missing check.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Add () to funcs in commit message

Thanks,
Charles

 sound/soc/codecs/cs42l43-jack.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/cs42l43-jack.c b/sound/soc/codecs/cs42l43-jack.c
index 4f7a405b7e06a..67ccdc8bab6f6 100644
--- a/sound/soc/codecs/cs42l43-jack.c
+++ b/sound/soc/codecs/cs42l43-jack.c
@@ -106,8 +106,13 @@ int cs42l43_set_jack(struct snd_soc_component *component,
 			goto error;
 		}
 
-		device_property_read_u32_array(cs42l43->dev, "cirrus,buttons-ohms",
-					       priv->buttons, ret);
+		ret = device_property_read_u32_array(cs42l43->dev, "cirrus,buttons-ohms",
+						     priv->buttons, ret);
+		if (ret < 0) {
+			dev_err(priv->dev, "Property cirrus,button-ohms malformed: %d\n",
+				ret);
+			goto error;
+		}
 	} else {
 		priv->buttons[0] = 70;
 		priv->buttons[1] = 185;
-- 
2.30.2

[PATCH v2 4/7] ASoC: cs42l43: Add pm_ptr around the power ops

[Charles Keepax]

Add missing pm_ptr around the power ops.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Added commit message

Thanks,
Charles

 sound/soc/codecs/cs42l43.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
index d418c0b0ce9aa..1852cb072bd0e 100644
--- a/sound/soc/codecs/cs42l43.c
+++ b/sound/soc/codecs/cs42l43.c
@@ -2349,7 +2349,7 @@ MODULE_DEVICE_TABLE(platform, cs42l43_codec_id_table);
 static struct platform_driver cs42l43_codec_driver = {
 	.driver = {
 		.name	= "cs42l43-codec",
-		.pm	= &cs42l43_codec_pm_ops,
+		.pm	= pm_ptr(&cs42l43_codec_pm_ops),
 	},
 
 	.probe		= cs42l43_codec_probe,
-- 
2.30.2

[PATCH v2 5/7] ASoC: cs42l43: Use USEC_PER_MSEC rather than hard coding

[Charles Keepax]

Use USEC_PER_MSEC rather than the hard coded value of 1000.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Added commit message

Thanks,
Charles

 sound/soc/codecs/cs42l43-jack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/cs42l43-jack.c b/sound/soc/codecs/cs42l43-jack.c
index 67ccdc8bab6f6..901b9dbcf5854 100644
--- a/sound/soc/codecs/cs42l43-jack.c
+++ b/sound/soc/codecs/cs42l43-jack.c
@@ -17,6 +17,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
+#include <linux/time.h>
 #include <linux/workqueue.h>
 #include <sound/control.h>
 #include <sound/jack.h>
@@ -647,7 +648,7 @@ static int cs42l43_run_load_detect(struct cs42l43_codec *priv, bool mic)
 static int cs42l43_run_type_detect(struct cs42l43_codec *priv)
 {
 	struct cs42l43 *cs42l43 = priv->core;
-	int timeout_ms = ((2 * priv->detect_us) / 1000) + 200;
+	int timeout_ms = ((2 * priv->detect_us) / USEC_PER_MSEC) + 200;
 	unsigned int type = 0xff;
 	unsigned long time_left;
 
-- 
2.30.2

[PATCH v2 6/7] ASoC: cs42l43: Refactor to use for_each_set_bit()

[Charles Keepax]

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(-)

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;
-- 
2.30.2

[PATCH v2 7/7] ASoC: cs42l43: Use fls to calculate the pre-divider for the PLL

[Charles Keepax]

Use fls to calculate the pre-divider and input frequency for the PLL,
this is marginally faster than the previous loop.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Change / into >>

Thanks,
Charles

 sound/soc/codecs/cs42l43.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
index 23e9557494afa..2c402086924d3 100644
--- a/sound/soc/codecs/cs42l43.c
+++ b/sound/soc/codecs/cs42l43.c
@@ -1338,10 +1338,9 @@ static int cs42l43_enable_pll(struct cs42l43_codec *priv)
 
 	dev_dbg(priv->dev, "Enabling PLL at %uHz\n", freq);
 
-	while (freq > cs42l43_pll_configs[ARRAY_SIZE(cs42l43_pll_configs) - 1].freq) {
-		div++;
-		freq /= 2;
-	}
+	div = fls(freq) -
+	      fls(cs42l43_pll_configs[ARRAY_SIZE(cs42l43_pll_configs) - 1].freq);
+	freq >>= div;
 
 	if (div <= CS42L43_PLL_REFCLK_DIV_MASK) {
 		int i;
-- 
2.30.2

Re: [PATCH v2 4/7] ASoC: cs42l43: Add pm_ptr around the power ops

[Andy Shevchenko]

On Thu, Jan 25, 2024 at 12:31 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> Add missing pm_ptr around the power ops.

pm_ptr() (here and in the Subject), but it's a minor one.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/7] ASoC: cs42l43: Refactor to use for_each_set_bit()

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/7] ASoC: cs42l43: Tidy up header includes

[Andy Shevchenko]

On Thu, Jan 25, 2024 at 12:31 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> Use more forward declarations, move header guards to cover other
> includes, and rely less on including headers through other headers.

For patches 1-5,7

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/7] ASoC: cs42l43: Tidy up header includes

[Mark Brown]

On Thu, 25 Jan 2024 10:31:11 +0000, Charles Keepax wrote:
> Use more forward declarations, move header guards to cover other
> includes, and rely less on including headers through other headers.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/7] ASoC: cs42l43: Tidy up header includes
      commit: fb430b06397e5eebefd42584fe4dfabf2a3632e0
[2/7] ASoC: cs42l43: Minor code tidy ups
      commit: 40f6281c1e7d733399bd42fe97a0aae00b967a91
[3/7] ASoC: cs42l43: Check error from device_property_read_u32_array()
      commit: a2e7cf55db781654fdb2d3b2529e28c4d93e24fc
[4/7] ASoC: cs42l43: Add pm_ptr around the power ops
      commit: 7a93a9abe44386b4caa0e67977f41b8c9f06b51c
[5/7] ASoC: cs42l43: Use USEC_PER_MSEC rather than hard coding
      commit: 96c716887c1a918d4cb4610f5cf111280fda48f0
[6/7] ASoC: cs42l43: Refactor to use for_each_set_bit()
      commit: fe04d1632cb4130fb47d18fe70ac292562a3b9c3
[7/7] ASoC: cs42l43: Use fls to calculate the pre-divider for the PLL
      commit: 31c6e53a4da5fe626b99e1ebf777d751994e3281

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v2 6/7] ASoC: cs42l43: Refactor to use for_each_set_bit()

[Charles Keepax]

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

Re: [PATCH v2 6/7] ASoC: cs42l43: Refactor to use for_each_set_bit()

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko