On 8/25/22 18:37, Chris Morgan wrote:
On Thu, Aug 25, 2022 at 03:54:06PM +0300, Matti Vaittinen wrote:
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...
Possibly, does such exist or is that a future endeavor?
No. Such a feature does not exist upstream. It was just some babbling I
did here :) I started drafting something "generic" that would do polling
of coulomb counter / battery state (full/relaxed) and then perform some
'CC resetting' to correct drifitng error and compute the SOC based on CC
values. That would obviously just be a machinery that calls driver
callbacks. I did support for two ROHM chargers using this concept but I
had to switch to some urgent tasks before I managed to do proper
testing. Might get back to that later though (depending on the other
duties).
I'm only really
doing the polling because that's how the BSP driver was set up (and I
think it makes sense to not repeatedly check for teeny-tiny changes all
the time).
I am far from being and expert on the chargers so I can't say if this is
the "de-facto" way of doing things. I just have a feeling this (this
meaning periodic reading from HW and then returning the 'cached' values
to user-space) is quite common (I may be wrong though, many others
including Sebastian and Linus W have _much_ more insight into how
chargers/charger drivers/user-space operate). Caching/polling sounds
like a task that could be implemented in the framework code rather than
in many individual drivers. (This comment does not mean I would expect
You to write such a framework for this driver - as I said, I am just
pondering aloud and waiting to see how others think of this :] )
If there's an existing framework let me know, otherwise I'll
keep my eye out in the future and revise this if I can when it's live.
So no existing framework and please, don't hold your breath waiting for
one ;) It's still great to know that you can be pinged if I manage to
cook-up something.
+
+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) :/
I used the AXP209 battery driver to help me fill in some of the gaps in
my understanding, that driver gets the battery details after
registration (ditto for the ingenic battery driver, which I also looked
at.
Sure. I think the current battery_info API requires a psy-device so
registration needs to be done prior calling it. This was one of the
things I changed while I was twiddling with the simple-gauge RFC series
(which is not in-tree). So this was also not a request to change your
driver but a generic note that it would be beneficial to decouple the
battery-info API from psy-device.
+
+ 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...
No, you're not wrong. The power_supply_get_battery_info does set the
values to -EINVAL, but it also then sets them based on the devicetree
without checking the return values.
You might want to check whether the device-tree APIs update the value if
property is not found - or if some error occurs. AFAIR they do not.
I'm not entirely clear if in the
event of an error it could set it to another value though or null,
so do you think performing a check to see if the value is less than or
equal to 0 would be sufficient?
I'd say this depends on the property. Some properties may have valid
negative values but I guess you know what to expect. In any case, I
assume that the value being non-zero does not guarantee it is valid so
the check should probably be improved.
+
+ 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.
It's confusing as hell (my first battery driver, so the comments
hopefully help everyone else as much as they helped me).
I think the fuel-gauging can be complex topic. Hence comments help a
lot. I do definitely like the comments in charger / fuel gauge drivers.
There was
also a bunch of weird errata like resetting the max charge current
when the plug-in IRQ fires that I felt needed to be documented.
Yes.
Thanks for looking at this, I value your input and will make the
changes once you let me know about the -EINVAL check.
The v9 was caught by my mail-filters :D Probably due to addition of the
autocancel work-queue. It was actually good for me to be pinged by
power-supply patches as I might find some time to continue fuel-gauge
work during the autumn/winter.
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
