| 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 |
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; > +}
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.
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.
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
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
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
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
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
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 >
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
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
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!
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.
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 --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)
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(-)