linux-kernel - Re: [PATCH v2] power: supply: max1720x: add read support for nvmem

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: <01020191b4bc8ff0-e7e8d909-4802-4076-9caf-cee0296fd10d-000000@eu-west-1.amazonses.com>
Date: Mon, 2 Sep 2024 21:55:42 +0000
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Dimitri Fedrau <dima.fedrau@...il.com>
Cc: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] power: supply: max1720x: add read support for nvmem

Hi,

On Sat, Aug 31, 2024 at 08:21:45PM GMT, Dimitri Fedrau wrote:
> ModelGauge m5 and device configuration values are stored in nonvolatile
> memory to prevent data loss if the IC loses power. Add read support for
> the nonvolatile memory on MAX1720X devices.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@...il.com>
> ---
> 
> Based on:
> 479b6d04964b "power: supply: add support for MAX1720x standalone fuel gauge"
> in branch for-next
> 
> Changes in V2:
>   - remove function max1720x_remove and use devm_add_action_or_reset() to
>     unregister info->ancillary to avoid race condition during module remove

Thanks, but the transformation is quite incomplete. You probably
should have a look what device managed resource actually means :)

> ---
>  drivers/power/supply/max1720x_battery.c | 220 ++++++++++++++++++++++--
>  1 file changed, 205 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> index edc262f0a62f..d27c94bdb835 100644
> --- a/drivers/power/supply/max1720x_battery.c
> +++ b/drivers/power/supply/max1720x_battery.c

[...]

> +static int max1720x_probe_nvmem(struct i2c_client *client,
> +				struct max1720x_device_info *info)
>  {
>  	struct device *dev = &client->dev;
> -	struct i2c_client *ancillary;
> +	struct nvmem_config nvmem_config = {

As noticed by the build bot: you need to add this include for the
struct:

#include <linux/nvmem-provider.h>

> +		.dev = dev,
> +		.name = "max1720x_nvmem",
> +		.cells = max1720x_nvmem_cells,
> +		.ncells = ARRAY_SIZE(max1720x_nvmem_cells),
> +		.read_only = true,
> +		.root_only = true,
> +		.reg_read = max1720x_nvmem_reg_read,
> +		.size = ARRAY_SIZE(max1720x_nvmem_cells) * 2,
> +		.word_size = 2,
> +		.stride = 2,
> +		.priv = info,
> +	};
> +	struct nvmem_device *nvmem;
> +	unsigned int val;
>  	int ret;
>  
> -	ancillary = i2c_new_ancillary_device(client, "nvmem", 0xb);
> -	if (IS_ERR(ancillary)) {
> +	info->ancillary = i2c_new_ancillary_device(client, "nvmem", 0xb);
> +	if (IS_ERR(info->ancillary)) {
>  		dev_err(dev, "Failed to initialize ancillary i2c device\n");
> -		return PTR_ERR(ancillary);
> +		return PTR_ERR(info->ancillary);
>  	}
>  
> -	ret = i2c_smbus_read_word_data(ancillary, MAX1720X_NRSENSE);
> -	i2c_unregister_device(ancillary);
> -	if (ret < 0)
> -		return ret;
> +	ret = devm_add_action_or_reset(dev, max1720x_unregister_ancillary, info);
> +	if (ret) {
> +		dev_err(dev, "Failed to add unregister callback\n");
> +		goto err;
> +	}
>  
> -	info->rsense = ret;
> +	info->regmap_nv = devm_regmap_init_i2c(info->ancillary,
> +					       &max1720x_nvmem_regmap_cfg);
> +	if (IS_ERR(info->regmap_nv)) {
> +		dev_err(dev, "regmap initialization of nvmem failed\n");
> +		ret = PTR_ERR(info->regmap_nv);
> +		goto err;
> +	}
> +
> +	ret = regmap_read(info->regmap_nv, MAX1720X_NRSENSE, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read sense resistor value\n");
> +		goto err;
> +	}
> +
> +	info->rsense = val;
>  	if (!info->rsense) {
>  		dev_warn(dev, "RSense not calibrated, set 10 mOhms!\n");
>  		info->rsense = 1000; /* in regs in 10^-5 */
>  	}
>  
> +	nvmem = devm_nvmem_register(dev, &nvmem_config);
> +	if (IS_ERR(nvmem)) {
> +		dev_err(dev, "Could not register nvmem!");
> +		ret = PTR_ERR(nvmem);
> +		goto err;
> +	}
> +
>  	return 0;
> +err:
> +	i2c_unregister_device(info->ancillary);

devm_add_action_or_reset() already unregisters on failure, so this
results in a double unregister. Please also simplify 'goto err'
with 'return ret;'.

> +
> +	return ret;
>  }
>  
>  static const struct power_supply_desc max1720x_bat_desc = {
> @@ -299,20 +487,22 @@ static int max1720x_probe(struct i2c_client *client)
>  
>  	psy_cfg.drv_data = info;
>  	psy_cfg.fwnode = dev_fwnode(dev);
> +	i2c_set_clientdata(client, info);
>  	info->regmap = devm_regmap_init_i2c(client, &max1720x_regmap_cfg);
>  	if (IS_ERR(info->regmap))
>  		return dev_err_probe(dev, PTR_ERR(info->regmap),
>  				     "regmap initialization failed\n");
>  
> -	ret = max1720x_probe_sense_resistor(client, info);
> +	ret = max1720x_probe_nvmem(client, info);
>  	if (ret)
> -		return dev_err_probe(dev, ret,
> -				     "Failed to read sense resistor value\n");
> +		return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
>  
>  	bat = devm_power_supply_register(dev, &max1720x_bat_desc, &psy_cfg);
> -	if (IS_ERR(bat))
> +	if (IS_ERR(bat)) {
> +		i2c_unregister_device(info->ancillary);

This is also already handled by devm and must be removed.

>  		return dev_err_probe(dev, PTR_ERR(bat),
>  				     "Failed to register power supply\n");
> +	}
>  
>  	return 0;
>  }

-- Sebastian

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