[PATCH v5 0/2] Add support for voltage regulator on ChromeOS EC.

[PATCH v5 0/2] Add support for voltage regulator on ChromeOS EC.

[Pi-Hsun Shih]

Add support for controlling voltage regulator that is connected and
controlled by ChromeOS EC. Kernel controls these regulators through
newly added EC host commands.

Changes from v4:
* Change compatible name from regulator-cros-ec to cros-ec-regulator.

Changes from v3:
* Fix dt bindings file name.
* Remove check around CONFIG_OF in driver.
* Add new host commands to cros_ec_trace.
* Address review comments.

Changes from v2:
* Add 'depends on OF' to Kconfig.
* Add Kconfig description about compiling as module.

Changes from v1:
* Change compatible string to google,regulator-cros-ec.
* Use reg property in device tree.
* Change license for dt binding according to checkpatch.pl.
* Address comments on code styles.

Pi-Hsun Shih (2):
  dt-bindings: regulator: Add DT binding for cros-ec-regulator
  regulator: Add driver for cros-ec-regulator

 .../regulator/google,cros-ec-regulator.yaml   |  51 ++++
 drivers/platform/chrome/cros_ec_trace.c       |   5 +
 drivers/regulator/Kconfig                     |  10 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/cros-ec-regulator.c         | 256 ++++++++++++++++++
 .../linux/platform_data/cros_ec_commands.h    |  82 ++++++
 6 files changed, 405 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml
 create mode 100644 drivers/regulator/cros-ec-regulator.c


base-commit: b29482fde649c72441d5478a4ea2c52c56d97a5e
-- 
2.27.0.278.ge193c7cf3a9-goog

[PATCH v5 1/2] dt-bindings: regulator: Add DT binding for cros-ec-regulator

[Pi-Hsun Shih]

Add DT binding documentation for cros-ec-regulator, a voltage regulator
controlled by ChromeOS EC.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
Changes from v4:
* Change compatible name from regulator-cros-ec to cros-ec-regulator.

Changes from v3:
* Fix dt bindings file name.
* Add full example.

Changes from v2:
* No change

Changes from v1:
* Change compatible string to google,regulator-cros-ec.
* Use reg property in device tree.
* Change license for dt binding according to checkpatch.pl.
---
 .../regulator/google,cros-ec-regulator.yaml   | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml

diff --git a/Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml b/Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml
new file mode 100644
index 000000000000..c9453d7ce227
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/google,cros-ec-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ChromeOS EC controlled voltage regulators
+
+maintainers:
+  - Pi-Hsun Shih <pihsun@chromium.org>
+
+description:
+  Any property defined as part of the core regulator binding, defined in
+  regulator.yaml, can also be used.
+
+allOf:
+  - $ref: "regulator.yaml#"
+
+properties:
+  compatible:
+    const: google,cros-ec-regulator
+
+  reg:
+    maxItems: 1
+    description: Identifier for the voltage regulator to ChromeOS EC.
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    spi0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        cros_ec: ec@0 {
+            compatible = "google,cros-ec-spi";
+            reg = <0>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            regulator@0 {
+                compatible = "google,cros-ec-regulator";
+                regulator-min-microvolt = <1800000>;
+                regulator-max-microvolt = <3300000>;
+                reg = <0>;
+            };
+        };
+    };
+...
-- 
2.27.0.278.ge193c7cf3a9-goog

[PATCH v5 2/2] regulator: Add driver for cros-ec-regulator

[Pi-Hsun Shih]

Add driver for cros-ec-regulator, representing a voltage regulator that
is connected and controlled by ChromeOS EC, and is controlled by kernel
with EC host commands.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
Changes from v4:
* Change compatible name from regulator-cros-ec to cros-ec-regulator.

Changes from v3:
* Remove check around CONFIG_OF.
* Add new host commands to cros_ec_trace.
* Use devm_kstrdup for duping regulator name.
* Change license header and add MODULE_AUTHOR.
* Address review comments.

Changes from v2:
* Add 'depends on OF' to Kconfig.
* Add Kconfig description about compiling as module.

Changes from v1:
* Change compatible string to google,regulator-cros-ec.
* Use reg property in device tree.
* Address comments on code styles.

This patch contains function cros_ec_cmd that is copied from the series:
https://lore.kernel.org/patchwork/project/lkml/list/?series=428457.

I can't find the first patch in that v2 series, so the function is
modified from v1 of that series according to reviewers comment:
https://lore.kernel.org/patchwork/patch/1188110/

I copied the function instead of depending on that series since I feel
the function is small enough, and the series has stalled for some time.
---
 drivers/platform/chrome/cros_ec_trace.c       |   5 +
 drivers/regulator/Kconfig                     |  10 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/cros-ec-regulator.c         | 256 ++++++++++++++++++
 .../linux/platform_data/cros_ec_commands.h    |  82 ++++++
 5 files changed, 354 insertions(+)
 create mode 100644 drivers/regulator/cros-ec-regulator.c

diff --git a/drivers/platform/chrome/cros_ec_trace.c b/drivers/platform/chrome/cros_ec_trace.c
index 523a39bd0ff6..425e9441b7ca 100644
--- a/drivers/platform/chrome/cros_ec_trace.c
+++ b/drivers/platform/chrome/cros_ec_trace.c
@@ -161,6 +161,11 @@
 	TRACE_SYMBOL(EC_CMD_ADC_READ), \
 	TRACE_SYMBOL(EC_CMD_ROLLBACK_INFO), \
 	TRACE_SYMBOL(EC_CMD_AP_RESET), \
+	TRACE_SYMBOL(EC_CMD_REGULATOR_GET_INFO), \
+	TRACE_SYMBOL(EC_CMD_REGULATOR_ENABLE), \
+	TRACE_SYMBOL(EC_CMD_REGULATOR_IS_ENABLED), \
+	TRACE_SYMBOL(EC_CMD_REGULATOR_SET_VOLTAGE), \
+	TRACE_SYMBOL(EC_CMD_REGULATOR_GET_VOLTAGE), \
 	TRACE_SYMBOL(EC_CMD_CR51_BASE), \
 	TRACE_SYMBOL(EC_CMD_CR51_LAST), \
 	TRACE_SYMBOL(EC_CMD_FP_PASSTHRU), \
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 8f677f5d79b4..c398e90e0e73 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -238,6 +238,16 @@ config REGULATOR_CPCAP
 	  Say y here for CPCAP regulator found on some Motorola phones
 	  and tablets such as Droid 4.
 
+config REGULATOR_CROS_EC
+	tristate "ChromeOS EC regulators"
+	depends on CROS_EC && OF
+	help
+	  This driver supports voltage regulators that is connected to ChromeOS
+	  EC and controlled through EC host commands.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called cros-ec-regulator.
+
 config REGULATOR_DA903X
 	tristate "Dialog Semiconductor DA9030/DA9034 regulators"
 	depends on PMIC_DA903X
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index e8f163371071..46592c160d22 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
 obj-$(CONFIG_REGULATOR_88PG86X) += 88pg86x.o
 obj-$(CONFIG_REGULATOR_88PM800) += 88pm800-regulator.o
 obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o
+obj-$(CONFIG_REGULATOR_CROS_EC) += cros-ec-regulator.o
 obj-$(CONFIG_REGULATOR_CPCAP) += cpcap-regulator.o
 obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
 obj-$(CONFIG_REGULATOR_AB3100) += ab3100.o
diff --git a/drivers/regulator/cros-ec-regulator.c b/drivers/regulator/cros-ec-regulator.c
new file mode 100644
index 000000000000..830ceefc704a
--- /dev/null
+++ b/drivers/regulator/cros-ec-regulator.c
@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright 2020 Google LLC.
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/slab.h>
+
+struct cros_ec_regulator_data {
+	struct regulator_desc desc;
+	struct regulator_dev *dev;
+	struct cros_ec_device *ec_dev;
+
+	u32 index;
+
+	u16 *voltages_mV;
+	u16 num_voltages;
+};
+
+static int cros_ec_cmd(struct cros_ec_device *ec, u32 version, u32 command,
+		       void *outdata, u32 outsize, void *indata, u32 insize)
+{
+	struct cros_ec_command *msg;
+	int ret;
+
+	msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->version = version;
+	msg->command = command;
+	msg->outsize = outsize;
+	msg->insize = insize;
+
+	if (outdata && outsize > 0)
+		memcpy(msg->data, outdata, outsize);
+
+	ret = cros_ec_cmd_xfer_status(ec, msg);
+	if (ret < 0)
+		goto cleanup;
+
+	if (insize)
+		memcpy(indata, msg->data, insize);
+
+cleanup:
+	kfree(msg);
+	return ret;
+}
+
+static int cros_ec_regulator_enable(struct regulator_dev *dev)
+{
+	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
+	struct ec_params_regulator_enable cmd = {
+		.index = data->index,
+		.enable = 1,
+	};
+
+	return cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_ENABLE, &cmd,
+			  sizeof(cmd), NULL, 0);
+}
+
+static int cros_ec_regulator_disable(struct regulator_dev *dev)
+{
+	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
+	struct ec_params_regulator_enable cmd = {
+		.index = data->index,
+		.enable = 0,
+	};
+
+	return cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_ENABLE, &cmd,
+			  sizeof(cmd), NULL, 0);
+}
+
+static int cros_ec_regulator_is_enabled(struct regulator_dev *dev)
+{
+	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
+	struct ec_params_regulator_is_enabled cmd = {
+		.index = data->index,
+	};
+	struct ec_response_regulator_is_enabled resp;
+	int ret;
+
+	ret = cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_IS_ENABLED, &cmd,
+			  sizeof(cmd), &resp, sizeof(resp));
+	if (ret < 0)
+		return ret;
+	return resp.enabled;
+}
+
+static int cros_ec_regulator_list_voltage(struct regulator_dev *dev,
+					  unsigned int selector)
+{
+	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
+
+	if (selector >= data->num_voltages)
+		return -EINVAL;
+
+	return data->voltages_mV[selector] * 1000;
+}
+
+static int cros_ec_regulator_get_voltage(struct regulator_dev *dev)
+{
+	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
+	struct ec_params_regulator_get_voltage cmd = {
+		.index = data->index,
+	};
+	struct ec_response_regulator_get_voltage resp;
+	int ret;
+
+	ret = cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_GET_VOLTAGE, &cmd,
+			  sizeof(cmd), &resp, sizeof(resp));
+	if (ret < 0)
+		return ret;
+	return resp.voltage_mv * 1000;
+}
+
+static int cros_ec_regulator_set_voltage(struct regulator_dev *dev, int min_uV,
+					 int max_uV, unsigned int *selector)
+{
+	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
+	int min_mV = DIV_ROUND_UP(min_uV, 1000);
+	int max_mV = max_uV / 1000;
+	struct ec_params_regulator_set_voltage cmd = {
+		.index = data->index,
+		.min_mv = min_mV,
+		.max_mv = max_mV,
+	};
+
+	/*
+	 * This can happen when the given range [min_uV, max_uV] doesn't
+	 * contain any voltage that can be represented exactly in mV.
+	 */
+	if (min_mV > max_mV)
+		return -EINVAL;
+	return cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_SET_VOLTAGE, &cmd,
+			   sizeof(cmd), NULL, 0);
+}
+
+static struct regulator_ops cros_ec_regulator_voltage_ops = {
+	.enable = cros_ec_regulator_enable,
+	.disable = cros_ec_regulator_disable,
+	.is_enabled = cros_ec_regulator_is_enabled,
+	.list_voltage = cros_ec_regulator_list_voltage,
+	.get_voltage = cros_ec_regulator_get_voltage,
+	.set_voltage = cros_ec_regulator_set_voltage,
+};
+
+static int cros_ec_regulator_init_info(struct device *dev,
+				       struct cros_ec_regulator_data *data)
+{
+	struct ec_params_regulator_get_info cmd = {
+		.index = data->index,
+	};
+	struct ec_response_regulator_get_info resp;
+	int ret;
+
+	ret = cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_GET_INFO, &cmd,
+			   sizeof(cmd), &resp, sizeof(resp));
+	if (ret < 0)
+		return ret;
+
+	data->num_voltages =
+		min_t(u16, ARRAY_SIZE(resp.voltages_mv), resp.num_voltages);
+	data->voltages_mV =
+		devm_kmemdup(dev, resp.voltages_mv,
+			     sizeof(u16) * data->num_voltages, GFP_KERNEL);
+	data->desc.n_voltages = data->num_voltages;
+
+	/* Make sure the returned name is always a valid string */
+	resp.name[sizeof(resp.name) - 1] = '\0';
+	data->desc.name = devm_kstrdup(dev, resp.name, GFP_KERNEL);
+	if (!data->desc.name)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int cros_ec_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct cros_ec_regulator_data *drvdata;
+	struct regulator_init_data *init_data;
+	struct regulator_config cfg = {};
+	struct regulator_desc *desc;
+	int ret;
+
+	drvdata = devm_kzalloc(
+		&pdev->dev, sizeof(struct cros_ec_regulator_data), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->ec_dev = dev_get_drvdata(dev->parent);
+	desc = &drvdata->desc;
+
+	init_data = of_get_regulator_init_data(dev, np, desc);
+	if (!init_data)
+		return -EINVAL;
+
+	ret = of_property_read_u32(np, "reg", &drvdata->index);
+	if (ret < 0)
+		return ret;
+
+	desc->owner = THIS_MODULE;
+	desc->type = REGULATOR_VOLTAGE;
+	desc->ops = &cros_ec_regulator_voltage_ops;
+
+	ret = cros_ec_regulator_init_info(dev, drvdata);
+	if (ret < 0)
+		return ret;
+
+	cfg.dev = &pdev->dev;
+	cfg.init_data = init_data;
+	cfg.driver_data = drvdata;
+	cfg.of_node = np;
+
+	drvdata->dev = regulator_register(&drvdata->desc, &cfg);
+	if (IS_ERR(drvdata->dev)) {
+		ret = PTR_ERR(drvdata->dev);
+		dev_err(&pdev->dev, "Failed to register regulator: %d\n", ret);
+		goto free_name;
+	}
+
+	platform_set_drvdata(pdev, drvdata);
+
+	return 0;
+
+free_name:
+	kfree(desc->name);
+	return ret;
+}
+
+static const struct of_device_id regulator_cros_ec_of_match[] = {
+	{ .compatible = "google,cros-ec-regulator", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, regulator_cros_ec_of_match);
+
+static struct platform_driver cros_ec_regulator_driver = {
+	.probe		= cros_ec_regulator_probe,
+	.driver		= {
+		.name		= "cros-ec-regulator",
+		.of_match_table = regulator_cros_ec_of_match,
+	},
+};
+
+module_platform_driver(cros_ec_regulator_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ChromeOS EC controlled regulator");
+MODULE_AUTHOR("Pi-Hsun Shih <pihsun@chromium.org>");
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index 69210881ebac..a417b51b5764 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -5430,6 +5430,88 @@ struct ec_response_rollback_info {
 /* Issue AP reset */
 #define EC_CMD_AP_RESET 0x0125
 
+/*****************************************************************************/
+/* Voltage regulator controls */
+
+/*
+ * Get basic info of voltage regulator for given index.
+ *
+ * Returns the regulator name and supported voltage list in mV.
+ */
+#define EC_CMD_REGULATOR_GET_INFO 0x012B
+
+/* Maximum length of regulator name */
+#define EC_REGULATOR_NAME_MAX_LEN 16
+
+/* Maximum length of the supported voltage list. */
+#define EC_REGULATOR_VOLTAGE_MAX_COUNT 16
+
+struct ec_params_regulator_get_info {
+	uint32_t index;
+} __ec_align4;
+
+struct ec_response_regulator_get_info {
+	char name[EC_REGULATOR_NAME_MAX_LEN];
+	uint16_t num_voltages;
+	uint16_t voltages_mv[EC_REGULATOR_VOLTAGE_MAX_COUNT];
+} __ec_align1;
+
+/*
+ * Configure the regulator as enabled / disabled.
+ */
+#define EC_CMD_REGULATOR_ENABLE 0x012C
+
+struct ec_params_regulator_enable {
+	uint32_t index;
+	uint8_t enable;
+} __ec_align4;
+
+/*
+ * Query if the regulator is enabled.
+ *
+ * Returns 1 if the regulator is enabled, 0 if not.
+ */
+#define EC_CMD_REGULATOR_IS_ENABLED 0x012D
+
+struct ec_params_regulator_is_enabled {
+	uint32_t index;
+} __ec_align4;
+
+struct ec_response_regulator_is_enabled {
+	uint8_t enabled;
+} __ec_align1;
+
+/*
+ * Set voltage for the voltage regulator within the range specified.
+ *
+ * The driver should select the voltage in range closest to min_mv.
+ *
+ * Also note that this might be called before the regulator is enabled, and the
+ * setting should be in effect after the regulator is enabled.
+ */
+#define EC_CMD_REGULATOR_SET_VOLTAGE 0x012E
+
+struct ec_params_regulator_set_voltage {
+	uint32_t index;
+	uint32_t min_mv;
+	uint32_t max_mv;
+} __ec_align4;
+
+/*
+ * Get the currently configured voltage for the voltage regulator.
+ *
+ * Note that this might be called before the regulator is enabled.
+ */
+#define EC_CMD_REGULATOR_GET_VOLTAGE 0x012F
+
+struct ec_params_regulator_get_voltage {
+	uint32_t index;
+} __ec_align4;
+
+struct ec_response_regulator_get_voltage {
+	uint32_t voltage_mv;
+} __ec_align4;
+
 /*****************************************************************************/
 /* The command range 0x200-0x2FF is reserved for Rotor. */
 
-- 
2.27.0.278.ge193c7cf3a9-goog

Re: [PATCH v5 1/2] dt-bindings: regulator: Add DT binding for cros-ec-regulator

[Enric Balletbo i Serra]

Hi Pi-Hsun,

On 11/6/20 10:25, Pi-Hsun Shih wrote:
> Add DT binding documentation for cros-ec-regulator, a voltage regulator
> controlled by ChromeOS EC.
> 
> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>

Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

> ---
> Changes from v4:
> * Change compatible name from regulator-cros-ec to cros-ec-regulator.
> 
> Changes from v3:
> * Fix dt bindings file name.
> * Add full example.
> 
> Changes from v2:
> * No change
> 
> Changes from v1:
> * Change compatible string to google,regulator-cros-ec.
> * Use reg property in device tree.
> * Change license for dt binding according to checkpatch.pl.
> ---
>  .../regulator/google,cros-ec-regulator.yaml   | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml b/Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml
> new file mode 100644
> index 000000000000..c9453d7ce227
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/google,cros-ec-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ChromeOS EC controlled voltage regulators
> +
> +maintainers:
> +  - Pi-Hsun Shih <pihsun@chromium.org>
> +
> +description:
> +  Any property defined as part of the core regulator binding, defined in
> +  regulator.yaml, can also be used.
> +
> +allOf:
> +  - $ref: "regulator.yaml#"
> +
> +properties:
> +  compatible:
> +    const: google,cros-ec-regulator
> +
> +  reg:
> +    maxItems: 1
> +    description: Identifier for the voltage regulator to ChromeOS EC.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    spi0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        cros_ec: ec@0 {
> +            compatible = "google,cros-ec-spi";
> +            reg = <0>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            regulator@0 {
> +                compatible = "google,cros-ec-regulator";
> +                regulator-min-microvolt = <1800000>;
> +                regulator-max-microvolt = <3300000>;
> +                reg = <0>;
> +            };
> +        };
> +    };
> +...
> 

Re: [PATCH v5 2/2] regulator: Add driver for cros-ec-regulator

[Enric Balletbo i Serra]

Hi Pi-Hsun,

Thank you for your patch, one last change, sorry to not request it before.

On 11/6/20 10:25, Pi-Hsun Shih wrote:
> Add driver for cros-ec-regulator, representing a voltage regulator that
> is connected and controlled by ChromeOS EC, and is controlled by kernel
> with EC host commands.
> 
> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> ---
> Changes from v4:
> * Change compatible name from regulator-cros-ec to cros-ec-regulator.
> 
> Changes from v3:
> * Remove check around CONFIG_OF.
> * Add new host commands to cros_ec_trace.
> * Use devm_kstrdup for duping regulator name.
> * Change license header and add MODULE_AUTHOR.
> * Address review comments.
> 
> Changes from v2:
> * Add 'depends on OF' to Kconfig.
> * Add Kconfig description about compiling as module.
> 
> Changes from v1:
> * Change compatible string to google,regulator-cros-ec.
> * Use reg property in device tree.
> * Address comments on code styles.
> 
> This patch contains function cros_ec_cmd that is copied from the series:
> https://lore.kernel.org/patchwork/project/lkml/list/?series=428457.
> 
> I can't find the first patch in that v2 series, so the function is
> modified from v1 of that series according to reviewers comment:
> https://lore.kernel.org/patchwork/patch/1188110/
> 
> I copied the function instead of depending on that series since I feel
> the function is small enough, and the series has stalled for some time.
> ---
>  drivers/platform/chrome/cros_ec_trace.c       |   5 +

Could you split this patch in two, one for chrome-platform that introduces the
new commands (cros_ec_commands and cros_ec_trace) and another one with the
regulator driver.

>  drivers/regulator/Kconfig                     |  10 +
>  drivers/regulator/Makefile                    |   1 +
>  drivers/regulator/cros-ec-regulator.c         | 256 ++++++++++++++++++
>  .../linux/platform_data/cros_ec_commands.h    |  82 ++++++
>  5 files changed, 354 insertions(+)
>  create mode 100644 drivers/regulator/cros-ec-regulator.c
> 
> diff --git a/drivers/platform/chrome/cros_ec_trace.c b/drivers/platform/chrome/cros_ec_trace.c
> index 523a39bd0ff6..425e9441b7ca 100644
> --- a/drivers/platform/chrome/cros_ec_trace.c
> +++ b/drivers/platform/chrome/cros_ec_trace.c
> @@ -161,6 +161,11 @@
>  	TRACE_SYMBOL(EC_CMD_ADC_READ), \
>  	TRACE_SYMBOL(EC_CMD_ROLLBACK_INFO), \
>  	TRACE_SYMBOL(EC_CMD_AP_RESET), \
> +	TRACE_SYMBOL(EC_CMD_REGULATOR_GET_INFO), \
> +	TRACE_SYMBOL(EC_CMD_REGULATOR_ENABLE), \
> +	TRACE_SYMBOL(EC_CMD_REGULATOR_IS_ENABLED), \
> +	TRACE_SYMBOL(EC_CMD_REGULATOR_SET_VOLTAGE), \
> +	TRACE_SYMBOL(EC_CMD_REGULATOR_GET_VOLTAGE), \


Also make sure this is following the order generated by the command

  sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p'  \
          include/linux/platform_data/cros_ec_commands.h

after introduce the EC commands.

With that fixed you can add:

Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

on both patches.

Thanks,
 Enric


>  	TRACE_SYMBOL(EC_CMD_CR51_BASE), \
>  	TRACE_SYMBOL(EC_CMD_CR51_LAST), \
>  	TRACE_SYMBOL(EC_CMD_FP_PASSTHRU), \
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 8f677f5d79b4..c398e90e0e73 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -238,6 +238,16 @@ config REGULATOR_CPCAP
>  	  Say y here for CPCAP regulator found on some Motorola phones
>  	  and tablets such as Droid 4.
>  
> +config REGULATOR_CROS_EC
> +	tristate "ChromeOS EC regulators"
> +	depends on CROS_EC && OF
> +	help
> +	  This driver supports voltage regulators that is connected to ChromeOS
> +	  EC and controlled through EC host commands.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called cros-ec-regulator.
> +
>  config REGULATOR_DA903X
>  	tristate "Dialog Semiconductor DA9030/DA9034 regulators"
>  	depends on PMIC_DA903X
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index e8f163371071..46592c160d22 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
>  obj-$(CONFIG_REGULATOR_88PG86X) += 88pg86x.o
>  obj-$(CONFIG_REGULATOR_88PM800) += 88pm800-regulator.o
>  obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o
> +obj-$(CONFIG_REGULATOR_CROS_EC) += cros-ec-regulator.o
>  obj-$(CONFIG_REGULATOR_CPCAP) += cpcap-regulator.o
>  obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
>  obj-$(CONFIG_REGULATOR_AB3100) += ab3100.o
> diff --git a/drivers/regulator/cros-ec-regulator.c b/drivers/regulator/cros-ec-regulator.c
> new file mode 100644
> index 000000000000..830ceefc704a
> --- /dev/null
> +++ b/drivers/regulator/cros-ec-regulator.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright 2020 Google LLC.
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/slab.h>
> +
> +struct cros_ec_regulator_data {
> +	struct regulator_desc desc;
> +	struct regulator_dev *dev;
> +	struct cros_ec_device *ec_dev;
> +
> +	u32 index;
> +
> +	u16 *voltages_mV;
> +	u16 num_voltages;
> +};
> +
> +static int cros_ec_cmd(struct cros_ec_device *ec, u32 version, u32 command,
> +		       void *outdata, u32 outsize, void *indata, u32 insize)
> +{
> +	struct cros_ec_command *msg;
> +	int ret;
> +
> +	msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	msg->version = version;
> +	msg->command = command;
> +	msg->outsize = outsize;
> +	msg->insize = insize;
> +
> +	if (outdata && outsize > 0)
> +		memcpy(msg->data, outdata, outsize);
> +
> +	ret = cros_ec_cmd_xfer_status(ec, msg);
> +	if (ret < 0)
> +		goto cleanup;
> +
> +	if (insize)
> +		memcpy(indata, msg->data, insize);
> +
> +cleanup:
> +	kfree(msg);
> +	return ret;
> +}
> +
> +static int cros_ec_regulator_enable(struct regulator_dev *dev)
> +{
> +	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
> +	struct ec_params_regulator_enable cmd = {
> +		.index = data->index,
> +		.enable = 1,
> +	};
> +
> +	return cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_ENABLE, &cmd,
> +			  sizeof(cmd), NULL, 0);
> +}
> +
> +static int cros_ec_regulator_disable(struct regulator_dev *dev)
> +{
> +	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
> +	struct ec_params_regulator_enable cmd = {
> +		.index = data->index,
> +		.enable = 0,
> +	};
> +
> +	return cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_ENABLE, &cmd,
> +			  sizeof(cmd), NULL, 0);
> +}
> +
> +static int cros_ec_regulator_is_enabled(struct regulator_dev *dev)
> +{
> +	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
> +	struct ec_params_regulator_is_enabled cmd = {
> +		.index = data->index,
> +	};
> +	struct ec_response_regulator_is_enabled resp;
> +	int ret;
> +
> +	ret = cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_IS_ENABLED, &cmd,
> +			  sizeof(cmd), &resp, sizeof(resp));
> +	if (ret < 0)
> +		return ret;
> +	return resp.enabled;
> +}
> +
> +static int cros_ec_regulator_list_voltage(struct regulator_dev *dev,
> +					  unsigned int selector)
> +{
> +	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
> +
> +	if (selector >= data->num_voltages)
> +		return -EINVAL;
> +
> +	return data->voltages_mV[selector] * 1000;
> +}
> +
> +static int cros_ec_regulator_get_voltage(struct regulator_dev *dev)
> +{
> +	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
> +	struct ec_params_regulator_get_voltage cmd = {
> +		.index = data->index,
> +	};
> +	struct ec_response_regulator_get_voltage resp;
> +	int ret;
> +
> +	ret = cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_GET_VOLTAGE, &cmd,
> +			  sizeof(cmd), &resp, sizeof(resp));
> +	if (ret < 0)
> +		return ret;
> +	return resp.voltage_mv * 1000;
> +}
> +
> +static int cros_ec_regulator_set_voltage(struct regulator_dev *dev, int min_uV,
> +					 int max_uV, unsigned int *selector)
> +{
> +	struct cros_ec_regulator_data *data = rdev_get_drvdata(dev);
> +	int min_mV = DIV_ROUND_UP(min_uV, 1000);
> +	int max_mV = max_uV / 1000;
> +	struct ec_params_regulator_set_voltage cmd = {
> +		.index = data->index,
> +		.min_mv = min_mV,
> +		.max_mv = max_mV,
> +	};
> +
> +	/*
> +	 * This can happen when the given range [min_uV, max_uV] doesn't
> +	 * contain any voltage that can be represented exactly in mV.
> +	 */
> +	if (min_mV > max_mV)
> +		return -EINVAL;
> +	return cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_SET_VOLTAGE, &cmd,
> +			   sizeof(cmd), NULL, 0);
> +}
> +
> +static struct regulator_ops cros_ec_regulator_voltage_ops = {
> +	.enable = cros_ec_regulator_enable,
> +	.disable = cros_ec_regulator_disable,
> +	.is_enabled = cros_ec_regulator_is_enabled,
> +	.list_voltage = cros_ec_regulator_list_voltage,
> +	.get_voltage = cros_ec_regulator_get_voltage,
> +	.set_voltage = cros_ec_regulator_set_voltage,
> +};
> +
> +static int cros_ec_regulator_init_info(struct device *dev,
> +				       struct cros_ec_regulator_data *data)
> +{
> +	struct ec_params_regulator_get_info cmd = {
> +		.index = data->index,
> +	};
> +	struct ec_response_regulator_get_info resp;
> +	int ret;
> +
> +	ret = cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_GET_INFO, &cmd,
> +			   sizeof(cmd), &resp, sizeof(resp));
> +	if (ret < 0)
> +		return ret;
> +
> +	data->num_voltages =
> +		min_t(u16, ARRAY_SIZE(resp.voltages_mv), resp.num_voltages);
> +	data->voltages_mV =
> +		devm_kmemdup(dev, resp.voltages_mv,
> +			     sizeof(u16) * data->num_voltages, GFP_KERNEL);
> +	data->desc.n_voltages = data->num_voltages;
> +
> +	/* Make sure the returned name is always a valid string */
> +	resp.name[sizeof(resp.name) - 1] = '\0';
> +	data->desc.name = devm_kstrdup(dev, resp.name, GFP_KERNEL);
> +	if (!data->desc.name)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static int cros_ec_regulator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct cros_ec_regulator_data *drvdata;
> +	struct regulator_init_data *init_data;
> +	struct regulator_config cfg = {};
> +	struct regulator_desc *desc;
> +	int ret;
> +
> +	drvdata = devm_kzalloc(
> +		&pdev->dev, sizeof(struct cros_ec_regulator_data), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->ec_dev = dev_get_drvdata(dev->parent);
> +	desc = &drvdata->desc;
> +
> +	init_data = of_get_regulator_init_data(dev, np, desc);
> +	if (!init_data)
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32(np, "reg", &drvdata->index);
> +	if (ret < 0)
> +		return ret;
> +
> +	desc->owner = THIS_MODULE;
> +	desc->type = REGULATOR_VOLTAGE;
> +	desc->ops = &cros_ec_regulator_voltage_ops;
> +
> +	ret = cros_ec_regulator_init_info(dev, drvdata);
> +	if (ret < 0)
> +		return ret;
> +
> +	cfg.dev = &pdev->dev;
> +	cfg.init_data = init_data;
> +	cfg.driver_data = drvdata;
> +	cfg.of_node = np;
> +
> +	drvdata->dev = regulator_register(&drvdata->desc, &cfg);
> +	if (IS_ERR(drvdata->dev)) {
> +		ret = PTR_ERR(drvdata->dev);
> +		dev_err(&pdev->dev, "Failed to register regulator: %d\n", ret);
> +		goto free_name;
> +	}
> +
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	return 0;
> +
> +free_name:
> +	kfree(desc->name);
> +	return ret;
> +}
> +
> +static const struct of_device_id regulator_cros_ec_of_match[] = {
> +	{ .compatible = "google,cros-ec-regulator", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, regulator_cros_ec_of_match);
> +
> +static struct platform_driver cros_ec_regulator_driver = {
> +	.probe		= cros_ec_regulator_probe,
> +	.driver		= {
> +		.name		= "cros-ec-regulator",
> +		.of_match_table = regulator_cros_ec_of_match,
> +	},
> +};
> +
> +module_platform_driver(cros_ec_regulator_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("ChromeOS EC controlled regulator");
> +MODULE_AUTHOR("Pi-Hsun Shih <pihsun@chromium.org>");
> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index 69210881ebac..a417b51b5764 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h
> @@ -5430,6 +5430,88 @@ struct ec_response_rollback_info {
>  /* Issue AP reset */
>  #define EC_CMD_AP_RESET 0x0125
>  
> +/*****************************************************************************/
> +/* Voltage regulator controls */
> +
> +/*
> + * Get basic info of voltage regulator for given index.
> + *
> + * Returns the regulator name and supported voltage list in mV.
> + */
> +#define EC_CMD_REGULATOR_GET_INFO 0x012B
> +
> +/* Maximum length of regulator name */
> +#define EC_REGULATOR_NAME_MAX_LEN 16
> +
> +/* Maximum length of the supported voltage list. */
> +#define EC_REGULATOR_VOLTAGE_MAX_COUNT 16
> +
> +struct ec_params_regulator_get_info {
> +	uint32_t index;
> +} __ec_align4;
> +
> +struct ec_response_regulator_get_info {
> +	char name[EC_REGULATOR_NAME_MAX_LEN];
> +	uint16_t num_voltages;
> +	uint16_t voltages_mv[EC_REGULATOR_VOLTAGE_MAX_COUNT];
> +} __ec_align1;
> +
> +/*
> + * Configure the regulator as enabled / disabled.
> + */
> +#define EC_CMD_REGULATOR_ENABLE 0x012C
> +
> +struct ec_params_regulator_enable {
> +	uint32_t index;
> +	uint8_t enable;
> +} __ec_align4;
> +
> +/*
> + * Query if the regulator is enabled.
> + *
> + * Returns 1 if the regulator is enabled, 0 if not.
> + */
> +#define EC_CMD_REGULATOR_IS_ENABLED 0x012D
> +
> +struct ec_params_regulator_is_enabled {
> +	uint32_t index;
> +} __ec_align4;
> +
> +struct ec_response_regulator_is_enabled {
> +	uint8_t enabled;
> +} __ec_align1;
> +
> +/*
> + * Set voltage for the voltage regulator within the range specified.
> + *
> + * The driver should select the voltage in range closest to min_mv.
> + *
> + * Also note that this might be called before the regulator is enabled, and the
> + * setting should be in effect after the regulator is enabled.
> + */
> +#define EC_CMD_REGULATOR_SET_VOLTAGE 0x012E
> +
> +struct ec_params_regulator_set_voltage {
> +	uint32_t index;
> +	uint32_t min_mv;
> +	uint32_t max_mv;
> +} __ec_align4;
> +
> +/*
> + * Get the currently configured voltage for the voltage regulator.
> + *
> + * Note that this might be called before the regulator is enabled.
> + */
> +#define EC_CMD_REGULATOR_GET_VOLTAGE 0x012F
> +
> +struct ec_params_regulator_get_voltage {
> +	uint32_t index;
> +} __ec_align4;
> +
> +struct ec_response_regulator_get_voltage {
> +	uint32_t voltage_mv;
> +} __ec_align4;
> +
>  /*****************************************************************************/
>  /* The command range 0x200-0x2FF is reserved for Rotor. */
>  
> 

Re: [PATCH v5 2/2] regulator: Add driver for cros-ec-regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On Thu, Jun 11, 2020 at 01:44:42PM +0200, Enric Balletbo i Serra wrote:
> On 11/6/20 10:25, Pi-Hsun Shih wrote:
> > Add driver for cros-ec-regulator, representing a voltage regulator that
> > is connected and controlled by ChromeOS EC, and is controlled by kernel
> > with EC host commands.

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 --]