Compatible with NCT3015Y-R and NCT3018Y-R

PATCH Superseded Mia Lin 2 participants 6 messages Sep 13, 2023 – Nov 9, 2023

Subsystems (2)

Real time clock (RTC) Arm/nuvoton npcm architecture

lore

Share & tools

lore

Mia Lin

author

2023-09-13 01:37 UTC #0 lore raw

v6 cover Compatible with NCT3015Y-R and NCT3018Y-R

Changes since version 6:
Move the part number parameter to struct nct3018y.

Changes since version 5:
Add global parameter to store part number.
Remove unnecessary changes for easier reading.

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 | 52 +++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 6 deletions(-)

Signature · 1 line

2.17.1

Mia Lin

author

2023-09-13 01:37 UTC #1 lore raw

PATCH v6 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.

Signed-off-by Mia Lin <[email protected]>

---
 drivers/rtc/rtc-nct3018y.c | 52 +++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 6 deletions(-)

drivers/rtc/rtc-nct3018y.c +46 -6

index ed4e606be8e5..b006b58e15e2 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,10 +38,12 @@
 #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;
 	struct i2c_client *client;
+	int part_num;
 #ifdef CONFIG_COMMON_CLK
 	struct clk_hw clkout_hw;
 #endif

@@ -177,8 +180,27 @@ static int nct3018y_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct nct3018y *nct3018y = dev_get_drvdata(dev);
 	unsigned char buf[4] = {0};
-	int err;
+	int err, flags;
+	int restore_flags = 0;
+
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
+	if (flags < 0) {
+		dev_dbg(&client->dev, "Failed to read NCT3018Y_REG_CTRL.\n");
+		return flags;
+	}
+
+	/* Check and set TWO bit */
+	if ((nct3018y->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, "Unable to write NCT3018Y_REG_CTRL.\n");
+			return err;
+		}
+	}
 
 	buf[0] = bin2bcd(tm->tm_sec);
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_SC, buf[0]);

@@ -212,6 +234,18 @@ static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		return -EIO;
 	}
 
+	/* Restore TWO bit */
+	if (restore_flags) {
+		if (nct3018y->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, "Unable to write NCT3018Y_REG_CTRL.\n");
+			return err;
+		}
+	}
+
 	return err;
 }

@@ -479,11 +513,17 @@ static int nct3018y_probe(struct i2c_client *client)
 		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_dbg(&client->dev, "Unable to write NCT3018Y_REG_CTRL\n");
-		return err;
+	nct3018y->part_num = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
+	if (nct3018y->part_num < 0) {
+		dev_dbg(&client->dev, "Failed to read NCT3018Y_REG_PART.\n");
+		return nct3018y->part_num;
+	} else if (nct3018y->part_num & 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, "Unable to write NCT3018Y_REG_CTRL.\n");
+			return err;
+		}
 	}
 
 	flags = 0;

Signature · 1 line

2.17.1

Alexandre Belloni

maintainer

2023-11-03 23:16 UTC #2 lore raw

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

Quoted 99 lines · Mia Lin

Signed-off-by Mia Lin <[email protected]>

---
 drivers/rtc/rtc-nct3018y.c | 52 +++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 6 deletions(-)

drivers/rtc/rtc-nct3018y.c +35 -1

index ed4e606be8e5..b006b58e15e2 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,10 +38,12 @@
 #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;
 	struct i2c_client *client;
+	int part_num;
 #ifdef CONFIG_COMMON_CLK
 	struct clk_hw clkout_hw;
 #endif

@@ -177,8 +180,27 @@ static int nct3018y_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct nct3018y *nct3018y = dev_get_drvdata(dev);
 	unsigned char buf[4] = {0};
-	int err;
+	int err, flags;
+	int restore_flags = 0;
+
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
+	if (flags < 0) {
+		dev_dbg(&client->dev, "Failed to read NCT3018Y_REG_CTRL.\n");
+		return flags;
+	}
+
+	/* Check and set TWO bit */
+	if ((nct3018y->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, "Unable to write NCT3018Y_REG_CTRL.\n");
+			return err;
+		}
+	}
 
 	buf[0] = bin2bcd(tm->tm_sec);
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_SC, buf[0]);

@@ -212,6 +234,18 @@ static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		return -EIO;
 	}
 
+	/* Restore TWO bit */
+	if (restore_flags) {
+		if (nct3018y->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, "Unable to write NCT3018Y_REG_CTRL.\n");
+			return err;
+		}
+	}
+
 	return err;
 }

@@ -479,11 +513,17 @@ static int nct3018y_probe(struct i2c_client *client)
 		dev_dbg(&client->dev, "%s: NCT3018Y_BIT_TWO is set\n", __func__);
 	}

drivers/rtc/rtc-nct3018y.c static int nct3018y_probe(struct i2c_client *client) · line 520

Quoted 10 lines

drivers/rtc/rtc-nct3018y.c +5 -5

-	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;
+	nct3018y->part_num = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
+	if (nct3018y->part_num < 0) {
+		dev_dbg(&client->dev, "Failed to read NCT3018Y_REG_PART.\n");
+		return nct3018y->part_num;
+	} else if (nct3018y->part_num & NCT3018Y_REG_PART_NCT3018Y) {

This is a weird way to check as this will accept any value of NCT3018Y_REG_PART as long as bit 1 is set, is this really what you want?

Quoted 12 lines

drivers/rtc/rtc-nct3018y.c +6 -0

+		flags = NCT3018Y_BIT_HF;
+		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 = 0;

Signature · 1 line

2.17.1

Signature · 3 lines

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

Alexandre Belloni

maintainer

2023-11-03 23:17 UTC #3 lore raw

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

Quoted 18 lines · Alexandre Belloni

Quoted 17 lines · Mia Lin
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.

Quoted 2 lines

Quoted 2 lines
Signed-off-by Mia Lin <[email protected]>
---

I forgot to add, please include a changelog here, this will make my reviews easier (and faster).

Quoted 111 lines

Quoted 80 lines

 drivers/rtc/rtc-nct3018y.c | 52 +++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 6 deletions(-)

drivers/rtc/rtc-nct3018y.c +35 -1

index ed4e606be8e5..b006b58e15e2 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,10 +38,12 @@
 #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;
 	struct i2c_client *client;
+	int part_num;
 #ifdef CONFIG_COMMON_CLK
 	struct clk_hw clkout_hw;
 #endif

@@ -177,8 +180,27 @@ static int nct3018y_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct nct3018y *nct3018y = dev_get_drvdata(dev);
 	unsigned char buf[4] = {0};
-	int err;
+	int err, flags;
+	int restore_flags = 0;
+
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
+	if (flags < 0) {
+		dev_dbg(&client->dev, "Failed to read NCT3018Y_REG_CTRL.\n");
+		return flags;
+	}
+
+	/* Check and set TWO bit */
+	if ((nct3018y->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, "Unable to write NCT3018Y_REG_CTRL.\n");
+			return err;
+		}
+	}
 
 	buf[0] = bin2bcd(tm->tm_sec);
 	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_SC, buf[0]);

@@ -212,6 +234,18 @@ static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		return -EIO;
 	}
 
+	/* Restore TWO bit */
+	if (restore_flags) {
+		if (nct3018y->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, "Unable to write NCT3018Y_REG_CTRL.\n");
+			return err;
+		}
+	}
+
 	return err;
 }

@@ -479,11 +513,17 @@ static int nct3018y_probe(struct i2c_client *client)
 		dev_dbg(&client->dev, "%s: NCT3018Y_BIT_TWO is set\n", __func__);
 	}

drivers/rtc/rtc-nct3018y.c static int nct3018y_probe(struct i2c_client *client) · line 520

Quoted 10 lines

drivers/rtc/rtc-nct3018y.c +5 -5

-	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;
+	nct3018y->part_num = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
+	if (nct3018y->part_num < 0) {
+		dev_dbg(&client->dev, "Failed to read NCT3018Y_REG_PART.\n");
+		return nct3018y->part_num;
+	} else if (nct3018y->part_num & NCT3018Y_REG_PART_NCT3018Y) {

This is a weird way to check as this will accept any value of NCT3018Y_REG_PART as long as bit 1 is set, is this really what you want?

Quoted 12 lines

drivers/rtc/rtc-nct3018y.c +6 -0

+		flags = NCT3018Y_BIT_HF;
+		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 = 0;

Signature · 1 line

2.17.1

Signature · 3 lines

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

Mining Lin

author

2023-11-09 12:33 UTC #4 lore raw

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

Dear Alexandre,

I will add change log next patch.

Thanks.
Best regard,
Mia

Sent from my iPhone

Quoted 143 lines · Alexandre Belloni

Quoted 18 lines · Alexandre Belloni

Quoted 18 lines · Mia Lin
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.

Quoted 2 lines

Quoted 2 lines
Signed-off-by Mia Lin <[email protected]>
---

I forgot to add, please include a changelog here, this will make my reviews easier (and faster).

Quoted 111 lines

Quoted 80 lines

drivers/rtc/rtc-nct3018y.c | 52 +++++++++++++++++++++++++++++++++-----

1 file changed, 46 insertions(+), 6 deletions(-)

drivers/rtc/rtc-nct3018y.c +35 -1

index ed4e606be8e5..b006b58e15e2 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,10 +38,12 @@
 #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;
   struct i2c_client *client;
+    int part_num;
 #ifdef CONFIG_COMMON_CLK
   struct clk_hw clkout_hw;
 #endif

@@ -177,8 +180,27 @@ static int nct3018y_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
   struct i2c_client *client = to_i2c_client(dev);
+    struct nct3018y *nct3018y = dev_get_drvdata(dev);
   unsigned char buf[4] = {0};
-    int err;
+    int err, flags;
+    int restore_flags = 0;
+
+    flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
+    if (flags < 0) {
+        dev_dbg(&client->dev, "Failed to read NCT3018Y_REG_CTRL.\n");
+        return flags;
+    }
+
+    /* Check and set TWO bit */
+    if ((nct3018y->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, "Unable to write NCT3018Y_REG_CTRL.\n");
+            return err;
+        }
+    }
 
   buf[0] = bin2bcd(tm->tm_sec);
   err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_SC, buf[0]);

@@ -212,6 +234,18 @@ static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
       return -EIO;
   }
 
+    /* Restore TWO bit */
+    if (restore_flags) {
+        if (nct3018y->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, "Unable to write NCT3018Y_REG_CTRL.\n");
+            return err;
+        }
+    }
+
   return err;
 }

@@ -479,11 +513,17 @@ static int nct3018y_probe(struct i2c_client *client)
       dev_dbg(&client->dev, "%s: NCT3018Y_BIT_TWO is set\n", __func__);
   }

drivers/rtc/rtc-nct3018y.c static int nct3018y_probe(struct i2c_client *client) · line 520

Quoted 10 lines

drivers/rtc/rtc-nct3018y.c +5 -5

-    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;
+    nct3018y->part_num = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
+    if (nct3018y->part_num < 0) {
+        dev_dbg(&client->dev, "Failed to read NCT3018Y_REG_PART.\n");
+        return nct3018y->part_num;
+    } else if (nct3018y->part_num & NCT3018Y_REG_PART_NCT3018Y) {

This is a weird way to check as this will accept any value of NCT3018Y_REG_PART as long as bit 1 is set, is this really what you want?

Quoted 12 lines

drivers/rtc/rtc-nct3018y.c +6 -0

+        flags = NCT3018Y_BIT_HF;
+        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 = 0;

Signature · 1 line

2.17.1

Signature · 3 lines

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

Mining Lin

author

2023-11-09 12:31 UTC #5 lore raw

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

In reply to Alexandre Belloni

Dear Alexandre,

Thanks for your comments.
The replies are as follow.

Thanks.
Best regard,
Mia

Sent from my iPhone

Quoted 102 lines · Alexandre Belloni

Quoted 99 lines · Mia Lin

Signed-off-by Mia Lin <[email protected]>

---
drivers/rtc/rtc-nct3018y.c | 52 +++++++++++++++++++++++++++++++++-----

1 file changed, 46 insertions(+), 6 deletions(-)

drivers/rtc/rtc-nct3018y.c +35 -1

index ed4e606be8e5..b006b58e15e2 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,10 +38,12 @@
 #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;
   struct i2c_client *client;
+    int part_num;
 #ifdef CONFIG_COMMON_CLK
   struct clk_hw clkout_hw;
 #endif

@@ -177,8 +180,27 @@ static int nct3018y_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
   struct i2c_client *client = to_i2c_client(dev);
+    struct nct3018y *nct3018y = dev_get_drvdata(dev);
   unsigned char buf[4] = {0};
-    int err;
+    int err, flags;
+    int restore_flags = 0;
+
+    flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
+    if (flags < 0) {
+        dev_dbg(&client->dev, "Failed to read NCT3018Y_REG_CTRL.\n");
+        return flags;
+    }
+
+    /* Check and set TWO bit */
+    if ((nct3018y->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, "Unable to write NCT3018Y_REG_CTRL.\n");
+            return err;
+        }
+    }
 
   buf[0] = bin2bcd(tm->tm_sec);
   err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_SC, buf[0]);

@@ -212,6 +234,18 @@ static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
       return -EIO;
   }
 
+    /* Restore TWO bit */
+    if (restore_flags) {
+        if (nct3018y->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, "Unable to write NCT3018Y_REG_CTRL.\n");
+            return err;
+        }
+    }
+
   return err;
 }

@@ -479,11 +513,17 @@ static int nct3018y_probe(struct i2c_client *client)
       dev_dbg(&client->dev, "%s: NCT3018Y_BIT_TWO is set\n", __func__);
   }

Quoted 13 lines

drivers/rtc/rtc-nct3018y.c static int nct3018y_probe(struct i2c_client *client) · line 520

Quoted 10 lines

drivers/rtc/rtc-nct3018y.c +5 -5

-    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;
+    nct3018y->part_num = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
+    if (nct3018y->part_num < 0) {
+        dev_dbg(&client->dev, "Failed to read NCT3018Y_REG_PART.\n");
+        return nct3018y->part_num;
+    } else if (nct3018y->part_num & NCT3018Y_REG_PART_NCT3018Y) {

This is a weird way to check as this will accept any value of NCT3018Y_REG_PART as long as bit 1 is set, is this really what you want?

［Mia] Thanks for your comments. I’ll fix it.

Quoted 18 lines

Quoted 13 lines

drivers/rtc/rtc-nct3018y.c +6 -0

+        flags = NCT3018Y_BIT_HF;
+        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 = 0;

Signature · 1 line

2.17.1

Signature · 3 lines

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