Skip to contentSkip to conversation

em28xx: overhaul em28xx_capture_area_set()

PATCH In review Frank Schäfer 2 participants 3 messages Jan 20, 2013 – Feb 6, 2013

Subsystems (2)
Media (V4L/DVB) EM28XX VIDEO4LINUX driver
lore
Share & tools
lore

Frank Schäfer

author
#0 lore raw

PATCH 1/1 em28xx: overhaul em28xx_capture_area_set()

- move the bit shifting of width+height values inside the function
- fix the debug message format and output values
- add comment about the size limit (e.g. EM277x supports >2MPix)
- make void, because error checking is incomplete and we never check the returned value (we would continue anyway)

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-core.c |   22 ++++++++++++----------

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

drivers/media/usb/em28xx/em28xx-core.c +12 -10
index 210859a..f516a63 100644
@@ -733,22 +733,24 @@ static int em28xx_accumulator_set(struct em28xx *dev, u8 xmin, u8 xmax,
 	return em28xx_write_regs(dev, EM28XX_R2B_YMAX, &ymax, 1);
 }
 
-static int em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,
+static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,
 				   u16 width, u16 height)
 {
-	u8 cwidth = width;
-	u8 cheight = height;
-	u8 overflow = (height >> 7 & 0x02) | (width >> 8 & 0x01);
+	u8 cwidth = width >> 2;
+	u8 cheight = height >> 2;
+	u8 overflow = (height >> 9 & 0x02) | (width >> 10 & 0x01);
+	/* NOTE: size limit: 2047x1023 = 2MPix */
 
-	em28xx_coredbg("em28xx Area Set: (%d,%d)\n",
-			(width | (overflow & 2) << 7),
-			(height | (overflow & 1) << 8));
+	em28xx_coredbg("capture area set to (%d,%d): %dx%d\n",
+		       hstart, vstart,
+		       ((overflow & 2) << 9 | cwidth << 2),
+		       ((overflow & 1) << 10 | cheight << 2));
 
 	em28xx_write_regs(dev, EM28XX_R1C_HSTART, &hstart, 1);
 	em28xx_write_regs(dev, EM28XX_R1D_VSTART, &vstart, 1);
 	em28xx_write_regs(dev, EM28XX_R1E_CWIDTH, &cwidth, 1);
 	em28xx_write_regs(dev, EM28XX_R1F_CHEIGHT, &cheight, 1);
-	return em28xx_write_regs(dev, EM28XX_R1B_OFLOW, &overflow, 1);
+	em28xx_write_regs(dev, EM28XX_R1B_OFLOW, &overflow, 1);
 }
 
 static int em28xx_scaler_set(struct em28xx *dev, u16 h, u16 v)

@@ -801,9 +803,9 @@ int em28xx_resolution_set(struct em28xx *dev)
 	   it out, we end up with the same format as the rest of the VBI
 	   region */
 	if (em28xx_vbi_supported(dev) == 1)
-		em28xx_capture_area_set(dev, 0, 2, width >> 2, height >> 2);
+		em28xx_capture_area_set(dev, 0, 2, width, height);
 	else
-		em28xx_capture_area_set(dev, 0, 0, width >> 2, height >> 2);
+		em28xx_capture_area_set(dev, 0, 0, width, height);
 
 	return em28xx_scaler_set(dev, dev->hscale, dev->vscale);
 }
Signature · 1 line
1.7.10.4
#1 lore raw

Em Sun, 20 Jan 2013 14:26:47 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 19 lines

- move the bit shifting of width+height values inside the function
- fix the debug message format and output values
- add comment about the size limit (e.g. EM277x supports >2MPix)
- make void, because error checking is incomplete and we never check the returned value (we would continue anyway)

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-core.c |   22 ++++++++++++----------

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

drivers/media/usb/em28xx/em28xx-core.c +0 -0
index 210859a..f516a63 100644
@@ -733,22 +733,24 @@ static int em28xx_accumulator_set(struct em28xx *dev, u8 xmin, u8 xmax,
 	return em28xx_write_regs(dev, EM28XX_R2B_YMAX, &ymax, 1);
 }

drivers/media/usb/em28xx/em28xx-core.c static int em28xx_accumulator_set(struct em28xx *dev, u8 xmin, u8 xmax, · line 741

Quoted 10 lines
drivers/media/usb/em28xx/em28xx-core.c +4 -4
-static int em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,
+static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,
 				   u16 width, u16 height)
 {
-	u8 cwidth = width;
-	u8 cheight = height;
-	u8 overflow = (height >> 7 & 0x02) | (width >> 8 & 0x01);
+	u8 cwidth = width >> 2;
+	u8 cheight = height >> 2;
+	u8 overflow = (height >> 9 & 0x02) | (width >> 10 & 0x01);

Hmm.. why did you change the above overflow bits? That change doesn't sound right to me. Ok, I don't have the datasheets, so I might be wrong, but if is there a bug there, please submit the bug fix into a separate patch, clearly explaining why such change is needed.

Quoted 31 lines
drivers/media/usb/em28xx/em28xx-core.c +8 -6
+	/* NOTE: size limit: 2047x1023 = 2MPix */
 
-	em28xx_coredbg("em28xx Area Set: (%d,%d)\n",
-			(width | (overflow & 2) << 7),
-			(height | (overflow & 1) << 8));
+	em28xx_coredbg("capture area set to (%d,%d): %dx%d\n",
+		       hstart, vstart,
+		       ((overflow & 2) << 9 | cwidth << 2),
+		       ((overflow & 1) << 10 | cheight << 2));
 
 	em28xx_write_regs(dev, EM28XX_R1C_HSTART, &hstart, 1);
 	em28xx_write_regs(dev, EM28XX_R1D_VSTART, &vstart, 1);
 	em28xx_write_regs(dev, EM28XX_R1E_CWIDTH, &cwidth, 1);
 	em28xx_write_regs(dev, EM28XX_R1F_CHEIGHT, &cheight, 1);
-	return em28xx_write_regs(dev, EM28XX_R1B_OFLOW, &overflow, 1);
+	em28xx_write_regs(dev, EM28XX_R1B_OFLOW, &overflow, 1);
 }
 
 static int em28xx_scaler_set(struct em28xx *dev, u16 h, u16 v)

@@ -801,9 +803,9 @@ int em28xx_resolution_set(struct em28xx *dev)
 	   it out, we end up with the same format as the rest of the VBI
 	   region */
 	if (em28xx_vbi_supported(dev) == 1)
-		em28xx_capture_area_set(dev, 0, 2, width >> 2, height >> 2);
+		em28xx_capture_area_set(dev, 0, 2, width, height);
 	else
-		em28xx_capture_area_set(dev, 0, 0, width >> 2, height >> 2);
+		em28xx_capture_area_set(dev, 0, 0, width, height);
 
 	return em28xx_scaler_set(dev, dev->hscale, dev->vscale);
 }
Signature · 2 lines
Cheers,
Mauro
author
#2 lore raw

Am 05.02.2013 23:48, schrieb Mauro Carvalho Chehab:

Quoted 22 lines

Em Sun, 20 Jan 2013 14:26:47 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 19 lines

- move the bit shifting of width+height values inside the function
- fix the debug message format and output values
- add comment about the size limit (e.g. EM277x supports >2MPix)
- make void, because error checking is incomplete and we never check the returned value (we would continue anyway)

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-core.c |   22 ++++++++++++----------

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

drivers/media/usb/em28xx/em28xx-core.c +0 -0
index 210859a..f516a63 100644
@@ -733,22 +733,24 @@ static int em28xx_accumulator_set(struct em28xx *dev, u8 xmin, u8 xmax,
 	return em28xx_write_regs(dev, EM28XX_R2B_YMAX, &ymax, 1);
 }
Quoted 11 lines

drivers/media/usb/em28xx/em28xx-core.c static int em28xx_accumulator_set(struct em28xx *dev, u8 xmin, u8 xmax, · line 741

Quoted 10 lines
drivers/media/usb/em28xx/em28xx-core.c +4 -4
-static int em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,
+static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,
 				   u16 width, u16 height)
 {
-	u8 cwidth = width;
-	u8 cheight = height;
-	u8 overflow = (height >> 7 & 0x02) | (width >> 8 & 0x01);
+	u8 cwidth = width >> 2;
+	u8 cheight = height >> 2;
+	u8 overflow = (height >> 9 & 0x02) | (width >> 10 & 0x01);

Hmm.. why did you change the above overflow bits?

Read the complete patch, the answer is...
...

Quoted 23 lines

That change doesn't
sound right to me. Ok, I don't have the datasheets, so I might be wrong, but if is there a bug there, please submit the bug fix into a separate patch, clearly explaining why such change is needed.

Quoted 18 lines
drivers/media/usb/em28xx/em28xx-core.c +6 -4
+	/* NOTE: size limit: 2047x1023 = 2MPix */
 
-	em28xx_coredbg("em28xx Area Set: (%d,%d)\n",
-			(width | (overflow & 2) << 7),
-			(height | (overflow & 1) << 8));
+	em28xx_coredbg("capture area set to (%d,%d): %dx%d\n",
+		       hstart, vstart,
+		       ((overflow & 2) << 9 | cwidth << 2),
+		       ((overflow & 1) << 10 | cheight << 2));
 
 	em28xx_write_regs(dev, EM28XX_R1C_HSTART, &hstart, 1);
 	em28xx_write_regs(dev, EM28XX_R1D_VSTART, &vstart, 1);
 	em28xx_write_regs(dev, EM28XX_R1E_CWIDTH, &cwidth, 1);
 	em28xx_write_regs(dev, EM28XX_R1F_CHEIGHT, &cheight, 1);
-	return em28xx_write_regs(dev, EM28XX_R1B_OFLOW, &overflow, 1);
+	em28xx_write_regs(dev, EM28XX_R1B_OFLOW, &overflow, 1);
 }
Quoted 10 lines

drivers/media/usb/em28xx/em28xx-core.c int em28xx_resolution_set(struct em28xx *dev) · line 808

Quoted 10 lines
drivers/media/usb/em28xx/em28xx-core.c +2 -2
 static int em28xx_scaler_set(struct em28xx *dev, u16 h, u16 v)

@@ -801,9 +803,9 @@ int em28xx_resolution_set(struct em28xx *dev)
 	   it out, we end up with the same format as the rest of the VBI
 	   region */
 	if (em28xx_vbi_supported(dev) == 1)
-		em28xx_capture_area_set(dev, 0, 2, width >> 2, height >> 2);
+		em28xx_capture_area_set(dev, 0, 2, width, height);
 	else
-		em28xx_capture_area_set(dev, 0, 0, width >> 2, height >> 2);
+		em28xx_capture_area_set(dev, 0, 0, width, height);

... here !
So it's not a real change.

Regards,
Frank

Quoted 3 lines
Quoted 3 lines
drivers/media/usb/em28xx/em28xx-core.c +0 -0
 	return em28xx_scaler_set(dev, dev->hscale, dev->vscale);
 }