Re: [PATCH 1/6] adv7180: fix broken standards handling

2016-04-24 Thread Lars-Peter Clausen 
On 04/22/2016 03:03 PM, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> The adv7180 attempts to autodetect the standard. Unfortunately this
> is seriously broken.
> 
> This patch removes the autodetect completely. Only the querystd op
> will detect the standard. Since the design of the adv7180 requires
> that you switch to a special autodetect mode you cannot call querystd
> when you are streaming.
> 
> So the s_stream op has been added so we know whether we are streaming
> or not, and if we are, then querystd returns EBUSY.
> 
> After testing this with a signal generator is became obvious that
> a sleep is needed between changing the standard to autodetect and
> reading the status. So the autodetect can never have worked well.
> 
> The s_std call now just sets the new standard without any querying.
> 
> If the driver supports the interrupt, then when it detects a standard
> change it will signal that by sending the V4L2_EVENT_SOURCE_CHANGE
> event.
> 
> With these changes this driver now behaves like all other video
> receivers.
> 
> Signed-off-by: Hans Verkuil 
> Cc: Niklas Söderlund 
> Cc: Lars-Peter Clausen 
> Cc: Federico Vaga 

Thanks for cleaning this up.

Acked-by: Lars-Peter Clausen 

--
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 1/6] adv7180: fix broken standards handling

2016-04-22 Thread Niklas Söderlund 
Hi Hans,

Tested-by: Niklas Söderlund 

On 2016-04-22 15:03:37 +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> The adv7180 attempts to autodetect the standard. Unfortunately this
> is seriously broken.
> 
> This patch removes the autodetect completely. Only the querystd op
> will detect the standard. Since the design of the adv7180 requires
> that you switch to a special autodetect mode you cannot call querystd
> when you are streaming.
> 
> So the s_stream op has been added so we know whether we are streaming
> or not, and if we are, then querystd returns EBUSY.
> 
> After testing this with a signal generator is became obvious that
> a sleep is needed between changing the standard to autodetect and
> reading the status. So the autodetect can never have worked well.
> 
> The s_std call now just sets the new standard without any querying.
> 
> If the driver supports the interrupt, then when it detects a standard
> change it will signal that by sending the V4L2_EVENT_SOURCE_CHANGE
> event.
> 
> With these changes this driver now behaves like all other video
> receivers.
> 
> Signed-off-by: Hans Verkuil 
> Cc: Niklas Söderlund 
> Cc: Lars-Peter Clausen 
> Cc: Federico Vaga 
> ---
>  drivers/media/i2c/adv7180.c | 118 
> ++--
>  1 file changed, 80 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 51a92b3..5a75a91 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -26,8 +26,9 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -192,8 +193,8 @@ struct adv7180_state {
>   struct mutexmutex; /* mutual excl. when accessing chip */
>   int irq;
>   v4l2_std_id curr_norm;
> - boolautodetect;
>   boolpowered;
> + boolstreaming;
>   u8  input;
>  
>   struct i2c_client   *client;
> @@ -338,12 +339,26 @@ static int adv7180_querystd(struct v4l2_subdev *sd, 
> v4l2_std_id *std)
>   if (err)
>   return err;
>  
> - /* when we are interrupt driven we know the state */
> - if (!state->autodetect || state->irq > 0)
> - *std = state->curr_norm;
> - else
> - err = __adv7180_status(state, NULL, std);
> + if (state->streaming) {
> + err = -EBUSY;
> + goto unlock;
> + }
> +
> + err = adv7180_set_video_standard(state,
> + ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM);
> + if (err)
> + goto unlock;
>  
> + msleep(100);
> + __adv7180_status(state, NULL, std);
> +
> + err = v4l2_std_to_adv7180(state->curr_norm);
> + if (err < 0)
> + goto unlock;
> +
> + err = adv7180_set_video_standard(state, err);
> +
> +unlock:
>   mutex_unlock(>mutex);
>   return err;
>  }
> @@ -387,23 +402,13 @@ static int adv7180_program_std(struct adv7180_state 
> *state)
>  {
>   int ret;
>  
> - if (state->autodetect) {
> - ret = adv7180_set_video_standard(state,
> - ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM);
> - if (ret < 0)
> - return ret;
> -
> - __adv7180_status(state, NULL, >curr_norm);
> - } else {
> - ret = v4l2_std_to_adv7180(state->curr_norm);
> - if (ret < 0)
> - return ret;
> -
> - ret = adv7180_set_video_standard(state, ret);
> - if (ret < 0)
> - return ret;
> - }
> + ret = v4l2_std_to_adv7180(state->curr_norm);
> + if (ret < 0)
> + return ret;
>  
> + ret = adv7180_set_video_standard(state, ret);
> + if (ret < 0)
> + return ret;
>   return 0;
>  }
>  
> @@ -415,18 +420,12 @@ static int adv7180_s_std(struct v4l2_subdev *sd, 
> v4l2_std_id std)
>   if (ret)
>   return ret;
>  
> - /* all standards -> autodetect */
> - if (std == V4L2_STD_ALL) {
> - state->autodetect = true;
> - } else {
> - /* Make sure we can support this std */
> - ret = v4l2_std_to_adv7180(std);
> - if (ret < 0)
> - goto out;
> + /* Make sure we can support this std */
> + ret = v4l2_std_to_adv7180(std);
> + if (ret < 0)
> + goto out;
>  
> - state->curr_norm = std;
> - state->autodetect = false;
> - }
> + state->curr_norm = std;
>  
>   ret = adv7180_program_std(state);
>  out:
> @@ -747,6 +746,40 @@ static int adv7180_g_tvnorms(struct v4l2_subdev *sd, 
> v4l2_std_id *norm)
>   return 0;
>  }
>  
> +static int

Re: [PATCH 1/6] adv7180: fix broken standards handling

2016-04-22 Thread Federico Vaga 
Acked-by: Federico Vaga 

On Friday, April 22, 2016 03:03:37 PM Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> The adv7180 attempts to autodetect the standard. Unfortunately this
> is seriously broken.
> 
> This patch removes the autodetect completely. Only the querystd op
> will detect the standard. Since the design of the adv7180 requires
> that you switch to a special autodetect mode you cannot call querystd
> when you are streaming.
> 
> So the s_stream op has been added so we know whether we are streaming
> or not, and if we are, then querystd returns EBUSY.
> 
> After testing this with a signal generator is became obvious that
> a sleep is needed between changing the standard to autodetect and
> reading the status. So the autodetect can never have worked well.
> 
> The s_std call now just sets the new standard without any querying.
> 
> If the driver supports the interrupt, then when it detects a standard
> change it will signal that by sending the V4L2_EVENT_SOURCE_CHANGE
> event.
> 
> With these changes this driver now behaves like all other video
> receivers.
> 
> Signed-off-by: Hans Verkuil 
> Cc: Niklas Söderlund 
> Cc: Lars-Peter Clausen 
> Cc: Federico Vaga 
> ---
>  drivers/media/i2c/adv7180.c | 118
> ++-- 1 file changed, 80
> insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 51a92b3..5a75a91 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -26,8 +26,9 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -192,8 +193,8 @@ struct adv7180_state {
>   struct mutexmutex; /* mutual excl. when accessing chip */
>   int irq;
>   v4l2_std_id curr_norm;
> - boolautodetect;
>   boolpowered;
> + boolstreaming;
>   u8  input;
> 
>   struct i2c_client   *client;
> @@ -338,12 +339,26 @@ static int adv7180_querystd(struct v4l2_subdev *sd,
> v4l2_std_id *std) if (err)
>   return err;
> 
> - /* when we are interrupt driven we know the state */
> - if (!state->autodetect || state->irq > 0)
> - *std = state->curr_norm;
> - else
> - err = __adv7180_status(state, NULL, std);
> + if (state->streaming) {
> + err = -EBUSY;
> + goto unlock;
> + }
> +
> + err = adv7180_set_video_standard(state,
> + ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM);
> + if (err)
> + goto unlock;
> 
> + msleep(100);
> + __adv7180_status(state, NULL, std);
> +
> + err = v4l2_std_to_adv7180(state->curr_norm);
> + if (err < 0)
> + goto unlock;
> +
> + err = adv7180_set_video_standard(state, err);
> +
> +unlock:
>   mutex_unlock(>mutex);
>   return err;
>  }
> @@ -387,23 +402,13 @@ static int adv7180_program_std(struct adv7180_state
> *state) {
>   int ret;
> 
> - if (state->autodetect) {
> - ret = adv7180_set_video_standard(state,
> - ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM);
> - if (ret < 0)
> - return ret;
> -
> - __adv7180_status(state, NULL, >curr_norm);
> - } else {
> - ret = v4l2_std_to_adv7180(state->curr_norm);
> - if (ret < 0)
> - return ret;
> -
> - ret = adv7180_set_video_standard(state, ret);
> - if (ret < 0)
> - return ret;
> - }
> + ret = v4l2_std_to_adv7180(state->curr_norm);
> + if (ret < 0)
> + return ret;
> 
> + ret = adv7180_set_video_standard(state, ret);
> + if (ret < 0)
> + return ret;
>   return 0;
>  }
> 
> @@ -415,18 +420,12 @@ static int adv7180_s_std(struct v4l2_subdev *sd,
> v4l2_std_id std) if (ret)
>   return ret;
> 
> - /* all standards -> autodetect */
> - if (std == V4L2_STD_ALL) {
> - state->autodetect = true;
> - } else {
> - /* Make sure we can support this std */
> - ret = v4l2_std_to_adv7180(std);
> - if (ret < 0)
> - goto out;
> + /* Make sure we can support this std */
> + ret = v4l2_std_to_adv7180(std);
> + if (ret < 0)
> + goto out;
> 
> - state->curr_norm = std;
> - state->autodetect = false;
> - }
> + state->curr_norm = std;
> 
>   ret = adv7180_program_std(state);
>  out:
> @@ -747,6 +746,40 @@ static int adv7180_g_tvnorms(struct v4l2_subdev *sd,
> v4l2_std_id *norm) return 0;
>  }
> 
> +static int adv7180_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +