by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH v2 08/11] mfd: tps65219: Add power-button support

On Tue, 26 Jul 2022, Jerome Neanne wrote:

> From: Markus Schneider-Pargmann <[email protected]>
>
> Using a power-button on the EN/PB/VSENSE pin of TPS65219 is optional, so
> this driver adds the mfd cell for tps65219-pwrbutton only if needed. Two
> interrupts are passed to the driver.

Power button support was already added in the previous patch.

> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> Signed-off-by: Jerome Neanne <[email protected]>
> ---
> drivers/mfd/tps65219.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c
> index 7366f251c21d..e1c30a49bf08 100644
> --- a/drivers/mfd/tps65219.c
> +++ b/drivers/mfd/tps65219.c
> @@ -335,7 +335,10 @@ static int tps65219_probe(struct i2c_client *client,
> struct tps65219 *tps;
> int ret;
> unsigned int chipid;
> + bool pwr_button;
> bool sys_pwr;
> + struct mfd_cell cells[TPS65219_MAX_CELLS];
> + int nr_cells = 0;
>
> tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
> if (!tps)
> @@ -364,9 +367,16 @@ static int tps65219_probe(struct i2c_client *client,
> return ret;
> }
>
> - ret = devm_mfd_add_devices(tps->dev, PLATFORM_DEVID_AUTO, tps65219_cells,
> - ARRAY_SIZE(tps65219_cells), NULL, 0,
> - NULL);
> + memcpy(&cells[nr_cells++], &tps65219_regulator_cell,
> + sizeof(tps65219_regulator_cell));
> + pwr_button = of_property_read_bool(tps->dev->of_node, "power-button");
> + if (pwr_button)
> + memcpy(&cells[nr_cells++], &tps65219_pwrbutton_cell,
> + sizeof(tps65219_pwrbutton_cell));

Gross, no way.

Please call devm_mfd_add_devices() twice.

> + ret = devm_mfd_add_devices(tps->dev, PLATFORM_DEVID_AUTO, cells,
> + nr_cells, NULL, 0,
> + regmap_irq_get_domain(tps->irq_data));
> if (ret) {
> dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> return ret;

--
DEPRECATED: Please use [email protected]

2022-08-25 09:37:28

by Markus Schneider-Pargmann

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] mfd: drivers: Add TI TPS65219 PMIC support

Hi Lee,

thanks for your comments. I am pushing the next version v4 for Jerome
and fixed most of your comments. Some inline comments.

On Mon, Aug 08, 2022 at 04:49:57PM +0100, Lee Jones wrote:
> On Tue, 26 Jul 2022, Jerome Neanne wrote:
>
> > 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.
>
> Really odd line break choices here.
>
> > It contains the following components:
> > - Regulators.
> > - Over Temperature warning and Shut down.
> > - GPIOs
> > - Multi Function Pins (MFP)
> >
> > This patch adds support for tps65219 mfd device. At this time only
>
> No such thing as an MFD device, what is it? PMIC, no?
>
> > the functionalities listed below are made available:
> >
> > - Regulators probe and functionalities
> > - warm and cold reset support
> > - SW shutdown support
> >
> > Signed-off-by: Jerome Neanne <[email protected]>
> > ---
> > MAINTAINERS | 1 +
> > drivers/mfd/Kconfig | 15 +++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/tps65219.c | 214 +++++++++++++++++++++++++++++
> > include/linux/mfd/tps65219.h | 251 +++++++++++++++++++++++++++++++++++
> > 5 files changed, 482 insertions(+)
> > create mode 100644 drivers/mfd/tps65219.c
> > create mode 100644 include/linux/mfd/tps65219.h
> >

...

> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 858cacf659d6..a8ff3d6ea3ab 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile

...

>
> > +//
> > +// Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
> > +//
> > +// This implementation derived from tps65218 authored by
> > +// "J Keerthy <[email protected]>"
>
> Which he probably copied from elsewhere.
>
> Please drop this line.
>
> > +//
>
> Drop this empty comment
>
> Only C++ for the SPDX line please.

Mark Brown wanted this the other way around:

"Please make the entire comment a C++ one so things look more
intentional."
https://lore.kernel.org/all/[email protected]/

Let me know what I should do here.

...

>
> > + if (pm_power_off)
> > + dev_warn(tps->dev, "Setup as system-power-controller but pm_power_off function already registered, overwriting\n");
>
> Is that really what you want?
>
> What about the other driver that thinks it's the
> system-power-controller? Seems wrong.

I am now not overwriting pm_power_off but print an error and probe
successfully.

...

> > diff --git a/include/linux/mfd/tps65219.h b/include/linux/mfd/tps65219.h
> > new file mode 100644
> > index 000000000000..e9197ab8bc75
> > --- /dev/null
> > +++ b/include/linux/mfd/tps65219.h
> > @@ -0,0 +1,251 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * linux/mfd/tps65219.h
>
> No filenames please. They tend to become unsynced.
>
> > + * Functions to access TPS65219 power management chip.
>
> As above.
>
> > + * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
> > + */
> > +
> > +#ifndef __LINUX_MFD_TPS65219_H
> > +#define __LINUX_MFD_TPS65219_H
>
> LINUX?

This seems to be quite common in include/linux/mfd/*.h to have
__LINUX_MFD_ as prefix (probably for the directory they are in). I am
assuming that's not the right style. What should be used instead?
__MFD_TPS65219_H or MFD_TPS65219_H or maybe something else?

...

> > +
> > +/**
> > + * struct tps65219 - tps65219 sub-driver chip access routines
> > + *
> > + * Device data may be used to access the TPS65219 chip
> > + */
>
> This is not a complete kerneldoc header.
>
> Please compile test with W=1.

I didn't see any errors reported for this struct with W=1. I am assuming
you meant missing field descriptions and added them.

Thanks,
Markus