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

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

[Brian Norris]

Hi,

This is version 2 of my series to support the new ChromeOS EC PWM API, so we
can control, e.g., a PWM backlight when its PWM is 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. Tomeu has acknowledged his USB PD
series is on hold for now, so this should be OK.

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

Please review.

Regards,
Brian

Change log (also documented in each patch):

v2:
 * drop the "google,max-pwms" property
 * separate the cros_ec vs. PWM abstractions a little more clearly in the driver
 * remove dynamic kzalloc()'s and rely on on-stack memory instead
 * auto-probe the number of PWMs supported

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 |  23 ++
 drivers/platform/chrome/cros_ec_proto.c            |  15 ++
 drivers/pwm/Kconfig                                |   7 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-cros-ec.c                          | 237 +++++++++++++++++++++
 include/linux/mfd/cros_ec.h                        |  18 ++
 include/linux/mfd/cros_ec_commands.h               |  31 +++
 7 files changed, 332 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 v2 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>
---
v2: no change

 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 b6e161f71b26..ecc544c728f0 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 64184d27e3cd..b1e48dfbf50a 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.
  */
@@ -226,6 +229,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 v2 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>
---
v2: no change

 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 v2 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, but we can autodetect this by checking the error
codes, so we don't need an extra property for 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>
---
v2: dropped the "google,max-pwms" property

 .../devicetree/bindings/pwm/google,cros-ec-pwm.txt | 23 ++++++++++++++++++++++
 1 file changed, 23 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..472bd46ab5a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
@@ -0,0 +1,23 @@
+* 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.
+
+Example:
+	cros-ec@0 {
+		compatible = "google,cros-ec-spi";
+
+		...
+
+		cros_ec_pwm: ec-pwm {
+			compatible = "google,cros-ec-pwm";
+			#pwm-cells = <1>;
+		};
+	};
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 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.

This driver supports only device tree at the moment, because that
provides a very flexible way of describing the relationship between PWMs
and their consumer devices (e.g., backlight). On a non-DT system, we'll
probably want to use the non-GENERIC addressing (i.e., we'll need to
make special device instances that will use EC_PWM_TYPE_KB_LIGHT or
EC_PWM_TYPE_DISPLAY_LIGHT), as well as the relatively inflexible
pwm_lookup infrastructure for matching devices. Defer that work for now.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v2:
 * refactor some abstractions to separate the PWM layer handling from
   the cros_ec handling
 * remove dynamic kzalloc()'s and rely on on-stack memory instead
 * auto-probe the number of PWMs supported

 drivers/pwm/Kconfig       |   7 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-cros-ec.c | 237 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 245 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..de7f554a86e2
--- /dev/null
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -0,0 +1,237 @@
+/*
+ * 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_device *ec, u8 index, u16 duty)
+{
+	struct {
+		struct cros_ec_command msg;
+		struct ec_params_pwm_set_duty params;
+	} buf;
+	struct ec_params_pwm_set_duty *params = &buf.params;
+	struct cros_ec_command *msg = &buf.msg;
+
+	memset(&buf, 0, sizeof(buf));
+
+	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 = index;
+
+	return cros_ec_cmd_xfer_status(ec, msg);
+}
+
+static int cros_ec_pwm_get_duty(struct cros_ec_device *ec, u8 index)
+{
+	struct {
+		struct cros_ec_command msg;
+		union {
+			struct ec_params_pwm_get_duty params;
+			struct ec_response_pwm_get_duty resp;
+		};
+	} buf;
+	struct ec_params_pwm_get_duty *params = &buf.params;
+	struct ec_response_pwm_get_duty *resp = &buf.resp;
+	struct cros_ec_command *msg = &buf.msg;
+	int ret;
+
+	memset(&buf, 0, sizeof(buf));
+
+	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 = index;
+
+	ret = cros_ec_cmd_xfer_status(ec, msg);
+	if (ret < 0)
+		return ret;
+
+	return resp->duty;
+}
+
+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->ec, pwm->hwpwm, 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->ec, pwm->hwpwm);
+	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_num_pwms(struct cros_ec_device *ec)
+{
+	int i, ret;
+
+	/* The index field is only 8 bits */
+	for (i = 0; i <= U8_MAX; i++) {
+		ret = cros_ec_pwm_get_duty(ec, i);
+		/*
+		 * We look for SUCCESS, INVALID_COMMAND, or INVALID_PARAM responses;
+		 * everything else is treated as an error
+		 */
+		if (ret == -EECRESULT - EC_RES_INVALID_COMMAND)
+			return -ENODEV;
+		else if (ret == -EECRESULT - EC_RES_INVALID_PARAM)
+			return i;
+		else if (ret < 0)
+			return ret;
+	}
+
+	return U8_MAX;
+}
+
+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 cros_ec_pwm_device *ec_pwm;
+	struct pwm_chip *chip;
+	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 = cros_ec_num_pwms(ec);
+	if (ret < 0) {
+		dev_err(dev, "Couldn't find PWMs: %d\n", ret);
+		return ret;
+	}
+	chip->npwm = ret;
+	dev_dbg(dev, "Probed %u PWMs\n", chip->npwm);
+
+	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 v2 3/4] doc: dt: pwm: add binding for ChromeOS EC PWM

[Rob Herring]

On Thu, Jun 02, 2016 at 06:21:43PM -0700, Brian Norris 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, but we can autodetect this by checking the error
> codes, so we don't need an extra property for 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>
> ---
> v2: dropped the "google,max-pwms" property
> 
>  .../devicetree/bindings/pwm/google,cros-ec-pwm.txt | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt

Acked-by: Rob Herring <robh@kernel.org>

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

[Lee Jones]

On Thu, 02 Jun 2016, Brian Norris wrote:

> 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>
> ---
> v2: no change
> 
>  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 b6e161f71b26..ecc544c728f0 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 64184d27e3cd..b1e48dfbf50a 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.
>   */
> @@ -226,6 +229,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

s/succes/success/

I guess whoever takes this patch can do a quick fix-up job on it, so:

Acked-by: Lee Jones <lee.jones@linaro.org>

> + * 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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Thu, 02 Jun 2016, Brian Norris wrote:

> 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>
> ---
> v2: no change
> 
>  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

Any reason this isn't represented in hex, like we do normally?

> +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,
> +};

Are these comments really necessary?  I'd recommend that if your
defines require comments, then they are not adequately named.  In this
case however, I'd suggest that they are and the comments are
superfluous.

> +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;

Please use kerneldoc format.

> +#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;

As above.

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

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Enric Balletbo i Serra]

On 16/06/16 17:30, Lee Jones wrote:
> On Thu, 02 Jun 2016, Brian Norris wrote:
> 
>> 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>
>> ---
>> v2: no change
>>
>>  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 b6e161f71b26..ecc544c728f0 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 64184d27e3cd..b1e48dfbf50a 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.
>>   */
>> @@ -226,6 +229,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
> 
> s/succes/success/
> 
> I guess whoever takes this patch can do a quick fix-up job on it, so:
> 
> Acked-by: Lee Jones <lee.jones@linaro.org>
> 
>> + * 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.
> 

This patch is also useful for other drivers like the cros-ec sensors that we plan to upstream soon, so will be good have it merged.

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

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

[Doug Anderson]

Lee,

On Thu, Jun 16, 2016 at 8:38 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 02 Jun 2016, Brian Norris wrote:
>
>> 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>
>> ---
>> v2: no change
>>
>>  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
>
> Any reason this isn't represented in hex, like we do normally?
>
>> +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,
>> +};
>
> Are these comments really necessary?  I'd recommend that if your
> defines require comments, then they are not adequately named.  In this
> case however, I'd suggest that they are and the comments are
> superfluous.
>
>> +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;
>
> Please use kerneldoc format.
>
>> +#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;

Probably the reason for all of these non-kernel-isms is that this
isn't a kernel file.  From the top of the file:

 * NOTE: This file is copied verbatim from the ChromeOS EC Open Source
 * project in an attempt to make future updates easy to make.

So the source of truth for this file is
<https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.

Someone could probably submit a CL to that project to make it a little
more kernel-ish and then we'd have to see if the EC team would accept
such changes...


-Doug

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

[Lee Jones]

On Thu, 16 Jun 2016, Doug Anderson wrote:
> On Thu, Jun 16, 2016 at 8:38 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Thu, 02 Jun 2016, Brian Norris wrote:
> >
> >> 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>
> >> ---
> >> v2: no change
> >>
> >>  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
> >
> > Any reason this isn't represented in hex, like we do normally?
> >
> >> +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,
> >> +};
> >
> > Are these comments really necessary?  I'd recommend that if your
> > defines require comments, then they are not adequately named.  In this
> > case however, I'd suggest that they are and the comments are
> > superfluous.
> >
> >> +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;
> >
> > Please use kerneldoc format.
> >
> >> +#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;
> 
> Probably the reason for all of these non-kernel-isms is that this
> isn't a kernel file.  From the top of the file:
> 
>  * NOTE: This file is copied verbatim from the ChromeOS EC Open Source
>  * project in an attempt to make future updates easy to make.
> 
> So the source of truth for this file is
> <https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.
> 
> Someone could probably submit a CL to that project to make it a little
> more kernel-ish and then we'd have to see if the EC team would accept
> such changes...

Hmmm... that kinda puts me in a difficult position.  Do I except
non-kernel code, which does not conform to our stands?

Naturally I'd be happier if you could try to make the code more
'kernely'.  The practices I mention above are still good ones, even if
you're not writing kernel specific code.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Doug Anderson]

Lee,

On Fri, Jun 17, 2016 at 1:06 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> Probably the reason for all of these non-kernel-isms is that this
>> isn't a kernel file.  From the top of the file:
>>
>>  * NOTE: This file is copied verbatim from the ChromeOS EC Open Source
>>  * project in an attempt to make future updates easy to make.
>>
>> So the source of truth for this file is
>> <https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.
>>
>> Someone could probably submit a CL to that project to make it a little
>> more kernel-ish and then we'd have to see if the EC team would accept
>> such changes...
>
> Hmmm... that kinda puts me in a difficult position.  Do I except
> non-kernel code, which does not conform to our stands?

What about if Brian made sure to just fully copy the latest version of
"cros_ec_commands.h" from the EC codebase and changed this commit
message to say:

Copy the latest version of "cros_ec_commands.h" from the Chrome OS EC
code base, which is the source of truth for this file.  See
<https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.

>From the commit message it would be clear that this is an external
file linked into the kernel for convenience.


> Naturally I'd be happier if you could try to make the code more
> 'kernely'.  The practices I mention above are still good ones, even if
> you're not writing kernel specific code.

In general requesting that code from outside the kernel conform to
"kerneldoc" seems like a bit of a stretch.  In general having some
type of parse-able format for comments is nice, but I could see that
in the Chrome OS EC codebase it would be a bit overkill.


Also: it would be awfully strange if we suddenly started changing the
coding convention of this file or we had half the file in one
convention and half in another.  The rest of this file is in EC
convention and it seems sane to keep it that way...


-Doug

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

[Thierry Reding]

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

On Fri, Jun 17, 2016 at 09:06:35AM +0100, Lee Jones wrote:
> On Thu, 16 Jun 2016, Doug Anderson wrote:
> > On Thu, Jun 16, 2016 at 8:38 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > > On Thu, 02 Jun 2016, Brian Norris wrote:
> > >
> > >> 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>
> > >> ---
> > >> v2: no change
> > >>
> > >>  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
> > >
> > > Any reason this isn't represented in hex, like we do normally?
> > >
> > >> +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,
> > >> +};
> > >
> > > Are these comments really necessary?  I'd recommend that if your
> > > defines require comments, then they are not adequately named.  In this
> > > case however, I'd suggest that they are and the comments are
> > > superfluous.
> > >
> > >> +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;
> > >
> > > Please use kerneldoc format.
> > >
> > >> +#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;
> > 
> > Probably the reason for all of these non-kernel-isms is that this
> > isn't a kernel file.  From the top of the file:
> > 
> >  * NOTE: This file is copied verbatim from the ChromeOS EC Open Source
> >  * project in an attempt to make future updates easy to make.
> > 
> > So the source of truth for this file is
> > <https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.
> > 
> > Someone could probably submit a CL to that project to make it a little
> > more kernel-ish and then we'd have to see if the EC team would accept
> > such changes...
> 
> Hmmm... that kinda puts me in a difficult position.  Do I except
> non-kernel code, which does not conform to our stands?

I think usually code that doesn't adhere to kernel coding style ends up
in the staging tree until it's been cleaned up enough.

The rule doesn't quite apply here because cleaning up isn't the issue.
But I don't know if we have any best practices for this kind of thing.

One thing that I've seen done in the past is to have this kind of
cross-OS header generated from some sort of definition file (XML, ...)
with an output filter for all supported OSes. Is that something that
could perhaps be done here?

That said, this might not even be worth it in this case. While I see how
it makes sense to avoid work updating this file for various coding
styles, the content in this file defines an ABI, so really the only
changes will be additions, and once they're merged they become set in
stone anyway. The amount of work updating the header, even taking into
account different coding styles should be very low.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Brian Norris]

On Fri, Jun 17, 2016 at 05:55:52PM +0200, Thierry Reding wrote:
> On Fri, Jun 17, 2016 at 09:06:35AM +0100, Lee Jones wrote:
> > On Thu, 16 Jun 2016, Doug Anderson wrote:
> > > On Thu, Jun 16, 2016 at 8:38 AM, Lee Jones <lee.jones@linaro.org> wrote:
[...]

> > > > Please use kerneldoc format.

[...]

> > > Probably the reason for all of these non-kernel-isms is that this
> > > isn't a kernel file.  From the top of the file:
> > > 
> > >  * NOTE: This file is copied verbatim from the ChromeOS EC Open Source
> > >  * project in an attempt to make future updates easy to make.
> > > 
> > > So the source of truth for this file is
> > > <https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.
> > > 
> > > Someone could probably submit a CL to that project to make it a little
> > > more kernel-ish and then we'd have to see if the EC team would accept
> > > such changes...
> > 
> > Hmmm... that kinda puts me in a difficult position.  Do I except
> > non-kernel code, which does not conform to our stands?

I could have sworn that there was an exception for existing code;
changing styles just for the sake of changing styles can hurt more than
help (e.g., in this instance, in comparing this file with the source of
truth -- the Chrome OS EC firmware sources). But I can't find an
explicit reference to this right now...

> I think usually code that doesn't adhere to kernel coding style ends up
> in the staging tree until it's been cleaned up enough.
> 
> The rule doesn't quite apply here because cleaning up isn't the issue.
> But I don't know if we have any best practices for this kind of thing.
> 
> One thing that I've seen done in the past is to have this kind of
> cross-OS header generated from some sort of definition file (XML, ...)
> with an output filter for all supported OSes. Is that something that
> could perhaps be done here?

I think I can safely say that rewriting this in XML is out of the
question for us.

> That said, this might not even be worth it in this case. While I see how
> it makes sense to avoid work updating this file for various coding
> styles, the content in this file defines an ABI, so really the only
> changes will be additions, and once they're merged they become set in
> stone anyway. The amount of work updating the header, even taking into
> account different coding styles should be very low.

IMO, there is value in being able to compare the two sources, and there
is little (but non-zero) value in making these rather minor suggested
changes. Additionally, reformatting this particular change to use
kerneldoc, when the rest of the file doesn't, seems inconsistent. How
does the following sound instead?

(1) Keep the existing patch as-is (perhaps modulo one or two of Lee's
    non-kerneldoc-related comments)
(2) I send a follow-up patch, to help synchronize the out-of-tree EC
    header with the kernel header, after we've considered converting it
    to better match kernel styles. BTW, we've filed a bug for this
    discussion [1], to ensure it doesn't get dropped on the floor.

Brian

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=621123

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

[Brian Norris]

On Thu, Jun 16, 2016 at 04:38:17PM +0100, Lee Jones wrote:
> On Thu, 02 Jun 2016, Brian Norris wrote:
> > 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>
> > ---
> > v2: no change
> > 
> >  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
> 
> Any reason this isn't represented in hex, like we do normally?

Hex would probably be clearer. I'll try to change that.

> > +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,
> > +};
> 
> Are these comments really necessary?  I'd recommend that if your
> defines require comments, then they are not adequately named.  In this
> case however, I'd suggest that they are and the comments are
> superfluous.

I don't think your rule holds water: there are definitely cases where
defines/enums require (or at least are better with) additional comments.
Sentence-long identifier names are not very readable, but sometimes a
sentence of comment can help.

Anyway, I think two of the three are probably unnecessary, if you really
want to ask. The first (EC_PWM_TYPE_GENERIC) seems useful.

But then, how do you suggest handling this in conjunction with your
kerneldoc suggestion? IIRC, kerneldoc requires that if one
entry/field/parameter is documented, then all most be documented. So
avoid kerneldoc on the enum, and just use inline comments?

[snip the rest, which was discussed in other branches of this thread]

Brian

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

[Lee Jones]

On Fri, 17 Jun 2016, Brian Norris wrote:

> On Thu, Jun 16, 2016 at 04:38:17PM +0100, Lee Jones wrote:
> > On Thu, 02 Jun 2016, Brian Norris wrote:
> > > 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>
> > > ---
> > > v2: no change
> > > 
> > >  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
> > 
> > Any reason this isn't represented in hex, like we do normally?
> 
> Hex would probably be clearer. I'll try to change that.
> 
> > > +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,
> > > +};
> > 
> > Are these comments really necessary?  I'd recommend that if your
> > defines require comments, then they are not adequately named.  In this
> > case however, I'd suggest that they are and the comments are
> > superfluous.
> 
> I don't think your rule holds water: there are definitely cases where
> defines/enums require (or at least are better with) additional comments.
> Sentence-long identifier names are not very readable, but sometimes a
> sentence of comment can help.

I was generalising.  There will always be exceptions to the rule, but
in the standard case we can be forthcoming enough with our naming
conventions that comments aren't required.

> Anyway, I think two of the three are probably unnecessary, if you really
> want to ask. The first (EC_PWM_TYPE_GENERIC) seems useful.
> 
> But then, how do you suggest handling this in conjunction with your
> kerneldoc suggestion? IIRC, kerneldoc requires that if one
> entry/field/parameter is documented, then all most be documented. So
> avoid kerneldoc on the enum, and just use inline comments?

Sounds reasonable.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Fri, 17 Jun 2016, Doug Anderson wrote:

> Lee,
> 
> On Fri, Jun 17, 2016 at 1:06 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >> Probably the reason for all of these non-kernel-isms is that this
> >> isn't a kernel file.  From the top of the file:
> >>
> >>  * NOTE: This file is copied verbatim from the ChromeOS EC Open Source
> >>  * project in an attempt to make future updates easy to make.
> >>
> >> So the source of truth for this file is
> >> <https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.
> >>
> >> Someone could probably submit a CL to that project to make it a little
> >> more kernel-ish and then we'd have to see if the EC team would accept
> >> such changes...
> >
> > Hmmm... that kinda puts me in a difficult position.  Do I except
> > non-kernel code, which does not conform to our stands?
> 
> What about if Brian made sure to just fully copy the latest version of
> "cros_ec_commands.h" from the EC codebase and changed this commit
> message to say:
> 
> Copy the latest version of "cros_ec_commands.h" from the Chrome OS EC
> code base, which is the source of truth for this file.  See
> <https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.
> 
> From the commit message it would be clear that this is an external
> file linked into the kernel for convenience.
> 
> 
> > Naturally I'd be happier if you could try to make the code more
> > 'kernely'.  The practices I mention above are still good ones, even if
> > you're not writing kernel specific code.
> 
> In general requesting that code from outside the kernel conform to
> "kerneldoc" seems like a bit of a stretch.  In general having some
> type of parse-able format for comments is nice, but I could see that
> in the Chrome OS EC codebase it would be a bit overkill.

It's unfortunate that kerneldoc is named so, since I think it's a nice
way to write structure/function headers regardless of code base and
not overkill at all.  It's certainly harder to convince !kernel code
to use the format, or at least easier for others to push back due to
the fact that is has 'kernel' in the name.

> Also: it would be awfully strange if we suddenly started changing the
> coding convention of this file or we had half the file in one
> convention and half in another.  The rest of this file is in EC
> convention and it seems sane to keep it that way...

Right.  It's also a shame we're only catching this now.  Really we
should have had this discussion in the first instance.

Taking into consideration that this file is already in the kernel and
that it's current format is also represented, I'm willing to keep
adding to it.  I would like to see an internal request to adopt
so-called kerneldoc.  Not because I am wish to blindly push our
standards to other code-bases, but because I am an advocate of the
format in general.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Brian Norris]

Hi Lee,

On Mon, Jun 20, 2016 at 09:00:51AM +0100, Lee Jones wrote:
> On Fri, 17 Jun 2016, Doug Anderson wrote:
> > On Fri, Jun 17, 2016 at 1:06 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > >> Probably the reason for all of these non-kernel-isms is that this
> > >> isn't a kernel file.  From the top of the file:
> > >>
> > >>  * NOTE: This file is copied verbatim from the ChromeOS EC Open Source
> > >>  * project in an attempt to make future updates easy to make.
> > >>
> > >> So the source of truth for this file is
> > >> <https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.
> > >>
> > >> Someone could probably submit a CL to that project to make it a little
> > >> more kernel-ish and then we'd have to see if the EC team would accept
> > >> such changes...
> > >
> > > Hmmm... that kinda puts me in a difficult position.  Do I except
> > > non-kernel code, which does not conform to our stands?
> > 
> > What about if Brian made sure to just fully copy the latest version of
> > "cros_ec_commands.h" from the EC codebase and changed this commit
> > message to say:
> > 
> > Copy the latest version of "cros_ec_commands.h" from the Chrome OS EC
> > code base, which is the source of truth for this file.  See
> > <https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.
> > 
> > From the commit message it would be clear that this is an external
> > file linked into the kernel for convenience.
> > 
> > 
> > > Naturally I'd be happier if you could try to make the code more
> > > 'kernely'.  The practices I mention above are still good ones, even if
> > > you're not writing kernel specific code.
> > 
> > In general requesting that code from outside the kernel conform to
> > "kerneldoc" seems like a bit of a stretch.  In general having some
> > type of parse-able format for comments is nice, but I could see that
> > in the Chrome OS EC codebase it would be a bit overkill.
> 
> It's unfortunate that kerneldoc is named so, since I think it's a nice
> way to write structure/function headers regardless of code base and
> not overkill at all.  It's certainly harder to convince !kernel code
> to use the format, or at least easier for others to push back due to
> the fact that is has 'kernel' in the name.
> 
> > Also: it would be awfully strange if we suddenly started changing the
> > coding convention of this file or we had half the file in one
> > convention and half in another.  The rest of this file is in EC
> > convention and it seems sane to keep it that way...
> 
> Right.  It's also a shame we're only catching this now.  Really we
> should have had this discussion in the first instance.

Well, I see your sign-off on 2 of 4 patches to this file :)

> Taking into consideration that this file is already in the kernel and
> that it's current format is also represented, I'm willing to keep
> adding to it.

Thanks, that seems quite reasonable.

> I would like to see an internal request to adopt
> so-called kerneldoc.  Not because I am wish to blindly push our
> standards to other code-bases, but because I am an advocate of the
> format in general.

As mentioned in another branch of this thread, such a request has
already been made (and it's public, so feel free to follow if you're
so inclined):

https://bugs.chromium.org/p/chromium/issues/detail?id=621123

It seems as if the firmware authors have already agreed that adopting a
more consistent style like kerneldoc would be a net postive. So you
should expect to eventually see a patch to update this file. But in the
meantime, I think it would make things sanest if we update the file
incrementally until that is done.

Regards,
Brian