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.
Thanks,
--
Luca
