| Message ID | 20220610175655.776153-3-colin.foster@in-advantage.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for VSC7512 control over SPI | expand |
On Fri, Jun 10, 2022 at 7:57 PM Colin Foster <colin.foster@in-advantage.com> 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 > <colin.foster@in-advantage.com> 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
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);
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 <colin.foster@in-advantage.com> --- drivers/net/mdio/mdio-mscc-miim.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)