Message ID | 20240506150830.23709-13-johan+linaro@kernel.org |
---|---|
State | New |
Headers | show |
Series | arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic | expand |
On 5/6/24 17:08, Johan Hovold wrote: > From: Satya Priya <quic_c_skakit@quicinc.com> > > 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/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > Cc: Stephen Boyd <swboyd@chromium.org> > [ johan: rework probe to match new binding, amend commit message and > Kconfig entry] > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- 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 <quic_c_skakit@quicinc.com> > > > > 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/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com > > 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 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> 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)
On Tue, May 07, 2024 at 08:22:34PM +0300, Andy Shevchenko wrote: > On Tue, May 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> 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 <quic_c_skakit@quicinc.com> > > 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/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > Cc: Stephen Boyd <swboyd@chromium.org> > [ johan: rework probe to match new binding, amend commit message and > Kconfig entry] > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > 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 <bryan.odonoghue@linaro.org>
Quoting Johan Hovold (2024-05-06 08:08:29) > From: Satya Priya <quic_c_skakit@quicinc.com> > > 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/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > Cc: Stephen Boyd <swboyd@chromium.org> > [ johan: rework probe to match new binding, amend commit message and > Kconfig entry] > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > 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 <johan@kernel.org> 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 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
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.
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 <johan@kernel.org> 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.
> 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 <quic_c_skakit@quicinc.com> > > > > > > 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/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com > > > > > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> This is my old email which is discontinued, please use <quic_skakitap@quicinc.com> > > > Cc: Stephen Boyd <swboyd@chromium.org> > > > [ johan: rework probe to match new binding, amend commit message and > > > Kconfig entry] > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > --- > > > > 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 <quic_skakitap@quicinc.com> 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.
On 5/14/2024 7:48 PM, Andy Shevchenko wrote: > On Tue, May 14, 2024 at 5:05 PM Satya Priya Kakitapalli > <quic_skakitap@quicinc.com> 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) <quic_skakitap@quicinc.com> wrote: > On 5/14/2024 7:48 PM, Andy Shevchenko wrote: > > On Tue, May 14, 2024 at 5:05 PM Satya Priya Kakitapalli > > <quic_skakitap@quicinc.com> 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.
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 <quic_c_skakit@quicinc.com> >>>> >>>> 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/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com >>>> >>>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > > This is my old email which is discontinued, please use <quic_skakitap@quicinc.com> Please submit an entry to the .mailmap file Konrad
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");