Hi Hans, On Thu, Jun 15, 2023 at 02:05:02PM +0200, Hans de Goede wrote: > Hi, > > On 6/15/23 13:41, Sakari Ailus wrote: > > Hi Hans, > > > > On Thu, Jun 15, 2023 at 12:15:47PM +0200, Hans de Goede wrote: > >> Hi, > >> > >> On 6/15/23 11:54, Sakari Ailus wrote: > >>> Hi Hans, > >>> > >>> On Thu, Jun 15, 2023 at 10:45:35AM +0200, Hans de Goede wrote: > >>>> Hi Sakari, > >>>> > >>>> On 6/14/23 22:39, Sakari Ailus wrote: > >>> > >>> ... > >>>>>> diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig > >>>>>> index 348559bc2468..523ba243261d 100644 > >>>>>> --- a/drivers/media/v4l2-core/Kconfig > >>>>>> +++ b/drivers/media/v4l2-core/Kconfig > >>>>>> @@ -74,6 +74,11 @@ config V4L2_FWNODE > >>>>>> config V4L2_ASYNC > >>>>>> tristate > >>>>>> > >>>>>> +config V4L2_CCI > >>>>>> + tristate > >>>>>> + depends on I2C > >>>>> > >>>>> This won't do anything if the dependent driver will select V4L2_CCI, will > >>>>> it? I'd let the sensor driver depend on I2C instead. CCI is also supported > >>>>> on I3C, for instance. > >>>> > >>>> It will cause a Kconfig error if the dependent driver does not depend > >>>> on I2C. Kconfig items doing select MUST depend on all the depends on > >>>> of the items they are selecting; and (continued below) > >>> > >>> Maybe this has changed? It used to be that these cases were silently > >>> ignored and it wasn't that long ago. I haven't been following this up. > >>> > >>> Nevertheless, this shouldn't depend on I2C as such. > >>> > >>>> > >>>>> > >>>>>> + select REGMAP_I2C > >>>>> > >>>>> This is a good question. > >>>>> > >>>>> How about adding V4L2_CCI_I2C that would select REGMAP_I2C? > >>>> > >>>> v4l2-cci.ko uses the devm_regmap_init_i2c() symbol, so > >>>> REGMAP_I2C must be enabled when V4L2_CCI is enabled and > >>>> REGMAP_I2C is a symbol which should be selected rather > >>>> then depended on when necessary. > >>> > >>> I agree. > >> > >> If you agree that because of the symbol dependency that > >> the select REGMAP_I2C is necessary then the depends on I2C > >> is also necessary because any Kconfig symbol selecting > >> another symbol MUST depends on all of the dependencies > >> of the selected symbol and REGMAP_I2C has: > >> > >> config REGMAP_I2C > >> tristate > >> depends on I2C > > > > Yes. > > > > How about putting cci_regmap_init_i2c() behind an #ifdef? Then there > > wouldn't be a need for REGMAP_I2C unconditionally, but dependent on I2C. > > > > I guess right now I2C is more or less given in many systems but binding CCI > > to it still seems dubious. > > Yes, I can wrap the cci_regmap_init_i2c() prototype + > implementation in > > #ifdef CONFIG_REGMAP_I2C > > for version 4. Downside of this is that all i2c sensor drivers > which want to use the CCI helpers now will need to have > a select REGMAP_I2C added to their Kconfig snippet. Not if you add a new option for V4L2_CCI, a bit like you suggested for sensor driver dependencies in general: config V4L2_CCI_I2C tristate depends on I2C select REGMAP_I2C select V4L2_CCI So individual drivers would then select this instead of the plain V4L2_CCI. The same could later on be done for I3C. -- Kind regards, Sakari Ailus
