| Message ID | 20200508204200.13481-4-sumit.semwal@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Qualcomm labibb regulator driver | expand |
On Sat, May 09, 2020 at 02:11:59AM +0530, Sumit Semwal wrote: > + ret = regmap_bulk_read(reg->regmap, reg->base + > + REG_LABIBB_STATUS1, &val, 1); > + if (ret < 0) { > + dev_err(reg->dev, "Read register failed ret = %d\n", ret); > + return ret; > + } Why a bulk read of a single register? > +static int _check_enabled_with_retries(struct regulator_dev *rdev, > + int retries, int enabled) > +{ This is not retrying, this is polling to see if the regulator actually enabled. > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev) > +{ > + ret = _check_enabled_with_retries(rdev, retries, 1); > + if (ret < 0) { > + dev_err(reg->dev, "retries exhausted: enable %s regulator\n", > + reg->desc.name); > + return ret; > + } If this is useful factor it out into a helper or the core, other devices also have status bits saying if the regulator is enabled. It looks like this may be mainly trying to open code something like enable_time, with possibly some issues where the time taken to enable varies a lot. > + if (ret) > + return 0; > + > + > + dev_err(reg->dev, "Can't enable %s\n", reg->desc.name); > + return -EINVAL; Return the actual error code (the logic here is quite convoluted). > + ret = regulator_disable_regmap(rdev); > + > + if (ret < 0) { You have lots of blank lines between operations and checking their return codes? > + ret = _check_enabled_with_retries(rdev, retries, 0); > + if (ret < 0) { > + dev_err(reg->dev, "retries exhausted: disable %s regulator\n", > + reg->desc.name); > + return ret; > + } Similarly to the enable path, but is this one about off_on_delay rather than enable_time? > + if (reg_data->type == QCOM_LAB_TYPE) { > + reg = &labibb->lab; > + reg->desc.enable_mask = LAB_ENABLE_CTL_MASK; > + } else { > + reg = &labibb->ibb; > + reg->desc.enable_mask = IBB_ENABLE_CTL_MASK; > + } Write a switch statement so this is extensible.
On Fri 08 May 13:41 PDT 2020, Sumit Semwal wrote: > From: Nisha Kumari <nishakumari@codeaurora.org> > > Qualcomm platforms have LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost) > Regulators, labibb for short, which are used as power supply for lowercase Regulators > LCD Mode displays. > > This patch adds labibb regulator driver for pmi8998 pmic, found on Uppercase PMIC > SDM845 platforms. > > Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org> > Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org> > > -- > v2: sumits: reworked the driver for more common code, and addressed > review comments from v1. This includes merging regulator_ops into > one, and allowing for future labibb variations. > --- > drivers/regulator/Kconfig | 10 + > drivers/regulator/Makefile | 1 + > drivers/regulator/qcom-labibb-regulator.c | 288 ++++++++++++++++++++++ > 3 files changed, 299 insertions(+) > create mode 100644 drivers/regulator/qcom-labibb-regulator.c > > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index f4b72cb098ef..58704a9fd05d 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -1167,5 +1167,15 @@ config REGULATOR_WM8994 > This driver provides support for the voltage regulators on the > WM8994 CODEC. > > +config REGULATOR_QCOM_LABIBB > + tristate "QCOM LAB/IBB regulator support" > + depends on SPMI || COMPILE_TEST > + help > + This driver supports Qualcomm's LAB/IBB regulators present on the > + Qualcomm's PMIC chip pmi8998. QCOM LAB and IBB are SPMI > + based PMIC implementations. LAB can be used as positive > + boost regulator and IBB can be used as a negative boost regulator > + for LCD display panel. > + > endif > > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index 6610ee001d9a..5b313786c0e8 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -87,6 +87,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o > obj-$(CONFIG_REGULATOR_MT6358) += mt6358-regulator.o > obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o > obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o > +obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o > obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o > obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o > obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o > diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c > new file mode 100644 > index 000000000000..a9dc7c060375 > --- /dev/null > +++ b/drivers/regulator/qcom-labibb-regulator.c > @@ -0,0 +1,288 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2019, The Linux Foundation. All rights reserved. > + > +#include <linux/module.h> > +#include <linux/of_irq.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/of_regulator.h> > + > +#define REG_PERPH_TYPE 0x04 > +#define QCOM_LAB_TYPE 0x24 > +#define QCOM_IBB_TYPE 0x20 > + > +#define REG_LABIBB_STATUS1 0x08 > +#define REG_LABIBB_ENABLE_CTL 0x46 > +#define LABIBB_STATUS1_VREG_OK_BIT BIT(7) > +#define LABIBB_CONTROL_ENABLE BIT(7) > + > +#define LAB_ENABLE_CTL_MASK BIT(7) > +#define IBB_ENABLE_CTL_MASK (BIT(7) | BIT(6)) > + > +#define POWER_DELAY 8000 > + > +struct labibb_regulator { > + struct regulator_desc desc; > + struct device *dev; > + struct regmap *regmap; > + struct regulator_dev *rdev; > + u16 base; > + u8 type; > +}; > + > +struct qcom_labibb { You pretty much use this as a local variable within probe, and then you use labibb_regulator in runtime. Perhaps you can just drop it? > + struct device *dev; > + struct regmap *regmap; > + struct labibb_regulator lab; > + struct labibb_regulator ibb; > +}; > + > +struct labibb_regulator_data { > + u16 base; > + const char *name; > + const char *irq_name; > + u8 type; > +}; > + > +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev) > +{ > + int ret; > + u8 val; > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > + > + ret = regmap_bulk_read(reg->regmap, reg->base + > + REG_LABIBB_STATUS1, &val, 1); > + if (ret < 0) { > + dev_err(reg->dev, "Read register failed ret = %d\n", ret); > + return ret; > + } > + > + if (val & LABIBB_STATUS1_VREG_OK_BIT) > + return 1; > + else > + return 0; return !!(val & LABIBB_STATUS1_VREG_OK_BIT); > +} > + > +static int _check_enabled_with_retries(struct regulator_dev *rdev, > + int retries, int enabled) > +{ > + int ret; > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > + > + while (retries--) { Mark's suggestion of extending _regulator_enable_delay() to support polling is_enable() seems reasonable. The only complication I can see is that code path currently doesn't have any expectations of the regulator not being operational at the end - this seems to offer that possibility. So some care needs to be taken there. But doing that would allow you to use regulator_enable_regmap() as your .enable function directly and you can drop qcom_labibb_regulator_enable() > + /* Wait for a small period before checking REG_LABIBB_STATUS1 */ > + usleep_range(POWER_DELAY, POWER_DELAY + 200); > + > + ret = qcom_labibb_regulator_is_enabled(rdev); > + > + if (ret < 0) { > + dev_err(reg->dev, "Can't read %s regulator status\n", > + reg->desc.name); > + return ret; > + } > + > + if (ret == enabled) > + return ret; > + > + } > + > + return -EINVAL; > +} > + > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev) > +{ > + int ret, retries = 10; > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > + > + ret = regulator_enable_regmap(rdev); > + > + if (ret < 0) { > + dev_err(reg->dev, "Write failed: enable %s regulator\n", > + reg->desc.name); > + return ret; > + } > + > + ret = _check_enabled_with_retries(rdev, retries, 1); > + if (ret < 0) { > + dev_err(reg->dev, "retries exhausted: enable %s regulator\n", > + reg->desc.name); > + return ret; > + } > + > + if (ret) > + return 0; This looks weird; check for timeout, check for errors and then return success. > + > + > + dev_err(reg->dev, "Can't enable %s\n", reg->desc.name); > + return -EINVAL; > +} > + > +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev) > +{ > + int ret, retries = 2; > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > + > + ret = regulator_disable_regmap(rdev); > + > + if (ret < 0) { > + dev_err(reg->dev, "Write failed: disable %s regulator\n", > + reg->desc.name); > + return ret; > + } > + > + ret = _check_enabled_with_retries(rdev, retries, 0); I don't think we care about waiting for the regulator to turning off, might be nice to ensure that an off/on cycle really is allowed to take its time though. So specifying an desc->off_on_delay of 8200 * 2 (the worst case usleep_range() * retries) should take care of this. And then you should be able to just use regulator_disable_regmap() as your .disable function. > + if (ret < 0) { > + dev_err(reg->dev, "retries exhausted: disable %s regulator\n", > + reg->desc.name); > + return ret; > + } > + > + if (!ret) > + return 0; > + > + dev_err(reg->dev, "Can't disable %s\n", reg->desc.name); > + return -EINVAL; > +} > + > +static struct regulator_ops qcom_labibb_ops = { > + .enable = qcom_labibb_regulator_enable, > + .disable = qcom_labibb_regulator_disable, > + .is_enabled = qcom_labibb_regulator_is_enabled, > +}; > + > +static int register_labibb_regulator(struct qcom_labibb *labibb, > + const struct labibb_regulator_data *reg_data, > + struct device_node *of_node) > +{ > + int ret; > + struct labibb_regulator *reg; > + struct regulator_config cfg = {}; > + > + if (reg_data->type == QCOM_LAB_TYPE) { > + reg = &labibb->lab; > + reg->desc.enable_mask = LAB_ENABLE_CTL_MASK; > + } else { > + reg = &labibb->ibb; > + reg->desc.enable_mask = IBB_ENABLE_CTL_MASK; > + } > + > + reg->dev = labibb->dev; > + reg->base = reg_data->base; > + reg->type = reg_data->type; > + reg->regmap = labibb->regmap; > + reg->desc.enable_reg = reg->base + REG_LABIBB_ENABLE_CTL; > + reg->desc.enable_val = LABIBB_CONTROL_ENABLE; > + reg->desc.of_match = reg_data->name; > + reg->desc.name = reg_data->name; > + reg->desc.owner = THIS_MODULE; > + reg->desc.type = REGULATOR_VOLTAGE; > + reg->desc.ops = &qcom_labibb_ops; > + > + cfg.dev = labibb->dev; > + cfg.driver_data = reg; > + cfg.regmap = labibb->regmap; > + cfg.of_node = of_node; > + > + reg->rdev = devm_regulator_register(labibb->dev, ®->desc, > + &cfg); Do you really need to wrap this line? > + if (IS_ERR(reg->rdev)) { > + ret = PTR_ERR(reg->rdev); > + dev_err(labibb->dev, > + "unable to register %s regulator\n", reg_data->name); > + return ret; > + } > + return 0; return PTR_ERR_OR_ZERO(reg->rdev); > +} > + > +static const struct labibb_regulator_data pmi8998_labibb_data[] = { > + {0xde00, "lab", "lab-sc-err", QCOM_LAB_TYPE}, Please shorten the interrupt to just "sc-err". > + {0xdc00, "ibb", "ibb-sc-err", QCOM_IBB_TYPE}, > + { }, > +}; > + > +static const struct of_device_id qcom_labibb_match[] = { > + { .compatible = "qcom,pmi8998-lab-ibb", .data = &pmi8998_labibb_data}, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, qcom_labibb_match); > + > +static int qcom_labibb_regulator_probe(struct platform_device *pdev) > +{ > + struct qcom_labibb *labibb; > + struct device_node *child; > + const struct of_device_id *match; > + const struct labibb_regulator_data *reg; > + u8 type; > + int ret; > + > + labibb = devm_kzalloc(&pdev->dev, sizeof(*labibb), GFP_KERNEL); > + if (!labibb) > + return -ENOMEM; > + > + labibb->regmap = dev_get_regmap(pdev->dev.parent, NULL); > + if (!labibb->regmap) { > + dev_err(&pdev->dev, "Couldn't get parent's regmap\n"); > + return -ENODEV; > + } > + > + labibb->dev = &pdev->dev; > + > + match = of_match_device(qcom_labibb_match, &pdev->dev); > + if (!match) > + return -ENODEV; > + > + for (reg = match->data; reg->name; reg++) { > + child = of_get_child_by_name(pdev->dev.of_node, reg->name); > + > + /* TODO: This validates if the type of regulator is indeed > + * what's mentioned in DT. > + * I'm not sure if this is needed, but we'll keep it for now. > + */ > + ret = regmap_bulk_read(labibb->regmap, > + reg->base + REG_PERPH_TYPE, > + &type, 1); > + if (ret < 0) { > + dev_err(labibb->dev, > + "Peripheral type read failed ret=%d\n", > + ret); > + return -EINVAL; > + } > + > + if ((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) { > + dev_err(labibb->dev, > + "qcom_labibb: unknown peripheral type\n"); > + return -EINVAL; > + } else if (type != reg->type) { > + dev_err(labibb->dev, > + "qcom_labibb: type read %x doesn't match DT %x\n", > + type, reg->type); > + return -EINVAL; > + } > + > + ret = register_labibb_regulator(labibb, reg, child); > + if (ret < 0) { > + dev_err(&pdev->dev, > + "qcom_labibb: error registering %s regulator: %d\n", > + child->full_name, ret); You already printed in register_labibb_regulator() so no need to print again. > + return ret; > + } > + } > + > + dev_set_drvdata(&pdev->dev, labibb); rdev_get_drvdata() returns cfg.driver_data, so you don't need this one. Regards, Bjorn > + return 0; > +} > + > +static struct platform_driver qcom_labibb_regulator_driver = { > + .driver = { > + .name = "qcom-lab-ibb-regulator", > + .of_match_table = qcom_labibb_match, > + }, > + .probe = qcom_labibb_regulator_probe, > +}; > +module_platform_driver(qcom_labibb_regulator_driver); > + > +MODULE_DESCRIPTION("Qualcomm labibb driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.26.2 >
On Mon, May 11, 2020 at 07:15:09PM -0700, Bjorn Andersson wrote: > On Fri 08 May 13:41 PDT 2020, Sumit Semwal wrote: > > + int ret; > > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > > + > > + while (retries--) { > Mark's suggestion of extending _regulator_enable_delay() to support > polling is_enable() seems reasonable. > The only complication I can see is that code path currently doesn't have > any expectations of the regulator not being operational at the end - > this seems to offer that possibility. So some care needs to be taken > there. Are we expecting that to happen in normal operation? Generally this is a pretty serious problem. In any caser if we're adding checks of status we'd need an error return if the status doesn't show the regulator is on after some reasonable time.
Hello Mark, Thank you for your review comments! On Mon, 11 May 2020 at 16:09, Mark Brown <broonie@kernel.org> wrote: > > On Sat, May 09, 2020 at 02:11:59AM +0530, Sumit Semwal wrote: > > > + ret = regmap_bulk_read(reg->regmap, reg->base + > > + REG_LABIBB_STATUS1, &val, 1); > > + if (ret < 0) { > > + dev_err(reg->dev, "Read register failed ret = %d\n", ret); > > + return ret; > > + } > > Why a bulk read of a single register? Right, will change. > > > +static int _check_enabled_with_retries(struct regulator_dev *rdev, > > + int retries, int enabled) > > +{ > > This is not retrying, this is polling to see if the regulator actually > enabled. Yes, will update accordingly. > > > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev) > > +{ > > > + ret = _check_enabled_with_retries(rdev, retries, 1); > > + if (ret < 0) { > > + dev_err(reg->dev, "retries exhausted: enable %s regulator\n", > > + reg->desc.name); > > + return ret; > > + } > > If this is useful factor it out into a helper or the core, other devices > also have status bits saying if the regulator is enabled. It looks like > this may be mainly trying to open code something like enable_time, with > possibly some issues where the time taken to enable varies a lot. > Makes sense; I am not terribly familiar with the regulator core and helpers, so let me look and refactor accordingly. > > + if (ret) > > + return 0; > > + > > + > > + dev_err(reg->dev, "Can't enable %s\n", reg->desc.name); > > + return -EINVAL; > > Return the actual error code (the logic here is quite convoluted). Will try to simplify. > > > + ret = regulator_disable_regmap(rdev); > > + > > + if (ret < 0) { > > You have lots of blank lines between operations and checking their > return codes? > will correct that. > > + ret = _check_enabled_with_retries(rdev, retries, 0); > > + if (ret < 0) { > > + dev_err(reg->dev, "retries exhausted: disable %s regulator\n", > > + reg->desc.name); > > + return ret; > > + } > > Similarly to the enable path, but is this one about off_on_delay rather > than enable_time? Got it. Let me look deeper. > > > + if (reg_data->type == QCOM_LAB_TYPE) { > > + reg = &labibb->lab; > > + reg->desc.enable_mask = LAB_ENABLE_CTL_MASK; > > + } else { > > + reg = &labibb->ibb; > > + reg->desc.enable_mask = IBB_ENABLE_CTL_MASK; > > + } > > Write a switch statement so this is extensible. I can change over to switch, though in the current set of downstream code I've seen, it doesn't look that it would get extended. But I guess there isn't any harm in moving over to switch. Will do. Best, Sumit.
Hello Bjorn, Thanks very much for the review. On Tue, 12 May 2020 at 07:46, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Fri 08 May 13:41 PDT 2020, Sumit Semwal wrote: > > > From: Nisha Kumari <nishakumari@codeaurora.org> > > > > Qualcomm platforms have LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost) > > Regulators, labibb for short, which are used as power supply for > > lowercase Regulators ok. > > > LCD Mode displays. > > > > This patch adds labibb regulator driver for pmi8998 pmic, found on > > Uppercase PMIC > ok. > > SDM845 platforms. > > > > Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org> > > Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org> > > > > -- > > v2: sumits: reworked the driver for more common code, and addressed > > review comments from v1. This includes merging regulator_ops into > > one, and allowing for future labibb variations. > > --- > > drivers/regulator/Kconfig | 10 + > > drivers/regulator/Makefile | 1 + > > drivers/regulator/qcom-labibb-regulator.c | 288 ++++++++++++++++++++++ > > 3 files changed, 299 insertions(+) > > create mode 100644 drivers/regulator/qcom-labibb-regulator.c > > > > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > > index f4b72cb098ef..58704a9fd05d 100644 > > --- a/drivers/regulator/Kconfig > > +++ b/drivers/regulator/Kconfig > > @@ -1167,5 +1167,15 @@ config REGULATOR_WM8994 > > This driver provides support for the voltage regulators on the > > WM8994 CODEC. > > > > +config REGULATOR_QCOM_LABIBB > > + tristate "QCOM LAB/IBB regulator support" > > + depends on SPMI || COMPILE_TEST > > + help > > + This driver supports Qualcomm's LAB/IBB regulators present on the > > + Qualcomm's PMIC chip pmi8998. QCOM LAB and IBB are SPMI > > + based PMIC implementations. LAB can be used as positive > > + boost regulator and IBB can be used as a negative boost regulator > > + for LCD display panel. > > + > > endif > > > > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > > index 6610ee001d9a..5b313786c0e8 100644 > > --- a/drivers/regulator/Makefile > > +++ b/drivers/regulator/Makefile > > @@ -87,6 +87,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o > > obj-$(CONFIG_REGULATOR_MT6358) += mt6358-regulator.o > > obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o > > obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o > > +obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o > > obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o > > obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o > > obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o > > diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c > > new file mode 100644 > > index 000000000000..a9dc7c060375 > > --- /dev/null > > +++ b/drivers/regulator/qcom-labibb-regulator.c > > @@ -0,0 +1,288 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (c) 2019, The Linux Foundation. All rights reserved. > > + > > +#include <linux/module.h> > > +#include <linux/of_irq.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/regulator/driver.h> > > +#include <linux/regulator/of_regulator.h> > > + > > +#define REG_PERPH_TYPE 0x04 > > +#define QCOM_LAB_TYPE 0x24 > > +#define QCOM_IBB_TYPE 0x20 > > + > > +#define REG_LABIBB_STATUS1 0x08 > > +#define REG_LABIBB_ENABLE_CTL 0x46 > > +#define LABIBB_STATUS1_VREG_OK_BIT BIT(7) > > +#define LABIBB_CONTROL_ENABLE BIT(7) > > + > > +#define LAB_ENABLE_CTL_MASK BIT(7) > > +#define IBB_ENABLE_CTL_MASK (BIT(7) | BIT(6)) > > + > > +#define POWER_DELAY 8000 > > + > > +struct labibb_regulator { > > + struct regulator_desc desc; > > + struct device *dev; > > + struct regmap *regmap; > > + struct regulator_dev *rdev; > > + u16 base; > > + u8 type; > > +}; > > + > > +struct qcom_labibb { > > You pretty much use this as a local variable within probe, and then you > use labibb_regulator in runtime. Perhaps you can just drop it? > Yes, you're right. It's probably a leftover from the re-design. I will drop it. > > + struct device *dev; > > + struct regmap *regmap; > > + struct labibb_regulator lab; > > + struct labibb_regulator ibb; > > +}; > > + > > +struct labibb_regulator_data { > > + u16 base; > > + const char *name; > > + const char *irq_name; > > + u8 type; > > +}; > > + > > +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev) > > +{ > > + int ret; > > + u8 val; > > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > > + > > + ret = regmap_bulk_read(reg->regmap, reg->base + > > + REG_LABIBB_STATUS1, &val, 1); > > + if (ret < 0) { > > + dev_err(reg->dev, "Read register failed ret = %d\n", ret); > > + return ret; > > + } > > + > > + if (val & LABIBB_STATUS1_VREG_OK_BIT) > > + return 1; > > + else > > + return 0; > > return !!(val & LABIBB_STATUS1_VREG_OK_BIT); > > > +} > > + > > +static int _check_enabled_with_retries(struct regulator_dev *rdev, > > + int retries, int enabled) > > +{ > > + int ret; > > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > > + > > + while (retries--) { > > Mark's suggestion of extending _regulator_enable_delay() to support > polling is_enable() seems reasonable. > > The only complication I can see is that code path currently doesn't have > any expectations of the regulator not being operational at the end - > this seems to offer that possibility. So some care needs to be taken > there. > I think when I try Mark's suggestion, there will be a need to communicate an error if we don't get a successful check of status after the requested number of retries. So yes, I will try and take care in the next iteration. > > But doing that would allow you to use regulator_enable_regmap() as your > .enable function directly and you can drop > qcom_labibb_regulator_enable() > Agreed. > > + /* Wait for a small period before checking REG_LABIBB_STATUS1 */ > > + usleep_range(POWER_DELAY, POWER_DELAY + 200); > > + > > + ret = qcom_labibb_regulator_is_enabled(rdev); > > + > > + if (ret < 0) { > > + dev_err(reg->dev, "Can't read %s regulator status\n", > > + reg->desc.name); > > + return ret; > > + } > > + > > + if (ret == enabled) > > + return ret; > > + > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev) > > +{ > > + int ret, retries = 10; > > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > > + > > + ret = regulator_enable_regmap(rdev); > > + > > + if (ret < 0) { > > + dev_err(reg->dev, "Write failed: enable %s regulator\n", > > + reg->desc.name); > > + return ret; > > + } > > + > > + ret = _check_enabled_with_retries(rdev, retries, 1); > > + if (ret < 0) { > > + dev_err(reg->dev, "retries exhausted: enable %s regulator\n", > > + reg->desc.name); > > + return ret; > > + } > > + > > + if (ret) > > + return 0; > > This looks weird; check for timeout, check for errors and then return > success. Yes, it does look weird - that's because the _check_enabled_with_retries returns 1 if it matches what you asked to check (enable or disable via last argument). That should've been coded differently. It should go away with the above mentioned rework. > > > + > > + > > + dev_err(reg->dev, "Can't enable %s\n", reg->desc.name); > > + return -EINVAL; > > +} > > + > > +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev) > > +{ > > + int ret, retries = 2; > > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > > + > > + ret = regulator_disable_regmap(rdev); > > + > > + if (ret < 0) { > > + dev_err(reg->dev, "Write failed: disable %s regulator\n", > > + reg->desc.name); > > + return ret; > > + } > > + > > + ret = _check_enabled_with_retries(rdev, retries, 0); > > I don't think we care about waiting for the regulator to turning off, > might be nice to ensure that an off/on cycle really is allowed to take > its time though. So specifying an desc->off_on_delay of 8200 * 2 (the > worst case usleep_range() * retries) should take care of this. Oh, that's neat. Will update. > > And then you should be able to just use regulator_disable_regmap() as > your .disable function. +1. > > > + if (ret < 0) { > > + dev_err(reg->dev, "retries exhausted: disable %s regulator\n", > > + reg->desc.name); > > + return ret; > > + } > > + > > + if (!ret) > > + return 0; > > + > > + dev_err(reg->dev, "Can't disable %s\n", reg->desc.name); > > + return -EINVAL; > > +} > > + > > +static struct regulator_ops qcom_labibb_ops = { > > + .enable = qcom_labibb_regulator_enable, > > + .disable = qcom_labibb_regulator_disable, > > + .is_enabled = qcom_labibb_regulator_is_enabled, > > +}; > > + > > +static int register_labibb_regulator(struct qcom_labibb *labibb, > > + const struct labibb_regulator_data *reg_data, > > + struct device_node *of_node) > > +{ > > + int ret; > > + struct labibb_regulator *reg; > > + struct regulator_config cfg = {}; > > + > > + if (reg_data->type == QCOM_LAB_TYPE) { > > + reg = &labibb->lab; > > + reg->desc.enable_mask = LAB_ENABLE_CTL_MASK; > > + } else { > > + reg = &labibb->ibb; > > + reg->desc.enable_mask = IBB_ENABLE_CTL_MASK; > > + } > > + > > + reg->dev = labibb->dev; > > + reg->base = reg_data->base; > > + reg->type = reg_data->type; > > + reg->regmap = labibb->regmap; > > + reg->desc.enable_reg = reg->base + REG_LABIBB_ENABLE_CTL; > > + reg->desc.enable_val = LABIBB_CONTROL_ENABLE; > > + reg->desc.of_match = reg_data->name; > > + reg->desc.name = reg_data->name; > > + reg->desc.owner = THIS_MODULE; > > + reg->desc.type = REGULATOR_VOLTAGE; > > + reg->desc.ops = &qcom_labibb_ops; > > + > > + cfg.dev = labibb->dev; > > + cfg.driver_data = reg; > > + cfg.regmap = labibb->regmap; > > + cfg.of_node = of_node; > > + > > + reg->rdev = devm_regulator_register(labibb->dev, ®->desc, > > + &cfg); > > Do you really need to wrap this line? No :) - rework leftover. Will correct. > > > + if (IS_ERR(reg->rdev)) { > > + ret = PTR_ERR(reg->rdev); > > + dev_err(labibb->dev, > > + "unable to register %s regulator\n", reg_data->name); > > + return ret; > > + } > > + return 0; > > return PTR_ERR_OR_ZERO(reg->rdev); > > > +} > > + > > +static const struct labibb_regulator_data pmi8998_labibb_data[] = { > > + {0xde00, "lab", "lab-sc-err", QCOM_LAB_TYPE}, > > Please shorten the interrupt to just "sc-err". Ok. And actually then I can remove it off from the labibb_regulotor_data too, since it will be the same for both the regulators. > > > + {0xdc00, "ibb", "ibb-sc-err", QCOM_IBB_TYPE}, > > + { }, > > +}; > > + > > +static const struct of_device_id qcom_labibb_match[] = { > > + { .compatible = "qcom,pmi8998-lab-ibb", .data = &pmi8998_labibb_data}, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, qcom_labibb_match); > > + > > +static int qcom_labibb_regulator_probe(struct platform_device *pdev) > > +{ > > + struct qcom_labibb *labibb; > > + struct device_node *child; > > + const struct of_device_id *match; > > + const struct labibb_regulator_data *reg; > > + u8 type; > > + int ret; > > + > > + labibb = devm_kzalloc(&pdev->dev, sizeof(*labibb), GFP_KERNEL); > > + if (!labibb) > > + return -ENOMEM; > > + > > + labibb->regmap = dev_get_regmap(pdev->dev.parent, NULL); > > + if (!labibb->regmap) { > > + dev_err(&pdev->dev, "Couldn't get parent's regmap\n"); > > + return -ENODEV; > > + } > > + > > + labibb->dev = &pdev->dev; > > + > > + match = of_match_device(qcom_labibb_match, &pdev->dev); > > + if (!match) > > + return -ENODEV; > > + > > + for (reg = match->data; reg->name; reg++) { > > + child = of_get_child_by_name(pdev->dev.of_node, reg->name); > > + > > + /* TODO: This validates if the type of regulator is indeed > > + * what's mentioned in DT. > > + * I'm not sure if this is needed, but we'll keep it for now. > > + */ > > + ret = regmap_bulk_read(labibb->regmap, > > + reg->base + REG_PERPH_TYPE, > > + &type, 1); > > + if (ret < 0) { > > + dev_err(labibb->dev, > > + "Peripheral type read failed ret=%d\n", > > + ret); > > + return -EINVAL; > > + } > > + > > + if ((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) { > > + dev_err(labibb->dev, > > + "qcom_labibb: unknown peripheral type\n"); > > + return -EINVAL; > > + } else if (type != reg->type) { > > + dev_err(labibb->dev, > > + "qcom_labibb: type read %x doesn't match DT %x\n", > > + type, reg->type); > > + return -EINVAL; > > + } > > + > > + ret = register_labibb_regulator(labibb, reg, child); > > + if (ret < 0) { > > + dev_err(&pdev->dev, > > + "qcom_labibb: error registering %s regulator: %d\n", > > + child->full_name, ret); > > You already printed in register_labibb_regulator() so no need to print > again. Got it. > > > + return ret; > > + } > > + } > > + > > + dev_set_drvdata(&pdev->dev, labibb); > > rdev_get_drvdata() returns cfg.driver_data, so you don't need this one. Got it, and it ties to your other comment about struct qcom_labibb. Will update. > > Regards, > Bjorn > > > + return 0; > > +} > > + > > +static struct platform_driver qcom_labibb_regulator_driver = { > > + .driver = { > > + .name = "qcom-lab-ibb-regulator", > > + .of_match_table = qcom_labibb_match, > > + }, > > + .probe = qcom_labibb_regulator_probe, > > +}; > > +module_platform_driver(qcom_labibb_regulator_driver); > > + > > +MODULE_DESCRIPTION("Qualcomm labibb driver"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.26.2 > > Best, Sumit.
Hello Mark, On Thu, 14 May 2020 at 16:57, Sumit Semwal <sumit.semwal@linaro.org> wrote: > > Hello Mark, > > Thank you for your review comments! > On Mon, 11 May 2020 at 16:09, Mark Brown <broonie@kernel.org> wrote: > > > > On Sat, May 09, 2020 at 02:11:59AM +0530, Sumit Semwal wrote: > > > > > + ret = regmap_bulk_read(reg->regmap, reg->base + > > > + REG_LABIBB_STATUS1, &val, 1); > > > + if (ret < 0) { > > > + dev_err(reg->dev, "Read register failed ret = %d\n", ret); > > > + return ret; > > > + } > > > > Why a bulk read of a single register? > Right, will change. > > > > > +static int _check_enabled_with_retries(struct regulator_dev *rdev, > > > + int retries, int enabled) > > > +{ > > > > This is not retrying, this is polling to see if the regulator actually > > enabled. > Yes, will update accordingly. > > > > > > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev) > > > +{ > > > > > + ret = _check_enabled_with_retries(rdev, retries, 1); > > > + if (ret < 0) { > > > + dev_err(reg->dev, "retries exhausted: enable %s regulator\n", > > > + reg->desc.name); > > > + return ret; > > > + } > > > > If this is useful factor it out into a helper or the core, other devices > > also have status bits saying if the regulator is enabled. It looks like > > this may be mainly trying to open code something like enable_time, with > > possibly some issues where the time taken to enable varies a lot. > > > Makes sense; I am not terribly familiar with the regulator core and > helpers, so let me look and refactor accordingly. Does something like this make sense, or did I misunderstand your suggestion completely? I'll send the updated patches accordingly. --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -2353,6 +2353,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev) + /* If max_time_poll_enabled is set for the regulator, + * Poll upto max_time_poll_enabled time to see if the regulator + * actually got enabled. + * For each iteration, wait for the enable_time delay calculated + * above already. + * If the regulator isn't enabled after max_time_poll_enabled has + * expired, return -REG_ENABLED_CHECK_FAILED. + */ + if (rdev->desc->max_time_poll_enabled) { + unsigned int remaining_time_to_poll = rdev->desc->max_time_poll_enabled; + + while (remaining_time_to_poll > 0) { + /* We've already waited for enable_time above; + * so we can start with immediate check of the + * status of the regulator. + */ + if (rdev->desc->ops->is_enabled(rdev)) + break; + + _regulator_enable_delay(delay); + remaining_time_to_poll -= delay; + } + + if (remaining_time_to_poll <= 0) { + rdev_err(rdev, "Enabled check failed.\n"); + return -REG_ENABLED_CHECK_FAILED; + } + } + Since atleast in my use case, the delay is really enable_time (time before we could check the status register), we could reuse the already-calculated 'delay' based on enable_time? > <snip> Best, Sumit.
On Wed, May 27, 2020 at 10:01:27PM +0530, Sumit Semwal wrote: > On Thu, 14 May 2020 at 16:57, Sumit Semwal <sumit.semwal@linaro.org> wrote: > > > If this is useful factor it out into a helper or the core, other devices > > > also have status bits saying if the regulator is enabled. It looks like > > > this may be mainly trying to open code something like enable_time, with > > > possibly some issues where the time taken to enable varies a lot. > > Makes sense; I am not terribly familiar with the regulator core and > > helpers, so let me look and refactor accordingly. > Does something like this make sense, or did I misunderstand your > suggestion completely? I'll send the updated patches accordingly. I guess.
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index f4b72cb098ef..58704a9fd05d 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -1167,5 +1167,15 @@ config REGULATOR_WM8994 This driver provides support for the voltage regulators on the WM8994 CODEC. +config REGULATOR_QCOM_LABIBB + tristate "QCOM LAB/IBB regulator support" + depends on SPMI || COMPILE_TEST + help + This driver supports Qualcomm's LAB/IBB regulators present on the + Qualcomm's PMIC chip pmi8998. QCOM LAB and IBB are SPMI + based PMIC implementations. LAB can be used as positive + boost regulator and IBB can be used as a negative boost regulator + for LCD display panel. + endif diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 6610ee001d9a..5b313786c0e8 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -87,6 +87,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o obj-$(CONFIG_REGULATOR_MT6358) += mt6358-regulator.o obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o +obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c new file mode 100644 index 000000000000..a9dc7c060375 --- /dev/null +++ b/drivers/regulator/qcom-labibb-regulator.c @@ -0,0 +1,288 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2019, The Linux Foundation. All rights reserved. + +#include <linux/module.h> +#include <linux/of_irq.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/of_regulator.h> + +#define REG_PERPH_TYPE 0x04 +#define QCOM_LAB_TYPE 0x24 +#define QCOM_IBB_TYPE 0x20 + +#define REG_LABIBB_STATUS1 0x08 +#define REG_LABIBB_ENABLE_CTL 0x46 +#define LABIBB_STATUS1_VREG_OK_BIT BIT(7) +#define LABIBB_CONTROL_ENABLE BIT(7) + +#define LAB_ENABLE_CTL_MASK BIT(7) +#define IBB_ENABLE_CTL_MASK (BIT(7) | BIT(6)) + +#define POWER_DELAY 8000 + +struct labibb_regulator { + struct regulator_desc desc; + struct device *dev; + struct regmap *regmap; + struct regulator_dev *rdev; + u16 base; + u8 type; +}; + +struct qcom_labibb { + struct device *dev; + struct regmap *regmap; + struct labibb_regulator lab; + struct labibb_regulator ibb; +}; + +struct labibb_regulator_data { + u16 base; + const char *name; + const char *irq_name; + u8 type; +}; + +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev) +{ + int ret; + u8 val; + struct labibb_regulator *reg = rdev_get_drvdata(rdev); + + ret = regmap_bulk_read(reg->regmap, reg->base + + REG_LABIBB_STATUS1, &val, 1); + if (ret < 0) { + dev_err(reg->dev, "Read register failed ret = %d\n", ret); + return ret; + } + + if (val & LABIBB_STATUS1_VREG_OK_BIT) + return 1; + else + return 0; +} + +static int _check_enabled_with_retries(struct regulator_dev *rdev, + int retries, int enabled) +{ + int ret; + struct labibb_regulator *reg = rdev_get_drvdata(rdev); + + while (retries--) { + /* Wait for a small period before checking REG_LABIBB_STATUS1 */ + usleep_range(POWER_DELAY, POWER_DELAY + 200); + + ret = qcom_labibb_regulator_is_enabled(rdev); + + if (ret < 0) { + dev_err(reg->dev, "Can't read %s regulator status\n", + reg->desc.name); + return ret; + } + + if (ret == enabled) + return ret; + + } + + return -EINVAL; +} + +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev) +{ + int ret, retries = 10; + struct labibb_regulator *reg = rdev_get_drvdata(rdev); + + ret = regulator_enable_regmap(rdev); + + if (ret < 0) { + dev_err(reg->dev, "Write failed: enable %s regulator\n", + reg->desc.name); + return ret; + } + + ret = _check_enabled_with_retries(rdev, retries, 1); + if (ret < 0) { + dev_err(reg->dev, "retries exhausted: enable %s regulator\n", + reg->desc.name); + return ret; + } + + if (ret) + return 0; + + + dev_err(reg->dev, "Can't enable %s\n", reg->desc.name); + return -EINVAL; +} + +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev) +{ + int ret, retries = 2; + struct labibb_regulator *reg = rdev_get_drvdata(rdev); + + ret = regulator_disable_regmap(rdev); + + if (ret < 0) { + dev_err(reg->dev, "Write failed: disable %s regulator\n", + reg->desc.name); + return ret; + } + + ret = _check_enabled_with_retries(rdev, retries, 0); + if (ret < 0) { + dev_err(reg->dev, "retries exhausted: disable %s regulator\n", + reg->desc.name); + return ret; + } + + if (!ret) + return 0; + + dev_err(reg->dev, "Can't disable %s\n", reg->desc.name); + return -EINVAL; +} + +static struct regulator_ops qcom_labibb_ops = { + .enable = qcom_labibb_regulator_enable, + .disable = qcom_labibb_regulator_disable, + .is_enabled = qcom_labibb_regulator_is_enabled, +}; + +static int register_labibb_regulator(struct qcom_labibb *labibb, + const struct labibb_regulator_data *reg_data, + struct device_node *of_node) +{ + int ret; + struct labibb_regulator *reg; + struct regulator_config cfg = {}; + + if (reg_data->type == QCOM_LAB_TYPE) { + reg = &labibb->lab; + reg->desc.enable_mask = LAB_ENABLE_CTL_MASK; + } else { + reg = &labibb->ibb; + reg->desc.enable_mask = IBB_ENABLE_CTL_MASK; + } + + reg->dev = labibb->dev; + reg->base = reg_data->base; + reg->type = reg_data->type; + reg->regmap = labibb->regmap; + reg->desc.enable_reg = reg->base + REG_LABIBB_ENABLE_CTL; + reg->desc.enable_val = LABIBB_CONTROL_ENABLE; + reg->desc.of_match = reg_data->name; + reg->desc.name = reg_data->name; + reg->desc.owner = THIS_MODULE; + reg->desc.type = REGULATOR_VOLTAGE; + reg->desc.ops = &qcom_labibb_ops; + + cfg.dev = labibb->dev; + cfg.driver_data = reg; + cfg.regmap = labibb->regmap; + cfg.of_node = of_node; + + reg->rdev = devm_regulator_register(labibb->dev, ®->desc, + &cfg); + if (IS_ERR(reg->rdev)) { + ret = PTR_ERR(reg->rdev); + dev_err(labibb->dev, + "unable to register %s regulator\n", reg_data->name); + return ret; + } + return 0; +} + +static const struct labibb_regulator_data pmi8998_labibb_data[] = { + {0xde00, "lab", "lab-sc-err", QCOM_LAB_TYPE}, + {0xdc00, "ibb", "ibb-sc-err", QCOM_IBB_TYPE}, + { }, +}; + +static const struct of_device_id qcom_labibb_match[] = { + { .compatible = "qcom,pmi8998-lab-ibb", .data = &pmi8998_labibb_data}, + { }, +}; +MODULE_DEVICE_TABLE(of, qcom_labibb_match); + +static int qcom_labibb_regulator_probe(struct platform_device *pdev) +{ + struct qcom_labibb *labibb; + struct device_node *child; + const struct of_device_id *match; + const struct labibb_regulator_data *reg; + u8 type; + int ret; + + labibb = devm_kzalloc(&pdev->dev, sizeof(*labibb), GFP_KERNEL); + if (!labibb) + return -ENOMEM; + + labibb->regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!labibb->regmap) { + dev_err(&pdev->dev, "Couldn't get parent's regmap\n"); + return -ENODEV; + } + + labibb->dev = &pdev->dev; + + match = of_match_device(qcom_labibb_match, &pdev->dev); + if (!match) + return -ENODEV; + + for (reg = match->data; reg->name; reg++) { + child = of_get_child_by_name(pdev->dev.of_node, reg->name); + + /* TODO: This validates if the type of regulator is indeed + * what's mentioned in DT. + * I'm not sure if this is needed, but we'll keep it for now. + */ + ret = regmap_bulk_read(labibb->regmap, + reg->base + REG_PERPH_TYPE, + &type, 1); + if (ret < 0) { + dev_err(labibb->dev, + "Peripheral type read failed ret=%d\n", + ret); + return -EINVAL; + } + + if ((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) { + dev_err(labibb->dev, + "qcom_labibb: unknown peripheral type\n"); + return -EINVAL; + } else if (type != reg->type) { + dev_err(labibb->dev, + "qcom_labibb: type read %x doesn't match DT %x\n", + type, reg->type); + return -EINVAL; + } + + ret = register_labibb_regulator(labibb, reg, child); + if (ret < 0) { + dev_err(&pdev->dev, + "qcom_labibb: error registering %s regulator: %d\n", + child->full_name, ret); + return ret; + } + } + + dev_set_drvdata(&pdev->dev, labibb); + return 0; +} + +static struct platform_driver qcom_labibb_regulator_driver = { + .driver = { + .name = "qcom-lab-ibb-regulator", + .of_match_table = qcom_labibb_match, + }, + .probe = qcom_labibb_regulator_probe, +}; +module_platform_driver(qcom_labibb_regulator_driver); + +MODULE_DESCRIPTION("Qualcomm labibb driver"); +MODULE_LICENSE("GPL v2");