Re: [GIT PATCHES FOR 2.6.37] Remove v4l2-i2c-drv.h and most of i2c-id.h
Hi Hans,
On Wed, 15 Sep 2010 22:00:26 +0200, Hans Verkuil wrote:
Mauro, Jean, Janne,
This patch series finally retires the hackish v4l2-i2c-drv.h. It served
honorably,
but now that the hg repository no longer supports kernels 2.6.26 it is time
to
remove it.
Note that this patch series builds on the vtx-removal patch series.
Several patches at the end remove unused i2c-id.h includes and remove bogus
uses
of the I2C_HW_ defines (as found in i2c-id.h).
After applying this patch series I get the following if I grep for
I2C_HW_ in the kernel sources:
skip some false positives in drivers/gpu
drivers/staging/lirc/lirc_i2c.c:if (adap-id ==
I2C_HW_B_CX2388x)
drivers/staging/lirc/lirc_i2c.c:if (adap-id ==
I2C_HW_B_CX2388x) {
drivers/staging/lirc/lirc_zilog.c:#ifdef I2C_HW_B_HDPVR
drivers/staging/lirc/lirc_zilog.c: if (ir-c_rx.adapter-id ==
I2C_HW_B_HDPVR) {
drivers/staging/lirc/lirc_zilog.c:#ifdef I2C_HW_B_HDPVR
drivers/staging/lirc/lirc_zilog.c: if (ir-c_rx.adapter-id ==
I2C_HW_B_HDPVR)
drivers/video/riva/rivafb-i2c.c:chan-adapter.id=
I2C_HW_B_RIVA;
drivers/media/video/ir-kbd-i2c.c: if (ir-c-adapter-id ==
I2C_HW_SAA7134 ir-c-addr == 0x30)
drivers/media/video/ir-kbd-i2c.c: if (adap-id ==
I2C_HW_B_CX2388x) {
drivers/media/video/saa7134/saa7134-i2c.c: .id=
I2C_HW_SAA7134,
drivers/media/video/cx88/cx88-i2c.c:core-i2c_adap.id = I2C_HW_B_CX2388x;
drivers/media/video/cx88/cx88-vp3054-i2c.c: vp3054_i2c-adap.id =
I2C_HW_B_CX2388x;
Jean, I guess the one in rivafb-i2c.c can just be removed, right?
Correct. The last user for that one was the tvaudio driver and
apparently you just cleaned it up.
Janne, the HDPVR checks in lirc no longer work since hdpvr never sets the
adapter ID (nor should it). This lirc code should be checked. I haven't
been following the IR changes, but there must be a better way of doing this.
The same is true for the CX2388x and SAA7134 checks. These all relate to the
IR subsystem.
Once we fixed these remaining users of the i2c-id.h defines, then Jean can
remove that header together with the adapter's 'id' field.
That would be very great. In all honesty I didn't expect it to happen
so fast, but if that happens, I'll be very happy! :) 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 2/5] cx22702: Drop useless initializations to 0
These variables are either unconditionally set right afterward, or
already set to 0 by kzalloc.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Steven Toth st...@kernellabs.com
---
drivers/media/dvb/frontends/cx22702.c |5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
--- linux-2.6.32-rc5.orig/drivers/media/dvb/frontends/cx22702.c 2009-10-17
14:01:34.0 +0200
+++ linux-2.6.32-rc5/drivers/media/dvb/frontends/cx22702.c 2009-10-17
14:23:48.0 +0200
@@ -508,7 +508,7 @@ static int cx22702_read_signal_strength(
{
struct cx22702_state *state = fe-demodulator_priv;
- u16 rs_ber = 0;
+ u16 rs_ber;
rs_ber = cx22702_readreg(state, 0x23);
*signal_strength = (rs_ber 8) | rs_ber;
@@ -519,7 +519,7 @@ static int cx22702_read_snr(struct dvb_f
{
struct cx22702_state *state = fe-demodulator_priv;
- u16 rs_ber = 0;
+ u16 rs_ber;
if (cx22702_readreg(state, 0xE4) 0x02) {
/* Realtime statistics */
rs_ber = (cx22702_readreg(state, 0xDE) 0x7F) 7
@@ -590,7 +590,6 @@ struct dvb_frontend *cx22702_attach(cons
/* setup the state */
state-config = config;
state-i2c = i2c;
- state-prevUCBlocks = 0;
/* check if the demod is there */
if (cx22702_readreg(state, 0x1f) != 0x3)
--
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 4/5] cx22702: Some things never change
The init sequence never changes so it can be marked const. Likewise,
cx22702_ops is a template and can thus be made read-only.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Steven Toth st...@kernellabs.com
---
drivers/media/dvb/frontends/cx22702.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- linux-2.6.32-rc5.orig/drivers/media/dvb/frontends/cx22702.c 2009-10-17
14:49:50.0 +0200
+++ linux-2.6.32-rc5/drivers/media/dvb/frontends/cx22702.c 2009-10-18
11:44:09.0 +0200
@@ -54,7 +54,7 @@ MODULE_PARM_DESC(debug, Enable verbose
#define dprintkif (debug) printk
/* Register values to initialise the demod */
-static u8 init_tab[] = {
+static const u8 init_tab[] = {
0x00, 0x00, /* Stop aquisition */
0x0B, 0x06,
0x09, 0x01,
@@ -576,7 +576,7 @@ static void cx22702_release(struct dvb_f
kfree(state);
}
-static struct dvb_frontend_ops cx22702_ops;
+static const struct dvb_frontend_ops cx22702_ops;
struct dvb_frontend *cx22702_attach(const struct cx22702_config *config,
struct i2c_adapter *i2c)
@@ -608,7 +608,7 @@ error:
}
EXPORT_SYMBOL(cx22702_attach);
-static struct dvb_frontend_ops cx22702_ops = {
+static const struct dvb_frontend_ops cx22702_ops = {
.info = {
.name = Conexant CX22702 DVB-T,
--
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/5 v2] cx22702: Clean up register access functions
Hi Mauro,
On Fri, 10 Sep 2010 12:13:32 -0300, Mauro Carvalho Chehab wrote:
Em 10-09-2010 10:27, Jean Delvare escreveu:
* Avoid temporary variables.
* Optimize success paths.
* Make error messages consistently verbose.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Steven Toth st...@kernellabs.com
---
drivers/media/dvb/frontends/cx22702.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
--- linux-2.6.32-rc5.orig/drivers/media/dvb/frontends/cx22702.c
2009-10-16 09:47:14.0 +0200
+++ linux-2.6.32-rc5/drivers/media/dvb/frontends/cx22702.c 2009-10-16
09:47:45.0 +0200
@@ -92,33 +92,36 @@ static int cx22702_writereg(struct cx227
ret = i2c_transfer(state-i2c, msg, 1);
- if (ret != 1)
+ if (ret != 1) {
As a suggestion, if you want to optimize success paths, you should use
unlikely() for error
checks. This tells gcc to optimize the code to avoid cache cleanups for the
likely condition:
if (unlikely(ret != 1)) {
Good point. Updated patch follows:
* * * * *
* Avoid temporary variables.
* Optimize success paths.
* Make error messages consistently verbose.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Steven Toth st...@kernellabs.com
---
Changes in v2:
* Use unlikely() to help gcc optimize the success paths.
drivers/media/dvb/frontends/cx22702.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
--- linux-2.6.35.orig/drivers/media/dvb/frontends/cx22702.c 2010-09-10
14:22:31.0 +0200
+++ linux-2.6.35/drivers/media/dvb/frontends/cx22702.c 2010-09-10
17:39:58.0 +0200
@@ -92,33 +92,36 @@ static int cx22702_writereg(struct cx227
ret = i2c_transfer(state-i2c, msg, 1);
- if (ret != 1)
+ if (unlikely(ret != 1)) {
printk(KERN_ERR
%s: error (reg == 0x%02x, val == 0x%02x, ret == %i)\n,
__func__, reg, data, ret);
+ return -1;
+ }
- return (ret != 1) ? -1 : 0;
+ return 0;
}
static u8 cx22702_readreg(struct cx22702_state *state, u8 reg)
{
int ret;
- u8 b0[] = { reg };
- u8 b1[] = { 0 };
+ u8 data;
struct i2c_msg msg[] = {
{ .addr = state-config-demod_address, .flags = 0,
- .buf = b0, .len = 1 },
+ .buf = reg, .len = 1 },
{ .addr = state-config-demod_address, .flags = I2C_M_RD,
- .buf = b1, .len = 1 } };
+ .buf = data, .len = 1 } };
ret = i2c_transfer(state-i2c, msg, 2);
- if (ret != 2)
- printk(KERN_ERR %s: readreg error (ret == %i)\n,
- __func__, ret);
+ if (unlikely(ret != 2)) {
+ printk(KERN_ERR %s: error (reg == 0x%02x, ret == %i)\n,
+ __func__, reg, ret);
+ return 0;
+ }
- return b1[0];
+ return data;
}
static int cx22702_set_inversion(struct cx22702_state *state, int inversion)
--
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 8/8] v4l: radio: si470x: fix unneeded free_irq() call
On Mon, 06 Sep 2010 08:53:50 +0200, Marek Szyprowski wrote:
In case of error during probe() the driver calls free_irq() function
on not yet allocated irq. This patches fixes the call sequence in case of
the error.
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Tobias Lorenz tobias.lor...@gmx.net
CC: Joonyoung Shim jy0922.s...@samsung.com
CC: Douglas Schilling Landgraf dougsl...@redhat.com
CC: Jean Delvare kh...@linux-fr.org
---
drivers/media/radio/si470x/radio-si470x-i2c.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c
b/drivers/media/radio/si470x/radio-si470x-i2c.c
index 67a4ec8..4ce541a 100644
--- a/drivers/media/radio/si470x/radio-si470x-i2c.c
+++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
@@ -395,7 +395,7 @@ static int __devinit si470x_i2c_probe(struct i2c_client
*client,
radio-registers[POWERCFG] = POWERCFG_ENABLE;
if (si470x_set_register(radio, POWERCFG) 0) {
retval = -EIO;
- goto err_all;
+ goto err_video;
}
msleep(110);
Good catch.
Acked-by: Jean Delvare kh...@linux-fr.org
--
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 04/17] cx25840: Make cx25840 i2c register read transactions atomic
Hi Andy,
On Mon, 19 Jul 2010 21:11:46 -0400, Andy Walls wrote:
There was a small window between writing the cx25840 register
address over the i2c bus and reading the register contents back from the
cx25840 device that the i2c adapter lock was released. This change ensures
the
adapter lock is not released until the register read is done.
Signed-off-by: Andy Walls awa...@md.metrocast.net
Good catch.
Acked-by: Jean Delvare kh...@linux-fr.org
Note that cx25840_and_or() still has a (smaller and less dangerous)
race window. If several calls to cx25840_and_or() happen in parallel on
the same register, some of the changes may be lost. I don't know if
this can be a problem in practice though. If it is, then additional
locking around cx25840_and_or() would be needed.
---
drivers/media/video/cx25840/cx25840-core.c | 58
+++-
1 files changed, 39 insertions(+), 19 deletions(-)
diff --git a/drivers/media/video/cx25840/cx25840-core.c
b/drivers/media/video/cx25840/cx25840-core.c
index bb4872b..4f908fa 100644
--- a/drivers/media/video/cx25840/cx25840-core.c
+++ b/drivers/media/video/cx25840/cx25840-core.c
@@ -80,33 +80,53 @@ int cx25840_write4(struct i2c_client *client, u16 addr,
u32 value)
u8 cx25840_read(struct i2c_client * client, u16 addr)
{
- u8 buffer[2];
- buffer[0] = addr 8;
- buffer[1] = addr 0xff;
-
- if (i2c_master_send(client, buffer, 2) 2)
- return 0;
-
- if (i2c_master_recv(client, buffer, 1) 1)
+ struct i2c_msg msgs[2];
+ u8 tx_buf[2], rx_buf[1];
+
+ /* Write register address */
+ tx_buf[0] = addr 8;
+ tx_buf[1] = addr 0xff;
+ msgs[0].addr = client-addr;
+ msgs[0].flags = 0;
+ msgs[0].len = 2;
+ msgs[0].buf = (char *) tx_buf;
+
+ /* Read data from register */
+ msgs[1].addr = client-addr;
+ msgs[1].flags = I2C_M_RD;
+ msgs[1].len = 1;
+ msgs[1].buf = (char *) rx_buf;
+
+ if (i2c_transfer(client-adapter, msgs, 2) 2)
return 0;
- return buffer[0];
+ return rx_buf[0];
}
u32 cx25840_read4(struct i2c_client * client, u16 addr)
{
- u8 buffer[4];
- buffer[0] = addr 8;
- buffer[1] = addr 0xff;
-
- if (i2c_master_send(client, buffer, 2) 2)
- return 0;
-
- if (i2c_master_recv(client, buffer, 4) 4)
+ struct i2c_msg msgs[2];
+ u8 tx_buf[2], rx_buf[4];
+
+ /* Write register address */
+ tx_buf[0] = addr 8;
+ tx_buf[1] = addr 0xff;
+ msgs[0].addr = client-addr;
+ msgs[0].flags = 0;
+ msgs[0].len = 2;
+ msgs[0].buf = (char *) tx_buf;
+
+ /* Read data from registers */
+ msgs[1].addr = client-addr;
+ msgs[1].flags = I2C_M_RD;
+ msgs[1].len = 4;
+ msgs[1].buf = (char *) rx_buf;
+
+ if (i2c_transfer(client-adapter, msgs, 2) 2)
return 0;
- return (buffer[3] 24) | (buffer[2] 16) |
- (buffer[1] 8) | buffer[0];
+ return (rx_buf[3] 24) | (rx_buf[2] 16) | (rx_buf[1] 8) |
+ rx_buf[0];
}
int cx25840_and_or(struct i2c_client *client, u16 addr, unsigned and_mask,
--
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 04/17] cx25840: Make cx25840 i2c register read transactions atomic
On Tue, 20 Jul 2010 08:35:26 -0400, Andy Walls wrote: On Tue, 2010-07-20 at 08:42 +0200, Jean Delvare wrote: Hi Andy, On Mon, 19 Jul 2010 21:11:46 -0400, Andy Walls wrote: There was a small window between writing the cx25840 register address over the i2c bus and reading the register contents back from the cx25840 device that the i2c adapter lock was released. This change ensures the adapter lock is not released until the register read is done. Signed-off-by: Andy Walls awa...@md.metrocast.net Good catch. Thanks. Acked-by: Jean Delvare kh...@linux-fr.org Note that cx25840_and_or() still has a (smaller and less dangerous) race window. If several calls to cx25840_and_or() happen in parallel on the same register, some of the changes may be lost. I don't know if this can be a problem in practice though. If it is, then additional locking around cx25840_and_or() would be needed. Ah, thank you for pointing that out. So, please bear with me while I think out loud: 1. There are many explicit cases of read-modify-write on a register in the cx25840 module, this is not the only one. 2. The bridge driver historically has always serialized access to the cx25840 module so races have never previously been an issue. 3. I have added a work handler in the cx23885 module that calls the cx25840 module's interrupt handler. Calls by the work handler are serialized with respect to themselves, but not with respect to serialized calls in #2. 4. IIRC, registers written to by the cx25840 interrupt handler are never written to by the other cx25840 module functions; and vice-versa. I will have to audit the cx25840 module code to be sure. 5. There is always a race on r-m-w on *some* registers in the 0x800-0x9ff range with the audio microcontroller that is built into the chip. The only way to provide locking for those is to halt the microcontroller. I've just looked at Patch 12/17 again. The interrupt handler only reads the CX23885_AUD_MC_INT_MASK_REG, which is used by the audio micrcontroller. The interrupt handler does do a r-m-w on the CX25840_AUD_INT_STAT_REG, but that register is not used by the microcontroller. So, I think I'm OK with just not dropping the i2c adapter lock in a cx25840 register read transaction until it is complete. Thanks for making me think that one through. :) Looks like you're OK then, but you may want to document this somewhere for future contributors to these 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 1/2] V4L/DVB: cx88: Move I2C IR initialization
Move I2C IR initialization from just after I2C bus setup to right
before non-I2C IR initialization. This is the same as was done for
the bttv driver several months ago. Might solve bugs which have not yet
been reported for some cards. It makes both drivers consistent, and
makes it easier to disable IR support (coming soon.)
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
drivers/media/video/cx88/cx88-cards.c |1 +
drivers/media/video/cx88/cx88-i2c.c |6 +-
drivers/media/video/cx88/cx88.h |1 +
3 files changed, 7 insertions(+), 1 deletion(-)
--- linux-2.6.34-rc3.orig/drivers/media/video/cx88/cx88-cards.c 2010-04-09
10:55:01.0 +0200
+++ linux-2.6.34-rc3/drivers/media/video/cx88/cx88-cards.c 2010-04-09
17:53:58.0 +0200
@@ -3498,6 +3498,7 @@ struct cx88_core *cx88_core_create(struc
}
cx88_card_setup(core);
+ cx88_i2c_init_ir(core);
cx88_ir_init(core, pci);
return core;
--- linux-2.6.34-rc3.orig/drivers/media/video/cx88/cx88-i2c.c 2010-04-09
14:04:04.0 +0200
+++ linux-2.6.34-rc3/drivers/media/video/cx88/cx88-i2c.c2010-04-09
17:53:58.0 +0200
@@ -181,6 +181,11 @@ int cx88_i2c_init(struct cx88_core *core
} else
printk(%s: i2c register FAILED\n, core-name);
+ return core-i2c_rc;
+}
+
+void cx88_i2c_init_ir(struct cx88_core *core)
+{
/* Instantiate the IR receiver device, if present */
if (0 == core-i2c_rc) {
struct i2c_board_info info;
@@ -196,7 +201,6 @@ int cx88_i2c_init(struct cx88_core *core
i2c_new_probed_device(core-i2c_adap, info, addr_list,
i2c_probe_func_quick_read);
}
- return core-i2c_rc;
}
/* --- */
--- linux-2.6.34-rc3.orig/drivers/media/video/cx88/cx88.h 2010-04-03
18:40:32.0 +0200
+++ linux-2.6.34-rc3/drivers/media/video/cx88/cx88.h2010-04-09
17:53:58.0 +0200
@@ -636,6 +636,7 @@ extern struct videobuf_queue_ops cx8800_
/* cx88-i2c.c */
extern int cx88_i2c_init(struct cx88_core *core, struct pci_dev *pci);
+extern void cx88_i2c_init_ir(struct cx88_core *core);
/* --- */
--
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] V4L/DVB: cx88: Let the user disable IR support
It might be useful to be able to disable the IR support, either for
debugging purposes, or just for users who know they won't use the IR
remote control anyway. On many cards, IR support requires expensive
polling/sampling which is better avoided if never needed.
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
drivers/media/video/cx88/cx88-cards.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
--- linux-2.6.34-rc3.orig/drivers/media/video/cx88/cx88-cards.c 2010-04-09
17:53:58.0 +0200
+++ linux-2.6.34-rc3/drivers/media/video/cx88/cx88-cards.c 2010-04-09
17:54:14.0 +0200
@@ -45,6 +45,10 @@ static unsigned int latency = UNSET;
module_param(latency,int,0444);
MODULE_PARM_DESC(latency,pci latency timer);
+static int disable_ir;
+module_param(disable_ir, int, 0444);
+MODULE_PARM_DESC(latency, Disable IR support);
+
#define info_printk(core, fmt, arg...) \
printk(KERN_INFO %s: fmt, core-name , ## arg)
@@ -3498,8 +3502,10 @@ struct cx88_core *cx88_core_create(struc
}
cx88_card_setup(core);
- cx88_i2c_init_ir(core);
- cx88_ir_init(core, pci);
+ if (!disable_ir) {
+ cx88_i2c_init_ir(core);
+ cx88_ir_init(core, pci);
+ }
return core;
}
--
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 6/8]i2c:i2c_core Fix warning: variable 'dummy' set but not used
On Tue, 15 Jun 2010 09:20:39 -0700, David Daney wrote: On 06/15/2010 04:40 AM, Jean Delvare wrote: __process_new_adapter() calls i2c_do_add_adapter() which always returns 0. Why should I check the return value of bus_for_each_drv() when I know it will always be 0 by construction? Also note that the same function is also called through bus_for_each_dev() somewhere else in i2c-core, and there is no warning there because bus_for_each_dev() is not marked __must_check. How consistent is this? If bus_for_each_dev() is OK without __must_check, then I can't see why bus_for_each_drv() wouldn't be. Well, I would advocate removing the __must_check then. I have just sent a patch to LKML doing exactly 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: [PATCH 7/8]ieee1394/sdp2 Fix warning: variable 'unit_characteristics' set but not used
On Mon, 14 Jun 2010 13:26:47 -0700, Justin P. Mattock wrote: Temporary fix until something is resolved This is wrong by design, sorry. Warnings aren't blocking, and thus need no temporary fix. Such temporary fixes would be only hiding the warning, cancelling the good work of gcc developers. Nack nack nack. to fix the below warning: CC [M] drivers/ieee1394/sbp2.o drivers/ieee1394/sbp2.c: In function 'sbp2_parse_unit_directory': drivers/ieee1394/sbp2.c:1353:6: warning: variable 'unit_characteristics' set but not used Signed-off-by: Justin P. Mattock justinmatt...@gmail.com --- drivers/ieee1394/sbp2.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/ieee1394/sbp2.c b/drivers/ieee1394/sbp2.c index 4565cb5..fcf8bd5 100644 --- a/drivers/ieee1394/sbp2.c +++ b/drivers/ieee1394/sbp2.c @@ -1356,6 +1356,8 @@ static void sbp2_parse_unit_directory(struct sbp2_lu *lu, management_agent_addr = 0; unit_characteristics = 0; + if (!unit_characteristics) + unit_characteristics = 0; firmware_revision = SBP2_ROM_VALUE_MISSING; model = ud-flags UNIT_DIRECTORY_MODEL_ID ? ud-model_id : SBP2_ROM_VALUE_MISSING; -- 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 6/8]i2c:i2c_core Fix warning: variable 'dummy' set but not used
Hi David,
On Mon, 14 Jun 2010 14:28:57 -0700, David Daney wrote:
On 06/14/2010 01:53 PM, Jean Delvare wrote:
Hi Justin,
On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote:
could be a right solution, could be wrong
here is the warning:
CC drivers/i2c/i2c-core.o
drivers/i2c/i2c-core.c: In function 'i2c_register_adapter':
drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used
Signed-off-by: Justin P. Mattockjustinmatt...@gmail.com
---
drivers/i2c/i2c-core.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 1cca263..79c6c26 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter
*adap)
mutex_lock(core_lock);
dummy = bus_for_each_drv(i2c_bus_type, NULL, adap,
__process_new_adapter);
+ if(!dummy)
+ dummy = 0;
One word: scripts/checkpatch.pl
In other news, the above is just plain wrong. First we force people to
read the result of bus_for_each_drv() and then when they do and don't
need the value, gcc complains, so we add one more layer of useless
code, which developers and possibly tools will later wonder and
complain about? I can easily imagine that a static code analyzer would
spot the above code as being a potential bug.
Let's stop this madness now please.
Either __must_check goes away from bus_for_each_drv() and from every
other function which raises this problem, or we must disable that new
type of warning gcc 4.6.0 generates. Depends which warnings we value
more, as we can't sanely have both.
That is the crux of the whole thing. Putting in crap to get rid of the
__must_check warning someone obviously wanted to provoke is just plain
wrong.
__process_new_adapter() calls i2c_do_add_adapter() which always returns
0. Why should I check the return value of bus_for_each_drv() when I
know it will always be 0 by construction?
Also note that the same function is also called through
bus_for_each_dev() somewhere else in i2c-core, and there is no warning
there because bus_for_each_dev() is not marked __must_check. How
consistent is this? If bus_for_each_dev() is OK without __must_check,
then I can't see why bus_for_each_drv() wouldn't be.
I don't know what the answer is, but in addition to your suggestion of
removing the __must_check, you might try:
BUG_ON(dummy != WHAT_IT_SHOULD_BE);
or
if (dummy != WHAT_IT_SHOULD_BE)
panic(nice message here);
Which will never trigger.
or
static inline void i_really_know_what_i_am_doing(int arg)
{
/*
* Trick the compiler because we don't want to
* handle error conditions.
*/
return;
}
..
..
..
i_really_know_what_i_am_doing(dummy);
Which is adding a lot of lines, and might eventually fail when the
compiler becomes smarter (if it isn't already). Thanks but no thanks.
If I really have to chose one of these evils, I'll go for BUG_ON(), at
least the intent is clear and the bloat is minimum.
--
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 6/8]i2c:i2c_core Fix warning: variable 'dummy' set but not used
Hi Justin, On Mon, 14 Jun 2010 14:06:12 -0700, Justin P. Mattock wrote: On 06/14/2010 01:53 PM, Jean Delvare wrote: Hi Justin, On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: could be a right solution, could be wrong here is the warning: CC drivers/i2c/i2c-core.o drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used Signed-off-by: Justin P. Mattockjustinmatt...@gmail.com --- drivers/i2c/i2c-core.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 1cca263..79c6c26 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) mutex_lock(core_lock); dummy = bus_for_each_drv(i2c_bus_type, NULL, adap, __process_new_adapter); + if(!dummy) + dummy = 0; One word: scripts/checkpatch.pl it was this, and/or just take the code out (since I'm a newbie) I was not (yet) arguing on the code itself, but on its format. Any patch you send should pass the formatting tests performed by scripts/checkpatch.pl. 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 6/8]i2c:i2c_core Fix warning: variable 'dummy' set but not used
Hi Justin, On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: could be a right solution, could be wrong here is the warning: CC drivers/i2c/i2c-core.o drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used Signed-off-by: Justin P. Mattock justinmatt...@gmail.com --- drivers/i2c/i2c-core.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 1cca263..79c6c26 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) mutex_lock(core_lock); dummy = bus_for_each_drv(i2c_bus_type, NULL, adap, __process_new_adapter); + if(!dummy) + dummy = 0; One word: scripts/checkpatch.pl In other news, the above is just plain wrong. First we force people to read the result of bus_for_each_drv() and then when they do and don't need the value, gcc complains, so we add one more layer of useless code, which developers and possibly tools will later wonder and complain about? I can easily imagine that a static code analyzer would spot the above code as being a potential bug. Let's stop this madness now please. Either __must_check goes away from bus_for_each_drv() and from every other function which raises this problem, or we must disable that new type of warning gcc 4.6.0 generates. Depends which warnings we value more, as we can't sanely have both. mutex_unlock(core_lock); 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 2/2] V4L/DVB: Use custom I2C probing function mechanism
Hi Wolfram, On Wed, 9 Jun 2010 17:05:40 +0200, Wolfram Sang wrote: Hi Jean, On Tue, Jun 08, 2010 at 10:01:00AM +0200, Jean Delvare wrote: Now that i2c-core offers the possibility to provide custom probing function for I2C devices, let's make use of it. Signed-off-by: Jean Delvare kh...@linux-fr.org Acked-by: Mauro Carvalho Chehab mche...@redhat.com If this custom function is in i2c-core, maybe it should be documented? What kind of documentation would you expect for a one-line function? Where, and aimed at who? Patch welcome ;) -- 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] i2c: Add support for custom probe function
The probe method used by i2c_new_probed_device() may not be suitable
for all cases. Let the caller provide its own, optional probe
function.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Acked-by: Mauro Carvalho Chehab mche...@redhat.com
---
Documentation/i2c/instantiating-devices |2 +-
drivers/i2c/i2c-core.c| 16 ++--
drivers/macintosh/therm_windtunnel.c |4 ++--
drivers/media/video/bt8xx/bttv-i2c.c |2 +-
drivers/media/video/cx18/cx18-i2c.c |3 ++-
drivers/media/video/em28xx/em28xx-cards.c |2 +-
drivers/media/video/ivtv/ivtv-i2c.c |9 +
drivers/media/video/v4l2-common.c |3 ++-
drivers/usb/host/ohci-pnx4008.c |2 +-
drivers/video/matrox/i2c-matroxfb.c |2 +-
include/linux/i2c.h |7 +--
11 files changed, 31 insertions(+), 21 deletions(-)
--- linux-2.6.35-rc1.orig/drivers/i2c/i2c-core.c2010-06-02
10:22:57.0 +0200
+++ linux-2.6.35-rc1/drivers/i2c/i2c-core.c 2010-06-02 18:41:29.0
+0200
@@ -1456,14 +1456,18 @@ static int i2c_detect(struct i2c_adapter
struct i2c_client *
i2c_new_probed_device(struct i2c_adapter *adap,
struct i2c_board_info *info,
- unsigned short const *addr_list)
+ unsigned short const *addr_list,
+ int (*probe)(struct i2c_adapter *, unsigned short addr))
{
int i;
- /* Stop here if the bus doesn't support probing */
- if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_BYTE)) {
- dev_err(adap-dev, Probing not supported\n);
- return NULL;
+ if (!probe) {
+ /* Stop here if the bus doesn't support probing */
+ if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_BYTE)) {
+ dev_err(adap-dev, Probing not supported\n);
+ return NULL;
+ }
+ probe = i2c_default_probe;
}
for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) {
@@ -1482,7 +1486,7 @@ i2c_new_probed_device(struct i2c_adapter
}
/* Test address responsiveness */
- if (i2c_default_probe(adap, addr_list[i]))
+ if (probe(adap, addr_list[i]))
break;
}
--- linux-2.6.35-rc1.orig/drivers/macintosh/therm_windtunnel.c 2010-05-31
09:59:27.0 +0200
+++ linux-2.6.35-rc1/drivers/macintosh/therm_windtunnel.c 2010-06-02
18:41:29.0 +0200
@@ -322,10 +322,10 @@ do_attach( struct i2c_adapter *adapter )
memset(info, 0, sizeof(struct i2c_board_info));
strlcpy(info.type, therm_ds1775, I2C_NAME_SIZE);
- i2c_new_probed_device(adapter, info, scan_ds1775);
+ i2c_new_probed_device(adapter, info, scan_ds1775, NULL);
strlcpy(info.type, therm_adm1030, I2C_NAME_SIZE);
- i2c_new_probed_device(adapter, info, scan_adm1030);
+ i2c_new_probed_device(adapter, info, scan_adm1030, NULL);
if( x.thermostat x.fan ) {
x.running = 1;
--- linux-2.6.35-rc1.orig/drivers/media/video/bt8xx/bttv-i2c.c 2010-05-30
15:37:17.0 +0200
+++ linux-2.6.35-rc1/drivers/media/video/bt8xx/bttv-i2c.c 2010-06-02
18:41:29.0 +0200
@@ -411,7 +411,7 @@ void __devinit init_bttv_i2c_ir(struct b
memset(info, 0, sizeof(struct i2c_board_info));
strlcpy(info.type, ir_video, I2C_NAME_SIZE);
- i2c_new_probed_device(btv-c.i2c_adap, info, addr_list);
+ i2c_new_probed_device(btv-c.i2c_adap, info, addr_list, NULL);
}
}
--- linux-2.6.35-rc1.orig/drivers/media/video/cx18/cx18-i2c.c 2010-05-31
09:59:28.0 +0200
+++ linux-2.6.35-rc1/drivers/media/video/cx18/cx18-i2c.c2010-06-02
18:41:29.0 +0200
@@ -117,7 +117,8 @@ static int cx18_i2c_new_ir(struct cx18 *
break;
}
- return i2c_new_probed_device(adap, info, addr_list) == NULL ? -1 : 0;
+ return i2c_new_probed_device(adap, info, addr_list, NULL) == NULL ?
+ -1 : 0;
}
int cx18_i2c_register(struct cx18 *cx, unsigned idx)
--- linux-2.6.35-rc1.orig/drivers/media/video/em28xx/em28xx-cards.c
2010-05-31 09:59:28.0 +0200
+++ linux-2.6.35-rc1/drivers/media/video/em28xx/em28xx-cards.c 2010-06-02
18:41:29.0 +0200
@@ -2357,7 +2357,7 @@ void em28xx_register_i2c_ir(struct em28x
if (dev-init_data.name)
info.platform_data = dev-init_data;
- i2c_new_probed_device(dev-i2c_adap, info, addr_list);
+ i2c_new_probed_device(dev-i2c_adap, info, addr_list, NULL);
}
void em28xx_card_setup(struct em28xx *dev)
--- linux-2.6.35-rc1.orig/drivers/media/video/ivtv/ivtv-i2c.c 2010-05-31
09:59:28.0 +0200
+++ linux-2.6.35-rc1/drivers/media/video/ivtv
[PATCH 2/2] V4L/DVB: Use custom I2C probing function mechanism
Now that i2c-core offers the possibility to provide custom probing
function for I2C devices, let's make use of it.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Acked-by: Mauro Carvalho Chehab mche...@redhat.com
---
drivers/i2c/i2c-core.c|7 +++
drivers/media/video/cx23885/cx23885-i2c.c | 15 ---
drivers/media/video/cx88/cx88-i2c.c | 19 ---
include/linux/i2c.h |3 +++
4 files changed, 18 insertions(+), 26 deletions(-)
--- linux-2.6.35-rc2.orig/drivers/media/video/cx23885/cx23885-i2c.c
2010-06-07 16:12:38.0 +0200
+++ linux-2.6.35-rc2/drivers/media/video/cx23885/cx23885-i2c.c 2010-06-08
09:17:06.0 +0200
@@ -365,17 +365,10 @@ int cx23885_i2c_register(struct cx23885_
memset(info, 0, sizeof(struct i2c_board_info));
strlcpy(info.type, ir_video, I2C_NAME_SIZE);
- /*
-* 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);
- }
+ /* Use quick read command for probe, some IR chips don't
+* support writes */
+ i2c_new_probed_device(bus-i2c_adap, info, addr_list,
+ i2c_probe_func_quick_read);
}
return bus-i2c_rc;
--- linux-2.6.35-rc2.orig/drivers/media/video/cx88/cx88-i2c.c 2010-06-07
16:12:38.0 +0200
+++ linux-2.6.35-rc2/drivers/media/video/cx88/cx88-i2c.c2010-06-08
09:17:06.0 +0200
@@ -188,24 +188,13 @@ 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);
- /*
-* 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;
- }
- }
+ /* Use quick read command for probe, some IR chips don't
+* support writes */
+ i2c_new_probed_device(core-i2c_adap, info, addr_list,
+ i2c_probe_func_quick_read);
}
return core-i2c_rc;
}
--- linux-2.6.35-rc2.orig/drivers/i2c/i2c-core.c2010-06-07
16:12:38.0 +0200
+++ linux-2.6.35-rc2/drivers/i2c/i2c-core.c 2010-06-08 09:17:06.0
+0200
@@ -1453,6 +1453,13 @@ static int i2c_detect(struct i2c_adapter
return err;
}
+int i2c_probe_func_quick_read(struct i2c_adapter *adap, unsigned short addr)
+{
+ return i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+ I2C_SMBUS_QUICK, NULL) = 0;
+}
+EXPORT_SYMBOL_GPL(i2c_probe_func_quick_read);
+
struct i2c_client *
i2c_new_probed_device(struct i2c_adapter *adap,
struct i2c_board_info *info,
--- linux-2.6.35-rc2.orig/include/linux/i2c.h 2010-06-07 16:15:10.0
+0200
+++ linux-2.6.35-rc2/include/linux/i2c.h2010-06-08 09:19:07.0
+0200
@@ -292,6 +292,9 @@ i2c_new_probed_device(struct i2c_adapter
unsigned short const *addr_list,
int (*probe)(struct i2c_adapter *, unsigned short addr));
+/* Common custom probe functions */
+extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short
addr);
+
/* For devices that use several addresses, use i2c_new_dummy() to make
* client handles for the extra addresses.
*/
--
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 RESEND] FusionHDTV: Use quick reads for I2C IR device probing
Mauro,
On Wed, 26 May 2010 15:05:11 +0200, 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. I had already sent on March 29th, but apparently it was
overlooked. I have further i2c patches which depend on this one, so
please process it quickly, otherwise I'll have to push it myself.
This fix is still not upstream! What do I have to do to get it there?
Please!
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
Re: [PATCH] drivers: remove all i2c_set_clientdata(client, NULL)
On Mon, 31 May 2010 14:55:48 +0200, Wolfram Sang wrote: I2C-drivers can use the clientdata-pointer to point to private data. As I2C devices are not really unregistered, but merely detached from their driver, it used to be the drivers obligation to clear this pointer during remove() or a failed probe(). As a couple of drivers forgot to do this, it was agreed that it was cleaner if the i2c-core does this clearance when appropriate, as there is no guarantee for the lifetime of the clientdata-pointer after remove() anyhow. This feature was added to the core with commit e4a7b9b04de15f6b63da5ccdd373ffa3057a3681 to fix the faulty drivers. As there is no need anymore to clear the clientdata-pointer, remove all current occurrences in the drivers to simplify the code and prevent confusion. Signed-off-by: Wolfram Sang w.s...@pengutronix.de Cc: Jean Delvare kh...@linux-fr.org (...) Applied, thanks. Will go to Linus in the next few days. -- 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] drivers: remove all i2c_set_clientdata(client, NULL)
Hi Dmitry, On Mon, 31 May 2010 12:09:12 -0700, Dmitry Torokhov wrote: Frankly I'd prefer taking input stuff through my tree with the goal of .36 merge window just to minimize potential merge issues. This is a simple cleanup patch that has no dependencies, so there is little gain from doing it all in one go. If I take the patch in my i2c tree, the aim is to merge it upstream immediately, so merge issues won't exist. -- 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] video/saa7134: potential null dereferences in debug code
On Sat, 29 May 2010 01:29:54 -0300, Mauro Carvalho Chehab wrote: Em Sat, 22 May 2010 22:59:21 +0200 Jean Delvare kh...@linux-fr.org escreveu: I would have used (null) instead of null for consistency with lib/vsprintf.c:string(). But more importantly, I suspect that a better fix would be to not call these macros when dev or ir, respectively, is NULL. The faulty dprintk calls in get_key_msi_tvanywhere_plus() and get_key_flydvb_trio() could be replaced with i2cdprintk (which is misnamed IMHO, BTW.) Agreed. Dan, could you please rework your patch according with Jean's feedback? He did already. -- 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 v3 1/2] video/saa7134: change dprintk() to i2cdprintk()
Hi Dan,
On Tue, 25 May 2010 11:19:53 +0200, Dan Carpenter wrote:
The problem is that dprintk() dereferences dev which is null here.
The i2cdprintk() uses ir so that's OK.
Signed-off-by: Dan Carpenter erro...@gmail.com
---
v2: Jean Delvare suggested that I use i2cdprintk() instead of modifying
dprintk().
v3: V2 had a bonus cleanup that I removed from v3
diff --git a/drivers/media/video/saa7134/saa7134-input.c
b/drivers/media/video/saa7134/saa7134-input.c
index e5565e2..7691bf2 100644
--- a/drivers/media/video/saa7134/saa7134-input.c
+++ b/drivers/media/video/saa7134/saa7134-input.c
@@ -141,8 +141,8 @@ static int get_key_flydvb_trio(struct IR_i2c *ir, u32
*ir_key, u32 *ir_raw)
struct saa7134_dev *dev = ir-c-adapter-algo_data;
if (dev == NULL) {
- dprintk(get_key_flydvb_trio:
- gir-c-adapter-algo_data is NULL!\n);
+ i2cdprintk(get_key_flydvb_trio:
+gir-c-adapter-algo_data is NULL!\n);
Sorry for noticing only now, but gir- in the comment is odd. As seen
in the code above, it's actually ir-. Maybe you want to fix this, as
you are already touching that line anyway.
Other than this, this patch is:
Acked-by: Jean Delvare kh...@linux-fr.org
return -EIO;
}
@@ -195,8 +195,8 @@ static int get_key_msi_tvanywhere_plus(struct IR_i2c *ir,
u32 *ir_key,
/* dev is needed to access GPIO. Used by the saa_readl macro. */
struct saa7134_dev *dev = ir-c-adapter-algo_data;
if (dev == NULL) {
- dprintk(get_key_msi_tvanywhere_plus:
- gir-c-adapter-algo_data is NULL!\n);
+ i2cdprintk(get_key_msi_tvanywhere_plus:
+gir-c-adapter-algo_data is NULL!\n);
return -EIO;
}
--
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 v3 2/2] video/saa7134: remove duplicate break
On Tue, 25 May 2010 11:21:50 +0200, Dan Carpenter wrote:
The original code had two break statements in a row.
Signed-off-by: Dan Carpenter erro...@gmail.com
Acked-by: Jean Delvare kh...@linux-fr.org
---
v3: Put this in a seperate patch.
diff --git a/drivers/media/video/saa7134/saa7134-input.c
b/drivers/media/video/saa7134/saa7134-input.c
index e5565e2..7691bf2 100644
--- a/drivers/media/video/saa7134/saa7134-input.c
+++ b/drivers/media/video/saa7134/saa7134-input.c
@@ -815,7 +815,6 @@ int saa7134_input_init1(struct saa7134_dev *dev)
mask_keyup = 0x02;
polling = 50; /* ms */
break;
- break;
}
if (NULL == ir_codes) {
printk(%s: Oops: IR config error [card=%d]\n,
--
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 RESEND] FusionHDTV: Use quick reads for I2C IR device probing
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. I had already sent on March 29th, but apparently it was
overlooked. I have further i2c patches which depend on this one, so
please process it quickly, otherwise I'll have to push it myself.
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-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
Re: [patch] video/saa7134: potential null dereferences in debug code
Hi Dan, On Sat, 22 May 2010 22:15:35 +0200, Dan Carpenter wrote: I modified the dprintk and i2cdprintk macros to handle null dev and ir pointers. There are two couple places that call dprintk() when dev is null. One is in get_key_msi_tvanywhere_plus() and the other is in get_key_flydvb_trio(). Signed-off-by: Dan Carpenter erro...@gmail.com diff --git a/drivers/media/video/saa7134/saa7134-input.c b/drivers/media/video/saa7134/saa7134-input.c index e5565e2..e14f2f8 100644 --- a/drivers/media/video/saa7134/saa7134-input.c +++ b/drivers/media/video/saa7134/saa7134-input.c @@ -61,9 +61,9 @@ MODULE_PARM_DESC(disable_other_ir, disable full codes of alternative remotes from other manufacturers); #define dprintk(fmt, arg...) if (ir_debug) \ - printk(KERN_DEBUG %s/ir: fmt, dev-name , ## arg) + printk(KERN_DEBUG %s/ir: fmt, dev ? dev-name : null, ## arg) #define i2cdprintk(fmt, arg...)if (ir_debug) \ - printk(KERN_DEBUG %s/ir: fmt, ir-name , ## arg) + printk(KERN_DEBUG %s/ir: fmt, ir ? ir-name : null, ## arg) /* Helper functions for RC5 and NEC decoding at GPIO16 or GPIO18 */ static int saa7134_rc5_irq(struct saa7134_dev *dev); I would have used (null) instead of null for consistency with lib/vsprintf.c:string(). But more importantly, I suspect that a better fix would be to not call these macros when dev or ir, respectively, is NULL. The faulty dprintk calls in get_key_msi_tvanywhere_plus() and get_key_flydvb_trio() could be replaced with i2cdprintk (which is misnamed IMHO, BTW.) -- 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/2] V4L/DVB: Use custom I2C probing function mechanism
Hi Mauro, On Fri, 09 Apr 2010 01:09:08 -0300, Mauro Carvalho Chehab wrote: Jean Delvare wrote: There are no other probing functions yet, this is the first one. I have added the mechanism to i2c-core for these very IR chips. Putting all probe functions together would mean moving them to i2c-core. This wasn't my original intent, but after all, it makes some sense. Would you be happy with the following? It seems fine for me. As you're touching on i2c core and on other drivers on this series, I think that the better is if you could apply it on your tree. Yes, this is the plan. However, before I can add them to my tree, patch named: [PATCH] FusionHDTV: Use quick reads for I2C IR device probing which I posted to the linux-media mailing list some weeks ago must go upstream. Otherwise these 2 patches do not apply cleanly. For both patches 1 and 2: Acked-by: Mauro Carvalho Chehab mche...@redhat.com 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 2/2] V4L/DVB: Use custom I2C probing function mechanism
Hi Mauro,
On Tue, 06 Apr 2010 02:34:46 -0300, Mauro Carvalho Chehab wrote:
Jean Delvare wrote:
On Mon, 05 Apr 2010 15:26:32 -0300, Mauro Carvalho Chehab wrote:
Please, don't add new things at ir-common module. It basically contains the
decoding functions for RC5 and pulse/distance, plus several IR keymaps.
With
the IR rework I'm doing, this module will go away, after having all the
current
IR decoders implemented via ir-raw-input binding.
The keymaps were already removed from it, on my experimental tree
(http://git.linuxtv.org/mchehab/ir.git), and rc5 decoder is already written
(but still needs a few fixes).
The new ir-core is creating an abstract way to deal with Remote
Controllers,
meant to be used not only by IR's, but also for other types of RC, like,
bluetooth and USB HID. It will also export a raw event interface, for use
with lirc. As this is the core of the RC subsystem, a i2c-specific binding
method also doesn't seem to belong there. SO, IMO, the better place is to
add
it as a static inline function at ir-kbd-i2c.h.
Ever tried to pass the address of an inline function as another
function's parameter? :)
:) Never tried... maybe gcc would to the hard thing, de-inlining it ;)
Well, we need to put this code somewhere. Where are the other probing
codes? Probably the better is to put them together.
There are no other probing functions yet, this is the first one. I have
added the mechanism to i2c-core for these very IR chips.
Putting all probe functions together would mean moving them to
i2c-core. This wasn't my original intent, but after all, it makes some
sense. Would you be happy with the following?
* * * * *
From: Jean Delvare kh...@linux-fr.org
Subject: V4L/DVB: Use custom I2C probing function mechanism
Now that i2c-core offers the possibility to provide custom probing
function for I2C devices, let's make use of it.
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
drivers/i2c/i2c-core.c|7 +++
drivers/media/video/cx23885/cx23885-i2c.c | 15 ---
drivers/media/video/cx88/cx88-i2c.c | 19 ---
include/linux/i2c.h |3 +++
4 files changed, 18 insertions(+), 26 deletions(-)
--- linux-2.6.34-rc3.orig/drivers/media/video/cx23885/cx23885-i2c.c
2010-04-06 11:31:20.0 +0200
+++ linux-2.6.34-rc3/drivers/media/video/cx23885/cx23885-i2c.c 2010-04-06
12:28:09.0 +0200
@@ -365,17 +365,10 @@ int cx23885_i2c_register(struct cx23885_
memset(info, 0, sizeof(struct i2c_board_info));
strlcpy(info.type, ir_video, I2C_NAME_SIZE);
- /*
-* 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);
- }
+ /* Use quick read command for probe, some IR chips don't
+* support writes */
+ i2c_new_probed_device(bus-i2c_adap, info, addr_list,
+ i2c_probe_func_quick_read);
}
return bus-i2c_rc;
--- linux-2.6.34-rc3.orig/drivers/media/video/cx88/cx88-i2c.c 2010-04-06
11:31:20.0 +0200
+++ linux-2.6.34-rc3/drivers/media/video/cx88/cx88-i2c.c2010-04-06
12:28:06.0 +0200
@@ -188,24 +188,13 @@ 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);
- /*
-* 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;
- }
- }
+ /* Use quick read command for probe, some IR chips don't
+* support writes */
+ i2c_new_probed_device(core-i2c_adap, info, addr_list
Re: [PATCH 2/2] V4L/DVB: Use custom I2C probing function mechanism
Hi Andy, On Sun, 04 Apr 2010 21:54:39 -0400, Andy Walls wrote: On Sun, 2010-04-04 at 16:14 +0200, Jean Delvare wrote: Now that i2c-core offers the possibility to provide custom probing function for I2C devices, let's make use of it. Signed-off-by: Jean Delvare kh...@linux-fr.org --- I wasn't too sure where to put the custom probe function: in each driver, in the ir-common module or in the v4l2-common module. I went for the second option as a middle ground, but am ready to discuss it if anyone objects. With respect to cx23885, could you comment on the interaction of this patch with some patches of yours that are not merged yet: http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/b39f8849a35b http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/3cf1ac545ca5 http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/ef5d2c08106f Are they related to the IR microcontroller not being probed properly? No, I don't expect any interaction between this new patch and the older patchset. The older patchset would let the cx23885 I2C implementation properly report slave nacks, but a successful IR device probing wouldn't return a nack. So, the patches can be merged in any order, nothing wrong will happen either way. (I tried to get these patches merged, but didn't due to problems with other patches in my PULL request, and then a severe shortage of time.) 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 2/2] V4L/DVB: Use custom I2C probing function mechanism
Hi Mauro, On Mon, 05 Apr 2010 15:26:32 -0300, Mauro Carvalho Chehab wrote: Jean Delvare wrote: Now that i2c-core offers the possibility to provide custom probing function for I2C devices, let's make use of it. Signed-off-by: Jean Delvare kh...@linux-fr.org --- I wasn't too sure where to put the custom probe function: in each driver, in the ir-common module or in the v4l2-common module. I went for the second option as a middle ground, but am ready to discuss it if anyone objects. Please, don't add new things at ir-common module. It basically contains the decoding functions for RC5 and pulse/distance, plus several IR keymaps. With the IR rework I'm doing, this module will go away, after having all the current IR decoders implemented via ir-raw-input binding. The keymaps were already removed from it, on my experimental tree (http://git.linuxtv.org/mchehab/ir.git), and rc5 decoder is already written (but still needs a few fixes). The new ir-core is creating an abstract way to deal with Remote Controllers, meant to be used not only by IR's, but also for other types of RC, like, bluetooth and USB HID. It will also export a raw event interface, for use with lirc. As this is the core of the RC subsystem, a i2c-specific binding method also doesn't seem to belong there. SO, IMO, the better place is to add it as a static inline function at ir-kbd-i2c.h. Ever tried to pass the address of an inline function as another function's parameter? :) -- 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] i2c: Add support for custom probe function
The probe method used by i2c_new_probed_device() may not be suitable
for all cases. Let the caller provide its own, optional probe
function.
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
Documentation/i2c/instantiating-devices |2
drivers/i2c/i2c-core.c| 65 -
drivers/macintosh/therm_windtunnel.c |4 -
drivers/media/video/bt8xx/bttv-i2c.c |2
drivers/media/video/cx18/cx18-i2c.c |3 -
drivers/media/video/em28xx/em28xx-cards.c |2
drivers/media/video/ivtv/ivtv-i2c.c |9 ++--
drivers/media/video/v4l2-common.c |3 -
drivers/usb/host/ohci-pnx4008.c |2
drivers/video/matrox/i2c-matroxfb.c |2
include/linux/i2c.h |7 ++-
11 files changed, 58 insertions(+), 43 deletions(-)
--- linux-2.6.34-rc3.orig/drivers/i2c/i2c-core.c2010-04-03
21:09:36.0 +0200
+++ linux-2.6.34-rc3/drivers/i2c/i2c-core.c 2010-04-04 13:28:17.0
+0200
@@ -1376,17 +1376,46 @@ static int i2c_detect(struct i2c_adapter
return err;
}
+/*
+ * Legacy default probe function, mostly relevant for SMBus. The default
+ * probe method is a quick write, but it is known to corrupt the 24RF08
+ * EEPROMs due to a state machine bug, and could also irreversibly
+ * write-protect some EEPROMs, so for address ranges 0x30-0x37 and 0x50-0x5f,
+ * we use a byte read instead. Also, some bus drivers don't implement quick
+ * write, so we fallback to a byte read in that case too.
+ * Returns 1 if probe succeeded, 0 if not.
+ */
+static int i2c_default_probe(struct i2c_adapter *adap, unsigned short addr)
+{
+ int err;
+ union i2c_smbus_data dummy;
+
+ if ((addr ~0x07) == 0x30 || (addr ~0x0f) == 0x50
+|| !i2c_check_functionality(adap, I2C_FUNC_SMBUS_QUICK))
+ err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+I2C_SMBUS_BYTE, dummy);
+ else
+ err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, 0,
+I2C_SMBUS_QUICK, NULL);
+
+ return err = 0;
+}
+
struct i2c_client *
i2c_new_probed_device(struct i2c_adapter *adap,
struct i2c_board_info *info,
- unsigned short const *addr_list)
+ unsigned short const *addr_list,
+ int (*probe)(struct i2c_adapter *, unsigned short addr))
{
int i;
- /* Stop here if the bus doesn't support probing */
- if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_BYTE)) {
- dev_err(adap-dev, Probing not supported\n);
- return NULL;
+ if (!probe) {
+ /* Stop here if the bus doesn't support probing */
+ if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_BYTE)) {
+ dev_err(adap-dev, Probing not supported\n);
+ return NULL;
+ }
+ probe = i2c_default_probe;
}
for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) {
@@ -1404,29 +1433,9 @@ i2c_new_probed_device(struct i2c_adapter
continue;
}
- /* Test address responsiveness
- The default probe method is a quick write, but it is known
- to corrupt the 24RF08 EEPROMs due to a state machine bug,
- and could also irreversibly write-protect some EEPROMs, so
- for address ranges 0x30-0x37 and 0x50-0x5f, we use a byte
- read instead. Also, some bus drivers don't implement
- quick write, so we fallback to a byte read it that case
- too. */
- if ((addr_list[i] ~0x07) == 0x30
-|| (addr_list[i] ~0x0f) == 0x50
-|| !i2c_check_functionality(adap, I2C_FUNC_SMBUS_QUICK)) {
- union i2c_smbus_data data;
-
- if (i2c_smbus_xfer(adap, addr_list[i], 0,
- I2C_SMBUS_READ, 0,
- I2C_SMBUS_BYTE, data) = 0)
- break;
- } else {
- if (i2c_smbus_xfer(adap, addr_list[i], 0,
- I2C_SMBUS_WRITE, 0,
- I2C_SMBUS_QUICK, NULL) = 0)
- break;
- }
+ /* Test address responsiveness */
+ if (probe(adap, addr_list[i]))
+ break;
}
if (addr_list[i] == I2C_CLIENT_END) {
--- linux-2.6.34-rc3.orig/drivers/macintosh/therm_windtunnel.c 2010-03-20
14:27:51.0 +0100
+++ linux-2.6.34-rc3/drivers/macintosh/therm_windtunnel.c 2010-04-04
10:12:14.0 +0200
@@ -323,10 +323,10 @@ do_attach( struct i2c_adapter *adapter )
memset
[PATCH 2/2] V4L/DVB: Use custom I2C probing function mechanism
Now that i2c-core offers the possibility to provide custom probing
function for I2C devices, let's make use of it.
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
I wasn't too sure where to put the custom probe function: in each driver,
in the ir-common module or in the v4l2-common module. I went for the
second option as a middle ground, but am ready to discuss it if anyone
objects.
drivers/media/IR/ir-functions.c | 12
drivers/media/video/cx23885/cx23885-i2c.c | 14 +++---
drivers/media/video/cx88/cx88-i2c.c | 18 +++---
include/media/ir-common.h |5 +
4 files changed, 23 insertions(+), 26 deletions(-)
--- linux-2.6.34-rc3.orig/drivers/media/video/cx23885/cx23885-i2c.c
2010-04-04 09:06:38.0 +0200
+++ linux-2.6.34-rc3/drivers/media/video/cx23885/cx23885-i2c.c 2010-04-04
13:34:34.0 +0200
@@ -28,6 +28,7 @@
#include cx23885.h
#include media/v4l2-common.h
+#include media/ir-common.h
static unsigned int i2c_debug;
module_param(i2c_debug, int, 0644);
@@ -365,17 +366,8 @@ int cx23885_i2c_register(struct cx23885_
memset(info, 0, sizeof(struct i2c_board_info));
strlcpy(info.type, ir_video, I2C_NAME_SIZE);
- /*
-* 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);
- }
+ i2c_new_probed_device(bus-i2c_adap, info, addr_list,
+ ir_i2c_probe);
}
return bus-i2c_rc;
--- linux-2.6.34-rc3.orig/drivers/media/video/cx88/cx88-i2c.c 2010-04-04
09:06:38.0 +0200
+++ linux-2.6.34-rc3/drivers/media/video/cx88/cx88-i2c.c2010-04-04
13:34:34.0 +0200
@@ -34,6 +34,7 @@
#include cx88.h
#include media/v4l2-common.h
+#include media/ir-common.h
static unsigned int i2c_debug;
module_param(i2c_debug, int, 0644);
@@ -188,24 +189,11 @@ 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);
- /*
-* 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;
- }
- }
+ i2c_new_probed_device(core-i2c_adap, info, addr_list,
+ ir_i2c_probe);
}
return core-i2c_rc;
}
--- linux-2.6.34-rc3.orig/drivers/media/IR/ir-functions.c 2010-03-18
17:06:30.0 +0100
+++ linux-2.6.34-rc3/drivers/media/IR/ir-functions.c2010-04-04
14:30:29.0 +0200
@@ -23,6 +23,7 @@
#include linux/module.h
#include linux/string.h
#include linux/jiffies.h
+#include linux/i2c.h
#include media/ir-common.h
/* --
*/
@@ -353,3 +354,14 @@ void ir_rc5_timer_keyup(unsigned long da
ir_input_nokey(ir-dev, ir-ir);
}
EXPORT_SYMBOL_GPL(ir_rc5_timer_keyup);
+
+/* Some functions only needed for I2C devices */
+#if defined CONFIG_I2C || defined CONFIG_I2C_MODULE
+/* use quick read command for probe, some IR chips don't support writes */
+int ir_i2c_probe(struct i2c_adapter *i2c, unsigned short addr)
+{
+ return i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0,
+ I2C_SMBUS_QUICK, NULL) = 0;
+}
+EXPORT_SYMBOL_GPL(ir_i2c_probe);
+#endif
--- linux-2.6.34-rc3.orig/include/media/ir-common.h 2010-03-18
17:06:30.0 +0100
+++ linux-2.6.34-rc3/include/media/ir-common.h 2010-04-04 14:29:54.0
+0200
@@ -97,6 +97,11 @@ u32 ir_rc5_decode(unsigned int code);
void ir_rc5_timer_end(unsigned long data);
void ir_rc5_timer_keyup(unsigned long data);
+#if defined CONFIG_I2C || defined CONFIG_I2C_MODULE
+struct i2c_adapter;
+int ir_i2c_probe(struct i2c_adapter *i2c, unsigned short addr
cx88 remote control event device
Hi Andrzej, Last year, you submitted a fix for the cx88 remote control not behaving properly on some cards. The fix works fine for me and lets me use my remote control, and I am very grateful for this. However, I have noticed (using powertop) that the cx88 driver is waking up the kernel 1250 times per second to handle the remote control. I understand that it is needed for proper operation when the remote control is in use. What I do not understand is why it still happens when nobody uses the remote control. Even when no application has the event device node opened, polling still happens. Can't we have the cx88 driver poll the remote control only when the device node is opened? I believe this would save some power by allowing the CPU to stay in higher C states. 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: cx88 remote control event device
Hi Mauro, On Wed, 31 Mar 2010 14:46:37 -0300, Mauro Carvalho Chehab wrote: Hi Jean, Jean Delvare wrote: Hi Andrzej, Last year, you submitted a fix for the cx88 remote control not behaving properly on some cards. The fix works fine for me and lets me use my remote control, and I am very grateful for this. However, I have noticed (using powertop) that the cx88 driver is waking up the kernel 1250 times per second to handle the remote control. I understand that it is needed for proper operation when the remote control is in use. What I do not understand is why it still happens when nobody uses the remote control. Even when no application has the event device node opened, polling still happens. Can't we have the cx88 driver poll the remote control only when the device node is opened? I believe this would save some power by allowing the CPU to stay in higher C states. The IR can be used even when nobody is opening the /dev/video device, as it is an input device that can be used to control other things, including the start of the video application. That's said, it makes sense to only enable the polling when the /dev/input/event device is opened. Sorry for not being clear originally; this is exactly what I meant. Btw, the same polling logic is also present on bttv and saa7134 drivers. As I'm doing a large IR rework, with the addition of the IR core subsystem, and the patch for handing the open/close is very simple, I've already wrote a patch for saa7134, on my IR tree: http://git.linuxtv.org/mchehab/ir.git?a=commitdiff;h=2b1d3acdb48266f05b82923b8db06e6c7ada0c72 The change itself is very simple, although I've added some additional checks to avoid the risk of having an IRQ while IR is disabled. Looks very good, nice to see someone is already working on the problem. I have one cx88 board on my IR test machine (although I need to find the IR sensor for the board I'm using there). If I find one that works, I'll try later to write a similar code to cx88. 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] 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
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 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: [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
[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] 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
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: [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: [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: [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: 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
[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: [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] 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 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
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: 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: 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: 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
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: [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 Receiver on an Tevii S470
On Sun, 22 Nov 2009 19:17:59 -0500, Andy Walls wrote: On Sun, 2009-11-22 at 21:32 +0100, Jean Delvare wrote: The fact that 0x30-0x37 and 0x50-0x5f all reply suggest that the bus driver erroneously returns success to SMBus receive byte transactions even when no device acks. This is a bug which should get fixed. If you point me to the I2C adapter driver code, I can take a look. Although Igor's information makes the original need for this moot, here is the i2c adapter driver code: http://linuxtv.org/hg/v4l-dvb/file/8bff7e6c44d4/linux/drivers/media/video/cx23885/cx23885-i2c.c The results are not surprising: i2c_slave_did_ack() is only called for zero-length transactions. For all other transactions, no check is done. This is incorrect. I have written 3 patches for cx23885-i2c.c, the second one should fix this particular issue. The other two are cleanups. Patches are there if you want to take a look / give them a try: http://khali.linux-fr.org/devel/misc/cx23885/ These are totally untested, and I don't know anything about the hardware, so they might need some more work. But at least you should get the idea of what's missing. Note the CX2388[578] chips have 3 I2C masters, 2 for external buses, and 1 internal on silicon bus which the driver sets up as the 3rd bus. The internal bus should at least have devices at 0x44 and 0x4c as confirmed above. I'll note the comment in this file, that indicates the on silicon I2C bus runs at 1.95 MHz: http://linuxtv.org/hg/v4l-dvb/file/8bff7e6c44d4/linux/drivers/media/video/cx23885/cx23885-core.c This is strange. For one thing, 1.95 MHz wouldn't be standard I2C but high-speed mode I2C. But more importantly, I fail to see how you could reach such speeds with a software-driven, byte-by-byte implementation. You need hardware buffers to reach high speeds on I2C. The TeVii S470 card had what looked like at serial I2C EEPROM with the A0, A1, and A2 pins all grounded, so I assume it is at 0x50 on one of the CX23885's external I2C buses. Probably. Hopefully my patches will show you where it is. -- 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 Receiver on an Tevii S470
Hi Andy, On Sun, 22 Nov 2009 15:11:47 -0500, Andy Walls wrote: On Sun, 2009-11-22 at 19:08 +0100, Matthias Fechner wrote: thanks a lot for your answer. I uploaded two pictures I did from the card, you can find it here: http://fechner.net/tevii-s470/ It is a CX23885. The driver I use is the ds3000. lspci says: [snip] Thanks for the pictures. OK so of the two other interesting chips on the S470: U4 is an I2C connected EEPROM - we don't care about that for IR. U10 appears to perhaps be a Silicon Labs C8051F300 microcontroller or similar: http://www.silabs.com/products/mcu/smallmcu/Pages/C8051F30x.aspx Since the 'F300 has an A/D convertor and has an SMBus interface (compatable with the I2C bus), I suspect this chip could be the IR controller on the TeVii S470. Could you as root: # modprobe cx23885 # modprobe i2c-dev # i2c-detect -l (to list all the i2c buses, including cx23885 mastered i2c buses) # i2c-detect -y N (to show the addresses in use on bus # N: only query the cx23885 buses) i2c-detect was in the lm-sensors package last I checked. (Jean can correct me if I'm wrong.) It is actually named i2cdetect (no dash). It used to live in the lm-sensors package (up to 2.10.x) but is now in i2c-tools: http://www.lm-sensors.org/wiki/I2CTools With that information, I should be able to figure out what I2C address that microcontroller is listening to. -- 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 Receiver on an Tevii S470
On Sun, 22 Nov 2009 21:25:27 +0100, Matthias Fechner wrote: Hi Andy, Andy Walls wrote: # modprobe cx23885 # modprobe i2c-dev # i2c-detect -l (to list all the i2c buses, including cx23885 mastered i2c buses) i2c-0smbus SMBus nForce2 adapter at 4d00 SMBus adapter i2c-1i2c cx23885[0] I2C adapter i2c-2i2c cx23885[0] I2C adapter i2c-3i2c cx23885[0] I2C adapter i2c-4i2c NVIDIA i2c adapter I2C adapter i2c-5i2c NVIDIA i2c adapter I2C adapter i2c-6i2c NVIDIA i2c adapter I2C adapter # i2c-detect -y N (to show the addresses in use on bus # N: only query the cx23885 buses) vdrhd1 ~ # i2cdetect -y 1 0 1 2 3 4 5 6 7 8 9 a b c d e f 00: -- -- -- -- -- -- -- -- -- -- -- -- -- 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: 30 31 32 33 34 35 36 37 -- -- -- -- -- -- -- -- 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 50: 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 70: -- -- -- -- -- -- -- -- vdrhd1 ~ # i2cdetect -y 2 0 1 2 3 4 5 6 7 8 9 a b c d e f 00: -- -- -- -- -- -- -- -- -- -- -- -- -- 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: 30 31 32 33 34 35 36 37 -- -- -- -- -- -- -- -- 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 50: 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f 60: -- -- -- -- -- -- -- -- 68 -- -- -- -- -- -- -- 70: -- -- -- -- -- -- -- -- vdrhd1 ~ # i2cdetect -y 3 0 1 2 3 4 5 6 7 8 9 a b c d e f 00: -- -- -- -- -- -- -- -- -- -- -- -- -- 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: 30 31 32 33 34 35 36 37 -- -- -- -- -- -- -- -- 40: -- -- -- -- 44 -- -- -- -- -- -- -- 4c -- -- -- 50: 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 70: -- -- -- -- -- -- -- -- The fact that 0x30-0x37 and 0x50-0x5f all reply suggest that the bus driver erroneously returns success to SMBus receive byte transactions even when no device acks. This is a bug which should get fixed. If you point me to the I2C adapter driver code, I can take a look. In the meantime, you can try i2cdetect -q to force i2cdetect to use SMBus quick commands for all the addresses. Beware though that some chips are known to not like it at all (in particular the infamous AT24RF08... not that I expect to ever see one on a TV adapter but you never know.) At least the above scan has already found 3 chips. -- 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: Details about DVB frontend API
On Tue, 17 Nov 2009 14:55:51 -0500, Devin Heitmueller wrote: On Tue, Nov 17, 2009 at 2:46 PM, Mauro Carvalho Chehab mche...@infradead.org wrote: I don't like the idea of creating structs grouping those parameters. While for certain devices this may mean a more direct approach, for others, this may not make sense, due to the way their API's were implemented (for example, devices with firmware may need several calls to get all those info). There is some value in providing grouping the results in a single request - in many cases the data is based off of the same internal registers, and Manu's proposed approach allows for a more atomic response. The fact that we currently do the status, SNR, strength, and UNC/BER in separate calls is one reason that people sometimes see inconsistent results in the output of tools like zap. As an example, they can see lines in the zap output where the lock is lost but SNR appears fine. In the case where the driver servicing the query needs to do three calls, it could be slightly more expensive, but only if we believe that it is commonplace to ask for a subset of the stats. For what it's worth, we have solved this problem in hwmon driver the following way: we cache related values (read from the same register or set of registers) for ~1 second. When user-space requests the information, if the cache is fresh it is used, otherwise the cache is first refreshed. That way we ensure that data returned to nearby user-space calls are taken from the same original register value. One advantage is that we thus did not have to map the API to the hardware register constraints and thus have the guarantee that all hardware designs fit. I don't know if a similar logic would help for DVB. -- 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: Details about DVB frontend API
Hi Devin, On Thu, 22 Oct 2009 15:27:20 -0400, Devin Heitmueller wrote: On Thu, Oct 22, 2009 at 3:13 PM, Jean Delvare kh...@linux-fr.org wrote: Hi folks, I am looking for details regarding the DVB frontend API. I've read linux-dvb-api-1.0.0.pdf, it roughly explains what the FE_READ_BER, FE_READ_SNR, FE_READ_SIGNAL_STRENGTH and FE_READ_UNCORRECTED_BLOCKS commands return, however it does not give any information about how the returned values should be interpreted (or, seen from the other end, how the frontend kernel drivers should encode these values.) If there documentation available that would explain this? For example, the signal strength. All I know so far is that this is a 16-bit value. But then what? Do greater values represent stronger signal or weaker signal? Are 0x and 0x special values? Is the returned value meaningful even when FE_HAS_SIGNAL is 0? When FE_HAS_LOCK is 0? Is the scale linear, or do some values have well-defined meanings, or is it arbitrary and each driver can have its own scale? What are the typical use cases by user-space application for this value? That's the kind of details I'd like to know, not only for the signal strength, but also for the SNR, BER and UB. Without this information, it seems a little difficult to have consistent frontend drivers. Thanks, -- Jean Delvare I try to raise this every six months or so. Check the mailing list archive for SNR in the subject line. Yes, it's all screwed up and inconsistent across demods. I took a crack at fixing it a few months ago by proposing a standard (and even offering to fix up all the demods to be consistent), and those efforts were derailed by some individuals who wanted what I would consider a perfect interface at the cost of something that worked for 98% of the userbase (I'm not going to point any fingers). And what did we get as a result? Nothing. I could have had this problem solved six months ago for 98% of the community, and instead we are right where we have been since the beginning of the project. /me stops thinking about this and goes and gets some coffee Sorry, I didn't mean to restart a war. I really expected a standard to exist but be possibly undocumented. I did not expect all these values to not be standardized at all :( Thanks to all who answered anyway. I sincerely hope that we can improve the situation in a near future. I am not too familiar with DVB driver development, but I believe that even loose standards would be much better than no standards at all. And strict standards might not be possible to implement properly anyway, in case we do not have detailed specifications of the hardware (which is relatively frequent as I understand it.) Taking the example of the signal strength, I think I would be fine with the following description: 16-bit value, 0 means weakest signal (possibly no signal at all), 0x means strongest signal. I suspect user-space applications would already be able to do something about this. 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: Details about DVB frontend API
On Thu, 22 Oct 2009 21:13:30 +0200, Jean Delvare wrote: For example, the signal strength. All I know so far is that this is a 16-bit value. But then what? Do greater values represent stronger signal or weaker signal? Are 0x and 0x special values? Is the returned value meaningful even when FE_HAS_SIGNAL is 0? When FE_HAS_LOCK is 0? Is the scale linear, or do some values have well-defined meanings, or is it arbitrary and each driver can have its own scale? What are the typical use cases by user-space application for this value? To close the chapter on signal strength... I understand now that we don't have strict rules about the exact values. But do we have at least a common agreement that greater values mean stronger signal? I am asking because the DVB-T adapter model I have here behaves very strangely in this respect. I get values of: * 0x when there's no signal at all * 0x2828 to 0x2e2e when signal is OK * greater values as signal weakens (I have an amplified antenna with manual gain control) up to 0x7272 I would have expected it the other way around: 0x for no signal and greater values as signal strengthens. I think the frontend driver (cx22702) needs to be fixed. -- 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
On Sun, 04 Oct 2009 21:54:37 -0400, Andy Walls wrote: On Mon, 2009-10-05 at 01:23 +0300, Aleksandr V. Piskunov wrote: So the solution(?) I found was to decrease the udelay in ivtv_i2c_algo_template from 10 to 5. Guess it just doubles the frequency of ivtv i2c bus or something like that. Problem went away, IR controller is now working as expected. That's a long standing error in the ivtv driver. It ran the I2C bus at 1/(2*10 usec) = 50 kHz instead of the standard 100 kHz. Technically any I2C device should be able to handle clock rates down to about DC IIRC; so there must be a bug in the IR microcontroller implementation. Also the CX23416 errantly marks its PCI register space as cacheable which is probably wrong (see lspci output). This may also be interfering with proper I2C operation with i2c_algo_bit depedning on the PCI bridges in your system. So question is: 1) Is it ok to decrease udelay for this board? Sure, I think. It would actually run the ivtv I2C bus at the nominal clock rate specified by the I2C specification. FWIW, 100 kHz isn't the nominal I2C clock rate, but the maximum clock rate for normal I2C. It is perfectly valid to run I2C buses as lower clock frequencies. I don't even think there is a minimum for I2C (but there is a minimum of 10 kHz for SMBus.) But of course different hardware implementations may not fully cover the standard I2C or SMBus frequency range, and it is possible that a TV adapter manufacturer designed its hardware to run the I2C bus at a fixed frequency and we have to use that frequency to make the adapter happy. I never had any reason to change it, as I feared causing regressions in many well tested boards. This is a possibility, indeed. But for obvious performance reasons, I'd rather use 100 kHz as the default, and let boards override it with a lower frequency of their choice if needed. Obviously this provides an easy improvement path, where each board can be tested separately and I2C bus frequency bumped from 50 kHz to 100 kHz after some good testing. Some boards might even support fast I2C, up to 400 kHz but limited to 250 kHz by the i2c-algo-bit implementation. -- 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
On Mon, 5 Oct 2009 11:50:31 +0300, Aleksandr V. Piskunov wrote: Try: # modprobe ivtv newi2c=1 to see if that works first. udelay=10, newi2c=0 = BAD udelay=10, newi2c=1 = BAD udelay=5, newi2c=0 = OK udelay=5, newi2c=1 = BAD The udelay value is only used by i2c-algo-bit, not newi2c, so the last test was not needed. newi2c=1 also throws some log messages, not sure if its ok or not. Oct 5 11:41:16 moon kernel: [45430.916449] ivtv: Start initialization, version 1.4.1 Oct 5 11:41:16 moon kernel: [45430.916618] ivtv0: Initializing card 0 Oct 5 11:41:16 moon kernel: [45430.916628] ivtv0: Autodetected AVerTV MCE 116 Plus card (cx23416 based) Oct 5 11:41:16 moon kernel: [45430.918887] ivtv :03:06.0: PCI INT A - GSI 20 (level, low) - IRQ 20 Oct 5 11:41:16 moon kernel: [45430.919229] ivtv0: i2c: i2c init Oct 5 11:41:16 moon kernel: [45430.919234] ivtv0: i2c: setting scl and sda to 1 Oct 5 11:41:16 moon kernel: [45430.937745] cx25840 0-0044: cx25843-23 found @ 0x88 (ivtv i2c driver #0) Oct 5 11:41:16 moon kernel: [45430.949145] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.951628] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.954191] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.956724] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.959211] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.961749] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.964236] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.966722] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.966786] ivtv0: i2c: i2c write to 43 failed Oct 5 11:41:16 moon kernel: [45430.971106] tuner 0-0061: chip found @ 0xc2 (ivtv i2c driver #0) Oct 5 11:41:16 moon kernel: [45430.974404] wm8739 0-001a: chip found @ 0x34 (ivtv i2c driver #0) Oct 5 11:41:16 moon kernel: [45430.986328] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.988871] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.991355] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.993904] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.996427] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45430.998938] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.001477] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.003968] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.004053] ivtv0: i2c: i2c write to 18 failed Oct 5 11:41:16 moon kernel: [45431.011333] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.013883] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.016418] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.018911] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.021463] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.023937] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.026478] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.028998] ivtv0: i2c: Slave did not ack Oct 5 11:41:16 moon kernel: [45431.029063] ivtv0: i2c: i2c write to 71 failed Oct 5 11:41:16 moon kernel: [45431.031468] ivtv0: i2c: Slave did not ack That would be I2C probe attempts such as the ones done by ir-kbd-i2c. Nothing to be afraid of. -- 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
Hi Andy,
On Sun, 04 Oct 2009 16:11:32 -0400, Andy Walls wrote:
On Sun, 2009-10-04 at 10:54 +0200, Jean Delvare wrote:
On Sat, 03 Oct 2009 11:44:20 -0400, Andy Walls wrote:
/* This array should match the IVTV_HW_ defines */
@@ -126,7 +131,8 @@
wm8739,
vp27smpx,
m52790,
- NULL
+ NULL,
+ NULL/* IVTV_HW_EM78P153S_IR_RX_AVER */
};
/* This array should match the IVTV_HW_ defines */
@@ -151,9 +157,38 @@
vp27smpx,
m52790,
gpio,
+ ir_rx_em78p153s_aver,
This exceeds the maximum length for I2C client names (19 chars max.) So
your patch won't work. I could make the name field slightly larger (say
23 chars) if really needed, but I'd rather have you simply use shorter
names.
I'll use shorter names. I was trying to be maintain some uniqueness.
The bridge driver has the knowledge of the exact chip and nothing else
does unless the bridge exposes it somehow. It seemed like a good way to
expose the knowledge.
The knowledge is already exposed through the platform data attached to
the instantiated i2c device (.ir_codes, .internal_get_key_func, .type
and .name). The i2c client name isn't used by the ir-kbd-i2c driver to
do anything useful.
+static int ivtv_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char
*type,
+u8 addr)
+{
+ struct i2c_board_info info;
+ 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 */
+ switch (hw) {
+ case IVTV_HW_EM78P153S_IR_RX_AVER:
+ info.platform_data = (void *) em78p153s_aver_ir_init_data;
Useless cast. You never need to cast to void *.
The compiler gripes because the const gets discarded; Mauro asked me
to fix it in cx18 previously. I could have cast it to the proper type,
but then it wouldn't have fit in 80 columns.
(void *) wasn't useless, it kept gcc, checkpatch, Mauro and me happy.
Now I guess I'll have to break the assignment to be over two lines. :(
Ah, good point, I had missed it. Well basically this means that you're
not supposed to pass const data structures as platform data. So maybe
you'd rather follow the approach used in the saa7134 and em28xx drivers.
--- 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.
I though a matching name was required for ir-kbd-i2c to bind to the IR
controller deivce. I personally don't like the ir_video name as it is
a bit too generic, but then again I don't know whwre that name is
visible outside the kernel. My plan was to have rather specific names,
so LIRC in the future could know automatically how to handle some of
these devices without the user trying to guess what an ir_video device
was as that name supplied no information to LIRC or the user.
The name is visible in sysfs as the client's name attribute. But no
user-space application should rely on this. If a user-space application
should use a name string, that should be the _input_ name and not the
i2c client name. For the simple reason that IR devices don't have to be
I2C devices.
The i2c device name is merely used for device-driver matching. For this
purpose, ir_video works just fine. As I said before, there is a point
in defining other names if it allows the ir-kbd-i2c driver to make
decisions by itself, or if you envision to move support for a specific
device to a separate driver as some point. If not then you're making
things more complex with zero benefit.
I'd like to add that, IMHO, LIRC shouldn't have to care about this at
all. The name should be purely informative. I have experimented in the
past with user-space trying to do device-specific handling based on a
name string. This is what we did in libsensors 2. This ended up being a
totally unmaintainable mess, where each new kernel had to be followed
by updated user-space. This was a pain and you really shouldn't go in
this direction. For libsensors 3, we've defined a clean sysfs
interface, which describes the functionality of each supported device,
so the library doesn't do any name-driver processing. Very easy to
maintain.
So if you want a piece of advice: either handle all device-specific
things in the kernel, or in user-space, but don't do half in the kernel
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
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
[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 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
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
[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, 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
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: [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: 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: [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: [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/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: [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
