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

From: Mining Lin
Date: Mon Aug 14 2023 - 22:46:48 EST

Next message: Hugh Dickins: "[PATCH] x86_64: Show CR4.PSE on auxiliaries like on BSP"
Previous message: Patrol: "個人情報流出の注意喚起について"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Dear Paul,

Thank you for your comments.
The replies are as follows.

Thanks.
Best regard,
Mia

> On Aug 10, 2023, at 4:16 PM, Paul Menzel <pmenzel@xxxxxxxxxxxxx> wrote:
>
> 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.
[Mia] I will add more detail. Thank you for your comments.
>
>> - 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?
[Mia] The NCT3015Y-R and NCT3018Y-R use the same datasheet but have different topologies.
Therefore, the settings here are to set according to different models to match the behavior of each chip.
>
>> - Use DT compatible to check the chip matches or not.
>
> Could you please add the datasheet name and revision?
[Mia] Use DT compatible to check does not make sense. I will remove it.
>
>> Signed-off-by: Mia Lin <mimi05633@xxxxxxxxx>
>> ---
>> 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.
[Mia] I will factor it 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?
[Mia] I will refine it.
>
>> + 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.)
[Mia] I will refine error messages. Thank you.
>
>> + 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

Next message: Hugh Dickins: "[PATCH] x86_64: Show CR4.PSE on auxiliaries like on BSP"
Previous message: Patrol: "個人情報流出の注意喚起について"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]