V4L/DVB: Use custom I2C probing function mechanism

Now that i2c-core offers the possibility to provide custom probing function for I2C devices, let's make use of it.

Signed-off-by Jean Delvare <[email protected]>

---
I wasn't too sure where to put the custom probe function: in each driver, in the ir-common module or in the v4l2-common module. I went for the second option as a middle ground, but am ready to discuss it if anyone objects.

 drivers/media/IR/ir-functions.c           |   12 ++++++++++++
 drivers/media/video/cx23885/cx23885-i2c.c |   14 +++-----------
 drivers/media/video/cx88/cx88-i2c.c       |   18 +++---------------
 include/media/ir-common.h                 |    5 +++++
 4 files changed, 23 insertions(+), 26 deletions(-)

drivers/media/video/cx23885/cx23885-i2c.c +3 -11

@@ -28,6 +28,7 @@
 #include "cx23885.h"
 
 #include <media/v4l2-common.h>
+#include <media/ir-common.h>
 
 static unsigned int i2c_debug;
 module_param(i2c_debug, int, 0644);

@@ -365,17 +366,8 @@ int cx23885_i2c_register(struct cx23885_
 
 		memset(&info, 0, sizeof(struct i2c_board_info));
 		strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
-		/*
-		 * We can't call i2c_new_probed_device() because it uses
-		 * quick writes for probing and the IR receiver device only
-		 * replies to reads.
-		 */
-		if (i2c_smbus_xfer(&bus->i2c_adap, addr_list[0], 0,
-				   I2C_SMBUS_READ, 0, I2C_SMBUS_QUICK,
-				   NULL) >= 0) {
-			info.addr = addr_list[0];
-			i2c_new_device(&bus->i2c_adap, &info);
-		}
+		i2c_new_probed_device(&bus->i2c_adap, &info, addr_list,
+				      ir_i2c_probe);
 	}
 
 	return bus->i2c_rc;

drivers/media/video/cx88/cx88-i2c.c +3 -15

@@ -34,6 +34,7 @@
 
 #include "cx88.h"
 #include <media/v4l2-common.h>
+#include <media/ir-common.h>
 
 static unsigned int i2c_debug;
 module_param(i2c_debug, int, 0644);

@@ -188,24 +189,11 @@ int cx88_i2c_init(struct cx88_core *core
 			0x18, 0x6b, 0x71,
 			I2C_CLIENT_END
 		};
-		const unsigned short *addrp;
 
 		memset(&info, 0, sizeof(struct i2c_board_info));
 		strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
-		/*
-		 * We can't call i2c_new_probed_device() because it uses
-		 * quick writes for probing and at least some R receiver
-		 * devices only reply to reads.
-		 */
-		for (addrp = addr_list; *addrp != I2C_CLIENT_END; addrp++) {
-			if (i2c_smbus_xfer(&core->i2c_adap, *addrp, 0,
-					   I2C_SMBUS_READ, 0,
-					   I2C_SMBUS_QUICK, NULL) >= 0) {
-				info.addr = *addrp;
-				i2c_new_device(&core->i2c_adap, &info);
-				break;
-			}
-		}
+		i2c_new_probed_device(&core->i2c_adap, &info, addr_list,
+				      ir_i2c_probe);
 	}
 	return core->i2c_rc;
 }

drivers/media/IR/ir-functions.c +12 -0

@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/string.h>
 #include <linux/jiffies.h>
+#include <linux/i2c.h>
 #include <media/ir-common.h>
 
 /* -------------------------------------------------------------------------- */

@@ -353,3 +354,14 @@ void ir_rc5_timer_keyup(unsigned long da
 	ir_input_nokey(ir->dev, &ir->ir);
 }
 EXPORT_SYMBOL_GPL(ir_rc5_timer_keyup);
+
+/* Some functions only needed for I2C devices */
+#if defined CONFIG_I2C || defined CONFIG_I2C_MODULE
+/* use quick read command for probe, some IR chips don't support writes */
+int ir_i2c_probe(struct i2c_adapter *i2c, unsigned short addr)
+{
+	return i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0,
+			      I2C_SMBUS_QUICK, NULL) >= 0;
+}
+EXPORT_SYMBOL_GPL(ir_i2c_probe);
+#endif

include/media/ir-common.h +5 -0

@@ -97,6 +97,11 @@ u32  ir_rc5_decode(unsigned int code);
 void ir_rc5_timer_end(unsigned long data);
 void ir_rc5_timer_keyup(unsigned long data);
 
+#if defined CONFIG_I2C || defined CONFIG_I2C_MODULE
+struct i2c_adapter;
+int ir_i2c_probe(struct i2c_adapter *i2c, unsigned short addr);
+#endif
+
 /* scancode->keycode map tables from ir-keymaps.c */
 
 extern struct ir_scancode_table ir_codes_empty_table;

Quoted 9 lines · Jean Delvare

Now that i2c-core offers the possibility to provide custom probing function for I2C devices, let's make use of it.

Signed-off-by Jean Delvare <[email protected]>

---
I wasn't too sure where to put the custom probe function: in each driver, in the ir-common module or in the v4l2-common module. I went for the second option as a middle ground, but am ready to discuss it if anyone objects.

With respect to cx23885, could you comment on the interaction of this patch with some patches of yours that are not merged yet:

http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/b39f8849a35b
http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/3cf1ac545ca5
http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/ef5d2c08106f

Are they related to the IR microcontroller not being probed properly?

(I tried to get these patches merged, but didn't due to problems with other patches in my PULL request, and then a severe shortage of time.)

Now that i2c-core offers the possibility to provide custom probing function for I2C devices, let's make use of it.

Signed-off-by Jean Delvare <[email protected]>

---
I wasn't too sure where to put the custom probe function: in each driver, in the ir-common module or in the v4l2-common module. I went for the second option as a middle ground, but am ready to discuss it if anyone objects.

 drivers/media/IR/ir-functions.c           |   12 ++++++++++++
 drivers/media/video/cx23885/cx23885-i2c.c |   14 +++-----------
 drivers/media/video/cx88/cx88-i2c.c       |   18 +++---------------
 include/media/ir-common.h                 |    5 +++++
 4 files changed, 23 insertions(+), 26 deletions(-)

drivers/media/video/cx23885/cx23885-i2c.c +3 -11

@@ -28,6 +28,7 @@
 #include "cx23885.h"
 
 #include <media/v4l2-common.h>
+#include <media/ir-common.h>
 
 static unsigned int i2c_debug;
 module_param(i2c_debug, int, 0644);

@@ -365,17 +366,8 @@ int cx23885_i2c_register(struct cx23885_
 
 		memset(&info, 0, sizeof(struct i2c_board_info));
 		strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
-		/*
-		 * We can't call i2c_new_probed_device() because it uses
-		 * quick writes for probing and the IR receiver device only
-		 * replies to reads.
-		 */
-		if (i2c_smbus_xfer(&bus->i2c_adap, addr_list[0], 0,
-				   I2C_SMBUS_READ, 0, I2C_SMBUS_QUICK,
-				   NULL) >= 0) {
-			info.addr = addr_list[0];
-			i2c_new_device(&bus->i2c_adap, &info);
-		}
+		i2c_new_probed_device(&bus->i2c_adap, &info, addr_list,
+				      ir_i2c_probe);
 	}
 
 	return bus->i2c_rc;

drivers/media/video/cx88/cx88-i2c.c +3 -15

@@ -34,6 +34,7 @@
 
 #include "cx88.h"
 #include <media/v4l2-common.h>
+#include <media/ir-common.h>
 
 static unsigned int i2c_debug;
 module_param(i2c_debug, int, 0644);

@@ -188,24 +189,11 @@ int cx88_i2c_init(struct cx88_core *core
 			0x18, 0x6b, 0x71,
 			I2C_CLIENT_END
 		};
-		const unsigned short *addrp;
 
 		memset(&info, 0, sizeof(struct i2c_board_info));
 		strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
-		/*
-		 * We can't call i2c_new_probed_device() because it uses
-		 * quick writes for probing and at least some R receiver
-		 * devices only reply to reads.
-		 */
-		for (addrp = addr_list; *addrp != I2C_CLIENT_END; addrp++) {
-			if (i2c_smbus_xfer(&core->i2c_adap, *addrp, 0,
-					   I2C_SMBUS_READ, 0,
-					   I2C_SMBUS_QUICK, NULL) >= 0) {
-				info.addr = *addrp;
-				i2c_new_device(&core->i2c_adap, &info);
-				break;
-			}
-		}
+		i2c_new_probed_device(&core->i2c_adap, &info, addr_list,
+				      ir_i2c_probe);
 	}
 	return core->i2c_rc;
 }

drivers/media/IR/ir-functions.c +12 -0

@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/string.h>
 #include <linux/jiffies.h>
+#include <linux/i2c.h>
 #include <media/ir-common.h>
 
 /* -------------------------------------------------------------------------- */

@@ -353,3 +354,14 @@ void ir_rc5_timer_keyup(unsigned long da
 	ir_input_nokey(ir->dev, &ir->ir);
 }
 EXPORT_SYMBOL_GPL(ir_rc5_timer_keyup);
+
+/* Some functions only needed for I2C devices */
+#if defined CONFIG_I2C || defined CONFIG_I2C_MODULE
+/* use quick read command for probe, some IR chips don't support writes */
+int ir_i2c_probe(struct i2c_adapter *i2c, unsigned short addr)
+{
+	return i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0,
+			      I2C_SMBUS_QUICK, NULL) >= 0;
+}
+EXPORT_SYMBOL_GPL(ir_i2c_probe);
+#endif

include/media/ir-common.h +5 -0

@@ -97,6 +97,11 @@ u32  ir_rc5_decode(unsigned int code);
 void ir_rc5_timer_end(unsigned long data);
 void ir_rc5_timer_keyup(unsigned long data);
 
+#if defined CONFIG_I2C || defined CONFIG_I2C_MODULE
+struct i2c_adapter;
+int ir_i2c_probe(struct i2c_adapter *i2c, unsigned short addr);
+#endif
+
 /* scancode->keycode map tables from ir-keymaps.c */
 
 extern struct ir_scancode_table ir_codes_empty_table;

Quoted 9 lines · Jean Delvare

Now that i2c-core offers the possibility to provide custom probing function for I2C devices, let's make use of it.

Signed-off-by Jean Delvare <[email protected]>

---
I wasn't too sure where to put the custom probe function: in each driver, in the ir-common module or in the v4l2-common module. I went for the second option as a middle ground, but am ready to discuss it if anyone objects.

Please, don't add new things at ir-common module. It basically contains the decoding functions for RC5 and pulse/distance, plus several IR keymaps. With the IR rework I'm doing, this module will go away, after having all the current IR decoders implemented via ir-raw-input binding.

The keymaps were already removed from it, on my experimental tree
(http://git.linuxtv.org/mchehab/ir.git), and rc5 decoder is already written (but still needs a few fixes).

The new ir-core is creating an abstract way to deal with Remote Controllers, meant to be used not only by IR's, but also for other types of RC, like, bluetooth and USB HID. It will also export a raw event interface, for use with lirc. As this is the core of the RC subsystem, a i2c-specific binding method also doesn't seem to belong there. SO, IMO, the better place is to add it as a static inline function at ir-kbd-i2c.h.

Hi Mauro,

Quoted 19 lines · Mauro Carvalho Chehab

Quoted 9 lines · Jean Delvare

Now that i2c-core offers the possibility to provide custom probing function for I2C devices, let's make use of it.

Signed-off-by Jean Delvare <[email protected]>

---
I wasn't too sure where to put the custom probe function: in each driver, in the ir-common module or in the v4l2-common module. I went for the second option as a middle ground, but am ready to discuss it if anyone objects.

Please, don't add new things at ir-common module. It basically contains the decoding functions for RC5 and pulse/distance, plus several IR keymaps. With the IR rework I'm doing, this module will go away, after having all the current IR decoders implemented via ir-raw-input binding.

The keymaps were already removed from it, on my experimental tree
(http://git.linuxtv.org/mchehab/ir.git), and rc5 decoder is already written (but still needs a few fixes).

Quoted 6 lines

The new ir-core is creating an abstract way to deal with Remote Controllers, meant to be used not only by IR's, but also for other types of RC, like, bluetooth and USB HID. It will also export a raw event interface, for use with lirc. As this is the core of the RC subsystem, a i2c-specific binding method also doesn't seem to belong there. SO, IMO, the better place is to add it as a static inline function at ir-kbd-i2c.h.

Ever tried to pass the address of an inline function as another function's parameter? :)

Quoted 17 lines · Jean Delvare

Quoted 15 lines · Mauro Carvalho Chehab

Please, don't add new things at ir-common module. It basically contains the decoding functions for RC5 and pulse/distance, plus several IR keymaps. With the IR rework I'm doing, this module will go away, after having all the current IR decoders implemented via ir-raw-input binding.

The keymaps were already removed from it, on my experimental tree
(http://git.linuxtv.org/mchehab/ir.git), and rc5 decoder is already written (but still needs a few fixes).

The new ir-core is creating an abstract way to deal with Remote Controllers, meant to be used not only by IR's, but also for other types of RC, like, bluetooth and USB HID. It will also export a raw event interface, for use with lirc. As this is the core of the RC subsystem, a i2c-specific binding method also doesn't seem to belong there. SO, IMO, the better place is to add it as a static inline function at ir-kbd-i2c.h.

Quoted 2 lines

Ever tried to pass the address of an inline function as another function's parameter? :)

:) Never tried... maybe gcc would to the hard thing, de-inlining it ;)

Well, we need to put this code somewhere. Where are the other probing codes? Probably the better is to put them together.

Hi Mauro,

Quoted 17 lines · Mauro Carvalho Chehab

Quoted 16 lines · Jean Delvare

Quoted 15 lines · Mauro Carvalho Chehab

Please, don't add new things at ir-common module. It basically contains the decoding functions for RC5 and pulse/distance, plus several IR keymaps. With the IR rework I'm doing, this module will go away, after having all the current IR decoders implemented via ir-raw-input binding.

The keymaps were already removed from it, on my experimental tree
(http://git.linuxtv.org/mchehab/ir.git), and rc5 decoder is already written (but still needs a few fixes).

The new ir-core is creating an abstract way to deal with Remote Controllers, meant to be used not only by IR's, but also for other types of RC, like, bluetooth and USB HID. It will also export a raw event interface, for use with lirc. As this is the core of the RC subsystem, a i2c-specific binding method also doesn't seem to belong there. SO, IMO, the better place is to add it as a static inline function at ir-kbd-i2c.h.

Quoted 6 lines

Quoted 2 lines

Ever tried to pass the address of an inline function as another function's parameter? :)

:) Never tried... maybe gcc would to the hard thing, de-inlining it ;)

Well, we need to put this code somewhere. Where are the other probing codes? Probably the better is to put them together.

There are no other probing functions yet, this is the first one. I have added the mechanism to i2c-core for these very IR chips.

Putting all probe functions together would mean moving them to i2c-core. This wasn't my original intent, but after all, it makes some sense. Would you be happy with the following?

* * * * *

From: Jean Delvare <[email protected]>
Subject: V4L/DVB: Use custom I2C probing function mechanism

Now that i2c-core offers the possibility to provide custom probing function for I2C devices, let's make use of it.

Signed-off-by Jean Delvare <[email protected]>

---
 drivers/i2c/i2c-core.c                    |    7 +++++++
 drivers/media/video/cx23885/cx23885-i2c.c |   15 ++++-----------
 drivers/media/video/cx88/cx88-i2c.c       |   19 ++++---------------
 include/linux/i2c.h                       |    3 +++
 4 files changed, 18 insertions(+), 26 deletions(-)

drivers/media/video/cx23885/cx23885-i2c.c +4 -11

@@ -365,17 +365,10 @@ int cx23885_i2c_register(struct cx23885_
 
 		memset(&info, 0, sizeof(struct i2c_board_info));
 		strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
-		/*
-		 * We can't call i2c_new_probed_device() because it uses
-		 * quick writes for probing and the IR receiver device only
-		 * replies to reads.
-		 */
-		if (i2c_smbus_xfer(&bus->i2c_adap, addr_list[0], 0,
-				   I2C_SMBUS_READ, 0, I2C_SMBUS_QUICK,
-				   NULL) >= 0) {
-			info.addr = addr_list[0];
-			i2c_new_device(&bus->i2c_adap, &info);
-		}
+		/* Use quick read command for probe, some IR chips don't
+		 * support writes */
+		i2c_new_probed_device(&bus->i2c_adap, &info, addr_list,
+				      i2c_probe_func_quick_read);
 	}
 
 	return bus->i2c_rc;

drivers/media/video/cx88/cx88-i2c.c +4 -15

@@ -188,24 +188,13 @@ int cx88_i2c_init(struct cx88_core *core
 			0x18, 0x6b, 0x71,
 			I2C_CLIENT_END
 		};
-		const unsigned short *addrp;
 
 		memset(&info, 0, sizeof(struct i2c_board_info));
 		strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
-		/*
-		 * We can't call i2c_new_probed_device() because it uses
-		 * quick writes for probing and at least some R receiver
-		 * devices only reply to reads.
-		 */
-		for (addrp = addr_list; *addrp != I2C_CLIENT_END; addrp++) {
-			if (i2c_smbus_xfer(&core->i2c_adap, *addrp, 0,
-					   I2C_SMBUS_READ, 0,
-					   I2C_SMBUS_QUICK, NULL) >= 0) {
-				info.addr = *addrp;
-				i2c_new_device(&core->i2c_adap, &info);
-				break;
-			}
-		}
+		/* Use quick read command for probe, some IR chips don't
+		 * support writes */
+		i2c_new_probed_device(&core->i2c_adap, &info, addr_list,
+				      i2c_probe_func_quick_read);
 	}
 	return core->i2c_rc;
 }

drivers/i2c/i2c-core.c +7 -0

@@ -1460,6 +1460,13 @@ static int i2c_default_probe(struct i2c_
 	return err >= 0;
 }
 
+int i2c_probe_func_quick_read(struct i2c_adapter *i2c, unsigned short addr)
+{
+	return i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0,
+			      I2C_SMBUS_QUICK, NULL) >= 0;
+}
+EXPORT_SYMBOL_GPL(i2c_probe_func_quick_read);
+
 struct i2c_client *
 i2c_new_probed_device(struct i2c_adapter *adap,
 		      struct i2c_board_info *info,

include/linux/i2c.h +3 -0

@@ -288,6 +288,9 @@ i2c_new_probed_device(struct i2c_adapter
 		      unsigned short const *addr_list,
 		      int (*probe)(struct i2c_adapter *, unsigned short addr));
 
+/* common custom probe functions */
+extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short addr);
+
 /* For devices that use several addresses, use i2c_new_dummy() to make
  * client handles for the extra addresses.
  */

Quoted 6 lines · Jean Delvare

There are no other probing functions yet, this is the first one. I have added the mechanism to i2c-core for these very IR chips.

Putting all probe functions together would mean moving them to i2c-core. This wasn't my original intent, but after all, it makes some sense. Would you be happy with the following?

It seems fine for me. As you're touching on i2c core and on other drivers on this series, I think that the better is if you could apply it on your tree.

For both patches 1 and 2:

Acked-by Mauro Carvalho Chehab <[email protected]>