From: Satya Priya <[email protected]>
Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
seven LDO regulators. Add a PM8008 regulator driver to support PMIC
regulator management via the regulator framework.
Note that this driver, originally submitted by Satya Priya [1], has been
reworked to match the new devicetree binding which no longer describes
each regulator as a separate device.
This avoids describing internal details like register offsets in the
devicetree and allows for extending the implementation with features
like over-current protection without having to update the binding.
Specifically note that the regulator interrupts are shared between all
regulators.
Note that the secondary regmap is looked up by name and that if the
driver ever needs to be generalised to support regulators provided by
the primary regmap (I2C address) such information could be added to a
driver lookup table matching on the parent compatible.
This also fixes the original implementation, which looked up regulators
by 'regulator-name' property rather than devicetree node name and which
prevented the regulators from being named to match board schematics.
[1] https://lore.kernel.org/r/[email protected]
Signed-off-by: Satya Priya <[email protected]>
Cc: Stephen Boyd <[email protected]>
[ johan: rework probe to match new binding, amend commit message and
Kconfig entry]
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/regulator/Kconfig | 7 +
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-pm8008-regulator.c | 215 ++++++++++++++++++++++
3 files changed, 223 insertions(+)
create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7db0a29b5b8d..d07d034ef1a2 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1027,6 +1027,13 @@ 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 enable support for the voltage regulators in
+ Qualcomm Technologies, Inc. PM8008 PMICs.
+
config REGULATOR_QCOM_REFGEN
tristate "Qualcomm REFGEN regulator driver"
depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 46fb569e6be8..0457b0925b3e 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -112,6 +112,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_REFGEN) += qcom-refgen-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
new file mode 100644
index 000000000000..51f1ce5e043c
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.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)
+
+#define NLDO_MIN_UV 528000
+#define NLDO_MAX_UV 1504000
+
+#define PLDO_MIN_UV 1504000
+#define PLDO_MAX_UV 3400000
+
+struct pm8008_regulator_data {
+ const char *name;
+ const char *supply_name;
+ u16 base;
+ int min_dropout_uv;
+ const struct linear_range *voltage_range;
+};
+
+struct pm8008_regulator {
+ 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 pm8008_regulator_data reg_data[] = {
+ /* name parent base headroom_uv voltage_range */
+ { "ldo1", "vdd_l1_l2", 0x4000, 225000, nldo_ranges, },
+ { "ldo2", "vdd_l1_l2", 0x4100, 225000, nldo_ranges, },
+ { "ldo3", "vdd_l3_l4", 0x4200, 300000, pldo_ranges, },
+ { "ldo4", "vdd_l3_l4", 0x4300, 300000, pldo_ranges, },
+ { "ldo5", "vdd_l5", 0x4400, 200000, pldo_ranges, },
+ { "ldo6", "vdd_l6", 0x4500, 200000, pldo_ranges, },
+ { "ldo7", "vdd_l7", 0x4600, 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 uV;
+
+ regmap_bulk_read(pm8008_reg->regmap,
+ LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
+
+ uV = le16_to_cpu(mV) * 1000;
+ return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
+}
+
+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;
+
+ rc = regulator_list_voltage_linear_range(rdev, selector);
+ if (rc < 0)
+ return rc;
+
+ /* voltage control register is set with voltage in millivolts */
+ mV = DIV_ROUND_UP(rc, 1000);
+
+ rc = pm8008_write_voltage(pm8008_reg, mV);
+ if (rc < 0)
+ return rc;
+
+ 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 regulator_config reg_config = {};
+ struct pm8008_regulator *pm8008_reg;
+ struct device *dev = &pdev->dev;
+ struct regulator_desc *rdesc;
+ struct regulator_dev *rdev;
+ struct regmap *regmap;
+ unsigned int val;
+ int rc, i;
+
+ regmap = dev_get_regmap(dev->parent, "secondary");
+ if (!regmap)
+ 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->base = reg_data[i].base;
+
+ /* get slew rate */
+ rc = regmap_bulk_read(pm8008_reg->regmap,
+ LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
+ if (rc < 0) {
+ dev_err(dev, "failed to read step rate: %d\n", rc);
+ return rc;
+ }
+ val &= STEP_RATE_MASK;
+ pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
+
+ rdesc = &pm8008_reg->rdesc;
+ rdesc->type = REGULATOR_VOLTAGE;
+ rdesc->ops = &pm8008_regulator_ops;
+ rdesc->name = reg_data[i].name;
+ rdesc->supply_name = reg_data[i].supply_name;
+ rdesc->of_match = reg_data[i].name;
+ rdesc->uV_step = VSET_STEP_UV;
+ rdesc->linear_ranges = reg_data[i].voltage_range;
+ rdesc->n_linear_ranges = 1;
+ BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
+ (ARRAY_SIZE(nldo_ranges) != 1));
+
+ if (reg_data[i].voltage_range == nldo_ranges) {
+ rdesc->min_uV = NLDO_MIN_UV;
+ rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
+ } else {
+ rdesc->min_uV = PLDO_MIN_UV;
+ rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
+ }
+
+ rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
+ rdesc->enable_mask = ENABLE_BIT;
+ rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
+ rdesc->regulators_node = of_match_ptr("regulators");
+
+ reg_config.dev = dev->parent;
+ reg_config.driver_data = pm8008_reg;
+ reg_config.regmap = pm8008_reg->regmap;
+
+ rdev = devm_regulator_register(dev, rdesc, ®_config);
+ if (IS_ERR(rdev)) {
+ rc = PTR_ERR(rdev);
+ dev_err(dev, "failed to register regulator %s: %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 Technologies, Inc. PM8008 PMIC Regulator Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:qcom-pm8008-regulator");
--
2.43.2
Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
> From: Satya Priya <[email protected]>
>
> Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> regulator management via the regulator framework.
>
> Note that this driver, originally submitted by Satya Priya [1], has been
> reworked to match the new devicetree binding which no longer describes
> each regulator as a separate device.
>
> This avoids describing internal details like register offsets in the
> devicetree and allows for extending the implementation with features
> like over-current protection without having to update the binding.
>
> Specifically note that the regulator interrupts are shared between all
> regulators.
>
> Note that the secondary regmap is looked up by name and that if the
> driver ever needs to be generalised to support regulators provided by
> the primary regmap (I2C address) such information could be added to a
> driver lookup table matching on the parent compatible.
>
> This also fixes the original implementation, which looked up regulators
> by 'regulator-name' property rather than devicetree node name and which
> prevented the regulators from being named to match board schematics.
> [1] https://lore.kernel.org/r/[email protected]
Make it Link: tag?
Link: URL [1]
..
> [ johan: rework probe to match new binding, amend commit message and
> Kconfig entry]
Wouldn't be better on one line?
..
+ array_size.h
+ bits.h
> +#include <linux/device.h>
> +#include <linux/kernel.h>
What is this header for?
+ math.h
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
+ asm/byteorder.h
..
> +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> +{
> + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> + __le16 mV;
> + int uV;
> +
> + regmap_bulk_read(pm8008_reg->regmap,
> + LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
Why casting?
> + uV = le16_to_cpu(mV) * 1000;
> + return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> +}
> +
> +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> + int mV)
> +{
> + __le16 vset_raw;
> +
> + vset_raw = cpu_to_le16(mV);
Can be joined to a single line.
> + return regmap_bulk_write(pm8008_reg->regmap,
> + LDO_VSET_LB_REG(pm8008_reg->base),
> + (const void *)&vset_raw, sizeof(vset_raw));
Why casting?
> +}
..
> +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;
> +
> + rc = regulator_list_voltage_linear_range(rdev, selector);
> + if (rc < 0)
> + return rc;
> +
> + /* voltage control register is set with voltage in millivolts */
> + mV = DIV_ROUND_UP(rc, 1000);
> + rc = pm8008_write_voltage(pm8008_reg, mV);
> + if (rc < 0)
> + return rc;
> +
> + return 0;
return pm8008_write_voltage(pm8008_reg, mV);
?
> +}
> +
> + regmap = dev_get_regmap(dev->parent, "secondary");
> + if (!regmap)
> + 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->base = reg_data[i].base;
> +
> + /* get slew rate */
> + rc = regmap_bulk_read(pm8008_reg->regmap,
> + LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> + if (rc < 0) {
> + dev_err(dev, "failed to read step rate: %d\n", rc);
> + return rc;
return dev_err_probe(...);
> + }
> + val &= STEP_RATE_MASK;
> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
> + rdesc = &pm8008_reg->rdesc;
> + rdesc->type = REGULATOR_VOLTAGE;
> + rdesc->ops = &pm8008_regulator_ops;
> + rdesc->name = reg_data[i].name;
> + rdesc->supply_name = reg_data[i].supply_name;
> + rdesc->of_match = reg_data[i].name;
> + rdesc->uV_step = VSET_STEP_UV;
> + rdesc->linear_ranges = reg_data[i].voltage_range;
> + rdesc->n_linear_ranges = 1;
> + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> + (ARRAY_SIZE(nldo_ranges) != 1));
> +
> + if (reg_data[i].voltage_range == nldo_ranges) {
> + rdesc->min_uV = NLDO_MIN_UV;
> + rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
> + } else {
> + rdesc->min_uV = PLDO_MIN_UV;
> + rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
> + }
> +
> + rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> + rdesc->enable_mask = ENABLE_BIT;
> + rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
> + rdesc->regulators_node = of_match_ptr("regulators");
> +
> + reg_config.dev = dev->parent;
> + reg_config.driver_data = pm8008_reg;
> + reg_config.regmap = pm8008_reg->regmap;
> +
> + rdev = devm_regulator_register(dev, rdesc, ®_config);
> + if (IS_ERR(rdev)) {
> + rc = PTR_ERR(rdev);
> + dev_err(dev, "failed to register regulator %s: %d\n",
> + reg_data[i].name, rc);
> + return rc;
return dev_err_probe(...);
> + }
> + }
> +
> + return 0;
> +}
..
> +static struct platform_driver pm8008_regulator_driver = {
> + .driver = {
> + .name = "qcom-pm8008-regulator",
> + },
> + .probe = pm8008_regulator_probe,
> +};
> +
Unneeded blank line.
> +module_platform_driver(pm8008_regulator_driver);
..
> +MODULE_ALIAS("platform:qcom-pm8008-regulator");
Use ID table instead.
--
With Best Regards,
Andy Shevchenko
On 5/6/24 17:08, Johan Hovold wrote:
> From: Satya Priya <[email protected]>
>
> Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> regulator management via the regulator framework.
>
> Note that this driver, originally submitted by Satya Priya [1], has been
> reworked to match the new devicetree binding which no longer describes
> each regulator as a separate device.
>
> This avoids describing internal details like register offsets in the
> devicetree and allows for extending the implementation with features
> like over-current protection without having to update the binding.
>
> Specifically note that the regulator interrupts are shared between all
> regulators.
>
> Note that the secondary regmap is looked up by name and that if the
> driver ever needs to be generalised to support regulators provided by
> the primary regmap (I2C address) such information could be added to a
> driver lookup table matching on the parent compatible.
>
> This also fixes the original implementation, which looked up regulators
> by 'regulator-name' property rather than devicetree node name and which
> prevented the regulators from being named to match board schematics.
>
> [1] https://lore.kernel.org/r/[email protected]
>
> Signed-off-by: Satya Priya <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> [ johan: rework probe to match new binding, amend commit message and
> Kconfig entry]
> Signed-off-by: Johan Hovold <[email protected]>
> ---
I'm a bit lukewarm on calling this qcom-pm8008-regulator.. But then
qcom-i2c-regulator or qpnp-i2c-regulator may bite due to being overly
generic.. Would you know whether this code will also be used for e.g.
PM8010?
Konrad
On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
> Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
> > From: Satya Priya <[email protected]>
> >
> > Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> > seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> > regulator management via the regulator framework.
> >
> > Note that this driver, originally submitted by Satya Priya [1], has been
> > reworked to match the new devicetree binding which no longer describes
> > each regulator as a separate device.
> > [1] https://lore.kernel.org/r/[email protected]
>
> Make it Link: tag?
>
> Link: URL [1]
Sure.
> > [ johan: rework probe to match new binding, amend commit message and
> > Kconfig entry]
>
> Wouldn't be better on one line?
Now you're really nit picking. ;) I think I prefer to stay within 72
columns.
> + array_size.h
> + bits.h
Ok.
> > +#include <linux/device.h>
>
> > +#include <linux/kernel.h>
>
> What is this header for?
Probably the ones that are not explicitly included.
> + math.h
>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/driver.h>
>
> + asm/byteorder.h
Ok, thanks.
> > +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> > +{
> > + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> > + __le16 mV;
> > + int uV;
> > +
> > + regmap_bulk_read(pm8008_reg->regmap,
> > + LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
>
> Why casting?
I tried not change things in the v15 from Qualcomm that I based this
on. I couldn't help cleaning up a few things in probe, which I was
touching anyway, but I left it there.
I'll drop the unnecessary cast.
> > + uV = le16_to_cpu(mV) * 1000;
> > + return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> > +}
> > +
> > +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> > + int mV)
> > +{
> > + __le16 vset_raw;
> > +
> > + vset_raw = cpu_to_le16(mV);
>
> Can be joined to a single line.
Sure.
> > + return regmap_bulk_write(pm8008_reg->regmap,
> > + LDO_VSET_LB_REG(pm8008_reg->base),
> > + (const void *)&vset_raw, sizeof(vset_raw));
>
> Why casting?
Same answer as above. Will drop.
> > +}
>
> ...
>
> > +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;
> > +
> > + rc = regulator_list_voltage_linear_range(rdev, selector);
> > + if (rc < 0)
> > + return rc;
> > +
> > + /* voltage control register is set with voltage in millivolts */
> > + mV = DIV_ROUND_UP(rc, 1000);
>
> > + rc = pm8008_write_voltage(pm8008_reg, mV);
> > + if (rc < 0)
> > + return rc;
> > +
> > + return 0;
>
> return pm8008_write_voltage(pm8008_reg, mV);
Possibly, but I tend to prefer explicit error paths.
> > +}
>
> > +
> > + regmap = dev_get_regmap(dev->parent, "secondary");
> > + if (!regmap)
> > + 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->base = reg_data[i].base;
> > +
> > + /* get slew rate */
> > + rc = regmap_bulk_read(pm8008_reg->regmap,
> > + LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> > + if (rc < 0) {
> > + dev_err(dev, "failed to read step rate: %d\n", rc);
> > + return rc;
>
> return dev_err_probe(...);
Nah, regmap won't trigger a probe deferral.
> > +static struct platform_driver pm8008_regulator_driver = {
> > + .driver = {
> > + .name = "qcom-pm8008-regulator",
> > + },
> > + .probe = pm8008_regulator_probe,
> > +};
>
> > +
>
> Unneeded blank line.
I noticed that one too, but such things are up the author to decide.
> > +module_platform_driver(pm8008_regulator_driver);
>
> ...
>
> > +MODULE_ALIAS("platform:qcom-pm8008-regulator");
>
> Use ID table instead.
No, the driver is not using an id-table for matching so the alias is
needed for module auto-loading.
Johan
On Tue, May 07, 2024 at 01:48:30PM +0200, Konrad Dybcio wrote:
> On 5/6/24 17:08, Johan Hovold wrote:
> > From: Satya Priya <[email protected]>
> >
> > Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> > seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> > regulator management via the regulator framework.
> >
> > Note that this driver, originally submitted by Satya Priya [1], has been
> > reworked to match the new devicetree binding which no longer describes
> > each regulator as a separate device.
> >
> > This avoids describing internal details like register offsets in the
> > devicetree and allows for extending the implementation with features
> > like over-current protection without having to update the binding.
> >
> > Specifically note that the regulator interrupts are shared between all
> > regulators.
> >
> > Note that the secondary regmap is looked up by name and that if the
> > driver ever needs to be generalised to support regulators provided by
> > the primary regmap (I2C address) such information could be added to a
> > driver lookup table matching on the parent compatible.
> >
> > This also fixes the original implementation, which looked up regulators
> > by 'regulator-name' property rather than devicetree node name and which
> > prevented the regulators from being named to match board schematics.
> >
> > [1] https://lore.kernel.org/r/[email protected]
> >
> > Signed-off-by: Satya Priya <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > [ johan: rework probe to match new binding, amend commit message and
> > Kconfig entry]
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
>
> I'm a bit lukewarm on calling this qcom-pm8008-regulator.. But then
> qcom-i2c-regulator or qpnp-i2c-regulator may bite due to being overly
> generic.. Would you know whether this code will also be used for e.g.
> PM8010?
Yes, for any sufficiently similar PMICs, including SPMI ones. So
'qpnp-regulator' would be a generic name, but only Qualcomm knows what
PMICs they have and how they are related -- the rest of us is left doing
tedious code forensics to try to make some sense of this.
So just like for compatible strings, letting the first supported PMIC
name the driver makes sense as we don't know when we'll want to add a
second one for another set of devices (and we don't want to call that
one 'qpnp-regulator-2'). On the other hand, these names are now mostly
internal and can more easily be renamed later.
Johan
On Tue, May 7, 2024 at 6:44 PM Johan Hovold <[email protected]> wrote:
> On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
> > Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
..
> > > [ johan: rework probe to match new binding, amend commit message and
> > > Kconfig entry]
> >
> > Wouldn't be better on one line?
>
> Now you're really nit picking. ;) I think I prefer to stay within 72
> columns.
Not really. The tag block is special and the format is rather one
entry per line. This might break some scriptings.
..
> > > +#include <linux/kernel.h>
> >
> > What is this header for?
>
> Probably the ones that are not explicitly included.
Please, remove it, it's a mess nowadays and most of what you need is
available via other headers.
..
> > return dev_err_probe(...);
>
> Nah, regmap won't trigger a probe deferral.
And it doesn't matter. What we gain with dev_err_probe() is:
- special handling of deferred probe
- unified format of messages in ->probe() stage
The second one is encouraged.
..
> > > +MODULE_ALIAS("platform:qcom-pm8008-regulator");
> >
> > Use ID table instead.
>
> No, the driver is not using an id-table for matching so the alias is
> needed for module auto-loading.
Then create one. Added Krzysztof for that. (He is working on dropping
MODULE_ALIAS() in cases like this one)
--
With Best Regards,
Andy Shevchenko
On 07/05/2024 19:22, Andy Shevchenko wrote:
> On Tue, May 7, 2024 at 6:44 PM Johan Hovold <[email protected]> wrote:
>> On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
>>> Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
>
> ...
>
>>>> [ johan: rework probe to match new binding, amend commit message and
>>>> Kconfig entry]
>>>
>>> Wouldn't be better on one line?
>>
>> Now you're really nit picking. ;) I think I prefer to stay within 72
>> columns.
>
> Not really. The tag block is special and the format is rather one
> entry per line. This might break some scriptings.
>
> ...
I think [] can be wrapped, I saw it at least many times and I use as well...
..
> ...
>
>>>> +MODULE_ALIAS("platform:qcom-pm8008-regulator");
>>>
>>> Use ID table instead.
>>
>> No, the driver is not using an id-table for matching so the alias is
>> needed for module auto-loading.
>
> Then create one. Added Krzysztof for that. (He is working on dropping
> MODULE_ALIAS() in cases like this one)
Yeah, please use ID table, since this is a driver (unless I missed
something). Module alias does not scale, leads to stale and duplicated
entries, so should not be used as substitute of ID table. Alias is
suitable for different cases.
Best regards,
Krzysztof
On Tue, May 07, 2024 at 08:22:34PM +0300, Andy Shevchenko wrote:
> On Tue, May 7, 2024 at 6:44 PM Johan Hovold <[email protected]> wrote:
> > > > [ johan: rework probe to match new binding, amend commit message and
> > > > Kconfig entry]
> > > Wouldn't be better on one line?
> > Now you're really nit picking. ;) I think I prefer to stay within 72
> > columns.
> Not really. The tag block is special and the format is rather one
> entry per line. This might break some scriptings.
No, really - the above is absolutely fine, random notes in the middle of
things are reasonably common and scripting that can't cope with them is
going to encounter a bunch of problems. This stuff needs to be readable
by humans and this is just a stylistic preference on your part.
On 06/05/2024 16:08, Johan Hovold wrote:
> From: Satya Priya <[email protected]>
>
> Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> regulator management via the regulator framework.
>
> Note that this driver, originally submitted by Satya Priya [1], has been
> reworked to match the new devicetree binding which no longer describes
> each regulator as a separate device.
>
> This avoids describing internal details like register offsets in the
> devicetree and allows for extending the implementation with features
> like over-current protection without having to update the binding.
>
> Specifically note that the regulator interrupts are shared between all
> regulators.
>
> Note that the secondary regmap is looked up by name and that if the
> driver ever needs to be generalised to support regulators provided by
> the primary regmap (I2C address) such information could be added to a
> driver lookup table matching on the parent compatible.
>
> This also fixes the original implementation, which looked up regulators
> by 'regulator-name' property rather than devicetree node name and which
> prevented the regulators from being named to match board schematics.
>
> [1] https://lore.kernel.org/r/[email protected]
>
> Signed-off-by: Satya Priya <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> [ johan: rework probe to match new binding, amend commit message and
> Kconfig entry]
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/regulator/Kconfig | 7 +
> drivers/regulator/Makefile | 1 +
> drivers/regulator/qcom-pm8008-regulator.c | 215 ++++++++++++++++++++++
> 3 files changed, 223 insertions(+)
> create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 7db0a29b5b8d..d07d034ef1a2 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1027,6 +1027,13 @@ 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 enable support for the voltage regulators in
> + Qualcomm Technologies, Inc. PM8008 PMICs.
> +
> config REGULATOR_QCOM_REFGEN
> tristate "Qualcomm REFGEN regulator driver"
> depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 46fb569e6be8..0457b0925b3e 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -112,6 +112,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_REFGEN) += qcom-refgen-regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 000000000000..51f1ce5e043c
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.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)
> +
> +#define NLDO_MIN_UV 528000
> +#define NLDO_MAX_UV 1504000
> +
> +#define PLDO_MIN_UV 1504000
> +#define PLDO_MAX_UV 3400000
> +
> +struct pm8008_regulator_data {
> + const char *name;
> + const char *supply_name;
> + u16 base;
> + int min_dropout_uv;
> + const struct linear_range *voltage_range;
> +};
> +
> +struct pm8008_regulator {
> + 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 pm8008_regulator_data reg_data[] = {
> + /* name parent base headroom_uv voltage_range */
> + { "ldo1", "vdd_l1_l2", 0x4000, 225000, nldo_ranges, },
> + { "ldo2", "vdd_l1_l2", 0x4100, 225000, nldo_ranges, },
> + { "ldo3", "vdd_l3_l4", 0x4200, 300000, pldo_ranges, },
> + { "ldo4", "vdd_l3_l4", 0x4300, 300000, pldo_ranges, },
> + { "ldo5", "vdd_l5", 0x4400, 200000, pldo_ranges, },
> + { "ldo6", "vdd_l6", 0x4500, 200000, pldo_ranges, },
> + { "ldo7", "vdd_l7", 0x4600, 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 uV;
> +
> + regmap_bulk_read(pm8008_reg->regmap,
> + LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
> +
> + uV = le16_to_cpu(mV) * 1000;
> + return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> +}
> +
> +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;
> +
> + rc = regulator_list_voltage_linear_range(rdev, selector);
> + if (rc < 0)
> + return rc;
> +
> + /* voltage control register is set with voltage in millivolts */
> + mV = DIV_ROUND_UP(rc, 1000);
> +
> + rc = pm8008_write_voltage(pm8008_reg, mV);
> + if (rc < 0)
> + return rc;
> +
> + 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 regulator_config reg_config = {};
> + struct pm8008_regulator *pm8008_reg;
> + struct device *dev = &pdev->dev;
> + struct regulator_desc *rdesc;
> + struct regulator_dev *rdev;
> + struct regmap *regmap;
> + unsigned int val;
> + int rc, i;
> +
> + regmap = dev_get_regmap(dev->parent, "secondary");
> + if (!regmap)
> + 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->base = reg_data[i].base;
> +
> + /* get slew rate */
> + rc = regmap_bulk_read(pm8008_reg->regmap,
> + LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> + if (rc < 0) {
> + dev_err(dev, "failed to read step rate: %d\n", rc);
> + return rc;
> + }
> + val &= STEP_RATE_MASK;
> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
> +
> + rdesc = &pm8008_reg->rdesc;
> + rdesc->type = REGULATOR_VOLTAGE;
> + rdesc->ops = &pm8008_regulator_ops;
> + rdesc->name = reg_data[i].name;
> + rdesc->supply_name = reg_data[i].supply_name;
> + rdesc->of_match = reg_data[i].name;
> + rdesc->uV_step = VSET_STEP_UV;
> + rdesc->linear_ranges = reg_data[i].voltage_range;
> + rdesc->n_linear_ranges = 1;
> + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> + (ARRAY_SIZE(nldo_ranges) != 1));
> +
> + if (reg_data[i].voltage_range == nldo_ranges) {
> + rdesc->min_uV = NLDO_MIN_UV;
> + rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
> + } else {
> + rdesc->min_uV = PLDO_MIN_UV;
> + rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
> + }
> +
> + rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> + rdesc->enable_mask = ENABLE_BIT;
> + rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
> + rdesc->regulators_node = of_match_ptr("regulators");
> +
> + reg_config.dev = dev->parent;
> + reg_config.driver_data = pm8008_reg;
> + reg_config.regmap = pm8008_reg->regmap;
> +
> + rdev = devm_regulator_register(dev, rdesc, ®_config);
> + if (IS_ERR(rdev)) {
> + rc = PTR_ERR(rdev);
> + dev_err(dev, "failed to register regulator %s: %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 Technologies, Inc. PM8008 PMIC Regulator Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:qcom-pm8008-regulator");
Tested-by: Bryan O'Donoghue <[email protected]>
Quoting Johan Hovold (2024-05-06 08:08:29)
> From: Satya Priya <[email protected]>
>
> Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> regulator management via the regulator framework.
>
> Note that this driver, originally submitted by Satya Priya [1], has been
> reworked to match the new devicetree binding which no longer describes
> each regulator as a separate device.
>
> This avoids describing internal details like register offsets in the
> devicetree and allows for extending the implementation with features
> like over-current protection without having to update the binding.
Thanks. I had to remember this topic.
>
> Specifically note that the regulator interrupts are shared between all
> regulators.
>
> Note that the secondary regmap is looked up by name and that if the
> driver ever needs to be generalised to support regulators provided by
> the primary regmap (I2C address) such information could be added to a
> driver lookup table matching on the parent compatible.
>
> This also fixes the original implementation, which looked up regulators
> by 'regulator-name' property rather than devicetree node name and which
> prevented the regulators from being named to match board schematics.
>
> [1] https://lore.kernel.org/r/[email protected]
>
> Signed-off-by: Satya Priya <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> [ johan: rework probe to match new binding, amend commit message and
> Kconfig entry]
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 000000000000..51f1ce5e043c
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.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)
Include bits.h?
> +
> +#define NLDO_MIN_UV 528000
> +#define NLDO_MAX_UV 1504000
> +
> +#define PLDO_MIN_UV 1504000
> +#define PLDO_MAX_UV 3400000
> +
> +struct pm8008_regulator_data {
> + const char *name;
> + const char *supply_name;
> + u16 base;
> + int min_dropout_uv;
> + const struct linear_range *voltage_range;
> +};
> +
> +struct pm8008_regulator {
> + struct regmap *regmap;
> + struct regulator_desc rdesc;
> + u16 base;
> + int step_rate;
Is struct regulator_desc::vsel_step usable for this? If not, can it be
unsigned?
> +};
> +
> +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 headroom_uv voltage_range */
> + { "ldo1", "vdd_l1_l2", 0x4000, 225000, nldo_ranges, },
> + { "ldo2", "vdd_l1_l2", 0x4100, 225000, nldo_ranges, },
> + { "ldo3", "vdd_l3_l4", 0x4200, 300000, pldo_ranges, },
> + { "ldo4", "vdd_l3_l4", 0x4300, 300000, pldo_ranges, },
> + { "ldo5", "vdd_l5", 0x4400, 200000, pldo_ranges, },
> + { "ldo6", "vdd_l6", 0x4500, 200000, pldo_ranges, },
> + { "ldo7", "vdd_l7", 0x4600, 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 uV;
Can this be unsigned? Doubt we have negative voltage and this would
match rdesc.min_uV type.
> +
> + regmap_bulk_read(pm8008_reg->regmap,
> + LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
Is struct regulator_desc::vsel_reg usable for this?
> +
> + uV = le16_to_cpu(mV) * 1000;
> + return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> +}
> +
> +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));
Is the cast to please sparse?
> +}
> +
> +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;
> +
> + rc = regulator_list_voltage_linear_range(rdev, selector);
> + if (rc < 0)
> + return rc;
> +
> + /* voltage control register is set with voltage in millivolts */
> + mV = DIV_ROUND_UP(rc, 1000);
> +
> + rc = pm8008_write_voltage(pm8008_reg, mV);
> + if (rc < 0)
> + return rc;
> +
> + return 0;
Can be shorter to save lines
return pm8008_write_voltage(pm8008_reg, mV);
> +}
> +
> +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 regulator_config reg_config = {};
> + struct pm8008_regulator *pm8008_reg;
> + struct device *dev = &pdev->dev;
> + struct regulator_desc *rdesc;
> + struct regulator_dev *rdev;
> + struct regmap *regmap;
> + unsigned int val;
> + int rc, i;
> +
> + regmap = dev_get_regmap(dev->parent, "secondary");
> + if (!regmap)
> + 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->base = reg_data[i].base;
> +
> + /* get slew rate */
> + rc = regmap_bulk_read(pm8008_reg->regmap,
> + LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> + if (rc < 0) {
> + dev_err(dev, "failed to read step rate: %d\n", rc);
Is it step rate or slew rate? The comment doesn't agree with the error
message.
> + return rc;
> + }
> + val &= STEP_RATE_MASK;
> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
> +
> + rdesc = &pm8008_reg->rdesc;
> + rdesc->type = REGULATOR_VOLTAGE;
> + rdesc->ops = &pm8008_regulator_ops;
> + rdesc->name = reg_data[i].name;
> + rdesc->supply_name = reg_data[i].supply_name;
> + rdesc->of_match = reg_data[i].name;
> + rdesc->uV_step = VSET_STEP_UV;
> + rdesc->linear_ranges = reg_data[i].voltage_range;
> + rdesc->n_linear_ranges = 1;
> + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
This should be an && not || right?
> + (ARRAY_SIZE(nldo_ranges) != 1));
> +
> + if (reg_data[i].voltage_range == nldo_ranges) {
> + rdesc->min_uV = NLDO_MIN_UV;
> + rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
> + } else {
> + rdesc->min_uV = PLDO_MIN_UV;
> + rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
> + }
> +
> + rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> + rdesc->enable_mask = ENABLE_BIT;
> + rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
> + rdesc->regulators_node = of_match_ptr("regulators");
> +
> + reg_config.dev = dev->parent;
> + reg_config.driver_data = pm8008_reg;
> + reg_config.regmap = pm8008_reg->regmap;
> +
> + rdev = devm_regulator_register(dev, rdesc, ®_config);
> + if (IS_ERR(rdev)) {
> + rc = PTR_ERR(rdev);
> + dev_err(dev, "failed to register regulator %s: %d\n",
> + reg_data[i].name, rc);
> + return rc;
Could be return dev_err_probe() to simplify.
On Tue, May 07, 2024 at 08:22:34PM +0300, Andy Shevchenko wrote:
> On Tue, May 7, 2024 at 6:44 PM Johan Hovold <[email protected]> wrote:
> > On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
> > > Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
> > > > [ johan: rework probe to match new binding, amend commit message and
> > > > Kconfig entry]
> > >
> > > Wouldn't be better on one line?
> >
> > Now you're really nit picking. ;) I think I prefer to stay within 72
> > columns.
>
> Not really. The tag block is special and the format is rather one
> entry per line. This might break some scriptings.
This is not a tag, and using line breaks here is perfectly fine.
> > > return dev_err_probe(...);
> >
> > Nah, regmap won't trigger a probe deferral.
>
> And it doesn't matter. What we gain with dev_err_probe() is:
> - special handling of deferred probe
> - unified format of messages in ->probe() stage
>
> The second one is encouraged.
I don't care about your personal preferences.
Johan
On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote:
> On 07/05/2024 19:22, Andy Shevchenko wrote:
> > On Tue, May 7, 2024 at 6:44 PM Johan Hovold <[email protected]> wrote:
> >> On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
> >>> Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
> >>>> +MODULE_ALIAS("platform:qcom-pm8008-regulator");
> >>>
> >>> Use ID table instead.
> >>
> >> No, the driver is not using an id-table for matching so the alias is
> >> needed for module auto-loading.
> >
> > Then create one. Added Krzysztof for that. (He is working on dropping
> > MODULE_ALIAS() in cases like this one)
>
> Yeah, please use ID table, since this is a driver (unless I missed
> something). Module alias does not scale, leads to stale and duplicated
> entries, so should not be used as substitute of ID table. Alias is
> suitable for different cases.
There's no scalability issue here. If the driver uses driver name
matching then there will always be exactly one alias needed.
That said, we may use a platform device id table instead of matching on
parent compatible if subdrivers are going to be reused by multiple
devices. I'll consider that.
Johan
On Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd wrote:
> Quoting Johan Hovold (2024-05-06 08:08:29)
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.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)
>
> Include bits.h?
Sure.
I wanted to avoid changing Qualcomm's v15 driver too much and
essentially submitted it unchanged except for the probe rework. I'll
take closer look at things like this for v2.
> > +struct pm8008_regulator {
> > + struct regmap *regmap;
> > + struct regulator_desc rdesc;
> > + u16 base;
> > + int step_rate;
>
> Is struct regulator_desc::vsel_step usable for this? If not, can it be
> unsigned?
Not sure, I'll take a look when respinning.
> > +};
> > +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> > +{
> > + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> > + __le16 mV;
> > + int uV;
>
> Can this be unsigned? Doubt we have negative voltage and this would
> match rdesc.min_uV type.
Makes sense.
> > +
> > + regmap_bulk_read(pm8008_reg->regmap,
> > + LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
>
> Is struct regulator_desc::vsel_reg usable for this?
Will look into that.
> > +
> > + uV = le16_to_cpu(mV) * 1000;
> > + return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> > +}
> > +
> > +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));
>
> Is the cast to please sparse?
No idea, I think it's just a stylistic preference that can be dropped.
> > +}
> > +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;
> > +
> > + rc = regulator_list_voltage_linear_range(rdev, selector);
> > + if (rc < 0)
> > + return rc;
> > +
> > + /* voltage control register is set with voltage in millivolts */
> > + mV = DIV_ROUND_UP(rc, 1000);
> > +
> > + rc = pm8008_write_voltage(pm8008_reg, mV);
> > + if (rc < 0)
> > + return rc;
> > +
> > + return 0;
>
> Can be shorter to save lines
>
> return pm8008_write_voltage(pm8008_reg, mV);
Possibly, but I tend to prefer explicit error paths (e.g. for symmetry).
> > +}
> > +static int pm8008_regulator_probe(struct platform_device *pdev)
> > +{
> > + struct regulator_config reg_config = {};
> > + struct pm8008_regulator *pm8008_reg;
> > + struct device *dev = &pdev->dev;
> > + struct regulator_desc *rdesc;
> > + struct regulator_dev *rdev;
> > + struct regmap *regmap;
> > + unsigned int val;
> > + int rc, i;
> > +
> > + regmap = dev_get_regmap(dev->parent, "secondary");
> > + if (!regmap)
> > + 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->base = reg_data[i].base;
> > +
> > + /* get slew rate */
> > + rc = regmap_bulk_read(pm8008_reg->regmap,
> > + LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> > + if (rc < 0) {
> > + dev_err(dev, "failed to read step rate: %d\n", rc);
>
> Is it step rate or slew rate? The comment doesn't agree with the error
> message.
Noticed that too, can update the comment.
> > + return rc;
> > + }
> > + val &= STEP_RATE_MASK;
> > + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
> > +
> > + rdesc = &pm8008_reg->rdesc;
> > + rdesc->type = REGULATOR_VOLTAGE;
> > + rdesc->ops = &pm8008_regulator_ops;
> > + rdesc->name = reg_data[i].name;
> > + rdesc->supply_name = reg_data[i].supply_name;
> > + rdesc->of_match = reg_data[i].name;
> > + rdesc->uV_step = VSET_STEP_UV;
> > + rdesc->linear_ranges = reg_data[i].voltage_range;
> > + rdesc->n_linear_ranges = 1;
> > + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
>
> This should be an && not || right?
No, I think this is correct as it stands if the intention is to prevent
anyone from extending either pldo_ranges or nldo_ranges.
> > + (ARRAY_SIZE(nldo_ranges) != 1));
> > +
> > + if (reg_data[i].voltage_range == nldo_ranges) {
> > + rdesc->min_uV = NLDO_MIN_UV;
> > + rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
> > + } else {
> > + rdesc->min_uV = PLDO_MIN_UV;
> > + rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
> > + }
> > +
> > + rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> > + rdesc->enable_mask = ENABLE_BIT;
> > + rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
> > + rdesc->regulators_node = of_match_ptr("regulators");
> > +
> > + reg_config.dev = dev->parent;
> > + reg_config.driver_data = pm8008_reg;
> > + reg_config.regmap = pm8008_reg->regmap;
> > +
> > + rdev = devm_regulator_register(dev, rdesc, ®_config);
> > + if (IS_ERR(rdev)) {
> > + rc = PTR_ERR(rdev);
> > + dev_err(dev, "failed to register regulator %s: %d\n",
> > + reg_data[i].name, rc);
> > + return rc;
>
> Could be return dev_err_probe() to simplify.
Possibly, but I think I prefer not using it when there is nothing that
can trigger a probe deferral.
Johan
On 09/05/2024 10:57, Johan Hovold wrote:
> On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote:
>> On 07/05/2024 19:22, Andy Shevchenko wrote:
>>> On Tue, May 7, 2024 at 6:44 PM Johan Hovold <[email protected]> wrote:
>>>> On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
>>>>> Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
>
>>>>>> +MODULE_ALIAS("platform:qcom-pm8008-regulator");
>>>>>
>>>>> Use ID table instead.
>>>>
>>>> No, the driver is not using an id-table for matching so the alias is
>>>> needed for module auto-loading.
>>>
>>> Then create one. Added Krzysztof for that. (He is working on dropping
>>> MODULE_ALIAS() in cases like this one)
>>
>> Yeah, please use ID table, since this is a driver (unless I missed
>> something). Module alias does not scale, leads to stale and duplicated
>> entries, so should not be used as substitute of ID table. Alias is
>> suitable for different cases.
>
> There's no scalability issue here. If the driver uses driver name
> matching then there will always be exactly one alias needed.
And then we add one more ID with driver data and how does it scale?
There is a way to make drivers uniform, standard and easy to read. Why
doing some other way? What is the benefit of the alias comparing to
regular module ID table?
Best regards,
Krzysztof
Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
> Quoting Johan Hovold (2024-05-06 08:08:29)
..
> > + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
>
> This should be an && not || right?
> > + (ARRAY_SIZE(nldo_ranges) != 1));
In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
better to have a static_assert() near to one of those arrays.
--
With Best Regards,
Andy Shevchenko
On Thu, May 09, 2024 at 03:07:02PM +0300, Andy Shevchenko wrote:
> Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
> > Quoting Johan Hovold (2024-05-06 08:08:29)
>
> ...
>
> > > + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> >
> > This should be an && not || right?
>
> > > + (ARRAY_SIZE(nldo_ranges) != 1));
>
> In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
> better to have a static_assert() near to one of those arrays.
I think the reason it is placed here is that the above line reads:
rdesc->n_linear_ranges = 1;
and that would need to change if anyone expands the arrays.
Johan
On Thu, May 09, 2024 at 12:48:18PM +0200, Krzysztof Kozlowski wrote:
> On 09/05/2024 10:57, Johan Hovold wrote:
> > On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote:
> >> On 07/05/2024 19:22, Andy Shevchenko wrote:
> >> Yeah, please use ID table, since this is a driver (unless I missed
> >> something). Module alias does not scale, leads to stale and duplicated
> >> entries, so should not be used as substitute of ID table. Alias is
> >> suitable for different cases.
> >
> > There's no scalability issue here. If the driver uses driver name
> > matching then there will always be exactly one alias needed.
>
> And then we add one more ID with driver data and how does it scale?
That's what I wrote in the part of my reply that you left out. If a
driver is going to be used for multiple devices, then a module id table
makes sense, but there is no need to go around adding redundant tables
just for the sake of it when a simple alias will do.
Johan
Thu, May 09, 2024 at 10:53:02AM +0200, Johan Hovold kirjoitti:
> On Tue, May 07, 2024 at 08:22:34PM +0300, Andy Shevchenko wrote:
> > On Tue, May 7, 2024 at 6:44 PM Johan Hovold <[email protected]> wrote:
> > > On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
> > > > Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
..
> > > > return dev_err_probe(...);
> > >
> > > Nah, regmap won't trigger a probe deferral.
> >
> > And it doesn't matter. What we gain with dev_err_probe() is:
> > - special handling of deferred probe
> > - unified format of messages in ->probe() stage
> >
> > The second one is encouraged.
>
> I don't care about your personal preferences.
Did I say it's mine personal preference.
Or should I put it as preference of several (majority?) maintainers?
Whatever, it's up to Lee.
--
With Best Regards,
Andy Shevchenko
> On Tue, May 07, 2024 at 01:48:30PM +0200, Konrad Dybcio wrote:
> > On 5/6/24 17:08, Johan Hovold wrote:
> > > From: Satya Priya <[email protected]>
> > >
> > > Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> > > seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> > > regulator management via the regulator framework.
> > >
> > > Note that this driver, originally submitted by Satya Priya [1], has been
> > > reworked to match the new devicetree binding which no longer describes
> > > each regulator as a separate device.
> > >
> > > This avoids describing internal details like register offsets in the
> > > devicetree and allows for extending the implementation with features
> > > like over-current protection without having to update the binding.
> > >
> > > Specifically note that the regulator interrupts are shared between all
> > > regulators.
> > >
> > > Note that the secondary regmap is looked up by name and that if the
> > > driver ever needs to be generalised to support regulators provided by
> > > the primary regmap (I2C address) such information could be added to a
> > > driver lookup table matching on the parent compatible.
> > >
> > > This also fixes the original implementation, which looked up regulators
> > > by 'regulator-name' property rather than devicetree node name and which
> > > prevented the regulators from being named to match board schematics.
> > >
> > > [1] https://lore.kernel.org/r/[email protected]
> > >
> > > Signed-off-by: Satya Priya <[email protected]>
This is my old email which is discontinued, please use <[email protected]>
> > > Cc: Stephen Boyd <[email protected]>
> > > [ johan: rework probe to match new binding, amend commit message and
> > > Kconfig entry]
> > > Signed-off-by: Johan Hovold <[email protected]>
> > > ---
> >
> > I'm a bit lukewarm on calling this qcom-pm8008-regulator.. But then
> > qcom-i2c-regulator or qpnp-i2c-regulator may bite due to being overly
> > generic.. Would you know whether this code will also be used for e.g.
> > PM8010?
>
> Yes, for any sufficiently similar PMICs, including SPMI ones. So
> 'qpnp-regulator' would be a generic name, but only Qualcomm knows what
> PMICs they have and how they are related -- the rest of us is left doing
> tedious code forensics to try to make some sense of this.
>
> So just like for compatible strings, letting the first supported PMIC
> name the driver makes sense as we don't know when we'll want to add a
> second one for another set of devices (and we don't want to call that
> one 'qpnp-regulator-2'). On the other hand, these names are now mostly
> internal and can more easily be renamed later.
There is a PMIC called PM8010 which also is implemented over I2C, which could use the same pm8008 regulator driver.
Hence it is better to use device_id instead of a MODULE_ALIAS().
> On Thu, May 09, 2024 at 03:07:02PM +0300, Andy Shevchenko wrote:
> > Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
> > > Quoting Johan Hovold (2024-05-06 08:08:29)
> >
> > ...
> >
> > > > + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> > >
> > > This should be an && not || right?
> >
> > > > + (ARRAY_SIZE(nldo_ranges) != 1));
> >
> > In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
> > better to have a static_assert() near to one of those arrays.
>
> I think the reason it is placed here is that the above line reads:
>
> rdesc->n_linear_ranges = 1;
>
> and that would need to change if anyone expands the arrays.
Correct. static_assert() cannot be used in the middle of code here, it can only be used at the declarations part which doesn't serve the purpose.
So, BUILD_BUG_ON is the only way to go here.
On Tue, May 14, 2024 at 5:05 PM Satya Priya Kakitapalli
<[email protected]> wrote:
> > On Thu, May 09, 2024 at 03:07:02PM +0300, Andy Shevchenko wrote:
> > > Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
> > > > Quoting Johan Hovold (2024-05-06 08:08:29)
..
> > > > > + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> > > >
> > > > This should be an && not || right?
> > >
> > > > > + (ARRAY_SIZE(nldo_ranges) != 1));
> > >
> > > In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
> > > better to have a static_assert() near to one of those arrays.
> >
> > I think the reason it is placed here is that the above line reads:
> >
> > rdesc->n_linear_ranges = 1;
> >
> > and that would need to change if anyone expands the arrays.
>
> Correct. static_assert() cannot be used in the middle of code here, it can only be used at the declarations part which doesn't serve the purpose.
I didn't get this. The ARRAY_SIZE():s are defined at compile time
globally. How does this prevent from using static_assert()?
> So, BUILD_BUG_ON is the only way to go here.
I don't think so.
--
With Best Regards,
Andy Shevchenko
On 5/14/2024 7:48 PM, Andy Shevchenko wrote:
> On Tue, May 14, 2024 at 5:05 PM Satya Priya Kakitapalli
> <[email protected]> wrote:
>>> On Thu, May 09, 2024 at 03:07:02PM +0300, Andy Shevchenko wrote:
>>>> Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
>>>>> Quoting Johan Hovold (2024-05-06 08:08:29)
> ...
>
>>>>>> + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
>>>>> This should be an && not || right?
>>>>>> + (ARRAY_SIZE(nldo_ranges) != 1));
>>>> In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
>>>> better to have a static_assert() near to one of those arrays.
>>> I think the reason it is placed here is that the above line reads:
>>>
>>> rdesc->n_linear_ranges = 1;
>>>
>>> and that would need to change if anyone expands the arrays.
>> Correct. static_assert() cannot be used in the middle of code here, it can only be used at the declarations part which doesn't serve the purpose.
> I didn't get this. The ARRAY_SIZE():s are defined at compile time
> globally. How does this prevent from using static_assert()?
The reason we added it here is to make sure the nlod_ranges and
pldo_ranges doesn't become larger, and we forget updating the
n_linear_ranges. Adding static_assert here is not feasible so adding a
BUILD_BUG_ON at this point makes sure the n_linear_ranges is proper.
>> So, BUILD_BUG_ON is the only way to go here.
> I don't think so.
>
On Tue, May 14, 2024 at 6:05 PM Satya Priya Kakitapalli (Temp)
<[email protected]> wrote:
> On 5/14/2024 7:48 PM, Andy Shevchenko wrote:
> > On Tue, May 14, 2024 at 5:05 PM Satya Priya Kakitapalli
> > <[email protected]> wrote:
> >>> On Thu, May 09, 2024 at 03:07:02PM +0300, Andy Shevchenko wrote:
> >>>> Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
> >>>>> Quoting Johan Hovold (2024-05-06 08:08:29)
..
> >>>>>> + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> >>>>> This should be an && not || right?
> >>>>>> + (ARRAY_SIZE(nldo_ranges) != 1));
> >>>> In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
> >>>> better to have a static_assert() near to one of those arrays.
> >>> I think the reason it is placed here is that the above line reads:
> >>>
> >>> rdesc->n_linear_ranges = 1;
> >>>
> >>> and that would need to change if anyone expands the arrays.
> >> Correct. static_assert() cannot be used in the middle of code here, it can only be used at the declarations part which doesn't serve the purpose.
> > I didn't get this. The ARRAY_SIZE():s are defined at compile time
> > globally. How does this prevent from using static_assert()?
> The reason we added it here is to make sure the nlod_ranges and
> pldo_ranges doesn't become larger, and we forget updating the
> n_linear_ranges.
> Adding static_assert here is not feasible so adding a
> BUILD_BUG_ON at this point makes sure the n_linear_ranges is proper.
No, static_assert() will do _exactly_ the same with better error
reporting and location, but what you are trying to say is that the
location is chosen to be near to the n_liner_ranges assignment which
happens at runtime, that's why it can't be used as an argument to
BUILD_BUG_ON(). Based on this discussion I think the comment is
missing before BUILD_BUG_ON() to explain the semantics of 1 and all
that was just said.
> >> So, BUILD_BUG_ON is the only way to go here.
> > I don't think so.
As i said.
--
With Best Regards,
Andy Shevchenko
On 5/14/24 15:43, Satya Priya Kakitapalli wrote:
>> On Tue, May 07, 2024 at 01:48:30PM +0200, Konrad Dybcio wrote:
>>> On 5/6/24 17:08, Johan Hovold wrote:
>>>> From: Satya Priya <[email protected]>
>>>>
>>>> Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
>>>> seven LDO regulators. Add a PM8008 regulator driver to support PMIC
>>>> regulator management via the regulator framework.
>>>>
>>>> Note that this driver, originally submitted by Satya Priya [1], has been
>>>> reworked to match the new devicetree binding which no longer describes
>>>> each regulator as a separate device.
>>>>
>>>> This avoids describing internal details like register offsets in the
>>>> devicetree and allows for extending the implementation with features
>>>> like over-current protection without having to update the binding.
>>>>
>>>> Specifically note that the regulator interrupts are shared between all
>>>> regulators.
>>>>
>>>> Note that the secondary regmap is looked up by name and that if the
>>>> driver ever needs to be generalised to support regulators provided by
>>>> the primary regmap (I2C address) such information could be added to a
>>>> driver lookup table matching on the parent compatible.
>>>>
>>>> This also fixes the original implementation, which looked up regulators
>>>> by 'regulator-name' property rather than devicetree node name and which
>>>> prevented the regulators from being named to match board schematics.
>>>>
>>>> [1] https://lore.kernel.org/r/[email protected]
>>>>
>>>> Signed-off-by: Satya Priya <[email protected]>
>
> This is my old email which is discontinued, please use <[email protected]>
Please submit an entry to the .mailmap file
Konrad
On 09/05/2024 14:26, Johan Hovold wrote:
> On Thu, May 09, 2024 at 12:48:18PM +0200, Krzysztof Kozlowski wrote:
>> On 09/05/2024 10:57, Johan Hovold wrote:
>>> On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote:
>>>> On 07/05/2024 19:22, Andy Shevchenko wrote:
>
>>>> Yeah, please use ID table, since this is a driver (unless I missed
>>>> something). Module alias does not scale, leads to stale and duplicated
>>>> entries, so should not be used as substitute of ID table. Alias is
>>>> suitable for different cases.
>>>
>>> There's no scalability issue here. If the driver uses driver name
>>> matching then there will always be exactly one alias needed.
>>
>> And then we add one more ID with driver data and how does it scale?
>
> That's what I wrote in the part of my reply that you left out. If a
> driver is going to be used for multiple devices, then a module id table
> makes sense, but there is no need to go around adding redundant tables
> just for the sake of it when a simple alias will do.
>
I still in general prefer ID tables, because I saw many times people
copy existing code while not understanding above subtleties thus they
just keep multiplying MODULE_ALIAS, but I understand your explanation
and it is reasonable.
FWIW:
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Thu, May 09, 2024 at 11:10:41AM +0200, Johan Hovold wrote:
> On Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd wrote:
> > Quoting Johan Hovold (2024-05-06 08:08:29)
> > > +struct pm8008_regulator {
> > > + struct regmap *regmap;
> > > + struct regulator_desc rdesc;
> > > + u16 base;
> > > + int step_rate;
> >
> > Is struct regulator_desc::vsel_step usable for this? If not, can it be
> > unsigned?
>
> Not sure, I'll take a look when respinning.
No, vsel_step is unrelated to this, which is really a slew rate.
I've reworked the driver and dropped this field in favour of
regulator_desc::ramp_delay.
> > > +};
> > > +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> > > +{
> > > + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> > > + __le16 mV;
> > > + int uV;
> > > +
> > > + regmap_bulk_read(pm8008_reg->regmap,
> > > + LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
> >
> > Is struct regulator_desc::vsel_reg usable for this?
>
> Will look into that.
I don't think vsel_reg can be used here as the voltage is set using two
registers (LSB and MSB).
> > > +
> > > + uV = le16_to_cpu(mV) * 1000;
> > > + return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> > > +}
Johan
On Tue, May 14, 2024 at 07:13:17PM +0530, Satya Priya Kakitapalli wrote:
> > On Tue, May 07, 2024 at 01:48:30PM +0200, Konrad Dybcio wrote:
> > > On 5/6/24 17:08, Johan Hovold wrote:
> > > > From: Satya Priya <[email protected]>
> > > >
> > > > Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> > > > seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> > > > regulator management via the regulator framework.
> > > >
> > > > Note that this driver, originally submitted by Satya Priya [1], has been
> > > > reworked to match the new devicetree binding which no longer describes
> > > > each regulator as a separate device.
> > > > [1] https://lore.kernel.org/r/[email protected]
> > > >
> > > > Signed-off-by: Satya Priya <[email protected]>
>
> This is my old email which is discontinued, please use <[email protected]>
I've cleaned up and reworked the driver for v2 and changed the
authorship to myself in the process, but I'll make sure to CC your new
address when submitting.
You should add an alias as Konrad suggested as you apparently have
commits that use your old address.
> > > > Cc: Stephen Boyd <[email protected]>
> > > > [ johan: rework probe to match new binding, amend commit message and
> > > > Kconfig entry]
> > > > Signed-off-by: Johan Hovold <[email protected]>
> > > > ---
> > >
> > > I'm a bit lukewarm on calling this qcom-pm8008-regulator.. But then
> > > qcom-i2c-regulator or qpnp-i2c-regulator may bite due to being overly
> > > generic.. Would you know whether this code will also be used for e.g.
> > > PM8010?
> >
> > Yes, for any sufficiently similar PMICs, including SPMI ones. So
> > 'qpnp-regulator' would be a generic name, but only Qualcomm knows what
> > PMICs they have and how they are related -- the rest of us is left doing
> > tedious code forensics to try to make some sense of this.
> >
> > So just like for compatible strings, letting the first supported PMIC
> > name the driver makes sense as we don't know when we'll want to add a
> > second one for another set of devices (and we don't want to call that
> > one 'qpnp-regulator-2'). On the other hand, these names are now mostly
> > internal and can more easily be renamed later.
>
> There is a PMIC called PM8010 which also is implemented over I2C,
> which could use the same pm8008 regulator driver.
> Hence it is better to use device_id instead of a MODULE_ALIAS().
Right, I've had PM8010 in mind all along, but it's hard to tell whether
it will be using the same (sub)driver until code is submitted since you
guys (Qualcomm) don't publish any documentation.
I've changed the regulator (and GPIO) drivers to use platform device id
matching for now either way.
Johan