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

[Mining Lin <mimi05633@gmail.com>]

Dear Alexandre,

Thank you for your comments.
I will refine and reply below.

Thanks.
Best Regards,
Mia

Medad Young <medadyoung@gmail.com> 於 2022年7月7日 週四 下午1:31寫道：
>
> Hello Alexandre,
>
> Thanks for your comments.
> I add Mining Lin <mimi05633@gmail.com> into this mail thread,
> and she is going to follow up this RTC driver.
> She will be in charge of maintaining this driver.
>
> Alexandre Belloni <alexandre.belloni@bootlin.com> 於 2022年6月25日 週六 凌晨4:26寫道：
> >
> > Hello,
> >
> > Please run ./scripts/checkpatch.pl --strict on your patch, there are a
> > bunch of issues.
> >
[Mia] I will run ./scripts/checkpatch.pl --strict on my patch to fix issues.

> > On 27/05/2022 16:46:47+0800, medadyoung@gmail.com wrote:
> > > +static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on)
> > > +{
> > > +     int err, flags;
> > > +
> > > +     dev_dbg(&client->dev, "%s:on:%d\n", __func__, on);
> > > +
> > > +     flags =  i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
> > > +     if (flags < 0) {
> > > +             dev_err(&client->dev,
> > > +                     "Failed to read NCT3018Y_REG_CTRL\n");
> >
> > You should cut down on the number of error messages, they are usually
> > not useful as the user doesn't have any specific action after getting
> > one of them apart from trying the action once again. Also, this will
> > make your code shorter. dev_dbg is fine.
> >
[Mia] I will modify dev_err to dev_dbg if there is an error and nothing to do.

> > > +/*
> > > + * In the routines that deal directly with the nct3018y hardware, we use
> > > + * rtc_time -- month 0-11, hour 0-23, yr = calendar year-epoch.
> > > + */
> > > +static int nct3018y_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +     unsigned char buf[10];
> > > +     int err;
> > > +
> >
> > You should still return an error if the time is invalid there but without
> > an error message.
> >
[Mia] I will verify the time by rtc_valid_tm(tm).

> > > +     err = i2c_smbus_read_i2c_block_data(client, NCT3018Y_REG_SC, sizeof(buf), buf);
> > > +     if (err < 0)
> > > +             return err;
> > > +
> > > +     tm->tm_sec = bcd2bin(buf[0] & 0x7F);
> > > +     tm->tm_min = bcd2bin(buf[2] & 0x7F);
> > > +     tm->tm_hour = bcd2bin(buf[4] & 0x3F);
> > > +     tm->tm_wday = buf[6] & 0x07;
> > > +     tm->tm_mday = bcd2bin(buf[7] & 0x3F);
> > > +     tm->tm_mon = bcd2bin(buf[8] & 0x1F) - 1;
> > > +     tm->tm_year = bcd2bin(buf[9]) + 100;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int nct3018y_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *tm)
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +     unsigned char buf[5];
> > > +     int err;
> > > +
> > > +     err = i2c_smbus_read_i2c_block_data(client, NCT3018Y_REG_SCA,
> > > +                                         sizeof(buf), buf);
> > > +     if (err < 0) {
> > > +             dev_err(&client->dev, "Unable to read date\n");
> > > +             return -EIO;
> > > +     }
> > > +
> > > +     dev_dbg(&client->dev, "%s: raw data is sec=%02x, min=%02x hr=%02x\n",
> > > +             __func__, buf[0], buf[2], buf[4]);
> > > +
> > > +     tm->time.tm_sec = bcd2bin(buf[0] & 0x7F);
> > > +     tm->time.tm_min = bcd2bin(buf[2] & 0x7F);
> > > +     tm->time.tm_hour = bcd2bin(buf[4] & 0x3F);
> > > +
> > > +     err = nct3018y_get_alarm_mode(client, &tm->enabled, &tm->pending);
> > > +     if (err < 0)
> > > +             return err;
> > > +
> > > +     dev_dbg(&client->dev, "%s:s=%d m=%d, hr=%d, enabled=%d, pending=%d\n",
> > > +             __func__, tm->time.tm_sec, tm->time.tm_min,
> > > +             tm->time.tm_hour, tm->enabled, tm->pending);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int nct3018y_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm)
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +     unsigned char buf[3];
> > > +     int err;
> > > +
> > > +     dev_dbg(dev, "%s, sec=%d, min=%d hour=%d tm->enabled:%d\n",
> > > +             __func__, tm->time.tm_sec, tm->time.tm_min, tm->time.tm_hour,
> > > +             tm->enabled);
> > > +
> > > +     buf[0] = bin2bcd(tm->time.tm_sec);
> > > +     buf[1] = bin2bcd(tm->time.tm_min);
> > > +     buf[2] = bin2bcd(tm->time.tm_hour);
> >
> > I don't think buf is useful in this function
[Mia]  I will remove buf in this function.

> > > +
> > > +     err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_SCA, buf[0]);
> > > +     if (err < 0) {
> > > +             dev_err(&client->dev, "Unable to write NCT3018Y_REG_SCA\n");
> > > +             return err;
> > > +     }
> > > +
> > > +     err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_MNA, buf[1]);
> > > +     if (err < 0) {
> > > +             dev_err(&client->dev, "Unable to write NCT3018Y_REG_MNA\n");
> > > +             return err;
> > > +     }
> > > +
> > > +     err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_HRA, buf[2]);
> > > +     if (err < 0) {
> > > +             dev_err(&client->dev, "Unable to write NCT3018Y_REG_HRA\n");
> > > +             return err;
> > > +     }
> > > +
> > > +     return nct3018y_set_alarm_mode(client, tm->enabled);
> > > +}
> > > +
> >
> >
> > > +static struct clk *nct3018y_clkout_register_clk(struct nct3018y *nct3018y)
> > > +{
> > > +     struct i2c_client *client = nct3018y->client;
> > > +     struct device_node *node = client->dev.of_node;
> > > +     struct clk *clk;
> > > +     struct clk_init_data init;
> > > +     int flags, err;
> > > +
> > > +     /* disable the clkout output */
> > > +     flags = 0;
> > > +     err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CLKO, flags);
> >
> > BTW, this introduces a glitch in the clock output if the clock is
> > actually used. Maybe you could just rely on the CCF core to disable this
> > clock when there are no users.
> >
[Mia] Do you mean there is no need to disable the clock output here?

> > > +     if (err < 0) {
> > > +             dev_err(&client->dev, "Unable to write oscillator status register\n");
> > > +             return ERR_PTR(err);
> > > +     }
> > > +
> > > +     init.name = "nct3018y-clkout";
> > > +     init.ops = &nct3018y_clkout_ops;
> > > +     init.flags = 0;
> > > +     init.parent_names = NULL;
> > > +     init.num_parents = 0;
> > > +     nct3018y->clkout_hw.init = &init;
> > > +
> > > +     /* optional override of the clockname */
> > > +     of_property_read_string(node, "clock-output-names", &init.name);
> > > +
> > > +     /* register the clock */
> > > +     clk = devm_clk_register(&client->dev, &nct3018y->clkout_hw);
> > > +
> > > +     if (!IS_ERR(clk))
> > > +             of_clk_add_provider(node, of_clk_src_simple_get, clk);
> > > +
> > > +     return clk;
> > > +}
> > > +#endif
> > > +
> > > +static const struct rtc_class_ops nct3018y_rtc_ops = {
> > > +     .read_time      = nct3018y_rtc_read_time,
> > > +     .set_time       = nct3018y_rtc_set_time,
> > > +     .read_alarm     = nct3018y_rtc_read_alarm,
> > > +     .set_alarm      = nct3018y_rtc_set_alarm,
> > > +     .alarm_irq_enable = nct3018y_irq_enable,
> > > +     .ioctl          = nct3018y_ioctl,
> > > +};
> > > +
> > > +static int nct3018y_probe(struct i2c_client *client,
> > > +                       const struct i2c_device_id *id)
> > > +{
> > > +     struct nct3018y *nct3018y;
> > > +     int err, flags;
> > > +
> > > +     if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> >
> > Don't you rather need I2C_FUNC_SMBUS_BYTE and I2C_FUNC_SMBUS_BLOCK_DATA?
> >
[Mia] I will also check I2C_FUNC_SMBUS_BYTE and I2C_FUNC_SMBUS_BLOCK_DATA.

> > > +             dev_err(&client->dev, "%s: ENODEV\n", __func__);
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     nct3018y = devm_kzalloc(&client->dev, sizeof(struct nct3018y),
> > > +                             GFP_KERNEL);
> > > +     if (!nct3018y)
> > > +             return -ENOMEM;
> > > +
> > > +     i2c_set_clientdata(client, nct3018y);
> > > +     nct3018y->client = client;
> > > +     device_set_wakeup_capable(&client->dev, 1);
> > > +
> > > +     flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
> > > +     if (flags < 0) {
> > > +             dev_err(&client->dev, "%s: read error\n", __func__);
> > > +             return flags;
> > > +     } else if (flags & NCT3018Y_BIT_TWO)
> > > +             dev_dbg(&client->dev, "%s: NCT3018Y_BIT_TWO is set\n", __func__);
> > > +
> > > +
> > > +     flags = NCT3018Y_BIT_TWO;
> > > +     err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CTRL, flags);
> > > +     if (err < 0) {
> > > +             dev_err(&client->dev, "Unable to write NCT3018Y_REG_CTRL\n");
> > > +             return err;
> > > +     }
> > > +
> > > +     flags = 0;
> > > +     err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_ST, flags);
> > > +     if (err < 0) {
> > > +             dev_err(&client->dev, "%s: write error\n", __func__);
> > > +             return err;
> > > +     }
> > > +
> > > +
> > > +     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;
> > > +
> > > +     if (client->irq > 0) {
> > > +             err = devm_request_threaded_irq(&client->dev, client->irq,
> > > +                             NULL, nct3018y_irq,
> > > +                             IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> > > +                             "nct3018y", client);
> > > +             if (err) {
> > > +                     dev_err(&client->dev, "unable to request IRQ %d\n", client->irq);
> > > +                     return err;
> > > +             }
> > > +     }
> >
> > You need to clear RTC_FEATURE_UPDATE_INTERRUPT and RTC_FEATURE_ALARM
> > from nct3018y->rtc->features when client->irq <= 0
> >
[Mia] I will clear RTC_FEATURE_UPDATE_INTERRUPT and RTC_FEATURE_ALARM
if client->irq <= 0.

> > > +
> > > +     return devm_rtc_register_device(nct3018y->rtc);
> > > +
> >
> > > +#ifdef CONFIG_COMMON_CLK
> > > +     /* register clk in common clk framework */
> > > +     nct3018y_clkout_register_clk(nct3018y);
> > > +#endif
> > > +
> >
> > This code is not reachable anymore
> >
[Mia] I will refine it. Thank you.

> > > +     return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id nct3018y_id[] = {
> > > +     { "nct3018y", 0 },
> > > +     { }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, nct3018y_id);
> > > +
> > > +
> > > +static const struct of_device_id nct3018y_of_match[] = {
> > > +     { .compatible = "nuvoton,nct3018y" },
> > > +     {}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, nct3018y_of_match);
> > > +
> > > +static struct i2c_driver nct3018y_driver = {
> > > +     .driver         = {
> > > +             .name   = "rtc-nct3018y",
> > > +             .of_match_table = of_match_ptr(nct3018y_of_match),
> > > +     },
> > > +     .probe          = nct3018y_probe,
> > > +     .id_table       = nct3018y_id,
> > > +};
> > > +
> > > +module_i2c_driver(nct3018y_driver);
> > > +
> > > +MODULE_AUTHOR("Medad CChien <ctcchien@nuvoton.com>");
> > > +MODULE_DESCRIPTION("Nuvoton NCT3018Y RTC driver");
> > > +MODULE_LICENSE("GPL v2");
> > > --
> > > 2.17.1
> > >
> >
> > --
> > Alexandre Belloni, co-owner and COO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
>
> B.R.
> Medad