| Message ID | 20221125092727.17414-1-wangweidong.a@awinic.com |
|---|---|
| Headers | show |
| Series | ASoC: codecs: Add Awinic AW883XX audio amplifier driver | expand |
On Fri, Nov 25, 2022 at 05:27:23PM +0800, wangweidong.a@awinic.com wrote: > +static int aw883xx_fade_time_info(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_info *uinfo) > +{ > + /* set kcontrol info */ > + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; > + uinfo->count = 1; > + uinfo->value.integer.min = 0; > + uinfo->value.integer.max = 1000000; This info callback reports bounds on the value... > +static int aw883xx_set_fade_in_time(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); > + struct aw883xx *aw883xx = snd_soc_component_get_drvdata(component); > + struct aw_device *aw_dev = aw883xx->aw_pa; > + > + if ((ucontrol->value.integer.value[0] > FADE_TIME_MAX) || > + (ucontrol->value.integer.value[0] < FADE_TIME_MIN)) { ...which aren't the same as the values being validated on put. It'd also help if the callbacks included the name of the op they're implementing, it'd make things eaiser to follow. > + dev_err(aw883xx->dev, "set val %ld overflow %d or less than :%d", > + ucontrol->value.integer.value[0], FADE_TIME_MAX, FADE_TIME_MAX); Userspace can use this to spam the logs, just return the error. > + return -1; Return a real error code like -EINVAL. > + aw883xx_dev_set_fade_time(ucontrol->value.integer.value[0], true, aw_dev); > + > + dev_dbg(aw883xx->dev, "step time %ld", ucontrol->value.integer.value[0]); > + return 1; > +} This will always report a change, generating spurious events. Test with the mixer-test kselftest to make sure everything is fine. > +static int aw883xx_dynamic_create_controls(struct aw883xx *aw883xx) > +{ > + struct snd_kcontrol_new *aw883xx_dev_control = NULL; > + char *kctl_name = NULL; > + > + aw883xx_dev_control = devm_kzalloc(aw883xx->codec->dev, > + sizeof(struct snd_kcontrol_new) * AW_KCONTROL_NUM, GFP_KERNEL); > + if (!aw883xx_dev_control) > + return -ENOMEM; > + > + kctl_name = devm_kzalloc(aw883xx->codec->dev, AW_NAME_BUF_MAX, GFP_KERNEL); > + if (!kctl_name) > + return -ENOMEM; > + > + snprintf(kctl_name, AW_NAME_BUF_MAX, "aw_dev_%u_prof", > + aw883xx->aw_pa->channel); > + > + aw883xx_dev_control[0].name = kctl_name; > + aw883xx_dev_control[0].iface = SNDRV_CTL_ELEM_IFACE_MIXER; > + aw883xx_dev_control[0].info = aw883xx_profile_info; > + aw883xx_dev_control[0].get = aw883xx_profile_get; > + aw883xx_dev_control[0].put = aw883xx_profile_set; As far as I can see this dynamic creation stuff is being done so that channel (which I can't find the initialisation for?) can be put into the control names. I can't tell why, if this is to distinguish multiple instances of these devices in the same system the core already has name_prefix which exists for this purpose and allows systems to provide meaningful names. > + memcpy(aw883xx->aw_cfg->data, cont->data, cont->size); > + ret = aw883xx_dev_load_acf_check(aw883xx->aw_cfg); > + if (ret < 0) { > + dev_err(aw883xx->dev, "Load [%s] failed ....!", AW883XX_ACF_FILE); > + vfree(aw883xx->aw_cfg); > + aw883xx->aw_cfg = NULL; > + release_firmware(cont); > + return ret; > + } > + release_firmware(cont); We could just release the firmware immediately after the memcpy(). > +static const struct snd_soc_dapm_widget aw883xx_dapm_widgets[] = { > + /* playback */ > + SND_SOC_DAPM_AIF_IN("AIF_RX", "Speaker_Playback", 0, SND_SOC_NOPM, 0, 0), > + SND_SOC_DAPM_OUTPUT("audio_out"), > + /* capture */ > + SND_SOC_DAPM_AIF_OUT("AIF_TX", "Speaker_Capture", 0, SND_SOC_NOPM, 0, 0), > + SND_SOC_DAPM_INPUT("iv_in"), > +}; Generally the inputs and outputs should correspond to the names of the physical pins on the device so they can be used in the DT bindings to connect things to them. > +static int aw883xx_add_widgets(struct aw883xx *aw883xx) > +{ > + struct snd_soc_dapm_widget *aw_widgets = NULL; > + struct snd_soc_dapm_route *aw_route = NULL; > + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(aw883xx->codec); > + > + /*add widgets*/ > + aw_widgets = devm_kzalloc(aw883xx->dev, > + sizeof(struct snd_soc_dapm_widget) * > + ARRAY_SIZE(aw883xx_dapm_widgets), > + GFP_KERNEL); > + if (!aw_widgets) > + return -ENOMEM; > + > + memcpy(aw_widgets, aw883xx_dapm_widgets, > + sizeof(struct snd_soc_dapm_widget) * ARRAY_SIZE(aw883xx_dapm_widgets)); > + > + snd_soc_dapm_new_controls(dapm, aw_widgets, ARRAY_SIZE(aw883xx_dapm_widgets)); I'm not sure why we're doing the alloc and copy here? > +static ssize_t rw_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) { > + aw883xx->reg_addr = (uint8_t)databuf[0]; > + if (aw883xx->aw_pa->ops.aw_check_rd_access(databuf[0])) > + regmap_write(aw883xx->regmap, databuf[0], databuf[1]); > + } else { > + if (sscanf(buf, "%x", &databuf[0]) == 1) > + aw883xx->reg_addr = (uint8_t)databuf[0]; > + } > + > + return count; > +} Remove all this, if there's a need for this for debug purposes then there's code in the regmap core to provide direct regmap read/write via debugfs. For production use provide ALSA controls for whatever needs controlling. We shouldn't have userspace able to do uncontrolled register writes, that means it can trivially do things which conflict with what the kernel is doing - we've got no real idea what state the device is in. All this sysfs stuff looks like it should go, or at least be in separate clearly explained patches. > +static ssize_t fade_enable_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ This is something that's already exposed via the ALSA API.
From: Weidong Wang <wangweidong.a@awinic.com> The Awinic AW883XX is an I2S/TDM input, high efficiency digital Smart K audio amplifier with an integrated 10.25V smart boost convert Add a DT schema for describing Awinic AW883xx audio amplifiers. They are controlled using I2C. v4 -> v5: Remove the encapsulation of the alsa api Report the error value in the Kcontrol Use dev_dbg instead of pr_debug Change the log print level Removing software restrictions on volume Delete the aw883xx_fw_wrok Apply for gpio using the gpiod api Delete the reg node Delete the fade_step node The fade_step node was removed and the fade_step Kcontrol was added Delete the description of the reg node from aw883xx,awinic.yaml file Delete the sound-channel node from the aw883xx file Change the warning: unused variable 'aw883xx_dt_match' Change the warning: stack frame size (1272) exceeds limit (1024) in 'aw883xx_dev_cfg_load' Weidong Wang (5): ASoC: codecs: Add i2c and codec registration for aw883xx and their associated operation functions ASoC: codecs: Implementation of aw883xx configuration file parsing function ASoC: codecs: aw883xx chip control logic, such as power on and off ASoC: codecs: Configure aw883xx chip register as well as Kconfig and Makefile ASoC: dt-bindings: Add schema for "awinic,aw883xx" .../bindings/sound/awinic,aw883xx.yaml | 49 + sound/soc/codecs/Kconfig | 10 + sound/soc/codecs/Makefile | 7 + sound/soc/codecs/aw883xx/aw883xx.c | 1673 ++++++++++++ sound/soc/codecs/aw883xx/aw883xx.h | 105 + sound/soc/codecs/aw883xx/aw883xx_bin_parse.c | 1312 ++++++++++ sound/soc/codecs/aw883xx/aw883xx_bin_parse.h | 145 ++ sound/soc/codecs/aw883xx/aw883xx_data_type.h | 148 ++ sound/soc/codecs/aw883xx/aw883xx_device.c | 1629 ++++++++++++ sound/soc/codecs/aw883xx/aw883xx_device.h | 543 ++++ sound/soc/codecs/aw883xx/aw883xx_init.c | 635 +++++ .../soc/codecs/aw883xx/aw883xx_pid_2049_reg.h | 2300 +++++++++++++++++ 12 files changed, 8556 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/ awinic,aw883xx.yaml create mode 100644 sound/soc/codecs/aw883xx/aw883xx.c create mode 100644 sound/soc/codecs/aw883xx/aw883xx.h create mode 100644 sound/soc/codecs/aw883xx/aw883xx_bin_parse.c create mode 100644 sound/soc/codecs/aw883xx/aw883xx_bin_parse.h create mode 100644 sound/soc/codecs/aw883xx/aw883xx_data_type.h create mode 100644 sound/soc/codecs/aw883xx/aw883xx_device.c create mode 100644 sound/soc/codecs/aw883xx/aw883xx_device.h create mode 100644 sound/soc/codecs/aw883xx/aw883xx_init.c create mode 100644 sound/soc/codecs/aw883xx/aw883xx_pid_2049_reg.h base-commit: 08ad43d554bacb9769c6a69d5f771f02f5ba411c