[PATCH v2 3/3] RTC: nuvoton: Add NCT3018Y real time clock driver

[Medad Young]

Dear Alexandre,

thanks for your comments

Alexandre Belloni <alexandre.belloni at bootlin.com> 於 2022年5月18日 週三 下午4:29寫道：
>
> 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

OK, I will add 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.

I understood, maybe I can remove it.
>
> > > > +     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

OK, I can remove it.

>
> > > > +
> > > > +     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?

I did set it forcefully in the probe function, so I will remove the
same operation here.

>
> > >
> > > > +             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.

yes, I will correct it

>
> > > > +     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.

thanks

>
> > > > +     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.

I got it.


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

B.R.
Medad