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. ... > >> +/** > >> + * cci_regmap_init_i2c() - Create regmap to use with cci_*() register access functions > >> + * > >> + * @client: i2c_client to create the regmap for > >> + * @reg_addr_bits: register address width to use (8 or 16) > >> + * > >> + * Note the memory for the created regmap is devm() managed, tied to the client. > >> + * > >> + * Return: %0 on success or a negative error code on failure. > >> + */ > >> +struct regmap *cci_regmap_init_i2c(struct i2c_client *client, int reg_addr_bits); > >> + > >> +#endif > > > > Could you run > > > > ./scripts/checkpatch.pl --strict --max-line-length=80 > > > > on this? > > As I mentioned during the v1 review where you also asked about > the 80 column limit, can we first please have an official > decision what the column limit is for drivers/media and then > also document this somewhere? This is documented in Documentation/driver-api/media/maintainer-entry-profile.rst and media tree follows that. > > Also note that you are asking me to modify the checkpatch > default max-line-length here. So basically you are deviating > from the official kernel coding style standards here. We're not. Note that checkpatch.pl is a tool to check code, it isn't the coding style itself, which is documented in Documentation/process/coding-style.rst . The default in checkpatch.pl was changed as it often produced many warnings where there was a justified reason for having longer lines (such as violating other rules in coding style). > > You are asking for 80 columns. Andy often adds review remarks > along the lines of: > > "this can fit on a single line" assuming the now official 100 > chars hard limit. I know... > > And I cannot make both you and Andy happy at the same time. > So please pick a limit, *document it* and then stick with it. > > This current inconsistency between reviewers is unhelpful. > > My personal opinion on this is that sometimes going over > 80 chars actually results in better readable code, > so I have a slight preference to just stick with the kernel > default of 100 chars. Sticking to the kernel default also > makes life a lot easier for people contributing to multiple > subsystems. So my vote goes to sticking with the new > kernel default of 100 chars. > > I'm happy to adjust this patch-set to fit everything in > 80 chars, but can we please first get some clarity on > what actual column limit we want for drivers/media ? Answered above. Note that the limit is not strict but it appears that in this set there are many longer lines than 80 but no apparent reason for having them that way. -- Kind regards, Sakari Ailus
