Re: [PATCH v2 2/2] power: supply: Add bd718(15/28/78) charger driver

[Andreas Kemnade <andreas@kemnade.info>]

Am Thu, 21 Aug 2025 09:38:12 +0300
schrieb Matti Vaittinen <mazziesaccount@gmail.com>:

> On 20/08/2025 23:21, Andreas Kemnade wrote:
> > Add charger driver for ROHM BD718(15/28/78) PMIC charger block.
> > It is a stripped down version of the driver here:
> > https://lore.kernel.org/lkml/dbd97c1b0d715aa35a8b4d79741e433d97c562aa.1637061794.git.matti.vaittinen@fi.rohmeurope.com/
> > 
> > For the ease of review and to do a step-by-step approach remove all the
> > coloumb counter related stuff and do not sneak in BD71827 support. That
> > also avoids non-trivial rebasing of the above series.
> > 
> > Changes besides that:
> > Replace the custom property by a standard one and do not use megaohms
> > for the current sense resistor.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>  
> 
> Thanks for doing this Andreas :) Just a question about maintenance and 
> one comment. After those are handled:
> 
> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> I also added couple of styling 'nits', which you can address if you agree.
> 
> > ---
> >   drivers/power/supply/Kconfig         |    9 +
> >   drivers/power/supply/Makefile        |    1 +
> >   drivers/power/supply/bd71828-power.c | 1135 ++++++++++++++++++++++++++++++++++
> >   3 files changed, 1145 insertions(+)
> > 
> > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> > index 79ddb006e2dad6bf96b71ed570a37c006b5f9433..16dddb2355d9b89ccf0ad453fb77c189cf36f00f 100644
> > --- a/drivers/power/supply/Kconfig
> > +++ b/drivers/power/supply/Kconfig
> > @@ -974,6 +974,15 @@ config CHARGER_UCS1002
> >   	  Say Y to enable support for Microchip UCS1002 Programmable
> >   	  USB Port Power Controller with Charger Emulation.
> >   
> > +config CHARGER_BD71828
> > +	tristate "Power-supply driver for ROHM BD71828 and BD71815 PMIC"
> > +	depends on MFD_ROHM_BD71828
> > +	help
> > +	  Say Y here to enable support for charger and battery
> > +	  in ROHM BD71815, BD71817, ROHM BD71828 power management
> > +	  ICs. This driver gets various bits of information about battery
> > +	  and charger states.
> > +
> >   config CHARGER_BD99954
> >   	tristate "ROHM bd99954 charger driver"
> >   	depends on I2C
> > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> > index f943c9150b326d41ff241f82610f70298635eb08..c6520a11f021c872f01250ae54eb4c63018cd428 100644
> > --- a/drivers/power/supply/Makefile
> > +++ b/drivers/power/supply/Makefile
> > @@ -113,6 +113,7 @@ obj-$(CONFIG_CHARGER_SC2731)	+= sc2731_charger.o
> >   obj-$(CONFIG_FUEL_GAUGE_SC27XX)	+= sc27xx_fuel_gauge.o
> >   obj-$(CONFIG_FUEL_GAUGE_STC3117)       += stc3117_fuel_gauge.o
> >   obj-$(CONFIG_CHARGER_UCS1002)	+= ucs1002_power.o
> > +obj-$(CONFIG_CHARGER_BD71828)	+= bd71828-power.o
> >   obj-$(CONFIG_CHARGER_BD99954)	+= bd99954-charger.o
> >   obj-$(CONFIG_CHARGER_WILCO)	+= wilco-charger.o
> >   obj-$(CONFIG_RN5T618_POWER)	+= rn5t618_power.o
> > diff --git a/drivers/power/supply/bd71828-power.c b/drivers/power/supply/bd71828-power.c
> > new file mode 100644  
> 
> Do you think you could agree to maintain this? I would like to also be 
> listed either as M: or R:
> 
> So please, add one more patch bringing in the maintainer info.
> 
ok, I will do.

> > index 0000000000000000000000000000000000000000..f074d7159de1cb832bc0fb5ab494cddeb96a8968
> > --- /dev/null
> > +++ b/drivers/power/supply/bd71828-power.c
> > @@ -0,0 +1,1135 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * bd71828-power.c
> > + * @file ROHM BD71815, BD71828 and BD71878 Charger driver
> > + *
> > + * Copyright 2021.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/rohm-bd71815.h>
> > +#include <linux/mfd/rohm-bd71828.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/power_supply.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +
> > +/* common defines */
> > +#define BD7182x_MASK_VBAT_U			0x1f
> > +#define BD7182x_MASK_VDCIN_U			0x0f
> > +#define BD7182x_MASK_IBAT_U			0x3f
> > +#define BD7182x_MASK_CURDIR_DISCHG		0x80
> > +#define BD7182x_MASK_CC_CCNTD_HI		0x0FFF
> > +#define BD7182x_MASK_CC_CCNTD			0x0FFFFFFF  
> 
> nit: I would still drop the unused defines. They make this look more 
> complex than it is and make also reviewing harder. (Same as with the 
> unused struct members - I think the defines should be introduced in the 
> patch which also uses them. (Well, register address definitions are an 
> exception - I'm not against defining all register addresses in initial 
> commit. Especially so when we have an MFD device, where register 
> addresses tend to live in different subsystem (MFD) tree.
> 
ok, I will take the brush. Time for a power-nap on the 'd' key...

Regards,
Andreas