Re: [PATCH 1/2] drivers: rtc: add max313xx series rtc driver

[Alexandre Belloni]

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