On Fri, May 30, 2008 at 10:15:43AM +0200, Takashi Iwai wrote:

The implementation looks good, simple enough.

Thanks.

Mark Brown wrote:

quoted

+config SND_JACK
+	tristate
+	depends on SND
+	depends on INPUT

The code is small, and I don't see a big merit to make it a module.

I agree - I had meant to flag this up, actually.  I hadn't been sure if
some of the other objects were being built conditionally for size or not
since they seemed relatively small too.  When I respin I'll remove this
option.

quoted

+	snprintf(jack->name, sizeof(jack->name), "%s %s",
+		 card->longname, jack->id);

The longname field could be sometimes really too long and verbose.
I guess shortname would match better.

The general style for input device names tends towards the long and
verbose.  For example, on my work laptop I have devices with names like:

   Macintosh mouse button emulation
   AT Translated Set 2 Keyboard
   Lid Switch
   Power Button (CM)
   Sleep Button (CM)
   AlpsPS/2 ALPS GlidePoint

The ALSA long name seems more idiomatic for this context.

quoted

+int snd_jack_new(struct snd_card *card, const char *id, int type,
+		 struct snd_jack **jjack)

(snip)

quoted

+	jack->input_dev->phys = "ALSA";
+	jack->input_dev->dev.parent = card->dev;

The card->dev pointer might not be initialized always at this stage.
You should check rather at register.

Will do.

Also, someone may want to pass a different device pointer for this.
Passing struct device * to snd_jack_new would be an alternative.

Good idea, that might be useful for ASoC v2 sound cards.  I'll add
something which allows the caller to optionally specify a parent,
defaulting to the sound card if none is given.

The only issue might be for user space figuring out which sound card
corresponds to which jack but if jacks might be implemented outside of
ALSA anyway (which is possible) that could happen anyway.