Re: [greybus-dev] [PATCH v4 1/7] staging: greybus: audio: Update snd_jack FW usage as per new APIs

[Alex Elder <elder@linaro.org>]

On 7/9/20 5:27 AM, Vaibhav Agarwal wrote:
> snd_soc_jack APIs are modified in recent kernel versions. This patch
> updates the codec driver to resolve the compilation errors related to
> jack framework.

Greg has already accepted this series so I won't review this now.  But
I still wanted to provide this comment.

It would be helpful in the future to provide a little more information
about the nature of the changes to these APIs.  As a reviewer I had to
go track them down to get a little more context about what you are doing
here.  So you could say something like:

  Audio jacks are now registered at the card level rather than being
  associated with a CODEC.  The new card-based API allows a jack's pins
  to be supplied when the jack is first registered.  See: 970939964c26
  ("ASoC: Allow to register jacks at the card level")

In other words, don't just say "the APIs changed," say "here is how
the APIs have changed."  This kind of introduction can be very helpful
and time saving for your reviewers.

					-Alex

> Signed-off-by: Vaibhav Agarwal <vaibhav.sr@gmail.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/staging/greybus/audio_codec.c | 54 +++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
> index 08746c85dea6..5d3a5e6a8fe6 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -709,17 +709,26 @@ static struct snd_soc_dai_driver gbaudio_dai[] = {
>  };
>  
>  static int gbaudio_init_jack(struct gbaudio_module_info *module,
> -			     struct snd_soc_codec *codec)
> +			     struct snd_soc_card *card)
>  {
>  	int ret;
> +	struct snd_soc_jack_pin *headset, *button;
>  
>  	if (!module->jack_mask)
>  		return 0;
>  
>  	snprintf(module->jack_name, NAME_SIZE, "GB %d Headset Jack",
>  		 module->dev_id);
> -	ret = snd_soc_jack_new(codec, module->jack_name, module->jack_mask,
> -			       &module->headset_jack);
> +
> +	headset = devm_kzalloc(module->dev, sizeof(*headset), GFP_KERNEL);
> +	if (!headset)
> +		return -ENOMEM;
> +
> +	headset->pin = module->jack_name;
> +	headset->mask = module->jack_mask;
> +
> +	ret = snd_soc_card_jack_new(card, module->jack_name, module->jack_mask,
> +				    &module->headset_jack, headset, 1);
>  	if (ret) {
>  		dev_err(module->dev, "Failed to create new jack\n");
>  		return ret;
> @@ -730,11 +739,21 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  
>  	snprintf(module->button_name, NAME_SIZE, "GB %d Button Jack",
>  		 module->dev_id);
> -	ret = snd_soc_jack_new(codec, module->button_name, module->button_mask,
> -			       &module->button_jack);
> +	button = devm_kzalloc(module->dev, sizeof(*button), GFP_KERNEL);
> +	if (!button) {
> +		ret = -ENOMEM;
> +		goto free_headset;
> +	}
> +
> +	button->pin = module->button_name;
> +	button->mask = module->button_mask;
> +
> +	ret = snd_soc_card_jack_new(card, module->button_name,
> +				    module->button_mask, &module->button_jack,
> +				    button, 1);
>  	if (ret) {
>  		dev_err(module->dev, "Failed to create button jack\n");
> -		return ret;
> +		goto free_headset;
>  	}
>  
>  	/*
> @@ -750,7 +769,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  				       KEY_MEDIA);
>  		if (ret) {
>  			dev_err(module->dev, "Failed to set BTN_0\n");
> -			return ret;
> +			goto free_button;
>  		}
>  	}
>  
> @@ -759,7 +778,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  				       KEY_VOICECOMMAND);
>  		if (ret) {
>  			dev_err(module->dev, "Failed to set BTN_1\n");
> -			return ret;
> +			goto free_button;
>  		}
>  	}
>  
> @@ -768,7 +787,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  				       KEY_VOLUMEUP);
>  		if (ret) {
>  			dev_err(module->dev, "Failed to set BTN_2\n");
> -			return ret;
> +			goto free_button;
>  		}
>  	}
>  
> @@ -777,7 +796,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  				       KEY_VOLUMEDOWN);
>  		if (ret) {
>  			dev_err(module->dev, "Failed to set BTN_0\n");
> -			return ret;
> +			goto free_button;
>  		}
>  	}
>  
> @@ -788,6 +807,16 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
>  	*/
>  
>  	return 0;
> +
> +free_button:
> +	snd_device_free(card->snd_card, module->button_jack.jack);
> +	list_del(&module->button_jack.list);
> +
> +free_headset:
> +	snd_device_free(card->snd_card, module->headset_jack.jack);
> +	list_del(&module->headset_jack.list);
> +
> +	return ret;
>  }
>  
>  int gbaudio_register_module(struct gbaudio_module_info *module)
> @@ -815,7 +844,7 @@ int gbaudio_register_module(struct gbaudio_module_info *module)
>  		return -EINVAL;
>  	}
>  
> -	ret = gbaudio_init_jack(module, codec);
> +	ret = gbaudio_init_jack(module, component->card);
>  	if (ret) {
>  		up_write(&card->controls_rwsem);
>  		return ret;
> @@ -942,7 +971,8 @@ void gbaudio_unregister_module(struct gbaudio_module_info *module)
>  
>  #ifdef CONFIG_SND_JACK
>  	/* free jack devices for this module from codec->jack_list */
> -	list_for_each_entry_safe(jack, next_j, &codec->jack_list, list) {
> +	list_for_each_entry_safe(jack, next_j, &component->card->jack_list,
> +				 list) {
>  		if (jack == &module->headset_jack)
>  			mask = GBCODEC_JACK_MASK;
>  		else if (jack == &module->button_jack)
>