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 <[email protected]>
---
Changes in V2:
As per Mark's comments
- Using regmap helpers for regulator enable/disable and is_enabled APIs
- Changed pr_err to dev_err wherever possible.
- Removed init_voltage property as it is not used.
- Removed if check for registering LDOs
- Other minor changes.
Changes in V3:
As per Stephen's comments,
- Removed unused includes
- Removed PM8008_MAX_LDO macro.
- Removed pm8008_read/write APIs, using regmap_bulk_read/write APIs
- Using le16_to_cpu/cpu_to_le16 APIs in pm8008_regulator_get/set_voltage
- Consolidated all probe related functions into single probe function.
- Added of_parse_cb call back and removed regulator-name matching loop.
- Fixed other minor nits.
Changes in V4:
- Removed unused members like rdev and of_node from pm8008_regulator struct.
- Replaced set_voltage with set_voltage_sel
- Removed init_data configuration as it is not needed.
- Removed few other unused assignments from probe
Changes in V5:
- Removed Compatible string.
- Changed the probe function accordingly to probe LDOs using mfd driver.
- Added max headrooms for LDOs and removed the part reading min-dropout from DT.
- Added base reg values in the regulator_data struct instead of reading it from DT.
drivers/regulator/Kconfig | 9 ++
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-pm8008-regulator.c | 234 ++++++++++++++++++++++++++++++
3 files changed, 244 insertions(+)
create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 22503e4..4372ad7 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 2e1b087..9ab3ad7 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..86043b4
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,234 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
+
+#include <linux/device.h>
+#include <linux/interrupt.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 STARTUP_DELAY_USEC 20
+#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_STATUS1_REG(base) ((base) + 0x08)
+#define VREG_READY_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)
+
+#define PM8008_NUM_LDOS 7
+
+struct 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;
+};
+
+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 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);
+ __le16 mV;
+ int rc;
+
+ rc = regmap_bulk_read(pm8008_reg->regmap,
+ LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
+ if (rc < 0) {
+ dev_err(&rdev->dev, "failed to read regulator voltage rc=%d\n", rc);
+ return rc;
+ }
+
+ return le16_to_cpu(mV) * 1000;
+}
+
+static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
+ int mV)
+{
+ int rc;
+ u16 vset_raw;
+
+ vset_raw = cpu_to_le16(mV);
+
+ rc = regmap_bulk_write(pm8008_reg->regmap,
+ LDO_VSET_LB_REG(pm8008_reg->base),
+ (const void *)&vset_raw, sizeof(vset_raw));
+ if (rc < 0) {
+ dev_err(pm8008_reg->dev, "failed to write voltage rc=%d\n", rc);
+ return rc;
+ }
+
+ return 0;
+}
+
+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;
+
+ 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 = pm8008_regulator_get_voltage,
+ .list_voltage = regulator_list_voltage_linear,
+ .set_voltage_time = pm8008_regulator_set_voltage_time,
+};
+
+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;
+}
+
+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, 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
>
Quoting Satya Priya (2022-02-08 06:52:18)
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 0000000..86043b4
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,234 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
> +
> +#include <linux/device.h>
> +#include <linux/interrupt.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 STARTUP_DELAY_USEC 20
> +#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_STATUS1_REG(base) ((base) + 0x08)
> +#define VREG_READY_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)
> +
> +#define PM8008_NUM_LDOS 7
> +
> +struct 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;
> +};
> +
> +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 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);
> + __le16 mV;
> + int rc;
> +
> + rc = regmap_bulk_read(pm8008_reg->regmap,
> + LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
> + if (rc < 0) {
> + dev_err(&rdev->dev, "failed to read regulator voltage rc=%d\n", rc);
> + return rc;
> + }
> +
> + return le16_to_cpu(mV) * 1000;
> +}
> +
> +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> + int mV)
> +{
> + int rc;
> + u16 vset_raw;
> +
> + vset_raw = cpu_to_le16(mV);
sparse should complain here that an le16 is degrading to a u16. Please
make vset_raw an __le16 as well.
> +
> + rc = regmap_bulk_write(pm8008_reg->regmap,
> + LDO_VSET_LB_REG(pm8008_reg->base),
> + (const void *)&vset_raw, sizeof(vset_raw));
> + if (rc < 0) {
> + dev_err(pm8008_reg->dev, "failed to write voltage rc=%d\n", rc);
Do we really need this? It could spam the logs in theory. We have
tracepoints on regmap that could be used to figure out that some
read/write failed. I'd like to see a plain
return regmap_bulk_write(...)
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +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);
Aren't there linear range APIs that can avoid any div roundups here?
> + if (mV < 0)
> + return mV;
> +
> + rc = pm8008_write_voltage(pm8008_reg, mV);
> + if (rc < 0)
> + return rc;
> +
> + 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 = pm8008_regulator_get_voltage,
> + .list_voltage = regulator_list_voltage_linear,
> + .set_voltage_time = pm8008_regulator_set_voltage_time,
> +};
> +
> +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;
> +}
> +
> +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);
Why can't we probe one regulators (plural) platform device instead of 8
regulator platform devices? A 'struct device' isn't exactly small and
it would be simpler to probe all the regulators in a single loop.
> +
> + regmap = dev_get_regmap(dev->parent, NULL);
> + if (!regmap) {
> + dev_err(dev, "parent regmap is missing\n");
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 <[email protected]>
> ---
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
--
The Linux Kernel guy at ROHM Semiconductors
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~ this year is the year of a signature writers block ~~
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 <[email protected]>
>> ---
>
> 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
>
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/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/[email protected]/
> 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