Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 26 May 2022 09:30:01 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Vitaly Rodionov <vitalyr@opensource.cirrus.com>
CC:     Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        Mark Brown <broonie@kernel.org>, <alsa-devel@alsa-project.org>,
        <patches@opensource.cirrus.com>, <linux-kernel@vger.kernel.org>,
        Stefan Binding <sbinding@opensource.cirrus.com>
Subject: Re: [PATCH v4 01/17] ALSA: hda: hda_cs_dsp_ctl: Add Library to
 support CS_DSP ALSA controls
Message-ID: <20220526093001.GP38351@ediswmail.ad.cirrus.com>
References: <20220525131638.5512-1-vitalyr@opensource.cirrus.com>
 <20220525131638.5512-2-vitalyr@opensource.cirrus.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220525131638.5512-2-vitalyr@opensource.cirrus.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Wed, May 25, 2022 at 02:16:22PM +0100, 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.
> 
> The apis to add and remove the controls start new threads when
> adding/removing controls since it is possible that setting an ALSA
> control would end up calling this api, which would then deadlock.
> 
> In the future, it will be necessary to load/unload firmware (and
> firmware ALSA controls) using a separate ALSA control, which means
> that the ability to call these apis from another ALSA control is
> required.
> 
> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com>
> ---
>  
> Changes since v2:
>  - Updated commit message to describe reasons
>    for adding/removing controls asynchronously
>  - Removed unnecessary code which handled unused
>    tlv or acked controls.
>  - Removed code which handled outdate firmware
>    versions when adding controls
>  
> +static unsigned int wmfw_convert_flags(unsigned int in, unsigned int len)
> +{
> +	unsigned int out, rd, wr, vol;
> +
> +	if (len > ADSP_MAX_STD_CTRL_SIZE) {
> +		rd = SNDRV_CTL_ELEM_ACCESS_TLV_READ;
> +		wr = SNDRV_CTL_ELEM_ACCESS_TLV_WRITE;
> +		vol = SNDRV_CTL_ELEM_ACCESS_VOLATILE;
> +
> +		out = SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
> +	} else {
> +		rd = SNDRV_CTL_ELEM_ACCESS_READ;
> +		wr = SNDRV_CTL_ELEM_ACCESS_WRITE;
> +		vol = SNDRV_CTL_ELEM_ACCESS_VOLATILE;
> +
> +		out = 0;
> +	}

This if should be changed if we are no longer supporting the TLV
controls, you want to report an error if we exceed the max
control size not switch to the TLV access flags.

> +static void hda_cs_dsp_ctl_add_work(struct work_struct *work)
> +{
> +	struct hda_cs_dsp_coeff_ctl *ctl = container_of(work,
> +							struct hda_cs_dsp_coeff_ctl,
> +							add_work);
> +	struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl;
> +	struct snd_kcontrol_new *kcontrol;
> +
> +	kcontrol = kzalloc(sizeof(*kcontrol), GFP_KERNEL);
> +	if (!kcontrol)
> +		return;
> +
> +	kcontrol->name = ctl->name;
> +	kcontrol->info = hda_cs_dsp_coeff_info;
> +	kcontrol->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> +	kcontrol->tlv.c = snd_soc_bytes_tlv_callback;
> +	kcontrol->private_value = (unsigned long)&ctl->bytes_ext;

Don't need to set tlv.c or the private_value for bytes_ext if we
are not using TLVs.

> +struct hda_cs_dsp_ctl_info {
> +	struct snd_card *card;
> +	enum hda_cs_dsp_fw_id fw_type;
> +	const char *amp_name;

Might be better to call this something slightly more generic than
amp_name. Just incase this stuff gets used with a CODEC or
something in the future.

Thanks,
Charles