[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87zfq9oa7t.wl-tiwai@suse.de>
Date: Mon, 22 Jul 2024 14:57:42 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Nick Weihs <nick.weihs@...il.com>
Cc: tiwai@...e.com,
lsa-devel@...a-project.org,
linux-sound@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ALSA: hda/realtek: Implement sound init sequence for Samsung Galaxy Book3 Pro 360
On Mon, 22 Jul 2024 02:10:59 +0200,
Nick Weihs wrote:
>
> Samsung Galaxy Book3 Pro 360 sends a large amount of data to the codec
> through hda processing coefficients. This data was captured using a
> modified version of QEMU, but the actual content of the data remains
> opaque to me. Elliding any part of the data seems to cause sound to
> not work.
>
> Signed-off-by: Nick Weihs <nick.weihs@...il.com>
Thanks for the patch. However, the patch isn't cleanly applicable as
your mailer seems breaking spaces. Please try to fix the mailer setup
(at best use git-send-email) or use attachment as the last resort.
About the code change:
> +static inline void alc298_samsung_write_coef_pack2(struct hda_codec *codec,
> + const unsigned short coefs[4])
Passing the fixed size array as an argument is hard to read and
error-prone. Better to define a struct instead?
> +{
> + int i;
> +
> + for (i = 0; i < 100; i++) {
> + if ((alc_read_coef_idx(codec, 0x26) & 0x0010) == 0)
> + break;
> + }
This loop looks unreliable. Usually this kind of loop should have
some delay and/or timeout as the break out condition.
> +static void alc298_fixup_samsung_amp2(struct hda_codec *codec,
> + const struct hda_fixup *fix, int action)
> +{
> + int i;
> + static const struct alc298_samsung_coeff_fixup_desc fixups1[] = {
> + { 0x99, 0x8000 }, { 0x82, 0x4408 }, { 0x32, 0x3f00 }, { 0x0e, 0x6f80 },
> + { 0x10, 0x0e21 }, { 0x55, 0x8000 }, { 0x08, 0x2fcf }, { 0x08, 0x2fcf },
> + { 0x2d, 0xc020 }, { 0x19, 0x0017 }, { 0x50, 0x1000 }, { 0x0e, 0x6f80 },
> + { 0x08, 0x2fcf }, { 0x80, 0x0011 }, { 0x2b, 0x0c10 }, { 0x2d, 0xc020 },
> + { 0x03, 0x0042 }, { 0x0f, 0x0062 }, { 0x08, 0x2fcf },
> + };
> +
> + static const unsigned short amp_0x38[][4] = {
> + { 0x2000, 0x0000, 0x0001, 0xB011 }, { 0x23FF, 0x0000, 0x0000, 0xB011 },
> + { 0x203A, 0x0000, 0x0080, 0xB011 }, { 0x23E1, 0x0000, 0x0000, 0xB011 },
(snip)
Such a bulk of data is usually put into another file, and included
from patch_realtek.c. Otherwise the code becomes too clumsy.
See sound/pci/hda/*_helper.c files as examples.
Also, we prefer the lower alphabet for hex numbers.
> + ///// First set of fixups
Too many slashes. Just use '//'.
Last but not least, try to fix coding-style issues reported by
scripts/checkpatch.pl.
thanks,
Takashi
Powered by blists - more mailing lists