Hello,
While working on camera sensor drivers, I got bothered by the amount of
bad practices we have grown slowly as a result of cargo-cult
programming. This patch series is an attempt to improve the situation by
addressing two common problems.
The first problem is addressed in patches 01/57 to 30/57. The subdev
.s_stream() operation is not reentrant, it should never be called to
start an already started subdev, or stop an already stopped one. This
requirement has never been formally documented, which has led many
drivers to track the streaming status and return immediately from
.s_stream() if the requested streaming state matches the current state.
Patch 01/57 first updates the documentation, and the next 29 patches
drop the unneeded checks from camera sensor drivers.
The second problem is addressed in patches 31/57 to 57/57. Starting and
stopping streaming on a camera pipeline requires coordination of the
different blocks. This is handled by the bridge driver, which starts and
stops streaming on the sensor explicitly with the .s_stream() operation.
Despite thus, many sensor drivers track the streaming state internally,
to stop the sensor in the system suspend handler if it is streaming, and
restart it in the system resume handler. This is not needed, and
couldn't work anyway in the general case as starting the sensor before
the bridge can prevent the bridge from synchronizing, for instance for
CSI-2 buses with a continuous clock.
Patches 31/57 to 34/57 refactor and improve the camera sensor
documentation, and the next 23 patches drop system suspend and resume
handlers from camera sensor drivers. Patch 53/57 is a bit of an oddball
as it drops stream handling from the runtime PM handlers instead, which
is even more incorrect as the sensor just can't be streaming already
when the runtime PM resume handler is called.
There are still a few related issues in drivers/media/i2c/ that I have
decided *not* to address in this series:
- The series focusses on camera sensor drivers, the lens controllers,
flash controllers, and the ADV748x (HDMI/CVBS to CSI-2 converter)
still implement system PM handlers.
- The et8ek8 driver still implements system PM handlers. While fixable,
the change will be bigger as the sensor should also move away from the
.s_power() operation. This requires more testing, which I can't easily
perform.
- More generally, the series doesn't address drivers that rely on the
.s_power() operation instead of implementing runtime PM (disregarding
the drivers other than camera sensors, there are still 17 of them).
The second part of the series unfortunately depends on the first part.
I've decided to split the patches in two parts to clearly show the two
separate issues, and then split them further by driver. If strongly
desired, I could squash patches from the first and second parts together
when they address the same driver, although that will cause quite a bit
of churn to update all commit messages. I prefer keeping the per-driver
split to facilitate backporting.
Laurent Pinchart (57):
media: v4l2-subdev: Document .s_stream() operation requirements
media: i2c: hi556: Drop check for reentrant .s_stream()
media: i2c: hi846: Drop check for reentrant .s_stream()
media: i2c: imx208: Drop check for reentrant .s_stream()
media: i2c: imx214: Drop check for reentrant .s_stream()
media: i2c: imx219: Drop check for reentrant .s_stream()
media: i2c: imx258: Drop check for reentrant .s_stream()
media: i2c: imx319: Drop check for reentrant .s_stream()
media: i2c: imx334: Drop check for reentrant .s_stream()
media: i2c: imx335: Drop check for reentrant .s_stream()
media: i2c: imx355: Drop check for reentrant .s_stream()
media: i2c: imx412: Drop check for reentrant .s_stream()
media: i2c: mt9m001: Drop check for reentrant .s_stream()
media: i2c: og01a1b: Drop check for reentrant .s_stream()
media: i2c: ov01a10: Drop check for reentrant .s_stream()
media: i2c: ov08d10: Drop check for reentrant .s_stream()
media: i2c: ov08x40: Drop check for reentrant .s_stream()
media: i2c: ov13858: Drop check for reentrant .s_stream()
media: i2c: ov13b10: Drop check for reentrant .s_stream()
media: i2c: ov2685: Drop check for reentrant .s_stream()
media: i2c: ov2740: Drop check for reentrant .s_stream()
media: i2c: ov4689: Drop check for reentrant .s_stream()
media: i2c: ov5647: Drop check for reentrant .s_stream()
media: i2c: ov5670: Drop check for reentrant .s_stream()
media: i2c: ov5675: Drop check for reentrant .s_stream()
media: i2c: ov5695: Drop check for reentrant .s_stream()
media: i2c: ov7740: Drop check for reentrant .s_stream()
media: i2c: ov8856: Drop check for reentrant .s_stream()
media: i2c: ov9282: Drop check for reentrant .s_stream()
media: i2c: ov9734: Drop check for reentrant .s_stream()
Documentation: media: camera-sensor: Fix typo and vocabulary selection
Documentation: media: camera-sensor: Use link to upstream DT bindings
Documentation: media: camera-sensor: Move power management section
Documentation: media: camera-sensor: Improve power management
documentation
media: i2c: ar0521: Drop system suspend and resume handlers
media: i2c: ccs: Drop system suspend and resume handlers
media: i2c: hi556: Drop system suspend and resume handlers
media: i2c: hi846: Drop system suspend and resume handlers
media: i2c: hi847: Drop system suspend and resume handlers
media: i2c: imx208: Drop system suspend and resume handlers
media: i2c: imx214: Drop system suspend and resume handlers
media: i2c: imx219: Drop system suspend and resume handlers
media: i2c: imx258: Drop system suspend and resume handlers
media: i2c: imx319: Drop system suspend and resume handlers
media: i2c: imx355: Drop system suspend and resume handlers
media: i2c: og01a1b: Drop system suspend and resume handlers
media: i2c: ov01a10: Drop system suspend and resume handlers
media: i2c: ov02a10: Drop system suspend and resume handlers
media: i2c: ov08d10: Drop system suspend and resume handlers
media: i2c: ov08x40: Drop system suspend and resume handlers
media: i2c: ov13858: Drop system suspend and resume handlers
media: i2c: ov2740: Drop system suspend and resume handlers
media: i2c: ov13b10: Drop stream handling in runtime PM handlers
media: i2c: ov5670: Drop system suspend and resume handlers
media: i2c: ov5675: Drop system suspend and resume handlers
media: i2c: ov8856: Drop system suspend and resume handlers
media: i2c: ov9734: Drop system suspend and resume handlers
.../driver-api/media/camera-sensor.rst | 107 +++++++++++-------
drivers/media/i2c/ar0521.c | 30 +----
drivers/media/i2c/ccs/ccs-core.c | 37 ------
drivers/media/i2c/hi556.c | 50 --------
drivers/media/i2c/hi846.c | 28 +----
drivers/media/i2c/hi847.c | 52 ---------
drivers/media/i2c/imx208.c | 47 --------
drivers/media/i2c/imx214.c | 40 -------
drivers/media/i2c/imx219.c | 44 -------
drivers/media/i2c/imx258.c | 40 -------
drivers/media/i2c/imx319.c | 44 -------
drivers/media/i2c/imx334.c | 9 --
drivers/media/i2c/imx335.c | 9 --
drivers/media/i2c/imx355.c | 45 --------
drivers/media/i2c/imx412.c | 9 --
drivers/media/i2c/mt9m001.c | 6 -
drivers/media/i2c/og01a1b.c | 50 --------
drivers/media/i2c/ov01a10.c | 59 +---------
drivers/media/i2c/ov02a10.c | 2 -
drivers/media/i2c/ov08d10.c | 52 ---------
drivers/media/i2c/ov08x40.c | 44 -------
drivers/media/i2c/ov13858.c | 44 -------
drivers/media/i2c/ov13b10.c | 38 +------
drivers/media/i2c/ov2685.c | 7 --
drivers/media/i2c/ov2740.c | 45 --------
drivers/media/i2c/ov4689.c | 7 --
drivers/media/i2c/ov5647.c | 6 -
drivers/media/i2c/ov5670.c | 36 ------
drivers/media/i2c/ov5675.c | 44 -------
drivers/media/i2c/ov5695.c | 6 -
drivers/media/i2c/ov7740.c | 7 --
drivers/media/i2c/ov8856.c | 47 --------
drivers/media/i2c/ov9282.c | 9 --
drivers/media/i2c/ov9734.c | 50 --------
include/media/v4l2-subdev.h | 4 +-
35 files changed, 76 insertions(+), 1078 deletions(-)
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
Hello, While working on camera sensor drivers, I got bothered by the amount of bad practices we have grown slowly as a result of cargo-cult programming. This patch series is an attempt to improve the situation by addressing two common problems. The first problem is addressed in patches 01/57 to 30/57. The subdev .s_stream() operation is not reentrant, it should never be called to start an already started subdev, or stop an already stopped one. This requirement has never been formally documented, which has led many drivers to track the streaming status and return immediately from .s_stream() if the requested streaming state matches the current state. Patch 01/57 first updates the documentation, and the next 29 patches drop the unneeded checks from camera sensor drivers. The second problem is addressed in patches 31/57 to 57/57. Starting and stopping streaming on a camera pipeline requires coordination of the different blocks. This is handled by the bridge driver, which starts and stops streaming on the sensor explicitly with the .s_stream() operation. Despite thus, many sensor drivers track the streaming state internally, to stop the sensor in the system suspend handler if it is streaming, and restart it in the system resume handler. This is not needed, and couldn't work anyway in the general case as starting the sensor before the bridge can prevent the bridge from synchronizing, for instance for CSI-2 buses with a continuous clock. Patches 31/57 to 34/57 refactor and improve the camera sensor documentation, and the next 23 patches drop system suspend and resume handlers from camera sensor drivers. Patch 53/57 is a bit of an oddball as it drops stream handling from the runtime PM handlers instead, which is even more incorrect as the sensor just can't be streaming already when the runtime PM resume handler is called. There are still a few related issues in drivers/media/i2c/ that I have decided *not* to address in this series: - The series focusses on camera sensor drivers, the lens controllers, flash controllers, and the ADV748x (HDMI/CVBS to CSI-2 converter) still implement system PM handlers. - The et8ek8 driver still implements system PM handlers. While fixable, the change will be bigger as the sensor should also move away from the .s_power() operation. This requires more testing, which I can't easily perform. - More generally, the series doesn't address drivers that rely on the .s_power() operation instead of implementing runtime PM (disregarding the drivers other than camera sensors, there are still 17 of them). The second part of the series unfortunately depends on the first part. I've decided to split the patches in two parts to clearly show the two separate issues, and then split them further by driver. If strongly desired, I could squash patches from the first and second parts together when they address the same driver, although that will cause quite a bit of churn to update all commit messages. I prefer keeping the per-driver split to facilitate backporting. Laurent Pinchart (57): media: v4l2-subdev: Document .s_stream() operation requirements media: i2c: hi556: Drop check for reentrant .s_stream() media: i2c: hi846: Drop check for reentrant .s_stream() media: i2c: imx208: Drop check for reentrant .s_stream() media: i2c: imx214: Drop check for reentrant .s_stream() media: i2c: imx219: Drop check for reentrant .s_stream() media: i2c: imx258: Drop check for reentrant .s_stream() media: i2c: imx319: Drop check for reentrant .s_stream() media: i2c: imx334: Drop check for reentrant .s_stream() media: i2c: imx335: Drop check for reentrant .s_stream() media: i2c: imx355: Drop check for reentrant .s_stream() media: i2c: imx412: Drop check for reentrant .s_stream() media: i2c: mt9m001: Drop check for reentrant .s_stream() media: i2c: og01a1b: Drop check for reentrant .s_stream() media: i2c: ov01a10: Drop check for reentrant .s_stream() media: i2c: ov08d10: Drop check for reentrant .s_stream() media: i2c: ov08x40: Drop check for reentrant .s_stream() media: i2c: ov13858: Drop check for reentrant .s_stream() media: i2c: ov13b10: Drop check for reentrant .s_stream() media: i2c: ov2685: Drop check for reentrant .s_stream() media: i2c: ov2740: Drop check for reentrant .s_stream() media: i2c: ov4689: Drop check for reentrant .s_stream() media: i2c: ov5647: Drop check for reentrant .s_stream() media: i2c: ov5670: Drop check for reentrant .s_stream() media: i2c: ov5675: Drop check for reentrant .s_stream() media: i2c: ov5695: Drop check for reentrant .s_stream() media: i2c: ov7740: Drop check for reentrant .s_stream() media: i2c: ov8856: Drop check for reentrant .s_stream() media: i2c: ov9282: Drop check for reentrant .s_stream() media: i2c: ov9734: Drop check for reentrant .s_stream() Documentation: media: camera-sensor: Fix typo and vocabulary selection Documentation: media: camera-sensor: Use link to upstream DT bindings Documentation: media: camera-sensor: Move power management section Documentation: media: camera-sensor: Improve power management documentation media: i2c: ar0521: Drop system suspend and resume handlers media: i2c: ccs: Drop system suspend and resume handlers media: i2c: hi556: Drop system suspend and resume handlers media: i2c: hi846: Drop system suspend and resume handlers media: i2c: hi847: Drop system suspend and resume handlers media: i2c: imx208: Drop system suspend and resume handlers media: i2c: imx214: Drop system suspend and resume handlers media: i2c: imx219: Drop system suspend and resume handlers media: i2c: imx258: Drop system suspend and resume handlers media: i2c: imx319: Drop system suspend and resume handlers media: i2c: imx355: Drop system suspend and resume handlers media: i2c: og01a1b: Drop system suspend and resume handlers media: i2c: ov01a10: Drop system suspend and resume handlers media: i2c: ov02a10: Drop system suspend and resume handlers media: i2c: ov08d10: Drop system suspend and resume handlers media: i2c: ov08x40: Drop system suspend and resume handlers media: i2c: ov13858: Drop system suspend and resume handlers media: i2c: ov2740: Drop system suspend and resume handlers media: i2c: ov13b10: Drop stream handling in runtime PM handlers media: i2c: ov5670: Drop system suspend and resume handlers media: i2c: ov5675: Drop system suspend and resume handlers media: i2c: ov8856: Drop system suspend and resume handlers media: i2c: ov9734: Drop system suspend and resume handlers .../driver-api/media/camera-sensor.rst | 107 +++++++++++------- drivers/media/i2c/ar0521.c | 30 +---- drivers/media/i2c/ccs/ccs-core.c | 37 ------ drivers/media/i2c/hi556.c | 50 -------- drivers/media/i2c/hi846.c | 28 +---- drivers/media/i2c/hi847.c | 52 --------- drivers/media/i2c/imx208.c | 47 -------- drivers/media/i2c/imx214.c | 40 ------- drivers/media/i2c/imx219.c | 44 ------- drivers/media/i2c/imx258.c | 40 ------- drivers/media/i2c/imx319.c | 44 ------- drivers/media/i2c/imx334.c | 9 -- drivers/media/i2c/imx335.c | 9 -- drivers/media/i2c/imx355.c | 45 -------- drivers/media/i2c/imx412.c | 9 -- drivers/media/i2c/mt9m001.c | 6 - drivers/media/i2c/og01a1b.c | 50 -------- drivers/media/i2c/ov01a10.c | 59 +--------- drivers/media/i2c/ov02a10.c | 2 - drivers/media/i2c/ov08d10.c | 52 --------- drivers/media/i2c/ov08x40.c | 44 ------- drivers/media/i2c/ov13858.c | 44 ------- drivers/media/i2c/ov13b10.c | 38 +------ drivers/media/i2c/ov2685.c | 7 -- drivers/media/i2c/ov2740.c | 45 -------- drivers/media/i2c/ov4689.c | 7 -- drivers/media/i2c/ov5647.c | 6 - drivers/media/i2c/ov5670.c | 36 ------ drivers/media/i2c/ov5675.c | 44 ------- drivers/media/i2c/ov5695.c | 6 - drivers/media/i2c/ov7740.c | 7 -- drivers/media/i2c/ov8856.c | 47 -------- drivers/media/i2c/ov9282.c | 9 -- drivers/media/i2c/ov9734.c | 50 -------- include/media/v4l2-subdev.h | 4 +- 35 files changed, 76 insertions(+), 1078 deletions(-) base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d