[v1] platform/chrome: cros_ec_lightbar: Check if ec supports suspend commands

[PATCH] platform/chrome: cros_ec_lightbar: Check if ec supports suspend commands

Posted by Brady Norander 3 months, 2 weeks ago

The Chromebook Pixel 2013 (Link)'s ec does not support the lightbar manual suspend commands.
As a result, attempting to suspend the device fails and prints the following error:

    cros-ec-lightbar cros-ec-lightbar.3.auto: PM: dpm_run_callback(): platform_pm_suspend returns -22
    cros-ec-lightbar cros-ec-lightbar.3.auto: PM: failed to suspend: error -22
    PM: Some devices failed to suspend, or early wake event detected

Check the return value of lb_manual_suspend_ctrl in cros_ec_lightbar_probe and disable manual
suspend control if an error is returned.

Signed-off-by: Brady Norander <bradynorander@gmail.com>

diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index 87634f6921b7..1b7e4c4e18bc 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -30,6 +30,13 @@ static unsigned long lb_interval_jiffies = 50 * HZ / 1000;
  */
 static bool userspace_control;
 
+/*
+ * Whether or not the lightbar supports the manual suspend commands.
+ * The Pixel 2013 (Link) does not while all other devices with a
+ * lightbar do.
+*/
+static bool has_manual_suspend;
+
 static ssize_t interval_msec_show(struct device *dev,
 				  struct device_attribute *attr, char *buf)
 {
@@ -550,7 +557,7 @@ static int cros_ec_lightbar_probe(struct platform_device *pd)
 		return -ENODEV;
 
 	/* Take control of the lightbar from the EC. */
-	lb_manual_suspend_ctrl(ec_dev, 1);
+	has_manual_suspend = (lb_manual_suspend_ctrl(ec_dev, 1) >= 0);
 
 	ret = sysfs_create_group(&ec_dev->class_dev.kobj,
 				 &cros_ec_lightbar_attr_group);
@@ -569,14 +576,15 @@ static void cros_ec_lightbar_remove(struct platform_device *pd)
 			   &cros_ec_lightbar_attr_group);
 
 	/* Let the EC take over the lightbar again. */
-	lb_manual_suspend_ctrl(ec_dev, 0);
+	if (has_manual_suspend)
+		lb_manual_suspend_ctrl(ec_dev, 0);
 }
 
 static int __maybe_unused cros_ec_lightbar_resume(struct device *dev)
 {
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 
-	if (userspace_control)
+	if (userspace_control || !has_manual_suspend)
 		return 0;
 
 	return lb_send_empty_cmd(ec_dev, LIGHTBAR_CMD_RESUME);
@@ -586,7 +594,7 @@ static int __maybe_unused cros_ec_lightbar_suspend(struct device *dev)
 {
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 
-	if (userspace_control)
+	if (userspace_control || !has_manual_suspend)
 		return 0;
 
 	return lb_send_empty_cmd(ec_dev, LIGHTBAR_CMD_SUSPEND);
-- 
2.51.0

Re: [PATCH] platform/chrome: cros_ec_lightbar: Check if ec supports suspend commands

Posted by Benson Leung 3 months, 1 week ago

On Thu, Oct 23, 2025 at 07:42:40PM -0400, Brady Norander wrote:
> The Chromebook Pixel 2013 (Link)'s ec does not support the lightbar manual suspend commands.
> As a result, attempting to suspend the device fails and prints the following error:
> 
>     cros-ec-lightbar cros-ec-lightbar.3.auto: PM: dpm_run_callback(): platform_pm_suspend returns -22
>     cros-ec-lightbar cros-ec-lightbar.3.auto: PM: failed to suspend: error -22
>     PM: Some devices failed to suspend, or early wake event detected
> 
> Check the return value of lb_manual_suspend_ctrl in cros_ec_lightbar_probe and disable manual
> suspend control if an error is returned.
> 
> Signed-off-by: Brady Norander <bradynorander@gmail.com>

Reviewed-by: Benson Leung <bleung@chromium.org>

> 
> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
> index 87634f6921b7..1b7e4c4e18bc 100644
> --- a/drivers/platform/chrome/cros_ec_lightbar.c
> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
> @@ -30,6 +30,13 @@ static unsigned long lb_interval_jiffies = 50 * HZ / 1000;
>   */
>  static bool userspace_control;
>  
> +/*
> + * Whether or not the lightbar supports the manual suspend commands.
> + * The Pixel 2013 (Link) does not while all other devices with a
> + * lightbar do.
> +*/
> +static bool has_manual_suspend;
> +
>  static ssize_t interval_msec_show(struct device *dev,
>  				  struct device_attribute *attr, char *buf)
>  {
> @@ -550,7 +557,7 @@ static int cros_ec_lightbar_probe(struct platform_device *pd)
>  		return -ENODEV;
>  
>  	/* Take control of the lightbar from the EC. */
> -	lb_manual_suspend_ctrl(ec_dev, 1);
> +	has_manual_suspend = (lb_manual_suspend_ctrl(ec_dev, 1) >= 0);
>  
>  	ret = sysfs_create_group(&ec_dev->class_dev.kobj,
>  				 &cros_ec_lightbar_attr_group);
> @@ -569,14 +576,15 @@ static void cros_ec_lightbar_remove(struct platform_device *pd)
>  			   &cros_ec_lightbar_attr_group);
>  
>  	/* Let the EC take over the lightbar again. */
> -	lb_manual_suspend_ctrl(ec_dev, 0);
> +	if (has_manual_suspend)
> +		lb_manual_suspend_ctrl(ec_dev, 0);
>  }
>  
>  static int __maybe_unused cros_ec_lightbar_resume(struct device *dev)
>  {
>  	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
>  
> -	if (userspace_control)
> +	if (userspace_control || !has_manual_suspend)
>  		return 0;
>  
>  	return lb_send_empty_cmd(ec_dev, LIGHTBAR_CMD_RESUME);
> @@ -586,7 +594,7 @@ static int __maybe_unused cros_ec_lightbar_suspend(struct device *dev)
>  {
>  	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
>  
> -	if (userspace_control)
> +	if (userspace_control || !has_manual_suspend)
>  		return 0;
>  
>  	return lb_send_empty_cmd(ec_dev, LIGHTBAR_CMD_SUSPEND);
> -- 
> 2.51.0
>

Re: [PATCH] platform/chrome: cros_ec_lightbar: Check if ec supports suspend commands

Posted by Tzung-Bi Shih 3 months, 2 weeks ago

On Thu, Oct 23, 2025 at 07:42:40PM -0400, Brady Norander wrote:
> The Chromebook Pixel 2013 (Link)'s ec does not support the lightbar manual suspend commands.
> As a result, attempting to suspend the device fails and prints the following error:
> 
>     cros-ec-lightbar cros-ec-lightbar.3.auto: PM: dpm_run_callback(): platform_pm_suspend returns -22
>     cros-ec-lightbar cros-ec-lightbar.3.auto: PM: failed to suspend: error -22
>     PM: Some devices failed to suspend, or early wake event detected

Thanks for looking on this.

> @@ -550,7 +557,7 @@ static int cros_ec_lightbar_probe(struct platform_device *pd)
>  		return -ENODEV;
>  
>  	/* Take control of the lightbar from the EC. */
> -	lb_manual_suspend_ctrl(ec_dev, 1);
> +	has_manual_suspend = (lb_manual_suspend_ctrl(ec_dev, 1) >= 0);

The driver doesn't emit an error if lb_manual_suspend_ctrl() returns an
error in the first place.  However, I think `has_manual_suspend` should
only check for -22.  E.g.:

has_manual_suspend = lb_manual_suspend_ctrl(...) != -EINVAL;

Re: [PATCH] platform/chrome: cros_ec_lightbar: Check if ec supports suspend commands

Posted by Brady Norander 3 months, 1 week ago

On 10/26/25 23:54, Tzung-Bi Shih wrote:
> On Thu, Oct 23, 2025 at 07:42:40PM -0400, Brady Norander wrote:
>> @@ -550,7 +557,7 @@ static int cros_ec_lightbar_probe(struct platform_device *pd)
>>   		return -ENODEV;
>>   
>>   	/* Take control of the lightbar from the EC. */
>> -	lb_manual_suspend_ctrl(ec_dev, 1);
>> +	has_manual_suspend = (lb_manual_suspend_ctrl(ec_dev, 1) >= 0);
> 
> The driver doesn't emit an error if lb_manual_suspend_ctrl() returns an
> error in the first place.  However, I think `has_manual_suspend` should
> only check for -22.  E.g.:
> 
> has_manual_suspend = lb_manual_suspend_ctrl(...) != -EINVAL;

Thanks for the review.
I originally decided to catch all errors to be on the safe side. After 
thinking about it some more, I agree that it should only check for
-EINVAL. Will send a v2.