ASoC: cs42l43: Tidy up header includes
Subsystems (3)
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
@@ -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
@@ -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
@@ -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
@@ -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
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
@@ -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 *ustruct 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
@@ -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
PATCH v2 3/7 ASoC: cs42l43: Check error from device_property_read_u32_array()
In reply to Charles KeepaxWhilst 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
@@ -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
PATCH v2 4/7 ASoC: cs42l43: Add pm_ptr around the power ops
In reply to Charles KeepaxAdd 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
@@ -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
Re: [PATCH v2 4/7] ASoC: cs42l43: Add pm_ptr around the power ops
Quoted 2 lines · Charles Keepax
Add missing
pm_ptraround the power ops.
pm_ptr() (here and in the Subject), but it's a minor one.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 5/7 ASoC: cs42l43: Use USEC_PER_MSEC rather than hard coding
In reply to Charles KeepaxUse 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
@@ -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
PATCH v2 6/7 ASoC: cs42l43: Refactor to use for_each_set_bit()
In reply to Charles KeepaxRefactor 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
@@ -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
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 usefor_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
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
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_bitsappears to measure against the bit position not the number of set bits. So for example0xFC000000would 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
PATCH v2 7/7 ASoC: cs42l43: Use fls to calculate the pre-divider for the PLL
In reply to Charles KeepaxUse 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
@@ -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
Quoted 3 lines · Charles Keepax
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
Signature · 2 lines
With Best Regards, Andy Shevchenko
Mark Brown
maintainerQuoted 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: 31c6e53a4da5fe626b99e1ebf777d751994e3281All 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