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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZvKvcBHJU2pKl809@finisterre.sirena.org.uk>
Date: Tue, 24 Sep 2024 14:24:16 +0200
From: Mark Brown <broonie@...nel.org>
To: Igor Prusov <ivprusov@...utedevices.com>
Cc: Liam Girdwood <lgirdwood@...il.com>, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-sound@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	kernel@...utedevices.com, prusovigor@...il.com,
	David Yang <yangxiaohua@...rest-semi.com>,
	Martin Kurbanov <mmkurbanov@...utedevices.com>
Subject: Re: [PATCH v2 2/2] ASoC: codecs: add ES7243E ADC driver

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)