Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Lee Jones <lee.jones@linaro.org>
To: Michael Walle <michael@walle.cc>
Cc: linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
	linux-pwm@vger.kernel.org, linux-watchdog@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Bartosz Golaszewski" <bgolaszewski@baylibre.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Jean Delvare" <jdelvare@suse.com>,
	"Guenter Roeck" <linux@roeck-us.net>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Wim Van Sebroeck" <wim@linux-watchdog.org>,
	"Shawn Guo" <shawnguo@kernel.org>, "Li Yang" <leoyang.li@nxp.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Jason Cooper" <jason@lakedaemon.net>,
	"Marc Zyngier" <maz@kernel.org>,
	"Mark Brown" <broonie@kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Will Deacon" <will@kernel.org>, "Pavel Machek" <pavel@ucw.cz>
Subject: Re: [PATCH v6 01/13] mfd: add simple regmap based I2C driver
Date: Tue, 28 Jul 2020 09:15:06 +0100
Message-ID: <20200728081506.GA2419169@dell> (raw)
In-Reply-To: <23a9ecf5fe4f15b9b20a91cc292aca80@walle.cc>

On Tue, 28 Jul 2020, Michael Walle wrote:

> Am 2020-07-28 09:19, schrieb Lee Jones:
> > On Sun, 26 Jul 2020, Michael Walle wrote:
> > 
> > > There are I2C devices which contain several different functions but
> > > doesn't require any special access functions. For these kind of
> > > drivers
> > > an I2C regmap should be enough.
> > > 
> > > Create an I2C driver which creates an I2C regmap and enumerates its
> > > children. If a device wants to use this as its MFD core driver, it has
> > > to add an individual compatible string. It may provide its own regmap
> > > configuration.
> > > 
> > > Subdevices can use dev_get_regmap() on the parent to get their regmap
> > > instance.
> > > 
> > > Signed-off-by: Michael Walle <michael@walle.cc>
> > > ---
> > > Changes since v5:
> > >  - removed "select MFD_CORE" in Kconfig
> > >  - removed help text in Kconfig, we assume that the users of this
> > 
> > That's the opposite of what I asked for.
> 
> What is the use to describe the symbol, if it is not user selectable?
> I'm not aware this is done anywhere in the kernel, am I missing
> something?

You mean in menuconfig?

I find 'help's helpful even outside of menuconfig.

Surely I'm not the only one who reads them 'raw'?

> > >    driver will have a "select MFD_SIMPLE_MFD_I2C". Instead added
> > >    a small description to the driver itself.
> > >  - removed "struct simple_mfd_i2c_config" and use regmap_config
> > >    directly
> > >  - changed builtin_i2c_driver() to module_i2c_driver(), added
> > >    MODULE_ boilerplate
> > >  - cleaned up the included files
> > > 
> > > Changes since v4:
> > >  - new patch. Lee, please bear with me. I didn't want to delay the
> > >    new version (where a lot of remarks on the other patches were
> > >    addressed) even more, just because we haven't figured out how
> > >    to deal with the MFD part. So for now, I've included this one.
> > > 
> > >  drivers/mfd/Kconfig          |  5 ++++
> > >  drivers/mfd/Makefile         |  1 +
> > >  drivers/mfd/simple-mfd-i2c.c | 55
> > > ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 61 insertions(+)
> > >  create mode 100644 drivers/mfd/simple-mfd-i2c.c
> > > 
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 33df0837ab41..c08539c7a166 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1162,6 +1162,11 @@ config MFD_SI476X_CORE
> > >  	  To compile this driver as a module, choose M here: the
> > >  	  module will be called si476x-core.
> > > 
> > > +config MFD_SIMPLE_MFD_I2C
> > > +	tristate
> > > +	depends on I2C
> > > +	select REGMAP_I2C
> > 
> > Please provide a full help.
> 
> See above.
> 
> > 
> > >  config MFD_SM501
> > >  	tristate "Silicon Motion SM501"
> > >  	depends on HAS_DMA
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index a60e5f835283..78d24a3e7c9e 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -264,3 +264,4 @@ obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
> > >  obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
> > > 
> > >  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
> > > +obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
> > > diff --git a/drivers/mfd/simple-mfd-i2c.c
> > > b/drivers/mfd/simple-mfd-i2c.c
> > > new file mode 100644
> > > index 000000000000..45090ddad104
> > > --- /dev/null
> > > +++ b/drivers/mfd/simple-mfd-i2c.c
> > > @@ -0,0 +1,55 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * A very simple I2C MFD driver
> > 
> > Simple MFD - I2C
> 
> ok.
> 
> > > + * The driver enumerates its children and registers a common
> > > regmap. Use
> > > + * dev_get_regmap(pdev->dev.parent, NULL) in the child nodes to
> > > fetch that
> > > + * regmap instance.
> > 
> > This driver creates a single register map with the intention for it to
> > be shared by all sub-devices.  Children can use their parent's device
> > structure (dev.parent) in order reference it.
> 
> Should this be appended or should it replace my paragraph? If its the
> latter,
> the "enumeration of the children" is missing.

If you want to keep that part, try:

This driver creates a single register map with the intention for it to
be shared by all sub-devices.  Children can use their parent's device
structure (dev.parent) in order reference it.

Once the register map has been successfully initialised, any
sub-devices represented by child nodes in Device Tree will be
subsequently registered.

> > > + * In the future this driver might be extended to support also
> > > other interfaces
> > > + * like SPI etc.
> > 
> > Remove this please.
> 
> Why would you remove information about the intention of this driver? If
> someone
> looks for a place to implement its SPI/I3C/Slimbus MFD driver this might
> come
> in handy.

By all means put something similar in the commit log, but it has no
place in the driver itself.  Besides, if we were to add support for
SPI, it is likely to be a completely separate/unrelated driver.

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

  reply index

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-25 23:18 [PATCH v6 00/13] Add support for Kontron sl28cpld Michael Walle
2020-07-25 23:18 ` [PATCH v6 01/13] mfd: add simple regmap based I2C driver Michael Walle
2020-07-28  7:19   ` Lee Jones
2020-07-28  7:42     ` Michael Walle
2020-07-28  8:15       ` Lee Jones [this message]
2020-07-28  8:27         ` Michael Walle
2020-07-28  8:35           ` Lee Jones
2020-07-25 23:18 ` [PATCH v6 02/13] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
2020-07-28  7:24   ` Lee Jones
2020-07-28  7:57     ` Michael Walle
2020-07-28  8:27       ` Lee Jones
2020-07-28  8:38         ` Michael Walle
2020-07-28  8:56           ` Lee Jones
2020-07-28  9:06             ` Michael Walle
2020-07-28  9:20               ` Lee Jones
2020-07-28  9:39                 ` Michael Walle
2020-07-25 23:18 ` [PATCH v6 03/13] mfd: simple-mfd-i2c: add sl28cpld support Michael Walle
2020-07-28  7:25   ` Lee Jones
2020-07-25 23:18 ` [PATCH v6 04/13] irqchip: add sl28cpld interrupt controller support Michael Walle
2020-07-25 23:18 ` [PATCH v6 05/13] watchdog: add support for sl28cpld watchdog Michael Walle
2020-07-25 23:18 ` [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller Michael Walle
2020-07-27  7:30   ` Thierry Reding
2020-07-28  7:43   ` Uwe Kleine-König
2020-07-28  8:21     ` Michael Walle
2020-07-28  9:47       ` Uwe Kleine-König
2020-07-28 10:51         ` Lee Jones
2020-07-25 23:18 ` [PATCH v6 07/13] gpio: add support for the sl28cpld GPIO controller Michael Walle
2020-07-26  9:16   ` Andy Shevchenko
2020-07-25 23:18 ` [PATCH v6 08/13] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
2020-07-26  9:18   ` Andy Shevchenko
2020-07-25 23:18 ` [PATCH v6 09/13] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
2020-07-25 23:18 ` [PATCH v6 10/13] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
2020-07-25 23:18 ` [PATCH v6 11/13] arm64: dts: freescale: sl28: enable LED support Michael Walle
2020-07-25 23:18 ` [PATCH v6 12/13] arm64: dts: freescale: sl28: enable fan support Michael Walle
2020-07-25 23:18 ` [PATCH v6 13/13] arm64: defconfig: enable the sl28cpld board management controller Michael Walle

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=20200728081506.GA2419169@dell \
    --to=lee.jones@linaro.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jason@lakedaemon.net \
    --cc=jdelvare@suse.com \
    --cc=leoyang.li@nxp.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=maz@kernel.org \
    --cc=michael@walle.cc \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=will@kernel.org \
    --cc=wim@linux-watchdog.org \
    /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

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git