> 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.
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");
