[PATCH] media: ov2740: add NVMEM interface to read customized OTP data

[PATCH] media: ov2740: add NVMEM interface to read customized OTP data

[Bingbu Cao]

From: Qingwu Zhang <qingwu.zhang@intel.com>

ov2740 includes 512bytes of one-time programmable memory and
256 bytes are reserved for customers which can be used to store
customized information. This patch provide an NVMEM interface
to support read out the customized data in OTP.

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

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 33025c6d23ac..fd0b6a903ec1 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -7,6 +7,8 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/nvmem-provider.h>
+#include <linux/regmap.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -59,6 +61,21 @@
 #define OV2740_TEST_PATTERN_ENABLE	BIT(7)
 #define OV2740_TEST_PATTERN_BAR_SHIFT	2
 
+/* ISP CTRL00 */
+#define OV2740_REG_ISP_CTRL00		0x5000
+/* ISP CTRL01 */
+#define OV2740_REG_ISP_CTRL01		0x5001
+/* Customer Addresses: 0x7010 - 0x710F */
+#define CUSTOMER_USE_OTP_SIZE		0x100
+/* OTP registers from sensor */
+#define OV2740_REG_OTP_CUSTOMER		0x7010
+
+struct nvm_data {
+	char *nvm_buffer;
+	struct nvmem_device *nvmem;
+	struct regmap *regmap;
+};
+
 enum {
 	OV2740_LINK_FREQ_360MHZ_INDEX,
 };
@@ -915,6 +932,130 @@ static int ov2740_remove(struct i2c_client *client)
 	return 0;
 }
 
+static int ov2740_load_otp_data(struct i2c_client *client, struct nvm_data *nvm)
+{
+	struct ov2740 *ov2740 = to_ov2740(i2c_get_clientdata(client));
+	u32 isp_ctrl00 = 0;
+	u32 isp_ctrl01 = 0;
+	int ret;
+
+	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");
+		goto exit;
+	}
+
+	/* Clear bit 5 of ISP CTRL00 */
+	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");
+		goto exit;
+	}
+
+	/* Clear bit 7 of ISP CTRL01 */
+	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");
+		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");
+		goto exit;
+	}
+
+	/*
+	 * Users are not allowed to access OTP-related registers and memory
+	 * during the 20 ms period after streaming starts (0x100 = 0x01).
+	 */
+	msleep(20);
+
+	ret = regmap_bulk_read(nvm->regmap, OV2740_REG_OTP_CUSTOMER,
+			       nvm->nvm_buffer, CUSTOMER_USE_OTP_SIZE);
+	if (ret) {
+		dev_err(&client->dev, "failed to read OTP data, ret %d\n", ret);
+		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);
+
+exit:
+	return ret;
+}
+
+static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
+			     size_t count)
+{
+	struct nvm_data *nvm = priv;
+
+	memcpy(val, nvm->nvm_buffer + off, count);
+
+	return 0;
+}
+
+static int ov2740_register_nvmem(struct i2c_client *client)
+{
+	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;
+
+	nvm = devm_kzalloc(dev, sizeof(*nvm), GFP_KERNEL);
+	if (!nvm)
+		return -ENOMEM;
+
+	regmap_config.val_bits = 8;
+	regmap_config.reg_bits = 16;
+	regmap_config.disable_locking = true;
+	regmap = devm_regmap_init_i2c(client, &regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	nvm->regmap = regmap;
+
+	nvmem_config.name = dev_name(dev);
+	nvmem_config.dev = dev;
+	nvmem_config.read_only = true;
+	nvmem_config.root_only = true;
+	nvmem_config.owner = THIS_MODULE;
+	nvmem_config.compat = true;
+	nvmem_config.base_dev = dev;
+	nvmem_config.reg_read = ov2740_nvmem_read;
+	nvmem_config.reg_write = NULL;
+	nvmem_config.priv = nvm;
+	nvmem_config.stride = 1;
+	nvmem_config.word_size = 1;
+	nvmem_config.size = CUSTOMER_USE_OTP_SIZE;
+
+	nvm->nvmem = devm_nvmem_register(dev, &nvmem_config);
+	if (IS_ERR(nvm->nvmem))
+		return PTR_ERR(nvm->nvmem);
+
+	nvm->nvm_buffer = devm_kzalloc(dev, CUSTOMER_USE_OTP_SIZE, GFP_KERNEL);
+	if (!nvm->nvm_buffer)
+		return -ENOMEM;
+
+	ret = ov2740_load_otp_data(client, nvm);
+	if (ret)
+		dev_err(dev, "failed to load OTP data, ret %d\n", ret);
+
+	return ret;
+}
+
 static int ov2740_probe(struct i2c_client *client)
 {
 	struct ov2740 *ov2740;
@@ -964,6 +1105,10 @@ static int ov2740_probe(struct i2c_client *client)
 		goto probe_error_media_entity_cleanup;
 	}
 
+	ret = ov2740_register_nvmem(client);
+	if (ret)
+		dev_err(&client->dev, "register nvmem failed, ret %d\n", ret);
+
 	/*
 	 * Device is already turned on by i2c-core with ACPI domain PM.
 	 * Enable runtime PM and turn off the device.
-- 
2.7.4

Re: [PATCH] media: ov2740: add NVMEM interface to read customized OTP data

[Sakari Ailus]

Hi Bingbu,

Thank you for the patch.

On Fri, Jun 12, 2020 at 05:42:02PM +0800, Bingbu Cao wrote:
> From: Qingwu Zhang <qingwu.zhang@intel.com>
> 
> ov2740 includes 512bytes of one-time programmable memory and
> 256 bytes are reserved for customers which can be used to store
> customized information. This patch provide an NVMEM interface
> to support read out the customized data in OTP.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> Signed-off-by: Qingwu Zhang <qingwu.zhang@intel.com>
> ---
>  drivers/media/i2c/ov2740.c | 145 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 145 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 33025c6d23ac..fd0b6a903ec1 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -7,6 +7,8 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/regmap.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -59,6 +61,21 @@
>  #define OV2740_TEST_PATTERN_ENABLE	BIT(7)
>  #define OV2740_TEST_PATTERN_BAR_SHIFT	2
>  
> +/* ISP CTRL00 */
> +#define OV2740_REG_ISP_CTRL00		0x5000
> +/* ISP CTRL01 */
> +#define OV2740_REG_ISP_CTRL01		0x5001
> +/* Customer Addresses: 0x7010 - 0x710F */
> +#define CUSTOMER_USE_OTP_SIZE		0x100
> +/* OTP registers from sensor */
> +#define OV2740_REG_OTP_CUSTOMER		0x7010
> +
> +struct nvm_data {
> +	char *nvm_buffer;
> +	struct nvmem_device *nvmem;
> +	struct regmap *regmap;
> +};
> +
>  enum {
>  	OV2740_LINK_FREQ_360MHZ_INDEX,
>  };
> @@ -915,6 +932,130 @@ static int ov2740_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static int ov2740_load_otp_data(struct i2c_client *client, struct nvm_data *nvm)
> +{
> +	struct ov2740 *ov2740 = to_ov2740(i2c_get_clientdata(client));
> +	u32 isp_ctrl00 = 0;
> +	u32 isp_ctrl01 = 0;
> +	int ret;
> +
> +	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");
> +		goto exit;
> +	}
> +
> +	/* Clear bit 5 of ISP CTRL00 */
> +	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");
> +		goto exit;
> +	}
> +
> +	/* Clear bit 7 of ISP CTRL01 */
> +	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");
> +		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");
> +		goto exit;
> +	}
> +
> +	/*
> +	 * Users are not allowed to access OTP-related registers and memory
> +	 * during the 20 ms period after streaming starts (0x100 = 0x01).
> +	 */
> +	msleep(20);
> +
> +	ret = regmap_bulk_read(nvm->regmap, OV2740_REG_OTP_CUSTOMER,
> +			       nvm->nvm_buffer, CUSTOMER_USE_OTP_SIZE);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read OTP data, ret %d\n", ret);
> +		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);
> +
> +exit:
> +	return ret;
> +}
> +
> +static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
> +			     size_t count)
> +{
> +	struct nvm_data *nvm = priv;
> +
> +	memcpy(val, nvm->nvm_buffer + off, count);
> +
> +	return 0;
> +}
> +
> +static int ov2740_register_nvmem(struct i2c_client *client)
> +{
> +	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;
> +
> +	nvm = devm_kzalloc(dev, sizeof(*nvm), GFP_KERNEL);
> +	if (!nvm)
> +		return -ENOMEM;
> +
> +	regmap_config.val_bits = 8;
> +	regmap_config.reg_bits = 16;
> +	regmap_config.disable_locking = true;
> +	regmap = devm_regmap_init_i2c(client, &regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	nvm->regmap = regmap;
> +
> +	nvmem_config.name = dev_name(dev);
> +	nvmem_config.dev = dev;
> +	nvmem_config.read_only = true;
> +	nvmem_config.root_only = true;
> +	nvmem_config.owner = THIS_MODULE;
> +	nvmem_config.compat = true;
> +	nvmem_config.base_dev = dev;
> +	nvmem_config.reg_read = ov2740_nvmem_read;
> +	nvmem_config.reg_write = NULL;
> +	nvmem_config.priv = nvm;
> +	nvmem_config.stride = 1;
> +	nvmem_config.word_size = 1;
> +	nvmem_config.size = CUSTOMER_USE_OTP_SIZE;
> +
> +	nvm->nvmem = devm_nvmem_register(dev, &nvmem_config);
> +	if (IS_ERR(nvm->nvmem))
> +		return PTR_ERR(nvm->nvmem);
> +
> +	nvm->nvm_buffer = devm_kzalloc(dev, CUSTOMER_USE_OTP_SIZE, GFP_KERNEL);
> +	if (!nvm->nvm_buffer)
> +		return -ENOMEM;
> +
> +	ret = ov2740_load_otp_data(client, nvm);
> +	if (ret)
> +		dev_err(dev, "failed to load OTP data, ret %d\n", ret);
> +
> +	return ret;
> +}
> +
>  static int ov2740_probe(struct i2c_client *client)
>  {
>  	struct ov2740 *ov2740;
> @@ -964,6 +1105,10 @@ static int ov2740_probe(struct i2c_client *client)
>  		goto probe_error_media_entity_cleanup;
>  	}
>  
> +	ret = ov2740_register_nvmem(client);
> +	if (ret)
> +		dev_err(&client->dev, "register nvmem failed, ret %d\n", ret);
> +
>  	/*
>  	 * Device is already turned on by i2c-core with ACPI domain PM.
>  	 * Enable runtime PM and turn off the device.

Could you add #ifdefs for CONFIG_NVMEM so this compiles when it's disabled?
It's bool so a simple #ifdef will do. I think ov2740_register_nvmem() could
have a dummy implementation so this wouldn't litter the probe function.

Thanks.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: ov2740: add NVMEM interface to read customized OTP data

[Srinivas Kandagatla]

On 15/06/2020 10:29, Sakari Ailus wrote:
>> +	ret = ov2740_register_nvmem(client);
>> +	if (ret)
>> +		dev_err(&client->dev, "register nvmem failed, ret %d\n", ret);
>> +
>>   	/*
>>   	 * Device is already turned on by i2c-core with ACPI domain PM.
>>   	 * Enable runtime PM and turn off the device.
> Could you add #ifdefs for CONFIG_NVMEM so this compiles when it's disabled?

NVMEM already has dummy functions. IMO its better to report an error to 
user as the current code does. This will atleast alert the users of 
existing nvmem provider that has been disabled!

However with ifdef users have to really look into code to be able to 
understand that there is nvmem provider as part of this driver.


Unless you have tight memory restrictions I see no great advantage of 
adding #ifdefs

--srini
> It's bool so a simple #ifdef will do. I think ov2740_register_nvmem() could
> have a dummy implementation so this wouldn't litter the probe function.
> 
> Thanks.

Re: [PATCH] media: ov2740: add NVMEM interface to read customized OTP data

[Sakari Ailus]

Hi Srinivas,

On Mon, Jun 15, 2020 at 10:46:20AM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 15/06/2020 10:29, Sakari Ailus wrote:
> > > +	ret = ov2740_register_nvmem(client);
> > > +	if (ret)
> > > +		dev_err(&client->dev, "register nvmem failed, ret %d\n", ret);
> > > +
> > >   	/*
> > >   	 * Device is already turned on by i2c-core with ACPI domain PM.
> > >   	 * Enable runtime PM and turn off the device.
> > Could you add #ifdefs for CONFIG_NVMEM so this compiles when it's disabled?
> 
> NVMEM already has dummy functions. IMO its better to report an error to user
> as the current code does. This will atleast alert the users of existing
> nvmem provider that has been disabled!
> 
> However with ifdef users have to really look into code to be able to
> understand that there is nvmem provider as part of this driver.

Right, I somehow missed there already were dummy functions for this.

The driver is a camera sensor driver that contains an EEPROM. I'd presume
that not all (or even many) users would care about the EEPROM content so
therefore requiring NVMEM to compile the driver seems unjustified.

Therefore the patch is fine as-is IMO.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: ov2740: add NVMEM interface to read customized OTP data

[Sergey Senozhatsky]

On (20/06/12 17:42), Bingbu Cao wrote:
>  
> +static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
> +			     size_t count)
> +{
> +	struct nvm_data *nvm = priv;
> +
> +	memcpy(val, nvm->nvm_buffer + off, count);
> +
> +	return 0;
> +}
> +
> +static int ov2740_register_nvmem(struct i2c_client *client)
> +{
> +	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;
> +
> +	nvm = devm_kzalloc(dev, sizeof(*nvm), GFP_KERNEL);
> +	if (!nvm)
> +		return -ENOMEM;
> +
> +	regmap_config.val_bits = 8;
> +	regmap_config.reg_bits = 16;
> +	regmap_config.disable_locking = true;
> +	regmap = devm_regmap_init_i2c(client, &regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	nvm->regmap = regmap;
> +
> +	nvmem_config.name = dev_name(dev);
> +	nvmem_config.dev = dev;
> +	nvmem_config.read_only = true;
> +	nvmem_config.root_only = true;
> +	nvmem_config.owner = THIS_MODULE;
> +	nvmem_config.compat = true;
> +	nvmem_config.base_dev = dev;
> +	nvmem_config.reg_read = ov2740_nvmem_read;
> +	nvmem_config.reg_write = NULL;
> +	nvmem_config.priv = nvm;
> +	nvmem_config.stride = 1;
> +	nvmem_config.word_size = 1;
> +	nvmem_config.size = CUSTOMER_USE_OTP_SIZE;
> +
> +	nvm->nvmem = devm_nvmem_register(dev, &nvmem_config);
> +	if (IS_ERR(nvm->nvmem))
> +		return PTR_ERR(nvm->nvmem);

This registers the NVM device, but the underlying structure is not
completely initialized yet. For instance, the ->num_buffer, which
->reg_read() uses as buffer base address, is still NULL at this point.
I'd be more comfortable if we first fully setup/prepare everything and
only then register the device.

> +
> +	nvm->nvm_buffer = devm_kzalloc(dev, CUSTOMER_USE_OTP_SIZE, GFP_KERNEL);
> +	if (!nvm->nvm_buffer)
> +		return -ENOMEM;

If this allocation fails, we return the error, which is handler as
dev_err() only, and keep everting as is. So any attempt of ->req_read()
will effectively do

	memcpy(val, NULL + off, count);

> +	ret = ov2740_load_otp_data(client, nvm);
> +	if (ret)
> +		dev_err(dev, "failed to load OTP data, ret %d\n", ret);
> +
> +	return ret;
> +}

If this step fails, the NVM device is still registered and available
and we still can call ->req_read(). Is this correct? Do we need to
have an "error out" path there and unregister the NVM device?

>  static int ov2740_probe(struct i2c_client *client)
>  {
>  	struct ov2740 *ov2740;
> @@ -964,6 +1105,10 @@ static int ov2740_probe(struct i2c_client *client)
>  		goto probe_error_media_entity_cleanup;
>  	}
>  
> +	ret = ov2740_register_nvmem(client);
> +	if (ret)
> +		dev_err(&client->dev, "register nvmem failed, ret %d\n", ret);
> +

Either this better be a fatal error, or we need to have an error-out
rollback/cleanup in ov2740_register_nvmem().

>  	/*
>  	 * Device is already turned on by i2c-core with ACPI domain PM.
>  	 * Enable runtime PM and turn off the device.

	-ss