Message-ID: <20240831181535.GA11135@debian>
Date: Sat, 31 Aug 2024 20:15:35 +0200
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] power: supply: max1720x: add read support for nvmem

Am Tue, Aug 27, 2024 at 07:28:43PM +0200 schrieb Sebastian Reichel:
> Hi,
> 
> On Wed, Jul 17, 2024 at 08:37:57PM 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
> > 
> > ---
> >  drivers/power/supply/max1720x_battery.c | 215 ++++++++++++++++++++++--
> >  1 file changed, 200 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> > index edc262f0a62f..175f36e83b85 100644
> > --- a/drivers/power/supply/max1720x_battery.c
> > +++ b/drivers/power/supply/max1720x_battery.c
> > @@ -16,7 +16,9 @@
> >  #include <asm/unaligned.h>
> >  
> >  /* Nonvolatile registers */
> > +#define MAX1720X_NXTABLE0		0x80
> >  #define MAX1720X_NRSENSE		0xCF	/* RSense in 10^-5 Ohm */
> > +#define MAX1720X_NDEVICE_NAME4		0xDF
> >  
> >  /* ModelGauge m5 */
> >  #define MAX172XX_STATUS			0x00	/* Status */
> > @@ -46,6 +48,8 @@ static const char *const max17205_model = "MAX17205";
> >  
> >  struct max1720x_device_info {
> >  	struct regmap *regmap;
> > +	struct regmap *regmap_nv;
> > +	struct i2c_client *ancillary;
> >  	int rsense;
> >  };
> >  
> > @@ -106,6 +110,134 @@ static const struct regmap_config max1720x_regmap_cfg = {
> >  	.cache_type = REGCACHE_RBTREE,
> >  };
> >  
> > +static const struct regmap_range max1720x_nvmem_allow[] = {
> > +	regmap_reg_range(MAX1720X_NXTABLE0, MAX1720X_NDEVICE_NAME4),
> > +};
> > +
> > +static const struct regmap_range max1720x_nvmem_deny[] = {
> > +	regmap_reg_range(0x00, 0x7F),
> > +	regmap_reg_range(0xE0, 0xFF),
> > +};
> > +
> > +static const struct regmap_access_table max1720x_nvmem_regs = {
> > +	.yes_ranges	= max1720x_nvmem_allow,
> > +	.n_yes_ranges	= ARRAY_SIZE(max1720x_nvmem_allow),
> > +	.no_ranges	= max1720x_nvmem_deny,
> > +	.n_no_ranges	= ARRAY_SIZE(max1720x_nvmem_deny),
> > +};
> > +
> > +static const struct regmap_config max1720x_nvmem_regmap_cfg = {
> > +	.reg_bits = 8,
> > +	.val_bits = 16,
> > +	.max_register = MAX1720X_NDEVICE_NAME4,
> > +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> > +	.rd_table = &max1720x_nvmem_regs,
> > +};
> > +
> > +static const struct nvmem_cell_info max1720x_nvmem_cells[] = {
> > +	{ .name = "nXTable0",  .offset = 0,  .bytes = 2, },
> > +	{ .name = "nXTable1",  .offset = 2,  .bytes = 2, },
> > +	{ .name = "nXTable2",  .offset = 4,  .bytes = 2, },
> > +	{ .name = "nXTable3",  .offset = 6,  .bytes = 2, },
> > +	{ .name = "nXTable4",  .offset = 8,  .bytes = 2, },
> > +	{ .name = "nXTable5",  .offset = 10, .bytes = 2, },
> > +	{ .name = "nXTable6",  .offset = 12, .bytes = 2, },
> > +	{ .name = "nXTable7",  .offset = 14, .bytes = 2, },
> > +	{ .name = "nXTable8",  .offset = 16, .bytes = 2, },
> > +	{ .name = "nXTable9",  .offset = 18, .bytes = 2, },
> > +	{ .name = "nXTable10", .offset = 20, .bytes = 2, },
> > +	{ .name = "nXTable11", .offset = 22, .bytes = 2, },
> > +	{ .name = "nUser18C",  .offset = 24, .bytes = 2, },
> > +	{ .name = "nUser18D",  .offset = 26, .bytes = 2, },
> > +	{ .name = "nODSCTh",   .offset = 28, .bytes = 2, },
> > +	{ .name = "nODSCCfg",  .offset = 30, .bytes = 2, },
> > +
> > +	{ .name = "nOCVTable0",  .offset = 32, .bytes = 2, },
> > +	{ .name = "nOCVTable1",  .offset = 34, .bytes = 2, },
> > +	{ .name = "nOCVTable2",  .offset = 36, .bytes = 2, },
> > +	{ .name = "nOCVTable3",  .offset = 38, .bytes = 2, },
> > +	{ .name = "nOCVTable4",  .offset = 40, .bytes = 2, },
> > +	{ .name = "nOCVTable5",  .offset = 42, .bytes = 2, },
> > +	{ .name = "nOCVTable6",  .offset = 44, .bytes = 2, },
> > +	{ .name = "nOCVTable7",  .offset = 46, .bytes = 2, },
> > +	{ .name = "nOCVTable8",  .offset = 48, .bytes = 2, },
> > +	{ .name = "nOCVTable9",  .offset = 50, .bytes = 2, },
> > +	{ .name = "nOCVTable10", .offset = 52, .bytes = 2, },
> > +	{ .name = "nOCVTable11", .offset = 54, .bytes = 2, },
> > +	{ .name = "nIChgTerm",   .offset = 56, .bytes = 2, },
> > +	{ .name = "nFilterCfg",  .offset = 58, .bytes = 2, },
> > +	{ .name = "nVEmpty",     .offset = 60, .bytes = 2, },
> > +	{ .name = "nLearnCfg",   .offset = 62, .bytes = 2, },
> > +
> > +	{ .name = "nQRTable00",  .offset = 64, .bytes = 2, },
> > +	{ .name = "nQRTable10",  .offset = 66, .bytes = 2, },
> > +	{ .name = "nQRTable20",  .offset = 68, .bytes = 2, },
> > +	{ .name = "nQRTable30",  .offset = 70, .bytes = 2, },
> > +	{ .name = "nCycles",     .offset = 72, .bytes = 2, },
> > +	{ .name = "nFullCapNom", .offset = 74, .bytes = 2, },
> > +	{ .name = "nRComp0",     .offset = 76, .bytes = 2, },
> > +	{ .name = "nTempCo",     .offset = 78, .bytes = 2, },
> > +	{ .name = "nIAvgEmpty",  .offset = 80, .bytes = 2, },
> > +	{ .name = "nFullCapRep", .offset = 82, .bytes = 2, },
> > +	{ .name = "nVoltTemp",   .offset = 84, .bytes = 2, },
> > +	{ .name = "nMaxMinCurr", .offset = 86, .bytes = 2, },
> > +	{ .name = "nMaxMinVolt", .offset = 88, .bytes = 2, },
> > +	{ .name = "nMaxMinTemp", .offset = 90, .bytes = 2, },
> > +	{ .name = "nSOC",        .offset = 92, .bytes = 2, },
> > +	{ .name = "nTimerH",     .offset = 94, .bytes = 2, },
> > +
> > +	{ .name = "nConfig",    .offset = 96,  .bytes = 2, },
> > +	{ .name = "nRippleCfg", .offset = 98,  .bytes = 2, },
> > +	{ .name = "nMiscCfg",   .offset = 100, .bytes = 2, },
> > +	{ .name = "nDesignCap", .offset = 102, .bytes = 2, },
> > +	{ .name = "nHibCfg",    .offset = 104, .bytes = 2, },
> > +	{ .name = "nPackCfg",   .offset = 106, .bytes = 2, },
> > +	{ .name = "nRelaxCfg",  .offset = 108, .bytes = 2, },
> > +	{ .name = "nConvgCfg",  .offset = 110, .bytes = 2, },
> > +	{ .name = "nNVCfg0",    .offset = 112, .bytes = 2, },
> > +	{ .name = "nNVCfg1",    .offset = 114, .bytes = 2, },
> > +	{ .name = "nNVCfg2",    .offset = 116, .bytes = 2, },
> > +	{ .name = "nSBSCfg",    .offset = 118, .bytes = 2, },
> > +	{ .name = "nROMID0",    .offset = 120, .bytes = 2, },
> > +	{ .name = "nROMID1",    .offset = 122, .bytes = 2, },
> > +	{ .name = "nROMID2",    .offset = 124, .bytes = 2, },
> > +	{ .name = "nROMID3",    .offset = 126, .bytes = 2, },
> > +
> > +	{ .name = "nVAlrtTh",      .offset = 128, .bytes = 2, },
> > +	{ .name = "nTAlrtTh",      .offset = 130, .bytes = 2, },
> > +	{ .name = "nSAlrtTh",      .offset = 132, .bytes = 2, },
> > +	{ .name = "nIAlrtTh",      .offset = 134, .bytes = 2, },
> > +	{ .name = "nUser1C4",      .offset = 136, .bytes = 2, },
> > +	{ .name = "nUser1C5",      .offset = 138, .bytes = 2, },
> > +	{ .name = "nFullSOCThr",   .offset = 140, .bytes = 2, },
> > +	{ .name = "nTTFCfg",       .offset = 142, .bytes = 2, },
> > +	{ .name = "nCGain",        .offset = 144, .bytes = 2, },
> > +	{ .name = "nTCurve",       .offset = 146, .bytes = 2, },
> > +	{ .name = "nTGain",        .offset = 148, .bytes = 2, },
> > +	{ .name = "nTOff",         .offset = 150, .bytes = 2, },
> > +	{ .name = "nManfctrName0", .offset = 152, .bytes = 2, },
> > +	{ .name = "nManfctrName1", .offset = 154, .bytes = 2, },
> > +	{ .name = "nManfctrName2", .offset = 156, .bytes = 2, },
> > +	{ .name = "nRSense",       .offset = 158, .bytes = 2, },
> > +
> > +	{ .name = "nUser1D0",       .offset = 160, .bytes = 2, },
> > +	{ .name = "nUser1D1",       .offset = 162, .bytes = 2, },
> > +	{ .name = "nAgeFcCfg",      .offset = 164, .bytes = 2, },
> > +	{ .name = "nDesignVoltage", .offset = 166, .bytes = 2, },
> > +	{ .name = "nUser1D4",       .offset = 168, .bytes = 2, },
> > +	{ .name = "nRFastVShdn",    .offset = 170, .bytes = 2, },
> > +	{ .name = "nManfctrDate",   .offset = 172, .bytes = 2, },
> > +	{ .name = "nFirstUsed",     .offset = 174, .bytes = 2, },
> > +	{ .name = "nSerialNumber0", .offset = 176, .bytes = 2, },
> > +	{ .name = "nSerialNumber1", .offset = 178, .bytes = 2, },
> > +	{ .name = "nSerialNumber2", .offset = 180, .bytes = 2, },
> > +	{ .name = "nDeviceName0",   .offset = 182, .bytes = 2, },
> > +	{ .name = "nDeviceName1",   .offset = 184, .bytes = 2, },
> > +	{ .name = "nDeviceName2",   .offset = 186, .bytes = 2, },
> > +	{ .name = "nDeviceName3",   .offset = 188, .bytes = 2, },
> > +	{ .name = "nDeviceName4",   .offset = 190, .bytes = 2, },
> > +};
> > +
> >  static const enum power_supply_property max1720x_battery_props[] = {
> >  	POWER_SUPPLY_PROP_PRESENT,
> >  	POWER_SUPPLY_PROP_CAPACITY,
> > @@ -249,31 +381,74 @@ static int max1720x_battery_get_property(struct power_supply *psy,
> >  	return ret;
> >  }
> >  
> > -static int max1720x_probe_sense_resistor(struct i2c_client *client,
> > -					 struct max1720x_device_info *info)
> > +static
> > +int max1720x_nvmem_reg_read(void *priv, unsigned int off, void *val, size_t len)
> > +{
> > +	struct max1720x_device_info *info = priv;
> > +	unsigned int reg = MAX1720X_NXTABLE0 + (off / 2);
> > +
> > +	return regmap_bulk_read(info->regmap_nv, reg, val, len / 2);
> > +}
> > +
> > +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 = {
> > +		.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);
> >  	}
> 
> Use devm_add_action_or_reset() to unregister info->ancillary. It
> results in cleaner code and also fixes a race condition during
> module remove, which frees info->ancillary before the regmap with
> your current patch.
>
> Otherwise LGTM.
>

Hi Sebastian,

thanks for pointing out, didn't know about the race condition. Will fix
it in V2.

Best regards,

Dimitri

> Greetings,
> 
> -- Sebastian
> 
> >  
> > -	ret = i2c_smbus_read_word_data(ancillary, MAX1720X_NRSENSE);
> > -	i2c_unregister_device(ancillary);
> > -	if (ret < 0)
> > -		return 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 = ret;
> > +	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);
> > +
> > +	return ret;
> >  }
> >  
> >  static const struct power_supply_desc max1720x_bat_desc = {
> > @@ -299,24 +474,33 @@ 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);
> >  		return dev_err_probe(dev, PTR_ERR(bat),
> >  				     "Failed to register power supply\n");
> > +	}
> >  
> >  	return 0;
> >  }
> >  
> > +static void max1720x_remove(struct i2c_client *client)
> > +{
> > +	struct max1720x_device_info *info = i2c_get_clientdata(client);
> > +
> > +	i2c_unregister_device(info->ancillary);
> > +}
> > +
> >  static const struct of_device_id max1720x_of_match[] = {
> >  	{ .compatible = "maxim,max17201" },
> >  	{}
> > @@ -329,6 +513,7 @@ static struct i2c_driver max1720x_i2c_driver = {
> >  		.of_match_table = max1720x_of_match,
> >  	},
> >  	.probe = max1720x_probe,
> > +	.remove = max1720x_remove,
> >  };
> >  module_i2c_driver(max1720x_i2c_driver);
> >  
> > -- 
> > 2.39.2
> > 

Powered by blists - more mailing lists