| Message ID | 1371482014-5244-3-git-send-email-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mon, Jun 17, 2013 at 05:13:34PM +0200, Sebastian Andrzej Siewior wrote: > There is no need to hardcode the number of instances here. It is better to > determine them at runtime. Even if the device provides two instances one > might only want to use one of them. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > drivers/usb/musb/musb_dsps.c | 33 +++++++++++++++++++++++---------- > 1 file changed, 23 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c > index d9ff390..0ac9934 100644 > --- a/drivers/usb/musb/musb_dsps.c > +++ b/drivers/usb/musb/musb_dsps.c > @@ -110,8 +110,6 @@ struct dsps_musb_wrapper { > /* miscellaneous stuff */ > u32 musb_core_offset; > u8 poll_seconds; > - /* number of musb instances */ > - u8 instances; > }; > > /** > @@ -124,6 +122,7 @@ struct dsps_glue { > struct timer_list timer[2]; /* otg_workaround timer */ > unsigned long last_timer[2]; /* last timer data for each instance */ > u32 __iomem *usb_ctrl[2]; > + u8 instances; > }; > > #define DSPS_AM33XX_CONTROL_MODULE_PHYS_0 0x44e10620 > @@ -646,6 +645,23 @@ static int dsps_probe(struct platform_device *pdev) > } > platform_set_drvdata(pdev, glue); > > + i = 1; > + do { > + iomem = platform_get_resource(pdev, IORESOURCE_MEM, i); IIRC this index starts at zero, no ? Meaning that this should be: i = 0 do { iomem = platform_get_resource(pdev, IORESOURCE_MEM, i); if (!iomem) break; i++; } while (true); glue->instances = i + 1; Also, why are you getting the resource and doing nothing with it ? Is it just to figure out the amount of instances ? Isn't there a DT helper to count how many child nodes a certain node has ?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 06/18/2013 10:31 AM, Felipe Balbi wrote: > Hi, Hi Felipe, >> --- a/drivers/usb/musb/musb_dsps.c +++ >> b/drivers/usb/musb/musb_dsps.c @@ -110,8 +110,6 @@ struct >> dsps_musb_wrapper { @@ -646,6 +645,23 @@ static int >> dsps_probe(struct platform_device *pdev) } >> platform_set_drvdata(pdev, glue); >> >> + i = 1; + do { + iomem = platform_get_resource(pdev, >> IORESOURCE_MEM, i); > > IIRC this index starts at zero, no ? Meaning that this should be: > > i = 0 do { iomem = platform_get_resource(pdev, IORESOURCE_MEM, i); > if (!iomem) break; > > i++; } while (true); > > glue->instances = i + 1; No. 3x ioadress i.e. 0, 1, 2 means two instances because we have 0 as the glue io address and two musb port (1 and 2). > Also, why are you getting the resource and doing nothing with it ? > Is it just to figure out the amount of instances ? Isn't there a DT > helper to exactly, just to count. > count how many child nodes a certain node has ? This isn't exactly a child node, is it? There is of_get_child_count() but this isn't a child, it is one property. Sebastian -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQIcBAEBCAAGBQJRwB3LAAoJEHuW6BYqjPXRu1MQALwY/DHJWIKBZm18OdlJizmC UZMfZCqwtObDtHcbUpMx5jFmF6jblVCY56Vx713oarRYwona65jRuCVp+BmPpvbh rH7Vfdj7ZK02sSXD49lu/eM+StOoLAEuCdZV5cP2sQCQHzGY50eDVXynjFTmr6wH nIC/MCPrTLGgH3bWLM1PshmIThbeovp/UupGXU0ABxThL3Dm2EAolBEWjvwAgbZy CUL3OoUvb6SdJiPkI4ORUvHzyS78JDsvEj1zuCio13m+/FW3uvvQEjIWefS/lAmP 0AyoDf+6VTdAhJVcEev5RJNJpIzFMZDQYjarHO8q5NSwr5WN+h+QOEQjYdQsAfkf Jv68nnj6mwE1HUv32EnTIu3il0101QU+dU4zDsMykCneMhY2GBXbRqjrfZXiTqKO WjtJg0HeKhIrG6i5ffLt/4kfPIyXYYJriOqJf2nvlBf7/RqsX4OT5ThIW2BAV7Zx 7qY9CgwDdSjyBq1GpcIGbpBNvzrVGxJfLxNec73EOJmHdu6zCZWLWpujAQIvv3kf XJWtPXYAVNB7lpSBaxJO6CeSE6YAy5CkxaL8EMeTcqKxwh/ntRw/YLt45MUb8Ao5 J82znl3hByO/9FNPi9KA4A6TDuFtRNNu5lm8o+PdQndTO6heMzJTs8AL/PqH1vRA q2e6Wxg65ZTs4aUUyVFF =f5YG -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Jun 18, 2013 at 10:43:55AM +0200, Sebastian Andrzej Siewior wrote: > >> --- a/drivers/usb/musb/musb_dsps.c +++ > >> b/drivers/usb/musb/musb_dsps.c @@ -110,8 +110,6 @@ struct > >> dsps_musb_wrapper { @@ -646,6 +645,23 @@ static int > >> dsps_probe(struct platform_device *pdev) } > >> platform_set_drvdata(pdev, glue); > >> > >> + i = 1; + do { + iomem = platform_get_resource(pdev, > >> IORESOURCE_MEM, i); > > > > IIRC this index starts at zero, no ? Meaning that this should be: > > > > i = 0 do { iomem = platform_get_resource(pdev, IORESOURCE_MEM, i); > > if (!iomem) break; > > > > i++; } while (true); > > > > glue->instances = i + 1; > > No. 3x ioadress i.e. 0, 1, 2 means two instances because we have 0 as > the glue io address and two musb port (1 and 2). heh, true. > > Also, why are you getting the resource and doing nothing with it ? > > Is it just to figure out the amount of instances ? Isn't there a DT > > helper to > > exactly, just to count. > > > count how many child nodes a certain node has ? > This isn't exactly a child node, is it? There is of_get_child_count() > but this isn't a child, it is one property. is it because we haven't added DTS support for musb core ? Eventually we can/should add and convert this to of_get_child_count() then. For now, we can go ahead with your approach. We have such a large amount of function pointers to sort out anyway :-(
On 06/18/2013 10:54 AM, Felipe Balbi wrote: > Hi, Hi Felipe, >> This isn't exactly a child node, is it? There is >> of_get_child_count() but this isn't a child, it is one property. > > is it because we haven't added DTS support for musb core ? > Eventually we can/should add and convert this to > of_get_child_count() then. For now, we can go ahead with your > approach. mother { child1 { }; child1 { }; }; That would be a child imho. For counting of phandles (for the number of assigned gpios for instance) you would use of_count_phandle_with_args(node, "gpios", "#gpio-cell-size"); We don't have an equivalent for "#gpio-cell-size". In our case it would be the sum of "#address-cells" and "#size-cells" minus 1 because we don't have a phandle and abuse a function :) Counting the number of "addresses" is special since bother its members (address and size) can be either 32bit or 64bit in size. > We have such a large amount of function pointers to sort out anyway > :-( If you want get the while() loop replaced by something else I have a few suggestions: - check for iomem(2). If it is there we have two instances. If not, just 1. This includes also the hope that we don't get a third port. - loop over of_address_to_resource(). This is what of_device_alloc() is doing: if (of_can_translate_address(np)) while (of_address_to_resource(np, num_reg, &temp_res) == 0) num_reg++; So it is different. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Jun 18, 2013 at 11:24:40AM +0200, Sebastian Andrzej Siewior wrote: > On 06/18/2013 10:54 AM, Felipe Balbi wrote: > > Hi, > > Hi Felipe, > > >> This isn't exactly a child node, is it? There is > >> of_get_child_count() but this isn't a child, it is one property. > > > > is it because we haven't added DTS support for musb core ? > > Eventually we can/should add and convert this to > > of_get_child_count() then. For now, we can go ahead with your > > approach. > > mother { > child1 { > }; > child1 { > }; > }; > > That would be a child imho. > For counting of phandles (for the number of assigned gpios for instance) > you would use > of_count_phandle_with_args(node, "gpios", "#gpio-cell-size"); > > We don't have an equivalent for "#gpio-cell-size". In our case it would > be the sum of "#address-cells" and "#size-cells" minus 1 because we > don't have a phandle and abuse a function :) > Counting the number of "addresses" is special since bother its > members (address and size) can be either 32bit or 64bit in size. > > > We have such a large amount of function pointers to sort out anyway > > :-( > > If you want get the while() loop replaced by something else I have a > few suggestions: > - check for iomem(2). If it is there we have two instances. If not, > just 1. This includes also the hope that we don't get a third port. > > - loop over of_address_to_resource(). This is what of_device_alloc() is > doing: > if (of_can_translate_address(np)) > while (of_address_to_resource(np, num_reg, &temp_res) == 0) > num_reg++; > So it is different. let's keep it as it is, I think once we convert musb-core to dt, all those issues will vanish and we will be able to use of_get_child_count() anyway. Forget my noise :-)
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c index d9ff390..0ac9934 100644 --- a/drivers/usb/musb/musb_dsps.c +++ b/drivers/usb/musb/musb_dsps.c @@ -110,8 +110,6 @@ struct dsps_musb_wrapper { /* miscellaneous stuff */ u32 musb_core_offset; u8 poll_seconds; - /* number of musb instances */ - u8 instances; }; /** @@ -124,6 +122,7 @@ struct dsps_glue { struct timer_list timer[2]; /* otg_workaround timer */ unsigned long last_timer[2]; /* last timer data for each instance */ u32 __iomem *usb_ctrl[2]; + u8 instances; }; #define DSPS_AM33XX_CONTROL_MODULE_PHYS_0 0x44e10620 @@ -646,6 +645,23 @@ static int dsps_probe(struct platform_device *pdev) } platform_set_drvdata(pdev, glue); + i = 1; + do { + iomem = platform_get_resource(pdev, IORESOURCE_MEM, i); + if (!iomem) { + i--; + break; + } + i++; + } while (1); + + glue->instances = i; + if (glue->instances < 1) { + dev_err(&pdev->dev, "Need atleast iomem for one port.\n"); + ret = -EINVAL; + goto err1_5; + } + /* enable the usbss clocks */ pm_runtime_enable(&pdev->dev); @@ -656,7 +672,7 @@ static int dsps_probe(struct platform_device *pdev) } /* create the child platform device for all instances of musb */ - for (i = 0; i < wrp->instances ; i++) { + for (i = 0; i < glue->instances; i++) { ret = dsps_create_musb_pdev(glue, i); if (ret != 0) { dev_err(&pdev->dev, "failed to create child pdev\n"); @@ -673,6 +689,7 @@ static int dsps_probe(struct platform_device *pdev) pm_runtime_put(&pdev->dev); err2: pm_runtime_disable(&pdev->dev); +err1_5: kfree(glue->wrp); err1: kfree(glue); @@ -682,11 +699,10 @@ static int dsps_probe(struct platform_device *pdev) static int dsps_remove(struct platform_device *pdev) { struct dsps_glue *glue = platform_get_drvdata(pdev); - const struct dsps_musb_wrapper *wrp = glue->wrp; int i; /* delete the child platform device */ - for (i = 0; i < wrp->instances ; i++) + for (i = 0; i < glue->instances; i++) platform_device_unregister(glue->musb[i]); /* disable usbss clocks */ @@ -702,10 +718,9 @@ static int dsps_suspend(struct device *dev) { struct platform_device *pdev = to_platform_device(dev->parent); struct dsps_glue *glue = platform_get_drvdata(pdev); - const struct dsps_musb_wrapper *wrp = glue->wrp; int i; - for (i = 0; i < wrp->instances; i++) + for (i = 0; i < glue->instances; i++) musb_dsps_phy_control(glue, i, 0); return 0; @@ -715,10 +730,9 @@ static int dsps_resume(struct device *dev) { struct platform_device *pdev = to_platform_device(dev->parent); struct dsps_glue *glue = platform_get_drvdata(pdev); - const struct dsps_musb_wrapper *wrp = glue->wrp; int i; - for (i = 0; i < wrp->instances; i++) + for (i = 0; i < glue->instances; i++) musb_dsps_phy_control(glue, i, 1); return 0; @@ -755,7 +769,6 @@ static const struct dsps_musb_wrapper ti81xx_driver_data = { .rxep_bitmap = (0xfffe << 16), .musb_core_offset = 0x400, .poll_seconds = 2, - .instances = 1, }; static const struct platform_device_id musb_dsps_id_table[] = {
There is no need to hardcode the number of instances here. It is better to determine them at runtime. Even if the device provides two instances one might only want to use one of them. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/usb/musb/musb_dsps.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)