linux-kernel - Re: [PATCH v3 4/5] ASoC: codecs: Add WCD939x Soundwire devices driver

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] [day] [month] [year] [list]

Message-ID: <d41d4cc3-81eb-431c-b158-673b7ef92727@linaro.org>
Date: Fri, 15 Dec 2023 14:19:50 +0100
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Mark Brown <broonie@...nel.org>
Cc: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
 Banajit Goswami <bgoswami@...cinc.com>, Andy Gross <agross@...nel.org>,
 Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konrad.dybcio@...aro.org>, Liam Girdwood
 <lgirdwood@...il.com>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
 Takashi Iwai <tiwai@...e.com>, linux-arm-msm@...r.kernel.org,
 alsa-devel@...a-project.org, linux-sound@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/5] ASoC: codecs: Add WCD939x Soundwire devices driver

On 13/12/2023 19:31, Mark Brown wrote:
> On Thu, Dec 07, 2023 at 11:28:07AM +0100, Neil Armstrong wrote:
>> Add Soundwire Slave driver for the WCD9390/WCD9395 Audio Codec.
>>
>> The WCD9390/WCD9395 Soundwire devices will be used by the
>> main WCD9390/WCD9395 Audio Codec driver to access registers
>> and configure Soundwire RX and TX ports.
> 
>> +static const struct reg_default wcd939x_defaults[] = {
> 
>> +	{ WCD939X_DIGITAL_MODE_STATUS_0, 0x00 },
>> +	{ WCD939X_DIGITAL_MODE_STATUS_1, 0x00 },
> 
> There's a bunch of registers like this which look like they should be
> volatile and are actually volatile which makes supplying defaults rather
> strange - in general volatile registers shouldn't have defaults.

Indeed I'll clean those up

> 
>> +	{ WCD939X_DIGITAL_EFUSE_REG_0, 0x00 },
>> +	{ WCD939X_DIGITAL_EFUSE_REG_1, 0xff },
>> +	{ WCD939X_DIGITAL_EFUSE_REG_2, 0xff },
> 
> With the fuse registers even though I'd expect them to be cachable the
> whole point is usually that these are programmable per device and
> therefore I'd not expect defaults, I'd expect them to be cached on first
> use.

Ack

> 
>> +static bool wcd939x_readonly_register(struct device *dev, unsigned int reg)
>> +{
> 
>> +	case WCD939X_DIGITAL_CHIP_ID0:
>> +	case WCD939X_DIGITAL_CHIP_ID1:
>> +	case WCD939X_DIGITAL_CHIP_ID2:
>> +	case WCD939X_DIGITAL_CHIP_ID3:
> 
>> +	case WCD939X_DIGITAL_EFUSE_REG_0:
>> +	case WCD939X_DIGITAL_EFUSE_REG_1:
>> +	case WCD939X_DIGITAL_EFUSE_REG_2:
> 
>> +	/* Consider all readonly registers as volatile */
>> +	.volatile_reg = wcd939x_readonly_register,
> 
> There's a bunch of the readonly registers that I'd expect to be cachable
> at runtime - I *hope* the chip ID doesn't change at runtime!  OTOH it
> likely doesn't matter so perhaps it's fine but the comment could use
> some improvement.


I'll improve this

> 
>> +static int wcd939x_sdw_component_bind(struct device *dev, struct device *master,
>> +				      void *data)
>> +{
>> +	/* Bind is required by component framework */
>> +	return 0;
>> +}
>> +
>> +static void wcd939x_sdw_component_unbind(struct device *dev,
>> +					 struct device *master, void *data)
>> +{
>> +	/* Unbind is required by component framework */
>> +}
>> +
>> +static const struct component_ops wcd939x_sdw_component_ops = {
>> +	.bind = wcd939x_sdw_component_bind,
>> +	.unbind = wcd939x_sdw_component_unbind,
>> +};
> 
> So what exactly is the component framework *doing* here then?  It really
> would be better to get this fixed in the component framework if this is
> a sensible usage.

So the component framework is here to synchronize probes of the main codec
and soundwire devices, because the main codec needs the soundwire devices
to access registers.
I assume this design was chosen to limit probe defer infinite loops waiting
for the soundwire devices to probe

I'll propose a change on the component framework, without any insurance it
would be accepted.

> 
>> +static int __maybe_unused wcd939x_sdw_runtime_resume(struct device *dev)
>> +{
>> +	struct wcd939x_sdw_priv *wcd = dev_get_drvdata(dev);
>> +
>> +	if (wcd->regmap) {
>> +		regcache_cache_only(wcd->regmap, false);
>> +		regcache_sync(wcd->regmap);
>> +	}
>> +
>> +	pm_runtime_mark_last_busy(dev);
> 
> The pm_runtime_mark_last_busy() in the resume function is a bit of a
> weird pattern - usually this is something that the user updates and more
> normally when releasing a runtime PM reference.

I took this from wcd938x_sd, I'll check the rationale of it in the resume function.

Thanks,
Neil