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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z/Mf1VQ1Ay/Fw3kh@git-send.richtek.com>
Date: Mon, 7 Apr 2025 08:44:05 +0800
From: ChiYuan Huang <cy_huang@...htek.com>
To: Mark Brown <broonie@...nel.org>
CC: Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
	<conor+dt@...nel.org>, Rob Herring <robh@...nel.org>, Liam Girdwood
	<lgirdwood@...il.com>, Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai
	<tiwai@...e.com>, Otto lin <otto_lin@...htek.com>, Allen Lin
	<allen_lin@...htek.com>, <devicetree@...r.kernel.org>,
	<linux-sound@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/4] ASoC: codecs: Add support for Richtek rt9123

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@...htek.com 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.