* [PATCH v5 0/1] Compatible with NCT3015Y-R and NCT3018Y-R @ 2023-09-05 6:03 Mia Lin 2023-09-05 6:03 ` [PATCH v5 1/1] rtc: nuvoton: " Mia Lin 0 siblings, 1 reply; 5+ messages in thread From: Mia Lin @ 2023-09-05 6:03 UTC (permalink / raw) To: avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, a.zummo, alexandre.belloni, KWLIU, JJLIU0, KFLIN, mylin1 Cc: openbmc, linux-rtc, linux-kernel, Mia Lin 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 | 87 ++++++++++++++++++++++++++++---------- 1 file changed, 64 insertions(+), 23 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v5 1/1] rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R 2023-09-05 6:03 [PATCH v5 0/1] Compatible with NCT3015Y-R and NCT3018Y-R Mia Lin @ 2023-09-05 6:03 ` Mia Lin 2023-09-05 6:35 ` Alexandre Belloni 0 siblings, 1 reply; 5+ messages in thread From: Mia Lin @ 2023-09-05 6:03 UTC (permalink / raw) To: avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, a.zummo, alexandre.belloni, KWLIU, JJLIU0, KFLIN, mylin1 Cc: openbmc, linux-rtc, linux-kernel, 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. - Refine error messages to pinpoint the correct location. Signed-off-by: Mia Lin <mimi05633@gmail.com> --- drivers/rtc/rtc-nct3018y.c | 87 ++++++++++++++++++++++++++++---------- 1 file changed, 64 insertions(+), 23 deletions(-) diff --git a/drivers/rtc/rtc-nct3018y.c b/drivers/rtc/rtc-nct3018y.c index a4e3f924837e..9ec20f241e15 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,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; @@ -46,6 +48,8 @@ struct nct3018y { #endif }; +static int part_num; + static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on) { int err, flags; @@ -55,7 +59,7 @@ static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on) 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 +71,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); 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; } @@ -155,7 +159,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 +182,44 @@ 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, flags; + int restore_flags = 0; + + 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 +230,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,7 +258,7 @@ 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; } @@ -257,19 +291,19 @@ static int nct3018y_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm) 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, "%s: Unable to write minutes alarm reg.\n", __func__); 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; } @@ -473,23 +507,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; + 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; + } else if (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, "%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 +547,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 { -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/1] rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R 2023-09-05 6:03 ` [PATCH v5 1/1] rtc: nuvoton: " Mia Lin @ 2023-09-05 6:35 ` Alexandre Belloni 2023-09-06 1:19 ` Mining Lin 0 siblings, 1 reply; 5+ messages in thread From: Alexandre Belloni @ 2023-09-05 6:35 UTC (permalink / raw) To: Mia Lin Cc: avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, a.zummo, KWLIU, JJLIU0, KFLIN, mylin1, openbmc, linux-rtc, linux-kernel On 05/09/2023 14:03:41+0800, Mia Lin wrote: > 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 <mimi05633@gmail.com> > --- > drivers/rtc/rtc-nct3018y.c | 87 ++++++++++++++++++++++++++++---------- > 1 file changed, 64 insertions(+), 23 deletions(-) > > diff --git a/drivers/rtc/rtc-nct3018y.c b/drivers/rtc/rtc-nct3018y.c > index a4e3f924837e..9ec20f241e15 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,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; > @@ -46,6 +48,8 @@ struct nct3018y { > #endif > }; > > +static int part_num; > + This must be part of struct nct3018y. > static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on) > { > int err, flags; > @@ -55,7 +59,7 @@ static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on) > 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__); If you really insist on this change, what about: #define pr_fmt(fmt) "%s: " fmt, __func__ > return flags; > } > > @@ -67,21 +71,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); > 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; > } > > @@ -155,7 +159,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 +182,44 @@ 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, flags; > + int restore_flags = 0; > + > + 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 +230,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,7 +258,7 @@ 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; > } > > @@ -257,19 +291,19 @@ static int nct3018y_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm) > > 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, "%s: Unable to write minutes alarm reg.\n", __func__); > 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; > } > > @@ -473,23 +507,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; > + 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; > + } else if (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, "%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 +547,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 { > -- > 2.17.1 > -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/1] rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R 2023-09-05 6:35 ` Alexandre Belloni @ 2023-09-06 1:19 ` Mining Lin 2023-09-06 14:01 ` Alexandre Belloni 0 siblings, 1 reply; 5+ messages in thread From: Mining Lin @ 2023-09-06 1:19 UTC (permalink / raw) To: Alexandre Belloni Cc: avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, a.zummo, KWLIU, JJLIU0, KFLIN, mylin1, openbmc, linux-rtc, linux-kernel Dear Alexandre, Thanks for your comments. The replies are as follow. Thanks. Best regard, Mia > > On Sep 5, 2023, at 2:35 PM, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 05/09/2023 14:03:41+0800, Mia Lin wrote: >> 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 <mimi05633@gmail.com> >> --- >> drivers/rtc/rtc-nct3018y.c | 87 ++++++++++++++++++++++++++++---------- >> 1 file changed, 64 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/rtc/rtc-nct3018y.c b/drivers/rtc/rtc-nct3018y.c >> index a4e3f924837e..9ec20f241e15 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,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; >> @@ -46,6 +48,8 @@ struct nct3018y { >> #endif >> }; >> >> +static int part_num; >> + > > This must be part of struct nct3018y. [Mia] I will move the part number parameter to struct nct3018y. > >> static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on) >> { >> int err, flags; >> @@ -55,7 +59,7 @@ static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on) >> 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__); > > If you really insist on this change, what about: > > #define pr_fmt(fmt) "%s: " fmt, __func__ [Mia] Do you mean to replace dev_dbg with pr_debug? If yes, for consistency, I'm going to refine all messages via pr_debug. Thank you for your suggestion. >> return flags; >> } >> >> @@ -67,21 +71,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); >> 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; >> } >> >> @@ -155,7 +159,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 +182,44 @@ 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, flags; >> + int restore_flags = 0; >> + >> + 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 +230,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,7 +258,7 @@ 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; >> } >> >> @@ -257,19 +291,19 @@ static int nct3018y_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm) >> >> 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, "%s: Unable to write minutes alarm reg.\n", __func__); >> 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; >> } >> >> @@ -473,23 +507,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; >> + 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; >> + } else if (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, "%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 +547,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 { >> -- >> 2.17.1 >> > > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/1] rtc: nuvoton: Compatible with NCT3015Y-R and NCT3018Y-R 2023-09-06 1:19 ` Mining Lin @ 2023-09-06 14:01 ` Alexandre Belloni 0 siblings, 0 replies; 5+ messages in thread From: Alexandre Belloni @ 2023-09-06 14:01 UTC (permalink / raw) To: Mining Lin Cc: avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, a.zummo, KWLIU, JJLIU0, KFLIN, mylin1, openbmc, linux-rtc, linux-kernel On 06/09/2023 09:19:29+0800, Mining Lin wrote: > >> static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on) > >> { > >> int err, flags; > >> @@ -55,7 +59,7 @@ static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on) > >> 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__); > > > > If you really insist on this change, what about: > > > > #define pr_fmt(fmt) "%s: " fmt, __func__ > [Mia] Do you mean to replace dev_dbg with pr_debug? If yes, for consistency, I'm going to refine all messages via pr_debug. Thank you for your suggestion. No, I mean that instead of adding __func__ to all the messages just define pr_fmt, this should achieve what you want. -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-09-06 14:01 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-05 6:03 [PATCH v5 0/1] Compatible with NCT3015Y-R and NCT3018Y-R Mia Lin 2023-09-05 6:03 ` [PATCH v5 1/1] rtc: nuvoton: " Mia Lin 2023-09-05 6:35 ` Alexandre Belloni 2023-09-06 1:19 ` Mining Lin 2023-09-06 14:01 ` Alexandre Belloni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).