From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9C4E9C433EF for ; Wed, 13 Apr 2022 15:23:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234478AbiDMP0B (ORCPT ); Wed, 13 Apr 2022 11:26:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40298 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231797AbiDMP0B (ORCPT ); Wed, 13 Apr 2022 11:26:01 -0400 Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [IPv6:2001:4b98:dc4:8::232]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ADE9147566 for ; Wed, 13 Apr 2022 08:23:39 -0700 (PDT) Received: (Authenticated sender: miquel.raynal@bootlin.com) by mail.gandi.net (Postfix) with ESMTPSA id 1B091200007; Wed, 13 Apr 2022 15:23:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1649863413; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=W8BfYBj4EiPXDdXWGpBdNp7BTk+bWEhw6PcxdbWPC5U=; b=exBAHR2/Wc4rkJGI565FgelNlGjkvILC/NsKCynnTTFPU14J1dS3KU7xRktK7nQtgTmDUu ja/YRK/kwVKigfDISerrtpiU41fF2vij6sWGwNQZcjkAVg5/MhXOMveWL0I347LVagFawW XJ/5ZMfdlCF4XlEhylUbDVUv/+siycujgmP6qq6485blJRuTrBLzbnFnDwYRHgNSWo5YGV 3aVcPMVCWMiV6DTbE9qyi31J0Ss7SAkx1ux/jJ+ckmdEmEDGnW/RLkIDtmqRmfAxnl2jog O/C9ndLSP8sQN2qGMGQW1jfyjK207kNs+pMEZmEZ3UTDV8oMqmcZ5d0bq5ZXIg== Date: Wed, 13 Apr 2022 17:23:27 +0200 From: Miquel Raynal To: Alexandre Belloni Cc: Alessandro Zummo , Rob Herring , devicetree@vger.kernel.org, Krzysztof Kozlowski , linux-renesas-soc@vger.kernel.org, Magnus Damm , Gareth Williams , Phil Edworthy , Geert Uytterhoeven , Stephen Boyd , Michael Turquette , linux-clk@vger.kernel.org, Milan Stevanovic , Jimmy Lalande , Pascal Eberhard , Thomas Petazzoni , Herve Codina , Clement Leger , linux-rtc@vger.kernel.org, Michel Pollet Subject: Re: [PATCH 3/7] rtc: rzn1: Add new RTC driver Message-ID: <20220413172327.73d1fcc1@xps13> In-Reply-To: References: <20220405184716.1578385-1-miquel.raynal@bootlin.com> <20220405184716.1578385-4-miquel.raynal@bootlin.com> Organization: Bootlin X-Mailer: Claws Mail 3.17.7 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-rtc@vger.kernel.org Hi Alex, > > + tm->tm_sec =3D bcd2bin(tm->tm_sec); > > + tm->tm_min =3D bcd2bin(tm->tm_min); > > + tm->tm_hour =3D bcd2bin(tm->tm_hour); > > + tm->tm_wday =3D bcd2bin(tm->tm_wday); > > + tm->tm_mday =3D bcd2bin(tm->tm_mday); > > + tm->tm_mon =3D bcd2bin(tm->tm_mon); > > + tm->tm_year =3D bcd2bin(tm->tm_year); > > + > > + dev_dbg(dev, "%d-%d-%d(%d)T%d:%d:%d\n", > > + tm->tm_year, tm->tm_mon, tm->tm_mday, tm->tm_wday, > > + tm->tm_hour, tm->tm_min, tm->tm_sec); > > + =20 >=20 > This is not really useful because we have tracepoints in the core. > Anyway, please use %ptR. Nice printk operator :) I've dropped this dev_dbg call, it actually does not serve any useful purpose. [...] > > +static int rzn1_rtc_probe(struct platform_device *pdev) > > +{ > > + struct rzn1_rtc *rtc; > > + int ret; > > + > > + rtc =3D devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL); > > + if (!rtc) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, rtc); > > + > > + rtc->clk =3D devm_clk_get(&pdev->dev, "hclk"); > > + if (IS_ERR(rtc->clk)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk), "Missing hclk\n"= ); > > + > > + rtc->base =3D devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(rtc->base)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(rtc->base), "Missing reg\n"= ); > > + > > + rtc->rtcdev =3D devm_rtc_allocate_device(&pdev->dev); > > + if (IS_ERR(rtc->rtcdev)) > > + return PTR_ERR(rtc); > > + > > + rtc->rtcdev->range_max =3D 3178591199UL; /* 100 years */ =20 >=20 > I'm not sure how you came to this value, this is 2070-09-22T05:59:59. > I'm pretty sure the RTC will not fail at that time. Also, the comment > seems fishy. The RTC itself as no "starting point", but just a counter that can count up to 100. So the max range is start-year + 100 years. But at this point I don't yet have access to the start-year value. What's your advise? > > + rtc->rtcdev->ops =3D &rzn1_rtc_ops; > > + > > + ret =3D r9a06g032_sysctrl_enable_rtc(true); > > + if (ret) > > + return ret; > > + > > + ret =3D clk_prepare_enable(rtc->clk); > > + if (ret) > > + goto disable_rtc; > > + > > + /* > > + * Ensure the clock counter is enabled. > > + * Set 24-hour mode and possible oscillator offset compensation in SU= BU mode. > > + */ > > + writel(RZN1_RTC_CTL0_CE | RZN1_RTC_CTL0_AMPM | RZN1_RTC_CTL0_SLSB_SUB= U, > > + rtc->base + RZN1_RTC_CTL0); > > + > > + /* Disable all interrupts */ > > + writel(0, rtc->base + RZN1_RTC_CTL1); > > + > > + /* Enable counter operation */ > > + writel(0, rtc->base + RZN1_RTC_CTL2); > > + =20 >=20 > I don't think you should do that unconditionally. The RTC is either > not already started (and doesn't carry the proper time/date) or already > started. It would be better to start it in .set_time. Maybe you can even > use that to detect whether it has already been set once. Actually there is only one (interesting) writeable bit in this register and it defaults to 0 whenever we enable the register interface so we don't really need to enable anything. But I've added the necessary logic to handle the situation where the bootloader would disable the rtc (which is the only situation where the driver can diagnose an erroneous time/date). > > + ret =3D devm_rtc_register_device(rtc->rtcdev); > > + if (ret) { > > + dev_err(&pdev->dev, "Failed to register RTC\n"); =20 >=20 > No error message is needed here. Ok. >=20 > > + goto disable_clk; > > + } > > + > > + return 0; > > + > > +disable_clk: > > + clk_disable_unprepare(rtc->clk); > > +disable_rtc: > > + r9a06g032_sysctrl_enable_rtc(false); > > + > > + return ret; > > +} > > + > > +static int rzn1_rtc_remove(struct platform_device *pdev) > > +{ > > + struct rzn1_rtc *rtc =3D platform_get_drvdata(pdev); > > + > > + clk_disable_unprepare(rtc->clk); > > + r9a06g032_sysctrl_enable_rtc(false); =20 >=20 > Does this stop the RTC or just the register interface? Mmmh actually if I reset the sysctrl bit manually I loose the counter entirely, so I'll drop this call. Thanks, Miqu=C3=A8l