| Message ID | 1326320516.3096.64.camel@pc786-ubu.gnet.global.vpn |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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.
--- 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); } }