Re: [RFC v8 net-next 08/16] mfd: ocelot: add support for the vsc7512 chip via spi

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

On Tue, May 10, 2022 at 04:32:26PM +0100, Lee Jones wrote:
> On Mon, 09 May 2022, Vladimir Oltean wrote:
> 
> > On Mon, May 09, 2022 at 04:49:22PM -0700, Colin Foster wrote:
> > > > > +struct regmap *ocelot_init_regmap_from_resource(struct device *child,
> > > > > +						const struct resource *res)
> > > > > +{
> > > > > +	struct device *dev = child->parent;
> > > > > +
> > > > > +	return ocelot_spi_devm_init_regmap(dev, child, res);
> > > > 
> > > > So much for being bus-agnostic :-/
> > > > Maybe get the struct ocelot_ddata and call ocelot_spi_devm_init_regmap()
> > > > via a function pointer which is populated by ocelot-spi.c? If you do
> > > > that don't forget to clean up drivers/mfd/ocelot.h of SPI specific stuff.
> > > 
> > > That was my initial design. "core" was calling into "spi" exclusively
> > > via function pointers.
> > > 
> > > The request was "Please find a clearer way to do this without function
> > > pointers"
> > > 
> > > https://lore.kernel.org/netdev/Ydwju35sN9QJqJ%2FP@xxxxxxxxxx/
> > 
> > Yeah, I'm not sure what Lee was looking for, either. In any case I agree
> > with the comment that you aren't configuring a bus. In this context it
> > seems more appropriate to call this function pointer "init_regmap", with
> > different implementations per transport.
> 
> FWIW, I'm still against using function pointers for this.
> 
> What about making ocelot_init_regmap_from_resource() an inline
> function and pushing it into one of the header files?
> 
> [As an aside, you don't need to pass both dev (parent) and child]

I see your point. This wasn't always the case, since ocelot-core prior
to v8 would call ocelot_spi_devm_init_regmap. Since this was changed,
the "dev, dev" part can all be handled internally. That's nice.

> 
> In there you could simply do:
> 
> inline struct regmap *ocelot_init_regmap_from_resource(struct device *dev,
> 						       const struct resource *res)
> {
> 	if (dev_get_drvdata(dev->parent)->spi)
> 		return ocelot_spi_devm_init_regmap(dev, res);
> 
> 	return NULL;
> }

If I do this, won't I have to declare ocelot_spi_devm_init_regmap in a
larger scope (include/soc/mscc/ocelot.h)? I like the idea of keeping it
more hidden inside drivers/mfd/ocelot.h, assuming I can't keep it
enclosed in drivers/mfd/ocelot-spi.c entirely.

> 
> Also, do you really need devm in the title?

Understood. I figured adding devm made it clearer about what is going
on. I don't have a strong opinion one way or another. If nothing else,
it'll shorten my function names which can be wordy... But as I say this
I noticed "ocelot_init_regmap_from_resource" doesn't have devm. I'll
remove it.

> 
> > Or alternatively you could leave the "core"/"spi" pseudo-separation up
> > to the next person who needs to add support for some other register I/O
> > method.
> 
> Or this.  Your call.

I do like having them separate. Even as I've been working on v8, it has
been clear that "these commits go toward improving the spi section"
while others are implementing core features (serdes, for example.)

I feel like v8 has landed in a pretty good spot between keeping
everything completely separate and having everything be one file.

> 
> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog