Re: [PATCH v2 2/2] ASoC: codecs: add ES7243E ADC driver

[Mark Brown <broonie@xxxxxxxxxx>]

On Fri, Sep 20, 2024 at 06:41:08PM +0300, Igor Prusov wrote:

> +config SND_SOC_ES7243E
> +	tristate "Everest Semi ES7243E CODEC"
> +

This is an I2C device, it should depend on I2C.

> @@ -0,0 +1,693 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/**
> + * es7243e.c - ASoC Everest Semiconductor ES7243E audio ADC driver
> + *

Please make the enitre comment a C++ one so things look more
intentional.

> + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.

All rights reserved?

> +static const struct reg_sequence init_sequence[] = {
> +	{ ES7243E_CLK2,			0x00 },
> +	{ ES7243E_SDP,			0x00 },
> +	{ ES7243E_TDM,			0x00 },
> +
> +	/* Set MCLK/LRCK ratio to 256 */
> +	{ ES7243E_ADC_OSR,		0x20 },

This should be dynamically configured rather than hard coded, provide
set_bclk_ratio().

> +	/* Set ADC volume to 0dB */
> +	{ ES7243E_ADC_VOL,		0xbf },

Most things should use the chip defaults, especially things like volumes
- userspace can configure whatever it needs.

> +static int es7243e_suspend(struct snd_soc_component *component)
> +{
> +	struct es7243e_priv *es7243e = snd_soc_component_get_drvdata(component);
> +	int ret;
> +	unsigned int val, mask;
> +
> +	val = FIELD_PREP(ES7243E_SDP_MUTE, 0);
> +	ret = snd_soc_component_update_bits(component, ES7243E_SDP,
> +					    ES7243E_SDP_MUTE, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = FIELD_PREP(ES7243E_PGA1_EN, 0);
> +	snd_soc_component_update_bits(component, ES7243E_PGA1,
> +				      ES7243E_PGA1_EN, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = FIELD_PREP(ES7243E_PGA2_EN, 0);
> +	snd_soc_component_update_bits(component, ES7243E_PGA2,
> +				      ES7243E_PGA2_EN, val);
> +	if (ret < 0)
> +		return ret;

This looks a lot like you should be using DAPM with events on the PGAs,
that will give power management at runtime as well.

> +	ret = snd_soc_component_write(component, ES7243E_PDN, 0xff);
> +	if (ret < 0)
> +		return ret;
> +
> +	mask = ES7243E_CLK1_ANA_ON | ES7243E_CLK1_ADC_ON;
> +	ret = snd_soc_component_update_bits(component, ES7243E_CLK1, mask, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	clk_disable_unprepare(es7243e->lrclk);
> +	clk_disable_unprepare(es7243e->sclk);

You could also use set_bias_level() for chip level power, it doesn't
look like there's substantial delay.

> +static int es7243e_resume(struct snd_soc_component *component)
> +{
> +	struct es7243e_priv *es7243e = snd_soc_component_get_drvdata(component);
> +	int ret;
> +	unsigned int val;

This doesn't resync the cache so the user volume settings will be lost.

Attachment: signature.asc
Description: PGP signature