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

[Medad Young <medadyoung@gmail.com>]

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.
>
> 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.
>
> > +/*
> > + * 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.
>
> > +     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
> > +
> > +     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.
>
> > +     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?
>
> > +             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
>
> > +
> > +     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
>
> > +     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