From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754479AbdBVISP (ORCPT ); Wed, 22 Feb 2017 03:18:15 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:41989 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753955AbdBVISI (ORCPT ); Wed, 22 Feb 2017 03:18:08 -0500 Date: Wed, 22 Feb 2017 09:18:05 +0100 From: Alexandre Belloni To: Sebastian Reichel Cc: Tony Lindgren , Alessandro Zummo , rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org Subject: Re: [PATCHv2] rtc: cpcap: new rtc driver Message-ID: <20170222081805.ap2yvarymlr5vvjo@piout.net> References: <20170220073535.27393-1-sre@kernel.org> <20170221061650.12636-1-sre@kernel.org> <20170221235212.hik3whcytw6xyevd@piout.net> <20170222015634.ugadzezywlrjduyx@earth> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170222015634.ugadzezywlrjduyx@earth> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 22/02/2017 at 02:56:34 +0100, Sebastian Reichel wrote: > > Does this mean there is a race condition? > > The logic (incl. comments) in this section are from the vendor > kernel driver and there is no documentation for CPCAP as far as > I know. I don't know if the hardware has logic to prevent a race > condition for the cpcap_tm.tod1 == 255 case. > That's fine, I was just curious :) > > > + err = cpcap_get_vendor(dev, rtc->regmap, &rtc->vendor); > > I think this means it depends on the mfd tree. > > Yes, but cpcap_get_vendor should get into mainline with the > 4.11 mfd pull request. So if you base your 4.12 for-next tree > on 4.11-rc1 everything should be fine. > OK, I'll take it for 4.12 then > > > + if (err) > > > + return err; > > > + > > > + rtc->alarm_irq= platform_get_irq(pdev, 0); > > > + err = devm_request_threaded_irq(dev, rtc->alarm_irq, NULL, > > > + cpcap_rtc_alarm_irq, IRQ_NONE, > > > + "rtc_alarm", rtc); > > > + if (err) { > > > + dev_err(dev, "Could not request alarm irq: %d\n", err); > > > + return err; > > > + } > > > + disable_irq(rtc->alarm_irq); > > > + > > > + rtc->update_irq= platform_get_irq(pdev, 1); > > > + err = devm_request_threaded_irq(dev, rtc->update_irq, NULL, > > > + cpcap_rtc_update_irq, IRQ_NONE, > > > + "rtc_1hz", rtc); > > > I don't think this IRQ is actually useful. It doesn't really harm but > > the tests should pass without it. > > Yes. RTC works perfectly fine with just the alarm irq. It also > works perfectly fine with just the 1 Hz irq (except for wakeup). > > I would like to keep the irq in the driver, so that it's explicitly > disabled. On Droid 4 mainline kernel is booted via kexec from > Android (AKA bootloader) and Motorola's Android kernel uses the > 1 Hz IRQ for some proprietary "secure clock daemon". > > I will add a comment. > OK, my plan was to remove all the RTC_UF users. I'll give it more thoughts. > > > + if (err) { > > > + dev_err(dev, "Could not request update irq: %d\n", err); > > > + return err; > > > + } > > > + disable_irq(rtc->update_irq); > > > + > > > + err = device_init_wakeup(dev, 1); > > > > If you use device_init_wakeup, I think it needs to be called before > > devm_rtc_device_register() to properly work. > > I successfully tested wakeup before sending this. But in case your > prefer it to be called before registering the RTC I can move the > call accordingly. > Then it is fine where it is. -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com