pvrusb2: Provide more information about IR units to lirc_zilog and ir-kbd-i2c

RFC PATCH In review Andy Walls 2 participants 4 messages Jan 17, 2011 Media (V4L/DVB)

Share & tools

Andy Walls

author

2011-01-17 00:45 UTC #0 lore raw

RFC PATCH 1/1 pvrusb2: Provide more information about IR units to lirc_zilog and ir-kbd-i2c

When registering an IR Rx device with the I2C subsystem, provide more detailed information about the IR device and default remote configuration for the IR driver modules.

Also explicitly register any IR Tx device with the I2C subsystem.

Signed-off-by Andy Walls <[email protected]>

Cc Mike Isely <[email protected]>

--
Mike,

As discussed on IRC, this patch will enable lirc_zilog to bind to Zilog Z8 IR units on devices supported by pvrusb2.

Please review and comment. This patch could have been written a number of ways. The way I chose was very direct: hard-coding information in a single function.

A git branch with this change, and the updated lirc_zilog, is here:

git://linuxtv.org/awalls/media_tree.git z8-pvrusb2

http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/z8-pvrusb2

Regards,
Andy

drivers/media/video/pvrusb2/pvrusb2-hdw-internal.h +2 -0

index ac94a8b..305e6aa 100644
@@ -40,6 +40,7 @@
 #include "pvrusb2-io.h"
 #include <media/v4l2-device.h>
 #include <media/cx2341x.h>
+#include <media/ir-kbd-i2c.h>
 #include "pvrusb2-devattr.h"
 
 /* Legal values for PVR2_CID_HSM */

@@ -202,6 +203,7 @@ struct pvr2_hdw {
 
 	/* IR related */
 	unsigned int ir_scheme_active; /* IR scheme as seen from the outside */
+	struct IR_i2c_init_data ir_init_data; /* params passed to IR modules */
 
 	/* Frequency table */
 	unsigned int freqTable[FREQTABLE_SIZE];

drivers/media/video/pvrusb2/pvrusb2-i2c-core.c +43 ~~-19~~

88 lines not on this page · raw message

Mike Isely

Acked-by on v1 1/1

2011-01-17 02:27 UTC #1 lore raw

Andy:

Is the IR_i2c_init_data struct instance required to remain around for the life of the driver's registration and is that why you stuffed it into the pvr2_hdw struct? Second: If the first question is yes, then is that struct considered to be read-only once it is set up and passed through to the i2c device registration function? In other words, could that structure be a const static initialized at compile time, perhaps as part of a table definition?

I believe I follow this and it looks good. The concept looks very simple and it's nice that the changes are really only in a single spot. Just thinking ahead about making the setup table-driven and not requiring data segment storage.

-Mike

Acked-by Mike Isely <[email protected]>

Quoted 151 lines · Andy Walls

When registering an IR Rx device with the I2C subsystem, provide more detailed information about the IR device and default remote configuration for the IR driver modules.

Also explicitly register any IR Tx device with the I2C subsystem.

Signed-off-by Andy Walls <[email protected]>

Cc Mike Isely <[email protected]>

--
Mike,

As discussed on IRC, this patch will enable lirc_zilog to bind to Zilog Z8 IR units on devices supported by pvrusb2.

Please review and comment. This patch could have been written a number of ways. The way I chose was very direct: hard-coding information in a single function.

A git branch with this change, and the updated lirc_zilog, is here:

git://linuxtv.org/awalls/media_tree.git z8-pvrusb2

http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/z8-pvrusb2

Regards,
Andy

drivers/media/video/pvrusb2/pvrusb2-hdw-internal.h +2 -0

index ac94a8b..305e6aa 100644
@@ -40,6 +40,7 @@
 #include "pvrusb2-io.h"
 #include <media/v4l2-device.h>
 #include <media/cx2341x.h>
+#include <media/ir-kbd-i2c.h>
 #include "pvrusb2-devattr.h"
 
 /* Legal values for PVR2_CID_HSM */

@@ -202,6 +203,7 @@ struct pvr2_hdw {
 
 	/* IR related */
 	unsigned int ir_scheme_active; /* IR scheme as seen from the outside */
+	struct IR_i2c_init_data ir_init_data; /* params passed to IR modules */
 
 	/* Frequency table */
 	unsigned int freqTable[FREQTABLE_SIZE];

drivers/media/video/pvrusb2/pvrusb2-i2c-core.c +43 ~~-20~~

index 7cbe18c..ccc8849 100644
@@ -19,6 +19,7 @@
  */
 
 #include <linux/i2c.h>
+#include <media/ir-kbd-i2c.h>
 #include "pvrusb2-i2c-core.h"
 #include "pvrusb2-hdw-internal.h"
 #include "pvrusb2-debug.h"

@@ -48,13 +49,6 @@ module_param_named(disable_autoload_ir_video, pvr2_disable_ir_video,
 MODULE_PARM_DESC(disable_autoload_ir_video,
 		 "1=do not try to autoload ir_video IR receiver");
 
-/* Mapping of IR schemes to known I2C addresses - if any */
-static const unsigned char ir_video_addresses[] = {
-	[PVR2_IR_SCHEME_ZILOG] = 0x71,
-	[PVR2_IR_SCHEME_29XXX] = 0x18,
-	[PVR2_IR_SCHEME_24XXX] = 0x18,
-};
-
 static int pvr2_i2c_write(struct pvr2_hdw *hdw, /* Context */
 			  u8 i2c_addr,      /* I2C address we're talking to */
 			  u8 *data,         /* Data to write */

@@ -574,26 +568,56 @@ static void do_i2c_scan(struct pvr2_hdw *hdw)
 static void pvr2_i2c_register_ir(struct pvr2_hdw *hdw)
 {
 	struct i2c_board_info info;
-	unsigned char addr = 0;
+	struct IR_i2c_init_data *init_data = &hdw->ir_init_data;
 	if (pvr2_disable_ir_video) {
 		pvr2_trace(PVR2_TRACE_INFO,
 			   "Automatic binding of ir_video has been disabled.");
 		return;
 	}
-	if (hdw->ir_scheme_active < ARRAY_SIZE(ir_video_addresses)) {
-		addr = ir_video_addresses[hdw->ir_scheme_active];
-	}
-	if (!addr) {
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	switch (hdw->ir_scheme_active) {
+	case PVR2_IR_SCHEME_24XXX: /* FX2-controlled IR */
+	case PVR2_IR_SCHEME_29XXX: /* Original 29xxx device */
+		init_data->ir_codes              = RC_MAP_HAUPPAUGE_NEW;
+		init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP;
+		init_data->type                  = RC_TYPE_RC5;
+		init_data->name                  = hdw->hdw_desc->description;
+		init_data->polling_interval      = 100; /* ms From ir-kbd-i2c */
+		/* IR Receiver */
+		info.addr          = 0x18;
+		info.platform_data = init_data;
+		strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
+		pvr2_trace(PVR2_TRACE_INFO, "Binding %s to i2c address 0x%02x.",
+			   info.type, info.addr);
+		i2c_new_device(&hdw->i2c_adap, &info);
+		break;
+	case PVR2_IR_SCHEME_ZILOG:     /* HVR-1950 style */
+	case PVR2_IR_SCHEME_24XXX_MCE: /* 24xxx MCE device */
+		init_data->ir_codes              = RC_MAP_HAUPPAUGE_NEW;
+		init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
+		init_data->type                  = RC_TYPE_RC5;
+		init_data->name                  = hdw->hdw_desc->description;
+		init_data->polling_interval      = 260; /* ms From lirc_zilog */
+		/* IR Receiver */
+		info.addr          = 0x71;
+		info.platform_data = init_data;
+		strlcpy(info.type, "ir_rx_z8f0811_haup", I2C_NAME_SIZE);
+		pvr2_trace(PVR2_TRACE_INFO, "Binding %s to i2c address 0x%02x.",
+			   info.type, info.addr);
+		i2c_new_device(&hdw->i2c_adap, &info);
+		/* IR Trasmitter */
+		info.addr          = 0x70;
+		info.platform_data = init_data;
+		strlcpy(info.type, "ir_tx_z8f0811_haup", I2C_NAME_SIZE);
+		pvr2_trace(PVR2_TRACE_INFO, "Binding %s to i2c address 0x%02x.",
+			   info.type, info.addr);
+		i2c_new_device(&hdw->i2c_adap, &info);
+		break;
+	default:
 		/* The device either doesn't support I2C-based IR or we
 		   don't know (yet) how to operate IR on the device. */
-		return;
+		break;
 	}
-	pvr2_trace(PVR2_TRACE_INFO,
-		   "Binding ir_video to i2c address 0x%02x.", addr);
-	memset(&info, 0, sizeof(struct i2c_board_info));
-	strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
-	info.addr = addr;
-	i2c_new_device(&hdw->i2c_adap, &info);
 }
 
 void pvr2_i2c_core_init(struct pvr2_hdw *hdw)
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Andy Walls

author

2011-01-17 03:08 UTC #2 lore raw

Quoted 5 lines · Mike Isely

Andy:
Is the IR_i2c_init_data struct instance required to remain around for the life of the driver's registration and is that why you stuffed it into the pvr2_hdw struct?

Yes.

The loading of ir-kbd-i2c or lirc_zilog can happen some undetermined amount of time after or before the pvrusb2 driver registers the I2C devices. The data needs to persist on a per device basis; it cannot come from the stack.

Quoted 3 lines

Second: If the first question is yes, then is
that struct considered to be read-only once it is set up and passed through to the i2c device registration function?

Yes. It will only be read once by lirc_zilog or ir-kbd-i2c in their probe methods. Although both of those modules can be unloaded and reloaded, so there is the possibility of being read again any number of times.

Quoted 3 lines

In other words, could
that structure be a const static initialized at compile time, perhaps as part of a table definition?

Right now, yes. In the near future, I need to use to to pass 3 non-const items though:

1. A "struct mutex *transceiver_lock" so that the bridge driver can pass a mutex to multiple modules accessing the Z8. That would be a per device instance mutex, instantiated and initialized by the bridge driver. The use case where this would be needed is a setup where ir-kbd-i2c handles Z8 IR Rx and lirc_zilog handles only Z8 IR Tx of the same chip.

2. A bridge driver provided "void (*reset_ir_chip)(struct i2c_adapter *adap)", or maybe "void (*reset_ir_chip)(void *priv)", callback to reset the transceiver chip when it gets hung. The original lirc_pvr150 module had some hard coded reset function names and calls in it, but they were removed with the rename to lirc_zilog and move into the kernel. I'd like to get that ability back.

3. A bridge driver provided private data structure for the "void *priv" argument of the aforementioned reset callback. This would also be a per device object instantiated and initialized by the bridge driver.

Quoted 4 lines

I believe I follow this and it looks good. The concept looks very simple and it's nice that the changes are really only in a single spot. Just thinking ahead about making the setup table-driven and not requiring data segment storage.

With the patch right now it could be constant, I think. You would have to use some generic name, like "pvrusb2 IR", instead of hdw->hdw_desc->description though.

For my future plans, if you don't provide a reset callback and don't wish to provide a mutex, then yes you can keep it constant.

I suspect not providing a reset callback may be OK.

Not providing a mutex is also OK but it imposes a limitation: only one IR module should be allowed to use the Z8 chip. That means only lirc_zilog for IR Tx/Rx with Rx through LIRC, or
only ir-kbd-i2c for IR Rx through the the Linux input subsystem.

Quoted 4 lines

-Mike
Acked-by Mike Isely <[email protected]>

Thanks. I'll pull this into my Z8 branch then.

Regards,
Andy

Quoted 154 lines · Andy Walls

Quoted 152 lines

When registering an IR Rx device with the I2C subsystem, provide more detailed information about the IR device and default remote configuration for the IR driver modules.

Also explicitly register any IR Tx device with the I2C subsystem.

Signed-off-by Andy Walls <[email protected]>

Cc Mike Isely <[email protected]>

--
Mike,

As discussed on IRC, this patch will enable lirc_zilog to bind to Zilog Z8 IR units on devices supported by pvrusb2.

Please review and comment. This patch could have been written a number of ways. The way I chose was very direct: hard-coding information in a single function.

A git branch with this change, and the updated lirc_zilog, is here:

git://linuxtv.org/awalls/media_tree.git z8-pvrusb2

http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/z8-pvrusb2

Regards,
Andy

drivers/media/video/pvrusb2/pvrusb2-hdw-internal.h +2 -0

index ac94a8b..305e6aa 100644
@@ -40,6 +40,7 @@
 #include "pvrusb2-io.h"
 #include <media/v4l2-device.h>
 #include <media/cx2341x.h>
+#include <media/ir-kbd-i2c.h>
 #include "pvrusb2-devattr.h"
 
 /* Legal values for PVR2_CID_HSM */

@@ -202,6 +203,7 @@ struct pvr2_hdw {
 
 	/* IR related */
 	unsigned int ir_scheme_active; /* IR scheme as seen from the outside */
+	struct IR_i2c_init_data ir_init_data; /* params passed to IR modules */
 
 	/* Frequency table */
 	unsigned int freqTable[FREQTABLE_SIZE];

drivers/media/video/pvrusb2/pvrusb2-i2c-core.c +43 ~~-20~~

index 7cbe18c..ccc8849 100644
@@ -19,6 +19,7 @@
  */
 
 #include <linux/i2c.h>
+#include <media/ir-kbd-i2c.h>
 #include "pvrusb2-i2c-core.h"
 #include "pvrusb2-hdw-internal.h"
 #include "pvrusb2-debug.h"

@@ -48,13 +49,6 @@ module_param_named(disable_autoload_ir_video, pvr2_disable_ir_video,
 MODULE_PARM_DESC(disable_autoload_ir_video,
 		 "1=do not try to autoload ir_video IR receiver");
 
-/* Mapping of IR schemes to known I2C addresses - if any */
-static const unsigned char ir_video_addresses[] = {
-	[PVR2_IR_SCHEME_ZILOG] = 0x71,
-	[PVR2_IR_SCHEME_29XXX] = 0x18,
-	[PVR2_IR_SCHEME_24XXX] = 0x18,
-};
-
 static int pvr2_i2c_write(struct pvr2_hdw *hdw, /* Context */
 			  u8 i2c_addr,      /* I2C address we're talking to */
 			  u8 *data,         /* Data to write */

@@ -574,26 +568,56 @@ static void do_i2c_scan(struct pvr2_hdw *hdw)
 static void pvr2_i2c_register_ir(struct pvr2_hdw *hdw)
 {
 	struct i2c_board_info info;
-	unsigned char addr = 0;
+	struct IR_i2c_init_data *init_data = &hdw->ir_init_data;
 	if (pvr2_disable_ir_video) {
 		pvr2_trace(PVR2_TRACE_INFO,
 			   "Automatic binding of ir_video has been disabled.");
 		return;
 	}
-	if (hdw->ir_scheme_active < ARRAY_SIZE(ir_video_addresses)) {
-		addr = ir_video_addresses[hdw->ir_scheme_active];
-	}
-	if (!addr) {
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	switch (hdw->ir_scheme_active) {
+	case PVR2_IR_SCHEME_24XXX: /* FX2-controlled IR */
+	case PVR2_IR_SCHEME_29XXX: /* Original 29xxx device */
+		init_data->ir_codes              = RC_MAP_HAUPPAUGE_NEW;
+		init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP;
+		init_data->type                  = RC_TYPE_RC5;
+		init_data->name                  = hdw->hdw_desc->description;
+		init_data->polling_interval      = 100; /* ms From ir-kbd-i2c */
+		/* IR Receiver */
+		info.addr          = 0x18;
+		info.platform_data = init_data;
+		strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
+		pvr2_trace(PVR2_TRACE_INFO, "Binding %s to i2c address 0x%02x.",
+			   info.type, info.addr);
+		i2c_new_device(&hdw->i2c_adap, &info);
+		break;
+	case PVR2_IR_SCHEME_ZILOG:     /* HVR-1950 style */
+	case PVR2_IR_SCHEME_24XXX_MCE: /* 24xxx MCE device */
+		init_data->ir_codes              = RC_MAP_HAUPPAUGE_NEW;
+		init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
+		init_data->type                  = RC_TYPE_RC5;
+		init_data->name                  = hdw->hdw_desc->description;
+		init_data->polling_interval      = 260; /* ms From lirc_zilog */
+		/* IR Receiver */
+		info.addr          = 0x71;
+		info.platform_data = init_data;
+		strlcpy(info.type, "ir_rx_z8f0811_haup", I2C_NAME_SIZE);
+		pvr2_trace(PVR2_TRACE_INFO, "Binding %s to i2c address 0x%02x.",
+			   info.type, info.addr);
+		i2c_new_device(&hdw->i2c_adap, &info);
+		/* IR Trasmitter */
+		info.addr          = 0x70;
+		info.platform_data = init_data;
+		strlcpy(info.type, "ir_tx_z8f0811_haup", I2C_NAME_SIZE);
+		pvr2_trace(PVR2_TRACE_INFO, "Binding %s to i2c address 0x%02x.",
+			   info.type, info.addr);
+		i2c_new_device(&hdw->i2c_adap, &info);
+		break;
+	default:
 		/* The device either doesn't support I2C-based IR or we
 		   don't know (yet) how to operate IR on the device. */
-		return;
+		break;
 	}
-	pvr2_trace(PVR2_TRACE_INFO,
-		   "Binding ir_video to i2c address 0x%02x.", addr);
-	memset(&info, 0, sizeof(struct i2c_board_info));
-	strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
-	info.addr = addr;
-	i2c_new_device(&hdw->i2c_adap, &info);
 }
 
 void pvr2_i2c_core_init(struct pvr2_hdw *hdw)
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mike Isely

2011-01-17 03:17 UTC #3 lore raw

Quoted 1 line · Andy Walls

On Sun, 2011-01-16 at 20:27 -0600, Mike Isely wrote:

[,,,]

Quoted 18 lines

Right now, yes. In the near future, I need to use to to pass 3 non-const items though:
1. A "struct mutex *transceiver_lock" so that the bridge driver can pass a mutex to multiple modules accessing the Z8. That would be a per device instance mutex, instantiated and initialized by the bridge driver. The use case where this would be needed is a setup where ir-kbd-i2c handles Z8 IR Rx and lirc_zilog handles only Z8 IR Tx of the same chip.
2. A bridge driver provided "void (*reset_ir_chip)(struct i2c_adapter *adap)", or maybe "void (*reset_ir_chip)(void *priv)", callback to reset the transceiver chip when it gets hung. The original lirc_pvr150 module had some hard coded reset function names and calls in it, but they were removed with the rename to lirc_zilog and move into the kernel. I'd like to get that ability back.

Quoted 4 lines

3. A bridge driver provided private data structure for the "void *priv" argument of the aforementioned reset callback. This would also be a per device object instantiated and initialized by the bridge driver.

I follow. Makes sense.

Something to consider, perhaps for the future: Seems like what you have here amounts to some configuration data which will always be read-only, and other data which maps to the "context" in which the driver is being used (e.g. mutex instance, callback private context pointer, etc). That configuration data, if packed up into its own struct, could then be squirreled away at compile-time by the bridge driver and provided as part of a single table lookup. This only makes sense if there are a lot of configuration bits - but here I count 6 different items.

Quoted 13 lines

Quoted 5 lines
I believe I follow this and it looks good. The concept looks very simple and it's nice that the changes are really only in a single spot. Just thinking ahead about making the setup table-driven and not requiring data segment storage.
With the patch right now it could be constant, I think. You would have to use some generic name, like "pvrusb2 IR", instead of hdw->hdw_desc->description though.
For my future plans, if you don't provide a reset callback and don't wish to provide a mutex, then yes you can keep it constant.

Quoted 6 lines

I suspect not providing a reset callback may be OK.
Not providing a mutex is also OK but it imposes a limitation: only one IR module should be allowed to use the Z8 chip. That means only lirc_zilog for IR Tx/Rx with Rx through LIRC, or
only ir-kbd-i2c for IR Rx through the the Linux input subsystem.

For the future, I have no problem providing a reset callback. And given what you've said, I see no reason to do anything here which would constrain what you're trying to accomplish. But if down the road you do set up a separate configuration struct which this context struct might point to, then I'd like to update the pvrusb2 driver to take advantage of it. But this is no big deal for now.

Quoted 7 lines

Quoted 5 lines
-Mike
Acked-by Mike Isely <[email protected]>
Thanks. I'll pull this into my Z8 branch then.

You're welcome.

-Mike

Signature · 3 lines

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8