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? 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). 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.
>
> > +
> > +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.
>
> > +
> > + 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. 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?
>
> > +
> > + 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). 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.
Thanks for looking at this, I value your input and will make the
changes once you let me know about the -EINVAL check.
>
> 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! ~~
