em28xx: add support for the em2765 bridge
Subsystems (2)
v2 cover em28xx: add support for the em2765 bridge
This patch series adds basic support for the em25xx/276x/7x/8x camera bridges. These devices differ from the em2710/2750 and em28xx bridges in several points:
1) a second i2c bus is provided which has to be accessed with a different read/write algorithm (=> patch 1)
2) a different frame data format is used (=> patch 3)
3) additional output formats (e.g. mpeg) are provided. This patch series does not (yet) add support for them, but it fixes the output format selection for these bridges (the current code sets bit 5 of the output format register, which has a different meaning for the other bridges and breaks capturing with em25xx family sdevices). (=> patch 4)
4) registers 0x34+0x35 (VBI_START_H/V for em28xx devices) are used for a different (unknown) purpose. This needs to be investigated further (could be zooming, cropping, image statistics or AWB/AE window selection). At normal operation, these registers are set to capturing (input) width/height / 16. (=> patch 5)
Patch 2 add the chip id of the em2765 as found in the "SpeedLink Vicious And Devine Laplace" webcam. The changes have also been tested with this device.
Changes since v1:
- rebased on the recent em28xx i2c bus changes (real support for 2 busses)
- moved i2c algorithm depending transfer function calls to separate functions
Frank Schäfer (5):
em28xx: add support for em25xx i2c bus B read/write/check device
operations
em28xx: add chip id of the em2765
em28xx: add support for em25xx/em276x/em277x/em278x frame data
processing
em28xx: make em28xx_set_outfmt() working with EM25xx family bridges
em28xx: write output frame resolution to regs 0x34+0x35 for em25xx
family bridges
drivers/media/usb/em28xx/em28xx-cards.c | 17 ++- drivers/media/usb/em28xx/em28xx-core.c | 27 +++- drivers/media/usb/em28xx/em28xx-i2c.c | 229 +++++++++++++++++++++++++------ drivers/media/usb/em28xx/em28xx-reg.h | 7 + drivers/media/usb/em28xx/em28xx-video.c | 72 +++++++++- drivers/media/usb/em28xx/em28xx.h | 11 +-
6 Dateien geändert, 315 Zeilen hinzugefügt(+), 48 Zeilen entfernt(-)
Signature · 1 line
1.7.10.4
PATCH v2 1/5 em28xx: add support for em25xx i2c bus B read/write/check device operations
The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm for i2c communication with the sensor, which is connected to a second i2c bus.
We don't know yet how to find out which devices support/use it. It's very likely used by all em25xx and em276x+ bridges. Tests with other em28xx chips (em2820, em2882/em2883) show, that this algorithm always succeeds there although no slave device is connected.
The algorithm likely also works for real i2c client devices (OV2640 uses SCCB), because the Windows driver seems to use it for probing Samsung and Kodak sensors.
--- drivers/media/usb/em28xx/em28xx-cards.c | 8 +- drivers/media/usb/em28xx/em28xx-i2c.c | 229 +++++++++++++++++++++++++------ drivers/media/usb/em28xx/em28xx.h | 10 +-
3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
drivers/media/usb/em28xx/em28xx-cards.c +6 -2
@@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,rt_mutex_init (&dev->i2c_bus_lock);/* register i2c bus 0 */ - retval =em28xx_i2c_register (dev,0 ); +if (dev->board.is_em2800) + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM2800 ); +else + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 0 - error [%d]!\n" , __func__, retval); goto unregister_dev; } +/* register i2c bus 1 */ if (dev->def_i2c_bus) { - retval =em28xx_i2c_register (dev,1 ); + retval =em28xx_i2c_register (dev,1 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 1 - error [%d]!\n" , __func__, retval);
drivers/media/usb/em28xx/em28xx-i2c.c +190 -39
286 lines not on this page · raw message
drivers/media/usb/em28xx/em28xx.h +9 -1
@@ -461,10 +461,17 @@ struct em28xx_fh {enum v4l2_buf_type type; }; +enum em28xx_i2c_algo_type { +EM28XX_I2C_ALGO_EM28XX =0 , +EM28XX_I2C_ALGO_EM2800 , +EM28XX_I2C_ALGO_EM25XX_BUS_B , +}; +struct em28xx_i2c_bus {struct em28xx *dev;unsigned bus; +enum em28xx_i2c_algo_type algo_type; };
@@ -651,7 +658,8 @@ struct em28xx_ops {/* Provided by em28xx-i2c.c */ void em28xx_do_i2c_scan (struct em28xx *dev,unsigned bus); -int em28xx_i2c_register (struct em28xx *dev,unsigned bus); +int em28xx_i2c_register (struct em28xx *dev,unsigned bus, +enum em28xx_i2c_algo_type algo_type);int em28xx_i2c_unregister (struct em28xx *dev,unsigned bus);/* Provided by em28xx-core.c */
Signature · 1 line
1.7.10.4
Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations
Em Sat, 23 Mar 2013 18:27:08 +0100
Frank Schäfer <[email protected]> escreveu:
Quoted 108 lines
The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm for i2c communication with the sensor, which is connected to a second i2c bus.
We don't know yet how to find out which devices support/use it. It's very likely used by all em25xx and em276x+ bridges. Tests with other em28xx chips (em2820, em2882/em2883) show, that this algorithm always succeeds there although no slave device is connected.
The algorithm likely also works for real i2c client devices (OV2640 uses SCCB), because the Windows driver seems to use it for probing Samsung and Kodak sensors.
Signed-off-by Frank Schäfer <[email protected]>--- drivers/media/usb/em28xx/em28xx-cards.c | 8 +- drivers/media/usb/em28xx/em28xx-i2c.c | 229 +++++++++++++++++++++++++------ drivers/media/usb/em28xx/em28xx.h | 10 +-3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
drivers/media/usb/em28xx/em28xx-cards.c +6
-2@@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,rt_mutex_init (&dev->i2c_bus_lock);/* register i2c bus 0 */ - retval =em28xx_i2c_register (dev,0 ); +if (dev->board.is_em2800) + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM2800 ); +else + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 0 - error [%d]!\n" , __func__, retval); goto unregister_dev; } +/* register i2c bus 1 */ if (dev->def_i2c_bus) { - retval =em28xx_i2c_register (dev,1 ); + retval =em28xx_i2c_register (dev,1 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 1 - error [%d]!\n" , __func__, retval);drivers/media/usb/em28xx/em28xx-i2c.c +48
-0@@ -5,6 +5,7 @@ Markus Rechberger <[email protected]> Mauro Carvalho Chehab <[email protected]> Sascha Sommer <[email protected]> + Copyright (C) 2013 Frank Schäfer <[email protected]> This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by@@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) }/* +* em25xx_bus_B_send_bytes +* write bytes to the i2c device +*/ +static int em25xx_bus_B_send_bytes (struct em28xx *dev,u16 addr,u8 *buf, +u16 len) +{ +int ret; + +if (len <1 || len >64 ) +return -EOPNOTSUPP ; +/* NOTE: limited by the USB ctrl message constraints +* Zero length reads always succeed, even if no device is connected */ + +/* Set register and write value */ + ret = dev->em28xx_write_regs_req (dev,0x06 , addr, buf, len); +/* NOTE: +* 0 byte writes always succeed, even if no device is connected. */ +if (ret != len) { +if (ret <0 ) { +em28xx_warn ("writing to i2c device at 0x%x failed " +"(error=%i)\n" , addr, ret); +return ret; + }else { +em28xx_warn ("%i bytes write to i2c device at 0x%x " +"requested, but %i bytes written\n" , + len, addr, ret); +return -EIO ; + } + } +/* Check success */ + ret = dev->em28xx_read_reg_req (dev,0x08 ,0x0000 ); +/* NOTE: the only error we've seen so far is +* 0x01 when the slave device is not present */ +if (ret ==0x00 ) { +return len; + }else if (ret >0 ) { +return -ENODEV ; + } + +return ret; +/* NOTE: With chips which do not support this operation, +* it seems to succeed ALWAYS ! (even if no device connected) */ +} + +/* +* em25xx_bus_B_recv_bytes +* read bytes from the i2c device
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 334
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c +10
-0+*/ +static int em25xx_bus_B_recv_bytes (struct em28xx *dev,u16 addr,u8 *buf, +u16 len) +{ +int ret; + +if (len <1 || len >64 ) +return -EOPNOTSUPP ; +/* NOTE: limited by the USB ctrl message constraints +* Zero length reads always succeed, even if no device is connected */
Please stick with Kernel's coding style, as described on Documentation/CodingStyle and on the common practices.
Multi-line comments are like:
/* * Foo * bar */
There are also a bunch of scripts/checkpatch.pl complains for this patch:
WARNING: please, no spaces at the start of a line #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>$
WARNING: space prohibited between function name and open parenthesis '(' #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>
WARNING: Avoid CamelCase: <Copyright>
#69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>
WARNING: Avoid CamelCase: <Frank>
#69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>
WARNING: Avoid CamelCase: <Sch>
#69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>
WARNING: quoted string split across lines #97: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:298:
+ em28xx_warn("writing to i2c device at 0x%x failed "
+ "(error=%i)\n", addr, ret);
WARNING: quoted string split across lines #101: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:302:
+ em28xx_warn("%i bytes write to i2c device at 0x%x "
+ "requested, but %i bytes written\n",
WARNING: braces {} are not necessary for any arm of this statement #110: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:311:
+ if (ret == 0x00) {
[...]
+ } else if (ret > 0) {
[...]
WARNING: braces {} are not necessary for any arm of this statement #156: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:357:
+ if (ret == 0x00) {
[...]
+ } else if (ret > 0) {
[...]
WARNING: braces {} are not necessary for any arm of this statement #190: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:391:
+ if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
[...]
+ } else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
[...]
+ } else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
[...]
WARNING: printk() should include KERN_ facility level #199: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:400:
+ printk(" no device\n");
WARNING: braces {} are not necessary for any arm of this statement #211: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:412:
+ if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
[...]
+ } else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
[...]
+ } else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
[...]
WARNING: printk() should include KERN_ facility level #220: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:421:
+ printk(" %02x", msg.buf[byte]);
WARNING: braces {} are not necessary for any arm of this statement #236: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:437:
+ if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
[...]
+ } else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
[...]
+ } else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
[...]
total: 0 errors, 14 warnings, 333 lines checked
Your patch has style problems, please review.
If any of these errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
PS.: I'll write a separate email if I find any non-coding style issue on this patch series. Won't comment anymore about coding style, as I'm assuming that you'll be fixing it on the other patches of this series if needed.
Regards,
Mauro
Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations
...
Am 24.03.2013 12:22, schrieb Mauro Carvalho Chehab:
Quoted 23 lines
Please stick with Kernel's coding style, as described on
Documentation/CodingStyleand on the common practices.Multi-line comments are like:
/* * Foo * bar */There are also a bunch of
scripts/checkpatch.plcomplains for this patch:WARNING: please, no spaces at the start of a line #69: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>$WARNING: space prohibited between function name and open parenthesis '(' #69: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>WARNING: Avoid CamelCase: <Copyright>
#69: FILE:drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>
Quoted 7 lines
WARNING: Avoid CamelCase: <Frank>
#69: FILE:drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>WARNING: Avoid CamelCase: <Sch>
#69: FILE:drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>
The "space prohibited between function name and open parenthesis" and "CamelCase" warnings are pure nonsense.
The "spaces at start of a line thing" applies to the whole and licences/copyright headers of the em28xx driver.
If you think we should change that - fine - but it really doesn't make sense to do this as part of this patch series.
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 302
Quoted 9 lines
WARNING: quoted string split across lines #97: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:298:drivers/media/usb/em28xx/em28xx-i2c.c +2
-0+em28xx_warn ("writing to i2c device at 0x%x failed " +"(error=%i)\n" , addr, ret);WARNING: quoted string split across lines #101: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:302:drivers/media/usb/em28xx/em28xx-i2c.c +2
-0+ em28xx_warn("%i bytes write to i2c device at 0x%x " + "requested, but %i bytes written\n",
Yes, these two are discussible.
AFAIK, strings should not be split across lines to avoid breaking grepping for strings.
In this case I decided for the "80 characters per line rule" instead, because grepping for strings containing placeholders IMHO doesn't make much sense.
We do exactly the same in the existing i2c functions.
Quoted 16 lines
WARNING: braces {} are not necessary for any arm of this statement #110: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:311:(quoted diff) +2
-0+ if (ret == 0x00) { + } else if (ret > 0) {WARNING: braces {} are not necessary for any arm of this statement #156: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:357:(quoted diff) +2
-0+ if (ret == 0x00) { + } else if (ret > 0) {WARNING: braces {} are not necessary for any arm of this statement #190: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:391:
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 395
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-i2c.c +3
-0+if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM28XX ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM2800 ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM25XX_BUS_B ) {
The patch perfectly matches the kernel coding style rules here !? What do you want me to change ?
Do you really think the code looks better without some of these braces ?
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 400
Quoted 3 lines
WARNING:
printk()should includeKERN_facility level #199: FILE:drivers/media/usb/em28xx/em28xx-i2c.c:400:drivers/media/usb/em28xx/em28xx-i2c.c +1
-0+printk (" no device\n" );
I only moved this piece of code around, but yes, that should really be fixed !
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 395
Quoted 8 lines
WARNING: braces {} are not necessary for any arm of this statement #211: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:412:drivers/media/usb/em28xx/em28xx-i2c.c +3
-0+if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM28XX ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM2800 ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM25XX_BUS_B ) {
See above.
Review
Quoted 3 lines
WARNING:
printk()should includeKERN_facility level #220: FILE:drivers/media/usb/em28xx/em28xx-i2c.c:421:(quoted diff) +1
-0+ printk(" %02x", msg.buf[byte]);
Same here, should indeed be fixed.
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 395
Quoted 8 lines
WARNING: braces {} are not necessary for any arm of this statement #236: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:437:drivers/media/usb/em28xx/em28xx-i2c.c +3
-0+if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM28XX ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM2800 ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM25XX_BUS_B ) {
See above.
Except for the two printk warnings, please tell me which changes you would like to see exactly.
I will send an updated version of this series then.
Regards,
Frank
Quoted 14 lines
total: 0 errors, 14 warnings, 333 lines checked
Your patch has style problems, please review.
If any of these errors are false positives, please report them to the maintainer, see CHECKPATCH in
MAINTAINERS.PS.: I'll write a separate email if I find any non-coding style issue on this patch series. Won't comment anymore about coding style, as I'm assuming that you'll be fixing it on the other patches of this series if needed.
Regards,
Mauro
Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations
Em Sun, 24 Mar 2013 14:04:47 +0100
Frank Schäfer <[email protected]> escreveu:
Quoted 34 lines
...
Am 24.03.2013 12:22, schrieb Mauro Carvalho Chehab:
Quoted 23 lines
Please stick with Kernel's coding style, as described on
Documentation/CodingStyleand on the common practices.Multi-line comments are like:
/* * Foo * bar */There are also a bunch of
scripts/checkpatch.plcomplains for this patch:WARNING: please, no spaces at the start of a line #69: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>$WARNING: space prohibited between function name and open parenthesis '(' #69: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>WARNING: Avoid CamelCase: <Copyright>
#69: FILE:drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>Quoted 7 lines
WARNING: Avoid CamelCase: <Frank>
#69: FILE:drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>WARNING: Avoid CamelCase: <Sch>
#69: FILE:drivers/media/usb/em28xx/em28xx-i2c.c:8:
+ Copyright (C) 2013 Frank Schäfer <[email protected]>
Quoted 6 lines
The "space prohibited between function name and open parenthesis" and "CamelCase" warnings are pure nonsense.
The "spaces at start of a line thing" applies to the whole and licences/copyright headers of the em28xx driver.
If you think we should change that - fine - but it really doesn't make sense to do this as part of this patch series.
The above complaints are because checkpatch doesn't like comments like:
/*
FOO
*/
As it should be, instead:
/*
* FOO
*/
You can safely ignore all the above. It is not worth to fix the comments there.
Quoted 7 lines
Quoted 7 lines
WARNING: quoted string split across lines #97: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:298:drivers/media/usb/em28xx/em28xx-i2c.c +2
-0+em28xx_warn ("writing to i2c device at 0x%x failed " +"(error=%i)\n" , addr, ret);
Quoted 12 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 302
Quoted 4 lines
WARNING: quoted string split across lines #101: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:302:drivers/media/usb/em28xx/em28xx-i2c.c +2
-0+ em28xx_warn("%i bytes write to i2c device at 0x%x " + "requested, but %i bytes written\n",Yes, these two are discussible.
AFAIK, strings should not be split across lines to avoid breaking grepping for strings.
In this case I decided for the "80 characters per line rule" instead, because grepping for strings containing placeholders IMHO doesn't make much sense.
We do exactly the same in the existing i2c functions.
The "not break strings" rule is stronger. When the code was written, 80 cols were a mandatory rule (and there was no checkpatch.pl). After lots of discussions (with took years), we're all set about two points:
- the 80 cols is a soft limit;
- strings should not be broken. Linus explicitly pointed on that
time that he doesn't want to see strings broken, as it makes harder to grep for the printed messages when needed.
Quoted 15 lines
Quoted 15 lines
WARNING: braces {} are not necessary for any arm of this statement #110: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:311:(quoted diff) +2
-0+ if (ret == 0x00) { + } else if (ret > 0) {WARNING: braces {} are not necessary for any arm of this statement #156: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:357:(quoted diff) +2
-0+ if (ret == 0x00) { + } else if (ret > 0) {
Quoted 12 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 395
Quoted 8 lines
WARNING: braces {} are not necessary for any arm of this statement #190: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:391:drivers/media/usb/em28xx/em28xx-i2c.c +3
-0+if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM28XX ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM2800 ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM25XX_BUS_B ) {The patch perfectly matches the kernel coding style rules here !? What do you want me to change ?
Do you really think the code looks better without some of these braces ?
For sure it looks better without the braces. Braces should be used only if there are multiple lines at the if chain.
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 400
Quoted 3 lines
WARNING:
printk()should includeKERN_facility level #199: FILE:drivers/media/usb/em28xx/em28xx-i2c.c:400:drivers/media/usb/em28xx/em28xx-i2c.c +1
-0+printk (" no device\n" );I only moved this piece of code around, but yes, that should really be fixed !
Where you're just moving the code, no.
Quoted 51 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 395
Quoted 9 lines
WARNING: braces {} are not necessary for any arm of this statement #211: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:412:drivers/media/usb/em28xx/em28xx-i2c.c +3
-0+if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM28XX ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM2800 ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM25XX_BUS_B ) {See above.
Review
Quoted 3 lines
WARNING:
printk()should includeKERN_facility level #220: FILE:drivers/media/usb/em28xx/em28xx-i2c.c:421:(quoted diff) +1
-0+ printk(" %02x", msg.buf[byte]);Same here, should indeed be fixed.
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 395
Quoted 8 lines
WARNING: braces {} are not necessary for any arm of this statement #236: FILE:
drivers/media/usb/em28xx/em28xx-i2c.c:437:drivers/media/usb/em28xx/em28xx-i2c.c +3
-0+if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM28XX ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM2800 ) { + }else if (i2c_bus->algo_type ==EM28XX_I2C_ALGO_EM25XX_BUS_B ) {See above.
Except for the two printk warnings, please tell me which changes you would like to see exactly.
I will send an updated version of this series then.Regards,
FrankQuoted 14 lines
total: 0 errors, 14 warnings, 333 lines checked
Your patch has style problems, please review.
If any of these errors are false positives, please report them to the maintainer, see CHECKPATCH in
MAINTAINERS.PS.: I'll write a separate email if I find any non-coding style issue on this patch series. Won't comment anymore about coding style, as I'm assuming that you'll be fixing it on the other patches of this series if needed.
Regards,
Mauro
Signature · 2 lines
Cheers, Mauro
Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations
In reply to Frank SchäferEm Sat, 23 Mar 2013 18:27:08 +0100
Frank Schäfer <[email protected]> escreveu:
Quoted 68 lines
The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm for i2c communication with the sensor, which is connected to a second i2c bus.
We don't know yet how to find out which devices support/use it. It's very likely used by all em25xx and em276x+ bridges. Tests with other em28xx chips (em2820, em2882/em2883) show, that this algorithm always succeeds there although no slave device is connected.
The algorithm likely also works for real i2c client devices (OV2640 uses SCCB), because the Windows driver seems to use it for probing Samsung and Kodak sensors.
Signed-off-by Frank Schäfer <[email protected]>--- drivers/media/usb/em28xx/em28xx-cards.c | 8 +- drivers/media/usb/em28xx/em28xx-i2c.c | 229 +++++++++++++++++++++++++------ drivers/media/usb/em28xx/em28xx.h | 10 +-3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
drivers/media/usb/em28xx/em28xx-cards.c +6
-2@@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,rt_mutex_init (&dev->i2c_bus_lock);/* register i2c bus 0 */ - retval =em28xx_i2c_register (dev,0 ); +if (dev->board.is_em2800) + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM2800 ); +else + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 0 - error [%d]!\n" , __func__, retval); goto unregister_dev; } +/* register i2c bus 1 */ if (dev->def_i2c_bus) { - retval =em28xx_i2c_register (dev,1 ); + retval =em28xx_i2c_register (dev,1 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 1 - error [%d]!\n" , __func__, retval);drivers/media/usb/em28xx/em28xx-i2c.c +8
-0@@ -5,6 +5,7 @@ Markus Rechberger <[email protected]> Mauro Carvalho Chehab <[email protected]> Sascha Sommer <[email protected]> + Copyright (C) 2013 Frank Schäfer <[email protected]> This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by@@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) }/* +* em25xx_bus_B_send_bytes +* write bytes to the i2c device +*/ +static int em25xx_bus_B_send_bytes (struct em28xx *dev,u16 addr,u8 *buf, +u16 len) +{ +int ret;
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 294
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c +10
-0+ +if (len <1 || len >64 ) +return -EOPNOTSUPP ; +/* NOTE: limited by the USB ctrl message constraints +* Zero length reads always succeed, even if no device is connected */ + +/* Set register and write value */ + ret = dev->em28xx_write_regs_req (dev,0x06 , addr, buf, len); +/* NOTE: +* 0 byte writes always succeed, even if no device is connected. */
You already noticed it on the previous note.
Quoted 7 lines
drivers/media/usb/em28xx/em28xx-i2c.c +7
-0+if (ret != len) { +if (ret <0 ) { +em28xx_warn ("writing to i2c device at 0x%x failed " +"(error=%i)\n" , addr, ret); +return ret; + }else { + em28xx_warn("%i bytes write to i2c device at 0x%x "
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 311
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c +10
-0+"requested, but %i bytes written\n" , + len, addr, ret); +return -EIO ; + } + } +/* Check success */ + ret = dev->em28xx_read_reg_req (dev,0x08 ,0x0000 ); +/* NOTE: the only error we've seen so far is +* 0x01 when the slave device is not present */ +if (ret ==0x00 ) {
Please simplify. just use: if (!ret)
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 319
Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c +8
-0+return len; + }else if (ret >0 ) { +return -ENODEV ; + } + +return ret; +/* NOTE: With chips which do not support this operation, +* it seems to succeed ALWAYS ! (even if no device connected) */
Sorry, but I didn't get what you're trying to explain here. What are those em25xx chips that don't support this operation?
Quoted 5 lines
drivers/media/usb/em28xx/em28xx-i2c.c +5
-0+} + +/* +* em25xx_bus_B_recv_bytes +* read bytes from the i2c device
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 334
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c +10
-0+*/ +static int em25xx_bus_B_recv_bytes (struct em28xx *dev,u16 addr,u8 *buf, +u16 len) +{ +int ret; + +if (len <1 || len >64 ) +return -EOPNOTSUPP ; +/* NOTE: limited by the USB ctrl message constraints +* Zero length reads always succeed, even if no device is connected */
You already explained about the zero length before.
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 339
Quoted 5 lines
drivers/media/usb/em28xx/em28xx-i2c.c +5
-0+ +/* Read value */ + ret = dev->em28xx_read_reg_req_len (dev,0x06 , addr, buf, len); +/* NOTE: +* 0 byte reads always succeed, even if no device is connected. */
You're insistent on that, won't you? ;)
Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c +8
-0+if (ret <0 ) { +em28xx_warn ("reading from i2c device at 0x%x failed (error=%i)\n" , + addr, ret); +return ret; + } +/* NOTE: some devices with two i2c busses have the bad habit to return 0 +* bytes if we are on bus B AND there was no write attempt to the +* specified slave address before AND no device is present at the
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 357
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c +10
-0+* requested slave address. +* Anyway, the next check will fail with -ENODEV in this case, so avoid +* spamming the system log on device probing and do nothing here. +*/ + +/* Check success */ + ret = dev->em28xx_read_reg_req (dev,0x08 ,0x0000 ); +/* NOTE: the only error we've seen so far is +* 0x01 when the slave device is not present */ +if (ret ==0x00 ) {
Same as already noticed.
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 319
Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c +8
-0+return len; + }else if (ret >0 ) { +return -ENODEV ; + } + +return ret; +/* NOTE: With chips which do not support this operation, +* it seems to succeed ALWAYS ! (even if no device connected) */
Again, didn't understand what you're meaning.
Regards,
Mauro
Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations
Am 24.03.2013 12:38, schrieb Mauro Carvalho Chehab:
Quoted 74 lines
Em Sat, 23 Mar 2013 18:27:08 +0100
Frank Schäfer <[email protected]> escreveu:Quoted 71 lines
The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm for i2c communication with the sensor, which is connected to a second i2c bus.
We don't know yet how to find out which devices support/use it. It's very likely used by all em25xx and em276x+ bridges. Tests with other em28xx chips (em2820, em2882/em2883) show, that this algorithm always succeeds there although no slave device is connected.
The algorithm likely also works for real i2c client devices (OV2640 uses SCCB), because the Windows driver seems to use it for probing Samsung and Kodak sensors.
Signed-off-by Frank Schäfer <[email protected]>--- drivers/media/usb/em28xx/em28xx-cards.c | 8 +- drivers/media/usb/em28xx/em28xx-i2c.c | 229 +++++++++++++++++++++++++------ drivers/media/usb/em28xx/em28xx.h | 10 +-3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
drivers/media/usb/em28xx/em28xx-cards.c +6
-2@@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,rt_mutex_init (&dev->i2c_bus_lock);/* register i2c bus 0 */ - retval =em28xx_i2c_register (dev,0 ); +if (dev->board.is_em2800) + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM2800 ); +else + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 0 - error [%d]!\n" , __func__, retval); goto unregister_dev; } +/* register i2c bus 1 */ if (dev->def_i2c_bus) { - retval =em28xx_i2c_register (dev,1 ); + retval =em28xx_i2c_register (dev,1 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 1 - error [%d]!\n" , __func__, retval);drivers/media/usb/em28xx/em28xx-i2c.c +11
-0@@ -5,6 +5,7 @@ Markus Rechberger <[email protected]> Mauro Carvalho Chehab <[email protected]> Sascha Sommer <[email protected]> + Copyright (C) 2013 Frank Schäfer <[email protected]> This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by@@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) }/* +* em25xx_bus_B_send_bytes +* write bytes to the i2c device +*/ +static int em25xx_bus_B_send_bytes (struct em28xx *dev,u16 addr,u8 *buf, +u16 len) +{ +int ret; + +if (len <1 || len >64 ) +return -EOPNOTSUPP ;
Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 294
Quoted 7 lines
drivers/media/usb/em28xx/em28xx-i2c.c +7
-0+/* NOTE: limited by the USB ctrl message constraints +* Zero length reads always succeed, even if no device is connected */ + +/* Set register and write value */ + ret = dev->em28xx_write_regs_req (dev,0x06 , addr, buf, len); +/* NOTE: +* 0 byte writes always succeed, even if no device is connected. */ You already noticed it on the previous note.
Yes. ;)
Quoted 12 lines
Quoted 12 lines
drivers/media/usb/em28xx/em28xx-i2c.c +11
-0+if (ret != len) { +if (ret <0 ) { +em28xx_warn ("writing to i2c device at 0x%x failed " +"(error=%i)\n" , addr, ret); +return ret; + }else { +em28xx_warn ("%i bytes write to i2c device at 0x%x " +"requested, but %i bytes written\n" , + len, addr, ret); +return -EIO ; + }
Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 311
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-i2c.c +6
-0+ } +/* Check success */ + ret = dev->em28xx_read_reg_req (dev,0x08 ,0x0000 ); +/* NOTE: the only error we've seen so far is +* 0x01 when the slave device is not present */ +if (ret ==0x00 ) {Please simplify. just use: if (!ret)
I would like to keep it as is because I think it better expresses the purposes of this check. I also used 0x00 instead of 0 on purpose. ret is a mixed value which is negative on errors and contains the data bytes (0x00 to 0xff) on success.
Ok, in this specific case all other values are covered with a single (ret > 0) check, but take a look at the comment and the em28xx-algo functions where we check for 0x10, too.
Quoted 11 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 319
Quoted 9 lines
drivers/media/usb/em28xx/em28xx-i2c.c +8
-0+return len; + }else if (ret >0 ) { +return -ENODEV ; + } + +return ret; +/* NOTE: With chips which do not support this operation, +* it seems to succeed ALWAYS ! (even if no device connected) */ Sorry, but I didn't get what you're trying to explain here. What are those em25xx chips that don't support this operation?
Hmm... I don't know how to explain it better...
The thing is, that this algo _seems_ to work also (at least with some) chips which actually don't support it (even if they don't provide a second i2c bus).
Quoted 17 lines
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-i2c.c +5
-0+} + +/* +* em25xx_bus_B_recv_bytes +* read bytes from the i2c device drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 334
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c +10
-0+*/ +static int em25xx_bus_B_recv_bytes (struct em28xx *dev,u16 addr,u8 *buf, +u16 len) +{ +int ret; + +if (len <1 || len >64 ) +return -EOPNOTSUPP ; +/* NOTE: limited by the USB ctrl message constraints +* Zero length reads always succeed, even if no device is connected */ You already explained about the zero length before.
Yepp, but in another function. ;)
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 339
Quoted 5 lines
drivers/media/usb/em28xx/em28xx-i2c.c +5
-0+ +/* Read value */ + ret = dev->em28xx_read_reg_req_len (dev,0x06 , addr, buf, len); +/* NOTE: +* 0 byte reads always succeed, even if no device is connected. */ You're insistent on that, won't you? ;)
Yeah, I like comments and I think there should be much more ! :)
But in this case I don't want to be insistent, I just want to make sure that important issues are expalined at all relevant code places. The alternative would be to reference comments at other places, which IMHO doesn't work well.
Regards,
Frank
Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations
Em Sun, 24 Mar 2013 13:53:40 +0100
Frank Schäfer <[email protected]> escreveu:
Quoted 84 lines
Am 24.03.2013 12:38, schrieb Mauro Carvalho Chehab:
Quoted 74 lines
Em Sat, 23 Mar 2013 18:27:08 +0100
Frank Schäfer <[email protected]> escreveu:Quoted 71 lines
The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm for i2c communication with the sensor, which is connected to a second i2c bus.
We don't know yet how to find out which devices support/use it. It's very likely used by all em25xx and em276x+ bridges. Tests with other em28xx chips (em2820, em2882/em2883) show, that this algorithm always succeeds there although no slave device is connected.
The algorithm likely also works for real i2c client devices (OV2640 uses SCCB), because the Windows driver seems to use it for probing Samsung and Kodak sensors.
Signed-off-by Frank Schäfer <[email protected]>--- drivers/media/usb/em28xx/em28xx-cards.c | 8 +- drivers/media/usb/em28xx/em28xx-i2c.c | 229 +++++++++++++++++++++++++------ drivers/media/usb/em28xx/em28xx.h | 10 +-3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
drivers/media/usb/em28xx/em28xx-cards.c +6
-2@@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,rt_mutex_init (&dev->i2c_bus_lock);/* register i2c bus 0 */ - retval =em28xx_i2c_register (dev,0 ); +if (dev->board.is_em2800) + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM2800 ); +else + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 0 - error [%d]!\n" , __func__, retval); goto unregister_dev; } +/* register i2c bus 1 */ if (dev->def_i2c_bus) { - retval =em28xx_i2c_register (dev,1 ); + retval =em28xx_i2c_register (dev,1 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 1 - error [%d]!\n" , __func__, retval);drivers/media/usb/em28xx/em28xx-i2c.c +11
-0@@ -5,6 +5,7 @@ Markus Rechberger <[email protected]> Mauro Carvalho Chehab <[email protected]> Sascha Sommer <[email protected]> + Copyright (C) 2013 Frank Schäfer <[email protected]> This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by@@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) }/* +* em25xx_bus_B_send_bytes +* write bytes to the i2c device +*/ +static int em25xx_bus_B_send_bytes (struct em28xx *dev,u16 addr,u8 *buf, +u16 len) +{ +int ret; + +if (len <1 || len >64 ) +return -EOPNOTSUPP ;Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 294
Quoted 7 lines
drivers/media/usb/em28xx/em28xx-i2c.c +7
-0+/* NOTE: limited by the USB ctrl message constraints +* Zero length reads always succeed, even if no device is connected */ + +/* Set register and write value */ + ret = dev->em28xx_write_regs_req (dev,0x06 , addr, buf, len); +/* NOTE: +* 0 byte writes always succeed, even if no device is connected. */ You already noticed it on the previous note.
Quoted 1 line
Yes. ;)
Well, there's no need to repeat the same thing twice at the same function ;)
Quoted 21 lines
Quoted 12 lines
Quoted 12 lines
drivers/media/usb/em28xx/em28xx-i2c.c +11
-0+if (ret != len) { +if (ret <0 ) { +em28xx_warn ("writing to i2c device at 0x%x failed " +"(error=%i)\n" , addr, ret); +return ret; + }else { +em28xx_warn ("%i bytes write to i2c device at 0x%x " +"requested, but %i bytes written\n" , + len, addr, ret); +return -EIO ; + }Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 311
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-i2c.c +6
-0+ } +/* Check success */ + ret = dev->em28xx_read_reg_req (dev,0x08 ,0x0000 ); +/* NOTE: the only error we've seen so far is +* 0x01 when the slave device is not present */ +if (ret ==0x00 ) {Please simplify. just use: if (!ret)
Quoted 2 lines
I would like to keep it as is because I think it better expresses the purposes of this check. I also used
0x00instead of 0 on purpose.
Why do you think it better expresses it? It is just a more verbose way of doing the same thing.
If you want to better express, then add a comment:
/* * Reg 08 value 0 means that the operation succeeded. * different values indicate that the I2C device was not found. */ if (!ret) return len;
Quoted 18 lines
ret is a mixed value which is negative on errors and contains the data bytes (
0x00to0xff) on success.
Ok, in this specific case all other values are covered with a single (ret > 0) check, but take a look at the comment and the em28xx-algo functions where we check for0x10, too.Quoted 11 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 319
Quoted 9 lines
drivers/media/usb/em28xx/em28xx-i2c.c +8
-0+return len; + }else if (ret >0 ) { +return -ENODEV ; + } + +return ret; +/* NOTE: With chips which do not support this operation, +* it seems to succeed ALWAYS ! (even if no device connected) */ Sorry, but I didn't get what you're trying to explain here. What are those em25xx chips that don't support this operation?
Quoted 4 lines
Hmm... I don't know how to explain it better...
The thing is, that this algo _seems_ to work also (at least with some) chips which actually don't support it (even if they don't provide a second i2c bus).
Again, what do you mean by "chips which actually don't support it"?
Are you talking about some versions of chips with this ID?
+ CHIP_ID_EM2765 = 54,
Or about something else? How those can be distinguished from the others that don't support it? Or they can't be distinguished?
Quoted 27 lines
Quoted 17 lines
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-i2c.c +5
-0+} + +/* +* em25xx_bus_B_recv_bytes +* read bytes from the i2c device drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 334
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c +10
-0+*/ +static int em25xx_bus_B_recv_bytes (struct em28xx *dev,u16 addr,u8 *buf, +u16 len) +{ +int ret; + +if (len <1 || len >64 ) +return -EOPNOTSUPP ; +/* NOTE: limited by the USB ctrl message constraints +* Zero length reads always succeed, even if no device is connected */ You already explained about the zero length before.
Yepp, but in another function. ;)
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 339
Quoted 5 lines
drivers/media/usb/em28xx/em28xx-i2c.c +5
-0+ +/* Read value */ + ret = dev->em28xx_read_reg_req_len (dev,0x06 , addr, buf, len); +/* NOTE: +* 0 byte reads always succeed, even if no device is connected. */ You're insistent on that, won't you? ;)
Quoted 6 lines
Yeah, I like comments and I think there should be much more ! :)
But in this case I don't want to be insistent, I just want to make sure that important issues are expalined at all relevant code places. The alternative would be to reference comments at other places, which IMHO doesn't work well.
Well, you don't need to explain the same thing twice at the same function ;)
Quoted 4 lines
Regards,
Frank
Signature · 2 lines
Cheers, Mauro
Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations
Am 24.03.2013 15:02, schrieb Mauro Carvalho Chehab:
Quoted 86 lines
Em Sun, 24 Mar 2013 13:53:40 +0100
Frank Schäfer <[email protected]> escreveu:Quoted 83 lines
Am 24.03.2013 12:38, schrieb Mauro Carvalho Chehab:
Quoted 82 lines
Em Sat, 23 Mar 2013 18:27:08 +0100
Frank Schäfer <[email protected]> escreveu:Quoted 68 lines
The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm for i2c communication with the sensor, which is connected to a second i2c bus.
We don't know yet how to find out which devices support/use it. It's very likely used by all em25xx and em276x+ bridges. Tests with other em28xx chips (em2820, em2882/em2883) show, that this algorithm always succeeds there although no slave device is connected.
The algorithm likely also works for real i2c client devices (OV2640 uses SCCB), because the Windows driver seems to use it for probing Samsung and Kodak sensors.
Signed-off-by Frank Schäfer <[email protected]>--- drivers/media/usb/em28xx/em28xx-cards.c | 8 +- drivers/media/usb/em28xx/em28xx-i2c.c | 229 +++++++++++++++++++++++++------ drivers/media/usb/em28xx/em28xx.h | 10 +-3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
drivers/media/usb/em28xx/em28xx-cards.c +6
-2@@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,rt_mutex_init (&dev->i2c_bus_lock);/* register i2c bus 0 */ - retval =em28xx_i2c_register (dev,0 ); +if (dev->board.is_em2800) + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM2800 ); +else + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 0 - error [%d]!\n" , __func__, retval); goto unregister_dev; } +/* register i2c bus 1 */ if (dev->def_i2c_bus) { - retval =em28xx_i2c_register (dev,1 ); + retval =em28xx_i2c_register (dev,1 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 1 - error [%d]!\n" , __func__, retval);drivers/media/usb/em28xx/em28xx-i2c.c +8
-0@@ -5,6 +5,7 @@ Markus Rechberger <[email protected]> Mauro Carvalho Chehab <[email protected]> Sascha Sommer <[email protected]> + Copyright (C) 2013 Frank Schäfer <[email protected]> This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by@@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) }/* +* em25xx_bus_B_send_bytes +* write bytes to the i2c device +*/ +static int em25xx_bus_B_send_bytes (struct em28xx *dev,u16 addr,u8 *buf, +u16 len) +{ +int ret;drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 294
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c +10
-0+ +if (len <1 || len >64 ) +return -EOPNOTSUPP ; +/* NOTE: limited by the USB ctrl message constraints +* Zero length reads always succeed, even if no device is connected */ + +/* Set register and write value */ + ret = dev->em28xx_write_regs_req (dev,0x06 , addr, buf, len); +/* NOTE: +* 0 byte writes always succeed, even if no device is connected. */ You already noticed it on the previous note.
Quoted 2 lines
Quoted 1 line
Yes. ;)
Well, there's no need to repeat the same thing twice at the same function ;)
Uhm yes... WTF... !??
This stuff has definitely been rebased too often... ;)
Quoted 24 lines
Quoted 19 lines
Quoted 11 lines
Quoted 11 lines
drivers/media/usb/em28xx/em28xx-i2c.c +11
-0+if (ret != len) { +if (ret <0 ) { +em28xx_warn ("writing to i2c device at 0x%x failed " +"(error=%i)\n" , addr, ret); +return ret; + }else { +em28xx_warn ("%i bytes write to i2c device at 0x%x " +"requested, but %i bytes written\n" , + len, addr, ret); +return -EIO ; + }Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 311
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-i2c.c +6
-0+ } +/* Check success */ + ret = dev->em28xx_read_reg_req (dev,0x08 ,0x0000 ); +/* NOTE: the only error we've seen so far is +* 0x01 when the slave device is not present */ +if (ret ==0x00 ) {Please simplify. just use: if (!ret)Quoted 2 lines
I would like to keep it as is because I think it better expresses the purposes of this check. I also used
0x00instead of 0 on purpose.Why do you think it better expresses it? It is just a more verbose way of doing the same thing.
Quoted 7 lines
If you want to better express, then add a comment:
/* * Reg 08 value 0 means that the operation succeeded. * different values indicate that the I2C device was not found. */ if (!ret) return len;
:)
I don't care too much. If you prefer it that way, no problem.
Quoted 23 lines
Quoted 16 lines
ret is a mixed value which is negative on errors and contains the data bytes (
0x00to0xff) on success.
Ok, in this specific case all other values are covered with a single (ret > 0) check, but take a look at the comment and the em28xx-algo functions where we check for0x10, too.Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 319
Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c +8
-0+return len; + }else if (ret >0 ) { +return -ENODEV ; + } + +return ret; +/* NOTE: With chips which do not support this operation, +* it seems to succeed ALWAYS ! (even if no device connected) */ Sorry, but I didn't get what you're trying to explain here. What are those em25xx chips that don't support this operation?
Quoted 4 lines
Hmm... I don't know how to explain it better...
The thing is, that this algo _seems_ to work also (at least with some) chips which actually don't support it (even if they don't provide a second i2c bus).Again, what do you mean by "chips which actually don't support it"?
Are you talking about some versions of chips with this ID?
Review
Quoted 1 line
(quoted diff) +1
-0+ CHIP_ID_EM2765 = 54,
We don't know any other way to distinguish between chips than the chip ID, right ? ;)
So the same chip ID means the same "chip" or "chip type" for us and different chip IDs mean different "chips" or "chip types". And even if there would be several sub-revisions, it's not likely that they would differ in such a significant feature.
I think the comment should be clear enough, but I could change it to
"chips with different chip ids which actually don't support it"
Would that make it clear enough for you ? Or do you have a better suggestion ?
Quoted 2 lines
Or about something else? How those can be distinguished from the others that don't support it? Or they can't be distinguished?
That's exactly the reason for this comment. ;)
I don't know, do you ?
Regards,
Frank
Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations
Em Sun, 24 Mar 2013 22:14:58 +0100
Frank Schäfer <[email protected]> escreveu:
Quoted 87 lines
Am 24.03.2013 15:02, schrieb Mauro Carvalho Chehab:
Quoted 86 lines
Em Sun, 24 Mar 2013 13:53:40 +0100
Frank Schäfer <[email protected]> escreveu:Quoted 83 lines
Am 24.03.2013 12:38, schrieb Mauro Carvalho Chehab:
Quoted 82 lines
Em Sat, 23 Mar 2013 18:27:08 +0100
Frank Schäfer <[email protected]> escreveu:Quoted 68 lines
The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm for i2c communication with the sensor, which is connected to a second i2c bus.
We don't know yet how to find out which devices support/use it. It's very likely used by all em25xx and em276x+ bridges. Tests with other em28xx chips (em2820, em2882/em2883) show, that this algorithm always succeeds there although no slave device is connected.
The algorithm likely also works for real i2c client devices (OV2640 uses SCCB), because the Windows driver seems to use it for probing Samsung and Kodak sensors.
Signed-off-by Frank Schäfer <[email protected]>--- drivers/media/usb/em28xx/em28xx-cards.c | 8 +- drivers/media/usb/em28xx/em28xx-i2c.c | 229 +++++++++++++++++++++++++------ drivers/media/usb/em28xx/em28xx.h | 10 +-3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
drivers/media/usb/em28xx/em28xx-cards.c +6
-2@@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,rt_mutex_init (&dev->i2c_bus_lock);/* register i2c bus 0 */ - retval =em28xx_i2c_register (dev,0 ); +if (dev->board.is_em2800) + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM2800 ); +else + retval =em28xx_i2c_register (dev,0 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 0 - error [%d]!\n" , __func__, retval); goto unregister_dev; } +/* register i2c bus 1 */ if (dev->def_i2c_bus) { - retval =em28xx_i2c_register (dev,1 ); + retval =em28xx_i2c_register (dev,1 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 1 - error [%d]!\n" , __func__, retval);drivers/media/usb/em28xx/em28xx-i2c.c +8
-0@@ -5,6 +5,7 @@ Markus Rechberger <[email protected]> Mauro Carvalho Chehab <[email protected]> Sascha Sommer <[email protected]> + Copyright (C) 2013 Frank Schäfer <[email protected]> This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by@@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) }/* +* em25xx_bus_B_send_bytes +* write bytes to the i2c device +*/ +static int em25xx_bus_B_send_bytes (struct em28xx *dev,u16 addr,u8 *buf, +u16 len) +{ +int ret;drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 294
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c +10
-0+ +if (len <1 || len >64 ) +return -EOPNOTSUPP ; +/* NOTE: limited by the USB ctrl message constraints +* Zero length reads always succeed, even if no device is connected */ + +/* Set register and write value */ + ret = dev->em28xx_write_regs_req (dev,0x06 , addr, buf, len); +/* NOTE: +* 0 byte writes always succeed, even if no device is connected. */ You already noticed it on the previous note.
Quoted 5 lines
Quoted 2 lines
Quoted 1 line
Yes. ;)
Well, there's no need to repeat the same thing twice at the same function ;)
Uhm yes... WTF... !??
This stuff has definitely been rebased too often... ;)
:)
Quoted 34 lines
Quoted 26 lines
Quoted 21 lines
Quoted 13 lines
Quoted 13 lines
drivers/media/usb/em28xx/em28xx-i2c.c +11
-0+if (ret != len) { +if (ret <0 ) { +em28xx_warn ("writing to i2c device at 0x%x failed " +"(error=%i)\n" , addr, ret); +return ret; + }else { +em28xx_warn ("%i bytes write to i2c device at 0x%x " +"requested, but %i bytes written\n" , + len, addr, ret); +return -EIO ; + }Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 311
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-i2c.c +6
-0+ } +/* Check success */ + ret = dev->em28xx_read_reg_req (dev,0x08 ,0x0000 ); +/* NOTE: the only error we've seen so far is +* 0x01 when the slave device is not present */ +if (ret ==0x00 ) {Please simplify. just use: if (!ret)Quoted 2 lines
I would like to keep it as is because I think it better expresses the purposes of this check. I also used
0x00instead of 0 on purpose.Why do you think it better expresses it? It is just a more verbose way of doing the same thing.
Quoted 7 lines
If you want to better express, then add a comment:
/* * Reg 08 value 0 means that the operation succeeded. * different values indicate that the I2C device was not found. */ if (!ret) return len;
Quoted 3 lines
:)
I don't care too much. If you prefer it that way, no problem.
Ok, thanks!
Quoted 33 lines
Quoted 24 lines
Quoted 17 lines
ret is a mixed value which is negative on errors and contains the data bytes (
0x00to0xff) on success.
Ok, in this specific case all other values are covered with a single (ret > 0) check, but take a look at the comment and the em28xx-algo functions where we check for0x10, too.Quoted 10 lines
drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) · line 319
Quoted 8 lines
drivers/media/usb/em28xx/em28xx-i2c.c +8
-0+return len; + }else if (ret >0 ) { +return -ENODEV ; + } + +return ret; +/* NOTE: With chips which do not support this operation, +* it seems to succeed ALWAYS ! (even if no device connected) */ Sorry, but I didn't get what you're trying to explain here. What are those em25xx chips that don't support this operation?
Quoted 4 lines
Hmm... I don't know how to explain it better...
The thing is, that this algo _seems_ to work also (at least with some) chips which actually don't support it (even if they don't provide a second i2c bus).Again, what do you mean by "chips which actually don't support it"?
Are you talking about some versions of chips with this ID?
Review
Quoted 1 line
(quoted diff) +1
-0+ CHIP_ID_EM2765 = 54,We don't know any other way to distinguish between chips than the chip ID, right ? ;)
So the same chip ID means the same "chip" or "chip type" for us and different chip IDs mean different "chips" or "chip types". And even if there would be several sub-revisions, it's not likely that they would differ in such a significant feature.
Quoted 2 lines
I think the comment should be clear enough, but I could change it to
"chips with different chip ids which actually don't support it"
Ah, now it is clear to me!
Quoted 18 lines
Would that make it clear enough for you ? Or do you have a better suggestion ?
Quoted 2 lines
Or about something else? How those can be distinguished from the others that don't support it? Or they can't be distinguished?
That's exactly the reason for this comment. ;)
I don't know, do you ?Regards,
Frank
Signature · 2 lines
Cheers, Mauro
PATCH v2 2/5 em28xx: add chip id of the em2765
In reply to Frank SchäferThis chip can be found in the SpeedLink VAD Laplace webcam (1ae7:9003 and 1ae7:9004).
--- drivers/media/usb/em28xx/em28xx-cards.c | 11 ++++++++++- drivers/media/usb/em28xx/em28xx-reg.h | 1 + drivers/media/usb/em28xx/em28xx.h | 1 +
3 Dateien geändert, 12 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
drivers/media/usb/em28xx/em28xx-cards.c +10 -1
@@ -3041,6 +3041,12 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,case CHIP_ID_EM2750 : chip_name ="em2750" ;break ; +case CHIP_ID_EM2765 : + chip_name ="em2765" ; + dev->wait_after_write =0 ; + dev->is_em25xx =1 ; + dev->eeprom_addrwidth_16bit =1 ; +break ;case CHIP_ID_EM2820 : chip_name ="em2710/2820" ;break ;
@@ -3151,7 +3157,10 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,/* register i2c bus 1 */ if (dev->def_i2c_bus) { - retval =em28xx_i2c_register (dev,1 ,EM28XX_I2C_ALGO_EM28XX ); +if (dev->is_em25xx) + retval =em28xx_i2c_register (dev,1 ,EM28XX_I2C_ALGO_EM25XX_BUS_B ); +else + retval =em28xx_i2c_register (dev,1 ,EM28XX_I2C_ALGO_EM28XX );if (retval <0 ) {em28xx_errdev ("%s: em28xx_i2c_register bus 1 - error [%d]!\n" , __func__, retval);
drivers/media/usb/em28xx/em28xx-reg.h +1 -0
@@ -219,6 +219,7 @@ enum em28xx_chip_id {CHIP_ID_EM2860 =34 ,CHIP_ID_EM2870 =35 ,CHIP_ID_EM2883 =36 , +CHIP_ID_EM2765 =54 ,CHIP_ID_EM2874 =65 ,CHIP_ID_EM2884 =68 , CHIP_ID_EM28174 = 113,
drivers/media/usb/em28xx/em28xx.h +1 -0
@@ -482,6 +482,7 @@ struct em28xx {int model;/* index in the device_data struct */ int devno;/* marks the number of this device */ enum em28xx_chip_id chip_id; + unsigned int is_em25xx:1; /* em25xx/em276x/7x/8x family bridge */ unsigned char disconnected:1; /* device has been diconnected */
Signature · 1 line
1.7.10.4
PATCH v2 3/5 em28xx: add support for em25xx/em276x/em277x/em278x frame data processing
In reply to Frank SchäferThe em25xx/em276x/em277x/em278x frame data format is different to the one used by the em2710/em2750/em28xx chips.
With the recent cleanups and reorganization of the frame data processing code it can be easily extended to support these devices.
--- drivers/media/usb/em28xx/em28xx-video.c | 72 ++++++++++++++++++++++++++++++-
1 Datei geändert, 71 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
drivers/media/usb/em28xx/em28xx-video.c +71 -1
@@ -76,6 +76,16 @@ MODULE_DESCRIPTION(DRIVER_DESC);MODULE_LICENSE ("GPL" );MODULE_VERSION (EM28XX_VERSION ); + +#define EM25XX_FRMDATAHDR_BYTE1 0x02 +#define EM25XX_FRMDATAHDR_BYTE2_STILL_IMAGE 0x20 +#define EM25XX_FRMDATAHDR_BYTE2_FRAME_END 0x02 +#define EM25XX_FRMDATAHDR_BYTE2_FRAME_ID 0x01 +#define EM25XX_FRMDATAHDR_BYTE2_MASK (EM25XX_FRMDATAHDR_BYTE2_STILL_IMAGE | \ + EM25XX_FRMDATAHDR_BYTE2_FRAME_END | \ + EM25XX_FRMDATAHDR_BYTE2_FRAME_ID) + +static unsigned int video_nr[] = {[0 ... (EM28XX_MAXBOARDS -1 )] =-1U };static unsigned int vbi_nr[] = {[0 ... (EM28XX_MAXBOARDS -1 )] =-1U };static unsigned int radio_nr[] = {[0 ... (EM28XX_MAXBOARDS -1 )] =-1U };
@@ -408,6 +418,62 @@ static inline void process_frame_data_em28xx(struct em28xx *dev,em28xx_copy_video (dev, buf, data_pkt, data_len); } +/* +* Process data packet according to the em25xx/em276x/7x/8x frame data format +*/ +static inline void process_frame_data_em25xx (struct em28xx *dev, +unsigned char *data_pkt, +unsigned int data_len) +{ +struct em28xx_buffer *buf = dev->usb_ctl.vid_buf; +struct em28xx_dmaqueue *dmaq = &dev->vidq; +bool frame_end =0 ; + +/* Check for header */ +/* NOTE: at least with bulk transfers, only the first packet +* has a header and has always set the FRAME_END bit */ +if (data_len >=2 ) {/* em25xx header is only 2 bytes long */ +if ((data_pkt[0 ] ==EM25XX_FRMDATAHDR_BYTE1 ) && + ((data_pkt[1 ] & ~EM25XX_FRMDATAHDR_BYTE2_MASK ) ==0x00 )) { + dev->top_field = !(data_pkt[1 ] & +EM25XX_FRMDATAHDR_BYTE2_FRAME_ID ); + frame_end = data_pkt[1 ] & +EM25XX_FRMDATAHDR_BYTE2_FRAME_END ; + data_pkt +=2 ; + data_len -=2 ; + } + +/* Finish field and prepare next (BULK only) */ +if (dev->analog_xfer_bulk && frame_end) { + buf =finish_field_prepare_next (dev, buf, dmaq); + dev->usb_ctl.vid_buf = buf; + } +/* NOTE: in ISOC mode when a new frame starts and buf==NULL, +* we COULD already prepare a buffer here to avoid skipping the +* first frame. +*/ + } + +/* Copy data */ +if (buf !=NULL && data_len >0 ) +em28xx_copy_video (dev, buf, data_pkt, data_len); + +/* Finish frame (ISOC only) => avoids lag of 1 frame */ +if (!dev->analog_xfer_bulk && frame_end) { + buf =finish_field_prepare_next (dev, buf, dmaq); + dev->usb_ctl.vid_buf = buf; + } + +/* NOTE: Tested with USB bulk transfers only ! +* The wording in the datasheet suggests that isoc might work different. +* The current code assumes that with isoc transfers each packet has a +* header like with the other em28xx devices. +*/ +/* NOTE: Support for interlaced mode is pure theory. It has not been +* tested and it is unknown if these devices actually support it. */ +/* NOTE: No VBI support yet (these chips likely do not support VBI). */ +} +/* Processes and copies the URB data content (video and VBI data) */ static inline int em28xx_urb_data_copy (struct em28xx *dev,struct urb *urb) {
@@ -460,7 +526,11 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)continue ; } -process_frame_data_em28xx (dev, usb_data_pkt, usb_data_len); +if (dev->is_em25xx) +process_frame_data_em25xx (dev, usb_data_pkt, usb_data_len); +else +process_frame_data_em28xx (dev, usb_data_pkt, usb_data_len); + }return 1 ; }
Signature · 1 line
1.7.10.4
PATCH v2 4/5 em28xx: make em28xx_set_outfmt() working with EM25xx family bridges
In reply to Frank SchäferStreaming doesn't work with the EM2765 if bit 5 of the output format register 0x27 is set.
It's actually not clear if really has to be set for the other chips, but for now let's keep it to avoid regressions and add a comment to the code.
--- drivers/media/usb/em28xx/em28xx-core.c | 20 +++++++++++++++-----
1 Datei geändert, 15 Zeilen hinzugefügt(+), 5 Zeilen entfernt(-)
drivers/media/usb/em28xx/em28xx-core.c +15 -5
@@ -697,12 +697,22 @@ int em28xx_vbi_supported(struct em28xx *dev)int em28xx_set_outfmt (struct em28xx *dev) {int ret; -u8 vinctrl; - - ret =em28xx_write_reg_bits (dev,EM28XX_R27_OUTFMT , - dev->format->reg |0x20 ,0xff ); +u8 fmt, vinctrl; + + fmt = dev->format->reg; +if (!dev->is_em25xx) + fmt |=0x20 ; +/* NOTE: it's not clear if this is really needed ! +* The datasheets say bit 5 is a reserved bit and devices seem to work +* fine without it. But the Windows driver sets it for em2710/50+em28xx +* devices and we've always been setting it, too. +* +* em2765 (em25xx, em276x/7x/8x ?) devices do NOT work with this bit set, +* it's likely used for an additional (compressed ?) format there. +*/ + ret =em28xx_write_reg (dev,EM28XX_R27_OUTFMT , fmt);if (ret <0 ) -return ret; +return ret; ret =em28xx_write_reg (dev,EM28XX_R10_VINMODE , dev->vinmode);if (ret <0 )
Signature · 1 line
1.7.10.4
PATCH v2 5/5 em28xx: write output frame resolution to regs 0x34+0x35 for em25xx family bridges
In reply to Frank SchäferThe Windows driver writes the output resolution to registers 0x34 (width / 16) and 0x35 (height / 16) always.
We don't know yet what these registers are used for.
--- drivers/media/usb/em28xx/em28xx-core.c | 7 +++++++ drivers/media/usb/em28xx/em28xx-reg.h | 6 ++++++
2 Dateien geändert, 13 Zeilen hinzugefügt(+)
drivers/media/usb/em28xx/em28xx-core.c +7 -0
@@ -766,6 +766,13 @@ static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,em28xx_write_regs (dev,EM28XX_R1E_CWIDTH , &cwidth,1 );em28xx_write_regs (dev,EM28XX_R1F_CHEIGHT , &cheight,1 );em28xx_write_regs (dev,EM28XX_R1B_OFLOW , &overflow,1 ); + +if (dev->is_em25xx) { +em28xx_write_reg (dev,0x34 , width >>4 ); +em28xx_write_reg (dev,0x35 , height >>4 ); + } +/* FIXME: function/meaning of these registers ? */ +/* FIXME: align width+height to multiples of 4 ?! */ }static int em28xx_scaler_set (struct em28xx *dev,u16 h,u16 v)
drivers/media/usb/em28xx/em28xx-reg.h +6 -0
@@ -167,6 +167,12 @@#define EM28XX_R34_VBI_START_H 0x34#define EM28XX_R35_VBI_START_V 0x35 +/* NOTE: the EM276x (and EM25xx, EM277x/8x ?) (camera bridges) use these +* registers for a different unknown purpose. +* => register 0x34 is set to capture width / 16 +* => register 0x35 is set to capture height / 16 +*/ +#define EM28XX_R36_VBI_WIDTH 0x36#define EM28XX_R37_VBI_HEIGHT 0x37
Signature · 1 line
1.7.10.4
Re: [PATCH v2 5/5] em28xx: write output frame resolution to regs 0x34+0x35 for em25xx family bridges
Em Sat, 23 Mar 2013 18:27:12 +0100
Frank Schäfer <[email protected]> escreveu:
Quoted 15 lines
The Windows driver writes the output resolution to registers
0x34(width / 16) and0x35(height / 16) always.
We don't know yet what these registers are used for.Signed-off-by Frank Schäfer <[email protected]>--- drivers/media/usb/em28xx/em28xx-core.c | 7 +++++++ drivers/media/usb/em28xx/em28xx-reg.h | 6 ++++++2 Dateien geändert, 13 Zeilen hinzugefügt(+)
drivers/media/usb/em28xx/em28xx-core.c +0
-0
drivers/media/usb/em28xx/em28xx-core.c static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart, · line 775
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-core.c +7
-0@@ -766,6 +766,13 @@ static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,em28xx_write_regs (dev,EM28XX_R1E_CWIDTH , &cwidth,1 );em28xx_write_regs (dev,EM28XX_R1F_CHEIGHT , &cheight,1 );em28xx_write_regs (dev,EM28XX_R1B_OFLOW , &overflow,1 ); + +if (dev->is_em25xx) { +em28xx_write_reg (dev,0x34 , width >>4 ); +em28xx_write_reg (dev,0x35 , height >>4 ); + } +/* FIXME: function/meaning of these registers ? */ +/* FIXME: */
Please move those comments to be _before_ the code you're commenting.
E. g. something like:
if (dev->is_em25xx) {
/*
* FIXME:
* - function/meaning of these registers are unknown;
* - align width+height to multiples of 4 ?!
*/
em28xx_write_reg(dev, 0x34, width >> 4);
em28xx_write_reg(dev, 0x35, height >> 4);
}Quoted 20 lines
drivers/media/usb/em28xx/em28xx-core.c +0
-0}static int em28xx_scaler_set (struct em28xx *dev,u16 h,u16 v)drivers/media/usb/em28xx/em28xx-reg.h +6
-0@@ -167,6 +167,12 @@#define EM28XX_R34_VBI_START_H 0x34#define EM28XX_R35_VBI_START_V 0x35 +/* NOTE: the EM276x (and EM25xx, EM277x/8x ?) (camera bridges) use these +* registers for a different unknown purpose. +* => register 0x34 is set to capture width / 16 +* => register 0x35 is set to capture height / 16 +*/ +#define EM28XX_R36_VBI_WIDTH 0x36#define EM28XX_R37_VBI_HEIGHT 0x37
Signature · 2 lines
Cheers, Mauro
Re: [PATCH v2 5/5] em28xx: write output frame resolution to regs 0x34+0x35 for em25xx family bridges
Am 24.03.2013 12:44, schrieb Mauro Carvalho Chehab:
Quoted 32 lines
Em Sat, 23 Mar 2013 18:27:12 +0100
Frank Schäfer <[email protected]> escreveu:Quoted 15 lines
The Windows driver writes the output resolution to registers
0x34(width / 16) and0x35(height / 16) always.
We don't know yet what these registers are used for.Signed-off-by Frank Schäfer <[email protected]>--- drivers/media/usb/em28xx/em28xx-core.c | 7 +++++++ drivers/media/usb/em28xx/em28xx-reg.h | 6 ++++++2 Dateien geändert, 13 Zeilen hinzugefügt(+)
drivers/media/usb/em28xx/em28xx-core.c +0
-0drivers/media/usb/em28xx/em28xx-core.c static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart, · line 775
Quoted 10 lines
drivers/media/usb/em28xx/em28xx-core.c +7
-0@@ -766,6 +766,13 @@ static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,em28xx_write_regs (dev,EM28XX_R1E_CWIDTH , &cwidth,1 );em28xx_write_regs (dev,EM28XX_R1F_CHEIGHT , &cheight,1 );em28xx_write_regs (dev,EM28XX_R1B_OFLOW , &overflow,1 ); + +if (dev->is_em25xx) { +em28xx_write_reg (dev,0x34 , width >>4 ); +em28xx_write_reg (dev,0x35 , height >>4 ); + } +/* FIXME: function/meaning of these registers ? */ +/* FIXME: align width+height to multiples of 4 ?! */ Please move those comments to be _before_ the code you're commenting.
E. g. something like:
Quoted 9 lines
if (dev->is_em25xx) {
/*
* FIXME:
* - function/meaning of these registers are unknown;
* - align width+height to multiples of 4 ?!
*/
em28xx_write_reg(dev, 0x34, width >> 4);
em28xx_write_reg(dev, 0x35, height >> 4);
}Ok, no problem.
Frank
Quoted 22 lines
Quoted 21 lines
drivers/media/usb/em28xx/em28xx-core.c +0
-0}static int em28xx_scaler_set (struct em28xx *dev,u16 h,u16 v)drivers/media/usb/em28xx/em28xx-reg.h +6
-0@@ -167,6 +167,12 @@#define EM28XX_R34_VBI_START_H 0x34#define EM28XX_R35_VBI_START_V 0x35 +/* NOTE: the EM276x (and EM25xx, EM277x/8x ?) (camera bridges) use these +* registers for a different unknown purpose. +* => register 0x34 is set to capture width / 16 +* => register 0x35 is set to capture height / 16 +*/ +#define EM28XX_R36_VBI_WIDTH 0x36#define EM28XX_R37_VBI_HEIGHT 0x37