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 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 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 > > 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 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 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 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
>
> Acked-by: Rob Herring
>
> 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 Tue, 2014-01-14 at 14:41 +, Mark Brown wrote: > On Tue, Jan 14, 2014 at 04:00:17PM +0800, Zhang Rui wrote: > > 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. > > > 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). > > OK that makes sense, but what does this have to do with the _HID and > _CID methods? If an ACPI enumerated SPI device has a _HID and a _CID, both of them need to be exposed in 'uevent', so that a driver that matches _CID can also have a chance to be probed. This can not be done by the current code, thus we need special handling of ACPI enumerated SPI devices. > Surely we're just replacing spi: with acpi: in the uevent? yes. 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/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
Reviewed-by: Mika Westerberg
---
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
[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
---
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
---
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 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
---
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
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 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
---
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 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
---
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
---
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 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
Reviewed-by: Mika Westerberg
---
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
[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
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
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: [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
>
> Makes sense. There is one comment below but other than that, you can add
> my,
>
> Reviewed-by: Mika Westerberg
>
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
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
> Cc: Mika Westerberg
> ---
> 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
[PATCH 3/3] I2C: fix module autoloading for ACPI enumerated devices
Signed-off-by: Zhang Rui
---
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
[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
---
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 2/3] Platform: fix module autoloading for ACPI enumerated devices
Signed-off-by: Zhang Rui --- 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
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 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 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
> > 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
> > ---
> > 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
> > #include
> > #include
> > +#include
> >
> > #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 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 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 wrote: > > > >From 6077a62f2865201ab6727ca7d628ee5e43aa57e1 Mon Sep 17 00:00:00 > > >2001 > > From: Zhang Rui > > 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
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
> > 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
> > ---
> > 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
[RFC PATCH 6/6] Introduce INT33B1 I2C controller driver
>From 817d814ecae91862f42a0447f455dae7f74cba27 Mon Sep 17 00:00:00 2001
From: Zhang Rui
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
---
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
+ *
+ * ~~
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+
+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 pl
[RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver
>From 6077a62f2865201ab6727ca7d628ee5e43aa57e1 Mon Sep 17 00:00:00 2001
From: Zhang Rui
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
---
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
+ *
+ * ~~
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define PREFIX "ACPI: "
+
+#define _COMPONENT ACPI_SPB_COMPONENT
+ACPI_MODULE_NAME("i2c_root");
+#define ACPI_I2C_ROOT_CLASS"i2c_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;
+
+ if (acpi_bus_get_device(handle, &device))
+ return AE_OK;
+
+ status = acpi_get_current_resources
[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
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
---
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 3/6] ACPI: introduce acpi_get_generic_resources
>From 9a851d177794129a89f720c7122cb39fd163126b Mon Sep 17 00:00:00 2001
From: Zhang Rui
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
---
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
+ *
+ * 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
+#include
+#include
+
+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_
[RFC PATCH 2/6] Introduce ACPI style match in platform_match
>From 5d7ecd12c2994b8c5905d52718c2870c3b62746e Mon Sep 17 00:00:00 2001
From: Zhang Rui
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
---
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
#include
#include
+#include
#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
+ /* attempt ACPI style match */
+ if (acpi_match_device_id(&pdev->dev, id->name) == 0) {
+ pdev->id_entry = id;
+ return id;
+ }
+#endif
if (strcmp(pdev->name, id->name) == 0) {
pdev->id_entry = id;
return id;
--
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 1/6] Introduce acpi_match_device_id().
>From 72df5d1f51fb27a4ba7f70a3b07df759d32b8288 Mon Sep 17 00:00:00 2001
From: Zhang Rui
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
---
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 0/6] ACPI: ACPI 5.0 device enumeration proposal
Hi, all, I'm working on ACPI device enumeration support recently, and here is the proposal I made to enumerate devices via ACPI namespace. 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. Patch 1 & 2 Introduce ACPI style device match method in platform_match. this is because an ACPI device may have multiple hardware ID (_HID) and Compatible IDs, aka, _CID, but in platform device code, only pdev->name is used to match the driver id_table currently. Patch 3 introduces a new API acpi_get_generic_resources(). this API is used to convert ACPI style resources to the generic struct resource. Patch 4 changes i2c_register_board_info from __init to __devinit. This is needed because I want to enumerate the slave devices in ACPI I2C control Driver, which supports hotplug in theory. Patch 5 introduces the ACPI I2C controller enumeration drive. it enumerate the ACPI I2C controller to a platform device and then enumerate the I2C slaves as I2C_board_info. Patch 6 is an example driver for an ACPI enuemrated I2C controller. Note that this is just prototype patch set, which just passes build test. Because I'd like to get your ideas about this before going on. any comments are welcome. 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: 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 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
> >> > To: Jean Delvare , Ben Dooks ,
> >> > Wolfram
> >> > Sang , Len Brown ,
> >> > , ,
> >> >
> >> > CC: Jiri Kosina , Stéphane Chatty , JJ
> >> > Ding
> >> >
> >> >
> >> > 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
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
> > To: Jean Delvare , Ben Dooks ,
> > Wolfram
> > Sang , Len Brown ,
> > , ,
> >
> > CC: Jiri Kosina , Stéphane Chatty , JJ
> > Ding
> >
> >
> > 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 acp
Re: Fwd: Hid over I2C and ACPI interaction
t;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 not release it for some
reason.
Second, you need to write your own PNP I2C controller driver, to
enumerate the I2C controller via ACPI, AND enumerate the I2C slave
devices under this controller to I2C bus. I also have a similar 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
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
---
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
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.
> > Counte
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 furt
