diff mbox series

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

Message ID 20231010005126.3425444-3-kieran.bingham@ideasonboard.com
State New
Headers
Series [1/5] media: dt-bindings: media: imx335: Add supply bindings |

Commit Message

Kieran Bingham Oct. 10, 2023, 12:51 a.m. UTC 
  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(-)

Comments

Umang Jain Oct. 10, 2023, 4:06 a.m. UTC | #1 
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;
>   }
kernel test robot Oct. 10, 2023, 4:10 a.m. UTC | #2 
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'


vim +159 drivers/media/i2c/imx335.c

45d19b5fb9aeab Martina Krasteva 2021-05-27  116  
45d19b5fb9aeab Martina Krasteva 2021-05-27  117  /**
45d19b5fb9aeab Martina Krasteva 2021-05-27  118   * struct imx335 - imx335 sensor device structure
45d19b5fb9aeab Martina Krasteva 2021-05-27  119   * @dev: Pointer to generic device
45d19b5fb9aeab Martina Krasteva 2021-05-27  120   * @client: Pointer to i2c client
45d19b5fb9aeab Martina Krasteva 2021-05-27  121   * @sd: V4L2 sub-device
45d19b5fb9aeab Martina Krasteva 2021-05-27  122   * @pad: Media pad. Only one pad supported
45d19b5fb9aeab Martina Krasteva 2021-05-27  123   * @reset_gpio: Sensor reset gpio
45d19b5fb9aeab Martina Krasteva 2021-05-27  124   * @inclk: Sensor input clock
45d19b5fb9aeab Martina Krasteva 2021-05-27  125   * @ctrl_handler: V4L2 control handler
45d19b5fb9aeab Martina Krasteva 2021-05-27  126   * @link_freq_ctrl: Pointer to link frequency control
45d19b5fb9aeab Martina Krasteva 2021-05-27  127   * @pclk_ctrl: Pointer to pixel clock control
45d19b5fb9aeab Martina Krasteva 2021-05-27  128   * @hblank_ctrl: Pointer to horizontal blanking control
45d19b5fb9aeab Martina Krasteva 2021-05-27  129   * @vblank_ctrl: Pointer to vertical blanking control
45d19b5fb9aeab Martina Krasteva 2021-05-27  130   * @exp_ctrl: Pointer to exposure control
45d19b5fb9aeab Martina Krasteva 2021-05-27  131   * @again_ctrl: Pointer to analog gain control
45d19b5fb9aeab Martina Krasteva 2021-05-27  132   * @vblank: Vertical blanking in lines
45d19b5fb9aeab Martina Krasteva 2021-05-27  133   * @cur_mode: Pointer to current selected sensor mode
45d19b5fb9aeab Martina Krasteva 2021-05-27  134   * @mutex: Mutex for serializing sensor controls
45d19b5fb9aeab Martina Krasteva 2021-05-27  135   * @streaming: Flag indicating streaming state
45d19b5fb9aeab Martina Krasteva 2021-05-27  136   */
45d19b5fb9aeab Martina Krasteva 2021-05-27  137  struct imx335 {
45d19b5fb9aeab Martina Krasteva 2021-05-27  138  	struct device *dev;
45d19b5fb9aeab Martina Krasteva 2021-05-27  139  	struct i2c_client *client;
45d19b5fb9aeab Martina Krasteva 2021-05-27  140  	struct v4l2_subdev sd;
45d19b5fb9aeab Martina Krasteva 2021-05-27  141  	struct media_pad pad;
45d19b5fb9aeab Martina Krasteva 2021-05-27  142  	struct gpio_desc *reset_gpio;
15931cfe0b52d1 Kieran Bingham   2023-10-10  143  	struct regulator_bulk_data supplies[IMX335_NUM_SUPPLIES];
15931cfe0b52d1 Kieran Bingham   2023-10-10  144  
45d19b5fb9aeab Martina Krasteva 2021-05-27  145  	struct clk *inclk;
45d19b5fb9aeab Martina Krasteva 2021-05-27  146  	struct v4l2_ctrl_handler ctrl_handler;
45d19b5fb9aeab Martina Krasteva 2021-05-27  147  	struct v4l2_ctrl *link_freq_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  148  	struct v4l2_ctrl *pclk_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  149  	struct v4l2_ctrl *hblank_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  150  	struct v4l2_ctrl *vblank_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  151  	struct {
45d19b5fb9aeab Martina Krasteva 2021-05-27  152  		struct v4l2_ctrl *exp_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  153  		struct v4l2_ctrl *again_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  154  	};
45d19b5fb9aeab Martina Krasteva 2021-05-27  155  	u32 vblank;
45d19b5fb9aeab Martina Krasteva 2021-05-27  156  	const struct imx335_mode *cur_mode;
45d19b5fb9aeab Martina Krasteva 2021-05-27  157  	struct mutex mutex;
45d19b5fb9aeab Martina Krasteva 2021-05-27  158  	bool streaming;
45d19b5fb9aeab Martina Krasteva 2021-05-27 @159  };
45d19b5fb9aeab Martina Krasteva 2021-05-27  160
Sakari Ailus Oct. 10, 2023, 6:12 a.m. UTC | #3 
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?

> +	 * 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);
> +	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 */

You're not handling the error case later on in the function.

> +
> +	/* 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 */
>  
>  	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;
>  }
Sakari Ailus Oct. 11, 2023, 11:06 a.m. UTC | #4 
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.
diff mbox series

Patch

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);
+	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 */
 
 	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;
 }