Skip to contentSkip to conversation

em28xx: i2c debugging cleanups and support for newer eeproms

PATCH Superseded Frank Schäfer 1 participant 12 messages Mar 1, 2013

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover em28xx: i2c debugging cleanups and support for newer eeproms

The first 3 patches clean up / simplify the debugging and info messages a bit.

Patches 4, 5 and 6 fix two bugs I've noticed while working on the eeprom stuff.

Patches 7-10 add support for the newer eeproms with 16 bit address width. This allows us to display the eeprom content, to calculate the eeprom hash for board hints for devices with generic USB IDs, to read the device configuration and to use tveeprom for Hauppauge devices just as with the old eeprom type.

The used eeprom type depends on the chip type/id (confirmed by the Empia datasheets). For unknown chips, the old 8 bit eeprom type is assumed, which makes sure that the eeprom content can't be damaged accidentally.
In video capturing/TV/DVB devices, the new eeprom still has the old eeprom content (device configuration + tveeprom structure for Hauppauge devices) embedded. Camera devices (em25xx, em276x+) however are using a different data structure, which isn't supported yet.

Patch 11 is a follow-up which enables tveeprom for the Hauppauge HVR-930C

All patches have been tested with the following devices:
- Hauppauge HVR-900 (normal 8 bit eeprom)
- Hauppauge HVR-930C (16 bit eeprom, tveeprom)
- SpeedLink VAD Laplace webcam (16 bit eeprom, no device config dataset)

I also checked the USB log of the MSI Digivox ATSC which confirms that non-Hauppauge devices are using the same eeprom format.

Frank Schäfer (11):
em28xx-i2c: replace printk() with the corresponding em28xx macros
em28xx-i2c: get rid of the dprintk2 macro
em28xx-i2c: also print debug messages at debug level 1
em28xx: do not interpret eeprom content if eeprom key is invalid
em28xx: fix eeprom data endianess
em28xx: make sure we are at i2c bus A when calling

    em28xx_i2c_register()

em28xx: add basic support for eeproms with 16 bit address width
em28xx: add helper function for reading data blocks from i2c clients
em28xx: do not store eeprom content permanently
em28xx: extract the device configuration dataset from eeproms with 16
bit address width
em28xx: enable tveeprom for device Hauppauge HVR-930C

 drivers/media/usb/em28xx/em28xx-cards.c |   45 +++--
 drivers/media/usb/em28xx/em28xx-i2c.c   |  284 ++++++++++++++++++++-----------
 drivers/media/usb/em28xx/em28xx.h       |   17 +-

3 Dateien geändert, 225 Zeilen hinzugefügt(+), 121 Zeilen entfernt(-)

Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 1/11 em28xx-i2c: replace printk() with the corresponding em28xx macros

Reduces the number of characters/lines, unifies the code and improves readability.

---
 drivers/media/usb/em28xx/em28xx-i2c.c |   55 ++++++++++++++-------------------

1 Datei geändert, 24 Zeilen hinzugefügt(+), 31 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +24 -31
index 8532c1d..8819b54 100644
@@ -399,7 +399,7 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 	/* Check if board has eeprom */
 	err = i2c_master_recv(&dev->i2c_client, &buf, 0);
 	if (err < 0) {
-		em28xx_errdev("board has no eeprom\n");
+		em28xx_info("board has no eeprom\n");
 		memset(eedata, 0, len);
 		return -ENODEV;
 	}
@@ -408,8 +408,7 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 
 	err = i2c_master_send(&dev->i2c_client, &buf, 1);
 	if (err != 1) {
-		printk(KERN_INFO "%s: Huh, no eeprom present (err=%d)?\n",
-		       dev->name, err);
+		em28xx_errdev("failed to read eeprom (err=%d)\n", err);
 		return err;
 	}
 
@@ -426,9 +425,7 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 
 		if (block !=
 		    (err = i2c_master_recv(&dev->i2c_client, p, block))) {
-			printk(KERN_WARNING
-			       "%s: i2c eeprom read error (err=%d)\n",
-			       dev->name, err);
+			em28xx_errdev("i2c eeprom read error (err=%d)\n", err);
 			return err;
 		}
 		size -= block;
@@ -436,7 +433,7 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 	}
 	for (i = 0; i < len; i++) {
 		if (0 == (i % 16))
-			printk(KERN_INFO "%s: i2c eeprom %02x:", dev->name, i);
+			em28xx_info("i2c eeprom %02x:", i);
 		printk(" %02x", eedata[i]);
 		if (15 == (i % 16))
 			printk("\n");
@@ -445,55 +442,51 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 	if (em_eeprom->id == 0x9567eb1a)
 		dev->hash = em28xx_hash_mem(eedata, len, 32);
 
-	printk(KERN_INFO "%s: EEPROM ID= 0x%08x, EEPROM hash = 0x%08lx\n",
-	       dev->name, em_eeprom->id, dev->hash);
+	em28xx_info("EEPROM ID = 0x%08x, EEPROM hash = 0x%08lx\n",
+		    em_eeprom->id, dev->hash);
 
-	printk(KERN_INFO "%s: EEPROM info:\n", dev->name);
+	em28xx_info("EEPROM info:\n");
 
 	switch (em_eeprom->chip_conf >> 4 & 0x3) {
 	case 0:
-		printk(KERN_INFO "%s:\tNo audio on board.\n", dev->name);
+		em28xx_info("\tNo audio on board.\n");
 		break;
 	case 1:
-		printk(KERN_INFO "%s:\tAC97 audio (5 sample rates)\n",
-				 dev->name);
+		em28xx_info("\tAC97 audio (5 sample rates)\n");
 		break;
 	case 2:
-		printk(KERN_INFO "%s:\tI2S audio, sample rate=32k\n",
-				 dev->name);
+		em28xx_info("\tI2S audio, sample rate=32k\n");
 		break;
 	case 3:
-		printk(KERN_INFO "%s:\tI2S audio, 3 sample rates\n",
-				 dev->name);
+		em28xx_info("\tI2S audio, 3 sample rates\n");
 		break;
 	}
 
 	if (em_eeprom->chip_conf & 1 << 3)
-		printk(KERN_INFO "%s:\tUSB Remote wakeup capable\n", dev->name);
+		em28xx_info("\tUSB Remote wakeup capable\n");
 
 	if (em_eeprom->chip_conf & 1 << 2)
-		printk(KERN_INFO "%s:\tUSB Self power capable\n", dev->name);
+		em28xx_info("\tUSB Self power capable\n");
 
 	switch (em_eeprom->chip_conf & 0x3) {
 	case 0:
-		printk(KERN_INFO "%s:\t500mA max power\n", dev->name);
+		em28xx_info("\t500mA max power\n");
 		break;
 	case 1:
-		printk(KERN_INFO "%s:\t400mA max power\n", dev->name);
+		em28xx_info("\t400mA max power\n");
 		break;
 	case 2:
-		printk(KERN_INFO "%s:\t300mA max power\n", dev->name);
+		em28xx_info("\t300mA max power\n");
 		break;
 	case 3:
-		printk(KERN_INFO "%s:\t200mA max power\n", dev->name);
+		em28xx_info("\t200mA max power\n");
 		break;
 	}
-	printk(KERN_INFO "%s:\tTable at 0x%02x, strings=0x%04x, 0x%04x, 0x%04x\n",
-				dev->name,
-				em_eeprom->string_idx_table,
-				em_eeprom->string1,
-				em_eeprom->string2,
-				em_eeprom->string3);
+	em28xx_info("\tTable at offset 0x%02x, strings=0x%04x, 0x%04x, 0x%04x\n",
+		    em_eeprom->string_idx_table,
+		    em_eeprom->string1,
+		    em_eeprom->string2,
+		    em_eeprom->string3);
 
 	return 0;
 }
@@ -570,8 +563,8 @@ void em28xx_do_i2c_scan(struct em28xx *dev)
 		if (rc < 0)
 			continue;
 		i2c_devicelist[i] = i;
-		printk(KERN_INFO "%s: found i2c device @ 0x%x [%s]\n",
-		       dev->name, i << 1, i2c_devs[i] ? i2c_devs[i] : "???");
+		em28xx_info("found i2c device @ 0x%x [%s]\n",
+			    i << 1, i2c_devs[i] ? i2c_devs[i] : "???");
 	}
 
 	dev->i2c_hash = em28xx_hash_mem(i2c_devicelist,
Signature · 1 line
1.7.10.4
#2 lore raw

PATCH 2/11 em28xx-i2c: get rid of the dprintk2 macro

In reply to Frank Schäfer

There is only a single place where the dprintk2 macro is used, so get rid of it.

---
 drivers/media/usb/em28xx/em28xx-i2c.c |   17 ++++++-----------

1 Datei geändert, 6 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +6 -11
index 8819b54..f970c29 100644
@@ -41,14 +41,6 @@ static unsigned int i2c_debug;
 module_param(i2c_debug, int, 0644);
 MODULE_PARM_DESC(i2c_debug, "enable debug messages [i2c]");
 
-#define dprintk2(lvl, fmt, args...)			\
-do {							\
-	if (i2c_debug >= lvl) {				\
-		printk(KERN_DEBUG "%s at %s: " fmt,	\
-		       dev->name, __func__ , ##args);	\
-      } 						\
-} while (0)
-
 /*
  * em2800_i2c_send_bytes()
  * send up to 4 bytes to the em2800 i2c device
@@ -295,9 +287,12 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 		return 0;
 	for (i = 0; i < num; i++) {
 		addr = msgs[i].addr << 1;
-		dprintk2(2, "%s %s addr=%x len=%d:",
-			 (msgs[i].flags & I2C_M_RD) ? "read" : "write",
-			 i == num - 1 ? "stop" : "nonstop", addr, msgs[i].len);
+		if (i2c_debug >= 2)
+			printk(KERN_DEBUG "%s at %s: %s %s addr=%02x len=%d:",
+			       dev->name, __func__ ,
+			       (msgs[i].flags & I2C_M_RD) ? "read" : "write",
+			       i == num - 1 ? "stop" : "nonstop",
+			       addr, msgs[i].len			     );
 		if (!msgs[i].len) { /* no len: check only for device presence */
 			if (dev->board.is_em2800)
 				rc = em2800_i2c_check_for_device(dev, addr);
Signature · 1 line
1.7.10.4
#3 lore raw

PATCH 3/11 em28xx-i2c: also print debug messages at debug level 1

In reply to Frank Schäfer

The current code uses only a single debug level and all debug messages are printed for i2c_debug >= 2 only. So debug level 1 is actually the same as level 0, which is odd.
Users expect debugging messages to become enabled for anything else than debug level 0.

Fix it and simplify the code a bit by printing the debug messages also at debug level 1;

---
 drivers/media/usb/em28xx/em28xx-i2c.c |   12 ++++++------

1 Datei geändert, 6 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +6 -6
index f970c29..d765567 100644
@@ -287,7 +287,7 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 		return 0;
 	for (i = 0; i < num; i++) {
 		addr = msgs[i].addr << 1;
-		if (i2c_debug >= 2)
+		if (i2c_debug)
 			printk(KERN_DEBUG "%s at %s: %s %s addr=%02x len=%d:",
 			       dev->name, __func__ ,
 			       (msgs[i].flags & I2C_M_RD) ? "read" : "write",
@@ -299,7 +299,7 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 			else
 				rc = em28xx_i2c_check_for_device(dev, addr);
 			if (rc == -ENODEV) {
-				if (i2c_debug >= 2)
+				if (i2c_debug)
 					printk(" no device\n");
 				return rc;
 			}
@@ -313,13 +313,13 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 				rc = em28xx_i2c_recv_bytes(dev, addr,
 							   msgs[i].buf,
 							   msgs[i].len);
-			if (i2c_debug >= 2) {
+			if (i2c_debug) {
 				for (byte = 0; byte < msgs[i].len; byte++)
 					printk(" %02x", msgs[i].buf[byte]);
 			}
 		} else {
 			/* write bytes */
-			if (i2c_debug >= 2) {
+			if (i2c_debug) {
 				for (byte = 0; byte < msgs[i].len; byte++)
 					printk(" %02x", msgs[i].buf[byte]);
 			}
@@ -334,11 +334,11 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 							   i == num - 1);
 		}
 		if (rc < 0) {
-			if (i2c_debug >= 2)
+			if (i2c_debug)
 				printk(" ERROR: %i\n", rc);
 			return rc;
 		}
-		if (i2c_debug >= 2)
+		if (i2c_debug)
 			printk("\n");
 	}
 
Signature · 1 line
1.7.10.4
#4 lore raw

PATCH 4/11 em28xx: do not interpret eeprom content if eeprom key is invalid

In reply to Frank Schäfer

If the eeprom key isn't valid, either a different (currently unknown) format is used or the eeprom is corrupted.
In both cases it doesn't make sense to interpret the data.
Also print an error message.

---
 drivers/media/usb/em28xx/em28xx-i2c.c |    8 ++++++--

1 Datei geändert, 6 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +6 -2
index d765567..9612086 100644
@@ -434,8 +434,12 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 			printk("\n");
 	}
 
-	if (em_eeprom->id == 0x9567eb1a)
-		dev->hash = em28xx_hash_mem(eedata, len, 32);
+	if (em_eeprom->id != 0x9567eb1a) {
+		em28xx_errdev("Unknown eeprom type or eeprom corrupted !");
+		return -ENODEV;
+	}
+
+	dev->hash = em28xx_hash_mem(eedata, len, 32);
 
 	em28xx_info("EEPROM ID = 0x%08x, EEPROM hash = 0x%08lx\n",
 		    em_eeprom->id, dev->hash);
Signature · 1 line
1.7.10.4
#5 lore raw

PATCH 5/11 em28xx: fix eeprom data endianess

In reply to Frank Schäfer

The data is stored as little endian in the eeprom.
Hence the correct data types should be used and the data should be converted to the machine endianess before using it.
The eeprom id (key) also isn't a 32 bit value but 4 separate bytes instead.

---
 drivers/media/usb/em28xx/em28xx-i2c.c |   22 ++++++++++++----------
 drivers/media/usb/em28xx/em28xx.h     |   12 ++++++------

2 Dateien geändert, 18 Zeilen hinzugefügt(+), 16 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +12 -10
index 9612086..19f3e4f 100644
@@ -434,19 +434,21 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 			printk("\n");
 	}
 
-	if (em_eeprom->id != 0x9567eb1a) {
+	if (em_eeprom->id[0] != 0x1a || em_eeprom->id[1] != 0xeb ||
+	    em_eeprom->id[2] != 0x67 || em_eeprom->id[3] != 0x95   ) {
 		em28xx_errdev("Unknown eeprom type or eeprom corrupted !");
 		return -ENODEV;
 	}
 
 	dev->hash = em28xx_hash_mem(eedata, len, 32);
 
-	em28xx_info("EEPROM ID = 0x%08x, EEPROM hash = 0x%08lx\n",
-		    em_eeprom->id, dev->hash);
+	em28xx_info("EEPROM ID = %02x %02x %02x %02x, EEPROM hash = 0x%08lx\n",
+		    em_eeprom->id[0], em_eeprom->id[1],
+		    em_eeprom->id[2], em_eeprom->id[3], dev->hash);
 
 	em28xx_info("EEPROM info:\n");
 
-	switch (em_eeprom->chip_conf >> 4 & 0x3) {
+	switch (le16_to_cpu(em_eeprom->chip_conf) >> 4 & 0x3) {
 	case 0:
 		em28xx_info("\tNo audio on board.\n");
 		break;
@@ -461,13 +463,13 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 		break;
 	}
 
-	if (em_eeprom->chip_conf & 1 << 3)
+	if (le16_to_cpu(em_eeprom->chip_conf) & 1 << 3)
 		em28xx_info("\tUSB Remote wakeup capable\n");
 
-	if (em_eeprom->chip_conf & 1 << 2)
+	if (le16_to_cpu(em_eeprom->chip_conf) & 1 << 2)
 		em28xx_info("\tUSB Self power capable\n");
 
-	switch (em_eeprom->chip_conf & 0x3) {
+	switch (le16_to_cpu(em_eeprom->chip_conf) & 0x3) {
 	case 0:
 		em28xx_info("\t500mA max power\n");
 		break;
@@ -483,9 +485,9 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 	}
 	em28xx_info("\tTable at offset 0x%02x, strings=0x%04x, 0x%04x, 0x%04x\n",
 		    em_eeprom->string_idx_table,
-		    em_eeprom->string1,
-		    em_eeprom->string2,
-		    em_eeprom->string3);
+		    le16_to_cpu(em_eeprom->string1),
+		    le16_to_cpu(em_eeprom->string2),
+		    le16_to_cpu(em_eeprom->string3));
 
 	return 0;
 }
drivers/media/usb/em28xx/em28xx.h +6 -6
index 4160a2a..90266a1 100644
@@ -405,15 +405,15 @@ struct em28xx_board {
 };
 
 struct em28xx_eeprom {
-	u32 id;			/* 0x9567eb1a */
-	u16 vendor_ID;
-	u16 product_ID;
+	u8 id[4];			/* 1a eb 67 95 */
+	__le16 vendor_ID;
+	__le16 product_ID;
 
-	u16 chip_conf;
+	__le16 chip_conf;
 
-	u16 board_conf;
+	__le16 board_conf;
 
-	u16 string1, string2, string3;
+	__le16 string1, string2, string3;
 
 	u8 string_idx_table;
 };
Signature · 1 line
1.7.10.4
#6 lore raw

PATCH 6/11 em28xx: make sure we are at i2c bus A when calling em28xx_i2c_register()

In reply to Frank Schäfer

The current code first configures register EM28XX_R06_I2C_CLK, which includes i2c speed, ack, wait and (on some devices) i2c bus selection.
The register value usually comes from the board definition. em28xx_i2c_register() is called afterwards, which also tries to read the eeprom. If the device uses i2c bus B, eeprom reading fails.

Fix the problem by selecting bus A before calling em28xx_i2c_register() and apply the board settings for register EM28XX_R06_I2C_CLK afterwards. I also noticed that this is what the Windows driver does.
To be sure the i2c bus scan works as expected/before, remove its call from em28xx_i2c_register() and call it directly after the i2c bus has been configured.

---
 drivers/media/usb/em28xx/em28xx-cards.c |   31 ++++++++++++++++++++-----------
 drivers/media/usb/em28xx/em28xx-i2c.c   |    7 -------

2 Dateien geändert, 20 Zeilen hinzugefügt(+), 18 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-cards.c +20 -11
index 9332d05..0d74734 100644
@@ -66,6 +66,10 @@ module_param(usb_xfer_mode, int, 0444);
 MODULE_PARM_DESC(usb_xfer_mode,
 		 "USB transfer mode for frame data (-1 = auto, 0 = prefer isoc, 1 = prefer bulk)");
 
+static unsigned int i2c_scan;
+module_param(i2c_scan, int, 0444);
+MODULE_PARM_DESC(i2c_scan, "scan i2c bus at insmod time");
+
 
 /* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
 static unsigned long em28xx_devused;
@@ -3074,8 +3078,20 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 		snprintf(dev->name, sizeof(dev->name), "%s #%d", chip_name, dev->devno);
 	}
 
+	/* Select i2c bus A (if necessary) */
+	if (dev->chip_id == CHIP_ID_EM2874 ||
+	    dev->chip_id == CHIP_ID_EM28174 ||
+	    dev->chip_id == CHIP_ID_EM2884)
+		em28xx_write_reg_bits(dev, EM28XX_R06_I2C_CLK, 0, EM2874_I2C_SECONDARY_BUS_SELECT);
+	/* Register i2c bus */
+	retval = em28xx_i2c_register(dev);
+	if (retval < 0) {
+		em28xx_errdev("%s: em28xx_i2c_register - error [%d]!\n",
+			__func__, retval);
+		return retval;
+	}
+	/* Configure i2c bus */
 	if (!dev->board.is_em2800) {
-		/* Resets I2C speed */
 		retval = em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, dev->board.i2c_speed);
 		if (retval < 0) {
 			em28xx_errdev("%s: em28xx_write_reg failed!"
@@ -3084,6 +3100,9 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 			return retval;
 		}
 	}
+	/* Scan i2c bus */
+	if (i2c_scan)
+		em28xx_do_i2c_scan(dev);
 
 	retval = v4l2_device_register(&interface->dev, &dev->v4l2_dev);
 	if (retval < 0) {
@@ -3094,14 +3113,6 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 	v4l2_ctrl_handler_init(hdl, 8);
 	dev->v4l2_dev.ctrl_handler = hdl;
 
-	/* register i2c bus */
-	retval = em28xx_i2c_register(dev);
-	if (retval < 0) {
-		em28xx_errdev("%s: em28xx_i2c_register - error [%d]!\n",
-			__func__, retval);
-		goto unregister_dev;
-	}
-
 	/*
 	 * Default format, used for tvp5150 or saa711x output formats
 	 */
@@ -3173,8 +3184,6 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 fail:
 	em28xx_i2c_unregister(dev);
 	v4l2_ctrl_handler_free(&dev->ctrl_handler);
-
-unregister_dev:
 	v4l2_device_unregister(&dev->v4l2_dev);
 
 	return retval;
drivers/media/usb/em28xx/em28xx-i2c.c +0 -7
index 19f3e4f..ebe4b20 100644
@@ -33,10 +33,6 @@
 
 /* ----------------------------------------------------------- */
 
-static unsigned int i2c_scan;
-module_param(i2c_scan, int, 0444);
-MODULE_PARM_DESC(i2c_scan, "scan i2c bus at insmod time");
-
 static unsigned int i2c_debug;
 module_param(i2c_debug, int, 0644);
 MODULE_PARM_DESC(i2c_debug, "enable debug messages [i2c]");
@@ -606,9 +602,6 @@ int em28xx_i2c_register(struct em28xx *dev)
 		return retval;
 	}
 
-	if (i2c_scan)
-		em28xx_do_i2c_scan(dev);
-
 	return 0;
 }
 
Signature · 1 line
1.7.10.4
#7 lore raw

PATCH 7/11 em28xx: add basic support for eeproms with 16 bit address width

In reply to Frank Schäfer

Newer devices (em2874, em2884, em28174, em25xx, em27[6,7,8]x) use eeproms with 16 bit instead of 8 bit address width.
The used eeprom type depends on the chip type, which makes sure eeproms can't be damaged.

This patch adds basic support for 16 bit eeproms only, which includes
- reading the content
- calculating the eeprom hash
- displaying the content

The eeprom content uses a different format, for which support will be added with subsequent patches.

Tested with the "Hauppauge HVR-930C" and the "Speedlink VAD Laplace webcam" (with additional experimental patches).

---
 drivers/media/usb/em28xx/em28xx-cards.c |    4 ++
 drivers/media/usb/em28xx/em28xx-i2c.c   |   69 ++++++++++++++++++++-----------
 drivers/media/usb/em28xx/em28xx.h       |    1 +

3 Dateien geändert, 49 Zeilen hinzugefügt(+), 25 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-cards.c +4 -0
index 0d74734..fa51f81 100644
@@ -2187,6 +2187,7 @@ static struct em28xx_hash_table em28xx_i2c_hash[] = {
 	{0x4ba50080, EM2861_BOARD_GADMEI_UTV330PLUS, TUNER_TNF_5335MF},
 	{0x6b800080, EM2874_BOARD_LEADERSHIP_ISDBT, TUNER_ABSENT},
 };
+/* NOTE: introduce a separate hash table for devices with 16 bit eeproms */
 
 /* I2C possible address to saa7115, tvp5150, msp3400, tvaudio */
 static unsigned short saa711x_addrs[] = {
@@ -3023,11 +3024,13 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 			chip_name = "em2874";
 			dev->reg_gpio_num = EM2874_R80_GPIO;
 			dev->wait_after_write = 0;
+			dev->eeprom_addrwidth_16bit = 1;
 			break;
 		case CHIP_ID_EM28174:
 			chip_name = "em28174";
 			dev->reg_gpio_num = EM2874_R80_GPIO;
 			dev->wait_after_write = 0;
+			dev->eeprom_addrwidth_16bit = 1;
 			break;
 		case CHIP_ID_EM2883:
 			chip_name = "em2882/3";
@@ -3037,6 +3040,7 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 			chip_name = "em2884";
 			dev->reg_gpio_num = EM2874_R80_GPIO;
 			dev->wait_after_write = 0;
+			dev->eeprom_addrwidth_16bit = 1;
 			break;
 		default:
 			printk(KERN_INFO DRIVER_NAME
drivers/media/usb/em28xx/em28xx-i2c.c +44 -25

107 lines not on this page · raw message

drivers/media/usb/em28xx/em28xx.h +1 -0
index 90266a1..139dfe5 100644
@@ -510,6 +510,7 @@ struct em28xx {
 	/* i2c i/o */
 	struct i2c_adapter i2c_adap;
 	struct i2c_client i2c_client;
+	unsigned char eeprom_addrwidth_16bit:1;
 	/* video for linux */
 	int users;		/* user count for exclusive use */
 	int streaming_users;    /* Number of actively streaming users */
Signature · 1 line
1.7.10.4
#8 lore raw

PATCH 8/11 em28xx: add helper function for reading data blocks from i2c clients

In reply to Frank Schäfer

Add a helper function for reading data blocks from i2c devices with 8 or 16 bit address width and 8 bit register width.
This allows us to reduce the size of new code added by the following patches. Works only for devices with activated register auto incrementation.

---
 drivers/media/usb/em28xx/em28xx-i2c.c |   74 ++++++++++++++++++++-------------

1 Datei geändert, 46 Zeilen hinzugefügt(+), 28 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +46 -28
index 7185812..a3e9547 100644
@@ -366,51 +366,69 @@ static inline unsigned long em28xx_hash_mem(char *buf, int length, int bits)
 	return (hash >> (32 - bits)) & 0xffffffffUL;
 }
 
+/* Helper function to read data blocks from i2c clients with 8 or 16 bit
+ * address width, 8 bit register width and auto incrementation been activated */
+static int em28xx_i2c_read_block(struct em28xx *dev, u16 addr, bool addr_w16,
+				 u16 len, u8 *data)
+{
+	int remain = len, rsize, rsize_max, ret;
+	u8 buf[2];
+
+	/* Sanity check */
+	if (addr + remain > (256 + addr_w16*256))
+		return -EINVAL;
+	/* Select address */
+	buf[0] = addr >> 8;
+	buf[1] = addr & 0xff;
+	ret = i2c_master_send(&dev->i2c_client, buf + !addr_w16, 1 + addr_w16);
+	if (ret < 0)
+		return ret;
+	/* Read data */
+	if (dev->board.is_em2800)
+		rsize_max = 4;
+	else
+		rsize_max = 64;
+	while (remain > 0) {
+		if (remain > rsize_max)
+			rsize = rsize_max;
+		else
+			rsize = remain;
+
+		ret = i2c_master_recv(&dev->i2c_client, data, rsize);
+		if (ret < 0)
+			return ret;
+
+		remain -= rsize;
+		data += rsize;
+	}
+
+	return len;
+}
+
 static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 {
-	unsigned char buf[2], *p = eedata;
+	unsigned char buf, *p = eedata;
 	struct em28xx_eeprom *em_eeprom = (void *)eedata;
-	int i, err, size = len, block, block_max;
+	int i, err;
 
 	dev->i2c_client.addr = 0xa0 >> 1;
 
 	/* Check if board has eeprom */
-	err = i2c_master_recv(&dev->i2c_client, buf, 0);
+	err = i2c_master_recv(&dev->i2c_client, &buf, 0);
 	if (err < 0) {
 		em28xx_info("board has no eeprom\n");
 		memset(eedata, 0, len);
 		return -ENODEV;
 	}
 
-	/* Select address memory address 0x00(00) */
-	buf[0] = 0;
-	buf[1] = 0;
-	err = i2c_master_send(&dev->i2c_client, buf, 1 + dev->eeprom_addrwidth_16bit);
-	if (err != 1 + dev->eeprom_addrwidth_16bit) {
+	/* Read EEPROM content */
+	err = em28xx_i2c_read_block(dev, 0x0000, dev->eeprom_addrwidth_16bit,
+				    len, p);
+	if (err != len) {
 		em28xx_errdev("failed to read eeprom (err=%d)\n", err);
 		return err;
 	}
 
-	/* Read eeprom content */
-	if (dev->board.is_em2800)
-		block_max = 4;
-	else
-		block_max = 64;
-	while (size > 0) {
-		if (size > block_max)
-			block = block_max;
-		else
-			block = size;
-
-		if (block !=
-		    (err = i2c_master_recv(&dev->i2c_client, p, block))) {
-			em28xx_errdev("i2c eeprom read error (err=%d)\n", err);
-			return err;
-		}
-		size -= block;
-		p += block;
-	}
-
 	/* Display eeprom content */
 	for (i = 0; i < len; i++) {
 		if (0 == (i % 16)) {
Signature · 1 line
1.7.10.4
#9 lore raw

PATCH 9/11 em28xx: do not store eeprom content permanently

In reply to Frank Schäfer

We currently reserve an array of 256 bytes for the eeprom content in the device struct. For eeproms with 16 bit address width it might even be necessary to increase the buffer size further.

Having such a big chunk of memory reserved even if the device has no eeprom and keeping it after it has already been processed seems to be a waste of memory.

Change the code to allocate + free the eeprom memory dynamically. This also makes it possible to handle different dataset sizes depending on what is stored/found in the eeprom.

---
 drivers/media/usb/em28xx/em28xx-cards.c |    9 +++++++-
 drivers/media/usb/em28xx/em28xx-i2c.c   |   35 +++++++++++++++++++------------
 drivers/media/usb/em28xx/em28xx.h       |    2 +-

3 Dateien geändert, 31 Zeilen hinzugefügt(+), 15 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-cards.c +8 -1
index fa51f81..2e3d3ad 100644
@@ -2743,6 +2743,9 @@ static void em28xx_card_setup(struct em28xx *dev)
 	case EM2883_BOARD_HAUPPAUGE_WINTV_HVR_950:
 	{
 		struct tveeprom tv;
+
+		if (dev->eedata == NULL)
+			break;
 #if defined(CONFIG_MODULES) && defined(MODULE)
 		request_module("tveeprom");
 #endif
@@ -2796,7 +2799,7 @@ static void em28xx_card_setup(struct em28xx *dev)
 		em28xx_set_mode(dev, EM28XX_ANALOG_MODE);
 		break;
 
-/*
+		/*
 		 * The Dikom DK300 is detected as an Kworld VS-DVB-T 323UR.
 		 *
 		 * This occurs because they share identical USB vendor and
@@ -2831,6 +2834,10 @@ static void em28xx_card_setup(struct em28xx *dev)
 				"addresses)\n\n");
 	}
 
+	/* Free eeprom data memory */
+	kfree(dev->eedata);
+	dev->eedata = NULL;
+
 	/* Allow override tuner type by a module parameter */
 	if (tuner >= 0)
 		dev->tuner_type = tuner;
drivers/media/usb/em28xx/em28xx-i2c.c +22 -13
index a3e9547..dfbc22e 100644
@@ -405,27 +405,33 @@ static int em28xx_i2c_read_block(struct em28xx *dev, u16 addr, bool addr_w16,
 	return len;
 }
 
-static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
+static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char **eedata, int len)
 {
-	unsigned char buf, *p = eedata;
-	struct em28xx_eeprom *em_eeprom = (void *)eedata;
+	u8 buf, *data;
+	struct em28xx_eeprom *em_eeprom;
 	int i, err;
 
+	*eedata = NULL;
+
 	dev->i2c_client.addr = 0xa0 >> 1;
 
 	/* Check if board has eeprom */
 	err = i2c_master_recv(&dev->i2c_client, &buf, 0);
 	if (err < 0) {
 		em28xx_info("board has no eeprom\n");
-		memset(eedata, 0, len);
 		return -ENODEV;
 	}
 
+	data = kzalloc(len, GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
 	/* Read EEPROM content */
 	err = em28xx_i2c_read_block(dev, 0x0000, dev->eeprom_addrwidth_16bit,
-				    len, p);
+				    len, data);
 	if (err != len) {
 		em28xx_errdev("failed to read eeprom (err=%d)\n", err);
+		kfree(data);
 		return err;
 	}
 
@@ -437,19 +443,19 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 			else
 				em28xx_info("i2c eeprom %02x:", i);
 		}
-		printk(" %02x", eedata[i]);
+		printk(" %02x", data[i]);
 		if (15 == (i % 16))
 			printk("\n");
 	}
 
 	if (dev->eeprom_addrwidth_16bit &&
-	    eedata[0] == 0x26 && eedata[3] == 0x00) {
+	    data[0] == 0x26 && data[3] == 0x00) {
 		/* new eeprom format; size 4-64kb */
-		dev->hash = em28xx_hash_mem(eedata, len, 32);
+		dev->hash = em28xx_hash_mem(data, len, 32);
 		em28xx_info("EEPROM hash = 0x%08lx\n", dev->hash);
 		em28xx_info("EEPROM info: boot page address = 0x%02x04, "
 			    "boot configuration = 0x%02x\n",
-			    eedata[1], eedata[2]);
+			    data[1], data[2]);
 		/* boot configuration (address 0x0002):
 		 * [0]   microcode download speed: 1 = 400 kHz; 0 = 100 kHz
 		 * [1]   always selects 12 kb RAM
@@ -467,13 +473,16 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
 		 */
 
 		return 0;
-	} else if (em_eeprom->id[0] != 0x1a || em_eeprom->id[1] != 0xeb ||
-		   em_eeprom->id[2] != 0x67 || em_eeprom->id[3] != 0x95   ) {
+	} else if (data[0] != 0x1a || data[1] != 0xeb ||
+		   data[2] != 0x67 || data[3] != 0x95   ) {
 		em28xx_info("unknown eeprom format or eeprom corrupted !\n");
 		return -ENODEV;
 	}
 
-	dev->hash = em28xx_hash_mem(eedata, len, 32);
+	*eedata = data;
+	em_eeprom = (void *)eedata;
+
+	dev->hash = em28xx_hash_mem(data, len, 32);
 
 	em28xx_info("EEPROM ID = %02x %02x %02x %02x, EEPROM hash = 0x%08lx\n",
 		    em_eeprom->id[0], em_eeprom->id[1],
@@ -631,7 +640,7 @@ int em28xx_i2c_register(struct em28xx *dev)
 	dev->i2c_client = em28xx_client_template;
 	dev->i2c_client.adapter = &dev->i2c_adap;
 
-	retval = em28xx_i2c_eeprom(dev, dev->eedata, sizeof(dev->eedata));
+	retval = em28xx_i2c_eeprom(dev, &dev->eedata, 256);
 	if ((retval < 0) && (retval != -ENODEV)) {
 		em28xx_errdev("%s: em28xx_i2_eeprom failed! retval [%d]\n",
 			__func__, retval);
drivers/media/usb/em28xx/em28xx.h +1 -1
index 139dfe5..77f600d 100644
@@ -562,7 +562,7 @@ struct em28xx {
 	/* resources in use */
 	unsigned int resources;
 
-	unsigned char eedata[256];
+	u8 *eedata;	/* currently always 256 bytes */
 
 	/* Isoc control struct */
 	struct em28xx_dmaqueue vidq;
Signature · 1 line
1.7.10.4
#10 lore raw

PATCH 10/11 em28xx: extract the device configuration dataset from eeproms with 16 bit address width

In reply to Frank Schäfer

The new eeproms with 16 address width still have the the device config dataset (the content of the old 8 bit eeproms) embedded.
Hauppauge also continues to include the tveeprom data structure inside this dataset in their devices.
The start address of the dataset depends on the start address of the microcode and a variable additional offset.

It should be mentioned that camera devices seem to use a different dataset type, which is not yet supported.

Tested with devices "Hauppauge HVR-930C". I've also checked the USB-log from the "MSI Digivox ATSC" and it works the same way.

---
 drivers/media/usb/em28xx/em28xx-i2c.c |  117 +++++++++++++++++++++++----------
 drivers/media/usb/em28xx/em28xx.h     |    4 +-

2 Dateien geändert, 85 Zeilen hinzugefügt(+), 36 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +82 -35
index dfbc22e..44bef43 100644
@@ -405,13 +405,18 @@ static int em28xx_i2c_read_block(struct em28xx *dev, u16 addr, bool addr_w16,
 	return len;
 }
 
-static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char **eedata, int len)
+static int em28xx_i2c_eeprom(struct em28xx *dev, u8 **eedata, u16 *eedata_len)
 {
-	u8 buf, *data;
-	struct em28xx_eeprom *em_eeprom;
+	const u16 len = 256;
+	/* FIXME common length/size for bytes to read, to display, hash
+	 * calculation and returned device dataset. Simplifies the code a lot,
+	 * but we might have to deal with multiple sizes in the future !      */
 	int i, err;
+	struct em28xx_eeprom *dev_config;
+	u8 buf, *data;
 
 	*eedata = NULL;
+	*eedata_len = 0;
 
 	dev->i2c_client.addr = 0xa0 >> 1;
 
@@ -431,8 +436,7 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char **eedata, int len
 				    len, data);
 	if (err != len) {
 		em28xx_errdev("failed to read eeprom (err=%d)\n", err);
-		kfree(data);
-		return err;
+		goto error;
 	}
 
 	/* Display eeprom content */
@@ -447,15 +451,25 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char **eedata, int len
 		if (15 == (i % 16))
 			printk("\n");
 	}
+	if (dev->eeprom_addrwidth_16bit)
+		em28xx_info("i2c eeprom %04x: ... (skipped)\n", i);
 
 	if (dev->eeprom_addrwidth_16bit &&
 	    data[0] == 0x26 && data[3] == 0x00) {
 		/* new eeprom format; size 4-64kb */
+		u16 mc_start;
+		u16 hwconf_offset;
+
 		dev->hash = em28xx_hash_mem(data, len, 32);
-		em28xx_info("EEPROM hash = 0x%08lx\n", dev->hash);
-		em28xx_info("EEPROM info: boot page address = 0x%02x04, "
+		mc_start = (data[1] << 8) + 4;	/* usually 0x0004 */
+
+		em28xx_info("EEPROM ID = %02x %02x %02x %02x, "
+			    "EEPROM hash = 0x%08lx\n",
+			    data[0], data[1], data[2], data[3], dev->hash);
+		em28xx_info("EEPROM info:\n");
+		em28xx_info("\tmicrocode start address = 0x%04x, "
 			    "boot configuration = 0x%02x\n",
-			    data[1], data[2]);
+			    mc_start, data[2]);
 		/* boot configuration (address 0x0002):
 		 * [0]   microcode download speed: 1 = 400 kHz; 0 = 100 kHz
 		 * [1]   always selects 12 kb RAM
@@ -465,32 +479,61 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char **eedata, int len
 		 *       characterization
 		 */
 
-		/* FIXME:
-		 * - read more than 256 bytes / addresses above 0x00ff
-		 * - find offset for device config dataset and extract it
-		 * - decrypt eeprom data for camera bridges (em25xx, em276x+)
-		 * - use separate/different eeprom hashes (not yet used)
+		/* Read hardware config dataset offset from address
+		 * (microcode start + 46)			    */
+		err = em28xx_i2c_read_block(dev, mc_start + 46, 1, 2, data);
+		if (err != 2) {
+			em28xx_errdev("failed to read hardware configuration data from eeprom (err=%d)\n",
+				      err);
+			goto error;
+		}
+
+		/* Calculate hardware config dataset start address */
+		hwconf_offset = mc_start + data[0] + (data[1] << 8);
+
+		/* Read hardware config dataset */
+		/* NOTE: the microcode copy can be multiple pages long, but
+		 * we assume the hardware config dataset is the same as in
+		 * the old eeprom and not longer than 256 bytes.
+		 * tveeprom is currently also limited to 256 bytes.
 		 */
+		err = em28xx_i2c_read_block(dev, hwconf_offset, 1, len, data);
+		if (err != len) {
+			em28xx_errdev("failed to read hardware configuration data from eeprom (err=%d)\n",
+				      err);
+			goto error;
+		}
 
-		return 0;
-	} else if (data[0] != 0x1a || data[1] != 0xeb ||
-		   data[2] != 0x67 || data[3] != 0x95   ) {
+		/* Verify hardware config dataset */
+		/* NOTE: not all devices provide this type of dataset */
+		if (data[0] != 0x1a || data[1] != 0xeb ||
+		    data[2] != 0x67 || data[3] != 0x95    ) {
+			em28xx_info("no hardware configuration dataset found in eeprom\n");
+			kfree(data);
+			return 0;
+		}
+
+		/* TODO: decrypt eeprom data for camera bridges (em25xx, em276x+) */
+
+	} else if (!dev->eeprom_addrwidth_16bit &&
+		   data[0] == 0x1a && data[1] == 0xeb &&
+		   data[2] == 0x67 && data[3] == 0x95   ) {
+		dev->hash = em28xx_hash_mem(data, len, 32);
+		em28xx_info("EEPROM ID = %02x %02x %02x %02x, "
+			    "EEPROM hash = 0x%08lx\n",
+			    data[0], data[1], data[2], data[3], dev->hash);
+		em28xx_info("EEPROM info:\n");
+	} else {
 		em28xx_info("unknown eeprom format or eeprom corrupted !\n");
-		return -ENODEV;
+		err = -ENODEV;
+		goto error;
 	}
 
 	*eedata = data;
-	em_eeprom = (void *)eedata;
+	*eedata_len = len;
+	dev_config = (void *)eedata;
 
-	dev->hash = em28xx_hash_mem(data, len, 32);
-
-	em28xx_info("EEPROM ID = %02x %02x %02x %02x, EEPROM hash = 0x%08lx\n",
-		    em_eeprom->id[0], em_eeprom->id[1],
-		    em_eeprom->id[2], em_eeprom->id[3], dev->hash);
-
-	em28xx_info("EEPROM info:\n");
-
-	switch (le16_to_cpu(em_eeprom->chip_conf) >> 4 & 0x3) {
+	switch (le16_to_cpu(dev_config->chip_conf) >> 4 & 0x3) {
 	case 0:
 		em28xx_info("\tNo audio on board.\n");
 		break;
@@ -505,13 +548,13 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char **eedata, int len
 		break;
 	}
 
-	if (le16_to_cpu(em_eeprom->chip_conf) & 1 << 3)
+	if (le16_to_cpu(dev_config->chip_conf) & 1 << 3)
 		em28xx_info("\tUSB Remote wakeup capable\n");
 
-	if (le16_to_cpu(em_eeprom->chip_conf) & 1 << 2)
+	if (le16_to_cpu(dev_config->chip_conf) & 1 << 2)
 		em28xx_info("\tUSB Self power capable\n");
 
-	switch (le16_to_cpu(em_eeprom->chip_conf) & 0x3) {
+	switch (le16_to_cpu(dev_config->chip_conf) & 0x3) {
 	case 0:
 		em28xx_info("\t500mA max power\n");
 		break;
@@ -526,12 +569,16 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char **eedata, int len
 		break;
 	}
 	em28xx_info("\tTable at offset 0x%02x, strings=0x%04x, 0x%04x, 0x%04x\n",
-		    em_eeprom->string_idx_table,
-		    le16_to_cpu(em_eeprom->string1),
-		    le16_to_cpu(em_eeprom->string2),
-		    le16_to_cpu(em_eeprom->string3));
+		    dev_config->string_idx_table,
+		    le16_to_cpu(dev_config->string1),
+		    le16_to_cpu(dev_config->string2),
+		    le16_to_cpu(dev_config->string3));
 
 	return 0;
+
+error:
+	kfree(data);
+	return err;
 }
 
 /* ----------------------------------------------------------- */
@@ -640,7 +687,7 @@ int em28xx_i2c_register(struct em28xx *dev)
 	dev->i2c_client = em28xx_client_template;
 	dev->i2c_client.adapter = &dev->i2c_adap;
 
-	retval = em28xx_i2c_eeprom(dev, &dev->eedata, 256);
+	retval = em28xx_i2c_eeprom(dev, &dev->eedata, &dev->eedata_len);
 	if ((retval < 0) && (retval != -ENODEV)) {
 		em28xx_errdev("%s: em28xx_i2_eeprom failed! retval [%d]\n",
 			__func__, retval);
drivers/media/usb/em28xx/em28xx.h +3 -1
index 77f600d..2d6d31a 100644
@@ -562,7 +562,9 @@ struct em28xx {
 	/* resources in use */
 	unsigned int resources;
 
-	u8 *eedata;	/* currently always 256 bytes */
+	/* eeprom content */
+	u8 *eedata;
+	u16 eedata_len;
 
 	/* Isoc control struct */
 	struct em28xx_dmaqueue vidq;
Signature · 1 line
1.7.10.4
#11 lore raw

PATCH 11/11 em28xx: enable tveeprom for device Hauppauge HVR-930C

In reply to Frank Schäfer
---
 drivers/media/usb/em28xx/em28xx-cards.c |    1 +

1 Datei geändert, 1 Zeile hinzugefügt(+)

drivers/media/usb/em28xx/em28xx-cards.c +1 -0
index 2e3d3ad..5a5b637 100644
@@ -2741,6 +2741,7 @@ static void em28xx_card_setup(struct em28xx *dev)
 	case EM2880_BOARD_HAUPPAUGE_WINTV_HVR_900_R2:
 	case EM2883_BOARD_HAUPPAUGE_WINTV_HVR_850:
 	case EM2883_BOARD_HAUPPAUGE_WINTV_HVR_950:
+	case EM2884_BOARD_HAUPPAUGE_WINTV_HVR_930C:
 	{
 		struct tveeprom tv;
 
Signature · 1 line
1.7.10.4