[PATCH V4 1/6] ASoC: codecs: Add i2c and codec registration for aw883xx and their associated operation functions

[Mark Brown]

On Tue, Nov 15, 2022 at 10:24:18AM +0800, wangweidong.a at awinic.com wrote:

> +/*
> + * aw883xx distinguish between codecs and components by version
> + */
> +static struct aw_componet_codec_ops aw_componet_codec_ops = {
> +	.kcontrol_codec = snd_soc_kcontrol_component,
> +	.codec_get_drvdata = snd_soc_component_get_drvdata,
> +	.add_codec_controls = snd_soc_add_component_controls,
> +	.unregister_codec = snd_soc_unregister_component,
> +	.register_codec = snd_soc_register_component,
> +};

I've started looking at this a few times but keep stopping due to
this bit.  CODECs and components (note the spelling BTW) are both
ASoC level concepts but this looks like the driver is trying to
define it's own abstraction using the same terms.  There's
nothing in the commit log or anything that explains this so it's
a bit of work to try to figure out what's going on which makes it
hard to follow.  It would really help to have some explanation of
what's going on rather than having to reverse engineer it from
the patches.

> +	mutex_lock(&aw883xx->dsp_lock);
> +	if (data_type == AW_DSP_16_DATA) {
> +	} else if (data_type == AW_DSP_32_DATA) {
> +	} else {
> +	}

This looks like it should be written as a switch statement.

> +	if ((ucontrol->value.integer.value[0] > FADE_TIME_MAX) ||
> +		(ucontrol->value.integer.value[0] < FADE_TIME_MIN)) {
> +		pr_debug("set val %ld overflow %d or  less than :%d",
> +			ucontrol->value.integer.value[0], FADE_TIME_MAX, FADE_TIME_MAX);
> +		return 0;
> +	}

Invalid values should report an error.

> +	pr_debug("step time %ld", ucontrol->value.integer.value[0]);

Use dev_ prints where possible.

> +	if (!aw883xx->dbg_en_prof) {
> +		dev_info(codec->dev, "profile close");
> +		return 0;
> +	}

This should be a debug print at most.

> +	/* check value valid */
> +	ret = aw883xx_dev_check_profile_index(aw883xx->aw_pa, ucontrol->value.integer.value[0]);
> +	if (ret) {
> +		dev_warn(codec->dev, "unsupported index %ld",
> +					ucontrol->value.integer.value[0]);
> +		return 0;
> +	}

No error messages from control sets, an application could make a
lot of noise in the logs.

> +static int aw883xx_volume_set(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{

> +	vol_desc->ctl_volume = value;
> +
> +	/*get smaller dB*/
> +	compared_vol = AW_GET_MAX_VALUE(vol_desc->ctl_volume,
> +		vol_desc->monitor_volume);
> +
> +	aw883xx_dev_set_volume(aw883xx->aw_pa, compared_vol);

Why is there this extra soft limit on volume?  This looks
confusing.

> +static void aw883xx_fw_wrok(struct work_struct *work)
> +{

wrok?

> +static int aw883xx_gpio_request(struct aw883xx *aw883xx)
> +{
> +	int ret = 0;
> +
> +	if (gpio_is_valid(aw883xx->reset_gpio)) {
> +		ret = devm_gpio_request_one(aw883xx->dev, aw883xx->reset_gpio,
> +			GPIOF_OUT_INIT_LOW, "aw883xx_rst");
> +		if (ret) {
> +			dev_err(aw883xx->dev, "rst request failed");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

Use gpiod_ APIs please for new code, the numeric GPIO API is
being phased out.

> +/*
> + * sys group attribute: reg
> + */
> +static ssize_t reg_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct aw883xx *aw883xx = dev_get_drvdata(dev);
> +	int reg_num = aw883xx->aw_pa->ops.aw_get_reg_num();
> +	ssize_t len = 0;
> +	uint8_t i = 0;
> +	unsigned int reg_val = 0;
> +
> +	for (i = 0; i < reg_num; i++) {
> +		if (aw883xx->aw_pa->ops.aw_check_rd_access(i)) {
> +			regmap_read(aw883xx->regmap, i, &reg_val);
> +			len += snprintf(buf + len, PAGE_SIZE - len,
> +					"reg:0x%02x=0x%04x\n", i, reg_val);
> +		}
> +	}
> +
> +	return len;
> +}

regmap already provides a debugfs interface for you.

> +static ssize_t reg_store(struct device *dev,
> +				struct device_attribute *attr, const char *buf,
> +				size_t count)
> +{
> +	struct aw883xx *aw883xx = dev_get_drvdata(dev);
> +	unsigned int databuf[2] = { 0 };
> +
> +	if (sscanf(buf, "%x %x", &databuf[0], &databuf[1]) == 2)
> +		regmap_write(aw883xx->regmap, databuf[0], databuf[1]);
> +
> +	return count;
> +}

It's not OK to provide a raw register write interface to
userspace, this allows userspace to just go around the back of
the driver and do whatever which makes it impossible to guarantee
that the state of the hardware matches what the driver thinks is
going on.  Needed functionality should go via some abstracted
kernel interface.  For debug use there is a regmap interface
which can do register writes for debug purposes if the kernel is
specially built for it.

Just remove all this debugfs code.

> +static ssize_t fade_step_store(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{

Controls should go through the ALSA APIs, not sysfs.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20221116/809e7ff9/attachment-0001.sig>