RE: [PATCH v2] ASoC: rt1320-sdw: Update the driver to support rt1321_va2

[Jack Yu <jack.yu@realtek.com>]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Sunday, November 8, 2026 12:56 AM
> To: Jack Yu <jack.yu@realtek.com>
> Cc: broonie@kernel.org; lgirdwood@gmail.com; alsa-devel@alsa-project.org;
> lars@metafoo.de; Flove(HsinFu) <flove@realtek.com>; Oder Chiou
> <oder_chiou@realtek.com>; Shuming [范書銘] <shumingf@realtek.com>;
> Derek [方德義] <derek.fang@realtek.com>
> Subject: Re: [PATCH v2] ASoC: rt1320-sdw: Update the driver to support
> rt1321_va2
> 
> 
> External mail : This email originated from outside the organization. Do not
> reply, click links, or open attachments unless you recognize the sender and
> know the content is safe.
> 
> 
> 
> > RT1321 VA2 needs a different flow from VA0/VA1:
> 
> > @@ -1291,6 +1307,12 @@ static int rt1320_check_fw_ready(struct
> rt1320_sdw_priv *rt1320)
> >               regmap_read(rt1320->regmap, cmd_addr, &tmp);
> >               if (tmp == 0)
> >                       break;
> > +             else if (tmp != 0x1 && tmp != 0x2 && tmp != 0x3 && is_va2)
> {
> > +                     dev_warn(dev, "%s abnormal val, tmp:%x",
> __func__, tmp);
> > +                     for (i = 0; i < 8; i++)
> > +                             regmap_write(rt1320->regmap,
> RT1321_FW_PARAM_ADDR + i, 0x00);
> > +                     break;
> 
> This will break out of the retry loop on error which looks like a success.

It's not an error path. 0x3fc2d301 is the handshake between the driver
and the FW, and the FW only ever writes 0x0~0x3 to it. On VA2 a spurious
value can show up there, and the FW team's recommendation is to clear
the command area, which brings the handshake back to idle so the flow
can carry on. In v3 it returns 0 explicitly, instead of breaking out of the loop.

> 
> > +_restore_:
> > +     if (is_va2) {
> > +             regmap_update_bits(rt1320->regmap, 0xc044, 0xe0, 0xe0);
> > +             regmap_update_bits(rt1320->regmap, 0xc047, 0x80, 0x80);
> > +             regmap_update_bits(rt1320->regmap, 0xd53b, 0x40, 0x40);
> > +             regmap_write(rt1320->regmap, 0xc570, 0x08);
> > +             regmap_write(rt1320->regmap, 0xc5fb, reg_c5fb);
> > +             regmap_write(rt1320->regmap, 0xc5c4, 0x10);
> 
> rt1320_calibrate() saves and restores 0xc570.

Fixed, rt1320_set_advancemode() now saves and restores 0xc570 as well.

> 
> > @@ -2088,6 +2181,7 @@ static int rt1320_rae_load(struct rt1320_sdw_priv
> *rt1320)
> >               regmap_update_bits(rt1320->regmap, 0x2000301c, 0x01,
> 0x01);
> >               /* Phase sync enable */
> >               regmap_update_bits(rt1320->regmap, 0xc047, 0x80,
> 0x80);
> > +             regmap_update_bits(rt1320->regmap, 0xd53b, 0x40, 0x40);
> 
> This is also updating the handling for VA0 and VA1.

Fixed, the 0xd53b update is limited to VA2 now.
> 
> >
> > +static void rt1320_companion_mode_patch(struct rt1320_sdw_priv
> *rt1320,
> > +     bool patch_ver)
> > +{
> > +     struct device *dev = &rt1320->sdw_slave->dev;
> > +     bool comp_mode_en = false;
> > +     unsigned int val;
> > +
> > +     if (patch_ver) {
> 
> This could just be a return if !patch_ver.

Done.
> 
> > +                     if (hdr_size && ptr_img->addr == 0x3fc2a300) {
> > +                             dev_dbg(dev, "%s, AFX1
> hdr[0x20]=0x%08x (%*ph)\n", __func__,
> > +                                     get_unaligned_le32(fw_data
> +
> > + 0x20), 4, fw_data + 0x20);
> > +
> > +                             rt1320->l_adv_gain = fw_data[0x20] |
> (fw_data[0x21] << 8);
> > +                             rt1320->r_adv_gain = fw_data[0x22] |
> > + (fw_data[0x23] << 8);
> 
> There are various RT1320_AFX_HDR_ macros added but not used...

They're used now.

> 
> > @@ -2590,6 +2733,12 @@ static int rt1320_pde23_event(struct
> snd_soc_dapm_widget *w,
> >       unsigned char ps0 = 0x0, ps3 = 0x3;
> >
> >       switch (event) {
> > +     case SND_SOC_DAPM_PRE_PMU:
> > +             if (rt1320->dev_id == RT1321_DEV_ID &&
> rt1320->version_id == RT1321_VA2) {
> > +                     regmap_write(rt1320->regmap, 0xc04c, 0xe2);
> 
> rt1320_companion_mode_patch() writes 0x54 when companion mode is
> enabled.
> 
The companion mode state is kept now, and the PDE23 PRE_PMU
handler writes the value matching the mode.


> > @@ -2962,7 +3111,7 @@ static int rt1320_r0_load_mode_put(struct
> snd_kcontrol *kcontrol,
> >               ucontrol->value.integer.value[1] == 0)
> >               return -EINVAL;
> >
> > -     ret = pm_runtime_resume(component->dev);
> > +     ret = pm_runtime_resume_and_get(component->dev);
> >       if (ret < 0 && ret != -EACCES)
> >               return ret;
> 
> With pm_runtime_resume_and_get() we can't accept -EACCES since the
> runtime PM reference is dropped.  Same in a couple of other cases.
> 
Fixed.

> --
> Mark Brown <broonie@kernel.org>