Re: [PATCH] ALSA: hda/realtek: Implement sound init sequence for Samsung Galaxy Book3 Pro 360

[Takashi Iwai <tiwai@xxxxxxx>]

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@xxxxxxxxx>

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