| Message ID | 1649939418-19861-8-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-04-14 05:30:16) > diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c > new file mode 100644 > index 0000000..4375ac5 > --- /dev/null > +++ b/drivers/regulator/qcom-pm8008-regulator.c > @@ -0,0 +1,201 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */ > + [..] > + > +static int pm8008_regulator_probe(struct platform_device *pdev) > +{ > + struct pm8008_data *chip = dev_get_drvdata(pdev->dev.parent); > + struct device *dev = &pdev->dev; > + struct regulator_dev *rdev; > + struct pm8008_regulator *pm8008_reg; > + struct regmap *regmap; > + struct regulator_config reg_config = {}; > + int rc, i; > + unsigned int reg; > + > + regmap = pm8008_get_regmap(chip, PM8008_REGULATORS_SID); Can we use dev_get_regmap(pdev->dev.parent, "regulators") here instead? The benefit to that is we don't tightly couple this driver to the pm8008_data structure or header file. > + if (!regmap) { > + dev_err(dev, "parent regmap is missing\n"); > + return -EINVAL; > + } > + > + for (i = 0; i < ARRAY_SIZE(reg_data); i++) { > + 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[i].base; > + > + /* get slew rate */ > + rc = regmap_bulk_read(pm8008_reg->regmap, > + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1); > + if (rc < 0) { > + dev_err(dev, "failed to read step rate configuration rc=%d\n", rc); > + return rc; > + } > + reg &= STEP_RATE_MASK; > + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg; > + > + pm8008_reg->rdesc.type = REGULATOR_VOLTAGE; > + pm8008_reg->rdesc.ops = &pm8008_regulator_ops; > + pm8008_reg->rdesc.name = reg_data[i].name; > + pm8008_reg->rdesc.supply_name = reg_data[i].supply_name; > + pm8008_reg->rdesc.of_match = reg_data[i].name; > + pm8008_reg->rdesc.uV_step = VSET_STEP_UV; > + pm8008_reg->rdesc.min_uV = reg_data[i].min_uv; > + pm8008_reg->rdesc.n_voltages > + = ((reg_data[i].max_uv - reg_data[i].min_uv) > + / pm8008_reg->rdesc.uV_step) + 1; > + pm8008_reg->rdesc.linear_ranges = reg_data[i].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[i].min_dropout_uv; > + pm8008_reg->voltage_selector = -ENOTRECOVERABLE; > + pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators"); > + > + 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[i].name, rc); > + return rc; > + } > + } > + > + return 0; > +} > + > +static struct platform_driver pm8008_regulator_driver = { > + .driver = { > + .name = "qcom-pm8008-regulator", I'd prefer to use an of_device_id table here. That would let us populate a "qcom,pm8008-regulators" node that had the ldo nodes as children and avoid mfd cells.
On Thu, Apr 14, 2022 at 05:25:49PM -0700, Stephen Boyd wrote: > Quoting Satya Priya (2022-04-14 05:30:16) > > +static struct platform_driver pm8008_regulator_driver = { > > + .driver = { > > + .name = "qcom-pm8008-regulator", > I'd prefer to use an of_device_id table here. That would let us populate > a "qcom,pm8008-regulators" node that had the ldo nodes as children and > avoid mfd cells. That's encoding the current Linux way of splitting up drivers into the DT rather than describing the hardware.
Quoting Mark Brown (2022-04-19 07:55:43) > On Thu, Apr 14, 2022 at 05:25:49PM -0700, Stephen Boyd wrote: > > Quoting Satya Priya (2022-04-14 05:30:16) > > > > +static struct platform_driver pm8008_regulator_driver = { > > > + .driver = { > > > + .name = "qcom-pm8008-regulator", > > > I'd prefer to use an of_device_id table here. That would let us populate > > a "qcom,pm8008-regulators" node that had the ldo nodes as children and > > avoid mfd cells. > > That's encoding the current Linux way of splitting up drivers into the > DT rather than describing the hardware. The DT binding has a subnode of the pm8008@8 node for 'regulators'. There's also a subnode for gpios (see qcom,pm8008-gpio). The gpio node has a reg property, so I'm confused how we can even have the regulators container node at the same level as the gpio node with a reg property. Every node that's a child of pm8008@8 either needs to have a reg property or not. What benefit does it have to not describe secondary i2c addresses as nodes in DT? I think it's necessary because the reset gpio needs to be deasserted before i2c read/write to either address, 8 or 9, will work. Otherwise I don't understand. Having the reset puts us into a small bind though because child nodes sometimes have a reg property and other times don't. This is the current example i2c { pm8008@8 { compatible = "qcom,pm8008"; #address-cells = <1>; #size-cells = <0>; reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>; gpios { compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio"; reg = <0xc000>; ... }; regulators { vdd_l1_l2-supply = <&vreg_s8b_1p2>; ldo1 { regulator-name = "pm8008_l1"; }; ldo2 { regulator-name = "pm8008_l2"; }; }; }; }; What should the final result be? Dropping the regulators node would end up with the same problem where ldo1 has no reg property. Adding a reg property to ldo1 might work, but it would be a register offset inside i2c address 9 while the binding makes it look like a register offset within 9. The binding for the container node could get two address cells, so that the first cell describes the i2c address offset? i2c { pm8008@8 { compatible = "qcom,pm8008"; #address-cells = <2>; #size-cells = <0>; reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>; vdd_l1_l2-supply = <&vreg_s8b_1p2>; gpios@0,c000 { compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio"; reg = <0x0 0xc000>; ... }; ldo1@1,30 { compatible = "qcom,pm8008-regulator"; reg = <0x1 0x30>; regulator-name = "pm8008_l1"; }; ldo2@1,40 { compatible = "qcom,pm8008-regulator"; reg = <0x1 0x40>; regulator-name = "pm8008_l2"; }; }; }; We don't make a node for each gpio so I don't know why we would make a node for each regulator. The above could be changed to have the regulators node and a reg property like this i2c { pm8008@8 { compatible = "qcom,pm8008"; #address-cells = <2>; #size-cells = <0>; reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>; gpios@0,c000 { compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio"; reg = <0x0 0xc000>; ... }; regulators@1,0 { compatible = "qcom,pm8008-regulators"; vdd_l1_l2-supply = <&vreg_s8b_1p2>; reg = <0x1 0x0>; ldo1 { regulator-name = "pm8008_l1"; }; ldo2 { regulator-name = "pm8008_l2"; }; }; }; }; I wonder if there's a mapping table property like i2c-ranges or something like that which could be used to map the i2c dummy to the first reg property. That would be super awesome so that when the platform bus is populated we could match up the regmap for the i2c device to the platform device automatically. By the way, Is there any document on "best practices" for i2c devicetree bindings? We should add details to the document to describe this situation so this can be conveyed faster next time.
On Tue, Apr 19, 2022 at 08:45:47AM -0700, Stephen Boyd wrote: > Quoting Mark Brown (2022-04-19 07:55:43) > > > > +static struct platform_driver pm8008_regulator_driver = { > > > > + .driver = { > > > > + .name = "qcom-pm8008-regulator", > > > I'd prefer to use an of_device_id table here. That would let us populate > > > a "qcom,pm8008-regulators" node that had the ldo nodes as children and > > > avoid mfd cells. > > That's encoding the current Linux way of splitting up drivers into the > > DT rather than describing the hardware. > The DT binding has a subnode of the pm8008@8 node for 'regulators'. > There's also a subnode for gpios (see qcom,pm8008-gpio). The gpio node > has a reg property, so I'm confused how we can even have the regulators > container node at the same level as the gpio node with a reg property. > Every node that's a child of pm8008@8 either needs to have a reg > property or not. That seems unfortunate if it's a limitation of DT :/ > What benefit does it have to not describe secondary i2c addresses as > nodes in DT? I think it's necessary because the reset gpio needs to be This is a platform device, not an I2C device. Converting it to an I2C device and describing the second I2C address as a separate device would be much less of a confusion here (and I suspect may well reflect the underlying physical implementation). I'm mostly concerned about these platform devices. The other option would be to describe each regulator the device supports as a separate IP which does have some hope of being reusable since it reflects the actual IPs here. > deasserted before i2c read/write to either address, 8 or 9, will work. > Otherwise I don't understand. Having the reset puts us into a small bind > though because child nodes sometimes have a reg property and other times > don't. Right, that's an issue and does tie the two I2C addresses of the device into a single thing. > What should the final result be? Dropping the regulators node would end > up with the same problem where ldo1 has no reg property. Adding a reg > property to ldo1 might work, but it would be a register offset inside > i2c address 9 while the binding makes it look like a register offset > within 9. The binding for the container node could get two address > cells, so that the first cell describes the i2c address offset? Yeah, I think we want reg properties on the LDOs. > i2c { > pm8008@8 { > compatible = "qcom,pm8008"; > #address-cells = <2>; > #size-cells = <0>; > reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>; > > vdd_l1_l2-supply = <&vreg_s8b_1p2>; > > gpios@0,c000 { > compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio"; > reg = <0x0 0xc000>; > ... > > }; > > ldo1@1,30 { This looks sensible to me. > We don't make a node for each gpio so I don't know why we would make a > node for each regulator. The above could be changed to have the > regulators node and a reg property like this GPIOs tend to be one IP block with one set of registers that provides a bank of GPIOs. Regulators tend to be a separate IP block for each individual regulator, typically repeated multiple times within a single chip. A binding that describes the regulators within the device should generally be describing these individual regulator IPs rather than just saying "there are some regulators" which doesn't add any information or promote reuse with other PMICs sharing the same IP.
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index a7effc5..ce8522d 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -925,6 +925,15 @@ config REGULATOR_PWM This driver supports PWM controlled voltage regulators. PWM duty cycle can increase or decrease the voltage. +config REGULATOR_QCOM_PM8008 + tristate "Qualcomm Technologies, Inc. PM8008 PMIC regulators" + depends on MFD_QCOM_PM8008 + help + Select this option to get support for the voltage regulators + of Qualcomm Technologies, Inc. PM8008 PMIC chip. PM8008 has 7 LDO + regulators. This driver provides support for basic operations like + set/get voltage and enable/disable. + config REGULATOR_QCOM_RPM tristate "Qualcomm RPM regulator driver" depends on MFD_QCOM_RPM diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 69b5a19..fc045d9 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -101,6 +101,7 @@ obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o +obj-$(CONFIG_REGULATOR_QCOM_PM8008) += qcom-pm8008-regulator.o obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c new file mode 100644 index 0000000..4375ac5 --- /dev/null +++ b/drivers/regulator/qcom-pm8008-regulator.c @@ -0,0 +1,201 @@ +// 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/mfd/qcom_pm8008.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> + +#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) + +#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 pm8008_regulator_data { + 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; +}; + +struct pm8008_regulator { + struct device *dev; + struct regmap *regmap; + struct regulator_desc rdesc; + u16 base; + int step_rate; + int voltage_selector; +}; + +static const struct linear_range nldo_ranges[] = { + REGULATOR_LINEAR_RANGE(528000, 0, 122, 8000), +}; + +static const struct linear_range pldo_ranges[] = { + REGULATOR_LINEAR_RANGE(1504000, 0, 237, 8000), +}; + +static const struct pm8008_regulator_data reg_data[] = { + /* name parent base min_uv max_uv headroom_uv voltage_range */ + { "ldo1", "vdd_l1_l2", 0x4000, 528000, 1504000, 225000, nldo_ranges, }, + { "ldo2", "vdd_l1_l2", 0x4100, 528000, 1504000, 225000, nldo_ranges, }, + { "ldo3", "vdd_l3_l4", 0x4200, 1504000, 3400000, 300000, pldo_ranges, }, + { "ldo4", "vdd_l3_l4", 0x4300, 1504000, 3400000, 300000, pldo_ranges, }, + { "ldo5", "vdd_l5", 0x4400, 1504000, 3400000, 200000, pldo_ranges, }, + { "ldo6", "vdd_l6", 0x4500, 1504000, 3400000, 200000, pldo_ranges, }, + { "ldo7", "vdd_l7", 0x4600, 1504000, 3400000, 200000, pldo_ranges, }, +}; + +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev) +{ + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev); + + return pm8008_reg->voltage_selector; +} + +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg, + int mV) +{ + __le16 vset_raw; + + vset_raw = cpu_to_le16(mV); + + return regmap_bulk_write(pm8008_reg->regmap, + LDO_VSET_LB_REG(pm8008_reg->base), + (const void *)&vset_raw, sizeof(vset_raw)); +} + +static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev, + int old_uV, int new_uv) +{ + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev); + + return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate); +} + +static int pm8008_regulator_set_voltage(struct regulator_dev *rdev, + unsigned int selector) +{ + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev); + int rc, mV; + + /* voltage control register is set with voltage in millivolts */ + mV = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev, selector), + 1000); + if (mV < 0) + return mV; + + rc = pm8008_write_voltage(pm8008_reg, mV); + if (rc < 0) + return rc; + + pm8008_reg->voltage_selector = selector; + dev_dbg(&rdev->dev, "voltage set to %d\n", mV * 1000); + return 0; +} + +static const struct regulator_ops pm8008_regulator_ops = { + .enable = regulator_enable_regmap, + .disable = regulator_disable_regmap, + .is_enabled = regulator_is_enabled_regmap, + .set_voltage_sel = pm8008_regulator_set_voltage, + .get_voltage_sel = pm8008_regulator_get_voltage, + .list_voltage = regulator_list_voltage_linear, + .set_voltage_time = pm8008_regulator_set_voltage_time, +}; + +static int pm8008_regulator_probe(struct platform_device *pdev) +{ + struct pm8008_data *chip = dev_get_drvdata(pdev->dev.parent); + struct device *dev = &pdev->dev; + struct regulator_dev *rdev; + struct pm8008_regulator *pm8008_reg; + struct regmap *regmap; + struct regulator_config reg_config = {}; + int rc, i; + unsigned int reg; + + regmap = pm8008_get_regmap(chip, PM8008_REGULATORS_SID); + if (!regmap) { + dev_err(dev, "parent regmap is missing\n"); + return -EINVAL; + } + + for (i = 0; i < ARRAY_SIZE(reg_data); i++) { + 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[i].base; + + /* get slew rate */ + rc = regmap_bulk_read(pm8008_reg->regmap, + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1); + if (rc < 0) { + dev_err(dev, "failed to read step rate configuration rc=%d\n", rc); + return rc; + } + reg &= STEP_RATE_MASK; + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg; + + pm8008_reg->rdesc.type = REGULATOR_VOLTAGE; + pm8008_reg->rdesc.ops = &pm8008_regulator_ops; + pm8008_reg->rdesc.name = reg_data[i].name; + pm8008_reg->rdesc.supply_name = reg_data[i].supply_name; + pm8008_reg->rdesc.of_match = reg_data[i].name; + pm8008_reg->rdesc.uV_step = VSET_STEP_UV; + pm8008_reg->rdesc.min_uV = reg_data[i].min_uv; + pm8008_reg->rdesc.n_voltages + = ((reg_data[i].max_uv - reg_data[i].min_uv) + / pm8008_reg->rdesc.uV_step) + 1; + pm8008_reg->rdesc.linear_ranges = reg_data[i].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[i].min_dropout_uv; + pm8008_reg->voltage_selector = -ENOTRECOVERABLE; + pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators"); + + 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[i].name, rc); + return rc; + } + } + + return 0; +} + +static struct platform_driver pm8008_regulator_driver = { + .driver = { + .name = "qcom-pm8008-regulator", + }, + .probe = pm8008_regulator_probe, +}; + +module_platform_driver(pm8008_regulator_driver); + +MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver"); +MODULE_LICENSE("GPL");
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> --- Changes in V8: - Changed the regulators_data struct name to pm8008_regulator_data Changes in V9: - Nothing has changed. Changes in V10: - Changed the driver name. - Removed unused header. - Use get_voltage_sel. drivers/regulator/Kconfig | 9 ++ drivers/regulator/Makefile | 1 + drivers/regulator/qcom-pm8008-regulator.c | 201 ++++++++++++++++++++++++++++++ 3 files changed, 211 insertions(+) create mode 100644 drivers/regulator/qcom-pm8008-regulator.c