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

Message-ID: <e8b5099c-ceb2-4605-94bc-efd09ad55cb7@sirena.org.uk>
Date:   Wed, 13 Dec 2023 18:31:56 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Neil Armstrong <neil.armstrong@...aro.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 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.

> +	{ 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.

> +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.

> +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.

> +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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)