| Message ID | 1358688407-5146-1-git-send-email-fschaefer.oss@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Sun, 20 Jan 2013 14:26:47 +0100 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > - 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 <fschaefer.oss@googlemail.com> > --- > drivers/media/usb/em28xx/em28xx-core.c | 22 ++++++++++++---------- > 1 Datei geändert, 12 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-) > > diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c > index 210859a..f516a63 100644 > --- a/drivers/media/usb/em28xx/em28xx-core.c > +++ b/drivers/media/usb/em28xx/em28xx-core.c > @@ -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); 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. > + /* 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); > }
Am 05.02.2013 23:48, schrieb Mauro Carvalho Chehab: > Em Sun, 20 Jan 2013 14:26:47 +0100 > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > >> - 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 <fschaefer.oss@googlemail.com> >> --- >> drivers/media/usb/em28xx/em28xx-core.c | 22 ++++++++++++---------- >> 1 Datei geändert, 12 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-) >> >> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c >> index 210859a..f516a63 100644 >> --- a/drivers/media/usb/em28xx/em28xx-core.c >> +++ b/drivers/media/usb/em28xx/em28xx-core.c >> @@ -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); > Hmm.. why did you change the above overflow bits? Read the complete patch, the answer is... ... > 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. > >> + /* 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); ... here ! So it's not a real change. Regards, Frank >> >> return em28xx_scaler_set(dev, dev->hscale, dev->vscale); >> } -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c index 210859a..f516a63 100644 --- a/drivers/media/usb/em28xx/em28xx-core.c +++ b/drivers/media/usb/em28xx/em28xx-core.c @@ -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); }
- 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 <fschaefer.oss@googlemail.com> --- drivers/media/usb/em28xx/em28xx-core.c | 22 ++++++++++++---------- 1 Datei geändert, 12 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)