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
