[PATCH 00/72] media: i2c: Reduce cargo-cult

[PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart]

Hello,

This patch series build on top of Mehdi's introduction of the
devm_v4l2_sensor_clk_get() helper (see [1]) to drastically reduce
cargo-cult in camera sensor drivers.

A large number of camera sensor drivers directly use the
"clock-frequency" property to retrieve the effective or desired external
clock rate. This is standard behaviour on ACPI platforms that don't
implement MIPI DisCo for Imaging, but usage of the property has leaked
to OF-based platforms, due to a combination of historical reasons (using
"clock-frequency" was initially considered right until before the
introduction of "assigned-clock-rates") and plain cargo-cult.

A large number of camera sensor drivers also set the rate of the
external clock with clk_set_rate(). This behaviour is also fine on ACPI
platforms, and has also leaked to OF-based platforms for the same
reasons.

Mehdi's "[PATCH v2 00/48] media: Add a helper for obtaining the clock
producer" series improves the situation by centralizing clock handling
for camera sensor in one helper function that implements the correct
behaviour for all types of platforms (and should later allow support of
MIPI DisCo for Imaging transparently for camera sensor drivers). It
doesn't however address direct access of the "clock-frequency" property
or direct calls to clk_set_rate() in drivers.

This series builds on top of the new helper to replace manual handling
of the clock frequency in camera sensor drivers. It starts by addressing
the DT bindings and reprecating the clock-frequency property for camera
sensor drivers in all YAML bindings (01/72) and in the et8ek8 text
bindings (02/72). After that, patches 03/72 and 04/72 make the clocks
property mandatory in the two camera sensor DT bindings that specified
it as optional. Finally for the DT side, patches 05/72 to 14/72 replace
clock-frequency with assigned-clock-rates, or drops the property
altogether when the source clock has a fixed rate. This aligns the DT
bindings and device tree sources to the current recommended practice.

After that, the next 5 patches are assorted drive-by changes. Patch
15/72 drops an unused header the belonged to a long gone driver, and
patch 17/72 drops unusued support for platform data in the mt9v032
driver. Patch 18/72 is the first that addresses clock rate handling by
dropping unneeded clock rate setting in the mt9v111 driver. Patch 19/72
takes a harsher approach for the ov6650 by dropping the driver
completely as the driver hasn't been used since v5.9.

The next part of the series replaces manual clock rate handling with
usage of the devm_v4l2_sensor_clk_get() helper in a large number of
camera sensor drivers that implement clock rate handling in a standard
way. This is done in patches 20/72 to 54/72. This interleaves the clock
rate handling changes with drive-by refactoring (in separate patches) to
make the code easier to deal with.

The final part of the series addresses the remaining drivers that
implement non-standard behaviours. It starts in 55/72 by adding a new
devm_v4l2_sensor_clk_get_legacy() helper function for those drivers,
similar to devm_v4l2_sensor_clk_get() but with a few more quirks. This
function should not be used in any new driver. The remaining patches,
from 53/72 to 72/72, use the new helper in drivers, interleaved with
drive-by refactoring similarly to the previous part.

Before this series, with Mehdi's series applied, 29 drivers read the
"clock-frequency" property and 18 drivers set the external clock rate.
With these series we go down to 1 and 3 respectively, namely the ccs,
mt9p031 and mt9v032 drivers. Clock handling in the CCS driver is a bit
more convoluted so I will leave to Sakari the honour of dropping the
last direct user of "clock-frequency" :-) As for the mt9p031 and mt9v032
driver, addressing the issue there is more difficult and likely not
worth it.

[1] https://lore.kernel.org/linux-media/cover.1750942967.git.mehdi.djait@linux.intel.com

Laurent Pinchart (72):
  dt-bindings: media: Deprecate clock-frequency property for camera
    sensors
  dt-bindings: media: et8ek8: Deprecate clock-frequency property
  dt-bindings: media: imx258: Make clocks property required
  dt-bindings: media: imx274: Make clocks property required
  ARM: dts: nxp: imx6qdl-pico: Replace clock-frequency in camera sensor
    node
  ARM: dts: nxp: imx6qdl-wandboard: Replace clock-frequency in camera
    sensor node
  ARM: dts: samsung: exynos4210-i9100: Replace clock-frequency in camera
    sensor node
  ARM: dts: samsung: exynos4412-midas: Replace clock-frequency in camera
    sensor node
  ARM: dts: ti: omap3-n900: Replace clock-frequency in camera sensor
    node
  ARM: dts: ti: omap3-n950: Replace clock-frequency in camera sensor
    node
  ARM: dts: ti: omap3-n9: Replace clock-frequency in camera sensor node
  arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace
    clock-frequency in camera sensor node
  arm64: dts: renesas: aistarvision-mipi-adapter-2.1: Drop
    clock-frequency from camera sensor node
  arm64: dts: renesas: rzg2l-smarc: Drop clock-frequency from camera
    sensor node
  media: i2c: mt9v022: Drop unused mt9v022.h header
  media: i2c: mt9v032: Replace client->dev usage
  media: i2c: mt9v032: Drop support for platform data
  media: i2c: mt9v111: Do not set clock rate manually
  media: i2c: ov6650: Drop unused driver
  media: i2c: hi556: Replace client->dev usage
  media: i2c: hi556: Use V4L2 sensor clock helper
  media: i2c: hi847: Replace client->dev usage
  media: i2c: hi847: Use V4L2 sensor clock helper
  media: i2c: imx208: Replace client->dev usage
  media: i2c: imx208: Use V4L2 sensor clock helper
  media: i2c: imx319: Replace client->dev usage
  media: i2c: imx319: Use V4L2 sensor clock helper
  media: i2c: imx355: Replace client->dev usage
  media: i2c: imx335: Use V4L2 sensor clock helper
  media: i2c: og01a1b: Replace client->dev usage
  media: i2c: og01a1b: Use V4L2 sensor clock helper
  media: i2c: ov02c10: Replace client->dev usage
  media: i2c: ov02c10: Use V4L2 sensor clock helper
  media: i2c: ov02e10: Replace client->dev usage
  media: i2c: ov02e10: Use V4L2 sensor clock helper
  media: i2c: ov08d10: Replace client->dev usage
  media: i2c: ov08d10: Use V4L2 sensor clock helper
  media: i2c: ov08x40: Replace client->dev usage
  media: i2c: ov08x40: Use V4L2 sensor clock helper
  media: i2c: ov13858: Replace client->dev usage
  media: i2c: ov13858: Use V4L2 sensor clock helper
  media: i2c: ov13b10: Replace client->dev usage
  media: i2c: ov13b10: Use V4L2 sensor clock helper
  media: i2c: ov2740: Replace client->dev usage
  media: i2c: ov2740: Use V4L2 sensor clock helper
  media: i2c: ov4689: Use V4L2 sensor clock helper
  media: i2c: ov5670: Replace client->dev usage
  media: i2c: ov5670: Use V4L2 sensor clock helper
  media: i2c: ov5675: Replace client->dev usage
  media: i2c: ov5675: Use V4L2 sensor clock helper
  media: i2c: ov5693: Use V4L2 sensor clock helper
  media: i2c: ov7251: Use V4L2 sensor clock helper
  media: i2c: ov9734: Replace client->dev usage
  media: i2c: ov9734: Use V4L2 sensor clock helper
  media: v4l2-common: Add legacy camera sensor clock helper
  media: i2c: et8ek8: Drop support for per-mode external clock frequency
  media: i2c: et8ek8: Use V4L2 legacy sensor clock helper
  media: i2c: gc05a2: Use V4L2 legacy sensor clock helper
  media: i2c: gc08a3: Use V4L2 legacy sensor clock helper
  media: i2c: imx258: Replace client->dev usage
  media: i2c: imx258: Use V4L2 legacy sensor clock helper
  media: i2c: imx290: Use V4L2 legacy sensor clock helper
  media: i2c: ov02a10: Replace client->dev usage
  media: i2c: ov02a10: Use V4L2 legacy sensor clock helper
  media: i2c: ov2685: Use V4L2 legacy sensor clock helper
  media: i2c: ov5645: Use V4L2 legacy sensor clock helper
  media: i2c: ov5695: Use V4L2 legacy sensor clock helper
  media: i2c: ov8856: Replace client->dev usage
  media: i2c: ov8856: Use V4L2 legacy sensor clock helper
  media: i2c: s5c73m3: Use V4L2 legacy sensor clock helper
  media: i2c: s5k5baf: Use V4L2 legacy sensor clock helper
  media: i2c: s5k6a3: Use V4L2 legacy sensor clock helper

 .../admin-guide/media/i2c-cardlist.rst        |    1 -
 .../bindings/media/i2c/mipi-ccs.yaml          |    7 +-
 .../bindings/media/i2c/ovti,ov02a10.yaml      |    3 +-
 .../bindings/media/i2c/ovti,ov5645.yaml       |    6 +-
 .../bindings/media/i2c/ovti,ov7251.yaml       |    6 +-
 .../bindings/media/i2c/ovti,ov8856.yaml       |    3 +-
 .../bindings/media/i2c/samsung,s5k5baf.yaml   |    6 +-
 .../bindings/media/i2c/samsung,s5k6a3.yaml    |    6 +-
 .../bindings/media/i2c/sony,imx258.yaml       |    1 +
 .../bindings/media/i2c/sony,imx274.yaml       |    4 +
 .../bindings/media/i2c/sony,imx290.yaml       |    5 +-
 .../bindings/media/i2c/toshiba,et8ek8.txt     |    8 +-
 MAINTAINERS                                   |    1 -
 arch/arm/boot/dts/nxp/imx/imx6qdl-pico.dtsi   |    5 +-
 .../boot/dts/nxp/imx/imx6qdl-wandboard.dtsi   |    5 +-
 .../arm/boot/dts/samsung/exynos4210-i9100.dts |    5 +-
 .../boot/dts/samsung/exynos4412-midas.dtsi    |    5 +-
 arch/arm/boot/dts/ti/omap/omap3-n9.dts        |    5 +-
 arch/arm/boot/dts/ti/omap/omap3-n900.dts      |    3 +-
 arch/arm/boot/dts/ti/omap/omap3-n950.dts      |    5 +-
 .../sdm845-db845c-navigation-mezzanine.dtso   |    3 +-
 .../aistarvision-mipi-adapter-2.1.dtsi        |    1 -
 .../dts/renesas/rz-smarc-cru-csi-ov5645.dtsi  |    1 -
 drivers/media/i2c/Kconfig                     |   10 +-
 drivers/media/i2c/Makefile                    |    1 -
 drivers/media/i2c/et8ek8/et8ek8_driver.c      |   27 +-
 drivers/media/i2c/et8ek8/et8ek8_mode.c        |    9 -
 drivers/media/i2c/et8ek8/et8ek8_reg.h         |    1 -
 drivers/media/i2c/gc05a2.c                    |    8 +-
 drivers/media/i2c/gc08a3.c                    |    8 +-
 drivers/media/i2c/hi556.c                     |   92 +-
 drivers/media/i2c/hi847.c                     |   84 +-
 drivers/media/i2c/imx208.c                    |   91 +-
 drivers/media/i2c/imx258.c                    |  105 +-
 drivers/media/i2c/imx290.c                    |   27 +-
 drivers/media/i2c/imx319.c                    |   92 +-
 drivers/media/i2c/imx355.c                    |   90 +-
 drivers/media/i2c/mt9v032.c                   |  104 +-
 drivers/media/i2c/mt9v111.c                   |    2 -
 drivers/media/i2c/og01a1b.c                   |  103 +-
 drivers/media/i2c/ov02a10.c                   |   45 +-
 drivers/media/i2c/ov02c10.c                   |  107 +-
 drivers/media/i2c/ov02e10.c                   |  105 +-
 drivers/media/i2c/ov08d10.c                   |   82 +-
 drivers/media/i2c/ov08x40.c                   |   95 +-
 drivers/media/i2c/ov13858.c                   |   69 +-
 drivers/media/i2c/ov13b10.c                   |  110 +-
 drivers/media/i2c/ov2685.c                    |    8 +-
 drivers/media/i2c/ov2740.c                    |   91 +-
 drivers/media/i2c/ov4689.c                    |   12 +-
 drivers/media/i2c/ov5645.c                    |   13 +-
 drivers/media/i2c/ov5670.c                    |  105 +-
 drivers/media/i2c/ov5675.c                    |   89 +-
 drivers/media/i2c/ov5693.c                    |   16 +-
 drivers/media/i2c/ov5695.c                    |    8 +-
 drivers/media/i2c/ov6650.c                    | 1147 -----------------
 drivers/media/i2c/ov7251.c                    |   26 +-
 drivers/media/i2c/ov8856.c                    |   93 +-
 drivers/media/i2c/ov9734.c                    |   82 +-
 drivers/media/i2c/s5c73m3/s5c73m3-core.c      |   15 +-
 drivers/media/i2c/s5c73m3/s5c73m3.h           |    2 -
 drivers/media/i2c/s5k5baf.c                   |   19 +-
 drivers/media/i2c/s5k6a3.c                    |   17 +-
 drivers/media/v4l2-core/v4l2-common.c         |   39 +-
 include/media/i2c/mt9v022.h                   |   13 -
 include/media/i2c/mt9v032.h                   |   12 -
 include/media/v4l2-common.h                   |   41 +-
 67 files changed, 1031 insertions(+), 2379 deletions(-)
 delete mode 100644 drivers/media/i2c/ov6650.c
 delete mode 100644 include/media/i2c/mt9v022.h
 delete mode 100644 include/media/i2c/mt9v032.h


base-commit: 95703a099e094c00a0714f4d6fa6d9f142ff3fda
prerequisite-patch-id: f9a80c958fab645e237a47c8e3bc19268fe699a0
prerequisite-patch-id: d058d644effe1953c742f6f0c7dd7692c61c733c
prerequisite-patch-id: 7b42fb0bc003a3857d332d404e935c01e917d73b
prerequisite-patch-id: d9b48f03a77c0fe88e60cf88256a395944b99810
prerequisite-patch-id: 9545c613887ee00591c30147553ac785703419e7
prerequisite-patch-id: 43ab82f52c5038df96167d26d63cd838a2d8eb58
prerequisite-patch-id: 182a9155342a92026b0da69532eaac966e2cd7be
prerequisite-patch-id: 2f9595bda17ce11348420cad7e446bc96b79b7e6
prerequisite-patch-id: 5e6e13044a003e00340c29539376d69e6bc1849c
prerequisite-patch-id: 32fcb56cf75ea45ff7cc556839ddbaffda509026
prerequisite-patch-id: 961a751cff6f39f6adad4e03a6dda045ccf42177
prerequisite-patch-id: 8b60d2f5b1e70480a9e6d2177f743a577db6e3ba
prerequisite-patch-id: 8ec9ba9f786d2f33e8b452bd380185351d1d97e1
prerequisite-patch-id: 24683271a88f01fef004788121ed0b0d610079e6
prerequisite-patch-id: f3bc82c9657b3bcdd81703a82efe90a39e598053
prerequisite-patch-id: 493444e38f6b82e080fec5f0e7f63fd95daca8c0
prerequisite-patch-id: 82f6ac141be65b812f142fb0e06f4501be9dc144
prerequisite-patch-id: e75c59c8eccf0248f5c8a9ba6cae398148a8d361
prerequisite-patch-id: 46aff7d253a16ebdc64dd4cb3b30f031579c3053
prerequisite-patch-id: 57e9cabf0ce8bda498fe2f16db6dd4a9d748d2fd
prerequisite-patch-id: 23781810738fe92eb26cb67ba16f554f58a6f2e4
prerequisite-patch-id: 3c82cca940d393e6ef78b79d7e065dbc62f03be9
prerequisite-patch-id: 52f23cc4ccfc514c7d24e9ac52ac537ba2256a51
prerequisite-patch-id: 2914df66fa49278e9163df769a810142652571b9
prerequisite-patch-id: a7d5dc1aa489556bdcd92d66d9ea78741a9a5d5c
prerequisite-patch-id: ac630eb405db2ea63c8ce3d037d0fa1ba635a397
prerequisite-patch-id: a4ea26b410045f28858d860bc382f6ec976be6d6
prerequisite-patch-id: 8e05fa6d4b7f0ae3feb21a8cfba71bf119d7ad8c
prerequisite-patch-id: e2db4be18a1c859947c90253b462b5d50611a4be
prerequisite-patch-id: 0fa8b8720b573929806bd35e3edc51f0544237ff
prerequisite-patch-id: 69f5743bca8d89adc28a35f3461d1c87e071e671
prerequisite-patch-id: 3076e3da660a8eef7f6f6dafb5fdd2bc2c83d754
prerequisite-patch-id: 09230d2fca61e552cc845242ed326b8eaf3514a4
prerequisite-patch-id: 63f0dc4815777b5005046c90c3fb1fe8ceb88664
prerequisite-patch-id: 012483569daea09f783c6f9a7c5c49bebb8959b0
prerequisite-patch-id: 7fe0df4335963fbf03535080ad24a4b31381c298
prerequisite-patch-id: e0b1ca15da78cb7c2acf58c6649cc26a2901ca28
prerequisite-patch-id: 8cc20810597925a8a0e52f7ed58e821c035244bf
prerequisite-patch-id: 4338a1728052ff14c029330de8f5efa65a69b3aa
prerequisite-patch-id: 198fd889e6f81ff249e56dd1a099cdf2527aef1f
prerequisite-patch-id: 9a6bef3b3dcf639f83b1255e639c99fb71f19008
prerequisite-patch-id: 57ccbadda2f33f6883289444ed7a3dbefaa9687d
prerequisite-patch-id: 9fa098d9e2757f54a616d1e7568a19f29d638d7a
prerequisite-patch-id: ef328feb7636c96629f030601b9ee950b4d62a42
prerequisite-patch-id: f9e70cc31c525d4344fc9e6e54ff2e1e3f5d80d0
prerequisite-patch-id: 9f1a026d73ac5fe39f8799915cf013843bd216aa
prerequisite-patch-id: 692b78c72fbe969fa8903982ebd6da61bd598550
prerequisite-patch-id: 2acccc54dd0fdeee252ba8eafbc16dd9a5cab0c1
-- 
Regards,

Laurent Pinchart

[PATCH 07/72] ARM: dts: samsung: exynos4210-i9100: Replace clock-frequency in camera sensor node

[Laurent Pinchart]

The clock-frequency for camera sensors has been deprecated in favour of
the assigned-clocks and assigned-clock-rates properties. Replace it in
the device tree.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 arch/arm/boot/dts/samsung/exynos4210-i9100.dts | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/samsung/exynos4210-i9100.dts b/arch/arm/boot/dts/samsung/exynos4210-i9100.dts
index 0d8495792a70..cdc768a87757 100644
--- a/arch/arm/boot/dts/samsung/exynos4210-i9100.dts
+++ b/arch/arm/boot/dts/samsung/exynos4210-i9100.dts
@@ -169,11 +169,14 @@ image-sensor@2d {
 			vdda-supply = <&cam_io_en_reg>;
 			vddreg-supply = <&vt_core_15v_reg>;
 			vddio-supply = <&vtcam_reg>;
+
 			clocks = <&camera 0>;
 			clock-names = "mclk";
+			assigned-clocks = <&camera 0>;
+			assigned-clock-rates = <24000000>;
+
 			stbyn-gpios = <&gpl2 0 GPIO_ACTIVE_LOW>;
 			rstn-gpios = <&gpl2 1 GPIO_ACTIVE_LOW>;
-			clock-frequency = <24000000>;
 
 			port {
 				s5k5bafx_ep: endpoint {
-- 
Regards,

Laurent Pinchart

[PATCH 08/72] ARM: dts: samsung: exynos4412-midas: Replace clock-frequency in camera sensor node

[Laurent Pinchart]

The clock-frequency for camera sensors has been deprecated in favour of
the assigned-clocks and assigned-clock-rates properties. Replace it in
the device tree.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 arch/arm/boot/dts/samsung/exynos4412-midas.dtsi | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/samsung/exynos4412-midas.dtsi b/arch/arm/boot/dts/samsung/exynos4412-midas.dtsi
index 3d5aace668dc..06eaf351fa3a 100644
--- a/arch/arm/boot/dts/samsung/exynos4412-midas.dtsi
+++ b/arch/arm/boot/dts/samsung/exynos4412-midas.dtsi
@@ -638,10 +638,13 @@ image-sensor@10 {
 		svdda-supply = <&cam_io_reg>;
 		svddio-supply = <&ldo19_reg>;
 		afvdd-supply = <&ldo19_reg>;
-		clock-frequency = <24000000>;
+
 		/* CAM_B_CLKOUT */
 		clocks = <&camera 1>;
 		clock-names = "extclk";
+		assigned-clocks = <&camera 1>;
+		assigned-clock-rates = <24000000>;
+
 		gpios = <&gpm1 6 GPIO_ACTIVE_LOW>;
 
 		port {
-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Tarang Raval]

Hi Laurent,

> This patch series build on top of Mehdi's introduction of the
> devm_v4l2_sensor_clk_get() helper (see [1]) to drastically reduce
> cargo-cult in camera sensor drivers.
> 
> A large number of camera sensor drivers directly use the
> "clock-frequency" property to retrieve the effective or desired external
> clock rate. This is standard behaviour on ACPI platforms that don't
> implement MIPI DisCo for Imaging, but usage of the property has leaked
> to OF-based platforms, due to a combination of historical reasons (using
> "clock-frequency" was initially considered right until before the
> introduction of "assigned-clock-rates") and plain cargo-cult.
> 
> A large number of camera sensor drivers also set the rate of the
> external clock with clk_set_rate(). This behaviour is also fine on ACPI
> platforms, and has also leaked to OF-based platforms for the same
> reasons.
> 
> Mehdi's "[PATCH v2 00/48] media: Add a helper for obtaining the clock
> producer" series improves the situation by centralizing clock handling
> for camera sensor in one helper function that implements the correct
> behaviour for all types of platforms (and should later allow support of
> MIPI DisCo for Imaging transparently for camera sensor drivers). It
> doesn't however address direct access of the "clock-frequency" property
> or direct calls to clk_set_rate() in drivers.
> 
> This series builds on top of the new helper to replace manual handling
> of the clock frequency in camera sensor drivers. It starts by addressing
> the DT bindings and reprecating the clock-frequency property for camera
> sensor drivers in all YAML bindings (01/72) and in the et8ek8 text
> bindings (02/72). After that, patches 03/72 and 04/72 make the clocks
> property mandatory in the two camera sensor DT bindings that specified
> it as optional. Finally for the DT side, patches 05/72 to 14/72 replace
> clock-frequency with assigned-clock-rates, or drops the property
> altogether when the source clock has a fixed rate. This aligns the DT
> bindings and device tree sources to the current recommended practice.
> 
> After that, the next 5 patches are assorted drive-by changes. Patch
> 15/72 drops an unused header the belonged to a long gone driver, and
> patch 17/72 drops unusued support for platform data in the mt9v032
> driver. Patch 18/72 is the first that addresses clock rate handling by
> dropping unneeded clock rate setting in the mt9v111 driver. Patch 19/72
> takes a harsher approach for the ov6650 by dropping the driver
> completely as the driver hasn't been used since v5.9.
> 
> The next part of the series replaces manual clock rate handling with
> usage of the devm_v4l2_sensor_clk_get() helper in a large number of
> camera sensor drivers that implement clock rate handling in a standard
> way. This is done in patches 20/72 to 54/72. This interleaves the clock
> rate handling changes with drive-by refactoring (in separate patches) to
> make the code easier to deal with.
> 
> The final part of the series addresses the remaining drivers that
> implement non-standard behaviours. It starts in 55/72 by adding a new
> devm_v4l2_sensor_clk_get_legacy() helper function for those drivers,
> similar to devm_v4l2_sensor_clk_get() but with a few more quirks. This
> function should not be used in any new driver. The remaining patches,
> from 53/72 to 72/72, use the new helper in drivers, interleaved with
> drive-by refactoring similarly to the previous part.
> 
> Before this series, with Mehdi's series applied, 29 drivers read the
> "clock-frequency" property and 18 drivers set the external clock rate.
> With these series we go down to 1 and 3 respectively, namely the ccs,
> mt9p031 and mt9v032 drivers. Clock handling in the CCS driver is a bit
> more convoluted so I will leave to Sakari the honour of dropping the
> last direct user of "clock-frequency" :-) As for the mt9p031 and mt9v032
> driver, addressing the issue there is more difficult and likely not
> worth it.
> 
> [1] https://lore.kernel.org/linux-media/cover.1750942967.git.mehdi.djait@linux.intel.com
> 
> Laurent Pinchart (72):
>   dt-bindings: media: Deprecate clock-frequency property for camera
>     sensors
>   dt-bindings: media: et8ek8: Deprecate clock-frequency property
>   dt-bindings: media: imx258: Make clocks property required
>   dt-bindings: media: imx274: Make clocks property required
>   ARM: dts: nxp: imx6qdl-pico: Replace clock-frequency in camera sensor
>     node
>   ARM: dts: nxp: imx6qdl-wandboard: Replace clock-frequency in camera
>     sensor node
>   ARM: dts: samsung: exynos4210-i9100: Replace clock-frequency in camera
>     sensor node
>   ARM: dts: samsung: exynos4412-midas: Replace clock-frequency in camera
>     sensor node
>   ARM: dts: ti: omap3-n900: Replace clock-frequency in camera sensor
>     node
>   ARM: dts: ti: omap3-n950: Replace clock-frequency in camera sensor
>     node
>   ARM: dts: ti: omap3-n9: Replace clock-frequency in camera sensor node
>   arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace
>     clock-frequency in camera sensor node
>   arm64: dts: renesas: aistarvision-mipi-adapter-2.1: Drop
>     clock-frequency from camera sensor node
>   arm64: dts: renesas: rzg2l-smarc: Drop clock-frequency from camera
>     sensor node
>   media: i2c: mt9v022: Drop unused mt9v022.h header
>   media: i2c: mt9v032: Replace client->dev usage
>   media: i2c: mt9v032: Drop support for platform data
>   media: i2c: mt9v111: Do not set clock rate manually
>   media: i2c: ov6650: Drop unused driver
>   media: i2c: hi556: Replace client->dev usage
>   media: i2c: hi556: Use V4L2 sensor clock helper
>   media: i2c: hi847: Replace client->dev usage
>   media: i2c: hi847: Use V4L2 sensor clock helper
>   media: i2c: imx208: Replace client->dev usage
>   media: i2c: imx208: Use V4L2 sensor clock helper
>   media: i2c: imx319: Replace client->dev usage
>   media: i2c: imx319: Use V4L2 sensor clock helper
>   media: i2c: imx355: Replace client->dev usage
>   media: i2c: imx335: Use V4L2 sensor clock helper
>   media: i2c: og01a1b: Replace client->dev usage
>   media: i2c: og01a1b: Use V4L2 sensor clock helper
>   media: i2c: ov02c10: Replace client->dev usage
>   media: i2c: ov02c10: Use V4L2 sensor clock helper
>   media: i2c: ov02e10: Replace client->dev usage
>   media: i2c: ov02e10: Use V4L2 sensor clock helper
>   media: i2c: ov08d10: Replace client->dev usage
>   media: i2c: ov08d10: Use V4L2 sensor clock helper
>   media: i2c: ov08x40: Replace client->dev usage
>   media: i2c: ov08x40: Use V4L2 sensor clock helper
>   media: i2c: ov13858: Replace client->dev usage
>   media: i2c: ov13858: Use V4L2 sensor clock helper
>   media: i2c: ov13b10: Replace client->dev usage
>   media: i2c: ov13b10: Use V4L2 sensor clock helper
>   media: i2c: ov2740: Replace client->dev usage
>   media: i2c: ov2740: Use V4L2 sensor clock helper
>   media: i2c: ov4689: Use V4L2 sensor clock helper
>   media: i2c: ov5670: Replace client->dev usage
>   media: i2c: ov5670: Use V4L2 sensor clock helper
>   media: i2c: ov5675: Replace client->dev usage
>   media: i2c: ov5675: Use V4L2 sensor clock helper
>   media: i2c: ov5693: Use V4L2 sensor clock helper
>   media: i2c: ov7251: Use V4L2 sensor clock helper
>   media: i2c: ov9734: Replace client->dev usage
>   media: i2c: ov9734: Use V4L2 sensor clock helper
>   media: v4l2-common: Add legacy camera sensor clock helper
>   media: i2c: et8ek8: Drop support for per-mode external clock frequency
>   media: i2c: et8ek8: Use V4L2 legacy sensor clock helper
>   media: i2c: gc05a2: Use V4L2 legacy sensor clock helper
>   media: i2c: gc08a3: Use V4L2 legacy sensor clock helper
>   media: i2c: imx258: Replace client->dev usage
>   media: i2c: imx258: Use V4L2 legacy sensor clock helper
>   media: i2c: imx290: Use V4L2 legacy sensor clock helper
>   media: i2c: ov02a10: Replace client->dev usage
>   media: i2c: ov02a10: Use V4L2 legacy sensor clock helper
>   media: i2c: ov2685: Use V4L2 legacy sensor clock helper
>   media: i2c: ov5645: Use V4L2 legacy sensor clock helper
>   media: i2c: ov5695: Use V4L2 legacy sensor clock helper
>   media: i2c: ov8856: Replace client->dev usage
>   media: i2c: ov8856: Use V4L2 legacy sensor clock helper
>   media: i2c: s5c73m3: Use V4L2 legacy sensor clock helper
>   media: i2c: s5k5baf: Use V4L2 legacy sensor clock helper
>   media: i2c: s5k6a3: Use V4L2 legacy sensor clock helper

If you are planning a v2 version of this patch series, please consider
incorporating the following improvements:

1. In the imx219 driver, you can also replace direct client->dev usage.
2. In the regulator code, you can reduce boilerplate by using 
   devm_regulator_bulk_get_enable().

Otherwise, I will submit a separate patch series on top of yours to 
address these points.

Best Regards,
Tarang

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart]

On Thu, Jul 24, 2025 at 11:42:55AM +0000, Tarang Raval wrote:
> Hi Laurent,
> 
> > This patch series build on top of Mehdi's introduction of the
> > devm_v4l2_sensor_clk_get() helper (see [1]) to drastically reduce
> > cargo-cult in camera sensor drivers.
> > 
> > A large number of camera sensor drivers directly use the
> > "clock-frequency" property to retrieve the effective or desired external
> > clock rate. This is standard behaviour on ACPI platforms that don't
> > implement MIPI DisCo for Imaging, but usage of the property has leaked
> > to OF-based platforms, due to a combination of historical reasons (using
> > "clock-frequency" was initially considered right until before the
> > introduction of "assigned-clock-rates") and plain cargo-cult.
> > 
> > A large number of camera sensor drivers also set the rate of the
> > external clock with clk_set_rate(). This behaviour is also fine on ACPI
> > platforms, and has also leaked to OF-based platforms for the same
> > reasons.
> > 
> > Mehdi's "[PATCH v2 00/48] media: Add a helper for obtaining the clock
> > producer" series improves the situation by centralizing clock handling
> > for camera sensor in one helper function that implements the correct
> > behaviour for all types of platforms (and should later allow support of
> > MIPI DisCo for Imaging transparently for camera sensor drivers). It
> > doesn't however address direct access of the "clock-frequency" property
> > or direct calls to clk_set_rate() in drivers.
> > 
> > This series builds on top of the new helper to replace manual handling
> > of the clock frequency in camera sensor drivers. It starts by addressing
> > the DT bindings and reprecating the clock-frequency property for camera
> > sensor drivers in all YAML bindings (01/72) and in the et8ek8 text
> > bindings (02/72). After that, patches 03/72 and 04/72 make the clocks
> > property mandatory in the two camera sensor DT bindings that specified
> > it as optional. Finally for the DT side, patches 05/72 to 14/72 replace
> > clock-frequency with assigned-clock-rates, or drops the property
> > altogether when the source clock has a fixed rate. This aligns the DT
> > bindings and device tree sources to the current recommended practice.
> > 
> > After that, the next 5 patches are assorted drive-by changes. Patch
> > 15/72 drops an unused header the belonged to a long gone driver, and
> > patch 17/72 drops unusued support for platform data in the mt9v032
> > driver. Patch 18/72 is the first that addresses clock rate handling by
> > dropping unneeded clock rate setting in the mt9v111 driver. Patch 19/72
> > takes a harsher approach for the ov6650 by dropping the driver
> > completely as the driver hasn't been used since v5.9.
> > 
> > The next part of the series replaces manual clock rate handling with
> > usage of the devm_v4l2_sensor_clk_get() helper in a large number of
> > camera sensor drivers that implement clock rate handling in a standard
> > way. This is done in patches 20/72 to 54/72. This interleaves the clock
> > rate handling changes with drive-by refactoring (in separate patches) to
> > make the code easier to deal with.
> > 
> > The final part of the series addresses the remaining drivers that
> > implement non-standard behaviours. It starts in 55/72 by adding a new
> > devm_v4l2_sensor_clk_get_legacy() helper function for those drivers,
> > similar to devm_v4l2_sensor_clk_get() but with a few more quirks. This
> > function should not be used in any new driver. The remaining patches,
> > from 53/72 to 72/72, use the new helper in drivers, interleaved with
> > drive-by refactoring similarly to the previous part.
> > 
> > Before this series, with Mehdi's series applied, 29 drivers read the
> > "clock-frequency" property and 18 drivers set the external clock rate.
> > With these series we go down to 1 and 3 respectively, namely the ccs,
> > mt9p031 and mt9v032 drivers. Clock handling in the CCS driver is a bit
> > more convoluted so I will leave to Sakari the honour of dropping the
> > last direct user of "clock-frequency" :-) As for the mt9p031 and mt9v032
> > driver, addressing the issue there is more difficult and likely not
> > worth it.
> > 
> > [1] https://lore.kernel.org/linux-media/cover.1750942967.git.mehdi.djait@linux.intel.com
> > 
> > Laurent Pinchart (72):
> >   dt-bindings: media: Deprecate clock-frequency property for camera
> >     sensors
> >   dt-bindings: media: et8ek8: Deprecate clock-frequency property
> >   dt-bindings: media: imx258: Make clocks property required
> >   dt-bindings: media: imx274: Make clocks property required
> >   ARM: dts: nxp: imx6qdl-pico: Replace clock-frequency in camera sensor
> >     node
> >   ARM: dts: nxp: imx6qdl-wandboard: Replace clock-frequency in camera
> >     sensor node
> >   ARM: dts: samsung: exynos4210-i9100: Replace clock-frequency in camera
> >     sensor node
> >   ARM: dts: samsung: exynos4412-midas: Replace clock-frequency in camera
> >     sensor node
> >   ARM: dts: ti: omap3-n900: Replace clock-frequency in camera sensor
> >     node
> >   ARM: dts: ti: omap3-n950: Replace clock-frequency in camera sensor
> >     node
> >   ARM: dts: ti: omap3-n9: Replace clock-frequency in camera sensor node
> >   arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace
> >     clock-frequency in camera sensor node
> >   arm64: dts: renesas: aistarvision-mipi-adapter-2.1: Drop
> >     clock-frequency from camera sensor node
> >   arm64: dts: renesas: rzg2l-smarc: Drop clock-frequency from camera
> >     sensor node
> >   media: i2c: mt9v022: Drop unused mt9v022.h header
> >   media: i2c: mt9v032: Replace client->dev usage
> >   media: i2c: mt9v032: Drop support for platform data
> >   media: i2c: mt9v111: Do not set clock rate manually
> >   media: i2c: ov6650: Drop unused driver
> >   media: i2c: hi556: Replace client->dev usage
> >   media: i2c: hi556: Use V4L2 sensor clock helper
> >   media: i2c: hi847: Replace client->dev usage
> >   media: i2c: hi847: Use V4L2 sensor clock helper
> >   media: i2c: imx208: Replace client->dev usage
> >   media: i2c: imx208: Use V4L2 sensor clock helper
> >   media: i2c: imx319: Replace client->dev usage
> >   media: i2c: imx319: Use V4L2 sensor clock helper
> >   media: i2c: imx355: Replace client->dev usage
> >   media: i2c: imx335: Use V4L2 sensor clock helper
> >   media: i2c: og01a1b: Replace client->dev usage
> >   media: i2c: og01a1b: Use V4L2 sensor clock helper
> >   media: i2c: ov02c10: Replace client->dev usage
> >   media: i2c: ov02c10: Use V4L2 sensor clock helper
> >   media: i2c: ov02e10: Replace client->dev usage
> >   media: i2c: ov02e10: Use V4L2 sensor clock helper
> >   media: i2c: ov08d10: Replace client->dev usage
> >   media: i2c: ov08d10: Use V4L2 sensor clock helper
> >   media: i2c: ov08x40: Replace client->dev usage
> >   media: i2c: ov08x40: Use V4L2 sensor clock helper
> >   media: i2c: ov13858: Replace client->dev usage
> >   media: i2c: ov13858: Use V4L2 sensor clock helper
> >   media: i2c: ov13b10: Replace client->dev usage
> >   media: i2c: ov13b10: Use V4L2 sensor clock helper
> >   media: i2c: ov2740: Replace client->dev usage
> >   media: i2c: ov2740: Use V4L2 sensor clock helper
> >   media: i2c: ov4689: Use V4L2 sensor clock helper
> >   media: i2c: ov5670: Replace client->dev usage
> >   media: i2c: ov5670: Use V4L2 sensor clock helper
> >   media: i2c: ov5675: Replace client->dev usage
> >   media: i2c: ov5675: Use V4L2 sensor clock helper
> >   media: i2c: ov5693: Use V4L2 sensor clock helper
> >   media: i2c: ov7251: Use V4L2 sensor clock helper
> >   media: i2c: ov9734: Replace client->dev usage
> >   media: i2c: ov9734: Use V4L2 sensor clock helper
> >   media: v4l2-common: Add legacy camera sensor clock helper
> >   media: i2c: et8ek8: Drop support for per-mode external clock frequency
> >   media: i2c: et8ek8: Use V4L2 legacy sensor clock helper
> >   media: i2c: gc05a2: Use V4L2 legacy sensor clock helper
> >   media: i2c: gc08a3: Use V4L2 legacy sensor clock helper
> >   media: i2c: imx258: Replace client->dev usage
> >   media: i2c: imx258: Use V4L2 legacy sensor clock helper
> >   media: i2c: imx290: Use V4L2 legacy sensor clock helper
> >   media: i2c: ov02a10: Replace client->dev usage
> >   media: i2c: ov02a10: Use V4L2 legacy sensor clock helper
> >   media: i2c: ov2685: Use V4L2 legacy sensor clock helper
> >   media: i2c: ov5645: Use V4L2 legacy sensor clock helper
> >   media: i2c: ov5695: Use V4L2 legacy sensor clock helper
> >   media: i2c: ov8856: Replace client->dev usage
> >   media: i2c: ov8856: Use V4L2 legacy sensor clock helper
> >   media: i2c: s5c73m3: Use V4L2 legacy sensor clock helper
> >   media: i2c: s5k5baf: Use V4L2 legacy sensor clock helper
> >   media: i2c: s5k6a3: Use V4L2 legacy sensor clock helper
> 
> If you are planning a v2 version of this patch series, please consider
> incorporating the following improvements:
> 
> 1. In the imx219 driver, you can also replace direct client->dev usage.

The series doesn't touch the imx219 driver. The patches that replace
client->dev usage were meant to simplify the other changes. Additional
client->dev removal should be done on top (and likely through all camera
sensor drivers in one go).

> 2. In the regulator code, you can reduce boilerplate by using 
>    devm_regulator_bulk_get_enable().

devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
generally don't want to enable power everywhere unconditionally, and
sensors very often need a guaranteed power up sequence.

> Otherwise, I will submit a separate patch series on top of yours to 
> address these points.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

On Thu, Jul 24, 2025 at 01:24:24PM +0000, Tarang Raval wrote:

> > > 2. In the regulator code, you can reduce boilerplate by using
> > >    devm_regulator_bulk_get_enable().

> > devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
> > generally don't want to enable power everywhere unconditionally, and
> > sensors very often need a guaranteed power up sequence.

> The regulators are optional, we supply power to the camera sensor directly
> through dedicated power rails and there is no strict enable sequence
> required in this case.

Those dedicated power rails getting their power from....

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart]

On Thu, Jul 24, 2025 at 01:24:24PM +0000, Tarang Raval wrote:
> > On Thu, Jul 24, 2025 at 11:42:55AM +0000, Tarang Raval wrote:
> > > Hi Laurent,
> > >
> > > > This patch series build on top of Mehdi's introduction of the
> > > > devm_v4l2_sensor_clk_get() helper (see [1]) to drastically reduce
> > > > cargo-cult in camera sensor drivers.
> > > >
> > > > A large number of camera sensor drivers directly use the
> > > > "clock-frequency" property to retrieve the effective or desired external
> > > > clock rate. This is standard behaviour on ACPI platforms that don't
> > > > implement MIPI DisCo for Imaging, but usage of the property has leaked
> > > > to OF-based platforms, due to a combination of historical reasons (using
> > > > "clock-frequency" was initially considered right until before the
> > > > introduction of "assigned-clock-rates") and plain cargo-cult.
> > > >
> > > > A large number of camera sensor drivers also set the rate of the
> > > > external clock with clk_set_rate(). This behaviour is also fine on ACPI
> > > > platforms, and has also leaked to OF-based platforms for the same
> > > > reasons.
> > > >
> > > > Mehdi's "[PATCH v2 00/48] media: Add a helper for obtaining the clock
> > > > producer" series improves the situation by centralizing clock handling
> > > > for camera sensor in one helper function that implements the correct
> > > > behaviour for all types of platforms (and should later allow support of
> > > > MIPI DisCo for Imaging transparently for camera sensor drivers). It
> > > > doesn't however address direct access of the "clock-frequency" property
> > > > or direct calls to clk_set_rate() in drivers.
> > > >
> > > > This series builds on top of the new helper to replace manual handling
> > > > of the clock frequency in camera sensor drivers. It starts by addressing
> > > > the DT bindings and reprecating the clock-frequency property for camera
> > > > sensor drivers in all YAML bindings (01/72) and in the et8ek8 text
> > > > bindings (02/72). After that, patches 03/72 and 04/72 make the clocks
> > > > property mandatory in the two camera sensor DT bindings that specified
> > > > it as optional. Finally for the DT side, patches 05/72 to 14/72 replace
> > > > clock-frequency with assigned-clock-rates, or drops the property
> > > > altogether when the source clock has a fixed rate. This aligns the DT
> > > > bindings and device tree sources to the current recommended practice.
> > > >
> > > > After that, the next 5 patches are assorted drive-by changes. Patch
> > > > 15/72 drops an unused header the belonged to a long gone driver, and
> > > > patch 17/72 drops unusued support for platform data in the mt9v032
> > > > driver. Patch 18/72 is the first that addresses clock rate handling by
> > > > dropping unneeded clock rate setting in the mt9v111 driver. Patch 19/72
> > > > takes a harsher approach for the ov6650 by dropping the driver
> > > > completely as the driver hasn't been used since v5.9.
> > > >
> > > > The next part of the series replaces manual clock rate handling with
> > > > usage of the devm_v4l2_sensor_clk_get() helper in a large number of
> > > > camera sensor drivers that implement clock rate handling in a standard
> > > > way. This is done in patches 20/72 to 54/72. This interleaves the clock
> > > > rate handling changes with drive-by refactoring (in separate patches) to
> > > > make the code easier to deal with.
> > > >
> > > > The final part of the series addresses the remaining drivers that
> > > > implement non-standard behaviours. It starts in 55/72 by adding a new
> > > > devm_v4l2_sensor_clk_get_legacy() helper function for those drivers,
> > > > similar to devm_v4l2_sensor_clk_get() but with a few more quirks. This
> > > > function should not be used in any new driver. The remaining patches,
> > > > from 53/72 to 72/72, use the new helper in drivers, interleaved with
> > > > drive-by refactoring similarly to the previous part.
> > > >
> > > > Before this series, with Mehdi's series applied, 29 drivers read the
> > > > "clock-frequency" property and 18 drivers set the external clock rate.
> > > > With these series we go down to 1 and 3 respectively, namely the ccs,
> > > > mt9p031 and mt9v032 drivers. Clock handling in the CCS driver is a bit
> > > > more convoluted so I will leave to Sakari the honour of dropping the
> > > > last direct user of "clock-frequency" :-) As for the mt9p031 and mt9v032
> > > > driver, addressing the issue there is more difficult and likely not
> > > > worth it.
> > > >
> > > > [1] https://lore.kernel.org/linux-media/
> cover.1750942967.git.mehdi.djait@linux.intel.com
> > > >
> > > > Laurent Pinchart (72):
> > > >   dt-bindings: media: Deprecate clock-frequency property for camera sensors
> > > >   dt-bindings: media: et8ek8: Deprecate clock-frequency property
> > > >   dt-bindings: media: imx258: Make clocks property required
> > > >   dt-bindings: media: imx274: Make clocks property required
> > > >   ARM: dts: nxp: imx6qdl-pico: Replace clock-frequency in camera sensor node
> > > >   ARM: dts: nxp: imx6qdl-wandboard: Replace clock-frequency in camera sensor node
> > > >   ARM: dts: samsung: exynos4210-i9100: Replace clock-frequency in camera sensor node
> > > >   ARM: dts: samsung: exynos4412-midas: Replace clock-frequency in camera sensor node
> > > >   ARM: dts: ti: omap3-n900: Replace clock-frequency in camera sensor node
> > > >   ARM: dts: ti: omap3-n950: Replace clock-frequency in camera sensor node
> > > >   ARM: dts: ti: omap3-n9: Replace clock-frequency in camera sensor node
> > > >   arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace clock-frequency in camera sensor node
> > > >   arm64: dts: renesas: aistarvision-mipi-adapter-2.1: Drop clock-frequency from camera sensor node
> > > >   arm64: dts: renesas: rzg2l-smarc: Drop clock-frequency from camera sensor node
> > > >   media: i2c: mt9v022: Drop unused mt9v022.h header
> > > >   media: i2c: mt9v032: Replace client->dev usage
> > > >   media: i2c: mt9v032: Drop support for platform data
> > > >   media: i2c: mt9v111: Do not set clock rate manually
> > > >   media: i2c: ov6650: Drop unused driver
> > > >   media: i2c: hi556: Replace client->dev usage
> > > >   media: i2c: hi556: Use V4L2 sensor clock helper
> > > >   media: i2c: hi847: Replace client->dev usage
> > > >   media: i2c: hi847: Use V4L2 sensor clock helper
> > > >   media: i2c: imx208: Replace client->dev usage
> > > >   media: i2c: imx208: Use V4L2 sensor clock helper
> > > >   media: i2c: imx319: Replace client->dev usage
> > > >   media: i2c: imx319: Use V4L2 sensor clock helper
> > > >   media: i2c: imx355: Replace client->dev usage
> > > >   media: i2c: imx335: Use V4L2 sensor clock helper
> > > >   media: i2c: og01a1b: Replace client->dev usage
> > > >   media: i2c: og01a1b: Use V4L2 sensor clock helper
> > > >   media: i2c: ov02c10: Replace client->dev usage
> > > >   media: i2c: ov02c10: Use V4L2 sensor clock helper
> > > >   media: i2c: ov02e10: Replace client->dev usage
> > > >   media: i2c: ov02e10: Use V4L2 sensor clock helper
> > > >   media: i2c: ov08d10: Replace client->dev usage
> > > >   media: i2c: ov08d10: Use V4L2 sensor clock helper
> > > >   media: i2c: ov08x40: Replace client->dev usage
> > > >   media: i2c: ov08x40: Use V4L2 sensor clock helper
> > > >   media: i2c: ov13858: Replace client->dev usage
> > > >   media: i2c: ov13858: Use V4L2 sensor clock helper
> > > >   media: i2c: ov13b10: Replace client->dev usage
> > > >   media: i2c: ov13b10: Use V4L2 sensor clock helper
> > > >   media: i2c: ov2740: Replace client->dev usage
> > > >   media: i2c: ov2740: Use V4L2 sensor clock helper
> > > >   media: i2c: ov4689: Use V4L2 sensor clock helper
> > > >   media: i2c: ov5670: Replace client->dev usage
> > > >   media: i2c: ov5670: Use V4L2 sensor clock helper
> > > >   media: i2c: ov5675: Replace client->dev usage
> > > >   media: i2c: ov5675: Use V4L2 sensor clock helper
> > > >   media: i2c: ov5693: Use V4L2 sensor clock helper
> > > >   media: i2c: ov7251: Use V4L2 sensor clock helper
> > > >   media: i2c: ov9734: Replace client->dev usage
> > > >   media: i2c: ov9734: Use V4L2 sensor clock helper
> > > >   media: v4l2-common: Add legacy camera sensor clock helper
> > > >   media: i2c: et8ek8: Drop support for per-mode external clock frequency
> > > >   media: i2c: et8ek8: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: gc05a2: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: gc08a3: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: imx258: Replace client->dev usage
> > > >   media: i2c: imx258: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: imx290: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: ov02a10: Replace client->dev usage
> > > >   media: i2c: ov02a10: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: ov2685: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: ov5645: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: ov5695: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: ov8856: Replace client->dev usage
> > > >   media: i2c: ov8856: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: s5c73m3: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: s5k5baf: Use V4L2 legacy sensor clock helper
> > > >   media: i2c: s5k6a3: Use V4L2 legacy sensor clock helper
> > >
> > > If you are planning a v2 version of this patch series, please consider
> > > incorporating the following improvements:
> > >
> > > 1. In the imx219 driver, you can also replace direct client->dev usage.
> > 
> > The series doesn't touch the imx219 driver. The patches that replace
> > client->dev usage were meant to simplify the other changes. Additional
> > client->dev removal should be done on top (and likely through all camera
> > sensor drivers in one go).
> 
> Okay, great
> 
> > > 2. In the regulator code, you can reduce boilerplate by using
> > >    devm_regulator_bulk_get_enable().
> > 
> > devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
> > generally don't want to enable power everywhere unconditionally, and
> > sensors very often need a guaranteed power up sequence.
> 
> The regulators are optional, we supply power to the camera sensor directly
> through dedicated power rails and there is no strict enable sequence 
> required in this case.

What exactly do you mean by "this case" ? Are you talking about one
particular sensor ? One particular camera module ?

> However, if you feel it's better to retain explicit handling for clarity 
> and flexibility, I’m happy to stick with the current approach.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Tarang Raval]

> > > > 2. In the regulator code, you can reduce boilerplate by using
> > > >    devm_regulator_bulk_get_enable().
> > >
> > > devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
> > > generally don't want to enable power everywhere unconditionally, and
> > > sensors very often need a guaranteed power up sequence.
> >
> > The regulators are optional, we supply power to the camera sensor directly
> > through dedicated power rails and there is no strict enable sequence
> > required in this case.
> 
> What exactly do you mean by "this case" ? Are you talking about one
> particular sensor ? One particular camera module ?

Laurent, by “this case” I meant the common scenario where power to the 
camera sensor is supplied by a PMIC regulator that is always-on. In such 
setups, the regulator is fixed and cannot be enabled or disabled from the 
driver, the sensor is always powered.

This is what I’ve seen in most platforms, where the CSI input connector 
provides fixed 3.3V/1.8V power rails directly to the camera module.

Of course, if the camera supply comes from a dedicated regulator controlled 
via a GPIO, then the driver would need to handle enable/disable sequencing 
explicitly. But I’m specifically referring to the first case, where the power rails 
are always-on.

Mark, depending on the hardware, the power rails could come either from a 
PMIC or from a regulator controlled by GPIO, but I’m talking about the always-on 
PMIC case here.

Best Regards,
Tarang

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 418 bytes --]

On Thu, Jul 24, 2025 at 02:20:10PM +0000, Tarang Raval wrote:

> Mark, depending on the hardware, the power rails could come either from a 
> PMIC or from a regulator controlled by GPIO, but I’m talking about the always-on 
> PMIC case here.

All of those things are regulators.  My point is that a supply being
optional means it can be physically absent from the device which does
happen but quite rarely.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart]

On Thu, Jul 24, 2025 at 02:20:10PM +0000, Tarang Raval wrote:
> > > > > 2. In the regulator code, you can reduce boilerplate by using
> > > > >    devm_regulator_bulk_get_enable().
> > > >
> > > > devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
> > > > generally don't want to enable power everywhere unconditionally, and
> > > > sensors very often need a guaranteed power up sequence.
> > >
> > > The regulators are optional, we supply power to the camera sensor directly
> > > through dedicated power rails and there is no strict enable sequence
> > > required in this case.
> > 
> > What exactly do you mean by "this case" ? Are you talking about one
> > particular sensor ? One particular camera module ?
> 
> Laurent, by “this case” I meant the common scenario where power to the 
> camera sensor is supplied by a PMIC regulator that is always-on. In such 
> setups, the regulator is fixed and cannot be enabled or disabled from the 
> driver, the sensor is always powered.
> 
> This is what I’ve seen in most platforms, where the CSI input connector 
> provides fixed 3.3V/1.8V power rails directly to the camera module.
> 
> Of course, if the camera supply comes from a dedicated regulator controlled 
> via a GPIO, then the driver would need to handle enable/disable sequencing 
> explicitly. But I’m specifically referring to the first case, where the power rails 
> are always-on.

How does the sensor driver know which of those two cases it is dealing
with ?

> Mark, depending on the hardware, the power rails could come either from a 
> PMIC or from a regulator controlled by GPIO, but I’m talking about the always-on 
> PMIC case here.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Tarang Raval]

> On Thu, Jul 24, 2025 at 02:20:10PM +0000, Tarang Raval wrote:
> > > > > > 2. In the regulator code, you can reduce boilerplate by using
> > > > > >    devm_regulator_bulk_get_enable().
> > > > >
> > > > > devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
> > > > > generally don't want to enable power everywhere unconditionally, and
> > > > > sensors very often need a guaranteed power up sequence.

-----(1)

> > > >
> > > > The regulators are optional, we supply power to the camera sensor directly
> > > > through dedicated power rails and there is no strict enable sequence
> > > > required in this case.
> > >
> > > What exactly do you mean by "this case" ? Are you talking about one
> > > particular sensor ? One particular camera module ?
> >
> > Laurent, by “this case” I meant the common scenario where power to the
> > camera sensor is supplied by a PMIC regulator that is always-on. In such
> > setups, the regulator is fixed and cannot be enabled or disabled from the
> > driver, the sensor is always powered.
> >
> > This is what I’ve seen in most platforms, where the CSI input connector
> > provides fixed 3.3V/1.8V power rails directly to the camera module.
> >
> > Of course, if the camera supply comes from a dedicated regulator controlled
> > via a GPIO, then the driver would need to handle enable/disable sequencing
> > explicitly. But I’m specifically referring to the first case, where the power rails
> > are always-on.
> 
> How does the sensor driver know which of those two cases it is dealing
> with ?

The sensor driver typically determines this via the presence (or absence) 
of regulator supply entries in the Device Tree. If a supply is not defined,
it's assumed to be always-on (e.g., provided by the board via fixed rails).
When defined, the driver retrieves and manages the regulator. This approach
allows a single driver to support both cases, by treating supplies as optional
and only enabling them when explicitly defined.

At comment (1): you gave two reasons why we cannot use devm_regulator_bulk_get_enable.

What I’m trying to say is:

You mentioned "generally don't want to enable power everywhere unconditionally," 
but on almost every platform, the power rails are always-on.

And regarding the second point — "sensors very often need a guaranteed power-up sequence" 
I don’t understand why this would be an issue. Even if we use devm_regulator_bulk_get_enable,
the power-up sequence remains the same. So how is it not a good option in this case?

Best Regards,
Tarang

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart]

On Fri, Jul 25, 2025 at 07:00:40AM +0000, Tarang Raval wrote:
> > On Thu, Jul 24, 2025 at 02:20:10PM +0000, Tarang Raval wrote:
> > > > > > > 2. In the regulator code, you can reduce boilerplate by using
> > > > > > >    devm_regulator_bulk_get_enable().
> > > > > >
> > > > > > devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
> > > > > > generally don't want to enable power everywhere unconditionally, and
> > > > > > sensors very often need a guaranteed power up sequence.
> 
> -----(1)
> 
> > > > >
> > > > > The regulators are optional, we supply power to the camera sensor directly
> > > > > through dedicated power rails and there is no strict enable sequence
> > > > > required in this case.
> > > >
> > > > What exactly do you mean by "this case" ? Are you talking about one
> > > > particular sensor ? One particular camera module ?
> > >
> > > Laurent, by “this case” I meant the common scenario where power to the
> > > camera sensor is supplied by a PMIC regulator that is always-on. In such
> > > setups, the regulator is fixed and cannot be enabled or disabled from the
> > > driver, the sensor is always powered.
> > >
> > > This is what I’ve seen in most platforms, where the CSI input connector
> > > provides fixed 3.3V/1.8V power rails directly to the camera module.
> > >
> > > Of course, if the camera supply comes from a dedicated regulator controlled
> > > via a GPIO, then the driver would need to handle enable/disable sequencing
> > > explicitly. But I’m specifically referring to the first case, where the power rails
> > > are always-on.
> > 
> > How does the sensor driver know which of those two cases it is dealing
> > with ?
> 
> The sensor driver typically determines this via the presence (or absence) 
> of regulator supply entries in the Device Tree. If a supply is not defined,
> it's assumed to be always-on (e.g., provided by the board via fixed rails).

Do we have sensor drivers that check the presense of supply properties ?
Drivers generally shouldn't.

> When defined, the driver retrieves and manages the regulator. This approach
> allows a single driver to support both cases, by treating supplies as optional
> and only enabling them when explicitly defined.

I don't see what you're trying to do here. A sensor always needs
supplies, regardless of whether or not they're always on. Drivers should
get the supplies with regulator_get() (or possibly the bulk API), and
then implement the power enable/disable sequences that the sensor
requires. If all suplies are manually controllable, this will produce
the correct sequence. If the supplies are always on, it will be a no-op.
That's a single implementation in the driver, you don't need to care
about the nature of the supplies, or their presence in DT.

> At comment (1): you gave two reasons why we cannot use devm_regulator_bulk_get_enable.
> 
> What I’m trying to say is:
> 
> You mentioned "generally don't want to enable power everywhere unconditionally," 
> but on almost every platform, the power rails are always-on.

"almost every platform" doesn't sound right to me. It does happen though.

> And regarding the second point — "sensors very often need a guaranteed power-up sequence" 
> I don’t understand why this would be an issue. Even if we use devm_regulator_bulk_get_enable,
> the power-up sequence remains the same. So how is it not a good option in this case?

Because the bulk API enables all regulators in parallel, it doesn't
guarantee sequencing.

Don't use devm_regulator_bulk_get_enable() in sensor drivers, implement
power enable/disable functions that do the right thing. That's the code
pattern I want to see.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Tarang Raval]

> On Fri, Jul 25, 2025 at 07:00:40AM +0000, Tarang Raval wrote:
> > > On Thu, Jul 24, 2025 at 02:20:10PM +0000, Tarang Raval wrote:
> > > > > > > > 2. In the regulator code, you can reduce boilerplate by using
> > > > > > > >    devm_regulator_bulk_get_enable().
> > > > > > >
> > > > > > > devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
> > > > > > > generally don't want to enable power everywhere unconditionally, and
> > > > > > > sensors very often need a guaranteed power up sequence.
> >
> > -----(1)
> >
> > > > > >
> > > > > > The regulators are optional, we supply power to the camera sensor directly
> > > > > > through dedicated power rails and there is no strict enable sequence
> > > > > > required in this case.
> > > > >
> > > > > What exactly do you mean by "this case" ? Are you talking about one
> > > > > particular sensor ? One particular camera module ?
> > > >
> > > > Laurent, by “this case” I meant the common scenario where power to the
> > > > camera sensor is supplied by a PMIC regulator that is always-on. In such
> > > > setups, the regulator is fixed and cannot be enabled or disabled from the
> > > > driver, the sensor is always powered.
> > > >
> > > > This is what I’ve seen in most platforms, where the CSI input connector
> > > > provides fixed 3.3V/1.8V power rails directly to the camera module.
> > > >
> > > > Of course, if the camera supply comes from a dedicated regulator controlled
> > > > via a GPIO, then the driver would need to handle enable/disable sequencing
> > > > explicitly. But I’m specifically referring to the first case, where the power rails
> > > > are always-on.
> > >
> > > How does the sensor driver know which of those two cases it is dealing
> > > with ?
> >
> > The sensor driver typically determines this via the presence (or absence)
> > of regulator supply entries in the Device Tree. If a supply is not defined,
> > it's assumed to be always-on (e.g., provided by the board via fixed rails).
> 
> Do we have sensor drivers that check the presense of supply properties ?
> Drivers generally shouldn't.
> 
> > When defined, the driver retrieves and manages the regulator. This approach
> > allows a single driver to support both cases, by treating supplies as optional
> > and only enabling them when explicitly defined.
> 
> I don't see what you're trying to do here. A sensor always needs
> supplies, regardless of whether or not they're always on. Drivers should
> get the supplies with regulator_get() (or possibly the bulk API), and
> then implement the power enable/disable sequences that the sensor
> requires. If all suplies are manually controllable, this will produce
> the correct sequence. If the supplies are always on, it will be a no-op.
> That's a single implementation in the driver, you don't need to care
> about the nature of the supplies, or their presence in DT.
> 
> > At comment (1): you gave two reasons why we cannot use devm_regulator_bulk_get_enable.
> >
> > What I’m trying to say is:
> >
> > You mentioned "generally don't want to enable power everywhere unconditionally,"
> > but on almost every platform, the power rails are always-on.
> 
> "almost every platform" doesn't sound right to me. It does happen though.
> 
> > And regarding the second point — "sensors very often need a guaranteed power-up sequence"
> > I don’t understand why this would be an issue. Even if we use devm_regulator_bulk_get_enable,
> > the power-up sequence remains the same. So how is it not a good option in this case?
> 
> Because the bulk API enables all regulators in parallel, it doesn't
> guarantee sequencing.

Except for a few drivers, almost all camera drivers use the bulk API, which suggests
that a guaranteed power-up sequence may not be strictly required in most cases.
 
> Don't use devm_regulator_bulk_get_enable() in sensor drivers, implement
> power enable/disable functions that do the right thing. That's the code
> pattern I want to see.

Perhaps I wasnt clear in my explanation. If you look at the patch below, you'll 
see that we are not changing any sequencing behavior.

I am not suggesting we use this API everywhere, only where it's appropriate and 
doesn't compromise sequencing requirements. 

Best Regards,
Tarang

------

diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
index da618c8cbadc..4dbf7215cef4 100644
--- a/drivers/media/i2c/imx283.c
+++ b/drivers/media/i2c/imx283.c
@@ -1176,8 +1176,8 @@ static int imx283_power_on(struct device *dev)
        struct imx283 *imx283 = to_imx283(sd);
        int ret;
 
-       ret = regulator_bulk_enable(ARRAY_SIZE(imx283_supply_name),
-                                   imx283->supplies);
+       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(imx283_supply_name),
+                                   imx283_supply_name);
        if (ret) {
                dev_err(imx283->dev, "failed to enable regulators\n");
                return ret;
@@ -1186,7 +1186,7 @@ static int imx283_power_on(struct device *dev)
        ret = clk_prepare_enable(imx283->xclk);
        if (ret) {
                dev_err(imx283->dev, "failed to enable clock\n");
-               goto reg_off;
+               return ret;
        }
 
        gpiod_set_value_cansleep(imx283->reset_gpio, 0);
@@ -1195,10 +1195,6 @@ static int imx283_power_on(struct device *dev)
                     IMX283_XCLR_MIN_DELAY_US + IMX283_XCLR_DELAY_RANGE_US);
 
        return 0;
-
-reg_off:
-       regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies);
-       return ret;
 }
 
 static int imx283_power_off(struct device *dev)
@@ -1207,24 +1203,11 @@ static int imx283_power_off(struct device *dev)
        struct imx283 *imx283 = to_imx283(sd);
 
        gpiod_set_value_cansleep(imx283->reset_gpio, 1);
-       regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies);
        clk_disable_unprepare(imx283->xclk);
 
        return 0;
 }
 
-static int imx283_get_regulators(struct imx283 *imx283)
-{
-       unsigned int i;
-
-       for (i = 0; i < ARRAY_SIZE(imx283_supply_name); i++)
-               imx283->supplies[i].supply = imx283_supply_name[i];
-
-       return devm_regulator_bulk_get(imx283->dev,
-                                      ARRAY_SIZE(imx283_supply_name),
-                                      imx283->supplies);
-}
-
 /* Verify chip ID */
 static int imx283_identify_module(struct imx283 *imx283)
 {
@@ -1480,12 +1463,6 @@ static int imx283_probe(struct i2c_client *client)
                return -EINVAL;
        }
 
-       ret = imx283_get_regulators(imx283);
-       if (ret) {
-               return dev_err_probe(imx283->dev, ret,
-                               "failed to get regulators\n");
-       }
-
        ret = imx283_parse_endpoint(imx283);
        if (ret) {
                dev_err(imx283->dev, "failed to parse endpoint configuration\n");

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart]

On Fri, Jul 25, 2025 at 10:35:28AM +0000, Tarang Raval wrote:
> > On Fri, Jul 25, 2025 at 07:00:40AM +0000, Tarang Raval wrote:
> > > > On Thu, Jul 24, 2025 at 02:20:10PM +0000, Tarang Raval wrote:
> > > > > > > > > 2. In the regulator code, you can reduce boilerplate by using
> > > > > > > > >    devm_regulator_bulk_get_enable().
> > > > > > > >
> > > > > > > > devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
> > > > > > > > generally don't want to enable power everywhere unconditionally, and
> > > > > > > > sensors very often need a guaranteed power up sequence.
> > >
> > > -----(1)
> > >
> > > > > > >
> > > > > > > The regulators are optional, we supply power to the camera sensor directly
> > > > > > > through dedicated power rails and there is no strict enable sequence
> > > > > > > required in this case.
> > > > > >
> > > > > > What exactly do you mean by "this case" ? Are you talking about one
> > > > > > particular sensor ? One particular camera module ?
> > > > >
> > > > > Laurent, by “this case” I meant the common scenario where power to the
> > > > > camera sensor is supplied by a PMIC regulator that is always-on. In such
> > > > > setups, the regulator is fixed and cannot be enabled or disabled from the
> > > > > driver, the sensor is always powered.
> > > > >
> > > > > This is what I’ve seen in most platforms, where the CSI input connector
> > > > > provides fixed 3.3V/1.8V power rails directly to the camera module.
> > > > >
> > > > > Of course, if the camera supply comes from a dedicated regulator controlled
> > > > > via a GPIO, then the driver would need to handle enable/disable sequencing
> > > > > explicitly. But I’m specifically referring to the first case, where the power rails
> > > > > are always-on.
> > > >
> > > > How does the sensor driver know which of those two cases it is dealing
> > > > with ?
> > >
> > > The sensor driver typically determines this via the presence (or absence)
> > > of regulator supply entries in the Device Tree. If a supply is not defined,
> > > it's assumed to be always-on (e.g., provided by the board via fixed rails).
> > 
> > Do we have sensor drivers that check the presense of supply properties ?
> > Drivers generally shouldn't.
> > 
> > > When defined, the driver retrieves and manages the regulator. This approach
> > > allows a single driver to support both cases, by treating supplies as optional
> > > and only enabling them when explicitly defined.
> > 
> > I don't see what you're trying to do here. A sensor always needs
> > supplies, regardless of whether or not they're always on. Drivers should
> > get the supplies with regulator_get() (or possibly the bulk API), and
> > then implement the power enable/disable sequences that the sensor
> > requires. If all suplies are manually controllable, this will produce
> > the correct sequence. If the supplies are always on, it will be a no-op.
> > That's a single implementation in the driver, you don't need to care
> > about the nature of the supplies, or their presence in DT.
> > 
> > > At comment (1): you gave two reasons why we cannot use devm_regulator_bulk_get_enable.
> > >
> > > What I’m trying to say is:
> > >
> > > You mentioned "generally don't want to enable power everywhere unconditionally,"
> > > but on almost every platform, the power rails are always-on.
> > 
> > "almost every platform" doesn't sound right to me. It does happen though.
> > 
> > > And regarding the second point — "sensors very often need a guaranteed power-up sequence"
> > > I don’t understand why this would be an issue. Even if we use devm_regulator_bulk_get_enable,
> > > the power-up sequence remains the same. So how is it not a good option in this case?
> > 
> > Because the bulk API enables all regulators in parallel, it doesn't
> > guarantee sequencing.
> 
> Except for a few drivers, almost all camera drivers use the bulk API, which suggests
> that a guaranteed power-up sequence may not be strictly required in most cases.
>  
> > Don't use devm_regulator_bulk_get_enable() in sensor drivers, implement
> > power enable/disable functions that do the right thing. That's the code
> > pattern I want to see.
> 
> Perhaps I wasnt clear in my explanation. If you look at the patch below, you'll 
> see that we are not changing any sequencing behavior.

You end up getting regulators every time power is enabled, and you don't
turn the supplies off at power off time. How is that even supposed to
work ? It completely breaks power management.

> I am not suggesting we use this API everywhere, only where it's appropriate and 
> doesn't compromise sequencing requirements. 
> 
> Best Regards,
> Tarang
> 
> ------
> 
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> index da618c8cbadc..4dbf7215cef4 100644
> --- a/drivers/media/i2c/imx283.c
> +++ b/drivers/media/i2c/imx283.c
> @@ -1176,8 +1176,8 @@ static int imx283_power_on(struct device *dev)
>         struct imx283 *imx283 = to_imx283(sd);
>         int ret;
>  
> -       ret = regulator_bulk_enable(ARRAY_SIZE(imx283_supply_name),
> -                                   imx283->supplies);
> +       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(imx283_supply_name),
> +                                   imx283_supply_name);
>         if (ret) {
>                 dev_err(imx283->dev, "failed to enable regulators\n");
>                 return ret;
> @@ -1186,7 +1186,7 @@ static int imx283_power_on(struct device *dev)
>         ret = clk_prepare_enable(imx283->xclk);
>         if (ret) {
>                 dev_err(imx283->dev, "failed to enable clock\n");
> -               goto reg_off;
> +               return ret;
>         }
>  
>         gpiod_set_value_cansleep(imx283->reset_gpio, 0);
> @@ -1195,10 +1195,6 @@ static int imx283_power_on(struct device *dev)
>                      IMX283_XCLR_MIN_DELAY_US + IMX283_XCLR_DELAY_RANGE_US);
>  
>         return 0;
> -
> -reg_off:
> -       regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies);
> -       return ret;
>  }
>  
>  static int imx283_power_off(struct device *dev)
> @@ -1207,24 +1203,11 @@ static int imx283_power_off(struct device *dev)
>         struct imx283 *imx283 = to_imx283(sd);
>  
>         gpiod_set_value_cansleep(imx283->reset_gpio, 1);
> -       regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies);
>         clk_disable_unprepare(imx283->xclk);
>  
>         return 0;
>  }
>  
> -static int imx283_get_regulators(struct imx283 *imx283)
> -{
> -       unsigned int i;
> -
> -       for (i = 0; i < ARRAY_SIZE(imx283_supply_name); i++)
> -               imx283->supplies[i].supply = imx283_supply_name[i];
> -
> -       return devm_regulator_bulk_get(imx283->dev,
> -                                      ARRAY_SIZE(imx283_supply_name),
> -                                      imx283->supplies);
> -}
> -
>  /* Verify chip ID */
>  static int imx283_identify_module(struct imx283 *imx283)
>  {
> @@ -1480,12 +1463,6 @@ static int imx283_probe(struct i2c_client *client)
>                 return -EINVAL;
>         }
>  
> -       ret = imx283_get_regulators(imx283);
> -       if (ret) {
> -               return dev_err_probe(imx283->dev, ret,
> -                               "failed to get regulators\n");
> -       }
> -
>         ret = imx283_parse_endpoint(imx283);
>         if (ret) {
>                 dev_err(imx283->dev, "failed to parse endpoint configuration\n");

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Tarang Raval]

> On Fri, Jul 25, 2025 at 10:35:28AM +0000, Tarang Raval wrote:
> > > On Fri, Jul 25, 2025 at 07:00:40AM +0000, Tarang Raval wrote:
> > > > > On Thu, Jul 24, 2025 at 02:20:10PM +0000, Tarang Raval wrote:
> > > > > > > > > > 2. In the regulator code, you can reduce boilerplate by using
> > > > > > > > > >    devm_regulator_bulk_get_enable().
> > > > > > > > >
> > > > > > > > > devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
> > > > > > > > > generally don't want to enable power everywhere unconditionally, and
> > > > > > > > > sensors very often need a guaranteed power up sequence.
> > > >
> > > > -----(1)
> > > >
> > > > > > > >
> > > > > > > > The regulators are optional, we supply power to the camera sensor directly
> > > > > > > > through dedicated power rails and there is no strict enable sequence
> > > > > > > > required in this case.
> > > > > > >
> > > > > > > What exactly do you mean by "this case" ? Are you talking about one
> > > > > > > particular sensor ? One particular camera module ?
> > > > > >
> > > > > > Laurent, by “this case” I meant the common scenario where power to the
> > > > > > camera sensor is supplied by a PMIC regulator that is always-on. In such
> > > > > > setups, the regulator is fixed and cannot be enabled or disabled from the
> > > > > > driver, the sensor is always powered.
> > > > > >
> > > > > > This is what I’ve seen in most platforms, where the CSI input connector
> > > > > > provides fixed 3.3V/1.8V power rails directly to the camera module.
> > > > > >
> > > > > > Of course, if the camera supply comes from a dedicated regulator controlled
> > > > > > via a GPIO, then the driver would need to handle enable/disable sequencing
> > > > > > explicitly. But I’m specifically referring to the first case, where the power rails
> > > > > > are always-on.
> > > > >
> > > > > How does the sensor driver know which of those two cases it is dealing
> > > > > with ?
> > > >
> > > > The sensor driver typically determines this via the presence (or absence)
> > > > of regulator supply entries in the Device Tree. If a supply is not defined,
> > > > it's assumed to be always-on (e.g., provided by the board via fixed rails).
> > >
> > > Do we have sensor drivers that check the presense of supply properties ?
> > > Drivers generally shouldn't.
> > >
> > > > When defined, the driver retrieves and manages the regulator. This approach
> > > > allows a single driver to support both cases, by treating supplies as optional
> > > > and only enabling them when explicitly defined.
> > >
> > > I don't see what you're trying to do here. A sensor always needs
> > > supplies, regardless of whether or not they're always on. Drivers should
> > > get the supplies with regulator_get() (or possibly the bulk API), and
> > > then implement the power enable/disable sequences that the sensor
> > > requires. If all suplies are manually controllable, this will produce
> > > the correct sequence. If the supplies are always on, it will be a no-op.
> > > That's a single implementation in the driver, you don't need to care
> > > about the nature of the supplies, or their presence in DT.
> > >
> > > > At comment (1): you gave two reasons why we cannot use devm_regulator_bulk_get_enable.
> > > >
> > > > What I’m trying to say is:
> > > >
> > > > You mentioned "generally don't want to enable power everywhere unconditionally,"
> > > > but on almost every platform, the power rails are always-on.
> > >
> > > "almost every platform" doesn't sound right to me. It does happen though.
> > >
> > > > And regarding the second point — "sensors very often need a guaranteed power-up sequence"
> > > > I don’t understand why this would be an issue. Even if we use devm_regulator_bulk_get_enable,
> > > > the power-up sequence remains the same. So how is it not a good option in this case?
> > >
> > > Because the bulk API enables all regulators in parallel, it doesn't
> > > guarantee sequencing.
> >
> > Except for a few drivers, almost all camera drivers use the bulk API, which suggests
> > that a guaranteed power-up sequence may not be strictly required in most cases.
> >
> > > Don't use devm_regulator_bulk_get_enable() in sensor drivers, implement
> > > power enable/disable functions that do the right thing. That's the code
> > > pattern I want to see.
> >
> > Perhaps I wasnt clear in my explanation. If you look at the patch below, you'll
> > see that we are not changing any sequencing behavior.
> 
> You end up getting regulators every time power is enabled, and you don't
> turn the supplies off at power off time. How is that even supposed to
> work ? It completely breaks power management.

Got it. The regulators stay enabled until the device is unbound.

If the power rails are always on, there’s effectively nothing to disable.
Why would that break power management?

This is a little bit confusing.

However, I now agree that we should not use it.

Best Regards,
Tarang

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 404 bytes --]

On Fri, Jul 25, 2025 at 07:00:40AM +0000, Tarang Raval wrote:

> The sensor driver typically determines this via the presence (or absence) 
> of regulator supply entries in the Device Tree. If a supply is not defined,
> it's assumed to be always-on (e.g., provided by the board via fixed rails).

No, this is broken.  The driver should unconditionally request whatever
supplies the device needs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Tarang Raval]

> > The sensor driver typically determines this via the presence (or absence)
> > of regulator supply entries in the Device Tree. If a supply is not defined,
> > it's assumed to be always-on (e.g., provided by the board via fixed rails).
>
> No, this is broken.  The driver should unconditionally request whatever
> supplies the device needs.

Yes, that’s clear from Laurent’s explanation:

"A sensor always needs
 supplies, regardless of whether or not they're always on. Drivers should
 get the supplies with regulator_get() (or possibly the bulk API)"

Thanks, Mark, for the clarification.

Best Regards,
Tarang

Re: (subset) [PATCH 00/72] media: i2c: Reduce cargo-cult

[Bjorn Andersson]

On Thu, 10 Jul 2025 20:46:56 +0300, Laurent Pinchart wrote:
> This patch series build on top of Mehdi's introduction of the
> devm_v4l2_sensor_clk_get() helper (see [1]) to drastically reduce
> cargo-cult in camera sensor drivers.
> 
> A large number of camera sensor drivers directly use the
> "clock-frequency" property to retrieve the effective or desired external
> clock rate. This is standard behaviour on ACPI platforms that don't
> implement MIPI DisCo for Imaging, but usage of the property has leaked
> to OF-based platforms, due to a combination of historical reasons (using
> "clock-frequency" was initially considered right until before the
> introduction of "assigned-clock-rates") and plain cargo-cult.
> 
> [...]

Applied, thanks!

[12/72] arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace clock-frequency in camera sensor node
        commit: 5433560caa5e7e677a8d4310bbec08312be765b4

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: (subset) [PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart]

Hi Bjorn,

On Mon, Aug 11, 2025 at 06:27:01PM -0500, Bjorn Andersson wrote:
> On Thu, 10 Jul 2025 20:46:56 +0300, Laurent Pinchart wrote:
> > This patch series build on top of Mehdi's introduction of the
> > devm_v4l2_sensor_clk_get() helper (see [1]) to drastically reduce
> > cargo-cult in camera sensor drivers.
> > 
> > A large number of camera sensor drivers directly use the
> > "clock-frequency" property to retrieve the effective or desired external
> > clock rate. This is standard behaviour on ACPI platforms that don't
> > implement MIPI DisCo for Imaging, but usage of the property has leaked
> > to OF-based platforms, due to a combination of historical reasons (using
> > "clock-frequency" was initially considered right until before the
> > introduction of "assigned-clock-rates") and plain cargo-cult.
> > 
> > [...]
> 
> Applied, thanks!
> 
> [12/72] arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace clock-frequency in camera sensor node
>         commit: 5433560caa5e7e677a8d4310bbec08312be765b4

I'm afraid that's too soon. This will introduce a breakage without a
corresponding change to the camera sensor driver.

I will post a v2 with the patches reordered. We could merge the V4L2
side in a rc1-based stable branch and merge than in the arm-soc tree as
well, but I think we can also delay the .dts changes to the next kernel
version. Do you have a preference ?

-- 
Regards,

Laurent Pinchart

Re: (subset) [PATCH 00/72] media: i2c: Reduce cargo-cult

[Krzysztof Kozlowski]

On 12/08/2025 10:51, Laurent Pinchart wrote:
> Hi Bjorn,
> 
> On Mon, Aug 11, 2025 at 06:27:01PM -0500, Bjorn Andersson wrote:
>> On Thu, 10 Jul 2025 20:46:56 +0300, Laurent Pinchart wrote:
>>> This patch series build on top of Mehdi's introduction of the
>>> devm_v4l2_sensor_clk_get() helper (see [1]) to drastically reduce
>>> cargo-cult in camera sensor drivers.
>>>
>>> A large number of camera sensor drivers directly use the
>>> "clock-frequency" property to retrieve the effective or desired external
>>> clock rate. This is standard behaviour on ACPI platforms that don't
>>> implement MIPI DisCo for Imaging, but usage of the property has leaked
>>> to OF-based platforms, due to a combination of historical reasons (using
>>> "clock-frequency" was initially considered right until before the
>>> introduction of "assigned-clock-rates") and plain cargo-cult.
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [12/72] arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace clock-frequency in camera sensor node
>>         commit: 5433560caa5e7e677a8d4310bbec08312be765b4
> 
> I'm afraid that's too soon. This will introduce a breakage without a
> corresponding change to the camera sensor driver.
> 
> I will post a v2 with the patches reordered. We could merge the V4L2
> side in a rc1-based stable branch and merge than in the arm-soc tree as


You cannot ("cannot" as not following the process) merge drivers into
DTS branch.


> well, but I think we can also delay the .dts changes to the next kernel


All users of DTS will be anyway affected and commit msg should address that.

> version. Do you have a preference ?



Best regards,
Krzysztof

Re: (subset) [PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart]

On Tue, Aug 12, 2025 at 10:58:30AM +0200, Krzysztof Kozlowski wrote:
> On 12/08/2025 10:51, Laurent Pinchart wrote:
> > On Mon, Aug 11, 2025 at 06:27:01PM -0500, Bjorn Andersson wrote:
> >> On Thu, 10 Jul 2025 20:46:56 +0300, Laurent Pinchart wrote:
> >>> This patch series build on top of Mehdi's introduction of the
> >>> devm_v4l2_sensor_clk_get() helper (see [1]) to drastically reduce
> >>> cargo-cult in camera sensor drivers.
> >>>
> >>> A large number of camera sensor drivers directly use the
> >>> "clock-frequency" property to retrieve the effective or desired external
> >>> clock rate. This is standard behaviour on ACPI platforms that don't
> >>> implement MIPI DisCo for Imaging, but usage of the property has leaked
> >>> to OF-based platforms, due to a combination of historical reasons (using
> >>> "clock-frequency" was initially considered right until before the
> >>> introduction of "assigned-clock-rates") and plain cargo-cult.
> >>>
> >>> [...]
> >>
> >> Applied, thanks!
> >>
> >> [12/72] arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace clock-frequency in camera sensor node
> >>         commit: 5433560caa5e7e677a8d4310bbec08312be765b4
> > 
> > I'm afraid that's too soon. This will introduce a breakage without a
> > corresponding change to the camera sensor driver.
> > 
> > I will post a v2 with the patches reordered. We could merge the V4L2
> > side in a rc1-based stable branch and merge than in the arm-soc tree as
> 
> You cannot ("cannot" as not following the process) merge drivers into
> DTS branch.

Ah, I wasn't aware of that. DTS trees don't allow merging stable
branches shared with other subsystems ? Does it mean that a DTS change
that depends on a driver change always need to be delayed by one kernel
version ?

> > well, but I think we can also delay the .dts changes to the next kernel
> 
> All users of DTS will be anyway affected and commit msg should address that.

Which commit message, the one for the driver changes or the one for the
DTS changes ? I plan in the next version to indicate that the DT changes
depend on the driver changes.

> > version. Do you have a preference ?

-- 
Regards,

Laurent Pinchart

Re: (subset) [PATCH 00/72] media: i2c: Reduce cargo-cult

[Krzysztof Kozlowski]

On 12/08/2025 11:39, Laurent Pinchart wrote:
>>>>
>>>> [12/72] arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace clock-frequency in camera sensor node
>>>>         commit: 5433560caa5e7e677a8d4310bbec08312be765b4
>>>
>>> I'm afraid that's too soon. This will introduce a breakage without a
>>> corresponding change to the camera sensor driver.
>>>
>>> I will post a v2 with the patches reordered. We could merge the V4L2
>>> side in a rc1-based stable branch and merge than in the arm-soc tree as
>>
>> You cannot ("cannot" as not following the process) merge drivers into
>> DTS branch.
> 
> Ah, I wasn't aware of that. DTS trees don't allow merging stable
> branches shared with other subsystems ? Does it mean that a DTS change

Not with driver subsystems. Why? Because it breaks encapsulation of
hardware description being entirely independent of given Linux driver
implementation.

BTW, it is already documented in maintainer-soc in ABI stability (I will
fix "devicetree" ambiguity to DTS) and driver branch dependencies.

> that depends on a driver change always need to be delayed by one kernel
> version ?

This is one solution, although as I mentioned later it still affects all
other users of DTS, so it has its own drawbacks.

Other solution is to keep both properties for more than one cycle.


> 
>>> well, but I think we can also delay the .dts changes to the next kernel
>>
>> All users of DTS will be anyway affected and commit msg should address that.
> 
> Which commit message, the one for the driver changes or the one for the
> DTS changes ? I plan in the next version to indicate that the DT changes
> depend on the driver changes.


DTS changes, so the soc maintainers can judge whether they care about
other DTS users or they do not.


Best regards,
Krzysztof

Re: (subset) [PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart]

On Tue, Aug 12, 2025 at 12:28:28PM +0200, Krzysztof Kozlowski wrote:
> On 12/08/2025 11:39, Laurent Pinchart wrote:
> >>>>
> >>>> [12/72] arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace clock-frequency in camera sensor node
> >>>>         commit: 5433560caa5e7e677a8d4310bbec08312be765b4
> >>>
> >>> I'm afraid that's too soon. This will introduce a breakage without a
> >>> corresponding change to the camera sensor driver.
> >>>
> >>> I will post a v2 with the patches reordered. We could merge the V4L2
> >>> side in a rc1-based stable branch and merge than in the arm-soc tree as
> >>
> >> You cannot ("cannot" as not following the process) merge drivers into
> >> DTS branch.
> > 
> > Ah, I wasn't aware of that. DTS trees don't allow merging stable
> > branches shared with other subsystems ? Does it mean that a DTS change
> 
> Not with driver subsystems. Why? Because it breaks encapsulation of
> hardware description being entirely independent of given Linux driver
> implementation.
> 
> BTW, it is already documented in maintainer-soc in ABI stability (I will
> fix "devicetree" ambiguity to DTS) and driver branch dependencies.
> 
> > that depends on a driver change always need to be delayed by one kernel
> > version ?
> 
> This is one solution, although as I mentioned later it still affects all
> other users of DTS, so it has its own drawbacks.
> 
> Other solution is to keep both properties for more than one cycle.

OK.

There's no urgency to merge the .dts changes, so I think delaying them
by one kernel release is the simplest option.

> >>> well, but I think we can also delay the .dts changes to the next kernel
> >>
> >> All users of DTS will be anyway affected and commit msg should address that.
> > 
> > Which commit message, the one for the driver changes or the one for the
> > DTS changes ? I plan in the next version to indicate that the DT changes
> > depend on the driver changes.
> 
> DTS changes, so the soc maintainers can judge whether they care about
> other DTS users or they do not.

Thank you for the clarification. I will do that.

-- 
Regards,

Laurent Pinchart

Re: (subset) [PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart]

On Tue, Aug 12, 2025 at 12:28:28PM +0200, Krzysztof Kozlowski wrote:
> On 12/08/2025 11:39, Laurent Pinchart wrote:
> >>>>
> >>>> [12/72] arm64: dts: qcom: sdm845-db845c-navigation-mezzanine: Replace clock-frequency in camera sensor node
> >>>>         commit: 5433560caa5e7e677a8d4310bbec08312be765b4
> >>>
> >>> I'm afraid that's too soon. This will introduce a breakage without a
> >>> corresponding change to the camera sensor driver.
> >>>
> >>> I will post a v2 with the patches reordered. We could merge the V4L2
> >>> side in a rc1-based stable branch and merge than in the arm-soc tree as
> >>
> >> You cannot ("cannot" as not following the process) merge drivers into
> >> DTS branch.
> > 
> > Ah, I wasn't aware of that. DTS trees don't allow merging stable
> > branches shared with other subsystems ? Does it mean that a DTS change
> 
> Not with driver subsystems. Why? Because it breaks encapsulation of
> hardware description being entirely independent of given Linux driver
> implementation.
> 
> BTW, it is already documented in maintainer-soc in ABI stability (I will
> fix "devicetree" ambiguity to DTS) and driver branch dependencies.

I've just read that document, and didn't interpret it as stricly
forbidding merging a driver branch in the arm-soc tree. The rule makes
sense though, as it makes it easier to ensure that backward
compatibility isn't broken by accident. The downside is that it can slow
down merging patches in some cases.

> > that depends on a driver change always need to be delayed by one kernel
> > version ?
> 
> This is one solution, although as I mentioned later it still affects all
> other users of DTS, so it has its own drawbacks.
> 
> Other solution is to keep both properties for more than one cycle.
> 
> >>> well, but I think we can also delay the .dts changes to the next kernel
> >>
> >> All users of DTS will be anyway affected and commit msg should address that.
> > 
> > Which commit message, the one for the driver changes or the one for the
> > DTS changes ? I plan in the next version to indicate that the DT changes
> > depend on the driver changes.
> 
> DTS changes, so the soc maintainers can judge whether they care about
> other DTS users or they do not.

-- 
Regards,

Laurent Pinchart