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

[Takashi Iwai]

On Tue, 25 Aug 2015 13:47:23 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Aug 25 2015 14:24, Takashi Iwai wrote:
> > 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.
> 
> I agree with it, of course. If possible, I'd like to post small changes 
> because a large patchset really exhausts me.
> 
> But this patchset is for new device drivers, sigh. The changes are just 
> for devices supported by this patchset, and have no advantages to the 
> other devices already supported by the other drivers. What is even 
> worse, it can bring disadvantages or overhead to current stack. 
> Therefore, it's meaning less to apply a part of the patchset.

Well, if the patch makes the existing things significant worse from
the performance POV, it's anyway not acceptable.  So, it's invalid as
a counter argument to split patchsets.

If the whole patchset is designed to implement one thing, then it's
unavoidable to have them as a single patchset.  But your patchset
looks more than that -- it starts from a rename patch, restructuring
patches, then patches for digi00x and tascam, at least.  (Also there
are PCM support, MIDI support, etc, which can be separated too.)


> > - __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.
> 
> I always have several nights for self-reviewing, nevertheless patchset 
> includes such a trivial mistakes... (This is one of reasons that I don't 
> like to post such a large patchset.)

The same applied to review -- errors can be overlooked more in a
larger patchset.

> For all that, the last part of this patchset is a bot worse. I concide 
> it, sorry. MIDI functionality of TASCAM FireWire series is enough 
> complicated to me and I always have a headache at considering about is 
> (then make some easy 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?
> 
> I think you mention about the codes in data block processing, such as 
> read_pcm_s32() in sound/firewire/amdtp-am824.c (originally in amdtp.c). 
> These functions are called as frequently as 8,000 times per second, thus 
> it's preferrable to keep the size as small as possible for CPU usage.
> 
> While, each data block processing includes quite similar codes. In this 
> meaning, your idea is valid, indeed. But currently, I adhere to the way 
> to implement different data block processing as what the shape is. 
> There's no public specification about it and I prefer to keep the codes 
> as easy to read as possible.

Well, if you provide simple ops like write_s32(), read_s32(),
silence_s32(), etc, all codes can be same.  The inner loop calls one
of this ops, which returns a BE32 value or convert to a native value,
etc.  The performance cost by such a function call isn't too tragic.


thanks,

Takashi