Re: [PATCH v3 4/8] rtc: ac100: Add RTC driver for X-Powers AC100

[Alexandre Belloni <alexandre.belloni@free-electrons.com>]

Hi,

A few comments, mostly about style.

On 20/06/2016 at 10:52:14 +0800, Chen-Yu Tsai wrote :
> +struct ac100_rtc_dev {
> +	struct rtc_device *rtc;
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct mutex mutex;

I don't think that mutex is needed. Instead, you can take rtc->ops_lock
from the interrupt handler.

> +	int irq;
> +	unsigned long alarm;
> +};
> +
> +static int ac100_rtc_get_time(struct device *dev, struct rtc_time *rtc_tm)
> +{
> +	struct ac100_rtc_dev *chip = dev_get_drvdata(dev);
> +	struct regmap *regmap = chip->regmap;
> +	u16 reg[7];
> +	int ret;
> +
> +	ret = regmap_bulk_read(regmap, AC100_RTC_SEC, reg, 7);
> +	if (ret)
> +		return ret;
> +
> +	rtc_tm->tm_sec  = bcd2bin(reg[0] & AC100_RTC_SEC_MASK);
> +	rtc_tm->tm_min  = bcd2bin(reg[1] & AC100_RTC_MIN_MASK);
> +	rtc_tm->tm_hour = bcd2bin(reg[2] & AC100_RTC_HOU_MASK);
> +	rtc_tm->tm_wday = bcd2bin(reg[3] & AC100_RTC_WEE_MASK);
> +	rtc_tm->tm_mday = bcd2bin(reg[4] & AC100_RTC_DAY_MASK);
> +	rtc_tm->tm_mon  = bcd2bin(reg[5] & AC100_RTC_MON_MASK);
> +	rtc_tm->tm_year = bcd2bin(reg[6] & AC100_RTC_YEA_MASK);
> +
> +	rtc_tm->tm_mon  -= 1;

I would put the - 1 inline with the first tm_mon assignment.

> +
> +	/*
> +	 * switch from (data_year->min)-relative offset to
> +	 * a (1900)-relative one
> +	 */
> +	rtc_tm->tm_year += AC100_YEAR_OFF;

Unless you feel the comment is absolutely necessary, I'd also put that
with the first tm_year assignment.

BTW, is that RTC aware that 0 is actually 1970? And so 2000 (30) is not
a leap year but 2016 is? I'd say that this is not the case, seeing
AC100_RTC_YEA_LEAP but it is worth checking.

Is this bit (AC100_RTC_YEA_LEAP) updated by the rtc? IF that is not the
case, the time will have to be set at least once between first of
January and 28th of February of leap years else it will fail...

> +
> +	return rtc_valid_tm(rtc_tm);
> +}
> +
> +static int ac100_rtc_set_time(struct device *dev, struct rtc_time *rtc_tm)
> +{
> +	struct ac100_rtc_dev *chip = dev_get_drvdata(dev);
> +	struct regmap *regmap = chip->regmap;
> +	int year;
> +	u16 reg[8];
> +
> +	/* our RTC has a limited year range... */
> +	year = rtc_tm->tm_year + 1900;
> +	if (year < AC100_YEAR_MIN || year > AC100_YEAR_MAX) {
> +		dev_err(dev, "rtc only supports year in range %d - %d\n",
> +			AC100_YEAR_MIN, AC100_YEAR_MAX);
> +		return -EINVAL;
> +	}
> +

What about:

year = rtc_tm->tm_year - AC100_YEAR_OFF
if (year < 0 || year > (AC100_YEAR_MAX - 1900))

It allows to reuse year for the reg[6] assignment. IT is a simple
suggestion, maybe the compiler is smarter than I am ;).

> +	/* correct offsets */
> +	rtc_tm->tm_year -= AC100_YEAR_OFF;
> +	rtc_tm->tm_mon += 1;
> +

Those could also got inline with their respective reg[] assignment


> +	/* convert to BCD */
> +	reg[0] = bin2bcd(rtc_tm->tm_sec)  & AC100_RTC_SEC_MASK;
> +	reg[1] = bin2bcd(rtc_tm->tm_min)  & AC100_RTC_MIN_MASK;
> +	reg[2] = bin2bcd(rtc_tm->tm_hour) & AC100_RTC_HOU_MASK;
> +	reg[3] = bin2bcd(rtc_tm->tm_wday) & AC100_RTC_WEE_MASK;
> +	reg[4] = bin2bcd(rtc_tm->tm_mday) & AC100_RTC_DAY_MASK;
> +	reg[5] = bin2bcd(rtc_tm->tm_mon)  & AC100_RTC_MON_MASK;
> +	reg[6] = bin2bcd(rtc_tm->tm_year) & AC100_RTC_YEA_MASK;
> +	/* trigger write */
> +	reg[7] = AC100_RTC_UPD_TRIGGER;
> +
> +	/* Is it a leap year? */
> +	if (is_leap_year(year))
> +		reg[6] |= AC100_RTC_YEA_LEAP;
> +
> +	return regmap_bulk_write(regmap, AC100_RTC_SEC, reg, 8);
> +}
> +
> +

[...]

> +	alrm_tm->tm_sec  = bcd2bin(reg[0] & AC100_ALM_SEC_MASK);
> +	alrm_tm->tm_min  = bcd2bin(reg[1] & AC100_ALM_MIN_MASK);
> +	alrm_tm->tm_hour = bcd2bin(reg[2] & AC100_ALM_HOU_MASK);
> +	alrm_tm->tm_wday = bcd2bin(reg[3] & AC100_ALM_WEE_MASK);
> +	alrm_tm->tm_mday = bcd2bin(reg[4] & AC100_ALM_DAY_MASK);
> +	alrm_tm->tm_mon  = bcd2bin(reg[5] & AC100_ALM_MON_MASK);
> +	alrm_tm->tm_year = bcd2bin(reg[6] & AC100_ALM_YEA_MASK);
> +
> +	alrm_tm->tm_mon  -= 1;
> +
> +	/*
> +	 * switch from (data_year->min)-relative offset to
> +	 * a (1900)-relative one
> +	 */
> +	alrm_tm->tm_year += AC100_YEAR_OFF;
> +

Well, same comments as in ac100_rtc_get_time()


> +out:
> +	mutex_unlock(&chip->mutex);
> +	return ret;
> +}
> +
> +static int ac100_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct ac100_rtc_dev *chip = dev_get_drvdata(dev);
> +	struct regmap *regmap = chip->regmap;
> +	struct rtc_time *alrm_tm = &alrm->time;
> +	u16 reg[8];
> +	int year;
> +	int ret;
> +
> +	/* our alarm has a limited year range... */
> +	year = alrm_tm->tm_year + 1900;
> +	if (year < AC100_YEAR_MIN || year > AC100_YEAR_MAX) {
> +		dev_err(dev, "alarm only supports year in range %d - %d\n",
> +			AC100_YEAR_MIN, AC100_YEAR_MAX);
> +		return -EINVAL;
> +	}
> +
> +	/* correct offsets */
> +	alrm_tm->tm_year -= AC100_YEAR_OFF;
> +	alrm_tm->tm_mon += 1;
> +

Same comment as ac100_rtc_set_time()

> +	/* convert to BCD */
> +	reg[0] = (bin2bcd(alrm_tm->tm_sec)  & AC100_ALM_SEC_MASK) |
> +			AC100_ALM_ENABLE_FLAG;
> +	reg[1] = (bin2bcd(alrm_tm->tm_min)  & AC100_ALM_MIN_MASK) |
> +			AC100_ALM_ENABLE_FLAG;
> +	reg[2] = (bin2bcd(alrm_tm->tm_hour) & AC100_ALM_HOU_MASK) |
> +			AC100_ALM_ENABLE_FLAG;
> +	/* Do not enable weekday alarm */
> +	reg[3] = bin2bcd(alrm_tm->tm_wday) & AC100_ALM_WEE_MASK;
> +	reg[4] = (bin2bcd(alrm_tm->tm_mday) & AC100_ALM_DAY_MASK) |
> +			AC100_ALM_ENABLE_FLAG;
> +	reg[5] = (bin2bcd(alrm_tm->tm_mon)  & AC100_ALM_MON_MASK) |
> +			AC100_ALM_ENABLE_FLAG;
> +	reg[6] = (bin2bcd(alrm_tm->tm_year) & AC100_ALM_YEA_MASK) |
> +			AC100_ALM_ENABLE_FLAG;
> +	/* trigger write */
> +	reg[7] = AC100_ALM_UPD_TRIGGER;
> +
> +	mutex_lock(&chip->mutex);
> +
> +	ret = regmap_bulk_write(regmap, AC100_ALM_SEC, reg, 8);
> +	if (ret)
> +		goto out;
> +
> +	ret = _ac100_rtc_alarm_irq_enable(chip, alrm->enabled);
> +
> +out:
> +	mutex_unlock(&chip->mutex);
> +	return ret;
> +}
> +

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com