Re: [PATCH 00/72] media: i2c: Reduce cargo-cult

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Fri, Jul 25, 2025 at 10:35:28AM +0000, Tarang Raval wrote:
> > On Fri, Jul 25, 2025 at 07:00:40AM +0000, Tarang Raval wrote:
> > > > On Thu, Jul 24, 2025 at 02:20:10PM +0000, Tarang Raval wrote:
> > > > > > > > > 2. In the regulator code, you can reduce boilerplate by using
> > > > > > > > >    devm_regulator_bulk_get_enable().
> > > > > > > >
> > > > > > > > devm_regulator_bulk_get_enable() doesn't seem to be a good idea. You
> > > > > > > > generally don't want to enable power everywhere unconditionally, and
> > > > > > > > sensors very often need a guaranteed power up sequence.
> > >
> > > -----(1)
> > >
> > > > > > >
> > > > > > > The regulators are optional, we supply power to the camera sensor directly
> > > > > > > through dedicated power rails and there is no strict enable sequence
> > > > > > > required in this case.
> > > > > >
> > > > > > What exactly do you mean by "this case" ? Are you talking about one
> > > > > > particular sensor ? One particular camera module ?
> > > > >
> > > > > Laurent, by “this case” I meant the common scenario where power to the
> > > > > camera sensor is supplied by a PMIC regulator that is always-on. In such
> > > > > setups, the regulator is fixed and cannot be enabled or disabled from the
> > > > > driver, the sensor is always powered.
> > > > >
> > > > > This is what I’ve seen in most platforms, where the CSI input connector
> > > > > provides fixed 3.3V/1.8V power rails directly to the camera module.
> > > > >
> > > > > Of course, if the camera supply comes from a dedicated regulator controlled
> > > > > via a GPIO, then the driver would need to handle enable/disable sequencing
> > > > > explicitly. But I’m specifically referring to the first case, where the power rails
> > > > > are always-on.
> > > >
> > > > How does the sensor driver know which of those two cases it is dealing
> > > > with ?
> > >
> > > The sensor driver typically determines this via the presence (or absence)
> > > of regulator supply entries in the Device Tree. If a supply is not defined,
> > > it's assumed to be always-on (e.g., provided by the board via fixed rails).
> > 
> > Do we have sensor drivers that check the presense of supply properties ?
> > Drivers generally shouldn't.
> > 
> > > When defined, the driver retrieves and manages the regulator. This approach
> > > allows a single driver to support both cases, by treating supplies as optional
> > > and only enabling them when explicitly defined.
> > 
> > I don't see what you're trying to do here. A sensor always needs
> > supplies, regardless of whether or not they're always on. Drivers should
> > get the supplies with regulator_get() (or possibly the bulk API), and
> > then implement the power enable/disable sequences that the sensor
> > requires. If all suplies are manually controllable, this will produce
> > the correct sequence. If the supplies are always on, it will be a no-op.
> > That's a single implementation in the driver, you don't need to care
> > about the nature of the supplies, or their presence in DT.
> > 
> > > At comment (1): you gave two reasons why we cannot use devm_regulator_bulk_get_enable.
> > >
> > > What I’m trying to say is:
> > >
> > > You mentioned "generally don't want to enable power everywhere unconditionally,"
> > > but on almost every platform, the power rails are always-on.
> > 
> > "almost every platform" doesn't sound right to me. It does happen though.
> > 
> > > And regarding the second point — "sensors very often need a guaranteed power-up sequence"
> > > I don’t understand why this would be an issue. Even if we use devm_regulator_bulk_get_enable,
> > > the power-up sequence remains the same. So how is it not a good option in this case?
> > 
> > Because the bulk API enables all regulators in parallel, it doesn't
> > guarantee sequencing.
> 
> Except for a few drivers, almost all camera drivers use the bulk API, which suggests
> that a guaranteed power-up sequence may not be strictly required in most cases.
>  
> > Don't use devm_regulator_bulk_get_enable() in sensor drivers, implement
> > power enable/disable functions that do the right thing. That's the code
> > pattern I want to see.
> 
> Perhaps I wasnt clear in my explanation. If you look at the patch below, you'll 
> see that we are not changing any sequencing behavior.

You end up getting regulators every time power is enabled, and you don't
turn the supplies off at power off time. How is that even supposed to
work ? It completely breaks power management.

> I am not suggesting we use this API everywhere, only where it's appropriate and 
> doesn't compromise sequencing requirements. 
> 
> Best Regards,
> Tarang
> 
> ------
> 
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> index da618c8cbadc..4dbf7215cef4 100644
> --- a/drivers/media/i2c/imx283.c
> +++ b/drivers/media/i2c/imx283.c
> @@ -1176,8 +1176,8 @@ static int imx283_power_on(struct device *dev)
>         struct imx283 *imx283 = to_imx283(sd);
>         int ret;
>  
> -       ret = regulator_bulk_enable(ARRAY_SIZE(imx283_supply_name),
> -                                   imx283->supplies);
> +       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(imx283_supply_name),
> +                                   imx283_supply_name);
>         if (ret) {
>                 dev_err(imx283->dev, "failed to enable regulators\n");
>                 return ret;
> @@ -1186,7 +1186,7 @@ static int imx283_power_on(struct device *dev)
>         ret = clk_prepare_enable(imx283->xclk);
>         if (ret) {
>                 dev_err(imx283->dev, "failed to enable clock\n");
> -               goto reg_off;
> +               return ret;
>         }
>  
>         gpiod_set_value_cansleep(imx283->reset_gpio, 0);
> @@ -1195,10 +1195,6 @@ static int imx283_power_on(struct device *dev)
>                      IMX283_XCLR_MIN_DELAY_US + IMX283_XCLR_DELAY_RANGE_US);
>  
>         return 0;
> -
> -reg_off:
> -       regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies);
> -       return ret;
>  }
>  
>  static int imx283_power_off(struct device *dev)
> @@ -1207,24 +1203,11 @@ static int imx283_power_off(struct device *dev)
>         struct imx283 *imx283 = to_imx283(sd);
>  
>         gpiod_set_value_cansleep(imx283->reset_gpio, 1);
> -       regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies);
>         clk_disable_unprepare(imx283->xclk);
>  
>         return 0;
>  }
>  
> -static int imx283_get_regulators(struct imx283 *imx283)
> -{
> -       unsigned int i;
> -
> -       for (i = 0; i < ARRAY_SIZE(imx283_supply_name); i++)
> -               imx283->supplies[i].supply = imx283_supply_name[i];
> -
> -       return devm_regulator_bulk_get(imx283->dev,
> -                                      ARRAY_SIZE(imx283_supply_name),
> -                                      imx283->supplies);
> -}
> -
>  /* Verify chip ID */
>  static int imx283_identify_module(struct imx283 *imx283)
>  {
> @@ -1480,12 +1463,6 @@ static int imx283_probe(struct i2c_client *client)
>                 return -EINVAL;
>         }
>  
> -       ret = imx283_get_regulators(imx283);
> -       if (ret) {
> -               return dev_err_probe(imx283->dev, ret,
> -                               "failed to get regulators\n");
> -       }
> -
>         ret = imx283_parse_endpoint(imx283);
>         if (ret) {
>                 dev_err(imx283->dev, "failed to parse endpoint configuration\n");

-- 
Regards,

Laurent Pinchart