Hi Vladimir,

On Tue, Sep 27, 2022 at 08:53:53PM +0300, Vladimir Oltean wrote:

Hi Colin,

On Sun, Sep 25, 2022 at 05:29:22PM -0700, Colin Foster wrote:

quoted

During development, it was believed that a wrapper for ocelot_regmap_init()
would be sufficient for the felix driver to work in non-mmio scenarios.
This was merged in during commit 242bd0c10bbd ("net: dsa: ocelot: felix:
add interface for custom regmaps")

As the external ocelot DSA driver grew closer to an acceptable state, it
was realized that most of the parameters that were passed in from struct
resource *res were useless and ignored. This is due to the fact that the
external ocelot DSA driver utilizes dev_get_regmap(dev, resource->name).

Instead of simply ignoring those parameters, refactor the API to only
require the name as an argument. MMIO scenarios this will reconstruct the
struct resource before calling ocelot_regmap_init(ocelot, resource). MFD
scenarios need only call dev_get_regmap(dev, name).

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---

I don't like how this turned out. I was expecting you not to look at the
exported resources from the ocelot-core anymore - that was kind of the
point of using just the names rather than the whole resource definitions.

I see your point. The init_regmap(name) interface collides with the
*_io_res arrays. Changing the init_regmap() interface doesn't really
change the underlying issue - *_io_res[] is the thing that you're
suggesting to go.

I'm interested to see where this is going. I feel like it might be a
constant names[] array, then felix_vsc9959_init_regmap() where the
specific name <> resource mapping happens. Maybe a common
felix_match_resource_to_name(name, res, len)?

That would definitely remove the need for exporting the
vsc7512_*_io_res[] arrays, which I didn't understand from your v1
review.


Something like:
include/soc/mscc/ocelot.h
#define OCELOT_RES_NAME_ANA "ana"

const char *ocelot_target_names[TARGET_MAX] = {[ANA] = OCELOT_RES_NAME_ANA};

...


drivers/net/dsa/ocelot/felix.c
for (i = 0; i < TARGET_MAX; i++)
    target = felix->info->init_regmap(ocelot_target_names[i]);

...


drivers/net/dsa/ocelot/felix_vsc9959.c
static const struct resource vsc9959_target_io_res[TARGET_MAX] = ...;

vsc9959_init_regmap(name)
{
    /* more logic for port_io_res, but you get the point */
    return felix_init_regmap(name, &vsc9959_target_io_res, TARGET_MAX);
}

I am also sorry for the mess that the felix driver currently is in, and
the fact that some things may have confused you.

Vladimir, you might be the last person on earth who owes me an apology.