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: <354ec362-0883-ccfe-9e19-7897318e5e08@kernel.org>
Date:   Fri, 18 Mar 2022 09:40:36 +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 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.


Best regards,
Krzysztof