[i2c] [patch 4/5] i2c board_info and i2c_new_device()
Jean Delvare
khali at linux-fr.org
Sat Mar 3 22:37:35 CET 2007
Hi David,
On Thu, 15 Feb 2007 00:46:10 -0800, David Brownell wrote:
> This provides partial support for new-style I2C driver binding, which is
> most useful on systems where SMBUS_QUICK probing is inadequate; that
> includes many embedded Linux systems.
As underlined in my previous post, this is useful for all chips which
cannot easily be probed and auto-configured in general, rather than
just on busses where zero-length messages aren't supported.
>
> - Define i2c_board_info and associated declaration calls, for mainboard
> specific code to declare what I2C devices are present and how they are
> configured.
>
> - Those declaration calls are statically linked (code in the init section)
> no matter how I2C is configured (a change in the build infrastructure).
> That exposes a new private data interface to the i2c-core component.
>
> - Define a new public function i2c_new_device(), so non-mainboard code
> (like a support module for a daughtercard) can declare board_info for
> I2C devices found there.
>
> - Export the previously-private i2c_unregister_device().
>
> In addition to a device's existence, addressing, and preferred driver,
> the board-specific information includes an IRQ (e.g. for an RTC alarm)
> and a platform_data pointer.
>
> Pending later patches using these new APIs, this is effectively a NOP.
>
> Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
>
> ---
> drivers/Makefile | 2 -
> drivers/i2c/Kconfig | 5 +++
> drivers/i2c/Makefile | 1
> drivers/i2c/i2c-boardinfo.c | 64 ++++++++++++++++++++++++++++++++++++++++
> drivers/i2c/i2c-core.c | 69 +++++++++++++++++++++++++++++++++++++++++++-
> drivers/i2c/i2c-core.h | 13 ++++++++
> include/linux/i2c.h | 62 +++++++++++++++++++++++++++++++++++++++
> 7 files changed, 214 insertions(+), 2 deletions(-)
>
>
> Index: i2c/include/linux/i2c.h
> ===================================================================
> --- i2c.orig/include/linux/i2c.h 2007-02-15 00:00:04.000000000 -0800
> +++ i2c/include/linux/i2c.h 2007-02-15 00:00:08.000000000 -0800
> @@ -163,6 +163,7 @@ struct i2c_client {
> int usage_count; /* How many accesses currently */
> /* to the client */
> struct device dev; /* the device structure */
> + int irq; /* irq issued by device (or -1) */
> struct list_head list;
> char name[I2C_NAME_SIZE];
> struct completion released;
What makes the irq more important than any other device-specific
attribute? Given that the i2c core itself doesn't make any use of the
irq, shouldn't it be stored in the specific client data structure?
I am asking because in my experience most I2C devices don't use an IRQ,
so I am reluctant to increase the structure size for something which
only some devices will use.
> @@ -184,6 +185,67 @@ static inline void i2c_set_clientdata (s
> dev_set_drvdata (&dev->dev, data);
> }
>
> +/**
> + * struct i2c_board_info - template for device creation
> + * @driver: identifies the driver to be bound to the device
> + * @dev_addr: stored in i2c_client.addr
> + * @bus_num: usually matches i2c_client.adapter->nr
> + * @platform_data: stored in i2c_client.dev.platform_data
> + * @irq: stored in i2c_client.irq
> +
> + * I2C doesn't actually support hardware probing, although controllers and
> + * devices may be able to use I2C_SMBUS_QUICK to tell whether or not there's
> + * a device at a given address. Drivers commonly need more information than
> + * that, such as chip type, configuration, associated IRQ, and so on.
> + *
> + * i2c_board_info is used to build tables of information listing I2C devices
> + * that are present. This information is used to grow the driver model tree
> + * for "new style" I2C drivers. For mainboards this is done statically using
> + * i2c_register_board_info(), where @bus_num represents an adapter that isn't
> + * yet available. For add-on boards, i2c_new_device() does this dynamically
> + * with the adapter already known.
> + */
> +struct i2c_board_info {
> + char driver[KOBJ_NAME_LEN];
I would add another string for the device type, but I guess you'll
argue that it belongs to the platform_data structure?
> + short bus_num;
I don't like this. This assumes you know in advance the numbers that
will be assigned to at least some of the busses. This is true only in a
limited number of cases, and this is quite fragile IMHO. This design
decision causes the whole mechanism to be only usable in this limited
number of cases - typically embedded systems. Multimedia adapters won't
be able to use it for example.
I don't like the idea that we're introducing a new API which will only
work in some cases. This is exactly what you are blaming the current
API for - that it was too hardware-monitoring centric. I don't want to
reproduce the same mistake. More about that below.
> + unsigned short dev_addr;
> + void *platform_data;
> + int irq;
I believe the irq should be just one member of the platform_data
structure for devices which need that.
> +};
> +
> +/**
> + * I2C_BOARD_INFO - macro used to list an i2c device and its driver
> + * @driver_name: identifies the driver to use with the device
> + * @busnum: numbers the bus to which the device is connected; ignored
> + * when i2c_new_device() explicitly idenfies that bus.
> + * @devaddr: the device's address on the bus.
> + *
> + * This macro initializes mandatory fields of a struct i2c_board_info,
> + * declaring what has been provided on a particular board. Optional
> + * fields (such as the associated irq, or device-specific platform_data)
> + * are provided using conventional syntax.
> + */
> +#define I2C_BOARD_INFO(driver_name,busnum,devaddr) \
> + .driver = driver_name, .bus_num = busnum, .dev_addr = devaddr
This macro doesn't look terribly useful. It's not like you'll have
dozens of chips to declare on each bus, and it doesn't save that much.
> +
> +
> +/* Add-on boards should register/unregister their devices; e.g. a board
> + * with integrated I2C, a config eeprom, sensors, and a codec that's
> + * used in conjunction with the primary hardware.
> + */
> +extern struct i2c_client *
> +i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info);
> +
> +extern void i2c_unregister_device(struct i2c_client *);
> +
> +/* Mainboard arch_initcall() code should register all its I2C devices.
> + * This is done at arch_initcall time, before declaring any i2c adapters.
> + * Modules for add-on boards must use other calls.
> + */
> +extern int
> +i2c_register_board_info(struct i2c_board_info const *info, unsigned n);
> +
> +
> /*
> * The following structs are for those who like to implement new bus drivers:
> * i2c_algorithm is the interface to a class of hardware solutions which can
> Index: i2c/drivers/Makefile
> ===================================================================
> --- i2c.orig/drivers/Makefile 2007-02-14 23:59:23.000000000 -0800
> +++ i2c/drivers/Makefile 2007-02-15 00:00:08.000000000 -0800
> @@ -58,7 +58,7 @@ obj-$(CONFIG_GAMEPORT) += input/gamepor
> obj-$(CONFIG_INPUT) += input/
> obj-$(CONFIG_I2O) += message/
> obj-$(CONFIG_RTC_LIB) += rtc/
> -obj-$(CONFIG_I2C) += i2c/
> +obj-y += i2c/
> obj-$(CONFIG_W1) += w1/
> obj-$(CONFIG_HWMON) += hwmon/
> obj-$(CONFIG_PHONE) += telephony/
> Index: i2c/drivers/i2c/Kconfig
> ===================================================================
> --- i2c.orig/drivers/i2c/Kconfig 2007-02-14 23:59:23.000000000 -0800
> +++ i2c/drivers/i2c/Kconfig 2007-02-15 00:00:08.000000000 -0800
> @@ -22,6 +22,11 @@ config I2C
> This I2C support can also be built as a module. If so, the module
> will be called i2c-core.
>
> +config I2C_BOARDINFO
> + boolean
> + depends on I2C
> + default y
> +
> config I2C_CHARDEV
> tristate "I2C device interface"
> depends on I2C
> Index: i2c/drivers/i2c/Makefile
> ===================================================================
> --- i2c.orig/drivers/i2c/Makefile 2007-02-14 23:59:23.000000000 -0800
> +++ i2c/drivers/i2c/Makefile 2007-02-15 00:00:08.000000000 -0800
> @@ -2,6 +2,7 @@
> # Makefile for the i2c core.
> #
>
> +obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o
> obj-$(CONFIG_I2C) += i2c-core.o
> obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o
> obj-y += busses/ chips/ algos/
> Index: i2c/drivers/i2c/i2c-core.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ i2c/drivers/i2c/i2c-core.h 2007-02-15 00:00:08.000000000 -0800
> @@ -0,0 +1,13 @@
> +struct boardinfo {
> + struct list_head list;
> + unsigned n_board_info;
> + struct i2c_board_info board_info[0];
> +};
I don't like having an i2c-specific structure with a name as generic as
"boardinfo".
> +
> +/* board_lock protects board_list and first_dynamic_bus_num.
> + * nobody except i2c-core is allowed to use these symbols.
> + */
> +extern struct mutex __i2c_board_lock;
> +extern struct list_head __i2c_board_list;
> +extern int __i2c_first_dynamic_bus_num;
If really nobody except i2c-core was allowed to use these symbols then
you wouldn't have to export them. i2c-mainboard too is allowed to use
them, obviously.
> +
> Index: i2c/drivers/i2c/i2c-boardinfo.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ i2c/drivers/i2c/i2c-boardinfo.c 2007-02-15 00:00:08.000000000 -0800
> @@ -0,0 +1,64 @@
> +#include <linux/kernel.h>
> +#include <linux/i2c.h>
> +
> +#include "i2c-core.h"
> +
> +
> +DEFINE_MUTEX(__i2c_board_lock);
> +LIST_HEAD(__i2c_board_list);
> +int __i2c_first_dynamic_bus_num;
> +
> +/* These symbols are exported ONLY FOR i2c-core.
> + * No other users will be supported.
> + */
> +EXPORT_SYMBOL_GPL(__i2c_board_lock);
> +EXPORT_SYMBOL_GPL(__i2c_board_list);
> +EXPORT_SYMBOL_GPL(__i2c_first_dynamic_bus_num);
> +
> +/**
> + * i2c_register_board_info - statically declare I2C devices
> + * @info: vector of i2c device descriptors
> + * @len: how many descriptors in the vector
> + *
> + * Systems using the Linux I2C driver stack can declare tables of board info
> + * while they initialize. This should be done in board-specific init code
> + * near arch_initcall() time, or equivalent, before any I2C adapter drivers are
> + * registered. For example, mainboard init code could define several devices,
> + * as could the init code for each daughtercard in a board stack.
> + *
> + * The I2C devices will be created later, after the adapter for the relevant bus
> + * has been registered. After that moment, standard driver model tools are used
> + * to bind "new style" I2C drivers to the devices. The bus number for any device
> + * declared using this routine is not available for dynamic allocation.
> + *
> + * The board info passed can safely be __initdata, but be careful of embedded
> + * pointers (for platform_data, functions, etc) since that won't be copied.
> + */
> +int __init
> +i2c_register_board_info(struct i2c_board_info const *info, unsigned len)
> +{
> + struct boardinfo *bi;
No two-letter variables please, they are a nightmare to grep for.
> + int i;
> +
> + if (len == 0)
> + return 0;
> +
> + bi = kmalloc(sizeof(*bi) + len * sizeof *info, GFP_KERNEL);
I don't like this kind of construct at all, because they are very
fragile. You assume that alignment will be OK, which might be true
right now, but might no longer be if the structures are modified at
some later point. And this is very architecture-dependent too.
And please always add the parentheses to your sizeof calls, for
consistency.
> + if (!bi)
> + return -ENOMEM;
> + bi->n_board_info = len;
> + memcpy(bi->board_info, info, len * sizeof *info);
> +
> + mutex_lock(&__i2c_board_lock);
> +
> + /* dynamic bus numbers will be assigned after the last static one */
> + for (i = 0; i < len; i++, info++) {
> + if (info->bus_num >= __i2c_first_dynamic_bus_num)
> + __i2c_first_dynamic_bus_num = info->bus_num + 1;
> + }
> +
> + list_add_tail(&bi->list, &__i2c_board_list);
> + mutex_unlock(&__i2c_board_lock);
> +
> + return 0;
> +}
> Index: i2c/drivers/i2c/i2c-core.c
> ===================================================================
> --- i2c.orig/drivers/i2c/i2c-core.c 2007-02-15 00:00:04.000000000 -0800
> +++ i2c/drivers/i2c/i2c-core.c 2007-02-15 00:00:08.000000000 -0800
> @@ -35,6 +35,8 @@
> #include <linux/completion.h>
> #include <asm/uaccess.h>
>
> +#include "i2c-core.h"
> +
>
> static LIST_HEAD(adapters);
> static DEFINE_MUTEX(core_lists);
> @@ -180,7 +182,53 @@ struct bus_type i2c_bus_type = {
> .resume = i2c_device_resume,
> };
>
> -static void i2c_unregister_device(struct i2c_client *client)
> +/**
> + * i2c_new_device - instantiate an i2c device for use with a new style driver
> + * @adap: the adapter managing the device
> + * @info: describes one I2C device
> + *
> + * Create a device to work with a new style i2c driver, where binding is
> + * handled through driver model probe()/remove() methods.
> + *
> + * This returns the new i2c client, which may be saved for later use with
> + * i2c_unregister_device(); or NULL to indicate an error.
> + */
> +struct i2c_client *
> +i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> +{
> + struct i2c_client *client;
> + int status;
> +
> + client = kzalloc(sizeof *client, GFP_KERNEL);
> + if (!client)
> + return NULL;
> +
> + client->adapter = adap;
> +
> + strlcpy(client->name, info->driver, sizeof client->name);
> + client->dev.platform_data = info->platform_data;
> + client->addr = info->dev_addr;
> + client->irq = info->irq;
> +
> + /* a new style driver may be bound to this device when we
> + * return from this function, or any later moment (e.g. maybe
> + * hotplugging will load the driver module).
> + */
> + status = i2c_attach_client(client);
> + if (status < 0) {
> + kfree(client);
> + client = NULL;
> + }
> + return client;
> +}
> +EXPORT_SYMBOL_GPL(i2c_new_device);
> +
> +
> +/**
> + * i2c_unregister_device - reverse effect of i2c_new_device()
> + * @client: value returned from i2c_new_device()
> + */
> +void i2c_unregister_device(struct i2c_client *client)
> {
> struct i2c_adapter *adap = client->adapter;
> struct i2c_driver *driver = client->driver;
> @@ -215,6 +263,7 @@ static void i2c_unregister_device(struct
> put_device(&client->dev);
>
> }
> +EXPORT_SYMBOL_GPL(i2c_unregister_device);
All the other exports are at the end of i2c-core.c, so for the sake of
consistency please do the same for the new ones.
>
>
> /* ------------------------------------------------------------------------- */
> @@ -325,6 +374,24 @@ out_list:
> goto out_unlock;
> }
>
> +static void i2c_scan_static_board_info(struct i2c_adapter *adap)
> +{
> + struct boardinfo *bi;
> +
> + mutex_lock(&__i2c_board_lock);
> + list_for_each_entry(bi, &__i2c_board_list, list) {
> + struct i2c_board_info *chip = bi->board_info;
> + unsigned n;
> +
> + for (n = bi->n_board_info; n > 0; n--, chip++) {
> + if (chip->bus_num != adap->nr)
> + continue;
> + (void) i2c_new_device(adap, chip);
> + }
> + }
> + mutex_unlock(&__i2c_board_lock);
> +}
This looks quite inefficient. I really don't buy the whole logic. First
you declare the devices using bus numbers, then you group all the
devices regardless of the bus number, then you create the busses and
they receive their number, and finally you must check all the devices
against all the adapters to find the matches.
I really fail to see why you need all this complexity (list, mutex,
concept of static i2c busses vs. dynamically allocated ones, separate
source file for the device declaration function...) I would prefer
something less embedded-centric, that would work for everyone, without
introducing new concepts and files.
What is wrong with this more simple approach:
i2c_adapter gets an extra optional member, a pointer to an array of
struct i2c_board_info. As each struct i2c_board_info is attached to a
given i2c_adapter, we don't need the bus_num field, and we also don't
need to test for matching bus numbers afterwards.
When an i2c_adapter is registered (in i2c_add_adapter), i2c-core looks
at the associated i2c_board_info data and calls i2c_new_device for all
entries. Likewise, when an i2c_adapter is unregistered, it first calls
i2c_unregister_device for all the device listed in the i2c_board_info
array.
There must be some reason why you came up with something much more
complex than that, while "my" proposal (actually I believe the original
proposal by Nathan Lutchansky was pretty much the same) only needs to
introduce one new structure (i2c_board_info) and one new function
(i2c_new_device), and will work with multimedia adapters too. I guess
I'm missing something, but I can't see what. Care to enlighten me?
Thanks,
--
Jean Delvare
More information about the i2c
mailing list