[PATCH 0/6] ASoC: codecs: msm8916-wcd-analog: Cleanup DT bindings
Drop the redundant reg-names and mclk from the PM8916 analog codec. Having the mclk on the analog codec is incorrect because only the digital codec consumes it directly.
Signed-off-by: Stephan Gerhold stephan@gerhold.net --- Stephan Gerhold (6): ASoC: dt-bindings: pm8916-analog-codec: Fix misleading example ASoC: dt-bindings: pm8916-analog-codec: Drop pointless reg-names ASoC: dt-bindings: pm8916-analog-codec: Drop invalid mclk ASoC: codecs: msm8916-wcd-analog: Drop invalid mclk ASoC: codecs: msm8916-wcd-analog: Properly handle probe errors arm64: dts: qcom: pm8916: Drop codec reg-names and mclk
.../sound/qcom,pm8916-wcd-analog-codec.yaml | 101 ++++++++++----------- arch/arm64/boot/dts/qcom/apq8016-sbc.dts | 2 - arch/arm64/boot/dts/qcom/pm8916.dtsi | 3 - sound/soc/codecs/msm8916-wcd-analog.c | 56 +++--------- 4 files changed, 62 insertions(+), 100 deletions(-) --- base-commit: 78b31c16983bb9e540d5a14540417275e6f3f4a5 change-id: 20230712-pm8916-mclk-dbc17169c598
Best regards,
SPMI devices typically have a single address cell and no size cell, i.e. reg = <0xf000> for the audio codec instead of reg = <0xf000 0x200>.
The example is a bit misleading because it uses the latter. Copying this into the device tree would be incorrect and was fixed before for pm8916.dtsi in commit c2f0cbb57dba ("arm64: dts: qcom: pm8916: Remove invalid reg size from wcd_codec").
Make the example more clear by adding the outer "pmic" node which specifies the same #address/size-cells that would be used in the real deivce tree.
Signed-off-by: Stephan Gerhold stephan@gerhold.net --- .../sound/qcom,pm8916-wcd-analog-codec.yaml | 92 ++++++++++++---------- 1 file changed, 50 insertions(+), 42 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml b/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml index c385028c4296..77e3cfba4746 100644 --- a/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml +++ b/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml @@ -115,46 +115,54 @@ examples: - | #include <dt-bindings/clock/qcom,gcc-msm8916.h> #include <dt-bindings/interrupt-controller/irq.h> - - audio-codec@f000{ - compatible = "qcom,pm8916-wcd-analog-codec"; - reg = <0xf000 0x200>; - reg-names = "pmic-codec-core"; - clocks = <&gcc GCC_CODEC_DIGCODEC_CLK>; - clock-names = "mclk"; - qcom,mbhc-vthreshold-low = <75 150 237 450 500>; - qcom,mbhc-vthreshold-high = <75 150 237 450 500>; - interrupt-parent = <&spmi_bus>; - interrupts = <0x1 0xf0 0x0 IRQ_TYPE_NONE>, - <0x1 0xf0 0x1 IRQ_TYPE_NONE>, - <0x1 0xf0 0x2 IRQ_TYPE_NONE>, - <0x1 0xf0 0x3 IRQ_TYPE_NONE>, - <0x1 0xf0 0x4 IRQ_TYPE_NONE>, - <0x1 0xf0 0x5 IRQ_TYPE_NONE>, - <0x1 0xf0 0x6 IRQ_TYPE_NONE>, - <0x1 0xf0 0x7 IRQ_TYPE_NONE>, - <0x1 0xf1 0x0 IRQ_TYPE_NONE>, - <0x1 0xf1 0x1 IRQ_TYPE_NONE>, - <0x1 0xf1 0x2 IRQ_TYPE_NONE>, - <0x1 0xf1 0x3 IRQ_TYPE_NONE>, - <0x1 0xf1 0x4 IRQ_TYPE_NONE>, - <0x1 0xf1 0x5 IRQ_TYPE_NONE>; - interrupt-names = "cdc_spk_cnp_int", - "cdc_spk_clip_int", - "cdc_spk_ocp_int", - "mbhc_ins_rem_det1", - "mbhc_but_rel_det", - "mbhc_but_press_det", - "mbhc_ins_rem_det", - "mbhc_switch_int", - "cdc_ear_ocp_int", - "cdc_hphr_ocp_int", - "cdc_hphl_ocp_det", - "cdc_ear_cnp_int", - "cdc_hphr_cnp_int", - "cdc_hphl_cnp_int"; - vdd-cdc-io-supply = <&pm8916_l5>; - vdd-cdc-tx-rx-cx-supply = <&pm8916_l5>; - vdd-micbias-supply = <&pm8916_l13>; - #sound-dai-cells = <1>; + #include <dt-bindings/spmi/spmi.h> + + pmic@1 { + compatible = "qcom,pm8916", "qcom,spmi-pmic"; + reg = <0x1 SPMI_USID>; + #address-cells = <1>; + #size-cells = <0>; + + audio-codec@f000 { + compatible = "qcom,pm8916-wcd-analog-codec"; + reg = <0xf000>; + reg-names = "pmic-codec-core"; + clocks = <&gcc GCC_CODEC_DIGCODEC_CLK>; + clock-names = "mclk"; + qcom,mbhc-vthreshold-low = <75 150 237 450 500>; + qcom,mbhc-vthreshold-high = <75 150 237 450 500>; + interrupt-parent = <&spmi_bus>; + interrupts = <0x1 0xf0 0x0 IRQ_TYPE_NONE>, + <0x1 0xf0 0x1 IRQ_TYPE_NONE>, + <0x1 0xf0 0x2 IRQ_TYPE_NONE>, + <0x1 0xf0 0x3 IRQ_TYPE_NONE>, + <0x1 0xf0 0x4 IRQ_TYPE_NONE>, + <0x1 0xf0 0x5 IRQ_TYPE_NONE>, + <0x1 0xf0 0x6 IRQ_TYPE_NONE>, + <0x1 0xf0 0x7 IRQ_TYPE_NONE>, + <0x1 0xf1 0x0 IRQ_TYPE_NONE>, + <0x1 0xf1 0x1 IRQ_TYPE_NONE>, + <0x1 0xf1 0x2 IRQ_TYPE_NONE>, + <0x1 0xf1 0x3 IRQ_TYPE_NONE>, + <0x1 0xf1 0x4 IRQ_TYPE_NONE>, + <0x1 0xf1 0x5 IRQ_TYPE_NONE>; + interrupt-names = "cdc_spk_cnp_int", + "cdc_spk_clip_int", + "cdc_spk_ocp_int", + "mbhc_ins_rem_det1", + "mbhc_but_rel_det", + "mbhc_but_press_det", + "mbhc_ins_rem_det", + "mbhc_switch_int", + "cdc_ear_ocp_int", + "cdc_hphr_ocp_int", + "cdc_hphl_ocp_det", + "cdc_ear_cnp_int", + "cdc_hphr_cnp_int", + "cdc_hphl_cnp_int"; + vdd-cdc-io-supply = <&pm8916_l5>; + vdd-cdc-tx-rx-cx-supply = <&pm8916_l5>; + vdd-micbias-supply = <&pm8916_l13>; + #sound-dai-cells = <1>; + }; };
On Tue, 18 Jul 2023 13:40:13 +0200, Stephan Gerhold wrote:
SPMI devices typically have a single address cell and no size cell, i.e. reg = <0xf000> for the audio codec instead of reg = <0xf000 0x200>.
The example is a bit misleading because it uses the latter. Copying this into the device tree would be incorrect and was fixed before for pm8916.dtsi in commit c2f0cbb57dba ("arm64: dts: qcom: pm8916: Remove invalid reg size from wcd_codec").
Make the example more clear by adding the outer "pmic" node which specifies the same #address/size-cells that would be used in the real deivce tree.
Signed-off-by: Stephan Gerhold stephan@gerhold.net
.../sound/qcom,pm8916-wcd-analog-codec.yaml | 92 ++++++++++++---------- 1 file changed, 50 insertions(+), 42 deletions(-)
Reviewed-by: Rob Herring robh@kernel.org
pm8916-wcd-analog-codec has just a single reg region, so having a name for it provides no additional value.
Drop it completely from the schema and example. There is not really any point in keeping it (even as deprecated) because it was never used. In old DTBs it will just be ignored as before.
Signed-off-by: Stephan Gerhold stephan@gerhold.net --- .../devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml b/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml index 77e3cfba4746..5053799c88b5 100644 --- a/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml +++ b/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml @@ -19,10 +19,6 @@ properties: reg: maxItems: 1
- reg-names: - items: - - const: pmic-codec-core - clocks: maxItems: 1
@@ -126,7 +122,6 @@ examples: audio-codec@f000 { compatible = "qcom,pm8916-wcd-analog-codec"; reg = <0xf000>; - reg-names = "pmic-codec-core"; clocks = <&gcc GCC_CODEC_DIGCODEC_CLK>; clock-names = "mclk"; qcom,mbhc-vthreshold-low = <75 150 237 450 500>;
On Tue, 18 Jul 2023 13:40:14 +0200, Stephan Gerhold wrote:
pm8916-wcd-analog-codec has just a single reg region, so having a name for it provides no additional value.
Drop it completely from the schema and example. There is not really any point in keeping it (even as deprecated) because it was never used. In old DTBs it will just be ignored as before.
Signed-off-by: Stephan Gerhold stephan@gerhold.net
.../devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml | 5 ----- 1 file changed, 5 deletions(-)
Reviewed-by: Rob Herring robh@kernel.org
The audio codec typically used for the MSM8916 SoC is split into two parts: the digital codec is part of the SoC, while the analog codec is part of the PM8916 PMIC.
The analog codec in the PMIC has no direct connection to the mclk of the SoC (GCC_CODEC_DIGCODEC_CLK). As the name of the clock suggests this is supplied to the digital part of the codec. During playback it will use this clock to transmit the audio data via the "CDC PDM" pins to the PMIC. In this case the analog codec indirectly receives the clock signal through the digital codec.
GCC_CODEC_DIGCODEC_CLK is already managed by the driver of the digital part of the codec in the SoC. Having this clock on the analog PMIC part additionally is redundant and incorrect because the analog codec cannot receive the clock signal without going through the digital codec.
Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Stephan Gerhold stephan@gerhold.net --- .../bindings/sound/qcom,pm8916-wcd-analog-codec.yaml | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml b/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml index 5053799c88b5..94e7a1860977 100644 --- a/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml +++ b/Documentation/devicetree/bindings/sound/qcom,pm8916-wcd-analog-codec.yaml @@ -19,13 +19,6 @@ properties: reg: maxItems: 1
- clocks: - maxItems: 1 - - clock-names: - items: - - const: mclk - interrupts: maxItems: 14
@@ -109,7 +102,6 @@ additionalProperties: false
examples: - | - #include <dt-bindings/clock/qcom,gcc-msm8916.h> #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/spmi/spmi.h>
@@ -122,8 +114,6 @@ examples: audio-codec@f000 { compatible = "qcom,pm8916-wcd-analog-codec"; reg = <0xf000>; - clocks = <&gcc GCC_CODEC_DIGCODEC_CLK>; - clock-names = "mclk"; qcom,mbhc-vthreshold-low = <75 150 237 450 500>; qcom,mbhc-vthreshold-high = <75 150 237 450 500>; interrupt-parent = <&spmi_bus>;
On Tue, 18 Jul 2023 13:40:15 +0200, Stephan Gerhold wrote:
The audio codec typically used for the MSM8916 SoC is split into two parts: the digital codec is part of the SoC, while the analog codec is part of the PM8916 PMIC.
The analog codec in the PMIC has no direct connection to the mclk of the SoC (GCC_CODEC_DIGCODEC_CLK). As the name of the clock suggests this is supplied to the digital part of the codec. During playback it will use this clock to transmit the audio data via the "CDC PDM" pins to the PMIC. In this case the analog codec indirectly receives the clock signal through the digital codec.
GCC_CODEC_DIGCODEC_CLK is already managed by the driver of the digital part of the codec in the SoC. Having this clock on the analog PMIC part additionally is redundant and incorrect because the analog codec cannot receive the clock signal without going through the digital codec.
Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Stephan Gerhold stephan@gerhold.net
.../bindings/sound/qcom,pm8916-wcd-analog-codec.yaml | 10 ---------- 1 file changed, 10 deletions(-)
Reviewed-by: Rob Herring robh@kernel.org
The audio codec typically used for the MSM8916 SoC is split into two parts: the digital codec is part of the SoC, while the analog codec is part of the PM8916 PMIC.
The analog codec in the PMIC has no direct connection to the mclk of the SoC (GCC_CODEC_DIGCODEC_CLK). As the name of the clock suggests this is supplied to the digital part of the codec. During playback it will use this clock to transmit the audio data via the "CDC PDM" pins to the PMIC. In this case the analog codec indirectly receives the clock signal through the digital codec.
GCC_CODEC_DIGCODEC_CLK is already managed by the driver of the digital part of the codec in the SoC. Having this clock on the analog PMIC part additionally is redundant and incorrect because the analog codec cannot receive the clock signal without going through the digital codec.
Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Stephan Gerhold stephan@gerhold.net --- sound/soc/codecs/msm8916-wcd-analog.c | 43 +++++------------------------------ 1 file changed, 6 insertions(+), 37 deletions(-)
diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c index cec90cf920ff..d4456a714c97 100644 --- a/sound/soc/codecs/msm8916-wcd-analog.c +++ b/sound/soc/codecs/msm8916-wcd-analog.c @@ -7,7 +7,6 @@ #include <linux/delay.h> #include <linux/regulator/consumer.h> #include <linux/types.h> -#include <linux/clk.h> #include <linux/of.h> #include <linux/platform_device.h> #include <linux/regmap.h> @@ -1198,12 +1197,6 @@ static int pm8916_wcd_analog_spmi_probe(struct platform_device *pdev) if (ret < 0) return ret;
- priv->mclk = devm_clk_get(dev, "mclk"); - if (IS_ERR(priv->mclk)) { - dev_err(dev, "failed to get mclk\n"); - return PTR_ERR(priv->mclk); - } - for (i = 0; i < ARRAY_SIZE(supply_names); i++) priv->supplies[i].supply = supply_names[i];
@@ -1214,17 +1207,9 @@ static int pm8916_wcd_analog_spmi_probe(struct platform_device *pdev) return ret; }
- ret = clk_prepare_enable(priv->mclk); - if (ret < 0) { - dev_err(dev, "failed to enable mclk %d\n", ret); - return ret; - } - irq = platform_get_irq_byname(pdev, "mbhc_switch_int"); - if (irq < 0) { - ret = irq; - goto err_disable_clk; - } + if (irq < 0) + return irq;
ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_mbhc_switch_irq_handler, @@ -1236,10 +1221,8 @@ static int pm8916_wcd_analog_spmi_probe(struct platform_device *pdev)
if (priv->mbhc_btn_enabled) { irq = platform_get_irq_byname(pdev, "mbhc_but_press_det"); - if (irq < 0) { - ret = irq; - goto err_disable_clk; - } + if (irq < 0) + return irq;
ret = devm_request_threaded_irq(dev, irq, NULL, mbhc_btn_press_irq_handler, @@ -1250,10 +1233,8 @@ static int pm8916_wcd_analog_spmi_probe(struct platform_device *pdev) dev_err(dev, "cannot request mbhc button press irq\n");
irq = platform_get_irq_byname(pdev, "mbhc_but_rel_det"); - if (irq < 0) { - ret = irq; - goto err_disable_clk; - } + if (irq < 0) + return irq;
ret = devm_request_threaded_irq(dev, irq, NULL, mbhc_btn_release_irq_handler, @@ -1270,17 +1251,6 @@ static int pm8916_wcd_analog_spmi_probe(struct platform_device *pdev) return devm_snd_soc_register_component(dev, &pm8916_wcd_analog, pm8916_wcd_analog_dai, ARRAY_SIZE(pm8916_wcd_analog_dai)); - -err_disable_clk: - clk_disable_unprepare(priv->mclk); - return ret; -} - -static void pm8916_wcd_analog_spmi_remove(struct platform_device *pdev) -{ - struct pm8916_wcd_analog_priv *priv = dev_get_drvdata(&pdev->dev); - - clk_disable_unprepare(priv->mclk); }
static const struct of_device_id pm8916_wcd_analog_spmi_match_table[] = { @@ -1296,7 +1266,6 @@ static struct platform_driver pm8916_wcd_analog_spmi_driver = { .of_match_table = pm8916_wcd_analog_spmi_match_table, }, .probe = pm8916_wcd_analog_spmi_probe, - .remove_new = pm8916_wcd_analog_spmi_remove, };
module_platform_driver(pm8916_wcd_analog_spmi_driver);
The probe() function fails with an error for platform_get_irq_byname() but only logs when devm_request_threaded_irq() fails. Make this consistent and fail to probe in that case as well. In practice this should never happen unless something is really wrong.
Signed-off-by: Stephan Gerhold stephan@gerhold.net --- sound/soc/codecs/msm8916-wcd-analog.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c index d4456a714c97..9ca381812975 100644 --- a/sound/soc/codecs/msm8916-wcd-analog.c +++ b/sound/soc/codecs/msm8916-wcd-analog.c @@ -1216,8 +1216,10 @@ static int pm8916_wcd_analog_spmi_probe(struct platform_device *pdev) IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, "mbhc switch irq", priv); - if (ret) + if (ret) { dev_err(dev, "cannot request mbhc switch irq\n"); + return ret; + }
if (priv->mbhc_btn_enabled) { irq = platform_get_irq_byname(pdev, "mbhc_but_press_det"); @@ -1229,8 +1231,10 @@ static int pm8916_wcd_analog_spmi_probe(struct platform_device *pdev) IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, "mbhc btn press irq", priv); - if (ret) + if (ret) { dev_err(dev, "cannot request mbhc button press irq\n"); + return ret; + }
irq = platform_get_irq_byname(pdev, "mbhc_but_rel_det"); if (irq < 0) @@ -1241,9 +1245,10 @@ static int pm8916_wcd_analog_spmi_probe(struct platform_device *pdev) IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, "mbhc btn release irq", priv); - if (ret) + if (ret) { dev_err(dev, "cannot request mbhc button release irq\n"); - + return ret; + } }
dev_set_drvdata(dev, priv);
Drop the redundant reg-names and mclk from the PM8916 analog codec that were removed from the DT schema. Having the mclk on the analog codec is incorrect because only the digital codec consumes it directly.
Signed-off-by: Stephan Gerhold stephan@gerhold.net --- arch/arm64/boot/dts/qcom/apq8016-sbc.dts | 2 -- arch/arm64/boot/dts/qcom/pm8916.dtsi | 3 --- 2 files changed, 5 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dts b/arch/arm64/boot/dts/qcom/apq8016-sbc.dts index f3d65a606194..52bf876b8cc9 100644 --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dts +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dts @@ -328,8 +328,6 @@ &mpss {
&pm8916_codec { status = "okay"; - clocks = <&gcc GCC_CODEC_DIGCODEC_CLK>; - clock-names = "mclk"; qcom,mbhc-vthreshold-low = <75 150 237 450 500>; qcom,mbhc-vthreshold-high = <75 150 237 450 500>; }; diff --git a/arch/arm64/boot/dts/qcom/pm8916.dtsi b/arch/arm64/boot/dts/qcom/pm8916.dtsi index 1ea8920ff369..78187c0c94ce 100644 --- a/arch/arm64/boot/dts/qcom/pm8916.dtsi +++ b/arch/arm64/boot/dts/qcom/pm8916.dtsi @@ -142,9 +142,6 @@ pm8916_vib: vibrator@c000 { pm8916_codec: audio-codec@f000 { compatible = "qcom,pm8916-wcd-analog-codec"; reg = <0xf000>; - reg-names = "pmic-codec-core"; - clocks = <&gcc GCC_CODEC_DIGCODEC_CLK>; - clock-names = "mclk"; interrupt-parent = <&spmi_bus>; interrupts = <0x1 0xf0 0x0 IRQ_TYPE_NONE>, <0x1 0xf0 0x1 IRQ_TYPE_NONE>,
On Tue, 18 Jul 2023 13:40:12 +0200, Stephan Gerhold wrote:
Drop the redundant reg-names and mclk from the PM8916 analog codec. Having the mclk on the analog codec is incorrect because only the digital codec consumes it directly.
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/6] ASoC: dt-bindings: pm8916-analog-codec: Fix misleading example commit: 944b5c7146fbd0a68f501d9a8a87c3fc5767a3de [2/6] ASoC: dt-bindings: pm8916-analog-codec: Drop pointless reg-names commit: dfc491e55255a96b2d43cdb74db10d4222890769 [3/6] ASoC: dt-bindings: pm8916-analog-codec: Drop invalid mclk commit: 469c6d9cd1cfb468f01a15f940272504a6b5d083 [4/6] ASoC: codecs: msm8916-wcd-analog: Drop invalid mclk commit: 97f29c1a6143762626f4f9bd9fc2f8a2282b9dcd [5/6] ASoC: codecs: msm8916-wcd-analog: Properly handle probe errors commit: 5c0f9652da47061ed3f7815c1dfeb205c545ce54
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
On Tue, 18 Jul 2023 13:40:12 +0200, Stephan Gerhold wrote:
Drop the redundant reg-names and mclk from the PM8916 analog codec. Having the mclk on the analog codec is incorrect because only the digital codec consumes it directly.
Applied, thanks!
[6/6] arm64: dts: qcom: pm8916: Drop codec reg-names and mclk commit: c7b34291bb376598ea4279658bf3100c8cb1487b
Best regards,
participants (4)
-
Bjorn Andersson -
Mark Brown -
Rob Herring -
Stephan Gerhold