Re: [PATCH v3 2/3] media: i2c: Add driver for OmniVision OV32C4

[Robert Bozik <robertbozik@gmail.com>]

On Sat, Aug 29, 2026, sashiko-bot@kernel.org wrote:

Hi,

Thank you for the review. All three findings are real. The first one
needs a different fix from the one the description implies, and I would
rather say why than fix it quietly.

> [High] Incorrect error handling of pm_runtime_get_if_active() leads to
> PM usage counter underflow.

Confirmed, but only for one of the two cases the negative return covers.

pm_runtime_get_if_active() returns -EINVAL when runtime PM is disabled
for the device and does not take a reference in that case, so the
unconditional pm_runtime_put() at the end of the function does underflow
the counter. That part is right.

Returning the error early would break the other case, though. With
CONFIG_PM=n the inline version in include/linux/pm_runtime.h returns
-EINVAL unconditionally, so an early return would make every control
write fail on a kernel without runtime PM - where the sensor is in fact
permanently powered and the writes are exactly what should happen. The
drivers in the tree that use this idiom (gc05a2, gc08a3, imx283 and
lm3560, twice) all let the negative value fall through for that reason.

v4 therefore keeps the fall-through and only drops a reference that was
actually taken:

	/*
	 * A negative return means runtime PM is disabled for the device,
	 * in which case it is powered and the registers can be written,
	 * but no reference was taken and none may be dropped below.
	 */
	pm = pm_runtime_get_if_active(ov32c4->dev);
	if (!pm)
		return 0;
	...
	if (pm > 0)
		pm_runtime_put(ov32c4->dev);

> [Medium] Missing colorspace and format field initializations in
> ov32c4_update_pad_format().

Right, thank you. The fields are left as userspace passed them and then
stored in the subdev state. gc05a2 and gc08a3 set all four, and v4 does
the same:

	fmt->colorspace = V4L2_COLORSPACE_RAW;
	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
	fmt->xfer_func = V4L2_XFER_FUNC_NONE;

> [Low] Missing stubs for ACPI resource functions when CONFIG_ACPI is
> disabled causes build errors.

Right, and it is a consequence of a change made in v3.
acpi_dev_get_resources() and acpi_dev_free_resource_list() are declared
inside the CONFIG_ACPI section of include/linux/acpi.h and have no stubs
in the #else branch, so with "depends on ACPI || COMPILE_TEST" dropped
from Kconfig in v3 the driver no longer builds without ACPI. v4 puts the
_CRS walk behind #ifdef CONFIG_ACPI with a stub returning 0, which is
also more honest: that lookup only means anything on an ACPI platform.

All three are fixed in my tree and will be in v4, which is waiting for
the rest of the review of v3. Rebuilt and re-tested on the machine after
the changes: the sensor probes, streams and the controls still work.

Thanks,
Robert