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

Quoting Sakari Ailus (2026-08-06 10:58:17)
> 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.

Ack.


Reviewed-by: Kieran Bingham <[email protected]>

> 
> -- 
> Regards,
> 
> Sakari Ailus