Hello,
On 19/10/2022 16:39:09+0300, Ibrahim Tilki wrote:
> +static int max313xx_set_time(struct device *dev, struct rtc_time *t)
> +{
> + struct max313xx *rtc = dev_get_drvdata(dev);
> + u8 regs[7];
> + int ret;
> +
> + if (t->tm_year < 100 || t->tm_year >= 300)
> + return -EINVAL;
This is unnecessary
> +
> + regs[0] = bin2bcd(t->tm_sec);
> + regs[1] = bin2bcd(t->tm_min);
> + regs[2] = bin2bcd(t->tm_hour);
> + regs[3] = bin2bcd(t->tm_wday + 1);
> + regs[4] = bin2bcd(t->tm_mday);
> + regs[5] = bin2bcd(t->tm_mon + 1);
> +
> + if (t->tm_year >= 200) {
> + regs[5] |= FIELD_PREP(MAX313XX_MONTH_CENTURY, 1);
> + regs[6] = bin2bcd(t->tm_year - 200);
> + } else {
> + regs[6] = bin2bcd(t->tm_year - 100);
> + }
regs[6] = bin2bcd(t->tm_year % 100); would be simpler
> +static int max313xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
> +{
> + struct max313xx *rtc = dev_get_drvdata(dev);
> + struct rtc_time time;
> + unsigned int reg;
> + u8 regs[6];
> + int ret;
> +
> + regs[0] = bin2bcd(t->time.tm_sec);
> + regs[1] = bin2bcd(t->time.tm_min);
> + regs[2] = bin2bcd(t->time.tm_hour);
> + regs[3] = bin2bcd(t->time.tm_mday);
> + regs[4] = bin2bcd(t->time.tm_mon + 1);
> +
> + if (t->time.tm_year >= 200) {
> + /*
> + * Century bit is shared between time and alarm registers so
> + * make sure that new alarm and RTC time is in the same century.
> + */
> + ret = max313xx_read_time(dev, &time);
> + if (ret)
> + return ret;
> +
> + if (time.tm_year < 200)
> + return -EINVAL;
> +
This doesn't feel right and it seems you are losing a whole range of
alarm years. The correct thing to do is to check whether the alarm is in
the same 100 years range.
> + /* Convert to 24Hr */
> + hour = bcd2bin(reg[MAX313XX_REG_HOUR] & 0x1f);
> + if (hour == 12)
> + hour = 0;
I'm not sure this is worth it, you should probably instead support
reading both formats and setting only 24h
> +
> + if (FIELD_GET(MAX313XX_HRS_F_AM_PM, reg[MAX313XX_REG_HOUR]))
> + hour += 12;
> +
> + reg[MAX313XX_REG_HOUR] = bin2bcd(hour);
> + /*
> + * If minute is 59, write all registers in case hour register
> + * gets updated during read-write cycle
> + */
> + if (reg[MAX313XX_REG_MINUTE] == 0x59)
> + return regmap_bulk_write(rtc->regmap, rtc->chip->sec_reg, reg, 7);
> +
> + return regmap_write(rtc->regmap, rtc->chip->sec_reg + MAX313XX_REG_HOUR,
> + reg[MAX313XX_REG_HOUR]);
You should probably reuse .set_time here
> +/* Some devices require initialization */
> +static int max313xx_init(struct max313xx *rtc)
> +{
> + switch (rtc->id) {
> + case ID_MAX31341:
> + case ID_MAX31342:
> + return regmap_update_bits(rtc->regmap, MAX3134X_CFG_REG,
> + MAX3134X_CFG_INIT_MASK,
> + MAX3134X_CFG_INIT_VALUE);
The comment is not really useful and now I'm intrigued and want to know
what this does!
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
