| Message ID | 1645182064-15843-6-git-send-email-quic_c_skakit@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Qualcomm Technologies, Inc. PM8008 regulator driver | expand |
Quoting Satya Priya (2022-02-18 03:01:03) > Add pm8008_infra and pm8008_regulators support for sc7280 idp. > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > --- > Changes in V2: > - As per Stephen's comments, replaced '_' with '-' for node names. > > Changes in V3: > - Changed the regulator node names as l1, l2 etc > - Changed "pm8008-regulators" to "regulators" > - Changed "qcom,min-dropout-voltage" to "regulator-min-dropout-voltage-microvolt" > > Changes in V4: > - Moved all common stuff to pm8008.dtsi and added board specific configurations here. > > Changes in V5: > - Changed the node names as per pm8008.dtsi > - Moved supply nodes to chip level (mfd node). > - Removed the regulator-mindropout property. > > Changes in V6: > - No changes. > > Changes in V7: > - No Changes. > > arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 66 ++++++++++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > index ecbf2b8..371ad19 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > @@ -263,6 +263,62 @@ > }; > }; > > +&i2c1 { Can we add another phandle? &pm8008_bus: &i2c1 { > + #address-cells = <1>; > + #size-cells = <0>; > + status = "okay"; > + > + #include "pm8008.dtsi" > +}; And then #include "pm8008.dtsi" and have the pm8008.dtsi file add itself as a child of &pm8008_bus? Then we can easily see that pm8008 is a child of pm8008_bus without having to figure out where the file is included. It also helps avoid polluting the i2c node with things that shouldn't be there in case we want to include configuration bits in the pm8008.dtsi file that aren't directly related to the bus node. > + > +&pm8008_infra { > + pinctrl-names = "default"; > + pinctrl-0 = <&pm8008_active>; > +}; > + > +&pm8008_regulators { > + vdd_l1_l2-supply = <&vreg_s8b_1p2>; > + vdd_l3_l4-supply = <&vreg_s1b_1p8>; > + vdd_l5-supply = <&vreg_bob>; > + vdd_l6-supply = <&vreg_bob>; > + vdd_l7-supply = <&vreg_bob>; > +}; > + > +&pm8008_l1 { > + regulator-min-microvolt = <950000>; > + regulator-max-microvolt = <1300000>; > +}; > + > +&pm8008_l2 { > + regulator-min-microvolt = <950000>; > + regulator-max-microvolt = <1250000>; > +}; > + > +&pm8008_l3 { > + regulator-min-microvolt = <1650000>; > + regulator-max-microvolt = <3000000>; > +}; > + > +&pm8008_l4 { > + regulator-min-microvolt = <1504000>; > + regulator-max-microvolt = <1600000>; > +}; > + > +&pm8008_l5 { > + regulator-min-microvolt = <2600000>; > + regulator-max-microvolt = <3000000>; > +}; > + > +&pm8008_l6 { > + regulator-min-microvolt = <2600000>; > + regulator-max-microvolt = <3000000>; > +}; > + > +&pm8008_l7 { > + regulator-min-microvolt = <3000000>; > + regulator-max-microvolt = <3544000>; > +}; > + > &qfprom { > vcc-supply = <&vreg_l1c_1p8>; > }; > @@ -375,6 +431,16 @@ > drive-strength = <2>; > }; > > +&pm8350c_gpios { > + pm8008_active: pm8008_active { No underscore in node names. pm8008_active: pm8008-active { > + pins = "gpio4"; > + function = "normal"; > + bias-disable; > + output-high; Is this a reset signal? Should the driver be deasserting the reset when it is ready? That could be the same time the gpio is acquired. > + power-source = <0>; > + }; > +};
On 2/19/2022 7:31 AM, Stephen Boyd wrote: > Quoting Satya Priya (2022-02-18 03:01:03) >> Add pm8008_infra and pm8008_regulators support for sc7280 idp. >> >> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> >> --- >> Changes in V2: >> - As per Stephen's comments, replaced '_' with '-' for node names. >> >> Changes in V3: >> - Changed the regulator node names as l1, l2 etc >> - Changed "pm8008-regulators" to "regulators" >> - Changed "qcom,min-dropout-voltage" to "regulator-min-dropout-voltage-microvolt" >> >> Changes in V4: >> - Moved all common stuff to pm8008.dtsi and added board specific configurations here. >> >> Changes in V5: >> - Changed the node names as per pm8008.dtsi >> - Moved supply nodes to chip level (mfd node). >> - Removed the regulator-mindropout property. >> >> Changes in V6: >> - No changes. >> >> Changes in V7: >> - No Changes. >> >> arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 66 ++++++++++++++++++++++++++++++++ >> 1 file changed, 66 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> index ecbf2b8..371ad19 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> @@ -263,6 +263,62 @@ >> }; >> }; >> >> +&i2c1 { > Can we add another phandle? > > &pm8008_bus: &i2c1 { Okay. >> + #address-cells = <1>; >> + #size-cells = <0>; >> + status = "okay"; >> + >> + #include "pm8008.dtsi" >> +}; > And then > > #include "pm8008.dtsi" Okay. > and have the pm8008.dtsi file add itself as a child of &pm8008_bus? Then > we can easily see that pm8008 is a child of pm8008_bus without having to > figure out where the file is included. It also helps avoid polluting the > i2c node with things that shouldn't be there in case we want to include > configuration bits in the pm8008.dtsi file that aren't directly related > to the bus node. > >> + >> +&pm8008_infra { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pm8008_active>; >> +}; >> + >> +&pm8008_regulators { >> + vdd_l1_l2-supply = <&vreg_s8b_1p2>; >> + vdd_l3_l4-supply = <&vreg_s1b_1p8>; >> + vdd_l5-supply = <&vreg_bob>; >> + vdd_l6-supply = <&vreg_bob>; >> + vdd_l7-supply = <&vreg_bob>; >> +}; >> + >> +&pm8008_l1 { >> + regulator-min-microvolt = <950000>; >> + regulator-max-microvolt = <1300000>; >> +}; >> + >> +&pm8008_l2 { >> + regulator-min-microvolt = <950000>; >> + regulator-max-microvolt = <1250000>; >> +}; >> + >> +&pm8008_l3 { >> + regulator-min-microvolt = <1650000>; >> + regulator-max-microvolt = <3000000>; >> +}; >> + >> +&pm8008_l4 { >> + regulator-min-microvolt = <1504000>; >> + regulator-max-microvolt = <1600000>; >> +}; >> + >> +&pm8008_l5 { >> + regulator-min-microvolt = <2600000>; >> + regulator-max-microvolt = <3000000>; >> +}; >> + >> +&pm8008_l6 { >> + regulator-min-microvolt = <2600000>; >> + regulator-max-microvolt = <3000000>; >> +}; >> + >> +&pm8008_l7 { >> + regulator-min-microvolt = <3000000>; >> + regulator-max-microvolt = <3544000>; >> +}; >> + >> &qfprom { >> vcc-supply = <&vreg_l1c_1p8>; >> }; >> @@ -375,6 +431,16 @@ >> drive-strength = <2>; >> }; >> >> +&pm8350c_gpios { >> + pm8008_active: pm8008_active { > No underscore in node names. pm8008_active: pm8008-active { Okay. >> + pins = "gpio4"; >> + function = "normal"; >> + bias-disable; >> + output-high; > Is this a reset signal? Should the driver be deasserting the reset when > it is ready? That could be the same time the gpio is acquired. I didn't get your question exactly.. hope this answers your query The pm8008 chip needs this gpio to be toggled , in order to come out of reset and start any transactions.. Please let me know if you have more queries >> + power-source = <0>; >> + }; >> +};
Quoting Satya Priya Kakitapalli (Temp) (2022-02-28 06:25:06) > > On 2/19/2022 7:31 AM, Stephen Boyd wrote: > > Quoting Satya Priya (2022-02-18 03:01:03) > > >> + pins = "gpio4"; > >> + function = "normal"; > >> + bias-disable; > >> + output-high; > > Is this a reset signal? Should the driver be deasserting the reset when > > it is ready? That could be the same time the gpio is acquired. > > > I didn't get your question exactly.. hope this answers your query > > The pm8008 chip needs this gpio to be toggled , in order to come out of > reset and start any transactions.. > > Please let me know if you have more queries Yes that answers it for me. Thanks. This is a reset gpio and should be a DT property like reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_HIGH> in the pm8008 node. When the driver probes it should get the gpio and do any toggling to take it out of reset. It shouldn't be done through pinconf settings.
On 3/1/2022 2:06 AM, Stephen Boyd wrote: > Quoting Satya Priya Kakitapalli (Temp) (2022-02-28 06:25:06) >> On 2/19/2022 7:31 AM, Stephen Boyd wrote: >>> Quoting Satya Priya (2022-02-18 03:01:03) >>>> + pins = "gpio4"; >>>> + function = "normal"; >>>> + bias-disable; >>>> + output-high; >>> Is this a reset signal? Should the driver be deasserting the reset when >>> it is ready? That could be the same time the gpio is acquired. >> >> I didn't get your question exactly.. hope this answers your query >> >> The pm8008 chip needs this gpio to be toggled , in order to come out of >> reset and start any transactions.. >> >> Please let me know if you have more queries > Yes that answers it for me. Thanks. > > This is a reset gpio and should be a DT property like > > reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_HIGH> > > in the pm8008 node. When the driver probes it should get the gpio and > do any toggling to take it out of reset. It shouldn't be done through > pinconf settings. Okay, IIUC, I have to remove the output-high here and add reset-gpios in pm8008 DT node. And then add below code in pm8008 mfd driver probe + chip->reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(chip->reset_gpio)) { + dev_err(chip->dev, "failed to acquire reset gpio\n"); + return PTR_ERR(chip->reset_gpio); + } + gpiod_set_value(chip->reset_gpio, 1); This is working for me, Please let me know if I'm missing something.
Quoting Satya Priya Kakitapalli (Temp) (2022-03-07 06:49:31) > > On 3/1/2022 2:06 AM, Stephen Boyd wrote: > > Quoting Satya Priya Kakitapalli (Temp) (2022-02-28 06:25:06) > >> On 2/19/2022 7:31 AM, Stephen Boyd wrote: > >>> Quoting Satya Priya (2022-02-18 03:01:03) > >>>> + pins = "gpio4"; > >>>> + function = "normal"; > >>>> + bias-disable; > >>>> + output-high; > >>> Is this a reset signal? Should the driver be deasserting the reset when > >>> it is ready? That could be the same time the gpio is acquired. > >> > >> I didn't get your question exactly.. hope this answers your query > >> > >> The pm8008 chip needs this gpio to be toggled , in order to come out of > >> reset and start any transactions.. > >> > >> Please let me know if you have more queries > > Yes that answers it for me. Thanks. > > > > This is a reset gpio and should be a DT property like > > > > reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_HIGH> > > > > in the pm8008 node. When the driver probes it should get the gpio and > > do any toggling to take it out of reset. It shouldn't be done through > > pinconf settings. > > > Okay, IIUC, I have to remove the output-high here and add reset-gpios > in pm8008 DT node. And then add below code in pm8008 mfd driver probe > > + chip->reset_gpio = devm_gpiod_get(chip->dev, "reset", > GPIOD_OUT_HIGH); > + if (IS_ERR(chip->reset_gpio)) { > + dev_err(chip->dev, "failed to acquire reset > gpio\n"); > + return PTR_ERR(chip->reset_gpio); > + } > + gpiod_set_value(chip->reset_gpio, 1); > > This is working for me, Please let me know if I'm missing something. > Yep looks good to me.
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi index ecbf2b8..371ad19 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi @@ -263,6 +263,62 @@ }; }; +&i2c1 { + #address-cells = <1>; + #size-cells = <0>; + status = "okay"; + + #include "pm8008.dtsi" +}; + +&pm8008_infra { + pinctrl-names = "default"; + pinctrl-0 = <&pm8008_active>; +}; + +&pm8008_regulators { + vdd_l1_l2-supply = <&vreg_s8b_1p2>; + vdd_l3_l4-supply = <&vreg_s1b_1p8>; + vdd_l5-supply = <&vreg_bob>; + vdd_l6-supply = <&vreg_bob>; + vdd_l7-supply = <&vreg_bob>; +}; + +&pm8008_l1 { + regulator-min-microvolt = <950000>; + regulator-max-microvolt = <1300000>; +}; + +&pm8008_l2 { + regulator-min-microvolt = <950000>; + regulator-max-microvolt = <1250000>; +}; + +&pm8008_l3 { + regulator-min-microvolt = <1650000>; + regulator-max-microvolt = <3000000>; +}; + +&pm8008_l4 { + regulator-min-microvolt = <1504000>; + regulator-max-microvolt = <1600000>; +}; + +&pm8008_l5 { + regulator-min-microvolt = <2600000>; + regulator-max-microvolt = <3000000>; +}; + +&pm8008_l6 { + regulator-min-microvolt = <2600000>; + regulator-max-microvolt = <3000000>; +}; + +&pm8008_l7 { + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3544000>; +}; + &qfprom { vcc-supply = <&vreg_l1c_1p8>; }; @@ -375,6 +431,16 @@ drive-strength = <2>; }; +&pm8350c_gpios { + pm8008_active: pm8008_active { + pins = "gpio4"; + function = "normal"; + bias-disable; + output-high; + power-source = <0>; + }; +}; + &qspi_cs0 { bias-disable; };
Add pm8008_infra and pm8008_regulators support for sc7280 idp. Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> --- Changes in V2: - As per Stephen's comments, replaced '_' with '-' for node names. Changes in V3: - Changed the regulator node names as l1, l2 etc - Changed "pm8008-regulators" to "regulators" - Changed "qcom,min-dropout-voltage" to "regulator-min-dropout-voltage-microvolt" Changes in V4: - Moved all common stuff to pm8008.dtsi and added board specific configurations here. Changes in V5: - Changed the node names as per pm8008.dtsi - Moved supply nodes to chip level (mfd node). - Removed the regulator-mindropout property. Changes in V6: - No changes. Changes in V7: - No Changes. arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 66 ++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)