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

[Mark Brown <broonie@kernel.org>]

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

On Fri, Jul 15, 2022 at 01:41:00PM +0800, Zhu Ning wrote:

Looks mostly good but there's still some issues here, plus the ones
Pierre found.  Only one or two are substantial though, some of the
things below are really minor:

Please check the coding style matches the kernel coding style -
checkpatch will probably help here.

> +	snd_soc_dapm_mutex_unlock(dapm);
> +}
> +static void es8326_jack_detect_handler(struct work_struct *work)

Blank line missing between functions.

> +	if(!es8326->jack)
> +		goto out;

Missing space after the if.

> +static int es8326_resume(struct snd_soc_component *component)
> +{

I'm not seeing anything in here to resync the register map with the
device - the driver is using a register cache so that's going to break
if the device looses power over suspend.  TBH it's not clear to me that
the driver isn't hard coding a specific set of paths...

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

Some of the hard coded register write sequences in here look a lot like
they're assuming a specific board layout and might need more device tree
configurability - what if the board doesn't use an interrupt or uses a
different one?

> +	ret = device_property_read_u8(component->dev, "everest,mic1-src", &es8326->mic1_src);
> +	if (ret != 0) {

This is adding a DT binding but there's no DT binding document.
Previous versions of this driver did have one, please include it with
every submission.

> --- /dev/null
> +++ b/sound/soc/codecs/es8326.h
> @@ -0,0 +1,187 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * es8326.c -- es8326 ALSA SoC audio driver

Commend has the wrong filename here.

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