* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 13:14 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 13:14 UTC (permalink / raw)
To: linux-arm-kernel
The emev2 driver stopped compiling in today's linux-next kernel:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
to add a dependency on that symbol:
* The symbol is user-selectable, but only one or two (including this
one) bus drivers actually implement it, and it makes no sense
if you don't have one of them.
* The other driver (R-Car) uses 'select I2C_SLAVE', which seems
reasonable in principle, but we should not do that on user
visible symbols.
* I2C slave mode could be implemented in a lot of other drivers
as an optional feature, but we shouldn't require enabling it
if we don't use it.
This changes the two drivers that provide I2C slave mode so they
can again build if the slave mode being disabled. To do this, I
move the definition of i2c_slave_event() and enum i2c_slave_event
out of the #ifdef and instead make the assignment of the reg_slave
and unreg_slave pointers optional in the bus drivers. The functions
implementing the feature are unused in that case, so they get
marked as __maybe_unused in order to still give compile-time
coverage.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: c31d0a00021d ("i2c: emev2: add slave support")
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 69c46fe13777..1c8d53f34dd3 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -985,7 +985,6 @@ config I2C_XLP9XX
config I2C_RCAR
tristate "Renesas R-Car I2C Controller"
depends on ARCH_SHMOBILE || COMPILE_TEST
- select I2C_SLAVE
help
If you say yes to this option, support will be included for the
R-Car I2C controller.
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 96bb4e749012..75d6095c5fe1 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -316,7 +316,7 @@ static u32 em_i2c_func(struct i2c_adapter *adap)
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
}
-static int em_i2c_reg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_reg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -334,7 +334,7 @@ static int em_i2c_reg_slave(struct i2c_client *slave)
return 0;
}
-static int em_i2c_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_unreg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -350,8 +350,10 @@ static int em_i2c_unreg_slave(struct i2c_client *slave)
static struct i2c_algorithm em_i2c_algo = {
.master_xfer = em_i2c_xfer,
.functionality = em_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = em_i2c_reg_slave,
.unreg_slave = em_i2c_unreg_slave,
+#endif
};
static int em_i2c_probe(struct platform_device *pdev)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 3ed1f0aa5eeb..e67824adeba0 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -521,7 +521,7 @@ out:
return ret;
}
-static int rcar_reg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_reg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -542,7 +542,7 @@ static int rcar_reg_slave(struct i2c_client *slave)
return 0;
}
-static int rcar_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_unreg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -568,8 +568,10 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
static const struct i2c_algorithm rcar_i2c_algo = {
.master_xfer = rcar_i2c_master_xfer,
.functionality = rcar_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = rcar_reg_slave,
.unreg_slave = rcar_unreg_slave,
+#endif
};
static const struct of_device_id rcar_i2c_dt_ids[] = {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 51028f351d13..69871e5ee44a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -254,7 +254,6 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
/* I2C slave support */
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
enum i2c_slave_event {
I2C_SLAVE_READ_REQUESTED,
I2C_SLAVE_WRITE_REQUESTED,
@@ -269,9 +268,12 @@ extern int i2c_slave_unregister(struct i2c_client *client);
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
{
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
return client->slave_cb(client, event, val);
-}
+#else
+ return 0;
#endif
+}
/**
* struct i2c_board_info - template for device creation
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 13:14 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 13:14 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: Niklas Söderlund, linux-sh, linux-arm-kernel, linux-kernel
The emev2 driver stopped compiling in today's linux-next kernel:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
to add a dependency on that symbol:
* The symbol is user-selectable, but only one or two (including this
one) bus drivers actually implement it, and it makes no sense
if you don't have one of them.
* The other driver (R-Car) uses 'select I2C_SLAVE', which seems
reasonable in principle, but we should not do that on user
visible symbols.
* I2C slave mode could be implemented in a lot of other drivers
as an optional feature, but we shouldn't require enabling it
if we don't use it.
This changes the two drivers that provide I2C slave mode so they
can again build if the slave mode being disabled. To do this, I
move the definition of i2c_slave_event() and enum i2c_slave_event
out of the #ifdef and instead make the assignment of the reg_slave
and unreg_slave pointers optional in the bus drivers. The functions
implementing the feature are unused in that case, so they get
marked as __maybe_unused in order to still give compile-time
coverage.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: c31d0a00021d ("i2c: emev2: add slave support")
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 69c46fe13777..1c8d53f34dd3 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -985,7 +985,6 @@ config I2C_XLP9XX
config I2C_RCAR
tristate "Renesas R-Car I2C Controller"
depends on ARCH_SHMOBILE || COMPILE_TEST
- select I2C_SLAVE
help
If you say yes to this option, support will be included for the
R-Car I2C controller.
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 96bb4e749012..75d6095c5fe1 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -316,7 +316,7 @@ static u32 em_i2c_func(struct i2c_adapter *adap)
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
}
-static int em_i2c_reg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_reg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -334,7 +334,7 @@ static int em_i2c_reg_slave(struct i2c_client *slave)
return 0;
}
-static int em_i2c_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_unreg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -350,8 +350,10 @@ static int em_i2c_unreg_slave(struct i2c_client *slave)
static struct i2c_algorithm em_i2c_algo = {
.master_xfer = em_i2c_xfer,
.functionality = em_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = em_i2c_reg_slave,
.unreg_slave = em_i2c_unreg_slave,
+#endif
};
static int em_i2c_probe(struct platform_device *pdev)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 3ed1f0aa5eeb..e67824adeba0 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -521,7 +521,7 @@ out:
return ret;
}
-static int rcar_reg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_reg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -542,7 +542,7 @@ static int rcar_reg_slave(struct i2c_client *slave)
return 0;
}
-static int rcar_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_unreg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -568,8 +568,10 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
static const struct i2c_algorithm rcar_i2c_algo = {
.master_xfer = rcar_i2c_master_xfer,
.functionality = rcar_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = rcar_reg_slave,
.unreg_slave = rcar_unreg_slave,
+#endif
};
static const struct of_device_id rcar_i2c_dt_ids[] = {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 51028f351d13..69871e5ee44a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -254,7 +254,6 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
/* I2C slave support */
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
enum i2c_slave_event {
I2C_SLAVE_READ_REQUESTED,
I2C_SLAVE_WRITE_REQUESTED,
@@ -269,9 +268,12 @@ extern int i2c_slave_unregister(struct i2c_client *client);
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
{
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
return client->slave_cb(client, event, val);
-}
+#else
+ return 0;
#endif
+}
/**
* struct i2c_board_info - template for device creation
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 13:14 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 13:14 UTC (permalink / raw)
To: linux-arm-kernel
The emev2 driver stopped compiling in today's linux-next kernel:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
to add a dependency on that symbol:
* The symbol is user-selectable, but only one or two (including this
one) bus drivers actually implement it, and it makes no sense
if you don't have one of them.
* The other driver (R-Car) uses 'select I2C_SLAVE', which seems
reasonable in principle, but we should not do that on user
visible symbols.
* I2C slave mode could be implemented in a lot of other drivers
as an optional feature, but we shouldn't require enabling it
if we don't use it.
This changes the two drivers that provide I2C slave mode so they
can again build if the slave mode being disabled. To do this, I
move the definition of i2c_slave_event() and enum i2c_slave_event
out of the #ifdef and instead make the assignment of the reg_slave
and unreg_slave pointers optional in the bus drivers. The functions
implementing the feature are unused in that case, so they get
marked as __maybe_unused in order to still give compile-time
coverage.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: c31d0a00021d ("i2c: emev2: add slave support")
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 69c46fe13777..1c8d53f34dd3 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -985,7 +985,6 @@ config I2C_XLP9XX
config I2C_RCAR
tristate "Renesas R-Car I2C Controller"
depends on ARCH_SHMOBILE || COMPILE_TEST
- select I2C_SLAVE
help
If you say yes to this option, support will be included for the
R-Car I2C controller.
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 96bb4e749012..75d6095c5fe1 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -316,7 +316,7 @@ static u32 em_i2c_func(struct i2c_adapter *adap)
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
}
-static int em_i2c_reg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_reg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -334,7 +334,7 @@ static int em_i2c_reg_slave(struct i2c_client *slave)
return 0;
}
-static int em_i2c_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_unreg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -350,8 +350,10 @@ static int em_i2c_unreg_slave(struct i2c_client *slave)
static struct i2c_algorithm em_i2c_algo = {
.master_xfer = em_i2c_xfer,
.functionality = em_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = em_i2c_reg_slave,
.unreg_slave = em_i2c_unreg_slave,
+#endif
};
static int em_i2c_probe(struct platform_device *pdev)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 3ed1f0aa5eeb..e67824adeba0 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -521,7 +521,7 @@ out:
return ret;
}
-static int rcar_reg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_reg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -542,7 +542,7 @@ static int rcar_reg_slave(struct i2c_client *slave)
return 0;
}
-static int rcar_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_unreg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -568,8 +568,10 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
static const struct i2c_algorithm rcar_i2c_algo = {
.master_xfer = rcar_i2c_master_xfer,
.functionality = rcar_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = rcar_reg_slave,
.unreg_slave = rcar_unreg_slave,
+#endif
};
static const struct of_device_id rcar_i2c_dt_ids[] = {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 51028f351d13..69871e5ee44a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -254,7 +254,6 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
/* I2C slave support */
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
enum i2c_slave_event {
I2C_SLAVE_READ_REQUESTED,
I2C_SLAVE_WRITE_REQUESTED,
@@ -269,9 +268,12 @@ extern int i2c_slave_unregister(struct i2c_client *client);
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
{
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
return client->slave_cb(client, event, val);
-}
+#else
+ return 0;
#endif
+}
/**
* struct i2c_board_info - template for device creation
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-10 13:14 ` Arnd Bergmann
(?)
@ 2015-12-10 13:34 ` Wolfram Sang
-1 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-10 13:34 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]
On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> The emev2 driver stopped compiling in today's linux-next kernel:
>
> drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
> drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
>
> It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> to add a dependency on that symbol:
>
> * The symbol is user-selectable, but only one or two (including this
> one) bus drivers actually implement it, and it makes no sense
> if you don't have one of them.
>
> * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
> reasonable in principle, but we should not do that on user
> visible symbols.
>
> * I2C slave mode could be implemented in a lot of other drivers
> as an optional feature, but we shouldn't require enabling it
> if we don't use it.
>
> This changes the two drivers that provide I2C slave mode so they
> can again build if the slave mode being disabled. To do this, I
> move the definition of i2c_slave_event() and enum i2c_slave_event
> out of the #ifdef and instead make the assignment of the reg_slave
> and unreg_slave pointers optional in the bus drivers. The functions
> implementing the feature are unused in that case, so they get
> marked as __maybe_unused in order to still give compile-time
> coverage.
Thanks a lot! Making this clear and consistent was on my todo-list,
unfortunately below some other items.
Both drivers have quite orthogonal slave_irq routines. What do you think
about grouping this and the reg/unreg-calls together and compile them
conditionally on I2C_SLAVE? I think the code savings are worth it.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 13:34 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-10 13:34 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-i2c, Niklas Söderlund, linux-sh, linux-arm-kernel,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]
On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> The emev2 driver stopped compiling in today's linux-next kernel:
>
> drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
> drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
>
> It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> to add a dependency on that symbol:
>
> * The symbol is user-selectable, but only one or two (including this
> one) bus drivers actually implement it, and it makes no sense
> if you don't have one of them.
>
> * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
> reasonable in principle, but we should not do that on user
> visible symbols.
>
> * I2C slave mode could be implemented in a lot of other drivers
> as an optional feature, but we shouldn't require enabling it
> if we don't use it.
>
> This changes the two drivers that provide I2C slave mode so they
> can again build if the slave mode being disabled. To do this, I
> move the definition of i2c_slave_event() and enum i2c_slave_event
> out of the #ifdef and instead make the assignment of the reg_slave
> and unreg_slave pointers optional in the bus drivers. The functions
> implementing the feature are unused in that case, so they get
> marked as __maybe_unused in order to still give compile-time
> coverage.
Thanks a lot! Making this clear and consistent was on my todo-list,
unfortunately below some other items.
Both drivers have quite orthogonal slave_irq routines. What do you think
about grouping this and the reg/unreg-calls together and compile them
conditionally on I2C_SLAVE? I think the code savings are worth it.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 13:34 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-10 13:34 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> The emev2 driver stopped compiling in today's linux-next kernel:
>
> drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
> drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
>
> It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> to add a dependency on that symbol:
>
> * The symbol is user-selectable, but only one or two (including this
> one) bus drivers actually implement it, and it makes no sense
> if you don't have one of them.
>
> * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
> reasonable in principle, but we should not do that on user
> visible symbols.
>
> * I2C slave mode could be implemented in a lot of other drivers
> as an optional feature, but we shouldn't require enabling it
> if we don't use it.
>
> This changes the two drivers that provide I2C slave mode so they
> can again build if the slave mode being disabled. To do this, I
> move the definition of i2c_slave_event() and enum i2c_slave_event
> out of the #ifdef and instead make the assignment of the reg_slave
> and unreg_slave pointers optional in the bus drivers. The functions
> implementing the feature are unused in that case, so they get
> marked as __maybe_unused in order to still give compile-time
> coverage.
Thanks a lot! Making this clear and consistent was on my todo-list,
unfortunately below some other items.
Both drivers have quite orthogonal slave_irq routines. What do you think
about grouping this and the reg/unreg-calls together and compile them
conditionally on I2C_SLAVE? I think the code savings are worth it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151210/29670801/attachment.sig>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-10 13:34 ` Wolfram Sang
(?)
(?)
@ 2015-12-10 13:51 ` Arnd Bergmann
-1 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 13:51 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 10 December 2015 14:34:46 Wolfram Sang wrote:
> On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> > The emev2 driver stopped compiling in today's linux-next kernel:
> >
> > drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> > drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
> > drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> > drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
> >
> > It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> > to add a dependency on that symbol:
> >
> > * The symbol is user-selectable, but only one or two (including this
> > one) bus drivers actually implement it, and it makes no sense
> > if you don't have one of them.
> >
> > * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
> > reasonable in principle, but we should not do that on user
> > visible symbols.
> >
> > * I2C slave mode could be implemented in a lot of other drivers
> > as an optional feature, but we shouldn't require enabling it
> > if we don't use it.
> >
> > This changes the two drivers that provide I2C slave mode so they
> > can again build if the slave mode being disabled. To do this, I
> > move the definition of i2c_slave_event() and enum i2c_slave_event
> > out of the #ifdef and instead make the assignment of the reg_slave
> > and unreg_slave pointers optional in the bus drivers. The functions
> > implementing the feature are unused in that case, so they get
> > marked as __maybe_unused in order to still give compile-time
> > coverage.
>
> Thanks a lot! Making this clear and consistent was on my todo-list,
> unfortunately below some other items.
>
> Both drivers have quite orthogonal slave_irq routines. What do you think
> about grouping this and the reg/unreg-calls together and compile them
> conditionally on I2C_SLAVE? I think the code savings are worth it.
Ah, I had missed that part, good point. The change below should take
care of leaving out the extra code here. Do you want to just fold that
in?
Arnd
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 75d6095c5fe1..b01c9f30c545 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -303,7 +303,7 @@ static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
{
struct em_i2c_device *priv = dev_id;
- if (em_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && em_i2c_slave_irq(priv))
return IRQ_HANDLED;
complete(&priv->msg_done);
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index e67824adeba0..a1ea66d6267e 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -428,7 +428,7 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
/* Only handle interrupts that are currently enabled */
msr &= rcar_i2c_read(priv, ICMIER);
if (!msr) {
- if (rcar_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && rcar_i2c_slave_irq(priv))
return IRQ_HANDLED;
return IRQ_NONE;
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 13:51 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 13:51 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, Niklas Söderlund, linux-sh, linux-arm-kernel,
linux-kernel
On Thursday 10 December 2015 14:34:46 Wolfram Sang wrote:
> On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> > The emev2 driver stopped compiling in today's linux-next kernel:
> >
> > drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> > drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
> > drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> > drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
> >
> > It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> > to add a dependency on that symbol:
> >
> > * The symbol is user-selectable, but only one or two (including this
> > one) bus drivers actually implement it, and it makes no sense
> > if you don't have one of them.
> >
> > * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
> > reasonable in principle, but we should not do that on user
> > visible symbols.
> >
> > * I2C slave mode could be implemented in a lot of other drivers
> > as an optional feature, but we shouldn't require enabling it
> > if we don't use it.
> >
> > This changes the two drivers that provide I2C slave mode so they
> > can again build if the slave mode being disabled. To do this, I
> > move the definition of i2c_slave_event() and enum i2c_slave_event
> > out of the #ifdef and instead make the assignment of the reg_slave
> > and unreg_slave pointers optional in the bus drivers. The functions
> > implementing the feature are unused in that case, so they get
> > marked as __maybe_unused in order to still give compile-time
> > coverage.
>
> Thanks a lot! Making this clear and consistent was on my todo-list,
> unfortunately below some other items.
>
> Both drivers have quite orthogonal slave_irq routines. What do you think
> about grouping this and the reg/unreg-calls together and compile them
> conditionally on I2C_SLAVE? I think the code savings are worth it.
Ah, I had missed that part, good point. The change below should take
care of leaving out the extra code here. Do you want to just fold that
in?
Arnd
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 75d6095c5fe1..b01c9f30c545 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -303,7 +303,7 @@ static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
{
struct em_i2c_device *priv = dev_id;
- if (em_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && em_i2c_slave_irq(priv))
return IRQ_HANDLED;
complete(&priv->msg_done);
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index e67824adeba0..a1ea66d6267e 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -428,7 +428,7 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
/* Only handle interrupts that are currently enabled */
msr &= rcar_i2c_read(priv, ICMIER);
if (!msr) {
- if (rcar_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && rcar_i2c_slave_irq(priv))
return IRQ_HANDLED;
return IRQ_NONE;
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 13:51 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 13:51 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-kernel, Niklas Söderlund, linux-i2c, linux-arm-kernel,
linux-sh
On Thursday 10 December 2015 14:34:46 Wolfram Sang wrote:
> On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> > The emev2 driver stopped compiling in today's linux-next kernel:
> >
> > drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> > drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
> > drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> > drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
> >
> > It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> > to add a dependency on that symbol:
> >
> > * The symbol is user-selectable, but only one or two (including this
> > one) bus drivers actually implement it, and it makes no sense
> > if you don't have one of them.
> >
> > * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
> > reasonable in principle, but we should not do that on user
> > visible symbols.
> >
> > * I2C slave mode could be implemented in a lot of other drivers
> > as an optional feature, but we shouldn't require enabling it
> > if we don't use it.
> >
> > This changes the two drivers that provide I2C slave mode so they
> > can again build if the slave mode being disabled. To do this, I
> > move the definition of i2c_slave_event() and enum i2c_slave_event
> > out of the #ifdef and instead make the assignment of the reg_slave
> > and unreg_slave pointers optional in the bus drivers. The functions
> > implementing the feature are unused in that case, so they get
> > marked as __maybe_unused in order to still give compile-time
> > coverage.
>
> Thanks a lot! Making this clear and consistent was on my todo-list,
> unfortunately below some other items.
>
> Both drivers have quite orthogonal slave_irq routines. What do you think
> about grouping this and the reg/unreg-calls together and compile them
> conditionally on I2C_SLAVE? I think the code savings are worth it.
Ah, I had missed that part, good point. The change below should take
care of leaving out the extra code here. Do you want to just fold that
in?
Arnd
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 75d6095c5fe1..b01c9f30c545 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -303,7 +303,7 @@ static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
{
struct em_i2c_device *priv = dev_id;
- if (em_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && em_i2c_slave_irq(priv))
return IRQ_HANDLED;
complete(&priv->msg_done);
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index e67824adeba0..a1ea66d6267e 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -428,7 +428,7 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
/* Only handle interrupts that are currently enabled */
msr &= rcar_i2c_read(priv, ICMIER);
if (!msr) {
- if (rcar_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && rcar_i2c_slave_irq(priv))
return IRQ_HANDLED;
return IRQ_NONE;
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 13:51 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 13:51 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 10 December 2015 14:34:46 Wolfram Sang wrote:
> On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> > The emev2 driver stopped compiling in today's linux-next kernel:
> >
> > drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> > drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
> > drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> > drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
> >
> > It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> > to add a dependency on that symbol:
> >
> > * The symbol is user-selectable, but only one or two (including this
> > one) bus drivers actually implement it, and it makes no sense
> > if you don't have one of them.
> >
> > * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
> > reasonable in principle, but we should not do that on user
> > visible symbols.
> >
> > * I2C slave mode could be implemented in a lot of other drivers
> > as an optional feature, but we shouldn't require enabling it
> > if we don't use it.
> >
> > This changes the two drivers that provide I2C slave mode so they
> > can again build if the slave mode being disabled. To do this, I
> > move the definition of i2c_slave_event() and enum i2c_slave_event
> > out of the #ifdef and instead make the assignment of the reg_slave
> > and unreg_slave pointers optional in the bus drivers. The functions
> > implementing the feature are unused in that case, so they get
> > marked as __maybe_unused in order to still give compile-time
> > coverage.
>
> Thanks a lot! Making this clear and consistent was on my todo-list,
> unfortunately below some other items.
>
> Both drivers have quite orthogonal slave_irq routines. What do you think
> about grouping this and the reg/unreg-calls together and compile them
> conditionally on I2C_SLAVE? I think the code savings are worth it.
Ah, I had missed that part, good point. The change below should take
care of leaving out the extra code here. Do you want to just fold that
in?
Arnd
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 75d6095c5fe1..b01c9f30c545 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -303,7 +303,7 @@ static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
{
struct em_i2c_device *priv = dev_id;
- if (em_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && em_i2c_slave_irq(priv))
return IRQ_HANDLED;
complete(&priv->msg_done);
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index e67824adeba0..a1ea66d6267e 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -428,7 +428,7 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
/* Only handle interrupts that are currently enabled */
msr &= rcar_i2c_read(priv, ICMIER);
if (!msr) {
- if (rcar_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && rcar_i2c_slave_irq(priv))
return IRQ_HANDLED;
return IRQ_NONE;
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-10 13:14 ` Arnd Bergmann
(?)
@ 2015-12-10 14:54 ` kbuild test robot
-1 siblings, 0 replies; 52+ messages in thread
From: kbuild test robot @ 2015-12-10 14:54 UTC (permalink / raw)
To: linux-sh
[-- Attachment #1: Type: text/plain, Size: 5094 bytes --]
Hi Arnd,
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on next-20151210]
[cannot apply to v4.4-rc4]
url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/i2c-allow-building-emev2-without-slave-mode-again/20151210-211642
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux i2c/for-next
config: x86_64-randconfig-r0-12102217 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
In file included from arch/x86/include/asm/realmode.h:5:0,
from arch/x86/include/asm/acpi.h:33,
from arch/x86/include/asm/fixmap.h:19,
from arch/x86/include/asm/apic.h:12,
from arch/x86/include/asm/smp.h:12,
from arch/x86/include/asm/mmzone_64.h:10,
from arch/x86/include/asm/mmzone.h:4,
from include/linux/mmzone.h:856,
from include/linux/gfp.h:5,
from include/linux/device.h:29,
from drivers/i2c/busses/i2c-emev2.c:15:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_irq_handler':
>> arch/x86/include/asm/io.h:53:3: warning: 'value' may be used uninitialized in this function [-Wmaybe-uninitialized]
{ asm volatile("mov" size " %0,%1": :reg (val), \
^
drivers/i2c/busses/i2c-emev2.c:232:13: note: 'value' was declared here
u8 status, value;
^
vim +/value +53 arch/x86/include/asm/io.h
b310f381d include/asm-x86/io.h venkatesh.pallipadi@intel.com 2008-03-18 37 #define ARCH_HAS_IOREMAP_WC
d838270e2 arch/x86/include/asm/io.h Toshi Kani 2015-06-04 38 #define ARCH_HAS_IOREMAP_WT
b310f381d include/asm-x86/io.h venkatesh.pallipadi@intel.com 2008-03-18 39
1c5b9069e arch/x86/include/asm/io.h Brian Gerst 2010-02-05 40 #include <linux/string.h>
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 41 #include <linux/compiler.h>
976e8f677 arch/x86/include/asm/io.h Jeremy Fitzhardinge 2009-02-06 42 #include <asm/page.h>
5b7c73e00 arch/x86/include/asm/io.h Mark Salter 2014-04-07 43 #include <asm/early_ioremap.h>
d6472302f arch/x86/include/asm/io.h Stephen Rothwell 2015-06-02 44 #include <asm/pgtable_types.h>
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 45
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 46 #define build_mmio_read(name, size, type, reg, barrier) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 47 static inline type name(const volatile void __iomem *addr) \
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 48 { type ret; asm volatile("mov" size " %1,%0":reg (ret) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 49 :"m" (*(volatile type __force *)addr) barrier); return ret; }
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 50
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 51 #define build_mmio_write(name, size, type, reg, barrier) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 52 static inline void name(type val, volatile void __iomem *addr) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 @53 { asm volatile("mov" size " %0,%1": :reg (val), \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 54 "m" (*(volatile type __force *)addr) barrier); }
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 55
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 56 build_mmio_read(readb, "b", unsigned char, "=q", :"memory")
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 57 build_mmio_read(readw, "w", unsigned short, "=r", :"memory")
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 58 build_mmio_read(readl, "l", unsigned int, "=r", :"memory")
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 59
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 60 build_mmio_read(__readb, "b", unsigned char, "=q", )
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 61 build_mmio_read(__readw, "w", unsigned short, "=r", )
:::::: The code at line 53 was first introduced by commit
:::::: c1f64a58003fd2efaa725a857e269a15f765791a x86: MMIO and gcc re-ordering issue
:::::: TO: Linus Torvalds <torvalds@linux-foundation.org>
:::::: CC: Ingo Molnar <mingo@elte.hu>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22579 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 14:54 ` kbuild test robot
0 siblings, 0 replies; 52+ messages in thread
From: kbuild test robot @ 2015-12-10 14:54 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild-all, Wolfram Sang, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5094 bytes --]
Hi Arnd,
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on next-20151210]
[cannot apply to v4.4-rc4]
url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/i2c-allow-building-emev2-without-slave-mode-again/20151210-211642
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux i2c/for-next
config: x86_64-randconfig-r0-12102217 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
In file included from arch/x86/include/asm/realmode.h:5:0,
from arch/x86/include/asm/acpi.h:33,
from arch/x86/include/asm/fixmap.h:19,
from arch/x86/include/asm/apic.h:12,
from arch/x86/include/asm/smp.h:12,
from arch/x86/include/asm/mmzone_64.h:10,
from arch/x86/include/asm/mmzone.h:4,
from include/linux/mmzone.h:856,
from include/linux/gfp.h:5,
from include/linux/device.h:29,
from drivers/i2c/busses/i2c-emev2.c:15:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_irq_handler':
>> arch/x86/include/asm/io.h:53:3: warning: 'value' may be used uninitialized in this function [-Wmaybe-uninitialized]
{ asm volatile("mov" size " %0,%1": :reg (val), \
^
drivers/i2c/busses/i2c-emev2.c:232:13: note: 'value' was declared here
u8 status, value;
^
vim +/value +53 arch/x86/include/asm/io.h
b310f381d include/asm-x86/io.h venkatesh.pallipadi@intel.com 2008-03-18 37 #define ARCH_HAS_IOREMAP_WC
d838270e2 arch/x86/include/asm/io.h Toshi Kani 2015-06-04 38 #define ARCH_HAS_IOREMAP_WT
b310f381d include/asm-x86/io.h venkatesh.pallipadi@intel.com 2008-03-18 39
1c5b9069e arch/x86/include/asm/io.h Brian Gerst 2010-02-05 40 #include <linux/string.h>
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 41 #include <linux/compiler.h>
976e8f677 arch/x86/include/asm/io.h Jeremy Fitzhardinge 2009-02-06 42 #include <asm/page.h>
5b7c73e00 arch/x86/include/asm/io.h Mark Salter 2014-04-07 43 #include <asm/early_ioremap.h>
d6472302f arch/x86/include/asm/io.h Stephen Rothwell 2015-06-02 44 #include <asm/pgtable_types.h>
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 45
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 46 #define build_mmio_read(name, size, type, reg, barrier) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 47 static inline type name(const volatile void __iomem *addr) \
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 48 { type ret; asm volatile("mov" size " %1,%0":reg (ret) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 49 :"m" (*(volatile type __force *)addr) barrier); return ret; }
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 50
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 51 #define build_mmio_write(name, size, type, reg, barrier) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 52 static inline void name(type val, volatile void __iomem *addr) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 @53 { asm volatile("mov" size " %0,%1": :reg (val), \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 54 "m" (*(volatile type __force *)addr) barrier); }
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 55
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 56 build_mmio_read(readb, "b", unsigned char, "=q", :"memory")
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 57 build_mmio_read(readw, "w", unsigned short, "=r", :"memory")
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 58 build_mmio_read(readl, "l", unsigned int, "=r", :"memory")
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 59
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 60 build_mmio_read(__readb, "b", unsigned char, "=q", )
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 61 build_mmio_read(__readw, "w", unsigned short, "=r", )
:::::: The code at line 53 was first introduced by commit
:::::: c1f64a58003fd2efaa725a857e269a15f765791a x86: MMIO and gcc re-ordering issue
:::::: TO: Linus Torvalds <torvalds@linux-foundation.org>
:::::: CC: Ingo Molnar <mingo@elte.hu>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22579 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 14:54 ` kbuild test robot
0 siblings, 0 replies; 52+ messages in thread
From: kbuild test robot @ 2015-12-10 14:54 UTC (permalink / raw)
To: linux-arm-kernel
Hi Arnd,
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on next-20151210]
[cannot apply to v4.4-rc4]
url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/i2c-allow-building-emev2-without-slave-mode-again/20151210-211642
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux i2c/for-next
config: x86_64-randconfig-r0-12102217 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
In file included from arch/x86/include/asm/realmode.h:5:0,
from arch/x86/include/asm/acpi.h:33,
from arch/x86/include/asm/fixmap.h:19,
from arch/x86/include/asm/apic.h:12,
from arch/x86/include/asm/smp.h:12,
from arch/x86/include/asm/mmzone_64.h:10,
from arch/x86/include/asm/mmzone.h:4,
from include/linux/mmzone.h:856,
from include/linux/gfp.h:5,
from include/linux/device.h:29,
from drivers/i2c/busses/i2c-emev2.c:15:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_irq_handler':
>> arch/x86/include/asm/io.h:53:3: warning: 'value' may be used uninitialized in this function [-Wmaybe-uninitialized]
{ asm volatile("mov" size " %0,%1": :reg (val), \
^
drivers/i2c/busses/i2c-emev2.c:232:13: note: 'value' was declared here
u8 status, value;
^
vim +/value +53 arch/x86/include/asm/io.h
b310f381d include/asm-x86/io.h venkatesh.pallipadi at intel.com 2008-03-18 37 #define ARCH_HAS_IOREMAP_WC
d838270e2 arch/x86/include/asm/io.h Toshi Kani 2015-06-04 38 #define ARCH_HAS_IOREMAP_WT
b310f381d include/asm-x86/io.h venkatesh.pallipadi at intel.com 2008-03-18 39
1c5b9069e arch/x86/include/asm/io.h Brian Gerst 2010-02-05 40 #include <linux/string.h>
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 41 #include <linux/compiler.h>
976e8f677 arch/x86/include/asm/io.h Jeremy Fitzhardinge 2009-02-06 42 #include <asm/page.h>
5b7c73e00 arch/x86/include/asm/io.h Mark Salter 2014-04-07 43 #include <asm/early_ioremap.h>
d6472302f arch/x86/include/asm/io.h Stephen Rothwell 2015-06-02 44 #include <asm/pgtable_types.h>
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 45
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 46 #define build_mmio_read(name, size, type, reg, barrier) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 47 static inline type name(const volatile void __iomem *addr) \
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 48 { type ret; asm volatile("mov" size " %1,%0":reg (ret) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 49 :"m" (*(volatile type __force *)addr) barrier); return ret; }
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 50
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 51 #define build_mmio_write(name, size, type, reg, barrier) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 52 static inline void name(type val, volatile void __iomem *addr) \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 @53 { asm volatile("mov" size " %0,%1": :reg (val), \
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 54 "m" (*(volatile type __force *)addr) barrier); }
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 55
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 56 build_mmio_read(readb, "b", unsigned char, "=q", :"memory")
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 57 build_mmio_read(readw, "w", unsigned short, "=r", :"memory")
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 58 build_mmio_read(readl, "l", unsigned int, "=r", :"memory")
c1f64a580 include/asm-x86/io.h Linus Torvalds 2008-05-27 59
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 60 build_mmio_read(__readb, "b", unsigned char, "=q", )
1c5b0eb66 include/asm-x86/io.h Mikael Pettersson 2008-08-13 61 build_mmio_read(__readw, "w", unsigned short, "=r", )
:::::: The code at line 53 was first introduced by commit
:::::: c1f64a58003fd2efaa725a857e269a15f765791a x86: MMIO and gcc re-ordering issue
:::::: TO: Linus Torvalds <torvalds@linux-foundation.org>
:::::: CC: Ingo Molnar <mingo@elte.hu>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 22579 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151210/0b651764/attachment-0001.obj>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-10 14:54 ` kbuild test robot
(?)
(?)
@ 2015-12-10 15:06 ` Arnd Bergmann
-1 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 15:06 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 10 December 2015 22:54:25 kbuild test robot wrote:
>
> In file included from arch/x86/include/asm/realmode.h:5:0,
> from arch/x86/include/asm/acpi.h:33,
> from arch/x86/include/asm/fixmap.h:19,
> from arch/x86/include/asm/apic.h:12,
> from arch/x86/include/asm/smp.h:12,
> from arch/x86/include/asm/mmzone_64.h:10,
> from arch/x86/include/asm/mmzone.h:4,
> from include/linux/mmzone.h:856,
> from include/linux/gfp.h:5,
> from include/linux/device.h:29,
> from drivers/i2c/busses/i2c-emev2.c:15:
> drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_irq_handler':
> >> arch/x86/include/asm/io.h:53:3: warning: 'value' may be used uninitialized in this function [-Wmaybe-uninitialized]
> { asm volatile("mov" size " %0,%1": :reg (val), \
> ^
> drivers/i2c/busses/i2c-emev2.c:232:13: note: 'value' was declared here
> u8 status, value;
> ^
The warning was indeed introduced by my change, but I think there
was a preexisting issue:
The slave_cb callback function is supposed to set the 'value'
here, but it might return an error not assign the pointer, which
is something that both the rcar and the emev2 drivers do not handle
correctly.
It might be best to change the callback to return 'void' and not
allow it to fail. At least the eeprom slave cannot fail anyway,
and it is the only implementation we have at the moment.
The inline function would then have to be changed to initialize
the 'value'.
Alternatively, the inline could return an error, and both bus
drivers check for the error before using 'value'.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 15:06 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 15:06 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, Wolfram Sang, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
On Thursday 10 December 2015 22:54:25 kbuild test robot wrote:
>
> In file included from arch/x86/include/asm/realmode.h:5:0,
> from arch/x86/include/asm/acpi.h:33,
> from arch/x86/include/asm/fixmap.h:19,
> from arch/x86/include/asm/apic.h:12,
> from arch/x86/include/asm/smp.h:12,
> from arch/x86/include/asm/mmzone_64.h:10,
> from arch/x86/include/asm/mmzone.h:4,
> from include/linux/mmzone.h:856,
> from include/linux/gfp.h:5,
> from include/linux/device.h:29,
> from drivers/i2c/busses/i2c-emev2.c:15:
> drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_irq_handler':
> >> arch/x86/include/asm/io.h:53:3: warning: 'value' may be used uninitialized in this function [-Wmaybe-uninitialized]
> { asm volatile("mov" size " %0,%1": :reg (val), \
> ^
> drivers/i2c/busses/i2c-emev2.c:232:13: note: 'value' was declared here
> u8 status, value;
> ^
The warning was indeed introduced by my change, but I think there
was a preexisting issue:
The slave_cb callback function is supposed to set the 'value'
here, but it might return an error not assign the pointer, which
is something that both the rcar and the emev2 drivers do not handle
correctly.
It might be best to change the callback to return 'void' and not
allow it to fail. At least the eeprom slave cannot fail anyway,
and it is the only implementation we have at the moment.
The inline function would then have to be changed to initialize
the 'value'.
Alternatively, the inline could return an error, and both bus
drivers check for the error before using 'value'.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 15:06 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 15:06 UTC (permalink / raw)
To: kbuild test robot
Cc: Niklas Söderlund, linux-sh, Wolfram Sang, linux-kernel,
linux-i2c, linux-arm-kernel, kbuild-all
On Thursday 10 December 2015 22:54:25 kbuild test robot wrote:
>
> In file included from arch/x86/include/asm/realmode.h:5:0,
> from arch/x86/include/asm/acpi.h:33,
> from arch/x86/include/asm/fixmap.h:19,
> from arch/x86/include/asm/apic.h:12,
> from arch/x86/include/asm/smp.h:12,
> from arch/x86/include/asm/mmzone_64.h:10,
> from arch/x86/include/asm/mmzone.h:4,
> from include/linux/mmzone.h:856,
> from include/linux/gfp.h:5,
> from include/linux/device.h:29,
> from drivers/i2c/busses/i2c-emev2.c:15:
> drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_irq_handler':
> >> arch/x86/include/asm/io.h:53:3: warning: 'value' may be used uninitialized in this function [-Wmaybe-uninitialized]
> { asm volatile("mov" size " %0,%1": :reg (val), \
> ^
> drivers/i2c/busses/i2c-emev2.c:232:13: note: 'value' was declared here
> u8 status, value;
> ^
The warning was indeed introduced by my change, but I think there
was a preexisting issue:
The slave_cb callback function is supposed to set the 'value'
here, but it might return an error not assign the pointer, which
is something that both the rcar and the emev2 drivers do not handle
correctly.
It might be best to change the callback to return 'void' and not
allow it to fail. At least the eeprom slave cannot fail anyway,
and it is the only implementation we have at the moment.
The inline function would then have to be changed to initialize
the 'value'.
Alternatively, the inline could return an error, and both bus
drivers check for the error before using 'value'.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 15:06 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-10 15:06 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 10 December 2015 22:54:25 kbuild test robot wrote:
>
> In file included from arch/x86/include/asm/realmode.h:5:0,
> from arch/x86/include/asm/acpi.h:33,
> from arch/x86/include/asm/fixmap.h:19,
> from arch/x86/include/asm/apic.h:12,
> from arch/x86/include/asm/smp.h:12,
> from arch/x86/include/asm/mmzone_64.h:10,
> from arch/x86/include/asm/mmzone.h:4,
> from include/linux/mmzone.h:856,
> from include/linux/gfp.h:5,
> from include/linux/device.h:29,
> from drivers/i2c/busses/i2c-emev2.c:15:
> drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_irq_handler':
> >> arch/x86/include/asm/io.h:53:3: warning: 'value' may be used uninitialized in this function [-Wmaybe-uninitialized]
> { asm volatile("mov" size " %0,%1": :reg (val), \
> ^
> drivers/i2c/busses/i2c-emev2.c:232:13: note: 'value' was declared here
> u8 status, value;
> ^
The warning was indeed introduced by my change, but I think there
was a preexisting issue:
The slave_cb callback function is supposed to set the 'value'
here, but it might return an error not assign the pointer, which
is something that both the rcar and the emev2 drivers do not handle
correctly.
It might be best to change the callback to return 'void' and not
allow it to fail. At least the eeprom slave cannot fail anyway,
and it is the only implementation we have at the moment.
The inline function would then have to be changed to initialize
the 'value'.
Alternatively, the inline could return an error, and both bus
drivers check for the error before using 'value'.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-10 15:06 ` Arnd Bergmann
(?)
@ 2015-12-10 15:17 ` Wolfram Sang
-1 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-10 15:17 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 161 bytes --]
> Alternatively, the inline could return an error, and both bus
> drivers check for the error before using 'value'.
I'll try to check these options tomorrow.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 15:17 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-10 15:17 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 161 bytes --]
> Alternatively, the inline could return an error, and both bus
> drivers check for the error before using 'value'.
I'll try to check these options tomorrow.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-10 15:17 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-10 15:17 UTC (permalink / raw)
To: linux-arm-kernel
> Alternatively, the inline could return an error, and both bus
> drivers check for the error before using 'value'.
I'll try to check these options tomorrow.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151210/7fcd5b3c/attachment.sig>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-10 15:06 ` Arnd Bergmann
(?)
@ 2015-12-12 16:20 ` Wolfram Sang
-1 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-12 16:20 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 964 bytes --]
Hi Arnd,
thanks for looking into this, but I don't get your point yet.
> The slave_cb callback function is supposed to set the 'value'
> here,
Only if a master wants to READ from us.
> but it might return an error not assign the pointer,
An error is only returned if a WRITE from a master was not accepted by
the slave backend.
> It might be best to change the callback to return 'void' and not
> allow it to fail.
We need that because in case of an errno, the slave should send NACK to
the master instead of ACK.
> At least the eeprom slave cannot fail anyway, and it is the only
> implementation we have at the moment.
True. But giving a slave the possibility to NACK a write should be
present IMO.
> Alternatively, the inline could return an error, and both bus
> drivers check for the error before using 'value'.
Hum, it does return an error?
return client->slave_cb(client, event, val);
You probably mean something else?
Regards,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-12 16:20 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-12 16:20 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 964 bytes --]
Hi Arnd,
thanks for looking into this, but I don't get your point yet.
> The slave_cb callback function is supposed to set the 'value'
> here,
Only if a master wants to READ from us.
> but it might return an error not assign the pointer,
An error is only returned if a WRITE from a master was not accepted by
the slave backend.
> It might be best to change the callback to return 'void' and not
> allow it to fail.
We need that because in case of an errno, the slave should send NACK to
the master instead of ACK.
> At least the eeprom slave cannot fail anyway, and it is the only
> implementation we have at the moment.
True. But giving a slave the possibility to NACK a write should be
present IMO.
> Alternatively, the inline could return an error, and both bus
> drivers check for the error before using 'value'.
Hum, it does return an error?
return client->slave_cb(client, event, val);
You probably mean something else?
Regards,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-12 16:20 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-12 16:20 UTC (permalink / raw)
To: linux-arm-kernel
Hi Arnd,
thanks for looking into this, but I don't get your point yet.
> The slave_cb callback function is supposed to set the 'value'
> here,
Only if a master wants to READ from us.
> but it might return an error not assign the pointer,
An error is only returned if a WRITE from a master was not accepted by
the slave backend.
> It might be best to change the callback to return 'void' and not
> allow it to fail.
We need that because in case of an errno, the slave should send NACK to
the master instead of ACK.
> At least the eeprom slave cannot fail anyway, and it is the only
> implementation we have at the moment.
True. But giving a slave the possibility to NACK a write should be
present IMO.
> Alternatively, the inline could return an error, and both bus
> drivers check for the error before using 'value'.
Hum, it does return an error?
return client->slave_cb(client, event, val);
You probably mean something else?
Regards,
Wolfram
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151212/232493e2/attachment.sig>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-12 16:20 ` Wolfram Sang
(?)
(?)
@ 2015-12-12 21:05 ` Arnd Bergmann
-1 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-12 21:05 UTC (permalink / raw)
To: linux-arm-kernel
On Saturday 12 December 2015 17:20:57 Wolfram Sang wrote:
> Hi Arnd,
>
> thanks for looking into this, but I don't get your point yet.
>
> > The slave_cb callback function is supposed to set the 'value'
> > here,
>
> Only if a master wants to READ from us.
Right, and can this never fail?
> > but it might return an error not assign the pointer,
>
> An error is only returned if a WRITE from a master was not accepted by
> the slave backend.
>
> > It might be best to change the callback to return 'void' and not
> > allow it to fail.
>
> We need that because in case of an errno, the slave should send NACK to
> the master instead of ACK.
>
> > At least the eeprom slave cannot fail anyway, and it is the only
> > implementation we have at the moment.
>
> True. But giving a slave the possibility to NACK a write should be
> present IMO.
Ok, fair enough.
> > Alternatively, the inline could return an error, and both bus
> > drivers check for the error before using 'value'.
>
> Hum, it does return an error?
>
> return client->slave_cb(client, event, val);
>
> You probably mean something else?
I mean specifically this code in em_i2c_slave_irq():
/* Send data */
event = status & I2C_BIT_STD0 ?
I2C_SLAVE_READ_REQUESTED :
I2C_SLAVE_READ_PROCESSED;
i2c_slave_event(priv->slave, event, &value);
writeb(value, priv->base + I2C_OFS_IIC0);
With my current code to turn i2c_slave_event() into an empty inline function
in case of !CONFIG_I2C_SLAVE, the compiler knows that "value" is uninitialized
at the point where we write it to the register, and warns about it.
The code will of course never run if slave mode is not allowed, but we should
still shut up the warning, either by making the inline i2c_slave_event
set 'value' to zero or 0xff, or by adding an error check:
ret = i2c_slave_event(priv->slave, event, &value);
if (!ret)
writeb(value, priv->base + I2C_OFS_IIC0);
and making the empty i2c_slave_event() function return -ENOSYS or -ENOTSUPP.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-12 21:05 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-12 21:05 UTC (permalink / raw)
To: Wolfram Sang
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
On Saturday 12 December 2015 17:20:57 Wolfram Sang wrote:
> Hi Arnd,
>
> thanks for looking into this, but I don't get your point yet.
>
> > The slave_cb callback function is supposed to set the 'value'
> > here,
>
> Only if a master wants to READ from us.
Right, and can this never fail?
> > but it might return an error not assign the pointer,
>
> An error is only returned if a WRITE from a master was not accepted by
> the slave backend.
>
> > It might be best to change the callback to return 'void' and not
> > allow it to fail.
>
> We need that because in case of an errno, the slave should send NACK to
> the master instead of ACK.
>
> > At least the eeprom slave cannot fail anyway, and it is the only
> > implementation we have at the moment.
>
> True. But giving a slave the possibility to NACK a write should be
> present IMO.
Ok, fair enough.
> > Alternatively, the inline could return an error, and both bus
> > drivers check for the error before using 'value'.
>
> Hum, it does return an error?
>
> return client->slave_cb(client, event, val);
>
> You probably mean something else?
I mean specifically this code in em_i2c_slave_irq():
/* Send data */
event = status & I2C_BIT_STD0 ?
I2C_SLAVE_READ_REQUESTED :
I2C_SLAVE_READ_PROCESSED;
i2c_slave_event(priv->slave, event, &value);
writeb(value, priv->base + I2C_OFS_IIC0);
With my current code to turn i2c_slave_event() into an empty inline function
in case of !CONFIG_I2C_SLAVE, the compiler knows that "value" is uninitialized
at the point where we write it to the register, and warns about it.
The code will of course never run if slave mode is not allowed, but we should
still shut up the warning, either by making the inline i2c_slave_event
set 'value' to zero or 0xff, or by adding an error check:
ret = i2c_slave_event(priv->slave, event, &value);
if (!ret)
writeb(value, priv->base + I2C_OFS_IIC0);
and making the empty i2c_slave_event() function return -ENOSYS or -ENOTSUPP.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-12 21:05 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-12 21:05 UTC (permalink / raw)
To: Wolfram Sang
Cc: Niklas Söderlund, kbuild test robot, linux-sh, linux-kernel,
linux-i2c, linux-arm-kernel, kbuild-all
On Saturday 12 December 2015 17:20:57 Wolfram Sang wrote:
> Hi Arnd,
>
> thanks for looking into this, but I don't get your point yet.
>
> > The slave_cb callback function is supposed to set the 'value'
> > here,
>
> Only if a master wants to READ from us.
Right, and can this never fail?
> > but it might return an error not assign the pointer,
>
> An error is only returned if a WRITE from a master was not accepted by
> the slave backend.
>
> > It might be best to change the callback to return 'void' and not
> > allow it to fail.
>
> We need that because in case of an errno, the slave should send NACK to
> the master instead of ACK.
>
> > At least the eeprom slave cannot fail anyway, and it is the only
> > implementation we have at the moment.
>
> True. But giving a slave the possibility to NACK a write should be
> present IMO.
Ok, fair enough.
> > Alternatively, the inline could return an error, and both bus
> > drivers check for the error before using 'value'.
>
> Hum, it does return an error?
>
> return client->slave_cb(client, event, val);
>
> You probably mean something else?
I mean specifically this code in em_i2c_slave_irq():
/* Send data */
event = status & I2C_BIT_STD0 ?
I2C_SLAVE_READ_REQUESTED :
I2C_SLAVE_READ_PROCESSED;
i2c_slave_event(priv->slave, event, &value);
writeb(value, priv->base + I2C_OFS_IIC0);
With my current code to turn i2c_slave_event() into an empty inline function
in case of !CONFIG_I2C_SLAVE, the compiler knows that "value" is uninitialized
at the point where we write it to the register, and warns about it.
The code will of course never run if slave mode is not allowed, but we should
still shut up the warning, either by making the inline i2c_slave_event
set 'value' to zero or 0xff, or by adding an error check:
ret = i2c_slave_event(priv->slave, event, &value);
if (!ret)
writeb(value, priv->base + I2C_OFS_IIC0);
and making the empty i2c_slave_event() function return -ENOSYS or -ENOTSUPP.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-12 21:05 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-12 21:05 UTC (permalink / raw)
To: linux-arm-kernel
On Saturday 12 December 2015 17:20:57 Wolfram Sang wrote:
> Hi Arnd,
>
> thanks for looking into this, but I don't get your point yet.
>
> > The slave_cb callback function is supposed to set the 'value'
> > here,
>
> Only if a master wants to READ from us.
Right, and can this never fail?
> > but it might return an error not assign the pointer,
>
> An error is only returned if a WRITE from a master was not accepted by
> the slave backend.
>
> > It might be best to change the callback to return 'void' and not
> > allow it to fail.
>
> We need that because in case of an errno, the slave should send NACK to
> the master instead of ACK.
>
> > At least the eeprom slave cannot fail anyway, and it is the only
> > implementation we have at the moment.
>
> True. But giving a slave the possibility to NACK a write should be
> present IMO.
Ok, fair enough.
> > Alternatively, the inline could return an error, and both bus
> > drivers check for the error before using 'value'.
>
> Hum, it does return an error?
>
> return client->slave_cb(client, event, val);
>
> You probably mean something else?
I mean specifically this code in em_i2c_slave_irq():
/* Send data */
event = status & I2C_BIT_STD0 ?
I2C_SLAVE_READ_REQUESTED :
I2C_SLAVE_READ_PROCESSED;
i2c_slave_event(priv->slave, event, &value);
writeb(value, priv->base + I2C_OFS_IIC0);
With my current code to turn i2c_slave_event() into an empty inline function
in case of !CONFIG_I2C_SLAVE, the compiler knows that "value" is uninitialized
at the point where we write it to the register, and warns about it.
The code will of course never run if slave mode is not allowed, but we should
still shut up the warning, either by making the inline i2c_slave_event
set 'value' to zero or 0xff, or by adding an error check:
ret = i2c_slave_event(priv->slave, event, &value);
if (!ret)
writeb(value, priv->base + I2C_OFS_IIC0);
and making the empty i2c_slave_event() function return -ENOSYS or -ENOTSUPP.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-12 21:05 ` Arnd Bergmann
(?)
@ 2015-12-13 9:09 ` Wolfram Sang
-1 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-13 9:09 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 800 bytes --]
> > > The slave_cb callback function is supposed to set the 'value'
> > > here,
> >
> > Only if a master wants to READ from us.
>
> Right, and can this never fail?
Exactly. The slave can stretch the clock if the value to be sent to the
master needs some processing first, but it must deliver something.
> With my current code to turn i2c_slave_event() into an empty inline function
> in case of !CONFIG_I2C_SLAVE, the compiler knows that "value" is uninitialized
> at the point where we write it to the register, and warns about it.
I overlooked that your first patch changed this code, sorry. Now I see
the problem.
What about not ifdeffing the inline function and keep the build error
whenever someone uses it without I2C_SLAVE being selected?
Thanks,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-13 9:09 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-13 9:09 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 800 bytes --]
> > > The slave_cb callback function is supposed to set the 'value'
> > > here,
> >
> > Only if a master wants to READ from us.
>
> Right, and can this never fail?
Exactly. The slave can stretch the clock if the value to be sent to the
master needs some processing first, but it must deliver something.
> With my current code to turn i2c_slave_event() into an empty inline function
> in case of !CONFIG_I2C_SLAVE, the compiler knows that "value" is uninitialized
> at the point where we write it to the register, and warns about it.
I overlooked that your first patch changed this code, sorry. Now I see
the problem.
What about not ifdeffing the inline function and keep the build error
whenever someone uses it without I2C_SLAVE being selected?
Thanks,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-13 9:09 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-13 9:09 UTC (permalink / raw)
To: linux-arm-kernel
> > > The slave_cb callback function is supposed to set the 'value'
> > > here,
> >
> > Only if a master wants to READ from us.
>
> Right, and can this never fail?
Exactly. The slave can stretch the clock if the value to be sent to the
master needs some processing first, but it must deliver something.
> With my current code to turn i2c_slave_event() into an empty inline function
> in case of !CONFIG_I2C_SLAVE, the compiler knows that "value" is uninitialized
> at the point where we write it to the register, and warns about it.
I overlooked that your first patch changed this code, sorry. Now I see
the problem.
What about not ifdeffing the inline function and keep the build error
whenever someone uses it without I2C_SLAVE being selected?
Thanks,
Wolfram
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151213/837a8f13/attachment.sig>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-13 9:09 ` Wolfram Sang
(?)
@ 2015-12-14 12:02 ` Arnd Bergmann
-1 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-14 12:02 UTC (permalink / raw)
To: linux-arm-kernel
On Sunday 13 December 2015 10:09:59 Wolfram Sang wrote:
>
> What about not ifdeffing the inline function and keep the build error
> whenever someone uses it without I2C_SLAVE being selected?
The inline function is only added there for the case that I2C_SLAVE is
disabled, so that would be pointless.
However, what we could do is move the extern declaration outside of
the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
check should then ensure that it never actually gets called, and we
get a link error if some driver gets it wrong.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-14 12:02 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-14 12:02 UTC (permalink / raw)
To: Wolfram Sang
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
On Sunday 13 December 2015 10:09:59 Wolfram Sang wrote:
>
> What about not ifdeffing the inline function and keep the build error
> whenever someone uses it without I2C_SLAVE being selected?
The inline function is only added there for the case that I2C_SLAVE is
disabled, so that would be pointless.
However, what we could do is move the extern declaration outside of
the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
check should then ensure that it never actually gets called, and we
get a link error if some driver gets it wrong.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-14 12:02 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-14 12:02 UTC (permalink / raw)
To: linux-arm-kernel
On Sunday 13 December 2015 10:09:59 Wolfram Sang wrote:
>
> What about not ifdeffing the inline function and keep the build error
> whenever someone uses it without I2C_SLAVE being selected?
The inline function is only added there for the case that I2C_SLAVE is
disabled, so that would be pointless.
However, what we could do is move the extern declaration outside of
the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
check should then ensure that it never actually gets called, and we
get a link error if some driver gets it wrong.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-14 12:02 ` Arnd Bergmann
(?)
@ 2015-12-14 13:52 ` Wolfram Sang
-1 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-14 13:52 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]
> > What about not ifdeffing the inline function and keep the build error
> > whenever someone uses it without I2C_SLAVE being selected?
>
> The inline function is only added there for the case that I2C_SLAVE is
> disabled, so that would be pointless.
>
> However, what we could do is move the extern declaration outside of
> the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> check should then ensure that it never actually gets called, and we
> get a link error if some driver gets it wrong.
Yes, that's what I meant: move the whole function (as it was before your
patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
even, because for !I2C_SLAVE, the client struct will not have the
slave_cb member.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-14 13:52 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-14 13:52 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]
> > What about not ifdeffing the inline function and keep the build error
> > whenever someone uses it without I2C_SLAVE being selected?
>
> The inline function is only added there for the case that I2C_SLAVE is
> disabled, so that would be pointless.
>
> However, what we could do is move the extern declaration outside of
> the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> check should then ensure that it never actually gets called, and we
> get a link error if some driver gets it wrong.
Yes, that's what I meant: move the whole function (as it was before your
patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
even, because for !I2C_SLAVE, the client struct will not have the
slave_cb member.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-14 13:52 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-14 13:52 UTC (permalink / raw)
To: linux-arm-kernel
> > What about not ifdeffing the inline function and keep the build error
> > whenever someone uses it without I2C_SLAVE being selected?
>
> The inline function is only added there for the case that I2C_SLAVE is
> disabled, so that would be pointless.
>
> However, what we could do is move the extern declaration outside of
> the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> check should then ensure that it never actually gets called, and we
> get a link error if some driver gets it wrong.
Yes, that's what I meant: move the whole function (as it was before your
patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
even, because for !I2C_SLAVE, the client struct will not have the
slave_cb member.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151214/f70353ff/attachment-0001.sig>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-14 13:52 ` Wolfram Sang
(?)
@ 2015-12-14 22:27 ` Arnd Bergmann
-1 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-14 22:27 UTC (permalink / raw)
To: linux-arm-kernel
On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > What about not ifdeffing the inline function and keep the build error
> > > whenever someone uses it without I2C_SLAVE being selected?
> >
> > The inline function is only added there for the case that I2C_SLAVE is
> > disabled, so that would be pointless.
> >
> > However, what we could do is move the extern declaration outside of
> > the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > check should then ensure that it never actually gets called, and we
> > get a link error if some driver gets it wrong.
>
> Yes, that's what I meant: move the whole function (as it was before your
> patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> even, because for !I2C_SLAVE, the client struct will not have the
> slave_cb member.
>
But we don't want a compile-error for randconfig builds, and we don't
want unnecessary #ifdef in the driver.
This change on top of my earlier patch should do what I meant:
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0236e5f2b5be..536641bad92d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -265,15 +265,15 @@ enum i2c_slave_event {
extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
{
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
return client->slave_cb(client, event, val);
+}
#else
- return 0;
+extern int i2c_slave_event(struct i2c_client *client, enum i2c_slave_event event, u8 *val);
#endif
-}
/**
* struct i2c_board_info - template for device creation
Arnd
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-14 22:27 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-14 22:27 UTC (permalink / raw)
To: Wolfram Sang
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > What about not ifdeffing the inline function and keep the build error
> > > whenever someone uses it without I2C_SLAVE being selected?
> >
> > The inline function is only added there for the case that I2C_SLAVE is
> > disabled, so that would be pointless.
> >
> > However, what we could do is move the extern declaration outside of
> > the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > check should then ensure that it never actually gets called, and we
> > get a link error if some driver gets it wrong.
>
> Yes, that's what I meant: move the whole function (as it was before your
> patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> even, because for !I2C_SLAVE, the client struct will not have the
> slave_cb member.
>
But we don't want a compile-error for randconfig builds, and we don't
want unnecessary #ifdef in the driver.
This change on top of my earlier patch should do what I meant:
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0236e5f2b5be..536641bad92d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -265,15 +265,15 @@ enum i2c_slave_event {
extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
{
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
return client->slave_cb(client, event, val);
+}
#else
- return 0;
+extern int i2c_slave_event(struct i2c_client *client, enum i2c_slave_event event, u8 *val);
#endif
-}
/**
* struct i2c_board_info - template for device creation
Arnd
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-14 22:27 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-14 22:27 UTC (permalink / raw)
To: linux-arm-kernel
On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > What about not ifdeffing the inline function and keep the build error
> > > whenever someone uses it without I2C_SLAVE being selected?
> >
> > The inline function is only added there for the case that I2C_SLAVE is
> > disabled, so that would be pointless.
> >
> > However, what we could do is move the extern declaration outside of
> > the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > check should then ensure that it never actually gets called, and we
> > get a link error if some driver gets it wrong.
>
> Yes, that's what I meant: move the whole function (as it was before your
> patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> even, because for !I2C_SLAVE, the client struct will not have the
> slave_cb member.
>
But we don't want a compile-error for randconfig builds, and we don't
want unnecessary #ifdef in the driver.
This change on top of my earlier patch should do what I meant:
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0236e5f2b5be..536641bad92d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -265,15 +265,15 @@ enum i2c_slave_event {
extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
{
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
return client->slave_cb(client, event, val);
+}
#else
- return 0;
+extern int i2c_slave_event(struct i2c_client *client, enum i2c_slave_event event, u8 *val);
#endif
-}
/**
* struct i2c_board_info - template for device creation
Arnd
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-14 22:27 ` Arnd Bergmann
(?)
@ 2015-12-17 12:01 ` Wolfram Sang
-1 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-17 12:01 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]
On Mon, Dec 14, 2015 at 11:27:22PM +0100, Arnd Bergmann wrote:
> On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > > What about not ifdeffing the inline function and keep the build error
> > > > whenever someone uses it without I2C_SLAVE being selected?
> > >
> > > The inline function is only added there for the case that I2C_SLAVE is
> > > disabled, so that would be pointless.
> > >
> > > However, what we could do is move the extern declaration outside of
> > > the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > > check should then ensure that it never actually gets called, and we
> > > get a link error if some driver gets it wrong.
> >
> > Yes, that's what I meant: move the whole function (as it was before your
> > patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> > even, because for !I2C_SLAVE, the client struct will not have the
> > slave_cb member.
> >
>
> But we don't want a compile-error for randconfig builds, and we don't
> want unnecessary #ifdef in the driver.
My conclusion for now is:
There needs something to be done surely, but currently I don't have the
bandwidth to do it or even play around with it. I am not fully happy
with your patches as well because __maybe_unused has some kind of "last
resort" feeling to me.
So, to get the build failures away immediately I'd simply submit a patch
for the emev driver to select I2C_SLAVE and postpone the proper fix to
later.
That being said, thanks a lot for your input. I will surely come back to it.
All the best,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-17 12:01 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-17 12:01 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]
On Mon, Dec 14, 2015 at 11:27:22PM +0100, Arnd Bergmann wrote:
> On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > > What about not ifdeffing the inline function and keep the build error
> > > > whenever someone uses it without I2C_SLAVE being selected?
> > >
> > > The inline function is only added there for the case that I2C_SLAVE is
> > > disabled, so that would be pointless.
> > >
> > > However, what we could do is move the extern declaration outside of
> > > the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > > check should then ensure that it never actually gets called, and we
> > > get a link error if some driver gets it wrong.
> >
> > Yes, that's what I meant: move the whole function (as it was before your
> > patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> > even, because for !I2C_SLAVE, the client struct will not have the
> > slave_cb member.
> >
>
> But we don't want a compile-error for randconfig builds, and we don't
> want unnecessary #ifdef in the driver.
My conclusion for now is:
There needs something to be done surely, but currently I don't have the
bandwidth to do it or even play around with it. I am not fully happy
with your patches as well because __maybe_unused has some kind of "last
resort" feeling to me.
So, to get the build failures away immediately I'd simply submit a patch
for the emev driver to select I2C_SLAVE and postpone the proper fix to
later.
That being said, thanks a lot for your input. I will surely come back to it.
All the best,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-17 12:01 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-17 12:01 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Dec 14, 2015 at 11:27:22PM +0100, Arnd Bergmann wrote:
> On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > > What about not ifdeffing the inline function and keep the build error
> > > > whenever someone uses it without I2C_SLAVE being selected?
> > >
> > > The inline function is only added there for the case that I2C_SLAVE is
> > > disabled, so that would be pointless.
> > >
> > > However, what we could do is move the extern declaration outside of
> > > the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > > check should then ensure that it never actually gets called, and we
> > > get a link error if some driver gets it wrong.
> >
> > Yes, that's what I meant: move the whole function (as it was before your
> > patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> > even, because for !I2C_SLAVE, the client struct will not have the
> > slave_cb member.
> >
>
> But we don't want a compile-error for randconfig builds, and we don't
> want unnecessary #ifdef in the driver.
My conclusion for now is:
There needs something to be done surely, but currently I don't have the
bandwidth to do it or even play around with it. I am not fully happy
with your patches as well because __maybe_unused has some kind of "last
resort" feeling to me.
So, to get the build failures away immediately I'd simply submit a patch
for the emev driver to select I2C_SLAVE and postpone the proper fix to
later.
That being said, thanks a lot for your input. I will surely come back to it.
All the best,
Wolfram
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151217/3aedb5c3/attachment.sig>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-17 12:01 ` Wolfram Sang
(?)
@ 2015-12-17 14:48 ` Arnd Bergmann
-1 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-17 14:48 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 17 December 2015 13:01:57 Wolfram Sang wrote:
> On Mon, Dec 14, 2015 at 11:27:22PM +0100, Arnd Bergmann wrote:
> > On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > > > What about not ifdeffing the inline function and keep the build error
> > > > > whenever someone uses it without I2C_SLAVE being selected?
> > > >
> > > > The inline function is only added there for the case that I2C_SLAVE is
> > > > disabled, so that would be pointless.
> > > >
> > > > However, what we could do is move the extern declaration outside of
> > > > the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > > > check should then ensure that it never actually gets called, and we
> > > > get a link error if some driver gets it wrong.
> > >
> > > Yes, that's what I meant: move the whole function (as it was before your
> > > patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> > > even, because for !I2C_SLAVE, the client struct will not have the
> > > slave_cb member.
> > >
> >
> > But we don't want a compile-error for randconfig builds, and we don't
> > want unnecessary #ifdef in the driver.
>
> My conclusion for now is:
>
> There needs something to be done surely, but currently I don't have the
> bandwidth to do it or even play around with it. I am not fully happy
> with your patches as well because __maybe_unused has some kind of "last
> resort" feeling to me.
I generally like __maybe_unused, but it's a matter of personal preference.
We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
pointers are always available in struct i2c_algorithm.
> So, to get the build failures away immediately I'd simply submit a patch
> for the emev driver to select I2C_SLAVE and postpone the proper fix to
> later.
>
> That being said, thanks a lot for your input. I will surely come back to it.
Just for reference, see below for my combined patch, in case you decide
to use that at a later point.
Arnd
8<---
Subject: [PATCH] i2c: emev2 depends on i2c slave mode
The emev2 driver stopped compiling in today's linux-next kernel:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
to add a dependency on that symbol:
* The symbol is user-selectable, but only one or two (including this
one) bus drivers actually implement it, and it makes no sense
if you don't have one of them.
* The other driver (R-Car) uses 'select I2C_SLAVE', which seems
reasonable in principle, but we should not do that on user
visible symbols.
* I2C slave mode could be implemented in a lot of other drivers
as an optional feature, but we shouldn't require enabling it
if we don't use it.
This changes the two drivers that provide I2C slave mode so they
can again build if the slave mode being disabled. To do this, I
move the definition of i2c_slave_event() and enum i2c_slave_event
out of the #ifdef and instead make the assignment of the reg_slave
and unreg_slave pointers optional in the bus drivers. The functions
implementing the feature are unused in that case, so they get
marked as __maybe_unused in order to still give compile-time
coverage.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: c31d0a00021d ("i2c: emev2: add slave support")
---
drivers/i2c/busses/Kconfig | 1 -
drivers/i2c/busses/i2c-emev2.c | 8 +++++---
drivers/i2c/busses/i2c-rcar.c | 8 +++++---
include/linux/i2c.h | 4 +++-
4 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index eaa7b4a0e484..f205b9fa7a74 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -985,7 +985,6 @@ config I2C_XLP9XX
config I2C_RCAR
tristate "Renesas R-Car I2C Controller"
depends on ARCH_SHMOBILE || COMPILE_TEST
- select I2C_SLAVE
help
If you say yes to this option, support will be included for the
R-Car I2C controller.
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 96bb4e749012..b01c9f30c545 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -303,7 +303,7 @@ static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
{
struct em_i2c_device *priv = dev_id;
- if (em_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && em_i2c_slave_irq(priv))
return IRQ_HANDLED;
complete(&priv->msg_done);
@@ -316,7 +316,7 @@ static u32 em_i2c_func(struct i2c_adapter *adap)
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
}
-static int em_i2c_reg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_reg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -334,7 +334,7 @@ static int em_i2c_reg_slave(struct i2c_client *slave)
return 0;
}
-static int em_i2c_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_unreg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -350,8 +350,10 @@ static int em_i2c_unreg_slave(struct i2c_client *slave)
static struct i2c_algorithm em_i2c_algo = {
.master_xfer = em_i2c_xfer,
.functionality = em_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = em_i2c_reg_slave,
.unreg_slave = em_i2c_unreg_slave,
+#endif
};
static int em_i2c_probe(struct platform_device *pdev)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index b2389c492579..ab7fa78b8030 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -430,7 +430,7 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
/* Only handle interrupts that are currently enabled */
msr &= rcar_i2c_read(priv, ICMIER);
if (!msr) {
- if (rcar_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && rcar_i2c_slave_irq(priv))
return IRQ_HANDLED;
return IRQ_NONE;
@@ -523,7 +523,7 @@ out:
return ret;
}
-static int rcar_reg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_reg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -544,7 +544,7 @@ static int rcar_reg_slave(struct i2c_client *slave)
return 0;
}
-static int rcar_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_unreg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -570,8 +570,10 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
static const struct i2c_algorithm rcar_i2c_algo = {
.master_xfer = rcar_i2c_master_xfer,
.functionality = rcar_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = rcar_reg_slave,
.unreg_slave = rcar_unreg_slave,
+#endif
};
static const struct of_device_id rcar_i2c_dt_ids[] = {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index bc2b19ad9357..2b0b08b183e0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -254,7 +254,6 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
/* I2C slave support */
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
enum i2c_slave_event {
I2C_SLAVE_READ_REQUESTED,
I2C_SLAVE_WRITE_REQUESTED,
@@ -266,11 +265,14 @@ enum i2c_slave_event {
extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
{
return client->slave_cb(client, event, val);
}
+#else
+extern int i2c_slave_event(struct i2c_client *client, enum i2c_slave_event event, u8 *val);
#endif
/**
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-17 14:48 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-17 14:48 UTC (permalink / raw)
To: Wolfram Sang
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
On Thursday 17 December 2015 13:01:57 Wolfram Sang wrote:
> On Mon, Dec 14, 2015 at 11:27:22PM +0100, Arnd Bergmann wrote:
> > On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > > > What about not ifdeffing the inline function and keep the build error
> > > > > whenever someone uses it without I2C_SLAVE being selected?
> > > >
> > > > The inline function is only added there for the case that I2C_SLAVE is
> > > > disabled, so that would be pointless.
> > > >
> > > > However, what we could do is move the extern declaration outside of
> > > > the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > > > check should then ensure that it never actually gets called, and we
> > > > get a link error if some driver gets it wrong.
> > >
> > > Yes, that's what I meant: move the whole function (as it was before your
> > > patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> > > even, because for !I2C_SLAVE, the client struct will not have the
> > > slave_cb member.
> > >
> >
> > But we don't want a compile-error for randconfig builds, and we don't
> > want unnecessary #ifdef in the driver.
>
> My conclusion for now is:
>
> There needs something to be done surely, but currently I don't have the
> bandwidth to do it or even play around with it. I am not fully happy
> with your patches as well because __maybe_unused has some kind of "last
> resort" feeling to me.
I generally like __maybe_unused, but it's a matter of personal preference.
We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
pointers are always available in struct i2c_algorithm.
> So, to get the build failures away immediately I'd simply submit a patch
> for the emev driver to select I2C_SLAVE and postpone the proper fix to
> later.
>
> That being said, thanks a lot for your input. I will surely come back to it.
Just for reference, see below for my combined patch, in case you decide
to use that at a later point.
Arnd
8<---
Subject: [PATCH] i2c: emev2 depends on i2c slave mode
The emev2 driver stopped compiling in today's linux-next kernel:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
to add a dependency on that symbol:
* The symbol is user-selectable, but only one or two (including this
one) bus drivers actually implement it, and it makes no sense
if you don't have one of them.
* The other driver (R-Car) uses 'select I2C_SLAVE', which seems
reasonable in principle, but we should not do that on user
visible symbols.
* I2C slave mode could be implemented in a lot of other drivers
as an optional feature, but we shouldn't require enabling it
if we don't use it.
This changes the two drivers that provide I2C slave mode so they
can again build if the slave mode being disabled. To do this, I
move the definition of i2c_slave_event() and enum i2c_slave_event
out of the #ifdef and instead make the assignment of the reg_slave
and unreg_slave pointers optional in the bus drivers. The functions
implementing the feature are unused in that case, so they get
marked as __maybe_unused in order to still give compile-time
coverage.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: c31d0a00021d ("i2c: emev2: add slave support")
---
drivers/i2c/busses/Kconfig | 1 -
drivers/i2c/busses/i2c-emev2.c | 8 +++++---
drivers/i2c/busses/i2c-rcar.c | 8 +++++---
include/linux/i2c.h | 4 +++-
4 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index eaa7b4a0e484..f205b9fa7a74 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -985,7 +985,6 @@ config I2C_XLP9XX
config I2C_RCAR
tristate "Renesas R-Car I2C Controller"
depends on ARCH_SHMOBILE || COMPILE_TEST
- select I2C_SLAVE
help
If you say yes to this option, support will be included for the
R-Car I2C controller.
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 96bb4e749012..b01c9f30c545 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -303,7 +303,7 @@ static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
{
struct em_i2c_device *priv = dev_id;
- if (em_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && em_i2c_slave_irq(priv))
return IRQ_HANDLED;
complete(&priv->msg_done);
@@ -316,7 +316,7 @@ static u32 em_i2c_func(struct i2c_adapter *adap)
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
}
-static int em_i2c_reg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_reg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -334,7 +334,7 @@ static int em_i2c_reg_slave(struct i2c_client *slave)
return 0;
}
-static int em_i2c_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_unreg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -350,8 +350,10 @@ static int em_i2c_unreg_slave(struct i2c_client *slave)
static struct i2c_algorithm em_i2c_algo = {
.master_xfer = em_i2c_xfer,
.functionality = em_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = em_i2c_reg_slave,
.unreg_slave = em_i2c_unreg_slave,
+#endif
};
static int em_i2c_probe(struct platform_device *pdev)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index b2389c492579..ab7fa78b8030 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -430,7 +430,7 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
/* Only handle interrupts that are currently enabled */
msr &= rcar_i2c_read(priv, ICMIER);
if (!msr) {
- if (rcar_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && rcar_i2c_slave_irq(priv))
return IRQ_HANDLED;
return IRQ_NONE;
@@ -523,7 +523,7 @@ out:
return ret;
}
-static int rcar_reg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_reg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -544,7 +544,7 @@ static int rcar_reg_slave(struct i2c_client *slave)
return 0;
}
-static int rcar_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_unreg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -570,8 +570,10 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
static const struct i2c_algorithm rcar_i2c_algo = {
.master_xfer = rcar_i2c_master_xfer,
.functionality = rcar_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = rcar_reg_slave,
.unreg_slave = rcar_unreg_slave,
+#endif
};
static const struct of_device_id rcar_i2c_dt_ids[] = {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index bc2b19ad9357..2b0b08b183e0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -254,7 +254,6 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
/* I2C slave support */
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
enum i2c_slave_event {
I2C_SLAVE_READ_REQUESTED,
I2C_SLAVE_WRITE_REQUESTED,
@@ -266,11 +265,14 @@ enum i2c_slave_event {
extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
{
return client->slave_cb(client, event, val);
}
+#else
+extern int i2c_slave_event(struct i2c_client *client, enum i2c_slave_event event, u8 *val);
#endif
/**
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-17 14:48 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-17 14:48 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 17 December 2015 13:01:57 Wolfram Sang wrote:
> On Mon, Dec 14, 2015 at 11:27:22PM +0100, Arnd Bergmann wrote:
> > On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > > > What about not ifdeffing the inline function and keep the build error
> > > > > whenever someone uses it without I2C_SLAVE being selected?
> > > >
> > > > The inline function is only added there for the case that I2C_SLAVE is
> > > > disabled, so that would be pointless.
> > > >
> > > > However, what we could do is move the extern declaration outside of
> > > > the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > > > check should then ensure that it never actually gets called, and we
> > > > get a link error if some driver gets it wrong.
> > >
> > > Yes, that's what I meant: move the whole function (as it was before your
> > > patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> > > even, because for !I2C_SLAVE, the client struct will not have the
> > > slave_cb member.
> > >
> >
> > But we don't want a compile-error for randconfig builds, and we don't
> > want unnecessary #ifdef in the driver.
>
> My conclusion for now is:
>
> There needs something to be done surely, but currently I don't have the
> bandwidth to do it or even play around with it. I am not fully happy
> with your patches as well because __maybe_unused has some kind of "last
> resort" feeling to me.
I generally like __maybe_unused, but it's a matter of personal preference.
We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
pointers are always available in struct i2c_algorithm.
> So, to get the build failures away immediately I'd simply submit a patch
> for the emev driver to select I2C_SLAVE and postpone the proper fix to
> later.
>
> That being said, thanks a lot for your input. I will surely come back to it.
Just for reference, see below for my combined patch, in case you decide
to use that at a later point.
Arnd
8<---
Subject: [PATCH] i2c: emev2 depends on i2c slave mode
The emev2 driver stopped compiling in today's linux-next kernel:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
to add a dependency on that symbol:
* The symbol is user-selectable, but only one or two (including this
one) bus drivers actually implement it, and it makes no sense
if you don't have one of them.
* The other driver (R-Car) uses 'select I2C_SLAVE', which seems
reasonable in principle, but we should not do that on user
visible symbols.
* I2C slave mode could be implemented in a lot of other drivers
as an optional feature, but we shouldn't require enabling it
if we don't use it.
This changes the two drivers that provide I2C slave mode so they
can again build if the slave mode being disabled. To do this, I
move the definition of i2c_slave_event() and enum i2c_slave_event
out of the #ifdef and instead make the assignment of the reg_slave
and unreg_slave pointers optional in the bus drivers. The functions
implementing the feature are unused in that case, so they get
marked as __maybe_unused in order to still give compile-time
coverage.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: c31d0a00021d ("i2c: emev2: add slave support")
---
drivers/i2c/busses/Kconfig | 1 -
drivers/i2c/busses/i2c-emev2.c | 8 +++++---
drivers/i2c/busses/i2c-rcar.c | 8 +++++---
include/linux/i2c.h | 4 +++-
4 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index eaa7b4a0e484..f205b9fa7a74 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -985,7 +985,6 @@ config I2C_XLP9XX
config I2C_RCAR
tristate "Renesas R-Car I2C Controller"
depends on ARCH_SHMOBILE || COMPILE_TEST
- select I2C_SLAVE
help
If you say yes to this option, support will be included for the
R-Car I2C controller.
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 96bb4e749012..b01c9f30c545 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -303,7 +303,7 @@ static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
{
struct em_i2c_device *priv = dev_id;
- if (em_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && em_i2c_slave_irq(priv))
return IRQ_HANDLED;
complete(&priv->msg_done);
@@ -316,7 +316,7 @@ static u32 em_i2c_func(struct i2c_adapter *adap)
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
}
-static int em_i2c_reg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_reg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -334,7 +334,7 @@ static int em_i2c_reg_slave(struct i2c_client *slave)
return 0;
}
-static int em_i2c_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_unreg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -350,8 +350,10 @@ static int em_i2c_unreg_slave(struct i2c_client *slave)
static struct i2c_algorithm em_i2c_algo = {
.master_xfer = em_i2c_xfer,
.functionality = em_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = em_i2c_reg_slave,
.unreg_slave = em_i2c_unreg_slave,
+#endif
};
static int em_i2c_probe(struct platform_device *pdev)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index b2389c492579..ab7fa78b8030 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -430,7 +430,7 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
/* Only handle interrupts that are currently enabled */
msr &= rcar_i2c_read(priv, ICMIER);
if (!msr) {
- if (rcar_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && rcar_i2c_slave_irq(priv))
return IRQ_HANDLED;
return IRQ_NONE;
@@ -523,7 +523,7 @@ out:
return ret;
}
-static int rcar_reg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_reg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -544,7 +544,7 @@ static int rcar_reg_slave(struct i2c_client *slave)
return 0;
}
-static int rcar_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_unreg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -570,8 +570,10 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
static const struct i2c_algorithm rcar_i2c_algo = {
.master_xfer = rcar_i2c_master_xfer,
.functionality = rcar_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = rcar_reg_slave,
.unreg_slave = rcar_unreg_slave,
+#endif
};
static const struct of_device_id rcar_i2c_dt_ids[] = {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index bc2b19ad9357..2b0b08b183e0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -254,7 +254,6 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
/* I2C slave support */
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
enum i2c_slave_event {
I2C_SLAVE_READ_REQUESTED,
I2C_SLAVE_WRITE_REQUESTED,
@@ -266,11 +265,14 @@ enum i2c_slave_event {
extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
{
return client->slave_cb(client, event, val);
}
+#else
+extern int i2c_slave_event(struct i2c_client *client, enum i2c_slave_event event, u8 *val);
#endif
/**
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-17 14:48 ` Arnd Bergmann
(?)
@ 2015-12-17 19:40 ` Wolfram Sang
-1 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-17 19:40 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 744 bytes --]
> > My conclusion for now is:
> >
> > There needs something to be done surely, but currently I don't have the
> > bandwidth to do it or even play around with it. I am not fully happy
> > with your patches as well because __maybe_unused has some kind of "last
> > resort" feeling to me.
>
> I generally like __maybe_unused, but it's a matter of personal preference.
> We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
> pointers are always available in struct i2c_algorithm.
Yes, I was thinking in this direction, looking at how PM does it. Needs
some playing around, though.
> Just for reference, see below for my combined patch, in case you decide
> to use that at a later point.
Thanks a lot!
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-17 19:40 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-17 19:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 744 bytes --]
> > My conclusion for now is:
> >
> > There needs something to be done surely, but currently I don't have the
> > bandwidth to do it or even play around with it. I am not fully happy
> > with your patches as well because __maybe_unused has some kind of "last
> > resort" feeling to me.
>
> I generally like __maybe_unused, but it's a matter of personal preference.
> We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
> pointers are always available in struct i2c_algorithm.
Yes, I was thinking in this direction, looking at how PM does it. Needs
some playing around, though.
> Just for reference, see below for my combined patch, in case you decide
> to use that at a later point.
Thanks a lot!
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-17 19:40 ` Wolfram Sang
0 siblings, 0 replies; 52+ messages in thread
From: Wolfram Sang @ 2015-12-17 19:40 UTC (permalink / raw)
To: linux-arm-kernel
> > My conclusion for now is:
> >
> > There needs something to be done surely, but currently I don't have the
> > bandwidth to do it or even play around with it. I am not fully happy
> > with your patches as well because __maybe_unused has some kind of "last
> > resort" feeling to me.
>
> I generally like __maybe_unused, but it's a matter of personal preference.
> We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
> pointers are always available in struct i2c_algorithm.
Yes, I was thinking in this direction, looking at how PM does it. Needs
some playing around, though.
> Just for reference, see below for my combined patch, in case you decide
> to use that at a later point.
Thanks a lot!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151217/1d5dfb0a/attachment.sig>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
2015-12-17 19:40 ` Wolfram Sang
(?)
(?)
@ 2015-12-17 19:57 ` Arnd Bergmann
-1 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-17 19:57 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 17 December 2015 20:40:17 Wolfram Sang wrote:
> > > My conclusion for now is:
> > >
> > > There needs something to be done surely, but currently I don't have the
> > > bandwidth to do it or even play around with it. I am not fully happy
> > > with your patches as well because __maybe_unused has some kind of "last
> > > resort" feeling to me.
> >
> > I generally like __maybe_unused, but it's a matter of personal preference.
> > We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
> > pointers are always available in struct i2c_algorithm.
>
> Yes, I was thinking in this direction, looking at how PM does it. Needs
> some playing around, though.
I think PM gets it slightly wrong, the way you have to use #ifdef leads
to subtle bugs all the time, and I actually have a patch that converts
a few dozen drivers to use __maybe_unused to shut up build warnings and
errors.
What you can do though is to use a reference like
#define __i2c_slave_ptr(x) (IS_ENABLED(CONFIG_I2C_SLAVE) ? (x) : NULL)
...
.reg_slave = __i2c_slave_ptr(em_i2c_reg_slave),
.unreg_slave = __i2c_slave_ptr(em_i2c_unreg_slave),
...
This has the same effect as the __maybe_unused annotation.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-17 19:57 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-17 19:57 UTC (permalink / raw)
To: Wolfram Sang
Cc: kbuild test robot, kbuild-all, linux-i2c, Niklas Söderlund,
linux-sh, linux-arm-kernel, linux-kernel
On Thursday 17 December 2015 20:40:17 Wolfram Sang wrote:
> > > My conclusion for now is:
> > >
> > > There needs something to be done surely, but currently I don't have the
> > > bandwidth to do it or even play around with it. I am not fully happy
> > > with your patches as well because __maybe_unused has some kind of "last
> > > resort" feeling to me.
> >
> > I generally like __maybe_unused, but it's a matter of personal preference.
> > We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
> > pointers are always available in struct i2c_algorithm.
>
> Yes, I was thinking in this direction, looking at how PM does it. Needs
> some playing around, though.
I think PM gets it slightly wrong, the way you have to use #ifdef leads
to subtle bugs all the time, and I actually have a patch that converts
a few dozen drivers to use __maybe_unused to shut up build warnings and
errors.
What you can do though is to use a reference like
#define __i2c_slave_ptr(x) (IS_ENABLED(CONFIG_I2C_SLAVE) ? (x) : NULL)
...
.reg_slave = __i2c_slave_ptr(em_i2c_reg_slave),
.unreg_slave = __i2c_slave_ptr(em_i2c_unreg_slave),
...
This has the same effect as the __maybe_unused annotation.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-17 19:57 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-17 19:57 UTC (permalink / raw)
To: Wolfram Sang
Cc: Niklas Söderlund, kbuild test robot, linux-sh, linux-kernel,
linux-i2c, linux-arm-kernel, kbuild-all
On Thursday 17 December 2015 20:40:17 Wolfram Sang wrote:
> > > My conclusion for now is:
> > >
> > > There needs something to be done surely, but currently I don't have the
> > > bandwidth to do it or even play around with it. I am not fully happy
> > > with your patches as well because __maybe_unused has some kind of "last
> > > resort" feeling to me.
> >
> > I generally like __maybe_unused, but it's a matter of personal preference.
> > We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
> > pointers are always available in struct i2c_algorithm.
>
> Yes, I was thinking in this direction, looking at how PM does it. Needs
> some playing around, though.
I think PM gets it slightly wrong, the way you have to use #ifdef leads
to subtle bugs all the time, and I actually have a patch that converts
a few dozen drivers to use __maybe_unused to shut up build warnings and
errors.
What you can do though is to use a reference like
#define __i2c_slave_ptr(x) (IS_ENABLED(CONFIG_I2C_SLAVE) ? (x) : NULL)
...
.reg_slave = __i2c_slave_ptr(em_i2c_reg_slave),
.unreg_slave = __i2c_slave_ptr(em_i2c_unreg_slave),
...
This has the same effect as the __maybe_unused annotation.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] i2c: allow building emev2 without slave mode again
@ 2015-12-17 19:57 ` Arnd Bergmann
0 siblings, 0 replies; 52+ messages in thread
From: Arnd Bergmann @ 2015-12-17 19:57 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 17 December 2015 20:40:17 Wolfram Sang wrote:
> > > My conclusion for now is:
> > >
> > > There needs something to be done surely, but currently I don't have the
> > > bandwidth to do it or even play around with it. I am not fully happy
> > > with your patches as well because __maybe_unused has some kind of "last
> > > resort" feeling to me.
> >
> > I generally like __maybe_unused, but it's a matter of personal preference.
> > We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
> > pointers are always available in struct i2c_algorithm.
>
> Yes, I was thinking in this direction, looking at how PM does it. Needs
> some playing around, though.
I think PM gets it slightly wrong, the way you have to use #ifdef leads
to subtle bugs all the time, and I actually have a patch that converts
a few dozen drivers to use __maybe_unused to shut up build warnings and
errors.
What you can do though is to use a reference like
#define __i2c_slave_ptr(x) (IS_ENABLED(CONFIG_I2C_SLAVE) ? (x) : NULL)
...
.reg_slave = __i2c_slave_ptr(em_i2c_reg_slave),
.unreg_slave = __i2c_slave_ptr(em_i2c_unreg_slave),
...
This has the same effect as the __maybe_unused annotation.
Arnd
^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2015-12-17 19:58 UTC | newest]
Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-10 13:14 [PATCH] i2c: allow building emev2 without slave mode again Arnd Bergmann
2015-12-10 13:14 ` Arnd Bergmann
2015-12-10 13:14 ` Arnd Bergmann
2015-12-10 13:34 ` Wolfram Sang
2015-12-10 13:34 ` Wolfram Sang
2015-12-10 13:34 ` Wolfram Sang
2015-12-10 13:51 ` Arnd Bergmann
2015-12-10 13:51 ` Arnd Bergmann
2015-12-10 13:51 ` Arnd Bergmann
2015-12-10 13:51 ` Arnd Bergmann
2015-12-10 14:54 ` kbuild test robot
2015-12-10 14:54 ` kbuild test robot
2015-12-10 14:54 ` kbuild test robot
2015-12-10 15:06 ` Arnd Bergmann
2015-12-10 15:06 ` Arnd Bergmann
2015-12-10 15:06 ` Arnd Bergmann
2015-12-10 15:06 ` Arnd Bergmann
2015-12-10 15:17 ` Wolfram Sang
2015-12-10 15:17 ` Wolfram Sang
2015-12-10 15:17 ` Wolfram Sang
2015-12-12 16:20 ` Wolfram Sang
2015-12-12 16:20 ` Wolfram Sang
2015-12-12 16:20 ` Wolfram Sang
2015-12-12 21:05 ` Arnd Bergmann
2015-12-12 21:05 ` Arnd Bergmann
2015-12-12 21:05 ` Arnd Bergmann
2015-12-12 21:05 ` Arnd Bergmann
2015-12-13 9:09 ` Wolfram Sang
2015-12-13 9:09 ` Wolfram Sang
2015-12-13 9:09 ` Wolfram Sang
2015-12-14 12:02 ` Arnd Bergmann
2015-12-14 12:02 ` Arnd Bergmann
2015-12-14 12:02 ` Arnd Bergmann
2015-12-14 13:52 ` Wolfram Sang
2015-12-14 13:52 ` Wolfram Sang
2015-12-14 13:52 ` Wolfram Sang
2015-12-14 22:27 ` Arnd Bergmann
2015-12-14 22:27 ` Arnd Bergmann
2015-12-14 22:27 ` Arnd Bergmann
2015-12-17 12:01 ` Wolfram Sang
2015-12-17 12:01 ` Wolfram Sang
2015-12-17 12:01 ` Wolfram Sang
2015-12-17 14:48 ` Arnd Bergmann
2015-12-17 14:48 ` Arnd Bergmann
2015-12-17 14:48 ` Arnd Bergmann
2015-12-17 19:40 ` Wolfram Sang
2015-12-17 19:40 ` Wolfram Sang
2015-12-17 19:40 ` Wolfram Sang
2015-12-17 19:57 ` Arnd Bergmann
2015-12-17 19:57 ` Arnd Bergmann
2015-12-17 19:57 ` Arnd Bergmann
2015-12-17 19:57 ` Arnd Bergmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.