[1/3] rtc: add rv3029c2 RTC support

repost patch from Gregory Hermant <gregory.hermant@calao-systems.com>
original patch:
http://kerneltrap.org/mailarchive/linux-kernel/2010/6/6/4580032

changes:
- add set_alarm function
- corrected 12/24h handling
- add comments from Wan ZongShun

Signed-off-by: Heiko Schocher <hs@denx.de>
cc: Gregory Hermant <gregory.hermant@calao-systems.com>
cc: Wan ZongShun <mcuos.com@gmail.com>
cc: Alessandro Zummo <a.zummo@towertech.it>
cc: rtc-linux@googlegroups.com
---
./scripts/checkpatch.pl 0001-rtc-add-rv3029c2-RTC-support.patch
total: 0 errors, 0 warnings, 502 lines checked

0001-rtc-add-rv3029c2-RTC-support.patch has no obvious style problems and is ready for submission.

 drivers/rtc/Kconfig        |    9 +
 drivers/rtc/Makefile       |    1 +
 drivers/rtc/rtc-rv3029c2.c |  480 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 490 insertions(+), 0 deletions(-)
 create mode 100644 drivers/rtc/rtc-rv3029c2.c

Hi Heiko,

On Mon, Jan 10, 2011 at 09:04:12AM +0100, Heiko Schocher wrote:
> repost patch from Gregory Hermant <gregory.hermant@calao-systems.com>
> original patch:
> http://kerneltrap.org/mailarchive/linux-kernel/2010/6/6/4580032
> 
> changes:
> - add set_alarm function
> - corrected 12/24h handling
> - add comments from Wan ZongShun

IMHO all this information should go below the "---" line.
Gregory's SoB is missing, too.

> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> cc: Gregory Hermant <gregory.hermant@calao-systems.com>
> cc: Wan ZongShun <mcuos.com@gmail.com>
> cc: Alessandro Zummo <a.zummo@towertech.it>
> cc: rtc-linux@googlegroups.com
> ---
> ./scripts/checkpatch.pl 0001-rtc-add-rv3029c2-RTC-support.patch
> total: 0 errors, 0 warnings, 502 lines checked
> 
> 0001-rtc-add-rv3029c2-RTC-support.patch has no obvious style problems and is ready for submission.
> 
>  drivers/rtc/Kconfig        |    9 +
>  drivers/rtc/Makefile       |    1 +
>  drivers/rtc/rtc-rv3029c2.c |  480 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 490 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/rtc/rtc-rv3029c2.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 2883428..0f6c119 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -354,6 +354,15 @@ config RTC_DRV_RX8025
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-rx8025.
>  
> +config RTC_DRV_RV3029C2
> +	tristate "Micro Crystal RTC"
> +	help
> +	  If you say yes here you get support for the Micro Crystal
> +	  RV3029-C2 RTC chips.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called rtc-rv3029c2.
> +
>  endif # I2C
>  
>  comment "SPI RTC drivers"
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 4c2832d..16c0cdc 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_RTC_DRV_RP5C01)	+= rtc-rp5c01.o
>  obj-$(CONFIG_RTC_DRV_RS5C313)	+= rtc-rs5c313.o
>  obj-$(CONFIG_RTC_DRV_RS5C348)	+= rtc-rs5c348.o
>  obj-$(CONFIG_RTC_DRV_RS5C372)	+= rtc-rs5c372.o
> +obj-$(CONFIG_RTC_DRV_RV3029C2)	+= rtc-rv3029c2.o
>  obj-$(CONFIG_RTC_DRV_RX8025)	+= rtc-rx8025.o
>  obj-$(CONFIG_RTC_DRV_RX8581)	+= rtc-rx8581.o
>  obj-$(CONFIG_RTC_DRV_S35390A)	+= rtc-s35390a.o
> diff --git a/drivers/rtc/rtc-rv3029c2.c b/drivers/rtc/rtc-rv3029c2.c
> new file mode 100644
> index 0000000..28d830f
> --- /dev/null
> +++ b/drivers/rtc/rtc-rv3029c2.c
> @@ -0,0 +1,480 @@
> +/*
> + * Micro Crystal RV-3029C2 rtc class driver
> + *
> + * Author: Gregory Hermant <gregory.hermant@calao-systems.com>
> + *
> + * based on previously existing rtc class drivers
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * NOTE: Currently this driver only supports the bare minimum for read
> + * and write the RTC. The extra features provided by this chip
> + * (alarms, trickle charger, eeprom, T° compensation) are unavailable.

This is wrong according to the patch description.

> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/bcd.h>
> +#include <linux/rtc.h>
> +
> +#define DRV_VERSION "0.1"

Is this really needed?

> +
> +/* Register map */
> +/* control section */
> +#define RV3029C2_ONOFF_CTRL		0x00
> +#define RV3029C2_IRQ_CTRL		0x01
> +#define RV3029C2_IRQ_CTRL_AIE		(1 << 0)
> +#define RV3029C2_IRQ_FLAGS		0x02
> +#define RV3029C2_IRQ_FLAGS_AF		(1 << 0)
> +#define RV3029C2_STATUS			0x03
> +#define RV3029C2_STATUS_VLOW1		(1 << 2)
> +#define RV3029C2_STATUS_VLOW2		(1 << 3)
> +#define RV3029C2_STATUS_SR		(1 << 4)
> +#define RV3029C2_STATUS_EEBUSY		(1 << 7)
> +#define RV3029C2_RST_CTRL		0x04
> +#define RV3029C2_CONTROL_SECTION_LEN	0x05
> +
> +/* watch section */
> +#define RV3029C2_W_SEC			0x08
> +#define RV3029C2_W_MINUTES		0x09
> +#define RV3029C2_W_HOURS		0x0A
> +#define RV3029C2_REG_HR_12_24		(1<<6)  /* 24h/12h mode */
> +#define RV3029C2_REG_HR_PM		(1<<5)  /* PM/AM bit in 12h mode */
> +#define RV3029C2_W_DATE			0x0B
> +#define RV3029C2_W_DAYS			0x0C
> +#define RV3029C2_W_MONTHS		0x0D
> +#define RV3029C2_W_YEARS		0x0E
> +#define RV3029C2_WATCH_SECTION_LEN	0x07
> +
> +/* alarm section */
> +#define RV3029C2_A_SC			0x10
> +#define RV3029C2_A_MN			0x11
> +#define RV3029C2_A_HR			0x12
> +#define RV3029C2_A_DT			0x13
> +#define RV3029C2_A_DW			0x14
> +#define RV3029C2_A_MO			0x15
> +#define RV3029C2_A_YR			0x16
> +#define RV3029C2_ALARM_SECTION_LEN	0x07
> +
> +/* timer section */
> +#define RV3029C2_TIMER_LOW		0x18
> +#define RV3029C2_TIMER_HIGH		0x19
> +
> +/* temperature section */
> +#define RV3029C2_TEMP_PAGE		0x20
> +
> +/* eeprom data section */
> +#define RV3029C2_E2P_EEDATA1		0x28
> +#define RV3029C2_E2P_EEDATA2		0x29
> +
> +/* eeprom control section */
> +#define RV3029C2_CONTROL_E2P_EECTRL	0x30
> +#define RV3029C2_TRICKLE_1K		(1<<0)  /*  1K resistance */
> +#define RV3029C2_TRICKLE_5K		(1<<1)  /*  5K resistance */
> +#define RV3029C2_TRICKLE_20K		(1<<2)  /* 20K resistance */
> +#define RV3029C2_TRICKLE_80K		(1<<3)  /* 80K resistance */
> +#define RV3029C2_CONTROL_E2P_XTALOFFSET	0x31
> +#define RV3029C2_CONTROL_E2P_QCOEF	0x32
> +#define RV3029C2_CONTROL_E2P_TURNOVER	0x33
> +
> +/* user ram section */
> +#define RV3029C2_USR1_RAM_PAGE		0x38
> +#define RV3029C2_USR1_SECTION_LEN	0x04
> +#define RV3029C2_USR2_RAM_PAGE		0x3C
> +#define RV3029C2_USR2_SECTION_LEN	0x04
> +
> +static struct i2c_driver rv3029c2_driver;

A bit of reorganization will make this unneeded.

> +
> +static int
> +rv3029c2_i2c_read_regs(struct i2c_client *client, u8 reg, u8 *buf,
> +	unsigned len)
> +{
> +	u8 reg_addr[1] = { reg } ;
> +	struct i2c_msg msgs[2] = {
> +		{client->addr, 0, sizeof(reg_addr), reg_addr},
> +		{client->addr, I2C_M_RD, len, buf}
> +	};
> +	int ret;
> +
> +	BUG_ON(reg > RV3029C2_USR1_RAM_PAGE + 7);
> +	BUG_ON(reg + len > RV3029C2_USR1_RAM_PAGE + 8);

I think BUG is too much here, kernel will be halted!

> +
> +	ret = i2c_transfer(client->adapter, msgs, 2);
> +	if (ret > 0)
> +		ret = 0;

What is this for?

> +	return ret;
> +}
> +
> +static int
> +rv3029c2_i2c_write_regs(struct i2c_client *client, u8 reg, u8 const buf[],
> +			unsigned len)
> +{
> +	u8 i2c_buf[8];
> +	struct i2c_msg msgs[1] = {
> +		{client->addr, 0, len + 1, i2c_buf}
> +	};
> +	int ret;
> +
> +	BUG_ON(reg > RV3029C2_USR1_RAM_PAGE + 7);
> +	BUG_ON(reg + len > RV3029C2_USR1_RAM_PAGE + 8);
> +
> +	i2c_buf[0] = reg;
> +	memcpy(&i2c_buf[1], &buf[0], len);
> +	ret = i2c_transfer(client->adapter, msgs, 1);
> +	if (ret > 0)
> +		ret = 0;

What is this for?

> +	return ret;
> +}
> +
> +/* simple check to see if we have a rv3029c2 */
> +static int
> +rv3029c2_i2c_validate_client(struct i2c_client *client)
> +{
> +	u8 regs[RV3029C2_WATCH_SECTION_LEN] = { 0, };
> +	u8 zero_mask[RV3029C2_WATCH_SECTION_LEN] = {
> +		0x80, 0x80, 0x80, 0xc0, 0xf8, 0xe0, 0x80
> +	};
> +	int i;
> +	int ret;
> +
> +	ret = rv3029c2_i2c_read_regs(client, RV3029C2_W_SEC, regs,
> +					RV3029C2_WATCH_SECTION_LEN);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < RV3029C2_WATCH_SECTION_LEN; ++i) {
> +		if (regs[i] & zero_mask[i]) /* check if bits are cleared */
> +			return -ENODEV;
> +	}

I haven't checked the datasheet; will this work after a reset when time was
set before?

> +
> +	return 0;
> +}
> +
> +static int
> +rv3029c2_i2c_get_sr(struct i2c_client *client)
> +{
> +	u8 buf[1] = { 0, };

Comma?

> +	int sr = rv3029c2_i2c_read_regs(client, RV3029C2_STATUS, buf, 1);
> +
> +	dev_dbg(&client->dev, "status = 0x%.2x (%d)\n", buf[0], buf[0]);
> +	if (sr < 0)
> +		return -EIO;
> +	return sr;
> +}
> +
> +static int
> +rv3029c2_i2c_read_time(struct i2c_client *client, struct rtc_time *tm)
> +{
> +	int sr;
> +	u8 regs[RV3029C2_WATCH_SECTION_LEN] = { 0, };
> +
> +	sr = rv3029c2_i2c_get_sr(client);
> +	if (sr < 0) {
> +		dev_err(&client->dev, "%s: reading SR failed\n", __func__);
> +		return -EIO;
> +	}
> +
> +	sr = rv3029c2_i2c_read_regs(client, RV3029C2_W_SEC , regs,
> +					RV3029C2_WATCH_SECTION_LEN);
> +	if (sr < 0) {
> +		dev_err(&client->dev, "%s: reading RTC section failed\n",
> +			__func__);
> +		return sr;
> +	}
> +
> +	tm->tm_sec = bcd2bin(regs[RV3029C2_W_SEC-RV3029C2_W_SEC]);
> +	tm->tm_min = bcd2bin(regs[RV3029C2_W_MINUTES-RV3029C2_W_SEC]);
> +
> +	/* HR field has a more complex interpretation */
> +	{
> +		const u8 _hr = regs[RV3029C2_W_HOURS-RV3029C2_W_SEC];
> +		if (_hr & RV3029C2_REG_HR_12_24) {
> +			/* 12h format */
> +			tm->tm_hour = bcd2bin(_hr & 0x1f);
> +			if (_hr & RV3029C2_REG_HR_PM)	/* PM flag set */
> +				tm->tm_hour += 12;
> +		} else /* 24h format */
> +			tm->tm_hour = bcd2bin(_hr & 0x3f);
> +	}
> +
> +	tm->tm_mday = bcd2bin(regs[RV3029C2_W_DATE-RV3029C2_W_SEC]);
> +	tm->tm_mon = bcd2bin(regs[RV3029C2_W_MONTHS-RV3029C2_W_SEC]) - 1;
> +	tm->tm_year = bcd2bin(regs[RV3029C2_W_YEARS-RV3029C2_W_SEC]) + 100;
> +	tm->tm_wday = bcd2bin(regs[RV3029C2_W_DAYS-RV3029C2_W_SEC]) - 1;
> +
> +	return rtc_valid_tm(tm);
> +}
> +
> +static int rv3029c2_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	return rv3029c2_i2c_read_time(to_i2c_client(dev), tm);
> +}
> +
> +static int
> +rv3029c2_i2c_read_alarm(struct i2c_client *client, struct rtc_wkalrm *alarm)
> +{
> +	struct rtc_time *const tm = &alarm->time;
> +	int sr;
> +	u8 regs[8];
> +
> +	sr = rv3029c2_i2c_get_sr(client);
> +	if (sr < 0) {
> +		dev_err(&client->dev, "%s: reading SR failed\n", __func__);
> +		return -EIO;
> +	}
> +
> +	sr = rv3029c2_i2c_read_regs(client, RV3029C2_A_SC, regs,
> +					RV3029C2_ALARM_SECTION_LEN);
> +
> +	if (sr < 0) {
> +		dev_err(&client->dev, "%s: reading alarm section failed\n",
> +			__func__);
> +		return sr;
> +	}
> +
> +	tm->tm_sec = bcd2bin(regs[RV3029C2_A_SC-RV3029C2_A_SC] & 0x7f);
> +	tm->tm_min = bcd2bin(regs[RV3029C2_A_MN-RV3029C2_A_SC] & 0x7f);
> +	tm->tm_hour = bcd2bin(regs[RV3029C2_A_HR-RV3029C2_A_SC] & 0x3f);
> +	tm->tm_mday = bcd2bin(regs[RV3029C2_A_DT-RV3029C2_A_SC] & 0x3f);
> +	tm->tm_mon = bcd2bin(regs[RV3029C2_A_MO-RV3029C2_A_SC] & 0x1f) - 1;
> +	tm->tm_year = bcd2bin(regs[RV3029C2_A_YR-RV3029C2_A_SC] & 0x7f) + 100;
> +	tm->tm_wday = bcd2bin(regs[RV3029C2_A_DW-RV3029C2_A_SC] & 0x07) - 1;
> +
> +	return 0;
> +}
> +
> +static int
> +rv3029c2_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	return rv3029c2_i2c_read_alarm(to_i2c_client(dev), alarm);
> +}
> +
> +static int rv3029c2_rtc_i2c_alarm_set_irq(struct i2c_client *client,
> +					int enable)
> +{
> +	int ret;
> +	u8 buf[1];
> +
> +	/* enable AIE irq */
> +	ret = rv3029c2_i2c_read_regs(client, RV3029C2_IRQ_CTRL,	buf, 1);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "can't read INT reg\n");
> +		return ret;
> +	}
> +	if (enable)
> +		buf[0] |= RV3029C2_IRQ_CTRL_AIE;
> +	else
> +		buf[0] &= ~RV3029C2_IRQ_CTRL_AIE;
> +
> +	ret = rv3029c2_i2c_write_regs(client, RV3029C2_IRQ_CTRL, buf, 1);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "can't set INT reg\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rv3029c2_rtc_i2c_set_alarm(struct i2c_client *client,
> +					struct rtc_wkalrm *alarm)
> +{
> +	struct rtc_time *const tm = &alarm->time;
> +	int sr;
> +	int ret;
> +	u8 regs[8];
> +
> +	/*
> +	 * The clock has an 8 bit wide bcd-coded register (they never learn)
> +	 * for the year. tm_year is an offset from 1900 and we are interested
> +	 * in the 2000-2099 range, so any value less than 100 is invalid.
> +	*/
> +	if (tm->tm_year < 100)
> +		return -EINVAL;
> +
> +	sr = rv3029c2_i2c_get_sr(client);
> +	if (sr < 0) {
> +		dev_err(&client->dev, "%s: reading SR failed\n", __func__);
> +		return -EIO;
> +	}
> +	regs[RV3029C2_A_SC-RV3029C2_A_SC] = bin2bcd(tm->tm_sec & 0x7f);
> +	regs[RV3029C2_A_MN-RV3029C2_A_SC] = bin2bcd(tm->tm_min & 0x7f);
> +	regs[RV3029C2_A_HR-RV3029C2_A_SC] = bin2bcd(tm->tm_hour & 0x3f);
> +	regs[RV3029C2_A_DT-RV3029C2_A_SC] = bin2bcd(tm->tm_mday & 0x3f);
> +	regs[RV3029C2_A_MO-RV3029C2_A_SC] = bin2bcd((tm->tm_mon & 0x1f) - 1);
> +	regs[RV3029C2_A_DW-RV3029C2_A_SC] = bin2bcd((tm->tm_wday & 7) - 1);
> +	regs[RV3029C2_A_YR-RV3029C2_A_SC] = bin2bcd((tm->tm_year & 0x7f) - 100);
> +
> +	sr = rv3029c2_i2c_write_regs(client, RV3029C2_A_SC, regs,
> +					RV3029C2_ALARM_SECTION_LEN);
> +	if (sr < 0)
> +		return sr;
> +
> +	if (alarm->enabled) {
> +		u8 buf[1];
> +
> +		/* clear AF flag */
> +		ret = rv3029c2_i2c_read_regs(client, RV3029C2_IRQ_FLAGS,
> +						buf, 1);
> +		if (ret < 0) {
> +			dev_err(&client->dev, "can't read alarm flag\n");
> +			return ret;
> +		}
> +		buf[0] &= ~RV3029C2_IRQ_FLAGS_AF;
> +		ret = rv3029c2_i2c_write_regs(client, RV3029C2_IRQ_FLAGS,
> +						buf, 1);
> +		if (ret < 0) {
> +			dev_err(&client->dev, "can't set alarm flag\n");
> +			return ret;
> +		}
> +		/* enable AIE irq */
> +		ret = rv3029c2_rtc_i2c_alarm_set_irq(client, 1);
> +		if (ret)
> +			return ret;
> +
> +		dev_dbg(&client->dev, "alarm IRQ armed\n");
> +	} else {
> +		/* disable AIE irq */
> +		ret = rv3029c2_rtc_i2c_alarm_set_irq(client, 1);
> +		if (ret)
> +			return ret;
> +
> +		dev_dbg(&client->dev, "alarm IRQ disabled\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int rv3029c2_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	return rv3029c2_rtc_i2c_set_alarm(to_i2c_client(dev), alarm);
> +}
> +
> +static int
> +rv3029c2_i2c_set_time(struct i2c_client *client, struct rtc_time const *tm)
> +{
> +	u8 regs[8];
> +	int sr;
> +
> +	/*
> +	 * The clock has an 8 bit wide bcd-coded register (they never learn)
> +	 * for the year. tm_year is an offset from 1900 and we are interested
> +	 * in the 2000-2099 range, so any value less than 100 is invalid.
> +	*/
> +	if (tm->tm_year < 100)
> +		return -EINVAL;
> +
> +	regs[RV3029C2_W_SEC-RV3029C2_W_SEC] = bin2bcd(tm->tm_sec);
> +	regs[RV3029C2_W_MINUTES-RV3029C2_W_SEC] = bin2bcd(tm->tm_min);
> +	regs[RV3029C2_W_HOURS-RV3029C2_W_SEC] = bin2bcd(tm->tm_hour);
> +	regs[RV3029C2_W_DATE-RV3029C2_W_SEC] = bin2bcd(tm->tm_mday);
> +	regs[RV3029C2_W_MONTHS-RV3029C2_W_SEC] = bin2bcd(tm->tm_mon+1);
> +	regs[RV3029C2_W_DAYS-RV3029C2_W_SEC] = bin2bcd((tm->tm_wday & 7)+1);
> +	regs[RV3029C2_W_YEARS-RV3029C2_W_SEC] = bin2bcd(tm->tm_year - 100);
> +
> +	sr = rv3029c2_i2c_get_sr(client);
> +	if (sr < 0) {
> +		dev_err(&client->dev, "%s: reading SR failed\n", __func__);
> +		return sr;
> +	}
> +
> +	sr = rv3029c2_i2c_write_regs(client, RV3029C2_W_SEC, regs,
> +					RV3029C2_WATCH_SECTION_LEN);
> +	if (sr < 0)
> +		return sr;
> +
> +	return 0;
> +}
> +
> +static int rv3029c2_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	return rv3029c2_i2c_set_time(to_i2c_client(dev), tm);
> +}
> +
> +static const struct rtc_class_ops rv3029c2_rtc_ops = {
> +	.read_time	= rv3029c2_rtc_read_time,
> +	.set_time	= rv3029c2_rtc_set_time,
> +	.read_alarm	= rv3029c2_rtc_read_alarm,
> +	.set_alarm	= rv3029c2_rtc_set_alarm,
> +};
> +
> +static int
> +rv3029c2_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct rtc_device *rtc;
> +	int rc = 0;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return -ENODEV;

As all blocks transferred are < 32, what about using the smbus_block functions?

> +
> +	if (rv3029c2_i2c_validate_client(client) < 0)
> +		return -ENODEV;
> +
> +	dev_info(&client->dev,
> +		"chip found, driver version " DRV_VERSION "\n");
> +
> +	rtc = rtc_device_register(rv3029c2_driver.driver.name,
> +				&client->dev, &rv3029c2_rtc_ops,
> +				THIS_MODULE);
> +
> +	if (IS_ERR(rtc))
> +		return PTR_ERR(rtc);
> +
> +	i2c_set_clientdata(client, rtc);
> +
> +	rc = rv3029c2_i2c_get_sr(client);
> +	if (rc < 0) {
> +		dev_err(&client->dev, "reading status failed\n");
> +		goto exit_unregister;
> +	}
> +
> +	return 0;
> +
> +exit_unregister:
> +	rtc_device_unregister(rtc);
> +
> +	return rc;
> +}
> +
> +static int rv3029c2_remove(struct i2c_client *client)
> +{
> +	struct rtc_device *rtc = i2c_get_clientdata(client);
> +
> +	rtc_device_unregister(rtc);
> +
> +	return 0;
> +}
> +
> +static struct i2c_device_id rv3029c2_id[] = {
> +	{ "rv3029c2", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, rv3029c2_id);
> +
> +static struct i2c_driver rv3029c2_driver = {
> +	.driver = {
> +		.name = "rtc-rv3029c2",
> +	},
> +	.probe = rv3029c2_probe,
> +	.remove = rv3029c2_remove,

Sections of probe() and remove() to __devinit/exit?

> +	.id_table = rv3029c2_id,
> +};
> +
> +static int __init rv3029c2_init(void)
> +{
> +	return i2c_add_driver(&rv3029c2_driver);
> +}
> +
> +static void __exit rv3029c2_exit(void)
> +{
> +	i2c_del_driver(&rv3029c2_driver);
> +}
> +
> +module_init(rv3029c2_init);
> +module_exit(rv3029c2_exit);
> +
> +MODULE_AUTHOR("Gregory Hermant <gregory.hermant@calao-systems.com>");
> +MODULE_DESCRIPTION("Micro Crystal RV3029C2 RTC driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_VERSION);
> -- 
> 1.7.3.4
> 
> -- 
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.

Regards,

   Wolfram

Hello Wolfram,

Wolfram Sang wrote:
> On Mon, Jan 10, 2011 at 09:04:12AM +0100, Heiko Schocher wrote:
>> repost patch from Gregory Hermant <gregory.hermant@calao-systems.com>
>> original patch:
>> http://kerneltrap.org/mailarchive/linux-kernel/2010/6/6/4580032
>>
>> changes:
>> - add set_alarm function
>> - corrected 12/24h handling
>> - add comments from Wan ZongShun
> 
> IMHO all this information should go below the "---" line.

Ok.

> Gregory's SoB is missing, too.

Ok, hope Gregory is reading this EMails ...

>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> cc: Gregory Hermant <gregory.hermant@calao-systems.com>
>> cc: Wan ZongShun <mcuos.com@gmail.com>
>> cc: Alessandro Zummo <a.zummo@towertech.it>
>> cc: rtc-linux@googlegroups.com
>> ---
[...]
>> diff --git a/drivers/rtc/rtc-rv3029c2.c b/drivers/rtc/rtc-rv3029c2.c
>> new file mode 100644
>> index 0000000..28d830f
>> --- /dev/null
>> +++ b/drivers/rtc/rtc-rv3029c2.c
>> @@ -0,0 +1,480 @@
>> +/*
>> + * Micro Crystal RV-3029C2 rtc class driver
>> + *
>> + * Author: Gregory Hermant <gregory.hermant@calao-systems.com>
>> + *
>> + * based on previously existing rtc class drivers
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * NOTE: Currently this driver only supports the bare minimum for read
>> + * and write the RTC. The extra features provided by this chip
>> + * (alarms, trickle charger, eeprom, T° compensation) are unavailable.
> 
> This is wrong according to the patch description.

Yep, change this.

>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/bcd.h>
>> +#include <linux/rtc.h>
>> +
>> +#define DRV_VERSION "0.1"
> 
> Is this really needed?

I think no, Gregory?

>> +
>> +/* Register map */
>> +/* control section */
>> +#define RV3029C2_ONOFF_CTRL		0x00
>> +#define RV3029C2_IRQ_CTRL		0x01
>> +#define RV3029C2_IRQ_CTRL_AIE		(1 << 0)
>> +#define RV3029C2_IRQ_FLAGS		0x02
>> +#define RV3029C2_IRQ_FLAGS_AF		(1 << 0)
>> +#define RV3029C2_STATUS			0x03
>> +#define RV3029C2_STATUS_VLOW1		(1 << 2)
>> +#define RV3029C2_STATUS_VLOW2		(1 << 3)
>> +#define RV3029C2_STATUS_SR		(1 << 4)
>> +#define RV3029C2_STATUS_EEBUSY		(1 << 7)
>> +#define RV3029C2_RST_CTRL		0x04
>> +#define RV3029C2_CONTROL_SECTION_LEN	0x05
>> +
>> +/* watch section */
>> +#define RV3029C2_W_SEC			0x08
>> +#define RV3029C2_W_MINUTES		0x09
>> +#define RV3029C2_W_HOURS		0x0A
>> +#define RV3029C2_REG_HR_12_24		(1<<6)  /* 24h/12h mode */
>> +#define RV3029C2_REG_HR_PM		(1<<5)  /* PM/AM bit in 12h mode */
>> +#define RV3029C2_W_DATE			0x0B
>> +#define RV3029C2_W_DAYS			0x0C
>> +#define RV3029C2_W_MONTHS		0x0D
>> +#define RV3029C2_W_YEARS		0x0E
>> +#define RV3029C2_WATCH_SECTION_LEN	0x07
>> +
>> +/* alarm section */
>> +#define RV3029C2_A_SC			0x10
>> +#define RV3029C2_A_MN			0x11
>> +#define RV3029C2_A_HR			0x12
>> +#define RV3029C2_A_DT			0x13
>> +#define RV3029C2_A_DW			0x14
>> +#define RV3029C2_A_MO			0x15
>> +#define RV3029C2_A_YR			0x16
>> +#define RV3029C2_ALARM_SECTION_LEN	0x07
>> +
>> +/* timer section */
>> +#define RV3029C2_TIMER_LOW		0x18
>> +#define RV3029C2_TIMER_HIGH		0x19
>> +
>> +/* temperature section */
>> +#define RV3029C2_TEMP_PAGE		0x20
>> +
>> +/* eeprom data section */
>> +#define RV3029C2_E2P_EEDATA1		0x28
>> +#define RV3029C2_E2P_EEDATA2		0x29
>> +
>> +/* eeprom control section */
>> +#define RV3029C2_CONTROL_E2P_EECTRL	0x30
>> +#define RV3029C2_TRICKLE_1K		(1<<0)  /*  1K resistance */
>> +#define RV3029C2_TRICKLE_5K		(1<<1)  /*  5K resistance */
>> +#define RV3029C2_TRICKLE_20K		(1<<2)  /* 20K resistance */
>> +#define RV3029C2_TRICKLE_80K		(1<<3)  /* 80K resistance */
>> +#define RV3029C2_CONTROL_E2P_XTALOFFSET	0x31
>> +#define RV3029C2_CONTROL_E2P_QCOEF	0x32
>> +#define RV3029C2_CONTROL_E2P_TURNOVER	0x33
>> +
>> +/* user ram section */
>> +#define RV3029C2_USR1_RAM_PAGE		0x38
>> +#define RV3029C2_USR1_SECTION_LEN	0x04
>> +#define RV3029C2_USR2_RAM_PAGE		0x3C
>> +#define RV3029C2_USR2_SECTION_LEN	0x04
>> +
>> +static struct i2c_driver rv3029c2_driver;
> 
> A bit of reorganization will make this unneeded.

Ok.

>> +
>> +static int
>> +rv3029c2_i2c_read_regs(struct i2c_client *client, u8 reg, u8 *buf,
>> +	unsigned len)
>> +{
>> +	u8 reg_addr[1] = { reg } ;
>> +	struct i2c_msg msgs[2] = {
>> +		{client->addr, 0, sizeof(reg_addr), reg_addr},
>> +		{client->addr, I2C_M_RD, len, buf}
>> +	};
>> +	int ret;
>> +
>> +	BUG_ON(reg > RV3029C2_USR1_RAM_PAGE + 7);
>> +	BUG_ON(reg + len > RV3029C2_USR1_RAM_PAGE + 8);
> 
> I think BUG is too much here, kernel will be halted!

Yep, of course, change this.

>> +
>> +	ret = i2c_transfer(client->adapter, msgs, 2);
>> +	if (ret > 0)
>> +		ret = 0;
> 
> What is this for?

Not really needed, I check, if it works with smbus commands.

>> +	return ret;
>> +}
>> +
>> +static int
>> +rv3029c2_i2c_write_regs(struct i2c_client *client, u8 reg, u8 const buf[],
>> +			unsigned len)
>> +{
>> +	u8 i2c_buf[8];
>> +	struct i2c_msg msgs[1] = {
>> +		{client->addr, 0, len + 1, i2c_buf}
>> +	};
>> +	int ret;
>> +
>> +	BUG_ON(reg > RV3029C2_USR1_RAM_PAGE + 7);
>> +	BUG_ON(reg + len > RV3029C2_USR1_RAM_PAGE + 8);
>> +
>> +	i2c_buf[0] = reg;
>> +	memcpy(&i2c_buf[1], &buf[0], len);
>> +	ret = i2c_transfer(client->adapter, msgs, 1);
>> +	if (ret > 0)
>> +		ret = 0;
> 
> What is this for?

see above ...

>> +	return ret;
>> +}
>> +
>> +/* simple check to see if we have a rv3029c2 */
>> +static int
>> +rv3029c2_i2c_validate_client(struct i2c_client *client)
>> +{
>> +	u8 regs[RV3029C2_WATCH_SECTION_LEN] = { 0, };
>> +	u8 zero_mask[RV3029C2_WATCH_SECTION_LEN] = {
>> +		0x80, 0x80, 0x80, 0xc0, 0xf8, 0xe0, 0x80
>> +	};
>> +	int i;
>> +	int ret;
>> +
>> +	ret = rv3029c2_i2c_read_regs(client, RV3029C2_W_SEC, regs,
>> +					RV3029C2_WATCH_SECTION_LEN);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	for (i = 0; i < RV3029C2_WATCH_SECTION_LEN; ++i) {
>> +		if (regs[i] & zero_mask[i]) /* check if bits are cleared */
>> +			return -ENODEV;
>> +	}
> 
> I haven't checked the datasheet; will this work after a reset when time was
> set before?

Yes. This function checks, if the not used bits in the registers are set to 0.

>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +rv3029c2_i2c_get_sr(struct i2c_client *client)
>> +{
>> +	u8 buf[1] = { 0, };
> 
> Comma?

good catch! But not longer used, if using smbus commands.

>> +static int
>> +rv3029c2_probe(struct i2c_client *client, const struct i2c_device_id *id)
>> +{
>> +	struct rtc_device *rtc;
>> +	int rc = 0;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>> +		return -ENODEV;
> 
> As all blocks transferred are < 32, what about using the smbus_block functions?

Yep, check this.

>> +
>> +	if (rv3029c2_i2c_validate_client(client) < 0)
>> +		return -ENODEV;
>> +
>> +	dev_info(&client->dev,
>> +		"chip found, driver version " DRV_VERSION "\n");
>> +
>> +	rtc = rtc_device_register(rv3029c2_driver.driver.name,
>> +				&client->dev, &rv3029c2_rtc_ops,
>> +				THIS_MODULE);
>> +
>> +	if (IS_ERR(rtc))
>> +		return PTR_ERR(rtc);
>> +
>> +	i2c_set_clientdata(client, rtc);
>> +
>> +	rc = rv3029c2_i2c_get_sr(client);
>> +	if (rc < 0) {
>> +		dev_err(&client->dev, "reading status failed\n");
>> +		goto exit_unregister;
>> +	}
>> +
>> +	return 0;
>> +
>> +exit_unregister:
>> +	rtc_device_unregister(rtc);
>> +
>> +	return rc;
>> +}
>> +
>> +static int rv3029c2_remove(struct i2c_client *client)
>> +{
>> +	struct rtc_device *rtc = i2c_get_clientdata(client);
>> +
>> +	rtc_device_unregister(rtc);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct i2c_device_id rv3029c2_id[] = {
>> +	{ "rv3029c2", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, rv3029c2_id);
>> +
>> +static struct i2c_driver rv3029c2_driver = {
>> +	.driver = {
>> +		.name = "rtc-rv3029c2",
>> +	},
>> +	.probe = rv3029c2_probe,
>> +	.remove = rv3029c2_remove,
> 
> Sections of probe() and remove() to __devinit/exit?

Ok.

>> +	.id_table = rv3029c2_id,
>> +};
>> +
>> +static int __init rv3029c2_init(void)
>> +{
>> +	return i2c_add_driver(&rv3029c2_driver);
>> +}
>> +
>> +static void __exit rv3029c2_exit(void)
>> +{
>> +	i2c_del_driver(&rv3029c2_driver);
>> +}
>> +
>> +module_init(rv3029c2_init);
>> +module_exit(rv3029c2_exit);
>> +
>> +MODULE_AUTHOR("Gregory Hermant <gregory.hermant@calao-systems.com>");
>> +MODULE_DESCRIPTION("Micro Crystal RV3029C2 RTC driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_VERSION(DRV_VERSION);
>> -- 
>> 1.7.3.4
>>
>> -- 
>> You received this message because you are subscribed to "rtc-linux".
>> Membership options at http://groups.google.com/group/rtc-linux .
>> Please read http://groups.google.com/group/rtc-linux/web/checklist
>> before submitting a driver.
> 
> Regards,
> 
>    Wolfram

Thanks for your review!

bye,
Heiko

> > Gregory's SoB is missing, too.
> 
> Ok, hope Gregory is reading this EMails ...

He gave it on the original patch, so you can (and should) reuse it.

> >> +	for (i = 0; i < RV3029C2_WATCH_SECTION_LEN; ++i) {
> >> +		if (regs[i] & zero_mask[i]) /* check if bits are cleared */
> >> +			return -ENODEV;
> >> +	}
> > 
> > I haven't checked the datasheet; will this work after a reset when time was
> > set before?
> 
> Yes. This function checks, if the not used bits in the registers are set to 0.

Now I see. Ehrm, well, I recognize two other drivers are doing this as well,
but I dunno, calling those functions "validate" is a bit too much. It is more
an educated guess... I tend to think it is better to not give guarantees
instead of just trying to keep them.

> >> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> >> +		return -ENODEV;
> > 
> > As all blocks transferred are < 32, what about using the smbus_block functions?
> 
> Yep, check this.

Cool, thanks for doing that.

Regards,

   Wolfram

Hello Wolfram,

Wolfram Sang wrote:
>>> Gregory's SoB is missing, too.
>> Ok, hope Gregory is reading this EMails ...
> 
> He gave it on the original patch, so you can (and should) reuse it.

added.

>>>> +	for (i = 0; i < RV3029C2_WATCH_SECTION_LEN; ++i) {
>>>> +		if (regs[i] & zero_mask[i]) /* check if bits are cleared */
>>>> +			return -ENODEV;
>>>> +	}
>>> I haven't checked the datasheet; will this work after a reset when time was
>>> set before?
>> Yes. This function checks, if the not used bits in the registers are set to 0.
> 
> Now I see. Ehrm, well, I recognize two other drivers are doing this as well,
> but I dunno, calling those functions "validate" is a bit too much. It is more
> an educated guess... I tend to think it is better to not give guarantees
> instead of just trying to keep them.

Ok, if nobody objects, I remove this "check" ...

>>>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>>>> +		return -ENODEV;
>>> As all blocks transferred are < 32, what about using the smbus_block functions?
>> Yep, check this.
> 
> Cool, thanks for doing that.

No problem, thanks for the hint!

I wait for more comments, before sending an updated patch.

bye,
Heiko