diff mbox

rtc: rtc-pcf8563: add alarm support

Message ID 1404402131-18556-2-git-send-email-vdonnefort@gmail.com
State Superseded
Headers show

Commit Message

Vincent Donnefort July 3, 2014, 3:42 p.m. UTC
This patch adds alarm support for the NXP PCF8563 chip.

Signed-off-by: Vincent Donnefort <vdonnefort@gmail.com>

Comments

Andrew Morton July 8, 2014, 9:19 p.m. UTC | #1
On Thu,  3 Jul 2014 17:42:11 +0200 Vincent Donnefort <vdonnefort@gmail.com> wrote:

> This patch adds alarm support for the NXP PCF8563 chip.
> 
> ...
>
> +static void pcf8563_work(struct work_struct *work)
> +{
> +	struct pcf8563 *pcf8563 = container_of(work, struct pcf8563, work);
> +	struct i2c_client *client = pcf8563->client;
> +
> +	mutex_lock(&pcf8563->rtc->ops_lock);
> +	pcf8563_set_alarm_mode(client, 1);
> +	enable_irq(client->irq);
> +	mutex_unlock(&pcf8563->rtc->ops_lock);
> +}
> +
> +static irqreturn_t pcf8563_irq(int irq, void *dev_id)
> +{
> +	struct pcf8563 *pcf8563 = i2c_get_clientdata(dev_id);
> +
> +	disable_irq_nosync(irq);
> +	schedule_work(&pcf8563->work);
> +	return IRQ_HANDLED;
> +}

Some code comments would be nice.  What is this IRQ handler actually
handling?  The alarm?  Why does it do what it it doing?

This interrupt was requested with IRQF_SHARED, so if some other device
is sharing the interrupt line, won't this function misbehave?

The code does schedule_work() but never does flush_work().  Isn't there
a risk that keventd will try to call pcf8563_work() after device
shutdown or even module removal?

> 
> ...
>
Simon Guinot July 9, 2014, 8:50 a.m. UTC | #2
On Thu, Jul 03, 2014 at 05:42:11PM +0200, Vincent Donnefort wrote:
> This patch adds alarm support for the NXP PCF8563 chip.
> 
> Signed-off-by: Vincent Donnefort <vdonnefort@gmail.com>

Hi Vincent,

> 
> diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
> index 63b558c..9448fed 100644
> --- a/drivers/rtc/rtc-pcf8563.c
> +++ b/drivers/rtc/rtc-pcf8563.c
> @@ -26,6 +26,8 @@
>  
>  #define PCF8563_REG_ST1		0x00 /* status */
>  #define PCF8563_REG_ST2		0x01
> +#define PCF8563_BIT_AIE		(1 << 1)
> +#define PCF8563_BIT_AF		(1 << 3)
>  
>  #define PCF8563_REG_SC		0x02 /* datetime */
>  #define PCF8563_REG_MN		0x03
> @@ -36,9 +38,6 @@
>  #define PCF8563_REG_YR		0x08
>  
>  #define PCF8563_REG_AMN		0x09 /* alarm */
> -#define PCF8563_REG_AHR		0x0A
> -#define PCF8563_REG_ADM		0x0B
> -#define PCF8563_REG_ADW		0x0C
>  
>  #define PCF8563_REG_CLKO	0x0D /* clock out */
>  #define PCF8563_REG_TMRC	0x0E /* timer control */
> @@ -67,37 +66,133 @@ struct pcf8563 {
>  	 */
>  	int c_polarity;	/* 0: MO_C=1 means 19xx, otherwise MO_C=1 means 20xx */
>  	int voltage_low; /* incicates if a low_voltage was detected */
> +
> +	struct work_struct work;
> +	struct i2c_client *client;
>  };
>  
> -/*
> - * In the routines that deal directly with the pcf8563 hardware, we use
> - * rtc_time -- month 0-11, hour 0-23, yr = calendar year-epoch.
> - */
> -static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
> +static int pcf8563_read_block_data(struct i2c_client *client, unsigned char reg,
> +				   unsigned char length, unsigned char *buf)
>  {
> -	struct pcf8563 *pcf8563 = i2c_get_clientdata(client);
> -	unsigned char buf[13] = { PCF8563_REG_ST1 };
> -
>  	struct i2c_msg msgs[] = {
>  		{/* setup read ptr */
>  			.addr = client->addr,
>  			.len = 1,
> -			.buf = buf
> +			.buf = &reg,
>  		},
> -		{/* read status + date */
> +		{
>  			.addr = client->addr,
>  			.flags = I2C_M_RD,
> -			.len = 13,
> +			.len = length,
>  			.buf = buf
>  		},
>  	};
>  
> -	/* read registers */
>  	if ((i2c_transfer(client->adapter, msgs, 2)) != 2) {
>  		dev_err(&client->dev, "%s: read error\n", __func__);
>  		return -EIO;
>  	}
>  
> +	return 0;
> +}
> +
> +static int pcf8563_write_block_data(struct i2c_client *client,
> +				   unsigned char reg, unsigned char length,
> +				   unsigned char *buf)
> +{
> +	int i, err;
> +
> +	for (i = 0; i < length; i++) {
> +		unsigned char data[2] = { reg + i, buf[i] };
> +
> +		err = i2c_master_send(client, data, sizeof(data));
> +		if (err != sizeof(data)) {
> +			dev_err(&client->dev,
> +				"%s: err=%d addr=%02x, data=%02x\n",
> +				__func__, err, data[0], data[1]);
> +			return -EIO;
> +		}
> +	}
> +
> +	return 0;
> +}

IIUC, you introduce two functions pcf8563_{read,write}_block_data which
helps to to factorize the I2C I/O operations.

Maybe we should have this refactoring work in a separate patch ?

> +
> +static int pcf8563_set_alarm_mode(struct i2c_client *client, bool on)
> +{
> +	unsigned char buf[2];
> +	int err;
> +
> +	err = pcf8563_read_block_data(client, PCF8563_REG_ST2, 1, buf + 1);
> +	if (err < 0)
> +		return err;
> +
> +	if (on)
> +		buf[1] |= PCF8563_BIT_AIE;
> +	else
> +		buf[1] &= ~PCF8563_BIT_AIE;
> +
> +	buf[1] &= ~PCF8563_BIT_AF;
> +	buf[0] = PCF8563_REG_ST2;
> +
> +	err = i2c_master_send(client, buf, 2);

Since you just introduced pcf8563_write_block_data, why you don't use it
here ?

> +	if (err < 0) {
> +		dev_err(&client->dev, "%s: write error\n", __func__);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}

...

>  static int pcf8563_probe(struct i2c_client *client,
>  				const struct i2c_device_id *id)
>  {
>  	struct pcf8563 *pcf8563;
> +	int err;
>  
>  	dev_dbg(&client->dev, "%s\n", __func__);
>  
> @@ -259,12 +412,35 @@ static int pcf8563_probe(struct i2c_client *client,
>  	dev_info(&client->dev, "chip found, driver version " DRV_VERSION "\n");
>  
>  	i2c_set_clientdata(client, pcf8563);
> +	pcf8563->client = client;
> +	device_set_wakeup_capable(&client->dev, 1);
>  
>  	pcf8563->rtc = devm_rtc_device_register(&client->dev,
>  				pcf8563_driver.driver.name,
>  				&pcf8563_rtc_ops, THIS_MODULE);
>  
>  	return PTR_ERR_OR_ZERO(pcf8563->rtc);
> +
> +	if (client->irq > 0) {
> +		INIT_WORK(&pcf8563->work, pcf8563_work);
> +		err = request_irq(client->irq, pcf8563_irq,
> +				  IRQF_SHARED|IRQF_TRIGGER_FALLING,
> +				  pcf8563->rtc->name, client);

I think you should use the devm_ variant here. This would simplify a
little bit the cleaning path.

Regards,

Simon
Vincent Donnefort July 21, 2014, 2:27 p.m. UTC | #3
changelog since v1:
  - use threaded IRQ
  - manage shared IRQ
  - remove workqueue
  - split patch to seperate read|write_block_data functions introducing
  - use devm_ function for requesting IRQ

Vincent Donnefort (2):
  rtc: rtc-pcf8563: introducing read|write_block_data
  rtc: rtc-pcf8563: add alarm support

 drivers/rtc/rtc-pcf8563.c | 231 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 202 insertions(+), 29 deletions(-)
diff mbox

Patch

diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 63b558c..9448fed 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -26,6 +26,8 @@ 
 
 #define PCF8563_REG_ST1		0x00 /* status */
 #define PCF8563_REG_ST2		0x01
+#define PCF8563_BIT_AIE		(1 << 1)
+#define PCF8563_BIT_AF		(1 << 3)
 
 #define PCF8563_REG_SC		0x02 /* datetime */
 #define PCF8563_REG_MN		0x03
@@ -36,9 +38,6 @@ 
 #define PCF8563_REG_YR		0x08
 
 #define PCF8563_REG_AMN		0x09 /* alarm */
-#define PCF8563_REG_AHR		0x0A
-#define PCF8563_REG_ADM		0x0B
-#define PCF8563_REG_ADW		0x0C
 
 #define PCF8563_REG_CLKO	0x0D /* clock out */
 #define PCF8563_REG_TMRC	0x0E /* timer control */
@@ -67,37 +66,133 @@  struct pcf8563 {
 	 */
 	int c_polarity;	/* 0: MO_C=1 means 19xx, otherwise MO_C=1 means 20xx */
 	int voltage_low; /* incicates if a low_voltage was detected */
+
+	struct work_struct work;
+	struct i2c_client *client;
 };
 
-/*
- * In the routines that deal directly with the pcf8563 hardware, we use
- * rtc_time -- month 0-11, hour 0-23, yr = calendar year-epoch.
- */
-static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
+static int pcf8563_read_block_data(struct i2c_client *client, unsigned char reg,
+				   unsigned char length, unsigned char *buf)
 {
-	struct pcf8563 *pcf8563 = i2c_get_clientdata(client);
-	unsigned char buf[13] = { PCF8563_REG_ST1 };
-
 	struct i2c_msg msgs[] = {
 		{/* setup read ptr */
 			.addr = client->addr,
 			.len = 1,
-			.buf = buf
+			.buf = &reg,
 		},
-		{/* read status + date */
+		{
 			.addr = client->addr,
 			.flags = I2C_M_RD,
-			.len = 13,
+			.len = length,
 			.buf = buf
 		},
 	};
 
-	/* read registers */
 	if ((i2c_transfer(client->adapter, msgs, 2)) != 2) {
 		dev_err(&client->dev, "%s: read error\n", __func__);
 		return -EIO;
 	}
 
+	return 0;
+}
+
+static int pcf8563_write_block_data(struct i2c_client *client,
+				   unsigned char reg, unsigned char length,
+				   unsigned char *buf)
+{
+	int i, err;
+
+	for (i = 0; i < length; i++) {
+		unsigned char data[2] = { reg + i, buf[i] };
+
+		err = i2c_master_send(client, data, sizeof(data));
+		if (err != sizeof(data)) {
+			dev_err(&client->dev,
+				"%s: err=%d addr=%02x, data=%02x\n",
+				__func__, err, data[0], data[1]);
+			return -EIO;
+		}
+	}
+
+	return 0;
+}
+
+static int pcf8563_set_alarm_mode(struct i2c_client *client, bool on)
+{
+	unsigned char buf[2];
+	int err;
+
+	err = pcf8563_read_block_data(client, PCF8563_REG_ST2, 1, buf + 1);
+	if (err < 0)
+		return err;
+
+	if (on)
+		buf[1] |= PCF8563_BIT_AIE;
+	else
+		buf[1] &= ~PCF8563_BIT_AIE;
+
+	buf[1] &= ~PCF8563_BIT_AF;
+	buf[0] = PCF8563_REG_ST2;
+
+	err = i2c_master_send(client, buf, 2);
+	if (err < 0) {
+		dev_err(&client->dev, "%s: write error\n", __func__);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int pcf8563_get_alarm_mode(struct i2c_client *client, unsigned char *en,
+				  unsigned char *pen)
+{
+	unsigned char buf;
+	int err;
+
+	err = pcf8563_read_block_data(client, PCF8563_REG_ST2, 1, &buf);
+	if (err)
+		return err;
+
+	*en = !!(buf & PCF8563_BIT_AIE);
+	*pen = !!(buf & PCF8563_BIT_AF);
+
+	return 0;
+}
+
+static void pcf8563_work(struct work_struct *work)
+{
+	struct pcf8563 *pcf8563 = container_of(work, struct pcf8563, work);
+	struct i2c_client *client = pcf8563->client;
+
+	mutex_lock(&pcf8563->rtc->ops_lock);
+	pcf8563_set_alarm_mode(client, 1);
+	enable_irq(client->irq);
+	mutex_unlock(&pcf8563->rtc->ops_lock);
+}
+
+static irqreturn_t pcf8563_irq(int irq, void *dev_id)
+{
+	struct pcf8563 *pcf8563 = i2c_get_clientdata(dev_id);
+
+	disable_irq_nosync(irq);
+	schedule_work(&pcf8563->work);
+	return IRQ_HANDLED;
+}
+
+/*
+ * In the routines that deal directly with the pcf8563 hardware, we use
+ * rtc_time -- month 0-11, hour 0-23, yr = calendar year-epoch.
+ */
+static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
+{
+	struct pcf8563 *pcf8563 = i2c_get_clientdata(client);
+	unsigned char buf[9];
+	int err;
+
+	err = pcf8563_read_block_data(client, PCF8563_REG_ST1, 9, buf);
+	if (err)
+		return err;
+
 	if (buf[PCF8563_REG_SC] & PCF8563_SC_LV) {
 		pcf8563->voltage_low = 1;
 		dev_info(&client->dev,
@@ -144,7 +239,7 @@  static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
 static int pcf8563_set_datetime(struct i2c_client *client, struct rtc_time *tm)
 {
 	struct pcf8563 *pcf8563 = i2c_get_clientdata(client);
-	int i, err;
+	int err;
 	unsigned char buf[9];
 
 	dev_dbg(&client->dev, "%s: secs=%d, mins=%d, hours=%d, "
@@ -170,19 +265,10 @@  static int pcf8563_set_datetime(struct i2c_client *client, struct rtc_time *tm)
 
 	buf[PCF8563_REG_DW] = tm->tm_wday & 0x07;
 
-	/* write register's data */
-	for (i = 0; i < 7; i++) {
-		unsigned char data[2] = { PCF8563_REG_SC + i,
-						buf[PCF8563_REG_SC + i] };
-
-		err = i2c_master_send(client, data, sizeof(data));
-		if (err != sizeof(data)) {
-			dev_err(&client->dev,
-				"%s: err=%d addr=%02x, data=%02x\n",
-				__func__, err, data[0], data[1]);
-			return -EIO;
-		}
-	}
+	err = pcf8563_write_block_data(client, PCF8563_REG_SC,
+				9 - PCF8563_REG_SC, buf + PCF8563_REG_SC);
+	if (err)
+		return err;
 
 	return 0;
 }
@@ -235,16 +321,83 @@  static int pcf8563_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return pcf8563_set_datetime(to_i2c_client(dev), tm);
 }
 
+static int pcf8563_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *tm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned char buf[4];
+	int err;
+
+	err = pcf8563_read_block_data(client, PCF8563_REG_AMN, 4, buf);
+	if (err)
+		return err;
+
+	dev_dbg(&client->dev,
+		"%s: raw data is min=%02x, hr=%02x, mday=%02x, wday=%02x\n",
+		__func__, buf[0], buf[1], buf[2], buf[3]);
+
+	tm->time.tm_min = bcd2bin(buf[0] & 0x7F);
+	tm->time.tm_hour = bcd2bin(buf[1] & 0x7F);
+	tm->time.tm_mday = bcd2bin(buf[2] & 0x1F);
+	tm->time.tm_wday = bcd2bin(buf[3] & 0x7);
+	tm->time.tm_mon = -1;
+	tm->time.tm_year = -1;
+	tm->time.tm_yday = -1;
+	tm->time.tm_isdst = -1;
+
+	err = pcf8563_get_alarm_mode(client, &tm->enabled, &tm->pending);
+	if (err < 0)
+		return err;
+
+	dev_dbg(&client->dev, "%s: tm is mins=%d, hours=%d, mday=%d, wday=%d,"
+		" enabled=%d, pending=%d\n", __func__, tm->time.tm_min,
+		tm->time.tm_hour, tm->time.tm_mday, tm->time.tm_wday,
+		tm->enabled, tm->pending);
+
+	return 0;
+}
+
+static int pcf8563_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned char buf[4];
+	int err;
+
+	dev_dbg(dev, "%s, min=%d hour=%d wday=%d mday=%d "
+		"enabled=%d pending=%d\n", __func__,
+		tm->time.tm_min, tm->time.tm_hour, tm->time.tm_wday,
+		tm->time.tm_mday, tm->enabled, tm->pending);
+
+	buf[0] = bin2bcd(tm->time.tm_min);
+	buf[1] = bin2bcd(tm->time.tm_hour);
+	buf[2] = bin2bcd(tm->time.tm_mday);
+	buf[3] = tm->time.tm_wday & 0x07;
+
+	err = pcf8563_write_block_data(client, PCF8563_REG_AMN, 4, buf);
+	if (err)
+		return err;
+
+	return pcf8563_set_alarm_mode(client, 1);
+}
+
+static int pcf8563_irq_enable(struct device *dev, unsigned int enabled)
+{
+	return pcf8563_set_alarm_mode(to_i2c_client(dev), !!enabled);
+}
+
 static const struct rtc_class_ops pcf8563_rtc_ops = {
 	.ioctl		= pcf8563_rtc_ioctl,
 	.read_time	= pcf8563_rtc_read_time,
 	.set_time	= pcf8563_rtc_set_time,
+	.read_alarm	= pcf8563_rtc_read_alarm,
+	.set_alarm	= pcf8563_rtc_set_alarm,
+	.alarm_irq_enable = pcf8563_irq_enable,
 };
 
 static int pcf8563_probe(struct i2c_client *client,
 				const struct i2c_device_id *id)
 {
 	struct pcf8563 *pcf8563;
+	int err;
 
 	dev_dbg(&client->dev, "%s\n", __func__);
 
@@ -259,12 +412,35 @@  static int pcf8563_probe(struct i2c_client *client,
 	dev_info(&client->dev, "chip found, driver version " DRV_VERSION "\n");
 
 	i2c_set_clientdata(client, pcf8563);
+	pcf8563->client = client;
+	device_set_wakeup_capable(&client->dev, 1);
 
 	pcf8563->rtc = devm_rtc_device_register(&client->dev,
 				pcf8563_driver.driver.name,
 				&pcf8563_rtc_ops, THIS_MODULE);
 
 	return PTR_ERR_OR_ZERO(pcf8563->rtc);
+
+	if (client->irq > 0) {
+		INIT_WORK(&pcf8563->work, pcf8563_work);
+		err = request_irq(client->irq, pcf8563_irq,
+				  IRQF_SHARED|IRQF_TRIGGER_FALLING,
+				  pcf8563->rtc->name, client);
+		if (err) {
+			dev_err(&client->dev, "unable to request IRQ %d\n",
+								client->irq);
+			return err;
+		}
+
+	}
+
+	return 0;
+}
+
+static int pcf8563_remove(struct i2c_client *client)
+{
+	free_irq(client->irq, client);
+	return 0;
 }
 
 static const struct i2c_device_id pcf8563_id[] = {
@@ -289,6 +465,7 @@  static struct i2c_driver pcf8563_driver = {
 		.of_match_table = of_match_ptr(pcf8563_of_match),
 	},
 	.probe		= pcf8563_probe,
+	.remove		= pcf8563_remove,
 	.id_table	= pcf8563_id,
 };