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

[Krzysztof Kozlowski <krzk@kernel.org>]

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@puri.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