All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lee Jones <lee.jones@linaro.org>
To: Markuss Broks <markuss.broks@gmail.com>
Cc: linux-kernel@vger.kernel.org, phone-devel@vger.kernel.org,
	~postmarketos/upstreaming@lists.sr.ht,
	Matti Vaittinen <mazziesaccount@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	MyungJoo Ham <myungjoo.ham@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD
Date: Mon, 18 Jul 2022 09:17:25 +0100	[thread overview]
Message-ID: <YtUXFTx1+vSrXx70@google.com> (raw)
In-Reply-To: <5498bf71-66a5-957e-ed3d-13e68b982562@gmail.com>

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@gmail.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@gmail.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@schaufler-ca.com>
> > >   L:	linux-security-module@vger.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.

[...]

> > > +++ b/include/linux/mfd/sm5703.h
> > > @@ -0,0 +1,105 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +
> > > +#ifndef _SM5703_H
> > > +#define _SM5703_H
> > > +
> > > +struct sm5703_dev {
> > > +	struct device *dev;
> > > +	struct regmap *regmap;
> > > +	struct gpio_desc *reset_gpio;
> > > +};
> > > +
> > > +// Regulator-related defines
> > No C++ style comments.
> > 
> > > +#define SM5703_REG_LDO1				0x1A
> > I'd drop the REG parts from these.
> I have no issues with that, however the already upstreamed sm5703-regulator
> driver uses those defines. If I change the define name, how should I make
> changes in that driver, would it be reasonable to send an additional patch
> together with the new MFD series?

It would.  You could also keep them in for now.

They're not a major blocker.

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

  reply	other threads:[~2022-07-18  8:17 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-23  8:53 [PATCH v5 0/5] Add support for Silicon Mitus SM5703 MFD Markuss Broks
2022-04-23  8:53 ` [PATCH v5 1/5] extcon: sm5502: Clarify SM5703's i2c device ID Markuss Broks
2022-04-26 15:44   ` Chanwoo Choi
2022-04-23  8:53 ` [PATCH v5 2/5] dt-bindings: regulator: Add bindings for Silicon Mitus SM5703 regulators Markuss Broks
2022-04-25 18:17   ` Rob Herring
2022-04-28 16:36   ` Rob Herring
2022-04-28 17:04     ` Markuss Broks
2022-04-28 18:45       ` Rob Herring
2022-04-23  8:53 ` [PATCH v5 3/5] dt-bindings: mfd: Add bindings for Silicon Mitus SM5703 MFD Markuss Broks
2022-04-25 18:17   ` Rob Herring
2022-04-23  8:53 ` [PATCH v5 4/5] mfd: sm5703: Add support for " Markuss Broks
2022-06-14 21:32   ` Lee Jones
2022-07-09 10:56     ` Markuss Broks
2022-07-11  7:56       ` Lee Jones
2022-07-15 16:16     ` Markuss Broks
2022-07-18  8:17       ` Lee Jones [this message]
2022-07-19 13:58         ` Markuss Broks
2022-07-20  8:22           ` Lee Jones
2022-07-20 12:33             ` Markuss Broks
2022-07-20 13:29               ` Lee Jones
2022-07-20 14:12                 ` Markuss Broks
2022-07-20 14:19                   ` Lee Jones
2022-04-23  8:53 ` [PATCH v5 5/5] regulator: sm5703-regulator: Add regulators " Markuss Broks
2022-04-26 15:51 ` (subset) [PATCH v5 0/5] Add support for Silicon Mitus " Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YtUXFTx1+vSrXx70@google.com \
    --to=lee.jones@linaro.org \
    --cc=broonie@kernel.org \
    --cc=cw00.choi@samsung.com \
    --cc=devicetree@vger.kernel.org \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=markuss.broks@gmail.com \
    --cc=mazziesaccount@gmail.com \
    --cc=myungjoo.ham@samsung.com \
    --cc=phone-devel@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=~postmarketos/upstreaming@lists.sr.ht \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.