Re: [PATCH] rtc-isl1208: Use SMBus functions if I2C isn't available

[Ben Gardner]

Hi Jonathan,

Thanks for the review.

On Wed, Oct 26, 2011 at 3:40 AM, Jonathan Cameron <jic23@xxxxxxxxx> wrote:
> On 10/26/11 03:30, Ben Gardner wrote:
>> The rtc-isl1208 driver currently depends on raw I2C functionality.
>> This patch adds a fall-back to SMBus functions so that the driver can be
>> used on a SMBus-only platforms, such as i2c-isch.
>>
> Perhaps a summary of how bad things would be if smbus were all that is used?
> Afterall it is emulated on i2c buses if they don't support it directly.

Read and writes to the RTC chip should be a very rare thing, so saving
a few cycles by using the native I2C block read/write seems less
important than being compatible.
Perhaps I should just switch it to using SMBus functions and eliminate
the I2C block ops altogether?

Anyone else have an opinion on this?

>> -     ret = i2c_transfer(client->adapter, msgs, 2);
>> -     if (ret > 0)
>> -             ret = 0;
> It's a bit early in the morning, but at least at first glance I think
> this is an i2c_smbus_i2c_read_block_data reimplementation?

Do you mean i2c_smbus_read_i2c_block_data()?
That function is a bit different - it uses i2c_transfer() to emulate
the SMBus block read.
I'm not aware of any SMBus emulation of a I2C block read.

>> +     if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> +             u8 reg_addr[1] = { reg };
>> +             struct i2c_msg msgs[2] = {
>> +                     {client->addr, 0, sizeof(reg_addr), reg_addr}
>> +                     ,
> Odd spacing.

Sure is. It was that way in the original, but I guess I'll fix it. Or
eliminate it, if I remove I2C ops in the next version.

I'll await further comments and then repost in a day or so.

Thanks,
Ben
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/