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

[Charles Keepax]

On Tue, Nov 13, 2018 at 12:49:16PM -0600, James Schulman wrote:
> Add driver support for Cirrus Logic CS35L36 boosted
> speaker amplifier
> 
> Signed-off-by: James Schulman <james.schulman at cirrus.com>
> ---

Mostly just some small nitpicky comments from me:

> +#include "cs35l36.h"
> +
> +struct reg_default cs35l36_reg[CS35L36_MAX_CACHE_REG] = {

Your allocating an array that is much larger than you need here.
This array will have an entry for all possible register addresses
on the device, whereas only a very small subset of those
addresses have defaults infact most of them arn't even registers.

> +	{CS35L36_TESTKEY_CTRL,			0x00000000},
...
> +struct  cs35l36_private {
> +	struct device *dev;
> +	struct cs35l36_platform_data pdata;
> +	struct regmap *regmap;
> +	struct regulator_bulk_data supplies[2];
> +	int num_supplies;
> +	int clksrc;
> +	int prev_clksrc;
> +	int extclk_freq;
> +	int extclk_cfg;
> +	int fll_igain;
> +	int sclk;
> +	int chip_version;
> +	int rev_id;
> +	struct gpio_desc *reset_gpio;
> +	struct completion global_pup_done;
> +	struct completion global_pdn_done;

These two completions are unused.

> +static int cs35l36_dai_set_sysclk(struct snd_soc_dai *dai,
> +				int clk_id, unsigned int freq, int dir)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct cs35l36_private *cs35l36 =
> +			snd_soc_component_get_drvdata(component);
> +	int fs1_val = 0;
> +	int fs2_val = 0;
> +
> +	/* Need the SCLK Frequency regardless of sysclk source */
> +	cs35l36->sclk = freq;

Yet it is only used locally in this function.

> +	if (cs35l36->sclk > 6000000) {
> +		fs1_val = 3 * 4 + 4;
> +		fs2_val = 8 * 4 + 4;
> +	}
> +
> +	if (cs35l36->sclk <= 6000000) {
> +		fs1_val = 3 * ((24000000 + cs35l36->sclk - 1) / cs35l36->sclk)
> +				+ 4;
> +		fs2_val = 5 * ((24000000 + cs35l36->sclk - 1) / cs35l36->sclk)
> +				+ 4;
> +	}

Nitpicking: Since both paths add 4 you could initialise fs1_val and
fs2_val to 4, then use += would avoid the line wraps for the + 4's
here.

> +static int cs35l36_pcm_startup(struct snd_pcm_substream *substream,
> +			       struct snd_soc_dai *dai)
> +{
> +	if (!substream->runtime)
> +		return 0;

This null check shouldn't be needed now since:

8053f21675b0 ("ASoC: dapm: Add a dummy snd_pcm_runtime to avoid NULL pointer access")

> +
> +	snd_pcm_hw_constraint_list(substream->runtime, 0,
> +				SNDRV_PCM_HW_PARAM_RATE, &cs35l36_constraints);
> +	return 0;
> +}
...
> +static int cs35l36_component_set_sysclk(struct snd_soc_component *component,
> +				int clk_id, int source, unsigned int freq,
> +				int dir)
> +{
> +	struct cs35l36_private *cs35l36 =
> +			snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	cs35l36->extclk_freq = freq;

extclk_freq doesn't appear to be used at all, is there an
expectation it will be in the future?

> +	cs35l36->prev_clksrc = cs35l36->clksrc;

Should this be stored in cs35l36_private, given it is only used
locally in this function?

> +
> +	switch (clk_id) {
> +	case 0:
> +		cs35l36->clksrc = CS35L36_PLLSRC_SCLK;
> +		break;
> +	case 1:
> +		cs35l36->clksrc = CS35L36_PLLSRC_LRCLK;
> +		break;
> +	case 2:
> +		cs35l36->clksrc = CS35L36_PLLSRC_PDMCLK;
> +		break;
> +	case 3:
> +		cs35l36->clksrc = CS35L36_PLLSRC_SELF;
> +		break;
> +	case 4:
> +		cs35l36->clksrc = CS35L36_PLLSRC_MCLK;

Seems clksrc is also only used locally, is there some future work
expected on the clocking?

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = cs35l36_get_clk_config(cs35l36, freq);
> +

Would it be simpler just to return a pointer to the
cs35l36_pll_sysclk_config structure?

> +	if (cs35l36->rev_id == CS35L36_REV_A0) {
> +		if (cs35l36->pdata.dcm_mode) {

Nitpick:
Could probably use an && here.

> +static int cs35l36_pac(struct cs35l36_private *cs35l36)
> +{
> +	int ret, count;
> +	unsigned int val;
> +
> +	if (cs35l36->rev_id == CS35L36_REV_B0) {

Nitpick:
This is already checked before you call the function, so I would
be inclined to drop it but if doing it here it would probably be
better to invert the logic and do:

if (cs35l36->rev_id != CS35L36_REV_B0)
	return 0;

> +	switch (cs35l36->rev_id) {
> +	case CS35L36_REV_A0:
> +		ret = regmap_register_patch(cs35l36->regmap,
> +				cs35l36_reva0_errata_patch,
> +				ARRAY_SIZE(cs35l36_reva0_errata_patch));
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to apply A0 errata patch %d\n",
> +					ret);
> +			goto err;
> +		}
> +		ret = regmap_register_patch(cs35l36->regmap,
> +					cs35l36_pac_int_patch,
> +					ARRAY_SIZE(cs35l36_pac_int_patch));
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to apply A0PAC errata patch %d\n",
> +					ret);
> +			goto err;
> +		}

Is this really what you want, having two calls to
remap_register_patch will apply the settings from both here, but
only the second of those calls would be reapplied on cache_sync
later. Now I guess you don't have any cache_syncs so it won't
cause a problem now but feels like it has the potential to cause
confusion down the road.

> +	ret = devm_request_threaded_irq(dev, i2c_client->irq, NULL, cs35l36_irq,
> +					IRQF_ONESHOT |
> +					irq_pol,

Nitpick probably fire this up on the same line as the ONESHOT.

> +					"cs35l36", cs35l36);

Thanks,
Charles