Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 18 May 2022 10:28:59 +0200
From:   Alexandre Belloni <alexandre.belloni@bootlin.com>
To:     Medad Young <medadyoung@gmail.com>
Cc:     Benjamin Fair <benjaminfair@google.com>,
        Nancy Yuen <yuenn@google.com>,
        Patrick Venture <venture@google.com>,
        Tali Perry <tali.perry1@gmail.com>,
        Tomer Maimon <tmaimon77@gmail.com>,
        Avi Fishman <avifishman70@gmail.com>,
        Rob Herring <robh+dt@kernel.org>, a.zummo@towertech.it,
        KWLIU@nuvoton.com, YSCHU@nuvoton.com, JJLIU0@nuvoton.com,
        KFTING <KFTING@nuvoton.com>, ctcchien@nuvoton.com,
        OpenBMC Maillist <openbmc@lists.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-rtc@vger.kernel.org
Subject: Re: [PATCH v2 3/3] RTC: nuvoton: Add NCT3018Y real time clock driver
Message-ID: <YoSuS+nFJoD4+oKM@mail.local>
References: <20220517092927.19537-1-ctcchien@nuvoton.com>
 <20220517092927.19537-4-ctcchien@nuvoton.com>
 <YoPzaSc/8BBVWWsB@mail.local>
 <CAHpyw9fw54hQrsPa4psbUs2VfBqHj+gMKDceL2N5k8_jU+434w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHpyw9fw54hQrsPa4psbUs2VfBqHj+gMKDceL2N5k8_jU+434w@mail.gmail.com>
Precedence: bulk

On 18/05/2022 11:11:00+0800, Medad Young wrote:
> > > +config RTC_DRV_NCT3018Y
> > > +     tristate "Nuvoton Real Time Clock"
> >
> > This definitively needs a better description
> 
> OK, I will add a better description.

To be clear, this needs at least the part number

> > > +     tm->tm_wday = buf[6] & 0x07;
> > > +     tm->tm_mday = bcd2bin(buf[7] & 0x3F);
> > > +     tm->tm_mon = bcd2bin(buf[8] & 0x1F) - 1 ; /* rtc mn 1-12 */
> > > +     tm->tm_year = bcd2bin(buf[9]) + 100;
> > > +
> > > +     dev_dbg(&client->dev, "%s:s=%d, m=%d, hr=%d, md=%d, m=%d, yr=%d, wd=%d\n",
> > > +             __func__, tm->tm_sec, tm->tm_min, tm->tm_hour, tm->tm_mday, tm->tm_mon,
> > > +             tm->tm_year, tm->tm_wday);
> > > +

I forgot but this dev_dbg is not particularily useful as we have
tracepoint in the core. However, if you want to keep it, please use
%ptR.

> > > +     return 0;
> > > +}
> > > +
> > > +static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +     unsigned char buf[10] = {0};
> > > +     int err;
> > > +
> > > +     dev_dbg(&client->dev, "%s:s=%d, m=%d, hr=%d, md=%d, m=%d, yr=%d, wd=%d\n",
> > > +             __func__, tm->tm_sec, tm->tm_min, tm->tm_hour, tm->tm_mday, tm->tm_mon,
> > > +             tm->tm_year, tm->tm_wday);

Ditto

> > > +
> > > +     err = nct3018y_read_block_data(client, NCT3018Y_REG_CTRL, 1, buf);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     if (!(buf[0] & NCT3018Y_BIT_TWO)) {
> > > +             dev_err(&client->dev,
> > > +                     " TWO is not set.\n");
> >
> > This is not useful, what is TWO?
> 
> TWO stands for Time Registers Write Ownership
> for NCT3018Y, driver needs to set this bit before writing to other registers
> 

Can't you simply set it forcefully here instead of erroring out?

> >
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     /* hours, minutes and seconds */
> > > +     buf[NCT3018Y_REG_SC] = bin2bcd(tm->tm_sec);
> > > +     buf[NCT3018Y_REG_MN] = bin2bcd(tm->tm_min);
> > > +     buf[NCT3018Y_REG_HR] = bin2bcd(tm->tm_hour);
> > > +     buf[NCT3018Y_REG_DW] = tm->tm_wday & 0x07;
> > > +     buf[NCT3018Y_REG_DM] = bin2bcd(tm->tm_mday);
> > > +
> > > +     /* month, 1 - 12 */
> > > +     buf[NCT3018Y_REG_MO] = bin2bcd(tm->tm_mon+1);
> > > +
> > > +     /* year and century */
> >
> > Were is the century?
> 
> I will update the comment, for there is no century.
> 
> >
> > > +     buf[NCT3018Y_REG_YR] = bin2bcd(tm->tm_year - 100);
> > > +
> > > +     return nct3018y_write_block_data(client, NCT3018Y_REG_SC, 10, buf);

So this overwrites the alarm which is something you must not do.

> > > +     buf[0] = bin2bcd(tm->time.tm_sec);
> > > +     buf[1] = bin2bcd(tm->time.tm_min);
> > > +     buf[2] = bin2bcd(tm->time.tm_hour);
> > > +
> > > +     err = nct3018y_write_block_data(client, NCT3018Y_REG_SCA, 1, buf);
> > > +     if (err)
> > > +             return err;
> >
> >
> > Writing byte per byte opens a huge window for a race condition here.
> >
> 
> I write byte per byte,
> because these three registers are not continuous
> 

Right, I did see it and then forgot.

> > > +     nct3018y->rtc = devm_rtc_allocate_device(&client->dev);
> > > +     if (IS_ERR(nct3018y->rtc))
> > > +             return PTR_ERR(nct3018y->rtc);
> > > +
> > > +     nct3018y->rtc->ops = &nct3018y_rtc_ops;
> > > +     nct3018y->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> > > +     nct3018y->rtc->range_max = RTC_TIMESTAMP_END_2099;
> > > +     nct3018y->rtc->set_start_time = true;
> >
> > Do you have a good reason to set set_start_time here?
> >
> 
> Sorry, I am new here.
> I just follow other drivers.
> so you think I should not set set_start_time, right?
> 

There are very few drivers that needs it, when they used to window the
dates they support back to 1970 which is not something you seem to care
about.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com