Hi Lars,
On 01/13/2015 01:01 PM, Lars-Peter Clausen wrote:
We have basically the same code to set the video standard in init_device()
and adv7180_s_std(). Factor this out into a common helper function.
Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
drivers/media/i2c/adv7180.c | 67
++---
1 file changed, 32 insertions(+), 35 deletions(-)
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 349cae3..4d9bcc8 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -304,37 +304,54 @@ static int adv7180_g_input_status(struct v4l2_subdev
*sd, u32 *status)
return ret;
}
-static int adv7180_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
+static int adv7180_program_std(struct adv7180_state *state)
{
- struct adv7180_state *state = to_state(sd);
- int ret = mutex_lock_interruptible(state-mutex);
- if (ret)
- return ret;
+ int ret;
- /* all standards - autodetect */
- if (std == V4L2_STD_ALL) {
+ if (state-autodetect) {
Acked-by: Hans Verkuil hans.verk...@cisco.com
That said, I am unhappy about this autodetect handling. That isn't something
that this patch changes, which is why I've Acked it, but I hope you can look
at this yourself.
The reason I don't like it is because 1) it is non-standard behavior of a video
receiver to turn on autodetect if STD_ALL is passed in. And 2) there are
multiple autodetect modes and the one chosen seems to imply that NTSC M is not
autodetected, only NTSC-J. I have no adv7180 hardware, so I can't test if that
is indeed what is happening. In any case, every autodetect mode seems to
autodetect only a subset of all possible standards according to the adv7180
datasheet, which doesn't really make it a real autodetect IMHO.
The third and last reason is that if the autodetect system switches from NTSC to
PAL you suddenly get larger frames. Depending on the exact DMA configuration
of the board this could lead to buffer overflows (e.g. if the DMA configuration
just DMAs until the end of frame, and yes, such terrible DMA implementations
exist).
An initial autodetect when the driver is loaded might make sense in order to get
a reasonable initial standard, but I am skeptical about using it anywhere else.
BTW, if you can easily detect standard changes via an interrupt, then you can
use that interrupt to send a V4L2_EVENT_SOURCE_CHANGE event. That would allow
applications to dynamically react to changes in the standard.
As I said, I have no adv718x hardware so I am unable to test this, but if you
could test this autodetect functionality and think about whether it should be
kept at all, then that would be useful.
Regards,
Hans
ret = adv7180_write(state, ADV7180_REG_INPUT_CONTROL,
ADV7180_INPUT_CONTROL_AD_PAL_BG_NTSC_J_SECAM
| state-input);
if (ret 0)
- goto out;
+ return ret;
__adv7180_status(state, NULL, state-curr_norm);
- state-autodetect = true;
} else {
- ret = v4l2_std_to_adv7180(std);
+ ret = v4l2_std_to_adv7180(state-curr_norm);
if (ret 0)
- goto out;
+ return ret;
ret = adv7180_write(state, ADV7180_REG_INPUT_CONTROL,
ret | state-input);
if (ret 0)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int adv7180_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
+{
+ struct adv7180_state *state = to_state(sd);
+ int ret = mutex_lock_interruptible(state-mutex);
+
+ 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;
state-curr_norm = std;
state-autodetect = false;
}
- ret = 0;
+
+ ret = adv7180_program_std(state);
out:
mutex_unlock(state-mutex);
return ret;
@@ -547,30 +564,10 @@ static int init_device(struct adv7180_state *state)
adv7180_write(state, ADV7180_REG_PWR_MAN, ADV7180_PWR_MAN_RES);
usleep_range(2000, 1);
- /* Initialize adv7180 */
- /* Enable autodetection */
- if (state-autodetect) {
- ret = adv7180_write(state, ADV7180_REG_INPUT_CONTROL,
- ADV7180_INPUT_CONTROL_AD_PAL_BG_NTSC_J_SECAM
- | state-input);
- if (ret 0)
- goto out_unlock;
-
- ret = adv7180_write(state, ADV7180_REG_AUTODETECT_ENABLE,
-
