| Message ID | 1328004002-24646-2-git-send-email-Barry.Song@csr.com |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 31, 2012 at 06:00:00PM +0800, Barry Song wrote: > From: Barry Song <Baohua.Song@csr.com> > > this patch helps to move the common pattern from > > " > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) { > ret = -ENODEV; > goto err; > } You don't need to do the error checking for 'res'. You can simply do res = platform_get_resource(pdev, IORESOURCE_MEM, 0); base = devm_request_and_ioremap(&dev->dev, res); devm_request_and_ioremap() will check res. Given that, I don't think we can save a lot with another wrapper. Thanks, Wolfram
2012/1/31 Wolfram Sang <w.sang@pengutronix.de>: > On Tue, Jan 31, 2012 at 06:00:00PM +0800, Barry Song wrote: >> From: Barry Song <Baohua.Song@csr.com> >> >> this patch helps to move the common pattern from >> >> " >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> if (!res) { >> ret = -ENODEV; >> goto err; >> } > > You don't need to do the error checking for 'res'. You can simply do > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > base = devm_request_and_ioremap(&dev->dev, res); i do know devm_request_and_ioremap() does res checking. but that is implicit, confused and not a smart way actually. actually, no people by now really use the implicit checking. that shows people don't really think that is a good programming way. > > devm_request_and_ioremap() will check res. Given that, I don't think > we can save a lot with another wrapper. i think we can save some. The story begins from Grant's feedback in: http://www.spinics.net/lists/arm-kernel/msg157644.html > > Thanks, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | -barry
Barry, > > You don't need to do the error checking for 'res'. You can simply do > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > base = devm_request_and_ioremap(&dev->dev, res); > > i do know devm_request_and_ioremap() does res checking. but that is > implicit, confused and not a smart way actually. I agree about the implicit thing (keep in mind the function is new). But calling a function "not smart" because it checks its arguments? I do like the NULL check of kfree() for example. > actually, no people by now really use the implicit checking. that > shows people don't really think that is a good programming way. I'd think most people just copy&paste and don't have an opinion either way, so the quantity doesn't show much. > > devm_request_and_ioremap() will check res. Given that, I don't think > > we can save a lot with another wrapper. > > i think we can save some. > The story begins from Grant's feedback in: > http://www.spinics.net/lists/arm-kernel/msg157644.html I am not sure using 'platform_devm_request_and_ioremap' and later using plain 'devm_*' functions (without platform_-prefix) is less confusing. The alternative would be to check which helper functions also use 'struct resource' and if they do checks on that. If all do that, you would have the simple rule, that you only need to check yourself if you access it yourself. Regards, Wolfram
hi Wolfram, 2012/1/31 Wolfram Sang <w.sang@pengutronix.de>: > Barry, > >> > You don't need to do the error checking for 'res'. You can simply do >> > >> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> > base = devm_request_and_ioremap(&dev->dev, res); >> >> i do know devm_request_and_ioremap() does res checking. but that is >> implicit, confused and not a smart way actually. > > I agree about the implicit thing (keep in mind the function is new). But > calling a function "not smart" because it checks its arguments? I do > like the NULL check of kfree() for example. i didn't mean it is not smart for devm_request_and_ioremap() to check its param. that is definitely ok. i mean it is not smart for people to depend on another function to do implicit check on what they should check by themselves. > >> actually, no people by now really use the implicit checking. that >> shows people don't really think that is a good programming way. > > I'd think most people just copy&paste and don't have an opinion either > way, so the quantity doesn't show much. > >> > devm_request_and_ioremap() will check res. Given that, I don't think >> > we can save a lot with another wrapper. >> >> i think we can save some. >> The story begins from Grant's feedback in: >> http://www.spinics.net/lists/arm-kernel/msg157644.html > > I am not sure using 'platform_devm_request_and_ioremap' and later using > plain 'devm_*' functions (without platform_-prefix) is less confusing. > The alternative would be to check which helper functions also use > 'struct resource' and if they do checks on that. If all do that, you > would have the simple rule, that you only need to check yourself if you > access it yourself. it is just a helper, just like clk_disable_unprepare() which do clk_disable() + clk_unprepare(). > > Regards, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | -barry
On Tue, Jan 31, 2012 at 12:33:58PM +0100, Wolfram Sang wrote: > Barry, > > > > You don't need to do the error checking for 'res'. You can simply do > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > base = devm_request_and_ioremap(&dev->dev, res); > > > > i do know devm_request_and_ioremap() does res checking. but that is > > implicit, confused and not a smart way actually. > > I agree about the implicit thing (keep in mind the function is new). But > calling a function "not smart" because it checks its arguments? I do > like the NULL check of kfree() for example. > > > actually, no people by now really use the implicit checking. that > > shows people don't really think that is a good programming way. > > I'd think most people just copy&paste and don't have an opinion either > way, so the quantity doesn't show much. > > > > devm_request_and_ioremap() will check res. Given that, I don't think > > > we can save a lot with another wrapper. > > > > i think we can save some. > > The story begins from Grant's feedback in: > > http://www.spinics.net/lists/arm-kernel/msg157644.html > > I am not sure using 'platform_devm_request_and_ioremap' and later using > plain 'devm_*' functions (without platform_-prefix) is less confusing. > The alternative would be to check which helper functions also use > 'struct resource' and if they do checks on that. If all do that, you > would have the simple rule, that you only need to check yourself if you > access it yourself. The reason I suggested the wrapper is that then the driver code doesn't need to fart around with the res pointer at all. It reduces boilerplate in platform drivers which I think is a good thing. g.
On Tue, Jan 31, 2012 at 06:00:00PM +0800, Barry Song wrote: > From: Barry Song <Baohua.Song@csr.com> > > this patch helps to move the common pattern from > > " > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) { > ret = -ENODEV; > goto err; > } > > base = devm_request_and_ioremap(&dev->dev, mem_res); > if (!base) { > ret = -ENODEV; > goto err; > } > " > > to > > " > base = platform_devm_request_and_ioremap(pdev, 0); > if (!base) { > ret = -ENODEV; > goto err; > } > " > > Signed-off-by: Barry Song <Baohua.Song@csr.com> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Erik Gilling <konkers@google.com> > Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp> > Cc: David Woodhouse <dwmw2@infradead.org> Acked-by: Grant Likely <grant.likely@secretlab.ca> > --- > drivers/base/platform.c | 19 +++++++++++++++++++ > include/linux/platform_device.h | 1 + > 2 files changed, 20 insertions(+), 0 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index f0c605e..39ca0ab 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -72,6 +72,25 @@ struct resource *platform_get_resource(struct platform_device *dev, > EXPORT_SYMBOL_GPL(platform_get_resource); > > /** > + * platform_devm_request_and_ioremap() - get resource, check, request region, > + * and ioremap resource > + * @dev: platform device > + * @num: IOMEM resource index > + */ > +void __iomem *platform_devm_request_and_ioremap(struct platform_device *dev, > + unsigned int num) > +{ > + struct resource *res; > + > + res = platform_get_resource(dev, IORESOURCE_MEM, num); > + if (!res) > + return NULL; > + > + return devm_request_and_ioremap(&dev->dev, res); > +} > +EXPORT_SYMBOL_GPL(platform_devm_request_and_ioremap); > + > +/** > * platform_get_irq - get an IRQ for a device > * @dev: platform device > * @num: IRQ number index > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 60e9994..768c0d7 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -44,6 +44,7 @@ extern struct device platform_bus; > > extern void arch_setup_pdev_archdata(struct platform_device *); > extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int); > +extern void __iomem *platform_devm_request_and_ioremap(struct platform_device *, unsigned int); > extern int platform_get_irq(struct platform_device *, unsigned int); > extern struct resource *platform_get_resource_byname(struct platform_device *, unsigned int, const char *); > extern int platform_get_irq_byname(struct platform_device *, const char *); > -- > 1.7.1 > > > > Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom > More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog
> > I am not sure using 'platform_devm_request_and_ioremap' and later using > > plain 'devm_*' functions (without platform_-prefix) is less confusing. > > The alternative would be to check which helper functions also use > > 'struct resource' and if they do checks on that. If all do that, you > > would have the simple rule, that you only need to check yourself if you > > access it yourself. > > The reason I suggested the wrapper is that then the driver code doesn't need > to fart around with the res pointer at all. It reduces boilerplate in platform > drivers which I think is a good thing. I do understand your motivation and fully agree with what you are aiming for (that's exactly why I implemented devm_request_and_ioremap()). This patch is a micro-optimization, though, and won't cut it IMHO. I still have issues with only one platform_devm_* and all the rest being devm_* (without platform_). Things might look better, if we'd for example also have platform_devm_request_irq() or something similar. That might be an approach where we can play around with and see what is left to do. Or, if other approaches might be more elegant. To discuss that, try things, etc, I'd simply like to have a bit more time. If we are accepting the first iteration right away, and people let run their coccinelle-scripts based on that, it might get annoying to change that a second time, I'd think. Regards, Wolfram
On Tue, Jan 31, 2012 at 11:00 AM, Barry Song <Barry.Song@csr.com> wrote: > From: Barry Song <Baohua.Song@csr.com> > > this patch helps to move the common pattern from Good idea! > @@ -44,6 +44,7 @@ extern struct device platform_bus; > > extern void arch_setup_pdev_archdata(struct platform_device *); > extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int); > +extern void __iomem *platform_devm_request_and_ioremap(struct platform_device *, unsigned int); But don't you want a reverse call for the module remove() function? Like: extern void __iomem *platform_devm_iounmap_and_free(struct platform_device *, unsigned int); ? Yours, Linus Walleij
On Tue, Jan 31, 2012 at 2:15 PM, Wolfram Sang <w.sang@pengutronix.de> wrote: > >> > I am not sure using 'platform_devm_request_and_ioremap' and later using >> > plain 'devm_*' functions (without platform_-prefix) is less confusing. >> > The alternative would be to check which helper functions also use >> > 'struct resource' and if they do checks on that. If all do that, you >> > would have the simple rule, that you only need to check yourself if you >> > access it yourself. >> >> The reason I suggested the wrapper is that then the driver code doesn't need >> to fart around with the res pointer at all. It reduces boilerplate in platform >> drivers which I think is a good thing. > > I do understand your motivation and fully agree with what you are aiming for > (that's exactly why I implemented devm_request_and_ioremap()). > > This patch is a micro-optimization, though, and won't cut it IMHO. I still have > issues with only one platform_devm_* and all the rest being devm_* (without > platform_). Things might look better, if we'd for example also have > platform_devm_request_irq() or something similar. That might be an approach > where we can play around with and see what is left to do. Or, if other > approaches might be more elegant. > > To discuss that, try things, etc, I'd simply like to have a bit more time. If > we are accepting the first iteration right away, and people let run their > coccinelle-scripts based on that, it might get annoying to change that a second > time, I'd think. Okay, I'm willing to sit-tight on this for a bit. g.
On Tue, Jan 31, 2012 at 2:20 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Jan 31, 2012 at 11:00 AM, Barry Song <Barry.Song@csr.com> wrote: > >> From: Barry Song <Baohua.Song@csr.com> >> >> this patch helps to move the common pattern from > > Good idea! > >> @@ -44,6 +44,7 @@ extern struct device platform_bus; >> >> extern void arch_setup_pdev_archdata(struct platform_device *); >> extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int); >> +extern void __iomem *platform_devm_request_and_ioremap(struct platform_device *, unsigned int); > > But don't you want a reverse call for the module remove() function? > > Like: > > extern void __iomem *platform_devm_iounmap_and_free(struct > platform_device *, unsigned int); I wouldn't because it is exactly the same as the normal unwind call. I don't think the name redirection buys us anything here. g.
hi Wolfram, 2012/2/1 Wolfram Sang <w.sang@pengutronix.de>: > >> > I am not sure using 'platform_devm_request_and_ioremap' and later using >> > plain 'devm_*' functions (without platform_-prefix) is less confusing. >> > The alternative would be to check which helper functions also use >> > 'struct resource' and if they do checks on that. If all do that, you >> > would have the simple rule, that you only need to check yourself if you >> > access it yourself. >> >> The reason I suggested the wrapper is that then the driver code doesn't need >> to fart around with the res pointer at all. It reduces boilerplate in platform >> drivers which I think is a good thing. > > I do understand your motivation and fully agree with what you are aiming for > (that's exactly why I implemented devm_request_and_ioremap()). > > This patch is a micro-optimization, though, and won't cut it IMHO. I still have > issues with only one platform_devm_* and all the rest being devm_* (without > platform_). Things might look better, if we'd for example also have > platform_devm_request_irq() or something similar. That might be an approach > where we can play around with and see what is left to do. Or, if other > approaches might be more elegant. > > To discuss that, try things, etc, I'd simply like to have a bit more time. If > we are accepting the first iteration right away, and people let run their > coccinelle-scripts based on that, it might get annoying to change that a second > time, I'd think. > i think it makes sense for us to have platform_devm_request_irq() and similar things. i'll take care. PS: as the man maintaining embedded systems of the i2c subsystem, would you comment the CSR i2c driver which has been there waiting for a long time? http://www.spinics.net/lists/linux-i2c/msg07081.html > Regards, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | Thanks barry
Linus, 2012/2/1 Linus Walleij <linus.walleij@linaro.org>: > On Tue, Jan 31, 2012 at 11:00 AM, Barry Song <Barry.Song@csr.com> wrote: > >> From: Barry Song <Baohua.Song@csr.com> >> >> this patch helps to move the common pattern from > > Good idea! > >> @@ -44,6 +44,7 @@ extern struct device platform_bus; >> >> extern void arch_setup_pdev_archdata(struct platform_device *); >> extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int); >> +extern void __iomem *platform_devm_request_and_ioremap(struct platform_device *, unsigned int); > > But don't you want a reverse call for the module remove() function? > > Like: > > extern void __iomem *platform_devm_iounmap_and_free(struct > platform_device *, unsigned int); that point is we have the garbage collection mechanism by calling devm_ function families. everything is undone on driver detach. so we will not have the free and unmap things. > > ? > > Yours, > Linus Walleij -barry
> i think it makes sense for us to have platform_devm_request_irq() and > similar things. i'll take care. I'll try, too. Then we can compare and learn from that. > PS: as the man maintaining embedded systems of the i2c subsystem, > would you comment the CSR i2c driver which has been there waiting for > a long time? > http://www.spinics.net/lists/linux-i2c/msg07081.html Relax, you sent that only two days ago.
2012/2/1 Wolfram Sang <w.sang@pengutronix.de>: > >> i think it makes sense for us to have platform_devm_request_irq() and >> similar things. i'll take care. > > I'll try, too. Then we can compare and learn from that. well, thanks. > >> PS: as the man maintaining embedded systems of the i2c subsystem, >> would you comment the CSR i2c driver which has been there waiting for >> a long time? >> http://www.spinics.net/lists/linux-i2c/msg07081.html > > Relax, you sent that only two days ago. the v5 is sent two days ago with deleting only one line, v1-v4 has been there 3 monthes :-) i was not complaining you haven't sent comments for v5. > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | -barry
On Wed, Feb 01, 2012 at 10:11:10AM +0800, Barry Song wrote: > i think it makes sense for us to have platform_devm_request_irq() and > similar things. i'll take care. Note that this one is more tricky than most as it's really important that the interrupt handler can't be called when the data it needs and so on have been freed. With things like memory this really doesn't matter and you can happily free in any order.
On Wed, Feb 01, 2012 at 12:56:42PM +0000, Mark Brown wrote: > On Wed, Feb 01, 2012 at 10:11:10AM +0800, Barry Song wrote: > > > i think it makes sense for us to have platform_devm_request_irq() and > > similar things. i'll take care. > > Note that this one is more tricky than most as it's really important > that the interrupt handler can't be called when the data it needs and so > on have been freed. With things like memory this really doesn't matter > and you can happily free in any order. Good point. g.
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index f0c605e..39ca0ab 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -72,6 +72,25 @@ struct resource *platform_get_resource(struct platform_device *dev, EXPORT_SYMBOL_GPL(platform_get_resource); /** + * platform_devm_request_and_ioremap() - get resource, check, request region, + * and ioremap resource + * @dev: platform device + * @num: IOMEM resource index + */ +void __iomem *platform_devm_request_and_ioremap(struct platform_device *dev, + unsigned int num) +{ + struct resource *res; + + res = platform_get_resource(dev, IORESOURCE_MEM, num); + if (!res) + return NULL; + + return devm_request_and_ioremap(&dev->dev, res); +} +EXPORT_SYMBOL_GPL(platform_devm_request_and_ioremap); + +/** * platform_get_irq - get an IRQ for a device * @dev: platform device * @num: IRQ number index diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 60e9994..768c0d7 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -44,6 +44,7 @@ extern struct device platform_bus; extern void arch_setup_pdev_archdata(struct platform_device *); extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int); +extern void __iomem *platform_devm_request_and_ioremap(struct platform_device *, unsigned int); extern int platform_get_irq(struct platform_device *, unsigned int); extern struct resource *platform_get_resource_byname(struct platform_device *, unsigned int, const char *); extern int platform_get_irq_byname(struct platform_device *, const char *);