[PATCH v2 0/2] Compatible with NCT3015Y-R and NCT3018Y-R

[PATCH v2 0/2] Compatible with NCT3015Y-R and NCT3018Y-R

[Mia Lin]

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 (2):
  dt-bindings: rtc: nuvoton: Add DT compatible
  rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R

 .../bindings/rtc/nuvoton,nct3018y.yaml        |  4 +-
 drivers/rtc/rtc-nct3018y.c                    | 88 ++++++++++++++++---
 2 files changed, 81 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] dt-bindings: rtc: nuvoton: Add DT compatible

[Mia Lin]

Add DT compatible "nuvoton,nct3015y" to select

Signed-off-by: Mia Lin <mimi05633@gmail.com>
---
 Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml b/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
index 4f9b5604acd9..67fc60fd395c 100644
--- a/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
+++ b/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
@@ -15,7 +15,9 @@ maintainers:
 
 properties:
   compatible:
-    const: nuvoton,nct3018y
+    enum:
+      - nuvoton,nct3018y
+      - nuvoton,nct3015y
 
   reg:
     maxItems: 1
-- 
2.17.1

[PATCH v2 2/2] rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R

[Mia Lin]

- 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 <mimi05633@gmail.com>
---
 drivers/rtc/rtc-nct3018y.c | 88 +++++++++++++++++++++++++++++++++-----
 1 file changed, 78 insertions(+), 10 deletions(-)

diff --git a/drivers/rtc/rtc-nct3018y.c b/drivers/rtc/rtc-nct3018y.c
index a4e3f924837e..edc73be3ab59 100644
--- a/drivers/rtc/rtc-nct3018y.c
+++ b/drivers/rtc/rtc-nct3018y.c
@@ -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");
@@ -109,8 +125,10 @@ static int nct3018y_get_alarm_mode(struct i2c_client *client, unsigned char *ala
 		*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;
 }
@@ -178,7 +196,30 @@ 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, restore_flags = 0;
+
+	part_num = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
+	if (part_num < 0) {
+		dev_dbg(&client->dev, "%s: read error\n", __func__);
+		return part_num;
+	}
+
+	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
+	if (flags < 0) {
+		dev_dbg(&client->dev, "%s: read error\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, "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;
@@ -530,7 +597,8 @@ static const struct i2c_device_id nct3018y_id[] = {
 MODULE_DEVICE_TABLE(i2c, nct3018y_id);
 
 static const struct of_device_id nct3018y_of_match[] = {
-	{ .compatible = "nuvoton,nct3018y" },
+	{ .compatible = "nuvoton,nct3015y", .data = &nct3015y_rtc_data },
+	{ .compatible = "nuvoton,nct3018y", .data = &nct3018y_rtc_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, nct3018y_of_match);
-- 
2.17.1

Re: [PATCH v2 1/2] dt-bindings: rtc: nuvoton: Add DT compatible

[Krzysztof Kozlowski]

On 09/08/2023 11:51, Mia Lin wrote:
> Add DT compatible "nuvoton,nct3015y" to select
> 
> Signed-off-by: Mia Lin <mimi05633@gmail.com>
> ---
>  Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml b/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
> index 4f9b5604acd9..67fc60fd395c 100644
> --- a/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
> +++ b/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
> @@ -15,7 +15,9 @@ maintainers:
>  
>  properties:
>    compatible:
> -    const: nuvoton,nct3018y
> +    enum:
> +      - nuvoton,nct3018y
> +      - nuvoton,nct3015y

Responding here, but based on your changelog and driver:
Why? Why do you need compatibles to verify the reported ID? Verifying ID
does not make sense. At all.

These are compatible devices, so you could have one more compatible
using old one as fallback. And no new entry in the driver.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R

[Krzysztof Kozlowski]

On 09/08/2023 11:51, Mia Lin wrote:
> -	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);

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.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R

[Alexandre Belloni]

On 09/08/2023 16:29:33+0200, Krzysztof Kozlowski wrote:
> On 09/08/2023 11:51, Mia Lin wrote:
> > -	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);
> 
> 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
use both.


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

Re: [PATCH v2 1/2] dt-bindings: rtc: nuvoton: Add DT compatible

[Paul Menzel]

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

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.

> Signed-off-by: Mia Lin <mimi05633@gmail.com>
> ---
>   Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml b/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
> index 4f9b5604acd9..67fc60fd395c 100644
> --- a/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
> +++ b/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
> @@ -15,7 +15,9 @@ maintainers:
>   
>   properties:
>     compatible:
> -    const: nuvoton,nct3018y
> +    enum:
> +      - nuvoton,nct3018y
> +      - nuvoton,nct3015y

Would sorting the list be useful?

>     reg:
>       maxItems: 1


Kind regards,

Paul

Re: [PATCH v2 2/2] rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R

[Paul Menzel]

Dear Mia,


Thank you for your patch.

It’d be great if you made the commit message summary/title more 
specific. Maybe:

Add support for NCT3015Y-R

Am 09.08.23 um 11:51 schrieb Mia Lin:

An introduction what the NCT3015Y-R is and listing the differences to 
NCT3018Y-R would be nice.

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

Why? This also looks unrelated to the NCT3015Y-R support. Could you 
factor it out into separate patch?

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

Could you please add the datasheet name and revision?

> Signed-off-by: Mia Lin <mimi05633@gmail.com>
> ---
>   drivers/rtc/rtc-nct3018y.c | 88 +++++++++++++++++++++++++++++++++-----
>   1 file changed, 78 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-nct3018y.c b/drivers/rtc/rtc-nct3018y.c
> index a4e3f924837e..edc73be3ab59 100644
> --- a/drivers/rtc/rtc-nct3018y.c
> +++ b/drivers/rtc/rtc-nct3018y.c
> @@ -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");
> @@ -109,8 +125,10 @@ static int nct3018y_get_alarm_mode(struct i2c_client *client, unsigned char *ala
>   		*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);
> +	}

The two hunks look like unrelated fixes. It’d be great, if you factored 
those out into a separate patch.

>   
>   	return 0;
>   }
> @@ -178,7 +196,30 @@ 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, restore_flags = 0;

Why is err now initialized to 0?

> +	part_num = i2c_smbus_read_byte_data(client, NCT3018Y_REG_PART);
> +	if (part_num < 0) {
> +		dev_dbg(&client->dev, "%s: read error\n", __func__);
> +		return part_num;
> +	}
> +
> +	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
> +	if (flags < 0) {
> +		dev_dbg(&client->dev, "%s: read error\n", __func__);

Could you make these distinct error messages, so users are able to 
pinpoint the correct location right away? (Or does `dev_dbg` already 
provide that information? Maybe the line? (Also more cases below.)

> +		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;
> @@ -530,7 +597,8 @@ static const struct i2c_device_id nct3018y_id[] = {
>   MODULE_DEVICE_TABLE(i2c, nct3018y_id);
>   
>   static const struct of_device_id nct3018y_of_match[] = {
> -	{ .compatible = "nuvoton,nct3018y" },
> +	{ .compatible = "nuvoton,nct3015y", .data = &nct3015y_rtc_data },
> +	{ .compatible = "nuvoton,nct3018y", .data = &nct3018y_rtc_data },
>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, nct3018y_of_match);


Kind regards,

Paul

Re: [PATCH v2 1/2] dt-bindings: rtc: nuvoton: Add DT compatible

[Mining Lin]

Dear Paul,

Thank you for your comments.
I  originally wanted to judge whether to match chip data by DT compatible, but it does not make sense.
Therefore, I will remove it in the v4 version.

Thanks.
Best regard,
Mia

> On Aug 10, 2023, at 3:55 PM, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> 
> 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
> 
>> 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.
> 
>> Signed-off-by: Mia Lin <mimi05633@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> diff --git a/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml b/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
>> index 4f9b5604acd9..67fc60fd395c 100644
>> --- a/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
>> +++ b/Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
>> @@ -15,7 +15,9 @@ maintainers:
>>    properties:
>>    compatible:
>> -    const: nuvoton,nct3018y
>> +    enum:
>> +      - nuvoton,nct3018y
>> +      - nuvoton,nct3015y
> 
> Would sorting the list be useful?
> 
>>    reg:
>>      maxItems: 1
> 
> 
> Kind regards,
> 
> Paul