Re: [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes
Hi Brian,
On Wed, Nov 07, 2018 at 06:49:38PM -0800, Brian Norris wrote:
> cros_ec_get_next_event() is documented to return 0 for success and
> negative for errors. It currently returns negative for some errors, and
> non-negative (number of bytes received) for success (including some "no
> data available" responses as zero). This mostly works out OK, because the
> callers were more or less ignoring the documentation, and only treating
> positive values as success (and indepdently checking the modification of
> 'wakeup').
>
> Let's button this up by avoiding pretending to handle event/wakeup
> distinctions when no event info was retrieved (i.e., returned 0 bytes).
> And fix the documentation of cros_ec_get_host_event() and
> cros_ec_get_next_event() to accurately describe their behavior.
>
> Signed-off-by: Brian Norris
Applied.
Thanks,
Benson
> ---
> v1 -> v2:
> * don't make as many changes to the API -- just fix the documentation
>and a few corner cases instead
> ---
> drivers/platform/chrome/cros_ec_proto.c | 4 ++--
> include/linux/mfd/cros_ec.h | 6 --
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c
> b/drivers/platform/chrome/cros_ec_proto.c
> index b6fd4838f60f..fff67b389c87 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -580,7 +580,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
> bool *wake_event)
>
> if (!ec_dev->mkbp_event_supported) {
> ret = get_keyboard_state_event(ec_dev);
> - if (ret < 0)
> + if (ret <= 0)
> return ret;
>
> if (wake_event)
> @@ -590,7 +590,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
> bool *wake_event)
> }
>
> ret = get_next_event(ec_dev);
> - if (ret < 0)
> + if (ret <= 0)
> return ret;
>
> if (wake_event) {
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index e44e3ec8a9c7..de8b588c8776 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -317,7 +317,9 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev);
> * @wake_event: Pointer to a bool set to true upon return if the event might
> be
> * treated as a wake event. Ignored if null.
> *
> - * Return: 0 on success or negative error code.
> + * Return: negative error code on errors; 0 for no data; or else number of
> + * bytes received (i.e., an event was retrieved successfully). Event types
> are
> + * written out to @ec_dev->event_data.event_type on success.
> */
> int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
>
> @@ -329,7 +331,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
> bool *wake_event);
> * events raised and call the functions in the ec notifier. This function
> * is a helper to know which events are raised.
> *
> - * Return: 0 on success or negative error code.
> + * Return: 0 on error or non-zero bitmask of one or more EC_HOST_EVENT_*.
> */
> u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>
> --
> 2.19.1.930.g4563a0d9d0-goog
>
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
ble...@google.com
Chromium OS Project
ble...@chromium.org
signature.asc
Description: PGP signature
Re: [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes
Hi Brian,
On Wed, Nov 07, 2018 at 06:49:38PM -0800, Brian Norris wrote:
> cros_ec_get_next_event() is documented to return 0 for success and
> negative for errors. It currently returns negative for some errors, and
> non-negative (number of bytes received) for success (including some "no
> data available" responses as zero). This mostly works out OK, because the
> callers were more or less ignoring the documentation, and only treating
> positive values as success (and indepdently checking the modification of
> 'wakeup').
>
> Let's button this up by avoiding pretending to handle event/wakeup
> distinctions when no event info was retrieved (i.e., returned 0 bytes).
> And fix the documentation of cros_ec_get_host_event() and
> cros_ec_get_next_event() to accurately describe their behavior.
>
> Signed-off-by: Brian Norris
Applied.
Thanks,
Benson
> ---
> v1 -> v2:
> * don't make as many changes to the API -- just fix the documentation
>and a few corner cases instead
> ---
> drivers/platform/chrome/cros_ec_proto.c | 4 ++--
> include/linux/mfd/cros_ec.h | 6 --
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c
> b/drivers/platform/chrome/cros_ec_proto.c
> index b6fd4838f60f..fff67b389c87 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -580,7 +580,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
> bool *wake_event)
>
> if (!ec_dev->mkbp_event_supported) {
> ret = get_keyboard_state_event(ec_dev);
> - if (ret < 0)
> + if (ret <= 0)
> return ret;
>
> if (wake_event)
> @@ -590,7 +590,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
> bool *wake_event)
> }
>
> ret = get_next_event(ec_dev);
> - if (ret < 0)
> + if (ret <= 0)
> return ret;
>
> if (wake_event) {
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index e44e3ec8a9c7..de8b588c8776 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -317,7 +317,9 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev);
> * @wake_event: Pointer to a bool set to true upon return if the event might
> be
> * treated as a wake event. Ignored if null.
> *
> - * Return: 0 on success or negative error code.
> + * Return: negative error code on errors; 0 for no data; or else number of
> + * bytes received (i.e., an event was retrieved successfully). Event types
> are
> + * written out to @ec_dev->event_data.event_type on success.
> */
> int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
>
> @@ -329,7 +331,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
> bool *wake_event);
> * events raised and call the functions in the ec notifier. This function
> * is a helper to know which events are raised.
> *
> - * Return: 0 on success or negative error code.
> + * Return: 0 on error or non-zero bitmask of one or more EC_HOST_EVENT_*.
> */
> u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>
> --
> 2.19.1.930.g4563a0d9d0-goog
>
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
ble...@google.com
Chromium OS Project
ble...@chromium.org
signature.asc
Description: PGP signature
Re: [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes
On Wed, 07 Nov 2018, Brian Norris wrote: > cros_ec_get_next_event() is documented to return 0 for success and > negative for errors. It currently returns negative for some errors, and > non-negative (number of bytes received) for success (including some "no > data available" responses as zero). This mostly works out OK, because the > callers were more or less ignoring the documentation, and only treating > positive values as success (and indepdently checking the modification of > 'wakeup'). > > Let's button this up by avoiding pretending to handle event/wakeup > distinctions when no event info was retrieved (i.e., returned 0 bytes). > And fix the documentation of cros_ec_get_host_event() and > cros_ec_get_next_event() to accurately describe their behavior. > > Signed-off-by: Brian Norris > --- > v1 -> v2: > * don't make as many changes to the API -- just fix the documentation >and a few corner cases instead > --- > drivers/platform/chrome/cros_ec_proto.c | 4 ++-- > include/linux/mfd/cros_ec.h | 6 -- > 2 files changed, 6 insertions(+), 4 deletions(-) Acked-by: Lee Jones -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes
On Wed, 07 Nov 2018, Brian Norris wrote: > cros_ec_get_next_event() is documented to return 0 for success and > negative for errors. It currently returns negative for some errors, and > non-negative (number of bytes received) for success (including some "no > data available" responses as zero). This mostly works out OK, because the > callers were more or less ignoring the documentation, and only treating > positive values as success (and indepdently checking the modification of > 'wakeup'). > > Let's button this up by avoiding pretending to handle event/wakeup > distinctions when no event info was retrieved (i.e., returned 0 bytes). > And fix the documentation of cros_ec_get_host_event() and > cros_ec_get_next_event() to accurately describe their behavior. > > Signed-off-by: Brian Norris > --- > v1 -> v2: > * don't make as many changes to the API -- just fix the documentation >and a few corner cases instead > --- > drivers/platform/chrome/cros_ec_proto.c | 4 ++-- > include/linux/mfd/cros_ec.h | 6 -- > 2 files changed, 6 insertions(+), 4 deletions(-) Acked-by: Lee Jones -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
[PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes
cros_ec_get_next_event() is documented to return 0 for success and
negative for errors. It currently returns negative for some errors, and
non-negative (number of bytes received) for success (including some "no
data available" responses as zero). This mostly works out OK, because the
callers were more or less ignoring the documentation, and only treating
positive values as success (and indepdently checking the modification of
'wakeup').
Let's button this up by avoiding pretending to handle event/wakeup
distinctions when no event info was retrieved (i.e., returned 0 bytes).
And fix the documentation of cros_ec_get_host_event() and
cros_ec_get_next_event() to accurately describe their behavior.
Signed-off-by: Brian Norris
---
v1 -> v2:
* don't make as many changes to the API -- just fix the documentation
and a few corner cases instead
---
drivers/platform/chrome/cros_ec_proto.c | 4 ++--
include/linux/mfd/cros_ec.h | 6 --
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c
b/drivers/platform/chrome/cros_ec_proto.c
index b6fd4838f60f..fff67b389c87 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -580,7 +580,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
bool *wake_event)
if (!ec_dev->mkbp_event_supported) {
ret = get_keyboard_state_event(ec_dev);
- if (ret < 0)
+ if (ret <= 0)
return ret;
if (wake_event)
@@ -590,7 +590,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
bool *wake_event)
}
ret = get_next_event(ec_dev);
- if (ret < 0)
+ if (ret <= 0)
return ret;
if (wake_event) {
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index e44e3ec8a9c7..de8b588c8776 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -317,7 +317,9 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev);
* @wake_event: Pointer to a bool set to true upon return if the event might be
* treated as a wake event. Ignored if null.
*
- * Return: 0 on success or negative error code.
+ * Return: negative error code on errors; 0 for no data; or else number of
+ * bytes received (i.e., an event was retrieved successfully). Event types are
+ * written out to @ec_dev->event_data.event_type on success.
*/
int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
@@ -329,7 +331,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
bool *wake_event);
* events raised and call the functions in the ec notifier. This function
* is a helper to know which events are raised.
*
- * Return: 0 on success or negative error code.
+ * Return: 0 on error or non-zero bitmask of one or more EC_HOST_EVENT_*.
*/
u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
--
2.19.1.930.g4563a0d9d0-goog
[PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes
cros_ec_get_next_event() is documented to return 0 for success and
negative for errors. It currently returns negative for some errors, and
non-negative (number of bytes received) for success (including some "no
data available" responses as zero). This mostly works out OK, because the
callers were more or less ignoring the documentation, and only treating
positive values as success (and indepdently checking the modification of
'wakeup').
Let's button this up by avoiding pretending to handle event/wakeup
distinctions when no event info was retrieved (i.e., returned 0 bytes).
And fix the documentation of cros_ec_get_host_event() and
cros_ec_get_next_event() to accurately describe their behavior.
Signed-off-by: Brian Norris
---
v1 -> v2:
* don't make as many changes to the API -- just fix the documentation
and a few corner cases instead
---
drivers/platform/chrome/cros_ec_proto.c | 4 ++--
include/linux/mfd/cros_ec.h | 6 --
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c
b/drivers/platform/chrome/cros_ec_proto.c
index b6fd4838f60f..fff67b389c87 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -580,7 +580,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
bool *wake_event)
if (!ec_dev->mkbp_event_supported) {
ret = get_keyboard_state_event(ec_dev);
- if (ret < 0)
+ if (ret <= 0)
return ret;
if (wake_event)
@@ -590,7 +590,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
bool *wake_event)
}
ret = get_next_event(ec_dev);
- if (ret < 0)
+ if (ret <= 0)
return ret;
if (wake_event) {
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index e44e3ec8a9c7..de8b588c8776 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -317,7 +317,9 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev);
* @wake_event: Pointer to a bool set to true upon return if the event might be
* treated as a wake event. Ignored if null.
*
- * Return: 0 on success or negative error code.
+ * Return: negative error code on errors; 0 for no data; or else number of
+ * bytes received (i.e., an event was retrieved successfully). Event types are
+ * written out to @ec_dev->event_data.event_type on success.
*/
int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
@@ -329,7 +331,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
bool *wake_event);
* events raised and call the functions in the ec notifier. This function
* is a helper to know which events are raised.
*
- * Return: 0 on success or negative error code.
+ * Return: 0 on error or non-zero bitmask of one or more EC_HOST_EVENT_*.
*/
u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
--
2.19.1.930.g4563a0d9d0-goog
