[PATCH v8 3/4] power: supply: Add charger driver for Rockchip RK817

[Chris Morgan]

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 at hotmail.com>
> > 
> > Add support for the Rockchip rk817 battery charger integrated into the
> > rk817 PMIC.
> > 
> > Signed-off-by: Chris Morgan <macromorgan at hotmail.com>
> > Signed-off-by: Maya Matuszczyk <maccraft123mc at gmail.com>
> > ---
> 
> 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 at gmail.com> since
while it's heavily modified it's based on her original work.

> 
> > + *
> > + * Authors: Maya Matuszczyk <maccraft123mc at gmail.com>
> > + *	    Chris Morgan <macromorgan at hotmail.com>
> > + */
> > +
> > +#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