* Re: [PATCH 1/2] rtc: max31343: Add a driver for Maxim MAX31343 [not found] ` <20211016192118.255624-2-alexey_firago@mentor.com> @ 2021-11-30 22:32 ` Alexandre Belloni 2021-12-04 17:25 ` afirago 0 siblings, 1 reply; 2+ messages in thread From: Alexandre Belloni @ 2021-11-30 22:32 UTC (permalink / raw) To: Alexey Firago, Guenter Roeck Cc: a.zummo, robh+dt, linux-rtc, devicetree, linux-kernel, linux-hwmon 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? > +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 > + 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? > + 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. -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH 1/2] rtc: max31343: Add a driver for Maxim MAX31343 2021-11-30 22:32 ` [PATCH 1/2] rtc: max31343: Add a driver for Maxim MAX31343 Alexandre Belloni @ 2021-12-04 17:25 ` afirago 0 siblings, 0 replies; 2+ messages in thread From: afirago @ 2021-12-04 17:25 UTC (permalink / raw) To: Alexandre Belloni, Guenter Roeck Cc: a.zummo, robh+dt, linux-rtc, devicetree, linux-kernel, linux-hwmon 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 ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-12-04 17:33 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20211016192118.255624-1-alexey_firago@mentor.com>
[not found] ` <20211016192118.255624-2-alexey_firago@mentor.com>
2021-11-30 22:32 ` [PATCH 1/2] rtc: max31343: Add a driver for Maxim MAX31343 Alexandre Belloni
2021-12-04 17:25 ` afirago
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox