Re: [PATCH v4] media: Add stk1160 new driver
Hans, On Tue, Jul 10, 2012 at 3:39 AM, Hans Verkuil hverk...@xs4all.nl wrote: Take a look at the latest videobuf2-core.h: I've added helper functions that check the owner. You can probably simplify the driver code quite a bit by using those helpers. Indeed, using latest vb2_xxx_fop and vb2_ioctl_xxx the driver can be heavily reduced. (Great work, by the way) Almost every function looks like a direct replacement, except for mmap. If you look at current stk1160, I'm taking the lock for mmap: mutex_lock(dev-v4l_lock); rc = vb2_mmap(dev-vb_vidq, vma); mutex_unlock(dev-v4l_lock); However, vb2_fop_mmap does no locking. I'm having a hard time understanding why this is not needed, perhaps you could clarify this a bit? Thanks, Ezequiel. -- 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 v4] media: Add stk1160 new driver
On Tue July 10 2012 05:17:41 Ezequiel Garcia wrote:
Hey Mauro,
On Fri, Jul 6, 2012 at 11:41 AM, Ezequiel Garcia elezegar...@gmail.com
wrote:
On Thu, Jul 5, 2012 at 9:01 PM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
Em 05-07-2012 19:36, Sylwester Nawrocki escreveu:
On 07/06/2012 12:11 AM, Mauro Carvalho Chehab wrote:
+static int vidioc_dqbuf(struct file *file, void *priv, struct
v4l2_buffer *p)
+{
+ struct stk1160 *dev = video_drvdata(file);
+
+ if (!stk1160_is_owner(dev, file))
+ return -EBUSY;
+
+ return vb2_dqbuf(dev-vb_vidq, p, file-f_flags O_NONBLOCK);
Take a look at the latest videobuf2-core.h: I've added helper functions
that check the owner. You can probably simplify the driver code quite a bit
by using those helpers.
Why to use O_NONBLOCK here? it should be doing whatever userspace wants.
This is OK, since the third argument to vb2_dqbuf() is a boolean
indicating
whether this call should be blocking or not. And a O_NONBLOCK masks
this
information out from file-f_flags.
Ah! OK then.
It might be better to initialize it during vb2 initialization, at open,
instead of requiring this argument every time vb_dqbuf() is called.
You can't do this at open since the application can change the NONBLOCK mode
after open. So the current approach is correct.
Regards,
Hans
Currently stk1160 doesn't implement an open call, but uses v4l2_fh_open
instead.
I'm not sure I should add a separate open, or perhaps you would accept
to initialize this non-block flag in vidioc_reqbufs.
On the other hand, many drivers are doing it at dqbuf, like here at stk1160,
and I was wondering: is it *that* bad?
Thanks,
Ezequiel.
--
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 v4] media: Add stk1160 new driver
Hi Hans,
On Tue, Jul 10, 2012 at 3:39 AM, Hans Verkuil hverk...@xs4all.nl wrote:
On Tue July 10 2012 05:17:41 Ezequiel Garcia wrote:
Hey Mauro,
On Fri, Jul 6, 2012 at 11:41 AM, Ezequiel Garcia elezegar...@gmail.com
wrote:
On Thu, Jul 5, 2012 at 9:01 PM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
Em 05-07-2012 19:36, Sylwester Nawrocki escreveu:
On 07/06/2012 12:11 AM, Mauro Carvalho Chehab wrote:
+static int vidioc_dqbuf(struct file *file, void *priv, struct
v4l2_buffer *p)
+{
+ struct stk1160 *dev = video_drvdata(file);
+
+ if (!stk1160_is_owner(dev, file))
+ return -EBUSY;
+
+ return vb2_dqbuf(dev-vb_vidq, p, file-f_flags O_NONBLOCK);
Take a look at the latest videobuf2-core.h: I've added helper functions
that check the owner. You can probably simplify the driver code quite a bit
by using those helpers.
Ok.
Why to use O_NONBLOCK here? it should be doing whatever userspace wants.
This is OK, since the third argument to vb2_dqbuf() is a boolean
indicating
whether this call should be blocking or not. And a O_NONBLOCK masks
this
information out from file-f_flags.
Ah! OK then.
It might be better to initialize it during vb2 initialization, at open,
instead of requiring this argument every time vb_dqbuf() is called.
You can't do this at open since the application can change the NONBLOCK mode
after open. So the current approach is correct.
Yes, that sounds ok. Let's wait until Mauro returns from holiday to discuss this
with him.
Also, what do you think about current_norm usage?
Regards,
Ezequiel.
--
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 v4] media: Add stk1160 new driver
On Tue 10 July 2012 14:26:11 Ezequiel Garcia wrote:
Hi Hans,
On Tue, Jul 10, 2012 at 3:39 AM, Hans Verkuil hverk...@xs4all.nl wrote:
On Tue July 10 2012 05:17:41 Ezequiel Garcia wrote:
Hey Mauro,
On Fri, Jul 6, 2012 at 11:41 AM, Ezequiel Garcia elezegar...@gmail.com
wrote:
On Thu, Jul 5, 2012 at 9:01 PM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
Em 05-07-2012 19:36, Sylwester Nawrocki escreveu:
On 07/06/2012 12:11 AM, Mauro Carvalho Chehab wrote:
+static int vidioc_dqbuf(struct file *file, void *priv, struct
v4l2_buffer *p)
+{
+ struct stk1160 *dev = video_drvdata(file);
+
+ if (!stk1160_is_owner(dev, file))
+ return -EBUSY;
+
+ return vb2_dqbuf(dev-vb_vidq, p, file-f_flags O_NONBLOCK);
Take a look at the latest videobuf2-core.h: I've added helper functions
that check the owner. You can probably simplify the driver code quite a bit
by using those helpers.
Ok.
Why to use O_NONBLOCK here? it should be doing whatever userspace
wants.
This is OK, since the third argument to vb2_dqbuf() is a boolean
indicating
whether this call should be blocking or not. And a O_NONBLOCK
masks this
information out from file-f_flags.
Ah! OK then.
It might be better to initialize it during vb2 initialization, at open,
instead of requiring this argument every time vb_dqbuf() is called.
You can't do this at open since the application can change the NONBLOCK mode
after open. So the current approach is correct.
Yes, that sounds ok. Let's wait until Mauro returns from holiday to discuss
this
with him.
Also, what do you think about current_norm usage?
Don't use it. Implement g_std instead. current_norm really doesn't add anything
useful, it is a bit too magical and it doesn't work if you have multiple nodes
that share the same std (e.g. video and vbi).
I'm removing it from existing drivers whenever I have the chance, and it will
eventually go away.
Regards,
Hans
--
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 v4] media: Add stk1160 new driver
Hey Mauro,
On Fri, Jul 6, 2012 at 11:41 AM, Ezequiel Garcia elezegar...@gmail.com wrote:
On Thu, Jul 5, 2012 at 9:01 PM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
Em 05-07-2012 19:36, Sylwester Nawrocki escreveu:
On 07/06/2012 12:11 AM, Mauro Carvalho Chehab wrote:
+static int vidioc_dqbuf(struct file *file, void *priv, struct
v4l2_buffer *p)
+{
+ struct stk1160 *dev = video_drvdata(file);
+
+ if (!stk1160_is_owner(dev, file))
+ return -EBUSY;
+
+ return vb2_dqbuf(dev-vb_vidq, p, file-f_flags O_NONBLOCK);
Why to use O_NONBLOCK here? it should be doing whatever userspace wants.
This is OK, since the third argument to vb2_dqbuf() is a boolean indicating
whether this call should be blocking or not. And a O_NONBLOCK masks this
information out from file-f_flags.
Ah! OK then.
It might be better to initialize it during vb2 initialization, at open,
instead of requiring this argument every time vb_dqbuf() is called.
Currently stk1160 doesn't implement an open call, but uses v4l2_fh_open instead.
I'm not sure I should add a separate open, or perhaps you would accept
to initialize this non-block flag in vidioc_reqbufs.
On the other hand, many drivers are doing it at dqbuf, like here at stk1160,
and I was wondering: is it *that* bad?
Thanks,
Ezequiel.
--
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 v4] media: Add stk1160 new driver
On Thu, Jul 5, 2012 at 9:01 PM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
Em 05-07-2012 19:36, Sylwester Nawrocki escreveu:
On 07/06/2012 12:11 AM, Mauro Carvalho Chehab wrote:
+static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer
*p)
+{
+ struct stk1160 *dev = video_drvdata(file);
+
+ if (!stk1160_is_owner(dev, file))
+ return -EBUSY;
+
+ return vb2_dqbuf(dev-vb_vidq, p, file-f_flags O_NONBLOCK);
Why to use O_NONBLOCK here? it should be doing whatever userspace wants.
This is OK, since the third argument to vb2_dqbuf() is a boolean indicating
whether this call should be blocking or not. And a O_NONBLOCK masks this
information out from file-f_flags.
Ah! OK then.
It might be better to initialize it during vb2 initialization, at open,
instead of requiring this argument every time vb_dqbuf() is called.
Okey, I'll do that.
Btw, just noticed a minor issue: an space is required before the operator.
That space was there, it got mangled on Sylwester answer. :-)
Regards,
Ezequiel.
--
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 v4] media: Add stk1160 new driver
Hi Mauro,
On Thu, Jul 5, 2012 at 7:11 PM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 99937c9..8d94d56 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -661,6 +661,8 @@ source drivers/media/video/hdpvr/Kconfig
source drivers/media/video/em28xx/Kconfig
+source drivers/media/video/stk1160/Kconfig
+
source drivers/media/video/tlg2300/Kconfig
source drivers/media/video/cx231xx/Kconfig
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index d209de0..7698b25 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -125,6 +125,7 @@ obj-$(CONFIG_VIDEO_HEXIUM_ORION) += hexium_orion.o
obj-$(CONFIG_VIDEO_HEXIUM_GEMINI) += hexium_gemini.o
obj-$(CONFIG_STA2X11_VIP) += sta2x11_vip.o
obj-$(CONFIG_VIDEO_TIMBERDALE) += timblogiw.o
+obj-$(CONFIG_VIDEO_STK1160) += stk1160/
obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o
obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o
diff --git a/drivers/media/video/stk1160/Kconfig
b/drivers/media/video/stk1160/Kconfig
new file mode 100644
index 000..7ae1685
--- /dev/null
+++ b/drivers/media/video/stk1160/Kconfig
@@ -0,0 +1,12 @@
+config VIDEO_STK1160
+ tristate STK1160 USB video capture support
+ depends on VIDEO_DEV I2C EASYCAP!=m EASYCAP!=y
Instead of it, why don't you just remove the EASYCAP driver?
Sure!
+ select VIDEOBUF2_VMALLOC
+ select VIDEO_SAA711X if VIDEO_HELPER_CHIPS_AUTO
+ select SND_AC97_CODEC
+
+ ---help---
+ This is a video4linux driver for STK1160 based video capture devices
+
+ To compile this driver as a module, choose M here: the
+ module will be called stk1160
diff --git a/drivers/media/video/stk1160/Makefile
b/drivers/media/video/stk1160/Makefile
new file mode 100644
index 000..5d8f1ba
--- /dev/null
+++ b/drivers/media/video/stk1160/Makefile
@@ -0,0 +1,6 @@
+stk1160-y := stk1160-core.o stk1160-v4l.o stk1160-video.o stk1160-i2c.o
stk1160-ac97.o
+
+obj-$(CONFIG_VIDEO_STK1160) += stk1160.o
+
+ccflags-y += -Wall
+ccflags-y += -Idrivers/media/video
diff --git a/drivers/media/video/stk1160/stk1160-ac97.c
b/drivers/media/video/stk1160/stk1160-ac97.c
new file mode 100644
index 000..529b05b
--- /dev/null
+++ b/drivers/media/video/stk1160/stk1160-ac97.c
@@ -0,0 +1,152 @@
+/*
+ * STK1160 driver
+ *
+ * Copyright (C) 2012 Ezequiel Garcia
+ * elezegarcia--a.t--gmail.com
+ *
+ * Based on Easycap driver by R.M. Thomas
+ * Copyright (C) 2010 R.M. Thomas
+ * rmthomas--a.t--sciolus.org
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include linux/module.h
+#include sound/core.h
+#include sound/initval.h
+#include sound/ac97_codec.h
+
+#include stk1160.h
+#include stk1160-reg.h
+
+static struct snd_ac97 *stk1160_ac97;
+
+static void stk1160_write_ac97(struct snd_ac97 *ac97, u16 reg, u16 value)
+{
+ struct stk1160 *dev = ac97-private_data;
+
+ /* Set codec register address */
+ stk1160_write_reg(dev, STK1160_AC97_ADDR, reg);
+
+ /* Set codec command */
+ stk1160_write_reg(dev, STK1160_AC97_CMD, value 0xff);
+ stk1160_write_reg(dev, STK1160_AC97_CMD + 1, (value 0xff00) 8);
+
+ /*
+ * Set command write bit to initiate write operation.
+ * The bit will be cleared when transfer is done.
+ */
+ stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c);
+}
+
+static u16 stk1160_read_ac97(struct snd_ac97 *ac97, u16 reg)
+{
+ struct stk1160 *dev = ac97-private_data;
+ u8 vall = 0;
+ u8 valh = 0;
+
+ /* Set codec register address */
+ stk1160_write_reg(dev, STK1160_AC97_ADDR, reg);
+
+ /*
+ * Set command read bit to initiate read operation.
+ * The bit will be cleared when transfer is done.
+ */
+ stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
+
+ /* Retrieve register value */
+ stk1160_read_reg(dev, STK1160_AC97_CMD, vall);
+ stk1160_read_reg(dev, STK1160_AC97_CMD + 1, valh);
+
+ return (valh 8) | vall;
+}
+
+static void stk1160_reset_ac97(struct snd_ac97 *ac97)
+{
+ struct stk1160 *dev = ac97-private_data;
+ /* Two-step reset AC97 interface and hardware codec */
+ stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x94);
+ stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x88);
+
+ /* Set 16-bit audio data and choose LR channel*/
Re: [PATCH v4] media: Add stk1160 new driver
On 07/06/2012 12:11 AM, Mauro Carvalho Chehab wrote:
+static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer
*p)
+{
+struct stk1160 *dev = video_drvdata(file);
+
+if (!stk1160_is_owner(dev, file))
+return -EBUSY;
+
+return vb2_dqbuf(dev-vb_vidq, p, file-f_flags O_NONBLOCK);
Why to use O_NONBLOCK here? it should be doing whatever userspace wants.
This is OK, since the third argument to vb2_dqbuf() is a boolean indicating
whether this call should be blocking or not. And a O_NONBLOCK masks this
information out from file-f_flags.
--
Regards,
Sylwester
--
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 v4] media: Add stk1160 new driver
Em 05-07-2012 19:36, Sylwester Nawrocki escreveu:
On 07/06/2012 12:11 AM, Mauro Carvalho Chehab wrote:
+static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer
*p)
+{
+ struct stk1160 *dev = video_drvdata(file);
+
+ if (!stk1160_is_owner(dev, file))
+ return -EBUSY;
+
+ return vb2_dqbuf(dev-vb_vidq, p, file-f_flags O_NONBLOCK);
Why to use O_NONBLOCK here? it should be doing whatever userspace wants.
This is OK, since the third argument to vb2_dqbuf() is a boolean indicating
whether this call should be blocking or not. And a O_NONBLOCK masks this
information out from file-f_flags.
Ah! OK then.
It might be better to initialize it during vb2 initialization, at open,
instead of requiring this argument every time vb_dqbuf() is called.
Btw, just noticed a minor issue: an space is required before the operator.
Regards,
Mauro
--
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
