| Message ID | 1357163240-23131-1-git-send-email-linux@prisktech.co.nz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 2, 2013 at 10:47 PM, Tony Prisk <linux@prisktech.co.nz> wrote: > This driver is missing a .remove callback, and the fail path on > probe is incomplete. > > If an error occurs in vt8500_add_chips, gpio_base is not unmapped. > The driver is also ignoring the return value from this function so > if a chip fails to register it completes as successful. > > Replaced pr_err with dev_err in vt8500_add_chips since the device is > available. > > There is also no .remove callback defined. To allow removing the > registered chips, I have moved *vtchip to be a static global. > > Signed-off-by: Tony Prisk <linux@prisktech.co.nz> Applied to fixes, thanks! Yours, Linus Walleij
On Thu, Jan 03, 2013 at 10:47:20AM +1300, Tony Prisk wrote: > There is also no .remove callback defined. To allow removing the > registered chips, I have moved *vtchip to be a static global. What? Why? > +/* Pointer to our array of chips */ > +static struct vt8500_gpio_chip *vtchip; > + ... > +static int vt8500_gpio_remove(struct platform_device *pdev) > +{ > + int i; > + int ret; > + const struct vt8500_gpio_data *data; > + void __iomem *gpio_base = vtchip[0].base; > + const struct of_device_id *of_id = > + of_match_device(vt8500_gpio_dt_ids, &pdev->dev); > + You can get at the vtchip pointer if you put it into the platform device's driver data pointer. That way, you're not artificially limiting this driver to just one device, and, with your changes it will go wrong if DT ever lists more than one device.
On Thu, Jan 10, 2013 at 11:57 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Thu, Jan 03, 2013 at 10:47:20AM +1300, Tony Prisk wrote: >> +static int vt8500_gpio_remove(struct platform_device *pdev) >> +{ >> + int i; >> + int ret; >> + const struct vt8500_gpio_data *data; >> + void __iomem *gpio_base = vtchip[0].base; >> + const struct of_device_id *of_id = >> + of_match_device(vt8500_gpio_dt_ids, &pdev->dev); >> + > > You can get at the vtchip pointer if you put it into the platform device's > driver data pointer. That way, you're not artificially limiting this > driver to just one device, and, with your changes it will go wrong if DT > ever lists more than one device. Good point, I'm sloppy today :-( Patch dropped. Tony pls proceed as indicated by Russell. Yours, Linus Walleij
On Thu, 2013-01-10 at 13:02 +0100, Linus Walleij wrote: > On Thu, Jan 10, 2013 at 11:57 AM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Thu, Jan 03, 2013 at 10:47:20AM +1300, Tony Prisk wrote: > > >> +static int vt8500_gpio_remove(struct platform_device *pdev) > >> +{ > >> + int i; > >> + int ret; > >> + const struct vt8500_gpio_data *data; > >> + void __iomem *gpio_base = vtchip[0].base; > >> + const struct of_device_id *of_id = > >> + of_match_device(vt8500_gpio_dt_ids, &pdev->dev); > >> + > > > > You can get at the vtchip pointer if you put it into the platform device's > > driver data pointer. That way, you're not artificially limiting this > > driver to just one device, and, with your changes it will go wrong if DT > > ever lists more than one device. > > Good point, I'm sloppy today :-( > > Patch dropped. > > Tony pls proceed as indicated by Russell. > > Yours, > Linus Walleij I must have been having a 'stupid' day to not realise that. Will fix. Thanks Russell. Regards Tony P
diff --git a/drivers/gpio/gpio-vt8500.c b/drivers/gpio/gpio-vt8500.c index b53320a..a147b33 100644 --- a/drivers/gpio/gpio-vt8500.c +++ b/drivers/gpio/gpio-vt8500.c @@ -122,11 +122,13 @@ static struct vt8500_gpio_data wm8650_data = { struct vt8500_gpio_chip { struct gpio_chip chip; - const struct vt8500_gpio_bank_regoffsets *regs; void __iomem *base; }; +/* Pointer to our array of chips */ +static struct vt8500_gpio_chip *vtchip; + #define to_vt8500(__chip) container_of(__chip, struct vt8500_gpio_chip, chip) @@ -224,7 +226,6 @@ static int vt8500_of_xlate(struct gpio_chip *gc, static int vt8500_add_chips(struct platform_device *pdev, void __iomem *base, const struct vt8500_gpio_data *data) { - struct vt8500_gpio_chip *vtchip; struct gpio_chip *chip; int i; int pin_cnt = 0; @@ -233,7 +234,7 @@ static int vt8500_add_chips(struct platform_device *pdev, void __iomem *base, sizeof(struct vt8500_gpio_chip) * data->num_banks, GFP_KERNEL); if (!vtchip) { - pr_err("%s: failed to allocate chip memory\n", __func__); + dev_err(&pdev->dev, "failed to allocate chip memory\n"); return -ENOMEM; } @@ -261,6 +262,7 @@ static int vt8500_add_chips(struct platform_device *pdev, void __iomem *base, gpiochip_add(chip); } + return 0; } @@ -273,36 +275,63 @@ static struct of_device_id vt8500_gpio_dt_ids[] = { static int vt8500_gpio_probe(struct platform_device *pdev) { + int ret; void __iomem *gpio_base; - struct device_node *np; + struct device_node *np = pdev->dev.of_node; const struct of_device_id *of_id = of_match_device(vt8500_gpio_dt_ids, &pdev->dev); - if (!of_id) { - dev_err(&pdev->dev, "Failed to find gpio controller\n"); + if (!np) { + dev_err(&pdev->dev, "GPIO node missing in devicetree\n"); return -ENODEV; } - np = pdev->dev.of_node; - if (!np) { - dev_err(&pdev->dev, "Missing GPIO description in devicetree\n"); - return -EFAULT; + if (!of_id) { + dev_err(&pdev->dev, "No matching driver data\n"); + return -ENODEV; } gpio_base = of_iomap(np, 0); if (!gpio_base) { dev_err(&pdev->dev, "Unable to map GPIO registers\n"); - of_node_put(np); return -ENOMEM; } - vt8500_add_chips(pdev, gpio_base, of_id->data); + ret = vt8500_add_chips(pdev, gpio_base, of_id->data); + if (ret) { + iounmap(gpio_base); + return ret; + } + + return 0; +} + +static int vt8500_gpio_remove(struct platform_device *pdev) +{ + int i; + int ret; + const struct vt8500_gpio_data *data; + void __iomem *gpio_base = vtchip[0].base; + const struct of_device_id *of_id = + of_match_device(vt8500_gpio_dt_ids, &pdev->dev); + + data = of_id->data; + + for (i = 0; i < data->num_banks; i++) { + ret = gpiochip_remove(&vtchip[i].chip); + if (ret) + dev_warn(&pdev->dev, "gpiochip_remove returned %d\n", + ret); + } + + iounmap(gpio_base); return 0; } static struct platform_driver vt8500_gpio_driver = { .probe = vt8500_gpio_probe, + .remove = vt8500_gpio_remove, .driver = { .name = "vt8500-gpio", .owner = THIS_MODULE,
This driver is missing a .remove callback, and the fail path on probe is incomplete. If an error occurs in vt8500_add_chips, gpio_base is not unmapped. The driver is also ignoring the return value from this function so if a chip fails to register it completes as successful. Replaced pr_err with dev_err in vt8500_add_chips since the device is available. There is also no .remove callback defined. To allow removing the registered chips, I have moved *vtchip to be a static global. Signed-off-by: Tony Prisk <linux@prisktech.co.nz> --- drivers/gpio/gpio-vt8500.c | 53 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 12 deletions(-)