| Message ID | 1552330521-4276-38-git-send-email-info@metux.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/42] drivers: gpio: 74xx-mmio: use devm_platform_ioremap_resource() | expand |
On Mon, Mar 11, 2019 at 07:55:17PM +0100, Enrico Weigelt, metux IT consult wrote: > Use the new helper that wraps the calls to platform_get_resource() > and devm_ioremap_resource() together. > > this driver deserves a bit more cleanup, to get rid of the global > variable giu_base, which makes it single-instance-only. > > Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net> > --- > drivers/gpio/gpio-vr41xx.c | 19 +++++-------------- > 1 file changed, 5 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpio/gpio-vr41xx.c b/drivers/gpio/gpio-vr41xx.c > index b13a49c..98cd715 100644 > --- a/drivers/gpio/gpio-vr41xx.c > +++ b/drivers/gpio/gpio-vr41xx.c > @@ -467,10 +467,9 @@ static int vr41xx_gpio_to_irq(struct gpio_chip *chip, unsigned offset) > > static int giu_probe(struct platform_device *pdev) > { > - struct resource *res; > unsigned int trigger, i, pin; > struct irq_chip *chip; > - int irq, ret; > + int irq; > > switch (pdev->id) { > case GPIO_50PINS_PULLUPDOWN: > @@ -489,21 +488,14 @@ static int giu_probe(struct platform_device *pdev) > return -ENODEV; > } > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (!res) > - return -EBUSY; > - > - giu_base = ioremap(res->start, resource_size(res)); > - if (!giu_base) > - return -ENOMEM; > + giu_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(giu_base)) > + return PTR_ERR(giu_base); The driver currently doesn't request the memory described in the resource, so technically you're changing behaviour here and with your change the driver could now fail if somebody else has already claimed the memory. Looking at arch/mips/vr41xx there doesn't seem to be an overlap between the memory region used by the GIU device and any others, so this should be safe. Not sure anyone still has hardware for this around to give it a spin. Thierry > > vr41xx_gpio_chip.parent = &pdev->dev; > > - ret = gpiochip_add_data(&vr41xx_gpio_chip, NULL); > - if (!ret) { > - iounmap(giu_base); > + if (gpiochip_add_data(&vr41xx_gpio_chip, NULL)) > return -ENODEV; > - } > > giu_write(GIUINTENL, 0); > giu_write(GIUINTENH, 0); > @@ -534,7 +526,6 @@ static int giu_probe(struct platform_device *pdev) > static int giu_remove(struct platform_device *pdev) > { > if (giu_base) { > - iounmap(giu_base); > giu_base = NULL; > } > > -- > 1.9.1 >
On 12.03.19 12:37, Thierry Reding wrote: > The driver currently doesn't request the memory described in the > resource, so technically you're changing behaviour here and with your > change the driver could now fail if somebody else has already claimed > the memory. hmm, using w/o requesting/claiming - isn't that a bug ? --mtx
On 3/12/2019 8:42 AM, Enrico Weigelt, metux IT consult wrote: > On 12.03.19 12:37, Thierry Reding wrote: > >> The driver currently doesn't request the memory described in the >> resource, so technically you're changing behaviour here and with your >> change the driver could now fail if somebody else has already claimed >> the memory. > > hmm, using w/o requesting/claiming - isn't that a bug ? Not necessarily, before regmap existed, you could have very well delegated a subset of a larger resource to a specific driver while a driver requesting that larger resource would be responsible for doing the request_mem_region(). As long as both drivers don't stomp on each other, this is a perfectly valid way to delegate, yet keep things modular/separate.
On Tue, Mar 12, 2019 at 1:56 AM Enrico Weigelt, metux IT consult <info@metux.net> wrote: > Use the new helper that wraps the calls to platform_get_resource() > and devm_ioremap_resource() together. > > this driver deserves a bit more cleanup, to get rid of the global > variable giu_base, which makes it single-instance-only. > > Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net> Holding this back until we have more consensus and some ACKs. Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-vr41xx.c b/drivers/gpio/gpio-vr41xx.c index b13a49c..98cd715 100644 --- a/drivers/gpio/gpio-vr41xx.c +++ b/drivers/gpio/gpio-vr41xx.c @@ -467,10 +467,9 @@ static int vr41xx_gpio_to_irq(struct gpio_chip *chip, unsigned offset) static int giu_probe(struct platform_device *pdev) { - struct resource *res; unsigned int trigger, i, pin; struct irq_chip *chip; - int irq, ret; + int irq; switch (pdev->id) { case GPIO_50PINS_PULLUPDOWN: @@ -489,21 +488,14 @@ static int giu_probe(struct platform_device *pdev) return -ENODEV; } - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) - return -EBUSY; - - giu_base = ioremap(res->start, resource_size(res)); - if (!giu_base) - return -ENOMEM; + giu_base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(giu_base)) + return PTR_ERR(giu_base); vr41xx_gpio_chip.parent = &pdev->dev; - ret = gpiochip_add_data(&vr41xx_gpio_chip, NULL); - if (!ret) { - iounmap(giu_base); + if (gpiochip_add_data(&vr41xx_gpio_chip, NULL)) return -ENODEV; - } giu_write(GIUINTENL, 0); giu_write(GIUINTENH, 0); @@ -534,7 +526,6 @@ static int giu_probe(struct platform_device *pdev) static int giu_remove(struct platform_device *pdev) { if (giu_base) { - iounmap(giu_base); giu_base = NULL; }
Use the new helper that wraps the calls to platform_get_resource() and devm_ioremap_resource() together. this driver deserves a bit more cleanup, to get rid of the global variable giu_base, which makes it single-instance-only. Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net> --- drivers/gpio/gpio-vr41xx.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)