| Message ID | 20231010005126.3425444-3-kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Sony IMX335 improvements | expand |
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; > }
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
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; > }
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 --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; }
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(-)