diff mbox

[v2,11/14] power: supply: bq24190_charger: Get input_current_limit from our supplier

Message ID 20170815200502.17339-12-hdegoede@redhat.com (mailing list archive)
State Deferred, archived
Headers show

Commit Message

Hans de Goede Aug. 15, 2017, 8:04 p.m. UTC
On some devices the USB Type-C port power (USB PD 2.0) negotiation is
done by a separate port-controller IC, while the current limit is
controlled through another (charger) IC.

It has been decided to model this by modelling the external Type-C
power brick (adapter/charger) as a power-supply class device which
supplies the charger-IC, with its voltage-now and current-max representing
the negotiated voltage and max current draw.

This commit adds support for this to the bq24190_charger driver by calling
power_supply_set_input_current_limit_from_supplier helper if the
"input-current-limit-from-supplier" device-property is set.

Note this replaces the functionality to get the current-limit from an
extcon device, which will be removed in a follow-up commit.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Wait a bit before applying current-max from our supplier as input-current-limit
 the bq24190 may still be in its power-good wait-state while our supplier is
 done negotating current-max and if we apply the limit to early then the
 input-current-limit will be reset to 0.5A by the bq24190 after its
 power-good wait is done.
---
 drivers/power/supply/bq24190_charger.c | 35 ++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

Liam Breck Aug. 16, 2017, 8:28 p.m. UTC | #1
Hi Hans,

On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
> done by a separate port-controller IC, while the current limit is
> controlled through another (charger) IC.
>
> It has been decided to model this by modelling the external Type-C
> power brick (adapter/charger) as a power-supply class device which
> supplies the charger-IC, with its voltage-now and current-max representing
> the negotiated voltage and max current draw.
>
> This commit adds support for this to the bq24190_charger driver by calling
> power_supply_set_input_current_limit_from_supplier helper if the
> "input-current-limit-from-supplier" device-property is set.
>
> Note this replaces the functionality to get the current-limit from an
> extcon device, which will be removed in a follow-up commit.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Wait a bit before applying current-max from our supplier as input-current-limit
>  the bq24190 may still be in its power-good wait-state while our supplier is
>  done negotating current-max and if we apply the limit to early then the
>  input-current-limit will be reset to 0.5A by the bq24190 after its
>  power-good wait is done.
> ---
>  drivers/power/supply/bq24190_charger.c | 35 ++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index f13f892..6f75c8e 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
>         struct extcon_dev               *extcon;
>         struct notifier_block           extcon_nb;
>         struct delayed_work             extcon_work;
> +       struct delayed_work             input_current_limit_work;
>         char                            model_name[I2C_NAME_SIZE];
>         bool                            initialized;
>         bool                            irq_event;
> +       bool                            input_current_limit_from_supplier;
>         struct mutex                    f_reg_lock;
>         u8                              f_reg;
>         u8                              ss_reg;
> @@ -1142,6 +1144,32 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
>         return ret;
>  }
>
> +static void bq24190_input_current_limit_work(struct work_struct *work)
> +{
> +       struct bq24190_dev_info *bdi =
> +               container_of(work, struct bq24190_dev_info,
> +                            input_current_limit_work.work);
> +
> +       power_supply_set_input_current_limit_from_supplier(bdi->charger);
> +}
> +
> +static void bq24190_charger_external_power_changed(struct power_supply *psy)
> +{
> +       struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
> +
> +       /*
> +        * The Power-Good detection may take up to 220ms, sometimes
> +        * the external charger detection is quicker, and the bq24190 will
> +        * reset to iinlim based on its own charger detection (which is not
> +        * hooked up when using external charger detection) resulting in a
> +        * too low default 500mA iinlim. Delay setting the input-current-limit
> +        * for 300ms to avoid this.
> +        */
> +       if (bdi->input_current_limit_from_supplier)
> +               queue_delayed_work(system_wq, &bdi->input_current_limit_work,
> +                                  msecs_to_jiffies(300));
> +}
> +
>  static enum power_supply_property bq24190_charger_properties[] = {
>         POWER_SUPPLY_PROP_CHARGE_TYPE,
>         POWER_SUPPLY_PROP_HEALTH,
> @@ -1170,6 +1198,7 @@ static const struct power_supply_desc bq24190_charger_desc = {
>         .get_property           = bq24190_charger_get_property,
>         .set_property           = bq24190_charger_set_property,
>         .property_is_writeable  = bq24190_charger_property_is_writeable,
> +       .external_power_changed = bq24190_charger_external_power_changed,
>  };
>
>  /* Battery power supply property routines */
> @@ -1651,6 +1680,8 @@ static int bq24190_probe(struct i2c_client *client,
>         mutex_init(&bdi->f_reg_lock);
>         bdi->f_reg = 0;
>         bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
> +       INIT_DELAYED_WORK(&bdi->input_current_limit_work,
> +                         bq24190_input_current_limit_work);
>
>         i2c_set_clientdata(client, bdi);
>
> @@ -1659,6 +1690,10 @@ static int bq24190_probe(struct i2c_client *client,
>                 return -EINVAL;
>         }
>
> +       bdi->input_current_limit_from_supplier =
> +               device_property_read_bool(dev,
> +                                         "input-current-limit-from-supplier");
> +

Maybe
  if (device_property_read_bool(dev,
"linux,input-current-limit-from-supplier")) {
       INIT_DELAYED_WORK(&bdi->input_current_limit_work,
                         bq24190_input_current_limit_work);
      bdi->input_current_limit_from_supplier = true; // or use a field
in bdi->input_current_limit_work as the flag?
   }

Also should document property for DT use assuming Sebastian is ok with
new power_supply method.

And can you rebase to my pending series in next pass? There's nothing
controversial in it :-)
Hans de Goede Aug. 28, 2017, 4:04 p.m. UTC | #2
Hi,

On 16-08-17 22:28, Liam Breck wrote:
> Hi Hans,
> 
> On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
>> done by a separate port-controller IC, while the current limit is
>> controlled through another (charger) IC.
>>
>> It has been decided to model this by modelling the external Type-C
>> power brick (adapter/charger) as a power-supply class device which
>> supplies the charger-IC, with its voltage-now and current-max representing
>> the negotiated voltage and max current draw.
>>
>> This commit adds support for this to the bq24190_charger driver by calling
>> power_supply_set_input_current_limit_from_supplier helper if the
>> "input-current-limit-from-supplier" device-property is set.
>>
>> Note this replaces the functionality to get the current-limit from an
>> extcon device, which will be removed in a follow-up commit.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Wait a bit before applying current-max from our supplier as input-current-limit
>>   the bq24190 may still be in its power-good wait-state while our supplier is
>>   done negotating current-max and if we apply the limit to early then the
>>   input-current-limit will be reset to 0.5A by the bq24190 after its
>>   power-good wait is done.
>> ---
>>   drivers/power/supply/bq24190_charger.c | 35 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index f13f892..6f75c8e 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
>>          struct extcon_dev               *extcon;
>>          struct notifier_block           extcon_nb;
>>          struct delayed_work             extcon_work;
>> +       struct delayed_work             input_current_limit_work;
>>          char                            model_name[I2C_NAME_SIZE];
>>          bool                            initialized;
>>          bool                            irq_event;
>> +       bool                            input_current_limit_from_supplier;
>>          struct mutex                    f_reg_lock;
>>          u8                              f_reg;
>>          u8                              ss_reg;
>> @@ -1142,6 +1144,32 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
>>          return ret;
>>   }
>>
>> +static void bq24190_input_current_limit_work(struct work_struct *work)
>> +{
>> +       struct bq24190_dev_info *bdi =
>> +               container_of(work, struct bq24190_dev_info,
>> +                            input_current_limit_work.work);
>> +
>> +       power_supply_set_input_current_limit_from_supplier(bdi->charger);
>> +}
>> +
>> +static void bq24190_charger_external_power_changed(struct power_supply *psy)
>> +{
>> +       struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>> +
>> +       /*
>> +        * The Power-Good detection may take up to 220ms, sometimes
>> +        * the external charger detection is quicker, and the bq24190 will
>> +        * reset to iinlim based on its own charger detection (which is not
>> +        * hooked up when using external charger detection) resulting in a
>> +        * too low default 500mA iinlim. Delay setting the input-current-limit
>> +        * for 300ms to avoid this.
>> +        */
>> +       if (bdi->input_current_limit_from_supplier)
>> +               queue_delayed_work(system_wq, &bdi->input_current_limit_work,
>> +                                  msecs_to_jiffies(300));
>> +}
>> +
>>   static enum power_supply_property bq24190_charger_properties[] = {
>>          POWER_SUPPLY_PROP_CHARGE_TYPE,
>>          POWER_SUPPLY_PROP_HEALTH,
>> @@ -1170,6 +1198,7 @@ static const struct power_supply_desc bq24190_charger_desc = {
>>          .get_property           = bq24190_charger_get_property,
>>          .set_property           = bq24190_charger_set_property,
>>          .property_is_writeable  = bq24190_charger_property_is_writeable,
>> +       .external_power_changed = bq24190_charger_external_power_changed,
>>   };
>>
>>   /* Battery power supply property routines */
>> @@ -1651,6 +1680,8 @@ static int bq24190_probe(struct i2c_client *client,
>>          mutex_init(&bdi->f_reg_lock);
>>          bdi->f_reg = 0;
>>          bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
>> +       INIT_DELAYED_WORK(&bdi->input_current_limit_work,
>> +                         bq24190_input_current_limit_work);
>>
>>          i2c_set_clientdata(client, bdi);
>>
>> @@ -1659,6 +1690,10 @@ static int bq24190_probe(struct i2c_client *client,
>>                  return -EINVAL;
>>          }
>>
>> +       bdi->input_current_limit_from_supplier =
>> +               device_property_read_bool(dev,
>> +                                         "input-current-limit-from-supplier");
>> +
> 
> Maybe
>    if (device_property_read_bool(dev,
> "linux,input-current-limit-from-supplier")) {
>         INIT_DELAYED_WORK(&bdi->input_current_limit_work,
>                           bq24190_input_current_limit_work);
>        bdi->input_current_limit_from_supplier = true; // or use a field
> in bdi->input_current_limit_work as the flag?
>     }

Done, except for making the flag a field in input_current_limit_work,
input_current_limit_work is of type struct delayed_work so I cannot just
add a field.

> Also should document property for DT use assuming Sebastian is ok with
> new power_supply method.

Also done for v3 of this patch.

> And can you rebase to my pending series in next pass? There's nothing
> controversial in it :-)

I've cherry-picked v3 of the patch adding the dt-binding-doc to my tree so
that I could add the documentation for the property on top. I will make sure
to rebase on top of the rest once the rest is merged by Sebastian (otherwise
I need to rebase after each revision of the patch-set).

Regards,

Hans
Liam Breck Aug. 28, 2017, 5:02 p.m. UTC | #3
Hi Hans,

On Mon, Aug 28, 2017 at 9:04 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 16-08-17 22:28, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
>>> done by a separate port-controller IC, while the current limit is
>>> controlled through another (charger) IC.
>>>
>>> It has been decided to model this by modelling the external Type-C
>>> power brick (adapter/charger) as a power-supply class device which
>>> supplies the charger-IC, with its voltage-now and current-max
>>> representing
>>> the negotiated voltage and max current draw.
>>>
>>> This commit adds support for this to the bq24190_charger driver by
>>> calling
>>> power_supply_set_input_current_limit_from_supplier helper if the
>>> "input-current-limit-from-supplier" device-property is set.
>>>
>>> Note this replaces the functionality to get the current-limit from an
>>> extcon device, which will be removed in a follow-up commit.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> -Wait a bit before applying current-max from our supplier as
>>> input-current-limit
>>>   the bq24190 may still be in its power-good wait-state while our
>>> supplier is
>>>   done negotating current-max and if we apply the limit to early then the
>>>   input-current-limit will be reset to 0.5A by the bq24190 after its
>>>   power-good wait is done.
>>> ---
>>>   drivers/power/supply/bq24190_charger.c | 35
>>> ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index f13f892..6f75c8e 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
>>>          struct extcon_dev               *extcon;
>>>          struct notifier_block           extcon_nb;
>>>          struct delayed_work             extcon_work;
>>> +       struct delayed_work             input_current_limit_work;
>>>          char                            model_name[I2C_NAME_SIZE];
>>>          bool                            initialized;
>>>          bool                            irq_event;
>>> +       bool
>>> input_current_limit_from_supplier;
>>>          struct mutex                    f_reg_lock;
>>>          u8                              f_reg;
>>>          u8                              ss_reg;
>>> @@ -1142,6 +1144,32 @@ static int
>>> bq24190_charger_property_is_writeable(struct power_supply *psy,
>>>          return ret;
>>>   }
>>>
>>> +static void bq24190_input_current_limit_work(struct work_struct *work)
>>> +{
>>> +       struct bq24190_dev_info *bdi =
>>> +               container_of(work, struct bq24190_dev_info,
>>> +                            input_current_limit_work.work);
>>> +
>>> +       power_supply_set_input_current_limit_from_supplier(bdi->charger);
>>> +}
>>> +
>>> +static void bq24190_charger_external_power_changed(struct power_supply
>>> *psy)
>>> +{
>>> +       struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>> +
>>> +       /*
>>> +        * The Power-Good detection may take up to 220ms, sometimes
>>> +        * the external charger detection is quicker, and the bq24190
>>> will
>>> +        * reset to iinlim based on its own charger detection (which is
>>> not
>>> +        * hooked up when using external charger detection) resulting in
>>> a
>>> +        * too low default 500mA iinlim. Delay setting the
>>> input-current-limit
>>> +        * for 300ms to avoid this.
>>> +        */
>>> +       if (bdi->input_current_limit_from_supplier)
>>> +               queue_delayed_work(system_wq,
>>> &bdi->input_current_limit_work,
>>> +                                  msecs_to_jiffies(300));
>>> +}
>>> +
>>>   static enum power_supply_property bq24190_charger_properties[] = {
>>>          POWER_SUPPLY_PROP_CHARGE_TYPE,
>>>          POWER_SUPPLY_PROP_HEALTH,
>>> @@ -1170,6 +1198,7 @@ static const struct power_supply_desc
>>> bq24190_charger_desc = {
>>>          .get_property           = bq24190_charger_get_property,
>>>          .set_property           = bq24190_charger_set_property,
>>>          .property_is_writeable  = bq24190_charger_property_is_writeable,
>>> +       .external_power_changed = bq24190_charger_external_power_changed,
>>>   };
>>>
>>>   /* Battery power supply property routines */
>>> @@ -1651,6 +1680,8 @@ static int bq24190_probe(struct i2c_client *client,
>>>          mutex_init(&bdi->f_reg_lock);
>>>          bdi->f_reg = 0;
>>>          bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state
>>> */
>>> +       INIT_DELAYED_WORK(&bdi->input_current_limit_work,
>>> +                         bq24190_input_current_limit_work);
>>>
>>>          i2c_set_clientdata(client, bdi);
>>>
>>> @@ -1659,6 +1690,10 @@ static int bq24190_probe(struct i2c_client
>>> *client,
>>>                  return -EINVAL;
>>>          }
>>>
>>> +       bdi->input_current_limit_from_supplier =
>>> +               device_property_read_bool(dev,
>>> +
>>> "input-current-limit-from-supplier");
>>> +
>>
>>
>> Maybe
>>    if (device_property_read_bool(dev,
>> "linux,input-current-limit-from-supplier")) {
>>         INIT_DELAYED_WORK(&bdi->input_current_limit_work,
>>                           bq24190_input_current_limit_work);
>>        bdi->input_current_limit_from_supplier = true; // or use a field
>> in bdi->input_current_limit_work as the flag?
>>     }
>
>
> Done, except for making the flag a field in input_current_limit_work,
> input_current_limit_work is of type struct delayed_work so I cannot just
> add a field.

What I meant was you could check an existing field in delayed_work to
see if it was init'd.

>> Also should document property for DT use assuming Sebastian is ok with
>> new power_supply method.
>
> Also done for v3 of this patch.
>
>> And can you rebase to my pending series in next pass? There's nothing
>> controversial in it :-)
>
>
> I've cherry-picked v3 of the patch adding the dt-binding-doc to my tree so
> that I could add the documentation for the property on top. I will make sure
> to rebase on top of the rest once the rest is merged by Sebastian (otherwise
> I need to rebase after each revision of the patch-set).

Sounds good.
Liam Breck Aug. 28, 2017, 6:07 p.m. UTC | #4
Hi Hans, I sent too soon...

On Mon, Aug 28, 2017 at 9:04 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 16-08-17 22:28, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
>>> done by a separate port-controller IC, while the current limit is
>>> controlled through another (charger) IC.
>>>
>>> It has been decided to model this by modelling the external Type-C
>>> power brick (adapter/charger) as a power-supply class device which
>>> supplies the charger-IC, with its voltage-now and current-max
>>> representing
>>> the negotiated voltage and max current draw.
>>>
>>> This commit adds support for this to the bq24190_charger driver by
>>> calling
>>> power_supply_set_input_current_limit_from_supplier helper if the
>>> "input-current-limit-from-supplier" device-property is set.
>>>
>>> Note this replaces the functionality to get the current-limit from an
>>> extcon device, which will be removed in a follow-up commit.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> -Wait a bit before applying current-max from our supplier as
>>> input-current-limit
>>>   the bq24190 may still be in its power-good wait-state while our
>>> supplier is
>>>   done negotating current-max and if we apply the limit to early then the
>>>   input-current-limit will be reset to 0.5A by the bq24190 after its
>>>   power-good wait is done.
>>> ---
>>>   drivers/power/supply/bq24190_charger.c | 35
>>> ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index f13f892..6f75c8e 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
>>>          struct extcon_dev               *extcon;
>>>          struct notifier_block           extcon_nb;
>>>          struct delayed_work             extcon_work;
>>> +       struct delayed_work             input_current_limit_work;
>>>          char                            model_name[I2C_NAME_SIZE];
>>>          bool                            initialized;
>>>          bool                            irq_event;
>>> +       bool
>>> input_current_limit_from_supplier;
>>>          struct mutex                    f_reg_lock;
>>>          u8                              f_reg;
>>>          u8                              ss_reg;
>>> @@ -1142,6 +1144,32 @@ static int
>>> bq24190_charger_property_is_writeable(struct power_supply *psy,
>>>          return ret;
>>>   }
>>>
>>> +static void bq24190_input_current_limit_work(struct work_struct *work)
>>> +{
>>> +       struct bq24190_dev_info *bdi =
>>> +               container_of(work, struct bq24190_dev_info,
>>> +                            input_current_limit_work.work);
>>> +
>>> +       power_supply_set_input_current_limit_from_supplier(bdi->charger);
>>> +}
>>> +
>>> +static void bq24190_charger_external_power_changed(struct power_supply
>>> *psy)
>>> +{
>>> +       struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>> +
>>> +       /*
>>> +        * The Power-Good detection may take up to 220ms, sometimes
>>> +        * the external charger detection is quicker, and the bq24190
>>> will
>>> +        * reset to iinlim based on its own charger detection (which is
>>> not
>>> +        * hooked up when using external charger detection) resulting in
>>> a
>>> +        * too low default 500mA iinlim. Delay setting the
>>> input-current-limit
>>> +        * for 300ms to avoid this.
>>> +        */
>>> +       if (bdi->input_current_limit_from_supplier)
>>> +               queue_delayed_work(system_wq,
>>> &bdi->input_current_limit_work,
>>> +                                  msecs_to_jiffies(300));
>>> +}
>>> +
>>>   static enum power_supply_property bq24190_charger_properties[] = {
>>>          POWER_SUPPLY_PROP_CHARGE_TYPE,
>>>          POWER_SUPPLY_PROP_HEALTH,
>>> @@ -1170,6 +1198,7 @@ static const struct power_supply_desc
>>> bq24190_charger_desc = {
>>>          .get_property           = bq24190_charger_get_property,
>>>          .set_property           = bq24190_charger_set_property,
>>>          .property_is_writeable  = bq24190_charger_property_is_writeable,
>>> +       .external_power_changed = bq24190_charger_external_power_changed,
>>>   };
>>>
>>>   /* Battery power supply property routines */
>>> @@ -1651,6 +1680,8 @@ static int bq24190_probe(struct i2c_client *client,
>>>          mutex_init(&bdi->f_reg_lock);
>>>          bdi->f_reg = 0;
>>>          bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state
>>> */
>>> +       INIT_DELAYED_WORK(&bdi->input_current_limit_work,
>>> +                         bq24190_input_current_limit_work);
>>>
>>>          i2c_set_clientdata(client, bdi);
>>>
>>> @@ -1659,6 +1690,10 @@ static int bq24190_probe(struct i2c_client
>>> *client,
>>>                  return -EINVAL;
>>>          }
>>>
>>> +       bdi->input_current_limit_from_supplier =
>>> +               device_property_read_bool(dev,
>>> +
>>> "input-current-limit-from-supplier");
>>> +
>>
>>
>> Maybe
>>    if (device_property_read_bool(dev,
>> "linux,input-current-limit-from-supplier")) {
>>         INIT_DELAYED_WORK(&bdi->input_current_limit_work,
>>                           bq24190_input_current_limit_work);
>>        bdi->input_current_limit_from_supplier = true; // or use a field
>> in bdi->input_current_limit_work as the flag?
>>     }
>
>
> Done, except for making the flag a field in input_current_limit_work,
> input_current_limit_work is of type struct delayed_work so I cannot just
> add a field.
>
>> Also should document property for DT use assuming Sebastian is ok with
>> new power_supply method.
>
>
> Also done for v3 of this patch.
>
>> And can you rebase to my pending series in next pass? There's nothing
>> controversial in it :-)
>
>
> I've cherry-picked v3 of the patch adding the dt-binding-doc to my tree so
> that I could add the documentation for the property on top. I will make sure
> to rebase on top of the rest once the rest is merged by Sebastian (otherwise
> I need to rebase after each revision of the patch-set).

If the property is linux,input-current-limit-from-supplier should it
also be documented elsewhere in DT bindings?
Hans de Goede Aug. 28, 2017, 7:08 p.m. UTC | #5
Hi,

On 28-08-17 20:07, Liam Breck wrote:
> Hi Hans, I sent too soon...
> 
> On Mon, Aug 28, 2017 at 9:04 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 16-08-17 22:28, Liam Breck wrote:
>>>
>>> Hi Hans,
>>>
>>> On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
>>>> done by a separate port-controller IC, while the current limit is
>>>> controlled through another (charger) IC.
>>>>
>>>> It has been decided to model this by modelling the external Type-C
>>>> power brick (adapter/charger) as a power-supply class device which
>>>> supplies the charger-IC, with its voltage-now and current-max
>>>> representing
>>>> the negotiated voltage and max current draw.
>>>>
>>>> This commit adds support for this to the bq24190_charger driver by
>>>> calling
>>>> power_supply_set_input_current_limit_from_supplier helper if the
>>>> "input-current-limit-from-supplier" device-property is set.
>>>>
>>>> Note this replaces the functionality to get the current-limit from an
>>>> extcon device, which will be removed in a follow-up commit.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v2:
>>>> -Wait a bit before applying current-max from our supplier as
>>>> input-current-limit
>>>>    the bq24190 may still be in its power-good wait-state while our
>>>> supplier is
>>>>    done negotating current-max and if we apply the limit to early then the
>>>>    input-current-limit will be reset to 0.5A by the bq24190 after its
>>>>    power-good wait is done.
>>>> ---
>>>>    drivers/power/supply/bq24190_charger.c | 35
>>>> ++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 35 insertions(+)
>>>>
>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>> b/drivers/power/supply/bq24190_charger.c
>>>> index f13f892..6f75c8e 100644
>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
>>>>           struct extcon_dev               *extcon;
>>>>           struct notifier_block           extcon_nb;
>>>>           struct delayed_work             extcon_work;
>>>> +       struct delayed_work             input_current_limit_work;
>>>>           char                            model_name[I2C_NAME_SIZE];
>>>>           bool                            initialized;
>>>>           bool                            irq_event;
>>>> +       bool
>>>> input_current_limit_from_supplier;
>>>>           struct mutex                    f_reg_lock;
>>>>           u8                              f_reg;
>>>>           u8                              ss_reg;
>>>> @@ -1142,6 +1144,32 @@ static int
>>>> bq24190_charger_property_is_writeable(struct power_supply *psy,
>>>>           return ret;
>>>>    }
>>>>
>>>> +static void bq24190_input_current_limit_work(struct work_struct *work)
>>>> +{
>>>> +       struct bq24190_dev_info *bdi =
>>>> +               container_of(work, struct bq24190_dev_info,
>>>> +                            input_current_limit_work.work);
>>>> +
>>>> +       power_supply_set_input_current_limit_from_supplier(bdi->charger);
>>>> +}
>>>> +
>>>> +static void bq24190_charger_external_power_changed(struct power_supply
>>>> *psy)
>>>> +{
>>>> +       struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>>> +
>>>> +       /*
>>>> +        * The Power-Good detection may take up to 220ms, sometimes
>>>> +        * the external charger detection is quicker, and the bq24190
>>>> will
>>>> +        * reset to iinlim based on its own charger detection (which is
>>>> not
>>>> +        * hooked up when using external charger detection) resulting in
>>>> a
>>>> +        * too low default 500mA iinlim. Delay setting the
>>>> input-current-limit
>>>> +        * for 300ms to avoid this.
>>>> +        */
>>>> +       if (bdi->input_current_limit_from_supplier)
>>>> +               queue_delayed_work(system_wq,
>>>> &bdi->input_current_limit_work,
>>>> +                                  msecs_to_jiffies(300));
>>>> +}
>>>> +
>>>>    static enum power_supply_property bq24190_charger_properties[] = {
>>>>           POWER_SUPPLY_PROP_CHARGE_TYPE,
>>>>           POWER_SUPPLY_PROP_HEALTH,
>>>> @@ -1170,6 +1198,7 @@ static const struct power_supply_desc
>>>> bq24190_charger_desc = {
>>>>           .get_property           = bq24190_charger_get_property,
>>>>           .set_property           = bq24190_charger_set_property,
>>>>           .property_is_writeable  = bq24190_charger_property_is_writeable,
>>>> +       .external_power_changed = bq24190_charger_external_power_changed,
>>>>    };
>>>>
>>>>    /* Battery power supply property routines */
>>>> @@ -1651,6 +1680,8 @@ static int bq24190_probe(struct i2c_client *client,
>>>>           mutex_init(&bdi->f_reg_lock);
>>>>           bdi->f_reg = 0;
>>>>           bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state
>>>> */
>>>> +       INIT_DELAYED_WORK(&bdi->input_current_limit_work,
>>>> +                         bq24190_input_current_limit_work);
>>>>
>>>>           i2c_set_clientdata(client, bdi);
>>>>
>>>> @@ -1659,6 +1690,10 @@ static int bq24190_probe(struct i2c_client
>>>> *client,
>>>>                   return -EINVAL;
>>>>           }
>>>>
>>>> +       bdi->input_current_limit_from_supplier =
>>>> +               device_property_read_bool(dev,
>>>> +
>>>> "input-current-limit-from-supplier");
>>>> +
>>>
>>>
>>> Maybe
>>>     if (device_property_read_bool(dev,
>>> "linux,input-current-limit-from-supplier")) {
>>>          INIT_DELAYED_WORK(&bdi->input_current_limit_work,
>>>                            bq24190_input_current_limit_work);
>>>         bdi->input_current_limit_from_supplier = true; // or use a field
>>> in bdi->input_current_limit_work as the flag?
>>>      }
>>
>>
>> Done, except for making the flag a field in input_current_limit_work,
>> input_current_limit_work is of type struct delayed_work so I cannot just
>> add a field.
>>
>>> Also should document property for DT use assuming Sebastian is ok with
>>> new power_supply method.
>>
>>
>> Also done for v3 of this patch.
>>
>>> And can you rebase to my pending series in next pass? There's nothing
>>> controversial in it :-)
>>
>>
>> I've cherry-picked v3 of the patch adding the dt-binding-doc to my tree so
>> that I could add the documentation for the property on top. I will make sure
>> to rebase on top of the rest once the rest is merged by Sebastian (otherwise
>> I need to rebase after each revision of the patch-set).
> 
> If the property is linux,input-current-limit-from-supplier should it
> also be documented elsewhere in DT bindings?

linux, just means that it is Linux specific, it can still be a
per-device binding, although maybe we should make it a generic
power-supply consumer property? And at it to:

Documentation/devicetree/bindings/power/supply/power_supply.txt

Since that binding does not appear to be Linux specific, I've
gone with ti,input-current-limit-from-supplier for now and I've
documented it in the bq24190 bindings for now.

I've also added the device-tree binding maintainers to the Cc,
for the still to be send v3 of the patch-set, so lets just wait
and see what they have to say.

Regards,

Hans
Sebastian Reichel Aug. 29, 2017, 11:40 a.m. UTC | #6
Hi,

On Tue, Aug 15, 2017 at 10:04:59PM +0200, Hans de Goede wrote:
> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
> done by a separate port-controller IC, while the current limit is
> controlled through another (charger) IC.
> 
> It has been decided to model this by modelling the external Type-C
> power brick (adapter/charger) as a power-supply class device which
> supplies the charger-IC, with its voltage-now and current-max representing
> the negotiated voltage and max current draw.
> 
> This commit adds support for this to the bq24190_charger driver by calling
> power_supply_set_input_current_limit_from_supplier helper if the
> "input-current-limit-from-supplier" device-property is set.
> 
> Note this replaces the functionality to get the current-limit from an
> extcon device, which will be removed in a follow-up commit.

I'm fine with the general approach, but ...

> [...]
> +	bdi->input_current_limit_from_supplier =
> +		device_property_read_bool(dev,
> +					  "input-current-limit-from-supplier");
> [...]

I wonder if we actually need this. I think we can just enable it
unconditionally when we have a parent power supply providing the
information.

-- Sebastian
Hans de Goede Aug. 29, 2017, 11:53 a.m. UTC | #7
Hi,

Thank you for your reviews / queuing!

On 29-08-17 13:40, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Aug 15, 2017 at 10:04:59PM +0200, Hans de Goede wrote:
>> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
>> done by a separate port-controller IC, while the current limit is
>> controlled through another (charger) IC.
>>
>> It has been decided to model this by modelling the external Type-C
>> power brick (adapter/charger) as a power-supply class device which
>> supplies the charger-IC, with its voltage-now and current-max representing
>> the negotiated voltage and max current draw.
>>
>> This commit adds support for this to the bq24190_charger driver by calling
>> power_supply_set_input_current_limit_from_supplier helper if the
>> "input-current-limit-from-supplier" device-property is set.
>>
>> Note this replaces the functionality to get the current-limit from an
>> extcon device, which will be removed in a follow-up commit.
> 
> I'm fine with the general approach, but ...
> 
>> [...]
>> +	bdi->input_current_limit_from_supplier =
>> +		device_property_read_bool(dev,
>> +					  "input-current-limit-from-supplier");
>> [...]
> 
> I wonder if we actually need this. I think we can just enable it
> unconditionally when we have a parent power supply providing the
> information.

I was thinking the same when implementing this, so this is fine with
me. I think it is best to just unconditionally call
power_supply_set_input_current_limit_from_supplier from the
external_power_changed callback, that will only get called if we've
a parent supply and that function will check that the parent has
a current-max property itself.

Please let me know if just unconditionally calling
power_supply_set_input_current_limit_from_supplier from the
external_power_changed callback is ok with you then I will do that
for v3 of the patch-set (from which I will drop the patches you've
already queued).

Regards,

Hans
Sebastian Reichel Aug. 29, 2017, 12:12 p.m. UTC | #8
Hi,

On Tue, Aug 29, 2017 at 01:53:24PM +0200, Hans de Goede wrote:
> Hi,
> 
> Thank you for your reviews / queuing!
> 
> On 29-08-17 13:40, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Tue, Aug 15, 2017 at 10:04:59PM +0200, Hans de Goede wrote:
> > > On some devices the USB Type-C port power (USB PD 2.0) negotiation is
> > > done by a separate port-controller IC, while the current limit is
> > > controlled through another (charger) IC.
> > > 
> > > It has been decided to model this by modelling the external Type-C
> > > power brick (adapter/charger) as a power-supply class device which
> > > supplies the charger-IC, with its voltage-now and current-max representing
> > > the negotiated voltage and max current draw.
> > > 
> > > This commit adds support for this to the bq24190_charger driver by calling
> > > power_supply_set_input_current_limit_from_supplier helper if the
> > > "input-current-limit-from-supplier" device-property is set.
> > > 
> > > Note this replaces the functionality to get the current-limit from an
> > > extcon device, which will be removed in a follow-up commit.
> > 
> > I'm fine with the general approach, but ...
> > 
> > > [...]
> > > +	bdi->input_current_limit_from_supplier =
> > > +		device_property_read_bool(dev,
> > > +					  "input-current-limit-from-supplier");
> > > [...]
> > 
> > I wonder if we actually need this. I think we can just enable it
> > unconditionally when we have a parent power supply providing the
> > information.
> 
> I was thinking the same when implementing this, so this is fine with
> me. I think it is best to just unconditionally call
> power_supply_set_input_current_limit_from_supplier from the
> external_power_changed callback, that will only get called if we've
> a parent supply and that function will check that the parent has
> a current-max property itself.
> 
> Please let me know if just unconditionally calling
> power_supply_set_input_current_limit_from_supplier from the
> external_power_changed callback is ok with you then I will do that
> for v3 of the patch-set (from which I will drop the patches you've
> already queued).

Makes sense to me.

-- Sebastian
diff mbox

Patch

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index f13f892..6f75c8e 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -159,9 +159,11 @@  struct bq24190_dev_info {
 	struct extcon_dev		*extcon;
 	struct notifier_block		extcon_nb;
 	struct delayed_work		extcon_work;
+	struct delayed_work		input_current_limit_work;
 	char				model_name[I2C_NAME_SIZE];
 	bool				initialized;
 	bool				irq_event;
+	bool				input_current_limit_from_supplier;
 	struct mutex			f_reg_lock;
 	u8				f_reg;
 	u8				ss_reg;
@@ -1142,6 +1144,32 @@  static int bq24190_charger_property_is_writeable(struct power_supply *psy,
 	return ret;
 }
 
+static void bq24190_input_current_limit_work(struct work_struct *work)
+{
+	struct bq24190_dev_info *bdi =
+		container_of(work, struct bq24190_dev_info,
+			     input_current_limit_work.work);
+
+	power_supply_set_input_current_limit_from_supplier(bdi->charger);
+}
+
+static void bq24190_charger_external_power_changed(struct power_supply *psy)
+{
+	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
+
+	/*
+	 * The Power-Good detection may take up to 220ms, sometimes
+	 * the external charger detection is quicker, and the bq24190 will
+	 * reset to iinlim based on its own charger detection (which is not
+	 * hooked up when using external charger detection) resulting in a
+	 * too low default 500mA iinlim. Delay setting the input-current-limit
+	 * for 300ms to avoid this.
+	 */
+	if (bdi->input_current_limit_from_supplier)
+		queue_delayed_work(system_wq, &bdi->input_current_limit_work,
+				   msecs_to_jiffies(300));
+}
+
 static enum power_supply_property bq24190_charger_properties[] = {
 	POWER_SUPPLY_PROP_CHARGE_TYPE,
 	POWER_SUPPLY_PROP_HEALTH,
@@ -1170,6 +1198,7 @@  static const struct power_supply_desc bq24190_charger_desc = {
 	.get_property		= bq24190_charger_get_property,
 	.set_property		= bq24190_charger_set_property,
 	.property_is_writeable	= bq24190_charger_property_is_writeable,
+	.external_power_changed	= bq24190_charger_external_power_changed,
 };
 
 /* Battery power supply property routines */
@@ -1651,6 +1680,8 @@  static int bq24190_probe(struct i2c_client *client,
 	mutex_init(&bdi->f_reg_lock);
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
+	INIT_DELAYED_WORK(&bdi->input_current_limit_work,
+			  bq24190_input_current_limit_work);
 
 	i2c_set_clientdata(client, bdi);
 
@@ -1659,6 +1690,10 @@  static int bq24190_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
+	bdi->input_current_limit_from_supplier =
+		device_property_read_bool(dev,
+					  "input-current-limit-from-supplier");
+
 	/*
 	 * Devicetree platforms should get extcon via phandle (not yet supported).
 	 * On ACPI platforms, extcon clients may invoke us with: