gpio: Renesas R-Car GPIO driver update
Subsystems (3)
Magnus Damm
maintainercover gpio: Renesas R-Car GPIO driver update
gpio: Renesas R-Car GPIO driver update
[PATCH 01/03] gpio: Renesas R-Car GPIO driver V2 [PATCH 02/03] gpio: rcar: Use IRQCHIP_SET_TYPE_MASKED [PATCH 03/03] gpio: rcar: Make use of devm functions
This series updates the R-Car GPIO driver with various changes kindly suggested by Laurent Pinchart.
Patch [01/03] is a drop in replacement for V1 of the R-Car GPIO driver. The flag in patch [02/03] is kept out of [01/03] to avoid changing the behaviour of the driver between V1 and V2. Also, devm support in [03/03] is kept out of [01/03] to make sure back porting goes as smooth as possible.
---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rcar.c | 417 +++++++++++++++++++++++++++++-- include/linux/platform_data/gpio-rcar.h | 25 + 4 files changed, 427 insertions(+), 22 deletions(-)
Magnus Damm
maintainerPATCH 1/3 gpio: Renesas R-Car GPIO driver V2
From: Magnus Damm <[email protected]>
This patch is V2 of a GPIO driver for the R-Car series of SoCs from Renesas. This driver is designed to be reusable between multiple SoCs that share the same basic building block, but so far it has only been used on R-Car H1 (r8a7779).
Each driver instance handles 32 GPIOs with individually maskable IRQs. The driver operates on a single I/O memory range and the 32 GPIOs are hooked up a single interrupt.
In the case of R-Car H1 either external IRQ pins or GPIOs with interrupts can be used for on-board interupts. For external IRQs 4 pins are supported, and in the case of GPIO there are 202 GPIOS as 202 interrupts hooked up via 6 driver instances and to the GIC and the Cortex-A9 Quad.
At this point this driver is interfacing as a regular platform device driver. In the future DT support will be submitted as an incremental feature patch.
---
Changes since V1:
- Update based on most suggestions from review by Laurent, thanks!
Please note that in V2 the driver is reworked to reduce the number of lines and includes minor changes related to headers, printouts and data types. In V2 the register access order is kept the same as in V1, and to make that happen IRQCHIP_SET_TYPE_MASKED is omitted. The interface to the Linux kernel is unchanged except the use of dev_dbg() instead of pr_debug(). So from a hardware point of view V2 is a drop in replacement for V1 and there should be no additional dependencies since devm is kept as a separate patch as well. All this to make back porting easier.
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rcar.c | 383 +++++++++++++++++++++++++++++++ include/linux/platform_data/gpio-rcar.h | 25 ++ 4 files changed, 415 insertions(+)
drivers/gpio/Kconfig +6 -0
@@ -201,6 +201,12 @@ config GPIO_PXA help Say yes here to support the PXA GPIO device +config GPIO_RCAR + tristate "Renesas R-Car GPIO" + depends on ARM + help + Say yes here to support GPIO on Renesas R-Car SoCs. + config GPIO_SPEAR_SPICS bool "ST SPEAr13xx SPI Chip Select as GPIO support" depends on PLAT_SPEAR
drivers/gpio/Makefile +1 -0
@@ -56,6 +56,7 @@ obj-$(CONFIG_GPIO_PL061) += gpio-pl061.o obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o +obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o obj-$(CONFIG_PLAT_SAMSUNG) += gpio-samsung.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o obj-$(CONFIG_GPIO_SCH) += gpio-sch.o
drivers/gpio/gpio-rcar.c new file +383 -0
383 lines not on this page · raw message
include/linux/platform_data/gpio-rcar.h new file +25 -0
@@ -0,0 +1,25 @@ +/* +* Renesas R-Car GPIO Support +* +* Copyright (C) 2013 Magnus Damm +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef __GPIO_RCAR_H__ +#define __GPIO_RCAR_H__ + +struct gpio_rcar_config { +unsigned int gpio_base; +unsigned int irq_base; +unsigned int number_of_pins; +}; + +#endif /* __GPIO_RCAR_H__ */
Re: [PATCH 01/03] gpio: Renesas R-Car GPIO driver V2
Hi Magnus,
Thanks for the patch.
I've reviewed the result of squashing the 3 patches together, I just have one comment.
Quoted 41 lines · On Wednesday 13 March 2013 20:32:13 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
This patch is V2 of a GPIO driver for the R-Car series of SoCs from Renesas. This driver is designed to be reusable between multiple SoCs that share the same basic building block, but so far it has only been used on R-Car H1 (r8a7779).
Each driver instance handles 32 GPIOs with individually maskable IRQs. The driver operates on a single I/O memory range and the 32 GPIOs are hooked up a single interrupt.
In the case of R-Car H1 either external IRQ pins or GPIOs with interrupts can be used for on-board interupts. For external IRQs 4 pins are supported, and in the case of GPIO there are 202 GPIOS as 202 interrupts hooked up via 6 driver instances and to the GIC and the Cortex-A9 Quad.
At this point this driver is interfacing as a regular platform device driver. In the future DT support will be submitted as an incremental feature patch.
Signed-off-by Magnus Damm <[email protected]>---
Changes since V1:
- Update based on most suggestions from review by Laurent, thanks!Please note that in V2 the driver is reworked to reduce the number of lines and includes minor changes related to headers, printouts and data types. In V2 the register access order is kept the same as in V1, and to make that happen
IRQCHIP_SET_TYPE_MASKEDis omitted. The interface to the Linux kernel is unchanged except the use ofdev_dbg()instead ofpr_debug(). So from a hardware point of view V2 is a drop in replacement for V1 and there should be no additional dependencies since devm is kept as a separate patch as well. All this to make back porting easier.drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rcar.c | 383 ++++++++++++++++++++++++++++ include/linux/platform_data/gpio-rcar.h | 25 ++ 4 files changed, 415 insertions(+)
[snip]
Quoted 17 lines
drivers/gpio/gpio-rcar.c new file +14
-0@@ -0,0 +1,383 @@ +/* +* Renesas R-Car GPIO Support +* +* Copyright (C) 2013 Magnus Damm +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/
[snip]
drivers/gpio/gpio-rcar.c line 237
Quoted 11 lines
+static int gpio_rcar_irq_domain_map(struct
irq_domain*h, unsigned int virq,drivers/gpio/gpio-rcar.c new file +9
-0+ irq_hw_number_t hw) +{ +struct gpio_rcar_priv *p = h->host_data; + +dev_dbg (&p->pdev->dev,"map hw irq = %d, virq = %d\n" , (int )hw, virq); + +irq_set_chip_data (virq, h->host_data); +irq_set_chip_and_handler (virq, &p->irq_chip, handle_level_irq); +set_irq_flags (virq,IRQF_VALID );/* kill me now */
What is that comment about ?
Quoted 2 lines
drivers/gpio/gpio-rcar.c new file +2
-0+return 0 ; +}
Signature · 3 lines
Regards, Laurent Pinchart
Magnus Damm
maintainerRe: [PATCH 01/03] gpio: Renesas R-Car GPIO driver V2
Quoted 72 lines · Laurent Pinchart
Hi Magnus,
Thanks for the patch.
I've reviewed the result of squashing the 3 patches together, I just have one comment.
Quoted 41 lines · On Wednesday 13 March 2013 20:32:13 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
This patch is V2 of a GPIO driver for the R-Car series of SoCs from Renesas. This driver is designed to be reusable between multiple SoCs that share the same basic building block, but so far it has only been used on R-Car H1 (r8a7779).
Each driver instance handles 32 GPIOs with individually maskable IRQs. The driver operates on a single I/O memory range and the 32 GPIOs are hooked up a single interrupt.
In the case of R-Car H1 either external IRQ pins or GPIOs with interrupts can be used for on-board interupts. For external IRQs 4 pins are supported, and in the case of GPIO there are 202 GPIOS as 202 interrupts hooked up via 6 driver instances and to the GIC and the Cortex-A9 Quad.
At this point this driver is interfacing as a regular platform device driver. In the future DT support will be submitted as an incremental feature patch.
Signed-off-by Magnus Damm <[email protected]>---
Changes since V1:
- Update based on most suggestions from review by Laurent, thanks!Please note that in V2 the driver is reworked to reduce the number of lines and includes minor changes related to headers, printouts and data types. In V2 the register access order is kept the same as in V1, and to make that happen
IRQCHIP_SET_TYPE_MASKEDis omitted. The interface to the Linux kernel is unchanged except the use ofdev_dbg()instead ofpr_debug(). So from a hardware point of view V2 is a drop in replacement for V1 and there should be no additional dependencies since devm is kept as a separate patch as well. All this to make back porting easier.drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rcar.c | 383 ++++++++++++++++++++++++++++ include/linux/platform_data/gpio-rcar.h | 25 ++ 4 files changed, 415 insertions(+)[snip]
Quoted 17 lines
drivers/gpio/gpio-rcar.c 2013-03-13 19:41:35.000000000 +0900 new file +14
-0@@ -0,0 +1,383 @@ +/* + * Renesas R-Car GPIO Support + * + * Copyright (C) 2013 Magnus Damm + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */[snip]
Quoted 13 lines
drivers/gpio/gpio-rcar.c 2013-03-13 19:41:35.000000000 +0900
Quoted 11 lines
+static int gpio_rcar_irq_domain_map(struct
irq_domain*h, unsigned int virq,drivers/gpio/gpio-rcar.c 2013-03-13 19:41:35.000000000 +0900 new file +9
-0+ irq_hw_number_t hw) +{ + struct gpio_rcar_priv *p = h->host_data; + + dev_dbg(&p->pdev->dev, "map hw irq = %d, virq = %d\n", (int)hw, virq); + + irq_set_chip_data(virq, h->host_data); + irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq); + set_irq_flags(virq, IRQF_VALID); /* kill me now */What is that comment about ?
I seem to have a habit of putting that comment in all IRQ code that I write for ARM. See these threads for more info:
https://lkml.org/lkml/2013/2/19/246
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/008795.html
Cheers,
/ magnus
Magnus Damm
maintainerPATCH 2/3 gpio: rcar: Use IRQCHIP_SET_TYPE_MASKED
In reply to Magnus DammFrom: Magnus Damm <[email protected]>
Set the flag IRQCHIP_SET_TYPE_MASKED in gpio-rcar.c to force mask of the interrupt while configuring trigger settings.
---
drivers/gpio/gpio-rcar.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpio-rcar.c +1 -1
@@ -301,7 +301,7 @@ static int gpio_rcar_probe(struct platfo irq_chip->irq_enable = gpio_rcar_irq_enable; irq_chip->irq_disable = gpio_rcar_irq_disable; irq_chip->irq_set_type = gpio_rcar_irq_set_type; - irq_chip->flags =IRQCHIP_SKIP_SET_WAKE ; + irq_chip->flags =IRQCHIP_SKIP_SET_WAKE |IRQCHIP_SET_TYPE_MASKED ; p->irq_domain = irq_domain_add_simple(pdev->dev.of_node, p->config.number_of_pins,
Magnus Damm
maintainerPATCH 3/3 gpio: rcar: Make use of devm functions
In reply to Magnus DammFrom: Magnus Damm <[email protected]>
Update the gpio-rcar.c driver to make use of devm
functions. This simplifies the error handling and
makes the code more compact.
---
drivers/gpio/gpio-rcar.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-)
drivers/gpio/gpio-rcar.c +11 -21
@@ -252,7 +252,7 @@ static int gpio_rcar_probe(struct platfoconst char *name =dev_name (&pdev->dev);int ret; - p =kzalloc (sizeof (*p),GFP_KERNEL ); + p =devm_kzalloc (&pdev->dev,sizeof (*p),GFP_KERNEL );if (!p) {dev_err (&pdev->dev,"failed to allocate driver data\n" ); ret = -ENOMEM ;
@@ -273,14 +273,15 @@ static int gpio_rcar_probe(struct platfoif (!io || !irq) {dev_err (&pdev->dev,"missing IRQ or IOMEM\n" ); ret = -EINVAL ; - goto err1; + goto err0; } - p->base =ioremap_nocache (io->start,resource_size (io)); + p->base =devm_ioremap_nocache (&pdev->dev, io->start, +resource_size (io));if (!p->base) {dev_err (&pdev->dev,"failed to remap I/O memory\n" ); ret = -ENXIO ; - goto err1; + goto err0; } gpio_chip = &p->gpio_chip;
@@ -310,19 +311,20 @@ static int gpio_rcar_probe(struct platfoif (!p->irq_domain) { ret = -ENXIO ;dev_err (&pdev->dev,"cannot initialize irq domain\n" ); - goto err2; + goto err1; } -if (request_irq (irq->start, gpio_rcar_irq_handler,0 , name, p)) { +if (devm_request_irq (&pdev->dev, irq->start, + gpio_rcar_irq_handler,0 , name, p)) {dev_err (&pdev->dev,"failed to request IRQ\n" ); ret = -ENOENT ; - goto err3; + goto err1; } ret =gpiochip_add (gpio_chip);if (ret) {dev_err (&pdev->dev,"failed to add GPIO controller\n" ); - goto err4; + goto err1; }dev_info (&pdev->dev,"driving %d GPIOs\n" , p->config.number_of_pins);
@@ -337,14 +339,8 @@ static int gpio_rcar_probe(struct platforeturn 0 ; -err4: -free_irq (irq->start, p); -err3: -irq_domain_remove (p->irq_domain); -err2: -iounmap (p->base); err1: -kfree (p); +irq_domain_remove (p->irq_domain); err0:return ret; }
@@ -352,19 +348,13 @@ err0:static int gpio_rcar_remove (struct platform_device *pdev) {struct gpio_rcar_priv *p =platform_get_drvdata (pdev); -struct resource *irq;int ret; ret =gpiochip_remove (&p->gpio_chip);if (ret)return ret; - irq =platform_get_resource (pdev,IORESOURCE_IRQ ,0 ); - -free_irq (irq->start, p);irq_domain_remove (p->irq_domain); -iounmap (p->base); -kfree (p);return 0 ; }
Hi Magnus,
Thanks for the patches.
Quoted 6 lines · On Wednesday 13 March 2013 20:32:03 Magnus Damm wrote:
gpio: Renesas R-Car GPIO driver update
[PATCH 01/03] gpio: Renesas R-Car GPIO driver V2 [PATCH 02/03] gpio: rcar: Use
IRQCHIP_SET_TYPE_MASKED[PATCH 03/03] gpio: rcar: Make use of devm functions
Quoted 8 lines
This series updates the R-Car GPIO driver with various changes kindly suggested by Laurent Pinchart.
Patch [01/03] is a drop in replacement for V1 of the R-Car GPIO driver. The flag in patch [02/03] is kept out of [01/03] to avoid changing the behaviour of the driver between V1 and V2. Also, devm support in [03/03] is kept out of [01/03] to make sure back porting goes as smooth as possible.
As I mentioned in a previous e-mail, all the devm_* functions used in 03/03 have been available since 2.6.30. Do you really need to port that driver to older kernels ?
Regarding 02/03, do you plan to squash it with 01/03 for the mainline submission ?
Signature · 3 lines
Regards, Laurent Pinchart
Magnus Damm
maintainerHi Laurent, Simon,
Quoted 3 lines · Laurent Pinchart
Hi Magnus,
Thanks for the patches.
Thanks for your review!
Quoted 16 lines · On Wednesday 13 March 2013 20:32:03 Magnus Damm wrote:
Quoted 6 lines
gpio: Renesas R-Car GPIO driver update
[PATCH 01/03] gpio: Renesas R-Car GPIO driver V2 [PATCH 02/03] gpio: rcar: Use
IRQCHIP_SET_TYPE_MASKED[PATCH 03/03] gpio: rcar: Make use of devm functionsQuoted 8 lines
This series updates the R-Car GPIO driver with various changes kindly suggested by Laurent Pinchart.
Patch [01/03] is a drop in replacement for V1 of the R-Car GPIO driver. The flag in patch [02/03] is kept out of [01/03] to avoid changing the behaviour of the driver between V1 and V2. Also, devm support in [03/03] is kept out of [01/03] to make sure back porting goes as smooth as possible.
Quoted 3 lines
As I mentioned in a previous e-mail, all the devm_* functions used in 03/03 have been available since 2.6.30. Do you really need to port that driver to older kernels ?
Well, it was earlier suggested to me that not using devm to begin with is a safe way forward for back porting. Also, as the individual patch shows, we save about 10 lines of code but add many more complex dependencies.
Simon, do you have any recommendation how for us regarding devm and back porting?
Quoted 2 lines
Regarding 02/03, do you plan to squash it with 01/03 for the mainline submission ?
Not unless someone puts a gun to my head. =) Of course, if a single patch is really required then I will follow that, but I can't really see why when we have a nice versioning control system that can help our development in various ways.
What is the reason behind you wanting to merge these patches?
From my point of view, if any incremental patch was a bug fix then i would of course request to fold it in, but in this case these are feature patches that would be beneficial to keep as individual commits. Keeping them separate allows us to bisect and also makes partial back porting easier if needed.
Thanks,
/ magnus
Hi Magnus,
Quoted 37 lines · On Thursday 14 March 2013 13:23:46 Magnus Damm wrote:
Quoted 16 lines · Laurent Pinchart
Quoted 6 lines · On Wednesday 13 March 2013 20:32:03 Magnus Damm wrote:
gpio: Renesas R-Car GPIO driver update
[PATCH 01/03] gpio: Renesas R-Car GPIO driver V2 [PATCH 02/03] gpio: rcar: Use
IRQCHIP_SET_TYPE_MASKED[PATCH 03/03] gpio: rcar: Make use of devm functionsQuoted 8 lines
This series updates the R-Car GPIO driver with various changes kindly suggested by Laurent Pinchart.
Patch [01/03] is a drop in replacement for V1 of the R-Car GPIO driver. The flag in patch [02/03] is kept out of [01/03] to avoid changing the behaviour of the driver between V1 and V2. Also, devm support in [03/03] is kept out of [01/03] to make sure back porting goes as smooth as possible.
Quoted 3 lines
As I mentioned in a previous e-mail, all the devm_* functions used in 03/03 have been available since 2.6.30. Do you really need to port that driver to older kernels ?
Well, it was earlier suggested to me that not using devm to begin with is a safe way forward for back porting. Also, as the individual patch shows, we save about 10 lines of code but add many more complex dependencies.
Simon, do you have any recommendation how for us regarding devm and back porting?
Quoted 2 lines
Regarding 02/03, do you plan to squash it with 01/03 for the mainline submission ?
Not unless someone puts a gun to my head. =) Of course, if a single patch is really required then I will follow that, but I can't really see why when we have a nice versioning control system that can help our development in various ways.
Quoted 7 lines
What is the reason behind you wanting to merge these patches?
From my point of view, if any incremental patch was a bug fix then i would of course request to fold it in, but in this case these are feature patches that would be beneficial to keep as individual commits. Keeping them separate allows us to bisect and also makes partial back porting easier if needed.
When submitting new drivers I usually try not to make the development history visible to mainline. It brings little additional value (beside possibly making backporting a bit easier, but in the devm_* case that shouldn't be a problem, unless Simon thinks otherwise) but adds review complexity, as reviewers need to validate the intermediate versions as well. More patches also mean more potential bisection breakages.
In this case (assuming there would be no backporting issue) there's no need for mainline to see that we started with a version that didn't use devm_* and then modified the code. I see no added value in that.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 23 lines · Laurent Pinchart
Hi Magnus,
Quoted 20 lines · On Thursday 14 March 2013 13:23:46 Magnus Damm wrote:
Quoted 19 lines · Laurent Pinchart
Quoted 6 lines · On Wednesday 13 March 2013 20:32:03 Magnus Damm wrote:
gpio: Renesas R-Car GPIO driver update
[PATCH 01/03] gpio: Renesas R-Car GPIO driver V2 [PATCH 02/03] gpio: rcar: Use
IRQCHIP_SET_TYPE_MASKED[PATCH 03/03] gpio: rcar: Make use of devm functionsQuoted 8 lines
This series updates the R-Car GPIO driver with various changes kindly suggested by Laurent Pinchart.
Patch [01/03] is a drop in replacement for V1 of the R-Car GPIO driver. The flag in patch [02/03] is kept out of [01/03] to avoid changing the behaviour of the driver between V1 and V2. Also, devm support in [03/03] is kept out of [01/03] to make sure back porting goes as smooth as possible.
As I mentioned in a previous e-mail, all the devm_* functions used in 03/03 have been available since 2.6.30. Do you really need to port that driver to older kernels ?
Not that I am aware of.
Actually, I am not aware of any serious back-porting to anything older than LTSI-3.4.25.
Quoted 7 lines
Quoted 7 lines
Well, it was earlier suggested to me that not using devm to begin with is a safe way forward for back porting. Also, as the individual patch shows, we save about 10 lines of code but add many more complex dependencies.
Simon, do you have any recommendation how for us regarding devm and back porting?
I see devm_* in LTSI-3.4.25 so unless I am missing something I don't think there will be back-porting implications for you writing your code against those functions.
Quoted 23 lines
Quoted 8 lines
Quoted 2 lines
Regarding 02/03, do you plan to squash it with 01/03 for the mainline submission ?
Not unless someone puts a gun to my head. =) Of course, if a single patch is really required then I will follow that, but I can't really see why when we have a nice versioning control system that can help our development in various ways.
Quoted 7 lines
What is the reason behind you wanting to merge these patches?
From my point of view, if any incremental patch was a bug fix then i would of course request to fold it in, but in this case these are feature patches that would be beneficial to keep as individual commits. Keeping them separate allows us to bisect and also makes partial back porting easier if needed.
When submitting new drivers I usually try not to make the development history visible to mainline. It brings little additional value (beside possibly making backporting a bit easier, but in the devm_* case that shouldn't be a problem, unless Simon thinks otherwise) but adds review complexity, as reviewers need to validate the intermediate versions as well. More patches also mean more potential bisection breakages.
Quoted 3 lines
In this case (assuming there would be no backporting issue) there's no need for mainline to see that we started with a version that didn't use devm_* and then modified the code. I see no added value in that.
+1
Magnus Damm
maintainerHi Laurent,
Quoted 48 lines · Laurent Pinchart
Hi Magnus,
Quoted 37 lines · On Thursday 14 March 2013 13:23:46 Magnus Damm wrote:
Quoted 16 lines · Laurent Pinchart
Quoted 6 lines · On Wednesday 13 March 2013 20:32:03 Magnus Damm wrote:
gpio: Renesas R-Car GPIO driver update
[PATCH 01/03] gpio: Renesas R-Car GPIO driver V2 [PATCH 02/03] gpio: rcar: Use
IRQCHIP_SET_TYPE_MASKED[PATCH 03/03] gpio: rcar: Make use of devm functionsQuoted 8 lines
This series updates the R-Car GPIO driver with various changes kindly suggested by Laurent Pinchart.
Patch [01/03] is a drop in replacement for V1 of the R-Car GPIO driver. The flag in patch [02/03] is kept out of [01/03] to avoid changing the behaviour of the driver between V1 and V2. Also, devm support in [03/03] is kept out of [01/03] to make sure back porting goes as smooth as possible.
Quoted 3 lines
As I mentioned in a previous e-mail, all the devm_* functions used in 03/03 have been available since 2.6.30. Do you really need to port that driver to older kernels ?
Well, it was earlier suggested to me that not using devm to begin with is a safe way forward for back porting. Also, as the individual patch shows, we save about 10 lines of code but add many more complex dependencies.
Simon, do you have any recommendation how for us regarding devm and back porting?
Quoted 2 lines
Regarding 02/03, do you plan to squash it with 01/03 for the mainline submission ?
Not unless someone puts a gun to my head. =) Of course, if a single patch is really required then I will follow that, but I can't really see why when we have a nice versioning control system that can help our development in various ways.
Quoted 7 lines
What is the reason behind you wanting to merge these patches?
From my point of view, if any incremental patch was a bug fix then i would of course request to fold it in, but in this case these are feature patches that would be beneficial to keep as individual commits. Keeping them separate allows us to bisect and also makes partial back porting easier if needed.
Quoted 6 lines
When submitting new drivers I usually try not to make the development history visible to mainline. It brings little additional value (beside possibly making backporting a bit easier, but in the devm_* case that shouldn't be a problem, unless Simon thinks otherwise) but adds review complexity, as reviewers need to validate the intermediate versions as well. More patches also mean more potential bisection breakages.
Huh, it seems that my point of view is the total opposite. I see that using incremental patches to show new development would make review _easier_. Perhaps that's not the case for most people.
Regarding bisection, having features broken out actually allows us to bisect compared to a single commit. I see that as a feature. Of course the developer needs to make sure that the incremental changes do not cause any breakages, but if the developer can't do that then there are other more serious issues that need to be fixed IMO.
Quoted 3 lines
In this case (assuming there would be no backporting issue) there's no need for mainline to see that we started with a version that didn't use devm_* and then modified the code. I see no added value in that.
So say that you write a driver and add say 8 features on top of that over time, wouldn't it make sense to share that information with other people? That way any party can bisect and locate issues that may come up. I find that useful regardless if the code is back ported or not.
Anyway, with this particular driver it doesn't really matter since the complexity is very low. And now Simon has gotten back to us with updated information about back porting.
I will pull it all together into a V3.
Thanks,
/ magnus
Hi Magnus,
Quoted 64 lines · On Tuesday 19 March 2013 12:36:52 Magnus Damm wrote:
Quoted 46 lines · Laurent Pinchart
Quoted 37 lines · On Thursday 14 March 2013 13:23:46 Magnus Damm wrote:
Quoted 16 lines · Laurent Pinchart
Quoted 6 lines · On Wednesday 13 March 2013 20:32:03 Magnus Damm wrote:
gpio: Renesas R-Car GPIO driver update
[PATCH 01/03] gpio: Renesas R-Car GPIO driver V2 [PATCH 02/03] gpio: rcar: Use
IRQCHIP_SET_TYPE_MASKED[PATCH 03/03] gpio: rcar: Make use of devm functionsQuoted 8 lines
This series updates the R-Car GPIO driver with various changes kindly suggested by Laurent Pinchart.
Patch [01/03] is a drop in replacement for V1 of the R-Car GPIO driver. The flag in patch [02/03] is kept out of [01/03] to avoid changing the behaviour of the driver between V1 and V2. Also, devm support in [03/03] is kept out of [01/03] to make sure back porting goes as smooth as possible.
Quoted 3 lines
As I mentioned in a previous e-mail, all the devm_* functions used in 03/03 have been available since 2.6.30. Do you really need to port that driver to older kernels ?
Well, it was earlier suggested to me that not using devm to begin with is a safe way forward for back porting. Also, as the individual patch shows, we save about 10 lines of code but add many more complex dependencies.
Simon, do you have any recommendation how for us regarding devm and back porting?
Quoted 2 lines
Regarding 02/03, do you plan to squash it with 01/03 for the mainline submission ?
Not unless someone puts a gun to my head. =) Of course, if a single patch is really required then I will follow that, but I can't really see why when we have a nice versioning control system that can help our development in various ways.
Quoted 7 lines
What is the reason behind you wanting to merge these patches?
From my point of view, if any incremental patch was a bug fix then i would of course request to fold it in, but in this case these are feature patches that would be beneficial to keep as individual commits. Keeping them separate allows us to bisect and also makes partial back porting easier if needed.
Quoted 6 lines
When submitting new drivers I usually try not to make the development history visible to mainline. It brings little additional value (beside possibly making backporting a bit easier, but in the devm_* case that shouldn't be a problem, unless Simon thinks otherwise) but adds review complexity, as reviewers need to validate the intermediate versions as well. More patches also mean more potential bisection breakages.
Huh, it seems that my point of view is the total opposite. I see that using incremental patches to show new development would make review _easier_. Perhaps that's not the case for most people.
Regarding bisection, having features broken out actually allows us to bisect compared to a single commit. I see that as a feature. Of course the developer needs to make sure that the incremental changes do not cause any breakages, but if the developer can't do that then there are other more serious issues that need to be fixed IMO.
Quoted 8 lines
Quoted 3 lines
In this case (assuming there would be no backporting issue) there's no need for mainline to see that we started with a version that didn't use devm_* and then modified the code. I see no added value in that.
So say that you write a driver and add say 8 features on top of that over time, wouldn't it make sense to share that information with other people? That way any party can bisect and locate issues that may come up. I find that useful regardless if the code is back ported or not.
Long answer short: it depends :-)
If you add features incrementally, submitting them as individual patches totally make sense. However, if your patches aim at simplifying the code instead of adding features (such as devm_* usage), individual patches make it more complex in my opinion. Reviewers will need to look at the intermediate, more complex code, to make sure that no bisection breakage points will be introduced. Squashing the patches together then make review easier (as there's less code to review) and don't really hurt bisection, as the simplification patches usually remove code and thus bugs.
Quoted 5 lines
Anyway, with this particular driver it doesn't really matter since the complexity is very low. And now Simon has gotten back to us with updated information about back porting.
I will pull it all together into a V3.
Thank you :-)
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 10 lines · Magnus Damm
Quoted 6 lines · Laurent Pinchart
When submitting new drivers I usually try not to make the development history visible to mainline. It brings little additional value (beside possibly making backporting a bit easier, but in the devm_* case that shouldn't be a problem, unless Simon thinks otherwise) but adds review complexity, as reviewers need to validate the intermediate versions as well. More patches also mean more potential bisection breakages.
Quoted 3 lines
Huh, it seems that my point of view is the total opposite. I see that using incremental patches to show new development would make review _easier_. Perhaps that's not the case for most people.
As subsystem maintainer what I don't want to see is a patch that at one point breaks something in some configuration and then later fixes it. Then I strongly prefer squashing. (Greg also mentions this in one of his seminars.)
What really makes me mad is the the above pattern + claim that it must be done in that way to preserve authorship. Like legaleaze or credit is more important that functionality.
If all patches are bisectable and perfectly fine then whether you take 8 stops when driving to Rome or just drive there in one big stretch is more a technical, secondary thing. Do whatever you like as long as all commits build and boot.
Yours,
Linus Walleij
Magnus Damm
maintainerQuoted 11 lines · Linus Walleij
Quoted 10 lines · Magnus Damm
Quoted 6 lines · Laurent Pinchart
When submitting new drivers I usually try not to make the development history visible to mainline. It brings little additional value (beside possibly making backporting a bit easier, but in the devm_* case that shouldn't be a problem, unless Simon thinks otherwise) but adds review complexity, as reviewers need to validate the intermediate versions as well. More patches also mean more potential bisection breakages.
Quoted 8 lines
Quoted 3 lines
Huh, it seems that my point of view is the total opposite. I see that using incremental patches to show new development would make review _easier_. Perhaps that's not the case for most people.
As subsystem maintainer what I don't want to see is a patch that at one point breaks something in some configuration and then later fixes it. Then I strongly prefer squashing. (Greg also mentions this in one of his seminars.)
Sure, I totally agree. I strongly dislike when people introduce breakage and then fix it later in the same series. It's pretty obvious to me, each incremental step needs to work by itself - if it doesn't then it should be reworked before it gets merged. I personally prefer to separate features from fixes. Fixes are always folded into the original patch.
Quoted 8 lines
What really makes me mad is the the above pattern + claim that it must be done in that way to preserve authorship. Like legaleaze or credit is more important that functionality.
If all patches are bisectable and perfectly fine then whether you take 8 stops when driving to Rome or just drive there in one big stretch is more a technical, secondary thing. Do whatever you like as long as all commits build and boot.
I suppose I prefer to stop for coffee in every village then. =)
/ magnus