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. Regards, Hans > >> >> <snip> >> >>> This is documented in >>> Documentation/driver-api/media/maintainer-entry-profile.rst and media tree >>> follows that. >> >> Ah, I missed that. Ok, I'll run >> >> ./scripts/checkpatch.pl --strict --max-line-length=80 >> >> and fix the warnings, with maybe one or 2 exceptions >> where longer lines really make the code more readable. > > Thank you. >
