[PATCH] [media] dib0700: Fix memory leak during initialization
Reported by kmemleak. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Mauro Carvalho Chehab mche...@infradead.org Cc: Devin Heitmueller dheitmuel...@kernellabs.com --- I am not familiar with the usb API, are we also supposed to call usb_kill_urb() in the error case maybe? drivers/media/dvb/dvb-usb/dib0700_core.c |2 ++ 1 file changed, 2 insertions(+) --- linux-3.3-rc3.orig/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-01-20 14:06:38.0 +0100 +++ linux-3.3-rc3/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-02-12 00:32:19.005334036 +0100 @@ -787,6 +787,8 @@ int dib0700_rc_setup(struct dvb_usb_devi if (ret) err(rc submit urb failed\n); + usb_free_urb(purb); + return ret; } -- Jean Delvare -- 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] cx22702: Fix signal strength
The signal strength value returned is not quite correct, it decreases
when I increase the gain of my antenna, and vice versa. It also
doesn't span over the whole 0x-0x range. Compute a value which
at least increases when signal strength increases, and spans the whole
allowed range.
In practice I get 67% with my antenna fully amplified and 51% with
no amplification. This is close enough to what I get on my other
DVB-T adapter with the same antenna.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Steven Toth st...@kernellabs.com
---
This was written without a datasheet so this is essentially
guess work.
drivers/media/dvb/frontends/cx22702.c | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
--- linux-3.3-rc3.orig/drivers/media/dvb/frontends/cx22702.c2012-02-11
08:28:15.0 +0100
+++ linux-3.3-rc3/drivers/media/dvb/frontends/cx22702.c 2012-02-12
14:45:33.361921465 +0100
@@ -502,10 +502,26 @@ static int cx22702_read_signal_strength(
u16 *signal_strength)
{
struct cx22702_state *state = fe-demodulator_priv;
+ u8 reg23;
- u16 rs_ber;
- rs_ber = cx22702_readreg(state, 0x23);
- *signal_strength = (rs_ber 8) | rs_ber;
+ /*
+* Experience suggests that the strength signal register works as
+* follows:
+* - In the absence of signal, value is 0xff.
+* - In the presence of a weak signal, bit 7 is set, not sure what
+* the lower 7 bits mean.
+* - In the presence of a strong signal, the register holds a 7-bit
+* value (bit 7 is cleared), with greater values standing for
+* weaker signals.
+*/
+ reg23 = cx22702_readreg(state, 0x23);
+ if (reg23 0x80) {
+ *signal_strength = 0;
+ } else {
+ reg23 = ~reg23 0x7f;
+ /* Scale to 16 bit */
+ *signal_strength = (reg23 9) | (reg23 2) | (reg23 5);
+ }
return 0;
}
--
Jean Delvare
--
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] dib0700: Fix memory leak during initialization
Hi Mauro,
Thanks for your reply.
On Thu, 08 Mar 2012 08:21:20 -0300, Mauro Carvalho Chehab wrote:
Em 12-02-2012 08:19, Jean Delvare escreveu:
Reported by kmemleak.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Devin Heitmueller dheitmuel...@kernellabs.com
---
I am not familiar with the usb API, are we also supposed to call
usb_kill_urb() in the error case maybe?
drivers/media/dvb/dvb-usb/dib0700_core.c |2 ++
1 file changed, 2 insertions(+)
--- linux-3.3-rc3.orig/drivers/media/dvb/dvb-usb/dib0700_core.c
2012-01-20 14:06:38.0 +0100
+++ linux-3.3-rc3/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-02-12
00:32:19.005334036 +0100
@@ -787,6 +787,8 @@ int dib0700_rc_setup(struct dvb_usb_devi
if (ret)
err(rc submit urb failed\n);
+ usb_free_urb(purb);
+
return ret;
}
This patch doesn't sound right on my eyes, as you're freeing
an URB that you've just submitted _before_ having it handled
by the dib0700_rc_urb_completion() callback.
Oops, you're totally right. I don't know a thing about USB as you can
see :(
Btw, it seems that there's a bug at the fist if there:
static void dib0700_rc_urb_completion(struct urb *purb)
{
struct dvb_usb_device *d = purb-context;
struct dib0700_rc_response *poll_reply;
u32 uninitialized_var(keycode);
u8 toggle;
deb_info(%s()\n, __func__);
if (d == NULL)
return;
if (d-rc_dev == NULL) {
/* This will occur if disable_rc_polling=1 */
usb_free_urb(purb);
return;
}
...
it should be, instead:
if (!d || !d-rc_dev) {
/* This will occur if disable_rc_polling=1 */
usb_free_urb(purb);
return;
}
!d can't actually happen, so it doesn't matter. d is passed by
dib0700_rc_setup() when calling usb_fill_bulk_urb(), and
dib0700_rc_setup() starts with dereferencing d, if it was NULL we'd
crash right away. Hence d is never NULL in dib0700_rc_urb_completion().
So this if (d == NULL) is just paranoia and might as well be removed.
That's said, clearly there's no condition to stop the DVB IR
handling.
Indeed, as I read the code, unless disable_rc_polling=1 or a fatal
error occurs, dib0700_rc_urb_completion will loop over and over
endlessly. I guess it's what RC polling is all about. No surprise why
my DVB-T card sucks so much power...
Probably, the right thing to do there is to add a function like:
int dib0700_disconnect(...)
{
usb_unlink_urb(urb);
usb_free_urb(urb);
dvb_usb_device_exit(...);
}
and use such function for the usb_driver disconnect handling:
static struct usb_driver dib0700_driver = {
.name = dvb_usb_dib0700,
.probe = dib0700_probe,
.disconnect = dib0700_disconnect,
.id_table = dib0700_usb_id_table,
};
This would avoid a memory leak on module removal, right? Sure, we can
do that, but what surprises me is that I don't remember removing the
module when kmemleak reported the leak to me. Oh well, kmemleak is
pretty new, maybe that was a false positive after all.
But is it OK to free the same URB twice? Your code above does it
unconditionally, while it may have been freed already
(disable_rc_polling=1 or a fatal error occurred). As it seems that
usb_unlink_urb() will call the completion callback with an error
status, and dib0700_rc_urb_completion() will free the URB when that
happens, I suppose it is sufficient to call usb_unlink_urb() in the
diconnect function?
Thanks,
--
Jean Delvare
--
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] dib0700: Fix memory leak during initialization
On Mon, 12 Mar 2012 07:28:02 -0300, Mauro Carvalho Chehab wrote: Em 12-03-2012 07:04, Jean Delvare escreveu: !d can't actually happen, so it doesn't matter. d is passed by dib0700_rc_setup() when calling usb_fill_bulk_urb(), and dib0700_rc_setup() starts with dereferencing d, if it was NULL we'd crash right away. Hence d is never NULL in dib0700_rc_urb_completion(). So this if (d == NULL) is just paranoia and might as well be removed. Ok. Feel free to remove it on your patch. I'll send a patch later today, yes. (...) Indeed, as I read the code, unless disable_rc_polling=1 or a fatal error occurs, dib0700_rc_urb_completion will loop over and over endlessly. I guess it's what RC polling is all about. No surprise why my DVB-T card sucks so much power... This code should not poll anymore, at least with a v1.2 firmware. The URB code will run only when the URB arrives. The URB will only arrive when some key is pressed. At key press, the URB handling code will re-trigger the URB handler to be prepared for a next key. You're completely right. I enabled debugging, I have no remote control on my card and the callback function is simply never called during run time. It is called when I rmmod the driver though, with a negative status value, which causes the urb to be freed. So there is no leak in this case already, even without applying the fix that we discussed earlier. I have no idea who calls dib0700_rc_urb_completion, but the debug log clearly shows it is called. I re-enabled kmemleak to understand exactly what was happening and I get it now. My card behaves differently on cold boots and warm reboots. In case of warm reboots I see the following error message in the log: rc submit urb failed This is where the actual leak occurs, as the URB was never submitted, dib0700_rc_urb_completion is never called, not even on module removal, so the URB never has a chance to be freed. Furthermore, the transfer_buffer of the urb is also never freed, not even in the regular case. So a kfree must be added before any call to usb_free_urb(). Patch follows. (I guess this all means that the remote control wouldn't work on warm reboots, but as I have no remote control I never noticed.) (...) Sure, we can do that, but what surprises me is that I don't remember removing the module when kmemleak reported the leak to me. Oh well, kmemleak is pretty new, maybe that was a false positive after all. It seems weird that kmemleak would warn about a leak before the memory removal. There are several places during driver init where data is alloced, and will only be freed at driver removal. The same happens with device probe/disconnect. If there are no references left to allocated memory, it makes sense to report immediately. -- Jean Delvare -- 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 1/2] [media] dib0700: Drop useless check when remote key is pressed
struct dvb_usb_device *d can never be NULL so don't waste time
checking for this.
Rationale: the urb's context is set when usb_fill_bulk_urb() is called
in dib0700_rc_setup(), and never changes after that. d is dereferenced
unconditionally in dib0700_rc_setup() so it can't be NULL or the
driver would crash right away.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Devin Heitmueller dheitmuel...@kernellabs.com
---
Devin, am I missing something?
drivers/media/dvb/dvb-usb/dib0700_core.c |3 ---
1 file changed, 3 deletions(-)
--- linux-3.3-rc7.orig/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13
11:09:13.0 +0100
+++ linux-3.3-rc7/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13
18:37:05.785953845 +0100
@@ -677,9 +677,6 @@ static void dib0700_rc_urb_completion(st
u8 toggle;
deb_info(%s()\n, __func__);
- if (d == NULL)
- return;
-
if (d-rc_dev == NULL) {
/* This will occur if disable_rc_polling=1 */
usb_free_urb(purb);
--
Jean Delvare
--
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 2/2 v2] [media] dib0700: Fix memory leak during initialization
Reported by kmemleak.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Devin Heitmueller dheitmuel...@kernellabs.com
---
Changes since v1:
* Don't free the URB when it is still in use.
* Fix a second leak (transfer_buffer).
drivers/media/dvb/dvb-usb/dib0700_core.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
--- linux-3.3-rc7.orig/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13
11:20:33.748864483 +0100
+++ linux-3.3-rc7/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13
17:42:20.316570058 +0100
@@ -679,6 +679,7 @@ static void dib0700_rc_urb_completion(st
deb_info(%s()\n, __func__);
if (d-rc_dev == NULL) {
/* This will occur if disable_rc_polling=1 */
+ kfree(purb-transfer_buffer);
usb_free_urb(purb);
return;
}
@@ -687,6 +688,7 @@ static void dib0700_rc_urb_completion(st
if (purb-status 0) {
deb_info(discontinuing polling\n);
+ kfree(purb-transfer_buffer);
usb_free_urb(purb);
return;
}
@@ -781,8 +783,11 @@ int dib0700_rc_setup(struct dvb_usb_devi
dib0700_rc_urb_completion, d);
ret = usb_submit_urb(purb, GFP_ATOMIC);
- if (ret)
+ if (ret) {
err(rc submit urb failed\n);
+ kfree(purb-transfer_buffer);
+ usb_free_urb(purb);
+ }
return ret;
}
--
Jean Delvare
--
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/2] [media] dib0700: Drop useless check when remote key is pressed
Hi Mauro, On Mon, 19 Mar 2012 19:26:11 -0300, Mauro Carvalho Chehab wrote: On Tue, Mar 13, 2012 at 1:50 PM, Jean Delvare kh...@linux-fr.org wrote: --- linux-3.3-rc7.orig/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13 11:09:13.0 +0100 +++ linux-3.3-rc7/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13 18:37:05.785953845 +0100 @@ -677,9 +677,6 @@ static void dib0700_rc_urb_completion(st u8 toggle; deb_info(%s()\n, __func__); - if (d == NULL) - return; - Well, usb_free_urb() is not called when d == NULL, so, if this condition ever happens, it will keep URB's allocated. Anyway, if struct dvb_usb_device *d is NULL, the driver has something very wrong happening on it, and nothing will work on it. I agree with Jean: it is better to just remove this code there. Yet, I'd be more happy if Jean's patch could check first if the status is below 0, in order to prevent a possible race condition at device disconnect. I'm not sure I see the race condition you're seeing. Do you believe purb-context would be NULL (or point to already-freed memory) when dib0700_rc_urb_completion is called as part of device disconnect? Or is it something else? I'll be happy to resubmit my patch series with a fix if you explain where you think there is a race condition. -- Jean Delvare -- 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/2] [media] dib0700: Drop useless check when remote key is pressed
Hi Mauro,
On Tue, 20 Mar 2012 09:17:54 -0300, Mauro Carvalho Chehab wrote:
Em 20-03-2012 04:20, Jean Delvare escreveu:
On Mon, 19 Mar 2012 19:26:11 -0300, Mauro Carvalho Chehab wrote:
Yet, I'd be more happy if Jean's patch could check first if the status is
below 0, in order to prevent a possible race condition at device
disconnect.
I'm not sure I see the race condition you're seeing. Do you believe
purb-context would be NULL (or point to already-freed memory) when
dib0700_rc_urb_completion is called as part of device disconnect? Or is
it something else? I'll be happy to resubmit my patch series with a fix
if you explain where you think there is a race condition.
What I'm saying is that the only potential chance of having a NULL value
for d is at the device disconnect/removal, if is there any bug when waiting
for the URB's to be killed.
So, it would be better to invert the error test logic to:
static void dib0700_rc_urb_completion(struct urb *purb)
{
struct dvb_usb_device *d = purb-context;
struct dib0700_rc_response *poll_reply;
u32 uninitialized_var(keycode);
u8 toggle;
poll_reply = purb-transfer_buffer;
if (purb-status 0) {
deb_info(discontinuing polling\n);
kfree(purb-transfer_buffer);
usb_free_urb(purb);
return;
}
deb_info(%s()\n, __func__);
if (d-rc_dev == NULL) {
/* This will occur if disable_rc_polling=1 */
kfree(purb-transfer_buffer);
usb_free_urb(purb);
return;
}
As, at device disconnect/completion, the status will indicate an error, and
the function will return before trying to de-referenciate rc_dev.
Hmm. I couldn't find any code that would reset purb-context. I tested
2000 rmmod dvb-usb-dib0700 on a 3.3.0 kernel with my two patches
applied, compiled with CONFIG_DEBUG_SLAB=y and CONFIG_DEBUG_VM=y, and
it did not crash nor report any problem. I don't think there is any
race here, so I see no point in changing the code. We just got rid of a
paranoid check, it is not to apply another paranoid patch.
--
Jean Delvare
--
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
[media] m920x: Fix uninitialized variable warning
drivers/media/usb/dvb-usb/m920x.c:91:6: warning: ret may be used
uninitialized in this function [-Wuninitialized]
drivers/media/usb/dvb-usb/m920x.c:70:6: note: ret was declared here
This is real, if a remote control has an empty initialization sequence
we would get success or failure randomly.
OTOH the initialization of ret in m920x_init is needless, the function
returns with an error as soon as an error happens, so the last return
can only be a success and we can hard-code 0 there.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Mauro Carvalho Chehab mche...@redhat.com
Cc: Antonio Ospite osp...@studenti.unina.it
---
Untested, I don't have the hardware.
drivers/media/usb/dvb-usb/m920x.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- linux-3.9-rc4.orig/drivers/media/usb/dvb-usb/m920x.c2013-03-05
10:33:29.497926362 +0100
+++ linux-3.9-rc4/drivers/media/usb/dvb-usb/m920x.c 2013-03-31
11:25:54.009509019 +0200
@@ -67,7 +67,8 @@ static inline int m920x_write(struct usb
static inline int m920x_write_seq(struct usb_device *udev, u8 request,
struct m920x_inits *seq)
{
- int ret;
+ int ret = 0;
+
while (seq-address) {
ret = m920x_write(udev, request, seq-data, seq-address);
if (ret != 0)
@@ -81,7 +82,7 @@ static inline int m920x_write_seq(struct
static int m920x_init(struct dvb_usb_device *d, struct m920x_inits *rc_seq)
{
- int ret = 0, i, epi, flags = 0;
+ int ret, i, epi, flags = 0;
int adap_enabled[M9206_MAX_ADAPTERS] = { 0 };
/* Remote controller init. */
@@ -124,7 +125,7 @@ static int m920x_init(struct dvb_usb_dev
}
}
- return ret;
+ return 0;
}
static int m920x_init_ep(struct usb_interface *intf)
--
Jean Delvare
--
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 v2] [media] m920x: Fix uninitialized variable warning
drivers/media/usb/dvb-usb/m920x.c:91:6: warning: ret may be used
uninitialized in this function [-Wuninitialized]
drivers/media/usb/dvb-usb/m920x.c:70:6: note: ret was declared here
This is real, if a remote control has an empty initialization sequence
we would get success or failure randomly.
OTOH the initialization of ret in m920x_init is needless, the function
returns with an error as soon as an error happens, so the last return
can only be a success and we can hard-code 0 there.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Mauro Carvalho Chehab mche...@redhat.com
Cc: Antonio Ospite osp...@studenti.unina.it
---
An even better and simpler fix, avoids a useless initialization in most
cases and is more consistent. And this is what gcc optimizes the code
to anyway.
drivers/media/usb/dvb-usb/m920x.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- linux-3.9-rc4.orig/drivers/media/usb/dvb-usb/m920x.c2013-03-31
12:03:26.473890149 +0200
+++ linux-3.9-rc4/drivers/media/usb/dvb-usb/m920x.c 2013-03-31
13:11:59.973117266 +0200
@@ -76,12 +76,12 @@ static inline int m920x_write_seq(struct
seq++;
}
- return ret;
+ return 0;
}
static int m920x_init(struct dvb_usb_device *d, struct m920x_inits *rc_seq)
{
- int ret = 0, i, epi, flags = 0;
+ int ret, i, epi, flags = 0;
int adap_enabled[M9206_MAX_ADAPTERS] = { 0 };
/* Remote controller init. */
@@ -124,7 +124,7 @@ static int m920x_init(struct dvb_usb_dev
}
}
- return ret;
+ return 0;
}
static int m920x_init_ep(struct usb_interface *intf)
--
Jean Delvare
--
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] [media] sony-btf-mpx: Drop needless newline in param description
Module parameter descriptions need not be terminated with a newline. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Hans Verkuil hans.verk...@cisco.com Cc: Mauro Carvalho Chehab mche...@redhat.com --- drivers/media/i2c/sony-btf-mpx.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-3.10-rc1.orig/drivers/media/i2c/sony-btf-mpx.c2013-05-13 15:27:46.176134998 +0200 +++ linux-3.10-rc1/drivers/media/i2c/sony-btf-mpx.c 2013-05-15 23:28:55.519803198 +0200 @@ -30,7 +30,7 @@ MODULE_LICENSE(GPL v2); static int debug; module_param(debug, int, 0644); -MODULE_PARM_DESC(debug, debug level 0=off(default) 1=on\n); +MODULE_PARM_DESC(debug, debug level 0=off(default) 1=on); /* #define MPX_DEBUG */ -- Jean Delvare -- 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 0/3] dvb-apps: Improve femon
Improvements to dvb-apps/femon: * femon: Share common code * femon: Display SNR in dB * femon: Handle -EOPNOTSUPP -- Jean Delvare -- 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 2/3] femon: Display SNR in dB
SNR is supposed to be reported by the frontend drivers in dB, so print
it that way for drivers which implement it properly.
---
util/femon/femon.c | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
--- dvb-apps-3ee111da5b3a.orig/util/femon/femon.c 2013-06-02
14:05:00.988323437 +0200
+++ dvb-apps-3ee111da5b3a/util/femon/femon.c2013-06-02 14:05:33.560792474
+0200
@@ -102,11 +102,20 @@ int check_frontend (struct dvbfe_handle
fe_info.lock ? 'L' : ' ');
if (human_readable) {
- printf (signal %3u%% | snr %3u%% | ber %d | unc %d | ,
- (fe_info.signal_strength * 100) / 0x,
- (fe_info.snr * 100) / 0x,
- fe_info.ber,
- fe_info.ucblocks);
+ // SNR should be in units of 0.1 dB but some drivers do
+ // not follow that rule, thus this heuristic.
+ if (fe_info.snr 1000)
+ printf (signal %3u%% | snr %4.1fdB | ber %d |
unc %d | ,
+ (fe_info.signal_strength * 100) /
0x,
+ fe_info.snr / 10.,
+ fe_info.ber,
+ fe_info.ucblocks);
+ else
+ printf (signal %3u%% | snr %3u%% | ber %d |
unc %d | ,
+ (fe_info.signal_strength * 100) /
0x,
+ (fe_info.snr * 100) / 0x,
+ fe_info.ber,
+ fe_info.ucblocks);
} else {
printf (signal %04x | snr %04x | ber %08x | unc %08x |
,
fe_info.signal_strength,
--
Jean Delvare
--
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 3/3] femon: Handle -EOPNOTSUPP
) + printf (ber %08x | , fe_info.ber); + else + printf (ber | ); + if (got_info DVBFE_INFO_UNCORRECTED_BLOCKS) + printf (unc %08x | , fe_info.ucblocks); + else + printf (unc | ); } if (fe_info.lock) -- Jean Delvare -- 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] drxk: change it to use request_firmware_nowait()
Hi Mauro, On Mon, 25 Jun 2012 17:06:28 -0300, Mauro Carvalho Chehab wrote: That's said, IMO, the best approach is to do: 1) add support for asynchronous probe at device core, for devices that requires firmware at probe(). The async_probe() will only be active if !usermodehelper_disabled. 2) export the I2C i2c_lock_adapter()/i2c_unlock_adapter() interface. Both functions are already exported since kernel 2.6.32. However these functions were originally made public for the shared pin case, where two pins can be used for either I2C or something else, and we have to prevent I2C usage when the other function is used. This does not help in your case. What you need additionally is that unlocked flavors of some i2c transfer functions (at least i2c_transfer itself) are exported as well. This isn't necessarily trivial though, as the locking and unlocking are taking place inside i2c_transfer(), not at its boundaries. I'm looking into this now. -- Jean Delvare -- 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] i2c: Export an unlocked flavor of i2c_transfer
Some drivers (in particular for TV cards) need exclusive access to
their I2C buses for specific operations. Export an unlocked flavor
of i2c_transfer to give them full control.
The unlocked flavor has the following limitations:
* Obviously, caller must hold the i2c adapter lock.
* No debug messages are logged. We don't want to log messages while
holding a rt_mutex.
* No check is done on the existence of adap-algo-master_xfer. It
is thus the caller's responsibility to ensure that the function is
OK to call.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Mauro Carvalho Chehab mche...@redhat.com
---
Mauro, would this be OK with you?
drivers/i2c/i2c-core.c | 44 +---
include/linux/i2c.h|3 +++
2 files changed, 36 insertions(+), 11 deletions(-)
--- linux-3.5-rc4.orig/drivers/i2c/i2c-core.c 2012-06-05 16:22:59.0
+0200
+++ linux-3.5-rc4/drivers/i2c/i2c-core.c2012-06-29 12:41:04.707793937
+0200
@@ -1312,6 +1312,37 @@ module_exit(i2c_exit);
*/
/**
+ * __i2c_transfer - unlocked flavor of i2c_transfer
+ * @adap: Handle to I2C bus
+ * @msgs: One or more messages to execute before STOP is issued to
+ * terminate the operation; each message begins with a START.
+ * @num: Number of messages to be executed.
+ *
+ * Returns negative errno, else the number of messages executed.
+ *
+ * Adapter lock must be held when calling this function. No debug logging
+ * takes place. adap-algo-master_xfer existence isn't checked.
+ */
+int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ unsigned long orig_jiffies;
+ int ret, try;
+
+ /* Retry automatically on arbitration loss */
+ orig_jiffies = jiffies;
+ for (ret = 0, try = 0; try = adap-retries; try++) {
+ ret = adap-algo-master_xfer(adap, msgs, num);
+ if (ret != -EAGAIN)
+ break;
+ if (time_after(jiffies, orig_jiffies + adap-timeout))
+ break;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL(__i2c_transfer);
+
+/**
* i2c_transfer - execute a single or combined I2C message
* @adap: Handle to I2C bus
* @msgs: One or more messages to execute before STOP is issued to
@@ -1325,8 +1356,7 @@ module_exit(i2c_exit);
*/
int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
{
- unsigned long orig_jiffies;
- int ret, try;
+ int ret;
/* REVISIT the fault reporting model here is weak:
*
@@ -1364,15 +1394,7 @@ int i2c_transfer(struct i2c_adapter *ada
i2c_lock_adapter(adap);
}
- /* Retry automatically on arbitration loss */
- orig_jiffies = jiffies;
- for (ret = 0, try = 0; try = adap-retries; try++) {
- ret = adap-algo-master_xfer(adap, msgs, num);
- if (ret != -EAGAIN)
- break;
- if (time_after(jiffies, orig_jiffies + adap-timeout))
- break;
- }
+ ret = __i2c_transfer(adap, msgs, num);
i2c_unlock_adapter(adap);
return ret;
--- linux-3.5-rc4.orig/include/linux/i2c.h 2012-06-05 16:23:05.0
+0200
+++ linux-3.5-rc4/include/linux/i2c.h 2012-06-29 10:29:47.865621249 +0200
@@ -68,6 +68,9 @@ extern int i2c_master_recv(const struct
*/
extern int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
int num);
+/* Unlocked flavor */
+extern int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+ int num);
/* This is the very generalized SMBus access routine. You probably do not
want to use this, though; one of the functions below may be much easier,
--
Jean Delvare
--
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: cannot ret error from probe - switch tuner to I2C driver model
Hi Antti,
The driver's .probe() after i2c_new_device() is simply not supposed to
fail. You should only use i2c_new_device() if you are 100% sure that
there is a device of the given type at the given address. Checking
i2c_get_clientdata() is an ugly trick that you should no longer need to
use.
i2c_driver.detect() is used for fully auto-detectable, standalone
devices. Mostly hardware monitoring chips fall into this category. You
should never use that mechanism for video chips, and as a matter of
fact, I2C_CLASS_* flags for analog and digital TV were killed years
ago. To make it clearer: for everything TV/video related, only .probe()
and .remove() are relevant, .detect() is not.
I think you simply want to use i2c_new_probed_device() instead of
i2c_new_device(), so that the i2c client is only instantiated if the
right device is present at the address. In the case where different
chips could be present and you have to differentiate between then, you
can provide a custom detection callback to i2c_new_probed_device(), and
have it return -ENODEV if the right device isn't found. If you don't
provide a custom detection callback, the default one is used
(i2c_probe_func_quick_read in i2c-core.c), that only checks for any
device's presence at a specified address.
Hope that clarifies,
Jean
On Tue, 15 Oct 2013 18:13:39 +0300, Antti Palosaari wrote:
Jean
could you look that and comment how I should implement it properly.
I saw you also added i2c_new_probed_device() [1] that these TV drivers
could be ported properly I2C model.
[1] http://lists.lm-sensors.org/pipermail/i2c/2007-March/000990.html
I am pretty sure I have looked and tested all the I2C pieces quite
carefully. There is .detect() callback which looks just what I need, but
unfortunately it is nor called in case of i2c_new_probed_device() and
i2c_new_device().
On 14.10.2013 05:31, Antti Palosaari wrote:
On 14.10.2013 03:10, Antti Palosaari wrote:
kernel: usb 1-2: rtl2832u_tuner_attach:
kernel: e4000 5-0064: e4000_probe:
kernel: usb 1-2: rtl2832u_tuner_attach: client ptr 88030a849000
See attached patch.
Is there any way to return error to caller?
Abuse platform data ptr from struct i2c_board_info and call
i2c_unregister_device() ?
Answer to myself: best option seems to be check i2c_get_clientdata()
pointer after i2c_new_device().
client = i2c_new_device(d-i2c_adap, info);
if (client)
if (i2c_get_clientdata(client) == NULL)
// OOPS, I2C probe fails
That is because it is set NULL in error case by really_probe() in
drivers/base/dd.c. Error status is also cleared there with comment:
/*
* Ignore errors returned by -probe so that the next driver can try
* its luck.
*/
That is told in I2C documentation too:
Note that starting with kernel 2.6.34, you don't have to set the `data'
field
to NULL in remove() or if probe() failed anymore. The i2c-core does this
automatically on these occasions. Those are also the only times the core
will
touch this field.
But maybe the comment for actual function, i2c_new_device, is still a
bit misleading as it says NULL is returned for the error. All the other
errors yes, but not for the I2C .probe() as it is reseted by device core.
* This returns the new i2c client, which may be saved for later use with
* i2c_unregister_device(); or NULL to indicate an error.
*/
struct i2c_client *
i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
regards
Antti
regards
Antti
---
drivers/media/tuners/e4000.c| 31
+++
drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 18 --
2 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 54e2d8a..f4e0567 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -442,6 +442,37 @@ err:
}
EXPORT_SYMBOL(e4000_attach);
+static int e4000_probe(struct i2c_client *client, const struct
i2c_device_id *did)
+{
+dev_info(client-dev, %s:\n, __func__);
+return -ENODEV;
+}
+
+static int e4000_remove(struct i2c_client *client)
+{
+dev_info(client-dev, %s:\n, __func__);
+return 0;
+}
+
+static const struct i2c_device_id e4000_id[] = {
+{e4000, 0},
+{}
+};
+
+MODULE_DEVICE_TABLE(i2c, e4000_id);
+
+static struct i2c_driver e4000_driver = {
+.driver = {
+.owner= THIS_MODULE,
+.name= e4000,
+},
+.probe= e4000_probe,
+.remove= e4000_remove,
+.id_table= e4000_id,
+};
+
+module_i2c_driver(e4000_driver);
+
MODULE_DESCRIPTION(Elonics E4000 silicon tuner driver);
MODULE_AUTHOR(Antti Palosaari cr...@iki.fi);
MODULE_LICENSE(GPL);
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model
Hi Michael, On Wed, 16 Oct 2013 13:04:42 -0400, Michael Krufky wrote: YIKES!! i2c_new_probed_device() does indeed probe the hardware -- this is unacceptable, as such an action can damage the ic. Is there some additional information that I'm missing that lets this perform an attach without probe? Oh, i2c_new_probed_device() probes the device, what a surprise! :D Try, I don't know, i2c_new_device() maybe if you don't want the probe? ;) -- Jean Delvare -- 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 REVIEW] e4000: convert DVB tuner to I2C driver model
On Wed, 16 Oct 2013 20:45:39 +0300, Antti Palosaari wrote: On 16.10.2013 20:33, Michael Krufky wrote: OK, I get it and it does seem OK. I'm just curious what kind of impact this refactoring would have over something like the b2c2-flexcop-fe driver, who does not know which ic's to attach based on device ids, but it does probe a few frontend combinations one after another, in an order that the driver authors knew was safe. I'd imaging that we'd write some helper abstraction function to switch out the info.type string as each driver gets probed? I think that can get quite ugly, but I know that the general population thinks dvb_attach() is even uglier, so maybe this could be the right path... Wanna take a crack at b2c2-flexcop-fe? heh, look the rtl28xxu driver in question, it does same. It probes maybe total 10 tuners - checking their device ID too. Probing plain I2C device address and making devision from that is simply dead idea. Standard I2C address range for these silicon tuners are 0x60-0x64 = need to read chip ID in order to detect = i2c_new_probed_device() is quite useless. Please remember you can pass a custom probe function to i2c_new_probed_device(). That probe function can read whatever chip ID register exists to decide if the probe should be successful or not. -- Jean Delvare -- 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/3] femon: Display SNR in dB
Hi Manu, On Sun, 24 Nov 2013 22:51:33 +0530, Manu Abraham wrote: Sorry, that I came upon this patch quite late. No problem, better late than never! :) On Mon, Jun 3, 2013 at 8:51 PM, Jean Delvare kh...@linux-fr.org wrote: SNR is supposed to be reported by the frontend drivers in dB, so print it that way for drivers which implement it properly. Not all frontends do report report the SNR in dB. Well, You can say quite some frontends do report it that way. Last time I discussed this, I was told that this was the preferred way for frontends to report the SNR. I also referred to this document: http://palosaari.fi/linux/v4l-dvb/snr_2012-05-21.txt I don't know now up-to-date it is by now, but back then it showed a significant number of frontends reporting in .1 dB already, including the ones I'm using right now (drx-3916k and drx-3913k.) With the current version of femon, femon -H reports it as 0%, which is quite useless. Thus my patch. Making the application report it in dB for frontends which do not will show up as incorrect results, from what I can say. My code has a conditional to interpret high values (= 1000) as % and low values ( 1000) as .1 dB. This is a heuristic which works fine for me in practice (tested on drxk, cx22702 and dib3000mc.) I would certainly welcome testing on other DVB cards. I seem to recall that Mauro suggested that values above 40 dB (?) were not possible so we could probably lower the threshold from 1000 to 400 or so if the current value causes trouble. I think the maximum I've see on my card was ~32 dB. If you find a significant number of frontend drivers for which the heuristic doesn't work, and lowering the threshold doesn't help, then I'm fine considering a different approach such as an extra command line parameter. But if only a small number of drivers cause trouble, I'd say these drivers should simply be fixed. Thanks, -- Jean Delvare -- 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 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization
Hi Julia, On Thu, 11 Oct 2012 08:45:43 +0200 (CEST), Julia Lawall wrote: I found 6 cases where there are more than 2 messages in the array. I didn't check how many cases where there are two messages but there is something other than one read and one write. Perhaps a reasonable option would be to use I2C_MSG_READ I2C_MSG_WRITE I2C_MSG_READ_OP I2C_MSG_WRITE_OP The last two are for the few cases where more flags are specified. As compared to the original proposal of I2C_MSG_OP, these keep the READ or WRITE idea in the macro name. The additional argument to the OP macros would be or'd with the read or write (nothing to do in this case) flags as appropriate. Mauro proposed INIT_I2C_READ_SUBADDR for the very common case where a message array has one read and one write. I think that putting one I2C_MSG_READ and one I2C_MSG_WRITE in this case is readable enough, and avoids the need to do something special for the cases that don't match the expectations of INIT_I2C_READ_SUBADDR. I propose not to do anything for the moment either for sizes or for message or buffer arrays that contain only one element. Please note that I resigned from my position of i2c subsystem maintainer, so I will not handle this. If you think this is important, you'll have to resubmit and Wolfram will decide what he wants to do about it. -- Jean Delvare -- 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] [media] usbvision: Drop broken 10-bit I2C address support
The support for 10-bit I2C addresses in usbvision seems plain broken
to me. I had already noticed that back in February 2007 [1]. The code
was not fixed since then, so I take it that it's not actually needed.
And as a matter of fact I don't know of any 10-bit addressed I2C
tuner, encode, decoder or the like.
So let's simply get rid of the broken and useless code.
I'm also adding I2C_FUNC_I2C, as the driver and hardware support plain
I2C messaging.
[1] http://marc.info/?l=linux-i2cm=117499415208244w=2
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
drivers/media/video/usbvision/usbvision-i2c.c | 46 ++---
1 file changed, 12 insertions(+), 34 deletions(-)
--- linux-3.2-rc0.orig/drivers/media/video/usbvision/usbvision-i2c.c
2011-07-22 04:17:23.0 +0200
+++ linux-3.2-rc0/drivers/media/video/usbvision/usbvision-i2c.c 2011-11-07
09:54:14.0 +0100
@@ -110,42 +110,20 @@ static inline int usb_find_address(struc
unsigned char addr;
int ret;
- if ((flags I2C_M_TEN)) {
- /* a ten bit address */
- addr = 0xf0 | ((msg-addr 7) 0x03);
- /* try extended address code... */
- ret = try_write_address(i2c_adap, addr, retries);
- if (ret != 1) {
- dev_err(i2c_adap-dev,
- died at extended address code, while
writing\n);
- return -EREMOTEIO;
- }
- add[0] = addr;
- if (flags I2C_M_RD) {
- /* okay, now switch into reading mode */
- addr |= 0x01;
- ret = try_read_address(i2c_adap, addr, retries);
- if (ret != 1) {
- dev_err(i2c_adap-dev,
- died at extended address code, while
reading\n);
- return -EREMOTEIO;
- }
- }
- } else {/* normal 7bit address */
- addr = (msg-addr 1);
- if (flags I2C_M_RD)
- addr |= 1;
+ addr = (msg-addr 1);
+ if (flags I2C_M_RD)
+ addr |= 1;
- add[0] = addr;
- if (flags I2C_M_RD)
- ret = try_read_address(i2c_adap, addr, retries);
- else
- ret = try_write_address(i2c_adap, addr, retries);
+ add[0] = addr;
+ if (flags I2C_M_RD)
+ ret = try_read_address(i2c_adap, addr, retries);
+ else
+ ret = try_write_address(i2c_adap, addr, retries);
+
+ if (ret != 1)
+ return -EREMOTEIO;
- if (ret != 1)
- return -EREMOTEIO;
- }
return 0;
}
@@ -184,7 +162,7 @@ usbvision_i2c_xfer(struct i2c_adapter *i
static u32 functionality(struct i2c_adapter *adap)
{
- return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR;
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
}
/* -exported algorithm data: - */
--
Jean Delvare
--
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 1/2] dvb-core: add generic helper function for I2C register
drivers that could benefit from it. At the moment only the tda18218 driver was reported to need it, that's not enough to generalize. You should take a look at drivers/misc/eeprom/at24.c, it contains fairly complete transfer functions which cover the various EEPROM types. Non-EEPROM devices could behave differently, but this would still seem to be a good start for any I2C device using block transfers. It was once proposed that these functions could make their way into i2c-core or a generic i2c helper function. Both at24 and Antti's proposal share the idea of storing information about the device capabilities (max block read and write lengths, but we could also put there alignment requirements or support for repeated start condition.) in a private structure. If we generalize the functions then this information would have to be stored in struct i2c_client and possibly struct i2c_adapter (or struct i2c_algorithm) so that the function can automatically find out the right sequence of commands for the adapter/slave combination. Speaking of struct i2c_client, I seem to remember that the dvb subsystem doesn't use it much at the moment. This might be an issue if you intend to get the generic code into i2c-core, as most helper functions rely on a valid i2c_client structure by design. -- Jean Delvare -- 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 1/2] dvb-core: add generic helper function for I2C register
On Wed, 09 Nov 2011 12:41:36 +0200, Antti Palosaari wrote:
On 11/09/2011 11:56 AM, Mauro Carvalho Chehab wrote:
Due to the way I2C locks are bound, doing something like the above and
something like:
struct i2c_msg msg[2] = {
{
.addr = i2c_cfg-addr,
.flags = 0,
.buf = buf,
},
{
.addr = i2c_cfg-addr,
.flags = 0,
.buf = buf2,
}
};
ret = i2c_transfer(i2c_cfg-adapter, msg, 2);
Produces a different result. In the latter case, I2C core avoids having any
other
transaction in the middle of the 2 messages.
In my understanding adding more messages than one means those should be
handled as one I2C transaction using REPEATED START.
I see one big problem here, it is our adapters. I think again, for the
experience I have, most of our I2C-adapters can do only 3 different
types of I2C xfers;
* I2C write
* I2C write + I2C read (combined with REPEATED START)
* I2C read (I suspect many adapters does not support that)
That means, I2C REPEATED writes are not possible.
Also, some adapters _or slaves_ won't support more than one repeated
start in a given transaction, so splitting block reads in continuous
chunks won't always work either. Which makes some sense if you think
about it: if both the slave and the controller supported larger blocks
then there would be no need to split the transfer into multiple
messages in the first place.
--
Jean Delvare
--
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] [media] video: Drop undue references to i2c-algo-bit
There's one comment that has been copied from bttv to many other
media/video drivers:
/* init + register i2c algo-bit adapter */
Meanwhile, many drivers use hardware I2C implementations instead of
relying on i2c-algo-bit, so this comment is misleading. Remove the
reference to algo-bit from all drivers, to avoid any confusion. This
is the best way to ensure that the comments won't go out of sync
again. Anyone interested in the implementation details would rather
look at the code itself.
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
drivers/media/video/au0828/au0828-i2c.c |2 +-
drivers/media/video/bt8xx/bttv-i2c.c |2 +-
drivers/media/video/cx18/cx18-i2c.c |2 +-
drivers/media/video/cx18/cx18-i2c.h |2 +-
drivers/media/video/cx23885/cx23885-i2c.c |2 +-
drivers/media/video/cx25821/cx25821-i2c.c |2 +-
drivers/media/video/cx88/cx88-i2c.c |2 +-
drivers/media/video/ivtv/ivtv-i2c.h |2 +-
8 files changed, 8 insertions(+), 8 deletions(-)
--- linux-3.2-rc0.orig/drivers/media/video/au0828/au0828-i2c.c 2011-07-22
04:17:23.0 +0200
+++ linux-3.2-rc0/drivers/media/video/au0828/au0828-i2c.c 2011-11-07
18:50:50.0 +0100
@@ -348,7 +348,7 @@ static void do_i2c_scan(char *name, stru
}
}
-/* init + register i2c algo-bit adapter */
+/* init + register i2c adapter */
int au0828_i2c_register(struct au0828_dev *dev)
{
dprintk(1, %s()\n, __func__);
--- linux-3.2-rc0.orig/drivers/media/video/bt8xx/bttv-i2c.c 2011-11-06
17:44:24.0 +0100
+++ linux-3.2-rc0/drivers/media/video/bt8xx/bttv-i2c.c 2011-11-07
18:51:13.0 +0100
@@ -346,7 +346,7 @@ static void do_i2c_scan(char *name, stru
}
}
-/* init + register i2c algo-bit adapter */
+/* init + register i2c adapter */
int __devinit init_bttv_i2c(struct bttv *btv)
{
strlcpy(btv-i2c_client.name, bttv internal, I2C_NAME_SIZE);
--- linux-3.2-rc0.orig/drivers/media/video/cx18/cx18-i2c.c 2011-07-22
04:17:23.0 +0200
+++ linux-3.2-rc0/drivers/media/video/cx18/cx18-i2c.c 2011-11-07
18:50:44.0 +0100
@@ -232,7 +232,7 @@ static struct i2c_algo_bit_data cx18_i2c
.timeout= CX18_ALGO_BIT_TIMEOUT*HZ /* jiffies */
};
-/* init + register i2c algo-bit adapter */
+/* init + register i2c adapter */
int init_cx18_i2c(struct cx18 *cx)
{
int i, err;
--- linux-3.2-rc0.orig/drivers/media/video/cx18/cx18-i2c.h 2011-07-22
04:17:23.0 +0200
+++ linux-3.2-rc0/drivers/media/video/cx18/cx18-i2c.h 2011-11-07
18:50:38.0 +0100
@@ -24,6 +24,6 @@
int cx18_i2c_register(struct cx18 *cx, unsigned idx);
struct v4l2_subdev *cx18_find_hw(struct cx18 *cx, u32 hw);
-/* init + register i2c algo-bit adapter */
+/* init + register i2c adapter */
int init_cx18_i2c(struct cx18 *cx);
void exit_cx18_i2c(struct cx18 *cx);
--- linux-3.2-rc0.orig/drivers/media/video/cx23885/cx23885-i2c.c
2011-11-06 17:44:24.0 +0100
+++ linux-3.2-rc0/drivers/media/video/cx23885/cx23885-i2c.c 2011-11-07
18:51:25.0 +0100
@@ -309,7 +309,7 @@ static void do_i2c_scan(char *name, stru
}
}
-/* init + register i2c algo-bit adapter */
+/* init + register i2c adapter */
int cx23885_i2c_register(struct cx23885_i2c *bus)
{
struct cx23885_dev *dev = bus-dev;
--- linux-3.2-rc0.orig/drivers/media/video/cx25821/cx25821-i2c.c
2011-11-06 17:44:24.0 +0100
+++ linux-3.2-rc0/drivers/media/video/cx25821/cx25821-i2c.c 2011-11-07
18:51:01.0 +0100
@@ -300,7 +300,7 @@ static struct i2c_client cx25821_i2c_cli
.name = cx25821 internal,
};
-/* init + register i2c algo-bit adapter */
+/* init + register i2c adapter */
int cx25821_i2c_register(struct cx25821_i2c *bus)
{
struct cx25821_dev *dev = bus-dev;
--- linux-3.2-rc0.orig/drivers/media/video/cx88/cx88-i2c.c 2011-07-22
04:17:23.0 +0200
+++ linux-3.2-rc0/drivers/media/video/cx88/cx88-i2c.c 2011-11-07
18:51:07.0 +0100
@@ -132,7 +132,7 @@ static void do_i2c_scan(const char *name
}
}
-/* init + register i2c algo-bit adapter */
+/* init + register i2c adapter */
int cx88_i2c_init(struct cx88_core *core, struct pci_dev *pci)
{
/* Prevents usage of invalid delay values */
--- linux-3.2-rc0.orig/drivers/media/video/ivtv/ivtv-i2c.h 2011-07-22
04:17:23.0 +0200
+++ linux-3.2-rc0/drivers/media/video/ivtv/ivtv-i2c.h 2011-11-07
18:50:55.0 +0100
@@ -25,7 +25,7 @@ struct i2c_client *ivtv_i2c_new_ir_legac
int ivtv_i2c_register(struct ivtv *itv, unsigned idx);
struct v4l2_subdev *ivtv_find_hw(struct ivtv *itv, u32 hw);
-/* init + register i2c algo-bit adapter */
+/* init + register i2c adapter */
int init_ivtv_i2c(struct ivtv *itv);
void exit_ivtv_i2c(struct ivtv *itv);
--
Jean Delvare
--
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
Re: [RFC 2/2] tda18218: use generic dvb_wr_regs()
On Thu, 10 Nov 2011 01:01:57 +0200, Antti Palosaari wrote: Hello After today discussion I think at least following configuration options could be possible: address len, for format msg register value len, for format msg max write len, for splitting max read len, for splitting option for repeated start, for splitting conditional error logging callback for I2C-mux (I2C-gate) general functions implemented: wr_regs, rd_regs, wr_reg, rd_reg, wr_reg_mask, wr_reg_bits, rd_reg_bits support for register banks? That's full list of ideas I have as now. At least in first phase I think only basic register reads and writes are enough. Yes, I suggest starting simple, and adding things later on as they become needed. I also suggest spelling out read and write, and also don't forget to prefix your public function names to avoid namespace collisions. I wonder if Jean could be as main leader of development since he has surely best knowledge about I2C and all possible users. Otherwise, I think I could do it only as linux-media common functionality, because I don't have enough knowledge about other users. If you want it to happen fast, then I suggest that you drive development yourself, and indeed implement it as a common linux-media functionality for now. Later on, once the code has been in place for some time with success, if other subsystems need something similar then we can consider moving the code to i2c-core or some generic i2c helper module. If you count on me to drive it, I am afraid it will take months, given my current workload and various tasks with a much higher priority than this. I will however be happy to help with code review. -- Jean Delvare -- 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 1/2] dvb-core: add generic helper function for I2C register
On Wed, 09 Nov 2011 10:02:08 -0200, Mauro Carvalho Chehab wrote: Em 09-11-2011 08:37, Jean Delvare escreveu: Speaking of struct i2c_client, I seem to remember that the dvb subsystem doesn't use it much at the moment. This might be an issue if you intend to get the generic code into i2c-core, as most helper functions rely on a valid i2c_client structure by design. Yes, DVB uses the low level I2C ops. I don't see any reason why not changing it to use struct i2c_client (well, except that such change would require lots of changes and tests). The lots of changes and tests part is indeed the problem. Furthermore I clearly remember discussing this with Michael a couple years ago (during the i2c device driver model rework) and telling him I would never force DVB to make use of i2c_client if they did not want to. I gave my word, taking it back now would be unfair. Well at least the new i2c model would make it possible, this wasn't the case before, but this is such a huge amount of work that I certainly don't want to push this on anyone. -- Jean Delvare -- 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 1/2] dvb-core: add generic helper function for I2C register
Hi Antti, As an additional note, it just occurred to me that what you are working on is somewhat related to Mark Brown's regmap. Look in drivers/base/regmap and see if maybe you can reuse and/or extend Mark's approach. -- Jean Delvare -- 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 10/10] media/dvb: add __init/__exit macros to drivers/media/dvb/bt8xx/bt878.c
On Tue, 22 Dec 2009 09:38:14 +0100, peterhu...@gmx.de wrote:
From: Peter Huewe peterhu...@gmx.de
Trivial patch which adds the __init/__exit macros to the module_init/
module_exit functions of
drivers/media/dvb/bt8xx/bt878.c
Please have a look at the small patch and either pull it through
your tree, or please ack' it so Jiri can pull it through the trivial
tree.
Patch against linux-next-tree, 22. Dez 08:38:18 CET 2009
but also present in linus tree.
Signed-off-by: Peter Huewe peterhu...@gmx.de
Acked-by: Jean Delvare kh...@linux-fr.org
---
drivers/media/dvb/bt8xx/bt878.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/dvb/bt8xx/bt878.c b/drivers/media/dvb/bt8xx/bt878.c
index a24c125..2a0886a 100644
--- a/drivers/media/dvb/bt8xx/bt878.c
+++ b/drivers/media/dvb/bt8xx/bt878.c
@@ -582,7 +582,7 @@ static int bt878_pci_driver_registered;
/* Module management functions */
/***/
-static int bt878_init_module(void)
+static int __init bt878_init_module(void)
{
bt878_num = 0;
bt878_pci_driver_registered = 0;
@@ -600,7 +600,7 @@ static int bt878_init_module(void)
return pci_register_driver(bt878_pci_driver);
}
-static void bt878_cleanup_module(void)
+static void __exit bt878_cleanup_module(void)
{
if (bt878_pci_driver_registered) {
bt878_pci_driver_registered = 0;
--
Jean Delvare
--
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: IR device at I2C address 0x7a
Hi Darek, Adding LMML to Cc. On Wed, 23 Dec 2009 18:10:08 +0100, Daro wrote: I have the problem you described at the mailing list with Asus MyCinema-P7131/P/FM/AV/RC Analog TV Card: IR remote control is not detected and i2c-adapter i2c-3: Invalid 7-bit address 0x7a error occurs. lspci gives the following output: 04:00.0 Multimedia controller: Philips Semiconductors SAA7131/SAA7133/SAA7135 Video Broadcast Decoder (rev d1) dmesg output I enclose in the attachment. I use: Linux DOMOWY 2.6.31-16-generic #53-Ubuntu SMP Tue Dec 8 04:02:15 UTC 2009 x86_64 GNU/Linux I would be gratefull for the help on that. (...) subsystem: 1043:4845, board: ASUS TV-FM 7135 [card=53,autodetected] (...) i2c-adapter i2c-3: Invalid 7-bit address 0x7a saa7133[0]: P7131 analog only, using entry of ASUSTeK P7131 Analog This error message will show on virtually every SAA713x-based board with no known IR setup. It doesn't imply your board has any I2C device at address 0x7a. So chances are that the message is harmless and you can simply ignore it. -- Jean Delvare -- 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: IR device at I2C address 0x7a:
Hi Felix, On Thu, 31 Dec 2009 08:18:51 +0100, Felix Wolfsteller wrote: Sorry to bump into you by mail directly. Feel free to redirect me to other channels and/or quote me. Adding LMML to Cc. My tv card (asus digimatrix, card=62, tuner=66; i think) might exhibit the issue you discussed and apparently patched (http://osdir.com/ml/linux-media/2009-10/msg00078.html). At least I am getting the same error message as quoted. For more or less extensive hardware details, see: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/481449 For the dmesg output containing the adress 0x7a line, see latest comments on that bug. I hope I can help and get helped ;) This error message will show on virtually every SAA713x-based board with no known IR setup. It doesn't imply your board has any I2C device at address 0x7a. So chances are that the message is harmless and you can simply ignore it. -- Jean Delvare -- 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: IR device at I2C address 0x7a
On Wed, 06 Jan 2010 20:10:30 +0100, Daro wrote: W dniu 06.01.2010 19:40, Jean Delvare pisze: On Wed, 06 Jan 2010 18:58:58 +0100, Daro wrote: It is not the error message itself that bothers me but the fact that IR remote control device is not detected and I cannot use it (I checked it on Windows and it's working). After finding this thread I thought it could have had something to do with this error mesage. Is there something that can be done to get my IR remote control working? Did it ever work on Linux? I have no experience on that. I bought this card just few weeks ago and tried it only on Karmic Koala. OK. You could try loading the saa7134 driver with option card=146 and see if it helps. -- Jean Delvare -- 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: IR device at I2C address 0x7a
On Sat, 09 Jan 2010 13:08:36 +0100, Daro wrote: W dniu 06.01.2010 21:21, Jean Delvare pisze: On Wed, 06 Jan 2010 18:58:58 +0100, Daro wrote: It is not the error message itself that bothers me but the fact that IR remote control device is not detected and I cannot use it (I checked it on Windows and it's working). After finding this thread I thought it could have had something to do with this error mesage. Is there something that can be done to get my IR remote control working? You could try loading the saa7134 driver with option card=146 and see if it helps. It works! [ 15.477875] input: saa7134 IR (ASUSTeK P7131 Analo as /devices/pci:00/:00:1e.0/:05:00.0/input/input8 Thank you very much fo your help. Then I would suggest the following patch: * * * * * From: Jean Delvare kh...@linux-fr.org Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131 Analog (card=146). However, by the time we find out, some card-specific initialization is missed. In particular, the fact that the IR is GPIO-based. Set it when we change the card type. Signed-off-by: Jean Delvare kh...@linux-fr.org Tested-by: Daro ghost-ri...@aster.pl --- linux/drivers/media/video/saa7134/saa7134-cards.c |1 + 1 file changed, 1 insertion(+) --- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-cards.c 2009-12-11 09:47:47.0 +0100 +++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-cards.c 2010-01-09 16:23:17.0 +0100 @@ -7257,6 +7257,7 @@ int saa7134_board_init2(struct saa7134_d printk(KERN_INFO %s: P7131 analog only, using entry of %s\n, dev-name, saa7134_boards[dev-board].name); + dev-has_remote = SAA7134_REMOTE_GPIO; } break; case SAA7134_BOARD_HAUPPAUGE_HVR1150: * * * * * I have another question regarding this driver: [ 21.340316] saa7133[0]: dsp access error [ 21.340320] saa7133[0]: dsp access error Do those messages imply something wrong? Can they have something do do with the fact I cannot get the sound out of tvtime application directly and have to use arecord | aplay workaround which causes undesirable delay? Yes, the message is certainly related to your sound problem. Maybe support for your card is incomplete. But I can't help with this, sorry. -- Jean Delvare -- 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: IR device at I2C address 0x7a
On Sun, 10 Jan 2010 00:18:46 +0100, hermann pitton wrote: Hi, Am Samstag, den 09.01.2010, 17:14 +0100 schrieb Jean Delvare: On Sat, 09 Jan 2010 13:08:36 +0100, Daro wrote: W dniu 06.01.2010 21:21, Jean Delvare pisze: On Wed, 06 Jan 2010 18:58:58 +0100, Daro wrote: It is not the error message itself that bothers me but the fact that IR remote control device is not detected and I cannot use it (I checked it on Windows and it's working). After finding this thread I thought it could have had something to do with this error mesage. Is there something that can be done to get my IR remote control working? You could try loading the saa7134 driver with option card=146 and see if it helps. It works! [ 15.477875] input: saa7134 IR (ASUSTeK P7131 Analo as /devices/pci:00/:00:1e.0/:05:00.0/input/input8 Thank you very much fo your help. Then I would suggest the following patch: * * * * * From: Jean Delvare kh...@linux-fr.org Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131 Analog (card=146). However, by the time we find out, some card-specific initialization is missed. In particular, the fact that the IR is GPIO-based. Set it when we change the card type. Signed-off-by: Jean Delvare kh...@linux-fr.org Tested-by: Daro ghost-ri...@aster.pl just to note it, the ASUS TV-FM 7135 with USB remote is different to the Asus My Cinema P7134 Analog only, not only for the remote, but also for inputs, but they have the same PCI subsystem. --- linux/drivers/media/video/saa7134/saa7134-cards.c |1 + 1 file changed, 1 insertion(+) --- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-cards.c 2009-12-11 09:47:47.0 +0100 +++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-cards.c 2010-01-09 16:23:17.0 +0100 @@ -7257,6 +7257,7 @@ int saa7134_board_init2(struct saa7134_d printk(KERN_INFO %s: P7131 analog only, using entry of %s\n, dev-name, saa7134_boards[dev-board].name); + dev-has_remote = SAA7134_REMOTE_GPIO; } break; case SAA7134_BOARD_HAUPPAUGE_HVR1150: * * * * * Must have been broken at that time, IIRC. What must have been broken, and when? You are confusing. Only moving saa7134_input_init1(dev) to static int saa7134_hwinit2 in saa7134-core.c did help, AFAIK, but I might be wrong. I admit I don't quite get why dev-has_remove should be set early (in saa7134_board_init1) given that for one board (SAA7134_BOARD_FLYDVB_TRIO) it is set later (in saa7134_board_init2) and apparently it works OK. It would make more sense to do it at the same time for all boards IMHO, possibly in a separate function to make it clearer. I am also curious if it wouldn't be even clearer and more efficient to store the default value of has_remote in struct saa7134_board. As far as I can see, only the SAA7134_BOARD_FLYDVB_TRIO needs a run-time check. -- Jean Delvare -- 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: I2C transaction questions: irq context and client locking
will suffer. Hope that helps, -- Jean Delvare -- 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] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
Hi Mauro, Hermann,
On Sat, 30 Jan 2010 01:47:41 +0100, hermann pitton wrote:
Am Freitag, den 29.01.2010, 13:40 -0200 schrieb Mauro Carvalho Chehab:
Jean Delvare wrote:
From: Jean Delvare kh...@linux-fr.org
Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants
Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131
Analog (card=146). However, by the time we find out, some
card-specific initialization is missed. In particular, the fact that
the IR is GPIO-based. Set it when we change the card type.
We also have to move the initialization of IR until after the card
number has been changed. I hope that this won't cause any problem.
Hi Jean,
Moving the initialization will likely cause regressions. The reason why
there
are two init codes there were due to the way the old i2c code used to work.
This got fixed after the i2c rework, but it caused regressions on that time.
I don't think there is any problem with having two init sequences. You
need the EEPROM to identify some devices, you need I2C support to
access the EEPROM, and you need some initialization to get I2C
operational.
This doesn't mean that some adjustments to the exact sequence aren't
possible. In particular, I don't see what else can depend on IR being
initialized, so I can't really see what harm can be done in moving IR
init code _later_ in the sequence. Looking at saa7134_input_init1(), I
see that it is essentially setting up software parameters in the
saa7134_dev structure, there is almost no hardware access. Only for a
few cards, we change a couple bits in registers GPIO_GPMODE* and
GPIO_GPSTATUS*. I honestly can't see how doing this _later_ in the init
sequence could be a problem.
The proper way would be to just muve the IR initialization on this board
from init1 to init2, instead of changing it for all other devices.
Hmm, OK. I think it's wrong, but I'm not the one to decide. Patch below.
Mauro, I do agree with you that it is likely better to go a way with
minimum chances for regressions, also given the current testing base and
that only this single card is involved..
I admit I am very surprised that we apparently can't get anyone to test
changes to a driver that supports 176 different models of TV cards :(
Do we end up with something card specific in core code here?
After all, we know this is a no go.
Hartmut and me thought back and forth on how to deal with it for quite
some while, unfortunately Hartmut is not present currently on the list,
but he voted for to have a separate entry for that card finally too.
What we seem to have now is:
1. We don't know, if Jean's patch really would cause regressions,
but it is likely hard to get all the testing done. No problems with a
FlyVideo3000 gpio remote at the time Roman suggested it, but I had
not any i2c remote that time ...
I doubt it matters, given that saa7134_input_init1() only cares about
GPIO-based IR:
int saa7134_input_init1(struct saa7134_dev *dev)
{
(...)
if (dev-has_remote != SAA7134_REMOTE_GPIO)
return -ENODEV;
So the moving the call to this function should have no effect on boards
with I2C-based IR.
(...)
Given what is also in the cruft for bttv, I would not care too much for
that single card on that now also ancient driver, just print what the
user can do to escape and any google would find it quickly too. For Asus
it is a unique problem on that driver so far.
This isn't how we're going to make Linux popular.
I should have some time on Sunday afternoon for testing, if we should go
that way.
Any testing you can provide is very welcome, thanks.
* * * * *
From: Jean Delvare kh...@linux-fr.org
Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants
Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131
Analog (card=146). However, by the time we find out, some
card-specific initialization is missed. In particular, the fact that
the IR is GPIO-based. Set it when we change the card type, and run
saa7134_input_init1().
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Daro ghost-ri...@aster.pl
Cc: Roman Kellner muzu...@gmx.net
---
linux/drivers/media/video/saa7134/saa7134-cards.c |5 +
1 file changed, 5 insertions(+)
--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-cards.c
2010-01-30 10:56:50.0 +0100
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-cards.c 2010-01-30
11:52:18.0 +0100
@@ -7299,6 +7299,11 @@ int saa7134_board_init2(struct saa7134_d
printk(KERN_INFO %s: P7131 analog only, using
entry of %s\n,
dev-name, saa7134_boards[dev-board].name);
+
+ /* IR init has already happened for other cards, so
+* we have to catch up. */
+ dev-has_remote = SAA7134_REMOTE_GPIO
Re: [PATCH] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
Hi Hermann, On Mon, 01 Feb 2010 02:16:35 +0100, hermann pitton wrote: For now, I only faked a P7131 Dual with a broken IR receiver on a 2.6.29 with recent, you can see that gpio 0x4 doesn't go high, but your patch should enable the remote on that P7131 analog only. I'm not sure why you had to fake anything? What I'd like to know is simply if my first patch had any negative effect on other cards. -- Jean Delvare -- 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] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
Hi Hermann, On Tue, 02 Feb 2010 02:47:53 +0100, hermann pitton wrote: Hi Jean, Am Montag, den 01.02.2010, 10:56 +0100 schrieb Jean Delvare: Hi Hermann, On Mon, 01 Feb 2010 02:16:35 +0100, hermann pitton wrote: For now, I only faked a P7131 Dual with a broken IR receiver on a 2.6.29 with recent, you can see that gpio 0x4 doesn't go high, but your patch should enable the remote on that P7131 analog only. I'm not sure why you had to fake anything? What I'd like to know is simply if my first patch had any negative effect on other cards. because I simply don't have that Asus My Cinema analog only in question. To recap, you previously announced a patch, tested by Daro, claiming to get the remote up under auto detection for that device and I told you having some doubts on it. My first patch was not actually tested by Daro. What he tested was loading the driver with card=146. At first I thought it was equivalent, but since then I have realized it wasn't. That's the reason why the Tested-by: was turned into a mere Cc: on my second and third patches. Mauro prefers to have a fix for that single card in need for now. Since nobody else cares, For now, see above, I can confirm that your last patch for that single device should work to get IR up with auto detection in delay after we change the card such late with eeprom detection. The meaning of that byte in use here is unknown to me, we should avoid such as much we can! It can turn out to be only some pseudo service. If your call for testers on your previous attempt, really reaches some for some reason, I'm with you, but for now I have to keep the car operable within all such snow. That I understand. What I don't understand is: if you have a SAA7134-based card, why don't you test my second patch (the one moving the call to saa7134_input_init1 to saa7134_hwinit2) on it, without faking anything? This would be a first, useful data point. Thanks, -- Jean Delvare -- 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] adv7180 builds since kernel 2.6.26
VIDEO_ADV7180 is listed twice in v4l/versions.txt: once in [2.6.31] and once in [2.6.26]. As I have tested that it builds fine in 2.6.26, drop the former entry. Signed-off-by: Jean Delvare kh...@linux-fr.org --- v4l/versions.txt |1 - 1 file changed, 1 deletion(-) --- v4l-dvb.orig/v4l/versions.txt 2010-01-25 21:25:50.0 +0100 +++ v4l-dvb/v4l/versions.txt2010-02-05 15:13:47.0 +0100 @@ -18,7 +18,6 @@ VIDEO_DM355_CCDC # Start version for those drivers - probably compile with older versions VIDEO_CX25821 VIDEO_CX25821_ALSA -VIDEO_ADV7180 RADIO_TEF6862 # follow_pfn needed by VIDEOBUF_DMA_CONTIG and drivers that use it VIDEOBUF_DMA_CONTIG -- Jean Delvare -- 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] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
Hi Daro, On Wed, 10 Feb 2010 17:38:18 +0100, Daro wrote: If some tests on my machine could be helpfull just let me know. Definitely. If you could please test both patches I sent (first one on 2010-01-27, second one on 2010-01-30, both should be in your mailbox) and confirm that they both work for you (so you no longer need to pass card=146 to the driver), that would be great. Mauro doesn't seem to be willing to apply the first patch, but for future reference I would still be interested to know if it would work at least in your case. The second patch is what Mauro is likely to apply, so it would be good to make sure it fixes your problem before actually applying it. Thanks! -- Jean Delvare -- 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] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
Hi Mauro, On Tue, 02 Feb 2010 17:09:05 -0200, Mauro Carvalho Chehab wrote: From: Jean Delvare kh...@linux-fr.org Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131 Analog (card=146). However, by the time we find out, some card-specific initialization is missed. In particular, the fact that the IR is GPIO-based. Set it when we change the card type, and run saa7134_input_init1(). Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Daro ghost-ri...@aster.pl Cc: Roman Kellner muzu...@gmx.net --- linux/drivers/media/video/saa7134/saa7134-cards.c |5 + 1 file changed, 5 insertions(+) --- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-cards.c 2010-01-30 10:56:50.0 +0100 +++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-cards.c 2010-01-30 11:52:18.0 +0100 @@ -7299,6 +7299,11 @@ int saa7134_board_init2(struct saa7134_d printk(KERN_INFO %s: P7131 analog only, using entry of %s\n, dev-name, saa7134_boards[dev-board].name); + + /* IR init has already happened for other cards, so +* we have to catch up. */ + dev-has_remote = SAA7134_REMOTE_GPIO; + saa7134_input_init1(dev); } break; case SAA7134_BOARD_HAUPPAUGE_HVR1150: This version of your patch makes sense to me. This logic will only apply for board SAA7134_BOARD_ASUSTeK_P7131_ANALOG, so, provided that someone with this board test it, I'm OK with it. Had Roman or Daro already test it? Not yet, but Daro just volunteered to do so... let's give him/her some time to proceed. -- Jean Delvare -- 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] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
SAA7134_BOARD_HAUPPAUGE_HVR1120 SAA7134_BOARD_HAUPPAUGE_HVR1150 SAA7134_BOARD_PHILIPS_TIGER_S SAA7134_BOARD_PINNACLE_PCTV_310 SAA7134_BOARD_VIDEOMATE_DVBT_200 SAA7134_BOARD_VIDEOMATE_DVBT_200A SAA7134_BOARD_VIDEOMATE_DVBT_300 So, there are a large set of boards that will be affected by this change. Your premise that the init1 only cares about GPIO IR is not true. It does contain the GPIO initializations to reset, turn on devices and enable i2c bridges for those devices. Eventually, on a few boards, the gpio's are only due to IR, but this is an exception. The current code does that: saa7134_board_init1(dev); saa7134_hwinit1(dev); msleep(100); saa7134_i2c_register(dev); saa7134_board_init2(dev); saa7134_hwinit2(dev); What happens is that the saa7134_board_init1(dev) code has lots of gpio codes, and most of those code is needed in order to enable i2c bridges or to turn on/reset some chips that would otherwise be on OFF or undefined state. Without the gpio code, done by init1, you may not be able to read eeprom or to detect the devices as needed by saa7134_board_init2(). I don't think I ever said otherwise. I never said that init1 as a whole only cared about GPIO IR. That's what I said of function saa7134_input_init1() and only this function! My first proposed patch didn't move all of init1 to init2. It was only moving saa7134_input_init1 (which doesn't yet have a counterpart in init2), and it is moving it from the end of init1 to the beginning of init2, so the movement isn't as big as it may sound. The steps saa7134_input_init1() is moving over are, in order: * saa7134_hw_enable1 * request_irq * saa7134_i2c_register So my point was that none of these 3 functions seemed to be dependent on saa7134_input_init1() having been called beforehand. Which is why I strongly question the fact that moving saa7134_input_init1() _after_ these 3 other initialization steps can have any negative effect. I wouldn't have claimed that moving saa7134_input_init1() _earlier_ in the init sequence would be safe, because there's nothing obvious about that. But moving it forward where I had put it did not seem terrific. I really would like a few users of this driver to just give it a try and report, because it seems a much more elegant fix to the bug at hand, than the workaround you'd accept instead. That's why I'm saying that, if in the specific case of the board you're dealing with, you're sure that an unknown GPIO state won't affect the code at saa7134_hwinit1() and at saa7134_i2c_register(), then you can move the GPIO code for this board to init2. Moving things between init1 and init2 are very tricky and requires testing on all the affected boards. So a change like what your patch proposed would require a test of all the 107 boards that are on init1. I bet you'll break several of them with this change. Under the assumption that saa7134_hwinit1() only touches GPIOs connected to IR receivers (and it certainly looks like this to me) I fail to see how these pins not being initialized could have any effect on non-IR code. Now, please don't get me wrong. I don't have any of these devices. I've posted that patch because a user incidentally pointed me to a problem and I had an idea how it could be fixed. I prefer my initial proposal because it looks better in the long run. If you don't like it and prefer the second proposal even though I think it's more of a workaround than a proper fix, it's really up to you. You're maintaining the subsystem and I am not, so you're the one deciding. Thanks, -- Jean Delvare -- 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] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
On Wed, 10 Feb 2010 16:40:03 -0200, Mauro Carvalho Chehab wrote: Jean Delvare wrote: Under the assumption that saa7134_hwinit1() only touches GPIOs connected to IR receivers (and it certainly looks like this to me) I fail to see how these pins not being initialized could have any effect on non-IR code. Now, i suspect that you're messing things again: are you referring to saa7134_hwinit1() or to saa7134_input_init1()? I suspect that you're talking about moving saa7134_input_init1(), since saa7134_hwinit1() has the muted and spinlock inits. It also has the setups for video, vbi and mpeg. So, moving it require more care. Err, you're right, I meant saa7134_input_init1() and not saa7134_hwinit1(), copy-and-paste error. Sorry for adding more confusion where it really wasn't needed... -- Jean Delvare -- 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 1/2] bttv: Move I2C IR initialization
Move I2C IR initialization from just after I2C bus setup to right
before non-I2C IR initialization. This avoids the case where an I2C IR
device is blocking audio support (at least the PV951 suffers from
this). It is also more logical to group IR support together,
regardless of the connectivity.
This fixes bug #15184:
http://bugzilla.kernel.org/show_bug.cgi?id=15184
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
As this fixes a regression, I suggest pushing to Linus quickly. This is
a candidate for 2.6.32-stable too.
linux/drivers/media/video/bt8xx/bttv-driver.c |1 +
linux/drivers/media/video/bt8xx/bttv-i2c.c| 10 +++---
linux/drivers/media/video/bt8xx/bttvp.h |1 +
3 files changed, 9 insertions(+), 3 deletions(-)
--- v4l-dvb.orig/linux/drivers/media/video/bt8xx/bttv-i2c.c 2009-12-11
09:47:47.0 +0100
+++ v4l-dvb/linux/drivers/media/video/bt8xx/bttv-i2c.c 2010-02-16
18:14:34.0 +0100
@@ -409,9 +409,14 @@ int __devinit init_bttv_i2c(struct bttv
}
if (0 == btv-i2c_rc i2c_scan)
do_i2c_scan(btv-c.v4l2_dev.name, btv-i2c_client);
-#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
- /* Instantiate the IR receiver device, if present */
+ return btv-i2c_rc;
+}
+
+/* Instantiate the I2C IR receiver device, if present */
+void __devinit init_bttv_i2c_ir(struct bttv *btv)
+{
+#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
if (0 == btv-i2c_rc) {
struct i2c_board_info info;
/* The external IR receiver is at i2c address 0x34 (0x35 for
@@ -432,7 +437,6 @@ int __devinit init_bttv_i2c(struct bttv
i2c_new_probed_device(btv-c.i2c_adap, info, addr_list);
}
#endif
- return btv-i2c_rc;
}
int __devexit fini_bttv_i2c(struct bttv *btv)
--- v4l-dvb.orig/linux/drivers/media/video/bt8xx/bttvp.h2009-04-06
10:10:24.0 +0200
+++ v4l-dvb/linux/drivers/media/video/bt8xx/bttvp.h 2010-02-16
18:13:31.0 +0100
@@ -281,6 +281,7 @@ extern unsigned int bttv_debug;
extern unsigned int bttv_gpio;
extern void bttv_gpio_tracking(struct bttv *btv, char *comment);
extern int init_bttv_i2c(struct bttv *btv);
+extern void init_bttv_i2c_ir(struct bttv *btv);
extern int fini_bttv_i2c(struct bttv *btv);
#define bttv_printk if (bttv_verbose) printk
--- v4l-dvb.orig/linux/drivers/media/video/bt8xx/bttv-driver.c 2009-12-11
09:47:47.0 +0100
+++ v4l-dvb/linux/drivers/media/video/bt8xx/bttv-driver.c 2010-02-16
18:13:31.0 +0100
@@ -4498,6 +4498,7 @@ static int __devinit bttv_probe(struct p
request_modules(btv);
}
+ init_bttv_i2c_ir(btv);
bttv_input_init(btv);
/* everything is fine */
--
Jean Delvare
--
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 (29 patches)
On Wed, 24 Feb 2010 10:10:16 -0300, Mauro Carvalho Chehab wrote: Hi Jean, Jean Delvare wrote: I have 3 patches pending which aren't in your list. I can see them in patchwork: http://patchwork.kernel.org/patch/79755/ http://patchwork.kernel.org/patch/79754/ http://patchwork.kernel.org/patch/77349/ The former two are in Accepted state, and actually I received an e-mail telling me they had been accepted, however I can't see them in the hg repository. So where are they? They are already on the git tree: commit 2887117b31b77ebe5fb42f95ea8d77a3716b405b Author: Jean Delvare kh...@linux-fr.org Date: Tue Feb 16 14:22:37 2010 -0300 V4L/DVB: bttv: Let the user disable IR support Add a new module parameter disable_ir to disable IR support. Several other drivers do that already, and this can be very handy for debugging purposes. Signed-off-by: Jean Delvare kh...@linux-fr.org Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com commit e151340a2a9e7147eb48114af0381122130266b0 Author: Jean Delvare kh...@linux-fr.org Date: Fri Feb 19 00:18:41 2010 -0300 V4L/DVB: bttv: Move I2C IR initialization Move I2C IR initialization from just after I2C bus setup to right before non-I2C IR initialization. This avoids the case where an I2C IR device is blocking audio support (at least the PV951 suffers from this). It is also more logical to group IR support together, regardless of the connectivity. This fixes bug #15184: http://bugzilla.kernel.org/show_bug.cgi?id=15184 Signed-off-by: Jean Delvare kh...@linux-fr.org CC: sta...@kernel.org Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com As patches in -hg are manually backported, maybe Douglas haven't backported it yet or he simply missed. Douglas, could you please check this? Ah, my bad. I totally missed that you had moved from hg to git for the main development tree. I was still pulling from hg and basing my patches on it. I will clone the git tree now, sorry for the confusion. The latter is in Not Applicable state, and I have no idea what it means. The patch is really simple and I see no formatting issue. Should I just resend it? This means that this patch is not applicable on -git. There's no versions.txt upstream. All patches that don't have upstream code are marked as such on patchwork. I generally ping Douglas on such cases, for him to double check on -hg. Anyway, the better is to c/c to Douglas on all patches that are meant only to the building system. Douglas, could you please check if you've applied this patch? Thanks Douglas. -- Jean Delvare -- 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] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
On Sat, 20 Feb 2010 04:07:05 +0100, hermann pitton wrote: Are you still waiting for Daro's report? Yes, I am still waiting. Unfortunately neither Daro nor Roman reported any test result so far. Now, if we go for my second patch, I guess we might as well apply it right now. It only affects this one card (Asus P7131 analog), and it was broken so far, so I don't think my patch can do any bad. As said, I would prefer to see all OEMs _not_ following Philips/NXP eeprom rules running into their own trash on GNU/Linux too. Then we have facts. That is much better than to provide a golden cloud for them. At least I won't help to debug such later ... If you did not manage to decipher all OEM eeprom content already, just let's go with the per card solution for now. Are you aware, that my intention is _not_ to spread the use of random and potentially invalid eeprom content for some sort of such auto detection? The other solution is not lost and in mind, if we should need to come back to it and are in details. Preferably the OEMs should take the responsibility for such. We can see, that even those always doing best on it, can't provide the missing informations for different LNA stuff after the Hauppauge/Pinnacle merge until now. If you claim to know it better, please share with us. I'm not claiming anything, and to be honest, I have no idea what you're talking about. -- Jean Delvare -- 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] bttv: fix compiler warning before kernel 2.6.30
On Sat, 06 Mar 2010 10:25:24 +0100, Németh Márton wrote:
From: Márton Németh nm...@freemail.hu
Fix the following compiler warnings when compiling before Linux
kernel version 2.6.30:
bttv-i2c.c: In function 'init_bttv_i2c':
bttv-i2c.c:440: warning: control reaches end of non-void function
Signed-off-by: Márton Németh nm...@freemail.hu
Good catch.
Acked-by: Jean Delvare kh...@linux-fr.org
Douglas, please apply quickly.
---
diff -r 41c5482f2dac linux/drivers/media/video/bt8xx/bttv-i2c.c
--- a/linux/drivers/media/video/bt8xx/bttv-i2c.c Thu Mar 04 02:49:46
2010 -0300
+++ b/linux/drivers/media/video/bt8xx/bttv-i2c.c Sat Mar 06 10:22:55
2010 +0100
@@ -409,7 +409,6 @@
}
if (0 == btv-i2c_rc i2c_scan)
do_i2c_scan(btv-c.v4l2_dev.name, btv-i2c_client);
-#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
return btv-i2c_rc;
}
@@ -417,6 +416,7 @@
/* Instantiate the I2C IR receiver device, if present */
void __devinit init_bttv_i2c_ir(struct bttv *btv)
{
+#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
if (0 == btv-i2c_rc) {
struct i2c_board_info info;
/* The external IR receiver is at i2c address 0x34 (0x35 for
--
Jean Delvare
--
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: [IR RC, REGRESSION] Didn't work IR RC
Hi Mauro, Dmitri, On Tue, 02 Mar 2010 05:49:17 -0300, Mauro Carvalho Chehab wrote: Dmitri Belimov wrote: When I add diff -r 37ff78330942 linux/drivers/media/video/ir-kbd-i2c.c --- a/linux/drivers/media/video/ir-kbd-i2c.cSun Feb 28 16:59:57 2010 -0300 +++ b/linux/drivers/media/video/ir-kbd-i2c.cTue Mar 02 10:31:31 2010 +0900 @@ -465,6 +519,11 @@ ir_type = IR_TYPE_OTHER; ir_codes= ir_codes_avermedia_cardbus_table; break; + case 0x2d: + /* Handled by saa7134-input */ + name= SAA713x remote; + ir_type = IR_TYPE_OTHER; + break; } #if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30) The IR subsystem register event device. But for get key code use ir_pool_key function. For our IR RC need use our special function. How I can do it? Just add your get_key callback to ir-get_key. If you want to do this from saa7134-input, please take a look at the code at em28xx_register_i2c_ir(). It basically fills the platform_data. While you're there, I suggest you to change your code to work with the full scancode (e. g. address + command), instead of just getting the command. Currently, em28xx-input has one I2C IR already changed to this mode (seek for full_code for the differences). You'll basically need to change the IR tables to contain address+command, and inform the used protocol (RC5/NEC) on it. The getkey function will need to return the full code as well. Sorry for the late reply. Is the problem solved by now, or is my help still needed? -- Jean Delvare -- 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: [IR RC, REGRESSION] Didn't work IR RC
On Wed, 10 Mar 2010 13:02:25 +0900, Dmitri Belimov wrote: Sorry for the late reply. Is the problem solved by now, or is my help still needed? Yes. I found what happens and solve this regression. Patch already comitted. diff -r 37ff78330942 linux/drivers/media/video/saa7134/saa7134-input.c --- a/linux/drivers/media/video/saa7134/saa7134-input.c Sun Feb 28 16:59:57 2010 -0300 +++ b/linux/drivers/media/video/saa7134/saa7134-input.c Thu Mar 04 08:35:15 2010 +0900 @@ -947,6 +947,7 @@ dev-init_data.name = BeholdTV; dev-init_data.get_key = get_key_beholdm6xx; dev-init_data.ir_codes = ir_codes_behold_table; + dev-init_data.type = IR_TYPE_NEC; info.addr = 0x2d; #endif break; None of my patches removed this statement, and IR_TYPE_NEC itself seems to be new in kernel 2.6.33, so I admit I don't quite understand how I my i2c changes could be responsible for the regression. Anyway, glad that you managed to fix it. -- Jean Delvare -- 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] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
Hi Daro, On Fri, 26 Feb 2010 17:19:38 +0100, Daro wrote: I did not forget I had offered to test the patch however I am now on vacation skiing so I will get back to you as soon I am back home. Are you back home by now? We are still waiting for your test results. We can't push the patch upstream without it, and if it takes too long, I'll probably just discard the patch and move to other tasks. -- Jean Delvare -- 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] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
Hi Daro, On Sun, 14 Mar 2010 03:38:11 +0100, Daro wrote: Hi Jean, I am back and ready to go :) As I am not much experienced Linux user I would apprieciate some more details: I have few linux kernels installed; which one should I test or it does not matter? 2.6.31-14-generic 2.6.31-16-generic 2.6.31-17-generic 2.6.31-19-generic 2.6.31-20-generic and one I compiled myself 2.6.32.2 I assume that to proceed with a test I should patch the certain version of kernel and compile it or could it be done other way? It will be easier with the kernel you compiled yourself. First of all, download the patch from: http://patchwork.kernel.org/patch/75883/raw/ Then, move to the source directory of your 2.6.32.2 kernel and apply the patch: $ cd ~/src/linux-2.6.32 $ patch -p2 ~/download/saa7134-Fix-IR-support-of-some-ASUS-TV-FM-7135-variants.patch Adjust the path in each command to match your own setup. Then just build and install the kernel: $ make $ sudo make modules_install $ sudo make install Or whatever method you use to install your self-compiled kernels. Then reboot to kernel 2.6.32.2 and test that the remote control works even when _not_ passing any card parameter to the driver. If you ever need to remove the patch, use: $ cd ~/src/linux-2.6.32 $ patch -p2 -R ~/download/saa7134-Fix-IR-support-of-some-ASUS-TV-FM-7135-variants.patch I hope my instructions are clear enough, if you have any problem, just ask. Thanks, -- Jean Delvare -- 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] saa7134: Fix IR support of some ASUS TV-FM 7135 variants
On Sun, 14 Mar 2010 20:34:34 +0100, Daro wrote: W dniu 14.03.2010 09:26, Jean Delvare pisze: It will be easier with the kernel you compiled yourself. First of all, download the patch from: http://patchwork.kernel.org/patch/75883/raw/ Then, move to the source directory of your 2.6.32.2 kernel and apply the patch: $ cd ~/src/linux-2.6.32 $ patch -p2 ~/download/saa7134-Fix-IR-support-of-some-ASUS-TV-FM-7135-variants.patch Adjust the path in each command to match your own setup. Then just build and install the kernel: $ make $ sudo make modules_install $ sudo make install Or whatever method you use to install your self-compiled kernels. Then reboot to kernel 2.6.32.2 and test that the remote control works even when _not_ passing any card parameter to the driver. If you ever need to remove the patch, use: $ cd ~/src/linux-2.6.32 $ patch -p2 -R ~/download/saa7134-Fix-IR-support-of-some-ASUS-TV-FM-7135-variants.patch I hope my instructions are clear enough, if you have any problem, just ask. Thanks, Hi Jean! It works! dmesg output is attached. Thanks for reporting! Mauro, you can pick my (second) patch now and apply it. However I will have to go back to generic kernel as under my self-built kernels fglrx driver stops working and I did not manage to solve it :( Or maybe you could give me a hint what to do with it? I can't help you with that, sorry, I don't use binary drivers. -- Jean Delvare -- 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] FusionHDTV: Use quick reads for I2C IR device probing
On Tue, 16 Mar 2010 12:05:02 +0100, Jean Delvare wrote:
Executive summary (as I understand it): the card that no longer works
is a DViCO FusionHDTV7 Dual Express
(CX23885_BOARD_DVICO_FUSIONHDTV_7_DUAL_EXP), bridge driver cx23885. It
has 2 xc5000 chips at I2C address 0x64 (on 2 different I2C buses, of
course), and an IR chip at 0x6b (on the first of these 2 I2C buses.)
The latter is reported to be missing with recent dvb-v4l trees.
The first thing to check is whether an ir_video I2C device is created
or not. Look in /sys/bus/i2c/devices, list all the entries there. You
should see two *-0064 entries for the xc5000 chips. You should also
see, but you probably won't, one *-006b entry for the IR chip. The
following command should let us know right away what is there:
$ grep . /sys/bus/i2c/devices/*/name
The ir_video device is supposed to be probed by cx23885_i2c_register().
If it is not created, it means that the probe failed. Maybe these chips
do not like the probe mechanism used by i2c-core (quick write) and only
reply to reads? In that case, we'd need to use reads to detect it. The
i2c core doesn't give us enough control to do this cleanly, but this
could be added if the need exists. In the meantime, we can do the probe
ourselves and instantiate the device unconditionally (by using
i2c_new_device instead of i2c_new_probed_device).
We have been debugging over IRC with Timothy, and I have a fix which he
tested successfully. Basically, the problem is that the IR device on
his chip only replies to read commands, but when switching ir-kbd-i2c
to the standard device driver binding model in kernel 2.6.31, I changed
the probing method from quick read to quick write as a side effect.
This is why the IR device was no longer being detected. Using a quick
read again solves the issue. Here comes a fix, tested by Timothy for
the cx23885 part, untested for the cx88 part but I'd be very surprised
if cx88-based FusionHDTV did not need the exact same fix
* * * * *
IR support on FusionHDTV cards is broken since kernel 2.6.31. One side
effect of the switch to the standard binding model for IR I2C devices
was to let i2c-core do the probing instead of the ir-kbd-i2c driver.
There is a slight difference between the two probe methods: i2c-core
uses 0-byte writes, while the ir-kbd-i2c was using 0-byte reads. As
some IR I2C devices only support reads, the new probe method fails to
detect them.
For now, revert to letting the driver do the probe, using 0-byte
reads. In the future, i2c-core will be extended to let callers of
i2c_new_probed_device() provide a custom probing function.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Tested-by: Timothy D. Lenz tl...@vorgon.com
---
This fix applies to kernels 2.6.31 to 2.6.34. Should be sent to Linus
quickly.
drivers/media/video/cx23885/cx23885-i2c.c | 12 +++-
drivers/media/video/cx88/cx88-i2c.c | 16 +++-
2 files changed, 26 insertions(+), 2 deletions(-)
--- linux-2.6.34-rc1.orig/drivers/media/video/cx23885/cx23885-i2c.c
2010-02-25 09:10:33.0 +0100
+++ linux-2.6.34-rc1/drivers/media/video/cx23885/cx23885-i2c.c 2010-03-18
13:33:05.0 +0100
@@ -365,7 +365,17 @@ int cx23885_i2c_register(struct cx23885_
memset(info, 0, sizeof(struct i2c_board_info));
strlcpy(info.type, ir_video, I2C_NAME_SIZE);
- i2c_new_probed_device(bus-i2c_adap, info, addr_list);
+ /*
+* We can't call i2c_new_probed_device() because it uses
+* quick writes for probing and the IR receiver device only
+* replies to reads.
+*/
+ if (i2c_smbus_xfer(bus-i2c_adap, addr_list[0], 0,
+ I2C_SMBUS_READ, 0, I2C_SMBUS_QUICK,
+ NULL) = 0) {
+ info.addr = addr_list[0];
+ i2c_new_device(bus-i2c_adap, info);
+ }
}
return bus-i2c_rc;
--- linux-2.6.34-rc1.orig/drivers/media/video/cx88/cx88-i2c.c 2010-02-25
09:08:40.0 +0100
+++ linux-2.6.34-rc1/drivers/media/video/cx88/cx88-i2c.c2010-03-18
13:33:05.0 +0100
@@ -188,10 +188,24 @@ int cx88_i2c_init(struct cx88_core *core
0x18, 0x6b, 0x71,
I2C_CLIENT_END
};
+ const unsigned short *addrp;
memset(info, 0, sizeof(struct i2c_board_info));
strlcpy(info.type, ir_video, I2C_NAME_SIZE);
- i2c_new_probed_device(core-i2c_adap, info, addr_list);
+ /*
+* We can't call i2c_new_probed_device() because it uses
+* quick writes for probing and at least some R receiver
+* devices only reply to reads.
+*/
+ for (addrp = addr_list; *addrp != I2C_CLIENT_END; addrp++) {
+ if (i2c_smbus_xfer(core
Re: [PATCH 12/24] media/video: fix dangling pointers
Hi Hans, On Sat, 20 Mar 2010 23:02:49 +0100, Hans Verkuil wrote: On Saturday 20 March 2010 15:12:53 Wolfram Sang wrote: Fix I2C-drivers which missed setting clientdata to NULL before freeing the structure it points to. Also fix drivers which do this _after_ the structure was freed already. I feel I am missing something here. Why does clientdata have to be set to NULL when we are tearing down the device anyway? We're not tearing down the device, that's the point. We are only unbinding it from its driver. The device itself survives the operation. (This is different from the legacy i2c binding model where the device itself would indeed be removed at that point, and that would be the reason why so many i2c drivers have it wrong now.) And if there is a good reason for doing this, then it should be done in v4l2_device_unregister_subdev or even in the i2c core, not in each drivers. Mark Brown (Cc'd) suggested this already, but I have mixed feelings about this. My reasoning is: 1* It is good practice to have memory freed not too far from where it was allocated, otherwise there is always a risk of unmatched pairs. In this regard, it seems preferable to let each i2c driver kfree the device memory it kalloc'd, be it in probe() or remove(). 2* References to allocated memory should be dropped before that memory is freed. This means that we want to call i2c_set_clientdata(c, NULL) before kfree(d). As a corollary, we can't do the former in i2c-core and the later in device drivers. So we are in the difficult situation where we can't do both in i2c-core because that violates point 1 above, we can't do half in i2c-core and half in device drivers because this violates point 2 above, so we fall back to doing both in device drivers, which doesn't violate any point but duplicates the code all around. Now, if we decide to handle this at a deeper driver model layer (i2c-core instead of device drivers) then I'm not sure why we would stop there... Wouldn't it be the driver core's job to clear the reference and free the allocated memory? I get the feeling that this would be a job for managed resources as some drivers already do for I/O ports and IRQs. Managed resources don't care about symmetry of allocation and freeing, by design (so it can violate point 1 above.) Aha! Isn't it exactly what devm_kzalloc() is all about? At this point, I guess that each subsystem maintainer can decide to either apply Wolfram's patch, or switch the drivers to managed resources. Very nice that we don't have to make this a subsystem-wide decision, so every actor can do the changes if/when they see fit. And why does coccinelle apparently find this in e.g. cs5345.c but not in saa7115.c, which has exactly the same construct? For that matter, I think almost all v4l i2c drivers do this. That I can't say, sorry. -- Jean Delvare -- 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 12/24] media/video: fix dangling pointers
Replying to myself... On Sun, 21 Mar 2010 14:46:55 +0100, Jean Delvare wrote: I get the feeling that this would be a job for managed resources as some drivers already do for I/O ports and IRQs. Managed resources don't care about symmetry of allocation and freeing, by design (so it can violate point 1 above.) Aha! Isn't it exactly what devm_kzalloc() is all about? Thinking about it again, this really only addresses the calls to kfree(), not the calls to i2c_set_clientdata(), so apparently I'm quite off-topic for this discussion. I still think that moving drivers to managed resources is the way to go, but that's a different issue. -- Jean Delvare -- 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: i2c interface bugs in dvb drivers
Hi Matthieu, On Sun, 21 Mar 2010 15:02:50 +0100, matthieu castet wrote: some dvb driver that export a i2c interface contains some mistakes because they mainly used by driver (frontend, tuner) wrote for them. But the i2c interface is exposed to everybody. One mistake is expect msg[i].addr with 8 bits address instead of 7 bits address. This make them use eeprom address at 0xa0 instead of 0x50. Also they shift tuner address (qt1010 tuner is likely to be at address 0x62, but some put it a 0xc4 (af9015, af9005, dtv5100)). Other mistakes is in xfer callback. Often the controller support a limited i2c support (n bytes write then m bytes read). The driver try to convert the linux i2c msg to this pattern, but they often miss cases : - msg[i].len can be null Zero, not null. This is a very rare case, I've never seen it in multi-message transactions (wouldn't make much sense IMHO), only in the single-message transaction known as SMBus quick command. Many controllers don't support it, so I2C bus drivers don't have to support it. It should be assumed by I2C chip drivers that an I2C adapter without functionality bit I2C_FUNC_SMBUS_QUICK does not support 0-byte messages. - msg write are not always followed by msg read And this can be dangerous if these interfaces are exported to userspace via i2c-dev : Even without that... We certainly hope to reuse client drivers for other families of DVB cards, and for this to work, every driver must stick to the standard. - some scanning program avoid eeprom by filtering 0x5x range, but now it is at 0xax range (well that should happen because scan limit should be 0x77) - some read only command can be interpreted as write command. Very bad indeed. What should be done ? Fix the drivers. Yes, definitely. The sooner, the better. Have a mode where i2c interface are not exported to everybody. I have considered this for a moment. It might be fair to let I2C bus drivers decide whether they want i2c-dev to expose their buses or not. We could use a new class bit (I2C_CLASS_USER or such) to this purpose. I didn't get to it yet though, as this doesn't seem to be urgent, and i2c-dev has so many other problems... That being said, this is hardly a valid answer to the problem you discovered. Preventing user-space from triggering bugs is not fixing them. Don't care. No, that's not an option. First why does the i2c stack doesn't check that the address is on 7 bits (like the attached patch) ? Performance reasons, I presume. Having to check this each time a transaction is attempted would be quite costly. Secondly, I suspect it was never thought that raw I2C messaging would become so popular. The original intent was to have all I2C device drivers (except maybe i2c-dev) register their clients. The legacy method for this (i2c_detect) _does_ have an address check included, and so do i2c_new_probed_device and i2c_sysfs_new_device. Probably we should add it to i2c_new_device as well, for consistency. It is way less expensive to check the address once and for all, than with each transaction. I certainly hope that DVB will move to client-based I2C device drivers at some point in time. Note though that the address check is in no way bullet-proof. If addresses in the range 0x03-0x3b are handled as 8-bit entities instead of 7-bit entities, they will appear to be in the range 0x06-0x76, which is perfectly valid. Also I believe a program for testing i2c interface corner case should catch most of these bugs : - null msg[i].len - different transactions on a device : - one write/read transaction - one write transaction then one read transaction [...] Does a such program exist ? We have several programs in the i2c-tools package, which can be used to this purpose: * i2cdetect * i2cdump * i2cget * i2cset With these 4 tools, almost all SMBus transaction types are covered. This is sufficient in most cases in my experience. If not, these tools can certainly get extended, at least to cover all of SMBus. -- Jean Delvare -- 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] FusionHDTV: Use quick reads for I2C IR device probing
Can the fix below please be picked quickly? This is a regression, the
fix should go upstream ASAP. Thanks.
On Fri, 19 Mar 2010 14:42:50 +0100, Jean Delvare wrote:
IR support on FusionHDTV cards is broken since kernel 2.6.31. One side
effect of the switch to the standard binding model for IR I2C devices
was to let i2c-core do the probing instead of the ir-kbd-i2c driver.
There is a slight difference between the two probe methods: i2c-core
uses 0-byte writes, while the ir-kbd-i2c was using 0-byte reads. As
some IR I2C devices only support reads, the new probe method fails to
detect them.
For now, revert to letting the driver do the probe, using 0-byte
reads. In the future, i2c-core will be extended to let callers of
i2c_new_probed_device() provide a custom probing function.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Tested-by: Timothy D. Lenz tl...@vorgon.com
---
This fix applies to kernels 2.6.31 to 2.6.34. Should be sent to Linus
quickly.
drivers/media/video/cx23885/cx23885-i2c.c | 12 +++-
drivers/media/video/cx88/cx88-i2c.c | 16 +++-
2 files changed, 26 insertions(+), 2 deletions(-)
--- linux-2.6.34-rc1.orig/drivers/media/video/cx23885/cx23885-i2c.c
2010-02-25 09:10:33.0 +0100
+++ linux-2.6.34-rc1/drivers/media/video/cx23885/cx23885-i2c.c
2010-03-18 13:33:05.0 +0100
@@ -365,7 +365,17 @@ int cx23885_i2c_register(struct cx23885_
memset(info, 0, sizeof(struct i2c_board_info));
strlcpy(info.type, ir_video, I2C_NAME_SIZE);
- i2c_new_probed_device(bus-i2c_adap, info, addr_list);
+ /*
+ * We can't call i2c_new_probed_device() because it uses
+ * quick writes for probing and the IR receiver device only
+ * replies to reads.
+ */
+ if (i2c_smbus_xfer(bus-i2c_adap, addr_list[0], 0,
+I2C_SMBUS_READ, 0, I2C_SMBUS_QUICK,
+NULL) = 0) {
+ info.addr = addr_list[0];
+ i2c_new_device(bus-i2c_adap, info);
+ }
}
return bus-i2c_rc;
--- linux-2.6.34-rc1.orig/drivers/media/video/cx88/cx88-i2c.c 2010-02-25
09:08:40.0 +0100
+++ linux-2.6.34-rc1/drivers/media/video/cx88/cx88-i2c.c 2010-03-18
13:33:05.0 +0100
@@ -188,10 +188,24 @@ int cx88_i2c_init(struct cx88_core *core
0x18, 0x6b, 0x71,
I2C_CLIENT_END
};
+ const unsigned short *addrp;
memset(info, 0, sizeof(struct i2c_board_info));
strlcpy(info.type, ir_video, I2C_NAME_SIZE);
- i2c_new_probed_device(core-i2c_adap, info, addr_list);
+ /*
+ * We can't call i2c_new_probed_device() because it uses
+ * quick writes for probing and at least some R receiver
+ * devices only reply to reads.
+ */
+ for (addrp = addr_list; *addrp != I2C_CLIENT_END; addrp++) {
+ if (i2c_smbus_xfer(core-i2c_adap, *addrp, 0,
+I2C_SMBUS_READ, 0,
+I2C_SMBUS_QUICK, NULL) = 0) {
+ info.addr = *addrp;
+ i2c_new_device(core-i2c_adap, info);
+ break;
+ }
+ }
}
return core-i2c_rc;
}
--
Jean Delvare
--
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] cx88: High resolution timer for Remote Controls
From: Andrzej Hajda andrzej.ha...@wp.pl
Patch solves problem of missed keystrokes on some remote controls,
as reported on http://bugzilla.kernel.org/show_bug.cgi?id=9637 .
Signed-off-by: Andrzej Hajda andrzej.ha...@wp.pl
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
Resending because last attempt resulted in folded lines:
http://www.spinics.net/lists/linux-media/msg06884.html
Patch was already resent by Andrzej on June 4th but apparently it was
overlooked.
Trent Piepho commented on the compatibility with kernels older than
2.6.20 being possibly broken:
http://www.spinics.net/lists/linux-media/msg06885.html
I don't think this is the case. The kernel version test was there
because the workqueue API changed in 2.6.20, but the hrtimer API did
not have such a change. This is why the version check has gone.
It is highly probable that the hrtimer API had its own incompatible
changes since it was introduced in kernel 2.6.16. By looking at the
code, I found the following ones:
* hrtimer_forward_now() was added with kernel 2.6.25 only:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=5e05ad7d4e3b11f935998882b5d9c3b257137f1b
But this is an inline function, so I presume this shouldn't be too
difficult to add to a compatibility header.
* Before 2.6.21, HRTIMER_MODE_REL was named HRTIMER_REL:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=c9cb2e3d7c9178ab75d0942f96abb3abe0369906
This too should be solvable in a compatibility header.
The rest doesn't seem to cause compatibility issues, but only actual
testing would confirm that.
This bug affects me, which is why I am motivated to get this fix
upstream. Please let me know how I can help.
linux/drivers/media/video/cx88/cx88-input.c | 37 ---
1 file changed, 17 insertions(+), 20 deletions(-)
--- v4l-dvb.orig/linux/drivers/media/video/cx88/cx88-input.c2009-07-02
15:13:08.0 +0200
+++ v4l-dvb/linux/drivers/media/video/cx88/cx88-input.c 2009-07-02
15:35:04.0 +0200
@@ -23,10 +23,10 @@
*/
#include linux/init.h
-#include linux/delay.h
#include linux/input.h
#include linux/pci.h
#include linux/module.h
+#include linux/hrtimer.h
#include compat.h
#include cx88.h
@@ -49,7 +49,7 @@ struct cx88_IR {
/* poll external decoder */
int polling;
- struct delayed_work work;
+ struct hrtimer timer;
u32 gpio_addr;
u32 last_gpio;
u32 mask_keycode;
@@ -145,31 +145,28 @@ static void cx88_ir_handle_key(struct cx
}
}
-#if LINUX_VERSION_CODE KERNEL_VERSION(2,6,20)
-static void cx88_ir_work(void *data)
-#else
-static void cx88_ir_work(struct work_struct *work)
-#endif
+enum hrtimer_restart cx88_ir_work(struct hrtimer *timer)
{
-#if LINUX_VERSION_CODE KERNEL_VERSION(2,6,20)
- struct cx88_IR *ir = data;
-#else
- struct cx88_IR *ir = container_of(work, struct cx88_IR, work.work);
-#endif
+ unsigned long missed;
+ struct cx88_IR *ir = container_of(timer, struct cx88_IR, timer);
cx88_ir_handle_key(ir);
- schedule_delayed_work(ir-work, msecs_to_jiffies(ir-polling));
+ missed = hrtimer_forward_now(ir-timer,
+ktime_set(0, ir-polling * 100));
+ if (missed 1)
+ ir_dprintk(Missed ticks %ld\n, missed - 1);
+
+ return HRTIMER_RESTART;
}
void cx88_ir_start(struct cx88_core *core, struct cx88_IR *ir)
{
if (ir-polling) {
-#if LINUX_VERSION_CODE KERNEL_VERSION(2,6,20)
- INIT_DELAYED_WORK(ir-work, cx88_ir_work, ir);
-#else
- INIT_DELAYED_WORK(ir-work, cx88_ir_work);
-#endif
- schedule_delayed_work(ir-work, 0);
+ hrtimer_init(ir-timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ ir-timer.function = cx88_ir_work;
+ hrtimer_start(ir-timer,
+ ktime_set(0, ir-polling * 100),
+ HRTIMER_MODE_REL);
}
if (ir-sampling) {
core-pci_irqmask |= PCI_INT_IR_SMPINT;
@@ -186,7 +183,7 @@ void cx88_ir_stop(struct cx88_core *core
}
if (ir-polling)
- cancel_delayed_work_sync(ir-work);
+ hrtimer_cancel(ir-timer);
}
/* -- */
--
Jean Delvare
--
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] cx88: High resolution timer for Remote Controls
On Thu, 2 Jul 2009 16:50:35 +0200, Jean Delvare wrote: From: Andrzej Hajda andrzej.ha...@wp.pl Patch solves problem of missed keystrokes on some remote controls, as reported on http://bugzilla.kernel.org/show_bug.cgi?id=9637 . Signed-off-by: Andrzej Hajda andrzej.ha...@wp.pl Signed-off-by: Jean Delvare kh...@linux-fr.org --- Resending because last attempt resulted in folded lines: http://www.spinics.net/lists/linux-media/msg06884.html Patch was already resent by Andrzej on June 4th but apparently it was overlooked. Trent Piepho commented on the compatibility with kernels older than 2.6.20 being possibly broken: http://www.spinics.net/lists/linux-media/msg06885.html I don't think this is the case. The kernel version test was there because the workqueue API changed in 2.6.20, but the hrtimer API did not have such a change. This is why the version check has gone. It is highly probable that the hrtimer API had its own incompatible changes since it was introduced in kernel 2.6.16. By looking at the code, I found the following ones: * hrtimer_forward_now() was added with kernel 2.6.25 only: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=5e05ad7d4e3b11f935998882b5d9c3b257137f1b But this is an inline function, so I presume this shouldn't be too difficult to add to a compatibility header. * Before 2.6.21, HRTIMER_MODE_REL was named HRTIMER_REL: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=c9cb2e3d7c9178ab75d0942f96abb3abe0369906 This too should be solvable in a compatibility header. The rest doesn't seem to cause compatibility issues, but only actual testing would confirm that. Actually there were more compatibility issues, the most important one being that not all functions of the hrtimer API are exported before 2.6.22. So unfortunately this bug fix means that the cx88 driver will no longer build on kernels 2.6.22. I'll post a new patch with this change, and another one for the hrtimer compatibility code. -- Jean Delvare -- 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 2/2] cx88: High resolution timer for Remote Controls
Patch solves problem of missed keystrokes on some remote controls,
as reported on http://bugzilla.kernel.org/show_bug.cgi?id=9637 .
Signed-off-by: Andrzej Hajda andrzej.ha...@wp.pl
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
Changes:
* Driver no longer builds on kernels 2.6.22, so add an entry to
v4l/versions.txt
* Add a missing static.
Build-tested on 2.6.22.
linux/drivers/media/video/cx88/cx88-input.c | 37
v4l/versions.txt|2 +
2 files changed, 19 insertions(+), 20 deletions(-)
--- a/linux/drivers/media/video/cx88/cx88-input.c
+++ b/linux/drivers/media/video/cx88/cx88-input.c
@@ -23,7 +23,7 @@
*/
#include linux/init.h
-#include linux/delay.h
+#include linux/hrtimer.h
#include linux/input.h
#include linux/pci.h
#include linux/module.h
@@ -49,7 +49,7 @@ struct cx88_IR {
/* poll external decoder */
int polling;
- struct delayed_work work;
+ struct hrtimer timer;
u32 gpio_addr;
u32 last_gpio;
u32 mask_keycode;
@@ -145,31 +145,28 @@ static void cx88_ir_handle_key(struct cx
}
}
-#if LINUX_VERSION_CODE KERNEL_VERSION(2,6,20)
-static void cx88_ir_work(void *data)
-#else
-static void cx88_ir_work(struct work_struct *work)
-#endif
+static enum hrtimer_restart cx88_ir_work(struct hrtimer *timer)
{
-#if LINUX_VERSION_CODE KERNEL_VERSION(2,6,20)
- struct cx88_IR *ir = data;
-#else
- struct cx88_IR *ir = container_of(work, struct cx88_IR, work.work);
-#endif
+ unsigned long missed;
+ struct cx88_IR *ir = container_of(timer, struct cx88_IR, timer);
cx88_ir_handle_key(ir);
- schedule_delayed_work(ir-work, msecs_to_jiffies(ir-polling));
+ missed = hrtimer_forward_now(ir-timer,
+ktime_set(0, ir-polling * 100));
+ if (missed 1)
+ ir_dprintk(Missed ticks %ld\n, missed - 1);
+
+ return HRTIMER_RESTART;
}
void cx88_ir_start(struct cx88_core *core, struct cx88_IR *ir)
{
if (ir-polling) {
-#if LINUX_VERSION_CODE KERNEL_VERSION(2,6,20)
- INIT_DELAYED_WORK(ir-work, cx88_ir_work, ir);
-#else
- INIT_DELAYED_WORK(ir-work, cx88_ir_work);
-#endif
- schedule_delayed_work(ir-work, 0);
+ hrtimer_init(ir-timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ ir-timer.function = cx88_ir_work;
+ hrtimer_start(ir-timer,
+ ktime_set(0, ir-polling * 100),
+ HRTIMER_MODE_REL);
}
if (ir-sampling) {
core-pci_irqmask |= PCI_INT_IR_SMPINT;
@@ -186,7 +183,7 @@ void cx88_ir_stop(struct cx88_core *core
}
if (ir-polling)
- cancel_delayed_work_sync(ir-work);
+ hrtimer_cancel(ir-timer);
}
/* -- */
--- a/v4l/versions.txt
+++ b/v4l/versions.txt
@@ -34,6 +34,8 @@ DVB_DRX397XD
DVB_DM1105
# This driver needs print_hex_dump
DVB_FIREDTV
+# This driver needs hrtimer API
+VIDEO_CX88
[2.6.20]
#This driver requires HID_REQ_GET_REPORT
--
Jean Delvare
--
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/2] Compatibility layer for hrtimer API
Hi Trent,
On Sun, 5 Jul 2009 01:13:14 -0700 (PDT), Trent Piepho wrote:
On Fri, 3 Jul 2009, Jean Delvare wrote:
Kernels 2.6.22 to 2.6.24 (inclusive) need some compatibility quirks
for the hrtimer API. For older kernels, some required functions were
not exported so there's nothing we can do. This means that drivers
using the hrtimer infrastructure will no longer work for kernels older
than 2.6.22.
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
v4l/compat.h | 18 ++
1 file changed, 18 insertions(+)
--- a/v4l/compat.h
+++ b/v4l/compat.h
@@ -480,4 +480,22 @@ static inline unsigned long v4l_compat_f
}
#endif
+/*
+ * Compatibility code for hrtimer API
+ * This will make hrtimer usable for kernels 2.6.22 and later.
+ * For earlier kernels, not all required functions are exported
+ * so there's nothing we can do.
+ */
+
+#if LINUX_VERSION_CODE KERNEL_VERSION(2, 6, 25) \
+ LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 22)
+#include linux/hrtimer.h
Instead of including hrtimer.h from compat.h it's better if you check if it
has already been included and only enable the compat code in that case.
That way hrtimer doesn't get included for files that don't need it and
might define something that conflicts with something from hrtimer. And it
prevents someone from forgetting to include hrtimer when they needed it,
but having the error masked because compat.h is doing it for them.
I see. But this will only work if compat.h is included after all
headers. If it always the case? I see for example that cx88-input
includes media/ir-common.h after compat.h.
+/* Forward a hrtimer so it expires after the hrtimer's current now */
+static inline unsigned long hrtimer_forward_now(struct hrtimer *timer,
+ ktime_t interval)
+{
+ return hrtimer_forward(timer, timer-base-get_time(), interval);
+}
+#endif
+
#endif /* _COMPAT_H */
--
Jean Delvare
--
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 Thu, 9 Jul 2009 11:44:46 -0400, Jarod Wilson wrote: On Tuesday 07 April 2009 08:36:17 Jean Delvare wrote: So, let's just forget the workarounds and go straight to the point: focus on merging lirc-i2c drivers. Will this happen next week? I fear not. Which is why I can't wait for it. And anyway, in order to merge the lirc_i2c driver, it must be turned into a new-style I2C driver first, so bridge drivers must be prepared for this, which is exactly what my patches are doing. For what its worth, I fixed up lirc_i2c a few days ago, and now have it working just fine with my pvr-250 under 2.6.31-rc2. Excellent. Apparently you did not hit any problem, but if you ever do need help for the i2c side of things, just ask and I'll be happy to help. Real Soon Now (I swear), I'm hoping to get up another head of steam for submitting lirc upstream. Multiple drivers have received a bunch of love in the past few weeks, so I think we're in a pretty good state to have another go at it... -- Jean Delvare -- 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/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type
Hi Mark, On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote: While you folks are looking into ir-kbd-i2c, perhaps one of you will fix the regressions introduced in 2.6.31-* ? The drive no longer detects/works with the I/R port on the Hauppauge PVR-250 cards, which is a user-visible regression. This is bad. If there a bugzilla entry? If not, where can I read more details / get in touch with an affected user? -- Jean Delvare -- 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/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type
Hi Andy,
On Fri, 17 Jul 2009 16:35:37 -0400, Andy Walls wrote:
This patch augments the init data passed by bridge drivers to ir-kbd-i2c
so that the ir_type can be set explicitly and so ir-kbd-i2c internal
get_key functions can be reused without requiring symbols from
ir-kbd-i2c in the bridge driver.
Regards,
Andy
Looks good. Minor suggestion below:
diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c
--- a/linux/drivers/media/video/ir-kbd-i2c.c Wed Jul 15 07:28:02 2009 -0300
+++ b/linux/drivers/media/video/ir-kbd-i2c.c Fri Jul 17 16:05:28 2009 -0400
@@ -478,7 +480,34 @@
ir_codes = init_data-ir_codes;
name = init_data-name;
+ if (init_data-type)
+ ir_type = init_data-type;
ir-get_key = init_data-get_key;
+ switch (init_data-internal_get_key_func) {
+ case IR_KBD_GET_KEY_PIXELVIEW:
+ ir-get_key = get_key_pixelview;
+ break;
+ case IR_KBD_GET_KEY_PV951:
+ ir-get_key = get_key_pv951;
+ break;
+ case IR_KBD_GET_KEY_HAUP:
+ ir-get_key = get_key_haup;
+ break;
+ case IR_KBD_GET_KEY_KNC1:
+ ir-get_key = get_key_knc1;
+ break;
+ case IR_KBD_GET_KEY_FUSIONHDTV:
+ ir-get_key = get_key_fusionhdtv;
+ break;
+ case IR_KBD_GET_KEY_HAUP_XVR:
+ ir-get_key = get_key_haup_xvr;
+ break;
+ case IR_KBD_GET_KEY_AVERMEDIA_CARDBUS:
+ ir-get_key = get_key_avermedia_cardbus;
+ break;
+ default:
+ break;
+ }
}
/* Make sure we are all setup before going on */
diff -r d754a2d5a376 linux/include/media/ir-kbd-i2c.h
--- a/linux/include/media/ir-kbd-i2c.hWed Jul 15 07:28:02 2009 -0300
+++ b/linux/include/media/ir-kbd-i2c.hFri Jul 17 16:05:28 2009 -0400
@@ -24,10 +24,27 @@
int(*get_key)(struct IR_i2c*, u32*, u32*);
};
+enum ir_kbd_get_key_fn {
+ IR_KBD_GET_KEY_NONE = 0,
As you never use IR_KBD_GET_KEY_NONE, you might as well not define it
and start with IR_KBD_GET_KEY_PIXELVIEW = 1. This would have the added
advantage that you could get rid of the default statement in the
above switch, letting gcc warn you (or any other developer) if you ever
add a new enum value and forget to handle it in ir_probe().
+ IR_KBD_GET_KEY_PIXELVIEW,
+ IR_KBD_GET_KEY_PV951,
+ IR_KBD_GET_KEY_HAUP,
+ IR_KBD_GET_KEY_KNC1,
+ IR_KBD_GET_KEY_FUSIONHDTV,
+ IR_KBD_GET_KEY_HAUP_XVR,
+ IR_KBD_GET_KEY_AVERMEDIA_CARDBUS,
+};
+
/* Can be passed when instantiating an ir_video i2c device */
struct IR_i2c_init_data {
IR_KEYTAB_TYPE *ir_codes;
const char *name;
+ inttype; /* IR_TYPE_RC5, IR_TYPE_PD, etc */
+ /*
+ * Specify either a function pointer or a value indicating one of
+ * ir_kbd_i2c's internal get_key functions
+ */
int(*get_key)(struct IR_i2c*, u32*, u32*);
+ enum ir_kbd_get_key_fn internal_get_key_func;
};
#endif
--
Jean Delvare
--
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/3] 2/3: cx18: Add i2c initialization for Z8F0811/Hauppage IR transceivers
the CX18_HW_ defines */
static const u8 hw_addrs[] = {
- 0, /* CX18_HW_TUNER */
- 0, /* CX18_HW_TVEEPROM */
- CX18_CS5345_I2C_ADDR, /* CX18_HW_CS5345 */
- 0, /* CX18_HW_DVB */
- 0, /* CX18_HW_418_AV */
- 0, /* CX18_HW_GPIO_MUX */
- 0, /* CX18_HW_GPIO_RESET_CTRL */
+ 0, /* CX18_HW_TUNER */
+ 0, /* CX18_HW_TVEEPROM */
+ CX18_CS5345_I2C_ADDR, /* CX18_HW_CS5345 */
+ 0, /* CX18_HW_DVB */
+ 0, /* CX18_HW_418_AV */
+ 0, /* CX18_HW_GPIO_MUX */
+ 0, /* CX18_HW_GPIO_RESET_CTRL */
+ CX18_Z8F0811_IR_TX_I2C_ADDR,/* CX18_HW_Z8F0811_IR_TX_HAUP */
+ CX18_Z8F0811_IR_RX_I2C_ADDR,/* CX18_HW_Z8F0811_IR_RX_HAUP */
};
/* This array should match the CX18_HW_ defines */
@@ -62,6 +66,8 @@
0, /* CX18_HW_418_AV */
0, /* CX18_HW_GPIO_MUX */
0, /* CX18_HW_GPIO_RESET_CTRL */
+ 0, /* CX18_HW_Z8F0811_IR_TX_HAUP */
+ 0, /* CX18_HW_Z8F0811_IR_RX_HAUP */
};
/* This array should match the CX18_HW_ defines */
@@ -73,6 +79,8 @@
NULL, /* CX18_HW_418_AV */
NULL, /* CX18_HW_GPIO_MUX */
NULL, /* CX18_HW_GPIO_RESET_CTRL */
+ NULL, /* CX18_HW_Z8F0811_IR_TX_HAUP */
+ NULL, /* CX18_HW_Z8F0811_IR_RX_HAUP */
};
/* This array should match the CX18_HW_ defines */
@@ -84,8 +92,43 @@
cx23418_AV,
gpio_mux,
gpio_reset_ctrl,
+ ir_tx_z8f0811_haup,
+ ir_rx_z8f0811_haup,
};
+static int cx18_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char
*type,
+u8 addr)
+{
+#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
+ struct i2c_board_info info;
+ struct IR_i2c_init_data ir_init_data;
+ unsigned short addr_list[2] = { addr, I2C_CLIENT_END };
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, type, I2C_NAME_SIZE);
+
+ /* Our default information for ir-kbd-i2c.c to use */
+ memset(ir_init_data, 0, sizeof(struct IR_i2c_init_data));
+ switch (hw) {
+ case CX18_HW_Z8F0811_IR_RX_HAUP:
+ ir_init_data.ir_codes = ir_codes_hauppauge_new;
+ ir_init_data.get_key = NULL;
This is a no-op, as ir_init_data was cleared with memset() right above.
+ ir_init_data.internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
+ ir_init_data.type = IR_TYPE_RC5;
+ ir_init_data.name = CX23418 Z8F0811 Hauppauge;
+ break;
+ default:
+ break;
+ }
+ if (ir_init_data.name)
+ info.platform_data = ir_init_data;
Not sure why you don't just put this in the switch to save a test? Even
the memset could go there as far as I can see.
+
+ return i2c_new_probed_device(adap, info, addr_list) == NULL ? -1 : 0;
+#else
+ return -1;
+#endif
+}
+
int cx18_i2c_register(struct cx18 *cx, unsigned idx)
{
struct v4l2_subdev *sd;
@@ -119,7 +162,10 @@
if (!hw_addrs[idx])
return -1;
- /* It's an I2C device other than an analog tuner */
+ if (hw CX18_HW_Z8F0811_IR_HAUP)
+ return cx18_i2c_new_ir(adap, hw, type, hw_addrs[idx]);
For consistency I'd move this up a bit (although I agree it is
functionally equivalent.)
+
+ /* It's an I2C device other than an analog tuner or IR chip */
sd = v4l2_i2c_new_subdev(cx-v4l2_dev, adap, mod, type, hw_addrs[idx]);
if (sd != NULL)
sd-grp_id = hw;
The rest looks OK.
--
Jean Delvare
--
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/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type
On Sun, 19 Jul 2009 09:17:30 -0400, Mark Lord wrote: Mark Lord wrote: Jean Delvare wrote: Hi Mark, On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote: While you folks are looking into ir-kbd-i2c, perhaps one of you will fix the regressions introduced in 2.6.31-* ? The drive no longer detects/works with the I/R port on the Hauppauge PVR-250 cards, which is a user-visible regression. This is bad. If there a bugzilla entry? If not, where can I read more details / get in touch with an affected user? .. I imagine there will be thousands of affected users once the kernel is released, but for now I'll volunteer as a guinea-pig. It is difficult to test with 2.6.31 on the system at present, though, because that kernel also breaks other things that the MythTV box relies on, and the system is in regular use as our only PVR. Right now, all I know is, that the PVR-250 IR port did not show up in /dev/input/ with 2.6.31 after loading ir_kbd_i2c. But it does show up there with all previous kernels going back to the 2.6.1x days. .. Actually, I meant to say that it does not show up in the output from the lsinput command, whereas it did show up there in all previous kernels. Never heard of lsinput, where does it come from? So, to keep the pain level reasonable, perhaps you could send some debugging patches, and I'll apply those, reconfigure the machine for 2.6.31 again, and collect some output for you. And also perhaps try a few things locally as well to speed up the process. Okay? I'd need additional information first, otherwise I have no clue where to start debugging. Which you just sent in a further post, as I see, so I'll follow-up there... -- Jean Delvare -- 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/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type
On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote: I'm debugging various other b0rked things in 2.6.31 here right now, so I had a closer look at the Hauppauge I/R remote issue. The ir_kbd_i2c driver *does* still find it after all. But the difference is that the output from 'lsinput' has changed and no longer says Hauppauge. Which prevents the application from finding the remote control in the same way as before. OK, thanks for the investigation. I'll hack the application code here now to use the new output, but I wonder what the the thousands of other users will do when they first try 2.6.31 after release ? Where does lsinput get the string from? What exactly was it before, and what is it exactly in 2.6.31? -- Jean Delvare -- 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: Add support for Z8F0811/Hauppage IR transceivers
Hi Andy,
On Fri, 17 Jul 2009 16:49:55 -0400, Andy Walls wrote:
This patch adds support for Zilog Z8F0811 IR transceiver chips on
CX2341[68] based boards to ir-kbd-i2c for both the old i2c binding model
and the new i2c binding model.
Regards,
Andy
diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c
--- a/linux/drivers/media/video/ir-kbd-i2c.c Wed Jul 15 07:28:02 2009 -0300
+++ b/linux/drivers/media/video/ir-kbd-i2c.c Fri Jul 17 16:05:28 2009 -0400
@@ -442,9 +442,11 @@
case 0x47:
case 0x71:
case 0x2d:
- if (adap-id == I2C_HW_B_CX2388x) {
+ if (adap-id == I2C_HW_B_CX2388x ||
+ adap-id == I2C_HW_B_CX2341X) {
/* Handled by cx88-input */
- name= CX2388x remote;
+ name = adap-id == I2C_HW_B_CX2341X ? CX2341x remote
+ : CX2388x remote;
ir_type = IR_TYPE_RC5;
ir-get_key = get_key_haup_xvr;
if (hauppauge == 1) {
@@ -697,7 +726,8 @@
static const struct i2c_device_id ir_kbd_id[] = {
/* Generic entry for any IR receiver */
{ ir_video, 0 },
- /* IR device specific entries could be added here */
+ /* IR device specific entries should be added here */
+ { ir_rx_z8f0811_haup, 0 },
{ }
};
Yes, looks good.
--
Jean Delvare
--
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: Regression 2.6.31: ioctl(EVIOCGNAME) no longer returns device name
On Sun, 19 Jul 2009 15:20:50 -0400, Mark Lord wrote: Mark Lord wrote: (resending.. somebody trimmed linux-kernel from the CC: earlier) FWIW I don't think it was there in the first place. Jean Delvare wrote: On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote: I'm debugging various other b0rked things in 2.6.31 here right now, so I had a closer look at the Hauppauge I/R remote issue. The ir_kbd_i2c driver *does* still find it after all. But the difference is that the output from 'lsinput' has changed and no longer says Hauppauge. Which prevents the application from finding the remote control in the same way as before. OK, thanks for the investigation. I'll hack the application code here now to use the new output, but I wonder what the the thousands of other users will do when they first try 2.6.31 after release ? .. Mmm.. appears to be a systemwide thing, not just for the i2c stuff. *All* of the input devices now no longer show their real names when queried with ioctl(EVIOCGNAME). This is a regression from 2.6.30. Note that the real names *are* still stored somewhere, because they do still show up correctly under /sys/ I was just coming to the same conclusion. So this doesn't have anything to do with the ir-kbd-i2c conversion after all... This is something for the input subsystem maintainers. I suspect this commit is related to the regression: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=3d5cb60ef3042ac479dab82e5a945966a0d54d53 -- Jean Delvare -- 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: ir-kbd-i2c: Drop irrelevant inline keywords
On Mon, 20 Jul 2009 20:09:44 -0400, Andy Walls wrote: On Sun, 2009-07-19 at 14:59 +0200, Jean Delvare wrote: Functions which are referenced by their address can't be inlined by definition. Signed-off-by: Jean Delvare kh...@linux-fr.org Jean, Looks godd to me, but you forgot to add [PATCH] to the subject. I'll add this one to my revised patch set I submit to the list, unless you object. Oops, you're right. Yes, please pick it up and push it forward, thanks! -- Jean Delvare -- 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/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type
Hi Andy,
On Mon, 20 Jul 2009 20:07:50 -0400, Andy Walls wrote:
On Sun, 2009-07-19 at 14:47 +0200, Jean Delvare wrote:
Hi Andy,
On Fri, 17 Jul 2009 16:35:37 -0400, Andy Walls wrote:
This patch augments the init data passed by bridge drivers to ir-kbd-i2c
so that the ir_type can be set explicitly and so ir-kbd-i2c internal
get_key functions can be reused without requiring symbols from
ir-kbd-i2c in the bridge driver.
Regards,
Andy
Looks good. Minor suggestion below:
Jean,
Thanks for the reply. My responses are inline.
diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c
--- a/linux/drivers/media/video/ir-kbd-i2c.c Wed Jul 15 07:28:02
2009 -0300
+++ b/linux/drivers/media/video/ir-kbd-i2c.c Fri Jul 17 16:05:28
2009 -0400
@@ -478,7 +480,34 @@
ir_codes = init_data-ir_codes;
name = init_data-name;
+ if (init_data-type)
+ ir_type = init_data-type;
ir-get_key = init_data-get_key;
+ switch (init_data-internal_get_key_func) {
+ case IR_KBD_GET_KEY_PIXELVIEW:
+ ir-get_key = get_key_pixelview;
+ break;
+ case IR_KBD_GET_KEY_PV951:
+ ir-get_key = get_key_pv951;
+ break;
+ case IR_KBD_GET_KEY_HAUP:
+ ir-get_key = get_key_haup;
+ break;
+ case IR_KBD_GET_KEY_KNC1:
+ ir-get_key = get_key_knc1;
+ break;
+ case IR_KBD_GET_KEY_FUSIONHDTV:
+ ir-get_key = get_key_fusionhdtv;
+ break;
+ case IR_KBD_GET_KEY_HAUP_XVR:
+ ir-get_key = get_key_haup_xvr;
+ break;
+ case IR_KBD_GET_KEY_AVERMEDIA_CARDBUS:
+ ir-get_key = get_key_avermedia_cardbus;
+ break;
+ default:
+ break;
+ }
}
/* Make sure we are all setup before going on */
diff -r d754a2d5a376 linux/include/media/ir-kbd-i2c.h
--- a/linux/include/media/ir-kbd-i2c.hWed Jul 15 07:28:02 2009 -0300
+++ b/linux/include/media/ir-kbd-i2c.hFri Jul 17 16:05:28 2009 -0400
@@ -24,10 +24,27 @@
int(*get_key)(struct IR_i2c*, u32*, u32*);
};
+enum ir_kbd_get_key_fn {
+ IR_KBD_GET_KEY_NONE = 0,
As you never use IR_KBD_GET_KEY_NONE, you might as well not define it
and start with IR_KBD_GET_KEY_PIXELVIEW = 1. This would have the added
advantage that you could get rid of the default statement in the
above switch, letting gcc warn you (or any other developer) if you ever
add a new enum value and forget to handle it in ir_probe().
From gcc-4.0.1 docs:
-Wswitch
Warn whenever a switch statement has an index of enumerated type
and lacks a case for one or more of the named codes of that
enumeration. (The presence of a default label prevents this
warning.) case labels outside the enumeration range also provoke
warnings when this option is used. This warning is enabled by
-Wall.
Since a calling driver may provide a value of 0 via a memset, I'd choose
keeping the enum label of IR_KBD_GET_KEY_NONE, add a case for it in the
switch(), and remove the default: case.
Yes, that's fine with me too. You might want to rename
IR_KBD_GET_KEY_NONE to IR_KBD_GET_KEY_CUSTOM then, and move
ir-get_key = init_data-get_key;
inside the switch.
It just seems wrong to let
drivers pass in 0 value for internal_get_key_func and the switch()
neither have an explicit nor a default: case for it. (Maybe it's just
the years of Ada programming that beat things like this into me...)
My idea was that a driver would
a. for a driver provided function, specify a pointer to the driver's
function in get_key and set the internal_get_key_func field set to 0
(IR_KBD_GET_KEY_NONE) likely via memset().
b. for a ir-kbd-i2c provided function, specify a NULL pointer in
get_key, and use an enumerated value in internal_get_key_func.
If both are specified, the switch() will set to use the ir-kbd-i2c
internal function, unless an invalid enum value was used.
My key point was that the default case would prevent gcc from helping
you. Any solution without the default case is thus fine with me.
--
Jean Delvare
--
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/~awalls/v4l-dvb
Hi Andy, On Sat, 05 Sep 2009 10:13:41 -0400, Andy Walls wrote: Mauro, Please pull from http://linuxtv.org/hg/~awalls/v4l-dvb for the following changeset: 01/01: cx18: ir-kbd-i2c initialization data should point to a persistent object http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=471784201e1b cx18-i2c.c | 23 ++- 1 file changed, 14 insertions(+), 9 deletions(-) I've marked this one a high priority so cx18 users can have working IR input. Thanks go to Dustin Mitchell for reporting the cx18 problem and testing the patch on a 2.6.30 kernel for me. Thanks go to Brian Rogers for pointing out the solution in the context of submitting a patch for a few other drivers. Good catch. Acked-by: Jean Delvare kh...@linux-fr.org As far as I can see, the em28xx and saa7134 drivers have the exact same problem. Is there anyone working on this? -- Jean Delvare -- 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/~awalls/v4l-dvb
On Sun, 06 Sep 2009 12:31:24 -0400, Andy Walls wrote: On Sat, 2009-09-05 at 20:46 +0200, Jean Delvare wrote: Hi Andy, On Sat, 05 Sep 2009 10:13:41 -0400, Andy Walls wrote: Mauro, Please pull from http://linuxtv.org/hg/~awalls/v4l-dvb for the following changeset: 01/01: cx18: ir-kbd-i2c initialization data should point to a persistent object http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=471784201e1b cx18-i2c.c | 23 ++- 1 file changed, 14 insertions(+), 9 deletions(-) I've marked this one a high priority so cx18 users can have working IR input. Thanks go to Dustin Mitchell for reporting the cx18 problem and testing the patch on a 2.6.30 kernel for me. Thanks go to Brian Rogers for pointing out the solution in the context of submitting a patch for a few other drivers. Jean, Good catch. Well I can take very little credit: a user reported a cx18 problem on the ivtv-users list, and I saw the solution pop up on the LMML for em28xx and saa7134. Acked-by: Jean Delvare kh...@linux-fr.org As far as I can see, the em28xx and saa7134 drivers have the exact same problem. Is there anyone working on this? Not me. (I don't have a 2.6.30 kernel setup yet.) Brain Rogers submitted a patch to the LMML and LKML on 4 Sep 09. Have a look: http://patchwork.kernel.org/patch/45707/ (The patch does not have V4L2 backward comptability stuff.) OK, very good then. -- Jean Delvare -- 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: [2.6.31] ir-kbd-i2c oops.
Hi Pawel, On Wed, 16 Sep 2009 03:00:28 +0200, Paweł Sikora wrote: the latest 2.6.31 kernel oopses in ir-kbd-i2c on my box: afaics the 2.6.28.10 is also affected. http://imgbin.org/index.php?page=imageid=776 http://imgbin.org/index.php?page=imageid=777 http://imgbin.org/index.php?page=imageid=778 installed pinnacle tv card with infra-red receiver: 05:00.0 Multimedia controller: Philips Semiconductors SAA7133/SAA7135 Video Broadcast Decoder (rev d1) Subsystem: Pinnacle Systems Inc. PCTV 110i (saa7133) Kernel driver in use: saa7134 Kernel modules: saa7134 if you need i'll provide more information. please, CC me on reply. This would have best been posted to linux-media... Cc'd. I think this would be fixed by the following patch: http://patchwork.kernel.org/patch/45707/ Can you please give it a try? If I am correct then only kernel 2.6.31 would be affected, 2.6.30 wouldn't be. -- Jean Delvare -- 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] Fix adv7180 build failures with old kernels
The adv7180 driver is a new-style i2c driver, unconditionally using struct i2c_device_id. As such, it can't be built on kernels older than 2.6.26. Signed-off-by: Jean Delvare kh...@linux-fr.org --- v4l/versions.txt |1 + 1 file changed, 1 insertion(+) --- v4l-dvb.orig/v4l/versions.txt 2009-09-26 13:10:09.0 +0200 +++ v4l-dvb/v4l/versions.txt2009-09-26 14:37:43.0 +0200 @@ -38,6 +38,7 @@ SOC_CAMERA_PLATFORM [2.6.26] # Requires struct i2c_device_id VIDEO_TVP514X +VIDEO_ADV7180 # requires id_table and new i2c stuff RADIO_TEA5764 VIDEO_THS7303 -- Jean Delvare -- 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: [2.6.31] ir-kbd-i2c oops.
Hi Pawel,
I am removing the linux-i2c list from Cc, because it seems clear that
your problem is related to specific media drivers and not the i2c
subsystem.
On Wed, 30 Sep 2009 10:16:15 +0200, Paweł Sikora wrote:
On Tuesday 29 September 2009 16:16:29 Jean Delvare wrote:
On Wed, 16 Sep 2009 10:03:32 +0200, Paweł Sikora wrote:
On Wednesday 16 September 2009 08:57:01 Jean Delvare wrote:
Hi Pawel,
I think this would be fixed by the following patch:
http://patchwork.kernel.org/patch/45707/
still oopses. this time i've attached full dmesg.
Any news on this? Do you have a refined list of kernels which have the
bug and kernels which do not?
afaics in the 2.6.2{7,8}, the remote sends some noises to pc.
effect: random characters on terminal and unusable login prompt.
now in the 2.6.31, the kernel module oopses during udev loading.
so i've renamed the .ko to prevent loading.
This is contradictory with your initial statement: afaics the
2.6.28.10 is also affected. It would be good to have real data points,
otherwise investigation will be very difficult...
It would be great if you could test kernel 2.6.30 and report whether it
oopses or not. The big ir-kbd-i2c changes went into kernel 2.6.31, so
my bet is that 2.6.30 should not oops, but I'd rather be certain of
this, otherwise we might keep searching in the wrong direction.
Tried 2.6.32-rc1? Tried the v4l-dvb repository?
no.
I am also skeptical about the +0x64/0x1a52, ir_input_init() is a rather
small function and I fail to see how it could be 6738 bytes in binary size.
i've attached asm dump of ir-common.ko
i found the '41 c7 80 cc ...' code in dump at adress 0x83e.
Not sure why you look at address 0x83e? The stack trace says +0x64. As
function ir_input_init() starts at 0x800, the oops address would be
0x864, which is:
864:f0 0f ab 31 lock bts %esi,(%rcx)
If my disassembler skills are still worth anything, this corresponds to
the set_bit instruction in:
for (i = 0; i IR_KEYTAB_SIZE; i++)
set_bit(ir-ir_codes[i], dev-keybit);
in the source code. This suggests that ir-ir_codes is smaller than
expected (sounds unlikely as this array is included in struct
ir_input_state) or dev-keybit isn't large enough (sounds unlikely as
well, it should be large enough to contain 0x300 bits while ir keycodes
are all below 0x100.) So most probably something went wrong before and
we're only noticing now.
Are you running distribution kernels or self-compiled ones? Any local
patches applied?
Would you be able to apply debug patches and rebuild your kernel?
At this point, all I can offer is instrumenting ir_probe() and
ir_input_init() with log messages to see exactly what code paths are
taken and what parameters are passed around.
--
Jean Delvare
--
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: [2.6.31] ir-kbd-i2c oops.
On Wed, 30 Sep 2009 13:52:27 +0200, Paweł Sikora wrote:
On Wednesday 30 September 2009 12:57:37 Jean Delvare wrote:
Are you running distribution kernels or self-compiled ones?
Any local patches applied?
Would you be able to apply debug patches and rebuild your kernel?
yes, i'm using patched (vserver,grsec) modular kernel from pld-linux
but i'm able to boot custom git build and do the bisect if necessary.
OK, then it would be great if you could try the patch below on top of
kernel 2.6.31, and report everything that gets logged before the oops.
Of course, if you can also bisect to find out which exact change causes
the oops, that would be very helpful.
---
drivers/media/common/ir-functions.c |8 +++-
drivers/media/video/ir-kbd-i2c.c|6 ++
2 files changed, 13 insertions(+), 1 deletion(-)
--- linux-2.6.31.orig/drivers/media/common/ir-functions.c 2009-06-10
05:05:27.0 +0200
+++ linux-2.6.31/drivers/media/common/ir-functions.c2009-09-30
14:15:10.0 +0200
@@ -62,6 +62,9 @@ void ir_input_init(struct input_dev *dev
{
int i;
+ pr_info(%s: dev=%p, ir=%p, ir_type=%d, ir_codes=%p\n,
+ __func__, dev, ir, ir_type, ir_codes);
+
ir-ir_type = ir_type;
if (ir_codes)
memcpy(ir-ir_codes, ir_codes, sizeof(ir-ir_codes));
@@ -69,8 +72,11 @@ void ir_input_init(struct input_dev *dev
dev-keycode = ir-ir_codes;
dev-keycodesize = sizeof(IR_KEYTAB_TYPE);
dev-keycodemax = IR_KEYTAB_SIZE;
- for (i = 0; i IR_KEYTAB_SIZE; i++)
+ for (i = 0; i IR_KEYTAB_SIZE; i++) {
+ pr_info(%s: [i=%d] Setting bit %u of dev-keybit\n,
+ __func__, i, ir-ir_codes[i]);
set_bit(ir-ir_codes[i], dev-keybit);
+ }
clear_bit(0, dev-keybit);
set_bit(EV_KEY, dev-evbit);
--- linux-2.6.31.orig/drivers/media/video/ir-kbd-i2c.c 2009-09-10
10:08:22.0 +0200
+++ linux-2.6.31/drivers/media/video/ir-kbd-i2c.c 2009-09-30
14:17:37.0 +0200
@@ -317,6 +317,7 @@ static int ir_probe(struct i2c_client *c
ir-input = input_dev;
i2c_set_clientdata(client, ir);
+ pr_info(%s: addr=0x%02hx\n, __func__, addr);
switch(addr) {
case 0x64:
name= Pixelview;
@@ -385,6 +386,9 @@ static int ir_probe(struct i2c_client *c
goto err_out_free;
}
+ pr_info(%s: [before override] ir_codes=%p, name=%s, get_key=%p\n,
+ __func__, ir_codes, name, ir-get_key);
+
/* Let the caller override settings */
if (client-dev.platform_data) {
const struct IR_i2c_init_data *init_data =
@@ -393,6 +397,8 @@ static int ir_probe(struct i2c_client *c
ir_codes = init_data-ir_codes;
name = init_data-name;
ir-get_key = init_data-get_key;
+ pr_info(%s: [after override] ir_codes=%p, name=%s,
get_key=%p\n,
+ __func__, ir_codes, name, ir-get_key);
}
/* Make sure we are all setup before going on */
--
Jean Delvare
--
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: [2.6.31] ir-kbd-i2c oops.
Hi Andy, On Wed, 30 Sep 2009 19:42:46 -0400, Andy Walls wrote: On Wed, 2009-09-30 at 12:57 +0200, Jean Delvare wrote: Not sure why you look at address 0x83e? The stack trace says +0x64. As function ir_input_init() starts at 0x800, the oops address would be 0x864, which is: 864:f0 0f ab 31 lock bts %esi,(%rcx) If my disassembler skills are still worth anything, this corresponds to the set_bit instruction in: for (i = 0; i IR_KEYTAB_SIZE; i++) set_bit(ir-ir_codes[i], dev-keybit); in the source code. This suggests that ir-ir_codes is smaller than expected (sounds unlikely as this array is included in struct ir_input_state) or dev-keybit isn't large enough (sounds unlikely as well, it should be large enough to contain 0x300 bits while ir keycodes are all below 0x100.) So most probably something went wrong before and we're only noticing now. Jean, You should be aware that the type of ir_codes changed recently from IR_KEYTAB_TYPE to struct ir_scancode_table * I'm not sure if it is the problem here, but it may be prudent to check that there's no mismatch between the module and the structure definitions being pulled in via #include (maybe by stopping gcc after the preprocessing with -E ). Thanks for the hint. As far as I can see, this change is new in kernel 2.6.32-rc1. In 2.6.31, which is where Pawel reported the issue, we still have IR_KEYTAB_TYPE. Pawel, are you by any chance mixing kernel drivers of different sources? Best would be to provide the output of rpm -qf and modinfo for all related kernel modules: rpm -qf /lib/modules/$(uname -r)/kernel/drivers/media/video/ir-kbd-i2c.ko rpm -qf /lib/modules/$(uname -r)/kernel/drivers/media/common/ir-common.ko rpm -qf /lib/modules/$(uname -r)/kernel/drivers/media/video/saa7134/saa7134.ko modinfo ir-kbd-i2c modinfo ir-common modinfo saa7134 Thanks, -- Jean Delvare -- 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: [2.6.31] ir-kbd-i2c oops.
On Thu, 01 Oct 2009 12:17:20 +0200, Paweł Sikora wrote: Dnia 01-10-2009 o 12:06:09 Jean Delvare kh...@linux-fr.org napisał(a): I'm not sure if it is the problem here, but it may be prudent to check that there's no mismatch between the module and the structure definitions being pulled in via #include (maybe by stopping gcc after the preprocessing with -E ). Thanks for the hint. As far as I can see, this change is new in kernel 2.6.32-rc1. In 2.6.31, which is where Pawel reported the issue, we still have IR_KEYTAB_TYPE. Pawel, are you by any chance mixing kernel drivers of different sources? everything is under control. i've two separated builds: - 2.6.31 from git with debugging patch. - vendor kernel from rpms. both kernels have separated initrd images for easy booting/testing. And both have the problem you reported? -- Jean Delvare -- 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: [2.6.31] ir-kbd-i2c oops.
= ir_codes_behold;
break;
case SAA7134_BOARD_AVERMEDIA_CARDBUS_501:
case SAA7134_BOARD_AVERMEDIA_CARDBUS_506:
@@ -767,8 +766,8 @@ void saa7134_probe_i2c_ir(struct saa7134
break;
}
- if (init_data.name)
- info.platform_data = init_data;
+ if (dev-ir_init_data.name)
+ info.platform_data = dev-ir_init_data;
/* No need to probe if address is known */
if (info.addr) {
i2c_new_device(dev-i2c_adap, info);
--- linux-2.6.31.orig/drivers/media/video/saa7134/saa7134.h 2009-09-10
10:08:22.0 +0200
+++ linux-2.6.31/drivers/media/video/saa7134/saa7134.h 2009-10-01
13:36:53.0 +0200
@@ -584,6 +584,9 @@ struct saa7134_dev {
intnosignal;
unsigned int insuspend;
+ /* I2C keyboard data */
+ struct IR_i2c_init_datair_init_data;
+
/* SAA7134_MPEG_* */
struct saa7134_ts ts;
struct saa7134_dmaqueuets_q;
--
Jean Delvare
--
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] i2c_board_info can be local
Recent fixes to the em28xx and saa7134 drivers have been overzealous.
While the ir-kbd-i2c platform data indeed needs to be persistent, the
struct i2c_board_info doesn't, as it is only used by i2c_new_device().
So revert a part of the original fixes, to save some memory.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Mauro Carvalho Chehab mche...@redhat.com
---
linux/drivers/media/video/em28xx/em28xx-cards.c |9 +
linux/drivers/media/video/em28xx/em28xx.h |1 -
linux/drivers/media/video/saa7134/saa7134-input.c | 21 +++--
linux/drivers/media/video/saa7134/saa7134.h |1 -
4 files changed, 16 insertions(+), 16 deletions(-)
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx-cards.c
2009-09-26 13:10:08.0 +0200
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx-cards.c 2009-10-02
10:05:47.0 +0200
@@ -2306,6 +2306,7 @@ void em28xx_register_i2c_ir(struct em28x
return;
}
#else
+ struct i2c_board_info info;
const unsigned short addr_list[] = {
0x30, 0x47, I2C_CLIENT_END
};
@@ -2313,9 +2314,9 @@ void em28xx_register_i2c_ir(struct em28x
if (disable_ir)
return;
- memset(dev-info, 0, sizeof(dev-info));
+ memset(info, 0, sizeof(struct i2c_board_info));
memset(dev-init_data, 0, sizeof(dev-init_data));
- strlcpy(dev-info.type, ir_video, I2C_NAME_SIZE);
+ strlcpy(info.type, ir_video, I2C_NAME_SIZE);
#endif
/* detect configure */
@@ -2361,8 +2362,8 @@ void em28xx_register_i2c_ir(struct em28x
#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
if (dev-init_data.name)
- dev-info.platform_data = dev-init_data;
- i2c_new_probed_device(dev-i2c_adap, dev-info, addr_list);
+ info.platform_data = dev-init_data;
+ i2c_new_probed_device(dev-i2c_adap, info, addr_list);
#endif
}
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx.h 2009-09-26
13:10:09.0 +0200
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx.h 2009-10-02
10:13:10.0 +0200
@@ -625,7 +625,6 @@ struct em28xx {
#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
/* I2C keyboard data */
- struct i2c_board_info info;
struct IR_i2c_init_data init_data;
#endif
};
--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-input.c
2009-09-26 13:10:09.0 +0200
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-input.c 2009-10-02
10:15:04.0 +0200
@@ -745,6 +745,7 @@ void saa7134_probe_i2c_ir(struct saa7134
#endif
{
#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
+ struct i2c_board_info info;
const unsigned short addr_list[] = {
0x7a, 0x47, 0x71, 0x2d,
I2C_CLIENT_END
@@ -771,9 +772,9 @@ void saa7134_probe_i2c_ir(struct saa7134
}
#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
- memset(dev-info, 0, sizeof(dev-info));
+ memset(info, 0, sizeof(struct i2c_board_info));
memset(dev-init_data, 0, sizeof(dev-init_data));
- strlcpy(dev-info.type, ir_video, I2C_NAME_SIZE);
+ strlcpy(info.type, ir_video, I2C_NAME_SIZE);
#endif
switch (dev-board) {
@@ -791,7 +792,7 @@ void saa7134_probe_i2c_ir(struct saa7134
#else
dev-init_data.get_key = get_key_pinnacle_color;
dev-init_data.ir_codes =
ir_codes_pinnacle_color_table;
- dev-info.addr = 0x47;
+ info.addr = 0x47;
#endif
} else {
#if LINUX_VERSION_CODE KERNEL_VERSION(2, 6, 30)
@@ -800,7 +801,7 @@ void saa7134_probe_i2c_ir(struct saa7134
#else
dev-init_data.get_key = get_key_pinnacle_grey;
dev-init_data.ir_codes = ir_codes_pinnacle_grey_table;
- dev-info.addr = 0x47;
+ info.addr = 0x47;
#endif
}
break;
@@ -824,7 +825,7 @@ void saa7134_probe_i2c_ir(struct saa7134
dev-init_data.name = MSI t...@nywhere Plus;
dev-init_data.get_key = get_key_msi_tvanywhere_plus;
dev-init_data.ir_codes = ir_codes_msi_tvanywhere_plus_table;
- dev-info.addr = 0x30;
+ info.addr = 0x30;
/* MSI t...@nywhere Plus controller doesn't seem to
respond to probes unless we read something from
an existing device. Weird...
@@ -875,22 +876,22 @@ void saa7134_probe_i2c_ir(struct saa7134
#else
case SAA7134_BOARD_AVERMEDIA_CARDBUS_501:
case SAA7134_BOARD_AVERMEDIA_CARDBUS_506:
- dev-info.addr = 0x40;
+ info.addr = 0x40;
#endif
break;
}
#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
if (dev-init_data.name)
- dev-info.platform_data = dev-init_data
[PATCH] Fix wrong sizeof
Which is why I have always preferred sizeof(struct foo) over sizeof(var). Signed-off-by: Jean Delvare kh...@linux-fr.org --- linux/drivers/media/dvb/dvb-usb/ce6230.c|2 +- linux/drivers/media/video/saa7164/saa7164-cmd.c |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) --- v4l-dvb.orig/linux/drivers/media/dvb/dvb-usb/ce6230.c 2009-09-26 13:10:08.0 +0200 +++ v4l-dvb/linux/drivers/media/dvb/dvb-usb/ce6230.c2009-10-02 11:03:17.0 +0200 @@ -105,7 +105,7 @@ static int ce6230_i2c_xfer(struct i2c_ad int i = 0; struct req_t req; int ret = 0; - memset(req, 0, sizeof(req)); + memset(req, 0, sizeof(req)); if (num 2) return -EINVAL; --- v4l-dvb.orig/linux/drivers/media/video/saa7164/saa7164-cmd.c 2009-09-26 13:10:09.0 +0200 +++ v4l-dvb/linux/drivers/media/video/saa7164/saa7164-cmd.c 2009-10-02 11:03:21.0 +0200 @@ -347,7 +347,7 @@ int saa7164_cmd_send(struct saa7164_dev /* Prepare some basic command/response structures */ memset(command_t, 0, sizeof(command_t)); - memset(response_t, 0, sizeof(response_t)); + memset(response_t, 0, sizeof(response_t)); pcommand_t = command_t; presponse_t = response_t; command_t.id = id; -- Jean Delvare -- 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
IR device at I2C address 0x7a
Hi all,
[Executive summary: Upmost Purple TV adapter testers wanted!]
While investigating another issue, I have noticed the following message
in the kernel logs of a saa7134 user:
i2c-adapter i2c-0: Invalid 7-bit address 0x7a
The address in question is attempted by an IR device probe, and is only
probed on SAA7134 adapters. The problem with this address is that it is
reserved by the I2C specification for 10-bit addressing, and is thus
not a valid 7-bit address. Before the conversion of ir-kbd-i2c to the
new-style i2c device driver binding model, device probing was done by
ir-kbd-i2c itself so no check was done by i2c-core for address
validity. But since kernel 2.6.31, probing at address 0x7a will be
denied by i2c-core.
So, SAA7134 adapters with an IR device at 0x7a are currently broken.
Do we know how many devices use this address for IR and which they
are? Tracking the changes in the source code, this address was added
in kernel 2.6.8 by Gerd Knorr:
http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commitdiff;h=581f0d1a0ded3e3d4408e5bb7f81b9ee221f6b7a
So this would be used by the Upmost Purple TV adapter. Question is,
are there other?
Some web research has pointed me to the Yuan TUN-900:
http://www.linuxtv.org/pipermail/linux-dvb/2008-January/023405.html
but it isn't clear to me whether the device at 0x7a on this adapter is
the same as the one on the Purple TV. And saa7134-cards says of the
TUN-900: Remote control not yet implemented so maybe we can just
ignore it for now.
If we have to, I could make i2c-core more permissive, but I am rather
reluctant to letting invalid addressed being probed, because you never
know how complying chips on the I2C bus will react. I am OK supporting
invalid addresses if they really exist out there, but the impact should
be limited to the hardware in question.
If we only have to care about the Upmost Purple TV, then the following
patch should solve the problem:
* * * * *
From: Jean Delvare kh...@linux-fr.org
Subject: saa7134: Fix IR support for Purple TV
The i2c core prevents us from probing I2C address 0x7a because it's
not a valid 7-bit address (reserved for 10-bit addressing.) So we must
stop probing this address, and explicitly list all adapters which use
it. Under the assumption that only the Upmost Purple TV adapter uses
this invalid address, this fix should do the trick.
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
linux/drivers/media/video/saa7134/saa7134-input.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-input.c
2009-10-02 13:26:39.0 +0200
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-input.c 2009-10-02
13:26:49.0 +0200
@@ -746,7 +746,7 @@ void saa7134_probe_i2c_ir(struct saa7134
{
#if LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 30)
const unsigned short addr_list[] = {
- 0x7a, 0x47, 0x71, 0x2d,
+ 0x47, 0x71, 0x2d,
I2C_CLIENT_END
};
@@ -813,6 +813,7 @@ void saa7134_probe_i2c_ir(struct saa7134
dev-init_data.name = Purple TV;
dev-init_data.get_key = get_key_purpletv;
dev-init_data.ir_codes = ir_codes_purpletv_table;
+ info.addr = 0x7a;
#endif
break;
case SAA7134_BOARD_MSI_TVATANYWHERE_PLUS:
--
Jean Delvare
--
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: [2.6.31] ir-kbd-i2c oops.
Hi Pawel, On Sat, 3 Oct 2009 12:08:36 +0200, Paweł Sikora wrote: On Thursday 01 October 2009 13:43:43 Jean Delvare wrote: Pawel, please give a try to the following patch. Please keep the debug patches apply too, in case we need additional info. the second patch helps. here's a dmesg log. OK. So the bug is exactly what I said on my very first reply. And the patch I pointed you to back then should have fixed it: http://patchwork.kernel.org/patch/45707/ You said it didn't, which makes me wonder if you really tested it properly... Anyway this is already fixed upstream, and the fix should be backported to 2.6.31-stable quickly. I'll make sure it happens. -- Jean Delvare http://khali.linux-fr.org/wishlist.html -- 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: [2.6.31] ir-kbd-i2c oops.
Hi Pawel, Please keep the list Cc'd. On Sat, 3 Oct 2009 17:30:44 +0200, Paweł Sikora wrote: On Saturday 03 October 2009 14:04:47 you wrote: OK. So the bug is exactly what I said on my very first reply. And the patch I pointed you to back then should have fixed it: http://patchwork.kernel.org/patch/45707/ You said it didn't, which makes me wonder if you really tested it properly... hmm, it's possible that i've ran system with wrong initrd and it had loaded unpatched /lib/modules/$build. i've tested patch 45707 today and it works, so my fault. moreover, with this patch i'm observing a flood in dmesg: [ 938.313245] i2c IR (Pinnacle PCTV): unknown key: key=0x12 raw=0x12 down=1 [ 938.419914] i2c IR (Pinnacle PCTV): unknown key: key=0x12 raw=0x12 down=0 [ 939.273249] i2c IR (Pinnacle PCTV): unknown key: key=0x24 raw=0x24 down=1 [ 939.379955] i2c IR (Pinnacle PCTV): unknown key: key=0x24 raw=0x24 down=0 Different issue, and I don't know much about IR support, but these keys aren't listed in ir_codes_pinnacle_color. Maybe you have a different variant of this remote control with more keys and we need to add their definitions. Which keys are triggering these messages? -- Jean Delvare http://khali.linux-fr.org/wishlist.html -- 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: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels
On Sun, 4 Oct 2009 11:31:39 +0300, Aleksandr V. Piskunov wrote: Tested on 2.6.30.8, one of Ubuntu mainline kernel builds. ivtv-i2c part works, ivtv_i2c_new_ir() gets called, according to /sys/bus/i2c device @ 0x40 gets a name ir_rx_em78p153s_ave. Now according to my (very) limited understanding of new binding model, ir-kbd-i2c should attach to this device by its name. Somehow it doesn't, ir-kbd-i2c gets loaded silently without doing anything. Change the device name to a shorter string (e.g. ir_rx_em78p153s). You're hitting the i2c client name length limit. More details about this in the details reply I'm writing right now. -- Jean Delvare -- 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: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels
?
+}
+
/* Instantiate the IR receiver device using probing -- undesirable */
int ivtv_i2c_new_ir_legacy(struct ivtv *itv)
{
@@ -185,9 +220,15 @@
? -1 : 0;
}
#else
+/* ir-kbd-i2c does the legacy I2C IR controller probe for old kernels*/
Missing space at end of comment.
+static int ivtv_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char
*type,
+u8 addr)
+{
+ return -1;
+}
+
int ivtv_i2c_new_ir_legacy(struct ivtv *itv)
{
- /* ir-kbd-i2c does the legacy I2C IR controller probe for old kernels*/
return -1;
}
#endif
@@ -221,8 +262,15 @@
sd-grp_id = 1 idx;
return sd ? 0 : -1;
}
+
+ if (hw IVTV_HW_EM78P153S_IR_RX_AVER)
Maybe use IVTV_HW_IR_ANY as I defined earlier? Otherwise you'll have to
modify the code with each new remote control you add.
+ return ivtv_i2c_new_ir(adap, hw, type, hw_addrs[idx]);
+
+ /* Is it not an I2C device or one we do not wish to register? */
if (!hw_addrs[idx])
return -1;
+
+ /* It's an I2C device other than an analog tuner or IR chip */
if (hw == IVTV_HW_UPD64031A || hw == IVTV_HW_UPD6408X) {
sd = v4l2_i2c_new_subdev(itv-v4l2_dev,
adap, mod, type, 0, I2C_ADDRS(hw_addrs[idx]));
Patch #3.
--- a/linux/drivers/media/video/ir-kbd-i2c.c Sat Oct 03 11:23:00 2009 -0400
+++ b/linux/drivers/media/video/ir-kbd-i2c.c Sat Oct 03 11:27:19 2009 -0400
@@ -730,6 +730,7 @@
{ ir_video, 0 },
/* IR device specific entries should be added here */
{ ir_rx_z8f0811_haup, 0 },
+ { ir_rx_em78p153s_aver, 0 },
{ }
};
I think we need to discuss this. I don't really see the point of adding
new entries if the ir-kbd-i2c driver doesn't do anything about it. This
makes device probing slower with no benefit. As long as you pass device
information with all the details, the ir-kbd-i2c driver won't care
about the device name.
So the question is, where are we going with the ir-kbd-i2c driver? Are
we happy with the current model where bridge drivers pass IR device
information? Or do we want to move to a model where they just pass a
device name and ir-kbd-i2c maps names to device information? In the
latter case, it makes sense to have many i2c_device_id entries in
ir-kbd-i2c, but in the former case it doesn't.
I guess the answer depends in part on how common IR devices and remote
controls are across adapters. If the same IR device is used on many
adapters then it makes some sense to move the definitions into
ir-kbd-i2c. But if devices are heavily adapter-dependent, and moving
the definitions into ir-kbd-i2c doesn't allow for any code refactoring,
then I don't quite see the point.
--
Jean Delvare
--
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
TerraTec Cinergy T PCIe Dual not working
Hi all, I have a TerraTec Cinergy T PCIe Dual card and I can't get it to work. I had been using it several months ago, with some success, although the stability wasn't ideal. Then I moved it to another machine to check if it was any better on Windows (and indeed it was better.) Now I put it back in my system and I can't get it to work at all. I have the following relevant kernel log messages: [0.362783] pci :0b:00.0: [14f1:8852] type 00 class 0x04 [0.362809] pci :0b:00.0: reg 10: [mem 0xfbc0-0xfbdf 64bit] [0.362940] pci :0b:00.0: supports D1 D2 [0.362942] pci :0b:00.0: PME# supported from D0 D1 D2 D3hot [0.362967] pci :0b:00.0: disabling ASPM on pre-1.1 PCIe device. You can enable it with 'pcie_aspm=force' [6.313259] cx23885 driver version 0.0.3 loaded [6.313898] CORE cx23885[0]: subsystem: 153b:117e, board: TerraTec Cinergy T PCIe Dual [card=34,autodetected] [6.498999] cx25840 11-0044: cx23885 A/V decoder found @ 0x88 (cx23885[0]) [7.114399] cx25840 11-0044: loaded v4l-cx23885-avcore-01.fw firmware (16382 bytes) [7.150641] cx23885_dvb_register() allocating 1 frontend(s) [7.150704] cx23885[0]: cx23885 based dvb card [7.178681] drxk: status = 0x639160d9 [7.180049] drxk: detected a drx-3916k, spin A3, xtal 20.250 MHz [7.245259] DRXK driver version 0.9.4300 [7.268760] drxk: frontend initialized. [7.961207] mt2063_attach: Attaching MT2063 [7.961271] DVB: registering new adapter (cx23885[0]) [7.961332] DVB: registering adapter 0 frontend 0 (DRXK DVB-T)... [7.961877] cx23885_dvb_register() allocating 1 frontend(s) [7.961881] cx23885[0]: cx23885 based dvb card [7.974320] drxk: status = 0x639130d9 [7.975838] drxk: detected a drx-3913k, spin A3, xtal 20.250 MHz [8.040888] DRXK driver version 0.9.4300 [8.064459] drxk: frontend initialized. [8.064467] mt2063_attach: Attaching MT2063 [8.064475] DVB: registering new adapter (cx23885[0]) [8.064482] DVB: registering adapter 1 frontend 0 (DRXK DVB-C DVB-T)... [8.065919] cx23885_dev_checkrevision() Hardware revision = 0xa5 [8.066004] cx23885[0]/0: found at :0b:00.0, rev: 4, irq: 28, latency: 0, mmio: 0xfbc0 [ 27.281490] mt2063: detected a mt2063 B3 [ 27.319390] mt2063: detected a mt2063 B3 I don't see anything wrong here, all the components are apparently found and identified properly. However I can't get the card to actually work, neither in me-tv nor in VLC. Me-tv (version 1.3.6) tells me: 03/06/13 14:20:10: Device: 'DRXK DVB-T' (DVB-T) at /dev/dvb/adapter0/frontend0 03/06/13 14:20:11: Device: 'DRXK DVB-C DVB-T' (DVB-C) at /dev/dvb/adapter1/frontend0 03/06/13 14:20:11: Frontend::tune_to(49000) 03/06/13 14:20:11: Waiting for signal lock ... And then Failed to lock channel. VLC (version 2.0.5) tells me: main stream error: cannot pre fill buffer I tried with kernels 3.4.30, 3.5.7, 3.6.0, 3.6.11, 3.7.10 and 3.8.2, with exactly the same results. How would I debug this further? Thanks, -- Jean Delvare -- 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: TerraTec Cinergy T PCIe Dual not working
Hi Oliver, Thanks for your fast reply. On Wed, 06 Mar 2013 14:58:05 +0100, Oliver Schinagl wrote: I have the same card, and have not much problems. I have some reception issues, but I don't think it's to blame on the card (yet). I do use tvheadend however. In anycase, can you use w_scan or dvb-scan? I have neither but I have scan from package dvb which does work. This gave me the idea to re-run scan with different frequency files and different antennas. It turns out that my problem is the antenna. I was using the antenna I have been using with my previous card, which is an internal DVB-T antenna with amplification (external power supply.) I get zero signal with that. But using the Terratec-provided cheap stick antenna, I get signal again, with reasonable quality (although not as stable as with the old card and the powered antenna.) I also get signal (but not all channels) with my original antenna _unpowered_ (thus signal not amplified.) I admit I don't quite understand. I would understand that a bad, unpowered antenna causes no signal to be sensed. But how is it possible that a supposedly better, powered antenna causes that kind of issue? Oliver, out of curiosity, what antenna are you using? The Terratec-provided one, or another one? -- Jean Delvare -- 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
drxk driver statistics
Hi all, I have a TerraTec Cinergy T PCIe Dual card, with DRX-3916K and DRX-3913K frontends. I am thus using the drxk dvb-frontend driver. While trying to find the best antenna, position and amplification, I found that the statistics returned by the drxk driver look quite bad: $ femon -H 3 FE: DRXK DVB-T (DVBT) status SCVYL | signal 0% | snr 0% | ber 0 | unc 38822 | FE_HAS_LOCK status SCVYL | signal 0% | snr 0% | ber 0 | unc 38822 | FE_HAS_LOCK status SCVYL | signal 0% | snr 0% | ber 0 | unc 38822 | FE_HAS_LOCK This is with TV looking reasonably good, so these figures are not plausible. $ femon 10 FE: DRXK DVB-T (DVBT) status SCVYL | signal 00de | snr 00f5 | ber | unc 97a6 | FE_HAS_LOCK status SCVYL | signal 00f0 | snr 00f5 | ber | unc 97a6 | FE_HAS_LOCK status SCVYL | signal 0117 | snr 00f6 | ber | unc 97a6 | FE_HAS_LOCK status SCVYL | signal 00b6 | snr 00eb | ber | unc 97a6 | FE_HAS_LOCK status SCVYL | signal 00d1 | snr 00e7 | ber | unc 97a6 | FE_HAS_LOCK status SCVYL | signal 0073 | snr 00ea | ber | unc 97a6 | FE_HAS_LOCK status SCVYL | signal 00a3 | snr 00ee | ber | unc 97a6 | FE_HAS_LOCK status SCVYL | signal 00b5 | snr 00f4 | ber | unc 97a6 | FE_HAS_LOCK status SCVYL | signal 00ba | snr 00f3 | ber | unc 97a6 | FE_HAS_LOCK status SCVYL | signal 00be | snr 00f0 | ber | unc 97a6 | FE_HAS_LOCK Signal values are changing too much, snr is stable enough but way too low, ber is apparently unimplemented, and unc is never reset AFAICS (it started at 1 when the system started and has been only increasing since then.) On my previous card, unc was an instant measurement, not a cumulative value, not sure which is correct. I would like to see these statistics improved. I am willing to help, however the drxk driver is rather complex (at least to my eyes) and I do not have a datasheet so I wouldn't know where to start. Is there anyone who can work on this and/or provide some guidance? Thanks, -- Jean Delvare -- 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: TerraTec Cinergy T PCIe Dual not working
Hi Oliver, On Wed, 06 Mar 2013 19:07:01 +0100, Oliver Schinagl wrote: On 03/06/13 16:03, Jean Delvare wrote: It turns out that my problem is the antenna. I was using the antenna I have been using with my previous card, which is an internal DVB-T antenna with amplification (external power supply.) I get zero signal with that. But using the Terratec-provided cheap stick antenna, I get signal again, with reasonable quality (although not as stable as with the old card and the powered antenna.) I also get signal (but not all channels) with my original antenna _unpowered_ (thus signal not amplified.) I admit I don't quite understand. I would understand that a bad, unpowered antenna causes no signal to be sensed. But how is it possible that a supposedly better, powered antenna causes that kind of issue? Oliver, out of curiosity, what antenna are you using? The Terratec-provided one, or another one? Right now, I use 11cm stripped coax :) But that's because I live 600 meters from the broadcasting tower. This actually gives me the best reception. The mini antenna that came with the thing worked quite well, but the wire was a bit short. Besides that I did use a powered antenna for a while, without the power connected, because it actually dampens the signal. That worked quite well for a while. Using it powered, with an external power source, actually made it much worse. My experience matches yours exactly. Thanks for confirming. I wonder if this is a limitation of the Linux drivers (not adjusting the tuner sensibility to the signal strength) or a hardware characteristic. -- Jean Delvare -- 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
[media] drxk_hard: Drop unused parameter
Last parameter of function GetLockStatus() isn't used so drop it.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Mauro Carvalho Chehab mche...@redhat.com
---
drivers/media/dvb-frontends/drxk_hard.c |5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
--- linux-3.8.orig/drivers/media/dvb-frontends/drxk_hard.c 2013-02-19
00:58:34.0 +0100
+++ linux-3.8/drivers/media/dvb-frontends/drxk_hard.c 2013-03-07
09:38:36.116748279 +0100
@@ -1947,8 +1947,7 @@ static int ShutDown(struct drxk_state *s
return 0;
}
-static int GetLockStatus(struct drxk_state *state, u32 *pLockStatus,
-u32 Time)
+static int GetLockStatus(struct drxk_state *state, u32 *pLockStatus)
{
int status = -EINVAL;
@@ -6398,7 +6397,7 @@ static int drxk_read_status(struct dvb_f
return -EAGAIN;
*status = 0;
- GetLockStatus(state, stat, 0);
+ GetLockStatus(state, stat);
if (stat == MPEG_LOCK)
*status |= 0x1f;
if (stat == FEC_LOCK)
--
Jean Delvare
--
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] [media] mceusb: Optimize DIV_ROUND_CLOSEST call
DIV_ROUND_CLOSEST is faster if the compiler knows it will only be dealing with unsigned dividends. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Andrew Morton a...@linux-foundation.org Cc: Guenter Roeck li...@roeck-us.net Cc: Mauro Carvalho Chehab mche...@infradead.org --- drivers/media/rc/mceusb.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-3.6-rc3.orig/drivers/media/rc/mceusb.c2012-08-04 21:49:27.0 +0200 +++ linux-3.6-rc3/drivers/media/rc/mceusb.c 2012-09-01 18:53:32.053042123 +0200 @@ -627,7 +627,7 @@ static void mceusb_dev_printdata(struct break; case MCE_RSP_EQIRCFS: period = DIV_ROUND_CLOSEST( - (1 data1 * 2) * (data2 + 1), 10); + (1U data1 * 2) * (data2 + 1), 10); if (!period) break; carrier = (1000 * 1000) / period; -- Jean Delvare -- 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] [media] cx23885: Select drivers for Terratec Cinergy T PCIe Dual
The Terratec Cinergy T PCIe Dual is based on the CX23885, and uses MT2063, DRX-3913k and DRX-3916k chips, so select the relevant drivers. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Stefan Ringel linu...@stefanringel.de Cc: Mauro Carvalho Chehab mche...@infradead.org --- drivers/media/video/cx23885/Kconfig |2 ++ 1 file changed, 2 insertions(+) --- linux-3.6-rc5.orig/drivers/media/video/cx23885/Kconfig 2012-07-21 22:58:29.0 +0200 +++ linux-3.6-rc5/drivers/media/video/cx23885/Kconfig 2012-09-13 15:57:13.833548830 +0200 @@ -23,7 +23,9 @@ config VIDEO_CX23885 select DVB_STV0900 if !DVB_FE_CUSTOMISE select DVB_DS3000 if !DVB_FE_CUSTOMISE select DVB_STV0367 if !DVB_FE_CUSTOMISE + select DVB_DRXK if !DVB_FE_CUSTOMISE select MEDIA_TUNER_MT2131 if !MEDIA_TUNER_CUSTOMISE + select MEDIA_TUNER_MT2063 if !MEDIA_TUNER_CUSTOMISE select MEDIA_TUNER_XC2028 if !MEDIA_TUNER_CUSTOMISE select MEDIA_TUNER_TDA8290 if !MEDIA_TUNER_CUSTOMISE select MEDIA_TUNER_TDA18271 if !MEDIA_TUNER_CUSTOMISE -- Jean Delvare -- 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] mceusb: Optimize DIV_ROUND_CLOSEST call
Hi Mauro, On Tue, 18 Sep 2012 12:49:53 -0300, Mauro Carvalho Chehab wrote: Em 01-09-2012 15:53, Jean Delvare escreveu: DIV_ROUND_CLOSEST is faster if the compiler knows it will only be dealing with unsigned dividends. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Andrew Morton a...@linux-foundation.org Cc: Guenter Roeck li...@roeck-us.net Cc: Mauro Carvalho Chehab mche...@infradead.org --- drivers/media/rc/mceusb.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-3.6-rc3.orig/drivers/media/rc/mceusb.c2012-08-04 21:49:27.0 +0200 +++ linux-3.6-rc3/drivers/media/rc/mceusb.c 2012-09-01 18:53:32.053042123 +0200 @@ -627,7 +627,7 @@ static void mceusb_dev_printdata(struct break; case MCE_RSP_EQIRCFS: period = DIV_ROUND_CLOSEST( - (1 data1 * 2) * (data2 + 1), 10); + (1U data1 * 2) * (data2 + 1), 10); if (!period) break; carrier = (1000 * 1000) / period; Hmm... this generates the following warning with W=1: drivers/media/rc/mceusb.c:629:4: warning: comparison of unsigned expression = 0 is always true [-Wtype-limits] drivers/media/rc/mceusb.c:629:4: warning: comparison of unsigned expression = 0 is always true [-Wtype-limits] I doubt this is the only warning of that kind. There must be a reason why -Wextra isn't enabled by default. Perhaps it makes sense to use an optimized version for unsigned, or to change the macro to take the data types into account. This was discussed before, but Andrew said he preferred a single macro. And I agree with him, having two macros would induce a risk of the wrong one being called. If you can come up with a variant of DIV_ROUND_CLOSEST which performs the same and doesn't trigger the warning above, we'll be happy to see it, but neither Guenter nor myself could come up with one. -- Jean Delvare -- 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 9/13] drivers/media/tuners/fc0011.c: use macros for i2c_msg initialization
On Sun, 7 Oct 2012 18:50:31 +0200 (CEST), Julia Lawall wrote:
On Sun, 7 Oct 2012, walter harms wrote:
Am 07.10.2012 17:38, schrieb Julia Lawall:
@@ -97,10 +96,8 @@ static int fc0011_readreg(struct fc0011_priv *priv, u8
reg, u8 *val)
{
u8 dummy;
struct i2c_msg msg[2] = {
- { .addr = priv-addr,
-.flags = 0, .buf = reg, .len = 1 },
- { .addr = priv-addr,
-.flags = I2C_M_RD, .buf = val ? : dummy, .len = 1 },
+ I2C_MSG_WRITE(priv-addr, reg, sizeof(reg)),
+ I2C_MSG_READ(priv-addr, val ? : dummy, sizeof(dummy)),
};
This dummy looks strange, can it be that this is used uninitialised ?
I'm not sure to understand the question. The read, when it happens in
i2c_transfer will initialize dummy. On the other hand, I don't know what
i2c_transfer does when the buffer is NULL and the size is 1. It does not
look very elegant at least.
i2c_transfer() itself won't care, it just passes the request down to the
underlying i2c bus driver. Most driver implementations will assume
proper buffer addresses as soon as size 0, so passing NULL instead
would crash them. In short, don't do that.
--
Jean Delvare
--
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/13] drivers/media/tuners/qt1010.c: use macros for i2c_msg initialization
Hi Julia, On Mon, 8 Oct 2012 07:24:11 +0200 (CEST), Julia Lawall wrote: Sorry, I mean either: I2C_MSG_WRITE(priv-cfg-i2c_address, reg, sizeof(reg)), I2C_MSG_READ(priv-cfg-i2c_address, val, sizeof(*val)), Of course. Sorry for not having seen that. I can do that. Eek, no, you can't, not in the general case at least. sizeof(*val) will return the size of the _first_ element of the destination buffer, which has nothing to do with the length of that buffer (which in turn might be rightfully longer than the read length for this specific message.) -- Jean Delvare -- 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/13] drivers/media/tuners/mxl5007t.c: use macros for i2c_msg initialization
Hi Julia,
On Sun, 7 Oct 2012 17:38:33 +0200, Julia Lawall wrote:
From: Julia Lawall julia.law...@lip6.fr
Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
Next time you send this patch set, please Cc me on every post so that I
don't have to hunt for them on lkml.org.
In each case, a length expressed as an explicit constant is also
re-expressed as the size of the buffer, when this is possible.
This is conceptually wrong, please don't do that. It is perfectly valid
to use a buffer which is larger than the message being written or read.
When exchanging multiple messages, it is actually quite common to
declare only 2 buffers and reuse them:
char reg;
char val[2];
struct i2c_msg msg[2] = {
{ .addr = addr, .flags = 0, .buf = reg, .len = 1 },
{ .addr = addr, .flags = I2C_M_RD, .buf = val, .len = 1 },
};
reg = 0x04;
i2c_transfer(i2c_adap, msg, 2);
/* Do stuff with val */
reg = 0x06;
msg[1].len = 2;
i2c_transfer(i2c_adap, msg, 2);
/* Do stuff with val */
Your conversion would read 2 bytes from register 0x04 instead of 1 in
the example above.
I am not opposed to the idea of i2c_msg initialization helper macros,
but please don't mix that with actual code changes which could have bad
side effects.
Thanks,
--
Jean Delvare
--
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/11] introduce macros for i2c_msg initialization
Hi Julia, On Sun, 7 Oct 2012 17:38:30 +0200, Julia Lawall wrote: This patch set introduces some macros for describing how an i2c_msg is being initialized. There are three macros: I2C_MSG_READ, for a read message, I2C_MSG_WRITE, for a write message, and I2C_MSG_OP, for some other kind of message, which is expected to be very rarely used. Some other kind of message is actually messages which need extra flags. They are still read or write messages. OK, I've read the whole series now and grepped the kernel tree so I have a better overview. There are a lot more occurrences than what you converted. I presume the conversions were just an example and you leave the rest up to the relevant maintainers (e.g. me) if they are interested? Given the huge number of affected drivers (a quick grep suggests 230 drivers and more than 300 occurrences), we'd better think twice before going on as it will be intrusive and hard to change afterward. So my first question will be: what is your goal with this change? Are you only trying to save a few lines of source code? Or do you expect to actually fix/prevent bugs by introducing these macros? Or something else? I admit I am not completely convinced by the benefit at the moment. A number of these drivers should be using i2c_smbus_*() functions instead of i2c_transfer() for improved compatibility, or i2c_master_send/recv() for single message transfers (383 occurrences!), so making i2c_transfer() easier to use isn't at the top of my priority list. And I see the extra work for the pre-processor, so we need a good reason for doing that. -- Jean Delvare -- 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
cx23885_wakeup: 3 buffers handled (should be 1)
Hi all, I own a TerraTec Cinergy T PCIe Dual. From times to times I see the following in the kernel log: [31508.278300] cx23885_wakeup: 3 buffers handled (should be 1) [36911.156435] cx23885_wakeup: 3 buffers handled (should be 1) [52519.479142] cx23885_wakeup: 3 buffers handled (should be 1) [53720.117787] cx23885_wakeup: 3 buffers handled (should be 1) [55521.077548] cx23885_wakeup: 3 buffers handled (should be 1) [56721.718509] cx23885_wakeup: 2 buffers handled (should be 1) [59723.313871] cx23885_wakeup: 2 buffers handled (should be 1) [78333.209720] cx23885_wakeup: 3 buffers handled (should be 1) Recordings using this card have errors from times to times, I suppose this could be related. Is there anything that can be done to prevent the above from happening? First occurrence of this message in my logs is on 2013-05-27, with kernel 3.9.4. I am available for any form of debugging or experimentation. Thanks, -- Jean Delvare -- 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
