linux-kernel - Re: [PATCH v2 2/3] ASoC: codecs: Add Awinic AW8898 amplifier 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 for Android: free password hash cracker in your pocket

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

Message-ID: <225692dd-c41b-4e9d-a20b-551f9bfb5051@wanadoo.fr>
Date: Sun, 6 Jul 2025 10:35:20 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Luca Weiss <luca@...aweiss.eu>, ~postmarketos/upstreaming@...ts.sr.ht,
 phone-devel@...r.kernel.org, Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
 Takashi Iwai <tiwai@...e.com>, Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>,
 Weidong Wang <wangweidong.a@...nic.com>
Cc: linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 2/3] ASoC: codecs: Add Awinic AW8898 amplifier driver

Le 05/07/2025 à 14:03, Luca Weiss a écrit :
> Add a driver for the AW8898 Audio Amplifier.
> 
> Signed-off-by: Luca Weiss <luca@...aweiss.eu>

Hi,

...

> +#define AW8898_CFG_NAME				"aw8898_cfg.bin"
> +
> +#define AW8898_NUM_SUPPLIES	3

See the probe below, but if simplified, AW8898_NUM_SUPPLIES would be 
useless and could be removed.

> +static const char *aw8898_supply_names[AW8898_NUM_SUPPLIES] = {

static const char * const ?

> +	"vdd",		/* Battery power */
> +	"vddio",	/* Digital IO power */
> +	"dvdd",		/* Digital power */
> +};
> +
> +static const char * const aw8898_dev_mode_text[] = {
> +	"Speaker",
> +	"Receiver",
> +};

...

> +static int aw8898_drv_event(struct snd_soc_dapm_widget *w,
> +				struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(component);
> +	int ret;

Maybe ret = 0; to simplify the code below?

Or, as done in aw8898_hw_params(), return -EINVAL; in the default case, 
and a plain return 0; at the end of the function?

> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		aw8898_set_power(aw8898, true);
> +
> +		if (!aw8898->cfg_loaded)
> +			aw8898_cold_start(aw8898);
> +
> +		ret = 0;
> +		break;
> +	case SND_SOC_DAPM_POST_PMD:
> +		aw8898_set_power(aw8898, false);
> +		ret = 0;
> +		break;
> +	default:
> +		dev_err(component->dev, "%s: invalid event %d\n", __func__, event);
> +		ret = -EINVAL;

Even if useless, having a break is more standard.

> +	}
> +
> +	return ret;
> +}

...

> +static int aw8898_check_chipid(struct aw8898 *aw8898)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(aw8898->regmap, AW8898_ID, &reg);
> +	if (ret < 0) {
> +		dev_err(&aw8898->client->dev,
> +			"Failed to read register AW8898_ID: %d\n", ret);

aw8898_check_chipid() is only called from the probe, so using return 
dev_err_probe() should be fine.

> +		return ret;
> +	}
> +
> +	dev_dbg(&aw8898->client->dev, "Read chip ID 0x%x\n", reg);
> +
> +	if (reg != AW8898_CHIP_ID) {
> +		dev_err(&aw8898->client->dev, "Unexpected chip ID: 0x%x\n",
> +			reg);

Same.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int aw8898_probe(struct i2c_client *client)
> +{
> +	struct aw8898 *aw8898;
> +	int ret;
> +
> +	aw8898 = devm_kzalloc(&client->dev, sizeof(*aw8898), GFP_KERNEL);
> +	if (!aw8898)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, aw8898);
> +	aw8898->client = client;
> +
> +	aw8898->regmap = devm_regmap_init_i2c(client, &aw8898_regmap);
> +	if (IS_ERR(aw8898->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(aw8898->regmap),
> +				     "Failed to allocate register map\n");
> +
> +	for (int i = 0; i < ARRAY_SIZE(aw8898->supplies); i++)
> +		aw8898->supplies[i].supply = aw8898_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(aw8898->supplies),
> +				      aw8898->supplies);

devm_regulator_bulk_get_enable() would simplify the code and 'struct 
aw8898'.

> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to get regulators\n");
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(aw8898->supplies),
> +				    aw8898->supplies);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to enable supplies: %d\n",
> +			ret);

If dev_err_probe() to be consistent with the code below and above.
But this would be removed if devm_regulator_bulk_get_enable() is used.

> +		return ret;
> +	}
> +
> +	aw8898->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(aw8898->reset))
> +		return dev_err_probe(&client->dev, PTR_ERR(aw8898->reset),
> +				     "Failed to get reset GPIO\n");
>

...

CJ