There are a few Ocelot chips that contain the logic for this bus, but are
controlled externally. Specifically the VSC7511, 7512, 7513, and 7514. In
the externally controlled configurations these registers are not
memory-mapped.
Add support for these non-memory-mapped configurations.
Signed-off-by: Colin Foster <[email protected]>
---
drivers/net/mdio/mdio-mscc-miim.c | 27 ++++++++-------------------
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index 08541007b18a..cd89a313cf82 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -12,6 +12,7 @@
#include <linux/iopoll.h>
#include <linux/kernel.h>
#include <linux/mdio/mdio-mscc-miim.h>
+#include <linux/mfd/ocelot.h>
#include <linux/module.h>
#include <linux/of_mdio.h>
#include <linux/phy.h>
@@ -270,43 +271,31 @@ static int mscc_miim_clk_set(struct mii_bus *bus)
static int mscc_miim_probe(struct platform_device *pdev)
{
- struct regmap *mii_regmap, *phy_regmap = NULL;
struct device_node *np = pdev->dev.of_node;
+ struct regmap *mii_regmap, *phy_regmap;
struct device *dev = &pdev->dev;
- void __iomem *regs, *phy_regs;
struct mscc_miim_dev *miim;
struct resource *res;
struct mii_bus *bus;
int ret;
- regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
- if (IS_ERR(regs)) {
- dev_err(dev, "Unable to map MIIM registers\n");
- return PTR_ERR(regs);
- }
-
- mii_regmap = devm_regmap_init_mmio(dev, regs, &mscc_miim_regmap_config);
-
+ ocelot_platform_init_regmap_from_resource(pdev, 0, &mii_regmap, NULL,
+ &mscc_miim_regmap_config);
if (IS_ERR(mii_regmap)) {
dev_err(dev, "Unable to create MIIM regmap\n");
return PTR_ERR(mii_regmap);
}
/* This resource is optional */
- res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ ocelot_platform_init_regmap_from_resource(pdev, 1, &phy_regmap, &res,
+ &mscc_miim_phy_regmap_config);
if (res) {
- phy_regs = devm_ioremap_resource(dev, res);
- if (IS_ERR(phy_regs)) {
- dev_err(dev, "Unable to map internal phy registers\n");
- return PTR_ERR(phy_regs);
- }
-
- phy_regmap = devm_regmap_init_mmio(dev, phy_regs,
- &mscc_miim_phy_regmap_config);
if (IS_ERR(phy_regmap)) {
dev_err(dev, "Unable to create phy register regmap\n");
return PTR_ERR(phy_regmap);
}
+ } else {
+ phy_regmap = NULL;
}
ret = mscc_miim_setup(dev, &bus, "mscc_miim", mii_regmap, 0);
--
2.25.1
On Fri, Jun 10, 2022 at 7:57 PM Colin Foster
<[email protected]> wrote:
>
> There are a few Ocelot chips that contain the logic for this bus, but are
> controlled externally. Specifically the VSC7511, 7512, 7513, and 7514. In
> the externally controlled configurations these registers are not
> memory-mapped.
>
> Add support for these non-memory-mapped configurations.
...
> + ocelot_platform_init_regmap_from_resource(pdev, 0, &mii_regmap, NULL,
> + &mscc_miim_regmap_config);
This is a bit non-standard, why not to follow the previously used API
design, i.e.
mii_regmap.map = ...
?
...
> + ocelot_platform_init_regmap_from_resource(pdev, 1, &phy_regmap, &res,
> + &mscc_miim_phy_regmap_config);
Ditto.
Also here is the question how '_from_' is aligned with '&res'. If
it's _from_ a resource then the resource is already a pointer.
...
> if (res) {
> - phy_regs = devm_ioremap_resource(dev, res);
> - if (IS_ERR(phy_regs)) {
> - dev_err(dev, "Unable to map internal phy registers\n");
> - return PTR_ERR(phy_regs);
> - }
> -
> - phy_regmap = devm_regmap_init_mmio(dev, phy_regs,
> - &mscc_miim_phy_regmap_config);
> if (IS_ERR(phy_regmap)) {
> dev_err(dev, "Unable to create phy register regmap\n");
> return PTR_ERR(phy_regmap);
> }
This looks weird. You check an error here instead of the API you
called. It's a weird design, the rationale of which is doubtful and
has to be at least explained.
> + } else {
> + phy_regmap = NULL;
> }
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Sat, Jun 11, 2022 at 12:34:59PM +0200, Andy Shevchenko wrote:
> On Fri, Jun 10, 2022 at 7:57 PM Colin Foster
> <[email protected]> wrote:
> >
> > There are a few Ocelot chips that contain the logic for this bus, but are
> > controlled externally. Specifically the VSC7511, 7512, 7513, and 7514. In
> > the externally controlled configurations these registers are not
> > memory-mapped.
> >
> > Add support for these non-memory-mapped configurations.
>
> ...
>
> > + ocelot_platform_init_regmap_from_resource(pdev, 0, &mii_regmap, NULL,
> > + &mscc_miim_regmap_config);
>
> This is a bit non-standard, why not to follow the previously used API
> design, i.e.
>
> mii_regmap.map = ...
>
> ?
I see your point. It looks like there's no reason to pass in &mii_regmap
and it can just be returned.
>
> ...
>
> > + ocelot_platform_init_regmap_from_resource(pdev, 1, &phy_regmap, &res,
> > + &mscc_miim_phy_regmap_config);
>
> Ditto.
>
> Also here is the question how '_from_' is aligned with '&res'. If
> it's _from_ a resource then the resource is already a pointer.
Yes, this is probably worth a second look. During v8 you noted that I
was repeating a lot of the same logic across several files, so I created
ocelot_platform_init_regmap_from_resource.
The "gotcha" is while most of those scenarios have a required resource,
the phy_regmap is optional - so a scenario where the resource doesn't
exist could be considered valid.
Would it make sense to make the init_regmap_from_resource function
return ERR_PTR(-ENOENT) if regs doesn't exist? That would clean up the
API quite a bit:
phy_regmap = ocelot_platform_init_regmap_from_resource(pdev, 1,
&map_config);
if (IS_ERR(phy_regmap) && phy_regmap != -ENOENT) {
dev_err(); ...
}
It looks like none of the two functions that would get returned
otherwise (devm_regmap_init or devm_regmap_init_mmio) would return that
value...
>
> ...
>
> > if (res) {
> > - phy_regs = devm_ioremap_resource(dev, res);
> > - if (IS_ERR(phy_regs)) {
> > - dev_err(dev, "Unable to map internal phy registers\n");
> > - return PTR_ERR(phy_regs);
> > - }
> > -
> > - phy_regmap = devm_regmap_init_mmio(dev, phy_regs,
> > - &mscc_miim_phy_regmap_config);
> > if (IS_ERR(phy_regmap)) {
> > dev_err(dev, "Unable to create phy register regmap\n");
> > return PTR_ERR(phy_regmap);
> > }
>
> This looks weird. You check an error here instead of the API you
> called. It's a weird design, the rationale of which is doubtful and
> has to be at least explained.
I agree. With the changes I'm suggesting above this block of code would
become:
if (IS_ERR(phy_regmap)) {
if (phy_regmap == -ENOENT) {
phy_regmap = NULL;
} else {
dev_err(dev, "...");
return PTR_ERR(phy_regmap);
}
}
That seems easier to follow than the if(res) block...
Thanks for the feedback!
>
> > + } else {
> > + phy_regmap = NULL;
> > }
>
> --
> With Best Regards,
> Andy Shevchenko