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

[Dimitri Fedrau <dima.fedrau@gmail.com>]

Am Mon, Sep 02, 2024 at 09:55:42PM +0000 schrieb Sebastian Reichel:
> 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@gmail.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 :)
>

Yes, I noticed shortly after sending V2, that I missed to remove all
i2c_unregister_device calls in the error path. Already prepared V3
handling this and also the missing header "linux/nvmem-provider.h".
Thanks for reviewing so quickly, I just hoped to get away with this by
sending V3 before you notice. :)

> > ---
> >  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>
>
Will fix it.

> > +		.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;'.
>
Will fix it.

> > +
> > +	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.
>
Will fix it.
> >  		return dev_err_probe(dev, PTR_ERR(bat),
> >  				     "Failed to register power supply\n");
> > +	}
> >  
> >  	return 0;
> >  }
> 
> -- Sebastian

Best regards
Dimitri Fedrau