Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap

From: jacopo mondi
Date: Thu Jul 19 2018 - 03:48:47 EST

Next message: piaojun: "Re: [V9fs-developer] [PATCH v3] Fix a hard lockup case in the virtio transport"
Previous message: Dmitry Vyukov: "Re: KASAN: stack-out-of-bounds Read in bpf_tcp_close"
In reply to: Wolfram Sang: "Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap"
Next in thread: Wolfram Sang: "Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello Mita-San,
thanks for keep pushing on this SCCB issue

Only one veryt minor nit, please see below..

On Tue, Jul 17, 2018 at 12:47:49AM +0900, Akinobu Mita wrote:
> Convert ov772x register access to use SCCB regmap.
>
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> Cc: Peter Rosin <peda@xxxxxxxxxx>
> Cc: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxxxx>
> Cc: Wolfram Sang <wsa@xxxxxxxxxxxxx>
> Cc: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> Cc: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
> Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
> ---
> drivers/media/i2c/Kconfig | 1 +
> drivers/media/i2c/ov772x.c | 192 +++++++++++++++++++--------------------------
> 2 files changed, 82 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 341452f..b923a51 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -715,6 +715,7 @@ config VIDEO_OV772X
> tristate "OmniVision OV772x sensor support"
> depends on I2C && VIDEO_V4L2
> depends on MEDIA_CAMERA_SUPPORT
> + select REGMAP_SCCB
> ---help---
> This is a Video4Linux2 sensor-level driver for the OmniVision
> OV772x camera.
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 7158c31..0d3ed23 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -21,6 +21,7 @@
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/regmap.h>
> #include <linux/slab.h>
> #include <linux/v4l2-mediabus.h>
> #include <linux/videodev2.h>
> @@ -414,6 +415,7 @@ struct ov772x_priv {
> struct v4l2_subdev subdev;
> struct v4l2_ctrl_handler hdl;
> struct clk *clk;
> + struct regmap *regmap;
> struct ov772x_camera_info *info;
> struct gpio_desc *pwdn_gpio;
> struct gpio_desc *rstb_gpio;
> @@ -549,51 +551,18 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
> return container_of(sd, struct ov772x_priv, subdev);
> }
>
> -static int ov772x_read(struct i2c_client *client, u8 addr)
> -{
> - int ret;
> - u8 val;
> -
> - ret = i2c_master_send(client, &addr, 1);
> - if (ret < 0)
> - return ret;
> - ret = i2c_master_recv(client, &val, 1);
> - if (ret < 0)
> - return ret;
> -
> - return val;
> -}
> -
> -static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
> -{
> - return i2c_smbus_write_byte_data(client, addr, value);
> -}
> -
> -static int ov772x_mask_set(struct i2c_client *client, u8 command, u8 mask,
> - u8 set)
> -{
> - s32 val = ov772x_read(client, command);
> -
> - if (val < 0)
> - return val;
> -
> - val &= ~mask;
> - val |= set & mask;
> -
> - return ov772x_write(client, command, val);
> -}
> -

If I were you I would have kept these functions and wrapped the regmap
operations there. This is not an issue though if you prefer it this
way :)

> -static int ov772x_reset(struct i2c_client *client)
> +static int ov772x_reset(struct ov772x_priv *priv)
> {
> int ret;
>
> - ret = ov772x_write(client, COM7, SCCB_RESET);
> + ret = regmap_write(priv->regmap, COM7, SCCB_RESET);
> if (ret < 0)
> return ret;
>
> usleep_range(1000, 5000);
>
> - return ov772x_mask_set(client, COM2, SOFT_SLEEP_MODE, SOFT_SLEEP_MODE);
> + return regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> + SOFT_SLEEP_MODE);
> }
>
> /*
> @@ -611,8 +580,8 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
> if (priv->streaming == enable)
> goto done;
>
> - ret = ov772x_mask_set(client, COM2, SOFT_SLEEP_MODE,
> - enable ? 0 : SOFT_SLEEP_MODE);
> + ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> + enable ? 0 : SOFT_SLEEP_MODE);
> if (ret)
> goto done;
>
> @@ -657,7 +626,6 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
> const struct ov772x_color_format *cfmt,
> const struct ov772x_win_size *win)
> {
> - struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> unsigned long fin = clk_get_rate(priv->clk);
> unsigned int best_diff;
> unsigned int fsize;
> @@ -723,11 +691,11 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
> }
> }
>
> - ret = ov772x_write(client, COM4, com4 | COM4_RESERVED);
> + ret = regmap_write(priv->regmap, COM4, com4 | COM4_RESERVED);
> if (ret < 0)
> return ret;
>
> - ret = ov772x_write(client, CLKRC, clkrc | CLKRC_RESERVED);
> + ret = regmap_write(priv->regmap, CLKRC, clkrc | CLKRC_RESERVED);
> if (ret < 0)
> return ret;
>
> @@ -788,8 +756,7 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct ov772x_priv *priv = container_of(ctrl->handler,
> struct ov772x_priv, hdl);
> - struct v4l2_subdev *sd = &priv->subdev;
> - struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct regmap *regmap = priv->regmap;
> int ret = 0;
> u8 val;
>
> @@ -808,27 +775,27 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> val = ctrl->val ? VFLIP_IMG : 0x00;
> if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
> val ^= VFLIP_IMG;
> - return ov772x_mask_set(client, COM3, VFLIP_IMG, val);
> + return regmap_update_bits(regmap, COM3, VFLIP_IMG, val);
> case V4L2_CID_HFLIP:
> val = ctrl->val ? HFLIP_IMG : 0x00;
> if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
> val ^= HFLIP_IMG;
> - return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
> + return regmap_update_bits(regmap, COM3, HFLIP_IMG, val);
> case V4L2_CID_BAND_STOP_FILTER:
> if (!ctrl->val) {
> /* Switch the filter off, it is on now */
> - ret = ov772x_mask_set(client, BDBASE, 0xff, 0xff);
> + ret = regmap_update_bits(regmap, BDBASE, 0xff, 0xff);
> if (!ret)
> - ret = ov772x_mask_set(client, COM8,
> - BNDF_ON_OFF, 0);
> + ret = regmap_update_bits(regmap, COM8,
> + BNDF_ON_OFF, 0);
> } else {
> /* Switch the filter on, set AEC low limit */
> val = 256 - ctrl->val;
> - ret = ov772x_mask_set(client, COM8,
> - BNDF_ON_OFF, BNDF_ON_OFF);
> + ret = regmap_update_bits(regmap, COM8,
> + BNDF_ON_OFF, BNDF_ON_OFF);
> if (!ret)
> - ret = ov772x_mask_set(client, BDBASE,
> - 0xff, val);
> + ret = regmap_update_bits(regmap, BDBASE,
> + 0xff, val);
> }
>
> return ret;
> @@ -841,18 +808,19 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> static int ov772x_g_register(struct v4l2_subdev *sd,
> struct v4l2_dbg_register *reg)
> {
> - struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct ov772x_priv *priv = to_ov772x(sd);
> int ret;
> + unsigned int val;

Nit: please keep variables sorted (move 'val' declaration one line
up).

Apart from that, for the ov772x driver:
Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>

Thanks
j

>
> reg->size = 1;
> if (reg->reg > 0xff)
> return -EINVAL;
>
> - ret = ov772x_read(client, reg->reg);
> + ret = regmap_read(priv->regmap, reg->reg, &val);
> if (ret < 0)
> return ret;
>
> - reg->val = (__u64)ret;
> + reg->val = (__u64)val;
>
> return 0;
> }
> @@ -860,13 +828,13 @@ static int ov772x_g_register(struct v4l2_subdev *sd,
> static int ov772x_s_register(struct v4l2_subdev *sd,
> const struct v4l2_dbg_register *reg)
> {
> - struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct ov772x_priv *priv = to_ov772x(sd);
>
> if (reg->reg > 0xff ||
> reg->val > 0xff)
> return -EINVAL;
>
> - return ov772x_write(client, reg->reg, reg->val);
> + return regmap_write(priv->regmap, reg->reg, reg->val);
> }
> #endif
>
> @@ -1004,7 +972,7 @@ static void ov772x_select_params(const struct v4l2_mbus_framefmt *mf,
>
> static int ov772x_edgectrl(struct ov772x_priv *priv)
> {
> - struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> + struct regmap *regmap = priv->regmap;
> int ret;
>
> if (!priv->info)
> @@ -1018,19 +986,19 @@ static int ov772x_edgectrl(struct ov772x_priv *priv)
> * Remove it when manual mode.
> */
>
> - ret = ov772x_mask_set(client, DSPAUTO, EDGE_ACTRL, 0x00);
> + ret = regmap_update_bits(regmap, DSPAUTO, EDGE_ACTRL, 0x00);
> if (ret < 0)
> return ret;
>
> - ret = ov772x_mask_set(client,
> - EDGE_TRSHLD, OV772X_EDGE_THRESHOLD_MASK,
> - priv->info->edgectrl.threshold);
> + ret = regmap_update_bits(regmap, EDGE_TRSHLD,
> + OV772X_EDGE_THRESHOLD_MASK,
> + priv->info->edgectrl.threshold);
> if (ret < 0)
> return ret;
>
> - ret = ov772x_mask_set(client,
> - EDGE_STRNGT, OV772X_EDGE_STRENGTH_MASK,
> - priv->info->edgectrl.strength);
> + ret = regmap_update_bits(regmap, EDGE_STRNGT,
> + OV772X_EDGE_STRENGTH_MASK,
> + priv->info->edgectrl.strength);
> if (ret < 0)
> return ret;
>
> @@ -1040,15 +1008,15 @@ static int ov772x_edgectrl(struct ov772x_priv *priv)
> *
> * Set upper and lower limit.
> */
> - ret = ov772x_mask_set(client,
> - EDGE_UPPER, OV772X_EDGE_UPPER_MASK,
> - priv->info->edgectrl.upper);
> + ret = regmap_update_bits(regmap, EDGE_UPPER,
> + OV772X_EDGE_UPPER_MASK,
> + priv->info->edgectrl.upper);
> if (ret < 0)
> return ret;
>
> - ret = ov772x_mask_set(client,
> - EDGE_LOWER, OV772X_EDGE_LOWER_MASK,
> - priv->info->edgectrl.lower);
> + ret = regmap_update_bits(regmap, EDGE_LOWER,
> + OV772X_EDGE_LOWER_MASK,
> + priv->info->edgectrl.lower);
> if (ret < 0)
> return ret;
> }
> @@ -1060,12 +1028,11 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> const struct ov772x_color_format *cfmt,
> const struct ov772x_win_size *win)
> {
> - struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> int ret;
> u8 val;
>
> /* Reset hardware. */
> - ov772x_reset(client);
> + ov772x_reset(priv);
>
> /* Edge Ctrl. */
> ret = ov772x_edgectrl(priv);
> @@ -1073,32 +1040,32 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> return ret;
>
> /* Format and window size. */
> - ret = ov772x_write(client, HSTART, win->rect.left >> 2);
> + ret = regmap_write(priv->regmap, HSTART, win->rect.left >> 2);
> if (ret < 0)
> goto ov772x_set_fmt_error;
> - ret = ov772x_write(client, HSIZE, win->rect.width >> 2);
> + ret = regmap_write(priv->regmap, HSIZE, win->rect.width >> 2);
> if (ret < 0)
> goto ov772x_set_fmt_error;
> - ret = ov772x_write(client, VSTART, win->rect.top >> 1);
> + ret = regmap_write(priv->regmap, VSTART, win->rect.top >> 1);
> if (ret < 0)
> goto ov772x_set_fmt_error;
> - ret = ov772x_write(client, VSIZE, win->rect.height >> 1);
> + ret = regmap_write(priv->regmap, VSIZE, win->rect.height >> 1);
> if (ret < 0)
> goto ov772x_set_fmt_error;
> - ret = ov772x_write(client, HOUTSIZE, win->rect.width >> 2);
> + ret = regmap_write(priv->regmap, HOUTSIZE, win->rect.width >> 2);
> if (ret < 0)
> goto ov772x_set_fmt_error;
> - ret = ov772x_write(client, VOUTSIZE, win->rect.height >> 1);
> + ret = regmap_write(priv->regmap, VOUTSIZE, win->rect.height >> 1);
> if (ret < 0)
> goto ov772x_set_fmt_error;
> - ret = ov772x_write(client, HREF,
> + ret = regmap_write(priv->regmap, HREF,
> ((win->rect.top & 1) << HREF_VSTART_SHIFT) |
> ((win->rect.left & 3) << HREF_HSTART_SHIFT) |
> ((win->rect.height & 1) << HREF_VSIZE_SHIFT) |
> ((win->rect.width & 3) << HREF_HSIZE_SHIFT));
> if (ret < 0)
> goto ov772x_set_fmt_error;
> - ret = ov772x_write(client, EXHCH,
> + ret = regmap_write(priv->regmap, EXHCH,
> ((win->rect.height & 1) << EXHCH_VSIZE_SHIFT) |
> ((win->rect.width & 3) << EXHCH_HSIZE_SHIFT));
> if (ret < 0)
> @@ -1107,15 +1074,14 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> /* Set DSP_CTRL3. */
> val = cfmt->dsp3;
> if (val) {
> - ret = ov772x_mask_set(client,
> - DSP_CTRL3, UV_MASK, val);
> + ret = regmap_update_bits(priv->regmap, DSP_CTRL3, UV_MASK, val);
> if (ret < 0)
> goto ov772x_set_fmt_error;
> }
>
> /* DSP_CTRL4: AEC reference point and DSP output format. */
> if (cfmt->dsp4) {
> - ret = ov772x_write(client, DSP_CTRL4, cfmt->dsp4);
> + ret = regmap_write(priv->regmap, DSP_CTRL4, cfmt->dsp4);
> if (ret < 0)
> goto ov772x_set_fmt_error;
> }
> @@ -1131,13 +1097,12 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> if (priv->hflip_ctrl->val)
> val ^= HFLIP_IMG;
>
> - ret = ov772x_mask_set(client,
> - COM3, SWAP_MASK | IMG_MASK, val);
> + ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val);
> if (ret < 0)
> goto ov772x_set_fmt_error;
>
> /* COM7: Sensor resolution and output format control. */
> - ret = ov772x_write(client, COM7, win->com7_bit | cfmt->com7);
> + ret = regmap_write(priv->regmap, COM7, win->com7_bit | cfmt->com7);
> if (ret < 0)
> goto ov772x_set_fmt_error;
>
> @@ -1150,10 +1115,11 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> if (priv->band_filter_ctrl->val) {
> unsigned short band_filter = priv->band_filter_ctrl->val;
>
> - ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);
> + ret = regmap_update_bits(priv->regmap, COM8,
> + BNDF_ON_OFF, BNDF_ON_OFF);
> if (!ret)
> - ret = ov772x_mask_set(client, BDBASE,
> - 0xff, 256 - band_filter);
> + ret = regmap_update_bits(priv->regmap, BDBASE,
> + 0xff, 256 - band_filter);
> if (ret < 0)
> goto ov772x_set_fmt_error;
> }
> @@ -1162,7 +1128,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>
> ov772x_set_fmt_error:
>
> - ov772x_reset(client);
> + ov772x_reset(priv);
>
> return ret;
> }
> @@ -1276,12 +1242,12 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
> return ret;
>
> /* Check and show product ID and manufacturer ID. */
> - pid = ov772x_read(client, PID);
> - if (pid < 0)
> - return pid;
> - ver = ov772x_read(client, VER);
> - if (ver < 0)
> - return ver;
> + ret = regmap_read(priv->regmap, PID, &pid);
> + if (ret < 0)
> + return ret;
> + ret = regmap_read(priv->regmap, VER, &ver);
> + if (ret < 0)
> + return ret;
>
> switch (VERSION(pid, ver)) {
> case OV7720:
> @@ -1297,12 +1263,12 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
> goto done;
> }
>
> - midh = ov772x_read(client, MIDH);
> - if (midh < 0)
> - return midh;
> - midl = ov772x_read(client, MIDL);
> - if (midl < 0)
> - return midl;
> + ret = regmap_read(priv->regmap, MIDH, &midh);
> + if (ret < 0)
> + return ret;
> + ret = regmap_read(priv->regmap, MIDL, &midl);
> + if (ret < 0)
> + return ret;
>
> dev_info(&client->dev,
> "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
> @@ -1386,8 +1352,12 @@ static int ov772x_probe(struct i2c_client *client,
> const struct i2c_device_id *did)
> {
> struct ov772x_priv *priv;
> - struct i2c_adapter *adapter = client->adapter;
> int ret;
> + static const struct regmap_config ov772x_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = DSPAUTO,
> + };
>
> if (!client->dev.of_node && !client->dev.platform_data) {
> dev_err(&client->dev,
> @@ -1395,16 +1365,16 @@ static int ov772x_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> - if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> - dev_err(&adapter->dev,
> - "I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
> - return -EIO;
> - }
> -
> priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
>
> + priv->regmap = devm_regmap_init_sccb(client, &ov772x_regmap_config);
> + if (IS_ERR(priv->regmap)) {
> + dev_err(&client->dev, "Failed to allocate register map\n");
> + return PTR_ERR(priv->regmap);
> + }
> +
> priv->info = client->dev.platform_data;
> mutex_init(&priv->lock);
>
> --
> 2.7.4
>

Attachment: signature.asc
Description: PGP signature

Next message: piaojun: "Re: [V9fs-developer] [PATCH v3] Fix a hard lockup case in the virtio transport"
Previous message: Dmitry Vyukov: "Re: KASAN: stack-out-of-bounds Read in bpf_tcp_close"
In reply to: Wolfram Sang: "Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap"
Next in thread: Wolfram Sang: "Re: [PATCH -next v4 2/3] media: ov772x: use SCCB regmap"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]