[PATCH 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 --- 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 | 19 ++++++++++--------- 4 files changed, 24 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..a491e9254568e 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,12 @@
#define CS42L43_N_BUTTONS 6
+struct device; +struct cs42l43; +struct snd_soc_component; +struct snd_soc_jack; +struct clk; + 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 --- sound/soc/codecs/cs42l43-jack.c | 8 ++++---- sound/soc/codecs/cs42l43.c | 12 ++++-------- 2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/sound/soc/codecs/cs42l43-jack.c b/sound/soc/codecs/cs42l43-jack.c index 1d8d7bf0a6b0d..d0569577a8699 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 }; @@ -873,8 +873,8 @@ 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); + static_assert(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; }
On Wed, Jan 24, 2024 at 6:56 PM Charles Keepax ckeepax@opensource.cirrus.com wrote:
Add some missing commas, refactor a couple small bits of code.
...
BUILD_BUG_ON(ARRAY_SIZE(cs42l43_jack_override_modes) !=ARRAY_SIZE(cs42l43_jack_text) - 1);
static_assert(ARRAY_SIZE(cs42l43_jack_override_modes) ==ARRAY_SIZE(cs42l43_jack_text) - 1);
Now you can move this closer to the both array definitions (as static_assert() can be global, and basically that is the idea behind it, besides nicer error messaging).
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 Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- 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 d0569577a8699..147c7017fd8b6 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;
On Wed, Jan 24, 2024 at 6:56 PM Charles Keepax ckeepax@opensource.cirrus.com wrote:
Whilst reading cirrus,buttons-ohms the error from device_property_read_u32_array is not checked, whilst there is a
..._array()
preceding device_property_count_u32 which is checked the property read
..._u32()
can still fail. Add the missing check.
Reviewed-by: Andy Shevchenko andy.shevchenko@gmail.com
Suggested-by: Andy Shevchenko andy.shevchenko@gmail.com Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- 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,
pm_ptr() (in the Subject)
On Wed, Jan 24, 2024 at 6:56 PM Charles Keepax ckeepax@opensource.cirrus.com wrote:
Commit message?
Suggested-by: Andy Shevchenko andy.shevchenko@gmail.com Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com
Suggested-by: Andy Shevchenko andy.shevchenko@gmail.com Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- 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 147c7017fd8b6..4bcf66cff566a 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;
On Wed, Jan 24, 2024 at 6:56 PM Charles Keepax ckeepax@opensource.cirrus.com wrote:
Commit message?
Suggested-by: Andy Shevchenko andy.shevchenko@gmail.com Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com
Otherwise, LGTM,
Reviewed-by: Andy Shevchenko andy.shevchenko@gmail.com
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 --- sound/soc/codecs/cs42l43.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c index 1852cb072bd0e..05ca20d4fd622 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) { - 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, sizeof(mask) * BITS_PER_BYTE) { + if (i == CS42L43_ASP_MAX_CHANNELS) { + 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,
On Wed, Jan 24, 2024 at 6:56 PM Charles Keepax ckeepax@opensource.cirrus.com wrote:
Refactor the code in cs42l43_mask_to_slots to use for_each_set_bit.
..._bit()
...
#include <linux/bitops.h>
+#include <linux/bits.h> +#include <linux/find.h>
No need, it's included by bitops.h (and there is kinda guarantee for these).
...
for_each_set_bit(slot, &mask, sizeof(mask) * BITS_PER_BYTE) {
BITS_PER_TYPE() ? But actually it's unsigned long, so BITS_PER_LONG should suffice. BUT. Why not use ..._MAX_CHANNELS here directly as it was in the original loop? You might need a static_assert() that tells you it's not longer than bits in the mask, but it probably is an overkill check.
...
if (i == CS42L43_ASP_MAX_CHANNELS) {dev_warn(priv->dev, "Too many channels in TDM mask: %lx\n",mask);
This is invariant to the loop, you may check even before it (I'm writing by memory, might have made mistake(s) in the snippet):
nslots = hweight_long(mask); if (nslots >= ..._MAX_CHANNELS) dev_warn(...);
return;
}
On Thu, Jan 25, 2024 at 12:42:13AM +0200, Andy Shevchenko wrote:
On Wed, Jan 24, 2024 at 6:56 PM Charles Keepax ckeepax@opensource.cirrus.com wrote:
Refactor the code in cs42l43_mask_to_slots to use for_each_set_bit.
..._bit()
...
#include <linux/bitops.h>
+#include <linux/bits.h> +#include <linux/find.h>
No need, it's included by bitops.h (and there is kinda guarantee for these).
We just moved a bunch of includes out of the cs42l43 header files to be directly included in the C files. It makes sense to be consistent if each file is going to directly include each header it uses it should do so. The header guards will weed out what is already included.
if (i == CS42L43_ASP_MAX_CHANNELS) {dev_warn(priv->dev, "Too many channels in TDM mask: %lx\n",mask);This is invariant to the loop, you may check even before it (I'm writing by memory, might have made mistake(s) in the snippet):
nslots = hweight_long(mask); if (nslots >= ..._MAX_CHANNELS) dev_warn(...);
return;
}
This would result in a change of behaviour, as one would need to return before the loop to ensure we didn't overflow the slots array. We could possible do something with masking the slots to ensure it only has the right number of bits set, but it is really starting to get a little micro-optimisation for something that is likely only going to run once whilst the kernel boots.
Thanks, Charles
Suggested-by: Andy Shevchenko andy.shevchenko@gmail.com Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- 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 05ca20d4fd622..ef154aa76d576 100644 --- a/sound/soc/codecs/cs42l43.c +++ b/sound/soc/codecs/cs42l43.c @@ -1336,10 +1336,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 /= 1 << div;
if (div <= CS42L43_PLL_REFCLK_DIV_MASK) { int i;
On Wed, Jan 24, 2024 at 6:56 PM Charles Keepax ckeepax@opensource.cirrus.com wrote:
Commit message?
Suggested-by: Andy Shevchenko andy.shevchenko@gmail.com Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com
...
div = fls(freq) -fls(cs42l43_pll_configs[ARRAY_SIZE(cs42l43_pll_configs) - 1].freq);freq /= 1 << div;
freq >>= div;
On Wed, Jan 24, 2024 at 6:56 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.
...
+struct device; +struct cs42l43; +struct snd_soc_component; +struct snd_soc_jack; +struct clk;
I would keep it sorted and grouped (in similar way how headers can be done, i.e. from more generic towards custom and local), like
struct clk; struct device;
struct snd_soc_component; struct snd_soc_jack;
struct cs42l43;
participants (2)
-
Andy Shevchenko -
Charles Keepax