On 20/03/2022 22:24, Sebastian Krzyszkowiak wrote:
> On piątek, 18 marca 2022 09:40:36 CET Krzysztof Kozlowski wrote:
>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>>> So far configuring the gauge was only possible using platform data,
>>> with no way to provide the configuration on device tree-based platforms.
>>>
>>> Change that by looking up the configuration values from monitored-battery
>>> property. This is especially useful on models implementing ModelGauge m5
>>> EZ
>>> algorithm, such as MAX17055, as all the required configuration can be
>>> derived from a "simple-battery" DT node there.
>>>
>>> In order to be able to access power supply framework in get_of_pdata,
>>> move devm_power_supply_register earlier in max17042_probe.
>>>
>>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@xxxxxxx>
>>> ---
>>>
>>> drivers/power/supply/max17042_battery.c | 50 +++++++++++++++++++------
>>> include/linux/power/max17042_battery.h | 1 +
>>> 2 files changed, 40 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/max17042_battery.c
>>> b/drivers/power/supply/max17042_battery.c index
>>> c39250349a1d..4c33565802d5 100644
>>> --- a/drivers/power/supply/max17042_battery.c
>>> +++ b/drivers/power/supply/max17042_battery.c
>>> @@ -937,7 +937,9 @@ max17042_get_of_pdata(struct max17042_chip *chip)
>>>
>>> struct device *dev = &chip->client->dev;
>>> struct device_node *np = dev->of_node;
>>> u32 prop;
>>>
>>> + u64 data64;
>>>
>>> struct max17042_platform_data *pdata;
>>>
>>> + struct power_supply_battery_info *info;
>>>
>>> pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> if (!pdata)
>>>
>>> @@ -961,6 +963,32 @@ max17042_get_of_pdata(struct max17042_chip *chip)
>>>
>>> if (of_property_read_s32(np, "maxim,over-volt", &pdata->vmax))
>>>
>>> pdata->vmax = INT_MAX;
>>>
>>> + if (pdata->enable_current_sense &&
>>> + power_supply_get_battery_info(chip->battery, &info) == 0) {
>>> + pdata->config_data = devm_kzalloc(dev, sizeof(*pdata-
>> config_data),
>>> GFP_KERNEL); + if (!pdata->config_data)
>>> + return NULL;
>>> +
>>> + if (info->charge_full_design_uah != -EINVAL) {
>>> + data64 = (u64)info->charge_full_design_uah *
> pdata->r_sns;
>>> + do_div(data64, MAX17042_CAPACITY_LSB);
>>> + pdata->config_data->design_cap = (u16)data64;
>>> + pdata->enable_por_init = true;
>>> + }
>>> + if (info->charge_term_current_ua != -EINVAL) {
>>> + data64 = (u64)info->charge_term_current_ua *
> pdata->r_sns;
>>> + do_div(data64, MAX17042_CURRENT_LSB);
>>> + pdata->config_data->ichgt_term = (u16)data64;
>>> + pdata->enable_por_init = true;
>>> + }
>>> + if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
>>> + if (info->voltage_max_design_uv > 4250000) {
>>> + pdata->config_data->model_cfg =
> MAX17055_MODELCFG_VCHG_BIT;
>>> + pdata->enable_por_init = true;
>>> + }
>>> + }
>>> + }
>>> +
>>>
>>> return pdata;
>>>
>>> }
>>> #endif
>>>
>>> @@ -1092,16 +1120,23 @@ static int max17042_probe(struct i2c_client
>>> *client,>
>>> return -EINVAL;
>>>
>>> }
>>>
>>> + i2c_set_clientdata(client, chip);
>>> + psy_cfg.drv_data = chip;
>>> + psy_cfg.of_node = dev->of_node;
>>> +
>>> + chip->battery = devm_power_supply_register(&client->dev,
> max17042_desc,
>>> +
> &psy_cfg);
>>> + if (IS_ERR(chip->battery)) {
>>> + dev_err(&client->dev, "failed: power supply
> register\n");
>>> + return PTR_ERR(chip->battery);
>>> + }
>>
>> I don't think it is correct. You register power supply, thus making it
>> available for system, before configuring most of the data. For short
>> time the chip might report to the system bogus results and events.
>>
>> Instead I think you should split it into two parts - init which happens
>> before registering power supply and after.
>
> Simply splitting initialization into two parts won't really help. If you set
> capacity, current, Vchg and refresh the model after registering power supply,
> you will still end up having a short time window with bogus results. Looking
> at other drivers, they seem to deal with it in the same way - they register
> the power supply early, before the driver can fully configure the device.
>
> To actually fix the problem with bogus data on init, it seems like we would
> either need some support from the power supply framework to notify it when can
> it actually start expecting correct data, or have a way to access the battery
> information without having to register power supply beforehand.
Indeed I spotted similar pattern in other drivers, so this might be a
common issue.
>
> Since power_supply_get_battery_info doesn't actually seem to depend on
> power_supply device at all - it uses psy->dev for devm functions and psy-
> of_node to read the data from - I wonder if it could be split into a function
> that only takes an of_node?
That might be the best approach.
Best regards,
Krzysztof
