Skip to contentSkip to conversation

ASoC: cs42l43: Tidy up header includes

PATCH Superseded Charles Keepax 3 participants 13 messages Jan 25, 2024 – Jan 29, 2024

Subsystems (3)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

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

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

---

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(-)
sound/soc/codecs/cs42l43-jack.c +5 -0
index 24a598f2ed9a3..1d8d7bf0a6b0d 100644
@@ -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"
sound/soc/codecs/cs42l43-sdw.c +1 -0
index 388f95853b699..60c00c05da055 100644
@@ -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>
sound/soc/codecs/cs42l43.c +8 -0
index 6a64681767de8..f2332f90f8337 100644
@@ -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>
sound/soc/codecs/cs42l43.h +12 -9
index 125e36861d5d5..9924c13e1eb53 100644
@@ -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;
Signature · 1 line
2.30.2
#1 lore raw

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

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

---

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(-)
sound/soc/codecs/cs42l43-jack.c +5 -5
index 1d8d7bf0a6b0d..4f7a405b7e06a 100644
@@ -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;
 
sound/soc/codecs/cs42l43.c +4 -8
index f2332f90f8337..d418c0b0ce9aa 100644
@@ -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;
 }
Signature · 1 line
2.30.2
#2 lore raw

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

In reply to 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.

---

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(-)
sound/soc/codecs/cs42l43-jack.c +7 -2
index 4f7a405b7e06a..67ccdc8bab6f6 100644
@@ -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;
Signature · 1 line
2.30.2
#3 lore raw

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

In reply to Charles Keepax

Add missing pm_ptr around the power ops.

---

Changes since v1:
- Added commit message

Thanks,
Charles

 sound/soc/codecs/cs42l43.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/codecs/cs42l43.c +1 -1
index d418c0b0ce9aa..1852cb072bd0e 100644
@@ -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,
Signature · 1 line
2.30.2
#4 lore raw

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

Quoted 2 lines · Charles Keepax

Add missing pm_ptr around the power ops.

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

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#5 lore raw

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

In reply to Charles Keepax

Use USEC_PER_MSEC rather than the hard coded value of 1000.

---

Changes since v1:
- Added commit message

Thanks,
Charles

 sound/soc/codecs/cs42l43-jack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
sound/soc/codecs/cs42l43-jack.c +2 -1
index 67ccdc8bab6f6..901b9dbcf5854 100644
@@ -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;
 
Signature · 1 line
2.30.2
#6 lore raw

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

In reply to Charles Keepax

Refactor the code in cs42l43_mask_to_slots() to use for_each_set_bit().

---

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(-)
sound/soc/codecs/cs42l43.c +20 -15
index 1852cb072bd0e..23e9557494afa 100644
@@ -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;
Signature · 1 line
2.30.2
#7 lore raw

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

Quoted 2 lines · Charles Keepax

Refactor the code in cs42l43_mask_to_slots() to use for_each_set_bit().

...

sound/soc/codecs/cs42l43.c line 14

Quoted 3 lines
sound/soc/codecs/cs42l43.c +2 -0
+#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).

sound/soc/codecs/cs42l43.c static int cs42l43_asp_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) · line 566

Quoted 9 lines
sound/soc/codecs/cs42l43.c +6 -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;
        }
       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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#8 lore raw

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

Quoted 16 lines · Andy Shevchenko

On Thu, Jan 25, 2024 at 12:31 PM Charles Keepax <[email protected]> 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).

sound/soc/codecs/cs42l43.c static int cs42l43_asp_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) · line 566

Quoted 9 lines
sound/soc/codecs/cs42l43.c +6 -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;
        }
Quoted 8 lines
       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

#9 lore raw

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

Quoted 24 lines · Charles Keepax
Quoted 23 lines · Andy Shevchenko

On Thu, Jan 25, 2024 at 12:31 PM Charles Keepax <[email protected]> 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).

sound/soc/codecs/cs42l43.c static int cs42l43_asp_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) · line 566

Quoted 9 lines
sound/soc/codecs/cs42l43.c +6 -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;
        }
       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");
Quoted 8 lines
Quoted 1 line

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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#10 lore raw

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

In reply to Charles Keepax

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

---

Changes since v1:
- Change / into >>

Thanks,
Charles

 sound/soc/codecs/cs42l43.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
sound/soc/codecs/cs42l43.c +3 -4
index 23e9557494afa..2c402086924d3 100644
@@ -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;
Signature · 1 line
2.30.2
maintainer
#12 lore raw
In reply to Charles Keepax
Quoted 4 lines · Charles Keepax

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