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