Re: [PATCH] i2c: avoid ifdeffery in I2C drivers with optional slave support

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]