Hi Enrico,
On Thu, Nov 29, 2018 at 11:55:48AM -0800, egran...@google.com wrote:
> From: Enrico Granata <egran...@chromium.org>
>
> The ChromeOS EC has support for signaling to the host that
> a single IRQ can serve multiple MKBP events.
>
> Doing this serves an optimization purpose, as it minimizes the
> number of round-trips into the interrupt handling machinery, and
> it proves beneficial to sensor timestamping as it keeps the desired
> synchronization of event times between the two processors.
>
> This patch adds kernel support for this EC feature, allowing the
> ec_irq to loop until all events have been served.
Might be worth being more explicit in this commit message to say that
you're adding support for EC_CMD_GET_NEXT_EVENT version 2?
> Signed-off-by: Enrico Granata <egran...@chromium.org>
Mostly looks fine; thanks for sending. I have a few small notes (and
some of that is not necessarily something resolve in this patch), but
with at least the cros_ec.h comment fixup, feel free to add my:
Reviewed-by: Brian Norris <briannor...@chromium.org>
> ---
> drivers/mfd/cros_ec.c | 20 +++++++++++++--
> drivers/platform/chrome/cros_ec_proto.c | 33 +++++++++++--------------
> include/linux/mfd/cros_ec.h | 3 ++-
> include/linux/mfd/cros_ec_commands.h | 15 +++++++++++
> 4 files changed, 50 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index fe6f83766144f..17903a378aa1a 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -51,13 +51,16 @@ static const struct mfd_cell ec_pd_cell = {
> .pdata_size = sizeof(pd_p),
> };
>
> -static irqreturn_t ec_irq_thread(int irq, void *data)
> +static bool ec_handle_event(struct cros_ec_device *ec_dev)
> {
> - struct cros_ec_device *ec_dev = data;
> bool wake_event = true;
> int ret;
> + bool ec_has_more_events = false;
>
> ret = cros_ec_get_next_event(ec_dev, &wake_event);
> + if (ret > 0)
> + ec_has_more_events =
> + ec_dev->event_data.event_type & EC_MKBP_HAS_MORE_EVENTS;
>
> /*
> * Signal only if wake host events or any interrupt if
> @@ -70,6 +73,19 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
> if (ret > 0)
> blocking_notifier_call_chain(&ec_dev->event_notifier,
> 0, ec_dev);
> +
> + return ec_has_more_events;
> +}
> +
> +static irqreturn_t ec_irq_thread(int irq, void *data)
> +{
> + struct cros_ec_device *ec_dev = data;
> + bool ec_has_more_events;
> +
> + do {
> + ec_has_more_events = ec_handle_event(ec_dev);
> + } while (ec_has_more_events);
> +
> return IRQ_HANDLED;
> }
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c
> b/drivers/platform/chrome/cros_ec_proto.c
> index b6fd4838f60f3..bb126d95b2fd4 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> ret = cros_ec_get_host_command_version_mask(ec_dev,
> EC_CMD_GET_NEXT_EVENT,
> &ver_mask);
It's not exactly new here (although you're using 'ver_mask' in new
ways), but cros_ec_get_host_command_version_mask() doesn't look 100%
right. It doesn't look at msg->result, and instead just assumes that if
we got some data back (send_command() > 0), then it must have been a
success. I don't think that's really guaranteed in general, although it
might be for the specific case of EC_CMD_GET_CMD_VERSIONS.
IOW, to be definitely sure we're not looking at a garbage result in
'ver_mask', we should probably fixup
cros_ec_get_host_command_version_mask().
> - if (ret < 0 || ver_mask == 0)
> + if (ret < 0 || ver_mask == 0) {
> ec_dev->mkbp_event_supported = 0;
> - else
> - ec_dev->mkbp_event_supported = 1;
> + dev_info(ec_dev->dev, "MKBP not supported\n");
> + } else {
> + ec_dev->mkbp_event_supported = fls(ver_mask);
> + dev_info(ec_dev->dev, "MKBP support version %u\n",
> + ec_dev->mkbp_event_supported - 1);
> + }
>
> /*
> * Get host event wake mask, assume all events are wake events
> @@ -530,28 +534,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
> {
> u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
> struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
> - static int cmd_version = 1;
> - int ret;
> + const int cmd_version = ec_dev->mkbp_event_supported - 1;
>
> if (ec_dev->suspended) {
> dev_dbg(ec_dev->dev, "Device suspended.\n");
> return -EHOSTDOWN;
> }
>
> - if (cmd_version == 1) {
> - ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> - sizeof(struct ec_response_get_next_event_v1));
> - if (ret < 0 || msg->result != EC_RES_INVALID_VERSION)
> - return ret;
> -
> - /* Fallback to version 0 for future send attempts */
> - cmd_version = 0;
> - }
> -
> - ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> + if (cmd_version == 0)
> + return get_next_event_xfer(ec_dev, msg, 0,
> sizeof(struct ec_response_get_next_event));
>
> - return ret;
> + return get_next_event_xfer(ec_dev, msg, cmd_version,
> + sizeof(struct ec_response_get_next_event_v1));
> }
>
> static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
> @@ -607,11 +602,13 @@ EXPORT_SYMBOL(cros_ec_get_next_event);
>
> u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
> {
> + u32 event_type =
> + ec_dev->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
> u32 host_event;
>
> BUG_ON(!ec_dev->mkbp_event_supported);
>
> - if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT)
> + if (event_type != EC_MKBP_EVENT_HOST_EVENT)
> return 0;
>
> if (ec_dev->event_size != sizeof(host_event)) {
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index e44e3ec8a9c7d..eb771ceeaeed1 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -152,7 +152,8 @@ struct cros_ec_device {
> int (*pkt_xfer)(struct cros_ec_device *ec,
> struct cros_ec_command *msg);
> struct mutex lock;
> - bool mkbp_event_supported;
> + /* 0 == not supported, otherwise it supports version x - 1 */
This comment belongs in the kerneldoc, which is above the struct
definition. You're invalidating the existing comment:
* @mkbp_event_supported: True if this EC supports the MKBP event protocol.
Brian
> + u8 mkbp_event_supported;
> struct blocking_notifier_head event_notifier;
>
> struct ec_response_get_next_event_v1 event_data;
...
