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