From: Stephen Boyd <sboyd@kernel.org>
To: "Conor Dooley" <conor+dt@kernel.org>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Lee Jones" <lee@kernel.org>,
"Linus Walleij" <linus.walleij@linaro.org>,
"Michael Turquette" <mturquette@baylibre.com>,
"Philipp Zabel" <p.zabel@pengutronix.de>,
"Rafael J. Wysocki" <rafael@kernel.org>,
"Rob Herring" <robh@kernel.org>,
"Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
"Théo Lebrun" <theo.lebrun@bootlin.com>
Cc: linux-mips@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
linux-gpio@vger.kernel.org,
Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>,
Gregory CLEMENT <gregory.clement@bootlin.com>,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
Tawfik Bayouk <tawfik.bayouk@mobileye.com>
Subject: Re: [PATCH v2 00/11] Add Mobileye EyeQ system controller support (clk, reset, pinctrl)
Date: Tue, 07 May 2024 14:48:34 -0700 [thread overview]
Message-ID: <62e1512be0bc44acae9afb34467753db.sboyd@kernel.org> (raw)
In-Reply-To: <D13HXGJGMS76.XIIIZLZBCZ09@bootlin.com>
Quoting Théo Lebrun (2024-05-07 07:52:49)
> On Sat May 4, 2024 at 4:34 AM CEST, Stephen Boyd wrote:
> >
> > Why can't you use auxiliary device and driver APIs?
>
> Good question. Reasons I see:
>
> - I didn't know about auxdev beforehand. I discussed the rework with a
> few colleagues and none mentioned it either.
>
> - It feels simpler to let each device access iomem resources. From my
> understanding, an auxdev is supposed to make function calls to its
> parent without inheriting iomem access. That sounds like it will put
> the register logic/knowledge inside a single driver, which could or
> could not be a better option.
You can pass the iomem pointer to the child device, either through the
struct device platform_data void pointer or you can make a wrapper
struct for struct auxiliary_device that the child device/driver, e.g.
pinctrl, would know about. Or you can use a regmap and pass that through
to the function that creates the auxiliary device.
Either way, we don't want the iomem register logic inside a single
driver. Conor recently made that change for mpfs. See this patch[1].
The syscon code uses a regmap so that register access uses a spinlock.
Maybe you need that, or maybe you don't. I don't know. It depends on if
the device has logical drivers that access some shared register. If that
doesn't happen then letting the logical drivers map and access the
registers with iomem accessors is fine. Otherwise, you want some sort of
mediator function, where regmap helps make that easy to provide.
>
> Implementing a function like this feels like cheating:
> int olb_read(struct device *dev, u32 offset, u32 *val);
>
> With an MFD, we hand over a part of the iomem resource to each child
> and they deal with it however they like.
>
> - Syscon is what I picked to share parts of OLB to other devices that
> need it. Currently that is only for I2C speed mode but other devices
> have wrapping-related registers. MFD and syscon are deeply connected
> so an MFD felt natural.
>
> - That would require picking one device that is platform driver, the
> rest being all aux devices. Clock driver appears to be the one, same
> as two existing mpfs and starfive-jh7110 that use auxdev for clk and
> reset.
>
> Main reason I see for picking auxdev is that it forces devices to
> interact with a defined internal API. That can lead to nicer
> abstractions rather than inheriting resources as is being done in MFD.
>
The simple-mfd binding encourages sub-nodes for drivers. This is an
anti-pattern because we want nodes for devices, not drivers. We should
discourage the use of that compatible in my opinion.
I could see the MFD subsystem gaining support for creating child
auxiliary devices for some compatible string node, and passing those
devices a regmap. Maybe that would be preferable to having to pick a
driver subsystem to put the platform driver in. Outside of making a
general purpose framework, you could put the platform driver in
drivers/mfd and have that populate the child devices like clk, reset,
pinctrl, etc.
The overall goal is still the same. Don't make child nodes.
[1] https://lore.kernel.org/linux-clk/20240424-strangle-sharpener-34755c5e6e3e@spud/
prev parent reply other threads:[~2024-05-07 21:48 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-03 14:20 [PATCH v2 00/11] Add Mobileye EyeQ system controller support (clk, reset, pinctrl) Théo Lebrun
2024-05-03 14:20 ` [PATCH v2 01/11] dt-bindings: clock: mobileye,eyeq5-clk: drop bindings Théo Lebrun
2024-05-03 15:57 ` Krzysztof Kozlowski
2024-05-03 16:05 ` Krzysztof Kozlowski
2024-05-07 15:07 ` Théo Lebrun
2024-05-07 15:34 ` Krzysztof Kozlowski
2024-05-03 14:20 ` [PATCH v2 02/11] dt-bindings: clock: mobileye,eyeq5-reset: " Théo Lebrun
2024-05-03 15:35 ` Rob Herring (Arm)
2024-05-03 14:20 ` [PATCH v2 03/11] dt-bindings: soc: mobileye: add EyeQ OLB system controller Théo Lebrun
2024-05-03 15:35 ` Rob Herring (Arm)
2024-05-07 12:51 ` Rob Herring
2024-05-03 14:20 ` [PATCH v2 04/11] driver core: platform: Introduce platform_device_add_with_name() Théo Lebrun
2024-05-03 14:20 ` [PATCH v2 05/11] mfd: Add cell device name Théo Lebrun
2024-05-03 14:20 ` [PATCH v2 06/11] mfd: olb: Add support for Mobileye OLB system-controller Théo Lebrun
2024-05-03 14:20 ` [PATCH v2 07/11] clk: divider: Introduce CLK_DIVIDER_EVEN_INTEGERS flag Théo Lebrun
2024-05-03 14:20 ` [PATCH v2 08/11] clk: eyeq: add driver Théo Lebrun
2024-05-03 14:20 ` [PATCH v2 09/11] reset: eyeq: add platform driver Théo Lebrun
2024-05-03 14:20 ` [PATCH v2 10/11] pinctrl: eyeq5: " Théo Lebrun
2024-05-03 14:20 ` [PATCH v2 11/11] MIPS: mobileye: eyeq5: add OLB system-controller node Théo Lebrun
2024-05-04 2:34 ` [PATCH v2 00/11] Add Mobileye EyeQ system controller support (clk, reset, pinctrl) Stephen Boyd
2024-05-07 14:52 ` Théo Lebrun
2024-05-07 15:14 ` Théo Lebrun
2024-05-07 21:48 ` Stephen Boyd [this message]
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=62e1512be0bc44acae9afb34467753db.sboyd@kernel.org \
--to=sboyd@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=gregory.clement@bootlin.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lee@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=p.zabel@pengutronix.de \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
--cc=tawfik.bayouk@mobileye.com \
--cc=theo.lebrun@bootlin.com \
--cc=thomas.petazzoni@bootlin.com \
--cc=tsbogend@alpha.franken.de \
--cc=vladimir.kondratiev@mobileye.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).