Re: [PATCH RFC 1/1] ASoC: rt1320: Use the BIOS init values on LG gram 16Z95U

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: rt1320: missing PDE23 power state entries in
  rt1320_vc_preset()
- [Low] ASoC: rt1320: duplicate board init table in rt1320_vc_preset()
--

commit e08ad88a1a372908c1aadce1ece061c49a996626
Author: JaeHoon Kim <jaehoon1744@dankook.ac.kr>

ASoC: rt1320: Use the BIOS init values on LG gram 16Z95U

This commit adds a DMI-matched register initialization sequence for the
RT1320 amplifier on the LG gram 16 16Z95U based on its BIOS SDCA function
initialization table.

> diff --git a/sound/soc/codecs/rt1320-sdw.c b/sound/soc/codecs/rt1320-sdw.c
> index fa89db0966514..b1d6ab8dfa45c 100644
> --- a/sound/soc/codecs/rt1320-sdw.c
> +++ b/sound/soc/codecs/rt1320-sdw.c
> @@ -2339,15 +2339,221 @@ static void rt1320_load_dspfw_work(struct work_struct *work)
>  	rt1320_dspfw_load_code(rt1320);
>  }
>  
> +/*
> + * LG gram 16 16Z95U: the BIOS provides a board specific SDCA function
> + * initialization table for the RT1320 SmartAmp (SW23 AF04). This is that
> + * table without its bulk RAM/patch areas (0xe000-0xe7ff, 0x10007000-,
> + * 0x1000d000-, 0x2000xxxx); the MCU patch is still loaded from
> + * linux-firmware at 0x3fc2bf83 as in rt1320_vc_blind_write[]. Registers
> + * that the table does not touch but rt1320_vc_blind_write[] does are kept
> + * and marked "drv".
> + */
> +static const struct reg_sequence rt1320_vc_lg_16z95u_blind_write[] = {

[Severity: Low]
This isn't a bug, but as the commit message notes that this patch is not
intended for merging as is, duplicating the 175-entry preset table for a
single DMI match copies over 120 identical writes from
rt1320_vc_blind_write[].  Applying only the board-specific register
overrides on top of rt1320_vc_blind_write[], or using the SDCA
mipi-sdca-function-initialization-table infrastructure in
find_sdca_init_table(), would avoid duplicating the base sequence.

Also, is the commit message accurate that 0x1000db15..0x1000db23 swaps the
left and right mapping compared to rt1320_vc_blind_write[]?  0x1000db00 is
an entry count (0x07) followed by seven 5-byte (32-bit little-endian
register, 8-bit value) entries, and 0x1000db15..0x1000db23 encodes the same
three register writes (0x010d = 0xbf, 0x0913 = 0x00, 0x0912 = 0x00) as
rt1320_vc_blind_write[], only in reverse order.

[ ... ]

> +	{ 0x1000cc46, 0x00 },
> +	{ 0xd486, 0x43 },
> +	{ 0x1000db00, 0x07 },

[ ... ]

> +	{ 0xc5f7, 0x22 },	/* drv */
> +	{ 0xc5f6, 0x22 },	/* drv */
> +	{ 0xd172, 0x2a },	/* drv */
> +	{ 0xc5d6, 0x01 },	/* drv */
> +};

[Severity: Medium]
Should rt1320_vc_lg_16z95u_blind_write[] keep the two
SDW_SDCA_CTL(FUNC_NUM_AMP, RT1320_SDCA_ENT_PDE23,
RT1320_SDCA_CTL_REQ_POWER_STATE, 0) entries from rt1320_vc_blind_write[]?

In rt1320_vc_blind_write[], releasing the MCU reset at 0xd486 = 0x43 is
followed by a PDE23 PS0 write (0x00), and the table ends with a PDE23 PS3
write (0x03):

sound/soc/codecs/rt1320-sdw.c:rt1320_vc_blind_write[] {
    ...
	{ 0x1000cc46, 0x00 },
	{ 0x0000d486, 0x43 },
	{ SDW_SDCA_CTL(FUNC_NUM_AMP, RT1320_SDCA_ENT_PDE23, RT1320_SDCA_CTL_REQ_POWER_STATE, 0), 0x00 },
	{ 0x1000db00, 0x07 },
    ...
	{ 0xd172, 0x2a },
	{ 0xc5d6, 0x01 },
	{ SDW_SDCA_CTL(FUNC_NUM_AMP, RT1320_SDCA_ENT_PDE23, RT1320_SDCA_CTL_REQ_POWER_STATE, 0), 0x03 },
};

rt1320_vc_preset() keys off those two entries in seq[] to poll
RT1320_KR0_INT_READY until the MCU reaches 0x1f and to wait for PDE23 power
state transitions via rt1320_pde_transition_delay().  Without them,
rt1320_vc_preset() writes 0x1000db00..0x1000db23 and triggers 0xd540 = 0x21
without waiting for the MCU to finish booting after reset release, and
skips both PDE23 power state transitions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112040143.6945-1-jaehoon1744@gmail.com?part=1