| Message ID | 20240925004643.1298510-1-potin.lai.pt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [linux,dev-6.6,1/1] rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R | expand |
On Wed, 2024-09-25 at 08:46 +0800, Potin Lai wrote: > From: Mia Lin <mimi05633@gmail.com> > > 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 <mimi05633@gmail.com> > --- > drivers/rtc/rtc-nct3018y.c | 52 +++++++++++++++++++++++++++++++++----- > 1 file changed, 46 insertions(+), 6 deletions(-) So I looked at the history of this driver upstream, and it appears that this is (approximately) a backport of an existing change: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=14688f1a91e1f37bc6bf50ff5241e857f24338e0 In the future, can you please provide such a link in the patch notes (i.e. here, below the `---` above but before the diff markers below). I compared what you've sent here and the patch above: ``` 0 andrew@heihei:~/src/kernel.org/linux/openbmc ((c58d8005433d...)) $ git diff 14688f1a91e1f37bc6bf50ff5241e857f24338e0 HEAD -- drivers/rtc/rtc-nct3018y.c diff --git a/drivers/rtc/rtc-nct3018y.c b/drivers/rtc/rtc-nct3018y.c index f488a189a465..076d8b99f913 100644 --- a/drivers/rtc/rtc-nct3018y.c +++ b/drivers/rtc/rtc-nct3018y.c @@ -102,6 +102,8 @@ static int nct3018y_get_alarm_mode(struct i2c_client *client, unsigned char *ala if (flags < 0) return flags; *alarm_enable = flags & NCT3018Y_BIT_AIE; + dev_dbg(&client->dev, "%s:alarm_enable:%x\n", __func__, *alarm_enable); + } if (alarm_flag) { @@ -110,11 +112,9 @@ static int nct3018y_get_alarm_mode(struct i2c_client *client, unsigned char *ala if (flags < 0) return flags; *alarm_flag = flags & NCT3018Y_BIT_AF; + dev_dbg(&client->dev, "%s:alarm_flag:%x\n", __func__, *alarm_flag); } - dev_dbg(&client->dev, "%s:alarm_enable:%x alarm_flag:%x\n", - __func__, *alarm_enable, *alarm_flag); - return 0; } ``` Given the hunks are fairly benign I've instead directly backported the upstream change. If you have any issues with this, please let me know. Andrew
On Wed, Sep 25, 2024 at 9:50 AM Andrew Jeffery <andrew@codeconstruct.com.au> wrote: > > On Wed, 2024-09-25 at 08:46 +0800, Potin Lai wrote: > > From: Mia Lin <mimi05633@gmail.com> > > > > 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 <mimi05633@gmail.com> > > --- > > drivers/rtc/rtc-nct3018y.c | 52 +++++++++++++++++++++++++++++++++----- > > 1 file changed, 46 insertions(+), 6 deletions(-) > > So I looked at the history of this driver upstream, and it appears that > this is (approximately) a backport of an existing change: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=14688f1a91e1f37bc6bf50ff5241e857f24338e0 > > In the future, can you please provide such a link in the patch notes > (i.e. here, below the `---` above but before the diff markers below). > Yes, this is a backport patch. Thank you for the notice, I will include the link in future. > I compared what you've sent here and the patch above: > > ``` > 0 andrew@heihei:~/src/kernel.org/linux/openbmc ((c58d8005433d...)) $ git diff 14688f1a91e1f37bc6bf50ff5241e857f24338e0 HEAD -- drivers/rtc/rtc-nct3018y.c > diff --git a/drivers/rtc/rtc-nct3018y.c b/drivers/rtc/rtc-nct3018y.c > index f488a189a465..076d8b99f913 100644 > --- a/drivers/rtc/rtc-nct3018y.c > +++ b/drivers/rtc/rtc-nct3018y.c > @@ -102,6 +102,8 @@ static int nct3018y_get_alarm_mode(struct i2c_client *client, unsigned char *ala > if (flags < 0) > return flags; > *alarm_enable = flags & NCT3018Y_BIT_AIE; > + dev_dbg(&client->dev, "%s:alarm_enable:%x\n", __func__, *alarm_enable); > + > } > > if (alarm_flag) { > @@ -110,11 +112,9 @@ static int nct3018y_get_alarm_mode(struct i2c_client *client, unsigned char *ala > if (flags < 0) > return flags; > *alarm_flag = flags & NCT3018Y_BIT_AF; > + dev_dbg(&client->dev, "%s:alarm_flag:%x\n", __func__, *alarm_flag); > } > > - dev_dbg(&client->dev, "%s:alarm_enable:%x alarm_flag:%x\n", > - __func__, *alarm_enable, *alarm_flag); > - > return 0; > } > ``` > > Given the hunks are fairly benign I've instead directly backported the > upstream change. > > If you have any issues with this, please let me know. No issue from me, thank you for the quick response and support. Potin > > Andrew
diff --git a/drivers/rtc/rtc-nct3018y.c b/drivers/rtc/rtc-nct3018y.c index c4533c0f53896..076d8b99f9131 100644 --- a/drivers/rtc/rtc-nct3018y.c +++ b/drivers/rtc/rtc-nct3018y.c @@ -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;