Re: [PATCH 3/3] media: adv7604: update timings on change of input signal

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

On Friday 11 December 2015 17:04:53 Ulrich Hecht wrote:
> Without this, g_crop will always return the boot-time state.
> 
> Signed-off-by: Ulrich Hecht 
> ---
>  drivers/media/i2c/adv7604.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 1bfa9f3..d7d0bb7 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1975,6 +1975,15 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32
> status, bool *handled)
> 
>   v4l2_subdev_notify_event(sd, _ev_fmt);
> 
> + /* update timings */
> + if (adv76xx_query_dv_timings(sd, >timings)
> + == -ENOLINK) {

Nitpicking, I would write this as

ret = adv76xx_query_dv_timings(sd, >timings);
if (ret == -ENOLINK) {

to make it more explicit that the function has side effects. Functions called 
inside an if () statement are often assumed (at least by me) to perform checks 
only and not modify their parameters.

> + /* no signal, fall back to default timings */
> + const struct v4l2_dv_timings cea640x480 =
> + V4L2_DV_BT_CEA_640X480P59_94;
> + state->timings = cea640x480;

You can write this as

state->timings = (struct v4l2_dv_timings)
V4L2_DV_BT_CEA_640X480P59_94;

without using a local variable.

(And now that I mention that I wonder whether the definition of 
V4L2_DV_BT_CEA_640X480P59_94 should be updated to include the (struct 
v4l2_dv_timings))

> + }
> +
>   if (handled)
>   *handled = true;
>   }

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] media: adv7604: update timings on change of input signal

[Ulrich Hecht]

Without this, g_crop will always return the boot-time state.

Signed-off-by: Ulrich Hecht 
---
 drivers/media/i2c/adv7604.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 1bfa9f3..d7d0bb7 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -1975,6 +1975,15 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32 
status, bool *handled)
 
v4l2_subdev_notify_event(sd, _ev_fmt);
 
+   /* update timings */
+   if (adv76xx_query_dv_timings(sd, >timings)
+   == -ENOLINK) {
+   /* no signal, fall back to default timings */
+   const struct v4l2_dv_timings cea640x480 =
+   V4L2_DV_BT_CEA_640X480P59_94;
+   state->timings = cea640x480;
+   }
+
if (handled)
*handled = true;
}
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] media: adv7604: update timings on change of input signal

[Sergei Shtylyov]

Hello.

On 12/11/2015 07:04 PM, Ulrich Hecht wrote:


Without this, g_crop will always return the boot-time state.

Signed-off-by: Ulrich Hecht 
---
  drivers/media/i2c/adv7604.c | 9 +
  1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 1bfa9f3..d7d0bb7 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -1975,6 +1975,15 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32 
status, bool *handled)

v4l2_subdev_notify_event(sd, _ev_fmt);

+   /* update timings */
+   if (adv76xx_query_dv_timings(sd, >timings)
+   == -ENOLINK) {


   Please don't put the binary operators on the continuation line, leave them 
at the end of he broken up line.


[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html