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@xxxxxxxxxxxxxxxx> > > > > --- > > > > 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