[PATCH v5 1/2] ASoC: cs35l41: CS35L41 Boosted Smart Amplifier

[Charles Keepax]

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 at 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 at 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 at 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