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

[Minkyu Kang <mk7.kang@samsung.com>]

Dear Akshay,

On 14/04/14 19:53, Lukasz Majewski wrote:
> 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 :-)
> 

I am always sensing about SAMSUNG related patches.
Please don't worry about it :)

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

Please fix it.

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

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.

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

next patch-set means v7? right?

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

Actually I have plan to rename from S5P_ to EXYNOS_. but not now.
It look OK to me.

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

Thanks,
Minkyu Kang.