On Thu, Jun 15, 2023 at 01:19:30PM +0000, Sakari Ailus wrote: > 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. In fact nearly the same can be achieved by just renaming V4L2_CCI V4L2_CCI_I2C now. -- Sakari Ailus
