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@xxxxxxxxxxxxxx>
>
> 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
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
