drivers: video: Move the sensors to their dedicated directory

[josuah]

This allows to have a more immediate insight of the different kinds of video drivers present in this directory, as well as help maintenance of a whole group at once i.e. during refactors.

I still need to test building it and run it on hardware...

I understand that these kind of modifications might not be desired for backward compatibility reasons, or avoid breaking ongoing pull requests.

[loicpoulain]

Now we have more drivers, maybe we should have a sensor subdir instead?

[josuah]

Thank you taking the time to review! I will fix the conflicts and other CI issues, rebase, and wait that the few ongoing video driver PRs are merged to reduce friction then apply the exact same changes to the new drivers, and finally un-draft it.

[ngphibang]

I totally agree with this restructure. Would it be better if we group them into subdirs named like below ?

• video/i2c for "what we currently called sensor" drivers
• video/platform/nxp, st, ti, mediatek, etc. for platform IPs drivers

To me, the term "sensor" does not seem to reflect well the hardware itself because a camera module, such as ov5640 or mt9m114 does not only contain the "raw Bayer sensor" but also ISP, FIFO, Microcontroller, etc. blocks inside it. It seems we "imitate" Linux again but why not if they are already well defined :-).

[josuah]

I will keep the video/platform/nxp for a separate PR to make this one easier to merge, and refactor it to integrate more recent contributions.

Good to be sharing the hierarchy with Linux: that makes things easier to understand for Linux users.

Also, "I2C" makes it very easy to decide/find where drivers go/are: if it is on-i2c-bus, then it is in this directory.

This might be for all the external parts not inside the same SoC (since I2C is used to control them).

[josuah]

Just rebased and put on top of I2C.
Anyone feel free to suggest something else. I am completely fine with updating the directory at the pace of the discussion.

It is tempting to merge #66994 first to avoid delaying it further.

[josuah]

Force push to rebase. @loicpoulain this is currently using the i2c suffix helping Linux devs to find their way around.

[kartben]

I am not sure I understand the rationale for the change (as in: I am saying this because I really don't understand, not because I am passive-aggressively questioning the fact this might be the right thing to do :))
Can you please give an example of what would be the other folders eventually added alongside i2c, and what they may contain?
Given that no other driver class in Zephyr introduces such a "grouping", I am really eager to understand what's different for video drivers.
Also, the commit message refers to "Video processing devices sometimes include external chips often
configured on I2C: sensors, flash, motors, decoders, ISPs" which sounds a lot like Zephyr MFD to me. You don't expect those flash, motor, etc. drivers to live under the drivers/video/ hierarchy, or do you?

Thanks!

[nordicjm]

Tagging @ceolin because this is becoming a mess, sensors has folders for manufacturers, so does boards, and this is grouping by interface? Why? Why is this needed? What other drivers are going to make their own groupings up? If drivers should be grouped then they should be grouped as existing things are - by manufacturer, otherwise not grouped at all. Tagged @ceolin for possible process meeting discussion

[kartben]

Tagging @ceolin because this is becoming a mess, sensors has folders for manufacturers, so does boards, and this is grouping by interface? Why? Why is this needed? What other drivers are going to make their own groupings up? If drivers should be grouped then they should be grouped as existing things are - by manufacturer, otherwise not grouped at all. Tagged @ceolin for possible process meeting discussion

I'm guessing you meant to tag @keith-zephyr?

[nordicjm]

Tagging @ceolin because this is becoming a mess, sensors has folders for manufacturers, so does boards, and this is grouping by interface? Why? Why is this needed? What other drivers are going to make their own groupings up? If drivers should be grouped then they should be grouped as existing things are - by manufacturer, otherwise not grouped at all. Tagged @ceolin for possible process meeting discussion

I'm guessing you meant to tag @keith-zephyr?

Right you are, my bad.

[josuah]

@kartben I am not sure I understand the rationale for the change

I tried to illustrate some very frequent situation of video drivers here:

As illustrated, it is frequent to have a set of I2C-controlled peripherals that drive a fast data feed, but whose drivers are not directly interacting with the data, instead handled by a separate DMA I/O core such as MIPI or DMCI.

They use a different subset of the video API:

• External components' drivers usually only implement the control subset of the API
  stream_start(), stream_stop(), set_format(), get_format(), get_caps(), set_ctrl(), get_ctrl(), set_signal()
• Internal peripherals drivers usually only implement the data subset of the API
  stream_start(), stream_stop(), enqueue(), dequeue(), flush()
  and forward all/most controls to their attached "sensor"

@kartben: You don't expect those flash, motor, etc. drivers to live under the drivers/video/ hierarchy, or do you?

For instance, the ArduCam Mega abstract all the sensor features including the zoom level, and offer a single video interface for both data and control [1]. Having a separate zoom motor could conveniently be interfaced to a video driver using that same zoom control ID. I would make sure to ask when this situation surfaces.

@nordicjm: Why is this needed?

I do not think this is needed. Looking that other driver classes sometimes had sub-directories (disk, usb_c, sensors...), I thought that it was the way to go. I am completely wrong!

Now that I look again, I notice that all drivers that have a sub-directory (other than per-vendor), actually implement a different ..._api_t! Not the case here.

@nordicjm: If drivers should be grouped then they should be grouped as existing things are - by manufacturer, otherwise not grouped at all

Thank you for the insights.

[1]: Good counter-example of a sensor that uses SPI for both control and data and does not fit in any of the category I was proposing... rookie mistake!

[josuah]

Closing this as I AFAIU there is no grouping needed at all. Thank you for the feedback and apologies or the needless noise!

[ngphibang]

I always think grouping video device drivers into two categories is necessary :

• external devices : drivers/video/i2c or drivers/video/sensors or drivers/video/external or whatever
• internal devices : drivers/video/platform/manufacturers

As pointed out by @josuah, currently all the video drivers are placed in the same folder and the current video API are made for all kinds of video devices (which should not). Perhaps not now but in long terms, we should categrorize the APIs and do the grouping. As an example, the set/get_ctrl API should concern only the external devices (sensors) not the internal ones.

[josuah]

All right, re-opening for the sake of discussion then!

[josuah]

Should the current sensors get a video_ prefix like all other video drivers? I can submit this as a separate PR too.

[loicpoulain]

Should the current sensors get a video_ prefix like all other video drivers? I can submit this as a separate PR too.

We're already in video directory, so it does not bring any useful info. Yeah some other drivers use that prefix...

[kartben]

It feels like this should be closed given some of the recent comments?

[josuah]

Given the feedback, I do not see something that can be immediately done regarding the naming of sensors.
I am fine with either solution, and will open a new PR for fixing the sorting in Kconfig/CMakeLists.txt.