<div dir="ltr"><div dir="ltr">Dear Alexandre,<div><br></div><div>Thanks for your comments.</div><div>The replies are as follow.</div><div><br></div><div>Thanks.</div><div>Best regard,</div><div>Mia<br><br>Alexandre Belloni <<a href="mailto:alexandre.belloni@bootlin.com" target="_blank">alexandre.belloni@bootlin.com</a>> 於 2023年8月24日 星期四寫道：<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br>
<br>
On 16/08/2023 09:25:40+0800, Mia Lin wrote:<br>
> -     dev_dbg(&client->dev, "%s:alarm_enable:%x alarm_flag:%x\n",<br>
> -             __func__, *alarm_enable, *alarm_flag);<br>
> +     if (alarm_enable && alarm_flag)<br>
<br>
I don't really see the point of conditionally displaying this debug<br>
message.<br></blockquote><div>[Mia] I will remove it. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +             dev_dbg(&client->dev, "%s: alarm_enable=%x, alarm_flag=%x.\n",<br>
> +                     __func__, *alarm_enable, *alarm_flag);<br>
>  <br>
>       return 0;<br>
>  }<br>
> @@ -123,17 +124,17 @@ static irqreturn_t nct3018y_irq(int irq, void *dev_id)<br>
>       unsigned char alarm_flag;<br>
>       unsigned char alarm_enable;<br>
>  <br>
> -     dev_dbg(&client->dev, "%s:irq:%d\n", __func__, irq);<br>
> +     dev_dbg(&client->dev, "%s: irq=%d.\n", __func__, irq);<br>
<br>
You have many of those changes where you only add a space, I feel like<br>
this is a matter of taste and this makes it more difficult than<br>
necessary to read your patch.<br></blockquote><div> [Mia] Sorry for the ambiguity. I will remove those unnecessary changes.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
>       err = nct3018y_get_alarm_mode(nct3018y->client, &alarm_enable, &alarm_flag);<br>
>       if (err)<br>
>               return IRQ_NONE;<br>
>  <br>
>       if (alarm_flag) {<br>
> -             dev_dbg(&client->dev, "%s:alarm flag:%x\n",<br>
> +             dev_dbg(&client->dev, "%s: alarm flag=%x.\n",<br>
>                       __func__, alarm_flag);<br>
>               rtc_update_irq(nct3018y->rtc, 1, RTC_IRQF | RTC_AF);<br>
>               nct3018y_set_alarm_mode(nct3018y->client, 0);<br>
> -             dev_dbg(&client->dev, "%s:IRQ_HANDLED\n", __func__);<br>
> +             dev_dbg(&client->dev, "%s: IRQ_HANDLED.\n", __func__);<br>
>               return IRQ_HANDLED;<br>
>       }<br>
>  <br>
> @@ -155,7 +156,7 @@ static int nct3018y_rtc_read_time(struct device *dev, struct rtc_time *tm)<br>
>               return err;<br>
>  <br>
>       if (!buf[0]) {<br>
> -             dev_dbg(&client->dev, " voltage <=1.7, date/time is not reliable.\n");<br>
> +             dev_dbg(&client->dev, "%s: voltage <=1.7, date/time is not reliable.\n", __func__);<br>
>               return -EINVAL;<br>
>       }<br>
>  <br>
> @@ -178,26 +179,50 @@ static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)<br>
>  {<br>
>       struct i2c_client *client = to_i2c_client(dev);<br>
>       unsigned char buf[4] = {0};<br>
> -     int err;<br>
> +     int err, part_num, flags;<br>
> +     int restore_flags = 0;<br>
> +<br>
> +     part_num = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);<br>
<br>
Do you really have to check the part number every time you set the time?<br>
I don't expect it to change once read in probe.<br></blockquote>[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.<br><div>Therefore, we use part number & TWO bit to determine whether we need to set the TWO bit first before set time. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +     if (part_num < 0) {<br>
> +             dev_dbg(&client->dev, "%s: Failed to read part info reg.\n", __func__);<br>
> +             return part_num;<br>
> +     }<br>
> +<br>
<br>
-- <br>
Alexandre Belloni, co-owner and COO, Bootlin<br>
Embedded Linux and Kernel engineering<br>
<a href="https://bootlin.com" target="_blank">https://bootlin.com</a><br>
</blockquote></div>
</div></div>