Re: [RESEND PATCH v7 1/6] mfd: cros_ec: Add MKBP event support

[Lee Jones <lee.jones@linaro.org>]

On Tue, 05 Apr 2016, Tomeu Vizoso wrote:

> From: Vic Yang <victoryang@google.com>
> 
> Newer revisions of the ChromeOS EC add more events besides the keyboard
> ones. So handle interrupts in the MFD driver and let consumers register
> for notifications for the events they might care.
> 
> To keep backward compatibility, if the EC doesn't support MKBP event, we
> fall back to the old MKBP key matrix host command.
> 
> Signed-off-by: Vic Yang <victoryang@chromium.org>
> [bleung: fixup some context changes going from v3.14 to v3.18]
> Signed-off-by: Benson Leung <bleung@chromium.org>
> [tomeu: adapted to changes in mainline (in power-supply and platform/chrome)]
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

I'm not interested in your BSP submission path.  As far as I'm
concerned *this* is the first submission.  If these guys are happy
with the patch, they can either choose to Ack or Review it.  Drop the
blurb in the middle.

> Cc: Randall Spangler <rspangler@chromium.org>
> Cc: Vincent Palatin <vpalatin@chromium.org>
> ---
> 
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4:
> - Calculate correctly the size of the payloads in
>   cros_ec_get_host_command_version_mask.
> 
> Changes in v3:
> - Remove duplicated prototype of cros_ec_get_host_event.
> 
> Changes in v2:
> - Allocate enough for the structs in cros_ec_get_host_command_version_mask,
>   not their pointers.
> - Allocate msg in the stack in get_next_event and
>   get_keyboard_state_event, as suggested by Gwendal.
> 
>  drivers/input/keyboard/cros_ec_keyb.c   | 135 ++++++++------------------------
>  drivers/mfd/cros_ec.c                   |  57 +++++++++++++-
>  drivers/platform/chrome/cros_ec_proto.c |  92 ++++++++++++++++++++++
>  include/linux/mfd/cros_ec.h             |  34 ++++++++
>  include/linux/mfd/cros_ec_commands.h    |  34 ++++++++

What are the *build time* dependencies that warrant all of these
changes happening in one patch?

>  5 files changed, 245 insertions(+), 107 deletions(-)

[...]
  
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index 0eee63542038..fbe78b819fdd 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -23,6 +23,7 @@
>  #include <linux/module.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/cros_ec.h>
> +#include <asm/unaligned.h>
>  
>  #define CROS_EC_DEV_EC_INDEX 0
>  #define CROS_EC_DEV_PD_INDEX 1
> @@ -49,11 +50,28 @@ static const struct mfd_cell ec_pd_cell = {
>  	.pdata_size = sizeof(pd_p),
>  };
>  
> +static irqreturn_t ec_irq_thread(int irq, void *data)
> +{
> +	struct cros_ec_device *ec_dev = data;
> +	int ret;
> +
> +	if (device_may_wakeup(ec_dev->dev))
> +		pm_wakeup_event(ec_dev->dev, 0);
> +
> +	ret = cros_ec_get_next_event(ec_dev);
> +	if (ret > 0)
> +		blocking_notifier_call_chain(&ec_dev->event_notifier,
> +					     0, ec_dev);
> +	return IRQ_HANDLED;
> +}
> +
>  int cros_ec_register(struct cros_ec_device *ec_dev)
>  {
>  	struct device *dev = ec_dev->dev;
>  	int err = 0;
>  
> +	BLOCKING_INIT_NOTIFIER_HEAD(&ec_dev->event_notifier);
> +
>  	ec_dev->max_request = sizeof(struct ec_params_hello);
>  	ec_dev->max_response = sizeof(struct ec_response_get_protocol_info);
>  	ec_dev->max_passthru = 0;
> @@ -70,13 +88,24 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  
>  	cros_ec_query_all(ec_dev);
>  
> +	if (ec_dev->irq) {
> +		err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
> +					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +					   "chromeos-ec", ec_dev);
> +		if (err) {
> +			dev_err(dev, "request irq %d: error %d\n",

This is an ugly error message.  Write them like you (as I user) would
like to see.  I suggest using proper English and grammar.

"Failed to request IRQ %d: %d", irq, err ?

> +				ec_dev->irq, err);
> +			return err;
> +		}
> +	}
> +
>  	err = mfd_add_devices(ec_dev->dev, PLATFORM_DEVID_AUTO, &ec_cell, 1,
>  			      NULL, ec_dev->irq, NULL);
>  	if (err) {
>  		dev_err(dev,
>  			"Failed to register Embedded Controller subdevice %d\n",
>  			err);
> -		return err;
> +		goto fail_mfd;
>  	}
>  
>  	if (ec_dev->max_passthru) {
> @@ -94,7 +123,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  			dev_err(dev,
>  				"Failed to register Power Delivery subdevice %d\n",
>  				err);
> -			return err;
> +			goto fail_mfd;
>  		}
>  	}
>  
> @@ -103,13 +132,18 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  		if (err) {
>  			mfd_remove_devices(dev);
>  			dev_err(dev, "Failed to register sub-devices\n");
> -			return err;
> +			goto fail_mfd;
>  		}
>  	}
>  
>  	dev_info(dev, "Chrome EC device registered\n");
>  
>  	return 0;
> +
> +fail_mfd:
> +	if (ec_dev->irq)
> +		free_irq(ec_dev->irq, ec_dev);
> +	return err;
>  }
>  EXPORT_SYMBOL(cros_ec_register);
>  
> @@ -136,13 +170,30 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
>  }
>  EXPORT_SYMBOL(cros_ec_suspend);
>  
> +static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
> +{
> +	while (cros_ec_get_next_event(ec_dev) > 0)
> +		blocking_notifier_call_chain(&ec_dev->event_notifier,
> +					     1, ec_dev);
> +}
> +
>  int cros_ec_resume(struct cros_ec_device *ec_dev)
>  {
>  	enable_irq(ec_dev->irq);
>  
> +	/*
> +	 * In some case, we need to distinguish events that occur during

s/case/cases/

s/distinguish/distinguish between/

> +	 * suspend if the EC is not a wake source. For example, keypresses
> +	 * during suspend should be discarded if it does not wake the system.
> +	 *
> +	 * If the EC is not a wake source, drain the event queue and mark them
> +	 * as "queued during suspend".
> +	 */
>  	if (ec_dev->wake_enabled) {
>  		disable_irq_wake(ec_dev->irq);
>  		ec_dev->wake_enabled = 0;
> +	} else {
> +		cros_ec_drain_events(ec_dev);
>  	}
>  
>  	return 0;

[...]

> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index a677c2bd485c..ddc935ef1911 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -72,6 +72,24 @@ struct cros_ec_command {
>  	uint8_t data[0];
>  };
>  
> +/*
> + * event_data is used by keyboard or event notifier:
> + * event_data format:
> + * If MKBP protocol is supported:
> + * 0           1
> + * +-----------+--------------------------------
> + * | type      | payload
> + * +-----------+--------------------------------
> + * |HOST_EVENT | EVENT (32 bit)
> + * |KEY_MATRIX | Keyboard keys pressed.
> + * |SENSOR_FIFO| Sensors FIFO information.
> + *
> + * Otherwise:
> + * 0           1
> + * +-----------+--------------------------------
> + * |Unused     | Keyboard keys pressed.
> + */

Personally, I don't think this documentation is required.  But if you
insist on supplying it, I think it'll be better placed near the
'struct ec_response_get_next_event' definition.

>  /**
>   * struct cros_ec_device - Information about a ChromeOS EC device
>   *
> @@ -107,6 +125,9 @@ struct cros_ec_command {
>   *     should check msg.result for the EC's result code.
>   * @pkt_xfer: send packet to EC and get response
>   * @lock: one transaction at a time
> + * @event_notifier: interrupt event notifier for transport devices.
> + * @event_data: raw payload transferred with the MKBP event.
> + * @event_size: size in bytes of the event data.
>   */
>  struct cros_ec_device {
>  
> @@ -135,6 +156,11 @@ struct cros_ec_device {
>  	int (*pkt_xfer)(struct cros_ec_device *ec,
>  			struct cros_ec_command *msg);
>  	struct mutex lock;
> +	bool mkbp_event_supported;

Did you document this?

> +	struct blocking_notifier_head event_notifier;
> +
> +	struct ec_response_get_next_event event_data;
> +	int event_size;
>  };
>  
>  /* struct cros_ec_platform - ChromeOS EC platform information
> @@ -252,6 +278,14 @@ int cros_ec_register(struct cros_ec_device *ec_dev);
>   */
>  int cros_ec_query_all(struct cros_ec_device *ec_dev);
>  
> +/**
> + * cros_ec_get_next_event -  Fetch next event from the ChromeOS EC
> + *
> + * @ec_dev: Device to fetch event from
> + * @return 0 if ok, -ve on error

I'd prefer easy to read/descriptive over trying to be smart.

The 'return' value doesn' require a @.  Instead, the return section
should look like "Return: <blah>".

I suggest: "Return: 0 on success, Linux error number on failure"

> + */
> +int cros_ec_get_next_event(struct cros_ec_device *ec_dev);
> +
>  /* sysfs stuff */
>  extern struct attribute_group cros_ec_attr_group;
>  extern struct attribute_group cros_ec_lightbar_attr_group;
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index 13b630c10d4c..d86526f0bd8e 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -1762,6 +1762,40 @@ struct ec_result_keyscan_seq_ctrl {
>  	};
>  } __packed;
>  
> +/*
> + * Get the next pending MKBP event.
> + *
> + * Returns EC_RES_UNAVAILABLE if there is no event pending.
> + */

You're documenting this command as if it's a function.  This command
does nothing by it's self, rather it is supplied to a function call,
which does the work.  Similarly this command returns nothing, the
device will provide the UNAVAILABLE return value.  Please update the
comment.

> +#define EC_CMD_GET_NEXT_EVENT 0x67
> +
> +enum ec_mkbp_event {
> +	/* Keyboard matrix changed. The event data is the new matrix state. */
> +	EC_MKBP_EVENT_KEY_MATRIX = 0,
> +
> +	/* New host event. The event data is 4 bytes of host event flags. */
> +	EC_MKBP_EVENT_HOST_EVENT = 1,
> +
> +	/* New Sensor FIFO data. The event data is fifo_info structure. */
> +	EC_MKBP_EVENT_SENSOR_FIFO = 2,
> +
> +	/* Number of MKBP events */
> +	EC_MKBP_EVENT_COUNT,
> +};
> +
> +union ec_response_get_next_data {
> +	uint8_t   key_matrix[13];
> +
> +	/* Unaligned */
> +	uint32_t  host_event;
> +} __packed;
> +
> +struct ec_response_get_next_event {
> +	uint8_t event_type;
> +	/* Followed by event data if any */
> +	union ec_response_get_next_data data;
> +} __packed;
> +
>  /*****************************************************************************/
>  /* Temperature sensor commands */
>  

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