Message-ID: <lxxwadaf2mrghy2kygm3cucb7ygl5qu4dxnmpbancrm2hwjm4g@5eudactyudca>
Date: Sun, 14 Apr 2024 19:29:56 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Mike Looijmans <mike.looijmans@...ic.nl>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Nícolas F. R. A. Prado <nfraprado@...labora.com>
Subject: Re: [PATCH v3 2/2] power: supply: ltc3350-charger: Add driver

[+cc Nicolas]

Hello Mike,

On Fri, Apr 12, 2024 at 08:53:58AM +0200, Mike Looijmans wrote:
> > 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?)

The script contains some bash specific shell extensions and should
use /bin/bash instead of /bin/sh in the shebang. Just call it with
/bin/bash ./tools/testing/... and you should get rid of them :)

Nicolas, do you want to send a fix for that to Shuah with Reported-by
from Mike?

[...]

> # Reported: '1' ()
> ok 6 ltc3350.sysfs.online

[...]

> # Reported: '711600' uA (711.6 mA)
> ok 24 ltc3350.sysfs.current_now

So it's full, but still getting charged with 0.7 Amps at ~23V
(i.e. 16W)? That seems quite high.

[...]

> > > +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.

ack

[...]

> > > +/* 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?

Yes, make it one extra patch adding POWER_SUPPLY_TECHNOLOGY_CAPACITOR
and one extra patch for the cell information properties. Also do not
forget to update all necessary files:

 * Documentation/ABI/testing/sysfs-class-power
 * include/linux/power_supply.h
 * drivers/power/supply/power_supply_sysfs.c

> > 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)
> > 
> > ...

After re-reading it: This only works for serial cells, but not for
parallel ones. While it's technically not possible to measure
parallel cells, it might be desired to expose the exact
configuration at some point. Thus it should be
NUMBER_OF_SERIAL_CELLS. Also the documentation for
CELL<X>_VOLTAGE_NOW should mention that this might measure more than
one cell, if there are multiple cells connected in parallel.

[...]

> > > +
> > > +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.

I checked and devm_regmap_init_i2c() calls into regmap_get_i2c_bus()
and that contains the same check. If it fails, the bus will not be
found and the function returns -ENOTSUPP. It should be fine to remove
the driver specific extra handling with the error being printed for
devm_regmap_init_i2c(). Considering this is mostly a hardware sanity
check, I would expect this error to only happen on developer
systems. A developer should be able to figure out what that means.

Greetings,

-- Sebastian

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

Powered by blists - more mailing lists