Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 2 Jun 2020 15:14:17 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Vaibhav Agarwal <vaibhav.sr@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alex Elder <elder@kernel.org>, Johan Hovold <johan@kernel.org>,
        Mark Greer <mgreer@animalcreek.com>,
        Takashi Iwai <tiwai@suse.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Mark Brown <broonie@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        devel@driverdev.osuosl.org, alsa-devel@alsa-project.org,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        linux-kernel@vger.kernel.org, greybus-dev@lists.linaro.org
Subject: Re: [RESEND PATCH v1 2/6] staging: greybus: audio: Maintain jack
 list within GB Audio module
Message-ID: <20200602121417.GE30374@kadam>
References: <cover.1591040859.git.vaibhav.sr@gmail.com>
 <ccb39352a30ab39df1534efafc9415aa89b156fa.1591040859.git.vaibhav.sr@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ccb39352a30ab39df1534efafc9415aa89b156fa.1591040859.git.vaibhav.sr@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jun 02, 2020 at 10:51:11AM +0530, Vaibhav Agarwal wrote:
> As per the current implementation for GB codec driver, a jack list is
> maintained for each module. And it expects the list to be populated by
> the snd_soc_jack structure which would require modifications in
> mainstream code.
> 
> However, this is not a necessary requirement and the list can be easily
> maintained within gbaudio_module_info as well. This patch provides the
> relevant changes for the same.
> 
> Signed-off-by: Vaibhav Agarwal <vaibhav.sr@gmail.com>
> ---
>  drivers/staging/greybus/audio_codec.c  | 76 ++++++++++++++------------
>  drivers/staging/greybus/audio_codec.h  | 10 +++-
>  drivers/staging/greybus/audio_module.c | 20 ++++---
>  3 files changed, 60 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
> index ebf8484f0ae7..a2ee587e5a79 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -712,7 +712,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  			     struct snd_soc_card *card)
>  {
>  	int ret;
> -
> +	struct gbaudio_jack *gba_jack, *n;
>  	struct snd_soc_jack *jack;

Because we got rid of the jack pointer then we can re-use the name here.

	struct gbaudio_jack *jack, *n;

We still don't want the "struct snd_soc_jack *jack;" pointer.

>  	struct snd_soc_jack_pin *headset, *button;
>  
> @@ -728,7 +728,8 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  
>  	headset->pin = module->jack_name;
>  	headset->mask = module->jack_mask;
> -	jack = &module->headset_jack;
> +	gba_jack = &module->headset;
> +	jack = &gba_jack->jack;

Use module->headset.jack directly.

>  
>  	ret = snd_soc_card_jack_new(card, module->jack_name, module->jack_mask,
>  				    jack, headset, 1);
> @@ -737,6 +738,9 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  		return ret;
>  	}
>  
> +	/* Add to module's jack list */
> +	list_add(&gba_jack->list, &module->jack_list);


Here as well.

> +
>  	if (!module->button_mask)
>  		return 0;
>  
> @@ -745,20 +749,24 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  	button = devm_kzalloc(module->dev, sizeof(*headset), GFP_KERNEL);
>  	if (!button) {
>  		ret = -ENOMEM;
> -		goto free_headset;
> +		goto free_jack;

Let's call the label "free_jacks" (plural).

>  	}
>  
>  	button->pin = module->button_name;
>  	button->mask = module->button_mask;
> -	jack = &module->button_jack;
> +	gba_jack = &module->button;
> +	jack = &gba_jack->jack;
>  
>  	ret = snd_soc_card_jack_new(card, module->button_name,
>  				    module->button_mask, jack, button, 1);
>  	if (ret) {
>  		dev_err(module->dev, "Failed to create button jack\n");
> -		goto free_headset;
> +		goto free_jack;
>  	}
>  
> +	/* Add to module's jack list */
> +	list_add(&gba_jack->list, &module->jack_list);
> +
>  	/*
>  	 * Currently, max 4 buttons are supported with following key mapping
>  	 * BTN_0 = KEY_MEDIA
> @@ -768,58 +776,55 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  	 */
>  
>  	if (module->button_mask & SND_JACK_BTN_0) {
> -		ret = snd_jack_set_key(module->button_jack.jack, SND_JACK_BTN_0,
> +		ret = snd_jack_set_key(jack->jack, SND_JACK_BTN_0,
>  				       KEY_MEDIA);
>  		if (ret) {
>  			dev_err(module->dev, "Failed to set BTN_0\n");
> -			goto free_button;
> +			goto free_jack;
>  		}
>  	}
>  
>  	if (module->button_mask & SND_JACK_BTN_1) {
> -		ret = snd_jack_set_key(module->button_jack.jack, SND_JACK_BTN_1,
> +		ret = snd_jack_set_key(jack->jack, SND_JACK_BTN_1,
>  				       KEY_VOICECOMMAND);
>  		if (ret) {
>  			dev_err(module->dev, "Failed to set BTN_1\n");
> -			goto free_button;
> +			goto free_jack;
>  		}
>  	}
>  
>  	if (module->button_mask & SND_JACK_BTN_2) {
> -		ret = snd_jack_set_key(module->button_jack.jack, SND_JACK_BTN_2,
> +		ret = snd_jack_set_key(jack->jack, SND_JACK_BTN_2,
>  				       KEY_VOLUMEUP);
>  		if (ret) {
>  			dev_err(module->dev, "Failed to set BTN_2\n");
> -			goto free_button;
> +			goto free_jack;
>  		}
>  	}
>  
>  	if (module->button_mask & SND_JACK_BTN_3) {
> -		ret = snd_jack_set_key(module->button_jack.jack, SND_JACK_BTN_3,
> +		ret = snd_jack_set_key(jack->jack, SND_JACK_BTN_3,
>  				       KEY_VOLUMEDOWN);
>  		if (ret) {
>  			dev_err(module->dev, "Failed to set BTN_0\n");
> -			goto free_button;
> +			goto free_jack;
>  		}
>  	}
>  
>  	/* FIXME
>  	 * verify if this is really required
>  	set_bit(INPUT_PROP_NO_DUMMY_RELEASE,
> -		module->button_jack.jack->input_dev->propbit);
> +		module->button->jack->jack->input_dev->propbit);
>  	*/
>  
>  	return 0;
>  
> -free_button:
> -	jack = &module->button_jack;
> -	snd_device_free(card->snd_card, jack->jack);
> -	list_del(&jack->list);
> -
> -free_headset:
> -	jack = &module->headset_jack;
> -	snd_device_free(card->snd_card, jack->jack);
> -	list_del(&jack->list);
> +free_jack:
> +	list_for_each_entry_safe(gba_jack, n, &module->jack_list, list) {
> +		jack = &gba_jack->jack;
> +		snd_device_free(card->snd_card, jack->jack);

Since we renamed "gba_jack" to "jack" then this becomes:

		snd_device_free(card->snd_card, jack->jack.jack);

Which is sort of weird, but still okay.

> +		list_del(&gba_jack->list);
> +	}
>  
>  	return ret;
>  }
> @@ -829,6 +834,7 @@ int gbaudio_register_module(struct gbaudio_module_info *module)
>  	int ret;
>  	struct snd_soc_codec *codec;
>  	struct snd_card *card;
> +	struct gbaudio_jack *gba_jack = NULL;

Don't introduce unused assignments.  It just silences static checker
warnings about uninitialized variables and introduces bugs.

Anyway, the same comments for the rest of the patch.  Please don't
introduce so many variables which aren't required and which hurt
grep-ability.

regards,
dan carpenter