From: Vladimir Oltean <vladimir.oltean@nxp.com> To: Colin Foster <colin.foster@in-advantage.com> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, Lee Jones <lee.jones@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King <linux@armlinux.org.uk>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Lars Povlsen <lars.povlsen@microchip.com>, Steen Hegelund <Steen.Hegelund@microchip.com>, "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>, Linus Walleij <linus.walleij@linaro.org>, Wolfram Sang <wsa@kernel.org>, Terry Bowman <terry.bowman@amd.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Arnd Bergmann <arnd@kernel.org> Subject: Re: [PATCH v11 net-next 1/9] mfd: ocelot: add helper to get regmap from a resource Date: Fri, 1 Jul 2022 16:21:28 +0000 [thread overview] Message-ID: <20220701162126.wbembm47snbggxwv@skbuf> (raw) In-Reply-To: <20220630200951.GB2152027@euler> On Thu, Jun 30, 2022 at 01:09:51PM -0700, Colin Foster wrote: > 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)); parentheses not needed around dev_get_regmap. > } > > 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]); Dumb question, why just "try"? > > 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? Sounds like something which could be named devm_platform_get_regmap_from_resource_or_parent(), but I'm not 100% clear where it should sit. Platform devices are independent of regmap, regmap is independent of platform devices, device core of both. FWIW platform devices are always built-in and have no config option; regmap is bool and is selected by others. Logically, the melting pot of regmaps and platform devices is mfd. However, it seems that include/linux/mfd/core.h only provides API for mfd parent drivers, not children. So a new header would be needed? Alternatively, you could just duplicate this logic in the drivers (by the way, only spelling out the function name takes up half of the implementation). How many times would it be needed? Felix DSA would roll its own thing, as mentioned. I'm thinking, let it be open coded for now, let's agree on the entire solution in terms of operations that are actually being done, and we can revisit proper placement for this later. > 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);" Sorry, I just realized we need to junk this idea with GPIO instead of 0. Presenting the entire resource table to all peripherals implies that there is no more than one single peripheral of each kind. This is not true for the MDIO controllers, where the driver would need to know it has to request the region corresponding to MIIM1 or MIIM2 according to some crystal ball.
WARNING: multiple messages have this Message-ID (diff)
From: Vladimir Oltean <vladimir.oltean@nxp.com> To: Colin Foster <colin.foster@in-advantage.com> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, Lee Jones <lee.jones@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King <linux@armlinux.org.uk>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Lars Povlsen <lars.povlsen@microchip.com>, Steen Hegelund <Steen.Hegelund@microchip.com>, "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>, Linus Walleij <linus.walleij@linaro.org>, Wolfram Sang <wsa@kernel.org>, Terry Bowman <terry.bowman@amd.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Arnd Bergmann <arnd@kernel.org> Subject: Re: [PATCH v11 net-next 1/9] mfd: ocelot: add helper to get regmap from a resource Date: Fri, 1 Jul 2022 16:21:28 +0000 [thread overview] Message-ID: <20220701162126.wbembm47snbggxwv@skbuf> (raw) In-Reply-To: <20220630200951.GB2152027@euler> On Thu, Jun 30, 2022 at 01:09:51PM -0700, Colin Foster wrote: > 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)); parentheses not needed around dev_get_regmap. > } > > 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]); Dumb question, why just "try"? > > 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? Sounds like something which could be named devm_platform_get_regmap_from_resource_or_parent(), but I'm not 100% clear where it should sit. Platform devices are independent of regmap, regmap is independent of platform devices, device core of both. FWIW platform devices are always built-in and have no config option; regmap is bool and is selected by others. Logically, the melting pot of regmaps and platform devices is mfd. However, it seems that include/linux/mfd/core.h only provides API for mfd parent drivers, not children. So a new header would be needed? Alternatively, you could just duplicate this logic in the drivers (by the way, only spelling out the function name takes up half of the implementation). How many times would it be needed? Felix DSA would roll its own thing, as mentioned. I'm thinking, let it be open coded for now, let's agree on the entire solution in terms of operations that are actually being done, and we can revisit proper placement for this later. > 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);" Sorry, I just realized we need to junk this idea with GPIO instead of 0. Presenting the entire resource table to all peripherals implies that there is no more than one single peripheral of each kind. This is not true for the MDIO controllers, where the driver would need to know it has to request the region corresponding to MIIM1 or MIIM2 according to some crystal ball. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-07-01 16:21 UTC|newest] Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-28 8:17 [PATCH v11 net-next 0/9] add support for VSC7512 control over SPI Colin Foster 2022-06-28 8:17 ` Colin Foster 2022-06-28 8:17 ` [PATCH v11 net-next 1/9] mfd: ocelot: add helper to get regmap from a resource Colin Foster 2022-06-28 8:17 ` Colin Foster 2022-06-28 12:50 ` Andy Shevchenko 2022-06-28 12:50 ` Andy Shevchenko 2022-06-28 15:33 ` Vladimir Oltean 2022-06-28 15:33 ` Vladimir Oltean 2022-06-28 16:08 ` Vladimir Oltean 2022-06-28 16:08 ` Vladimir Oltean 2022-06-28 17:25 ` Colin Foster 2022-06-28 17:25 ` Colin Foster 2022-06-28 18:47 ` Vladimir Oltean 2022-06-28 18:47 ` Vladimir Oltean 2022-06-28 18:56 ` Vladimir Oltean 2022-06-28 18:56 ` Vladimir Oltean 2022-06-28 19:04 ` Andy Shevchenko 2022-06-28 19:04 ` Andy Shevchenko 2022-06-28 19:56 ` Colin Foster 2022-06-28 19:56 ` Colin Foster 2022-06-29 17:53 ` Vladimir Oltean 2022-06-29 17:53 ` Vladimir Oltean 2022-06-29 20:39 ` Colin Foster 2022-06-29 20:39 ` Colin Foster 2022-06-29 23:08 ` Vladimir Oltean 2022-06-29 23:08 ` Vladimir Oltean 2022-06-29 23:54 ` Colin Foster 2022-06-29 23:54 ` Colin Foster 2022-06-30 7:54 ` Lee Jones 2022-06-30 7:54 ` Lee Jones 2022-06-30 13:11 ` Vladimir Oltean 2022-06-30 13:11 ` Vladimir Oltean 2022-06-30 20:09 ` Colin Foster 2022-06-30 20:09 ` Colin Foster 2022-07-01 16:21 ` Vladimir Oltean [this message] 2022-07-01 16:21 ` Vladimir Oltean 2022-07-01 17:18 ` Colin Foster 2022-07-01 17:18 ` Colin Foster 2022-07-02 12:42 ` Vladimir Oltean 2022-07-02 12:42 ` Vladimir Oltean 2022-07-02 16:17 ` Colin Foster 2022-07-02 16:17 ` Colin Foster 2022-06-28 8:17 ` [PATCH v11 net-next 2/9] net: mdio: mscc-miim: add ability to be used in a non-mmio configuration Colin Foster 2022-06-28 8:17 ` Colin Foster 2022-06-28 16:26 ` Vladimir Oltean 2022-06-28 16:26 ` Vladimir Oltean 2022-06-28 18:31 ` Colin Foster 2022-06-28 18:31 ` Colin Foster 2022-06-28 8:17 ` [PATCH v11 net-next 3/9] pinctrl: ocelot: allow pinctrl-ocelot to be loaded as a module Colin Foster 2022-06-28 8:17 ` Colin Foster 2022-06-28 12:53 ` Andy Shevchenko 2022-06-28 12:53 ` Andy Shevchenko 2022-06-28 18:25 ` Colin Foster 2022-06-28 18:25 ` Colin Foster 2022-06-28 19:00 ` Andy Shevchenko 2022-06-28 19:00 ` Andy Shevchenko 2022-06-30 11:56 ` Linus Walleij 2022-06-30 11:56 ` Linus Walleij 2022-06-28 8:17 ` [PATCH v11 net-next 4/9] pinctrl: ocelot: add ability to be used in a non-mmio configuration Colin Foster 2022-06-28 8:17 ` Colin Foster 2022-06-28 8:17 ` [PATCH v11 net-next 5/9] pinctrl: microchip-sgpio: allow sgpio driver to be used as a module Colin Foster 2022-06-28 8:17 ` Colin Foster 2022-06-28 12:55 ` Andy Shevchenko 2022-06-28 12:55 ` Andy Shevchenko 2022-06-28 8:17 ` [PATCH v11 net-next 6/9] pinctrl: microchip-sgpio: add ability to be used in a non-mmio configuration Colin Foster 2022-06-28 8:17 ` Colin Foster 2022-06-28 8:17 ` [PATCH v11 net-next 7/9] resource: add define macro for register address resources Colin Foster 2022-06-28 8:17 ` Colin Foster 2022-06-28 8:17 ` [PATCH v11 net-next 8/9] dt-bindings: mfd: ocelot: add bindings for VSC7512 Colin Foster 2022-06-28 8:17 ` Colin Foster 2022-06-28 13:15 ` Rob Herring 2022-06-28 13:15 ` Rob Herring 2022-06-28 18:19 ` Colin Foster 2022-06-28 18:19 ` Colin Foster 2022-06-30 13:17 ` Vladimir Oltean 2022-06-30 13:17 ` Vladimir Oltean 2022-06-28 8:17 ` [PATCH v11 net-next 9/9] mfd: ocelot: add support for the vsc7512 chip via spi Colin Foster 2022-06-28 8:17 ` Colin Foster 2022-06-28 20:07 ` Randy Dunlap 2022-06-28 20:07 ` Randy Dunlap 2022-06-28 20:24 ` Colin Foster 2022-06-28 20:24 ` Colin Foster
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220701162126.wbembm47snbggxwv@skbuf \ --to=vladimir.oltean@nxp.com \ --cc=Steen.Hegelund@microchip.com \ --cc=UNGLinuxDriver@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=andrew@lunn.ch \ --cc=andy.shevchenko@gmail.com \ --cc=arnd@kernel.org \ --cc=colin.foster@in-advantage.com \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=edumazet@google.com \ --cc=hkallweit1@gmail.com \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=kuba@kernel.org \ --cc=lars.povlsen@microchip.com \ --cc=lee.jones@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=netdev@vger.kernel.org \ --cc=pabeni@redhat.com \ --cc=robh+dt@kernel.org \ --cc=terry.bowman@amd.com \ --cc=wsa@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.