| Message ID | 1644331940-18986-1-git-send-email-quic_c_skakit@quicinc.com |
|---|---|
| Headers | show |
| Series | Add Qualcomm Technologies, Inc. PM8008 regulator driver | expand |
On Tue, Feb 08, 2022 at 08:22:18PM +0530, Satya Priya wrote: > +static int pm8008_regulator_of_parse(struct device_node *node, > + const struct regulator_desc *desc, > + struct regulator_config *config) > +{ > + struct pm8008_regulator *pm8008_reg = config->driver_data; > + int rc; > + unsigned int reg; > + > + /* get slew rate */ > + rc = regmap_bulk_read(pm8008_reg->regmap, > + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1); > + if (rc < 0) { > + dev_err(pm8008_reg->dev, > + "%s: failed to read step rate configuration rc=%d\n", > + pm8008_reg->rdesc.name, rc); > + return rc; > + } > + reg &= STEP_RATE_MASK; > + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg; > + > + return 0; This is not doing any parsing of any DT properties at all, it is just reading a default value back from the hardware. This shouldn't be in the of_parse() callback, it should be done on probe() or something so that if someone adds ACPI support or whatever there's no surprise breakage, and so that we've got this configured even if there's no DT bindings for the specific regulator. > +} > + > +static int pm8008_regulator_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + int id = pdev->id % PM8008_NUM_LDOS; > + struct regulator_dev *rdev; > + struct pm8008_regulator *pm8008_reg; > + struct regmap *regmap; > + struct regulator_config reg_config = {}; > + int rc; > + > + dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1); > + > + regmap = dev_get_regmap(dev->parent, NULL); > + if (!regmap) { > + dev_err(dev, "parent regmap is missing\n"); > + return -EINVAL; > + } > + > + pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL); > + if (!pm8008_reg) > + return -ENOMEM; > + > + pm8008_reg->regmap = regmap; > + pm8008_reg->dev = dev; > + pm8008_reg->base = reg_data[id].base; > + > + pm8008_reg->rdesc.type = REGULATOR_VOLTAGE; > + pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators"); > + pm8008_reg->rdesc.ops = &pm8008_regulator_ops; > + pm8008_reg->rdesc.name = reg_data[id].name; > + pm8008_reg->rdesc.supply_name = reg_data[id].supply_name; > + pm8008_reg->rdesc.of_match = reg_data[id].name; > + pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse; > + pm8008_reg->rdesc.uV_step = VSET_STEP_UV; > + pm8008_reg->rdesc.min_uV = reg_data[id].min_uv; > + pm8008_reg->rdesc.n_voltages > + = ((reg_data[id].max_uv - reg_data[id].min_uv) > + / pm8008_reg->rdesc.uV_step) + 1; > + pm8008_reg->rdesc.linear_ranges = reg_data[id].voltage_range; > + pm8008_reg->rdesc.n_linear_ranges = 1; > + pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base); > + pm8008_reg->rdesc.enable_mask = ENABLE_BIT; > + pm8008_reg->rdesc.min_dropout_uV = reg_data[id].min_dropout_uv; > + > + reg_config.dev = dev->parent; > + reg_config.driver_data = pm8008_reg; > + > + rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, ®_config); > + if (IS_ERR(rdev)) { > + rc = PTR_ERR(rdev); > + dev_err(dev, "%s: failed to register regulator rc=%d\n", > + reg_data[id].name, rc); > + return rc; > + } > + > + return 0; > +} > + > +static struct platform_driver pm8008_regulator_driver = { > + .driver = { > + .name = "qcom,pm8008-regulators", > + }, > + .probe = pm8008_regulator_probe, > +}; > + > +module_platform_driver(pm8008_regulator_driver); > + > +MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver"); > +MODULE_LICENSE("GPL"); > -- > 2.7.4 >
On Tue, 08 Feb 2022 20:22:16 +0530, Satya Priya wrote: > Add regulators node 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> > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > --- > 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. > > .../devicetree/bindings/mfd/qcom,pm8008.yaml | 49 +++++++++++++++++++--- > 1 file changed, 44 insertions(+), 5 deletions(-) > Reviewed-by: Rob Herring <robh@kernel.org>
Quoting Satya Priya (2022-02-08 06:52:19) > Add base DTS file for pm8008 with infra and regulator nodes. > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > --- > Changes in V4: > - This is newly added in V4, to add all the pm8008 common stuff. > > Changes in V5: > - Changed the mfd node names from pm8008_chip to pm8008_infra and > pm8008_ldo to pm8008_regulators as they re more appropriate. > - Changed the compatible for pm8008@9 mfd node to differentiate from > pm8008@8 node in driver. > - Removed compatible for regulators node. > - Removed reg property for LDOs and added in driver. > > arch/arm64/boot/dts/qcom/pm8008.dtsi | 46 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi > > diff --git a/arch/arm64/boot/dts/qcom/pm8008.dtsi b/arch/arm64/boot/dts/qcom/pm8008.dtsi > new file mode 100644 > index 0000000..8e04983 > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi > @@ -0,0 +1,46 @@ > +// SPDX-License-Identifier: BSD-3-Clause > +// Copyright (c) 2021, The Linux Foundation. All rights reserved. > + > +pm8008_infra: pm8008@8 { > + compatible = "qcom,pm8008"; > + reg = <0x8>; > + #address-cells = <1>; > + #size-cells = <0>; > +}; > + > +pm8008_regulators: pm8008@9 { > + compatible = "qcom,pm8008-regulators"; > + reg = <0x9>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + regulators { What is the point of the 'regulators' node? Why can't we just put LDO1 directly underneath the node that has the "qcom,pm8008-regulators" compatible? > + pm8008_l1: LDO1 { > + regulator-name = "pm8008_l1"; > + }; > +
On Tue 08 Feb 08:52 CST 2022, Satya Priya wrote: > diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c [..] > +static int pm8008_regulator_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + int id = pdev->id % PM8008_NUM_LDOS; Why does this driver look completely different from all the other Qualcomm regulator drivers that we already have, and why do you register one platform_device per regulator? The fundamental difference in design makes it hard to maintain and you're wasting quite a bit of memory with the unnecessary platfrom_device objects. Regards, Bjorn
On 2/9/2022 7:48 PM, Mark Brown wrote: > On Tue, Feb 08, 2022 at 08:22:18PM +0530, Satya Priya wrote: > >> +static int pm8008_regulator_of_parse(struct device_node *node, >> + const struct regulator_desc *desc, >> + struct regulator_config *config) >> +{ >> + struct pm8008_regulator *pm8008_reg = config->driver_data; >> + int rc; >> + unsigned int reg; >> + >> + /* get slew rate */ >> + rc = regmap_bulk_read(pm8008_reg->regmap, >> + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1); >> + if (rc < 0) { >> + dev_err(pm8008_reg->dev, >> + "%s: failed to read step rate configuration rc=%d\n", >> + pm8008_reg->rdesc.name, rc); >> + return rc; >> + } >> + reg &= STEP_RATE_MASK; >> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg; >> + >> + return 0; > This is not doing any parsing of any DT properties at all, it is just > reading a default value back from the hardware. This shouldn't be in > the of_parse() callback, it should be done on probe() or something so > that if someone adds ACPI support or whatever there's no surprise > breakage, and so that we've got this configured even if there's no DT > bindings for the specific regulator. Okay, I'll move it to probe. > >> +} >> + >> +static int pm8008_regulator_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + int id = pdev->id % PM8008_NUM_LDOS; >> + struct regulator_dev *rdev; >> + struct pm8008_regulator *pm8008_reg; >> + struct regmap *regmap; >> + struct regulator_config reg_config = {}; >> + int rc; >> + >> + dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1); >> + >> + regmap = dev_get_regmap(dev->parent, NULL); >> + if (!regmap) { >> + dev_err(dev, "parent regmap is missing\n"); >> + return -EINVAL; >> + } >> + >> + pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL); >> + if (!pm8008_reg) >> + return -ENOMEM; >> + >> + pm8008_reg->regmap = regmap; >> + pm8008_reg->dev = dev; >> + pm8008_reg->base = reg_data[id].base; >> + >> + pm8008_reg->rdesc.type = REGULATOR_VOLTAGE; >> + pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators"); >> + pm8008_reg->rdesc.ops = &pm8008_regulator_ops; >> + pm8008_reg->rdesc.name = reg_data[id].name; >> + pm8008_reg->rdesc.supply_name = reg_data[id].supply_name; >> + pm8008_reg->rdesc.of_match = reg_data[id].name; >> + pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse; >> + pm8008_reg->rdesc.uV_step = VSET_STEP_UV; >> + pm8008_reg->rdesc.min_uV = reg_data[id].min_uv; >> + pm8008_reg->rdesc.n_voltages >> + = ((reg_data[id].max_uv - reg_data[id].min_uv) >> + / pm8008_reg->rdesc.uV_step) + 1; >> + pm8008_reg->rdesc.linear_ranges = reg_data[id].voltage_range; >> + pm8008_reg->rdesc.n_linear_ranges = 1; >> + pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base); >> + pm8008_reg->rdesc.enable_mask = ENABLE_BIT; >> + pm8008_reg->rdesc.min_dropout_uV = reg_data[id].min_dropout_uv; >> + >> + reg_config.dev = dev->parent; >> + reg_config.driver_data = pm8008_reg; >> + >> + rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, ®_config); >> + if (IS_ERR(rdev)) { >> + rc = PTR_ERR(rdev); >> + dev_err(dev, "%s: failed to register regulator rc=%d\n", >> + reg_data[id].name, rc); >> + return rc; >> + } >> + >> + return 0; >> +} >> + >> +static struct platform_driver pm8008_regulator_driver = { >> + .driver = { >> + .name = "qcom,pm8008-regulators", >> + }, >> + .probe = pm8008_regulator_probe, >> +}; >> + >> +module_platform_driver(pm8008_regulator_driver); >> + >> +MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.7.4 >>
On 2/10/2022 7:11 AM, Stephen Boyd wrote: > Quoting Satya Priya (2022-02-08 06:52:19) >> Add base DTS file for pm8008 with infra and regulator nodes. >> >> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> >> --- >> Changes in V4: >> - This is newly added in V4, to add all the pm8008 common stuff. >> >> Changes in V5: >> - Changed the mfd node names from pm8008_chip to pm8008_infra and >> pm8008_ldo to pm8008_regulators as they re more appropriate. >> - Changed the compatible for pm8008@9 mfd node to differentiate from >> pm8008@8 node in driver. >> - Removed compatible for regulators node. >> - Removed reg property for LDOs and added in driver. >> >> arch/arm64/boot/dts/qcom/pm8008.dtsi | 46 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi >> >> diff --git a/arch/arm64/boot/dts/qcom/pm8008.dtsi b/arch/arm64/boot/dts/qcom/pm8008.dtsi >> new file mode 100644 >> index 0000000..8e04983 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi >> @@ -0,0 +1,46 @@ >> +// SPDX-License-Identifier: BSD-3-Clause >> +// Copyright (c) 2021, The Linux Foundation. All rights reserved. >> + >> +pm8008_infra: pm8008@8 { >> + compatible = "qcom,pm8008"; >> + reg = <0x8>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> +}; >> + >> +pm8008_regulators: pm8008@9 { >> + compatible = "qcom,pm8008-regulators"; >> + reg = <0x9>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + regulators { > What is the point of the 'regulators' node? Why can't we just put LDO1 > directly underneath the node that has the "qcom,pm8008-regulators" > compatible? It is not mandatory to have it but I think it makes the layout look a little better. And it seems to be commonly used upstream. >> + pm8008_l1: LDO1 { >> + regulator-name = "pm8008_l1"; >> + }; >> +
On 2/11/2022 6:27 AM, Bjorn Andersson wrote: > On Tue 08 Feb 08:52 CST 2022, Satya Priya wrote: >> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c > [..] >> +static int pm8008_regulator_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + int id = pdev->id % PM8008_NUM_LDOS; > Why does this driver look completely different from all the other > Qualcomm regulator drivers that we already have, and why do you register > one platform_device per regulator? Based on Mark's suggestions and the discussion happened here [1], I've changed the design like this. [1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1637314953-4215-3-git-send-email-quic_c_skakit@quicinc.com/ > The fundamental difference in design makes it hard to maintain and > you're wasting quite a bit of memory with the unnecessary > platfrom_device objects. I'm going to change this. I will add only one cell with .name to match with the regulator driver and probe all the regulators using a single loop. That way we don't waste lot of memory by registering multiple regulator platform devices. > Regards, > Bjorn
Hi Satya, It's always nice to see new PMIC drivers :) I just one question after reading your patch - please ignore it if it has already been discussed before - for some reason this version was caught by my filters where the previous versions didn't. It means I do not know the full history. On 2/8/22 16:52, Satya Priya wrote: > 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> > --- snip > + > +static int pm8008_regulator_of_parse(struct device_node *node, > + const struct regulator_desc *desc, > + struct regulator_config *config) > +{ > + struct pm8008_regulator *pm8008_reg = config->driver_data; > + int rc; > + unsigned int reg; > + > + /* get slew rate */ > + rc = regmap_bulk_read(pm8008_reg->regmap, > + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1); > + if (rc < 0) { > + dev_err(pm8008_reg->dev, > + "%s: failed to read step rate configuration rc=%d\n", > + pm8008_reg->rdesc.name, rc); > + return rc; > + } > + reg &= STEP_RATE_MASK; > + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg; > + > + return 0; > +} I wonder why this is done in the of_parse_cb? Could this perhaps be done directly in probe - I don't think this is actually parsing the device_node properties, right? Overall this looks pretty nice to me. Best Regards -- Matti
Hi Matti, Thanks for reviewing the patches! On 2/11/2022 4:31 PM, Matti Vaittinen wrote: > Hi Satya, > > It's always nice to see new PMIC drivers :) I just one question after > reading your patch - please ignore it if it has already been discussed > before - for some reason this version was caught by my filters where > the previous versions didn't. It means I do not know the full history. > On 2/8/22 16:52, Satya Priya wrote: >> 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> >> --- > > snip > >> + >> +static int pm8008_regulator_of_parse(struct device_node *node, >> + const struct regulator_desc *desc, >> + struct regulator_config *config) >> +{ >> + struct pm8008_regulator *pm8008_reg = config->driver_data; >> + int rc; >> + unsigned int reg; >> + >> + /* get slew rate */ >> + rc = regmap_bulk_read(pm8008_reg->regmap, >> + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1); >> + if (rc < 0) { >> + dev_err(pm8008_reg->dev, >> + "%s: failed to read step rate configuration rc=%d\n", >> + pm8008_reg->rdesc.name, rc); >> + return rc; >> + } >> + reg &= STEP_RATE_MASK; >> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg; >> + >> + return 0; >> +} > > I wonder why this is done in the of_parse_cb? Could this perhaps be > done directly in probe - I don't think this is actually parsing the > device_node properties, right? > Right, I will move this part to probe. In the previous version there was some code here which did the DT parsing, now that I removed all that, I should move this to probe. Thanks, Satya Priya > Overall this looks pretty nice to me. > > Best Regards > -- Matti >