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

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.


> 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().


--
Kieran


> Cc: [email protected]
> Signed-off-by: Serin Yeh <[email protected]>
> Reviewed-by: Bryan O'Donoghue <[email protected]>
> ---
>  drivers/media/i2c/ov8856.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 8bedb47cd7cf..9d2b0469a576 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -2082,9 +2082,6 @@ static int ov8856_power_on(struct device *dev)
>         struct ov8856 *ov8856 = to_ov8856(sd);
>         int ret;
>  
> -       if (is_acpi_node(dev_fwnode(dev)))
> -               return 0;
> -
>         ret = clk_prepare_enable(ov8856->xvclk);
>         if (ret < 0) {
>                 dev_err(dev, "failed to enable xvclk\n");
> @@ -2120,9 +2117,6 @@ static int ov8856_power_off(struct device *dev)
>         struct v4l2_subdev *sd = dev_get_drvdata(dev);
>         struct ov8856 *ov8856 = to_ov8856(sd);
>  
> -       if (is_acpi_node(dev_fwnode(dev)))
> -               return 0;
> -
>         gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
>         regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
>                                ov8856->supplies);
> @@ -2293,21 +2287,18 @@ static int ov8856_get_hwcfg(struct ov8856 *ov8856)
>                 dev_warn(dev, "external clock rate %u is unsupported",
>                          xvclk_rate);
>  
> -       if (!is_acpi_node(fwnode)) {
> -               ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> -                                                            GPIOD_OUT_LOW);
> -               if (IS_ERR(ov8856->reset_gpio))
> -                       return PTR_ERR(ov8856->reset_gpio);
> +       ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +                                                    GPIOD_OUT_LOW);
> +       if (IS_ERR(ov8856->reset_gpio))
> +               return PTR_ERR(ov8856->reset_gpio);
>  
> -               for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> -                       ov8856->supplies[i].supply = ov8856_supply_names[i];
> +       for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> +               ov8856->supplies[i].supply = ov8856_supply_names[i];
>  
> -               ret = devm_regulator_bulk_get(dev,
> -                                             ARRAY_SIZE(ov8856_supply_names),
> -                                             ov8856->supplies);
> -               if (ret)
> -                       return ret;
> -       }
> +       ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names),
> +                                     ov8856->supplies);
> +       if (ret)
> +               return ret;
>  
>         ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>         if (!ep)
> -- 
> 2.25.1
>