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
