Re: [PATCH] media: pvrusb2: Convert timers to use timer_setup()
Ack'ed (separate formal reply)
-Mike
On Wed, 25 Oct 2017, Kees Cook wrote:
> Eek, sorry, this uses timer_setup_on_stack() which is only in -next.
> If you can Ack this, I can carry it in the timer tree.
>
> Thanks!
>
> -Kees
>
> On Tue, Oct 24, 2017 at 5:22 PM, Kees Cook <keesc...@chromium.org> wrote:
> > In preparation for unconditionally passing the struct timer_list pointer to
> > all timer callbacks, switch to using the new timer_setup() and from_timer()
> > to pass the timer pointer explicitly.
> >
> > Cc: Mike Isely <is...@pobox.com>
> > Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> > Cc: linux-media@vger.kernel.org
> > Signed-off-by: Kees Cook <keesc...@chromium.org>
> > ---
> > drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 64
> > ++---
> > 1 file changed, 36 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > index ad5b25b89699..8289ee482f49 100644
> > --- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > +++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > @@ -330,10 +330,10 @@ static void pvr2_hdw_state_log_state(struct pvr2_hdw
> > *);
> > static int pvr2_hdw_cmd_usbstream(struct pvr2_hdw *hdw,int runFl);
> > static int pvr2_hdw_commit_setup(struct pvr2_hdw *hdw);
> > static int pvr2_hdw_get_eeprom_addr(struct pvr2_hdw *hdw);
> > -static void pvr2_hdw_quiescent_timeout(unsigned long);
> > -static void pvr2_hdw_decoder_stabilization_timeout(unsigned long);
> > -static void pvr2_hdw_encoder_wait_timeout(unsigned long);
> > -static void pvr2_hdw_encoder_run_timeout(unsigned long);
> > +static void pvr2_hdw_quiescent_timeout(struct timer_list *);
> > +static void pvr2_hdw_decoder_stabilization_timeout(struct timer_list *);
> > +static void pvr2_hdw_encoder_wait_timeout(struct timer_list *);
> > +static void pvr2_hdw_encoder_run_timeout(struct timer_list *);
> > static int pvr2_issue_simple_cmd(struct pvr2_hdw *,u32);
> > static int pvr2_send_request_ex(struct pvr2_hdw *hdw,
> > unsigned int timeout,int probe_fl,
> > @@ -2373,18 +2373,15 @@ struct pvr2_hdw *pvr2_hdw_create(struct
> > usb_interface *intf,
> > }
> > if (!hdw) goto fail;
> >
> > - setup_timer(>quiescent_timer, pvr2_hdw_quiescent_timeout,
> > - (unsigned long)hdw);
> > + timer_setup(>quiescent_timer, pvr2_hdw_quiescent_timeout, 0);
> >
> > - setup_timer(>decoder_stabilization_timer,
> > - pvr2_hdw_decoder_stabilization_timeout,
> > - (unsigned long)hdw);
> > + timer_setup(>decoder_stabilization_timer,
> > + pvr2_hdw_decoder_stabilization_timeout, 0);
> >
> > - setup_timer(>encoder_wait_timer, pvr2_hdw_encoder_wait_timeout,
> > - (unsigned long)hdw);
> > + timer_setup(>encoder_wait_timer, pvr2_hdw_encoder_wait_timeout,
> > + 0);
> >
> > - setup_timer(>encoder_run_timer, pvr2_hdw_encoder_run_timeout,
> > - (unsigned long)hdw);
> > + timer_setup(>encoder_run_timer, pvr2_hdw_encoder_run_timeout,
> > 0);
> >
> > hdw->master_state = PVR2_STATE_DEAD;
> >
> > @@ -3539,10 +3536,16 @@ static void pvr2_ctl_read_complete(struct urb *urb)
> > complete(>ctl_done);
> > }
> >
> > +struct hdw_timer {
> > + struct timer_list timer;
> > + struct pvr2_hdw *hdw;
> > +};
> >
> > -static void pvr2_ctl_timeout(unsigned long data)
> > +static void pvr2_ctl_timeout(struct timer_list *t)
> > {
> > - struct pvr2_hdw *hdw = (struct pvr2_hdw *)data;
> > + struct hdw_timer *timer = from_timer(timer, t, timer);
> > + struct pvr2_hdw *hdw = timer->hdw;
> > +
> > if (hdw->ctl_write_pend_flag || hdw->ctl_read_pend_flag) {
> > hdw->ctl_timeout_flag = !0;
> > if (hdw->ctl_write_pend_flag)
> > @@ -3564,7 +3567,10 @@ static int pvr2_send_request_ex(struct pvr2_hdw *hdw,
> > {
> > unsigned int idx;
> > int status = 0;
> > - struct timer_list timer;
> > + struct hdw_timer timer = {
> > + .hdw = hdw,
> > + };
> > +
> > if (!hdw->ctl_lock_held) {
> > pvr2_trace(PVR2_TRACE_ERROR_LEGS,
> >"Atte
Re: [PATCH] media: pvrusb2: Convert timers to use timer_setup()
Acked-By: Mike Isely <is...@pobox.com>
On Tue, 24 Oct 2017, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
>
> Cc: Mike Isely <is...@pobox.com>
> Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Kees Cook <keesc...@chromium.org>
> ---
> drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 64
> ++---
> 1 file changed, 36 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> index ad5b25b89699..8289ee482f49 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> @@ -330,10 +330,10 @@ static void pvr2_hdw_state_log_state(struct pvr2_hdw *);
> static int pvr2_hdw_cmd_usbstream(struct pvr2_hdw *hdw,int runFl);
> static int pvr2_hdw_commit_setup(struct pvr2_hdw *hdw);
> static int pvr2_hdw_get_eeprom_addr(struct pvr2_hdw *hdw);
> -static void pvr2_hdw_quiescent_timeout(unsigned long);
> -static void pvr2_hdw_decoder_stabilization_timeout(unsigned long);
> -static void pvr2_hdw_encoder_wait_timeout(unsigned long);
> -static void pvr2_hdw_encoder_run_timeout(unsigned long);
> +static void pvr2_hdw_quiescent_timeout(struct timer_list *);
> +static void pvr2_hdw_decoder_stabilization_timeout(struct timer_list *);
> +static void pvr2_hdw_encoder_wait_timeout(struct timer_list *);
> +static void pvr2_hdw_encoder_run_timeout(struct timer_list *);
> static int pvr2_issue_simple_cmd(struct pvr2_hdw *,u32);
> static int pvr2_send_request_ex(struct pvr2_hdw *hdw,
> unsigned int timeout,int probe_fl,
> @@ -2373,18 +2373,15 @@ struct pvr2_hdw *pvr2_hdw_create(struct usb_interface
> *intf,
> }
> if (!hdw) goto fail;
>
> - setup_timer(>quiescent_timer, pvr2_hdw_quiescent_timeout,
> - (unsigned long)hdw);
> + timer_setup(>quiescent_timer, pvr2_hdw_quiescent_timeout, 0);
>
> - setup_timer(>decoder_stabilization_timer,
> - pvr2_hdw_decoder_stabilization_timeout,
> - (unsigned long)hdw);
> + timer_setup(>decoder_stabilization_timer,
> + pvr2_hdw_decoder_stabilization_timeout, 0);
>
> - setup_timer(>encoder_wait_timer, pvr2_hdw_encoder_wait_timeout,
> - (unsigned long)hdw);
> + timer_setup(>encoder_wait_timer, pvr2_hdw_encoder_wait_timeout,
> + 0);
>
> - setup_timer(>encoder_run_timer, pvr2_hdw_encoder_run_timeout,
> - (unsigned long)hdw);
> + timer_setup(>encoder_run_timer, pvr2_hdw_encoder_run_timeout, 0);
>
> hdw->master_state = PVR2_STATE_DEAD;
>
> @@ -3539,10 +3536,16 @@ static void pvr2_ctl_read_complete(struct urb *urb)
> complete(>ctl_done);
> }
>
> +struct hdw_timer {
> + struct timer_list timer;
> + struct pvr2_hdw *hdw;
> +};
>
> -static void pvr2_ctl_timeout(unsigned long data)
> +static void pvr2_ctl_timeout(struct timer_list *t)
> {
> - struct pvr2_hdw *hdw = (struct pvr2_hdw *)data;
> + struct hdw_timer *timer = from_timer(timer, t, timer);
> + struct pvr2_hdw *hdw = timer->hdw;
> +
> if (hdw->ctl_write_pend_flag || hdw->ctl_read_pend_flag) {
> hdw->ctl_timeout_flag = !0;
> if (hdw->ctl_write_pend_flag)
> @@ -3564,7 +3567,10 @@ static int pvr2_send_request_ex(struct pvr2_hdw *hdw,
> {
> unsigned int idx;
> int status = 0;
> - struct timer_list timer;
> + struct hdw_timer timer = {
> + .hdw = hdw,
> + };
> +
> if (!hdw->ctl_lock_held) {
> pvr2_trace(PVR2_TRACE_ERROR_LEGS,
> "Attempted to execute control transfer without
> lock!!");
> @@ -3621,8 +3627,8 @@ static int pvr2_send_request_ex(struct pvr2_hdw *hdw,
> hdw->ctl_timeout_flag = 0;
> hdw->ctl_write_pend_flag = 0;
> hdw->ctl_read_pend_flag = 0;
> - setup_timer(, pvr2_ctl_timeout, (unsigned long)hdw);
> - timer.expires = jiffies + timeout;
> + timer_setup_on_stack(, pvr2_ctl_timeout, 0);
> + timer.timer.expires = jiffies + timeout;
>
> if (write_len && write_data) {
> hdw->cmd_debug_state = 2;
> @@ -3677,7 +3683,7 @@ status);
> }
>
> /* Start timer */
> - add_timer();
> + add_timer();
>
> /* Now wait for all I/O to complete */
Re: [PATCH 05/24] media: v4l2-dev: convert VFL_TYPE_* into an enum
Acked-By: Mike Isely <is...@pobox.com>
On Mon, 9 Oct 2017, Mauro Carvalho Chehab wrote:
> Using enums makes easier to document, as it can use kernel-doc
> markups. It also allows cross-referencing, with increases the
> kAPI readability.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> Documentation/media/kapi/v4l2-dev.rst | 17 ++---
> drivers/media/pci/cx88/cx88-blackbird.c | 3 +-
> drivers/media/pci/cx88/cx88-video.c | 10 +++---
> drivers/media/pci/cx88/cx88.h | 4 +--
> drivers/media/pci/saa7134/saa7134-video.c | 2 ++
> drivers/media/usb/cx231xx/cx231xx-video.c | 2 ++
> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 2 ++
> drivers/media/usb/tm6000/tm6000-video.c | 2 ++
> drivers/media/v4l2-core/v4l2-dev.c| 10 +++---
> include/media/v4l2-dev.h | 59
> +--
> include/media/v4l2-mediabus.h | 30
> 11 files changed, 98 insertions(+), 43 deletions(-)
>
> diff --git a/Documentation/media/kapi/v4l2-dev.rst
> b/Documentation/media/kapi/v4l2-dev.rst
> index b29aa616c267..7bb0505b60f1 100644
> --- a/Documentation/media/kapi/v4l2-dev.rst
> +++ b/Documentation/media/kapi/v4l2-dev.rst
> @@ -196,11 +196,18 @@ device.
> Which device is registered depends on the type argument. The following
> types exist:
>
> -- ``VFL_TYPE_GRABBER``: ``/dev/videoX`` for video input/output devices
> -- ``VFL_TYPE_VBI``: ``/dev/vbiX`` for vertical blank data (i.e. closed
> captions, teletext)
> -- ``VFL_TYPE_RADIO``: ``/dev/radioX`` for radio tuners
> -- ``VFL_TYPE_SDR``: ``/dev/swradioX`` for Software Defined Radio tuners
> -- ``VFL_TYPE_TOUCH``: ``/dev/v4l-touchX`` for touch sensors
> +==
> ==
> +:c:type:`vfl_devnode_type` Device nameUsage
> +==
> ==
> +``VFL_TYPE_GRABBER`` ``/dev/videoX`` for video input/output
> devices
> +``VFL_TYPE_VBI`` ``/dev/vbiX`` for vertical blank data
> (i.e.
> + closed captions, teletext)
> +``VFL_TYPE_RADIO`` ``/dev/radioX`` for radio tuners
> +``VFL_TYPE_SUBDEV````/dev/v4l-subdevX`` for V4L2 subdevices
> +``VFL_TYPE_SDR`` ``/dev/swradioX`` for Software Defined Radio
> + (SDR) tuners
> +``VFL_TYPE_TOUCH`` ``/dev/v4l-touchX`` for touch sensors
> +==
> ==
>
> The last argument gives you a certain amount of control over the device
> device node number used (i.e. the X in ``videoX``). Normally you will pass -1
> diff --git a/drivers/media/pci/cx88/cx88-blackbird.c
> b/drivers/media/pci/cx88/cx88-blackbird.c
> index e3101f04941c..0e0952e60795 100644
> --- a/drivers/media/pci/cx88/cx88-blackbird.c
> +++ b/drivers/media/pci/cx88/cx88-blackbird.c
> @@ -805,8 +805,7 @@ static int vidioc_querycap(struct file *file, void *priv,
>
> strcpy(cap->driver, "cx88_blackbird");
> sprintf(cap->bus_info, "PCI:%s", pci_name(dev->pci));
> - cx88_querycap(file, core, cap);
> - return 0;
> + return cx88_querycap(file, core, cap);
> }
>
> static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
> diff --git a/drivers/media/pci/cx88/cx88-video.c
> b/drivers/media/pci/cx88/cx88-video.c
> index 7d25ecd4404b..9be682cdb644 100644
> --- a/drivers/media/pci/cx88/cx88-video.c
> +++ b/drivers/media/pci/cx88/cx88-video.c
> @@ -806,8 +806,8 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void
> *priv,
> return 0;
> }
>
> -void cx88_querycap(struct file *file, struct cx88_core *core,
> -struct v4l2_capability *cap)
> +int cx88_querycap(struct file *file, struct cx88_core *core,
> + struct v4l2_capability *cap)
> {
> struct video_device *vdev = video_devdata(file);
>
> @@ -825,11 +825,14 @@ void cx88_querycap(struct file *file, struct cx88_core
> *core,
> case VFL_TYPE_VBI:
> cap->device_caps |= V4L2_CAP_VBI_CAPTURE;
> break;
> + default:
> + return -EINVAL;
> }
> cap->capabilities = cap->device_caps | V4L2_CAP_VIDEO_CAPTURE |
> V4L2_CAP_VBI_CAPTURE | V4L2_CAP_DEVICE_CAPS;
> if (core->board.radio.type == CX88_RADIO)
> cap->capabilities |= V4L2_CAP_RADIO;
> + return 0;
> }
> EXPORT_SYMBOL(cx88_querycap);
>
Re: usb/media/pvrusb2: warning in pvr2_send_request_ex/usb_submit_urb
What you have here is way beyond just feeding random crap in via the syscall interface. To cause this you have to fake the presence of a pvrusb2 compatible *hardware* USB device and then lie about its endpoint configuration. Is that really a concern here? Are we now saying that any kernel driver which talks via USB must now also specifically verify the exact expected USB endpoint configuration? Where does that end? How about the vendor-specific RPC protocol that the hardware actually implements over the bulk endpoint? It's likely that the pvrusb2 driver may be making assumptions about the expected responses over that protocol. Please realize that I'm not dismissing this. I can see some merit in this. But I'm just a bit surprised that now we're going this far. Is this really the intention? You're talking about code (pvrusb2_send_request_ex()) that hasn't changed in about 10 years. With this level of paranoia there's got to be a pretty target-rich environment over the set of kernel-supported USB devices. To take this another step, wouldn't that same level of paranoia be a concern for any externally connected PCI-Express device? Because that's another external way into the computer that involves very non-trivial and very hardware-centric protocols. Thunderbolt devices would be an example of this. -Mike On Wed, 20 Sep 2017, Andrey Konovalov wrote: > Hi! > > I've got the following report while fuzzing the kernel with syzkaller. > > On commit ebb2c2437d8008d46796902ff390653822af6cc4 (Sep 18). > > There seems to be no check on endpoint type before submitting bulk urb > in pvr2_send_request_ex(). > > usb 1-1: New USB device found, idVendor=2040, idProduct=7500 > usb 1-1: New USB device strings: Mfr=0, Product=255, SerialNumber=0 > usb 1-1: Product: a > gadgetfs: configuration #6 > pvrusb2: Hardware description: WinTV HVR-1950 Model 750xx > usb 1-1: BOGUS urb xfer, pipe 3 != type 1 > [ cut here ] > WARNING: CPU: 1 PID: 2713 at drivers/usb/core/urb.c:449 > usb_submit_urb+0xf8a/0x11d0 > Modules linked in: > CPU: 1 PID: 2713 Comm: pvrusb2-context Not tainted > 4.14.0-rc1-42251-gebb2c2437d80 #210 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > task: 88006b7a18c0 task.stack: 880069978000 > RIP: 0010:usb_submit_urb+0xf8a/0x11d0 drivers/usb/core/urb.c:448 > RSP: 0018:88006997f990 EFLAGS: 00010286 > RAX: 0029 RBX: 880063661900 RCX: > RDX: 0029 RSI: 86876d60 RDI: ed000d32ff24 > RBP: 88006997fa90 R08: 11000d32fdca R09: > R10: R11: R12: 11000d32ff39 > R13: 0001 R14: 0003 R15: 880068bbed68 > FS: () GS:88006c60() knlGS: > CS: 0010 DS: ES: CR0: 80050033 > CR2: 01032000 CR3: 6a0ff000 CR4: 06f0 > Call Trace: > pvr2_send_request_ex+0xa57/0x1d80 > drivers/media/usb/pvrusb2/pvrusb2-hdw.c:3645 > pvr2_hdw_check_firmware drivers/media/usb/pvrusb2/pvrusb2-hdw.c:1812 > pvr2_hdw_setup_low drivers/media/usb/pvrusb2/pvrusb2-hdw.c:2107 > pvr2_hdw_setup drivers/media/usb/pvrusb2/pvrusb2-hdw.c:2250 > pvr2_hdw_initialize+0x548/0x3c10 drivers/media/usb/pvrusb2/pvrusb2-hdw.c:2327 > pvr2_context_check drivers/media/usb/pvrusb2/pvrusb2-context.c:118 > pvr2_context_thread_func+0x361/0x8c0 > drivers/media/usb/pvrusb2/pvrusb2-context.c:167 > kthread+0x3a1/0x470 kernel/kthread.c:231 > ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431 > Code: 48 8b 85 30 ff ff ff 48 8d b8 98 00 00 00 e8 ee 82 89 fe 45 89 > e8 44 89 f1 4c 89 fa 48 89 c6 48 c7 c7 40 c0 ea 86 e8 30 1b dc fc <0f> > ff e9 9b f7 ff ff e8 aa 95 25 fd e9 80 f7 ff ff e8 50 74 f3 > ---[ end trace 6919030503719da6 ]--- > -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
Re: [PATCH v2 5/6] media/usb/pvrusb2: Support for V4L2_CTRL_WHICH_DEF_VAL
Looks good to me (still), including now the change I had previously
suggested. For the record, the ack still applies. (I guess you can
consider this to be an ack of the ack...)
-Mike
On Thu, 29 Oct 2015, Ricardo Ribalda Delgado wrote:
> This driver does not use the control infrastructure.
> Add support for the new field which on structure
> v4l2_ext_controls
>
> Acked-by: Mike Isely <is...@pobox.com>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.riba...@gmail.com>
> ---
> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> index 1c5f85bf7ed4..81f788b7b242 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> @@ -628,6 +628,7 @@ static int pvr2_g_ext_ctrls(struct file *file, void *priv,
> struct pvr2_v4l2_fh *fh = file->private_data;
> struct pvr2_hdw *hdw = fh->channel.mc_head->hdw;
> struct v4l2_ext_control *ctrl;
> + struct pvr2_ctrl *cptr;
> unsigned int idx;
> int val;
> int ret;
> @@ -635,8 +636,15 @@ static int pvr2_g_ext_ctrls(struct file *file, void
> *priv,
> ret = 0;
> for (idx = 0; idx < ctls->count; idx++) {
> ctrl = ctls->controls + idx;
> - ret = pvr2_ctrl_get_value(
> - pvr2_hdw_get_ctrl_v4l(hdw, ctrl->id), );
> + cptr = pvr2_hdw_get_ctrl_v4l(hdw, ctrl->id);
> + if (cptr) {
> + if (ctls->which == V4L2_CTRL_WHICH_DEF_VAL)
> + pvr2_ctrl_get_def(cptr, );
> + else
> + ret = pvr2_ctrl_get_value(cptr, );
> + } else
> + ret = -EINVAL;
> +
> if (ret) {
> ctls->error_idx = idx;
> return ret;
> @@ -658,6 +666,10 @@ static int pvr2_s_ext_ctrls(struct file *file, void
> *priv,
> unsigned int idx;
> int ret;
>
> + /* Default value cannot be changed */
> + if (ctls->which == V4L2_CTRL_WHICH_DEF_VAL)
> + return -EINVAL;
> +
> ret = 0;
> for (idx = 0; idx < ctls->count; idx++) {
> ctrl = ctls->controls + idx;
>
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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 v2 07/10] media/usb/pvrusb2: Support for V4L2_CTRL_WHICH_DEF_VAL
The code you've added is carefully checking the return pointer from
pvr2_hdw_get_ctrl_v4l() yet the original code did not operate this way.
The result is that now there's this unbalanced effect where it appears
that the validity of the pvr2_ctrl instance is only checked on one side
of the if-statement. I would recommend instead to elevate the call to
pvr2_hdw_get_ctrl_v4l() out of the if-statement - since in both cases
it's being called the same way both times. Then do the validity check
in that one spot and that simplifies the if-statement all the way down
to choosing between pvr2_ctrl_get_value() vs pvr2_ctrl_get_def().
It's not a correctness comment; what you have should work fine. So I'm
ack'ing this in any case:
Acked-By: Mike Isely is...@pobox.com
But you can do the above pretty easily safely, and simplify it a bit
further.
-Mike
On Fri, 21 Aug 2015, Ricardo Ribalda Delgado wrote:
This driver does not use the control infrastructure.
Add support for the new field which on structure
v4l2_ext_controls
Signed-off-by: Ricardo Ribalda Delgado ricardo.riba...@gmail.com
---
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
index 1c5f85bf7ed4..43b2f2214798 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
@@ -628,6 +628,7 @@ static int pvr2_g_ext_ctrls(struct file *file, void *priv,
struct pvr2_v4l2_fh *fh = file-private_data;
struct pvr2_hdw *hdw = fh-channel.mc_head-hdw;
struct v4l2_ext_control *ctrl;
+ struct pvr2_ctrl *cptr;
unsigned int idx;
int val;
int ret;
@@ -635,8 +636,18 @@ static int pvr2_g_ext_ctrls(struct file *file, void
*priv,
ret = 0;
for (idx = 0; idx ctls-count; idx++) {
ctrl = ctls-controls + idx;
- ret = pvr2_ctrl_get_value(
+ if (ctls-which == V4L2_CTRL_WHICH_DEF_VAL) {
+ cptr = pvr2_hdw_get_ctrl_v4l(hdw, ctrl-id);
+ if (cptr)
+ pvr2_ctrl_get_def(cptr, val);
+ else
+ ret = -EINVAL;
+
+
+ } else
+ ret = pvr2_ctrl_get_value(
pvr2_hdw_get_ctrl_v4l(hdw, ctrl-id), val);
+
if (ret) {
ctls-error_idx = idx;
return ret;
@@ -658,6 +669,10 @@ static int pvr2_s_ext_ctrls(struct file *file, void
*priv,
unsigned int idx;
int ret;
+ /* Default value cannot be changed */
+ if (ctls-which == V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
ret = 0;
for (idx = 0; idx ctls-count; idx++) {
ctrl = ctls-controls + idx;
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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] [media] [pvrusb2]: remove dead retry cmd code
Sorry been asleep at the wheel here. I'll take a look.
Please realize that the code path being talked about here HAS worked -
because the encoder does tend to fail and this is how the driver
recovers.
-Mike
On Fri, 16 Jan 2015, Hans Verkuil wrote:
On 01/16/2015 12:29 PM, Haim Daniel wrote:
It looks that if (try_count 20) continue jumps to end of the do ...
while(0) loop and goes out.
Ah, you are right. But that is obviously not what was intended, so just
removing
it is not a proper 'fix'.
Mike, can you take a look at this?
Regards,
Hans
--hd.
On Fri, 2015-01-16 at 11:57 +0100, Hans Verkuil wrote:
On 01/05/2015 11:38 PM, Haim Daniel wrote:
In case a command is timed out, current flow sets the retry_flag
and does nothing.
Really? That's not how I read the code: it retries up to 20 times before
bailing out.
Perhaps you missed the if (try_count 20) continue; line?
Regards,
Hans
Signed-off-by: Haim Daniel haim.dan...@gmail.com
---
drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 15 +--
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
index f7702ae..02028aa 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
@@ -145,8 +145,6 @@ static int pvr2_encoder_cmd(void *ctxt,
u32 *argp)
{
unsigned int poll_count;
- unsigned int try_count = 0;
- int retry_flag;
int ret = 0;
unsigned int idx;
/* These sizes look to be limited by the FX2 firmware implementation */
@@ -213,8 +211,6 @@ static int pvr2_encoder_cmd(void *ctxt,
break;
}
- retry_flag = 0;
- try_count++;
ret = 0;
wrData[0] = 0;
wrData[1] = cmd;
@@ -245,11 +241,9 @@ static int pvr2_encoder_cmd(void *ctxt,
}
if (rdData[0] (poll_count 1000)) continue;
if (!rdData[0]) {
- retry_flag = !0;
pvr2_trace(
PVR2_TRACE_ERROR_LEGS,
- Encoder timed out waiting for us
- ; arranging to retry);
+ Encoder timed out waiting for us);
} else {
pvr2_trace(
PVR2_TRACE_ERROR_LEGS,
@@ -269,13 +263,6 @@ static int pvr2_encoder_cmd(void *ctxt,
ret = -EBUSY;
break;
}
- if (retry_flag) {
- if (try_count 20) continue;
- pvr2_trace(
- PVR2_TRACE_ERROR_LEGS,
- Too many retries...);
- ret = -EBUSY;
- }
if (ret) {
del_timer_sync(hdw-encoder_run_timer);
hdw-state_encoder_ok = 0;
--
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
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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 02/14] pvrusb2: fix sparse warning
Acked-by: Mike Isely is...@pobox.com
-Mike
On Fri, 4 Oct 2013, Hans Verkuil wrote:
From: Hans Verkuil hans.verk...@cisco.com
drivers/media/usb/pvrusb2/pvrusb2-hdw.c:2871:13: warning: symbol
'pvr2_hdw_get_detected_std' was not declared. Should it be static?
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
Cc: Mike Isely is...@pobox.com
---
drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
index c4d51d7..ea05f67 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
@@ -2868,7 +2868,7 @@ static void pvr2_subdev_set_control(struct pvr2_hdw
*hdw, int id,
pvr2_subdev_set_control(hdw, id, #lab, (hdw)-lab##_val); \
}
-v4l2_std_id pvr2_hdw_get_detected_std(struct pvr2_hdw *hdw)
+static v4l2_std_id pvr2_hdw_get_detected_std(struct pvr2_hdw *hdw)
{
v4l2_std_id std;
std = (v4l2_std_id)hdw-std_mask_avail;
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: [REVIEWv2 PATCH 07/12] pvrusb2: use v4l2_dev instead of the deprecated parent field.
Acked-By: Mike Isely is...@pobox.com
-Mike
On Wed, 12 Jun 2013, Hans Verkuil wrote:
From: Hans Verkuil hans.verk...@cisco.com
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
---
drivers/media/usb/pvrusb2/pvrusb2-hdw.c |4
drivers/media/usb/pvrusb2/pvrusb2-hdw.h |4
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c |7 ---
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
index d329209..c4d51d7 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
@@ -2704,6 +2704,10 @@ static void pvr2_hdw_remove_usb_stuff(struct pvr2_hdw
*hdw)
pvr2_hdw_render_useless(hdw);
}
+void pvr2_hdw_set_v4l2_dev(struct pvr2_hdw *hdw, struct video_device *vdev)
+{
+ vdev-v4l2_dev = hdw-v4l2_dev;
+}
/* Destroy hardware interaction structure */
void pvr2_hdw_destroy(struct pvr2_hdw *hdw)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.h
b/drivers/media/usb/pvrusb2/pvrusb2-hdw.h
index 1a135cf..4184707 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.h
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.h
@@ -22,6 +22,7 @@
#include linux/usb.h
#include linux/videodev2.h
+#include media/v4l2-dev.h
#include pvrusb2-io.h
#include pvrusb2-ctrl.h
@@ -138,6 +139,9 @@ const char *pvr2_hdw_get_device_identifier(struct
pvr2_hdw *);
/* Called when hardware has been unplugged */
void pvr2_hdw_disconnect(struct pvr2_hdw *);
+/* Sets v4l2_dev of a video_device struct */
+void pvr2_hdw_set_v4l2_dev(struct pvr2_hdw *, struct video_device *);
+
/* Get the number of defined controls */
unsigned int pvr2_hdw_get_ctrl_count(struct pvr2_hdw *);
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
index 82f619b..d77069e 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
@@ -31,6 +31,7 @@
#include linux/videodev2.h
#include linux/module.h
#include media/v4l2-dev.h
+#include media/v4l2-device.h
#include media/v4l2-common.h
#include media/v4l2-ioctl.h
@@ -870,8 +871,8 @@ static void pvr2_v4l2_dev_destroy(struct pvr2_v4l2_dev
*dip)
static void pvr2_v4l2_dev_disassociate_parent(struct pvr2_v4l2_dev *dip)
{
if (!dip) return;
- if (!dip-devbase.parent) return;
- dip-devbase.parent = NULL;
+ if (!dip-devbase.v4l2_dev-dev) return;
+ dip-devbase.v4l2_dev-dev = NULL;
device_move(dip-devbase.dev, NULL, DPM_ORDER_NONE);
}
@@ -1321,7 +1322,7 @@ static void pvr2_v4l2_dev_init(struct pvr2_v4l2_dev
*dip,
if (nr_ptr (unit_number = 0) (unit_number PVR_NUM)) {
mindevnum = nr_ptr[unit_number];
}
- dip-devbase.parent = usbdev-dev;
+ pvr2_hdw_set_v4l2_dev(hdw, dip-devbase);
if ((video_register_device(dip-devbase,
dip-v4l_type, mindevnum) 0)
(video_register_device(dip-devbase,
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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 33/68] [media] pvrusb2: get rid of warning: no previous prototype
Acked-By: Mike Isely is...@pobox.com
On Sat, 27 Oct 2012, Mauro Carvalho Chehab wrote:
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c:199:5: warning: no previous
prototype for 'pvr2_s_std' [-Wmissing-prototypes]
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c:368:5: warning: no previous
prototype for 'pvr2_s_frequency' [-Wmissing-prototypes]
Cc: Mike Isely is...@pobox.com
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
---
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
index db249ca..6930676 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
@@ -196,7 +196,7 @@ static int pvr2_g_std(struct file *file, void *priv,
v4l2_std_id *std)
return ret;
}
-int pvr2_s_std(struct file *file, void *priv, v4l2_std_id *std)
+static int pvr2_s_std(struct file *file, void *priv, v4l2_std_id *std)
{
struct pvr2_v4l2_fh *fh = file-private_data;
struct pvr2_hdw *hdw = fh-channel.mc_head-hdw;
@@ -365,7 +365,7 @@ static int pvr2_s_tuner(struct file *file, void *priv,
struct v4l2_tuner *vt)
vt-audmode);
}
-int pvr2_s_frequency(struct file *file, void *priv, struct v4l2_frequency
*vf)
+static int pvr2_s_frequency(struct file *file, void *priv, struct
v4l2_frequency *vf)
{
struct pvr2_v4l2_fh *fh = file-private_data;
struct pvr2_hdw *hdw = fh-channel.mc_head-hdw;
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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] pvr2: fix minor storage
Completely agree! Thanks for spotting that one.
Signed-off-by: Mike Isely is...@pobox.com
-Mike
On Thu, 25 Oct 2012, Alan Cox wrote:
From: Alan Cox a...@linux.intel.com
This should have break statements in it.
Signed-off-by: Alan Cox a...@linux.intel.com
---
drivers/media/usb/pvrusb2/pvrusb2-hdw.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
index fb828ba..299751a 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
@@ -3563,9 +3563,9 @@ void pvr2_hdw_v4l_store_minor_number(struct pvr2_hdw
*hdw,
enum pvr2_v4l_type index,int v)
{
switch (index) {
- case pvr2_v4l_type_video: hdw-v4l_minor_number_video = v;
- case pvr2_v4l_type_vbi: hdw-v4l_minor_number_vbi = v;
- case pvr2_v4l_type_radio: hdw-v4l_minor_number_radio = v;
+ case pvr2_v4l_type_video: hdw-v4l_minor_number_video = v;break;
+ case pvr2_v4l_type_vbi: hdw-v4l_minor_number_vbi = v;break;
+ case pvr2_v4l_type_radio: hdw-v4l_minor_number_radio = v;break;
default: break;
}
}
--
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
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: RFC: Core + Radio profile
On Wed, 22 Aug 2012, Mauro Carvalho Chehab wrote: Em 22-08-2012 07:11, Hans Verkuil escreveu: I've added some more core profile requirements. Streaming I/O is not supported by radio nodes. Hmm... pvrusb2/ivtv? Ok, it makes sense to move it to use the alsa mpeg API there. If we're enforcing it, we should deprecate the current way there, and make it use ALSA. I am unaware of any ALSA MPEG API. It's entirely likely that this is because I haven't been paying attention. Nevertheless, can you please point me at any documentation on this so I can get up to speed? Currently the pvrusb2 driver does not attempt to perform any processing or filtering of the data stream, so radio data is just the same mpeg stream as video (but without any real embedded video data). If I have to get into the business of processing the MPEG data in order to adhere to this proposal, then that will be a very big deal for this driver. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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] pvrusb2: Declare MODULE_FIRMWARE usage
Acked-By: Mike Isely is...@pobox.com
-Mike
On Thu, 26 Jul 2012, Tim Gardner wrote:
Cc: Mike Isely is...@pobox.com
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: linux-media@vger.kernel.org
Signed-off-by: Tim Gardner tim.gard...@canonical.com
---
drivers/media/video/pvrusb2/pvrusb2-devattr.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/media/video/pvrusb2/pvrusb2-devattr.c
b/drivers/media/video/pvrusb2/pvrusb2-devattr.c
index d8c8982..adc501d3 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-devattr.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-devattr.c
@@ -54,8 +54,9 @@ static const struct pvr2_device_client_desc
pvr2_cli_29xxx[] = {
{ .module_id = PVR2_CLIENT_ID_DEMOD },
};
+#define PVR2_FIRMWARE_29xxx v4l-pvrusb2-29xxx-01.fw
static const char *pvr2_fw1_names_29xxx[] = {
- v4l-pvrusb2-29xxx-01.fw,
+ PVR2_FIRMWARE_29xxx,
};
static const struct pvr2_device_desc pvr2_device_29xxx = {
@@ -87,8 +88,9 @@ static const struct pvr2_device_client_desc
pvr2_cli_24xxx[] = {
{ .module_id = PVR2_CLIENT_ID_DEMOD },
};
+#define PVR2_FIRMWARE_24xxx v4l-pvrusb2-24xxx-01.fw
static const char *pvr2_fw1_names_24xxx[] = {
- v4l-pvrusb2-24xxx-01.fw,
+ PVR2_FIRMWARE_24xxx,
};
static const struct pvr2_device_desc pvr2_device_24xxx = {
@@ -369,8 +371,9 @@ static const struct pvr2_device_client_desc
pvr2_cli_73xxx[] = {
.i2c_address_list = \x42},
};
+#define PVR2_FIRMWARE_73xxx v4l-pvrusb2-73xxx-01.fw
static const char *pvr2_fw1_names_73xxx[] = {
- v4l-pvrusb2-73xxx-01.fw,
+ PVR2_FIRMWARE_73xxx,
};
static const struct pvr2_device_desc pvr2_device_73xxx = {
@@ -475,8 +478,9 @@ static const struct pvr2_dvb_props pvr2_751xx_dvb_props =
{
};
#endif
+#define PVR2_FIRMWARE_75xxx v4l-pvrusb2-73xxx-01.fw
static const char *pvr2_fw1_names_75xxx[] = {
- v4l-pvrusb2-73xxx-01.fw,
+ PVR2_FIRMWARE_75xxx,
};
static const struct pvr2_device_desc pvr2_device_750xx = {
@@ -556,7 +560,10 @@ struct usb_device_id pvr2_device_table[] = {
};
MODULE_DEVICE_TABLE(usb, pvr2_device_table);
-
+MODULE_FIRMWARE(PVR2_FIRMWARE_29xxx);
+MODULE_FIRMWARE(PVR2_FIRMWARE_24xxx);
+MODULE_FIRMWARE(PVR2_FIRMWARE_73xxx);
+MODULE_FIRMWARE(PVR2_FIRMWARE_75xxx);
/*
Stuff for Emacs to see, in order to encourage consistent editing style:
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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
[GIT PULL FOR 3.5] pvrusb2 driver updates
Mauro: Please pull - this includes a long-awaited change courtesy of Hans Verkuil which finally transitions the driver to video_ioctl2. -Mike Isely The following changes since commit a1ac5dc28d2b4ca78e183229f7c595ffd725241c: [media] gspca - sn9c20x: Change the exposure setting of Omnivision sensors (2012-05-03 15:29:56 -0300) are available in the git repository at: git://git.linuxtv.org/mcisely/pvrusb2-20120504.git pvrusb2-merge-20120504 Hans Verkuil (1): pvrusb2: convert to video_ioctl2 Mike Isely (9): pvrusb2: Stop statically initializing reserved struct fields to zero pvrusb2: Clean up pvr2_hdw_get_detected_std() pvrusb2: Implement querystd for videodev_ioctl2 pvrusb2: Transform video standard detection result into read-only control ID pvrusb2: Fix truncated video standard names (trivial) pvrusb2: Base available video standards on what hardware supports pvrusb2: Trivial tweak to get rid of some redundant dereferences pvrusb2: Get rid of obsolete code for video standard enumeration pvrusb2: For querystd, start with list of hardware-supported standards drivers/media/video/pvrusb2/pvrusb2-hdw-internal.h |6 +- drivers/media/video/pvrusb2/pvrusb2-hdw.c | 193 +--- drivers/media/video/pvrusb2/pvrusb2-hdw.h |9 +- drivers/media/video/pvrusb2/pvrusb2-v4l2.c | 1343 ++-- 4 files changed, 735 insertions(+), 816 deletions(-) -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [PATCHv2 5/8] [media] pvrusb2: initialize standards mask before detecting standard
Mauro:
With the line you've just added, then the = arg assignment in the
immediate prior line is effectively dead code. Try this instead:
case VIDIOC_QUERYSTD:
{
- v4l2_std_id *std = arg;
+ v4l2_std_id *std = V4L2_STD_ALL;
ret = pvr2_hdw_get_detected_std(hdw, std);
break;
}
-Mike
On Tue, 4 Oct 2011, Mauro Carvalho Chehab wrote:
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
---
drivers/media/video/pvrusb2/pvrusb2-v4l2.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
b/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
index 0d029da..ce7ac45 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
@@ -230,6 +230,7 @@ static long pvr2_v4l2_do_ioctl(struct file *file,
unsigned int cmd, void *arg)
case VIDIOC_QUERYSTD:
{
v4l2_std_id *std = arg;
+ *std = V4L2_STD_ALL;
ret = pvr2_hdw_get_detected_std(hdw, std);
break;
}
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: [PATCHv2 5/8] [media] pvrusb2: initialize standards mask before detecting standard
On Wed, 5 Oct 2011, Mauro Carvalho Chehab wrote:
Em 05-10-2011 11:00, Mike Isely escreveu:
Mauro:
With the line you've just added, then the = arg assignment in the
immediate prior line is effectively dead code. Try this instead:
Look better:
v4l2_std_id *std = arg;
+ *std = V4L2_STD_ALL;
The above code is creating a pointer 'std' of the type 'v4l2_std_id', and
initializing the pointer with the void *arg.
Oh yeah, you're absolutely right. I got visually tricked by the well
known confusing C initialization syntax when dealing with pointers!
I've got to not respond to stuff like this in the morning until I've
finished waking up. Duh...
Then, it is doing an indirect reference to the pointer, filling its
contents with V4L2_STD_ALL value.
The code above is sane (and, btw, it works). After those patches, the
detection code will detect PAL/M or NTSC/M depending on the channel I
tune here (my cable operator broadcasts some channels with one format,
and others with the other one). Before this patch and the msp3400, it
would return a mask with PAL/M and PAL/60 or a mask with all NTSC/M formats.
Regarding your first version of the patch:
Acked-By: Mike Isely is...@pobox.com
-Mike
Regards,
Mauro.
case VIDIOC_QUERYSTD:
{
- v4l2_std_id *std = arg;
+ v4l2_std_id *std = V4L2_STD_ALL;
ret = pvr2_hdw_get_detected_std(hdw, std);
break;
}
-Mike
On Tue, 4 Oct 2011, Mauro Carvalho Chehab wrote:
Signed-off-by: Mauro Carvalho Chehabmche...@redhat.com
---
drivers/media/video/pvrusb2/pvrusb2-v4l2.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
b/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
index 0d029da..ce7ac45 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
@@ -230,6 +230,7 @@ static long pvr2_v4l2_do_ioctl(struct file *file,
unsigned int cmd, void *arg)
case VIDIOC_QUERYSTD:
{
v4l2_std_id *std = arg;
+ *std = V4L2_STD_ALL;
ret = pvr2_hdw_get_detected_std(hdw, std);
break;
}
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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 2/2] [media] pvrusb2: implement VIDIOC_QUERYSTD
Acked-By: Mike Isely is...@pobox.com
-Mike
On Mon, 3 Oct 2011, Mauro Carvalho Chehab wrote:
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
---
drivers/media/video/pvrusb2/pvrusb2-hdw.c |7 +++
drivers/media/video/pvrusb2/pvrusb2-hdw.h |3 +++
drivers/media/video/pvrusb2/pvrusb2-v4l2.c |7 +++
3 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
index e98d382..5a6f24d 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
@@ -2993,6 +2993,13 @@ static void pvr2_subdev_set_control(struct pvr2_hdw
*hdw, int id,
pvr2_subdev_set_control(hdw, id, #lab, (hdw)-lab##_val); \
}
+int pvr2_hdw_get_detected_std(struct pvr2_hdw *hdw, v4l2_std_id *std)
+{
+ v4l2_device_call_all(hdw-v4l2_dev, 0,
+ video, querystd, std);
+ return 0;
+}
+
/* Execute whatever commands are required to update the state of all the
sub-devices so that they match our current control values. */
static void pvr2_subdev_update(struct pvr2_hdw *hdw)
diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.h
b/drivers/media/video/pvrusb2/pvrusb2-hdw.h
index d7753ae..6654658 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.h
+++ b/drivers/media/video/pvrusb2/pvrusb2-hdw.h
@@ -214,6 +214,9 @@ struct pvr2_stream *pvr2_hdw_get_video_stream(struct
pvr2_hdw *);
int pvr2_hdw_get_stdenum_value(struct pvr2_hdw *hdw,struct v4l2_standard
*std,
unsigned int idx);
+/* Get the detected video standard */
+int pvr2_hdw_get_detected_std(struct pvr2_hdw *hdw, v4l2_std_id *std);
+
/* Enable / disable retrieval of CPU firmware or prom contents. This must
be enabled before pvr2_hdw_cpufw_get() will function. Note that doing
this may prevent the device from running (and leaving this mode may
diff --git a/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
b/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
index e27f8ab..0d029da 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
@@ -227,6 +227,13 @@ static long pvr2_v4l2_do_ioctl(struct file *file,
unsigned int cmd, void *arg)
break;
}
+ case VIDIOC_QUERYSTD:
+ {
+ v4l2_std_id *std = arg;
+ ret = pvr2_hdw_get_detected_std(hdw, std);
+ break;
+ }
+
case VIDIOC_G_STD:
{
int val = 0;
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: [RFCv6 PATCH 04/10] pvrusb2: fix g/s_tuner support.
I understand that this patch would not have been need had the pvrusb2
driver been using videodev_ioctl2. This is a situation that I'm going
to (finally) remedy ASAP. In the mean time...
Acked-By: Mike Isely is...@pobox.com
-Mike
On Tue, 14 Jun 2011, Hans Verkuil wrote:
From: Hans Verkuil hans.verk...@cisco.com
The tuner-core subdev requires that the type field of v4l2_tuner is
filled in correctly. This is done in v4l2-ioctl.c, but pvrusb2 doesn't
use that yet, so we have to do it manually based on whether the current
input is radio or not.
Tested with my pvrusb2.
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
---
drivers/media/video/pvrusb2/pvrusb2-hdw.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
index 9d0dd08..e98d382 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
@@ -3046,6 +3046,8 @@ static void pvr2_subdev_update(struct pvr2_hdw *hdw)
if (hdw-input_dirty || hdw-audiomode_dirty || hdw-force_dirty) {
struct v4l2_tuner vt;
memset(vt, 0, sizeof(vt));
+ vt.type = (hdw-input_val == PVR2_CVAL_INPUT_RADIO) ?
+ V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
vt.audmode = hdw-audiomode_val;
v4l2_device_call_all(hdw-v4l2_dev, 0, tuner, s_tuner, vt);
}
@@ -5171,6 +5173,8 @@ void pvr2_hdw_status_poll(struct pvr2_hdw *hdw)
{
struct v4l2_tuner *vtp = hdw-tuner_signal_info;
memset(vtp, 0, sizeof(*vtp));
+ vtp-type = (hdw-input_val == PVR2_CVAL_INPUT_RADIO) ?
+ V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
hdw-tuner_signal_stale = 0;
/* Note: There apparently is no replacement for VIDIOC_CROPCAP
using v4l2-subdev - therefore we can't support that AT ALL right
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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/6] [media] pvrusb2: check for allocation failures
I'll look at the surrounding code and see what makes sense there. Having an error leg for allocation failures is a useful thing. -Mike Dan Carpenter wrote: On Fri, Mar 25, 2011 at 11:33:36PM -0500, Mike Isely wrote: Acked-By: Mike Isely is...@pobox.com I'd need to reformat this one to get it to apply... :/ It doesn't actually fix the bug so it's not worth it. regards, dan carpenter -- 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 -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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] [media] pvrusb2: white space changes
I vehemently object to this scale of disruption to the pvrusb2 driver
source code purely to move around a bunch of braces and whitespace.
ESPECIALLY the massive ridiculous changes having to do with if-statement
syntax!
Nacked-By: Mike Isely is...@pobox.com
On Sat, 26 Mar 2011, Dan Carpenter wrote:
* Broke up if statements so that the condition and the body are on
separate lines.
* Added spaces around commas and other operator characters.
* Removed extra blank lines.
* Added blank lines after declarations.
* Changed C99 comments into kernel style.
* Fixed checkpatch complaints where { char was on its own line but it
wasn't the start of a function.
Signed-off-by: Dan Carpenter erro...@gmail.com
diff --git a/drivers/media/video/pvrusb2/pvrusb2-std.c
b/drivers/media/video/pvrusb2/pvrusb2-std.c
index ca9f83a..a5d4867 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-std.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-std.c
@@ -28,39 +28,38 @@ struct std_name {
v4l2_std_id id;
};
-
#define CSTD_PAL \
- (V4L2_STD_PAL_B| \
- V4L2_STD_PAL_B1| \
- V4L2_STD_PAL_G| \
- V4L2_STD_PAL_H| \
- V4L2_STD_PAL_I| \
- V4L2_STD_PAL_D| \
- V4L2_STD_PAL_D1| \
- V4L2_STD_PAL_K| \
- V4L2_STD_PAL_M| \
- V4L2_STD_PAL_N| \
- V4L2_STD_PAL_Nc| \
+ (V4L2_STD_PAL_B | \
+ V4L2_STD_PAL_B1 | \
+ V4L2_STD_PAL_G | \
+ V4L2_STD_PAL_H | \
+ V4L2_STD_PAL_I | \
+ V4L2_STD_PAL_D | \
+ V4L2_STD_PAL_D1 | \
+ V4L2_STD_PAL_K | \
+ V4L2_STD_PAL_M | \
+ V4L2_STD_PAL_N | \
+ V4L2_STD_PAL_Nc | \
V4L2_STD_PAL_60)
#define CSTD_NTSC \
- (V4L2_STD_NTSC_M| \
- V4L2_STD_NTSC_M_JP| \
- V4L2_STD_NTSC_M_KR| \
+ (V4L2_STD_NTSC_M| \
+ V4L2_STD_NTSC_M_JP | \
+ V4L2_STD_NTSC_M_KR | \
V4L2_STD_NTSC_443)
#define CSTD_ATSC \
- (V4L2_STD_ATSC_8_VSB| \
+ (V4L2_STD_ATSC_8_VSB | \
V4L2_STD_ATSC_16_VSB)
#define CSTD_SECAM \
- (V4L2_STD_SECAM_B| \
- V4L2_STD_SECAM_D| \
- V4L2_STD_SECAM_G| \
- V4L2_STD_SECAM_H| \
- V4L2_STD_SECAM_K| \
- V4L2_STD_SECAM_K1| \
- V4L2_STD_SECAM_L| \
+ (V4L2_STD_SECAM_B | \
+ V4L2_STD_SECAM_D | \
+ V4L2_STD_SECAM_G | \
+ V4L2_STD_SECAM_H | \
+ V4L2_STD_SECAM_K | \
+ V4L2_STD_SECAM_K1 | \
+ V4L2_STD_SECAM_L | \
V4L2_STD_SECAM_LC)
#define TSTD_B (V4L2_STD_PAL_B|V4L2_STD_SECAM_B)
@@ -82,39 +81,40 @@ struct std_name {
/* Mapping of standard bits to color system */
static const struct std_name std_groups[] = {
- {PAL,CSTD_PAL},
- {NTSC,CSTD_NTSC},
- {SECAM,CSTD_SECAM},
- {ATSC,CSTD_ATSC},
+ {PAL, CSTD_PAL},
+ {NTSC, CSTD_NTSC},
+ {SECAM, CSTD_SECAM},
+ {ATSC, CSTD_ATSC},
};
/* Mapping of standard bits to modulation system */
static const struct std_name std_items[] = {
- {B,TSTD_B},
- {B1,TSTD_B1},
- {D,TSTD_D},
- {D1,TSTD_D1},
- {G,TSTD_G},
- {H,TSTD_H},
- {I,TSTD_I},
- {K,TSTD_K},
- {K1,TSTD_K1},
- {L,TSTD_L},
- {LC,V4L2_STD_SECAM_LC},
- {M,TSTD_M},
- {Mj,V4L2_STD_NTSC_M_JP},
- {443,V4L2_STD_NTSC_443},
- {Mk,V4L2_STD_NTSC_M_KR},
- {N,TSTD_N},
- {Nc,TSTD_Nc},
- {60,TSTD_60},
- {8VSB,V4L2_STD_ATSC_8_VSB},
- {16VSB,V4L2_STD_ATSC_16_VSB},
+ {B, TSTD_B},
+ {B1,TSTD_B1},
+ {D, TSTD_D},
+ {D1,TSTD_D1},
+ {G, TSTD_G},
+ {H, TSTD_H},
+ {I, TSTD_I},
+ {K, TSTD_K},
+ {K1,TSTD_K1},
+ {L, TSTD_L},
+ {LC,V4L2_STD_SECAM_LC},
+ {M, TSTD_M},
+ {Mj,V4L2_STD_NTSC_M_JP},
+ {443, V4L2_STD_NTSC_443},
+ {Mk,V4L2_STD_NTSC_M_KR},
+ {N, TSTD_N},
+ {Nc,TSTD_Nc},
+ {60,TSTD_60},
+ {8VSB, V4L2_STD_ATSC_8_VSB},
+ {16VSB, V4L2_STD_ATSC_16_VSB},
};
-
-// Search an array of std_name structures and return a pointer to the
-// element with the matching name.
+/*
+ * Search an array of std_name structures and return a pointer to the
+ * element with the matching name.
+ */
static const struct std_name *find_std_name(const struct std_name *arrPtr,
unsigned int arrSize,
const char *bufPtr,
@@ -122,16 +122,18 @@ static const struct std_name *find_std_name(const
struct std_name *arrPtr,
{
unsigned int idx;
const struct std_name *p;
+
for (idx = 0; idx arrSize; idx++) {
p = arrPtr + idx;
- if (strlen(p-name) != bufSize) continue;
- if (!memcmp(bufPtr,p-name,bufSize)) return p;
+ if (strlen(p-name) != bufSize)
+ continue;
+ if (!memcmp(bufPtr, p-name, bufSize))
+ return p
Re: [PATCH 2/6] [media] pvrusb2: fix remaining checkpatch.pl complaints
I am OK with the #include change, but NOT the if-statement change. But
since it's bundled into one patch...
Nacked-By: Mike Isely is...@pobox.com
On Sat, 26 Mar 2011, Dan Carpenter wrote:
* Include linux/string.h instead of asm/string.h.
* Remove unneeded curly braces.
Signed-off-by: Dan Carpenter erro...@gmail.com
diff --git a/drivers/media/video/pvrusb2/pvrusb2-std.c
b/drivers/media/video/pvrusb2/pvrusb2-std.c
index a5d4867..370a9ab 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-std.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-std.c
@@ -20,7 +20,7 @@
#include pvrusb2-std.h
#include pvrusb2-debug.h
-#include asm/string.h
+#include linux/string.h
#include linux/slab.h
struct std_name {
@@ -294,9 +294,8 @@ static struct v4l2_standard *match_std(v4l2_std_id id)
unsigned int idx;
for (idx = 0; idx generic_standards_cnt; idx++) {
- if (generic_standards[idx].id id) {
+ if (generic_standards[idx].id id)
return generic_standards + idx;
- }
}
return NULL;
}
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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/6] [media] pvrusb2: check for allocation failures
Acked-By: Mike Isely is...@pobox.com On Sat, 26 Mar 2011, Dan Carpenter wrote: This function returns NULL on failure so lets do that if kzalloc() fails. There is a separate problem that the caller for this function doesn't check for errors... Signed-off-by: Dan Carpenter erro...@gmail.com diff --git a/drivers/media/video/pvrusb2/pvrusb2-std.c b/drivers/media/video/pvrusb2/pvrusb2-std.c index 370a9ab..b214f77 100644 --- a/drivers/media/video/pvrusb2/pvrusb2-std.c +++ b/drivers/media/video/pvrusb2/pvrusb2-std.c @@ -388,6 +388,9 @@ struct v4l2_standard *pvr2_std_create_enum(unsigned int *countptr, stddefs = kzalloc(sizeof(struct v4l2_standard) * std_cnt, GFP_KERNEL); + if (!stddefs) + return NULL; + for (idx = 0; idx std_cnt; idx++) stddefs[idx].index = idx; -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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 4/6] [media] pvrusb2: fix camel case variables
It not worth this scale of source code disruption to the source code
just to rename a bunch of variables. I'm sorry, but...
Nacked-By: Mike Isely is...@pobox.com
On Sat, 26 Mar 2011, Dan Carpenter wrote:
This patch renames some variables to bring them more in line with
kernel CodingStyle.
arrPtr = arr
arrSize = arr_size
bufPtr = buf
bufSize = buf_size
Signed-off-by: Dan Carpenter erro...@gmail.com
diff --git a/drivers/media/video/pvrusb2/pvrusb2-std.c
b/drivers/media/video/pvrusb2/pvrusb2-std.c
index b214f77..d5a679f 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-std.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-std.c
@@ -115,26 +115,26 @@ static const struct std_name std_items[] = {
* Search an array of std_name structures and return a pointer to the
* element with the matching name.
*/
-static const struct std_name *find_std_name(const struct std_name *arrPtr,
- unsigned int arrSize,
- const char *bufPtr,
- unsigned int bufSize)
+static const struct std_name *find_std_name(const struct std_name *arr,
+ unsigned int arr_size,
+ const char *buf,
+ unsigned int buf_size)
{
unsigned int idx;
const struct std_name *p;
- for (idx = 0; idx arrSize; idx++) {
- p = arrPtr + idx;
- if (strlen(p-name) != bufSize)
+ for (idx = 0; idx arr_size; idx++) {
+ p = arr + idx;
+ if (strlen(p-name) != buf_size)
continue;
- if (!memcmp(bufPtr, p-name, bufSize))
+ if (!memcmp(buf, p-name, buf_size))
return p;
}
return NULL;
}
-int pvr2_std_str_to_id(v4l2_std_id *idPtr, const char *bufPtr,
-unsigned int bufSize)
+int pvr2_std_str_to_id(v4l2_std_id *idPtr, const char *buf,
+unsigned int buf_size)
{
v4l2_std_id id = 0;
v4l2_std_id cmsk = 0;
@@ -144,27 +144,27 @@ int pvr2_std_str_to_id(v4l2_std_id *idPtr, const char
*bufPtr,
char ch;
const struct std_name *sp;
- while (bufSize) {
+ while (buf_size) {
if (!mMode) {
cnt = 0;
- while ((cnt bufSize) (bufPtr[cnt] != '-'))
+ while ((cnt buf_size) (buf[cnt] != '-'))
cnt++;
- if (cnt = bufSize)
+ if (cnt = buf_size)
return 0; /* No more characters */
sp = find_std_name(std_groups, ARRAY_SIZE(std_groups),
-bufPtr, cnt);
+buf, cnt);
if (!sp)
return 0; /* Illegal color system name */
cnt++;
- bufPtr += cnt;
- bufSize -= cnt;
+ buf += cnt;
+ buf_size -= cnt;
mMode = !0;
cmsk = sp-id;
continue;
}
cnt = 0;
- while (cnt bufSize) {
- ch = bufPtr[cnt];
+ while (cnt buf_size) {
+ ch = buf[cnt];
if (ch == ';') {
mMode = 0;
break;
@@ -174,7 +174,7 @@ int pvr2_std_str_to_id(v4l2_std_id *idPtr, const char
*bufPtr,
cnt++;
}
sp = find_std_name(std_items, ARRAY_SIZE(std_items),
-bufPtr, cnt);
+buf, cnt);
if (!sp)
return 0; /* Illegal modulation system ID */
t = sp-id cmsk;
@@ -182,10 +182,10 @@ int pvr2_std_str_to_id(v4l2_std_id *idPtr, const char
*bufPtr,
return 0; /* Specific color + modulation system
illegal */
id |= t;
- if (cnt bufSize)
+ if (cnt buf_size)
cnt++;
- bufPtr += cnt;
- bufSize -= cnt;
+ buf += cnt;
+ buf_size -= cnt;
}
if (idPtr)
@@ -193,7 +193,7 @@ int pvr2_std_str_to_id(v4l2_std_id *idPtr, const char
*bufPtr,
return !0;
}
-unsigned int pvr2_std_id_to_str(char *bufPtr, unsigned int bufSize,
+unsigned int pvr2_std_id_to_str(char *buf, unsigned int buf_size,
v4l2_std_id id)
{
unsigned int idx1, idx2;
@@ -212,26 +212,26 @@ unsigned int pvr2_std_id_to_str(char *bufPtr, unsigned
int bufSize,
continue
Re: [PATCH 5/5] [media] pvrusb2: delete generic_standards_cnt
Are you actually serious about this? Well it's a small change...
Acked-By: Mike Isely is...@pobox.com
On Sat, 26 Mar 2011, Dan Carpenter wrote:
The generic_standards_cnt define is only used in one place and it's
more readable to just call ARRAY_SIZE(generic_standards) directly.
Signed-off-by: Dan Carpenter erro...@gmail.com
diff --git a/drivers/media/video/pvrusb2/pvrusb2-std.c
b/drivers/media/video/pvrusb2/pvrusb2-std.c
index d5a679f..9bebc08 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-std.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-std.c
@@ -287,13 +287,11 @@ static struct v4l2_standard generic_standards[] = {
}
};
-#define generic_standards_cnt ARRAY_SIZE(generic_standards)
-
static struct v4l2_standard *match_std(v4l2_std_id id)
{
unsigned int idx;
- for (idx = 0; idx generic_standards_cnt; idx++) {
+ for (idx = 0; idx ARRAY_SIZE(generic_standards); idx++) {
if (generic_standards[idx].id id)
return generic_standards + idx;
}
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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 6/6] [media] pvrusb2: replace !0 with 1
That's an opinion which I as the driver author disagree with. Strongly.
How hard is it to read not false?
Nacked-By: Mike Isely is...@pobox.com
On Sat, 26 Mar 2011, Dan Carpenter wrote:
Using !0 is less readable than just saying 1.
Signed-off-by: Dan Carpenter erro...@gmail.com
diff --git a/drivers/media/video/pvrusb2/pvrusb2-std.c
b/drivers/media/video/pvrusb2/pvrusb2-std.c
index 9bebc08..ca4f67b 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-std.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-std.c
@@ -158,7 +158,7 @@ int pvr2_std_str_to_id(v4l2_std_id *idPtr, const char
*buf,
cnt++;
buf += cnt;
buf_size -= cnt;
- mMode = !0;
+ mMode = 1;
cmsk = sp-id;
continue;
}
@@ -190,7 +190,7 @@ int pvr2_std_str_to_id(v4l2_std_id *idPtr, const char
*buf,
if (idPtr)
*idPtr = id;
- return !0;
+ return 1;
}
unsigned int pvr2_std_id_to_str(char *buf, unsigned int buf_size,
@@ -217,10 +217,10 @@ unsigned int pvr2_std_id_to_str(char *buf, unsigned int
buf_size,
buf_size -= c2;
buf += c2;
}
- cfl = !0;
+ cfl = 1;
c2 = scnprintf(buf, buf_size,
%s-, gp-name);
- gfl = !0;
+ gfl = 1;
} else {
c2 = scnprintf(buf, buf_size, /);
}
@@ -315,7 +315,7 @@ static int pvr2_std_fill(struct v4l2_standard *std,
v4l2_std_id id)
std-name[bcnt] = 0;
pvr2_trace(PVR2_TRACE_STD, Set up standard idx=%u name=%s,
std-index, std-name);
- return !0;
+ return 1;
}
/*
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: compilation warnings/errors
On Fri, 11 Mar 2011, Mike Isely wrote: On Fri, 11 Mar 2011, Mauro Carvalho Chehab wrote: /home/mchehab/new_build/v4l/pvrusb2-v4l2.c: In function 'pvr2_v4l2_do_ioctl': /home/mchehab/new_build/v4l/pvrusb2-v4l2.c:798:23: warning: variable 'cap' set but not used [-Wunused-but-set-variable] I will look into these. I'm a little puzzled right now since silly stuff like this usually doesn't get by me. Unfortunately I can't look at it right this minute. Expect to hear from me on Sunday. I looked at these two warnings. It's dead code that should be removed. Amazingly enough, this particular bit of crap has been in the driver, unnoticed, since 2008! I have a pull request coming for more pvrusb2 patches, probably in a few more hours, once I'm done testing. A fix for this will be in the patch set. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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
[GIT PATCHES FOR 2.6.39] pvrusb2 driver fixes / improvements
Mauro: Please pull the following patches. Note also that the Implement support for Terratec Grabster AV400 is not as big of a change as it might sound. The work to implement that really amounted to just some extra table entries, plus those changes have been out in the wild via the standalone pvrusb2 driver for quite some time. Getting that into the kernel is long overdue. -Mike The following changes since commit 41f3becb7bef489f9e8c35284dd88a1ff59b190c: Hans Verkuil (1): [media] V4L DocBook: update V4L2 version are available in the git repository at: git://git.linuxtv.org/mcisely/pvrusb2-dev.git pvrusb2-merge-2 Mike Isely (2): pvrusb2: Implement support for Terratec Grabster AV400 pvrusb2: Remove dead code Xiaochen Wang (1): pvrusb2: check kmalloc return value drivers/media/video/pvrusb2/pvrusb2-cx2584x-v4l.c | 18 +++ drivers/media/video/pvrusb2/pvrusb2-devattr.c | 24 + drivers/media/video/pvrusb2/pvrusb2-hdw.c | 24 ++--- drivers/media/video/pvrusb2/pvrusb2-v4l2.c|2 - 4 files changed, 58 insertions(+), 10 deletions(-) -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: compilation warnings/errors
On Fri, 11 Mar 2011, Mauro Carvalho Chehab wrote: /home/mchehab/new_build/v4l/pvrusb2-v4l2.c: In function 'pvr2_v4l2_do_ioctl': /home/mchehab/new_build/v4l/pvrusb2-v4l2.c:798:23: warning: variable 'cap' set but not used [-Wunused-but-set-variable] I will look into these. I'm a little puzzled right now since silly stuff like this usually doesn't get by me. Unfortunately I can't look at it right this minute. Expect to hear from me on Sunday. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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
[GIT PULL FOR 2.6.39] pvrusb2 driver
Mauro, [Note: This is my first real attempt at using git to get changes pulled, so please let me know if I missed a step. These changes are all relatively minor and have been sitting around for while. There will be more to follow once I'm sure I am doing this process correctly... -Mike Isely] The following changes since commit 5ed4bbdae09d207d141759e013a0f3c24ae76ecc: Mauro Carvalho Chehab (1): [media] tuner-core: Don't touch at standby during tuner_lookup are available in the git repository at: git://git.linuxtv.org/mcisely/pvrusb2-dev.git pvrusb2-merge-1 Mike Isely (5): pvrusb2: Handle change of mode before handling change of video standard pvrusb2: Minor cosmetic code tweak pvrusb2: Fix a few missing default control values, for cropping pvrusb2: Minor VBI tweak to help potential CC support pvrusb2: Use sysfs_attr_init() where appropriate Servaas Vandenberghe (1): pvrusb2: width and height maximum values. drivers/media/video/pvrusb2/pvrusb2-hdw.c | 60 +++ drivers/media/video/pvrusb2/pvrusb2-sysfs.c |9 2 files changed, 43 insertions(+), 26 deletions(-) -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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] ir-kbd-i2c: improve remote behavior with z8 behind usb
The pvrusb2 change is obviously trivial so I have no issue with it.
Acked-By: Mike Isely is...@pobox.com
Note the spelling of my last name Isely not Isley. A good way to
remember is to think of the normal word wisely and just drop the
leading w. (And yes, is...@isely.net and is...@pobox.com lead to the
same inbox.)
-Mike
On Thu, 20 Jan 2011, Jarod Wilson wrote:
Add the same are you ready? i2c_master_send() poll command to
get_key_haup_xvr found in lirc_zilog, which is apparently seen in
the Windows driver for the PVR-150 w/a z8. This stabilizes what is
received from both the HD-PVR and HVR-1950, even with their polling
intervals at the default of 100, thus the removal of the custom
260ms polling_interval in pvrusb2-i2c-core.c.
CC: Andy Walls awa...@md.metrocast.net
CC: Mike Isley is...@isley.net
Signed-off-by: Jarod Wilson ja...@redhat.com
---
drivers/media/video/ir-kbd-i2c.c | 13 +
drivers/media/video/pvrusb2/pvrusb2-i2c-core.c |1 -
2 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/ir-kbd-i2c.c
b/drivers/media/video/ir-kbd-i2c.c
index d2b20ad..a221ad6 100644
--- a/drivers/media/video/ir-kbd-i2c.c
+++ b/drivers/media/video/ir-kbd-i2c.c
@@ -128,6 +128,19 @@ static int get_key_haup(struct IR_i2c *ir, u32 *ir_key,
u32 *ir_raw)
static int get_key_haup_xvr(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
{
+ int ret;
+ unsigned char buf[1] = { 0 };
+
+ /*
+ * This is the same apparent are you ready? poll command observed
+ * watching Windows driver traffic and implemented in lirc_zilog. With
+ * this added, we get far saner remote behavior with z8 chips on usb
+ * connected devices, even with the default polling interval of 100ms.
+ */
+ ret = i2c_master_send(ir-c, buf, 1);
+ if (ret != 1)
+ return (ret 0) ? ret : -EINVAL;
+
return get_key_haup_common (ir, ir_key, ir_raw, 6, 3);
}
diff --git a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
index ccc8849..451ecd4 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
@@ -597,7 +597,6 @@ static void pvr2_i2c_register_ir(struct pvr2_hdw *hdw)
init_data-internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
init_data-type = RC_TYPE_RC5;
init_data-name = hdw-hdw_desc-description;
- init_data-polling_interval = 260; /* ms From lirc_zilog */
/* IR Receiver */
info.addr = 0x71;
info.platform_data = init_data;
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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] ir-kbd-i2c: improve remote behavior with z8 behind usb
On Fri, 21 Jan 2011, Mike Isely wrote: Note the spelling of my last name Isely not Isley. A good way to remember is to think of the normal word wisely and just drop the leading w. (And yes, is...@isely.net and is...@pobox.com lead to the same inbox.) And of course having said that, I then failed to fix the cc list. Sorry about that. D'Oh!!! -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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] ir-kbd-i2c: improve remote behavior with z8 behind usb
On Fri, 21 Jan 2011, Jarod Wilson wrote: On Fri, Jan 21, 2011 at 10:31:42AM -0600, Mike Isely wrote: The pvrusb2 change is obviously trivial so I have no issue with it. Acked-By: Mike Isely is...@pobox.com Note the spelling of my last name Isely not Isley. A good way to remember is to think of the normal word wisely and just drop the leading w. (And yes, is...@isely.net and is...@pobox.com lead to the same inbox.) Thanks Mike, apologies about the misspelling, I didn't catch it until after I hit send. I had the Isley Brothers in my head. :) No problem. It's a very common mistake. And no, I'm not related to them. For the record, I generally don't get concerned about the spelling of my name, unless the error causes problems (e.g. lost e-mail) or the error gets propagated to a large list where it might multiply... Anyway, sorry also about taking this thread off topic. Enough said... -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [GIT PATCHES for 2.6.38] Zilog Z8 IR unit fixes
On Wed, 19 Jan 2011, Andy Walls wrote: On Wed, 2011-01-19 at 00:20 -0500, Jarod Wilson wrote: Not working with lirc_zilog yet, it fails to load, due to an -EIO ret to one of the i2c_master_send() calls in lirc_zilog during probe of the TX side. Haven't looked into it any more than that yet. Well technically lirc_zilog doesn't probe anymore. It relies on the bridge driver telling it the truth. The bridge driver (pvrusb2) still does one probe if it's a 24xxx device: It probes 0x71 in order to determine if it is dealing with an MCE variant device. Hauppauge did not change the USB ID when they released the 24xxx MCE variant (which has the IR blaster, thus the zilog device). The only way to tell the two devices apart is by discovering the existence of the zilog device - and the bridge driver needs to do this in order to properly disable its emulated I2C IR receiver which would otherwise be needed for the non-MCE device. Based on the discussion here, could that probe be a source of trouble on the 24XXX MCE device? This probing behavior does not happen for HVR-1950 (or HVR-1900) since there's only one possible IR configuration there. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [GIT PATCHES for 2.6.38] Zilog Z8 IR unit fixes
On Wed, 19 Jan 2011, Andy Walls wrote: On Wed, 2011-01-19 at 13:40 +0100, Jean Delvare wrote: On Wed, 19 Jan 2011 07:21:58 -0500, Andy Walls wrote: For debugging, you might want to hack in a probe of address 0x70 for your HVR-1950, to ensure the Tx side responds in the bridge driver. ... keeping in mind that the Z8 doesn't seem to like quick writes, so short reads should be used for probing purpose. Noted. Thanks. Actually, I think that might be due to the controller in the USB connected devices (hdpvr and pvrusb2). The PCI connected devices, like cx18 cards, don't have a problem with the Z8, the default I2C probe method, and i2c-algo-bit. (A good example of why only bridge drivers should do any required probing.) Looking at the code in pvrusb2, it appears to already use a 0 length read for a probe: http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/pvrusb2/pvrusb2-i2c-core.c;h=ccc884948f34b385563ccbf548c5f80b33cd4f08;hb=refs/heads/staging/for_2.6.38-rc1#l542 Yes but that function is used in two places: (1) If a bus scan is performed during initialization (normally it isn't), and (2) it is called once ONLY for a 24xxx device (targeting 0x71) in order to determine if it is dealing with the MCE variant. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [GIT PATCHES for 2.6.38] Zilog Z8 IR unit fixes
On Wed, 19 Jan 2011, Andy Walls wrote: [...] So the HVR-1950 only has Z8's capable of both Tx and Rx? No HVR-1950 has an Rx only Z8 unit? As far as I know, that is indeed the case - Tx and Rx always. It's the older 24xxx devices where there could be a difference, and that's why the probe only takes place there. (And in the receive-only 24xxx configuration it's not a Z8 but something wierd that is only accessible through FX2 commands not via I2C, which is why the bridge driver emulates the older I2C chip, making IR reception behave like the original 29xxx device.) -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [GIT PATCHES for 2.6.38] Zilog Z8 IR unit fixes
On Wed, 19 Jan 2011, Jean Delvare wrote: Hi Andy, On Sun, 16 Jan 2011 14:20:49 -0500, Andy Walls wrote: 3. I hear from Jean, or whomever really cares about ir-kbd-i2c, if adding some new fields for struct IR_i2c_init_data is acceptable. Specifically, I'd like to add a transceiver_lock mutex, a transceiver reset callback, and a data pointer for that reset callback. (Only lirc_zilog would use the reset callback and data pointer.) Adding fields to these structures is perfectly fine, if you need to do that, just go on. But I'm a little confused about the names you chose, ir_transceiver_lock and transceiver_lock. These seem too TX-oriented for a mutex that is supposed to synchronize TX and RX access. It's particularly surprising for the ir-kbd-i2c driver, which as far as I know only supports RX. The name xcvr_lock you used for lirc_zilog seems more appropriate. Actually the term transceiver is normally understood to mean both directions. Otherwise it would be receiver or transmitter. Another screwy as aspect of english, and I say this as a native english speaker. The term xcvr is usually just considered to be shorthand for transceiver. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [GIT PATCHES for 2.6.38] Zilog Z8 IR unit fixes
On Wed, 19 Jan 2011, Jarod Wilson wrote: On Jan 19, 2011, at 8:20 AM, Mike Isely wrote: This probing behavior does not happen for HVR-1950 (or HVR-1900) since there's only one possible IR configuration there. Just to be 100% clear, the device I'm poking it is definitely an HVR-1950, using ir_scheme PVR2_IR_SCHEME_ZILOG, so the probe bits shouldn't coming into play with anything I'm doing. Only just now started looking at the pvrusb2 code. Wow, there's a LOT of it. ;) Yes, and yes :-) The standalone driver version (which is loaded with ifdef's that allow compilation back to 2.6.11) makes the in-kernel driver look small by comparison. There is a fair degree of compartmentalization between the modules. The roadmap to what it does for just HVR-1950 you can find by first looking at the declarations in pvrusb2-devattr.h and then the device-specific configurations in pvrusb2-devattr.c. From there you can usually grep your way around to see how those configuration bits affect the rest of the driver. Most of the really fun stuff is in pvrusb2-hdw.c. Pretty much everything else supports or uses that central component. The actual stuff which deals with I2C is not that large. Beyond making the access possible at all, the driver largely just tries to stay out of the way of external logic that needs to reach the bus. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [RFC PATCH] pvrusb2: Provide more information about IR units to lirc_zilog and ir-kbd-i2c
Andy:
Is the IR_i2c_init_data struct instance required to remain around for
the life of the driver's registration and is that why you stuffed it
into the pvr2_hdw struct? Second: If the first question is yes, then is
that struct considered to be read-only once it is set up and passed
through to the i2c device registration function? In other words, could
that structure be a const static initialized at compile time, perhaps
as part of a table definition?
I believe I follow this and it looks good. The concept looks very
simple and it's nice that the changes are really only in a single spot.
Just thinking ahead about making the setup table-driven and not
requiring data segment storage.
-Mike
Acked-By: Mike Isely is...@pobox.com
On Sun, 16 Jan 2011, Andy Walls wrote:
When registering an IR Rx device with the I2C subsystem, provide more detailed
information about the IR device and default remote configuration for the IR
driver modules.
Also explicitly register any IR Tx device with the I2C subsystem.
Signed-off-by: Andy Walls awa...@md.metrocast.net
Cc: Mike Isely is...@isely.net
--
Mike,
As discussed on IRC, this patch will enable lirc_zilog to bind to Zilog
Z8 IR units on devices supported by pvrusb2.
Please review and comment. This patch could have been written a number
of ways. The way I chose was very direct: hard-coding information in a
single function.
A git branch with this change, and the updated lirc_zilog, is here:
git://linuxtv.org/awalls/media_tree.git z8-pvrusb2
http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/z8-pvrusb2
Regards,
Andy
diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw-internal.h
b/drivers/media/video/pvrusb2/pvrusb2-hdw-internal.h
index ac94a8b..305e6aa 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw-internal.h
+++ b/drivers/media/video/pvrusb2/pvrusb2-hdw-internal.h
@@ -40,6 +40,7 @@
#include pvrusb2-io.h
#include media/v4l2-device.h
#include media/cx2341x.h
+#include media/ir-kbd-i2c.h
#include pvrusb2-devattr.h
/* Legal values for PVR2_CID_HSM */
@@ -202,6 +203,7 @@ struct pvr2_hdw {
/* IR related */
unsigned int ir_scheme_active; /* IR scheme as seen from the outside */
+ struct IR_i2c_init_data ir_init_data; /* params passed to IR modules */
/* Frequency table */
unsigned int freqTable[FREQTABLE_SIZE];
diff --git a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
index 7cbe18c..ccc8849 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
@@ -19,6 +19,7 @@
*/
#include linux/i2c.h
+#include media/ir-kbd-i2c.h
#include pvrusb2-i2c-core.h
#include pvrusb2-hdw-internal.h
#include pvrusb2-debug.h
@@ -48,13 +49,6 @@ module_param_named(disable_autoload_ir_video,
pvr2_disable_ir_video,
MODULE_PARM_DESC(disable_autoload_ir_video,
1=do not try to autoload ir_video IR receiver);
-/* Mapping of IR schemes to known I2C addresses - if any */
-static const unsigned char ir_video_addresses[] = {
- [PVR2_IR_SCHEME_ZILOG] = 0x71,
- [PVR2_IR_SCHEME_29XXX] = 0x18,
- [PVR2_IR_SCHEME_24XXX] = 0x18,
-};
-
static int pvr2_i2c_write(struct pvr2_hdw *hdw, /* Context */
u8 i2c_addr, /* I2C address we're talking to */
u8 *data, /* Data to write */
@@ -574,26 +568,56 @@ static void do_i2c_scan(struct pvr2_hdw *hdw)
static void pvr2_i2c_register_ir(struct pvr2_hdw *hdw)
{
struct i2c_board_info info;
- unsigned char addr = 0;
+ struct IR_i2c_init_data *init_data = hdw-ir_init_data;
if (pvr2_disable_ir_video) {
pvr2_trace(PVR2_TRACE_INFO,
Automatic binding of ir_video has been disabled.);
return;
}
- if (hdw-ir_scheme_active ARRAY_SIZE(ir_video_addresses)) {
- addr = ir_video_addresses[hdw-ir_scheme_active];
- }
- if (!addr) {
+ memset(info, 0, sizeof(struct i2c_board_info));
+ switch (hdw-ir_scheme_active) {
+ case PVR2_IR_SCHEME_24XXX: /* FX2-controlled IR */
+ case PVR2_IR_SCHEME_29XXX: /* Original 29xxx device */
+ init_data-ir_codes = RC_MAP_HAUPPAUGE_NEW;
+ init_data-internal_get_key_func = IR_KBD_GET_KEY_HAUP;
+ init_data-type = RC_TYPE_RC5;
+ init_data-name = hdw-hdw_desc-description;
+ init_data-polling_interval = 100; /* ms From ir-kbd-i2c */
+ /* IR Receiver */
+ info.addr = 0x18;
+ info.platform_data = init_data;
+ strlcpy(info.type, ir_video, I2C_NAME_SIZE);
+ pvr2_trace(PVR2_TRACE_INFO, Binding %s to i2c address 0x%02x.,
+info.type
Re: [RFC PATCH] pvrusb2: Provide more information about IR units to lirc_zilog and ir-kbd-i2c
On Sun, 16 Jan 2011, Andy Walls wrote: On Sun, 2011-01-16 at 20:27 -0600, Mike Isely wrote: [,,,] Right now, yes. In the near future, I need to use to to pass 3 non-const items though: 1. A struct mutex *transceiver_lock so that the bridge driver can pass a mutex to multiple modules accessing the Z8. That would be a per device instance mutex, instantiated and initialized by the bridge driver. The use case where this would be needed is a setup where ir-kbd-i2c handles Z8 IR Rx and lirc_zilog handles only Z8 IR Tx of the same chip. 2. A bridge driver provided void (*reset_ir_chip)(struct i2c_adapter *adap), or maybe void (*reset_ir_chip)(void *priv), callback to reset the transceiver chip when it gets hung. The original lirc_pvr150 module had some hard coded reset function names and calls in it, but they were removed with the rename to lirc_zilog and move into the kernel. I'd like to get that ability back. 3. A bridge driver provided private data structure for the void *priv argument of the aforementioned reset callback. This would also be a per device object instantiated and initialized by the bridge driver. I follow. Makes sense. Something to consider, perhaps for the future: Seems like what you have here amounts to some configuration data which will always be read-only, and other data which maps to the context in which the driver is being used (e.g. mutex instance, callback private context pointer, etc). That configuration data, if packed up into its own struct, could then be squirreled away at compile-time by the bridge driver and provided as part of a single table lookup. This only makes sense if there are a lot of configuration bits - but here I count 6 different items. I believe I follow this and it looks good. The concept looks very simple and it's nice that the changes are really only in a single spot. Just thinking ahead about making the setup table-driven and not requiring data segment storage. With the patch right now it could be constant, I think. You would have to use some generic name, like pvrusb2 IR, instead of hdw-hdw_desc-description though. For my future plans, if you don't provide a reset callback and don't wish to provide a mutex, then yes you can keep it constant. I suspect not providing a reset callback may be OK. Not providing a mutex is also OK but it imposes a limitation: only one IR module should be allowed to use the Z8 chip. That means only lirc_zilog for IR Tx/Rx with Rx through LIRC, or only ir-kbd-i2c for IR Rx through the the Linux input subsystem. For the future, I have no problem providing a reset callback. And given what you've said, I see no reason to do anything here which would constrain what you're trying to accomplish. But if down the road you do set up a separate configuration struct which this context struct might point to, then I'd like to update the pvrusb2 driver to take advantage of it. But this is no big deal for now. -Mike Acked-By: Mike Isely is...@pobox.com Thanks. I'll pull this into my Z8 branch then. You're welcome. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: Volunteers needed: BKL removal: replace .ioctl by .unlocked_ioctl
I'll take care of the pvrusb2 driver. How soon does this need to be completed? -Mike On Sat, 18 Dec 2010, Hans Verkuil wrote: On Saturday, December 18, 2010 12:31:26 Hans Verkuil wrote: Driver list: saa7146 (Hans Verkuil) mem2mem_testdev (Pawel Osciak or Marek Szyprowski) cx23885 (Steve Toth) cx18-alsa (Andy Walls) omap24xxcam (Sakari Ailus or David Cohen) au0828 (Janne Grunau) cpia2 (Andy Walls or Hans Verkuil) cx231xx (Mauro Carvalho Chehab) davinci (Muralidharan Karicheri) saa6588 (Hans Verkuil) pvrusb2 (Mike Isely) usbvision (Hans Verkuil) s5p-fimc (Sylwester Nawrocki) fsl-viu (Anatolij Gustschin) tlg2300 (Mauro Carvalho Chehab) zr364xx (Hans de Goede) soc_camera (Guennadi Liakhovetski) usbvideo/vicam (Hans de Goede) s2255drv (Pete Eberlein) bttv (Mauro Carvalho Chehab) stk-webcam (Hans de Goede) se401 (Hans de Goede) si4713-i2c (Hans Verkuil) dsbr100 (Hans Verkuil) Oops, si4713-i2c and saa6588 are subdevs, so those two can be removed from this list. Regards, Hans -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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 16/16] v4l: Remove module_name argument to the v4l2_i2c_new_subdev* functions
For just the pvrusb2 part of the patch series below
Acked-By: Mike Isely is...@pobox.com
On Fri, 24 Sep 2010, Laurent Pinchart wrote:
The argument isn't used anymore by the functions, remote it.
Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
drivers/media/radio/radio-si4713.c|2 +-
drivers/media/video/au0828/au0828-cards.c |4 ++--
drivers/media/video/bt8xx/bttv-cards.c| 22 +++---
drivers/media/video/cafe_ccic.c |2 +-
drivers/media/video/cx18/cx18-i2c.c |8
drivers/media/video/cx231xx/cx231xx-cards.c |4 ++--
drivers/media/video/cx23885/cx23885-cards.c |2 +-
drivers/media/video/cx23885/cx23885-video.c |4 ++--
drivers/media/video/cx88/cx88-cards.c |9 -
drivers/media/video/cx88/cx88-video.c |7 +++
drivers/media/video/davinci/vpfe_capture.c|1 -
drivers/media/video/davinci/vpif_capture.c|1 -
drivers/media/video/davinci/vpif_display.c|2 +-
drivers/media/video/em28xx/em28xx-cards.c | 18 +-
drivers/media/video/fsl-viu.c |2 +-
drivers/media/video/ivtv/ivtv-i2c.c | 22 +-
drivers/media/video/mxb.c | 12 ++--
drivers/media/video/pvrusb2/pvrusb2-hdw.c |6 ++
drivers/media/video/saa7134/saa7134-cards.c |8
drivers/media/video/saa7134/saa7134-core.c|4 ++--
drivers/media/video/sh_vou.c |2 +-
drivers/media/video/soc_camera.c |2 +-
drivers/media/video/usbvision/usbvision-i2c.c |6 +++---
drivers/media/video/v4l2-common.c | 15 +--
drivers/media/video/vino.c|4 ++--
drivers/media/video/zoran/zoran_card.c|5 ++---
drivers/staging/go7007/go7007-driver.c|2 +-
drivers/staging/tm6000/tm6000-cards.c |4 ++--
include/media/v4l2-common.h | 16 ++--
29 files changed, 88 insertions(+), 108 deletions(-)
diff --git a/drivers/media/radio/radio-si4713.c
b/drivers/media/radio/radio-si4713.c
index 045b10f..d49c215 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -291,7 +291,7 @@ static int radio_si4713_pdriver_probe(struct
platform_device *pdev)
goto unregister_v4l2_dev;
}
- sd = v4l2_i2c_new_subdev_board(rsdev-v4l2_dev, adapter, NULL,
+ sd = v4l2_i2c_new_subdev_board(rsdev-v4l2_dev, adapter,
pdata-subdev_board_info, NULL);
if (!sd) {
dev_err(pdev-dev, Cannot get v4l2 subdevice\n);
diff --git a/drivers/media/video/au0828/au0828-cards.c
b/drivers/media/video/au0828/au0828-cards.c
index 0453816..01be89f 100644
--- a/drivers/media/video/au0828/au0828-cards.c
+++ b/drivers/media/video/au0828/au0828-cards.c
@@ -212,7 +212,7 @@ void au0828_card_setup(struct au0828_dev *dev)
be abstracted out if we ever need to support a different
demod) */
sd = v4l2_i2c_new_subdev(dev-v4l2_dev, dev-i2c_adap,
- NULL, au8522, 0x8e 1, NULL);
+ au8522, 0x8e 1, NULL);
if (sd == NULL)
printk(KERN_ERR analog subdev registration failed\n);
}
@@ -221,7 +221,7 @@ void au0828_card_setup(struct au0828_dev *dev)
if (dev-board.tuner_type != TUNER_ABSENT) {
/* Load the tuner module, which does the attach */
sd = v4l2_i2c_new_subdev(dev-v4l2_dev, dev-i2c_adap,
- NULL, tuner, dev-board.tuner_addr, NULL);
+ tuner, dev-board.tuner_addr, NULL);
if (sd == NULL)
printk(KERN_ERR tuner subdev registration fail\n);
diff --git a/drivers/media/video/bt8xx/bttv-cards.c
b/drivers/media/video/bt8xx/bttv-cards.c
index 87d8b00..49efcf6 100644
--- a/drivers/media/video/bt8xx/bttv-cards.c
+++ b/drivers/media/video/bt8xx/bttv-cards.c
@@ -3529,7 +3529,7 @@ void __devinit bttv_init_card2(struct bttv *btv)
struct v4l2_subdev *sd;
sd = v4l2_i2c_new_subdev(btv-c.v4l2_dev,
- btv-c.i2c_adap, NULL, saa6588, 0, addrs);
+ btv-c.i2c_adap, saa6588, 0, addrs);
btv-has_saa6588 = (sd != NULL);
}
@@ -3554,7 +3554,7 @@ void __devinit bttv_init_card2(struct bttv *btv)
};
btv-sd_msp34xx = v4l2_i2c_new_subdev(btv-c.v4l2_dev,
- btv-c.i2c_adap, NULL, msp3400, 0, addrs);
+ btv-c.i2c_adap, msp3400, 0, addrs);
if (btv-sd_msp34xx)
return;
goto no_audio;
@@ -3568,7 +3568,7 @@ void __devinit bttv_init_card2
Re: [PATCH 07/16] pvrusb2: Don't use module names to load I2C modules
Acked-By: Mike Isely is...@pobox.com
On Fri, 24 Sep 2010, Laurent Pinchart wrote:
With the v4l2_i2c_new_subdev* functions now supporting loading modules
based on modaliases, replace the hardcoded module name passed to those
functions by NULL.
All corresponding I2C modules have been checked, and all of them include
a module aliases table with names corresponding to what the pvrusb2
driver uses.
Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
drivers/media/video/pvrusb2/pvrusb2-hdw.c | 11 ++-
1 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
index 70ea578..bef2027 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
@@ -2082,20 +2082,13 @@ static int pvr2_hdw_load_subdev(struct pvr2_hdw *hdw,
return -EINVAL;
}
- /* Note how the 2nd and 3rd arguments are the same for
- * v4l2_i2c_new_subdev(). Why?
- * Well the 2nd argument is the module name to load, while the 3rd
- * argument is documented in the framework as being the chipid -
- * and every other place where I can find examples of this, the
- * chipid appears to just be the module name again. So here we
- * just do the same thing. */
if (i2ccnt == 1) {
pvr2_trace(PVR2_TRACE_INIT,
Module ID %u:
Setting up with specified i2c address 0x%x,
mid, i2caddr[0]);
sd = v4l2_i2c_new_subdev(hdw-v4l2_dev, hdw-i2c_adap,
- fname, fname,
+ NULL, fname,
i2caddr[0], NULL);
} else {
pvr2_trace(PVR2_TRACE_INIT,
@@ -2103,7 +2096,7 @@ static int pvr2_hdw_load_subdev(struct pvr2_hdw *hdw,
Setting up with address probe list,
mid);
sd = v4l2_i2c_new_subdev(hdw-v4l2_dev, hdw-i2c_adap,
- fname, fname,
+ NULL, fname,
0, i2caddr);
}
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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] V4L/DVB: pvrusb2: remove unneeded NULL checks
Based on the surrounding code (the unconditional dereference), I agree
that this particular bit of coding paranoia is not doing much good.
Acked-by: Mike Isely is...@pobox.com
On Thu, 19 Aug 2010, Dan Carpenter wrote:
We dereference maskptr unconditionally at the start of the function
and also inside the call to parse_tlist() towards the end of the
function. This function is called from store_val_any() and it always
passes a non-NULL pointer.
Signed-off-by: Dan Carpenter erro...@gmail.com
diff --git a/drivers/media/video/pvrusb2/pvrusb2-ctrl.c
b/drivers/media/video/pvrusb2/pvrusb2-ctrl.c
index 1b992b8..55ea914 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-ctrl.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-ctrl.c
@@ -513,7 +513,7 @@ int pvr2_ctrl_sym_to_value(struct pvr2_ctrl *cptr,
if (ret = 0) {
ret = pvr2_ctrl_range_check(cptr,*valptr);
}
- if (maskptr) *maskptr = ~0;
+ *maskptr = ~0;
} else if (cptr-info-type == pvr2_ctl_bool) {
ret = parse_token(ptr,len,valptr,boolNames,
ARRAY_SIZE(boolNames));
@@ -522,7 +522,7 @@ int pvr2_ctrl_sym_to_value(struct pvr2_ctrl *cptr,
} else if (ret == 0) {
*valptr = (*valptr 1) ? !0 : 0;
}
- if (maskptr) *maskptr = 1;
+ *maskptr = 1;
} else if (cptr-info-type == pvr2_ctl_enum) {
ret = parse_token(
ptr,len,valptr,
@@ -531,7 +531,7 @@ int pvr2_ctrl_sym_to_value(struct pvr2_ctrl *cptr,
if (ret = 0) {
ret = pvr2_ctrl_range_check(cptr,*valptr);
}
- if (maskptr) *maskptr = ~0;
+ *maskptr = ~0;
} else if (cptr-info-type == pvr2_ctl_bitmask) {
ret = parse_tlist(
ptr,len,maskptr,valptr,
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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 2/2] media: video: pvrusb2: remove custom hex_to_bin()
Andy:
Acked-By: Mike Isely is...@pobox.com
-Mike
On Tue, 27 Jul 2010, Andy Shevchenko wrote:
Signed-off-by: Andy Shevchenko andy.shevche...@gmail.com
Cc: Mike Isely is...@pobox.com
---
drivers/media/video/pvrusb2/pvrusb2-debugifc.c | 14 ++
1 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/media/video/pvrusb2/pvrusb2-debugifc.c
b/drivers/media/video/pvrusb2/pvrusb2-debugifc.c
index e9b11e1..4279ebb 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-debugifc.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-debugifc.c
@@ -94,8 +94,6 @@ static int debugifc_parse_unsigned_number(const char
*buf,unsigned int count,
u32 *num_ptr)
{
u32 result = 0;
- u32 val;
- int ch;
int radix = 10;
if ((count = 2) (buf[0] == '0')
((buf[1] == 'x') || (buf[1] == 'X'))) {
@@ -107,17 +105,9 @@ static int debugifc_parse_unsigned_number(const char
*buf,unsigned int count,
}
while (count--) {
- ch = *buf++;
- if ((ch = '0') (ch = '9')) {
- val = ch - '0';
- } else if ((ch = 'a') (ch = 'f')) {
- val = ch - 'a' + 10;
- } else if ((ch = 'A') (ch = 'F')) {
- val = ch - 'A' + 10;
- } else {
+ int val = hex_to_bin(*buf++);
+ if (val 0 || val = radix)
return -EINVAL;
- }
- if (val = radix) return -EINVAL;
result *= radix;
result += val;
}
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: Status of the patches under review at LMML (60 patches)
On Wed, 7 Jul 2010, Sven Barth wrote: Hi! Am 06.07.2010 15:06, schrieb Mauro Carvalho Chehab: == Waiting for Mike Iselyis...@isely.net review == Apr,25 2010: Problem with cx25840 and Terratec Grabster AV400 http://patchwork.kernel.org/patch/94960 Is Mike really the maintainer of the cx25840 module and not only of the pvrusb2 one? If he's not the maintainer you should contact the real one, cause I don't think that Mike can help much regarding patches for the cx25840 in that case. Also I might need to adjust the patch cause of the recent changes that happened there the last few months. (I don't know when I'll find time for this...) Regards, Sven No I am definitely not the maintainer of that module, and my knowledge of its inner workings (though improved a lot lately) is still not very good. All I can do here is verify that it doesn't break the pvrusb2 driver (which is what I was planning on doing). -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: Status of the patches under review at LMML (60 patches)
On Tue, 6 Jul 2010, Mauro Carvalho Chehab wrote:
This is the summary of the patches that are currently under review at
Linux Media Mailing List linux-media@vger.kernel.org.
Each patch is represented by its submission date, the subject (up to 70
chars) and the patchwork link (if submitted via email).
P.S.: This email is c/c to the developers where some action is expected.
If you were copied, please review the patches, acking/nacking or
submitting an update.
[...]
== Waiting for Mike Isely is...@isely.net review ==
Apr,25 2010: Problem with cx25840 and Terratec Grabster AV400
http://patchwork.kernel.org/patch/94960
These are cx25840 patches and I'm not the maintainer of that module. I
can't really speak to the correctness of the changes. Best I can do is
to try the patch with a few pvrusb2-driven devices here that use the
cx25840 module. I've done that now (HVR-1950 and PVR-USB2 model 24012)
and everything continues to work fine. Note, this part of the patch:
int hw_fix = state-pvr150_workaround;
-
- if (std == V4L2_STD_NTSC_M_JP) {
+ if (std == V4L2_STD_NTSC_M_JP) {
/* Japan uses EIAJ audio standard */
cx25840_write(client, 0x808, hw_fix ? 0x2f : 0xf7);
} else if (std == V4L2_STD_NTSC_M_KR) {
is a whitespace-only change which introduces a bogus tab and messes up
the indentation of that opening if-statement. It should probably be
removed from the patch. Other than that, you have my ack:
Acked-By: Mike Isely is...@pobox.com
-Mike
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: [git:v4l-dvb/other] V4L/DVB: drivers/media/video/pvrusb2: Add missing mutex_unlock
On Sat, 3 Jul 2010, Douglas Schilling Landgraf wrote: Hello Mike, Mike Isely wrote: Mauro: FYI, I posted an Acked-By: Mike Isely is...@pobox.com weeks ago, back on 27-May, immediately after the patch was posted. It's a great catch, and the bug has been there since basically the beginning of the driver. Was I ever supposed to see any kind of reaction to that ack (e.g. having the Acked-By added to the patch)? I had posted it in reply to the original patch, copied back to the patch author, to lkml, to linux-media, kernel-janitors, and Mauro. -Mike It seems my mistake since I have added CC instead of Acked-by, sorry. This happened because usually I add CC to the authors of drivers when I took patches from patchwork and I wanna notify them. In your case, I missed the acked-by. Mauro, if possible, could you please replace CC to the correct Acked-by before submit this patch to Linus? Hmm, going through my old e-mail now I can see that the patch was picked up for -mm on 1-Jun. At that time I was marked as a CC: for the patch - which I'd expect as the driver maintainer. But no Acked-By: was showing. Maybe that's when the ack got missed. Obviously I have no issue with this patch. My only real concern is that nobody thinks I might have been ignoring it. Thanks for following up. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: Subject: Composite input from OnAir Creator - use as security camera
doesn't seem to be in Lenny. Months (years?) ago, I did manage to get MythTV to work with analog signals but the reception was horrible so I gave up. Watching TV is not a high priority and I never installed the amplified roof antenna. I never figured out how to select the composite or S-video inputs from within MythTV. o xawtv xawtv doesn't have a norm for NTSC (only PAL and SECAM) so even though I can select the composite input: no picture. It also says: v4l-conf had some trouble, trying to continue anyway Warning: Cannot convert string -*-ledfixed-medium-r-*--39-*-*-*-c-*-*-* to type FontStruct v4l2: read: rc=262144/size=442368 So, what else can I try? Any hints welcome. Thanks. A. -- 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 -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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 5/17] drivers/media/video/pvrusb2: Add missing mutex_unlock
I looked through my revision history and that bug has been there in the
driver source since at least May 2005, long before it was ever merged
into the kernel. Wow, what a great catch. Thanks!
Acked-By: Mike Isely is...@pobox.com
-Mike
On Wed, 26 May 2010, Julia Lawall wrote:
From: Julia Lawall ju...@diku.dk
Add a mutex_unlock missing on the error path. In the other functions in
the same file the locks and unlocks of this mutex appear to be balanced,
so it would seem that the same should hold in this case.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// smpl
@@
expression E1;
@@
* mutex_lock(E1,...);
+... when != E1
if (...) {
... when != E1
* return ...;
}
...+
* mutex_unlock(E1,...);
// /smpl
Signed-off-by: Julia Lawall ju...@diku.dk
---
drivers/media/video/pvrusb2/pvrusb2-ioread.c |5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/media/video/pvrusb2/pvrusb2-ioread.c
b/drivers/media/video/pvrusb2/pvrusb2-ioread.c
index b482478..bba6115 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-ioread.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-ioread.c
@@ -223,7 +223,10 @@ int pvr2_ioread_setup(struct pvr2_ioread *cp,struct
pvr2_stream *sp)
pvr2_ioread_setup (setup) id=%p,cp);
pvr2_stream_kill(sp);
ret = pvr2_stream_set_buffer_count(sp,BUFFER_COUNT);
- if (ret 0) return ret;
+ if (ret 0) {
+ mutex_unlock(cp-mutex);
+ return ret;
+ }
for (idx = 0; idx BUFFER_COUNT; idx++) {
bp = pvr2_stream_get_buffer(sp,idx);
pvr2_buffer_set_buffer(bp,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
Mauro: You are reading too much into that comment. I never said it was valid to do what had been done, only that for the longest time this is what the driver did and it never caused a problem that I was made aware of. What I said there was correct, that this is what the driver had been doing in the past, that it's definitely causing a problem now and thus that is why this patch exists. I'd really rather you not mess with my comment. Probably too late however. -Mike On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: Mike Isely wrote: Please from http://linuxtv.org/hg/~mcisely/pvrusb2-patches for the following pvrusb2 driver fixes / improvements: - pvrusb2: Minor debug code fixup - pvrusb2: Fix Gotview hardware support - pvrusb2: Avoid using stack allocated buffers when performing USB I/O Your comment for this patch is wrong: pvrusb2: The pvrusb2 driver has for the longest time used a (tiny) stack allocated buffer for some of its I/O with the hardware. Apparently later kernels don't like this behavior and trap it at run-time, causing nasty complaints to the kernel log. This trivial change fixes the one case in the driver where this had been happening. It were never valid to use stack for DMA, as kernel provides no warranty that the stack would be on a page that can do DMA. In a matter of fact, as most x86 USB drivers accept DMA at the first 3Gb of RAM space, this bug is generally not noticed on i386/x86_64 archs. Yet, if your machine has more than 3Gb, there are some chances that the stack would be at the HIGHMEM area, where DMA is not supported by the processor. As this is a common error, newer kernels have some instrumentation support to warn about such troubles. I'll be fixing the comment. -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: Mike Isely wrote: On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: Mike Isely wrote: Mauro: You are reading too much into that comment. I never said it was valid to do what had been done, only that for the longest time this is what the driver did and it never caused a problem that I was made aware of. What I said there was correct, that this is what the driver had been doing in the past, that it's definitely causing a problem now and thus that is why this patch exists. As I said, this is not right: Apparently later kernels don't like this behavior Mauro: That statement was in reference to the fact that previously the problem had gone undetected, but now later kernels can notice and complain about this, thus later kernels don't like this behavior. We can debate that perhaps the statement can be worded better, but that doesn't make it *wrong*. Calling 2.6.12 kernel as later kernels doesn't seem right to me (that was about the kernel were em28xx driver were introduced). The point when the em28xx driver appeared has nothing to do with this. The point when the kernel started complaining about the use of a stack based USB I/O buffers is the relevant point, which was not back in 2.6.12. I learned of this behavior (that is, receiving warnings about the usage) as being new in the 2.6.34 timeframe, the point when a user pointed out the complaint message in his kernel log; at that time I had not yet tested against that kernel version. It is not later kernels. DMA over stack were never supported. Your driver had a bug that you didn't noticed for long time, probably because nobody reported you this issue, since it appears only on some non-Intel archs and on i386 with more than 3.12 Gb of RAM, and when the stack happens to be after the first 3.12 Gb (with is a somewhat rare condition). I understand your point perfectly that this was never right or valid. In fact, I also understood that point long before you decided to explain it to me here - after all my realization of this problem in the driver is why I wrote the patch in the first place. Absolutely no argument there about the importance of the change. None of that however justifies putting words into an author's mouth, which is effectively what you did by replacing that commit comment. First of all, it is clearly stated at the patch that the description were changed and by whom: [mche...@redhat.com: fix patch description] Second, it is an usual upstream practice to fix descriptions where needed. The patch description is a precious resource for people that are seeking for similar bugs, and for those that are trying to understand some code. So, it is important to not send broken/incomplete comments to kernel, or comments that may have a dubious interpretation. So, subsystem maintainers frequently need to fix comments. Anyway, to end this discussion, I can simply revert the patch from the staging tree, waiting for a new patch from you with a fixed comment. Also, if you prefer, next time, I won't apply any patch from you if I found a bad comment without your ack, even if it means that you'll probably loose a merge window. Leave it as is. What's done is done. Your replaced comment is of course still correct. I would just appreciate some better sensitivity in the future about replacing authors' comments, especially since in this case your interpretation of my original comment was wrong. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: Mike Isely wrote: [snip] The point when the kernel started complaining about the use of a stack based USB I/O buffers is the relevant point, which was not back in 2.6.12. I learned of this behavior (that is, receiving warnings about the usage) as being new in the 2.6.34 timeframe, the point when a user pointed out the complaint message in his kernel log; at that time I had not yet tested against that kernel version. Older kernels also complain if the stack were actually out of the DMA range and you try to program DMA there. Yet, only now we've seen consumer PC's with lots of RAM. One of my test targets is a PC in 32 bit mode with 4GB of RAM; strange then that I've never seen the kernel complain there. The bad buffer has been in the driver for even longer than that and nobody raised the issue to me until about a month ago (the fix was created back then but for other reasons that you already know it didn't become available in -hg until last week). Leave it as is. What's done is done. Your replaced comment is of course still correct. I would just appreciate some better sensitivity in the future about replacing authors' comments, especially since in this case your interpretation of my original comment was wrong. Ok. Thanks. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
Please from http://linuxtv.org/hg/~mcisely/pvrusb2-patches for the following pvrusb2 driver fixes / improvements: - pvrusb2: Minor debug code fixup - pvrusb2: Fix Gotview hardware support - pvrusb2: Avoid using stack allocated buffers when performing USB I/O - pvrusb2: New feature to mark specific hardware support as experimental - pvrusb2: Fix kernel oops at device unregistration - pvrusb2: Fix missing header include - pvrusb2: Fix USB parent device reference count - pvrusb2: Fix minor internal array allocation - pvrusb2: Fix kernel oops on device tear-down - pvrusb2: Call sysfs_attr_init() appropriately... pvrusb2-devattr.c |1 pvrusb2-devattr.h |5 pvrusb2-hdw.c | 26 + pvrusb2-main.c|4 +-- pvrusb2-sysfs.c | 64 +++--- pvrusb2-v4l2.c| 16 ++--- 6 files changed, 107 insertions(+), 9 deletions(-) These are primarily a collection of stability fixes. Thanks, -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: Problem with cx25840 and Terratec Grabster AV400
On Sat, 24 Apr 2010, Sven Barth wrote: On 24.04.2010 19:13, Mike Isely wrote: Actually the support in the pvrusb2 driver was never really completed. But since I don't have a sample of the hardware here I went on ahead and merged what was there so that it could get exposure and the remaining problems sorted out. -Mike Hi! Although you never really completed that support for the AV400 it runs pretty well once you've touched the cx25840 source. I'm using it for months now and it runs better than it did with Windows (I sometimes had troubles with audio there which led to an out of sync audio track). Unfortunately I can't really say it is supported in the pvrusb2 driver until it actually works well enough that a user doesn't have to hack driver source (pvrusb2 or otherwise). Otherwise I'm just going to get inundated with help requests for this. Not having a sample of the device here I'm handicapped from debugging such issues. I've just made a change to the pvrusb2 driver to allow for the ability to mark a piece of hardware (such as this device) as experimental. Such devices will generate a warning in the kernel log upon initialization. The experimental marker doesn't impact the ability to use the device; it just triggers the warning message. Once we know the device is working acceptably well enough, the marker can be turned off. This should help avoid misleading others about whether or not the pvrusb2 driver fully supports a particular piece of hardware. I wrote the last mail, because I want to sort out why the cx25837 chip in my device is behaving differently than expected by the corresponding driver and to remove the need to patch the v4l sources manually. Once I don't need to fear that the next system update breaks the device again (because cx25840.ko is overwritten), I'm more then willed to help you to complete the support for my device in your driver (feature testing, etc). We definitely need to do this. Regards, Sven PS: Did you read my mail from last December? http://www.isely.net/pipermail/pvrusb2/2009-December/002716.html Yeah, I saw it back then, and then I probably got distracted away :-( The key issue is that your hardware doesn't seem to work until you make those two changes to the v4l-dvb cx25840 driver. Obviously one can't just make those changes without understanding the implications for other users of the driver. I (or someone expert at the cx25840 module) needs to study that patch and understand what is best to do for the driver. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: Problem with cx25840 and Terratec Grabster AV400
On Sat, 24 Apr 2010, Sven Barth wrote: Hi! On 24.04.2010 22:24, Mike Isely wrote: On Sat, 24 Apr 2010, Sven Barth wrote: Hi! Although you never really completed that support for the AV400 it runs pretty well once you've touched the cx25840 source. I'm using it for months now and it runs better than it did with Windows (I sometimes had troubles with audio there which led to an out of sync audio track). Unfortunately I can't really say it is supported in the pvrusb2 driver until it actually works well enough that a user doesn't have to hack driver source (pvrusb2 or otherwise). Otherwise I'm just going to get inundated with help requests for this. Not having a sample of the device here I'm handicapped from debugging such issues. I don't want to have this hacking as much as you do. But currently it's the only way that works for me (I'm really glad that it has come that far ^^)... I'll try to help here as good as I can (and time permits) to solve this issue. I understand. I've just made a change to the pvrusb2 driver to allow for the ability to mark a piece of hardware (such as this device) as experimental. Such devices will generate a warning in the kernel log upon initialization. The experimental marker doesn't impact the ability to use the device; it just triggers the warning message. Once we know the device is working acceptably well enough, the marker can be turned off. This should help avoid misleading others about whether or not the pvrusb2 driver fully supports a particular piece of hardware. No offense intended, but do you really think that people will read that? Normal users (using Ubuntu, etc) don't really care whether their device is marked as experimental or not... they just want it to work and thus can go to great lengths to disturb the developers working on their driver... No offense taken. Not a problem. But I felt it was at least important enough for the driver to document this fact. For those who use the device who are capable of attempting some hacking - those people WILL see the message and hopefully that will encourage such folks to contact the author (me) for assistance in further stabilizing the device. The intent wasn't for the flag to be any excuse not to work on it - I just want to leave a marker indicating that the driver is not expected to be fully working (or supported) at this time. PS: Did you read my mail from last December? http://www.isely.net/pipermail/pvrusb2/2009-December/002716.html Yeah, I saw it back then, and then I probably got distracted away :-( I know that problem pretty well. ^^ I was only curious. Spending a lot of time today catching up on stuff like this. Just smoked out two kernel oopses in the driver today as well. The key issue is that your hardware doesn't seem to work until you make those two changes to the v4l-dvb cx25840 driver. Obviously one can't just make those changes without understanding the implications for other users of the driver. I (or someone expert at the cx25840 module) needs to study that patch and understand what is best to do for the driver. -Mike It would be interesting to know why the v4l devs disabled the audio routing for cx2583x chips and whether it was intended that a cx25837 chip gets the same treatment as a e.g. cx25836. I wish I could provide specific information about that :-( And those implications you're talking about is the reason why I wrote here: I want to check whether there is a better or more correct way than to disable those checks (it works here, because I have only that one device that contains a cx2583x chip...). Just a thought: can it be that my chip's audio routing isn't set to the correct value after initialization and thus it needs to be set at least once, while all other chips default to a working routing after initialization? Could be a design mistake done by Terratec... There is no one correct audio routing. And by audio routing I mean the wiring between the chip and the various audio inputs that feed it. The choice for how to route all this is up to the vendor of the device. In many cases there is a common reference design that the vendor starts from, in which case such routing will be more common across devices. But that's just luck really. The cx25840 driver provides an API to things like the pvrusb2 driver to select the proper routing based on that bridge driver's knowledge of the surrounding hardware. This is one of the areas that have to be worked on when porting to a new device. The PVR2_ROUTING_SCHEME_ enumeration in the pvrusb2 driver is part of this. With that all said I haven't looked closely enough at your patch to the cx25840 module so I'm only assuming that we're talking about the same thing here. I have a funny feeling that you're hitting on something else however. I need to look at this more
Re: [PATCH] device_attributes: add sysfs_attr_init() for dynamic attributes
Acked-By: Mike Isely is...@pobox.com
(in the context of the pvrusb2 driver related changes)
-Mike
On Mon, 22 Mar 2010, Wolfram Sang wrote:
Made necessary by 6992f5334995af474c2b58d010d08bc597f0f2fe.
Found by this semantic patch:
@ init @
type T;
identifier A;
@@
T {
...
struct device_attribute A;
...
};
@ main extends init @
expression E;
statement S;
identifier err;
T *name;
@@
... when != sysfs_attr_init(name-A.attr);
(
+ sysfs_attr_init(name-A.attr);
if (device_create_file(E, name-A))
S
|
+ sysfs_attr_init(name-A.attr);
err = device_create_file(E, name-A);
)
While reviewing, I put the initialization to apropriate places.
Signed-off-by: Wolfram Sang w.s...@pengutronix.de
Cc: Eric W. Biederman ebied...@xmission.com
Cc: Greg KH gre...@suse.de
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Mike Isely is...@pobox.com
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Sujith Thomas sujith.tho...@intel.com
Cc: Matthew Garrett m...@redhat.com
---
The thermal-sys.c-part should fix bugs #15548 and #15584.
drivers/macintosh/windfarm_core.c |1 +
drivers/media/video/pvrusb2/pvrusb2-sysfs.c |8
drivers/platform/x86/intel_menlow.c |1 +
drivers/thermal/thermal_sys.c |1 +
drivers/video/fsl-diu-fb.c |1 +
5 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/macintosh/windfarm_core.c
b/drivers/macintosh/windfarm_core.c
index 419795f..f447642 100644
--- a/drivers/macintosh/windfarm_core.c
+++ b/drivers/macintosh/windfarm_core.c
@@ -209,6 +209,7 @@ int wf_register_control(struct wf_control *new_ct)
kref_init(new_ct-ref);
list_add(new_ct-link, wf_controls);
+ sysfs_attr_init(new_ct-attr.attr);
new_ct-attr.attr.name = new_ct-name;
new_ct-attr.attr.mode = 0644;
new_ct-attr.show = wf_show_control;
diff --git a/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
b/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
index 6c23456..71f5056 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
@@ -423,10 +423,12 @@ static void pvr2_sysfs_add_debugifc(struct pvr2_sysfs
*sfp)
dip = kzalloc(sizeof(*dip),GFP_KERNEL);
if (!dip) return;
+ sysfs_attr_init(dip-attr_debugcmd.attr);
dip-attr_debugcmd.attr.name = debugcmd;
dip-attr_debugcmd.attr.mode = S_IRUGO|S_IWUSR|S_IWGRP;
dip-attr_debugcmd.show = debugcmd_show;
dip-attr_debugcmd.store = debugcmd_store;
+ sysfs_attr_init(dip-attr_debuginfo.attr);
dip-attr_debuginfo.attr.name = debuginfo;
dip-attr_debuginfo.attr.mode = S_IRUGO;
dip-attr_debuginfo.show = debuginfo_show;
@@ -644,6 +646,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
return;
}
+ sysfs_attr_init(sfp-attr_v4l_minor_number.attr);
sfp-attr_v4l_minor_number.attr.name = v4l_minor_number;
sfp-attr_v4l_minor_number.attr.mode = S_IRUGO;
sfp-attr_v4l_minor_number.show = v4l_minor_number_show;
@@ -658,6 +661,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
sfp-v4l_minor_number_created_ok = !0;
}
+ sysfs_attr_init(sfp-attr_v4l_radio_minor_number.attr);
sfp-attr_v4l_radio_minor_number.attr.name = v4l_radio_minor_number;
sfp-attr_v4l_radio_minor_number.attr.mode = S_IRUGO;
sfp-attr_v4l_radio_minor_number.show = v4l_radio_minor_number_show;
@@ -672,6 +676,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
sfp-v4l_radio_minor_number_created_ok = !0;
}
+ sysfs_attr_init(sfp-attr_unit_number.attr);
sfp-attr_unit_number.attr.name = unit_number;
sfp-attr_unit_number.attr.mode = S_IRUGO;
sfp-attr_unit_number.show = unit_number_show;
@@ -685,6 +690,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
sfp-unit_number_created_ok = !0;
}
+ sysfs_attr_init(sfp-attr_bus_info.attr);
sfp-attr_bus_info.attr.name = bus_info_str;
sfp-attr_bus_info.attr.mode = S_IRUGO;
sfp-attr_bus_info.show = bus_info_show;
@@ -699,6 +705,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
sfp-bus_info_created_ok = !0;
}
+ sysfs_attr_init(sfp-attr_hdw_name.attr);
sfp-attr_hdw_name.attr.name = device_hardware_type;
sfp-attr_hdw_name.attr.mode = S_IRUGO;
sfp-attr_hdw_name.show = hdw_name_show;
@@ -713,6 +720,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
sfp-hdw_name_created_ok = !0;
}
+ sysfs_attr_init(sfp-attr_hdw_desc.attr);
sfp-attr_hdw_desc.attr.name = device_hardware_description;
sfp-attr_hdw_desc.attr.mode = S_IRUGO;
sfp-attr_hdw_desc.show = hdw_desc_show
Re: [PATCH 09/11] pvrusb2-v4l2: Rename dev_info to pdi
Acked-By: Mike Isely is...@pobox.com
-Mike
On Mon, 5 Apr 2010, Joe Perches wrote:
There is a macro called dev_info that prints struct device specific
information. Having variables with the same name can be confusing and
prevents conversion of the macro to a function.
Rename the existing dev_info variables to something else in preparation
to converting the dev_info macro to a function.
Signed-off-by: Joe Perches j...@perches.com
---
drivers/media/video/pvrusb2/pvrusb2-v4l2.c | 22 +++---
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
b/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
index cc8ddb2..ba32c91 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-v4l2.c
@@ -48,7 +48,7 @@ struct pvr2_v4l2_dev {
struct pvr2_v4l2_fh {
struct pvr2_channel channel;
- struct pvr2_v4l2_dev *dev_info;
+ struct pvr2_v4l2_dev *pdi;
enum v4l2_priority prio;
struct pvr2_ioread *rhp;
struct file *file;
@@ -161,7 +161,7 @@ static long pvr2_v4l2_do_ioctl(struct file *file,
unsigned int cmd, void *arg)
{
struct pvr2_v4l2_fh *fh = file-private_data;
struct pvr2_v4l2 *vp = fh-vhead;
- struct pvr2_v4l2_dev *dev_info = fh-dev_info;
+ struct pvr2_v4l2_dev *pdi = fh-pdi;
struct pvr2_hdw *hdw = fh-channel.mc_head-hdw;
long ret = -EINVAL;
@@ -563,14 +563,14 @@ static long pvr2_v4l2_do_ioctl(struct file *file,
unsigned int cmd, void *arg)
case VIDIOC_STREAMON:
{
- if (!fh-dev_info-stream) {
+ if (!fh-pdi-stream) {
/* No stream defined for this node. This means
that we're not currently allowed to stream from
this node. */
ret = -EPERM;
break;
}
- ret = pvr2_hdw_set_stream_type(hdw,dev_info-config);
+ ret = pvr2_hdw_set_stream_type(hdw,pdi-config);
if (ret 0) return ret;
ret = pvr2_hdw_set_streaming(hdw,!0);
break;
@@ -578,7 +578,7 @@ static long pvr2_v4l2_do_ioctl(struct file *file,
unsigned int cmd, void *arg)
case VIDIOC_STREAMOFF:
{
- if (!fh-dev_info-stream) {
+ if (!fh-pdi-stream) {
/* No stream defined for this node. This means
that we're not currently allowed to stream from
this node. */
@@ -1031,7 +1031,7 @@ static int pvr2_v4l2_open(struct file *file)
}
init_waitqueue_head(fhp-wait_data);
- fhp-dev_info = dip;
+ fhp-pdi = dip;
pvr2_trace(PVR2_TRACE_STRUCT,Creating pvr_v4l2_fh id=%p,fhp);
pvr2_channel_init(fhp-channel,vp-channel.mc_head);
@@ -1112,7 +1112,7 @@ static int pvr2_v4l2_iosetup(struct pvr2_v4l2_fh *fh)
struct pvr2_hdw *hdw;
if (fh-rhp) return 0;
- if (!fh-dev_info-stream) {
+ if (!fh-pdi-stream) {
/* No stream defined for this node. This means that we're
not currently allowed to stream from this node. */
return -EPERM;
@@ -1121,21 +1121,21 @@ static int pvr2_v4l2_iosetup(struct pvr2_v4l2_fh *fh)
/* First read() attempt. Try to claim the stream and start
it... */
if ((ret = pvr2_channel_claim_stream(fh-channel,
- fh-dev_info-stream)) != 0) {
+ fh-pdi-stream)) != 0) {
/* Someone else must already have it */
return ret;
}
- fh-rhp = pvr2_channel_create_mpeg_stream(fh-dev_info-stream);
+ fh-rhp = pvr2_channel_create_mpeg_stream(fh-pdi-stream);
if (!fh-rhp) {
pvr2_channel_claim_stream(fh-channel,NULL);
return -ENOMEM;
}
hdw = fh-channel.mc_head-hdw;
- sp = fh-dev_info-stream-stream;
+ sp = fh-pdi-stream-stream;
pvr2_stream_set_callback(sp,(pvr2_stream_callback)pvr2_v4l2_notify,fh);
- pvr2_hdw_set_stream_type(hdw,fh-dev_info-config);
+ pvr2_hdw_set_stream_type(hdw,fh-pdi-config);
if ((ret = pvr2_hdw_set_streaming(hdw,!0)) 0) return ret;
return pvr2_ioread_set_enabled(fh-rhp,!0);
}
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: RFC: exposing controls in sysfs
On Thu, 8 Apr 2010, hermann pitton wrote: Hi, Am Mittwoch, den 07.04.2010, 20:50 +0200 schrieb Lars Hanisch: Am 06.04.2010 16:33, schrieb Mike Isely: [snip] Mike, do you know of anyone actively using that additional information? Yes. The VDR project at one time implemented a plugin to directly interface to the pvrusb2 driver in this manner. I do not know if it is still being used since I don't maintain that plugin. Just FYI: The PVR USB2 device is now handled by the pvrinput-plugin, which uses only ioctls. The old pvrusb2-plugin is obsolete. http://projects.vdr-developer.org/projects/show/plg-pvrinput Lars: Thanks for letting me know about that - until this message I had no idea if VDR was still using that interface. Regards, Lars. [snip] thanks Lars. Mike is really caring and went out for even any most obscure tuner bit to help to improve such stuff in the past, when we have been without any data sheets. Hermann: You might have me confused with Mike Krufky there - he's the one who did so much of the tuner driver overhauling in v4l-dvb in the past. To open second, maybe third and even forth ways for apps to use a device, likely going out of sync soon, does only load maintenance work without real gain. Well it was an experiment at the time to see how well such a concept would work. I had done it in a way to minimize maintenance load going forward. On both counts I feel the interface actually has done very well, nonstandard though it may be. I still get the general impression that the user community really has liked the sysfs interface, but the developers never really got very fond of it :-( We should stay sharp to discover something others don't want to let us know about. All other ideas about markets are illusions. Or? So, debugfs sounds much better than sysfs for my taste. Any app and any driver, going out of sync on the latter, will remind us that backward compat _must always be guaranteed_ ... Or did change anything on that and is sysfs excluded from that rule? Backwards compatibility is very important and thus any kind of new interface deserves a lot of forethought to ensure that choices are made in the present that people will regret in the future. Making an interface self-describing is one way that helps with compatibility: if the app can discover on its own how to use the interface then it can adapt to interface changes in the future. I think a lot of people get their brains so wrapped around the ioctl-way of doing things and then they try to map that concept into a sysfs-like (or debugfs-like) abstraction that they don't see how to naturally take advantage of what is possible there. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: RFC: exposing controls in sysfs
On Wed, 7 Apr 2010, Hans Verkuil wrote: [...] Perhaps we should just not do this in sysfs at all but in debugfs? We have a lot more freedom there. No requirement of one-value-per-file, and if we need to we can change things in the future. It would actually be easier to issue ioctl commands to a driver from debugfs since we have a proper struct file there. It could be implemented as a separate module that can be loaded if debugfs is enabled and suddenly you have all this extra debug functionality. I admit, I would really enjoy writing something like this. I just don't want to do this in sysfs as that makes it too 'official' so to speak. In other words, mainline applications should not use sysfs, but home-grown scripts are free to use it as far as I am concerned. How much of a problem would that be for you, Mike? On the one hand users have to mount debugfs, but on the other hand it will be consistent for all drivers that use the control framework. And you should be able to ditch a substantial amount of code :-) Adding a debugfs interface that can be used by all V4L drivers is obviously a concept I would not have any problem with. However that does not necessarily mean that I would agree with eventual removal of the pvrusb2 driver's existing sysfs interface. That would depend on whether or not doing such a thing loses functionality and what the driver's user community would think about it. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: RFC: exposing controls in sysfs
is that it is (within limits) self-describing. For each control you get not only the ability to get (and usually set) its value, but you can inspect its limits, learn what the default value is, retrieve a description (though being english only that has limited utility), and discover its type. The sysfs interface in the driver tries to use symbolic values where possible, including when setting / clearing bits in a bit mask. The fact that such information is there helps when writing, say, a generic dialog-based wrapper to provide a TUI (Text User Interface) over the interface without having to encode too much specific information in the wrapper itself. Thus if new controls get added (or changed) later then such things just automatically adapt. Of course this isn't a perfect thing, but it helps. Comments? Ideas? Once we commit to something it is there for a long time to come since sysfs is after all a public API (although it seems to be more changable than ioctls). I have found it useful over a long period of time. I also suggest that if such an interface is defined in the general case that it should include some introspection capabilities, which will make it easier (or even possible) to evolve the interface over time while minimizing backwards compatibility issues. If such a generic interface were made available, I could see an argument to remove the sysfs interface from the pvrusb2 driver. HOWEVER there is a community using it, and also unless the generic interface were a complete replacement, the pvrusb2 piece will probably have to stay. (Note: the sysfs interface in the pvrusb2 driver is already a CONFIG_XX parameter so it's easy to deselect it when building the driver.) -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: RFC: exposing controls in sysfs
On Tue, 6 Apr 2010, Hans Verkuil wrote: [...] One thing that might be useful is to prefix the name with the control class name. E.g. hue becomes user_hue and audio_crc becomes mpeg_audio_crc. It would groups them better. Or one could make a controls/user and controls/mpeg directory. That might not be such a bad idea actually. I agree with grouping in concept, and using subdirectories is not a bad thing. Probably however you'd want to ensure that in the end all the controls end up logically at the same depth in the tree. [...] An in between solution would be to add _type files. So you would have 'hue' and 'hue_type'. 'cat hue_type' would give something like: int 0 255 1 128 0x Hue In other words 'type min max step flags name'. There was I thought at some point in the past a kernel policy that sysfs controls were supposed to limit themselves to one value per node. And for menu controls like stream_type (hmm, that would become stream_type_type...) you would get: menu 0 5 1 0 Stream Type MPEG-2 Program Stream MPEG-1 System Stream MPEG-2 DVD-compatible Stream MPEG-1 VCD-compatible Stream MPEG-2 SVCD-compatible Stream Note the empty line to denote the unsupported menu item (transport stream). This would give the same information with just a single extra file. Still not sure whether it is worth it though. Just remember that the more complex / subtle you make the node contents, then the more parsing will be required for any program that tries to use it. I also think it's probably a bad idea for example to define a format where the whitespace conveys additional information. The case where I've seen whitespace as part of the syntax actually work cleanly is in Python. -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: RFC: exposing controls in sysfs
On Tue, 6 Apr 2010, Devin Heitmueller wrote: [...] I tend to agree with Hans. We've already got *too many* interfaces that do the same thing. The testing matrix is already a nightmare - V4L1 versus V4L2, mmap() versus read(), legacy controls versus extended controls, and don't get even me started on VBI. We should be working to make drivers and interfaces simpler, with *fewer* ways of doing the same thing. The flexibility of providing yet another interface via sysfs compared to just calling v4l2-ctl just isn't worth the extra testing overhead. We've already got too much stuff that needs to be fixed and not enough good developers to warrant making the code more complicated with little tangible benefit. If another API (e.g. sysfs) is defined and it is specifically NOT permitted to be a complete set, then one can ultimately end up with situations where in order to effectively use a driver then multiple APIs *must* be used by the application. That's even worse. This situation already exists in the pvrusb2 driver and it's not because of sysfs - it's because of V4L and DVB. When the pvrusb2 driver is used to handle a hybrid device (such as the HVR-1950) one has to use both the DVB and V4L APIs in order to effectively operate the device. This is because both APIs provide something not available in the other. And this really sucks if all the user wants to do is stream mpeg, darn it! And I don't care if it is digital or analog. I think that situation is very wrong; given that the HVR-1950 can spit out mpeg in either mode the user shouldn't be forced to make his application choice based on which mode he wants. There's only ONE application out there that allows the user to operate an HVR-1950 without being forced to deal with this: MythTV, and that's because, well, MythTV implements both APIs: V4L and DVB. I really, really dislike situations that arise where multiple APIs are *required* to operate a device, when really there should just be one API. That said, if multiple APIs are to be exported by the driver interface, then such APIs really should be as complete as possible in order to avoid potential problems later where because of previous limiting choices of API design now multiple APIs become required. I agree that testing against multiple APIs can be a pain and a drain on effort. But that has not happened with the pvrusb2 driver. It should be possible to implement the API in a way that minimizes further thrashing due to driver changes. The pvrusb2 sysfs implementation there is programmatically created when the driver comes up. The code which implements that interface really doesn't have any logic specific to particular API functions; it is just a reflection of what is internally in the driver. If new knobs are added to the pvrusb2 driver, then the knob automatically appears in the sysfs interface. If you were to go through the change history of the pvrusb2-sysfs.c module, all you're really going to find are changes caused by the sysfs class environment itself (i.e. when struct class was morphed into struct device), not the driver or its functionality. And nobody I've talked to who writes applications that work with V4L has been screaming OMG, if only V4L had a sysfs interface to manage controls! The experience I've seen with users and the pvrusb2 interface is that once they discover the sysfs API, the response is in fact very positive. Most users of the driver had no concept that such a thing was even possible until they were exposed to it. Now that's not to say that we should all be screaming for this - but if people didn't really understand what was possible, then how could they ask for it? -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: RFC: exposing controls in sysfs
On Tue, 6 Apr 2010, Markus Rechberger wrote: [...] how about security permissions? while you can easily change the permission levels for nodes in /dev you can't do this so easily with sysfs entries. I don't really think this is needed at all some applications will start to use ioctl some other apps might go for sysfs.. this makes the API a little bit whacko This is an excellent point. I should have brought this up sooner. The driver has control over the modes of the nodes in sysfs. The driver does NOT have control over the owner / group of those nodes. It is possible to change the owner / group from userspace, and I *think* it's possible to create a udev rule to do this, but honestly I have not investigated this possibility so I don't fully know. This is one serious potential drawback to using sysfs as a driver API. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: RFC: exposing controls in sysfs
On Tue, 6 Apr 2010, Laurent Pinchart wrote: Hi Andy, On Tuesday 06 April 2010 13:06:18 Andy Walls wrote: On Tue, 2010-04-06 at 08:37 +0200, Hans Verkuil wrote: [snip] Again, I still don't know whether we should do this. It is dangerously seductive because it would be so trivial to implement. It's like watching ships run aground on a shallow sandbar that all the locals know about. The waters off of 'Point /sys' are full of usability shipwrecks. I don't know if it's some siren's song, the lack of a light house, or just strange currents that deceive even seasoned navigators Let the user run 'v4l2-ctl -d /dev/videoN -L' to learn about the control metatdata. It's not as easy as typing 'cat', but the user base using sysfs in an interactive shell or shell script should also know how to use v4l2-ctl. In embedded systems, the final system deployment should not need the control metadata available from sysfs in a command shell anyway. I fully agree with this. If we push the idea one step further, why do we need to expose controls in sysfs at all ? I have found it useful to have the sysfs interface within the pvrusb2 driver. If it is going to take a lot of work to specifically craft a sysfs interface that exports the V4L API, then it will probably be a pain to maintain going forward. By a lot of work I mean that each V4L API function would have to be explicitly coded for in this interface, thus as the V4L API evolves over time then extra work must be expended each time to keep the sysfs interface in step. If that is to be the case then it may not be worth it. In the pvrusb2 driver this has not been the case because the code I wrote which implements the sysfs interface for the driver does this programmatically. That is, there is nothing in the pvrusb2-sysfs.c module which is specific to a particular function. Instead, when the module initializes it is able to enumerate the API on its own and generate the appropriate interface for each control it finds. Thus as the pvrusb2 driver's implementation has evolved over time, the sysfs implementation has simply continues to do its job, automatically reflecting internal changes without any extra work in that module's code. I don't know if that same strategy could be done in the V4L core. If it could, then this would probably alleviate a lot of concerns about testing / maintenance going forward. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: OnAir USB HDTV Creator
On Fri, 26 Feb 2010, Dean wrote: I am trying to use an 'OnAir USB HDTV Creator' (from autumnwave.com). According to http://www.linuxtv.org/wiki/index.php/OnAir_USB_HDTV_Creator This device is supported, however it's not working for me. Following the instructions at above link, I tried this: modprobe pvrusb2 initusbreset=0 The result: FATAL: Error inserting pvrusb2 (/lib/modules/2.6.31.12-desktop586-1mnb/kernel/drivers/media/video/pvrusb2/pvrusb2.ko.gz): Unknown symbol in module, or unknown parameter (see dmesg) Dean: The initusbreset module option no longer exists. That's why your modprobe command is failing. That feature was removed from the driver, due to a change in USB stack behavior that started with the 2.6.27 kernel. (The resolution hinted at in the wiki page was in fact just removal of the feature.) So you need to not use initusbreset=0. The advice in the wiki is over a year out of date. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
Please from http://linuxtv.org/hg/~mcisely/pvrusb2-patches for the following pvrusb2 driver fixes / improvements: - pvrusb2: Enforce a 300msec stabilization interval during stream strart - pvrusb2: Reduce encoder quiet period - pvrusb2: Adjust 300msec digitizer wait to be more selective pvrusb2-hdw-internal.h | 12 - pvrusb2-hdw.c | 61 - pvrusb2-hdw.h |1 3 files changed, 61 insertions(+), 13 deletions(-) These fixes improve mpeg streaming startup stability for any pvrusb2-driven device which contains an saa7115 video digitizer. Thanks, -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: Do any drivers access the cx25840 module in an atomic context?
On Mon, 18 Jan 2010, Andy Walls wrote: On Mon, 2010-01-18 at 14:18 -0600, Mike Isely wrote: On Mon, 18 Jan 2010, Andy Walls wrote: Any definitive confirmation anyone can give on any of these drivers would be helpful and would save me some time. Mike, Great! Thank you for the answers. You're welcome. If you would indulge one more (compound) question: Looking at the I2C master implementation in pvrusb2, it looks like it would be OK for me to combine the i2c_master_write() and i2c_master_read() in cx25840_read() into a single 2 msg i2c_transfer() without the pvrusb2 driver having a problem. a. Is that correct? Yes, that is correct. b. Is there a limit on the combined payload, such that a the cx25840_read4() would not work as a combined i2c_transfer() ? There is an overall limit on the size of the I2C transfer. This is due to the underlying firmware on pvrusb2-support devices. Essentially the entire outgoing transfer plus a few bytes of overhead has to fit inside a single 64 byte URB. This also limits the atomic read size to roughly 64 bytes as well (the URB size on the returned data). You should be able to reliably write up to 48 bytes at a time, perhaps even a little more. This issue caused a problem for the cx25840 module a few years back when it used to do firmware downloads with large (e.g. 1024 byte or larger) single I2C transfers. Hans told me then it was that large because it allowed the ivtv driver to run more efficiently, but we cut it back to 48 bytes since it triggered problems with I2C adapters (e.g. pvrusb2) which could not handle such larger transfers at all. The pvrusb2 driver's I2C adapter is really just a proxy for the I2C implementation in the device at the far end of the USB cable. So it works at a higher level than one might normally expect - it operates at the transfer level, no bit-banging. The communications protocol required by the hardware limits the I2C transfers to be either a write of some size, or an atomic write followed by a read of various sizes. The pvrusb2 implementation looks at the incoming transfers and tries to map them as best it can over what the device protocol allows. Generally this means that if it is possible it will do the right thing. In the specific case you mentioned above, the result should in fact be exactly what you need. (I'm saying that without having looked at that area of code for quite a while, but it's what I remember being in my head when I did that part..) -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [GIT PULL for 2.6.32] V4L/DVB updates
On Tue, 1 Dec 2009, Mauro Carvalho Chehab wrote: Mike Isely wrote: On Mon, 30 Nov 2009, Mauro Carvalho Chehab wrote: Em Sat, 28 Nov 2009 14:33:30 -0600 (CST) Mike Isely is...@isely.net escreveu: Mauro: I had also posted up two high priority pvrusb2 patches that should really be cherry-picked for 2.6.32. You've already pulled them into v4l/dvb and I did mark them as high priority at the time. These patches enable use of FX2 microcontroller firmware that is 16KB in size. Hauppauge is no longer shipping 8KB firmware for HVR-1950 and HVR-1900 and without these changes then those devices won't work AT ALL in kernel 2.6.32. You can find these within the v4l-dvb Mercurial repository here: Changeset 13495:87c3853fe2b3 Subject: pvrusb2: Support 16KB FX2 firmware http://linuxtv.org/hg/v4l-dvb/rev/87c3853fe2b3 Changeset 13500:d4c418d4b25c Subject: pvrusb2: Fix lingering 16KB FX2 Firmware issues http://linuxtv.org/hg/v4l-dvb/rev/d4c418d4b25c I do not believe these patches have any ordering dependencies with other patches, though between the two the second one technically should come after the first. There are. Picking just those patches broke compilation. Mauro: Please forward to me the compilation errors. Right now I am just not seeing how a patch this trivial could have any compilation dependencies. And unfortunately I will not be able to reproduce your build setup until at least Tuesday night. I must be blind. Also, it seemed too late for adding support for newer boards/firmware when Linus is about to release a kernel. This is not a new feature. It's a bug fix due to something that Hauppauge recently did. Hauppauge is NO LONGER officially distributing FX2 firmware with their hardware which the driver can use. This simply prevents any new HVR-1950 / HVR-1900 users from working under Linux. This fixes breakage for previously working hardware. The root cause is simple - the firmware blob is just larger now - and the fix is trivial. It absolutely needs to go in. In fact, this should go back to a 2.6.31.x and a 2.6.27.x release as well, though in those cases I have to figure out if driver source code is still close enough for the same patches to still work. I am sorry this is showing up late for you. There are multiple reasons for this. However I did mark these patches as high priority, following your v4l-dvb changeset process. I did comment on the pull request that these were important but I guess I needed to also specifically call these out in the pull request text as well. If these don't get in now as part of the official 2.6.32 release, these absolutely need to be queued for 2.6.32.1. We are very late for 2.6.32. I'm not sure if are there still time for it. I'll seek for some time during this week to add those patches at the upstream tree and removing them from the development tree and see what compilation issues arise. Mauro: Thanks. Guess I also really need to get up to speed on git, finally... -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [GIT PULL for 2.6.32] V4L/DVB updates
On Sat, 28 Nov 2009, Hans Verkuil wrote: On Friday 27 November 2009 22:40:01 Stefan Lippers-Hollmann wrote: Hi On Friday 27 November 2009, Mauro Carvalho Chehab wrote: Hi Linus, Please pull from: ssh://master.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-2.6.git for_linus For the following drivers and building fixes: - radio-gemtek-pci: fix double mutex_lock - v4l: add more missing linux/sched.h includes - soc-camera: properly initialise the device object when reusing - soc-camera: sh_mobile_ceu_camera: call pm_runtime_disable - em28xx: fix Reddo DVB-C USB TV Box GPIO - davinci: remove stray duplicate config pointer - SMS_SIANO_MDTV should depend on HAS_DMA - cxusb: Fix hang on DViCO FusionHDTV DVB-T Dual Digital 4 (rev 1) - sh_mobile_ceu_camera: fix compile warning - Fix wrong parameter order in memset [...] Please consider cherry picking the following two patches from Hans Verkuil [1]: - add the missing s2250-loader.h - s2250 mutex patch Good catch. Mauro, can you please add these two as well for 2.6.32? I'm sure I marked these two as high-prio patches. This staging driver is actively being used and developed so this regression should be fixed. Mauro: I had also posted up two high priority pvrusb2 patches that should really be cherry-picked for 2.6.32. You've already pulled them into v4l/dvb and I did mark them as high priority at the time. These patches enable use of FX2 microcontroller firmware that is 16KB in size. Hauppauge is no longer shipping 8KB firmware for HVR-1950 and HVR-1900 and without these changes then those devices won't work AT ALL in kernel 2.6.32. You can find these within the v4l-dvb Mercurial repository here: Changeset 13495:87c3853fe2b3 Subject: pvrusb2: Support 16KB FX2 firmware http://linuxtv.org/hg/v4l-dvb/rev/87c3853fe2b3 Changeset 13500:d4c418d4b25c Subject: pvrusb2: Fix lingering 16KB FX2 Firmware issues http://linuxtv.org/hg/v4l-dvb/rev/d4c418d4b25c I do not believe these patches have any ordering dependencies with other patches, though between the two the second one technically should come after the first. Thanks, -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-20091124
Mauro: Please from http://linuxtv.org/hg/~mcisely/pvrusb2-20091124 for the following pvrusb2 driver fixes / improvements: - pvrusb2: Support 16KB FX2 firmware - pvrusb2: Support manual extraction of 16KB FX2 firmware - pvrusb2: Shorten device hardware description text to work around V4L shortcoming - pvrusb2: Bind I2C address 0x71 for Zilog IR devices - pvrusb2: Cosmetic tweak to minimize size_t exposure - pvrusb2: Fix lingering 16KB FX2 Firmware issues pvrusb2-debugifc.c | 14 +- pvrusb2-devattr.c | 13 - pvrusb2-devattr.h |1 + pvrusb2-hdw.c | 44 +--- pvrusb2-hdw.h |2 +- pvrusb2-i2c-core.c |1 + 6 files changed, 49 insertions(+), 26 deletions(-) The 16KB fixes are pretty important - Hauppauge has updated the FX2 firmware for HVR-1950 and HVR-1900 devices such that it is 16KB in size now. Unfortunately the driver for years had been enforcing the size to be 8KB which made it unable to load the newer firmware. This causes a problem for new users of the driver since the driver CD from Hauppauge contains this newer firmware. With the 16KB fixes this problem is solved. They are marked high priority. Thanks, -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-20091011
On Thu, 29 Oct 2009, Mauro Carvalho Chehab wrote: Em Sun, 11 Oct 2009 22:53:14 -0500 (CDT) Mike Isely is...@isely.net escreveu: Mauro: Please from http://linuxtv.org/hg/~mcisely/pvrusb2-20091011 for a few various pvrusb2 fixes / improvements. No critical bug fixes here, just a bunch of little things: - pvrusb2: Make more info available to udev - pvrusb2: Soften encoder warning message - pvrusb2: Improve diagnostic info on driver initialization failure - pvrusb2: Report hardware description to kernel log upon initialization - pvrusb2: Add hardware description to debuginfo output - pvrusb2: Fix redundant message on driver initialization failure (missing break) - pvrusb2: Cosmetic kernel log tweak Committed. Please fix the existing CodingStyle erros added on this changeset on a next pull request: We've had this discussion many times in the past. I am not going to have that debate yet again. These will NOT be changed. -Mike ERROR: trailing statements should be on next line #26: FILE: linux/drivers/media/video/pvrusb2/pvrusb2-v4l2.c:919: + if (!dip) return; + if (!dip) return; ERROR: trailing statements should be on next line #27: FILE: linux/drivers/media/video/pvrusb2/pvrusb2-v4l2.c:920: + if (!dip-devbase.parent) return; + if (!dip-devbase.parent) return; Cheers, Mauro -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-20091011
Mauro: Please from http://linuxtv.org/hg/~mcisely/pvrusb2-20091011 for a few various pvrusb2 fixes / improvements. No critical bug fixes here, just a bunch of little things: - pvrusb2: Make more info available to udev - pvrusb2: Soften encoder warning message - pvrusb2: Improve diagnostic info on driver initialization failure - pvrusb2: Report hardware description to kernel log upon initialization - pvrusb2: Add hardware description to debuginfo output - pvrusb2: Fix redundant message on driver initialization failure (missing break) - pvrusb2: Cosmetic kernel log tweak pvrusb2-debugifc.c |3 +++ pvrusb2-encoder.c |5 - pvrusb2-hdw-internal.h |1 + pvrusb2-hdw.c | 33 - pvrusb2-v4l2.c | 21 - 5 files changed, 52 insertions(+), 11 deletions(-) -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: How to make my device work with linux?
On Thu, 1 Oct 2009, Wellington Terumi Uemura wrote: I was looking around to find that there is a driver for that Fujitsu MB86A16 inside the Linux Mantis Driver project, Fujitsu MB86A16 DVB-S/DSS DC Receiver driver made by Manu Abraham http://www.verbraak.org/wiki/index.php/Linux_Mantis_driver. I've done a few tests with usbsnoop and other tools but USB sniffer doesn't see any valid command, jut a bunch of bytes that makes no sense: http://www.isely.net/pvrusb2/firmware.html#FX2 What you've pointed at here is a page that describes using a trick with the pvrusb2 driver to suck an image of the FX2 firmware out of the FX2 processor itself. That won't work in your case however since it requires that the pvrusb2 driver already be talking to the chip. The procedure documented at that link is really about firmware extraction not reverse-engineering the data link protocol between the FX2 and the host. I will try my luck compiling that Fujitsu driver, but my best guess is that without a proper I/O from that FX2 it will end up with nothing at all. It's that data link protocol that you need to understand. Please realize that the FX2 is just an 8051 microcontroller which happens to have a fairly interesting USB device interface resident on the same silicon. Beyond that, the chip's behavior is really up to whatever the firmware does. For pvrusb2-driven devices that firmware's behavior is pretty well understood. That driver also benefits from the fact that essentially all USB hosted analog (and some hybrid) capture cards with an mpeg encoder and an FX2 all are derivations from a reference design by a single vendor. That reference design included reference firmware, which each manufacturer of course tweaked a bit. For that reason, all those different devices tend to implement a similar enough data link protocol that the pvrusb2 driver can handle them all with the same implementation. The problem is, we don't know if any of that is true for your device. You are dealing with a digital-only capture device so it can't be based on that same reference design. It is entirely sensible that the FX2 firmware was set up in that case with similar requirements in mind so the result *might* be similar in behavior. But it really isn't known. So when you scan documentation for other drivers (e.g. pvrusb2) you must really look at it all with a rather large helping of scepticism. Mike Krufky mentions a driver for the TDA18271 and he's right. There is one - because the pvrusb2 driver also relies on that when driving an HVR-1950 capture device which happens to use that same part. But that isn't the driver you need. What you need is a bridge driver that can implement the host side of the data link protocol implemented by your device's FX2. That is what the pvrusb2 driver does for the capture devices it handles. With the proper bridge driver set up, then the TDA18271 sub-device driver can ride over that data link to establish communications with its hardware in the device. THEN you'll be on the way to having something working. I know that none of the about is the answer you're looking for. But perhaps it will lead you in the right direction. It is entirely possible that there is another bridge driver out there which can handle this part, but I can't help you there. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: How to make my device work with linux?
On Thu, 1 Oct 2009, Devin Heitmueller wrote: On Thu, Oct 1, 2009 at 6:03 PM, Wellington Terumi Uemura wellingtonuem...@gmail.com wrote: It's not the answer that I was looking for but looks like the thing is much more complex than just compile and run drivers, this gives me another perspective, like a dead end. Thank you Mike. Well, it's certainly possible to get it to work if you're willing to make the investment. It's just one of those situations where you realize quickly that you're going to have to be prepared to do *way* more work than just adding a new board profile. Just because there are drivers for the chips on your device doesn't mean that it is trivial to get working. Cheers, Devin And actually I wasn't intending on totally discouraging you either. But you do need to see the perspective of what you're trying to do otherwise you may just get frustrated. Things aren't hopeless. The cxusb module in DVB might be something you should look at. I guess it depends on how deep you wish to dive here. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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] s2255drv: Don't conditionalize video buffer completion on waiting processes
# HG changeset patch
# User Mike Isely is...@pobox.com
# Date 1253739604 18000
# Node ID 522a74147753ba59c7f45e368439928090a286f2
# Parent e349075171ddf939381fad432c23c1269abc4899
s2255drv: Don't conditionalize video buffer completion on waiting processes
From: Mike Isely is...@pobox.com
The s2255 driver had logic which aborted processing of a video frame
if there was no process waiting on the video buffer in question. That
simply doesn't work when the application is doing things in an
asynchronous manner. If the application went to the trouble to queue
the buffer in the first place, then the driver should always attempt
to complete it - even if the application at that moment has its
attention turned elsewhere. Applications which always blocked waiting
for I/O on the capture device would not have been affected by this.
Applications which *mostly* blocked waiting for I/O on the capture
device probably only would have been somewhat affected (frame lossage,
at a rate which goes up as the application blocks less). Applications
which never blocked on the capture device (e.g. polling only) however
would never have been able to receive any video frames, since in that
case this is anyone waiting on this? check on the buffer never would
have evalutated true. This patch just deletes that harmful check
against the buffer's wait queue.
Priority: high
Signed-off-by: Mike Isely is...@pobox.com
diff -r e349075171dd -r 522a74147753 linux/drivers/media/video/s2255drv.c
--- a/linux/drivers/media/video/s2255drv.c Mon Sep 21 10:42:22 2009 -0500
+++ b/linux/drivers/media/video/s2255drv.c Wed Sep 23 16:00:04 2009 -0500
@@ -599,11 +599,6 @@
buf = list_entry(dma_q-active.next,
struct s2255_buffer, vb.queue);
- if (!waitqueue_active(buf-vb.done)) {
- /* no one active */
- rc = -1;
- goto unlock;
- }
list_del(buf-vb.queue);
do_gettimeofday(buf-vb.ts);
dprintk(100, [%p/%d] wakeup\n, buf, buf-vb.i);
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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] s2255drv: Don't conditionalize video buffer completion on waiting processes
On Wed, 23 Sep 2009, dean wrote:
This seems ok. This portion of code was based on vivi.c, so that might be
checked also.
Yes, after seeing the mention of vivi in this driver I looked at vivi.c
and saw the same construct there. Though I'm willing to bet that it's
just as incorrect there as it was here, I haven't tested or otherwise
used vivi so I wasn't prepared to recommend a patch for it as well.
Probably vivi should be fixed, since it is after all intended as a model
for other v4l driver developers. (And are there any other drivers based
on vivi which have inherited this bug as well?)
-Mike
Mike Isely wrote:
# HG changeset patch
# User Mike Isely is...@pobox.com
# Date 1253739604 18000
# Node ID 522a74147753ba59c7f45e368439928090a286f2
# Parent e349075171ddf939381fad432c23c1269abc4899
s2255drv: Don't conditionalize video buffer completion on waiting processes
From: Mike Isely is...@pobox.com
The s2255 driver had logic which aborted processing of a video frame
if there was no process waiting on the video buffer in question. That
simply doesn't work when the application is doing things in an
asynchronous manner. If the application went to the trouble to queue
the buffer in the first place, then the driver should always attempt
to complete it - even if the application at that moment has its
attention turned elsewhere. Applications which always blocked waiting
for I/O on the capture device would not have been affected by this.
Applications which *mostly* blocked waiting for I/O on the capture
device probably only would have been somewhat affected (frame lossage,
at a rate which goes up as the application blocks less). Applications
which never blocked on the capture device (e.g. polling only) however
would never have been able to receive any video frames, since in that
case this is anyone waiting on this? check on the buffer never would
have evalutated true. This patch just deletes that harmful check
against the buffer's wait queue.
Priority: high
Signed-off-by: Mike Isely is...@pobox.com
diff -r e349075171dd -r 522a74147753 linux/drivers/media/video/s2255drv.c
--- a/linux/drivers/media/video/s2255drv.c Mon Sep 21 10:42:22 2009 -0500
+++ b/linux/drivers/media/video/s2255drv.c Wed Sep 23 16:00:04 2009 -0500
@@ -599,11 +599,6 @@
buf = list_entry(dma_q-active.next,
struct s2255_buffer, vb.queue);
- if (!waitqueue_active(buf-vb.done)) {
- /* no one active */
- rc = -1;
- goto unlock;
- }
list_del(buf-vb.queue);
do_gettimeofday(buf-vb.ts);
dprintk(100, [%p/%d] wakeup\n, buf, buf-vb.i);
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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] bttv: Fix potential out-of-order field processing
Mauro:
You can also directly pull this from:
http://linuxtv.org/hg/~mcisely/bttv-patches/
Sorry about the excessively long commit description, but I felt it
important to fully explain this somewhat subtle problem for what is
otherwise a mature driver. The actual patch is tiny.
-Mike
# HG changeset patch
# User Mike Isely is...@pobox.com
# Date 1253545748 18000
# Node ID 760a8bc4028014493ccbbe85d0c9e8c91873fc23
# Parent 29e4ba1a09bcf9a03a653b2124929f5359fef772
bttv: Fix potential out-of-order field processing
From: Mike Isely is...@pobox.com
There is a subtle interaction in the bttv driver which can result in
fields being repeatedly processed out of order. This is a problem
specifically when running in V4L2_FIELD_ALTERNATE mode (probably the
most common case).
1. The determination of which fields are associated with which buffers
happens in videobuf, before the bttv driver gets a chance to queue the
corresponding DMA. Thus by the point when the DMA is queued for a
given buffer, the algorithm has to do the queuing based on the
buffer's already assigned field type - not based on which field is
next in the video stream.
2. The driver normally tries to queue both the top and bottom fields
at the same time (see bttv_irq_next_video()). It tries to sort out
top vs bottom by looking at the field type for the next 2 available
buffers and assigning them appropriately.
3. However the bttv driver *always* actually processes the top field
first. There's even an interrupt set aside for specifically
recognizing when the top field has been processed so that it can be
marked done even while the bottom field is still being DMAed.
Given all of the above, if one gets into a situation where
bttv_irq_next_video() gets entered when the first available buffer has
been pre-associated as a bottom field, then the function is going to
process the buffers out of order. That first available buffer will be
put into the bottom field slot and the buffer after that will be put
into the top field slot. Problem is, since the top field is always
processed first by the driver, then that second buffer (the one after
the first available buffer) will be the first one to be finished.
Because of the strict fifo handling of all video buffers, then that
top field won't be seen by the app until after the bottom field is
also processed. Worse still, the app will get back the
chronologically later bottom field first, *before* the top field is
received. The buffer's timestamps will even be backwards.
While not fatal to most TV apps, this behavior can subtlely degrade
userspace deinterlacing (probably will cause jitter). That's probably
why it has gone unnoticed. But it will also cause serious problems if
the app in question discards all but the latest received buffer (a
latency minimizing tactic) - causing one field to only ever be
displayed since the other is now always late. Unfortunately once you
get into this state, you're stuck this way - because having consumed
two buffers, now the next time around the first available buffer
will again be a bottom field and the same thing happens.
How can we get into this state? In a perfect world, where there's
always a few free buffers queued to the driver, it should be
impossible. However if something disrupts streaming, e.g. if the
userspace app can't queue free buffers fast enough for a moment due
perhaps to a CPU scheduling glitch, then the driver can get
momentarily starved and some number of fields will be dropped. That's
OK. But if an odd number of fields get dropped, then that first
available buffer might be the bottom field and now we're stuck...
This patch fixes that problem by deliberately only setting up a single
field for one frame if we don't get a top field as the first available
buffer. By purposely skipping the other field, then we only handle a
single buffer thus bringing things back into proper sync (i.e. top
field first) for the next frame. To do this we just drop the few
lines in bttv_irq_next_video() that attempt to set up the second
buffer when that second buffer isn't for the bottom field.
This is definitely a problem in when in V4L2_FIELD_ALTERNATE mode. In
the other modes this change either has no effect or doesn't harm
things any further anyway.
Priority: high
Signed-off-by: Mike Isely is...@pobox.com
diff -r 29e4ba1a09bc -r 760a8bc40280
linux/drivers/media/video/bt8xx/bttv-driver.c
--- a/linux/drivers/media/video/bt8xx/bttv-driver.c Sat Sep 19 09:45:22
2009 -0300
+++ b/linux/drivers/media/video/bt8xx/bttv-driver.c Mon Sep 21 10:09:08
2009 -0500
@@ -3828,11 +3828,34 @@
if (!V4L2_FIELD_HAS_BOTH(item-vb.field)
(item-vb.queue.next != btv-capture)) {
item = list_entry(item-vb.queue.next, struct
bttv_buffer, vb.queue);
+ /* Mike Isely is...@pobox.com - Only check
+* and set up the bottom field in the logic
[PATCH] bttv: Fix reversed polarity error when switching video standard
Mauro:
You can also directly pull this from:
http://linuxtv.org/hg/~mcisely/bttv-patches/
Again, another longer-than usual commit description here. Same reason
as before. This bug is a little less subtle than the other one - and
the patch is even smaller (one character).
-Mike
# HG changeset patch
# User Mike Isely is...@pobox.com
# Date 1253547742 18000
# Node ID e349075171ddf939381fad432c23c1269abc4899
# Parent 760a8bc4028014493ccbbe85d0c9e8c91873fc23
bttv: Fix reversed polarity error when switching video standard
From: Mike Isely is...@pobox.com
The bttv driver function which handles switching of the video standard
(set_tvnorm() in bttv-driver.c) includes a check which can optionally
also reset the cropping configuration to a default value. It is
optional based on a comparison of the cropcap parameters of the
previous vs the newly requested video standard. The comparison is
being done with a memcmp(), a function which only returns a true value
if the comparison actually fails.
This if-statement appears to have been written to assume wrong
memcmp() semantics. That is, it was re-initializing the cropping
configuration only if the new video standard did NOT have different
cropcap values. That doesn't make any sense. One definitely should
reset things if the cropcap parameters are different - if there's any
comparison to made at all.
The effect of this problem was that a transition from, say, PAL to
NTSC would leave in place old cropping setup that made sense for the
PAL geometry but not for NTSC. If the application doesn't care about
cropping it also won't try to reset the cropping configuration,
resulting in an improperly cropped video frame. In the case I was
testing this actually caused black video frames to be displayed.
Another interesting effect of this bug is that if one does something
which does NOT change the video standard and this function is run,
then the cropping setup gets reset anyway - again because of the
backwards comparison. It turns out that just running anything which
merely opens and closes the video device node (e.g. v4l-info) will
cause this to happen. One can argue that simply opening the device
node and not doing anything to it should not mess with any of its
state - but because of this behavior, any TV app which does such
things (e.g. xawtv) probably therefore doesn't see the problem.
The solution is to fix the sense of the if-statement. It's easy to
see how this mistake could have been made given how memcmp() works.
The patch is therefore removal of a single ! character from the
if-statement in set_tvnorm in bttv-driver.c.
Priority: high
Signed-off-by: Mike Isely is...@pobox.com
diff -r 760a8bc40280 -r e349075171dd
linux/drivers/media/video/bt8xx/bttv-driver.c
--- a/linux/drivers/media/video/bt8xx/bttv-driver.c Mon Sep 21 10:09:08
2009 -0500
+++ b/linux/drivers/media/video/bt8xx/bttv-driver.c Mon Sep 21 10:42:22
2009 -0500
@@ -1322,7 +1322,7 @@
tvnorm = bttv_tvnorms[norm];
- if (!memcmp(bttv_tvnorms[btv-tvnorm].cropcap, tvnorm-cropcap,
+ if (memcmp(bttv_tvnorms[btv-tvnorm].cropcap, tvnorm-cropcap,
sizeof (tvnorm-cropcap))) {
bttv_crop_reset(btv-crop[0], norm);
btv-crop[1] = btv-crop[0]; /* current = default */
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Acked-By: Mike Isely is...@pobox.com -Mike On Fri, 7 Aug 2009, Hans Verkuil wrote: Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the following: - pvrusb2: fix compile warning - cx24113: fix mips compiler warning - hdpvr: add missing initialization of current_norm - v4l2-ioctl: fix G_STD and G_PARM default handlers - v4l2-ctl: fix get/set-parm bugs and add get/set-output-parm support Thanks, Hans diffstat: linux/drivers/media/dvb/frontends/cx24113.c |6 + linux/drivers/media/video/hdpvr/hdpvr-video.c |2 linux/drivers/media/video/pvrusb2/pvrusb2-audio.c |5 - linux/drivers/media/video/v4l2-ioctl.c| 15 ++- v4l2-apps/util/v4l2-ctl.cpp | 98 +- 5 files changed, 101 insertions(+), 25 deletions(-) -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: offering bounty for GPL'd dual em28xx support
On Wed, 22 Jul 2009, Mauro Carvalho Chehab wrote: Em Wed, 22 Jul 2009 11:06:12 -0400 Devin Heitmueller dheitmuel...@kernellabs.com escreveu: On Wed, Jul 22, 2009 at 11:01 AM, Jelle de Jongjelledej...@powercraft.nl wrote: Funky timing of those mails :D. I saw only after sending my mail that Steve was talking about analog and that this is indeed different. Dual analog tuner support should be possible right? Maybe with some other analog usb chipsets? I don't know what the usb blocksize is or if they are isochronous transfers or bulk or control. I assume the video must be uncompressed transferred over usb because the decoding chip is on the usb device is not capable of doing compression encoding after the analog video decoding? Are there usb devices that do such tricks? There were older devices that did compression, mainly designed to fit the stream inside of 12Mbps USB. However, they required onboard RAM to buffer the frame which added considerable cost (in addition to the overhead of doing the compression), and as a result pretty much all of the USB 2.0 designs I have seen do not do any on-chip compression. The example which comes to mind is the Hauppauge Win-TV USB which uses the usbvision chipset. pvrusb2 also has compression, provided by an external mpeg encoder. Those devices are USB 2.0 I know this is a fairly old thread, but I've been away on vacation and I'm catching up on e-mail right now. So forgive me if this has already been answered... The Hauppauge Win-TV PVR-USB2 is the most well-known device in this category and it's what the pvrusb2 driver originally targeted. This device uses a dedicated mpeg encoder chip within the device, so the video stream coming from the hardware is actually compressed video (mpeg format, mostly DVD-style mpeg2). The question of USB 1.1 vs USB 2.0 is actually due to the device's microcontroller (the bridge chip) not the mpeg encoder. In the pvrusb2 case, that controller is a Cypress FX2 which includes an on-chip USB 2.0 high-speed device interface. But the mpeg encoder actually doesn't REQUIRE USB 2.0 high-speed. The default encoder settings configured by the pvrusb2 driver actually work quite well over USB 1.1, since the resulting video stream requires significantly less bandwidth than the 12Mbps that USB 1.1 can theoretically supply. I've actually successfully tested such a configuration here. The hardware works fine over USB 1.1. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-dev
[sorry, reposted with corrected subject tag] Mauro: Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2-dev for a number of pvrusb2 driver fixes. The first two in particular are high-priority critical fixes that clean up a nasty regression involving analog capture on the HVR-1950 and the older 24xxx model series. It would be good to see those at least backported to a 2.6.30.x release. - pvrusb2: Fix hardware scaling when used with cx25840 - pvrusb2: Re-fix hardware scaling on video standard change - pvrusb2: Change initial default frequency setting - pvrusb2: Improve handling of routing schemes - pvrusb2: De-obfuscate code which handles routing schemes pvrusb2-audio.c | 14 ++- pvrusb2-cs53l32a.c| 26 +++-- pvrusb2-cx2584x-v4l.c | 39 +--- pvrusb2-hdw.c | 60 -- pvrusb2-video-v4l.c | 37 +- 5 files changed, 98 insertions(+), 78 deletions(-) -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: s5h1411_readreg: readreg error (ret == -5)
I am unable to reproduce the s5h1411 error here. However my HVR-1950 loads the s5h1409 module - NOT s5h1411.ko; I wonder if Hauppauge has changed chips on newer devices and so you're running a different tuner module. That would explain the different behavior. Unfortunately it also means it will be very difficult for me to track the problem down here since I don't have that device variant. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: s5h1411_readreg: readreg error (ret == -5)
Well now I feel like an idiot. Thanks for pointing that out in my own
code :-)
Still digging through this.
-Mike
On Fri, 12 Jun 2009, Andy Walls wrote:
On Fri, 2009-06-12 at 15:33 -0500, Mike Isely wrote:
I am unable to reproduce the s5h1411 error here.
However my HVR-1950 loads the s5h1409 module - NOT s5h1411.ko; I wonder
if Hauppauge has changed chips on newer devices and so you're running a
different tuner module.
The digital demodulator driver to use is hardcoded in pvrusb2-devattr.c:
static const struct pvr2_dvb_props pvr2_750xx_dvb_props = {
.frontend_attach = pvr2_s5h1409_attach,
.tuner_attach= pvr2_tda18271_8295_attach,
};
static const struct pvr2_dvb_props pvr2_751xx_dvb_props = {
.frontend_attach = pvr2_s5h1411_attach,
.tuner_attach= pvr2_tda18271_8295_attach,
};
...
static const struct pvr2_device_desc pvr2_device_750xx = {
.description = WinTV HVR-1950 Model Category 750xx,
...
.dvb_props = pvr2_750xx_dvb_props,
#endif
};
...
static const struct pvr2_device_desc pvr2_device_751xx = {
.description = WinTV HVR-1950 Model Category 751xx,
...
.dvb_props = pvr2_751xx_dvb_props,
#endif
};
That would explain the different behavior.
Unfortunately it also means it will be very difficult for me to track
the problem down here since I don't have that device variant.
If you have more than 1 HVR-1950, maybe one is a 751xx variant.
When I ran into I2C errors often, it was because of PCI bus errors
screwing up the bit banging. Obviously, that's not the case here.
-Andy
-Mike
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: s5h1411_readreg: readreg error (ret == -5)
On Thu, 11 Jun 2009, Steven Toth wrote: Mike Isely wrote: On Sun, 7 Jun 2009, Roger wrote: From looking at linux/drivers/media/dvb/frontends/s5h1411.c, The s5h1411_readreg wants to see 2 but is getting -5 from the i2c bus. --- Snip --- s5h1411_readreg: readreg error (ret == -5) pvrusb2: unregistering DVB devices device: 'dvb0.net0': device_unregister --- Snip --- What exactly does this mean? Roger: It means that the module attempted an I2C transfer and the transfer failed. The I2C adapter within the pvrusb2 driver will return either the number of bytes that it transferred or a failure code. The failure code, as is normal convention in the kernel, will be a negated errno value. Thus the expected value of 2 would be the fact that it probably tried a 2 byte transfer, while the actual value returned of -5 indicate an EIO error, which is what the pvrusb2 driver will return when the underlying I2C transaction has failed. Of course the real question is not that it failed but why it failed. And for that I unfortunately do not have an answer. It's possible that the s5h1411 driver did something that the chip didn't like and the chip responded by going deaf on the I2C bus. More than a few I2C-driven parts can behave this way. It's also possible that the part might have been busy and unable to respond - but usually in that case the driver for such a part will be written with this in mind and will know how / when to communicate with the hardware. Roger: Another possibility, although I don't know the PVRUSB2 driver too well, the s5h1411 is being held in reset when the driver unloads _AFTER_ the last active use was analog video (assuming the s5h1411 is floated in reset as the FX2 input port might be shared with the analog encoder) Good point. The pvrusb2 driver is not currently doing anything specific - or at least deliberate - via the FX2 to move that part in/out of reset. (Of course, I am issuing FX2 commands to shift modes and that might in turn be triggering other things.) But even if I did do something specific, what kind of impact is that likely to do on the corresponding, blissfully ignorant, driver? This actually drives towards a larger issue - the pvrusb2 driver works with various V4L-only sub-devices, e.g. cx25840, which have no relevance in digital mode but I can't really control when that corresponding driver is enabled / disabled. So if I have to take an extra step to physically disable a chip when in digital mode, then this might impact the sub-driver which otherwise is going to have no clue what is really going on. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: s5h1411_readreg: readreg error (ret == -5)
On Sun, 7 Jun 2009, Roger wrote: From looking at linux/drivers/media/dvb/frontends/s5h1411.c, The s5h1411_readreg wants to see 2 but is getting -5 from the i2c bus. --- Snip --- s5h1411_readreg: readreg error (ret == -5) pvrusb2: unregistering DVB devices device: 'dvb0.net0': device_unregister --- Snip --- What exactly does this mean? Roger: It means that the module attempted an I2C transfer and the transfer failed. The I2C adapter within the pvrusb2 driver will return either the number of bytes that it transferred or a failure code. The failure code, as is normal convention in the kernel, will be a negated errno value. Thus the expected value of 2 would be the fact that it probably tried a 2 byte transfer, while the actual value returned of -5 indicate an EIO error, which is what the pvrusb2 driver will return when the underlying I2C transaction has failed. Of course the real question is not that it failed but why it failed. And for that I unfortunately do not have an answer. It's possible that the s5h1411 driver did something that the chip didn't like and the chip responded by going deaf on the I2C bus. More than a few I2C-driven parts can behave this way. It's also possible that the part might have been busy and unable to respond - but usually in that case the driver for such a part will be written with this in mind and will know how / when to communicate with the hardware. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [PULL] http://kernellabs.com/hg/~stoth/tda10048-merge/
I see no issues here with the pvrusb2 part of it... Acked-by: Mike Isely is...@pobox.com On Wed, 20 May 2009, Steven Toth wrote: Mauro, Please pull from http://kernellabs.com/hg/~stoth/tda10048-merge/ - TDA10048: Ensure the I/F changes during DVB-T 6/7/8 bandwidth changes. - cx23885: Ensure we specify I/F's for all bandwidths - pvrusb2: Ensure we specify I/F's for all bandwidths - TDA10048: Missing two I/F's / Pll combinations from the PLL table - cx23885: fix tda10048 IF frequencies for the Hauppauge WinTV-HVR1210 dvb/frontends/tda10048.c| 188 +--- dvb/frontends/tda10048.h|6 video/cx23885/cx23885-dvb.c |8 video/pvrusb2/pvrusb2-devattr.c |4 4 files changed, 139 insertions(+), 67 deletions(-) This was regression tested on the following products. HVR-1900, HVR-1200 and HVR-1700. Thanks -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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 0/8] ir-kbd-i2c conversion to the new i2c binding model (v3)
On Thu, 14 May 2009, Jean Delvare wrote: On Thu, 14 May 2009 21:25:02 +0200, Oldřich Jedlička wrote: On Wednesday 13 of May 2009 at 21:45:59, Jean Delvare wrote: Hi all, Here comes an update of my conversion of ir-kbd-i2c to the new i2c binding model. I've split it into 8 pieces for easier review. Firstly there is 1 preliminary patch: Hi Jean, works for me, as usual :-) I've used the all-in-one patch and the up-to-date v4l-dvb tree (compiled yesterday for completeness). Oldrich, thanks a lot for testing and reporting, this is very appreciated. Jean: I tried the all-in-one patch here on a PVR-USB2 24xxx model (slightly older v4l-dvb repo and 2.6.27.13 vanilla kernel) and it worked fine. I'll add an acked-by to the corresponding (trivial) pvrusb2 patch that you've posted. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
Re: [PATCH 8/8] pvrusb2: Instantiate ir_video I2C device by default
On Wed, 13 May 2009, Jean Delvare wrote:
Now that the ir-kbd-i2c driver has been converted to a new-style i2c
driver, we can instantiate the ir_video I2C device by default. The
pvr2_disable_ir_video is kept to disable the IR receiver, either
because the user doesn't use it, or for debugging purpose.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Mike Isely is...@pobox.com
Acked-by: Mike Isely is...@pobox.com
---
linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- v4l-dvb.orig/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
2009-05-13 18:05:54.0 +0200
+++ v4l-dvb/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
2009-05-13 18:06:32.0 +0200
@@ -43,7 +43,7 @@ static int ir_mode[PVR_NUM] = { [0 ... P
module_param_array(ir_mode, int, NULL, 0444);
MODULE_PARM_DESC(ir_mode,specify: 0=disable IR reception, 1=normal IR);
-static int pvr2_disable_ir_video = 1;
+static int pvr2_disable_ir_video;
module_param_named(disable_autoload_ir_video, pvr2_disable_ir_video,
int, S_IRUGO|S_IWUSR);
MODULE_PARM_DESC(disable_autoload_ir_video,
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-dev
On Mon, 11 May 2009, Mauro Carvalho Chehab wrote: Em Mon, 11 May 2009 22:09:26 -0300 Mauro Carvalho Chehab mche...@infradead.org escreveu: Em Sat, 9 May 2009 16:49:31 -0500 (CDT) Mike Isely is...@isely.net escreveu: Mauro: Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2-dev for various pvrusb2 changesets. Several have to do with IR as previously discussed with Jean Delvare. He's waiting for these changes. Other stuff is more of a miscellaneous / cleanup nature. Hmm... this one failed when importing on -git: Changeset: 11749 From: Greg Kroah-Hartman gre...@suse.de Commiter: Mike Isely is...@pobox.com Date: Fri May 01 22:36:33 2009 -0500 Subject: pvrusb2: remove driver_data direct access of struct device In the near future, the driver core is going to not allow direct access to the driver_data pointer in struct device. Instead, the functions dev_get_drvdata() and dev_set_drvdata() should be used. These functions have been around since the beginning, so are backwards compatible with all older kernel versions. Priority: normal Cc: Mauro Carvalho Chehab mche...@infradead.org Cc: linux-media@vger.kernel.org $ patch -p1 -i 11749.patch patching file drivers/media/video/pvrusb2/pvrusb2-sysfs.c Reversed (or previously applied) patch detected! Assume -R? [n] It seems that you've got a Greg's patch, removed the parts that touch on other files, applied on your tree and asked me to merge it. Please, never do it, since this will cause merge problems when exporting the patches to git. Next time, just reply with an acked-by, and let Patchwork to add your ack on the original patch. I in fact did what you are asking for here (i.e. wait for it to show up on its own) before for another change that got rid of usb_settoggle(). It took a LONG time - WEEKS - for that fix to get back into v4l-dvb via the mechanism you just described. And this had the effect of breaking the v4l-dvb repository for a period of time when the kernel mainline then unpublished the usb_settoggle() function. I do NOT like to see that happen. That caused me to decide not to rely on what you are now telling me to do. I also disagree with this for another reason. What happens if, say, Greg generates a patch that I need before I can proceed with other changes? Do I just sit around and wait for it to trickle back before I can continue? That seems wrong. In addition in the past when there have been pvrusb2 changes generated from upstream you have asked if I was planning on pulling them in myself - which I've done in the past. It seems wrong on its face to tell me that I can't go get a patch that affects my driver. AND... In the case of the remove usb_settoggle() patch, I *DID* in fact add my acked-by to that patch. Greg dutifully took note of this and ensured it was part of the git patch. However when it got back into v4l-dvb, the acked-by attribution was missing. I complained about this to you and your response was that this was a fault of the pathway / mechanism and that I should basically accept this. So now you're telling me to do this anyway? Whatever. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-dev
Mauro: Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2-dev for various pvrusb2 changesets. Several have to do with IR as previously discussed with Jean Delvare. He's waiting for these changes. Other stuff is more of a miscellaneous / cleanup nature. -Mike - pvrusb2: Select, track, and report IR scheme in use with the device - pvrusb2: Update to work with upcoming ir_video changes in v4l-dvb core - pvrusb2: Set ir_video autoloading to default disabled - pvrusb2: Bump up version advertised through v4l interface - pvrusb2: Don't use the internal i2c client list - pvrusb2: remove driver_data direct access of struct device - pvrusb2: Allocate a routing ID for future support of Terratec Grabster AV400 pvrusb2-devattr.c |1 pvrusb2-devattr.h | 23 +- pvrusb2-hdw-internal.h |3 + pvrusb2-hdw.c | 78 - pvrusb2-i2c-core.c | 53 +++-- pvrusb2-sysfs.c| 22 ++--- pvrusb2-v4l2.c |2 - 7 files changed, 106 insertions(+), 76 deletions(-) -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [PULL] http://linuxtv.org/hg/~stoth/tda10048
Nice catch. Thanks. -Mike On Tue, 5 May 2009, Steven Toth wrote: In addition to this: Please pull from http://www.linuxtv.org/hg/~stoth/tda10048 - tda10048: Added option to block i2c gate control from other drivers. - pvrusb2: Ensure the PVRUSB2 disabled the i2c gate on the tda10048. dvb/frontends/tda10048.c| 222 +++- dvb/frontends/tda10048.h| 17 + video/cx23885/cx23885-dvb.c |4 video/pvrusb2/pvrusb2-devattr.c |3 4 files changed, 244 insertions(+), 2 deletions(-) This fixes a bug in the current PVRUSB2 tree where DVB-T is not working due to multiple repeaters upsteam of the tuner on the same i2c bus. - Steve Steven Toth wrote: Mauro, Please pull from http://www.linuxtv.org/hg/~stoth/tda10048 - tda10048: Add ability to select I/F at attach time. - cx23885: For tda10048 boards ensure we specify the I/F - pvrusb2: Ensure we specify the I/F at attach time. dvb/frontends/tda10048.c| 219 +++- dvb/frontends/tda10048.h| 14 + video/cx23885/cx23885-dvb.c |4 video/pvrusb2/pvrusb2-devattr.c |2 4 files changed, 237 insertions(+), 2 deletions(-) The TDA10048 used to have a hard-coded I/F, I've improved this to support different I/F's and ensured that all current bridge drivers specify their needs. Regards, - Steve -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [cron job] v4l-dvb daily build 2.6.22 and up: ERRORS, 2.6.16-2.6.21: ERRORS
On Fri, 1 May 2009, Alexey Klimov wrote: Hello, On Mon, Apr 20, 2009 at 3:59 AM, Mike Isely is...@isely.net wrote: [...] So the kernel already has this; it just needs to be pulled back into v4l-dvb. It's an obvious trivial thing for now and I've acked it there. Obviously we're getting had here because you're compiling against a kernel snapshot that's been changed but v4l-dvb doesn't have the corresponding change in its local copy of the pvrusb2 driver. Part of the fun of synchronizing changes from different trees :-( Well, good to know that this thing is already fixed. I'm very sorry for the mess. No apology needed. Really - this mess wasn't caused by you. If anything I should have just immediately pulled that patch into hg and not waited for it to trickle back to Mauro. That would have avoided the error. So, all I can say is that I'm sorry you had to hit this! -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
Re: [PATCH 2/6] ir-kbd-i2c: Switch to the new-style device binding model
Jean: I have another idea that I think you'll like. I'm putting the finishing touches on the patch right now. What I have implements correct ir_video loading for the pvrusb2 driver. It also includes a lookup table (though with only 1 entry right now) to determine the proper I2C address and I use i2c_new_device() now instead of i2c_new_probed_device(). I've also renamed things to be ir_video everywhere instead of ir_kbd_i2c as was discussed. In particular the disable option is there like before, but now it's called disable_autoload_ir_video. So far this is exactly what I was saying before. But I'm also making one more change: the disable_autoload_ir_video option default value will - for now - be 1, i.e. everything above I just described starts off disabled. This means that the entire patch can be pulled _right_ _now_ without breaking anything at all, because the outward behavior is still unchanged. Then, when you're ready to bring your stuff in, all you need to do is include a 1-line change to pvrusb2-i2c-core.c to switch the default value of disable_autoload_ir_video back to 0, which now enables all the corresponding pvrusb2 changes that you need to avoid any breakage inside my driver. Just to be absolutely crystal clear, here's the change you will be able to do once these changes are in: diff -r 8d2e1361520c linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c --- a/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c Fri May 01 20:23:39 2009 -0500 +++ b/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c Fri May 01 20:24:23 2009 -0500 @@ -43,7 +43,7 @@ module_param_array(ir_mode, int, NULL, 0444); MODULE_PARM_DESC(ir_mode,specify: 0=disable IR reception, 1=normal IR); +static int pvr2_disable_ir_video; -static int pvr2_disable_ir_video = 1; module_param_named(disable_autoload_ir_video, pvr2_disable_ir_video, int, S_IRUGO|S_IWUSR); MODULE_PARM_DESC(disable_autoload_ir_video, So with all this, what I am setting up right now will cause no harm to the existing situation, requires no actual messing with module options, and once you're reading, just include the 1-line change above and you're set. There's no race here, no gap in IR handling. -Mike On Thu, 23 Apr 2009, Mike Isely wrote: Hi Jean, I had actually written out a longer, detailed, point-by-point reply earlier today, but before I could finish it I got interrupted with a crisis. And then another. And that's kind of how my day went. Now I'm finally back to this, but I have another e-mail debacle to immediately deal with (thank you pobox.com, not!) so I'm tossing that unfinished lengthy reply. I think I can sum this whole thing up very simply. Here's what I think should be happening: 1a. Your existing IR changeset, minus the pvrusb2 changes, should be merged. 1b. In parallel with (1a) I modify my pvrusb2 changeset to use the right module name and I adjust the driver option name to match. 2. My pvrusb2 changeset, with changes from (1b) is merged. 3. Andy's proposed change for ir_kbd_i2c to support additional IR devices is investigated and probably merged. 4. I test the changed ir_kbd_i2c against additional pvrusb2 devices known not to work previously with ir_kbd_i2c. If they work, then I will create a pvrusb2 patch to load ir_video in those cases as well as the cases already set up (which still won't cover all possible pvrusb2-driven devices). I think this satisfies the remaining issues, except that from between steps 1 and 2 ir_kbd_i2c won't work with the pvrusb2 driver. But you know, I'm OK with that. I expect (2) to happen within a few days after (1) so the impact should be minimal. It certainly won't escape into the kernel tree that way. In addition, my impression from the community of pvrusb2 users is that the preferred IR strategy is lirc anyway, and it's a foregone conclusion that they are all going to be, uh, impacted once your compatibility parts are gone from i2c. From where I'm sitting the gap from (1) to (2) is trivial compared to that impending mess - realize I'm not complaining since after all (a) it really falls to the lirc developers to fix that, (b) you do need to get your changes in, and (c) there's little I can do for lirc there except to keep it working as long as possible with the pvrusb2 driver. I'm just pointing out that I'm OK with the step 1 - 2 gap for the pvrusb2 driver because it's small and will be nothing compared to what's about to happen with lirc. If you still don't like any of that, well, then I give up. In that case, then merge your changes with the pvrusb2 changes included. I won't ack them, but I'll just deal with it later. Because while your changes might keep ir_kbd_i2c going, they will also immediately break the more-useful and apparently more-used lirc (by always binding ir_kbd_i2c even in cases in the pvrusb2 driver where it's known
Re: [PATCH] media: remove driver_data direct access of struct device
Acked-By: Mike Isely is...@pobox.com
Note #1: I am just acking the pvrusb2 part of this.
Note #2: I am immediately pulling the pvrusb2 part of these changes into
that driver.
-Mike
On Thu, 30 Apr 2009, Greg Kroah-Hartman wrote:
From: Greg Kroah-Hartman gre...@suse.de
In the near future, the driver core is going to not allow direct access
to the driver_data pointer in struct device. Instead, the functions
dev_get_drvdata() and dev_set_drvdata() should be used. These functions
have been around since the beginning, so are backwards compatible with
all older kernel versions.
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: linux-media@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman gre...@suse.de
---
drivers/media/dvb/firewire/firedtv-1394.c |4 ++--
drivers/media/dvb/firewire/firedtv-dvb.c|2 +-
drivers/media/video/pvrusb2/pvrusb2-sysfs.c | 22 +++---
3 files changed, 14 insertions(+), 14 deletions(-)
--- a/drivers/media/dvb/firewire/firedtv-1394.c
+++ b/drivers/media/dvb/firewire/firedtv-1394.c
@@ -225,7 +225,7 @@ fail_free:
static int node_remove(struct device *dev)
{
- struct firedtv *fdtv = dev-driver_data;
+ struct firedtv *fdtv = dev_get_drvdata(dev);
fdtv_dvb_unregister(fdtv);
@@ -242,7 +242,7 @@ static int node_remove(struct device *de
static int node_update(struct unit_directory *ud)
{
- struct firedtv *fdtv = ud-device.driver_data;
+ struct firedtv *fdtv = dev_get_drvdata(ud-device);
if (fdtv-isochannel = 0)
cmp_establish_pp_connection(fdtv, fdtv-subunit,
--- a/drivers/media/dvb/firewire/firedtv-dvb.c
+++ b/drivers/media/dvb/firewire/firedtv-dvb.c
@@ -268,7 +268,7 @@ struct firedtv *fdtv_alloc(struct device
if (!fdtv)
return NULL;
- dev-driver_data= fdtv;
+ dev_set_drvdata(dev, fdtv);
fdtv-device= dev;
fdtv-isochannel= -1;
fdtv-voltage = 0xff;
--- a/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
@@ -539,7 +539,7 @@ static void class_dev_destroy(struct pvr
sfp-attr_unit_number);
}
pvr2_sysfs_trace(Destroying class_dev id=%p,sfp-class_dev);
- sfp-class_dev-driver_data = NULL;
+ dev_set_drvdata(sfp-class_dev, NULL);
device_unregister(sfp-class_dev);
sfp-class_dev = NULL;
}
@@ -549,7 +549,7 @@ static ssize_t v4l_minor_number_show(str
struct device_attribute *attr, char *buf)
{
struct pvr2_sysfs *sfp;
- sfp = (struct pvr2_sysfs *)class_dev-driver_data;
+ sfp = dev_get_drvdata(class_dev);
if (!sfp) return -EINVAL;
return scnprintf(buf,PAGE_SIZE,%d\n,
pvr2_hdw_v4l_get_minor_number(sfp-channel.hdw,
@@ -561,7 +561,7 @@ static ssize_t bus_info_show(struct devi
struct device_attribute *attr, char *buf)
{
struct pvr2_sysfs *sfp;
- sfp = (struct pvr2_sysfs *)class_dev-driver_data;
+ sfp = dev_get_drvdata(class_dev);
if (!sfp) return -EINVAL;
return scnprintf(buf,PAGE_SIZE,%s\n,
pvr2_hdw_get_bus_info(sfp-channel.hdw));
@@ -572,7 +572,7 @@ static ssize_t hdw_name_show(struct devi
struct device_attribute *attr, char *buf)
{
struct pvr2_sysfs *sfp;
- sfp = (struct pvr2_sysfs *)class_dev-driver_data;
+ sfp = dev_get_drvdata(class_dev);
if (!sfp) return -EINVAL;
return scnprintf(buf,PAGE_SIZE,%s\n,
pvr2_hdw_get_type(sfp-channel.hdw));
@@ -583,7 +583,7 @@ static ssize_t hdw_desc_show(struct devi
struct device_attribute *attr, char *buf)
{
struct pvr2_sysfs *sfp;
- sfp = (struct pvr2_sysfs *)class_dev-driver_data;
+ sfp = dev_get_drvdata(class_dev);
if (!sfp) return -EINVAL;
return scnprintf(buf,PAGE_SIZE,%s\n,
pvr2_hdw_get_desc(sfp-channel.hdw));
@@ -595,7 +595,7 @@ static ssize_t v4l_radio_minor_number_sh
char *buf)
{
struct pvr2_sysfs *sfp;
- sfp = (struct pvr2_sysfs *)class_dev-driver_data;
+ sfp = dev_get_drvdata(class_dev);
if (!sfp) return -EINVAL;
return scnprintf(buf,PAGE_SIZE,%d\n,
pvr2_hdw_v4l_get_minor_number(sfp-channel.hdw,
@@ -607,7 +607,7 @@ static ssize_t unit_number_show(struct d
struct device_attribute *attr, char *buf)
{
struct pvr2_sysfs *sfp;
- sfp = (struct pvr2_sysfs *)class_dev-driver_data;
+ sfp = dev_get_drvdata(class_dev);
if (!sfp) return -EINVAL;
return scnprintf(buf,PAGE_SIZE,%d\n,
pvr2_hdw_get_unit_number(sfp-channel.hdw));
@@ -635,7 +635,7 @@ static void class_dev_create(struct pvr2
Re: [PATCH] pvrusb2: Don't use the internal i2c client list
On Thu, 30 Apr 2009, Jean Delvare wrote:
The i2c core used to maintain a list of client for each adapter. This
is a duplication of what the driver core already does, so this list
will be removed as part of a future cleanup. Anyone using this list
must stop doing so.
For pvrusb2, I propose the following change, which should lead to an
equally informative output. The only difference is that i2c clients
which are not a v4l2 subdev won't show up, but I guess this case is
not supposed to happen anyway.
It will happen for anything i2c used by v4l which itself is not really a
part of v4l. That would include, uh, lirc.
I will review and test this first chance I get which should be tomorrow.
-Mike
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Mike Isely is...@pobox.com
---
Mike, can you please review and test this patch? Thanks.
linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c | 56
+--
1 file changed, 13 insertions(+), 43 deletions(-)
--- v4l-dvb.orig/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c
2009-04-30 16:52:32.0 +0200
+++ v4l-dvb/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c 2009-04-30
17:20:37.0 +0200
@@ -4920,65 +4920,35 @@ static unsigned int pvr2_hdw_report_clie
unsigned int tcnt = 0;
unsigned int ccnt;
struct i2c_client *client;
- struct list_head *item;
- void *cd;
const char *p;
unsigned int id;
- ccnt = scnprintf(buf, acnt, Associated v4l2-subdev drivers:);
+ ccnt = scnprintf(buf, acnt, Associated v4l2-subdev drivers and I2C
clients:\n);
tcnt += ccnt;
v4l2_device_for_each_subdev(sd, hdw-v4l2_dev) {
id = sd-grp_id;
p = NULL;
if (id ARRAY_SIZE(module_names)) p = module_names[id];
if (p) {
- ccnt = scnprintf(buf + tcnt, acnt - tcnt, %s, p);
+ ccnt = scnprintf(buf + tcnt, acnt - tcnt, %s:, p);
tcnt += ccnt;
} else {
ccnt = scnprintf(buf + tcnt, acnt - tcnt,
- (unknown id=%u), id);
+(unknown id=%u):, id);
tcnt += ccnt;
}
- }
- ccnt = scnprintf(buf + tcnt, acnt - tcnt, \n);
- tcnt += ccnt;
-
- ccnt = scnprintf(buf + tcnt, acnt - tcnt, I2C clients:\n);
- tcnt += ccnt;
-
- mutex_lock(hdw-i2c_adap.clist_lock);
- list_for_each(item, hdw-i2c_adap.clients) {
- client = list_entry(item, struct i2c_client, list);
- ccnt = scnprintf(buf + tcnt, acnt - tcnt,
-%s: i2c=%02x, client-name, client-addr);
- tcnt += ccnt;
- cd = i2c_get_clientdata(client);
- v4l2_device_for_each_subdev(sd, hdw-v4l2_dev) {
- if (cd == sd) {
- id = sd-grp_id;
- p = NULL;
- if (id ARRAY_SIZE(module_names)) {
- p = module_names[id];
- }
- if (p) {
- ccnt = scnprintf(buf + tcnt,
- acnt - tcnt,
- subdev=%s, p);
- tcnt += ccnt;
- } else {
- ccnt = scnprintf(buf + tcnt,
- acnt - tcnt,
- subdev= id %u),
- id);
- tcnt += ccnt;
- }
- break;
- }
+ client = v4l2_get_subdevdata(sd);
+ if (client) {
+ ccnt = scnprintf(buf + tcnt, acnt - tcnt,
+ %s @ %02x\n, client-name,
+ client-addr);
+ tcnt += ccnt;
+ } else {
+ ccnt = scnprintf(buf + tcnt, acnt - tcnt,
+ no i2c client\n);
+ tcnt += ccnt;
}
- ccnt = scnprintf(buf + tcnt, acnt - tcnt, \n);
- tcnt += ccnt;
}
- mutex_unlock(hdw-i2c_adap.clist_lock);
return tcnt;
}
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
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 2/6] ir-kbd-i2c: Switch to the new-style device binding model
Hi Jean, I had actually written out a longer, detailed, point-by-point reply earlier today, but before I could finish it I got interrupted with a crisis. And then another. And that's kind of how my day went. Now I'm finally back to this, but I have another e-mail debacle to immediately deal with (thank you pobox.com, not!) so I'm tossing that unfinished lengthy reply. I think I can sum this whole thing up very simply. Here's what I think should be happening: 1a. Your existing IR changeset, minus the pvrusb2 changes, should be merged. 1b. In parallel with (1a) I modify my pvrusb2 changeset to use the right module name and I adjust the driver option name to match. 2. My pvrusb2 changeset, with changes from (1b) is merged. 3. Andy's proposed change for ir_kbd_i2c to support additional IR devices is investigated and probably merged. 4. I test the changed ir_kbd_i2c against additional pvrusb2 devices known not to work previously with ir_kbd_i2c. If they work, then I will create a pvrusb2 patch to load ir_video in those cases as well as the cases already set up (which still won't cover all possible pvrusb2-driven devices). I think this satisfies the remaining issues, except that from between steps 1 and 2 ir_kbd_i2c won't work with the pvrusb2 driver. But you know, I'm OK with that. I expect (2) to happen within a few days after (1) so the impact should be minimal. It certainly won't escape into the kernel tree that way. In addition, my impression from the community of pvrusb2 users is that the preferred IR strategy is lirc anyway, and it's a foregone conclusion that they are all going to be, uh, impacted once your compatibility parts are gone from i2c. From where I'm sitting the gap from (1) to (2) is trivial compared to that impending mess - realize I'm not complaining since after all (a) it really falls to the lirc developers to fix that, (b) you do need to get your changes in, and (c) there's little I can do for lirc there except to keep it working as long as possible with the pvrusb2 driver. I'm just pointing out that I'm OK with the step 1 - 2 gap for the pvrusb2 driver because it's small and will be nothing compared to what's about to happen with lirc. If you still don't like any of that, well, then I give up. In that case, then merge your changes with the pvrusb2 changes included. I won't ack them, but I'll just deal with it later. Because while your changes might keep ir_kbd_i2c going, they will also immediately break the more-useful and apparently more-used lirc (by always binding ir_kbd_i2c even in cases in the pvrusb2 driver where it's known that it can't even work but lirc would have) which as far as I'm concerned is far worse than the step 1 - 2 gap above. But it's just another gap and I'll push another pvrusb2 changeset on top to clean it up. So just do whatever you think is best right now, if you disagree with the sequence above. Now I will go off and deal with the steamer that pobox.com has just handed me :-( -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [cron job] v4l-dvb daily build 2.6.22 and up: ERRORS, 2.6.16-2.6.21: ERRORS
On Mon, 20 Apr 2009, Alexey Klimov wrote: [...] When trying to compile v4l-dvb tree under 2.6.30-rc2 (up-to-date) i have such error with pvr2 module: CC [M] /w/new/v4l-dvb/v4l/pvrusb2-hdw.o /w/new/v4l-dvb/v4l/pvrusb2-hdw.c: In function 'pvr2_upload_firmware1': /w/new/v4l-dvb/v4l/pvrusb2-hdw.c:1474: error: implicit declaration of function 'usb_settoggle' /w/new/v4l-dvb/v4l/pvrusb2-hdw.c: In function 'pvr2_hdw_load_modules': /w/new/v4l-dvb/v4l/pvrusb2-hdw.c:2133: warning: format not a string literal and no format arguments make[3]: *** [/w/new/v4l-dvb/v4l/pvrusb2-hdw.o] Error 1 make[2]: *** [_module_/w/new/v4l-dvb/v4l] Error 2 It's probably due to this git commit: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=3444b26afa145148951112534f298bdc554ec789 I don't have idea how to fix it fast and correctly. This might explain things a bit. The following thread took place on linux-usb on 7-April: quote On Tue, 7 Apr 2009, Greg KH wrote: On Tue, Apr 07, 2009 at 05:31:55PM +, David Vrabel wrote: Wireless USB endpoint state has a sequence number and a current window and not just a single toggle bit. So allow HCDs to provide a endpoint_reset method and call this or clear the software toggles as required (after a clear halt). usb_settoggle() and friends are then HCD internal and are moved into core/hcd.h. You remove this api, yet the pvrusb2 driver used it, and you don't seem to have resolved the issue where it was calling it: diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.c b/drivers/media/video/pvrusb2/pvrusb2-hdw.c index fa304e5..b86682d 100644 --- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c +++ b/drivers/media/video/pvrusb2/pvrusb2-hdw.c @@ -1418,7 +1418,6 @@ static int pvr2_upload_firmware1(struct pvr2_hdw *hdw) return ret; } - usb_settoggle(hdw-usb_dev, 0 0xf, !(0 USB_DIR_IN), 0); usb_clear_halt(hdw-usb_dev, usb_sndbulkpipe(hdw-usb_dev, 0 0x7f)); pipe = usb_sndctrlpipe(hdw-usb_dev, 0); Should usb_reset_endpoint() be called here instead? Speaking as the maintainer of that driver, I'm OK with accepting this as-is for now. This is a sequence that should not interfere with normal driver operation. I will look at this further a little later (not likely before the merge window closes) and if this change turns out to cause a problem I'll make a follow-up fix upstream. Acked-By: Mike Isely is...@pobox.com -Mike /quote So the kernel already has this; it just needs to be pulled back into v4l-dvb. It's an obvious trivial thing for now and I've acked it there. Obviously we're getting had here because you're compiling against a kernel snapshot that's been changed but v4l-dvb doesn't have the corresponding change in its local copy of the pvrusb2 driver. Part of the fun of synchronizing changes from different trees :-( Mauro: If you just want to take this as-is (or find the git commit and pull it down), I'm fine. Otherwise I'll set up a repo that you can pull from with this single-line change. The above changeset also has the following attributions: From: David Vrabel david.vra...@csr.com Signed-off-by: David Vrabel david.vra...@csr.com -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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 2/6] ir-kbd-i2c: Switch to the new-style device binding model
I thought we were going to leave the pvrusb2 driver out of this since I've already got a change ready that also includes additional logic to take into account the properties of the hardware device (i.e. only activate ir-kbd-i2c when we know it has a chance of working). -Mike On Fri, 17 Apr 2009, Jean Delvare wrote: Let card drivers probe for IR receiver devices and instantiate them if found. Ultimately it would be better if we could stop probing completely, but I suspect this won't be possible for all card types. There's certainly room for cleanups. For example, some drivers are sharing I2C adapter IDs, so they also had to share the list of I2C addresses being probed for an IR receiver. Now that each driver explicitly says which addresses should be probed, maybe some addresses can be dropped from some drivers. Also, the special cases in saa7134-i2c should probably be handled on a per-board basis. This would be more efficient and less risky than always probing extra addresses on all boards. I'll give it a try later. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Mauro Carvalho Chehab mche...@infradead.org Cc: Hans Verkuil hverk...@xs4all.nl Cc: Andy Walls awa...@radix.net Cc: Mike Isely is...@pobox.com --- linux/drivers/media/video/bt8xx/bttv-i2c.c | 21 + linux/drivers/media/video/cx231xx/cx231xx-cards.c| 11 linux/drivers/media/video/cx231xx/cx231xx-i2c.c |3 linux/drivers/media/video/cx231xx/cx231xx.h |1 linux/drivers/media/video/cx23885/cx23885-i2c.c | 12 + linux/drivers/media/video/cx88/cx88-i2c.c| 13 + linux/drivers/media/video/em28xx/em28xx-cards.c | 20 + linux/drivers/media/video/em28xx/em28xx-i2c.c|3 linux/drivers/media/video/em28xx/em28xx-input.c |6 linux/drivers/media/video/em28xx/em28xx.h|1 linux/drivers/media/video/ir-kbd-i2c.c | 200 +++--- linux/drivers/media/video/ivtv/ivtv-i2c.c| 31 ++ linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c | 24 ++ linux/drivers/media/video/saa7134/saa7134-i2c.c |3 linux/drivers/media/video/saa7134/saa7134-input.c| 86 ++- linux/drivers/media/video/saa7134/saa7134.h |1 linux/include/media/ir-kbd-i2c.h |2 17 files changed, 244 insertions(+), 194 deletions(-) [...] -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [RFC] Anticipating lirc breakage
On Tue, 7 Apr 2009, Jean Delvare wrote: I'll rework my patch set to implement strategy #1 and post it when I'm done. As far as I can see this should be very similar to my original attempt, with just ir_video devices instead or ir-kbd devices, and also fixes for the minor issues that have been reported. Do you want me to include pvrusb2 in my new patch set, or should I still leave it to you? If you were to include pvrusb2, you would also need the changeset which expands the IR scheme handling to make it possible to correctly select when to bind the driver. But Mauro hasn't pulled that so you can't easily build on it right now. And as I understand it, the only real impact in the second changeset in the pending series is just the name of the module (i.e. change ir-kbd to ir_video). It's extra work for you to do this. So just let me deal with it. If my understanding above is correct, I'll just fix the second patch and the pvrusb2 driver should be ready to go for this. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- 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: [RFC] Anticipating lirc breakage
On Mon, 6 Apr 2009, Jean Delvare wrote: Hi all, In the light of recent discussions and planed changes to the i2c subsystem and the ir-kbd-i2c driver, I will try to summarize the situation and make some proposals. Note that I am really not sure what we want to do, so this is a true request for opinions. First of all, the original reason for these changes is that I want to get rid of the legacy i2c binding model. As far as IR support is concerned, this means two things: * The ir-kbd-i2c driver must be converted to the new i2c binding model. I have been working on this already. * The removal of the legacy model will break lirc_i2c and possibly other lirc drivers. These will have to be converted to the new i2c binding model as well. While discussing my changes to ir-kbd-i2c, I received objections that these would adversely affect lirc users, and proposals were made to work around this problem, either by the means of module parameters, or by adding per-card logic in the bridge drivers. While this temporarily addresses the conflict with lirc, I feel like it is wasted energy because the second point is much more critical for lirc users. I'm going to remove the legacy i2c model pretty soon now and lirc_i2c and friends will have to be updated. This means two things: It wasn't really wasting that much energy for me. The change was simple and now that you made me look at this issue more closely, I realize I need to do something more serious in the pvrusb2 driver anyway to enable better control over which IR receiver(s) are to be enabled when the user has multiple devices. So in the end for me at least, it wasn't a waste. * There is no point in refining the ir-kbd-i2c conversion for users of the current lirc drivers, because the removal of the legacy i2c model will break these drivers a lot more anyway. * We need to come up with a strategy that makes it possible for lirc modules to still work afterwards. This is not trivial because the new i2c binding model makes life much harder for rogue/out-of-tree i2c drivers (note, this is just a side effect, the new model was not designed with this in mind.) The main technical problems I see as far as converting lirc to the new i2c binding model is concerned are: * Going the .detect() route is not as flexible as it was beforehand. In particular, having per-board probed address lists is no longer possible. It is possible to filter the addresses on a per-board basis after the fact, but the probes will still be issued for all addresses. I seem to remember that probing random addresses did cause trouble in the past on some boards, so I doubt we want to go that route. This is the reason why I decided to NOT go the .detect() route for ir-kbd-i2c conversion. * If we don't use .detect(), the bridge drivers must instantiate the devices themselves. That's what my ir-kbd-i2c patches do. One requirement is then that the bridge drivers and the chip drivers agree on the i2c device name(s). This was true for ir-kbd-i2c, it is true for lirc as well. Where it becomes difficult is that lirc lives outside of the kernel, while bridge drivers live inside the kernel. This will make the changes more difficult to synchronize. Probably a good incentive for lirc developers to merge their drivers into the kernel tree. While I think we all agree that lirc drivers should be merged in the kernel tree, it is pretty clear that it won't happen tomorrow. And, more importantly, it won't happen before I get rid of the legacy i2c binding model. So we need to come up with a design which makes it possible to keep using out-of-tree lirc drivers. It doesn't need to be perfect, but it has to somewhat work for now. Yup. Totally agree. My initial proposal made bridge drivers create ir-kbd [1] I2C devices for the ir-kbd-i2c driver to use. Mike Isely changed this in the pvrusb2 bridge driver to only instantiate the devices for boards on which ir-kbd-i2c is known to work. While this makes sense for the current situation (lirc_i2c is a legacy i2c driver) it will break as soon as lirc_i2c is converted to a new-style i2c driver: the converted lirc_i2c driver will need I2C devices to bind to, and the pvrusb2 driver won't create them. Right, because we haven't addressed the question of still making possible the choice of which actual driver to load. The change I proposed and implemented (within the pvrusb2 driver) was just a simple low-risk attempt to allow for the status quo to still be possible within the pvrusb2 driver. That will work for _right this moment_, but once you've removed the legacy i2c binding mechanism, there's no longer any status quo to maintain. Same goes for cx18 boards, as Andy Walls and myself agreed to not create I2C devices for the IR receivers, because we know that ir-kbd-i2c doesn't support them. This made sense for the current situation, but the lirc_i2c
