From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Mon, 14 Apr 2014 17:23:20 +0200 Subject: [U-Boot] [PATCH v6 1/4] EXYNOS5: Add gpio pin numbering feature In-Reply-To: <1397487356-8632-1-git-send-email-akshay.s@samsung.com> References: <1397487356-8632-1-git-send-email-akshay.s@samsung.com> Message-ID: <534BFD68.8060808@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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