Hi Sowjanya,

On Fri, Jul 31, 2020 at 09:34:15AM -0700, Sowjanya Komatineni wrote:

On 7/31/20 9:26 AM, Sakari Ailus wrote:

quoted

Hi Sowjanya,

Thanks for the patch.

On Mon, Jul 20, 2020 at 10:01:34AM -0700, Sowjanya Komatineni wrote:

quoted

IMX274 has VANA analog 2.8V supply, VDIG digital core 1.8V supply,
and VDDL digital io 1.2V supply which are optional based on camera
module design.

IMX274 also need external 24Mhz clock and is optional based on
camera module design.

The sensor appears to be able to use other frequencies, too. Could you
check in the driver the frequency is correct? This should be found in DT
bindings, too.

External input clock is not in DT. So added it as part of this series.

We are mostly using 24Mhz I/P with IMX274 on designs we have and also on
leopard module which has onboard XTAL for 24Mhz

Yes. This information still should be found in DT as the xtal isn't part of
the sensor.

quoted

quoted

This patch adds support for IMX274 power on and off to enable and
disable these supplies and external clock.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
  drivers/media/i2c/imx274.c | 132 +++++++++++++++++++++++++++++++++++++++++++--
  1 file changed, 129 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 55869ff..7157b1d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c

@@ -19,6 +19,7 @@
  #include <linux/module.h>
  #include <linux/of_gpio.h>
  #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
  #include <linux/slab.h>
  #include <linux/v4l2-mediabus.h>
  #include <linux/videodev2.h>

@@ -131,6 +132,15 @@
  #define IMX274_TABLE_WAIT_MS			0
  #define IMX274_TABLE_END			1
+/* regulator supplies */
+static const char * const imx274_supply_names[] = {
+	"VDDL",  /* IF (1.2V) supply */
+	"VDIG",  /* Digital Core (1.8V) supply */
+	"VANA",  /* Analog (2.8V) supply */
+};
+
+#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names)

Please use ARRAY_SIZE() directly.

quoted

+
  /*
   * imx274 I2C operation related structure
   */

@@ -501,6 +511,8 @@ struct imx274_ctrls {
   * @frame_rate: V4L2 frame rate structure
   * @regmap: Pointer to regmap structure
   * @reset_gpio: Pointer to reset gpio
+ * @supplies: imx274 analog and digital supplies
+ * @inck: input clock to imx274
   * @lock: Mutex structure
   * @mode: Parameters for the selected readout mode
   */

@@ -514,6 +526,8 @@ struct stimx274 {
  	struct v4l2_fract frame_interval;
  	struct regmap *regmap;
  	struct gpio_desc *reset_gpio;
+	struct regulator *supplies[IMX274_NUM_SUPPLIES];
+	struct clk *inck;
  	struct mutex lock; /* mutex lock for operations */
  	const struct imx274_mode *mode;
  };

@@ -767,6 +781,99 @@ static void imx274_reset(struct stimx274 *priv, int rst)
  	usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
  }
+/*
+ * imx274_power_on - Function called to power on the sensor
+ * @imx274: Pointer to device structure
+ */
+static int imx274_power_on(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct stimx274 *imx274 = to_imx274(sd);
+	int i, ret;
+
+	ret = clk_prepare_enable(imx274->inck);
+	if (ret) {
+		dev_err(&imx274->client->dev,
+			"Failed to enable input clock: %d\n", ret);
+		return ret;
+	}
+

Could you use regulator_bulk_enable() instead? Same for disable.

Using regulator_bulk_enable() makes these regulators mandatory.

How? regulator_bulk_enable() simply does call regulator_enable() on all the
regulators.

-- 
Sakari Ailus