From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752594AbcCBDwZ (ORCPT ); Tue, 1 Mar 2016 22:52:25 -0500 Received: from mailout1.w1.samsung.com ([210.118.77.11]:60557 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751144AbcCBDwX (ORCPT ); Tue, 1 Mar 2016 22:52:23 -0500 X-AuditID: cbfec7f5-f79b16d000005389-27-56d6637467d6 Subject: Re: [PATCH 1/2] rtc: max77686: Add support for MAX20024/MAX77620 RTC IP To: Laxman Dewangan , alexandre.belloni@free-electrons.com, a.zummo@towertech.it References: <1456750705-13579-1-git-send-email-ldewangan@nvidia.com> <56D63A9C.20506@samsung.com> <56D64CAC.5030704@nvidia.com> Cc: cw00.choi@samsung.com, rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org, javier@osg.samsung.com, rklein@nvidia.com From: Krzysztof Kozlowski X-Enigmail-Draft-Status: N1110 Message-id: <56D6636F.6090403@samsung.com> Date: Wed, 02 Mar 2016 12:52:15 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-version: 1.0 In-reply-to: <56D64CAC.5030704@nvidia.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrOLMWRmVeSWpSXmKPExsVy+t/xq7olydfCDLa84bRYcvEqu0XHtcVM Fte/PGe1ePN2DZPF6xeGFkv3rWaxuLxrDpvFsuOrWS32d3YwOnB6PNl0kdFjz8STbB69ze/Y PLb032X36NuyitFj+ryfTB6fN8kFsEdx2aSk5mSWpRbp2yVwZcx6uIi94JhUxZ9NQQ2MO0W7 GDk5JARMJKad3cMCYYtJXLi3nq2LkYtDSGApo8T5OReYQBJCAk8ZJdY3BnUxcnAICwRI3Dym CGKKCGRInFzgCVHeyCjxqHsWC4jDLNAA1Nu7kR2kl03AWGLz8iVsEAvkJHq7J4Et4xXQkjh4 YClYDYuAqsTaxwvA4qICERKHO7vYIWoEJX5MvgcW5wSqX99yhxFkMbOAnsT9i1ogYWYBeYnN a94yT2AUnIWkYxZC1SwkVQsYmVcxiqaWJhcUJ6XnGukVJ+YWl+al6yXn525ihETH1x2MS49Z HWIU4GBU4uHN+HQ1TIg1say4MvcQowQHs5IIL2vMtTAh3pTEyqrUovz4otKc1OJDjNIcLEri vDN3vQ8REkhPLEnNTk0tSC2CyTJxcEo1MGax1XGd3Vd0aL/adIFjl95NmmC349RsLu/Gjth5 9/ZHzToTH7S2/MaTBSLK/JGZdsflNibtYfM7M+3BBnvOB7/NW9/eXpuys9w6nUPgy0l5PqkK qwf+DSk+nTMKg5zk+E4JVjy537Lh0L0ZsbJSlyMO/bp2R87v1uvzoVkM3RH7Pi47o3Kw7qoS S3FGoqEWc1FxIgAm0N01igIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02.03.2016 11:15, Laxman Dewangan wrote: > > On Wednesday 02 March 2016 06:28 AM, Krzysztof Kozlowski wrote: >> On 29.02.2016 21:58, Laxman Dewangan wrote: >> + .alarm_pending_status_reg = MAX77620_RTC_ALARM_PENDING_STATUS_REG, >> Just skip the alarm_pending_status_reg (so it will be 0x0) and check for >> non-zero value later? >> >> It might be a little bit non consistent approach to how we map RTC >> registers (REG_RTC_NONE)... so I don't have strong feelings about this. > > I choose -1 because 0 is also valid. > So I can have macro for INVALID register which is -1 and use here, other > places direct register as STATUS2. There is only one value used here so 0 not valid. But I don't mind that approach. > > >> >>> + if (info->drv_data->rtc_irq_from_platform) { >>> + struct platform_device *pdev = to_platform_device(info->dev); >>> + >>> + info->rtc_irq = platform_get_irq(pdev, 0); >> It may return -ERRNO. What happens then? > > MFD is initializing the irq and so it will not fail on this particular > case. > Even if error, the regmap_add_irq should fail. > > Let me handle error at this point only to avoid any assumption and > further processing with error, by returning error. > > >> >>> + } else { >>> + info->rtc_irq = parent_i2c->irq; >>> + } >>> info->regmap = dev_get_regmap(parent, NULL); >>> if (!info->regmap) { >>> @@ -802,6 +840,8 @@ static SIMPLE_DEV_PM_OPS(max77686_rtc_pm_ops, >>> static const struct platform_device_id rtc_id[] = { >>> { "max77686-rtc", .driver_data = >>> (kernel_ulong_t)&max77686_drv_data, }, >>> { "max77802-rtc", .driver_data = >>> (kernel_ulong_t)&max77802_drv_data, }, >>> + { "max77620-rtc", .driver_data = >>> (kernel_ulong_t)&max77620_drv_data, }, >>> + { "max20024-rtc", .driver_data = >>> (kernel_ulong_t)&max77620_drv_data, }, >> There shouldn't be "max20024-rtc". This is exactly the same as >> "max77620-rtc" so re-use existing id. No point of duplicating device >> names for 100% compatible devices. >> >> > I am thinking that having compatible for each device which it supports > is better. > > In MFD, I have made all sub module of max20024 as max20024-. > I have not mixed the sub module name for max20024 with max77620 module. The point of compatible is to be... compatible so you don't create compatibles for the same meaning! However this is actually not a compatible but a matching name... which should follow the same idea. You did not give any argument why this is better. For me, code like this: { "max77802-rtc", .driver_data = (kernel_ulong_t)&max77802_drv_data, }, { "max77620-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, }, { "max77621-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, }, { "max77622-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, }, { "max77623-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, }, { "max776xx-some-other-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, }, { "max77624-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, }, is useless, ridiculous and obfuscated. It is duplication of code just "because". The child driver is selected by matching mfd-cell or compatible. We are reusing child drivers so reuse under the same name. Best regards, Krzysztof