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 X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 14404C4363D for ; Fri, 25 Sep 2020 05:44:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C34D72176B for ; Fri, 25 Sep 2020 05:44:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727132AbgIYFot (ORCPT ); Fri, 25 Sep 2020 01:44:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57474 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726925AbgIYFot (ORCPT ); Fri, 25 Sep 2020 01:44:49 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EA80BC0613CE for ; Thu, 24 Sep 2020 22:44:48 -0700 (PDT) Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kLgXC-0006gp-Hj; Fri, 25 Sep 2020 07:44:42 +0200 Received: from ukl by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1kLgWu-00035h-Ql; Fri, 25 Sep 2020 07:44:24 +0200 Date: Fri, 25 Sep 2020 07:44:24 +0200 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Jonathan =?utf-8?Q?Neusch=C3=A4fer?= Cc: linux-kernel@vger.kernel.org, Alexandre Belloni , Heiko Stuebner , linux-pwm@vger.kernel.org, Linus Walleij , Thierry Reding , Fabio Estevam , linux-rtc@vger.kernel.org, Arnd Bergmann , Mauro Carvalho Chehab , Sam Ravnborg , Daniel Palmer , Andy Shevchenko , Andreas Kemnade , NXP Linux Team , devicetree@vger.kernel.org, Stephan Gerhold , allen , Sascha Hauer , Lubomir Rintel , Rob Herring , Lee Jones , linux-arm-kernel@lists.infradead.org, Alessandro Zummo , Mark Brown , Pengutronix Kernel Team , Heiko Stuebner , Josua Mayer , Shawn Guo , "David S. Miller" Subject: Re: [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded controller Message-ID: <20200925054424.snlr3lggnsv575wu@pengutronix.de> References: <20200924192455.2484005-1-j.neuschaefer@gmx.net> <20200924192455.2484005-6-j.neuschaefer@gmx.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="b55wkovfqn6iyquo" Content-Disposition: inline In-Reply-To: <20200924192455.2484005-6-j.neuschaefer@gmx.net> X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --b55wkovfqn6iyquo Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hello Jonathan, On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neusch=E4fer wrote: > +#define NTXEC_REG_WRITE_YEAR 0x10 > +#define NTXEC_REG_WRITE_MONTH 0x11 > +#define NTXEC_REG_WRITE_DAY 0x12 > +#define NTXEC_REG_WRITE_HOUR 0x13 > +#define NTXEC_REG_WRITE_MINUTE 0x14 > +#define NTXEC_REG_WRITE_SECOND 0x15 > + > +#define NTXEC_REG_READ_YM 0x20 > +#define NTXEC_REG_READ_DH 0x21 > +#define NTXEC_REG_READ_MS 0x23 Is this an official naming? I think at least ..._MS is a poor name. Maybe consider ..._MINSEC instead and make the other two names a bit longer for consistency? > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc =3D dev_get_drvdata(dev); > + unsigned int value; > + int res; > + > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); > + if (res < 0) > + return res; > + > + tm->tm_year =3D (value >> 8) + 100; > + tm->tm_mon =3D (value & 0xff) - 1; > + > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); > + if (res < 0) > + return res; > + > + tm->tm_mday =3D value >> 8; > + tm->tm_hour =3D value & 0xff; > + > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); > + if (res < 0) > + return res; > + > + tm->tm_min =3D value >> 8; > + tm->tm_sec =3D value & 0xff; > + > + return 0; > +} > + > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc =3D dev_get_drvdata(dev); > + int res =3D 0; > + > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(= tm->tm_year - 100)); > + if (res) > + return res; > + > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8= (tm->tm_mon + 1)); > + if (res) > + return res; > + > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(t= m->tm_mday)); > + if (res) > + return res; > + > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(= tm->tm_hour)); > + if (res) > + return res; > + > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg= 8(tm->tm_min)); > + if (res) > + return res; > + > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8= (tm->tm_sec)); I wonder: Is this racy? If you write minute, does the seconds reset to zero or something like that? Or can it happen, that after writing the minute register and before writing the second register the seconds overflow and you end up with the time set to a minute later than intended? If so it might be worth to set the seconds to 0 at the start of the function (with an explaining comment). =2Eread_time has a similar race. What happens if minutes overflow between reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS? > +static struct platform_driver ntxec_rtc_driver =3D { > + .driver =3D { > + .name =3D "ntxec-rtc", > + }, > + .probe =3D ntxec_rtc_probe, No .remove function? > +}; > +module_platform_driver(ntxec_rtc_driver); > + > +MODULE_AUTHOR("Jonathan Neusch=E4fer "); > +MODULE_DESCRIPTION("RTC driver for Netronix EC"); > +MODULE_LICENSE("GPL"); Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --b55wkovfqn6iyquo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAl9tg7UACgkQwfwUeK3K 7AlEBAf/RcD3gHide1qc5EijimG6s9s/zjWGhHAO0leLwogNaMyb6GdjoIjrvRHf ETLl9BUqziVi69LGA+Ub9K0+LgKyN024FRYZ/U/RqSo8PfRSw40ehnJibb+fdWjL W0zTK31qxkj+YBnqWd32Z27hpPCZe0uk6yqaguK9aIPy7XAiwu0gah2e13tD2Bk1 6Cxr82gYMQw8iHO7EjWkQ9D3Yzwcy9ihTCxL8GJtTRif93ZchKTwoLq4wbmf8tc1 5fU0cYWXZ97+4+48/dGfqUQ6fQ2yHZXZR69MNmwFnAIaVleVXMc1B6pYIbmHhnoq rbAZQzSE6gvnqI203rqDXBH6YXRZCw== =tcyy -----END PGP SIGNATURE----- --b55wkovfqn6iyquo-- 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 X-Spam-Level: X-Spam-Status: No, score=-5.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8B652C4363D for ; Fri, 25 Sep 2020 05:46:26 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3A5BB20838 for ; Fri, 25 Sep 2020 05:46:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="xen6mWUI" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3A5BB20838 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type:Cc: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=aLms6ZI/8/Z4sl+DV3xlyVm7OLXC4wupuegVkmwbywM=; b=xen6mWUI8kI9PfekJPkF4rDfT vBjqohCuExwi/rEkKql5/EPdq7XDkuqhwaTO4/GmijNaMzNOgidg/KLh1xSy4QJ/4PrfHKSSa6O1k L8QK4ObqJMce1rvcXCX8Gg9qBjrKb10l9ePHMTTlB857wEgKwTTQ8sd5BpLuJkX5blTDNZzh/wvem M5oEN9pF45BOpzwoFcCv+fyZlIwIpqlV/0L5wE3tqUWTN6CFZzGYciLMU4fo9KLtKkoLJTV5feSfU +ShTdmyKfTcnZWt5yHKCqyu4dt2XhTVyKreU8Lcw+L/23KTVKggBkkWbR/L4c79IWm0ZAMUB7uv+s oqdhC+8Kw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kLgXX-0005S7-6f; Fri, 25 Sep 2020 05:45:03 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kLgXU-0005RY-PT for linux-arm-kernel@lists.infradead.org; Fri, 25 Sep 2020 05:45:01 +0000 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kLgXC-0006gp-Hj; Fri, 25 Sep 2020 07:44:42 +0200 Received: from ukl by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1kLgWu-00035h-Ql; Fri, 25 Sep 2020 07:44:24 +0200 Date: Fri, 25 Sep 2020 07:44:24 +0200 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Jonathan =?utf-8?Q?Neusch=C3=A4fer?= Subject: Re: [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded controller Message-ID: <20200925054424.snlr3lggnsv575wu@pengutronix.de> References: <20200924192455.2484005-1-j.neuschaefer@gmx.net> <20200924192455.2484005-6-j.neuschaefer@gmx.net> MIME-Version: 1.0 In-Reply-To: <20200924192455.2484005-6-j.neuschaefer@gmx.net> X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200925_014500_838673_A5CF13A4 X-CRM114-Status: GOOD ( 25.22 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alexandre Belloni , Heiko Stuebner , devicetree@vger.kernel.org, Linus Walleij , Thierry Reding , Sam Ravnborg , linux-rtc@vger.kernel.org, Arnd Bergmann , Mauro Carvalho Chehab , Lee Jones , Daniel Palmer , Andy Shevchenko , Andreas Kemnade , NXP Linux Team , linux-pwm@vger.kernel.org, Stephan Gerhold , allen , Sascha Hauer , Lubomir Rintel , Rob Herring , Fabio Estevam , linux-arm-kernel@lists.infradead.org, Alessandro Zummo , linux-kernel@vger.kernel.org, Mark Brown , Pengutronix Kernel Team , Heiko Stuebner , Josua Mayer , Shawn Guo , "David S. Miller" Content-Type: multipart/mixed; boundary="===============5536133845694098503==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============5536133845694098503== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="b55wkovfqn6iyquo" Content-Disposition: inline --b55wkovfqn6iyquo Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hello Jonathan, On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neusch=E4fer wrote: > +#define NTXEC_REG_WRITE_YEAR 0x10 > +#define NTXEC_REG_WRITE_MONTH 0x11 > +#define NTXEC_REG_WRITE_DAY 0x12 > +#define NTXEC_REG_WRITE_HOUR 0x13 > +#define NTXEC_REG_WRITE_MINUTE 0x14 > +#define NTXEC_REG_WRITE_SECOND 0x15 > + > +#define NTXEC_REG_READ_YM 0x20 > +#define NTXEC_REG_READ_DH 0x21 > +#define NTXEC_REG_READ_MS 0x23 Is this an official naming? I think at least ..._MS is a poor name. Maybe consider ..._MINSEC instead and make the other two names a bit longer for consistency? > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc =3D dev_get_drvdata(dev); > + unsigned int value; > + int res; > + > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); > + if (res < 0) > + return res; > + > + tm->tm_year =3D (value >> 8) + 100; > + tm->tm_mon =3D (value & 0xff) - 1; > + > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); > + if (res < 0) > + return res; > + > + tm->tm_mday =3D value >> 8; > + tm->tm_hour =3D value & 0xff; > + > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); > + if (res < 0) > + return res; > + > + tm->tm_min =3D value >> 8; > + tm->tm_sec =3D value & 0xff; > + > + return 0; > +} > + > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc =3D dev_get_drvdata(dev); > + int res =3D 0; > + > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(= tm->tm_year - 100)); > + if (res) > + return res; > + > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8= (tm->tm_mon + 1)); > + if (res) > + return res; > + > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(t= m->tm_mday)); > + if (res) > + return res; > + > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(= tm->tm_hour)); > + if (res) > + return res; > + > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg= 8(tm->tm_min)); > + if (res) > + return res; > + > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8= (tm->tm_sec)); I wonder: Is this racy? If you write minute, does the seconds reset to zero or something like that? Or can it happen, that after writing the minute register and before writing the second register the seconds overflow and you end up with the time set to a minute later than intended? If so it might be worth to set the seconds to 0 at the start of the function (with an explaining comment). =2Eread_time has a similar race. What happens if minutes overflow between reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS? > +static struct platform_driver ntxec_rtc_driver =3D { > + .driver =3D { > + .name =3D "ntxec-rtc", > + }, > + .probe =3D ntxec_rtc_probe, No .remove function? > +}; > +module_platform_driver(ntxec_rtc_driver); > + > +MODULE_AUTHOR("Jonathan Neusch=E4fer "); > +MODULE_DESCRIPTION("RTC driver for Netronix EC"); > +MODULE_LICENSE("GPL"); Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --b55wkovfqn6iyquo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAl9tg7UACgkQwfwUeK3K 7AlEBAf/RcD3gHide1qc5EijimG6s9s/zjWGhHAO0leLwogNaMyb6GdjoIjrvRHf ETLl9BUqziVi69LGA+Ub9K0+LgKyN024FRYZ/U/RqSo8PfRSw40ehnJibb+fdWjL W0zTK31qxkj+YBnqWd32Z27hpPCZe0uk6yqaguK9aIPy7XAiwu0gah2e13tD2Bk1 6Cxr82gYMQw8iHO7EjWkQ9D3Yzwcy9ihTCxL8GJtTRif93ZchKTwoLq4wbmf8tc1 5fU0cYWXZ97+4+48/dGfqUQ6fQ2yHZXZR69MNmwFnAIaVleVXMc1B6pYIbmHhnoq rbAZQzSE6gvnqI203rqDXBH6YXRZCw== =tcyy -----END PGP SIGNATURE----- --b55wkovfqn6iyquo-- --===============5536133845694098503== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============5536133845694098503==--