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

[Krzysztof Kozlowski]

On 11/06/2023 13:57, Mark Brown wrote:
> 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.

Sure, makes sense, I'll drop all of the above.

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

Just to be clear - I don't have access to datasheet so I don't know what
are these. The downstream driver actually initializes (writes to) two
OTP registers - 38 and 40.

> 
>> +static bool wsa884x_readonly_register(struct device *dev, unsigned int reg)
>> +{
>> +	switch (reg) {
> 
> In general the read only registers probably shouldn't have defaults...

For the case when regmap is being read before device enumerates (thus
synced)?

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

Right.

> 
>> +static struct regmap_config wsa884x_regmap_config = {
>> +	.reg_bits = 32,
>> +	.val_bits = 8,
>> +	.cache_type = REGCACHE_RBTREE,
> 
> Please use REGCACHE_MAPLE for new devices.

Ack

> 
>> +	/* 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?

Sincerely, I don't know. Without any documentation, I am relying
entirely on downstream driver which has some code like this. I don't
know whether some parts here make sense only for downstream case or
shall be applicable also for upstream.

Best regards,
Krzysztof