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/
