[alsa-devel] [PATCH 1/2] ASoC: cs35l36: Add support for Cirrus CS35L36 Amplifier

[Mark Brown]

On Tue, Nov 13, 2018 at 12:49:16PM -0600, James Schulman wrote:

> +++ b/sound/soc/codecs/cs35l36-tables.c
> @@ -0,0 +1,315 @@
> +/*
> + * cs35l36-tables.c -- CS35L36 ALSA SoC audio driver
> + *
> + * Copyright 2018 Cirrus Logic, Inc.

Please use SPDX headers.

> +	SOC_SINGLE_TLV("AMP PCM Gain", CS35L36_AMP_GAIN_CTRL, 5, 0x13, 0,
> +			amp_gain_tlv),

All volume controls should end in Volume, see control-names.rst.

> +		regmap_read(cs35l36->regmap, CS35L36_INT4_RAW_STATUS, &reg);
> +		if (reg & CS35L36_PLL_UNLOCK_MASK)
> +			dev_crit(cs35l36->dev, "PLL Unlocked\n");

WARN_ON_ONCE?

> +	}
> +	return 0;
> +}
> +static const char * const cs35l36_chan_text[] = {

Missing blank line.

> +static const char * const cs35l36_boost_text[] = {
> +	"On",
> +	"Off",
> +};
> +
> +static SOC_ENUM_SINGLE_DECL(boost_enum, SND_SOC_NOPM, 0,
> +		cs35l36_boost_text);
> +
> +static const struct snd_kcontrol_new cs35l36_boost_mux[] = {
> +	SOC_DAPM_ENUM("Boost Enable", boost_enum),
> +};

Simple on/off conttrols should be _SINGLE controls with Switch at the
end of their name.  It's not super clear why this is in DAPM as a widget
at all, the routing around it is:

+       {"BOOST Mux", "On", "Channel Mux"},
+       {"CLASS H", NULL, "BOOST Mux"},

which is a bit perplexing.  Possibly this should be handled in the event
for the main amplifier?

> +	if (cs35l36->sclk > 6000000) {
> +		fs1_val = 3 * 4 + 4;
> +		fs2_val = 8 * 4 + 4;
> +	}

These are just constants, why write out the operations?

> +
> +	if (cs35l36->sclk <= 6000000) {

if/else?

> +	/*
> +	 * Rev B0 has 2 versions
> +	 * L36 is 10V
> +	 * L37 is 12V

\o/

> +	}
> +
> +	return IRQ_HANDLED;

This unconditionally reports that it handled the interrupt, even if it
didn't.  This will stop the interrupt core handling for faulty interrupt
lines working and will disrupt any support that gets added for handling
shared threaded interrupts.

> +	if (pdata)
> +		cs35l36->pdata = *pdata;
> +	else {

Use { } on both sides of the if (see coding-style.rst)?

> +	cs35l36->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						      GPIOD_OUT_LOW);
> +	if (IS_ERR(cs35l36->reset_gpio)) {
> +		ret = PTR_ERR(cs35l36->reset_gpio);
> +		cs35l36->reset_gpio = NULL;
> +		if (ret == -EBUSY)
> +			dev_info(dev,
> +				 "Reset line busy, assuming shared reset\n");
> +		else {
> +			dev_err(dev, "Failed to get reset GPIO: %d\n", ret);
> +			goto err;
> +		}
> +	}

This doesn't handle -EPROBE_DEFER.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20181113/3d126644/attachment.sig>