[PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Sergey Senozhatsky]

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 <sergey.senozhatsky@gmail.com>
---
 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

[PATCH 2/2] i2c: do not export i2c_of_match_device() symbol

[Sergey Senozhatsky]

Do not export i2c_of_match_device().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 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

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Sergey Senozhatsky]

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)
 {

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 4286 bytes --]

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 <sergey.senozhatsky@gmail.com>

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Sergey Senozhatsky]

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 <sergey.senozhatsky@gmail.com>
> 
> 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

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

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 <lkp@intel.com>

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/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52703 bytes --]

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Andy Shevchenko]

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 <sergey.senozhatsky@gmail.com>
> > 
> > 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

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Andy Shevchenko]

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

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Sergey Senozhatsky]

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

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Sergey Senozhatsky]

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

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Sergey Senozhatsky]

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;
 

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>

> ---
> 
> 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

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1805 bytes --]

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 <andriy.shevchenko@linux.intel.com>

Sergey,

Can you send a proper patch (with patch description) and me and Jean
Delvare <jdelvare@suse.de> 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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2608 bytes --]

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 <lkp@intel.com>

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/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41782 bytes --]

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]

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 <lkp@intel.com>

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/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27702 bytes --]

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Sergey Senozhatsky]

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 <andriy.shevchenko@linux.intel.com>
> 
> Sergey,
> 
> Can you send a proper patch (with patch description) and me and Jean
> Delvare <jdelvare@suse.de> 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

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
> > 
> > Sergey,
> > 
> > Can you send a proper patch (with patch description) and me and Jean
> > Delvare <jdelvare@suse.de> 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

Re: [PATCH 1/2] i2c: consider devices with of_match_table during i2c device probing

[Sergey Senozhatsky]

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