[Resent to correct linux-i2c address]
Following up to Sebastien Barre's recent patch, here I go one step
further, replacing i2c block transfers with SMBus byte transfers.
Sticking to pure SMBus makes the driver work on our board, which has an
nVidia SMBus controller driving a DS1339; nforce2 controllers
unfortunately don't support any flavor of i2c block transfer.
Reading or writing registers repeatedly until they stick is rather
nauseating but I'm aiming for correctness if not elegance. I would
appreciate any suggestions for improvement.
Signed-off-by: Ed Swierk <[email protected]>
On Monday 05 January 2009, Ed Swierk wrote:
> Reading or writing registers repeatedly until they stick is rather
> nauseating but I'm aiming for correctness if not elegance. I would
> appreciate any suggestions for improvement.
Can i2c_smbus_read_i2c_block_data()/i2c_ smbus_write_i2c_block_data()
work better -- without nauseating anyone?
Faking atomic writes doesn't seem "correct" to me. Minimally there
is the case of an alarm misfiring.
- Dave
Hi Ed,
On Mon, 05 Jan 2009 09:41:01 -0800, Ed Swierk wrote:
> [Resent to correct linux-i2c address]
>
> Following up to Sebastien Barre's recent patch, here I go one step
> further, replacing i2c block transfers with SMBus byte transfers.
Please, no. Adding a compatibility quirk may be acceptable, but forcing
everyone to use it instead of the more efficient original code is not
fair.
> Sticking to pure SMBus makes the driver work on our board, which has an
> nVidia SMBus controller driving a DS1339; nforce2 controllers
> unfortunately don't support any flavor of i2c block transfer.
Are you certain the nForce2 controllers can't do it? The i2c-nforce2
driver doesn't implement it, but this doesn't mean the hardware can't
do it. I don't have any datasheet for these chips, but I know their
SMBus implementation is very similar to those of the AMD 8111, and
i2c-amd8111 has support for I2C block reads and writes. I think it
would be worth giving it a try, by copying the i2c-amd8111
implementation into the i2c-nforce2 driver and seeing if it happens to
just work. If it works, that would be more elegant than your proposed
hack to the rtc-ds1307 driver.
> Reading or writing registers repeatedly until they stick is rather
> nauseating but I'm aiming for correctness if not elegance. I would
> appreciate any suggestions for improvement.
See above.
--
Jean Delvare
On Wed, Jan 7, 2009 at 5:24 AM, Jean Delvare <[email protected]> wrote:
> Are you certain the nForce2 controllers can't do it? The i2c-nforce2
> driver doesn't implement it, but this doesn't mean the hardware can't
> do it. I don't have any datasheet for these chips, but I know their
> SMBus implementation is very similar to those of the AMD 8111, and
> i2c-amd8111 has support for I2C block reads and writes. I think it
> would be worth giving it a try, by copying the i2c-amd8111
> implementation into the i2c-nforce2 driver and seeing if it happens to
> just work. If it works, that would be more elegant than your proposed
> hack to the rtc-ds1307 driver.
I checked the datasheet and also tried every possible SMBus command
value to try to discover any supported commands that the i2c-nforce2
driver happens not to use, to no avail. The nVidia SMBus is pure
SMBus.
I could change the ds1307 driver to check whether the controller
supports i2c block commands and fall back to emulation only if they
are not available. Would that address your concerns?
--Ed
Hi Ed,
On Wed, 7 Jan 2009 07:22:12 -0800, Ed Swierk wrote:
> On Wed, Jan 7, 2009 at 5:24 AM, Jean Delvare <[email protected]> wrote:
> > Are you certain the nForce2 controllers can't do it? The i2c-nforce2
> > driver doesn't implement it, but this doesn't mean the hardware can't
> > do it. I don't have any datasheet for these chips, but I know their
> > SMBus implementation is very similar to those of the AMD 8111, and
> > i2c-amd8111 has support for I2C block reads and writes. I think it
> > would be worth giving it a try, by copying the i2c-amd8111
> > implementation into the i2c-nforce2 driver and seeing if it happens to
> > just work. If it works, that would be more elegant than your proposed
> > hack to the rtc-ds1307 driver.
>
> I checked the datasheet and also tried every possible SMBus command
> value to try to discover any supported commands that the i2c-nforce2
> driver happens not to use, to no avail. The nVidia SMBus is pure
> SMBus.
Did you try 0x4a (as i2c-amd8111 is using)?
So, you have the datasheet... Is this something you would be allowed to
share with me?
> I could change the ds1307 driver to check whether the controller
> supports i2c block commands and fall back to emulation only if they
> are not available. Would that address your concerns?
Yes, that would. Same thing the eeprom or lm93 drivers are doing, to
only name a few of them. Should be fairly easy.
--
Jean Delvare
On Wed, Jan 7, 2009 at 7:27 AM, Jean Delvare <[email protected]> wrote:
> Did you try 0x4a (as i2c-amd8111 is using)?
Yes, it returns an error, as do all the other unsupported commands.
> So, you have the datasheet... Is this something you would be allowed to
> share with me?
Unfortunately not; we acquired it under an NDA.
>> I could change the ds1307 driver to check whether the controller
>> supports i2c block commands and fall back to emulation only if they
>> are not available. Would that address your concerns?
>
> Yes, that would. Same thing the eeprom or lm93 drivers are doing, to
> only name a few of them. Should be fairly easy.
OK, will do.
--Ed
On Wed, 7 Jan 2009 07:43:18 -0800, Ed Swierk wrote:
> On Wed, Jan 7, 2009 at 7:27 AM, Jean Delvare <[email protected]> wrote:
> > Did you try 0x4a (as i2c-amd8111 is using)?
>
> Yes, it returns an error, as do all the other unsupported commands.
Oh well.
> > So, you have the datasheet... Is this something you would be allowed to
> > share with me?
>
> Unfortunately not; we acquired it under an NDA.
I expected that :(
By any chance, the datasheet doesn't explain why SMBus block
transactions of size 32 lock the chip, nor how to work around it?
> >> I could change the ds1307 driver to check whether the controller
> >> supports i2c block commands and fall back to emulation only if they
> >> are not available. Would that address your concerns?
> >
> > Yes, that would. Same thing the eeprom or lm93 drivers are doing, to
> > only name a few of them. Should be fairly easy.
>
> OK, will do.
OK. BTW, designing a system with an SMBus master which is so clearly
inappropriate for the I2C chips that are connected to it wasn't exactly
smart to start with. Whoever did this should be told to think twice
about it next time.
--
Jean Delvare
Here is an updated patch for rtc-ds1307 that allows the driver to work
with SMBus controllers like nforce2 that do not support i2c block
transfers. The driver now checks the capabilities of the controller and
emulates the block transfers with SMBus byte transfers only if
necessary.
Signed-off-by: Ed Swierk <[email protected]>
Hi Ed,
On Mon, 19 Jan 2009 15:59:37 -0800, Ed Swierk wrote:
> Here is an updated patch for rtc-ds1307 that allows the driver to work
> with SMBus controllers like nforce2 that do not support i2c block
> transfers. The driver now checks the capabilities of the controller and
> emulates the block transfers with SMBus byte transfers only if
> necessary.
>
> Signed-off-by: Ed Swierk <[email protected]>
Patch looks reasonable, with just minor objections:
> Index: linux-2.6.27.4/drivers/rtc/rtc-ds1307.c
> ===================================================================
> --- linux-2.6.27.4.orig/drivers/rtc/rtc-ds1307.c
> +++ linux-2.6.27.4/drivers/rtc/rtc-ds1307.c
> @@ -94,6 +94,10 @@ struct ds1307 {
> struct i2c_client *client;
> struct rtc_device *rtc;
> struct work_struct work;
> + s32 (*read_block_data)(struct i2c_client *client, u8 command,
> + u8 length, u8 *values);
> + s32 (*write_block_data)(struct i2c_client *client, u8 command,
> + u8 length, const u8 *values);
> };
>
> struct chip_desc {
> @@ -132,6 +136,61 @@ MODULE_DEVICE_TABLE(i2c, ds1307_id);
>
> /*----------------------------------------------------------------------*/
>
> +static s32 ds1307_read_block_data_once(struct i2c_client *client, u8 command,
> + u8 length, u8 *values)
> +{
> + s32 i, data;
> + for (i = 0; i < length; i++) {
> + data = i2c_smbus_read_byte_data(client, command + i);
> + if (data < 0)
> + return data;
> + *(values + i) = data;
> + }
> + return i;
> +}
> +
> +static s32 ds1307_read_block_data(struct i2c_client *client, u8 command,
> + u8 length, u8 *values)
> +{
> + u8 oldvalues[I2C_SMBUS_BLOCK_MAX];
> + s32 ret;
> + pr_debug("ds1307_read_block_data (length=%d)\n", length);
Please use dev_dbg(&client->dev, ...).
> + ret = ds1307_read_block_data_once(client, command, length, values);
> + if (ret < 0)
> + return ret;
> + do {
> + s32 ret;
Redeclaring a variable that already exists?
> + memcpy(oldvalues, values, length);
> + ret = ds1307_read_block_data_once(client, command, length, values);
> + if (ret < 0)
> + return ret;
> + } while (memcmp(oldvalues, values, length));
What guarantee do you have that you'll ever leave this loop?
> + return length;
> +}
> +
> +static s32 ds1307_write_block_data(struct i2c_client *client, u8 command,
> + u8 length, const u8 *values)
> +{
> + u8 currvalues[I2C_SMBUS_BLOCK_MAX];
> + pr_debug("ds1307_write_block_data (length=%d)\n", length);
dev_dbg
> + do {
> + s32 i, ret;
> + for (i = 0; i < length; i++) {
> + ret = i2c_smbus_write_byte_data(client, command + i,
> + *(values + i));
> + if (ret < 0)
> + return ret;
> + }
> + ret = ds1307_read_block_data_once(client, command, length,
> + currvalues);
> + if (ret < 0)
> + return ret;
> + } while (memcmp(currvalues, values, length));
Same question as above, what if you get stuck in the loop?
> + return length;
> +}
> +
> +/*----------------------------------------------------------------------*/
> +
> /*
All the rest is fine.
--
Jean Delvare
Here's another attempt at a patch for rtc-ds1307 that allows the driver
to work with SMBus controllers like nforce2 that do not support i2c
block transfers. The byte-oriented compatibility routines now give up
after 10 unsuccessful attempts to read or write a block of registers
without conflict.
Signed-off-by: Ed Swierk <[email protected]>
On Sun, 25 Jan 2009 22:33:50 -0800, Ed Swierk wrote:
> Here's another attempt at a patch for rtc-ds1307 that allows the driver
> to work with SMBus controllers like nforce2 that do not support i2c
> block transfers. The byte-oriented compatibility routines now give up
> after 10 unsuccessful attempts to read or write a block of registers
> without conflict.
>
> Signed-off-by: Ed Swierk <[email protected]>
Looks alright to me this time.
Acked-by: Jean Delvare <[email protected]>
--
Jean Delvare
On Sunday 25 January 2009, Ed Swierk wrote:
> Here's another attempt at a patch for rtc-ds1307 that allows the driver
> to work with SMBus controllers like nforce2 that do not support i2c
> block transfers. The byte-oriented compatibility routines now give up
> after 10 unsuccessful attempts to read or write a block of registers
> without conflict.
>
> Signed-off-by: Ed Swierk <[email protected]>
Well, other than the *conceptual* ugliness of this (which
we seem to be stuck with, courtesy the board designer) ...
I have two patch format issues:
- Always put a line of whitespace after variable
declarations.
- Include text like the above as part of the patch,
even if you're stuck with a mailer that mangles
non-attached text.
You're using Evolution, which is mentioned in the
Documentation/email-clients.txt file as having a
way (albeit awkward) to send normal patch email.
Given those changes:
Acked-by: David Brownell <[email protected]>
Thanks.
- Dave
On Mon, 2009-01-26 at 13:06 -0800, David Brownell wrote:
> I have two patch format issues:
>
> - Always put a line of whitespace after variable
> declarations.
>
> - Include text like the above as part of the patch,
> even if you're stuck with a mailer that mangles
> non-attached text.
>
> You're using Evolution, which is mentioned in the
> Documentation/email-clients.txt file as having a
> way (albeit awkward) to send normal patch email.
>
> Given those changes:
>
> Acked-by: David Brownell <[email protected]>
>
> Thanks.
Thanks for the feedback. Here you go:
Allow the rtc-ds1307 driver to work with SMBus controllers like nforce2
that do not support i2c block transfers.
Signed-off-by: Ed Swierk <[email protected]>
Index: linux-2.6.27.4/drivers/rtc/rtc-ds1307.c
===================================================================
--- linux-2.6.27.4.orig/drivers/rtc/rtc-ds1307.c
+++ linux-2.6.27.4/drivers/rtc/rtc-ds1307.c
@@ -94,6 +94,10 @@ struct ds1307 {
struct i2c_client *client;
struct rtc_device *rtc;
struct work_struct work;
+ s32 (*read_block_data)(struct i2c_client *client, u8 command,
+ u8 length, u8 *values);
+ s32 (*write_block_data)(struct i2c_client *client, u8 command,
+ u8 length, const u8 *values);
};
struct chip_desc {
@@ -132,6 +136,79 @@ MODULE_DEVICE_TABLE(i2c, ds1307_id);
/*----------------------------------------------------------------------*/
+#define BLOCK_DATA_MAX_TRIES 10
+
+static s32 ds1307_read_block_data_once(struct i2c_client *client, u8 command,
+ u8 length, u8 *values)
+{
+ s32 i, data;
+
+ for (i = 0; i < length; i++) {
+ data = i2c_smbus_read_byte_data(client, command + i);
+ if (data < 0)
+ return data;
+ values[i] = data;
+ }
+ return i;
+}
+
+static s32 ds1307_read_block_data(struct i2c_client *client, u8 command,
+ u8 length, u8 *values)
+{
+ u8 oldvalues[I2C_SMBUS_BLOCK_MAX];
+ s32 ret;
+ int tries = 0;
+
+ dev_dbg(&client->dev, "ds1307_read_block_data (length=%d)\n", length);
+ ret = ds1307_read_block_data_once(client, command, length, values);
+ if (ret < 0)
+ return ret;
+ do {
+ if (++tries > BLOCK_DATA_MAX_TRIES) {
+ dev_err(&client->dev,
+ "ds1307_read_block_data failed\n");
+ return -EIO;
+ }
+ memcpy(oldvalues, values, length);
+ ret = ds1307_read_block_data_once(client, command, length,
+ values);
+ if (ret < 0)
+ return ret;
+ } while (memcmp(oldvalues, values, length));
+ return length;
+}
+
+static s32 ds1307_write_block_data(struct i2c_client *client, u8 command,
+ u8 length, const u8 *values)
+{
+ u8 currvalues[I2C_SMBUS_BLOCK_MAX];
+ int tries = 0;
+
+ dev_dbg(&client->dev, "ds1307_write_block_data (length=%d)\n", length);
+ do {
+ s32 i, ret;
+
+ if (++tries > BLOCK_DATA_MAX_TRIES) {
+ dev_err(&client->dev,
+ "ds1307_write_block_data failed\n");
+ return -EIO;
+ }
+ for (i = 0; i < length; i++) {
+ ret = i2c_smbus_write_byte_data(client, command + i,
+ values[i]);
+ if (ret < 0)
+ return ret;
+ }
+ ret = ds1307_read_block_data_once(client, command, length,
+ currvalues);
+ if (ret < 0)
+ return ret;
+ } while (memcmp(currvalues, values, length));
+ return length;
+}
+
+/*----------------------------------------------------------------------*/
+
/*
* The IRQ logic includes a "real" handler running in IRQ context just
* long enough to schedule this workqueue entry. We need a task context
@@ -202,7 +279,7 @@ static int ds1307_get_time(struct device
int tmp;
/* read the RTC date and time registers all at once */
- tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
+ tmp = ds1307->read_block_data(ds1307->client,
DS1307_REG_SECS, 7, ds1307->regs);
if (tmp != 7) {
dev_err(dev, "%s error %d\n", "read", tmp);
@@ -279,7 +356,7 @@ static int ds1307_set_time(struct device
"write", buf[0], buf[1], buf[2], buf[3],
buf[4], buf[5], buf[6]);
- result = i2c_smbus_write_i2c_block_data(ds1307->client, 0, 7, buf);
+ result = ds1307->write_block_data(ds1307->client, 0, 7, buf);
if (result < 0) {
dev_err(dev, "%s error %d\n", "write", result);
return result;
@@ -297,7 +374,7 @@ static int ds1307_read_alarm(struct devi
return -EINVAL;
/* read all ALARM1, ALARM2, and status registers at once */
- ret = i2c_smbus_read_i2c_block_data(client,
+ ret = ds1307->read_block_data(client,
DS1339_REG_ALARM1_SECS, 9, ds1307->regs);
if (ret != 9) {
dev_err(dev, "%s error %d\n", "alarm read", ret);
@@ -356,7 +433,7 @@ static int ds1307_set_alarm(struct devic
t->enabled, t->pending);
/* read current status of both alarms and the chip */
- ret = i2c_smbus_read_i2c_block_data(client,
+ ret = ds1307->read_block_data(client,
DS1339_REG_ALARM1_SECS, 9, buf);
if (ret != 9) {
dev_err(dev, "%s error %d\n", "alarm write", ret);
@@ -391,7 +468,7 @@ static int ds1307_set_alarm(struct devic
}
buf[8] = status & ~(DS1337_BIT_A1I | DS1337_BIT_A2I);
- ret = i2c_smbus_write_i2c_block_data(client,
+ ret = ds1307->write_block_data(client,
DS1339_REG_ALARM1_SECS, 9, buf);
if (ret < 0) {
dev_err(dev, "can't set alarm time\n");
@@ -479,7 +556,7 @@ ds1307_nvram_read(struct kobject *kobj,
if (unlikely(!count))
return count;
- result = i2c_smbus_read_i2c_block_data(client, 8 + off, count, buf);
+ result = ds1307->read_block_data(client, 8 + off, count, buf);
if (result < 0)
dev_err(&client->dev, "%s error %d\n", "nvram read", result);
return result;
@@ -490,9 +567,11 @@ ds1307_nvram_write(struct kobject *kobj,
char *buf, loff_t off, size_t count)
{
struct i2c_client *client;
+ struct ds1307 *ds1307;
int result;
client = kobj_to_i2c_client(kobj);
+ ds1307 = i2c_get_clientdata(client);
if (unlikely(off >= NVRAM_SIZE))
return -EFBIG;
@@ -501,7 +580,7 @@ ds1307_nvram_write(struct kobject *kobj,
if (unlikely(!count))
return count;
- result = i2c_smbus_write_i2c_block_data(client, 8 + off, count, buf);
+ result = ds1307->write_block_data(client, 8 + off, count, buf);
if (result < 0) {
dev_err(&client->dev, "%s error %d\n", "nvram write", result);
return result;
@@ -536,9 +615,8 @@ static int __devinit ds1307_probe(struct
int want_irq = false;
unsigned char *buf;
- if (!i2c_check_functionality(adapter,
- I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
- I2C_FUNC_SMBUS_I2C_BLOCK))
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)
+ && !i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
return -EIO;
if (!(ds1307 = kzalloc(sizeof(struct ds1307), GFP_KERNEL)))
@@ -548,6 +626,13 @@ static int __devinit ds1307_probe(struct
i2c_set_clientdata(client, ds1307);
ds1307->type = id->driver_data;
buf = ds1307->regs;
+ if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
+ ds1307->read_block_data = i2c_smbus_read_i2c_block_data;
+ ds1307->write_block_data = i2c_smbus_write_i2c_block_data;
+ } else {
+ ds1307->read_block_data = ds1307_read_block_data;
+ ds1307->write_block_data = ds1307_write_block_data;
+ }
switch (ds1307->type) {
case ds_1337:
@@ -558,7 +643,7 @@ static int __devinit ds1307_probe(struct
want_irq = true;
}
/* get registers that the "rtc" read below won't read... */
- tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
+ tmp = ds1307->read_block_data(ds1307->client,
DS1337_REG_CONTROL, 2, buf);
if (tmp != 2) {
pr_debug("read error %d\n", tmp);
@@ -596,7 +681,7 @@ static int __devinit ds1307_probe(struct
read_rtc:
/* read RTC registers */
- tmp = i2c_smbus_read_i2c_block_data(ds1307->client, 0, 8, buf);
+ tmp = ds1307->read_block_data(ds1307->client, 0, 8, buf);
if (tmp != 8) {
pr_debug("read error %d\n", tmp);
err = -EIO;