[PATCH v4 1/1] rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R

[Minying Lin]

Dear Alexandre,

Thanks for your comments.
The replies are as follow.

Thanks.
Best regard,
Mia

Alexandre Belloni <alexandre.belloni at bootlin.com> 於 2023年8月24日 星期四寫道：

> Hello,
>
> On 16/08/2023 09:25:40+0800, Mia Lin wrote:
> > -     dev_dbg(&client->dev, "%s:alarm_enable:%x alarm_flag:%x\n",
> > -             __func__, *alarm_enable, *alarm_flag);
> > +     if (alarm_enable && alarm_flag)
>
> I don't really see the point of conditionally displaying this debug
> message.
>
[Mia] I will remove it.

>
> > +             dev_dbg(&client->dev, "%s: alarm_enable=%x,
> alarm_flag=%x.\n",
> > +                     __func__, *alarm_enable, *alarm_flag);
> >
> >       return 0;
> >  }
> > @@ -123,17 +124,17 @@ static irqreturn_t nct3018y_irq(int irq, void
> *dev_id)
> >       unsigned char alarm_flag;
> >       unsigned char alarm_enable;
> >
> > -     dev_dbg(&client->dev, "%s:irq:%d\n", __func__, irq);
> > +     dev_dbg(&client->dev, "%s: irq=%d.\n", __func__, irq);
>
> You have many of those changes where you only add a space, I feel like
> this is a matter of taste and this makes it more difficult than
> necessary to read your patch.
>
 [Mia] Sorry for the ambiguity. I will remove those unnecessary changes.

>
> >       err = nct3018y_get_alarm_mode(nct3018y->client, &alarm_enable,
> &alarm_flag);
> >       if (err)
> >               return IRQ_NONE;
> >
> >       if (alarm_flag) {
> > -             dev_dbg(&client->dev, "%s:alarm flag:%x\n",
> > +             dev_dbg(&client->dev, "%s: alarm flag=%x.\n",
> >                       __func__, alarm_flag);
> >               rtc_update_irq(nct3018y->rtc, 1, RTC_IRQF | RTC_AF);
> >               nct3018y_set_alarm_mode(nct3018y->client, 0);
> > -             dev_dbg(&client->dev, "%s:IRQ_HANDLED\n", __func__);
> > +             dev_dbg(&client->dev, "%s: IRQ_HANDLED.\n", __func__);
> >               return IRQ_HANDLED;
> >       }
> >
> > @@ -155,7 +156,7 @@ static int nct3018y_rtc_read_time(struct device
> *dev, struct rtc_time *tm)
> >               return err;
> >
> >       if (!buf[0]) {
> > -             dev_dbg(&client->dev, " voltage <=1.7, date/time is not
> reliable.\n");
> > +             dev_dbg(&client->dev, "%s: voltage <=1.7, date/time is not
> reliable.\n", __func__);
> >               return -EINVAL;
> >       }
> >
> > @@ -178,26 +179,50 @@ static int nct3018y_rtc_set_time(struct device
> *dev, struct rtc_time *tm)
> >  {
> >       struct i2c_client *client = to_i2c_client(dev);
> >       unsigned char buf[4] = {0};
> > -     int err;
> > +     int err, part_num, flags;
> > +     int restore_flags = 0;
> > +
> > +     part_num = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
>
> Do you really have to check the part number every time you set the time?
> I don't expect it to change once read in probe.
>
[Mia] Due to the 3018Y's topology, we need to set the TWO bit first to
obtain the write time capability, but the 3015Y does not have this problem.
Therefore, we use part number & TWO bit to determine whether we need to set
the TWO bit first before set time.

>
> > +     if (part_num < 0) {
> > +             dev_dbg(&client->dev, "%s: Failed to read part info
> reg.\n", __func__);
> > +             return part_num;
> > +     }
> > +
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20230829/59a506c5/attachment.htm>