Fri, Aug 04, 2023 at 11:46:02AM +0100, Charles Keepax kirjoitti:
> The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface
> (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed
> for portable applications. It provides a high dynamic range, stereo
> DAC for headphone output, two integrated Class D amplifiers for
> loudspeakers, and two ADCs for wired headset microphone input or
> stereo line input. PDM inputs are provided for digital microphones.
>
> The ASoC component provides the majority of the functionality of the
> device, all the audio functions.
...
> +static const unsigned int cs42l43_accdet_us[] = {
> + 20, 100, 1000, 10000, 50000, 75000, 100000, 200000
+ comma.
> +};
> +
> +static const unsigned int cs42l43_accdet_db_ms[] = {
> + 0, 125, 250, 500, 750, 1000, 1250, 1500
Ditto.
> +};
> +
> +static const unsigned int cs42l43_accdet_ramp_ms[] = { 10, 40, 90, 170 };
> +
> +static const unsigned int cs42l43_accdet_bias_sense[] = {
> + 14, 23, 41, 50, 60, 68, 86, 95, 0,
(as you done it here)
> +};
...
> +int cs42l43_set_jack(struct snd_soc_component *component,
> + struct snd_soc_jack *jack, void *d)
> +{
> + struct cs42l43_codec *priv = snd_soc_component_get_drvdata(component);
> + struct cs42l43 *cs42l43 = priv->core;
> + /* This tip sense invert is always set, HW wants an inverted signal */
> + unsigned int tip_deb = CS42L43_TIPSENSE_INV_MASK;
> + unsigned int hs2 = 0x2 << CS42L43_HSDET_MODE_SHIFT;
BIT(1) ?
> + unsigned int autocontrol = 0, pdncntl = 0;
> + int ret;
> +
> + dev_dbg(priv->dev, "Configure accessory detect\n");
> +
> + ret = pm_runtime_resume_and_get(priv->dev);
> + if (ret) {
> + dev_err(priv->dev, "Failed to resume for jack config: %d\n", ret);
> + return ret;
> + }
> + mutex_lock(&priv->jack_lock);
Use cleanup.h?
> + priv->jack_hp = jack;
> +
> + if (!jack)
> + goto done;
> +
> + ret = device_property_count_u32(cs42l43->dev, "cirrus,buttons-ohms");
> + if (ret != -EINVAL) {
> + if (ret < 0) {
> + dev_err(priv->dev, "Property cirrus,buttons-ohms malformed: %d\n",
> + ret);
> + goto error;
> + }
> +
> + if (ret > CS42L43_N_BUTTONS) {
> + ret = -EINVAL;
> + dev_err(priv->dev, "Property cirrus,buttons-ohms too many entries\n");
> + goto error;
> + }
> +
> + device_property_read_u32_array(cs42l43->dev, "cirrus,buttons-ohms",
> + priv->buttons, ret);
Strictly speaking this might fail, better to check the error code again.
> + } else {
> + priv->buttons[0] = 70;
> + priv->buttons[1] = 185;
> + priv->buttons[2] = 355;
> + priv->buttons[3] = 735;
> + }
> +
> + ret = cs42l43_find_index(priv, "cirrus,detect-us", 10000, &priv->detect_us,
> + cs42l43_accdet_us, ARRAY_SIZE(cs42l43_accdet_us));
> + if (ret < 0)
> + goto error;
> +
> + hs2 |= ret << CS42L43_AUTO_HSDET_TIME_SHIFT;
> +
> + priv->bias_low = device_property_read_bool(cs42l43->dev, "cirrus,bias-low");
> +
> + ret = cs42l43_find_index(priv, "cirrus,bias-ramp-ms", 170,
> + &priv->bias_ramp_ms, cs42l43_accdet_ramp_ms,
> + ARRAY_SIZE(cs42l43_accdet_ramp_ms));
> + if (ret < 0)
> + goto error;
> +
> + hs2 |= ret << CS42L43_HSBIAS_RAMP_SHIFT;
> +
> + ret = cs42l43_find_index(priv, "cirrus,bias-sense-microamp", 0,
> + &priv->bias_sense_ua, cs42l43_accdet_bias_sense,
> + ARRAY_SIZE(cs42l43_accdet_bias_sense));
> + if (ret < 0)
> + goto error;
> +
> + if (priv->bias_sense_ua)
> + autocontrol |= ret << CS42L43_HSBIAS_SENSE_TRIP_SHIFT;
> +
> + if (!device_property_read_bool(cs42l43->dev, "cirrus,button-automute"))
> + autocontrol |= CS42L43_S0_AUTO_ADCMUTE_DISABLE_MASK;
> +
> + ret = device_property_read_u32(cs42l43->dev, "cirrus,tip-debounce-ms",
> + &priv->tip_debounce_ms);
> + if (ret < 0 && ret != -EINVAL) {
> + dev_err(priv->dev, "Property cirrus,tip-debounce-ms malformed: %d\n", ret);
> + goto error;
> + }
> +
> + /* This tip sense invert is set normally, as TIPSENSE_INV already inverted */
> + if (device_property_read_bool(cs42l43->dev, "cirrus,tip-invert"))
> + autocontrol |= 0x1 << CS42L43_JACKDET_INV_SHIFT;
> +
> + if (device_property_read_bool(cs42l43->dev, "cirrus,tip-disable-pullup"))
> + autocontrol |= 0x1 << CS42L43_JACKDET_MODE_SHIFT;
BIT(0) ?
> + else
> + autocontrol |= 0x3 << CS42L43_JACKDET_MODE_SHIFT;
GENMASK(1, 0) ?
> +
> + ret = cs42l43_find_index(priv, "cirrus,tip-fall-db-ms", 500,
> + NULL, cs42l43_accdet_db_ms,
> + ARRAY_SIZE(cs42l43_accdet_db_ms));
> + if (ret < 0)
> + goto error;
> +
> + tip_deb |= ret << CS42L43_TIPSENSE_FALLING_DB_TIME_SHIFT;
> +
> + ret = cs42l43_find_index(priv, "cirrus,tip-rise-db-ms", 500,
> + NULL, cs42l43_accdet_db_ms,
> + ARRAY_SIZE(cs42l43_accdet_db_ms));
> + if (ret < 0)
> + goto error;
> +
> + tip_deb |= ret << CS42L43_TIPSENSE_RISING_DB_TIME_SHIFT;
> +
> + if (device_property_read_bool(cs42l43->dev, "cirrus,use-ring-sense")) {
> + unsigned int ring_deb = 0;
> +
> + priv->use_ring_sense = true;
> +
> + /* HW wants an inverted signal, so invert the invert */
> + if (!device_property_read_bool(cs42l43->dev, "cirrus,ring-invert"))
> + ring_deb |= CS42L43_RINGSENSE_INV_MASK;
> +
> + if (!device_property_read_bool(cs42l43->dev,
> + "cirrus,ring-disable-pullup"))
> + ring_deb |= CS42L43_RINGSENSE_PULLUP_PDNB_MASK;
> +
> + ret = cs42l43_find_index(priv, "cirrus,ring-fall-db-ms", 500,
> + NULL, cs42l43_accdet_db_ms,
> + ARRAY_SIZE(cs42l43_accdet_db_ms));
> + if (ret < 0)
> + goto error;
> +
> + ring_deb |= ret << CS42L43_RINGSENSE_FALLING_DB_TIME_SHIFT;
> +
> + ret = cs42l43_find_index(priv, "cirrus,ring-rise-db-ms", 500,
> + NULL, cs42l43_accdet_db_ms,
> + ARRAY_SIZE(cs42l43_accdet_db_ms));
> + if (ret < 0)
> + goto error;
> +
> + ring_deb |= ret << CS42L43_RINGSENSE_RISING_DB_TIME_SHIFT;
> + pdncntl |= CS42L43_RING_SENSE_EN_MASK;
> +
> + regmap_update_bits(cs42l43->regmap, CS42L43_RINGSENSE_DEB_CTRL,
> + CS42L43_RINGSENSE_INV_MASK |
> + CS42L43_RINGSENSE_PULLUP_PDNB_MASK |
> + CS42L43_RINGSENSE_FALLING_DB_TIME_MASK |
> + CS42L43_RINGSENSE_RISING_DB_TIME_MASK,
> + ring_deb);
> + }
> +
> + regmap_update_bits(cs42l43->regmap, CS42L43_TIPSENSE_DEB_CTRL,
> + CS42L43_TIPSENSE_INV_MASK |
> + CS42L43_TIPSENSE_FALLING_DB_TIME_MASK |
> + CS42L43_TIPSENSE_RISING_DB_TIME_MASK, tip_deb);
> + regmap_update_bits(cs42l43->regmap, CS42L43_HS2,
> + CS42L43_HSBIAS_RAMP_MASK | CS42L43_HSDET_MODE_MASK |
> + CS42L43_AUTO_HSDET_TIME_MASK, hs2);
> +
> +done:
> + ret = 0;
> +
> + regmap_update_bits(cs42l43->regmap, CS42L43_HS_BIAS_SENSE_AND_CLAMP_AUTOCONTROL,
> + CS42L43_JACKDET_MODE_MASK | CS42L43_S0_AUTO_ADCMUTE_DISABLE_MASK |
> + CS42L43_HSBIAS_SENSE_TRIP_MASK, autocontrol);
> + regmap_update_bits(cs42l43->regmap, CS42L43_PDNCNTL,
> + CS42L43_RING_SENSE_EN_MASK, pdncntl);
> +
> + dev_dbg(priv->dev, "Successfully configured accessory detect\n");
> +
> +error:
> + mutex_unlock(&priv->jack_lock);
> +
> + pm_runtime_mark_last_busy(priv->dev);
> + pm_runtime_put_autosuspend(priv->dev);
> +
> + return ret;
> +}
...
> +static void cs42l43_start_hs_bias(struct cs42l43_codec *priv, bool force_high)
> +{
> + struct cs42l43 *cs42l43 = priv->core;
> + unsigned int val = 0x3 << CS42L43_HSBIAS_MODE_SHIFT;
GENMASK() ?
> +
> + dev_dbg(priv->dev, "Start headset bias\n");
> +
> + regmap_update_bits(cs42l43->regmap, CS42L43_HS2,
> + CS42L43_HS_CLAMP_DISABLE_MASK, CS42L43_HS_CLAMP_DISABLE_MASK);
> +
> + if (!force_high && priv->bias_low)
> + val = 0x2 << CS42L43_HSBIAS_MODE_SHIFT;
BIT(1) ?
> + regmap_update_bits(cs42l43->regmap, CS42L43_MIC_DETECT_CONTROL_1,
> + CS42L43_HSBIAS_MODE_MASK, val);
> +
> + msleep(priv->bias_ramp_ms);
> +}
...
> +static void cs42l43_start_button_detect(struct cs42l43_codec *priv)
> +{
> + struct cs42l43 *cs42l43 = priv->core;
> + unsigned int val = 0x3 << CS42L43_BUTTON_DETECT_MODE_SHIFT;
GENMASK() ?
> + dev_dbg(priv->dev, "Start button detect\n");
> +
> + priv->button_detect_running = true;
> +
> + if (priv->bias_low)
> + val = 0x1 << CS42L43_BUTTON_DETECT_MODE_SHIFT;
BIT(0) ?
> + regmap_update_bits(cs42l43->regmap, CS42L43_MIC_DETECT_CONTROL_1,
> + CS42L43_BUTTON_DETECT_MODE_MASK |
> + CS42L43_MIC_LVL_DET_DISABLE_MASK, val);
> +
> + if (priv->bias_sense_ua) {
> + regmap_update_bits(cs42l43->regmap,
> + CS42L43_HS_BIAS_SENSE_AND_CLAMP_AUTOCONTROL,
> + CS42L43_HSBIAS_SENSE_EN_MASK |
> + CS42L43_AUTO_HSBIAS_CLAMP_EN_MASK,
> + CS42L43_HSBIAS_SENSE_EN_MASK |
> + CS42L43_AUTO_HSBIAS_CLAMP_EN_MASK);
> + }
> +}
...
Okay, looks like those shifted values more like individual numbers (not bits or
masks), ideally they would be defined with the proper names...
...
> + int timeout_ms = ((2 * priv->detect_us) / 1000) + 200;
USEC_PER_MSEC ?
...
> +static const char * const cs42l43_jack_text[] = {
> + "None", "CTIA", "OMTP", "Headphone", "Line-Out",
> + "Line-In", "Microphone", "Optical",
Better to have this by power of 2 blocks (seems it's related to the possible
values ranges in the HW).
If it's just a coincidence that there are 8 of them, perhaps other (logical)
grouping is better?
> +};
...
> + BUILD_BUG_ON(ARRAY_SIZE(cs42l43_jack_override_modes) !=
> + ARRAY_SIZE(cs42l43_jack_text) - 1);
Use static_assert() instead.
...
> +#define CS42L43_IRQ_COMPLETE(name) \
> +static irqreturn_t cs42l43_##name(int irq, void *data) \
> +{ \
> + struct cs42l43_codec *priv = data; \
> + dev_dbg(priv->dev, #name " completed\n"); \
> + complete(&priv->name); \
> + return IRQ_HANDLED; \
> +}
> +
> +CS42L43_IRQ_COMPLETE(pll_ready)
> +CS42L43_IRQ_COMPLETE(hp_startup)
> +CS42L43_IRQ_COMPLETE(hp_shutdown)
> +CS42L43_IRQ_COMPLETE(type_detect)
> +CS42L43_IRQ_COMPLETE(spkr_shutdown)
> +CS42L43_IRQ_COMPLETE(spkl_shutdown)
> +CS42L43_IRQ_COMPLETE(spkr_startup)
> +CS42L43_IRQ_COMPLETE(spkl_startup)
> +CS42L43_IRQ_COMPLETE(load_detect)
#undef?
...
> +static void cs42l43_mask_to_slots(struct cs42l43_codec *priv, unsigned int mask, int *slots)
> +{
> + int i;
> +
> + for (i = 0; i < CS42L43_ASP_MAX_CHANNELS; ++i) {
> + int slot = ffs(mask) - 1;
> +
> + if (slot < 0)
> + return;
> +
> + slots[i] = slot;
> +
> + mask &= ~(1 << slot);
> + }
for_each_set_bit() ?
> + if (mask)
> + dev_warn(priv->dev, "Too many channels in TDM mask\n");
> +}
...
> +static int cs42l43_decim_get(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> + struct cs42l43_codec *priv = snd_soc_component_get_drvdata(component);
> + int ret;
> +
> + ret = cs42l43_shutter_get(priv, CS42L43_STATUS_MIC_SHUTTER_MUTE_SHIFT);
> + if (ret < 0)
> + return ret;
> + else if (!ret)
Reundant 'else'
> + ucontrol->value.integer.value[0] = ret;
> + else
> + ret = cs42l43_dapm_get_volsw(kcontrol, ucontrol);
and why not positive check?
> + return ret;
Or even simply as
if (ret > 0)
ret = cs42l43_dapm_get_volsw(kcontrol, ucontrol);
else if (ret == 0)
ucontrol->value.integer.value[0] = ret;
return ret;
> +}
...
> +static int cs42l43_spk_get(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
As per above.
...
> + while (freq > cs42l43_pll_configs[ARRAY_SIZE(cs42l43_pll_configs) - 1].freq) {
> + div++;
> + freq /= 2;
> + }
fls() / fls_long()?
...
> + if (!time_left)
> + return -ETIMEDOUT;
> + else
> + return 0;
Redundant 'else'.
...
> + // Don't use devm as we need to get against the MFD device
This is weird...
> + priv->mclk = clk_get_optional(cs42l43->dev, "mclk");
> + if (IS_ERR(priv->mclk)) {
> + dev_err_probe(priv->dev, PTR_ERR(priv->mclk), "Failed to get mclk\n");
> + goto err_pm;
> + }
> +
> + ret = devm_snd_soc_register_component(priv->dev, &cs42l43_component_drv,
> + cs42l43_dais, ARRAY_SIZE(cs42l43_dais));
> + if (ret) {
> + dev_err_probe(priv->dev, ret, "Failed to register component\n");
> + goto err_clk;
> + }
> +
> + pm_runtime_mark_last_busy(priv->dev);
> + pm_runtime_put_autosuspend(priv->dev);
> +
> + return 0;
> +
> +err_clk:
> + clk_put(priv->mclk);
> +err_pm:
> + pm_runtime_put_sync(priv->dev);
> +
> + return ret;
> +}
> +
> +static int cs42l43_codec_remove(struct platform_device *pdev)
> +{
> + struct cs42l43_codec *priv = platform_get_drvdata(pdev);
> +
> + clk_put(priv->mclk);
You have clocks put before anything else, and your remove order is broken now.
To fix this (in case you may not used devm_clk_get() call), you should drop
devm calls all way after the clk_get(). Do we have
snd_soc_register_component()? If yes, use it to fix.
I believe you never tested rmmod/modprobe in a loop.
> + return 0;
> +}
...
> +static const struct dev_pm_ops cs42l43_codec_pm_ops = {
> + SET_RUNTIME_PM_OPS(NULL, cs42l43_codec_runtime_resume, NULL)
> +};
Why not new PM macros?
...
> + .pm = &cs42l43_codec_pm_ops,
pm_sleep_ptr()?
...
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/regmap.h>
> +#include <linux/soundwire/sdw.h>
> +#include <linux/types.h>
> +#include <sound/cs42l43.h>
> +#include <sound/pcm.h>
> +#include <sound/soc-jack.h>
This block is messed up. Some headers can be replaced by forward declarations,
some are missing... Please, follow IWYU principle.
...
> +#ifndef CS42L43_ASOC_INT_H
> +#define CS42L43_ASOC_INT_H
Why not guarding other inclusions? It makes build slower!
--
With Best Regards,
Andy Shevchenko
