Messages in this thread | | | Date | Wed, 22 Jun 2022 10:40:20 +0200 | Subject | Re: [PATCH v7 01/14] ALSA: hda: hda_cs_dsp_ctl: Add Library to support CS_DSP ALSA controls | From | Amadeusz Sławiński <> |
| |
On 6/22/2022 9:46 AM, Vitaly Rodionov wrote: > From: Stefan Binding <sbinding@opensource.cirrus.com> > > The cs35l41 part contains a DSP which is able to run firmware. > The cs_dsp library can be used to control the DSP. > These controls can be exposed to userspace using ALSA controls. > This library adds apis to be able to interface between > cs_dsp and hda drivers and expose the relevant controls as > ALSA controls. > > Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > --- > MAINTAINERS | 1 + > sound/pci/hda/Kconfig | 4 + > sound/pci/hda/Makefile | 2 + > sound/pci/hda/hda_cs_dsp_ctl.c | 207 +++++++++++++++++++++++++++++++++ > sound/pci/hda/hda_cs_dsp_ctl.h | 33 ++++++ > 5 files changed, 247 insertions(+) > create mode 100644 sound/pci/hda/hda_cs_dsp_ctl.c > create mode 100644 sound/pci/hda/hda_cs_dsp_ctl.h >
...
> + > +static unsigned int wmfw_convert_flags(unsigned int in) > +{ > + unsigned int out, rd, wr, vol; > + > + rd = SNDRV_CTL_ELEM_ACCESS_READ; > + wr = SNDRV_CTL_ELEM_ACCESS_WRITE; > + vol = SNDRV_CTL_ELEM_ACCESS_VOLATILE; > + > + out = 0; > + > + if (in) { > + out |= rd; > + if (in & WMFW_CTL_FLAG_WRITEABLE) > + out |= wr; > + if (in & WMFW_CTL_FLAG_VOLATILE) > + out |= vol; > + } else { > + out |= rd | wr | vol; > + } > + > + return out; > +}
This is more question of preference, so you can leave above function as is, but you could also do something like the following, which is bit shorter: static unsigned int wmfw_convert_flags(unsigned int in) { unsigned int out = SNDRV_CTL_ELEM_ACCESS_READ;
if (!in) return SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_VOLATILE;
if (in & WMFW_CTL_FLAG_WRITEABLE) out |= SNDRV_CTL_ELEM_ACCESS_WRITE; if (in & WMFW_CTL_FLAG_VOLATILE) out |= SNDRV_CTL_ELEM_ACCESS_VOLATILE;
return out; }
> + > +static int hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl) > +{ > + struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; > + struct snd_kcontrol_new *kcontrol; > + struct snd_kcontrol *kctl; > + int ret = 0; > + > + if (cs_ctl->len > ADSP_MAX_STD_CTRL_SIZE) { > + dev_err(cs_ctl->dsp->dev, "Control %s: length %zu exceeds maximum %d\n", ctl->name, > + cs_ctl->len, ADSP_MAX_STD_CTRL_SIZE); > + return -EINVAL; > + } > + > + kcontrol = kzalloc(sizeof(*kcontrol), GFP_KERNEL); > + if (!kcontrol) > + return -ENOMEM; > + > + kcontrol->name = ctl->name; > + kcontrol->info = hda_cs_dsp_coeff_info; > + kcontrol->iface = SNDRV_CTL_ELEM_IFACE_MIXER; > + kcontrol->private_value = (unsigned long)ctl; > + kcontrol->access = wmfw_convert_flags(cs_ctl->flags); > + > + kcontrol->get = hda_cs_dsp_coeff_get; > + kcontrol->put = hda_cs_dsp_coeff_put; > + > + kctl = snd_ctl_new1(kcontrol, NULL);
Wouldn't kctl = snd_ctl_new1(kcontrol, ctl); work instead of kcontrol->private_value = (unsigned long)ctl; ... kctl = snd_ctl_new1(kcontrol, NULL); ?
You can then get the value using snd_kcontrol_chip() macro, so instead of doing quite long lines with casts like: struct hda_cs_dsp_coeff_ctl *ctl = (struct hda_cs_dsp_coeff_ctl *)kctl->private_value; you could do struct hda_cs_dsp_coeff_ctl *ctl = snd_kcontrol_chip(kctl);
| |