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, ®);
> + 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.
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
