Message-ID: <1468f494-8c36-4ffa-985e-6737bcd9e13d@topic.nl>
Date: Tue, 28 May 2024 07:18:35 +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 v5 5/5] power: supply: ltc3350-charger: Add driver

On 27-05-2024 22:40, Sebastian Reichel wrote:
> Hi,
>
> Sorry for being late with reviewing v5. Considering this adds quite
> a bit of new userspace ABI, I did not want to rush this.

The project is still ongoing, so I can spend some time still.


>
> On Tue, Apr 16, 2024 at 02:18:18PM +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>
>>
>> ---
>>
>> Changes in v5:
>> Fix ltc3350_set_scaled() arguments in "store" routines
>> Measurement values are signed
>> Report technology as "Capacitor"
>> Report "full" when "capgd" asserts
>> Move charge current to "current_now" of capacitor
>> Add ABI documentation
>>
>> Changes in v4:
>> Split into "charger" and "capacitor" parts
>> Use (new) standard properties
>> Header include fixups
>> Explain local "scale" units
>> Drop i2c_check_functionality
>> Use dev_err_probe
>> Use dev_fwnode
>> Drop of_match_ptr
>>
>> Changes in v2:
>> Duplicate "vin_ov" and "vin_uv" attributes
>>
>>   .../ABI/testing/sysfs-class-power-ltc3350     |  88 ++
>>   drivers/power/supply/Kconfig                  |  10 +
>>   drivers/power/supply/Makefile                 |   1 +
>>   drivers/power/supply/ltc3350-charger.c        | 789 ++++++++++++++++++
>>   4 files changed, 888 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-class-power-ltc3350
>>   create mode 100644 drivers/power/supply/ltc3350-charger.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-power-ltc3350 b/Documentation/ABI/testing/sysfs-class-power-ltc3350
>> new file mode 100644
>> index 000000000000..d4a2bb0fb62b
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-power-ltc3350
>> @@ -0,0 +1,88 @@
>> +What:		/sys/class/power_supply/ltc3350/charge_status
>> +Date:		April 2024
>> +KernelVersion:	6.9
>> +Description:
>> +		Detailed charge status information as reported by the chip. This
>> +		returns the raw register value in hex.
>> +
>> +		Access: Read
> With regmap you should be able to get register content via debugfs
> for debug purposes. Is there any other reason this is needed?

Not really, and it turns out we're not using it either. So this should 
go away.

>
>> +What:		/sys/class/power_supply/ltc3350/vshunt
>> +Date:		April 2024
>> +KernelVersion:	6.9
>> +Description:
>> +		Voltage for "shunting" the capacitors in the stack. When the
>> +		capacitor voltage is above this value, the chip will discharge
>> +		the excess voltage using a shunt resistor.
>> +		This is typically used to limit the voltage on a single cell,
>> +		to compensate for imbalance and prevent damaging the capacitor
>> +		while charging. It can also be used to forcibly discharge the
>> +		capacitors.
>> +
>> +		Access: Read, Write
>> +
>> +		Valid values: In microvolts, defaults to 2.7V
>> +
>> +What:		/sys/class/power_supply/ltc3350/gpi
>> +Date:		April 2024
>> +KernelVersion:	6.9
>> +Description:
>> +		General purpose input voltage. Returns a single measurement.
> I think this should get this extra sentence:
>
> "For example used for temperature measurement of the supercapacitor
> stack using an NTC thermistor."

ok


>> +
>> +		Access: Read
>> +
>> +		Valid values: In microvolts
>> +
>> +What:		/sys/class/power_supply/ltc3350/gpi_ov
>> +Date:		April 2024
>> +KernelVersion:	6.9
>> +Description:
>> +		General purpose input voltage overvoltage level. Triggers an
>> +		alert for userspace when the voltage goes above this value.
>> +
>> +		Access: Read, Write
>> +
>> +		Valid values: In microvolts
>> +
>> +What:		/sys/class/power_supply/ltc3350/gpi_uv
>> +Date:		April 2024
>> +KernelVersion:	6.9
>> +Description:
>> +		General purpose input voltage undervoltage level. Triggers an
>> +		alert for userspace when the voltage drops below this value.
>> +
>> +		Access: Read, Write
>> +
>> +		Valid values: In microvolts
> The custom properties are not part of the uevent, so I guess there
> won't be much of an alert. A sensible way to properly handle this
> would be registration of a single-channel IIO ADC device. Are you
> using this feature? Otherwise it might be sensible to drop GPI
> support for now.

Lucky guess above, we're indeed using it for NTC measurement.

That the uevent doesn't include the measurement is no problem. Userspace 
sets various alarm levels, listens to netlink and when an event occurs 
it just reads the device's properties anyway to determine what to do.

Registering IIO is an option too, but makes things complex. It might be 
better to leave it out and add the "gpi" functionality in a later patch 
adding IIO maintainers to the reviewers.


>
>> +What:		/sys/class/power_supply/ltc3350/vin
>> +Date:		April 2024
>> +KernelVersion:	6.9
>> +Description:
>> +		Charger input voltage. Returns a single measurement.
>> +
>> +		Access: Read
>> +
>> +		Valid values: In microvolts
>> +
>> +What:		/sys/class/power_supply/ltc3350/vin_ov
>> +Date:		April 2024
>> +KernelVersion:	6.9
>> +Description:
>> +		Input voltage overvoltage level. Triggers an alert for userspace
>> +		when the voltage goes above this value.
>> +
>> +		Access: Read, Write
>> +
>> +		Valid values: In microvolts
>> +
>> +What:		/sys/class/power_supply/ltc3350/vin_uv
>> +Date:		April 2024
>> +KernelVersion:	6.9
>> +Description:
>> +		Input voltage undervoltage level. Triggers an alert for
>> +		userspace when the voltage drops below this value.
>> +
>> +		Access: Read, Write
>> +
>> +		Valid values: In microvolts
> I added some bits about vin later.

>
>> ...

>> +static int ltc3350_get_property(struct power_supply *psy, enum power_supply_property psp,
>> +				union power_supply_propval *val)
>> +{
>> +	struct ltc3350_info *info = power_supply_get_drvdata(psy);
>> +
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_STATUS:
>> +		return ltc3350_get_status(info, val);
>> +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
>> +		return ltc3350_get_charge_type(info, val);
>> +	case POWER_SUPPLY_PROP_ONLINE:
>> +		return ltc3350_get_online(info, val);
>> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +		return ltc3350_get_scaled(info, LTC3350_REG_MEAS_VOUT, 22100, &val->intval);
>> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
>> +		return ltc3350_get_scaled(info, LTC3350_REG_VOUT_UV_LVL, 22100, &val->intval);
>> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX:
>> +		return ltc3350_get_scaled(info, LTC3350_REG_VOUT_OV_LVL, 22100, &val->intval);
> For USB chargers VOLTAGE_NOW/MIN/MAX refers to VBUS, which is the
> voltage on the USB lines and thus the input voltage. From my
> understanding of the LTC3350 this would be VIN and not VOUT. With
> VOUT being supplied by either VIN or the Capacitors.
>
> So I think your custom vin/vin_uv/vin_ov should be exposed with the
> above properties.
>
> My understanding for VOUT is, that this is basically the system
> supply - I would expect more regulators to follow, which convert
> it to typical voltages like 3.3V or 1.8V. In that case it would
> make sense to expose VOUT as regulator, so that it can be referenced
> as vin-supply. You can find a few examples for charger drivers doing
> that for USB-OTG functionality.

Correct, VOUT would normally be feeding further DC-DC converters. And 
for that part, the LTC3350 is a (buck-boost) regulator. I'll dig into it.

If you could name some specific drivers that you'd consider good 
examples, that'd be helpful (and avoids me picking a bad apple).


> ...
>> +
>> +static const struct i2c_device_id ltc3350_i2c_id_table[] = {
>> +	{ "ltc3350", 0 },
> please drop the 0.
ok
>
>> +	{ },
>> +};
>> +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	= ltc3350_of_match,
>> +	},
>> +};
>> +module_i2c_driver(ltc3350_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Mike Looijmans <mike.looijmans@...ic.nl>");
>> +MODULE_DESCRIPTION("LTC3350 charger driver");
> Greetings,
>
> -- 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