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

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(-)

Hi Kieran,

Thank you for the patch.

On 10/10/23 6:21 AM, 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
> +	 * All power supplies should finish rising within 200ms.
> +	 */
> +	"avdd", /* Analog (2.9V) supply */
> +	"ovdd", /* Digital I/O (1.8V) supply */
> +	"dvdd", /* Digital Core (1.2V) supply */
> +};
> +
> +#define IMX335_NUM_SUPPLIES ARRAY_SIZE(imx335_supply_name)
> +
>   /**
>    * struct imx335_mode - imx335 sensor mode structure
>    * @width: Frame width
> @@ -126,6 +139,8 @@ struct imx335 {
>   	struct v4l2_subdev sd;
>   	struct media_pad pad;
>   	struct gpio_desc *reset_gpio;
> +	struct regulator_bulk_data supplies[IMX335_NUM_SUPPLIES];
> +
>   	struct clk *inclk;
>   	struct v4l2_ctrl_handler ctrl_handler;
>   	struct v4l2_ctrl *link_freq_ctrl;
> @@ -781,6 +796,17 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
>   		return PTR_ERR(imx335->reset_gpio);
>   	}
>   
> +	for (i = 0; i < IMX335_NUM_SUPPLIES; i++)
> +		imx335->supplies[i].supply = imx335_supply_name[i];
> +
> +	ret = devm_regulator_bulk_get(imx335->dev,
> +				      IMX335_NUM_SUPPLIES,
> +				      imx335->supplies);

Shouldn't this go directly in  probe() function ? (Taking IMX219 driver 
as a reference..)
> +	if (ret) {
> +		dev_err(imx335->dev, "Failed to get regulators\n");
> +		return ret;
> +	}
> +
>   	/* Get sensor input clock */
>   	imx335->inclk = devm_clk_get(imx335->dev, NULL);
>   	if (IS_ERR(imx335->inclk)) {
> @@ -859,6 +885,17 @@ static int imx335_power_on(struct device *dev)
>   	struct imx335 *imx335 = to_imx335(sd);
>   	int ret;
>   
> +	ret = regulator_bulk_enable(IMX335_NUM_SUPPLIES,
> +				    imx335->supplies);
> +	if (ret) {
> +		dev_err(dev, "%s: failed to enable regulators\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	usleep_range(500, 550); /* Tlow */
> +
> +	/* Set XCLR */
>   	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
>   
>   	ret = clk_prepare_enable(imx335->inclk);
> @@ -867,7 +904,7 @@ static int imx335_power_on(struct device *dev)
>   		goto error_reset;
>   	}
>   
> -	usleep_range(20, 22);
> +	usleep_range(20, 22); /* T4 */

It would be help to document this addition of comment in the commit 
message as well.
>   
>   	return 0;
>   
> @@ -889,8 +926,8 @@ static int imx335_power_off(struct device *dev)
>   	struct imx335 *imx335 = to_imx335(sd);
>   
>   	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
> -
>   	clk_disable_unprepare(imx335->inclk);
> +	regulator_bulk_disable(IMX335_NUM_SUPPLIES, imx335->supplies);
>   
>   	return 0;
>   }

Quoting kernel test robot (2023-10-10 05:10:17)
> Hi Kieran,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on media-tree/master]
> [also build test WARNING on sailus-media-tree/streams linus/master v6.6-rc5 next-20231009]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Kieran-Bingham/media-dt-bindings-media-imx335-Add-supply-bindings/20231010-085313
> base:   git://linuxtv.org/media_tree.git master
> patch link:    https://lore.kernel.org/r/20231010005126.3425444-3-kieran.bingham%40ideasonboard.com
> patch subject: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231010/202310101224.43dpo3Ng-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231010/202310101224.43dpo3Ng-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202310101224.43dpo3Ng-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
> >> drivers/media/i2c/imx335.c:159: warning: Function parameter or member 'supplies' not described in 'imx335'
> 

Aha, thank you KTR - I'll fix this.
--
Kieran

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