Re: [PATCH 00/25 v2] ALSA: support AMDTP variants

From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org, clemens@ladisch.de,
	ffado-devel@lists.sf.net
Subject: Re: [PATCH 00/25 v2] ALSA: support AMDTP variants
Date: Tue, 25 Aug 2015 20:47:23 +0900	[thread overview]
Message-ID: <55DC55CB.1020207@sakamocchi.jp> (raw)
In-Reply-To: <s5hoahw3pqf.wl-tiwai@suse.de>

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.

> - __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.)

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.

Thanks

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel