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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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.

-- 
Regards,

Laurent Pinchart




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux