| Message ID | 20230708072835.3035398-4-quic_jprakash@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | iio: adc: Add support for QCOM SPMI PMIC5 Gen3 ADC | expand |
Hi Krzysztof, On 7/9/2023 10:48 PM, Krzysztof Kozlowski wrote: > On 08/07/2023 09:28, Jishnu Prakash wrote: >> The name "ADC7" needs to be replaced with the name "ADC5_GEN2" >> everywhere to match the convention used for these ADC peripherals >> on Qualcomm Technologies, Inc. PMICs. Update devicetree files for > We do not rename compatibles to match convention. Please provide proper > rationale. I'll avoid renaming the compatible directly, will just mark it deprecated - but is it fine to do the other changes, for updating the macro names used in devicetree (replacing the ADC7 macros with the ADC5 Gen2 macros)? I do see an example of a macro change in devicetree done in this patch: https://lore.kernel.org/all/cover.1646388139.git.zong.li@sifive.com/. Patch 2 here replaced some macro definitions: https://lore.kernel.org/all/f9284873c2993a9952d9fe4f8dd5e89f20daab75.1646388139.git.zong.li@sifive.com/. Patch 3 made the corresponding update in devicetree files: https://lore.kernel.org/all/db92d209fa700f7da8bc8028083476fcc138d80e.1646388139.git.zong.li@sifive.com/. From this mail, it looks like the maintainer was willing to pick them at that time: https://lore.kernel.org/all/20220315225652.CDAD1C340E8@smtp.kernel.org/, would something similar be possible here? Thanks, Jishnu
Hi Krzysztof, On 10/23/2023 12:02 PM, Krzysztof Kozlowski wrote: > On 23/10/2023 08:09, Jishnu Prakash wrote: >> Hi Krzysztof, >> >> On 7/9/2023 10:48 PM, Krzysztof Kozlowski wrote: >>> On 08/07/2023 09:28, Jishnu Prakash wrote: >>>> The name "ADC7" needs to be replaced with the name "ADC5_GEN2" >>>> everywhere to match the convention used for these ADC peripherals >>>> on Qualcomm Technologies, Inc. PMICs. Update devicetree files for >>> We do not rename compatibles to match convention. Please provide proper >>> rationale. >> I'll avoid renaming the compatible directly, will just mark it >> deprecated - but is it fine to do the other changes, for updating the >> macro names used in devicetree (replacing the ADC7 macros with the ADC5 >> Gen2 macros)? > Please provide proper rationale why "ADC7 needs to be replaced". Your > marketing is not a proper rationale. The name "ADC7" was the one used internally at first, but it got changed later to "ADC5 Gen2" by our HW team, after we had added this support both downstream and upstream. Since we are now adding support for the next generation named "ADC5 Gen3", we thought it would be helpful to indicate in some way that this generation (ADC7) lies between the earlier ADC5 and the latest ADC5 Gen3. Since you do not want us to modify the existing bindings, is it fine if I just add a new compatible for ADC5 Gen2 and comments to indicate the ADC7 compatible should be considered deprecated? If you are not convinced, we can drop the Gen2 name related changes from the patch series. > >> I do see an example of a macro change in devicetree done in this patch: >> https://lore.kernel.org/all/cover.1646388139.git.zong.li@sifive.com/. >> >> Patch 2 here replaced some macro definitions: >> https://lore.kernel.org/all/f9284873c2993a9952d9fe4f8dd5e89f20daab75.1646388139.git.zong.li@sifive.com/. >> >> Patch 3 made the corresponding update in devicetree files: >> https://lore.kernel.org/all/db92d209fa700f7da8bc8028083476fcc138d80e.1646388139.git.zong.li@sifive.com/. > And what is rationale in that patchset? Right, I see that the change was made to refactor the driver code and avoid unused variable errors, not just a name change. Thanks, Jishnu > >> >> From this mail, it looks like the maintainer was willing to pick them >> at that time: >> https://lore.kernel.org/all/20220315225652.CDAD1C340E8@smtp.kernel.org/, >> would something similar be possible here? > For stated before marketing reasons - no, would not be possible. > > Best regards, > Krzysztof >
On 09/11/2023 09:22, Jishnu Prakash wrote: > Hi Krzysztof, > > On 10/23/2023 12:02 PM, Krzysztof Kozlowski wrote: >> On 23/10/2023 08:09, Jishnu Prakash wrote: >>> Hi Krzysztof, >>> >>> On 7/9/2023 10:48 PM, Krzysztof Kozlowski wrote: >>>> On 08/07/2023 09:28, Jishnu Prakash wrote: >>>>> The name "ADC7" needs to be replaced with the name "ADC5_GEN2" >>>>> everywhere to match the convention used for these ADC peripherals >>>>> on Qualcomm Technologies, Inc. PMICs. Update devicetree files for >>>> We do not rename compatibles to match convention. Please provide proper >>>> rationale. >>> I'll avoid renaming the compatible directly, will just mark it >>> deprecated - but is it fine to do the other changes, for updating the >>> macro names used in devicetree (replacing the ADC7 macros with the ADC5 >>> Gen2 macros)? >> Please provide proper rationale why "ADC7 needs to be replaced". Your >> marketing is not a proper rationale. > > > The name "ADC7" was the one used internally at first, but it got changed > later to "ADC5 Gen2" by our HW team, after we had added this support > both downstream and upstream. Since we are now adding support for the > next generation named "ADC5 Gen3", we thought it would be helpful to > indicate in some way that this generation (ADC7) lies between the > earlier ADC5 and the latest ADC5 Gen3. You keep replying with the same arguments as before. I wrote that marketing, so how you call your devices and then change your mind, is not the valid rationale. > > Since you do not want us to modify the existing bindings, is it fine if > I just add a new compatible for ADC5 Gen2 and comments to indicate the > ADC7 compatible should be considered deprecated? No, because adc7 compatible is valid and there is no reason to replace it. Just because you changed naming does not matter for compatibles. It's just unique string, that's it. Don't touch it. > > If you are not convinced, we can drop the Gen2 name related changes from > the patch series. Feel free to add comments or descriptions, if you want to map some marketing name to real hardware or to compatibles. Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/qcom/pmk8350.dtsi b/arch/arm64/boot/dts/qcom/pmk8350.dtsi index bc6297e7253e..149d2bb43d2d 100644 --- a/arch/arm64/boot/dts/qcom/pmk8350.dtsi +++ b/arch/arm64/boot/dts/qcom/pmk8350.dtsi @@ -50,7 +50,7 @@ pon_resin: resin { }; pmk8350_vadc: adc@3100 { - compatible = "qcom,spmi-adc7"; + compatible = "qcom,spmi-adc5-gen2"; reg = <0x3100>; #address-cells = <1>; #size-cells = <0>; @@ -59,7 +59,7 @@ pmk8350_vadc: adc@3100 { }; pmk8350_adc_tm: adc-tm@3400 { - compatible = "qcom,adc-tm7"; + compatible = "qcom,spmi-adc-tm5-gen2"; reg = <0x3400>; interrupts = <PMK8350_SID 0x34 0x0 IRQ_TYPE_EDGE_RISING>; #address-cells = <1>; diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts index 15222e92e3f5..bc65e6c6232f 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts @@ -7,7 +7,7 @@ /dts-v1/; -#include <dt-bindings/iio/qcom,spmi-adc7-pmr735a.h> +#include <dt-bindings/iio/qcom,spmi-adc5-gen2-pmr735a.h> #include "sc7280-idp.dtsi" #include "pmr735a.dtsi" @@ -74,7 +74,7 @@ &nvme_3v3_regulator { &pmk8350_vadc { pmr735a-die-temp@403 { - reg = <PMR735A_ADC7_DIE_TEMP>; + reg = <PMR735A_ADC5_GEN2_DIE_TEMP>; label = "pmr735a_die_temp"; qcom,pre-scaling = <1 1>; }; diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi index 21027042cf13..da413694e230 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi @@ -5,7 +5,7 @@ * Copyright (c) 2021, The Linux Foundation. All rights reserved. */ -#include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h> +#include <dt-bindings/iio/qcom,spmi-adc5-gen2-pmk8350.h> #include <dt-bindings/input/linux-event-codes.h> #include "sc7280.dtsi" #include "pm7325.dtsi" @@ -433,7 +433,7 @@ &pcie1_phy { &pmk8350_vadc { pmk8350-die-temp@3 { - reg = <PMK8350_ADC7_DIE_TEMP>; + reg = <PMK8350_ADC5_GEN2_DIE_TEMP>; label = "pmk8350_die_temp"; qcom,pre-scaling = <1 1>; }; diff --git a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi index 9137db066d9e..ed26bff7432d 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi @@ -11,8 +11,8 @@ * Copyright 2022 Google LLC. */ -#include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h> -#include <dt-bindings/iio/qcom,spmi-adc7-pmr735a.h> +#include <dt-bindings/iio/qcom,spmi-adc5-gen2-pmk8350.h> +#include <dt-bindings/iio/qcom,spmi-adc5-gen2-pmr735a.h> #include <dt-bindings/pinctrl/qcom,pmic-gpio.h> #include <dt-bindings/regulator/qcom,rpmh-regulator.h> @@ -384,13 +384,13 @@ &pm8350c_pwm { &pmk8350_vadc { pmk8350-die-temp@3 { - reg = <PMK8350_ADC7_DIE_TEMP>; + reg = <PMK8350_ADC5_GEN2_DIE_TEMP>; label = "pmk8350_die_temp"; qcom,pre-scaling = <1 1>; }; pmr735a-die-temp@403 { - reg = <PMR735A_ADC7_DIE_TEMP>; + reg = <PMR735A_ADC5_GEN2_DIE_TEMP>; label = "pmr735a_die_temp"; qcom,pre-scaling = <1 1>; }; diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts index 7cc3028440b6..cfd5dbbacdcb 100644 --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts @@ -7,9 +7,9 @@ /dts-v1/; #include <dt-bindings/gpio/gpio.h> -#include <dt-bindings/iio/qcom,spmi-adc7-pm8350.h> -#include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h> -#include <dt-bindings/iio/qcom,spmi-adc7-pmr735a.h> +#include <dt-bindings/iio/qcom,spmi-adc5-gen2-pm8350.h> +#include <dt-bindings/iio/qcom,spmi-adc5-gen2-pmk8350.h> +#include <dt-bindings/iio/qcom,spmi-adc5-gen2-pmr735a.h> #include <dt-bindings/input/gpio-keys.h> #include <dt-bindings/input/input.h> #include <dt-bindings/regulator/qcom,rpmh-regulator.h> @@ -747,7 +747,7 @@ &pmk8280_adc_tm { sys-therm@0 { reg = <0>; - io-channels = <&pmk8280_vadc PM8350_ADC7_AMUX_THM1_100K_PU(1)>; + io-channels = <&pmk8280_vadc PM8350_ADC5_GEN2_AMUX_THM1_100K_PU(1)>; qcom,hw-settle-time-us = <200>; qcom,avg-samples = <2>; qcom,ratiometric; @@ -755,7 +755,7 @@ sys-therm@0 { sys-therm@1 { reg = <1>; - io-channels = <&pmk8280_vadc PM8350_ADC7_AMUX_THM2_100K_PU(1)>; + io-channels = <&pmk8280_vadc PM8350_ADC5_GEN2_AMUX_THM2_100K_PU(1)>; qcom,hw-settle-time-us = <200>; qcom,avg-samples = <2>; qcom,ratiometric; @@ -763,7 +763,7 @@ sys-therm@1 { sys-therm@2 { reg = <2>; - io-channels = <&pmk8280_vadc PM8350_ADC7_AMUX_THM3_100K_PU(1)>; + io-channels = <&pmk8280_vadc PM8350_ADC5_GEN2_AMUX_THM3_100K_PU(1)>; qcom,hw-settle-time-us = <200>; qcom,avg-samples = <2>; qcom,ratiometric; @@ -771,7 +771,7 @@ sys-therm@2 { sys-therm@3 { reg = <3>; - io-channels = <&pmk8280_vadc PM8350_ADC7_AMUX_THM4_100K_PU(1)>; + io-channels = <&pmk8280_vadc PM8350_ADC5_GEN2_AMUX_THM4_100K_PU(1)>; qcom,hw-settle-time-us = <200>; qcom,avg-samples = <2>; qcom,ratiometric; @@ -779,7 +779,7 @@ sys-therm@3 { sys-therm@4 { reg = <4>; - io-channels = <&pmk8280_vadc PM8350_ADC7_AMUX_THM1_100K_PU(3)>; + io-channels = <&pmk8280_vadc PM8350_ADC5_GEN2_AMUX_THM1_100K_PU(3)>; qcom,hw-settle-time-us = <200>; qcom,avg-samples = <2>; qcom,ratiometric; @@ -787,7 +787,7 @@ sys-therm@4 { sys-therm@5 { reg = <5>; - io-channels = <&pmk8280_vadc PM8350_ADC7_AMUX_THM2_100K_PU(3)>; + io-channels = <&pmk8280_vadc PM8350_ADC5_GEN2_AMUX_THM2_100K_PU(3)>; qcom,hw-settle-time-us = <200>; qcom,avg-samples = <2>; qcom,ratiometric; @@ -795,7 +795,7 @@ sys-therm@5 { sys-therm@6 { reg = <6>; - io-channels = <&pmk8280_vadc PM8350_ADC7_AMUX_THM3_100K_PU(3)>; + io-channels = <&pmk8280_vadc PM8350_ADC5_GEN2_AMUX_THM3_100K_PU(3)>; qcom,hw-settle-time-us = <200>; qcom,avg-samples = <2>; qcom,ratiometric; @@ -803,7 +803,7 @@ sys-therm@6 { sys-therm@7 { reg = <7>; - io-channels = <&pmk8280_vadc PM8350_ADC7_AMUX_THM4_100K_PU(3)>; + io-channels = <&pmk8280_vadc PM8350_ADC5_GEN2_AMUX_THM4_100K_PU(3)>; qcom,hw-settle-time-us = <200>; qcom,avg-samples = <2>; qcom,ratiometric; @@ -837,88 +837,88 @@ &pmk8280_vadc { status = "okay"; pmic-die-temp@3 { - reg = <PMK8350_ADC7_DIE_TEMP>; + reg = <PMK8350_ADC5_GEN2_DIE_TEMP>; qcom,pre-scaling = <1 1>; label = "pmk8350_die_temp"; }; xo-therm@44 { - reg = <PMK8350_ADC7_AMUX_THM1_100K_PU>; + reg = <PMK8350_ADC5_GEN2_AMUX_THM1_100K_PU>; qcom,hw-settle-time = <200>; qcom,ratiometric; label = "pmk8350_xo_therm"; }; pmic-die-temp@103 { - reg = <PM8350_ADC7_DIE_TEMP(1)>; + reg = <PM8350_ADC5_GEN2_DIE_TEMP(1)>; qcom,pre-scaling = <1 1>; label = "pmc8280_1_die_temp"; }; sys-therm@144 { - reg = <PM8350_ADC7_AMUX_THM1_100K_PU(1)>; + reg = <PM8350_ADC5_GEN2_AMUX_THM1_100K_PU(1)>; qcom,hw-settle-time = <200>; qcom,ratiometric; label = "sys_therm1"; }; sys-therm@145 { - reg = <PM8350_ADC7_AMUX_THM2_100K_PU(1)>; + reg = <PM8350_ADC5_GEN2_AMUX_THM2_100K_PU(1)>; qcom,hw-settle-time = <200>; qcom,ratiometric; label = "sys_therm2"; }; sys-therm@146 { - reg = <PM8350_ADC7_AMUX_THM3_100K_PU(1)>; + reg = <PM8350_ADC5_GEN2_AMUX_THM3_100K_PU(1)>; qcom,hw-settle-time = <200>; qcom,ratiometric; label = "sys_therm3"; }; sys-therm@147 { - reg = <PM8350_ADC7_AMUX_THM4_100K_PU(1)>; + reg = <PM8350_ADC5_GEN2_AMUX_THM4_100K_PU(1)>; qcom,hw-settle-time = <200>; qcom,ratiometric; label = "sys_therm4"; }; pmic-die-temp@303 { - reg = <PM8350_ADC7_DIE_TEMP(3)>; + reg = <PM8350_ADC5_GEN2_DIE_TEMP(3)>; qcom,pre-scaling = <1 1>; label = "pmc8280_2_die_temp"; }; sys-therm@344 { - reg = <PM8350_ADC7_AMUX_THM1_100K_PU(3)>; + reg = <PM8350_ADC5_GEN2_AMUX_THM1_100K_PU(3)>; qcom,hw-settle-time = <200>; qcom,ratiometric; label = "sys_therm5"; }; sys-therm@345 { - reg = <PM8350_ADC7_AMUX_THM2_100K_PU(3)>; + reg = <PM8350_ADC5_GEN2_AMUX_THM2_100K_PU(3)>; qcom,hw-settle-time = <200>; qcom,ratiometric; label = "sys_therm6"; }; sys-therm@346 { - reg = <PM8350_ADC7_AMUX_THM3_100K_PU(3)>; + reg = <PM8350_ADC5_GEN2_AMUX_THM3_100K_PU(3)>; qcom,hw-settle-time = <200>; qcom,ratiometric; label = "sys_therm7"; }; sys-therm@347 { - reg = <PM8350_ADC7_AMUX_THM4_100K_PU(3)>; + reg = <PM8350_ADC5_GEN2_AMUX_THM4_100K_PU(3)>; qcom,hw-settle-time = <200>; qcom,ratiometric; label = "sys_therm8"; }; pmic-die-temp@403 { - reg = <PMR735A_ADC7_DIE_TEMP>; + reg = <PMR735A_ADC5_GEN2_DIE_TEMP>; qcom,pre-scaling = <1 1>; label = "pmr735a_die_temp"; }; diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi index a0ba535bb6c9..ae9491adaa6a 100644 --- a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi +++ b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi @@ -78,7 +78,7 @@ pmk8280_pon_resin: resin { }; pmk8280_vadc: adc@3100 { - compatible = "qcom,spmi-adc7"; + compatible = "qcom,spmi-adc5-gen2"; reg = <0x3100>; interrupts-extended = <&spmi_bus 0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>; #address-cells = <1>; diff --git a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts index e3dc49951523..52119cc1250e 100644 --- a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts +++ b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts @@ -9,7 +9,7 @@ #define PMK8350_SID 6 #include <dt-bindings/gpio/gpio.h> -#include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h> +#include <dt-bindings/iio/qcom,spmi-adc5-gen2-pmk8350.h> #include <dt-bindings/input/input.h> #include <dt-bindings/leds/common.h> #include <dt-bindings/pinctrl/qcom,pmic-gpio.h> @@ -517,7 +517,7 @@ &pmk8350_rtc { &pmk8350_vadc { adc-chan@644 { - reg = <PMK8350_ADC7_AMUX_THM1_100K_PU>; + reg = <PMK8350_ADC5_GEN2_AMUX_THM1_100K_PU>; qcom,ratiometric; qcom,hw-settle-time = <200>; qcom,pre-scaling = <1 1>;
The name "ADC7" needs to be replaced with the name "ADC5_GEN2" everywhere to match the convention used for these ADC peripherals on Qualcomm Technologies, Inc. PMICs. Update devicetree files for the corresponding name change done in bindings and driver. Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com> --- arch/arm64/boot/dts/qcom/pmk8350.dtsi | 4 +- arch/arm64/boot/dts/qcom/sc7280-idp.dts | 4 +- arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 4 +- arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 8 ++-- .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 48 +++++++++---------- arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 2 +- .../boot/dts/qcom/sm7225-fairphone-fp4.dts | 4 +- 7 files changed, 37 insertions(+), 37 deletions(-)