On Mon, Aug 16, 2021 at 05:43:09PM -0500, David Rhodes wrote:
SoC Audio driver for the Cirrus Logic CS35L41 amplifier
Signed-off-by: David Rhodes drhodes@opensource.cirrus.com
+++ b/sound/soc/codecs/cs35l41-i2c.c @@ -0,0 +1,115 @@ +/* SPDX-License-Identifier: GPL-2.0
- cs35l41-i2c.c -- CS35l41 I2C driver
- Copyright 2017-2021 Cirrus Logic, Inc.
- Author: David Rhodes david.rhodes@cirrus.com
- */
SPDX headers in the C files need to be //, it's apparently some tooling thing somewhere that requires the C and H files to be different. I believe Mark was suggesting you changed this to look like:
// SPDX-License-Identifier: GPL-2.0 // // cs35l41-i2c.c -- CS35l41 I2C driver // // Copyright 2017-2021 Cirrus Logic, Inc. // // Author: David Rhodes david.rhodes@cirrus.com
+bool cs35l41_readable_reg(struct device *dev, unsigned int reg) +{
- switch (reg) {
<snip>
- case CS35L41_TIMER1_CONTROL:
- case CS35L41_TIMER1_COUNT_PRESET:
- case CS35L41_TIMER1_STATUS:
- case CS35L41_TIMER1_COUNT_READBACK:
- case CS35L41_TIMER1_DSP_CLK_CFG:
- case CS35L41_TIMER1_DSP_CLK_STATUS:
- case CS35L41_TIMER2_CONTROL:
- case CS35L41_TIMER2_COUNT_PRESET:
- case CS35L41_TIMER2_STATUS:
- case CS35L41_TIMER2_COUNT_READBACK:
- case CS35L41_TIMER2_DSP_CLK_CFG:
- case CS35L41_TIMER2_DSP_CLK_STATUS:
- case CS35L41_DFT_JTAG_CONTROL:
These should be dropped, these are registers for the firmware peripherals and will never be used from the driver.
+static const struct snd_kcontrol_new cs35l41_aud_controls[] = {
- SOC_SINGLE_SX_TLV("Digital PCM Volume", CS35L41_AMP_DIG_VOL_CTRL,
3, 0x4CF, 0x391, dig_vol_tlv),- SOC_SINGLE_TLV("Analog PCM Volume", CS35L41_AMP_GAIN_CTRL, 5, 0x14, 0,
amp_gain_tlv),- SOC_ENUM("PCM Soft Ramp", pcm_sft_ramp),
- SOC_SINGLE("Boost Converter Enable", CS35L41_PWR_CTRL2, 4, 3, 0),
Doesn't seem right to have an ALSA control to enable the boost converter. Controlling high voltages is definitely something that should be under kernel control.
- SOC_SINGLE("Boost Target Voltage", CS35L41_BSTCVRT_VCTRL1, 0, 0xAA, 0),
Ditto here for the boost voltage.
- SOC_SINGLE("Weak FET Threshold", CS35L41_WKFET_CFG,
CS35L41_CH_WKFET_THLD_SHIFT, 0xF, 0),- SOC_SINGLE("Weak FET Delay", CS35L41_WKFET_CFG,
CS35L41_CH_WKFET_DLY_SHIFT, 7, 0),
Likewise this weak FET stuff, it looks hardware dependent and probably shouldn't be in userspace control, the datasheet states:
The strength of the output drivers is reduced when operating in a Weak-FET Drive Mode and must not be used to drive a large load.
Which strongly implies to me this should be in DT/driver control.
- SOC_SINGLE("Temp Warn Threshold", CS35L41_DTEMP_WARN_THLD,
0, CS35L41_TEMP_THLD_MASK, 0),
Again temperature warnings don't feel like they should be getting set through an ALSA control.
+static int cs35l41_component_probe(struct snd_soc_component *component) +{
- struct cs35l41_private *cs35l41 =
snd_soc_component_get_drvdata(component);- component->regmap = cs35l41->regmap;
You sure this is necessary? The core should do this for us, its only necessary if the regmap is on a different struct device to the CODEC which isn't the case in this driver. Also if this is necessary it should be using snd_soc_component_init_regmap.
+static const struct snd_soc_component_driver soc_component_dev_cs35l41 = {
- .name = "cs35l41-codec",
- .probe = cs35l41_component_probe,
- .dapm_widgets = cs35l41_dapm_widgets,
- .num_dapm_widgets = ARRAY_SIZE(cs35l41_dapm_widgets),
- .dapm_routes = cs35l41_audio_map,
- .num_dapm_routes = ARRAY_SIZE(cs35l41_audio_map),
- .controls = cs35l41_aud_controls,
- .num_controls = ARRAY_SIZE(cs35l41_aud_controls),
- .set_sysclk = cs35l41_component_set_sysclk,
+};
Probably don't need quite so many blank lines here.
+int cs35l41_probe(struct cs35l41_private *cs35l41,
struct cs35l41_platform_data *pdata)+{
- u32 regid, reg_revid, i, mtl_revid, int_status, chipid_match;
- int irq_pol = 0;
- int timeout;
- int ret;
Extra blank line.
- for (i = 0; i < CS35L41_NUM_SUPPLIES; i++)
cs35l41->supplies[i].supply = cs35l41_supplies[i];- ret = devm_regulator_bulk_get(cs35l41->dev, CS35L41_NUM_SUPPLIES,
cs35l41->supplies);- if (ret != 0) {
dev_err(cs35l41->dev,"Failed to request core supplies: %d\n",ret);return ret;- }
- ret = regulator_bulk_enable(CS35L41_NUM_SUPPLIES, cs35l41->supplies);
- if (ret != 0) {
dev_err(cs35l41->dev,"Failed to enable core supplies: %d\n", ret);return ret;- }
- if (pdata) {
cs35l41->pdata = *pdata;- } else {
ret = cs35l41_handle_pdata(cs35l41->dev, &cs35l41->pdata,cs35l41);if (ret != 0) {ret = -ENODEV;goto err;}- }
Any reason to not just parse this before enabling the supplies? Parsing the pdata shouldn't require the hardware to be enabled all the settings are being applied through set_pdata later on, means you can just return on an error as well rather than having to disable the supplies again. On the topic of errors any reason this sets all errors to ENODEV, why not return the error that cs35l41_handle_pdata returned? Seems more helpful.
Thanks, Charles