Hi Wolfram,
As previously discussed I believe it should be okay for an I2C device
driver _not_ supply an I2C ID table to match to. The I2C subsystem
should be able to match via other means, such as via OF and/or ACPI
tables. The blocking factor during our previous conversation was
to keep registering via sysfs up and running. This set does that.
After thinking more deeply about the problem, it occurred to me that
any I2C device driver which issues an {of,acpi}_match)_device()
would also fail their probe(). Bolted on to this set is a new, more
generic way for these devices to match against any of the tables
which are present in the kernel today i.e. OF, ACPI and I2C.
NB: ACPI is not fully supported, but OF is.
I hope this ticks all of your boxes.
Kind regards,
Lee
drivers/i2c/i2c-core.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
drivers/of/device.c | 16 +++++++++++++++-
include/acpi/platform/aclinux.h | 5 +++--
include/linux/i2c.h | 19 +++++++++++++++++++
include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 135 insertions(+), 10 deletions(-)
Any drivers which support ACPI and Device Tree probing need to include
both respective header files. Without this patch, if a driver is being
used on a platform which does not support ACPI and subsequently does not
have the config option enabled, but includes linux/acpi.h the build
breaks with:
In file included from ../include/acpi/platform/acenv.h:150:0,
from ../include/acpi/acpi.h:56,
from ../include/linux/match.h:2,
from ../drivers/i2c/i2c-core.c:43:
../include/acpi/platform/aclinux.h:73:23:
fatal error: asm/acenv.h: No such file or directory
#include <asm/acenv.h>
^
Cc: Lv Zheng <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
include/acpi/platform/aclinux.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
index cd1f052..fdf7663 100644
--- a/include/acpi/platform/aclinux.h
+++ b/include/acpi/platform/aclinux.h
@@ -70,9 +70,10 @@
#ifdef EXPORT_ACPI_INTERFACES
#include <linux/export.h>
#endif
-#include <asm/acenv.h>
-#ifndef CONFIG_ACPI
+#ifdef CONFIG_ACPI
+#include <asm/acenv.h>
+#else
/* External globals for __KERNEL__, stubs is needed */
--
1.8.3.2
A great deal of I2C devices are currently matched via DT node name, and
as such the compatible naming convention of '<vendor>,<device>' has gone
somewhat awry - some nodes don't supply one, some supply an arbitrary
string and others the correct device name with an arbitrary vendor prefix.
In an effort to correct this problem we have to supply a mechanism to
match a device by compatible string AND by simple device name. This
function strips off the '<vendor>,' part of a supplied compatible string
and attempts to match without it.
The plan is to remove this function once all of the compatible strings
for each device have been brought into line.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/i2c/i2c-core.c | 25 +++++++++++++++++++++++++
include/linux/i2c.h | 10 ++++++++++
2 files changed, 35 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d3802dc..7dcd5c3 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1090,6 +1090,31 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
return i2c_verify_adapter(dev);
}
EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
+
+const struct of_device_id
+*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
+ struct device *dev)
+{
+ const struct i2c_client *client = i2c_verify_client(dev);
+ const char *name;
+
+ if (!(client && matches))
+ return NULL;
+
+ for (; matches->compatible[0]; matches++) {
+ name = strchr(matches->compatible, ',');
+ if (!name)
+ name = matches->compatible;
+ else
+ name++;
+
+ if (!strncmp(client->name, name, strlen(client->name)))
+ return matches;
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(i2c_of_match_device_strip_vendor);
#else
static void of_i2c_register_devices(struct i2c_adapter *adap) { }
#endif /* CONFIG_OF */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..f7ec75b 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -564,6 +564,9 @@ extern struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
/* must call put_device() when done with returned i2c_adapter device */
extern struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node);
+extern const struct of_device_id
+*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
+ struct device *dev);
#else
static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
@@ -575,6 +578,13 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
{
return NULL;
}
+
+const struct of_device_id
+*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
+ struct device *dev)
+{
+ return NULL;
+}
#endif /* CONFIG_OF */
#endif /* _LINUX_I2C_H */
--
1.8.3.2
Currently this is a helper function for the I2C subsystem to aid the
matching of non-standard compatible strings and devices which use DT
and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
it has been made more generic as it can be used to only make one call
for drivers which support any mixture of OF, ACPI and/or I2C matching.
The initial aim is for of_match_device() to be replaced by this call
in all I2C device drivers.
[1] Documentation/i2c/instantiating-devices
Signed-off-by: Lee Jones <[email protected]>
---
include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
create mode 100644 include/linux/match.h
diff --git a/include/linux/match.h b/include/linux/match.h
new file mode 100644
index 0000000..20a08e2
--- /dev/null
+++ b/include/linux/match.h
@@ -0,0 +1,40 @@
+#include <linux/of.h>
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+
+static void *device_match(struct device *dev)
+{
+ struct device_driver *driver = dev->driver;
+
+ if (!driver)
+ return NULL;
+
+ /* Attempt an OF style match */
+ if (IS_ENABLED(CONFIG_OF)) {
+ const struct of_device_id *of_match =
+ i2c_of_match_device(driver->of_match_table, dev);
+ if (of_match)
+ return (void *)of_match;
+ }
+
+ /* Then ACPI style match */
+ if (IS_ENABLED(CONFIG_ACPI)) {
+ const struct acpi_device_id *acpi_match =
+ acpi_match_device(driver->acpi_match_table, dev);
+ if (acpi_match)
+ return (void *)acpi_match;
+ }
+
+ /* Finally an I2C match */
+ if (IS_ENABLED(CONFIG_I2C)) {
+ struct i2c_client *client = i2c_verify_client(dev);
+ struct i2c_driver *i2c_drv = to_i2c_driver(driver);
+ struct i2c_device_id *i2c_match;
+
+ i2c_match = i2c_match_id(i2c_drv->id_table, client);
+ if (i2c_match)
+ return (void *)i2c_match;
+ }
+
+ return NULL;
+}
--
1.8.3.2
Currently the I2C framework insists on devices supplying an I2C ID
table. Many of the devices which do so unnecessarily adding quite a
few wasted lines to kernel code. This patch allows drivers a means
to 'not' supply the aforementioned table and match on either DT
and/or ACPI match tables instead.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/i2c/i2c-core.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e8c2cba..71970da 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -100,7 +100,7 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
/* Attempt an OF style match */
- if (of_driver_match_device(dev, drv))
+ if (i2c_of_match_device(drv->of_match_table, dev))
return 1;
/* Then ACPI style match */
@@ -268,7 +268,18 @@ static int i2c_device_probe(struct device *dev)
return 0;
driver = to_i2c_driver(dev->driver);
- if (!driver->probe || !driver->id_table)
+ if (!driver->probe)
+ return -EINVAL;
+
+ /*
+ * An I2C ID table is not mandatory, if and only if, a suitable Device
+ * Tree and/or ACPI match table entry is supplied for the probing
+ * device.
+ *
+ * TODO: Provide 'device type' to 'ACPI node' call and match here.
+ */
+ if (!driver->id_table &&
+ !i2c_of_match_device(dev->driver->of_match_table, dev))
return -ENODEV;
if (!device_can_wakeup(&client->dev))
--
1.8.3.2
The I2C framework supplies a means for devices to be registered without
the requirement for platform_data, DT or ACPI. The current solution is
that every single I2C device in the kernel is forced to supply a
normally empty/sparse I2C ID table so the I2C subsystem can match to.
In an effort to a) rid the kernel of these tables and b) generify all
(OF, ACPI and I2C) matching we need to provide some temporary work
arounds until we can straighten out the blocking factors.
This change is meant to be temporary and will be stripped out once
we've converted all I2C device drivers from of_match_device() over
to the new generic match_device().
Signed-off-by: Lee Jones <[email protected]>
---
drivers/of/device.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index dafb973..28913bf 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -1,6 +1,7 @@
#include <linux/string.h>
#include <linux/kernel.h>
#include <linux/of.h>
+#include <linux/i2c.h>
#include <linux/of_device.h>
#include <linux/init.h>
#include <linux/module.h>
@@ -21,9 +22,22 @@
const struct of_device_id *of_match_device(const struct of_device_id *matches,
const struct device *dev)
{
+ const struct of_device_id *match;
+
if ((!matches) || (!dev->of_node))
return NULL;
- return of_match_node(matches, dev->of_node);
+
+ match = of_match_node(matches, dev->of_node);
+ if (match)
+ return match;
+
+ /*
+ * Low impact workaround, until we can convert all I2C compatible
+ * strings over to the correctly specified <vendor>,<device> format.
+ */
+ match = i2c_of_match_device_strip_vendor(matches, (struct device *)dev);
+
+ return match;
}
EXPORT_SYMBOL(of_match_device);
--
1.8.3.2
This function provides a single call for all I2C devices which need to
match firstly using traditional OF means i.e by of_node, then if that
fails we attempt to match using the supplied I2C client name with a
list of supplied compatible strings with the '<vendor>,' string
removed. The latter is required due to the unruly naming conventions
used currently by I2C devices.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/i2c/i2c-core.c | 13 +++++++++++++
include/linux/i2c.h | 9 +++++++++
2 files changed, 22 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7dcd5c3..e8c2cba 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1115,6 +1115,19 @@ const struct of_device_id
return NULL;
}
EXPORT_SYMBOL_GPL(i2c_of_match_device_strip_vendor);
+
+const struct of_device_id
+*i2c_of_match_device(const struct of_device_id *matches, struct device *dev)
+{
+ const struct of_device_id *match;
+
+ match = of_match_device(matches, dev);
+ if (match)
+ return match;
+
+ return i2c_of_match_device_strip_vendor(matches, dev);
+}
+EXPORT_SYMBOL_GPL(i2c_of_match_device);
#else
static void of_i2c_register_devices(struct i2c_adapter *adap) { }
#endif /* CONFIG_OF */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f7ec75b..ab5866f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -565,6 +565,9 @@ extern struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
extern struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node);
extern const struct of_device_id
+*i2c_of_match_device(const struct of_device_id *matches, struct device *dev);
+
+extern const struct of_device_id
*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
struct device *dev);
#else
@@ -580,6 +583,12 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
}
const struct of_device_id
+*i2c_of_match_device(const struct of_device_id *matches, struct device *dev)
+{
+ return NULL;
+}
+
+const struct of_device_id
*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
struct device *dev)
{
--
1.8.3.2
Here we're providing dereference protection for i2c_match_id(), which
saves us having to do it each time it's called. We're also stripping
out the (now) needless check in i2c_device_match(). This patch paves
the way for other, similar code trimming.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/i2c/i2c-core.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7c7f4b8..d3802dc 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -82,6 +82,9 @@ void i2c_transfer_trace_unreg(void)
static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
const struct i2c_client *client)
{
+ if (!(id && client))
+ return NULL;
+
while (id->name[0]) {
if (strcmp(client->name, id->name) == 0)
return id;
@@ -95,8 +98,6 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
struct i2c_client *client = i2c_verify_client(dev);
struct i2c_driver *driver;
- if (!client)
- return 0;
/* Attempt an OF style match */
if (of_driver_match_device(dev, drv))
@@ -107,9 +108,10 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
return 1;
driver = to_i2c_driver(drv);
- /* match on an id table if there is one */
- if (driver->id_table)
- return i2c_match_id(driver->id_table, client) != NULL;
+
+ /* Finally an I2C match */
+ if (i2c_match_id(driver->id_table, client))
+ return 1;
return 0;
}
--
1.8.3.2
On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
> Any drivers which support ACPI and Device Tree probing need to include
> both respective header files. Without this patch, if a driver is being
> used on a platform which does not support ACPI and subsequently does not
> have the config option enabled, but includes linux/acpi.h the build
> breaks with:
>
> In file included from ../include/acpi/platform/acenv.h:150:0,
> from ../include/acpi/acpi.h:56,
> from ../include/linux/match.h:2,
> from ../drivers/i2c/i2c-core.c:43:
> ../include/acpi/platform/aclinux.h:73:23:
> fatal error: asm/acenv.h: No such file or directory
> #include <asm/acenv.h>
> ^
Which kernel does this happen with?
> Cc: Lv Zheng <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>
> ---
> include/acpi/platform/aclinux.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> index cd1f052..fdf7663 100644
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -70,9 +70,10 @@
> #ifdef EXPORT_ACPI_INTERFACES
> #include <linux/export.h>
> #endif
> -#include <asm/acenv.h>
>
> -#ifndef CONFIG_ACPI
> +#ifdef CONFIG_ACPI
> +#include <asm/acenv.h>
> +#else
>
> /* External globals for __KERNEL__, stubs is needed */
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> Currently this is a helper function for the I2C subsystem to aid the
> matching of non-standard compatible strings and devices which use DT
> and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
> it has been made more generic as it can be used to only make one call
> for drivers which support any mixture of OF, ACPI and/or I2C matching.
>
> The initial aim is for of_match_device() to be replaced by this call
> in all I2C device drivers.
>
> [1] Documentation/i2c/instantiating-devices
>
> Signed-off-by: Lee Jones <[email protected]>
Mika, can you please have a look at this, please?
> ---
> include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
> create mode 100644 include/linux/match.h
>
> diff --git a/include/linux/match.h b/include/linux/match.h
> new file mode 100644
> index 0000000..20a08e2
> --- /dev/null
> +++ b/include/linux/match.h
> @@ -0,0 +1,40 @@
> +#include <linux/of.h>
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +
> +static void *device_match(struct device *dev)
> +{
> + struct device_driver *driver = dev->driver;
> +
> + if (!driver)
> + return NULL;
> +
> + /* Attempt an OF style match */
> + if (IS_ENABLED(CONFIG_OF)) {
> + const struct of_device_id *of_match =
> + i2c_of_match_device(driver->of_match_table, dev);
> + if (of_match)
> + return (void *)of_match;
> + }
> +
> + /* Then ACPI style match */
> + if (IS_ENABLED(CONFIG_ACPI)) {
> + const struct acpi_device_id *acpi_match =
> + acpi_match_device(driver->acpi_match_table, dev);
> + if (acpi_match)
> + return (void *)acpi_match;
> + }
> +
> + /* Finally an I2C match */
> + if (IS_ENABLED(CONFIG_I2C)) {
> + struct i2c_client *client = i2c_verify_client(dev);
> + struct i2c_driver *i2c_drv = to_i2c_driver(driver);
> + struct i2c_device_id *i2c_match;
> +
> + i2c_match = i2c_match_id(i2c_drv->id_table, client);
> + if (i2c_match)
> + return (void *)i2c_match;
> + }
> +
> + return NULL;
> +}
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, Jun 04, 2014 at 02:37:42PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> > Currently this is a helper function for the I2C subsystem to aid the
> > matching of non-standard compatible strings and devices which use DT
> > and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
> > it has been made more generic as it can be used to only make one call
> > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> >
> > The initial aim is for of_match_device() to be replaced by this call
> > in all I2C device drivers.
> >
> > [1] Documentation/i2c/instantiating-devices
> >
> > Signed-off-by: Lee Jones <[email protected]>
>
> Mika, can you please have a look at this, please?
I don't see any fundamental problems with this wrt. ACPI.
That said, I find it kind of weird to have generic function that then
has knowledge of how different buses do their matching.
I would rather see something like firmware_device_match(dev) that goes
and matches from DT/ACPI and leave bus specific match to happen internal
to that bus.
>
>
> > ---
> > include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> > create mode 100644 include/linux/match.h
> >
> > diff --git a/include/linux/match.h b/include/linux/match.h
> > new file mode 100644
> > index 0000000..20a08e2
> > --- /dev/null
> > +++ b/include/linux/match.h
> > @@ -0,0 +1,40 @@
> > +#include <linux/of.h>
> > +#include <linux/acpi.h>
> > +#include <linux/i2c.h>
> > +
> > +static void *device_match(struct device *dev)
> > +{
> > + struct device_driver *driver = dev->driver;
> > +
> > + if (!driver)
> > + return NULL;
> > +
> > + /* Attempt an OF style match */
> > + if (IS_ENABLED(CONFIG_OF)) {
> > + const struct of_device_id *of_match =
> > + i2c_of_match_device(driver->of_match_table, dev);
> > + if (of_match)
> > + return (void *)of_match;
> > + }
> > +
> > + /* Then ACPI style match */
> > + if (IS_ENABLED(CONFIG_ACPI)) {
> > + const struct acpi_device_id *acpi_match =
> > + acpi_match_device(driver->acpi_match_table, dev);
> > + if (acpi_match)
> > + return (void *)acpi_match;
> > + }
> > +
> > + /* Finally an I2C match */
> > + if (IS_ENABLED(CONFIG_I2C)) {
> > + struct i2c_client *client = i2c_verify_client(dev);
> > + struct i2c_driver *i2c_drv = to_i2c_driver(driver);
> > + struct i2c_device_id *i2c_match;
> > +
> > + i2c_match = i2c_match_id(i2c_drv->id_table, client);
> > + if (i2c_match)
> > + return (void *)i2c_match;
> > + }
> > +
> > + return NULL;
> > +}
> >
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:
> On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
> > Any drivers which support ACPI and Device Tree probing need to include
> > both respective header files. Without this patch, if a driver is being
> > used on a platform which does not support ACPI and subsequently does not
> > have the config option enabled, but includes linux/acpi.h the build
> > breaks with:
> >
> > In file included from ../include/acpi/platform/acenv.h:150:0,
> > from ../include/acpi/acpi.h:56,
> > from ../include/linux/match.h:2,
> > from ../drivers/i2c/i2c-core.c:43:
> > ../include/acpi/platform/aclinux.h:73:23:
> > fatal error: asm/acenv.h: No such file or directory
> > #include <asm/acenv.h>
> > ^
>
> Which kernel does this happen with?
a0a962d (tag: refs/tags/next-20140602, refs/remotes/next/master)
Add linux-next specific files for 20140602
> > Cc: Lv Zheng <[email protected]>
> > Cc: Rafael J. Wysocki <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > include/acpi/platform/aclinux.h | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> > index cd1f052..fdf7663 100644
> > --- a/include/acpi/platform/aclinux.h
> > +++ b/include/acpi/platform/aclinux.h
> > @@ -70,9 +70,10 @@
> > #ifdef EXPORT_ACPI_INTERFACES
> > #include <linux/export.h>
> > #endif
> > -#include <asm/acenv.h>
> >
> > -#ifndef CONFIG_ACPI
> > +#ifdef CONFIG_ACPI
> > +#include <asm/acenv.h>
> > +#else
> >
> > /* External globals for __KERNEL__, stubs is needed */
> >
> >
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 04 Jun 2014, Mika Westerberg wrote:
> On Wed, Jun 04, 2014 at 02:37:42PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> > > Currently this is a helper function for the I2C subsystem to aid the
> > > matching of non-standard compatible strings and devices which use DT
> > > and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
> > > it has been made more generic as it can be used to only make one call
> > > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> > >
> > > The initial aim is for of_match_device() to be replaced by this call
> > > in all I2C device drivers.
> > >
> > > [1] Documentation/i2c/instantiating-devices
> > >
> > > Signed-off-by: Lee Jones <[email protected]>
> >
> > Mika, can you please have a look at this, please?
>
> I don't see any fundamental problems with this wrt. ACPI.
>
> That said, I find it kind of weird to have generic function that then
> has knowledge of how different buses do their matching.
>
> I would rather see something like firmware_device_match(dev) that goes
> and matches from DT/ACPI and leave bus specific match to happen internal
> to that bus.
Unfortunately that completely defeats the object of the patch. When a
of_match_device() is invoked it solely looks up devices based on OF
matching, but I2C is special in that devices can be registered via
sysfs, thus will no have device node. If of_match_device() is called
in one of these instances it will fail. The idea of this patch is to
generify the matching into something does has the knowledge to firstly
attempt a traditional match, and if that fails will fall back to a
special i2c_{of,acpi}_match_device() which knows how to deal with
node-less registration.
We don't support that for ACPI yet, as I don't have a system to test
it on, but when we do acpi_match_device() in the patch will too be
swapped out for an equivalent i2c_acpi_match_device().
Actually, I've just spotted that this patch is wrong I need to change
it in the following way:
> > > ---
> > > include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 40 insertions(+)
> > > create mode 100644 include/linux/match.h
> > >
> > > diff --git a/include/linux/match.h b/include/linux/match.h
> > > new file mode 100644
> > > index 0000000..20a08e2
> > > --- /dev/null
> > > +++ b/include/linux/match.h
> > > @@ -0,0 +1,40 @@
> > > +#include <linux/of.h>
> > > +#include <linux/acpi.h>
> > > +#include <linux/i2c.h>
> > > +
> > > +static void *device_match(struct device *dev)
> > > +{
> > > + struct device_driver *driver = dev->driver;
> > > +
> > > + if (!driver)
> > > + return NULL;
> > > +
> > > + /* Attempt an OF style match */
> > > + if (IS_ENABLED(CONFIG_OF)) {
> > > + const struct of_device_id *of_match =
> > > + i2c_of_match_device(driver->of_match_table, dev);
This should be of_match_device()
> > > + if (of_match)
> > > + return (void *)of_match;
> > > + }
> > > +
> > > + /* Then ACPI style match */
> > > + if (IS_ENABLED(CONFIG_ACPI)) {
> > > + const struct acpi_device_id *acpi_match =
> > > + acpi_match_device(driver->acpi_match_table, dev);
> > > + if (acpi_match)
> > > + return (void *)acpi_match;
> > > + }
> > > +
> > > + /* Finally an I2C match */
> > > + if (IS_ENABLED(CONFIG_I2C)) {
> > > + struct i2c_client *client = i2c_verify_client(dev);
> > > + struct i2c_driver *i2c_drv = to_i2c_driver(driver);
> > > + struct i2c_device_id *i2c_match;
i2c_of_match_device() and later i2c_acpi_match_device() should be here.
> > > + i2c_match = i2c_match_id(i2c_drv->id_table, client);
> > > + if (i2c_match)
> > > + return (void *)i2c_match;
> > > + }
> > > +
> > > + return NULL;
> > > +}
> > >
> >
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Jun 4, 2014 at 7:09 AM, Lee Jones <[email protected]> wrote:
> A great deal of I2C devices are currently matched via DT node name, and
> as such the compatible naming convention of '<vendor>,<device>' has gone
> somewhat awry - some nodes don't supply one, some supply an arbitrary
> string and others the correct device name with an arbitrary vendor prefix.
>
> In an effort to correct this problem we have to supply a mechanism to
> match a device by compatible string AND by simple device name. This
> function strips off the '<vendor>,' part of a supplied compatible string
> and attempts to match without it.
>
> The plan is to remove this function once all of the compatible strings
> for each device have been brought into line.
Then should you add a warning for cases needing to be fixed?
Rob
>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/i2c/i2c-core.c | 25 +++++++++++++++++++++++++
> include/linux/i2c.h | 10 ++++++++++
> 2 files changed, 35 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index d3802dc..7dcd5c3 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1090,6 +1090,31 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> return i2c_verify_adapter(dev);
> }
> EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
> +
> +const struct of_device_id
> +*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
> + struct device *dev)
> +{
> + const struct i2c_client *client = i2c_verify_client(dev);
> + const char *name;
> +
> + if (!(client && matches))
> + return NULL;
> +
> + for (; matches->compatible[0]; matches++) {
> + name = strchr(matches->compatible, ',');
> + if (!name)
> + name = matches->compatible;
> + else
> + name++;
> +
> + if (!strncmp(client->name, name, strlen(client->name)))
> + return matches;
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(i2c_of_match_device_strip_vendor);
> #else
> static void of_i2c_register_devices(struct i2c_adapter *adap) { }
> #endif /* CONFIG_OF */
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index b556e0a..f7ec75b 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -564,6 +564,9 @@ extern struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
> /* must call put_device() when done with returned i2c_adapter device */
> extern struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node);
>
> +extern const struct of_device_id
> +*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
> + struct device *dev);
> #else
>
> static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
> @@ -575,6 +578,13 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
> {
> return NULL;
> }
> +
> +const struct of_device_id
> +*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
> + struct device *dev)
> +{
> + return NULL;
> +}
> #endif /* CONFIG_OF */
>
> #endif /* _LINUX_I2C_H */
> --
> 1.8.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 04 Jun 2014, Rob Herring wrote:
> On Wed, Jun 4, 2014 at 7:09 AM, Lee Jones <[email protected]> wrote:
> > A great deal of I2C devices are currently matched via DT node name, and
> > as such the compatible naming convention of '<vendor>,<device>' has gone
> > somewhat awry - some nodes don't supply one, some supply an arbitrary
> > string and others the correct device name with an arbitrary vendor prefix.
> >
> > In an effort to correct this problem we have to supply a mechanism to
> > match a device by compatible string AND by simple device name. This
> > function strips off the '<vendor>,' part of a supplied compatible string
> > and attempts to match without it.
> >
> > The plan is to remove this function once all of the compatible strings
> > for each device have been brought into line.
>
> Then should you add a warning for cases needing to be fixed?
I was just going to go and fix them all, but I could do that as well?
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > drivers/i2c/i2c-core.c | 25 +++++++++++++++++++++++++
> > include/linux/i2c.h | 10 ++++++++++
> > 2 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index d3802dc..7dcd5c3 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -1090,6 +1090,31 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> > return i2c_verify_adapter(dev);
> > }
> > EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
> > +
> > +const struct of_device_id
> > +*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
> > + struct device *dev)
> > +{
> > + const struct i2c_client *client = i2c_verify_client(dev);
> > + const char *name;
> > +
> > + if (!(client && matches))
> > + return NULL;
> > +
> > + for (; matches->compatible[0]; matches++) {
> > + name = strchr(matches->compatible, ',');
> > + if (!name)
> > + name = matches->compatible;
> > + else
> > + name++;
> > +
> > + if (!strncmp(client->name, name, strlen(client->name)))
> > + return matches;
> > + }
> > +
> > + return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(i2c_of_match_device_strip_vendor);
> > #else
> > static void of_i2c_register_devices(struct i2c_adapter *adap) { }
> > #endif /* CONFIG_OF */
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index b556e0a..f7ec75b 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -564,6 +564,9 @@ extern struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
> > /* must call put_device() when done with returned i2c_adapter device */
> > extern struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node);
> >
> > +extern const struct of_device_id
> > +*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
> > + struct device *dev);
> > #else
> >
> > static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
> > @@ -575,6 +578,13 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
> > {
> > return NULL;
> > }
> > +
> > +const struct of_device_id
> > +*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
> > + struct device *dev)
> > +{
> > + return NULL;
> > +}
> > #endif /* CONFIG_OF */
> >
> > #endif /* _LINUX_I2C_H */
> >
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wednesday, June 04, 2014 01:51:37 PM Lee Jones wrote:
> On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:
>
> > On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
> > > Any drivers which support ACPI and Device Tree probing need to include
> > > both respective header files. Without this patch, if a driver is being
> > > used on a platform which does not support ACPI and subsequently does not
> > > have the config option enabled, but includes linux/acpi.h the build
> > > breaks with:
> > >
> > > In file included from ../include/acpi/platform/acenv.h:150:0,
> > > from ../include/acpi/acpi.h:56,
> > > from ../include/linux/match.h:2,
> > > from ../drivers/i2c/i2c-core.c:43:
> > > ../include/acpi/platform/aclinux.h:73:23:
> > > fatal error: asm/acenv.h: No such file or directory
> > > #include <asm/acenv.h>
> > > ^
> >
> > Which kernel does this happen with?
>
> a0a962d (tag: refs/tags/next-20140602, refs/remotes/next/master)
> Add linux-next specific files for 20140602
It looks like the problem is with include/linux/match.h that should not
include acpi/acpi.h directly.
But I can't find this file in the Linus' next branch even, so I guess it's
on its way to that branch?
Rafael
Hi, Lee
> From: Lee Jones [mailto:[email protected]]
> Sent: Wednesday, June 04, 2014 8:10 PM
>
> Any drivers which support ACPI and Device Tree probing need to include
> both respective header files. Without this patch, if a driver is being
> used on a platform which does not support ACPI and subsequently does not
> have the config option enabled, but includes linux/acpi.h the build
> breaks with:
>
> In file included from ../include/acpi/platform/acenv.h:150:0,
> from ../include/acpi/acpi.h:56,
> from ../include/linux/match.h:2,
> from ../drivers/i2c/i2c-core.c:43:
> ../include/acpi/platform/aclinux.h:73:23:
> fatal error: asm/acenv.h: No such file or directory
> #include <asm/acenv.h>
> ^
> Cc: Lv Zheng <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>
> ---
> include/acpi/platform/aclinux.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> index cd1f052..fdf7663 100644
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -70,9 +70,10 @@
> #ifdef EXPORT_ACPI_INTERFACES
> #include <linux/export.h>
> #endif
> -#include <asm/acenv.h>
>
> -#ifndef CONFIG_ACPI
> +#ifdef CONFIG_ACPI
> +#include <asm/acenv.h>
> +#else
This is exactly what I want to do in the next step.
But you are a bit faster here.
I believe:
The miss-ordered inclusions of <asm/acpi.h> is the culprit of all of the miss-ordered inclusions in arch/x86/include/asm.
You should have noted that <asm/acpi.h> was originally unexpected included by some x86 specific headers.
Simply doing <asm/acenv.h> exlusion in this way might be able to fix your issue for your architecture, but it could be very likely breaking x86 builds.
You might be able to find another way to solve your build issue - for example, creating an empty <asm/acenv.h> for arch/arm.
Thanks and best regards
-Lv
>
> /* External globals for __KERNEL__, stubs is needed */
>
> --
> 1.8.3.2
Hi,
> From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
> Sent: Thursday, June 05, 2014 5:30 AM
>
> On Wednesday, June 04, 2014 01:51:37 PM Lee Jones wrote:
> > On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:
> >
> > > On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
> > > > Any drivers which support ACPI and Device Tree probing need to include
> > > > both respective header files. Without this patch, if a driver is being
> > > > used on a platform which does not support ACPI and subsequently does not
> > > > have the config option enabled, but includes linux/acpi.h the build
> > > > breaks with:
> > > >
> > > > In file included from ../include/acpi/platform/acenv.h:150:0,
> > > > from ../include/acpi/acpi.h:56,
> > > > from ../include/linux/match.h:2,
> > > > from ../drivers/i2c/i2c-core.c:43:
> > > > ../include/acpi/platform/aclinux.h:73:23:
> > > > fatal error: asm/acenv.h: No such file or directory
> > > > #include <asm/acenv.h>
> > > > ^
> > >
> > > Which kernel does this happen with?
> >
> > a0a962d (tag: refs/tags/next-20140602, refs/remotes/next/master)
> > Add linux-next specific files for 20140602
>
> It looks like the problem is with include/linux/match.h that should not
> include acpi/acpi.h directly.
This is another example that many mis-ordered inclusions are caused by the mis-ordered <asm/acpi.h> inclusion.
>
> But I can't find this file in the Linus' next branch even, so I guess it's
> on its way to that branch?
>
I guess,
In their tree, they have CONFIG_ACPI enabled for ARM, but we've changed to make:
1. <asm/acenv.h> the architecture specific layer for ACPICA, and
2. <asm/acpi.h> is now the architecture specific layer for Linux ACPI.
So they need to follow this.
Thanks and best regards
-Lv
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Hi, Lee
> From: Lee Jones [mailto:[email protected]]
> Sent: Wednesday, June 04, 2014 8:52 PM
> To: Rafael J. Wysocki
>
> On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:
>
> > On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
> > > Any drivers which support ACPI and Device Tree probing need to include
> > > both respective header files. Without this patch, if a driver is being
> > > used on a platform which does not support ACPI and subsequently does not
> > > have the config option enabled, but includes linux/acpi.h the build
> > > breaks with:
> > >
> > > In file included from ../include/acpi/platform/acenv.h:150:0,
> > > from ../include/acpi/acpi.h:56,
> > > from ../include/linux/match.h:2,
> > > from ../drivers/i2c/i2c-core.c:43:
> > > ../include/acpi/platform/aclinux.h:73:23:
> > > fatal error: asm/acenv.h: No such file or directory
> > > #include <asm/acenv.h>
> > > ^
Note that:
In our tree:
<asm/acenv.h> is only included by <acpi/acpi.h>.
And <acpi/acpi.h> is only included by
1. <linux/acpi.h> when CONFIG_ACPI enabled
2. <linux/sfi_acpi.h> - this is x86 specific, we'll clean it up by implementing stubs for all ACPI external interfaces.
So there is no case we need to exclude <asm/acenv.h> when CONFIG_ACPI is not enabled.
I cannot find linux/match.h here.
If <linux/match.h> want to include ACPI features, it shouldn't include <acpi/acpi.h>, but should include <linux/acpi.h>.
Please refer to:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8b48463f
And stop including <acpi/acpi.h> directly in any cases.
Thanks and best regards
-Lv
> >
> > Which kernel does this happen with?
>
> a0a962d (tag: refs/tags/next-20140602, refs/remotes/next/master)
> Add linux-next specific files for 20140602
>
> > > Cc: Lv Zheng <[email protected]>
> > > Cc: Rafael J. Wysocki <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Signed-off-by: Lee Jones <[email protected]>
> > > ---
> > > include/acpi/platform/aclinux.h | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> > > index cd1f052..fdf7663 100644
> > > --- a/include/acpi/platform/aclinux.h
> > > +++ b/include/acpi/platform/aclinux.h
> > > @@ -70,9 +70,10 @@
> > > #ifdef EXPORT_ACPI_INTERFACES
> > > #include <linux/export.h>
> > > #endif
> > > -#include <asm/acenv.h>
> > >
> > > -#ifndef CONFIG_ACPI
> > > +#ifdef CONFIG_ACPI
> > > +#include <asm/acenv.h>
> > > +#else
> > >
> > > /* External globals for __KERNEL__, stubs is needed */
> > >
> > >
> >
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Hi Lv,
On 2014-6-5 8:56, Zheng, Lv wrote:
> Hi, Lee
>
>> From: Lee Jones [mailto:[email protected]]
>> Sent: Wednesday, June 04, 2014 8:10 PM
>>
>> Any drivers which support ACPI and Device Tree probing need to include
>> both respective header files. Without this patch, if a driver is being
>> used on a platform which does not support ACPI and subsequently does not
>> have the config option enabled, but includes linux/acpi.h the build
>> breaks with:
>>
>> In file included from ../include/acpi/platform/acenv.h:150:0,
>> from ../include/acpi/acpi.h:56,
>> from ../include/linux/match.h:2,
>> from ../drivers/i2c/i2c-core.c:43:
>> ../include/acpi/platform/aclinux.h:73:23:
>> fatal error: asm/acenv.h: No such file or directory
>> #include <asm/acenv.h>
>> ^
>> Cc: Lv Zheng <[email protected]>
>> Cc: Rafael J. Wysocki <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Lee Jones <[email protected]>
>> ---
>> include/acpi/platform/aclinux.h | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
>> index cd1f052..fdf7663 100644
>> --- a/include/acpi/platform/aclinux.h
>> +++ b/include/acpi/platform/aclinux.h
>> @@ -70,9 +70,10 @@
>> #ifdef EXPORT_ACPI_INTERFACES
>> #include <linux/export.h>
>> #endif
>> -#include <asm/acenv.h>
>>
>> -#ifndef CONFIG_ACPI
>> +#ifdef CONFIG_ACPI
>> +#include <asm/acenv.h>
>> +#else
>
> This is exactly what I want to do in the next step.
> But you are a bit faster here.
> I believe:
> The miss-ordered inclusions of <asm/acpi.h> is the culprit of all of the miss-ordered inclusions in arch/x86/include/asm.
> You should have noted that <asm/acpi.h> was originally unexpected included by some x86 specific headers.
> Simply doing <asm/acenv.h> exlusion in this way might be able to fix your issue for your architecture, but it could be very likely breaking x86 builds.
> You might be able to find another way to solve your build issue - for example, creating an empty <asm/acenv.h> for arch/arm.
Yes, we solve this issue as you suggested for arch/arm64.
since ARM32 will not support ACPI in the near future,
we may find another way to fix it.
Thanks
Hanjun
On 2014-6-5 9:14, Zheng, Lv wrote:
> Hi, Lee
>
>> From: Lee Jones [mailto:[email protected]]
>> Sent: Wednesday, June 04, 2014 8:52 PM
>> To: Rafael J. Wysocki
>>
>> On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:
>>
>>> On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
>>>> Any drivers which support ACPI and Device Tree probing need to include
>>>> both respective header files. Without this patch, if a driver is being
>>>> used on a platform which does not support ACPI and subsequently does not
>>>> have the config option enabled, but includes linux/acpi.h the build
>>>> breaks with:
>>>>
>>>> In file included from ../include/acpi/platform/acenv.h:150:0,
>>>> from ../include/acpi/acpi.h:56,
>>>> from ../include/linux/match.h:2,
>>>> from ../drivers/i2c/i2c-core.c:43:
>>>> ../include/acpi/platform/aclinux.h:73:23:
>>>> fatal error: asm/acenv.h: No such file or directory
>>>> #include <asm/acenv.h>
>>>> ^
>
> Note that:
> In our tree:
> <asm/acenv.h> is only included by <acpi/acpi.h>.
> And <acpi/acpi.h> is only included by
> 1. <linux/acpi.h> when CONFIG_ACPI enabled
> 2. <linux/sfi_acpi.h> - this is x86 specific, we'll clean it up by implementing stubs for all ACPI external interfaces.
> So there is no case we need to exclude <asm/acenv.h> when CONFIG_ACPI is not enabled.
>
> I cannot find linux/match.h here.
> If <linux/match.h> want to include ACPI features, it shouldn't include <acpi/acpi.h>, but should include <linux/acpi.h>.
> Please refer to:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8b48463f
> And stop including <acpi/acpi.h> directly in any cases.
Ah, I agree, please ignore my previous email,
sorry for the noise.
Since it is very important to include <linux/acpi.h> but not <acpi/acpi.h>,
can we document it somewhere as the guidance? Then people will not
make such mistake :)
Thanks
Hanjun
Hi,
> From: Hanjun Guo [mailto:[email protected]]
> Sent: Thursday, June 05, 2014 12:11 PM
> To: Zheng, Lv; Lee Jones; Rafael J. Wysocki
>
> On 2014-6-5 9:14, Zheng, Lv wrote:
> > Hi, Lee
> >
> >> From: Lee Jones [mailto:[email protected]]
> >> Sent: Wednesday, June 04, 2014 8:52 PM
> >> To: Rafael J. Wysocki
> >>
> >> On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:
> >>
> >>> On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
> >>>> Any drivers which support ACPI and Device Tree probing need to include
> >>>> both respective header files. Without this patch, if a driver is being
> >>>> used on a platform which does not support ACPI and subsequently does not
> >>>> have the config option enabled, but includes linux/acpi.h the build
> >>>> breaks with:
> >>>>
> >>>> In file included from ../include/acpi/platform/acenv.h:150:0,
> >>>> from ../include/acpi/acpi.h:56,
> >>>> from ../include/linux/match.h:2,
> >>>> from ../drivers/i2c/i2c-core.c:43:
> >>>> ../include/acpi/platform/aclinux.h:73:23:
> >>>> fatal error: asm/acenv.h: No such file or directory
> >>>> #include <asm/acenv.h>
> >>>> ^
> >
> > Note that:
> > In our tree:
> > <asm/acenv.h> is only included by <acpi/acpi.h>.
> > And <acpi/acpi.h> is only included by
> > 1. <linux/acpi.h> when CONFIG_ACPI enabled
> > 2. <linux/sfi_acpi.h> - this is x86 specific, we'll clean it up by implementing stubs for all ACPI external interfaces.
> > So there is no case we need to exclude <asm/acenv.h> when CONFIG_ACPI is not enabled.
> >
> > I cannot find linux/match.h here.
> > If <linux/match.h> want to include ACPI features, it shouldn't include <acpi/acpi.h>, but should include <linux/acpi.h>.
> > Please refer to:
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8b48463f
> > And stop including <acpi/acpi.h> directly in any cases.
>
> Ah, I agree, please ignore my previous email,
> sorry for the noise.
>
> Since it is very important to include <linux/acpi.h> but not <acpi/acpi.h>,
> can we document it somewhere as the guidance? Then people will not
> make such mistake :)
After I test the ACPICA stubs and remove the only abnormal direct <acpi/acpi.h> inclusion from <linux/sfi_acpi.h>.
We could have a commit to add something like:
#ifndef _LINUX_ACPI_H
#error ....
#endif
To the <acpi/platform/aclinux.h> (the Linux OSL for ACPICA headers, it is included by <acpi/acpi.h>).
Hope this can be the hint to prevent future mistakes.
Thanks and best regards
-Lv
>
> Thanks
> Hanjun
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Jun 04, 2014 at 02:28:20PM +0100, Lee Jones wrote:
> On Wed, 04 Jun 2014, Mika Westerberg wrote:
> > On Wed, Jun 04, 2014 at 02:37:42PM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> > > > Currently this is a helper function for the I2C subsystem to aid the
> > > > matching of non-standard compatible strings and devices which use DT
> > > > and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
> > > > it has been made more generic as it can be used to only make one call
> > > > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> > > >
> > > > The initial aim is for of_match_device() to be replaced by this call
> > > > in all I2C device drivers.
> > > >
> > > > [1] Documentation/i2c/instantiating-devices
> > > >
> > > > Signed-off-by: Lee Jones <[email protected]>
> > >
> > > Mika, can you please have a look at this, please?
> >
> > I don't see any fundamental problems with this wrt. ACPI.
> >
> > That said, I find it kind of weird to have generic function that then
> > has knowledge of how different buses do their matching.
> >
> > I would rather see something like firmware_device_match(dev) that goes
> > and matches from DT/ACPI and leave bus specific match to happen internal
> > to that bus.
>
> Unfortunately that completely defeats the object of the patch. When a
> of_match_device() is invoked it solely looks up devices based on OF
> matching, but I2C is special in that devices can be registered via
> sysfs, thus will no have device node. If of_match_device() is called
> in one of these instances it will fail. The idea of this patch is to
> generify the matching into something does has the knowledge to firstly
> attempt a traditional match, and if that fails will fall back to a
> special i2c_{of,acpi}_match_device() which knows how to deal with
> node-less registration.
OK, then but since this is now I2C specific, why call it device_match()
instead of something like i2c_device_match()? Or do you have plans to
add there more knowledge about other buses like SPI and PCI to name few?
On Thu, 05 Jun 2014, Mika Westerberg wrote:
> On Wed, Jun 04, 2014 at 02:28:20PM +0100, Lee Jones wrote:
> > On Wed, 04 Jun 2014, Mika Westerberg wrote:
> > > On Wed, Jun 04, 2014 at 02:37:42PM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> > > > > Currently this is a helper function for the I2C subsystem to aid the
> > > > > matching of non-standard compatible strings and devices which use DT
> > > > > and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
> > > > > it has been made more generic as it can be used to only make one call
> > > > > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> > > > >
> > > > > The initial aim is for of_match_device() to be replaced by this call
> > > > > in all I2C device drivers.
> > > > >
> > > > > [1] Documentation/i2c/instantiating-devices
> > > > >
> > > > > Signed-off-by: Lee Jones <[email protected]>
> > > >
> > > > Mika, can you please have a look at this, please?
> > >
> > > I don't see any fundamental problems with this wrt. ACPI.
> > >
> > > That said, I find it kind of weird to have generic function that then
> > > has knowledge of how different buses do their matching.
> > >
> > > I would rather see something like firmware_device_match(dev) that goes
> > > and matches from DT/ACPI and leave bus specific match to happen internal
> > > to that bus.
> >
> > Unfortunately that completely defeats the object of the patch. When a
> > of_match_device() is invoked it solely looks up devices based on OF
> > matching, but I2C is special in that devices can be registered via
> > sysfs, thus will no have device node. If of_match_device() is called
> > in one of these instances it will fail. The idea of this patch is to
> > generify the matching into something does has the knowledge to firstly
> > attempt a traditional match, and if that fails will fall back to a
> > special i2c_{of,acpi}_match_device() which knows how to deal with
> > node-less registration.
>
> OK, then but since this is now I2C specific, why call it device_match()
> instead of something like i2c_device_match()? Or do you have plans to
So in an early incarnation of the patch I did just that, and it might
not actually be such a bad idea still - I'm open to other people's
opinions on this.
> add there more knowledge about other buses like SPI and PCI to name few?
... but yes, this is the new idea - that it can be expanded as required.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 4 Jun 2014 13:09:54 +0100, Lee Jones <[email protected]> wrote:
> Currently the I2C framework insists on devices supplying an I2C ID
> table. Many of the devices which do so unnecessarily adding quite a
> few wasted lines to kernel code. This patch allows drivers a means
> to 'not' supply the aforementioned table and match on either DT
> and/or ACPI match tables instead.
>
> Signed-off-by: Lee Jones <[email protected]>
Looks okay to me. I've not applied or tested in any way though.
Acked-by: Grant Likely <[email protected]>
> ---
> drivers/i2c/i2c-core.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index e8c2cba..71970da 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -100,7 +100,7 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
>
>
> /* Attempt an OF style match */
> - if (of_driver_match_device(dev, drv))
> + if (i2c_of_match_device(drv->of_match_table, dev))
> return 1;
>
> /* Then ACPI style match */
> @@ -268,7 +268,18 @@ static int i2c_device_probe(struct device *dev)
> return 0;
>
> driver = to_i2c_driver(dev->driver);
> - if (!driver->probe || !driver->id_table)
> + if (!driver->probe)
> + return -EINVAL;
> +
> + /*
> + * An I2C ID table is not mandatory, if and only if, a suitable Device
> + * Tree and/or ACPI match table entry is supplied for the probing
> + * device.
> + *
> + * TODO: Provide 'device type' to 'ACPI node' call and match here.
> + */
> + if (!driver->id_table &&
> + !i2c_of_match_device(dev->driver->of_match_table, dev))
> return -ENODEV;
>
> if (!device_can_wakeup(&client->dev))
> --
> 1.8.3.2
>
On Wed, 4 Jun 2014 13:09:56 +0100, Lee Jones <[email protected]> wrote:
> Currently this is a helper function for the I2C subsystem to aid the
> matching of non-standard compatible strings and devices which use DT
> and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
> it has been made more generic as it can be used to only make one call
> for drivers which support any mixture of OF, ACPI and/or I2C matching.
>
> The initial aim is for of_match_device() to be replaced by this call
> in all I2C device drivers.
>
> [1] Documentation/i2c/instantiating-devices
>
> Signed-off-by: Lee Jones <[email protected]>
I don't like this. It drops all type safety on the match entry
and the caller has no idea what it got back.
g.
> ---
> include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
> create mode 100644 include/linux/match.h
>
> diff --git a/include/linux/match.h b/include/linux/match.h
> new file mode 100644
> index 0000000..20a08e2
> --- /dev/null
> +++ b/include/linux/match.h
> @@ -0,0 +1,40 @@
> +#include <linux/of.h>
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +
> +static void *device_match(struct device *dev)
> +{
> + struct device_driver *driver = dev->driver;
> +
> + if (!driver)
> + return NULL;
> +
> + /* Attempt an OF style match */
> + if (IS_ENABLED(CONFIG_OF)) {
> + const struct of_device_id *of_match =
> + i2c_of_match_device(driver->of_match_table, dev);
> + if (of_match)
> + return (void *)of_match;
> + }
> +
> + /* Then ACPI style match */
> + if (IS_ENABLED(CONFIG_ACPI)) {
> + const struct acpi_device_id *acpi_match =
> + acpi_match_device(driver->acpi_match_table, dev);
> + if (acpi_match)
> + return (void *)acpi_match;
> + }
> +
> + /* Finally an I2C match */
> + if (IS_ENABLED(CONFIG_I2C)) {
> + struct i2c_client *client = i2c_verify_client(dev);
> + struct i2c_driver *i2c_drv = to_i2c_driver(driver);
> + struct i2c_device_id *i2c_match;
> +
> + i2c_match = i2c_match_id(i2c_drv->id_table, client);
> + if (i2c_match)
> + return (void *)i2c_match;
> + }
> +
> + return NULL;
> +}
> --
> 1.8.3.2
>
On Thu, 5 Jun 2014 09:20:08 +0100, Lee Jones <[email protected]> wrote:
> On Thu, 05 Jun 2014, Mika Westerberg wrote:
> > On Wed, Jun 04, 2014 at 02:28:20PM +0100, Lee Jones wrote:
> > > On Wed, 04 Jun 2014, Mika Westerberg wrote:
> > > > On Wed, Jun 04, 2014 at 02:37:42PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> > > > > > Currently this is a helper function for the I2C subsystem to aid the
> > > > > > matching of non-standard compatible strings and devices which use DT
> > > > > > and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
> > > > > > it has been made more generic as it can be used to only make one call
> > > > > > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> > > > > >
> > > > > > The initial aim is for of_match_device() to be replaced by this call
> > > > > > in all I2C device drivers.
> > > > > >
> > > > > > [1] Documentation/i2c/instantiating-devices
> > > > > >
> > > > > > Signed-off-by: Lee Jones <[email protected]>
> > > > >
> > > > > Mika, can you please have a look at this, please?
> > > >
> > > > I don't see any fundamental problems with this wrt. ACPI.
> > > >
> > > > That said, I find it kind of weird to have generic function that then
> > > > has knowledge of how different buses do their matching.
> > > >
> > > > I would rather see something like firmware_device_match(dev) that goes
> > > > and matches from DT/ACPI and leave bus specific match to happen internal
> > > > to that bus.
> > >
> > > Unfortunately that completely defeats the object of the patch. When a
> > > of_match_device() is invoked it solely looks up devices based on OF
> > > matching, but I2C is special in that devices can be registered via
> > > sysfs, thus will no have device node. If of_match_device() is called
> > > in one of these instances it will fail. The idea of this patch is to
> > > generify the matching into something does has the knowledge to firstly
> > > attempt a traditional match, and if that fails will fall back to a
> > > special i2c_{of,acpi}_match_device() which knows how to deal with
> > > node-less registration.
> >
> > OK, then but since this is now I2C specific, why call it device_match()
> > instead of something like i2c_device_match()? Or do you have plans to
>
> So in an early incarnation of the patch I did just that, and it might
> not actually be such a bad idea still - I'm open to other people's
> opinions on this.
>
> > add there more knowledge about other buses like SPI and PCI to name few?
>
> ... but yes, this is the new idea - that it can be expanded as required.
The whole point of this series is to deal with a special use case of i2c
that we don't need to support for the other bus types. We're having to
just through special hoops to make it work and I don't want to expand it
to other bus types if at all possible.
g.
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
On Thu, 05 Jun 2014, Grant Likely wrote:
> On Wed, 4 Jun 2014 13:09:56 +0100, Lee Jones <[email protected]> wrote:
> > Currently this is a helper function for the I2C subsystem to aid the
> > matching of non-standard compatible strings and devices which use DT
> > and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
> > it has been made more generic as it can be used to only make one call
> > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> >
> > The initial aim is for of_match_device() to be replaced by this call
> > in all I2C device drivers.
> >
> > [1] Documentation/i2c/instantiating-devices
> >
> > Signed-off-by: Lee Jones <[email protected]>
>
> I don't like this. It drops all type safety on the match entry
> and the caller has no idea what it got back.
Okay, so what's the best way forward?
Introduce a i2c_of_match_device() call instead?
> > ---
> > include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> > create mode 100644 include/linux/match.h
> >
> > diff --git a/include/linux/match.h b/include/linux/match.h
> > new file mode 100644
> > index 0000000..20a08e2
> > --- /dev/null
> > +++ b/include/linux/match.h
> > @@ -0,0 +1,40 @@
> > +#include <linux/of.h>
> > +#include <linux/acpi.h>
> > +#include <linux/i2c.h>
> > +
> > +static void *device_match(struct device *dev)
> > +{
> > + struct device_driver *driver = dev->driver;
> > +
> > + if (!driver)
> > + return NULL;
> > +
> > + /* Attempt an OF style match */
> > + if (IS_ENABLED(CONFIG_OF)) {
> > + const struct of_device_id *of_match =
> > + i2c_of_match_device(driver->of_match_table, dev);
> > + if (of_match)
> > + return (void *)of_match;
> > + }
> > +
> > + /* Then ACPI style match */
> > + if (IS_ENABLED(CONFIG_ACPI)) {
> > + const struct acpi_device_id *acpi_match =
> > + acpi_match_device(driver->acpi_match_table, dev);
> > + if (acpi_match)
> > + return (void *)acpi_match;
> > + }
> > +
> > + /* Finally an I2C match */
> > + if (IS_ENABLED(CONFIG_I2C)) {
> > + struct i2c_client *client = i2c_verify_client(dev);
> > + struct i2c_driver *i2c_drv = to_i2c_driver(driver);
> > + struct i2c_device_id *i2c_match;
> > +
> > + i2c_match = i2c_match_id(i2c_drv->id_table, client);
> > + if (i2c_match)
> > + return (void *)i2c_match;
> > + }
> > +
> > + return NULL;
> > +}
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Jun 5, 2014 at 11:37 AM, Lee Jones <[email protected]> wrote:
> On Thu, 05 Jun 2014, Grant Likely wrote:
>
>> On Wed, 4 Jun 2014 13:09:56 +0100, Lee Jones <[email protected]> wrote:
>> > Currently this is a helper function for the I2C subsystem to aid the
>> > matching of non-standard compatible strings and devices which use DT
>> > and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
>> > it has been made more generic as it can be used to only make one call
>> > for drivers which support any mixture of OF, ACPI and/or I2C matching.
>> >
>> > The initial aim is for of_match_device() to be replaced by this call
>> > in all I2C device drivers.
>> >
>> > [1] Documentation/i2c/instantiating-devices
>> >
>> > Signed-off-by: Lee Jones <[email protected]>
>>
>> I don't like this. It drops all type safety on the match entry
>> and the caller has no idea what it got back.
>
> Okay, so what's the best way forward?
>
> Introduce a i2c_of_match_device() call instead?
I still think the way to do it is to emulate the missing i2c_device_id
when calling the drivers .probe() hook by having a temporary copy on
the stack and filling it with data from the OF or ACPI table....
Actually I would completely skip trying to get the data from ACPI.
Since all of this is to continue supporting instantiating devices from
sysfs, having a fallback to the OF table completely solves that use
case.
Anticipating an objection of: "what do we do with drivers that are
ACPI only?"... That will be an incredibly rare case. If that ever does
happen then we'll solve it by simply adding an of_device_id table.
g.
On Thu, 05 Jun 2014, Grant Likely wrote:
> On Thu, Jun 5, 2014 at 11:37 AM, Lee Jones <[email protected]> wrote:
> > On Thu, 05 Jun 2014, Grant Likely wrote:
> >
> >> On Wed, 4 Jun 2014 13:09:56 +0100, Lee Jones <[email protected]> wrote:
> >> > Currently this is a helper function for the I2C subsystem to aid the
> >> > matching of non-standard compatible strings and devices which use DT
> >> > and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
> >> > it has been made more generic as it can be used to only make one call
> >> > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> >> >
> >> > The initial aim is for of_match_device() to be replaced by this call
> >> > in all I2C device drivers.
> >> >
> >> > [1] Documentation/i2c/instantiating-devices
> >> >
> >> > Signed-off-by: Lee Jones <[email protected]>
> >>
> >> I don't like this. It drops all type safety on the match entry
> >> and the caller has no idea what it got back.
> >
> > Okay, so what's the best way forward?
> >
> > Introduce a i2c_of_match_device() call instead?
>
> I still think the way to do it is to emulate the missing i2c_device_id
> when calling the drivers .probe() hook by having a temporary copy on
> the stack and filling it with data from the OF or ACPI table....
That's the opposite of what I'm trying to achieve. I'm trying to get
rid of unused i2c_device_id tables, rather than reinforce their
mandatory existence. I think an i2c_of_match_device() with knowledge
of how to match via pure DT principles (of_node/compatible) and a
fall-back, which is able to match on a provided of_device_id table
alone i.e. without the requirement of an existing of_node.
I've also been mulling over the idea of removing the second probe()
parameter, as suggested by Wolfram. However, this has quite deep
ramifications which would require a great deal of driver adaptions.
> Actually I would completely skip trying to get the data from ACPI.
> Since all of this is to continue supporting instantiating devices from
> sysfs, having a fallback to the OF table completely solves that use
> case.
> Anticipating an objection of: "what do we do with drivers that are
> ACPI only?"... That will be an incredibly rare case. If that ever does
> happen then we'll solve it by simply adding an of_device_id table.
I'm fine with leaving out ACPI support for now.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 4 Jun 2014 13:09:52 +0100, Lee Jones <[email protected]> wrote:
> A great deal of I2C devices are currently matched via DT node name, and
> as such the compatible naming convention of '<vendor>,<device>' has gone
> somewhat awry - some nodes don't supply one, some supply an arbitrary
> string and others the correct device name with an arbitrary vendor prefix.
>
> In an effort to correct this problem we have to supply a mechanism to
> match a device by compatible string AND by simple device name. This
> function strips off the '<vendor>,' part of a supplied compatible string
> and attempts to match without it.
>
> The plan is to remove this function once all of the compatible strings
> for each device have been brought into line.
>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/i2c/i2c-core.c | 25 +++++++++++++++++++++++++
> include/linux/i2c.h | 10 ++++++++++
> 2 files changed, 35 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index d3802dc..7dcd5c3 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1090,6 +1090,31 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> return i2c_verify_adapter(dev);
> }
> EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
> +
> +const struct of_device_id
> +*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
> + struct device *dev)
> +{
> + const struct i2c_client *client = i2c_verify_client(dev);
> + const char *name;
> +
> + if (!(client && matches))
> + return NULL;
> +
> + for (; matches->compatible[0]; matches++) {
> + name = strchr(matches->compatible, ',');
> + if (!name)
> + name = matches->compatible;
> + else
> + name++;
> +
> + if (!strncmp(client->name, name, strlen(client->name)))
> + return matches;
> + }
Is it actually necessary to strip off the vendor name? It would be fine
to make users include the vendor prefix when creating the device in
sysfs. In fact that would be preferrable for new drivers so that vendor
prefixes start getting used correctly.
If you're worried about preserving exisiting ABI, then I would make
striping the prefix an option that drivers can enable, but by default
only match on the full string.
g.
On Thu, Jun 5, 2014 at 4:55 PM, Lee Jones <[email protected]> wrote:
> On Thu, 05 Jun 2014, Grant Likely wrote:
>> On Thu, Jun 5, 2014 at 11:37 AM, Lee Jones <[email protected]> wrote:
>> > On Thu, 05 Jun 2014, Grant Likely wrote:
>> >
>> >> On Wed, 4 Jun 2014 13:09:56 +0100, Lee Jones <[email protected]> wrote:
>> >> > Currently this is a helper function for the I2C subsystem to aid the
>> >> > matching of non-standard compatible strings and devices which use DT
>> >> > and/or ACPI, but do not supply any nodes (see: [1] Method 4). However,
>> >> > it has been made more generic as it can be used to only make one call
>> >> > for drivers which support any mixture of OF, ACPI and/or I2C matching.
>> >> >
>> >> > The initial aim is for of_match_device() to be replaced by this call
>> >> > in all I2C device drivers.
>> >> >
>> >> > [1] Documentation/i2c/instantiating-devices
>> >> >
>> >> > Signed-off-by: Lee Jones <[email protected]>
>> >>
>> >> I don't like this. It drops all type safety on the match entry
>> >> and the caller has no idea what it got back.
>> >
>> > Okay, so what's the best way forward?
>> >
>> > Introduce a i2c_of_match_device() call instead?
>>
>> I still think the way to do it is to emulate the missing i2c_device_id
>> when calling the drivers .probe() hook by having a temporary copy on
>> the stack and filling it with data from the OF or ACPI table....
>
> That's the opposite of what I'm trying to achieve. I'm trying to get
> rid of unused i2c_device_id tables, rather than reinforce their
> mandatory existence. I think an i2c_of_match_device() with knowledge
> of how to match via pure DT principles (of_node/compatible) and a
> fall-back, which is able to match on a provided of_device_id table
> alone i.e. without the requirement of an existing of_node.
What I'm suggesting does allow all those tables to be removed, but
without a painful API breakage... having said that, it is only the
drivers that drop their i2c_device_id tables that need to support the
fallback behaviour, so it would be fine to pass null into the
i2c_device_id argument and make the driver call the new function that
returns an of_device_id regardless of whether a node is attached.
> I've also been mulling over the idea of removing the second probe()
> parameter, as suggested by Wolfram. However, this has quite deep
> ramifications which would require a great deal of driver adaptions.
That is of course the ideal. It would be a lot of work (I count 633
users), but it would get i2c exactly where you want it to be. I did
that kind of work when I merge platform_bus_type with
of_platform_bus_type.
You can mitigate it though by creating a .probe2 hook that doesn't
have the parameter and then change over all the users in separate
commits, finally removing the old hook when safe to do so.
g.
On Thu, 05 Jun 2014, Grant Likely wrote:
> On Wed, 4 Jun 2014 13:09:52 +0100, Lee Jones <[email protected]> wrote:
> > A great deal of I2C devices are currently matched via DT node name, and
> > as such the compatible naming convention of '<vendor>,<device>' has gone
> > somewhat awry - some nodes don't supply one, some supply an arbitrary
> > string and others the correct device name with an arbitrary vendor prefix.
> >
> > In an effort to correct this problem we have to supply a mechanism to
> > match a device by compatible string AND by simple device name. This
> > function strips off the '<vendor>,' part of a supplied compatible string
> > and attempts to match without it.
> >
> > The plan is to remove this function once all of the compatible strings
> > for each device have been brought into line.
> >
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > drivers/i2c/i2c-core.c | 25 +++++++++++++++++++++++++
> > include/linux/i2c.h | 10 ++++++++++
> > 2 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index d3802dc..7dcd5c3 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -1090,6 +1090,31 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> > return i2c_verify_adapter(dev);
> > }
> > EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
> > +
> > +const struct of_device_id
> > +*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
> > + struct device *dev)
> > +{
> > + const struct i2c_client *client = i2c_verify_client(dev);
> > + const char *name;
> > +
> > + if (!(client && matches))
> > + return NULL;
> > +
> > + for (; matches->compatible[0]; matches++) {
> > + name = strchr(matches->compatible, ',');
> > + if (!name)
> > + name = matches->compatible;
> > + else
> > + name++;
> > +
> > + if (!strncmp(client->name, name, strlen(client->name)))
> > + return matches;
> > + }
>
> Is it actually necessary to strip off the vendor name? It would be fine
> to make users include the vendor prefix when creating the device in
> sysfs. In fact that would be preferrable for new drivers so that vendor
> prefixes start getting used correctly.
I see a few issues with this strategy. Firstly, there are already
users registering their devices via sysfs, some are taking their
device names from an EEPROM which would require reprogramming in order
to prefix the vendor ID. I'm keen not to break existing systems -
which not stripping off the vendor name would inevitably do.
Secondly, I'm not sure how Wolfram would feel about the client->name
containing a DT compatible string. And finally, other than looking
at the kernel source, there is no real way for a user to know if the
device supports ACPI or OF, or neither and if an i2c_device_table is
supplied or not.
Remember that the idea of this set is to remove i2c_device_table's,
doing so will break any devices which are still registering via sysfs
with only the model number of the device. I think the sysfs
interface should be a black-box. I think requiring users to have
kernel knowledge is a sub-optimal idea.
> If you're worried about preserving exisiting ABI, then I would make
> striping the prefix an option that drivers can enable, but by default
> only match on the full string.
>
> g.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Jun 05, 2014 at 04:55:09PM +0100, Lee Jones wrote:
> On Thu, 05 Jun 2014, Grant Likely wrote:
> > I still think the way to do it is to emulate the missing i2c_device_id
> > when calling the drivers .probe() hook by having a temporary copy on
> > the stack and filling it with data from the OF or ACPI table....
> That's the opposite of what I'm trying to achieve. I'm trying to get
> rid of unused i2c_device_id tables, rather than reinforce their
> mandatory existence. I think an i2c_of_match_device() with knowledge
> of how to match via pure DT principles (of_node/compatible) and a
> fall-back, which is able to match on a provided of_device_id table
> alone i.e. without the requirement of an existing of_node.
> I've also been mulling over the idea of removing the second probe()
> parameter, as suggested by Wolfram. However, this has quite deep
> ramifications which would require a great deal of driver adaptions.
If you're going to do that another option is to refactor the probe()
function to take the driver_data as an argument and then have the core
pass that from whatever table it matched from rather than the entire
i2c_device_id structure. That way the driver just needs to supply all
the ID tables mapping binding information to whatever it needs and the
core can pass in the driver data from whatever table it matched against.
On Fri, 06 Jun 2014, Mark Brown wrote:
> On Thu, Jun 05, 2014 at 04:55:09PM +0100, Lee Jones wrote:
> > On Thu, 05 Jun 2014, Grant Likely wrote:
>
> > > I still think the way to do it is to emulate the missing i2c_device_id
> > > when calling the drivers .probe() hook by having a temporary copy on
> > > the stack and filling it with data from the OF or ACPI table....
>
> > That's the opposite of what I'm trying to achieve. I'm trying to get
> > rid of unused i2c_device_id tables, rather than reinforce their
> > mandatory existence. I think an i2c_of_match_device() with knowledge
> > of how to match via pure DT principles (of_node/compatible) and a
> > fall-back, which is able to match on a provided of_device_id table
> > alone i.e. without the requirement of an existing of_node.
>
> > I've also been mulling over the idea of removing the second probe()
> > parameter, as suggested by Wolfram. However, this has quite deep
> > ramifications which would require a great deal of driver adaptions.
>
> If you're going to do that another option is to refactor the probe()
> function to take the driver_data as an argument and then have the core
> pass that from whatever table it matched from rather than the entire
> i2c_device_id structure. That way the driver just needs to supply all
> the ID tables mapping binding information to whatever it needs and the
> core can pass in the driver data from whatever table it matched against.
Unfortunately this means we're back to the aforementioned typing
issue. For struct {platform,i2c,spi,acpi,etc}_device_id the driver
data is a kernel ulong but the of_device_id's driver data attribute is
a void*.
I've just started work on a migration over to a new probe(). I don't
think it's all that much work, but if there are any objections I'd
prefer to hear them now rather than waste any time.
I propose to convert a couple of drivers, one which doesn't use the
driver_data and one that does, but is DT only and send them for
review. See if Wolfram et. al like the method.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, 6 Jun 2014 13:36:53 +0100, Lee Jones <[email protected]> wrote:
> On Fri, 06 Jun 2014, Mark Brown wrote:
>
> > On Thu, Jun 05, 2014 at 04:55:09PM +0100, Lee Jones wrote:
> > > On Thu, 05 Jun 2014, Grant Likely wrote:
> >
> > > > I still think the way to do it is to emulate the missing i2c_device_id
> > > > when calling the drivers .probe() hook by having a temporary copy on
> > > > the stack and filling it with data from the OF or ACPI table....
> >
> > > That's the opposite of what I'm trying to achieve. I'm trying to get
> > > rid of unused i2c_device_id tables, rather than reinforce their
> > > mandatory existence. I think an i2c_of_match_device() with knowledge
> > > of how to match via pure DT principles (of_node/compatible) and a
> > > fall-back, which is able to match on a provided of_device_id table
> > > alone i.e. without the requirement of an existing of_node.
> >
> > > I've also been mulling over the idea of removing the second probe()
> > > parameter, as suggested by Wolfram. However, this has quite deep
> > > ramifications which would require a great deal of driver adaptions.
> >
> > If you're going to do that another option is to refactor the probe()
> > function to take the driver_data as an argument and then have the core
> > pass that from whatever table it matched from rather than the entire
> > i2c_device_id structure. That way the driver just needs to supply all
> > the ID tables mapping binding information to whatever it needs and the
> > core can pass in the driver data from whatever table it matched against.
>
> Unfortunately this means we're back to the aforementioned typing
> issue. For struct {platform,i2c,spi,acpi,etc}_device_id the driver
> data is a kernel ulong but the of_device_id's driver data attribute is
> a void*.
We're actually okay there. Each subsystem defines it's own convention
about what those values mean. ulong and void* are the same size and
every user I've seen stuffs the same data into the data field of both
tables.
> I've just started work on a migration over to a new probe(). I don't
> think it's all that much work, but if there are any objections I'd
> prefer to hear them now rather than waste any time.
I have no problem with that approach.
> I propose to convert a couple of drivers, one which doesn't use the
> driver_data and one that does, but is DT only and send them for
> review. See if Wolfram et. al like the method.
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
On Fri, 6 Jun 2014 09:10:26 +0100, Lee Jones <[email protected]> wrote:
> On Thu, 05 Jun 2014, Grant Likely wrote:
>
> > On Wed, 4 Jun 2014 13:09:52 +0100, Lee Jones <[email protected]> wrote:
> > > A great deal of I2C devices are currently matched via DT node name, and
> > > as such the compatible naming convention of '<vendor>,<device>' has gone
> > > somewhat awry - some nodes don't supply one, some supply an arbitrary
> > > string and others the correct device name with an arbitrary vendor prefix.
> > >
> > > In an effort to correct this problem we have to supply a mechanism to
> > > match a device by compatible string AND by simple device name. This
> > > function strips off the '<vendor>,' part of a supplied compatible string
> > > and attempts to match without it.
> > >
> > > The plan is to remove this function once all of the compatible strings
> > > for each device have been brought into line.
> > >
> > > Signed-off-by: Lee Jones <[email protected]>
> > > ---
> > > drivers/i2c/i2c-core.c | 25 +++++++++++++++++++++++++
> > > include/linux/i2c.h | 10 ++++++++++
> > > 2 files changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > index d3802dc..7dcd5c3 100644
> > > --- a/drivers/i2c/i2c-core.c
> > > +++ b/drivers/i2c/i2c-core.c
> > > @@ -1090,6 +1090,31 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> > > return i2c_verify_adapter(dev);
> > > }
> > > EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
> > > +
> > > +const struct of_device_id
> > > +*i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
> > > + struct device *dev)
> > > +{
> > > + const struct i2c_client *client = i2c_verify_client(dev);
> > > + const char *name;
> > > +
> > > + if (!(client && matches))
> > > + return NULL;
> > > +
> > > + for (; matches->compatible[0]; matches++) {
> > > + name = strchr(matches->compatible, ',');
> > > + if (!name)
> > > + name = matches->compatible;
> > > + else
> > > + name++;
> > > +
> > > + if (!strncmp(client->name, name, strlen(client->name)))
> > > + return matches;
> > > + }
> >
> > Is it actually necessary to strip off the vendor name? It would be fine
> > to make users include the vendor prefix when creating the device in
> > sysfs. In fact that would be preferrable for new drivers so that vendor
> > prefixes start getting used correctly.
>
> I see a few issues with this strategy. Firstly, there are already
> users registering their devices via sysfs, some are taking their
> device names from an EEPROM which would require reprogramming in order
> to prefix the vendor ID. I'm keen not to break existing systems -
> which not stripping off the vendor name would inevitably do.
> Secondly, I'm not sure how Wolfram would feel about the client->name
> containing a DT compatible string. And finally, other than looking
> at the kernel source, there is no real way for a user to know if the
> device supports ACPI or OF, or neither and if an i2c_device_table is
> supplied or not.
I just went and looked at the code. I2C_NAME_SIZE is fixed to 20
characters. Compatible strings can be larger than that, so you're right.
Stuffing it into the name field isn't a good solution.
g.
On Sat, Jun 07, 2014 at 12:42:53AM +0100, Grant Likely wrote:
> On Fri, 6 Jun 2014 13:36:53 +0100, Lee Jones <[email protected]> wrote:
> > On Fri, 06 Jun 2014, Mark Brown wrote:
> > > > I've also been mulling over the idea of removing the second probe()
> > > > parameter, as suggested by Wolfram. However, this has quite deep
> > > > ramifications which would require a great deal of driver adaptions.
> > > If you're going to do that another option is to refactor the probe()
> > > function to take the driver_data as an argument and then have the core
> > > pass that from whatever table it matched from rather than the entire
> > > i2c_device_id structure. That way the driver just needs to supply all
> > > the ID tables mapping binding information to whatever it needs and the
> > > core can pass in the driver data from whatever table it matched against.
> > Unfortunately this means we're back to the aforementioned typing
> > issue. For struct {platform,i2c,spi,acpi,etc}_device_id the driver
> > data is a kernel ulong but the of_device_id's driver data attribute is
> > a void*.
> We're actually okay there. Each subsystem defines it's own convention
> about what those values mean. ulong and void* are the same size and
> every user I've seen stuffs the same data into the data field of both
> tables.
Indeed - if we're going to go with that approach it seems to me like we
should just pick one and put any casting in the core.
> > I've just started work on a migration over to a new probe(). I don't
> > think it's all that much work, but if there are any objections I'd
> > prefer to hear them now rather than waste any time.
> I have no problem with that approach.
Converting probe() makes sense to me. I think I would prefer to see the
ID lookup handled as an argument to probe() rather than with functions
in probe() but it's not the end of the world if it isn't.