| Message ID | 1591954922-14006-1-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov2740: add NVMEM interface to read customized OTP data | expand |
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, ®map_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.
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.
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.
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, ®map_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
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, ®map_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.