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 "proms...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot