Re: [PATCH v2 08/10] rtc: New driver for RTC in Netronix embedded controller

Jonathan Neuschäfer
2020-09-10T19:50:50Z

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

On Sat, Sep 05, 2020 at 08:56:35PM +0300, Andy Shevchenko wrote:
> On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> wrote:
[...]
> > +#include <linux/of_device.h>
>
>
> 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)
>
>
> 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 = dev_get_drvdata(dev);
> > + int res = 0;
> > +
> > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR,
> > u8_to_reg(tm->tm_year - 100));
> > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH,
> > u8_to_reg(tm->tm_mon + 1));
> > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY,
> > u8_to_reg(tm->tm_mday));
> > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR,
> > u8_to_reg(tm->tm_hour));
> > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE,
> > u8_to_reg(tm->tm_min));
> > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND,
> > u8_to_reg(tm->tm_sec));
> > +
> > + return (res < 0) ? -EIO : 0;
>
>
> Hmm... (I stumbled over res |= 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[] = {
> > + { .compatible = "netronix,ntxec-rtc" },
> > + { },
>
>
> No need for comma in terminator line.

Okay

Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]