lkml.org 
[lkml]   [2022]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v11 net-next 1/9] mfd: ocelot: add helper to get regmap from a resource
    On Thu, Jun 30, 2022 at 01:11:56PM +0000, Vladimir Oltean wrote:
    > On Wed, Jun 29, 2022 at 04:54:35PM -0700, Colin Foster wrote:
    > > > > In that case, "name" would either be hard-coded to match what is in
    > > > > drivers/mfd/ocelot-core.c. The other option is to fall back to
    > > > > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in
    > > > > resource->name. I'll be able to deal with that when I try it. (hopefully
    > > > > this evening)
    > > >
    > > > I'm not exactly clear on what you'd do with the REG resource once you
    > > > get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;"
    > > > from the device tree, what next, who's going to set up the SPI regmap
    > > > for you?
    > >
    > > The REG resource would only get the resource name, while the MFD core
    > > driver would set up the regmaps.
    > >
    > > e.g. drivers/mfd/ocelot-core.c has (annotated):
    > > static const struct resource vsc7512_sgpio_resources[] = {
    > > DEFINE_RES_REG_NAMED(start, size, "gcb_gpio") };
    > >
    > > Now, the drivers/pinctrl/pinctrl-ocelot.c expects resource 0 to be the
    > > gpio resource, and gets the resource by index.
    > >
    > > So for this there seem to be two options:
    > > Option 1:
    > > drivers/pinctrl/pinctrl-ocelot.c:
    > > res = platform_get_resource(pdev, IORESOURCE_REG, 0);
    > > map = dev_get_regmap(dev->parent, res->name);
    > >
    > >
    > > OR Option 2:
    > > include/linux/mfd/ocelot.h has something like:
    > > #define GCB_GPIO_REGMAP_NAME "gcb_gpio"
    > >
    > > and drivers/pinctrl/pinctrl-ocelot.c skips get_resource and jumps to:
    > > map = dev_get_regmap(dev->parent, GCB_GPIO_REGMAP_NAME);
    > >
    > > (With error checking, macro reuse, etc.)
    > >
    > >
    > > I like option 1, since it then makes ocelot-pinctrl.c have no reliance
    > > on include/linux/mfd/ocelot.h. But in both cases, all the regmaps are
    > > set up in advance during the start of ocelot_core_init, just before
    > > devm_mfd_add_devices is called.
    > >
    > >
    > > I should be able to test this all tonight.
    >
    > I see what you mean now with the named resources from drivers/mfd/ocelot-core.c.
    > I don't particularly like the platform_get_resource(0) option, because
    > it's not as obvious/searchable what resource the pinctrl driver is
    > asking for.
    >
    > I suppose a compromise variant might be to combine the 2 options.
    > Put enum ocelot_target in a header included by both drivers/mfd/ocelot-core.c,
    > then create a _single_ resource table in the MFD driver, indexed by enum
    > ocelot_target:
    >
    > static const struct resource vsc7512_resources[TARGET_MAX] = {
    > [ANA] = DEFINE_RES_REG_NAMED(start, end, "ana"),
    > ...
    > };
    >
    > then provide the exact same resource table to all children.
    >
    > In the pinctrl driver you can then do:
    > res = platform_get_resource(pdev, IORESOURCE_REG, GPIO);
    > map = dev_get_regmap(dev->parent, res->name);
    >
    > and you get both the benefit of not hardcoding the string twice, and the
    > benefit of having some obvious keyword which can be used to link the mfd
    > driver to the child driver via grep, for those trying to understand what
    > goes on.
    >
    > In addition, if there's a single resource table used for all peripherals,
    > theoretically you need to modify less code in mfd/ocelot-core.c in case
    > one driver or another needs access to one more regmap, if that regmap
    > happened to be needed by some other driver already. Plus fewer tables to
    > lug around, in general.

    Ok... so I haven't yet changed any of the pinctrl / mdio drivers yet,
    but I'm liking this:

    static inline struct regmap *
    ocelot_regmap_from_resource(struct platform_device *pdev, unsigned int index,
    const struct regmap_config *config)
    {
    struct device *dev = &pdev->dev;
    struct resource *res;
    u32 __iomem *regs;

    res = platform_get_resource(pdev, IORESOURCE_MEM, index);
    if (res) {
    regs = devm_ioremap_resource(dev, res);
    if (IS_ERR(regs))
    return ERR_CAST(regs);
    return devm_regmap_init_mmio(dev, regs, config);
    }

    /*
    * Fall back to using REG and getting the resource from the parent
    * device, which is possible in an MFD configuration
    */
    res = platform_get_resource(pdev, IORESOURCE_REG, index);
    if (!res)
    return ERR_PTR(-ENOENT);

    return (dev_get_regmap(dev->parent, res->name));
    }

    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]);

    return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
    ndevs, NULL, 0, NULL);
    }
    EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT);

    we're good! (sorry about spaces / tabs... I have to up my mutt/vim/tmux
    game still)


    I like the enum / macro idea for cleanup, but I think that's a different
    problem I can address. The main question I have now is this:

    The ocelot_regmap_from_resource now has nothing to do with the ocelot
    MFD system. It is generic. (If you listen carefully, you might hear me
    cheering)

    I can keep this in linux/mfd/ocelot.h, but is this actually something
    that belongs elsewhere? platform? device? mfd-core?


    And yes, I like the idea of changing the driver to
    "ocelot_regmap_from_resource(pdev, GPIO, config);" from
    "ocelot_regmap_from_resource(pdev, 0, config);"

    \
     
     \ /
      Last update: 2022-06-30 22:11    [W:3.078 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site