[PATCH 0/4] pwm: add support for ChromeOS EC PWM

[PATCH 0/4] pwm: add support for ChromeOS EC PWM

[Brian Norris]

Hi,

This series adds support for the new ChromeOS EC PWM API, so we can control,
e.g., the backlight when it's attached to the EC. It uses Boris's latest
"atomic" hooks for the PWM API (i.e., the ->apply() callback), which were
recently merged.

It seems nice to have the cros_ec_cmd_xfer_status() helper, which we have
locally in the ChromeOS kernel, and which has been part of another larger patch
series:

https://lkml.org/lkml/2016/4/12/342

So I've picked it into this series as well. Obviously, I don't care which one
is taken, but AFAICT, Tomeu's USB PD series isn't extremely active right now.

As this touches MFD (sort of), drivers/platform/chrome/, and drivers/pwm/, I'm
not sure who it should all go through: Lee, Thierry, or Olof?

Anyway, please review.

Regards,
Brian

Brian Norris (3):
  mfd: cros_ec: add EC_PWM function definitions
  doc: dt: pwm: add binding for ChromeOS EC PWM
  pwm: add ChromeOS EC PWM driver

Tomeu Vizoso (1):
  mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

 .../devicetree/bindings/pwm/google,cros-ec-pwm.txt |  25 +++
 drivers/platform/chrome/cros_ec_proto.c            |  15 ++
 drivers/pwm/Kconfig                                |   7 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-cros-ec.c                          | 230 +++++++++++++++++++++
 include/linux/mfd/cros_ec.h                        |  18 ++
 include/linux/mfd/cros_ec_commands.h               |  31 +++
 7 files changed, 327 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
 create mode 100644 drivers/pwm/pwm-cros-ec.c

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Brian Norris]

From: Tomeu Vizoso <tomeu.vizoso@collabora.com>

So that callers of cros_ec_cmd_xfer don't have to repeat boilerplate
code when checking for errors from the EC side.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Benson Leung <bleung@chromium.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
Stolen from:
  https://lkml.org/lkml/2016/4/12/342
  https://patchwork.kernel.org/patch/8810001/

 drivers/platform/chrome/cros_ec_proto.c | 15 +++++++++++++++
 include/linux/mfd/cros_ec.h             | 18 ++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 990308ca384f..75eb90d2e4f9 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -380,3 +380,18 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 	return ret;
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer);
+
+int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
+			    struct cros_ec_command *msg)
+{
+	int ret;
+
+	ret = cros_ec_cmd_xfer(ec_dev, msg);
+	if (ret < 0)
+		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
+	else if (msg->result != EC_RES_SUCCESS)
+		return -EECRESULT - msg->result;
+
+	return ret;
+}
+EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index a677c2bd485c..b43153f1aca5 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -40,6 +40,9 @@
 #define EC_MAX_REQUEST_OVERHEAD		1
 #define EC_MAX_RESPONSE_OVERHEAD	2
 
+/* ec_command return value for non-success result from EC */
+#define EECRESULT 1000
+
 /*
  * Command interface between EC and AP, for LPC, I2C and SPI interfaces.
  */
@@ -224,6 +227,21 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 		     struct cros_ec_command *msg);
 
 /**
+ * cros_ec_cmd_xfer_status - Send a command to the ChromeOS EC
+ *
+ * This function is identical to cros_ec_cmd_xfer, except it returns succes
+ * status only if both the command was transmitted successfully and the EC
+ * replied with success status. It's not necessary to check msg->result when
+ * using this function.
+ *
+ * @ec_dev: EC device
+ * @msg: Message to write
+ * @return: Num. of bytes transferred on success, <0 on failure
+ */
+int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
+			    struct cros_ec_command *msg);
+
+/**
  * cros_ec_remove - Remove a ChromeOS EC
  *
  * Call this to deregister a ChromeOS EC, then clean up any private data.
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/4] mfd: cros_ec: add EC_PWM function definitions

[Brian Norris]

The EC_CMD_PWM_{GET,SET}_DUTY commands allow us to control a PWM that is
attached to the EC, rather than the main host SoC. The API provides
functionality-based (e.g., keyboard light, backlight) or index-based
addressing of the PWM(s). Duty cycles are represented by a 16-bit value,
where 0 maps to 0% duty cycle and U16_MAX maps to 100%. The period
cannot be controlled.

This command set is more generic than, e.g.,
EC_CMD_PWM_{GET,SET}_KEYBOARD_BACKLIGHT and could possibly used to
replace it on future products.

Let's update the command header to include the definitions.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 include/linux/mfd/cros_ec_commands.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 13b630c10d4c..d673575e0ada 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -949,6 +949,37 @@ struct ec_params_pwm_set_fan_duty {
 	uint32_t percent;
 } __packed;
 
+#define EC_CMD_PWM_SET_DUTY 0x25
+/* 16 bit duty cycle, 65535 = 100% */
+#define EC_PWM_MAX_DUTY 65535
+
+enum ec_pwm_type {
+	/* All types, indexed by board-specific enum pwm_channel */
+	EC_PWM_TYPE_GENERIC = 0,
+	/* Keyboard backlight */
+	EC_PWM_TYPE_KB_LIGHT,
+	/* Display backlight */
+	EC_PWM_TYPE_DISPLAY_LIGHT,
+	EC_PWM_TYPE_COUNT,
+};
+
+struct ec_params_pwm_set_duty {
+	uint16_t duty;     /* Duty cycle, EC_PWM_MAX_DUTY = 100% */
+	uint8_t pwm_type;  /* ec_pwm_type */
+	uint8_t index;     /* Type-specific index, or 0 if unique */
+} __packed;
+
+#define EC_CMD_PWM_GET_DUTY 0x26
+
+struct ec_params_pwm_get_duty {
+	uint8_t pwm_type;  /* ec_pwm_type */
+	uint8_t index;     /* Type-specific index, or 0 if unique */
+} __packed;
+
+struct ec_response_pwm_get_duty {
+	uint16_t duty;     /* Duty cycle, EC_PWM_MAX_DUTY = 100% */
+} __packed;
+
 /*****************************************************************************/
 /*
  * Lightbar commands. This looks worse than it is. Since we only use one HOST
-- 
2.8.0.rc3.226.g39d4020

[PATCH 3/4] doc: dt: pwm: add binding for ChromeOS EC PWM

[Brian Norris]

The ChromeOS Embedded Controller can support controlling its attached
PWMs via its host-command interface. The number of supported PWMs varies
on a per-board basis, so we define a "google,max-pwms" property to
handle this. And because the EC only allows specifying the duty cycle
and not the period, we don't specify the period via pwm-cells, and
instead have only support 1 cell -- to specify the index.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 .../devicetree/bindings/pwm/google,cros-ec-pwm.txt | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
new file mode 100644
index 000000000000..f1c9540fc23f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
@@ -0,0 +1,25 @@
+* PWM controlled by ChromeOS EC
+
+Google's ChromeOS EC PWM is a simple PWM attached to the Embedded Controller
+(EC) and controlled via a host-command interface.
+
+An EC PWM node should be only found as a sub-node of the EC node (see
+Documentation/devicetree/bindings/mfd/cros-ec.txt).
+
+Required properties:
+- compatible: Must contain "google,cros-ec-pwm"
+- #pwm-cells: Should be 1. The cell specifies the PWM index.
+- google,max-pwms: Specifies the number of PWMs supported by the EC.
+
+Example:
+	cros-ec@0 {
+		compatible = "google,cros-ec-spi";
+
+		...
+
+		cros_ec_pwm: ec-pwm {
+			compatible = "google,cros-ec-pwm";
+			#pwm-cells = <1>;
+			google,max-pwms = <4>;
+		};
+	};
-- 
2.8.0.rc3.226.g39d4020

[PATCH 4/4] pwm: add ChromeOS EC PWM driver

[Brian Norris]

Use the new ChromeOS EC EC_CMD_PWM_{GET,SET}_DUTY commands to control
one or more PWMs attached to the Embedded Controller. Because the EC
allows us to modify the duty cycle (as a percentage, where U16_MAX is
100%) but not the period, we assign the period a fixed value of
EC_PWM_MAX_DUTY and reject all attempts to change it.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/pwm/Kconfig       |   7 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-cros-ec.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 238 insertions(+)
 create mode 100644 drivers/pwm/pwm-cros-ec.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index c182efc62c7b..4f2b16a50f42 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -137,6 +137,13 @@ config PWM_CRC
 	  Generic PWM framework driver for Crystalcove (CRC) PMIC based PWM
 	  control.
 
+config PWM_CROS_EC
+	tristate "ChromeOS EC PWM driver"
+	depends on MFD_CROS_EC
+	help
+	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
+	  Controller.
+
 config PWM_EP93XX
 	tristate "Cirrus Logic EP93xx PWM support"
 	depends on ARCH_EP93XX
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index dd35bc121a18..ffde923cf3df 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_BRCMSTB)	+= pwm-brcmstb.o
 obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
+obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
 obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
new file mode 100644
index 000000000000..cf65e44fe355
--- /dev/null
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -0,0 +1,230 @@
+/*
+ * Copyright (C) 2016 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2, as published by
+ * the Free Software Foundation.
+ *
+ * Expose a PWM controlled by the ChromeOS EC to the host processor.
+ */
+
+#include <linux/module.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+/**
+ * struct cros_ec_pwm_device - Driver data for EC PWM
+ *
+ * @dev: Device node
+ * @ec: Pointer to EC device
+ * @chip: PWM controller chip
+ */
+struct cros_ec_pwm_device {
+	struct device *dev;
+	struct cros_ec_device *ec;
+	struct pwm_chip chip;
+};
+
+static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
+{
+	return container_of(c, struct cros_ec_pwm_device, chip);
+}
+
+static int cros_ec_pwm_set_duty(struct cros_ec_pwm_device *ec_pwm,
+				   struct pwm_device *pwm,
+				   uint16_t duty)
+{
+	struct cros_ec_device *ec = ec_pwm->ec;
+	struct ec_params_pwm_set_duty *params;
+	struct cros_ec_command *msg;
+	int ret;
+
+	msg = kzalloc(sizeof(*msg) + sizeof(*params), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+	params = (void *)&msg->data[0];
+
+	msg->version = 0;
+	msg->command = EC_CMD_PWM_SET_DUTY;
+	msg->insize = 0;
+	msg->outsize = sizeof(*params);
+
+	params->duty = duty;
+	params->pwm_type = EC_PWM_TYPE_GENERIC;
+	params->index = pwm->hwpwm;
+
+	ret = cros_ec_cmd_xfer_status(ec, msg);
+	kfree(msg);
+	return ret;
+}
+
+static int cros_ec_pwm_get_duty(struct cros_ec_pwm_device *ec_pwm,
+				struct pwm_device *pwm)
+{
+	struct cros_ec_device *ec = ec_pwm->ec;
+	struct ec_params_pwm_get_duty *params;
+	struct ec_response_pwm_get_duty *resp;
+	struct cros_ec_command *msg;
+	int ret;
+
+	msg = kzalloc(sizeof(*msg) + max(sizeof(*params), sizeof(*resp)),
+			GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+	params = (void *)&msg->data[0];
+	resp = (void *)&msg->data[0];
+
+	msg->version = 0;
+	msg->command = EC_CMD_PWM_GET_DUTY;
+	msg->insize = sizeof(*params);
+	msg->outsize = sizeof(*resp);
+
+	params->pwm_type = EC_PWM_TYPE_GENERIC;
+	params->index = pwm->hwpwm;
+
+	ret = cros_ec_cmd_xfer_status(ec, msg);
+	if (ret < 0)
+		goto out;
+
+	ret = resp->duty;
+
+out:
+	kfree(msg);
+	return ret;
+}
+
+static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     struct pwm_state *state)
+{
+	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
+
+	/* The EC won't let us change the period */
+	if (state->period != EC_PWM_MAX_DUTY)
+		return -EINVAL;
+
+	return cros_ec_pwm_set_duty(ec_pwm, pwm, state->duty_cycle);
+}
+
+static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
+	int ret;
+
+	ret = cros_ec_pwm_get_duty(ec_pwm, pwm);
+	if (ret < 0) {
+		dev_err(chip->dev, "error getting initial duty: %d\n", ret);
+		return;
+	}
+
+	state->enabled = (ret > 0);
+	state->period = EC_PWM_MAX_DUTY;
+	state->duty_cycle = ret;
+}
+
+static struct pwm_device *
+cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	if (args->args[0] >= pc->npwm)
+		return ERR_PTR(-EINVAL);
+
+	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	/* The EC won't let us change the period */
+	pwm->args.period = EC_PWM_MAX_DUTY;
+
+	return pwm;
+}
+
+static const struct pwm_ops cros_ec_pwm_ops = {
+	.get_state	= cros_ec_pwm_get_state,
+	.apply		= cros_ec_pwm_apply,
+	.owner		= THIS_MODULE,
+};
+
+static int cros_ec_pwm_probe(struct platform_device *pdev)
+{
+	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct cros_ec_pwm_device *ec_pwm;
+	struct pwm_chip *chip;
+	u32 val;
+	int ret;
+
+	if (!ec) {
+		dev_err(dev, "no parent EC device\n");
+		return -EINVAL;
+	}
+
+	ec_pwm = devm_kzalloc(dev, sizeof(*ec_pwm), GFP_KERNEL);
+	if (!ec_pwm)
+		return -ENOMEM;
+	chip = &ec_pwm->chip;
+	ec_pwm->ec = ec;
+
+	/* PWM chip */
+	chip->dev = dev;
+	chip->ops = &cros_ec_pwm_ops;
+	chip->of_xlate = cros_ec_pwm_xlate;
+	chip->of_pwm_n_cells = 1;
+	chip->base = -1;
+	ret = of_property_read_u32(np, "google,max-pwms", &val);
+	if (ret) {
+		dev_err(dev, "Couldn't read max-pwms property: %d\n", ret);
+		return ret;
+	}
+	/* The index field is only 8 bits */
+	if (val > U8_MAX) {
+		dev_err(dev, "Can't support %u PWMs\n", val);
+		return -EINVAL;
+	}
+	chip->npwm = val;
+
+	ret = pwmchip_add(chip);
+	if (ret < 0) {
+		dev_err(dev, "cannot register PWM: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, ec_pwm);
+
+	return ret;
+}
+
+static int cros_ec_pwm_remove(struct platform_device *dev)
+{
+	struct cros_ec_pwm_device *ec_pwm = platform_get_drvdata(dev);
+	struct pwm_chip *chip = &ec_pwm->chip;
+
+	return pwmchip_remove(chip);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id cros_ec_pwm_of_match[] = {
+	{ .compatible = "google,cros-ec-pwm" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cros_ec_pwm_of_match);
+#endif
+
+static struct platform_driver cros_ec_pwm_driver = {
+	.probe = cros_ec_pwm_probe,
+	.remove = cros_ec_pwm_remove,
+	.driver = {
+		.name = "cros-ec-pwm",
+		.of_match_table = of_match_ptr(cros_ec_pwm_of_match),
+	},
+};
+module_platform_driver(cros_ec_pwm_driver);
+
+MODULE_ALIAS("platform:cros-ec-pwm");
+MODULE_DESCRIPTION("ChromeOS EC PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 3/4] doc: dt: pwm: add binding for ChromeOS EC PWM

[Gwendal Grignou]

Instead of using device tree, assuming you have firmware control,
another way could be to add a firmware feature:
for instance, there is one EC_FEATURE_PWM_FAN, the fan PWM, one for
the keyboard lightning as well. (see num ec_feature_code)
By adding one more, you let cros_ec_dev load the platform driver for
you, it works even if the machine does not use device tree.

Gwendal.

On Fri, May 27, 2016 at 6:39 PM, Brian Norris <briannorris@chromium.org> wrote:
> The ChromeOS Embedded Controller can support controlling its attached
> PWMs via its host-command interface. The number of supported PWMs varies
> on a per-board basis, so we define a "google,max-pwms" property to
> handle this. And because the EC only allows specifying the duty cycle
> and not the period, we don't specify the period via pwm-cells, and
> instead have only support 1 cell -- to specify the index.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  .../devicetree/bindings/pwm/google,cros-ec-pwm.txt | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
> new file mode 100644
> index 000000000000..f1c9540fc23f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
> @@ -0,0 +1,25 @@
> +* PWM controlled by ChromeOS EC
> +
> +Google's ChromeOS EC PWM is a simple PWM attached to the Embedded Controller
> +(EC) and controlled via a host-command interface.
> +
> +An EC PWM node should be only found as a sub-node of the EC node (see
> +Documentation/devicetree/bindings/mfd/cros-ec.txt).
> +
> +Required properties:
> +- compatible: Must contain "google,cros-ec-pwm"
> +- #pwm-cells: Should be 1. The cell specifies the PWM index.
> +- google,max-pwms: Specifies the number of PWMs supported by the EC.
> +
> +Example:
> +       cros-ec@0 {
> +               compatible = "google,cros-ec-spi";
> +
> +               ...
> +
> +               cros_ec_pwm: ec-pwm {
> +                       compatible = "google,cros-ec-pwm";
> +                       #pwm-cells = <1>;
> +                       google,max-pwms = <4>;
> +               };
> +       };
> --
> 2.8.0.rc3.226.g39d4020
>

Re: [PATCH 4/4] pwm: add ChromeOS EC PWM driver

[Gwendal Grignou]

On Fri, May 27, 2016 at 6:39 PM, Brian Norris <briannorris@chromium.org> wrote:
> Use the new ChromeOS EC EC_CMD_PWM_{GET,SET}_DUTY commands to control
> one or more PWMs attached to the Embedded Controller. Because the EC
> allows us to modify the duty cycle (as a percentage, where U16_MAX is
> 100%) but not the period, we assign the period a fixed value of
> EC_PWM_MAX_DUTY and reject all attempts to change it.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---

> + */
> +struct cros_ec_pwm_device {
> +       struct device *dev;
> +       struct cros_ec_device *ec;
> +       struct pwm_chip chip;
> +};
> +
> +static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
> +{
> +       return container_of(c, struct cros_ec_pwm_device, chip);
> +}
> +
> +static int cros_ec_pwm_set_duty(struct cros_ec_pwm_device *ec_pwm,
> +                                  struct pwm_device *pwm,
> +                                  uint16_t duty)
Given you seprated the pwm stuff from the EC stuff and focusing on
sending a EC command here, the first parameter should be of
cros_ec_device* instead of cros_ec_pwm_device*.
> +{
> +       struct cros_ec_device *ec = ec_pwm->ec;
> +       struct ec_params_pwm_set_duty *params;
> +       struct cros_ec_command *msg;
> +       int ret;
> +
> +       msg = kzalloc(sizeof(*msg) + sizeof(*params), GFP_KERNEL);
Use an ad-hoc data structure on the stack, so you will always be able
to send the command to the EC.
> +       if (!msg)
> +               return -ENOMEM;
> +       params = (void *)&msg->data[0];
> +
> +       msg->version = 0;
> +       msg->command = EC_CMD_PWM_SET_DUTY;
> +       msg->insize = 0;
> +       msg->outsize = sizeof(*params);
> +
> +       params->duty = duty;
> +       params->pwm_type = EC_PWM_TYPE_GENERIC;
> +       params->index = pwm->hwpwm;
> +
> +       ret = cros_ec_cmd_xfer_status(ec, msg);
> +       kfree(msg);
> +       return ret;
> +}
> +
> +static int cros_ec_pwm_get_duty(struct cros_ec_pwm_device *ec_pwm,
> +                               struct pwm_device *pwm)
Idem.
> +{
> +       struct cros_ec_device *ec = ec_pwm->ec;
> +       struct ec_params_pwm_get_duty *params;
> +       struct ec_response_pwm_get_duty *resp;
> +       struct cros_ec_command *msg;
> +       int ret;
> +
> +       msg = kzalloc(sizeof(*msg) + max(sizeof(*params), sizeof(*resp)),
Idem.
> +                       GFP_KERNEL);
> +       if (!msg)
> +               return -ENOMEM;
> +       params = (void *)&msg->data[0];
> +       resp = (void *)&msg->data[0];
> +
> +       msg->version = 0;
> +       msg->command = EC_CMD_PWM_GET_DUTY;
> +       msg->insize = sizeof(*params);
> +       msg->outsize = sizeof(*resp);
> +
> +       params->pwm_type = EC_PWM_TYPE_GENERIC;
> +       params->index = pwm->hwpwm;
> +
> +       ret = cros_ec_cmd_xfer_status(ec, msg);
> +       if (ret < 0)
> +               goto out;
> +
> +       ret = resp->duty;
> +
> +out:
> +       kfree(msg);
> +       return ret;
> +}
> +
> +static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +                            struct pwm_state *state)
> +{
> +       struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> +
> +       /* The EC won't let us change the period */
> +       if (state->period != EC_PWM_MAX_DUTY)
> +               return -EINVAL;
> +
> +       return cros_ec_pwm_set_duty(ec_pwm, pwm, state->duty_cycle);
I would use ec_pwm->ec here.
> +}
> +
> +static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +                                 struct pwm_state *state)
> +{
> +       struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> +       int ret;
> +
> +       ret = cros_ec_pwm_get_duty(ec_pwm, pwm);
> +       if (ret < 0) {
> +               dev_err(chip->dev, "error getting initial duty: %d\n", ret);
> +               return;
> +       }
> +
> +       state->enabled = (ret > 0);
> +       state->period = EC_PWM_MAX_DUTY;
> +       state->duty_cycle = ret;
> +}
> +
> +static struct pwm_device *
> +cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
> +{
> +       struct pwm_device *pwm;
> +
> +       if (args->args[0] >= pc->npwm)
> +               return ERR_PTR(-EINVAL);
> +
> +       pwm = pwm_request_from_chip(pc, args->args[0], NULL);
> +       if (IS_ERR(pwm))
> +               return pwm;
> +
> +       /* The EC won't let us change the period */
> +       pwm->args.period = EC_PWM_MAX_DUTY;
> +
> +       return pwm;
> +}
> +
> +static const struct pwm_ops cros_ec_pwm_ops = {
> +       .get_state      = cros_ec_pwm_get_state,
> +       .apply          = cros_ec_pwm_apply,
> +       .owner          = THIS_MODULE,
> +};
> +
> +static int cros_ec_pwm_probe(struct platform_device *pdev)
> +{
> +       struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct cros_ec_pwm_device *ec_pwm;
> +       struct pwm_chip *chip;
> +       u32 val;
> +       int ret;
> +
> +       if (!ec) {
> +               dev_err(dev, "no parent EC device\n");
> +               return -EINVAL;
> +       }
> +
> +       ec_pwm = devm_kzalloc(dev, sizeof(*ec_pwm), GFP_KERNEL);
> +       if (!ec_pwm)
> +               return -ENOMEM;
> +       chip = &ec_pwm->chip;
> +       ec_pwm->ec = ec;
> +
> +       /* PWM chip */
> +       chip->dev = dev;
> +       chip->ops = &cros_ec_pwm_ops;
> +       chip->of_xlate = cros_ec_pwm_xlate;
> +       chip->of_pwm_n_cells = 1;
> +       chip->base = -1;
> +       ret = of_property_read_u32(np, "google,max-pwms", &val);
> +       if (ret) {
> +               dev_err(dev, "Couldn't read max-pwms property: %d\n", ret);
Does it mean this driver does not work when device tree is not used by
the platform?
The rest of the driver still compiles.
> +               return ret;
> +       }
> +       /* The index field is only 8 bits */
> +       if (val > U8_MAX) {
> +               dev_err(dev, "Can't support %u PWMs\n", val);
> +               return -EINVAL;
> +       }
> +       chip->npwm = val;
> +
> +       ret = pwmchip_add(chip);
> +       if (ret < 0) {
> +               dev_err(dev, "cannot register PWM: %d\n", ret);
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, ec_pwm);
> +
> +       return ret;
> +}
> +
> +static int cros_ec_pwm_remove(struct platform_device *dev)
> +{
> +       struct cros_ec_pwm_device *ec_pwm = platform_get_drvdata(dev);
> +       struct pwm_chip *chip = &ec_pwm->chip;
> +
> +       return pwmchip_remove(chip);
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id cros_ec_pwm_of_match[] = {
> +       { .compatible = "google,cros-ec-pwm" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, cros_ec_pwm_of_match);
> +#endif
> +
> +static struct platform_driver cros_ec_pwm_driver = {
> +       .probe = cros_ec_pwm_probe,
> +       .remove = cros_ec_pwm_remove,
> +       .driver = {
> +               .name = "cros-ec-pwm",
> +               .of_match_table = of_match_ptr(cros_ec_pwm_of_match),
> +       },
> +};
> +module_platform_driver(cros_ec_pwm_driver);
> +
> +MODULE_ALIAS("platform:cros-ec-pwm");
> +MODULE_DESCRIPTION("ChromeOS EC PWM driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.8.0.rc3.226.g39d4020
>

Re: [PATCH 0/4] pwm: add support for ChromeOS EC PWM

[Tomeu Vizoso]

On 05/28/2016 03:39 AM, Brian Norris wrote:
> Hi,
> 
> This series adds support for the new ChromeOS EC PWM API, so we can control,
> e.g., the backlight when it's attached to the EC. It uses Boris's latest
> "atomic" hooks for the PWM API (i.e., the ->apply() callback), which were
> recently merged.
> 
> It seems nice to have the cros_ec_cmd_xfer_status() helper, which we have
> locally in the ChromeOS kernel, and which has been part of another larger patch
> series:
> 
> https://lkml.org/lkml/2016/4/12/342
> 
> So I've picked it into this series as well. Obviously, I don't care which one
> is taken, but AFAICT, Tomeu's USB PD series isn't extremely active right now.

Hi Brian,

that's on hold during the ongoing discussion about type-c userspace API,
because right now the value of that patchset is on how userspace can
decide from what port to charge (if any), and it was badly abusing a
power supply property for that.

Regards,

Tomeu

> As this touches MFD (sort of), drivers/platform/chrome/, and drivers/pwm/, I'm
> not sure who it should all go through: Lee, Thierry, or Olof?
> 
> Anyway, please review.
> 
> Regards,
> Brian
> 
> Brian Norris (3):
>   mfd: cros_ec: add EC_PWM function definitions
>   doc: dt: pwm: add binding for ChromeOS EC PWM
>   pwm: add ChromeOS EC PWM driver
> 
> Tomeu Vizoso (1):
>   mfd: cros_ec: Add cros_ec_cmd_xfer_status helper
> 
>  .../devicetree/bindings/pwm/google,cros-ec-pwm.txt |  25 +++
>  drivers/platform/chrome/cros_ec_proto.c            |  15 ++
>  drivers/pwm/Kconfig                                |   7 +
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/pwm-cros-ec.c                          | 230 +++++++++++++++++++++
>  include/linux/mfd/cros_ec.h                        |  18 ++
>  include/linux/mfd/cros_ec_commands.h               |  31 +++
>  7 files changed, 327 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
>  create mode 100644 drivers/pwm/pwm-cros-ec.c
> 

Re: [PATCH 4/4] pwm: add ChromeOS EC PWM driver

[Brian Norris]

Hi Gwendal,

Thanks for the review.

On Sat, May 28, 2016 at 10:02:33PM -0700, Gwendal Grignou wrote:
> On Fri, May 27, 2016 at 6:39 PM, Brian Norris <briannorris@chromium.org> wrote:
> > Use the new ChromeOS EC EC_CMD_PWM_{GET,SET}_DUTY commands to control
> > one or more PWMs attached to the Embedded Controller. Because the EC
> > allows us to modify the duty cycle (as a percentage, where U16_MAX is
> > 100%) but not the period, we assign the period a fixed value of
> > EC_PWM_MAX_DUTY and reject all attempts to change it.
> >
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> 
> > + */
> > +struct cros_ec_pwm_device {
> > +       struct device *dev;
> > +       struct cros_ec_device *ec;
> > +       struct pwm_chip chip;
> > +};
> > +
> > +static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
> > +{
> > +       return container_of(c, struct cros_ec_pwm_device, chip);
> > +}
> > +
> > +static int cros_ec_pwm_set_duty(struct cros_ec_pwm_device *ec_pwm,
> > +                                  struct pwm_device *pwm,
> > +                                  uint16_t duty)
> Given you seprated the pwm stuff from the EC stuff and focusing on
> sending a EC command here, the first parameter should be of
> cros_ec_device* instead of cros_ec_pwm_device*.

Good idea, done. I'll also change the 'pwm_device' arg into just a u8
index, since that's all we care about at this level of abstraction.

> > +{
> > +       struct cros_ec_device *ec = ec_pwm->ec;
> > +       struct ec_params_pwm_set_duty *params;
> > +       struct cros_ec_command *msg;
> > +       int ret;
> > +
> > +       msg = kzalloc(sizeof(*msg) + sizeof(*params), GFP_KERNEL);
> Use an ad-hoc data structure on the stack, so you will always be able
> to send the command to the EC.

Sure, can do. I guess an anonymous struct will do well here.

> > +       if (!msg)
> > +               return -ENOMEM;
> > +       params = (void *)&msg->data[0];
> > +
> > +       msg->version = 0;
> > +       msg->command = EC_CMD_PWM_SET_DUTY;
> > +       msg->insize = 0;
> > +       msg->outsize = sizeof(*params);
> > +
> > +       params->duty = duty;
> > +       params->pwm_type = EC_PWM_TYPE_GENERIC;
> > +       params->index = pwm->hwpwm;
> > +
> > +       ret = cros_ec_cmd_xfer_status(ec, msg);
> > +       kfree(msg);
> > +       return ret;
> > +}
> > +
> > +static int cros_ec_pwm_get_duty(struct cros_ec_pwm_device *ec_pwm,
> > +                               struct pwm_device *pwm)
> Idem.

Sure.

> > +{
> > +       struct cros_ec_device *ec = ec_pwm->ec;
> > +       struct ec_params_pwm_get_duty *params;
> > +       struct ec_response_pwm_get_duty *resp;
> > +       struct cros_ec_command *msg;
> > +       int ret;
> > +
> > +       msg = kzalloc(sizeof(*msg) + max(sizeof(*params), sizeof(*resp)),
> Idem.

Will do. Here, I guess an anonymous struct containing a union of
ec_{params,response}_pwm_get_duty will do it.

> > +                       GFP_KERNEL);
> > +       if (!msg)
> > +               return -ENOMEM;
> > +       params = (void *)&msg->data[0];
> > +       resp = (void *)&msg->data[0];
> > +
> > +       msg->version = 0;
> > +       msg->command = EC_CMD_PWM_GET_DUTY;
> > +       msg->insize = sizeof(*params);
> > +       msg->outsize = sizeof(*resp);
> > +
> > +       params->pwm_type = EC_PWM_TYPE_GENERIC;
> > +       params->index = pwm->hwpwm;
> > +
> > +       ret = cros_ec_cmd_xfer_status(ec, msg);
> > +       if (ret < 0)
> > +               goto out;
> > +
> > +       ret = resp->duty;
> > +
> > +out:
> > +       kfree(msg);
> > +       return ret;
> > +}
> > +
> > +static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                            struct pwm_state *state)
> > +{
> > +       struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> > +
> > +       /* The EC won't let us change the period */
> > +       if (state->period != EC_PWM_MAX_DUTY)
> > +               return -EINVAL;
> > +
> > +       return cros_ec_pwm_set_duty(ec_pwm, pwm, state->duty_cycle);
> I would use ec_pwm->ec here.

Sure.

> > +}
> > +
> > +static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                                 struct pwm_state *state)
> > +{
> > +       struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> > +       int ret;
> > +
> > +       ret = cros_ec_pwm_get_duty(ec_pwm, pwm);
> > +       if (ret < 0) {
> > +               dev_err(chip->dev, "error getting initial duty: %d\n", ret);
> > +               return;
> > +       }
> > +
> > +       state->enabled = (ret > 0);
> > +       state->period = EC_PWM_MAX_DUTY;
> > +       state->duty_cycle = ret;
> > +}
> > +
> > +static struct pwm_device *
> > +cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
> > +{
> > +       struct pwm_device *pwm;
> > +
> > +       if (args->args[0] >= pc->npwm)
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       pwm = pwm_request_from_chip(pc, args->args[0], NULL);
> > +       if (IS_ERR(pwm))
> > +               return pwm;
> > +
> > +       /* The EC won't let us change the period */
> > +       pwm->args.period = EC_PWM_MAX_DUTY;
> > +
> > +       return pwm;
> > +}
> > +
> > +static const struct pwm_ops cros_ec_pwm_ops = {
> > +       .get_state      = cros_ec_pwm_get_state,
> > +       .apply          = cros_ec_pwm_apply,
> > +       .owner          = THIS_MODULE,
> > +};
> > +
> > +static int cros_ec_pwm_probe(struct platform_device *pdev)
> > +{
> > +       struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *np = dev->of_node;
> > +       struct cros_ec_pwm_device *ec_pwm;
> > +       struct pwm_chip *chip;
> > +       u32 val;
> > +       int ret;
> > +
> > +       if (!ec) {
> > +               dev_err(dev, "no parent EC device\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       ec_pwm = devm_kzalloc(dev, sizeof(*ec_pwm), GFP_KERNEL);
> > +       if (!ec_pwm)
> > +               return -ENOMEM;
> > +       chip = &ec_pwm->chip;
> > +       ec_pwm->ec = ec;
> > +
> > +       /* PWM chip */
> > +       chip->dev = dev;
> > +       chip->ops = &cros_ec_pwm_ops;
> > +       chip->of_xlate = cros_ec_pwm_xlate;
> > +       chip->of_pwm_n_cells = 1;
> > +       chip->base = -1;
> > +       ret = of_property_read_u32(np, "google,max-pwms", &val);
> > +       if (ret) {
> > +               dev_err(dev, "Couldn't read max-pwms property: %d\n", ret);
> Does it mean this driver does not work when device tree is not used by
> the platform?
> The rest of the driver still compiles.

Correct. I think that's just how many OF-based drivers tend to work;
they might *compile* with !CONFIG_OF, but all the useful functions will
return errors, and the probe will fail quickly. Anyway, for this
particular case (max-pwms), I think we've figured out we can possibly
discover this dynamically, so I might drop this property.

The bigger problem here is that we're doing PWM device
instantiation/matching through the use of OF-based translation (see the
->of_xlate and ->of_pwm_n_cells above). So if you're thinking about
using this driver as-is on non-DT platforms, you aren't going to get
very far :) Apparently there is some provision for this (see struct
pwm_lookup / PWM_LOOKUP()), but I haven't really analyzed how we could
adapt this for a cros_ec, non-DT system.

Brian

> > +               return ret;
> > +       }
> > +       /* The index field is only 8 bits */
> > +       if (val > U8_MAX) {
> > +               dev_err(dev, "Can't support %u PWMs\n", val);
> > +               return -EINVAL;
> > +       }
> > +       chip->npwm = val;
> > +
> > +       ret = pwmchip_add(chip);
> > +       if (ret < 0) {
> > +               dev_err(dev, "cannot register PWM: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, ec_pwm);
> > +
> > +       return ret;
> > +}
> > +
> > +static int cros_ec_pwm_remove(struct platform_device *dev)
> > +{
> > +       struct cros_ec_pwm_device *ec_pwm = platform_get_drvdata(dev);
> > +       struct pwm_chip *chip = &ec_pwm->chip;
> > +
> > +       return pwmchip_remove(chip);
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id cros_ec_pwm_of_match[] = {
> > +       { .compatible = "google,cros-ec-pwm" },
> > +       {},
> > +};
> > +MODULE_DEVICE_TABLE(of, cros_ec_pwm_of_match);
> > +#endif
> > +
> > +static struct platform_driver cros_ec_pwm_driver = {
> > +       .probe = cros_ec_pwm_probe,
> > +       .remove = cros_ec_pwm_remove,
> > +       .driver = {
> > +               .name = "cros-ec-pwm",
> > +               .of_match_table = of_match_ptr(cros_ec_pwm_of_match),
> > +       },
> > +};
> > +module_platform_driver(cros_ec_pwm_driver);
> > +
> > +MODULE_ALIAS("platform:cros-ec-pwm");
> > +MODULE_DESCRIPTION("ChromeOS EC PWM driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.8.0.rc3.226.g39d4020
> >

Re: [PATCH 3/4] doc: dt: pwm: add binding for ChromeOS EC PWM

[Brian Norris]

Hi Gwendal,

On Sat, May 28, 2016 at 10:00:45PM -0700, Gwendal Grignou wrote:

(Top posting?)

> Instead of using device tree, assuming you have firmware control,
> another way could be to add a firmware feature:

I do have firmware control, but I don't think that will be too necessary
actually.

> for instance, there is one EC_FEATURE_PWM_FAN, the fan PWM, one for
> the keyboard lightning as well. (see num ec_feature_code)
> By adding one more, you let cros_ec_dev load the platform driver for
> you, it works even if the machine does not use device tree.

I think we can actually get this without doing the EC_FEATURE_* thing
(which notably is not in upstream, BTW), nor by requiring a separate
node with the "google,cros-ec-pwm" property, but instead by running a
sample EC_CMD_PWM_GET_DUTY command on indeces [0, 255], stopping at the
first INVAL_PARAM failure (if we stop at 0, then we have no PWM API at
all).

But that still leaves the problem of mapping PWMs to consumer devices.
The phandle translation is very helpful for our DT-based systems, but
there isn't a really nice equivalent for non-DT ones. I see struct
pwm_lookup, which looks like it could do some of what we want, but we'd
still either need to encode a ton of board-specific information in the
kernel, or else start exposing PWMs via the non-EC_PWM_TYPE_GENERIC
methods (see the new enum ec_pwm_type, where we can see
EC_PWM_TYPE_KB_LIGHT and EC_PWM_TYPE_DISPLAY_LIGHT).

Anyway, along this line, perhaps it makes sense to:

 (a) drop the "google,cros-ec-pwm" property (via the probe method I
 described above)
 (b) drop the separate node for "google,cros-ec-pwm", since the presence
 of this feature can be detected by the same methods as in (a)

leaving the only DT binding change to be to:

 (c) add an optional #pwm-cells property to the cros-ec node
 (Documentation/devicetree/bindings/mfd/cros-ec.txt) so that we can
 still utilize the nice PWM of_xlate stuff (and its corresponding pwms =
 <...> property for consumer devices)

This would set us up for a minimal reliance on device tree (we can try
to expose EC_PWM_TYPE_KB_LIGHT or EC_PWM_TYPE_DISPLAY_LIGHT via the
pwm_lookup infrastructure, once we need to support a non-DT system),
without losing much of its benefits (we can still do index-based /
phandle lookups with DT). The remaining question is: where should this
minimal PWM driver go, then? We would want to make calls to it from the
cros_ec MFD/platform driver, so...
drivers/platform/chrome/cros_ec_dev.c? Or more likely a modularized
drivers/platform/chrome/cros_ec_pwm.c, where cros_ec_dev.c can make a
few calls to it?

Brian

> Gwendal.
> 
> On Fri, May 27, 2016 at 6:39 PM, Brian Norris <briannorris@chromium.org> wrote:
> > The ChromeOS Embedded Controller can support controlling its attached
> > PWMs via its host-command interface. The number of supported PWMs varies
> > on a per-board basis, so we define a "google,max-pwms" property to
> > handle this. And because the EC only allows specifying the duty cycle
> > and not the period, we don't specify the period via pwm-cells, and
> > instead have only support 1 cell -- to specify the index.
> >
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> >  .../devicetree/bindings/pwm/google,cros-ec-pwm.txt | 25 ++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
> > new file mode 100644
> > index 000000000000..f1c9540fc23f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
> > @@ -0,0 +1,25 @@
> > +* PWM controlled by ChromeOS EC
> > +
> > +Google's ChromeOS EC PWM is a simple PWM attached to the Embedded Controller
> > +(EC) and controlled via a host-command interface.
> > +
> > +An EC PWM node should be only found as a sub-node of the EC node (see
> > +Documentation/devicetree/bindings/mfd/cros-ec.txt).
> > +
> > +Required properties:
> > +- compatible: Must contain "google,cros-ec-pwm"
> > +- #pwm-cells: Should be 1. The cell specifies the PWM index.
> > +- google,max-pwms: Specifies the number of PWMs supported by the EC.
> > +
> > +Example:
> > +       cros-ec@0 {
> > +               compatible = "google,cros-ec-spi";
> > +
> > +               ...
> > +
> > +               cros_ec_pwm: ec-pwm {
> > +                       compatible = "google,cros-ec-pwm";
> > +                       #pwm-cells = <1>;
> > +                       google,max-pwms = <4>;
> > +               };
> > +       };
> > --
> > 2.8.0.rc3.226.g39d4020
> >

Re: [PATCH 3/4] doc: dt: pwm: add binding for ChromeOS EC PWM

[Brian Norris]

Hi,

After some more thought, a small change in plans.

On Tue, May 31, 2016 at 06:10:59PM -0700, Brian Norris wrote:
> On Sat, May 28, 2016 at 10:00:45PM -0700, Gwendal Grignou wrote:
> > Instead of using device tree, assuming you have firmware control,
> > another way could be to add a firmware feature:
> 
> I do have firmware control, but I don't think that will be too necessary
> actually.
> 
> > for instance, there is one EC_FEATURE_PWM_FAN, the fan PWM, one for
> > the keyboard lightning as well. (see num ec_feature_code)
> > By adding one more, you let cros_ec_dev load the platform driver for
> > you, it works even if the machine does not use device tree.
> 
> I think we can actually get this without doing the EC_FEATURE_* thing
> (which notably is not in upstream, BTW), nor by requiring a separate
> node with the "google,cros-ec-pwm" property, but instead by running a
> sample EC_CMD_PWM_GET_DUTY command on indeces [0, 255], stopping at the
> first INVAL_PARAM failure (if we stop at 0, then we have no PWM API at
> all).
> 
> But that still leaves the problem of mapping PWMs to consumer devices.
> The phandle translation is very helpful for our DT-based systems, but
> there isn't a really nice equivalent for non-DT ones. I see struct
> pwm_lookup, which looks like it could do some of what we want, but we'd
> still either need to encode a ton of board-specific information in the
> kernel, or else start exposing PWMs via the non-EC_PWM_TYPE_GENERIC
> methods (see the new enum ec_pwm_type, where we can see
> EC_PWM_TYPE_KB_LIGHT and EC_PWM_TYPE_DISPLAY_LIGHT).

I think the way that DT systems and non-DT systems work means that we
really want to address this in different ways. DT has nice phandles,
which naturally work well with index-based addressing, while non-DT has
the much inferior pwm_lookup stuff, which we'd have to encode directly
in the drivers, and which would probably work out better with the
TYPE-based addressing. So if/when we want to work on the non-DT case,
we'll just need to extend this driver to support either method.
But as we're interested in DT right now, I plan to only implement the DT
method.

> Anyway, along this line, perhaps it makes sense to:
> 
>  (a) drop the "google,cros-ec-pwm" property (via the probe method I
>  described above)

So I will be doing (a) still...

>  (b) drop the separate node for "google,cros-ec-pwm", since the presence
>  of this feature can be detected by the same methods as in (a)
> 
> leaving the only DT binding change to be to:
> 
>  (c) add an optional #pwm-cells property to the cros-ec node
>  (Documentation/devicetree/bindings/mfd/cros-ec.txt) so that we can
>  still utilize the nice PWM of_xlate stuff (and its corresponding pwms =
>  <...> property for consumer devices)

...but I'm not doing (b) or (c).

Will send v2 shortly.

Regards,
Brian