| Message ID | 20220726120556.2881-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 39cc0f20d1bc2bfd16aa8a05db84755d04d25b3c |
| Headers | show |
| Series | [v1,1/8] media: ov2740: Remove duplicative pointer in struct nvm_data | expand |
Hi, Andy
Thanks for your patch.
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote: > The struct i2c_client pointer is used only to get driver data, > associated with a struct device or print messages on behalf. > Moreover, the very same pointer to a struct device is already > assigned by a regmap and can be retrieved from there. > No need to keep a duplicative pointer. Thanks, Bungbu, for the review. Can it be now applied?
On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote: > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote: > > The struct i2c_client pointer is used only to get driver data, > > associated with a struct device or print messages on behalf. > > Moreover, the very same pointer to a struct device is already > > assigned by a regmap and can be retrieved from there. > > No need to keep a duplicative pointer. > > Thanks, Bungbu, for the review. Can it be now applied? Don't see this being applied or commented why not... Mauro? Or who is taking care of this driver nowadays?
On Fri, Nov 11, 2022 at 02:05:37PM +0200, Andy Shevchenko wrote: > On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote: > > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote: > > > The struct i2c_client pointer is used only to get driver data, > > > associated with a struct device or print messages on behalf. > > > Moreover, the very same pointer to a struct device is already > > > assigned by a regmap and can be retrieved from there. > > > No need to keep a duplicative pointer. > > > > Thanks, Bungbu, for the review. Can it be now applied? > > Don't see this being applied or commented why not... > > Mauro? Or who is taking care of this driver nowadays? Okay, found a private response by Mauro where he tells that Sakari can take care of this. Sakari, should I resend this to you with all tags applied? Or you can use `b4` tool that allows to avoid unneeded resend.
On Fri, Nov 11, 2022 at 02:08:48PM +0200, Andy Shevchenko wrote: > On Fri, Nov 11, 2022 at 02:05:37PM +0200, Andy Shevchenko wrote: > > On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote: > > > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote: > > > > The struct i2c_client pointer is used only to get driver data, > > > > associated with a struct device or print messages on behalf. > > > > Moreover, the very same pointer to a struct device is already > > > > assigned by a regmap and can be retrieved from there. > > > > No need to keep a duplicative pointer. > > > > > > Thanks, Bungbu, for the review. Can it be now applied? > > > > Don't see this being applied or commented why not... > > > > Mauro? Or who is taking care of this driver nowadays? > > Okay, found a private response by Mauro where he tells that Sakari can take > care of this. Sakari, should I resend this to you with all tags applied? > Or you can use `b4` tool that allows to avoid unneeded resend. No need to. But please cc me on the next time. I'll take a look now...
On Fri, Nov 11, 2022 at 02:58:45PM +0000, Sakari Ailus wrote: > On Fri, Nov 11, 2022 at 02:08:48PM +0200, Andy Shevchenko wrote: > > On Fri, Nov 11, 2022 at 02:05:37PM +0200, Andy Shevchenko wrote: > > > On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote: > > > > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote: > > > > > The struct i2c_client pointer is used only to get driver data, > > > > > associated with a struct device or print messages on behalf. > > > > > Moreover, the very same pointer to a struct device is already > > > > > assigned by a regmap and can be retrieved from there. > > > > > No need to keep a duplicative pointer. > > > > > > > > Thanks, Bungbu, for the review. Can it be now applied? > > > > > > Don't see this being applied or commented why not... > > > > > > Mauro? Or who is taking care of this driver nowadays? > > > > Okay, found a private response by Mauro where he tells that Sakari can take > > care of this. Sakari, should I resend this to you with all tags applied? > > Or you can use `b4` tool that allows to avoid unneeded resend. > > No need to. But please cc me on the next time. I'll take a look now... How should I know whom to Cc? Can we update MAINTAINERS accordingly, please?
Hi Andy, On Fri, Nov 11, 2022 at 05:29:16PM +0200, Andy Shevchenko wrote: > On Fri, Nov 11, 2022 at 02:58:45PM +0000, Sakari Ailus wrote: > > On Fri, Nov 11, 2022 at 02:08:48PM +0200, Andy Shevchenko wrote: > > > On Fri, Nov 11, 2022 at 02:05:37PM +0200, Andy Shevchenko wrote: > > > > On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote: > > > > > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote: > > > > > > The struct i2c_client pointer is used only to get driver data, > > > > > > associated with a struct device or print messages on behalf. > > > > > > Moreover, the very same pointer to a struct device is already > > > > > > assigned by a regmap and can be retrieved from there. > > > > > > No need to keep a duplicative pointer. > > > > > > > > > > Thanks, Bungbu, for the review. Can it be now applied? > > > > > > > > Don't see this being applied or commented why not... > > > > > > > > Mauro? Or who is taking care of this driver nowadays? > > > > > > Okay, found a private response by Mauro where he tells that Sakari can take > > > care of this. Sakari, should I resend this to you with all tags applied? > > > Or you can use `b4` tool that allows to avoid unneeded resend. > > > > No need to. But please cc me on the next time. I'll take a look now... > > How should I know whom to Cc? Can we update MAINTAINERS accordingly, please? Good question. In media tree we've listed the maintainers in wiki, as the information would be hard to keep up-to-date file-wise: <URL:https://www.linuxtv.org/wiki/index.php/Media_Maintainers> So it helps if you cc me to camera sensor driver patches, but they're neither ignored if you don't. It usually takes a little bit more time but not nearly as much as this time. Cc Hans.
On Fri, Nov 11, 2022 at 07:41:28PM +0000, Sakari Ailus wrote: > On Fri, Nov 11, 2022 at 05:29:16PM +0200, Andy Shevchenko wrote: > > On Fri, Nov 11, 2022 at 02:58:45PM +0000, Sakari Ailus wrote: > > > On Fri, Nov 11, 2022 at 02:08:48PM +0200, Andy Shevchenko wrote: > > > > On Fri, Nov 11, 2022 at 02:05:37PM +0200, Andy Shevchenko wrote: > > > > > On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote: > > > > > > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote: > > > > > > > The struct i2c_client pointer is used only to get driver data, > > > > > > > associated with a struct device or print messages on behalf. > > > > > > > Moreover, the very same pointer to a struct device is already > > > > > > > assigned by a regmap and can be retrieved from there. > > > > > > > No need to keep a duplicative pointer. > > > > > > > > > > > > Thanks, Bungbu, for the review. Can it be now applied? > > > > > > > > > > Don't see this being applied or commented why not... > > > > > > > > > > Mauro? Or who is taking care of this driver nowadays? > > > > > > > > Okay, found a private response by Mauro where he tells that Sakari can take > > > > care of this. Sakari, should I resend this to you with all tags applied? > > > > Or you can use `b4` tool that allows to avoid unneeded resend. > > > > > > No need to. But please cc me on the next time. I'll take a look now... > > > > How should I know whom to Cc? Can we update MAINTAINERS accordingly, please? > > Good question. In media tree we've listed the maintainers in wiki, as > the information would be hard to keep up-to-date file-wise: > > <URL:https://www.linuxtv.org/wiki/index.php/Media_Maintainers> Unfortunately get_maintainer.pl doesn't know about this. > So it helps if you cc me to camera sensor driver patches, but they're > neither ignored if you don't. It usually takes a little bit more time > but not nearly as much as this time. > > Cc Hans.
diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index d5f0eabf20c6..c975db1bbe8c 100644 --- a/drivers/media/i2c/ov2740.c +++ b/drivers/media/i2c/ov2740.c @@ -77,7 +77,6 @@ #define OV2740_REG_OTP_CUSTOMER 0x7010 struct nvm_data { - struct i2c_client *client; struct nvmem_device *nvmem; struct regmap *regmap; char *nvm_buffer; @@ -649,34 +648,28 @@ static void ov2740_update_pad_format(const struct ov2740_mode *mode, static int ov2740_load_otp_data(struct nvm_data *nvm) { - struct i2c_client *client; - struct ov2740 *ov2740; + struct device *dev = regmap_get_device(nvm->regmap); + struct ov2740 *ov2740 = to_ov2740(dev_get_drvdata(dev)); u32 isp_ctrl00 = 0; u32 isp_ctrl01 = 0; int ret; - if (!nvm) - return -EINVAL; - if (nvm->nvm_buffer) return 0; - client = nvm->client; - ov2740 = to_ov2740(i2c_get_clientdata(client)); - nvm->nvm_buffer = kzalloc(CUSTOMER_USE_OTP_SIZE, GFP_KERNEL); if (!nvm->nvm_buffer) return -ENOMEM; ret = ov2740_read_reg(ov2740, OV2740_REG_ISP_CTRL00, 1, &isp_ctrl00); if (ret) { - dev_err(&client->dev, "failed to read ISP CTRL00\n"); + dev_err(dev, "failed to read ISP CTRL00\n"); goto err; } ret = ov2740_read_reg(ov2740, OV2740_REG_ISP_CTRL01, 1, &isp_ctrl01); if (ret) { - dev_err(&client->dev, "failed to read ISP CTRL01\n"); + dev_err(dev, "failed to read ISP CTRL01\n"); goto err; } @@ -684,7 +677,7 @@ static int ov2740_load_otp_data(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 set ISP CTRL00\n"); + dev_err(dev, "failed to set ISP CTRL00\n"); goto err; } @@ -692,14 +685,14 @@ static int ov2740_load_otp_data(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 set ISP CTRL01\n"); + dev_err(dev, "failed to set ISP CTRL01\n"); goto err; } ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1, OV2740_MODE_STREAMING); if (ret) { - dev_err(&client->dev, "failed to set streaming mode\n"); + dev_err(dev, "failed to set streaming mode\n"); goto err; } @@ -712,26 +705,26 @@ static int ov2740_load_otp_data(struct nvm_data *nvm) 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); + dev_err(dev, "failed to read OTP data, ret %d\n", ret); goto err; } 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"); + dev_err(dev, "failed to set streaming mode\n"); goto err; } ret = ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL01, 1, isp_ctrl01); if (ret) { - dev_err(&client->dev, "failed to set ISP CTRL01\n"); + dev_err(dev, "failed to set ISP CTRL01\n"); goto err; } ret = ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL00, 1, isp_ctrl00); if (ret) { - dev_err(&client->dev, "failed to set ISP CTRL00\n"); + dev_err(dev, "failed to set ISP CTRL00\n"); goto err; } @@ -746,7 +739,6 @@ static int ov2740_load_otp_data(struct nvm_data *nvm) static int ov2740_start_streaming(struct ov2740 *ov2740) { struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd); - struct nvm_data *nvm = ov2740->nvm; const struct ov2740_reg_list *reg_list; int link_freq_index; int ret = 0; @@ -755,7 +747,8 @@ static int ov2740_start_streaming(struct ov2740 *ov2740) if (ret) return ret; - ov2740_load_otp_data(nvm); + if (ov2740->nvm) + ov2740_load_otp_data(ov2740->nvm); link_freq_index = ov2740->cur_mode->link_freq_index; reg_list = &link_freq_configs[link_freq_index].reg_list; @@ -1071,9 +1064,8 @@ 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); + struct device *dev = regmap_get_device(nvm->regmap); + struct ov2740 *ov2740 = to_ov2740(dev_get_drvdata(dev)); int ret = 0; mutex_lock(&ov2740->mutex); @@ -1120,7 +1112,6 @@ static int ov2740_register_nvmem(struct i2c_client *client, return PTR_ERR(regmap); nvm->regmap = regmap; - nvm->client = client; nvmem_config.name = dev_name(dev); nvmem_config.dev = dev;
The struct i2c_client pointer is used only to get driver data, associated with a struct device or print messages on behalf. Moreover, the very same pointer to a struct device is already assigned by a regmap and can be retrieved from there. No need to keep a duplicative pointer. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/media/i2c/ov2740.c | 39 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 24 deletions(-)