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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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