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 <jneanne@xxxxxxxxxxxx> > > --- > > 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 <j-keerthy@xxxxxx>" > > 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/YtayikFdidxXXubS@xxxxxxxxxxxxx/ 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
