Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Wed, 11 Oct 2023 11:06:19 +0000
From:   Sakari Ailus <sakari.ailus@iki.fi>
To:     Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc:     linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        "Paul J. Murphy" <paul.j.murphy@intel.com>,
        Daniele Alessandrelli <daniele.alessandrelli@intel.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies
Message-ID: <ZSaBq5ucJ5PrxI00@valkosipuli.retiisi.eu>
References: <20231010005126.3425444-1-kieran.bingham@ideasonboard.com>
 <20231010005126.3425444-3-kieran.bingham@ideasonboard.com>
 <ZSTrOB10pDhJq6gG@valkosipuli.retiisi.eu>
 <169701731909.277971.10840095252716847805@ping.linuxembedded.co.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <169701731909.277971.10840095252716847805@ping.linuxembedded.co.uk>
Precedence: bulk

Hi Kieran,

On Wed, Oct 11, 2023 at 10:41:59AM +0100, Kieran Bingham wrote:
> Quoting Sakari Ailus (2023-10-10 07:12:08)
> > Hi Kieran,
> > 
> > On Tue, Oct 10, 2023 at 01:51:23AM +0100, Kieran Bingham wrote:
> > > Provide support for enabling and disabling regulator supplies to control
> > > power to the camera sensor.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/imx335.c | 41 ++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 39 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > > index ec729126274b..bf12b9b69fce 100644
> > > --- a/drivers/media/i2c/imx335.c
> > > +++ b/drivers/media/i2c/imx335.c
> > > @@ -75,6 +75,19 @@ struct imx335_reg_list {
> > >       const struct imx335_reg *regs;
> > >  };
> > >  
> > > +static const char * const imx335_supply_name[] = {
> > > +     /*
> > > +      * Turn on the power supplies so that they rise in order of
> > > +      *           1.2v,-> 1.8 -> 2.9v
> > 
> > This won't happen with regulator_bulk_enable(). Does the spec require this?
> 
> Every camera I've ever seen handles this in hardware. (I know that's not
> an excuse as somewhere there could be a device that routes each of these
> separately).
> 
> Perhaps I shouldn't have added the comment ;-) But I thought it was
> useful while I was working through anyway, and could be important for
> other devices indeed.
> 
> The datasheet states:
> 
> ```
> 1. Turn On the power supplies so that the power supplies rise in order
> of 1.2 V power supply (DVDD1, DVDD2) → 1.8 V power supply (OVDD) → 2.9 V
> power supply (AVDD1, AVDD2). In addition, all power supplies should
> finish rising within 200 ms.

That's an annoying requirement but I'd presume this to work just fine in
practice. The device is reset in any case (as you describe below). Some
boards might not wire the reset GPIO though, and then it's when it gets
interesting.

To literally implement the documented sequence, you should separately
enable the regulators one by one.

Although I don't object just removing the comment either.

-- 
Regards,

Sakari Ailus