lkml.org 
[lkml]   [2021]   [Dec]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] rtc: max31343: Add a driver for Maxim MAX31343
From
Date
Hello,

Thanks for your review.

On 01.12.2021 01:32, Alexandre Belloni wrote:
> Hello,
>
> On 16/10/2021 22:21:17+0300, Alexey Firago wrote:
>> +#define MAX31343_REG_TIMER_CFG (0x05)
>> +#define TIMER_CFG_TFS GENMASK(1, 0) /* Timer frequency */
>> +#define TIMER_CFG_TRPT BIT(2) /* Timer repeat mode */
>> +#define TIMER_CFG_TPAUSE BIT(3) /* Timer Pause */
>> +#define TIMER_CFG_TE BIT(4) /* Timer enable */
>> +
>> +/* RTC section */
>> +#define MAX31343_REG_SEC (0x06)
>> +#define SEC10_MASK GENMASK(6, 4) /* RTC seconds in multiples of 10 */
>> +#define SEC_MASK GENMASK(3, 0) /* RTC seconds value */
>
> I'm not convinced having separate masks is useful here, was that
> automatically generated?

I've just exported those definitions from the table in the datasheet
prior to creating the driver.

>> +static int max31343_rtc_set_time(struct device *dev, struct rtc_time *tm)
>> +{
>> + struct max31343_rtc_data *max31343 = dev_get_drvdata(dev);
>> + u8 date[7];
>> + int ret;
>> +
>> + dev_dbg(dev, "RTC set time %04d-%02d-%02d %02d/%02d/%02d\n",
>> + tm->tm_year + 1900, tm->tm_mon, tm->tm_mday,
>> + tm->tm_hour, tm->tm_min, tm->tm_sec);
>> +
>
> This could use %ptR

Will change it (or probably completely remove it) after hwmon review.

>> + date[0] = bin2bcd(tm->tm_sec);
>> + date[1] = bin2bcd(tm->tm_min);
>> + date[2] = bin2bcd(tm->tm_hour);
>> + date[3] = tm->tm_wday;
>> + date[4] = bin2bcd(tm->tm_mday);
>> + date[5] = bin2bcd(tm->tm_mon + 1);
>> +
>> + if (tm->tm_year >= 200)
>> + date[5] |= CENTURY;
>> + date[6] = bin2bcd(tm->tm_year % 100);
>> +
>> + ret = regmap_bulk_write(max31343->regmap, MAX31343_REG_SEC, date,
>> + sizeof(date));
>> + return ret;
>> +}
>> +
>
> [...]
>
>> +static int
>> +max31343_probe(struct i2c_client *client, const struct i2c_device_id *id)
>> +{
>> + struct max31343_rtc_data *max31343 = NULL;
>> + int ret, status;
>> + struct nvmem_config nvmem_cfg = {
>> + .name = "max31343_nvram",
>> + .word_size = 1,
>> + .stride = 1,
>> + .size = MAX31343_RAM_SIZE,
>> + .type = NVMEM_TYPE_BATTERY_BACKED,
>> + .reg_read = max31343_nvram_read,
>> + .reg_write = max31343_nvram_write,
>> + };
>> +
>> + max31343 = devm_kzalloc(&client->dev, sizeof(struct max31343_rtc_data),
>> + GFP_KERNEL);
>> + if (!max31343)
>> + return -ENOMEM;
>> +
>> + max31343->regmap = devm_regmap_init_i2c(client, &max31343_regmap_config);
>> + if (IS_ERR(max31343->regmap))
>> + return PTR_ERR(max31343->regmap);
>> +
>> + i2c_set_clientdata(client, max31343);
>> +
>> + ret = regmap_read(max31343->regmap, MAX31343_REG_STATUS, &status);
>> + if (ret < 0)
>> + return ret;
>> +
>> + max31343->rtc = devm_rtc_allocate_device(&client->dev);
>> + if (IS_ERR(max31343->rtc))
>> + return PTR_ERR(max31343->rtc);
>> +
>> + max31343->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
>> + max31343->rtc->range_max = RTC_TIMESTAMP_END_2199;
>
> For my information, did you check the time continuity in this interval?

Yes, I've checked setting of out-of-range values, checked edge values
and checked 2099 -> 2100 -> 2101 transitions.

>
>> + max31343->rtc->ops = &max31343_rtc_ops;
>> + ret = devm_rtc_register_device(max31343->rtc);
>> + if (ret)
>> + return ret;
>> +
>> + nvmem_cfg.priv = max31343->regmap;
>> + devm_rtc_nvmem_register(max31343->rtc, &nvmem_cfg);
>> + max31343_hwmon_register(&client->dev);
>
> The whole driver seems ok, I'd like to get a review from the hwmon
> maintainers on the hwmon part as it is quite large.

Ok.

Thanks,
Alexey

\
 
 \ /
  Last update: 2021-12-04 18:34    [W:0.049 / U:0.468 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site