Re: [PATCH V1 2/2] ASoC: codecs: Add aw88081 amplifier driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



Le 18/10/2024 à 11:43, wangweidong.a-tUEr1MkLeujQT0dZR+AlfA@xxxxxxxxxxxxxxxx a écrit :
From: Weidong Wang <wangweidong.a-tUEr1MkLeujQT0dZR+AlfA@xxxxxxxxxxxxxxxx>

The driver is for amplifiers aw88081 of Awinic Technology Corporation.
The awinic AW88081 is an I2S/TDM input, high efficiency digital
Smart K audio amplifier

Signed-off-by: Weidong Wang <wangweidong.a-tUEr1MkLeujQT0dZR+AlfA@xxxxxxxxxxxxxxxx>
---

Hi,

...

+
+#include <linux/i2c.h>
+#include <linux/firmware.h>

Sometimes, alphabecal order is prefered.

+#include <linux/regmap.h>
+#include <sound/soc.h>
+#include "aw88081.h"
+#include "aw88395/aw88395_device.h"
+
+static const struct regmap_config aw88081_regmap_config = {
+	.val_bits = 16,
+	.reg_bits = 8,
+	.max_register = AW88081_REG_MAX,
+	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+};

...

+static int aw88081_dev_check_syspll(struct aw_device *aw_dev)
+{
+	int ret;
+
+	ret = aw88081_dev_check_mode1_pll(aw_dev);
+	if (ret) {
+		dev_dbg(aw_dev->dev, "mode1 check iis failed try switch to mode2 check");
+		ret = aw88081_dev_check_mode2_pll(aw_dev);
+		if (ret) {
+			dev_err(aw_dev->dev, "mode2 check iis failed");
+			return ret;
+		}
+	}
+
+	return ret;

Here and in some other places, return 0; could be used to be more explicit.

+}

...

+static int aw88081_reg_update(struct aw88081 *aw88081, bool force)
+{
+	struct aw_device *aw_dev = aw88081->aw_pa;
+	int ret;
+
+	if (force) {
+		ret = regmap_write(aw_dev->regmap,
+					AW88081_ID_REG, AW88081_SOFT_RESET_VALUE);
+		if (ret)
+			return ret;
+
+		ret = aw88081_dev_fw_update(aw88081);
+		if (ret)
+			return ret;
+	} else {
+		if (aw_dev->prof_cur != aw_dev->prof_index) {
+			ret = aw88081_dev_fw_update(aw88081);
+			if (ret)
+				return ret;
+		} else {
+			ret = 0;

This else could be removed, and an explicit return 0; used below at the end of the function.

+		}
+	}
+
+	aw_dev->prof_cur = aw_dev->prof_index;
+
+	return ret;
+}

...

+static int aw88081_profile_info(struct snd_kcontrol *kcontrol,
+			 struct snd_ctl_elem_info *uinfo)
+{
+	struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
+	struct aw88081 *aw88081 = snd_soc_component_get_drvdata(codec);
+	char *prof_name, *name;
+	int count, ret;
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
+	uinfo->count = 1;
+
+	count = aw88081->aw_pa->prof_info.count;
+	if (count <= 0) {
+		uinfo->value.enumerated.items = 0;
+		return 0;
+	}
+
+	uinfo->value.enumerated.items = count;
+
+	if (uinfo->value.enumerated.item >= count)
+		uinfo->value.enumerated.item = count - 1;
+
+	name = uinfo->value.enumerated.name;
+	count = uinfo->value.enumerated.item;
+
+	ret = aw88081_dev_get_prof_name(aw88081->aw_pa, count, &prof_name);
+	if (ret) {
+		strscpy(uinfo->value.enumerated.name, "null",
+						strlen("null") + 1);

Np real use fot this hand computed length. Using the 2 parameters version of strscpy() should just be fine, I think.

+		return 0;
+	}
+
+	strscpy(name, prof_name, sizeof(uinfo->value.enumerated.name));

If uinfo->value.enumerated.name was used directly as in the if block above, then 'name' could be removed and the 2 parameters only variant of strscpy() coulb be used instead, I think.

+
+	return 0;
+}

...

+static int aw88081_init(struct aw88081 *aw88081, struct i2c_client *i2c, struct regmap *regmap)
+{
+	struct aw_device *aw_dev;
+	unsigned int chip_id;
+	int ret;
+
+	/* read chip id */
+	ret = regmap_read(regmap, AW88081_ID_REG, &chip_id);
+	if (ret) {
+		dev_err(&i2c->dev, "%s read chipid error. ret = %d", __func__, ret);
+		return ret;
+	}
+	if (chip_id != AW88081_CHIP_ID) {
+		dev_err(&i2c->dev, "unsupported device");
+		return -ENXIO;
+	}
+
+	dev_dbg(&i2c->dev, "chip id = %x\n", chip_id);
+
+	aw_dev = devm_kzalloc(&i2c->dev, sizeof(*aw_dev), GFP_KERNEL);
+	if (!aw_dev)
+		return -ENOMEM;
+
+	aw88081->aw_pa = aw_dev;
+	aw_dev->i2c = i2c;
+	aw_dev->regmap = regmap;
+	aw_dev->dev = &i2c->dev;
+	aw_dev->chip_id = AW88081_CHIP_ID;
+	aw_dev->acf = NULL;
+	aw_dev->prof_info.prof_desc = NULL;
+	aw_dev->prof_info.count = 0;

No need to init here, and channel below, unless an explicit 0 is more informative than the kzalloc() above.

+	aw_dev->prof_info.prof_type = AW88395_DEV_NONE_TYPE_ID;
+	aw_dev->channel = 0;
+	aw_dev->fw_status = AW88081_DEV_FW_FAILED;
+	aw_dev->fade_step = AW88081_VOLUME_STEP_DB;
+	aw_dev->volume_desc.ctl_volume = AW88081_VOL_DEFAULT_VALUE;
+	aw_dev->volume_desc.mute_volume = AW88081_MUTE_VOL;
+	aw88081_parse_channel_dt(aw88081);
+
+	return ret;
+}

...

+static int aw88081_request_firmware_file(struct aw88081 *aw88081)
+{
+	const struct firmware *cont = NULL;
+	int ret;
+
+	aw88081->aw_pa->fw_status = AW88081_DEV_FW_FAILED;
+
+	ret = request_firmware(&cont, AW88081_ACF_FILE, aw88081->aw_pa->dev);
+	if (ret)
+		return dev_err_probe(aw88081->aw_pa->dev, ret,
+					"load [%s] failed!", AW88081_ACF_FILE);
+
+	dev_dbg(aw88081->aw_pa->dev, "loaded %s - size: %zu\n",
+			AW88081_ACF_FILE, cont ? cont->size : 0);
+
+	aw88081->aw_cfg = devm_kzalloc(aw88081->aw_pa->dev, cont->size + sizeof(int), GFP_KERNEL);
+	if (!aw88081->aw_cfg) {
+		release_firmware(cont);
+		return -ENOMEM;
+	}
+	aw88081->aw_cfg->len = (int)cont->size;
+	memcpy(aw88081->aw_cfg->data, cont->data, cont->size);
+	release_firmware(cont);
+
+	ret = aw88395_dev_load_acf_check(aw88081->aw_pa, aw88081->aw_cfg);
+	if (ret) {
+		dev_err(aw88081->aw_pa->dev, "load [%s] failed !", AW88081_ACF_FILE);

return dev_err_probe() to be consistent and less verbose.

+		return ret;
+	}
+
+	mutex_lock(&aw88081->lock);
+	/* aw device init */
+	ret = aw88081_dev_init(aw88081, aw88081->aw_cfg);
+	if (ret)
+		dev_err(aw88081->aw_pa->dev, "dev init failed");

return dev_err_probe() to be consistent?

+	mutex_unlock(&aw88081->lock);
+
+	return ret;
+}


...

+static int aw88081_i2c_probe(struct i2c_client *i2c)
+{
+	struct aw88081 *aw88081;
+	int ret;
+
+	ret = i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C);
+	if (!ret)
+		return dev_err_probe(&i2c->dev, -ENXIO, "check_functionality failed");
+
+	aw88081 = devm_kzalloc(&i2c->dev, sizeof(*aw88081), GFP_KERNEL);
+	if (!aw88081)
+		return -ENOMEM;
+
+	mutex_init(&aw88081->lock);
+
+	i2c_set_clientdata(i2c, aw88081);
+
+	aw88081->regmap = devm_regmap_init_i2c(i2c, &aw88081_regmap_config);
+	if (IS_ERR(aw88081->regmap)) {
+		ret = PTR_ERR(aw88081->regmap);
+		return dev_err_probe(&i2c->dev, ret, "failed to init regmap: %d\n", ret);

No need to repeat 'ret' at theend of the message.

+	}
+
+	/* aw pa init */
+	ret = aw88081_init(aw88081, i2c, aw88081->regmap);
+	if (ret)
+		return ret;
+
+	ret = devm_snd_soc_register_component(&i2c->dev,
+			&soc_codec_dev_aw88081,
+			aw88081_dai, ARRAY_SIZE(aw88081_dai));
+	if (ret)
+		dev_err(&i2c->dev, "failed to register aw88081: %d", ret);

dev_err_probe() to be consistent?

+
+	return ret;
+}

...

CJ




[Index of Archives]     [Pulseaudio]     [Linux Audio Users]     [ALSA Devel]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]

  Powered by Linux