Hi, On 6/15/23 15:19, 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. > > The same could later on be done for I3C. Ack, I'll do that for v4. But first lets see if there is going to be more feedback on v3. Regards, Hans
