[PATCH] [for 3.7] stk1160: Add support for S-Video input

[PATCH] [for 3.7] stk1160: Add support for S-Video input

[Ezequiel Garcia]

In order to fully replace easycap driver with stk1160,
it's also necessary to add S-Video support.

A similar patch backported for v3.2 kernel has been
tested by three different users.

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
Hi Mauro,

I'm sending this for inclusion in v3.7 second media pull request.
I realize it's very late, so I understand if you don't
want to pick it.

 drivers/media/usb/stk1160/stk1160-core.c |   15 +++++++++++----
 drivers/media/usb/stk1160/stk1160-v4l.c  |    7 ++++++-
 drivers/media/usb/stk1160/stk1160.h      |    3 ++-
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
index b627408..34a26e0 100644
--- a/drivers/media/usb/stk1160/stk1160-core.c
+++ b/drivers/media/usb/stk1160/stk1160-core.c
@@ -100,12 +100,21 @@ int stk1160_write_reg(struct stk1160 *dev, u16 reg, u16 value)
 
 void stk1160_select_input(struct stk1160 *dev)
 {
+	int route;
 	static const u8 gctrl[] = {
-		0x98, 0x90, 0x88, 0x80
+		0x98, 0x90, 0x88, 0x80, 0x98
 	};
 
-	if (dev->ctl_input < ARRAY_SIZE(gctrl))
+	if (dev->ctl_input == STK1160_SVIDEO_INPUT)
+		route = SAA7115_SVIDEO3;
+	else
+		route = SAA7115_COMPOSITE0;
+
+	if (dev->ctl_input < ARRAY_SIZE(gctrl)) {
+		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
+				route, 0, 0);
 		stk1160_write_reg(dev, STK1160_GCTRL, gctrl[dev->ctl_input]);
+	}
 }
 
 /* TODO: We should break this into pieces */
@@ -351,8 +360,6 @@ static int stk1160_probe(struct usb_interface *interface,
 
 	/* i2c reset saa711x */
 	v4l2_device_call_all(&dev->v4l2_dev, 0, core, reset, 0);
-	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
-				0, 0, 0);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0);
 
 	/* reset stk1160 to default values */
diff --git a/drivers/media/usb/stk1160/stk1160-v4l.c b/drivers/media/usb/stk1160/stk1160-v4l.c
index fe6e857..6694f9e 100644
--- a/drivers/media/usb/stk1160/stk1160-v4l.c
+++ b/drivers/media/usb/stk1160/stk1160-v4l.c
@@ -419,7 +419,12 @@ static int vidioc_enum_input(struct file *file, void *priv,
 	if (i->index > STK1160_MAX_INPUT)
 		return -EINVAL;
 
-	sprintf(i->name, "Composite%d", i->index);
+	/* S-Video special handling */
+	if (i->index == STK1160_SVIDEO_INPUT)
+		sprintf(i->name, "S-Video");
+	else
+		sprintf(i->name, "Composite%d", i->index);
+
 	i->type = V4L2_INPUT_TYPE_CAMERA;
 	i->std = dev->vdev.tvnorms;
 	return 0;
diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
index 3feba00..68c8707 100644
--- a/drivers/media/usb/stk1160/stk1160.h
+++ b/drivers/media/usb/stk1160/stk1160.h
@@ -46,7 +46,8 @@
 
 #define STK1160_MIN_PKT_SIZE 3072
 
-#define STK1160_MAX_INPUT 3
+#define STK1160_MAX_INPUT 4
+#define STK1160_SVIDEO_INPUT 4
 
 #define STK1160_I2C_TIMEOUT 100
 
-- 
1.7.4.4

Re: [PATCH] [for 3.7] stk1160: Add support for S-Video input

[Mauro Carvalho Chehab]

Em Tue,  9 Oct 2012 19:01:03 -0300
Ezequiel Garcia <elezegarcia@gmail.com> escreveu:

> In order to fully replace easycap driver with stk1160,
> it's also necessary to add S-Video support.
> 
> A similar patch backported for v3.2 kernel has been
> tested by three different users.
> 
> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> ---
> Hi Mauro,
> 
> I'm sending this for inclusion in v3.7 second media pull request.
> I realize it's very late, so I understand if you don't
> want to pick it.
> 
>  drivers/media/usb/stk1160/stk1160-core.c |   15 +++++++++++----
>  drivers/media/usb/stk1160/stk1160-v4l.c  |    7 ++++++-
>  drivers/media/usb/stk1160/stk1160.h      |    3 ++-
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
> index b627408..34a26e0 100644
> --- a/drivers/media/usb/stk1160/stk1160-core.c
> +++ b/drivers/media/usb/stk1160/stk1160-core.c
> @@ -100,12 +100,21 @@ int stk1160_write_reg(struct stk1160 *dev, u16 reg, u16 value)
>  
>  void stk1160_select_input(struct stk1160 *dev)
>  {
> +	int route;
>  	static const u8 gctrl[] = {
> -		0x98, 0x90, 0x88, 0x80
> +		0x98, 0x90, 0x88, 0x80, 0x98
>  	};
>  
> -	if (dev->ctl_input < ARRAY_SIZE(gctrl))
> +	if (dev->ctl_input == STK1160_SVIDEO_INPUT)
> +		route = SAA7115_SVIDEO3;
> +	else
> +		route = SAA7115_COMPOSITE0;
> +
> +	if (dev->ctl_input < ARRAY_SIZE(gctrl)) {
> +		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
> +				route, 0, 0);
>  		stk1160_write_reg(dev, STK1160_GCTRL, gctrl[dev->ctl_input]);
> +	}
>  }
>  
>  /* TODO: We should break this into pieces */
> @@ -351,8 +360,6 @@ static int stk1160_probe(struct usb_interface *interface,
>  
>  	/* i2c reset saa711x */
>  	v4l2_device_call_all(&dev->v4l2_dev, 0, core, reset, 0);
> -	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
> -				0, 0, 0);
>  	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0);
>  
>  	/* reset stk1160 to default values */
> diff --git a/drivers/media/usb/stk1160/stk1160-v4l.c b/drivers/media/usb/stk1160/stk1160-v4l.c
> index fe6e857..6694f9e 100644
> --- a/drivers/media/usb/stk1160/stk1160-v4l.c
> +++ b/drivers/media/usb/stk1160/stk1160-v4l.c
> @@ -419,7 +419,12 @@ static int vidioc_enum_input(struct file *file, void *priv,
>  	if (i->index > STK1160_MAX_INPUT)
>  		return -EINVAL;
>  
> -	sprintf(i->name, "Composite%d", i->index);
> +	/* S-Video special handling */
> +	if (i->index == STK1160_SVIDEO_INPUT)
> +		sprintf(i->name, "S-Video");
> +	else
> +		sprintf(i->name, "Composite%d", i->index);
> +

Had you ever test this patch with the v4l2-compliance tool?

It seems broken to me: this driver has just two inputs. So, it should return
-EINVAL for all inputs after that, or otherwise userspace applications that
query the inputs will loop forever!

Regards,
Mauro

Re: [PATCH] [for 3.7] stk1160: Add support for S-Video input

[Ezequiel Garcia]

On Tue, Oct 9, 2012 at 7:25 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em Tue,  9 Oct 2012 19:01:03 -0300
> Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
>
>> In order to fully replace easycap driver with stk1160,
>> it's also necessary to add S-Video support.
>>
>> A similar patch backported for v3.2 kernel has been
>> tested by three different users.
>>
>> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
>> ---
>> Hi Mauro,
>>
>> I'm sending this for inclusion in v3.7 second media pull request.
>> I realize it's very late, so I understand if you don't
>> want to pick it.
>>
>>  drivers/media/usb/stk1160/stk1160-core.c |   15 +++++++++++----
>>  drivers/media/usb/stk1160/stk1160-v4l.c  |    7 ++++++-
>>  drivers/media/usb/stk1160/stk1160.h      |    3 ++-
>>  3 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
>> index b627408..34a26e0 100644
>> --- a/drivers/media/usb/stk1160/stk1160-core.c
>> +++ b/drivers/media/usb/stk1160/stk1160-core.c
>> @@ -100,12 +100,21 @@ int stk1160_write_reg(struct stk1160 *dev, u16 reg, u16 value)
>>
>>  void stk1160_select_input(struct stk1160 *dev)
>>  {
>> +     int route;
>>       static const u8 gctrl[] = {
>> -             0x98, 0x90, 0x88, 0x80
>> +             0x98, 0x90, 0x88, 0x80, 0x98
>>       };
>>
>> -     if (dev->ctl_input < ARRAY_SIZE(gctrl))
>> +     if (dev->ctl_input == STK1160_SVIDEO_INPUT)
>> +             route = SAA7115_SVIDEO3;
>> +     else
>> +             route = SAA7115_COMPOSITE0;
>> +
>> +     if (dev->ctl_input < ARRAY_SIZE(gctrl)) {
>> +             v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
>> +                             route, 0, 0);
>>               stk1160_write_reg(dev, STK1160_GCTRL, gctrl[dev->ctl_input]);
>> +     }
>>  }
>>
>>  /* TODO: We should break this into pieces */
>> @@ -351,8 +360,6 @@ static int stk1160_probe(struct usb_interface *interface,
>>
>>       /* i2c reset saa711x */
>>       v4l2_device_call_all(&dev->v4l2_dev, 0, core, reset, 0);
>> -     v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
>> -                             0, 0, 0);
>>       v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0);
>>
>>       /* reset stk1160 to default values */
>> diff --git a/drivers/media/usb/stk1160/stk1160-v4l.c b/drivers/media/usb/stk1160/stk1160-v4l.c
>> index fe6e857..6694f9e 100644
>> --- a/drivers/media/usb/stk1160/stk1160-v4l.c
>> +++ b/drivers/media/usb/stk1160/stk1160-v4l.c
>> @@ -419,7 +419,12 @@ static int vidioc_enum_input(struct file *file, void *priv,
>>       if (i->index > STK1160_MAX_INPUT)
>>               return -EINVAL;
>>

Look here... I'm returning EINVAL after  STK1160_MAX_INPUT.
(see below)

>> -     sprintf(i->name, "Composite%d", i->index);
>> +     /* S-Video special handling */
>> +     if (i->index == STK1160_SVIDEO_INPUT)
>> +             sprintf(i->name, "S-Video");
>> +     else
>> +             sprintf(i->name, "Composite%d", i->index);
>> +
>
> Had you ever test this patch with the v4l2-compliance tool?
>
> It seems broken to me: this driver has just two inputs. So, it should return
> -EINVAL for all inputs after that, or otherwise userspace applications that
> query the inputs will loop forever!
>

Actually the driver has five inputs, since there are two kinds of devices:
one with four composites, and another with one composite and one s-video.
So, I simply support all of them, since there's no way to distinguish.

I just tested this patch with v4l2-compliance and with qv4l2 and there
are no regressions.

Unless I'm missing something, I think the patch is OK.
Let me know if you want me to change something.

    Ezequiel.

Re: [PATCH] [for 3.7] stk1160: Add support for S-Video input

[Mauro Carvalho Chehab]

Em Tue, 9 Oct 2012 20:52:06 -0300
Ezequiel Garcia <elezegarcia@gmail.com> escreveu:

> On Tue, Oct 9, 2012 at 7:25 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
> > Em Tue,  9 Oct 2012 19:01:03 -0300
> > Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
> >
> >> In order to fully replace easycap driver with stk1160,
> >> it's also necessary to add S-Video support.
> >>
> >> A similar patch backported for v3.2 kernel has been
> >> tested by three different users.
> >>
> >> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> >> ---
> >> Hi Mauro,
> >>
> >> I'm sending this for inclusion in v3.7 second media pull request.
> >> I realize it's very late, so I understand if you don't
> >> want to pick it.
> >>
> >>  drivers/media/usb/stk1160/stk1160-core.c |   15 +++++++++++----
> >>  drivers/media/usb/stk1160/stk1160-v4l.c  |    7 ++++++-
> >>  drivers/media/usb/stk1160/stk1160.h      |    3 ++-
> >>  3 files changed, 19 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
> >> index b627408..34a26e0 100644
> >> --- a/drivers/media/usb/stk1160/stk1160-core.c
> >> +++ b/drivers/media/usb/stk1160/stk1160-core.c
> >> @@ -100,12 +100,21 @@ int stk1160_write_reg(struct stk1160 *dev, u16 reg, u16 value)
> >>
> >>  void stk1160_select_input(struct stk1160 *dev)
> >>  {
> >> +     int route;
> >>       static const u8 gctrl[] = {
> >> -             0x98, 0x90, 0x88, 0x80
> >> +             0x98, 0x90, 0x88, 0x80, 0x98
> >>       };
> >>
> >> -     if (dev->ctl_input < ARRAY_SIZE(gctrl))
> >> +     if (dev->ctl_input == STK1160_SVIDEO_INPUT)
> >> +             route = SAA7115_SVIDEO3;
> >> +     else
> >> +             route = SAA7115_COMPOSITE0;
> >> +
> >> +     if (dev->ctl_input < ARRAY_SIZE(gctrl)) {
> >> +             v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
> >> +                             route, 0, 0);
> >>               stk1160_write_reg(dev, STK1160_GCTRL, gctrl[dev->ctl_input]);
> >> +     }
> >>  }
> >>
> >>  /* TODO: We should break this into pieces */
> >> @@ -351,8 +360,6 @@ static int stk1160_probe(struct usb_interface *interface,
> >>
> >>       /* i2c reset saa711x */
> >>       v4l2_device_call_all(&dev->v4l2_dev, 0, core, reset, 0);
> >> -     v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
> >> -                             0, 0, 0);
> >>       v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0);
> >>
> >>       /* reset stk1160 to default values */
> >> diff --git a/drivers/media/usb/stk1160/stk1160-v4l.c b/drivers/media/usb/stk1160/stk1160-v4l.c
> >> index fe6e857..6694f9e 100644
> >> --- a/drivers/media/usb/stk1160/stk1160-v4l.c
> >> +++ b/drivers/media/usb/stk1160/stk1160-v4l.c
> >> @@ -419,7 +419,12 @@ static int vidioc_enum_input(struct file *file, void *priv,
> >>       if (i->index > STK1160_MAX_INPUT)
> >>               return -EINVAL;
> >>
> 
> Look here... I'm returning EINVAL after  STK1160_MAX_INPUT.
> (see below)

Sorry, my bad. I was, in fact, tricked by the first hunk, where you was
routing to either saa7115 video3 or composite0. Now, I noticed that this
device has an extra video switch, controlled via STK1160_GCTRL register.

Tricky.

> 
> >> -     sprintf(i->name, "Composite%d", i->index);
> >> +     /* S-Video special handling */
> >> +     if (i->index == STK1160_SVIDEO_INPUT)
> >> +             sprintf(i->name, "S-Video");
> >> +     else
> >> +             sprintf(i->name, "Composite%d", i->index);
> >> +
> >
> > Had you ever test this patch with the v4l2-compliance tool?
> >
> > It seems broken to me: this driver has just two inputs. So, it should return
> > -EINVAL for all inputs after that, or otherwise userspace applications that
> > query the inputs will loop forever!
> >
> 
> Actually the driver has five inputs, since there are two kinds of devices:
> one with four composites, and another with one composite and one s-video.
> So, I simply support all of them, since there's no way to distinguish.

Ok then. 

> 
> I just tested this patch with v4l2-compliance and with qv4l2 and there
> are no regressions.
> 
> Unless I'm missing something, I think the patch is OK.
> Let me know if you want me to change something.

With the light of your comments, the patch looks fine on my eyes.

> 
>     Ezequiel.


-- 
Regards,
Mauro

Re: [PATCH] [for 3.7] stk1160: Add support for S-Video input

[Mauro Carvalho Chehab]

Em Tue, 9 Oct 2012 21:04:46 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:

> Em Tue, 9 Oct 2012 20:52:06 -0300
> Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
> 
> > On Tue, Oct 9, 2012 at 7:25 PM, Mauro Carvalho Chehab
> > <mchehab@redhat.com> wrote:
> > > Em Tue,  9 Oct 2012 19:01:03 -0300
> > > Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
> > >

> > Let me know if you want me to change something.

Hmm... patchwork didn't get it. The MIME type for the attachment
is wrong... it is "text/plain". That may explain why patchwork
didn't get it. Could you please re-submit it with the patch
inlined, or if you really can't do it, using the proper MIME
types for patch?

Thanks!
Mauro

Re: [PATCH] [for 3.7] stk1160: Add support for S-Video input

[Mauro Carvalho Chehab]

Em Tue, 9 Oct 2012 21:11:04 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:

> Em Tue, 9 Oct 2012 21:04:46 -0300
> Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:
> 
> > Em Tue, 9 Oct 2012 20:52:06 -0300
> > Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
> > 
> > > On Tue, Oct 9, 2012 at 7:25 PM, Mauro Carvalho Chehab
> > > <mchehab@redhat.com> wrote:
> > > > Em Tue,  9 Oct 2012 19:01:03 -0300
> > > > Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
> > > >
> 
> > > Let me know if you want me to change something.
> 
> Hmm... patchwork didn't get it. The MIME type for the attachment
> is wrong... it is "text/plain". That may explain why patchwork
> didn't get it. Could you please re-submit it with the patch
> inlined, or if you really can't do it, using the proper MIME
> types for patch?

Nevermind. the patch looks correct. I'll double-check what's wrong
at patchwork side.
> 
> Thanks!
> Mauro


-- 
Regards,
Mauro

Re: [PATCH] [for 3.7] stk1160: Add support for S-Video input

[Ezequiel Garcia]

On Tue, Oct 9, 2012 at 9:13 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em Tue, 9 Oct 2012 21:11:04 -0300
> Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:
>
>> Em Tue, 9 Oct 2012 21:04:46 -0300
>> Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:
>>
>> > Em Tue, 9 Oct 2012 20:52:06 -0300
>> > Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
>> >
>> > > On Tue, Oct 9, 2012 at 7:25 PM, Mauro Carvalho Chehab
>> > > <mchehab@redhat.com> wrote:
>> > > > Em Tue,  9 Oct 2012 19:01:03 -0300
>> > > > Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
>> > > >
>>
>> > > Let me know if you want me to change something.
>>
>> Hmm... patchwork didn't get it. The MIME type for the attachment
>> is wrong... it is "text/plain". That may explain why patchwork
>> didn't get it. Could you please re-submit it with the patch
>> inlined, or if you really can't do it, using the proper MIME
>> types for patch?
>
> Nevermind. the patch looks correct. I'll double-check what's wrong
> at patchwork side.

Great! Thanks!

    Ezequiel