| Message ID | 1645182064-15843-1-git-send-email-quic_c_skakit@quicinc.com |
|---|---|
| Headers | show |
| Series | Add Qualcomm Technologies, Inc. PM8008 regulator driver | expand |
Quoting Satya Priya (2022-02-18 03:00:59) > Add regulators and their supply nodes. Add separate compatible > "qcom,pm8008-regulators" to differentiate between pm8008 infra > and pm8008 regulators mfd devices. > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > --- Is the register layout compatible with SPMI regulators? The gpio node seems to be fully compatible and the same driver probes there for SPMI and i2c, so I wonder why we can't extend the existing SPMI gpio and regulator bindings to have the new compatible strings for pm8008. Is anything really different, or do we have the same device talking i2c instead of SPMI now? Possibly it's exposing the different hardware blocks inside the PMIC at different i2c addresses. It looks like the i2c address is 0x8 and then there's 16-bits of address space inside the i2c device to do things. 0x9 is the i2c address for the regulators and then each ldo is at some offset in there? > Changes in V2: > - As per Rob's comments changed "pm8008[a-z]?-regulator" to > "^pm8008[a-z]?-regulators". > > Changes in V3: > - Fixed bot errors. > - As per stephen's comments, changed "^pm8008[a-z]?-regulators$" to > "regulators". > > Changes in V4: > - Changed compatible string to "qcom,pm8008-regulators" > > Changes in V5: > - Remove compatible for regulators node. > - Move supply nodes of the regulators to chip level. > > Changes in V6: > - No changes. > > Changes in V7: > - Removed the intermediate regulators node and added ldos > directly under mfd node. > > .../devicetree/bindings/mfd/qcom,pm8008.yaml | 50 +++++++++++++++++++--- > 1 file changed, 43 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml > index ec3138c..6b3b53e 100644 > --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml > +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml > @@ -16,7 +16,9 @@ description: | > > properties: > compatible: > - const: qcom,pm8008 > + enum: > + - qcom,pm8008 > + - qcom,pm8008-regulators > > reg: > description: > @@ -44,6 +46,21 @@ properties: > "#size-cells": > const: 0 > > + vdd_l1_l2-supply: > + description: Input supply phandle of ldo1 and ldo2 regulators. > + > + vdd_l3_l4-supply: > + description: Input supply phandle of ldo3 and ldo4 regulators. > + > + vdd_l5-supply: > + description: Input supply phandle of ldo5 regulator. > + > + vdd_l6-supply: > + description: Input supply phandle of ldo6 regulator. > + > + vdd_l7-supply: > + description: Input supply phandle of ldo7 regulator. > + > patternProperties: > "^gpio@[0-9a-f]+$": > type: object > @@ -85,13 +102,16 @@ patternProperties: > > additionalProperties: false > > + "^ldo[1-7]$": > + type: object > + $ref: "../regulator/regulator.yaml#" > + description: PM8008 regulator peripherals of PM8008 regulator device > + > required: > - compatible > - reg > - - interrupts > - "#address-cells" > - "#size-cells" > - - "#interrupt-cells" > > additionalProperties: false > > @@ -102,13 +122,11 @@ examples: > qupv3_se13_i2c { > #address-cells = <1>; > #size-cells = <0>; > - pm8008i@8 { > + pm8008_infra: pm8008@8 { > compatible = "qcom,pm8008"; > reg = <0x8>; > #address-cells = <1>; > #size-cells = <0>; > - interrupt-controller; > - #interrupt-cells = <2>; > > interrupt-parent = <&tlmm>; > interrupts = <32 IRQ_TYPE_EDGE_RISING>; I still fail to see what this part of the diff has to do with regulators. Can it be split off to a different patch with a clear description of why interrupt-controller and #interrupt-cells is no longer required for qcom,pm8008? It really looks like we're combining the binding for qcom,pm8008 and qcom,pm8008-regulators at the same level, which looks wrong. We don't want to describe the least common denominator between the two bindings. Why not make two different bindings and files? One for the interrupty gpio/interrupt controller device (at 0x8) and one for the regulator one (at 0x9)? > @@ -123,6 +141,24 @@ examples: > #interrupt-cells = <2>; > }; > }; > - }; > > + pm8008_regulators: pm8008@9 { pmic@9, or regulators@9? The node name should be generic. > + compatible = "qcom,pm8008-regulators"; > + reg = <0x9>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + 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: ldo1 { > + regulator-name = "pm8008_l1"; > + regulator-min-microvolt = <950000>; > + regulator-max-microvolt = <1300000>; > + }; > + }; For some i2c devices that appear on multiple i2c addresses we make an i2c client for each address in the driver that attaches to the node we put in DT. I suppose that won't work easily here. Either way, it would make it much clearer if this existing binding was left alone. Is there other functionality inside the i2c address 0x9 register space that isn't regulators?
Quoting Satya Priya (2022-02-18 03:01:01) > Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC > containing 7 LDO regulators. Add a PM8008 regulator driver to > support PMIC regulator management via the regulator framework. > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c > new file mode 100644 > index 0000000..1c52864 > --- /dev/null > +++ b/drivers/regulator/qcom-pm8008-regulator.c > @@ -0,0 +1,205 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */ > + > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/machine.h> > + > +#define VSET_STEP_MV 8 > +#define VSET_STEP_UV (VSET_STEP_MV * 1000) > + > +#define LDO_ENABLE_REG(base) ((base) + 0x46) > +#define ENABLE_BIT BIT(7) This is SPMI_COMMON_REG_ENABLE and SPMI_COMMON_ENABLE_MASK > + > +#define LDO_VSET_LB_REG(base) ((base) + 0x40) > + > +#define LDO_STEPPER_CTL_REG(base) ((base) + 0x3b) > +#define DEFAULT_VOLTAGE_STEPPER_RATE 38400 > +#define STEP_RATE_MASK GENMASK(1, 0) > + > +struct regulator_data { Please use a name like spmi_regulator_data :) Or pm8008_regulator_data? Something that isn't so generic. It seems similar to qcom_spmi-regulator.c so is there some reason we can't shove this into there? Less code for things that aren't relevant I guess? > + const char *name; > + const char *supply_name; > + u16 base; > + int min_uv; > + int max_uv; > + int min_dropout_uv; > + const struct linear_range *voltage_range;
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:20 AM, Stephen Boyd wrote: > Quoting Satya Priya (2022-02-18 03:01:01) >> Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC >> containing 7 LDO regulators. Add a PM8008 regulator driver to >> support PMIC regulator management via the regulator framework. >> >> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> >> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c >> new file mode 100644 >> index 0000000..1c52864 >> --- /dev/null >> +++ b/drivers/regulator/qcom-pm8008-regulator.c >> @@ -0,0 +1,205 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */ >> + >> +#include <linux/device.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> +#include <linux/regulator/driver.h> >> +#include <linux/regulator/machine.h> >> + >> +#define VSET_STEP_MV 8 >> +#define VSET_STEP_UV (VSET_STEP_MV * 1000) >> + >> +#define LDO_ENABLE_REG(base) ((base) + 0x46) >> +#define ENABLE_BIT BIT(7) > This is SPMI_COMMON_REG_ENABLE and SPMI_COMMON_ENABLE_MASK > >> + >> +#define LDO_VSET_LB_REG(base) ((base) + 0x40) >> + >> +#define LDO_STEPPER_CTL_REG(base) ((base) + 0x3b) >> +#define DEFAULT_VOLTAGE_STEPPER_RATE 38400 >> +#define STEP_RATE_MASK GENMASK(1, 0) >> + >> +struct regulator_data { > Please use a name like spmi_regulator_data :) Or pm8008_regulator_data? > Something that isn't so generic. Okay. > It seems similar to qcom_spmi-regulator.c so is there some reason we > can't shove this into there? Less code for things that aren't relevant I > guess? As mentioned earlier, the register layout is not compatible and some other differences w.r.t to code like headrooms, using mfd driver to register ldos etc.. >> + const char *name; >> + const char *supply_name; >> + u16 base; >> + int min_uv; >> + int max_uv; >> + int min_dropout_uv; >> + const struct linear_range *voltage_range;
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:14:56) > > On 2/19/2022 7:09 AM, Stephen Boyd wrote: > > Quoting Satya Priya (2022-02-18 03:00:59) > >> Add regulators and their supply nodes. Add separate compatible > >> "qcom,pm8008-regulators" to differentiate between pm8008 infra > >> and pm8008 regulators mfd devices. > >> > >> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > >> --- > > Is the register layout compatible with SPMI regulators? The gpio node > > seems to be fully compatible and the same driver probes there for SPMI > > and i2c, so I wonder why we can't extend the existing SPMI gpio and > > regulator bindings to have the new compatible strings for pm8008. Is > > anything really different, or do we have the same device talking i2c > > instead of SPMI now? Possibly it's exposing the different hardware > > blocks inside the PMIC at different i2c addresses. It looks like the i2c > > address is 0x8 and then there's 16-bits of address space inside the i2c > > device to do things. 0x9 is the i2c address for the regulators and then > > each ldo is at some offset in there? > > > The register layout is not compatible with spmi regulators, I see some > differences w.r.t VOLTAGE_CTL, EN_CTL, MODE_CTL registers. Also, there > is no headroom related stuff in the spmi driver. It sounds like minor differences and/or improvements to the existing SPMI regulator hardware. > >> interrupt-parent = <&tlmm>; > >> interrupts = <32 IRQ_TYPE_EDGE_RISING>; > > I still fail to see what this part of the diff has to do with > > regulators. Can it be split off to a different patch with a clear > > description of why interrupt-controller and #interrupt-cells is no > > longer required for qcom,pm8008? > > > This diff has nothing to do with regulators, I removed it to avoid yaml > errors during dtbs check. > > I'll move this to a separate patch. Ok, thanks!
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.