svn commit: r363382 - head/sys/dev/gpio

[Andriy Gapon]

On 21/07/2020 10:35, Andriy Gapon wrote:
> Author: avg
> Date: Tue Jul 21 07:35:03 2020
> New Revision: 363382
> URL: https://svnweb.freebsd.org/changeset/base/363382
> 
> Log:
>   gpioiic: never drive lines active high
>   
>   I2C communication is done by a combination of driving a line low or
>   letting it float, so that it is either pulled up or driven low by
>   another party.
>   
>   r355276 besides the stated goal of the change -- using the new GPIO API
>   -- also changed the logic, so that active state is signaled by actively
>   driving a line.

Actually, the code was not incorrect.
Ian pointed out something that I overlooked.
The driver configures I2C pins as GPIO_PIN_OPENDRAIN and that alone should have
ensured the correct behavior of setting active and inactive output states (that
is, active == hi-Z).
The real problem is that only a few drivers implement or emulate that
configuration bit.  Far from all hardware provides that option natively too.

>   That worked with iicbb prior to r362042, but stopped working after that
>   commit on at least some hardware.  My guess that the breakage was
>   related to getting an ACK bit.  A device expected to be able to drive
>   SDA actively low, but controller was actively driving it high for some
>   time.
>   
>   Anyway, this change seems to fix the problem.
>   Tested using gpioiic on Orange Pi PC Plus with HTU21 sensor.
>   
>   Reported by:	Nick Kostirya <nikolay.kostirya at i11.co>
>   Reviewed by:	manu
>   MFC after:	1 week
>   Differential Revision: https://reviews.freebsd.org/D25684
> 
> Modified:
>   head/sys/dev/gpio/gpioiic.c
> 
> Modified: head/sys/dev/gpio/gpioiic.c
> ==============================================================================
> --- head/sys/dev/gpio/gpioiic.c	Mon Jul 20 23:57:53 2020	(r363381)
> +++ head/sys/dev/gpio/gpioiic.c	Tue Jul 21 07:35:03 2020	(r363382)
> @@ -191,16 +191,14 @@ static void
>  gpioiic_setsda(device_t dev, int val)
>  {
>  	struct gpioiic_softc *sc = device_get_softc(dev);
> -	int err;
>  
> -	/*
> -	 * Some controllers cannot set an output value while a pin is in input
> -	 * mode; in that case we set the pin again after changing mode.
> -	 */
> -	err = gpio_pin_set_active(sc->sdapin, val);
> -	gpio_pin_setflags(sc->sdapin, GPIO_PIN_OUTPUT | GPIO_PIN_OPENDRAIN);
> -	if (err != 0)
> -		gpio_pin_set_active(sc->sdapin, val);
> +	if (val) {
> +		gpio_pin_setflags(sc->sdapin, GPIO_PIN_INPUT);
> +	} else {
> +		gpio_pin_setflags(sc->sdapin,
> +		    GPIO_PIN_OUTPUT | GPIO_PIN_OPENDRAIN);
> +		gpio_pin_set_active(sc->sdapin, 0);
> +	}
>  }
>  
>  static void
> @@ -208,8 +206,13 @@ gpioiic_setscl(device_t dev, int val)
>  {
>  	struct gpioiic_softc *sc = device_get_softc(dev);
>  
> -	gpio_pin_setflags(sc->sclpin, GPIO_PIN_OUTPUT | GPIO_PIN_OPENDRAIN);
> -	gpio_pin_set_active(sc->sclpin, val);
> +	if (val) {
> +		gpio_pin_setflags(sc->sclpin, GPIO_PIN_INPUT);
> +	} else {
> +		gpio_pin_setflags(sc->sclpin,
> +		    GPIO_PIN_OUTPUT | GPIO_PIN_OPENDRAIN);
> +		gpio_pin_set_active(sc->sclpin, 0);
> +	}
>  }
>  
>  static int
> 


-- 
Andriy Gapon