[U-Boot] [PATCH v6 1/4] EXYNOS5: Add gpio pin numbering feature

[Lukasz Majewski]

Hi Akshay,

> Hi Lukasz,
> 
> >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.
> >
> 
> 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 :-)

> 
> >
> >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?
> >
> 
> 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.

> 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. 

> These patches are meant for
> Exynos5 only.

We will probably go with your approach to make (_finally_) the gpio code
consistent for Exynos4/5.

> 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.

I'm a bit confused now. Was this patch set build tested or not? 

> 
> >
> >> +
> >> +/* A list of valid GPIO numbers for the asm-generic/gpio.h
> >> interface */ +enum exynos5_gpio_pin {
> >> +	/* 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?
> >
> 
> 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. 

> 
> >
> >IN the patch 2/4 you have:
> >
> >-		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);
> >
> >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 ?

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.

> 
> >
> >With the same patch:
> >
> >-  	case PERIPH_ID_UART1:
> >-		bank = &gpio1->d0;
> >-		start = 0;
> >+		start = EXYNOS5_GPIO_D00;
> >
> >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.

> 
> >
> >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?
> >
> >
> >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.

> 
> >--
> >Best regards,
> >
> >Lukasz Majewski
> >
> >Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> >
> 
> Regards,
> Akshay Saraswat



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group