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
