* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-14 2:50 ` David Collins
0 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-14 2:50 UTC (permalink / raw)
To: linux-arm-kernel
Add the QCOM RPMh regulator driver to manage PMIC regulators
which are controlled via RPMh on some Qualcomm Technologies, Inc.
SoCs. RPMh is a hardware block which contains several
accelerators which are used to manage various hardware resources
that are shared between the processors of the SoC. The final
hardware state of a regulator is determined within RPMh by
performing max aggregation of the requests made by all of the
processors.
Add support for PMIC regulator control via the voltage regulator
manager (VRM) and oscillator buffer (XOB) RPMh accelerators. VRM
supports manipulation of enable state, voltage, mode, and
headroom voltage. XOB supports manipulation of enable state.
Signed-off-by: David Collins <collinsd@codeaurora.org>
---
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom_rpmh-regulator.c | 910 ++++++++++++++++++++++++++++++++
3 files changed, 920 insertions(+)
create mode 100644 drivers/regulator/qcom_rpmh-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 097f617..e0ecd0a 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
Qualcomm RPM as a module. The module will be named
"qcom_rpm-regulator".
+config REGULATOR_QCOM_RPMH
+ tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
+ depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
+ 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
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 590674f..c2274dd 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o
obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom_rpmh-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o
obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
diff --git a/drivers/regulator/qcom_rpmh-regulator.c b/drivers/regulator/qcom_rpmh-regulator.c
new file mode 100644
index 0000000..03b1301
--- /dev/null
+++ b/drivers/regulator/qcom_rpmh-regulator.c
@@ -0,0 +1,910 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#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
+ * %VRM: RPMh VRM accelerator which supports voting on enable, voltage,
+ * mode, and headroom voltage of LDO, SMPS, and BOB type PMIC
+ * regulators.
+ * %XOB: RPMh XOB accelerator which supports voting on the enable state
+ * of PMIC regulators.
+ */
+enum rpmh_regulator_type {
+ VRM,
+ XOB,
+};
+
+#define RPMH_VRM_HEADROOM_MAX_UV 511000
+
+#define RPMH_REGULATOR_REG_VRM_VOLTAGE 0x0
+#define RPMH_REGULATOR_REG_ENABLE 0x4
+#define RPMH_REGULATOR_REG_VRM_MODE 0x8
+#define RPMH_REGULATOR_REG_VRM_HEADROOM 0xC
+
+#define RPMH_REGULATOR_DISABLE 0x0
+#define RPMH_REGULATOR_ENABLE 0x1
+
+#define RPMH_REGULATOR_MODE_COUNT 4
+
+/**
+ * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
+ * @regulator_type: RPMh accelerator type used to manage this
+ * regulator
+ * @ops: Pointer to regulator ops callback structure
+ * @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
+ * @pmic_mode_map: Array indexed by regulator framework mode
+ * containing PMIC hardware modes. Must be large
+ * enough to index all framework modes supported
+ * by this regulator hardware type.
+ * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined
+ * in device tree to a regulator framework mode
+ * @bypass_mode: VRM PMIC mode value corresponding to bypass
+ * (pass-through) for this regulator. Only used
+ * by BOB type via VRM.
+ */
+struct rpmh_vreg_hw_data {
+ enum rpmh_regulator_type regulator_type;
+ const struct regulator_ops *ops;
+ const struct regulator_linear_range voltage_range;
+ int n_voltages;
+ const u32 *pmic_mode_map;
+ unsigned int (*of_map_mode)(unsigned int mode);
+ u32 bypass_mode;
+};
+
+/**
+ * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
+ * single regulator device
+ * @rpmh_client: Handle used for rpmh communications
+ * @addr: Base address of the regulator resource within
+ * an RPMh accelerator
+ * @rdesc: Regulator descriptor
+ * @hw_data: PMIC regulator configuration data for this RPMh
+ * regulator
+ * @regulator_type: RPMh accelerator type for this regulator
+ * resource
+ * @always_wait_for_ack: Boolean flag indicating if a request must always
+ * wait for an ACK from RPMh before continuing even
+ * if it corresponds to a strictly lower power
+ * state (e.g. enabled --> disabled).
+ * @drms_mode: Array of regulator framework modes which can
+ * be configured dynamically for this regulator
+ * via the set_load() callback.
+ * @drms_mode_max_uA: Array of maximum load currents in microamps
+ * supported by the corresponding modes in
+ * drms_mode. Elements must be specified in
+ * strictly increasing order.
+ * @drms_mode_count: The number of elements in drms_mode array.
+ * @enabled: Boolean indicating if the regulator is enabled
+ * or not
+ * @voltage_selector: Selector used for get_voltage_sel() and
+ * set_voltage_sel() callbacks
+ * @mode: RPMh VRM regulator current framework mode
+ * @bypassed: Boolean indicating if the regulator is in
+ * bypass (pass-through) mode or not. This is
+ * only used by BOB rpmh-regulator resources.
+ */
+struct rpmh_vreg {
+ struct rpmh_client *rpmh_client;
+ u32 addr;
+ struct regulator_desc rdesc;
+ const struct rpmh_vreg_hw_data *hw_data;
+ enum rpmh_regulator_type regulator_type;
+ bool always_wait_for_ack;
+ unsigned int *drms_mode;
+ int *drms_mode_max_uA;
+ size_t drms_mode_count;
+
+ bool enabled;
+ int voltage_selector;
+ unsigned int mode;
+ bool bypassed;
+};
+
+/**
+ * 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: RPMh regulator resource name format string.
+ * This must include exactly one field: '%s' which
+ * is filled at run-time with the PMIC ID provided
+ * by device tree property qcom,pmic-id. Example:
+ * "ldo%s1" for RPMh resource "ldoa1".
+ * @supply_name: Parent supply regulator name
+ * @hw_data: Configuration data for this PMIC regulator type
+ */
+struct rpmh_vreg_init_data {
+ const char *name;
+ const char *resource_name;
+ const char *supply_name;
+ const struct rpmh_vreg_hw_data *hw_data;
+};
+
+/**
+ * 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->rpmh_client,
+ RPMH_ACTIVE_ONLY_STATE, cmd, count);
+ else
+ ret = rpmh_write_async(vreg->rpmh_client,
+ RPMH_ACTIVE_ONLY_STATE, cmd, count);
+
+ return ret;
+}
+
+static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ return vreg->enabled;
+}
+
+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;
+
+ ret = rpmh_regulator_send_request(vreg, &cmd, 1, true);
+
+ if (!ret)
+ vreg->enabled = true;
+
+ return ret;
+}
+
+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,
+ };
+ int ret;
+
+ ret = rpmh_regulator_send_request(vreg, &cmd, 1, false);
+
+ if (!ret)
+ vreg->enabled = false;
+
+ return ret;
+}
+
+static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
+ unsigned int selector)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ struct tcs_cmd cmd = {
+ .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
+ };
+ int ret;
+
+ /* VRM voltage control register is set with voltage in millivolts. */
+ cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
+ selector), 1000);
+
+ ret = rpmh_regulator_send_request(vreg, &cmd, 1,
+ selector > vreg->voltage_selector);
+ if (!ret)
+ vreg->voltage_selector = selector;
+
+ return 0;
+}
+
+static int rpmh_regulator_vrm_get_voltage_sel(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ return vreg->voltage_selector;
+}
+
+static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
+ unsigned int mode, bool bypassed)
+{
+ struct tcs_cmd cmd = {
+ .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
+ };
+
+ cmd.data = bypassed ? vreg->hw_data->bypass_mode
+ : vreg->hw_data->pmic_mode_map[mode];
+
+ return rpmh_regulator_send_request(vreg, &cmd, 1, true);
+}
+
+static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
+ unsigned int mode)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ int ret;
+
+ if (mode == vreg->mode)
+ return 0;
+ else if (mode > REGULATOR_MODE_STANDBY)
+ return -EINVAL;
+
+ ret = rpmh_regulator_vrm_set_mode_bypass(vreg, mode, vreg->bypassed);
+ if (!ret)
+ vreg->mode = mode;
+
+ return ret;
+}
+
+static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ return vreg->mode;
+}
+
+static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ int i;
+
+ for (i = 0; i < vreg->drms_mode_count - 1; i++)
+ if (load_uA < vreg->drms_mode_max_uA[i])
+ break;
+
+ return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
+}
+
+static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
+ bool enable)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ int ret;
+
+ if (vreg->bypassed == enable)
+ return 0;
+
+ ret = rpmh_regulator_vrm_set_mode_bypass(vreg, vreg->mode, enable);
+ if (!ret)
+ vreg->bypassed = enable;
+
+ return ret;
+}
+
+static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
+ bool *enable)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ *enable = vreg->bypassed;
+
+ return 0;
+}
+
+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_sel = rpmh_regulator_vrm_set_voltage_sel,
+ .get_voltage_sel = rpmh_regulator_vrm_get_voltage_sel,
+ .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_vrm_bypass_ops = {
+ .enable = rpmh_regulator_enable,
+ .disable = rpmh_regulator_disable,
+ .is_enabled = rpmh_regulator_is_enabled,
+ .set_voltage_sel = rpmh_regulator_vrm_set_voltage_sel,
+ .get_voltage_sel = rpmh_regulator_vrm_get_voltage_sel,
+ .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,
+ .set_bypass = rpmh_regulator_vrm_set_bypass,
+ .get_bypass = rpmh_regulator_vrm_get_bypass,
+};
+
+static const struct regulator_ops rpmh_regulator_xob_ops = {
+ .enable = rpmh_regulator_enable,
+ .disable = rpmh_regulator_disable,
+ .is_enabled = rpmh_regulator_is_enabled,
+};
+
+/**
+ * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
+ * for a VRM RPMh resource from device tree
+ * vreg: Pointer to the individual rpmh-regulator resource
+ * dev: Pointer to the top level rpmh-regulator PMIC device
+ * node: Pointer to the individual rpmh-regulator resource
+ * device node
+ *
+ * This function initializes the drms_mode[] and drms_mode_max_uA[] arrays 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 *dev, struct device_node *node)
+{
+ const char *prop;
+ int i, len, ret, mode;
+ u32 *buf;
+
+ /* qcom,allowed-drms-modes is optional */
+ prop = "qcom,allowed-drms-modes";
+ len = of_property_count_elems_of_size(node, prop, sizeof(u32));
+ if (len < 0)
+ return 0;
+
+ vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
+ GFP_KERNEL);
+ vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
+ sizeof(*vreg->drms_mode_max_uA), GFP_KERNEL);
+ if (!vreg->drms_mode || !vreg->drms_mode_max_uA)
+ return -ENOMEM;
+ vreg->drms_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) {
+ dev_err(dev, "%s: unable to read %s, ret=%d\n",
+ node->name, prop, ret);
+ goto done;
+ }
+
+ for (i = 0; i < len; i++) {
+ mode = vreg->hw_data->of_map_mode(buf[i]);
+ if (mode <= 0) {
+ dev_err(dev, "%s: element %d of %s = %u is invalid for this regulator\n",
+ node->name, i, prop, buf[i]);
+ ret = -EINVAL;
+ goto done;
+ }
+
+ vreg->drms_mode[i] = mode;
+ }
+
+ prop = "qcom,drms-mode-threshold-currents";
+ len = of_property_count_elems_of_size(node, prop, sizeof(u32));
+ if (len != vreg->drms_mode_count) {
+ dev_err(dev, "%s: invalid element count=%d for %s\n",
+ node->name, len, prop);
+ ret = -EINVAL;
+ goto done;
+ }
+
+ ret = of_property_read_u32_array(node, prop, buf, len);
+ if (ret < 0) {
+ dev_err(dev, "%s: unable to read %s, ret=%d\n",
+ node->name, prop, ret);
+ goto done;
+ }
+
+ for (i = 0; i < len; i++) {
+ vreg->drms_mode_max_uA[i] = buf[i];
+
+ if (i > 0 && vreg->drms_mode_max_uA[i]
+ <= vreg->drms_mode_max_uA[i - 1]) {
+ dev_err(dev, "%s: %s elements are not in ascending order\n",
+ node->name, prop);
+ ret = -EINVAL;
+ goto done;
+ }
+ }
+
+done:
+ kfree(buf);
+ return ret;
+}
+
+/**
+ * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
+ * request for this regulator based on optional device tree
+ * properties
+ * vreg: Pointer to the individual rpmh-regulator resource
+ * dev: Pointer to the top level rpmh-regulator PMIC device
+ * node: Pointer to the individual rpmh-regulator resource
+ * device node
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg,
+ struct device *dev, struct device_node *node)
+{
+ struct tcs_cmd cmd[2] = {};
+ const struct regulator_linear_range *range;
+ const char *prop;
+ int cmd_count = 0;
+ int ret, selector;
+ u32 uV;
+
+ if (vreg->hw_data->regulator_type == VRM) {
+ prop = "qcom,headroom-voltage";
+ ret = of_property_read_u32(node, prop, &uV);
+ if (!ret) {
+ if (uV > RPMH_VRM_HEADROOM_MAX_UV) {
+ dev_err(dev, "%s: %s=%u is invalid\n",
+ node->name, prop, uV);
+ return -EINVAL;
+ }
+
+ cmd[cmd_count].addr
+ = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
+ cmd[cmd_count++].data = DIV_ROUND_UP(uV, 1000);
+ }
+
+ prop = "qcom,regulator-initial-voltage";
+ ret = of_property_read_u32(node, prop, &uV);
+ if (!ret) {
+ range = &vreg->hw_data->voltage_range;
+ selector = DIV_ROUND_UP(uV - range->min_uV,
+ range->uV_step) + range->min_sel;
+ if (uV < range->min_uV || selector > range->max_sel) {
+ dev_err(dev, "%s: %s=%u is invalid\n",
+ node->name, prop, uV);
+ return -EINVAL;
+ }
+
+ vreg->voltage_selector = selector;
+
+ cmd[cmd_count].addr
+ = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
+ cmd[cmd_count++].data
+ = DIV_ROUND_UP(selector * range->uV_step
+ + range->min_uV, 1000);
+ }
+ }
+
+ if (cmd_count) {
+ ret = rpmh_regulator_send_request(vreg, cmd, cmd_count, true);
+ if (ret < 0) {
+ dev_err(dev, "%s: could not send default config, ret=%d\n",
+ node->name, ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+/**
+ * rpmh_regulator_init_vreg() - initialize all attributes of an rpmh-regulator
+ * vreg: Pointer to the individual rpmh-regulator resource
+ * dev: Pointer to the top level rpmh-regulator PMIC device
+ * node: Pointer to the individual rpmh-regulator resource
+ * device node
+ * pmic_id: String used to identify the top level rpmh-regulator
+ * PMIC device on the board
+ * rpmh_data: Pointer to a null-terminated array of rpmh-regulator
+ * resources defined for the top level PMIC device
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
+ struct device_node *node, const char *pmic_id,
+ const struct rpmh_vreg_init_data *rpmh_data)
+{
+ struct regulator_config reg_config = {};
+ char rpmh_resource_name[20] = "";
+ struct regulator_dev *rdev;
+ enum rpmh_regulator_type type;
+ struct regulator_init_data *init_data;
+ unsigned int mode;
+ int i, ret;
+
+ for (; rpmh_data->name; rpmh_data++)
+ if (!strcmp(rpmh_data->name, node->name))
+ break;
+
+ if (!rpmh_data->name) {
+ dev_err(dev, "Unknown regulator %s\n", node->name);
+ return -EINVAL;
+ }
+
+ scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
+ rpmh_data->resource_name, pmic_id);
+
+ vreg->addr = cmd_db_read_addr(rpmh_resource_name);
+ if (!vreg->addr) {
+ dev_err(dev, "%s: could not find RPMh address for resource %s\n",
+ node->name, rpmh_resource_name);
+ return -ENODEV;
+ }
+
+ vreg->rdesc.name = rpmh_data->name;
+ vreg->rdesc.supply_name = rpmh_data->supply_name;
+ vreg->regulator_type = rpmh_data->hw_data->regulator_type;
+ vreg->hw_data = rpmh_data->hw_data;
+
+ if (rpmh_data->hw_data->n_voltages) {
+ 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;
+ }
+
+ type = vreg->regulator_type;
+
+ if (type == VRM) {
+ ret = rpmh_regulator_parse_vrm_modes(vreg, dev, node);
+ if (ret < 0)
+ return ret;
+ }
+
+ vreg->always_wait_for_ack = of_property_read_bool(node,
+ "qcom,always-wait-for-ack");
+
+ vreg->rdesc.owner = THIS_MODULE;
+ vreg->rdesc.type = REGULATOR_VOLTAGE;
+ vreg->rdesc.ops = vreg->hw_data->ops;
+ vreg->rdesc.of_map_mode = vreg->hw_data->of_map_mode;
+
+ init_data = of_get_regulator_init_data(dev, node, &vreg->rdesc);
+ if (!init_data)
+ return -ENOMEM;
+
+ if (type == XOB && init_data->constraints.min_uV) {
+ vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
+ vreg->rdesc.n_voltages = 1;
+ }
+
+ if (vreg->hw_data->of_map_mode) {
+ init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
+ for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++) {
+ mode = vreg->hw_data->of_map_mode(i);
+ if (mode > 0)
+ init_data->constraints.valid_modes_mask |= mode;
+ }
+ }
+
+ reg_config.dev = dev;
+ reg_config.init_data = init_data;
+ reg_config.of_node = node;
+ reg_config.driver_data = vreg;
+
+ ret = rpmh_regulator_load_default_parameters(vreg, dev, node);
+ if (ret < 0)
+ return ret;
+
+ rdev = devm_regulator_register(dev, &vreg->rdesc, ®_config);
+ if (IS_ERR(rdev)) {
+ ret = PTR_ERR(rdev);
+ rdev = NULL;
+ dev_err(dev, "%s: devm_regulator_register() failed, ret=%d\n",
+ node->name, ret);
+ return ret;
+ }
+
+ dev_dbg(dev, "%s regulator registered for RPMh resource %s @ 0x%05X\n",
+ node->name, rpmh_resource_name, vreg->addr);
+
+ return ret;
+}
+
+static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
+ [REGULATOR_MODE_STANDBY] = 4,
+ [REGULATOR_MODE_IDLE] = 5,
+ [REGULATOR_MODE_NORMAL] = -EINVAL,
+ [REGULATOR_MODE_FAST] = 7,
+};
+
+static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode)
+{
+ static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
+ [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY,
+ [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
+ [RPMH_REGULATOR_MODE_AUTO] = -EINVAL,
+ [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST,
+ };
+
+ if (mode >= RPMH_REGULATOR_MODE_COUNT)
+ return -EINVAL;
+
+ return of_mode_map[mode];
+}
+
+static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
+ [REGULATOR_MODE_STANDBY] = 4,
+ [REGULATOR_MODE_IDLE] = 5,
+ [REGULATOR_MODE_NORMAL] = 6,
+ [REGULATOR_MODE_FAST] = 7,
+};
+
+static unsigned int rpmh_regulator_pmic4_smps_of_map_mode(unsigned int mode)
+{
+ static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
+ [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,
+ };
+
+ if (mode >= RPMH_REGULATOR_MODE_COUNT)
+ return -EINVAL;
+
+ return of_mode_map[mode];
+}
+
+static const u32 pmic_mode_map_pmic4_bob[REGULATOR_MODE_STANDBY + 1] = {
+ [REGULATOR_MODE_STANDBY] = -EINVAL,
+ [REGULATOR_MODE_IDLE] = 1,
+ [REGULATOR_MODE_NORMAL] = 2,
+ [REGULATOR_MODE_FAST] = 3,
+};
+
+static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
+{
+ static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
+ [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,
+ };
+
+ if (mode >= RPMH_REGULATOR_MODE_COUNT)
+ return -EINVAL;
+
+ return of_mode_map[mode];
+}
+
+static const struct rpmh_vreg_hw_data pmic4_pldo = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000),
+ .n_voltages = 256,
+ .pmic_mode_map = pmic_mode_map_pmic4_ldo,
+ .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_pldo_lv = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(1256000, 0, 127, 8000),
+ .n_voltages = 128,
+ .pmic_mode_map = pmic_mode_map_pmic4_ldo,
+ .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_nldo = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(312000, 0, 127, 8000),
+ .n_voltages = 128,
+ .pmic_mode_map = pmic_mode_map_pmic4_ldo,
+ .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_hfsmps3 = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 215, 8000),
+ .n_voltages = 216,
+ .pmic_mode_map = pmic_mode_map_pmic4_smps,
+ .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_ftsmps426 = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 258, 4000),
+ .n_voltages = 259,
+ .pmic_mode_map = pmic_mode_map_pmic4_smps,
+ .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_bob = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_bypass_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
+ .n_voltages = 84,
+ .pmic_mode_map = pmic_mode_map_pmic4_bob,
+ .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
+ .bypass_mode = 0,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_lvs = {
+ .regulator_type = XOB,
+ .ops = &rpmh_regulator_xob_ops,
+ /* LVS hardware does not support voltage or mode configuration. */
+};
+
+#define RPMH_VREG(_name, _resource_name, _hw_data, _supply_name) \
+{ \
+ .name = _name, \
+ .resource_name = _resource_name, \
+ .hw_data = _hw_data, \
+ .supply_name = _supply_name, \
+}
+
+static const struct rpmh_vreg_init_data pm8998_vreg_data[] = {
+ RPMH_VREG("smps1", "smp%s1", &pmic4_ftsmps426, "vdd_s1"),
+ RPMH_VREG("smps2", "smp%s2", &pmic4_ftsmps426, "vdd_s2"),
+ RPMH_VREG("smps3", "smp%s3", &pmic4_hfsmps3, "vdd_s3"),
+ RPMH_VREG("smps4", "smp%s4", &pmic4_hfsmps3, "vdd_s4"),
+ RPMH_VREG("smps5", "smp%s5", &pmic4_hfsmps3, "vdd_s5"),
+ RPMH_VREG("smps6", "smp%s6", &pmic4_ftsmps426, "vdd_s6"),
+ RPMH_VREG("smps7", "smp%s7", &pmic4_ftsmps426, "vdd_s7"),
+ RPMH_VREG("smps8", "smp%s8", &pmic4_ftsmps426, "vdd_s8"),
+ RPMH_VREG("smps9", "smp%s9", &pmic4_ftsmps426, "vdd_s9"),
+ RPMH_VREG("smps10", "smp%s10", &pmic4_ftsmps426, "vdd_s10"),
+ RPMH_VREG("smps11", "smp%s11", &pmic4_ftsmps426, "vdd_s11"),
+ RPMH_VREG("smps12", "smp%s12", &pmic4_ftsmps426, "vdd_s12"),
+ RPMH_VREG("smps13", "smp%s13", &pmic4_ftsmps426, "vdd_s13"),
+ RPMH_VREG("ldo1", "ldo%s1", &pmic4_nldo, "vdd_l1_l27"),
+ RPMH_VREG("ldo2", "ldo%s2", &pmic4_nldo, "vdd_l2_l8_l17"),
+ RPMH_VREG("ldo3", "ldo%s3", &pmic4_nldo, "vdd_l3_l11"),
+ RPMH_VREG("ldo4", "ldo%s4", &pmic4_nldo, "vdd_l4_l5"),
+ RPMH_VREG("ldo5", "ldo%s5", &pmic4_nldo, "vdd_l4_l5"),
+ RPMH_VREG("ldo6", "ldo%s6", &pmic4_pldo, "vdd_l6"),
+ RPMH_VREG("ldo7", "ldo%s7", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
+ RPMH_VREG("ldo8", "ldo%s8", &pmic4_nldo, "vdd_l2_l8_l17"),
+ RPMH_VREG("ldo9", "ldo%s9", &pmic4_pldo, "vdd_l9"),
+ RPMH_VREG("ldo10", "ldo%s10", &pmic4_pldo, "vdd_l10_l23_l25"),
+ RPMH_VREG("ldo11", "ldo%s11", &pmic4_nldo, "vdd_l3_l11"),
+ RPMH_VREG("ldo12", "ldo%s12", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
+ RPMH_VREG("ldo13", "ldo%s13", &pmic4_pldo, "vdd_l13_l19_l21"),
+ RPMH_VREG("ldo14", "ldo%s14", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
+ RPMH_VREG("ldo15", "ldo%s15", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
+ RPMH_VREG("ldo16", "ldo%s16", &pmic4_pldo, "vdd_l16_l28"),
+ RPMH_VREG("ldo17", "ldo%s17", &pmic4_nldo, "vdd_l2_l8_l17"),
+ RPMH_VREG("ldo18", "ldo%s18", &pmic4_pldo, "vdd_l18_l22"),
+ RPMH_VREG("ldo19", "ldo%s19", &pmic4_pldo, "vdd_l13_l19_l21"),
+ RPMH_VREG("ldo20", "ldo%s20", &pmic4_pldo, "vdd_l20_l24"),
+ RPMH_VREG("ldo21", "ldo%s21", &pmic4_pldo, "vdd_l13_l19_l21"),
+ RPMH_VREG("ldo22", "ldo%s22", &pmic4_pldo, "vdd_l18_l22"),
+ RPMH_VREG("ldo23", "ldo%s23", &pmic4_pldo, "vdd_l10_l23_l25"),
+ RPMH_VREG("ldo24", "ldo%s24", &pmic4_pldo, "vdd_l20_l24"),
+ RPMH_VREG("ldo25", "ldo%s25", &pmic4_pldo, "vdd_l10_l23_l25"),
+ RPMH_VREG("ldo26", "ldo%s26", &pmic4_nldo, "vdd_l26"),
+ RPMH_VREG("ldo27", "ldo%s27", &pmic4_nldo, "vdd_l1_l27"),
+ RPMH_VREG("ldo28", "ldo%s28", &pmic4_pldo, "vdd_l16_l28"),
+ RPMH_VREG("lvs1", "vs%s1", &pmic4_lvs, "vdd_lvs1_lvs2"),
+ RPMH_VREG("lvs2", "vs%s2", &pmic4_lvs, "vdd_lvs1_lvs2"),
+ {},
+};
+
+static const struct rpmh_vreg_init_data pmi8998_vreg_data[] = {
+ RPMH_VREG("bob", "bob%s1", &pmic4_bob, "vdd_bob"),
+ {},
+};
+
+static const struct rpmh_vreg_init_data pm8005_vreg_data[] = {
+ RPMH_VREG("smps1", "smp%s1", &pmic4_ftsmps426, "vdd_s1"),
+ RPMH_VREG("smps2", "smp%s2", &pmic4_ftsmps426, "vdd_s2"),
+ RPMH_VREG("smps3", "smp%s3", &pmic4_ftsmps426, "vdd_s3"),
+ RPMH_VREG("smps4", "smp%s4", &pmic4_ftsmps426, "vdd_s4"),
+ {},
+};
+
+static int rpmh_regulator_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ const struct rpmh_vreg_init_data *vreg_data;
+ struct rpmh_client *rpmh_client;
+ struct device_node *node;
+ struct rpmh_vreg *vreg;
+ const char *pmic_id;
+ int ret;
+
+ ret = cmd_db_ready();
+ if (ret < 0) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "Command DB not available, ret=%d\n", ret);
+ return ret;
+ }
+
+ vreg_data = of_device_get_match_data(dev);
+ if (!vreg_data)
+ return -ENODEV;
+
+ ret = of_property_read_string(dev->of_node, "qcom,pmic-id", &pmic_id);
+ if (ret < 0) {
+ dev_err(dev, "qcom,pmic-id missing in DT node\n");
+ return ret;
+ }
+
+ rpmh_client = rpmh_get_client(pdev);
+ if (IS_ERR(rpmh_client))
+ return PTR_ERR(rpmh_client);
+ platform_set_drvdata(pdev, rpmh_client);
+
+ for_each_available_child_of_node(dev->of_node, node) {
+ vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
+ if (!vreg) {
+ ret = -ENOMEM;
+ of_node_put(node);
+ goto cleanup;
+ }
+
+ vreg->rpmh_client = rpmh_client;
+
+ ret = rpmh_regulator_init_vreg(vreg, dev, node, pmic_id,
+ vreg_data);
+ if (ret < 0) {
+ of_node_put(node);
+ goto cleanup;
+ }
+ }
+
+ return 0;
+
+cleanup:
+ rpmh_release(rpmh_client);
+
+ return ret;
+}
+
+static int rpmh_regulator_remove(struct platform_device *pdev)
+{
+ struct rpmh_client *rpmh_client = platform_get_drvdata(pdev);
+
+ rpmh_release(rpmh_client);
+
+ return 0;
+}
+
+static const struct of_device_id rpmh_regulator_match_table[] = {
+ {
+ .compatible = "qcom,pm8998-rpmh-regulators",
+ .data = pm8998_vreg_data,
+ },
+ {
+ .compatible = "qcom,pmi8998-rpmh-regulators",
+ .data = pmi8998_vreg_data,
+ },
+ {
+ .compatible = "qcom,pm8005-rpmh-regulators",
+ .data = pm8005_vreg_data,
+ },
+ {}
+};
+MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table);
+
+static struct platform_driver rpmh_regulator_driver = {
+ .driver = {
+ .name = "qcom-rpmh-regulator",
+ .of_match_table = of_match_ptr(rpmh_regulator_match_table),
+ },
+ .probe = rpmh_regulator_probe,
+ .remove = rpmh_regulator_remove,
+};
+module_platform_driver(rpmh_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm RPMh regulator driver");
+MODULE_LICENSE("GPL v2");
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [v2,2/2] regulator: add QCOM RPMh regulator driver
2018-04-14 2:50 ` David Collins
@ 2018-04-17 18:23 ` Matthias Kaehlcke
-1 siblings, 0 replies; 51+ messages in thread
From: Matthias Kaehlcke @ 2018-04-17 18:23 UTC (permalink / raw)
To: David Collins
Cc: broonie, lgirdwood, robh+dt, mark.rutland, linux-arm-msm,
linux-arm-kernel, devicetree, linux-kernel, rnayak, sboyd,
dianders
On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
> Add the QCOM RPMh regulator driver to manage PMIC regulators
> which are controlled via RPMh on some Qualcomm Technologies, Inc.
> SoCs. RPMh is a hardware block which contains several
> accelerators which are used to manage various hardware resources
> that are shared between the processors of the SoC. The final
> hardware state of a regulator is determined within RPMh by
> performing max aggregation of the requests made by all of the
> processors.
>
> Add support for PMIC regulator control via the voltage regulator
> manager (VRM) and oscillator buffer (XOB) RPMh accelerators. VRM
> supports manipulation of enable state, voltage, mode, and
> headroom voltage. XOB supports manipulation of enable state.
>
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> ---
> drivers/regulator/Kconfig | 9 +
> drivers/regulator/Makefile | 1 +
> drivers/regulator/qcom_rpmh-regulator.c | 910 ++++++++++++++++++++++++++++++++
> 3 files changed, 920 insertions(+)
> create mode 100644 drivers/regulator/qcom_rpmh-regulator.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 097f617..e0ecd0a 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
> Qualcomm RPM as a module. The module will be named
> "qcom_rpm-regulator".
>
> +config REGULATOR_QCOM_RPMH
> + tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
> + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
> + 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
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 590674f..c2274dd 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o
> obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
> obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
> +obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom_rpmh-regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o
> obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
> diff --git a/drivers/regulator/qcom_rpmh-regulator.c b/drivers/regulator/qcom_rpmh-regulator.c
> new file mode 100644
> index 0000000..03b1301
> --- /dev/null
> +++ b/drivers/regulator/qcom_rpmh-regulator.c
> @@ -0,0 +1,910 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#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
> + * %VRM: RPMh VRM accelerator which supports voting on enable, voltage,
> + * mode, and headroom voltage of LDO, SMPS, and BOB type PMIC
> + * regulators.
> + * %XOB: RPMh XOB accelerator which supports voting on the enable state
> + * of PMIC regulators.
> + */
> +enum rpmh_regulator_type {
> + VRM,
> + XOB,
> +};
> +
> +#define RPMH_VRM_HEADROOM_MAX_UV 511000
> +
> +#define RPMH_REGULATOR_REG_VRM_VOLTAGE 0x0
> +#define RPMH_REGULATOR_REG_ENABLE 0x4
> +#define RPMH_REGULATOR_REG_VRM_MODE 0x8
> +#define RPMH_REGULATOR_REG_VRM_HEADROOM 0xC
> +
> +#define RPMH_REGULATOR_DISABLE 0x0
> +#define RPMH_REGULATOR_ENABLE 0x1
> +
> +#define RPMH_REGULATOR_MODE_COUNT 4
> +
> +/**
> + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
> + * @regulator_type: RPMh accelerator type used to manage this
> + * regulator
> + * @ops: Pointer to regulator ops callback structure
> + * @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
> + * @pmic_mode_map: Array indexed by regulator framework mode
> + * containing PMIC hardware modes. Must be large
> + * enough to index all framework modes supported
> + * by this regulator hardware type.
> + * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined
> + * in device tree to a regulator framework mode
The name of the field is a bit misleading, this is a map of RPMh mode
to regulator framework mode, the device tree just happens to be the
place where this mapping is defined.
> + * @bypass_mode: VRM PMIC mode value corresponding to bypass
> + * (pass-through) for this regulator. Only used
> + * by BOB type via VRM.
> + */
> +struct rpmh_vreg_hw_data {
> + enum rpmh_regulator_type regulator_type;
> + const struct regulator_ops *ops;
> + const struct regulator_linear_range voltage_range;
> + int n_voltages;
> + const u32 *pmic_mode_map;
> + unsigned int (*of_map_mode)(unsigned int mode);
> + u32 bypass_mode;
> +};
> +
> +/**
> + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
> + * single regulator device
> + * @rpmh_client: Handle used for rpmh communications
nit: RPMh
> + * @addr: Base address of the regulator resource within
> + * an RPMh accelerator
> + * @rdesc: Regulator descriptor
> + * @hw_data: PMIC regulator configuration data for this RPMh
> + * regulator
> + * @regulator_type: RPMh accelerator type for this regulator
> + * resource
> + * @always_wait_for_ack: Boolean flag indicating if a request must always
> + * wait for an ACK from RPMh before continuing even
> + * if it corresponds to a strictly lower power
> + * state (e.g. enabled --> disabled).
> + * @drms_mode: Array of regulator framework modes which can
> + * be configured dynamically for this regulator
> + * via the set_load() callback.
> + * @drms_mode_max_uA: Array of maximum load currents in microamps
> + * supported by the corresponding modes in
> + * drms_mode. Elements must be specified in
> + * strictly increasing order.
> + * @drms_mode_count: The number of elements in drms_mode array.
> + * @enabled: Boolean indicating if the regulator is enabled
> + * or not
> + * @voltage_selector: Selector used for get_voltage_sel() and
> + * set_voltage_sel() callbacks
> + * @mode: RPMh VRM regulator current framework mode
> + * @bypassed: Boolean indicating if the regulator is in
> + * bypass (pass-through) mode or not. This is
> + * only used by BOB rpmh-regulator resources.
> + */
> +struct rpmh_vreg {
> + struct rpmh_client *rpmh_client;
> + u32 addr;
> + struct regulator_desc rdesc;
> + const struct rpmh_vreg_hw_data *hw_data;
> + enum rpmh_regulator_type regulator_type;
This value is already available via rpmh_vreg->hw_data->regulator_type,
why duplicate it? The field is assigned in rpmh_regulator_init_vreg()
and only read once in the same function, there seems to be no need for
it, not even to improve readability.
> + bool always_wait_for_ack;
> + unsigned int *drms_mode;
> + int *drms_mode_max_uA;
> + size_t drms_mode_count;
> +
> + bool enabled;
> + int voltage_selector;
> + unsigned int mode;
> + bool bypassed;
> +};
> +
> +/**
> + * 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: RPMh regulator resource name format string.
> + * This must include exactly one field: '%s' which
> + * is filled at run-time with the PMIC ID provided
> + * by device tree property qcom,pmic-id. Example:
> + * "ldo%s1" for RPMh resource "ldoa1".
> + * @supply_name: Parent supply regulator name
> + * @hw_data: Configuration data for this PMIC regulator type
> + */
> +struct rpmh_vreg_init_data {
> + const char *name;
> + const char *resource_name;
> + const char *supply_name;
> + const struct rpmh_vreg_hw_data *hw_data;
> +};
> +
> +/**
> + * 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->rpmh_client,
> + RPMH_ACTIVE_ONLY_STATE, cmd, count);
> + else
> + ret = rpmh_write_async(vreg->rpmh_client,
> + RPMH_ACTIVE_ONLY_STATE, cmd, count);
> +
> + return ret;
> +}
> +
> +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + return vreg->enabled;
> +}
> +
> +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;
> +
> + ret = rpmh_regulator_send_request(vreg, &cmd, 1, true);
> +
> + if (!ret)
> + vreg->enabled = true;
> +
> + return ret;
> +}
> +
> +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,
> + };
> + int ret;
> +
> + ret = rpmh_regulator_send_request(vreg, &cmd, 1, false);
> +
> + if (!ret)
> + vreg->enabled = false;
> +
> + return ret;
> +}
> +
> +static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
> + unsigned int selector)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
> + };
> + int ret;
> +
> + /* VRM voltage control register is set with voltage in millivolts. */
> + cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
> + selector), 1000);
> +
> + ret = rpmh_regulator_send_request(vreg, &cmd, 1,
> + selector > vreg->voltage_selector);
> + if (!ret)
> + vreg->voltage_selector = selector;
> +
> + return 0;
> +}
> +
> +static int rpmh_regulator_vrm_get_voltage_sel(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + return vreg->voltage_selector;
> +}
> +
> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
> + unsigned int mode, bool bypassed)
> +{
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
> + };
> +
> + cmd.data = bypassed ? vreg->hw_data->bypass_mode
> + : vreg->hw_data->pmic_mode_map[mode];
> +
> + return rpmh_regulator_send_request(vreg, &cmd, 1, true);
> +}
> +
> +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
> + unsigned int mode)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int ret;
> +
> + if (mode == vreg->mode)
> + return 0;
> + else if (mode > REGULATOR_MODE_STANDBY)
> + return -EINVAL;
> +
> + ret = rpmh_regulator_vrm_set_mode_bypass(vreg, mode, vreg->bypassed);
> + if (!ret)
> + vreg->mode = mode;
> +
> + return ret;
> +}
> +
> +static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + return vreg->mode;
> +}
> +
> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int i;
> +
> + for (i = 0; i < vreg->drms_mode_count - 1; i++)
> + if (load_uA < vreg->drms_mode_max_uA[i])
Shouldn't this be '<='?
nit: IMO 'vreg->drms_mode_max_uA[i] >= load_uA' would be more readable.
> + break;
> +
> + return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
> +}
> +
> +static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
> + bool enable)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int ret;
> +
> + if (vreg->bypassed == enable)
> + return 0;
> +
> + ret = rpmh_regulator_vrm_set_mode_bypass(vreg, vreg->mode, enable);
> + if (!ret)
> + vreg->bypassed = enable;
> +
> + return ret;
> +}
> +
> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
> + bool *enable)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + *enable = vreg->bypassed;
> +
> + return 0;
> +}
> +
> +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_sel = rpmh_regulator_vrm_set_voltage_sel,
> + .get_voltage_sel = rpmh_regulator_vrm_get_voltage_sel,
> + .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_vrm_bypass_ops = {
> + .enable = rpmh_regulator_enable,
> + .disable = rpmh_regulator_disable,
> + .is_enabled = rpmh_regulator_is_enabled,
> + .set_voltage_sel = rpmh_regulator_vrm_set_voltage_sel,
> + .get_voltage_sel = rpmh_regulator_vrm_get_voltage_sel,
> + .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,
> + .set_bypass = rpmh_regulator_vrm_set_bypass,
> + .get_bypass = rpmh_regulator_vrm_get_bypass,
> +};
> +
> +static const struct regulator_ops rpmh_regulator_xob_ops = {
> + .enable = rpmh_regulator_enable,
> + .disable = rpmh_regulator_disable,
> + .is_enabled = rpmh_regulator_is_enabled,
> +};
> +
> +/**
> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
> + * for a VRM RPMh resource from device tree
> + * vreg: Pointer to the individual rpmh-regulator resource
> + * dev: Pointer to the top level rpmh-regulator PMIC device
> + * node: Pointer to the individual rpmh-regulator resource
> + * device node
> + *
> + * This function initializes the drms_mode[] and drms_mode_max_uA[] arrays 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 *dev, struct device_node *node)
> +{
> + const char *prop;
> + int i, len, ret, mode;
> + u32 *buf;
> +
> + /* qcom,allowed-drms-modes is optional */
> + prop = "qcom,allowed-drms-modes";
> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
> + if (len < 0)
> + return 0;
> +
> + vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
> + GFP_KERNEL);
> + vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
> + sizeof(*vreg->drms_mode_max_uA), GFP_KERNEL);
> + if (!vreg->drms_mode || !vreg->drms_mode_max_uA)
> + return -ENOMEM;
> + vreg->drms_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) {
> + dev_err(dev, "%s: unable to read %s, ret=%d\n",
> + node->name, prop, ret);
> + goto done;
> + }
> +
> + for (i = 0; i < len; i++) {
> + mode = vreg->hw_data->of_map_mode(buf[i]);
> + if (mode <= 0) {
> + dev_err(dev, "%s: element %d of %s = %u is invalid for this regulator\n",
> + node->name, i, prop, buf[i]);
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + vreg->drms_mode[i] = mode;
> + }
> +
> + prop = "qcom,drms-mode-threshold-currents";
> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
> + if (len != vreg->drms_mode_count) {
> + dev_err(dev, "%s: invalid element count=%d for %s\n",
> + node->name, len, prop);
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + ret = of_property_read_u32_array(node, prop, buf, len);
> + if (ret < 0) {
> + dev_err(dev, "%s: unable to read %s, ret=%d\n",
> + node->name, prop, ret);
> + goto done;
> + }
> +
> + for (i = 0; i < len; i++) {
> + vreg->drms_mode_max_uA[i] = buf[i];
> +
> + if (i > 0 && vreg->drms_mode_max_uA[i]
> + <= vreg->drms_mode_max_uA[i - 1]) {
> + dev_err(dev, "%s: %s elements are not in ascending order\n",
> + node->name, prop);
> + ret = -EINVAL;
> + goto done;
> + }
> + }
> +
> +done:
> + kfree(buf);
> + return ret;
> +}
> +
> +/**
> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
> + * request for this regulator based on optional device tree
> + * properties
> + * vreg: Pointer to the individual rpmh-regulator resource
> + * dev: Pointer to the top level rpmh-regulator PMIC device
> + * node: Pointer to the individual rpmh-regulator resource
> + * device node
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg,
> + struct device *dev, struct device_node *node)
> +{
> + struct tcs_cmd cmd[2] = {};
> + const struct regulator_linear_range *range;
> + const char *prop;
> + int cmd_count = 0;
> + int ret, selector;
> + u32 uV;
> +
> + if (vreg->hw_data->regulator_type == VRM) {
> + prop = "qcom,headroom-voltage";
> + ret = of_property_read_u32(node, prop, &uV);
> + if (!ret) {
> + if (uV > RPMH_VRM_HEADROOM_MAX_UV) {
> + dev_err(dev, "%s: %s=%u is invalid\n",
> + node->name, prop, uV);
> + return -EINVAL;
> + }
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
> + cmd[cmd_count++].data = DIV_ROUND_UP(uV, 1000);
> + }
> +
> + prop = "qcom,regulator-initial-voltage";
> + ret = of_property_read_u32(node, prop, &uV);
> + if (!ret) {
> + range = &vreg->hw_data->voltage_range;
> + selector = DIV_ROUND_UP(uV - range->min_uV,
> + range->uV_step) + range->min_sel;
> + if (uV < range->min_uV || selector > range->max_sel) {
> + dev_err(dev, "%s: %s=%u is invalid\n",
> + node->name, prop, uV);
> + return -EINVAL;
> + }
> +
> + vreg->voltage_selector = selector;
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
> + cmd[cmd_count++].data
> + = DIV_ROUND_UP(selector * range->uV_step
> + + range->min_uV, 1000);
> + }
> + }
> +
> + if (cmd_count) {
> + ret = rpmh_regulator_send_request(vreg, cmd, cmd_count, true);
> + if (ret < 0) {
> + dev_err(dev, "%s: could not send default config, ret=%d\n",
> + node->name, ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * rpmh_regulator_init_vreg() - initialize all attributes of an rpmh-regulator
> + * vreg: Pointer to the individual rpmh-regulator resource
> + * dev: Pointer to the top level rpmh-regulator PMIC device
> + * node: Pointer to the individual rpmh-regulator resource
> + * device node
> + * pmic_id: String used to identify the top level rpmh-regulator
> + * PMIC device on the board
> + * rpmh_data: Pointer to a null-terminated array of rpmh-regulator
> + * resources defined for the top level PMIC device
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
> + struct device_node *node, const char *pmic_id,
> + const struct rpmh_vreg_init_data *rpmh_data)
> +{
> + struct regulator_config reg_config = {};
> + char rpmh_resource_name[20] = "";
> + struct regulator_dev *rdev;
> + enum rpmh_regulator_type type;
> + struct regulator_init_data *init_data;
> + unsigned int mode;
> + int i, ret;
> +
> + for (; rpmh_data->name; rpmh_data++)
> + if (!strcmp(rpmh_data->name, node->name))
> + break;
> +
> + if (!rpmh_data->name) {
> + dev_err(dev, "Unknown regulator %s\n", node->name);
> + return -EINVAL;
> + }
> +
> + scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
> + rpmh_data->resource_name, pmic_id);
> +
> + vreg->addr = cmd_db_read_addr(rpmh_resource_name);
> + if (!vreg->addr) {
> + dev_err(dev, "%s: could not find RPMh address for resource %s\n",
> + node->name, rpmh_resource_name);
> + return -ENODEV;
> + }
> +
> + vreg->rdesc.name = rpmh_data->name;
> + vreg->rdesc.supply_name = rpmh_data->supply_name;
> + vreg->regulator_type = rpmh_data->hw_data->regulator_type;
> + vreg->hw_data = rpmh_data->hw_data;
> +
> + if (rpmh_data->hw_data->n_voltages) {
> + 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;
> + }
> +
> + type = vreg->regulator_type;
> +
> + if (type == VRM) {
> + ret = rpmh_regulator_parse_vrm_modes(vreg, dev, node);
> + if (ret < 0)
> + return ret;
> + }
> +
> + vreg->always_wait_for_ack = of_property_read_bool(node,
> + "qcom,always-wait-for-ack");
> +
> + vreg->rdesc.owner = THIS_MODULE;
> + vreg->rdesc.type = REGULATOR_VOLTAGE;
> + vreg->rdesc.ops = vreg->hw_data->ops;
> + vreg->rdesc.of_map_mode = vreg->hw_data->of_map_mode;
> +
> + init_data = of_get_regulator_init_data(dev, node, &vreg->rdesc);
> + if (!init_data)
> + return -ENOMEM;
> +
> + if (type == XOB && init_data->constraints.min_uV) {
> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
> + vreg->rdesc.n_voltages = 1;
> + }
> +
> + if (vreg->hw_data->of_map_mode) {
> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
> + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++) {
> + mode = vreg->hw_data->of_map_mode(i);
> + if (mode > 0)
> + init_data->constraints.valid_modes_mask |= mode;
> + }
> + }
> +
> + reg_config.dev = dev;
> + reg_config.init_data = init_data;
> + reg_config.of_node = node;
> + reg_config.driver_data = vreg;
> +
> + ret = rpmh_regulator_load_default_parameters(vreg, dev, node);
> + if (ret < 0)
> + return ret;
> +
> + rdev = devm_regulator_register(dev, &vreg->rdesc, ®_config);
> + if (IS_ERR(rdev)) {
> + ret = PTR_ERR(rdev);
> + rdev = NULL;
> + dev_err(dev, "%s: devm_regulator_register() failed, ret=%d\n",
> + node->name, ret);
> + return ret;
> + }
> +
> + dev_dbg(dev, "%s regulator registered for RPMh resource %s @ 0x%05X\n",
> + node->name, rpmh_resource_name, vreg->addr);
> +
> + return ret;
> +}
> +
> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
> + [REGULATOR_MODE_STANDBY] = 4,
> + [REGULATOR_MODE_IDLE] = 5,
> + [REGULATOR_MODE_NORMAL] = -EINVAL,
> + [REGULATOR_MODE_FAST] = 7,
> +};
Define constants for the modes on the PMIC4 side?
> +
> +static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode)
> +{
> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
> + [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY,
> + [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
> + [RPMH_REGULATOR_MODE_AUTO] = -EINVAL,
> + [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST,
> + };
> +
> + if (mode >= RPMH_REGULATOR_MODE_COUNT)
> + return -EINVAL;
> +
> + return of_mode_map[mode];
> +}
> +
> +static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
> + [REGULATOR_MODE_STANDBY] = 4,
> + [REGULATOR_MODE_IDLE] = 5,
> + [REGULATOR_MODE_NORMAL] = 6,
> + [REGULATOR_MODE_FAST] = 7,
> +};
> +
> +static unsigned int rpmh_regulator_pmic4_smps_of_map_mode(unsigned int mode)
> +{
> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
> + [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,
> + };
> +
> + if (mode >= RPMH_REGULATOR_MODE_COUNT)
> + return -EINVAL;
> +
> + return of_mode_map[mode];
> +}
> +
> +static const u32 pmic_mode_map_pmic4_bob[REGULATOR_MODE_STANDBY + 1] = {
> + [REGULATOR_MODE_STANDBY] = -EINVAL,
> + [REGULATOR_MODE_IDLE] = 1,
> + [REGULATOR_MODE_NORMAL] = 2,
> + [REGULATOR_MODE_FAST] = 3,
> +};
> +
> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
> +{
> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
> + [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,
> + };
> +
> + if (mode >= RPMH_REGULATOR_MODE_COUNT)
> + return -EINVAL;
> +
> + return of_mode_map[mode];
> +}
> +
> +static const struct rpmh_vreg_hw_data pmic4_pldo = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000),
> + .n_voltages = 256,
> + .pmic_mode_map = pmic_mode_map_pmic4_ldo,
> + .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_pldo_lv = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(1256000, 0, 127, 8000),
> + .n_voltages = 128,
> + .pmic_mode_map = pmic_mode_map_pmic4_ldo,
> + .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_nldo = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(312000, 0, 127, 8000),
> + .n_voltages = 128,
> + .pmic_mode_map = pmic_mode_map_pmic4_ldo,
> + .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_hfsmps3 = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 215, 8000),
> + .n_voltages = 216,
> + .pmic_mode_map = pmic_mode_map_pmic4_smps,
> + .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_ftsmps426 = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 258, 4000),
> + .n_voltages = 259,
> + .pmic_mode_map = pmic_mode_map_pmic4_smps,
> + .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_bob = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_bypass_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
> + .n_voltages = 84,
> + .pmic_mode_map = pmic_mode_map_pmic4_bob,
> + .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
> + .bypass_mode = 0,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_lvs = {
> + .regulator_type = XOB,
> + .ops = &rpmh_regulator_xob_ops,
> + /* LVS hardware does not support voltage or mode configuration. */
> +};
> +
> +#define RPMH_VREG(_name, _resource_name, _hw_data, _supply_name) \
> +{ \
> + .name = _name, \
> + .resource_name = _resource_name, \
> + .hw_data = _hw_data, \
> + .supply_name = _supply_name, \
> +}
> +
> +static const struct rpmh_vreg_init_data pm8998_vreg_data[] = {
> + RPMH_VREG("smps1", "smp%s1", &pmic4_ftsmps426, "vdd_s1"),
> + RPMH_VREG("smps2", "smp%s2", &pmic4_ftsmps426, "vdd_s2"),
> + RPMH_VREG("smps3", "smp%s3", &pmic4_hfsmps3, "vdd_s3"),
> + RPMH_VREG("smps4", "smp%s4", &pmic4_hfsmps3, "vdd_s4"),
> + RPMH_VREG("smps5", "smp%s5", &pmic4_hfsmps3, "vdd_s5"),
> + RPMH_VREG("smps6", "smp%s6", &pmic4_ftsmps426, "vdd_s6"),
> + RPMH_VREG("smps7", "smp%s7", &pmic4_ftsmps426, "vdd_s7"),
> + RPMH_VREG("smps8", "smp%s8", &pmic4_ftsmps426, "vdd_s8"),
> + RPMH_VREG("smps9", "smp%s9", &pmic4_ftsmps426, "vdd_s9"),
> + RPMH_VREG("smps10", "smp%s10", &pmic4_ftsmps426, "vdd_s10"),
> + RPMH_VREG("smps11", "smp%s11", &pmic4_ftsmps426, "vdd_s11"),
> + RPMH_VREG("smps12", "smp%s12", &pmic4_ftsmps426, "vdd_s12"),
> + RPMH_VREG("smps13", "smp%s13", &pmic4_ftsmps426, "vdd_s13"),
> + RPMH_VREG("ldo1", "ldo%s1", &pmic4_nldo, "vdd_l1_l27"),
> + RPMH_VREG("ldo2", "ldo%s2", &pmic4_nldo, "vdd_l2_l8_l17"),
> + RPMH_VREG("ldo3", "ldo%s3", &pmic4_nldo, "vdd_l3_l11"),
> + RPMH_VREG("ldo4", "ldo%s4", &pmic4_nldo, "vdd_l4_l5"),
> + RPMH_VREG("ldo5", "ldo%s5", &pmic4_nldo, "vdd_l4_l5"),
> + RPMH_VREG("ldo6", "ldo%s6", &pmic4_pldo, "vdd_l6"),
> + RPMH_VREG("ldo7", "ldo%s7", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
> + RPMH_VREG("ldo8", "ldo%s8", &pmic4_nldo, "vdd_l2_l8_l17"),
> + RPMH_VREG("ldo9", "ldo%s9", &pmic4_pldo, "vdd_l9"),
> + RPMH_VREG("ldo10", "ldo%s10", &pmic4_pldo, "vdd_l10_l23_l25"),
> + RPMH_VREG("ldo11", "ldo%s11", &pmic4_nldo, "vdd_l3_l11"),
> + RPMH_VREG("ldo12", "ldo%s12", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
> + RPMH_VREG("ldo13", "ldo%s13", &pmic4_pldo, "vdd_l13_l19_l21"),
> + RPMH_VREG("ldo14", "ldo%s14", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
> + RPMH_VREG("ldo15", "ldo%s15", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
> + RPMH_VREG("ldo16", "ldo%s16", &pmic4_pldo, "vdd_l16_l28"),
> + RPMH_VREG("ldo17", "ldo%s17", &pmic4_nldo, "vdd_l2_l8_l17"),
> + RPMH_VREG("ldo18", "ldo%s18", &pmic4_pldo, "vdd_l18_l22"),
> + RPMH_VREG("ldo19", "ldo%s19", &pmic4_pldo, "vdd_l13_l19_l21"),
> + RPMH_VREG("ldo20", "ldo%s20", &pmic4_pldo, "vdd_l20_l24"),
> + RPMH_VREG("ldo21", "ldo%s21", &pmic4_pldo, "vdd_l13_l19_l21"),
> + RPMH_VREG("ldo22", "ldo%s22", &pmic4_pldo, "vdd_l18_l22"),
> + RPMH_VREG("ldo23", "ldo%s23", &pmic4_pldo, "vdd_l10_l23_l25"),
> + RPMH_VREG("ldo24", "ldo%s24", &pmic4_pldo, "vdd_l20_l24"),
> + RPMH_VREG("ldo25", "ldo%s25", &pmic4_pldo, "vdd_l10_l23_l25"),
> + RPMH_VREG("ldo26", "ldo%s26", &pmic4_nldo, "vdd_l26"),
> + RPMH_VREG("ldo27", "ldo%s27", &pmic4_nldo, "vdd_l1_l27"),
> + RPMH_VREG("ldo28", "ldo%s28", &pmic4_pldo, "vdd_l16_l28"),
> + RPMH_VREG("lvs1", "vs%s1", &pmic4_lvs, "vdd_lvs1_lvs2"),
> + RPMH_VREG("lvs2", "vs%s2", &pmic4_lvs, "vdd_lvs1_lvs2"),
> + {},
> +};
> +
> +static const struct rpmh_vreg_init_data pmi8998_vreg_data[] = {
> + RPMH_VREG("bob", "bob%s1", &pmic4_bob, "vdd_bob"),
> + {},
> +};
> +
> +static const struct rpmh_vreg_init_data pm8005_vreg_data[] = {
> + RPMH_VREG("smps1", "smp%s1", &pmic4_ftsmps426, "vdd_s1"),
> + RPMH_VREG("smps2", "smp%s2", &pmic4_ftsmps426, "vdd_s2"),
> + RPMH_VREG("smps3", "smp%s3", &pmic4_ftsmps426, "vdd_s3"),
> + RPMH_VREG("smps4", "smp%s4", &pmic4_ftsmps426, "vdd_s4"),
> + {},
> +};
> +
> +static int rpmh_regulator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct rpmh_vreg_init_data *vreg_data;
> + struct rpmh_client *rpmh_client;
> + struct device_node *node;
> + struct rpmh_vreg *vreg;
> + const char *pmic_id;
> + int ret;
> +
> + ret = cmd_db_ready();
> + if (ret < 0) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
> + return ret;
> + }
> +
> + vreg_data = of_device_get_match_data(dev);
> + if (!vreg_data)
> + return -ENODEV;
> +
> + ret = of_property_read_string(dev->of_node, "qcom,pmic-id", &pmic_id);
> + if (ret < 0) {
> + dev_err(dev, "qcom,pmic-id missing in DT node\n");
> + return ret;
> + }
> +
> + rpmh_client = rpmh_get_client(pdev);
> + if (IS_ERR(rpmh_client))
> + return PTR_ERR(rpmh_client);
> + platform_set_drvdata(pdev, rpmh_client);
> +
> + for_each_available_child_of_node(dev->of_node, node) {
> + vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
> + if (!vreg) {
> + ret = -ENOMEM;
> + of_node_put(node);
> + goto cleanup;
> + }
> +
> + vreg->rpmh_client = rpmh_client;
> +
> + ret = rpmh_regulator_init_vreg(vreg, dev, node, pmic_id,
> + vreg_data);
> + if (ret < 0) {
> + of_node_put(node);
> + goto cleanup;
> + }
> + }
> +
> + return 0;
> +
> +cleanup:
> + rpmh_release(rpmh_client);
> +
> + return ret;
> +}
> +
> +static int rpmh_regulator_remove(struct platform_device *pdev)
> +{
> + struct rpmh_client *rpmh_client = platform_get_drvdata(pdev);
> +
> + rpmh_release(rpmh_client);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rpmh_regulator_match_table[] = {
> + {
> + .compatible = "qcom,pm8998-rpmh-regulators",
> + .data = pm8998_vreg_data,
> + },
> + {
> + .compatible = "qcom,pmi8998-rpmh-regulators",
> + .data = pmi8998_vreg_data,
> + },
> + {
> + .compatible = "qcom,pm8005-rpmh-regulators",
> + .data = pm8005_vreg_data,
> + },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table);
> +
> +static struct platform_driver rpmh_regulator_driver = {
> + .driver = {
> + .name = "qcom-rpmh-regulator",
> + .of_match_table = of_match_ptr(rpmh_regulator_match_table),
> + },
> + .probe = rpmh_regulator_probe,
> + .remove = rpmh_regulator_remove,
> +};
> +module_platform_driver(rpmh_regulator_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm RPMh regulator driver");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 51+ messages in thread* [v2,2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-17 18:23 ` Matthias Kaehlcke
0 siblings, 0 replies; 51+ messages in thread
From: Matthias Kaehlcke @ 2018-04-17 18:23 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
> Add the QCOM RPMh regulator driver to manage PMIC regulators
> which are controlled via RPMh on some Qualcomm Technologies, Inc.
> SoCs. RPMh is a hardware block which contains several
> accelerators which are used to manage various hardware resources
> that are shared between the processors of the SoC. The final
> hardware state of a regulator is determined within RPMh by
> performing max aggregation of the requests made by all of the
> processors.
>
> Add support for PMIC regulator control via the voltage regulator
> manager (VRM) and oscillator buffer (XOB) RPMh accelerators. VRM
> supports manipulation of enable state, voltage, mode, and
> headroom voltage. XOB supports manipulation of enable state.
>
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> ---
> drivers/regulator/Kconfig | 9 +
> drivers/regulator/Makefile | 1 +
> drivers/regulator/qcom_rpmh-regulator.c | 910 ++++++++++++++++++++++++++++++++
> 3 files changed, 920 insertions(+)
> create mode 100644 drivers/regulator/qcom_rpmh-regulator.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 097f617..e0ecd0a 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
> Qualcomm RPM as a module. The module will be named
> "qcom_rpm-regulator".
>
> +config REGULATOR_QCOM_RPMH
> + tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
> + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
> + 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
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 590674f..c2274dd 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o
> obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
> obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
> +obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom_rpmh-regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o
> obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
> diff --git a/drivers/regulator/qcom_rpmh-regulator.c b/drivers/regulator/qcom_rpmh-regulator.c
> new file mode 100644
> index 0000000..03b1301
> --- /dev/null
> +++ b/drivers/regulator/qcom_rpmh-regulator.c
> @@ -0,0 +1,910 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#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
> + * %VRM: RPMh VRM accelerator which supports voting on enable, voltage,
> + * mode, and headroom voltage of LDO, SMPS, and BOB type PMIC
> + * regulators.
> + * %XOB: RPMh XOB accelerator which supports voting on the enable state
> + * of PMIC regulators.
> + */
> +enum rpmh_regulator_type {
> + VRM,
> + XOB,
> +};
> +
> +#define RPMH_VRM_HEADROOM_MAX_UV 511000
> +
> +#define RPMH_REGULATOR_REG_VRM_VOLTAGE 0x0
> +#define RPMH_REGULATOR_REG_ENABLE 0x4
> +#define RPMH_REGULATOR_REG_VRM_MODE 0x8
> +#define RPMH_REGULATOR_REG_VRM_HEADROOM 0xC
> +
> +#define RPMH_REGULATOR_DISABLE 0x0
> +#define RPMH_REGULATOR_ENABLE 0x1
> +
> +#define RPMH_REGULATOR_MODE_COUNT 4
> +
> +/**
> + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
> + * @regulator_type: RPMh accelerator type used to manage this
> + * regulator
> + * @ops: Pointer to regulator ops callback structure
> + * @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
> + * @pmic_mode_map: Array indexed by regulator framework mode
> + * containing PMIC hardware modes. Must be large
> + * enough to index all framework modes supported
> + * by this regulator hardware type.
> + * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined
> + * in device tree to a regulator framework mode
The name of the field is a bit misleading, this is a map of RPMh mode
to regulator framework mode, the device tree just happens to be the
place where this mapping is defined.
> + * @bypass_mode: VRM PMIC mode value corresponding to bypass
> + * (pass-through) for this regulator. Only used
> + * by BOB type via VRM.
> + */
> +struct rpmh_vreg_hw_data {
> + enum rpmh_regulator_type regulator_type;
> + const struct regulator_ops *ops;
> + const struct regulator_linear_range voltage_range;
> + int n_voltages;
> + const u32 *pmic_mode_map;
> + unsigned int (*of_map_mode)(unsigned int mode);
> + u32 bypass_mode;
> +};
> +
> +/**
> + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
> + * single regulator device
> + * @rpmh_client: Handle used for rpmh communications
nit: RPMh
> + * @addr: Base address of the regulator resource within
> + * an RPMh accelerator
> + * @rdesc: Regulator descriptor
> + * @hw_data: PMIC regulator configuration data for this RPMh
> + * regulator
> + * @regulator_type: RPMh accelerator type for this regulator
> + * resource
> + * @always_wait_for_ack: Boolean flag indicating if a request must always
> + * wait for an ACK from RPMh before continuing even
> + * if it corresponds to a strictly lower power
> + * state (e.g. enabled --> disabled).
> + * @drms_mode: Array of regulator framework modes which can
> + * be configured dynamically for this regulator
> + * via the set_load() callback.
> + * @drms_mode_max_uA: Array of maximum load currents in microamps
> + * supported by the corresponding modes in
> + * drms_mode. Elements must be specified in
> + * strictly increasing order.
> + * @drms_mode_count: The number of elements in drms_mode array.
> + * @enabled: Boolean indicating if the regulator is enabled
> + * or not
> + * @voltage_selector: Selector used for get_voltage_sel() and
> + * set_voltage_sel() callbacks
> + * @mode: RPMh VRM regulator current framework mode
> + * @bypassed: Boolean indicating if the regulator is in
> + * bypass (pass-through) mode or not. This is
> + * only used by BOB rpmh-regulator resources.
> + */
> +struct rpmh_vreg {
> + struct rpmh_client *rpmh_client;
> + u32 addr;
> + struct regulator_desc rdesc;
> + const struct rpmh_vreg_hw_data *hw_data;
> + enum rpmh_regulator_type regulator_type;
This value is already available via rpmh_vreg->hw_data->regulator_type,
why duplicate it? The field is assigned in rpmh_regulator_init_vreg()
and only read once in the same function, there seems to be no need for
it, not even to improve readability.
> + bool always_wait_for_ack;
> + unsigned int *drms_mode;
> + int *drms_mode_max_uA;
> + size_t drms_mode_count;
> +
> + bool enabled;
> + int voltage_selector;
> + unsigned int mode;
> + bool bypassed;
> +};
> +
> +/**
> + * 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: RPMh regulator resource name format string.
> + * This must include exactly one field: '%s' which
> + * is filled at run-time with the PMIC ID provided
> + * by device tree property qcom,pmic-id. Example:
> + * "ldo%s1" for RPMh resource "ldoa1".
> + * @supply_name: Parent supply regulator name
> + * @hw_data: Configuration data for this PMIC regulator type
> + */
> +struct rpmh_vreg_init_data {
> + const char *name;
> + const char *resource_name;
> + const char *supply_name;
> + const struct rpmh_vreg_hw_data *hw_data;
> +};
> +
> +/**
> + * 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->rpmh_client,
> + RPMH_ACTIVE_ONLY_STATE, cmd, count);
> + else
> + ret = rpmh_write_async(vreg->rpmh_client,
> + RPMH_ACTIVE_ONLY_STATE, cmd, count);
> +
> + return ret;
> +}
> +
> +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + return vreg->enabled;
> +}
> +
> +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;
> +
> + ret = rpmh_regulator_send_request(vreg, &cmd, 1, true);
> +
> + if (!ret)
> + vreg->enabled = true;
> +
> + return ret;
> +}
> +
> +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,
> + };
> + int ret;
> +
> + ret = rpmh_regulator_send_request(vreg, &cmd, 1, false);
> +
> + if (!ret)
> + vreg->enabled = false;
> +
> + return ret;
> +}
> +
> +static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
> + unsigned int selector)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
> + };
> + int ret;
> +
> + /* VRM voltage control register is set with voltage in millivolts. */
> + cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
> + selector), 1000);
> +
> + ret = rpmh_regulator_send_request(vreg, &cmd, 1,
> + selector > vreg->voltage_selector);
> + if (!ret)
> + vreg->voltage_selector = selector;
> +
> + return 0;
> +}
> +
> +static int rpmh_regulator_vrm_get_voltage_sel(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + return vreg->voltage_selector;
> +}
> +
> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
> + unsigned int mode, bool bypassed)
> +{
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
> + };
> +
> + cmd.data = bypassed ? vreg->hw_data->bypass_mode
> + : vreg->hw_data->pmic_mode_map[mode];
> +
> + return rpmh_regulator_send_request(vreg, &cmd, 1, true);
> +}
> +
> +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
> + unsigned int mode)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int ret;
> +
> + if (mode == vreg->mode)
> + return 0;
> + else if (mode > REGULATOR_MODE_STANDBY)
> + return -EINVAL;
> +
> + ret = rpmh_regulator_vrm_set_mode_bypass(vreg, mode, vreg->bypassed);
> + if (!ret)
> + vreg->mode = mode;
> +
> + return ret;
> +}
> +
> +static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + return vreg->mode;
> +}
> +
> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int i;
> +
> + for (i = 0; i < vreg->drms_mode_count - 1; i++)
> + if (load_uA < vreg->drms_mode_max_uA[i])
Shouldn't this be '<='?
nit: IMO 'vreg->drms_mode_max_uA[i] >= load_uA' would be more readable.
> + break;
> +
> + return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
> +}
> +
> +static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
> + bool enable)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int ret;
> +
> + if (vreg->bypassed == enable)
> + return 0;
> +
> + ret = rpmh_regulator_vrm_set_mode_bypass(vreg, vreg->mode, enable);
> + if (!ret)
> + vreg->bypassed = enable;
> +
> + return ret;
> +}
> +
> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
> + bool *enable)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + *enable = vreg->bypassed;
> +
> + return 0;
> +}
> +
> +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_sel = rpmh_regulator_vrm_set_voltage_sel,
> + .get_voltage_sel = rpmh_regulator_vrm_get_voltage_sel,
> + .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_vrm_bypass_ops = {
> + .enable = rpmh_regulator_enable,
> + .disable = rpmh_regulator_disable,
> + .is_enabled = rpmh_regulator_is_enabled,
> + .set_voltage_sel = rpmh_regulator_vrm_set_voltage_sel,
> + .get_voltage_sel = rpmh_regulator_vrm_get_voltage_sel,
> + .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,
> + .set_bypass = rpmh_regulator_vrm_set_bypass,
> + .get_bypass = rpmh_regulator_vrm_get_bypass,
> +};
> +
> +static const struct regulator_ops rpmh_regulator_xob_ops = {
> + .enable = rpmh_regulator_enable,
> + .disable = rpmh_regulator_disable,
> + .is_enabled = rpmh_regulator_is_enabled,
> +};
> +
> +/**
> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
> + * for a VRM RPMh resource from device tree
> + * vreg: Pointer to the individual rpmh-regulator resource
> + * dev: Pointer to the top level rpmh-regulator PMIC device
> + * node: Pointer to the individual rpmh-regulator resource
> + * device node
> + *
> + * This function initializes the drms_mode[] and drms_mode_max_uA[] arrays 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 *dev, struct device_node *node)
> +{
> + const char *prop;
> + int i, len, ret, mode;
> + u32 *buf;
> +
> + /* qcom,allowed-drms-modes is optional */
> + prop = "qcom,allowed-drms-modes";
> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
> + if (len < 0)
> + return 0;
> +
> + vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
> + GFP_KERNEL);
> + vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
> + sizeof(*vreg->drms_mode_max_uA), GFP_KERNEL);
> + if (!vreg->drms_mode || !vreg->drms_mode_max_uA)
> + return -ENOMEM;
> + vreg->drms_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) {
> + dev_err(dev, "%s: unable to read %s, ret=%d\n",
> + node->name, prop, ret);
> + goto done;
> + }
> +
> + for (i = 0; i < len; i++) {
> + mode = vreg->hw_data->of_map_mode(buf[i]);
> + if (mode <= 0) {
> + dev_err(dev, "%s: element %d of %s = %u is invalid for this regulator\n",
> + node->name, i, prop, buf[i]);
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + vreg->drms_mode[i] = mode;
> + }
> +
> + prop = "qcom,drms-mode-threshold-currents";
> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
> + if (len != vreg->drms_mode_count) {
> + dev_err(dev, "%s: invalid element count=%d for %s\n",
> + node->name, len, prop);
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + ret = of_property_read_u32_array(node, prop, buf, len);
> + if (ret < 0) {
> + dev_err(dev, "%s: unable to read %s, ret=%d\n",
> + node->name, prop, ret);
> + goto done;
> + }
> +
> + for (i = 0; i < len; i++) {
> + vreg->drms_mode_max_uA[i] = buf[i];
> +
> + if (i > 0 && vreg->drms_mode_max_uA[i]
> + <= vreg->drms_mode_max_uA[i - 1]) {
> + dev_err(dev, "%s: %s elements are not in ascending order\n",
> + node->name, prop);
> + ret = -EINVAL;
> + goto done;
> + }
> + }
> +
> +done:
> + kfree(buf);
> + return ret;
> +}
> +
> +/**
> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
> + * request for this regulator based on optional device tree
> + * properties
> + * vreg: Pointer to the individual rpmh-regulator resource
> + * dev: Pointer to the top level rpmh-regulator PMIC device
> + * node: Pointer to the individual rpmh-regulator resource
> + * device node
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg,
> + struct device *dev, struct device_node *node)
> +{
> + struct tcs_cmd cmd[2] = {};
> + const struct regulator_linear_range *range;
> + const char *prop;
> + int cmd_count = 0;
> + int ret, selector;
> + u32 uV;
> +
> + if (vreg->hw_data->regulator_type == VRM) {
> + prop = "qcom,headroom-voltage";
> + ret = of_property_read_u32(node, prop, &uV);
> + if (!ret) {
> + if (uV > RPMH_VRM_HEADROOM_MAX_UV) {
> + dev_err(dev, "%s: %s=%u is invalid\n",
> + node->name, prop, uV);
> + return -EINVAL;
> + }
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
> + cmd[cmd_count++].data = DIV_ROUND_UP(uV, 1000);
> + }
> +
> + prop = "qcom,regulator-initial-voltage";
> + ret = of_property_read_u32(node, prop, &uV);
> + if (!ret) {
> + range = &vreg->hw_data->voltage_range;
> + selector = DIV_ROUND_UP(uV - range->min_uV,
> + range->uV_step) + range->min_sel;
> + if (uV < range->min_uV || selector > range->max_sel) {
> + dev_err(dev, "%s: %s=%u is invalid\n",
> + node->name, prop, uV);
> + return -EINVAL;
> + }
> +
> + vreg->voltage_selector = selector;
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
> + cmd[cmd_count++].data
> + = DIV_ROUND_UP(selector * range->uV_step
> + + range->min_uV, 1000);
> + }
> + }
> +
> + if (cmd_count) {
> + ret = rpmh_regulator_send_request(vreg, cmd, cmd_count, true);
> + if (ret < 0) {
> + dev_err(dev, "%s: could not send default config, ret=%d\n",
> + node->name, ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * rpmh_regulator_init_vreg() - initialize all attributes of an rpmh-regulator
> + * vreg: Pointer to the individual rpmh-regulator resource
> + * dev: Pointer to the top level rpmh-regulator PMIC device
> + * node: Pointer to the individual rpmh-regulator resource
> + * device node
> + * pmic_id: String used to identify the top level rpmh-regulator
> + * PMIC device on the board
> + * rpmh_data: Pointer to a null-terminated array of rpmh-regulator
> + * resources defined for the top level PMIC device
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
> + struct device_node *node, const char *pmic_id,
> + const struct rpmh_vreg_init_data *rpmh_data)
> +{
> + struct regulator_config reg_config = {};
> + char rpmh_resource_name[20] = "";
> + struct regulator_dev *rdev;
> + enum rpmh_regulator_type type;
> + struct regulator_init_data *init_data;
> + unsigned int mode;
> + int i, ret;
> +
> + for (; rpmh_data->name; rpmh_data++)
> + if (!strcmp(rpmh_data->name, node->name))
> + break;
> +
> + if (!rpmh_data->name) {
> + dev_err(dev, "Unknown regulator %s\n", node->name);
> + return -EINVAL;
> + }
> +
> + scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
> + rpmh_data->resource_name, pmic_id);
> +
> + vreg->addr = cmd_db_read_addr(rpmh_resource_name);
> + if (!vreg->addr) {
> + dev_err(dev, "%s: could not find RPMh address for resource %s\n",
> + node->name, rpmh_resource_name);
> + return -ENODEV;
> + }
> +
> + vreg->rdesc.name = rpmh_data->name;
> + vreg->rdesc.supply_name = rpmh_data->supply_name;
> + vreg->regulator_type = rpmh_data->hw_data->regulator_type;
> + vreg->hw_data = rpmh_data->hw_data;
> +
> + if (rpmh_data->hw_data->n_voltages) {
> + 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;
> + }
> +
> + type = vreg->regulator_type;
> +
> + if (type == VRM) {
> + ret = rpmh_regulator_parse_vrm_modes(vreg, dev, node);
> + if (ret < 0)
> + return ret;
> + }
> +
> + vreg->always_wait_for_ack = of_property_read_bool(node,
> + "qcom,always-wait-for-ack");
> +
> + vreg->rdesc.owner = THIS_MODULE;
> + vreg->rdesc.type = REGULATOR_VOLTAGE;
> + vreg->rdesc.ops = vreg->hw_data->ops;
> + vreg->rdesc.of_map_mode = vreg->hw_data->of_map_mode;
> +
> + init_data = of_get_regulator_init_data(dev, node, &vreg->rdesc);
> + if (!init_data)
> + return -ENOMEM;
> +
> + if (type == XOB && init_data->constraints.min_uV) {
> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
> + vreg->rdesc.n_voltages = 1;
> + }
> +
> + if (vreg->hw_data->of_map_mode) {
> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
> + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++) {
> + mode = vreg->hw_data->of_map_mode(i);
> + if (mode > 0)
> + init_data->constraints.valid_modes_mask |= mode;
> + }
> + }
> +
> + reg_config.dev = dev;
> + reg_config.init_data = init_data;
> + reg_config.of_node = node;
> + reg_config.driver_data = vreg;
> +
> + ret = rpmh_regulator_load_default_parameters(vreg, dev, node);
> + if (ret < 0)
> + return ret;
> +
> + rdev = devm_regulator_register(dev, &vreg->rdesc, ®_config);
> + if (IS_ERR(rdev)) {
> + ret = PTR_ERR(rdev);
> + rdev = NULL;
> + dev_err(dev, "%s: devm_regulator_register() failed, ret=%d\n",
> + node->name, ret);
> + return ret;
> + }
> +
> + dev_dbg(dev, "%s regulator registered for RPMh resource %s @ 0x%05X\n",
> + node->name, rpmh_resource_name, vreg->addr);
> +
> + return ret;
> +}
> +
> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
> + [REGULATOR_MODE_STANDBY] = 4,
> + [REGULATOR_MODE_IDLE] = 5,
> + [REGULATOR_MODE_NORMAL] = -EINVAL,
> + [REGULATOR_MODE_FAST] = 7,
> +};
Define constants for the modes on the PMIC4 side?
> +
> +static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode)
> +{
> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
> + [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY,
> + [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
> + [RPMH_REGULATOR_MODE_AUTO] = -EINVAL,
> + [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST,
> + };
> +
> + if (mode >= RPMH_REGULATOR_MODE_COUNT)
> + return -EINVAL;
> +
> + return of_mode_map[mode];
> +}
> +
> +static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
> + [REGULATOR_MODE_STANDBY] = 4,
> + [REGULATOR_MODE_IDLE] = 5,
> + [REGULATOR_MODE_NORMAL] = 6,
> + [REGULATOR_MODE_FAST] = 7,
> +};
> +
> +static unsigned int rpmh_regulator_pmic4_smps_of_map_mode(unsigned int mode)
> +{
> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
> + [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,
> + };
> +
> + if (mode >= RPMH_REGULATOR_MODE_COUNT)
> + return -EINVAL;
> +
> + return of_mode_map[mode];
> +}
> +
> +static const u32 pmic_mode_map_pmic4_bob[REGULATOR_MODE_STANDBY + 1] = {
> + [REGULATOR_MODE_STANDBY] = -EINVAL,
> + [REGULATOR_MODE_IDLE] = 1,
> + [REGULATOR_MODE_NORMAL] = 2,
> + [REGULATOR_MODE_FAST] = 3,
> +};
> +
> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
> +{
> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
> + [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,
> + };
> +
> + if (mode >= RPMH_REGULATOR_MODE_COUNT)
> + return -EINVAL;
> +
> + return of_mode_map[mode];
> +}
> +
> +static const struct rpmh_vreg_hw_data pmic4_pldo = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000),
> + .n_voltages = 256,
> + .pmic_mode_map = pmic_mode_map_pmic4_ldo,
> + .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_pldo_lv = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(1256000, 0, 127, 8000),
> + .n_voltages = 128,
> + .pmic_mode_map = pmic_mode_map_pmic4_ldo,
> + .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_nldo = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(312000, 0, 127, 8000),
> + .n_voltages = 128,
> + .pmic_mode_map = pmic_mode_map_pmic4_ldo,
> + .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_hfsmps3 = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 215, 8000),
> + .n_voltages = 216,
> + .pmic_mode_map = pmic_mode_map_pmic4_smps,
> + .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_ftsmps426 = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 258, 4000),
> + .n_voltages = 259,
> + .pmic_mode_map = pmic_mode_map_pmic4_smps,
> + .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_bob = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_bypass_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
> + .n_voltages = 84,
> + .pmic_mode_map = pmic_mode_map_pmic4_bob,
> + .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
> + .bypass_mode = 0,
> +};
> +
> +static const struct rpmh_vreg_hw_data pmic4_lvs = {
> + .regulator_type = XOB,
> + .ops = &rpmh_regulator_xob_ops,
> + /* LVS hardware does not support voltage or mode configuration. */
> +};
> +
> +#define RPMH_VREG(_name, _resource_name, _hw_data, _supply_name) \
> +{ \
> + .name = _name, \
> + .resource_name = _resource_name, \
> + .hw_data = _hw_data, \
> + .supply_name = _supply_name, \
> +}
> +
> +static const struct rpmh_vreg_init_data pm8998_vreg_data[] = {
> + RPMH_VREG("smps1", "smp%s1", &pmic4_ftsmps426, "vdd_s1"),
> + RPMH_VREG("smps2", "smp%s2", &pmic4_ftsmps426, "vdd_s2"),
> + RPMH_VREG("smps3", "smp%s3", &pmic4_hfsmps3, "vdd_s3"),
> + RPMH_VREG("smps4", "smp%s4", &pmic4_hfsmps3, "vdd_s4"),
> + RPMH_VREG("smps5", "smp%s5", &pmic4_hfsmps3, "vdd_s5"),
> + RPMH_VREG("smps6", "smp%s6", &pmic4_ftsmps426, "vdd_s6"),
> + RPMH_VREG("smps7", "smp%s7", &pmic4_ftsmps426, "vdd_s7"),
> + RPMH_VREG("smps8", "smp%s8", &pmic4_ftsmps426, "vdd_s8"),
> + RPMH_VREG("smps9", "smp%s9", &pmic4_ftsmps426, "vdd_s9"),
> + RPMH_VREG("smps10", "smp%s10", &pmic4_ftsmps426, "vdd_s10"),
> + RPMH_VREG("smps11", "smp%s11", &pmic4_ftsmps426, "vdd_s11"),
> + RPMH_VREG("smps12", "smp%s12", &pmic4_ftsmps426, "vdd_s12"),
> + RPMH_VREG("smps13", "smp%s13", &pmic4_ftsmps426, "vdd_s13"),
> + RPMH_VREG("ldo1", "ldo%s1", &pmic4_nldo, "vdd_l1_l27"),
> + RPMH_VREG("ldo2", "ldo%s2", &pmic4_nldo, "vdd_l2_l8_l17"),
> + RPMH_VREG("ldo3", "ldo%s3", &pmic4_nldo, "vdd_l3_l11"),
> + RPMH_VREG("ldo4", "ldo%s4", &pmic4_nldo, "vdd_l4_l5"),
> + RPMH_VREG("ldo5", "ldo%s5", &pmic4_nldo, "vdd_l4_l5"),
> + RPMH_VREG("ldo6", "ldo%s6", &pmic4_pldo, "vdd_l6"),
> + RPMH_VREG("ldo7", "ldo%s7", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
> + RPMH_VREG("ldo8", "ldo%s8", &pmic4_nldo, "vdd_l2_l8_l17"),
> + RPMH_VREG("ldo9", "ldo%s9", &pmic4_pldo, "vdd_l9"),
> + RPMH_VREG("ldo10", "ldo%s10", &pmic4_pldo, "vdd_l10_l23_l25"),
> + RPMH_VREG("ldo11", "ldo%s11", &pmic4_nldo, "vdd_l3_l11"),
> + RPMH_VREG("ldo12", "ldo%s12", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
> + RPMH_VREG("ldo13", "ldo%s13", &pmic4_pldo, "vdd_l13_l19_l21"),
> + RPMH_VREG("ldo14", "ldo%s14", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
> + RPMH_VREG("ldo15", "ldo%s15", &pmic4_pldo_lv, "vdd_l7_l12_l14_l15"),
> + RPMH_VREG("ldo16", "ldo%s16", &pmic4_pldo, "vdd_l16_l28"),
> + RPMH_VREG("ldo17", "ldo%s17", &pmic4_nldo, "vdd_l2_l8_l17"),
> + RPMH_VREG("ldo18", "ldo%s18", &pmic4_pldo, "vdd_l18_l22"),
> + RPMH_VREG("ldo19", "ldo%s19", &pmic4_pldo, "vdd_l13_l19_l21"),
> + RPMH_VREG("ldo20", "ldo%s20", &pmic4_pldo, "vdd_l20_l24"),
> + RPMH_VREG("ldo21", "ldo%s21", &pmic4_pldo, "vdd_l13_l19_l21"),
> + RPMH_VREG("ldo22", "ldo%s22", &pmic4_pldo, "vdd_l18_l22"),
> + RPMH_VREG("ldo23", "ldo%s23", &pmic4_pldo, "vdd_l10_l23_l25"),
> + RPMH_VREG("ldo24", "ldo%s24", &pmic4_pldo, "vdd_l20_l24"),
> + RPMH_VREG("ldo25", "ldo%s25", &pmic4_pldo, "vdd_l10_l23_l25"),
> + RPMH_VREG("ldo26", "ldo%s26", &pmic4_nldo, "vdd_l26"),
> + RPMH_VREG("ldo27", "ldo%s27", &pmic4_nldo, "vdd_l1_l27"),
> + RPMH_VREG("ldo28", "ldo%s28", &pmic4_pldo, "vdd_l16_l28"),
> + RPMH_VREG("lvs1", "vs%s1", &pmic4_lvs, "vdd_lvs1_lvs2"),
> + RPMH_VREG("lvs2", "vs%s2", &pmic4_lvs, "vdd_lvs1_lvs2"),
> + {},
> +};
> +
> +static const struct rpmh_vreg_init_data pmi8998_vreg_data[] = {
> + RPMH_VREG("bob", "bob%s1", &pmic4_bob, "vdd_bob"),
> + {},
> +};
> +
> +static const struct rpmh_vreg_init_data pm8005_vreg_data[] = {
> + RPMH_VREG("smps1", "smp%s1", &pmic4_ftsmps426, "vdd_s1"),
> + RPMH_VREG("smps2", "smp%s2", &pmic4_ftsmps426, "vdd_s2"),
> + RPMH_VREG("smps3", "smp%s3", &pmic4_ftsmps426, "vdd_s3"),
> + RPMH_VREG("smps4", "smp%s4", &pmic4_ftsmps426, "vdd_s4"),
> + {},
> +};
> +
> +static int rpmh_regulator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct rpmh_vreg_init_data *vreg_data;
> + struct rpmh_client *rpmh_client;
> + struct device_node *node;
> + struct rpmh_vreg *vreg;
> + const char *pmic_id;
> + int ret;
> +
> + ret = cmd_db_ready();
> + if (ret < 0) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
> + return ret;
> + }
> +
> + vreg_data = of_device_get_match_data(dev);
> + if (!vreg_data)
> + return -ENODEV;
> +
> + ret = of_property_read_string(dev->of_node, "qcom,pmic-id", &pmic_id);
> + if (ret < 0) {
> + dev_err(dev, "qcom,pmic-id missing in DT node\n");
> + return ret;
> + }
> +
> + rpmh_client = rpmh_get_client(pdev);
> + if (IS_ERR(rpmh_client))
> + return PTR_ERR(rpmh_client);
> + platform_set_drvdata(pdev, rpmh_client);
> +
> + for_each_available_child_of_node(dev->of_node, node) {
> + vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
> + if (!vreg) {
> + ret = -ENOMEM;
> + of_node_put(node);
> + goto cleanup;
> + }
> +
> + vreg->rpmh_client = rpmh_client;
> +
> + ret = rpmh_regulator_init_vreg(vreg, dev, node, pmic_id,
> + vreg_data);
> + if (ret < 0) {
> + of_node_put(node);
> + goto cleanup;
> + }
> + }
> +
> + return 0;
> +
> +cleanup:
> + rpmh_release(rpmh_client);
> +
> + return ret;
> +}
> +
> +static int rpmh_regulator_remove(struct platform_device *pdev)
> +{
> + struct rpmh_client *rpmh_client = platform_get_drvdata(pdev);
> +
> + rpmh_release(rpmh_client);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rpmh_regulator_match_table[] = {
> + {
> + .compatible = "qcom,pm8998-rpmh-regulators",
> + .data = pm8998_vreg_data,
> + },
> + {
> + .compatible = "qcom,pmi8998-rpmh-regulators",
> + .data = pmi8998_vreg_data,
> + },
> + {
> + .compatible = "qcom,pm8005-rpmh-regulators",
> + .data = pm8005_vreg_data,
> + },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table);
> +
> +static struct platform_driver rpmh_regulator_driver = {
> + .driver = {
> + .name = "qcom-rpmh-regulator",
> + .of_match_table = of_match_ptr(rpmh_regulator_match_table),
> + },
> + .probe = rpmh_regulator_probe,
> + .remove = rpmh_regulator_remove,
> +};
> +module_platform_driver(rpmh_regulator_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm RPMh regulator driver");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [v2,2/2] regulator: add QCOM RPMh regulator driver
2018-04-17 18:23 ` Matthias Kaehlcke
@ 2018-04-17 19:15 ` David Collins
-1 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-17 19:15 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: broonie, lgirdwood, robh+dt, mark.rutland, linux-arm-msm,
linux-arm-kernel, devicetree, linux-kernel, rnayak, sboyd,
dianders
On 04/17/2018 11:23 AM, Matthias Kaehlcke wrote:
> On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
>> Add the QCOM RPMh regulator driver to manage PMIC regulators
>> which are controlled via RPMh on some Qualcomm Technologies, Inc.
>> SoCs. RPMh is a hardware block which contains several
>> accelerators which are used to manage various hardware resources
>> that are shared between the processors of the SoC. The final
>> hardware state of a regulator is determined within RPMh by
>> performing max aggregation of the requests made by all of the
>> processors.
>> [...]
>> +/**
>> + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
>> + * @regulator_type: RPMh accelerator type used to manage this
>> + * regulator
>> + * @ops: Pointer to regulator ops callback structure
>> + * @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
>> + * @pmic_mode_map: Array indexed by regulator framework mode
>> + * containing PMIC hardware modes. Must be large
>> + * enough to index all framework modes supported
>> + * by this regulator hardware type.
>> + * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined
>> + * in device tree to a regulator framework mode
>
> The name of the field is a bit misleading, this is a map of RPMh mode
> to regulator framework mode, the device tree just happens to be the
> place where this mapping is defined.
of_map_mode name is used here to match the struct regulator_desc field by
the same name that it is assigned to [1]. Do you think that the name
should be changed to something else?
>> +/**
>> + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
>> + * single regulator device
>> + * @rpmh_client: Handle used for rpmh communications
>
> nit: RPMh
I'll change this.
>> +struct rpmh_vreg {
>> + struct rpmh_client *rpmh_client;
>> + u32 addr;
>> + struct regulator_desc rdesc;
>> + const struct rpmh_vreg_hw_data *hw_data;
>> + enum rpmh_regulator_type regulator_type;
>
> This value is already available via rpmh_vreg->hw_data->regulator_type,
> why duplicate it? The field is assigned in rpmh_regulator_init_vreg()
> and only read once in the same function, there seems to be no need for
> it, not even to improve readability.
This is present to specifically allow for a future change to support
overriding the regulator_type value from device tree in order to force
RPMh resources to be handled via XOB instead of VRM in a board-specific
manner. I included support of the property qcom,rpmh-resource-type in the
first version of this patch. I removed this property from the second
version of the patch based upon review feedback since SDM845 does not
explicitly need it (though an upcoming chip will).
I'll remove regulator_type from struct rpmh_vreg. It shouldn't be
particularly painful to add it back in when needed for XOB override support.
>> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + int i;
>> +
>> + for (i = 0; i < vreg->drms_mode_count - 1; i++)
>> + if (load_uA < vreg->drms_mode_max_uA[i])
>
> Shouldn't this be '<='?
>
> nit: IMO 'vreg->drms_mode_max_uA[i] >= load_uA' would be more readable.
I chose to use '<' here in order to maintain the non-inclusive limit
semantics of the downstream RPMh regulator driver. E.g. with an LPM
threshold of 10000 uA, load_uA == 10000 would result in a request for HPM
instead of LPM.
I suppose that I can change this to '<=' to be more logically consistent.
>> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>> + [REGULATOR_MODE_STANDBY] = 4,
>> + [REGULATOR_MODE_IDLE] = 5,
>> + [REGULATOR_MODE_NORMAL] = -EINVAL,
>> + [REGULATOR_MODE_FAST] = 7,
>> +};
>
> Define constants for the modes on the PMIC4 side?
Are you suggesting something like this?
#define PMIC4_LDO_MODE_RETENTION 4
#define PMIC4_LDO_MODE_LPM 5
#define PMIC4_LDO_MODE_HPM 7
static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
[REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
[REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM,
[REGULATOR_MODE_NORMAL] = -EINVAL,
[REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM,
};
#define PMIC4_SMPS_MODE_RETENTION 4
#define PMIC4_SMPS_MODE_PFM 5
#define PMIC4_SMPS_MODE_AUTO 6
#define PMIC4_SMPS_MODE_PWM 7
static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
[REGULATOR_MODE_STANDBY] = PMIC4_SMPS_MODE_RETENTION,
[REGULATOR_MODE_IDLE] = PMIC4_SMPS_MODE_PFM,
[REGULATOR_MODE_NORMAL] = PMIC4_SMPS_MODE_AUTO,
[REGULATOR_MODE_FAST] = PMIC4_SMPS_MODE_PWM,
};
I considered using this approach, but it didn't seem like it increased
readability and did increase the line count. Each of the constants would
only be used once. Would you prefer this style (or something else)?
Thanks,
David
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/regulator/driver.h?h=v4.17-rc1#n370
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 51+ messages in thread* [v2,2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-17 19:15 ` David Collins
0 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-17 19:15 UTC (permalink / raw)
To: linux-arm-kernel
On 04/17/2018 11:23 AM, Matthias Kaehlcke wrote:
> On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
>> Add the QCOM RPMh regulator driver to manage PMIC regulators
>> which are controlled via RPMh on some Qualcomm Technologies, Inc.
>> SoCs. RPMh is a hardware block which contains several
>> accelerators which are used to manage various hardware resources
>> that are shared between the processors of the SoC. The final
>> hardware state of a regulator is determined within RPMh by
>> performing max aggregation of the requests made by all of the
>> processors.
>> [...]
>> +/**
>> + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
>> + * @regulator_type: RPMh accelerator type used to manage this
>> + * regulator
>> + * @ops: Pointer to regulator ops callback structure
>> + * @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
>> + * @pmic_mode_map: Array indexed by regulator framework mode
>> + * containing PMIC hardware modes. Must be large
>> + * enough to index all framework modes supported
>> + * by this regulator hardware type.
>> + * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined
>> + * in device tree to a regulator framework mode
>
> The name of the field is a bit misleading, this is a map of RPMh mode
> to regulator framework mode, the device tree just happens to be the
> place where this mapping is defined.
of_map_mode name is used here to match the struct regulator_desc field by
the same name that it is assigned to [1]. Do you think that the name
should be changed to something else?
>> +/**
>> + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
>> + * single regulator device
>> + * @rpmh_client: Handle used for rpmh communications
>
> nit: RPMh
I'll change this.
>> +struct rpmh_vreg {
>> + struct rpmh_client *rpmh_client;
>> + u32 addr;
>> + struct regulator_desc rdesc;
>> + const struct rpmh_vreg_hw_data *hw_data;
>> + enum rpmh_regulator_type regulator_type;
>
> This value is already available via rpmh_vreg->hw_data->regulator_type,
> why duplicate it? The field is assigned in rpmh_regulator_init_vreg()
> and only read once in the same function, there seems to be no need for
> it, not even to improve readability.
This is present to specifically allow for a future change to support
overriding the regulator_type value from device tree in order to force
RPMh resources to be handled via XOB instead of VRM in a board-specific
manner. I included support of the property qcom,rpmh-resource-type in the
first version of this patch. I removed this property from the second
version of the patch based upon review feedback since SDM845 does not
explicitly need it (though an upcoming chip will).
I'll remove regulator_type from struct rpmh_vreg. It shouldn't be
particularly painful to add it back in when needed for XOB override support.
>> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + int i;
>> +
>> + for (i = 0; i < vreg->drms_mode_count - 1; i++)
>> + if (load_uA < vreg->drms_mode_max_uA[i])
>
> Shouldn't this be '<='?
>
> nit: IMO 'vreg->drms_mode_max_uA[i] >= load_uA' would be more readable.
I chose to use '<' here in order to maintain the non-inclusive limit
semantics of the downstream RPMh regulator driver. E.g. with an LPM
threshold of 10000 uA, load_uA == 10000 would result in a request for HPM
instead of LPM.
I suppose that I can change this to '<=' to be more logically consistent.
>> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>> + [REGULATOR_MODE_STANDBY] = 4,
>> + [REGULATOR_MODE_IDLE] = 5,
>> + [REGULATOR_MODE_NORMAL] = -EINVAL,
>> + [REGULATOR_MODE_FAST] = 7,
>> +};
>
> Define constants for the modes on the PMIC4 side?
Are you suggesting something like this?
#define PMIC4_LDO_MODE_RETENTION 4
#define PMIC4_LDO_MODE_LPM 5
#define PMIC4_LDO_MODE_HPM 7
static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
[REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
[REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM,
[REGULATOR_MODE_NORMAL] = -EINVAL,
[REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM,
};
#define PMIC4_SMPS_MODE_RETENTION 4
#define PMIC4_SMPS_MODE_PFM 5
#define PMIC4_SMPS_MODE_AUTO 6
#define PMIC4_SMPS_MODE_PWM 7
static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
[REGULATOR_MODE_STANDBY] = PMIC4_SMPS_MODE_RETENTION,
[REGULATOR_MODE_IDLE] = PMIC4_SMPS_MODE_PFM,
[REGULATOR_MODE_NORMAL] = PMIC4_SMPS_MODE_AUTO,
[REGULATOR_MODE_FAST] = PMIC4_SMPS_MODE_PWM,
};
I considered using this approach, but it didn't seem like it increased
readability and did increase the line count. Each of the constants would
only be used once. Would you prefer this style (or something else)?
Thanks,
David
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/regulator/driver.h?h=v4.17-rc1#n370
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [v2,2/2] regulator: add QCOM RPMh regulator driver
2018-04-17 19:15 ` David Collins
@ 2018-04-17 19:47 ` Matthias Kaehlcke
-1 siblings, 0 replies; 51+ messages in thread
From: Matthias Kaehlcke @ 2018-04-17 19:47 UTC (permalink / raw)
To: David Collins
Cc: broonie, lgirdwood, robh+dt, mark.rutland, linux-arm-msm,
linux-arm-kernel, devicetree, linux-kernel, rnayak, sboyd,
dianders
On Tue, Apr 17, 2018 at 12:15:18PM -0700, David Collins wrote:
> On 04/17/2018 11:23 AM, Matthias Kaehlcke wrote:
> > On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
> >> Add the QCOM RPMh regulator driver to manage PMIC regulators
> >> which are controlled via RPMh on some Qualcomm Technologies, Inc.
> >> SoCs. RPMh is a hardware block which contains several
> >> accelerators which are used to manage various hardware resources
> >> that are shared between the processors of the SoC. The final
> >> hardware state of a regulator is determined within RPMh by
> >> performing max aggregation of the requests made by all of the
> >> processors.
> >> [...]
> >> +/**
> >> + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
> >> + * @regulator_type: RPMh accelerator type used to manage this
> >> + * regulator
> >> + * @ops: Pointer to regulator ops callback structure
> >> + * @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
> >> + * @pmic_mode_map: Array indexed by regulator framework mode
> >> + * containing PMIC hardware modes. Must be large
> >> + * enough to index all framework modes supported
> >> + * by this regulator hardware type.
> >> + * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined
> >> + * in device tree to a regulator framework mode
> >
> > The name of the field is a bit misleading, this is a map of RPMh mode
> > to regulator framework mode, the device tree just happens to be the
> > place where this mapping is defined.
>
> of_map_mode name is used here to match the struct regulator_desc field by
> the same name that it is assigned to [1]. Do you think that the name
> should be changed to something else?
Thanks, I missed that it's the name of the field in struct
regulator_desc field, in that case it certainly makes sense to use the
same name.
> >> +/**
> >> + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
> >> + * single regulator device
> >> + * @rpmh_client: Handle used for rpmh communications
> >
> > nit: RPMh
>
> I'll change this.
>
>
> >> +struct rpmh_vreg {
> >> + struct rpmh_client *rpmh_client;
> >> + u32 addr;
> >> + struct regulator_desc rdesc;
> >> + const struct rpmh_vreg_hw_data *hw_data;
> >> + enum rpmh_regulator_type regulator_type;
> >
> > This value is already available via rpmh_vreg->hw_data->regulator_type,
> > why duplicate it? The field is assigned in rpmh_regulator_init_vreg()
> > and only read once in the same function, there seems to be no need for
> > it, not even to improve readability.
>
> This is present to specifically allow for a future change to support
> overriding the regulator_type value from device tree in order to force
> RPMh resources to be handled via XOB instead of VRM in a board-specific
> manner. I included support of the property qcom,rpmh-resource-type in the
> first version of this patch. I removed this property from the second
> version of the patch based upon review feedback since SDM845 does not
> explicitly need it (though an upcoming chip will).
>
> I'll remove regulator_type from struct rpmh_vreg. It shouldn't be
> particularly painful to add it back in when needed for XOB override support.
>
>
> >> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
> >> +{
> >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> >> + int i;
> >> +
> >> + for (i = 0; i < vreg->drms_mode_count - 1; i++)
> >> + if (load_uA < vreg->drms_mode_max_uA[i])
> >
> > Shouldn't this be '<='?
> >
> > nit: IMO 'vreg->drms_mode_max_uA[i] >= load_uA' would be more readable.
>
> I chose to use '<' here in order to maintain the non-inclusive limit
> semantics of the downstream RPMh regulator driver. E.g. with an LPM
> threshold of 10000 uA, load_uA == 10000 would result in a request for HPM
> instead of LPM.
>
> I suppose that I can change this to '<=' to be more logically consistent.
>
>
> >> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
> >> + [REGULATOR_MODE_STANDBY] = 4,
> >> + [REGULATOR_MODE_IDLE] = 5,
> >> + [REGULATOR_MODE_NORMAL] = -EINVAL,
> >> + [REGULATOR_MODE_FAST] = 7,
> >> +};
> >
> > Define constants for the modes on the PMIC4 side?
>
> Are you suggesting something like this?
>
> #define PMIC4_LDO_MODE_RETENTION 4
> #define PMIC4_LDO_MODE_LPM 5
> #define PMIC4_LDO_MODE_HPM 7
>
> static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
> [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
> [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM,
> [REGULATOR_MODE_NORMAL] = -EINVAL,
> [REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM,
> };
>
> #define PMIC4_SMPS_MODE_RETENTION 4
> #define PMIC4_SMPS_MODE_PFM 5
> #define PMIC4_SMPS_MODE_AUTO 6
> #define PMIC4_SMPS_MODE_PWM 7
>
> static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
> [REGULATOR_MODE_STANDBY] = PMIC4_SMPS_MODE_RETENTION,
> [REGULATOR_MODE_IDLE] = PMIC4_SMPS_MODE_PFM,
> [REGULATOR_MODE_NORMAL] = PMIC4_SMPS_MODE_AUTO,
> [REGULATOR_MODE_FAST] = PMIC4_SMPS_MODE_PWM,
> };
>
> I considered using this approach, but it didn't seem like it increased
> readability and did increase the line count. Each of the constants would
> only be used once. Would you prefer this style (or something else)?
Personally I prefer this style, since the constants are more
expressive than the literals. I agree that the 'inline' constant
definition is a bit noisy, perhaps it would be better to move the
definitions to the top of the file or group them before the definition
of pmic_mode_map_pmic4_ldo. Alteratively you could create a
drivers/regulator/qcom_rpmh-regulator.h and also move the definitions
of struct struct rpmh_vreg_hw_data, rpmh_vreg, ... there.
Thanks
Matthias
^ permalink raw reply [flat|nested] 51+ messages in thread* [v2,2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-17 19:47 ` Matthias Kaehlcke
0 siblings, 0 replies; 51+ messages in thread
From: Matthias Kaehlcke @ 2018-04-17 19:47 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Apr 17, 2018 at 12:15:18PM -0700, David Collins wrote:
> On 04/17/2018 11:23 AM, Matthias Kaehlcke wrote:
> > On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
> >> Add the QCOM RPMh regulator driver to manage PMIC regulators
> >> which are controlled via RPMh on some Qualcomm Technologies, Inc.
> >> SoCs. RPMh is a hardware block which contains several
> >> accelerators which are used to manage various hardware resources
> >> that are shared between the processors of the SoC. The final
> >> hardware state of a regulator is determined within RPMh by
> >> performing max aggregation of the requests made by all of the
> >> processors.
> >> [...]
> >> +/**
> >> + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
> >> + * @regulator_type: RPMh accelerator type used to manage this
> >> + * regulator
> >> + * @ops: Pointer to regulator ops callback structure
> >> + * @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
> >> + * @pmic_mode_map: Array indexed by regulator framework mode
> >> + * containing PMIC hardware modes. Must be large
> >> + * enough to index all framework modes supported
> >> + * by this regulator hardware type.
> >> + * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined
> >> + * in device tree to a regulator framework mode
> >
> > The name of the field is a bit misleading, this is a map of RPMh mode
> > to regulator framework mode, the device tree just happens to be the
> > place where this mapping is defined.
>
> of_map_mode name is used here to match the struct regulator_desc field by
> the same name that it is assigned to [1]. Do you think that the name
> should be changed to something else?
Thanks, I missed that it's the name of the field in struct
regulator_desc field, in that case it certainly makes sense to use the
same name.
> >> +/**
> >> + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
> >> + * single regulator device
> >> + * @rpmh_client: Handle used for rpmh communications
> >
> > nit: RPMh
>
> I'll change this.
>
>
> >> +struct rpmh_vreg {
> >> + struct rpmh_client *rpmh_client;
> >> + u32 addr;
> >> + struct regulator_desc rdesc;
> >> + const struct rpmh_vreg_hw_data *hw_data;
> >> + enum rpmh_regulator_type regulator_type;
> >
> > This value is already available via rpmh_vreg->hw_data->regulator_type,
> > why duplicate it? The field is assigned in rpmh_regulator_init_vreg()
> > and only read once in the same function, there seems to be no need for
> > it, not even to improve readability.
>
> This is present to specifically allow for a future change to support
> overriding the regulator_type value from device tree in order to force
> RPMh resources to be handled via XOB instead of VRM in a board-specific
> manner. I included support of the property qcom,rpmh-resource-type in the
> first version of this patch. I removed this property from the second
> version of the patch based upon review feedback since SDM845 does not
> explicitly need it (though an upcoming chip will).
>
> I'll remove regulator_type from struct rpmh_vreg. It shouldn't be
> particularly painful to add it back in when needed for XOB override support.
>
>
> >> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
> >> +{
> >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> >> + int i;
> >> +
> >> + for (i = 0; i < vreg->drms_mode_count - 1; i++)
> >> + if (load_uA < vreg->drms_mode_max_uA[i])
> >
> > Shouldn't this be '<='?
> >
> > nit: IMO 'vreg->drms_mode_max_uA[i] >= load_uA' would be more readable.
>
> I chose to use '<' here in order to maintain the non-inclusive limit
> semantics of the downstream RPMh regulator driver. E.g. with an LPM
> threshold of 10000 uA, load_uA == 10000 would result in a request for HPM
> instead of LPM.
>
> I suppose that I can change this to '<=' to be more logically consistent.
>
>
> >> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
> >> + [REGULATOR_MODE_STANDBY] = 4,
> >> + [REGULATOR_MODE_IDLE] = 5,
> >> + [REGULATOR_MODE_NORMAL] = -EINVAL,
> >> + [REGULATOR_MODE_FAST] = 7,
> >> +};
> >
> > Define constants for the modes on the PMIC4 side?
>
> Are you suggesting something like this?
>
> #define PMIC4_LDO_MODE_RETENTION 4
> #define PMIC4_LDO_MODE_LPM 5
> #define PMIC4_LDO_MODE_HPM 7
>
> static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
> [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
> [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM,
> [REGULATOR_MODE_NORMAL] = -EINVAL,
> [REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM,
> };
>
> #define PMIC4_SMPS_MODE_RETENTION 4
> #define PMIC4_SMPS_MODE_PFM 5
> #define PMIC4_SMPS_MODE_AUTO 6
> #define PMIC4_SMPS_MODE_PWM 7
>
> static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
> [REGULATOR_MODE_STANDBY] = PMIC4_SMPS_MODE_RETENTION,
> [REGULATOR_MODE_IDLE] = PMIC4_SMPS_MODE_PFM,
> [REGULATOR_MODE_NORMAL] = PMIC4_SMPS_MODE_AUTO,
> [REGULATOR_MODE_FAST] = PMIC4_SMPS_MODE_PWM,
> };
>
> I considered using this approach, but it didn't seem like it increased
> readability and did increase the line count. Each of the constants would
> only be used once. Would you prefer this style (or something else)?
Personally I prefer this style, since the constants are more
expressive than the literals. I agree that the 'inline' constant
definition is a bit noisy, perhaps it would be better to move the
definitions to the top of the file or group them before the definition
of pmic_mode_map_pmic4_ldo. Alteratively you could create a
drivers/regulator/qcom_rpmh-regulator.h and also move the definitions
of struct struct rpmh_vreg_hw_data, rpmh_vreg, ... there.
Thanks
Matthias
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [v2,2/2] regulator: add QCOM RPMh regulator driver
2018-04-17 19:47 ` Matthias Kaehlcke
@ 2018-04-18 21:34 ` David Collins
-1 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-18 21:34 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: broonie, lgirdwood, robh+dt, mark.rutland, linux-arm-msm,
linux-arm-kernel, devicetree, linux-kernel, rnayak, sboyd,
dianders
On 04/17/2018 12:47 PM, Matthias Kaehlcke wrote:
> On Tue, Apr 17, 2018 at 12:15:18PM -0700, David Collins wrote:
>> On 04/17/2018 11:23 AM, Matthias Kaehlcke wrote:
>>> On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
>>>> [...]
>>>> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>>>> + [REGULATOR_MODE_STANDBY] = 4,
>>>> + [REGULATOR_MODE_IDLE] = 5,
>>>> + [REGULATOR_MODE_NORMAL] = -EINVAL,
>>>> + [REGULATOR_MODE_FAST] = 7,
>>>> +};
>>>
>>> Define constants for the modes on the PMIC4 side?
>>
>> Are you suggesting something like this?
>>
>> #define PMIC4_LDO_MODE_RETENTION 4
>> #define PMIC4_LDO_MODE_LPM 5
>> #define PMIC4_LDO_MODE_HPM 7
>>
>> static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>> [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
>> [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM,
>> [REGULATOR_MODE_NORMAL] = -EINVAL,
>> [REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM,
>> };
>>
>> #define PMIC4_SMPS_MODE_RETENTION 4
>> #define PMIC4_SMPS_MODE_PFM 5
>> #define PMIC4_SMPS_MODE_AUTO 6
>> #define PMIC4_SMPS_MODE_PWM 7
>>
>> static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
>> [REGULATOR_MODE_STANDBY] = PMIC4_SMPS_MODE_RETENTION,
>> [REGULATOR_MODE_IDLE] = PMIC4_SMPS_MODE_PFM,
>> [REGULATOR_MODE_NORMAL] = PMIC4_SMPS_MODE_AUTO,
>> [REGULATOR_MODE_FAST] = PMIC4_SMPS_MODE_PWM,
>> };
>>
>> I considered using this approach, but it didn't seem like it increased
>> readability and did increase the line count. Each of the constants would
>> only be used once. Would you prefer this style (or something else)?
>
> Personally I prefer this style, since the constants are more
> expressive than the literals. I agree that the 'inline' constant
> definition is a bit noisy, perhaps it would be better to move the
> definitions to the top of the file or group them before the definition
> of pmic_mode_map_pmic4_ldo. Alteratively you could create a
> drivers/regulator/qcom_rpmh-regulator.h and also move the definitions
> of struct struct rpmh_vreg_hw_data, rpmh_vreg, ... there.
I'll add constants for the per-type regulator modes at the top of the
file. We'll see if other reviewers like them.
Take care,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 51+ messages in thread* [v2,2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-18 21:34 ` David Collins
0 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-18 21:34 UTC (permalink / raw)
To: linux-arm-kernel
On 04/17/2018 12:47 PM, Matthias Kaehlcke wrote:
> On Tue, Apr 17, 2018 at 12:15:18PM -0700, David Collins wrote:
>> On 04/17/2018 11:23 AM, Matthias Kaehlcke wrote:
>>> On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
>>>> [...]
>>>> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>>>> + [REGULATOR_MODE_STANDBY] = 4,
>>>> + [REGULATOR_MODE_IDLE] = 5,
>>>> + [REGULATOR_MODE_NORMAL] = -EINVAL,
>>>> + [REGULATOR_MODE_FAST] = 7,
>>>> +};
>>>
>>> Define constants for the modes on the PMIC4 side?
>>
>> Are you suggesting something like this?
>>
>> #define PMIC4_LDO_MODE_RETENTION 4
>> #define PMIC4_LDO_MODE_LPM 5
>> #define PMIC4_LDO_MODE_HPM 7
>>
>> static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>> [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
>> [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM,
>> [REGULATOR_MODE_NORMAL] = -EINVAL,
>> [REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM,
>> };
>>
>> #define PMIC4_SMPS_MODE_RETENTION 4
>> #define PMIC4_SMPS_MODE_PFM 5
>> #define PMIC4_SMPS_MODE_AUTO 6
>> #define PMIC4_SMPS_MODE_PWM 7
>>
>> static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
>> [REGULATOR_MODE_STANDBY] = PMIC4_SMPS_MODE_RETENTION,
>> [REGULATOR_MODE_IDLE] = PMIC4_SMPS_MODE_PFM,
>> [REGULATOR_MODE_NORMAL] = PMIC4_SMPS_MODE_AUTO,
>> [REGULATOR_MODE_FAST] = PMIC4_SMPS_MODE_PWM,
>> };
>>
>> I considered using this approach, but it didn't seem like it increased
>> readability and did increase the line count. Each of the constants would
>> only be used once. Would you prefer this style (or something else)?
>
> Personally I prefer this style, since the constants are more
> expressive than the literals. I agree that the 'inline' constant
> definition is a bit noisy, perhaps it would be better to move the
> definitions to the top of the file or group them before the definition
> of pmic_mode_map_pmic4_ldo. Alteratively you could create a
> drivers/regulator/qcom_rpmh-regulator.h and also move the definitions
> of struct struct rpmh_vreg_hw_data, rpmh_vreg, ... there.
I'll add constants for the per-type regulator modes at the top of the
file. We'll see if other reviewers like them.
Take care,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [v2,2/2] regulator: add QCOM RPMh regulator driver
2018-04-17 18:23 ` Matthias Kaehlcke
@ 2018-04-18 17:02 ` Mark Brown
-1 siblings, 0 replies; 51+ messages in thread
From: Mark Brown @ 2018-04-18 17:02 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David Collins, lgirdwood, robh+dt, mark.rutland, linux-arm-msm,
linux-arm-kernel, devicetree, linux-kernel, rnayak, sboyd,
dianders
[-- Attachment #1: Type: text/plain, Size: 546 bytes --]
On Tue, Apr 17, 2018 at 11:23:47AM -0700, Matthias Kaehlcke wrote:
> On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
> > Add the QCOM RPMh regulator driver to manage PMIC regulators
> > which are controlled via RPMh on some Qualcomm Technologies, Inc.
> > SoCs. RPMh is a hardware block which contains several
Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* [v2,2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-18 17:02 ` Mark Brown
0 siblings, 0 replies; 51+ messages in thread
From: Mark Brown @ 2018-04-18 17:02 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Apr 17, 2018 at 11:23:47AM -0700, Matthias Kaehlcke wrote:
> On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
> > Add the QCOM RPMh regulator driver to manage PMIC regulators
> > which are controlled via RPMh on some Qualcomm Technologies, Inc.
> > SoCs. RPMh is a hardware block which contains several
Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180418/221bb544/attachment.sig>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
2018-04-14 2:50 ` David Collins
@ 2018-04-17 20:02 ` Doug Anderson
-1 siblings, 0 replies; 51+ messages in thread
From: Doug Anderson @ 2018-04-17 20:02 UTC (permalink / raw)
To: David Collins
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd, Matthias Kaehlcke
Hi,
On Fri, Apr 13, 2018 at 7:50 PM, David Collins <collinsd@codeaurora.org> wrote:
> +#define RPMH_REGULATOR_DISABLE 0x0
> +#define RPMH_REGULATOR_ENABLE 0x1
In the last version Stephen said he didn't like the above two #defines
and you said you'd remove them, yet they are still here. Explanation?
> + * @drms_mode: Array of regulator framework modes which can
> + * be configured dynamically for this regulator
> + * via the set_load() callback.
Using the singular for something that is an array is confusing. Why
not "drms_modes" or "drms_mode_arr"? In the past review you said
'Perhaps something along the lines of "drms_modes"'.
> +struct rpmh_vreg {
> + struct rpmh_client *rpmh_client;
> + u32 addr;
> + struct regulator_desc rdesc;
> + const struct rpmh_vreg_hw_data *hw_data;
> + enum rpmh_regulator_type regulator_type;
> + bool always_wait_for_ack;
> + unsigned int *drms_mode;
> + int *drms_mode_max_uA;
> + size_t drms_mode_count;
> +
> + bool enabled;
> + int voltage_selector;
> + unsigned int mode;
> + bool bypassed;
nit: stick next to "enabled" to make slightly better structure packing...
> +static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
> + unsigned int selector)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
> + };
> + int ret;
> +
> + /* VRM voltage control register is set with voltage in millivolts. */
> + cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
> + selector), 1000);
> +
> + ret = rpmh_regulator_send_request(vreg, &cmd, 1,
> + selector > vreg->voltage_selector);
If you init vreg->voltage_selector to an error as I suggest in
rpmh_regulator_load_default_parameters() you'll need to handle it
here. See below.
> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
> + unsigned int mode, bool bypassed)
> +{
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
> + };
Please add:
if (mode & ~(REGULATOR_MODE_STANDBY |
REGULATOR_MODE_IDLE |
REGULATOR_MODE_NORMAL |
REGULATOR_MODE_FAST))
return -EINVAL;
That way if someone adds a new mode you don't index off the end of
your array. Ah, I see, you have this in rpmh_regulator_vrm_set_mode
by checking if mode > REGULATOR_MODE_STANDBY. That works. Can you
move it here so it's closer to where the array access is?
> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int i;
> +
> + for (i = 0; i < vreg->drms_mode_count - 1; i++)
> + if (load_uA < vreg->drms_mode_max_uA[i])
> + break;
Can you put a warning here if the requested load_uA is larger than the
largest supported, and/or perhaps consider it an error case?
> +
> + return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
Might not hurt to have a comment saying that this calls
rpmh_regulator_vrm_set_mode() instead of calling
rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
to change the mode returned by a future call to get_mode().
> +static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
> + bool enable)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int ret;
> +
> + if (vreg->bypassed == enable)
> + return 0;
Just like for enable, remove this check. The regulator core does it for you.
> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
> + bool *enable)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + *enable = vreg->bypassed;
> +
> + return 0;
Should you return an error code if nobody has ever called set_bypass?
...or is it OK to just return "not bypassed"? Please document this in
the code.
> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
> + struct device *dev, struct device_node *node)
> +{
> + const char *prop;
> + int i, len, ret, mode;
> + u32 *buf;
> +
> + /* qcom,allowed-drms-modes is optional */
> + prop = "qcom,allowed-drms-modes";
> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
> + if (len < 0)
> + return 0;
> +
> + vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
> + GFP_KERNEL);
> + vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
> + sizeof(*vreg->drms_mode_max_uA), GFP_KERNEL);
> + if (!vreg->drms_mode || !vreg->drms_mode_max_uA)
> + return -ENOMEM;
> + vreg->drms_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) {
> + dev_err(dev, "%s: unable to read %s, ret=%d\n",
> + node->name, prop, ret);
> + goto done;
> + }
> +
> + for (i = 0; i < len; i++) {
> + mode = vreg->hw_data->of_map_mode(buf[i]);
> + if (mode <= 0) {
Should be < 0, not <= 0 right? Unless we take Javier's suggestion
(see <https://patchwork.kernel.org/patch/10346081/>) and make 0 be
invalid...
> +/**
> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
> + * request for this regulator based on optional device tree
> + * properties
> + * vreg: Pointer to the individual rpmh-regulator resource
> + * dev: Pointer to the top level rpmh-regulator PMIC device
> + * node: Pointer to the individual rpmh-regulator resource
> + * device node
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg,
> + struct device *dev, struct device_node *node)
> +{
> + struct tcs_cmd cmd[2] = {};
> + const struct regulator_linear_range *range;
> + const char *prop;
> + int cmd_count = 0;
> + int ret, selector;
> + u32 uV;
> +
> + if (vreg->hw_data->regulator_type == VRM) {
> + prop = "qcom,headroom-voltage";
> + ret = of_property_read_u32(node, prop, &uV);
> + if (!ret) {
> + if (uV > RPMH_VRM_HEADROOM_MAX_UV) {
> + dev_err(dev, "%s: %s=%u is invalid\n",
> + node->name, prop, uV);
> + return -EINVAL;
> + }
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
> + cmd[cmd_count++].data = DIV_ROUND_UP(uV, 1000);
> + }
> +
> + prop = "qcom,regulator-initial-voltage";
> + ret = of_property_read_u32(node, prop, &uV);
> + if (!ret) {
> + range = &vreg->hw_data->voltage_range;
> + selector = DIV_ROUND_UP(uV - range->min_uV,
> + range->uV_step) + range->min_sel;
> + if (uV < range->min_uV || selector > range->max_sel) {
> + dev_err(dev, "%s: %s=%u is invalid\n",
> + node->name, prop, uV);
> + return -EINVAL;
> + }
> +
> + vreg->voltage_selector = selector;
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
> + cmd[cmd_count++].data
> + = DIV_ROUND_UP(selector * range->uV_step
> + + range->min_uV, 1000);
> + }
Seems like you want an "else { vreg->voltage_selector = -EINVAL; }".
Otherwise "get_voltage_sel" will return selector 0 before the first
set, right?
Previously Mark said: "If the driver can't read values it should
return an appropriate error code."
...and previously you said: "I'll try this out and see if the
regulator framework complains during regulator registration."
> +/**
> + * rpmh_regulator_init_vreg() - initialize all attributes of an rpmh-regulator
> + * vreg: Pointer to the individual rpmh-regulator resource
> + * dev: Pointer to the top level rpmh-regulator PMIC device
> + * node: Pointer to the individual rpmh-regulator resource
> + * device node
> + * pmic_id: String used to identify the top level rpmh-regulator
> + * PMIC device on the board
> + * rpmh_data: Pointer to a null-terminated array of rpmh-regulator
> + * resources defined for the top level PMIC device
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
> + struct device_node *node, const char *pmic_id,
> + const struct rpmh_vreg_init_data *rpmh_data)
> +{
> + struct regulator_config reg_config = {};
> + char rpmh_resource_name[20] = "";
> + struct regulator_dev *rdev;
> + enum rpmh_regulator_type type;
> + struct regulator_init_data *init_data;
> + unsigned int mode;
> + int i, ret;
> +
> + for (; rpmh_data->name; rpmh_data++)
> + if (!strcmp(rpmh_data->name, node->name))
> + break;
> +
> + if (!rpmh_data->name) {
> + dev_err(dev, "Unknown regulator %s\n", node->name);
> + return -EINVAL;
> + }
> +
> + scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
> + rpmh_data->resource_name, pmic_id);
If the resulting string is exactly 20 characters then
rpmh_resource_name won't be zero terminated. Please handle this
properly.
> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
I may have suggested using this as an array that could be used as a
"map" (index by regulator framework mode and get the PMIC mode), but
now that I see it it doesn't seem super appealing since the regulator
framework mode is not 0, 1, 2, 3 but is actually 1, 2, 4, 8. ...but I
guess it's not too bad--you're allocating 9 ints to map 4 elements and
I guess that's about as efficient as you're going to get even if it
feels a bit ugly.
...but still a few improvements:
* Don't specify the size of the array as "REGULATOR_MODE_STANDBY + 1".
Let the compiler handle this. It should do the right thing. Then if
someone ever changes the order of the #defines things will just work,
I think.
* Make sure that users of these arrays check that the mode is one of
the mode you know about. That way if someone does add a new mode you
don't index off your array. I'll put a comment in the user.
Also: the type of this array is 'u32' but you're assigning -EINVAL in
some cases. Please fix to be signed. Here and on other similar
arrays.
> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
> +{
> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
> + [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,
> + };
You're sticking a negative value in an array of unsigned inits. Here
and in other similar functions.
I know, I know. The function is defined to return an unsigned int.
It's wrong. of_regulator.c clearly puts the return code in a signed
int. First attempt at fixing this is at
<https://patchwork.kernel.org/patch/10346081/>.
> +static const struct rpmh_vreg_hw_data pmic4_bob = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_bypass_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
> + .n_voltages = 84,
> + .pmic_mode_map = pmic_mode_map_pmic4_bob,
> + .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
> + .bypass_mode = 0,
Remove .bypass_mode from the structure and just change
rpmh_regulator_vrm_set_mode_bypass() to set 0 if we're in bypass.
Right now 100% of PMICs that support bypass use 0 as the bypass mode.
If you ever have a future PMIC that uses a non-zero mode for bypass
then we can always add this back. ...and if no future PMICs ever use
a non-zero bypass mode then we don't need the complexity of having
another field in this struct.
If you don't do this you might get arguments from some people saying
that they don't like seeing inits of "= 0" in static structures (Linux
conventions seem to like you to just assume that structs are
zero-initted).
> +static int rpmh_regulator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct rpmh_vreg_init_data *vreg_data;
> + struct rpmh_client *rpmh_client;
> + struct device_node *node;
> + struct rpmh_vreg *vreg;
> + const char *pmic_id;
> + int ret;
> +
> + ret = cmd_db_ready();
> + if (ret < 0) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
> + return ret;
> + }
In the last patch Stephen said:
> We should just make rpmh parent device call cmd_db_ready() so that these
> devices aren't even populated until then and so that cmd_db_ready() is
> only in one place. Lina?
Any news here?
> +
> + vreg_data = of_device_get_match_data(dev);
> + if (!vreg_data)
> + return -ENODEV;
> +
> + ret = of_property_read_string(dev->of_node, "qcom,pmic-id", &pmic_id);
> + if (ret < 0) {
> + dev_err(dev, "qcom,pmic-id missing in DT node\n");
> + return ret;
> + }
> +
> + rpmh_client = rpmh_get_client(pdev);
> + if (IS_ERR(rpmh_client))
> + return PTR_ERR(rpmh_client);
> + platform_set_drvdata(pdev, rpmh_client);
> +
> + for_each_available_child_of_node(dev->of_node, node) {
> + vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
> + if (!vreg) {
> + ret = -ENOMEM;
> + of_node_put(node);
> + goto cleanup;
> + }
> +
> + vreg->rpmh_client = rpmh_client;
> +
> + ret = rpmh_regulator_init_vreg(vreg, dev, node, pmic_id,
> + vreg_data);
> + if (ret < 0) {
> + of_node_put(node);
> + goto cleanup;
> + }
> + }
> +
> + return 0;
> +
> +cleanup:
> + rpmh_release(rpmh_client);
Still no devm_rpmh_get_client()? If Lina is too busy spinning her
patch series for other stuff, just add it to RPMH as a patch in your
series. I believe it's just this (untested):
---
int devm_rpmh_release(struct device *dev, void *res)
{
struct platform_device *pdev = to_platform_device(dev);
rpmh_release(pdev);
}
int devm_rpmh_get_client(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
void *dr;
int rc;
dr = devres_alloc(devm_rpmh_release, 0, GFP_KERNEL);
if (!dr)
return -ENOMEM;
rc = rpmh_get_client(pdev);
if (!rc)
devres_add(dev, dr);
else
devres_free(dr);
return rc;
}
---
Note that you'll get rid of the error handling in probe, the whole
remove function, and the need to do platform_set_drvdata().
-Doug
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-17 20:02 ` Doug Anderson
0 siblings, 0 replies; 51+ messages in thread
From: Doug Anderson @ 2018-04-17 20:02 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Fri, Apr 13, 2018 at 7:50 PM, David Collins <collinsd@codeaurora.org> wrote:
> +#define RPMH_REGULATOR_DISABLE 0x0
> +#define RPMH_REGULATOR_ENABLE 0x1
In the last version Stephen said he didn't like the above two #defines
and you said you'd remove them, yet they are still here. Explanation?
> + * @drms_mode: Array of regulator framework modes which can
> + * be configured dynamically for this regulator
> + * via the set_load() callback.
Using the singular for something that is an array is confusing. Why
not "drms_modes" or "drms_mode_arr"? In the past review you said
'Perhaps something along the lines of "drms_modes"'.
> +struct rpmh_vreg {
> + struct rpmh_client *rpmh_client;
> + u32 addr;
> + struct regulator_desc rdesc;
> + const struct rpmh_vreg_hw_data *hw_data;
> + enum rpmh_regulator_type regulator_type;
> + bool always_wait_for_ack;
> + unsigned int *drms_mode;
> + int *drms_mode_max_uA;
> + size_t drms_mode_count;
> +
> + bool enabled;
> + int voltage_selector;
> + unsigned int mode;
> + bool bypassed;
nit: stick next to "enabled" to make slightly better structure packing...
> +static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
> + unsigned int selector)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
> + };
> + int ret;
> +
> + /* VRM voltage control register is set with voltage in millivolts. */
> + cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
> + selector), 1000);
> +
> + ret = rpmh_regulator_send_request(vreg, &cmd, 1,
> + selector > vreg->voltage_selector);
If you init vreg->voltage_selector to an error as I suggest in
rpmh_regulator_load_default_parameters() you'll need to handle it
here. See below.
> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
> + unsigned int mode, bool bypassed)
> +{
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
> + };
Please add:
if (mode & ~(REGULATOR_MODE_STANDBY |
REGULATOR_MODE_IDLE |
REGULATOR_MODE_NORMAL |
REGULATOR_MODE_FAST))
return -EINVAL;
That way if someone adds a new mode you don't index off the end of
your array. Ah, I see, you have this in rpmh_regulator_vrm_set_mode
by checking if mode > REGULATOR_MODE_STANDBY. That works. Can you
move it here so it's closer to where the array access is?
> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int i;
> +
> + for (i = 0; i < vreg->drms_mode_count - 1; i++)
> + if (load_uA < vreg->drms_mode_max_uA[i])
> + break;
Can you put a warning here if the requested load_uA is larger than the
largest supported, and/or perhaps consider it an error case?
> +
> + return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
Might not hurt to have a comment saying that this calls
rpmh_regulator_vrm_set_mode() instead of calling
rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
to change the mode returned by a future call to get_mode().
> +static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
> + bool enable)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int ret;
> +
> + if (vreg->bypassed == enable)
> + return 0;
Just like for enable, remove this check. The regulator core does it for you.
> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
> + bool *enable)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + *enable = vreg->bypassed;
> +
> + return 0;
Should you return an error code if nobody has ever called set_bypass?
...or is it OK to just return "not bypassed"? Please document this in
the code.
> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
> + struct device *dev, struct device_node *node)
> +{
> + const char *prop;
> + int i, len, ret, mode;
> + u32 *buf;
> +
> + /* qcom,allowed-drms-modes is optional */
> + prop = "qcom,allowed-drms-modes";
> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
> + if (len < 0)
> + return 0;
> +
> + vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
> + GFP_KERNEL);
> + vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
> + sizeof(*vreg->drms_mode_max_uA), GFP_KERNEL);
> + if (!vreg->drms_mode || !vreg->drms_mode_max_uA)
> + return -ENOMEM;
> + vreg->drms_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) {
> + dev_err(dev, "%s: unable to read %s, ret=%d\n",
> + node->name, prop, ret);
> + goto done;
> + }
> +
> + for (i = 0; i < len; i++) {
> + mode = vreg->hw_data->of_map_mode(buf[i]);
> + if (mode <= 0) {
Should be < 0, not <= 0 right? Unless we take Javier's suggestion
(see <https://patchwork.kernel.org/patch/10346081/>) and make 0 be
invalid...
> +/**
> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
> + * request for this regulator based on optional device tree
> + * properties
> + * vreg: Pointer to the individual rpmh-regulator resource
> + * dev: Pointer to the top level rpmh-regulator PMIC device
> + * node: Pointer to the individual rpmh-regulator resource
> + * device node
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg,
> + struct device *dev, struct device_node *node)
> +{
> + struct tcs_cmd cmd[2] = {};
> + const struct regulator_linear_range *range;
> + const char *prop;
> + int cmd_count = 0;
> + int ret, selector;
> + u32 uV;
> +
> + if (vreg->hw_data->regulator_type == VRM) {
> + prop = "qcom,headroom-voltage";
> + ret = of_property_read_u32(node, prop, &uV);
> + if (!ret) {
> + if (uV > RPMH_VRM_HEADROOM_MAX_UV) {
> + dev_err(dev, "%s: %s=%u is invalid\n",
> + node->name, prop, uV);
> + return -EINVAL;
> + }
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
> + cmd[cmd_count++].data = DIV_ROUND_UP(uV, 1000);
> + }
> +
> + prop = "qcom,regulator-initial-voltage";
> + ret = of_property_read_u32(node, prop, &uV);
> + if (!ret) {
> + range = &vreg->hw_data->voltage_range;
> + selector = DIV_ROUND_UP(uV - range->min_uV,
> + range->uV_step) + range->min_sel;
> + if (uV < range->min_uV || selector > range->max_sel) {
> + dev_err(dev, "%s: %s=%u is invalid\n",
> + node->name, prop, uV);
> + return -EINVAL;
> + }
> +
> + vreg->voltage_selector = selector;
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
> + cmd[cmd_count++].data
> + = DIV_ROUND_UP(selector * range->uV_step
> + + range->min_uV, 1000);
> + }
Seems like you want an "else { vreg->voltage_selector = -EINVAL; }".
Otherwise "get_voltage_sel" will return selector 0 before the first
set, right?
Previously Mark said: "If the driver can't read values it should
return an appropriate error code."
...and previously you said: "I'll try this out and see if the
regulator framework complains during regulator registration."
> +/**
> + * rpmh_regulator_init_vreg() - initialize all attributes of an rpmh-regulator
> + * vreg: Pointer to the individual rpmh-regulator resource
> + * dev: Pointer to the top level rpmh-regulator PMIC device
> + * node: Pointer to the individual rpmh-regulator resource
> + * device node
> + * pmic_id: String used to identify the top level rpmh-regulator
> + * PMIC device on the board
> + * rpmh_data: Pointer to a null-terminated array of rpmh-regulator
> + * resources defined for the top level PMIC device
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
> + struct device_node *node, const char *pmic_id,
> + const struct rpmh_vreg_init_data *rpmh_data)
> +{
> + struct regulator_config reg_config = {};
> + char rpmh_resource_name[20] = "";
> + struct regulator_dev *rdev;
> + enum rpmh_regulator_type type;
> + struct regulator_init_data *init_data;
> + unsigned int mode;
> + int i, ret;
> +
> + for (; rpmh_data->name; rpmh_data++)
> + if (!strcmp(rpmh_data->name, node->name))
> + break;
> +
> + if (!rpmh_data->name) {
> + dev_err(dev, "Unknown regulator %s\n", node->name);
> + return -EINVAL;
> + }
> +
> + scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
> + rpmh_data->resource_name, pmic_id);
If the resulting string is exactly 20 characters then
rpmh_resource_name won't be zero terminated. Please handle this
properly.
> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
I may have suggested using this as an array that could be used as a
"map" (index by regulator framework mode and get the PMIC mode), but
now that I see it it doesn't seem super appealing since the regulator
framework mode is not 0, 1, 2, 3 but is actually 1, 2, 4, 8. ...but I
guess it's not too bad--you're allocating 9 ints to map 4 elements and
I guess that's about as efficient as you're going to get even if it
feels a bit ugly.
...but still a few improvements:
* Don't specify the size of the array as "REGULATOR_MODE_STANDBY + 1".
Let the compiler handle this. It should do the right thing. Then if
someone ever changes the order of the #defines things will just work,
I think.
* Make sure that users of these arrays check that the mode is one of
the mode you know about. That way if someone does add a new mode you
don't index off your array. I'll put a comment in the user.
Also: the type of this array is 'u32' but you're assigning -EINVAL in
some cases. Please fix to be signed. Here and on other similar
arrays.
> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
> +{
> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
> + [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,
> + };
You're sticking a negative value in an array of unsigned inits. Here
and in other similar functions.
I know, I know. The function is defined to return an unsigned int.
It's wrong. of_regulator.c clearly puts the return code in a signed
int. First attempt at fixing this is at
<https://patchwork.kernel.org/patch/10346081/>.
> +static const struct rpmh_vreg_hw_data pmic4_bob = {
> + .regulator_type = VRM,
> + .ops = &rpmh_regulator_vrm_bypass_ops,
> + .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
> + .n_voltages = 84,
> + .pmic_mode_map = pmic_mode_map_pmic4_bob,
> + .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
> + .bypass_mode = 0,
Remove .bypass_mode from the structure and just change
rpmh_regulator_vrm_set_mode_bypass() to set 0 if we're in bypass.
Right now 100% of PMICs that support bypass use 0 as the bypass mode.
If you ever have a future PMIC that uses a non-zero mode for bypass
then we can always add this back. ...and if no future PMICs ever use
a non-zero bypass mode then we don't need the complexity of having
another field in this struct.
If you don't do this you might get arguments from some people saying
that they don't like seeing inits of "= 0" in static structures (Linux
conventions seem to like you to just assume that structs are
zero-initted).
> +static int rpmh_regulator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct rpmh_vreg_init_data *vreg_data;
> + struct rpmh_client *rpmh_client;
> + struct device_node *node;
> + struct rpmh_vreg *vreg;
> + const char *pmic_id;
> + int ret;
> +
> + ret = cmd_db_ready();
> + if (ret < 0) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
> + return ret;
> + }
In the last patch Stephen said:
> We should just make rpmh parent device call cmd_db_ready() so that these
> devices aren't even populated until then and so that cmd_db_ready() is
> only in one place. Lina?
Any news here?
> +
> + vreg_data = of_device_get_match_data(dev);
> + if (!vreg_data)
> + return -ENODEV;
> +
> + ret = of_property_read_string(dev->of_node, "qcom,pmic-id", &pmic_id);
> + if (ret < 0) {
> + dev_err(dev, "qcom,pmic-id missing in DT node\n");
> + return ret;
> + }
> +
> + rpmh_client = rpmh_get_client(pdev);
> + if (IS_ERR(rpmh_client))
> + return PTR_ERR(rpmh_client);
> + platform_set_drvdata(pdev, rpmh_client);
> +
> + for_each_available_child_of_node(dev->of_node, node) {
> + vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
> + if (!vreg) {
> + ret = -ENOMEM;
> + of_node_put(node);
> + goto cleanup;
> + }
> +
> + vreg->rpmh_client = rpmh_client;
> +
> + ret = rpmh_regulator_init_vreg(vreg, dev, node, pmic_id,
> + vreg_data);
> + if (ret < 0) {
> + of_node_put(node);
> + goto cleanup;
> + }
> + }
> +
> + return 0;
> +
> +cleanup:
> + rpmh_release(rpmh_client);
Still no devm_rpmh_get_client()? If Lina is too busy spinning her
patch series for other stuff, just add it to RPMH as a patch in your
series. I believe it's just this (untested):
---
int devm_rpmh_release(struct device *dev, void *res)
{
struct platform_device *pdev = to_platform_device(dev);
rpmh_release(pdev);
}
int devm_rpmh_get_client(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
void *dr;
int rc;
dr = devres_alloc(devm_rpmh_release, 0, GFP_KERNEL);
if (!dr)
return -ENOMEM;
rc = rpmh_get_client(pdev);
if (!rc)
devres_add(dev, dr);
else
devres_free(dr);
return rc;
}
---
Note that you'll get rid of the error handling in probe, the whole
remove function, and the need to do platform_set_drvdata().
-Doug
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
2018-04-17 20:02 ` Doug Anderson
@ 2018-04-18 23:30 ` David Collins
-1 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-18 23:30 UTC (permalink / raw)
To: Doug Anderson
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd, Matthias Kaehlcke
On 04/17/2018 01:02 PM, Doug Anderson wrote:
> On Fri, Apr 13, 2018 at 7:50 PM, David Collins <collinsd@codeaurora.org> wrote:
>> +#define RPMH_REGULATOR_DISABLE 0x0
>> +#define RPMH_REGULATOR_ENABLE 0x1
>
> In the last version Stephen said he didn't like the above two #defines
> and you said you'd remove them, yet they are still here. Explanation?
I thought that he was referring to the comments above the constants since
his review comment was immediately following the second of two comments as
opposed to these constants. I'll let him follow-up on this point if
necessary.
>> + * @drms_mode: Array of regulator framework modes which can
>> + * be configured dynamically for this regulator
>> + * via the set_load() callback.
>
> Using the singular for something that is an array is confusing. Why
> not "drms_modes" or "drms_mode_arr"? In the past review you said
> 'Perhaps something along the lines of "drms_modes"'.
It seems awkward to me to use a plural for arrays as it leads to indexing
like this: vreg->drms_modes[i]. "mode i" seems better than "modes i".
However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
if that style is preferred.
>> +struct rpmh_vreg {
>> + struct rpmh_client *rpmh_client;
>> + u32 addr;
>> + struct regulator_desc rdesc;
>> + const struct rpmh_vreg_hw_data *hw_data;
>> + enum rpmh_regulator_type regulator_type;
>> + bool always_wait_for_ack;
>> + unsigned int *drms_mode;
>> + int *drms_mode_max_uA;
>> + size_t drms_mode_count;
>> +
>> + bool enabled;
>> + int voltage_selector;
>> + unsigned int mode;
>> + bool bypassed;
>
> nit: stick next to "enabled" to make slightly better structure packing...
Ok
>> +static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
>> + unsigned int selector)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + struct tcs_cmd cmd = {
>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
>> + };
>> + int ret;
>> +
>> + /* VRM voltage control register is set with voltage in millivolts. */
>> + cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
>> + selector), 1000);
>> +
>> + ret = rpmh_regulator_send_request(vreg, &cmd, 1,
>> + selector > vreg->voltage_selector);
>
> If you init vreg->voltage_selector to an error as I suggest in
> rpmh_regulator_load_default_parameters() you'll need to handle it
> here. See below.
Ok
>> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>> + unsigned int mode, bool bypassed)
>> +{
>> + struct tcs_cmd cmd = {
>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>> + };
>
> Please add:
>
> if (mode & ~(REGULATOR_MODE_STANDBY |
> REGULATOR_MODE_IDLE |
> REGULATOR_MODE_NORMAL |
> REGULATOR_MODE_FAST))
> return -EINVAL;
>
> That way if someone adds a new mode you don't index off the end of
> your array. Ah, I see, you have this in rpmh_regulator_vrm_set_mode
> by checking if mode > REGULATOR_MODE_STANDBY. That works. Can you
> move it here so it's closer to where the array access is?
Theoretically, the (mode > REGULATOR_MODE_STANDBY) check in
rpmh_regulator_vrm_set_mode() shouldn't be necessary at all. I felt safer
leaving it in though. The framework ensures that no mode values may be
passed into rpmh_regulator_vrm_set_mode() which is not identified in
constraints.valid_modes_mask. Similar sanitization happens for internal
rpmh_regulator_vrm_set_mode() calls.
I'll move the (mode > REGULATOR_MODE_STANDBY) check into
rpmh_regulator_vrm_set_mode_bypass().
>> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + int i;
>> +
>> + for (i = 0; i < vreg->drms_mode_count - 1; i++)
>> + if (load_uA < vreg->drms_mode_max_uA[i])
>> + break;
>
> Can you put a warning here if the requested load_uA is larger than the
> largest supported, and/or perhaps consider it an error case?
I'll add a warning.
>> +
>> + return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
>
> Might not hurt to have a comment saying that this calls
> rpmh_regulator_vrm_set_mode() instead of calling
> rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
> to change the mode returned by a future call to get_mode().
This seems pretty clear on inspection of the very closely spaced
functions. I don't see the need for a comment about it.
>> +static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
>> + bool enable)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + int ret;
>> +
>> + if (vreg->bypassed == enable)
>> + return 0;
>
> Just like for enable, remove this check. The regulator core does it for you.
Ok
>> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
>> + bool *enable)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> +
>> + *enable = vreg->bypassed;
>> +
>> + return 0;
>
> Should you return an error code if nobody has ever called set_bypass?
> ...or is it OK to just return "not bypassed"? Please document this in
> the code.
I think it is fine to return "not bypassed" by default if there is no
configuration in place to enable bypassing. How are you suggesting that
this be documented in the code?
>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
>> + struct device *dev, struct device_node *node)
>> +{
>> + const char *prop;
>> + int i, len, ret, mode;
>> + u32 *buf;
>> +
>> + /* qcom,allowed-drms-modes is optional */
>> + prop = "qcom,allowed-drms-modes";
>> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
>> + if (len < 0)
>> + return 0;
>> +
>> + vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
>> + GFP_KERNEL);
>> + vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
>> + sizeof(*vreg->drms_mode_max_uA), GFP_KERNEL);
>> + if (!vreg->drms_mode || !vreg->drms_mode_max_uA)
>> + return -ENOMEM;
>> + vreg->drms_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) {
>> + dev_err(dev, "%s: unable to read %s, ret=%d\n",
>> + node->name, prop, ret);
>> + goto done;
>> + }
>> +
>> + for (i = 0; i < len; i++) {
>> + mode = vreg->hw_data->of_map_mode(buf[i]);
>> + if (mode <= 0) {
>
> Should be < 0, not <= 0 right? Unless we take Javier's suggestion
> (see <https://patchwork.kernel.org/patch/10346081/>) and make 0 be
> invalid...
It looks like the way forward is REGULATOR_MODE_INVALID == 0 so '<= 0' is
fine here. I suppose that it could be changed to '==
REGULATOR_MODE_INVALID' as well.
>> + prop = "qcom,regulator-initial-voltage";
>> + ret = of_property_read_u32(node, prop, &uV);
>> + if (!ret) {
>> + range = &vreg->hw_data->voltage_range;
>> + selector = DIV_ROUND_UP(uV - range->min_uV,
>> + range->uV_step) + range->min_sel;
>> + if (uV < range->min_uV || selector > range->max_sel) {
>> + dev_err(dev, "%s: %s=%u is invalid\n",
>> + node->name, prop, uV);
>> + return -EINVAL;
>> + }
>> +
>> + vreg->voltage_selector = selector;
>> +
>> + cmd[cmd_count].addr
>> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
>> + cmd[cmd_count++].data
>> + = DIV_ROUND_UP(selector * range->uV_step
>> + + range->min_uV, 1000);
>> + }
>
> Seems like you want an "else { vreg->voltage_selector = -EINVAL; }".
> Otherwise "get_voltage_sel" will return selector 0 before the first
> set, right?
>
> Previously Mark said: "If the driver can't read values it should
> return an appropriate error code."
> ...and previously you said: "I'll try this out and see if the
> regulator framework complains during regulator registration."
I tested out what happens when vreg->voltage_selector = -EINVAL is set
when qcom,regulator-initial-voltage is not present. This results in
devm_regulator_register() failing and subsequently causing the
qcom_rpmh-regulator probe to fail. The error happens in
machine_constraints_voltage() [1].
This leaves two courses of action:
1. (current patch set) allow voltage_selector to stay 0 if uninitialized
2. Set voltage_selector = -EINVAL by default and specify in DT binding
documentation that qcom,regulator-initial-voltage is required for VRM
managed RPMh regulator resources which have regulator-min-microvolt and
regulator-max-microvolt specified.
Are you ok with the DT implications of option #2?
>> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
>> + struct device_node *node, const char *pmic_id,
>> + const struct rpmh_vreg_init_data *rpmh_data)
>> +{
>> + struct regulator_config reg_config = {};
>> + char rpmh_resource_name[20] = "";
>> + struct regulator_dev *rdev;
>> + enum rpmh_regulator_type type;
>> + struct regulator_init_data *init_data;
>> + unsigned int mode;
>> + int i, ret;
>> +
>> + for (; rpmh_data->name; rpmh_data++)
>> + if (!strcmp(rpmh_data->name, node->name))
>> + break;
>> +
>> + if (!rpmh_data->name) {
>> + dev_err(dev, "Unknown regulator %s\n", node->name);
>> + return -EINVAL;
>> + }
>> +
>> + scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
>> + rpmh_data->resource_name, pmic_id);
>
> If the resulting string is exactly 20 characters then
> rpmh_resource_name won't be zero terminated. Please handle this
> properly.
The output of scnprintf() is always null-terminated; therefore, no other
check is needed. Also, RPMh resource names stored in SMEM command DB data
structure are at most 8 bytes (<= 7 char + '\0' or 8 char and no '\0') so
using 20 chars in the buffer is overkill anyway.
>> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>
> I may have suggested using this as an array that could be used as a
> "map" (index by regulator framework mode and get the PMIC mode), but
> now that I see it it doesn't seem super appealing since the regulator
> framework mode is not 0, 1, 2, 3 but is actually 1, 2, 4, 8. ...but I
> guess it's not too bad--you're allocating 9 ints to map 4 elements and
> I guess that's about as efficient as you're going to get even if it
> feels a bit ugly.
I'm ok with the sparse mapping as it makes indexing as simple as possible
and the extra space used is insignificant.
> ...but still a few improvements:
>
> * Don't specify the size of the array as "REGULATOR_MODE_STANDBY + 1".
> Let the compiler handle this. It should do the right thing. Then if
> someone ever changes the order of the #defines things will just work,
> I think.
>
> * Make sure that users of these arrays check that the mode is one of
> the mode you know about. That way if someone does add a new mode you
> don't index off your array. I'll put a comment in the user.
Even if a new mode was added, the relative ordering of the existing modes
should not change and valid_modes_mask will only allow modes currently
supported by the driver. I'd like to keep the bound checks as simple as
possible. By explicitly sizing the arrays and then only checking for mode
> REGULATOR_MODE_STANDBY when indexing into the array we can be sure that
no out-of-bound access can ever occur. Also, if one of the existing mode
value was made larger than REGULATOR_MODE_STANDBY it would be easy to
catch as it would cause a compilation error.
Thus, I'd prefer to keep the array sizing and index checking as-in unless
there is a major objection.
> Also: the type of this array is 'u32' but you're assigning -EINVAL in
> some cases. Please fix to be signed. Here and on other similar
> arrays.
Ok.
>> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
>> +{
>> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
>> + [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,
>> + };
>
> You're sticking a negative value in an array of unsigned inits. Here
> and in other similar functions.
>
> I know, I know. The function is defined to return an unsigned int.
> It's wrong. of_regulator.c clearly puts the return code in a signed
> int. First attempt at fixing this is at
> <https://patchwork.kernel.org/patch/10346081/>.
I can change the error cases to use REGULATOR_MODE_INVALID which is added
by this change still under review [2].
>> +static const struct rpmh_vreg_hw_data pmic4_bob = {
>> + .regulator_type = VRM,
>> + .ops = &rpmh_regulator_vrm_bypass_ops,
>> + .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
>> + .n_voltages = 84,
>> + .pmic_mode_map = pmic_mode_map_pmic4_bob,
>> + .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
>> + .bypass_mode = 0,
>
> Remove .bypass_mode from the structure and just change
> rpmh_regulator_vrm_set_mode_bypass() to set 0 if we're in bypass.
> Right now 100% of PMICs that support bypass use 0 as the bypass mode.
> If you ever have a future PMIC that uses a non-zero mode for bypass
> then we can always add this back. ...and if no future PMICs ever use
> a non-zero bypass mode then we don't need the complexity of having
> another field in this struct.
>
> If you don't do this you might get arguments from some people saying
> that they don't like seeing inits of "= 0" in static structures (Linux
> conventions seem to like you to just assume that structs are
> zero-initted).
Upcoming PMICs use 2 for bypass mode instead of 0. That is why I left
this in. I suppose that I can remove this member for now and add it back
in when newer PMIC support is added.
>> +static int rpmh_regulator_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + const struct rpmh_vreg_init_data *vreg_data;
>> + struct rpmh_client *rpmh_client;
>> + struct device_node *node;
>> + struct rpmh_vreg *vreg;
>> + const char *pmic_id;
>> + int ret;
>> +
>> + ret = cmd_db_ready();
>> + if (ret < 0) {
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
>> + return ret;
>> + }
>
> In the last patch Stephen said:
>
>> We should just make rpmh parent device call cmd_db_ready() so that these
>> devices aren't even populated until then and so that cmd_db_ready() is
>> only in one place. Lina?
>
> Any news here?
I don't think that Lina has tried to include this in her rpmh series yet.
>> +cleanup:
>> + rpmh_release(rpmh_client);
>
> Still no devm_rpmh_get_client()? If Lina is too busy spinning her
> patch series for other stuff, just add it to RPMH as a patch in your
> series. I believe it's just this (untested):
>
> ---
>
> int devm_rpmh_release(struct device *dev, void *res)
> {
> struct platform_device *pdev = to_platform_device(dev);
> rpmh_release(pdev);
> }
>
> int devm_rpmh_get_client(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> void *dr;
> int rc;
>
> dr = devres_alloc(devm_rpmh_release, 0, GFP_KERNEL);
> if (!dr)
> return -ENOMEM;
>
> rc = rpmh_get_client(pdev);
> if (!rc)
> devres_add(dev, dr);
> else
> devres_free(dr);
>
> return rc;
> }
>
> ---
>
> Note that you'll get rid of the error handling in probe, the whole
> remove function, and the need to do platform_set_drvdata().
My understanding is that Lina is going to remove both rpmh_get_client()
and rpmh_release(). In their place, rpmh functions will use the consumer
device pointer as a handle and manage any necessary state internally [3].
I'll update this patch once she uploads a new series with this interface
modification.
Thanks,
David
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/core.c?h=v4.17-rc1#n888
[2]: https://patchwork.kernel.org/patch/10348631/
[3]: https://lkml.org/lkml/2018/4/10/1273
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-18 23:30 ` David Collins
0 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-18 23:30 UTC (permalink / raw)
To: linux-arm-kernel
On 04/17/2018 01:02 PM, Doug Anderson wrote:
> On Fri, Apr 13, 2018 at 7:50 PM, David Collins <collinsd@codeaurora.org> wrote:
>> +#define RPMH_REGULATOR_DISABLE 0x0
>> +#define RPMH_REGULATOR_ENABLE 0x1
>
> In the last version Stephen said he didn't like the above two #defines
> and you said you'd remove them, yet they are still here. Explanation?
I thought that he was referring to the comments above the constants since
his review comment was immediately following the second of two comments as
opposed to these constants. I'll let him follow-up on this point if
necessary.
>> + * @drms_mode: Array of regulator framework modes which can
>> + * be configured dynamically for this regulator
>> + * via the set_load() callback.
>
> Using the singular for something that is an array is confusing. Why
> not "drms_modes" or "drms_mode_arr"? In the past review you said
> 'Perhaps something along the lines of "drms_modes"'.
It seems awkward to me to use a plural for arrays as it leads to indexing
like this: vreg->drms_modes[i]. "mode i" seems better than "modes i".
However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
if that style is preferred.
>> +struct rpmh_vreg {
>> + struct rpmh_client *rpmh_client;
>> + u32 addr;
>> + struct regulator_desc rdesc;
>> + const struct rpmh_vreg_hw_data *hw_data;
>> + enum rpmh_regulator_type regulator_type;
>> + bool always_wait_for_ack;
>> + unsigned int *drms_mode;
>> + int *drms_mode_max_uA;
>> + size_t drms_mode_count;
>> +
>> + bool enabled;
>> + int voltage_selector;
>> + unsigned int mode;
>> + bool bypassed;
>
> nit: stick next to "enabled" to make slightly better structure packing...
Ok
>> +static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
>> + unsigned int selector)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + struct tcs_cmd cmd = {
>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
>> + };
>> + int ret;
>> +
>> + /* VRM voltage control register is set with voltage in millivolts. */
>> + cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
>> + selector), 1000);
>> +
>> + ret = rpmh_regulator_send_request(vreg, &cmd, 1,
>> + selector > vreg->voltage_selector);
>
> If you init vreg->voltage_selector to an error as I suggest in
> rpmh_regulator_load_default_parameters() you'll need to handle it
> here. See below.
Ok
>> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>> + unsigned int mode, bool bypassed)
>> +{
>> + struct tcs_cmd cmd = {
>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>> + };
>
> Please add:
>
> if (mode & ~(REGULATOR_MODE_STANDBY |
> REGULATOR_MODE_IDLE |
> REGULATOR_MODE_NORMAL |
> REGULATOR_MODE_FAST))
> return -EINVAL;
>
> That way if someone adds a new mode you don't index off the end of
> your array. Ah, I see, you have this in rpmh_regulator_vrm_set_mode
> by checking if mode > REGULATOR_MODE_STANDBY. That works. Can you
> move it here so it's closer to where the array access is?
Theoretically, the (mode > REGULATOR_MODE_STANDBY) check in
rpmh_regulator_vrm_set_mode() shouldn't be necessary at all. I felt safer
leaving it in though. The framework ensures that no mode values may be
passed into rpmh_regulator_vrm_set_mode() which is not identified in
constraints.valid_modes_mask. Similar sanitization happens for internal
rpmh_regulator_vrm_set_mode() calls.
I'll move the (mode > REGULATOR_MODE_STANDBY) check into
rpmh_regulator_vrm_set_mode_bypass().
>> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + int i;
>> +
>> + for (i = 0; i < vreg->drms_mode_count - 1; i++)
>> + if (load_uA < vreg->drms_mode_max_uA[i])
>> + break;
>
> Can you put a warning here if the requested load_uA is larger than the
> largest supported, and/or perhaps consider it an error case?
I'll add a warning.
>> +
>> + return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
>
> Might not hurt to have a comment saying that this calls
> rpmh_regulator_vrm_set_mode() instead of calling
> rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
> to change the mode returned by a future call to get_mode().
This seems pretty clear on inspection of the very closely spaced
functions. I don't see the need for a comment about it.
>> +static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
>> + bool enable)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + int ret;
>> +
>> + if (vreg->bypassed == enable)
>> + return 0;
>
> Just like for enable, remove this check. The regulator core does it for you.
Ok
>> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
>> + bool *enable)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> +
>> + *enable = vreg->bypassed;
>> +
>> + return 0;
>
> Should you return an error code if nobody has ever called set_bypass?
> ...or is it OK to just return "not bypassed"? Please document this in
> the code.
I think it is fine to return "not bypassed" by default if there is no
configuration in place to enable bypassing. How are you suggesting that
this be documented in the code?
>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
>> + struct device *dev, struct device_node *node)
>> +{
>> + const char *prop;
>> + int i, len, ret, mode;
>> + u32 *buf;
>> +
>> + /* qcom,allowed-drms-modes is optional */
>> + prop = "qcom,allowed-drms-modes";
>> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
>> + if (len < 0)
>> + return 0;
>> +
>> + vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
>> + GFP_KERNEL);
>> + vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
>> + sizeof(*vreg->drms_mode_max_uA), GFP_KERNEL);
>> + if (!vreg->drms_mode || !vreg->drms_mode_max_uA)
>> + return -ENOMEM;
>> + vreg->drms_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) {
>> + dev_err(dev, "%s: unable to read %s, ret=%d\n",
>> + node->name, prop, ret);
>> + goto done;
>> + }
>> +
>> + for (i = 0; i < len; i++) {
>> + mode = vreg->hw_data->of_map_mode(buf[i]);
>> + if (mode <= 0) {
>
> Should be < 0, not <= 0 right? Unless we take Javier's suggestion
> (see <https://patchwork.kernel.org/patch/10346081/>) and make 0 be
> invalid...
It looks like the way forward is REGULATOR_MODE_INVALID == 0 so '<= 0' is
fine here. I suppose that it could be changed to '==
REGULATOR_MODE_INVALID' as well.
>> + prop = "qcom,regulator-initial-voltage";
>> + ret = of_property_read_u32(node, prop, &uV);
>> + if (!ret) {
>> + range = &vreg->hw_data->voltage_range;
>> + selector = DIV_ROUND_UP(uV - range->min_uV,
>> + range->uV_step) + range->min_sel;
>> + if (uV < range->min_uV || selector > range->max_sel) {
>> + dev_err(dev, "%s: %s=%u is invalid\n",
>> + node->name, prop, uV);
>> + return -EINVAL;
>> + }
>> +
>> + vreg->voltage_selector = selector;
>> +
>> + cmd[cmd_count].addr
>> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
>> + cmd[cmd_count++].data
>> + = DIV_ROUND_UP(selector * range->uV_step
>> + + range->min_uV, 1000);
>> + }
>
> Seems like you want an "else { vreg->voltage_selector = -EINVAL; }".
> Otherwise "get_voltage_sel" will return selector 0 before the first
> set, right?
>
> Previously Mark said: "If the driver can't read values it should
> return an appropriate error code."
> ...and previously you said: "I'll try this out and see if the
> regulator framework complains during regulator registration."
I tested out what happens when vreg->voltage_selector = -EINVAL is set
when qcom,regulator-initial-voltage is not present. This results in
devm_regulator_register() failing and subsequently causing the
qcom_rpmh-regulator probe to fail. The error happens in
machine_constraints_voltage() [1].
This leaves two courses of action:
1. (current patch set) allow voltage_selector to stay 0 if uninitialized
2. Set voltage_selector = -EINVAL by default and specify in DT binding
documentation that qcom,regulator-initial-voltage is required for VRM
managed RPMh regulator resources which have regulator-min-microvolt and
regulator-max-microvolt specified.
Are you ok with the DT implications of option #2?
>> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
>> + struct device_node *node, const char *pmic_id,
>> + const struct rpmh_vreg_init_data *rpmh_data)
>> +{
>> + struct regulator_config reg_config = {};
>> + char rpmh_resource_name[20] = "";
>> + struct regulator_dev *rdev;
>> + enum rpmh_regulator_type type;
>> + struct regulator_init_data *init_data;
>> + unsigned int mode;
>> + int i, ret;
>> +
>> + for (; rpmh_data->name; rpmh_data++)
>> + if (!strcmp(rpmh_data->name, node->name))
>> + break;
>> +
>> + if (!rpmh_data->name) {
>> + dev_err(dev, "Unknown regulator %s\n", node->name);
>> + return -EINVAL;
>> + }
>> +
>> + scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
>> + rpmh_data->resource_name, pmic_id);
>
> If the resulting string is exactly 20 characters then
> rpmh_resource_name won't be zero terminated. Please handle this
> properly.
The output of scnprintf() is always null-terminated; therefore, no other
check is needed. Also, RPMh resource names stored in SMEM command DB data
structure are at most 8 bytes (<= 7 char + '\0' or 8 char and no '\0') so
using 20 chars in the buffer is overkill anyway.
>> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>
> I may have suggested using this as an array that could be used as a
> "map" (index by regulator framework mode and get the PMIC mode), but
> now that I see it it doesn't seem super appealing since the regulator
> framework mode is not 0, 1, 2, 3 but is actually 1, 2, 4, 8. ...but I
> guess it's not too bad--you're allocating 9 ints to map 4 elements and
> I guess that's about as efficient as you're going to get even if it
> feels a bit ugly.
I'm ok with the sparse mapping as it makes indexing as simple as possible
and the extra space used is insignificant.
> ...but still a few improvements:
>
> * Don't specify the size of the array as "REGULATOR_MODE_STANDBY + 1".
> Let the compiler handle this. It should do the right thing. Then if
> someone ever changes the order of the #defines things will just work,
> I think.
>
> * Make sure that users of these arrays check that the mode is one of
> the mode you know about. That way if someone does add a new mode you
> don't index off your array. I'll put a comment in the user.
Even if a new mode was added, the relative ordering of the existing modes
should not change and valid_modes_mask will only allow modes currently
supported by the driver. I'd like to keep the bound checks as simple as
possible. By explicitly sizing the arrays and then only checking for mode
> REGULATOR_MODE_STANDBY when indexing into the array we can be sure that
no out-of-bound access can ever occur. Also, if one of the existing mode
value was made larger than REGULATOR_MODE_STANDBY it would be easy to
catch as it would cause a compilation error.
Thus, I'd prefer to keep the array sizing and index checking as-in unless
there is a major objection.
> Also: the type of this array is 'u32' but you're assigning -EINVAL in
> some cases. Please fix to be signed. Here and on other similar
> arrays.
Ok.
>> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
>> +{
>> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
>> + [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,
>> + };
>
> You're sticking a negative value in an array of unsigned inits. Here
> and in other similar functions.
>
> I know, I know. The function is defined to return an unsigned int.
> It's wrong. of_regulator.c clearly puts the return code in a signed
> int. First attempt at fixing this is at
> <https://patchwork.kernel.org/patch/10346081/>.
I can change the error cases to use REGULATOR_MODE_INVALID which is added
by this change still under review [2].
>> +static const struct rpmh_vreg_hw_data pmic4_bob = {
>> + .regulator_type = VRM,
>> + .ops = &rpmh_regulator_vrm_bypass_ops,
>> + .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
>> + .n_voltages = 84,
>> + .pmic_mode_map = pmic_mode_map_pmic4_bob,
>> + .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
>> + .bypass_mode = 0,
>
> Remove .bypass_mode from the structure and just change
> rpmh_regulator_vrm_set_mode_bypass() to set 0 if we're in bypass.
> Right now 100% of PMICs that support bypass use 0 as the bypass mode.
> If you ever have a future PMIC that uses a non-zero mode for bypass
> then we can always add this back. ...and if no future PMICs ever use
> a non-zero bypass mode then we don't need the complexity of having
> another field in this struct.
>
> If you don't do this you might get arguments from some people saying
> that they don't like seeing inits of "= 0" in static structures (Linux
> conventions seem to like you to just assume that structs are
> zero-initted).
Upcoming PMICs use 2 for bypass mode instead of 0. That is why I left
this in. I suppose that I can remove this member for now and add it back
in when newer PMIC support is added.
>> +static int rpmh_regulator_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + const struct rpmh_vreg_init_data *vreg_data;
>> + struct rpmh_client *rpmh_client;
>> + struct device_node *node;
>> + struct rpmh_vreg *vreg;
>> + const char *pmic_id;
>> + int ret;
>> +
>> + ret = cmd_db_ready();
>> + if (ret < 0) {
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
>> + return ret;
>> + }
>
> In the last patch Stephen said:
>
>> We should just make rpmh parent device call cmd_db_ready() so that these
>> devices aren't even populated until then and so that cmd_db_ready() is
>> only in one place. Lina?
>
> Any news here?
I don't think that Lina has tried to include this in her rpmh series yet.
>> +cleanup:
>> + rpmh_release(rpmh_client);
>
> Still no devm_rpmh_get_client()? If Lina is too busy spinning her
> patch series for other stuff, just add it to RPMH as a patch in your
> series. I believe it's just this (untested):
>
> ---
>
> int devm_rpmh_release(struct device *dev, void *res)
> {
> struct platform_device *pdev = to_platform_device(dev);
> rpmh_release(pdev);
> }
>
> int devm_rpmh_get_client(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> void *dr;
> int rc;
>
> dr = devres_alloc(devm_rpmh_release, 0, GFP_KERNEL);
> if (!dr)
> return -ENOMEM;
>
> rc = rpmh_get_client(pdev);
> if (!rc)
> devres_add(dev, dr);
> else
> devres_free(dr);
>
> return rc;
> }
>
> ---
>
> Note that you'll get rid of the error handling in probe, the whole
> remove function, and the need to do platform_set_drvdata().
My understanding is that Lina is going to remove both rpmh_get_client()
and rpmh_release(). In their place, rpmh functions will use the consumer
device pointer as a handle and manage any necessary state internally [3].
I'll update this patch once she uploads a new series with this interface
modification.
Thanks,
David
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/core.c?h=v4.17-rc1#n888
[2]: https://patchwork.kernel.org/patch/10348631/
[3]: https://lkml.org/lkml/2018/4/10/1273
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
2018-04-18 23:30 ` David Collins
@ 2018-04-19 6:04 ` Stephen Boyd
-1 siblings, 0 replies; 51+ messages in thread
From: Stephen Boyd @ 2018-04-19 6:04 UTC (permalink / raw)
To: David Collins, Doug Anderson
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Matthias Kaehlcke
Quoting David Collins (2018-04-18 16:30:26)
> On 04/17/2018 01:02 PM, Doug Anderson wrote:
> > On Fri, Apr 13, 2018 at 7:50 PM, David Collins <collinsd@codeaurora.org> wrote:
> >> +#define RPMH_REGULATOR_DISABLE 0x0
> >> +#define RPMH_REGULATOR_ENABLE 0x1
> >
> > In the last version Stephen said he didn't like the above two #defines
> > and you said you'd remove them, yet they are still here. Explanation?
>
> I thought that he was referring to the comments above the constants since
> his review comment was immediately following the second of two comments as
> opposed to these constants. I'll let him follow-up on this point if
> necessary.
>
I think I was talking about the comments. The defines don't look super
useful either though.
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-19 6:04 ` Stephen Boyd
0 siblings, 0 replies; 51+ messages in thread
From: Stephen Boyd @ 2018-04-19 6:04 UTC (permalink / raw)
To: linux-arm-kernel
Quoting David Collins (2018-04-18 16:30:26)
> On 04/17/2018 01:02 PM, Doug Anderson wrote:
> > On Fri, Apr 13, 2018 at 7:50 PM, David Collins <collinsd@codeaurora.org> wrote:
> >> +#define RPMH_REGULATOR_DISABLE 0x0
> >> +#define RPMH_REGULATOR_ENABLE 0x1
> >
> > In the last version Stephen said he didn't like the above two #defines
> > and you said you'd remove them, yet they are still here. Explanation?
>
> I thought that he was referring to the comments above the constants since
> his review comment was immediately following the second of two comments as
> opposed to these constants. I'll let him follow-up on this point if
> necessary.
>
I think I was talking about the comments. The defines don't look super
useful either though.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
2018-04-18 23:30 ` David Collins
@ 2018-04-19 16:16 ` Doug Anderson
-1 siblings, 0 replies; 51+ messages in thread
From: Doug Anderson @ 2018-04-19 16:16 UTC (permalink / raw)
To: David Collins
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd, Matthias Kaehlcke
Hi,
On Wed, Apr 18, 2018 at 4:30 PM, David Collins <collinsd@codeaurora.org> wrote:
>>> + * @drms_mode: Array of regulator framework modes which can
>>> + * be configured dynamically for this regulator
>>> + * via the set_load() callback.
>>
>> Using the singular for something that is an array is confusing. Why
>> not "drms_modes" or "drms_mode_arr"? In the past review you said
>> 'Perhaps something along the lines of "drms_modes"'.
>
> It seems awkward to me to use a plural for arrays as it leads to indexing
> like this: vreg->drms_modes[i]. "mode i" seems better than "modes i".
> However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
> if that style is preferred.
I'd very much like a plural here.
>>> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>>> + unsigned int mode, bool bypassed)
>>> +{
>>> + struct tcs_cmd cmd = {
>>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>>> + };
>>
>> Please add:
>>
>> if (mode & ~(REGULATOR_MODE_STANDBY |
>> REGULATOR_MODE_IDLE |
>> REGULATOR_MODE_NORMAL |
>> REGULATOR_MODE_FAST))
>> return -EINVAL;
>>
>> That way if someone adds a new mode you don't index off the end of
>> your array. Ah, I see, you have this in rpmh_regulator_vrm_set_mode
>> by checking if mode > REGULATOR_MODE_STANDBY. That works. Can you
>> move it here so it's closer to where the array access is?
>
> Theoretically, the (mode > REGULATOR_MODE_STANDBY) check in
> rpmh_regulator_vrm_set_mode() shouldn't be necessary at all. I felt safer
> leaving it in though. The framework ensures that no mode values may be
> passed into rpmh_regulator_vrm_set_mode() which is not identified in
> constraints.valid_modes_mask. Similar sanitization happens for internal
> rpmh_regulator_vrm_set_mode() calls.
>
> I'll move the (mode > REGULATOR_MODE_STANDBY) check into
> rpmh_regulator_vrm_set_mode_bypass().
Ah, good point about the valid_modes_mask! I'm happy with moving the
test here. I wouldn't mind a comment saying that the check is
probably overkill because the framework already checks
valid_modes_mask but shouldn't hurt.
>>> +
>>> + return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
>>
>> Might not hurt to have a comment saying that this calls
>> rpmh_regulator_vrm_set_mode() instead of calling
>> rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
>> to change the mode returned by a future call to get_mode().
>
> This seems pretty clear on inspection of the very closely spaced
> functions. I don't see the need for a comment about it.
It wasn't clear to me--I thought it might have just been because you
didn't want to manually pass in the current bypass state. Then I
looked at the function and thought there might have been a bug because
it was saving the mode. Then I looked back at the regulator framework
and finally came to the conclusion that set_load() is supposed to
change the mode (AKA: you'd expect that calling get_mode() after
set_load() would show you the mode you ended up at).
I guess this is all perhaps obvious to any regulator framework
experts, but since I spent 5 minutes digging through all that it
seemed like it deserved a comment to save the next person the 5
minutes. ...but I won't insist.
>>> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
>>> + bool *enable)
>>> +{
>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>> +
>>> + *enable = vreg->bypassed;
>>> +
>>> + return 0;
>>
>> Should you return an error code if nobody has ever called set_bypass?
>> ...or is it OK to just return "not bypassed"? Please document this in
>> the code.
>
> I think it is fine to return "not bypassed" by default if there is no
> configuration in place to enable bypassing. How are you suggesting that
> this be documented in the code?
I guess I wish the function had comments and then it could be
documented there. ...but none of the functions in this file do...
I guess you're right that it's clear enough without a comment, so
let's just leave it as is.
>>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
>>> + struct device *dev, struct device_node *node)
>>> +{
>>> + const char *prop;
>>> + int i, len, ret, mode;
>>> + u32 *buf;
>>> +
>>> + /* qcom,allowed-drms-modes is optional */
>>> + prop = "qcom,allowed-drms-modes";
>>> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
>>> + if (len < 0)
>>> + return 0;
>>> +
>>> + vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
>>> + GFP_KERNEL);
>>> + vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
>>> + sizeof(*vreg->drms_mode_max_uA), GFP_KERNEL);
>>> + if (!vreg->drms_mode || !vreg->drms_mode_max_uA)
>>> + return -ENOMEM;
>>> + vreg->drms_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) {
>>> + dev_err(dev, "%s: unable to read %s, ret=%d\n",
>>> + node->name, prop, ret);
>>> + goto done;
>>> + }
>>> +
>>> + for (i = 0; i < len; i++) {
>>> + mode = vreg->hw_data->of_map_mode(buf[i]);
>>> + if (mode <= 0) {
>>
>> Should be < 0, not <= 0 right? Unless we take Javier's suggestion
>> (see <https://patchwork.kernel.org/patch/10346081/>) and make 0 be
>> invalid...
>
> It looks like the way forward is REGULATOR_MODE_INVALID == 0 so '<= 0' is
> fine here. I suppose that it could be changed to '==
> REGULATOR_MODE_INVALID' as well.
Yes, assuming my patch lands using "==" is better. Checking whether
an unsigned value is <= 0 is confusing.
>>> + prop = "qcom,regulator-initial-voltage";
>>> + ret = of_property_read_u32(node, prop, &uV);
>>> + if (!ret) {
>>> + range = &vreg->hw_data->voltage_range;
>>> + selector = DIV_ROUND_UP(uV - range->min_uV,
>>> + range->uV_step) + range->min_sel;
>>> + if (uV < range->min_uV || selector > range->max_sel) {
>>> + dev_err(dev, "%s: %s=%u is invalid\n",
>>> + node->name, prop, uV);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + vreg->voltage_selector = selector;
>>> +
>>> + cmd[cmd_count].addr
>>> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
>>> + cmd[cmd_count++].data
>>> + = DIV_ROUND_UP(selector * range->uV_step
>>> + + range->min_uV, 1000);
>>> + }
>>
>> Seems like you want an "else { vreg->voltage_selector = -EINVAL; }".
>> Otherwise "get_voltage_sel" will return selector 0 before the first
>> set, right?
>>
>> Previously Mark said: "If the driver can't read values it should
>> return an appropriate error code."
>> ...and previously you said: "I'll try this out and see if the
>> regulator framework complains during regulator registration."
>
> I tested out what happens when vreg->voltage_selector = -EINVAL is set
> when qcom,regulator-initial-voltage is not present. This results in
> devm_regulator_register() failing and subsequently causing the
> qcom_rpmh-regulator probe to fail. The error happens in
> machine_constraints_voltage() [1].
>
> This leaves two courses of action:
> 1. (current patch set) allow voltage_selector to stay 0 if uninitialized
> 2. Set voltage_selector = -EINVAL by default and specify in DT binding
> documentation that qcom,regulator-initial-voltage is required for VRM
> managed RPMh regulator resources which have regulator-min-microvolt and
> regulator-max-microvolt specified.
>
> Are you ok with the DT implications of option #2?
You'd need to ask Mark if he's OK with it, but a option #3 is to add a
patch to your series fix the regulator framework to try setting the
voltage if _regulator_get_voltage() fails. Presumably in
machine_constraints_voltage() you'd now do something like:
int target_min, target_max;
int current_uV = _regulator_get_voltage(rdev);
if (current_uV < 0) {
/* Maybe this regulator's hardware can't be read and needs to be initted */
_regulator_do_set_voltage(
rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
current_uV = _regulator_get_voltage(rdev);
}
if (current_uV < 0) {
rdev_err(rdev,
"failed to get the current voltage(%d)\n",
current_uV);
return current_uV;
}
If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
but this needs to be documented _somewhere_ (unlike for bypass it's
not obvious, so you need to find someplace to put it). I'd rather not
hack the DT to deal with our software limitations.
>>> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
>>> + struct device_node *node, const char *pmic_id,
>>> + const struct rpmh_vreg_init_data *rpmh_data)
>>> +{
>>> + struct regulator_config reg_config = {};
>>> + char rpmh_resource_name[20] = "";
>>> + struct regulator_dev *rdev;
>>> + enum rpmh_regulator_type type;
>>> + struct regulator_init_data *init_data;
>>> + unsigned int mode;
>>> + int i, ret;
>>> +
>>> + for (; rpmh_data->name; rpmh_data++)
>>> + if (!strcmp(rpmh_data->name, node->name))
>>> + break;
>>> +
>>> + if (!rpmh_data->name) {
>>> + dev_err(dev, "Unknown regulator %s\n", node->name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
>>> + rpmh_data->resource_name, pmic_id);
>>
>> If the resulting string is exactly 20 characters then
>> rpmh_resource_name won't be zero terminated. Please handle this
>> properly.
>
> The output of scnprintf() is always null-terminated; therefore, no other
> check is needed. Also, RPMh resource names stored in SMEM command DB data
> structure are at most 8 bytes (<= 7 char + '\0' or 8 char and no '\0') so
> using 20 chars in the buffer is overkill anyway.
Wow, not sure where I looked to see that scnprintf() didn't always
null-terminate. Sorry. Ignore this.
>>> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>>
>> I may have suggested using this as an array that could be used as a
>> "map" (index by regulator framework mode and get the PMIC mode), but
>> now that I see it it doesn't seem super appealing since the regulator
>> framework mode is not 0, 1, 2, 3 but is actually 1, 2, 4, 8. ...but I
>> guess it's not too bad--you're allocating 9 ints to map 4 elements and
>> I guess that's about as efficient as you're going to get even if it
>> feels a bit ugly.
>
> I'm ok with the sparse mapping as it makes indexing as simple as possible
> and the extra space used is insignificant.
>
>
>> ...but still a few improvements:
>>
>> * Don't specify the size of the array as "REGULATOR_MODE_STANDBY + 1".
>> Let the compiler handle this. It should do the right thing. Then if
>> someone ever changes the order of the #defines things will just work,
>> I think.
>>
>> * Make sure that users of these arrays check that the mode is one of
>> the mode you know about. That way if someone does add a new mode you
>> don't index off your array. I'll put a comment in the user.
>
> Even if a new mode was added, the relative ordering of the existing modes
> should not change and valid_modes_mask will only allow modes currently
> supported by the driver. I'd like to keep the bound checks as simple as
> possible. By explicitly sizing the arrays and then only checking for mode
>> REGULATOR_MODE_STANDBY when indexing into the array we can be sure that
> no out-of-bound access can ever occur. Also, if one of the existing mode
> value was made larger than REGULATOR_MODE_STANDBY it would be easy to
> catch as it would cause a compilation error.
>
> Thus, I'd prefer to keep the array sizing and index checking as-in unless
> there is a major objection.
OK.
>>> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
>>> +{
>>> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
>>> + [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,
>>> + };
>>
>> You're sticking a negative value in an array of unsigned inits. Here
>> and in other similar functions.
>>
>> I know, I know. The function is defined to return an unsigned int.
>> It's wrong. of_regulator.c clearly puts the return code in a signed
>> int. First attempt at fixing this is at
>> <https://patchwork.kernel.org/patch/10346081/>.
>
> I can change the error cases to use REGULATOR_MODE_INVALID which is added
> by this change still under review [2].
I haven't seen Mark NAK it (yet), so for lack of a better option I'd
start using it in your patch and document in the commit message that
it depends on my patch.
>>> +static const struct rpmh_vreg_hw_data pmic4_bob = {
>>> + .regulator_type = VRM,
>>> + .ops = &rpmh_regulator_vrm_bypass_ops,
>>> + .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
>>> + .n_voltages = 84,
>>> + .pmic_mode_map = pmic_mode_map_pmic4_bob,
>>> + .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
>>> + .bypass_mode = 0,
>>
>> Remove .bypass_mode from the structure and just change
>> rpmh_regulator_vrm_set_mode_bypass() to set 0 if we're in bypass.
>> Right now 100% of PMICs that support bypass use 0 as the bypass mode.
>> If you ever have a future PMIC that uses a non-zero mode for bypass
>> then we can always add this back. ...and if no future PMICs ever use
>> a non-zero bypass mode then we don't need the complexity of having
>> another field in this struct.
>>
>> If you don't do this you might get arguments from some people saying
>> that they don't like seeing inits of "= 0" in static structures (Linux
>> conventions seem to like you to just assume that structs are
>> zero-initted).
>
> Upcoming PMICs use 2 for bypass mode instead of 0. That is why I left
> this in. I suppose that I can remove this member for now and add it back
> in when newer PMIC support is added.
I'm OK with keeping it as long as there is a real user coming up.
IMHO with the #defines as suggested by Matthias this will look better
anyway (it will be more obvious that this isn't a boolean, for
instance).
>>> +cleanup:
>>> + rpmh_release(rpmh_client);
>>
>> Still no devm_rpmh_get_client()? If Lina is too busy spinning her
>> patch series for other stuff, just add it to RPMH as a patch in your
>> series. I believe it's just this (untested):
>>
>> ---
>>
>> int devm_rpmh_release(struct device *dev, void *res)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> rpmh_release(pdev);
>> }
>>
>> int devm_rpmh_get_client(struct device *dev)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> void *dr;
>> int rc;
>>
>> dr = devres_alloc(devm_rpmh_release, 0, GFP_KERNEL);
>> if (!dr)
>> return -ENOMEM;
>>
>> rc = rpmh_get_client(pdev);
>> if (!rc)
>> devres_add(dev, dr);
>> else
>> devres_free(dr);
>>
>> return rc;
>> }
>>
>> ---
>>
>> Note that you'll get rid of the error handling in probe, the whole
>> remove function, and the need to do platform_set_drvdata().
>
> My understanding is that Lina is going to remove both rpmh_get_client()
> and rpmh_release(). In their place, rpmh functions will use the consumer
> device pointer as a handle and manage any necessary state internally [3].
> I'll update this patch once she uploads a new series with this interface
> modification.
OK, sounds good. Information like this is nice to include somewhere
in the cover letter or the patch description so it's more obvious that
you wouldn't want this patch to land until that's sorted out.
-Doug
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-19 16:16 ` Doug Anderson
0 siblings, 0 replies; 51+ messages in thread
From: Doug Anderson @ 2018-04-19 16:16 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Wed, Apr 18, 2018 at 4:30 PM, David Collins <collinsd@codeaurora.org> wrote:
>>> + * @drms_mode: Array of regulator framework modes which can
>>> + * be configured dynamically for this regulator
>>> + * via the set_load() callback.
>>
>> Using the singular for something that is an array is confusing. Why
>> not "drms_modes" or "drms_mode_arr"? In the past review you said
>> 'Perhaps something along the lines of "drms_modes"'.
>
> It seems awkward to me to use a plural for arrays as it leads to indexing
> like this: vreg->drms_modes[i]. "mode i" seems better than "modes i".
> However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
> if that style is preferred.
I'd very much like a plural here.
>>> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>>> + unsigned int mode, bool bypassed)
>>> +{
>>> + struct tcs_cmd cmd = {
>>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>>> + };
>>
>> Please add:
>>
>> if (mode & ~(REGULATOR_MODE_STANDBY |
>> REGULATOR_MODE_IDLE |
>> REGULATOR_MODE_NORMAL |
>> REGULATOR_MODE_FAST))
>> return -EINVAL;
>>
>> That way if someone adds a new mode you don't index off the end of
>> your array. Ah, I see, you have this in rpmh_regulator_vrm_set_mode
>> by checking if mode > REGULATOR_MODE_STANDBY. That works. Can you
>> move it here so it's closer to where the array access is?
>
> Theoretically, the (mode > REGULATOR_MODE_STANDBY) check in
> rpmh_regulator_vrm_set_mode() shouldn't be necessary at all. I felt safer
> leaving it in though. The framework ensures that no mode values may be
> passed into rpmh_regulator_vrm_set_mode() which is not identified in
> constraints.valid_modes_mask. Similar sanitization happens for internal
> rpmh_regulator_vrm_set_mode() calls.
>
> I'll move the (mode > REGULATOR_MODE_STANDBY) check into
> rpmh_regulator_vrm_set_mode_bypass().
Ah, good point about the valid_modes_mask! I'm happy with moving the
test here. I wouldn't mind a comment saying that the check is
probably overkill because the framework already checks
valid_modes_mask but shouldn't hurt.
>>> +
>>> + return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
>>
>> Might not hurt to have a comment saying that this calls
>> rpmh_regulator_vrm_set_mode() instead of calling
>> rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
>> to change the mode returned by a future call to get_mode().
>
> This seems pretty clear on inspection of the very closely spaced
> functions. I don't see the need for a comment about it.
It wasn't clear to me--I thought it might have just been because you
didn't want to manually pass in the current bypass state. Then I
looked at the function and thought there might have been a bug because
it was saving the mode. Then I looked back at the regulator framework
and finally came to the conclusion that set_load() is supposed to
change the mode (AKA: you'd expect that calling get_mode() after
set_load() would show you the mode you ended up at).
I guess this is all perhaps obvious to any regulator framework
experts, but since I spent 5 minutes digging through all that it
seemed like it deserved a comment to save the next person the 5
minutes. ...but I won't insist.
>>> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
>>> + bool *enable)
>>> +{
>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>> +
>>> + *enable = vreg->bypassed;
>>> +
>>> + return 0;
>>
>> Should you return an error code if nobody has ever called set_bypass?
>> ...or is it OK to just return "not bypassed"? Please document this in
>> the code.
>
> I think it is fine to return "not bypassed" by default if there is no
> configuration in place to enable bypassing. How are you suggesting that
> this be documented in the code?
I guess I wish the function had comments and then it could be
documented there. ...but none of the functions in this file do...
I guess you're right that it's clear enough without a comment, so
let's just leave it as is.
>>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
>>> + struct device *dev, struct device_node *node)
>>> +{
>>> + const char *prop;
>>> + int i, len, ret, mode;
>>> + u32 *buf;
>>> +
>>> + /* qcom,allowed-drms-modes is optional */
>>> + prop = "qcom,allowed-drms-modes";
>>> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
>>> + if (len < 0)
>>> + return 0;
>>> +
>>> + vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
>>> + GFP_KERNEL);
>>> + vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
>>> + sizeof(*vreg->drms_mode_max_uA), GFP_KERNEL);
>>> + if (!vreg->drms_mode || !vreg->drms_mode_max_uA)
>>> + return -ENOMEM;
>>> + vreg->drms_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) {
>>> + dev_err(dev, "%s: unable to read %s, ret=%d\n",
>>> + node->name, prop, ret);
>>> + goto done;
>>> + }
>>> +
>>> + for (i = 0; i < len; i++) {
>>> + mode = vreg->hw_data->of_map_mode(buf[i]);
>>> + if (mode <= 0) {
>>
>> Should be < 0, not <= 0 right? Unless we take Javier's suggestion
>> (see <https://patchwork.kernel.org/patch/10346081/>) and make 0 be
>> invalid...
>
> It looks like the way forward is REGULATOR_MODE_INVALID == 0 so '<= 0' is
> fine here. I suppose that it could be changed to '==
> REGULATOR_MODE_INVALID' as well.
Yes, assuming my patch lands using "==" is better. Checking whether
an unsigned value is <= 0 is confusing.
>>> + prop = "qcom,regulator-initial-voltage";
>>> + ret = of_property_read_u32(node, prop, &uV);
>>> + if (!ret) {
>>> + range = &vreg->hw_data->voltage_range;
>>> + selector = DIV_ROUND_UP(uV - range->min_uV,
>>> + range->uV_step) + range->min_sel;
>>> + if (uV < range->min_uV || selector > range->max_sel) {
>>> + dev_err(dev, "%s: %s=%u is invalid\n",
>>> + node->name, prop, uV);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + vreg->voltage_selector = selector;
>>> +
>>> + cmd[cmd_count].addr
>>> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
>>> + cmd[cmd_count++].data
>>> + = DIV_ROUND_UP(selector * range->uV_step
>>> + + range->min_uV, 1000);
>>> + }
>>
>> Seems like you want an "else { vreg->voltage_selector = -EINVAL; }".
>> Otherwise "get_voltage_sel" will return selector 0 before the first
>> set, right?
>>
>> Previously Mark said: "If the driver can't read values it should
>> return an appropriate error code."
>> ...and previously you said: "I'll try this out and see if the
>> regulator framework complains during regulator registration."
>
> I tested out what happens when vreg->voltage_selector = -EINVAL is set
> when qcom,regulator-initial-voltage is not present. This results in
> devm_regulator_register() failing and subsequently causing the
> qcom_rpmh-regulator probe to fail. The error happens in
> machine_constraints_voltage() [1].
>
> This leaves two courses of action:
> 1. (current patch set) allow voltage_selector to stay 0 if uninitialized
> 2. Set voltage_selector = -EINVAL by default and specify in DT binding
> documentation that qcom,regulator-initial-voltage is required for VRM
> managed RPMh regulator resources which have regulator-min-microvolt and
> regulator-max-microvolt specified.
>
> Are you ok with the DT implications of option #2?
You'd need to ask Mark if he's OK with it, but a option #3 is to add a
patch to your series fix the regulator framework to try setting the
voltage if _regulator_get_voltage() fails. Presumably in
machine_constraints_voltage() you'd now do something like:
int target_min, target_max;
int current_uV = _regulator_get_voltage(rdev);
if (current_uV < 0) {
/* Maybe this regulator's hardware can't be read and needs to be initted */
_regulator_do_set_voltage(
rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
current_uV = _regulator_get_voltage(rdev);
}
if (current_uV < 0) {
rdev_err(rdev,
"failed to get the current voltage(%d)\n",
current_uV);
return current_uV;
}
If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
but this needs to be documented _somewhere_ (unlike for bypass it's
not obvious, so you need to find someplace to put it). I'd rather not
hack the DT to deal with our software limitations.
>>> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
>>> + struct device_node *node, const char *pmic_id,
>>> + const struct rpmh_vreg_init_data *rpmh_data)
>>> +{
>>> + struct regulator_config reg_config = {};
>>> + char rpmh_resource_name[20] = "";
>>> + struct regulator_dev *rdev;
>>> + enum rpmh_regulator_type type;
>>> + struct regulator_init_data *init_data;
>>> + unsigned int mode;
>>> + int i, ret;
>>> +
>>> + for (; rpmh_data->name; rpmh_data++)
>>> + if (!strcmp(rpmh_data->name, node->name))
>>> + break;
>>> +
>>> + if (!rpmh_data->name) {
>>> + dev_err(dev, "Unknown regulator %s\n", node->name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
>>> + rpmh_data->resource_name, pmic_id);
>>
>> If the resulting string is exactly 20 characters then
>> rpmh_resource_name won't be zero terminated. Please handle this
>> properly.
>
> The output of scnprintf() is always null-terminated; therefore, no other
> check is needed. Also, RPMh resource names stored in SMEM command DB data
> structure are at most 8 bytes (<= 7 char + '\0' or 8 char and no '\0') so
> using 20 chars in the buffer is overkill anyway.
Wow, not sure where I looked to see that scnprintf() didn't always
null-terminate. Sorry. Ignore this.
>>> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>>
>> I may have suggested using this as an array that could be used as a
>> "map" (index by regulator framework mode and get the PMIC mode), but
>> now that I see it it doesn't seem super appealing since the regulator
>> framework mode is not 0, 1, 2, 3 but is actually 1, 2, 4, 8. ...but I
>> guess it's not too bad--you're allocating 9 ints to map 4 elements and
>> I guess that's about as efficient as you're going to get even if it
>> feels a bit ugly.
>
> I'm ok with the sparse mapping as it makes indexing as simple as possible
> and the extra space used is insignificant.
>
>
>> ...but still a few improvements:
>>
>> * Don't specify the size of the array as "REGULATOR_MODE_STANDBY + 1".
>> Let the compiler handle this. It should do the right thing. Then if
>> someone ever changes the order of the #defines things will just work,
>> I think.
>>
>> * Make sure that users of these arrays check that the mode is one of
>> the mode you know about. That way if someone does add a new mode you
>> don't index off your array. I'll put a comment in the user.
>
> Even if a new mode was added, the relative ordering of the existing modes
> should not change and valid_modes_mask will only allow modes currently
> supported by the driver. I'd like to keep the bound checks as simple as
> possible. By explicitly sizing the arrays and then only checking for mode
>> REGULATOR_MODE_STANDBY when indexing into the array we can be sure that
> no out-of-bound access can ever occur. Also, if one of the existing mode
> value was made larger than REGULATOR_MODE_STANDBY it would be easy to
> catch as it would cause a compilation error.
>
> Thus, I'd prefer to keep the array sizing and index checking as-in unless
> there is a major objection.
OK.
>>> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
>>> +{
>>> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
>>> + [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,
>>> + };
>>
>> You're sticking a negative value in an array of unsigned inits. Here
>> and in other similar functions.
>>
>> I know, I know. The function is defined to return an unsigned int.
>> It's wrong. of_regulator.c clearly puts the return code in a signed
>> int. First attempt at fixing this is at
>> <https://patchwork.kernel.org/patch/10346081/>.
>
> I can change the error cases to use REGULATOR_MODE_INVALID which is added
> by this change still under review [2].
I haven't seen Mark NAK it (yet), so for lack of a better option I'd
start using it in your patch and document in the commit message that
it depends on my patch.
>>> +static const struct rpmh_vreg_hw_data pmic4_bob = {
>>> + .regulator_type = VRM,
>>> + .ops = &rpmh_regulator_vrm_bypass_ops,
>>> + .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
>>> + .n_voltages = 84,
>>> + .pmic_mode_map = pmic_mode_map_pmic4_bob,
>>> + .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
>>> + .bypass_mode = 0,
>>
>> Remove .bypass_mode from the structure and just change
>> rpmh_regulator_vrm_set_mode_bypass() to set 0 if we're in bypass.
>> Right now 100% of PMICs that support bypass use 0 as the bypass mode.
>> If you ever have a future PMIC that uses a non-zero mode for bypass
>> then we can always add this back. ...and if no future PMICs ever use
>> a non-zero bypass mode then we don't need the complexity of having
>> another field in this struct.
>>
>> If you don't do this you might get arguments from some people saying
>> that they don't like seeing inits of "= 0" in static structures (Linux
>> conventions seem to like you to just assume that structs are
>> zero-initted).
>
> Upcoming PMICs use 2 for bypass mode instead of 0. That is why I left
> this in. I suppose that I can remove this member for now and add it back
> in when newer PMIC support is added.
I'm OK with keeping it as long as there is a real user coming up.
IMHO with the #defines as suggested by Matthias this will look better
anyway (it will be more obvious that this isn't a boolean, for
instance).
>>> +cleanup:
>>> + rpmh_release(rpmh_client);
>>
>> Still no devm_rpmh_get_client()? If Lina is too busy spinning her
>> patch series for other stuff, just add it to RPMH as a patch in your
>> series. I believe it's just this (untested):
>>
>> ---
>>
>> int devm_rpmh_release(struct device *dev, void *res)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> rpmh_release(pdev);
>> }
>>
>> int devm_rpmh_get_client(struct device *dev)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> void *dr;
>> int rc;
>>
>> dr = devres_alloc(devm_rpmh_release, 0, GFP_KERNEL);
>> if (!dr)
>> return -ENOMEM;
>>
>> rc = rpmh_get_client(pdev);
>> if (!rc)
>> devres_add(dev, dr);
>> else
>> devres_free(dr);
>>
>> return rc;
>> }
>>
>> ---
>>
>> Note that you'll get rid of the error handling in probe, the whole
>> remove function, and the need to do platform_set_drvdata().
>
> My understanding is that Lina is going to remove both rpmh_get_client()
> and rpmh_release(). In their place, rpmh functions will use the consumer
> device pointer as a handle and manage any necessary state internally [3].
> I'll update this patch once she uploads a new series with this interface
> modification.
OK, sounds good. Information like this is nice to include somewhere
in the cover letter or the patch description so it's more obvious that
you wouldn't want this patch to land until that's sorted out.
-Doug
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
2018-04-19 16:16 ` Doug Anderson
@ 2018-04-20 22:08 ` David Collins
-1 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-20 22:08 UTC (permalink / raw)
To: Doug Anderson, Mark Brown
Cc: Liam Girdwood, Rob Herring, Mark Rutland, linux-arm-msm,
Linux ARM, devicetree, LKML, Rajendra Nayak, Stephen Boyd,
Matthias Kaehlcke
On 04/19/2018 09:16 AM, Doug Anderson wrote:
> On Wed, Apr 18, 2018 at 4:30 PM, David Collins <collinsd@codeaurora.org> wrote:
>>>> + * @drms_mode: Array of regulator framework modes which can
>>>> + * be configured dynamically for this regulator
>>>> + * via the set_load() callback.
>>>
>>> Using the singular for something that is an array is confusing. Why
>>> not "drms_modes" or "drms_mode_arr"? In the past review you said
>>> 'Perhaps something along the lines of "drms_modes"'.
>>
>> It seems awkward to me to use a plural for arrays as it leads to indexing
>> like this: vreg->drms_modes[i]. "mode i" seems better than "modes i".
>> However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
>> if that style is preferred.
>
> I'd very much like a plural here.
Ok. I'll change this to be plural.
>>>> + prop = "qcom,regulator-initial-voltage";
>>>> + ret = of_property_read_u32(node, prop, &uV);
>>>> + if (!ret) {
>>>> + range = &vreg->hw_data->voltage_range;
>>>> + selector = DIV_ROUND_UP(uV - range->min_uV,
>>>> + range->uV_step) + range->min_sel;
>>>> + if (uV < range->min_uV || selector > range->max_sel) {
>>>> + dev_err(dev, "%s: %s=%u is invalid\n",
>>>> + node->name, prop, uV);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + vreg->voltage_selector = selector;
>>>> +
>>>> + cmd[cmd_count].addr
>>>> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
>>>> + cmd[cmd_count++].data
>>>> + = DIV_ROUND_UP(selector * range->uV_step
>>>> + + range->min_uV, 1000);
>>>> + }
>>>
>>> Seems like you want an "else { vreg->voltage_selector = -EINVAL; }".
>>> Otherwise "get_voltage_sel" will return selector 0 before the first
>>> set, right?
>>>
>>> Previously Mark said: "If the driver can't read values it should
>>> return an appropriate error code."
>>> ...and previously you said: "I'll try this out and see if the
>>> regulator framework complains during regulator registration."
>>
>> I tested out what happens when vreg->voltage_selector = -EINVAL is set
>> when qcom,regulator-initial-voltage is not present. This results in
>> devm_regulator_register() failing and subsequently causing the
>> qcom_rpmh-regulator probe to fail. The error happens in
>> machine_constraints_voltage() [1].
>>
>> This leaves two courses of action:
>> 1. (current patch set) allow voltage_selector to stay 0 if uninitialized
>> 2. Set voltage_selector = -EINVAL by default and specify in DT binding
>> documentation that qcom,regulator-initial-voltage is required for VRM
>> managed RPMh regulator resources which have regulator-min-microvolt and
>> regulator-max-microvolt specified.
>>
>> Are you ok with the DT implications of option #2?
>
> You'd need to ask Mark if he's OK with it, but a option #3 is to add a
> patch to your series fix the regulator framework to try setting the
> voltage if _regulator_get_voltage() fails. Presumably in
> machine_constraints_voltage() you'd now do something like:
>
> int target_min, target_max;
> int current_uV = _regulator_get_voltage(rdev);
> if (current_uV < 0) {
> /* Maybe this regulator's hardware can't be read and needs to be initted */
> _regulator_do_set_voltage(
> rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
> current_uV = _regulator_get_voltage(rdev);
> }
> if (current_uV < 0) {
> rdev_err(rdev,
> "failed to get the current voltage(%d)\n",
> current_uV);
> return current_uV;
> }
>
> If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
> but this needs to be documented _somewhere_ (unlike for bypass it's
> not obvious, so you need to find someplace to put it). I'd rather not
> hack the DT to deal with our software limitations.
I'm not opposed to your option #3 though it does seem a little hacky and
tailored to the qcom_rpmh-regulator specific case. Note that I think it
would be better to vote for min_uV to max_uV than min_uV to min_uV though.
Mark, what are your thoughts on the best way to handle this situation?
>>>> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
>>>> +{
>>>> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
>>>> + [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,
>>>> + };
>>>
>>> You're sticking a negative value in an array of unsigned inits. Here
>>> and in other similar functions.
>>>
>>> I know, I know. The function is defined to return an unsigned int.
>>> It's wrong. of_regulator.c clearly puts the return code in a signed
>>> int. First attempt at fixing this is at
>>> <https://patchwork.kernel.org/patch/10346081/>.
>>
>> I can change the error cases to use REGULATOR_MODE_INVALID which is added
>> by this change still under review [2].
>
> I haven't seen Mark NAK it (yet), so for lack of a better option I'd
> start using it in your patch and document in the commit message that
> it depends on my patch.
Your patch was accepted. I'll switch to using REGULATOR_MODE_INVALID.
Thanks,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-20 22:08 ` David Collins
0 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-20 22:08 UTC (permalink / raw)
To: linux-arm-kernel
On 04/19/2018 09:16 AM, Doug Anderson wrote:
> On Wed, Apr 18, 2018 at 4:30 PM, David Collins <collinsd@codeaurora.org> wrote:
>>>> + * @drms_mode: Array of regulator framework modes which can
>>>> + * be configured dynamically for this regulator
>>>> + * via the set_load() callback.
>>>
>>> Using the singular for something that is an array is confusing. Why
>>> not "drms_modes" or "drms_mode_arr"? In the past review you said
>>> 'Perhaps something along the lines of "drms_modes"'.
>>
>> It seems awkward to me to use a plural for arrays as it leads to indexing
>> like this: vreg->drms_modes[i]. "mode i" seems better than "modes i".
>> However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
>> if that style is preferred.
>
> I'd very much like a plural here.
Ok. I'll change this to be plural.
>>>> + prop = "qcom,regulator-initial-voltage";
>>>> + ret = of_property_read_u32(node, prop, &uV);
>>>> + if (!ret) {
>>>> + range = &vreg->hw_data->voltage_range;
>>>> + selector = DIV_ROUND_UP(uV - range->min_uV,
>>>> + range->uV_step) + range->min_sel;
>>>> + if (uV < range->min_uV || selector > range->max_sel) {
>>>> + dev_err(dev, "%s: %s=%u is invalid\n",
>>>> + node->name, prop, uV);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + vreg->voltage_selector = selector;
>>>> +
>>>> + cmd[cmd_count].addr
>>>> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
>>>> + cmd[cmd_count++].data
>>>> + = DIV_ROUND_UP(selector * range->uV_step
>>>> + + range->min_uV, 1000);
>>>> + }
>>>
>>> Seems like you want an "else { vreg->voltage_selector = -EINVAL; }".
>>> Otherwise "get_voltage_sel" will return selector 0 before the first
>>> set, right?
>>>
>>> Previously Mark said: "If the driver can't read values it should
>>> return an appropriate error code."
>>> ...and previously you said: "I'll try this out and see if the
>>> regulator framework complains during regulator registration."
>>
>> I tested out what happens when vreg->voltage_selector = -EINVAL is set
>> when qcom,regulator-initial-voltage is not present. This results in
>> devm_regulator_register() failing and subsequently causing the
>> qcom_rpmh-regulator probe to fail. The error happens in
>> machine_constraints_voltage() [1].
>>
>> This leaves two courses of action:
>> 1. (current patch set) allow voltage_selector to stay 0 if uninitialized
>> 2. Set voltage_selector = -EINVAL by default and specify in DT binding
>> documentation that qcom,regulator-initial-voltage is required for VRM
>> managed RPMh regulator resources which have regulator-min-microvolt and
>> regulator-max-microvolt specified.
>>
>> Are you ok with the DT implications of option #2?
>
> You'd need to ask Mark if he's OK with it, but a option #3 is to add a
> patch to your series fix the regulator framework to try setting the
> voltage if _regulator_get_voltage() fails. Presumably in
> machine_constraints_voltage() you'd now do something like:
>
> int target_min, target_max;
> int current_uV = _regulator_get_voltage(rdev);
> if (current_uV < 0) {
> /* Maybe this regulator's hardware can't be read and needs to be initted */
> _regulator_do_set_voltage(
> rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
> current_uV = _regulator_get_voltage(rdev);
> }
> if (current_uV < 0) {
> rdev_err(rdev,
> "failed to get the current voltage(%d)\n",
> current_uV);
> return current_uV;
> }
>
> If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
> but this needs to be documented _somewhere_ (unlike for bypass it's
> not obvious, so you need to find someplace to put it). I'd rather not
> hack the DT to deal with our software limitations.
I'm not opposed to your option #3 though it does seem a little hacky and
tailored to the qcom_rpmh-regulator specific case. Note that I think it
would be better to vote for min_uV to max_uV than min_uV to min_uV though.
Mark, what are your thoughts on the best way to handle this situation?
>>>> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
>>>> +{
>>>> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
>>>> + [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,
>>>> + };
>>>
>>> You're sticking a negative value in an array of unsigned inits. Here
>>> and in other similar functions.
>>>
>>> I know, I know. The function is defined to return an unsigned int.
>>> It's wrong. of_regulator.c clearly puts the return code in a signed
>>> int. First attempt at fixing this is at
>>> <https://patchwork.kernel.org/patch/10346081/>.
>>
>> I can change the error cases to use REGULATOR_MODE_INVALID which is added
>> by this change still under review [2].
>
> I haven't seen Mark NAK it (yet), so for lack of a better option I'd
> start using it in your patch and document in the commit message that
> it depends on my patch.
Your patch was accepted. I'll switch to using REGULATOR_MODE_INVALID.
Thanks,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
2018-04-20 22:08 ` David Collins
@ 2018-04-24 17:45 ` Mark Brown
-1 siblings, 0 replies; 51+ messages in thread
From: Mark Brown @ 2018-04-24 17:45 UTC (permalink / raw)
To: David Collins
Cc: Doug Anderson, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd, Matthias Kaehlcke
[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]
On Fri, Apr 20, 2018 at 03:08:57PM -0700, David Collins wrote:
> On 04/19/2018 09:16 AM, Doug Anderson wrote:
> > On Wed, Apr 18, 2018 at 4:30 PM, David Collins <collinsd@codeaurora.org> wrote:
Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
> > You'd need to ask Mark if he's OK with it, but a option #3 is to add a
> > patch to your series fix the regulator framework to try setting the
> > voltage if _regulator_get_voltage() fails. Presumably in
> > machine_constraints_voltage() you'd now do something like:
> >
> > int target_min, target_max;
> > int current_uV = _regulator_get_voltage(rdev);
> > if (current_uV < 0) {
> > /* Maybe this regulator's hardware can't be read and needs to be initted */
> > _regulator_do_set_voltage(
> > rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
> > current_uV = _regulator_get_voltage(rdev);
> > }
> > if (current_uV < 0) {
> > rdev_err(rdev,
> > "failed to get the current voltage(%d)\n",
> > current_uV);
> > return current_uV;
> > }
> > If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
> > but this needs to be documented _somewhere_ (unlike for bypass it's
> > not obvious, so you need to find someplace to put it). I'd rather not
> > hack the DT to deal with our software limitations.
> I'm not opposed to your option #3 though it does seem a little hacky and
> tailored to the qcom_rpmh-regulator specific case. Note that I think it
> would be better to vote for min_uV to max_uV than min_uV to min_uV though.
> Mark, what are your thoughts on the best way to handle this situation?
I think that's probably only OK if we have a specific error code for the
regulator being limited in this way otherwise our error handling for I/O
problems involves us trying to reconfigure supplies which seems like it
would be risky.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-24 17:45 ` Mark Brown
0 siblings, 0 replies; 51+ messages in thread
From: Mark Brown @ 2018-04-24 17:45 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 20, 2018 at 03:08:57PM -0700, David Collins wrote:
> On 04/19/2018 09:16 AM, Doug Anderson wrote:
> > On Wed, Apr 18, 2018 at 4:30 PM, David Collins <collinsd@codeaurora.org> wrote:
Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
> > You'd need to ask Mark if he's OK with it, but a option #3 is to add a
> > patch to your series fix the regulator framework to try setting the
> > voltage if _regulator_get_voltage() fails. Presumably in
> > machine_constraints_voltage() you'd now do something like:
> >
> > int target_min, target_max;
> > int current_uV = _regulator_get_voltage(rdev);
> > if (current_uV < 0) {
> > /* Maybe this regulator's hardware can't be read and needs to be initted */
> > _regulator_do_set_voltage(
> > rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
> > current_uV = _regulator_get_voltage(rdev);
> > }
> > if (current_uV < 0) {
> > rdev_err(rdev,
> > "failed to get the current voltage(%d)\n",
> > current_uV);
> > return current_uV;
> > }
> > If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
> > but this needs to be documented _somewhere_ (unlike for bypass it's
> > not obvious, so you need to find someplace to put it). I'd rather not
> > hack the DT to deal with our software limitations.
> I'm not opposed to your option #3 though it does seem a little hacky and
> tailored to the qcom_rpmh-regulator specific case. Note that I think it
> would be better to vote for min_uV to max_uV than min_uV to min_uV though.
> Mark, what are your thoughts on the best way to handle this situation?
I think that's probably only OK if we have a specific error code for the
regulator being limited in this way otherwise our error handling for I/O
problems involves us trying to reconfigure supplies which seems like it
would be risky.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180424/09cd228d/attachment.sig>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
2018-04-24 17:45 ` Mark Brown
@ 2018-04-24 21:09 ` David Collins
-1 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-24 21:09 UTC (permalink / raw)
To: Mark Brown
Cc: Doug Anderson, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd, Matthias Kaehlcke
On 04/24/2018 10:45 AM, Mark Brown wrote:
>>> You'd need to ask Mark if he's OK with it, but a option #3 is to add a
>>> patch to your series fix the regulator framework to try setting the
>>> voltage if _regulator_get_voltage() fails. Presumably in
>>> machine_constraints_voltage() you'd now do something like:
>>>
>>> int target_min, target_max;
>>> int current_uV = _regulator_get_voltage(rdev);
>>> if (current_uV < 0) {
>>> /* Maybe this regulator's hardware can't be read and needs to be initted */
>>> _regulator_do_set_voltage(
>>> rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
>>> current_uV = _regulator_get_voltage(rdev);
>>> }
>>> if (current_uV < 0) {
>>> rdev_err(rdev,
>>> "failed to get the current voltage(%d)\n",
>>> current_uV);
>>> return current_uV;
>>> }
>
>>> If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
>>> but this needs to be documented _somewhere_ (unlike for bypass it's
>>> not obvious, so you need to find someplace to put it). I'd rather not
>>> hack the DT to deal with our software limitations.
>
>> I'm not opposed to your option #3 though it does seem a little hacky and
>> tailored to the qcom_rpmh-regulator specific case. Note that I think it
>> would be better to vote for min_uV to max_uV than min_uV to min_uV though.
>
>> Mark, what are your thoughts on the best way to handle this situation?
>
> I think that's probably only OK if we have a specific error code for the
> regulator being limited in this way otherwise our error handling for I/O
> problems involves us trying to reconfigure supplies which seems like it
> would be risky.
Would you be ok with -EAGAIN being used for this purpose?
Thanks,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-24 21:09 ` David Collins
0 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-24 21:09 UTC (permalink / raw)
To: linux-arm-kernel
On 04/24/2018 10:45 AM, Mark Brown wrote:
>>> You'd need to ask Mark if he's OK with it, but a option #3 is to add a
>>> patch to your series fix the regulator framework to try setting the
>>> voltage if _regulator_get_voltage() fails. Presumably in
>>> machine_constraints_voltage() you'd now do something like:
>>>
>>> int target_min, target_max;
>>> int current_uV = _regulator_get_voltage(rdev);
>>> if (current_uV < 0) {
>>> /* Maybe this regulator's hardware can't be read and needs to be initted */
>>> _regulator_do_set_voltage(
>>> rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
>>> current_uV = _regulator_get_voltage(rdev);
>>> }
>>> if (current_uV < 0) {
>>> rdev_err(rdev,
>>> "failed to get the current voltage(%d)\n",
>>> current_uV);
>>> return current_uV;
>>> }
>
>>> If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
>>> but this needs to be documented _somewhere_ (unlike for bypass it's
>>> not obvious, so you need to find someplace to put it). I'd rather not
>>> hack the DT to deal with our software limitations.
>
>> I'm not opposed to your option #3 though it does seem a little hacky and
>> tailored to the qcom_rpmh-regulator specific case. Note that I think it
>> would be better to vote for min_uV to max_uV than min_uV to min_uV though.
>
>> Mark, what are your thoughts on the best way to handle this situation?
>
> I think that's probably only OK if we have a specific error code for the
> regulator being limited in this way otherwise our error handling for I/O
> problems involves us trying to reconfigure supplies which seems like it
> would be risky.
Would you be ok with -EAGAIN being used for this purpose?
Thanks,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
2018-04-24 21:09 ` David Collins
@ 2018-04-25 10:31 ` Mark Brown
-1 siblings, 0 replies; 51+ messages in thread
From: Mark Brown @ 2018-04-25 10:31 UTC (permalink / raw)
To: David Collins
Cc: Doug Anderson, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd, Matthias Kaehlcke
[-- Attachment #1: Type: text/plain, Size: 609 bytes --]
On Tue, Apr 24, 2018 at 02:09:47PM -0700, David Collins wrote:
> On 04/24/2018 10:45 AM, Mark Brown wrote:
> > I think that's probably only OK if we have a specific error code for the
> > regulator being limited in this way otherwise our error handling for I/O
> > problems involves us trying to reconfigure supplies which seems like it
> > would be risky.
> Would you be ok with -EAGAIN being used for this purpose?
Using -EAGAIN for "I can't ever read the configuration from this
regulator" doesn't seem right - it's not like any number of retries
will ever manage to read the value back.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-25 10:31 ` Mark Brown
0 siblings, 0 replies; 51+ messages in thread
From: Mark Brown @ 2018-04-25 10:31 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Apr 24, 2018 at 02:09:47PM -0700, David Collins wrote:
> On 04/24/2018 10:45 AM, Mark Brown wrote:
> > I think that's probably only OK if we have a specific error code for the
> > regulator being limited in this way otherwise our error handling for I/O
> > problems involves us trying to reconfigure supplies which seems like it
> > would be risky.
> Would you be ok with -EAGAIN being used for this purpose?
Using -EAGAIN for "I can't ever read the configuration from this
regulator" doesn't seem right - it's not like any number of retries
will ever manage to read the value back.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180425/2373cdee/attachment.sig>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
2018-04-25 10:31 ` Mark Brown
@ 2018-04-25 21:04 ` David Collins
-1 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-25 21:04 UTC (permalink / raw)
To: Mark Brown
Cc: Doug Anderson, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd, Matthias Kaehlcke
>>> I think that's probably only OK if we have a specific error code for the
>>> regulator being limited in this way otherwise our error handling for I/O
>>> problems involves us trying to reconfigure supplies which seems like it
>>> would be risky.
>> Would you be ok with -EAGAIN being used for this purpose?
> Using -EAGAIN for "I can't ever read the configuration from this
> regulator" doesn't seem right - it's not like any number of retries
> will ever manage to read the value back.
In this case, the _regulator_get_voltage() call can succeed, but only
after a voltage is explicitly requested from the framework side. The
intention here would then be to call _regulator_do_set_voltage() with the
constraint min_uV to max_uV range. After that, subsequent
_regulator_get_voltage() calls will be successful.
Here is the general idea:
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 65f9b7c..e61983d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -910,6 +910,19 @@ static int machine_constraints_voltage(struct
regulator_dev *rdev,
rdev->constraints->min_uV && rdev->constraints->max_uV) {
int target_min, target_max;
int current_uV = _regulator_get_voltage(rdev);
+ if (current_uV == -EAGAIN) {
+ /*
+ * Regulator voltage cannot be read until after
+ * configuration; try setting constraint range.
+ */
+ rdev_info(rdev, "Setting %d-%duV\n",
+ rdev->constraints->min_uV,
+ rdev->constraints->max_uV);
+ _regulator_do_set_voltage(rdev,
+ rdev->constraints->min_uV,
+ rdev->constraints->max_uV);
+ current_uV = _regulator_get_voltage(rdev);
+ }
if (current_uV < 0) {
rdev_err(rdev,
"failed to get the current voltage(%d)\n",
Do you still have reservations about using -EAGAIN for this purpose? If
so, which error code would you suggest using?
Thanks,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-04-25 21:04 ` David Collins
0 siblings, 0 replies; 51+ messages in thread
From: David Collins @ 2018-04-25 21:04 UTC (permalink / raw)
To: linux-arm-kernel
>>> I think that's probably only OK if we have a specific error code for the
>>> regulator being limited in this way otherwise our error handling for I/O
>>> problems involves us trying to reconfigure supplies which seems like it
>>> would be risky.
>> Would you be ok with -EAGAIN being used for this purpose?
> Using -EAGAIN for "I can't ever read the configuration from this
> regulator" doesn't seem right - it's not like any number of retries
> will ever manage to read the value back.
In this case, the _regulator_get_voltage() call can succeed, but only
after a voltage is explicitly requested from the framework side. The
intention here would then be to call _regulator_do_set_voltage() with the
constraint min_uV to max_uV range. After that, subsequent
_regulator_get_voltage() calls will be successful.
Here is the general idea:
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 65f9b7c..e61983d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -910,6 +910,19 @@ static int machine_constraints_voltage(struct
regulator_dev *rdev,
rdev->constraints->min_uV && rdev->constraints->max_uV) {
int target_min, target_max;
int current_uV = _regulator_get_voltage(rdev);
+ if (current_uV == -EAGAIN) {
+ /*
+ * Regulator voltage cannot be read until after
+ * configuration; try setting constraint range.
+ */
+ rdev_info(rdev, "Setting %d-%duV\n",
+ rdev->constraints->min_uV,
+ rdev->constraints->max_uV);
+ _regulator_do_set_voltage(rdev,
+ rdev->constraints->min_uV,
+ rdev->constraints->max_uV);
+ current_uV = _regulator_get_voltage(rdev);
+ }
if (current_uV < 0) {
rdev_err(rdev,
"failed to get the current voltage(%d)\n",
Do you still have reservations about using -EAGAIN for this purpose? If
so, which error code would you suggest using?
Thanks,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
2018-04-25 21:04 ` David Collins
@ 2018-05-01 21:02 ` Mark Brown
-1 siblings, 0 replies; 51+ messages in thread
From: Mark Brown @ 2018-05-01 21:02 UTC (permalink / raw)
To: David Collins
Cc: Doug Anderson, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd, Matthias Kaehlcke
[-- Attachment #1: Type: text/plain, Size: 786 bytes --]
On Wed, Apr 25, 2018 at 02:04:56PM -0700, David Collins wrote:
> > Using -EAGAIN for "I can't ever read the configuration from this
> > regulator" doesn't seem right - it's not like any number of retries
> > will ever manage to read the value back.
> In this case, the _regulator_get_voltage() call can succeed, but only
> after a voltage is explicitly requested from the framework side. The
...
> Do you still have reservations about using -EAGAIN for this purpose? If
> so, which error code would you suggest using?
Yes, that's clearly a problem - -EAGAIN is more for situations where you
can just immediately retry like signal interruptions. If the caller
repetedly sits and tries to read the voltage it'll never succeed unless
something else comes along and sets something.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver
@ 2018-05-01 21:02 ` Mark Brown
0 siblings, 0 replies; 51+ messages in thread
From: Mark Brown @ 2018-05-01 21:02 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Apr 25, 2018 at 02:04:56PM -0700, David Collins wrote:
> > Using -EAGAIN for "I can't ever read the configuration from this
> > regulator" doesn't seem right - it's not like any number of retries
> > will ever manage to read the value back.
> In this case, the _regulator_get_voltage() call can succeed, but only
> after a voltage is explicitly requested from the framework side. The
...
> Do you still have reservations about using -EAGAIN for this purpose? If
> so, which error code would you suggest using?
Yes, that's clearly a problem - -EAGAIN is more for situations where you
can just immediately retry like signal interruptions. If the caller
repetedly sits and tries to read the voltage it'll never succeed unless
something else comes along and sets something.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180502/c3175675/attachment.sig>
^ permalink raw reply [flat|nested] 51+ messages in thread