[PATCH] i2c-gpio.c: correct logic of pdata->scl_is_open_drain

[Voss, Nikolaus]

If pdata->scl_is_open_drain was set, the driver used push-pull output
for SCL, not open-drain output.

Signed-off-by: Nikolaus Voss <n.v...@weinmann.de>
---
drivers/i2c/busses/i2c-gpio.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index a651779..b161335 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -104,7 +104,7 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
if (ret)
goto err_request_scl;

- if (pdata->sda_is_open_drain) {
+ if (!pdata->sda_is_open_drain) {
gpio_direction_output(pdata->sda_pin, 1);
bit_data->setsda = i2c_gpio_setsda_val;
} else {
@@ -112,7 +112,7 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
bit_data->setsda = i2c_gpio_setsda_dir;
}

- if (pdata->scl_is_open_drain || pdata->scl_is_output_only) {
+ if (!pdata->scl_is_open_drain || pdata->scl_is_output_only) {
gpio_direction_output(pdata->scl_pin, 1);
bit_data->setscl = i2c_gpio_setscl_val;
} else {
--
1.7.4.1

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

[Håvard Skinnemoen]

On Mon, Oct 31, 2011 at 9:14 AM, Voss, Nikolaus <N.V...@weinmann.de> wrote:
> If pdata->scl_is_open_drain was set, the driver used push-pull output
> for SCL, not open-drain output.
>
> Signed-off-by: Nikolaus Voss <n.v...@weinmann.de>

{sda,scl}_is_open_drain indicates that the GPIO hardware is set up to
do open drain so the software doesn't have to, i.e.
gpio_set_value(pin, 1) will turn off the output driver rather than
drive the pin high, so the _val functions will do the right thing.

In other words, the existing code is correct.

Havard

[Voss, Nikolaus]

Hi Havard,

> {sda,scl}_is_open_drain indicates that the GPIO hardware is set up to
> do open drain so the software doesn't have to, i.e.
> gpio_set_value(pin, 1) will turn off the output driver rather than
> drive the pin high, so the _val functions will do the right thing.

gpio_set_value(pin, 1) drives the output high same as
gpio_direction_output(pin, 1) does (documented in Documentation/gpio.txt),
so the gpio will be changed from open-drain to push-pull.

In contrast, gpio_direction_input(pin) will turn off the output driver.

Open-drain io is bound to "!is_open_drain" and push-pull io is bound to
"is_open_drain", so I still think this should be the other way round.

Niko

[Håvard Skinnemoen]

On Mon, Oct 31, 2011 at 10:59 PM, Voss, Nikolaus <N.V...@weinmann.de> wrote:
> Hi Havard,
>
>> {sda,scl}_is_open_drain indicates that the GPIO hardware is set up to
>> do open drain so the software doesn't have to, i.e.
>> gpio_set_value(pin, 1) will turn off the output driver rather than
>> drive the pin high, so the _val functions will do the right thing.
>
> gpio_set_value(pin, 1) drives the output high same as
> gpio_direction_output(pin, 1) does (documented in Documentation/gpio.txt),
> so the gpio will be changed from open-drain to push-pull.

Normally, yes. But if the pin is configured as open-drain (which is
not covered by the GPIO API because it's highly platform-specific),
gpio_set_value(pin, 1) will turn off the output driver.

> In contrast, gpio_direction_input(pin) will turn off the output driver.

Yes, but it's (usually) much more expensive.

> Open-drain io is bound to "!is_open_drain" and push-pull io is bound to
> "is_open_drain", so I still think this should be the other way round.

It is never correct to use push-pull I/O for i2c, so the flag does not
specify the desired behavior of the driver, it specifies what the
hardware has been configured to do so that the driver can choose the
cheapest way to do open-drain I/O.

And even if you could argue that the flag should be inverted, it has
had the same meaning since the driver was introduced several years
ago, so changing it now will break every single platform which
currently uses i2c-gpio.

Havard

[Voss, Nikolaus]

> It is never correct to use push-pull I/O for i2c, so the flag does not
> specify the desired behavior of the driver, it specifies what the
> hardware has been configured to do so that the driver can choose the
> cheapest way to do open-drain I/O.

Ok, seems rather logical after all.

>
> And even if you could argue that the flag should be inverted, it has
> had the same meaning since the driver was introduced several years
> ago, so changing it now will break every single platform which
> currently uses i2c-gpio.

I completely agree.

Thanks for the explanation,
Niko

[H Hartley Sweeten]

On Tuesday, November 01, 2011 1:02 AM, Voss, Nikolaus wrote:
>> It is never correct to use push-pull I/O for i2c, so the flag does not
>> specify the desired behavior of the driver, it specifies what the
>> hardware has been configured to do so that the driver can choose the
>> cheapest way to do open-drain I/O.
>
> Ok, seems rather logical after all.
>
>>
>> And even if you could argue that the flag should be inverted, it has
>> had the same meaning since the driver was introduced several years
>> ago, so changing it now will break every single platform which
>> currently uses i2c-gpio.
>
> I completely agree.
>

Just for the record...

The GPIO pins used for I2C on the ep93xx platform can be configured as
open-drain I/O pins. When they are, and external pull-ups are applied
to the signals, the I2C bus works correctly when the "is_open_drain"
flags are set. If the pull-ups are missing the bus does not work.
If the "is_open_drain" flags are not set, the I2C bus works correctly
regardless of the existence of the pull-ups.

Based on that I think the i2c-gpio driver's use of the "is_open_drain"
flags is correct.

Regards,
Hartley

[Voss, Nikolaus]

> If the "is_open_drain" flags are not set, the I2C bus works correctly
> regardless of the existence of the pull-ups.

No, I2C can only work with anything pulling up SDA weakly, usually an
external pullup. This is independent of the "is_open_drain" property
which only describes how the driver internally accomplishes the high
impedance state.

SCL can be driven push-pull ("scl_is_output_only") but will not be
I2C-compliant any more (no clock stretching), and only clients not relying
on that feature will work.

Niko