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

login
register
mail settings
Submitter Austin Boyle
Date Jan. 11, 2012, 10:21 p.m.
Message ID <1326320516.3096.64.camel@pc786-ubu.gnet.global.vpn>
Download mbox | patch
Permalink /patch/135508/
State New
Headers show

Comments

Austin Boyle - Jan. 11, 2012, 10:21 p.m.
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
Wolfram Sang - Jan. 18, 2012, 9:41 p.m.
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
Wolfram Sang - Jan. 19, 2012, 7:45 p.m.
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
Austin Boyle - Feb. 2, 2012, 12:37 a.m.
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.
Wolfram Sang - Feb. 2, 2012, 2:53 p.m.
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
Austin Boyle - Feb. 2, 2012, 10:28 p.m.
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.
Wolfram Sang - Feb. 7, 2012, 2:56 p.m.
> 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.

Patch

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