Message-ID: <s5h38qj2cqj.wl%tiwai@suse.de>
Date:	Fri, 09 Aug 2013 15:52:04 +0200
From:	Takashi Iwai <tiwai@...e.de>
To:	"Felipe F. Tonello" <eu@...ipetonello.com>
Cc:	alsa-devel@...a-project.org, Jaroslav Kysela <perex@...ex.cz>,
	Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] ALSA: Added jack detection KControl support

At Thu,  8 Aug 2013 23:21:55 -0700,
Felipe F. Tonello wrote:
> 
> From: "Felipe F. Tonello" <eu@...ipetonello.com>
> 
> This patch adds jack support for ALSA KControl.
> 
> This support is necessary since the new kcontrol is used by user-space
> daemons, such as PulseAudio(>=2.0), to do jack detection.)
> 
> Also, HDA's CONFIG_SND_HDA_INPUT_JACK is disabled because it causes to use standard
> snd_jack_new() to create jacks. This can cause conflict since this codec creates
> jack controls directly.
> 
> It makes sure that all codecs using ALSA jack API are updated to use the new
> API.

Well, while this is a good move forward, this patch is a bit too
intrusive as a single patch.  It breaks way too many.  "Break then 
fix" is no good attitude, especially when it's just something for
future.

So, in general, I prefer a way with more gradual changes.
Let's see in details below:

> 
> Signed-off-by: Felipe F. Tonello <eu@...ipetonello.com>
> ---
>  include/sound/control.h            |  2 +-
>  include/sound/jack.h               |  8 ++--
>  include/sound/soc.h                |  2 +-
>  sound/core/Kconfig                 |  1 +
>  sound/core/ctljack.c               |  3 +-
>  sound/core/jack.c                  | 88 +++++++++++++++++++++++++++++++++++---
>  sound/pci/hda/Kconfig              |  8 ----
>  sound/pci/oxygen/xonar_wm87x6.c    |  2 +-
>  sound/soc/fsl/wm1133-ev1.c         |  4 +-
>  sound/soc/mid-x86/mfld_machine.c   |  4 +-
>  sound/soc/omap/ams-delta.c         |  2 +-
>  sound/soc/omap/omap-abe-twl6040.c  |  2 +-
>  sound/soc/omap/omap-twl4030.c      |  2 +-
>  sound/soc/omap/rx51.c              |  4 +-
>  sound/soc/pxa/hx4700.c             |  2 +-
>  sound/soc/pxa/palm27x.c            |  2 +-
>  sound/soc/pxa/ttc-dkb.c            |  6 +--
>  sound/soc/pxa/z2.c                 |  2 +-
>  sound/soc/samsung/goni_wm8994.c    |  5 ++-
>  sound/soc/samsung/h1940_uda1380.c  |  2 +-
>  sound/soc/samsung/littlemill.c     | 10 ++---
>  sound/soc/samsung/lowland.c        |  6 +--
>  sound/soc/samsung/rx1950_uda1380.c |  2 +-
>  sound/soc/samsung/smartq_wm8987.c  |  2 +-
>  sound/soc/samsung/speyside.c       |  6 +--
>  sound/soc/samsung/tobermory.c      |  4 +-
>  sound/soc/soc-jack.c               |  5 ++-
>  sound/soc/tegra/tegra_alc5632.c    |  2 +-
>  sound/soc/tegra/tegra_rt5640.c     |  2 +-
>  sound/soc/tegra/tegra_wm8903.c     |  4 +-
>  30 files changed, 135 insertions(+), 59 deletions(-)
> 
> diff --git a/include/sound/control.h b/include/sound/control.h
> index 5358892..ffeb6b6 100644
> --- a/include/sound/control.h
> +++ b/include/sound/control.h
> @@ -242,6 +242,6 @@ void snd_ctl_sync_vmaster(struct snd_kcontrol *kctl, bool hook_only);
>  struct snd_kcontrol *
>  snd_kctl_jack_new(const char *name, int idx, void *private_data);
>  void snd_kctl_jack_report(struct snd_card *card,
> -			  struct snd_kcontrol *kctl, bool status);
> +                          struct snd_kcontrol *kctl, bool status);

Please don't include space or coding-fix changes.


>  
>  #endif	/* __SOUND_CONTROL_H */
> diff --git a/include/sound/jack.h b/include/sound/jack.h
> index 5891657..0d36f20 100644
> --- a/include/sound/jack.h
> +++ b/include/sound/jack.h
> @@ -26,6 +26,7 @@
>  #include <sound/core.h>
>  
>  struct input_dev;
> +struct snd_kcontrol;
>  
>  /**
>   * Jack types which can be reported.  These values are used as a
> @@ -58,11 +59,12 @@ enum snd_jack_types {
>  
>  struct snd_jack {
>  	struct input_dev *input_dev;
> +	struct snd_kcontrol *kctl[SND_JACK_SWITCH_TYPES]; /* control for each key */
>  	int registered;
>  	int type;
>  	const char *id;
>  	char name[100];
> -	unsigned int key[6];   /* Keep in sync with definitions above */
> +	unsigned int key[SND_JACK_SWITCH_TYPES];   /* Keep in sync with definitions above */
>  	void *private_data;
>  	void (*private_free)(struct snd_jack *);
>  };
> @@ -70,7 +72,7 @@ struct snd_jack {
>  #ifdef CONFIG_SND_JACK
>  
>  int snd_jack_new(struct snd_card *card, const char *id, int type,
> -		 struct snd_jack **jack);
> +                 int idx, struct snd_jack **jack);

The biggest point here is that the patch changes the API function,
from both API and behavioral POV.  That's why you need to modify so
many patches in a single patch.

Try to create a new function doing this and keep the old API and
behavior as is.  Then adapt / replace with the new API gradually.
And in the last step, you can obsolete the former API.

Or, at least keep snd_sock_jack_new() as is without additional index
but just use idx=0.  Then a half of the whole patch can be reduced.

Above all, the multiple indices don't work anyway with the snd_jack
stuff in the current form.  The index was introduced only for kjack,
and for HD-audio, snd_jack is provided just for a compatibility
reason, thus it doesn't matter much even if the multiple indices don't
work with it. 

That being said, before going further, we need to consider how to
handle the input jack stuff with multiple indices.


>  void snd_jack_set_parent(struct snd_jack *jack, struct device *parent);
>  int snd_jack_set_key(struct snd_jack *jack, enum snd_jack_types type,
>  		     int keytype);
> @@ -80,7 +82,7 @@ void snd_jack_report(struct snd_jack *jack, int status);
>  #else
>  
>  static inline int snd_jack_new(struct snd_card *card, const char *id, int type,
> -			       struct snd_jack **jack)
> +                               int idx, struct snd_jack **jack)
>  {
>  	return 0;
>  }
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 6eabee7..31bea52 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -436,7 +436,7 @@ int snd_soc_platform_trigger(struct snd_pcm_substream *substream,
>  
>  /* Jack reporting */
>  int snd_soc_jack_new(struct snd_soc_codec *codec, const char *id, int type,
> -		     struct snd_soc_jack *jack);
> +                     int idx, struct snd_soc_jack *jack);
>  void snd_soc_jack_report(struct snd_soc_jack *jack, int status, int mask);
>  int snd_soc_jack_add_pins(struct snd_soc_jack *jack, int count,
>  			  struct snd_soc_jack_pin *pins);
> diff --git a/sound/core/Kconfig b/sound/core/Kconfig
> index c0c2f57..8167615 100644
> --- a/sound/core/Kconfig
> +++ b/sound/core/Kconfig
> @@ -20,6 +20,7 @@ config SND_COMPRESS_OFFLOAD
>  # to avoid having to force INPUT on.
>  config SND_JACK
>  	bool
> +	select SND_KCTL_JACK
>  
>  config SND_SEQUENCER
>  	tristate "Sequencer support"
> diff --git a/sound/core/ctljack.c b/sound/core/ctljack.c
> index e4b38fb..441158d 100644
> --- a/sound/core/ctljack.c
> +++ b/sound/core/ctljack.c
> @@ -14,7 +14,7 @@
>  #include <sound/core.h>
>  #include <sound/control.h>
>  
> -#define jack_detect_kctl_info	snd_ctl_boolean_mono_info
> +#define	jack_detect_kctl_info	snd_ctl_boolean_mono_info
>  
>  static int jack_detect_kctl_get(struct snd_kcontrol *kcontrol,
>  				struct snd_ctl_elem_value *ucontrol)
> @@ -38,6 +38,7 @@ snd_kctl_jack_new(const char *name, int idx, void *private_data)
>  	kctl = snd_ctl_new1(&jack_detect_kctl, private_data);
>  	if (!kctl)
>  		return NULL;
> +
>  	snprintf(kctl->id.name, sizeof(kctl->id.name), "%s Jack", name);
>  	kctl->id.index = idx;
>  	kctl->private_value = 0;

No unnecessary space changes please.


> diff --git a/sound/core/jack.c b/sound/core/jack.c
> index b35fe73..128154b 100644
> --- a/sound/core/jack.c
> +++ b/sound/core/jack.c
> @@ -24,6 +24,7 @@
>  #include <linux/module.h>
>  #include <sound/jack.h>
>  #include <sound/core.h>
> +#include <sound/control.h>
>  
>  static int jack_switch_types[SND_JACK_SWITCH_TYPES] = {
>  	SW_HEADPHONE_INSERT,
> @@ -37,6 +38,7 @@ static int jack_switch_types[SND_JACK_SWITCH_TYPES] = {
>  static int snd_jack_dev_free(struct snd_device *device)
>  {
>  	struct snd_jack *jack = device->device_data;
> +	int i;
>  
>  	if (jack->private_free)
>  		jack->private_free(jack);
> @@ -48,19 +50,54 @@ static int snd_jack_dev_free(struct snd_device *device)
>  	else
>  		input_free_device(jack->input_dev);
>  
> +	/* Free available KControls*/
> +	for (i = 0; i < SND_JACK_SWITCH_TYPES; ++i)
> +		if (jack->kctl[i])
> +			snd_ctl_remove(device->card, jack->kctl[i]);
> +
>  	kfree(jack->id);
>  	kfree(jack);
>  
>  	return 0;
>  }
>  
> +const char * jack_get_name_by_key(const char *name, int key)
> +{
> +	char *jack_name;
> +	size_t jack_name_size;
> +	const char *key_name;
> +
> +	switch(key) {
> +	case SW_HEADPHONE_INSERT: key_name = "Headphone"; break;
> +	case SW_MICROPHONE_INSERT: key_name = "Mic"; break;
> +	case SW_LINEOUT_INSERT: key_name = "Line Out"; break;
> +	case SW_JACK_PHYSICAL_INSERT: key_name = "Mechanical"; break;
> +	case SW_VIDEOOUT_INSERT: key_name = "Video Out"; break;
> +	case SW_LINEIN_INSERT: key_name = "Line In"; break;
> +	default: key_name = "Unknown";
> +	}
> +
> +	/* Avoid duplicate name in KControl */
> +	if (strcmp(name, key_name) != 0) {

Better to check via strstr() or such.
The name can be like "Front Headphone".

> +		/* allocate necessary memory space only */
> +		jack_name_size = strlen(name) + strlen(key_name) + 4;
> +		jack_name = kmalloc(jack_name_size, GFP_KERNEL);

This leads to a memory leak.  Make this function to put a string on
the given buffer instead of returning a string pointer.

> +
> +		snprintf(jack_name, jack_name_size, "%s (%s)", name, key_name);
> +	} else {
> +		jack_name = (char *)name;
> +	}
> +
> +	return jack_name;
> +}
> +
>  static int snd_jack_dev_register(struct snd_device *device)
>  {
>  	struct snd_jack *jack = device->device_data;
>  	struct snd_card *card = device->card;
>  	int err, i;
>  
> -	snprintf(jack->name, sizeof(jack->name), "%s %s",
> +	snprintf(jack->name, sizeof(jack->name), "%s %s Jack",
>  		 card->shortname, jack->id);

This breaks the compatibility with the existing code.
You must not change the name of the existing input jack element.
Some drivers create "Headphone" and some do "Headphone Jack", yes.
It's bad, but too late to change.


>  	jack->input_dev->name = jack->name;
>  
> @@ -81,6 +118,18 @@ static int snd_jack_dev_register(struct snd_device *device)
>  		input_set_capability(jack->input_dev, EV_KEY, jack->key[i]);
>  	}
>  
> +	/* We don't need to free the control, it's freed by snd_ctl_add itself
> +	   if an error occur */
> +	for (i = 0; i < SND_JACK_SWITCH_TYPES; ++i)
> +		if (jack->kctl[i]) {
> +			err = snd_ctl_add(card, jack->kctl[i]);
> +			if (err < 0) {
> +				pr_notice("%s: ALSA Jack Control not available for '%s'\n", __func__,
> +				          jack_get_name_by_key(jack->id, jack_switch_types[i]));
> +				jack->kctl[i] = NULL;
> +			}
> +		}
> +
>  	err = input_register_device(jack->input_dev);
>  	if (err == 0)
>  		jack->registered = 1;
> @@ -91,22 +140,31 @@ static int snd_jack_dev_register(struct snd_device *device)
>  /**
>   * snd_jack_new - Create a new jack
>   * @card:  the card instance
> - * @id:    an identifying string for this jack
> + * @id:    an identifying string for this jack, " Jack" is appended to the
> + *         string
>   * @type:  a bitmask of enum snd_jack_type values that can be detected by
>   *         this jack
> + * @idx:   The index of the ALSA control created to represent the jack.
>   * @jjack: Used to provide the allocated jack object to the caller.
>   *
>   * Creates a new jack object.
>   *
> + * This function creates a Jack Kcontrol for each @type id as "@id @type Jack".
> + * Eg.: A jack with @type SND_JACK_HEADSET will have two KControls created,
> + * "@id Headphone Jack" and "@id Mic Jack". If @id and @type strings are
> + * equals, then this KControl will be named "@id Jack" only.
> + *
>   * Return: Zero if successful, or a negative error code on failure.
>   * On success @jjack will be initialised.
>   */
>  int snd_jack_new(struct snd_card *card, const char *id, int type,
> -		 struct snd_jack **jjack)
> +                 int idx, struct snd_jack **jjack)
>  {
>  	struct snd_jack *jack;
> +	struct snd_kcontrol *kctl;
>  	int err;
>  	int i;
> +
>  	static struct snd_device_ops ops = {
>  		.dev_free = snd_jack_dev_free,
>  		.dev_register = snd_jack_dev_register,
> @@ -129,10 +187,20 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
>  	jack->type = type;
>  
>  	for (i = 0; i < SND_JACK_SWITCH_TYPES; i++)
> -		if (type & (1 << i))
> +		if (type & (1 << i)) {
>  			input_set_capability(jack->input_dev, EV_SW,
>  					     jack_switch_types[i]);
>  
> +			/* card is the private_data */
> +			kctl = snd_kctl_jack_new(jack_get_name_by_key(id, jack_switch_types[i]), idx, card);
> +			if (!kctl) {
> +				err = -ENOMEM;
> +				goto fail_kctl;
> +			}
> +
> +			jack->kctl[i] = kctl;
> +		}
> +
>  	err = snd_device_new(card, SNDRV_DEV_JACK, jack, &ops);
>  	if (err < 0)
>  		goto fail_input;
> @@ -141,6 +209,11 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
>  
>  	return 0;
>  
> +fail_kctl:
> +	for (i = 0; i < SND_JACK_SWITCH_TYPES; ++i)
> +		if (jack->kctl[i])
> +			snd_ctl_remove(card, jack->kctl[i]);
> +
>  fail_input:
>  	input_free_device(jack->input_dev);
>  	kfree(jack->id);
> @@ -232,10 +305,15 @@ void snd_jack_report(struct snd_jack *jack, int status)
>  
>  	for (i = 0; i < ARRAY_SIZE(jack_switch_types); i++) {
>  		int testbit = 1 << i;
> -		if (jack->type & testbit)
> +		if (jack->type & testbit) {
>  			input_report_switch(jack->input_dev,
>  					    jack_switch_types[i],
>  					    status & testbit);
> +
> +			/* Update ALSA KControl interface */
> +			snd_kctl_jack_report((struct snd_card *)jack->kctl[i]->private_data,
> +			                     jack->kctl[i], status & testbit);

Better to do a NULL check.
In this patch, snd_ctl_add() error isn't handled as a fatal error,
thus jack->kctl[i] can be NULL.


> +		}
>  	}
>  
>  	input_sync(jack->input_dev);
> diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
> index 59c5e9c..561abc7 100644
> --- a/sound/pci/hda/Kconfig
> +++ b/sound/pci/hda/Kconfig
> @@ -65,14 +65,6 @@ config SND_HDA_INPUT_BEEP_MODE
>  	  Set 1 to always enable the digital beep interface for HD-audio by
>  	  default.
>  
> -config SND_HDA_INPUT_JACK
> -	bool "Support jack plugging notification via input layer"
> -	depends on INPUT=y || INPUT=SND
> -	select SND_JACK
> -	help
> -	  Say Y here to enable the jack plugging notification via
> -	  input layer.
> -

I understand why you remove this Kconfig.  But by this action, you
introduced an obvious regression, i.e. the input jack control is no
longer created for HD-audio.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists