[1/1,RFC] Support for DS1306 over SPI [RFC]

Further work will be performed to add support for alarm
and non volatile memory.

Signed-off-by: Antonio R. Costa <costa.antonior@gmail.com>
---
 drivers/rtc/Kconfig          |    6 +
 drivers/rtc/Makefile         |    1 +
 drivers/rtc/rtc-ds1306-spi.c |  759 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 766 insertions(+), 0 deletions(-)
 create mode 100644 drivers/rtc/rtc-ds1306-spi.c

On Tue, 25 Nov 2008 17:28:06 +0100
"Antonio R. Costa" <costa.antonior@gmail.com> wrote:

> 
> Further work will be performed to add support for alarm
> and non volatile memory.
> 
> Signed-off-by: Antonio R. Costa <costa.antonior@gmail.com>

 Hello,

   thanks for your submission, comments below. this is a first review
 to handle the bigger things, there are probably many details
 to take care of.

 please review http://groups.google.com/group/rtc-linux/web/checklist
 and use checkpatch.pl -strict to check your patch.

 you might want to check the -mm tree for recent patches
 to other spi drivers.

> ---
>  drivers/rtc/Kconfig          |    6 +
>  drivers/rtc/Makefile         |    1 +
>  drivers/rtc/rtc-ds1306-spi.c |  759 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 766 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/rtc/rtc-ds1306-spi.c

 don't add the bus name to the drive rname


> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 4949dc4..85d12ac 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -282,6 +282,12 @@ config RTC_DRV_MAX6902
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-max6902.
>  
> +config RTC_DRV_DS1306
> +	boolean "Dallas DS1306 on SPI"
> +	depends on SPI
> +	help
> +	  If you say yes here you get support for the Maxim-Dallas DS1306 RTC chip via SPI.
> +
>  config RTC_DRV_R9701
>  	tristate "Epson RTC-9701JE"
>  	help
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index b6e14d5..1080fc7 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_RTC_DRV_BFIN)	+= rtc-bfin.o
>  obj-$(CONFIG_RTC_DRV_CMOS)	+= rtc-cmos.o
>  obj-$(CONFIG_RTC_DRV_DS1216)	+= rtc-ds1216.o
>  obj-$(CONFIG_RTC_DRV_DS1302)	+= rtc-ds1302.o
> +obj-$(CONFIG_RTC_DRV_DS1306)	+= rtc-ds1306-spi.o
>  obj-$(CONFIG_RTC_DRV_DS1307)	+= rtc-ds1307.o
>  obj-$(CONFIG_RTC_DRV_DS1374)	+= rtc-ds1374.o
>  obj-$(CONFIG_RTC_DRV_DS1511)	+= rtc-ds1511.o
> diff --git a/drivers/rtc/rtc-ds1306-spi.c b/drivers/rtc/rtc-ds1306-spi.c
> new file mode 100644
> index 0000000..8b38983
> --- /dev/null
> +++ b/drivers/rtc/rtc-ds1306-spi.c
> @@ -0,0 +1,759 @@
> +/*
> + * rtc-ds1306.c - RTC driver for DS1306 on SPI.
> + *
> + *  Copyright (C) 2007 Antonio R. Costa (costa.antonior@atmel.com)
> + *
> + * 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 Foundatio
> + */
> +
> +
> +#ifdef DEBUG
> +#define DS1306_DEBUG
> +#endif

 nope!


> +#include <linux/module.h>
> +#include <linux/version.h>
> +
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/rtc.h>
> +#include <linux/spi/spi.h>
> +#include <linux/string.h>
> +#include <linux/bcd.h>
> +#include <linux/delay.h>
> +
> +#include <linux/spinlock.h>

 too many includes, do you need them all?


> +
> +/* Registers map */
> +#define DS1306_A0_OFF		0x07
> +#define DS1306_A1_OFF		0x0b
> +#define DS1306_W_OFF		0x80
> +
> +#define DS1306_REG_SECS		0x00
> +#define DS1306_REG_MINS		0x01
> +#define DS1306_REG_HOURS	0x02
> +#define DS1306_REG_DAY		0x03
> +#define DS1306_REG_DATE		0x04
> +#define DS1306_REG_MONTHS	0x05
> +#define DS1306_REG_YEARS	0x06
> +
> +#define DS1306_REG_CTRL		0x0f
> +#define DS1306_REG_SR		0x10
> +
> +#define DS1306_NVRAM		0x20
> +#define DS1306_NVRAM_SIZE	0x60
> +#define DS1306_SIZE		0x80
> +
> +/* Register address macros */
> +
> +#define DS1306_W(x) \
> +	((x) + DS1306_W_OFF)
> +
> +#define DS1306_A0(x) \
> +	((x) + DS1306_A0_OFF)
> +
> +#define DS1306_A1(x) \
> +	((x) + DS1306_A1_OFF)
> +
> +
> +/* Bit fields */
> +
> +#define DS1306_SEC		0x0f
> +#define DS1306_10_SEC		0x70
> +
> +#define DS1306_MIN		0x0f
> +#define DS1306_10_MIN		0x70
> +
> +#define DS1306_HOUR		0x0f
> +#define DS1306_10_HOUR_12	0x10
> +#define DS1306_10_HOUR_24	0x30
> +#define DS1306_P_A		0x20
> +#define DS1306_12_24		0x40
> +
> +#define DS1306_DAY		0x07
> +
> +#define DS1306_DATE		0x0f
> +#define DS1306_10_DATE		0x30
> +
> +#define DS1306_MONTH		0x0f
> +#define DS1306_10_MONTH		0x30
> +
> +#define DS1306_YEAR		0x0f
> +#define DS1306_10_YEAR		0xf0
> +
> +#define DS1306_CTRL_W		0x40
> +#define DS1306_CTRL_HZ		0x04
> +#define DS1306_CTRL_AIE1	0x02
> +#define DS1306_CTRL_AIE0	0x01
> +
> +#define DS1306_SR_IRQF0		0x01
> +#define DS1306_SR_IRQF1		0x02
> +
> +struct ds1306_data_t {
> +	u8	reg_addr;
> +	u8	reg_data[7];
> +};
> +
> +union ds1306_data_u {
> +	struct ds1306_data_t s;
> +	u8		     b[8];
> +};
> +
> +struct ds1306_rtc {
> +	struct rtc_device	*rtc;
> +	union ds1306_data_u	data;
> +	u8	 		irq;
> +	u8			irq_mask;
> +	u16			tx_buf[2];
> +	u16			rx_buf[2];
> +	spinlock_t		lock;

 are you sure you need your own locking?
 you can use irq_lock or ops_lock in the struct rtc_device
 if appropriate.

> +};
> +
> +
> +static void ds1306_set_reg(struct device *dev, unsigned char address, u8 data)
> +{
> +        struct spi_device *spi = to_spi_device(dev);
> +        u8 tx_buf[2];
> +
> +        tx_buf[0] = DS1306_W(address);
> +        tx_buf[1] = data;
> +
> +        spi_write(spi, tx_buf, 2);
> +}

 please use spi_write_then_read (with a 0 read count)
 and local buffers. 


> +static int ds1306_get_reg(struct device *dev, unsigned char address, u8 *data)
> +{
> +        struct spi_device *spi = to_spi_device(dev);
> +        struct spi_message message;
> +        struct spi_transfer xfer;
> +	u8 rx_buf[2];
> +	u8 tx_buf[2];
> +        int status;
> +
> +        if (!data)
> +                return -EINVAL;
> +
> +        /* Build our spi message */
> +        spi_message_init(&message);
> +        memset(&xfer, 0, sizeof(xfer));
> +        xfer.len = 2;
> +        /* Can tx_buf and rx_buf be equal? The doc in spi.h is not sure... */
> +        xfer.tx_buf = tx_buf;
> +        xfer.rx_buf = rx_buf;
> +
> +        tx_buf[0] = address;
> +
> +        spi_message_add_tail(&xfer, &message);
> +
> +        /* do the I/O */
> +        status = spi_sync(spi, &message);
> +        if (status == 0)
> +                status = message.status;
> +        else
> +                return status;
> +
> +        *data = rx_buf[1];
> +
> +        return status;
> +}

 spi_write_then_read here too. always with local buffers.

> +
> +static int ds1306_read_burst(struct device *dev, u8 *buf, u8 offset,u8 len)
> +{
> +        struct spi_device *spi = to_spi_device(dev);
> +        struct spi_message message;
> +        struct spi_transfer xfer;
> +        u8 tx_buf[DS1306_SIZE+1], rx_buf[DS1306_SIZE+1];
> +	int status,i;
> +
> +	if(unlikely(len > DS1306_SIZE))
> +		return -ENOMEM;
> +
> +        /* build the message using temporary buffers */
> +        spi_message_init(&message);
> +        memset(&xfer, 0, sizeof(xfer));
> +        xfer.len = 1+len;    /* Address + Burst read len registers */
> +        tx_buf[0] = offset;  /* First register address */
> +        xfer.tx_buf = tx_buf;
> +        xfer.rx_buf = rx_buf;
> +        spi_message_add_tail(&xfer, &message);

 ditto.

> +        /* do the I/O */
> +        status = spi_sync(spi, &message);
> +        if (status == 0)
> +                status = message.status;
> +        else
> +                return status;
> +
> +	/* Data could be thrown away */
> +	if(unlikely(!buf))
> +		return 0;
> +
> +	/* Copy of data to final buffer skipping litter
> +	 * in the first position.
> +	 */
> +	for(i=1;i<len+1;i++)
> +		buf[i]=rx_buf[i];
> +
> +#ifdef DS1306_DEBUG
> +        dev_dbg(dev,"\n%s : offset: 0x%08x len: %u RTC values:\n",__FUNCTION__,offset, len);
> +	for(i=0;i<len;i++)
> +		dev_dbg(dev,"buf[%d]: %u\n",i,(unsigned int)rx_buf[i]);
> +#endif
> +
> +        return 0;
> +
> +}
> +
> +static int ds1306_write_burst(struct device *dev, u8 *buf, u8 offset, u8 len)
> +{
> +        struct spi_device *spi = to_spi_device(dev);
> +        struct ds1306_rtc *chip = dev_get_drvdata(dev);
> +        struct spi_message message;
> +        struct spi_transfer xfer;
> +	u8 tx_buf[DS1306_SIZE+1], rx_buf[DS1306_SIZE+1];
> +        int status,i;
> +
> +	if(unlikely(len > DS1306_SIZE))
> +		return -1;
> +
> +	/* build the message */
> +        spi_message_init(&message);
> +        memset(&xfer, 0, sizeof(xfer));
> +	memset(tx_buf,0, sizeof(tx_buf));
> +	memset(rx_buf,0, sizeof(rx_buf));
> +
> +        xfer.len = 1+len;		/* Address + Burst read len registers */
> +        tx_buf[0] = DS1306_W(offset);	/* First register address */
> +        xfer.tx_buf = tx_buf;
> +        xfer.rx_buf = rx_buf;
> +
> +	if(unlikely(!buf))
> +		return -ENOMEM;
> +	for(i=1;i<len+1;i++)
> +		tx_buf[i] = buf[i];
> +
> +#ifdef DS1306_DEBUG
> +        dev_dbg(dev,"%s : offset 0x%08x len: %u RTC values:\n",__FUNCTION__,offset,len);
> +	for(i=0;i<len;i++)
> +		dev_dbg(dev,"buf[%d]: %u\n",i,tx_buf[i]);
> +#endif
> +
> +	/* Remove write protection */
> +        ds1306_set_reg(dev, DS1306_REG_CTRL, chip->irq_mask);
> +
> +        spi_message_add_tail(&xfer, &message);
> +
> +        /* do the I/O */
> +        status = spi_sync(spi, &message);
> +
> +	/* Write protect */
> +        ds1306_set_reg(dev, DS1306_REG_CTRL, DS1306_CTRL_W | chip->irq_mask);
> +
> +        if (status == 0)
> +                status = message.status;
> +        else
> +                return status;
> +
> +        return 0;
> +}

 here again

> +
> +static int ds1306_get_datetime(struct device *dev, struct rtc_time *dt)
> +{
> +        struct spi_device *spi = to_spi_device(dev);
> +        struct ds1306_rtc *chip = dev_get_drvdata(dev);
> +        struct spi_message message;
> +        struct spi_transfer xfer;
> +        int status;
> +
> +
> +	/* build the message */
> +        spi_message_init(&message);
> +        memset(&xfer, 0, sizeof(xfer));
> +        xfer.len = 1+7;       /* Address + Burst read 7 registers */
> +        chip->data.s.reg_addr = DS1306_REG_SECS;    /* First register address */
> +        xfer.tx_buf = &chip->data.s.reg_addr;
> +        xfer.rx_buf = &chip->data.s.reg_addr;
> +        spi_message_add_tail(&xfer, &message);
> +
> +        /* do the I/O */
> +        status = spi_sync(spi, &message);
> +
> +        if (status == 0)
> +                status = message.status;
> +        else
> +                return status;

 and here

> +	/* ARC
> +	 * The chip sends data in this order:
> +         * Seconds, Minutes, Hours, Day, Date, Month, Year
> +	 * Year starts from 1970 2 digit wide.
> +	 */
> +
> +        dt->tm_sec      = BCD2BIN(chip->data.s.reg_data[0]);
> +        dt->tm_min      = BCD2BIN(chip->data.s.reg_data[1]);
> +        dt->tm_hour     = BCD2BIN(chip->data.s.reg_data[2]);
> +        dt->tm_wday     = BCD2BIN(chip->data.s.reg_data[3]);
> +        dt->tm_mday     = BCD2BIN(chip->data.s.reg_data[4]);
> +        dt->tm_mon      = BCD2BIN(chip->data.s.reg_data[5]);
> +        dt->tm_year 	= BCD2BIN(chip->data.s.reg_data[6]);

 use the lower case version of the macro

> +	/* ARC
> +	 * 1) Time struct starts counting years from 1900.
> +	 * 2) We want to keep track from year 1970.
> +	 * So year will be tm_year+1900-1970 = tm_year-70;
> +	 * 3) Time struct starts counting month from 0.
> +	 * So month will be tm_mon+1
> +	 */
> +	dt->tm_mon -= 1;
> +	dt->tm_year += 70;
> +
> +#ifdef DS1306_DEBUG
> +        dev_dbg(dev,"%s : Reading RTC values:\n",__FUNCTION__);
> +        dev_dbg(dev,"tm_sec : %i\n",dt->tm_sec);
> +        dev_dbg(dev,"tm_min : %i\n",dt->tm_min);
> +        dev_dbg(dev,"tm_hour: %i\n",dt->tm_hour);
> +        dev_dbg(dev,"tm_mday: %i\n",dt->tm_mday);
> +        dev_dbg(dev,"tm_wday: %i\n",dt->tm_wday);
> +        dev_dbg(dev,"tm_year: %i\n",dt->tm_year);
> +#endif
> +
> +
> +        return 0;
> +}

 return rtc_valid_tm

> +
> +static int ds1306_set_datetime(struct device *dev, struct rtc_time *dt)
> +{
> +        struct ds1306_rtc *chip = dev_get_drvdata(dev);
> +
> +	/* ARC
> +	 * Refer to comments in ds1306_get_datetime
> +	 */
> +	dt->tm_mon += 1;
> +        dt->tm_year -= 70;
> +
> +#ifdef DS1306_DEBUG
> +        dev_dbg(dev,"\n%s : Setting RTC values\n",__FUNCTION__);
> +        dev_dbg(dev,"tm_sec : %i\n",dt->tm_sec);
> +        dev_dbg(dev,"tm_min : %i\n",dt->tm_min);
> +        dev_dbg(dev,"tm_hour: %i\n",dt->tm_hour);
> +        dev_dbg(dev,"tm_mday: %i\n",dt->tm_mday);
> +        dev_dbg(dev,"tm_wday: %i\n",dt->tm_wday);
> +        dev_dbg(dev,"tm_year: %i\n",dt->tm_year);
> +#endif

 dev_dbg alone would do just fine without the ifdef

> +        /* Remove write protection */
> +        ds1306_set_reg(dev, DS1306_REG_CTRL, chip->irq_mask);
> +
> +        ds1306_set_reg(dev, DS1306_REG_SECS,  BIN2BCD(dt->tm_sec));
> +        ds1306_set_reg(dev, DS1306_REG_MINS,  BIN2BCD(dt->tm_min));
> +        ds1306_set_reg(dev, DS1306_REG_HOURS, BIN2BCD(dt->tm_hour));
> +
> +        ds1306_set_reg(dev, DS1306_REG_DATE,  BIN2BCD(dt->tm_mday));
> +        ds1306_set_reg(dev, DS1306_REG_MONTHS,BIN2BCD(dt->tm_mon));
> +        ds1306_set_reg(dev, DS1306_REG_DAY,   BIN2BCD(dt->tm_wday));
> +        ds1306_set_reg(dev, DS1306_REG_YEARS, BIN2BCD(dt->tm_year));
> +
> +        /* Write protect */
> +        ds1306_set_reg(dev, DS1306_REG_CTRL, DS1306_CTRL_W | chip->irq_mask);
> +
> +        return 0;
> +}
> +
> +static int ds1306_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	int ret = 0;
> +	down_interruptible(&dev->sem);
> +
> +        ret=ds1306_get_datetime(dev, tm);
> +
> +	up(&dev->sem);
> +
> +	return ret;
> +}
> +
> +static int ds1306_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	int ret = 0;
> +	down_interruptible(&dev->sem);
> +
> +        ret=ds1306_set_datetime(dev, tm);
> +
> +	up(&dev->sem);
> +
> +	return ret;
> +}

 you don't need locking here

> +static int ds1306_read_alarm(struct device *dev, struct rtc_time *tm,u8 alarm)
> +{
> +	int ret = 0;
> +	union ds1306_data_u data;
> +	dev_dbg(dev,"%s\n", __FUNCTION__);
> +
> +	if(likely(alarm==0))
> +		ret = ds1306_read_burst(dev,data.s.reg_data,DS1306_A0(DS1306_REG_SECS),4);
> +	else
> +		ret = ds1306_read_burst(dev,data.s.reg_data,DS1306_A1(DS1306_REG_SECS),4);
> +
> +	tm->tm_sec = BCD2BIN(data.s.reg_data[0]);
> +	tm->tm_min = BCD2BIN(data.s.reg_data[1]);
> +	tm->tm_hour = BCD2BIN(data.s.reg_data[2]);
> +	tm->tm_wday = BCD2BIN(data.s.reg_data[3]);

 wrong macro

> +	tm->tm_mday = 0;
> +	tm->tm_mon = 0;
> +	tm->tm_year = 0;
> +
> +#ifdef DS1306_DEBUG
> +        dev_dbg(dev,"\n%s: RTC values:\n",__FUNCTION__);
> +        dev_dbg(dev,"tm_sec : %i\n",tm->tm_sec);
> +        dev_dbg(dev,"tm_min : %i\n",tm->tm_min);
> +        dev_dbg(dev,"tm_hour: %i\n",tm->tm_hour);
> +        dev_dbg(dev,"tm_mday: %i\n",tm->tm_mday);
> +        dev_dbg(dev,"tm_wday: %i\n",tm->tm_wday);
> +        dev_dbg(dev,"tm_year: %i\n",tm->tm_year);
> +#endif
> +	return ret;
> +}
> +
> +static int ds1306_set_alarm(struct device *dev, struct rtc_time *tm, u8 alarm)
> +{
> +        struct platform_device *pdev = to_platform_device(dev);
> +        struct ds1306_rtc *chip = platform_get_drvdata(pdev);
> +	union  ds1306_data_u data;
> +	int ret;
> +
> +	if(!tm)
> +		return 0;

 no need to check, tm will always be there and valid

> +	data.s.reg_data[0] = BIN2BCD(tm->tm_sec);
> +	data.s.reg_data[1] = BIN2BCD(tm->tm_min);
> +	data.s.reg_data[2] = BIN2BCD(tm->tm_hour);
> +	data.s.reg_data[3] = BIN2BCD(tm->tm_wday);
> +
> +#ifdef DS1306_DEBUG
> +	dev_dbg(dev,"\n%s: RTC values:\n",__FUNCTION__);
> +	dev_dbg(dev,"tm_sec : %i\n",tm->tm_sec);
> +	dev_dbg(dev,"tm_min : %i\n",tm->tm_min);
> +	dev_dbg(dev,"tm_hour: %i\n",tm->tm_hour);
> +	dev_dbg(dev,"tm_mday: %i\n",tm->tm_mday);
> +	dev_dbg(dev,"tm_wday: %i\n",tm->tm_wday);
> +	dev_dbg(dev,"tm_year: %i\n",tm->tm_year);
> +#endif
> +
> +	/* Remove write protection */
> +        ds1306_set_reg(dev, DS1306_REG_CTRL, chip->irq_mask);
> +
> +	if(alarm==0)
> +		ret = ds1306_write_burst(dev,data.s.reg_data,DS1306_A0(DS1306_REG_SECS),4);
> +	else
> +		ret = ds1306_write_burst(dev,data.s.reg_data,DS1306_A1(DS1306_REG_SECS),4);
> +
> +	/* Write protect */
> +        ds1306_set_reg(dev, DS1306_REG_CTRL, DS1306_CTRL_W | chip->irq_mask);
> +
> +	return ret;
> +}
> +
> +
> +static int ds1306_set_wkalarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +        struct platform_device *pdev = to_platform_device(dev);
> +        struct ds1306_rtc *chip = platform_get_drvdata(pdev);
> +	int ret = 0;
> +
> +	dev_dbg(dev,"%s\n", __FUNCTION__);
> +
> +	down_interruptible(&dev->sem);
> +
> +	ret = ds1306_set_alarm(dev, &alarm->time,0);
> +
> +	if(alarm->enabled & RTC_AF)
> +		chip->irq_mask |= RTC_AF;
> +	else
> +		chip->irq_mask &= ~RTC_AF;
> +
> +	dev_dbg(dev, "%s wkalarm: 0x%08x irq_mask: 0x%08x\n",
> +		__FUNCTION__, alarm->enabled, chip->irq_mask);
> +
> +	/* Remove write protection */
> +        ds1306_set_reg(dev, DS1306_REG_CTRL, chip->irq_mask);
> +
> +	ds1306_set_reg(dev,DS1306_REG_CTRL, chip->irq_mask);
> +
> +	/* Write protect */
> +        ds1306_set_reg(dev, DS1306_REG_CTRL, DS1306_CTRL_W | chip->irq_mask);
> +
> +	up(&dev->sem);
> +
> +	return 0;
> +}
> +
> +static int ds1306_read_wkalarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +        struct ds1306_rtc *chip = dev_get_drvdata(dev);
> +
> +	int ret = 0;
> +
> +	dev_dbg(dev,"%s\n", __FUNCTION__);
> +
> +	down_interruptible(&dev->sem);
> +
> +	ret = ds1306_read_alarm(dev, &alarm->time,0);
> +
> +	ret |= ds1306_get_reg(dev,DS1306_REG_CTRL,&chip->irq_mask);
> +	ret |= ds1306_get_reg(dev,DS1306_REG_SR,&alarm->pending);
> +
> +	up(&dev->sem);
> +
> +	if(ret)
> +		return ret;
> +
> +	alarm->enabled &= RTC_AF;
> +	alarm->pending &= RTC_AF;
> +	return 0;
> +}
> +
> +
> +#if (defined(CONFIG_DS1306_RTC_IRQ) || defined(CONFIG_DS1306_RTC_HZ))
> +static int ds1306_irq_set_state(struct device *dev, int mask)
> +{
> +        struct ds1306_rtc *chip = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev,"%s\n", __FUNCTION__);
> +	dev_dbg(dev,"irq mask : 0x%08x\n", mask);
> +
> +	chip->irq_mask = mask;
> +
> +	/* Remove write protection then write
> +	 * again to be sure the the alarm flags
> +	 * has been written
> +	 */
> +	ds1306_set_reg(dev,DS1306_REG_CTRL,chip->irq_mask);
> +	ds1306_set_reg(dev,DS1306_REG_CTRL,chip->irq_mask);
> +	ds1306_set_reg(dev,DS1306_REG_CTRL,DS1306_CTRL_W | chip->irq_mask);
> +
> +	return 0;
> +}
> +#endif
> +
> +static int ds1306_irq_set_freq(struct device *dev, int freq)
> +{
> +	return 0;
> +}

 wrong. no empty functions.

> +
> +#if (defined(CONFIG_DS1306_RTC_IRQ) || defined(CONFIG_DS1306_RTC_HZ))
> +static int ds1306_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
> +{
> +        struct platform_device *pdev = to_platform_device(dev);
> +        struct ds1306_rtc *chip = platform_get_drvdata(pdev);
> +
> +	dev_dbg(dev,"*** %s ***\n", __FUNCTION__);
> +
> +	down_interruptible(&dev->sem);
> +
> +        switch (cmd) {
> +/*
> + * Alarm support ?
> + */
> +#ifdef CONFIG_DS1306_RTC_IRQ
> +	case RTC_UIE_OFF:
> +		dev_dbg(dev,"ioctl: RTC_UIE_OFF\n");
> +		ds1306_irq_set_state(dev,chip->irq_mask & ~DS1306_CTRL_AIE1);
> +		break;
> +	case RTC_UIE_ON:
> +		dev_dbg(dev,"ioctl: RTC_UIE_ON\n");
> +		ds1306_irq_set_state(dev,chip->irq_mask | DS1306_CTRL_AIE1);
> +		break;
> +	case RTC_AIE_OFF:
> +		dev_dbg(dev,"ioctl: RTC_AIE_OFF\n");
> +		ds1306_irq_set_state(dev,chip->irq_mask & ~DS1306_CTRL_AIE0);
> +		break;
> +	case RTC_AIE_ON:
> +		dev_dbg(dev,"ioctl: RTC_AIE_ON\n");
> +		ds1306_irq_set_state(dev,chip->irq_mask | DS1306_CTRL_AIE0);
> +		break;
> +	case RTC_ALM_SET:
> +		dev_dbg(dev,"ioctl: RTC_ALM_SET\n");
> +		ds1306_set_alarm(dev,(struct rtc_time *) arg,0);
> +		break;
> +	case RTC_ALM_READ:
> +		dev_dbg(dev,"ioctl: RTC_ALM_READ\n");
> +		ds1306_read_alarm(dev,(struct rtc_time *) arg,0);
> +		break;
> +#endif
> +/*
> + * HZ signal support ?
> + */
> +#ifdef CONFIG_DS1306_RTC_HZ
> +	case RTC_PIE_OFF:
> +		dev_dbg(dev,"ioctl: RTC_PIE_OFF\n");
> +                ds1306_irq_set_state(dev,chip->irq_mask & ~DS1306_CTRL_HZ);
> +                break;
> +        case RTC_PIE_ON:
> +		dev_dbg(dev,"ioctl: RTC_PIE_ON\n");
> +                ds1306_irq_set_state(dev,chip->irq_mask | DS1306_CTRL_HZ);
> +                break;
> +#endif
> +        default:
> +		dev_dbg(dev,"ioctl: cmd: 0x%08x arg: %p \n",cmd,(void*)arg);
> +		up(&dev->sem);
> +                return -ENOIOCTLCMD;
> +        }
> +
> +	up(&dev->sem);
> +
> +        return 0;
> +}
> +#endif

 don;t implement those ioctls directly,
 the rtc class will handle them with the appropriate pointers.
 see the latest -mm tree for the interface.

> +static const struct rtc_class_ops ds1306_rtc_ops = {
> +	.read_time	= ds1306_read_time,
> +	.set_time	= ds1306_set_time,
> +	.read_alarm	= ds1306_read_wkalarm,
> +	.set_alarm	= ds1306_set_wkalarm,
> +#if (defined(CONFIG_DS1306_RTC_IRQ) || defined(CONFIG_DS1306_RTC_HZ))
> +	.irq_set_state  = ds1306_irq_set_state,
> +#endif
> +	.irq_set_freq	= ds1306_irq_set_freq,
> +#if (defined(CONFIG_DS1306_RTC_IRQ) || defined(CONFIG_DS1306_RTC_HZ))
> +	.ioctl		= ds1306_ioctl,
> +#endif
> +};
> +
> +
> +struct ds1306_wq_alarm_t {
> +	unsigned int alarm;
> +	struct device *pdev;
> +};
> +
> +struct ds1306_wq_alarm_t ds1306_wq_alarm_data;
> +
> +void ds1306_read_alarm_wq(void)
> +{
> +	down_interruptible(&ds1306_wq_alarm_data.pdev->sem);
> +
> +	if(ds1306_wq_alarm_data.alarm==0)
> +		ds1306_read_burst(ds1306_wq_alarm_data.pdev,NULL,DS1306_A0(DS1306_REG_SECS),4);
> +	else
> +		ds1306_read_burst(ds1306_wq_alarm_data.pdev,NULL,DS1306_A1(DS1306_REG_SECS),4);
> +	up(&ds1306_wq_alarm_data.pdev->sem);
> +}
> +
> +
> +DECLARE_WORK(ds1306_alarm_work, ds1306_read_alarm_wq);
> +
> +static irqreturn_t ds1306_rtc_interrupt(int irq, void *dev)
> +{
> +        struct platform_device *pdev = to_platform_device(dev);
> +        struct ds1306_rtc *chip = platform_get_drvdata(pdev);
> +
> +	unsigned int ret = 0;
> +	unsigned char rtc_sr = 0;
> +	unsigned long events = 0;
> +
> +	spin_lock(&chip->lock);
> +
> +	ret=ds1306_get_reg(dev,DS1306_REG_SR,&rtc_sr);
> +
> +	if(rtc_sr & DS1306_SR_IRQF0) {
> +		ds1306_wq_alarm_data.alarm=0;
> +		ds1306_wq_alarm_data.pdev=dev;
> +		schedule_work(&ds1306_alarm_work);
> +		events |= RTC_IRQF | RTC_AF;
> +	}
> +
> +	if(rtc_sr & DS1306_SR_IRQF1) {
> +		ds1306_wq_alarm_data.alarm=1;
> +		ds1306_wq_alarm_data.pdev=dev;
> +		schedule_work(&ds1306_alarm_work);
> +		events |= RTC_IRQF | RTC_AF;
> +	}
> +
> +	rtc_update_irq(chip->rtc, 1, events);
> +
> +	spin_unlock(&chip->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +
> +static int __devinit ds1306_probe(struct spi_device *spi)
> +{
> +	struct ds1306_rtc *chip;
> +        struct rtc_device *rtc;
> +        u8 tmp;
> +        int res;
> +
> +        rtc = rtc_device_register("rtc-ds1306",
> +                                &spi->dev, &ds1306_rtc_ops, THIS_MODULE);
> +        if (IS_ERR(rtc))
> +                return PTR_ERR(rtc);
> +
> +        spi->mode = SPI_CS_HIGH | SPI_CPOL | SPI_CPHA;
> +        spi->bits_per_word = 8;
> +        spi_setup(spi);

 the bus must be initialized prior to the device registration!

> +        chip = kzalloc(sizeof *chip, GFP_KERNEL);
> +        if (!chip) {
> +                rtc_device_unregister(rtc);
> +                return -ENOMEM;
> +        }
> +        chip->rtc = rtc;
> +	spin_lock_init(&chip->lock);
> +        dev_set_drvdata(&spi->dev, chip);
> +
> +        res = ds1306_get_reg(&spi->dev, DS1306_REG_SECS, &tmp);
> +        if (res) {
> +                rtc_device_unregister(rtc);
> +                return res;
> +        }

 do this prior to registration.

> +//	request_irq(30,ds1306_rtc_interrupt,IRQF_SHARED,"rtc-ds1306",&spi->dev);

 what's that? no dead code or C99 comments

> +        return 0;
> +}
> +
> +static int __devexit ds1306_remove(struct spi_device *spi)
> +{
> +	struct ds1306_rtc *chip = platform_get_drvdata(spi);
> +	if(chip->rtc)
> +		rtc_device_unregister(chip->rtc);

 if you are here, chip->rtc is surely valid

> +	kfree(chip);
> +
> +	return 0;
> +}
> +
> +static struct spi_driver ds1306_driver = {
> +	.driver = {
> +		.name	= "rtc-ds1306",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= ds1306_probe,
> +	.remove		= __devexit_p(ds1306_remove),
> +};
> +
> +static int __init ds1306_init(void)
> +{
> +	printk(KERN_INFO "DS1306 SPI driver\n");
 no banners

> +	return spi_register_driver(&ds1306_driver);
> +}
> +module_init(ds1306_init);
> +
> +static void __exit ds1306_exit(void)
> +{
> +	spi_unregister_driver(&ds1306_driver);
> +}
> +module_exit(ds1306_exit);
> +
> +MODULE_AUTHOR("Antonio R. Costa <costa.antonior@atmel.com>");
> +MODULE_DESCRIPTION("RTC driver for DS1306 on SPI bus");
> +MODULE_LICENSE("GPL");
> -- 
> 1.5.4.3
> 
> 
> >