Message-ID: <efae4037-c22a-40be-8ba9-7c1c12ece042@topic.nl>
Date: Fri, 12 Apr 2024 08:53:58 +0200
From: Mike Looijmans <mike.looijmans@...ic.nl>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
CC: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] power: supply: ltc3350-charger: Add driver

Assume "yes, will do" to any feedback I've skipped.

On 10-04-2024 09:56, Sebastian Reichel wrote:
> Hi,
>
> On Tue, Apr 09, 2024 at 03:54:41PM +0200, Mike Looijmans wrote:
>> The LTC3350 is a backup power controller that can charge and monitor
>> a series stack of one to four supercapacitors.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@...ic.nl>
>>
>> ---
> please share output of
> ./tools/testing/selftests/power_supply/test_power_supply_properties.sh
> below the fold with your next submission. It's useful for verifying,
> that you got the unit scaling correct for the standard properties :)

Will do. Did a quick run on the driver as it is now, that yields the 
following output:

(Any thoughts on the "arithmetic syntax error" messages?)

# ./test_power_supply_properties.sh
TAP version 13
/test_power_supply_properties.sh: line 26: arithmetic syntax error
1..
# Testing device ltc3350
ok 1 ltc3350.exists
ok 2 ltc3350.uevent.NAME
ok 3 ltc3350.sysfs.type
ok 4 ltc3350.uevent.TYPE
ok 5 ltc3350.sysfs.usb_type # SKIP
# Reported: '1' ()
ok 6 ltc3350.sysfs.online
ok 7 ltc3350.sysfs.present # SKIP
# Reported: 'Full'
ok 8 ltc3350.sysfs.status
ok 9 ltc3350.sysfs.capacity # SKIP
ok 10 ltc3350.sysfs.capacity_level # SKIP
ok 11 ltc3350.sysfs.model_name # SKIP
ok 12 ltc3350.sysfs.manufacturer # SKIP
ok 13 ltc3350.sysfs.serial_number # SKIP
ok 14 ltc3350.sysfs.technology # SKIP
ok 15 ltc3350.sysfs.cycle_count # SKIP
ok 16 ltc3350.sysfs.scope # SKIP
# Reported: '3200000' uA (3200 mA)
ok 17 ltc3350.sysfs.input_current_limit
ok 18 ltc3350.sysfs.input_voltage_limit # SKIP
# Reported: '23828220' uV (23.8282 V)
ok 19 ltc3350.sysfs.voltage_now
ok 20 ltc3350.sysfs.voltage_min # SKIP
ok 21 ltc3350.sysfs.voltage_max # SKIP
ok 22 ltc3350.sysfs.voltage_min_design # SKIP
ok 23 ltc3350.sysfs.voltage_max_design # SKIP
# Reported: '711600' uA (711.6 mA)
ok 24 ltc3350.sysfs.current_now
ok 25 ltc3350.sysfs.current_max # SKIP
ok 26 ltc3350.sysfs.charge_now # SKIP
ok 27 ltc3350.sysfs.charge_full # SKIP
ok 28 ltc3350.sysfs.charge_full_design # SKIP
ok 29 ltc3350.sysfs.power_now # SKIP
ok 30 ltc3350.sysfs.energy_now # SKIP
ok 31 ltc3350.sysfs.energy_full # SKIP
ok 32 ltc3350.sysfs.energy_full_design # SKIP
ok 33 ltc3350.sysfs.energy_full_design # SKIP
# Totals: pass:9 fail:0 xfail:0 xpass:0 skip:24 error:0
/test_power_supply_properties.sh: line 102: arithmetic syntax error

>> (no changes since v2)
>>
>> Changes in v2:
>> Duplicate "vin_ov" and "vin_uv" attributes
>>
>>   drivers/power/supply/Kconfig           |  10 +
>>   drivers/power/supply/Makefile          |   1 +
>>   drivers/power/supply/ltc3350-charger.c | 718 +++++++++++++++++++++++++
>>   3 files changed, 729 insertions(+)
>>   create mode 100644 drivers/power/supply/ltc3350-charger.c
>>
>> +/* Custom properties */
>> +
>> +static ssize_t ltc3350_attr_show(struct device *dev,
>> +				 struct device_attribute *attr, char *buf,
>> +				 unsigned int reg, unsigned int scale)
>> +{
>> +	struct power_supply *psy = to_power_supply(dev);
>> +	struct ltc3350_info *info = power_supply_get_drvdata(psy);
>> +	unsigned int regval;
>> +	int ret;
>> +
>> +	ret = regmap_read(info->regmap, reg, &regval);
>> +	if (ret)
>> +		return ret;
>> +
>> +	regval *= scale; /* Scale is in 10 μV units */
> please keep custom uAPI consistent with the general uAPI and use µV.

I'll amend the comment to clarify that this is about the scale factor 
passed into this method. This prevents overflow while keeping all 
calculations in 32 bits.


>
>> +	regval /= 10;
>> +
>> +	return sprintf(buf, "%u\n", regval);
>> +}
>> +
>> +static ssize_t ltc3350_attr_store(struct device *dev, struct device_attribute *attr,
>> +				  const char *buf, size_t count,
>> +				  unsigned int reg, unsigned int scale)
>> +{
>> +	struct power_supply *psy = to_power_supply(dev);
>> +	struct ltc3350_info *info = power_supply_get_drvdata(psy);
>> +	unsigned long val;
>> +	int ret;
>> +
>> +	ret = kstrtoul(buf, 10, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val *= 10;
>> +	val = DIV_ROUND_CLOSEST(val, scale); /* Scale is in 10 μV units */
> obviously also applied to writing.
>
>> +
>> +	ret = regmap_write(info->regmap, reg, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* When writing to one of the LVL registers, update the alarm mask */
>> +	if (reg >= LTC3350_REG_CAP_UV_LVL && reg <= LTC3350_REG_CAP_LO_LVL)
>> +		ltc3350_enable_alarm(info, reg);
>> +
>> +	return count;
>> +}
>> +
>> +#define LTC3350_DEVICE_ATTR_RO(_name, _reg, _scale)				\
>> +static ssize_t _name##_show(struct device *dev, struct device_attribute *attr,	\
>> +			    char *buf)						\
>> +{										\
>> +	return ltc3350_attr_show(dev, attr, buf, _reg, _scale);			\
>> +}										\
>> +static DEVICE_ATTR_RO(_name)
>> +
>> +#define LTC3350_DEVICE_ATTR_RW(_name, _reg, _scale)				\
>> +static ssize_t _name##_show(struct device *dev, struct device_attribute *attr,	\
>> +			    char *buf)						\
>> +{										\
>> +	return ltc3350_attr_show(dev, attr, buf, _reg, _scale);			\
>> +}										\
>> +static ssize_t _name##_store(struct device *dev, struct device_attribute *attr, \
>> +			     const char *buf, size_t count)			\
>> +{										\
>> +	return ltc3350_attr_store(dev, attr, buf, count, _reg, _scale);		\
>> +}										\
>> +static DEVICE_ATTR_RW(_name)
>> +
>> +/* Shunt voltage, 183.5μV per LSB */
>> +LTC3350_DEVICE_ATTR_RW(vshunt, LTC3350_REG_VSHUNT, 1835);
>> +
>> +/* Single capacitor voltages, 183.5μV per LSB */
>> +LTC3350_DEVICE_ATTR_RO(vcap1, LTC3350_REG_MEAS_VCAP1, 1835);
>> +LTC3350_DEVICE_ATTR_RO(vcap2, LTC3350_REG_MEAS_VCAP2, 1835);
>> +LTC3350_DEVICE_ATTR_RO(vcap3, LTC3350_REG_MEAS_VCAP3, 1835);
>> +LTC3350_DEVICE_ATTR_RO(vcap4, LTC3350_REG_MEAS_VCAP4, 1835);
>> +LTC3350_DEVICE_ATTR_RW(cap_uv, LTC3350_REG_CAP_UV_LVL, 1835);
>> +LTC3350_DEVICE_ATTR_RW(cap_ov, LTC3350_REG_CAP_OV_LVL, 1835);
>> +
>> +/* General purpose input, 183.5μV per LSB */
>> +LTC3350_DEVICE_ATTR_RO(gpi, LTC3350_REG_MEAS_GPI, 1835);
>> +LTC3350_DEVICE_ATTR_RW(gpi_uv, LTC3350_REG_GPI_UV_LVL, 1835);
>> +LTC3350_DEVICE_ATTR_RW(gpi_ov, LTC3350_REG_GPI_OV_LVL, 1835);
>> +
>> +/* Input voltage, 2.21mV per LSB */
>> +LTC3350_DEVICE_ATTR_RO(vin, LTC3350_REG_MEAS_VIN, 22100);
>> +LTC3350_DEVICE_ATTR_RW(vin_uv, LTC3350_REG_VIN_UV_LVL, 22100);
>> +LTC3350_DEVICE_ATTR_RW(vin_ov, LTC3350_REG_VIN_OV_LVL, 22100);
>> +
>> +/* Capacitor stack voltage, 1.476 mV per LSB */
>> +LTC3350_DEVICE_ATTR_RO(vcap, LTC3350_REG_MEAS_VCAP, 14760);
>> +LTC3350_DEVICE_ATTR_RW(vcap_uv, LTC3350_REG_VCAP_UV_LVL, 14760);
>> +LTC3350_DEVICE_ATTR_RW(vcap_ov, LTC3350_REG_VCAP_OV_LVL, 14760);
> I suppose it would be sensible to expose this as a second
> power_supply device of type battery with ltc3350_config.supplied_to
> set to this second power_supply device.
>
> Then you can map
>
> LTC3350_REG_MEAS_VCAP -> VOLTAGE_NOW
> LTC3350_REG_VCAP_UV_LVL -> VOLTAGE_MIN
> LTC3350_REG_VCAP_OV_LVL -> VOLTAGE_MAX
> LTC3350_REG_VSHUNT -> CURRENT_NOW
> TECHNOLOGY = POWER_SUPPLY_TECHNOLOGY_CAPACITOR (new)

Makes sense.

Should I create a separate patch that adds the new properties?


> Also the single capacitor voltages are similar to per-cell voltage
> information, so I think we should create generic properties for
> that:
>
> LTC3350_REG_NUM_CAPS   -> NUMBER_OF_CELLS (new)
> LTC3350_REG_MEAS_VCAP1 -> CELL1_VOLTAGE_NOW (new)
> LTC3350_REG_MEAS_VCAP2 -> CELL2_VOLTAGE_NOW (new)
> LTC3350_REG_MEAS_VCAP3 -> CELL3_VOLTAGE_NOW (new)
> LTC3350_REG_MEAS_VCAP4 -> CELL4_VOLTAGE_NOW (new)
> LTC3350_REG_CAP_UV_LVL -> CELL_VOLTAGE_MIN (new)
> LTC3350_REG_CAP_OV_LVL -> CELL_VOLTAGE_MAX (new)
>
> ...

>> +
>> +static int ltc3350_probe(struct i2c_client *client)
>> +{
>> +	struct i2c_adapter *adapter = client->adapter;
>> +	struct device *dev = &client->dev;
>> +	struct ltc3350_info *info;
>> +	struct power_supply_config ltc3350_config = {};
>> +	int ret;
>> +
>> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
>> +		dev_err(dev, "No support for SMBUS_WORD_DATA\n");
>> +		return -ENODEV;
>> +	}
> return dev_err_probe(dev, -ENODEV, "No support for SMBUS_WORD_DATA\n");
>
> But I think this can just be dropped. devm_regmap_init_i2c() should
> generate an error, if the i2c adapter requirements are not met.

It's quite interesting to see what other drivers do here. Most report a 
message, and there's no consensus on the value returned.


>> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	info->client = client;
>> +	i2c_set_clientdata(client, info);
>> +
>> +	info->regmap = devm_regmap_init_i2c(client, &ltc3350_regmap_config);
>> +	if (IS_ERR(info->regmap)) {
>> +		dev_err(dev, "Failed to initialize register map\n");
>> +		return PTR_ERR(info->regmap);
>> +	}
> dev_err_probe()
>
>> +
>> +	ret = device_property_read_u32(dev, "lltc,rsnsc-micro-ohms",
>> +				       &info->rsnsc);
>> +	if (ret) {
>> +		dev_err(dev, "Missing lltc,rsnsc-micro-ohms property\n");
>> +		return ret;
>> +	}
> dev_err_probe()
>
>> +	if (!info->rsnsc)
>> +		return -EINVAL;
>> +
>> +	ret = device_property_read_u32(dev, "lltc,rsnsi-micro-ohms",
>> +				       &info->rsnsi);
>> +	if (ret) {
>> +		dev_err(dev, "Missing lltc,rsnsi-micro-ohms property\n");
>> +		return ret;
>> +	}
> dev_err_probe()
>
>> +	if (!info->rsnsi)
>> +		return -EINVAL;
>> +
>> +	/* Clear and disable all interrupt sources*/
>> +	ret = regmap_write(info->regmap, LTC3350_REG_CLR_ALARMS, 0xFFFF);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to write configuration\n");
>> +		return ret;
>> +	}
> dev_err_probe()
>
>> +	regmap_write(info->regmap, LTC3350_REG_MSK_ALARMS, 0);
>> +	regmap_write(info->regmap, LTC3350_REG_MSK_MON_STATUS, 0);
>> +
>> +	ltc3350_config.of_node = dev->of_node;
> replace with
>
> ltc3350_config.fwnode = dev_fwnode(dev);
>
>> +	ltc3350_config.drv_data = info;
>> +	ltc3350_config.attr_grp = ltc3350_attr_groups;
>> +
>> +	info->charger = devm_power_supply_register(dev, &ltc3350_desc,
>> +						   &ltc3350_config);
>> +	if (IS_ERR(info->charger)) {
>> +		dev_err(dev, "Failed to register charger\n");
>> +		return PTR_ERR(info->charger);
>> +	}
> dev_err_probe()
>
>> +
>> +	/* Enable interrupts on status changes */
>> +	regmap_write(info->regmap, LTC3350_REG_MSK_MON_STATUS,
>> +		     LTC3350_MON_POWER_FAILED | LTC3350_MON_POWER_RETURNED);
>> +
>> +	return 0;
>> +}
>> +
>> +static void ltc3350_alert(struct i2c_client *client, enum i2c_alert_protocol type,
>> +			  unsigned int flag)
>> +{
>> +	struct ltc3350_info *info = i2c_get_clientdata(client);
>> +
>> +	if (type != I2C_PROTOCOL_SMBUS_ALERT)
>> +		return;
>> +
>> +	power_supply_changed(info->charger);
>> +}
>> +
>> +static const struct i2c_device_id ltc3350_i2c_id_table[] = {
>> +	{ "ltc3350", 0 },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ltc3350_i2c_id_table);
>> +
>> +static const struct of_device_id ltc3350_of_match[] = {
>> +	{ .compatible = "lltc,ltc3350", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, ltc3350_of_match);
>> +
>> +static struct i2c_driver ltc3350_driver = {
>> +	.probe		= ltc3350_probe,
>> +	.alert		= ltc3350_alert,
>> +	.id_table	= ltc3350_i2c_id_table,
>> +	.driver = {
>> +		.name		= "ltc3350-charger",
>> +		.of_match_table	= of_match_ptr(ltc3350_of_match),
> Please check what of_match_ptr() does and then drop it :)

Interesting :)


>
>> +	},
>> +};
>> +module_i2c_driver(ltc3350_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Mike Looijmans <mike.looijmans@...ic.nl>");
>> +MODULE_DESCRIPTION("LTC3350 charger driver");
> Thanks for your patch,
>
> -- Sebastian


-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@...ic.nl
W: www.topic.nl

Powered by blists - more mailing lists