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