From: andy.shevchenko@gmail.com
To: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: broonie@kernel.org, lee@kernel.org, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
linus.walleij@linaro.org, vkoul@kernel.org, lgirdwood@gmail.com,
yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com,
pierre-louis.bossart@linux.intel.com,
alsa-devel@alsa-project.org, patches@opensource.cirrus.com,
devicetree@vger.kernel.org, linux-gpio@vger.kernel.org,
linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 6/6] ASoC: cs42l43: Add support for the cs42l43
Date: Thu, 18 Jan 2024 19:41:54 +0200 [thread overview]
Message-ID: <Zali4qxdegY7H6eY@surfacebook.localdomain> (raw)
In-Reply-To: <20230804104602.395892-7-ckeepax@opensource.cirrus.com>
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
next prev parent reply other threads:[~2024-01-18 17:41 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-04 10:45 [PATCH v7 0/6] Add cs42l43 PC focused SoundWire CODEC Charles Keepax
2023-08-04 10:45 ` [PATCH v7 1/6] soundwire: bus: Allow SoundWire peripherals to register IRQ handlers Charles Keepax
2024-01-18 16:16 ` andy.shevchenko
2023-08-04 10:45 ` [PATCH v7 2/6] dt-bindings: mfd: cirrus,cs42l43: Add initial DT binding Charles Keepax
2023-08-04 10:45 ` [PATCH v7 3/6] mfd: cs42l43: Add support for cs42l43 core driver Charles Keepax
2023-08-29 14:06 ` Guenter Roeck
2023-08-29 16:22 ` Charles Keepax
2023-08-31 13:54 ` Guenter Roeck
2024-01-18 16:42 ` andy.shevchenko
2024-01-19 11:32 ` Charles Keepax
2024-01-19 16:01 ` Andy Shevchenko
2023-08-04 10:46 ` [PATCH v7 4/6] pinctrl: cs42l43: Add support for the cs42l43 Charles Keepax
2024-01-18 16:56 ` andy.shevchenko
2023-08-04 10:46 ` [PATCH v7 5/6] spi: cs42l43: Add SPI controller support Charles Keepax
2024-01-18 17:06 ` andy.shevchenko
2024-01-19 11:49 ` Charles Keepax
2024-01-19 16:09 ` Andy Shevchenko
2024-01-31 15:41 ` Charles Keepax
2024-01-31 20:17 ` Andy Shevchenko
2023-08-04 10:46 ` [PATCH v7 6/6] ASoC: cs42l43: Add support for the cs42l43 Charles Keepax
2024-01-18 17:41 ` andy.shevchenko [this message]
2024-01-18 18:11 ` Mark Brown
2024-01-18 20:46 ` Andy Shevchenko
2024-01-18 22:07 ` Mark Brown
2024-01-19 16:13 ` Andy Shevchenko
2024-01-19 16:59 ` Charles Keepax
2024-01-19 17:24 ` Andy Shevchenko
2023-08-17 11:09 ` (subset) [PATCH v7 0/6] Add cs42l43 PC focused SoundWire CODEC Lee Jones
2023-08-17 11:26 ` Lee Jones
2023-08-18 15:40 ` [GIT PULL] Immutable branch between MFD, Pinctrl and soundwire due for the v6.6 merge window Lee Jones
2023-08-18 22:39 ` (subset) [PATCH v7 0/6] Add cs42l43 PC focused SoundWire CODEC Mark Brown
2023-08-22 16:33 ` Mark Brown
2024-01-18 16:58 ` andy.shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zali4qxdegY7H6eY@surfacebook.localdomain \
--to=andy.shevchenko@gmail.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=ckeepax@opensource.cirrus.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lee@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=patches@opensource.cirrus.com \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=robh+dt@kernel.org \
--cc=sanyog.r.kale@intel.com \
--cc=vkoul@kernel.org \
--cc=yung-chuan.liao@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).