| Message ID | 20200602100924.26256-5-sumit.semwal@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Qualcomm labibb regulator driver | expand |
On Tue, Jun 02, 2020 at 03:39:23PM +0530, Sumit Semwal wrote: > +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev) > +{ > + int ret; > + unsigned int val; > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > + > + ret = regmap_read(reg->regmap, reg->base + REG_LABIBB_STATUS1, &val); > + if (ret < 0) { > + dev_err(reg->dev, "Read register failed ret = %d\n", ret); > + return ret; > + } > + return !!(val & LABIBB_STATUS1_VREG_OK_BIT); > +} This should be a get_status() callback... > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev) > +{ > + return regulator_enable_regmap(rdev); > +} > + > +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev) > +{ > + return regulator_disable_regmap(rdev); > +} ...is_enabled() should just be regulator_is_enabled_regmap() and these functions should just be removed entirely, you can use the regmap operations directly as the ops without the wrapper. > + match = of_match_device(qcom_labibb_match, &pdev->dev); > + if (!match) > + return -ENODEV; > + > + for (reg_data = match->data; reg_data->name; reg_data++) { > + child = of_get_child_by_name(pdev->dev.of_node, reg_data->name); > + > + if (WARN_ON(child == NULL)) > + return -EINVAL; This feels like the DT bindings are confused - why do we need to search like this? > + dev_info(dev, "Registering %s regulator\n", child->full_name); This is noise, remove it. The regulator framework will announce new regulators anyway.
Hi Mark, Thank you very much for reviewing. On Tue, 2 Jun 2020 at 17:02, Mark Brown <broonie@kernel.org> wrote: > > On Tue, Jun 02, 2020 at 03:39:23PM +0530, Sumit Semwal wrote: > > > +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev) > > +{ > > + int ret; > > + unsigned int val; > > + struct labibb_regulator *reg = rdev_get_drvdata(rdev); > > + > > + ret = regmap_read(reg->regmap, reg->base + REG_LABIBB_STATUS1, &val); > > + if (ret < 0) { > > + dev_err(reg->dev, "Read register failed ret = %d\n", ret); > > + return ret; > > + } > > + return !!(val & LABIBB_STATUS1_VREG_OK_BIT); > > +} > > This should be a get_status() callback... > From my (limited) understanding of downstream code, it seemed like for this set of regulators, the 'enabled' check is done via the 'REG_LABIBB_STATUS1 reg; for some reason, not via the same enable_reg / enable_mask ones. That's why I used it as is_enabled() callback. I will try and check with the QC folks to clarify this point about their hardware. > > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev) > > +{ > > + return regulator_enable_regmap(rdev); > > +} > > + > > +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev) > > +{ > > + return regulator_disable_regmap(rdev); > > +} > > ...is_enabled() should just be regulator_is_enabled_regmap() and these > functions should just be removed entirely, you can use the regmap > operations directly as the ops without the wrapper. The 2 wrappers are a precursor to the next patch, where we keep track of regulator's enable status to check during SC handling. > > > + match = of_match_device(qcom_labibb_match, &pdev->dev); > > + if (!match) > > + return -ENODEV; > > + > > + for (reg_data = match->data; reg_data->name; reg_data++) { > > + child = of_get_child_by_name(pdev->dev.of_node, reg_data->name); > > + > > + if (WARN_ON(child == NULL)) > > + return -EINVAL; > > This feels like the DT bindings are confused - why do we need to search > like this? The WARN_ON? This was suggested by Bjorn to catch the case where the DT binding for a PMIC instantiates only one of the regulators. > > > + dev_info(dev, "Registering %s regulator\n", child->full_name); > > This is noise, remove it. The regulator framework will announce new > regulators anyway. Agreed. will remove in the next iteration. Best, Sumit.
On Tue, Jun 02, 2020 at 05:40:45PM +0530, Sumit Semwal wrote: > On Tue, 2 Jun 2020 at 17:02, Mark Brown <broonie@kernel.org> wrote: > > On Tue, Jun 02, 2020 at 03:39:23PM +0530, Sumit Semwal wrote: > > This should be a get_status() callback... > From my (limited) understanding of downstream code, it seemed like for > this set of regulators, the 'enabled' check is done via the > 'REG_LABIBB_STATUS1 reg; for some reason, not via the same enable_reg > / enable_mask ones. That's why I used it as is_enabled() callback. > I will try and check with the QC folks to clarify this point about > their hardware. The way this is functioning at the minute the downstream code is just buggy. > > ...is_enabled() should just be regulator_is_enabled_regmap() and these > > functions should just be removed entirely, you can use the regmap > > operations directly as the ops without the wrapper. > The 2 wrappers are a precursor to the next patch, where we keep track > of regulator's enable status to check during SC handling. Add the functions when they're useful, not before. TBH if the register is write only you're probably better off adding a register cache. > > > + match = of_match_device(qcom_labibb_match, &pdev->dev); > > > + if (!match) > > > + return -ENODEV; > > > + > > > + for (reg_data = match->data; reg_data->name; reg_data++) { > > > + child = of_get_child_by_name(pdev->dev.of_node, reg_data->name); > > > + > > > + if (WARN_ON(child == NULL)) > > > + return -EINVAL; > > > > This feels like the DT bindings are confused - why do we need to search > > like this? > The WARN_ON? This was suggested by Bjorn to catch the case where the > DT binding for a PMIC instantiates only one of the regulators. No, this whole loop - why this whole match and get child stuff?
Hello Mark, On Tue, 2 Jun 2020 at 17:55, Mark Brown <broonie@kernel.org> wrote: > > On Tue, Jun 02, 2020 at 05:40:45PM +0530, Sumit Semwal wrote: > > On Tue, 2 Jun 2020 at 17:02, Mark Brown <broonie@kernel.org> wrote: > > > On Tue, Jun 02, 2020 at 03:39:23PM +0530, Sumit Semwal wrote: > > > > This should be a get_status() callback... > > > From my (limited) understanding of downstream code, it seemed like for > > this set of regulators, the 'enabled' check is done via the > > 'REG_LABIBB_STATUS1 reg; for some reason, not via the same enable_reg > > / enable_mask ones. That's why I used it as is_enabled() callback. > > I will try and check with the QC folks to clarify this point about > > their hardware. > > The way this is functioning at the minute the downstream code is just > buggy. Apologies for the delay in responding - I pinged the QC folks, and was waiting for their reply but haven't got any response so far. I tried your suggestion to use the ENABLE_CTL register for checking if the regulator is actually enabled. In my limited testing on the Poco, it seems like the STATUS1 register updates faster than the ENABLE_CTL register, so on the device, I see noticeable lag when I use ENABLE_CTL for is_enabled() check. [This is especially true for the IBB, which takes longer to become usable than the LAB regulator.] I understand from a pure regulators' correctness point of view, ENABLE_CTL should be the one checked there, so I can change the patch as you suggested, but there seems to be some performance penalty there. > > > > ...is_enabled() should just be regulator_is_enabled_regmap() and these > > > functions should just be removed entirely, you can use the regmap > > > operations directly as the ops without the wrapper. > > > The 2 wrappers are a precursor to the next patch, where we keep track > > of regulator's enable status to check during SC handling. > > Add the functions when they're useful, not before. TBH if the register > is write only you're probably better off adding a register cache. Agreed, I will remove the wrappers from here, using the regmap functions, and add the wrappers with the SC handling patch. > > > > > + match = of_match_device(qcom_labibb_match, &pdev->dev); > > > > + if (!match) > > > > + return -ENODEV; > > > > + > > > > + for (reg_data = match->data; reg_data->name; reg_data++) { > > > > + child = of_get_child_by_name(pdev->dev.of_node, reg_data->name); > > > > + > > > > + if (WARN_ON(child == NULL)) > > > > + return -EINVAL; > > > > > > This feels like the DT bindings are confused - why do we need to search > > > like this? > > > The WARN_ON? This was suggested by Bjorn to catch the case where the > > DT binding for a PMIC instantiates only one of the regulators. > > No, this whole loop - why this whole match and get child stuff? This loop mechanism is what I saw in the other qcom regulators upstream, so thought it was an acceptable way. For the two children nodes, do you recommend another mechanism to get and validate both nodes? Best, Sumit.
On Wed, Jun 17, 2020 at 05:12:35PM +0530, Sumit Semwal wrote: > I understand from a pure regulators' correctness point of view, > ENABLE_CTL should be the one checked there, so I can change the patch > as you suggested, but there seems to be some performance penalty > there. I thought the goal was to have the performance penalty to ensure that the regulator had actually started? > > > The WARN_ON? This was suggested by Bjorn to catch the case where the > > > DT binding for a PMIC instantiates only one of the regulators. > > No, this whole loop - why this whole match and get child stuff? > This loop mechanism is what I saw in the other qcom regulators > upstream, so thought it was an acceptable way. > For the two children nodes, do you recommend another mechanism to get > and validate both nodes? I don't understand what you mean by "two children nodes" here?
On Wed, 17 Jun 2020 at 17:17, Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jun 17, 2020 at 05:12:35PM +0530, Sumit Semwal wrote: > > > I understand from a pure regulators' correctness point of view, > > ENABLE_CTL should be the one checked there, so I can change the patch > > as you suggested, but there seems to be some performance penalty > > there. > > I thought the goal was to have the performance penalty to ensure that > the regulator had actually started? IMHO, with the poll_enabled_time mechanism added, we would not need to wait for the full enabled_time time for the regulator to get enabled, but we could poll (and potentially know earlier) if the regulator is enabled. The performance penalty I was talking, is about how should we check if the regulator is really enabled or not - via reading the STATUS1 register, which seems to tell the status a bit faster, or via reading the ENABLE_CTL register which we also use to enable/disable the regulator, but which seems to be slower in updating the status. > > > > > The WARN_ON? This was suggested by Bjorn to catch the case where the > > > > DT binding for a PMIC instantiates only one of the regulators. > > > > No, this whole loop - why this whole match and get child stuff? > > > This loop mechanism is what I saw in the other qcom regulators > > upstream, so thought it was an acceptable way. > > For the two children nodes, do you recommend another mechanism to get > > and validate both nodes? > > I don't understand what you mean by "two children nodes" here? The two 'lab' and 'ibb' regulator nodes that are part of the labibb node. Best, Sumit.
On Wed, Jun 17, 2020 at 05:27:12PM +0530, Sumit Semwal wrote: > On Wed, 17 Jun 2020 at 17:17, Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jun 17, 2020 at 05:12:35PM +0530, Sumit Semwal wrote: > > > I understand from a pure regulators' correctness point of view, > > > ENABLE_CTL should be the one checked there, so I can change the patch > > > as you suggested, but there seems to be some performance penalty > > > there. > > I thought the goal was to have the performance penalty to ensure that > > the regulator had actually started? > IMHO, with the poll_enabled_time mechanism added, we would not need to > wait for the full enabled_time time for the regulator to get enabled, > but we could poll (and potentially know earlier) if the regulator is > enabled. > The performance penalty I was talking, is about how should we check if > the regulator is really enabled or not - via reading the STATUS1 > register, which seems to tell the status a bit faster, or via reading > the ENABLE_CTL register which we also use to enable/disable the > regulator, but which seems to be slower in updating the status. That seems... interesting. Are you sure the regulator has fully ramped when STATUS1 starts flagging? > > > > > The WARN_ON? This was suggested by Bjorn to catch the case where the > > > > > DT binding for a PMIC instantiates only one of the regulators. > > > > No, this whole loop - why this whole match and get child stuff? > > > This loop mechanism is what I saw in the other qcom regulators > > > upstream, so thought it was an acceptable way. > > > For the two children nodes, do you recommend another mechanism to get > > > and validate both nodes? > > I don't understand what you mean by "two children nodes" here? > The two 'lab' and 'ibb' regulator nodes that are part of the labibb node. Use of_match and regulators_node like other regulator drivers.
On Wed, 17 Jun 2020 at 17:36, Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jun 17, 2020 at 05:27:12PM +0530, Sumit Semwal wrote: > > On Wed, 17 Jun 2020 at 17:17, Mark Brown <broonie@kernel.org> wrote: > > > On Wed, Jun 17, 2020 at 05:12:35PM +0530, Sumit Semwal wrote: > > > > > I understand from a pure regulators' correctness point of view, > > > > ENABLE_CTL should be the one checked there, so I can change the patch > > > > as you suggested, but there seems to be some performance penalty > > > > there. > > > > I thought the goal was to have the performance penalty to ensure that > > > the regulator had actually started? > > > IMHO, with the poll_enabled_time mechanism added, we would not need to > > wait for the full enabled_time time for the regulator to get enabled, > > but we could poll (and potentially know earlier) if the regulator is > > enabled. > > The performance penalty I was talking, is about how should we check if > > the regulator is really enabled or not - via reading the STATUS1 > > register, which seems to tell the status a bit faster, or via reading > > the ENABLE_CTL register which we also use to enable/disable the > > regulator, but which seems to be slower in updating the status. > > That seems... interesting. Are you sure the regulator has fully ramped > when STATUS1 starts flagging? On a consumer device, I am not sure I have any way of checking that, but if there's some way you'd like me to validate it, I'll be happy to. > > > > > > > The WARN_ON? This was suggested by Bjorn to catch the case where the > > > > > > DT binding for a PMIC instantiates only one of the regulators. > > > > > > No, this whole loop - why this whole match and get child stuff? > > > > > This loop mechanism is what I saw in the other qcom regulators > > > > upstream, so thought it was an acceptable way. > > > > For the two children nodes, do you recommend another mechanism to get > > > > and validate both nodes? > > > > I don't understand what you mean by "two children nodes" here? > > > The two 'lab' and 'ibb' regulator nodes that are part of the labibb node. > > Use of_match and regulators_node like other regulator drivers. Ok, let me see what I can do with those; we still need to flag if some platform only instantiates one of the two lab/ibb regulators - I was given the impression they're 'both or none' case. Best, Sumit.
On Wed, Jun 17, 2020 at 05:39:26PM +0530, Sumit Semwal wrote: > On Wed, 17 Jun 2020 at 17:36, Mark Brown <broonie@kernel.org> wrote: > > That seems... interesting. Are you sure the regulator has fully ramped > > when STATUS1 starts flagging? > On a consumer device, I am not sure I have any way of checking that, > but if there's some way you'd like me to validate it, I'll be happy > to. Without any way of validating what's going on or information on the hardware I'd be inclined to go with whatever reports more slowly for safety.
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..33b764ac69d1 --- /dev/null +++ b/drivers/regulator/qcom-labibb-regulator.c @@ -0,0 +1,194 @@ +// SPDX-License-Identifier: GPL-2.0-only +// Copyright (c) 2020, 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 LABIBB_OFF_ON_DELAY 1000 +#define LAB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 2) +#define IBB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 10) +#define LABIBB_POLL_ENABLED_TIME 1000 + +struct labibb_regulator { + struct regulator_desc desc; + struct device *dev; + struct regmap *regmap; + struct regulator_dev *rdev; + u16 base; + u8 type; +}; + +struct labibb_regulator_data { + u16 base; + const char *name; + u8 type; + unsigned int enable_time; + unsigned int enable_mask; +}; + +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev) +{ + int ret; + unsigned int val; + struct labibb_regulator *reg = rdev_get_drvdata(rdev); + + ret = regmap_read(reg->regmap, reg->base + REG_LABIBB_STATUS1, &val); + if (ret < 0) { + dev_err(reg->dev, "Read register failed ret = %d\n", ret); + return ret; + } + return !!(val & LABIBB_STATUS1_VREG_OK_BIT); +} + +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev) +{ + return regulator_enable_regmap(rdev); +} + +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev) +{ + return regulator_disable_regmap(rdev); +} + +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 struct regulator_dev *register_labibb_regulator(struct labibb_regulator *reg, + const struct labibb_regulator_data *reg_data, + struct device_node *of_node) +{ + struct regulator_config cfg = {}; + int ret; + + reg->base = reg_data->base; + reg->type = reg_data->type; + reg->desc.enable_mask = reg_data->enable_mask; + 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; + + reg->desc.enable_time = reg_data->enable_time; + reg->desc.poll_enabled_time = LABIBB_POLL_ENABLED_TIME; + reg->desc.off_on_delay = LABIBB_OFF_ON_DELAY; + + cfg.dev = reg->dev; + cfg.driver_data = reg; + cfg.regmap = reg->regmap; + cfg.of_node = of_node; + + return devm_regulator_register(reg->dev, ®->desc, &cfg); +} + +static const struct labibb_regulator_data pmi8998_labibb_data[] = { + {0xde00, "lab", QCOM_LAB_TYPE, LAB_ENABLE_TIME, LAB_ENABLE_CTL_MASK}, + {0xdc00, "ibb", QCOM_IBB_TYPE, IBB_ENABLE_TIME, IBB_ENABLE_CTL_MASK}, + { }, +}; + +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 labibb_regulator *labibb_reg; + struct device *dev = &pdev->dev; + struct device_node *child; + const struct of_device_id *match; + const struct labibb_regulator_data *reg_data; + struct regmap *reg_regmap; + unsigned int type; + int ret; + + reg_regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!reg_regmap) { + dev_err(&pdev->dev, "Couldn't get parent's regmap\n"); + return -ENODEV; + } + + match = of_match_device(qcom_labibb_match, &pdev->dev); + if (!match) + return -ENODEV; + + for (reg_data = match->data; reg_data->name; reg_data++) { + child = of_get_child_by_name(pdev->dev.of_node, reg_data->name); + + if (WARN_ON(child == NULL)) + return -EINVAL; + + /* Validate if the type of regulator is indeed + * what's mentioned in DT. + */ + ret = regmap_read(reg_regmap, reg_data->base + REG_PERPH_TYPE, + &type); + if (ret < 0) { + dev_err(dev, + "Peripheral type read failed ret=%d\n", + ret); + return -EINVAL; + } + + if (WARN_ON((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) || + WARN_ON(type != reg_data->type)) + return -EINVAL; + + labibb_reg = devm_kzalloc(&pdev->dev, sizeof(*labibb_reg), + GFP_KERNEL); + if (!labibb_reg) + return -ENOMEM; + + labibb_reg->regmap = reg_regmap; + labibb_reg->dev = dev; + + dev_info(dev, "Registering %s regulator\n", child->full_name); + + labibb_reg->rdev = register_labibb_regulator(labibb_reg, reg_data, child); + if (IS_ERR(labibb_reg->rdev)) { + dev_err(dev, + "qcom_labibb: error registering %s : %d\n", + child->full_name, ret); + return PTR_ERR(labibb_reg->rdev); + } + } + + 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");