On Tue, Aug 23, 2022 at 06:35:40PM +0200, Sebastian Reichel wrote: > Hi, > > On Mon, Aug 08, 2022 at 12:38:08PM -0500, 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> > > --- > > Looks mostly good. Just three things: > > > [...] > > --- /dev/null > > +++ b/drivers/power/supply/rk817_charger.c > > @@ -0,0 +1,1151 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Charger Driver for Rockchip rk817 > > + * > > + * Copyright (c) 2021 > > The Copyright line is incomplete. Thank you, I'll update it to say Copyright (c) 2021 Maya Matuszczyk <maccraft123mc@xxxxxxxxx> since while it's heavily modified it's based on her original work. > > > + * > > + * Authors: Maya Matuszczyk <maccraft123mc@xxxxxxxxx> > > + * Chris Morgan <macromorgan@xxxxxxxxxxx> > > + */ > > + > > +#include <linux/mfd/rk808.h> > > +#include <linux/irq.h> > > +#include <linux/of_gpio.h> > > [...] > > Why are you including of_gpio.h? You are not using any. Thanks, sorry. I'll change it to just linux/of.h since I am using a few things from that. Might have been from a previous version where I had used GPIOs instead of PMIC registers to detect charger status. > > > + INIT_DELAYED_WORK(&charger->work, rk817_charging_monitor); > > + /* Get and populate the first set of values. */ > > + schedule_delayed_work(&charger->work, 0); > > [...] > > what happens with the delayed work when you remove the driver? > Check for devm_delayed_work_autocancel(). I'll test using that and submit again once I've confirmed it works. Thank you for all your feedback. > > > -- Sebastian
