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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux