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.
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/ |
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.
> 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/ |
rtc: ds1307: generalise ram size and offset
From: Austin Boyle <[email protected]>
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 <[email protected]>
---
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);
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 <[email protected]>
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 <[email protected]>
---
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);
On Thu, 9 Feb 2012 11:23:16 +1300
Austin Boyle <[email protected]> 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.
> > 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/ |
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.
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/ |