[alsa-devel] More HDA NID / control / proc related changes

[Takashi Iwai]

At Mon, 14 Dec 2009 15:32:54 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Mon, 14 Dec 2009, Takashi Iwai wrote:
> 
> > At Mon, 14 Dec 2009 13:34:19 +0100 (CET),
> > Jaroslav Kysela wrote:
> >>
> >> On Mon, 14 Dec 2009, Takashi Iwai wrote:
> >>
> >>> At Mon, 14 Dec 2009 09:46:50 +0100 (CET),
> >>> Jaroslav Kysela wrote:
> >>>>
> >>>> On Tue, 8 Dec 2009, Jaroslav Kysela wrote:
> >>>>
> >>>>> Hi all,
> >>>>>
> >>>>> 	continuing the work of extending the HDA codec proc contents, I would
> >>>>> like to introduce two new patches:
> >>>>>
> >>>>> ALSA: hda - add more NID->Control mapping
> >>>>> ALSA: hda - introduce HDA_SUBDEV_AMP_FLAG (ControlAmp in proc)
> >>>>>
> >>>>> 	Patches can be obtained here:
> >>>>>
> >>>>> http://git.alsa-project.org/?p=alsa-kernel.git;a=shortlog;h=topic/hda-nid
> >>>>
> >>>> I merged these patches and added patch named:
> >>>>
> >>>> ALSA: hda - simplify usage of HDA_SUBDEV_AMP_FLAG
> >>>>
> >>>> .. to my main GIT tree.
> >>>>
> >>>> The next idea is to modify hda-analyzer to show the codec routes and
> >>>> assigned mixer controls.
> >>>
> >>> snd_hda_add_nids() looks buggy to me.  It doesn't increment nids
> >>> pointer.
> >>
> >> Good point. Fixed now.
> >>
> >>> Also, snd_hda_add_nids() and snd_hda_nid_add() are a bit confusing and
> >>> inconsistent, IMO.
> >>
> >> It is consistent with ctl functions:
> >>
> >> snd_hda_ctl_add -> snd_hda_nid_add
> >> snd_hda_add_new_ctls -> snd_hda_add_nids
> >
> > Ah.  But both function names look too similar, I'd say, and the
> > relationship above can't be seen obviously from the names.
> 
> Any idea to rename functions?

Nor particular in mind, but I'd name like snd_hda_ctl_add_nid() if
it's for snd_hda_ctl_add().

Or, use the same name snd_hda_add_nid() and snd_hda_add_nids(), unify
the argument order, but make the latter accept array, or so. 


> >>> Anyway, it'd be really, really helpful if you make a proper pullable
> >>> branch based on the upstream tree.  Right now I can't pull your
> >>> commits but only do cherry-picks, which is basically stupid when both
> >>> are using GIT.
> >>
> >> I found the possible changes (resolving clashes) during merges very evil,
> >> altough I understand your easy work scheme.
> >
> > Right.  IOW, the commits that have been already published for the
> > public tree shouldn't be rebased.  The rebasing is the most evil thing
> > for the published commits.
> >
> > Rebasing doesn't matter for local commits, of course.  Also, it's also
> > more or less OK for some test trees / branches.  But, never rebase if
> > a branch gets merged.
> >
> >> Also, I don't like the missing
> >> lines in comments (Signed-off-by etc.) for merged patches for all involved
> >> people. It makes more difficult to track the patch flow.
> >
> > Well, the meta info has to be set properly *before* merge.  So, the
> > only question is whether a developed branch is ready for merging or
> > not...
> 
> Unfortunately, I'm not talking about the meta-info. The patch delivery 
> should be in the patch comment itself according to the SubmittingPatches 
> document. For example:
> 
> commit 761c9d45d14e0afa3c0b8eb84b4075602e50533b
> Author: Olof Johansson <olof at lixom.net>
> Date:   Thu Dec 10 11:15:55 2009 -0600
> 
>      ASoC: Fix build of OMAP sound drivers
> 
>      ....
>      Reported-by: Anand Gadiyar <gadiyar at ti.com>
>      Signed-off-by: Olof Johansson <olof at lixom.net>
>      Acked-by: Liam Girdwood <lrg at slimlogic.co.uk>
>      Signed-off-by: Mark Brown <broonie at opensource.wolfsonmicro.com>
> 
> Where's your Signed-off-by: line? You rely on the SCM system to obtain 
> this information from the 'Merge' commit. I don't think that it's good.

This is fully normal.  Do you see sign-off in each pull by Linus?
Many trees with sub-trees or sub-projects are done in that way.
See x86 tree, for example.


thanks,

Takashi