| Message ID | 1418226977-17128-1-git-send-email-ricardo.ribalda@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 12/10/2014 04:56 PM, Ricardo Ribalda Delgado wrote: > This way we do not need to transverse the device tree manually and we > support hot plugged devices. > > Also Implement remove callback so the driver can be unloaded > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> > --- > > v4: Add missing module_exit > > drivers/gpio/gpio-xilinx.c | 83 ++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 66 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c > index e668ec4..c7ed92b 100644 > --- a/drivers/gpio/gpio-xilinx.c > +++ b/drivers/gpio/gpio-xilinx.c > @@ -44,12 +44,18 @@ > * gpio_state: GPIO state shadow register > * gpio_dir: GPIO direction shadow register > * gpio_lock: Lock used for synchronization > + * inited: True if the port has been inited > */ > struct xgpio_instance { > struct of_mm_gpio_chip mmchip; > u32 gpio_state; > u32 gpio_dir; > spinlock_t gpio_lock; > + bool inited; > +}; > + > +struct xgpio { > + struct xgpio_instance port[2]; > }; > > /** > @@ -172,24 +178,56 @@ static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc) > } > > /** > + * xgpio_remove - Remove method for the GPIO device. > + * @pdev: pointer to the platform device > + * > + * This function remove gpiochips and frees all the allocated resources. > + */ > +static int xgpio_remove(struct platform_device *pdev) > +{ > + struct xgpio *xgpio = platform_get_drvdata(pdev); > + int i; > + > + for (i = 0; i < 2; i++) { > + if (!xgpio->port[i].inited) > + continue; > + gpiochip_remove(&xgpio->port[i].mmchip.gc); > + > + if (i == 1) > + xgpio->port[i].mmchip.regs -= XGPIO_CHANNEL_OFFSET; > + > + iounmap(xgpio->port[i].mmchip.regs); > + kfree(xgpio->port[i].mmchip.gc.label); > + } > + > + return 0; > +} > + > +/** > * xgpio_of_probe - Probe method for the GPIO device. > - * @np: pointer to device tree node > + * @pdev: pointer to the platform device > * > * This function probes the GPIO device in the device tree. It initializes the > * driver data structure. It returns 0, if the driver is bound to the GPIO > * device, or a negative value if there is an error. > */ > -static int xgpio_of_probe(struct device_node *np) > +static int xgpio_probe(struct platform_device *pdev) > { > + struct xgpio *xgpio; > struct xgpio_instance *chip; > int status = 0; > + struct device_node *np = pdev->dev.of_node; > const u32 *tree_info; > u32 ngpio; > > - chip = kzalloc(sizeof(*chip), GFP_KERNEL); > - if (!chip) > + xgpio = devm_kzalloc(&pdev->dev, sizeof(*xgpio), GFP_KERNEL); > + if (!xgpio) > return -ENOMEM; > > + platform_set_drvdata(pdev, xgpio); > + > + chip = &xgpio->port[0]; > + > /* Update GPIO state shadow register with default value */ > of_property_read_u32(np, "xlnx,dout-default", &chip->gpio_state); > > @@ -209,6 +247,7 @@ static int xgpio_of_probe(struct device_node *np) > > spin_lock_init(&chip->gpio_lock); > > + chip->mmchip.gc.dev = &pdev->dev; > chip->mmchip.gc.direction_input = xgpio_dir_in; > chip->mmchip.gc.direction_output = xgpio_dir_out; > chip->mmchip.gc.get = xgpio_get; > @@ -219,20 +258,18 @@ static int xgpio_of_probe(struct device_node *np) > /* Call the OF gpio helper to setup and register the GPIO device */ > status = of_mm_gpiochip_add(np, &chip->mmchip); > if (status) { > - kfree(chip); > pr_err("%s: error in probe function with status %d\n", > np->full_name, status); > return status; > } > + chip->inited = true; > > pr_info("XGpio: %s: registered, base is %d\n", np->full_name, > chip->mmchip.gc.base); > > tree_info = of_get_property(np, "xlnx,is-dual", NULL); > if (tree_info && be32_to_cpup(tree_info)) { > - chip = kzalloc(sizeof(*chip), GFP_KERNEL); > - if (!chip) > - return -ENOMEM; > + chip = &xgpio->port[1]; > > /* Update GPIO state shadow register with default value */ > of_property_read_u32(np, "xlnx,dout-default-2", > @@ -254,6 +291,7 @@ static int xgpio_of_probe(struct device_node *np) > > spin_lock_init(&chip->gpio_lock); > > + chip->mmchip.gc.dev = &pdev->dev; > chip->mmchip.gc.direction_input = xgpio_dir_in; > chip->mmchip.gc.direction_output = xgpio_dir_out; > chip->mmchip.gc.get = xgpio_get; > @@ -264,7 +302,7 @@ static int xgpio_of_probe(struct device_node *np) > /* Call the OF gpio helper to setup and register the GPIO dev */ > status = of_mm_gpiochip_add(np, &chip->mmchip); > if (status) { > - kfree(chip); > + xgpio_remove(pdev); > pr_err("%s: error in probe function with status %d\n", > np->full_name, status); > return status; > @@ -272,6 +310,7 @@ static int xgpio_of_probe(struct device_node *np) > > /* Add dual channel offset */ > chip->mmchip.regs += XGPIO_CHANNEL_OFFSET; > + chip->inited = true; > > pr_info("XGpio: %s: dual channel registered, base is %d\n", > np->full_name, chip->mmchip.gc.base); > @@ -285,19 +324,29 @@ static const struct of_device_id xgpio_of_match[] = { > { /* end of list */ }, > }; > > -static int __init xgpio_init(void) > -{ > - struct device_node *np; > +MODULE_DEVICE_TABLE(of, xgpio_of_match); > > - for_each_matching_node(np, xgpio_of_match) > - xgpio_of_probe(np); > +static struct platform_driver xgpio_plat_driver = { > + .probe = xgpio_probe, > + .remove = xgpio_remove, > + .driver = { > + .name = "gpio-xilinx", > + .of_match_table = xgpio_of_match, > + }, > +}; > > - return 0; > +static int __init xgpio_init(void) > +{ > + return platform_driver_register(&xgpio_plat_driver); > } > > -/* Make sure we get initialized before anyone else tries to use us */ > subsys_initcall(xgpio_init); > -/* No exit call at the moment as we cannot unregister of GPIO chips */ > + > +static void __exit xgpio_exit(void) > +{ > + platform_driver_unregister(&xgpio_plat_driver); > +} > +module_exit(xgpio_exit); > > MODULE_AUTHOR("Xilinx, Inc."); > MODULE_DESCRIPTION("Xilinx GPIO driver"); > Feel free to add my acked-by line to all these patches. We have IRQ support in xilinx tree but I also know that this driver should be changed a little bit but these patches are at least good step on that way. Thanks, Michal -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 11, 2014 at 12:56 AM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > This way we do not need to transverse the device tree manually and we > support hot plugged devices. > > Also Implement remove callback so the driver can be unloaded > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> > --- > > v4: Add missing module_exit > > drivers/gpio/gpio-xilinx.c | 83 ++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 66 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c > index e668ec4..c7ed92b 100644 > --- a/drivers/gpio/gpio-xilinx.c > +++ b/drivers/gpio/gpio-xilinx.c > @@ -44,12 +44,18 @@ > * gpio_state: GPIO state shadow register > * gpio_dir: GPIO direction shadow register > * gpio_lock: Lock used for synchronization > + * inited: True if the port has been inited > */ > struct xgpio_instance { > struct of_mm_gpio_chip mmchip; > u32 gpio_state; > u32 gpio_dir; > spinlock_t gpio_lock; > + bool inited; > +}; > + > +struct xgpio { > + struct xgpio_instance port[2]; > }; > > /** > @@ -172,24 +178,56 @@ static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc) > } > > /** > + * xgpio_remove - Remove method for the GPIO device. > + * @pdev: pointer to the platform device > + * > + * This function remove gpiochips and frees all the allocated resources. > + */ > +static int xgpio_remove(struct platform_device *pdev) > +{ > + struct xgpio *xgpio = platform_get_drvdata(pdev); > + int i; > + > + for (i = 0; i < 2; i++) { > + if (!xgpio->port[i].inited) > + continue; > + gpiochip_remove(&xgpio->port[i].mmchip.gc); > + > + if (i == 1) > + xgpio->port[i].mmchip.regs -= XGPIO_CHANNEL_OFFSET; > + > + iounmap(xgpio->port[i].mmchip.regs); This should not be here. The mapping and call to gpiochip_add() are performed by of_mm_gpiochip_add(). We should thus have a of_mm_gpiochip_remove() function that undoes what _add did instead of expected all users to do unmap themselves. Can you add a patch to your series that adds this function? Also I am not sure I understand why the unmapping is done only once. Both chips are supposed to have been added (and thus mapped) at this stage. Oh right I see, so this driver ends up mapping the same area twice! Not only are you iomapping the same area twice, you are unmapping it only once, and only if the chip is dual. This looks very broken. Couldn't you redesign the driver the following way: only add one chip (since you have 1 DT node), with an extra member to track which GPIOs belong to the second chip (in case it is dual), and change the other functions to handle this. > + kfree(xgpio->port[i].mmchip.gc.label); > + } > + > + return 0; > +} > + > +/** > * xgpio_of_probe - Probe method for the GPIO device. > - * @np: pointer to device tree node > + * @pdev: pointer to the platform device > * > * This function probes the GPIO device in the device tree. It initializes the > * driver data structure. It returns 0, if the driver is bound to the GPIO > * device, or a negative value if there is an error. > */ > -static int xgpio_of_probe(struct device_node *np) > +static int xgpio_probe(struct platform_device *pdev) > { > + struct xgpio *xgpio; > struct xgpio_instance *chip; > int status = 0; > + struct device_node *np = pdev->dev.of_node; > const u32 *tree_info; > u32 ngpio; > > - chip = kzalloc(sizeof(*chip), GFP_KERNEL); > - if (!chip) > + xgpio = devm_kzalloc(&pdev->dev, sizeof(*xgpio), GFP_KERNEL); > + if (!xgpio) > return -ENOMEM; > > + platform_set_drvdata(pdev, xgpio); > + > + chip = &xgpio->port[0]; > + > /* Update GPIO state shadow register with default value */ > of_property_read_u32(np, "xlnx,dout-default", &chip->gpio_state); > > @@ -209,6 +247,7 @@ static int xgpio_of_probe(struct device_node *np) > > spin_lock_init(&chip->gpio_lock); > > + chip->mmchip.gc.dev = &pdev->dev; > chip->mmchip.gc.direction_input = xgpio_dir_in; > chip->mmchip.gc.direction_output = xgpio_dir_out; > chip->mmchip.gc.get = xgpio_get; > @@ -219,20 +258,18 @@ static int xgpio_of_probe(struct device_node *np) > /* Call the OF gpio helper to setup and register the GPIO device */ > status = of_mm_gpiochip_add(np, &chip->mmchip); > if (status) { > - kfree(chip); > pr_err("%s: error in probe function with status %d\n", > np->full_name, status); > return status; > } > + chip->inited = true; > > pr_info("XGpio: %s: registered, base is %d\n", np->full_name, > chip->mmchip.gc.base); > > tree_info = of_get_property(np, "xlnx,is-dual", NULL); > if (tree_info && be32_to_cpup(tree_info)) { > - chip = kzalloc(sizeof(*chip), GFP_KERNEL); > - if (!chip) > - return -ENOMEM; > + chip = &xgpio->port[1]; > > /* Update GPIO state shadow register with default value */ > of_property_read_u32(np, "xlnx,dout-default-2", > @@ -254,6 +291,7 @@ static int xgpio_of_probe(struct device_node *np) > > spin_lock_init(&chip->gpio_lock); > > + chip->mmchip.gc.dev = &pdev->dev; > chip->mmchip.gc.direction_input = xgpio_dir_in; > chip->mmchip.gc.direction_output = xgpio_dir_out; > chip->mmchip.gc.get = xgpio_get; > @@ -264,7 +302,7 @@ static int xgpio_of_probe(struct device_node *np) > /* Call the OF gpio helper to setup and register the GPIO dev */ > status = of_mm_gpiochip_add(np, &chip->mmchip); > if (status) { > - kfree(chip); > + xgpio_remove(pdev); > pr_err("%s: error in probe function with status %d\n", > np->full_name, status); > return status; > @@ -272,6 +310,7 @@ static int xgpio_of_probe(struct device_node *np) > > /* Add dual channel offset */ > chip->mmchip.regs += XGPIO_CHANNEL_OFFSET; > + chip->inited = true; The DT binding also suggests this should be one single chip. There is a lot of redundant code that you could suppress if you follow the design I suggest above. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Alexandre > This should not be here. The mapping and call to gpiochip_add() are > performed by of_mm_gpiochip_add(). We should thus have a > of_mm_gpiochip_remove() function that undoes what _add did instead of > expected all users to do unmap themselves. Can you add a patch to your > series that adds this function? > > Also I am not sure I understand why the unmapping is done only once. > Both chips are supposed to have been added (and thus mapped) at this > stage. Oh right I see, so this driver ends up mapping the same area > twice! Not only are you iomapping the same area twice, you are > unmapping it only once, and only if the chip is dual. This looks very > broken. > If you look carefully you can see that it is unmapped twice if it is called twice. iounmap is called inside the for loop. > Couldn't you redesign the driver the following way: only add one chip > (since you have 1 DT node), with an extra member to track which GPIOs > belong to the second chip (in case it is dual), and change the other > functions to handle this. I do not mind rearranging the driver so there is only one gpio device, even for dual chips, but I think this should be done in a separate patch. What about? 1) Keep the current patchset and then 2) Add another patchset with - xilinx-gpio: only one gpio device - add of_mm_gpiochip_remove() to the api - xilinx gpio: use of_mm_gpiochip_remove - others: use of_mm_gpiochip_remove Regards!
On Wed, Dec 17, 2014 at 8:02 PM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > Hello Alexandre > >> This should not be here. The mapping and call to gpiochip_add() are >> performed by of_mm_gpiochip_add(). We should thus have a >> of_mm_gpiochip_remove() function that undoes what _add did instead of >> expected all users to do unmap themselves. Can you add a patch to your >> series that adds this function? > >> >> Also I am not sure I understand why the unmapping is done only once. >> Both chips are supposed to have been added (and thus mapped) at this >> stage. Oh right I see, so this driver ends up mapping the same area >> twice! Not only are you iomapping the same area twice, you are >> unmapping it only once, and only if the chip is dual. This looks very >> broken. >> > > If you look carefully you can see that it is unmapped twice if it is > called twice. iounmap is called inside the for loop. D'oh, you are right of course. I don't know why, but I thought the iounmap() was part of the if (i == 1) conditional block. >_< > > >> Couldn't you redesign the driver the following way: only add one chip >> (since you have 1 DT node), with an extra member to track which GPIOs >> belong to the second chip (in case it is dual), and change the other >> functions to handle this. > > I do not mind rearranging the driver so there is only one gpio device, > even for dual chips, but I think this should be done in a separate > patch. > > What about? > > 1) Keep the current patchset > > and then > > 2) Add another patchset with > > - xilinx-gpio: only one gpio device > - add of_mm_gpiochip_remove() to the api > - xilinx gpio: use of_mm_gpiochip_remove > - others: use of_mm_gpiochip_remove I think this would look better this way: - xilinx-gpio: remove offset property - xilinx-gpio: only one gpio device - add of_mm_gpiochip_remove() to the api - xilinx-gpio: use of_mm_gpiochip_remove - xilinx-gpio: Convert the driver to platform device interface. (others: use of_mm_gpiochip_remove would be appreciated of course, but I won't ask you to go that far and fix everybody). The reason for this order is that your current patch would be shorter is the driver is turned to add one device only first. It's also generally better to work on cleaner code. But to switch the driver to single-device, you will first need to remove the offset property (IIUC, at least). -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 17, 2014 at 2:26 PM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Wed, Dec 17, 2014 at 8:02 PM, Ricardo Ribalda Delgado > <ricardo.ribalda@gmail.com> wrote: >> Hello Alexandre >> >>> This should not be here. The mapping and call to gpiochip_add() are >>> performed by of_mm_gpiochip_add(). We should thus have a >>> of_mm_gpiochip_remove() function that undoes what _add did instead of >>> expected all users to do unmap themselves. Can you add a patch to your >>> series that adds this function? >> >>> >>> Also I am not sure I understand why the unmapping is done only once. >>> Both chips are supposed to have been added (and thus mapped) at this >>> stage. Oh right I see, so this driver ends up mapping the same area >>> twice! Not only are you iomapping the same area twice, you are >>> unmapping it only once, and only if the chip is dual. This looks very >>> broken. >>> >> >> If you look carefully you can see that it is unmapped twice if it is >> called twice. iounmap is called inside the for loop. > > D'oh, you are right of course. I don't know why, but I thought the > iounmap() was part of the if (i == 1) conditional block. >_< > >> >> >>> Couldn't you redesign the driver the following way: only add one chip >>> (since you have 1 DT node), with an extra member to track which GPIOs >>> belong to the second chip (in case it is dual), and change the other >>> functions to handle this. >> >> I do not mind rearranging the driver so there is only one gpio device, >> even for dual chips, but I think this should be done in a separate >> patch. >> >> What about? >> >> 1) Keep the current patchset >> >> and then >> >> 2) Add another patchset with >> >> - xilinx-gpio: only one gpio device >> - add of_mm_gpiochip_remove() to the api >> - xilinx gpio: use of_mm_gpiochip_remove >> - others: use of_mm_gpiochip_remove > > I think this would look better this way: > > - xilinx-gpio: remove offset property > - xilinx-gpio: only one gpio device > - add of_mm_gpiochip_remove() to the api > - xilinx-gpio: use of_mm_gpiochip_remove > - xilinx-gpio: Convert the driver to platform device interface. > > (others: use of_mm_gpiochip_remove would be appreciated of course, but > I won't ask you to go that far and fix everybody). > > The reason for this order is that your current patch would be shorter > is the driver is turned to add one device only first. It's also > generally better to work on cleaner code. But to switch the driver to > single-device, you will first need to remove the offset property > (IIUC, at least). I totally see your point but I rather do not touch the first patchset, two reasons. One is that other people has already acked it and that it will make my life easier and probably have it ready before the holidays :) Anyway I could change your mind to: - xilinx-gpio: remove offset property - xilinx-gpio: Convert the driver to platform device interface. - xilinx-gpio: only one gpio device - add of_mm_gpiochip_remove() to the api - xilinx-gpio: use of_mm_gpiochip_remove ? Regards!
On Wed, Dec 17, 2014 at 10:31 PM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > On Wed, Dec 17, 2014 at 2:26 PM, Alexandre Courbot <gnurou@gmail.com> wrote: >> On Wed, Dec 17, 2014 at 8:02 PM, Ricardo Ribalda Delgado >> <ricardo.ribalda@gmail.com> wrote: >>> Hello Alexandre >>> >>>> This should not be here. The mapping and call to gpiochip_add() are >>>> performed by of_mm_gpiochip_add(). We should thus have a >>>> of_mm_gpiochip_remove() function that undoes what _add did instead of >>>> expected all users to do unmap themselves. Can you add a patch to your >>>> series that adds this function? >>> >>>> >>>> Also I am not sure I understand why the unmapping is done only once. >>>> Both chips are supposed to have been added (and thus mapped) at this >>>> stage. Oh right I see, so this driver ends up mapping the same area >>>> twice! Not only are you iomapping the same area twice, you are >>>> unmapping it only once, and only if the chip is dual. This looks very >>>> broken. >>>> >>> >>> If you look carefully you can see that it is unmapped twice if it is >>> called twice. iounmap is called inside the for loop. >> >> D'oh, you are right of course. I don't know why, but I thought the >> iounmap() was part of the if (i == 1) conditional block. >_< >> >>> >>> >>>> Couldn't you redesign the driver the following way: only add one chip >>>> (since you have 1 DT node), with an extra member to track which GPIOs >>>> belong to the second chip (in case it is dual), and change the other >>>> functions to handle this. >>> >>> I do not mind rearranging the driver so there is only one gpio device, >>> even for dual chips, but I think this should be done in a separate >>> patch. >>> >>> What about? >>> >>> 1) Keep the current patchset >>> >>> and then >>> >>> 2) Add another patchset with >>> >>> - xilinx-gpio: only one gpio device >>> - add of_mm_gpiochip_remove() to the api >>> - xilinx gpio: use of_mm_gpiochip_remove >>> - others: use of_mm_gpiochip_remove >> >> I think this would look better this way: >> >> - xilinx-gpio: remove offset property >> - xilinx-gpio: only one gpio device >> - add of_mm_gpiochip_remove() to the api >> - xilinx-gpio: use of_mm_gpiochip_remove >> - xilinx-gpio: Convert the driver to platform device interface. >> >> (others: use of_mm_gpiochip_remove would be appreciated of course, but >> I won't ask you to go that far and fix everybody). >> >> The reason for this order is that your current patch would be shorter >> is the driver is turned to add one device only first. It's also >> generally better to work on cleaner code. But to switch the driver to >> single-device, you will first need to remove the offset property >> (IIUC, at least). > > > I totally see your point but I rather do not touch the first patchset, > two reasons. > > One is that other people has already acked it and that it will make my > life easier and probably have it ready before the holidays :) Maintainers are also interested in making their life easier, you know - a concern that should be shared by anyone who wants to see their patches merged. ;) > > > Anyway I could change your mind to: > - xilinx-gpio: remove offset property > - xilinx-gpio: Convert the driver to platform device interface. > - xilinx-gpio: only one gpio device > - add of_mm_gpiochip_remove() to the api > - xilinx-gpio: use of_mm_gpiochip_remove > ? Well, at the end of the day it would be the same, and even in its current form this patch is an improvement. So I guess it would be ok. What I like about my plan is that this patch comes last, so you are obliged to reorganize the driver - whereas if we merge this series now you can just run away. :P Let's do this way: - xilinx-gpio: remove offset property - add of_mm_gpiochip_remove() to the api - xilinx-gpio: Convert the driver to platform device interface (using of_mm_gpiochip_remove()) - xilinx-gpio: only one gpio device (if you don't run away, that is) Adding of_mm_gpiochip_remove() is a trivial task, but is really critical to remove the device correctly, which your platform device patch needs to do. It won't add much work for you, but at least all the improvements that are non-local to the xilinx driver will be there. Deal? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello I cannot do only of_mm_gpiochip_remove before only_one_gpio_device, because I have to restore the initial io address :S. Anyway I have prepared a new patchset with hopefully all you need. I have resend also the old patches so you can have a general view. I think that only gpio-mpc5200.c would benefit from the new API, I can send a patchset to support removal and use the new API if you want. Thanks! On Wed, Dec 17, 2014 at 3:05 PM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Wed, Dec 17, 2014 at 10:31 PM, Ricardo Ribalda Delgado > <ricardo.ribalda@gmail.com> wrote: >> On Wed, Dec 17, 2014 at 2:26 PM, Alexandre Courbot <gnurou@gmail.com> wrote: >>> On Wed, Dec 17, 2014 at 8:02 PM, Ricardo Ribalda Delgado >>> <ricardo.ribalda@gmail.com> wrote: >>>> Hello Alexandre >>>> >>>>> This should not be here. The mapping and call to gpiochip_add() are >>>>> performed by of_mm_gpiochip_add(). We should thus have a >>>>> of_mm_gpiochip_remove() function that undoes what _add did instead of >>>>> expected all users to do unmap themselves. Can you add a patch to your >>>>> series that adds this function? >>>> >>>>> >>>>> Also I am not sure I understand why the unmapping is done only once. >>>>> Both chips are supposed to have been added (and thus mapped) at this >>>>> stage. Oh right I see, so this driver ends up mapping the same area >>>>> twice! Not only are you iomapping the same area twice, you are >>>>> unmapping it only once, and only if the chip is dual. This looks very >>>>> broken. >>>>> >>>> >>>> If you look carefully you can see that it is unmapped twice if it is >>>> called twice. iounmap is called inside the for loop. >>> >>> D'oh, you are right of course. I don't know why, but I thought the >>> iounmap() was part of the if (i == 1) conditional block. >_< >>> >>>> >>>> >>>>> Couldn't you redesign the driver the following way: only add one chip >>>>> (since you have 1 DT node), with an extra member to track which GPIOs >>>>> belong to the second chip (in case it is dual), and change the other >>>>> functions to handle this. >>>> >>>> I do not mind rearranging the driver so there is only one gpio device, >>>> even for dual chips, but I think this should be done in a separate >>>> patch. >>>> >>>> What about? >>>> >>>> 1) Keep the current patchset >>>> >>>> and then >>>> >>>> 2) Add another patchset with >>>> >>>> - xilinx-gpio: only one gpio device >>>> - add of_mm_gpiochip_remove() to the api >>>> - xilinx gpio: use of_mm_gpiochip_remove >>>> - others: use of_mm_gpiochip_remove >>> >>> I think this would look better this way: >>> >>> - xilinx-gpio: remove offset property >>> - xilinx-gpio: only one gpio device >>> - add of_mm_gpiochip_remove() to the api >>> - xilinx-gpio: use of_mm_gpiochip_remove >>> - xilinx-gpio: Convert the driver to platform device interface. >>> >>> (others: use of_mm_gpiochip_remove would be appreciated of course, but >>> I won't ask you to go that far and fix everybody). >>> >>> The reason for this order is that your current patch would be shorter >>> is the driver is turned to add one device only first. It's also >>> generally better to work on cleaner code. But to switch the driver to >>> single-device, you will first need to remove the offset property >>> (IIUC, at least). >> >> >> I totally see your point but I rather do not touch the first patchset, >> two reasons. >> >> One is that other people has already acked it and that it will make my >> life easier and probably have it ready before the holidays :) > > Maintainers are also interested in making their life easier, you know > - a concern that should be shared by anyone who wants to see their > patches merged. ;) > >> >> >> Anyway I could change your mind to: >> - xilinx-gpio: remove offset property >> - xilinx-gpio: Convert the driver to platform device interface. >> - xilinx-gpio: only one gpio device >> - add of_mm_gpiochip_remove() to the api >> - xilinx-gpio: use of_mm_gpiochip_remove >> ? > > Well, at the end of the day it would be the same, and even in its > current form this patch is an improvement. So I guess it would be ok. > What I like about my plan is that this patch comes last, so you are > obliged to reorganize the driver - whereas if we merge this series now > you can just run away. :P > > Let's do this way: > > - xilinx-gpio: remove offset property > - add of_mm_gpiochip_remove() to the api > - xilinx-gpio: Convert the driver to platform device interface (using > of_mm_gpiochip_remove()) > - xilinx-gpio: only one gpio device (if you don't run away, that is) > > Adding of_mm_gpiochip_remove() is a trivial task, but is really > critical to remove the device correctly, which your platform device > patch needs to do. It won't add much work for you, but at least all > the improvements that are non-local to the xilinx driver will be > there. > > Deal?
diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c index e668ec4..c7ed92b 100644 --- a/drivers/gpio/gpio-xilinx.c +++ b/drivers/gpio/gpio-xilinx.c @@ -44,12 +44,18 @@ * gpio_state: GPIO state shadow register * gpio_dir: GPIO direction shadow register * gpio_lock: Lock used for synchronization + * inited: True if the port has been inited */ struct xgpio_instance { struct of_mm_gpio_chip mmchip; u32 gpio_state; u32 gpio_dir; spinlock_t gpio_lock; + bool inited; +}; + +struct xgpio { + struct xgpio_instance port[2]; }; /** @@ -172,24 +178,56 @@ static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc) } /** + * xgpio_remove - Remove method for the GPIO device. + * @pdev: pointer to the platform device + * + * This function remove gpiochips and frees all the allocated resources. + */ +static int xgpio_remove(struct platform_device *pdev) +{ + struct xgpio *xgpio = platform_get_drvdata(pdev); + int i; + + for (i = 0; i < 2; i++) { + if (!xgpio->port[i].inited) + continue; + gpiochip_remove(&xgpio->port[i].mmchip.gc); + + if (i == 1) + xgpio->port[i].mmchip.regs -= XGPIO_CHANNEL_OFFSET; + + iounmap(xgpio->port[i].mmchip.regs); + kfree(xgpio->port[i].mmchip.gc.label); + } + + return 0; +} + +/** * xgpio_of_probe - Probe method for the GPIO device. - * @np: pointer to device tree node + * @pdev: pointer to the platform device * * This function probes the GPIO device in the device tree. It initializes the * driver data structure. It returns 0, if the driver is bound to the GPIO * device, or a negative value if there is an error. */ -static int xgpio_of_probe(struct device_node *np) +static int xgpio_probe(struct platform_device *pdev) { + struct xgpio *xgpio; struct xgpio_instance *chip; int status = 0; + struct device_node *np = pdev->dev.of_node; const u32 *tree_info; u32 ngpio; - chip = kzalloc(sizeof(*chip), GFP_KERNEL); - if (!chip) + xgpio = devm_kzalloc(&pdev->dev, sizeof(*xgpio), GFP_KERNEL); + if (!xgpio) return -ENOMEM; + platform_set_drvdata(pdev, xgpio); + + chip = &xgpio->port[0]; + /* Update GPIO state shadow register with default value */ of_property_read_u32(np, "xlnx,dout-default", &chip->gpio_state); @@ -209,6 +247,7 @@ static int xgpio_of_probe(struct device_node *np) spin_lock_init(&chip->gpio_lock); + chip->mmchip.gc.dev = &pdev->dev; chip->mmchip.gc.direction_input = xgpio_dir_in; chip->mmchip.gc.direction_output = xgpio_dir_out; chip->mmchip.gc.get = xgpio_get; @@ -219,20 +258,18 @@ static int xgpio_of_probe(struct device_node *np) /* Call the OF gpio helper to setup and register the GPIO device */ status = of_mm_gpiochip_add(np, &chip->mmchip); if (status) { - kfree(chip); pr_err("%s: error in probe function with status %d\n", np->full_name, status); return status; } + chip->inited = true; pr_info("XGpio: %s: registered, base is %d\n", np->full_name, chip->mmchip.gc.base); tree_info = of_get_property(np, "xlnx,is-dual", NULL); if (tree_info && be32_to_cpup(tree_info)) { - chip = kzalloc(sizeof(*chip), GFP_KERNEL); - if (!chip) - return -ENOMEM; + chip = &xgpio->port[1]; /* Update GPIO state shadow register with default value */ of_property_read_u32(np, "xlnx,dout-default-2", @@ -254,6 +291,7 @@ static int xgpio_of_probe(struct device_node *np) spin_lock_init(&chip->gpio_lock); + chip->mmchip.gc.dev = &pdev->dev; chip->mmchip.gc.direction_input = xgpio_dir_in; chip->mmchip.gc.direction_output = xgpio_dir_out; chip->mmchip.gc.get = xgpio_get; @@ -264,7 +302,7 @@ static int xgpio_of_probe(struct device_node *np) /* Call the OF gpio helper to setup and register the GPIO dev */ status = of_mm_gpiochip_add(np, &chip->mmchip); if (status) { - kfree(chip); + xgpio_remove(pdev); pr_err("%s: error in probe function with status %d\n", np->full_name, status); return status; @@ -272,6 +310,7 @@ static int xgpio_of_probe(struct device_node *np) /* Add dual channel offset */ chip->mmchip.regs += XGPIO_CHANNEL_OFFSET; + chip->inited = true; pr_info("XGpio: %s: dual channel registered, base is %d\n", np->full_name, chip->mmchip.gc.base); @@ -285,19 +324,29 @@ static const struct of_device_id xgpio_of_match[] = { { /* end of list */ }, }; -static int __init xgpio_init(void) -{ - struct device_node *np; +MODULE_DEVICE_TABLE(of, xgpio_of_match); - for_each_matching_node(np, xgpio_of_match) - xgpio_of_probe(np); +static struct platform_driver xgpio_plat_driver = { + .probe = xgpio_probe, + .remove = xgpio_remove, + .driver = { + .name = "gpio-xilinx", + .of_match_table = xgpio_of_match, + }, +}; - return 0; +static int __init xgpio_init(void) +{ + return platform_driver_register(&xgpio_plat_driver); } -/* Make sure we get initialized before anyone else tries to use us */ subsys_initcall(xgpio_init); -/* No exit call at the moment as we cannot unregister of GPIO chips */ + +static void __exit xgpio_exit(void) +{ + platform_driver_unregister(&xgpio_plat_driver); +} +module_exit(xgpio_exit); MODULE_AUTHOR("Xilinx, Inc."); MODULE_DESCRIPTION("Xilinx GPIO driver");
This way we do not need to transverse the device tree manually and we support hot plugged devices. Also Implement remove callback so the driver can be unloaded Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> --- v4: Add missing module_exit drivers/gpio/gpio-xilinx.c | 83 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 66 insertions(+), 17 deletions(-)