Re: [i2c] [PATCH] i2c driver for Maxim max9485 audio clock generator chip
What you want me to do? Put in a bare .h file for the part? Bare .h files are much harder to find and reuse. It is also going to force anyone reusing this part into reading the datasheet and determining how the part works. On Wed, Oct 22, 2008 at 2:38 AM, Jean Delvare [EMAIL PROTECTED] wrote: On Tue, 21 Oct 2008 19:57:09 -0400, Jon Smirl wrote: On Tue, Oct 21, 2008 at 4:36 AM, Jean Delvare [EMAIL PROTECTED] wrote: Honestly I don't see any value in this driver. There's nothing you can do with it that you couldn't already do without it. I need a driver so that my device tree will bind and tell me the i2c address of the device. This is rubbish. It's perfectly fine to let the I2C device be instantiated from the device tree without having a driver that binds to it. The device tree and the instantiated device, not the driver, tell you the I2C address of the device. We may also build a device in the future with two of these chips. I fail to see how it matters. -- Jean Delvare -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] i2c driver for Maxim max9485 audio clock generator chip
On Tue, Oct 21, 2008 at 4:36 AM, Jean Delvare [EMAIL PROTECTED] wrote: Honestly I don't see any value in this driver. There's nothing you can do with it that you couldn't already do without it. I need a driver so that my device tree will bind and tell me the i2c address of the device. We may also build a device in the future with two of these chips. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] [V2 PATCH] i2c driver for Maxim max9485 audio clock generator chip
Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- drivers/misc/Kconfig|9 drivers/misc/Makefile |1 drivers/misc/max9485.c | 105 +++ include/linux/i2c/max9485.h | 39 4 files changed, 154 insertions(+), 0 deletions(-) create mode 100644 drivers/misc/max9485.c create mode 100644 include/linux/i2c/max9485.h diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index efd3aa0..010baa6 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -391,6 +391,15 @@ config ATMEL_SSC If unsure, say N. +config MAX9485 + tristate Maxim MAX9485 Programmable Audio Clock Generator + help + If you say yes here you get support for Maxim MAX9485 + Programmable Audio Clock Generator. + + This driver can also be built as a module. If so, the module + will be called max9485. + config INTEL_MENLOW tristate Thermal Management driver for Intel menlow platform depends on ACPI_THERMAL diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index c6c13f6..6133434 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -24,6 +24,7 @@ obj-$(CONFIG_SONY_LAPTOP) += sony-laptop.o obj-$(CONFIG_THINKPAD_ACPI)+= thinkpad_acpi.o obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o +obj-$(CONFIG_MAX9485) += max9485.o obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o obj-$(CONFIG_KGDB_TESTS) += kgdbts.o diff --git a/drivers/misc/max9485.c b/drivers/misc/max9485.c new file mode 100644 index 000..c1316f2 --- /dev/null +++ b/drivers/misc/max9485.c @@ -0,0 +1,105 @@ +/* + * Maxim max9485 Programmable Audio Clock Generator driver + * + * Written by: Jon Smirl [EMAIL PROTECTED] + * + * Copyright (C) 2008 Digispeaker.com + * Copyright (C) 2008 Jon Smirl [EMAIL PROTECTED] + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +/* + * This device is usually under the control of ALSA and should not be changed + * from userspace. The main purpose of this driver is to locate the i2c address + * of where the chip is located. + */ + +#include linux/module.h +#include linux/i2c.h +#include linux/sysfs.h +#include linux/i2c/max9485.h + +int max9485_set(struct i2c_client *max9485, u8 value) +{ + return i2c_smbus_write_byte(max9485, value); +} +EXPORT_SYMBOL_GPL(max9485_set); + +/* + * Display the only register + */ +static ssize_t max9485_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct i2c_client *client = to_i2c_client(dev); + int rc; + + rc = i2c_smbus_read_byte(client); + if (rc 0) + return rc; + + return sprintf(buf, %s%s%s%s%s%s, + (rc MAX9485_MCLK ? MCLK+ : ), + (rc MAX9485_CLK_OUT_2 ? CLK2+ : ), + (rc MAX9485_CLK_OUT_2 ? CLK1+ : ), + (rc MAX9485_DOUBLED ? DOUBLED+ : ), + (rc MAX9485_SCALE_768 ? 768x+ : (rc MAX9485_SCALE_384 ? 384x+ : 256x+)), + ((rc 3) == MAX9485_FREQUENCY_48 ? 48Khz : + ((rc 3) == MAX9485_FREQUENCY_441 ? 44.1Khz : + ((rc 3) == MAX9485_FREQUENCY_32 ? 32Khz : 12Khz; +} +static DEVICE_ATTR(max9485, S_IRUGO, max9485_show, NULL); + +/* + * Called when a max9485 device is matched with this driver + */ +static int max9485_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + if (!i2c_check_functionality(client-adapter, I2C_FUNC_SMBUS_BYTE)) { + dev_err(client-dev, i2c bus does not support the max9485\n); + return -ENODEV; + } + return sysfs_create_file(client-dev.kobj, dev_attr_max9485.attr); +} + +static int max9485_remove(struct i2c_client *client) +{ + sysfs_remove_file(client-dev.kobj, dev_attr_max9485.attr); + return 0; +} + +static const struct i2c_device_id max9485_id[] = { + { max9485, 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, max9485_id); + +static struct i2c_driver max9485_driver = { + .driver = { + .name = max9485, + }, + .probe = max9485_probe, + .remove = max9485_remove, + .id_table = max9485_id, +}; + +static int __init max9485_init(void) +{ + return i2c_add_driver(max9485_driver); +} + +static void __exit max9485_exit(void) +{ + i2c_del_driver(max9485_driver); +} + +MODULE_AUTHOR(Jon Smirl [EMAIL PROTECTED]); +MODULE_DESCRIPTION(Maxim MAX9485 Programmable Audio Clock Generator driver); +MODULE_LICENSE(GPL); + +module_init(max9485_init); +module_exit(max9485_exit); diff --git a/include/linux/i2c/max9485.h b/include
[i2c] [PATCH] i2c driver for Maxim max9485 audio clock generator chip
Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- drivers/i2c/chips/Kconfig |9 drivers/i2c/chips/Makefile |1 drivers/i2c/chips/max9485.c | 106 +++ include/linux/i2c/max9485.h | 38 +++ 4 files changed, 154 insertions(+), 0 deletions(-) create mode 100644 drivers/i2c/chips/max9485.c create mode 100644 include/linux/i2c/max9485.h diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig index 1735682..bc2a6d3 100644 --- a/drivers/i2c/chips/Kconfig +++ b/drivers/i2c/chips/Kconfig @@ -40,6 +40,15 @@ config AT24 This driver can also be built as a module. If so, the module will be called at24. +config MAX9485 + tristate Maxim MAX9485 Programmable Audio Clock Generator + help + If you say yes here you get support for Maxim MAX9485 + Programmable Audio Clock Generator. + + This driver can also be built as a module. If so, the module + will be called max9485. + config SENSORS_EEPROM tristate EEPROM reader depends on EXPERIMENTAL diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile index ca520fa..1560baf 100644 --- a/drivers/i2c/chips/Makefile +++ b/drivers/i2c/chips/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_DS1682) += ds1682.o obj-$(CONFIG_AT24) += at24.o +obj-$(CONFIG_MAX9485) += max9485.o obj-$(CONFIG_SENSORS_EEPROM) += eeprom.o obj-$(CONFIG_SENSORS_MAX6875) += max6875.o obj-$(CONFIG_SENSORS_PCA9539) += pca9539.o diff --git a/drivers/i2c/chips/max9485.c b/drivers/i2c/chips/max9485.c new file mode 100644 index 000..65058b4 --- /dev/null +++ b/drivers/i2c/chips/max9485.c @@ -0,0 +1,106 @@ +/* + * Maxim max9485 Programmable Audio Clock Generator driver + * + * Written by: Jon Smirl [EMAIL PROTECTED] + * + * Copyright (C) Digispeaker.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +/* + * This device is under the control of ALSA and can not be changed from + * userspace. The main purpose of this driver is to locate the i2c address + * of where the chip is located. + */ + +#include linux/module.h +#include linux/i2c.h +#include linux/sysfs.h +#include linux/i2c/max9485.h + +int max9485_set(struct i2c_client *max9485, u8 value) +{ + int rc; + + if (!max9485) + return -ENODEV; + + rc = i2c_smbus_write_byte(max9485, value); + if (rc 0) + return -EIO; + + return 0; +} +EXPORT_SYMBOL_GPL(max9485_set); + +/* + * Display the only register + */ +static ssize_t max9485_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct i2c_client *client = to_i2c_client(dev); + int rc; + + rc = i2c_smbus_read_byte(client); + if (rc 0) + return -EIO; + + return sprintf(buf, 0x%02X\n, rc); +} +static DEVICE_ATTR(max9485, S_IRUGO, max9485_show, NULL); + +/* + * Called when a max9485 device is matched with this driver + */ +static int max9485_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + if (!i2c_check_functionality(client-adapter, + I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE)) { + dev_err(client-dev, i2c bus does not support the max9485\n); + return -ENODEV; + } + return sysfs_create_file(client-dev.kobj, dev_attr_max9485.attr); +} + +static int max9485_remove(struct i2c_client *client) +{ + sysfs_remove_file(client-dev.kobj, dev_attr_max9485.attr); + return 0; +} + +static const struct i2c_device_id max9485_id[] = { + { max9485, 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, max9485_id); + +static struct i2c_driver max9485_driver = { + .driver = { + .name = max9485, + }, + .probe = max9485_probe, + .remove = max9485_remove, + .id_table = max9485_id, +}; + +static int __init max9485_init(void) +{ + return i2c_add_driver(max9485_driver); +} + +static void __exit max9485_exit(void) +{ + i2c_del_driver(max9485_driver); +} + +MODULE_AUTHOR(Jon Smirl [EMAIL PROTECTED]); +MODULE_DESCRIPTION(Maxim MAX9485 Programmable Audio Clock Generator driver); +MODULE_LICENSE(GPL); + +module_init(max9485_init); +module_exit(max9485_exit); diff --git a/include/linux/i2c/max9485.h b/include/linux/i2c/max9485.h new file mode 100644 index 000..0c97450 --- /dev/null +++ b/include/linux/i2c/max9485.h @@ -0,0 +1,38 @@ +/* + * Maxim max9485 Programmable Audio Clock Generator driver + * + * Written by: Jon Smirl [EMAIL PROTECTED] + * + * Copyright (C) Digispeaker.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published
[i2c] [PATCH] Add the of_find_i2c_device_by_node function
Add the of_find_i2c_device_by_node function. This allows you to follow a reference in the device tree to an i2c device node and then locate the linux device instantiated by the device tree. Example use, an i2s codec controlled by i2c. Depends on patch exporting i2c root bus symbol. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- drivers/of/of_i2c.c | 28 1 files changed, 24 insertions(+), 4 deletions(-) diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c index 6a98dc8..ba7b394 100644 --- a/drivers/of/of_i2c.c +++ b/drivers/of/of_i2c.c @@ -19,7 +19,7 @@ void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node) { - void *result; + struct i2c_client *i2c_dev; struct device_node *node; for_each_child_of_node(adap_node, node) { @@ -41,18 +41,38 @@ void of_register_i2c_devices(struct i2c_adapter *adap, info.addr = *addr; - request_module(info.type); + request_module(%s, info.type); - result = i2c_new_device(adap, info); - if (result == NULL) { + i2c_dev = i2c_new_device(adap, info); + if (i2c_dev == NULL) { printk(KERN_ERR of-i2c: Failed to load driver for %s\n, info.type); irq_dispose_mapping(info.irq); continue; } + + i2c_dev-dev.archdata.of_node = of_node_get(node); } } EXPORT_SYMBOL(of_register_i2c_devices); +static int of_dev_node_match(struct device *dev, void *data) +{ +return dev-archdata.of_node == data; +} + +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node) +{ + struct device *dev; + + dev = bus_find_device(i2c_bus_type, NULL, node, +of_dev_node_match); + if (!dev) + return NULL; + + return to_i2c_client(dev); +} +EXPORT_SYMBOL(of_find_i2c_device_by_node); + MODULE_LICENSE(GPL); diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h index bd2a870..17d5897 100644 --- a/include/linux/of_i2c.h +++ b/include/linux/of_i2c.h @@ -16,5 +16,7 @@ void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node); +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node); + #endif /* __LINUX_OF_I2C_H */ ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] Add the of_find_i2c_device_by_node function
On Sun, Oct 19, 2008 at 5:20 PM, Anton Vorontsov [EMAIL PROTECTED] wrote: Hi Jon, On Sun, Oct 19, 2008 at 10:00:40AM -0400, Jon Smirl wrote: Add the of_find_i2c_device_by_node function. This allows you to follow a reference in the device tree to an i2c device node and then locate the linux device instantiated by the device tree. Example use, an i2s codec controlled by i2c. Depends on patch exporting i2c root bus symbol. Signed-off-by: Jon Smirl [EMAIL PROTECTED] Few comments are below. --- drivers/of/of_i2c.c | 28 1 files changed, 24 insertions(+), 4 deletions(-) diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c index 6a98dc8..ba7b394 100644 --- a/drivers/of/of_i2c.c +++ b/drivers/of/of_i2c.c @@ -19,7 +19,7 @@ void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node) { - void *result; + struct i2c_client *i2c_dev; struct device_node *node; for_each_child_of_node(adap_node, node) { @@ -41,18 +41,38 @@ void of_register_i2c_devices(struct i2c_adapter *adap, info.addr = *addr; - request_module(info.type); + request_module(%s, info.type); Patch description doesn't mention this change. Patches for this have been posted before by other people and they aren't making it in. This is the original mail http://lkml.org/lkml/2008/6/13/290 http://lkml.org/lkml/2008/6/12/8 I can't find the ones patching i2c. - result = i2c_new_device(adap, info); - if (result == NULL) { + i2c_dev = i2c_new_device(adap, info); + if (i2c_dev == NULL) { printk(KERN_ERR of-i2c: Failed to load driver for %s\n, info.type); irq_dispose_mapping(info.irq); continue; } + + i2c_dev-dev.archdata.of_node = of_node_get(node); Would break sparc build. Plus setting this after i2c_new_device() isn't right... Recently I sent few patches to deal with the archdata, could you please rebase your patch against these three patches? http://lkml.org/lkml/2008/10/16/250 http://lkml.org/lkml/2008/10/16/251 http://lkml.org/lkml/2008/10/16/252 } } EXPORT_SYMBOL(of_register_i2c_devices); +static int of_dev_node_match(struct device *dev, void *data) +{ +return dev-archdata.of_node == data; +} + +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node) This should be documented. Especially the fact that every time you call this function, you must call device_put() when you're done with the returned i2c_client. +{ + struct device *dev; + + dev = bus_find_device(i2c_bus_type, NULL, node, + of_dev_node_match); + if (!dev) + return NULL; + + return to_i2c_client(dev); +} +EXPORT_SYMBOL(of_find_i2c_device_by_node); + MODULE_LICENSE(GPL); diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h index bd2a870..17d5897 100644 --- a/include/linux/of_i2c.h +++ b/include/linux/of_i2c.h @@ -16,5 +16,7 @@ void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node); +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node); + #endif /* __LINUX_OF_I2C_H */ Thanks, -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM
On 8/28/08, David Brownell [EMAIL PROTECTED] wrote: On Wednesday 27 August 2008, Jon Smirl wrote: On 8/27/08, David Brownell [EMAIL PROTECTED] wrote: On Tuesday 26 August 2008, Trent Piepho wrote: Lots of tv cards have eeproms with configuration information. They use an I2C driver called tveeprom that exports this: int tveeprom_read(struct i2c_client *c, unsigned char *eedata, int len) That presumes the driver somehow has access to that I2C client. How about using bus_find_device? PowerPC uses it to locate the i2c device using pointers in the device tree. Still making too many assumptions for my taste ... like using I2C, and in this case also OF. What I want is an approach that can work with all the drivers with EEPROM and NVRAM support that I've happenened on so far: drivers/i2c/chips/at24.c drivers/spi/chips/at25.c drivers/rtc/rtc-cmos.c drivers/rtc/rtc-ds1305.c drivers/rtc/rtc-ds1307.c drivers/rtc/rtc-ds1511.c drivers/rtc/rtc-ds1553.c drivers/rtc/rtc-ds1742.c drivers/rtc/rtc-m48t59.c drivers/rtc/rtc-stk17ta8.c All those could easily export a (renamed) struct at24_iface * so their persistent (and updatable) memory could be exported to kernel code using the very same (simple) interface. Discussion about how to use a different interface than what's shown in the $SUBJECT patch seems to want to promote (a) each of those ten drivers having a *different* interface exposed by EXPORT_SYMBOL(), or else don't support in-kernel access at all, and (b) a bunch of extra glue code to support it, like the OF-specific bits you showed below and then going to the code that knows to use those bits to get which device, and how to ask those devices for their data. Moreover, IMO the basic question is still whether there is a good reason not to build on the $SUBJECT patch. (So far: no.) Sure it *could* be done differently -- especially if think (a) is good but being able to reuse interfaces is bad -- but it's like spending so much time choosing what color to paint the bikeshed that the bikeshed itself never gets built... There are two parts to the puzzle. 1) A common way to export the interface. That can be addressed by applying the technique in the initial patch. 2) How do the drivers find each other. Device trees already have a way for drivers to find each other. This is an example of a audio codec that lives on both the i2c and i2s bus. The codec-handle is used to retrieve the tas0 node pointer. of_find_i2c_device_by_node then searches the archdata for that pointer. struct i2c_client *of_find_i2c_device_by_node(struct device_node *node) could be generalized by having it search multiple buses for the device. dev-archdata.of_node is just a cookie, it is only used to match against. The pointer could be a void*. I didn't like setup call part of the proposal. It is building another way for drivers to find each other. How can you generate an equivalent to the archdata.of_node cookie for other platforms? Another problem with the setup callback, how do you tell identical devices apart? [EMAIL PROTECTED] { /* PSC2 in i2s mode */ compatible = fsl,mpc5200b-psc-i2s,fsl,mpc5200-psc-i2s; cell-index = 1; reg = 0x2200 0x100; interrupts = 0x2 0x2 0x0; interrupt-parent = mpc5200_pic; codec-handle = tas0; }; [EMAIL PROTECTED] { #address-cells = 1; #size-cells = 0; compatible = fsl,mpc5200b-i2c,fsl,mpc5200-i2c,fsl-i2c; cell-index = 0; reg = 0x3d00 0x40; interrupts = 0x2 0xf 0x0; interrupt-parent = mpc5200_pic; fsl5200-clocking; tas0:[EMAIL PROTECTED] { compatible = ti,tas5504; reg = 0x1b; }; clock0:[EMAIL PROTECTED] { compatible = maxim,max9485; reg = 0x68; }; }; - Dave p.s. The only nontrivial technical issue I have with $PATCH is that struct at24_iface needs to cope better with rmmod at24. An optional teardown(...) call would suffice. +static int of_dev_node_match(struct device *dev, void *data) +{ +return dev-archdata.of_node == data; +} + +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node) +{ + struct device *dev; + + dev = bus_find_device(i2c_bus_type, NULL, node, +of_dev_node_match
Re: [i2c] Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM
On 8/27/08, David Brownell [EMAIL PROTECTED] wrote: On Tuesday 26 August 2008, Trent Piepho wrote: Lots of tv cards have eeproms with configuration information. They use an I2C driver called tveeprom that exports this: int tveeprom_read(struct i2c_client *c, unsigned char *eedata, int len) That presumes the driver somehow has access to that I2C client. How about using bus_find_device? PowerPC uses it to locate the i2c device using pointers in the device tree. +static int of_dev_node_match(struct device *dev, void *data) +{ +return dev-archdata.of_node == data; +} + +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node) +{ + struct device *dev; + + dev = bus_find_device(i2c_bus_type, NULL, node, +of_dev_node_match); + if (!dev) + return NULL; + + return to_i2c_client(dev); +} +EXPORT_SYMBOL(of_find_i2c_device_by_node); + (Also, that it's an I2C EEPROM. I keep wanting to have something usable for SPI EEPROMs and NVRAM too. But maybe nobody else cares.) Last I heard, the U-Boot policy was not to touch controllers that are not required during boot. Despite relatively common cases like needing to set up MAC addresses ... maybe this patch of Kevin's can make it a bunch easier to cope with that U-Boot policy. :) U-boot can put the MAC address into a property of the MAC's device node in the OF device tree (which u-boot passes to the kernel). Maybe with PowerPC it does. Not on ARM. ;) -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 8/1/08, Timur Tabi [EMAIL PROTECTED] wrote: On Thu, Jul 31, 2008 at 9:35 PM, Jon Smirl [EMAIL PROTECTED] wrote: I've having trouble with whether these clocks are a system resource or something that belongs to i2c. If they are a system resource then we should make nodes in the root and use a phandle in the i2c node to link to them. I can't speak for 52xx, but for 8[356]xx, I would say the clocks belong to the I2C devices. The screwball determination of whether to divide by 1, 2, or 3 only applies to the I2C device only. The divider itself is not used to drive a clock for any other device. If you disable I2C support, then you don't need to care about the divider (or its output clock) at all. That's why I think have the I2C clock in the parent node is wrong, because it would only be used if you had I2C child nodes. If you didn't have any I2C nodes, then you would need to delete the property from the parent node as well. I see this as being one of three ways... The source clocks belong to the platform and the clock messiness belongs in the platform code. The source clocks belong to i2c. The i2c driver needs to use platform specific bindings like Grant proposed. I don't like the third choice. Keep a simple Linux driver for i2c and the platform, and then move all of the messy code into uboot. BTW, the messy code is about 10 lines. It's going to take more than 10 lines to hide those 10 lines. I'm also of the view that all clocks are system resources. Linux is designed to have clock domains, we just aren't using them on PowerPC. Check out arch/powerpc/kernel/clock.c. They are mostly used on ARM. Could we define domains I2C1, I2C2,.. and then implement them? That implies using the root node to communicate the clock speeds to Linux. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 8/1/08, Timur Tabi [EMAIL PROTECTED] wrote: On Thu, Jul 31, 2008 at 9:35 PM, Jon Smirl [EMAIL PROTECTED] wrote: I've having trouble with whether these clocks are a system resource or something that belongs to i2c. If they are a system resource then we should make nodes in the root and use a phandle in the i2c node to link to them. I can't speak for 52xx, but for 8[356]xx, I would say the clocks belong to the I2C devices. The screwball determination of whether to divide by 1, 2, or 3 only applies to the I2C device only. The divider itself is not used to drive a clock for any other device. If you disable I2C support, then you don't need to care about the divider (or its output clock) at all. That's why I think have the I2C clock in the parent node is wrong, because it would only be used if you had I2C child nodes. If you didn't have any I2C nodes, then you would need to delete the property from the parent node as well. I see this as being one of three ways... The source clocks belong to the platform and the clock messiness belongs in the platform code. The source clocks belong to i2c. The i2c driver needs to use platform specific bindings like Grant proposed. I don't like the third choice. Keep a simple Linux driver for i2c and the platform, and then move all of the messy code into uboot. BTW, the messy code is about 10 lines. It's going to take more than 10 lines to hide those 10 lines. I'm also of the view that all clocks are system resources. Linux is designed to have clock domains, we just aren't using them on PowerPC. Check out arch/powerpc/kernel/clock.c. They are mostly used on ARM. Could we define domains I2C1, I2C2,.. and then implement them? That implies using the root node to communicate the clock speeds to Linux. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 8/1/08, Timur Tabi [EMAIL PROTECTED] wrote: Jon Smirl wrote: What about the Efika which is mpc5200 based and doesn't use uboot? How does the Efika handle the dozen other properties that U-Boot normally initializes in the device tree? Efika is like the original OpenFirmware. It has a Forth interpreter and builds the device tree itself. It doesn't use flat device trees that get filled in by the boot firmware. -- Timur Tabi Linux Kernel Developer @ Freescale -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Scott Wood [EMAIL PROTECTED] wrote: Timur Tabi wrote: Grant Likely wrote: No it doesn't, it depends on the register interface to decide compatibility. Clock interface is part of that. I don't think so. The interface for programming the clock registers is identical on all 8[356]xx parts. The only thing that matters is what specific values to put in the FDR and DFSR registers to get a desired I2C bus speed. If it affects the values you need to write to the registers to achieve a given result, how is it not a difference in the register interface? I propose the property clock-frequency, like this: [EMAIL PROTECTED] { #address-cells = 1; #size-cells = 0; cell-index = 0; compatible = fsl-i2c; reg = 0x3000 0x100; interrupts = 14 0x8; interrupt-parent = ipic; dfsrr; clock-frequency = 0xblablabla; -- added by U-Boot }; A clock-frequency property is OK, and is in line with what we do in other types of nodes. However, in the long run it might be nice to introduce some sort of clock binding where, for example, the i2c node can point to a clock elsewhere in the device tree as an input clock. That way, less knowledge is required by the firmware to poke values all over the place, and it also allows one to describe situations where the frequency of the input clock can change (such as in low-power modes). PowerPC,[EMAIL PROTECTED] { timebase-frequency = 0; /* From Bootloader */ bus-frequency = 0;/* From Bootloader */ clock-frequency = 0; /* From Bootloader */ }; The mpc5200 code already has mpc52xx_find_ipb_freq() to get it. I should grep before suggesting things. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Wolfgang Grandegger [EMAIL PROTECTED] wrote: Timur Tabi wrote: Wolfgang Grandegger wrote: But clock-frequency, aka bus-frequency, is already used by fsl_get_sys_freq(): http://lxr.linux.no/linux+v2.6.26/arch/powerpc/sysdev/fsl_soc.c#L80 So? clock-frequency is a per-node property. I use it in the codec node on the 8610 (mpc8610_hpcd.dts). It does not mean platform clock frequency. U-Boot could then fixup that value like bus-frequency() and the i2c-mpc driver simply calls fsl_get_i2c_freq(). This is just more complicated than it needs to be. Why should the I2C driver fetch the platform clock and the divider from the parent node, and then do additional math, when it could just get the value it needs right from the node it's probing? I'm a bit confused. The frequency of the I2C source clock and the real I2C clock frequency are two different things. The first one is common for all I2C devices, the second can be different. What properties would you like to use for defining both? For mpc5200 it is easy, mpc52xx_find_ipb_freq() already exists to get the source clock for the i2c devices. Each i2c node in the device tree can then specify clock-frequency = 40; or let it default. You have the input clock and the desired clock, do some math and you can set FDR. For the 8xxx chips there is no simple solution for obtain the input clock for the i2c section. Maybe create something like i2c-frequency and set it from uboot. The make another accessor function like mpc8xxx_find_i2c_freq(). PowerPC,[EMAIL PROTECTED] { timebase-frequency = 0; /* From Bootloader */ bus-frequency = 0;/* From Bootloader */ clock-frequency = 0; /* From Bootloader */ i2c-frequency = 0;/* From Bootloader */ }; Instead of creating i2c-frequency in the device tree, you could put this piece of code in the the mpc8xxx platform driver and use it to implement mpc8xxx_find_i2c_freq(). Read the PORDEVSR2_SEC_CFG bit back from whatever uboot set it too. #if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \ defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555) gd-i2c1_clk = sys_info.freqSystemBus; #elif defined(CONFIG_MPC8544) /* * On the 8544, the I2C clock is the same as the SEC clock. This can be * either CCB/2 or CCB/3, depending on the value of cfg_sec_freq. See * 4.4.3.3 of the 8544 RM. Note that this might actually work for all * 85xx, but only the 8544 has cfg_sec_freq, so it's unknown if the * PORDEVSR2_SEC_CFG bit is 0 on all 85xx boards that are not an 8544. */ if (gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG) gd-i2c1_clk = sys_info.freqSystemBus / 3; else gd-i2c1_clk = sys_info.freqSystemBus / 2; #else /* Most 85xx SOCs use CCB/2, so this is the default behavior. */ gd-i2c1_clk = sys_info.freqSystemBus / 2; #endif gd-i2c2_clk = gd-i2c1_clk; -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Timur Tabi [EMAIL PROTECTED] wrote: Jon Smirl wrote: For mpc5200 it is easy, mpc52xx_find_ipb_freq() already exists to get the source clock for the i2c devices. Each i2c node in the device tree can then specify clock-frequency = 40; or let it default. You 400KHz is the *speed* of the I2C bus, so let's be sure to use speed in the property name. Reserve the word bus for the input clock to the I2C device. #if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \ defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555) gd-i2c1_clk = sys_info.freqSystemBus; #elif defined(CONFIG_MPC8544) /* * On the 8544, the I2C clock is the same as the SEC clock. This can be * either CCB/2 or CCB/3, depending on the value of cfg_sec_freq. See * 4.4.3.3 of the 8544 RM. Note that this might actually work for all * 85xx, but only the 8544 has cfg_sec_freq, so it's unknown if the * PORDEVSR2_SEC_CFG bit is 0 on all 85xx boards that are not an 8544. */ if (gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG) gd-i2c1_clk = sys_info.freqSystemBus / 3; else gd-i2c1_clk = sys_info.freqSystemBus / 2; #else /* Most 85xx SOCs use CCB/2, so this is the default behavior. */ gd-i2c1_clk = sys_info.freqSystemBus / 2; #endif gd-i2c2_clk = gd-i2c1_clk; I think the whole point is to eliminate duplicating this code in the Linux driver. It's hideously ugly, so it should be limited as much as possible. It wouldn't go into the i2c driver, it would go into the mpc8xxx platform driver. Why is it bad to put it into the mpc8xxx platform driver? It is an accurate description of the mpc8xxx platform isn't it? -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Grant Likely [EMAIL PROTECTED] wrote: On Thu, Jul 31, 2008 at 03:37:07PM -0500, Timur Tabi wrote: Grant Likely wrote: How is the divider controlled? Is it a fixed property of the SoC? Yes. The divider is either 1, 2, or 3, and the only way to know which one it is is to look up the specific SOC model number. And depending on the SOC model, there may also be a register that needs to be looked up. a shared register setting? or a register setting within the i2c device? The I2C device itself has no idea what the divider is. It only sees the result of the divider. Then that absolutely suggests to me that either the final clock or the divider should be encoded in the i2c node; not in the soc node. Isn't there a single global divider that generates all the i2c source clocks? You don't want to copy a global value into each i2c node. Aren't we talking about the /2 or /3 or /1 divider that appears to be randomly implemented on various members of the mpc8xxx family? g. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Timur Tabi [EMAIL PROTECTED] wrote: Jon Smirl wrote: Isn't there a single global divider that generates all the i2c source clocks? You don't want to copy a global value into each i2c node. Why not? There are only two I2C devices, and it's theoretically possible for them to have different input clock frequencies. Keeping it in the I2C node allows the I2C driver to reference a property directly in the node that its probing. But that's the same as saying we should copy the system clock frequency into all of the PSC nodes because we might implement hardware where they aren't all clocked off from the same input clock source. Aren't we talking about the /2 or /3 or /1 divider that appears to be randomly implemented on various members of the mpc8xxx family? I don't this these dividers or clocks need to be exposed at all if you'd just put that ugly code snippet into your platform driver. Yes. -- Timur Tabi Linux kernel developer at Freescale -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Trent Piepho [EMAIL PROTECTED] wrote: On Thu, 31 Jul 2008, Jon Smirl wrote: Here's my idea: [EMAIL PROTECTED] { compatible = fsl-i2c; bus-frequency = 10; /* Either */ source-clock-frequency = 0; /* OR */ source-clock = ccb; }; Can't we hide a lot of this on platforms where the source clock is not messed up? For example the mpc5200 doesn't need any of this, the needed frequency is already available in mpc52xx_find_ipb_freq(). mpc5200 doesn't need any uboot change. Next would be normal mpc8xxx platforms where i2c is driven by a single clock, add a uboot filled in parameter in the root node (or I think it can be computed off of the ones uboot is already filling in). make a mpc8xxx_find_i2c_freq() function. May not need to change device tree and uboot. Finally use this for those days when the tea leaves were especially bad. Both a device tree and uboot change. If you have to support clock speed in the i2c node anyway, what's the point of the other options? So that I don't have to change my existing mpc5200 systems. mpc5200 has no need for specifying the source clock in each i2c node, hardware doesn't allow it. Except the i2c clock isn't always a based on an integer divider of the CCB frequency. What's more, it's not always the same for both i2c controllers. Suppose i2c #1 uses CCB times 2/3 and i2c #2 uses CCB/2, how would fsl_get_i2c_freq() figure that out from bus-frequency and i2c-clock-divider? If you get the CCB frequency from uboot and know the chip model, can't you compute these in the platform code? Then make a mpc8xxx_find_i2c_freq(cell_index). You need a bunch of random other device registers (SEC, ethernet, sdhc, etc.) too. I don't see why we want to go through the trouble of having uboot tell us things about a chip that are fixed in stone. Obviously something like the frequency of the external crystal needs to be passed up, but why pass up stuff that can be computed (or recovered by reading a register)? One could also say that if U-boot has to have the code and already calculated the value, why duplicate the code and the calculation in Linux? What about the Efika which is mpc5200 based and doesn't use uboot? -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Grant Likely [EMAIL PROTECTED] wrote: Your mixing up device tree layout with implementation details. Device tree layout must come first. mpc52xx_find_ipb_freq() is just a convenience function that walks up the device tree looking for a bus-frequency property. So, instead of making arguments based on available helper functions, make your arguments based on how data should be laid out in the device tree. Currently mpc5200 bindings simply depend on finding a bus-frequency property in the parent node for determining the input clock and I don't see any pressing reason to change that (though it probably needs to be documented better). However, for the complex cases that Trent and Timur are talking about, it makes perfect sense to have an optional property in the i2c node itself that defines a different clock. Once that decision has been made and documented, then the driver can be modified and the appropriate helper functions added to adapt the device tree data into something useful. I've having trouble with whether these clocks are a system resource or something that belongs to i2c. If they are a system resource then we should make nodes in the root and use a phandle in the i2c node to link to them. The phandle in the mpc5200 case could be implicit since it is fixed in silicon. Is this register in a system register bank or an i2c one? gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG Remember (and chant this to yourself). The device tree describes *hardware*. It does not describe Linux internal implementation details. g. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] at24.c, how to get kobject for sysfs/bin_attribute methods?
PowerPC is going to use this Add the of_find_i2c_device_by_node function From: Jon Smirl [EMAIL PROTECTED] Add the of_find_i2c_device_by_node function. This allows you to follow a reference in the device tree to an i2c device node and then locate the linux device instantiated by the device tree. Example use, an i2s codec controlled by i2c. Depends on patch exporting i2c root bus symbol. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- drivers/of/of_i2c.c| 28 include/linux/of_i2c.h |2 ++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c index 6a98dc8..ba7b394 100644 --- a/drivers/of/of_i2c.c +++ b/drivers/of/of_i2c.c @@ -19,7 +19,7 @@ void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node) { - void *result; + struct i2c_client *i2c_dev; struct device_node *node; for_each_child_of_node(adap_node, node) { @@ -41,18 +41,38 @@ void of_register_i2c_devices(struct i2c_adapter *adap, info.addr = *addr; - request_module(info.type); + request_module(%s, info.type); - result = i2c_new_device(adap, info); - if (result == NULL) { + i2c_dev = i2c_new_device(adap, info); + if (i2c_dev == NULL) { printk(KERN_ERR of-i2c: Failed to load driver for %s\n, info.type); irq_dispose_mapping(info.irq); continue; } + + i2c_dev-dev.archdata.of_node = of_node_get(node); } } EXPORT_SYMBOL(of_register_i2c_devices); +static int of_dev_node_match(struct device *dev, void *data) +{ +return dev-archdata.of_node == data; +} + +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node) +{ + struct device *dev; + + dev = bus_find_device(i2c_bus_type, NULL, node, +of_dev_node_match); + if (!dev) + return NULL; + + return to_i2c_client(dev); +} +EXPORT_SYMBOL(of_find_i2c_device_by_node); + MODULE_LICENSE(GPL); diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h index bd2a870..17d5897 100644 --- a/include/linux/of_i2c.h +++ b/include/linux/of_i2c.h @@ -16,5 +16,7 @@ void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node); +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node); + #endif /* __LINUX_OF_I2C_H */ -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] Problem with restricted I2C algorithms in kernel 2.6.26!
On 7/26/08, Andrew Morton [EMAIL PROTECTED] wrote: (cc's added) On Wed, 16 Jul 2008 12:33:57 -0700 D. Kelly [EMAIL PROTECTED] wrote: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=3845de25c5f83cd52729570f7b501679d37ca8de The patch at the preceeding url disables the users ability to select I2C algorithms. Specifically the reason stated was: The algorithm drivers are helper drivers that are selected automatically as needed. There's no point in listing them in the config menu, it can only confuse users and waste their time. I support Jean's decision on this. Very few people know how to correctly enable those algorithms and most people get them wrong. Now they are set automatically. What about merging a placeholder driver for dvb-s2 that selects the needed i2c algorithm? Then merge a real driver as soon as possible. My out of tree drivers are written as a patch against the kernel. The patch contains a select for the algorithm in my Kconfig additions. When the drivers work, I'll submit them. The algorithm drivers will not be 'selected automatically as needed' if the user is compiling something outside of the kernel that requires them! Just one example, there are drivers found in the V4L dvb driver tree that require i2c bit-banging be enabled. The drivers are now broken because the user is not allowed to enable bit-banging himself. The only way around this is to revert the patch manually or enable something else in the kernel, that he doesn't need, just to get bit-banging. It's a very bad idea to assume that nothing built outside of the kernel may need i2c algorithms. Furthermore, the whole point of being able to customize your kernel is so you can select only the things which you need. It makes no good sense to intentionally disable/restrict the users ability to do so. Additionally, assuming the ability to select i2c algorithms will only confuse the user and waste their time is ridiculous. The user should be allowed to decide for himself what he needs regarding this! One of the biggest reasons people choose to compile things from cvs/svn/mercurial/etc. is because it gives them access to newer bug fixes and support for things not yet present in the kernel source. A perfect example, the multiproto dvb driver tree. Users wanting support for dvb-s2 devices have to compile drivers outside of the kernel because it's simply not available in the kernel and won't be for some time. I've contacted one of the i2c subsystem maintainers, Jean Delvare, but unfortunately he doesn't seem to care about this problem and his advice in dealing with it is to Just get these drivers merged in the kernel. Ah ah ah!... Clearly the more sane and reasonable solution is to not cripple the menu options in the first place, especially when it creates no benefit and only serves to limit/restrict the users ability to select what he needs. I'm asking that the patch be reverted and anyone in agreement to please voice their opinion here in public. Best regards, -Derek ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] rtc-pcf8563: Remove client validation
On 7/22/08, Laurent Pinchart [EMAIL PROTECTED] wrote: Hi Alexander, On Wednesday 02 July 2008, Jean Delvare wrote: On Wed, 2 Jul 2008 13:32:14 +0200, Laurent Pinchart wrote: Validating clients with black magic register checks doesn't make much sense for new-style i2c driver and has been known to fail on valid NXP pcf8563 chips. This patch removes the client validation code. I've hit this same problem with the rtc8564. When the chips are first installed the registers haven't been initialized. This check makes the driver load fail which prevents the user space clock app from running and initializing the registers. The work around was to set the initial time from uboot. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] int vs uint
On 7/21/08, Ben Dooks [EMAIL PROTECTED] wrote: On Sun, Jul 20, 2008 at 12:47:38PM -0400, Jon Smirl wrote: On 7/20/08, Jon Smirl [EMAIL PROTECTED] wrote: On 7/20/08, Ben Dooks [EMAIL PROTECTED] wrote: On Sat, Jul 19, 2008 at 08:46:41PM -0400, Jon Smirl wrote: There are a lot places in the i2c API where int is used when the parameter can't be negative. For example, there are more /* * The master routines are the ones normally used to transmit data to devices * on a bus (or read from them). Apart from two basic transfer functions to * transmit one message at a time, a more complex version can be used to * transmit an arbitrary number of messages without interruption. */ extern int i2c_master_send(struct i2c_client *,const char* ,int); extern int i2c_master_recv(struct i2c_client *,char* ,int); /* Transfer num messages. */ extern int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num); u8 level; /* nesting level for lockdep */ Wouldn't these generate more efficient code if switched to uints? I'm not sure, most of the time there's not a lot of difference between the signed and unsigned case. If you can provide an example of where this is actually true then I would be interested to see... Assigning between lengths causes sign extended instructions. Mixing signed and unsigned in expressions can cause them too. Entirely dependant on the architecture, so very rarely see anything like this happening on ARM. You get the best code generation by using int or unsigned int as appropriate and supplying the compiler with the correct description of the variable. The compiler is also able to catch more errors. If the math can only yield a negative result and it is being compared to a unsigned int, you'll get compiler errors like expression is constant. The compiler checking argument is the most compelling of these, are you offering to do the necessary changes, or is this fishing to find someone else to do it? Fishing. Changes like these go a lot smoother if one of the maintainers do them. Doing them as an outside patch will a generate a pile of merge conflicts with other patches made during the cycle. I'd just like to see the i2c interface be as accurate as possible. It is clear that some of those ints should be uints. -- Ben ([EMAIL PROTECTED], http://www.fluff.org/) 'a smiley only costs 4 bytes' -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] int vs uint
On 7/20/08, Ben Dooks [EMAIL PROTECTED] wrote: On Sat, Jul 19, 2008 at 08:46:41PM -0400, Jon Smirl wrote: There are a lot places in the i2c API where int is used when the parameter can't be negative. For example, there are more /* * The master routines are the ones normally used to transmit data to devices * on a bus (or read from them). Apart from two basic transfer functions to * transmit one message at a time, a more complex version can be used to * transmit an arbitrary number of messages without interruption. */ extern int i2c_master_send(struct i2c_client *,const char* ,int); extern int i2c_master_recv(struct i2c_client *,char* ,int); /* Transfer num messages. */ extern int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num); u8 level; /* nesting level for lockdep */ Wouldn't these generate more efficient code if switched to uints? I'm not sure, most of the time there's not a lot of difference between the signed and unsigned case. If you can provide an example of where this is actually true then I would be interested to see... Assigning between lengths causes sign extended instructions. Mixing signed and unsigned in expressions can cause them too. You get the best code generation by using int or unsigned int as appropriate and supplying the compiler with the correct description of the variable. Technically, an unsigned int or simply an unsigned would be a reasonable change given that you can't really have a minus number of transfers. -- Ben ([EMAIL PROTECTED], http://www.fluff.org/) 'a smiley only costs 4 bytes' -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] int vs uint
On 7/20/08, Jon Smirl [EMAIL PROTECTED] wrote: On 7/20/08, Ben Dooks [EMAIL PROTECTED] wrote: On Sat, Jul 19, 2008 at 08:46:41PM -0400, Jon Smirl wrote: There are a lot places in the i2c API where int is used when the parameter can't be negative. For example, there are more /* * The master routines are the ones normally used to transmit data to devices * on a bus (or read from them). Apart from two basic transfer functions to * transmit one message at a time, a more complex version can be used to * transmit an arbitrary number of messages without interruption. */ extern int i2c_master_send(struct i2c_client *,const char* ,int); extern int i2c_master_recv(struct i2c_client *,char* ,int); /* Transfer num messages. */ extern int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num); u8 level; /* nesting level for lockdep */ Wouldn't these generate more efficient code if switched to uints? I'm not sure, most of the time there's not a lot of difference between the signed and unsigned case. If you can provide an example of where this is actually true then I would be interested to see... Assigning between lengths causes sign extended instructions. Mixing signed and unsigned in expressions can cause them too. You get the best code generation by using int or unsigned int as appropriate and supplying the compiler with the correct description of the variable. The compiler is also able to catch more errors. If the math can only yield a negative result and it is being compared to a unsigned int, you'll get compiler errors like expression is constant. Technically, an unsigned int or simply an unsigned would be a reasonable change given that you can't really have a minus number of transfers. -- Ben ([EMAIL PROTECTED], http://www.fluff.org/) 'a smiley only costs 4 bytes' -- Jon Smirl [EMAIL PROTECTED] -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] int vs uint
There are a lot places in the i2c API where int is used when the parameter can't be negative. For example, there are more /* * The master routines are the ones normally used to transmit data to devices * on a bus (or read from them). Apart from two basic transfer functions to * transmit one message at a time, a more complex version can be used to * transmit an arbitrary number of messages without interruption. */ extern int i2c_master_send(struct i2c_client *,const char* ,int); extern int i2c_master_recv(struct i2c_client *,char* ,int); /* Transfer num messages. */ extern int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num); u8 level; /* nesting level for lockdep */ Wouldn't these generate more efficient code if switched to uints? -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] i2c: cdev lock_kernel() pushdown
On 7/15/08, Jean Delvare [EMAIL PROTECTED] wrote: On Tue, 15 Jul 2008 10:14:06 -0600, Jonathan Corbet wrote: Hi, Jean, I am looking at this patch of yours: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=3db633ee352bfe20d4a2b0c3c8a46ce31a6c7149 I believe that no locking is needed in i2cdev_open(). Do you have any reason to think it does? If not, can I simply revert this patch? Before now, i2cdev_open() has always had the protection of the BKL. When I pushed that locking down into the individual open() functions, I really had to take a pretty conservative approach and assume that the BKL was needed unless that was really obviously not the case. In i2cdev_open(), for example, there's: i2c_dev = i2c_dev_get_by_minor(minor); and I really don't know what keeps *i2c_dev from going away during the rest of the call. I'm *not* saying there is a problem here; I just don't know. So the only thing I could really do is to push the BKL down and let the maintainers sort it out. ...all of which is my long-winded way of saying that, if you're convinced that i2cdev_open() is safe in the absence of the BKL, feel free to take it out. Good point. i2c_dev is guaranteed to stay thanks to the call to i2c_get_adapter(), however it happens _after_ the call to i2c_dev_get_by_minor(), so without additional locking, this is racy. That being said, I'm not sure how lock_kernel() is supposed to help. Is the BKL held when i2cdev_detach_adapter() is called? If not, then I suspect that the current code is already racy. I'll look into this, thanks for the hint. Is i2c-dev vulnerable to the i2c device disappearing, for example rmmod it? Would combining i2c-dev into i2c core and integrating it with the core's lock protection make things easier to lock? You could make a compile time option to remove it for small systems. If it's in the core is attach/detach adapter still needed? I haven't looked at any of this in detail, but i2c-dev is only 6K of code. Half of the 6K might disappear if integrated into the core. What happens if user space and an in-kernel user both try using the device? I've never tried doing that. Should the presence of an in-kernel user make the user space device disappear? -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] i2c: cdev lock_kernel() pushdown
On 7/15/08, Jean Delvare [EMAIL PROTECTED] wrote: Hi Jon, On Tue, 15 Jul 2008 13:01:07 -0400, Jon Smirl wrote: On 7/15/08, Jean Delvare [EMAIL PROTECTED] wrote: On Tue, 15 Jul 2008 10:14:06 -0600, Jonathan Corbet wrote: Hi, Jean, I am looking at this patch of yours: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=3db633ee352bfe20d4a2b0c3c8a46ce31a6c7149 I believe that no locking is needed in i2cdev_open(). Do you have any reason to think it does? If not, can I simply revert this patch? Before now, i2cdev_open() has always had the protection of the BKL. When I pushed that locking down into the individual open() functions, I really had to take a pretty conservative approach and assume that the BKL was needed unless that was really obviously not the case. In i2cdev_open(), for example, there's: i2c_dev = i2c_dev_get_by_minor(minor); and I really don't know what keeps *i2c_dev from going away during the rest of the call. I'm *not* saying there is a problem here; I just don't know. So the only thing I could really do is to push the BKL down and let the maintainers sort it out. ...all of which is my long-winded way of saying that, if you're convinced that i2cdev_open() is safe in the absence of the BKL, feel free to take it out. Good point. i2c_dev is guaranteed to stay thanks to the call to i2c_get_adapter(), however it happens _after_ the call to i2c_dev_get_by_minor(), so without additional locking, this is racy. That being said, I'm not sure how lock_kernel() is supposed to help. Is the BKL held when i2cdev_detach_adapter() is called? If not, then I suspect that the current code is already racy. I'll look into this, thanks for the hint. Is i2c-dev vulnerable to the i2c device disappearing, for example rmmod it? In general no, but there is a race where this can actually happen. Would combining i2c-dev into i2c core and integrating it with the core's lock protection make things easier to lock? Definitely. That's something I want to do for other reasons anyway. You could make a compile time option to remove it for small systems. If it's in the core is attach/detach adapter still needed? i2c-dev will probably soon be the last user of i2c_adapter.attach_adapter and i2c_adapter.detach_adapter, and this is indeed one of my incentives to look into how things could be rearranged. I haven't looked at any of this in detail, but i2c-dev is only 6K of code. Half of the 6K might disappear if integrated into the core. I doubt we can remove 3 kB just by moving things around. The book-keeping is relatively small. But this isn't my goal anyway. My goal is to make things cleaner and possibly safer. One thing we may do if size is a concern, is keep the functional part of i2c-dev in its separate module, and only merge the book-keeping and char device creation into i2c-core. What happens if user space and an in-kernel user both try using the device? I've never tried doing that. Should the presence of an in-kernel user make the user space device disappear? What do you mean by device? The i2c bus, or a specific i2c device (client)? The i2c bus can be used by any amount of users, in-kernel or user-space. For i2c devices, it's more complex, depending on whether an i2c_client is registered or not. Only one i2c_client can be registered at a given address, but nothing prevents raw accesses to the devices. i2c-dev makes such raw accesses, even though it has an initial check whether the i2c address is in used by a registered client or not. This needs to be cleaned up from the ground up, the current approach is not satisfactory at all and things are getting worse with the advent of new-style i2c devices, because the have a different notion of business. My idea was that when a client registers it would indicate whether or not it wants a user space device. Most in-kernel clients would not want a user space device. A sysfs attribute could force the creation of a user space device for debugging. You might also want to make a global i2c sysfs attribute that you would echo an i2c address into. Doing that would cause a user space device to be created for that address. Now you can manipulate devices with no in-kernel driver. Of course we can't stop raw access but life is simpler if everyone avoids doing it. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops
On 7/12/08, Wolfram Sang [EMAIL PROTECTED] wrote: On Fri, Jul 11, 2008 at 12:23:23PM -0600, Grant Likely wrote: On Fri, Jul 11, 2008 at 09:48:59PM +0400, Anton Vorontsov wrote: Firstly kernel warns at set_irq_chip, and then dies completely: Trying to install chip for IRQ-1 diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c index b2ccdcb..95a24de 100644 --- a/drivers/of/of_i2c.c +++ b/drivers/of/of_i2c.c @@ -93,10 +93,8 @@ void of_register_i2c_devices(struct i2c_adapter *adap, if (info.irq == NO_IRQ) info.irq = -1; What is the reason that info.irq is set to -1 in the first place? This looks like another bug to me. Does something in the i2c layer depend on the -1 value? You already acked a fix to this :) I wasn't sure if my patch would make it on its own, as Jon Smirl was also working on fixes to of_i2c.c and he seemed to pick up this issue, too. I did another patch for the mpc-i2c driver changing all of the comparisons to NO_IRQ. My understanding of this is the ppc has NO_IRQ set to -1, and powerpc has NO_IRQ = 0. So to make all of this work right you have to use the NO_IRQ symbol and you can't check against 0 or -1 directly. I also believe work is underway to get all platforms to NO_IRQ = 0 but I don't know if it is completed yet. (Original patch is here: http://ozlabs.org/pipermail/linuxppc-dev/2008-June/058801.html ) All the best, Wolfram -- Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de Pengutronix - Linux Solutions for Science and Industry -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFIeGSED27XaX1/VRsRAr+EAJ948UwobnY7WSSR4i/ywjof1+8dJACfWzPN bhW6NXgBCnwqITIC5rSXeAI= =W3sj -END PGP SIGNATURE- ___ Linuxppc-dev mailing list [EMAIL PROTECTED] https://ozlabs.org/mailman/listinfo/linuxppc-dev -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] IEEE 1275, Device tree standard
This is the IEEE standard that specifies the devices names we are using. ftp://playground.sun.com/pub/p1275/coredoc/1275-1994/ -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4
On 7/1/08, Jean Delvare [EMAIL PROTECTED] wrote: I'm fine with this patch. In particular, exporting i2c_bus_type is OK. It was un-exported only because it had no user left, but it can be exported again if needed. Another solution would be to move drivers/of/of_i2c into the i2c directory and make it part of i2c core on powerpc builds. I'm not the one to push this upstream though, as the patch is essentially an openfirmware patch. That would be something for Jochen Friedrich and Paul Mackerras I guess. Would be nice to have a MAINTAINERS entry for OF... -- Jean Delvare -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4
On 7/1/08, Jean Delvare [EMAIL PROTECTED] wrote: Hi Jon, On Mon, 30 Jun 2008 19:01:28 -0400, Jon Smirl wrote: Add the of_find_i2c_device_by_node function. This allows you to follow a reference in the device to an i2c device node and then locate the linux device instantiated by the device tree. Example use, an i2s codec controlled by i2c. --- drivers/i2c/i2c-core.c |2 +- drivers/of/of_i2c.c| 37 ++--- include/linux/i2c.h|3 +++ include/linux/of_i2c.h |2 ++ 4 files changed, 32 insertions(+), 12 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index d0175f4..e3abe1b 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -208,7 +208,7 @@ static struct device_attribute i2c_dev_attrs[] = { { }, }; -static struct bus_type i2c_bus_type = { +struct bus_type i2c_bus_type = { .name = i2c, .dev_attrs = i2c_dev_attrs, .match = i2c_device_match, What if i2c-core is built as a module? Don't you need to export the symbol then? Jean, can you re-export i2c_bus_type and then I'll drop that part from the patch? That way the patch won't hit multiple maintainers. diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c index 715a444..ca69a16 100644 --- a/drivers/of/of_i2c.c +++ b/drivers/of/of_i2c.c @@ -75,7 +75,7 @@ static int of_find_i2c_driver(struct device_node *node, void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node) { - void *result; + struct i2c_client *i2c_dev; struct device_node *node; for_each_child_of_node(adap_node, node) { @@ -90,29 +90,44 @@ void of_register_i2c_devices(struct i2c_adapter *adap, continue; } - info.irq = irq_of_parse_and_map(node, 0); - if (info.irq == NO_IRQ) - info.irq = -1; - - if (of_find_i2c_driver(node, info) 0) { - irq_dispose_mapping(info.irq); + if (of_find_i2c_driver(node, info) 0) continue; - } + info.irq = irq_of_parse_and_map(node, 0); info.addr = *addr; - request_module(info.type); + request_module(%s, info.type); A separate fix for this issue was already sent by Maximilian Attems a few days go: http://lists.lm-sensors.org/pipermail/i2c/2008-June/004030.html - result = i2c_new_device(adap, info); - if (result == NULL) { + i2c_dev = i2c_new_device(adap, info); + if (i2c_dev == NULL) { printk(KERN_ERR of-i2c: Failed to load driver for %s\n, info.type); irq_dispose_mapping(info.irq); continue; } + + i2c_dev-dev.archdata.of_node = of_node_get(node); } } EXPORT_SYMBOL(of_register_i2c_devices); +static int of_dev_node_match(struct device *dev, void *data) +{ +return dev-archdata.of_node == data; +} + +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node) +{ + struct device *dev; + + dev = bus_find_device(i2c_bus_type, NULL, node, + of_dev_node_match); + if (!dev) + return NULL; + + return to_i2c_client(dev); +} +EXPORT_SYMBOL(of_find_i2c_device_by_node); + MODULE_LICENSE(GPL); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index fb9af6a..186b22d 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -92,6 +92,9 @@ extern s32 i2c_smbus_read_i2c_block_data(struct i2c_client * client, extern s32 i2c_smbus_write_i2c_block_data(struct i2c_client * client, u8 command, u8 length, const u8 *values); + +/* Base of the i2c bus */ +extern struct bus_type i2c_bus_type; /* * A driver is capable of handling one or more physical devices present on diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h index bd2a870..17d5897 100644 --- a/include/linux/of_i2c.h +++ b/include/linux/of_i2c.h @@ -16,5 +16,7 @@ void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node); +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node); + #endif /* __LINUX_OF_I2C_H */ I'm fine with this patch. In particular, exporting i2c_bus_type is OK. It was un-exported only because it had no user left, but it can be exported again if needed. I'm not the one
Re: [i2c] [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4
On 7/1/08, Jean Delvare [EMAIL PROTECTED] wrote: On Tue, 1 Jul 2008 11:12:58 -0400, Jon Smirl wrote: On 7/1/08, Jean Delvare [EMAIL PROTECTED] wrote: I'm fine with this patch. In particular, exporting i2c_bus_type is OK. It was un-exported only because it had no user left, but it can be exported again if needed. Another solution would be to move drivers/of/of_i2c into the i2c directory and make it part of i2c core on powerpc builds. I don't think this is a good idea. Merging arch-specific code (or half-arch-specific code in this case) into arch-neutral drivers ends up being a pain to maintain. People will keep sending me patches for stuff I don't know anything about and can't help with. Having of-specific stuff in just one directory as is the case now sounds much better to me. All it's missing is a MAINTAINERS entry. A side effect of this is that the small pieces of code in drivers/of have to be compiled into stand alone modules and they may need access to internal symbols from the subsystem. If they were directly linked into the subsystems you wouldn't need to make the internal symbols visible. Now the subsystems have to be careful about breaking the in-kernel, external users of the symbols and we've made it possible for out of tree drivers to get to internal structures. -- Jean Delvare -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4
On 7/1/08, Grant Likely [EMAIL PROTECTED] wrote: On Tue, Jul 01, 2008 at 11:12:58AM -0400, Jon Smirl wrote: On 7/1/08, Jean Delvare [EMAIL PROTECTED] wrote: I'm fine with this patch. In particular, exporting i2c_bus_type is OK. It was un-exported only because it had no user left, but it can be exported again if needed. Another solution would be to move drivers/of/of_i2c into the i2c directory and make it part of i2c core on powerpc builds. My preference is for things like of_spi and of_i2c to go with the related busses; I think it makes more sense to keep all the I2C stuff together, but I've already lost that battle once. This is a similar problem to adding aliases to the i2c driver drivers for the device tree names of the i2c devices. Instead we have code in drivers/of/of_i2c.c that tries to guess the translation from device tree to linux names. Adding aliases to the drivers would eliminate the need for of_find_i2c_driver(). I've previously posted patches implementing device tree names in the drivers that used ifdef to only instantiate on powerpc builds. For example diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c index e07274d..9cd1770 100644 --- a/drivers/i2c/chips/tps65010.c +++ b/drivers/i2c/chips/tps65010.c @@ -571,6 +571,10 @@ static const struct i2c_device_id tps65010_id[] = { { tps65011, TPS65011 }, { tps65012, TPS65012 }, { tps65013, TPS65013 }, + OF_ID(ti,tps65010, TPS65010) + OF_ID(ti,tps65011, TPS65011) + OF_ID(ti,tps65012, TPS65012) + OF_ID(ti,tps65013, TPS65013) { }, }; MODULE_DEVICE_TABLE(i2c, tps65010_id); -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4
On 7/1/08, Jean Delvare [EMAIL PROTECTED] wrote: On Tue, 1 Jul 2008 10:45:18 -0600, Grant Likely wrote: On Tue, Jul 01, 2008 at 06:29:49PM +0200, Jean Delvare wrote: On Tue, 1 Jul 2008 11:12:58 -0400, Jon Smirl wrote: On 7/1/08, Jean Delvare [EMAIL PROTECTED] wrote: I'm fine with this patch. In particular, exporting i2c_bus_type is OK. It was un-exported only because it had no user left, but it can be exported again if needed. Another solution would be to move drivers/of/of_i2c into the i2c directory and make it part of i2c core on powerpc builds. I don't think this is a good idea. Merging arch-specific code (or half-arch-specific code in this case) into arch-neutral drivers ends up being a pain to maintain. People will keep sending me patches for stuff I don't know anything about and can't help with. Having of-specific stuff in just one directory as is the case now sounds much better to me. All it's missing is a MAINTAINERS entry. But the other side of the coin is that each driver must have driver-specific OF code to translate data in the device tree to data usable by the driver. It doesn't make any sense to me for that stuff to live anywhere other that with the driver that it supports. This code is glue between OF and subsystems. As with any glue code, you can argue forever on which side you want to push the code to. Both answers are valid. All I see on my personal side is that I don't have any system using OF and no knowledge about it either, so I can't maintain of_i2c. So having that file in drivers/of rather than drivers/i2c will make my life easier for sure. While I'd guess that most (all?) OF-based systems have an I2C bus, so whoever is responsible for drivers/of should be able to maintain of_i2c. We could modify the Makefile for i2c core to get the source out of drivers/of and link it into i2c-core. That would remove the need to export symbols. Or you could move the file into the i2c directory and just put a note on it that Grant is the maintainer. -- Jean Delvare -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4
On 7/1/08, Jean Delvare [EMAIL PROTECTED] wrote: On Tue, 1 Jul 2008 13:00:08 -0400, Jon Smirl wrote: On 7/1/08, Grant Likely [EMAIL PROTECTED] wrote: My preference is for things like of_spi and of_i2c to go with the related busses; I think it makes more sense to keep all the I2C stuff together, but I've already lost that battle once. This is a similar problem to adding aliases to the i2c driver drivers for the device tree names of the i2c devices. Instead we have code in drivers/of/of_i2c.c that tries to guess the translation from device tree to linux names. Adding aliases to the drivers would eliminate the need for of_find_i2c_driver(). I've previously posted patches implementing device tree names in the drivers that used ifdef to only instantiate on powerpc builds. For example diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c index e07274d..9cd1770 100644 --- a/drivers/i2c/chips/tps65010.c +++ b/drivers/i2c/chips/tps65010.c @@ -571,6 +571,10 @@ static const struct i2c_device_id tps65010_id[] = { { tps65011, TPS65011 }, { tps65012, TPS65012 }, { tps65013, TPS65013 }, + OF_ID(ti,tps65010, TPS65010) + OF_ID(ti,tps65011, TPS65011) + OF_ID(ti,tps65012, TPS65012) + OF_ID(ti,tps65013, TPS65013) { }, }; MODULE_DEVICE_TABLE(i2c, tps65010_id); Yeah, yeah, you've been asking for this for months already, but it's just not going to happen, sorry. You want to abuse the standard Linux alias mechanism for your personal (i.e. openfirmware) use, but that's bad. Linux drivers shouldn't have to know whether they are used in openfirmware trees and what device names are used there. And device names as seen by user-space shouldn't vary depending on whether the device comes from an openfirmware tree or not - otherwise all user-space apps need to learn about both naming conversions. Unsurprisingly, no other subsystem does what you propose. Then what are all of the PCI aliases doing? The only difference is that you are recognizing the PCI group as a naming authority and not recognizing the PowerPC device tree group. But on the PowerPC platform that is our naming authority. That's why I proposed adding the names on ifdefs so that they disappear on non PowerPC platforms. PS - adding an alias to a driver does not change the name of the driver. My PCI e1000 module has about 100 aliases but it is always e1000. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4
On 7/1/08, Jon Smirl [EMAIL PROTECTED] wrote: On 7/1/08, Jean Delvare [EMAIL PROTECTED] wrote: On Tue, 1 Jul 2008 13:00:08 -0400, Jon Smirl wrote: On 7/1/08, Grant Likely [EMAIL PROTECTED] wrote: My preference is for things like of_spi and of_i2c to go with the related busses; I think it makes more sense to keep all the I2C stuff together, but I've already lost that battle once. This is a similar problem to adding aliases to the i2c driver drivers for the device tree names of the i2c devices. Instead we have code in drivers/of/of_i2c.c that tries to guess the translation from device tree to linux names. Adding aliases to the drivers would eliminate the need for of_find_i2c_driver(). I've previously posted patches implementing device tree names in the drivers that used ifdef to only instantiate on powerpc builds. For example diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c index e07274d..9cd1770 100644 --- a/drivers/i2c/chips/tps65010.c +++ b/drivers/i2c/chips/tps65010.c @@ -571,6 +571,10 @@ static const struct i2c_device_id tps65010_id[] = { { tps65011, TPS65011 }, { tps65012, TPS65012 }, { tps65013, TPS65013 }, + OF_ID(ti,tps65010, TPS65010) + OF_ID(ti,tps65011, TPS65011) + OF_ID(ti,tps65012, TPS65012) + OF_ID(ti,tps65013, TPS65013) { }, }; MODULE_DEVICE_TABLE(i2c, tps65010_id); Yeah, yeah, you've been asking for this for months already, but it's just not going to happen, sorry. You want to abuse the standard Linux alias mechanism for your personal (i.e. openfirmware) use, but that's bad. Linux drivers shouldn't have to know whether they are used in openfirmware trees and what device names are used there. And device names as seen by user-space shouldn't vary depending on whether the device comes from an openfirmware tree or not - otherwise all user-space apps need to learn about both naming conversions. Unsurprisingly, no other subsystem does what you propose. Then what are all of the PCI aliases doing? The only difference is that you are recognizing the PCI group as a naming authority and not recognizing the PowerPC device tree group. But on the PowerPC platform that is our naming authority. That's why I proposed adding the names on ifdefs so that they disappear on non PowerPC platforms. PS - adding an alias to a driver does not change the name of the driver. My PCI e1000 module has about 100 aliases but it is always e1000. Here's my e1000e sysfs entry: [EMAIL PROTECTED]:/sys/bus/pci/devices/:00:19.0$ ls broken_parity_status device local_cpus power resource2 uevent bus driver modaliasresource subsystem vendor class enable msi_bus resource0 subsystem_device configirq net:eth0resource1 subsystem_vendor [EMAIL PROTECTED]:/sys/bus/pci/devices/:00:19.0$ cat modalias pci:v8086d104Bsv1028sd01DBbc02sc00i00 This is the module alias that was used to load the driver. [EMAIL PROTECTED]:/sys/bus/pci/devices/:00:19.0$ ls -l driver lrwxrwxrwx 1 root root 0 2008-07-01 08:52 driver - ../../../bus/pci/drivers/e1000e The driver is always e1000e no matter which alias was used to load it. e1000e is controled by the name field of the driver structure. That's the publicly visible name for the driver. Adding the OF aliases would change the modalias entry, not the driver name. The i2c implementation is adding a field to a device entry that contains the driver name. No other device drivers I could find do this. [EMAIL PROTECTED]:/sys/bus/i2c/devices/1-0050$ ls bus driver eeprom modalias name power subsystem uevent [EMAIL PROTECTED]:/sys/bus/i2c/devices/1-0050$ cat name eeprom [EMAIL PROTECTED]:/sys/bus/i2c/devices/1-0050$ ls -l driver lrwxrwxrwx 1 root root 0 2008-07-01 14:05 driver - ../../../../bus/i2c/drivers/eeprom [EMAIL PROTECTED]:/sys/bus/i2c/devices/1-0050$ cat modalias [EMAIL PROTECTED]:/sys/bus/i2c/devices/1-0050$ I believe the correct way to get the driver name from sysfs is to follow the driver link. The name field is probably legacy. Other drivers in the system don't have a name entry on the device node. Is the user space i2c code looking at the modalias entry? -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] [PATCH] Export the i2c_bus_type symbol, standalone V1
Export the root of the i2c bus so that PowerPC device tree code can iterate over devices on the i2c bus. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- drivers/i2c/i2c-core.c |3 ++- include/linux/i2c.h|3 +++ 2 files changed, 5 insertions(+), 1 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index d0175f4..05866ef 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -208,7 +208,7 @@ static struct device_attribute i2c_dev_attrs[] = { { }, }; -static struct bus_type i2c_bus_type = { +struct bus_type i2c_bus_type = { .name = i2c, .dev_attrs = i2c_dev_attrs, .match = i2c_device_match, @@ -219,6 +219,7 @@ static struct bus_type i2c_bus_type = { .suspend= i2c_device_suspend, .resume = i2c_device_resume, }; +EXPORT_SYMBOL_GPL(i2c_bus_type); /** diff --git a/include/linux/i2c.h b/include/linux/i2c.h index fb9af6a..186b22d 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -92,6 +92,9 @@ extern s32 i2c_smbus_read_i2c_block_data(struct i2c_client * client, extern s32 i2c_smbus_write_i2c_block_data(struct i2c_client * client, u8 command, u8 length, const u8 *values); + +/* Base of the i2c bus */ +extern struct bus_type i2c_bus_type; /* * A driver is capable of handling one or more physical devices present on ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] Aliases and device names for i2c
Is the problem is stemming from having the i2c drivers support multiple similar but not exactly the same chips? These similar chips are being exposed as aliases which they really aren't; they are actually different chips. A solution to this is for the drivers to register themselves multiple times, one for each different chip. Doing that frees up aliases for true aliases which is what device trees need. That also resolve the issue of needing a name attribute in the sysfs device node. We shouldn't need that attribute. By registering multiple times, the link from the device to the driver will contain the correct chip id. Of course we can still make a legacy name attribute until user space is converted. With this structure modalias will contain the alias used to load the driver. The driver link will have pfc856?.name.name pfc856?.name.name can also be use to create the name attribute. static const struct i2c_device_id pcf8563_id[] = { { pcf8563, 0 }, { samsung,pcf8563, 0 }, { } }; MODULE_DEVICE_TABLE(i2c, pcf8563_id); static struct i2c_driver pcf8563_driver = { .driver = { .name = pcf8563, }, .probe = pcf8563_probe, .remove = pcf8563_remove, .id_table = pcf8563_id, }; static int __init pcf8563_init(void) { return i2c_add_driver(pcf8563_driver); } static void __exit pcf8563_exit(void) { i2c_del_driver(pcf8563_driver); } static const struct i2c_device_id pcf8563_id[] = { { rtc8564, 0 }, { dallas,rtc8564, 0 }, { } }; MODULE_DEVICE_TABLE(i2c, pcf8563_id); static struct i2c_driver pcf8563_driver = { .driver = { .name = pcf8564, }, .probe = pcf8563_probe, .remove = pcf8563_remove, .id_table = pcf8564_id, }; static int __init pcf8564_init(void) { return i2c_add_driver(pcf8564_driver); } static void __exit pcf8564_exit(void) { i2c_del_driver(pcf8564_driver); } -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] [PATCH] Convert i2c-mpc from a platform driver into a of_platform driver, V2
This version adds of_find_i2c_device_by_node() which is needed by ASOC drivers. To make it work I had to make the i2c bus struct visible. It also picks up a couple of other minor fixes that have been posted to the lists. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- arch/powerpc/sysdev/fsl_soc.c | 122 - drivers/i2c/busses/i2c-mpc.c | 104 --- drivers/i2c/i2c-core.c|2 - drivers/of/of_i2c.c | 37 +--- include/linux/i2c.h |3 + include/linux/of_i2c.h|2 + 6 files changed, 92 insertions(+), 178 deletions(-) diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c index 019657c..ebcec73 100644 --- a/arch/powerpc/sysdev/fsl_soc.c +++ b/arch/powerpc/sysdev/fsl_soc.c @@ -414,128 +414,6 @@ err: arch_initcall(gfar_of_init); -#ifdef CONFIG_I2C_BOARDINFO -#include linux/i2c.h -struct i2c_driver_device { - char*of_device; - char*i2c_type; -}; - -static struct i2c_driver_device i2c_devices[] __initdata = { - {ricoh,rs5c372a, rs5c372a}, - {ricoh,rs5c372b, rs5c372b}, - {ricoh,rv5c386, rv5c386}, - {ricoh,rv5c387a, rv5c387a}, - {dallas,ds1307, ds1307}, - {dallas,ds1337, ds1337}, - {dallas,ds1338, ds1338}, - {dallas,ds1339, ds1339}, - {dallas,ds1340, ds1340}, - {stm,m41t00, m41t00}, - {dallas,ds1374, ds1374}, -}; - -static int __init of_find_i2c_driver(struct device_node *node, -struct i2c_board_info *info) -{ - int i; - - for (i = 0; i ARRAY_SIZE(i2c_devices); i++) { - if (!of_device_is_compatible(node, i2c_devices[i].of_device)) - continue; - if (strlcpy(info-type, i2c_devices[i].i2c_type, - I2C_NAME_SIZE) = I2C_NAME_SIZE) - return -ENOMEM; - return 0; - } - return -ENODEV; -} - -static void __init of_register_i2c_devices(struct device_node *adap_node, - int bus_num) -{ - struct device_node *node = NULL; - - while ((node = of_get_next_child(adap_node, node))) { - struct i2c_board_info info = {}; - const u32 *addr; - int len; - - addr = of_get_property(node, reg, len); - if (!addr || len sizeof(int) || *addr (1 10) - 1) { - printk(KERN_WARNING fsl_soc.c: invalid i2c device entry\n); - continue; - } - - info.irq = irq_of_parse_and_map(node, 0); - if (info.irq == NO_IRQ) - info.irq = -1; - - if (of_find_i2c_driver(node, info) 0) - continue; - - info.addr = *addr; - - i2c_register_board_info(bus_num, info, 1); - } -} - -static int __init fsl_i2c_of_init(void) -{ - struct device_node *np; - unsigned int i = 0; - struct platform_device *i2c_dev; - int ret; - - for_each_compatible_node(np, NULL, fsl-i2c) { - struct resource r[2]; - struct fsl_i2c_platform_data i2c_data; - const unsigned char *flags = NULL; - - memset(r, 0, sizeof(r)); - memset(i2c_data, 0, sizeof(i2c_data)); - - ret = of_address_to_resource(np, 0, r[0]); - if (ret) - goto err; - - of_irq_to_resource(np, 0, r[1]); - - i2c_dev = platform_device_register_simple(fsl-i2c, i, r, 2); - if (IS_ERR(i2c_dev)) { - ret = PTR_ERR(i2c_dev); - goto err; - } - - i2c_data.device_flags = 0; - flags = of_get_property(np, dfsrr, NULL); - if (flags) - i2c_data.device_flags |= FSL_I2C_DEV_SEPARATE_DFSRR; - - flags = of_get_property(np, fsl5200-clocking, NULL); - if (flags) - i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200; - - ret = - platform_device_add_data(i2c_dev, i2c_data, -sizeof(struct - fsl_i2c_platform_data)); - if (ret) - goto unreg; - - of_register_i2c_devices(np, i++); - } - - return 0; - -unreg: - platform_device_unregister(i2c_dev); -err: - return ret; -} - -arch_initcall(fsl_i2c_of_init); -#endif #ifdef CONFIG_PPC_83xx static int __init mpc83xx_wdt_init(void) diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index a076129..36bea49 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -17,7 +17,8 @@ #include linux/module.h #include
[i2c] FInding the right hwmon driver on x86
Does ACPI or something like that have a field containing the name of the hardware monitor chip? If so, we can do the same thing the powerpc code is doing and dynamically load the right module. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] Fwd: rtc pfc8563 driver lost a chip id
I think this got dropped during the conversion to the new probe signature. From the Kconfig... config RTC_DRV_PCF8563 tristate Philips PCF8563/Epson RTC8564 help If you say yes here you get support for the Philips PCF8563 RTC chip. The Epson RTC8564 should work as well. This driver can also be built as a module. If so, the module will be called rtc-pcf8563. -- Forwarded message -- From: Jean Delvare [EMAIL PROTECTED] Date: Jun 11, 2008 3:04 AM Subject: Re: [i2c] rtc pfc8563 driver lost a chip id To: Jon Smirl [EMAIL PROTECTED] Cc: Linux I2C i2c@lm-sensors.org Hi Jon, On Tue, 10 Jun 2008 18:22:50 -0400, Jon Smirl wrote: It does the rtc8564 too. diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c index 0fc4c36..748a502 100644 --- a/drivers/rtc/rtc-pcf8563.c +++ b/drivers/rtc/rtc-pcf8563.c @@ -302,6 +302,7 @@ static int pcf8563_remove(struct i2c_client *client) static const struct i2c_device_id pcf8563_id[] = { { pcf8563, 0 }, + { rtc8564, 0 }, { } }; MODULE_DEVICE_TABLE(i2c, pcf8563_id); Please check MAINTAINERS for the right person and list to send this patch to. -- Jean Delvare -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On 6/11/08, Wolfram Sang [EMAIL PROTECTED] wrote: On Tue, Jun 10, 2008 at 10:40:45PM -0400, Jon Smirl wrote: Convert i2c-mpc from a platform driver into an of_platform driver. This patch is much smaller since Jochen already added of_find_i2c_driver(). Versions of this have been posted before. Signed-ff-by: Jon Smirl [EMAIL PROTECTED] Typo: Signed-off... (I'm curious, do such typos enforce resending the patch?) I just cut and pasted this version to get the comments. Next pass I will send it using stgit which will add the right signed-off line and fix the wrapping. Tested-by: Wolfram Sang [EMAIL PROTECTED] -- drivers/i2c/busses/i2c-mpc.c | 105 +- 1 files changed, 63 insertions(+), 42 deletions(-) diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index a076129..76d8091 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -18,6 +18,8 @@ #include linux/sched.h #include linux/init.h #include linux/platform_device.h +#include linux/of_platform.h +#include linux/of_i2c.h #include asm/io.h #include linux/fsl_devices.h @@ -25,13 +27,13 @@ #include linux/interrupt.h #include linux/delay.h -#define MPC_I2C_ADDR 0x00 +#define DRV_NAME mpc-i2c + #define MPC_I2C_FDR 0x04 #define MPC_I2C_CR 0x08 #define MPC_I2C_SR 0x0c #define MPC_I2C_DR 0x10 #define MPC_I2C_DFSRR 0x14 -#define MPC_I2C_REGION 0x20 #define CCR_MEN 0x80 #define CCR_MIEN 0x40 @@ -315,102 +317,121 @@ static struct i2c_adapter mpc_ops = { .timeout = 1, }; -static int fsl_i2c_probe(struct platform_device *pdev) +static int fsl_i2c_probe(struct of_device *op, const struct of_device_id *match) The above two lines have to be concatenated, otherwise the patch will not apply. Also, __devinit? { int result = 0; struct mpc_i2c *i2c; - struct fsl_i2c_platform_data *pdata; - struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0); - - pdata = (struct fsl_i2c_platform_data *) pdev-dev.platform_data; i2c = kzalloc(sizeof(*i2c), GFP_KERNEL); if (!i2c) return -ENOMEM; - i2c-irq = platform_get_irq(pdev, 0); - if (i2c-irq 0) - i2c-irq = NO_IRQ; /* Use polling */ + if (of_get_property(op-node, dfsrr, NULL)) + i2c-flags |= FSL_I2C_DEV_SEPARATE_DFSRR; - i2c-flags = pdata-device_flags; - init_waitqueue_head(i2c-queue); + if (of_device_is_compatible(op-node, mpc5200-i2c)) + i2c-flags |= FSL_I2C_DEV_CLOCK_5200; - i2c-base = ioremap((phys_addr_t)r-start, MPC_I2C_REGION); + init_waitqueue_head(i2c-queue); + i2c-base = of_iomap(op-node, 0); if (!i2c-base) { printk(KERN_ERR i2c-mpc - failed to map controller\n); result = -ENOMEM; goto fail_map; } - if (i2c-irq != NO_IRQ) - if ((result = request_irq(i2c-irq, mpc_i2c_isr, - IRQF_SHARED, i2c-mpc, i2c)) 0) { - printk(KERN_ERR -i2c-mpc - failed to attach interrupt\n); - goto fail_irq; - } + i2c-irq = irq_of_parse_and_map(op-node, 0); + if (i2c-irq == NO_IRQ) { + result = -ENXIO; Minor thing, but I wonder if -EINVAL is more appropriate? + goto fail_irq; + } + + result = request_irq(i2c-irq, mpc_i2c_isr, + IRQF_SHARED, i2c-mpc, i2c); + if (result 0) { + printk(KERN_ERR i2c-mpc - failed to attach interrupt\n); + goto fail_request; + } mpc_i2c_setclock(i2c); - platform_set_drvdata(pdev, i2c); + + dev_set_drvdata(op-dev, i2c); i2c-adap = mpc_ops; - i2c-adap.nr = pdev-id; i2c_set_adapdata(i2c-adap, i2c); - i2c-adap.dev.parent = pdev-dev; - if ((result = i2c_add_numbered_adapter(i2c-adap)) 0) { + i2c-adap.dev.parent = op-dev; + + result = i2c_add_adapter(i2c-adap); + if (result 0) { printk(KERN_ERR i2c-mpc - failed to add adapter\n); goto fail_add; } + of_register_i2c_devices(i2c-adap, op-node); return result; - fail_add: - if (i2c-irq != NO_IRQ) - free_irq(i2c-irq, i2c); - fail_irq: + fail_add: + dev_set_drvdata(op-dev, NULL); + free_irq(i2c-irq, i2c); + fail_request: + irq_dispose_mapping(i2c-irq); + fail_irq: iounmap(i2c-base); - fail_map: + fail_map: kfree(i2c); return result; }; -static int
[i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
Convert i2c-mpc from a platform driver into an of_platform driver. This patch is much smaller since Jochen already added of_find_i2c_driver(). Versions of this have been posted before. Signed-ff-by: Jon Smirl [EMAIL PROTECTED] -- drivers/i2c/busses/i2c-mpc.c | 105 +- 1 files changed, 63 insertions(+), 42 deletions(-) diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index a076129..76d8091 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -18,6 +18,8 @@ #include linux/sched.h #include linux/init.h #include linux/platform_device.h +#include linux/of_platform.h +#include linux/of_i2c.h #include asm/io.h #include linux/fsl_devices.h @@ -25,13 +27,13 @@ #include linux/interrupt.h #include linux/delay.h -#define MPC_I2C_ADDR 0x00 +#define DRV_NAME mpc-i2c + #define MPC_I2C_FDR0x04 #define MPC_I2C_CR 0x08 #define MPC_I2C_SR 0x0c #define MPC_I2C_DR 0x10 #define MPC_I2C_DFSRR 0x14 -#define MPC_I2C_REGION 0x20 #define CCR_MEN 0x80 #define CCR_MIEN 0x40 @@ -315,102 +317,121 @@ static struct i2c_adapter mpc_ops = { .timeout = 1, }; -static int fsl_i2c_probe(struct platform_device *pdev) +static int fsl_i2c_probe(struct of_device *op, const struct of_device_id *match) { int result = 0; struct mpc_i2c *i2c; - struct fsl_i2c_platform_data *pdata; - struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0); - - pdata = (struct fsl_i2c_platform_data *) pdev-dev.platform_data; i2c = kzalloc(sizeof(*i2c), GFP_KERNEL); if (!i2c) return -ENOMEM; - i2c-irq = platform_get_irq(pdev, 0); - if (i2c-irq 0) - i2c-irq = NO_IRQ; /* Use polling */ + if (of_get_property(op-node, dfsrr, NULL)) + i2c-flags |= FSL_I2C_DEV_SEPARATE_DFSRR; - i2c-flags = pdata-device_flags; - init_waitqueue_head(i2c-queue); + if (of_device_is_compatible(op-node, mpc5200-i2c)) + i2c-flags |= FSL_I2C_DEV_CLOCK_5200; - i2c-base = ioremap((phys_addr_t)r-start, MPC_I2C_REGION); + init_waitqueue_head(i2c-queue); + i2c-base = of_iomap(op-node, 0); if (!i2c-base) { printk(KERN_ERR i2c-mpc - failed to map controller\n); result = -ENOMEM; goto fail_map; } - if (i2c-irq != NO_IRQ) - if ((result = request_irq(i2c-irq, mpc_i2c_isr, - IRQF_SHARED, i2c-mpc, i2c)) 0) { - printk(KERN_ERR - i2c-mpc - failed to attach interrupt\n); - goto fail_irq; - } + i2c-irq = irq_of_parse_and_map(op-node, 0); + if (i2c-irq == NO_IRQ) { + result = -ENXIO; + goto fail_irq; + } + + result = request_irq(i2c-irq, mpc_i2c_isr, + IRQF_SHARED, i2c-mpc, i2c); + if (result 0) { + printk(KERN_ERR i2c-mpc - failed to attach interrupt\n); + goto fail_request; + } mpc_i2c_setclock(i2c); - platform_set_drvdata(pdev, i2c); + + dev_set_drvdata(op-dev, i2c); i2c-adap = mpc_ops; - i2c-adap.nr = pdev-id; i2c_set_adapdata(i2c-adap, i2c); - i2c-adap.dev.parent = pdev-dev; - if ((result = i2c_add_numbered_adapter(i2c-adap)) 0) { + i2c-adap.dev.parent = op-dev; + + result = i2c_add_adapter(i2c-adap); + if (result 0) { printk(KERN_ERR i2c-mpc - failed to add adapter\n); goto fail_add; } + of_register_i2c_devices(i2c-adap, op-node); return result; - fail_add: - if (i2c-irq != NO_IRQ) - free_irq(i2c-irq, i2c); - fail_irq: + fail_add: + dev_set_drvdata(op-dev, NULL); + free_irq(i2c-irq, i2c); + fail_request: + irq_dispose_mapping(i2c-irq); + fail_irq: iounmap(i2c-base); - fail_map: + fail_map: kfree(i2c); return result; }; -static int fsl_i2c_remove(struct platform_device *pdev) +static int fsl_i2c_remove(struct of_device *op) { - struct mpc_i2c *i2c = platform_get_drvdata(pdev); + struct mpc_i2c *i2c = dev_get_drvdata(op-dev); i2c_del_adapter(i2c-adap); - platform_set_drvdata(pdev, NULL); + dev_set_drvdata(op-dev, NULL); if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); + irq_dispose_mapping(i2c-irq); iounmap(i2c-base); kfree(i2c); return 0; }; -/* work with hotplug and coldplug */ -MODULE_ALIAS(platform:fsl-i2c); +static const struct of_device_id mpc_i2c_of_match[] = { + { + .compatible = fsl-i2c, + }, + {}, +}; +MODULE_DEVICE_TABLE(of, mpc_i2c_of_match); + /* Structure for a device driver */ -static struct
Re: [i2c] [PATCH 0/4] i2c: Introduce i2c listeners
On 6/5/08, Jean Delvare [EMAIL PROTECTED] wrote: Doesn't something like this work? (obviously this isn't complete) for (address = start; address = stop;) { make a new client for (; address =stop; address++) { client-address = address; res = device_register(client-dev); if (res != -ENODEV) break; } } delete the extra client You don't seriously propose to scan all addresses on every I2C bus for every i2c chip driver that we load, do you? This would be awfully slow and dangerous. No way. This is just a small part of the code, you need to add in all the other parts from the patch. Exactly the same addresses would be scanned under each scheme. .My question is, does the driver need two entry points (detect and probe) or could only one (probe) be used? I think detect and probe can be the same function. With the detect() function it would look somthing like this: for (address = start; address = stop;) { for (; address =stop; address++) { do a generic detect if (driver-detect(address) { make a new client client-address = address; res = device_register(client-dev); if (!res) delete the extra client } } } -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 0/4] i2c: Introduce i2c listeners
On 6/5/08, David Brownell [EMAIL PROTECTED] wrote: On Wednesday 04 June 2008, Jon Smirl wrote: Now that we've had this discussion it looks to me that the scan capability should be added to struct i2c_driver instead of being standalone. I guess what is a little strange to me is that the pre-probe() function is like a static driver function and probe() is an instance function. The problem there is: how do you know which pre-probe() function(s) to try? If you could answer that reliably you'd be able to probe() directly. If you can't, you'd need to load all I2C driver modules until one works... not the desired system model. I see now that the adapter class should be added to the driver name alias string. The we could load all modules of a given class. But I don't think we need to go searching for the right module to load since i2c is not generally a dynamic bus. If you install a video card with an i2c bus, the driver for the video card can just load_module the right i2c driver modules. I do wonder if we could get hardware monitors to autoload. When the adapter driver for the motherboard loads, it could load module all drivers of class hardware_monitor. Then each one could do its detection thing. Unload the ones that weren't found. That would really help people who can figure out the right driver module to load. You could even get fancy and generate a uevent after the right module was found. A script in the uevent could save the info so you don't have to repeat on each boot. Of course this assume that the hardware monitor modules can reliably autodetect. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 0/4] i2c: Introduce i2c listeners
On 6/5/08, Jean Delvare [EMAIL PROTECTED] wrote: On Thu, 5 Jun 2008 10:55:28 -0400, Jon Smirl wrote: I see now that the adapter class should be added to the driver name alias string. The we could load all modules of a given class. Ah ah. Oh wait, you were serious? Come on, why would anyone on earth want to load all modules of a given class? But I don't think we need to go searching for the right module to load since i2c is not generally a dynamic bus. If you install a video card with an i2c bus, the driver for the video card can just load_module the right i2c driver modules. Sometimes yes, sometimes no. For most V4L/DVB, the main driver should know which i2c chip drivers it needs, but unfortunately for old drivers such as bttv this knowledge has never been recorded, so there is no alternative to (at least partial) probing in practice. For video adapters, they could load the eeprom driver as they all give access to EDID EEPROMs, but if the card also has a hardware monitoring chip, we simply don't know what it is. We just can't record in the kernel drivers what hardware monitoring chip exists on thousands of cards out there (same problem as for motherboards.) I do wonder if we could get hardware monitors to autoload. Only if they don't do the device detection/identification themselves. I've considered it for a moment but now I'm fairly certain that it would be a dangerous and unmaintainable mess. Better let each driver include its detection/identification routine, and delegate the responsibility of loading the right driver to user-space (as is already the case.) When the adapter driver for the motherboard loads, it could load module all drivers of class hardware_monitor. No, really, forget about this, that's simply never going to happen. That's way too costly and dangerous. Then each one could do its detection thing. Unload the ones that weren't found. And do it all over again when the graphics adapter driver is loaded? And when an parport-to-I2C or USB-to-I2C adapter is plugged? That would really help people who can figure out the right driver module to load. If people can't run sensors-detect there's not much we can do for them ;) What about live CDs and things like that? I do agree that there is no immediate need to do something like this. But in the long run a scheme like this could eliminate the need for sensors-detect. It doesn't take as long to load/unload the modules as you might think, they would all be on an initrd. The probing time should be the same as the sensors-detect script. Maintenance would be better since the detection code would only exist in one place instead of two. File this away to think about for the future. After the drivers are converted to the new model and have the ability to detect this can be experimented with. You could even get fancy and generate a uevent after the right module was found. A script in the uevent could save the info so you don't have to repeat on each boot. Of course this assume that the hardware monitor modules can reliably autodetect. You mean, instead of just using the sensors-detect script which we already have and has worked fine for (almost) everyone for 8 years now? And the few cases where sensors-detect didn't work are a very good reason to _not_ attempt to do the same in the kernel. SMBus probing is _very_ tricky and we really want to control everything that happens. -- Jean Delvare -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 0/4] i2c: Introduce i2c listeners
On 6/4/08, Jean Delvare [EMAIL PROTECTED] wrote: Hi all, This patch set demonstrates a new concept which I would like to add to the i2c subsystem. It is named i2c listeners and is based on the following structure: How does this fit in with the existing driver bus structure in kernel? This sounds a lot like hot plug and buses already implement hot plug events. This isn't exactly hot plug but it may be close enough that the existing bus APIs will work. Note that both devices and entire buses can be hot plugged with existing code. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 0/4] i2c: Introduce i2c listeners
On 6/4/08, Jean Delvare [EMAIL PROTECTED] wrote: On Wed, 4 Jun 2008 15:28:54 -0400, Jon Smirl wrote: On 6/4/08, Jon Smirl [EMAIL PROTECTED] wrote: On 6/4/08, Jean Delvare [EMAIL PROTECTED] wrote: Hi all, This patch set demonstrates a new concept which I would like to add to the i2c subsystem. It is named i2c listeners and is based on the following structure: Could the existing API be used something like this? Drivers register with i2c core using standard driver registration. Class and address ranges are exposed in a structure like PCI IDs. I2C can make up it's own ID structure equivalent to the PCI IDs one. No, no, no. I've been repeating it many times already: I2C device addresses are NOT IDs. If you're even thinking of going in this direction, you're plain wrong, sorry. Many devices share the same address, and many devices can use different addresses. The address is really only one attribute of an I2C device, amongst many others. It's not an identifier. I know these aren't identifiers, I'm proposing that they be ranges of address to scan. The driver model doesn't force these to be IDs, but in most cases they are IDs. To the driver model it's just a pointer to a struct, no interpretation is done of the structure. It's the PCI bus driver that interprets these structs as identifiers. In our case the bus driver is inside i2c core and it interprets the structs as address ranges and then calls the probe function sequentially with the different value. I shouldn't have compared this to PCI IDs, I was just thinking of it as data that gets processed by i2c core. PCI IDs are an example of data that is processed by the PCI core. Their meaning as ids is supplied by PCI core, not the kernel driver framework. Please also realize that we _do_ have IDs for I2C devices by now. As there are no standard numbers as PCI has, we use arbitrary strings (typically the device name). That's what new-style drivers use for device binding since kernel 2.6.26. And you can't have more than one ID format for a given subsystem, and we don't want to break new-style drivers, so thinking about a radically different form of IDs for the i2c subsystem is a waste of time: it's simply not going to happen. When a new adapter/bus is created i2c core gets the event. Core then Side question: why would i2c-core want events when adapters are created, given that it does create them itself? I used 'event' as a generic term for meaning the driver core was aware that the adapter was created. scans the registered driver structures looking for class matches. When a match is encountered it repeatedly calls the driver's probe function with the addresses from the range. Driver returns true/false on each probe which triggers creation of the device. The driver's probe function is, by definition of the device driver model, called on an existing device. So the probe function cannot decide whether the device should be created or not - it's too late at this point. I don't believe this is has to be true although usually it is. A driver is passed in information from the bus core on the probe function. It then looks at that information and decides if it wants to bind. Drivers return true/false if they want to bind. If they return false the device structure is destroyed (or reused for the next probe call). There is nothing in the model that requires an actual device to be present for a probe call to happen. A perfectly valid reason for refusing to bind would be not having hardware present at the address. This is how older ISA drivers are loaded that predate PNP. They search for the hardware in the probe function. That's probably why it is called probe but that predates my involvement in Linux. One thing that we could consider though is merging i2c_listener into i2c_driver. For some reason I had not considered it - but I somehow expect funny locking issues if we do that. We are not far apart on this proposal if you combine the class/address_data fields into i2c_driver and then use the existing probe function instead of detect(). The i2c_client struct already has an address field. A driver would then check to see if there was hardware at the i2c_client.address and return false if it is not there. i2c core would just reuse the i2c_client struct with the next address and do another probe. Operations on an adapter need to be serialized but I thought the adapters already supported that. If they don't they need to be fixed anyway because of the BKL removal. Why do drivers need the bus creation (attach/detach adapter) events? Originally, to probe the adapters in search of devices to support. I seem to remember that some V4L/DVB drivers do more than that in their attach_adapter callback, but I need to take a closer look. And i2c-dev deserves particular attention, as it needs to do bookkeeping of all the adapters that are present
Re: [i2c] [PATCH 0/4] i2c: Introduce i2c listeners
On 6/4/08, David Brownell [EMAIL PROTECTED] wrote: It then looks at that information and decides if it wants to bind. Drivers return true/false if they want to bind. Wrong. The probe returns zero if it bound, else negative errno. Those functions don't return booleans. You guys are so literal. I should have said return 0 or -ENODEV or some other error. If they return false the device structure is destroyed (or reused for the next probe call). No. The struct device is not destroyed at that time for ANY driver model code I've seen in Linux. Destroying it would prevent driver modules from binding to that device when they get loaded later. Don't they get destroyed when hotplug devices are removed from the system? Could this be considered a hotplug, create struct device, probe to see if it is claimed, unhotplug if not claimed, destroy struct device. But I agree with what you meant, that they never get destroyed when there is known hardware backing them in the system. They search for the hardware in the probe function. That's probably why it is called probe but that predates my involvement in Linux. And that legacy model is something that's getting removed everywhere practical. Such legacy drivers don't support hotplugging or most of the other mechanisms defined by the driver model, since they have the driver creating the device structs (instead of bus infrastructure). Those drivers are getting removed because the hardware is disappearing and we have ACPI/PNP now to tell if if is present, not because they misused the driver code. We just don't need the old style probing code anymore and it was unreliable. There is is a parallel here. With PowerPC device trees I can put the exact address of the i2c device into the board specific device tree. Now there's no need to probe addresses. That's equivalent to what's happening with ACPI and serial ports. Maybe we'll implement device trees on all archs and then we can get rid of i2c device probing. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 0/4] i2c: Introduce i2c listeners
On 6/4/08, David Brownell [EMAIL PROTECTED] wrote: On Wednesday 04 June 2008, Trent Piepho wrote: Couldn't you say the probe function is called on a potential device? The probe function can return -ENODEV, in which can other driver's probes get called, and it's perfectly ok if no driver binds to it. The way PCI works, is that when a new pci bus is created, each address is probed ... by config space accessors which all PCI devices support. I made a mistake comparing this to PCI IDs. I should not have used PCI IDs as an example and gotten us off on the tangent of not having a global i2c device namespace. Adding the address range to search is an unrelated problem to the name space issue. and a device is created if anything responds. The generic bus code tries to match each device to a driver or drivers ... using a formally managed set of product identifiers. and calls those drivers' probe functions. The drivers don't have to claim the device in the probe function. The bus code handles all the cases of a driver or bus getting added or removed in various orders. So why can't I2C do this too? No such product identifiers, and in general no way to tell what's sitting at a given address. And in fact, there's no sure way to tell if a device is present there, since when an I2C device is busy, it's not required to ack its address. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 2/3] i2c: Let bus drivers add SPD to their class
On 6/3/08, David Brownell [EMAIL PROTECTED] wrote: On Tuesday 03 June 2008, Jean Delvare wrote: Note that I took a conservative approach here, adding I2C_CLASS_SPD to all drivers that had I2C_CLASS_HWMON before. This is to make sure that the eeprom driver doesn't stop probing buses where SPD EEPROMs live. But the truth is that, for most of these, I simply have no idea whether they can host SPD EEPROMs or not. Few embedded platforms use discrete sticks of DRAM. My two cents: use the opposite default in those cases. I'll highlight a few below, where I happen to have more specific knowledge. So, bus driver maintainers and users should feel free to remove the SPD class from drivers those buses never have SPD EEPROMs or they don't want the eeprom driver to bind to them. Likewise, feel free to add the SPD class to any bus driver I might have missed. Signed-off-by: Jean Delvare [EMAIL PROTECTED] --- drivers/i2c/busses/i2c-ali1535.c |2 +- drivers/i2c/busses/i2c-ali1563.c |2 +- drivers/i2c/busses/i2c-ali15x3.c |2 +- drivers/i2c/busses/i2c-amd756.c|2 +- drivers/i2c/busses/i2c-amd8111.c |2 +- drivers/i2c/busses/i2c-at91.c |2 +- I've never heard of an AT91 board using DRAM sticks ... drivers/i2c/busses/i2c-cpm.c |2 +- drivers/i2c/busses/i2c-davinci.c |2 +- ... or a DaVinci one ... drivers/i2c/busses/i2c-elektor.c |2 +- drivers/i2c/busses/i2c-gpio.c |2 +- drivers/i2c/busses/i2c-i801.c |2 +- drivers/i2c/busses/i2c-ibm_iic.c |4 ++-- drivers/i2c/busses/i2c-iop3xx.c|2 +- drivers/i2c/busses/i2c-isch.c |2 +- drivers/i2c/busses/i2c-mpc.c |2 +- Freescale embedded, no sticks drivers/i2c/busses/i2c-mv64xxx.c |2 +- drivers/i2c/busses/i2c-nforce2.c |2 +- drivers/i2c/busses/i2c-ocores.c|2 +- drivers/i2c/busses/i2c-omap.c |2 +- ... or an OMAP one ... drivers/i2c/busses/i2c-parport-light.c |2 +- drivers/i2c/busses/i2c-parport.c |2 +- drivers/i2c/busses/i2c-pasemi.c|2 +- drivers/i2c/busses/i2c-piix4.c |2 +- drivers/i2c/busses/i2c-pmcmsp.c|2 +- drivers/i2c/busses/i2c-s3c2410.c |2 +- drivers/i2c/busses/i2c-sibyte.c|4 ++-- drivers/i2c/busses/i2c-sis5595.c |2 +- drivers/i2c/busses/i2c-sis630.c|2 +- drivers/i2c/busses/i2c-sis96x.c|2 +- drivers/i2c/busses/i2c-stub.c |2 +- drivers/i2c/busses/i2c-tiny-usb.c |2 +- ... DRAM-over-USB would be bizarre too ... drivers/i2c/busses/i2c-via.c |2 +- drivers/i2c/busses/i2c-viapro.c|2 +- drivers/i2c/busses/scx200_acb.c|2 +- include/linux/i2c.h|1 + 35 files changed, 37 insertions(+), 36 deletions(-) ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 2/3] i2c: Let bus drivers add SPD to their class
On 6/3/08, Trent Piepho [EMAIL PROTECTED] wrote: On Tue, 3 Jun 2008, Jon Smirl wrote: drivers/i2c/busses/i2c-elektor.c |2 +- drivers/i2c/busses/i2c-gpio.c |2 +- drivers/i2c/busses/i2c-i801.c |2 +- drivers/i2c/busses/i2c-ibm_iic.c |4 ++-- drivers/i2c/busses/i2c-iop3xx.c|2 +- drivers/i2c/busses/i2c-isch.c |2 +- drivers/i2c/busses/i2c-mpc.c |2 +- Freescale embedded, no sticks I know you're wrong about this one, because I have a Freescale board with an mpc i2c adapter with SPD SODIMMs on the I2C bus sitting right in front of me. The whole reason I added hexdump support the I2C tools' SPD parser was so I could parse the SPD data by hexdumping the eeproms with busybox. Which CPU are you using? Maybe we should change to a socket. Using SPD with embedded boards isn't uncommon. It's much easier to design a board with a SO-DIMM slot than actual chips. In small quantities at least, it's cheaper too. Using SPD is much more flexiable when you want to change memory size or speed, even if the memory isn't socketed. If you look at U-Boot, most platforms are using SPD for DRAM controller setup and not hard coded values. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] i2c: Push ioctl BKL down into the i2c code
On 5/23/08, Alan Cox [EMAIL PROTECTED] wrote: On Fri, 23 May 2008 09:35:45 +0200 Jean Delvare [EMAIL PROTECTED] wrote: Hi Alan, On Thu, 22 May 2008 22:23:27 +0100, Alan Cox wrote: Signed-off-by: Alan Cox [EMAIL PROTECTED] Description of what the patch does and why it is needed, please. I can't apply it without that. My first impression is a patch making the code bigger and more complex with no obvious benefit ;) Alan, for people not familiar with the BKL attaching a write up or pointer to the patches on the recommended ways to convert these locks to something else would help. Some embedded developers won't know what to do with these patches and they don't follow lkml. It pushes the BKL down into the i2c driver. The intention is to remove the big kernel lock ioctl method from the file_operations structure so that we can work on getting rid of the big kernel lock for good. It's one of a series of patches that give me an x86-32 tree with no -ioctl method at all. Similar activity is going on for the other calls made under the BKL the goal being to push it down into drivers and then eliminate it. ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
On 2/19/08, Jean Delvare [EMAIL PROTECTED] wrote: i2c-irq = platform_get_irq(pdev, 0); - if (i2c-irq 0) { + if (i2c-irq NO_IRQ) { I am skeptical about this one. Can platform_get_irq() really return NO_IRQ? I thought that the IRQ resource would be plain missing if the device has no IRQ, so I would expect: i2c-irq = platform_get_irq(pdev, 0); if (i2c-irq 0) i2c-irq = NO_IRQ; /* Use polling */ Testing against NO_IRQ suggests that devices with no IRQ would still have an IRQ resource defined and explicitly set to NO_IRQ. Sounds weird to me. Can you please clarify this point? Your fix is correct. I'm not sure polling worked in the original driver. For what it's worth, no other kernel driver checks for irq NO_IRQ. They all check for irq 0 after calling platform_get_irq(). result = -ENXIO; goto fail_get_irq; } @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) goto fail_map; } - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) if ((result = request_irq(i2c-irq, mpc_i2c_isr, IRQF_SHARED, i2c-mpc, i2c)) 0) { printk(KERN_ERR @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) return result; fail_add: - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); fail_irq: iounmap(i2c-base); @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev) i2c_del_adapter(i2c-adap); platform_set_drvdata(pdev, NULL); - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); iounmap(i2c-base); The rest looks good. -- Jean Delvare -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
New version with your fix. diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index bbe787b..b141057 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing) u32 x; int result = 0; - if (i2c-irq == 0) + if (i2c-irq == NO_IRQ) { while (!(readb(i2c-base + MPC_I2C_SR) CSR_MIF)) { schedule(); @@ -329,10 +329,9 @@ static int fsl_i2c_probe(struct platform_device *pdev) return -ENOMEM; i2c-irq = platform_get_irq(pdev, 0); - if (i2c-irq 0) { - result = -ENXIO; - goto fail_get_irq; - } + if (i2c-irq 0) + i2c-irq = NO_IRQ; /* Use polling */ + i2c-flags = pdata-device_flags; init_waitqueue_head(i2c-queue); @@ -344,7 +343,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) goto fail_map; } - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) if ((result = request_irq(i2c-irq, mpc_i2c_isr, IRQF_SHARED, i2c-mpc, i2c)) 0) { printk(KERN_ERR @@ -367,7 +366,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) return result; fail_add: - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); fail_irq: iounmap(i2c-base); @@ -384,7 +383,7 @@ static int fsl_i2c_remove(struct platform_device *pdev) i2c_del_adapter(i2c-adap); platform_set_drvdata(pdev, NULL); - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); iounmap(i2c-base); -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
I attached the diff file. I had forgot that I renamed the file so it wasn't getting compiled. I compiled it this time. I've made too many other changes to it to test this version on my current hardware. -- Jon Smirl [EMAIL PROTECTED] diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index bbe787b..d73edef 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing) u32 x; int result = 0; - if (i2c-irq == 0) + if (i2c-irq == NO_IRQ) { while (!(readb(i2c-base + MPC_I2C_SR) CSR_MIF)) { schedule(); @@ -329,10 +329,9 @@ static int fsl_i2c_probe(struct platform_device *pdev) return -ENOMEM; i2c-irq = platform_get_irq(pdev, 0); - if (i2c-irq 0) { - result = -ENXIO; - goto fail_get_irq; - } + if (i2c-irq 0) + i2c-irq = NO_IRQ; /* Use polling */ + i2c-flags = pdata-device_flags; init_waitqueue_head(i2c-queue); @@ -344,7 +343,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) goto fail_map; } - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) if ((result = request_irq(i2c-irq, mpc_i2c_isr, IRQF_SHARED, i2c-mpc, i2c)) 0) { printk(KERN_ERR @@ -367,12 +366,11 @@ static int fsl_i2c_probe(struct platform_device *pdev) return result; fail_add: - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); fail_irq: iounmap(i2c-base); fail_map: - fail_get_irq: kfree(i2c); return result; }; @@ -384,7 +382,7 @@ static int fsl_i2c_remove(struct platform_device *pdev) i2c_del_adapter(i2c-adap); platform_set_drvdata(pdev, NULL); - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); iounmap(i2c-base); ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 1/2 v2] i2c: Add support for device alias names
On 4/29/08, Jean Delvare [EMAIL PROTECTED] wrote: Based on earlier work by Jon Smirl and Jochen Friedrich. +/* Looks like: i2c:S */ +static int do_i2c_entry(const char *filename, struct i2c_device_id *id, + char *alias) +{ + sprintf(alias, I2C_MODULE_PREFIX %s, id-name); Do you want to add the colon? + + return 1; +} -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
On 2/22/08, Jochen Friedrich [EMAIL PROTECTED] wrote: Hi Jean, +/* + * Wait for patch from Jon Smirl + * #include powerpc-common.h + */ It doesn't make sense to merge this comment upstream. I know you don't like the patch from Jon Smirl and you also explained your reasons. Fortunately, I2c no longer uses numeric device IDs but names. So what are the alternatives? 1. modify the I2c subsystem to accept OF names additionally to I2c names (proposed by Jon smirl). The correct statement is: modify the i2c subsystem to support the standard kernel driver aliasing mechanism. Leaving powerpc and OF out of the argument for the moment, i2c has a custom aliasing scheme on the x86 too. So as a first step, can we remove the custom i2c aliasing scheme and change i2c to use standard module aliases on the x86? Patches for this already exist. On 2/23/08, Jean Delvare [EMAIL PROTECTED] wrote: The problem I have with this is that it breaks compatibility. The chip name is not only used for device/driver matching, it is also exported to userspace as a sysfs attribute (name). Applications might rely on it. At least libsensors does. I think there is some confusion here. The OF aliases are only used by the kernel to load the correct driver. Would doing something like this help? static struct i2c_device_id pcf8563_id[] = { {pcf8563, 0, sysfs_legacy_name}, {rtc8564, 0, sysfs_legacy_name}, OF_ID(phillips,pcf8563, pcf8563_id[0], 0) OF_ID(epson,rtc8564, pcf8563_id[1], 0) {}, }; MODULE_DEVICE_TABLE(i2c, pcf8563_id); Then in the probe function you can use the pointer to find the base id entry and i2c never has to be aware that the OF alias exists. 2. record the I2c name in the dts tree, either as separate tag (like linux,i2c-name=i2c-name) Not really practical for the millions of machines (all PowerPC Macs) already shipped. or as additional compatible entry (like compatible=..., linux,i2c-name). 3. use a glue layer with a translation map. Audio codecs have exactly the same problem. There are probably other devices that also need mapping. This mapping table will need to contain a map from the OF names to the kernel driver names. It will need to stored permanently in RAM and contain all possible mappings. This table will only grow in size. The kernel has a widely used mechanism for mapping -- aliases in the device drivers. Why do we want to build a new, parallel one? What we are doing now is option 4. 4. Use kconfig to build custom kernels for each target system. Don't load drivers automatically based on what the BIOS tells us. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] Convert pfc8563 i2c driver from old style to new style
On 2/19/08, Jean Delvare [EMAIL PROTECTED] wrote: Hi Jon, On Mon, 21 Jan 2008 15:09:01 -0500, Jon Smirl wrote: Convert pfc8563 i2c driver from old style to new style. Let's just forget about this patch until the dynamic module loading support goes into the base. The Phytec PCM030 mpc5200 board uses this chip. They are staying out of tree because they've integrated real-time support which is not in mainline yet. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] [PATCH 3/3] Add device tree compatible aliases to i2c drivers
PowerPC device trees use a different naming convention than the Linux kernel. Provide alias names for i2c drivers in order to allow them to be loaded by device tree name. The OF_ID macro ensures that the aliases are only present in powerpc builds and separated into their own namespace. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- drivers/hwmon/f75375s.c |2 ++ drivers/i2c/chips/ds1682.c |1 + drivers/i2c/chips/menelaus.c |1 + drivers/i2c/chips/tps65010.c |4 drivers/i2c/chips/tsl2550.c |1 + drivers/rtc/rtc-ds1307.c |6 ++ drivers/rtc/rtc-ds1374.c |1 + drivers/rtc/rtc-m41t80.c |8 drivers/rtc/rtc-pcf8563.c|2 ++ drivers/rtc/rtc-rs5c372.c|4 10 files changed, 30 insertions(+), 0 deletions(-) diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c index 4cb4db4..dd548e7 100644 --- a/drivers/hwmon/f75375s.c +++ b/drivers/hwmon/f75375s.c @@ -132,6 +132,8 @@ static struct i2c_driver f75375_legacy_driver = { static const struct i2c_device_id f75375_id[] = { { f75373, f75373 }, { f75375, f75375 }, + OF_ID(fintek,f75373, f75373) + OF_ID(fintek,f75375, f75375) { }, }; MODULE_DEVICE_TABLE(i2c, f75375_id); diff --git a/drivers/i2c/chips/ds1682.c b/drivers/i2c/chips/ds1682.c index 51ff518..817ad1f 100644 --- a/drivers/i2c/chips/ds1682.c +++ b/drivers/i2c/chips/ds1682.c @@ -237,6 +237,7 @@ static int ds1682_remove(struct i2c_client *client) static const struct i2c_device_id ds1682_id[] = { { ds1682, 0 }, + OF_ID(dallas,ds1682, 0) { }, }; MODULE_DEVICE_TABLE(i2c, ds1682_id); diff --git a/drivers/i2c/chips/menelaus.c b/drivers/i2c/chips/menelaus.c index 1f9ac5e..ba7d619 100644 --- a/drivers/i2c/chips/menelaus.c +++ b/drivers/i2c/chips/menelaus.c @@ -1245,6 +1245,7 @@ static int __exit menelaus_remove(struct i2c_client *client) static const struct i2c_device_id menelaus_id[] = { { menelaus, 0 }, + OF_ID(ti,twl92330, 0) { }, }; MODULE_DEVICE_TABLE(i2c, menelaus_id); diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c index e07274d..9cd1770 100644 --- a/drivers/i2c/chips/tps65010.c +++ b/drivers/i2c/chips/tps65010.c @@ -571,6 +571,10 @@ static const struct i2c_device_id tps65010_id[] = { { tps65011, TPS65011 }, { tps65012, TPS65012 }, { tps65013, TPS65013 }, + OF_ID(ti,tps65010, TPS65010) + OF_ID(ti,tps65011, TPS65011) + OF_ID(ti,tps65012, TPS65012) + OF_ID(ti,tps65013, TPS65013) { }, }; MODULE_DEVICE_TABLE(i2c, tps65010_id); diff --git a/drivers/i2c/chips/tsl2550.c b/drivers/i2c/chips/tsl2550.c index 2add8be..8071d6d 100644 --- a/drivers/i2c/chips/tsl2550.c +++ b/drivers/i2c/chips/tsl2550.c @@ -454,6 +454,7 @@ static int tsl2550_resume(struct i2c_client *client) static const struct i2c_device_id tsl2550_id[] = { { tsl2550, 0 }, + OF_ID(taos,tsl2550, 0) { }, }; MODULE_DEVICE_TABLE(i2c, tsl2550_id); diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c index a5614ab..e363a5f 100644 --- a/drivers/rtc/rtc-ds1307.c +++ b/drivers/rtc/rtc-ds1307.c @@ -129,6 +129,12 @@ static const struct i2c_device_id ds1307_id[] = { { ds1339, ds_1339 }, { ds1340, ds_1340 }, { m41t00, m41t00 }, + OF_ID(dallas,ds1307, ds_1307) + OF_ID(dallas,ds1337, ds_1337) + OF_ID(dallas,ds1338, ds_1338) + OF_ID(dallas,ds1339, ds_1339) + OF_ID(dallas,ds1340, ds_1340) + OF_ID(stm,m41t00, m41t00) {}, }; MODULE_DEVICE_TABLE(i2c, ds1307_id); diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c index 376ceeb..e4f680a 100644 --- a/drivers/rtc/rtc-ds1374.c +++ b/drivers/rtc/rtc-ds1374.c @@ -43,6 +43,7 @@ static const struct i2c_device_id ds1374_id[] = { { ds1374, 0 }, + OF_ID(dallas,ds1374, 0) {}, }; MODULE_DEVICE_TABLE(i2c, ds1374_id); diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c index c672557..663f8b5 100644 --- a/drivers/rtc/rtc-m41t80.c +++ b/drivers/rtc/rtc-m41t80.c @@ -69,6 +69,14 @@ static const struct i2c_device_id m41t80_id[] = { { m41st84, M41T80_FEATURE_HT | M41T80_FEATURE_BL }, { m41st85, M41T80_FEATURE_HT | M41T80_FEATURE_BL }, { m41st87, M41T80_FEATURE_HT | M41T80_FEATURE_BL }, + OF_ID(stm,m41t80, 0) + OF_ID(stm,m41t81, M41T80_FEATURE_HT) + OF_ID(stm,m41t81s, M41T80_FEATURE_HT | M41T80_FEATURE_BL) + OF_ID(stm,m41t82, M41T80_FEATURE_HT | M41T80_FEATURE_BL) + OF_ID(stm,m41t83, M41T80_FEATURE_HT | M41T80_FEATURE_BL) + OF_ID(stm,m41st84, M41T80_FEATURE_HT | M41T80_FEATURE_BL) + OF_ID(stm,m41st85, M41T80_FEATURE_HT | M41T80_FEATURE_BL) + OF_ID(stm,m41st87, M41T80_FEATURE_HT | M41T80_FEATURE_BL) {}, }; MODULE_DEVICE_TABLE(i2c, m41t80_id); diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c index 8eff549..51d7471 100644
[i2c] [PATCH 1/3] Rename i2c-mpc to i2c-mpc-drv in preparation for breaking out common code.
Rename i2c-mpc to i2c-mpc-drv in preparation for breaking out common code. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- drivers/i2c/busses/Makefile |2 drivers/i2c/busses/i2c-mpc-drv.c | 421 ++ drivers/i2c/busses/i2c-mpc.c | 421 -- 3 files changed, 423 insertions(+), 421 deletions(-) create mode 100644 drivers/i2c/busses/i2c-mpc-drv.c delete mode 100644 drivers/i2c/busses/i2c-mpc.c diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index ea7068f..171800d 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -2,6 +2,8 @@ # Makefile for the i2c bus drivers. # +i2c-mpc-objs := i2c-mpc-drv.o + obj-$(CONFIG_I2C_ALI1535) += i2c-ali1535.o obj-$(CONFIG_I2C_ALI1563) += i2c-ali1563.o obj-$(CONFIG_I2C_ALI15X3) += i2c-ali15x3.o diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc-drv.c similarity index 100% rename from drivers/i2c/busses/i2c-mpc.c rename to drivers/i2c/busses/i2c-mpc-drv.c ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [GIT PULL] i2c updates for 2.6.25
On 1/28/08, Jean Delvare [EMAIL PROTECTED] wrote: Hi Jon, On Sun, 27 Jan 2008 12:36:03 -0500, Jon Smirl wrote: On 1/27/08, Jean Delvare [EMAIL PROTECTED] wrote: Linus, Please pull the i2c subsystem updates for Linux 2.6.25 from: git://jdelvare.pck.nerim.net/jdelvare-2.6 i2c-for-linus The support for modalias loading of i2c modules isn't in here, what's the status? The status is that I want to give people some time to comment on my modalias patches before I merge them. I also didn't have the time to look into the latest patches you sent yet, and I'd rather merge everything at once. So the merge window for this is rather 2.6.26. Delaying these until 2.6.26 which won't be final until 2009 forces us to ship boxes using a private kernel. These patches have been around since November. Does it really take over a year to get these changes in? -- Jean Delvare -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [GIT PULL] i2c updates for 2.6.25
On 1/27/08, Jean Delvare [EMAIL PROTECTED] wrote: Linus, Please pull the i2c subsystem updates for Linux 2.6.25 from: git://jdelvare.pck.nerim.net/jdelvare-2.6 i2c-for-linus The support for modalias loading of i2c modules isn't in here, what's the status? -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
Ben, do you approve of this? How should error be checked for, is NO_IRQ right? The current code in the kernel looks to be broken because of these checks, the ppc build is wrong and powerpc polled mode doesn't work. On 1/21/08, Jon Smirl [EMAIL PROTECTED] wrote: Alter the mpc i2c driver to use the NO_IRQ symbol instead of the constant zero when checking for valid interrupts. NO_IRQ=-1 on ppc and NO_IRQ=0 on powerpc so the checks against zero are not correct. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- drivers/i2c/busses/i2c-mpc.c | 10 +- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index bbe787b..d20959d 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing) u32 x; int result = 0; - if (i2c-irq == 0) + if (i2c-irq == NO_IRQ) { while (!(readb(i2c-base + MPC_I2C_SR) CSR_MIF)) { schedule(); @@ -329,7 +329,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) return -ENOMEM; i2c-irq = platform_get_irq(pdev, 0); - if (i2c-irq 0) { + if (i2c-irq NO_IRQ) { result = -ENXIO; goto fail_get_irq; } @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) goto fail_map; } - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) if ((result = request_irq(i2c-irq, mpc_i2c_isr, IRQF_SHARED, i2c-mpc, i2c)) 0) { printk(KERN_ERR @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) return result; fail_add: - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); fail_irq: iounmap(i2c-base); @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev) i2c_del_adapter(i2c-adap); platform_set_drvdata(pdev, NULL); - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); iounmap(i2c-base); ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
On 1/24/08, Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: On Thu, 2008-01-24 at 17:32 -0500, Jon Smirl wrote: Ben, do you approve of this? How should error be checked for, is NO_IRQ right? The current code in the kernel looks to be broken because of these checks, the ppc build is wrong and powerpc polled mode doesn't work. == 0 should work on powerpc since NO_IRQ is defined to be 0 there no ? The driver being patched is used in both the powerpc and ppc builds. Anyway, using the symbolic constant is always nicer I suppose. Ben. On 1/21/08, Jon Smirl [EMAIL PROTECTED] wrote: Alter the mpc i2c driver to use the NO_IRQ symbol instead of the constant zero when checking for valid interrupts. NO_IRQ=-1 on ppc and NO_IRQ=0 on powerpc so the checks against zero are not correct. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- drivers/i2c/busses/i2c-mpc.c | 10 +- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index bbe787b..d20959d 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing) u32 x; int result = 0; - if (i2c-irq == 0) + if (i2c-irq == NO_IRQ) { while (!(readb(i2c-base + MPC_I2C_SR) CSR_MIF)) { schedule(); @@ -329,7 +329,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) return -ENOMEM; i2c-irq = platform_get_irq(pdev, 0); - if (i2c-irq 0) { + if (i2c-irq NO_IRQ) { result = -ENXIO; goto fail_get_irq; } @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) goto fail_map; } - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) if ((result = request_irq(i2c-irq, mpc_i2c_isr, IRQF_SHARED, i2c-mpc, i2c)) 0) { printk(KERN_ERR @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev) return result; fail_add: - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); fail_irq: iounmap(i2c-base); @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev) i2c_del_adapter(i2c-adap); platform_set_drvdata(pdev, NULL); - if (i2c-irq != 0) + if (i2c-irq != NO_IRQ) free_irq(i2c-irq, i2c); iounmap(i2c-base); ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 3/3] Add device tree compatible aliases to i2c drivers
On 1/23/08, Matt Sealey [EMAIL PROTECTED] wrote: Jon Smirl wrote: --- a/drivers/i2c/chips/tps65010.c +++ b/drivers/i2c/chips/tps65010.c @@ -571,6 +571,10 @@ static const struct i2c_device_id tps65010_id[] = { { tps65011, TPS65011 }, { tps65012, TPS65012 }, { tps65013, TPS65013 }, + OF_ID(ti,tps65010, TPS65010) + OF_ID(ti,tps65011, TPS65011) + OF_ID(ti,tps65012, TPS65012) + OF_ID(ri,tps65013, TPS65013) { }, ti, ti, ti, ri? --- a/drivers/rtc/rtc-ds1307.c +++ b/drivers/rtc/rtc-ds1307.c @@ -129,6 +129,12 @@ static const struct i2c_device_id ds1307_id[] = { { ds1339, ds_1339 }, { ds1340, ds_1340 }, { m41t00, m41t00 }, + OF_ID(dallas,ds1307, ds_1307) + OF_ID(dallas,ds1337, ds_1337) + OF_ID(dallas,ds1338, ds_1338) + OF_ID(dallas,ds1339, ds_1339) + OF_ID(dallas,ds1340, ds_1340) + OF_ID(stm,m41t00, m41t00) {}, }; The convention is to use the stock ticker, capitalized, if a company has one, I guess dallas is MXIM these days, but dallas is a good substitute based on the fact that most people still remember Dallas clock chips and so on from the Ancient Days. STMicroelectronics is STM. I couldn't care less about case sensitivity, but the stock ticker thing was always a good idea.. it gives a sort of grounding in reality for the manufacturer names. I'll put in whatever the Open Firmware police tell me to. We just all need to come to an agreement. Are we still following this convention or are the names of devices simply arbitrarily defined by Linux kernel developers now? -- Matt Sealey [EMAIL PROTECTED] Genesi, Manager, Developer Relations -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] [PATCH 2/3] Convert PowerPC MPC i2c to of_platform_driver from platform_driver
Convert MPC i2c driver from a platform_driver to a of_platform_driver. Add the ability to dynamically load i2c drivers based on device tree names. Routine names were changed from fsl_ to mpc_ to make them match the file name. Common code moved to powerpc-common.* Orginal ppc driver left intact for deletion when ppc arch is removed. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- arch/powerpc/sysdev/fsl_soc.c | 125 --- drivers/i2c/busses/Makefile |2 drivers/i2c/busses/i2c-mpc-drv.c| 165 --- drivers/i2c/busses/powerpc-common.c | 81 + drivers/i2c/busses/powerpc-common.h | 23 + include/linux/mod_devicetable.h |9 ++ 6 files changed, 263 insertions(+), 142 deletions(-) create mode 100644 drivers/i2c/busses/powerpc-common.c create mode 100644 drivers/i2c/busses/powerpc-common.h diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c index e4b14a5..d6ef264 100644 --- a/arch/powerpc/sysdev/fsl_soc.c +++ b/arch/powerpc/sysdev/fsl_soc.c @@ -318,131 +318,6 @@ err: arch_initcall(gfar_of_init); -#ifdef CONFIG_I2C_BOARDINFO -#include linux/i2c.h -struct i2c_driver_device { - char*of_device; - char*i2c_type; -}; - -static struct i2c_driver_device i2c_devices[] __initdata = { - {ricoh,rs5c372a, rs5c372a,}, - {ricoh,rs5c372b, rs5c372b,}, - {ricoh,rv5c386, rv5c386,}, - {ricoh,rv5c387a, rv5c387a,}, - {dallas,ds1307, ds1307,}, - {dallas,ds1337, ds1337,}, - {dallas,ds1338, ds1338,}, - {dallas,ds1339, ds1339,}, - {dallas,ds1340, ds1340,}, - {stm,m41t00, m41t00}, - {dallas,ds1374, rtc-ds1374,}, -}; - -static int __init of_find_i2c_driver(struct device_node *node, -struct i2c_board_info *info) -{ - int i; - - for (i = 0; i ARRAY_SIZE(i2c_devices); i++) { - if (!of_device_is_compatible(node, i2c_devices[i].of_device)) - continue; - if (strlcpy(info-type, i2c_devices[i].i2c_type, - I2C_NAME_SIZE) = I2C_NAME_SIZE) - return -ENOMEM; - return 0; - } - return -ENODEV; -} - -static void __init of_register_i2c_devices(struct device_node *adap_node, - int bus_num) -{ - struct device_node *node = NULL; - - while ((node = of_get_next_child(adap_node, node))) { - struct i2c_board_info info = {}; - const u32 *addr; - int len; - - addr = of_get_property(node, reg, len); - if (!addr || len sizeof(int) || *addr (1 10) - 1) { - printk(KERN_WARNING fsl_soc.c: invalid i2c device entry\n); - continue; - } - - info.irq = irq_of_parse_and_map(node, 0); - if (info.irq == NO_IRQ) - info.irq = -1; - - if (of_find_i2c_driver(node, info) 0) - continue; - - info.addr = *addr; - - i2c_register_board_info(bus_num, info, 1); - } -} - -static int __init fsl_i2c_of_init(void) -{ - struct device_node *np; - unsigned int i; - struct platform_device *i2c_dev; - int ret; - - for (np = NULL, i = 0; -(np = of_find_compatible_node(np, i2c, fsl-i2c)) != NULL; -i++) { - struct resource r[2]; - struct fsl_i2c_platform_data i2c_data; - const unsigned char *flags = NULL; - - memset(r, 0, sizeof(r)); - memset(i2c_data, 0, sizeof(i2c_data)); - - ret = of_address_to_resource(np, 0, r[0]); - if (ret) - goto err; - - of_irq_to_resource(np, 0, r[1]); - - i2c_dev = platform_device_register_simple(fsl-i2c, i, r, 2); - if (IS_ERR(i2c_dev)) { - ret = PTR_ERR(i2c_dev); - goto err; - } - - i2c_data.device_flags = 0; - flags = of_get_property(np, dfsrr, NULL); - if (flags) - i2c_data.device_flags |= FSL_I2C_DEV_SEPARATE_DFSRR; - - flags = of_get_property(np, fsl5200-clocking, NULL); - if (flags) - i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200; - - ret = - platform_device_add_data(i2c_dev, i2c_data, -sizeof(struct - fsl_i2c_platform_data)); - if (ret) - goto unreg; - - of_register_i2c_devices(np, i); - } - - return 0; - -unreg: - platform_device_unregister(i2c_dev); -err: - return ret
[i2c] i2c_adapter.class and new style drivers
Is i2c_adapter.class relevant anymore with new style drivers? On 1/2/08, Jon Smirl [EMAIL PROTECTED] wrote: On 1/2/08, Jochen Friedrich [EMAIL PROTECTED] wrote: IMHO, there should be a node attribute to override i2c_adapter.class. Legacy i2c drivers (in particular v4l and dvb drivers) use this class to decide which adapter to bind to. dbox2 needs I2C_CLASS_TV_DIGITAL (4). I hadn't paid any attention to i2c_adapter.class. Now that you mention it I wonder why i2c has it. It appears to be a holdover from the older mechanism of searching for devices and it is used as a filter to reduce the number of device being searched. I would think a new style driver could just ignore it. It is probably best to fix it in a successive patch. -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] i2c_new_device is losing error codes
i2c_new_device() is not propagating error codes up. Callers will need to be fixed to use PTR_ERR() to recover the errors. 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) --should return -ENOMEM; return NULL; client-adapter = adap; client-dev.platform_data = info-platform_data; device_init_wakeup(client-dev, info-flags I2C_CLIENT_WAKE); client-flags = info-flags ~I2C_CLIENT_WAKE; client-addr = info-addr; client-irq = info-irq; strlcpy(client-name, info-type, sizeof(client-name)); /* 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). and the device * refcount model is the standard driver model one. */ status = i2c_attach_client(client); --error status is not propagated up if (status 0) { kfree(client); client = NULL; } return client; } EXPORT_SYMBOL_GPL(i2c_new_device); -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 0/5] Version 17, series to add device tree naming to i2c
On 1/11/08, Jean Delvare [EMAIL PROTECTED] wrote: Hi Jon, On Wed, 19 Dec 2007 23:41:36 -0500, Jon Smirl wrote: Since copying i2c-mpc.c to maintain support for the ppc architecture seems to be an issue; instead rework i2c-mpc.c to use CONFIG_PPC_MERGE #ifdefs to support both the ppc and powerpc architecture. When ppc is deleted in six months these #ifdefs will need to be removed. Another rework of the i2c for powerpc device tree patch. This version implements standard alias naming only on the powerpc platform and only for the device tree names. The old naming mechanism of i2c_client.name,driver_name is left in place and not changed for non-powerpc platforms. This patch is fully capable of dynamically loading the i2c modules. You can modprobe in the i2c-mpc driver and the i2c modules described in the device tree will be automatically loaded. Modules also work if compiled in. The follow on patch to module-init-tools is also needed since the i2c subsystem has never implemented dynamic loading. The following series implements standard linux module aliasing for i2c modules on arch=powerpc. It then converts the mpc i2c driver from being a platform driver to an open firmware one. I2C device names are picked up from the device tree. Module aliasing is used to translate from device tree names into to linux kernel names. Several i2c drivers are updated to use the new aliasing. Now that I have read all the previous versions of this patch series and, more importantly, all objections that were raised on the way, I can start reviewing the latest iteration of your patches. I'll also do some testing, although I have no powerpc stuff here, but at least I want to make sure that there are no regressions introduced by your patches on x86. Various people were worried about x86. Around version 15 I altered the patches so that they only impacted PowerPC. If they impact x86 in current form that is a bug. When x86 is ready for it I do think dynamic module loading should be implemented there also. -- Jean Delvare -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
Re: [i2c] [PATCH 0/5] Version 17, series to add device tree naming to i2c
On 1/11/08, Jean Delvare [EMAIL PROTECTED] wrote: Secondly, it promotes OF device names as acceptable aliases. This I don't think I agree with. While I see some value in moving the OF name - Linux name translation to the drivers themselves (even though I don't see this as a mandatory move either), this doesn't imply that OF names should be used as aliases. I don't like the idea that different architectures will name the same device differently in a visible way. This could easily break user-space code that makes assumptions on the device names (libsensors comes to mind.) So, I think that this part will need some more discussion. They're aliases. On the x86 my e1000 Ethernet driver loads using this alias name: pci:v8086d1010sv*sd*bc*sc*i* In fact, the e1000 driver has 63 alias names in addition to e1000 But it's still the e1000 driver after it is loaded. [EMAIL PROTECTED]:/home/linux/drivers/net/e1000$ lsmod | grep e1000 e1000 115968 0 Loading a I2C driver with an OF alias name is not going to change the module name after it is loaded. In fact, once the module is in memory there's no way to tell what name was used to load it. OF device names are set by the Open Firmware committee. It is not reasonable to force the Linux names back into Open Firmware since this would force the other operating systems using Open Firmware to adopt the Linux names. This issue hasn't been visible before since there was a global table in the PowerPC code mapping all known Open Firmware names into linux names. Keeping this as a global table doesn't scale. The mapping needs to be done by each device individually. -- Jean Delvare -- Jon Smirl [EMAIL PROTECTED] ___ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c
[i2c] [PATCH 19 4/5] Convert PowerPC MPC i2c to of_platform_driver from platform_driver
Convert MPC i2c driver from being a platform_driver to an open firmware version. Error returns were improved. Routine names were changed from fsl_ to mpc_ to make them match the file name. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- arch/powerpc/sysdev/fsl_soc.c | 96 -- drivers/i2c/busses/i2c-mpc.c | 183 - 2 files changed, 180 insertions(+), 99 deletions(-) diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c index 94e5c73..d6ef264 100644 --- a/arch/powerpc/sysdev/fsl_soc.c +++ b/arch/powerpc/sysdev/fsl_soc.c @@ -318,102 +318,6 @@ err: arch_initcall(gfar_of_init); -#ifdef CONFIG_I2C_BOARDINFO -#include linux/i2c.h - -static void __init of_register_i2c_devices(struct device_node *adap_node, - int bus_num) -{ - struct device_node *node = NULL; - const char *compatible; - - while ((node = of_get_next_child(adap_node, node))) { - struct i2c_board_info info = {}; - const u32 *addr; - int len; - - addr = of_get_property(node, reg, len); - if (!addr || len sizeof(int) || *addr (1 10) - 1) { - printk(KERN_WARNING fsl_soc.c: invalid i2c device entry\n); - continue; - } - - info.irq = irq_of_parse_and_map(node, 0); - if (info.irq == NO_IRQ) - info.irq = -1; - - compatible = of_get_property(node, compatible, len); - if (!compatible) { - printk(KERN_WARNING i2c-mpc.c: invalid entry, missing compatible attribute\n); - continue; - } - strncpy(info.type, compatible, sizeof(info.type)); - - info.addr = *addr; - - i2c_register_board_info(bus_num, info, 1); - } -} - -static int __init fsl_i2c_of_init(void) -{ - struct device_node *np; - unsigned int i; - struct platform_device *i2c_dev; - int ret; - - for (np = NULL, i = 0; -(np = of_find_compatible_node(np, i2c, fsl-i2c)) != NULL; -i++) { - struct resource r[2]; - struct fsl_i2c_platform_data i2c_data; - const unsigned char *flags = NULL; - - memset(r, 0, sizeof(r)); - memset(i2c_data, 0, sizeof(i2c_data)); - - ret = of_address_to_resource(np, 0, r[0]); - if (ret) - goto err; - - of_irq_to_resource(np, 0, r[1]); - - i2c_dev = platform_device_register_simple(fsl-i2c, i, r, 2); - if (IS_ERR(i2c_dev)) { - ret = PTR_ERR(i2c_dev); - goto err; - } - - i2c_data.device_flags = 0; - flags = of_get_property(np, dfsrr, NULL); - if (flags) - i2c_data.device_flags |= FSL_I2C_DEV_SEPARATE_DFSRR; - - flags = of_get_property(np, fsl5200-clocking, NULL); - if (flags) - i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200; - - ret = - platform_device_add_data(i2c_dev, i2c_data, -sizeof(struct - fsl_i2c_platform_data)); - if (ret) - goto unreg; - - of_register_i2c_devices(np, i); - } - - return 0; - -unreg: - platform_device_unregister(i2c_dev); -err: - return ret; -} - -arch_initcall(fsl_i2c_of_init); -#endif - #ifdef CONFIG_PPC_83xx static int __init mpc83xx_wdt_init(void) { diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index 7c35a8f..91fa73c 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -17,7 +17,7 @@ #include linux/module.h #include linux/sched.h #include linux/init.h -#include linux/platform_device.h +#include linux/of_platform.h #include asm/io.h #include linux/fsl_devices.h @@ -25,13 +25,13 @@ #include linux/interrupt.h #include linux/delay.h -#define MPC_I2C_ADDR 0x00 +#define DRV_NAME mpc-i2c + #define MPC_I2C_FDR0x04 #define MPC_I2C_CR 0x08 #define MPC_I2C_SR 0x0c #define MPC_I2C_DR 0x10 #define MPC_I2C_DFSRR 0x14 -#define MPC_I2C_REGION 0x20 #define CCR_MEN 0x80 #define CCR_MIEN 0x40 @@ -316,6 +316,181 @@ static struct i2c_adapter mpc_ops = { .retries = 1 }; +struct i2c_driver_device { + char*of_device; + char*i2c_driver; + char*i2c_type; +}; + +#ifdef CONFIG_PPC_MERGE + +static void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node) +{ + struct device_node *node = NULL; + + while ((node = of_get_next_child(adap_node, node))) { + struct
Re: [i2c] [PATCH 19 1/5] Implement module aliasing for i2c to translate from device tree names
Comment was wrong, I2C_OF_MODULE_PREFIX was needed. Add it back. Implement module aliasing for i2c to translate from device tree names This patch allows new style i2c chip drivers to have alias names using the official kernel aliasing system and MODULE_DEVICE_TABLE(). I've tested it on PowerPC and x86. This change is required for PowerPC device tree support. Signed-off-by: Jon Smirl [EMAIL PROTECTED] --- drivers/hwmon/f75375s.c |4 ++-- drivers/i2c/chips/ds1682.c |2 +- drivers/i2c/chips/menelaus.c|2 +- drivers/i2c/chips/tps65010.c|2 +- drivers/i2c/chips/tsl2550.c |2 +- drivers/i2c/i2c-core.c | 24 +++- drivers/rtc/rtc-ds1307.c|2 +- drivers/rtc/rtc-ds1374.c|2 +- drivers/rtc/rtc-m41t80.c|2 +- drivers/rtc/rtc-rs5c372.c |2 +- include/linux/i2c.h |5 ++--- include/linux/mod_devicetable.h | 20 scripts/mod/file2alias.c| 12 13 files changed, 67 insertions(+), 14 deletions(-) diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c index 6892f76..2247de6 100644 --- a/drivers/hwmon/f75375s.c +++ b/drivers/hwmon/f75375s.c @@ -117,7 +117,7 @@ struct f75375_data { static int f75375_attach_adapter(struct i2c_adapter *adapter); static int f75375_detect(struct i2c_adapter *adapter, int address, int kind); static int f75375_detach_client(struct i2c_client *client); -static int f75375_probe(struct i2c_client *client); +static int f75375_probe(struct i2c_client *client, const struct i2c_device_id *id); static int f75375_remove(struct i2c_client *client); static struct i2c_driver f75375_legacy_driver = { @@ -628,7 +628,7 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data, } -static int f75375_probe(struct i2c_client *client) +static int f75375_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct f75375_data *data = i2c_get_clientdata(client); struct f75375s_platform_data *f75375s_pdata = client-dev.platform_data; diff --git a/drivers/i2c/chips/ds1682.c b/drivers/i2c/chips/ds1682.c index 9e94542..93c0441 100644 --- a/drivers/i2c/chips/ds1682.c +++ b/drivers/i2c/chips/ds1682.c @@ -200,7 +200,7 @@ static struct bin_attribute ds1682_eeprom_attr = { /* * Called when a ds1682 device is matched with this driver */ -static int ds1682_probe(struct i2c_client *client) +static int ds1682_probe(struct i2c_client *client, const struct i2c_device_id *id) { int rc; diff --git a/drivers/i2c/chips/menelaus.c b/drivers/i2c/chips/menelaus.c index 2dea012..89ef9b6 100644 --- a/drivers/i2c/chips/menelaus.c +++ b/drivers/i2c/chips/menelaus.c @@ -1149,7 +1149,7 @@ static inline void menelaus_rtc_init(struct menelaus_chip *m) static struct i2c_driver menelaus_i2c_driver; -static int menelaus_probe(struct i2c_client *client) +static int menelaus_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct menelaus_chip*menelaus; int rev = 0, val; diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c index e320994..6b13642 100644 --- a/drivers/i2c/chips/tps65010.c +++ b/drivers/i2c/chips/tps65010.c @@ -465,7 +465,7 @@ static int __exit tps65010_remove(struct i2c_client *client) return 0; } -static int tps65010_probe(struct i2c_client *client) +static int tps65010_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct tps65010 *tps; int status; diff --git a/drivers/i2c/chips/tsl2550.c b/drivers/i2c/chips/tsl2550.c index 3de4b19..27c553d 100644 --- a/drivers/i2c/chips/tsl2550.c +++ b/drivers/i2c/chips/tsl2550.c @@ -364,7 +364,7 @@ static int tsl2550_init_client(struct i2c_client *client) */ static struct i2c_driver tsl2550_driver; -static int __devinit tsl2550_probe(struct i2c_client *client) +static int __devinit tsl2550_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct i2c_adapter *adapter = to_i2c_adapter(client-dev.parent); struct tsl2550_data *data; diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index b5e13e4..5f62230 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -47,6 +47,19 @@ static DEFINE_IDR(i2c_adapter_idr); /* - */ +static const struct i2c_device_id *i2c_match_id( + const struct i2c_device_id *id, struct i2c_client *client) +{ + /* only powerpc drivers implement the id_table, +* it is empty on other platforms */ + while (id-name[0]) { + if (strcmp(client-name, id-name) == 0) + return id; + id++; + } + return NULL; +} + static int i2c_device_match(struct device *dev, struct device_driver *drv) { struct i2c_client *client = to_i2c_client(dev