by Vladimir Oltean

[permalink] [raw]

Subject: Re: [PATCH v11 net-next 1/9] mfd: ocelot: add helper to get regmap from a resource

On Fri, Jul 01, 2022 at 10:18:31AM -0700, Colin Foster wrote:
> While I have your ear: do I need to check for dev->parent == NULL before
> calling dev_get_regmap? I see find_dr will call
> (dev->parent)->devres_head... but specifically "does every device have a
> valid parent?"

While the technical answer is "no", the practical answer is "pretty much".
Platform devices sit at least on the "platform" bus created in drivers/base/platform.c,
and they are reparented to the "platform_bus" struct device named "platform"
within platform_device_add(), if they don't have a parent.

Additionally, for MMIO-controlled platform devices in Ocelot, these have
as parent a platform device probed by the drivers/bus/simple-pm-bus.c
driver on the "ahb@70000000" simple-bus OF node. That simple-bus
platform device has as parent the "platform_bus" device mentioned above.

So it's a pretty long way to the top in the device hierarchy, I wouldn't
concern myself too much with checking for NULL, unless you intend to
call dev_get_regmap() on a parent's parent's parent, or things like that.

> > > }
> > >
> > > So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain
> > > an inline helper function. And so long as ocelot_core_init does this:
> > >
> > > static void ocelot_core_try_add_regmap(struct device *dev,
> > > const struct resource *res)
> > > {
> > > if (!dev_get_regmap(dev, res->name)) {
> > > ocelot_spi_init_regmap(dev, res);
> > > }
> > > }
> > >
> > > static void ocelot_core_try_add_regmaps(struct device *dev,
> > > const struct mfd_cell *cell)
> > > {
> > > int i;
> > >
> > > for (i = 0; i < cell->num_resources; i++) {
> > > ocelot_core_try_add_regmap(dev, &cell->resources[i]);
> > > }
> > > }
> > >
> > > int ocelot_core_init(struct device *dev)
> > > {
> > > int i, ndevs;
> > >
> > > ndevs = ARRAY_SIZE(vsc7512_devs);
> > >
> > > for (i = 0; i < ndevs; i++)
> > > ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
> >
> > Dumb question, why just "try"?
>
> Because of this conditional:
> > > if (!dev_get_regmap(dev, res->name)) {
> Don't add it if it is already there.

Hmm. So that's because you add regmaps iterating by the resource table
of each device. What if you keep a single resource table for regmap
creation purposes, and the device resource tables as separate?

> This might get interesting... The soc uses the HSIO regmap by way of
> syscon. Among other things, drivers/phy/mscc/phy-ocelot-serdes.c. If
> dev->parent has all the regmaps, what role does syscon play?
>
> But that's a problem for another day...

Interesting question. I think part of the reason why syscon exists is to
not have OF nodes with overlapping address regions. In that sense, its
need does not go away here - I expect the layout of OF nodes beneath the
ocelot SPI device to be the same as their AHB variants. But in terms of
driver implementation, I don't know. Even if the OF nodes for your MFD
functions will contain all the regs that their AHB variants do, I'd
personally still be inclined to also hardcode those as resources in the
ocelot mfd parent driver and use those - case in which the OF regs will
more or less exist just as a formality. Maybe because the HSIO syscon is
already compatible with "simple-mfd", devices beneath it should just
probe. I haven't studied how syscon_node_to_regmap() behaves when the
syscon itself is probed as a MFD function. If that "just works", then
the phy-ocelot-serdes.c driver might not need to be modified.

2022-07-02 16:20:17

by Colin Foster

[permalink] [raw]

Subject: Re: [PATCH v11 net-next 1/9] mfd: ocelot: add helper to get regmap from a resource

On Sat, Jul 02, 2022 at 12:42:06PM +0000, Vladimir Oltean wrote:
> On Fri, Jul 01, 2022 at 10:18:31AM -0700, Colin Foster wrote:
> > While I have your ear: do I need to check for dev->parent == NULL before
> > calling dev_get_regmap? I see find_dr will call
> > (dev->parent)->devres_head... but specifically "does every device have a
> > valid parent?"
>
> While the technical answer is "no", the practical answer is "pretty much".
> Platform devices sit at least on the "platform" bus created in drivers/base/platform.c,
> and they are reparented to the "platform_bus" struct device named "platform"
> within platform_device_add(), if they don't have a parent.
>
> Additionally, for MMIO-controlled platform devices in Ocelot, these have
> as parent a platform device probed by the drivers/bus/simple-pm-bus.c
> driver on the "ahb@70000000" simple-bus OF node. That simple-bus
> platform device has as parent the "platform_bus" device mentioned above.
>
> So it's a pretty long way to the top in the device hierarchy, I wouldn't
> concern myself too much with checking for NULL, unless you intend to
> call dev_get_regmap() on a parent's parent's parent, or things like that.

Thanks for the info. I have the NULL check in there, since I followed
the code and didn't see anything in device initialization that always
initializes parent. Maybe a default initializer would be
dev->parent = dev;

>
> > > > }
> > > >
> > > > So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain
> > > > an inline helper function. And so long as ocelot_core_init does this:
> > > >
> > > > static void ocelot_core_try_add_regmap(struct device *dev,
> > > > const struct resource *res)
> > > > {
> > > > if (!dev_get_regmap(dev, res->name)) {
> > > > ocelot_spi_init_regmap(dev, res);
> > > > }
> > > > }
> > > >
> > > > static void ocelot_core_try_add_regmaps(struct device *dev,
> > > > const struct mfd_cell *cell)
> > > > {
> > > > int i;
> > > >
> > > > for (i = 0; i < cell->num_resources; i++) {
> > > > ocelot_core_try_add_regmap(dev, &cell->resources[i]);
> > > > }
> > > > }
> > > >
> > > > int ocelot_core_init(struct device *dev)
> > > > {
> > > > int i, ndevs;
> > > >
> > > > ndevs = ARRAY_SIZE(vsc7512_devs);
> > > >
> > > > for (i = 0; i < ndevs; i++)
> > > > ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
> > >
> > > Dumb question, why just "try"?
> >
> > Because of this conditional:
> > > > if (!dev_get_regmap(dev, res->name)) {
> > Don't add it if it is already there.
>
> Hmm. So that's because you add regmaps iterating by the resource table
> of each device. What if you keep a single resource table for regmap
> creation purposes, and the device resource tables as separate?

That would work - though it seems like it might be adding extra info
that isn't necessary. I'll take a look.

>
> > This might get interesting... The soc uses the HSIO regmap by way of
> > syscon. Among other things, drivers/phy/mscc/phy-ocelot-serdes.c. If
> > dev->parent has all the regmaps, what role does syscon play?
> >
> > But that's a problem for another day...
>
> Interesting question. I think part of the reason why syscon exists is to
> not have OF nodes with overlapping address regions. In that sense, its
> need does not go away here - I expect the layout of OF nodes beneath the
> ocelot SPI device to be the same as their AHB variants. But in terms of
> driver implementation, I don't know. Even if the OF nodes for your MFD
> functions will contain all the regs that their AHB variants do, I'd
> personally still be inclined to also hardcode those as resources in the
> ocelot mfd parent driver and use those - case in which the OF regs will
> more or less exist just as a formality. Maybe because the HSIO syscon is
> already compatible with "simple-mfd", devices beneath it should just
> probe. I haven't studied how syscon_node_to_regmap() behaves when the
> syscon itself is probed as a MFD function. If that "just works", then
> the phy-ocelot-serdes.c driver might not need to be modified.

That'd be nice! When I looked into it a few months ago I came to the
conclusion that I'd need to implement "mscc,ocelot-hsio" but maybe
there's something I missed.