| Message ID | 20240529162958.18081-14-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic | expand |
On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote: > > The Qualcomm PM8008 is an I2C-controlled PMIC containing seven LDO > regulators. > > The driver is based on a driver submitted by Satya Priya, but it has > been cleaned up and 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 the > device-id table. > > 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. ... > +#include <linux/array_size.h> > +#include <linux/bits.h> > +#include <linux/device.h> > +#include <linux/math.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/regulator/driver.h> + types.h + asm/byteorder.h ... > +static int pm8008_regulator_set_voltage_sel(struct regulator_dev *rdev, unsigned int sel) > +{ > + struct pm8008_regulator *preg = rdev_get_drvdata(rdev); > + unsigned int mV; > + __le16 val; > + int ret; > + > + ret = regulator_list_voltage_linear_range(rdev, sel); > + if (ret < 0) > + return ret; > + > + mV = DIV_ROUND_UP(ret, 1000); MILLI from units.h ? > + val = cpu_to_le16(mV); > + ret = regmap_bulk_write(preg->regmap, preg->base + LDO_VSET_LB_REG, > + &val, sizeof(val)); > + if (ret < 0) > + return ret; > + > + return 0; May be written as return regmap_bulk_write(...); > +} > + > +static int pm8008_regulator_get_voltage_sel(struct regulator_dev *rdev) > +{ > + struct pm8008_regulator *preg = rdev_get_drvdata(rdev); > + unsigned int uV; > + __le16 val; > + int ret; > + > + ret = regmap_bulk_read(preg->regmap, preg->base + LDO_VSET_LB_REG, > + &val, sizeof(val)); > + if (ret < 0) > + return ret; > + > + uV = le16_to_cpu(val) * 1000; MILLI ? > + return (uV - preg->desc.min_uV) / preg->desc.uV_step; > +} ... > + rdev = devm_regulator_register(dev, desc, &config); > + if (IS_ERR(rdev)) { > + ret = PTR_ERR(rdev); > + dev_err(dev, "failed to register regulator %s: %d\n", > + desc->name, ret); > + return ret; It's possible to use return dev_err_probe(...); even for non-probe functions. > + } > + }
On Wed, May 29, 2024 at 11:02:57PM +0300, Andy Shevchenko wrote: > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote: > > +#include <linux/array_size.h> > > +#include <linux/bits.h> > > +#include <linux/device.h> > > +#include <linux/math.h> > > +#include <linux/module.h> > > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/regulator/driver.h> > > + types.h This one is already pulled in indirectly and I'm not going to respin for this. > + asm/byteorder.h Already explicitly included in the code you left out. > > +static int pm8008_regulator_set_voltage_sel(struct regulator_dev *rdev, unsigned int sel) > > +{ > > + struct pm8008_regulator *preg = rdev_get_drvdata(rdev); > > + unsigned int mV; > > + __le16 val; > > + int ret; > > + > > + ret = regulator_list_voltage_linear_range(rdev, sel); > > + if (ret < 0) > > + return ret; > > + > > + mV = DIV_ROUND_UP(ret, 1000); > > MILLI from units.h ? Nah. > > + val = cpu_to_le16(mV); > > > + ret = regmap_bulk_write(preg->regmap, preg->base + LDO_VSET_LB_REG, > > + &val, sizeof(val)); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > May be written as > > return regmap_bulk_write(...); I obviously prefer it the way I wrote it. > > + rdev = devm_regulator_register(dev, desc, &config); > > + if (IS_ERR(rdev)) { > > + ret = PTR_ERR(rdev); > > + dev_err(dev, "failed to register regulator %s: %d\n", > > + desc->name, ret); > > + return ret; > > It's possible to use > > return dev_err_probe(...); > > even for non-probe functions. This is a probe function(), but as I've told you repeatedly I'm not going to use dev_err_probe() here. Johan
On Thu, May 30, 2024 at 11:14 AM Johan Hovold <johan@kernel.org> wrote: > On Wed, May 29, 2024 at 11:02:57PM +0300, Andy Shevchenko wrote: > > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote: ... > > > +#include <linux/array_size.h> > > > +#include <linux/bits.h> > > > +#include <linux/device.h> > > > +#include <linux/math.h> > > > +#include <linux/module.h> > > > > > +#include <linux/of.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/regmap.h> > > > +#include <linux/regulator/driver.h> > > > > + types.h > > This one is already pulled in indirectly and I'm not going to respin for > this. > > > + asm/byteorder.h > > Already explicitly included in the code you left out. Is there any guarantee it will be like this? I don't think so. That's why there is an IWYU principle to give more flexibility of reshuffling the (core) headers. And I believe you know that we have way too far dependency hell in the headers in the kernel. Have you seen what Ingo tried to do and what the potential achievements are? ... > > > + rdev = devm_regulator_register(dev, desc, &config); > > > + if (IS_ERR(rdev)) { > > > + ret = PTR_ERR(rdev); > > > + dev_err(dev, "failed to register regulator %s: %d\n", > > > + desc->name, ret); > > > + return ret; > > > > It's possible to use > > > > return dev_err_probe(...); > > > > even for non-probe functions. (this should be "non-probe deferred functions") > This is a probe function(), but as I've told you repeatedly I'm not > going to use dev_err_probe() here. Yeah, I got it, some developers are leaving in the previous decades to make code very verbose for no benefit, no problem.
On Thu, May 30, 2024 at 11:46:12AM +0300, Andy Shevchenko wrote: > On Thu, May 30, 2024 at 11:14 AM Johan Hovold <johan@kernel.org> wrote: > > On Wed, May 29, 2024 at 11:02:57PM +0300, Andy Shevchenko wrote: > > > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote: > > ... > > > > > +#include <linux/array_size.h> > > > > +#include <linux/bits.h> > > > > +#include <linux/device.h> > > > > +#include <linux/math.h> > > > > +#include <linux/module.h> > > > > > > > +#include <linux/of.h> > > > > +#include <linux/platform_device.h> > > > > +#include <linux/regmap.h> > > > > +#include <linux/regulator/driver.h> > > > > > > + types.h > > > > This one is already pulled in indirectly and I'm not going to respin for > > this. > > > > > + asm/byteorder.h > > > > Already explicitly included in the code you left out. > > Is there any guarantee it will be like this? I don't think so. That's > why there is an IWYU principle to give more flexibility of reshuffling > the (core) headers. And I believe you know that we have way too far > dependency hell in the headers in the kernel. Have you seen what Ingo > tried to do and what the potential achievements are? The driver is using cpu_to_le16() from asm/byteorder.h so the __le16 type definition will be pulled in. > > ... > > > > > + rdev = devm_regulator_register(dev, desc, &config); > > > > + if (IS_ERR(rdev)) { > > > > + ret = PTR_ERR(rdev); > > > > + dev_err(dev, "failed to register regulator %s: %d\n", > > > > + desc->name, ret); > > > > + return ret; > > > > > > It's possible to use > > > > > > return dev_err_probe(...); > > > > > > even for non-probe functions. > > (this should be "non-probe deferred functions") > > > This is a probe function(), but as I've told you repeatedly I'm not > > going to use dev_err_probe() here. > > Yeah, I got it, some developers are leaving in the previous decades to > make code very verbose for no benefit, no problem. And some developers write unreadable code just to save a few lines of code. I prefer clarity. Johan
On Wed, May 29, 2024 at 06:29:57PM +0200, Johan Hovold wrote: > The Qualcomm PM8008 is an I2C-controlled PMIC containing seven LDO > regulators. > > The driver is based on a driver submitted by Satya Priya, but it has > been cleaned up and reworked to match the new devicetree binding which > no longer describes each regulator as a separate device. Reviewed-by: Mark Brown <broonie@kernel.org>
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index 7db0a29b5b8d..546533735be8 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 PM8008 PMIC regulators" + depends on MFD_QCOM_PM8008 + help + Select this option to enable support for the voltage regulators in + Qualcomm 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..da017c1969d0 --- /dev/null +++ b/drivers/regulator/qcom-pm8008-regulator.c @@ -0,0 +1,198 @@ +// 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. + * Copyright (c) 2024 Linaro Limited + */ + +#include <linux/array_size.h> +#include <linux/bits.h> +#include <linux/device.h> +#include <linux/math.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/regulator/driver.h> + +#include <asm/byteorder.h> + +#define DEFAULT_VOLTAGE_STEPPER_RATE 38400 + +#define LDO_STEPPER_CTL_REG 0x3b +#define STEP_RATE_MASK GENMASK(1, 0) + +#define LDO_VSET_LB_REG 0x40 + +#define LDO_ENABLE_REG 0x46 +#define ENABLE_BIT BIT(7) + +struct pm8008_regulator { + struct regmap *regmap; + struct regulator_desc desc; + unsigned int base; +}; + +struct pm8008_regulator_data { + const char *name; + const char *supply_name; + unsigned int base; + int min_dropout_uV; + const struct linear_range *voltage_range; +}; + +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 pm8008_reg_data[] = { + { "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_set_voltage_sel(struct regulator_dev *rdev, unsigned int sel) +{ + struct pm8008_regulator *preg = rdev_get_drvdata(rdev); + unsigned int mV; + __le16 val; + int ret; + + ret = regulator_list_voltage_linear_range(rdev, sel); + if (ret < 0) + return ret; + + mV = DIV_ROUND_UP(ret, 1000); + + val = cpu_to_le16(mV); + + ret = regmap_bulk_write(preg->regmap, preg->base + LDO_VSET_LB_REG, + &val, sizeof(val)); + if (ret < 0) + return ret; + + return 0; +} + +static int pm8008_regulator_get_voltage_sel(struct regulator_dev *rdev) +{ + struct pm8008_regulator *preg = rdev_get_drvdata(rdev); + unsigned int uV; + __le16 val; + int ret; + + ret = regmap_bulk_read(preg->regmap, preg->base + LDO_VSET_LB_REG, + &val, sizeof(val)); + if (ret < 0) + return ret; + + uV = le16_to_cpu(val) * 1000; + + return (uV - preg->desc.min_uV) / preg->desc.uV_step; +} + +static const struct regulator_ops pm8008_regulator_ops = { + .list_voltage = regulator_list_voltage_linear, + .set_voltage_sel = pm8008_regulator_set_voltage_sel, + .get_voltage_sel = pm8008_regulator_get_voltage_sel, + .enable = regulator_enable_regmap, + .disable = regulator_disable_regmap, + .is_enabled = regulator_is_enabled_regmap, +}; + +static int pm8008_regulator_probe(struct platform_device *pdev) +{ + const struct pm8008_regulator_data *data; + struct regulator_config config = {}; + struct device *dev = &pdev->dev; + struct pm8008_regulator *preg; + struct regulator_desc *desc; + struct regulator_dev *rdev; + struct regmap *regmap; + unsigned int val; + int ret, i; + + regmap = dev_get_regmap(dev->parent, "secondary"); + if (!regmap) + return -EINVAL; + + for (i = 0; i < ARRAY_SIZE(pm8008_reg_data); i++) { + data = &pm8008_reg_data[i]; + + preg = devm_kzalloc(dev, sizeof(*preg), GFP_KERNEL); + if (!preg) + return -ENOMEM; + + preg->regmap = regmap; + preg->base = data->base; + + desc = &preg->desc; + + desc->name = data->name; + desc->supply_name = data->supply_name; + desc->of_match = data->name; + desc->regulators_node = of_match_ptr("regulators"); + desc->ops = &pm8008_regulator_ops; + desc->type = REGULATOR_VOLTAGE; + desc->owner = THIS_MODULE; + + desc->linear_ranges = data->voltage_range; + desc->n_linear_ranges = 1; + desc->uV_step = desc->linear_ranges[0].step; + desc->min_uV = desc->linear_ranges[0].min; + desc->n_voltages = linear_range_values_in_range(&desc->linear_ranges[0]); + + ret = regmap_read(regmap, preg->base + LDO_STEPPER_CTL_REG, &val); + if (ret < 0) { + dev_err(dev, "failed to read step rate: %d\n", ret); + return ret; + } + val &= STEP_RATE_MASK; + desc->ramp_delay = DEFAULT_VOLTAGE_STEPPER_RATE >> val; + + desc->min_dropout_uV = data->min_dropout_uV; + + desc->enable_reg = preg->base + LDO_ENABLE_REG; + desc->enable_mask = ENABLE_BIT; + + config.dev = dev->parent; + config.driver_data = preg; + config.regmap = regmap; + + rdev = devm_regulator_register(dev, desc, &config); + if (IS_ERR(rdev)) { + ret = PTR_ERR(rdev); + dev_err(dev, "failed to register regulator %s: %d\n", + desc->name, ret); + return ret; + } + } + + return 0; +} + +static const struct platform_device_id pm8008_regulator_id_table[] = { + { "pm8008-regulator" }, + { } +}; +MODULE_DEVICE_TABLE(platform, pm8008_regulator_id_table); + +static struct platform_driver pm8008_regulator_driver = { + .driver = { + .name = "qcom-pm8008-regulator", + }, + .probe = pm8008_regulator_probe, + .id_table = pm8008_regulator_id_table, +}; +module_platform_driver(pm8008_regulator_driver); + +MODULE_DESCRIPTION("Qualcomm PM8008 PMIC regulator driver"); +MODULE_LICENSE("GPL");
The Qualcomm PM8008 is an I2C-controlled PMIC containing seven LDO regulators. The driver is based on a driver submitted by Satya Priya, but it has been cleaned up and 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 the device-id table. 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. Link: https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com Cc: Satya Priya Kakitapalli <quic_skakitap@quicinc.com> Cc: Stephen Boyd <swboyd@chromium.org> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/regulator/Kconfig | 7 + drivers/regulator/Makefile | 1 + drivers/regulator/qcom-pm8008-regulator.c | 198 ++++++++++++++++++++++ 3 files changed, 206 insertions(+) create mode 100644 drivers/regulator/qcom-pm8008-regulator.c