Re: [PATCH] power: supply: bq25890: Add support for setting IINLIM

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 8/1/22 05:00, Marek Vasut wrote:
> On 7/31/22 11:52, Hans de Goede wrote:
> 
> [...]
> 
>>> +static int bq25890_power_supply_set_property(struct power_supply *psy,
>>> +                         enum power_supply_property psp,
>>> +                         const union power_supply_propval *val)
>>> +{
>>> +    struct bq25890_device *bq = power_supply_get_drvdata(psy);
>>> +    u32 lval;
>>> +
>>> +    switch (psp) {
>>> +    case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>>> +        lval = clamp(val->intval, 100000, 3250000);
>>> +        lval = DIV_ROUND_UP(lval - 100000, 50000);
>>
>> I'm not sure DIV_ROUND_UP is the right thing to do here. This means
>> that when the user e.g. asks for 1040 mA the iinlim will get set to
>> 1050mA so more then which is being requested.
>>
>> IMHO it would be better to use rounding down, aka standard divide
>> behavior here.
>>
>> But even better would be to replace both lval = ... statements
>> with a single:
>>
>>     lval = bq25890_find_idx(val->intval, TBL_IINLIM);
>>
>> which takes care of all this for you and is also what is used
>> by bq25890_charger_external_power_changed() to set iinlim based
>> on charger-type-detection done by other chips on the board
>> (e.g. PMICs / usb-phys Type-C controllers).
> 
> Nice, fixed in v2, thanks.
> 
> There is one thing which I don't quite understand about this driver though -- shouldn't we implement .external_power_changed() callback and then somehow listen for which charger gets plugged in (like, USB standard one, or 1.5A one or 3A one, or even some adapter), and based on that tweak the IINLIM too ? Or is this more of a userspace kind of policy, so it should be up to userspace to write this sysfs entry as needed ?

There already is an external_power_changed() callback that is what
the bq25890_charger_external_power_changed() function I mentioned is... ?

Regards,

Hans