Compatible with NCT3015Y-R and NCT3018Y-R

Add DT compatible "nuvoton,nct3015y" to select

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

---
 Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml +0 -0

index 4f9b5604acd9..67fc60fd395c 100644

Add DT compatible "nuvoton,nct3015y" to select

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

---
  Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml +0 -0

index 4f9b5604acd9..67fc60fd395c 100644

Dear Mia,

Thank you for your patch. It’d be great if you mentioned nct3015y in the commit message summary/title. Maybe:

dt-bindings: rtc: Add compatible nct3015y to nuvoton,nct3018y

Quoted 2 lines

Am 09.08.23 um 11:51 schrieb Mia Lin:
Add DT compatible "nuvoton,nct3015y" to select

What do you mean by “to select”?

Also, maybe add how you tested this.

Quoted 9 lines

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

---
 Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml +0 -0

index 4f9b5604acd9..67fc60fd395c 100644

Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml maintainers: · line 19

Quoted 6 lines

Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml +3 -1

@@ -15,7 +15,9 @@ maintainers:
   properties:
   compatible:
-    const: nuvoton,nct3018y
+    enum:
+      - nuvoton,nct3018y
+      - nuvoton,nct3015y

Would sorting the list be useful?

Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml maintainers: · line 23

Quoted 2 lines

Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml +0 -0

   reg:
     maxItems: 1

Kind regards,

Paul

drivers/rtc/rtc-nct3018y.c +8 -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;
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
+	if (flags < 0) {
+		dev_dbg(&client->dev, "%s: read error\n", __func__);
+		return flags;
+	} else if (flags & NCT3018Y_REG_PART_NCT3018Y) {
+		if (!(flags & data->part_number))
+			dev_warn(&client->dev, "%s: part_num=0x%x but NCT3018Y_REG_PART=0x%x\n",
+				 __func__, data->part_number, flags);

Quoted 20 lines · Mia Lin

drivers/rtc/rtc-nct3018y.c +15 -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;
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
+	if (flags < 0) {
+		dev_dbg(&client->dev, "%s: read error\n", __func__);
+		return flags;
+	} else if (flags & NCT3018Y_REG_PART_NCT3018Y) {
+		if (!(flags & data->part_number))
+			dev_warn(&client->dev, "%s: part_num=0x%x but NCT3018Y_REG_PART=0x%x\n",
+				 __func__, data->part_number, flags);
+		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;
+		}
+	} else if (flags & NCT3018Y_REG_PART_NCT3015Y) {

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

Quoted 3 lines

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

+		if (!(flags & data->part_number))
+			dev_warn(&client->dev, "%s: part_num=0x%x but NCT3018Y_REG_PART=0x%x\n",
+				 __func__, data->part_number, flags);

I don't think this is correct. Kernel's job is not to verify the DT... and why would it verify the device based on DT? You have here device detection so use it directly without this dance of comparing with compatible/match data.

Dear Alexandre and Krzysztof,

Thank you for your comments.

Best regards,

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

Quoted 16 lines · Mia Lin

- 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: read error\n", __func__);

Quoted 7 lines

+ return flags;

+ } else if (flags & NCT3018Y_REG_PART_NCT3018Y) {

+ if (!(flags & data->part_number))

+ dev_warn(&client->dev, "%s: part_num=0x%x but

NCT3018Y_REG_PART=0x%x\n",

Quoted 12 lines

+ __func__, data->part_number, flags);

+ 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;

Quoted 7 lines

+ }

+ } else if (flags & NCT3018Y_REG_PART_NCT3015Y) {

+ if (!(flags & data->part_number))

+ dev_warn(&client->dev, "%s: part_num=0x%x but

NCT3018Y_REG_PART=0x%x\n",

Quoted 1 line

+ __func__, data->part_number, flags);

I don't think this is correct. Kernel's job is not to verify the DT...

and why would it verify the device based on DT? You have here device

detection so use it directly without this dance of comparing with

compatible/match data.

I fully agree here, either you trust your DT or the device ID but do not

--

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

- Use DT compatible to check the chip matches or not.

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

---
  drivers/rtc/rtc-nct3018y.c | 88 +++++++++++++++++++++++++++++++++-----
  1 file changed, 78 insertions(+), 10 deletions(-)

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

index a4e3f924837e..edc73be3ab59 100644
@@ -7,6 +7,7 @@
  #include <linux/i2c.h>
  #include <linux/module.h>
  #include <linux/of.h>
+#include <linux/of_device.h>
  #include <linux/rtc.h>
  #include <linux/slab.h>
 

@@ -23,6 +24,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 +39,20 @@
  #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_NCT3015Y	0x01
+#define NCT3018Y_REG_PART_NCT3018Y	0x02
+
+struct rtc_data {
+	u8 part_number;
+};
+
+static const struct rtc_data nct3015y_rtc_data = {
+	.part_number = NCT3018Y_REG_PART_NCT3015Y,
+};
+
+static const struct rtc_data nct3018y_rtc_data = {
+	.part_number = NCT3018Y_REG_PART_NCT3018Y,
+};
 
  struct nct3018y {
  	struct rtc_device *rtc;

@@ -52,7 +68,7 @@ static int nct3018y_set_alarm_mode(struct i2c_client *client, bool 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");

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

+		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, "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 +253,18 @@ static int nct3018y_rtc_set_time(struct device *dev, struct rtc_time *tm)
  		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, "Unable to write NCT3018Y_REG_CTRL\n");
+			return err;
+		}
+	}
+
  	return err;
  }
 

@@ -456,6 +509,7 @@ static int nct3018y_probe(struct i2c_client *client)
  {
  	struct nct3018y *nct3018y;
  	int err, flags;
+	const struct rtc_data *data = of_device_get_match_data(&client->dev);
 
  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
  				     I2C_FUNC_SMBUS_BYTE |

@@ -479,11 +533,24 @@ 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;
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
+	if (flags < 0) {
+		dev_dbg(&client->dev, "%s: read error\n", __func__);
+		return flags;
+	} else if (flags & NCT3018Y_REG_PART_NCT3018Y) {
+		if (!(flags & data->part_number))
+			dev_warn(&client->dev, "%s: part_num=0x%x but NCT3018Y_REG_PART=0x%x\n",
+				 __func__, data->part_number, flags);
+		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;
+		}
+	} else if (flags & NCT3018Y_REG_PART_NCT3015Y) {
+		if (!(flags & data->part_number))
+			dev_warn(&client->dev, "%s: part_num=0x%x but NCT3018Y_REG_PART=0x%x\n",
+				 __func__, data->part_number, flags);
  	}
 
  	flags = 0;