RE: [PATCH v3 1/2] media: i2c: ov8856 : remove ACPI node bypass mechanism

Hi Sakari,

Thanks for the review.

> -----Original Message-----
> From: Sakari Ailus <[email protected]>
> Sent: Wednesday, July 29, 2026 2:01 AM
> To: Yeh, Serin <[email protected]>
> Cc: [email protected]; Su, Jimmy <[email protected]>; Sapre, Sarang <[email protected]>; [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH v3 1/2] media: i2c: ov8856 : remove ACPI node bypass mechanism
>
> Hi Serin,
>
> On Thu, Jul 23, 2026 at 03:40:36PM +0800, Serin Yeh wrote:
> > The driver currently skips power-on and power-off sequences when
> > running with ACPI devices by checking is_acpi_node(). This results in
> > all power configuration steps (GPIO, and regulators) being bypassed.
> >
> > Remove the is_acpi_node() checks to ensure that the full power
> > sequence is always executed regardless of the firmware interface.
> >
> > With these changes, the driver behaves consistently across DT and ACPI
> > platforms and ensures correct hardware initialization.
> >
> > Fixes: d2fa1134a48b ("media: i2c: ov8856: Use V4L2 legacy sensor clock
> > helper")
> > Cc: [email protected]
> > Signed-off-by: Serin Yeh <[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);
>
> Can you fix this (multiple devm_regulator_bulk_get() should never have been
> done) first in a separate patch, as Bryan suggested?
After discussing this with you, I believe the devm_regulator_bulk_get() logic is clear. The devm_regulator_bulk_get() is currently guarded by the is_acpi_node() conditional branch rather than being placed inside a loop.
I'll address Bryan's suggestion and modify the commit log in v4.

Thanks.

Regards,
Serin Yeh