| Message ID | 20230611102657.74714-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] ASoC: dt-bindings: qcom,wsa8840: Add WSA884x family of speakers | expand |
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
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.
diff --git a/Documentation/devicetree/bindings/sound/qcom,wsa8840.yaml b/Documentation/devicetree/bindings/sound/qcom,wsa8840.yaml new file mode 100644 index 000000000000..e6723c9e312a --- /dev/null +++ b/Documentation/devicetree/bindings/sound/qcom,wsa8840.yaml @@ -0,0 +1,66 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/qcom,wsa8840.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm WSA8840/WSA8845/WSA8845H smart speaker amplifier + +maintainers: + - Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> + - Srinivas Kandagatla <srinivas.kandagatla@linaro.org> + +description: + WSA884X is a family of Qualcomm Aqstic smart speaker amplifiers using + SoundWire digital audio interface. + +allOf: + - $ref: dai-common.yaml# + +properties: + compatible: + const: sdw20217020400 + + reg: + maxItems: 1 + + powerdown-gpios: + description: Powerdown/Shutdown line to use (pin SD_N) + maxItems: 1 + + '#sound-dai-cells': + const: 0 + + vdd-1p8-supply: true + vdd-io-supply: true + +required: + - compatible + - reg + - powerdown-gpios + - '#sound-dai-cells' + - vdd-1p8-supply + - vdd-io-supply + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + soundwire-controller { + #address-cells = <2>; + #size-cells = <0>; + + speaker@0,1 { + compatible = "sdw20217020400"; + reg = <0 1>; + pinctrl-names = "default"; + pinctrl-0 = <&spkr_2_sd_n_active>; + powerdown-gpios = <&lpass_tlmm 18 GPIO_ACTIVE_LOW>; + #sound-dai-cells = <0>; + sound-name-prefix = "SpkrRight"; + vdd-1p8-supply = <&vreg_l15b_1p8>; + vdd-io-supply = <&vreg_l3g_1p2>; + }; + };
Add binding for WSA8840/WSA8845/WSA8845H smart speaker amplifiers used in Qualcomm QRD8550 board with SM8550 SoC. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- Changes in v2: 1. Correct compatible (sdw version 1 -> 2). Cc: Patrick Lai <quic_plai@quicinc.com> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- .../bindings/sound/qcom,wsa8840.yaml | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/qcom,wsa8840.yaml