[alsa-devel] [PATCH 00/25 v2] ALSA: support AMDTP variants

[Takashi Iwai]

On Mon, 24 Aug 2015 08:26:56 +0200,
Takashi Iwai wrote:
> 
> On Sun, 23 Aug 2015 16:58:44 +0200,
> Takashi Sakamoto wrote:
> > 
> > On 2015年08月23日 17:04, Takashi Iwai wrote:
> > > On Sat, 22 Aug 2015 11:19:16 +0200,
> > > Takashi Sakamoto wrote:
> > >>
> > >> This patchset for Linux 4.3 updates my previous one:
> > > 
> > > Sorry this seems too late for 4.3.  If this got reviewed and acked by
> > > Clemens or other, I would take it.  But otherwise it's too intrusive
> > > to take in the last day of the development cycle.
> > > 
> > > 
> > > Takashi
> > 
> > OK.
> 
> Now 4.2-rc8 was released unexpectedly, so we have a chance to review
> for one more week.  Let's see.

So I started a quick review from the bottom, and as already posted,
there are obvious errors, and I stopped reading at that point.

Here are a few suggestions:

- In general, the series is way too big and contains a wide variety of
  changes.  Better to split to smaller patch series.  This makes much
  easier to review, and more importantly to merge.  Otherwise a
  failure in one patch would block the whole series.

- __u32 or such is basically for API exposed to user space.  In
  kernel, better to use u32 instead.

- When you write a new code, consider using EXPORT_SYMBOL_GPL()
  instead of EXPORT_SYMBOL().

- Re-read each patch once before submission as if you were a reviewer;
  this often helps to catch the typos or simple mistakes.

Also, I see quite a few code duplications in the series.  For example,
PCM read/write/silence loop is almost identical except for a few lines
of differences inside the loop.  Ditto for MIDI accessors.  Can they
be shared somehow better?


thanks,

Takashi