Re: Registering I2C devices on X86

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

Hi Richard,

Sorry for the late reply, your post slipped through the cracks.

On Thu, 03 Jun 2010 14:39:08 +0200, Richard Röjfors wrote:
> On 06/03/2010 08:21 AM, Jean Delvare wrote:
> > On Wed, 02 Jun 2010 13:29:36 +0200, Richard Röjfors wrote:
> >> The "problem" I see is that the CPU and the chipset + I2C chip will be populated on several
> >> different boards. It would mean that the I2C bus driver would need knowledge of all the boards where
> >> it is used. And the bus driver itself can not really detect which board it's running on.
> >>
> >> That is why I kept the I2C device setup in a separate driver. So each board would have a separate
> >> "setup" driver. Isn't that the most clean solution right now?
> >
> > I have to admit I don't clearly understand what you are doing. It
> > should become clearer when I see your code.
> 
> The I2C bus driver used is a standard one and the setup of I2C devices depends on the actual board 
> the chips are deployed on.
> 
> I do stuff like this, example of a driver for one specific board (there are more I2C devices to come):
> 
> #include <linux/gpio.h>
> #include <linux/i2c.h>
> #include <linux/i2c/tsc2007.h>
> 
> #define DRIVER_NAME "the_module"
> 
> static int	i2c_bus = 0;
> static unsigned	tsc2007_irq_pin = 102;
> 
> static __devinitdata struct tsc2007_platform_data tsc2007_platform_data = {
> 	.model = 2007,
> 	.x_plate_ohms = 200
> };
> 
> static __initdata struct i2c_board_info tsc2007_i2c_board_info = {
> 	I2C_BOARD_INFO("tsc2007", 0x48),
> 	.platform_data = &tsc2007_platform_data,
> 	/* irq to be filled in runtime */
> };
> 
> static struct i2c_client *tsc2007_client;
> 
> static __init int the_module_get_irq(unsigned gpio_pin)
> {
> 	int err;
> 
> 	err = gpio_request(gpio_pin, DRIVER_NAME);
> 	if (err)
> 		return err;
> 
> 	err = gpio_direction_input(gpio_pin);
> 	if (err)
> 		goto err;
> 
> 	err = gpio_to_irq(gpio_pin);
> 	if (err < 0)
> 		goto err;
> 
> 	return err;
> err:
> 	gpio_free(tsc2007_irq_pin);
> 	return err;
> }
> 
> static __init int the_module_init(void)
> {
> 	struct i2c_adapter *adapt;
> 	int err;
> 
> 	adapt = i2c_get_adapter(i2c_bus);

How do you know the value of i2c_bus? This is the key problem. If you
do not declare your I2C chips at the platform data level, there are no
I2C bus numbers reserved for static numbering. And even if you managed
to change that, the I2C adapters on x86 do not ask for a specific bus
number: they pick the first one available.

Maybe it happens to work for you right now, but this is very fragile.
I2C bus number 0 could become something completely different (for
example a DDC channel on the graphics adapter) at any time, depending
on which drivers are included in your kernel, the order in which they
are loaded/linked, and which exact hardware you run on.

If you want to take this approach, you need to extend the current API
first. You need a way to reserve static I2C bus numbers without
declaring devices on them. And you need to change the bus drivers (e.g.
i2c-i801.c) to request this specific bus number under specific
circumstances. So you won't avoid platform/machine specific code in the
I2C bus
driver.

The above is certainly doable, but if you need to do that kind of
thing, then it's probably time to rethink the whole thing. If x86
starts being used for embedded-style applications, then the
platform/machine handling should be adjusted. I see no reason why the
same approach that works for ARM platforms wouldn't work for x86
platforms as well.


> 	if (!adapt) {
> 		printk(KERN_ERR "%s: Failed to get I2C adapter\n", __func__);
> 		return -ENODEV;
> 	}
> 
> 	err = the_module_get_irq(tsc2007_irq_pin);
> 	if (err < 0)
> 		goto put_adapter;
> 	tsc2007_i2c_board_info.irq = err;
> 
> 	/* add in the devices on the bus */
> 	tsc2007_client = i2c_new_device(adapt, &tsc2007_i2c_board_info);
> 	if (!tsc2007_client)
> 		goto free_tsc2007_pin;
> 
> 	i2c_put_adapter(adapt);
> 
> 	return 0;
> 
> free_tsc2007_pin:
> 	gpio_free(tsc2007_irq_pin);
> put_adapter:
> 	i2c_put_adapter(adapt);
> 
> 	return err;
> }
> 
> static void __exit the_module_exit(void)
> {
> 	i2c_unregister_device(tsc2007_client);
> }


-- 
Jean Delvare