[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-i2c
Subject:    Re: [PATCHv3] i2c: add generic I2C multiplexer using gpio api
From:       Peter Korsgaard <jacmet () sunsite ! dk>
Date:       2010-11-30 14:02:44
Message-ID: 87aakqykmz.fsf () macbook ! be ! 48ers ! dk
[Download RAW message or body]

>>>>> "Jean" == Jean Delvare <khali@linux-fr.org> writes:

 Jean> Hi Peter,
 Jean> Thanks for the updated patch.

You're welcome. Thanks for the feedback.


 >> SCL/SDA of the master I2C bus is multiplexed to virtual bus 1..M
 >> according to the settings of the GPIO pins 1..N.

 Jean> Again: these are not virtual buses. These are bus segments behind a
 Jean> mux, and they are as real as the trunk segment in front of the mux.

Ok, I renamed every instance of 'virtual bus' with 'bus segment'.


 Jean> I'm eager to give it a try on my own PC system, where the SMBus
 Jean> is multiplexed using ICH10 GPIO pins. Unfortunately I don't think
 Jean> there is a driver for the Intel ICH GPIO pins, so I'll have to
 Jean> write one. And then I'll have to write some glue code to
 Jean> instantiate the relevant platform device on my system. I'll let
 Jean> you know when I get there.

Nice!


 >> +static struct gpio_i2cmux_platform_data myboard_i2cmux_data = {
 >> +	.parent		= 1,
 >> +	.base_nr	= 2, /* optional */
 >> +	.values		= myboard_gpiomux_values,
 >> +	.n_values	= ARRAY_SIZE(myboard_gpiomux_values),
 >> +	.gpio		= myboard_gpiomux_gpios,

 Jean> .gpios

 >> +	.gpios		= ARRAY_SIZE(myboard_gpiomux_gpios),

 Jean> .n_gpios

Ups, fixed.


 >> +GENERIC GPIO I2C MULTIPLEXER DRIVER
 >> +M:	Peter Korsgaard <peter.korsgaard@barco.com>
 >> +L:	linux-i2c@vger.kernel.org
 >> +S:	Supported
 >> +F:	drivers/i2c/muxes/gpio-i2cmux.c
 >> +F:	include/linux/gpio-i2mux.h

 Jean> Typo: gpio-i2cmux.h. And you're missing:

 Jean> F:	Documentation/i2c/muxes/gpio-i2cmux

Fixed.


 >> + config I2C_MUX_GPIO
 >> +	tristate "GPIO-based I2C multiplexer"
 >> +	depends on GENERIC_GPIO
 >> +	help
 >> +	  If you say yes to this option, support will be included for a
 >> +	  GPIO based I2C multiplexer. This driver provides access to
 >> +	  I2C busses connected through a MUX, which is controlled
 >> +	  through GPIO pins.
 >> +
 >> +	  This driver can also be built as a module.  If so, the module
 >> +	  will be called gpio-i2cmux.
 >> +

 Jean> Alphabetic order -> goes at the beginning of the list.

 >> +obj-$(CONFIG_I2C_MUX_GPIO)	+= gpio-i2cmux.o

 Jean> Alphabetic order -> goes at the beginning of the list.

Fixed.


 >> +	for (i = 0; i < pdata->n_values; i++) {
 >> +		u32 nr = pdata->base_nr ? (pdata->base_nr + i) : 0;
 >> +		bool has_idle = (pdata->idle != (unsigned)-1);

 Jean> has_idle doesn't depend on i, so it would be more efficiently set
 Jean> outside the loop. You could even store a function pointer instead of a
 Jean> boolean value, to be even more efficient.

 Jean> BTW, would it make sense to

 Jean> #define GPIO_I2CMUX_NO_IDLE	((unsigned)-1)

 Jean> to avoid arbitrary cast and constant in the code?

Ok, added define to platform header, and changed the probe code to use
a function pointer instead.


Will send v4 shortly - Thanks for the review.

-- 
Bye, Peter Korsgaard
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]