Re: [PATCH v2 2/3] ASoC: codecs: Add Awinic AW8898 amplifier driver

[Christophe JAILLET]

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

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