[PATCH v3 1/2] ASoC: codecs: add support for ES8326

[Mark Brown]

On Tue, Jul 26, 2022 at 09:17:46PM +0800, Zhu Ning wrote:

> +static struct snd_pcm_hw_constraint_list es8326_constraints = {
> +	.count = ARRAY_SIZE(es8326_rates),
> +	.list = es8326_rates,
> +};
> +
> +static int es8326_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> +				 int clk_id, unsigned int freq, int dir)
> +{
> +	struct snd_soc_component *codec = codec_dai->component;
> +	struct es8326_priv *es8326 = snd_soc_component_get_drvdata(codec);
> +
> +	es8326->sysclk = freq;
> +
> +	if (freq == 0) {
> +		es8326->sysclk_constraints->list = NULL;
> +		es8326->sysclk_constraints->count = 0;
> +		return 0;
> +	}
> +
> +	es8326->sysclk_constraints = &es8326_constraints;

Nothing ever restores the constraints if a clock is specified again, and
in general it's odd that the enable/disable don't match up - if we're
setting variable constraints I'd expect that in the freq == 0 case we
should be setting ->sysclk_constraints to NULL rather than the contents.
Indeed, we'll segfault here if the frequency is set to 0 without having
first been set to some actual value.  It's also bad to modify static
data potentially shared between multiple instances of the device in a
system.

Having said all that though I'm not clear why we're doing this
constraint stuff at all, we never reference sysclk_constraints during
startup and teardown and you'd usually do this because you want to set
constraints that depend on the sysclk but this is just a constant set of
constraints that should be set in the DAI desription.

> +	if (coeff >= 0) {
> +		regmap_write(es8326->regmap,  ES8326_CLK_DIV1_04,
> +			     coeff_div[coeff].reg4);
> +		regmap_write(es8326->regmap,  ES8326_CLK_DIV2_05,
> +			     coeff_div[coeff].reg5);
> +		regmap_write(es8326->regmap,  ES8326_CLK_DLL_06,
> +			     coeff_div[coeff].reg6);
> +		regmap_write(es8326->regmap,  ES8326_CLK_MUX_07,
> +			     coeff_div[coeff].reg7);
> +		regmap_write(es8326->regmap,  ES8326_CLK_ADC_SEL_08,
> +			     coeff_div[coeff].reg8);
> +		regmap_write(es8326->regmap,  ES8326_CLK_DAC_SEL_09,
> +			     coeff_div[coeff].reg9);
> +		regmap_write(es8326->regmap,  ES8326_CLK_ADC_OSR_0A,
> +			     coeff_div[coeff].rega);
> +		regmap_write(es8326->regmap,  ES8326_CLK_DAC_OSR_0B,
> +			     coeff_div[coeff].regb);
> +	}

This will just leave the divider setup at whatever they were last set at
if we don't get a value which given the names of the registers I suspect
won't go too well, it'd be better to print a warning just in case.

> +	regmap_write(es8326->regmap, ES8326_INT_SOURCE_58, 0x08);
> +	regmap_write(es8326->regmap, ES8326_INTOUT_IO_59, 0x45);

This really does look like board specific configuration which should
come from DT.
-------------- 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/20220726/bd23e36a/attachment.sig>