* [PATCH] gpio-pca953x: fall back to byte-at-a-time for 24-bit io
@ 2017-12-05 2:17 Andrew Cooks
2017-12-29 9:44 ` Andy Shevchenko
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooks @ 2017-12-05 2:17 UTC (permalink / raw)
To: Linus Walleij, open list:GPIO SUBSYSTEM, open list
Cc: platypus-sw, Andrew Cooks
Using TCA6424A with i2c-piix4 bus driver requires byte-at-a-time IO,
because the i2c-piix4 driver (and probably some SMBus controllers) don't
support I2C_SMBUS_I2C_BLOCK_DATA.
Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
---
drivers/gpio/gpio-pca953x.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 1b9dbf6..9e74934 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -205,11 +205,23 @@ static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
{
+ int ret, i;
int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
- return i2c_smbus_write_i2c_block_data(chip->client,
+ if (i2c_check_functionality(chip->client->adapter,
+ I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
+ return i2c_smbus_write_i2c_block_data(chip->client,
(reg << bank_shift) | REG_ADDR_AI,
NBANK(chip), val);
+ } else {
+ for (i = 0; i < NBANK(chip); i++) {
+ ret = i2c_smbus_write_byte_data(chip->client,
+ (reg << 1) + i, val[i]);
+ if (ret < 0)
+ return ret;
+ }
+ return ret;
+ }
}
static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
@@ -249,7 +261,7 @@ static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
{
int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
- return i2c_smbus_read_i2c_block_data(chip->client,
+ return i2c_smbus_read_i2c_block_data_or_emulated(chip->client,
(reg << bank_shift) | REG_ADDR_AI,
NBANK(chip), val);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] gpio-pca953x: fall back to byte-at-a-time for 24-bit io
2017-12-05 2:17 [PATCH] gpio-pca953x: fall back to byte-at-a-time for 24-bit io Andrew Cooks
@ 2017-12-29 9:44 ` Andy Shevchenko
2018-01-29 4:11 ` Andrew Cooks
0 siblings, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2017-12-29 9:44 UTC (permalink / raw)
To: Andrew Cooks
Cc: Linus Walleij, open list:GPIO SUBSYSTEM, open list, platypus-sw
On Tue, Dec 5, 2017 at 4:17 AM, Andrew Cooks <andrew.cooks@opengear.com> wrote:
> Using TCA6424A with i2c-piix4 bus driver requires byte-at-a-time IO,
> because the i2c-piix4 driver (and probably some SMBus controllers) don't
> support I2C_SMBUS_I2C_BLOCK_DATA.
Why not to fix piix4 for now?
> static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
> {
> + int ret, i;
> int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>
> - return i2c_smbus_write_i2c_block_data(chip->client,
> + if (i2c_check_functionality(chip->client->adapter,
> + I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> + return i2c_smbus_write_i2c_block_data(chip->client,
> (reg << bank_shift) | REG_ADDR_AI,
> NBANK(chip), val);
> + } else {
Redundant and makes useless indentation level below.
> + for (i = 0; i < NBANK(chip); i++) {
> + ret = i2c_smbus_write_byte_data(chip->client,
> + (reg << 1) + i, val[i]);
> + if (ret < 0)
> + return ret;
> + }
> + return ret;
> + }
> }
>
> static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
> @@ -249,7 +261,7 @@ static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
> {
> int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>
> - return i2c_smbus_read_i2c_block_data(chip->client,
> + return i2c_smbus_read_i2c_block_data_or_emulated(chip->client,
> (reg << bank_shift) | REG_ADDR_AI,
> NBANK(chip), val);
Don't we have a counter part for writing?
Perhaps, it might be another option.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gpio-pca953x: fall back to byte-at-a-time for 24-bit io
2017-12-29 9:44 ` Andy Shevchenko
@ 2018-01-29 4:11 ` Andrew Cooks
2018-02-08 12:10 ` Jean Delvare
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooks @ 2018-01-29 4:11 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, open list:GPIO SUBSYSTEM, open list, platypus-sw,
Jean Delvare, linux-i2c
Hi Andy
On 29/12/17 19:44, Andy Shevchenko wrote:
> On Tue, Dec 5, 2017 at 4:17 AM, Andrew Cooks <andrew.cooks@opengear.com> wrote:
>> Using TCA6424A with i2c-piix4 bus driver requires byte-at-a-time IO,
>> because the i2c-piix4 driver (and probably some SMBus controllers) don't
>> support I2C_SMBUS_I2C_BLOCK_DATA.
>
> Why not to fix piix4 for now?
The piix4 driver applies to so many chips and has been around for such a long time, that I don't know if this kind of change is safe to make. Do you think it's safe to assume that all the implementations that use this driver can handle the 3byte block writes?
I've added Jean to the CC list to get the piix4 maintainer's perspective.
>
>> static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>> {
>> + int ret, i;
>> int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>>
>> - return i2c_smbus_write_i2c_block_data(chip->client,
>> + if (i2c_check_functionality(chip->client->adapter,
>> + I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
>> + return i2c_smbus_write_i2c_block_data(chip->client,
>> (reg << bank_shift) | REG_ADDR_AI,
>> NBANK(chip), val);
>
>> + } else {
>
> Redundant and makes useless indentation level below.
Thanks, will fix this.
>
>> + for (i = 0; i < NBANK(chip); i++) {
>> + ret = i2c_smbus_write_byte_data(chip->client,
>> + (reg << 1) + i, val[i]);
>> + if (ret < 0)
>> + return ret;
>> + }
>> + return ret;
>> + }
>> }
>>
>> static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
>> @@ -249,7 +261,7 @@ static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>> {
>> int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>>
>> - return i2c_smbus_read_i2c_block_data(chip->client,
>> + return i2c_smbus_read_i2c_block_data_or_emulated(chip->client,
>> (reg << bank_shift) | REG_ADDR_AI,
>> NBANK(chip), val);
>
> Don't we have a counter part for writing?
>
> Perhaps, it might be another option.
>
Again, I don't know if it's safe to assume that the i2c controllers can do this and would appreciate comments.
Thanks for your review!
a.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gpio-pca953x: fall back to byte-at-a-time for 24-bit io
2018-01-29 4:11 ` Andrew Cooks
@ 2018-02-08 12:10 ` Jean Delvare
0 siblings, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2018-02-08 12:10 UTC (permalink / raw)
To: Andrew Cooks
Cc: Andy Shevchenko, Linus Walleij, linux-gpio, linux-kernel,
platypus-sw, linux-i2c
On Mon, 29 Jan 2018 14:11:35 +1000, Andrew Cooks wrote:
> On 29/12/17 19:44, Andy Shevchenko wrote:
> > On Tue, Dec 5, 2017 at 4:17 AM, Andrew Cooks <andrew.cooks@opengear.com> wrote:
> >> Using TCA6424A with i2c-piix4 bus driver requires byte-at-a-time IO,
> >> because the i2c-piix4 driver (and probably some SMBus controllers) don't
> >> support I2C_SMBUS_I2C_BLOCK_DATA.
> >
> > Why not to fix piix4 for now?
>
> The piix4 driver applies to so many chips and has been around for such a long time, that I don't know if this kind of change is safe to make. Do you think it's safe to assume that all the implementations that use this driver can handle the 3byte block writes?
>
> I've added Jean to the CC list to get the piix4 maintainer's perspective.
This is a hardware limitation, not a driver deficiency. The original
Intel PIIX4 SMBus implementation did not support I2C Block transfers,
only SMBus Block transfers. I2C Block transfer support was added by
Intel to the 82801 (ICH5) only.
I have checked the few AMD datasheets I have here and it doesn't seem
that AMD implemented I2C Block transfers support either.
> >> @@ -249,7 +261,7 @@ static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
> >> {
> >> int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> >>
> >> - return i2c_smbus_read_i2c_block_data(chip->client,
> >> + return i2c_smbus_read_i2c_block_data_or_emulated(chip->client,
> >> (reg << bank_shift) | REG_ADDR_AI,
> >> NBANK(chip), val);
> >
> > Don't we have a counter part for writing?
> >
> > Perhaps, it might be another option.
>
> Again, I don't know if it's safe to assume that the i2c controllers can do this and would appreciate comments.
>
> Thanks for your review!
I would be very cautious with
i2c_smbus_read_i2c_block_data_or_emulated(). There are no official
semantics attached to I2C transfers, each device can have its own
idea of how it should react to a given transfer.
i2c_smbus_read_i2c_block_data_or_emulated() was implemented with
EEPROMs in mind and there is absolutely no guarantee that the
"emulation" will do the right thing. Before calling it, you must check
all the code paths in i2c_smbus_read_i2c_block_data_or_emulated() and
ensure that all will do the right thing for your device.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-08 12:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-05 2:17 [PATCH] gpio-pca953x: fall back to byte-at-a-time for 24-bit io Andrew Cooks
2017-12-29 9:44 ` Andy Shevchenko
2018-01-29 4:11 ` Andrew Cooks
2018-02-08 12:10 ` Jean Delvare
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).