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

[Akshay Saraswat]

Hi Minkyu, Simon and Lukasz,

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

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.

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

Yes, next patch set would be v7.

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

Regards,
Akshay Saraswat