[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.
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;
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; }
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;
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,
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.
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;
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;
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.
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;
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
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
participants (3)
-
Andy Shevchenko -
Charles Keepax -
Mark Brown