regulator: add QCOM RPMh regulator driver

From: Fengguang Wu <[email protected]>

drivers/regulator/qcom_rpmh-regulator.c:1104:3-8:
No need to set .owner here. The core will do it.

+/**
+ * struct rpmh_regulator_mode - RPMh VRM mode attributes

+ * @pmic_mode:                 Raw PMIC mode value written into VRM mode voting
+ *                             register (i.e. RPMH_REGULATOR_MODE_*)
+ * @framework_mode:            Regulator framework mode value
+ *                             (i.e. REGULATOR_MODE_*)
+ * @min_load_ua:               The minimum load current in microamps which
+ *                             would utilize this mode

drivers/regulator/qcom_rpmh-regulator.c new file +11 -0

+static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev,
+                               int min_uv, int max_uv, unsigned int *selector)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
+       };
+       const struct regulator_linear_range *range;
+       int mv, uv, ret;
+       bool wait_for_ack;
+

drivers/regulator/qcom_rpmh-regulator.c new file +14 -0

+static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
+                                       unsigned int mode)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
+       };
+       int i, ret;
+
+       if (mode == vreg->mode)
+               return 0;
+
+       for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
+               if (vreg->hw_data->mode_map[i].framework_mode == mode)

drivers/regulator/qcom_rpmh-regulator.c new file +17 -0

+/**
+ * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
+ *             for a VRM RPMh resource from device tree
+ * vreg:               Pointer to the rpmh regulator resource
+ *
+ * This function initializes the mode[] array of vreg based upon the values
+ * of optional device tree properties.
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
+{
+       struct device_node *node = vreg->of_node;
+       const struct rpmh_regulator_mode *map;
+       const char *prop;
+       int i, len, ret;
+       u32 *buf;

drivers/regulator/qcom_rpmh-regulator.c new file +21 -0

+/**
+ * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated
+ *             with the PMIC and initialize important pointers for each
+ *             regulator
+ * @pmic:              Pointer to the RPMh regulator PMIC
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic)
+{
+       struct device_node *node;
+       int i;
+
+       pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node);
+       if (pmic->vreg_count == 0) {
+               dev_err(pmic->dev, "could not find any regulator subnodes\n");
+               return -ENODEV;
+       }
+
+       pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count,
+                       sizeof(*pmic->vreg), GFP_KERNEL);

drivers/regulator/qcom_rpmh-regulator.c new file +38 -0

+/**
+ * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
+ *             request for this regulator based on optional device tree
+ *             properties
+ * @vreg:              Pointer to the RPMh regulator
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg)
+{
+       struct tcs_cmd cmd[2] = { };
+       const char *prop;
+       int cmd_count = 0;
+       int ret;
+       u32 temp;
+
+       if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) {
+               prop = "qcom,headroom-voltage";
+               ret = of_property_read_u32(vreg->of_node, prop, &temp);
+               if (!ret) {
+                       if (temp < RPMH_VRM_HEADROOM_MIN_UV ||
+                           temp > RPMH_VRM_HEADROOM_MAX_UV) {
+                               vreg_err(vreg, "%s=%u is invalid\n",
+                                       prop, temp);
+                               return -EINVAL;
+                       }
+                       vreg->headroom_voltage = temp;
+
+                       cmd[cmd_count].addr
+                               = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
+                       cmd[cmd_count++].data
+                               = DIV_ROUND_UP(vreg->headroom_voltage, 1000);
+               }
+
+               prop = "qcom,regulator-initial-voltage";
+               ret = of_property_read_u32(vreg->of_node, prop, &temp);
+               if (!ret) {
+                       if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) {

drivers/regulator/qcom_rpmh-regulator.c new file +57 -0

+/**
+ * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator
+ * @vreg:              Pointer to the RPMh regulator
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg)
+{
+       struct device *dev = vreg->pmic->dev;
+       struct regulator_config reg_config = {};
+       const struct rpmh_vreg_init_data *rpmh_data = NULL;
+       const char *type_name = NULL;
+       enum rpmh_regulator_type type;
+       struct regulator_init_data *init_data;
+       int ret, i;
+
+       for (i = 0; i < vreg->pmic->init_data->count; i++) {
+               if (!strcmp(vreg->pmic->init_data->vreg_data[i].name,
+                           vreg->of_node->name)) {
+                       rpmh_data = &vreg->pmic->init_data->vreg_data[i];
+                       break;
+               }
+       }
+
+       if (!rpmh_data) {
+               dev_err(dev, "Unknown regulator %s for %s RPMh regulator PMIC\n",
+                       vreg->of_node->name, vreg->pmic->init_data->name);
+               return -EINVAL;
+       }
+
+       vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d",
+                       rpmh_data->resource_name_base, vreg->pmic->pmic_id,
+                       rpmh_data->id);
+       if (!vreg->resource_name)
+               return -ENOMEM;
+
+       vreg->addr = cmd_db_read_addr(vreg->resource_name);
+       if (!vreg->addr) {
+               vreg_err(vreg, "could not find RPMh address for resource %s\n",
+                       vreg->resource_name);
+               return -ENODEV;
+       }
+
+       vreg->rdesc.name = rpmh_data->name;
+       vreg->rdesc.supply_name = rpmh_data->supply_name;
+       vreg->regulator_type = rpmh_data->regulator_type;
+       vreg->hw_data = rpmh_data->hw_data;
+
+       if (rpmh_data->hw_data->voltage_range) {
+               vreg->rdesc.linear_ranges = rpmh_data->hw_data->voltage_range;
+               vreg->rdesc.n_linear_ranges = 1;
+               vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages;
+       }
+
+       /* Optional override for the default RPMh accelerator type */
+       ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type",
+                                       &type_name);

drivers/regulator/qcom_rpmh-regulator.c new file +27 -0

+/**
+ * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each
+ *             of the regulator nodes associated with it
+ * @pdev:              Pointer to the platform device of the RPMh PMIC
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       const struct of_device_id *match;
+       struct rpmh_pmic *pmic;
+       struct device_node *node;
+       int ret, i;
+
+       node = dev->of_node;
+
+       if (!node) {
+               dev_err(dev, "Device tree node is missing\n");
+               return -EINVAL;
+       }
+
+       ret = cmd_db_ready();
+       if (ret < 0) {
+               if (ret != -EPROBE_DEFER)
+                       dev_err(dev, "Command DB not available, ret=%d\n", ret);
+               return ret;

drivers/regulator/qcom_rpmh-regulator.c new file +6 -0

+       },
+       .probe          = rpmh_regulator_probe,
+       .remove         = rpmh_regulator_remove,
+};
+
+static int rpmh_regulator_init(void)

include/dt-bindings/regulator/qcom,rpmh-regulator.h new file +22 -0

+ * properties of an RPMh resource.  Each type of regulator supports a subset of
+ * the possible modes.
+ *
+ * %RPMH_REGULATOR_MODE_PASS:  Pass-through mode in which output is directly
+ *                             tied to input.  This mode is only supported by
+ *                             BOB type regulators.
+ * %RPMH_REGULATOR_MODE_RET:   Retention mode in which only an extremely small
+ *                             load current is allowed.  This mode is supported
+ *                             by LDO and SMPS type regulators.
+ * %RPMH_REGULATOR_MODE_LPM:   Low power mode in which a small load current is
+ *                             allowed.  This mode corresponds to PFM for SMPS
+ *                             and BOB type regulators.  This mode is supported
+ *                             by LDO, HFSMPS, BOB, and PMIC4 FTSMPS type
+ *                             regulators.
+ * %RPMH_REGULATOR_MODE_AUTO:  Auto mode in which the regulator hardware
+ *                             automatically switches between LPM and HPM based
+ *                             upon the real-time load current.  This mode is
+ *                             supported by HFSMPS, BOB, and PMIC4 FTSMPS type
+ *                             regulators.
+ * %RPMH_REGULATOR_MODE_HPM:   High power mode in which the full rated current
+ *                             of the regulator is allowed.  This mode
+ *                             corresponds to PWM for SMPS and BOB type

Quoted 8 lines · David Collins

+/**
+ * struct rpmh_regulator_mode - RPMh VRM mode attributes

+ * @pmic_mode:                 Raw PMIC mode value written into VRM mode voting
+ *                             register (i.e. RPMH_REGULATOR_MODE_*)
+ * @framework_mode:            Regulator framework mode value
+ *                             (i.e. REGULATOR_MODE_*)
+ * @min_load_ua:               The minimum load current in microamps which
+ *                             would utilize this mode

Thinking of this as "min load" seems backward to me. Shouldn't we be keeping track of "max load". AKA:

Use "idle" if the load is less than 1000 mA
Use "normal" if the load is more than 1000 mA but less than 5000 mA Use "fast" if the load is more than 5000 mA.

I'd think of "1000 mA as the max load that "idle" can handle". The reason I don't think of it as "min" is that you can't say "1000 mA is the min load the "normal" can handle". Normal can handle smaller loads just fine, it's just not ideal.

Thinking of it in terms of "max" would also get rid of that weird "needs to be 0" entry in the device tree too. You could either leave the number off the top one (and set to INT_MAX in software?) or you could use that to put in some sort of sane maximum current. I'd bet there's something in the datasheet for this. If someone in software got mixed up and kept requesting more and more current, this could catch their error.

drivers/regulator/qcom_rpmh-regulator.c line 245

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+       return vreg->enabled;

You can't read hardware? The regulator framework expects you to read hardware. If it was common not to be able to read hardware then the regulator framework would just keep track of the enabled state for you. Right now if a regulator was left on by the BIOS (presumably some have to be since otherwise you're running on a computer that takes no power), you'll still return false at bootup? Seems non-ideal.

Quoted 8 lines

drivers/regulator/qcom_rpmh-regulator.c new file +8 -0

+}
+
+static int rpmh_regulator_enable(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+               .data = RPMH_REGULATOR_ENABLE,

drivers/regulator/qcom_rpmh-regulator.c line 258

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+       };
+       int ret;
+
+       if (vreg->enabled)
+               return 0;

Does the "if" test above ever hit? I'd think the regulator framework would handle this.

Quoted 6 lines

drivers/regulator/qcom_rpmh-regulator.c new file +6 -0

+static int rpmh_regulator_disable(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+               .data = RPMH_REGULATOR_DISABLE,

drivers/regulator/qcom_rpmh-regulator.c line 281

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+       };
+       int ret;
+
+       if (!vreg->enabled)
+               return 0;

Does the "if" test above ever hit? I'd think the regulator framework would handle this.

Quoted 11 lines

drivers/regulator/qcom_rpmh-regulator.c new file +11 -0

+static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev,
+                               int min_uv, int max_uv, unsigned int *selector)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
+       };
+       const struct regulator_linear_range *range;
+       int mv, uv, ret;
+       bool wait_for_ack;
+

drivers/regulator/qcom_rpmh-regulator.c line 314

Quoted 10 lines

drivers/regulator/qcom_rpmh-regulator.c new file +10 -0

+       mv = DIV_ROUND_UP(min_uv, 1000);
+       uv = mv * 1000;
+       if (uv > max_uv) {
+               vreg_err(vreg, "no set points available in range %d-%d uV\n",
+                       min_uv, max_uv);
+               return -EINVAL;
+       }
+
+       range = vreg->hw_data->voltage_range;
+       *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step);

It seems like you should remove the existing check you have for "if (uv > max_uv)" and replace with a check here. Specifically, it seems like the DIV_ROUND_UP for the selector could also bump you over the max. AKA:

...
bool wait_for_ack;

range = vreg->hw_data->voltage_range;
*selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step); uv = *selector * range->uV_step + range->min_uV
if (uv > max_uv) {
...
}

Hold up, though. Why don't you implement set_voltage_sel() instead of set_voltage()? That's what literally everyone else does, well except PWM regulators. Using that will get rid of most of this code, won't it? Even the check to see if perhaps the voltage isn't changing.

drivers/regulator/qcom_rpmh-regulator.c line 319

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+
+       if (uv == vreg->voltage)
+               return 0;
+
+       wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage;

Do you often see "wait_for_ack = false" in reality? Most regulator usage I've seen requests a fairly tight range. AKA: I don't often see:

set_voltage(min=3000mV, max=3300mV)
set_voltage(min=1800mV, max=3300mV)

Instead, I see:

set_voltage(min=3000mV, max=3300mV)
set_voltage(min=1800mV, max=1900mV)

So you'll always have wait_for_ack = true in the cases I've seen.

...but are you certain it's useful to wait for an ack anyway when the voltage is falling? Most regulators won't guarantee that the voltage has actually fallen even after they ack you. Specifically if a regulator is under light load and it doesn't have an active discharge circuit then the regulator might fall very slowly over time. As a specific example, see commit 7c5209c315ea ("mmc: core: Increase delay for voltage to stabilize from 3.3V to 1.8V").

That was a lot of words, so to sum it all up:

* If you have no actual examples where you see "wait_for_ack = false" then remove this code and always wait.

* If you have evidence that the time spent waiting for the ack is slowing you down, consider always setting wait_for_ack to false when you're lowering the voltage. Anyone who truly cares could just set something like the device tree property regulator-settling-time-down-us. ...or, assuming Mark doesn't hate it, they could set the always-wait-for-ack property in the device tree.

NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy ARC things for this?), we're probably talking about a small handful of voltage transitions per boot, right?

drivers/regulator/qcom_rpmh-regulator.c line 337

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+       return vreg->voltage;

I guess there's no way to read the voltage at bootup? So this will return 0 until someone sets it? ...maybe less of a big deal due to the "qcom,regulator-initial-voltage" property?

Quoted 18 lines

drivers/regulator/qcom_rpmh-regulator.c new file +18 -0

+static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
+                                       unsigned int mode)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
+       };
+       int i, ret;
+
+       if (mode == vreg->mode)
+               return 0;
+
+       for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
+               if (vreg->hw_data->mode_map[i].framework_mode == mode)
+                       break;
+       if (i >= RPMH_REGULATOR_MODE_COUNT) {
+               vreg_err(vreg, "invalid mode=%u\n", mode);
+               return -EINVAL;

drivers/regulator/qcom_rpmh-regulator.c line 363

Quoted 6 lines

drivers/regulator/qcom_rpmh-regulator.c new file +6 -0

+       }
+
+       cmd.data = vreg->hw_data->mode_map[i].pmic_mode;
+
+       ret = rpmh_regulator_send_request(vreg, &cmd, 1,
+                                         mode < vreg->mode || !vreg->mode);

Please explain the "mode < vreg->mode || !vreg->mode" test in words.

drivers/regulator/qcom_rpmh-regulator.c line 378

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+       return vreg->mode;

You'll probably guess that I'd expect you to read this from hardware.

Quoted 24 lines

drivers/regulator/qcom_rpmh-regulator.c new file +24 -0

+/**
+ * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
+ *             for a VRM RPMh resource from device tree
+ * vreg:               Pointer to the rpmh regulator resource
+ *
+ * This function initializes the mode[] array of vreg based upon the values
+ * of optional device tree properties.
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
+{
+       struct device_node *node = vreg->of_node;
+       const struct rpmh_regulator_mode *map;
+       const char *prop;
+       int i, len, ret;
+       u32 *buf;
+
+       map = vreg->hw_data->mode_map;
+       if (!map)
+               return 0;
+
+       /* qcom,allowed-modes is optional */
+       prop = "qcom,allowed-modes";

drivers/regulator/qcom_rpmh-regulator.c line 464

Quoted 3 lines

drivers/regulator/qcom_rpmh-regulator.c new file +3 -0

+       len = of_property_count_elems_of_size(node, prop, sizeof(u32));
+       if (len < 0)
+               return 0;

Seems like it might be worth it to count "qcom,mode-threshold-currents" too and confirm the count is the same? If someone left in an extra threshold current you won't notice otherwise (right?)

drivers/regulator/qcom_rpmh-regulator.c line 467

Quoted 3 lines

drivers/regulator/qcom_rpmh-regulator.c new file +3 -0

+
+       vreg->mode_map = devm_kcalloc(vreg->pmic->dev, len,
+                               sizeof(*vreg->mode_map), GFP_KERNEL);

I keep getting myself confused because you have two things called "mode_map" and they work differently:

vreg->mode_map - contains 1 element per allowed mode. Need to iterate through this to map "framework mode" to "pmic mode". Note: because of the need to iterate this isn't really a "map" in my mind.

vreg->hw_data->mode_map - contains 1 element for each possible "device tree mode". Index into this using "device tree mode" and you get both a "framework mode" and "pmic mode".

IMHO it would be better to have a table like "dt_to_linux_mode" that was just a simple list:

static const int dt_to_linux_mode_bob[] = [
[RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY, [RPMH_REGULATOR_MODE_RET] = -EINVAL, [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
];

static const int dt_to_linux_mode_ldo_smps[] =
[RPMH_REGULATOR_MODE_PASS] = -EINVAL, [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
];

You'd only use that in the "map_mode" functions and when parsing qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could actually just call the map_mode function. This would be especially a good way to do this if you moved "allowed-modes" into the regulator core, which seems like a good idea.

The nice thing about this is that only place you need to conceptually keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing code. Otherwise you just think of the Linux version.

Once you do the above, then your other list could just be named "allowed_modes". This would make it obvious that this isn't a map itself but that you could iterate over it to accomplish a mapping.

Quoted 21 lines

drivers/regulator/qcom_rpmh-regulator.c new file +21 -0

+/**
+ * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated
+ *             with the PMIC and initialize important pointers for each
+ *             regulator
+ * @pmic:              Pointer to the RPMh regulator PMIC
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic)
+{
+       struct device_node *node;
+       int i;
+
+       pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node);
+       if (pmic->vreg_count == 0) {
+               dev_err(pmic->dev, "could not find any regulator subnodes\n");
+               return -ENODEV;
+       }
+
+       pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count,
+                       sizeof(*pmic->vreg), GFP_KERNEL);

drivers/regulator/qcom_rpmh-regulator.c line 559

Quoted 10 lines

drivers/regulator/qcom_rpmh-regulator.c new file +10 -0

+       if (!pmic->vreg)
+               return -ENOMEM;
+
+       i = 0;
+       for_each_available_child_of_node(pmic->dev->of_node, node) {
+               pmic->vreg[i].of_node = node;
+               pmic->vreg[i].pmic = pmic;
+
+               i++;
+       }

While I can believe that things don't crash, you're not quite using for_each_available_child_of_node() correctly. You need to reorganize your code structure to fix. Specifically the "node" that's provided to each iteration only has its refcount held for each iteration. By the end of your function the refcount of all the "of_node"s that you stored wil be 0. Doh.

You could try to fix this by adding a of_node_get() before storing the node and that would work, but that would complicate your error handling. You'll need to do this in a "cleanup" error path of probe and in remove. :(

A better solution is to get rid of the rpmh_regulator_allocate_vreg() function and move the functionality into probe. There, instead of iterating over pmic->vreg_count, just use the for_each_available_child_of_node() function. If rpmh_regulator_init_vreg() you'll have to manually of_node_put() but that should be OK.

Why will that work? You'll call rpmh_regulator_init_vreg() while the reference count is still held. In that function you'll call devm_regulator_register(), which will grab the refcount if it needs it. Since that's a devm_ function you can be sure that it will properly drop the refcount at the right times.

NOTE: with this you presumably should remove the "of_node" from the "struct rpmh_vreg". It seems like it shouldn't be needed anymore and it's good not to keep the pointer around if you didn't call of_node_get() yourself.

Quoted 44 lines

drivers/regulator/qcom_rpmh-regulator.c new file +44 -0

+/**
+ * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
+ *             request for this regulator based on optional device tree
+ *             properties
+ * @vreg:              Pointer to the RPMh regulator
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg)
+{
+       struct tcs_cmd cmd[2] = { };
+       const char *prop;
+       int cmd_count = 0;
+       int ret;
+       u32 temp;
+
+       if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) {
+               prop = "qcom,headroom-voltage";
+               ret = of_property_read_u32(vreg->of_node, prop, &temp);
+               if (!ret) {
+                       if (temp < RPMH_VRM_HEADROOM_MIN_UV ||
+                           temp > RPMH_VRM_HEADROOM_MAX_UV) {
+                               vreg_err(vreg, "%s=%u is invalid\n",
+                                       prop, temp);
+                               return -EINVAL;
+                       }
+                       vreg->headroom_voltage = temp;
+
+                       cmd[cmd_count].addr
+                               = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
+                       cmd[cmd_count++].data
+                               = DIV_ROUND_UP(vreg->headroom_voltage, 1000);
+               }
+
+               prop = "qcom,regulator-initial-voltage";
+               ret = of_property_read_u32(vreg->of_node, prop, &temp);
+               if (!ret) {
+                       if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) {
+                               vreg_err(vreg, "%s=%u is invalid\n",
+                                       prop, temp);
+                               return -EINVAL;
+                       }
+                       vreg->voltage = temp;
+

drivers/regulator/qcom_rpmh-regulator.c line 611

Quoted 4 lines

drivers/regulator/qcom_rpmh-regulator.c new file +4 -0

+                       cmd[cmd_count].addr
+                               = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
+                       cmd[cmd_count++].data
+                               = DIV_ROUND_UP(vreg->voltage, 1000);

It seems like you should set vreg->voltage to the actual result of the division * 1000. AKA: if the user said the initial voltage was 123,456 then vreg->voltage should become 124,000.

Actually, shouldn't you somehow resolve this with "struct rpmh_vreg_hw_data". It seems like some regulators have 128 steps, some have 256 steps, etc. You should have a function that reconciles the requested voltage with the one that hardware will actually provide.

...and, actually, you should share code for this reconciliation with rpmh_regulator_vrm_set_voltage().

Quoted 62 lines

drivers/regulator/qcom_rpmh-regulator.c new file +62 -0

+/**
+ * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator
+ * @vreg:              Pointer to the RPMh regulator
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg)
+{
+       struct device *dev = vreg->pmic->dev;
+       struct regulator_config reg_config = {};
+       const struct rpmh_vreg_init_data *rpmh_data = NULL;
+       const char *type_name = NULL;
+       enum rpmh_regulator_type type;
+       struct regulator_init_data *init_data;
+       int ret, i;
+
+       for (i = 0; i < vreg->pmic->init_data->count; i++) {
+               if (!strcmp(vreg->pmic->init_data->vreg_data[i].name,
+                           vreg->of_node->name)) {
+                       rpmh_data = &vreg->pmic->init_data->vreg_data[i];
+                       break;
+               }
+       }
+
+       if (!rpmh_data) {
+               dev_err(dev, "Unknown regulator %s for %s RPMh regulator PMIC\n",
+                       vreg->of_node->name, vreg->pmic->init_data->name);
+               return -EINVAL;
+       }
+
+       vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d",
+                       rpmh_data->resource_name_base, vreg->pmic->pmic_id,
+                       rpmh_data->id);
+       if (!vreg->resource_name)
+               return -ENOMEM;
+
+       vreg->addr = cmd_db_read_addr(vreg->resource_name);
+       if (!vreg->addr) {
+               vreg_err(vreg, "could not find RPMh address for resource %s\n",
+                       vreg->resource_name);
+               return -ENODEV;
+       }
+
+       vreg->rdesc.name = rpmh_data->name;
+       vreg->rdesc.supply_name = rpmh_data->supply_name;
+       vreg->regulator_type = rpmh_data->regulator_type;
+       vreg->hw_data = rpmh_data->hw_data;
+
+       if (rpmh_data->hw_data->voltage_range) {
+               vreg->rdesc.linear_ranges = rpmh_data->hw_data->voltage_range;
+               vreg->rdesc.n_linear_ranges = 1;
+               vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages;
+       }
+
+       /* Optional override for the default RPMh accelerator type */
+       ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type",
+                                       &type_name);
+       if (!ret) {
+               if (!strcmp("vrm", type_name)) {
+                       vreg->regulator_type = RPMH_REGULATOR_TYPE_VRM;
+               } else if (!strcmp("xob", type_name)) {
+                       vreg->regulator_type = RPMH_REGULATOR_TYPE_XOB;

drivers/regulator/qcom_rpmh-regulator.c line 693

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+               } else {
+                       vreg_err(vreg, "Unknown RPMh accelerator type %s\n",
+                               type_name);
+                       return -EINVAL;
+               }

As per comment in device tree patch, it seems really weird that you can override this. Are you sure?

drivers/regulator/qcom_rpmh-regulator.c line 804

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static const struct rpmh_regulator_mode
+rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = {
+       [RPMH_REGULATOR_MODE_PASS] = {
+               .pmic_mode = 0,
+               .framework_mode = REGULATOR_MODE_STANDBY,

Is "PASS" truly the same concept as the Linux concept of STANDBY. If so, why do you need a separate define for it?

If it truly is the same, it seems like you can simplify everything by just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and "RPMH_REGULATOR_MODE_PASS" and just call it "RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that "standby" maps to "retention" for some regulators and maps to "pass" for other regulators if you want to map PMIC documentation. ...but getting rid of this distinction simply means less error checking and fewer tables in Linux.

If "pass" really shouldn't map to "standby" then this seems like a hack and you should add the concept of "pass" to the core regulator framework.

drivers/regulator/qcom_rpmh-regulator.c line 851

Quoted 3 lines

drivers/regulator/qcom_rpmh-regulator.c new file +3 -0

+static const struct rpmh_vreg_hw_data pmic4_pldo_hw_data = {
+       .voltage_range = &(const struct regulator_linear_range)
+                       REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000),

This seems pretty iffy to me. You're relying on the compiler to make an anonymous chunk of memory with a "struct regulator_linear_range" in it and then storing a pointer to said anonymous chunk of memory? Nobody else using REGULATOR_LINEAR_RANGE() does this.

Why not just get rid of the pointer and put the structure right inside "struct rpmh_vreg_hw_data"? It'll get rid of the weird cast / strange anonymous chunk, save an indirection, and save 4 bytes for a pointer.

Quoted 35 lines

drivers/regulator/qcom_rpmh-regulator.c new file +35 -0

+/**
+ * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each
+ *             of the regulator nodes associated with it
+ * @pdev:              Pointer to the platform device of the RPMh PMIC
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       const struct of_device_id *match;
+       struct rpmh_pmic *pmic;
+       struct device_node *node;
+       int ret, i;
+
+       node = dev->of_node;
+
+       if (!node) {
+               dev_err(dev, "Device tree node is missing\n");
+               return -EINVAL;
+       }
+
+       ret = cmd_db_ready();
+       if (ret < 0) {
+               if (ret != -EPROBE_DEFER)
+                       dev_err(dev, "Command DB not available, ret=%d\n", ret);
+               return ret;
+       }
+
+       pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+       if (!pmic)
+               return -ENOMEM;
+
+       pmic->dev = dev;
+       platform_set_drvdata(pdev, pmic);

drivers/regulator/qcom_rpmh-regulator.c line 1039

Quoted 2 lines

drivers/regulator/qcom_rpmh-regulator.c new file +2 -0

+
+       pmic->rpmh_client = rpmh_get_client(pdev);

It seems like you'd do yourself (and the other clients of rpmh) a favor if you added a devm_rpmh_get_client() in a patch before this one. Adding a devm version of a calls is pretty easy and you'll be able to completely get rid of your "remove" function. ...and get rid of the "cleanup" exit here.

drivers/regulator/qcom_rpmh-regulator.c line 1104

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static struct platform_driver rpmh_regulator_driver = {
+       .driver         = {
+               .name           = "qcom-rpmh-regulator",
+               .of_match_table = rpmh_regulator_match_table,
+               .owner          = THIS_MODULE,

As per the robot, no need to set .owner here. The core will do it.

Quoted 15 lines

drivers/regulator/qcom_rpmh-regulator.c new file +15 -0

+       },
+       .probe          = rpmh_regulator_probe,
+       .remove         = rpmh_regulator_remove,
+};
+
+static int rpmh_regulator_init(void)
+{
+       return platform_driver_register(&rpmh_regulator_driver);
+}
+
+static void rpmh_regulator_exit(void)
+{
+       platform_driver_unregister(&rpmh_regulator_driver);
+}
+

Quoted 1 line

+arch_initcall(rpmh_regulator_init);

I always get yelled at when I try to use arch_initcall() for stuff like this. You should do what everyone else does and use module_platform_driver() to declare your driver. Yeah, regulators are important and (as I remember) they get probed slightly early anyway, but everything else in the system just gotta deal with the fact that they'll sometimes get deferred probes.

include/dt-bindings/regulator/qcom,rpmh-regulator.h

Quoted 9 lines

drivers/regulator/qcom_rpmh-regulator.c new file +4 -0

+module_exit(rpmh_regulator_exit);
+
+MODULE_DESCRIPTION("Qualcomm RPMh regulator driver");
+MODULE_LICENSE("GPL v2");

include/dt-bindings/regulator/qcom,rpmh-regulator.h new file +0 -0

new file mode 100644
index 0000000..f854e0e

Device tree guys will yell at you here. The "include/dt-bindings" bits are supposed to be together with the bindings. Different maintainers have different beliefs here, but I think the way that's least likely to get you yelled at by the most people is:

Patch #1: bindings (.txt and include file)
Patch #2: the driver

...with the idea being that if another operating system wanted just the bindings they could get it all in one patch.

include/dt-bindings/regulator/qcom,rpmh-regulator.h line 8

Quoted 9 lines

include/dt-bindings/regulator/qcom,rpmh-regulator.h new file +8 -0

@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
+
+#ifndef __QCOM_RPMH_REGULATOR_H
+#define __QCOM_RPMH_REGULATOR_H
+
+/*
+ * These mode constants may be used for qcom,allowed-modes and qcom,init-mode

Not "qcom,init-mode". This is actually "regulator-initial-mode" now.

Quoted 6 lines

drivers/regulator/qcom_rpmh-regulator.c line 245

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+       return vreg->enabled;

Quoted 7 lines

You can't read hardware? The regulator framework expects you to read hardware. If it was common not to be able to read hardware then the regulator framework would just keep track of the enabled state for you. Right now if a regulator was left on by the BIOS (presumably some have to be since otherwise you're running on a computer that takes no power), you'll still return false at bootup? Seems non-ideal.

Correct, the RPMh interface provides no mechanism to poll the physical PMIC regulator hardware state.

However, that isn't entirely a bad thing due to the nature of RPMh regulator control. Several processors within the SoC besides the application processor are able to vote on the state of PMIC regulators via RPMh. The VRM and XOB RPMh hardware blocks perform max aggregation across votes from different processors for each votable quantity for a given regulator (enable, voltage, mode, and headroom voltage for VRM; enable for XOB). The aggregated values are then configured in the PMIC regulator via SPMI transactions.

If Linux could read the physical PMIC regulator state and report it back via is_enabled(), get_voltage() and get_mode() regulator ops, then we could get into situations where the applications processor fails to vote on a regulator which is requested by a Linux consumer. For example, say that the modem processor has requested a given regulator to be enabled via RPMh. Later, a Linux consumer driver on the application processor calls regulator_enable() for the regulator. The regulator framework will call the is_enabled() callback which will return true. This would then short-circuit the regulator_enable() call and skip calling the rpmh-regulator enable() callback. This means that the application processor would have no enable vote present within RPMh for the regulator.
If the modem processor then voted to disable the regulator, it would
physically be disabled even though a consumer on the application processor expects it to stay enabled.

Quoted 30 lines

Quoted 11 lines

drivers/regulator/qcom_rpmh-regulator.c new file +11 -0

+static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev,
+                               int min_uv, int max_uv, unsigned int *selector)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
+       };
+       const struct regulator_linear_range *range;
+       int mv, uv, ret;
+       bool wait_for_ack;
+

drivers/regulator/qcom_rpmh-regulator.c line 314

Quoted 10 lines

drivers/regulator/qcom_rpmh-regulator.c new file +10 -0

+       mv = DIV_ROUND_UP(min_uv, 1000);
+       uv = mv * 1000;
+       if (uv > max_uv) {
+               vreg_err(vreg, "no set points available in range %d-%d uV\n",
+                       min_uv, max_uv);
+               return -EINVAL;
+       }
+
+       range = vreg->hw_data->voltage_range;
+       *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step);

It seems like you should remove the existing check you have for "if (uv > max_uv)" and replace with a check here. Specifically, it seems like the DIV_ROUND_UP for the selector could also bump you over the max. AKA:

...
bool wait_for_ack;

Quoted 6 lines

range = vreg->hw_data->voltage_range;
*selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step); uv = *selector * range->uV_step + range->min_uV
if (uv > max_uv) {
...
}

The VRM voltage vote register has units of mV (as does the voltage control register for the PMIC regulators). The PMIC hardware automatically rounds up the mV requests to the next physically available set point. This was designed to simplify the life for software so that there isn't a need to worry about voltage step size which varies between regulator types.

The intention of the mv check was to verify that what is being programmed into VRM is within the requested min_uv-max_uv range. However, since this driver is forced to be aware of the step size, I can modify this as you suggested with the stricter step-size bounding.

Quoted 4 lines

Hold up, though. Why don't you implement set_voltage_sel() instead of set_voltage()? That's what literally everyone else does, well except PWM regulators. Using that will get rid of most of this code, won't it? Even the check to see if perhaps the voltage isn't changing.

There are two cases that I can think of: 1. Having a set_voltage() callback allows for delaying for an RPMh request ACK during certain voltage set point decreasing scenarios (to be elaborated below).

2.
Having a get_voltage() as opposed to get_voltage_sel() callback allows an uninitialized voltage of 0 to be returned in the case that no initial voltage is specified in device tree. This eliminates the possibility of the regulator framework short-circuiting a regulator_set_voltage() call that happens to match the voltage corresponding to selector 0.

Quoted 10 lines

drivers/regulator/qcom_rpmh-regulator.c line 319

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+
+       if (uv == vreg->voltage)
+               return 0;
+
+       wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage;

Do you often see "wait_for_ack = false" in reality? Most regulator usage I've seen requests a fairly tight range. AKA: I don't often see:

Quoted 2 lines

set_voltage(min=3000mV, max=3300mV)
set_voltage(min=1800mV, max=3300mV)

I can't think of a good example off the top of my head. However, this situation is certainly valid and can arise any time that there is a shared rail and the hardware connected to it has minimum voltage requirements per use case but large maximum voltage allowed.

Quoted 7 lines

Instead, I see:

set_voltage(min=3000mV, max=3300mV)
set_voltage(min=1800mV, max=1900mV)

So you'll always have wait_for_ack = true in the cases I've seen.

Quoted 7 lines

...but are you certain it's useful to wait for an ack anyway when the voltage is falling? Most regulators won't guarantee that the voltage has actually fallen even after they ack you. Specifically if a regulator is under light load and it doesn't have an active discharge circuit then the regulator might fall very slowly over time. As a specific example, see commit 7c5209c315ea ("mmc: core: Increase delay for voltage to stabilize from 3.3V to 1.8V").

I'm aware of one important instance in which decreasing voltage needs a
delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case that I had in mind with the 'max_uv < vreg->voltage' check. However, you are correct that hardware will report completion of the voltage slewing early in this case since the comparator is checking for output >= set point. I can remove this special case check.

Quoted 5 lines

That was a lot of words, so to sum it all up:

* If you have no actual examples where you see "wait_for_ack = false" then remove this code and always wait.

Quoted 7 lines

* If you have evidence that the time spent waiting for the ack is slowing you down, consider always setting wait_for_ack to false when you're lowering the voltage. Anyone who truly cares could just set something like the device tree property regulator-settling-time-down-us. ...or, assuming Mark doesn't hate it, they could set the always-wait-for-ack property in the device tree.

I'm ok with changing the logic so that it waits for an ACK when increasing voltage (required for correctness) and does not wait for an ACK when decreasing voltage.

Quoted 3 lines

NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy ARC things for this?), we're probably talking about a small handful of voltage transitions per boot, right?

VRM isn't used for CPU DVFS. I haven't profiled the quantity of RPMh regulator requests made during different use cases.

Quoted 9 lines

drivers/regulator/qcom_rpmh-regulator.c line 337

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+       return vreg->voltage;

I guess there's no way to read the voltage at bootup? So this will return 0 until someone sets it? ...maybe less of a big deal due to the "qcom,regulator-initial-voltage" property?

Correct. As mentioned above, there is no way to read the physical PMIC regulator voltage from RPMh. Yes, this will return 0 unless qcom,regulator-initial-voltage is specified or the set_voltage() regulator op is called. Returning an invalid voltage of 0 uV here is convenient as it ensures that there is no way to accidentally skip setting the voltage due to returning the voltage of selector 0.

Quoted 18 lines

Quoted 18 lines

drivers/regulator/qcom_rpmh-regulator.c new file +18 -0

+static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
+                                       unsigned int mode)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
+       };
+       int i, ret;
+
+       if (mode == vreg->mode)
+               return 0;
+
+       for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
+               if (vreg->hw_data->mode_map[i].framework_mode == mode)
+                       break;
+       if (i >= RPMH_REGULATOR_MODE_COUNT) {
+               vreg_err(vreg, "invalid mode=%u\n", mode);
+               return -EINVAL;

Quoted 8 lines

drivers/regulator/qcom_rpmh-regulator.c line 363

Quoted 6 lines

drivers/regulator/qcom_rpmh-regulator.c new file +6 -0

+       }
+
+       cmd.data = vreg->hw_data->mode_map[i].pmic_mode;
+
+       ret = rpmh_regulator_send_request(vreg, &cmd, 1,
+                                         mode < vreg->mode || !vreg->mode);

Please explain the "mode < vreg->mode || !vreg->mode" test in words.

This waits for an ACK from RPMh of successfully mode transition when switching to a higher power mode (regulator framework modes are ordered with highest power == lowest numerical value) or when setting the first mode.

Quoted 25 lines

IMHO it would be better to have a table like "dt_to_linux_mode" that was just a simple list:

static const int dt_to_linux_mode_bob[] = [
[RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY, [RPMH_REGULATOR_MODE_RET] = -EINVAL, [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
];

static const int dt_to_linux_mode_ldo_smps[] =
[RPMH_REGULATOR_MODE_PASS] = -EINVAL, [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
];

You'd only use that in the "map_mode" functions and when parsing qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could actually just call the map_mode function. This would be especially a good way to do this if you moved "allowed-modes" into the regulator core, which seems like a good idea.

Quoted 3 lines

The nice thing about this is that only place you need to conceptually keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing code. Otherwise you just think of the Linux version.

I would be ok with defining arrays like these to handle the mapping of PMIC regulator hardware agnostic "device tree mode" to "framework mode". However, a second set of arrays would then be needed to map from "framework mode" to hardware specific "pmic mode". I suppose that having the second array indexed by framework mode would make the code a little simpler since iteration would not be needed.

Here is an explanation for why the "device tree mode" abstraction is present in the first place. Between different Qualcomm Technologies, Inc. PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, LPM/PFM, Retention, and pass-through). However, the register values for the same modes vary between different regulator types and different PMIC

families. This patch is adding support for several PMIC4 family PMICs. The values needed for to-be-released PMIC5 PMIC regulators are different. As an example, here are the different values for LPM/PFM across PMIC families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" ensures that it is not possible to inadvertently specify a PMIC specific mode in device tree which corresponds to the wrong type or family but which aliases a value that would be accepted as correct.

Quoted 3 lines

Once you do the above, then your other list could just be named "allowed_modes". This would make it obvious that this isn't a map itself but that you could iterate over it to accomplish a mapping.

I'm concerned with using the name "allowed_modes" as it tends to imply the wrong idea. Perhaps something along the lines of "drms_modes" would be better (to go along with REGULATOR_CHANGE_DRMS). These would be the modes utilized by the set_load() callback along with corresponding threshold load currents.

Quoted 18 lines

drivers/regulator/qcom_rpmh-regulator.c line 804

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static const struct rpmh_regulator_mode
+rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = {
+       [RPMH_REGULATOR_MODE_PASS] = {
+               .pmic_mode = 0,
+               .framework_mode = REGULATOR_MODE_STANDBY,

Is "PASS" truly the same concept as the Linux concept of STANDBY. If so, why do you need a separate define for it?

If it truly is the same, it seems like you can simplify everything by just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and "RPMH_REGULATOR_MODE_PASS" and just call it "RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that "standby" maps to "retention" for some regulators and maps to "pass" for other regulators if you want to map PMIC documentation. ...but getting rid of this distinction simply means less error checking and fewer tables in Linux.

Quoted 3 lines

If "pass" really shouldn't map to "standby" then this seems like a hack and you should add the concept of "pass" to the core regulator framework.

For Qualcomm Technologies, Inc. PMIC regulators, retention mode (RET) is a very low power mode which offers better power effeciency (i.e. lower ground current) than low power mode (LPM) for low load scenarios with the trade off of worse load regulator. It is typically applied when the system is asleep for regulators that must stay on but which have very low and stable load. Retention maps very well to REGULATOR_MODE_STANDBY.

Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected, the BOB output is directly connected to the BOB input (typically Vbat). This does not map exactly to REGULATOR_MODE_STANDBY. I agree that it is somewhat hacky to use it in this way. However, doing so makes qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode

could be handled via get_bypass() and set_bypass() regulator ops. Doing this would require more complicated ops selections in the driver since it could no longer be determined simply by VRM vs XOB, it would instead need to be BOB VRM, other VRM, and XOB. The BOB set_mode() and set_bypass() callbacks would be complicated because both would be writing to the same VRM address (mode control) with bypass==true taking precedence. Additionally, there is currently no way to specify default bypass from DT.
A BOB-specific property would be needed to get this information.

Quoted 15 lines

Quoted 15 lines

drivers/regulator/qcom_rpmh-regulator.c new file +15 -0

+       },
+       .probe          = rpmh_regulator_probe,
+       .remove         = rpmh_regulator_remove,
+};
+
+static int rpmh_regulator_init(void)
+{
+       return platform_driver_register(&rpmh_regulator_driver);
+}
+
+static void rpmh_regulator_exit(void)
+{
+       platform_driver_unregister(&rpmh_regulator_driver);
+}
+

Quoted 8 lines

Quoted 1 line

+arch_initcall(rpmh_regulator_init);

I always get yelled at when I try to use arch_initcall() for stuff like this. You should do what everyone else does and use module_platform_driver() to declare your driver. Yeah, regulators are important and (as I remember) they get probed slightly early anyway, but everything else in the system just gotta deal with the fact that they'll sometimes get deferred probes.

I agree that consumers should handle probe deferral. Unfortunately, reality isn't always so nice. Also, probe deferrals increase boot-up time which is particularly problematic in the mobile space.

I suppose that I
can change this to module_platform_driver() for now. If any major issues arise it could be changed back to arch_initcall().

Note that both qcom_rpm-regulator.c and qcom_smd-regulator.c are using subsys_initcall() right now in place of module_platform_driver().

Quoted 3 lines · David Collins

There are two cases that I can think of: 1. Having a set_voltage() callback allows for delaying for an RPMh request ACK during certain voltage set point decreasing scenarios (to be elaborated below).

Can't you still have a delay in set_voltage_sel()?

Quoted 6 lines

2.
Having a get_voltage() as opposed to get_voltage_sel() callback allows an uninitialized voltage of 0 to be returned in the case that no initial voltage is specified in device tree. This eliminates the possibility of the regulator framework short-circuiting a regulator_set_voltage() call that happens to match the voltage corresponding to selector 0.

Interesting. I suppose you could mix them (have set_voltage_sel() and get_voltage()) as long as you documented why you were doing it. Then we'd have to see if Mark was happy with that...

Quoted 6 lines

I'm aware of one important instance in which decreasing voltage needs a
delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case that I had in mind with the 'max_uv < vreg->voltage' check. However, you are correct that hardware will report completion of the voltage slewing early in this case since the comparator is checking for output >= set point. I can remove this special case check.

Quoted 5 lines

Here is an explanation for why the "device tree mode" abstraction is present in the first place. Between different Qualcomm Technologies, Inc. PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, LPM/PFM, Retention, and pass-through). However, the register values for the same modes vary between different regulator types and different PMIC

Quoted 8 lines

families. This patch is adding support for several PMIC4 family PMICs. The values needed for to-be-released PMIC5 PMIC regulators are different. As an example, here are the different values for LPM/PFM across PMIC families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" ensures that it is not possible to inadvertently specify a PMIC specific mode in device tree which corresponds to the wrong type or family but which aliases a value that would be accepted as correct.

I'm OK with having the "device tree mode" abstraction, and in fact the current regulator framework seems to want you to have this anyway. If I read the code correctly, you're required to have the conversion function and there's no default.

Quoted 3 lines

Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected, the BOB output is directly connected to the BOB input (typically Vbat).

Quoted 3 lines

qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode could be handled via get_bypass() and set_bypass() regulator ops. Doing this would require more complicated ops selections in the driver since it

I've never poked at the get_bypass() / set_bypass(), but it sounds better to me to use them. I'm not a fan of the current hack. Even aside from the bit of hackiness, I'm slightly concerned that some of your logic that generally assumes lower integers = lower power modes would break.

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

+arch_initcall(rpmh_regulator_init);

I always get yelled at when I try to use arch_initcall() for stuff like this. You should do what everyone else does and use

Quoted 3 lines

I agree that consumers should handle probe deferral. Unfortunately, reality isn't always so nice. Also, probe deferrals increase boot-up time which is particularly problematic in the mobile space.

Sigh, yeah. I'm not a fan either. If you can convince Mark that you should use arch_initcall() or subsys_initcall() I won't yell. ...but in the past I've seen others get yelled at.

Note: in actuality it doesn't always increase boot time a whole lot.

...but deferrals _do_ for sure increase the time for certain peripherals to come up, and if those peripherals are things like the LCD displays then it sucks.

Quoted 13 lines

Quoted 13 lines

Here is an explanation for why the "device tree mode" abstraction is present in the first place. Between different Qualcomm Technologies, Inc. PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, LPM/PFM, Retention, and pass-through). However, the register values for the same modes vary between different regulator types and different PMIC families. This patch is adding support for several PMIC4 family PMICs. The values needed for to-be-released PMIC5 PMIC regulators are different. As an example, here are the different values for LPM/PFM across PMIC families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" ensures that it is not possible to inadvertently specify a PMIC specific mode in device tree which corresponds to the wrong type or family but which aliases a value that would be accepted as correct.

Quoted 4 lines

I'm OK with having the "device tree mode" abstraction, and in fact the current regulator framework seems to want you to have this anyway. If I read the code correctly, you're required to have the conversion function and there's no default.

I didn't spot this in the code but something called "device tree mode" sounds like it's going to be awfully confusing...

Assuming I didn't mess up my analysis, the entire job of of_map_mode() is to convert from one integer to another. It should take the number that was specified in the device tree and convert it to a REGULATOR_MODE_XXX. That means that the regulator framework is enforcing a distinct and per-regulator numbering system for the mode (I called this "device tree mode").

So basically it sounds like everyone makes up some arbitrary numbering system that is only used in their device tree files and needs to be mapped into the standard numbering system...

Quoted 7 lines · Doug Anderson

Quoted 3 lines · David Collins

There are two cases that I can think of: 1. Having a set_voltage() callback allows for delaying for an RPMh request ACK during certain voltage set point decreasing scenarios (to be elaborated below).

Can't you still have a delay in set_voltage_sel()?

We have specific support for adding ramp delays, no need to open code it in operations.

Quoted 6 lines

Quoted 6 lines

2.
Having a get_voltage() as opposed to get_voltage_sel() callback allows an uninitialized voltage of 0 to be returned in the case that no initial voltage is specified in device tree. This eliminates the possibility of the regulator framework short-circuiting a regulator_set_voltage() call that happens to match the voltage corresponding to selector 0.

Quoted 3 lines

Interesting. I suppose you could mix them (have set_voltage_sel() and get_voltage()) as long as you documented why you were doing it. Then we'd have to see if Mark was happy with that...

This is a *terrible* idea which will almost certainly break. If the driver can't read values it should return an appropriate error code.

Quoted 6 lines

Quoted 6 lines

I'm aware of one important instance in which decreasing voltage needs a
delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case that I had in mind with the 'max_uv < vreg->voltage' check. However, you are correct that hardware will report completion of the voltage slewing early in this case since the comparator is checking for output >= set point. I can remove this special case check.

You can't usefully wait for voltages to fall, you can never guarantee what the loading on the device is. It's something the user has to manage if they care.

Quoted 13 lines

Quoted 13 lines

Here is an explanation for why the "device tree mode" abstraction is present in the first place. Between different Qualcomm Technologies, Inc. PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, LPM/PFM, Retention, and pass-through). However, the register values for the same modes vary between different regulator types and different PMIC families. This patch is adding support for several PMIC4 family PMICs. The values needed for to-be-released PMIC5 PMIC regulators are different. As an example, here are the different values for LPM/PFM across PMIC families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" ensures that it is not possible to inadvertently specify a PMIC specific mode in device tree which corresponds to the wrong type or family but which aliases a value that would be accepted as correct.

Quoted 4 lines

I'm OK with having the "device tree mode" abstraction, and in fact the current regulator framework seems to want you to have this anyway. If I read the code correctly, you're required to have the conversion function and there's no default.

I didn't spot this in the code but something called "device tree mode" sounds like it's going to be awfully confusing...

Quoted 3 lines

Quoted 3 lines

Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected, the BOB output is directly connected to the BOB input (typically Vbat).

...

Quoted 3 lines

Quoted 3 lines

qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode could be handled via get_bypass() and set_bypass() regulator ops. Doing this would require more complicated ops selections in the driver since it

This is exactly the functionality supported by the bypass operations. Any complexity due to the hardware design is unfortunate but honestly the way the QC regulator stuff is designed they seem like a bit of a lost cause on that front - they look very different to any other hardware we've seen.

Quoted 5 lines

I've never poked at the get_bypass() / set_bypass(), but it sounds better to me to use them. I'm not a fan of the current hack. Even aside from the bit of hackiness, I'm slightly concerned that some of your logic that generally assumes lower integers = lower power modes would break.

Yes, abusing the framework is just going to make things even worse.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

+arch_initcall(rpmh_regulator_init);

I always get yelled at when I try to use arch_initcall() for stuff like this. You should do what everyone else does and use

I agree that consumers should handle probe deferral. Unfortunately, reality isn't always so nice. Also, probe deferrals increase boot-up time which is particularly problematic in the mobile space.

Quoted 3 lines

Sigh, yeah. I'm not a fan either. If you can convince Mark that you should use arch_initcall() or subsys_initcall() I won't yell. ...but in the past I've seen others get yelled at.

Do you have concrete consumers that have a good reason for doing this?

Quoted 1 line

Note: in actuality it doesn't always increase boot time a whole lot.

Note also that we now have the device dependency mechanism that Raphael implemented with the explicit idea that that it'd be used to avoid unneeded deferrals.

Quoted 3 lines

...but deferrals _do_ for sure increase the time for certain peripherals to come up, and if those peripherals are things like the LCD displays then it sucks.

There's been some discussion of allowing the user to specific certain devices to target as priorities for probing which should deal with that.

Quoted 2 lines · Mark Brown

I didn't spot this in the code but something called "device tree mode" sounds like it's going to be awfully confusing...

As I explained in the earlier email, it makes the device tree configurations much simpler and less confusing/error prone. I'd like to keep this concept around unless their are strong objections.

Quoted 26 lines · David Collins

Quoted 6 lines

drivers/regulator/qcom_rpmh-regulator.c line 245

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+       return vreg->enabled;

Quoted 7 lines

You can't read hardware? The regulator framework expects you to read hardware. If it was common not to be able to read hardware then the regulator framework would just keep track of the enabled state for you. Right now if a regulator was left on by the BIOS (presumably some have to be since otherwise you're running on a computer that takes no power), you'll still return false at bootup? Seems non-ideal.

Correct, the RPMh interface provides no mechanism to poll the physical PMIC regulator hardware state.

However, that isn't entirely a bad thing due to the nature of RPMh regulator control. Several processors within the SoC besides the application processor are able to vote on the state of PMIC regulators via RPMh. The VRM and XOB RPMh hardware blocks perform max aggregation across votes from different processors for each votable quantity for a given regulator (enable, voltage, mode, and headroom voltage for VRM; enable for XOB). The aggregated values are then configured in the PMIC regulator via SPMI transactions.

Quoted 14 lines

If Linux could read the physical PMIC regulator state and report it back via is_enabled(), get_voltage() and get_mode() regulator ops, then we could get into situations where the applications processor fails to vote on a regulator which is requested by a Linux consumer. For example, say that the modem processor has requested a given regulator to be enabled via RPMh. Later, a Linux consumer driver on the application processor calls regulator_enable() for the regulator. The regulator framework will call the is_enabled() callback which will return true. This would then short-circuit the regulator_enable() call and skip calling the rpmh-regulator enable() callback. This means that the application processor would have no enable vote present within RPMh for the regulator.
If the modem processor then voted to disable the regulator, it would
physically be disabled even though a consumer on the application processor expects it to stay enabled.

How about if I say it another way: can you read the vote that the boot code (pre-Linux) might have left for this regulator? ...if the boot code didn't init it, then I guess you could return 0, but if it did it would be nice to report it.

AKA: I understand your concerns about other (non-AP) users. I just want to read the state that the BIOS left us in.

Quoted 54 lines

Quoted 30 lines

Quoted 11 lines

drivers/regulator/qcom_rpmh-regulator.c new file +11 -0

+static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev,
+                               int min_uv, int max_uv, unsigned int *selector)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
+       };
+       const struct regulator_linear_range *range;
+       int mv, uv, ret;
+       bool wait_for_ack;
+

drivers/regulator/qcom_rpmh-regulator.c line 314

Quoted 10 lines

drivers/regulator/qcom_rpmh-regulator.c new file +10 -0

+       mv = DIV_ROUND_UP(min_uv, 1000);
+       uv = mv * 1000;
+       if (uv > max_uv) {
+               vreg_err(vreg, "no set points available in range %d-%d uV\n",
+                       min_uv, max_uv);
+               return -EINVAL;
+       }
+
+       range = vreg->hw_data->voltage_range;
+       *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step);

It seems like you should remove the existing check you have for "if (uv > max_uv)" and replace with a check here. Specifically, it seems like the DIV_ROUND_UP for the selector could also bump you over the max. AKA:

...
bool wait_for_ack;

Quoted 6 lines

range = vreg->hw_data->voltage_range;
*selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step); uv = *selector * range->uV_step + range->min_uV
if (uv > max_uv) {
...
}

The VRM voltage vote register has units of mV (as does the voltage control register for the PMIC regulators). The PMIC hardware automatically rounds up the mV requests to the next physically available set point. This was designed to simplify the life for software so that there isn't a need to worry about voltage step size which varies between regulator types.

The intention of the mv check was to verify that what is being programmed into VRM is within the requested min_uv-max_uv range. However, since this driver is forced to be aware of the step size, I can modify this as you suggested with the stricter step-size bounding.

Quoted 4 lines

Hold up, though. Why don't you implement set_voltage_sel() instead of set_voltage()? That's what literally everyone else does, well except PWM regulators. Using that will get rid of most of this code, won't it? Even the check to see if perhaps the voltage isn't changing.

Quoted 3 lines

There are two cases that I can think of: 1. Having a set_voltage() callback allows for delaying for an RPMh request ACK during certain voltage set point decreasing scenarios (to be elaborated below).

Can't you still have a delay in set_voltage_sel()?

Quoted 6 lines

2.
Having a get_voltage() as opposed to get_voltage_sel() callback allows an uninitialized voltage of 0 to be returned in the case that no initial voltage is specified in device tree. This eliminates the possibility of the regulator framework short-circuiting a regulator_set_voltage() call that happens to match the voltage corresponding to selector 0.

Interesting. I suppose you could mix them (have set_voltage_sel() and get_voltage()) as long as you documented why you were doing it. Then we'd have to see if Mark was happy with that...

Quoted 10 lines

Quoted 10 lines

drivers/regulator/qcom_rpmh-regulator.c line 319

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+
+       if (uv == vreg->voltage)
+               return 0;
+
+       wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage;

Do you often see "wait_for_ack = false" in reality? Most regulator usage I've seen requests a fairly tight range. AKA: I don't often see:

Quoted 7 lines

Quoted 2 lines

set_voltage(min=3000mV, max=3300mV)
set_voltage(min=1800mV, max=3300mV)

I can't think of a good example off the top of my head. However, this situation is certainly valid and can arise any time that there is a shared rail and the hardware connected to it has minimum voltage requirements per use case but large maximum voltage allowed.

Yup. I'm not saying it's not allowed, it's just not something I've come across so far. ...but (assuming I read the code correctly) this is the only case your "wait_for_ack" was optimizing for. I don't mind having the optimization if it's needed, I just don't want it if there are no known users.

Quoted 36 lines

Quoted 7 lines

Instead, I see:

set_voltage(min=3000mV, max=3300mV)
set_voltage(min=1800mV, max=1900mV)

So you'll always have wait_for_ack = true in the cases I've seen.

Quoted 7 lines

...but are you certain it's useful to wait for an ack anyway when the voltage is falling? Most regulators won't guarantee that the voltage has actually fallen even after they ack you. Specifically if a regulator is under light load and it doesn't have an active discharge circuit then the regulator might fall very slowly over time. As a specific example, see commit 7c5209c315ea ("mmc: core: Increase delay for voltage to stabilize from 3.3V to 1.8V").

I'm aware of one important instance in which decreasing voltage needs a
delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case that I had in mind with the 'max_uv < vreg->voltage' check. However, you are correct that hardware will report completion of the voltage slewing early in this case since the comparator is checking for output >= set point. I can remove this special case check.

Quoted 5 lines

That was a lot of words, so to sum it all up:

* If you have no actual examples where you see "wait_for_ack = false" then remove this code and always wait.

Quoted 7 lines

* If you have evidence that the time spent waiting for the ack is slowing you down, consider always setting wait_for_ack to false when you're lowering the voltage. Anyone who truly cares could just set something like the device tree property regulator-settling-time-down-us. ...or, assuming Mark doesn't hate it, they could set the always-wait-for-ack property in the device tree.

Quoted 3 lines

I'm ok with changing the logic so that it waits for an ACK when increasing voltage (required for correctness) and does not wait for an ACK when decreasing voltage.

Sounds fine with me. You could even add a comment saying that waiting for an Ack when falling isn't so useful unless the line has some sort of active discharge. You've already got a device tree feature for this, so it would be a good to document it there too.

We'll also have to see if Mark wants this to be more generic in some way. It seems that other regulators could benefit from this type of knowledge and it would be nice if drivers could specify this and have it work on all PMICs.

Quoted 18 lines

Quoted 3 lines

NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy ARC things for this?), we're probably talking about a small handful of voltage transitions per boot, right?

VRM isn't used for CPU DVFS. I haven't profiled the quantity of RPMh regulator requests made during different use cases.

Quoted 9 lines

drivers/regulator/qcom_rpmh-regulator.c line 337

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+       return vreg->voltage;

I guess there's no way to read the voltage at bootup? So this will return 0 until someone sets it? ...maybe less of a big deal due to the "qcom,regulator-initial-voltage" property?

Quoted 6 lines

Correct. As mentioned above, there is no way to read the physical PMIC regulator voltage from RPMh. Yes, this will return 0 unless qcom,regulator-initial-voltage is specified or the set_voltage() regulator op is called. Returning an invalid voltage of 0 uV here is convenient as it ensures that there is no way to accidentally skip setting the voltage due to returning the voltage of selector 0.

I actually wonder if it would be wiser to return an error code if you try to read a regulator that has never been set.

Quoted 27 lines

Quoted 26 lines

Quoted 14 lines

drivers/regulator/qcom_rpmh-regulator.c new file +14 -0

+static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
+                                       unsigned int mode)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
+       };
+       int i, ret;
+
+       if (mode == vreg->mode)
+               return 0;
+
+       for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
+               if (vreg->hw_data->mode_map[i].framework_mode == mode)

drivers/regulator/qcom_rpmh-regulator.c line 363

Quoted 10 lines

drivers/regulator/qcom_rpmh-regulator.c new file +10 -0

+                       break;
+       if (i >= RPMH_REGULATOR_MODE_COUNT) {
+               vreg_err(vreg, "invalid mode=%u\n", mode);
+               return -EINVAL;
+       }
+
+       cmd.data = vreg->hw_data->mode_map[i].pmic_mode;
+
+       ret = rpmh_regulator_send_request(vreg, &cmd, 1,
+                                         mode < vreg->mode || !vreg->mode);

Please explain the "mode < vreg->mode || !vreg->mode" test in words.

Quoted 3 lines

This waits for an ACK from RPMh of successfully mode transition when switching to a higher power mode (regulator framework modes are ordered with highest power == lowest numerical value) or when setting the first mode.

Sorry, I meant: please add this explanation to comments in the function.

Quoted 29 lines

Quoted 28 lines

IMHO it would be better to have a table like "dt_to_linux_mode" that was just a simple list:

static const int dt_to_linux_mode_bob[] = [
[RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY, [RPMH_REGULATOR_MODE_RET] = -EINVAL, [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
];

static const int dt_to_linux_mode_ldo_smps[] =
[RPMH_REGULATOR_MODE_PASS] = -EINVAL, [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
];

You'd only use that in the "map_mode" functions and when parsing qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could actually just call the map_mode function. This would be especially a good way to do this if you moved "allowed-modes" into the regulator core, which seems like a good idea.

The nice thing about this is that only place you need to conceptually keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing code. Otherwise you just think of the Linux version.

Quoted 6 lines

I would be ok with defining arrays like these to handle the mapping of PMIC regulator hardware agnostic "device tree mode" to "framework mode". However, a second set of arrays would then be needed to map from "framework mode" to hardware specific "pmic mode". I suppose that having the second array indexed by framework mode would make the code a little simpler since iteration would not be needed.

Yeah, I was thinking that with 2 arrays the code would be more obvious.

Quoted 5 lines

Here is an explanation for why the "device tree mode" abstraction is present in the first place. Between different Qualcomm Technologies, Inc. PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, LPM/PFM, Retention, and pass-through). However, the register values for the same modes vary between different regulator types and different PMIC

Quoted 8 lines

families. This patch is adding support for several PMIC4 family PMICs. The values needed for to-be-released PMIC5 PMIC regulators are different. As an example, here are the different values for LPM/PFM across PMIC families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" ensures that it is not possible to inadvertently specify a PMIC specific mode in device tree which corresponds to the wrong type or family but which aliases a value that would be accepted as correct.

I'm OK with having the "device tree mode" abstraction, and in fact the current regulator framework seems to want you to have this anyway. If I read the code correctly, you're required to have the conversion function and there's no default.

Quoted 4 lines

Quoted 3 lines

Once you do the above, then your other list could just be named "allowed_modes". This would make it obvious that this isn't a map itself but that you could iterate over it to accomplish a mapping.

Quoted 5 lines

I'm concerned with using the name "allowed_modes" as it tends to imply the wrong idea. Perhaps something along the lines of "drms_modes" would be better (to go along with REGULATOR_CHANGE_DRMS). These would be the modes utilized by the set_load() callback along with corresponding threshold load currents.

Yeah, that name is OK by me. At least I can lookup in the header file to find out that DRMS means "Dynamic Regulator Mode Switching". :)

Quoted 22 lines

Quoted 18 lines

drivers/regulator/qcom_rpmh-regulator.c line 804

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static const struct rpmh_regulator_mode
+rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = {
+       [RPMH_REGULATOR_MODE_PASS] = {
+               .pmic_mode = 0,
+               .framework_mode = REGULATOR_MODE_STANDBY,

Is "PASS" truly the same concept as the Linux concept of STANDBY. If so, why do you need a separate define for it?

If it truly is the same, it seems like you can simplify everything by just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and "RPMH_REGULATOR_MODE_PASS" and just call it "RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that "standby" maps to "retention" for some regulators and maps to "pass" for other regulators if you want to map PMIC documentation. ...but getting rid of this distinction simply means less error checking and fewer tables in Linux.

Quoted 3 lines

If "pass" really shouldn't map to "standby" then this seems like a hack and you should add the concept of "pass" to the core regulator framework.

Quoted 6 lines

For Qualcomm Technologies, Inc. PMIC regulators, retention mode (RET) is a very low power mode which offers better power effeciency (i.e. lower ground current) than low power mode (LPM) for low load scenarios with the trade off of worse load regulator. It is typically applied when the system is asleep for regulators that must stay on but which have very low and stable load. Retention maps very well to REGULATOR_MODE_STANDBY.

Yup, this seems like a perfect mapping between the Qualcomm mode and the regulator framework.

Quoted 6 lines

Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected, the BOB output is directly connected to the BOB input (typically Vbat). This does not map exactly to REGULATOR_MODE_STANDBY. I agree that it is somewhat hacky to use it in this way. However, doing so makes qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode

Quoted 8 lines

could be handled via get_bypass() and set_bypass() regulator ops. Doing this would require more complicated ops selections in the driver since it could no longer be determined simply by VRM vs XOB, it would instead need to be BOB VRM, other VRM, and XOB. The BOB set_mode() and set_bypass() callbacks would be complicated because both would be writing to the same VRM address (mode control) with bypass==true taking precedence. Additionally, there is currently no way to specify default bypass from DT.
A BOB-specific property would be needed to get this information.

I've never poked at the get_bypass() / set_bypass(), but it sounds better to me to use them. I'm not a fan of the current hack. Even aside from the bit of hackiness, I'm slightly concerned that some of your logic that generally assumes lower integers = lower power modes would break.

For instance in rpmh_regulator_vrm_set_mode() switching to "PASS" would look like you're switching to a low power mode so you'd skip the "wait for ack", right? I could sorta imagine things getting confused when trying to specify the mA load and having it switch modes automatically.

I'd also notice that "regulator/qcom_spmi-regulator.c" seems to be using get_bypass() / set_bypass(), so maybe qcom-related clients will expect this?

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

drivers/regulator/qcom_rpmh-regulator.c new file +15 -0

+       },
+       .probe          = rpmh_regulator_probe,
+       .remove         = rpmh_regulator_remove,
+};
+
+static int rpmh_regulator_init(void)
+{
+       return platform_driver_register(&rpmh_regulator_driver);
+}
+
+static void rpmh_regulator_exit(void)
+{
+       platform_driver_unregister(&rpmh_regulator_driver);
+}
+

Quoted 8 lines

Quoted 1 line

+arch_initcall(rpmh_regulator_init);

I always get yelled at when I try to use arch_initcall() for stuff like this. You should do what everyone else does and use module_platform_driver() to declare your driver. Yeah, regulators are important and (as I remember) they get probed slightly early anyway, but everything else in the system just gotta deal with the fact that they'll sometimes get deferred probes.

Quoted 3 lines

I agree that consumers should handle probe deferral. Unfortunately, reality isn't always so nice. Also, probe deferrals increase boot-up time which is particularly problematic in the mobile space.

Sigh, yeah. I'm not a fan either. If you can convince Mark that you should use arch_initcall() or subsys_initcall() I won't yell. ...but in the past I've seen others get yelled at.

Note: in actuality it doesn't always increase boot time a whole lot. In theory as long as the CPU is running full bore and you're not killing parallelism too much then the only "waste" is the bit of time to run the start of the probe. It's not much. Every time someone claims "but my boot is slow because of deferrals" others say "where is the evidence?" and I don't remember a whole lot of evidence being presented. Maybe you have evidence?

...but deferrals _do_ for sure increase the time for certain peripherals to come up, and if those peripherals are things like the LCD displays then it sucks.

Quoted 3 lines

I suppose that I
can change this to module_platform_driver() for now. If any major issues arise it could be changed back to arch_initcall().

Probably wise.

Quoted 2 lines

Note that both qcom_rpm-regulator.c and qcom_smd-regulator.c are using subsys_initcall() right now in place of module_platform_driver().

Yeah, I've made the argument before and been told "they are grandfathered in."

drivers/regulator/Kconfig +6 -0

+       help
+         This driver supports control of PMIC regulators via the RPMh hardware
+         block found on Qualcomm Technologies Inc. SoCs.  RPMh regulator
+         control allows for voting on regulator state between multiple
+         processors within the SoC.
+
 config REGULATOR_QCOM_SMD_RPM
        tristate "Qualcomm SMD based RPM regulator driver"
        depends on QCOM_SMD_RPM

drivers/regulator/qcom_rpmh-regulator.c new file +6 -0

new file mode 100644
index 0000000..808f949
@@ -0,0 +1,1124 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/bitops.h>

drivers/regulator/qcom_rpmh-regulator.c new file +19 -0

+#include <linux/regulator/of_regulator.h>
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
+
+/**
+ * enum rpmh_regulator_type - supported RPMh accelerator types
+ * %RPMH_REGULATOR_TYPE_VRM:   RPMh VRM accelerator which supports voting on
+ *                             enable, voltage, mode, and headroom voltage of
+ *                             LDO, SMPS, VS, and BOB type PMIC regulators.
+ * %RPMH_REGULATOR_TYPE_XOB:   RPMh XOB accelerator which supports voting on
+ *                             the enable state of PMIC regulators.
+ */
+enum rpmh_regulator_type {
+       RPMH_REGULATOR_TYPE_VRM,
+       RPMH_REGULATOR_TYPE_XOB,
+};
+
+/* Min and max limits of VRM resource request parameters */

drivers/regulator/qcom_rpmh-regulator.c new file +45 -0

+#define RPMH_REGULATOR_MODE_COUNT              5
+
+/**
+ * struct rpmh_regulator_mode - RPMh VRM mode attributes
+ * @pmic_mode:                 Raw PMIC mode value written into VRM mode voting
+ *                             register (i.e. RPMH_REGULATOR_MODE_*)
+ * @framework_mode:            Regulator framework mode value
+ *                             (i.e. REGULATOR_MODE_*)
+ * @min_load_ua:               The minimum load current in microamps which
+ *                             would utilize this mode
+ *
+ * Software selects the lowest mode for which aggr_load_ua >= min_load_ua.
+ */
+struct rpmh_regulator_mode {
+       u32                             pmic_mode;
+       u32                             framework_mode;
+       int                             min_load_ua;
+};
+
+/**
+ * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
+ * @mode_map:                  Array of size RPMH_REGULATOR_MODE_COUNT which
+ *                             maps RPMH_REGULATOR_MODE_* indices into PMIC
+ *                             mode and regulator framework mode that are
+ *                             supported by this PMIC regulator type
+ * @voltage_range:             The single range of voltages supported by this
+ *                             PMIC regulator type
+ * @n_voltages:                        The number of unique voltage set points defined
+ *                             by voltage_range
+ * @of_map_mode:               Maps an RPMH_REGULATOR_MODE_* mode value defined
+ *                             in device tree to a regulator framework mode
+ */
+struct rpmh_vreg_hw_data {
+       const struct rpmh_regulator_mode        *mode_map;
+       const struct regulator_linear_range     *voltage_range;
+       int                                     n_voltages;
+       unsigned int                          (*of_map_mode)(unsigned int mode);
+};
+
+struct rpmh_pmic;
+
+/**
+ * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
+ *             single regulator device
+ * @of_node:                   Device tree node pointer of the regulator

drivers/regulator/qcom_rpmh-regulator.c new file +4 -0

+ * @hw_data:                   PMIC regulator configuration data for this RPMh
+ *                             regulator
+ * @regulator_type:            RPMh accelerator type for this regulator
+ *                             resource

drivers/regulator/qcom_rpmh-regulator.c new file +35 -0

+ * @supply_name:               Parent supply regulator name
+ * @id:                                Regulator number within the PMIC
+ * @regulator_type:            RPMh accelerator type used to manage this
+ *                             regulator
+ * @hw_data:                   Configuration data for this PMIC regulator type
+ */
+struct rpmh_vreg_init_data {
+       const char                      *name;
+       const char                      *resource_name_base;
+       const char                      *supply_name;
+       int                             id;
+       enum rpmh_regulator_type        regulator_type;
+       const struct rpmh_vreg_hw_data  *hw_data;
+};
+
+/**
+ * struct rpmh_pmic_init_data - initialization data for a PMIC
+ * @name:                      PMIC name
+ * @vreg_data:                 Array of data for each regulator in the PMIC
+ * @count:                     Number of entries in vreg_data
+ */
+struct rpmh_pmic_init_data {
+       const char                              *name;
+       const struct rpmh_vreg_init_data        *vreg_data;
+       int                                     count;
+};
+
+/**
+ * struct rpmh_pmic - top level data structure of all regulators found on a PMIC
+ * @dev:                       Device pointer of the PMIC device for the
+ *                             regulators
+ * @rpmh_client:               Handle used for rpmh communications
+ * @vreg:                      Array of rpmh regulator structs representing the
+ *                             individual regulators found on this PMIC chip
+ *                             which are configured via device tree.

drivers/regulator/qcom_rpmh-regulator.c new file +39 -0

+};
+
+#define vreg_err(vreg, message, ...) \
+       pr_err("%s %s: " message, (vreg)->pmic->init_data->name, \
+               (vreg)->rdesc.name, ##__VA_ARGS__)
+#define vreg_info(vreg, message, ...) \
+       pr_info("%s %s: " message, (vreg)->pmic->init_data->name, \
+               (vreg)->rdesc.name, ##__VA_ARGS__)
+#define vreg_debug(vreg, message, ...) \
+       pr_debug("%s %s: " message, (vreg)->pmic->init_data->name, \
+               (vreg)->rdesc.name, ##__VA_ARGS__)
+
+/**
+ * rpmh_regulator_send_request() - send the request to RPMh
+ * @vreg:              Pointer to the RPMh regulator
+ * @cmd:               RPMh commands to send
+ * @count:             Size of cmd array
+ * @wait_for_ack:      Boolean indicating if execution must wait until the
+ *                     request has been acknowledged as complete
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_send_request(struct rpmh_vreg *vreg,
+                       struct tcs_cmd *cmd, int count, bool wait_for_ack)
+{
+       int ret;
+
+       if (wait_for_ack || vreg->always_wait_for_ack)
+               ret = rpmh_write(vreg->pmic->rpmh_client,
+                               RPMH_ACTIVE_ONLY_STATE, cmd, count);
+       else
+               ret = rpmh_write_async(vreg->pmic->rpmh_client,
+                               RPMH_ACTIVE_ONLY_STATE, cmd, count);
+       if (ret < 0)
+               vreg_err(vreg, "rpmh_write() failed, ret=%d\n", ret);
+
+       return ret;
+}
+

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+static int rpmh_regulator_enable(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,

drivers/regulator/qcom_rpmh-regulator.c new file +7 -0

+               return ret;
+       }
+
+       vreg->enabled = true;
+
+       return 0;
+}

+ * mode_count = 3
+ * mode_map[].min_load_ua = 0, 100000, 6000000
+ *

+ *     load_ua = 10000   --> i = 0
+ *     load_ua = 250000  --> i = 1
+ *     load_ua = 7000000 --> i = 2

+ *
+ * Return: 0 on success, errno on failure
+ */

drivers/regulator/qcom_rpmh-regulator.c new file +39 -0

+static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_ua)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       int i;
+
+       /* No need to check element 0 as it will be the default. */
+       for (i = vreg->mode_count - 1; i > 0; i--)
+               if (vreg->mode_map[i].min_load_ua <= load_ua)
+                       break;
+
+       return rpmh_regulator_vrm_set_mode(rdev,
+                                          vreg->mode_map[i].framework_mode);
+}
+
+static const struct regulator_ops rpmh_regulator_vrm_ops = {
+       .enable                 = rpmh_regulator_enable,
+       .disable                = rpmh_regulator_disable,
+       .is_enabled             = rpmh_regulator_is_enabled,
+       .set_voltage            = rpmh_regulator_vrm_set_voltage,
+       .get_voltage            = rpmh_regulator_vrm_get_voltage,
+       .list_voltage           = regulator_list_voltage_linear_range,
+       .set_mode               = rpmh_regulator_vrm_set_mode,
+       .get_mode               = rpmh_regulator_vrm_get_mode,
+       .set_load               = rpmh_regulator_vrm_set_load,
+};
+
+static const struct regulator_ops rpmh_regulator_xob_ops = {
+       .enable                 = rpmh_regulator_enable,
+       .disable                = rpmh_regulator_disable,
+       .is_enabled             = rpmh_regulator_is_enabled,
+};
+
+static const struct regulator_ops *rpmh_regulator_ops[] = {
+       [RPMH_REGULATOR_TYPE_VRM]       = &rpmh_regulator_vrm_ops,
+       [RPMH_REGULATOR_TYPE_XOB]       = &rpmh_regulator_xob_ops,
+};
+
+/**
+ * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations

drivers/regulator/qcom_rpmh-regulator.c new file +92 -0

+       struct device_node *node = vreg->of_node;
+       const struct rpmh_regulator_mode *map;
+       const char *prop;
+       int i, len, ret;
+       u32 *buf;
+
+       map = vreg->hw_data->mode_map;
+       if (!map)
+               return 0;
+
+       /* qcom,allowed-modes is optional */
+       prop = "qcom,allowed-modes";
+       len = of_property_count_elems_of_size(node, prop, sizeof(u32));
+       if (len < 0)
+               return 0;
+
+       vreg->mode_map = devm_kcalloc(vreg->pmic->dev, len,
+                               sizeof(*vreg->mode_map), GFP_KERNEL);
+       if (!vreg->mode_map)
+               return -ENOMEM;
+       vreg->mode_count = len;
+
+       buf = kcalloc(len, sizeof(*buf), GFP_KERNEL);
+       if (!buf)
+               return -ENOMEM;
+
+       ret = of_property_read_u32_array(node, prop, buf, len);
+       if (ret < 0) {
+               vreg_err(vreg, "unable to read %s, ret=%d\n",
+                       prop, ret);
+               goto done;
+       }
+
+       for (i = 0; i < len; i++) {
+               if (buf[i] >= RPMH_REGULATOR_MODE_COUNT
+                   || !map[buf[i]].framework_mode) {
+                       vreg_err(vreg, "element %d of %s = %u is invalid for this regulator\n",
+                               i, prop, buf[i]);
+                       ret = -EINVAL;
+                       goto done;
+               }
+
+               vreg->mode_map[i].pmic_mode = map[buf[i]].pmic_mode;
+               vreg->mode_map[i].framework_mode = map[buf[i]].framework_mode;
+
+               if (i > 0 && vreg->mode_map[i].pmic_mode
+                               <= vreg->mode_map[i - 1].pmic_mode) {
+                       vreg_err(vreg, "%s elements are not in ascending order\n",
+                               prop);
+                       ret = -EINVAL;
+                       goto done;
+               }
+       }
+
+       prop = "qcom,mode-threshold-currents";
+       ret = of_property_read_u32_array(node, prop, buf, len);
+       if (ret < 0) {
+               vreg_err(vreg, "unable to read %s, ret=%d\n",
+                       prop, ret);
+               goto done;
+       }
+
+       for (i = 0; i < len; i++) {
+               vreg->mode_map[i].min_load_ua = buf[i];
+
+               if (i > 0 && vreg->mode_map[i].min_load_ua
+                               <= vreg->mode_map[i - 1].min_load_ua) {
+                       vreg_err(vreg, "%s elements are not in ascending order\n",
+                               prop);
+                       ret = -EINVAL;
+                       goto done;
+               }
+       }
+
+done:
+       kfree(buf);
+       return ret;
+}
+
+/**
+ * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated
+ *             with the PMIC and initialize important pointers for each
+ *             regulator
+ * @pmic:              Pointer to the RPMh regulator PMIC
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic)
+{
+       struct device_node *node;
+       int i;
+

drivers/regulator/qcom_rpmh-regulator.c new file +20 -0

+
+       i = 0;
+       for_each_available_child_of_node(pmic->dev->of_node, node) {
+               pmic->vreg[i].of_node = node;
+               pmic->vreg[i].pmic = pmic;
+
+               i++;
+       }
+
+       return 0;
+}
+
+/**
+ * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
+ *             request for this regulator based on optional device tree
+ *             properties
+ * @vreg:              Pointer to the RPMh regulator
+ *
+ * Return: 0 on success, errno on failure
+ */

drivers/regulator/qcom_rpmh-regulator.c new file +7 -0

+               ret = of_property_read_u32(vreg->of_node, prop, &temp);
+               if (!ret) {
+                       if (temp < RPMH_VRM_HEADROOM_MIN_UV ||
+                           temp > RPMH_VRM_HEADROOM_MAX_UV) {
+                               vreg_err(vreg, "%s=%u is invalid\n",
+                                       prop, temp);
+                               return -EINVAL;

drivers/regulator/qcom_rpmh-regulator.c new file +20 -0

+               ret = of_property_read_u32(vreg->of_node, prop, &temp);
+               if (!ret) {
+                       if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) {
+                               vreg_err(vreg, "%s=%u is invalid\n",
+                                       prop, temp);
+                               return -EINVAL;
+                       }
+                       vreg->voltage = temp;
+
+                       cmd[cmd_count].addr
+                               = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
+                       cmd[cmd_count++].data
+                               = DIV_ROUND_UP(vreg->voltage, 1000);
+               }
+       }
+
+       if (cmd_count) {
+               ret = rpmh_regulator_send_request(vreg, cmd, cmd_count, true);
+               if (ret < 0) {
+                       vreg_err(vreg, "could not send default config, ret=%d\n",

+ * @vreg: Pointer to the RPMh regulator
+ *
+ * Return: 0 on success, errno on failure
+ */

drivers/regulator/qcom_rpmh-regulator.c new file +19 -0

+static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg)
+{
+       struct device *dev = vreg->pmic->dev;
+       struct regulator_config reg_config = {};
+       const struct rpmh_vreg_init_data *rpmh_data = NULL;
+       const char *type_name = NULL;
+       enum rpmh_regulator_type type;
+       struct regulator_init_data *init_data;
+       int ret, i;
+
+       for (i = 0; i < vreg->pmic->init_data->count; i++) {
+               if (!strcmp(vreg->pmic->init_data->vreg_data[i].name,
+                           vreg->of_node->name)) {
+                       rpmh_data = &vreg->pmic->init_data->vreg_data[i];
+                       break;
+               }
+       }
+
+       if (!rpmh_data) {

drivers/regulator/qcom_rpmh-regulator.c new file +11 -0

+
+       vreg->addr = cmd_db_read_addr(vreg->resource_name);
+       if (!vreg->addr) {
+               vreg_err(vreg, "could not find RPMh address for resource %s\n",
+                       vreg->resource_name);
+               return -ENODEV;
+       }
+
+       vreg->rdesc.name = rpmh_data->name;
+       vreg->rdesc.supply_name = rpmh_data->supply_name;
+       vreg->regulator_type = rpmh_data->regulator_type;

drivers/regulator/qcom_rpmh-regulator.c new file +32 -0

+                                       &type_name);
+       if (!ret) {
+               if (!strcmp("vrm", type_name)) {
+                       vreg->regulator_type = RPMH_REGULATOR_TYPE_VRM;
+               } else if (!strcmp("xob", type_name)) {
+                       vreg->regulator_type = RPMH_REGULATOR_TYPE_XOB;
+               } else {
+                       vreg_err(vreg, "Unknown RPMh accelerator type %s\n",
+                               type_name);
+                       return -EINVAL;
+               }
+       }
+
+       type = vreg->regulator_type;
+
+       if (type == RPMH_REGULATOR_TYPE_VRM) {
+               ret = rpmh_regulator_parse_vrm_modes(vreg);
+               if (ret < 0) {
+                       vreg_err(vreg, "could not parse vrm mode mapping, ret=%d\n",
+                               ret);
+                       return ret;
+               }
+       }
+
+       vreg->always_wait_for_ack = of_property_read_bool(vreg->of_node,
+                                               "qcom,always-wait-for-ack");
+
+       vreg->rdesc.owner       = THIS_MODULE;
+       vreg->rdesc.type        = REGULATOR_VOLTAGE;
+       vreg->rdesc.ops         = rpmh_regulator_ops[type];
+       vreg->rdesc.of_map_mode = vreg->hw_data->of_map_mode;
+

drivers/regulator/qcom_rpmh-regulator.c new file +32 -0

+               for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
+                       init_data->constraints.valid_modes_mask
+                               |= vreg->hw_data->mode_map[i].framework_mode;
+       }
+
+       reg_config.dev                  = dev;
+       reg_config.init_data            = init_data;
+       reg_config.of_node              = vreg->of_node;
+       reg_config.driver_data          = vreg;
+
+       ret = rpmh_regulator_load_default_parameters(vreg);
+       if (ret < 0) {
+               vreg_err(vreg, "unable to load default parameters, ret=%d\n",
+                       ret);
+               return ret;
+       }
+
+       vreg->rdev = devm_regulator_register(dev, &vreg->rdesc, &reg_config);
+       if (IS_ERR(vreg->rdev)) {
+               ret = PTR_ERR(vreg->rdev);
+               vreg->rdev = NULL;
+               vreg_err(vreg, "devm_regulator_register() failed, ret=%d\n",
+                       ret);
+               return ret;
+       }
+
+       vreg_debug(vreg, "registered RPMh resource %s @ 0x%05X\n",
+               vreg->resource_name, vreg->addr);
+
+       return ret;
+}
+

drivers/regulator/qcom_rpmh-regulator.c new file +7 -0

+
+static const struct of_device_id rpmh_regulator_match_table[] = {
+       {
+               .compatible = "qcom,pm8998-rpmh-regulators",
+               .data = &pm8998_pmic_data,
+       },
+       {

drivers/regulator/qcom_rpmh-regulator.c new file +12 -0

+
+/**
+ * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each
+ *             of the regulator nodes associated with it
+ * @pdev:              Pointer to the platform device of the RPMh PMIC
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       const struct of_device_id *match;

drivers/regulator/qcom_rpmh-regulator.c new file +4 -0

+
+       pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+       if (!pmic)
+               return -ENOMEM;

drivers/regulator/qcom_rpmh-regulator.c new file +5 -0

+                       ret);
+               goto cleanup;
+       }
+
+       for (i = 0; i < pmic->vreg_count; i++) {

drivers/regulator/qcom_rpmh-regulator.c new file +4 -0

+
+       return ret;
+
+cleanup:

drivers/regulator/qcom_rpmh-regulator.c new file +3 -0

+
+       return 0;
+}

Quoting David Collins (2018-03-16 18:09:10)

Quoted 8 lines

drivers/regulator/Kconfig +0 -0

index 097f617..e0ecd0a 100644
@@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
          Qualcomm RPM as a module. The module will be named
          "qcom_rpm-regulator".
 

drivers/regulator/Kconfig config REGULATOR_QCOM_RPM · line 676

Quoted 3 lines

drivers/regulator/Kconfig +3 -0

+config REGULATOR_QCOM_RPMH
+       tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
+       depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST

What's the build dependency on OF?

Quoted 1 line

drivers/regulator/qcom_rpmh-regulator.c +0 -0

drivers/regulator/qcom_rpmh-regulator.c line 16

Quoted 3 lines

drivers/regulator/qcom_rpmh-regulator.c +3 -0

+#include <linux/string.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>

Including machine is usually a red flag in regulator drivers.

Quoted 1 line

+/* Register offsets: */

Why the colon? ^
Why even have the comment?

drivers/regulator/qcom_rpmh-regulator.c line 55

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c +5 -0

+/* Enable register values: */
+#define RPMH_REGULATOR_DISABLE                 0x0
+#define RPMH_REGULATOR_ENABLE                  0x1
+
+/* Number of unique hardware modes supported: */

Both above also look useless.

Quoted 2 lines

+ * @rdev: Regulator device pointer returned by
+ * devm_regulator_register()

Is this used? Why save it around?

Quoted 1 line

+ * @voltage: RPMh VRM regulator voltage in microvolts

So call it uV?

Quoted 3 lines

+ * @mode:                      RPMh VRM regulator current framework mode
+ * @headroom_voltage:          RPMh VRM regulator minimum headroom voltage
+ *                             required

headroom_uV?

drivers/regulator/qcom_rpmh-regulator.c line 140

Quoted 12 lines

drivers/regulator/qcom_rpmh-regulator.c +12 -0

+struct rpmh_vreg {
+       struct device_node              *of_node;
+       struct rpmh_pmic                *pmic;
+       const char                      *resource_name;
+       u32                             addr;
+       struct regulator_desc           rdesc;
+       struct regulator_dev            *rdev;
+       const struct rpmh_vreg_hw_data  *hw_data;
+       enum rpmh_regulator_type        regulator_type;
+       bool                            always_wait_for_ack;
+       struct rpmh_regulator_mode      *mode_map;
+       int                             mode_count;

size_t?

drivers/regulator/qcom_rpmh-regulator.c line 145

Quoted 5 lines

drivers/regulator/qcom_rpmh-regulator.c +5 -0

+
+       bool                            enabled;
+       int                             voltage;
+       unsigned int                    mode;
+       int                             headroom_voltage;

Please try to limit the things that are assigned into these structs and then never used outside of init. It adds complexity to the code when a local variable in the function would work just as well.

Quoted 5 lines

+ * struct rpmh_vreg_init_data - initialization data for an RPMh regulator
+ * @name: Name for the regulator which also corresponds
+ * to the device tree subnode name of the regulator
+ * @resource_name_base: RPMh regulator resource name prefix. E.g.
+ * "ldo" for RPMh resource "ldoa1".

Maybe it should be "ldo%c1"? Then we could kasprintf the name with the pmic_id and drop the 'id' member entirely.

drivers/regulator/qcom_rpmh-regulator.c line 198

Quoted 4 lines

drivers/regulator/qcom_rpmh-regulator.c +4 -0

+struct rpmh_pmic {
+       struct device                           *dev;
+       struct rpmh_client                      *rpmh_client;
+       struct rpmh_vreg                        *vreg;

It's a circle! Life is a circle! It's a circle!

drivers/regulator/qcom_rpmh-regulator.c line 201

Quoted 3 lines

drivers/regulator/qcom_rpmh-regulator.c +3 -0

+       int                                     vreg_count;
+       const char                              *pmic_id;
+       const struct rpmh_pmic_init_data        *init_data;

Hopefully we don't really need this entire struct and we can just use local variables instead.

Quoted 11 lines

drivers/regulator/qcom_rpmh-regulator.c +11 -0

+static int rpmh_regulator_enable(struct regulator_dev *rdev)
+{
+       struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+       struct tcs_cmd cmd = {
+               .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+               .data = RPMH_REGULATOR_ENABLE,
+       };
+       int ret;
+
+       if (vreg->enabled)
+               return 0;

drivers/regulator/qcom_rpmh-regulator.c line 262

Quoted 4 lines

drivers/regulator/qcom_rpmh-regulator.c +4 -0

+
+       ret = rpmh_regulator_send_request(vreg, &cmd, 1, true);
+       if (ret < 0) {
+               vreg_err(vreg, "enable failed, ret=%d\n", ret);

Do we really need all these error messages deep down in the regulator drivers? If consumers care, they'll print some error message themselves anyway.

Quoted 11 lines

+/**
+ * rpmh_regulator_vrm_set_load() - set the PMIC mode based upon the maximum load
+ * required from the VRM rpmh-regulator
+ * @rdev: Regulator device pointer for the rpmh-regulator
+ * @load_ua: Maximum current required from all consumers in microamps
+ *
+ * This function is passed as a callback function into the regulator ops that
+ * are registered for each VRM rpmh-regulator device.
+ *
+ * This function sets the mode of the regulator to that which has the highest
+ * min support load less than or equal to load_ua. Example:

s/support/supported/

Quoted 7 lines

+/**
+ * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
+ * for a VRM RPMh resource from device tree
+ * vreg: Pointer to the rpmh regulator resource
+ *
+ * This function initializes the mode[] array of vreg based upon the values
+ * of optional device tree properties.