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

[Przemyslaw Marczak]

Hi,
I missed this email before.

On 04/14/2014 04:55 PM, Akshay Saraswat wrote:
> 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.
>

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

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

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