Unlike acpi_match_device(), acpi_driver_match_device() does
consider devices that provide of_match_table and performs
of_compatible() matching for such devices. The key point here is
that ACPI of_compatible() matching - acpi_of_match_device() - is
part of ACPI and does not depend on CONFIG_OF.
Consider the following case:
o !CONFIG_OF system
o probing of i2c device that provides .of_match_table, but no .id_table
i2c_device_probe()
...
if (!driver->id_table &&
!i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
!i2c_of_match_device(dev->driver->of_match_table, client)) {
status = -ENODEV;
goto put_sync_adapter;
}
i2c_of_match_device() depends on CONFIG_OF and, thus, is always false.
i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching
takes place, even though the device provides .of_match_table and ACPI,
technically, is capable of matching such device. The result is -ENODEV.
Probing will succeed, however, if we'd use .of_match_table aware ACPI
matching.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/i2c/i2c-core-acpi.c | 10 ++++------
drivers/i2c/i2c-core-base.c | 2 +-
drivers/i2c/i2c-core.h | 10 +++-------
3 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 2ade99b105b9..1dd152ae75e5 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -276,14 +276,12 @@ void i2c_acpi_register_devices(struct i2c_adapter *adap)
dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
}
-const struct acpi_device_id *
-i2c_acpi_match_device(const struct acpi_device_id *matches,
- struct i2c_client *client)
+bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
{
- if (!(client && matches))
- return NULL;
+ if (!client)
+ return false;
- return acpi_match_device(matches, &client->dev);
+ return acpi_driver_match_device(&client->dev, dev->driver);
}
static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = {
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 34a9609f256d..35ec6335852b 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -480,7 +480,7 @@ static int i2c_device_probe(struct device *dev)
* or ACPI ID table is supplied for the probing device.
*/
if (!driver->id_table &&
- !i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
+ !i2c_acpi_match_device(dev, client) &&
!i2c_of_match_device(dev->driver->of_match_table, client)) {
status = -ENODEV;
goto put_sync_adapter;
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 94ff1693b391..7ee6a6e3fb8d 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -59,19 +59,15 @@ static inline int __i2c_check_suspended(struct i2c_adapter *adap)
}
#ifdef CONFIG_ACPI
-const struct acpi_device_id *
-i2c_acpi_match_device(const struct acpi_device_id *matches,
- struct i2c_client *client);
+bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client);
void i2c_acpi_register_devices(struct i2c_adapter *adap);
int i2c_acpi_get_irq(struct i2c_client *client);
#else /* CONFIG_ACPI */
static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
-static inline const struct acpi_device_id *
-i2c_acpi_match_device(const struct acpi_device_id *matches,
- struct i2c_client *client)
+bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
{
- return NULL;
+ return false;
}
static inline int i2c_acpi_get_irq(struct i2c_client *client)
--
2.28.0
Do not export i2c_of_match_device().
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/i2c/i2c-core-of.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 3ed74aa4b44b..77faa9cc05c9 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -223,7 +223,6 @@ const struct of_device_id
return i2c_of_match_device_sysfs(matches, client);
}
-EXPORT_SYMBOL_GPL(i2c_of_match_device);
#if IS_ENABLED(CONFIG_OF_DYNAMIC)
static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
--
2.28.0
On Wed, Aug 26, 2020 at 01:29:37PM +0900, Sergey Senozhatsky wrote:
> Unlike acpi_match_device(), acpi_driver_match_device() does
> consider devices that provide of_match_table and performs
> of_compatible() matching for such devices. The key point here is
> that ACPI of_compatible() matching - acpi_of_match_device() - is
> part of ACPI and does not depend on CONFIG_OF.
>
> Consider the following case:
> o !CONFIG_OF system
> o probing of i2c device that provides .of_match_table, but no .id_table
>
> i2c_device_probe()
> ...
> if (!driver->id_table &&
> !i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
> !i2c_of_match_device(dev->driver->of_match_table, client)) {
> status = -ENODEV;
> goto put_sync_adapter;
> }
>
> i2c_of_match_device() depends on CONFIG_OF and, thus, is always false.
> i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching
> takes place, even though the device provides .of_match_table and ACPI,
> technically, is capable of matching such device. The result is -ENODEV.
> Probing will succeed, however, if we'd use .of_match_table aware ACPI
> matching.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
We have currently this in for-current which is even removing
i2c_acpi_match_device():
http://patchwork.ozlabs.org/project/linux-i2c/list/?series=196990&state=*
From a glimpse, this is basically equal but CCing Andy for the details.
> ---
> drivers/i2c/i2c-core-acpi.c | 10 ++++------
> drivers/i2c/i2c-core-base.c | 2 +-
> drivers/i2c/i2c-core.h | 10 +++-------
> 3 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 2ade99b105b9..1dd152ae75e5 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -276,14 +276,12 @@ void i2c_acpi_register_devices(struct i2c_adapter *adap)
> dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
> }
>
> -const struct acpi_device_id *
> -i2c_acpi_match_device(const struct acpi_device_id *matches,
> - struct i2c_client *client)
> +bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
> {
> - if (!(client && matches))
> - return NULL;
> + if (!client)
> + return false;
>
> - return acpi_match_device(matches, &client->dev);
> + return acpi_driver_match_device(&client->dev, dev->driver);
> }
>
> static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = {
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 34a9609f256d..35ec6335852b 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -480,7 +480,7 @@ static int i2c_device_probe(struct device *dev)
> * or ACPI ID table is supplied for the probing device.
> */
> if (!driver->id_table &&
> - !i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
> + !i2c_acpi_match_device(dev, client) &&
> !i2c_of_match_device(dev->driver->of_match_table, client)) {
> status = -ENODEV;
> goto put_sync_adapter;
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 94ff1693b391..7ee6a6e3fb8d 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -59,19 +59,15 @@ static inline int __i2c_check_suspended(struct i2c_adapter *adap)
> }
>
> #ifdef CONFIG_ACPI
> -const struct acpi_device_id *
> -i2c_acpi_match_device(const struct acpi_device_id *matches,
> - struct i2c_client *client);
> +bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client);
> void i2c_acpi_register_devices(struct i2c_adapter *adap);
>
> int i2c_acpi_get_irq(struct i2c_client *client);
> #else /* CONFIG_ACPI */
> static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
> -static inline const struct acpi_device_id *
> -i2c_acpi_match_device(const struct acpi_device_id *matches,
> - struct i2c_client *client)
> +bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
> {
> - return NULL;
> + return false;
> }
>
> static inline int i2c_acpi_get_irq(struct i2c_client *client)
> --
> 2.28.0
>
On (20/08/26 13:29), Sergey Senozhatsky wrote:
> Unlike acpi_match_device(), acpi_driver_match_device() does
> consider devices that provide of_match_table and performs
> of_compatible() matching for such devices. The key point here is
> that ACPI of_compatible() matching - acpi_of_match_device() - is
> part of ACPI and does not depend on CONFIG_OF.
>
> Consider the following case:
> o !CONFIG_OF system
> o probing of i2c device that provides .of_match_table, but no .id_table
>
> i2c_device_probe()
> ...
> if (!driver->id_table &&
> !i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
> !i2c_of_match_device(dev->driver->of_match_table, client)) {
> status = -ENODEV;
> goto put_sync_adapter;
> }
>
> i2c_of_match_device() depends on CONFIG_OF and, thus, is always false.
> i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching
> takes place, even though the device provides .of_match_table and ACPI,
> technically, is capable of matching such device. The result is -ENODEV.
> Probing will succeed, however, if we'd use .of_match_table aware ACPI
> matching.
Or, alternatively, we can drop i2c_acpi_match_device() and use
i2c_device_match() in i2c_device_probe().
---
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 34a9609f256d..14bfc5c83f84 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -96,7 +96,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;
-
/* Attempt an OF style match */
if (i2c_of_match_device(drv->of_match_table, client))
return 1;
@@ -480,8 +479,7 @@ static int i2c_device_probe(struct device *dev)
* or ACPI ID table is supplied for the probing device.
*/
if (!driver->id_table &&
- !i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
- !i2c_of_match_device(dev->driver->of_match_table, client)) {
+ !(client && i2c_device_match(&client->dev, dev->driver))) {
status = -ENODEV;
goto put_sync_adapter;
}
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 94ff1693b391..8ce261167a2d 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -59,20 +59,11 @@ static inline int __i2c_check_suspended(struct i2c_adapter *adap)
}
#ifdef CONFIG_ACPI
-const struct acpi_device_id *
-i2c_acpi_match_device(const struct acpi_device_id *matches,
- struct i2c_client *client);
void i2c_acpi_register_devices(struct i2c_adapter *adap);
int i2c_acpi_get_irq(struct i2c_client *client);
#else /* CONFIG_ACPI */
static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
-static inline const struct acpi_device_id *
-i2c_acpi_match_device(const struct acpi_device_id *matches,
- struct i2c_client *client)
-{
- return NULL;
-}
static inline int i2c_acpi_get_irq(struct i2c_client *client)
{
On (20/08/26 07:08), Wolfram Sang wrote:
> On Wed, Aug 26, 2020 at 01:29:37PM +0900, Sergey Senozhatsky wrote:
> > Unlike acpi_match_device(), acpi_driver_match_device() does
> > consider devices that provide of_match_table and performs
> > of_compatible() matching for such devices. The key point here is
> > that ACPI of_compatible() matching - acpi_of_match_device() - is
> > part of ACPI and does not depend on CONFIG_OF.
> >
> > Consider the following case:
> > o !CONFIG_OF system
> > o probing of i2c device that provides .of_match_table, but no .id_table
> >
> > i2c_device_probe()
> > ...
> > if (!driver->id_table &&
> > !i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
> > !i2c_of_match_device(dev->driver->of_match_table, client)) {
> > status = -ENODEV;
> > goto put_sync_adapter;
> > }
> >
> > i2c_of_match_device() depends on CONFIG_OF and, thus, is always false.
> > i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching
> > takes place, even though the device provides .of_match_table and ACPI,
> > technically, is capable of matching such device. The result is -ENODEV.
> > Probing will succeed, however, if we'd use .of_match_table aware ACPI
> > matching.
> >
> > Signed-off-by: Sergey Senozhatsky <[email protected]>
>
> We have currently this in for-current which is even removing
> i2c_acpi_match_device():
>
> http://patchwork.ozlabs.org/project/linux-i2c/list/?series=196990&state=*
Oh, nice!
Can we go a bit further and use i2c_device_match() in i2c_device_probe()
instead of a mix of APIs from different subsystems?
E.g.
if (!driver->id_table &&
- !i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
- !i2c_of_match_device(dev->driver->of_match_table, client)) {
+ !(client && i2c_device_match(&client->dev, dev->driver))) {
status = -ENODEV;
goto put_sync_adapter;
}
-ss
Hi Sergey,
I love your patch! Yet something to improve:
[auto build test ERROR on v5.9-rc2]
[also build test ERROR on next-20200825]
[cannot apply to wsa/i2c/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/i2c-consider-devices-with-of_match_table-during-i2c-device-probing/20200826-123138
base: d012a7190fc1fd72ed48911e77ca97ba4521bccd
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
sh4-linux-ld: drivers/i2c/i2c-core-smbus.o: in function `i2c_acpi_match_device':
>> (.text+0x2400): multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:(.text+0x3260): first defined here
sh4-linux-ld: drivers/i2c/i2c-core-slave.o: in function `i2c_acpi_match_device':
(.text+0x2c0): multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:(.text+0x3260): first defined here
sh4-linux-ld: drivers/i2c/i2c-core-of.o: in function `i2c_acpi_match_device':
(.text+0x600): multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:(.text+0x3260): first defined here
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Wed, Aug 26, 2020 at 02:25:44PM +0900, Sergey Senozhatsky wrote:
> On (20/08/26 07:08), Wolfram Sang wrote:
> > On Wed, Aug 26, 2020 at 01:29:37PM +0900, Sergey Senozhatsky wrote:
> > > Unlike acpi_match_device(), acpi_driver_match_device() does
> > > consider devices that provide of_match_table and performs
> > > of_compatible() matching for such devices. The key point here is
> > > that ACPI of_compatible() matching - acpi_of_match_device() - is
> > > part of ACPI and does not depend on CONFIG_OF.
> > >
> > > Consider the following case:
> > > o !CONFIG_OF system
> > > o probing of i2c device that provides .of_match_table, but no .id_table
> > >
> > > i2c_device_probe()
> > > ...
> > > if (!driver->id_table &&
> > > !i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
> > > !i2c_of_match_device(dev->driver->of_match_table, client)) {
> > > status = -ENODEV;
> > > goto put_sync_adapter;
> > > }
> > >
> > > i2c_of_match_device() depends on CONFIG_OF and, thus, is always false.
> > > i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching
> > > takes place, even though the device provides .of_match_table and ACPI,
> > > technically, is capable of matching such device. The result is -ENODEV.
> > > Probing will succeed, however, if we'd use .of_match_table aware ACPI
> > > matching.
Looks like you read same StackOverflow question :-)
> > > Signed-off-by: Sergey Senozhatsky <[email protected]>
> >
> > We have currently this in for-current which is even removing
> > i2c_acpi_match_device():
> >
> > http://patchwork.ozlabs.org/project/linux-i2c/list/?series=196990&state=*
>
> Oh, nice!
> Can we go a bit further and use i2c_device_match() in i2c_device_probe()
> instead of a mix of APIs from different subsystems?
> E.g.
>
> if (!driver->id_table &&
> - !i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
> - !i2c_of_match_device(dev->driver->of_match_table, client)) {
> + !(client && i2c_device_match(&client->dev, dev->driver))) {
You probably meant simply:
if (!i2c_device_match(dev, dev->driver)) {
> status = -ENODEV;
> goto put_sync_adapter;
> }
On the first glance it will work the same way but slightly longer in case of ID
table matching.
Send a patch!
--
With Best Regards,
Andy Shevchenko
On (20/08/26 12:53), Andy Shevchenko wrote:
> On Wed, Aug 26, 2020 at 02:25:44PM +0900, Sergey Senozhatsky wrote:
> > On (20/08/26 07:08), Wolfram Sang wrote:
> > > On Wed, Aug 26, 2020 at 01:29:37PM +0900, Sergey Senozhatsky wrote:
[..]
> > > > i2c_of_match_device() depends on CONFIG_OF and, thus, is always false.
> > > > i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching
> > > > takes place, even though the device provides .of_match_table and ACPI,
> > > > technically, is capable of matching such device. The result is -ENODEV.
> > > > Probing will succeed, however, if we'd use .of_match_table aware ACPI
> > > > matching.
>
> Looks like you read same StackOverflow question :-)
Nope :) Ran into actual media/i2c driver probing issue several days ago
[..]
> > if (!driver->id_table &&
> > - !i2c_acpi_match_device(dev->driver->acpi_match_table, client) &&
> > - !i2c_of_match_device(dev->driver->of_match_table, client)) {
> > + !(client && i2c_device_match(&client->dev, dev->driver))) {
>
> You probably meant simply:
>
> if (!i2c_device_match(dev, dev->driver)) {
>
> > status = -ENODEV;
> > goto put_sync_adapter;
> > }
That's shorter, yes. I wanted to keep the existing "workaround" in order
to avoid extra id_table matching. Because it probably will take place
earlier somewhere in
bus_for_each_dev()
__driver_attach()
i2c_device_match() // OF ACPI id_table match
> On the first glance it will work the same way but slightly longer in case of ID
> table matching.
Right.
-ss
On (20/08/26 12:56), Andy Shevchenko wrote:
> > You probably meant simply:
> >
> > if (!i2c_device_match(dev, dev->driver)) {
> >
> > > status = -ENODEV;
> > > goto put_sync_adapter;
> > > }
> >
> > On the first glance it will work the same way but slightly longer in case of ID
> > table matching.
> >
> > Send a patch!
>
> But then the question is why we have this code in the ->probe() at all?
> ->match() is run before probe by bus core, no?
That's a good question.
There is also one more .id_table traversal done right before ->probe()
call:
driver->probe(client, i2c_match_id(driver->id_table, client))
So in the worst case we can end up doing 3 .id_table lookups.
-ss
On (20/08/26 19:24), Sergey Senozhatsky wrote:
> > But then the question is why we have this code in the ->probe() at all?
> > ->match() is run before probe by bus core, no?
>
> That's a good question.
Everything seem to be working OK on my test board with this patch:
---
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 5ec082e2039d..77eea5c0bc71 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -475,17 +475,6 @@ static int i2c_device_probe(struct device *dev)
driver = to_i2c_driver(dev->driver);
- /*
- * An I2C ID table is not mandatory, if and only if, a suitable OF
- * or ACPI ID table is supplied for the probing device.
- */
- if (!driver->id_table &&
- !acpi_driver_match_device(dev, dev->driver) &&
- !i2c_of_match_device(dev->driver->of_match_table, client)) {
- status = -ENODEV;
- goto put_sync_adapter;
- }
-
if (client->flags & I2C_CLIENT_WAKE) {
int wakeirq;
On Wed, Aug 26, 2020 at 07:38:07PM +0900, Sergey Senozhatsky wrote:
> On (20/08/26 19:24), Sergey Senozhatsky wrote:
> > > But then the question is why we have this code in the ->probe() at all?
> > > ->match() is run before probe by bus core, no?
> >
> > That's a good question.
>
> Everything seem to be working OK on my test board with this patch:
I'm okay with it, but I want to hear Wolfram about this.
If it gets a green light to go, feel free to add
Reviewed-by: Andy Shevchenko <[email protected]>
> ---
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 5ec082e2039d..77eea5c0bc71 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -475,17 +475,6 @@ static int i2c_device_probe(struct device *dev)
>
> driver = to_i2c_driver(dev->driver);
>
> - /*
> - * An I2C ID table is not mandatory, if and only if, a suitable OF
> - * or ACPI ID table is supplied for the probing device.
> - */
> - if (!driver->id_table &&
> - !acpi_driver_match_device(dev, dev->driver) &&
> - !i2c_of_match_device(dev->driver->of_match_table, client)) {
> - status = -ENODEV;
> - goto put_sync_adapter;
> - }
> -
> if (client->flags & I2C_CLIENT_WAKE) {
> int wakeirq;
>
--
With Best Regards,
Andy Shevchenko
On Wed, Aug 26, 2020 at 01:54:26PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 26, 2020 at 07:38:07PM +0900, Sergey Senozhatsky wrote:
> > On (20/08/26 19:24), Sergey Senozhatsky wrote:
> > > > But then the question is why we have this code in the ->probe() at all?
> > > > ->match() is run before probe by bus core, no?
> > >
> > > That's a good question.
> >
> > Everything seem to be working OK on my test board with this patch:
>
> I'm okay with it, but I want to hear Wolfram about this.
> If it gets a green light to go, feel free to add
> Reviewed-by: Andy Shevchenko <[email protected]>
Sergey,
Can you send a proper patch (with patch description) and me and Jean
Delvare <[email protected]> in the To: field?
The origins of this matching code are pretty old and Jean is more
experienced there than I am. Nonetheless, I will check it, too, of
course.
Thanks for the work!
>
> > ---
> >
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 5ec082e2039d..77eea5c0bc71 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -475,17 +475,6 @@ static int i2c_device_probe(struct device *dev)
> >
> > driver = to_i2c_driver(dev->driver);
> >
> > - /*
> > - * An I2C ID table is not mandatory, if and only if, a suitable OF
> > - * or ACPI ID table is supplied for the probing device.
> > - */
> > - if (!driver->id_table &&
> > - !acpi_driver_match_device(dev, dev->driver) &&
> > - !i2c_of_match_device(dev->driver->of_match_table, client)) {
> > - status = -ENODEV;
> > - goto put_sync_adapter;
> > - }
> > -
> > if (client->flags & I2C_CLIENT_WAKE) {
> > int wakeirq;
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Hi Sergey,
I love your patch! Yet something to improve:
[auto build test ERROR on v5.9-rc2]
[cannot apply to wsa/i2c/for-next next-20200826]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/i2c-consider-devices-with-of_match_table-during-i2c-device-probing/20200826-123138
base: d012a7190fc1fd72ed48911e77ca97ba4521bccd
config: arc-randconfig-r026-20200826 (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
arceb-elf-ld: drivers/i2c/i2c-core-smbus.o: in function `i2c_acpi_match_device':
>> drivers/i2c/i2c-core.h:71: multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:drivers/i2c/i2c-core.h:71: first defined here
arceb-elf-ld: drivers/i2c/i2c-core-slave.o: in function `i2c_acpi_match_device':
>> drivers/i2c/i2c-core.h:71: multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:drivers/i2c/i2c-core.h:71: first defined here
arceb-elf-ld: drivers/i2c/i2c-core-of.o: in function `i2c_acpi_match_device':
>> drivers/i2c/i2c-core.h:71: multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:drivers/i2c/i2c-core.h:71: first defined here
# https://github.com/0day-ci/linux/commit/8cfc5676303d021ce274c0b608cac342b4aeda55
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sergey-Senozhatsky/i2c-consider-devices-with-of_match_table-during-i2c-device-probing/20200826-123138
git checkout 8cfc5676303d021ce274c0b608cac342b4aeda55
vim +71 drivers/i2c/i2c-core.h
16c9db1dd84cef Charles Keepax 2019-06-27 64
16c9db1dd84cef Charles Keepax 2019-06-27 65 int i2c_acpi_get_irq(struct i2c_client *client);
53f8f7c5cf145d Wolfram Sang 2017-05-23 66 #else /* CONFIG_ACPI */
53f8f7c5cf145d Wolfram Sang 2017-05-23 67 static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
8cfc5676303d02 Sergey Senozhatsky 2020-08-26 68 bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
c64ffff7a9d1ee Andy Shevchenko 2017-07-17 69 {
8cfc5676303d02 Sergey Senozhatsky 2020-08-26 70 return false;
c64ffff7a9d1ee Andy Shevchenko 2017-07-17 @71 }
16c9db1dd84cef Charles Keepax 2019-06-27 72
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Sergey,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v5.9-rc2]
[cannot apply to wsa/i2c/for-next next-20200826]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/i2c-consider-devices-with-of_match_table-during-i2c-device-probing/20200826-123138
base: d012a7190fc1fd72ed48911e77ca97ba4521bccd
config: arm64-randconfig-r002-20200826 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 7cfcecece0e0430937cf529ce74d3a071a4dedc6)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from drivers/i2c/i2c-boardinfo.c:13:
>> drivers/i2c/i2c-core.h:68:6: warning: no previous prototype for function 'i2c_acpi_match_device' [-Wmissing-prototypes]
bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
^
drivers/i2c/i2c-core.h:68:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
^
static
1 warning generated.
# https://github.com/0day-ci/linux/commit/8cfc5676303d021ce274c0b608cac342b4aeda55
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sergey-Senozhatsky/i2c-consider-devices-with-of_match_table-during-i2c-device-probing/20200826-123138
git checkout 8cfc5676303d021ce274c0b608cac342b4aeda55
vim +/i2c_acpi_match_device +68 drivers/i2c/i2c-core.h
64
65 int i2c_acpi_get_irq(struct i2c_client *client);
66 #else /* CONFIG_ACPI */
67 static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
> 68 bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
69 {
70 return false;
71 }
72
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Wed, Aug 26, 2020 at 12:53:56PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 26, 2020 at 02:25:44PM +0900, Sergey Senozhatsky wrote:
> > On (20/08/26 07:08), Wolfram Sang wrote:
...
> You probably meant simply:
>
> if (!i2c_device_match(dev, dev->driver)) {
>
> > status = -ENODEV;
> > goto put_sync_adapter;
> > }
>
> On the first glance it will work the same way but slightly longer in case of ID
> table matching.
>
> Send a patch!
But then the question is why we have this code in the ->probe() at all?
->match() is run before probe by bus core, no?
Wolfram?
--
With Best Regards,
Andy Shevchenko
On (20/08/26 13:23), Wolfram Sang wrote:
> On Wed, Aug 26, 2020 at 01:54:26PM +0300, Andy Shevchenko wrote:
> > On Wed, Aug 26, 2020 at 07:38:07PM +0900, Sergey Senozhatsky wrote:
> > > On (20/08/26 19:24), Sergey Senozhatsky wrote:
> > > > > But then the question is why we have this code in the ->probe() at all?
> > > > > ->match() is run before probe by bus core, no?
> > > >
> > > > That's a good question.
> > >
> > > Everything seem to be working OK on my test board with this patch:
> >
> > I'm okay with it, but I want to hear Wolfram about this.
> > If it gets a green light to go, feel free to add
> > Reviewed-by: Andy Shevchenko <[email protected]>
>
> Sergey,
>
> Can you send a proper patch (with patch description) and me and Jean
> Delvare <[email protected]> in the To: field?
>
> The origins of this matching code are pretty old and Jean is more
> experienced there than I am. Nonetheless, I will check it, too, of
> course.
Oh, sure, will do. Is that OK if I'll base my patch on linux-next?
I'm also going to test the patch on more devices here on my side.
-ss
On Wed, Aug 26, 2020 at 11:18:10PM +0900, Sergey Senozhatsky wrote:
> On (20/08/26 13:23), Wolfram Sang wrote:
> > On Wed, Aug 26, 2020 at 01:54:26PM +0300, Andy Shevchenko wrote:
> > > On Wed, Aug 26, 2020 at 07:38:07PM +0900, Sergey Senozhatsky wrote:
> > > > On (20/08/26 19:24), Sergey Senozhatsky wrote:
> > > > > > But then the question is why we have this code in the ->probe() at all?
> > > > > > ->match() is run before probe by bus core, no?
> > > > >
> > > > > That's a good question.
> > > >
> > > > Everything seem to be working OK on my test board with this patch:
> > >
> > > I'm okay with it, but I want to hear Wolfram about this.
> > > If it gets a green light to go, feel free to add
> > > Reviewed-by: Andy Shevchenko <[email protected]>
> >
> > Sergey,
> >
> > Can you send a proper patch (with patch description) and me and Jean
> > Delvare <[email protected]> in the To: field?
> >
> > The origins of this matching code are pretty old and Jean is more
> > experienced there than I am. Nonetheless, I will check it, too, of
> > course.
>
> Oh, sure, will do. Is that OK if I'll base my patch on linux-next?
> I'm also going to test the patch on more devices here on my side.
Today's one includes above mentioned patches, I think it's okay.
The i2c/for-next is currently listed
ab70935d37bb i2c: Remove 'default n' from busses/Kconfig
on top of current, don't see how it may interfere with this.
--
With Best Regards,
Andy Shevchenko
On (20/08/26 17:34), Andy Shevchenko wrote:
> > Oh, sure, will do. Is that OK if I'll base my patch on linux-next?
> > I'm also going to test the patch on more devices here on my side.
>
> Today's one includes above mentioned patches, I think it's okay.
Right, just noticed that as well. Thanks.
-ss