| Message ID | 1368173847-5661-18-git-send-email-wsa@the-dreams.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 10-05-2013 12:17, Wolfram Sang wrote: > devm_ioremap_resource does sanity checks on the given resource. No need to > duplicate this in the driver. > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 05/10/2013 03:07 PM, Sergei Shtylyov wrote: > >> devm_ioremap_resource does sanity checks on the given resource. No >> need to >> duplicate this in the driver. > >> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > > Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> I was too fast, I'm now withdawing my ACK. > WBR, Sergei > WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 05/10/2013 12:17 PM, Wolfram Sang wrote: > devm_ioremap_resource does sanity checks on the given resource. No need to > duplicate this in the driver. > > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > --- > drivers/net/ethernet/renesas/sh_eth.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > index 33dc6f2..6175839 100644 > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c > @@ -2661,14 +2661,6 @@ static int sh_eth_drv_probe(struct platform_device *pdev) > struct sh_eth_private *mdp = NULL; > struct sh_eth_plat_data *pd = pdev->dev.platform_data; > > - /* get base addr */ > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (unlikely(res == NULL)) { > - dev_err(&pdev->dev, "invalid resource\n"); > - ret = -EINVAL; > - goto out; > - } > - > ndev = alloc_etherdev(sizeof(struct sh_eth_private)); > if (!ndev) { > ret = -ENOMEM; There's ndev->base_addr = res->start; below this which you broke. NAK. You should really compile-test your patches. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> You should really compile-test your patches.
Easier said than done with a cleanup series touching various platforms
and architectures. If there are already solutions how to find a .config
compiling the source file modified, I am all ears.
That being said, the actual mistake I did was embarassing, I agree. I am
sorry for that and will improve. And, doing things means doing mistakes,
too.
Regards,
Wolfram
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 12-05-2013 13:42, Wolfram Sang wrote: >> You should really compile-test your patches. > Easier said than done with a cleanup series touching various platforms > and architectures. If there are already solutions how to find a .config > compiling the source file modified, I am all ears. You can always try to do: $ make drivers/net/ethernet/renesas/sh_eth.o with a .config file you have. It will compile a file regardless of whether it's enabled in your .config or not... > That being said, the actual mistake I did was embarassing, I agree. I am > sorry for that and will improve. And, doing things means doing mistakes, > too. Be more attentive in the future please. :-) > Regards, > Wolfram WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > You can always try to do: > > $ make drivers/net/ethernet/renesas/sh_eth.o > > with a .config file you have. It will compile a file regardless of > whether it's enabled in your .config or not... Been there. This mainly works for platform independent drivers. In this case: CC drivers/net/ethernet/renesas/sh_eth.o drivers/net/ethernet/renesas/sh_eth.c: In function 'sh_eth_dev_init': drivers/net/ethernet/renesas/sh_eth.c:1274:2: error: implicit declaration of function 'sh_eth_reset' [-Werror=implicit-function-declaration] drivers/net/ethernet/renesas/sh_eth.c: In function 'sh_eth_drv_probe': drivers/net/ethernet/renesas/sh_eth.c:2724:13: error: 'sh_eth_my_cpu_data' undeclared (first use in this function) drivers/net/ethernet/renesas/sh_eth.c:2724:13: note: each undeclared identifier is reported only once for each function it appears in drivers/net/ethernet/renesas/sh_eth.c: At top level: drivers/net/ethernet/renesas/sh_eth.c:889:12: warning: 'sh_eth_check_reset' defined but not used [-Wunused-function] > >That being said, the actual mistake I did was embarassing, I agree. I am > >sorry for that and will improve. And, doing things means doing mistakes, > >too. > > Be more attentive in the future please. :-) Sure thing. Regards, Wolfram -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 05/12/2013 08:32 PM, Wolfram Sang wrote: > >> You can always try to do: >> >> $ make drivers/net/ethernet/renesas/sh_eth.o >> >> with a .config file you have. It will compile a file regardless of >> whether it's enabled in your .config or not... > Been there. This mainly works for platform independent drivers. In this case: > > CC drivers/net/ethernet/renesas/sh_eth.o > drivers/net/ethernet/renesas/sh_eth.c: In function 'sh_eth_dev_init': > drivers/net/ethernet/renesas/sh_eth.c:1274:2: error: implicit declaration of function 'sh_eth_reset' [-Werror=implicit-function-declaration] > drivers/net/ethernet/renesas/sh_eth.c: In function 'sh_eth_drv_probe': > drivers/net/ethernet/renesas/sh_eth.c:2724:13: error: 'sh_eth_my_cpu_data' undeclared (first use in this function) > drivers/net/ethernet/renesas/sh_eth.c:2724:13: note: each undeclared identifier is reported only once for each function it appears in > drivers/net/ethernet/renesas/sh_eth.c: At top level: > drivers/net/ethernet/renesas/sh_eth.c:889:12: warning: 'sh_eth_check_reset' defined but not used [-Wunused-function] At least you can try to filter out errors/warning caused by your patch from those that are caused by platform dependency. Concerning this driver, hopefully the platform dependencies will go away in 3.11. > Regards, > > Wolfram > WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c index 33dc6f2..6175839 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c @@ -2661,14 +2661,6 @@ static int sh_eth_drv_probe(struct platform_device *pdev) struct sh_eth_private *mdp = NULL; struct sh_eth_plat_data *pd = pdev->dev.platform_data; - /* get base addr */ - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (unlikely(res == NULL)) { - dev_err(&pdev->dev, "invalid resource\n"); - ret = -EINVAL; - goto out; - } - ndev = alloc_etherdev(sizeof(struct sh_eth_private)); if (!ndev) { ret = -ENOMEM; @@ -2697,6 +2689,7 @@ static int sh_eth_drv_probe(struct platform_device *pdev) mdp = netdev_priv(ndev); mdp->num_tx_ring = TX_RING_SIZE; mdp->num_rx_ring = RX_RING_SIZE; + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); mdp->addr = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(mdp->addr)) { ret = PTR_ERR(mdp->addr); @@ -2745,11 +2738,6 @@ static int sh_eth_drv_probe(struct platform_device *pdev) if (mdp->cd->tsu) { struct resource *rtsu; rtsu = platform_get_resource(pdev, IORESOURCE_MEM, 1); - if (!rtsu) { - dev_err(&pdev->dev, "Not found TSU resource\n"); - ret = -ENODEV; - goto out_release; - } mdp->tsu_addr = devm_ioremap_resource(&pdev->dev, rtsu); if (IS_ERR(mdp->tsu_addr)) { ret = PTR_ERR(mdp->tsu_addr);
devm_ioremap_resource does sanity checks on the given resource. No need to duplicate this in the driver. Signed-off-by: Wolfram Sang <wsa@the-dreams.de> --- drivers/net/ethernet/renesas/sh_eth.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)