Re: [BUG] OV02C10 on Dell 16 Premium DA16250 (ARL): INT3472 handshake-derived "dvdd" regulator registered but never linked to sensor, sensor probe fails with -EREMOTEIO

[Marco Nenciarini <mnencia@kcore.it>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hans, Sakari,

Samuele's data is in and it confirms both halves of the wrong-instance
keying.

DSC0 = INT3472:0c, DSC1 = INT3472:01. The decisive line from DSC0's
probe, with int3472 dyndbg on:

  int3472-discrete INT3472:0c: Sensor name HIMX1092:00

So DSC0's reverse-_DEP walk resolves to the first consumer that _DEPs
it, the Himax IR camera (HIMX1092:00, Windows Hello), and that is the
dev_name DSC0's dvdd supply_map is keyed on. The OV02C10 (OVTI02C1:00)
never appears in it. regulator_summary corroborates from the other
side: the only dvdd on
the whole platform is INT3472:0c-dvdd (DSC0), orphaned at use=0, and
DSC1 (which the RGB sensor actually _DEPs) exposes no dvdd at all, only
avdd/dovdd/reset.

So the topology is settled: there is exactly one dvdd handshake on the
platform, gated by DSC0, keyed to the IR sensor DSC0 serves, while the
RGB sensor that appears to want it _DEPs DSC1 instead, and nothing
connects the two. The remaining inference is the rail-to-failure link
itself: dvdd falls to a dummy under full constraints, and the chip-ID
read at 0x300a returns -EREMOTEIO with no retry. That chain is
consistent with the data but I have not proven dvdd is the cause as
opposed to a coincident orphan (see the test ask to Samuele below),
so I would not call the failure mechanism closed yet, only the keying.

On whose defect this is: either the firmware under-specifies the
dependency (the OV02C10's dvdd is physically gated by DSC0 but its _DEP
points only at DSC1), or the kernel's reverse-_DEP consumer model
cannot express a rail that lives on a sibling INT3472 instance. Either
way this is shipping DA16250 firmware that will not change, so the
camera needs an in-tree path regardless of where we assign blame.

That makes this the same class of problem int3472 already handles with
the second_sensor quirk. avdd_second_sensor (the Lenovo Miix 510 entry
in discrete_quirks.c) already plants a second supply_map entry, keyed
to a hardcoded device name in addition to the reverse-_DEP sensor_name,
and skl_int3472_register_regulator() takes second_sensor for exactly
that. The DA16250 is the same shape, just on dvdd/HANDSHAKE rather than
avdd/POWER_ENABLE, and the HANDSHAKE branch currently always passes
second_sensor = NULL. So the contained fix is: extend second_sensor to
cover the dvdd/HANDSHAKE arm too. The struct has a single second_sensor
field today (avdd_second_sensor), so generalising that one field reads
cleaner to me than adding a per-con_id dvdd_second_sensor, but either
works; then add a DA16250 DMI entry pointing dvdd's second consumer at
i2c-OVTI02C1:00. This adds OVTI02C1 as a second consumer of DSC0's
dvdd, it does not move the rail off HIMX1092:00, so the IR camera's own
supply is untouched.

The alternative is to make that second-consumer resolution automatic
rather than DMI-gated, i.e. teach int3472 to discover that a sensor may
draw a rail from an instance it does not _DEP on. More correct in
principle, but ACPI gives no signal to key it on here (that is the
firmware gap), so it would need a heuristic and I would not want it
silently re-homing rails on boards where the current keying is right.

My instinct is the DMI quirk, extending the mechanism you already use
for avdd. DMI is the right key rather than the int3472_gpio_map[] HID
table, because the OV02C10 part is not the problem, the DA16250 _DEP
topology is. I would leave the fully-automatic resolution open in case
more ARL boards turn up the same split. Tell me which way you want it
and I will prototype the quirk against the DA16250.

Samuele, here is a concrete test that closes the last gap. It is the
proposed fix in miniature, so a positive result validates both at once.
Two ways to run it, pick whichever suits your setup. In both, the
signal is the same: does the 0x300a chip-ID read in dmesg succeed, or
still return -EREMOTEIO?

Option A, the patch (preferred, it exercises the real consumer path).
In skl_int3472_handle_gpio_resources(), in
drivers/platform/x86/intel/int3472/discrete.c, add the two marked lines
to the regulator arm of the switch:

	case INT3472_GPIO_TYPE_POWER_ENABLE:
		second_sensor = int3472->quirks.avdd_second_sensor;
		fallthrough;
	case INT3472_GPIO_TYPE_DOVDD:
	case INT3472_GPIO_TYPE_HANDSHAKE:
+		if (type == INT3472_GPIO_TYPE_HANDSHAKE)
+			second_sensor = "i2c-OVTI02C1:00";	/* test */
		ret = skl_int3472_register_regulator(int3472, gpio, enable_time_us,
						     con_id, second_sensor);

This adds OVTI02C1:00 as a second consumer of DSC0's dvdd alongside the
existing HIMX1092:00 mapping; it does not move the rail off the IR
camera. The hardcoded string makes this test-only (it would mis-key on
any other handshake board); the shipped form is the DMI-gated quirk
above. The hunk applies to a recent mainline discrete.c, where the
HANDSHAKE case shares the register_regulator call via the POWER_ENABLE
fallthrough, so build from a source tree matching your running kernel.
First confirm int3472 is a module, not built in:

	modinfo intel_skl_int3472_discrete
	# or check CONFIG_INTEL_SKL_INT3472 in your kernel config

If it is =y you need a full kernel build instead. If =m, build and
install just this module, then reboot for a clean re-probe:

	make -C /lib/modules/$(uname -r)/build \
	     M=$PWD/drivers/platform/x86/intel/int3472 modules
	sudo make -C /lib/modules/$(uname -r)/build \
	     M=$PWD/drivers/platform/x86/intel/int3472 modules_install
	sudo depmod -a
	sudo reboot

(Build against the configured source for your running kernel, or the
new .ko may refuse to load on a modversions/CRC mismatch.) After
reboot, in /sys/kernel/debug/regulator/regulator_summary the
INT3472:0c-dvdd line should now list the i2c-OVTI02C1:00 device as a
consumer (use count > 0), and dmesg shows whether 0x300a now succeeds.

Option B, no rebuild, force the rail on by hand. On this board DSC0's
_DSM exposes only func 2 (the dvdd handshake) and func 3 (the IR-flood
strobe), so unbinding it drops exactly those two and nothing the
OV02C10 needs. dvdd is a GPIO-gated regulator, so as root:

	# 1. BEFORE unbinding, capture the dvdd enable line: int3472
	#    requests it with consumer label "dvdd", so in gpioinfo find
	#    the line whose consumer is "dvdd" and note its gpiochip (the
	#    block header) and offset. The label disappears once you
	#    unbind, so record chip+offset now. Your int3472 dyndbg log
	#    cross-checks it:
	#      "INT3472:0c: dvdd <acpi-gpio-path> pin <N> active-<high|low>"
	#    (that line gives the ACPI controller path and pin, not the
	#    gpiochip number, so map it through gpioinfo). Note the
	#    active- sense too, you need it in step 3:
	gpioinfo

	# 2. release the line by unbinding the PMIC (drops only DSC0's
	#    dvdd regulator and IR strobe, not the sensor, which _DEPs
	#    DSC1):
	echo INT3472:0c > /sys/bus/platform/drivers/int3472-discrete/unbind

	# 3. drive the line to its ON level and HOLD it (own shell, leave
	#    running). ON is =1 if step 1 showed active-high, =0 if
	#    active-low. Using the gpiochip and offset from step 1:
	#    libgpiod v1:  gpioset --mode=signal gpiochipN <offset>=<on>
	#    libgpiod v2:  gpioset -c gpiochipN <offset>=<on>  (holds until Ctrl-C)
	gpioset --mode=signal gpiochipN <offset>=<on>

	# 4. in another shell, re-probe the sensor and read the log:
	echo i2c-OVTI02C1:00 > /sys/bus/i2c/drivers/ov02c10/bind
	dmesg | tail

	# 5. when done, Ctrl-C the gpioset and reboot to restore normal
	#    driver state.

(If step 4 says the device is already bound, unbind it first via the
same path, then bind.) regulator_get("dvdd") lands on a dummy here,
since there is no provider once DSC0 is unbound, but that is a no-op
enable and the rail is physically on because you are holding the GPIO
at its ON level, so the chip-ID read is still the signal.

Either way: if 0x300a succeeds, that pins powering dvdd as what
unblocks the chip-ID read, and the fix is exactly Option A folded into
a DA16250 DMI quirk. If it still fails, dvdd is not the (only) problem
and the -EREMOTEIO is coming from reset, clock, or the NX33 bridge, and
we look there instead. Option A is the better single test, since it
keeps DSC0 bound and exercises the real enable path with the right
polarity and timing; Option B trades that for no rebuild, so if the two
disagree, trust A.

Thanks,
Marco
-----BEGIN PGP SIGNATURE-----

iQIyBAEBCAAdFiEEfCO4BD5l0pgKIbbiWJ8D8BulUDgFAmovDcIACgkQWJ8D8Bul
UDjAng/48+VJljU31kWvg6tTrofMNHBwjXYlCAepNnQqVr8FyKxeM+fgzucIjqU/
dq2OJP2TgDej13gGFLSrsduel4FNrcTqc3KYM+uYSnRCGX4Pqi7r4uT5M4tvY2Pt
mrDGBvi4ZK6RgYfrCMQus1mNtVaJcBw/mKPdxdQK84KmI6ohKh9yFsDgizVF2WHP
LFvJ2YQWEHf1jeisa5h+y1c4+RRPUIzmz8qW12jFLr7Dba7U+pNP+OaZZwZin6Dh
uPNa8rrkv+WFWKR7kJfK6woaJpR0VVHWIg0s/4AOID/3I5LIBBd1ruTZwJNnBMkz
TApKmqU9hXANXxZ+oGLfCW7OoqLvNfcuo0kE35dqzKLxrpkGYHgFnX7N7mD4+Rgp
B57rUUSIBrOs1dn2qgSussZoEr0dNp24r6K+YH1r/thMw4kdQxgovD4se8momCij
tGPP0hi/8hB2uXBSRJSH/wI03qrU/JVOqVgmc8W19tPemZdQuNxvSrwnEgUaAFx/
9uZZJoMBX3HM373eVd1/M4Bv3j4fGesjKI8JeYkGkzQioZOATV0PIfTv+K04Ytyb
QGCwanp3I0FFUVBqMpiwuRIm/xOxl1NEtrpLU/AtbfBF6D17RobMidTvPy3URFRg
fkSASULsYSvlKZLZpZoNquUZmcoqBwcLawewWza8T9DeJMlkfw==
=8fhX
-----END PGP SIGNATURE-----