Re: [PATCH 2/2] ALSA: emu10k1: use high-level I/O in set_filterQ()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Apr 23, 2023 at 09:25:39AM +0200, Takashi Iwai wrote:
Again, you must have a bit more say here...
For example, you didn't write why this change is needed.
You thought it obvious?  No, readers don't know.

it is obvious from the patch - the code becomes much shorter and more legible. and someone who just reads the log/blame wouldn't care, because it doesn't actually explain anything. but whatever.

On Sun, Apr 23, 2023 at 09:35:46AM +0200, Takashi Iwai wrote:
On Sat, 22 Apr 2023 18:10:20 +0200,
Oswald Buddenhagen wrote:

... and also use more pre-defined constants on the way (some of which
required adjustment).

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@xxxxxx>

Applied this one, but skipped the patch 2.

which is funny, because that commit message misses the obvious "why" part as well - it just mentions an additional thing that is unique to this patch.

so to be consistent, you should reject both patches and wait for an update.

BTW, it would be really better if we define some macro for the
highlevel I/O definition.  It's cumbersome to decode and check
manually at review whether the conversion is correct, and it's
error-prone.

yes, i considered that.
i also considered many more refactorings, and had to hold myself back -
there are enough nice-to-have patches in this series as-is.
i mean, 15 years ago it would have made sense to go crazy, but now the hardware is a bit too obsolete to go much beyond what i actually need for my project. i'm assuming some people outside the western sphere are still using our scrap with linux, but we rarely hear from them, so it's hard to know ...

regards



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux