| Message ID | 20140630144114.GB25926@lee--X1 |
|---|---|
| State | New |
| Headers | show |
Hi, On Monday 30 June 2014 08:11 PM, Lee Jones wrote: > phy: miphy365x: Represent each PHY channel as a DT subnode > > This has the added advantages of being able to enable/disable each > of the channels as simply as enabling/disabling the DT node. > > Suggested-by: Kishon Vijay Abraham I <kishon@ti.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > diff --git a/drivers/phy/phy-miphy365x.c b/drivers/phy/phy-miphy365x.c > index 1109f42..2c4ea6e 100644 > --- a/drivers/phy/phy-miphy365x.c > +++ b/drivers/phy/phy-miphy365x.c > @@ -17,6 +17,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_platform.h> > +#include <linux/of_address.h> > #include <linux/clk.h> > #include <linux/phy/phy.h> > #include <linux/delay.h> > @@ -144,6 +145,7 @@ struct miphy365x { > void __iomem *pcie; > u8 type; > u8 port; > + bool enabled; > }; > > struct miphy365x_dev { > @@ -468,7 +470,7 @@ static struct phy *miphy365x_xlate(struct device *dev, > struct miphy365x_dev *state = dev_get_drvdata(dev); > u8 port, type; > > - if (args->count != 2) { > + if (args->args_count != 2) { > dev_err(dev, "Invalid number of cells in 'phy' property\n"); > return ERR_PTR(-EINVAL); > } > @@ -484,6 +486,11 @@ static struct phy *miphy365x_xlate(struct device *dev, > if (WARN_ON(port >= ARRAY_SIZE(ports))) > return ERR_PTR(-EINVAL); > > + if (!state->phys[port].enabled) { > + dev_warn(dev, "PHY port %d is disabled\n", port); > + return ERR_PTR(-EINVAL); > + } > + > if (type == MIPHY_TYPE_SATA) > state->phys[port].base = state->phys[port].sata; > else if (type == MIPHY_TYPE_PCIE) > @@ -503,38 +510,75 @@ static struct phy_ops miphy365x_ops = { > .owner = THIS_MODULE, > }; > > -static int miphy365x_get_base_addr(struct platform_device *pdev, > - struct miphy365x_phy *phy, u8 port) > +static int miphy365x_get_base_addr_one(struct platform_device *pdev, > + struct miphy365x *phy, > + struct device_node *child, > + int index) > { > - struct resource *res; > - char type[6]; > + void __iomem *base; > + const char *name; > + int ret; > > - sprintf(type, "sata%d", port); > + base = of_iomap(child, index); > + if (!base) { > + dev_err(&pdev->dev, "Failed to map %s\n", child->full_name); > + return -EINVAL; > + } > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, type); > - if (!res) > - return -ENODEV; > + ret = of_property_read_string_index(child, "reg-names", index, &name); > + if (ret) { > + dev_err(&pdev->dev, > + "%s: no reg-names property not found\n", child->name); > + return ret; > + } > > - phy->sata = devm_ioremap(&pdev->dev, res->start, resource_size(res)); > - if (!phy->sata) > - return -ENOMEM; > + if (!strncmp(name, "sata", 4)) > + phy->sata = base; > + else if (!strncmp(name, "pcie", 4)) > + phy->pcie = base; > + else { > + dev_err(&pdev->dev, "reg-names in %s not sata or pcie: %s", > + child->name, name); > + return -EINVAL; > + } > > - sprintf(type, "pcie%d", port); > + return 0; > +} > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, type); > - if (!res) > - return -ENODEV; > +static int miphy365x_get_base_addr(struct platform_device *pdev, > + struct miphy365x *phy, > + struct device_node *child) > +{ > + int index; > + int ret; > > - phy->pcie = devm_ioremap(&pdev->dev, res->start, resource_size(res)); > - if (!phy->pcie) > - return -ENOMEM; > + /* Each port handles SATA or PCIE. */ > + for (index = 0; index < 2; index++) { > + ret = miphy365x_get_base_addr_one(pdev, phy, > + child, index); > + if (ret) > + return ret; > + } > > return 0; > } > > -static int miphy365x_of_probe(struct device_node *np, > +static int miphy365x_of_probe(struct platform_device *pdev, > struct miphy365x_dev *phy_dev) > { > + struct device_node *np = pdev->dev.of_node; > + struct device_node *child; > + int child_count = 0; > + > + for_each_child_of_node(np, child) > + child_count++; use of_get_child_count() instead. > + > + if (child_count != ARRAY_SIZE(ports)) { > + dev_err(&pdev->dev, "%d ports supported, %d supplied\n", > + ARRAY_SIZE(ports), child_count); > + return -EINVAL; > + } > + > phy_dev->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); > if (IS_ERR(phy_dev->regmap)) { > dev_err(phy_dev->dev, "No syscfg phandle specified\n"); > @@ -556,11 +600,10 @@ static int miphy365x_of_probe(struct device_node *np, > > static int miphy365x_probe(struct platform_device *pdev) > { > - struct device_node *np = pdev->dev.of_node; > + struct device_node *child, *np = pdev->dev.of_node; > struct miphy365x_dev *phy_dev; > struct device *dev = &pdev->dev; > struct phy_provider *provider; > - u8 port; > int ret; > > if (!np) { > @@ -572,7 +615,7 @@ static int miphy365x_probe(struct platform_device *pdev) > if (!phy_dev) > return -ENOMEM; > > - ret = miphy365x_of_probe(np, phy_dev); > + ret = miphy365x_of_probe(pdev, phy_dev); > if (ret) > return ret; > > @@ -582,8 +625,9 @@ static int miphy365x_probe(struct platform_device *pdev) > > mutex_init(&phy_dev->miphy_mutex); > > - for (port = 0; port < ARRAY_SIZE(ports); port++) { > + for_each_child_of_node(np, child) { > struct phy *phy; > + static int port = 0; > > phy = devm_phy_create(dev, &miphy365x_ops, NULL); > if (IS_ERR(phy)) { > @@ -591,15 +635,17 @@ static int miphy365x_probe(struct platform_device *pdev) > return PTR_ERR(phy); > } > > - phy_dev->phys[port].phy = phy; > - phy_dev->phys[port].port = port; > - > - ret = miphy365x_get_base_addr(pdev, > - &phy_dev->phys[port], port); > + ret = miphy365x_get_base_addr(pdev, &phy_dev->phys[port], > + child); > if (ret) > return ret; > > + phy_dev->phys[port].phy = phy; > + phy_dev->phys[port].port = port; > + phy_dev->phys[port].enabled = !!of_device_is_available(child); > + > phy_set_drvdata(phy, &phy_dev->phys[port]); > + port++; > } > > provider = devm_of_phy_provider_register(dev, miphy365x_xlate); I think you can merge this to your original patch. Cheers Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> On Monday 30 June 2014 08:11 PM, Lee Jones wrote: > > phy: miphy365x: Represent each PHY channel as a DT subnode > > > > This has the added advantages of being able to enable/disable each > > of the channels as simply as enabling/disabling the DT node. > > > > Suggested-by: Kishon Vijay Abraham I <kishon@ti.com> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > diff --git a/drivers/phy/phy-miphy365x.c b/drivers/phy/phy-miphy365x.c > > index 1109f42..2c4ea6e 100644 > > --- a/drivers/phy/phy-miphy365x.c > > +++ b/drivers/phy/phy-miphy365x.c [...] > > -static int miphy365x_of_probe(struct device_node *np, > > +static int miphy365x_of_probe(struct platform_device *pdev, > > struct miphy365x_dev *phy_dev) > > { > > + struct device_node *np = pdev->dev.of_node; > > + struct device_node *child; > > + int child_count = 0; > > + > > + for_each_child_of_node(np, child) > > + child_count++; > > use of_get_child_count() instead. Ah, nice. I'll do that. [...] > I think you can merge this to your original patch. I can do that, but I thought It'd be nice to keep some history and show the migration over to a different setup. This is particularly important for when we back-port the changes back into the internal development kernel.
On Wednesday 02 July 2014 05:04 PM, Lee Jones wrote: >> On Monday 30 June 2014 08:11 PM, Lee Jones wrote: >>> phy: miphy365x: Represent each PHY channel as a DT subnode >>> >>> This has the added advantages of being able to enable/disable each >>> of the channels as simply as enabling/disabling the DT node. >>> >>> Suggested-by: Kishon Vijay Abraham I <kishon@ti.com> >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>> >>> diff --git a/drivers/phy/phy-miphy365x.c b/drivers/phy/phy-miphy365x.c >>> index 1109f42..2c4ea6e 100644 >>> --- a/drivers/phy/phy-miphy365x.c >>> +++ b/drivers/phy/phy-miphy365x.c > > [...] > >>> -static int miphy365x_of_probe(struct device_node *np, >>> +static int miphy365x_of_probe(struct platform_device *pdev, >>> struct miphy365x_dev *phy_dev) >>> { >>> + struct device_node *np = pdev->dev.of_node; >>> + struct device_node *child; >>> + int child_count = 0; >>> + >>> + for_each_child_of_node(np, child) >>> + child_count++; >> >> use of_get_child_count() instead. > > Ah, nice. I'll do that. > > [...] > >> I think you can merge this to your original patch. > > I can do that, but I thought It'd be nice to keep some history and > show the migration over to a different setup. This is particularly > important for when we back-port the changes back into the internal > development kernel. cool.. i'm fine with it. Cheers Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, 02 Jul 2014, Kishon Vijay Abraham I wrote: > On Wednesday 02 July 2014 05:04 PM, Lee Jones wrote: > >> On Monday 30 June 2014 08:11 PM, Lee Jones wrote: > >>> phy: miphy365x: Represent each PHY channel as a DT subnode > >>> > >>> This has the added advantages of being able to enable/disable each > >>> of the channels as simply as enabling/disabling the DT node. > >>> > >>> Suggested-by: Kishon Vijay Abraham I <kishon@ti.com> > >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> > >>> > >>> diff --git a/drivers/phy/phy-miphy365x.c b/drivers/phy/phy-miphy365x.c > >>> index 1109f42..2c4ea6e 100644 > >>> --- a/drivers/phy/phy-miphy365x.c > >>> +++ b/drivers/phy/phy-miphy365x.c > > > > [...] > > > >>> -static int miphy365x_of_probe(struct device_node *np, > >>> +static int miphy365x_of_probe(struct platform_device *pdev, > >>> struct miphy365x_dev *phy_dev) > >>> { > >>> + struct device_node *np = pdev->dev.of_node; > >>> + struct device_node *child; > >>> + int child_count = 0; > >>> + > >>> + for_each_child_of_node(np, child) > >>> + child_count++; > >> > >> use of_get_child_count() instead. > > > > Ah, nice. I'll do that. > > > > [...] > > > >> I think you can merge this to your original patch. > > > > I can do that, but I thought It'd be nice to keep some history and > > show the migration over to a different setup. This is particularly > > important for when we back-port the changes back into the internal > > development kernel. > > cool.. i'm fine with it. Thanks.
diff --git a/drivers/phy/phy-miphy365x.c b/drivers/phy/phy-miphy365x.c index 1109f42..2c4ea6e 100644 --- a/drivers/phy/phy-miphy365x.c +++ b/drivers/phy/phy-miphy365x.c @@ -17,6 +17,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_platform.h> +#include <linux/of_address.h> #include <linux/clk.h> #include <linux/phy/phy.h> #include <linux/delay.h> @@ -144,6 +145,7 @@ struct miphy365x { void __iomem *pcie; u8 type; u8 port; + bool enabled; }; struct miphy365x_dev { @@ -468,7 +470,7 @@ static struct phy *miphy365x_xlate(struct device *dev, struct miphy365x_dev *state = dev_get_drvdata(dev); u8 port, type; - if (args->count != 2) { + if (args->args_count != 2) { dev_err(dev, "Invalid number of cells in 'phy' property\n"); return ERR_PTR(-EINVAL); } @@ -484,6 +486,11 @@ static struct phy *miphy365x_xlate(struct device *dev, if (WARN_ON(port >= ARRAY_SIZE(ports))) return ERR_PTR(-EINVAL); + if (!state->phys[port].enabled) { + dev_warn(dev, "PHY port %d is disabled\n", port); + return ERR_PTR(-EINVAL); + } + if (type == MIPHY_TYPE_SATA) state->phys[port].base = state->phys[port].sata; else if (type == MIPHY_TYPE_PCIE) @@ -503,38 +510,75 @@ static struct phy_ops miphy365x_ops = { .owner = THIS_MODULE, }; -static int miphy365x_get_base_addr(struct platform_device *pdev, - struct miphy365x_phy *phy, u8 port) +static int miphy365x_get_base_addr_one(struct platform_device *pdev, + struct miphy365x *phy, + struct device_node *child, + int index) { - struct resource *res; - char type[6]; + void __iomem *base; + const char *name; + int ret; - sprintf(type, "sata%d", port); + base = of_iomap(child, index); + if (!base) { + dev_err(&pdev->dev, "Failed to map %s\n", child->full_name); + return -EINVAL; + } - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, type); - if (!res) - return -ENODEV; + ret = of_property_read_string_index(child, "reg-names", index, &name); + if (ret) { + dev_err(&pdev->dev, + "%s: no reg-names property not found\n", child->name); + return ret; + } - phy->sata = devm_ioremap(&pdev->dev, res->start, resource_size(res)); - if (!phy->sata) - return -ENOMEM; + if (!strncmp(name, "sata", 4)) + phy->sata = base; + else if (!strncmp(name, "pcie", 4)) + phy->pcie = base; + else { + dev_err(&pdev->dev, "reg-names in %s not sata or pcie: %s", + child->name, name); + return -EINVAL; + } - sprintf(type, "pcie%d", port); + return 0; +} - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, type); - if (!res) - return -ENODEV; +static int miphy365x_get_base_addr(struct platform_device *pdev, + struct miphy365x *phy, + struct device_node *child) +{ + int index; + int ret; - phy->pcie = devm_ioremap(&pdev->dev, res->start, resource_size(res)); - if (!phy->pcie) - return -ENOMEM; + /* Each port handles SATA or PCIE. */ + for (index = 0; index < 2; index++) { + ret = miphy365x_get_base_addr_one(pdev, phy, + child, index); + if (ret) + return ret; + } return 0; } -static int miphy365x_of_probe(struct device_node *np, +static int miphy365x_of_probe(struct platform_device *pdev, struct miphy365x_dev *phy_dev) { + struct device_node *np = pdev->dev.of_node; + struct device_node *child; + int child_count = 0; + + for_each_child_of_node(np, child) + child_count++; + + if (child_count != ARRAY_SIZE(ports)) { + dev_err(&pdev->dev, "%d ports supported, %d supplied\n", + ARRAY_SIZE(ports), child_count); + return -EINVAL; + } + phy_dev->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); if (IS_ERR(phy_dev->regmap)) { dev_err(phy_dev->dev, "No syscfg phandle specified\n"); @@ -556,11 +600,10 @@ static int miphy365x_of_probe(struct device_node *np, static int miphy365x_probe(struct platform_device *pdev) { - struct device_node *np = pdev->dev.of_node; + struct device_node *child, *np = pdev->dev.of_node; struct miphy365x_dev *phy_dev; struct device *dev = &pdev->dev; struct phy_provider *provider; - u8 port; int ret; if (!np) { @@ -572,7 +615,7 @@ static int miphy365x_probe(struct platform_device *pdev) if (!phy_dev) return -ENOMEM; - ret = miphy365x_of_probe(np, phy_dev); + ret = miphy365x_of_probe(pdev, phy_dev); if (ret) return ret; @@ -582,8 +625,9 @@ static int miphy365x_probe(struct platform_device *pdev) mutex_init(&phy_dev->miphy_mutex); - for (port = 0; port < ARRAY_SIZE(ports); port++) { + for_each_child_of_node(np, child) { struct phy *phy; + static int port = 0; phy = devm_phy_create(dev, &miphy365x_ops, NULL); if (IS_ERR(phy)) { @@ -591,15 +635,17 @@ static int miphy365x_probe(struct platform_device *pdev) return PTR_ERR(phy); } - phy_dev->phys[port].phy = phy; - phy_dev->phys[port].port = port; - - ret = miphy365x_get_base_addr(pdev, - &phy_dev->phys[port], port); + ret = miphy365x_get_base_addr(pdev, &phy_dev->phys[port], + child); if (ret) return ret; + phy_dev->phys[port].phy = phy; + phy_dev->phys[port].port = port; + phy_dev->phys[port].enabled = !!of_device_is_available(child); + phy_set_drvdata(phy, &phy_dev->phys[port]); + port++; } provider = devm_of_phy_provider_register(dev, miphy365x_xlate);
phy: miphy365x: Represent each PHY channel as a DT subnode This has the added advantages of being able to enable/disable each of the channels as simply as enabling/disabling the DT node. Suggested-by: Kishon Vijay Abraham I <kishon@ti.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/