Re: [PATCH v8 09/10] power: supply: Support ROHM bd99954 charger

From: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
To: "sebastian.reichel@collabora.com" <sebastian.reichel@collabora.com>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
	"andrei.stefanescu@microchip.com"
	<andrei.stefanescu@microchip.com>,
	"heiko@sntech.de" <heiko@sntech.de>,
	"rafael@kernel.org" <rafael@kernel.org>,
	"tony@atomide.com" <tony@atomide.com>,
	"linus.walleij@linaro.org" <linus.walleij@linaro.org>,
	"brendanhiggins@google.com" <brendanhiggins@google.com>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"kholk11@gmail.com" <kholk11@gmail.com>,
	"hsin-hsiung.wang@mediatek.com" <hsin-hsiung.wang@mediatek.com>,
	"linux-samsung-soc@vger.kernel.org"
	<linux-samsung-soc@vger.kernel.org>,
	"axel.lin@ingics.com" <axel.lin@ingics.com>,
	"gregory.clement@bootlin.com" <gregory.clement@bootlin.com>,
	"b.zolnierkie@samsung.com" <b.zolnierkie@samsung.com>,
	"krzk@kernel.org" <krzk@kernel.org>,
	"m.reichl@fivetechno.de" <m.reichl@fivetechno.de>,
	"bgolaszewski@baylibre.com" <bgolaszewski@baylibre.com>,
	"wens@csie.org" <wens@csie.org>,
	"andreas@kemnade.info" <andreas@kemnade.info>,
	"agross@kernel.org" <agross@kernel.org>,
	"Laine, Markus" <Markus.Laine@fi.rohmeurope.com>,
	"Adam.Thomson.Opensource@diasemi.com"
	<Adam.Thomson.Opensource@diasemi.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"ckeepax@opensource.cirrus.com" <ckeepax@opensource.cirrus.com>,
	"linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
	"rf@opensource.cirrus.com" <rf@opensource.cirrus.com>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"linux-mediatek@lists.infradead.org"
	<linux-mediatek@lists.infradead.org>,
	"matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"andriy.shevchenko@linux.intel.com"
	<andriy.shevchenko@linux.intel.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"support.opensource@diasemi.com" <support.opensource@diasemi.com>,
	"baolin.wang@linaro.org" <baolin.wang@linaro.org>,
	"sbkim73@samsung.com" <sbkim73@samsung.com>,
	"patches@opensource.cirrus.com" <patches@opensource.cirrus.com>,
	"rdunlap@infradead.org" <rdunlap@infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Mutanen, Mikko" <Mikko.Mutanen@fi.rohmeurope.com>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"mazziesaccount@gmail.com" <mazziesaccount@gmail.com>
Subject: Re: [PATCH v8 09/10] power: supply: Support ROHM bd99954 charger
Date: Mon, 6 Apr 2020 09:59:04 +0000	[thread overview]
Message-ID: <334ae92d62a32675bc18f331c9981e169e33e5e0.camel@fi.rohmeurope.com> (raw)
In-Reply-To: <20200405032233.btlwo5wvzmrrw65k@earth.universe>

On Sun, 2020-04-05 at 05:22 +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Apr 03, 2020 at 11:48:05AM +0300, Matti Vaittinen wrote:
> > The ROHM BD99954 is a Battery Management LSI for 1-4 cell Lithium-
> > Ion
> > secondary battery intended to be used in space-constraint equipment
> > such
> > as Low profile Notebook PC, Tablets and other applications. BD99954
> > provides a Dual-source Battery Charger, two port BC1.2 detection
> > and a
> > Battery Monitor.
> > 
> > Support ROHM BD99954 Charger IC.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > 
> > No changes since v7
> 
> Thanks, the drivers looks mostly good to me, I found some minor
> things. Please fix before sending the next (and hopefully final)
> revision.

I will :) Thanks for all the effort when doing the review. Much
appreciated! And I'll rebase and send the next version after v5.7-rc1
gets tagged. I'll fix these at that version.

> 
> >  drivers/power/supply/Kconfig           |    9 +
> >  drivers/power/supply/Makefile          |    1 +
> >  drivers/power/supply/bd99954-charger.c | 1149
> > ++++++++++++++++++++++++
> >  include/linux/power/bd99954-charger.h  | 1075
> > ++++++++++++++++++++++
> 
> Please move this header to drivers/power/supply/, the public
> include directory is only needed for platform_data and MFD.
> 

Makes sense.

> > +	dev_info(bd->dev, "Found DB99954 chip rev %d\n", bd->chip_rev);
> 
> DB -> BD?

Right. Well spotted :)

> > +
> > +static int bd9995x_remove(struct i2c_client *client)
> > +{
> > +	int ret;
> > +	struct bd9995x_device *bd = i2c_get_clientdata(client);
> > +
> > +	disable_irq(client->irq);
> > +
> > +	/*
> > +	 * Reset all registers to default values. This should also
> > disable
> > +	 * CHG_EN bit. If this is not required we can get rid of the
> > remove.
> > +	 */
> > +	ret = bd9995x_chip_reset(bd);
> 
> You can register this using devm_add_action_or_reset() before the
> devm_request_threaded_irq() call in probe and get rid of the remove
> function :)

Yep! That was a good point! I think I had never used
devm_add_action_or_reset() - this is a handy thing for my toolbox! :)

Best Regards
	Matti Vaittinen
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek