Re: [PATCH 2/4] ASoC: codecs: Add support for Richtek rt9123

[ChiYuan Huang]

On Fri, Apr 04, 2025 at 04:03:57PM +0100, Mark Brown wrote:
> On Fri, Apr 04, 2025 at 10:22:12PM +0800, cy_huang@xxxxxxxxxxx wrote:
> 
> > +static int rt9123_enable_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol,
> > +			       int event)
> > +{
> 
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	snd_soc_component_write_field(comp, RT9123_REG_AMPCTRL, RT9123_MASK_AMPON, enable);
> > +
> > +	pm_runtime_mark_last_busy(dev);
> > +	pm_runtime_put_autosuspend(dev);
> 
> What's going on with the runtime PM stuff here?  Especially for the DAPM
> widget usually the ASoC core will be able to keep devices runtime PM
> enabled so long as they are in use so I'd expect this not to have any
> impact.  Why not just use a normal DAPM widget?
> 
That's because The RG 0x01 'RT9123_REG_AMPCTRL' is mixed with other volatile
status bitfield like as 'SW_RST', 'SYS_STATE'. That's why I use pm_runtime to
make sure the RG can really be accessed at that time. Actually, the
mixed RG bitfield  for 'RW' and 'RO' is a bad design.
> > +static int rt9123_xhandler_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
> > +	struct device *dev = comp->dev;
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (rt9123_kcontrol_name_comp(kcontrol, "SPK Gain Volume") == 0)
> > +		ret = snd_soc_get_volsw(kcontrol, ucontrol);
> > +	else
> > +		ret = snd_soc_get_enum_double(kcontrol, ucontrol);
> 
> This is even more unusual - it'll runtime PM enable the device every
> time we write to a control, even if the device is idle.  The driver does
> implement a register cache so it's especially confusing, we'll power up
> the device, resync the cache, write to the hardware then power the
> device off again.  Usually you'd just use the standard operations and
> then let the register writes get synced to the cache whenever it gets
> enabled for actual use.  Again, why not just use standard controls?
> 
Same as the last one.

........

Others will be modified in v2.

Thanks.