Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Markuss Broks <markuss.broks@xxxxxxxxx>]

Hi Lee,

On 7/20/22 16:29, Lee Jones wrote:
> On Wed, 20 Jul 2022, Markuss Broks wrote:
>
> > Hi Lee,
> >
> > On 7/20/22 11:22, Lee Jones wrote:
> > > On Tue, 19 Jul 2022, Markuss Broks wrote:
> > >
> > > > Hi Lee,
> > > >
> > > > On 7/18/22 11:17, Lee Jones wrote:
> > > > > On Fri, 15 Jul 2022, Markuss Broks wrote:
> > > > >
> > > > > > Hi Lee,
> > > > > >
> > > > > > Sorry to bother you again, but I've got additional questions while I was
> > > > > > preparing the next version of the series:
> > > > > >
> > > > > > On 6/15/22 00:32, Lee Jones wrote:
> > > > > > > On Sat, 23 Apr 2022, Markuss Broks wrote:
> > > > > > >
> > > > > > > > Silicon Mitus SM5703 is a multi-function device, meant to be
> > > > > > > Please avoid using the term MFD.
> > > > > > >
> > > > > > > How is the device described in the data-sheet?
> > > > > > >
> > > > > > > What do you mean by "meant to be"?
> > > > > > >
> > > > > > > > used in mobile devices. It has several modules: LDO, BUCK regulators,
> > > > > > > Modules or functions?
> > > > > > >
> > > > > > > > charger circuit, flash LED driver, a fuel gauge for monitoring the battery
> > > > > > > > and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
> > > > > > > > they use separate i2c lines to communicate with the device, while charger
> > > > > > > "I2C"
> > > > > > >
> > > > > > > > circuit, LED driver and regulators are on the main i2c line the device is
> > > > > > > > controlled with.
> > > > > > > >
> > > > > > > > Signed-off-by: Markuss Broks <markuss.broks@xxxxxxxxx>
> > > > > > > > ---
> > > > > > > >      MAINTAINERS                |   8 +++
> > > > > > > >      drivers/mfd/Kconfig        |  13 +++++
> > > > > > > >      drivers/mfd/Makefile       |   1 +
> > > > > > > >      drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
> > > > > > > >      include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
> > > > > > > >      5 files changed, 205 insertions(+)
> > > > > > > >      create mode 100644 drivers/mfd/sm5703.c
> > > > > > > >      create mode 100644 include/linux/mfd/sm5703.h
> > > > > > > >
> > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > > > > index 6157e706ed02..6125ed1a3be4 100644
> > > > > > > > --- a/MAINTAINERS
> > > > > > > > +++ b/MAINTAINERS
> > > > > > > > @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
> > > > > > > >      F:	include/linux/srcu*.h
> > > > > > > >      F:	kernel/rcu/srcu*.c
> > > > > > > > +SM5703 MFD DRIVER
> > > > > > > > +M:	Markuss Broks <markuss.broks@xxxxxxxxx>
> > > > > > > > +S:	Maintained
> > > > > > > > +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
> > > > > > > > +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> > > > > > > > +F:	drivers/mfd/sm5703.c
> > > > > > > > +F:	drivers/regulator/sm5703-regulator.c
> > > > > > > > +
> > > > > > > >      SMACK SECURITY MODULE
> > > > > > > >      M:	Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> > > > > > > >      L:	linux-security-module@xxxxxxxxxxxxxxx
> > > > > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > > > > > index 3b59456f5545..c13a99ceae99 100644
> > > > > > > > --- a/drivers/mfd/Kconfig
> > > > > > > > +++ b/drivers/mfd/Kconfig
> > > > > > > > @@ -1237,6 +1237,19 @@ config MFD_SM501
> > > > > > > >      	  interface. The device may be connected by PCI or local bus with
> > > > > > > >      	  varying functions enabled.
> > > > > > > > +config MFD_SM5703
> > > > > > > > +	tristate "Silicon Mitus SM5703 MFD"
> > > > > > > > +	depends on I2C
> > > > > > > > +	depends on OF
> > > > > > > > +	select MFD_CORE
> > > > > > > > +	select REGMAP_I2C
> > > > > > > > +	help
> > > > > > > > +	  This is the core driver for the Silicon Mitus SM5703 multi-function
> > > > > > > Please drop the MFD term, as above.
> > > > > > >
> > > > > > > > +	  device. This device is meant to be used in phones and it has numerous
> > > > > > > "meant to be"?
> > > > > > >
> > > > > > > > +	  modules, including LED controller, regulators, fuel gauge, MUIC and
> > > > > > > Either "an LED controller" or "LED controllers"
> > > > > > >
> > > > > > > Same with "charger circuit" below.
> > > > > > >
> > > > > > > > +	  charger circuit. It also support muxing a serial interface over USB
> > > > > > > "supports"
> > > > > > >
> > > > > > > What kind of serial?
> > > > > > >
> > > > > > > > +	  data lines.
> > > > > > > > +
> > > > > > > >      config MFD_SM501_GPIO
> > > > > > > >      	bool "Export GPIO via GPIO layer"
> > > > > > > >      	depends on MFD_SM501 && GPIOLIB
> > > > > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > > > > > > index 858cacf659d6..ca8b86736a36 100644
> > > > > > > > --- a/drivers/mfd/Makefile
> > > > > > > > +++ b/drivers/mfd/Makefile
> > > > > > > > @@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
> > > > > > > >      rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
> > > > > > > >      obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
> > > > > > > >      obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
> > > > > > > > +obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
> > > > > > > > diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..7f9838149051
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/mfd/sm5703.c
> > > > > > > > @@ -0,0 +1,78 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > > > +
> > > > > > > > +#include <linux/err.h>
> > > > > > > > +#include <linux/delay.h>
> > > > > > > > +#include <linux/gpio/consumer.h>
> > > > > > > > +#include <linux/i2c.h>
> > > > > > > > +#include <linux/mfd/core.h>
> > > > > > > > +#include <linux/mfd/sm5703.h>
> > > > > > > > +#include <linux/module.h>
> > > > > > > > +#include <linux/of_device.h>
> > > > > > > > +#include <linux/regmap.h>
> > > > > > > > +
> > > > > > > > +static const struct mfd_cell sm5703_devs[] = {
> > > > > > > > +	{ .name = "sm5703-regulator", },
> > > > > > > > +};
> > > > > > > Where are the rest of the child drivers?
> > > > > > Should those devices still be present even though there's no driver for them
> > > > > > (yet) ? I have a WIP version of driver for almost every function, but I
> > > > > > currently lack time to get them done.
> > > > > Without them the driver-set is useless, no?
> > > > >
> > > > > We try to refrain from applying dead code.
> > > > >
> > > > > A lot of it has a tendency to stay that way.
> > > > Well, in my opinion, having just the regulator driver is already useful
> > > > enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for
> > > > powering the touchscreen. Touchscreen is kind of vital functionality for a
> > > > phone so I decided to upstream parts that are necessary for it first and
> > > > finish up other stuff later. It's not the only board that uses SM5703's
> > > > regulators for supplying all different kinds of hardware, either.
> > > Upstreaming functionality which is useful on its own is fine, but that
> > > doesn't tick all of the boxes to justify an MFD.  This is a lot of
> > > code which is hard to justify if it only registers a Regulator driver.
> > Do you think I should hold on this series until I have other things done?
> > Alternatively, I could make the regulator driver standalone, dedicated, but
> > then when I'd add other functionality I'd have to redo it and add the MFD
> > driver back, that I believe would be quite annoying from maintainers' and
> > sanity perspective. The other functions left on the main control I2C are
> > also not really "vital" to device's functionality (Flash LED and charger),
> > so the regulator function makes the most sense to be available first, which
> > was my motivation behind upstreaming that first.
> What's the reason for this being an MFD in the first place?
Well, the "main" I2C interface is shared between three functions: 
regulators, Flash LED and charger. I call this one the "main" one 
because the device is controlled with it: you can select internal clock 
rate, enable or disable the PMIC and do other things that more of apply 
to the whole PMIC, not just the separate functions (as is the case with 
fuel gauge and MUIC I2C) . That's the reason for this being an MFD: 
those three functions share one I2C interface. While they might not be 
implemented at the moment, this places a foundation for adding future 
support.
- Markuss