linux-kernel - Re: [PATCH 4/4] power: supply: max17042_battery: read battery properties from device tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 23 Mar 2022 10:48:13 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Sebastian Krzyszkowiak <sebastian.krzyszkowiak@...i.sm>,
        Hans de Goede <hdegoede@...hat.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Sebastian Reichel <sre@...nel.org>, linux-pm@...r.kernel.org
Cc:     Purism Kernel Team <kernel@...i.sm>, Rob Herring <robh@...nel.org>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 4/4] power: supply: max17042_battery: read battery
 properties from device tree

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@...i.sm>
>>> ---
>>>
>>>  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