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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, Apr 14, 2020 at 04:40:09PM +0200, Jean Delvare wrote:
> On Tue, 14 Apr 2020 13:56:00 +0200, Sascha Hauer wrote:
> > On Fri, Apr 10, 2020 at 11:29:14AM +0200, Jean Delvare wrote:
> > > More importantly I can't see how the ifdef'd members of struct
> > > i2c_algorithm are the cause of the problem mentioned by Sascha. He
> > > seems to be concerned by drivers with *optional* I2C slave support
> > > having ifdefs. Why can't this be solved in these drivers directly? What
> > > prevents these drivers from unconditionally selecting I2C_SLAVE if that
> > > makes their code more simple? This moves the overhead decision to the
> > > device driver instead of forcing it to the whole subsystem across all
> > > architectures.  
> > 
> > The drivers could select I2C_SLAVE when they have I2C slave support and
> > in fact some drivers do this already. This means that we have the
> > overhead of unneeded I2C slave support when we need that driver in the
> > Kernel.
> 
> I can't make sense of this statement, sorry. How is I2C slave support
> "unneeded" if your kernel includes at least one kernel which needs it?

I never used I2C slave 
> 
> It is true that I2C slave support is included in the kernel code as
> soon as any driver selects I2C_SLAVE, even if that driver is not
> currently loaded. The only way around that would be to move the common
> code for it to a separate module and all specific members to different,
> dedicated structures. But that would in turn cause more overhead for
> people who need slave support. The current implementation is the result
> of a trade-off decision I made back then. It is the same design goal
> which explains why I2C_SMBUS is a separate option: many system classes
> do not need it and I did not want to waste memory on these. The
> difference of I2C_SMBUS is that it was large and isolated enough to
> warrant a separate kernel module altogether.
> 
> > I just thought it would be nice to have I2C slave support optional while
> > still allowing to avoid ifdefs in the driver. Particularly this doesn't
> > look nice:
> > 
> >  static const struct i2c_algorithm i2c_imx_algo = {
> >         .master_xfer    = i2c_imx_xfer,
> >         .functionality  = i2c_imx_func,
> > +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> > +       .reg_slave      = i2c_imx_reg_slave,
> > +       .unreg_slave    = i2c_imx_unreg_slave,
> > +#endif
> > }
> 
> Probably a matter of taste, personally I see nothing wrong with it.
> 
> > 
> > The implementation of these functions need ifdefs as well and compile
> > coverage gets worse.
> 
> Sorry but you lost me here. How can I2C slave support be "optional" and
> at the same time going without ifdefs?

static int i2c_imx_reg_slave(struct i2c_client *client)
{
	if (!IS_ENABLED(CONFIG_I2C_SLAVE))
		return -ESOMETHING;
	...
}

The code is gone without CONFIG_I2C_SLAVE enabled, yet the compile coverage
is there.

The patch I sent was a suggestion to do it like that. If that's not
wanted I am fine with that and happily select CONFIG_I2C_SLAVE from the
driver entry in Kconfig, or better, suggest Biwen Li
(https://patchwork.kernel.org/patch/11271067/) to do this.

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 |