On Tue, 05 Apr 2016, Tomeu Vizoso wrote: > From: Vic Yang <[email protected]> > > 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 <[email protected]> > [bleung: fixup some context changes going from v3.14 to v3.18] > Signed-off-by: Benson Leung <[email protected]> > [tomeu: adapted to changes in mainline (in power-supply and platform/chrome)] > Signed-off-by: Tomeu Vizoso <[email protected]>
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 <[email protected]> > Cc: Vincent Palatin <[email protected]> > --- > > 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
