Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <285260c6-b9ff-b5f5-4e98-03ab4d6514c9@gmail.com>
Date:   Thu, 21 Sep 2023 10:10:08 +0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v3 2/2] ALSA: Add new driver for MARIAN M2 sound card
To:     Takashi Iwai <tiwai@suse.de>
Cc:     perex@perex.cz, tiwai@suse.com, corbet@lwn.net,
        alsa-devel@alsa-project.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
        skhan@linuxfoundation.org
References: <20230920151610.113880-1-ivan.orlov0322@gmail.com>
 <20230920151610.113880-2-ivan.orlov0322@gmail.com>
 <87edisvny4.wl-tiwai@suse.de>
Content-Language: en-US
From:   Ivan Orlov <ivan.orlov0322@gmail.com>
In-Reply-To: <87edisvny4.wl-tiwai@suse.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/20/23 19:55, Takashi Iwai wrote:
>> +	spin_lock(&marian->reglock);
>> +	marian_generic_set_dco(marian, ucontrol->value.integer.value[0]);
>> +	spin_unlock(&marian->reglock);
> 
> The control get/put callbacks can sleep, hence usually it's
> spin_lock_irq().  Or if the all places for this lock are sleepable
> context, use a mutex instead.
> 

Alright, it looks like reglock is used in sleepable context only, so 
I'll replace it with mutex. Thanks!

>> +static int marian_control_pcm_loopback_info(struct snd_kcontrol *kcontrol,
>> +					    struct snd_ctl_elem_info *uinfo)
>> +{
>> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
>> +	uinfo->count = 1;
>> +	uinfo->value.integer.min = 0;
>> +	uinfo->value.integer.max = 1;
>> +
>> +	return 0;
> 
> This can be replaced with snd_ctl_boolean_mono_info.
> 
>

Oh, I forgot to use 'snd_ctl_boolean_mono_info' here. Will be done.

>> +}
>> +
>> +static int marian_control_pcm_loopback_get(struct snd_kcontrol *kcontrol,
>> +					   struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct marian_card *marian = snd_kcontrol_chip(kcontrol);
>> +
>> +	ucontrol->value.integer.value[0] = marian->loopback;
>> +
>> +	return 0;
>> +}
>> +
>> +static int marian_control_pcm_loopback_put(struct snd_kcontrol *kcontrol,
>> +					   struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct marian_card *marian = snd_kcontrol_chip(kcontrol);
>> +
>> +	marian->loopback = ucontrol->value.integer.value[0];
> 
> Better to normalize with !!ucontrol->value.integer.value[0].
> The value check isn't done as default.
> 
Will be fixed, thanks!

>> +static int marian_control_pcm_loopback_create(struct marian_card *marian)
>> +{
>> +	struct snd_kcontrol_new c = {
>> +		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
>> +		.name = "Loopback",
> 
> Better to have "Switch" suffix.
>

Yeah, and I guess some other controls also have to be renamed. Will be 
done :)

>> +static int marian_m2_output_frame_mode_create(struct marian_card *marian, char *label, u32 idx)
>> +{
>> +	struct snd_kcontrol_new c = {
>> +		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
>> +		.name = label,
>> +		.private_value = idx,
>> +		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
> 
> Does this have to be VOLATILE?  Some others look also dubious.
> Basically you set the value via this mixer element, then it's
> persistent.
>

As I understand, some controls which depend on external inputs (like the 
'Input 1 Freq', which measures frequency on the Input 1), should be 
volatile, right?

This one definitely shouldn't, so I will change it to persistent.

Thank you for your prompt reply and review!

--
Kind regards,
Ivan Orlov

> 
> thanks,
> 
> Takashi