Message ID | 20150126171745.3d6d0fec@endymion.delvare |
---|---|
State | Superseded |
Headers | show |
> > Own module: Again, undecided. On the one hand it makes for a nice > > encapsulation, on the other hand there is overhead for having another > > module. I am very happy that the core code for slave support is so slim. > > I gave a try to the separate module approach and I have to agree that > it seems overkill given the small amount of code. OK, thanks for trying! > Something like this? Yes, pretty much what I had in mind. One issue, though: > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > enum i2c_slave_event { > I2C_SLAVE_REQ_READ_START, > I2C_SLAVE_REQ_READ_END, > @@ -263,6 +266,7 @@ static inline int i2c_slave_event(struct > { > return client->slave_cb(client, event, val); > } > +#endif This should fail because bus drivers need those enums for their slave backend. Try building i2c-sh_mobile which builds with an x86 toolchain as well. * Either we leave this included, so bus drivers don't need any ifdeffery or * we mandate that bus drivers also use the ifedeffery. Then, we could also mask out the (un)reg_slave callbacks in struct i2c_adapter What do you think?
On Mon, 26 Jan 2015 17:30:13 +0100, Wolfram Sang wrote: > > > > Own module: Again, undecided. On the one hand it makes for a nice > > > encapsulation, on the other hand there is overhead for having another > > > module. I am very happy that the core code for slave support is so slim. > > > > I gave a try to the separate module approach and I have to agree that > > it seems overkill given the small amount of code. > > OK, thanks for trying! > > > Something like this? > > Yes, pretty much what I had in mind. One issue, though: > > > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > > enum i2c_slave_event { > > I2C_SLAVE_REQ_READ_START, > > I2C_SLAVE_REQ_READ_END, > > @@ -263,6 +266,7 @@ static inline int i2c_slave_event(struct > > { > > return client->slave_cb(client, event, val); > > } > > +#endif > > This should fail because bus drivers need those enums for their slave > backend. Try building i2c-sh_mobile which builds with an x86 toolchain > as well. Sorry I missed that, because there is currently no i2c bus driver implementing slave support on x86-64. > * Either we leave this included, so bus drivers don't need any ifdeffery We can do that. The enum itself has no run-time cost so I don't mind. > or > > * we mandate that bus drivers also use the ifedeffery. Then, we could > also mask out the (un)reg_slave callbacks in struct i2c_adapter > > What do you think? Oh, I admit I completely missed the (un)reg_slave callbacks in my first patch. While I am happy with a few ifdefs in i2c-core and i2c.h, I agree it will become messy if these are required in device drivers as well. Hmm, what about bus drivers with slave mode support must select CONFIG_I2C_SLAVE? This solves my problem nicely, and makes no change compared to the current situation for people using slave mode. Thanks,
> Hmm, what about bus drivers with slave mode support must select > CONFIG_I2C_SLAVE? This solves my problem nicely, and makes no change > compared to the current situation for people using slave mode. I like that! Let's do it this way.
--- linux-3.19-rc6.orig/drivers/i2c/i2c-core.c 2015-01-26 12:47:26.467671896 +0100 +++ linux-3.19-rc6/drivers/i2c/i2c-core.c 2015-01-26 12:50:23.541420438 +0100 @@ -2962,6 +2962,7 @@ trace: } EXPORT_SYMBOL(i2c_smbus_xfer); +#if IS_ENABLED(CONFIG_I2C_SLAVE) int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb) {drivers/i2c/i2c-smbus.c int ret; @@ -3009,6 +3010,7 @@ int i2c_slave_unregister(struct i2c_clie return ret; } EXPORT_SYMBOL_GPL(i2c_slave_unregister); +#endif MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>"); MODULE_DESCRIPTION("I2C-Bus main module"); --- linux-3.19-rc6.orig/include/linux/i2c.h 2015-01-26 12:47:26.470671959 +0100 +++ linux-3.19-rc6/include/linux/i2c.h 2015-01-26 12:52:00.027462551 +0100 @@ -222,7 +222,9 @@ struct i2c_client { struct device dev; /* the device structure */ int irq; /* irq issued by device */ struct list_head detected; +#if IS_ENABLED(CONFIG_I2C_SLAVE) i2c_slave_cb_t slave_cb; /* callback for slave mode */ +#endif }; #define to_i2c_client(d) container_of(d, struct i2c_client, dev) @@ -247,6 +249,7 @@ static inline void i2c_set_clientdata(st /* I2C slave support */ +#if IS_ENABLED(CONFIG_I2C_SLAVE) enum i2c_slave_event { I2C_SLAVE_REQ_READ_START, I2C_SLAVE_REQ_READ_END, @@ -263,6 +266,7 @@ static inline int i2c_slave_event(struct { return client->slave_cb(client, event, val); } +#endif /** * struct i2c_board_info - template for device creation