Re: [PATCH v5 4/6] mfd: tps65219: Add driver for TI TPS65219 PMIC

[jerome Neanne]

On 31/10/2022 15:48, Biju Das wrote:
> > Subject: Re: [PATCH v5 4/6] mfd: tps65219: Add driver for TI TPS65219
> > PMIC
> >
> >
> >
> > On 31/10/2022 12:13, Biju Das wrote:
> > > Hi,
> > >
> > > > Subject: [PATCH v5 4/6] mfd: tps65219: Add driver for TI TPS65219
> > > > PMIC
> > > >
> > > > The TPS65219 is a power management IC PMIC designed to supply a
> > wide
> > > > range of SoCs in both portable and stationary applications. Any SoC
> > > > can control
> > > > TPS65219 over a standard I2C interface.
> > > >
> > > > It contains the following components:
> > > > - Regulators.
> > > > - Over Temperature warning and Shut down.
> > > > - GPIOs
> > > > - Multi Function Pins (MFP)
> > > > - power-button
> > > >
> > > > This patch adds support for tps65219 PMIC. At this time only the
> > > > functionalities listed below are made available:
> > > >
> > > > - Regulators probe and functionalities
> > > > - warm and cold reset support
> > > > - SW shutdown support
> > > > - Regulator warnings via IRQs
> > > > - Power-button via IRQ
> > > >
> > > > Signed-off-by: Jerome Neanne <jneanne@xxxxxxxxxxxx>
> > > > Signed-off-by: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>
> > > > ---
> > > >    MAINTAINERS                  |   1 +
> > > >    drivers/mfd/Kconfig          |  14 ++
> > > >    drivers/mfd/Makefile         |   1 +
> > > >    drivers/mfd/tps65219.c       | 320
> > ++++++++++++++++++++++++++++++++
> > > >    include/linux/mfd/tps65219.h | 345
> > +++++++++++++++++++++++++++++++++++
> > > >    5 files changed, 681 insertions(+)
> > > >    create mode 100644 drivers/mfd/tps65219.c  create mode 100644
> > > > include/linux/mfd/tps65219.h
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > > f35b29ffd5fb..960df879c635 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -14926,6 +14926,7 @@ F:	drivers/mfd/menelaus.c
> > > >    F:	drivers/mfd/palmas.c
> > > >    F:	drivers/mfd/tps65217.c
> > > >    F:	drivers/mfd/tps65218.c
> > > > +F:	drivers/mfd/tps65219.c
> > > >    F:	drivers/mfd/tps65910.c
> > > >    F:	drivers/mfd/twl-core.[ch]
> > > >    F:	drivers/mfd/twl4030*.c
> > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > > > abb58ab1a1a4..1a846c7dd0c2 100644
> > > > --- a/drivers/mfd/Kconfig
> > > > +++ b/drivers/mfd/Kconfig
> > > > @@ -1576,6 +1576,20 @@ config MFD_TPS65218
> > > >    	  This driver can also be built as a module.  If so, the module
> > > >    	  will be called tps65218.
> > > >
> > > > +config MFD_TPS65219
> > > > +	tristate "TI TPS65219 Power Management IC"
> > > > +	depends on I2C && OF
> > > > +	select MFD_CORE
> > > > +	select REGMAP_I2C
> > > > +	select REGMAP_IRQ
> > > > +	help
> > > > +	  If you say yes here you get support for the TPS65219 series of
> > > > Power
> > > > +	  Management ICs. These include voltage regulators, GPIOs and
> > > > +	  push/power button that are often used in portable devices.
> > > > +
> > > > +	  This driver can also be built as a module. If so, the module
> > > > +	  will be called tps65219.
> > > > +
> > > >    config MFD_TPS6586X
> > > >    	bool "TI TPS6586x Power Management chips"
> > > >    	depends on I2C=y
> > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > > > 858cacf659d6..a8ff3d6ea3ab 100644
> > > > --- a/drivers/mfd/Makefile
> > > > +++ b/drivers/mfd/Makefile
> > > > @@ -101,6 +101,7 @@ obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> > > >    obj-$(CONFIG_MFD_TPS65086)	+= tps65086.o
> > > >    obj-$(CONFIG_MFD_TPS65217)	+= tps65217.o
> > > >    obj-$(CONFIG_MFD_TPS65218)	+= tps65218.o
> > > > +obj-$(CONFIG_MFD_TPS65219)	+= tps65219.o
> > > >    obj-$(CONFIG_MFD_TPS65910)	+= tps65910.o
> > > >    obj-$(CONFIG_MFD_TPS65912)	+= tps65912-core.o
> > > >    obj-$(CONFIG_MFD_TPS65912_I2C)	+= tps65912-i2c.o
> > > > diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c new
> > file
> > > > mode
> > > > 100644 index 000000000000..c1638483e069
> > > > --- /dev/null
> > > > +++ b/drivers/mfd/tps65219.c
> > > > @@ -0,0 +1,320 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +//
> > > > +// Driver for TPS65219 Integrated Power Management Integrated
> > Chips
> > > > +(PMIC) // // Copyright (C) 2022 BayLibre Incorporated -
> > > > +
> > > > +
> > > > +static int tps65219_restart(struct notifier_block *this,
> > > > +			    unsigned long reboot_mode, void *cmd) {
> > > > +	struct tps65219 *tps;
> > > > +
> > > > +	tps = container_of(this, struct tps65219, nb);
> > > > +	if (!tps) {
> > > > +		pr_err("tps65219: Restarting failed because the pointer to
> > > > tps65219 is invalid\n");
> > > Why not dev_error?
> > Because I can't get correct device then: if !tps, I can't get tps->dev
> > Then can't reference device in dev_error. Do you have a better
> > suggestion than this pr_err?
>
> How container_of can fail?
Good point. This check sounds useless. I'll just remove then...

Thanks for highlighting this.

Jerome.