Hi Peter,

On Tue, 30 Nov 2010 15:03:08 +0100, Peter Korsgaard wrote:
> Add an i2c mux driver providing access to i2c bus segments using a
> hardware MUX sitting on a master bus and controlled through gpio pins.
> 
> E.G. something like:
> 
>   ----------              ----------  Bus segment 1   - - - - -
>  |          | SCL/SDA    |          |-------------- |           |
>  |          |------------|          |
>  |          |            |          | Bus segment 2 |           |
>  |  Linux   | GPIO 1..N  |   MUX    |---------------   Devices
>  |          |------------|          |               |           |
>  |          |            |          | Bus segment M
>  |          |            |          |---------------|           |
>   ----------              ----------                  - - - - -
> 
> SCL/SDA of the master I2C bus is multiplexed to bus segment 1..M
> according to the settings of the GPIO pins 1..N.

I've fixed two remaining minor things myself, and your patch is ready
to be applied now, except for one issue I just noticed. See below.

> (..)
> --- /dev/null
> +++ b/Documentation/i2c/muxes/gpio-i2cmux
> (...)
> +Usage
> +-----
> +
> +gpio-i2cmux uses the platform bus, so you need to provide a struct
> +platform_device with the platform_data pointing to a struct
> +gpio_i2cmux_platform_data with the I2C adapter number of the master
> +bus, the number bus segments to create and the GPIO pins used

"of" got lost in the battle, I've added it back.

> +to control it. See include/linux/gpio-i2cmux.h for details.

> (...)
> --- /dev/null
> +++ b/drivers/i2c/muxes/gpio-i2cmux.c
> (...)
> +     for (i = 0; i < pdata->n_gpios; i++) {
> +             ret = gpio_request(pdata->gpios[i], "gpio-i2cmux");
> +             if (ret)
> +                     goto err_request_gpio;
> +             gpio_direction_output(pdata->gpios[i], pdata->idle & (1 << i));

This looks wrong if pdata->idle == GPIO_I2CMUX_NO_IDLE. I think we want
something along the lines of:

        unsigned initial_state;

        initial_state = pdata->idle == GPIO_I2CMUX_NO_IDLE ?
                        pdata->values[0] : pdata->idle;

                (...)
                gpio_direction_output(pdata->gpios[i], initial_state & (1 << 
i));

What do you think? An alternative is to leave the direction
uninitialized and hope it's already OK, but I'm not sure how realistic
this is.

> +     }
> +
> +

Double blank line, I've removed one.

> +     if (pdata->idle != GPIO_I2CMUX_NO_IDLE)
> +             deselect = gpiomux_deselect;

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to