Re: [PATCH 1/8] mfd: ocelot: refactor bus-specific regmap initialization

[Lee Jones]

On Fri, 21 Mar 2025, Rasmus Villemoes wrote:

> On Fri, Mar 21 2025, Lee Jones <lee@xxxxxxxxxx> wrote:
> 
> > On Wed, 19 Mar 2025, Colin Foster wrote:
> >
> >> On Wed, Mar 19, 2025 at 01:30:51PM +0100, Rasmus Villemoes wrote:
> >> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> >> > index 41aff27088548..78b5fe15efdd2 100644
> >> > --- a/drivers/mfd/ocelot-core.c
> >> > +++ b/drivers/mfd/ocelot-core.c
> >> > @@ -200,10 +200,12 @@ static const struct mfd_cell vsc7512_devs[] = {
> >> >  static void ocelot_core_try_add_regmap(struct device *dev,
> >> >                                        const struct resource *res)
> >> >  {
> >> > +       struct ocelot_ddata *ddata = dev_get_drvdata(dev);
> >> > +
> >> >         if (dev_get_regmap(dev, res->name))
> >> >                 return;
> >> > 
> >> > -       ocelot_spi_init_regmap(dev, res);
> >> > +       ddata->init_regmap(dev, res);
> >> 
> >> I remember changing this from function pointers to the direct function
> >> call during initial development, per Lee's suggestion. I like it though,
> >> and I'm glad to see multiple users now.
> >
> > Yeah, we're still not going to be putting call-backs into device data.
> 
> OK. Can you explain why that is such a bad design?

It opens things up for all kinds of other 'cleverness' (a.k.a. hackery).

Save call-backs for things like class-level ops, not driver level hacks.

> > Either pass the differentiating config through to the core driver
> 
> So you mean something like defining a new struct ocelot_backend_ops { } with
> those function pointers, and pass an instance of that to
> ocelot_core_init (and from there down to the static helper functions)?
> That should be doable.

No call-backs at all.  Pass a pointer to the Regmap data.

> > or handle the differentiation inside the *-i2c.c / *-spi.c files.
> 
> I really fail to see how that could be done. Currently, the core file
> has a hard-coded call of ocelot_spi_init_regmap(). I don't suppose you
> mean to teach that function to realize "hey, this struct device is not
> really a struct spi_device, let's delegate to ocelot_mdio_init_regmap()
> instead". So _somehow_ the core will need to know to call one or the
> other init_regmap implementation. I could add some "enum ocelot_type"
> and switch on that and then call the appropriate bus-specific function,
> but that's morally equivalent to having the function pointers.

Enums are acceptable for this use-case.  Opaque function pointers that
are troublesome to debug / read without; running the code, printing
pointers then conducting a system table look-up, are not.

It's not that function pointers wouldn't work perfectly well in this
scenario.  The issue is the precedent it (or any of the previous
attempts to do this) would set and the chaos that would follow.

-- 
Lee Jones [李琼斯]