Re: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting Sakari Ailus (2023-10-11 12:06:19)
> 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.

Correct - this board does not expose the reset/XCLR to me anyway, so I
can not control that.

> 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.

Given that neither the existing user, nor this user (me) need this,
*and* the schematics of my (working) camera module show that it's fine
to enable all regulators at the same time - I'll remove the comment.

--
Thanks

Kieran

> 
> -- 
> Regards,
> 
> Sakari Ailus