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

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

[Austin Boyle]

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

--- 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);
 		}
 	}
 

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Austin Boyle]

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.

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2352 bytes --]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Austin Boyle]

Hi Wolfram,

Thanks for reviewing the patch.

> I think the stuff related to 'offset' should be in a separate patch with
> specific commit-msg.
Agreed. I will submit a separate patch after this one.

---

This patch generalises NVRAM to support RAM with other size and offset, such
as the 64 bytes of SRAM on the mcp7941x.

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

--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -105,6 +105,8 @@ enum ds_type {
 struct ds1307 {
 	u8			offset; /* register's offset */
 	u8			regs[11];
+	u16			nvram_offset;
+	u16			nvram_size;
 	enum ds_type		type;
 	unsigned long		flags;
 #define HAS_NVRAM	0		/* bit 0 == sysfs file active */
@@ -119,19 +121,22 @@ struct ds1307 {
 };
 
 struct chip_desc {
-	unsigned		nvram56:1;
 	unsigned		alarm:1;
+	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,
 	},
 	[ds_1337] = {
 		.alarm		= 1,
 	},
 	[ds_1338] = {
-		.nvram56	= 1,
+		.nvram_offset	= 8,
+		.nvram_size	= 56,
 	},
 	[ds_1339] = {
 		.alarm		= 1,
@@ -139,6 +144,11 @@ static const struct chip_desc chips[last_ds_type] = {
 	[ds_3231] = {
 		.alarm		= 1,
 	},
+	[mcp7941x] = {
+		/* this is battery backed SRAM */
+		.nvram_offset	= 0x20,
+		.nvram_size	= 0x40,
+	},
 };
 
 static const struct i2c_device_id ds1307_id[] = {
@@ -543,8 +553,6 @@ static const struct rtc_class_ops ds13xx_rtc_ops = {
 
 /*----------------------------------------------------------------------*/
 
-#define NVRAM_SIZE	56
-
 static ssize_t
 ds1307_nvram_read(struct file *filp, struct kobject *kobj,
 		struct bin_attribute *attr,
@@ -557,14 +565,15 @@ ds1307_nvram_read(struct file *filp, struct kobject *kobj,
 	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 +591,15 @@ ds1307_nvram_write(struct file *filp, struct kobject *kobj,
 	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 +615,6 @@ static struct bin_attribute nvram = {
 
 	.read	= ds1307_nvram_read,
 	.write	= ds1307_nvram_write,
-	.size	= NVRAM_SIZE,
 };
 
 /*----------------------------------------------------------------------*/
@@ -638,7 +647,12 @@ static int __devinit ds1307_probe(struct i2c_client *client,
 
 	ds1307->client	= client;
 	ds1307->type	= id->driver_data;
-	ds1307->offset	= 0;
+	if (chip) {
+		if (chip->nvram_size)
+			ds1307->nvram_size = chip->nvram_size;
+		if (chip->nvram_offset)
+			ds1307->nvram_offset = chip->nvram_offset;
+	}
 
 	buf = ds1307->regs;
 	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
@@ -893,11 +907,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);
 		}
 	}

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

Hi Austin,

> This patch generalises NVRAM to support RAM with other size and offset, such
> as the 64 bytes of SRAM on the mcp7941x.
> 
> Signed-off-by: Austin Boyle <Austin.Boyle@aviatnet.com>

I'll pick it up with one minor change:

Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>

> +	if (chip) {
> +		if (chip->nvram_size)
> +			ds1307->nvram_size = chip->nvram_size;
> +		if (chip->nvram_offset)
> +			ds1307->nvram_offset = chip->nvram_offset;

I'd say we can spare the two ifs above.

> +	}

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]


Sorry, found another thing.

> +	if (chip) {
> +		if (chip->nvram_size)
> +			ds1307->nvram_size = chip->nvram_size;
> +		if (chip->nvram_offset)
> +			ds1307->nvram_offset = chip->nvram_offset;
> +	}

I moved only the assignments...

>  
>  	buf = ds1307->regs;
>  	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
> @@ -893,11 +907,12 @@ read_rtc:
>  		dev_dbg(&client->dev, "got IRQ %d\n", client->irq);
>  	}
>  
> -	if (chip && chip->nvram56) {
> +	if (chip && chip->nvram_size) {

.. to here when I saw...

> +		nvram.size = ds1307->nvram_size;

... that nvram is static and we are changing it. Yeah, it is very unlikely but
if we have two RTC with different nvram sizes, one of them will not work correctly.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Austin Boyle]

Hi Wolfram,

I'm sorry about the slow response - I was away at linux.conf.au and then
haven't found enough time since I got back. 

On Thu, 2012-01-19 at 20:45 +0100, Wolfram Sang wrote:
> Sorry, found another thing.
> 
> > +	if (chip) {
> > +		if (chip->nvram_size)
> > +			ds1307->nvram_size = chip->nvram_size;
> > +		if (chip->nvram_offset)
> > +			ds1307->nvram_offset = chip->nvram_offset;
> > +	}
> 
> I moved only the assignments...
> 
> >  
> >  	buf = ds1307->regs;
> >  	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
> > @@ -893,11 +907,12 @@ read_rtc:
> >  		dev_dbg(&client->dev, "got IRQ %d\n", client->irq);
> >  	}
> >  
> > -	if (chip && chip->nvram56) {
> > +	if (chip && chip->nvram_size) {
> 
> .. to here when I saw...
> 
> > +		nvram.size = ds1307->nvram_size;
> 
> ... that nvram is static and we are changing it. Yeah, it is very unlikely but
> if we have two RTC with different nvram sizes, one of them will not work correctly.
I see the issue. Am I right that it would only occur when you have two
I2C buses, each with one of the RTC chips supported by this driver?

The fix I thought of would be to add a 'struct bin_attribute' pointer to
'struct ds1307' and then in the probe function allocate the structure,
set the size & callbacks, and pass it as an argument to
sysfs_create_bin_file. Do you think this is viable? 

It looks like Andrew Morton has already added this patch to the -mm
tree. Has he also grabbed the patches that this depends on?

Thanks,
Austin.

Re: [rtc-linux] Re: [PATCH v3] rtc: ds1307: generalise ram size and offset

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

Hi Austin,

> I'm sorry about the slow response - I was away at linux.conf.au and then
> haven't found enough time since I got back. 

Don't worry, I understand.

> > ... that nvram is static and we are changing it. Yeah, it is very unlikely but
> > if we have two RTC with different nvram sizes, one of them will not work correctly.
> I see the issue. Am I right that it would only occur when you have two
> I2C buses, each with one of the RTC chips supported by this driver?

Basically yes. They could be on one bus, though, using different addresses.

> The fix I thought of would be to add a 'struct bin_attribute' pointer to
> 'struct ds1307' and then in the probe function allocate the structure,
> set the size & callbacks, and pass it as an argument to
> sysfs_create_bin_file. Do you think this is viable? 

Yes.

> It looks like Andrew Morton has already added this patch to the -mm
> tree. Has he also grabbed the patches that this depends on?

I will check that and mail him later today.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Austin Boyle]

Hi Wolfram,

I have been thinking about this some more but maybe I don't understand
the problem...

> > ... that nvram is static and we are changing it. Yeah, it is very unlikely but
> > if we have two RTC with different nvram sizes, one of them will not work correctly.
> I see the issue. Am I right that it would only occur when you have two
> I2C buses, each with one of the RTC chips supported by this driver?

On my system, it looks like each rtc would have it's own directory e.g.:
# ls /sys/class/rtc
rtc0
# cat /sys/class/rtc/rtc0/device/name
ds1388
# ls /sys/class/rtc/rtc0/device/
driver     modalias   name    nvram    rtc    subsystem     uevent

So if you have multiple RTCs, even of the same type, they would be in
rtc0, rtc1, rtc2

Since they each have their own nvram, does it matter if the size is
changed?

Cheers,
Austin.

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

> So if you have multiple RTCs, even of the same type, they would be in
> rtc0, rtc1, rtc2
> 
> Since they each have their own nvram, does it matter if the size is
> changed?

Yes, because currently you pass a pointer to a static struct to the
sysfs_bin_attribute. If you change the size of the static struct to
another value, all other instances will get the new size, too, because
they still use the pointer to it.

You could just delete the size from the static struct, copy it over and
add the correct size to the copy and pass the copy to create_bin_file().
Or you initialize directly.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Austin Boyle]

rtc: ds1307: generalise ram size and offset

From: Austin Boyle <Austin.Boyle@aviatnet.com>

This patch generalises NVRAM to support RAM with other size and offset, such
as the 64 bytes of SRAM on the mcp7941x.

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

--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -105,6 +105,8 @@ enum ds_type {
 struct ds1307 {
 	u8			offset; /* register's offset */
 	u8			regs[11];
+	u16			nvram_offset;
+	struct bin_attribute	*nvram;
 	enum ds_type		type;
 	unsigned long		flags;
 #define HAS_NVRAM	0		/* bit 0 == sysfs file active */
@@ -119,19 +121,22 @@ struct ds1307 {
 };
 
 struct chip_desc {
-	unsigned		nvram56:1;
 	unsigned		alarm:1;
+	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,
 	},
 	[ds_1337] = {
 		.alarm		= 1,
 	},
 	[ds_1338] = {
-		.nvram56	= 1,
+		.nvram_offset	= 8,
+		.nvram_size	= 56,
 	},
 	[ds_1339] = {
 		.alarm		= 1,
@@ -139,6 +144,11 @@ static const struct chip_desc chips[last_ds_type] = {
 	[ds_3231] = {
 		.alarm		= 1,
 	},
+	[mcp7941x] = {
+		/* this is battery backed SRAM */
+		.nvram_offset	= 0x20,
+		.nvram_size	= 0x40,
+	},
 };
 
 static const struct i2c_device_id ds1307_id[] = {
@@ -543,8 +553,6 @@ static const struct rtc_class_ops ds13xx_rtc_ops = {
 
 /*----------------------------------------------------------------------*/
 
-#define NVRAM_SIZE	56
-
 static ssize_t
 ds1307_nvram_read(struct file *filp, struct kobject *kobj,
 		struct bin_attribute *attr,
@@ -557,14 +565,15 @@ ds1307_nvram_read(struct file *filp, struct kobject *kobj,
 	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 +591,15 @@ ds1307_nvram_write(struct file *filp, struct kobject *kobj,
 	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;
@@ -597,17 +607,6 @@ ds1307_nvram_write(struct file *filp, struct kobject *kobj,
 	return count;
 }
 
-static struct bin_attribute nvram = {
-	.attr = {
-		.name	= "nvram",
-		.mode	= S_IRUGO | S_IWUSR,
-	},
-
-	.read	= ds1307_nvram_read,
-	.write	= ds1307_nvram_write,
-	.size	= NVRAM_SIZE,
-};
-
 /*----------------------------------------------------------------------*/
 
 static int __devinit ds1307_probe(struct i2c_client *client,
@@ -893,16 +892,31 @@ read_rtc:
 		dev_dbg(&client->dev, "got IRQ %d\n", client->irq);
 	}
 
-	if (chip && chip->nvram56) {
-		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");
+	if (chip && chip->nvram_size) {
+		ds1307->nvram = kzalloc(sizeof(struct bin_attribute),
+							GFP_KERNEL);
+		if (!ds1307->nvram) {
+			err = -ENOMEM;
+			goto exit_nvram;
+		}
+		ds1307->nvram->attr.name = "nvram";
+		ds1307->nvram->attr.mode = S_IRUGO | S_IWUSR;
+		ds1307->nvram->read = ds1307_nvram_read,
+		ds1307->nvram->write = ds1307_nvram_write,
+		ds1307->nvram->size = chip->nvram_size;
+		ds1307->nvram_offset = chip->nvram_offset;
+		err = sysfs_create_bin_file(&client->dev.kobj, ds1307->nvram);
+		if (err) {
+			kfree(ds1307->nvram);
+			goto exit_nvram;
 		}
+		set_bit(HAS_NVRAM, &ds1307->flags);
+		dev_info(&client->dev, "%d bytes nvram\n", ds1307->nvram->size);
 	}
 
 	return 0;
 
+exit_nvram:
 exit_irq:
 	rtc_device_unregister(ds1307->rtc);
 exit_free:
@@ -919,8 +933,10 @@ static int __devexit ds1307_remove(struct i2c_client *client)
 		cancel_work_sync(&ds1307->work);
 	}
 
-	if (test_and_clear_bit(HAS_NVRAM, &ds1307->flags))
-		sysfs_remove_bin_file(&client->dev.kobj, &nvram);
+	if (test_and_clear_bit(HAS_NVRAM, &ds1307->flags)) {
+		sysfs_remove_bin_file(&client->dev.kobj, ds1307->nvram);
+		kfree(ds1307->nvram);
+	}
 
 	rtc_device_unregister(ds1307->rtc);
 	kfree(ds1307);

[PATCH v4] rtc: ds1307: generalise ram size and offset

[Austin Boyle]

Hi,

Wolfram: I saw the comments on your previous patch that chip will never be NULL
so I have removed the check from this patch also. This version is against
your most recent tree.

Andrew: Is it best for you if Wolfram adds this to his tree and then you
can pull the patches it depends on at the same time? The version of this
patch that has currently been added to -mm has some problems.

---

rtc: ds1307: generalise ram size and offset

From: Austin Boyle <Austin.Boyle@aviatnet.com>

This patch generalises NVRAM to support RAM with other size and offset, such
as the 64 bytes of SRAM on the mcp7941x.

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 38f0a1072f
rtc: ds1307: simplify irq setup code  f5af1f6ffe
rtc: ds1307: refactor chip_desc table c0920a32b7

--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -105,6 +105,8 @@ enum ds_type {
 struct ds1307 {
 	u8			offset; /* register's offset */
 	u8			regs[11];
+	u16			nvram_offset;
+	struct bin_attribute	*nvram;
 	enum ds_type		type;
 	unsigned long		flags;
 #define HAS_NVRAM	0		/* bit 0 == sysfs file active */
@@ -119,19 +121,22 @@ struct ds1307 {
 };
 
 struct chip_desc {
-	unsigned		nvram56:1;
 	unsigned		alarm:1;
+	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,
 	},
 	[ds_1337] = {
 		.alarm		= 1,
 	},
 	[ds_1338] = {
-		.nvram56	= 1,
+		.nvram_offset	= 8,
+		.nvram_size	= 56,
 	},
 	[ds_1339] = {
 		.alarm		= 1,
@@ -139,6 +144,11 @@ static const struct chip_desc chips[last_ds_type] = {
 	[ds_3231] = {
 		.alarm		= 1,
 	},
+	[mcp7941x] = {
+		/* this is battery backed SRAM */
+		.nvram_offset	= 0x20,
+		.nvram_size	= 0x40,
+	},
 };
 
 static const struct i2c_device_id ds1307_id[] = {
@@ -543,8 +553,6 @@ static const struct rtc_class_ops ds13xx_rtc_ops = {
 
 /*----------------------------------------------------------------------*/
 
-#define NVRAM_SIZE	56
-
 static ssize_t
 ds1307_nvram_read(struct file *filp, struct kobject *kobj,
 		struct bin_attribute *attr,
@@ -557,14 +565,15 @@ ds1307_nvram_read(struct file *filp, struct kobject *kobj,
 	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 +591,15 @@ ds1307_nvram_write(struct file *filp, struct kobject *kobj,
 	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;
@@ -597,17 +607,6 @@ ds1307_nvram_write(struct file *filp, struct kobject *kobj,
 	return count;
 }
 
-static struct bin_attribute nvram = {
-	.attr = {
-		.name	= "nvram",
-		.mode	= S_IRUGO | S_IWUSR,
-	},
-
-	.read	= ds1307_nvram_read,
-	.write	= ds1307_nvram_write,
-	.size	= NVRAM_SIZE,
-};
-
 /*----------------------------------------------------------------------*/
 
 static int __devinit ds1307_probe(struct i2c_client *client,
@@ -894,16 +893,31 @@ read_rtc:
 		dev_dbg(&client->dev, "got IRQ %d\n", client->irq);
 	}
 
-	if (chip->nvram56) {
-		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");
+	if (chip->nvram_size) {
+		ds1307->nvram = kzalloc(sizeof(struct bin_attribute),
+							GFP_KERNEL);
+		if (!ds1307->nvram) {
+			err = -ENOMEM;
+			goto exit_nvram;
+		}
+		ds1307->nvram->attr.name = "nvram";
+		ds1307->nvram->attr.mode = S_IRUGO | S_IWUSR;
+		ds1307->nvram->read = ds1307_nvram_read,
+		ds1307->nvram->write = ds1307_nvram_write,
+		ds1307->nvram->size = chip->nvram_size;
+		ds1307->nvram_offset = chip->nvram_offset;
+		err = sysfs_create_bin_file(&client->dev.kobj, ds1307->nvram);
+		if (err) {
+			kfree(ds1307->nvram);
+			goto exit_nvram;
 		}
+		set_bit(HAS_NVRAM, &ds1307->flags);
+		dev_info(&client->dev, "%d bytes nvram\n", ds1307->nvram->size);
 	}
 
 	return 0;
 
+exit_nvram:
 exit_irq:
 	rtc_device_unregister(ds1307->rtc);
 exit_free:
@@ -920,8 +934,10 @@ static int __devexit ds1307_remove(struct i2c_client *client)
 		cancel_work_sync(&ds1307->work);
 	}
 
-	if (test_and_clear_bit(HAS_NVRAM, &ds1307->flags))
-		sysfs_remove_bin_file(&client->dev.kobj, &nvram);
+	if (test_and_clear_bit(HAS_NVRAM, &ds1307->flags)) {
+		sysfs_remove_bin_file(&client->dev.kobj, ds1307->nvram);
+		kfree(ds1307->nvram);
+	}
 
 	rtc_device_unregister(ds1307->rtc);
 	kfree(ds1307);

Re: [PATCH v4] rtc: ds1307: generalise ram size and offset

[Andrew Morton]

On Thu, 9 Feb 2012 11:23:16 +1300
Austin Boyle <Austin.Boyle@Aviatnet.com> wrote:

> this patch is based on Wolfram Sang's tree:
> git://git.pengutronix.de/git/wsa/linux-2.6.git ds1307

Wolfram, that tree appears not to be in linux-next?  If so, please
let's get it in there.

Meanwhile I dropped the old version of this patch and won't (can't)
apply v4.

Re: [PATCH v4] rtc: ds1307: generalise ram size and offset

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

> > this patch is based on Wolfram Sang's tree:
> > git://git.pengutronix.de/git/wsa/linux-2.6.git ds1307
> 
> Wolfram, that tree appears not to be in linux-next?  If so, please
> let's get it in there.

This is a temporary tree to coordinate the works between David, Austin
and me. I wanted to resend all the patches to the rtc-list once Austin
fixed the last flaws I spotted.

I am tempted to do a tree for rtc-drivers, though. Didn't come around to
try to contact Alessandro regarding that topic yet.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4] rtc: ds1307: generalise ram size and offset

[Austin Boyle]

Hi Wolfram,

> > Wolfram, that tree appears not to be in linux-next?  If so, please
> > let's get it in there.
Let me know if there is anything I can do to assist.

> This is a temporary tree to coordinate the works between David, Austin
> and me. I wanted to resend all the patches to the rtc-list once Austin
> fixed the last flaws I spotted.
Do you have any comments/objections regarding v4?

Cheers,
Austin.

Re: [PATCH v4] rtc: ds1307: generalise ram size and offset

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

Austin,

> Do you have any comments/objections regarding v4?

Nope, I am happy with it. I picked the patch and pushed out an updated
branch based on 3.3-rc4 [1]. I'll give people two days to test it before
sending the series to Andrew.

Thanks,

   Wolfram

[1] git://git.pengutronix.de/git/wsa/linux-2.6.git ds1307

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Austin Boyle]

Hi David,

Did you ever get time to test this patch?

Thanks,
Austin.

On Thu, 2011-11-03 at 16:25 -0500, David Anders wrote:
> Austin,
> 
> On Thu, Nov 3, 2011 at 4:07 PM, Austin Boyle <Austin.Boyle@aviatnet.com> wrote:
> > From: Austin Boyle <Austin.Boyle@aviatnet.com>
> >
> > 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.
> >
> > Cc: Wolfram Sang <w.sang@pengutronix.de>
> > Cc: David Anderson <danders.dev@gmail.com>
> 
> please correct my name: David Anders
> 
> > Cc: Alessandro Zummo <a.zummo@towertech.it>
> > Cc: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > Signed-off-by: Austin Boyle <Austin.Boyle@aviatnet.com>
> 
> i'll apply and test for MCP7941x parts
> 
> > ---
> > 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
> >
> > --- 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);
> >                }
> >        }
> >
> >
> >
> >
> 
> Dave

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

[David Anders]

Austin,

On Thu, Nov 3, 2011 at 4:07 PM, Austin Boyle <Austin.Boyle@aviatnet.com> wrote:
> From: Austin Boyle <Austin.Boyle@aviatnet.com>
>
> 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.
>
> Cc: Wolfram Sang <w.sang@pengutronix.de>
> Cc: David Anderson <danders.dev@gmail.com>

please correct my name: David Anders

> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Signed-off-by: Austin Boyle <Austin.Boyle@aviatnet.com>

i'll apply and test for MCP7941x parts

> ---
> 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
>
> --- 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);
>                }
>        }
>
>
>
>

Dave

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

[Austin Boyle]

From: Austin Boyle <Austin.Boyle@aviatnet.com>

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.

Cc: Wolfram Sang <w.sang@pengutronix.de>
Cc: David Anderson <danders.dev@gmail.com>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
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

--- 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);
 		}
 	}