diff mbox series

[v2,1/2] media: ov2740: only do OTP data read on demand from user

Message ID 1604907581-2808-1-git-send-email-bingbu.cao@intel.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/2] media: ov2740: only do OTP data read on demand from user | expand

Commit Message

Bingbu Cao Nov. 9, 2020, 7:39 a.m. UTC 
OTP data access of ov2740 in probe need power up, it may cause
the camera flash LED blink during probe if the LED use same power
rail with camera, this patch move the OTP data access out of
probe, it will only occur on demand from user by nvmem sysfs.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
Signed-off-by: Qingwu Zhang <qingwu.zhang@intel.com>
---
 drivers/media/i2c/ov2740.c | 87 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 66 insertions(+), 21 deletions(-)

Comments

Sakari Ailus Nov. 10, 2020, 11:38 a.m. UTC | #1 
Hi Bingbu,

On Mon, Nov 09, 2020 at 03:39:40PM +0800, Bingbu Cao wrote:
> OTP data access of ov2740 in probe need power up, it may cause
> the camera flash LED blink during probe if the LED use same power
> rail with camera, this patch move the OTP data access out of
> probe, it will only occur on demand from user by nvmem sysfs.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> Signed-off-by: Qingwu Zhang <qingwu.zhang@intel.com>
> ---
>  drivers/media/i2c/ov2740.c | 87 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 66 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 64ecb6917dd3..4bb4fe4bcab6 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -71,9 +71,11 @@
>  #define OV2740_REG_OTP_CUSTOMER		0x7010
>  
>  struct nvm_data {
> -	char *nvm_buffer;
> +	struct i2c_client *client;
>  	struct nvmem_device *nvmem;
>  	struct regmap *regmap;
> +	char *nvm_buffer;
> +	bool ready;

Thanks for the update, this indeed makes the set much easier to review.

Would it be possible to just rely on the nvm_buffer, and remove ready
field? Essentially, having NULL nvm_buffer would tell the memory isn't
allocated nor the NVM data is loaded. What do you think?

This would also mean moving the memory allocation where NVM data is read.
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 64ecb6917dd3..4bb4fe4bcab6 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -71,9 +71,11 @@ 
 #define OV2740_REG_OTP_CUSTOMER		0x7010
 
 struct nvm_data {
-	char *nvm_buffer;
+	struct i2c_client *client;
 	struct nvmem_device *nvmem;
 	struct regmap *regmap;
+	char *nvm_buffer;
+	bool ready;
 };
 
 enum {
@@ -335,6 +337,9 @@  struct ov2740 {
 
 	/* Streaming on/off */
 	bool streaming;
+
+	/* NVM data inforamtion */
+	struct nvm_data *nvm;
 };
 
 static inline struct ov2740 *to_ov2740(struct v4l2_subdev *subdev)
@@ -930,18 +935,26 @@  static int ov2740_remove(struct i2c_client *client)
 	return 0;
 }
 
-static int ov2740_load_otp_data(struct i2c_client *client, struct nvm_data *nvm)
+static int ov2740_load_otp_data(struct nvm_data *nvm)
 {
+	struct i2c_client *client = nvm->client;
 	struct ov2740 *ov2740 = to_ov2740(i2c_get_clientdata(client));
 	u32 isp_ctrl00 = 0;
 	u32 isp_ctrl01 = 0;
 	int ret;
 
+	if (!nvm)
+		return -EINVAL;
+
+	if (nvm->ready)
+		return 0;
+
 	ret = ov2740_read_reg(ov2740, OV2740_REG_ISP_CTRL00, 1, &isp_ctrl00);
 	if (ret) {
 		dev_err(&client->dev, "failed to read ISP CTRL00\n");
 		goto exit;
 	}
+
 	ret = ov2740_read_reg(ov2740, OV2740_REG_ISP_CTRL01, 1, &isp_ctrl01);
 	if (ret) {
 		dev_err(&client->dev, "failed to read ISP CTRL01\n");
@@ -952,7 +965,7 @@  static int ov2740_load_otp_data(struct i2c_client *client, struct nvm_data *nvm)
 	ret = ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL00, 1,
 			       isp_ctrl00 & ~BIT(5));
 	if (ret) {
-		dev_err(&client->dev, "failed to write ISP CTRL00\n");
+		dev_err(&client->dev, "failed to set ISP CTRL00\n");
 		goto exit;
 	}
 
@@ -960,14 +973,14 @@  static int ov2740_load_otp_data(struct i2c_client *client, struct nvm_data *nvm)
 	ret = ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL01, 1,
 			       isp_ctrl01 & ~BIT(7));
 	if (ret) {
-		dev_err(&client->dev, "failed to write ISP CTRL01\n");
+		dev_err(&client->dev, "failed to set ISP CTRL01\n");
 		goto exit;
 	}
 
 	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
 			       OV2740_MODE_STREAMING);
 	if (ret) {
-		dev_err(&client->dev, "failed to start streaming\n");
+		dev_err(&client->dev, "failed to set streaming mode\n");
 		goto exit;
 	}
 
@@ -984,11 +997,26 @@  static int ov2740_load_otp_data(struct i2c_client *client, struct nvm_data *nvm)
 		goto exit;
 	}
 
-	ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
-			 OV2740_MODE_STANDBY);
-	ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL01, 1, isp_ctrl01);
-	ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL00, 1, isp_ctrl00);
+	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
+			       OV2740_MODE_STANDBY);
+	if (ret) {
+		dev_err(&client->dev, "failed to set streaming mode\n");
+		goto exit;
+	}
+
+	ret = ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL01, 1, isp_ctrl01);
+	if (ret) {
+		dev_err(&client->dev, "failed to set ISP CTRL01\n");
+		goto exit;
+	}
+
+	ret = ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL00, 1, isp_ctrl00);
+	if (ret) {
+		dev_err(&client->dev, "failed to set ISP CTRL00\n");
+		goto exit;
+	}
 
+	nvm->ready = true;
 exit:
 	return ret;
 }
@@ -997,20 +1025,38 @@  static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
 			     size_t count)
 {
 	struct nvm_data *nvm = priv;
+	struct v4l2_subdev *sd = i2c_get_clientdata(nvm->client);
+	struct device *dev = &nvm->client->dev;
+	struct ov2740 *ov2740 = to_ov2740(sd);
+	int ret = 0;
 
-	memcpy(val, nvm->nvm_buffer + off, count);
+	mutex_lock(&ov2740->mutex);
 
-	return 0;
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
+		goto exit;
+	}
+
+	ret = ov2740_load_otp_data(nvm);
+	if (!ret)
+		memcpy(val, nvm->nvm_buffer + off, count);
+
+	pm_runtime_put(dev);
+exit:
+	mutex_unlock(&ov2740->mutex);
+	return ret;
 }
 
-static int ov2740_register_nvmem(struct i2c_client *client)
+static int ov2740_register_nvmem(struct i2c_client *client,
+				 struct ov2740 *ov2740)
 {
 	struct nvm_data *nvm;
 	struct regmap_config regmap_config = { };
 	struct nvmem_config nvmem_config = { };
 	struct regmap *regmap;
 	struct device *dev = &client->dev;
-	int ret = 0;
+	int ret;
 
 	nvm = devm_kzalloc(dev, sizeof(*nvm), GFP_KERNEL);
 	if (!nvm)
@@ -1028,12 +1074,7 @@  static int ov2740_register_nvmem(struct i2c_client *client)
 		return PTR_ERR(regmap);
 
 	nvm->regmap = regmap;
-
-	ret = ov2740_load_otp_data(client, nvm);
-	if (ret) {
-		dev_err(dev, "failed to load OTP data, ret %d\n", ret);
-		return ret;
-	}
+	nvm->client = client;
 
 	nvmem_config.name = dev_name(dev);
 	nvmem_config.dev = dev;
@@ -1051,7 +1092,11 @@  static int ov2740_register_nvmem(struct i2c_client *client)
 
 	nvm->nvmem = devm_nvmem_register(dev, &nvmem_config);
 
-	return PTR_ERR_OR_ZERO(nvm->nvmem);
+	ret = PTR_ERR_OR_ZERO(nvm->nvmem);
+	if (!ret)
+		ov2740->nvm = nvm;
+
+	return ret;
 }
 
 static int ov2740_probe(struct i2c_client *client)
@@ -1103,7 +1148,7 @@  static int ov2740_probe(struct i2c_client *client)
 		goto probe_error_media_entity_cleanup;
 	}
 
-	ret = ov2740_register_nvmem(client);
+	ret = ov2740_register_nvmem(client, ov2740);
 	if (ret)
 		dev_warn(&client->dev, "register nvmem failed, ret %d\n", ret);