[alsa-devel] [PATCH v6 1/6] ALSA: jack: create jack kcontrols for every jack input

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Monday, April 20, 2015 4:06 PM
> To: Jie, Yang
> Cc: broonie at kernel.org; alsa-devel at alsa-project.org; Girdwood, Liam R;
> tanu.kaskinen at linux.intel.com; Liam Girdwood
> Subject: Re: [PATCH v6 1/6] ALSA: jack: create jack kcontrols for every jack
> input
> 
> At Mon, 20 Apr 2015 07:52:20 +0000,
> Jie, Yang wrote:
> >
> > > > @@ -82,10 +84,19 @@ struct snd_jack {
> > > >  	void (*private_free)(struct snd_jack *);  };
> > > >
> > > > +struct snd_jack_kctl {
> > > > +	struct snd_kcontrol *kctl;
> > > > +	struct list_head list;		/* list of controls belong to the same
> > > jack */
> > > > +	unsigned int mask_bits;	/* one of the corresponding bits of
> > > status change will report to this kctl */
> > > > +	void *private_data;
> > > > +	void (*private_free)(struct snd_jack_kctl *jack_kctl); };
> > >
> > > You don't have to expose this.  The hda_jack.c doesn't actually need
> > > these whole bits, as it supposes 1:1 as jack:kctl mapping.  You can
> > > get rid of hda_jack->kctl since snd_jack_report() does it already,
> > > thus the private_data and the destructor can be dropped, too.
> >
> > The patch 3/6 removes had_jack->kctl, here I added priate destructor
> > for HDA jack to reset hda_jack_tbl jacks->nid and jacks->jack to 0.
> > Seems these are needed for HDA jack logic, otherwise, I can already
> > dropped them. :(
> 
> It's not needed, as far as I understand your code.
> 
> Try to look from from the top-level POV: the hda driver just needs to register
> a jack object via snd_jack_new(), reports it, and remove it in destructor.  The
> private_data was needed for tracking the codec-local kctls, but now they are
> managed in jack object itself, thus we don't have to keep eyes on it.  The
> private_free is for removing the kctl list, and since this is gone, we don't have
> to take it, either.  In other words, we don't have to take care of kctls at all in
> hda driver side.  This would be the ideal situation.
 
Thanks for explaining it. Finally I make clear that jack private_free will call
hda_free_jack_priv() to reset jacks->nid/jack, so I have no worry now. :)

> 
> (snip)
> > > snd_kctl_jack_new() itself.  At the same time, the code doing the
> > > same thing in hda_jack.c can be removed.  Then the index argument of
> >
> > A little concern that there may be a little difference here:
> > hda_jack.c:get_unique_index() check the codec kctl list, but in my
> > patch we check card kctl list, I am afraid that it may influence the
> > exist HDA usage, wo just keep what HDA did before for HDA jack.
> >
> > Takashi, do you think it's a risk to change from codec kctl list to
> > card kctl list for HDA part?
> 
> No, there shouldn't.  The id must be unique for the whole card, not specific
> to codecs.  That is, the code looking through
> snd_ctl_find_id() is even safer than the current version.
 
OK, then I feel reassured to replace it. :)

> 
> 
> thanks,
> 
> Takashi