On Fri, Nov 25, 2022 at 05:27:23PM +0800, wangweidong.a@xxxxxxxxxx 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.
Attachment:
signature.asc
Description: PGP signature
