Re: [PATCH v5 5/5] power: supply: ltc3350-charger: Add driver
From: Mike Looijmans
Date: Tue May 28 2024 - 01:18:57 EST
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@xxxxxxxx>
---
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@xxxxxxxx>");
+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@xxxxxxxx
W: www.topic.nl