Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 10 Sep 2020 21:42:20 +0200
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Lee Jones <lee.jones@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= 
        <u.kleine-koenig@pengutronix.de>,
        Alessandro Zummo <a.zummo@towertech.it>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Heiko Stuebner <heiko.stuebner@theobroma-systems.com>,
        Stephan Gerhold <stephan@gerhold.net>,
        Lubomir Rintel <lkundrak@v3.sk>,
        Mark Brown <broonie@kernel.org>, allen <allen.chen@ite.com.tw>,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
        "linux-rtc@vger.kernel.org" <linux-rtc@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        Heiko Stuebner <heiko@sntech.de>,
        Josua Mayer <josua.mayer@jm0.eu>,
        Andreas Kemnade <andreas@kemnade.info>,
        Arnd Bergmann <arnd@arndb.de>, Daniel Palmer <daniel@0x0f.com>
Subject: Re: [PATCH v2 08/10] rtc: New driver for RTC in Netronix embedded
 controller
Message-ID: <20200910194220.GE3306@latitude>
References: <20200905133230.1014581-1-j.neuschaefer@gmx.net>
 <20200905144503.1067124-1-j.neuschaefer@gmx.net>
 <CAHp75Vca+Tp0v_Q4RBuUNSo_5aq_u6mBGeXecHULC1Avgm5=Xg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="M/SuVGWktc5uNpra"
Content-Disposition: inline
In-Reply-To: <CAHp75Vca+Tp0v_Q4RBuUNSo_5aq_u6mBGeXecHULC1Avgm5=Xg@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--M/SuVGWktc5uNpra
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sat, Sep 05, 2020 at 08:56:35PM +0300, Andy Shevchenko wrote:
> On Saturday, September 5, 2020, Jonathan Neusch=C3=A4fer <j.neuschaefer@g=
mx.net>
> wrote:
[...]
> > +#include <linux/of_device.h>
>=20
>=20
> No user for this. Perhaps you meant mod_devicetable.h?

Yes

> > +/* Convert an 8-bit value into the correct format for writing into a
> > register */
> > +#define u8_to_reg(x) (((x) & 0xff) << 8)
>=20
>=20
> This needs more explanation. Does register be16?

Yes, the registers are treated as be16 in the base driver. I wrote a
slightly longer explanation in response to your other review.

> > +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,
> > u8_to_reg(tm->tm_year - 100));
> > +       res |=3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH,
> > u8_to_reg(tm->tm_mon + 1));
> > +       res |=3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY,
> > u8_to_reg(tm->tm_mday));
> > +       res |=3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR,
> > u8_to_reg(tm->tm_hour));
> > +       res |=3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE,
> > u8_to_reg(tm->tm_min));
> > +       res |=3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND,
> > u8_to_reg(tm->tm_sec));
> > +
> > +       return (res < 0) ? -EIO : 0;
>=20
>=20
> Hmm... (I stumbled over res |=3D parts)

I'll convert it to the (more verbose but also more correct) pattern of
returning each error early.

> > +static const struct of_device_id ntxec_rtc_of_match[] =3D {
> > +       { .compatible =3D "netronix,ntxec-rtc" },
> > +       { },
>=20
>=20
> No need for comma in terminator line.

Okay


Thanks,
Jonathan Neusch=C3=A4fer

--M/SuVGWktc5uNpra
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAl9agZQACgkQCDBEmo7z
X9t7Vg//TklGb5L5lL/xuITfZUbaWRE0NO9ebO0gQJ0jIXLZw0XMii5+QFlgjmn/
53tgs2wghDAcz4Es/t9azuuiTLkt76nIZU2uYGX17ULQ9RK3bEA3Wz8N+7VY3QXM
txcZ4McFlMCmWAhclQDnK3VVywI3Nu0iF6CNnWQY1xRH4KmjyIAbXkalDiYI3y3s
VRcdUjBTaOEQSnNoQFSew2DN1yl+OpN5z7XONYznBWMpnMAzB5+QL/IHrIYjvVZt
3lnu2beHd4eUGHH6RsyqyRGJHM59uUckMynmEdXjuoktOdLJte3pXdce4EE7HF6k
LJCMrRcZP7XKe+AP4DQaFD3fr9ZHU6z6yqtgePzGh6aAtgjjnC16Y4mgEK0Lvvb8
5Z1PU9e39G1WqsSnacmdRPBgxTT8hbogcnroW4GKfE3VW2ph1CE4CDCc1cBYzu93
NhtNw6fTa9gXJXETC5zEH+Xf/Yf9K5cCwsuvzVGnBJbw57530eMuzV4/PjwOlayM
fDa4BM+N9Lf/EIfzupT5CTcTFvUmz5Ao4aM1XQOFTtdtEjKdqZV1O2Y1tH7+Zfj3
TDG1itKF2vm9iToGWEr4wMoe/X75/uf7YV7kGhN8kv3kIPBpeeygOyP96dJo1Gk5
gQ2N3HBm2oesXQSvoEhMmCuUSLHkuL5sB7Zz1NRMN7XomQ+7rEA=
=9c8/
-----END PGP SIGNATURE-----

--M/SuVGWktc5uNpra--