From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755436AbcANBVD (ORCPT ); Wed, 13 Jan 2016 20:21:03 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:16570 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754105AbcANBVA (ORCPT ); Wed, 13 Jan 2016 20:21:00 -0500 X-AuditID: cbfec7f5-f79b16d000005389-b3-5696f7f9d912 Subject: Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP To: Laxman Dewangan , robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, linus.walleij@linaro.org, gnurou@gmail.com, lee.jones@linaro.org, broonie@kernel.org, a.zummo@towertech.it, alexandre.belloni@free-electrons.com References: <1452590273-16421-1-git-send-email-ldewangan@nvidia.com> <1452590273-16421-6-git-send-email-ldewangan@nvidia.com> <569594F0.4020007@samsung.com> <5695CD73.5010709@nvidia.com> <5695D268.3060705@samsung.com> <5695D1AF.9000806@nvidia.com> <5696665A.5030508@nvidia.com> <5696F0C4.1060904@samsung.com> Cc: lgirdwood@gmail.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, rtc-linux@googlegroups.com, swarren@nvidia.com, treding@nvidia.com From: Krzysztof Kozlowski X-Enigmail-Draft-Status: N1110 Message-id: <5696F7F8.108@samsung.com> Date: Thu, 14 Jan 2016 10:20:56 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-version: 1.0 In-reply-to: <5696F0C4.1060904@samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrFIsWRmVeSWpSXmKPExsVy+t/xq7o/v08LM+ho17VYcvEqu0XHtcVM FlMfPmGzmH/kHKtF/5uFrBbnXj1iARIrGS1evzC0WLpvNYvF/a9HGS2+XelgspjyZzmTxeb5 fxgtLu+aw2ax9PpFJosJ09eyWLTuPcJusb+zg9HixvQWVovbv/kcRDzWzFvD6HG5r5fJ48mm i4weO2fdZffYM/Ekm8fK5V/YPDat6mTzuHNtD5tHb/M7No++LasYPabP+8nk8XmTXABPFJdN SmpOZllqkb5dAlfG1fc7GAtuyFZMuPOEtYFxg1gXIyeHhICJxNk/l1ghbDGJC/fWs3UxcnEI CSxllHi+eTqU85RRYvqcfkaQKmGBGIkXq+eygCREBLYzSWy68YwRomoPk8TrNQ/AHGaBrYwS 37d+ZAFpYRMwlti8fAkbxBI5id7uSWBxXgE1iZdbjoHZLAKqEkcW3weyOThEBSIkFu3IhCgR lPgx+R5YCaeAtkTbkWdgJcwCehL3L2qBhJkF5CU2r3nLPIFRcBaSjlkIVbOQVC1gZF7FKJpa mlxQnJSea6RXnJhbXJqXrpecn7uJERLDX3cwLj1mdYhRgINRiYd3we1pYUKsiWXFlbmHGCU4 mJVEeO++BQrxpiRWVqUW5ccXleakFh9ilOZgURLnnbnrfYiQQHpiSWp2ampBahFMlomDU6qB kU+nRLVcuu3vtqdRdY5lTMEqZwrCBeyX2vPkrT+2tGQy/4WsXTdrVaZd3fboz5zJETbHYx7d f3s53Z7nsEHclMluTfNTe+tmvcsT+BvR6dhj86sxWl6z71XLVd4tEzU2zFz8bOd5S4VCBUW2 a/HWiufaV+5Ou+PRFt9o9GK2/2eXadEnrBV5lViKMxINtZiLihMBkQP5Lt0CAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 14.01.2016 09:50, Krzysztof Kozlowski wrote: > On 13.01.2016 23:59, Laxman Dewangan wrote: >> >> On Wednesday 13 January 2016 09:55 AM, Laxman Dewangan wrote: >>> >>> On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote: >>>> On 13.01.2016 13:07, Laxman Dewangan wrote: >>>>> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote: >>>> >>>>> That is also fine to me but still I am not comfortable with the config >>>>> name and driver file name as this does not suggest the common. >>>> The name does not matter. Really. We have a lot of drivers with a >>>> specific device-like name and supporting different devices. To point >>>> that your argument is invalid - your initial name of driver >>>> "rtc-max77620.c" supported totally different "names": the max77620 and >>>> max20024. It also wasn't suggesting something "common"... >>> In all config string, I have mentioned the MAX20024. >>> >>> >>>> With my approach we are not developing common think neither. We just >>>> want to extend/re-use existing max77686 (or max77802) driver for new >>>> devices. Just like everywhere else. >>> >>> OK, fine to me if this is acceptable. >>> I will drop this rtc patch form this series and work in max77686 >>> driver to modify first and once merged, use this config on my defconfig. >>> >> >> Here, MAX686 RTC driver needs two regmap handle, one for the rtc_regmap >> and other for STATUS2 register access. >> >> ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2, >> &val); >> if (ret < 0) { >> dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n", >> __func__, __LINE__, ret); >> goto out; >> } >> >> >> We can not have two regmap handle on rtc driver as both regmap (pmic and >> rtc) registered with different i2c device. >> >> Also this register should not be accessed by RTC driver if we want to >> decouple as this is very much MAX77686 register set. >> Do we need this code? >> static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm >> *alrm) >> { >> struct max77686_rtc_info *info = dev_get_drvdata(dev); >> u8 data[RTC_NR_TIME]; >> unsigned int val; >> int i, ret; >> >> :::::::: >> alrm->pending = 0; >> ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2, >> &val); >> if (ret < 0) { >> dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n", >> __func__, __LINE__, ret); >> goto out; >> } >> >> if (val & (1 << 4)) /* RTCA1 */ >> alrm->pending = 1; >> >> } > > Most of the drivers set the 'pending' field when reading alarm. Your > original driver did not. > > The max77802 does exactly the same (BTW, these should be merged as > well... I'll add this to the TODO list) so I think this is necessary. How about merging max77802 to max77686 first? The only differences I found are: 1. It uses main MFD/PMIC regmap. This can be solved as part of decoupling code. The driver will get MFD's regmap and set up its own (only on max77686). The max77802 will only use parent's regmap. 2. It has different register address. We need a register-layout/configuration structure. The logic is the same except few differences (e.g. presence of MAX77802_RTC_AE1). It may be easier to merge them now, before adding support for max77620? I could handle this probably next week or in the following week (assuming someone would test max77802 because I don't have the hardware). Anyway I think we should develop these RTC patches having this in mind: merge all of them. Best regards, Krzysztof