[alsa-devel] [PATCH 1/2] ASoC: Add support for CS35L32 Boosted Amplifier

[Mark Brown]

On Tue, Jul 29, 2014 at 03:02:42PM -0500, Brian Austin wrote:

> +	case CS35L32_LED_STATUS:
> +	case CS35L32_FLASH_MODE:
> +	case CS35L32_MOVIE_MODE:
> +	case CS35L32_FLASH_TIMER:
> +	case CS35L32_FLASH_INHIBIT:

Should this be an MFD?  Can always be refactored later if required
though.

> +static bool cs35l32_volatile_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case CS35L32_DEVID_AB:
> +	case CS35L32_DEVID_CD:
> +	case CS35L32_DEVID_E:
> +	case CS35L32_FAB_ID:
> +	case CS35L32_REV_ID:
> +		return 1;
> +	default:
> +		return 0;
> +	}
> +}

Should the interrupt and LED status registers not also be volatile?

> +static const struct snd_kcontrol_new cs35l32_snd_controls[] = {
> +	SOC_SINGLE_TLV("SPK Amp Volume", CS35L32_CLASSD_CTL,
> +		       3, 0x04, 1, classd_ctl_tlv),

Speaker Volume.

> +	SOC_SINGLE("Gain Zero Cross", CS35L32_CLASSD_CTL, 2, 1, 0),

Zero Cross Switch perhaps (if it's an on/off control it should be called
Switch)?

> +static int int_clear(struct snd_soc_dapm_widget *w,
> +			struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = w->codec;
> +
> +	if (SND_SOC_DAPM_EVENT_ON(event)) {
> +		snd_soc_read(codec, CS35L32_INT_STATUS_1);
> +		snd_soc_read(codec, CS35L32_INT_STATUS_2);
> +	} else {
> +		return 0;
> +	}
> +	return 0;
> +}

This seems...  icky.  Shouldn't there be an interrupt handler doing
this?

> +static int cs35l32_codec_set_sysclk(struct snd_soc_codec *codec,
> +			      int clk_id, int source, unsigned int freq, int dir)
> +{
> +
> +	switch (freq) {

> +	case CS35L32_MCLK_6144MHZ:

Not sure these defines add anything over just using the numbers and it
avoids ickyness with the fact that I bet this isn't really 6.144GHz.

> +static int cs35l32_probe(struct snd_soc_codec *codec)
> +{
> +	/* Power down the AMP */
> +	snd_soc_update_bits(codec, CS35L32_PWRCTL1, CS35L32_PDN_AMP,
> +			    CS35L32_PDN_AMP);
> +
> +	/* Clear MCLK Error Bit since we don't have the clock yet */
> +	snd_soc_read(codec, CS35L32_INT_STATUS_1);
> +
> +	return 0;
> +}

Any reason not to do these in the device level probe()?

> +static int cs35l32_remove(struct snd_soc_codec *codec)
> +{
> +	struct cs35l32_private *cs35l32 = snd_soc_codec_get_drvdata(codec);
> +
> +	regulator_bulk_free(ARRAY_SIZE(cs35l32->supplies), cs35l32->supplies);
> +	return 0;
> +}

The regulators should be being acquired and released in the device level
probe(), though this could be dropped entirely with devm.

> +	ret = regmap_register_patch(cs35l32->regmap, cs35l32_monitor_patch,
> +				    ARRAY_SIZE(cs35l32_monitor_patch));

Should either pay attention to the return value or not assign ret
(better to pay attention but it's not like it'd be the first CODEC to
ignore it).

> +	/* initialize codec */
> +	ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_AB, &reg);
> +	devid = (reg & 0xFF) << 12;
> +
> +	ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_CD, &reg);
> +	devid |= (reg & 0xFF) << 4;
> +
> +	ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_E, &reg);
> +	devid |= (reg & 0xF0) >> 4;
> +
> +	if (devid != CS35L32_CHIP_ID) {
> +		ret = -ENODEV;
> +		dev_err(&i2c_client->dev,
> +			"CS35L32 Device ID (%X). Expected %X\n",
> +			devid, CS35L32_CHIP_ID);
> +		return ret;
> +	}

Should the ID check not be done before we register the patch in case
it's the wrong device and we do something bad to it by writing to it?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140731/7dd4f34f/attachment.sig>