Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices
On Wed, 2014-01-15 at 17:08 +0200, Mika Westerberg wrote: On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote: diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 3a94b79..2f4aea2 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, char *buf) { struct platform_device *pdev = to_platform_device(dev); - int len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); Here you have PAGE_SIZE -1... + if (len != -ENODEV) + return len; + + len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name); return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len; } @@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) if (rc != -ENODEV) return rc; + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; + add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX, pdev-name); return 0; diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index d74c0b3..c4c5588 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv) static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env) { struct i2c_client *client = to_i2c_client(dev); + int rc; + + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; if (add_uevent_var(env, MODALIAS=%s%s, I2C_MODULE_PREFIX, client-name)) @@ -409,6 +414,12 @@ static ssize_t show_modalias(struct device *dev, struct device_attribute *attr, char *buf) { struct i2c_client *client = to_i2c_client(dev); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); and here + if (len != -ENODEV) + return len; + return sprintf(buf, %s%s\n, I2C_MODULE_PREFIX, client-name); } diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 349ebba..ab70eda 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -58,6 +58,11 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, char *buf) { const struct spi_device *spi = to_spi_device(dev); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE); but here it is PAGE_SIZE. good catch. Perhaps it should be PAGE_SIZE in all sites? dev_attr_show() will give a warning message if modalias_show() returns PAGE_SIZE, thus I'd prefer to use PAGE_SIZE - 1 for all sites. thanks, rui -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices
On Thu, 2014-01-16 at 12:28 +, Mark Brown wrote: On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote: ACPI enumerated devices has ACPI style _HID and _CID strings, all of these strings can be used for both driver loading and matching. Currently, in Platform, I2C and SPI bus, the ACPI style driver matching is supported by invoking acpi_driver_match_device() in bus .match() callback. But, the module autoloading is still broken. Acked-by: Mark Brown broo...@linaro.org modulo the PAGE_SIZE stuff Mika noted - unless this changes radically please just assume I'm OK with it. thanks. -rui -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices
On Thu, 2014-01-16 at 13:27 +0100, Wolfram Sang wrote: diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index d74c0b3..c4c5588 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv) static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env) { struct i2c_client *client = to_i2c_client(dev); + int rc; + + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; if (add_uevent_var(env, MODALIAS=%s%s, I2C_MODULE_PREFIX, client-name)) I wonder why we don't have/need that with CONFIG_OF? Because probably nobody is using modules with i2c devices there? This seems a gap to me but I'm not 100% sure. I saw Grant Likely introduced the OF style MODALIAS to platform bus, and OF style registration/binding to i2c bus, maybe he has an answer for this. thanks, rui -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices
On Fri, 2014-01-17 at 02:28 +0100, Rafael J. Wysocki wrote: On Thursday, January 16, 2014 04:04:35 PM Zhang Rui wrote: On Wed, 2014-01-15 at 17:08 +0200, Mika Westerberg wrote: On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote: diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 3a94b79..2f4aea2 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, char *buf) { struct platform_device *pdev = to_platform_device(dev); - int len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); Here you have PAGE_SIZE -1... + if (len != -ENODEV) + return len; + + len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name); return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len; } @@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) if (rc != -ENODEV) return rc; + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; + add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX, pdev-name); return 0; diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index d74c0b3..c4c5588 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv) static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env) { struct i2c_client *client = to_i2c_client(dev); + int rc; + + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; if (add_uevent_var(env, MODALIAS=%s%s, I2C_MODULE_PREFIX, client-name)) @@ -409,6 +414,12 @@ static ssize_t show_modalias(struct device *dev, struct device_attribute *attr, char *buf) { struct i2c_client *client = to_i2c_client(dev); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); and here + if (len != -ENODEV) + return len; + return sprintf(buf, %s%s\n, I2C_MODULE_PREFIX, client-name); } diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 349ebba..ab70eda 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -58,6 +58,11 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, char *buf) { const struct spi_device *spi = to_spi_device(dev); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE); but here it is PAGE_SIZE. good catch. Perhaps it should be PAGE_SIZE in all sites? dev_attr_show() will give a warning message if modalias_show() returns PAGE_SIZE, thus I'd prefer to use PAGE_SIZE - 1 for all sites. So I changed the PAGE_SIZE to PAGE_SIZE - 1 in the last instance and queued up the whole series for 3.14 in linux-pm.git/linux-next. Please have a look at that and let me know if there's anything wrong with it. the change looks okay to me. thanks, rui Thanks! -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.
-Original Message- From: Rob Herring [mailto:robherri...@gmail.com] Sent: Wednesday, January 15, 2014 9:45 PM To: Zhang, Rui Cc: linux-ker...@vger.kernel.org; linux-a...@vger.kernel.org; linux- i...@vger.kernel.org; linux-...@vger.kernel.org; w...@the-dreams.de; Mark Brown; Greg Kroah-Hartman; Wysocki, Rafael J; Grant Likely; Rob Herring; jarkko.nik...@linux.intel.com; mika.westerb...@linux.intel.com; devicet...@vger.kernel.org Subject: Re: [PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus. Importance: High On Tue, Jan 14, 2014 at 2:46 AM, Zhang Rui rui.zh...@intel.com wrote: Fix a problem that, the platform bus supports the OF style modalias in .uevent() call, but not in its device 'modalias' sysfs attribute. cc: devicet...@vger.kernel.org Signed-off-by: Zhang Rui rui.zh...@intel.com Acked-by: Rob Herring r...@kernel.org As there doesn't appear any dependency with the rest of this series, I can take this. Thanks. -rui One minor nit below. --- drivers/base/platform.c |4 drivers/of/device.c |3 +++ include/linux/of_device.h |6 ++ 3 files changed, 13 insertions(+) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 2f4aea2..bc78848 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -679,6 +679,10 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, struct platform_device *pdev = to_platform_device(dev); int len; + len = of_device_get_modalias(dev, buf, PAGE_SIZE -1); + if (len != -ENODEV) + return len; + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); if (len != -ENODEV) return len; diff --git a/drivers/of/device.c b/drivers/of/device.c index f685e55..dafb973 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -85,6 +85,9 @@ ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len) int cplen, i; ssize_t tsize, csize, repend; + if ((!dev) || (!dev-of_node))\ Don't need the parentheses here. + return -ENODEV; + /* Name Type */ csize = snprintf(str, len, of:N%sT%s, dev-of_node-name, dev-of_node-type); diff --git a/include/linux/of_device.h b/include/linux/of_device.h index 82ce324..8d7dd67 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -64,6 +64,12 @@ static inline int of_driver_match_device(struct device *dev, static inline void of_device_uevent(struct device *dev, struct kobj_uevent_env *env) { } +static inline int of_device_get_modalias(struct device *dev, + char *str, ssize_t len) { + return -ENODEV; +} + static inline int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env) { -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices
On Mon, 2014-01-13 at 17:35 +, Mark Brown wrote: On Mon, Jan 13, 2014 at 09:48:31PM +0800, Zhang Rui wrote: ACPI enumerated devices has ACPI style _HID and _CID strings, all of these strings can be used for both driver loading and matching. But currently, in Platform, I2C and SPI bus, only the ACPI style driver matching is supported by invoking acpi_driver_match_device() in bus .match() callback. I don't understand what this means, sorry. sorry, I should be clearer about this. As far as I can tell ACPI style _HID and _CID strings are something different to ACPI style driver matching but what that actually means is not at all clear to me so I don't know what problem this is intended to fix. I gave a more detailed description about the problem in the changelog of patch 2/4. Take the following piece of code for example, static const struct acpi_device_id xxx_acpi_match[] = { { INTABCD, 0 }, { } }; MODULE_DEVICE_TABLE(acpi, xxx_acpi_match); this can be seen in a platform/I2C/SPI driver that supports an ACPI enumerated device, right? If this piece of code is used in a platform driver for an ACPI enumerated platform device, the platform DRIVER module_alias is acpi:INTABCD, but the uevent attribute of its platform device node is platform:INTABCD:00 (PREFIX:platform_device-name). If this piece of code is used in an I2C driver for an ACPI enumerated i2c device, the i2c driver module_alias is acpi:INTABCD, but the uevent of its i2c device node is i2c:INTABCD:00 (PREFIX:i2c_client-name). If this piece of code is used in an *SPI* driver for an ACPI enumerated spi device, the spi driver module_alias is acpi:INTABCD, but the uevent of its spi device node is spi:INTABCD (PREFIX:spi_device-modalias). thus when the device node is created, the driver will not be loaded automatically because their modalias do not match. Please also always remember to CC maintainers on patches. okay, will resend the patches later. thanks, rui diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 349ebba..ab70eda 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -58,6 +58,11 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, char *buf) { const struct spi_device *spi = to_spi_device(dev); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE); + if (len != -ENODEV) + return len; return sprintf(buf, %s%s\n, SPI_MODULE_PREFIX, spi-modalias); } What does this do and why can't acpi_driver_match_device() handle this like it does for other ACPI devices? We don't need to add such code for other firmware interfaces... -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] module autoloading fixes
Hi, all, This patch set fixes a couple of module autoloading problem. Patch 1/4 fixes a bug in ACPI device 'modalias' and 'uevent' attributes, although the bug can rarely be reproduced (only if there is an output error of snprintf, or the ids are longer than 1024 bytes) Patch 2/4 introduces two new APIs for exporting ACPI style 'modalias' and 'uevent' attributes in other buses. Patch 3/4 introduce support for ACPI style 'modalias' and 'uevent' attributes in platform, I2C and SPI bus. Patch 4/4 add OF style 'modalias' support for platform bus. I did some tests and can confirm that the code for ACPI enumerated platform bus device works well. I tried with a patch with convert ACPI Fan device/driver to platform bus, and can confirm that the code for ACPI enumerated platform device works well, both the platform Fan driver and device show their modalias as acpi:PNP0C0B. thanks, rui Zhang Rui (4): ACPI: fix create_modalias() return value handling ACPI: add module autoloading support for ACPI enumerated devices fix module autoloading for ACPI enumerated devices OF: introduce OF style 'modalias' support for platform bus. drivers/acpi/scan.c | 73 + drivers/base/platform.c | 16 +- drivers/i2c/i2c-core.c| 11 +++ drivers/of/device.c |3 ++ drivers/spi/spi.c | 10 +++ include/linux/acpi.h | 15 ++ include/linux/of_device.h |6 7 files changed, 127 insertions(+), 7 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] fix module autoloading for ACPI enumerated devices
ACPI enumerated devices has ACPI style _HID and _CID strings, all of these strings can be used for both driver loading and matching. Currently, in Platform, I2C and SPI bus, the ACPI style driver matching is supported by invoking acpi_driver_match_device() in bus .match() callback. But, the module autoloading is still broken. For example, there is any ACPI device with _HID INTABCD that is enumerated to platform bus, and we have a driver that can probe it. The driver exports its module_alias as acpi:INTABCD use the following code static const struct acpi_device_id xxx_acpi_match[] = { { INTABCD, 0 }, { } }; MODULE_DEVICE_TABLE(acpi, xxx_acpi_match); But, unfortunately, the device' modalias is shown as platform:INTABCD:00, please refer to modalias_show() and platform_uevent() in drivers/base/platform.c. This results in that the driver will not be loaded automatically when the device node is created, because their modalias do not match. This also applies to I2C and SPI bus. With this patch, the device' modalias will be shown as acpi:INTABCD as well. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/base/platform.c | 12 +++- drivers/i2c/i2c-core.c | 11 +++ drivers/spi/spi.c | 10 ++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 3a94b79..2f4aea2 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, char *buf) { struct platform_device *pdev = to_platform_device(dev); - int len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); + if (len != -ENODEV) + return len; + + len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name); return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len; } @@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) if (rc != -ENODEV) return rc; + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; + add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX, pdev-name); return 0; diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index d74c0b3..c4c5588 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv) static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env) { struct i2c_client *client = to_i2c_client(dev); + int rc; + + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; if (add_uevent_var(env, MODALIAS=%s%s, I2C_MODULE_PREFIX, client-name)) @@ -409,6 +414,12 @@ static ssize_t show_modalias(struct device *dev, struct device_attribute *attr, char *buf) { struct i2c_client *client = to_i2c_client(dev); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); + if (len != -ENODEV) + return len; + return sprintf(buf, %s%s\n, I2C_MODULE_PREFIX, client-name); } diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 349ebba..ab70eda 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -58,6 +58,11 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, char *buf) { const struct spi_device *spi = to_spi_device(dev); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE); + if (len != -ENODEV) + return len; return sprintf(buf, %s%s\n, SPI_MODULE_PREFIX, spi-modalias); } @@ -114,6 +119,11 @@ static int spi_match_device(struct device *dev, struct device_driver *drv) static int spi_uevent(struct device *dev, struct kobj_uevent_env *env) { const struct spi_device *spi = to_spi_device(dev); + int rc; + + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; add_uevent_var(env, MODALIAS=%s%s, SPI_MODULE_PREFIX, spi-modalias); return 0; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] ACPI: fix create_modalias() return value handling
Currently, create_modalias() handles the output truncated case in an improper way (return -EINVAL). Plus, acpi_device_uevent() and acpi_device_modalias_show() do improper check for the create_modalias() return value as well. This patch fixes create_modalias() to return -EINVAL if there is an output error, return -ENOMEM if the output is truncated, and also fixes both acpi_device_uevent() and acpi_device_modalias_show() to do proper return value check. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/acpi/scan.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index fd39459..c925321 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -85,6 +85,9 @@ int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler, * Creates hid/cid(s) string needed for modalias and uevent * e.g. on a device with hid:IBM0001 and cid:ACPI0001 you get: * char *modalias: acpi:IBM0001:ACPI0001 + * Return: 0: no _HID and no _CID + * -EINVAL: output error + * -ENOMEM: output is truncated */ static int create_modalias(struct acpi_device *acpi_dev, char *modalias, int size) @@ -101,8 +104,10 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias, list_for_each_entry(id, acpi_dev-pnp.ids, list) { count = snprintf(modalias[len], size, %s:, id-id); - if (count 0 || count = size) - return -EINVAL; + if (count 0) + return EINVAL; + if (count = size) + return -ENOMEM; len += count; size -= count; } @@ -116,10 +121,9 @@ acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, cha struct acpi_device *acpi_dev = to_acpi_device(dev); int len; - /* Device has no HID and no CID or string is 1024 */ len = create_modalias(acpi_dev, buf, 1024); if (len = 0) - return 0; + return len; buf[len++] = '\n'; return len; } @@ -782,8 +786,8 @@ static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env) return -ENOMEM; len = create_modalias(acpi_dev, env-buf[env-buflen - 1], sizeof(env-buf) - env-buflen); - if (len = (sizeof(env-buf) - env-buflen)) - return -ENOMEM; + if (len = 0) + return len; env-buflen += len; return 0; } -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.
Fix a problem that, the platform bus supports the OF style modalias in .uevent() call, but not in its device 'modalias' sysfs attribute. cc: devicet...@vger.kernel.org Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/base/platform.c |4 drivers/of/device.c |3 +++ include/linux/of_device.h |6 ++ 3 files changed, 13 insertions(+) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 2f4aea2..bc78848 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -679,6 +679,10 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, struct platform_device *pdev = to_platform_device(dev); int len; + len = of_device_get_modalias(dev, buf, PAGE_SIZE -1); + if (len != -ENODEV) + return len; + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); if (len != -ENODEV) return len; diff --git a/drivers/of/device.c b/drivers/of/device.c index f685e55..dafb973 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -85,6 +85,9 @@ ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len) int cplen, i; ssize_t tsize, csize, repend; + if ((!dev) || (!dev-of_node)) + return -ENODEV; + /* Name Type */ csize = snprintf(str, len, of:N%sT%s, dev-of_node-name, dev-of_node-type); diff --git a/include/linux/of_device.h b/include/linux/of_device.h index 82ce324..8d7dd67 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -64,6 +64,12 @@ static inline int of_driver_match_device(struct device *dev, static inline void of_device_uevent(struct device *dev, struct kobj_uevent_env *env) { } +static inline int of_device_get_modalias(struct device *dev, + char *str, ssize_t len) +{ + return -ENODEV; +} + static inline int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env) { -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] ACPI: add module autoloading support for ACPI enumerated devices
An ACPI enumerated device may have its compatible id strings. To support the compatible ACPI ids (acpi_device-pnp.ids), we introduced acpi_driver_match_device() to match the driver-acpi_match_table and acpi_device-pnp.ids. For those drivers, MODULE_DEVICE_TABLE(acpi, xxx) is used to exports the driver module alias in the format of acpi:device_compatible_ids. But in the mean time, the current code does not export the ACPI compatible strings as part of the module_alias for the ACPI enumerated devices, which will break the module autoloading. Take the following piece of code for example, static const struct acpi_device_id xxx_acpi_match[] = { { INTABCD, 0 }, { } }; MODULE_DEVICE_TABLE(acpi, xxx_acpi_match); If this piece of code is used in a platform driver for an ACPI enumerated platform device, the platform driver module_alias is acpi:INTABCD, but the uevent attribute of its platform device node is platform:INTABCD:00 (PREFIX:platform_device-name). If this piece of code is used in an i2c driver for an ACPI enumerated i2c device, the i2c driver module_alias is acpi:INTABCD, but the uevent of its i2c device node is i2c:INTABCD:00 (PREFIX:i2c_client-name). If this piece of code is used in an spi driver for an ACPI enumerated spi device, the spi driver module_alias is acpi:INTABCD, but the uevent of its spi device node is spi:INTABCD (PREFIX:spi_device-modalias). The reason why the module autoloading is not broken for now is that the uevent file of the ACPI device node is acpi:INTABCD. Thus it is the ACPI device node creation that loads the platform/i2c/spi driver. So this is a problem that will affect us the day when the ACPI bus is removed from device model. This patch introduces two new APIs, one for exporting ACPI ids in uevent MODALIAS field, and another for exporting ACPI ids in device' modalias sysfs attribute. For any bus that supports ACPI enumerated devices, it needs to invoke these two functions for their uevent and modalias attribute. Signed-off-by: Zhang Rui rui.zh...@intel.com Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com --- drivers/acpi/scan.c | 57 ++ include/linux/acpi.h | 15 + 2 files changed, 72 insertions(+) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index c925321..680bb56 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -116,6 +116,63 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias, return len; } +/* + * Creates uevent modalias field for ACPI enumerated devices. + * Because the other buses does not support ACPI HIDs CIDs. + * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get: + * acpi:IBM0001:ACPI0001 + */ +int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env) +{ + struct acpi_device *acpi_dev; + int len; + + acpi_dev = ACPI_COMPANION(dev); + if (!acpi_dev) + return -ENODEV; + + /* Fall back to bus specific way of modalias exporting */ + if (list_empty(acpi_dev-pnp.ids)) + return -ENODEV; + + if (add_uevent_var(env, MODALIAS=)) + return -ENOMEM; + len = create_modalias(acpi_dev, env-buf[env-buflen - 1], + sizeof(env-buf) - env-buflen); + if (len = 0) + return len; + env-buflen += len; + return 0; +} +EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias); + +/* + * Creates modalias sysfs attribute for ACPI enumerated devices. + * Because the other buses does not support ACPI HIDs CIDs. + * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get: + * acpi:IBM0001:ACPI0001 + */ +int acpi_device_modalias(struct device *dev, char *buf, int size) +{ + struct acpi_device *acpi_dev; + int len; + + acpi_dev = ACPI_COMPANION(dev); + if (!acpi_dev) + return -ENODEV; + + /* Fall back to bus specific way of modalias exporting */ + if (list_empty(acpi_dev-pnp.ids)) + return -ENODEV; + + len = create_modalias(acpi_dev, buf, size -1); + if (len = 0) + return len; + buf[len++] = '\n'; + return len; +} +EXPORT_SYMBOL_GPL(acpi_device_modalias); + static ssize_t acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, char *buf) { struct acpi_device *acpi_dev = to_acpi_device(dev); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d9099b1..22325ed 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -409,6 +409,9 @@ static inline bool acpi_driver_match_device(struct device *dev, return !!acpi_match_device(drv-acpi_match_table, dev); } +int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *); +int acpi_device_modalias(struct device *, char *, int); + #define ACPI_PTR(_ptr) (_ptr) #else /* !CONFIG_ACPI */ @@ -488,6 +491,18 @@ static inline bool
[PATCH 0/4] module autoloading fixes
Hi, all, This patch set fixes a couple of module autoloading problem. Patch 1/4 fixes a bug in ACPI device 'modalias' and 'uevent' attributes, although the bug can rarely be reproduced (only if there is an output error of snprintf, or the ids are longer than 1024 bytes) Patch 2/4 introduces two new APIs for exporting ACPI style 'modalias' and 'uevent' attributes in other buses. Patch 3/4 introduce support for ACPI style 'modalias' and 'uevent' attributes in platform, I2C and SPI bus. Patch 4/4 add OF style 'modalias' support for platform bus. I did some tests and can confirm that the code for ACPI enumerated platform bus device works well. I tried with a patch with convert ACPI Fan device/driver to platform bus, and can confirm that the code for ACPI enumerated platform device works well, both the platform Fan driver and device show their modalias as acpi:PNP0C0B. thanks, rui Zhang Rui (4): ACPI: fix create_modalias() return value handling ACPI: add module autoloading support for ACPI enumerated devices fix module autoloading for ACPI enumerated devices OF: introduce OF style 'modalias' support for platform bus. drivers/acpi/scan.c | 73 + drivers/base/platform.c | 16 +- drivers/i2c/i2c-core.c| 11 +++ drivers/of/device.c |3 ++ drivers/spi/spi.c | 10 +++ include/linux/acpi.h | 15 ++ include/linux/of_device.h |6 7 files changed, 127 insertions(+), 7 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] ACPI: add module autoloading support for ACPI enumerated devices
An ACPI enumerated device may have its compatible id strings. To support the compatible ACPI ids (acpi_device-pnp.ids), we introduced acpi_driver_match_device() to match the driver-acpi_match_table and acpi_device-pnp.ids. For those drivers, MODULE_DEVICE_TABLE(acpi, xxx) is used to exports the driver module alias in the format of acpi:device_compatible_ids. But in the mean time, the current code does not export the ACPI compatible strings as part of the module_alias for the ACPI enumerated devices, which will break the module autoloading. Take the following piece of code for example, static const struct acpi_device_id xxx_acpi_match[] = { { INTABCD, 0 }, { } }; MODULE_DEVICE_TABLE(acpi, xxx_acpi_match); If this piece of code is used in a platform driver for an ACPI enumerated platform device, the platform driver module_alias is acpi:INTABCD, but the uevent attribute of its platform device node is platform:INTABCD:00 (PREFIX:platform_device-name). If this piece of code is used in an i2c driver for an ACPI enumerated i2c device, the i2c driver module_alias is acpi:INTABCD, but the uevent of its i2c device node is i2c:INTABCD:00 (PREFIX:i2c_client-name). If this piece of code is used in an spi driver for an ACPI enumerated spi device, the spi driver module_alias is acpi:INTABCD, but the uevent of its spi device node is spi:INTABCD (PREFIX:spi_device-modalias). The reason why the module autoloading is not broken for now is that the uevent file of the ACPI device node is acpi:INTABCD. Thus it is the ACPI device node creation that loads the platform/i2c/spi driver. So this is a problem that will affect us the day when the ACPI bus is removed from device model. This patch introduces two new APIs, one for exporting ACPI ids in uevent MODALIAS field, and another for exporting ACPI ids in device' modalias sysfs attribute. For any bus that supports ACPI enumerated devices, it needs to invoke these two functions for their uevent and modalias attribute. Signed-off-by: Zhang Rui rui.zh...@intel.com Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com --- drivers/acpi/scan.c | 57 ++ include/linux/acpi.h | 15 + 2 files changed, 72 insertions(+) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index c925321..680bb56 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -116,6 +116,63 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias, return len; } +/* + * Creates uevent modalias field for ACPI enumerated devices. + * Because the other buses does not support ACPI HIDs CIDs. + * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get: + * acpi:IBM0001:ACPI0001 + */ +int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env) +{ + struct acpi_device *acpi_dev; + int len; + + acpi_dev = ACPI_COMPANION(dev); + if (!acpi_dev) + return -ENODEV; + + /* Fall back to bus specific way of modalias exporting */ + if (list_empty(acpi_dev-pnp.ids)) + return -ENODEV; + + if (add_uevent_var(env, MODALIAS=)) + return -ENOMEM; + len = create_modalias(acpi_dev, env-buf[env-buflen - 1], + sizeof(env-buf) - env-buflen); + if (len = 0) + return len; + env-buflen += len; + return 0; +} +EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias); + +/* + * Creates modalias sysfs attribute for ACPI enumerated devices. + * Because the other buses does not support ACPI HIDs CIDs. + * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get: + * acpi:IBM0001:ACPI0001 + */ +int acpi_device_modalias(struct device *dev, char *buf, int size) +{ + struct acpi_device *acpi_dev; + int len; + + acpi_dev = ACPI_COMPANION(dev); + if (!acpi_dev) + return -ENODEV; + + /* Fall back to bus specific way of modalias exporting */ + if (list_empty(acpi_dev-pnp.ids)) + return -ENODEV; + + len = create_modalias(acpi_dev, buf, size -1); + if (len = 0) + return len; + buf[len++] = '\n'; + return len; +} +EXPORT_SYMBOL_GPL(acpi_device_modalias); + static ssize_t acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, char *buf) { struct acpi_device *acpi_dev = to_acpi_device(dev); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d9099b1..22325ed 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -409,6 +409,9 @@ static inline bool acpi_driver_match_device(struct device *dev, return !!acpi_match_device(drv-acpi_match_table, dev); } +int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *); +int acpi_device_modalias(struct device *, char *, int); + #define ACPI_PTR(_ptr) (_ptr) #else /* !CONFIG_ACPI */ @@ -488,6 +491,18 @@ static inline bool
[PATCH 1/4] ACPI: fix create_modalias() return value handling
Currently, create_modalias() handles the output truncated case in an improper way (return -EINVAL). Plus, acpi_device_uevent() and acpi_device_modalias_show() do improper check for the create_modalias() return value as well. This patch fixes create_modalias() to return -EINVAL if there is an output error, return -ENOMEM if the output is truncated, and also fixes both acpi_device_uevent() and acpi_device_modalias_show() to do proper return value check. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/acpi/scan.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index fd39459..c925321 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -85,6 +85,9 @@ int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler, * Creates hid/cid(s) string needed for modalias and uevent * e.g. on a device with hid:IBM0001 and cid:ACPI0001 you get: * char *modalias: acpi:IBM0001:ACPI0001 + * Return: 0: no _HID and no _CID + * -EINVAL: output error + * -ENOMEM: output is truncated */ static int create_modalias(struct acpi_device *acpi_dev, char *modalias, int size) @@ -101,8 +104,10 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias, list_for_each_entry(id, acpi_dev-pnp.ids, list) { count = snprintf(modalias[len], size, %s:, id-id); - if (count 0 || count = size) - return -EINVAL; + if (count 0) + return EINVAL; + if (count = size) + return -ENOMEM; len += count; size -= count; } @@ -116,10 +121,9 @@ acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, cha struct acpi_device *acpi_dev = to_acpi_device(dev); int len; - /* Device has no HID and no CID or string is 1024 */ len = create_modalias(acpi_dev, buf, 1024); if (len = 0) - return 0; + return len; buf[len++] = '\n'; return len; } @@ -782,8 +786,8 @@ static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env) return -ENOMEM; len = create_modalias(acpi_dev, env-buf[env-buflen - 1], sizeof(env-buf) - env-buflen); - if (len = (sizeof(env-buf) - env-buflen)) - return -ENOMEM; + if (len = 0) + return len; env-buflen += len; return 0; } -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] fix module autoloading for ACPI enumerated devices
ACPI enumerated devices has ACPI style _HID and _CID strings, all of these strings can be used for both driver loading and matching. But currently, in Platform, I2C and SPI bus, only the ACPI style driver matching is supported by invoking acpi_driver_match_device() in bus .match() callback. This patch fixes module autoloading on those buses for ACPI enumerated devices. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/base/platform.c | 12 +++- drivers/i2c/i2c-core.c | 11 +++ drivers/spi/spi.c | 10 ++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 3a94b79..2f4aea2 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, char *buf) { struct platform_device *pdev = to_platform_device(dev); - int len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); + if (len != -ENODEV) + return len; + + len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name); return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len; } @@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) if (rc != -ENODEV) return rc; + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; + add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX, pdev-name); return 0; diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index d74c0b3..c4c5588 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv) static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env) { struct i2c_client *client = to_i2c_client(dev); + int rc; + + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; if (add_uevent_var(env, MODALIAS=%s%s, I2C_MODULE_PREFIX, client-name)) @@ -409,6 +414,12 @@ static ssize_t show_modalias(struct device *dev, struct device_attribute *attr, char *buf) { struct i2c_client *client = to_i2c_client(dev); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); + if (len != -ENODEV) + return len; + return sprintf(buf, %s%s\n, I2C_MODULE_PREFIX, client-name); } diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 349ebba..ab70eda 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -58,6 +58,11 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, char *buf) { const struct spi_device *spi = to_spi_device(dev); + int len; + + len = acpi_device_modalias(dev, buf, PAGE_SIZE); + if (len != -ENODEV) + return len; return sprintf(buf, %s%s\n, SPI_MODULE_PREFIX, spi-modalias); } @@ -114,6 +119,11 @@ static int spi_match_device(struct device *dev, struct device_driver *drv) static int spi_uevent(struct device *dev, struct kobj_uevent_env *env) { const struct spi_device *spi = to_spi_device(dev); + int rc; + + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; add_uevent_var(env, MODALIAS=%s%s, SPI_MODULE_PREFIX, spi-modalias); return 0; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.
Fix a problem that, the platform bus supports the OF style modalias in .uevent() call, but not in its device' modalias sysfs attribute. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/base/platform.c |4 drivers/of/device.c |3 +++ include/linux/of_device.h |6 ++ 3 files changed, 13 insertions(+) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 2f4aea2..bc78848 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -679,6 +679,10 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a, struct platform_device *pdev = to_platform_device(dev); int len; + len = of_device_get_modalias(dev, buf, PAGE_SIZE -1); + if (len != -ENODEV) + return len; + len = acpi_device_modalias(dev, buf, PAGE_SIZE -1); if (len != -ENODEV) return len; diff --git a/drivers/of/device.c b/drivers/of/device.c index f685e55..dafb973 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -85,6 +85,9 @@ ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len) int cplen, i; ssize_t tsize, csize, repend; + if ((!dev) || (!dev-of_node)) + return -ENODEV; + /* Name Type */ csize = snprintf(str, len, of:N%sT%s, dev-of_node-name, dev-of_node-type); diff --git a/include/linux/of_device.h b/include/linux/of_device.h index 82ce324..8d7dd67 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -64,6 +64,12 @@ static inline int of_driver_match_device(struct device *dev, static inline void of_device_uevent(struct device *dev, struct kobj_uevent_env *env) { } +static inline int of_device_get_modalias(struct device *dev, + char *str, ssize_t len) +{ + return -ENODEV; +} + static inline int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env) { -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: Fix modalias for ACPI enumerated I2C devices
On Mon, 2013-10-14 at 20:47 +0800, Zhang Rui wrote: On Mon, 2013-10-14 at 14:18 +0300, Jarkko Nikula wrote: On 10/14/2013 12:23 PM, Zhang Rui wrote: Hi, On Thu, 2013-10-10 at 17:17 +0300, Jarkko Nikula wrote: There is a minor fault about ACPI enumerated I2C devices with their modalias attribute. Now modalias is set by device instance not by hardware ID. For example i2c:INTABCD:00, i2c:INTABCD:01 etc. This means each device instance gets different modalias which does match with generated modules.alias. Currently this is not problem as matching can happen also with acpi:INTABCD modalias. IMO, this is not the proper fix for the modalias problem because ACPI enumerated I2C device may have compatible ids. Instead, we should export all the compatible ids as the modules alias of the ACPI enumerated I2C device. can you please take a look at the patch I sent out earlier? https://patchwork.kernel.org/patch/3034991/ https://patchwork.kernel.org/patch/3035041/ https://patchwork.kernel.org/patch/3035021/ I see. This makes sense as it avoids that same device has two different modaliases from both acpi and other subsystem. How about modalias nodes in sysfs, should they also reflect what is matching uvent? good catch, will fix modalias as well in next version. Hi, I have a question about the device uevent and modalias sysfs attributes. what is the relationship between these two? Am I right to say that, if there is the MODALIAS field in uevent file, this field must be consistent with the content in modalias attribute? I checked the code in drivers/base/platform.c, static ssize_t modalias_show(struct device *dev, struct device_attribute *a, char *buf) { struct platform_device *pdev = to_platform_device(dev); int len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name); return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len; } static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) { struct platform_device *pdev = to_platform_device(dev); int rc; /* Some devices have extra OF data and an OF-style MODALIAS */ rc = of_device_uevent_modalias(dev, env); if (rc != -ENODEV) return rc; add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX, pdev-name); return 0; } This means that the OF-style MODALIAS is not shown in modalias sysfs attribute. is this a bug? thanks, rui -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] Platform: fix module autoloading for ACPI enumerated devices
Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/base/platform.c |4 1 file changed, 4 insertions(+) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 4f8bef3..80d2250 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -690,6 +690,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) if (rc != -ENODEV) return rc; + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; + add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX, pdev-name); return 0; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] ACPI: add module autoloading support for ACPI enumerated devices
An ACPI enumerated device may have its compatible id strings. To support the compatible ACPI ids (acpi_device-pnp.ids), we introduced acpi_driver_match_device() to match the driver-acpi_match_table and acpi_device-pnp.ids. For those drivers, MODULE_DEVICE_TABLE(acpi, xxx) is used to exports the driver module alias in the format of acpi:device_compatible_ids. But in the mean time, the current code does not export the ACPI compatible strings as part of the module_alias for the ACPI enumerated devices, which will break the module autoloading. Take the following piece of code for example, static const struct acpi_device_id xxx_acpi_match[] = { { INTABCD, 0 }, { } }; MODULE_DEVICE_TABLE(acpi, xxx_acpi_match); If this piece of code is used in a platform driver for an ACPI enumerated platform device, the platform driver module_alias is acpi:INTABCD, but the uevent attribute of its platform device node is platform:INTABCD:00 (PREFIX:pdev-name). If this piece of code is used in an i2c driver for an ACPI enumerated i2c device, the i2c driver module_alias is acpi:INTABCD, but the uevent of its i2c device node is i2c:INTABCD:00 (PREFIX:client-name). The reason why the module autoloading is not broken for now is that the uevent file of the ACPI device node is acpi:INTABCD. Thus it is the ACPI device node creation that loads the platform/i2c driver. So this is a problem that will affect us the day when the ACPI bus is removed from device model. This patch introduces a new function to exporting ACPI ids as the module_alias, for the ACPI enumerate devices. For any drivers using MODULE_DEVICE_TALBE(acpi, blabla), the uevent file of its associated device must be fixed by invoking this function. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/acpi/scan.c | 24 include/linux/acpi.h |8 2 files changed, 32 insertions(+) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 407ad13..db6f879 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -111,6 +111,30 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias, return len; } +int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env) +{ + struct acpi_device *acpi_dev; + int result; + int len; + + result = acpi_bus_get_device(ACPI_HANDLE(dev), acpi_dev); + if (result) + return result; + + if (list_empty(acpi_dev-pnp.ids)) + return 0; + + if (add_uevent_var(env, MODALIAS=)) + return -ENOMEM; + len = create_modalias(acpi_dev, env-buf[env-buflen - 1], + sizeof(env-buf) - env-buflen); + if (len = (sizeof(env-buf) - env-buflen)) + return -ENOMEM; + env-buflen += len; + return 0; +} +EXPORT_SYMBOL(acpi_device_uevent_modalias); + static ssize_t acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, char *buf) { struct acpi_device *acpi_dev = to_acpi_device(dev); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index a5db4ae..117fa26 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -401,6 +401,8 @@ static inline bool acpi_driver_match_device(struct device *dev, return !!acpi_match_device(drv-acpi_match_table, dev); } +int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *); + #define ACPI_PTR(_ptr) (_ptr) #else /* !CONFIG_ACPI */ @@ -471,6 +473,12 @@ static inline bool acpi_driver_match_device(struct device *dev, return false; } +static inline int acpi_device_uevent_modalias(struct device *dev, + struct kobj_uevent_env *env) +{ + return -ENODEV; +} + #define ACPI_PTR(_ptr) (NULL) #endif /* !CONFIG_ACPI */ -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] I2C: fix module autoloading for ACPI enumerated devices
Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/i2c/i2c-core.c |5 + 1 file changed, 5 insertions(+) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 3be58f8..d75b679 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv) static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env) { struct i2c_client *client = to_i2c_client(dev); + int rc; + + rc = acpi_device_uevent_modalias(dev, env); + if (rc != -ENODEV) + return rc; if (add_uevent_var(env, MODALIAS=%s%s, I2C_MODULE_PREFIX, client-name)) -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: Fix modalias for ACPI enumerated I2C devices
Hi, On Thu, 2013-10-10 at 17:17 +0300, Jarkko Nikula wrote: There is a minor fault about ACPI enumerated I2C devices with their modalias attribute. Now modalias is set by device instance not by hardware ID. For example i2c:INTABCD:00, i2c:INTABCD:01 etc. This means each device instance gets different modalias which does match with generated modules.alias. Currently this is not problem as matching can happen also with acpi:INTABCD modalias. IMO, this is not the proper fix for the modalias problem because ACPI enumerated I2C device may have compatible ids. Instead, we should export all the compatible ids as the modules alias of the ACPI enumerated I2C device. can you please take a look at the patch I sent out earlier? https://patchwork.kernel.org/patch/3034991/ https://patchwork.kernel.org/patch/3035041/ https://patchwork.kernel.org/patch/3035021/ thanks, rui Fix this by using ACPI hardware ID. Signed-off-by: Jarkko Nikula jarkko.nik...@linux.intel.com Cc: Mika Westerberg mika.westerb...@linux.intel.com --- Generated on top of v3.12-rc4-29-g0e7a3ed. --- drivers/i2c/i2c-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 29d3f04..6dd0c53 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -,7 +,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, if (ret 0 || !info.addr) return AE_OK; - strlcpy(info.type, dev_name(adev-dev), sizeof(info.type)); + strlcpy(info.type, acpi_device_hid(adev), sizeof(info.type)); if (!i2c_new_device(adapter, info)) { dev_err(adapter-dev, failed to add I2C device %s from ACPI\n, -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] ACPI: add module autoloading support for ACPI enumerated devices
On Mon, 2013-10-14 at 13:14 +0300, Mika Westerberg wrote: On Mon, Oct 14, 2013 at 05:10:32PM +0800, Zhang Rui wrote: An ACPI enumerated device may have its compatible id strings. To support the compatible ACPI ids (acpi_device-pnp.ids), we introduced acpi_driver_match_device() to match the driver-acpi_match_table and acpi_device-pnp.ids. For those drivers, MODULE_DEVICE_TABLE(acpi, xxx) is used to exports the driver module alias in the format of acpi:device_compatible_ids. But in the mean time, the current code does not export the ACPI compatible strings as part of the module_alias for the ACPI enumerated devices, which will break the module autoloading. Take the following piece of code for example, static const struct acpi_device_id xxx_acpi_match[] = { { INTABCD, 0 }, { } }; MODULE_DEVICE_TABLE(acpi, xxx_acpi_match); If this piece of code is used in a platform driver for an ACPI enumerated platform device, the platform driver module_alias is acpi:INTABCD, but the uevent attribute of its platform device node is platform:INTABCD:00 (PREFIX:pdev-name). If this piece of code is used in an i2c driver for an ACPI enumerated i2c device, the i2c driver module_alias is acpi:INTABCD, but the uevent of its i2c device node is i2c:INTABCD:00 (PREFIX:client-name). The reason why the module autoloading is not broken for now is that the uevent file of the ACPI device node is acpi:INTABCD. Thus it is the ACPI device node creation that loads the platform/i2c driver. So this is a problem that will affect us the day when the ACPI bus is removed from device model. This patch introduces a new function to exporting ACPI ids as the module_alias, for the ACPI enumerate devices. For any drivers using MODULE_DEVICE_TALBE(acpi, blabla), the uevent file ^ TABLE? of its associated device must be fixed by invoking this function. Signed-off-by: Zhang Rui rui.zh...@intel.com Makes sense. There is one comment below but other than that, you can add my, Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com Thanks for reviewing. For the three patches. I wonder if we should do the same for SPI bus as well? yes, you are right. Actually, I should check for buses that supports acpi_driver_match_device() and fix them all. Will add it in next version. thanks, rui --- drivers/acpi/scan.c | 24 include/linux/acpi.h |8 2 files changed, 32 insertions(+) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 407ad13..db6f879 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -111,6 +111,30 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias, return len; } +int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env) +{ + struct acpi_device *acpi_dev; + int result; + int len; + + result = acpi_bus_get_device(ACPI_HANDLE(dev), acpi_dev); + if (result) + return result; + + if (list_empty(acpi_dev-pnp.ids)) + return 0; + + if (add_uevent_var(env, MODALIAS=)) + return -ENOMEM; + len = create_modalias(acpi_dev, env-buf[env-buflen - 1], + sizeof(env-buf) - env-buflen); + if (len = (sizeof(env-buf) - env-buflen)) + return -ENOMEM; + env-buflen += len; + return 0; +} +EXPORT_SYMBOL(acpi_device_uevent_modalias); _GPL? -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: Fix modalias for ACPI enumerated I2C devices
On Mon, 2013-10-14 at 14:18 +0300, Jarkko Nikula wrote: On 10/14/2013 12:23 PM, Zhang Rui wrote: Hi, On Thu, 2013-10-10 at 17:17 +0300, Jarkko Nikula wrote: There is a minor fault about ACPI enumerated I2C devices with their modalias attribute. Now modalias is set by device instance not by hardware ID. For example i2c:INTABCD:00, i2c:INTABCD:01 etc. This means each device instance gets different modalias which does match with generated modules.alias. Currently this is not problem as matching can happen also with acpi:INTABCD modalias. IMO, this is not the proper fix for the modalias problem because ACPI enumerated I2C device may have compatible ids. Instead, we should export all the compatible ids as the modules alias of the ACPI enumerated I2C device. can you please take a look at the patch I sent out earlier? https://patchwork.kernel.org/patch/3034991/ https://patchwork.kernel.org/patch/3035041/ https://patchwork.kernel.org/patch/3035021/ I see. This makes sense as it avoids that same device has two different modaliases from both acpi and other subsystem. How about modalias nodes in sysfs, should they also reflect what is matching uvent? good catch, will fix modalias as well in next version. thanks, rui -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 1/6] Introduce acpi_match_device_id().
-Original Message- From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com] Sent: Monday, October 01, 2012 2:38 PM To: Zhang, Rui Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown; Rafael J. Wysocki; Grant Likely; Dirk Brandewie Subject: Re: [RFC PATCH 1/6] Introduce acpi_match_device_id(). Importance: High On Sat, Sep 29, 2012 at 01:31:52PM +, Zhang, Rui wrote: +{ + acpi_handle handle = DEVICE_ACPI_HANDLE(dev); If the device is not bound to an ACPI handle this will return NULL. And I don't see you doing that in this series meaning that.. You're right, I should set pdev-archdata.acpi_handle to the I2C controller in i2c_root.c. There already is an API for that - check drivers/acpi/glue.c. Do you mean acpi_bind? acpi_bind_one will bind the physical device node to the ACPI device, But for the i2c controller ACPI device, the physical node is the I2C adapter in the I2C bus. If we introduce I2C bus ACPI binding, which is what we're doing now, the i2c_adapter-dev.archdata.acpi_handle will be set to the ACPI i2c controller handle by acpi_bind_one. Thanks, rui -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 2/6] Introduce ACPI style match in platform_match
-Original Message- From: linux-acpi-ow...@vger.kernel.org [mailto:linux-acpi- ow...@vger.kernel.org] On Behalf Of Mika Westerberg Sent: Monday, October 01, 2012 2:47 PM To: Zhang, Rui Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown; Rafael J. Wysocki; Grant Likely; Dirk Brandewie Subject: Re: [RFC PATCH 2/6] Introduce ACPI style match in platform_match Importance: High On Fri, Sep 28, 2012 at 03:39:15PM +0800, Zhang Rui wrote: From 5d7ecd12c2994b8c5905d52718c2870c3b62746e Mon Sep 17 00:00:00 2001 From: Zhang Rui rui.zh...@intel.com Date: Fri, 28 Sep 2012 14:51:03 +0800 Subject: [RFC PATCH 2/6] Introduce ACPI style match in platform_match Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/base/platform.c |8 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index a1a7225..90e64c6f 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -20,6 +20,7 @@ #include linux/err.h #include linux/slab.h #include linux/pm_runtime.h +#include linux/acpi.h #include base.h @@ -635,6 +636,13 @@ static const struct platform_device_id *platform_match_id( struct platform_device *pdev) { while (id-name[0]) { +#ifdef CONFIG_ACPI I don't think the above is needed as you stub the acpi_match_device_id() out when !CONFIG_ACPI. You're right, I'll remove this. How about I2C and SPI slave devices? We're introduce the I2C/SPI bus ACPI binding, and the i2c/spi bus .match method can be redirected to acpi callbacks. Thanks, rui -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver
-Original Message- From: linux-i2c-ow...@vger.kernel.org [mailto:linux-i2c- ow...@vger.kernel.org] On Behalf Of Mika Westerberg Sent: Monday, October 01, 2012 2:55 PM To: Zhang, Rui Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown; Rafael J. Wysocki; Grant Likely; Dirk Brandewie Subject: Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver Importance: High On Fri, Sep 28, 2012 at 03:40:32PM +0800, Zhang Rui wrote: +acpi_status __init i2c_enumerate_slave(acpi_handle handle, u32 level, + void *data, void **return_value) { + int result; + acpi_status status; + struct acpi_buffer buffer; + struct acpi_resource *resource; + struct acpi_resource_gpio *gpio; + struct acpi_resource_i2c_serialbus *i2c; + int i; + struct acpi_i2c_root *root = data; + struct i2c_board_info info; + struct acpi_device *device; + + if (acpi_bus_get_device(handle, device)) + return AE_OK; + + status = acpi_get_current_resources(handle, buffer); + if (ACPI_FAILURE(status)) { + dev_err(device-dev, Failed to get ACPI resources\n); + return AE_OK; + } + + for (i = 0; i buffer.length; i += sizeof(struct acpi_resource)) { + resource = (struct acpi_resource *)(buffer.pointer + i); + + switch (resource-type) { + case ACPI_RESOURCE_TYPE_GPIO: + gpio = resource-data.gpio; + + if (gpio-connection_type == ACPI_RESOURCE_GPIO_TYPE_INT) { + result = + acpi_device_get_gpio_irq + (gpio-resource_source.string_ptr, +gpio-pin_table[0], info.irq); acpi_device_get_gpio_irq() is not defined in this patch series? ACPI GPIO controller patch has already been sent out, but in ACPI mailing list only. Also you need to do the gpio_request()/gpio_to_irq() things somewhere. Are they handled in acpi_device_get_gpio_irq()? Yep. How about GpioIo resources? This is not covered in this patch set, but will be in the next patch set. + if (result) + dev_err(device-dev, + Failed to get IRQ\n); + } + break; + case ACPI_RESOURCE_TYPE_SERIAL_BUS: + i2c = resource-data.i2c_serial_bus; + + info.addr = i2c-slave_address; + break; + default: + break; + } + } + + add_slave(root, info); + + kfree(buffer.pointer); + return AE_OK; +} + +static int __devinit acpi_i2c_root_add(struct acpi_device *device) { + acpi_status status; + struct acpi_i2c_root *root; + struct resource *resources; + int result; + + if (!device-pnp.unique_id) { + dev_err(device-dev, + Unsupported ACPI I2C controller. No UID\n); Where does this restriction come from? As far as I understand UID is optional. _UID is optional. But it seems to be required for SPB buses that need ACPI device enumeration. At least this is true for the ACPI 5 compatible ACPI tables I have. Thanks, rui -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal
-Original Message- From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com] Sent: Monday, October 01, 2012 2:45 PM To: Zhang, Rui Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown; Rafael J. Wysocki; Grant Likely; Dirk Brandewie Subject: Re: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal Importance: High On Fri, Sep 28, 2012 at 03:37:43PM +0800, Zhang Rui wrote: the main idea is that, for Serial Buses like I2C and SPI, we enumerate the controller as a platform device, and then enumerate the slaves via i2c/spi_register_board_info. And then, when the controller is really probed and enabled in the platform driver, the SPI/I2C bus code will enumerate I2C/SPI slaves automatically. And for the other devices, we will enumerate all of them as platform devices, which is not covered in this patch set yet. Can you show some example how we could use this new code for example with an existing I2C/SPI slave driver? This is just prototype patch set that I want to illustrate my idea on ACPI device enumeration, to get more thoughts on this. So no example driver so far. Let's say the device uses few GPIOs, one for interrupt and other for triggering firmware download. In addition to that it needs a special parameters that can be extracted running the _DSM method of the device. Normally the driver would get this stuff from the platform data or from Device Tree but how it is done with these patches? Can you show me an example driver that gets the special parameters from Device Tree? Thanks, rui -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 1/6] Introduce acpi_match_device_id().
-Original Message- From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com] Sent: Friday, September 28, 2012 10:14 PM To: Zhang, Rui Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown; Rafael J. Wysocki; Grant Likely; Dirk Brandewie Subject: Re: [RFC PATCH 1/6] Introduce acpi_match_device_id(). Importance: High On Fri, Sep 28, 2012 at 03:38:30PM +0800, Zhang Rui wrote: From 72df5d1f51fb27a4ba7f70a3b07df759d32b8288 Mon Sep 17 00:00:00 2001 From: Zhang Rui rui.zh...@intel.com Date: Thu, 27 Sep 2012 15:11:55 +0800 Subject: [RFC PATCH 1/6] Introduce acpi_match_device_id(). This API is used to check if a device id string is compatible with an ACPI device, either PNP id exported via _HID or compatible ids exported via _CID control method. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/acpi/scan.c | 22 ++ include/acpi/acpi_bus.h |6 ++ 2 files changed, 28 insertions(+), 0 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index d1ecca2..936a7c9 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -312,6 +312,28 @@ int acpi_match_device_ids(struct acpi_device *device, } EXPORT_SYMBOL(acpi_match_device_ids); +int acpi_match_device_id(const struct device *dev, const char *id) Would be good idea to implement this in terms of of_match_device() so that it returns pointer to the matched id. This way drivers can get the -driver_data pretty easily if needed. Good idea. Will do that in v2. +{ + acpi_handle handle = DEVICE_ACPI_HANDLE(dev); If the device is not bound to an ACPI handle this will return NULL. And I don't see you doing that in this series meaning that.. You're right, I should set pdev-archdata.acpi_handle to the I2C controller in i2c_root.c. Thanks, rui -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver
-Original Message- From: Alan Cox [mailto:a...@lxorguk.ukuu.org.uk] Sent: Friday, September 28, 2012 8:54 PM To: Zhang, Rui Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown; Rafael J. Wysocki; Grant Likely; Dirk Brandewie Subject: Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver Importance: High On Fri, 28 Sep 2012 15:40:32 +0800 Zhang Rui rui.zh...@intel.com wrote: From 6077a62f2865201ab6727ca7d628ee5e43aa57e1 Mon Sep 17 00:00:00 2001 From: Zhang Rui rui.zh...@intel.com Date: Fri, 24 Aug 2012 15:18:25 +0800 Subject: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver This driver is able to 1) enumerate I2C controller via ACPI namespace and register it as a platform device. 2) enumerate I2C slave devices via ACPI namespace. Will this also trigger and work with the ACPI4 based devices that seem to have I²C tables that Linux currently doesn't understand (eq some GMA600/Oaktrail platforms) ? I'm not aware of this. I think the answer is probably No because these PNPids are pretty new. But we can check FADT.revision to make the driver work on ACPI5 platforms only. Thanks, rui
[RFC PATCH 1/6] Introduce acpi_match_device_id().
From 72df5d1f51fb27a4ba7f70a3b07df759d32b8288 Mon Sep 17 00:00:00 2001 From: Zhang Rui rui.zh...@intel.com Date: Thu, 27 Sep 2012 15:11:55 +0800 Subject: [RFC PATCH 1/6] Introduce acpi_match_device_id(). This API is used to check if a device id string is compatible with an ACPI device, either PNP id exported via _HID or compatible ids exported via _CID control method. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/acpi/scan.c | 22 ++ include/acpi/acpi_bus.h |6 ++ 2 files changed, 28 insertions(+), 0 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index d1ecca2..936a7c9 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -312,6 +312,28 @@ int acpi_match_device_ids(struct acpi_device *device, } EXPORT_SYMBOL(acpi_match_device_ids); +int acpi_match_device_id(const struct device *dev, const char *id) +{ + acpi_handle handle = DEVICE_ACPI_HANDLE(dev); + struct acpi_device *device; + struct acpi_hardware_id *hwid; + acpi_status status; + + if (!handle || !id) + return -ENODEV; + + status = acpi_bus_get_device(handle, device); + if (ACPI_FAILURE(status)) + return -ENODEV; + + list_for_each_entry(hwid, device-pnp.ids, list) + if (!strcmp(id, hwid-id)) + return 0; + + return -ENODEV; +} +EXPORT_SYMBOL(acpi_match_device_id); + static void acpi_free_ids(struct acpi_device *device) { struct acpi_hardware_id *id, *tmp; diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index bde976e..8b5b124 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -378,6 +378,7 @@ int acpi_bus_start(struct acpi_device *device); acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd); int acpi_match_device_ids(struct acpi_device *device, const struct acpi_device_id *ids); +int acpi_match_device_id(const struct device *, const char *); int acpi_create_dir(struct acpi_device *); void acpi_remove_dir(struct acpi_device *); @@ -448,6 +449,11 @@ static inline int acpi_pm_device_sleep_wake(struct device *dev, bool enable) static inline int register_acpi_bus_type(void *bus) { return 0; } static inline int unregister_acpi_bus_type(void *bus) { return 0; } +static inline int acpi_match_device_id(const struct device *device, + const char *name) +{ + return -ENODEV; +} #endif /* CONFIG_ACPI */ -- 1.7.7.6 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 3/6] ACPI: introduce acpi_get_generic_resources
From 9a851d177794129a89f720c7122cb39fd163126b Mon Sep 17 00:00:00 2001 From: Zhang Rui rui.zh...@intel.com Date: Fri, 28 Sep 2012 08:34:05 +0800 Subject: [RFC PATCH 3/6] ACPI: introduce acpi_get_generic_resources Introduce acpi_get_generic_resources() to convert ACPI style resources to struct resource. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/acpi/Makefile |1 + drivers/acpi/resource.c | 165 +++ include/acpi/acpi_bus.h |1 + 3 files changed, 167 insertions(+), 0 deletions(-) create mode 100644 drivers/acpi/resource.c diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 6b1d535..4b65608 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -46,6 +46,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o ifdef CONFIG_ACPI_VIDEO acpi-y += video_detect.o endif +acpi-y += resource.o # These are (potentially) separate modules obj-$(CONFIG_ACPI_AC) += ac.o diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c new file mode 100644 index 000..30a5204 --- /dev/null +++ b/drivers/acpi/resource.c @@ -0,0 +1,165 @@ +/* + * resource.c -- convert ACPI resource to generic resource + * + * Copyright (c) 2012 Zhang Rui rui.zh...@intel.com + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + */ +#include linux/kernel.h +#include linux/export.h +#include linux/acpi.h + +static int irq_flags(int triggering, int polarity, int sharable) +{ + int flags; + + if (triggering == ACPI_LEVEL_SENSITIVE) { + if (polarity == ACPI_ACTIVE_LOW) + flags = IORESOURCE_IRQ_LOWLEVEL; + else + flags = IORESOURCE_IRQ_HIGHLEVEL; + } else { + if (polarity == ACPI_ACTIVE_LOW) + flags = IORESOURCE_IRQ_LOWEDGE; + else + flags = IORESOURCE_IRQ_HIGHEDGE; + } + + if (sharable == ACPI_SHARED) + flags |= IORESOURCE_IRQ_SHAREABLE; + + return flags; +} + +static void acpi_get_irq_resource(struct acpi_resource *res, + struct resource *resource) +{ + struct acpi_resource_irq *irq = res-data.irq; + int t, p; + + if (irq-interrupt_count == 0) + resource-flags = IORESOURCE_DISABLED; + + if (!acpi_get_override_irq(irq-interrupts[0], t, p)) { + t = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE; + p = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH; + + if (irq-triggering != t || irq-polarity != p) { + irq-triggering = t; + irq-polarity = p; + } + } + + resource-flags = + irq_flags(irq-triggering, irq-polarity, irq-sharable); + resource-flags |= IORESOURCE_IRQ; + resource-start = irq-interrupts[0]; + resource-end = irq-interrupts[0]; +} + +static void acpi_get_extended_irq_resource(struct acpi_resource *res, + struct resource *resource) +{ + struct acpi_resource_extended_irq *irq = res-data.extended_irq; + int t, p; + + if (irq-interrupt_count == 0) + resource-flags = IORESOURCE_DISABLED; + + if (!acpi_get_override_irq(irq-interrupts[0], t, p)) { + t = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE; + p = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH; + + if (irq-triggering != t || irq-polarity != p) { + irq-triggering = t; + irq-polarity = p; + } + } + + resource-flags = + irq_flags(irq-triggering, irq-polarity, irq-sharable); + resource-flags |= IORESOURCE_IRQ; + resource-start = irq-interrupts[0]; + resource-end = irq-interrupts[0]; +} + +static void acpi_get_mem_resource(struct acpi_resource *res, + struct resource *resource) +{ + struct acpi_resource_fixed_memory32 *mem = res-data.fixed_memory32; + + if (mem-address_length == 0) + resource-flags |= IORESOURCE_DISABLED; + if (mem-write_protect == ACPI_READ_WRITE_MEMORY) + resource-flags |= IORESOURCE_MEM_WRITEABLE; + + resource-flags |= IORESOURCE_MEM; + resource-start = mem-address; + resource-end = mem-address + mem-address_length - 1; +} + +int acpi_get_generic_resources(struct acpi_device *device, + struct resource
[RFC PATCH 4/6] Change i2c_register_board_info from __init to __devinit
From 34aa38e12c04544d89af2eae46de284dc8a03b8d Mon Sep 17 00:00:00 2001 From: Zhang Rui rui.zh...@intel.com Date: Thu, 27 Sep 2012 15:42:23 +0800 Subject: [RFC PATCH 4/6] Change i2c_register_board_info from __init to __devinit. ACPI 5 supports enumerating I2C adapter and its slaves via ACPI namespace, and this needs to be done at runtime, after the ACPI I2C controller driver being loaded. detailed usage of this API can be found in next patch. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/i2c/i2c-boardinfo.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/i2c/i2c-boardinfo.c b/drivers/i2c/i2c-boardinfo.c index f24cc64..1ecbbdc 100644 --- a/drivers/i2c/i2c-boardinfo.c +++ b/drivers/i2c/i2c-boardinfo.c @@ -61,7 +61,7 @@ EXPORT_SYMBOL_GPL(__i2c_first_dynamic_bus_num); * The board info passed can safely be __initdata, but be careful of embedded * pointers (for platform_data, functions, etc) since that won't be copied. */ -int __init +int __devinit i2c_register_board_info(int busnum, struct i2c_board_info const *info, unsigned len) { -- 1.7.7.6 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver
From 6077a62f2865201ab6727ca7d628ee5e43aa57e1 Mon Sep 17 00:00:00 2001 From: Zhang Rui rui.zh...@intel.com Date: Fri, 24 Aug 2012 15:18:25 +0800 Subject: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver This driver is able to 1) enumerate I2C controller via ACPI namespace and register it as a platform device. 2) enumerate I2C slave devices via ACPI namespace. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/acpi/Makefile |1 + drivers/acpi/i2c_root.c | 229 +++ drivers/acpi/sysfs.c|1 + include/acpi/acpi_drivers.h |1 + 4 files changed, 232 insertions(+), 0 deletions(-) create mode 100644 drivers/acpi/i2c_root.c diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 4b65608..5b14f05 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -35,6 +35,7 @@ acpi-y+= scan.o acpi-y += processor_core.o acpi-y += ec.o acpi-y += gpio.o +acpi-y += i2c_root.o acpi-$(CONFIG_ACPI_DOCK) += dock.o acpi-y += pci_root.o pci_link.o pci_irq.o pci_bind.o acpi-y += power.o diff --git a/drivers/acpi/i2c_root.c b/drivers/acpi/i2c_root.c new file mode 100644 index 000..b9a042b --- /dev/null +++ b/drivers/acpi/i2c_root.c @@ -0,0 +1,229 @@ +/* + * i2c_root.c - ACPI I2C controller Driver ($Revision: 40 $) + * + * Copyright (C) 2012 Intel Corp + * Copyright (C) 2012 Zhang Rui rui.zh...@intel.com + * + * ~~ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA. + * + * ~~ + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/init.h +#include linux/types.h +#include linux/spinlock.h +#include linux/pm.h +#include linux/pm_runtime.h +#include linux/platform_device.h +#include linux/i2c.h +#include linux/acpi.h +#include linux/slab.h +#include acpi/acpi_bus.h +#include acpi/acpi_drivers.h +#include acpi/apei.h + +#define PREFIX ACPI: + +#define _COMPONENT ACPI_SPB_COMPONENT +ACPI_MODULE_NAME(i2c_root); +#define ACPI_I2C_ROOT_CLASSi2c_root +#define ACPI_I2C_ROOT_DEVICE_NAME I2C Controller + +static int acpi_i2c_root_add(struct acpi_device *device); +static int acpi_i2c_root_remove(struct acpi_device *device, int type); + +static const struct acpi_device_id root_device_ids[] = { + {INT33B1, 0}, + {, 0}, +}; + +MODULE_DEVICE_TABLE(acpi, root_device_ids); + +static struct acpi_driver acpi_i2c_root_driver = { + .name = i2c_root, + .class = ACPI_I2C_ROOT_CLASS, + .ids = root_device_ids, + .ops = { + .add = acpi_i2c_root_add, + .remove = acpi_i2c_root_remove, + }, +}; + +struct acpi_i2c_root { + struct acpi_device *device; + struct platform_device *pdev; + int busnum; + int slaves; + struct i2c_board_info *info; +}; + +static int add_slave(struct acpi_i2c_root *root, struct i2c_board_info *info) +{ + struct i2c_board_info *p; + + if (!info) + return 0; + + p = kzalloc(sizeof(*p) * (root-slaves + 1), GFP_KERNEL); + if (!p) + return -ENOMEM; + + memcpy(p, info, sizeof(*p)); + if (root-info) + memcpy(p + 1, root-info, sizeof(*p) * root-slaves); + + kfree(root-info); + root-info = p; + root-slaves++; + return 0; +} + +static int register_slaves(struct acpi_i2c_root *root) +{ + return i2c_register_board_info(root-busnum, root-info, root-slaves); +} + +/* + * The i2c info registering call back for each i2c slave device + */ +acpi_status __init i2c_enumerate_slave(acpi_handle handle, u32 level, + void *data, void **return_value) +{ + int result; + acpi_status status; + struct acpi_buffer buffer; + struct acpi_resource *resource; + struct acpi_resource_gpio *gpio; + struct acpi_resource_i2c_serialbus *i2c; + int i; + struct acpi_i2c_root *root = data; + struct i2c_board_info info; + struct acpi_device *device
[RFC PATCH 6/6] Introduce INT33B1 I2C controller driver
From 817d814ecae91862f42a0447f455dae7f74cba27 Mon Sep 17 00:00:00 2001 From: Zhang Rui rui.zh...@intel.com Date: Fri, 24 Aug 2012 15:20:38 +0800 Subject: [RFC PATCH 6/6] Introduce INT33B1 I2C controller driver This is a dummy platform device driver to illustrate my idea about how a really I2C controller should work on ACPI 5 platforms. It just probes the INT33B1 I2C controller which is enumerated by ACPI. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/i2c/busses/Kconfig|8 +++ drivers/i2c/busses/Makefile |1 + drivers/i2c/busses/i2c-33b1.c | 117 + 3 files changed, 126 insertions(+), 0 deletions(-) create mode 100644 drivers/i2c/busses/i2c-33b1.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index b4aaa1b..536a19c 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -257,6 +257,14 @@ config I2C_SCMI To compile this driver as a module, choose M here: the module will be called i2c-scmi. +config I2C_33B1 + tristate INT33B1 I2C controller + help + This driver supports the ACPI enumerated INT33B1 I2C controller. + + To compile this driver as a module, choose M here: + the module will be called i2c-scmi. + endif # ACPI comment Mac SMBus host controller drivers diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index ce3c2be..8e478bd 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -4,6 +4,7 @@ # ACPI drivers obj-$(CONFIG_I2C_SCMI) += i2c-scmi.o +obj-$(CONFIG_I2C_33B1) += i2c-33b1.o # PC SMBus host controller drivers obj-$(CONFIG_I2C_ALI1535) += i2c-ali1535.o diff --git a/drivers/i2c/busses/i2c-33b1.c b/drivers/i2c/busses/i2c-33b1.c new file mode 100644 index 000..152c3e4 --- /dev/null +++ b/drivers/i2c/busses/i2c-33b1.c @@ -0,0 +1,117 @@ +/* + * i2c_33b1.c -INT33B1 i2c Controller Driver + * + * Copyright (c) 2012 Intel Corp + * Copyright (c) 2012 Zhang Rui rui.zh...@intel.com + * + * ~~ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * ~~ + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/slab.h +#include linux/init.h +#include linux/ioport.h +#include linux/delay.h +#include linux/errno.h +#include linux/i2c.h +#include linux/io.h +#include linux/platform_device.h + + +struct int33b1_i2c_private { +struct i2c_adapter adap; +void __iomem *iobase; +}; + + +static int int33b1_i2c_xfer(struct i2c_adapter *adap, + struct i2c_msg *msgs, int num) +{ + return 0; +} + +static u32 int33b1_func(struct i2c_adapter *adap) +{ + /* Emulate SMBUS over I2C */ + return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C; +} + +static struct i2c_algorithm int33b1_i2c_algo = { + .master_xfer= int33b1_i2c_xfer, + .functionality = int33b1_func, +}; + +static int __devinit int33b1_i2c_probe(struct platform_device *pdev) +{ + struct int33b1_i2c_private *priv; + struct resource *res; + int ret; + + priv = devm_kzalloc(pdev-dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + priv-iobase = devm_request_and_ioremap(pdev-dev, res); + if (!priv-iobase) { + dev_err(pdev-dev, devm_request_and_ioremap failed\n); + return -EBUSY; + } + + priv-adap.dev.parent = pdev-dev; + priv-adap.owner= THIS_MODULE; + priv-adap.algo_data= priv; + priv-adap.algo = int33b1_i2c_algo; + priv-adap.nr = pdev-id; + priv-adap.class= I2C_CLASS_HWMON; + snprintf(priv-adap.name, sizeof(priv-adap.name), int33b1-i2c); + + i2c_set_adapdata(priv-adap, priv); + ret = i2c_add_numbered_adapter(priv-adap); + if (ret 0) { + dev_err(priv-adap.dev, Failed to add i2c bus.\n); + return ret; + } + + platform_set_drvdata(pdev, priv); + dev_info(priv-adap.dev, Added I2C Bus.\n); + return 0; +} + +static int __devexit int33b1_i2c_remove(struct platform_device *pdev) +{ + struct int33b1_i2c_private *priv; + + priv = platform_get_drvdata(pdev); + i2c_del_adapter(priv-adap); + platform_set_drvdata(pdev, NULL); + return 0; +} + +static struct platform_driver
Re: Fwd: Hid over I2C and ACPI interaction
On 四, 2012-07-05 at 10:44 +0200, Benjamin Tissoires wrote: Hi, Many thanks for these information. It seems like I was on the right track, but I didn't saw the hidden part of the iceberg. yep. I've already written the i2c slave part (and the acpi handling to get the HID register by using the DSM should work), but I need now the whole ACPI pnp drivers... you need the ACPI/PNP I2C controller driver. But without a real ACPI 5.0 mainboard, I think it will be quite difficult to implement and debug this ACPI stuff. yes, that's the problem I have. I can not send out the code based on some example ASL code. :) thanks, rui Cheers, Benjamin On Thu, Jul 5, 2012 at 9:20 AM, Zhang Rui rui.zh...@intel.com wrote: Hah, seems I forgot to reply to Benjamin. On 四, 2012-07-05 at 15:01 +0800, Zhang Rui wrote: Original Message Subject: Hid over I2C and ACPI interaction Date: Wed, 4 Jul 2012 15:46:35 +0200 From: Benjamin Tissoires benjamin.tissoi...@gmail.com To: Jean Delvare kh...@linux-fr.org, Ben Dooks ben-li...@fluff.org, Wolfram Sang w.s...@pengutronix.de, Len Brown l...@kernel.org, linux-a...@vger.kernel.org, linux-i2c@vger.kernel.org, linux-ker...@vger.kernel.org CC: Jiri Kosina jkos...@suse.cz, Stéphane Chatty cha...@enac.fr, JJ Ding jj_d...@emc.com.tw Hi Guys, I'm the co-author and the maintainer of the hid-multitouch driver. To support even more devices, I started the implementation of the HID over I2C protocol specification which is introduced by Win8. I'm quite comfortable with the hid and the I2C part, but I'm blocked with the interaction with the ACPI for the pnp part. I wanted to have your advice/help on this problem. I've add in the recipients list the maintainers of i2c and ACPI, sorry for the noise if you don't feel concerned about this. So, let's go deeper in the problem ;-) Microsoft's spec asks the OEM to fill the ACPI DSDT to provide the following scope in the ASL layout: begin of ASL Scope (\_SB) { // // General Purpose I/O, ports 0...127 // Device(HIDI2C_DEVICE1) { Name(_ADR,0) Name (_HID, MSFT1234”) Name (_CID, PNP0C50) Name (_UID, 3) Method(_CRS, 0x0, NotSerialized) { Name (RBUF, ResourceTemplate () { // Address 0x07 on I2C-X (OEM selects this address) //IHV SPECIFIC I2C3 = I2C Controller; TGD0 = GPIO Controller; I2CSerialBus (0x07, ControllerInitiated, 10,AddressingMode7Bit, \\_SB.I2C3) GpioInt(Level, ActiveLow, Exclusive, PullUp, 0, \\_SB. TGD0, 0 , ResourceConsumer, , ) {40} }) Return(RBUF) } Method(_DSM, 0x4, NotSerialized) { // BreakPoint Store (Method _DSM begin, Debug) // DSM UUID switch(ToBuffer(Arg0)) { // ACPI DSM UUID for HIDI2C case(ToUUID(3CDFF6F7-4267-4555-AD05-B30A3D8938DE)) { // DSM function which returns the HID Descriptor Address (skipped) } default { // No other GUIDs supported Return(Buffer(One) { 0x00 }) } } } } end of ASL yep, this is an ACPI enumerated I2C controller. Summary: - a HID over I2C device has to present the Compatibility ID PNP0C50 - in the _CRS block, the address, the adapter and the gpioInt are defined (or referenced) - it presents a Device Specific Method (_DSM) which returns the HID Descriptor register address. This register is our entry point for retrieving the information about our hid device (so it's mandatory to obtain it). Where am I: - I've written a first layer on top of i2c that retrieves the hid register (currently the address 0x0001 is hardcoded), then get the report desccriptors and the input events, and forward all this stuff to the hid layer. - It's working with a custom emulated HID over i2c touchpad, while waiting for the one a manufacturer should send to me. - The detection and the addition to the adapter is done by adding the address in the lists and the name through the i2c -detect callback (which is not very good, because I don't have the interrupt line there). - I've written a first acpi implementation that rely on the DEVICE_ACPI_HANDLE macro to get the ACPI handle of the device (if available). - I'm not able to do some tests with the ACPI, as I don't know how to implement this DSDT on my computer (I'm missing the I2C part), and the manufacturer returned the mainboard with the right DSDT to the OEM. My questions: - will the current acpi implementation handle I2C devices? you still need to write your own
Re: Fwd: Hid over I2C and ACPI interaction
driver for SPI controller and SD/MMC controller. Third, you need a I2C slave device driver to handle the I2C slave device in I2C bus. here is a BKM I wrote, hope it helps. And also any comments are welcome. :) From 0a0fa4ff7b4b06c6560de94a78b15c6adfd86e34 Mon Sep 17 00:00:00 2001 From: Zhang Rui rui.zh...@intel.com Date: Mon, 26 Dec 2011 10:42:04 +0800 As many SoC IP blocks are not hardware self-enumerable, the firmware, aka, ACPI tables, is responsible for enumerating/reserving/assigning system resources to these devices. This tutorial talks about how to enumerate these devices via ACPI namespace. Signed-off-by: Zhang Rui rui.zh...@intel.com --- Documentation/acpi/acpi-device-probing.txt | 466 1 file changed, 466 insertions(+) create mode 100644 Documentation/acpi/acpi-device-probing.txt diff --git a/Documentation/acpi/acpi-device-probing.txt b/Documentation/acpi/acpi-device-probing.txt new file mode 100644 index 000..82efbf3 --- /dev/null +++ b/Documentation/acpi/acpi-device-probing.txt @@ -0,0 +1,466 @@ + +HOWTO enumerate devices via ACPI + +Copyright (c) 2011-2012 Intel Corporation + +Contrast to hardware self-enumerable devices(e.g. USB, PCI) on PC platform, +many SoC IP blocks can not be self enumerated. +We used to introduce platform specific code for these devices. +But now, with ACPI 5.0, there is no requirement for the hardware to be +self-discoverable, enumerable or re-locatable, as the firmware is responsible +for enumerating/reserving/assigning system resources (such as address ranges or +interrupts) to the device. + +This document will show how to enumerate and configure a device via ACPI. +If you want to get more details about why and when we need this, +please refer to ACPI spec 5.0 and +Intel Architecture Platform Compatibility Definition. + +Note that although these are ACPI devices, we prefer to use PnP drivers for them, +this is because: +1. all the non-ACPI-predefined Devices are exported as PnP devices as well +2. PnP bus is a well designed bus. Probing via PnP layer saves a lot of work + for the device driver, e.g. getting parsing ACPI resources. + += +1. Understand device definition in ACPI namespace + [Case study 1] SD/MMC controller +2. Driver for a leaf device + 2.1 Make a list of supported PnP ids + 2.2 Implement .probe/.remove callbacks for the PnP driver + 2.3 Fill in the pnp_driver structure + 2.4 Register the PnP driver +3. Driver for a master device on a non-self-enumerable bus + [Case Study 2] SPI controller and its slave device + 3.1 Probe the master device + 3.2 Walk ACPI namesapce to get the child devices of the master device + 3.3 Register these child devices as slave devices + 3.4 Write slave device driver +4. Misc += + +- +1. Understand device definition in ACPI namespace +- + +To enumerate a device in ACPI namespace, we need to find out and understand +HOW the device is defined in ACPI namespace first. + +[Case study 1 ] SD/MMC Controller + +Here is an ASL example code for SD/MMC controller definition in ACPI namespace. + +Device (EMMC) +{ +Name (_ADR, Zero) +/* I use PNP, an arbitrary string, here, as PnP id is device specific */ +Name (_HID, PNP) +Name (_CID, PNP) +Name (_UID, 4) + +Method (_CRS, 0, NotSerialized) +{ +Name (RBUF, ResourceTemplate () +{ +Memory32Fixed (ReadWrite, +0xFFA5, // Address Base +0x0100, // Address Length +) +Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, ) +{ +0x001b, +} +}) +Return (RBUF) +} + +Method (_STA, 0, NotSerialized) +{ +Return (0x0F) +} +} + +_ADR : the address of this device on its parent bus. Useless in this case. +_HID : the PnP id for this device. +_CID : the compatible PnP id. use this as the PnP id if _HID doesn't exist. +_CRS : the system resources currently allocated to this device. + the Memory32Fixed part shows an Mem space for the device, + and the Interrupt part shows the device interrupt. +_STA : the current status of the device, e.g. it's enabled/disabled/removed. + +By reading this example ASL code, we should know that there is a SD/MMC controller +on this platform, it's mem space base address is 0xFFA5, length is 0x0100, +and the irq for this device is 0x1b
Re: Fwd: Hid over I2C and ACPI interaction
Hah, seems I forgot to reply to Benjamin. On 四, 2012-07-05 at 15:01 +0800, Zhang Rui wrote: Original Message Subject: Hid over I2C and ACPI interaction Date: Wed, 4 Jul 2012 15:46:35 +0200 From: Benjamin Tissoires benjamin.tissoi...@gmail.com To: Jean Delvare kh...@linux-fr.org, Ben Dooks ben-li...@fluff.org, Wolfram Sang w.s...@pengutronix.de, Len Brown l...@kernel.org, linux-a...@vger.kernel.org, linux-i2c@vger.kernel.org, linux-ker...@vger.kernel.org CC: Jiri Kosina jkos...@suse.cz, Stéphane Chatty cha...@enac.fr, JJ Ding jj_d...@emc.com.tw Hi Guys, I'm the co-author and the maintainer of the hid-multitouch driver. To support even more devices, I started the implementation of the HID over I2C protocol specification which is introduced by Win8. I'm quite comfortable with the hid and the I2C part, but I'm blocked with the interaction with the ACPI for the pnp part. I wanted to have your advice/help on this problem. I've add in the recipients list the maintainers of i2c and ACPI, sorry for the noise if you don't feel concerned about this. So, let's go deeper in the problem ;-) Microsoft's spec asks the OEM to fill the ACPI DSDT to provide the following scope in the ASL layout: begin of ASL Scope (\_SB) { // // General Purpose I/O, ports 0...127 // Device(HIDI2C_DEVICE1) { Name(_ADR,0) Name (_HID, MSFT1234”) Name (_CID, PNP0C50) Name (_UID, 3) Method(_CRS, 0x0, NotSerialized) { Name (RBUF, ResourceTemplate () { // Address 0x07 on I2C-X (OEM selects this address) //IHV SPECIFIC I2C3 = I2C Controller; TGD0 = GPIO Controller; I2CSerialBus (0x07, ControllerInitiated, 10,AddressingMode7Bit, \\_SB.I2C3) GpioInt(Level, ActiveLow, Exclusive, PullUp, 0, \\_SB. TGD0, 0 , ResourceConsumer, , ) {40} }) Return(RBUF) } Method(_DSM, 0x4, NotSerialized) { // BreakPoint Store (Method _DSM begin, Debug) // DSM UUID switch(ToBuffer(Arg0)) { // ACPI DSM UUID for HIDI2C case(ToUUID(3CDFF6F7-4267-4555-AD05-B30A3D8938DE)) { // DSM function which returns the HID Descriptor Address (skipped) } default { // No other GUIDs supported Return(Buffer(One) { 0x00 }) } } } } end of ASL yep, this is an ACPI enumerated I2C controller. Summary: - a HID over I2C device has to present the Compatibility ID PNP0C50 - in the _CRS block, the address, the adapter and the gpioInt are defined (or referenced) - it presents a Device Specific Method (_DSM) which returns the HID Descriptor register address. This register is our entry point for retrieving the information about our hid device (so it's mandatory to obtain it). Where am I: - I've written a first layer on top of i2c that retrieves the hid register (currently the address 0x0001 is hardcoded), then get the report desccriptors and the input events, and forward all this stuff to the hid layer. - It's working with a custom emulated HID over i2c touchpad, while waiting for the one a manufacturer should send to me. - The detection and the addition to the adapter is done by adding the address in the lists and the name through the i2c -detect callback (which is not very good, because I don't have the interrupt line there). - I've written a first acpi implementation that rely on the DEVICE_ACPI_HANDLE macro to get the ACPI handle of the device (if available). - I'm not able to do some tests with the ACPI, as I don't know how to implement this DSDT on my computer (I'm missing the I2C part), and the manufacturer returned the mainboard with the right DSDT to the OEM. My questions: - will the current acpi implementation handle I2C devices? you still need to write your own device driver for the device. - it seems to me that the .archdata field is left blank during the i2c device initialization in all paths I've seen. Is that true? - who puts the name int the struct i2c_board_info? (for hot-plugged i2c devices). - finally, what is the best way of handling ACPI for those I2C devices: 1) everything is fine, I should have the ACPI handle in .archdata. 2) someone has to implement the handling of I2C in the pnpACPI layer (by adding I2CSerialBus handling and creating there the i2c slave). 3) I should create an acpi driver which handles PNP0C50 and which creates the i2c slaves. exactly. As this I2C controller uses the GPIO interrupt, we need an ACPI GPIO controller driver for interrupts first. I already have such a patch in hand, but have
Re: [PATCH] ALS: TSL2550 driver move from i2c/chips
On Tue, 2009-10-13 at 01:02 +0800, Jonathan Cameron wrote: Jean Delvare wrote: Hi Jonathan, On Mon, 12 Oct 2009 15:19:07 +0100, Jonathan Cameron wrote: @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = { }; /* + * IDR to assign each registered device a unique id + */ +static DEFINE_IDR(tsl2550_idr); +static DEFINE_SPINLOCK(tsl2550_idr_lock); +#define TSL2550_DEV_MAX 9 Such an arbitrary limit is simply not acceptable. Fair enough, but it is based on restricting the size of the char array needed for the name when registering with als. Hence single digit decimal maximum. Do you suggest leaving it unrestricted (which makes code a little fiddly given variable size of max idr) or some other limit? The name size really isn't an issue. You won't notice 16 bytes instead of 9. And it's not like we'll have millions of these devices. I agree, it's merely a question of picking some suitable limit. IDR's on a 64 bit box will do something in the ball park of 2e18 which might be an excessive limit ;) I'll leave this be for next version on basis I'm in favour of moving this into the core and hence as you said this will be irrelevant here anyway. +static int tsl2550_get_id(void) +{ +int ret, val; + +idr_again: +if (unlikely(idr_pre_get(tsl2550_idr, GFP_KERNEL) == 0)) +return -ENOMEM; +spin_lock(tsl2550_idr_lock); +ret = idr_get_new(tsl2550_idr, NULL, val); +if (unlikely(ret == -EAGAIN)) +goto idr_again; +else if (unlikely(ret)) +return ret; +if (val TSL2550_DEV_MAX) +return -ENOMEM; +return val; +} + +static void tsl2550_free_id(int val) +{ +spin_lock(tsl2550_idr_lock); +idr_remove(tsl2550_idr, val); +spin_unlock(tsl2550_idr_lock); +} Having to implement this in _every_ ALS driver sounds wrong and painful. If uniqueness of any kind must be provided, it should be handled by the ALS core and not by individual drivers, otherwise you can be certain that at least one driver will get it wrong someday. I agree. The reason this originally occurred is that the acpi layer apparently doesn't allow for simultaneous probing of multiple drivers and hence can get away with a simple counter and no locking. I'd imaging that als-class devices are simply named als%u. Just like hwmon devices are named hwmon%u, input devices are names input%u and event%u, etc. I don't know of any class pushing the naming down to its drivers, do you? The only example I can remember are i2c drivers back in year 1999, when they were part of lm-sensors.I have personally put an end to this years ago. This debate started in the als thread. Personally I couldn't care less either way but it does need to be put to bed before that subsystem merges. To my mind either approach is trivially handled in a userspace library so it doesn't matter. I don't suppose you can remember what the original reasons for squashing this naming approach were? Code duplication. Having the same unique-id handling code repeated in 50 drivers was no fun, as it did not add any value compared to a central handling. Counter argument placed (cc'd Pavel and Corentin for this point) is that having a generic name, e.g. hwmon0 and a name field in sysfs is superfluous when we can combine the two. @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev, return ret; } -static DEVICE_ATTR(lux1_input, S_IRUGO, +static DEVICE_ATTR(illuminance, S_IRUGO, tsl2550_show_lux1_input, NULL); Question: if I have a light sensing chip with two sensors, how are we going to handle it? Will we register 2 als class devices? The initial naming convention had the advantage that you could have more than one sensor per device, but I don't know if it is desirable in practice. This only becomes and issue if we have two sensors measuring illuminance (or approximation of it). The only two sensor chips I know of have one broadband and one in the infrared tsl2561 and I think the isl chip with a driver currently in misc. The combination of these two are needed to calculate illuminance. Some of the original discussion went into how to support separate access to the individual sensors. Decision was to leave that question until it becomes relevant. Basically we would put the current drivers in just supporting illuminance and see if anyone asked for furthers support. One tricky aspect is what the units should be for particular frequency ranges. At least illuminance is cleanly defined (even if chips are only fairly coarsely approximating it. Hmm, this isn't the point I was raising. I was thinking of a light sensor chip which would sense light in different locations. Think chip with remote sensors. This is
Re: [PATCH] ALS: TSL2550 driver move from i2c/chips
update Pavel's email address. On Fri, 2009-10-16 at 09:37 +0800, Zhang Rui wrote: On Tue, 2009-10-13 at 01:02 +0800, Jonathan Cameron wrote: Jean Delvare wrote: Hi Jonathan, On Mon, 12 Oct 2009 15:19:07 +0100, Jonathan Cameron wrote: @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = { }; /* + * IDR to assign each registered device a unique id + */ +static DEFINE_IDR(tsl2550_idr); +static DEFINE_SPINLOCK(tsl2550_idr_lock); +#define TSL2550_DEV_MAX 9 Such an arbitrary limit is simply not acceptable. Fair enough, but it is based on restricting the size of the char array needed for the name when registering with als. Hence single digit decimal maximum. Do you suggest leaving it unrestricted (which makes code a little fiddly given variable size of max idr) or some other limit? The name size really isn't an issue. You won't notice 16 bytes instead of 9. And it's not like we'll have millions of these devices. I agree, it's merely a question of picking some suitable limit. IDR's on a 64 bit box will do something in the ball park of 2e18 which might be an excessive limit ;) I'll leave this be for next version on basis I'm in favour of moving this into the core and hence as you said this will be irrelevant here anyway. +static int tsl2550_get_id(void) +{ + int ret, val; + +idr_again: + if (unlikely(idr_pre_get(tsl2550_idr, GFP_KERNEL) == 0)) + return -ENOMEM; + spin_lock(tsl2550_idr_lock); + ret = idr_get_new(tsl2550_idr, NULL, val); + if (unlikely(ret == -EAGAIN)) + goto idr_again; + else if (unlikely(ret)) + return ret; + if (val TSL2550_DEV_MAX) + return -ENOMEM; + return val; +} + +static void tsl2550_free_id(int val) +{ + spin_lock(tsl2550_idr_lock); + idr_remove(tsl2550_idr, val); + spin_unlock(tsl2550_idr_lock); +} Having to implement this in _every_ ALS driver sounds wrong and painful. If uniqueness of any kind must be provided, it should be handled by the ALS core and not by individual drivers, otherwise you can be certain that at least one driver will get it wrong someday. I agree. The reason this originally occurred is that the acpi layer apparently doesn't allow for simultaneous probing of multiple drivers and hence can get away with a simple counter and no locking. I'd imaging that als-class devices are simply named als%u. Just like hwmon devices are named hwmon%u, input devices are names input%u and event%u, etc. I don't know of any class pushing the naming down to its drivers, do you? The only example I can remember are i2c drivers back in year 1999, when they were part of lm-sensors.I have personally put an end to this years ago. This debate started in the als thread. Personally I couldn't care less either way but it does need to be put to bed before that subsystem merges. To my mind either approach is trivially handled in a userspace library so it doesn't matter. I don't suppose you can remember what the original reasons for squashing this naming approach were? Code duplication. Having the same unique-id handling code repeated in 50 drivers was no fun, as it did not add any value compared to a central handling. Counter argument placed (cc'd Pavel and Corentin for this point) is that having a generic name, e.g. hwmon0 and a name field in sysfs is superfluous when we can combine the two. @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev, return ret; } -static DEVICE_ATTR(lux1_input, S_IRUGO, +static DEVICE_ATTR(illuminance, S_IRUGO, tsl2550_show_lux1_input, NULL); Question: if I have a light sensing chip with two sensors, how are we going to handle it? Will we register 2 als class devices? The initial naming convention had the advantage that you could have more than one sensor per device, but I don't know if it is desirable in practice. This only becomes and issue if we have two sensors measuring illuminance (or approximation of it). The only two sensor chips I know of have one broadband and one in the infrared tsl2561 and I think the isl chip with a driver currently in misc. The combination of these two are needed to calculate illuminance. Some of the original discussion went into how to support separate access to the individual sensors. Decision was to leave that question until it becomes relevant. Basically we would put the current drivers in just supporting illuminance and see if anyone asked for furthers support. One tricky aspect is what the units should be for particular frequency ranges. At least illuminance is cleanly defined (even if chips are only fairly coarsely approximating it. Hmm