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

[Mark Brown]

On Mon, Jun 12, 2023 at 09:25:44AM +0200, Krzysztof Kozlowski wrote:
> On 11/06/2023 13:57, Mark Brown wrote:
> > On Sun, Jun 11, 2023 at 12:26:57PM +0200, Krzysztof Kozlowski wrote:

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

That's...  counterintuitive given the naming.

> >> +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)?

If you're reading read only registers from the cache defaults like that
you may as well cut out the middle man and not bother parsing the
register at all - the value is just hard coded.  Either power up the
device to find out what the values are or use the values directly.

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

This sounds like someone hard coding their use case TBH.

Attachment: signature.asc
Description: PGP signature