diff mbox series

[v3,2/2] i2c: Add i2c_device_get_match_data() callback

Message ID 20230801170318.82682-3-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Extend device_get_match_data() to struct bus_type | expand

Commit Message

Biju Das Aug. 1, 2023, 5:03 p.m. UTC
Add i2c_device_get_match_data() callback to struct bus_type().

While at it, introduced i2c_get_match_data_helper() to avoid code
duplication with i2c_get_match_data().

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Extended to support i2c_of_match_device() as suggested by Andy.
 * Changed i2c_of_match_device_sysfs() as non-static function as it is
   needed for i2c_device_get_match_data().
 * Added a TODO comment to use i2c_verify_client() when it accepts const
   pointer.
 * Added multiple returns to make code path for device_get_match_data()
   faster in i2c_get_match_data().
RFC v1->v2:
 * Replaced "Signed-off-by"->"Suggested-by" tag for Dmitry.
 * Fixed build warnings reported by kernel test robot <lkp@intel.com>
 * Added const qualifier to return type and parameter struct i2c_driver
   in i2c_get_match_data_helper().
 * Added const qualifier to struct i2c_driver in i2c_get_match_data()
 * Dropped driver variable from i2c_device_get_match_data()
 * Replaced to_i2c_client with logic for assigning verify_client as it
   returns non const pointer.
---
 drivers/i2c/i2c-core-base.c | 49 +++++++++++++++++++++++++++++++------
 drivers/i2c/i2c-core-of.c   |  3 ++-
 include/linux/i2c.h         | 11 +++++++++
 3 files changed, 54 insertions(+), 9 deletions(-)

Comments

Andy Shevchenko Aug. 1, 2023, 7:28 p.m. UTC | #1
On Tue, Aug 01, 2023 at 06:03:18PM +0100, Biju Das wrote:
> Add i2c_device_get_match_data() callback to struct bus_type().
> 
> While at it, introduced i2c_get_match_data_helper() to avoid code
> duplication with i2c_get_match_data().

Have you used --patience when prepared the patch for sending?

...

> -const void *i2c_get_match_data(const struct i2c_client *client)
> +static const void *i2c_get_match_data_helper(const struct i2c_driver *driver,
> +					     const struct i2c_client *client)
>  {

> -	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);

Does it make sense to remove and add an additional parameter? In one case it's
a copy, in another it probably the same, just hidden in the code.

>  	const struct i2c_device_id *match;
> +
> +	match = i2c_match_id(driver->id_table, client);
> +	if (!match)
> +		return NULL;
> +
> +	return (const void *)match->driver_data;
> +}
> +
> +static const void *i2c_device_get_match_data(const struct device *dev)
> +{
> +	/* TODO: use i2c_verify_client() when it accepts const pointer */
> +	const struct i2c_client *client = (dev->type == &i2c_client_type) ?
> +					  to_i2c_client(dev) : NULL;
>  	const void *data;

> +	if (!dev->driver)
> +		return NULL;

When can this be true?

> +	data = i2c_get_match_data_helper(to_i2c_driver(dev->driver), client);
> +	if (data)
> +		return data;
>  
> -		data = (const void *)match->driver_data;
> +	if (dev->driver->of_match_table) {
> +		const struct of_device_id *match;
> +
> +		match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
> +						  (struct i2c_client *)client);
> +		if (match)
> +			data = match->data;
>  	}
>  
>  	return data;
>  }

...

> -static const struct of_device_id*
> +const struct of_device_id*

While here, add a missing space after of_device_id.

...

> +const struct of_device_id*

Ditto.

Or use below (weird) style in case it occurs more often than usual one.

> +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> +			  struct i2c_client *client);
> +
>  const struct of_device_id
>  *i2c_of_match_device(const struct of_device_id *matches,
>  		     struct i2c_client *client);

...

> +static inline const struct of_device_id
> +*i2c_of_match_device(const struct of_device_id *matches,

As per above.

> +		     struct i2c_client *client)
> +{
> +	return NULL;
> +}
Andy Shevchenko Aug. 1, 2023, 7:29 p.m. UTC | #2
On Tue, Aug 01, 2023 at 06:03:18PM +0100, Biju Das wrote:
> Add i2c_device_get_match_data() callback to struct bus_type().
> 
> While at it, introduced i2c_get_match_data_helper() to avoid code
> duplication with i2c_get_match_data().

...

>  * Changed i2c_of_match_device_sysfs() as non-static function as it is
>    needed for i2c_device_get_match_data().

Btw, this can be split to a separate change.
Dmitry Torokhov Aug. 1, 2023, 8:41 p.m. UTC | #3
On Tue, Aug 01, 2023 at 10:28:38PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 01, 2023 at 06:03:18PM +0100, Biju Das wrote:
> > Add i2c_device_get_match_data() callback to struct bus_type().
> > 
> > While at it, introduced i2c_get_match_data_helper() to avoid code
> > duplication with i2c_get_match_data().
> 
> Have you used --patience when prepared the patch for sending?
> 
> ...
> 
> > -const void *i2c_get_match_data(const struct i2c_client *client)
> > +static const void *i2c_get_match_data_helper(const struct i2c_driver *driver,
> > +					     const struct i2c_client *client)
> >  {
> 
> > -	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
> 
> Does it make sense to remove and add an additional parameter? In one case it's
> a copy, in another it probably the same, just hidden in the code.
> 
> >  	const struct i2c_device_id *match;
> > +
> > +	match = i2c_match_id(driver->id_table, client);
> > +	if (!match)
> > +		return NULL;
> > +
> > +	return (const void *)match->driver_data;
> > +}
> > +
> > +static const void *i2c_device_get_match_data(const struct device *dev)
> > +{
> > +	/* TODO: use i2c_verify_client() when it accepts const pointer */
> > +	const struct i2c_client *client = (dev->type == &i2c_client_type) ?
> > +					  to_i2c_client(dev) : NULL;
> >  	const void *data;
> 
> > +	if (!dev->driver)
> > +		return NULL;
> 
> When can this be true?

It is not guaranteed that the function is always called on a device
bound to a driver (even though we normally expect this to be the case).

> 
> > +	data = i2c_get_match_data_helper(to_i2c_driver(dev->driver), client);
> > +	if (data)
> > +		return data;
> >  
> > -		data = (const void *)match->driver_data;
> > +	if (dev->driver->of_match_table) {
> > +		const struct of_device_id *match;
> > +
> > +		match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
> > +						  (struct i2c_client *)client);

Can we make i2c_of_match_device_sysfs() take a const pointer to a
client? Casting away constness is something that we should avoid.

> > +		if (match)
> > +			data = match->data;
> >  	}
> >  
> >  	return data;
> >  }
> 
> ...
> 
> > -static const struct of_device_id*
> > +const struct of_device_id*
> 
> While here, add a missing space after of_device_id.
> 
> ...
> 
> > +const struct of_device_id*
> 
> Ditto.
> 
> Or use below (weird) style in case it occurs more often than usual one.
> 
> > +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> > +			  struct i2c_client *client);
> > +
> >  const struct of_device_id
> >  *i2c_of_match_device(const struct of_device_id *matches,
> >  		     struct i2c_client *client);
> 
> ...
> 
> > +static inline const struct of_device_id
> > +*i2c_of_match_device(const struct of_device_id *matches,
> 
> As per above.

Was it supposed to be i2c_of_match_device_sysfs()? Also, this should be
in drivers/i2c/i2c-core.h, not in public header.

Thanks.
kernel test robot Aug. 2, 2023, 4:12 a.m. UTC | #4
Hi Biju,

kernel test robot noticed the following build errors:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.5-rc4 next-20230801]
[cannot apply to sailus-media-tree/streams]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Biju-Das/drivers-fwnode-Extend-device_get_match_data-to-struct-bus_type/20230802-010931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link:    https://lore.kernel.org/r/20230801170318.82682-3-biju.das.jz%40bp.renesas.com
patch subject: [PATCH v3 2/2] i2c: Add i2c_device_get_match_data() callback
config: hexagon-randconfig-r024-20230731 (https://download.01.org/0day-ci/archive/20230802/202308021149.cnDNnUAh-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce: (https://download.01.org/0day-ci/archive/20230802/202308021149.cnDNnUAh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308021149.cnDNnUAh-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from drivers/i2c/i2c-core-slave.c:12:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/i2c/i2c-core-slave.c:12:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/i2c/i2c-core-slave.c:12:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   In file included from drivers/i2c/i2c-core-slave.c:12:
>> include/linux/i2c.h:1038:2: error: redefinition of 'i2c_of_match_device'
   *i2c_of_match_device(const struct of_device_id *matches,
    ^
   include/linux/i2c.h:1031:2: note: previous definition is here
   *i2c_of_match_device(const struct of_device_id *matches,
    ^
   6 warnings and 1 error generated.
--
   In file included from drivers/i2c/i2c-core-base.c:23:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/i2c/i2c-core-base.c:23:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/i2c/i2c-core-base.c:23:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   In file included from drivers/i2c/i2c-core-base.c:23:
>> include/linux/i2c.h:1038:2: error: redefinition of 'i2c_of_match_device'
   *i2c_of_match_device(const struct of_device_id *matches,
    ^
   include/linux/i2c.h:1031:2: note: previous definition is here
   *i2c_of_match_device(const struct of_device_id *matches,
    ^
>> drivers/i2c/i2c-core-base.c:146:11: error: implicit declaration of function 'i2c_of_match_device_sysfs' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                   match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
                           ^
   drivers/i2c/i2c-core-base.c:146:11: note: did you mean 'i2c_of_match_device'?
   include/linux/i2c.h:1038:2: note: 'i2c_of_match_device' declared here
   *i2c_of_match_device(const struct of_device_id *matches,
    ^
>> drivers/i2c/i2c-core-base.c:146:9: warning: incompatible integer to pointer conversion assigning to 'const struct of_device_id *' from 'int' [-Wint-conversion]
                   match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
                         ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   7 warnings and 2 errors generated.


vim +/i2c_of_match_device +1038 include/linux/i2c.h

  1036	
  1037	static inline const struct of_device_id
> 1038	*i2c_of_match_device(const struct of_device_id *matches,
  1039			     struct i2c_client *client)
  1040	{
  1041		return NULL;
  1042	}
  1043
kernel test robot Aug. 2, 2023, 4:22 a.m. UTC | #5
Hi Biju,

kernel test robot noticed the following build errors:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.5-rc4 next-20230801]
[cannot apply to sailus-media-tree/streams]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Biju-Das/drivers-fwnode-Extend-device_get_match_data-to-struct-bus_type/20230802-010931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link:    https://lore.kernel.org/r/20230801170318.82682-3-biju.das.jz%40bp.renesas.com
patch subject: [PATCH v3 2/2] i2c: Add i2c_device_get_match_data() callback
config: parisc-randconfig-r026-20230731 (https://download.01.org/0day-ci/archive/20230802/202308021202.uZeI7Cnt-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230802/202308021202.uZeI7Cnt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308021202.uZeI7Cnt-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from drivers/i2c/i2c-core-base.c:23:
   include/linux/i2c.h:1038:2: error: redefinition of 'i2c_of_match_device'
    1038 | *i2c_of_match_device(const struct of_device_id *matches,
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/i2c.h:1031:2: note: previous definition of 'i2c_of_match_device' with type 'const struct of_device_id *(const struct of_device_id *, struct i2c_client *)'
    1031 | *i2c_of_match_device(const struct of_device_id *matches,
         |  ^~~~~~~~~~~~~~~~~~~
   drivers/i2c/i2c-core-base.c: In function 'i2c_device_get_match_data':
>> drivers/i2c/i2c-core-base.c:146:25: error: implicit declaration of function 'i2c_of_match_device_sysfs'; did you mean 'i2c_of_match_device'? [-Werror=implicit-function-declaration]
     146 |                 match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~
         |                         i2c_of_match_device
>> drivers/i2c/i2c-core-base.c:146:23: warning: assignment to 'const struct of_device_id *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     146 |                 match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
         |                       ^
   cc1: some warnings being treated as errors


vim +146 drivers/i2c/i2c-core-base.c

   128	
   129	static const void *i2c_device_get_match_data(const struct device *dev)
   130	{
   131		/* TODO: use i2c_verify_client() when it accepts const pointer */
   132		const struct i2c_client *client = (dev->type == &i2c_client_type) ?
   133						  to_i2c_client(dev) : NULL;
   134		const void *data;
   135	
   136		if (!dev->driver)
   137			return NULL;
   138	
   139		data = i2c_get_match_data_helper(to_i2c_driver(dev->driver), client);
   140		if (data)
   141			return data;
   142	
   143		if (dev->driver->of_match_table) {
   144			const struct of_device_id *match;
   145	
 > 146			match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
   147							  (struct i2c_client *)client);
   148			if (match)
   149				data = match->data;
   150		}
   151	
   152		return data;
   153	}
   154
kernel test robot Aug. 2, 2023, 4:22 a.m. UTC | #6
Hi Biju,

kernel test robot noticed the following build errors:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.5-rc4 next-20230801]
[cannot apply to sailus-media-tree/streams]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Biju-Das/drivers-fwnode-Extend-device_get_match_data-to-struct-bus_type/20230802-010931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link:    https://lore.kernel.org/r/20230801170318.82682-3-biju.das.jz%40bp.renesas.com
patch subject: [PATCH v3 2/2] i2c: Add i2c_device_get_match_data() callback
config: i386-randconfig-r014-20230731 (https://download.01.org/0day-ci/archive/20230802/202308021233.ztWtV82e-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230802/202308021233.ztWtV82e-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308021233.ztWtV82e-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/i2c/i2c-core-base.c:23:
>> include/linux/i2c.h:1038:2: error: redefinition of 'i2c_of_match_device'
   *i2c_of_match_device(const struct of_device_id *matches,
    ^
   include/linux/i2c.h:1031:2: note: previous definition is here
   *i2c_of_match_device(const struct of_device_id *matches,
    ^
>> drivers/i2c/i2c-core-base.c:146:11: error: call to undeclared function 'i2c_of_match_device_sysfs'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
                           ^
   drivers/i2c/i2c-core-base.c:146:11: note: did you mean 'i2c_of_match_device'?
   include/linux/i2c.h:1038:2: note: 'i2c_of_match_device' declared here
   *i2c_of_match_device(const struct of_device_id *matches,
    ^
>> drivers/i2c/i2c-core-base.c:146:9: error: incompatible integer to pointer conversion assigning to 'const struct of_device_id *' from 'int' [-Wint-conversion]
                   match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
                         ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   3 errors generated.
--
   In file included from drivers/i2c/i2c-smbus.c:11:
>> include/linux/i2c.h:1038:2: error: redefinition of 'i2c_of_match_device'
   *i2c_of_match_device(const struct of_device_id *matches,
    ^
   include/linux/i2c.h:1031:2: note: previous definition is here
   *i2c_of_match_device(const struct of_device_id *matches,
    ^
   1 error generated.


vim +/i2c_of_match_device +1038 include/linux/i2c.h

  1036	
  1037	static inline const struct of_device_id
> 1038	*i2c_of_match_device(const struct of_device_id *matches,
  1039			     struct i2c_client *client)
  1040	{
  1041		return NULL;
  1042	}
  1043
kernel test robot Aug. 2, 2023, 5:25 a.m. UTC | #7
Hi Biju,

kernel test robot noticed the following build warnings:

[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.5-rc4 next-20230801]
[cannot apply to sailus-media-tree/streams]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Biju-Das/drivers-fwnode-Extend-device_get_match_data-to-struct-bus_type/20230802-010931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link:    https://lore.kernel.org/r/20230801170318.82682-3-biju.das.jz%40bp.renesas.com
patch subject: [PATCH v3 2/2] i2c: Add i2c_device_get_match_data() callback
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230802/202308021316.lB5HzyKZ-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230802/202308021316.lB5HzyKZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308021316.lB5HzyKZ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/fb.h:6,
                    from include/linux/fb.h:7,
                    from include/linux/vga_switcheroo.h:34,
                    from sound/pci/hda/hda_intel.c:52:
   include/linux/i2c.h:1038:2: error: redefinition of 'i2c_of_match_device'
    1038 | *i2c_of_match_device(const struct of_device_id *matches,
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/i2c.h:1031:2: note: previous definition of 'i2c_of_match_device' with type 'const struct of_device_id *(const struct of_device_id *, struct i2c_client *)'
    1031 | *i2c_of_match_device(const struct of_device_id *matches,
         |  ^~~~~~~~~~~~~~~~~~~
   In file included from include/linux/container_of.h:5,
                    from include/linux/kernel.h:21,
                    from arch/x86/include/asm/percpu.h:27,
                    from arch/x86/include/asm/current.h:10,
                    from include/linux/sched.h:12,
                    from include/linux/delay.h:23,
                    from sound/pci/hda/hda_intel.c:23:
   include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
      16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
         |                                                   ^
   include/linux/compiler.h:231:33: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
     231 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                 ^~~~~~~~~~~~~~~~~
   include/linux/kernel.h:56:59: note: in expansion of macro '__must_be_array'
      56 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                                           ^~~~~~~~~~~~~~~
   include/linux/moduleparam.h:517:20: note: in expansion of macro 'ARRAY_SIZE'
     517 |         = { .max = ARRAY_SIZE(array), .num = nump,                      \
         |                    ^~~~~~~~~~
   include/linux/moduleparam.h:501:9: note: in expansion of macro 'module_param_array_named'
     501 |         module_param_array_named(name, name, type, nump, perm)
         |         ^~~~~~~~~~~~~~~~~~~~~~~~
   sound/pci/hda/hda_intel.c:125:1: note: in expansion of macro 'module_param_array'
     125 | module_param_array(index, int, NULL, 0444);
         | ^~~~~~~~~~~~~~~~~~
>> sound/pci/hda/hda_intel.c:104:12: warning: 'index' defined but not used [-Wunused-variable]
     104 | static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
         |            ^~~~~


vim +/index +104 sound/pci/hda/hda_intel.c

33124929a23c5b5 Takashi Iwai      2014-06-26  102  
^1da177e4c3f415 Linus Torvalds    2005-04-16  103  
5aba4f8ec72b2b8 Takashi Iwai      2008-01-07 @104  static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
5aba4f8ec72b2b8 Takashi Iwai      2008-01-07  105  static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
a67ff6a54095e27 Rusty Russell     2011-12-15  106  static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP;
5aba4f8ec72b2b8 Takashi Iwai      2008-01-07  107  static char *model[SNDRV_CARDS];
1dac6695c683c66 Takashi Iwai      2012-09-13  108  static int position_fix[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
5c0d7bc103dd1ae Takashi Iwai      2008-06-10  109  static int bdl_pos_adj[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
5aba4f8ec72b2b8 Takashi Iwai      2008-01-07  110  static int probe_mask[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
d4d9cd0338892e7 Takashi Iwai      2008-12-19  111  static int probe_only[SNDRV_CARDS];
26a6cb6cca225f6 David Henningsson 2012-10-09  112  static int jackpoll_ms[SNDRV_CARDS];
41438f1314b0f6f Takashi Iwai      2017-01-12  113  static int single_cmd = -1;
71623855e20c3fe Takashi Iwai      2009-09-28  114  static int enable_msi = -1;
4ea6fbc8eb23c3a Takashi Iwai      2009-06-17  115  #ifdef CONFIG_SND_HDA_PATCH_LOADER
4ea6fbc8eb23c3a Takashi Iwai      2009-06-17  116  static char *patch[SNDRV_CARDS];
4ea6fbc8eb23c3a Takashi Iwai      2009-06-17  117  #endif
2dca0bba70ce3c2 Jaroslav Kysela   2009-11-13  118  #ifdef CONFIG_SND_HDA_INPUT_BEEP
0920c9b4c4d8960 Takashi Iwai      2012-07-03  119  static bool beep_mode[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] =
2dca0bba70ce3c2 Jaroslav Kysela   2009-11-13  120  					CONFIG_SND_HDA_INPUT_BEEP_MODE};
2dca0bba70ce3c2 Jaroslav Kysela   2009-11-13  121  #endif
7fba6aea4472f01 Takashi Iwai      2020-01-09  122  static bool dmic_detect = 1;
d045bceff5a904b Jaroslav Kysela   2023-02-02  123  static bool ctl_dev_id = IS_ENABLED(CONFIG_SND_HDA_CTL_DEV_ID) ? 1 : 0;
^1da177e4c3f415 Linus Torvalds    2005-04-16  124
Biju Das Aug. 2, 2023, 6:34 a.m. UTC | #8
Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/2] i2c: Add i2c_device_get_match_data()
> callback
> 
> On Tue, Aug 01, 2023 at 06:03:18PM +0100, Biju Das wrote:
> > Add i2c_device_get_match_data() callback to struct bus_type().
> >
> > While at it, introduced i2c_get_match_data_helper() to avoid code
> > duplication with i2c_get_match_data().
> 
> ...
> 
> >  * Changed i2c_of_match_device_sysfs() as non-static function as it is
> >    needed for i2c_device_get_match_data().
> 
> Btw, this can be split to a separate change.

OK, first patch is callback with I2C table match and
Second patch is for handling i2c_of_match_device().

Cheers,
Biju
Biju Das Aug. 2, 2023, 7:59 a.m. UTC | #9
Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/2] i2c: Add i2c_device_get_match_data()
> callback
> 
> On Tue, Aug 01, 2023 at 06:03:18PM +0100, Biju Das wrote:
> > Add i2c_device_get_match_data() callback to struct bus_type().
> >
> > While at it, introduced i2c_get_match_data_helper() to avoid code
> > duplication with i2c_get_match_data().
> 
> Have you used --patience when prepared the patch for sending?

Normally, I use "git format-patch -n --subject-prefix="PATCH vY" --cover-letter" for preparing patch.

I see there is a difference with "git format-patch -n --patience *".

I will send this patch series with --patience option.

> 
> ...
> 
> > -const void *i2c_get_match_data(const struct i2c_client *client)
> > +static const void *i2c_get_match_data_helper(const struct i2c_driver
> *driver,
> > +					     const struct i2c_client *client)
> >  {
> 
> > -	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
> 
> Does it make sense to remove and add an additional parameter? In one
> case it's a copy, in another it probably the same, just hidden in the
> code.

Ok, you mean add the below check in i2c_device_get_match_data() and
drop *driver parameter from i2c_get_match_data_helper().

if (!client || !dev->driver)                                             
 return NULL;

> 
> >  	const struct i2c_device_id *match;
> > +
> > +	match = i2c_match_id(driver->id_table, client);
> > +	if (!match)
> > +		return NULL;
> > +
> > +	return (const void *)match->driver_data; }
> > +
> > +static const void *i2c_device_get_match_data(const struct device
> > +*dev) {
> > +	/* TODO: use i2c_verify_client() when it accepts const pointer */
> > +	const struct i2c_client *client = (dev->type ==
> &i2c_client_type) ?
> > +					  to_i2c_client(dev) : NULL;
> >  	const void *data;
> 
> > +	if (!dev->driver)
> > +		return NULL;
> 
> When can this be true?
> 
> > +	data = i2c_get_match_data_helper(to_i2c_driver(dev->driver),
> client);
> > +	if (data)
> > +		return data;
> >
> > -		data = (const void *)match->driver_data;
> > +	if (dev->driver->of_match_table) {
> > +		const struct of_device_id *match;
> > +
> > +		match = i2c_of_match_device_sysfs(dev->driver-
> >of_match_table,
> > +						  (struct i2c_client *)client);
> > +		if (match)
> > +			data = match->data;
> >  	}
> >
> >  	return data;
> >  }
> 
> ...
> 
> > -static const struct of_device_id*
> > +const struct of_device_id*
> 
> While here, add a missing space after of_device_id.

OK.

> 
> ...
> 
> > +const struct of_device_id*
> 
> Ditto.
> 
> Or use below (weird) style in case it occurs more often than usual one.

It is one of case. So, I will use space after of_device_id.

> 
> > +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> > +			  struct i2c_client *client);
> > +
> >  const struct of_device_id
> >  *i2c_of_match_device(const struct of_device_id *matches,
> >  		     struct i2c_client *client);
> 
> ...
> 
> > +static inline const struct of_device_id *i2c_of_match_device(const
> > +struct of_device_id *matches,
> 
> As per above.

OK, This will be moved to i2c-core.h.


Cheers,
Biju

> 
> > +		     struct i2c_client *client)
> > +{
> > +	return NULL;
> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
>
Biju Das Aug. 2, 2023, 9:34 a.m. UTC | #10
Hi Dmitry Torokhov,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/2] i2c: Add i2c_device_get_match_data()
> callback
> 
> On Tue, Aug 01, 2023 at 10:28:38PM +0300, Andy Shevchenko wrote:
> > On Tue, Aug 01, 2023 at 06:03:18PM +0100, Biju Das wrote:
> > > Add i2c_device_get_match_data() callback to struct bus_type().
> > >
> > > While at it, introduced i2c_get_match_data_helper() to avoid code
> > > duplication with i2c_get_match_data().
> >
> > Have you used --patience when prepared the patch for sending?
> >
> > ...
> >
> > > -const void *i2c_get_match_data(const struct i2c_client *client)
> > > +static const void *i2c_get_match_data_helper(const struct
> i2c_driver *driver,
> > > +					     const struct i2c_client *client)
> > >  {
> >
> > > -	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
> >
> > Does it make sense to remove and add an additional parameter? In one
> > case it's a copy, in another it probably the same, just hidden in the
> code.
> >
> > >  	const struct i2c_device_id *match;
> > > +
> > > +	match = i2c_match_id(driver->id_table, client);
> > > +	if (!match)
> > > +		return NULL;
> > > +
> > > +	return (const void *)match->driver_data; }
> > > +
> > > +static const void *i2c_device_get_match_data(const struct device
> > > +*dev) {
> > > +	/* TODO: use i2c_verify_client() when it accepts const pointer */
> > > +	const struct i2c_client *client = (dev->type ==
> &i2c_client_type) ?
> > > +					  to_i2c_client(dev) : NULL;
> > >  	const void *data;
> >
> > > +	if (!dev->driver)
> > > +		return NULL;
> >
> > When can this be true?
> 
> It is not guaranteed that the function is always called on a device
> bound to a driver (even though we normally expect this to be the case).
> 
> >
> > > +	data = i2c_get_match_data_helper(to_i2c_driver(dev->driver),
> client);
> > > +	if (data)
> > > +		return data;
> > >
> > > -		data = (const void *)match->driver_data;
> > > +	if (dev->driver->of_match_table) {
> > > +		const struct of_device_id *match;
> > > +
> > > +		match = i2c_of_match_device_sysfs(dev->driver-
> >of_match_table,
> > > +						  (struct i2c_client *)client);
> 
> Can we make i2c_of_match_device_sysfs() take a const pointer to a
> client? Casting away constness is something that we should avoid.

I agree, we are not supposed to modify client pointer inside
i2c_of_match_device_sysfs(), so const makes sense here.
Wolfram, I guess you are ok with it.

> 
> > > +		if (match)
> > > +			data = match->data;
> > >  	}
> > >
> > >  	return data;
> > >  }
> >
> > ...
> >
> > > -static const struct of_device_id*
> > > +const struct of_device_id*
> >
> > While here, add a missing space after of_device_id.
> >
> > ...
> >
> > > +const struct of_device_id*
> >
> > Ditto.
> >
> > Or use below (weird) style in case it occurs more often than usual
> one.
> >
> > > +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> > > +			  struct i2c_client *client);
> > > +
> > >  const struct of_device_id
> > >  *i2c_of_match_device(const struct of_device_id *matches,
> > >  		     struct i2c_client *client);
> >
> > ...
> >
> > > +static inline const struct of_device_id *i2c_of_match_device(const
> > > +struct of_device_id *matches,
> >
> > As per above.
> 
> Was it supposed to be i2c_of_match_device_sysfs()? Also, this should be
> in drivers/i2c/i2c-core.h, not in public header.

Agreed.

Cheers,
Biju
Biju Das Aug. 2, 2023, 11:04 a.m. UTC | #11
I will send V4 fixing this error and also fixing review comments from Andy and Dmitry.

Cheers,
Biju

> Subject: Re: [PATCH v3 2/2] i2c: Add i2c_device_get_match_data()
> callback
>
> Hi Biju,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on wsa/i2c/for-next] [also build test ERROR on
> driver-core/driver-core-testing driver-core/driver-core-next driver-
> core/driver-core-linus linus/master v6.5-rc4 next-20230801] [cannot
> apply to sailus-media-tree/streams] [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%2Fdocs%2Fgit-format-
> patch%23_base_tree_information&data=05%7C01%7Cbiju.das.jz%40bp.renesas.c
> om%7C0a6df48bde964a9d1df208db930ec91c%7C53d82571da1947e49cb4625a166a4a2a
> %7C0%7C0%7C638265463936796169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=p
> KKEfl6xAPN%2F3isXNWiWAvVClYEajI1Yih0NrnrohWI%3D&reserved=0]
>
> url:
> https://github/
> .com%2Fintel-lab-lkp%2Flinux%2Fcommits%2FBiju-Das%2Fdrivers-fwnode-
> Extend-device_get_match_data-to-struct-bus_type%2F20230802-
> 010931&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C0a6df48bde964a9d1df
> 208db930ec91c%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C6382654639367
> 96169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi
> I6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=CUEi5wumZLT0TK8CAzNxKJ6tW
> csSgwP1y3m9IHDbAlo%3D&reserved=0
> base:
> https://git.ke/
> rnel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fwsa%2Flinux.git&data=05%7C
> 01%7Cbiju.das.jz%40bp.renesas.com%7C0a6df48bde964a9d1df208db930ec91c%7C5
> 3d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638265463936796169%7CUnknown%7
> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C3000%7C%7C%7C&sdata=m7eaBOcvLLPUFOm4TZkwztRu68DeRSUm0k5I%2FFck
> 2Ok%3D&reserved=0 i2c/for-next
> patch link:
> https://lore.k/
> ernel.org%2Fr%2F20230801170318.82682-3-
> biju.das.jz%2540bp.renesas.com&data=05%7C01%7Cbiju.das.jz%40bp.renesas.c
> om%7C0a6df48bde964a9d1df208db930ec91c%7C53d82571da1947e49cb4625a166a4a2a
> %7C0%7C0%7C638265463936796169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=2
> 8xs5DcJNWd52QHMFU1Mp%2F%2FWz4rSWjbzA5EXBozJpbw%3D&reserved=0
> patch subject: [PATCH v3 2/2] i2c: Add i2c_device_get_match_data()
> callback
> config: hexagon-randconfig-r024-20230731
> (https://downl/
> oad.01.org%2F0day-ci%2Farchive%2F20230802%2F202308021149.cnDNnUAh-
> lkp%40intel.com%2Fconfig&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C0
> a6df48bde964a9d1df208db930ec91c%7C53d82571da1947e49cb4625a166a4a2a%7C0%7
> C0%7C638265463936796169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=LH08RuO
> 8H23e6niXnY39LvZP5opxVNOlcrCWcbUSlLA%3D&reserved=0)
> compiler: clang version 14.0.6
> (https://githu/
> b.com%2Fllvm%2Fllvm-
> project.git&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C0a6df48bde964a
> 9d1df208db930ec91c%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63826546
> 3936796169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mcqSYepniOPtvFzx7hv%
> 2FzYAkNGk%2BhzFjP3K4xY6ojPM%3D&reserved=0
> f28c006a5895fc0e329fe15fead81e37457cb1d1)
> reproduce:
> (https://downl/
> oad.01.org%2F0day-ci%2Farchive%2F20230802%2F202308021149.cnDNnUAh-
> lkp%40intel.com%2Freproduce&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%
> 7C0a6df48bde964a9d1df208db930ec91c%7C53d82571da1947e49cb4625a166a4a2a%7C
> 0%7C0%7C638265463936796169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL
> CJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jDK%
> 2Be3Ghf14uVCvz%2BzPtNQrCxQ4PpxzqRJrnE3BTzLU%3D&reserved=0)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new
> version of the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes:
> | https://lore/
> | .kernel.org%2Foe-kbuild-all%2F202308021149.cnDNnUAh-lkp%40intel.com%2F
> | &data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C0a6df48bde964a9d1df208d
> | b930ec91c%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638265463936796
> | 169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi
> | I6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Dmp8go7rkus5Z5pTxiss9OL
> | j49ja%2F3DPGWFcwzn397M%3D&reserved=0
>
> All error/warnings (new ones prefixed by >>):
>
>    In file included from drivers/i2c/i2c-core-slave.c:12:
>    In file included from include/linux/i2c.h:19:
>    In file included from include/linux/regulator/consumer.h:35:
>    In file included from include/linux/suspend.h:5:
>    In file included from include/linux/swap.h:9:
>    In file included from include/linux/memcontrol.h:13:
>    In file included from include/linux/cgroup.h:26:
>    In file included from include/linux/kernel_stat.h:9:
>    In file included from include/linux/interrupt.h:11:
>    In file included from include/linux/hardirq.h:11:
>    In file included
> from ./arch/hexagon/include/generated/asm/hardirq.h:1:
>    In file included from include/asm-generic/hardirq.h:17:
>    In file included from include/linux/irq.h:20:
>    In file included from include/linux/io.h:13:
>    In file included from arch/hexagon/include/asm/io.h:334:
>    include/asm-generic/io.h:547:31: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            val = __raw_readb(PCI_IOBASE + addr);
>                              ~~~~~~~~~~ ^
>    include/asm-generic/io.h:560:61: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE +
> addr));
>                                                            ~~~~~~~~~~ ^
>    include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded
> from macro '__le16_to_cpu'
>    #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
>                                                      ^
>    In file included from drivers/i2c/i2c-core-slave.c:12:
>    In file included from include/linux/i2c.h:19:
>    In file included from include/linux/regulator/consumer.h:35:
>    In file included from include/linux/suspend.h:5:
>    In file included from include/linux/swap.h:9:
>    In file included from include/linux/memcontrol.h:13:
>    In file included from include/linux/cgroup.h:26:
>    In file included from include/linux/kernel_stat.h:9:
>    In file included from include/linux/interrupt.h:11:
>    In file included from include/linux/hardirq.h:11:
>    In file included
> from ./arch/hexagon/include/generated/asm/hardirq.h:1:
>    In file included from include/asm-generic/hardirq.h:17:
>    In file included from include/linux/irq.h:20:
>    In file included from include/linux/io.h:13:
>    In file included from arch/hexagon/include/asm/io.h:334:
>    include/asm-generic/io.h:573:61: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE +
> addr));
>                                                            ~~~~~~~~~~ ^
>    include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded
> from macro '__le32_to_cpu'
>    #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
>                                                      ^
>    In file included from drivers/i2c/i2c-core-slave.c:12:
>    In file included from include/linux/i2c.h:19:
>    In file included from include/linux/regulator/consumer.h:35:
>    In file included from include/linux/suspend.h:5:
>    In file included from include/linux/swap.h:9:
>    In file included from include/linux/memcontrol.h:13:
>    In file included from include/linux/cgroup.h:26:
>    In file included from include/linux/kernel_stat.h:9:
>    In file included from include/linux/interrupt.h:11:
>    In file included from include/linux/hardirq.h:11:
>    In file included
> from ./arch/hexagon/include/generated/asm/hardirq.h:1:
>    In file included from include/asm-generic/hardirq.h:17:
>    In file included from include/linux/irq.h:20:
>    In file included from include/linux/io.h:13:
>    In file included from arch/hexagon/include/asm/io.h:334:
>    include/asm-generic/io.h:584:33: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            __raw_writeb(value, PCI_IOBASE + addr);
>                                ~~~~~~~~~~ ^
>    include/asm-generic/io.h:594:59: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE +
> addr);
>                                                          ~~~~~~~~~~ ^
>    include/asm-generic/io.h:604:59: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE +
> addr);
>                                                          ~~~~~~~~~~ ^
>    In file included from drivers/i2c/i2c-core-slave.c:12:
> >> include/linux/i2c.h:1038:2: error: redefinition of
> 'i2c_of_match_device'
>    *i2c_of_match_device(const struct of_device_id *matches,
>     ^
>    include/linux/i2c.h:1031:2: note: previous definition is here
>    *i2c_of_match_device(const struct of_device_id *matches,
>     ^
>    6 warnings and 1 error generated.
> --
>    In file included from drivers/i2c/i2c-core-base.c:23:
>    In file included from include/linux/i2c.h:19:
>    In file included from include/linux/regulator/consumer.h:35:
>    In file included from include/linux/suspend.h:5:
>    In file included from include/linux/swap.h:9:
>    In file included from include/linux/memcontrol.h:13:
>    In file included from include/linux/cgroup.h:26:
>    In file included from include/linux/kernel_stat.h:9:
>    In file included from include/linux/interrupt.h:11:
>    In file included from include/linux/hardirq.h:11:
>    In file included
> from ./arch/hexagon/include/generated/asm/hardirq.h:1:
>    In file included from include/asm-generic/hardirq.h:17:
>    In file included from include/linux/irq.h:20:
>    In file included from include/linux/io.h:13:
>    In file included from arch/hexagon/include/asm/io.h:334:
>    include/asm-generic/io.h:547:31: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            val = __raw_readb(PCI_IOBASE + addr);
>                              ~~~~~~~~~~ ^
>    include/asm-generic/io.h:560:61: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE +
> addr));
>                                                            ~~~~~~~~~~ ^
>    include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded
> from macro '__le16_to_cpu'
>    #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
>                                                      ^
>    In file included from drivers/i2c/i2c-core-base.c:23:
>    In file included from include/linux/i2c.h:19:
>    In file included from include/linux/regulator/consumer.h:35:
>    In file included from include/linux/suspend.h:5:
>    In file included from include/linux/swap.h:9:
>    In file included from include/linux/memcontrol.h:13:
>    In file included from include/linux/cgroup.h:26:
>    In file included from include/linux/kernel_stat.h:9:
>    In file included from include/linux/interrupt.h:11:
>    In file included from include/linux/hardirq.h:11:
>    In file included
> from ./arch/hexagon/include/generated/asm/hardirq.h:1:
>    In file included from include/asm-generic/hardirq.h:17:
>    In file included from include/linux/irq.h:20:
>    In file included from include/linux/io.h:13:
>    In file included from arch/hexagon/include/asm/io.h:334:
>    include/asm-generic/io.h:573:61: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE +
> addr));
>                                                            ~~~~~~~~~~ ^
>    include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded
> from macro '__le32_to_cpu'
>    #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
>                                                      ^
>    In file included from drivers/i2c/i2c-core-base.c:23:
>    In file included from include/linux/i2c.h:19:
>    In file included from include/linux/regulator/consumer.h:35:
>    In file included from include/linux/suspend.h:5:
>    In file included from include/linux/swap.h:9:
>    In file included from include/linux/memcontrol.h:13:
>    In file included from include/linux/cgroup.h:26:
>    In file included from include/linux/kernel_stat.h:9:
>    In file included from include/linux/interrupt.h:11:
>    In file included from include/linux/hardirq.h:11:
>    In file included
> from ./arch/hexagon/include/generated/asm/hardirq.h:1:
>    In file included from include/asm-generic/hardirq.h:17:
>    In file included from include/linux/irq.h:20:
>    In file included from include/linux/io.h:13:
>    In file included from arch/hexagon/include/asm/io.h:334:
>    include/asm-generic/io.h:584:33: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            __raw_writeb(value, PCI_IOBASE + addr);
>                                ~~~~~~~~~~ ^
>    include/asm-generic/io.h:594:59: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE +
> addr);
>                                                          ~~~~~~~~~~ ^
>    include/asm-generic/io.h:604:59: warning: performing pointer
> arithmetic on a null pointer has undefined behavior [-Wnull-pointer-
> arithmetic]
>            __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE +
> addr);
>                                                          ~~~~~~~~~~ ^
>    In file included from drivers/i2c/i2c-core-base.c:23:
> >> include/linux/i2c.h:1038:2: error: redefinition of
> 'i2c_of_match_device'
>    *i2c_of_match_device(const struct of_device_id *matches,
>     ^
>    include/linux/i2c.h:1031:2: note: previous definition is here
>    *i2c_of_match_device(const struct of_device_id *matches,
>     ^
> >> drivers/i2c/i2c-core-base.c:146:11: error: implicit declaration of
> >> function 'i2c_of_match_device_sysfs' is invalid in C99
> >> [-Werror,-Wimplicit-function-declaration]
>                    match = i2c_of_match_device_sysfs(dev->driver-
> >of_match_table,
>                            ^
>    drivers/i2c/i2c-core-base.c:146:11: note: did you mean
> 'i2c_of_match_device'?
>    include/linux/i2c.h:1038:2: note: 'i2c_of_match_device' declared here
>    *i2c_of_match_device(const struct of_device_id *matches,
>     ^
> >> drivers/i2c/i2c-core-base.c:146:9: warning: incompatible integer to
> >> pointer conversion assigning to 'const struct of_device_id *' from
> >> 'int' [-Wint-conversion]
>                    match = i2c_of_match_device_sysfs(dev->driver-
> >of_match_table,
>                          ^
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    7 warnings and 2 errors generated.
>
>
> vim +/i2c_of_match_device +1038 include/linux/i2c.h
>
>   1036
>   1037        static inline const struct of_device_id
> > 1038        *i2c_of_match_device(const struct of_device_id *matches,
>   1039                             struct i2c_client *client)
>   1040        {
>   1041                return NULL;
>   1042        }
>   1043
>
> --
> 0-DAY CI Kernel Test Service
> https://github/
> .com%2Fintel%2Flkp-
> tests%2Fwiki&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C0a6df48bde964
> a9d1df208db930ec91c%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C6382654
> 63936796169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIi
> LCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RfyYpwqM0HOM%2BLuRc
> vB4%2F1qYFLQwDKwj9bCbA6Y965c%3D&reserved=0
Andy Shevchenko Aug. 2, 2023, 2:23 p.m. UTC | #12
On Wed, Aug 02, 2023 at 09:34:18AM +0000, Biju Das wrote:

...

> > Can we make i2c_of_match_device_sysfs() take a const pointer to a
> > client? Casting away constness is something that we should avoid.
> 
> I agree, we are not supposed to modify client pointer inside
> i2c_of_match_device_sysfs(), so const makes sense here.
> Wolfram, I guess you are ok with it.


Don't forget to do that in a separate patch as well!
Andy Shevchenko Aug. 2, 2023, 2:41 p.m. UTC | #13
On Wed, Aug 02, 2023 at 07:59:21AM +0000, Biju Das wrote:
> > On Tue, Aug 01, 2023 at 06:03:18PM +0100, Biju Das wrote:

...

> > > -	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
> > 
> > Does it make sense to remove and add an additional parameter? In one
> > case it's a copy, in another it probably the same, just hidden in the
> > code.
> 
> Ok, you mean add the below check in i2c_device_get_match_data() and
> drop *driver parameter from i2c_get_match_data_helper().

Right.

> if (!client || !dev->driver)                                             
>  return NULL;

Not sure if you need this here in this static helper.
Andy Shevchenko Aug. 2, 2023, 2:44 p.m. UTC | #14
On Wed, Aug 02, 2023 at 06:34:56AM +0000, Biju Das wrote:
> > On Tue, Aug 01, 2023 at 06:03:18PM +0100, Biju Das wrote:

...

> > >  * Changed i2c_of_match_device_sysfs() as non-static function as it is
> > >    needed for i2c_device_get_match_data().
> > 
> > Btw, this can be split to a separate change.
> 
> OK, first patch is callback with I2C table match and
> Second patch is for handling i2c_of_match_device().

One patch making API non-static, second, third, ... patches -- other things.
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..7db881a923b6 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -114,23 +114,55 @@  const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
 }
 EXPORT_SYMBOL_GPL(i2c_match_id);
 
-const void *i2c_get_match_data(const struct i2c_client *client)
+static const void *i2c_get_match_data_helper(const struct i2c_driver *driver,
+					     const struct i2c_client *client)
 {
-	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
 	const struct i2c_device_id *match;
+
+	match = i2c_match_id(driver->id_table, client);
+	if (!match)
+		return NULL;
+
+	return (const void *)match->driver_data;
+}
+
+static const void *i2c_device_get_match_data(const struct device *dev)
+{
+	/* TODO: use i2c_verify_client() when it accepts const pointer */
+	const struct i2c_client *client = (dev->type == &i2c_client_type) ?
+					  to_i2c_client(dev) : NULL;
 	const void *data;
 
-	data = device_get_match_data(&client->dev);
-	if (!data) {
-		match = i2c_match_id(driver->id_table, client);
-		if (!match)
-			return NULL;
+	if (!dev->driver)
+		return NULL;
+
+	data = i2c_get_match_data_helper(to_i2c_driver(dev->driver), client);
+	if (data)
+		return data;
 
-		data = (const void *)match->driver_data;
+	if (dev->driver->of_match_table) {
+		const struct of_device_id *match;
+
+		match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
+						  (struct i2c_client *)client);
+		if (match)
+			data = match->data;
 	}
 
 	return data;
 }
+
+const void *i2c_get_match_data(const struct i2c_client *client)
+{
+	const struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
+	const void *data;
+
+	data = device_get_match_data(&client->dev);
+	if (data)
+		return data;
+
+	return i2c_get_match_data_helper(driver, client);
+}
 EXPORT_SYMBOL(i2c_get_match_data);
 
 static int i2c_device_match(struct device *dev, struct device_driver *drv)
@@ -695,6 +727,7 @@  struct bus_type i2c_bus_type = {
 	.probe		= i2c_device_probe,
 	.remove		= i2c_device_remove,
 	.shutdown	= i2c_device_shutdown,
+	.get_match_data	= i2c_device_get_match_data,
 };
 EXPORT_SYMBOL_GPL(i2c_bus_type);
 
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index a6c407d36800..514bf8cddb81 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -113,7 +113,7 @@  void of_i2c_register_devices(struct i2c_adapter *adap)
 	of_node_put(bus);
 }
 
-static const struct of_device_id*
+const struct of_device_id*
 i2c_of_match_device_sysfs(const struct of_device_id *matches,
 				  struct i2c_client *client)
 {
@@ -141,6 +141,7 @@  i2c_of_match_device_sysfs(const struct of_device_id *matches,
 
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs);
 
 const struct of_device_id
 *i2c_of_match_device(const struct of_device_id *matches,
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 3430cc2b05a6..597aa0f1ed0b 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -999,6 +999,10 @@  static inline struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node
 	return i2c_get_adapter_by_fwnode(of_fwnode_handle(node));
 }
 
+const struct of_device_id*
+i2c_of_match_device_sysfs(const struct of_device_id *matches,
+			  struct i2c_client *client);
+
 const struct of_device_id
 *i2c_of_match_device(const struct of_device_id *matches,
 		     struct i2c_client *client);
@@ -1030,6 +1034,13 @@  static inline const struct of_device_id
 	return NULL;
 }
 
+static inline const struct of_device_id
+*i2c_of_match_device(const struct of_device_id *matches,
+		     struct i2c_client *client)
+{
+	return NULL;
+}
+
 static inline int of_i2c_get_board_info(struct device *dev,
 					struct device_node *node,
 					struct i2c_board_info *info)