| Message ID | 20200616141244.49407-4-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | v4l2-subdev: Introduce [g|s]et_mbus_format pad op | expand |
Hi Janusz, On Tue, Jun 16, 2020 at 04:12:38PM +0200, Jacopo Mondi wrote: > Use the new get_mbus_config and set_mbus_config pad operations in place > of the video operations currently in use. > > Compared to other drivers where the same conversion has been performed, > ov6650 proved to be a bit more tricky, as the existing g_mbus_config > implementation did not report the currently applied configuration but > the set of all possible configuration options. > > Adapt the driver to support the semantic of the two newly introduced > operations: > - get_mbus_config reports the current media bus configuration > - set_mbus_config applies only changes explicitly requested and updates > the provided cfg parameter to report what has actually been applied to > the hardware. > > Compile-tested only. As the original author of this driver, are you still looking after it ? Do you have any opinion on this patch/series? I should have cc-ed you from the beginning probably, but get_maintainer didn't point to you :) Thanks j > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/ov6650.c | 56 ++++++++++++++++++++++++++------------ > 1 file changed, 39 insertions(+), 17 deletions(-) > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c > index 91906b94f978..d2e7a8556ed7 100644 > --- a/drivers/media/i2c/ov6650.c > +++ b/drivers/media/i2c/ov6650.c > @@ -921,46 +921,68 @@ static const struct v4l2_subdev_core_ops ov6650_core_ops = { > }; > > /* Request bus settings on camera side */ > -static int ov6650_g_mbus_config(struct v4l2_subdev *sd, > - struct v4l2_mbus_config *cfg) > +static int ov6650_get_mbus_config(struct v4l2_subdev *sd, > + unsigned int pad, > + struct v4l2_mbus_config *cfg) > { > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + u8 comj, comf; > + int ret; > + > + ret = ov6650_reg_read(client, REG_COMJ, &comj); > + if (ret) > + return ret; > > - cfg->flags = V4L2_MBUS_MASTER | > - V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_PCLK_SAMPLE_FALLING | > - V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_LOW | > - V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_LOW | > - V4L2_MBUS_DATA_ACTIVE_HIGH; > + ret = ov6650_reg_read(client, REG_COMF, &comf); > + if (ret) > + return ret; > + > + cfg->flags = V4L2_MBUS_MASTER > + | ((comj & COMJ_VSYNC_HIGH) ? V4L2_MBUS_VSYNC_ACTIVE_HIGH > + : V4L2_MBUS_VSYNC_ACTIVE_LOW) > + | ((comf & COMF_HREF_LOW) ? V4L2_MBUS_HSYNC_ACTIVE_LOW > + : V4L2_MBUS_HSYNC_ACTIVE_HIGH) > + | ((comj & COMJ_PCLK_RISING) ? V4L2_MBUS_PCLK_SAMPLE_RISING > + : V4L2_MBUS_PCLK_SAMPLE_FALLING); > cfg->type = V4L2_MBUS_PARALLEL; > > return 0; > } > > /* Alter bus settings on camera side */ > -static int ov6650_s_mbus_config(struct v4l2_subdev *sd, > - const struct v4l2_mbus_config *cfg) > +static int ov6650_set_mbus_config(struct v4l2_subdev *sd, > + unsigned int pad, > + struct v4l2_mbus_config *cfg) > { > struct i2c_client *client = v4l2_get_subdevdata(sd); > - int ret; > + int ret = 0; > > if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING) > ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0); > - else > + else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING) > ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING); > if (ret) > - return ret; > + goto error; > > if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW) > ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0); > - else > + else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) > ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW); > if (ret) > - return ret; > + goto error; > > if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) > ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0); > - else > + else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) > ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH); > > +error: > + /* > + * Update the configuration to report what is actually applied to > + * the hardware. > + */ > + ov6650_get_mbus_config(sd, pad, cfg); > + > return ret; > } > > @@ -968,8 +990,6 @@ static const struct v4l2_subdev_video_ops ov6650_video_ops = { > .s_stream = ov6650_s_stream, > .g_frame_interval = ov6650_g_frame_interval, > .s_frame_interval = ov6650_s_frame_interval, > - .g_mbus_config = ov6650_g_mbus_config, > - .s_mbus_config = ov6650_s_mbus_config, > }; > > static const struct v4l2_subdev_pad_ops ov6650_pad_ops = { > @@ -978,6 +998,8 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = { > .set_selection = ov6650_set_selection, > .get_fmt = ov6650_get_fmt, > .set_fmt = ov6650_set_fmt, > + .get_mbus_config = ov6650_get_mbus_config, > + .set_mbus_config = ov6650_set_mbus_config, > }; > > static const struct v4l2_subdev_ops ov6650_subdev_ops = { > -- > 2.27.0 >
Hi Jacopo, Thanks for bringing my attention to this patch. On Tuesday, June 16, 2020 4:12:38 P.M. CEST Jacopo Mondi wrote: > Use the new get_mbus_config and set_mbus_config pad operations in place > of the video operations currently in use. > > Compared to other drivers where the same conversion has been performed, > ov6650 proved to be a bit more tricky, as the existing g_mbus_config > implementation did not report the currently applied configuration but > the set of all possible configuration options. Assuming that was in line with officially supported semantics of the old API, not a misinterpretation, I would really like to see that limitation of the new API actually compensated with V4L2_SUBDEV_FORMAT_TRY support added to it. > > Adapt the driver to support the semantic of the two newly introducedV4L2_SUBDEV_FORMAT_TRY > operations: > - get_mbus_config reports the current media bus configuration > - set_mbus_config applies only changes explicitly requested and updates > the provided cfg parameter to report what has actually been applied to > the hardware. > > Compile-tested only. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/ov6650.c | 56 ++++++++++++++++++++++++++------------ > 1 file changed, 39 insertions(+), 17 deletions(-) > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c > index 91906b94f978..d2e7a8556ed7 100644 > --- a/drivers/media/i2c/ov6650.c > +++ b/drivers/media/i2c/ov6650.c > @@ -921,46 +921,68 @@ static const struct v4l2_subdev_core_ops ov6650_core_ops = { > }; > > /* Request bus settings on camera side */ > -static int ov6650_g_mbus_config(struct v4l2_subdev *sd, > - struct v4l2_mbus_config *cfg) > +static int ov6650_get_mbus_config(struct v4l2_subdev *sd, > + unsigned int pad, > + struct v4l2_mbus_config *cfg) > { > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + u8 comj, comf; > + int ret; > + > + ret = ov6650_reg_read(client, REG_COMJ, &comj); > + if (ret) > + return ret; > > - cfg->flags = V4L2_MBUS_MASTER | > - V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_PCLK_SAMPLE_FALLING | > - V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_LOW | > - V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_LOW | > - V4L2_MBUS_DATA_ACTIVE_HIGH; > + ret = ov6650_reg_read(client, REG_COMF, &comf); > + if (ret) > + return ret; > + > + cfg->flags = V4L2_MBUS_MASTER > + | ((comj & COMJ_VSYNC_HIGH) ? V4L2_MBUS_VSYNC_ACTIVE_HIGH > + : V4L2_MBUS_VSYNC_ACTIVE_LOW) > + | ((comf & COMF_HREF_LOW) ? V4L2_MBUS_HSYNC_ACTIVE_LOW > + : V4L2_MBUS_HSYNC_ACTIVE_HIGH) > + | ((comj & COMJ_PCLK_RISING) ? V4L2_MBUS_PCLK_SAMPLE_RISING > + : V4L2_MBUS_PCLK_SAMPLE_FALLING); You probably missed hardware default V4L2_MBUS_DATA_ACTIVE_HIGH. > cfg->type = V4L2_MBUS_PARALLEL; > > return 0; > } > > /* Alter bus settings on camera side */ > -static int ov6650_s_mbus_config(struct v4l2_subdev *sd, > - const struct v4l2_mbus_config *cfg) > +static int ov6650_set_mbus_config(struct v4l2_subdev *sd, > + unsigned int pad, > + struct v4l2_mbus_config *cfg) > { > struct i2c_client *client = v4l2_get_subdevdata(sd); > - int ret; > + int ret = 0; > > if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING) > ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0); > - else > + else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING) Have you thought of extending v4l2_subdev_call_pad_wrappers with a check for only one of mutually exclusive flags specified by user? > ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING); > if (ret) > - return ret; > + goto error; > > if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW) > ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0); > - else > + else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) > ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW); > if (ret) > - return ret; > + goto error; > > if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) > ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0); > - else > + else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) > ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH); > > +error: > + /* > + * Update the configuration to report what is actually applied to > + * the hardware. > + */ > + ov6650_get_mbus_config(sd, pad, cfg); Populating cfg->flags by ov6650_get_mbus_config() without checking for a potential error it may return can result in invalid data silently returned to user. Maybe it would be better to fetch current hardware status first, fail on error, then update the result with successfully performed hardware state modifications. Thanks, Janusz > + > return ret; > } > > @@ -968,8 +990,6 @@ static const struct v4l2_subdev_video_ops ov6650_video_ops = { > .s_stream = ov6650_s_stream, > .g_frame_interval = ov6650_g_frame_interval, > .s_frame_interval = ov6650_s_frame_interval, > - .g_mbus_config = ov6650_g_mbus_config, > - .s_mbus_config = ov6650_s_mbus_config, > }; > > static const struct v4l2_subdev_pad_ops ov6650_pad_ops = { > @@ -978,6 +998,8 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = { > .set_selection = ov6650_set_selection, > .get_fmt = ov6650_get_fmt, > .set_fmt = ov6650_set_fmt, > + .get_mbus_config = ov6650_get_mbus_config, > + .set_mbus_config = ov6650_set_mbus_config, > }; > > static const struct v4l2_subdev_ops ov6650_subdev_ops = { >
Hello Janusz, thanks for your quick reply On Sun, Jun 21, 2020 at 01:38:46PM +0200, Janusz Krzysztofik wrote: > Hi Jacopo, > > Thanks for bringing my attention to this patch. > > On Tuesday, June 16, 2020 4:12:38 P.M. CEST Jacopo Mondi wrote: > > Use the new get_mbus_config and set_mbus_config pad operations in place > > of the video operations currently in use. > > > > Compared to other drivers where the same conversion has been performed, > > ov6650 proved to be a bit more tricky, as the existing g_mbus_config > > implementation did not report the currently applied configuration but > > the set of all possible configuration options. > > Assuming that was in line with officially supported semantics of the old API, > not a misinterpretation, I would really like to see that limitation of the new > API actually compensated with V4L2_SUBDEV_FORMAT_TRY support added to it. > I'm not sure this is a limitation, it's more by design that the new get_mbus_config() only reports the current configuration. To be honest, compared to the other users of the old g_mbus_config() this driver was the only one implementing the operation in this way, maybe as it's the sole user of s_mbus_config() left out of staging ? I would however consider supporting FORMAT_TRY even if I'm not actually sure if fully makes sense. For the format operations (get/set_format()) FORMAT_TRY is used for concurrent applications to test a format without stepping on each other toes. get|set_mbus_config() are kAPI only, and I'm not sure we need to stay safe against concurrent configuration attempts... I'll think about this a bit more. Seems a development that could go on top, right ? > > > > Adapt the driver to support the semantic of the two newly introducedV4L2_SUBDEV_FORMAT_TRY > > operations: > > - get_mbus_config reports the current media bus configuration > > - set_mbus_config applies only changes explicitly requested and updates > > the provided cfg parameter to report what has actually been applied to > > the hardware. > > > > Compile-tested only. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/ov6650.c | 56 ++++++++++++++++++++++++++------------ > > 1 file changed, 39 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c > > index 91906b94f978..d2e7a8556ed7 100644 > > --- a/drivers/media/i2c/ov6650.c > > +++ b/drivers/media/i2c/ov6650.c > > @@ -921,46 +921,68 @@ static const struct v4l2_subdev_core_ops ov6650_core_ops = { > > }; > > > > /* Request bus settings on camera side */ > > -static int ov6650_g_mbus_config(struct v4l2_subdev *sd, > > - struct v4l2_mbus_config *cfg) > > +static int ov6650_get_mbus_config(struct v4l2_subdev *sd, > > + unsigned int pad, > > + struct v4l2_mbus_config *cfg) > > { > > + struct i2c_client *client = v4l2_get_subdevdata(sd); > > + u8 comj, comf; > > + int ret; > > + > > + ret = ov6650_reg_read(client, REG_COMJ, &comj); > > + if (ret) > > + return ret; > > > > - cfg->flags = V4L2_MBUS_MASTER | > > - V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_PCLK_SAMPLE_FALLING | > > - V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_LOW | > > - V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_LOW | > > - V4L2_MBUS_DATA_ACTIVE_HIGH; > > + ret = ov6650_reg_read(client, REG_COMF, &comf); > > + if (ret) > > + return ret; > > + > > + cfg->flags = V4L2_MBUS_MASTER > > + | ((comj & COMJ_VSYNC_HIGH) ? V4L2_MBUS_VSYNC_ACTIVE_HIGH > > + : V4L2_MBUS_VSYNC_ACTIVE_LOW) > > + | ((comf & COMF_HREF_LOW) ? V4L2_MBUS_HSYNC_ACTIVE_LOW > > + : V4L2_MBUS_HSYNC_ACTIVE_HIGH) > > + | ((comj & COMJ_PCLK_RISING) ? V4L2_MBUS_PCLK_SAMPLE_RISING > > + : V4L2_MBUS_PCLK_SAMPLE_FALLING); > > You probably missed hardware default V4L2_MBUS_DATA_ACTIVE_HIGH. > Indeed I did :/ Thanks for spotting > > cfg->type = V4L2_MBUS_PARALLEL; > > > > return 0; > > } > > > > /* Alter bus settings on camera side */ > > -static int ov6650_s_mbus_config(struct v4l2_subdev *sd, > > - const struct v4l2_mbus_config *cfg) > > +static int ov6650_set_mbus_config(struct v4l2_subdev *sd, > > + unsigned int pad, > > + struct v4l2_mbus_config *cfg) > > { > > struct i2c_client *client = v4l2_get_subdevdata(sd); > > - int ret; > > + int ret = 0; > > > > if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING) > > ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0); > > - else > > + else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING) > > Have you thought of extending v4l2_subdev_call_pad_wrappers with a check for > only one of mutually exclusive flags specified by user? > Good question, but I wonder if this shouldn't be an accepted behaviour. The caller can provide all settings it want to allow the callee to chose which one to apply. The operation returns what has been actually applied by the callee, so that the caller can adjust itself to what the callee chose. Alternatively, it's up to the caller to specify its preference without mutually exclusive options, and the callee tries to adjust to what has been requested. Also in this case the operation returns what has actually been applied, so the caller can later adjust if it could. Seems like a small difference, but it might be good to exapnd the operations description to describe this to avoid each single implementer going in slightly different directions ? > > ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING); > > if (ret) > > - return ret; > > + goto error; > > > > if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW) > > ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0); > > - else > > + else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) > > ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW); > > if (ret) > > - return ret; > > + goto error; > > > > if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) > > ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0); > > - else > > + else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) > > ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH); > > > > +error: > > + /* > > + * Update the configuration to report what is actually applied to > > + * the hardware. > > + */ > > + ov6650_get_mbus_config(sd, pad, cfg); > > Populating cfg->flags by ov6650_get_mbus_config() without checking for a > potential error it may return can result in invalid data silently returned to > user. Maybe it would be better to fetch current hardware status first, fail on > error, then update the result with successfully performed hardware state > modifications. I'm not sure I got what you mean 8) Would if be enough to check for the return value of ov6650_get_mbus_config() (or actually returning it directly at the end of this function). Thanks j > > Thanks, > Janusz > > > + > > return ret; > > } > > > > @@ -968,8 +990,6 @@ static const struct v4l2_subdev_video_ops ov6650_video_ops = { > > .s_stream = ov6650_s_stream, > > .g_frame_interval = ov6650_g_frame_interval, > > .s_frame_interval = ov6650_s_frame_interval, > > - .g_mbus_config = ov6650_g_mbus_config, > > - .s_mbus_config = ov6650_s_mbus_config, > > }; > > > > static const struct v4l2_subdev_pad_ops ov6650_pad_ops = { > > @@ -978,6 +998,8 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = { > > .set_selection = ov6650_set_selection, > > .get_fmt = ov6650_get_fmt, > > .set_fmt = ov6650_set_fmt, > > + .get_mbus_config = ov6650_get_mbus_config, > > + .set_mbus_config = ov6650_set_mbus_config, > > }; > > > > static const struct v4l2_subdev_ops ov6650_subdev_ops = { > > > > > >
On 24/06/2020 10:11, Jacopo Mondi wrote: > Hello Janusz, > thanks for your quick reply > > On Sun, Jun 21, 2020 at 01:38:46PM +0200, Janusz Krzysztofik wrote: >> Hi Jacopo, >> >> Thanks for bringing my attention to this patch. >> >> On Tuesday, June 16, 2020 4:12:38 P.M. CEST Jacopo Mondi wrote: >>> Use the new get_mbus_config and set_mbus_config pad operations in place >>> of the video operations currently in use. >>> >>> Compared to other drivers where the same conversion has been performed, >>> ov6650 proved to be a bit more tricky, as the existing g_mbus_config >>> implementation did not report the currently applied configuration but >>> the set of all possible configuration options. >> >> Assuming that was in line with officially supported semantics of the old API, >> not a misinterpretation, I would really like to see that limitation of the new >> API actually compensated with V4L2_SUBDEV_FORMAT_TRY support added to it. >> > > I'm not sure this is a limitation, it's more by design that the new > get_mbus_config() only reports the current configuration. Right. The old behavior was a left-over from the soc-camera driver that required that. But soc-camera is from pre-device-tree times, and today this information should come from the device tree. > > To be honest, compared to the other users of the old g_mbus_config() > this driver was the only one implementing the operation in this way, > maybe as it's the sole user of s_mbus_config() left out of staging ? > > I would however consider supporting FORMAT_TRY even if I'm not > actually sure if fully makes sense. For the format operations > (get/set_format()) FORMAT_TRY is used for concurrent applications to > test a format without stepping on each other toes. > get|set_mbus_config() are kAPI only, and I'm not sure we need to stay > safe against concurrent configuration attempts... I'll think about > this a bit more. Seems a development that could go on top, right ? I wouldn't do this unless it turns out to be actually needed. Regards, Hans > >>> >>> Adapt the driver to support the semantic of the two newly introducedV4L2_SUBDEV_FORMAT_TRY >>> operations: >>> - get_mbus_config reports the current media bus configuration >>> - set_mbus_config applies only changes explicitly requested and updates >>> the provided cfg parameter to report what has actually been applied to >>> the hardware. >>> >>> Compile-tested only. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> --- >>> drivers/media/i2c/ov6650.c | 56 ++++++++++++++++++++++++++------------ >>> 1 file changed, 39 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c >>> index 91906b94f978..d2e7a8556ed7 100644 >>> --- a/drivers/media/i2c/ov6650.c >>> +++ b/drivers/media/i2c/ov6650.c >>> @@ -921,46 +921,68 @@ static const struct v4l2_subdev_core_ops ov6650_core_ops = { >>> }; >>> >>> /* Request bus settings on camera side */ >>> -static int ov6650_g_mbus_config(struct v4l2_subdev *sd, >>> - struct v4l2_mbus_config *cfg) >>> +static int ov6650_get_mbus_config(struct v4l2_subdev *sd, >>> + unsigned int pad, >>> + struct v4l2_mbus_config *cfg) >>> { >>> + struct i2c_client *client = v4l2_get_subdevdata(sd); >>> + u8 comj, comf; >>> + int ret; >>> + >>> + ret = ov6650_reg_read(client, REG_COMJ, &comj); >>> + if (ret) >>> + return ret; >>> >>> - cfg->flags = V4L2_MBUS_MASTER | >>> - V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_PCLK_SAMPLE_FALLING | >>> - V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_LOW | >>> - V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_LOW | >>> - V4L2_MBUS_DATA_ACTIVE_HIGH; >>> + ret = ov6650_reg_read(client, REG_COMF, &comf); >>> + if (ret) >>> + return ret; >>> + >>> + cfg->flags = V4L2_MBUS_MASTER >>> + | ((comj & COMJ_VSYNC_HIGH) ? V4L2_MBUS_VSYNC_ACTIVE_HIGH >>> + : V4L2_MBUS_VSYNC_ACTIVE_LOW) >>> + | ((comf & COMF_HREF_LOW) ? V4L2_MBUS_HSYNC_ACTIVE_LOW >>> + : V4L2_MBUS_HSYNC_ACTIVE_HIGH) >>> + | ((comj & COMJ_PCLK_RISING) ? V4L2_MBUS_PCLK_SAMPLE_RISING >>> + : V4L2_MBUS_PCLK_SAMPLE_FALLING); >> >> You probably missed hardware default V4L2_MBUS_DATA_ACTIVE_HIGH. >> > > Indeed I did :/ > > Thanks for spotting > >>> cfg->type = V4L2_MBUS_PARALLEL; >>> >>> return 0; >>> } >>> >>> /* Alter bus settings on camera side */ >>> -static int ov6650_s_mbus_config(struct v4l2_subdev *sd, >>> - const struct v4l2_mbus_config *cfg) >>> +static int ov6650_set_mbus_config(struct v4l2_subdev *sd, >>> + unsigned int pad, >>> + struct v4l2_mbus_config *cfg) >>> { >>> struct i2c_client *client = v4l2_get_subdevdata(sd); >>> - int ret; >>> + int ret = 0; >>> >>> if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING) >>> ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0); >>> - else >>> + else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING) >> >> Have you thought of extending v4l2_subdev_call_pad_wrappers with a check for >> only one of mutually exclusive flags specified by user? >> > > Good question, but I wonder if this shouldn't be an accepted > behaviour. The caller can provide all settings it want to allow the > callee to chose which one to apply. The operation returns what has > been actually applied by the callee, so that the caller can adjust > itself to what the callee chose. > > Alternatively, it's up to the caller to specify its preference without > mutually exclusive options, and the callee tries to adjust to what has > been requested. Also in this case the operation returns what has > actually been applied, so the caller can later adjust if it could. > > Seems like a small difference, but it might be good to exapnd the > operations description to describe this to avoid each single > implementer going in slightly different directions ? > >>> ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING); >>> if (ret) >>> - return ret; >>> + goto error; >>> >>> if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW) >>> ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0); >>> - else >>> + else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) >>> ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW); >>> if (ret) >>> - return ret; >>> + goto error; >>> >>> if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) >>> ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0); >>> - else >>> + else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) >>> ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH); >>> >>> +error: >>> + /* >>> + * Update the configuration to report what is actually applied to >>> + * the hardware. >>> + */ >>> + ov6650_get_mbus_config(sd, pad, cfg); >> >> Populating cfg->flags by ov6650_get_mbus_config() without checking for a >> potential error it may return can result in invalid data silently returned to >> user. Maybe it would be better to fetch current hardware status first, fail on >> error, then update the result with successfully performed hardware state >> modifications. > > I'm not sure I got what you mean 8) > > Would if be enough to check for the return value of > ov6650_get_mbus_config() (or actually returning it directly at the end > of this function). > > Thanks > j > >> >> Thanks, >> Janusz >> >>> + >>> return ret; >>> } >>> >>> @@ -968,8 +990,6 @@ static const struct v4l2_subdev_video_ops ov6650_video_ops = { >>> .s_stream = ov6650_s_stream, >>> .g_frame_interval = ov6650_g_frame_interval, >>> .s_frame_interval = ov6650_s_frame_interval, >>> - .g_mbus_config = ov6650_g_mbus_config, >>> - .s_mbus_config = ov6650_s_mbus_config, >>> }; >>> >>> static const struct v4l2_subdev_pad_ops ov6650_pad_ops = { >>> @@ -978,6 +998,8 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = { >>> .set_selection = ov6650_set_selection, >>> .get_fmt = ov6650_get_fmt, >>> .set_fmt = ov6650_set_fmt, >>> + .get_mbus_config = ov6650_get_mbus_config, >>> + .set_mbus_config = ov6650_set_mbus_config, >>> }; >>> >>> static const struct v4l2_subdev_ops ov6650_subdev_ops = { >>> >> >> >> >>
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 91906b94f978..d2e7a8556ed7 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -921,46 +921,68 @@ static const struct v4l2_subdev_core_ops ov6650_core_ops = { }; /* Request bus settings on camera side */ -static int ov6650_g_mbus_config(struct v4l2_subdev *sd, - struct v4l2_mbus_config *cfg) +static int ov6650_get_mbus_config(struct v4l2_subdev *sd, + unsigned int pad, + struct v4l2_mbus_config *cfg) { + struct i2c_client *client = v4l2_get_subdevdata(sd); + u8 comj, comf; + int ret; + + ret = ov6650_reg_read(client, REG_COMJ, &comj); + if (ret) + return ret; - cfg->flags = V4L2_MBUS_MASTER | - V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_PCLK_SAMPLE_FALLING | - V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_LOW | - V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_LOW | - V4L2_MBUS_DATA_ACTIVE_HIGH; + ret = ov6650_reg_read(client, REG_COMF, &comf); + if (ret) + return ret; + + cfg->flags = V4L2_MBUS_MASTER + | ((comj & COMJ_VSYNC_HIGH) ? V4L2_MBUS_VSYNC_ACTIVE_HIGH + : V4L2_MBUS_VSYNC_ACTIVE_LOW) + | ((comf & COMF_HREF_LOW) ? V4L2_MBUS_HSYNC_ACTIVE_LOW + : V4L2_MBUS_HSYNC_ACTIVE_HIGH) + | ((comj & COMJ_PCLK_RISING) ? V4L2_MBUS_PCLK_SAMPLE_RISING + : V4L2_MBUS_PCLK_SAMPLE_FALLING); cfg->type = V4L2_MBUS_PARALLEL; return 0; } /* Alter bus settings on camera side */ -static int ov6650_s_mbus_config(struct v4l2_subdev *sd, - const struct v4l2_mbus_config *cfg) +static int ov6650_set_mbus_config(struct v4l2_subdev *sd, + unsigned int pad, + struct v4l2_mbus_config *cfg) { struct i2c_client *client = v4l2_get_subdevdata(sd); - int ret; + int ret = 0; if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING) ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0); - else + else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING) ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING); if (ret) - return ret; + goto error; if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW) ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0); - else + else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW); if (ret) - return ret; + goto error; if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0); - else + else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH); +error: + /* + * Update the configuration to report what is actually applied to + * the hardware. + */ + ov6650_get_mbus_config(sd, pad, cfg); + return ret; } @@ -968,8 +990,6 @@ static const struct v4l2_subdev_video_ops ov6650_video_ops = { .s_stream = ov6650_s_stream, .g_frame_interval = ov6650_g_frame_interval, .s_frame_interval = ov6650_s_frame_interval, - .g_mbus_config = ov6650_g_mbus_config, - .s_mbus_config = ov6650_s_mbus_config, }; static const struct v4l2_subdev_pad_ops ov6650_pad_ops = { @@ -978,6 +998,8 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = { .set_selection = ov6650_set_selection, .get_fmt = ov6650_get_fmt, .set_fmt = ov6650_set_fmt, + .get_mbus_config = ov6650_get_mbus_config, + .set_mbus_config = ov6650_set_mbus_config, }; static const struct v4l2_subdev_ops ov6650_subdev_ops = {
Use the new get_mbus_config and set_mbus_config pad operations in place of the video operations currently in use. Compared to other drivers where the same conversion has been performed, ov6650 proved to be a bit more tricky, as the existing g_mbus_config implementation did not report the currently applied configuration but the set of all possible configuration options. Adapt the driver to support the semantic of the two newly introduced operations: - get_mbus_config reports the current media bus configuration - set_mbus_config applies only changes explicitly requested and updates the provided cfg parameter to report what has actually been applied to the hardware. Compile-tested only. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/ov6650.c | 56 ++++++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 17 deletions(-)