| Message ID | 20191204095348.9192-1-s.hauer@pengutronix.de |
|---|---|
| State | Under Review |
| Delegated to: | Wolfram Sang |
| Headers | show |
| Series | i2c: avoid ifdeffery in I2C drivers with optional slave support | expand |
Hi Sascha, On 04/12/19 10:53, Sascha Hauer wrote: > Always add the (un)reg_slave hooks to struct i2c_algorithm, even when > I2C slave support is disabled. With the cost of some binary space I2C > drivers with optional I2C slave support no longer have to #ifdef > the hooks. For the same reason add a stub for i2c_slave_event and make > enum i2c_slave_event present without I2C slave support. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> I like the idea, but I have a question below. > --- > include/linux/i2c.h | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index d2f786706657..74ebfcb43dd2 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -359,7 +359,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, > @@ -368,6 +367,7 @@ enum i2c_slave_event { > I2C_SLAVE_STOP, > }; > > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb); > extern int i2c_slave_unregister(struct i2c_client *client); > extern bool i2c_detect_slave_mode(struct device *dev); > @@ -379,6 +379,11 @@ static inline int i2c_slave_event(struct i2c_client *client, > } > #else > static inline bool i2c_detect_slave_mode(struct device *dev) { return false; } > +static inline int i2c_slave_event(struct i2c_client *client, > + enum i2c_slave_event event, u8 *val) > +{ > + return -EINVAL; > +} > #endif > > /** > @@ -553,10 +558,8 @@ struct i2c_algorithm { > /* To determine what the adapter supports */ > u32 (*functionality)(struct i2c_adapter *adap); > > -#if IS_ENABLED(CONFIG_I2C_SLAVE) > int (*reg_slave)(struct i2c_client *client); > int (*unreg_slave)(struct i2c_client *client); > -#endif Assuming I2C slave users are a minority, would it make sense to move the two slave-related function pointers to a new 'struct i2c_slave_ops' and store a 'struct i2c_slave_ops*' here? This would to set a limit to the size increase for the majority of users. Thanks,
On Thu, Dec 05, 2019 at 03:45:09PM +0100, Luca Ceresoli wrote: > Hi Sascha, > > On 04/12/19 10:53, Sascha Hauer wrote: > > Always add the (un)reg_slave hooks to struct i2c_algorithm, even when > > I2C slave support is disabled. With the cost of some binary space I2C > > drivers with optional I2C slave support no longer have to #ifdef > > the hooks. For the same reason add a stub for i2c_slave_event and make > > enum i2c_slave_event present without I2C slave support. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > I like the idea, but I have a question below. > > > --- > > include/linux/i2c.h | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > > index d2f786706657..74ebfcb43dd2 100644 > > --- a/include/linux/i2c.h > > +++ b/include/linux/i2c.h > > @@ -359,7 +359,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, > > @@ -368,6 +367,7 @@ enum i2c_slave_event { > > I2C_SLAVE_STOP, > > }; > > > > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > > extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb); > > extern int i2c_slave_unregister(struct i2c_client *client); > > extern bool i2c_detect_slave_mode(struct device *dev); > > @@ -379,6 +379,11 @@ static inline int i2c_slave_event(struct i2c_client *client, > > } > > #else > > static inline bool i2c_detect_slave_mode(struct device *dev) { return false; } > > +static inline int i2c_slave_event(struct i2c_client *client, > > + enum i2c_slave_event event, u8 *val) > > +{ > > + return -EINVAL; > > +} > > #endif > > > > /** > > @@ -553,10 +558,8 @@ struct i2c_algorithm { > > /* To determine what the adapter supports */ > > u32 (*functionality)(struct i2c_adapter *adap); > > > > -#if IS_ENABLED(CONFIG_I2C_SLAVE) > > int (*reg_slave)(struct i2c_client *client); > > int (*unreg_slave)(struct i2c_client *client); > > -#endif > > Assuming I2C slave users are a minority, would it make sense to move the > two slave-related function pointers to a new 'struct i2c_slave_ops' and > store a 'struct i2c_slave_ops*' here? This would to set a limit to the > size increase for the majority of users. Would be doable I guess. I have no strong opinion here, but that would be done as a separate patch anyway, so should prevent this one from being merged. Sascha
On Wed, Dec 04, 2019 at 10:53:48AM +0100, Sascha Hauer wrote: > Always add the (un)reg_slave hooks to struct i2c_algorithm, even when > I2C slave support is disabled. With the cost of some binary space I2C > drivers with optional I2C slave support no longer have to #ifdef > the hooks. For the same reason add a stub for i2c_slave_event and make > enum i2c_slave_event present without I2C slave support. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> This kind of reverts d5fd120e7860 ("i2c: Only include slave support if selected"), so adding Jean here for more discussion. I don't mind the additional bytes used in i2c_algorithm, so I am in favor of this approach. I do mind the extra bytes used in each and every i2c_client (which is not affected in this patch). What we could do on top of this: Because i2c-slave backends will be rare (and only those need it), it might be worthwhile to introduce a struct i2c_slave_client and embed the original i2c_client there. Maybe this way, we could get rid of the I2C_SLAVE symbol entirely? The I2C core code is not a lot as well. > --- > include/linux/i2c.h | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index d2f786706657..74ebfcb43dd2 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -359,7 +359,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, > @@ -368,6 +367,7 @@ enum i2c_slave_event { > I2C_SLAVE_STOP, > } > > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb); > extern int i2c_slave_unregister(struct i2c_client *client); > extern bool i2c_detect_slave_mode(struct device *dev); > @@ -379,6 +379,11 @@ static inline int i2c_slave_event(struct i2c_client *client, > } > #else > static inline bool i2c_detect_slave_mode(struct device *dev) { return false; } > +static inline int i2c_slave_event(struct i2c_client *client, > + enum i2c_slave_event event, u8 *val) > +{ > + return -EINVAL; > +} > #endif > > /** > @@ -553,10 +558,8 @@ struct i2c_algorithm { > /* To determine what the adapter supports */ > u32 (*functionality)(struct i2c_adapter *adap); > > -#if IS_ENABLED(CONFIG_I2C_SLAVE) > int (*reg_slave)(struct i2c_client *client); > int (*unreg_slave)(struct i2c_client *client); > -#endif > }; > > /** > -- > 2.24.0 >
Hi Wolfram, Sascha, On Thu, 9 Apr 2020 15:40:27 +0200, Wolfram Sang wrote: > On Wed, Dec 04, 2019 at 10:53:48AM +0100, Sascha Hauer wrote: > > Always add the (un)reg_slave hooks to struct i2c_algorithm, even when > > I2C slave support is disabled. With the cost of some binary space I2C > > drivers with optional I2C slave support no longer have to #ifdef > > the hooks. For the same reason add a stub for i2c_slave_event and make > > enum i2c_slave_event present without I2C slave support. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > This kind of reverts d5fd120e7860 ("i2c: Only include slave support if > selected"), so adding Jean here for more discussion. That commit made sense then as there was only exactly 1 kernel driver needing this. This might be revisited when more drivers need it. That being said, as far as I can see only 8 drivers need it today, which isn't that many, and more importantly, several architectures will typically not include support for any of them (i386, x86_64 and s390x for example). > I don't mind the additional bytes used in i2c_algorithm, so I am in > favor of this approach. I find it questionable to increase the memory footprint on all x86_64 systems out there for a feature they do not need. Sure it's only 16 bytes in one structure, but if every subsystem does the same on a regular basis, it adds up. More importantly I can't see how the ifdef'd members of struct i2c_algorithm are the cause of the problem mentioned by Sascha. He seems to be concerned by drivers with *optional* I2C slave support having ifdefs. Why can't this be solved in these drivers directly? What prevents these drivers from unconditionally selecting I2C_SLAVE if that makes their code more simple? This moves the overhead decision to the device driver instead of forcing it to the whole subsystem across all architectures.
On Fri, Apr 10, 2020 at 11:29:14AM +0200, Jean Delvare wrote: > Hi Wolfram, Sascha, > > On Thu, 9 Apr 2020 15:40:27 +0200, Wolfram Sang wrote: > > On Wed, Dec 04, 2019 at 10:53:48AM +0100, Sascha Hauer wrote: > > > Always add the (un)reg_slave hooks to struct i2c_algorithm, even when > > > I2C slave support is disabled. With the cost of some binary space I2C > > > drivers with optional I2C slave support no longer have to #ifdef > > > the hooks. For the same reason add a stub for i2c_slave_event and make > > > enum i2c_slave_event present without I2C slave support. > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > This kind of reverts d5fd120e7860 ("i2c: Only include slave support if > > selected"), so adding Jean here for more discussion. > > That commit made sense then as there was only exactly 1 kernel driver > needing this. This might be revisited when more drivers need it. > > That being said, as far as I can see only 8 drivers need it today, which > isn't that many, and more importantly, several architectures will > typically not include support for any of them (i386, x86_64 and s390x > for example). > > > I don't mind the additional bytes used in i2c_algorithm, so I am in > > favor of this approach. > > I find it questionable to increase the memory footprint on all x86_64 > systems out there for a feature they do not need. Sure it's only 16 > bytes in one structure, but if every subsystem does the same on a > regular basis, it adds up. > > More importantly I can't see how the ifdef'd members of struct > i2c_algorithm are the cause of the problem mentioned by Sascha. He > seems to be concerned by drivers with *optional* I2C slave support > having ifdefs. Why can't this be solved in these drivers directly? What > prevents these drivers from unconditionally selecting I2C_SLAVE if that > makes their code more simple? This moves the overhead decision to the > device driver instead of forcing it to the whole subsystem across all > architectures. The drivers could select I2C_SLAVE when they have I2C slave support and in fact some drivers do this already. This means that we have the overhead of unneeded I2C slave support when we need that driver in the Kernel. I just thought it would be nice to have I2C slave support optional while still allowing to avoid ifdefs in the driver. Particularly this doesn't look nice: static const struct i2c_algorithm i2c_imx_algo = { .master_xfer = i2c_imx_xfer, .functionality = i2c_imx_func, +#if IS_ENABLED(CONFIG_I2C_SLAVE) + .reg_slave = i2c_imx_reg_slave, + .unreg_slave = i2c_imx_unreg_slave, +#endif } The implementation of these functions need ifdefs as well and compile coverage gets worse. Yes, we could select I2C_SLAVE from the driver and I don't really care which way we choose, the space overhead is marginal either way. Sascha
On Tue, 14 Apr 2020 13:56:00 +0200, Sascha Hauer wrote: > On Fri, Apr 10, 2020 at 11:29:14AM +0200, Jean Delvare wrote: > > More importantly I can't see how the ifdef'd members of struct > > i2c_algorithm are the cause of the problem mentioned by Sascha. He > > seems to be concerned by drivers with *optional* I2C slave support > > having ifdefs. Why can't this be solved in these drivers directly? What > > prevents these drivers from unconditionally selecting I2C_SLAVE if that > > makes their code more simple? This moves the overhead decision to the > > device driver instead of forcing it to the whole subsystem across all > > architectures. > > The drivers could select I2C_SLAVE when they have I2C slave support and > in fact some drivers do this already. This means that we have the > overhead of unneeded I2C slave support when we need that driver in the > Kernel. I can't make sense of this statement, sorry. How is I2C slave support "unneeded" if your kernel includes at least one kernel which needs it? It is true that I2C slave support is included in the kernel code as soon as any driver selects I2C_SLAVE, even if that driver is not currently loaded. The only way around that would be to move the common code for it to a separate module and all specific members to different, dedicated structures. But that would in turn cause more overhead for people who need slave support. The current implementation is the result of a trade-off decision I made back then. It is the same design goal which explains why I2C_SMBUS is a separate option: many system classes do not need it and I did not want to waste memory on these. The difference of I2C_SMBUS is that it was large and isolated enough to warrant a separate kernel module altogether. > I just thought it would be nice to have I2C slave support optional while > still allowing to avoid ifdefs in the driver. Particularly this doesn't > look nice: > > static const struct i2c_algorithm i2c_imx_algo = { > .master_xfer = i2c_imx_xfer, > .functionality = i2c_imx_func, > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > + .reg_slave = i2c_imx_reg_slave, > + .unreg_slave = i2c_imx_unreg_slave, > +#endif > } Probably a matter of taste, personally I see nothing wrong with it. > > The implementation of these functions need ifdefs as well and compile > coverage gets worse. Sorry but you lost me here. How can I2C slave support be "optional" and at the same time going without ifdefs? With your patch, device drivers would include slave support even if it will never be called because that support was not selected at the i2c core level. That's pretty confusing if you ask me. It is true that code coverage gets harder with additional configuration switches, that's the price to pay for being modular. > Yes, we could select I2C_SLAVE from the driver and I don't really care > which way we choose, the space overhead is marginal either way. With the current design, the idea is indeed to let drivers select I2C_SLAVE when they need it. And I think this solves your problem nicely as it lets you remove the ifdefs from your driver. I think it only makes sense to have ifdefs in the driver itself if that driver can be used on architectures or systems which will never need slave support and others which will need it, and these systems are different distribution targets, so that the decision can be made at build time. If the decision is not going to be made at build time then the ifdefs only clutter the code and have no value. More generally I think it is important to have design goals and to stick to them. The design goal of the current implementation was to let architectures which do not need slave support have no overhead at all. This comes at the cost of #ifdefs in the i2c core code and increased code coverage needs. Usually it should not need #ifdefs in the device drivers, with the exception of cross-platform devices mentioned above. If this is no longer desired and we prefer to have more simple core code and better code coverage at the cost of runtime overhead on millions of machine, then it can be done but then it must be done completely, that is, the I2C_SLAVE symbol goes away entirely and slave support is included in the kernel always, as Wolfram suggested in his reply. I wouldn't be happy with this move personally but I'm not the one making the decision and at least it would make sense.
On Tue, Apr 14, 2020 at 04:40:09PM +0200, Jean Delvare wrote: > On Tue, 14 Apr 2020 13:56:00 +0200, Sascha Hauer wrote: > > On Fri, Apr 10, 2020 at 11:29:14AM +0200, Jean Delvare wrote: > > > More importantly I can't see how the ifdef'd members of struct > > > i2c_algorithm are the cause of the problem mentioned by Sascha. He > > > seems to be concerned by drivers with *optional* I2C slave support > > > having ifdefs. Why can't this be solved in these drivers directly? What > > > prevents these drivers from unconditionally selecting I2C_SLAVE if that > > > makes their code more simple? This moves the overhead decision to the > > > device driver instead of forcing it to the whole subsystem across all > > > architectures. > > > > The drivers could select I2C_SLAVE when they have I2C slave support and > > in fact some drivers do this already. This means that we have the > > overhead of unneeded I2C slave support when we need that driver in the > > Kernel. > > I can't make sense of this statement, sorry. How is I2C slave support > "unneeded" if your kernel includes at least one kernel which needs it? I never used I2C slave > > It is true that I2C slave support is included in the kernel code as > soon as any driver selects I2C_SLAVE, even if that driver is not > currently loaded. The only way around that would be to move the common > code for it to a separate module and all specific members to different, > dedicated structures. But that would in turn cause more overhead for > people who need slave support. The current implementation is the result > of a trade-off decision I made back then. It is the same design goal > which explains why I2C_SMBUS is a separate option: many system classes > do not need it and I did not want to waste memory on these. The > difference of I2C_SMBUS is that it was large and isolated enough to > warrant a separate kernel module altogether. > > > I just thought it would be nice to have I2C slave support optional while > > still allowing to avoid ifdefs in the driver. Particularly this doesn't > > look nice: > > > > static const struct i2c_algorithm i2c_imx_algo = { > > .master_xfer = i2c_imx_xfer, > > .functionality = i2c_imx_func, > > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > > + .reg_slave = i2c_imx_reg_slave, > > + .unreg_slave = i2c_imx_unreg_slave, > > +#endif > > } > > Probably a matter of taste, personally I see nothing wrong with it. > > > > > The implementation of these functions need ifdefs as well and compile > > coverage gets worse. > > Sorry but you lost me here. How can I2C slave support be "optional" and > at the same time going without ifdefs? static int i2c_imx_reg_slave(struct i2c_client *client) { if (!IS_ENABLED(CONFIG_I2C_SLAVE)) return -ESOMETHING; ... } The code is gone without CONFIG_I2C_SLAVE enabled, yet the compile coverage is there. The patch I sent was a suggestion to do it like that. If that's not wanted I am fine with that and happily select CONFIG_I2C_SLAVE from the driver entry in Kconfig, or better, suggest Biwen Li (https://patchwork.kernel.org/patch/11271067/) to do this. Sascha
On Wed, 15 Apr 2020 07:16:19 +0200, Sascha Hauer wrote: > On Tue, Apr 14, 2020 at 04:40:09PM +0200, Jean Delvare wrote: > > Sorry but you lost me here. How can I2C slave support be "optional" and > > at the same time going without ifdefs? > > static int i2c_imx_reg_slave(struct i2c_client *client) > { > if (!IS_ENABLED(CONFIG_I2C_SLAVE)) > return -ESOMETHING; > ... > } > > The code is gone without CONFIG_I2C_SLAVE enabled, yet the compile coverage > is there. OK, *now* I see where you were going from the beginning ;-) And that makes sense, for drivers which want optional I2C slave support. Now I think this is down to 2 questions: 1* Does the i2c-imx driver actually need optional slave support, or can this support be included unconditionally? Which distributions or builds include this driver, and do they typically enable I2C_SLAVE? I took a look at the SUSE kernel configs and we already have I2C_SLAVE enabled on armv7 and arm64, so it will make no difference there. Only our armv6 config includes this driver without I2C_SLAVE, so that's the only one for which keeping the slave support optional would help. My point is: you stated that you never used slave support, and neither did I, but that's not really relevant. What is relevant is whether kernels including these drivers are being built with I2C_SLAVE or not in practice. If they are then it doesn't matter if individual drivers go the conditional or unconditional way, unless they add their own Kconfig option to explicitly enable slave support (see below). 2* More generally, how many drivers would benefit from your proposed change? At the moment I count 8 drivers selecting I2C_SLAVE, 3 of these have a separate Kconfig option for including slave support which actually makes slave support optional too but in a different way. 1 driver (i2c-slave-eeprom) depends on I2C_SLAVE, and 1 driver (i2c-aspeed) has #ifdefs for optional slave support. So we have a total of 6 drivers which support slave mode unconditionally and 4 drivers which have conditional support. That doesn't mean that they are right doing what they do though. Their authors may have gone for unconditional just because they don't like ifdefs, or they may have gone for conditional to play it safe. I'm also wondering why the 3 drivers which have a dedicated Kconfig option (I2C_AT91_SLAVE_EXPERIMENTAL, I2C_DESIGNWARE_SLAVE and I2C_PXA_SLAVE) did it that way. Is it on purpose because they actually want to be able to force slave mode off even if support is available at i2c core level? Is it by politeness to not forcibly enable slave mode as soon as the driver is enabled? It matters because in the former case, your proposed change is of no interest to them, while in the latter case it is. Unfortunately that's a lot of questions in the end and I do not know the answers nor am I willing to spend time finding them, sorry. > The patch I sent was a suggestion to do it like that. If that's not > wanted I am fine with that and happily select CONFIG_I2C_SLAVE from the > driver entry in Kconfig, or better, suggest Biwen Li > (https://patchwork.kernel.org/patch/11271067/) to do this. Well, there are advantages to both approaches, and without answers to the questions above, I see no reason to favor one or the other. In such situation I tend to stick to what we have. But of course the decision is Wolfram's not mine.
One last thing I meant to mention but forgot... On Wed, 15 Apr 2020 07:16:19 +0200, Sascha Hauer wrote: > static int i2c_imx_reg_slave(struct i2c_client *client) > { > if (!IS_ENABLED(CONFIG_I2C_SLAVE)) > return -ESOMETHING; > ... > } > > The code is gone without CONFIG_I2C_SLAVE enabled, yet the compile coverage > is there. Compile coverage is nice but it comes at a cost. With the approach above, the code will be built, then discarded. When the code is #ifdef'd out, it isn't built at all. This means that your approach, although it has advantages, increases the build time. And don't tell me "you only build once", we live at the time of continuous integration so we keep building kernels. As a support engineer, I build kernels daily, and even though I have access to a powerful build farm for that purpose, it still takes 30 to 60 minutes to get my kernel built each time. Obviously most of that time isn't spent on the i2c-imx driver ;-) but if every piece of code does the same, build time will inevitably increase.
On Fri, Apr 17, 2020 at 04:00:14PM +0200, Jean Delvare wrote: > One last thing I meant to mention but forgot... > > On Wed, 15 Apr 2020 07:16:19 +0200, Sascha Hauer wrote: > > static int i2c_imx_reg_slave(struct i2c_client *client) > > { > > if (!IS_ENABLED(CONFIG_I2C_SLAVE)) > > return -ESOMETHING; > > ... > > } > > > > The code is gone without CONFIG_I2C_SLAVE enabled, yet the compile coverage > > is there. > > Compile coverage is nice but it comes at a cost. With the approach > above, the code will be built, then discarded. When the code is > #ifdef'd out, it isn't built at all. This means that your approach, > although it has advantages, increases the build time. > > And don't tell me "you only build once", we live at the time of > continuous integration so we keep building kernels. As a support > engineer, I build kernels daily, and even though I have access to a > powerful build farm for that purpose, it still takes 30 to 60 minutes > to get my kernel built each time. Obviously most of that time isn't > spent on the i2c-imx driver ;-) but if every piece of code does the > same, build time will inevitably increase. Well often enough I spend more time building Kernels than actually running them, so don't tell me. Yes, build time increases when we do things in C rather than in CPP, but each #ifdef doubles the number of builds necessary to let the compiler go though all paths. I've sent and received more than enough patches that fix #ifdefs for the more unlikely cases which were not compile tested. Sascha
diff --git a/include/linux/i2c.h b/include/linux/i2c.h index d2f786706657..74ebfcb43dd2 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -359,7 +359,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, @@ -368,6 +367,7 @@ enum i2c_slave_event { I2C_SLAVE_STOP, }; +#if IS_ENABLED(CONFIG_I2C_SLAVE) extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb); extern int i2c_slave_unregister(struct i2c_client *client); extern bool i2c_detect_slave_mode(struct device *dev); @@ -379,6 +379,11 @@ static inline int i2c_slave_event(struct i2c_client *client, } #else static inline bool i2c_detect_slave_mode(struct device *dev) { return false; } +static inline int i2c_slave_event(struct i2c_client *client, + enum i2c_slave_event event, u8 *val) +{ + return -EINVAL; +} #endif /** @@ -553,10 +558,8 @@ struct i2c_algorithm { /* To determine what the adapter supports */ u32 (*functionality)(struct i2c_adapter *adap); -#if IS_ENABLED(CONFIG_I2C_SLAVE) int (*reg_slave)(struct i2c_client *client); int (*unreg_slave)(struct i2c_client *client); -#endif }; /**
Always add the (un)reg_slave hooks to struct i2c_algorithm, even when I2C slave support is disabled. With the cost of some binary space I2C drivers with optional I2C slave support no longer have to #ifdef the hooks. For the same reason add a stub for i2c_slave_event and make enum i2c_slave_event present without I2C slave support. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- include/linux/i2c.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)