EXYNOS5: Add gpio pin numbering feature

PATCH Superseded Akshay Saraswat 2 participants 2 messages Apr 14, 2014

lore

Share & tools

lore

Akshay Saraswat

author

2014-04-14 14:55 UTC #0 lore raw

PATCH v6 1/4 EXYNOS5: Add gpio pin numbering feature

Hi Minkyu, Simon and Lukasz,

Quoted 51 lines

Dear Akshay,
Quoted 13 lines · Lukasz Majewski
Hi Akshay,
Quoted 11 lines
Hi Lukasz,
Quoted 8 lines
Hi Akshay,
I'm not Samsung tree maintainer, but by chance I've come across those patches and...
First question - why have you omitted u-boot-samsung tree maintainer? I've added Minkyu to CC.
Quoted 7 lines
Quoted 2 lines
Minkyu has an email ID "promsoft at gmail.com" and I added that in CC. Probably you don't know this email id :-)
I do know it :-), but this is not the official one.
Adding involved people to CC really speed up things :-)
I am always sensing about SAMSUNG related patches. Please don't worry about it :)
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Also in the cover letter you claim that this patch was "build tested" for Exynos4 based boards. Why didn't you add at least one maintainer of those boards to CC?
Quoted 8 lines
Quoted 5 lines
In cover letter I have not mentioned anywhere that I have built or tested these patches over Exynos4.
Patch 2/4 says "Build tested"
because Rajeshwari did build images for Exynos4 boards and that was successfull but nobody tested booting those images.
As Przemek written to you in the other mail. There is a build problem with trats2/trats boards.
Please fix it.
Quoted 6 lines
Quoted 2 lines
I do not possess any Exynos4 board.
That is why it is a good practice to ask maintainer's of those boards to test it for you.

Quoted 8 lines

Quoted 2 lines
Quoted 2 lines
These patches are meant for
Exynos5 only.
No. please consider Exynos4 also.
If you make patches for Exynos4 too, then it will be great job. Przemek or Lukasz will help your test.
Quoted 1 line

I borrowed an Origen board and doing changes for Exynos4 as well. I'll push next patch-set tomorrow with Exynos4 and 5 support together.

Quoted 9 lines

Quoted 6 lines
We will probably go with your approach to make (_finally_) the gpio code consistent for Exynos4/5.
Quoted 3 lines
But Yes, there are compiler errors introduced for
smkc100 because of this new patch-set and I will fix them in the next patch-set.
next patch-set means v7? right?

Yes, next patch set would be v7.

Quoted 103 lines

Quoted 51 lines
I'm a bit confused now. Was this patch set build tested or not?
Quoted 17 lines
Quoted 16 lines
Review
Quoted 11 lines
(quoted diff) +2 -0
+
+/* A list of valid GPIO numbers for the asm-generic/gpio.h
interface */ +enum exynos5_gpio_pin {
(quoted diff) +6 -0
+	/* GPIO_PART1_STARTS */
+	EXYNOS5_GPIO_A00,	/* 0 */
+	EXYNOS5_GPIO_A01,
+	EXYNOS5_GPIO_A02,
+	EXYNOS5_GPIO_A03,
+	EXYNOS5_GPIO_A04,
According to the patch description, you had a compilation error when were adding the support for Exynos 5250 and 5420. Why you fix the problem by rewriting the whole framework?
Quoted 3 lines
This framework is not intended to fix compiler warnings or errors but to make GPIO numbering easy to remember and sequential, without any holes in between.
Samsung boards were swinging from part+bank+pin number approach to sequential GPIO number from time to time. I think it is a good time to clean things up.
Quoted 11 lines
Quoted 11 lines
IN the patch 2/4 you have:
(quoted diff) +3 -3
-		gpio_cfg_pin(start + i, GPIO_FUNC(0x2));
-		gpio_set_pull(start + i, GPIO_PULL_NONE);
-		gpio_set_drv(start + i, GPIO_DRV_4X);
+		gpio_cfg_pin(start + i, S5P_GPIO_FUNC(0x2));
+		gpio_set_pull(start + i, S5P_GPIO_PULL_NONE);
+		gpio_set_drv(start + i, S5P_GPIO_DRV_4X);
Quoted 11 lines
Quoted 4 lines
What is the rationale to change the name to S5P_GPIO and not stick to GPIO_FUNC? In which way gpios for Exynos5 are different than for Exynos4? Cannot we finally reuse the Exynos 4 and 5 code?
We have enum member GPIO_INPUT in common/cmd_gpio.c and GPIO_INPUT define in arch-exynos/gpio.h. To remove such conflicts we renamed all s5p defines from "GPIO_*" to "S5P_GPIO_*".
We are using the same s5p_gpio.c for both Exynos4 and 5 as far as I know. I dont get the exact issue here. Do you want me to remove "S5P_". Is that it ?
Quoted 3 lines
S5P_ corresponds to at most Exynos4 SoC (Up till S5PV310). However, since the file is named s5p_gpio.c, then I think that S5P_ is a appropriate prefix.
Actually I have plan to rename from S5P_ to EXYNOS_. but not now. It look OK to me.
Quoted 45 lines
Quoted 10 lines
Quoted 10 lines
With the same patch:
(quoted diff) +1 -3
-  	case PERIPH_ID_UART1:
-		bank = &gpio1->d0;
-		start = 0;
+		start = EXYNOS5_GPIO_D00;
Quoted 6 lines
Quoted 3 lines
What is wrong with specifying the bank field? Why your gpio command cannot use the bank approach?
Ultimately we are using banks and pin_nums specific to the bank only after we extract exact bank from the sequential pin_num.
Ok.
Quoted 7 lines
Quoted 7 lines
And one more question: Is this work compliant with new driver model, which will be accepted at the merge window after the v2014.04 release?
Quoted 5 lines
Quoted 3 lines
If not, then there is no point to review this code, since GPIO would need to be adjusted to use this framework.
Please explain more. I don't get this as well :-)
My point is that the new driver model (introduced by Simon) is going to be included. I'm concern if after introduction of it we would need to rewrite the gpio code to comply with it.
Quoted 9 lines
Quoted 8 lines
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
Regards,
Akshay Saraswat
Thanks,
Minkyu Kang.

Regards,
Akshay Saraswat

Przemyslaw Marczak

2014-04-14 15:23 UTC #1 lore raw

PATCH v6 1/4 EXYNOS5: Add gpio pin numbering feature

Hi,
I missed this email before.

Quoted 56 lines · Akshay Saraswat

Hi Minkyu, Simon and Lukasz,
Quoted 54 lines
Dear Akshay,
Quoted 13 lines · Lukasz Majewski
Hi Akshay,
Quoted 11 lines
Hi Lukasz,
Quoted 8 lines
Hi Akshay,
I'm not Samsung tree maintainer, but by chance I've come across those patches and...
First question - why have you omitted u-boot-samsung tree maintainer? I've added Minkyu to CC.
Quoted 7 lines
Quoted 2 lines
Minkyu has an email ID "promsoft at gmail.com" and I added that in CC. Probably you don't know this email id :-)
I do know it :-), but this is not the official one.
Adding involved people to CC really speed up things :-)
I am always sensing about SAMSUNG related patches. Please don't worry about it :)
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Also in the cover letter you claim that this patch was "build tested" for Exynos4 based boards. Why didn't you add at least one maintainer of those boards to CC?
Quoted 8 lines
Quoted 5 lines
In cover letter I have not mentioned anywhere that I have built or tested these patches over Exynos4.
Patch 2/4 says "Build tested"
because Rajeshwari did build images for Exynos4 boards and that was successfull but nobody tested booting those images.
As Przemek written to you in the other mail. There is a build problem with trats2/trats boards.
Please fix it.
Quoted 8 lines
Quoted 2 lines
I do not possess any Exynos4 board.
That is why it is a good practice to ask maintainer's of those boards to test it for you.
Quoted 2 lines
These patches are meant for
Exynos5 only.

Quoted 9 lines

Quoted 5 lines
No. please consider Exynos4 also.
If you make patches for Exynos4 too, then it will be great job. Przemek or Lukasz will help your test.
Quoted 1 line
I borrowed an Origen board and doing changes for Exynos4 as well. I'll push next patch-set tomorrow with Exynos4 and 5 support together.

Great. Please look at my comments. If you finish work with your patches then I add the same feature for s5pc1xx.

Quoted 121 lines

Quoted 9 lines
Quoted 6 lines
We will probably go with your approach to make (_finally_) the gpio code consistent for Exynos4/5.
Quoted 3 lines
But Yes, there are compiler errors introduced for
smkc100 because of this new patch-set and I will fix them in the next patch-set.
next patch-set means v7? right?
Yes, next patch set would be v7.
Quoted 109 lines
Quoted 51 lines
I'm a bit confused now. Was this patch set build tested or not?
Quoted 17 lines
Quoted 16 lines
Review
Quoted 11 lines
(quoted diff) +2 -0
+
+/* A list of valid GPIO numbers for the asm-generic/gpio.h
interface */ +enum exynos5_gpio_pin {
(quoted diff) +6 -0
+	/* GPIO_PART1_STARTS */
+	EXYNOS5_GPIO_A00,	/* 0 */
+	EXYNOS5_GPIO_A01,
+	EXYNOS5_GPIO_A02,
+	EXYNOS5_GPIO_A03,
+	EXYNOS5_GPIO_A04,
According to the patch description, you had a compilation error when were adding the support for Exynos 5250 and 5420. Why you fix the problem by rewriting the whole framework?
Quoted 3 lines
This framework is not intended to fix compiler warnings or errors but to make GPIO numbering easy to remember and sequential, without any holes in between.
Samsung boards were swinging from part+bank+pin number approach to sequential GPIO number from time to time. I think it is a good time to clean things up.
Quoted 11 lines
Quoted 11 lines
IN the patch 2/4 you have:
(quoted diff) +3 -3
-		gpio_cfg_pin(start + i, GPIO_FUNC(0x2));
-		gpio_set_pull(start + i, GPIO_PULL_NONE);
-		gpio_set_drv(start + i, GPIO_DRV_4X);
+		gpio_cfg_pin(start + i, S5P_GPIO_FUNC(0x2));
+		gpio_set_pull(start + i, S5P_GPIO_PULL_NONE);
+		gpio_set_drv(start + i, S5P_GPIO_DRV_4X);
Quoted 11 lines
Quoted 4 lines
What is the rationale to change the name to S5P_GPIO and not stick to GPIO_FUNC? In which way gpios for Exynos5 are different than for Exynos4? Cannot we finally reuse the Exynos 4 and 5 code?
We have enum member GPIO_INPUT in common/cmd_gpio.c and GPIO_INPUT define in arch-exynos/gpio.h. To remove such conflicts we renamed all s5p defines from "GPIO_*" to "S5P_GPIO_*".
We are using the same s5p_gpio.c for both Exynos4 and 5 as far as I know. I dont get the exact issue here. Do you want me to remove "S5P_". Is that it ?
Quoted 3 lines
S5P_ corresponds to at most Exynos4 SoC (Up till S5PV310). However, since the file is named s5p_gpio.c, then I think that S5P_ is a appropriate prefix.
Actually I have plan to rename from S5P_ to EXYNOS_. but not now. It look OK to me.
Quoted 50 lines
Quoted 10 lines
Quoted 10 lines
With the same patch:
(quoted diff) +1 -3
-  	case PERIPH_ID_UART1:
-		bank = &gpio1->d0;
-		start = 0;
+		start = EXYNOS5_GPIO_D00;
Quoted 6 lines
Quoted 3 lines
What is wrong with specifying the bank field? Why your gpio command cannot use the bank approach?
Ultimately we are using banks and pin_nums specific to the bank only after we extract exact bank from the sequential pin_num.
Ok.
Quoted 7 lines
Quoted 7 lines
And one more question: Is this work compliant with new driver model, which will be accepted at the merge window after the v2014.04 release?
Quoted 5 lines
Quoted 3 lines
If not, then there is no point to review this code, since GPIO would need to be adjusted to use this framework.
Please explain more. I don't get this as well :-)
My point is that the new driver model (introduced by Simon) is going to be included. I'm concern if after introduction of it we would need to rewrite the gpio code to comply with it.
Quoted 11 lines
Quoted 8 lines
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
Regards,
Akshay Saraswat

Quoted 7 lines

Quoted 3 lines
Thanks,
Minkyu Kang.
Regards,
Akshay Saraswat

Thanks!

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com