linux-kernel - Re: [PATCH v2 2/2] ASoC: codecs: wsa884x: Add WSA884x family of speakers

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: <191859d3-42e3-4ef2-87ff-dd56864103f9@sirena.org.uk>
Date:   Sun, 11 Jun 2023 12:57:10 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...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>, alsa-devel@...a-project.org,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Patrick Lai <quic_plai@...cinc.com>
Subject: Re: [PATCH v2 2/2] ASoC: codecs: wsa884x: Add WSA884x family of
 speakers

On Sun, Jun 11, 2023 at 12:26:57PM +0200, Krzysztof Kozlowski wrote:

> +static struct reg_default wsa884x_defaults[] = {

> +	{ WSA884X_CHIP_ID0,			0x00 },
> +	{ WSA884X_CHIP_ID1,			0x00 },
> +	{ WSA884X_CHIP_ID2,			0x04 },
> +	{ WSA884X_CHIP_ID3,			0x02 },
> +	{ WSA884X_BUS_ID,			0x00 },

It is generally bad practice to provide defaults for ID registers since
it rather defeats the point of having them.

> +	{ WSA884X_INTR_STATUS0,			0x00 },
> +	{ WSA884X_INTR_STATUS1,			0x00 },

Interrupt status registers will be volatile and therefore should not
have defaults.

> +	{ WSA884X_OTP_REG_0,			0x05 },
> +	{ WSA884X_OTP_REG_1,			0x49 },
> +	{ WSA884X_OTP_REG_2,			0x80 },
> +	{ WSA884X_OTP_REG_3,			0xc9 },
> +	{ WSA884X_OTP_REG_4,			0x40 },
> +	{ WSA884X_OTP_REG_5,			0xff },
> +	{ WSA884X_OTP_REG_6,			0xff },
> +	{ WSA884X_OTP_REG_7,			0xff },
> +	{ WSA884X_OTP_REG_8,			0xff },
> +	{ WSA884X_OTP_REG_9,			0xff },
> +	{ WSA884X_OTP_REG_10,			0xff },
> +	{ WSA884X_OTP_REG_11,			0xff },
> +	{ WSA884X_OTP_REG_12,			0xff },
> +	{ WSA884X_OTP_REG_13,			0xff },
> +	{ WSA884X_OTP_REG_14,			0xff },
> +	{ WSA884X_OTP_REG_15,			0xff },
> +	{ WSA884X_OTP_REG_16,			0xff },
> +	{ WSA884X_OTP_REG_17,			0xff },
> +	{ WSA884X_OTP_REG_18,			0xff },
> +	{ WSA884X_OTP_REG_19,			0xff },
> +	{ WSA884X_OTP_REG_20,			0xff },
> +	{ WSA884X_OTP_REG_21,			0xff },
> +	{ WSA884X_OTP_REG_22,			0xff },
> +	{ WSA884X_OTP_REG_23,			0xff },
> +	{ WSA884X_OTP_REG_24,			0x00 },
> +	{ WSA884X_OTP_REG_25,			0x22 },
> +	{ WSA884X_OTP_REG_26,			0x03 },
> +	{ WSA884X_OTP_REG_27,			0x00 },
> +	{ WSA884X_OTP_REG_28,			0x00 },
> +	{ WSA884X_OTP_REG_29,			0x00 },
> +	{ WSA884X_OTP_REG_30,			0x00 },
> +	{ WSA884X_OTP_REG_31,			0x8f },
> +	{ WSA884X_OTP_REG_32,			0x00 },
> +	{ WSA884X_OTP_REG_33,			0xff },
> +	{ WSA884X_OTP_REG_34,			0x0f },
> +	{ WSA884X_OTP_REG_35,			0x12 },
> +	{ WSA884X_OTP_REG_36,			0x08 },
> +	{ WSA884X_OTP_REG_37,			0x1f },
> +	{ WSA884X_OTP_REG_38,			0x0b },
> +	{ WSA884X_OTP_REG_39,			0x00 },
> +	{ WSA884X_OTP_REG_40,			0x00 },
> +	{ WSA884X_OTP_REG_41,			0x00 },
> +	{ WSA884X_OTP_REG_63,			0x40 },

These appear to be OTP data which suggests that they shouldn't have
defaults either since they can be programmed.

> +static bool wsa884x_readonly_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {

In general the read only registers probably shouldn't have defaults...

> +static bool wsa884x_volatile_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case WSA884X_ANA_WO_CTL_0:
> +	case WSA884X_ANA_WO_CTL_1:
> +		return true;
> +	}
> +	return wsa884x_readonly_register(dev, reg);
> +}

...and the volatile regiseters definitely not, the default values will
never be used and just waste space.

> +static struct regmap_config wsa884x_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_RBTREE,

Please use REGCACHE_MAPLE for new devices.

> +	/* Speaker mode by default */
> +	{ WSA884X_DRE_CTL_0, 0x7 << WSA884X_DRE_CTL_0_PROG_DELAY_SHIFT },
> +	{ WSA884X_CLSH_CTL_0, (0x37 & ~WSA884X_CLSH_CTL_0_DLY_CODE_MASK) |
> +			      (0x6 << WSA884X_CLSH_CTL_0_DLY_CODE_SHIFT) },
> +	{ WSA884X_CLSH_SOFT_MAX, 0xff },

Why not just leave as the chip default?

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