i2c: avoid ifdeffery in I2C drivers with optional slave support
diff mbox series

Message ID 20191204095348.9192-1-s.hauer@pengutronix.de
State New
Headers show
Series
  • i2c: avoid ifdeffery in I2C drivers with optional slave support
Related show

Commit Message

Sascha Hauer Dec. 4, 2019, 9:53 a.m. UTC
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(-)

Comments

Luca Ceresoli Dec. 5, 2019, 2:45 p.m. UTC | #1
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,
Sascha Hauer Dec. 6, 2019, 7:44 a.m. UTC | #2
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

Patch
diff mbox series

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
 };
 
 /**