Re: [PATCH] i2c-eg20t: use dynamically registered adapter number

[Jean Delvare <jdelvare@suse.de>]

Hi all,

On Mon, 19 Sep 2016 11:02:47 +0200, Jean Delvare wrote:
> My own proposal was checking __i2c_first_dynamic_bus_num. To be honest
> I can't see the added value of relying on i2c_get_adapter() instead.
> Would the result not be exactly the same? Plus it seems racy, just
> because i2c_get_adapter() returns NULL at one point in time doesn't
> mean the bus numbers will not have been assigned by the time you call
> i2c_add_numbered_adapter().
> (...)
> If you consider it a workaround, what would be the "real fix" for you?
> 
> I was wondering if selecting one of these drivers could set a Kconfig
> option to initialize __i2c_first_dynamic_bus_num to a non-zero value.
> Unfortunately there does not seem to be a way to set a numeric value to
> a Kconfig option using select. We would have to do it indirectly as
> with CONFIG_HZ:
> 
> choice
> 	default I2C_RESERVED_BUS_NR_0
> 	config I2C_RESERVED_BUS_NR_0
> 	config I2C_RESERVED_BUS_NR_2
> endchoice
> 
> config I2C_RESERVED_BUS_NR
> 	int
> 	default 0 if I2C_RESERVED_BUS_NR_0
> 	default 2 if I2C_RESERVED_BUS_NR_2
> 
> config I2C_EG20T
> 	tristate "Intel EG20T PCH/LAPIS Semicon IOH(ML7213/ML7223/ML7831) I2C"
> 	depends on PCI && (X86_32 || MIPS || COMPILE_TEST)
> 	select I2C_RESERVED_BUS_NR_2
> 
> And in i2c-core.c:
> 
> __i2c_first_dynamic_bus_num = I2C_RESERVED_BUS_NR;
> 
> If that's possible at all... I'm not sure if select works on choice
> config options.
> 
> Alternatively we could set the default directly based on driver
> selection:
> 
> config I2C_RESERVED_BUS_NR
> 	int
> 	default 0
> 	default 2 if I2C_EG20T
> 
> This is more simple but a little harder to maintain. One possible
> problem is if the number of buses isn't known at build time but could
> change depending on the hardware, for example. Also I don't know if
> more than one such driver can be included in the same kernel.
> 
> Or we can make it a user-visible option and leave it on whoever
> configures the kernel to get it right. In all cases it would move the
> decision to build-time instead of being set dynamically at run-time. 
> 
> Note that I am not claiming this is necessarily better than my initial
> proposal. I just wanted to mention the possibility.

I slept over it and start wondering if we aren't trying to address the
problem at the wrong end.

The initial reason why i2c_add_numbered_adapter() was used in the
i2c-eg20t driver (and, I suppose, i2c-pasemi and all other bus drivers)
is because the platform initialization code/data (native or OF-based)
may be declaring slave I2C devices on these buses. This bumps
__i2c_first_dynamic_bus_num, and the bus driver needs to call
i2c_add_numbered_adapter() to get bus numbers below that.

Maybe in that case the bus driver should ALSO get the static bus number
information from the platform initialization code/data. Then the bus
driver would check that information, and use it to call
i2c_add_numbered_adapter() as appropriate if present, or simply call
i2c_add_adapter() if not (or ask i2c_add_numbered_adapter for bus
number -1, which has the same result.)

Looks like drivers i2c-s3c2410, i2c-designware-*, i2c-kempld and
i2c-pxa are already getting this right. So maybe we don't need to
introduce another mechanism, but instead use what is already in place?

-- 
Jean Delvare
SUSE L3 Support