Skip to contentSkip to conversation

Compatible with NCT3015Y-R and NCT3018Y-R

PATCH Superseded Mia Lin 2 participants 6 messages Aug 16, 2023 – Aug 31, 2023

Subsystems (2)
Real time clock (RTC) Arm/nuvoton npcm architecture

v4 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Mia Lin

author
#0 lore raw

v4 cover Compatible with NCT3015Y-R and NCT3018Y-R

Changes since version 4:
Add an introduction bewteen NCT3015Y-R and NCT3018Y-R. Restore the initial value of err in nct3018y_rtc_set_time(). Refine error messages to pinpoint the correct location.

Changes since version 3:
Remove the comparison between DT compatible and chip data.

Changes since version 2:
Add DT compatible to check the chip matches or not.

Changes since version 1:
rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R

Mia Lin (1):
rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R

 drivers/rtc/rtc-nct3018y.c | 122 +++++++++++++++++++++++++------------
 1 file changed, 83 insertions(+), 39 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

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

The NCT3015Y-R and NCT3018Y-R use the same datasheet
but have different topologies as follows.
- Topology (Only 1st i2c can set TWO bit and HF bit)
In NCT3015Y-R,
rtc 1st i2c is connected to a host CPU
rtc 2nd i2c is connected to a BMC
In NCT3018Y-R,
rtc 1st i2c is connected to a BMC
rtc 2nd i2c is connected to a host CPU
In order to be compatible with NCT3015Y-R and NCT3018Y-R,
- In probe,
If part number is NCT3018Y-R, only set HF bit to 24-Hour format. Else, do nothing
- In set_time,
If part number is NCT3018Y-R && TWO bit is 0,
change TWO bit to 1, and restore TWO bit after updating time.
- Refine error messages to pinpoint the correct location.

Signed-off-by Mia Lin <[email protected]>
---
 drivers/rtc/rtc-nct3018y.c | 122 +++++++++++++++++++++++++------------
 1 file changed, 83 insertions(+), 39 deletions(-)
drivers/rtc/rtc-nct3018y.c +83 -39
index a4e3f924837e..1a674e105682 100644
@@ -23,6 +23,7 @@
 #define NCT3018Y_REG_CTRL	0x0A /* timer control */
 #define NCT3018Y_REG_ST		0x0B /* status */
 #define NCT3018Y_REG_CLKO	0x0C /* clock out */
+#define NCT3018Y_REG_PART	0x21 /* part info */
 
 #define NCT3018Y_BIT_AF		BIT(7)
 #define NCT3018Y_BIT_ST		BIT(7)

@@ -37,6 +38,7 @@
 #define NCT3018Y_REG_BAT_MASK		0x07
 #define NCT3018Y_REG_CLKO_F_MASK	0x03 /* frequenc mask */
 #define NCT3018Y_REG_CLKO_CKE		0x80 /* clock out enabled */
+#define NCT3018Y_REG_PART_NCT3018Y	0x02
 
 struct nct3018y {
 	struct rtc_device *rtc;

@@ -50,12 +52,12 @@ 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);
+	dev_dbg(&client->dev, "%s: on=%d.\n", __func__, on);
 
-	flags =  i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
 	if (flags < 0) {
 		dev_dbg(&client->dev,
-			"Failed to read NCT3018Y_REG_CTRL\n");
+			"%s: Failed to read ctrl reg.\n", __func__);
 		return flags;
 	}
 

@@ -67,21 +69,21 @@ static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on)
 	flags |= NCT3018Y_BIT_CIE;
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CTRL, flags);
 	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to write NCT3018Y_REG_CTRL\n");
+		dev_dbg(&client->dev, "%s: Unable to write ctrl reg.\n", __func__);
 		return err;
 	}
 
-	flags =  i2c_smbus_read_byte_data(client, NCT3018Y_REG_ST);
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_ST);
 	if (flags < 0) {
 		dev_dbg(&client->dev,
-			"Failed to read NCT3018Y_REG_ST\n");
+			"%s: Failed to read status reg.\n", __func__);
 		return flags;
 	}
 
 	flags &= ~(NCT3018Y_BIT_AF);
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_ST, flags);
 	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to write NCT3018Y_REG_ST\n");
+		dev_dbg(&client->dev, "%s: Unable to write status reg.\n", __func__);
 		return err;
 	}
 

@@ -94,23 +96,22 @@ static int nct3018y_get_alarm_mode(struct i2c_client *client, unsigned char *ala
 	int flags;
 
 	if (alarm_enable) {
-		dev_dbg(&client->dev, "%s:NCT3018Y_REG_CTRL\n", __func__);
-		flags =  i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
+		flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
 		if (flags < 0)
 			return flags;
 		*alarm_enable = flags & NCT3018Y_BIT_AIE;
 	}
 
 	if (alarm_flag) {
-		dev_dbg(&client->dev, "%s:NCT3018Y_REG_ST\n", __func__);
-		flags =  i2c_smbus_read_byte_data(client, NCT3018Y_REG_ST);
+		flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_ST);
 		if (flags < 0)
 			return flags;
 		*alarm_flag = flags & NCT3018Y_BIT_AF;
 	}
 
-	dev_dbg(&client->dev, "%s:alarm_enable:%x alarm_flag:%x\n",
-		__func__, *alarm_enable, *alarm_flag);
+	if (alarm_enable && alarm_flag)
+		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);
 	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);
+	if (part_num < 0) {
+		dev_dbg(&client->dev, "%s: Failed to read part info reg.\n", __func__);
+		return part_num;
+	}
+
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
+	if (flags < 0) {
+		dev_dbg(&client->dev, "%s: Failed to read ctrl reg.\n", __func__);
+		return flags;
+	}
+
+	/* Check and set TWO bit */
+	if ((part_num & NCT3018Y_REG_PART_NCT3018Y) && !(flags & NCT3018Y_BIT_TWO)) {
+		restore_flags = 1;
+		flags |= NCT3018Y_BIT_TWO;
+		err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CTRL, flags);
+		if (err < 0) {
+			dev_dbg(&client->dev, "%s: Unable to write ctrl reg.\n", __func__);
+			return err;
+		}
+	}
 
 	buf[0] = bin2bcd(tm->tm_sec);
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_SC, buf[0]);
 	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to write NCT3018Y_REG_SC\n");
+		dev_dbg(&client->dev, "%s: Unable to write seconds reg.\n", __func__);
 		return err;
 	}
 
 	buf[0] = bin2bcd(tm->tm_min);
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_MN, buf[0]);
 	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to write NCT3018Y_REG_MN\n");
+		dev_dbg(&client->dev, "%s: Unable to write minutes reg.\n", __func__);
 		return err;
 	}
 
 	buf[0] = bin2bcd(tm->tm_hour);
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_HR, buf[0]);
 	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to write NCT3018Y_REG_HR\n");
+		dev_dbg(&client->dev, "%s: Unable to write hour reg.\n", __func__);
 		return err;
 	}
 

@@ -208,10 +233,22 @@ static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	err = i2c_smbus_write_i2c_block_data(client, NCT3018Y_REG_DW,
 					     sizeof(buf), buf);
 	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to write for day and mon and year\n");
+		dev_dbg(&client->dev, "%s: Unable to write for day and mon and year.\n", __func__);
 		return -EIO;
 	}
 
+	/* Restore TWO bit */
+	if (restore_flags) {
+		if (part_num & NCT3018Y_REG_PART_NCT3018Y)
+			flags &= ~NCT3018Y_BIT_TWO;
+
+		err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CTRL, flags);
+		if (err < 0) {
+			dev_dbg(&client->dev, "%s: Unable to write ctrl reg.\n", __func__);
+			return err;
+		}
+	}
+
 	return err;
 }
 

@@ -224,11 +261,11 @@ static int nct3018y_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *tm)
 	err = i2c_smbus_read_i2c_block_data(client, NCT3018Y_REG_SCA,
 					    sizeof(buf), buf);
 	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to read date\n");
+		dev_dbg(&client->dev, "%s: Unable to read date.\n", __func__);
 		return -EIO;
 	}
 
-	dev_dbg(&client->dev, "%s: raw data is sec=%02x, min=%02x hr=%02x\n",
+	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);

@@ -239,7 +276,7 @@ static int nct3018y_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *tm)
 	if (err < 0)
 		return err;
 
-	dev_dbg(&client->dev, "%s:s=%d m=%d, hr=%d, enabled=%d, pending=%d\n",
+	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);
 

@@ -251,25 +288,25 @@ static int nct3018y_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm)
 	struct i2c_client *client = to_i2c_client(dev);
 	int err;
 
-	dev_dbg(dev, "%s, sec=%d, min=%d hour=%d tm->enabled:%d\n",
+	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);
 
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_SCA, bin2bcd(tm->time.tm_sec));
 	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to write NCT3018Y_REG_SCA\n");
+		dev_dbg(&client->dev, "%s: Unable to write seconds alarm reg.\n", __func__);
 		return err;
 	}
 
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_MNA, bin2bcd(tm->time.tm_min));
 	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to write NCT3018Y_REG_MNA\n");
+		dev_dbg(&client->dev, "Unable to write minutes alarm reg.\n");
 		return err;
 	}
 
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_HRA, bin2bcd(tm->time.tm_hour));
 	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to write NCT3018Y_REG_HRA\n");
+		dev_dbg(&client->dev, "%s: Unable to write hour alarm reg.\n", __func__);
 		return err;
 	}
 

@@ -278,7 +315,7 @@ static int nct3018y_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm)
 
 static int nct3018y_irq_enable(struct device *dev, unsigned int enabled)
 {
-	dev_dbg(dev, "%s: alarm enable=%d\n", __func__, enabled);
+	dev_dbg(dev, "%s: alarm enable=%d.\n", __func__, enabled);
 
 	return nct3018y_set_alarm_mode(to_i2c_client(dev), enabled);
 }

@@ -473,23 +510,29 @@ static int nct3018y_probe(struct i2c_client *client)
 
 	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
 	if (flags < 0) {
-		dev_dbg(&client->dev, "%s: read error\n", __func__);
+		dev_dbg(&client->dev, "%s: Failed to read ctrl reg.\n", __func__);
 		return flags;
 	} else if (flags & NCT3018Y_BIT_TWO) {
-		dev_dbg(&client->dev, "%s: NCT3018Y_BIT_TWO is set\n", __func__);
+		dev_dbg(&client->dev, "%s: TWO bit is set.\n", __func__);
 	}
 
-	flags = NCT3018Y_BIT_TWO;
-	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CTRL, flags);
-	if (err < 0) {
-		dev_dbg(&client->dev, "Unable to write NCT3018Y_REG_CTRL\n");
-		return err;
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
+	if (flags < 0) {
+		dev_dbg(&client->dev, "%s: Failed to read part info reg.\n", __func__);
+		return flags;
+	} else if (flags & NCT3018Y_REG_PART_NCT3018Y) {
+		flags = NCT3018Y_BIT_HF;
+		err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CTRL, flags);
+		if (err < 0) {
+			dev_dbg(&client->dev, "%s: Unable to write ctrl reg.\n", __func__);
+			return err;
+		}
 	}
 
 	flags = 0;
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_ST, flags);
 	if (err < 0) {
-		dev_dbg(&client->dev, "%s: write error\n", __func__);
+		dev_dbg(&client->dev, "%s: Failed to clear status reg.\n", __func__);
 		return err;
 	}
 

@@ -507,7 +550,8 @@ static int nct3018y_probe(struct i2c_client *client)
 						IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
 						"nct3018y", client);
 		if (err) {
-			dev_dbg(&client->dev, "unable to request IRQ %d\n", client->irq);
+			dev_dbg(&client->dev, "%s: Unable to request IRQ %d.\n",
+				__func__, client->irq);
 			return err;
 		}
 	} else {
Signature · 1 line
2.17.1
maintainer
#2 lore raw

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

Hello,

drivers/rtc/rtc-nct3018y.c static int nct3018y_get_alarm_mode(struct i2c_client *client, unsigned char *ala · line 112

Quoted 3 lines · Mia Lin
drivers/rtc/rtc-nct3018y.c +1 -2
-	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.

drivers/rtc/rtc-nct3018y.c static irqreturn_t nct3018y_irq(int irq, void *dev_id) · line 127

Quoted 11 lines
drivers/rtc/rtc-nct3018y.c +3 -1
+		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.

Quoted 25 lines
drivers/rtc/rtc-nct3018y.c +3 -3
 	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;
 	}

drivers/rtc/rtc-nct3018y.c static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 185

Quoted 8 lines
drivers/rtc/rtc-nct3018y.c +4 -1
@@ -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.

Quoted 5 lines
drivers/rtc/rtc-nct3018y.c +5 -0
+	if (part_num < 0) {
+		dev_dbg(&client->dev, "%s: Failed to read part info reg.\n", __func__);
+		return part_num;
+	}
+
Signature · 3 lines
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
author
#3 lore raw

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

Dear Alexandre,

Thanks for your comments.
The replies are as follow.

Thanks.
Best regard,
Mia

Alexandre Belloni <[email protected]> 於 2023年8月24日 星期四寫道：

Quoted 10 lines

Hello,

drivers/rtc/rtc-nct3018y.c static int nct3018y_get_alarm_mode(struct i2c_client *client, unsigned char *ala · line 112

Quoted 3 lines · Mia Lin
drivers/rtc/rtc-nct3018y.c +1 -2
-     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.

Quoted 12 lines
Quoted 2 lines

+ dev_dbg(&client->dev, "%s: alarm_enable=%x,

alarm_flag=%x.\n",

drivers/rtc/rtc-nct3018y.c static irqreturn_t nct3018y_irq(int irq, void

Quoted 5 lines
drivers/rtc/rtc-nct3018y.c +1 -0
+                     __func__, *alarm_enable, *alarm_flag);
 
      return 0;
 }

@@ -123,17 +124,17 @@ static irqreturn_t nct3018y_irq(int irq, void

*dev_id)

Quoted 3 lines
drivers/rtc/rtc-nct3018y.c +0 -0
      unsigned char alarm_flag;
      unsigned char alarm_enable;
Quoted 7 lines

drivers/rtc/rtc-nct3018y.c static irqreturn_t nct3018y_irq(int irq, void *dev_id) · line 127

Quoted 2 lines
drivers/rtc/rtc-nct3018y.c +1 -1
-     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.

Quoted 40 lines

Review static irqreturn_t nct3018y_irq(int irq, void · line 128

Quoted 2 lines
(quoted diff) +0 -0
      err = nct3018y_get_alarm_mode(nct3018y->client, &alarm_enable,

&alarm_flag);

drivers/rtc/rtc-nct3018y.c static int nct3018y_rtc_read_time(struct device

Quoted 15 lines
drivers/rtc/rtc-nct3018y.c +2 -2
      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)

drivers/rtc/rtc-nct3018y.c static int nct3018y_rtc_read_time(struct device *dev, struct rtc_time *tm)

Quoted 4 lines
drivers/rtc/rtc-nct3018y.c +0 -1
              return err;
 
      if (!buf[0]) {
-             dev_dbg(&client->dev, " voltage <=1.7, date/time is not

reliable.\n");

Review static int nct3018y_rtc_read_time(struct device · line 159

Quoted 1 line
(quoted diff) +1 -0
+             dev_dbg(&client->dev, "%s: voltage <=1.7, date/time is not

reliable.\n", __func__);

drivers/rtc/rtc-nct3018y.c static int nct3018y_rtc_set_time(struct device

Quoted 4 lines
drivers/rtc/rtc-nct3018y.c +0 -0
              return -EINVAL;
      }
 

@@ -178,26 +179,50 @@ static int nct3018y_rtc_set_time(struct device

*dev, struct rtc_time *tm)

drivers/rtc/rtc-nct3018y.c static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 185

Quoted 8 lines
drivers/rtc/rtc-nct3018y.c +4 -1
 {
      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);
Quoted 3 lines

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.

Quoted 13 lines

Review static int nct3018y_rtc_set_time(struct device · line 187

Quoted 3 lines
(quoted diff) +2 -0
+     if (part_num < 0) {
+             dev_dbg(&client->dev, "%s: Failed to read part info

reg.\n", __func__);

drivers/rtc/rtc-nct3018y.c static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 190

Quoted 3 lines
drivers/rtc/rtc-nct3018y.c +3 -0
+             return part_num;
+     }
+

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

maintainer
#4 lore raw

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

Quoted 8 lines · Minying Lin
Quoted 3 lines

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.

Sure but why don't you store the info somewhere instead of reading it from the RTC every time?

Quoted 13 lines
Quoted 13 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+     if (part_num < 0) {
+             dev_dbg(&client->dev, "%s: Failed to read part info

reg.\n", __func__);

drivers/rtc/rtc-nct3018y.c static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 190

Quoted 3 lines
drivers/rtc/rtc-nct3018y.c +3 -0
+             return part_num;
+     }
+

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

Signature · 3 lines
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
author
#5 lore raw

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

Dear Alexandre,

Thanks for your comments.
I will store the part number to global parameter in probe and use it to check before setting time.

Thanks.
Best regard,
Mia

Sent from my iPhone

Quoted 33 lines · Alexandre Belloni
Quoted 8 lines · Minying Lin
Quoted 3 lines

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.

Sure but why don't you store the info somewhere instead of reading it from the RTC every time?

Quoted 13 lines
Quoted 13 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+     if (part_num < 0) {
+             dev_dbg(&client->dev, "%s: Failed to read part info

reg.\n", __func__);

drivers/rtc/rtc-nct3018y.c static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 190

Quoted 3 lines
drivers/rtc/rtc-nct3018y.c +3 -0
+             return part_num;
+     }
+

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

Signature · 3 lines
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com