diff mbox

[v3] rtc: ds1307: generalise ram size and offset

Message ID 1326144071.3096.25.camel@pc786-ubu.gnet.global.vpn
State Superseded
Headers show

Commit Message

Austin Boyle Jan. 9, 2012, 9:21 p.m. UTC 
Hi Wolfram,

Is the git tree you created for the ds1307 still active? How do you go about getting
those patches merged into linux-next?

Could you commit the following patch to your tree? Otherwise should I just create a
new patch against the latest kernel and send it to Andrew Morton?

Cheers,
Austin.

---

This patch generalises NVRAM to support RAM with other size and offset, such
as the 64 bytes of SRAM on the mcp7941x. Register offsets are added to chip
description instead of being hard-coded into probe function.

Signed-off-by: Austin Boyle <Austin.Boyle@aviatnet.com>
---
this patch is based on Wolfram Sang's tree:
git://git.pengutronix.de/git/wsa/linux-2.6.git ds1307

patch depends on:
rtc: ds1307: comment and format cleanup 21af5f7bd6
rtc: ds1307: simplify irq setup code  8c63e03627
rtc: ds1307: refactor chip_desc table e246db081d
rtc: add initial support for mcp7941x parts e69bba2d3a

Comments

Wolfram Sang Jan. 9, 2012, 10:17 p.m. UTC | #1 
Austin,

> Is the git tree you created for the ds1307 still active? How do you go about getting
> those patches merged into linux-next?

Well, yes, I was "actively" waiting for the rewritten NVRAM handling ;) (have I
missed a patch meanwhile?) I will review and pick up the patch in the next days
and then send a pull-request to Andrew.

Regards,

   Wolfram
Austin Boyle Jan. 10, 2012, 12:33 a.m. UTC | #2 
Hi Wolfram,

> Well, yes, I was "actively" waiting for the rewritten NVRAM handling ;) (have I
> missed a patch meanwhile?)
I sent the same patch to the mailing list with subject "[PATCH v3] rtc:
ds1307: generalise ram size and offset" on 4/11/11. The only error
identified was that I misspelt the name of David Anders in the CC list.

>  I will review and pick up the patch in the next days
> and then send a pull-request to Andrew.
Excellent thank you :)

Regards,
Austin.
Wolfram Sang Jan. 11, 2012, 11:06 a.m. UTC | #3 
Austin,

> --- a/drivers/rtc/rtc-ds1307.c	2011-10-10 11:22:22.674690998 +1300
> +++ b/drivers/rtc/rtc-ds1307.c	2011-11-04 10:02:27.859155009 +1300
> @@ -104,6 +104,8 @@ enum ds_type {
>  
>  struct ds1307 {
>  	u8			offset; /* register's offset */
> +	u16			nvram_offset;
> +	u16			nvram_size;

I'd put them further down in the struct, at least after regs.


>  	u8			regs[11];
>  	enum ds_type		type;
>  	unsigned long		flags;
> @@ -119,26 +121,37 @@ struct ds1307 {
>  };
>  
>  struct chip_desc {
> -	unsigned		nvram56:1;
>  	unsigned		alarm:1;
> +	u8			offset;

I think the stuff related to 'offset' should be in a separate patch with
specific commit-msg.

> +	u16			nvram_offset;
> +	u16			nvram_size;
>  };
>  
>  static const struct chip_desc chips[last_ds_type] = {
>  	[ds_1307] = {
> -		.nvram56	= 1,
> +		.nvram_offset	= 8,
> +		.nvram_size	= 56, /* 56 bytes NVRAM */

Skip the comment, should be obvious.

>  	},
>  	[ds_1337] = {
>  		.alarm		= 1,
>  	},
>  	[ds_1338] = {
> -		.nvram56	= 1,
> +		.nvram_offset	= 8,
> +		.nvram_size	= 56, /* 56 bytes NVRAM */
>  	},
>  	[ds_1339] = {
>  		.alarm		= 1,
>  	},
> +	[ds_1388] = {
> +		.offset		= 1, /* seconds starts at 1 */
> +	},
>  	[ds_3231] = {
>  		.alarm		= 1,
>  	},
> +	[mcp7941x] = {
> +		.nvram_offset	= 0x20,
> +		.nvram_size	= 64, /* 64 bytes SRAM */

Minor: hex value for size, too? Comment should probably emphasize that
this is SRAM not NVRAM, the size is obvious again.

> +	},
>  };
>  
...

> @@ -638,7 +650,19 @@ static int __devinit ds1307_probe(struct
>  
>  	ds1307->client	= client;
>  	ds1307->type	= id->driver_data;
> -	ds1307->offset	= 0;
> +
> +	if (chip && chip->offset)
> +		ds1307->offset = chip->offset;
> +	else
> +		ds1307->offset = 0;
> +	if (chip && chip->nvram_size)
> +		ds1307->nvram_size = chip->nvram_size;
> +	else
> +		ds1307->nvram_size = 0;
> +	if (chip && chip->nvram_offset)
> +		ds1307->nvram_offset = chip->nvram_offset;
> +	else
> +		ds1307->nvram_offset = 0;

ds1307 is kzalloced, so you can simplify this. Then, you also need to
check only once if chip != NULL.

Regards,

   Wolfram
diff mbox

Patch

--- a/drivers/rtc/rtc-ds1307.c	2011-10-10 11:22:22.674690998 +1300
+++ b/drivers/rtc/rtc-ds1307.c	2011-11-04 10:02:27.859155009 +1300
@@ -104,6 +104,8 @@  enum ds_type {
 
 struct ds1307 {
 	u8			offset; /* register's offset */
+	u16			nvram_offset;
+	u16			nvram_size;
 	u8			regs[11];
 	enum ds_type		type;
 	unsigned long		flags;
@@ -119,26 +121,37 @@  struct ds1307 {
 };
 
 struct chip_desc {
-	unsigned		nvram56:1;
 	unsigned		alarm:1;
+	u8			offset;
+	u16			nvram_offset;
+	u16			nvram_size;
 };
 
 static const struct chip_desc chips[last_ds_type] = {
 	[ds_1307] = {
-		.nvram56	= 1,
+		.nvram_offset	= 8,
+		.nvram_size	= 56, /* 56 bytes NVRAM */
 	},
 	[ds_1337] = {
 		.alarm		= 1,
 	},
 	[ds_1338] = {
-		.nvram56	= 1,
+		.nvram_offset	= 8,
+		.nvram_size	= 56, /* 56 bytes NVRAM */
 	},
 	[ds_1339] = {
 		.alarm		= 1,
 	},
+	[ds_1388] = {
+		.offset		= 1, /* seconds starts at 1 */
+	},
 	[ds_3231] = {
 		.alarm		= 1,
 	},
+	[mcp7941x] = {
+		.nvram_offset	= 0x20,
+		.nvram_size	= 64, /* 64 bytes SRAM */
+	},
 };
 
 static const struct i2c_device_id ds1307_id[] = {
@@ -543,8 +556,6 @@  static const struct rtc_class_ops ds13xx
 
 /*----------------------------------------------------------------------*/
 
-#define NVRAM_SIZE	56
-
 static ssize_t
 ds1307_nvram_read(struct file *filp, struct kobject *kobj,
 		struct bin_attribute *attr,
@@ -557,14 +568,15 @@  ds1307_nvram_read(struct file *filp, str
 	client = kobj_to_i2c_client(kobj);
 	ds1307 = i2c_get_clientdata(client);
 
-	if (unlikely(off >= NVRAM_SIZE))
+	if (unlikely(off >= ds1307->nvram_size))
 		return 0;
-	if ((off + count) > NVRAM_SIZE)
-		count = NVRAM_SIZE - off;
+	if ((off + count) > ds1307->nvram_size)
+		count = ds1307->nvram_size - off;
 	if (unlikely(!count))
 		return count;
 
-	result = ds1307->read_block_data(client, 8 + off, count, buf);
+	result = ds1307->read_block_data(client, ds1307->nvram_offset + off,
+								count, buf);
 	if (result < 0)
 		dev_err(&client->dev, "%s error %d\n", "nvram read", result);
 	return result;
@@ -582,14 +594,15 @@  ds1307_nvram_write(struct file *filp, st
 	client = kobj_to_i2c_client(kobj);
 	ds1307 = i2c_get_clientdata(client);
 
-	if (unlikely(off >= NVRAM_SIZE))
+	if (unlikely(off >= ds1307->nvram_size))
 		return -EFBIG;
-	if ((off + count) > NVRAM_SIZE)
-		count = NVRAM_SIZE - off;
+	if ((off + count) > ds1307->nvram_size)
+		count = ds1307->nvram_size - off;
 	if (unlikely(!count))
 		return count;
 
-	result = ds1307->write_block_data(client, 8 + off, count, buf);
+	result = ds1307->write_block_data(client, ds1307->nvram_offset + off,
+								count, buf);
 	if (result < 0) {
 		dev_err(&client->dev, "%s error %d\n", "nvram write", result);
 		return result;
@@ -605,7 +618,6 @@  static struct bin_attribute nvram = {
 
 	.read	= ds1307_nvram_read,
 	.write	= ds1307_nvram_write,
-	.size	= NVRAM_SIZE,
 };
 
 /*----------------------------------------------------------------------*/
@@ -638,7 +650,19 @@  static int __devinit ds1307_probe(struct
 
 	ds1307->client	= client;
 	ds1307->type	= id->driver_data;
-	ds1307->offset	= 0;
+
+	if (chip && chip->offset)
+		ds1307->offset = chip->offset;
+	else
+		ds1307->offset = 0;
+	if (chip && chip->nvram_size)
+		ds1307->nvram_size = chip->nvram_size;
+	else
+		ds1307->nvram_size = 0;
+	if (chip && chip->nvram_offset)
+		ds1307->nvram_offset = chip->nvram_offset;
+	else
+		ds1307->nvram_offset = 0;
 
 	buf = ds1307->regs;
 	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
@@ -756,9 +780,6 @@  static int __devinit ds1307_probe(struct
 						  hour);
 		}
 		break;
-	case ds_1388:
-		ds1307->offset = 1; /* Seconds starts at 1 */
-		break;
 	default:
 		break;
 	}
@@ -893,11 +914,12 @@  read_rtc:
 		dev_dbg(&client->dev, "got IRQ %d\n", client->irq);
 	}
 
-	if (chip && chip->nvram56) {
+	if (chip && chip->nvram_size) {
+		nvram.size = ds1307->nvram_size;
 		err = sysfs_create_bin_file(&client->dev.kobj, &nvram);
 		if (err == 0) {
 			set_bit(HAS_NVRAM, &ds1307->flags);
-			dev_info(&client->dev, "56 bytes nvram\n");
+			dev_info(&client->dev, "%zd bytes nvram\n", nvram.size);
 		}
 	}