Re: [PATCH 5/6] mfd: cros_ec_dev: Check communication with ec at resume

[Enric Balletbo i Serra]

Hi Gwendal,

On 23/02/18 10:01, Gwendal Grignou wrote:
> On Thu, Feb 22, 2018 at 1:41 AM, Enric Balletbo i Serra
>  wrote:
>> Hi Gwendal,
>>
>> On 22/02/18 03:20, Gwendal Grignou wrote:
>>> On Mon, Feb 19, 2018 at 2:41 PM, Enric Balletbo i Serra
>>>  wrote:
 From: Gwendal Grignou 
>>> This patch is not needed anymore. It was added to

 Send dummy command to EC at resume time, wait for status.
 If EC loses that command and return a status for the
 pre-suspend command, we will not interpret that status as an
 answer for an important command.

 Signed-off-by: Gwendal Grignou 
 Signed-off-by: Enric Balletbo i Serra 
>>> Hold on for this commit. We have only used it in 3.14 kernels. It
>>> looks like the issue has been addressed by a more elegant solution at
>>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0225a9de5971b99770344c04e0eba5b35ca68f71
>>
>> Just to make sure.
>>
>> I don't see this patch reverted in current chromeos-4.4, seems that both are
>> applied. The patch is still there [1]. So, to double check, can I remove this
>> patch from this series and add only the one that moves the system sleep pm 
>> ops
>> to late?
> You're right, it is still in chromeos-4.4. If it is still really
> needed, we will apply it later.
> Thanks,
> 
Sounds good, I'll resend another version removing this patch and adding the
other. Let's leave this patch in standby.

 Enric


> Gwendal.
>>
>> [1]
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/platform/chrome/cros_ec_dev.c#782
>>
 ---
  drivers/mfd/cros_ec_dev.c | 10 ++
  1 file changed, 10 insertions(+)

 diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
 index fc98e0564061..c901839317ae 100644
 --- a/drivers/mfd/cros_ec_dev.c
 +++ b/drivers/mfd/cros_ec_dev.c
 @@ -553,6 +553,16 @@ static __maybe_unused int ec_device_suspend(struct 
 device *dev)
  static __maybe_unused int ec_device_resume(struct device *dev)
  {
 struct cros_ec_dev *ec = dev_get_drvdata(dev);
 +   char msg[sizeof(struct ec_response_get_version) +
 +sizeof(CROS_EC_DEV_VERSION)];
 +   int ret;
 +
 +   /* Be sure the communication with the EC is reestablished */
 +   ret = ec_get_version(ec, msg, sizeof(msg));
 +   if (ret < 0) {
 +   dev_err(ec->ec_dev->dev, "No EC response at resume: %d\n", 
 ret);
 +   return 0;
>>> Besides, that looks wrong, we should try to resume the lightbar.
 +   }

 lb_resume(ec);

 --
 2.16.1

>>
>> Thanks,
>>   Enric
> 

Re: [PATCH 5/6] mfd: cros_ec_dev: Check communication with ec at resume

[Enric Balletbo i Serra]

Hi Gwendal,

On 23/02/18 10:01, Gwendal Grignou wrote:
> On Thu, Feb 22, 2018 at 1:41 AM, Enric Balletbo i Serra
>  wrote:
>> Hi Gwendal,
>>
>> On 22/02/18 03:20, Gwendal Grignou wrote:
>>> On Mon, Feb 19, 2018 at 2:41 PM, Enric Balletbo i Serra
>>>  wrote:
 From: Gwendal Grignou 
>>> This patch is not needed anymore. It was added to

 Send dummy command to EC at resume time, wait for status.
 If EC loses that command and return a status for the
 pre-suspend command, we will not interpret that status as an
 answer for an important command.

 Signed-off-by: Gwendal Grignou 
 Signed-off-by: Enric Balletbo i Serra 
>>> Hold on for this commit. We have only used it in 3.14 kernels. It
>>> looks like the issue has been addressed by a more elegant solution at
>>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0225a9de5971b99770344c04e0eba5b35ca68f71
>>
>> Just to make sure.
>>
>> I don't see this patch reverted in current chromeos-4.4, seems that both are
>> applied. The patch is still there [1]. So, to double check, can I remove this
>> patch from this series and add only the one that moves the system sleep pm 
>> ops
>> to late?
> You're right, it is still in chromeos-4.4. If it is still really
> needed, we will apply it later.
> Thanks,
> 
Sounds good, I'll resend another version removing this patch and adding the
other. Let's leave this patch in standby.

 Enric


> Gwendal.
>>
>> [1]
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/platform/chrome/cros_ec_dev.c#782
>>
 ---
  drivers/mfd/cros_ec_dev.c | 10 ++
  1 file changed, 10 insertions(+)

 diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
 index fc98e0564061..c901839317ae 100644
 --- a/drivers/mfd/cros_ec_dev.c
 +++ b/drivers/mfd/cros_ec_dev.c
 @@ -553,6 +553,16 @@ static __maybe_unused int ec_device_suspend(struct 
 device *dev)
  static __maybe_unused int ec_device_resume(struct device *dev)
  {
 struct cros_ec_dev *ec = dev_get_drvdata(dev);
 +   char msg[sizeof(struct ec_response_get_version) +
 +sizeof(CROS_EC_DEV_VERSION)];
 +   int ret;
 +
 +   /* Be sure the communication with the EC is reestablished */
 +   ret = ec_get_version(ec, msg, sizeof(msg));
 +   if (ret < 0) {
 +   dev_err(ec->ec_dev->dev, "No EC response at resume: %d\n", 
 ret);
 +   return 0;
>>> Besides, that looks wrong, we should try to resume the lightbar.
 +   }

 lb_resume(ec);

 --
 2.16.1

>>
>> Thanks,
>>   Enric
> 

Re: [PATCH 5/6] mfd: cros_ec_dev: Check communication with ec at resume

[Gwendal Grignou]

On Thu, Feb 22, 2018 at 1:41 AM, Enric Balletbo i Serra
 wrote:
> Hi Gwendal,
>
> On 22/02/18 03:20, Gwendal Grignou wrote:
>> On Mon, Feb 19, 2018 at 2:41 PM, Enric Balletbo i Serra
>>  wrote:
>>> From: Gwendal Grignou 
>> This patch is not needed anymore. It was added to
>>>
>>> Send dummy command to EC at resume time, wait for status.
>>> If EC loses that command and return a status for the
>>> pre-suspend command, we will not interpret that status as an
>>> answer for an important command.
>>>
>>> Signed-off-by: Gwendal Grignou 
>>> Signed-off-by: Enric Balletbo i Serra 
>> Hold on for this commit. We have only used it in 3.14 kernels. It
>> looks like the issue has been addressed by a more elegant solution at
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0225a9de5971b99770344c04e0eba5b35ca68f71
>
> Just to make sure.
>
> I don't see this patch reverted in current chromeos-4.4, seems that both are
> applied. The patch is still there [1]. So, to double check, can I remove this
> patch from this series and add only the one that moves the system sleep pm ops
> to late?
You're right, it is still in chromeos-4.4. If it is still really
needed, we will apply it later.
Thanks,

Gwendal.
>
> [1]
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/platform/chrome/cros_ec_dev.c#782
>
>>> ---
>>>  drivers/mfd/cros_ec_dev.c | 10 ++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>>> index fc98e0564061..c901839317ae 100644
>>> --- a/drivers/mfd/cros_ec_dev.c
>>> +++ b/drivers/mfd/cros_ec_dev.c
>>> @@ -553,6 +553,16 @@ static __maybe_unused int ec_device_suspend(struct 
>>> device *dev)
>>>  static __maybe_unused int ec_device_resume(struct device *dev)
>>>  {
>>> struct cros_ec_dev *ec = dev_get_drvdata(dev);
>>> +   char msg[sizeof(struct ec_response_get_version) +
>>> +sizeof(CROS_EC_DEV_VERSION)];
>>> +   int ret;
>>> +
>>> +   /* Be sure the communication with the EC is reestablished */
>>> +   ret = ec_get_version(ec, msg, sizeof(msg));
>>> +   if (ret < 0) {
>>> +   dev_err(ec->ec_dev->dev, "No EC response at resume: %d\n", 
>>> ret);
>>> +   return 0;
>> Besides, that looks wrong, we should try to resume the lightbar.
>>> +   }
>>>
>>> lb_resume(ec);
>>>
>>> --
>>> 2.16.1
>>>
>
> Thanks,
>   Enric

Re: [PATCH 5/6] mfd: cros_ec_dev: Check communication with ec at resume

[Gwendal Grignou]

On Thu, Feb 22, 2018 at 1:41 AM, Enric Balletbo i Serra
 wrote:
> Hi Gwendal,
>
> On 22/02/18 03:20, Gwendal Grignou wrote:
>> On Mon, Feb 19, 2018 at 2:41 PM, Enric Balletbo i Serra
>>  wrote:
>>> From: Gwendal Grignou 
>> This patch is not needed anymore. It was added to
>>>
>>> Send dummy command to EC at resume time, wait for status.
>>> If EC loses that command and return a status for the
>>> pre-suspend command, we will not interpret that status as an
>>> answer for an important command.
>>>
>>> Signed-off-by: Gwendal Grignou 
>>> Signed-off-by: Enric Balletbo i Serra 
>> Hold on for this commit. We have only used it in 3.14 kernels. It
>> looks like the issue has been addressed by a more elegant solution at
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0225a9de5971b99770344c04e0eba5b35ca68f71
>
> Just to make sure.
>
> I don't see this patch reverted in current chromeos-4.4, seems that both are
> applied. The patch is still there [1]. So, to double check, can I remove this
> patch from this series and add only the one that moves the system sleep pm ops
> to late?
You're right, it is still in chromeos-4.4. If it is still really
needed, we will apply it later.
Thanks,

Gwendal.
>
> [1]
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/platform/chrome/cros_ec_dev.c#782
>
>>> ---
>>>  drivers/mfd/cros_ec_dev.c | 10 ++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>>> index fc98e0564061..c901839317ae 100644
>>> --- a/drivers/mfd/cros_ec_dev.c
>>> +++ b/drivers/mfd/cros_ec_dev.c
>>> @@ -553,6 +553,16 @@ static __maybe_unused int ec_device_suspend(struct 
>>> device *dev)
>>>  static __maybe_unused int ec_device_resume(struct device *dev)
>>>  {
>>> struct cros_ec_dev *ec = dev_get_drvdata(dev);
>>> +   char msg[sizeof(struct ec_response_get_version) +
>>> +sizeof(CROS_EC_DEV_VERSION)];
>>> +   int ret;
>>> +
>>> +   /* Be sure the communication with the EC is reestablished */
>>> +   ret = ec_get_version(ec, msg, sizeof(msg));
>>> +   if (ret < 0) {
>>> +   dev_err(ec->ec_dev->dev, "No EC response at resume: %d\n", 
>>> ret);
>>> +   return 0;
>> Besides, that looks wrong, we should try to resume the lightbar.
>>> +   }
>>>
>>> lb_resume(ec);
>>>
>>> --
>>> 2.16.1
>>>
>
> Thanks,
>   Enric

Re: [PATCH 5/6] mfd: cros_ec_dev: Check communication with ec at resume

[Enric Balletbo i Serra]

Hi Gwendal,

On 22/02/18 03:20, Gwendal Grignou wrote:
> On Mon, Feb 19, 2018 at 2:41 PM, Enric Balletbo i Serra
>  wrote:
>> From: Gwendal Grignou 
> This patch is not needed anymore. It was added to
>>
>> Send dummy command to EC at resume time, wait for status.
>> If EC loses that command and return a status for the
>> pre-suspend command, we will not interpret that status as an
>> answer for an important command.
>>
>> Signed-off-by: Gwendal Grignou 
>> Signed-off-by: Enric Balletbo i Serra 
> Hold on for this commit. We have only used it in 3.14 kernels. It
> looks like the issue has been addressed by a more elegant solution at
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0225a9de5971b99770344c04e0eba5b35ca68f71

Just to make sure.

I don't see this patch reverted in current chromeos-4.4, seems that both are
applied. The patch is still there [1]. So, to double check, can I remove this
patch from this series and add only the one that moves the system sleep pm ops
to late?

[1]
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/platform/chrome/cros_ec_dev.c#782

>> ---
>>  drivers/mfd/cros_ec_dev.c | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>> index fc98e0564061..c901839317ae 100644
>> --- a/drivers/mfd/cros_ec_dev.c
>> +++ b/drivers/mfd/cros_ec_dev.c
>> @@ -553,6 +553,16 @@ static __maybe_unused int ec_device_suspend(struct 
>> device *dev)
>>  static __maybe_unused int ec_device_resume(struct device *dev)
>>  {
>> struct cros_ec_dev *ec = dev_get_drvdata(dev);
>> +   char msg[sizeof(struct ec_response_get_version) +
>> +sizeof(CROS_EC_DEV_VERSION)];
>> +   int ret;
>> +
>> +   /* Be sure the communication with the EC is reestablished */
>> +   ret = ec_get_version(ec, msg, sizeof(msg));
>> +   if (ret < 0) {
>> +   dev_err(ec->ec_dev->dev, "No EC response at resume: %d\n", 
>> ret);
>> +   return 0;
> Besides, that looks wrong, we should try to resume the lightbar.
>> +   }
>>
>> lb_resume(ec);
>>
>> --
>> 2.16.1
>>

Thanks,
  Enric

Re: [PATCH 5/6] mfd: cros_ec_dev: Check communication with ec at resume

[Enric Balletbo i Serra]

Hi Gwendal,

On 22/02/18 03:20, Gwendal Grignou wrote:
> On Mon, Feb 19, 2018 at 2:41 PM, Enric Balletbo i Serra
>  wrote:
>> From: Gwendal Grignou 
> This patch is not needed anymore. It was added to
>>
>> Send dummy command to EC at resume time, wait for status.
>> If EC loses that command and return a status for the
>> pre-suspend command, we will not interpret that status as an
>> answer for an important command.
>>
>> Signed-off-by: Gwendal Grignou 
>> Signed-off-by: Enric Balletbo i Serra 
> Hold on for this commit. We have only used it in 3.14 kernels. It
> looks like the issue has been addressed by a more elegant solution at
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0225a9de5971b99770344c04e0eba5b35ca68f71

Just to make sure.

I don't see this patch reverted in current chromeos-4.4, seems that both are
applied. The patch is still there [1]. So, to double check, can I remove this
patch from this series and add only the one that moves the system sleep pm ops
to late?

[1]
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/platform/chrome/cros_ec_dev.c#782

>> ---
>>  drivers/mfd/cros_ec_dev.c | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>> index fc98e0564061..c901839317ae 100644
>> --- a/drivers/mfd/cros_ec_dev.c
>> +++ b/drivers/mfd/cros_ec_dev.c
>> @@ -553,6 +553,16 @@ static __maybe_unused int ec_device_suspend(struct 
>> device *dev)
>>  static __maybe_unused int ec_device_resume(struct device *dev)
>>  {
>> struct cros_ec_dev *ec = dev_get_drvdata(dev);
>> +   char msg[sizeof(struct ec_response_get_version) +
>> +sizeof(CROS_EC_DEV_VERSION)];
>> +   int ret;
>> +
>> +   /* Be sure the communication with the EC is reestablished */
>> +   ret = ec_get_version(ec, msg, sizeof(msg));
>> +   if (ret < 0) {
>> +   dev_err(ec->ec_dev->dev, "No EC response at resume: %d\n", 
>> ret);
>> +   return 0;
> Besides, that looks wrong, we should try to resume the lightbar.
>> +   }
>>
>> lb_resume(ec);
>>
>> --
>> 2.16.1
>>

Thanks,
  Enric

Re: [PATCH 5/6] mfd: cros_ec_dev: Check communication with ec at resume

[Gwendal Grignou]

On Mon, Feb 19, 2018 at 2:41 PM, Enric Balletbo i Serra
 wrote:
> From: Gwendal Grignou 
This patch is not needed anymore. It was added to
>
> Send dummy command to EC at resume time, wait for status.
> If EC loses that command and return a status for the
> pre-suspend command, we will not interpret that status as an
> answer for an important command.
>
> Signed-off-by: Gwendal Grignou 
> Signed-off-by: Enric Balletbo i Serra 
Hold on for this commit. We have only used it in 3.14 kernels. It
looks like the issue has been addressed by a more elegant solution at
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0225a9de5971b99770344c04e0eba5b35ca68f71
> ---
>  drivers/mfd/cros_ec_dev.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index fc98e0564061..c901839317ae 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -553,6 +553,16 @@ static __maybe_unused int ec_device_suspend(struct 
> device *dev)
>  static __maybe_unused int ec_device_resume(struct device *dev)
>  {
> struct cros_ec_dev *ec = dev_get_drvdata(dev);
> +   char msg[sizeof(struct ec_response_get_version) +
> +sizeof(CROS_EC_DEV_VERSION)];
> +   int ret;
> +
> +   /* Be sure the communication with the EC is reestablished */
> +   ret = ec_get_version(ec, msg, sizeof(msg));
> +   if (ret < 0) {
> +   dev_err(ec->ec_dev->dev, "No EC response at resume: %d\n", 
> ret);
> +   return 0;
Besides, that looks wrong, we should try to resume the lightbar.
> +   }
>
> lb_resume(ec);
>
> --
> 2.16.1
>

Re: [PATCH 5/6] mfd: cros_ec_dev: Check communication with ec at resume

[Gwendal Grignou]

On Mon, Feb 19, 2018 at 2:41 PM, Enric Balletbo i Serra
 wrote:
> From: Gwendal Grignou 
This patch is not needed anymore. It was added to
>
> Send dummy command to EC at resume time, wait for status.
> If EC loses that command and return a status for the
> pre-suspend command, we will not interpret that status as an
> answer for an important command.
>
> Signed-off-by: Gwendal Grignou 
> Signed-off-by: Enric Balletbo i Serra 
Hold on for this commit. We have only used it in 3.14 kernels. It
looks like the issue has been addressed by a more elegant solution at
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0225a9de5971b99770344c04e0eba5b35ca68f71
> ---
>  drivers/mfd/cros_ec_dev.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index fc98e0564061..c901839317ae 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -553,6 +553,16 @@ static __maybe_unused int ec_device_suspend(struct 
> device *dev)
>  static __maybe_unused int ec_device_resume(struct device *dev)
>  {
> struct cros_ec_dev *ec = dev_get_drvdata(dev);
> +   char msg[sizeof(struct ec_response_get_version) +
> +sizeof(CROS_EC_DEV_VERSION)];
> +   int ret;
> +
> +   /* Be sure the communication with the EC is reestablished */
> +   ret = ec_get_version(ec, msg, sizeof(msg));
> +   if (ret < 0) {
> +   dev_err(ec->ec_dev->dev, "No EC response at resume: %d\n", 
> ret);
> +   return 0;
Besides, that looks wrong, we should try to resume the lightbar.
> +   }
>
> lb_resume(ec);
>
> --
> 2.16.1
>