From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754842AbcIEIIi convert rfc822-to-8bit (ORCPT ); Mon, 5 Sep 2016 04:08:38 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:60556 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754654AbcIEIIc (ORCPT ); Mon, 5 Sep 2016 04:08:32 -0400 X-AuditID: cbfee68d-f79286d000007a9a-67-57cd27fd5d91 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 8BIT Message-id: <57CD27FC.2020201@samsung.com> Date: Mon, 05 Sep 2016 17:08:28 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Tomasz Figa Cc: =?UTF-8?B?S3J6eXN6dG9mIEtvesWCb3dza2k=?= , Kukjin Kim , Rob Herring , Mark Rutland , Catalin Marinas , Will Deacon , devicetree , linux-arm-kernel , "linux-samsung-soc@vger.kernel.org" , linux-kernel , Krzysztof Kozlowski , Jaehoon Chung , "sw0312.kim" , Joonyoung Shim , InKi Dae , Jonghwa Lee , Beomho Seo , jaewon02.kim@samsung.com, human.hwang@samsung.com, Inha Song , ingi2.kim@samsung.com, Marek Szyprowski , Andrzej Hajda , Sylwester Nawrocki , chanwoo@kernel.org, Linus Walleij , "linux-gpio@vger.kernel.org" Subject: Re: [PATCH v2 3/7] pinctrl: samsung: Add the support the multiple IORESOURCE_MEM for one pin-bank References: <1472046551-703-1-git-send-email-cw00.choi@samsung.com> <1472046551-703-4-git-send-email-cw00.choi@samsung.com> In-reply-to: X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa0iTURzGOXtvUzTf1tTjyMqRCJWV144lWdCHERWBWCGULXtR8b5pZBeU 0MxL87IRY7VuqOXUtKk1tdLWItG8ZelWNq3F0Cy65116X/chvz38zv//POc5HD4maCZE/ISU DEaWIk0Sk854rTA43X/Br+fw1g92MXp7r5dA3T8fUOhbVTFAZebXOLphYlmlWkGg1oVRgAyW 3yQqHy3FkSHHRCDz7EUCFfR8wtC4dQBHk+MBqMQ2iaG+vgYKqebv8JDeNkSgxhvzAA22XiOR uu8JD9WZ3lOocniAh/Iemyj07Es+gdTKCRLpWtm5iR+d+K7VktrrtUDSonlPSfS6AlIyMvSI lDRWZEsUTTog+aVfc5CKdg4/ySQlnGJkW3Yed47XNo4RadVrTysGm4kcMOFRCPh8SAfDP6pD hcCJlR6w31pPclpA3wWw1BTh4MGwZeojrxA4s1wD4ExbP+AOXOmVcFppxTkfjPaDKlUyhzFa CGsu3ScceiOsujWJOXbHACwxFJCO3Q1woaOTx2mc9oXt91uXPEmWt4+bl2bcaB/4ZtoGOH93 +ggs6szisJCN+qsqoThPjP7Mhy0zhqWwVXQSfLaYSznCRnnwxU8VxS070ZFw7oKQ45DOd4Kj dUrCEUzDv0oj7ngIb6jvwByFveDTu2a8FEDNspqa/zU1y2pqltW8CXAdcGfSYtPkJ+JkAZvl 0mR5Zkrc5tjUZD1gf1P3ov2yAbzr2GEENB+IXVz7514eFhDSU/KsZCMIYS9UhoncY1PZD5iS ERMQFBqIQoJDggK3hYWKPV19RDORAjpOmsEkMkwaI4uRZSYxciPg8Z1EOaC9BAZVFR99nr1p /5hfVLVV7RsV9lRw3qWpZqSrJW+vxceYWtd07ljDmXWWyZGvT6JrusKv7uutlimu9kT8stiG 6xNF36/cmotLsNm1m7Jt/pmvfG9qc+bX3y7au3tC7Zb6YMq77UC8x8ft5SvCz+4Zbz4Y7RUY o02f9cxV2NNhxUMxLo+XBmzAZHLpP4sq7qBIAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrFKsWRmVeSWpSXmKPExsVy+t9jQd2/6mfDDf7slbS4te4cq8XpT9vY Ld4v62G0mHjjCovF/CNAsaUz+lgtdv29z2ix4+YXNotJ9yewWOxoOMJqceNXG6tF59knzBYv 7l1ksXj9wtCi//FrZovz5zewW0z5s5zJYtPja6wWm+f/YbS4vGsOm8WM8/uYLNYeuctusfT6 RSaL1r1H2C0Ov2lntZgx+SWbxapdQHUvP55gcZDxWDNvDaPHzll32T02repk87hzbQ+bx+Yl 9R59W1YxenzeJBfAHtXAaJORmpiSWqSQmpecn5KZl26r5B0c7xxvamZgqGtoaWGupJCXmJtq q+TiE6DrlpkD9LiSQlliTilQKCCxuFhJ3w7ThNAQN10LmMYIXd+QILgeIwM0kLCGMWPxw7Ws BT/lKnbu/8HWwLhTrIuRk0NCwERi5/dHTBC2mMSFe+vZuhi5OIQEZjFK/Nx9gREkwSsgKPFj 8j2WLkYODmYBeYkjl7IhTHWJKVNyIcofMEr07+hkgyjXkvh74ATYTBYBVYn9G3eBjWEDiu9/ cQOshl9AUeLqj8eMIHNEBSIkuk9UgoRFgEZ+m9LPDjKTWeAVh8TOnztYQRLCAjkSh/+1sEMs u88kcfzTFHaQZk6BYInfTSITGAVnIbl0FsKlsxAuXcDIvIpRIrUguaA4KT3XMC+1XK84Mbe4 NC9dLzk/dxMjOJ09k9rBeHCX+yFGAQ5GJR7eCbpnwoVYE8uKK3MPMUpwMCuJ8KqonQ0X4k1J rKxKLcqPLyrNSS0+xGgK9OpEZinR5Hxgqs0riTc0NjEzsjQyN7QwMjZXEud9/H9dmJBAemJJ anZqakFqEUwfEwenVAPj9vMvPPSXf41rc2rOXbjQ/pfZoa92mQkrcq9ePDX9BfutiPs7ctwO K9vZRT9zWZNsKvJ2ibyuS800aZ3oJQ2G2xZs0/P/yBnxjqV0gmXzul9H6ixvr7MuYM9NbZ1r dFry24o5wv8P5OR7XY24oDnXPEBT2yvqF/Oy5TPsiptVmmsydniXLb2qxFKckWioxVxUnAgA t0CUvn0DAAA= DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Tomasz, I'm sorry for late reply. On 2016년 08월 25일 23:41, Tomasz Figa wrote: > 2016-08-25 23:30 GMT+09:00 Tomasz Figa : >>> + } >>> + >>> +#define EXYNOS_PIN_BANK_EINTN_EXT(pins, reg, id, pctl_idx, eint_idx) \ >>> + { \ >>> + .type = &bank_type_off, \ >>> + .pctl_offset = reg, \ >>> + .nr_pins = pins, \ >>> + .eint_type = EINT_TYPE_NONE, \ >>> + .name = id, \ >>> + .pctl_res_idx = pctl_idx, \ >>> + .eint_res_idx = eint_dix \ >>> + } >> >> Your patch 4/7 doesn't seem to use this one, so this is dead code for >> the time being. Please add when there is real need for it. >> >> Also it doesn't really make much sense to have index for both pctl and >> eint. Please define first entry of regs property as always pointing to >> pctl registers and by also eint registers for usual controllers. Then >> second regs entry would be eint registers for controllers with >> separate register blocks. Then there is only a need to have >> eint_res_idx in the driver and no need for pctl_res_idx, because it >> would be always 0. > > Ah, sorry, I got confused again by which registers are where in these > GPF banks. Let's make it the other way around and make DT contain eint > registers in first regs entry and hardcode eint_res_idx to 0 for the > time being. I got with slight confusion. Do you mean that you want to remove the 'eint_res_idx' because it is always zero(0) as your comment. And do you agree to add 'pctl_res_idx'? Also, as you commented, the eint_res_idx for both GPA and GPFx is zero(0). Example: pinctrl_alive: pinctrl@10580000 { compatible = "samsung,exynos5433-pinctrl"; /* ALIVE domain , IMEM domain */ reg = <0x10580000 0x1a20>, <0x11090000 0x100>; wakeup-interrupt-controller { compatible = "samsung,exynos7-wakeup-eint"; interrupts = ; }; }; GPA's eint_res_idx is 0 GPA's pctl_res_idx is 0 GPFx's eint_res_idx is 0 GPFx's pctl_res_idx is 1 However it should be still beneficial to refactor the code > and calculate per-bank eint_base to avoid adding the offset every > time, similarly to pctl_base/offset, from my suggestion below. I agree. I'll modify it according to your comment. > >>> @@ -345,7 +346,8 @@ static void pin_to_reg_bank(struct samsung_pinctrl_drv_data *drvdata, >>> ((b->pin_base + b->nr_pins - 1) < pin)) >>> b++; >>> >>> - *reg = drvdata->virt_base + b->pctl_offset; >>> + pctl_res_idx = b->pctl_res_idx; >>> + *reg = drvdata->virt_base[pctl_res_idx] + b->pctl_offset; >> >> I suggested something slightly different. Instead of >> bank::pctl_res_idx, I proposed bank::pctl_base. >> bank_info::pctl_res_idx would be specified only in init driver data >> and bank::pctl_base would be calculated at probe time as >> drvdata->virt_base[bank_info->pctl_res_idx] + bank_info->pctl_offset. >> This would eliminate the need to do any indexing and adding further in >> the code and make things simpler. >> >> Taking my other comments above, pctl part would be unchanged and only >> eint addresses and offsets would be affected. > > Ah, scratch this one sentence. I got confused with the register layout > again, sorry. Please refactor both eint and pctl as I suggested in the > upper paragraph. > > Best regards, > Tomasz > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- Best Regards, Chanwoo Choi