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.