[PATCH] power_supply: rt9455_charger: Properly notify userspace about charging events

[PATCH] power_supply: rt9455_charger: Properly notify userspace about charging events

[Anda-Maria Nicolae]

Do not call power_supply_changed() when CHRVPI interrupt has occurred.
CHRVPI interrupt occurs when the charger is connected to or disconnected
from the power source. Call power_supply_changed() after PWR_RDY bit is
read, to distinguish between connection to or disconnection from the power
source.
Also, call power_supply_changed() after the battery is reconnected to the
charger, to notify userspace that the battery is no longer absent.

Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@intel.com>
---
 drivers/power/rt9455_charger.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/power/rt9455_charger.c b/drivers/power/rt9455_charger.c
index 08baac6..a49a9d4 100644
--- a/drivers/power/rt9455_charger.c
+++ b/drivers/power/rt9455_charger.c
@@ -973,7 +973,6 @@ static int rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
 
 	if (irq2 & GET_MASK(F_CHRVPI)) {
 		dev_dbg(dev, "Charger fault occurred\n");
-		alert_userspace = true;
 		/*
 		 * CHRVPI bit is set in 2 cases:
 		 * 1. when the power source is connected to the charger.
@@ -981,6 +980,9 @@ static int rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
 		 * To identify the case, PWR_RDY bit is checked. Because
 		 * PWR_RDY bit is set / cleared after CHRVPI interrupt is
 		 * triggered, it is used delayed_work to later read PWR_RDY bit.
+		 * Also, do not set to true alert_userspace, because there is no
+		 * need to notify userspace when CHRVPI interrupt has occurred.
+		 * Userspace will be notified after PWR_RDY bit is read.
 		 */
 		queue_delayed_work(system_power_efficient_wq,
 				   &info->pwr_rdy_work,
@@ -1178,7 +1180,7 @@ static irqreturn_t rt9455_irq_handler_thread(int irq, void *data)
 		/*
 		 * Sometimes, an interrupt occurs while rt9455_probe() function
 		 * is executing and power_supply_register() is not yet called.
-		 * Do not call power_supply_charged() in this case.
+		 * Do not call power_supply_changed() in this case.
 		 */
 		if (info->charger)
 			power_supply_changed(info->charger);
@@ -1478,6 +1480,11 @@ static void rt9455_pwr_rdy_work_callback(struct work_struct *work)
 				   RT9455_MAX_CHARGING_TIME * HZ);
 		break;
 	}
+	/*
+	 * Notify userspace that the charger has been either connected to or
+	 * disconnected from the power source.
+	 */
+	power_supply_changed(info->charger);
 }
 
 static void rt9455_max_charging_time_work_callback(struct work_struct *work)
@@ -1533,6 +1540,11 @@ static void rt9455_batt_presence_work_callback(struct work_struct *work)
 			if (ret)
 				dev_err(dev, "Failed to unmask BATAB interrupt\n");
 		}
+		/*
+		 * Notify userspace that the battery is now connected to the
+		 * charger.
+		 */
+		power_supply_changed(info->charger);
 	}
 }
 
-- 
1.7.9.5

Re: [PATCH] power_supply: rt9455_charger: Properly notify userspace about charging events

[Anda Nicolae]

On Wed, Jun 17, 2015 at 7:28 PM, Anda-Maria Nicolae
<anda-maria.nicolae@intel.com> wrote:
> Do not call power_supply_changed() when CHRVPI interrupt has occurred.
> CHRVPI interrupt occurs when the charger is connected to or disconnected
> from the power source. Call power_supply_changed() after PWR_RDY bit is
> read, to distinguish between connection to or disconnection from the power
> source.
> Also, call power_supply_changed() after the battery is reconnected to the
> charger, to notify userspace that the battery is no longer absent.
>
> Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@intel.com>
> ---
>  drivers/power/rt9455_charger.c |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/rt9455_charger.c b/drivers/power/rt9455_charger.c
> index 08baac6..a49a9d4 100644
> --- a/drivers/power/rt9455_charger.c
> +++ b/drivers/power/rt9455_charger.c
> @@ -973,7 +973,6 @@ static int rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
>
>         if (irq2 & GET_MASK(F_CHRVPI)) {
>                 dev_dbg(dev, "Charger fault occurred\n");
> -               alert_userspace = true;
>                 /*
>                  * CHRVPI bit is set in 2 cases:
>                  * 1. when the power source is connected to the charger.
> @@ -981,6 +980,9 @@ static int rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
>                  * To identify the case, PWR_RDY bit is checked. Because
>                  * PWR_RDY bit is set / cleared after CHRVPI interrupt is
>                  * triggered, it is used delayed_work to later read PWR_RDY bit.
> +                * Also, do not set to true alert_userspace, because there is no
> +                * need to notify userspace when CHRVPI interrupt has occurred.
> +                * Userspace will be notified after PWR_RDY bit is read.
>                  */
>                 queue_delayed_work(system_power_efficient_wq,
>                                    &info->pwr_rdy_work,
> @@ -1178,7 +1180,7 @@ static irqreturn_t rt9455_irq_handler_thread(int irq, void *data)
>                 /*
>                  * Sometimes, an interrupt occurs while rt9455_probe() function
>                  * is executing and power_supply_register() is not yet called.
> -                * Do not call power_supply_charged() in this case.
> +                * Do not call power_supply_changed() in this case.
>                  */
>                 if (info->charger)
>                         power_supply_changed(info->charger);
> @@ -1478,6 +1480,11 @@ static void rt9455_pwr_rdy_work_callback(struct work_struct *work)
>                                    RT9455_MAX_CHARGING_TIME * HZ);
>                 break;
>         }
> +       /*
> +        * Notify userspace that the charger has been either connected to or
> +        * disconnected from the power source.
> +        */
> +       power_supply_changed(info->charger);
>  }
>
>  static void rt9455_max_charging_time_work_callback(struct work_struct *work)
> @@ -1533,6 +1540,11 @@ static void rt9455_batt_presence_work_callback(struct work_struct *work)
>                         if (ret)
>                                 dev_err(dev, "Failed to unmask BATAB interrupt\n");
>                 }
> +               /*
> +                * Notify userspace that the battery is now connected to the
> +                * charger.
> +                */
> +               power_supply_changed(info->charger);
>         }
>  }
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Hi all,

Can you please take a look at the patch listed above?

Thanks,
Anda

Re: [PATCH] power_supply: rt9455_charger: Properly notify userspace about charging events

[Krzysztof Kozlowski]

2015-06-18 1:28 GMT+09:00 Anda-Maria Nicolae <anda-maria.nicolae@intel.com>:
> Do not call power_supply_changed() when CHRVPI interrupt has occurred.

What I cannot find here is the answer to: why? I am not familiar with
the driver so this is just guessing that you wanted to notify
userspace about connected/disconnected charger. In both cases (before
and after the patch) this is achieved, isn't it?

Best regards,
Krzysztof


> CHRVPI interrupt occurs when the charger is connected to or disconnected
> from the power source. Call power_supply_changed() after PWR_RDY bit is
> read, to distinguish between connection to or disconnection from the power
> source.
> Also, call power_supply_changed() after the battery is reconnected to the
> charger, to notify userspace that the battery is no longer absent.
>
> Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@intel.com>
> ---
>  drivers/power/rt9455_charger.c |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/rt9455_charger.c b/drivers/power/rt9455_charger.c
> index 08baac6..a49a9d4 100644
> --- a/drivers/power/rt9455_charger.c
> +++ b/drivers/power/rt9455_charger.c
> @@ -973,7 +973,6 @@ static int rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
>
>         if (irq2 & GET_MASK(F_CHRVPI)) {
>                 dev_dbg(dev, "Charger fault occurred\n");
> -               alert_userspace = true;
>                 /*
>                  * CHRVPI bit is set in 2 cases:
>                  * 1. when the power source is connected to the charger.
> @@ -981,6 +980,9 @@ static int rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
>                  * To identify the case, PWR_RDY bit is checked. Because
>                  * PWR_RDY bit is set / cleared after CHRVPI interrupt is
>                  * triggered, it is used delayed_work to later read PWR_RDY bit.
> +                * Also, do not set to true alert_userspace, because there is no
> +                * need to notify userspace when CHRVPI interrupt has occurred.
> +                * Userspace will be notified after PWR_RDY bit is read.
>                  */
>                 queue_delayed_work(system_power_efficient_wq,
>                                    &info->pwr_rdy_work,
> @@ -1178,7 +1180,7 @@ static irqreturn_t rt9455_irq_handler_thread(int irq, void *data)
>                 /*
>                  * Sometimes, an interrupt occurs while rt9455_probe() function
>                  * is executing and power_supply_register() is not yet called.
> -                * Do not call power_supply_charged() in this case.
> +                * Do not call power_supply_changed() in this case.
>                  */
>                 if (info->charger)
>                         power_supply_changed(info->charger);
> @@ -1478,6 +1480,11 @@ static void rt9455_pwr_rdy_work_callback(struct work_struct *work)
>                                    RT9455_MAX_CHARGING_TIME * HZ);
>                 break;
>         }
> +       /*
> +        * Notify userspace that the charger has been either connected to or
> +        * disconnected from the power source.
> +        */
> +       power_supply_changed(info->charger);
>  }
>
>  static void rt9455_max_charging_time_work_callback(struct work_struct *work)
> @@ -1533,6 +1540,11 @@ static void rt9455_batt_presence_work_callback(struct work_struct *work)
>                         if (ret)
>                                 dev_err(dev, "Failed to unmask BATAB interrupt\n");
>                 }
> +               /*
> +                * Notify userspace that the battery is now connected to the
> +                * charger.
> +                */
> +               power_supply_changed(info->charger);
>         }
>  }
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] power_supply: rt9455_charger: Properly notify userspace about charging events

[Anda-Maria Nicolae]

Hello Krzysztof,

The information provided to userspace is not accurate in current driver 
implementation and it is accurate with this patch.
Also, when the battery is reconnected to the charger (after it has been 
disconnected from the charger), the charger does not trigger any 
interrupt. Since power_supply_changed() is called only in interrupt 
handler, the userspace is never notified that the battery is reconnected.

Let me explain to you why power_supply_changed() is called in 
rt9455_pwr_rdy_work_callback() and not in interrupt handler, when CHRVPI 
interrupt occurs.

I created a userspace tool that waits for notifications from the driver. 
A notification is sent when the driver calls power_supply_changed().
When my userspace tool receives a notification, it reads the files from 
/sys/class/power_supply/rt9455-charger/ folder. The tool reads each file 
this to figure it out what has been changed, since the driver notifies 
the tool only when a change has been made.
These files include /sys/class/power_supply/rt9455-charger/online. This 
file displays 1 if the power source is connected to the charger and 0 
otherwise.
When the userspace tool reads 
/sys/class/power_supply/rt9455-charger/online, 
rt9455_charger_get_online() function from the driver is called.
rt9455_charger_get_online() returns the value of PWR_RDY bit.
But whenever the power source is connected to / disconnected from the 
charger, RT9455 first sets CHRVPI interrupt bit, triggers an interrupt, 
and after almost 1-2 seconds, the charger also updates PWR_RDY bit.
So, if we check PWR_RDY bit immediately after CHRVPI bit is set, we will 
not obtain accurate info about the change that has been made (i.e.: if 
the power source was connected or disconnected).
This is why the driver has struct delayed_work pwr_rdy_work, which is 
scheduled whenever CHRVPI interrupt bit is set.
This is why in this patch, in rt9455_pwr_rdy_work_callback(), 
power_supply_changed() is called, and not in interrupt handler.

Let me explain to you why power_supply_changed() is called in 
rt9455_batt_presence_work_callback().

As previously stated, the userspace tool is never notified that the 
battery is reconnected. But since the driver uses struct delayed_work 
batt_presence_work, to determine whether the battery has been 
reconnected, we can call power_supply_changed() whenever the driver 
determines the battery is reconnected (i.e. BATAB interrupt bit is cleared).
This is why in this patch, in rt9455_batt_presence_work_callback(), 
power_supply_changed() is called.

On 07/02/2015 03:49 AM, Krzysztof Kozlowski wrote:
> 2015-06-18 1:28 GMT+09:00 Anda-Maria Nicolae <anda-maria.nicolae@intel.com>:
>> Do not call power_supply_changed() when CHRVPI interrupt has occurred.
> What I cannot find here is the answer to: why? I am not familiar with
> the driver so this is just guessing that you wanted to notify
> userspace about connected/disconnected charger. In both cases (before
> and after the patch) this is achieved, isn't it?
>
> Best regards,
> Krzysztof
>
>
>> CHRVPI interrupt occurs when the charger is connected to or disconnected
>> from the power source. Call power_supply_changed() after PWR_RDY bit is
>> read, to distinguish between connection to or disconnection from the power
>> source.
>> Also, call power_supply_changed() after the battery is reconnected to the
>> charger, to notify userspace that the battery is no longer absent.
>>
>> Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@intel.com>
>> ---
>>   drivers/power/rt9455_charger.c |   16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/power/rt9455_charger.c b/drivers/power/rt9455_charger.c
>> index 08baac6..a49a9d4 100644
>> --- a/drivers/power/rt9455_charger.c
>> +++ b/drivers/power/rt9455_charger.c
>> @@ -973,7 +973,6 @@ static int rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
>>
>>          if (irq2 & GET_MASK(F_CHRVPI)) {
>>                  dev_dbg(dev, "Charger fault occurred\n");
>> -               alert_userspace = true;
>>                  /*
>>                   * CHRVPI bit is set in 2 cases:
>>                   * 1. when the power source is connected to the charger.
>> @@ -981,6 +980,9 @@ static int rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
>>                   * To identify the case, PWR_RDY bit is checked. Because
>>                   * PWR_RDY bit is set / cleared after CHRVPI interrupt is
>>                   * triggered, it is used delayed_work to later read PWR_RDY bit.
>> +                * Also, do not set to true alert_userspace, because there is no
>> +                * need to notify userspace when CHRVPI interrupt has occurred.
>> +                * Userspace will be notified after PWR_RDY bit is read.
>>                   */
>>                  queue_delayed_work(system_power_efficient_wq,
>>                                     &info->pwr_rdy_work,
>> @@ -1178,7 +1180,7 @@ static irqreturn_t rt9455_irq_handler_thread(int irq, void *data)
>>                  /*
>>                   * Sometimes, an interrupt occurs while rt9455_probe() function
>>                   * is executing and power_supply_register() is not yet called.
>> -                * Do not call power_supply_charged() in this case.
>> +                * Do not call power_supply_changed() in this case.
>>                   */
>>                  if (info->charger)
>>                          power_supply_changed(info->charger);
>> @@ -1478,6 +1480,11 @@ static void rt9455_pwr_rdy_work_callback(struct work_struct *work)
>>                                     RT9455_MAX_CHARGING_TIME * HZ);
>>                  break;
>>          }
>> +       /*
>> +        * Notify userspace that the charger has been either connected to or
>> +        * disconnected from the power source.
>> +        */
>> +       power_supply_changed(info->charger);
>>   }
>>
>>   static void rt9455_max_charging_time_work_callback(struct work_struct *work)
>> @@ -1533,6 +1540,11 @@ static void rt9455_batt_presence_work_callback(struct work_struct *work)
>>                          if (ret)
>>                                  dev_err(dev, "Failed to unmask BATAB interrupt\n");
>>                  }
>> +               /*
>> +                * Notify userspace that the battery is now connected to the
>> +                * charger.
>> +                */
>> +               power_supply_changed(info->charger);
>>          }
>>   }
>>
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] power_supply: rt9455_charger: Properly notify userspace about charging events

[Krzysztof Kozlowski]

On 02.07.2015 16:40, Anda-Maria Nicolae wrote:
> Hello Krzysztof,
> 
> The information provided to userspace is not accurate in current driver
> implementation and it is accurate with this patch.
> Also, when the battery is reconnected to the charger (after it has been
> disconnected from the charger), the charger does not trigger any
> interrupt. Since power_supply_changed() is called only in interrupt
> handler, the userspace is never notified that the battery is reconnected.
> 
> Let me explain to you why power_supply_changed() is called in
> rt9455_pwr_rdy_work_callback() and not in interrupt handler, when CHRVPI
> interrupt occurs.
> 
> I created a userspace tool that waits for notifications from the driver.
> A notification is sent when the driver calls power_supply_changed().
> When my userspace tool receives a notification, it reads the files from
> /sys/class/power_supply/rt9455-charger/ folder. The tool reads each file
> this to figure it out what has been changed, since the driver notifies
> the tool only when a change has been made.
> These files include /sys/class/power_supply/rt9455-charger/online. This
> file displays 1 if the power source is connected to the charger and 0
> otherwise.
> When the userspace tool reads
> /sys/class/power_supply/rt9455-charger/online,
> rt9455_charger_get_online() function from the driver is called.
> rt9455_charger_get_online() returns the value of PWR_RDY bit.
> But whenever the power source is connected to / disconnected from the
> charger, RT9455 first sets CHRVPI interrupt bit, triggers an interrupt,
> and after almost 1-2 seconds, the charger also updates PWR_RDY bit.
> So, if we check PWR_RDY bit immediately after CHRVPI bit is set, we will
> not obtain accurate info about the change that has been made (i.e.: if
> the power source was connected or disconnected).
> This is why the driver has struct delayed_work pwr_rdy_work, which is
> scheduled whenever CHRVPI interrupt bit is set.
> This is why in this patch, in rt9455_pwr_rdy_work_callback(),
> power_supply_changed() is called, and not in interrupt handler.

That explains it, thank you. It would be nice if such answer to "why"
would be also in commit message (maybe a shorter one).

Beside of that the patch looks good:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof



> 
> Let me explain to you why power_supply_changed() is called in
> rt9455_batt_presence_work_callback().
> 
> As previously stated, the userspace tool is never notified that the
> battery is reconnected. But since the driver uses struct delayed_work
> batt_presence_work, to determine whether the battery has been
> reconnected, we can call power_supply_changed() whenever the driver
> determines the battery is reconnected (i.e. BATAB interrupt bit is
> cleared).
> This is why in this patch, in rt9455_batt_presence_work_callback(),
> power_supply_changed() is called.
> 
> On 07/02/2015 03:49 AM, Krzysztof Kozlowski wrote:
>> 2015-06-18 1:28 GMT+09:00 Anda-Maria Nicolae
>> <anda-maria.nicolae@intel.com>:
>>> Do not call power_supply_changed() when CHRVPI interrupt has occurred.
>> What I cannot find here is the answer to: why? I am not familiar with
>> the driver so this is just guessing that you wanted to notify
>> userspace about connected/disconnected charger. In both cases (before
>> and after the patch) this is achieved, isn't it?
>>
>> Best regards,
>> Krzysztof
>>
>>
>>> CHRVPI interrupt occurs when the charger is connected to or disconnected
>>> from the power source. Call power_supply_changed() after PWR_RDY bit is
>>> read, to distinguish between connection to or disconnection from the
>>> power
>>> source.
>>> Also, call power_supply_changed() after the battery is reconnected to
>>> the
>>> charger, to notify userspace that the battery is no longer absent.
>>>
>>> Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@intel.com>
>>> ---
>>>   drivers/power/rt9455_charger.c |   16 ++++++++++++++--
>>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/power/rt9455_charger.c
>>> b/drivers/power/rt9455_charger.c
>>> index 08baac6..a49a9d4 100644
>>> --- a/drivers/power/rt9455_charger.c
>>> +++ b/drivers/power/rt9455_charger.c
>>> @@ -973,7 +973,6 @@ static int
>>> rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
>>>
>>>          if (irq2 & GET_MASK(F_CHRVPI)) {
>>>                  dev_dbg(dev, "Charger fault occurred\n");
>>> -               alert_userspace = true;
>>>                  /*
>>>                   * CHRVPI bit is set in 2 cases:
>>>                   * 1. when the power source is connected to the
>>> charger.
>>> @@ -981,6 +980,9 @@ static int
>>> rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
>>>                   * To identify the case, PWR_RDY bit is checked.
>>> Because
>>>                   * PWR_RDY bit is set / cleared after CHRVPI
>>> interrupt is
>>>                   * triggered, it is used delayed_work to later read
>>> PWR_RDY bit.
>>> +                * Also, do not set to true alert_userspace, because
>>> there is no
>>> +                * need to notify userspace when CHRVPI interrupt has
>>> occurred.
>>> +                * Userspace will be notified after PWR_RDY bit is read.
>>>                   */
>>>                  queue_delayed_work(system_power_efficient_wq,
>>>                                     &info->pwr_rdy_work,
>>> @@ -1178,7 +1180,7 @@ static irqreturn_t
>>> rt9455_irq_handler_thread(int irq, void *data)
>>>                  /*
>>>                   * Sometimes, an interrupt occurs while
>>> rt9455_probe() function
>>>                   * is executing and power_supply_register() is not
>>> yet called.
>>> -                * Do not call power_supply_charged() in this case.
>>> +                * Do not call power_supply_changed() in this case.
>>>                   */
>>>                  if (info->charger)
>>>                          power_supply_changed(info->charger);
>>> @@ -1478,6 +1480,11 @@ static void
>>> rt9455_pwr_rdy_work_callback(struct work_struct *work)
>>>                                     RT9455_MAX_CHARGING_TIME * HZ);
>>>                  break;
>>>          }
>>> +       /*
>>> +        * Notify userspace that the charger has been either
>>> connected to or
>>> +        * disconnected from the power source.
>>> +        */
>>> +       power_supply_changed(info->charger);
>>>   }
>>>
>>>   static void rt9455_max_charging_time_work_callback(struct
>>> work_struct *work)
>>> @@ -1533,6 +1540,11 @@ static void
>>> rt9455_batt_presence_work_callback(struct work_struct *work)
>>>                          if (ret)
>>>                                  dev_err(dev, "Failed to unmask BATAB
>>> interrupt\n");
>>>                  }
>>> +               /*
>>> +                * Notify userspace that the battery is now connected
>>> to the
>>> +                * charger.
>>> +                */
>>> +               power_supply_changed(info->charger);
>>>          }
>>>   }
>>>
>>> -- 
>>> 1.7.9.5
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>