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
next prev parent 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).