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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3482664.QJadu78ljV@pliszka>
Date:   Sun, 20 Mar 2022 22:24:57 +0100
From:   Sebastian Krzyszkowiak <sebastian.krzyszkowiak@...i.sm>
To:     Hans de Goede <hdegoede@...hat.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Sebastian Reichel <sre@...nel.org>, linux-pm@...r.kernel.org,
        Krzysztof Kozlowski <krzk@...nel.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 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.

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?

Cheers,
Sebastian
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)