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

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

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