Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Michael Walle <michael@walle.cc>
To: Lee Jones <lee.jones@linaro.org>
Cc: "Andy Shevchenko" <andy.shevchenko@gmail.com>,
	"Ranjani Sridharan" <ranjani.sridharan@linux.intel.com>,
	david.m.ertman@intel.com, shiraz.saleem@intel.com,
	"Rob Herring" <robh+dt@kernel.org>,
	"Mark Brown" <broonie@kernel.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	linux-hwmon@vger.kernel.org, linux-pwm@vger.kernel.org,
	linux-watchdog@vger.kernel.org,
	"linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Bartosz Golaszewski" <bgolaszewski@baylibre.com>,
	"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>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH v4 02/11] mfd: Add support for Kontron sl28cpld management controller
Date: Wed, 10 Jun 2020 09:10:18 +0200
Message-ID: <3a6931248f0efcaf8efbb5425a9bd833@walle.cc> (raw)
In-Reply-To: <20200609194505.GQ4106@dell>

Am 2020-06-09 21:45, schrieb Lee Jones:
> On Tue, 09 Jun 2020, Michael Walle wrote:
> 
>> Am 2020-06-09 17:19, schrieb Lee Jones:
>> > On Tue, 09 Jun 2020, Michael Walle wrote:
>> >
>> > > Am 2020-06-09 08:47, schrieb Lee Jones:
>> > > > On Mon, 08 Jun 2020, Michael Walle wrote:
>> > > >
>> > > > > Am 2020-06-08 20:56, schrieb Lee Jones:
>> > > > > > On Mon, 08 Jun 2020, Michael Walle wrote:
>> > > > > >
>> > > > > > > Am 2020-06-08 12:02, schrieb Andy Shevchenko:
>> > > > > > > > +Cc: some Intel people WRT our internal discussion about similar
>> > > > > > > > problem and solutions.
>> > > > > > > >
>> > > > > > > > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote:
>> > > > > > > > > On Sat, 06 Jun 2020, Michael Walle wrote:
>> > > > > > > > > > Am 2020-06-06 13:46, schrieb Mark Brown:
>> > > > > > > > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote:
>> > > > > > > > > > > > Am 2020-06-05 12:50, schrieb Mark Brown:
>> > > > > > > >
>> > > > > > > > ...
>> > > > > > > >
>> > > > > > > > > Right.  I'm suggesting a means to extrapolate complex shared and
>> > > > > > > > > sometimes intertwined batches of register sets to be consumed by
>> > > > > > > > > multiple (sub-)devices spanning different subsystems.
>> > > > > > > > >
>> > > > > > > > > Actually scrap that.  The most common case I see is a single Regmap
>> > > > > > > > > covering all child-devices.
>> > > > > > > >
>> > > > > > > > Yes, because often we need a synchronization across the entire address
>> > > > > > > > space of the (parent) device in question.
>> > > > > > > >
>> > > > > > > > >  It would be great if there was a way in
>> > > > > > > > > which we could make an assumption that the entire register address
>> > > > > > > > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between
>> > > > > > > > > each of the devices described by its child-nodes.  Probably by picking
>> > > > > > > > > up on the 'simple-mfd' compatible string in the first instance.
>> > > > > > > > >
>> > > > > > > > > Rob, is the above something you would contemplate?
>> > > > > > > > >
>> > > > > > > > > Michael, do your register addresses overlap i.e. are they intermingled
>> > > > > > > > > with one another?  Do multiple child devices need access to the same
>> > > > > > > > > registers i.e. are they shared?
>> > > > > > >
>> > > > > > > No they don't overlap, expect for maybe the version register, which is
>> > > > > > > just there once and not per function block.
>> > > > > >
>> > > > > > Then what's stopping you having each device Regmap their own space?
>> > > > >
>> > > > > Because its just one I2C device, AFAIK thats not possible, right?
>> > > >
>> > > > Not sure what (if any) the restrictions are.
>> > >
>> > > You can only have one device per I2C address. Therefore, I need one
>> > > device
>> > > which is enumerated by the I2C bus, which then enumerates its
>> > > sub-devices.
>> > > I thought this was one of the use cases for MFD. (Regardless of how a
>> > > sub-device access its registers). So even in the "simple-regmap"
>> > > case this
>> > > would need to be an i2c device.
>> 
>> Here (see below)
> 
> Yes, it should still be an I2C device.
> 
>> > >
>> > > E.g.
>> > >
>> > > &i2cbus {
>> > >   mfd-device@10 {
>> > >     compatible = "simple-regmap", "simple-mfd";
>> > >     reg = <10>;
>> > >     regmap,reg-bits = <8>;
>> > >     regmap,val-bits = <8>;
>> > >     sub-device@0 {
>> > >       compatible = "vendor,sub-device0";
>> > >       reg = <0>;
>> > >     };
>> > >     ...
>> > > };
>> > >
>> > > Or if you just want the regmap:
>> > >
>> > > &soc {
>> > >   regmap: regmap@fff0000 {
>> > >     compatible = "simple-regmap";
>> > >     reg = <0xfff0000>;
>> > >     regmap,reg-bits = <16>;
>> > >     regmap,val-bits = <32>;
>> > >   };
>> > >
>> > >   enet-which-needs-syscon-too@1000000 {
>> > >     vendor,ctrl-regmap = <&regmap>;
>> > >   };
>> > > };
>> > >
>> > > Similar to the current syscon (which is MMIO only..).
>> >
>> > We do not need a 'simple-regmap' solution for your use-case.
>> >
>> > Since your device's registers are segregated, just split up the
>> > register map and allocate each sub-device with it's own slice.
>> 
>> I don't get it, could you make a device tree example for my
>> use-case? (see also above)
> 
>     &i2cbus {
>         mfd-device@10 {
>             compatible = "simple-mfd";
>             reg = <10>;
> 
>             sub-device@10 {
>                 compatible = "vendor,sub-device";
>                 reg = <10>;
>             };
>    };
> 
> The Regmap config would be present in each of the child devices.
> 
> Each child device would call devm_regmap_init_i2c() in .probe().

Ah, I see. If I'm not wrong, this still means to create an i2c
device driver with the name "simple-mfd".

Besides that, I don't like this, because:
  - Rob already expressed its concerns with "simple-mfd" and so on.
  - you need to duplicate the config in each sub device
  - which also means you are restricting the sub devices to be
    i2c only (unless you implement and duplicate other regmap configs,
    too). For this driver, SPI and MMIO may be viable options.

Thus, I'd rather implement a simple-mfd.c which implement a common
I2C driver for now and populate its children using
devm_of_platform_populate(). This could be extended to support other
type of regmaps like SPI in the future.

Also some MFD drivers could be moved to this, a likely candidate is
the smsc-ece1099.c. Although I don't really understand its purpose,
if don't have CONFIG_OF.

Judging from the existing code, this simple-mfd.c wouldn't just be
"a list of compatible" strings but also additional quirks and tweaks
for particular devices in this list.

-michael

  reply index

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-04 21:10 [PATCH v4 00/11] Add support for Kontron sl28cpld Michael Walle
2020-06-04 21:10 ` [PATCH v4 01/11] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
2020-06-09 16:28   ` Rob Herring
2020-06-04 21:10 ` [PATCH v4 02/11] mfd: Add support for Kontron sl28cpld management controller Michael Walle
2020-06-05  6:57   ` Lee Jones
2020-06-05  9:51     ` Michael Walle
2020-06-05 10:50     ` Mark Brown
2020-06-05 20:07       ` Michael Walle
2020-06-06 11:46         ` Mark Brown
2020-06-06 12:45           ` Michael Walle
2020-06-08  8:28             ` Lee Jones
2020-06-08 10:02               ` Andy Shevchenko
2020-06-08 15:41                 ` Michael Walle
2020-06-08 18:56                   ` Lee Jones
2020-06-08 21:09                     ` Michael Walle
2020-06-09  6:47                       ` Lee Jones
2020-06-09 14:38                         ` Michael Walle
2020-06-09 14:42                           ` Mark Brown
2020-06-09 15:01                             ` Michael Walle
2020-06-09 17:15                               ` Rob Herring
2020-06-09 17:29                                 ` Mark Brown
2020-06-09 18:41                                 ` Lee Jones
2020-06-09 15:19                           ` Lee Jones
2020-06-09 15:30                             ` Michael Walle
2020-06-09 19:45                               ` Lee Jones
2020-06-10  7:10                                 ` Michael Walle [this message]
2020-06-10  7:19                                   ` Lee Jones
2020-06-10  7:49                                     ` Michael Walle
2020-06-10  7:56                                       ` Lee Jones
2020-06-10  9:27                                         ` Michael Walle
2020-06-10 18:30                                           ` Lee Jones
2020-06-10 17:16                                     ` Rob Herring
2020-06-10 18:02                                       ` Lee Jones
2020-06-08 18:20                 ` Lee Jones
2020-06-09 16:54               ` Rob Herring
2020-06-09 18:52                 ` Lee Jones
2020-06-05  8:01   ` Andy Shevchenko
2020-06-05  8:02     ` Andy Shevchenko
2020-06-05 10:09     ` Michael Walle
2020-06-05 10:48       ` Andy Shevchenko
2020-06-05 11:51         ` Michael Walle
2020-06-04 21:10 ` [PATCH v4 03/11] irqchip: add sl28cpld interrupt controller support Michael Walle
2020-06-05  1:26   ` kernel test robot
2020-06-05  8:07   ` Andy Shevchenko
2020-06-08 15:12   ` kernel test robot
2020-06-04 21:10 ` [PATCH v4 04/11] watchdog: add support for sl28cpld watchdog Michael Walle
2020-06-05  8:14   ` Andy Shevchenko
2020-06-05 10:24     ` Michael Walle
2020-06-05 10:50       ` Andy Shevchenko
2020-06-05 13:52         ` Guenter Roeck
2020-06-05 14:09           ` Andy Shevchenko
2020-06-05 15:05             ` Guenter Roeck
2020-06-05 16:04               ` Andy Shevchenko
2020-06-05 16:34                 ` Guenter Roeck
2020-06-04 21:10 ` [PATCH v4 05/11] pwm: add support for sl28cpld PWM controller Michael Walle
2020-06-05  8:15   ` Andy Shevchenko
2020-06-05  8:49   ` Lee Jones
2020-06-05  9:33     ` Andy Shevchenko
2020-06-05 11:39     ` Michael Walle
2020-06-05 18:17     ` Michael Walle
2020-06-08  7:46       ` Lee Jones
2020-06-04 21:10 ` [PATCH v4 06/11] gpio: add support for the sl28cpld GPIO controller Michael Walle
2020-06-05 12:00   ` Andy Shevchenko
2020-06-05 12:42     ` Michael Walle
2020-06-05 13:15       ` Andy Shevchenko
2020-06-05 18:44         ` Michael Walle
2020-06-05 21:19           ` Andy Shevchenko
2020-06-04 21:10 ` [PATCH v4 07/11] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
2020-06-05 12:06   ` Andy Shevchenko
2020-06-04 21:10 ` [PATCH v4 08/11] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
2020-06-04 21:10 ` [PATCH v4 09/11] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
2020-06-04 21:10 ` [PATCH v4 10/11] arm64: dts: freescale: sl28: enable LED support Michael Walle
2020-06-04 21:10 ` [PATCH v4 11/11] arm64: dts: freescale: sl28: enable fan support Michael Walle
2020-06-05  6:13 ` [PATCH v4 00/11] Add support for Kontron sl28cpld Lee Jones

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=3a6931248f0efcaf8efbb5425a9bd833@walle.cc \
    --to=michael@walle.cc \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=broonie@kernel.org \
    --cc=david.m.ertman@intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jason@lakedaemon.net \
    --cc=jdelvare@suse.com \
    --cc=lee.jones@linaro.org \
    --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=ranjani.sridharan@linux.intel.com \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=shiraz.saleem@intel.com \
    --cc=tglx@linutronix.de \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --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