Re: [PATCH v4 1/2] media: i2c: ov8856 : remove ACPI node bypass mechanism

Hi Kieran,

On Thu, Aug 06, 2026 at 09:46:10AM +0100, Kieran Bingham wrote:
> Quoting Serin Yeh (2026-08-06 07:18:04)
> > On Intel platforms the sensor is enumerated via ACPI, and an INT3472
> > "discrete" companion device (the power-logic-control driver under
> > drivers/platform/x86/intel/int3472) registers the sensor's
> > reset/powerdown GPIOs and the regulator supply. These resources are
> > therefore discoverable and usable through the standard gpiod/regulator
> > lookups at probe time.
> > 
> > However, the driver guarded both the GPIO/regulator acquisition in
> > ov8856_get_hwcfg() and the whole power-on/off sequence with
> > is_acpi_node() and skipped them on ACPI. As a result the sensor was
> > never powered or reset by the driver, and probe failed when reading the
> > chip ID in ov8856_identify_module().
> > 
> > Remove the is_acpi_node() checks so the GPIO/regulator resources are
> > acquired and the full power sequence runs regardless of the firmware
> > interface. The driver now behaves consistently across DT and ACPI
> > platforms and initialises the hardware correctly.
> 
> The calls to get GPIO resources is 'optional' but regulators aren't I
> don't think. Is this still ok ?
> 
> I think it is - and we can't run a device without the regulators - but I
> don't know how it works in ACPI land.
> 
> 
> Sakari, could this cause any regression ? I think it's ok - I just have
> a niggle of worry here in case there's something I'm not seeing.

There should be no need for explicit regulator control on ACPI systems,
however the reality isn't always aligned with how things should be. So on
some systems you do have to control regulators, even on ACPI.

> 
> 
> > Tested on Intel platforms with kernel 6.17 and 7.0.
> > 
> > Fixes: d2fa1134a48b ("media: i2c: ov8856: Use V4L2 legacy sensor clock helper")
> 
> Is this accurate though? I don't think that commit caused or added the
> ACPI checks. Was this just the first line that matched a git blame ?
> 
> I suspect the blame might lie further back.
> 
> devm_gpiod_get_optional was still guarded by
>  "if (!is_acpi_node(fwnode))"
> 
> before d2fa1134a48b, it was just a bit higher before
> devm_v4l2_sensor_clk_get().

I think Fixes: could be dropped altogether: the patch is simply adding
support for systems where you do need to control regulators on ACPI.

-- 
Regards,

Sakari Ailus