On 8/23/22 22:30, Chris Morgan wrote:
From: Chris Morgan <macromorgan@xxxxxxxxxxx>
Add support for the Rockchip rk817 battery charger integrated into the
rk817 PMIC.
Signed-off-by: Chris Morgan <macromorgan@xxxxxxxxxxx>
Signed-off-by: Maya Matuszczyk <maccraft123mc@xxxxxxxxx>
---
drivers/power/supply/Kconfig | 6 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/rk817_charger.c | 1157 ++++++++++++++++++++++++++
3 files changed, 1164 insertions(+)
create mode 100644 drivers/power/supply/rk817_charger.c
+
+static void rk817_charging_monitor(struct work_struct *work)
+{
+ struct rk817_charger *charger;
+
+ charger = container_of(work, struct rk817_charger, work.work);
+
+ rk817_read_props(charger);
+
+ /* Run every 8 seconds like the BSP driver did. */
+ queue_delayed_work(system_wq, &charger->work, msecs_to_jiffies(8000));
+}
I really think we would benefit from some more framework code which
could handle the periodic polling tasks and the coulomb counter drift
corrections when battery is full/relaxed. I think I might revive the
simple-gauge patch series...
+
+static int rk817_charger_probe(struct platform_device *pdev)
+{
+
+ charger->sleep_filter_current_ua = of_value;
+
+ charger->bat_ps = devm_power_supply_register(&pdev->dev,
+ &rk817_bat_desc, &pscfg);
+
+ charger->chg_ps = devm_power_supply_register(&pdev->dev,
+ &rk817_chg_desc, &pscfg);
Hmm. I think I should respin the patch which added interface for getting
the battery info w/o psy-device. Now we need to take into account the
situation where the psy-core accesses the driver after the registration
- and prior filling the battery details from the battery node (below) :/
+
+ if (IS_ERR(charger->chg_ps))
+ return dev_err_probe(dev, -EINVAL,
+ "Battery failed to probe\n");
+
+ if (IS_ERR(charger->chg_ps))
+ return dev_err_probe(dev, -EINVAL,
+ "Charger failed to probe\n");
+
+ ret = power_supply_get_battery_info(charger->bat_ps,
+ &bat_info);
+ if (ret) {
+ return dev_err_probe(dev, ret,
+ "Unable to get battery info: %d\n", ret);
+ } > +
+ if ((!bat_info->charge_full_design_uah) ||
+ (!bat_info->voltage_min_design_uv) ||
+ (!bat_info->voltage_max_design_uv) ||
+ (!bat_info->constant_charge_voltage_max_uv) ||
+ (!bat_info->constant_charge_current_max_ua) ||
+ (!bat_info->charge_term_current_ua)) {
+ return dev_err_probe(dev, -EINVAL,
+ "Required battery info missing.\n");
+ }
Just a question - should the values be compared to -EINVAL (I think the
power_supply_get_battery_info() did internally initialize many of the
fields to -EINVAL and not to 0?). Maybe I am wrong...
+
+ charger->bat_charge_full_design_uah = bat_info->charge_full_design_uah;
+ charger->bat_voltage_min_design_uv = bat_info->voltage_min_design_uv;
+ charger->bat_voltage_max_design_uv = bat_info->voltage_max_design_uv;
+
Generally, I did _really_ like the proper commenting/documenting of the
driver. In my eyes this looked like one nice piece of a driver.
When the error checking of values returned by the
power_supply_get_battery_info() is checked - FWIW:
Reviewed-By: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
Yours
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
