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

[Mark Brown]

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?

Attachment: signature.asc
Description: PGP signature