On Tue, Nov 15, 2022 at 10:24:18AM +0800, wangweidong.a@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, ®_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.