On Thu, Jun 15, 2023 at 12:22:58AM +0300, Laurent Pinchart wrote: > On Wed, Jun 14, 2023 at 08:30:16PM +0000, sakari.ailus@xxxxxxxxxxxxxxx wrote: > > On Wed, Jun 14, 2023 at 06:50:14PM +0200, Hans de Goede wrote: > > > On 6/14/23 18:05, sakari.ailus@xxxxxxxxxxxxxxx wrote: > > > > On Wed, Jun 14, 2023 at 05:47:01PM +0200, Hans de Goede wrote: > > > >> Hi All, > > > >> > > > >> The ov5693 driver uses v4l2_subdev_get_try_crop() / > > > >> v4l2_subdev_get_try_format() both of which are > > > >> only defined if CONFIG_VIDEO_V4L2_SUBDEV_API=y . > > > >> > > > >> Yet it does not do select VIDEO_V4L2_SUBDEV_API > > > >> in its Kconfig bits ? > > > >> > > > >> Note I've not seen any build errors because of this, > > > >> I guess we somehow end up getting away with this... > > > >> > > > >> But still I think the select should be added ? > > > > > > > > I agree. > > > > > > > > The reason there haven't been compile failures is that there's a vast > > > > number of sensor drivers that all select this so for a failure you'd need > > > > to select this one but none of the others. > > > > > > > > I can send a fix. > > > > > > Also see my follow-up email. If we're going to fix this > > > we really should fix it properly. As mentioned in > > > my folow-up email an intermediate Kconfig option > > > might be best. > > > > > > E.g. doing: > > > > > > grep -l v4l2_async_register_subdev drivers/media/i2c/*.c > > > > > > And comparing that to Kconfig finds the following Kconfig > > > entries lacking a select V4L2_FWNODE / select V4l2_ASYNC > > > > > > VIDEO_IMX208 > > > VIDEO_IMX258 > > > VIDEO_IMX274 > > > VIDEO_IMX319 > > > VIDEO_IMX355 > > > VIDEO_OV6650 > > > VIDEO_OV7740 > > > VIDEO_OV9640 > > > VIDEO_OV9650 > > > > > > and I stopped checking after the ov* drivers since I think > > > the above list makes my point. > > > > Yeah, sometimes difficult to find errors get repeated. Luckily it's "just" > > a compilation problem. > > > > Using V4L2 fwnode and V4L2 sub-device APIs are still unrelated as such > > although in practice they do often happen together. There are still quite a > > few sensor drivers that don't need both of them. Some can be compiled with > > VIDEO_V4L2_SUBDEV_API disabled, too, but I'm not sure how useful that > > really is. The rest are probably not usable outside a very specific scope, > > such as I²C async matching used by a handful of receiver drivers (none use > > MC, thus no sub-device API either). > > > > Perhaps we could group these in two classes where the common class has > > V4L2_FWNODE and VIDEO_V4L2_SUBDEV_API selected? I'm not sure having an > > intermediate, somewhat obscure, option would be helpful. > > > > Also cc Hans and Laurent. > > I'm all for simplifying the current state and removing the need to get > every Kconfig entry right by moving the dependencies to a common > location. Also, if sensor drivers are encouraged to use new APIs, but not all of them have been converted, I'd be fine selecting the new APIs unconditionally even if no selected sensor driver uses them. -- Regards, Laurent Pinchart
