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 |
