Re: [PATCH] ASoC: codecs: add support for ES8326

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2724 bytes --]

On Thu, Jul 07, 2022 at 09:18:56AM +0800, Zhu Ning wrote:

> The ES8326 codec is not compatible with ES8316 and requires a dedicated driver.

There's a bunch of bot reported issues as well as those below.  Other
than the interrupt management this should all be fairly small, the bulk
of the driver looks good:

> +	SOC_ENUM_SINGLE(ES8326_DAC_DSM_4D, 4, 4, dacpol_txt);
> +static const struct soc_enum alc_winsize =
> +	SOC_ENUM_SINGLE(ES8326_ADC_RAMPRATE_2E, 4, 16, winsize);
> +static const struct soc_enum drc_winsize =
> +	SOC_ENUM_SINGLE(ES8326_DRC_WINSIZE_54, 4, 16, winsize);
> +static const struct snd_kcontrol_new es8326_snd_controls[] = {

We needs osme blank lines between declarations here to improve
legibility.

> +/*
> + * Note that this should be called from init rather than from hw_params.
> + */
> +static int es8326_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> +				 int clk_id, unsigned int freq, int dir)

I don't see any reason why it *couldn't* be called from hw_params -
it'll mean the constraints don't take effect but that might be desirable
if it's called from hw_params due to being able to reprogram the input
clock.

> +}
> +static int es8326_probe(struct snd_soc_component *component)

More missing blank lines.

> +	ret = device_property_read_u8(component->dev, "everest,mic1-src", &es8326->mic1_src);
> +	if (ret != 0) {
> +		dev_dbg(component->dev, "mic1-src return %d", ret);
> +		es8326->mic1_src = ES8326_ADC_AMIC;
> +	}
> +	dev_dbg(component->dev, "mic1-src %x", es8326->mic1_src);
> +
> +	ret = device_property_read_u8(component->dev, "everest,mic2-src", &es8326->mic2_src);

This is adding a DT binding, the binding needs to be documented.

> +	if((reg && ES8326_VERSION_B) == 1)

Coding style and I'm not sure the logic there is what's intended?

> +	{
> +		regmap_write(es8326->regmap, ES8326_ANA_VSEL_1C, 0x7F);
> +		regmap_write(es8326->regmap, ES8326_VMIDLOW_22, 0x0F);
> +		regmap_write(es8326->regmap, ES8326_DAC2HPMIX_25, 0xAA);
> +		regmap_write(es8326->regmap, ES8326_HP_DRVIER_24, 0x20);
> +	}

Should there be something similar in the resume path?  I'm also not
seeing anything that manages the register cache over suspend.

> +	/* Enable irq and sync initial jack state */
> +	enable_irq(es8326->irq);
> +	es8326_irq(es8326->irq, es8326);

The driver souldn't need to enable or disable the IRQ by hand, it should
just configure the device to not generate interrupts when not in use.
Enabling and disabling doesn't play nicely with shared interrupts and is
in general typically a warning sign.

> +#ifdef CONFIG_OF
> +static const struct of_device_id es8326_of_match[] = {
> +	{ .compatible = "everest, es8326", },

There shouldn't be a space in the compatible string.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]