Message-ID: <YtgDSelfobu/a/be@google.com>
Date:   Wed, 20 Jul 2022 14:29:45 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Markuss Broks <markuss.broks@...il.com>
Cc:     linux-kernel@...r.kernel.org, phone-devel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht,
        Matti Vaittinen <mazziesaccount@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

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@...il.com>
> > > > > > > ---
> > > > > > >     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@...il.com>
> > > > > > > +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@...aufler-ca.com>
> > > > > > >     L:	linux-security-module@...r.kernel.org
> > > > > > > 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?

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists