sh-pfc: Add r8a73a4 pinmux support
Hi Magnus,
Thanks for the patch.
Quoted 6 lines · On Monday 18 March 2013 23:50:39 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add initial PFC support for the r8a73a4 SoC.
At this point only GPIO interface is supported, move to newer interfaces planned as incremental changes.
I'm fine with splitting PFC support for the r8a73a4 into a first patch that implements the old API and incremental changes that move to the new API, to making partial backporting easier. However, to avoid increasing my already high work load related to the PFC driver, I don't want to push this patch to mainline without the incremental changes that convert the driver to the pinctrl API. Could you thus please submit a v2 that will include pinctrl support ?
(I believe this summarizes the discussion we had yesterday, please let me know if there had been any misunderstanding).
Quoted 7 lines
Original authors are Morimoto-san with help from Yoshii-san, thanks to them for the heavy lifting. Adjusted by Magnus to work together with updated code in
drivers/pinctrl.Signed-off-by Kuninori Morimoto <[email protected]>Signed-off-by Takashi Yoshii <[email protected]>Signed-off-by Magnus Damm <[email protected]>
The code looks OK to me. I don't have a copy of the r8a73a4 datasheet so I can't verify all the details.
Quoted 6 lines
---
Written against "next" renesas.git
811689afc214564c4a5f238ecf4d8bdc0e52b615
Requires "r8a73a4.h" provided by
[PATCH 00/04] ARM: shmobile: r8a73a4 SoC support V2
Quoted 7 lines
arch/arm/mach-shmobile/include/mach/r8a73a4.h | 918 ++++++++ drivers/pinctrl/sh-pfc/Kconfig | 5 drivers/pinctrl/sh-pfc/Makefile | 1 drivers/pinctrl/sh-pfc/core.c | 3 drivers/pinctrl/sh-pfc/core.h | 1 drivers/pinctrl/sh-pfc/pfc-r8a73a4.c | 2826 ++++++++++++++++++++++ 6 files changed, 3754 insertions(+)
Could you please split this in two patches, one for arch/arm and one for drivers/pinctrl ?
The following comments apply to the pinctrl API only, they're hints for the v2 mentioned above.
Quoted 6 lines
arch/arm/mach-shmobile/include/mach/r8a73a4.h +0
-022:38:11.000000000 +0900 @@ -1,6 +1,924 @@
#ifndef__ASM_R8A73A4_H__
#define__ASM_R8A73A4_H__
arch/arm/mach-shmobile/include/mach/r8a73a4.h
Quoted 10 lines
arch/arm/mach-shmobile/include/mach/r8a73a4.h +10
-0+/* +* Pin Function Controller: +* GPIO_FN_xx - GPIO used to select pin function +* GPIO_PORTxx - GPIO mapped to real I/O pin on CPU +*/ +enum { + +/* PORT */ +GPIO_PORT0 ,GPIO_PORT1 ,GPIO_PORT2 ,GPIO_PORT3 ,GPIO_PORT4 , +GPIO_PORT5 ,GPIO_PORT6 ,GPIO_PORT7 ,GPIO_PORT8 ,GPIO_PORT9 ,
[snip]
The whole enum should be removed when moving to the pinctrl API. Port numbers will be used directly instead of the GPIO_PORTx values.
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c
Quoted 2 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c new file +0
-0
22:44:53.000000000
[snip]
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c
Quoted 6 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c new file +6
-0+const struct sh_pfc_soc_info r8a73a4_pinmux_info = { + .name ="r8a73a4_pfc" , + + .input = {PINMUX_INPUT_BEGIN ,PINMUX_INPUT_END }, + .input_pu = {PINMUX_INPUT_PULLUP_BEGIN ,PINMUX_INPUT_PULLUP_END }, + .input_pd = {PINMUX_INPUT_PULLDOWN_BEGIN ,PINMUX_INPUT_PULLDOWN_END },
PU/PD should be handled as in
sh-pfc: sh73a0: Add bias (pull-up/down) pinconf support
sh-pfc: sh73a0: Remove pull-up function GPIOS
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c
Quoted 5 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c new file +5
-0+ .output = { PINMUX_OUTPUT_BEGIN, PINMUX_OUTPUT_END }, + .function = { PINMUX_FUNCTION_BEGIN, PINMUX_FUNCTION_END }, + + .pins = pinmux_pins, + .nr_pins = ARRAY_SIZE(pinmux_pins),
GPIO numbers are sparse, don't forget to add ranges as in
ARM: shmobile: sh73a0: Support sparse GPIO numbers
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c
Quoted 2 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c new file +2
-0+ .func_gpios = pinmux_func_gpios, + .nr_func_gpios = ARRAY_SIZE(pinmux_func_gpios),
You should add functions for all the functions used by board code. See for instance
sh-pfc: sh7372: Add SDHCI and MMCIF pin groups and functions
Finally you should remove support for function GPIOs as in the following patches (not in Simon's tree yet, they're available from my git tree atgit://linuxtv.org/pinchartl/fbdev.git pinmux/3.9/gpio)
sh-pfc: r8a7779: Remove function GPIO
sh-pfc: r8a7779: Don't use GPIO enum entries
ARM: shmobile: r8a7779: Remove all GPIOs
Quoted 6 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c new file +6
-0+ .cfg_regs = pinmux_config_regs, + .data_regs = pinmux_data_regs, + + .gpio_data = pinmux_data, + .gpio_data_size = ARRAY_SIZE(pinmux_data), +};
Signature · 3 lines
Regards, Laurent Pinchart
Hi Laurent,
Thanks for your comments.
Quoted 12 lines · Laurent Pinchart
Hi Magnus,
Thanks for the patch.
Quoted 6 lines · On Monday 18 March 2013 23:50:39 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add initial PFC support for the r8a73a4 SoC.
At this point only GPIO interface is supported, move to newer interfaces planned as incremental changes.
Quoted 7 lines
I'm fine with splitting PFC support for the r8a73a4 into a first patch that implements the old API and incremental changes that move to the new API, to making partial backporting easier. However, to avoid increasing my already high work load related to the PFC driver, I don't want to push this patch to mainline without the incremental changes that convert the driver to the pinctrl API. Could you thus please submit a v2 that will include pinctrl support ?
I am happy to add pinctrl support, especially when I get to control the timing myself.
Quoted 2 lines
(I believe this summarizes the discussion we had yesterday, please let me know if there had been any misunderstanding).
Yes, I believe it summarizes the discussion quite well. As for your requirement of incremental changes to get code merged, I hear what you're saying but after pondering about it a bit I must say this really doesn't make my life any easier. I would like to discuss more with you how to find a good middle ground without having to revert to primitive things like holding code hostage.
Quoted 10 lines
Quoted 7 lines
Original authors are Morimoto-san with help from Yoshii-san, thanks to them for the heavy lifting. Adjusted by Magnus to work together with updated code in
drivers/pinctrl.Signed-off-by Kuninori Morimoto <[email protected]>Signed-off-by Takashi Yoshii <[email protected]>Signed-off-by Magnus Damm <[email protected]>The code looks OK to me. I don't have a copy of the r8a73a4 datasheet so I can't verify all the details.
Thanks.
Quoted 14 lines
Quoted 6 lines
---
Written against "next" renesas.git
811689afc214564c4a5f238ecf4d8bdc0e52b615
Requires "r8a73a4.h" provided by
[PATCH 00/04] ARM: shmobile: r8a73a4 SoC support V2Quoted 7 lines
arch/arm/mach-shmobile/include/mach/r8a73a4.h | 918 ++++++++ drivers/pinctrl/sh-pfc/Kconfig | 5 drivers/pinctrl/sh-pfc/Makefile | 1 drivers/pinctrl/sh-pfc/core.c | 3 drivers/pinctrl/sh-pfc/core.h | 1 drivers/pinctrl/sh-pfc/pfc-r8a73a4.c | 2826 ++++++++++++++++++++++ 6 files changed, 3754 insertions(+)
Quoted 2 lines
Could you please split this in two patches, one for
arch/armand one fordrivers/pinctrl?
Do you mean this initial patch or the incremental PINCTRL ones? I've looked at your incremental r8a7779 changes and they seem to be implemented following such style.
I'm happy to split out changes the same way you're doing. But like I mentioned above, I dislike being forced to add these as a requirement for upstream merge. I can however live with it, and prioritize this over for instance pending paperwork, not my problem. What I really don't want is to invest time into something by splitting out code and then you will ask for it all to be merged together like in the case of GPIO.
Of course, with the same logic you can then squeeze together your own patches too and then everyone looses - especially anyone doing bisect. So I still think using a version control system with incremental changes has it's clear advantages, and if we can use it to track down the individual commit that's a nice feature to have. Especially when it comes to correctness about these things like pinmux tables.
Anyway, regarding splitting code and headers, I don't really understand why you want to split the header for the initial patch. I do however realize that there are dependencies that need to be handled. Can you outline exactly how you want it, and sync that with the party that will merge the code (Simon?)?
So to summarize, I will adjust to split the code any way the merging party wants it to be done. I'm not going to split it just for fun. =)
Quoted 22 lines
The following comments apply to the pinctrl API only, they're hints for the v2 mentioned above.
Quoted 16 lines
arch/arm/mach-shmobile/include/mach/r8a73a4.h 2013-03-18 renamed from arch/arm/mach-shmobile/include/mach/r8a73a4.h +0
-022:38:11.000000000 +0900 @@ -1,6 +1,924 @@
#ifndef__ASM_R8A73A4_H__
#define__ASM_R8A73A4_H__arch/arm/mach-shmobile/include/mach/r8a73a4.h 2013-03-18 renamed from arch/arm/mach-shmobile/include/mach/r8a73a4.h +10
-0+/* + * Pin Function Controller: + * GPIO_FN_xx - GPIO used to select pin function + * GPIO_PORTxx - GPIO mapped to real I/O pin on CPU + */ +enum { + + /* PORT */ + GPIO_PORT0, GPIO_PORT1, GPIO_PORT2, GPIO_PORT3, GPIO_PORT4, + GPIO_PORT5, GPIO_PORT6, GPIO_PORT7, GPIO_PORT8, GPIO_PORT9,[snip]
Quoted 2 lines
The whole enum should be removed when moving to the pinctrl API. Port numbers will be used directly instead of the
GPIO_PORTxvalues.
Right, thanks for clarifying.
Quoted 15 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18
Quoted 2 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18 new file +0
-022:44:53.000000000
[snip]
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18
Quoted 6 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18 new file +6
-0+const struct sh_pfc_soc_info r8a73a4_pinmux_info = { + .name = "r8a73a4_pfc", + + .input = { PINMUX_INPUT_BEGIN, PINMUX_INPUT_END }, + .input_pu = { PINMUX_INPUT_PULLUP_BEGIN, PINMUX_INPUT_PULLUP_END }, + .input_pd = { PINMUX_INPUT_PULLDOWN_BEGIN, PINMUX_INPUT_PULLDOWN_END },PU/PD should be handled as in
Quoted 2 lines
sh-pfc: sh73a0: Add bias (pull-up/down) pinconf support
sh-pfc: sh73a0: Remove pull-up function GPIOS
Ok, thanks for the pointer.
Quoted 9 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18
Quoted 5 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18 new file +5
-0+ .output = { PINMUX_OUTPUT_BEGIN, PINMUX_OUTPUT_END }, + .function = { PINMUX_FUNCTION_BEGIN, PINMUX_FUNCTION_END }, + + .pins = pinmux_pins, + .nr_pins = ARRAY_SIZE(pinmux_pins),GPIO numbers are sparse, don't forget to add ranges as in
ARM: shmobile: sh73a0: Support sparse GPIO numbers
Thanks.
Quoted 7 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18
Quoted 2 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18 new file +2
-0+ .func_gpios = pinmux_func_gpios, + .nr_func_gpios = ARRAY_SIZE(pinmux_func_gpios),You should add functions for all the functions used by board code. See for instance
sh-pfc: sh7372: Add SDHCI and MMCIF pin groups and functions
I understand. The current board code is rather limited, so I guess it should be quite easy.
Quoted 7 lines
Finally you should remove support for function GPIOs as in the following patches (not in Simon's tree yet, they're available from my git tree at
git://linuxtv.org/pinchartl/fbdev.gitpinmux/3.9/gpio)sh-pfc: r8a7779: Remove function GPIO
sh-pfc: r8a7779: Don't use GPIO enum entries
ARM: shmobile: r8a7779: Remove all GPIOs
Thanks for explaining how to move forward.
Cheers,
/ magnus
Hi Magnus,
Quoted 17 lines · On Tuesday 26 March 2013 15:29:22 Magnus Damm wrote:
Quoted 8 lines · Laurent Pinchart
Quoted 6 lines · On Monday 18 March 2013 23:50:39 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add initial PFC support for the r8a73a4 SoC.
At this point only GPIO interface is supported, move to newer interfaces planned as incremental changes.
Quoted 7 lines
I'm fine with splitting PFC support for the r8a73a4 into a first patch that implements the old API and incremental changes that move to the new API, to making partial backporting easier. However, to avoid increasing my already high work load related to the PFC driver, I don't want to push this patchto mainline without the incremental changes that convert the driver to the pinctrl API. Could you thus please submit a v2 that will include pinctrl support ?
Quoted 2 lines
I am happy to add pinctrl support, especially when I get to control the timing myself.
That means I will finally have an opportunity to sleep a bit, thank you :-)
Quoted 9 lines
Quoted 2 lines
(I believe this summarizes the discussion we had yesterday, please let me know if there had been any misunderstanding).
Yes, I believe it summarizes the discussion quite well. As for your requirement of incremental changes to get code merged, I hear what you're saying but after pondering about it a bit I must say this really doesn't make my life any easier. I would like to discuss more with you how to find a good middle ground without having to revert to primitive things like holding code hostage.
Sure, I'm open to discussion. Please keep in mind that this is an interim situation only, function GPIOs should soon turn into a distant memory of a bad nightmare for all new platforms :-)
Quoted 30 lines
Quoted 10 lines
Quoted 7 lines
Original authors are Morimoto-san with help from Yoshii-san, thanks to them for the heavy lifting. Adjusted by Magnus to work together with updated code in
drivers/pinctrl.Signed-off-by Kuninori Morimoto <[email protected]>Signed-off-by Takashi Yoshii <[email protected]>Signed-off-by Magnus Damm <[email protected]>The code looks OK to me. I don't have a copy of the r8a73a4 datasheet so I can't verify all the details.
Thanks.
Quoted 17 lines
Quoted 16 lines
---
Written against "next" renesas.git
811689afc214564c4a5f238ecf4d8bdc0e52b615Requires "
r8a73a4.h" provided by[PATCH 00/04] ARM: shmobile: r8a73a4 SoC support V2
arch/arm/mach-shmobile/include/mach/r8a73a4.h | 918 ++++++++ drivers/pinctrl/sh-pfc/Kconfig | 5 drivers/pinctrl/sh-pfc/Makefile | 1 drivers/pinctrl/sh-pfc/core.c | 3 drivers/pinctrl/sh-pfc/core.h | 1 drivers/pinctrl/sh-pfc/pfc-r8a73a4.c | 2826 +++++++++++++++++++ 6 files changed, 3754 insertions(+)
Quoted 6 lines
Quoted 2 lines
Could you please split this in two patches, one for
arch/armand one fordrivers/pinctrl?Do you mean this initial patch or the incremental PINCTRL ones? I've looked at your incremental r8a7779 changes and they seem to be implemented following such style.
I'm talking about this patch. My understanding is that patches should not touch arch/ and drivers/ at the same time unless strictly required.
Quoted 6 lines
I'm happy to split out changes the same way you're doing. But like I mentioned above, I dislike being forced to add these as a requirement for upstream merge. I can however live with it, and prioritize this over for instance pending paperwork, not my problem. What I really don't want is to invest time into something by splitting out code and then you will ask for it all to be merged together like in the case of GPIO.
My preference would of course to go straight to the pinctrl API without going through function GPIOs. As this would make partial backporting more complex I'm OK with splitting the code in initial patches that add PFC + function GPIOs support and incremental patches that replace function GPIOs with a proper pinctrl implementation, as in your v2.
Quoted 6 lines
Of course, with the same logic you can then squeeze together your own patches too and then everyone looses - especially anyone doing bisect. So I still think using a version control system with incremental changes has it's clear advantages, and if we can use it to track down the individual commit that's a nice feature to have. Especially when it comes to correctness about these things like pinmux tables.
I think there was a fundamental misunderstanding. I've never meant to advocate for all patches being squashed together. In the particular case of your GPIO patch set my point was that the intermediate version didn't bring any additional value and made the code more complex to review.
Quoted 5 lines
Anyway, regarding splitting code and headers, I don't really understand why you want to split the header for the initial patch. I do however realize that there are dependencies that need to be handled. Can you outline exactly how you want it, and sync that with the party that will merge the code (Simon?)?
Simon might be able to provide more information here. As far as I understand touching both arch/arm/ and drivers/ in the same patch is discouraged.
Quoted 79 lines
So to summarize, I will adjust to split the code any way the merging party wants it to be done. I'm not going to split it just for fun. =)
Quoted 23 lines
The following comments apply to the pinctrl API only, they're hints for the v2 mentioned above.
Quoted 17 lines
arch/arm/mach-shmobile/include/mach/r8a73a4.h 2013-03-18 renamed from arch/arm/mach-shmobile/include/mach/r8a73a4.h +0
-022:38:11.000000000 +0900 @@ -1,6 +1,924 @@
#ifndef
__ASM_R8A73A4_H__
#define__ASM_R8A73A4_H__arch/arm/mach-shmobile/include/mach/r8a73a4.h 2013-03-18 renamed from arch/arm/mach-shmobile/include/mach/r8a73a4.h +10
-0+/* + * Pin Function Controller: + * GPIO_FN_xx - GPIO used to select pin function + * GPIO_PORTxx - GPIO mapped to real I/O pin on CPU + */ +enum { + + /* PORT */ + GPIO_PORT0, GPIO_PORT1, GPIO_PORT2, GPIO_PORT3, GPIO_PORT4, + GPIO_PORT5, GPIO_PORT6, GPIO_PORT7, GPIO_PORT8, GPIO_PORT9,[snip]
Quoted 2 lines
The whole enum should be removed when moving to the pinctrl API. Port numbers will be used directly instead of the
GPIO_PORTxvalues.Right, thanks for clarifying.
Quoted 16 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18
Quoted 2 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18 new file +0
-022:44:53.000000000
[snip]
Quoted 7 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18 new file +6
-0+const struct sh_pfc_soc_info r8a73a4_pinmux_info = { + .name = "r8a73a4_pfc", + + .input = { PINMUX_INPUT_BEGIN, PINMUX_INPUT_END }, + .input_pu = { PINMUX_INPUT_PULLUP_BEGIN, PINMUX_INPUT_PULLUP_END }, + .input_pd = { PINMUX_INPUT_PULLDOWN_BEGIN,
PINMUX_INPUT_PULLDOWN_END},>PU/PD should be handled as in
Quoted 2 lines
sh-pfc: sh73a0: Add bias (pull-up/down) pinconf support
sh-pfc: sh73a0: Remove pull-up function GPIOSOk, thanks for the pointer.
Quoted 9 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18
Quoted 5 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18 new file +5
-0+ .output = { PINMUX_OUTPUT_BEGIN, PINMUX_OUTPUT_END }, + .function = { PINMUX_FUNCTION_BEGIN, PINMUX_FUNCTION_END }, + + .pins = pinmux_pins, + .nr_pins = ARRAY_SIZE(pinmux_pins),GPIO numbers are sparse, don't forget to add ranges as in
ARM: shmobile: sh73a0: Support sparse GPIO numbers
Thanks.
Quoted 7 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18
Quoted 2 lines
drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18 new file +2
-0+ .func_gpios = pinmux_func_gpios, + .nr_func_gpios = ARRAY_SIZE(pinmux_func_gpios),You should add functions for all the functions used by board code. See for instance
sh-pfc: sh7372: Add SDHCI and MMCIF pin groups and functions
I understand. The current board code is rather limited, so I guess it should be quite easy.
Quoted 4 lines
Finally you should remove support for function GPIOs as in the following patches (not in Simon's tree yet, they're available from my git tree at
git://linuxtv.org/pinchartl/fbdev.gitpinmux/3.9/gpio)
Quoted 5 lines
Quoted 3 lines
sh-pfc: r8a7779: Remove function GPIO
sh-pfc: r8a7779: Don't use GPIO enum entries
ARM: shmobile: r8a7779: Remove all GPIOsThanks for explaining how to move forward.
You're welcome.
Signature · 3 lines
Regards, Laurent Pinchart
Hi Laurent,
Quoted 20 lines · Laurent Pinchart
Hi Magnus,
Quoted 17 lines · On Tuesday 26 March 2013 15:29:22 Magnus Damm wrote:
Quoted 15 lines · Laurent Pinchart
Quoted 6 lines · On Monday 18 March 2013 23:50:39 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add initial PFC support for the r8a73a4 SoC.
At this point only GPIO interface is supported, move to newer interfaces planned as incremental changes.
I'm fine with splitting PFC support for the r8a73a4 into a first patch that implements the old API and incremental changes that move to the new API, to making partial backporting easier. However, to avoid increasing my already high work load related to the PFC driver, I don't want to push this patchto mainline without the incremental changes that convert the driver to the pinctrl API. Could you thus please submit a v2 that will include pinctrl support ?
Quoted 4 lines
Quoted 2 lines
I am happy to add pinctrl support, especially when I get to control the timing myself.
That means I will finally have an opportunity to sleep a bit, thank you :-)
You can sleep all you want when you're old! =)
Quoted 10 lines
Quoted 9 lines
Quoted 2 lines
(I believe this summarizes the discussion we had yesterday, please let me know if there had been any misunderstanding).
Yes, I believe it summarizes the discussion quite well. As for your requirement of incremental changes to get code merged, I hear what you're saying but after pondering about it a bit I must say this really doesn't make my life any easier. I would like to discuss more with you how to find a good middle ground without having to revert to primitive things like holding code hostage.
Quoted 3 lines
Sure, I'm open to discussion. Please keep in mind that this is an interim situation only, function GPIOs should soon turn into a distant memory of a bad nightmare for all new platforms :-)
I agree that new platforms should be converted. Regarding nightmares, you should have seen the PFC setup code that was spread out across the board code soon 5 years ago before I introduced (the now hopefully soon replaced) function GPIO abstraction. =)
Quoted 33 lines
Quoted 33 lines
Quoted 10 lines
Quoted 7 lines
Original authors are Morimoto-san with help from Yoshii-san, thanks to them for the heavy lifting. Adjusted by Magnus to work together with updated code in
drivers/pinctrl.Signed-off-by Kuninori Morimoto <[email protected]>Signed-off-by Takashi Yoshii <[email protected]>Signed-off-by Magnus Damm <[email protected]>The code looks OK to me. I don't have a copy of the r8a73a4 datasheet so I can't verify all the details.
Thanks.
Quoted 19 lines
Quoted 9 lines
---
Written against "next" renesas.git
811689afc214564c4a5f238ecf4d8bdc0e52b615Requires "
r8a73a4.h" provided by[PATCH 00/04] ARM: shmobile: r8a73a4 SoC support V2
Quoted 7 lines
arch/arm/mach-shmobile/include/mach/r8a73a4.h | 918 ++++++++ drivers/pinctrl/sh-pfc/Kconfig | 5 drivers/pinctrl/sh-pfc/Makefile | 1 drivers/pinctrl/sh-pfc/core.c | 3 drivers/pinctrl/sh-pfc/core.h | 1 drivers/pinctrl/sh-pfc/pfc-r8a73a4.c | 2826 +++++++++++++++++++ 6 files changed, 3754 insertions(+)Could you please split this in two patches, one for
arch/armand one fordrivers/pinctrl?
Quoted 6 lines
Quoted 3 lines
Do you mean this initial patch or the incremental PINCTRL ones? I've looked at your incremental r8a7779 changes and they seem to be implemented following such style.
I'm talking about this patch. My understanding is that patches should not touch
arch/anddrivers/at the same time unless strictly required.
I agree that it's nice as long as you can avoid that.
Actually, I would prefer not to not have the enums in that location at all. When the PFC tables were kept under arch/arm/mach-shmobile I didn't mind so much having the PFC enums in include files under mach/, but once they were moved out into driver/pinctrl/sh-pfc/ we still seemed to keep those includes under mach/. Of course, the goal is to remove the enums in the long run so perhaps it doesn't make much sense to first move them to soon after remove them.
Quoted 7 lines
Quoted 6 lines
I'm happy to split out changes the same way you're doing. But like I mentioned above, I dislike being forced to add these as a requirement for upstream merge. I can however live with it, and prioritize this over for instance pending paperwork, not my problem. What I really don't want is to invest time into something by splitting out code and then you will ask for it all to be merged together like in the case of GPIO.
Quoted 5 lines
My preference would of course to go straight to the pinctrl API without going through function GPIOs. As this would make partial backporting more complex I'm OK with splitting the code in initial patches that add PFC + function GPIOs support and incremental patches that replace function GPIOs with a proper pinctrl implementation, as in your v2.
I would also prefer to go straight to the pinctrl API, but as you said about the back porting...
Quoted 11 lines
Quoted 6 lines
Of course, with the same logic you can then squeeze together your own patches too and then everyone looses - especially anyone doing bisect. So I still think using a version control system with incremental changes has it's clear advantages, and if we can use it to track down the individual commit that's a nice feature to have. Especially when it comes to correctness about these things like pinmux tables.
I think there was a fundamental misunderstanding. I've never meant to advocate for all patches being squashed together. In the particular case of your GPIO patch set my point was that the intermediate version didn't bring any additional value and made the code more complex to review.
Ok, I think I understand better now.
I'm not sure, but you may find it odd that I broke out that particular devm patch. But I've done so for other drivers earlier without any review problems. Also, please note that I kept the devm stuff separate because I was earlier recommended to do so to minimize back porting dependencies.
Good thing was that you did clarify that such separation wasn't needed anymore, so in the end it turned out that folding it in didn't hurt.
Quoted 3 lines
Quoted 1 line
Thanks for explaining how to move forward.
You're welcome.
Thanks for your help!
/ magnus