* [PATCH 1/6] em28xx: simplify device state tracking
2012-12-27 23:02 [PATCH 0/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
@ 2012-12-27 23:02 ` Frank Schäfer
2012-12-27 23:02 ` [PATCH 2/6] em28xx: refactor the code in em28xx_usb_disconnect() Frank Schäfer
` (4 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Frank Schäfer @ 2012-12-27 23:02 UTC (permalink / raw)
To: mchehab; +Cc: linux-media, Frank Schäfer
DEV_INITIALIZED of enum em28xx_dev_state state is used nowhere and there is no
need for DEV_MISCONFIGURED, so remove this enum and use a boolean field
'disconnected' in the device struct instead.
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
drivers/media/usb/em28xx/em28xx-cards.c | 5 ++---
drivers/media/usb/em28xx/em28xx-core.c | 4 ++--
drivers/media/usb/em28xx/em28xx-dvb.c | 4 ++--
drivers/media/usb/em28xx/em28xx-video.c | 12 +++---------
drivers/media/usb/em28xx/em28xx.h | 12 ++----------
5 Dateien geändert, 11 Zeilen hinzugefügt(+), 26 Zeilen entfernt(-)
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index f5cac47..8496a06 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3501,11 +3501,10 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
"deallocation are deferred on close.\n",
video_device_node_name(dev->vdev));
- dev->state |= DEV_MISCONFIGURED;
em28xx_uninit_usb_xfer(dev, dev->mode);
- dev->state |= DEV_DISCONNECTED;
+ dev->disconnected = 1;
} else {
- dev->state |= DEV_DISCONNECTED;
+ dev->disconnected = 1;
em28xx_release_resources(dev);
}
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index b10d959..6916e87 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -77,7 +77,7 @@ int em28xx_read_reg_req_len(struct em28xx *dev, u8 req, u16 reg,
int ret;
int pipe = usb_rcvctrlpipe(dev->udev, 0);
- if (dev->state & DEV_DISCONNECTED)
+ if (dev->disconnected)
return -ENODEV;
if (len > URB_MAX_CTRL_SIZE)
@@ -153,7 +153,7 @@ int em28xx_write_regs_req(struct em28xx *dev, u8 req, u16 reg, char *buf,
int ret;
int pipe = usb_sndctrlpipe(dev->udev, 0);
- if (dev->state & DEV_DISCONNECTED)
+ if (dev->disconnected)
return -ENODEV;
if ((len < 1) || (len > URB_MAX_CTRL_SIZE))
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index a70b19e..e206c2b 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -133,7 +133,7 @@ static inline int em28xx_dvb_urb_data_copy(struct em28xx *dev, struct urb *urb)
if (!dev)
return 0;
- if ((dev->state & DEV_DISCONNECTED) || (dev->state & DEV_MISCONFIGURED))
+ if (dev->disconnected)
return 0;
if (urb->status < 0)
@@ -1320,7 +1320,7 @@ static int em28xx_dvb_fini(struct em28xx *dev)
if (dev->dvb) {
struct em28xx_dvb *dvb = dev->dvb;
- if (dev->state & DEV_DISCONNECTED) {
+ if (dev->disconnected) {
/* We cannot tell the device to sleep
* once it has been unplugged. */
if (dvb->fe[0])
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 4c1726d..824f298 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -442,7 +442,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
if (!dev)
return 0;
- if ((dev->state & DEV_DISCONNECTED) || (dev->state & DEV_MISCONFIGURED))
+ if (dev->disconnected)
return 0;
if (urb->status < 0)
@@ -790,16 +790,10 @@ handle:
static int check_dev(struct em28xx *dev)
{
- if (dev->state & DEV_DISCONNECTED) {
+ if (dev->disconnected) {
em28xx_errdev("v4l2 ioctl: device not present\n");
return -ENODEV;
}
-
- if (dev->state & DEV_MISCONFIGURED) {
- em28xx_errdev("v4l2 ioctl: device is misconfigured; "
- "close and open it again\n");
- return -EIO;
- }
return 0;
}
@@ -2068,7 +2062,7 @@ static int em28xx_v4l2_close(struct file *filp)
if (dev->users == 1) {
/* the device is already disconnect,
free the remaining resources */
- if (dev->state & DEV_DISCONNECTED) {
+ if (dev->disconnected) {
em28xx_release_resources(dev);
kfree(dev->alt_max_pkt_size_isoc);
mutex_unlock(&dev->lock);
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 062841e..7a40b92 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -437,13 +437,6 @@ struct em28xx_eeprom {
u8 string_idx_table;
};
-/* device states */
-enum em28xx_dev_state {
- DEV_INITIALIZED = 0x01,
- DEV_DISCONNECTED = 0x02,
- DEV_MISCONFIGURED = 0x04,
-};
-
#define EM28XX_AUDIO_BUFS 5
#define EM28XX_NUM_AUDIO_PACKETS 64
#define EM28XX_AUDIO_MAX_PACKET_SIZE 196 /* static value */
@@ -494,6 +487,8 @@ struct em28xx {
int devno; /* marks the number of this device */
enum em28xx_chip_id chip_id;
+ unsigned char disconnected:1; /* device has been diconnected */
+
int audio_ifnum;
struct v4l2_device v4l2_dev;
@@ -561,9 +556,6 @@ struct em28xx {
struct em28xx_audio adev;
- /* states */
- enum em28xx_dev_state state;
-
/* capture state tracking */
int capture_type;
unsigned char top_field:1;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 2/6] em28xx: refactor the code in em28xx_usb_disconnect()
2012-12-27 23:02 [PATCH 0/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
2012-12-27 23:02 ` [PATCH 1/6] em28xx: simplify device state tracking Frank Schäfer
@ 2012-12-27 23:02 ` Frank Schäfer
2013-01-02 19:40 ` Antti Palosaari
2012-12-27 23:02 ` [PATCH 3/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
` (3 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Frank Schäfer @ 2012-12-27 23:02 UTC (permalink / raw)
To: mchehab; +Cc: linux-media, Frank Schäfer
The main purpose of this patch is to move the call of em28xx_release_resources()
after the call of em28xx_close_extension().
This is necessary, because some resources might be needed/used by the extensions
fini() functions when they get closed.
Also mark the device as disconnected earlier in this function and unify the
em28xx_uninit_usb_xfer() calls for analog and digital mode.
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
drivers/media/usb/em28xx/em28xx-cards.c | 26 +++++++++++---------------
1 Datei geändert, 11 Zeilen hinzugefügt(+), 15 Zeilen entfernt(-)
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 8496a06..40c3e45 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3478,6 +3478,8 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
if (!dev)
return;
+ dev->disconnected = 1;
+
if (dev->is_audio_only) {
mutex_lock(&dev->lock);
em28xx_close_extension(dev);
@@ -3489,32 +3491,26 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
flush_request_modules(dev);
- /* wait until all current v4l2 io is finished then deallocate
- resources */
mutex_lock(&dev->lock);
v4l2_device_disconnect(&dev->v4l2_dev);
if (dev->users) {
- em28xx_warn
- ("device %s is open! Deregistration and memory "
- "deallocation are deferred on close.\n",
- video_device_node_name(dev->vdev));
+ em28xx_warn("device %s is open! Deregistration and memory deallocation are deferred on close.\n",
+ video_device_node_name(dev->vdev));
- em28xx_uninit_usb_xfer(dev, dev->mode);
- dev->disconnected = 1;
- } else {
- dev->disconnected = 1;
- em28xx_release_resources(dev);
+ em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
+ em28xx_uninit_usb_xfer(dev, EM28XX_DIGITAL_MODE);
}
- /* free DVB isoc buffers */
- em28xx_uninit_usb_xfer(dev, EM28XX_DIGITAL_MODE);
+ em28xx_close_extension(dev);
+ /* NOTE: must be called BEFORE the resources are released */
+
+ if (!dev->users)
+ em28xx_release_resources(dev);
mutex_unlock(&dev->lock);
- em28xx_close_extension(dev);
-
if (!dev->users) {
kfree(dev->alt_max_pkt_size_isoc);
kfree(dev);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/6] em28xx: refactor the code in em28xx_usb_disconnect()
2012-12-27 23:02 ` [PATCH 2/6] em28xx: refactor the code in em28xx_usb_disconnect() Frank Schäfer
@ 2013-01-02 19:40 ` Antti Palosaari
2013-01-02 21:18 ` Frank Schäfer
0 siblings, 1 reply; 20+ messages in thread
From: Antti Palosaari @ 2013-01-02 19:40 UTC (permalink / raw)
To: Frank Schäfer; +Cc: mchehab, linux-media
On 12/28/2012 01:02 AM, Frank Schäfer wrote:
> The main purpose of this patch is to move the call of em28xx_release_resources()
> after the call of em28xx_close_extension().
> This is necessary, because some resources might be needed/used by the extensions
> fini() functions when they get closed.
> Also mark the device as disconnected earlier in this function and unify the
> em28xx_uninit_usb_xfer() calls for analog and digital mode.
This looks like it could fix that one bug I reported earlier. Care to
look these:
em28xx releases I2C adapter earlier than demod/tuner/sec
http://www.spinics.net/lists/linux-media/msg54693.html
em28xx #0: submit of audio urb failed
http://www.spinics.net/lists/linux-media/msg54694.html
regards
Antti
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/6] em28xx: refactor the code in em28xx_usb_disconnect()
2013-01-02 19:40 ` Antti Palosaari
@ 2013-01-02 21:18 ` Frank Schäfer
0 siblings, 0 replies; 20+ messages in thread
From: Frank Schäfer @ 2013-01-02 21:18 UTC (permalink / raw)
To: Antti Palosaari, Linux Media Mailing List
Am 02.01.2013 20:40, schrieb Antti Palosaari:
> On 12/28/2012 01:02 AM, Frank Schäfer wrote:
>> The main purpose of this patch is to move the call of
>> em28xx_release_resources()
>> after the call of em28xx_close_extension().
>> This is necessary, because some resources might be needed/used by the
>> extensions
>> fini() functions when they get closed.
>> Also mark the device as disconnected earlier in this function and
>> unify the
>> em28xx_uninit_usb_xfer() calls for analog and digital mode.
>
> This looks like it could fix that one bug I reported earlier. Care to
> look these:
>
> em28xx releases I2C adapter earlier than demod/tuner/sec
> http://www.spinics.net/lists/linux-media/msg54693.html
Yes, this one gets fixed with the patch.
>
> em28xx #0: submit of audio urb failed
> http://www.spinics.net/lists/linux-media/msg54694.html
Seems to describe more than one bug.
What do mean with "when I plug em28xx device with analog support" ? Did
you mean "unplug" ?
Does this device use an external audio IC connected via i2c (e.g. EM202) ?
If yes, I think the patch should fix this issue, too.
Do you still have access to this device ? Can you test the patch ?
Regards,
Frank
>
>
> regards
> Antti
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/6] em28xx: make remote controls of devices with external IR IC working again
2012-12-27 23:02 [PATCH 0/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
2012-12-27 23:02 ` [PATCH 1/6] em28xx: simplify device state tracking Frank Schäfer
2012-12-27 23:02 ` [PATCH 2/6] em28xx: refactor the code in em28xx_usb_disconnect() Frank Schäfer
@ 2012-12-27 23:02 ` Frank Schäfer
2013-01-04 21:12 ` Mauro Carvalho Chehab
2012-12-27 23:02 ` [PATCH 4/6] em28xx: IR RC: get rid of function em28xx_get_key_terratec() Frank Schäfer
` (2 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Frank Schäfer @ 2012-12-27 23:02 UTC (permalink / raw)
To: mchehab; +Cc: linux-media, Frank Schäfer
Tested with device "Terratec Cinergy 200 USB".
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
drivers/media/usb/em28xx/em28xx-cards.c | 9 +-
drivers/media/usb/em28xx/em28xx-i2c.c | 1 +
drivers/media/usb/em28xx/em28xx-input.c | 142 +++++++++++++++++--------------
3 Dateien geändert, 83 Zeilen hinzugefügt(+), 69 Zeilen entfernt(-)
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 40c3e45..3b226b1 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -488,6 +488,7 @@ struct em28xx_board em28xx_boards[] = {
.name = "Terratec Cinergy 250 USB",
.tuner_type = TUNER_LG_PAL_NEW_TAPC,
.has_ir_i2c = 1,
+ .ir_codes = RC_MAP_EM_TERRATEC,
.tda9887_conf = TDA9887_PRESENT,
.decoder = EM28XX_SAA711X,
.input = { {
@@ -508,6 +509,7 @@ struct em28xx_board em28xx_boards[] = {
.name = "Pinnacle PCTV USB 2",
.tuner_type = TUNER_LG_PAL_NEW_TAPC,
.has_ir_i2c = 1,
+ .ir_codes = RC_MAP_PINNACLE_GREY,
.tda9887_conf = TDA9887_PRESENT,
.decoder = EM28XX_SAA711X,
.input = { {
@@ -533,6 +535,7 @@ struct em28xx_board em28xx_boards[] = {
.decoder = EM28XX_TVP5150,
.has_msp34xx = 1,
.has_ir_i2c = 1,
+ .ir_codes = RC_MAP_HAUPPAUGE,
.input = { {
.type = EM28XX_VMUX_TELEVISION,
.vmux = TVP5150_COMPOSITE0,
@@ -629,6 +632,7 @@ struct em28xx_board em28xx_boards[] = {
.valid = EM28XX_BOARD_NOT_VALIDATED,
.tuner_type = TUNER_PHILIPS_FM1216ME_MK3,
.has_ir_i2c = 1,
+ .ir_codes = RC_MAP_WINFAST_USBII_DELUXE,
.tvaudio_addr = 0x58,
.tda9887_conf = TDA9887_PRESENT |
TDA9887_PORT2_ACTIVE |
@@ -1222,6 +1226,7 @@ struct em28xx_board em28xx_boards[] = {
.name = "Terratec Cinergy 200 USB",
.is_em2800 = 1,
.has_ir_i2c = 1,
+ .ir_codes = RC_MAP_EM_TERRATEC,
.tuner_type = TUNER_LG_TALN,
.tda9887_conf = TDA9887_PRESENT,
.decoder = EM28XX_SAA711X,
@@ -2912,7 +2917,7 @@ static void request_module_async(struct work_struct *work)
if (dev->board.has_dvb)
request_module("em28xx-dvb");
- if (dev->board.ir_codes && !disable_ir)
+ if ((dev->board.ir_codes || dev->board.has_ir_i2c) && !disable_ir)
request_module("em28xx-rc");
#endif /* CONFIG_MODULES */
}
@@ -2935,8 +2940,6 @@ static void flush_request_modules(struct em28xx *dev)
*/
void em28xx_release_resources(struct em28xx *dev)
{
- /*FIXME: I2C IR should be disconnected */
-
em28xx_release_analog_resources(dev);
em28xx_i2c_unregister(dev);
diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 44533e4..39c5a3e 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -470,6 +470,7 @@ static struct i2c_client em28xx_client_template = {
static char *i2c_devs[128] = {
[0x4a >> 1] = "saa7113h",
[0x52 >> 1] = "drxk",
+ [0x3e >> 1] = "remote IR sensor",
[0x60 >> 1] = "remote IR sensor",
[0x8e >> 1] = "remote IR sensor",
[0x86 >> 1] = "tda9887",
diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index 3598221..631e252 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -5,6 +5,7 @@
Markus Rechberger <mrechberger@gmail.com>
Mauro Carvalho Chehab <mchehab@infradead.org>
Sascha Sommer <saschasommer@freenet.de>
+ Copyright (C) 2012 Frank Schäfer <fschaefer.oss@googlemail.com>
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
@@ -34,6 +35,8 @@
#define EM28XX_SBUTTON_QUERY_INTERVAL 500
#define EM28XX_R0C_USBSUSP_SNAPSHOT 0x20
+#define EM28XX_RC_QUERY_INTERVAL 100
+
static unsigned int ir_debug;
module_param(ir_debug, int, 0644);
MODULE_PARM_DESC(ir_debug, "enable debug messages [IR]");
@@ -67,13 +70,14 @@ struct em28xx_IR {
char name[32];
char phys[32];
- /* poll external decoder */
int polling;
struct delayed_work work;
unsigned int full_code:1;
unsigned int last_readcount;
u64 rc_type;
+ struct i2c_client *i2c_dev; /* external i2c IR receiver/decoder */
+
int (*get_key)(struct em28xx_IR *, struct em28xx_ir_poll_result *);
};
@@ -452,7 +456,7 @@ static int em28xx_ir_change_protocol(struct rc_dev *rc_dev, u64 *rc_type)
}
}
-static void em28xx_register_i2c_ir(struct em28xx *dev)
+static int em28xx_register_i2c_ir(struct em28xx *dev, struct rc_dev *rc_dev)
{
/* Leadtek winfast tv USBII deluxe can find a non working IR-device */
/* at address 0x18, so if that address is needed for another board in */
@@ -470,30 +474,46 @@ static void em28xx_register_i2c_ir(struct em28xx *dev)
switch (dev->model) {
case EM2800_BOARD_TERRATEC_CINERGY_200:
case EM2820_BOARD_TERRATEC_CINERGY_250:
- dev->init_data.ir_codes = RC_MAP_EM_TERRATEC;
- dev->init_data.get_key = em28xx_get_key_terratec;
dev->init_data.name = "i2c IR (EM28XX Terratec)";
+ dev->init_data.type = RC_BIT_OTHER;
+ dev->init_data.get_key = em28xx_get_key_terratec;
break;
case EM2820_BOARD_PINNACLE_USB_2:
- dev->init_data.ir_codes = RC_MAP_PINNACLE_GREY;
- dev->init_data.get_key = em28xx_get_key_pinnacle_usb_grey;
dev->init_data.name = "i2c IR (EM28XX Pinnacle PCTV)";
+ dev->init_data.type = RC_BIT_OTHER;
+ dev->init_data.get_key = em28xx_get_key_pinnacle_usb_grey;
break;
case EM2820_BOARD_HAUPPAUGE_WINTV_USB_2:
- dev->init_data.ir_codes = RC_MAP_HAUPPAUGE;
- dev->init_data.get_key = em28xx_get_key_em_haup;
dev->init_data.name = "i2c IR (EM2840 Hauppauge)";
+ dev->init_data.type = RC_BIT_RC5;
+ dev->init_data.get_key = em28xx_get_key_em_haup;
break;
case EM2820_BOARD_LEADTEK_WINFAST_USBII_DELUXE:
- dev->init_data.ir_codes = RC_MAP_WINFAST_USBII_DELUXE;
- dev->init_data.get_key = em28xx_get_key_winfast_usbii_deluxe;
dev->init_data.name = "i2c IR (EM2820 Winfast TV USBII Deluxe)";
+ dev->init_data.type = RC_BIT_OTHER;
+ dev->init_data.get_key = em28xx_get_key_winfast_usbii_deluxe;
break;
}
- if (dev->init_data.name)
+ if (dev->init_data.name && dev->board.ir_codes) {
+ dev->init_data.ir_codes = dev->board.ir_codes;
+ dev->init_data.polling_interval = EM28XX_RC_QUERY_INTERVAL;
+ dev->init_data.rc_dev = rc_dev;
info.platform_data = &dev->init_data;
- i2c_new_probed_device(&dev->i2c_adap, &info, addr_list, NULL);
+ } else {
+ em28xx_warn("Unknown i2c remote control device.\n");
+ em28xx_warn("If the remote control doesn't work properly, please contact <linux-media@vger.kernel.org>\n");
+ }
+
+ dev->ir->i2c_dev = i2c_new_probed_device(&dev->i2c_adap, &info, addr_list, NULL);
+ if (NULL == dev->ir->i2c_dev)
+ return -ENODEV;
+
+#if defined(CONFIG_MODULES) && defined(MODULE)
+ request_module("ir-kbd-i2c");
+#endif
+
+ return 0;
}
/**********************************************************
@@ -590,7 +610,7 @@ static int em28xx_ir_init(struct em28xx *dev)
int err = -ENOMEM;
u64 rc_type;
- if (dev->board.ir_codes == NULL) {
+ if (dev->board.ir_codes == NULL && !dev->board.has_ir_i2c) {
/* No remote control support */
em28xx_warn("Remote control support is not available for "
"this card.\n");
@@ -607,68 +627,56 @@ static int em28xx_ir_init(struct em28xx *dev)
dev->ir = ir;
ir->rc = rc;
- /*
- * em2874 supports more protocols. For now, let's just announce
- * the two protocols that were already tested
- */
- rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC;
- rc->priv = ir;
- rc->change_protocol = em28xx_ir_change_protocol;
- rc->open = em28xx_ir_start;
- rc->close = em28xx_ir_stop;
-
- switch (dev->chip_id) {
- case CHIP_ID_EM2860:
- case CHIP_ID_EM2883:
- rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC;
- break;
- case CHIP_ID_EM2884:
- case CHIP_ID_EM2874:
- case CHIP_ID_EM28174:
- rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC | RC_BIT_RC6_0;
- break;
- default:
- err = -ENODEV;
- goto error;
- }
-
- /* By default, keep protocol field untouched */
- rc_type = RC_BIT_UNKNOWN;
- err = em28xx_ir_change_protocol(rc, &rc_type);
- if (err)
- goto error;
-
- /* This is how often we ask the chip for IR information */
- ir->polling = 100; /* ms */
-
- /* init input device */
- snprintf(ir->name, sizeof(ir->name), "em28xx IR (%s)",
- dev->name);
-
+ snprintf(ir->name, sizeof(ir->name), "em28xx IR (%s)", dev->name);
usb_make_path(dev->udev, ir->phys, sizeof(ir->phys));
strlcat(ir->phys, "/input0", sizeof(ir->phys));
+ ir->polling = EM28XX_RC_QUERY_INTERVAL;
- rc->input_name = ir->name;
- rc->input_phys = ir->phys;
- rc->input_id.bustype = BUS_USB;
rc->input_id.version = 1;
rc->input_id.vendor = le16_to_cpu(dev->udev->descriptor.idVendor);
rc->input_id.product = le16_to_cpu(dev->udev->descriptor.idProduct);
rc->dev.parent = &dev->udev->dev;
- rc->map_name = dev->board.ir_codes;
rc->driver_name = MODULE_NAME;
- /* all done */
- err = rc_register_device(rc);
- if (err)
- goto error;
-
- em28xx_register_i2c_ir(dev);
+ if (dev->board.has_ir_i2c) {
+ err = em28xx_register_i2c_ir(dev, rc);
+ if (err < 0)
+ goto error;
+ } else {
+ switch (dev->chip_id) {
+ case CHIP_ID_EM2860:
+ case CHIP_ID_EM2883:
+ rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC;
+ break;
+ case CHIP_ID_EM2884:
+ case CHIP_ID_EM2874:
+ case CHIP_ID_EM28174:
+ rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC | RC_BIT_RC6_0;
+ break;
+ default:
+ err = -ENODEV;
+ goto error;
+ }
+ rc->priv = ir;
+ rc->change_protocol = em28xx_ir_change_protocol;
+ rc->open = em28xx_ir_start;
+ rc->close = em28xx_ir_stop;
+ rc->input_name = ir->name;
+ rc->input_phys = ir->phys;
+ rc->input_id.bustype = BUS_USB;
+ rc->map_name = dev->board.ir_codes;
+
+ /* By default, keep protocol field untouched */
+ rc_type = RC_BIT_UNKNOWN;
+ err = em28xx_ir_change_protocol(rc, &rc_type);
+ if (err)
+ goto error;
+
+ err = rc_register_device(rc);
+ if (err < 0)
+ goto error;
+ }
-#if defined(CONFIG_MODULES) && defined(MODULE)
- if (dev->board.has_ir_i2c)
- request_module("ir-kbd-i2c");
-#endif
if (dev->board.has_snapshot_button)
em28xx_register_snapshot_button(dev);
@@ -691,7 +699,9 @@ static int em28xx_ir_fini(struct em28xx *dev)
if (!ir)
return 0;
- if (ir->rc)
+ if (ir->i2c_dev)
+ i2c_unregister_device(ir->i2c_dev);
+ else if (ir->rc)
rc_unregister_device(ir->rc);
/* done */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/6] em28xx: make remote controls of devices with external IR IC working again
2012-12-27 23:02 ` [PATCH 3/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
@ 2013-01-04 21:12 ` Mauro Carvalho Chehab
2013-01-05 12:58 ` Frank Schäfer
0 siblings, 1 reply; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2013-01-04 21:12 UTC (permalink / raw)
To: Frank Schäfer; +Cc: linux-media
Em Fri, 28 Dec 2012 00:02:45 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> Tested with device "Terratec Cinergy 200 USB".
Sorry, but this patch is completely wrong ;)
The fix here is simple: just move the initialization to happen
earlier.
I'm posting it right now, together with a bunch of other fixes for
I2C-based IR devices.
Regards,
Mauro
>
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
> drivers/media/usb/em28xx/em28xx-cards.c | 9 +-
> drivers/media/usb/em28xx/em28xx-i2c.c | 1 +
> drivers/media/usb/em28xx/em28xx-input.c | 142 +++++++++++++++++--------------
> 3 Dateien geändert, 83 Zeilen hinzugefügt(+), 69 Zeilen entfernt(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index 40c3e45..3b226b1 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -488,6 +488,7 @@ struct em28xx_board em28xx_boards[] = {
> .name = "Terratec Cinergy 250 USB",
> .tuner_type = TUNER_LG_PAL_NEW_TAPC,
> .has_ir_i2c = 1,
> + .ir_codes = RC_MAP_EM_TERRATEC,
> .tda9887_conf = TDA9887_PRESENT,
> .decoder = EM28XX_SAA711X,
> .input = { {
> @@ -508,6 +509,7 @@ struct em28xx_board em28xx_boards[] = {
> .name = "Pinnacle PCTV USB 2",
> .tuner_type = TUNER_LG_PAL_NEW_TAPC,
> .has_ir_i2c = 1,
> + .ir_codes = RC_MAP_PINNACLE_GREY,
> .tda9887_conf = TDA9887_PRESENT,
> .decoder = EM28XX_SAA711X,
> .input = { {
> @@ -533,6 +535,7 @@ struct em28xx_board em28xx_boards[] = {
> .decoder = EM28XX_TVP5150,
> .has_msp34xx = 1,
> .has_ir_i2c = 1,
> + .ir_codes = RC_MAP_HAUPPAUGE,
> .input = { {
> .type = EM28XX_VMUX_TELEVISION,
> .vmux = TVP5150_COMPOSITE0,
> @@ -629,6 +632,7 @@ struct em28xx_board em28xx_boards[] = {
> .valid = EM28XX_BOARD_NOT_VALIDATED,
> .tuner_type = TUNER_PHILIPS_FM1216ME_MK3,
> .has_ir_i2c = 1,
> + .ir_codes = RC_MAP_WINFAST_USBII_DELUXE,
> .tvaudio_addr = 0x58,
> .tda9887_conf = TDA9887_PRESENT |
> TDA9887_PORT2_ACTIVE |
> @@ -1222,6 +1226,7 @@ struct em28xx_board em28xx_boards[] = {
> .name = "Terratec Cinergy 200 USB",
> .is_em2800 = 1,
> .has_ir_i2c = 1,
> + .ir_codes = RC_MAP_EM_TERRATEC,
> .tuner_type = TUNER_LG_TALN,
> .tda9887_conf = TDA9887_PRESENT,
> .decoder = EM28XX_SAA711X,
> @@ -2912,7 +2917,7 @@ static void request_module_async(struct work_struct *work)
>
> if (dev->board.has_dvb)
> request_module("em28xx-dvb");
> - if (dev->board.ir_codes && !disable_ir)
> + if ((dev->board.ir_codes || dev->board.has_ir_i2c) && !disable_ir)
> request_module("em28xx-rc");
> #endif /* CONFIG_MODULES */
> }
> @@ -2935,8 +2940,6 @@ static void flush_request_modules(struct em28xx *dev)
> */
> void em28xx_release_resources(struct em28xx *dev)
> {
> - /*FIXME: I2C IR should be disconnected */
> -
> em28xx_release_analog_resources(dev);
>
> em28xx_i2c_unregister(dev);
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> index 44533e4..39c5a3e 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -470,6 +470,7 @@ static struct i2c_client em28xx_client_template = {
> static char *i2c_devs[128] = {
> [0x4a >> 1] = "saa7113h",
> [0x52 >> 1] = "drxk",
> + [0x3e >> 1] = "remote IR sensor",
> [0x60 >> 1] = "remote IR sensor",
> [0x8e >> 1] = "remote IR sensor",
> [0x86 >> 1] = "tda9887",
> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> index 3598221..631e252 100644
> --- a/drivers/media/usb/em28xx/em28xx-input.c
> +++ b/drivers/media/usb/em28xx/em28xx-input.c
> @@ -5,6 +5,7 @@
> Markus Rechberger <mrechberger@gmail.com>
> Mauro Carvalho Chehab <mchehab@infradead.org>
> Sascha Sommer <saschasommer@freenet.de>
> + Copyright (C) 2012 Frank Schäfer <fschaefer.oss@googlemail.com>
>
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> @@ -34,6 +35,8 @@
> #define EM28XX_SBUTTON_QUERY_INTERVAL 500
> #define EM28XX_R0C_USBSUSP_SNAPSHOT 0x20
>
> +#define EM28XX_RC_QUERY_INTERVAL 100
> +
> static unsigned int ir_debug;
> module_param(ir_debug, int, 0644);
> MODULE_PARM_DESC(ir_debug, "enable debug messages [IR]");
> @@ -67,13 +70,14 @@ struct em28xx_IR {
> char name[32];
> char phys[32];
>
> - /* poll external decoder */
> int polling;
> struct delayed_work work;
> unsigned int full_code:1;
> unsigned int last_readcount;
> u64 rc_type;
>
> + struct i2c_client *i2c_dev; /* external i2c IR receiver/decoder */
> +
> int (*get_key)(struct em28xx_IR *, struct em28xx_ir_poll_result *);
> };
>
> @@ -452,7 +456,7 @@ static int em28xx_ir_change_protocol(struct rc_dev *rc_dev, u64 *rc_type)
> }
> }
>
> -static void em28xx_register_i2c_ir(struct em28xx *dev)
> +static int em28xx_register_i2c_ir(struct em28xx *dev, struct rc_dev *rc_dev)
> {
> /* Leadtek winfast tv USBII deluxe can find a non working IR-device */
> /* at address 0x18, so if that address is needed for another board in */
> @@ -470,30 +474,46 @@ static void em28xx_register_i2c_ir(struct em28xx *dev)
> switch (dev->model) {
> case EM2800_BOARD_TERRATEC_CINERGY_200:
> case EM2820_BOARD_TERRATEC_CINERGY_250:
> - dev->init_data.ir_codes = RC_MAP_EM_TERRATEC;
> - dev->init_data.get_key = em28xx_get_key_terratec;
> dev->init_data.name = "i2c IR (EM28XX Terratec)";
> + dev->init_data.type = RC_BIT_OTHER;
> + dev->init_data.get_key = em28xx_get_key_terratec;
> break;
> case EM2820_BOARD_PINNACLE_USB_2:
> - dev->init_data.ir_codes = RC_MAP_PINNACLE_GREY;
> - dev->init_data.get_key = em28xx_get_key_pinnacle_usb_grey;
> dev->init_data.name = "i2c IR (EM28XX Pinnacle PCTV)";
> + dev->init_data.type = RC_BIT_OTHER;
> + dev->init_data.get_key = em28xx_get_key_pinnacle_usb_grey;
> break;
> case EM2820_BOARD_HAUPPAUGE_WINTV_USB_2:
> - dev->init_data.ir_codes = RC_MAP_HAUPPAUGE;
> - dev->init_data.get_key = em28xx_get_key_em_haup;
> dev->init_data.name = "i2c IR (EM2840 Hauppauge)";
> + dev->init_data.type = RC_BIT_RC5;
> + dev->init_data.get_key = em28xx_get_key_em_haup;
> break;
> case EM2820_BOARD_LEADTEK_WINFAST_USBII_DELUXE:
> - dev->init_data.ir_codes = RC_MAP_WINFAST_USBII_DELUXE;
> - dev->init_data.get_key = em28xx_get_key_winfast_usbii_deluxe;
> dev->init_data.name = "i2c IR (EM2820 Winfast TV USBII Deluxe)";
> + dev->init_data.type = RC_BIT_OTHER;
> + dev->init_data.get_key = em28xx_get_key_winfast_usbii_deluxe;
> break;
> }
>
> - if (dev->init_data.name)
> + if (dev->init_data.name && dev->board.ir_codes) {
> + dev->init_data.ir_codes = dev->board.ir_codes;
> + dev->init_data.polling_interval = EM28XX_RC_QUERY_INTERVAL;
> + dev->init_data.rc_dev = rc_dev;
> info.platform_data = &dev->init_data;
> - i2c_new_probed_device(&dev->i2c_adap, &info, addr_list, NULL);
> + } else {
> + em28xx_warn("Unknown i2c remote control device.\n");
> + em28xx_warn("If the remote control doesn't work properly, please contact <linux-media@vger.kernel.org>\n");
> + }
> +
> + dev->ir->i2c_dev = i2c_new_probed_device(&dev->i2c_adap, &info, addr_list, NULL);
> + if (NULL == dev->ir->i2c_dev)
> + return -ENODEV;
> +
> +#if defined(CONFIG_MODULES) && defined(MODULE)
> + request_module("ir-kbd-i2c");
> +#endif
> +
> + return 0;
> }
>
> /**********************************************************
> @@ -590,7 +610,7 @@ static int em28xx_ir_init(struct em28xx *dev)
> int err = -ENOMEM;
> u64 rc_type;
>
> - if (dev->board.ir_codes == NULL) {
> + if (dev->board.ir_codes == NULL && !dev->board.has_ir_i2c) {
> /* No remote control support */
> em28xx_warn("Remote control support is not available for "
> "this card.\n");
> @@ -607,68 +627,56 @@ static int em28xx_ir_init(struct em28xx *dev)
> dev->ir = ir;
> ir->rc = rc;
>
> - /*
> - * em2874 supports more protocols. For now, let's just announce
> - * the two protocols that were already tested
> - */
> - rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC;
> - rc->priv = ir;
> - rc->change_protocol = em28xx_ir_change_protocol;
> - rc->open = em28xx_ir_start;
> - rc->close = em28xx_ir_stop;
> -
> - switch (dev->chip_id) {
> - case CHIP_ID_EM2860:
> - case CHIP_ID_EM2883:
> - rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC;
> - break;
> - case CHIP_ID_EM2884:
> - case CHIP_ID_EM2874:
> - case CHIP_ID_EM28174:
> - rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC | RC_BIT_RC6_0;
> - break;
> - default:
> - err = -ENODEV;
> - goto error;
> - }
> -
> - /* By default, keep protocol field untouched */
> - rc_type = RC_BIT_UNKNOWN;
> - err = em28xx_ir_change_protocol(rc, &rc_type);
> - if (err)
> - goto error;
> -
> - /* This is how often we ask the chip for IR information */
> - ir->polling = 100; /* ms */
> -
> - /* init input device */
> - snprintf(ir->name, sizeof(ir->name), "em28xx IR (%s)",
> - dev->name);
> -
> + snprintf(ir->name, sizeof(ir->name), "em28xx IR (%s)", dev->name);
> usb_make_path(dev->udev, ir->phys, sizeof(ir->phys));
> strlcat(ir->phys, "/input0", sizeof(ir->phys));
> + ir->polling = EM28XX_RC_QUERY_INTERVAL;
>
> - rc->input_name = ir->name;
> - rc->input_phys = ir->phys;
> - rc->input_id.bustype = BUS_USB;
> rc->input_id.version = 1;
> rc->input_id.vendor = le16_to_cpu(dev->udev->descriptor.idVendor);
> rc->input_id.product = le16_to_cpu(dev->udev->descriptor.idProduct);
> rc->dev.parent = &dev->udev->dev;
> - rc->map_name = dev->board.ir_codes;
> rc->driver_name = MODULE_NAME;
>
> - /* all done */
> - err = rc_register_device(rc);
> - if (err)
> - goto error;
> -
> - em28xx_register_i2c_ir(dev);
> + if (dev->board.has_ir_i2c) {
> + err = em28xx_register_i2c_ir(dev, rc);
> + if (err < 0)
> + goto error;
> + } else {
> + switch (dev->chip_id) {
> + case CHIP_ID_EM2860:
> + case CHIP_ID_EM2883:
> + rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC;
> + break;
> + case CHIP_ID_EM2884:
> + case CHIP_ID_EM2874:
> + case CHIP_ID_EM28174:
> + rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC | RC_BIT_RC6_0;
> + break;
> + default:
> + err = -ENODEV;
> + goto error;
> + }
> + rc->priv = ir;
> + rc->change_protocol = em28xx_ir_change_protocol;
> + rc->open = em28xx_ir_start;
> + rc->close = em28xx_ir_stop;
> + rc->input_name = ir->name;
> + rc->input_phys = ir->phys;
> + rc->input_id.bustype = BUS_USB;
> + rc->map_name = dev->board.ir_codes;
> +
> + /* By default, keep protocol field untouched */
> + rc_type = RC_BIT_UNKNOWN;
> + err = em28xx_ir_change_protocol(rc, &rc_type);
> + if (err)
> + goto error;
> +
> + err = rc_register_device(rc);
> + if (err < 0)
> + goto error;
> + }
>
> -#if defined(CONFIG_MODULES) && defined(MODULE)
> - if (dev->board.has_ir_i2c)
> - request_module("ir-kbd-i2c");
> -#endif
> if (dev->board.has_snapshot_button)
> em28xx_register_snapshot_button(dev);
>
> @@ -691,7 +699,9 @@ static int em28xx_ir_fini(struct em28xx *dev)
> if (!ir)
> return 0;
>
> - if (ir->rc)
> + if (ir->i2c_dev)
> + i2c_unregister_device(ir->i2c_dev);
> + else if (ir->rc)
> rc_unregister_device(ir->rc);
>
> /* done */
--
Cheers,
Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 3/6] em28xx: make remote controls of devices with external IR IC working again
2013-01-04 21:12 ` Mauro Carvalho Chehab
@ 2013-01-05 12:58 ` Frank Schäfer
2013-01-05 13:26 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 20+ messages in thread
From: Frank Schäfer @ 2013-01-05 12:58 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media
Am 04.01.2013 22:12, schrieb Mauro Carvalho Chehab:
> Em Fri, 28 Dec 2012 00:02:45 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Tested with device "Terratec Cinergy 200 USB".
> Sorry, but this patch is completely wrong ;)
Completely wrong ? That's not helpful...
Please elaborate a bit more on this so that I can do things right next
time. ;)
Regards,
Frank
> The fix here is simple: just move the initialization to happen
> earlier.
>
> I'm posting it right now, together with a bunch of other fixes for
> I2C-based IR devices.
>
> Regards,
> Mauro
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>> drivers/media/usb/em28xx/em28xx-cards.c | 9 +-
>> drivers/media/usb/em28xx/em28xx-i2c.c | 1 +
>> drivers/media/usb/em28xx/em28xx-input.c | 142 +++++++++++++++++--------------
>> 3 Dateien geändert, 83 Zeilen hinzugefügt(+), 69 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>> index 40c3e45..3b226b1 100644
>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>> @@ -488,6 +488,7 @@ struct em28xx_board em28xx_boards[] = {
>> .name = "Terratec Cinergy 250 USB",
>> .tuner_type = TUNER_LG_PAL_NEW_TAPC,
>> .has_ir_i2c = 1,
>> + .ir_codes = RC_MAP_EM_TERRATEC,
>> .tda9887_conf = TDA9887_PRESENT,
>> .decoder = EM28XX_SAA711X,
>> .input = { {
>> @@ -508,6 +509,7 @@ struct em28xx_board em28xx_boards[] = {
>> .name = "Pinnacle PCTV USB 2",
>> .tuner_type = TUNER_LG_PAL_NEW_TAPC,
>> .has_ir_i2c = 1,
>> + .ir_codes = RC_MAP_PINNACLE_GREY,
>> .tda9887_conf = TDA9887_PRESENT,
>> .decoder = EM28XX_SAA711X,
>> .input = { {
>> @@ -533,6 +535,7 @@ struct em28xx_board em28xx_boards[] = {
>> .decoder = EM28XX_TVP5150,
>> .has_msp34xx = 1,
>> .has_ir_i2c = 1,
>> + .ir_codes = RC_MAP_HAUPPAUGE,
>> .input = { {
>> .type = EM28XX_VMUX_TELEVISION,
>> .vmux = TVP5150_COMPOSITE0,
>> @@ -629,6 +632,7 @@ struct em28xx_board em28xx_boards[] = {
>> .valid = EM28XX_BOARD_NOT_VALIDATED,
>> .tuner_type = TUNER_PHILIPS_FM1216ME_MK3,
>> .has_ir_i2c = 1,
>> + .ir_codes = RC_MAP_WINFAST_USBII_DELUXE,
>> .tvaudio_addr = 0x58,
>> .tda9887_conf = TDA9887_PRESENT |
>> TDA9887_PORT2_ACTIVE |
>> @@ -1222,6 +1226,7 @@ struct em28xx_board em28xx_boards[] = {
>> .name = "Terratec Cinergy 200 USB",
>> .is_em2800 = 1,
>> .has_ir_i2c = 1,
>> + .ir_codes = RC_MAP_EM_TERRATEC,
>> .tuner_type = TUNER_LG_TALN,
>> .tda9887_conf = TDA9887_PRESENT,
>> .decoder = EM28XX_SAA711X,
>> @@ -2912,7 +2917,7 @@ static void request_module_async(struct work_struct *work)
>>
>> if (dev->board.has_dvb)
>> request_module("em28xx-dvb");
>> - if (dev->board.ir_codes && !disable_ir)
>> + if ((dev->board.ir_codes || dev->board.has_ir_i2c) && !disable_ir)
>> request_module("em28xx-rc");
>> #endif /* CONFIG_MODULES */
>> }
>> @@ -2935,8 +2940,6 @@ static void flush_request_modules(struct em28xx *dev)
>> */
>> void em28xx_release_resources(struct em28xx *dev)
>> {
>> - /*FIXME: I2C IR should be disconnected */
>> -
>> em28xx_release_analog_resources(dev);
>>
>> em28xx_i2c_unregister(dev);
>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
>> index 44533e4..39c5a3e 100644
>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>> @@ -470,6 +470,7 @@ static struct i2c_client em28xx_client_template = {
>> static char *i2c_devs[128] = {
>> [0x4a >> 1] = "saa7113h",
>> [0x52 >> 1] = "drxk",
>> + [0x3e >> 1] = "remote IR sensor",
>> [0x60 >> 1] = "remote IR sensor",
>> [0x8e >> 1] = "remote IR sensor",
>> [0x86 >> 1] = "tda9887",
>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
>> index 3598221..631e252 100644
>> --- a/drivers/media/usb/em28xx/em28xx-input.c
>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
>> @@ -5,6 +5,7 @@
>> Markus Rechberger <mrechberger@gmail.com>
>> Mauro Carvalho Chehab <mchehab@infradead.org>
>> Sascha Sommer <saschasommer@freenet.de>
>> + Copyright (C) 2012 Frank Schäfer <fschaefer.oss@googlemail.com>
>>
>> This program is free software; you can redistribute it and/or modify
>> it under the terms of the GNU General Public License as published by
>> @@ -34,6 +35,8 @@
>> #define EM28XX_SBUTTON_QUERY_INTERVAL 500
>> #define EM28XX_R0C_USBSUSP_SNAPSHOT 0x20
>>
>> +#define EM28XX_RC_QUERY_INTERVAL 100
>> +
>> static unsigned int ir_debug;
>> module_param(ir_debug, int, 0644);
>> MODULE_PARM_DESC(ir_debug, "enable debug messages [IR]");
>> @@ -67,13 +70,14 @@ struct em28xx_IR {
>> char name[32];
>> char phys[32];
>>
>> - /* poll external decoder */
>> int polling;
>> struct delayed_work work;
>> unsigned int full_code:1;
>> unsigned int last_readcount;
>> u64 rc_type;
>>
>> + struct i2c_client *i2c_dev; /* external i2c IR receiver/decoder */
>> +
>> int (*get_key)(struct em28xx_IR *, struct em28xx_ir_poll_result *);
>> };
>>
>> @@ -452,7 +456,7 @@ static int em28xx_ir_change_protocol(struct rc_dev *rc_dev, u64 *rc_type)
>> }
>> }
>>
>> -static void em28xx_register_i2c_ir(struct em28xx *dev)
>> +static int em28xx_register_i2c_ir(struct em28xx *dev, struct rc_dev *rc_dev)
>> {
>> /* Leadtek winfast tv USBII deluxe can find a non working IR-device */
>> /* at address 0x18, so if that address is needed for another board in */
>> @@ -470,30 +474,46 @@ static void em28xx_register_i2c_ir(struct em28xx *dev)
>> switch (dev->model) {
>> case EM2800_BOARD_TERRATEC_CINERGY_200:
>> case EM2820_BOARD_TERRATEC_CINERGY_250:
>> - dev->init_data.ir_codes = RC_MAP_EM_TERRATEC;
>> - dev->init_data.get_key = em28xx_get_key_terratec;
>> dev->init_data.name = "i2c IR (EM28XX Terratec)";
>> + dev->init_data.type = RC_BIT_OTHER;
>> + dev->init_data.get_key = em28xx_get_key_terratec;
>> break;
>> case EM2820_BOARD_PINNACLE_USB_2:
>> - dev->init_data.ir_codes = RC_MAP_PINNACLE_GREY;
>> - dev->init_data.get_key = em28xx_get_key_pinnacle_usb_grey;
>> dev->init_data.name = "i2c IR (EM28XX Pinnacle PCTV)";
>> + dev->init_data.type = RC_BIT_OTHER;
>> + dev->init_data.get_key = em28xx_get_key_pinnacle_usb_grey;
>> break;
>> case EM2820_BOARD_HAUPPAUGE_WINTV_USB_2:
>> - dev->init_data.ir_codes = RC_MAP_HAUPPAUGE;
>> - dev->init_data.get_key = em28xx_get_key_em_haup;
>> dev->init_data.name = "i2c IR (EM2840 Hauppauge)";
>> + dev->init_data.type = RC_BIT_RC5;
>> + dev->init_data.get_key = em28xx_get_key_em_haup;
>> break;
>> case EM2820_BOARD_LEADTEK_WINFAST_USBII_DELUXE:
>> - dev->init_data.ir_codes = RC_MAP_WINFAST_USBII_DELUXE;
>> - dev->init_data.get_key = em28xx_get_key_winfast_usbii_deluxe;
>> dev->init_data.name = "i2c IR (EM2820 Winfast TV USBII Deluxe)";
>> + dev->init_data.type = RC_BIT_OTHER;
>> + dev->init_data.get_key = em28xx_get_key_winfast_usbii_deluxe;
>> break;
>> }
>>
>> - if (dev->init_data.name)
>> + if (dev->init_data.name && dev->board.ir_codes) {
>> + dev->init_data.ir_codes = dev->board.ir_codes;
>> + dev->init_data.polling_interval = EM28XX_RC_QUERY_INTERVAL;
>> + dev->init_data.rc_dev = rc_dev;
>> info.platform_data = &dev->init_data;
>> - i2c_new_probed_device(&dev->i2c_adap, &info, addr_list, NULL);
>> + } else {
>> + em28xx_warn("Unknown i2c remote control device.\n");
>> + em28xx_warn("If the remote control doesn't work properly, please contact <linux-media@vger.kernel.org>\n");
>> + }
>> +
>> + dev->ir->i2c_dev = i2c_new_probed_device(&dev->i2c_adap, &info, addr_list, NULL);
>> + if (NULL == dev->ir->i2c_dev)
>> + return -ENODEV;
>> +
>> +#if defined(CONFIG_MODULES) && defined(MODULE)
>> + request_module("ir-kbd-i2c");
>> +#endif
>> +
>> + return 0;
>> }
>>
>> /**********************************************************
>> @@ -590,7 +610,7 @@ static int em28xx_ir_init(struct em28xx *dev)
>> int err = -ENOMEM;
>> u64 rc_type;
>>
>> - if (dev->board.ir_codes == NULL) {
>> + if (dev->board.ir_codes == NULL && !dev->board.has_ir_i2c) {
>> /* No remote control support */
>> em28xx_warn("Remote control support is not available for "
>> "this card.\n");
>> @@ -607,68 +627,56 @@ static int em28xx_ir_init(struct em28xx *dev)
>> dev->ir = ir;
>> ir->rc = rc;
>>
>> - /*
>> - * em2874 supports more protocols. For now, let's just announce
>> - * the two protocols that were already tested
>> - */
>> - rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC;
>> - rc->priv = ir;
>> - rc->change_protocol = em28xx_ir_change_protocol;
>> - rc->open = em28xx_ir_start;
>> - rc->close = em28xx_ir_stop;
>> -
>> - switch (dev->chip_id) {
>> - case CHIP_ID_EM2860:
>> - case CHIP_ID_EM2883:
>> - rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC;
>> - break;
>> - case CHIP_ID_EM2884:
>> - case CHIP_ID_EM2874:
>> - case CHIP_ID_EM28174:
>> - rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC | RC_BIT_RC6_0;
>> - break;
>> - default:
>> - err = -ENODEV;
>> - goto error;
>> - }
>> -
>> - /* By default, keep protocol field untouched */
>> - rc_type = RC_BIT_UNKNOWN;
>> - err = em28xx_ir_change_protocol(rc, &rc_type);
>> - if (err)
>> - goto error;
>> -
>> - /* This is how often we ask the chip for IR information */
>> - ir->polling = 100; /* ms */
>> -
>> - /* init input device */
>> - snprintf(ir->name, sizeof(ir->name), "em28xx IR (%s)",
>> - dev->name);
>> -
>> + snprintf(ir->name, sizeof(ir->name), "em28xx IR (%s)", dev->name);
>> usb_make_path(dev->udev, ir->phys, sizeof(ir->phys));
>> strlcat(ir->phys, "/input0", sizeof(ir->phys));
>> + ir->polling = EM28XX_RC_QUERY_INTERVAL;
>>
>> - rc->input_name = ir->name;
>> - rc->input_phys = ir->phys;
>> - rc->input_id.bustype = BUS_USB;
>> rc->input_id.version = 1;
>> rc->input_id.vendor = le16_to_cpu(dev->udev->descriptor.idVendor);
>> rc->input_id.product = le16_to_cpu(dev->udev->descriptor.idProduct);
>> rc->dev.parent = &dev->udev->dev;
>> - rc->map_name = dev->board.ir_codes;
>> rc->driver_name = MODULE_NAME;
>>
>> - /* all done */
>> - err = rc_register_device(rc);
>> - if (err)
>> - goto error;
>> -
>> - em28xx_register_i2c_ir(dev);
>> + if (dev->board.has_ir_i2c) {
>> + err = em28xx_register_i2c_ir(dev, rc);
>> + if (err < 0)
>> + goto error;
>> + } else {
>> + switch (dev->chip_id) {
>> + case CHIP_ID_EM2860:
>> + case CHIP_ID_EM2883:
>> + rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC;
>> + break;
>> + case CHIP_ID_EM2884:
>> + case CHIP_ID_EM2874:
>> + case CHIP_ID_EM28174:
>> + rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC | RC_BIT_RC6_0;
>> + break;
>> + default:
>> + err = -ENODEV;
>> + goto error;
>> + }
>> + rc->priv = ir;
>> + rc->change_protocol = em28xx_ir_change_protocol;
>> + rc->open = em28xx_ir_start;
>> + rc->close = em28xx_ir_stop;
>> + rc->input_name = ir->name;
>> + rc->input_phys = ir->phys;
>> + rc->input_id.bustype = BUS_USB;
>> + rc->map_name = dev->board.ir_codes;
>> +
>> + /* By default, keep protocol field untouched */
>> + rc_type = RC_BIT_UNKNOWN;
>> + err = em28xx_ir_change_protocol(rc, &rc_type);
>> + if (err)
>> + goto error;
>> +
>> + err = rc_register_device(rc);
>> + if (err < 0)
>> + goto error;
>> + }
>>
>> -#if defined(CONFIG_MODULES) && defined(MODULE)
>> - if (dev->board.has_ir_i2c)
>> - request_module("ir-kbd-i2c");
>> -#endif
>> if (dev->board.has_snapshot_button)
>> em28xx_register_snapshot_button(dev);
>>
>> @@ -691,7 +699,9 @@ static int em28xx_ir_fini(struct em28xx *dev)
>> if (!ir)
>> return 0;
>>
>> - if (ir->rc)
>> + if (ir->i2c_dev)
>> + i2c_unregister_device(ir->i2c_dev);
>> + else if (ir->rc)
>> rc_unregister_device(ir->rc);
>>
>> /* done */
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 3/6] em28xx: make remote controls of devices with external IR IC working again
2013-01-05 12:58 ` Frank Schäfer
@ 2013-01-05 13:26 ` Mauro Carvalho Chehab
2013-01-05 13:51 ` Frank Schäfer
0 siblings, 1 reply; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2013-01-05 13:26 UTC (permalink / raw)
To: Frank Schäfer; +Cc: linux-media
Em Sat, 05 Jan 2013 13:58:20 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> Am 04.01.2013 22:12, schrieb Mauro Carvalho Chehab:
> > Em Fri, 28 Dec 2012 00:02:45 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Tested with device "Terratec Cinergy 200 USB".
> > Sorry, but this patch is completely wrong ;)
>
> Completely wrong ? That's not helpful...
> Please elaborate a bit more on this so that I can do things right next
> time. ;)
Sorry, I was too busy yesterday with the tests to elaborate it more.
In general, big patches like that to fix bug fixes are generally wrong:
they touch on a lot of the code and it is hard to be sure that it doesn't
come with regressions on it.
In this particular case, it was:
- mixing bug fixes with some other random stuff;
- moving only one part of the IR needed data elsewhere (it were
moving the IR tables, to the board descriptions, keeping them on
a separate part of the code, obfuscating the code);
- putting a large amount of the code inside an if, increasing the
driver's complexity with no need;
- initializing some data for IR that are never used, at em28xx_ir_init;
- not fixing the snapshot button.
The bug fix was as simple as:
1) move snapshot button register to happen before IR;
2) move I2C init to happen before the em2860/2874 IR init.
See:
http://git.linuxtv.org/media_tree.git/commitdiff/8303dc9952758ab3060a3ee9a19ecb6fec83c600
That's simple to review, and the produced patch can be accepted later at
stable, because it is not a code rewrite.
Of course, if we backport this to -stable, this one is also recommended:
http://git.linuxtv.org/media_tree.git/commitdiff/728f9778e273a11a65926ac21574e6ca8d911ebf
in order to autoload the RC extension automatically for I2C IR's, but this
is a separate bug.
Btw, I really prefer to have the RC tables for the I2C devices inside
em28xx-input, as:
1) there are other board-specific platform_data that needed to
be filled for the IR to work there;
2) we want to keep all those platform_data initialization together,
to make the code simpler to maintain;
3) moving all those data to em28xx cards struct is a bad idea, as
newer em28xx won't use I2C IR's, as the new chipsets have already
its own IR decoder. Moving those 4-5 fields to the board struct
would increase its size for every board. So, it would be a waste
of space.
Regards,
Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/6] em28xx: make remote controls of devices with external IR IC working again
2013-01-05 13:26 ` Mauro Carvalho Chehab
@ 2013-01-05 13:51 ` Frank Schäfer
0 siblings, 0 replies; 20+ messages in thread
From: Frank Schäfer @ 2013-01-05 13:51 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media
Am 05.01.2013 14:26, schrieb Mauro Carvalho Chehab:
> Em Sat, 05 Jan 2013 13:58:20 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 04.01.2013 22:12, schrieb Mauro Carvalho Chehab:
>>> Em Fri, 28 Dec 2012 00:02:45 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> Tested with device "Terratec Cinergy 200 USB".
>>> Sorry, but this patch is completely wrong ;)
>> Completely wrong ? That's not helpful...
>> Please elaborate a bit more on this so that I can do things right next
>> time. ;)
> Sorry, I was too busy yesterday with the tests to elaborate it more.
>
> In general, big patches like that to fix bug fixes are generally wrong:
> they touch on a lot of the code and it is hard to be sure that it doesn't
> come with regressions on it.
Ok, I think you are right.
The patch description was definitely insufficient. I should have
explained the issues I tried to address in more details.
Maybe I should have split this into several smaller patches, too.
> In this particular case, it was:
> - mixing bug fixes with some other random stuff;
> - moving only one part of the IR needed data elsewhere (it were
> moving the IR tables, to the board descriptions, keeping them on
> a separate part of the code, obfuscating the code);
> - putting a large amount of the code inside an if, increasing the
> driver's complexity with no need;
> - initializing some data for IR that are never used, at em28xx_ir_init;
> - not fixing the snapshot button.
>
> The bug fix was as simple as:
> 1) move snapshot button register to happen before IR;
> 2) move I2C init to happen before the em2860/2874 IR init.
See the mail I've sent a few minutes ago.
> ...
>
> Btw, I really prefer to have the RC tables for the I2C devices inside
> em28xx-input, as:
>
> 1) there are other board-specific platform_data that needed to
> be filled for the IR to work there;
Sure.
> 2) we want to keep all those platform_data initialization together,
> to make the code simpler to maintain;
platform_data initialization is kept together, no changes here.
To me it seems to be important to keep all _board_ specific stuff
together as much as possible.
> 3) moving all those data to em28xx cards struct is a bad idea, as
> newer em28xx won't use I2C IR's, as the new chipsets have already
> its own IR decoder. Moving those 4-5 fields to the board struct
> would increase its size for every board. So, it would be a waste
> of space.
Im my opinion, having board specifc code spread all over the code is a
desease. It makes the code bug prone.
Actually, this was one of the reasons why the i2c rc got broken...
Sure, it's hard to avoid, especially for the DVB stuff. But we should at
least reduce it to a minimum.
For the RC map, it's easy to do this, as the corresponding field is
already there.
Regards,
Frank
>
> Regards,
> Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/6] em28xx: IR RC: get rid of function em28xx_get_key_terratec()
2012-12-27 23:02 [PATCH 0/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
` (2 preceding siblings ...)
2012-12-27 23:02 ` [PATCH 3/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
@ 2012-12-27 23:02 ` Frank Schäfer
2013-01-05 2:41 ` Mauro Carvalho Chehab
2012-12-27 23:02 ` [PATCH 5/6] em28xx: IR RC: move assignment of get_key functions from *_change_protocol() functions to em28xx_ir_init() Frank Schäfer
2012-12-27 23:02 ` [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1() Frank Schäfer
5 siblings, 1 reply; 20+ messages in thread
From: Frank Schäfer @ 2012-12-27 23:02 UTC (permalink / raw)
To: mchehab; +Cc: linux-media, Frank Schäfer
Module "ir-kbd-i2c" already provides this function as IR_KBD_GET_KEY_KNC1.
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
drivers/media/usb/em28xx/em28xx-input.c | 30 +-----------------------------
1 Datei geändert, 1 Zeile hinzugefügt(+), 29 Zeilen entfernt(-)
diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index 631e252..62b6cb7 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -85,34 +85,6 @@ struct em28xx_IR {
I2C IR based get keycodes - should be used with ir-kbd-i2c
**********************************************************/
-static int em28xx_get_key_terratec(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
-{
- unsigned char b;
-
- /* poll IR chip */
- if (1 != i2c_master_recv(ir->c, &b, 1)) {
- i2cdprintk("read error\n");
- return -EIO;
- }
-
- /* it seems that 0xFE indicates that a button is still hold
- down, while 0xff indicates that no button is hold
- down. 0xfe sequences are sometimes interrupted by 0xFF */
-
- i2cdprintk("key %02x\n", b);
-
- if (b == 0xff)
- return 0;
-
- if (b == 0xfe)
- /* keep old data */
- return 1;
-
- *ir_key = b;
- *ir_raw = b;
- return 1;
-}
-
static int em28xx_get_key_em_haup(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
{
unsigned char buf[2];
@@ -476,7 +448,7 @@ static int em28xx_register_i2c_ir(struct em28xx *dev, struct rc_dev *rc_dev)
case EM2820_BOARD_TERRATEC_CINERGY_250:
dev->init_data.name = "i2c IR (EM28XX Terratec)";
dev->init_data.type = RC_BIT_OTHER;
- dev->init_data.get_key = em28xx_get_key_terratec;
+ dev->init_data.internal_get_key_func = IR_KBD_GET_KEY_KNC1;
break;
case EM2820_BOARD_PINNACLE_USB_2:
dev->init_data.name = "i2c IR (EM28XX Pinnacle PCTV)";
--
1.7.10.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/6] em28xx: IR RC: get rid of function em28xx_get_key_terratec()
2012-12-27 23:02 ` [PATCH 4/6] em28xx: IR RC: get rid of function em28xx_get_key_terratec() Frank Schäfer
@ 2013-01-05 2:41 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2013-01-05 2:41 UTC (permalink / raw)
To: Frank Schäfer; +Cc: linux-media
Em Fri, 28 Dec 2012 00:02:46 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> Module "ir-kbd-i2c" already provides this function as IR_KBD_GET_KEY_KNC1.
See my comment for patch 6/6.
Regards,
Mauro
>
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
> drivers/media/usb/em28xx/em28xx-input.c | 30 +-----------------------------
> 1 Datei geändert, 1 Zeile hinzugefügt(+), 29 Zeilen entfernt(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> index 631e252..62b6cb7 100644
> --- a/drivers/media/usb/em28xx/em28xx-input.c
> +++ b/drivers/media/usb/em28xx/em28xx-input.c
> @@ -85,34 +85,6 @@ struct em28xx_IR {
> I2C IR based get keycodes - should be used with ir-kbd-i2c
> **********************************************************/
>
> -static int em28xx_get_key_terratec(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
> -{
> - unsigned char b;
> -
> - /* poll IR chip */
> - if (1 != i2c_master_recv(ir->c, &b, 1)) {
> - i2cdprintk("read error\n");
> - return -EIO;
> - }
> -
> - /* it seems that 0xFE indicates that a button is still hold
> - down, while 0xff indicates that no button is hold
> - down. 0xfe sequences are sometimes interrupted by 0xFF */
> -
> - i2cdprintk("key %02x\n", b);
> -
> - if (b == 0xff)
> - return 0;
> -
> - if (b == 0xfe)
> - /* keep old data */
> - return 1;
> -
> - *ir_key = b;
> - *ir_raw = b;
> - return 1;
> -}
> -
> static int em28xx_get_key_em_haup(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
> {
> unsigned char buf[2];
> @@ -476,7 +448,7 @@ static int em28xx_register_i2c_ir(struct em28xx *dev, struct rc_dev *rc_dev)
> case EM2820_BOARD_TERRATEC_CINERGY_250:
> dev->init_data.name = "i2c IR (EM28XX Terratec)";
> dev->init_data.type = RC_BIT_OTHER;
> - dev->init_data.get_key = em28xx_get_key_terratec;
> + dev->init_data.internal_get_key_func = IR_KBD_GET_KEY_KNC1;
> break;
> case EM2820_BOARD_PINNACLE_USB_2:
> dev->init_data.name = "i2c IR (EM28XX Pinnacle PCTV)";
--
Cheers,
Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/6] em28xx: IR RC: move assignment of get_key functions from *_change_protocol() functions to em28xx_ir_init()
2012-12-27 23:02 [PATCH 0/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
` (3 preceding siblings ...)
2012-12-27 23:02 ` [PATCH 4/6] em28xx: IR RC: get rid of function em28xx_get_key_terratec() Frank Schäfer
@ 2012-12-27 23:02 ` Frank Schäfer
2012-12-27 23:02 ` [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1() Frank Schäfer
5 siblings, 0 replies; 20+ messages in thread
From: Frank Schäfer @ 2012-12-27 23:02 UTC (permalink / raw)
To: mchehab; +Cc: linux-media, Frank Schäfer
The get_key functions are independent from the selected protocol, so assign
them once only at device initialization.
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
drivers/media/usb/em28xx/em28xx-input.c | 6 ++----
1 Datei geändert, 2 Zeilen hinzugefügt(+), 4 Zeilen entfernt(-)
diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index 62b6cb7..186820a 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -360,7 +360,6 @@ static int em2860_ir_change_protocol(struct rc_dev *rc_dev, u64 *rc_type)
*rc_type = ir->rc_type;
return -EINVAL;
}
- ir->get_key = default_polling_getkey;
em28xx_write_reg_bits(dev, EM28XX_R0F_XCLK, dev->board.xclk,
EM28XX_XCLK_IR_RC5_MODE);
@@ -396,10 +395,7 @@ static int em2874_ir_change_protocol(struct rc_dev *rc_dev, u64 *rc_type)
*rc_type = ir->rc_type;
return -EINVAL;
}
-
- ir->get_key = em2874_polling_getkey;
em28xx_write_regs(dev, EM2874_R50_IR_CONFIG, &ir_config, 1);
-
em28xx_write_reg_bits(dev, EM28XX_R0F_XCLK, dev->board.xclk,
EM28XX_XCLK_IR_RC5_MODE);
@@ -618,11 +614,13 @@ static int em28xx_ir_init(struct em28xx *dev)
switch (dev->chip_id) {
case CHIP_ID_EM2860:
case CHIP_ID_EM2883:
+ ir->get_key = default_polling_getkey;
rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC;
break;
case CHIP_ID_EM2884:
case CHIP_ID_EM2874:
case CHIP_ID_EM28174:
+ ir->get_key = em2874_polling_getkey;
rc->allowed_protos = RC_BIT_RC5 | RC_BIT_NEC | RC_BIT_RC6_0;
break;
default:
--
1.7.10.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1()
2012-12-27 23:02 [PATCH 0/6] em28xx: make remote controls of devices with external IR IC working again Frank Schäfer
` (4 preceding siblings ...)
2012-12-27 23:02 ` [PATCH 5/6] em28xx: IR RC: move assignment of get_key functions from *_change_protocol() functions to em28xx_ir_init() Frank Schäfer
@ 2012-12-27 23:02 ` Frank Schäfer
2013-01-05 2:39 ` Mauro Carvalho Chehab
5 siblings, 1 reply; 20+ messages in thread
From: Frank Schäfer @ 2012-12-27 23:02 UTC (permalink / raw)
To: mchehab; +Cc: linux-media, Frank Schäfer
- return valid key code when button is hold
- debug: print key code only when a button is pressed
Tested with device "Terratec Cinergy 200 USB" (em28xx).
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
drivers/media/i2c/ir-kbd-i2c.c | 15 +++++----------
1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)
diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
index 08ae067..2984b7d 100644
--- a/drivers/media/i2c/ir-kbd-i2c.c
+++ b/drivers/media/i2c/ir-kbd-i2c.c
@@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
return -EIO;
}
- /* it seems that 0xFE indicates that a button is still hold
- down, while 0xff indicates that no button is hold
- down. 0xfe sequences are sometimes interrupted by 0xFF */
-
- dprintk(2,"key %02x\n", b);
-
- if (b == 0xff)
+ if (b == 0xff) /* no button */
return 0;
- if (b == 0xfe)
- /* keep old data */
- return 1;
+ if (b == 0xfe) /* button is still hold */
+ b = ir->rc->last_scancode; /* keep old data */
+
+ dprintk(2,"key %02x\n", b);
*ir_key = b;
*ir_raw = b;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1()
2012-12-27 23:02 ` [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1() Frank Schäfer
@ 2013-01-05 2:39 ` Mauro Carvalho Chehab
2013-01-05 13:32 ` Frank Schäfer
0 siblings, 1 reply; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2013-01-05 2:39 UTC (permalink / raw)
To: Frank Schäfer; +Cc: linux-media
Em Fri, 28 Dec 2012 00:02:48 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> - return valid key code when button is hold
> - debug: print key code only when a button is pressed
>
> Tested with device "Terratec Cinergy 200 USB" (em28xx).
>
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
> drivers/media/i2c/ir-kbd-i2c.c | 15 +++++----------
> 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)
>
> diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
> index 08ae067..2984b7d 100644
> --- a/drivers/media/i2c/ir-kbd-i2c.c
> +++ b/drivers/media/i2c/ir-kbd-i2c.c
> @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
> return -EIO;
> }
>
> - /* it seems that 0xFE indicates that a button is still hold
> - down, while 0xff indicates that no button is hold
> - down. 0xfe sequences are sometimes interrupted by 0xFF */
> -
> - dprintk(2,"key %02x\n", b);
> -
> - if (b == 0xff)
> + if (b == 0xff) /* no button */
> return 0;
>
> - if (b == 0xfe)
> - /* keep old data */
> - return 1;
> + if (b == 0xfe) /* button is still hold */
> + b = ir->rc->last_scancode; /* keep old data */
> +
> + dprintk(2,"key %02x\n", b);
>
> *ir_key = b;
> *ir_raw = b;
Don't do that. This piece of code is old, and it was added there
before the em28xx driver. Originally, the ir-i2c-kbd were used by
bttv and saa7134 drivers and the code there were auto-detecting the
I2C IR hardware decoding chips that used to be very common on media
devices. I'm almost sure that the original device that started using
this code is this model:
drivers/media/pci/bt8xx/bttv-cards.c: .name = "Typhoon TView RDS + FM Stereo / KNC1 TV Station RDS",
That's why it is called as KNC1, but there are other cards that use
it as well. I think I have one bttv using it. Not sure.
The routine on em28xx is a fork of the original one, that was changed
to work with the devices there.
FYI, most of those I2C IR codes are provided by some generic 8-bits
micro-processor, generally labeled with weird names, like KS007.
The code inside those can be different, depending on the firmware
inside, and also its I2C address.
That's one of the reasons why we moved the code that used to be
inside ir-i2c-kbd into the drivers that actually use it, like
em28xx: this way, we can track its usage and fix, as the remaining
get_key code inside-i2c-kbd are old, auto-detected, nobody knows
precisely what devices use them, and the current developers don't own
the hardware where they're used.
In other words, please, don't touch at the get_key routines inside
ir-kbd-i2c. If you find a bug, please fix at em28xx-input instead, if
you find a bug.
--
Cheers,
Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1()
2013-01-05 2:39 ` Mauro Carvalho Chehab
@ 2013-01-05 13:32 ` Frank Schäfer
2013-01-05 15:25 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 20+ messages in thread
From: Frank Schäfer @ 2013-01-05 13:32 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List
Am 05.01.2013 03:39, schrieb Mauro Carvalho Chehab:
> Em Fri, 28 Dec 2012 00:02:48 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> - return valid key code when button is hold
>> - debug: print key code only when a button is pressed
>>
>> Tested with device "Terratec Cinergy 200 USB" (em28xx).
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>> drivers/media/i2c/ir-kbd-i2c.c | 15 +++++----------
>> 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
>> index 08ae067..2984b7d 100644
>> --- a/drivers/media/i2c/ir-kbd-i2c.c
>> +++ b/drivers/media/i2c/ir-kbd-i2c.c
>> @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
>> return -EIO;
>> }
>>
>> - /* it seems that 0xFE indicates that a button is still hold
>> - down, while 0xff indicates that no button is hold
>> - down. 0xfe sequences are sometimes interrupted by 0xFF */
>> -
>> - dprintk(2,"key %02x\n", b);
>> -
>> - if (b == 0xff)
>> + if (b == 0xff) /* no button */
>> return 0;
>>
>> - if (b == 0xfe)
>> - /* keep old data */
>> - return 1;
>> + if (b == 0xfe) /* button is still hold */
>> + b = ir->rc->last_scancode; /* keep old data */
>> +
>> + dprintk(2,"key %02x\n", b);
>>
>> *ir_key = b;
>> *ir_raw = b;
> Don't do that. This piece of code is old, and it was added there
> before the em28xx driver. Originally, the ir-i2c-kbd were used by
> bttv and saa7134 drivers and the code there were auto-detecting the
> I2C IR hardware decoding chips that used to be very common on media
> devices. I'm almost sure that the original device that started using
> this code is this model:
>
> drivers/media/pci/bt8xx/bttv-cards.c: .name = "Typhoon TView RDS + FM Stereo / KNC1 TV Station RDS",
>
> That's why it is called as KNC1, but there are other cards that use
> it as well. I think I have one bttv using it. Not sure.
>
> The routine on em28xx is a fork of the original one, that was changed
> to work with the devices there.
Indeed, it's a fork, 100% identical:
static int em28xx_get_key_terratec(struct IR_i2c *ir, u32 *ir_key, u32
*ir_raw)
{
unsigned char b;
/* poll IR chip */
if (1 != i2c_master_recv(ir->c, &b, 1)) {
i2cdprintk("read error\n");
return -EIO;
}
/* it seems that 0xFE indicates that a button is still hold
down, while 0xff indicates that no button is hold
down. 0xfe sequences are sometimes interrupted by 0xFF */
i2cdprintk("key %02x\n", b);
if (b == 0xff)
return 0;
if (b == 0xfe)
/* keep old data */
return 1;
*ir_key = b;
*ir_raw = b;
return 1;
}
static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
{
unsigned char b;
/* poll IR chip */
if (1 != i2c_master_recv(ir->c, &b, 1)) {
dprintk(1,"read error\n");
return -EIO;
}
/* it seems that 0xFE indicates that a button is still hold
down, while 0xff indicates that no button is hold
down. 0xfe sequences are sometimes interrupted by 0xFF */
dprintk(2,"key %02x\n", b);
if (b == 0xff)
return 0;
if (b == 0xfe)
/* keep old data */
return 1;
*ir_key = b;
*ir_raw = b;
return 1;
}
Why should we keep two 100% identical functions ? See patch 4/6.
I'm 99% sure that both devices are absolutely identical.
Concerning the fix I'm suggesting here:
First of all, I have to say that the Terratec RC works even without this
patch.
Nevertheless, I think the function should really return valid values for
ir_key and ir_raw when 0xfe=button hold is received. Especially because
the function succeeds.
This also allows us to make u32 ir_key, ir_raw in ir_key_poll() in
ir-kbd-i2c.c non-static.
While I agree that we should be careful, I can't see how this can cause
any trouble.
The second thing is the small fix for the key code debug output. Don't
you think it makes sense ?
Regards,
Frank
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1()
2013-01-05 13:32 ` Frank Schäfer
@ 2013-01-05 15:25 ` Mauro Carvalho Chehab
2013-01-06 20:32 ` Frank Schäfer
0 siblings, 1 reply; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2013-01-05 15:25 UTC (permalink / raw)
To: Frank Schäfer; +Cc: Linux Media Mailing List
Em Sat, 05 Jan 2013 14:32:30 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> Am 05.01.2013 03:39, schrieb Mauro Carvalho Chehab:
> > Em Fri, 28 Dec 2012 00:02:48 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> - return valid key code when button is hold
> >> - debug: print key code only when a button is pressed
> >>
> >> Tested with device "Terratec Cinergy 200 USB" (em28xx).
> >>
> >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >> ---
> >> drivers/media/i2c/ir-kbd-i2c.c | 15 +++++----------
> >> 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)
> >>
> >> diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
> >> index 08ae067..2984b7d 100644
> >> --- a/drivers/media/i2c/ir-kbd-i2c.c
> >> +++ b/drivers/media/i2c/ir-kbd-i2c.c
> >> @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
> >> return -EIO;
> >> }
> >>
> >> - /* it seems that 0xFE indicates that a button is still hold
> >> - down, while 0xff indicates that no button is hold
> >> - down. 0xfe sequences are sometimes interrupted by 0xFF */
> >> -
> >> - dprintk(2,"key %02x\n", b);
> >> -
> >> - if (b == 0xff)
> >> + if (b == 0xff) /* no button */
> >> return 0;
> >>
> >> - if (b == 0xfe)
> >> - /* keep old data */
> >> - return 1;
> >> + if (b == 0xfe) /* button is still hold */
> >> + b = ir->rc->last_scancode; /* keep old data */
> >> +
> >> + dprintk(2,"key %02x\n", b);
> >>
> >> *ir_key = b;
> >> *ir_raw = b;
> > Don't do that. This piece of code is old, and it was added there
> > before the em28xx driver. Originally, the ir-i2c-kbd were used by
> > bttv and saa7134 drivers and the code there were auto-detecting the
> > I2C IR hardware decoding chips that used to be very common on media
> > devices. I'm almost sure that the original device that started using
> > this code is this model:
> >
> > drivers/media/pci/bt8xx/bttv-cards.c: .name = "Typhoon TView RDS + FM Stereo / KNC1 TV Station RDS",
> >
> > That's why it is called as KNC1, but there are other cards that use
> > it as well. I think I have one bttv using it. Not sure.
> >
> > The routine on em28xx is a fork of the original one, that was changed
> > to work with the devices there.
>
> Indeed, it's a fork, 100% identical:
>
>
> static int em28xx_get_key_terratec(struct IR_i2c *ir, u32 *ir_key, u32
> *ir_raw)
> {
> unsigned char b;
>
> /* poll IR chip */
> if (1 != i2c_master_recv(ir->c, &b, 1)) {
> i2cdprintk("read error\n");
> return -EIO;
> }
>
> /* it seems that 0xFE indicates that a button is still hold
> down, while 0xff indicates that no button is hold
> down. 0xfe sequences are sometimes interrupted by 0xFF */
>
> i2cdprintk("key %02x\n", b);
>
> if (b == 0xff)
> return 0;
>
> if (b == 0xfe)
> /* keep old data */
> return 1;
>
> *ir_key = b;
> *ir_raw = b;
> return 1;
> }
>
>
>
>
> static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
> {
> unsigned char b;
>
> /* poll IR chip */
> if (1 != i2c_master_recv(ir->c, &b, 1)) {
> dprintk(1,"read error\n");
> return -EIO;
> }
>
> /* it seems that 0xFE indicates that a button is still hold
> down, while 0xff indicates that no button is hold
> down. 0xfe sequences are sometimes interrupted by 0xFF */
>
> dprintk(2,"key %02x\n", b);
>
> if (b == 0xff)
> return 0;
>
> if (b == 0xfe)
> /* keep old data */
> return 1;
>
> *ir_key = b;
> *ir_raw = b;
> return 1;
> }
>
>
>
> Why should we keep two 100% identical functions ? See patch 4/6.
> I'm 99% sure that both devices are absolutely identical.
99% sure is not enough. A simple firmware difference at the microprocessor
is enough to make the devices different.
Also, this was widely discussed several years ago, when we decided to cleanup
the I2C code. We then invested lot of efforts to move those get_keys away
from ir-i2c-kbd. The only things left there are the ones we identified that
were needed by auto-detection mode on old devices that we don't have.
What was decided is to move everything that we know to the *-input driver,
keeping there only the legacy stuff.
> Concerning the fix I'm suggesting here:
> First of all, I have to say that the Terratec RC works even without this
> patch.
> Nevertheless, I think the function should really return valid values for
> ir_key and ir_raw when 0xfe=button hold is received. Especially because
> the function succeeds.
> This also allows us to make u32 ir_key, ir_raw in ir_key_poll() in
> ir-kbd-i2c.c non-static.
> While I agree that we should be careful, I can't see how this can cause
> any trouble.
Ok, the net effect is the same, except that the current way is faster, as it
will skip some code that would simply put the value that it is already at
ir_key/ir_raw again.
As this polling code is known to cause performance loss on some machines,
the quickest, the best. Also, the better is to report long press events
on a different way, as the input core can handle those on a different way
(that's why there are there keyup/keydown kABI calls). A patch for better
handle rc=1 return code at ir-i2c-kbd is needed.
In any case, I don't see any need for patches 4/6 or 6/6.
> The second thing is the small fix for the key code debug output. Don't
> you think it makes sense ?
Now that we have "ir-keycode -t", all those key/scancode printk's inside
the driver are pretty much useless, as both are reported as events.
In the past, when most of the RC code were written, those prints were
needed, as the scancode weren't reported to userspace.
Regards,
Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1()
2013-01-05 15:25 ` Mauro Carvalho Chehab
@ 2013-01-06 20:32 ` Frank Schäfer
2013-01-07 16:40 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 20+ messages in thread
From: Frank Schäfer @ 2013-01-06 20:32 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List
Am 05.01.2013 16:25, schrieb Mauro Carvalho Chehab:
> Em Sat, 05 Jan 2013 14:32:30 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 05.01.2013 03:39, schrieb Mauro Carvalho Chehab:
>>> Em Fri, 28 Dec 2012 00:02:48 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> - return valid key code when button is hold
>>>> - debug: print key code only when a button is pressed
>>>>
>>>> Tested with device "Terratec Cinergy 200 USB" (em28xx).
>>>>
>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>> ---
>>>> drivers/media/i2c/ir-kbd-i2c.c | 15 +++++----------
>>>> 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)
>>>>
>>>> diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
>>>> index 08ae067..2984b7d 100644
>>>> --- a/drivers/media/i2c/ir-kbd-i2c.c
>>>> +++ b/drivers/media/i2c/ir-kbd-i2c.c
>>>> @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
>>>> return -EIO;
>>>> }
>>>>
>>>> - /* it seems that 0xFE indicates that a button is still hold
>>>> - down, while 0xff indicates that no button is hold
>>>> - down. 0xfe sequences are sometimes interrupted by 0xFF */
>>>> -
>>>> - dprintk(2,"key %02x\n", b);
>>>> -
>>>> - if (b == 0xff)
>>>> + if (b == 0xff) /* no button */
>>>> return 0;
>>>>
>>>> - if (b == 0xfe)
>>>> - /* keep old data */
>>>> - return 1;
>>>> + if (b == 0xfe) /* button is still hold */
>>>> + b = ir->rc->last_scancode; /* keep old data */
>>>> +
>>>> + dprintk(2,"key %02x\n", b);
>>>>
>>>> *ir_key = b;
>>>> *ir_raw = b;
>>> Don't do that. This piece of code is old, and it was added there
>>> before the em28xx driver. Originally, the ir-i2c-kbd were used by
>>> bttv and saa7134 drivers and the code there were auto-detecting the
>>> I2C IR hardware decoding chips that used to be very common on media
>>> devices. I'm almost sure that the original device that started using
>>> this code is this model:
>>>
>>> drivers/media/pci/bt8xx/bttv-cards.c: .name = "Typhoon TView RDS + FM Stereo / KNC1 TV Station RDS",
>>>
>>> That's why it is called as KNC1, but there are other cards that use
>>> it as well. I think I have one bttv using it. Not sure.
>>>
>>> The routine on em28xx is a fork of the original one, that was changed
>>> to work with the devices there.
>> Indeed, it's a fork, 100% identical:
>>
>>
>> static int em28xx_get_key_terratec(struct IR_i2c *ir, u32 *ir_key, u32
>> *ir_raw)
>> {
>> unsigned char b;
>>
>> /* poll IR chip */
>> if (1 != i2c_master_recv(ir->c, &b, 1)) {
>> i2cdprintk("read error\n");
>> return -EIO;
>> }
>>
>> /* it seems that 0xFE indicates that a button is still hold
>> down, while 0xff indicates that no button is hold
>> down. 0xfe sequences are sometimes interrupted by 0xFF */
>>
>> i2cdprintk("key %02x\n", b);
>>
>> if (b == 0xff)
>> return 0;
>>
>> if (b == 0xfe)
>> /* keep old data */
>> return 1;
>>
>> *ir_key = b;
>> *ir_raw = b;
>> return 1;
>> }
>>
>>
>>
>>
>> static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
>> {
>> unsigned char b;
>>
>> /* poll IR chip */
>> if (1 != i2c_master_recv(ir->c, &b, 1)) {
>> dprintk(1,"read error\n");
>> return -EIO;
>> }
>>
>> /* it seems that 0xFE indicates that a button is still hold
>> down, while 0xff indicates that no button is hold
>> down. 0xfe sequences are sometimes interrupted by 0xFF */
>>
>> dprintk(2,"key %02x\n", b);
>>
>> if (b == 0xff)
>> return 0;
>>
>> if (b == 0xfe)
>> /* keep old data */
>> return 1;
>>
>> *ir_key = b;
>> *ir_raw = b;
>> return 1;
>> }
>>
>>
>>
>> Why should we keep two 100% identical functions ? See patch 4/6.
>> I'm 99% sure that both devices are absolutely identical.
> 99% sure is not enough. A simple firmware difference at the microprocessor
> is enough to make the devices different.
I agree, but that's irrelevant. What counts is that the _code_ ist 100%
identical.
> Also, this was widely discussed several years ago, when we decided to cleanup
> the I2C code. We then invested lot of efforts to move those get_keys away
> from ir-i2c-kbd. The only things left there are the ones we identified that
> were needed by auto-detection mode on old devices that we don't have.
>
> What was decided is to move everything that we know to the *-input driver,
> keeping there only the legacy stuff.
Uhm... ok.
My assumption was, that the goal is the opposite (move as much common
code as possible to i2c-ir-kbd).
I'm a bit puzzled about this decision...
Okay.... but then... why do we still use ir-kbd-i2c in em28xx ?
We can easily get rid of it. Everything we need is already on board.
I can send a patch if you want.
>
>> Concerning the fix I'm suggesting here:
>> First of all, I have to say that the Terratec RC works even without this
>> patch.
>> Nevertheless, I think the function should really return valid values for
>> ir_key and ir_raw when 0xfe=button hold is received. Especially because
>> the function succeeds.
>> This also allows us to make u32 ir_key, ir_raw in ir_key_poll() in
>> ir-kbd-i2c.c non-static.
>> While I agree that we should be careful, I can't see how this can cause
>> any trouble.
> Ok, the net effect is the same, except that the current way is faster, as it
> will skip some code that would simply put the value that it is already at
> ir_key/ir_raw again.
Faster... ok :) How much ? ;)
I would say its ugly coding. And a potential source of a regression.
> As this polling code is known to cause performance loss on some machines,
> the quickest, the best. Also, the better is to report long press events
> on a different way, as the input core can handle those on a different way
> (that's why there are there keyup/keydown kABI calls). A patch for better
> handle rc=1 return code at ir-i2c-kbd is needed.
Sounds good.
> In any case, I don't see any need for patches 4/6 or 6/6.
>
>> The second thing is the small fix for the key code debug output. Don't
>> you think it makes sense ?
> Now that we have "ir-keycode -t", all those key/scancode printk's inside
> the driver are pretty much useless, as both are reported as events.
>
> In the past, when most of the RC code were written, those prints were
Then we should remove them.
Regards,
Frank
> needed, as the scancode weren't reported to userspace.
>
> Regards,
> Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1()
2013-01-06 20:32 ` Frank Schäfer
@ 2013-01-07 16:40 ` Mauro Carvalho Chehab
2013-01-08 17:39 ` Frank Schäfer
0 siblings, 1 reply; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2013-01-07 16:40 UTC (permalink / raw)
To: Frank Schäfer; +Cc: Linux Media Mailing List
Em Sun, 06 Jan 2013 21:32:31 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> Am 05.01.2013 16:25, schrieb Mauro Carvalho Chehab:
> > Em Sat, 05 Jan 2013 14:32:30 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 05.01.2013 03:39, schrieb Mauro Carvalho Chehab:
> >>> Em Fri, 28 Dec 2012 00:02:48 +0100
> >>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>>
> >>>> - return valid key code when button is hold
> >>>> - debug: print key code only when a button is pressed
> >>>>
> >>>> Tested with device "Terratec Cinergy 200 USB" (em28xx).
> >>>>
> >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >>>> ---
> >>>> drivers/media/i2c/ir-kbd-i2c.c | 15 +++++----------
> >>>> 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)
> >>>>
> >>>> diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
> >>>> index 08ae067..2984b7d 100644
> >>>> --- a/drivers/media/i2c/ir-kbd-i2c.c
> >>>> +++ b/drivers/media/i2c/ir-kbd-i2c.c
> >>>> @@ -184,18 +184,13 @@ static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
> >>>> return -EIO;
> >>>> }
> >>>>
> >>>> - /* it seems that 0xFE indicates that a button is still hold
> >>>> - down, while 0xff indicates that no button is hold
> >>>> - down. 0xfe sequences are sometimes interrupted by 0xFF */
> >>>> -
> >>>> - dprintk(2,"key %02x\n", b);
> >>>> -
> >>>> - if (b == 0xff)
> >>>> + if (b == 0xff) /* no button */
> >>>> return 0;
> >>>>
> >>>> - if (b == 0xfe)
> >>>> - /* keep old data */
> >>>> - return 1;
> >>>> + if (b == 0xfe) /* button is still hold */
> >>>> + b = ir->rc->last_scancode; /* keep old data */
> >>>> +
> >>>> + dprintk(2,"key %02x\n", b);
> >>>>
> >>>> *ir_key = b;
> >>>> *ir_raw = b;
> >>> Don't do that. This piece of code is old, and it was added there
> >>> before the em28xx driver. Originally, the ir-i2c-kbd were used by
> >>> bttv and saa7134 drivers and the code there were auto-detecting the
> >>> I2C IR hardware decoding chips that used to be very common on media
> >>> devices. I'm almost sure that the original device that started using
> >>> this code is this model:
> >>>
> >>> drivers/media/pci/bt8xx/bttv-cards.c: .name = "Typhoon TView RDS + FM Stereo / KNC1 TV Station RDS",
> >>>
> >>> That's why it is called as KNC1, but there are other cards that use
> >>> it as well. I think I have one bttv using it. Not sure.
> >>>
> >>> The routine on em28xx is a fork of the original one, that was changed
> >>> to work with the devices there.
> >> Indeed, it's a fork, 100% identical:
> >>
> >>
> >> static int em28xx_get_key_terratec(struct IR_i2c *ir, u32 *ir_key, u32
> >> *ir_raw)
> >> {
> >> unsigned char b;
> >>
> >> /* poll IR chip */
> >> if (1 != i2c_master_recv(ir->c, &b, 1)) {
> >> i2cdprintk("read error\n");
> >> return -EIO;
> >> }
> >>
> >> /* it seems that 0xFE indicates that a button is still hold
> >> down, while 0xff indicates that no button is hold
> >> down. 0xfe sequences are sometimes interrupted by 0xFF */
> >>
> >> i2cdprintk("key %02x\n", b);
> >>
> >> if (b == 0xff)
> >> return 0;
> >>
> >> if (b == 0xfe)
> >> /* keep old data */
> >> return 1;
> >>
> >> *ir_key = b;
> >> *ir_raw = b;
> >> return 1;
> >> }
> >>
> >>
> >>
> >>
> >> static int get_key_knc1(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
> >> {
> >> unsigned char b;
> >>
> >> /* poll IR chip */
> >> if (1 != i2c_master_recv(ir->c, &b, 1)) {
> >> dprintk(1,"read error\n");
> >> return -EIO;
> >> }
> >>
> >> /* it seems that 0xFE indicates that a button is still hold
> >> down, while 0xff indicates that no button is hold
> >> down. 0xfe sequences are sometimes interrupted by 0xFF */
> >>
> >> dprintk(2,"key %02x\n", b);
> >>
> >> if (b == 0xff)
> >> return 0;
> >>
> >> if (b == 0xfe)
> >> /* keep old data */
> >> return 1;
> >>
> >> *ir_key = b;
> >> *ir_raw = b;
> >> return 1;
> >> }
> >>
> >>
> >>
> >> Why should we keep two 100% identical functions ? See patch 4/6.
> >> I'm 99% sure that both devices are absolutely identical.
> > 99% sure is not enough. A simple firmware difference at the microprocessor
> > is enough to make the devices different.
>
> I agree, but that's irrelevant. What counts is that the _code_ ist 100%
> identical.
>
> > Also, this was widely discussed several years ago, when we decided to cleanup
> > the I2C code. We then invested lot of efforts to move those get_keys away
> > from ir-i2c-kbd. The only things left there are the ones we identified that
> > were needed by auto-detection mode on old devices that we don't have.
> >
> > What was decided is to move everything that we know to the *-input driver,
> > keeping there only the legacy stuff.
>
> Uhm... ok.
> My assumption was, that the goal is the opposite (move as much common
> code as possible to i2c-ir-kbd).
> I'm a bit puzzled about this decision...
>
> Okay.... but then... why do we still use ir-kbd-i2c in em28xx ?
> We can easily get rid of it. Everything we need is already on board.
>
> I can send a patch if you want.
No. We don't want to mix I2C client code inside the I2C bus drivers.
If we had started to code it right now, we would likely have used a
different approach for those I2C IR's, but it is not valuable enough
to change it right now, as it works, and there's nothing new happening
here.
The RC trend is to not use hardware decoding anymore, as this doesn't
follow Microsoft's MCE architecture. All newer chipsets I'm aware of
just sends a sequence of pulse/space duration, and let the kernel to
decode. Empia seems to be late on following this market trend. Even so,
I don't see any new board design with an IR I2C hardware for years.
So, the better is to just not re-engineer the things that are working,
in order to avoid breaking them.
> > In any case, I don't see any need for patches 4/6 or 6/6.
> >
> >> The second thing is the small fix for the key code debug output. Don't
> >> you think it makes sense ?
> > Now that we have "ir-keycode -t", all those key/scancode printk's inside
> > the driver are pretty much useless, as both are reported as events.
> >
> > In the past, when most of the RC code were written, those prints were
>
> Then we should remove them.
That makes sense on my eyes.
Yet, writing/reviewing such cleanup patches would have a very low priority.
Btw, all drivers have a lot of printk's message from the time they were
written that can be cleaned up, especially nowadays, as several of those
printk's can be replaced by Kernel tracing facilities.
If one is willing to do it, I expect that it would be reviewing all
of them and not just those ones.
--
Cheers,
Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 6/6] ir-kbd-i2c: fix get_key_knc1()
2013-01-07 16:40 ` Mauro Carvalho Chehab
@ 2013-01-08 17:39 ` Frank Schäfer
0 siblings, 0 replies; 20+ messages in thread
From: Frank Schäfer @ 2013-01-08 17:39 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List
Am 07.01.2013 17:40, schrieb Mauro Carvalho Chehab:
>>> Also, this was widely discussed several years ago, when we decided to cleanup
>>> the I2C code. We then invested lot of efforts to move those get_keys away
>>> from ir-i2c-kbd. The only things left there are the ones we identified that
>>> were needed by auto-detection mode on old devices that we don't have.
>>>
>>> What was decided is to move everything that we know to the *-input driver,
>>> keeping there only the legacy stuff.
>> Uhm... ok.
>> My assumption was, that the goal is the opposite (move as much common
>> code as possible to i2c-ir-kbd).
>> I'm a bit puzzled about this decision...
>>
>> Okay.... but then... why do we still use ir-kbd-i2c in em28xx ?
>> We can easily get rid of it. Everything we need is already on board.
>>
>> I can send a patch if you want.
> No. We don't want to mix I2C client code inside the I2C bus drivers.
Well, you can't have both at the same time. :D
Either do it in a separate module (ir-kbd-i2c) or in the em28xx driver !? ;)
> If we had started to code it right now, we would likely have used a
> different approach for those I2C IR's, but it is not valuable enough
> to change it right now, as it works, and there's nothing new happening
> here.
>
> The RC trend is to not use hardware decoding anymore, as this doesn't
> follow Microsoft's MCE architecture. All newer chipsets I'm aware of
> just sends a sequence of pulse/space duration, and let the kernel to
> decode. Empia seems to be late on following this market trend. Even so,
> I don't see any new board design with an IR I2C hardware for years.
True, but doesn't change the fact that the current code can be improved.
I think it's valuable enough, as we can get rid of a module dependency here.
> So, the better is to just not re-engineer the things that are working,
> in order to avoid breaking them.
Yeah, we should _always_ be careful.
But it's definitely not re-engineering. The key polling and decoding
functions are already there.
The main change is, that em28xx-input would call them itself instead of
ir-kbd-i2c, which is what it already does with the built-in decoders.
For maximum safety, we can use the same key handling function as ir-kbd-i2c.
If we don't do it now (yet we have devices for testing), it will likely
never happen.
>
>>> In any case, I don't see any need for patches 4/6 or 6/6.
>>>
>>>> The second thing is the small fix for the key code debug output. Don't
>>>> you think it makes sense ?
>>> Now that we have "ir-keycode -t", all those key/scancode printk's inside
>>> the driver are pretty much useless, as both are reported as events.
>>>
>>> In the past, when most of the RC code were written, those prints were
>> Then we should remove them.
> That makes sense on my eyes.
>
> Yet, writing/reviewing such cleanup patches would have a very low priority.
> Btw, all drivers have a lot of printk's message from the time they were
> written that can be cleaned up, especially nowadays, as several of those
> printk's can be replaced by Kernel tracing facilities.
>
> If one is willing to do it, I expect that it would be reviewing all
> of them and not just those ones.
Sure.
Regards,
Frank
^ permalink raw reply [flat|nested] 20+ messages in thread