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.
Attachment:
signature.asc
Description: PGP signature
