* [PATCH] em28xx: overhaul em28xx_capture_area_set()
@ 2013-01-20 13:26 Frank Schäfer
2013-02-05 22:48 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 3+ messages in thread
From: Frank Schäfer @ 2013-01-20 13:26 UTC (permalink / raw)
To: mchehab; +Cc: linux-media, Frank Schäfer
- 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);
+ /* 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);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] em28xx: overhaul em28xx_capture_area_set()
2013-01-20 13:26 [PATCH] em28xx: overhaul em28xx_capture_area_set() Frank Schäfer
@ 2013-02-05 22:48 ` Mauro Carvalho Chehab
2013-02-06 15:36 ` Frank Schäfer
0 siblings, 1 reply; 3+ messages in thread
From: Mauro Carvalho Chehab @ 2013-02-05 22:48 UTC (permalink / raw)
To: Frank Schäfer; +Cc: linux-media, Devin Heitmueller
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);
> }
--
Cheers,
Mauro
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] em28xx: overhaul em28xx_capture_area_set()
2013-02-05 22:48 ` Mauro Carvalho Chehab
@ 2013-02-06 15:36 ` Frank Schäfer
0 siblings, 0 replies; 3+ messages in thread
From: Frank Schäfer @ 2013-02-06 15:36 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List, Devin Heitmueller
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);
>> }
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-02-06 15:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-20 13:26 [PATCH] em28xx: overhaul em28xx_capture_area_set() Frank Schäfer
2013-02-05 22:48 ` Mauro Carvalho Chehab
2013-02-06 15:36 ` Frank Schäfer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).