Make ocelot-core truly bus-agnostic by letting the bus-specific part
set an ->init_regmap callback in struct ocelot_ddata, instead of
relying on the bus being spi.
With this, the only symbol in the MFD_OCELOT_SPI namespace vanishes,
and hence ocelot-core should no longer import that.
This is preparation for adding support for mdio-based management of
the Ocelot chip.
Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
drivers/mfd/ocelot-core.c | 5 +++--
drivers/mfd/ocelot-spi.c | 4 ++--
drivers/mfd/ocelot.h | 6 ++----
3 files changed, 7 insertions(+), 8 deletions(-)
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);
}
static void ocelot_core_try_add_regmaps(struct device *dev,
@@ -231,4 +233,3 @@ EXPORT_SYMBOL_NS(ocelot_core_init, "MFD_OCELOT");
MODULE_DESCRIPTION("Externally Controlled Ocelot Chip Driver");
MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
MODULE_LICENSE("GPL");
-MODULE_IMPORT_NS("MFD_OCELOT_SPI");
diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
index 1fed9878c3231..a320a613d00e1 100644
--- a/drivers/mfd/ocelot-spi.c
+++ b/drivers/mfd/ocelot-spi.c
@@ -181,7 +181,7 @@ static const struct regmap_bus ocelot_spi_regmap_bus = {
.read = ocelot_spi_regmap_bus_read,
};
-struct regmap *ocelot_spi_init_regmap(struct device *dev, const struct resource *res)
+static struct regmap *ocelot_spi_init_regmap(struct device *dev, const struct resource *res)
{
struct regmap_config regmap_config;
@@ -193,7 +193,6 @@ struct regmap *ocelot_spi_init_regmap(struct device *dev, const struct resource
return devm_regmap_init(dev, &ocelot_spi_regmap_bus, dev, ®map_config);
}
-EXPORT_SYMBOL_NS(ocelot_spi_init_regmap, "MFD_OCELOT_SPI");
static int ocelot_spi_probe(struct spi_device *spi)
{
@@ -207,6 +206,7 @@ static int ocelot_spi_probe(struct spi_device *spi)
return -ENOMEM;
spi_set_drvdata(spi, ddata);
+ ddata->init_regmap = ocelot_spi_init_regmap;
if (spi->max_speed_hz <= 500000) {
ddata->spi_padding_bytes = 0;
diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
index b8bc2f1486e24..4305e7a55cb1a 100644
--- a/drivers/mfd/ocelot.h
+++ b/drivers/mfd/ocelot.h
@@ -12,6 +12,7 @@ struct resource;
/**
* struct ocelot_ddata - Private data for an external Ocelot chip
+ * @init_regmap: Bus-specific callback for initializing regmap.
* @gcb_regmap: General Configuration Block regmap. Used for
* operations like chip reset.
* @cpuorg_regmap: CPU Device Origin Block regmap. Used for operations
@@ -24,6 +25,7 @@ struct resource;
* data of a SPI read operation.
*/
struct ocelot_ddata {
+ struct regmap * (*init_regmap)(struct device *dev, const struct resource *res);
struct regmap *gcb_regmap;
struct regmap *cpuorg_regmap;
int spi_padding_bytes;
@@ -33,10 +35,6 @@ struct ocelot_ddata {
int ocelot_chip_reset(struct device *dev);
int ocelot_core_init(struct device *dev);
-/* SPI-specific routines that won't be necessary for other interfaces */
-struct regmap *ocelot_spi_init_regmap(struct device *dev,
- const struct resource *res);
-
#define OCELOT_SPI_BYTE_ORDER_LE 0x00000000
#define OCELOT_SPI_BYTE_ORDER_BE 0x81818181
--
2.49.0On 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.
Reviewed-by: Colin Foster <colin.foster@in-advantage.com>
Tested-by: Colin Foster <colin.foster@in-advantage.com>
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.
Either pass the differentiating config through to the core driver or
handle the differentiation inside the *-i2c.c / *-spi.c files.
--
Lee Jones [李琼斯]
On Fri, Mar 21 2025, Lee Jones <lee@kernel.org> 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?
> 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.
> 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.
Thanks,
Rasmus
On Fri, 21 Mar 2025, Rasmus Villemoes wrote:
> On Fri, Mar 21 2025, Lee Jones <lee@kernel.org> 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 [李琼斯]
© 2016 - 2025 Red Hat, Inc.