Le 05/07/2025 à 14:03, Luca Weiss a écrit :
Add a driver for the AW8898 Audio Amplifier.
Signed-off-by: Luca Weiss <[email protected]>
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, ®);
+ 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
