Hi! On Tue, Dec 01, 2015 at 03:42:18PM +0100, Alexandre Belloni wrote: > Hi, > > Thanks for that patch, I'm sorry I didn't find the time to reply to your > previous version. > No problem. > On 01/12/2015 at 14:48:20 +0100, Steffen Trumtrar wrote : > > diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c > > new file mode 100644 > > index 000000000000..7a828adf9794 > > --- /dev/null > > +++ b/drivers/rtc/rtc-rx6110.c > > @@ -0,0 +1,480 @@ > > +/* > > + * Driver for the Epson RTC module RX-6110 SA > > + * > > + * Copyright(C) 2015 Pengutronix, Steffen Trumtrar <kernel@xxxxxxxxxxxxxx> > > + * Copyright(C) SEIKO EPSON CORPORATION 2013. All rights reserved. > > + * > > + * Derived from RX-8025 driver: > > + * Copyright (C) 2009 Wolfgang Grandegger <wg@xxxxxxxxxxxxxx> > > + * > > + * Copyright (C) 2005 by Digi International Inc. > > + * All rights reserved. > > + * > > + * Modified by fengjh at rising.com.cn > > + * <http://lists.lm-sensors.org/mailman/listinfo/lm-sensors> > > + * 2006.11 > > + * > > + * Code cleanup by Sergei Poselenov, <sposelenov@xxxxxxxxxxx> > > + * Converted to new style by Wolfgang Grandegger <wg@xxxxxxxxxxxxxx> > > + * Alarm and periodic interrupt added by Dmitry Rakhchev <rda@xxxxxxxxxxx> > > + * > > + * > > Please remove all those unnecessary copyrights and credits. The original > rx-8025 has been heavily rewritten anyway. Also, all the epson drivers > suffer from a lot of issues (some of those you already fixed) because > they are based on an old version of the driver. > Okay. I wasn't sure how okay it is to just get rid of it. I removed everything after the SEIKO EPSON copyright. > > +/* Extension Register (1Dh) bit positions */ > > +#define RX6110_BIT_EXT_TSEL (7 << 0) > > +#define RX6110_BIT_EXT_WADA (1 << 3) > > +#define RX6110_BIT_EXT_TE (1 << 4) > > +#define RX6110_BIT_EXT_USEL (1 << 5) > > +#define RX6110_BIT_EXT_FSEL (3 << 6) > > + > > +/* Flag Register (1Eh) bit positions */ > > +#define RX6110_BIT_FLAG_VLF (1 << 1) > > +#define RX6110_BIT_FLAG_AF (1 << 3) > > +#define RX6110_BIT_FLAG_TF (1 << 4) > > +#define RX6110_BIT_FLAG_UF (1 << 5) > > + > > +/* Control Register (1Fh) bit positions */ > > +#define RX6110_BIT_CTRL_TSTP (1 << 2) > > +#define RX6110_BIT_CTRL_AIE (1 << 3) > > +#define RX6110_BIT_CTRL_TIE (1 << 4) > > +#define RX6110_BIT_CTRL_UIE (1 << 5) > > +#define RX6110_BIT_CTRL_STOP (1 << 6) > > +#define RX6110_BIT_CTRL_TEST (1 << 7) > > + > > Can you use the BIT() macro? > > > +static struct spi_driver rx6110_driver; > > + > > +struct rx6110_data { > > + struct rtc_device *rtc; > > + struct regmap *regmap; > > + int ctrlreg; > > ctrlreg is cached but never used. > > > +}; > > + > > +/** > > + * rx6110_get_week_day - translate reg_week_day to tm_wday > > + * @reg_week_day: weekday register value > > + * > > + * Return: an integer representing the tm_wday > > + */ > > +static int rx6110_get_week_day(u8 reg_week_day) > > +{ > > + int tm_wday = -1; > > + int i; > > + > > + for (i = 0; i < 7; i++) { > > + if (reg_week_day & 1) { > > + tm_wday = i; > > + break; > > + } > > + reg_week_day >>= 1; > > + } > > + > > + return tm_wday; > > +} > > ffs() is probably better. > > > + > > +/** > > + * rx6110_set_time - set the current time in the rx6110 registers > > + * > > + * @dev: the rtc device in use > > + * @tm: holds date and time > > + * > > + * BUG: The HW assumes every year that is a multiple of 4 to be a leap > > + * year. Next time this is wrong is 2100, which will not be a leap year > > + * > > + * Note: If STOP is not set/cleared, the clock will start when the seconds > > + * register is written > > + * > > + */ > > +static int rx6110_set_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct rx6110_data *rx6110 = dev_get_drvdata(dev); > > + int ret; > > + > > + if (tm->tm_year > 137) > > + return -EINVAL; > > + > > Seeing the comment comment above, this should probably be if > (tm->tm_year < 100 || tm->tm_year >= 200) > I don't think this particular part has any issue > handling 2038. However, on 32bit platform, your userspace is probably > not ready to handle those date. hwclock should return the correct date. > > > + /* set STOP bit before changing clock/calendar */ > > + ret = regmap_update_bits(rx6110->regmap, RX6110_REG_CTRL, > > + RX6110_BIT_CTRL_STOP, RX6110_BIT_CTRL_STOP); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(rx6110->regmap, RX6110_REG_SEC, > > + bin2bcd(tm->tm_sec)); > > + if (ret) > > + return ret; > > + ret = regmap_write(rx6110->regmap, RX6110_REG_MIN, > > + bin2bcd(tm->tm_min)); > > + if (ret) > > + return ret; > > + ret = regmap_write(rx6110->regmap, RX6110_REG_HOUR, > > + bin2bcd(tm->tm_hour)); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(rx6110->regmap, RX6110_REG_MDAY, > > + bin2bcd(tm->tm_mday)); > > + if (ret) > > + return ret; > > + ret = regmap_write(rx6110->regmap, RX6110_REG_MONTH, > > + bin2bcd(tm->tm_mon + 1)); > > + if (ret) > > + return ret; > > + ret = regmap_write(rx6110->regmap, RX6110_REG_YEAR, > > + bin2bcd(tm->tm_year % 100)); > > + if (ret) > > + return ret; > > + ret = regmap_write(rx6110->regmap, RX6110_REG_WDAY, > > + 1 << bin2bcd(tm->tm_wday)); > > + if (ret) > > + return ret; > > + > > I feel that using a bulk write between setting and clearing the STOP bit > would be more efficient, in particular if one day we want to support > i2c. > > > + /* clear STOP bit after changing clock/calendar */ > > + ret = regmap_update_bits(rx6110->regmap, RX6110_REG_CTRL, > > + RX6110_BIT_CTRL_STOP, 0); > > + > > + return ret; > > +} > > +/** > > + * rx6110_reset_time - reset the time to 1970/1/1 00:00 > > + * @dev: the rtc device in use > > + * @tm: holds date and time > > + */ > > +static int rx6110_reset_time(struct device *dev, struct rtc_time *tm) > > +{ > > + int ret; > > + > > + tm->tm_sec = 0; > > + tm->tm_min = 0; > > + tm->tm_hour = 0; > > + tm->tm_wday = 0; > > + tm->tm_mday = 1; > > + tm->tm_mon = 0; > > + tm->tm_year = 70; > > + ret = rx6110_set_time(dev, tm); > > + if (ret) { > > + dev_err(dev, "Failed to set time.\n"); > > + return ret; > > + } > > + > > + return 0; > > +} > > This reset function is unnecessary. > > > > + > > +/** > > + * rx6110_get_time - get the current time from the rx6110 registers > > + * @dev: the rtc device in use > > + * @tm: holds date and time > > + */ > > +static int rx6110_get_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct rx6110_data *rx6110 = dev_get_drvdata(dev); > > + unsigned char date[7]; > > + int flags; > > + int ret; > > + > > + ret = regmap_read(rx6110->regmap, RX6110_REG_FLAG, &flags); > > + if (ret) > > + return -EINVAL; > > + > > + /* check for VLF Flag (set at power-on) */ > > + if ((flags & RX6110_BIT_FLAG_VLF)) > > + return -EINVAL; > > + > > + /* read registers to date */ > > + ret = regmap_bulk_read(rx6110->regmap, RX6110_REG_SEC, date, 7); > > + if (ret) > > + return ret; > > + > > + tm->tm_sec = bcd2bin(date[0] & 0x7f); > > + tm->tm_min = bcd2bin(date[1] & 0x7f); > > + /* only 24-hour clock */ > > + tm->tm_hour = bcd2bin(date[2] & 0x3f); > > + tm->tm_wday = rx6110_get_week_day(date[3] & 0x7f); > > + tm->tm_mday = bcd2bin(date[4] & 0x3f); > > + tm->tm_mon = bcd2bin(date[5] & 0x1f) - 1; > > + tm->tm_year = bcd2bin(date[6]); > > + > > + if (tm->tm_year < 70) > > + tm->tm_year += 100; > > + > > I think you are better off not playing with the date and only support > dates between 2000 and 2100. I don't really think anybody really care > about dates more than 15 years in the past. > > > + if (tm->tm_year > 137) { > > + ret = rx6110_reset_time(dev, tm); > > + if (ret) > > + return ret; > > + } > > Please, never reset the date/time. This will confuse userspace. When > silently resetting the time, userspace as no way of knowing whether an error > really happened or you used a time machine ;). Return an error instead. > > I think the check should be if (tm->tm_year > 200). > > > + > > + dev_dbg(dev, "%s: date %ds %dm %dh %dmd %dm %dy\n", __func__, > > + tm->tm_sec, tm->tm_min, tm->tm_hour, > > + tm->tm_mday, tm->tm_mon, tm->tm_year); > > + > > + return rtc_valid_tm(tm); > > +} > > + > > +/** > > + * rx6110_init - initialize the rx6110 registers > > + * > > + * @rx6110: pointer to the rx6110 struct in use > > + * > > + */ > > +static int rx6110_init(struct rx6110_data *rx6110) > > +{ > > + struct rtc_device *rtc = rx6110->rtc; > > + int need_clear = 0; > > + int need_reset = 0; > > + int ext; > > + int flags; > > + int ctrl; > > + int ret; > > + > > + /* set reserved register 0x17 with specified value of 0xB8 */ > > + ret = regmap_write(rx6110->regmap, RX6110_REG_RES1, 0xB8); > > + if (ret) > > + return ret; > > + > > + /* set reserved register 0x30 with specified value of 0x00 */ > > + ret = regmap_write(rx6110->regmap, RX6110_REG_RES1, 0x00); > > + if (ret) > > + return ret; > > + > > + /* set reserved register 0x31 with specified value of 0x10 */ > > + ret = regmap_write(rx6110->regmap, RX6110_REG_RES1, 0x10); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(rx6110->regmap, RX6110_REG_IRQ, 0x0); > > + if (ret) > > + return ret; > > + > > + ret = regmap_update_bits(rx6110->regmap, RX6110_REG_EXT, > > + RX6110_BIT_EXT_TE, 0); > > + if (ret) > > + return ret; > > + > > + /* get current extension, flag, control register values */ > > + ret = regmap_read(rx6110->regmap, RX6110_REG_EXT, &ext); > > + if (ret) > > + return ret; > > + > > + ret = regmap_read(rx6110->regmap, RX6110_REG_FLAG, &flags); > > + if (ret) > > + return ret; > > + > > + /* clear ctrl register */ > > + ret = regmap_write(rx6110->regmap, RX6110_REG_CTRL, 0); > > + if (ret) > > + return ret; > > + > > + ctrl = 0; > > + > > + ret = regmap_write(rx6110->regmap, RX6110_REG_ALMIN, 0); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(rx6110->regmap, RX6110_REG_ALHOUR, 0); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(rx6110->regmap, RX6110_REG_ALWDAY, 0); > > + if (ret) > > + return ret; > > + > > + dev_dbg(&rtc->dev, "ext: %x, flag: %x, ctrl: %x\n", ext, flags, ctrl); > > + > > + /* check for VLF Flag (set at power-on) */ > > + if ((flags & RX6110_BIT_FLAG_VLF)) { > > + dev_warn(&rtc->dev, "Frequency stop was detected, probably due to a supply voltage drop\n"); > > + need_reset = 1; > > + need_clear = 1; > > + } > > Again, never reset the time. The correct handling of that flag is to > return an error on read until the time is set again.you can check the > current rx8025 or rv8803 drivers, they handle the same logic. Maybe you > could also align the warnings message for missed alarms on those drivers. > (...) To make it short: I have done everything of the above :-) Thanks, Steffen Trumtrar -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
