Date: Tue, 07 May 2024 14:48:34 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Conor Dooley <conor+dt@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Lee Jones <lee@...nel.org>, Linus Walleij <linus.walleij@...aro.org>, Michael Turquette <mturquette@...libre.com>, Philipp Zabel <p.zabel@...gutronix.de>, Rafael J. Wysocki <rafael@...nel.org>, Rob Herring <robh@...nel.org>, Thomas Bogendoerfer <tsbogend@...ha.franken.de>, Théo Lebrun <theo.lebrun@...tlin.com>
Cc: linux-mips@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, linux-gpio@...r.kernel.org, Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>, Gregory CLEMENT <gregory.clement@...tlin.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>, Tawfik Bayouk <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH v2 00/11] Add Mobileye EyeQ system controller support (clk, reset, pinctrl)

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/

Powered by blists - more mailing lists