cron job: media_tree daily build: WARNINGS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Thu Nov 28 04:00:29 CET 2013 git branch: test git hash: 258d2fbf874c87830664cb7ef41f9741c1abffac gcc version:i686-linux-gcc (GCC) 4.8.1 sparse version: 0.4.5-rc1 host hardware: x86_64 host os:3.12-0.slh.2-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-exynos: OK linux-git-arm-mx: OK linux-git-arm-omap: OK linux-git-arm-omap1: OK linux-git-arm-pxa: OK linux-git-blackfin: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.31.14-i686: WARNINGS linux-2.6.32.27-i686: WARNINGS linux-2.6.33.7-i686: WARNINGS linux-2.6.34.7-i686: WARNINGS linux-2.6.35.9-i686: WARNINGS linux-2.6.36.4-i686: WARNINGS linux-2.6.37.6-i686: WARNINGS linux-2.6.38.8-i686: WARNINGS linux-2.6.39.4-i686: WARNINGS linux-3.0.60-i686: WARNINGS linux-3.1.10-i686: WARNINGS linux-3.2.37-i686: OK linux-3.3.8-i686: OK linux-3.4.27-i686: WARNINGS linux-3.5.7-i686: WARNINGS linux-3.6.11-i686: WARNINGS linux-3.7.4-i686: WARNINGS linux-3.8-i686: WARNINGS linux-3.9.2-i686: WARNINGS linux-3.10.1-i686: OK linux-3.11.1-i686: OK linux-3.12-i686: OK linux-3.13-rc1-i686: OK linux-2.6.31.14-x86_64: WARNINGS linux-2.6.32.27-x86_64: WARNINGS linux-2.6.33.7-x86_64: WARNINGS linux-2.6.34.7-x86_64: WARNINGS linux-2.6.35.9-x86_64: WARNINGS linux-2.6.36.4-x86_64: WARNINGS linux-2.6.37.6-x86_64: WARNINGS linux-2.6.38.8-x86_64: WARNINGS linux-2.6.39.4-x86_64: WARNINGS linux-3.0.60-x86_64: WARNINGS linux-3.1.10-x86_64: WARNINGS linux-3.2.37-x86_64: OK linux-3.3.8-x86_64: OK linux-3.4.27-x86_64: WARNINGS linux-3.5.7-x86_64: WARNINGS linux-3.6.11-x86_64: WARNINGS linux-3.7.4-x86_64: WARNINGS linux-3.8-x86_64: WARNINGS linux-3.9.2-x86_64: WARNINGS linux-3.10.1-x86_64: OK linux-3.11.1-x86_64: OK linux-3.12-x86_64: OK linux-3.13-rc1-x86_64: OK apps: OK spec-git: OK sparse version: 0.4.5-rc1 sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] drivers: staging: media: go7007: go7007-usb.c use pr_*() instead of dev_*() before 'go' initialized in go7007_usb_probe()
On 11/27/2013 06:43 PM, Dan Carpenter wrote: > On Wed, Nov 27, 2013 at 12:24:22PM +0800, Chen Gang wrote: >> On 11/27/2013 12:03 PM, Greg KH wrote: >>> On Wed, Nov 27, 2013 at 11:48:08AM +0800, Chen Gang wrote: dev_*() assumes 'go' is already initialized, so need use pr_*() instead of before 'go' initialized. Related warning (with allmodconfig under hexagon): CC [M] drivers/staging/media/go7007/go7007-usb.o drivers/staging/media/go7007/go7007-usb.c: In function 'go7007_usb_probe': drivers/staging/media/go7007/go7007-usb.c:1060:2: warning: 'go' may be used uninitialized in this function [-Wuninitialized] Also remove useless code after 'return' statement. >>> >>> This should all be fixed in my staging-linus branch already, right? No >>> need for this anymore from what I can tell, sorry. >>> >> >> That's all right (in fact don't need sorry). :-) >> >> And excuse me, I am not quite familiar upstream kernel version merging >> and branches. Is it still better/suitable/possible to sync some bug fix >> patches from staging brach to next brach? > > next syncs with everyone once a day. > OK, thanks. :-) -- Chen Gang -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] az6007: support Technisat Cablestar Combo HDCI (minus remote)
Any chances this could get applied?
Thanks,
Roland
Am 02.11.2013 20:49, schrieb linux-media-ow...@vger.kernel.org:
> This is similar to the Terratec H7. It works with the same az6007 firmware as
> the former, however the drx-k firmware of the H7 will NOT work. Hence use
> a different firmware name. The firmware does not need to exist as the one in
> the eeprom is just fine as long as the h7 one doesn't get loaded, but maybe
> some day someone wants to load it (the one from the h5 would work too).
> Also since the config entry is now different anyway disable support for rc.
> AFAIK the Technisat remote (TS35) is RC5 and the code (which a code comment
> claims doesn't work anyway) only would handle NEC hence it's pointless
> creating
> a device and polling it if we already know it can't work.
> CI is untested.
> Originally based on idea found on
> http://www.linuxtv.org/wiki/index.php/TechniSat_CableStar_Combo_HD_CI claiming
> only id needs to be added (but failed to mention it only worked because the
> driver couldn't find the h7 drx-k firmware...).
>
> Signed-off-by: Roland Scheidegger
> ---
> drivers/media/dvb-core/dvb-usb-ids.h | 1 +
> drivers/media/usb/dvb-usb-v2/az6007.c | 59
> +++
> 2 files changed, 60 insertions(+)
>
> diff --git a/drivers/media/dvb-core/dvb-usb-ids.h
> b/drivers/media/dvb-core/dvb-usb-ids.h
> index 419a2d6..4a53454 100644
> --- a/drivers/media/dvb-core/dvb-usb-ids.h
> +++ b/drivers/media/dvb-core/dvb-usb-ids.h
> @@ -365,6 +365,7 @@
> #define USB_PID_TERRATEC_DVBS2CI_V2 0x10ac
> #define USB_PID_TECHNISAT_USB2_HDCI_V1 0x0001
> #define USB_PID_TECHNISAT_USB2_HDCI_V2 0x0002
> +#define USB_PID_TECHNISAT_USB2_CABLESTAR_HDCI0x0003
> #define USB_PID_TECHNISAT_AIRSTAR_TELESTICK_20x0004
> #define USB_PID_TECHNISAT_USB2_DVB_S20x0500
> #define USB_PID_CPYTO_REDI_PC50A 0xa803
> diff --git a/drivers/media/usb/dvb-usb-v2/az6007.c
> b/drivers/media/usb/dvb-usb-v2/az6007.c
> index 44c64ef3..c1051c3 100644
> --- a/drivers/media/usb/dvb-usb-v2/az6007.c
> +++ b/drivers/media/usb/dvb-usb-v2/az6007.c
> @@ -68,6 +68,19 @@ static struct drxk_config terratec_h7_drxk = {
> .microcode_name = "dvb-usb-terratec-h7-drxk.fw",
> };
>
> +static struct drxk_config cablestar_hdci_drxk = {
> + .adr = 0x29,
> + .parallel_ts = true,
> + .dynamic_clk = true,
> + .single_master = true,
> + .enable_merr_cfg = true,
> + .no_i2c_bridge = false,
> + .chunk_size = 64,
> + .mpeg_out_clk_strength = 0x02,
> + .qam_demod_parameter_count = 2,
> + .microcode_name = "dvb-usb-technisat-cablestar-hdci-drxk.fw",
> +};
> +
> static int drxk_gate_ctrl(struct dvb_frontend *fe, int enable)
> {
> struct az6007_device_state *st = fe_to_priv(fe);
> @@ -630,6 +643,27 @@ static int az6007_frontend_attach(struct dvb_usb_adapter
> *adap)
> return 0;
> }
>
> +static int az6007_cablestar_hdci_frontend_attach(struct dvb_usb_adapter
> *adap)
> +{
> + struct az6007_device_state *st = adap_to_priv(adap);
> + struct dvb_usb_device *d = adap_to_d(adap);
> +
> + pr_debug("attaching demod drxk\n");
> +
> + adap->fe[0] = dvb_attach(drxk_attach, &cablestar_hdci_drxk,
> + &d->i2c_adap);
> + if (!adap->fe[0])
> + return -EINVAL;
> +
> + adap->fe[0]->sec_priv = adap;
> + st->gate_ctrl = adap->fe[0]->ops.i2c_gate_ctrl;
> + adap->fe[0]->ops.i2c_gate_ctrl = drxk_gate_ctrl;
> +
> + az6007_ci_init(adap);
> +
> + return 0;
> +}
> +
> static int az6007_tuner_attach(struct dvb_usb_adapter *adap)
> {
> struct dvb_usb_device *d = adap_to_d(adap);
> @@ -868,6 +902,29 @@ static struct dvb_usb_device_properties az6007_props = {
> }
> };
>
> +static struct dvb_usb_device_properties az6007_cablestar_hdci_props = {
> + .driver_name = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
> + .firmware= AZ6007_FIRMWARE,
> +
> + .adapter_nr = adapter_nr,
> + .size_of_priv= sizeof(struct az6007_device_state),
> + .i2c_algo= &az6007_i2c_algo,
> + .tuner_attach= az6007_tuner_attach,
> + .frontend_attach = az6007_cablestar_hdci_frontend_attach,
> + .streaming_ctrl = az6007_streaming_ctrl,
> +/* ditch get_rc_config as it can't work (TS35 remote, I believe it's rc5) */
> + .get_rc_config = NULL,
> + .read_mac_address= az6007_read_mac_addr,
> + .download_firmware = az6007_download_firmware,
> + .identify_state = az6007_identify_state,
> + .power_ctrl = az6007_power_ctrl,
> + .num_adapters= 1,
> + .adapter = {
> + { .stream = DVB_USB_STREAM_BULK(0x02, 10, 4096), }
> + }
> +};
> +
> static struct usb_device_id az6007_usb_table[] = {
> {DVB_USB
Re: libdvbv5: dvb_table_pat_init is leaking memory
Em Wed, 27 Nov 2013 20:31:21 -0200 Mauro Carvalho Chehab escreveu: > Hi Gregor, > > Em Wed, 27 Nov 2013 22:55:32 +0100 > Gregor Jasny escreveu: > > > Hello, > > > > Coverity noticed that dvb_table_pat_init leaks the reallocated memory > > stored in pat: > > http://git.linuxtv.org/v4l-utils.git/blob/HEAD:/lib/libdvbv5/descriptors/pat.c#l26 > > > > Mauro, could you please check? > > On my tests with Valgrind, I'm not noticing any memory leak there, at > least on the very latest version I pushed today[1]. > > I tested here with DVB-T, DVB-T2, DVB-S, DVB-S2 and DVB-C. > > I didn't test the current version yet with ATSC or ISDB-T. Those are > on my todo list. I'll likely do ATSC test today or tomorrow. > > ISDB-T test might take some time, as I'm having some troubles to test it > here those days. > > That's said, I would love to get rid of that realloc() on PAT, but this > would break the existing userspace interface. So, such change, if done, > would require some care, as at least tvdaemon relies on it. > > Regards, > Mauro > > [1] Not sure if you noticed, but I added ~80 patches for it today. Gregor, After looking into it inside coverity, I suspect that coverity is complaining because the code is: pat = some_function(pat, size); E. g. it is not understanding that realloc takes the original pointer as an entry, and returns a pointer to the newer pointer with the bigger size. So, it thinks that the first memory allocated for pat is not visible anymore, and it will leak. FYI, this is the result of a DVB-C scanning (35 frequencies, each with its own PAT table): ==16035== Memcheck, a memory error detector ==16035== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. ==16035== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info ==16035== Command: ./utils/dvb/dvbv5-scan -I channel /home/mchehab/dvbc-teste ==16035== ERRORcommand BANDWIDTH_HZ (5) not found during retrieve INFO Scanning frequency #1 57300 Lock (0x1f) Quality= Poor Signal= 100.00% C/N= 36.40dB UCB= 36535 postBER= 1.34x10^-3 PER= 4.78x10^-3 ==16035== Warning: noted but unhandled ioctl 0x6f2a with no size/direction hints ==16035==This could cause spurious value errors to appear. ==16035==See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper. ==16035== Warning: noted but unhandled ioctl 0x6f2a with no size/direction hints ==16035==This could cause spurious value errors to appear. ==16035==See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper. ==16035== Warning: noted but unhandled ioctl 0x6f2a with no size/direction hints ==16035==This could cause spurious value errors to appear. ==16035==See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper. INFO Service SBT, provider (null): digital television INFO Service Globo, provider Globo: digital television INFO Service Record, provider (null): digital television INFO Service Band, provider (null): digital television INFO Service LBV - Rede Mundial, provider (null): digital television INFO Service Cartoon Network, provider (null): digital television INFO Service TNT, provider (null): digital television INFO Service Boomerang, provider (null): digital television INFO Service NET Games, provider (null): digital television INFO Service NET Música, provider (null): digital television INFO Service Pagode, provider (null): digital radio INFO Service Axé, provider (null): digital radio INFO Service Festa, provider (null): digital radio INFO Service Trilhas Sonoras, provider (null): digital radio INFO Service Rádio Globo RJ, provider (null): digital radio INFO Service 01070138, provider (null): user defined INFO Service 01070238, provider (null): user defined INFO New transponder/channel found: #2: 71700 INFO New transponder/channel found: #3: 72300 INFO New transponder/channel found: #4: 54900 INFO New transponder/channel found: #5: 55500 INFO New transponder/channel found: #6: 56100 INFO New transponder/channel found: #7: 56700 INFO New transponder/channel found: #8: 43500 INFO New transponder/channel found: #9: 44100 INFO New transponder/channel found: #10: 44700 INFO New transponder/channel found: #11: 45300 INFO New transponder/channel found: #12: 45900 INFO New transponder/channel found: #13: 46500 INFO New transponder/channel found: #14: 47100 INFO New transponder/channel found: #15: 57900 INFO New transponder/channel found: #16: 58500 INFO New transponder/channel found: #17: 59100 INFO New transponder/channel found: #18: 59700 INFO New transponder/channel found: #19: 60300 INFO New transponder/channel found: #20: 60900 INFO New transponder/channel found: #21: 61500 INFO New transponder/channel found: #22: 62100 INFO
Re: libdvbv5: dvb_table_pat_init is leaking memory
Hi Gregor, Em Wed, 27 Nov 2013 22:55:32 +0100 Gregor Jasny escreveu: > Hello, > > Coverity noticed that dvb_table_pat_init leaks the reallocated memory > stored in pat: > http://git.linuxtv.org/v4l-utils.git/blob/HEAD:/lib/libdvbv5/descriptors/pat.c#l26 > > Mauro, could you please check? On my tests with Valgrind, I'm not noticing any memory leak there, at least on the very latest version I pushed today[1]. I tested here with DVB-T, DVB-T2, DVB-S, DVB-S2 and DVB-C. I didn't test the current version yet with ATSC or ISDB-T. Those are on my todo list. I'll likely do ATSC test today or tomorrow. ISDB-T test might take some time, as I'm having some troubles to test it here those days. That's said, I would love to get rid of that realloc() on PAT, but this would break the existing userspace interface. So, such change, if done, would require some care, as at least tvdaemon relies on it. Regards, Mauro [1] Not sure if you noticed, but I added ~80 patches for it today. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] af9033: fix broken I2C
Driver did not work anymore since I2C has gone broken due
to recent commit:
commit 37ebaf6891ee81687bb558e8375c0712d8264ed8
[media] dvb-frontends: Don't use dynamic static allocation
Signed-off-by: Antti Palosaari
---
drivers/media/dvb-frontends/af9033.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/dvb-frontends/af9033.c
b/drivers/media/dvb-frontends/af9033.c
index 30ee590..08de532 100644
--- a/drivers/media/dvb-frontends/af9033.c
+++ b/drivers/media/dvb-frontends/af9033.c
@@ -171,7 +171,7 @@ static int af9033_wr_reg_val_tab(struct af9033_state *state,
const struct reg_val *tab, int tab_len)
{
int ret, i, j;
- u8 buf[MAX_XFER_SIZE];
+ u8 buf[212];
if (tab_len > sizeof(buf)) {
dev_warn(&state->i2c->dev,
--
1.8.4.2
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] af9035: fix broken I2C and USB I/O
There was three small buffer len calculation bugs which caused
driver non-working. These are coming from recent commit:
commit 7760e148350bf6df95662bc0db3734e9d991cb03
[media] af9035: Don't use dynamic static allocation
Signed-off-by: Antti Palosaari
---
drivers/media/usb/dvb-usb-v2/af9035.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c
b/drivers/media/usb/dvb-usb-v2/af9035.c
index c8fcd78..403bf43 100644
--- a/drivers/media/usb/dvb-usb-v2/af9035.c
+++ b/drivers/media/usb/dvb-usb-v2/af9035.c
@@ -131,7 +131,7 @@ static int af9035_wr_regs(struct dvb_usb_device *d, u32
reg, u8 *val, int len)
{
u8 wbuf[MAX_XFER_SIZE];
u8 mbox = (reg >> 16) & 0xff;
- struct usb_req req = { CMD_MEM_WR, mbox, sizeof(wbuf), wbuf, 0, NULL };
+ struct usb_req req = { CMD_MEM_WR, mbox, 6 + len, wbuf, 0, NULL };
if (6 + len > sizeof(wbuf)) {
dev_warn(&d->udev->dev, "%s: i2c wr: len=%d is too big!\n",
@@ -238,7 +238,7 @@ static int af9035_i2c_master_xfer(struct i2c_adapter *adap,
} else {
/* I2C */
u8 buf[MAX_XFER_SIZE];
- struct usb_req req = { CMD_I2C_RD, 0, sizeof(buf),
+ struct usb_req req = { CMD_I2C_RD, 0, 5 + msg[0].len,
buf, msg[1].len, msg[1].buf };
if (5 + msg[0].len > sizeof(buf)) {
@@ -274,8 +274,8 @@ static int af9035_i2c_master_xfer(struct i2c_adapter *adap,
} else {
/* I2C */
u8 buf[MAX_XFER_SIZE];
- struct usb_req req = { CMD_I2C_WR, 0, sizeof(buf), buf,
- 0, NULL };
+ struct usb_req req = { CMD_I2C_WR, 0, 5 + msg[0].len,
+ buf, 0, NULL };
if (5 + msg[0].len > sizeof(buf)) {
dev_warn(&d->udev->dev,
--
1.8.4.2
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Support for Terratec CINERGY S2 Stick HD new revision
Hi, i recently bought the above USB stick, but it seems to have a new/unknown revision. Does someone have any details which hardware is inside? [8.580089] usb 1-1.3: New USB device found, idVendor=0ccd, idProduct=0102 [8.593691] usb 1-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [8.602703] usb 1-1.3: Product: CINERGY S2 Stick HD [8.615684] usb 1-1.3: Manufacturer: Terratec [8.628459] usb 1-1.3: SerialNumber: Thanks Peter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
On Wednesday 27 November 2013 15:50:18 Lars-Peter Clausen wrote: > On 11/27/2013 01:14 PM, Hans Verkuil wrote: > [...] > > >>> For our systems the adv7604 interrupts is not always hooked up to a gpio > >>> irq, instead a register has to be read to figure out which device > >>> actually produced the irq. > >> > >> Where is that register located ? Shouldn't it be modeled as an interrupt > >> controller ? > > > > It's a PCIe interrupt whose handler needs to read several FPGA registers > > in order to figure out which interrupt was actually triggered. I don't > > know enough about interrupt controller to understand whether it can be > > modeled as a 'standard' interrupt. > > This sounds as if it should be implemented as a irq_chip driver. There are a > couple of examples in drivers/irqchip/ Exactly, that was my point. A piece of hardware that takes several interrupt inputs, includes mask and flag registers and generate a single interrupt towards the system is an interrupt controller and should have be handled by the Linux irqchip infrastructure. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
[...]
>> The driver enables multiple interrupts on the chip, however, the
>> adv7604_isr callback doesn't seem to handle them correctly.
>> According to the docs:
>> "If an interrupt event occurs, and then a second interrupt event occurs
>> before the system controller has cleared or masked the first interrupt
>> event, the ADV7611 does not generate a second interrupt signal."
>>
>> However, the interrupt_service_routine doesn't account for that.
>> For example, in case fmt_change interrupt happens while
>> fmt_change_digital interrupt is being processed by the adv7604_isr
>> routine. If fmt_change status is set just before we clear
>> fmt_change_digital, we never clear fmt_change. Thus, we end up with
>> fmt_change interrupt missed and therefore further interrupts disabled.
>> I've tried to call the adv7604_isr routine in a loop and return from
>> the worlqueue only when all interrupt status bits are cleared. This did
>> help a bit, but sometimes I started getting lots of I2C read/write
>> errors for some reason.
>
> I'm not sure if there is much that can be done about this. The code
> reads the interrupt status, then clears the interrupts right after.
> There is always a race condition there since this isn't atomic ('read
> and clear'). Unless Lars-Peter has a better idea?
>
> What can be improved, though, is to clear not just the interrupts that
> were read, but all the interrupts that are unmasked. You are right, you
> could loose an interrupt that way.
Wouldn't level-trigerred interrupts fix the issue ?
>>
>> In this case we need to disable the IRQ line in the IRQ handler and
>> re-enable it in the workqueue. (we can't call the interrupt service routine
>> from the interrupt context.)
>
> Can't we just flag the interrupt in a non-threaded IRQ handler, acknowledge
> the interrupt and then schedule work on a workqueue for the bottom half ?
Acknowledging the interrupt will require a non IRQ context, since it has to
do I2C transfers.
- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
Hi Valentine,
(CC'ing Linus Walleij, Wolfram Sang and LAKML)
On Wednesday 27 November 2013 16:32:01 Valentine wrote:
> On 11/27/2013 04:14 PM, Hans Verkuil wrote:
> > On 11/27/13 12:39, Laurent Pinchart wrote:
> >> On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
> >>> On 11/26/2013 10:28 PM, Valentine wrote:
> On 11/20/2013 07:53 PM, Valentine wrote:
> > On 11/20/2013 07:42 PM, Hans Verkuil wrote:
[snip]
> >>> So I want to keep the interrupt_service_routine(). However, adding a
> >>> gpio field to the platform_data that, if set, will tell the driver to
> >>> request an irq and setup a workqueue that calls
> >>> interrupt_service_routine() would be fine with me. That will benefit a
> >>> lot of people since using gpios is much more common.
> >>
> >> We should use the i2c_board_info.irq field for that, not a field in the
> >> platform data structure. The IRQ line could be hooked up to a non-GPIO
> >> IRQ.
> >
> > Yes, of course. Although the adv7604 has two interrupt lines, so if you
> > would want to use the second, then that would still have to be specified
> > through the platform data.
>
> In this case the GPIO should be configured as interrupt source in the
> platform code. But this doesn't seem to work with R-Car GPIO since it is
> initialized later, and the gpio_to_irq function returns an error.
> The simplest way seemed to use a GPIO number in the platform data
> to have the adv driver configure the pin and request the IRQ.
> I'm not sure how to easily defer I2C board info IRQ setup (and
> camera/subdevice probing) until GPIO driver is ready.
Good question. This looks like a core problem to me, not specific to the
adv761x driver. Linus, Wolfram, do you have a comment on that ?
> The driver enables multiple interrupts on the chip, however, the
> adv7604_isr callback doesn't seem to handle them correctly.
> According to the docs:
> "If an interrupt event occurs, and then a second interrupt event occurs
> before the system controller has cleared or masked the first interrupt
> event, the ADV7611 does not generate a second interrupt signal."
>
> However, the interrupt_service_routine doesn't account for that.
> For example, in case fmt_change interrupt happens while
> fmt_change_digital interrupt is being processed by the adv7604_isr
> routine. If fmt_change status is set just before we clear
> fmt_change_digital, we never clear fmt_change. Thus, we end up with
> fmt_change interrupt missed and therefore further interrupts disabled.
> I've tried to call the adv7604_isr routine in a loop and return from
> the worlqueue only when all interrupt status bits are cleared. This did
> help a bit, but sometimes I started getting lots of I2C read/write
> errors for some reason.
> >>>
> >>> I'm not sure if there is much that can be done about this. The code
> >>> reads the interrupt status, then clears the interrupts right after.
> >>> There is always a race condition there since this isn't atomic ('read
> >>> and clear'). Unless Lars-Peter has a better idea?
> >>>
> >>> What can be improved, though, is to clear not just the interrupts that
> >>> were read, but all the interrupts that are unmasked. You are right, you
> >>> could loose an interrupt that way.
> >>
> >> Wouldn't level-trigerred interrupts fix the issue ?
>
> In this case we need to disable the IRQ line in the IRQ handler and
> re-enable it in the workqueue. (we can't call the interrupt service routine
> from the interrupt context.)
Can't we just flag the interrupt in a non-threaded IRQ handler, acknowledge
the interrupt and then schedule work on a workqueue for the bottom half ?
> This however didn't seem to work with R-Car GPIO. Calling
> disable_irq_nosync(irq); from the GPIO LEVEL interrupt handler doesn't seem
> to disable it for some reason.
>
> Also if the isr is called by the upper level camera driver, we assume that
> it needs special handling (disabling/enabling) for the ADV76xx interrupt
> although it uses the API interrupt_service_routine callback. Not a big
> deal, but still doesn't look pretty to me.
>
> > See my earlier reply.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
Hi Hans,
On Wednesday 27 November 2013 13:14:41 Hans Verkuil wrote:
> On 11/27/13 12:39, Laurent Pinchart wrote:
> > On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
> >> On 11/26/2013 10:28 PM, Valentine wrote:
> >>> On 11/20/2013 07:53 PM, Valentine wrote:
> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> > Hi Valentine,
> >>>
> >>> Hi Hans,
> >>>
> > Did you ever look at this adv7611 driver:
> >
> > https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e
> > 4afa42af2daa12
>
> No, I missed that one somehow, although I did search for the
> adv7611/7612 before implementing this one. I'm going to look closer at
> the patch and test it.
> >>>
> >>> I've tried the patch and I doubt that it was ever tested on adv7611.
> >>> I haven't been able to make it work so far. Here's the description of
> >>> some of the issues I've encountered.
> >>>
> >>> The patch does not apply cleanly so I had to make small adjustments just
> >>> to make it apply without changing the functionality.
> >>>
> >>> First of all the driver (adv7604_dummy_client function) does not set
> >>> default I2C slave addresses in the I/O map in case they are not set in
> >>> the platform data.
> >>> This is not needed for 7604, since the default addresses are already set
> >>> in the I/O map after chip reset. However, the map is zeroed on 7611/7612
> >>> after power up, and we always have to set it manually.
> >>
> >> So, the platform data for the 7611/2 should always give i2c addresses.
> >> That seems reasonable.
> >>
> >>> I had to implement the IRQ handler since the soc_camera model does not
> >>> use interrupt_service_routine subdevice callback and R-Car VIN knows
> >>> nothing about adv7612 interrupt routed to a GPIO pin. So I had to
> >>> schedule a workqueue and call adv7604_isr from there in case an
> >>> interrupt happens.
> >>
> >> For our systems the adv7604 interrupts is not always hooked up to a gpio
> >> irq, instead a register has to be read to figure out which device
> >> actually produced the irq.
> >
> > Where is that register located ? Shouldn't it be modeled as an interrupt
> > controller ?
>
> It's a PCIe interrupt whose handler needs to read several FPGA registers
> in order to figure out which interrupt was actually triggered. I don't
> know enough about interrupt controller to understand whether it can be
> modeled as a 'standard' interrupt.
I've replied to this separately.
> >> So I want to keep the interrupt_service_routine(). However, adding a gpio
> >> field to the platform_data that, if set, will tell the driver to request
> >> an irq and setup a workqueue that calls interrupt_service_routine() would
> >> be fine with me. That will benefit a lot of people since using gpios is
> >> much more common.
> >
> > We should use the i2c_board_info.irq field for that, not a field in the
> > platform data structure. The IRQ line could be hooked up to a non-GPIO
> > IRQ.
>
> Yes, of course. Although the adv7604 has two interrupt lines, so if you
> would want to use the second, then that would still have to be specified
> through the platform data.
I believe we should then extend the I2C interrupt support. The reason for
doing so is that we want to use the interrupt DT bindings for DT platforms,
and those are handled by the I2C core.
> >>> The driver enables multiple interrupts on the chip, however, the
> >>> adv7604_isr callback doesn't seem to handle them correctly.
> >>> According to the docs:
> >>> "If an interrupt event occurs, and then a second interrupt event occurs
> >>> before the system controller has cleared or masked the first interrupt
> >>> event, the ADV7611 does not generate a second interrupt signal."
> >>>
> >>> However, the interrupt_service_routine doesn't account for that.
> >>> For example, in case fmt_change interrupt happens while
> >>> fmt_change_digital interrupt is being processed by the adv7604_isr
> >>> routine. If fmt_change status is set just before we clear
> >>> fmt_change_digital, we never clear fmt_change. Thus, we end up with
> >>> fmt_change interrupt missed and therefore further interrupts disabled.
> >>> I've tried to call the adv7604_isr routine in a loop and return from the
> >>> worlqueue only when all interrupt status bits are cleared. This did help
> >>> a bit, but sometimes I started getting lots of I2C read/write errors for
> >>> some reason.
> >>
> >> I'm not sure if there is much that can be done about this. The code reads
> >> the interrupt status, then clears the interrupts right after. There is
> >> always a race condition there since this isn't atomic ('read and clear').
> >> Unless Lars-Peter has a better idea?
> >>
> >> What can be improved, though, is to clear not just the interrupts that
> >> were read, but all the interrupts that are unmasked. You are right, you
> >> couldloose an interrupt that way.
> >
> > Wouldn't level-trigerred interrupts fix the issue ?
>
> See my earlier repl
[PATCH] cx231xx: fix i2c debug prints
Do not shift the already 7bit i2c address.
Print a message also for write+read transactions.
For write+read, print the read buffer correctly instead of using the write
buffer.
Signed-off-by: Matthias Schwarzott
---
drivers/media/usb/cx231xx/cx231xx-i2c.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c
b/drivers/media/usb/cx231xx/cx231xx-i2c.c
index 96a5a09..a0d2235 100644
--- a/drivers/media/usb/cx231xx/cx231xx-i2c.c
+++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c
@@ -371,9 +371,9 @@ static int cx231xx_i2c_xfer(struct i2c_adapter
*i2c_adap,
mutex_lock(&dev->i2c_lock);
for (i = 0; i < num; i++) {
- addr = msgs[i].addr >> 1;
+ addr = msgs[i].addr;
- dprintk2(2, "%s %s addr=%x len=%d:",
+ dprintk2(2, "%s %s addr=0x%x len=%d:",
(msgs[i].flags & I2C_M_RD) ? "read" : "write",
i == num - 1 ? "stop" : "nonstop", addr,
msgs[i].len);
if (!msgs[i].len) {
@@ -395,13 +395,21 @@ static int cx231xx_i2c_xfer(struct i2c_adapter
*i2c_adap,
} else if (i + 1 < num && (msgs[i + 1].flags & I2C_M_RD) &&
msgs[i].addr == msgs[i + 1].addr
&& (msgs[i].len <= 2) && (bus->nr < 3)) {
+ /* write bytes */
+ if (i2c_debug >= 2) {
+ for (byte = 0; byte < msgs[i].len; byte++)
+ printk(" %02x", msgs[i].buf[byte]);
+ }
/* read bytes */
+ dprintk2(2, "plus %s %s addr=0x%x len=%d:",
+ (msgs[i+1].flags & I2C_M_RD) ? "read" :
"write",
+ i+1 == num - 1 ? "stop" : "nonstop",
addr, msgs[i+1].len);
rc = cx231xx_i2c_recv_bytes_with_saddr(i2c_adap,
&msgs[i],
&msgs[i + 1]);
if (i2c_debug >= 2) {
- for (byte = 0; byte < msgs[i].len; byte++)
- printk(" %02x", msgs[i].buf[byte]);
+ for (byte = 0; byte < msgs[i+1].len; byte++)
+ printk(" %02x",
msgs[i+1].buf[byte]);
}
i++;
} else {
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] cx231xx: Add missing selects for MEDIA_SUBDRV_AUTOSELECT
The two drivers LGDT3305 and TDA18271C2DD were not autoselected, so the cx231xx_dvb module could not be loaded if MEDIA_SUBDRV_AUTOSELECT is enabled. Signed-off-by: Matthias Schwarzott --- drivers/media/usb/cx231xx/Kconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/media/usb/cx231xx/Kconfig b/drivers/media/usb/cx231xx/Kconfig index 86feeea..f14c5e8 100644 --- a/drivers/media/usb/cx231xx/Kconfig +++ b/drivers/media/usb/cx231xx/Kconfig @@ -45,6 +45,8 @@ config VIDEO_CX231XX_DVB select MEDIA_TUNER_XC5000 if MEDIA_SUBDRV_AUTOSELECT select MEDIA_TUNER_TDA18271 if MEDIA_SUBDRV_AUTOSELECT select DVB_MB86A20S if MEDIA_SUBDRV_AUTOSELECT + select DVB_LGDT3305 if MEDIA_SUBDRV_AUTOSELECT + select DVB_TDA18271C2DD if MEDIA_SUBDRV_AUTOSELECT ---help--- This adds support for DVB cards based on the -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
On 11/27/2013 01:14 PM, Hans Verkuil wrote: [...] >>> For our systems the adv7604 interrupts is not always hooked up to a gpio >>> irq, instead a register has to be read to figure out which device actually >>> produced the irq. >> >> Where is that register located ? Shouldn't it be modeled as an interrupt >> controller ? > > It's a PCIe interrupt whose handler needs to read several FPGA registers > in order to figure out which interrupt was actually triggered. I don't > know enough about interrupt controller to understand whether it can be > modeled as a 'standard' interrupt. This sounds as if it should be implemented as a irq_chip driver. There are a couple of examples in drivers/irqchip/ - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
On 11/27/2013 05:07 PM, Lars-Peter Clausen wrote: On 11/27/2013 01:32 PM, Valentine wrote: On 11/27/2013 04:14 PM, Hans Verkuil wrote: Hi Laurent, On 11/27/13 12:39, Laurent Pinchart wrote: Hi Hans, On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote: On 11/26/2013 10:28 PM, Valentine wrote: On 11/20/2013 07:53 PM, Valentine wrote: On 11/20/2013 07:42 PM, Hans Verkuil wrote: Hi Valentine, Hi Hans, Did you ever look at this adv7611 driver: https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a fa42af2daa12 No, I missed that one somehow, although I did search for the adv7611/7612 before implementing this one. I'm going to look closer at the patch and test it. I've tried the patch and I doubt that it was ever tested on adv7611. I haven't been able to make it work so far. Here's the description of some of the issues I've encountered. The patch does not apply cleanly so I had to make small adjustments just to make it apply without changing the functionality. First of all the driver (adv7604_dummy_client function) does not set default I2C slave addresses in the I/O map in case they are not set in the platform data. This is not needed for 7604, since the default addresses are already set in the I/O map after chip reset. However, the map is zeroed on 7611/7612 after power up, and we always have to set it manually. So, the platform data for the 7611/2 should always give i2c addresses. That seems reasonable. I had to implement the IRQ handler since the soc_camera model does not use interrupt_service_routine subdevice callback and R-Car VIN knows nothing about adv7612 interrupt routed to a GPIO pin. So I had to schedule a workqueue and call adv7604_isr from there in case an interrupt happens. For our systems the adv7604 interrupts is not always hooked up to a gpio irq, instead a register has to be read to figure out which device actually produced the irq. Where is that register located ? Shouldn't it be modeled as an interrupt controller ? It's a PCIe interrupt whose handler needs to read several FPGA registers in order to figure out which interrupt was actually triggered. I don't know enough about interrupt controller to understand whether it can be modeled as a 'standard' interrupt. So I want to keep the interrupt_service_routine(). However, adding a gpio field to the platform_data that, if set, will tell the driver to request an irq and setup a workqueue that calls interrupt_service_routine() would be fine with me. That will benefit a lot of people since using gpios is much more common. We should use the i2c_board_info.irq field for that, not a field in the platform data structure. The IRQ line could be hooked up to a non-GPIO IRQ. Yes, of course. Although the adv7604 has two interrupt lines, so if you would want to use the second, then that would still have to be specified through the platform data. In this case the GPIO should be configured as interrupt source in the platform code. But this doesn't seem to work with R-Car GPIO since it is initialized later, and the gpio_to_irq function returns an error. The simplest way seemed to use a GPIO number in the platform data to have the adv driver configure the pin and request the IRQ. I'm not sure how to easily defer I2C board info IRQ setup (and camera/subdevice probing) until GPIO driver is ready. The GPIO driver should set up the GPIO pin as a interrupt pin when the interrupt is requested. We should not have to add hacks to adv7604 driver to workaround a broken GPIO driver. The GPIO driver does set up the pin as IRQ when the interrupt is requested. The problem is that we can't get the IRQ number from the GPIO pin number until the GPIO driver is started, which happens after the I2C device is registered by the platform code. So, the platform (board) init can't set up the i2c board info IRQ. Using GPIO numbers in platform data seems a simple solution to that. Besides, the chip supports two IRQ lines (as Hans has mentioned), while the I2C board info has only one irq member. I'm not sure that gpio_to_irq is supposed to always work (even at early board initialization) and never return -EPROBE_DEFER. If it is, then it's a GPIO driver issue. Otherwise, this is a question of I2C slave deferred probing, I think, which is not that simple. The driver enables multiple interrupts on the chip, however, the adv7604_isr callback doesn't seem to handle them correctly. According to the docs: "If an interrupt event occurs, and then a second interrupt event occurs before the system controller has cleared or masked the first interrupt event, the ADV7611 does not generate a second interrupt signal." However, the interrupt_service_routine doesn't account for that. For example, in case fmt_change interrupt happens while fmt_change_digital interrupt is being processed by the adv7604_isr routine. If fmt_change status is set just before we clear fmt_change_digital, we never clear fmt_change. Thus, we end up w
Re: [PATCH V2] media: i2c: Add ADV761X support
On 11/27/2013 01:32 PM, Valentine wrote: > On 11/27/2013 04:14 PM, Hans Verkuil wrote: >> Hi Laurent, >> >> On 11/27/13 12:39, Laurent Pinchart wrote: >>> Hi Hans, >>> >>> On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote: On 11/26/2013 10:28 PM, Valentine wrote: > On 11/20/2013 07:53 PM, Valentine wrote: >> On 11/20/2013 07:42 PM, Hans Verkuil wrote: >>> Hi Valentine, > > Hi Hans, > >>> Did you ever look at this adv7611 driver: >>> >>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a >>> fa42af2daa12 >> >> No, I missed that one somehow, although I did search for the adv7611/7612 >> before implementing this one. I'm going to look closer at the patch and >> test it. > > I've tried the patch and I doubt that it was ever tested on adv7611. > I haven't been able to make it work so far. Here's the description of some > of the issues I've encountered. > > The patch does not apply cleanly so I had to make small adjustments just > to make it apply without changing the functionality. > > First of all the driver (adv7604_dummy_client function) does not set > default I2C slave addresses in the I/O map in case they are not set in > the platform data. > This is not needed for 7604, since the default addresses are already set > in the I/O map after chip reset. However, the map is zeroed on 7611/7612 > after power up, and we always have to set it manually. So, the platform data for the 7611/2 should always give i2c addresses. That seems reasonable. > I had to implement the IRQ handler since the soc_camera model does not use > interrupt_service_routine subdevice callback and R-Car VIN knows nothing > about adv7612 interrupt routed to a GPIO pin. > So I had to schedule a workqueue and call adv7604_isr from there in case > an interrupt happens. For our systems the adv7604 interrupts is not always hooked up to a gpio irq, instead a register has to be read to figure out which device actually produced the irq. >>> >>> Where is that register located ? Shouldn't it be modeled as an interrupt >>> controller ? >> >> It's a PCIe interrupt whose handler needs to read several FPGA registers >> in order to figure out which interrupt was actually triggered. I don't >> know enough about interrupt controller to understand whether it can be >> modeled as a 'standard' interrupt. >> >>> So I want to keep the interrupt_service_routine(). However, adding a gpio field to the platform_data that, if set, will tell the driver to request an irq and setup a workqueue that calls interrupt_service_routine() would be fine with me. That will benefit a lot of people since using gpios is much more common. >>> >>> We should use the i2c_board_info.irq field for that, not a field in the >>> platform data structure. The IRQ line could be hooked up to a non-GPIO IRQ. >> >> Yes, of course. Although the adv7604 has two interrupt lines, so if you >> would want to use the second, then that would still have to be specified >> through the platform data. > > In this case the GPIO should be configured as interrupt source in the platform > code. But this doesn't seem to work with R-Car GPIO since it is initialized > later, and the gpio_to_irq function returns an error. > The simplest way seemed to use a GPIO number in the platform data > to have the adv driver configure the pin and request the IRQ. > I'm not sure how to easily defer I2C board info IRQ setup (and > camera/subdevice probing) > until GPIO driver is ready. The GPIO driver should set up the GPIO pin as a interrupt pin when the interrupt is requested. We should not have to add hacks to adv7604 driver to workaround a broken GPIO driver. > >> >>> > The driver enables multiple interrupts on the chip, however, the > adv7604_isr callback doesn't seem to handle them correctly. > According to the docs: > "If an interrupt event occurs, and then a second interrupt event occurs > before the system controller has cleared or masked the first interrupt > event, the ADV7611 does not generate a second interrupt signal." > > However, the interrupt_service_routine doesn't account for that. > For example, in case fmt_change interrupt happens while fmt_change_digital > interrupt is being processed by the adv7604_isr routine. If fmt_change > status is set just before we clear fmt_change_digital, we never clear > fmt_change. Thus, we end up with fmt_change interrupt missed and > therefore further interrupts disabled. I've tried to call the adv7604_isr > routine in a loop and return from the worlqueue only when all interrupt > status bits are cleared. This did help a bit, but sometimes I started > getting lots of I2C read/write errors for some reason. I'm not sure if there is much that can be done about this. The code read
Re: [PATCH V2] media: i2c: Add ADV761X support
On 11/27/2013 04:14 PM, Hans Verkuil wrote:
Hi Laurent,
On 11/27/13 12:39, Laurent Pinchart wrote:
Hi Hans,
On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
On 11/26/2013 10:28 PM, Valentine wrote:
On 11/20/2013 07:53 PM, Valentine wrote:
On 11/20/2013 07:42 PM, Hans Verkuil wrote:
Hi Valentine,
Hi Hans,
Did you ever look at this adv7611 driver:
https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a
fa42af2daa12
No, I missed that one somehow, although I did search for the adv7611/7612
before implementing this one. I'm going to look closer at the patch and
test it.
I've tried the patch and I doubt that it was ever tested on adv7611.
I haven't been able to make it work so far. Here's the description of some
of the issues I've encountered.
The patch does not apply cleanly so I had to make small adjustments just
to make it apply without changing the functionality.
First of all the driver (adv7604_dummy_client function) does not set
default I2C slave addresses in the I/O map in case they are not set in
the platform data.
This is not needed for 7604, since the default addresses are already set
in the I/O map after chip reset. However, the map is zeroed on 7611/7612
after power up, and we always have to set it manually.
So, the platform data for the 7611/2 should always give i2c addresses. That
seems reasonable.
I had to implement the IRQ handler since the soc_camera model does not use
interrupt_service_routine subdevice callback and R-Car VIN knows nothing
about adv7612 interrupt routed to a GPIO pin.
So I had to schedule a workqueue and call adv7604_isr from there in case
an interrupt happens.
For our systems the adv7604 interrupts is not always hooked up to a gpio
irq, instead a register has to be read to figure out which device actually
produced the irq.
Where is that register located ? Shouldn't it be modeled as an interrupt
controller ?
It's a PCIe interrupt whose handler needs to read several FPGA registers
in order to figure out which interrupt was actually triggered. I don't
know enough about interrupt controller to understand whether it can be
modeled as a 'standard' interrupt.
So I want to keep the interrupt_service_routine(). However, adding a gpio
field to the platform_data that, if set, will tell the driver to request an
irq and setup a workqueue that calls interrupt_service_routine() would be
fine with me. That will benefit a lot of people since using gpios is much
more common.
We should use the i2c_board_info.irq field for that, not a field in the
platform data structure. The IRQ line could be hooked up to a non-GPIO IRQ.
Yes, of course. Although the adv7604 has two interrupt lines, so if you
would want to use the second, then that would still have to be specified
through the platform data.
In this case the GPIO should be configured as interrupt source in the platform
code. But this doesn't seem to work with R-Car GPIO since it is initialized
later, and the gpio_to_irq function returns an error.
The simplest way seemed to use a GPIO number in the platform data
to have the adv driver configure the pin and request the IRQ.
I'm not sure how to easily defer I2C board info IRQ setup (and camera/subdevice
probing)
until GPIO driver is ready.
The driver enables multiple interrupts on the chip, however, the
adv7604_isr callback doesn't seem to handle them correctly.
According to the docs:
"If an interrupt event occurs, and then a second interrupt event occurs
before the system controller has cleared or masked the first interrupt
event, the ADV7611 does not generate a second interrupt signal."
However, the interrupt_service_routine doesn't account for that.
For example, in case fmt_change interrupt happens while fmt_change_digital
interrupt is being processed by the adv7604_isr routine. If fmt_change
status is set just before we clear fmt_change_digital, we never clear
fmt_change. Thus, we end up with fmt_change interrupt missed and
therefore further interrupts disabled. I've tried to call the adv7604_isr
routine in a loop and return from the worlqueue only when all interrupt
status bits are cleared. This did help a bit, but sometimes I started
getting lots of I2C read/write errors for some reason.
I'm not sure if there is much that can be done about this. The code reads
the interrupt status, then clears the interrupts right after. There is
always a race condition there since this isn't atomic ('read and clear').
Unless Lars-Peter has a better idea?
What can be improved, though, is to clear not just the interrupts that were
read, but all the interrupts that are unmasked. You are right, you could
loose an interrupt that way.
Wouldn't level-trigerred interrupts fix the issue ?
In this case we need to disable the IRQ line in the IRQ handler and re-enable
it in the workqueue.
(we can't call the interrupt service routine from the interrupt context.)
This however didn't seem to work with R-Car GPIO.
Calling disable_irq_nosync(ir
Re: [PATCH V2] media: i2c: Add ADV761X support
Hi Laurent,
On 11/27/13 12:39, Laurent Pinchart wrote:
> Hi Hans,
>
> On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
>> On 11/26/2013 10:28 PM, Valentine wrote:
>>> On 11/20/2013 07:53 PM, Valentine wrote:
On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> Hi Valentine,
>>>
>>> Hi Hans,
>>>
> Did you ever look at this adv7611 driver:
>
> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a
> fa42af2daa12
No, I missed that one somehow, although I did search for the adv7611/7612
before implementing this one. I'm going to look closer at the patch and
test it.
>>>
>>> I've tried the patch and I doubt that it was ever tested on adv7611.
>>> I haven't been able to make it work so far. Here's the description of some
>>> of the issues I've encountered.
>>>
>>> The patch does not apply cleanly so I had to make small adjustments just
>>> to make it apply without changing the functionality.
>>>
>>> First of all the driver (adv7604_dummy_client function) does not set
>>> default I2C slave addresses in the I/O map in case they are not set in
>>> the platform data.
>>> This is not needed for 7604, since the default addresses are already set
>>> in the I/O map after chip reset. However, the map is zeroed on 7611/7612
>>> after power up, and we always have to set it manually.
>>
>> So, the platform data for the 7611/2 should always give i2c addresses. That
>> seems reasonable.
>>
>>> I had to implement the IRQ handler since the soc_camera model does not use
>>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing
>>> about adv7612 interrupt routed to a GPIO pin.
>>> So I had to schedule a workqueue and call adv7604_isr from there in case
>>> an interrupt happens.
>>
>> For our systems the adv7604 interrupts is not always hooked up to a gpio
>> irq, instead a register has to be read to figure out which device actually
>> produced the irq.
>
> Where is that register located ? Shouldn't it be modeled as an interrupt
> controller ?
It's a PCIe interrupt whose handler needs to read several FPGA registers
in order to figure out which interrupt was actually triggered. I don't
know enough about interrupt controller to understand whether it can be
modeled as a 'standard' interrupt.
>
>> So I want to keep the interrupt_service_routine(). However, adding a gpio
>> field to the platform_data that, if set, will tell the driver to request an
>> irq and setup a workqueue that calls interrupt_service_routine() would be
>> fine with me. That will benefit a lot of people since using gpios is much
>> more common.
>
> We should use the i2c_board_info.irq field for that, not a field in the
> platform data structure. The IRQ line could be hooked up to a non-GPIO IRQ.
Yes, of course. Although the adv7604 has two interrupt lines, so if you
would want to use the second, then that would still have to be specified
through the platform data.
>
>>> The driver enables multiple interrupts on the chip, however, the
>>> adv7604_isr callback doesn't seem to handle them correctly.
>>> According to the docs:
>>> "If an interrupt event occurs, and then a second interrupt event occurs
>>> before the system controller has cleared or masked the first interrupt
>>> event, the ADV7611 does not generate a second interrupt signal."
>>>
>>> However, the interrupt_service_routine doesn't account for that.
>>> For example, in case fmt_change interrupt happens while fmt_change_digital
>>> interrupt is being processed by the adv7604_isr routine. If fmt_change
>>> status is set just before we clear fmt_change_digital, we never clear
>>> fmt_change. Thus, we end up with fmt_change interrupt missed and
>>> therefore further interrupts disabled. I've tried to call the adv7604_isr
>>> routine in a loop and return from the worlqueue only when all interrupt
>>> status bits are cleared. This did help a bit, but sometimes I started
>>> getting lots of I2C read/write errors for some reason.
>>
>> I'm not sure if there is much that can be done about this. The code reads
>> the interrupt status, then clears the interrupts right after. There is
>> always a race condition there since this isn't atomic ('read and clear').
>> Unless Lars-Peter has a better idea?
>>
>> What can be improved, though, is to clear not just the interrupts that were
>> read, but all the interrupts that are unmasked. You are right, you could
>> loose an interrupt that way.
>
> Wouldn't level-trigerred interrupts fix the issue ?
See my earlier reply.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
Hi Hans,
On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
> On 11/26/2013 10:28 PM, Valentine wrote:
> > On 11/20/2013 07:53 PM, Valentine wrote:
> >> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> >>> Hi Valentine,
> >
> > Hi Hans,
> >
> >>> Did you ever look at this adv7611 driver:
> >>>
> >>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a
> >>> fa42af2daa12
> >>
> >> No, I missed that one somehow, although I did search for the adv7611/7612
> >> before implementing this one. I'm going to look closer at the patch and
> >> test it.
> >
> > I've tried the patch and I doubt that it was ever tested on adv7611.
> > I haven't been able to make it work so far. Here's the description of some
> > of the issues I've encountered.
> >
> > The patch does not apply cleanly so I had to make small adjustments just
> > to make it apply without changing the functionality.
> >
> > First of all the driver (adv7604_dummy_client function) does not set
> > default I2C slave addresses in the I/O map in case they are not set in
> > the platform data.
> > This is not needed for 7604, since the default addresses are already set
> > in the I/O map after chip reset. However, the map is zeroed on 7611/7612
> > after power up, and we always have to set it manually.
>
> So, the platform data for the 7611/2 should always give i2c addresses. That
> seems reasonable.
>
> > I had to implement the IRQ handler since the soc_camera model does not use
> > interrupt_service_routine subdevice callback and R-Car VIN knows nothing
> > about adv7612 interrupt routed to a GPIO pin.
> > So I had to schedule a workqueue and call adv7604_isr from there in case
> > an interrupt happens.
>
> For our systems the adv7604 interrupts is not always hooked up to a gpio
> irq, instead a register has to be read to figure out which device actually
> produced the irq.
Where is that register located ? Shouldn't it be modeled as an interrupt
controller ?
> So I want to keep the interrupt_service_routine(). However, adding a gpio
> field to the platform_data that, if set, will tell the driver to request an
> irq and setup a workqueue that calls interrupt_service_routine() would be
> fine with me. That will benefit a lot of people since using gpios is much
> more common.
We should use the i2c_board_info.irq field for that, not a field in the
platform data structure. The IRQ line could be hooked up to a non-GPIO IRQ.
> > The driver enables multiple interrupts on the chip, however, the
> > adv7604_isr callback doesn't seem to handle them correctly.
> > According to the docs:
> > "If an interrupt event occurs, and then a second interrupt event occurs
> > before the system controller has cleared or masked the first interrupt
> > event, the ADV7611 does not generate a second interrupt signal."
> >
> > However, the interrupt_service_routine doesn't account for that.
> > For example, in case fmt_change interrupt happens while fmt_change_digital
> > interrupt is being processed by the adv7604_isr routine. If fmt_change
> > status is set just before we clear fmt_change_digital, we never clear
> > fmt_change. Thus, we end up with fmt_change interrupt missed and
> > therefore further interrupts disabled. I've tried to call the adv7604_isr
> > routine in a loop and return from the worlqueue only when all interrupt
> > status bits are cleared. This did help a bit, but sometimes I started
> > getting lots of I2C read/write errors for some reason.
>
> I'm not sure if there is much that can be done about this. The code reads
> the interrupt status, then clears the interrupts right after. There is
> always a race condition there since this isn't atomic ('read and clear').
> Unless Lars-Peter has a better idea?
>
> What can be improved, though, is to clear not just the interrupts that were
> read, but all the interrupts that are unmasked. You are right, you could
> loose an interrupt that way.
Wouldn't level-trigerred interrupts fix the issue ?
> > I'm also not sure how the dv_timing API should be used.
> > The internal adv7604 state->timings structure is used when getting mbus
> > format. However, the driver does not set the structure neither at
> > start-up nor in the interrupt service callback when format changes. Is it
> > supposed to be set by the upper level camera driver?
>
> It would be nice if the adv7604 would set up an initial timings format. In
> our case it is indeed the bridge driver that sets it up, but in the general
> case it is better if the i2c driver also sets an initial timings struct.
> 720p60 is generally a good initial value.
>
> The irq certainly shouldn't change timings: changing timings will most
> likely require changes in the video buffer sizes, which generally requires
> stopping streaming, reconfiguring the pipeline and restarting streaming.
> That's not something the i2c driver can do.
>
> The confusion you have with mbus vs dv_timings is that soc_camera lacks
> dv_timings support. It was designed fo
Re: [PATCH V2] media: i2c: Add ADV761X support
On 11/27/13 10:59, Lars-Peter Clausen wrote:
> [...]
>>> I had to implement the IRQ handler since the soc_camera model does not use
>>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing
>>> about adv7612
>>> interrupt routed to a GPIO pin.
>>> So I had to schedule a workqueue and call adv7604_isr from there in case an
>>> interrupt happens.
>>
>> For our systems the adv7604 interrupts is not always hooked up to a gpio
>> irq, instead
>> a register has to be read to figure out which device actually produced the
>> irq. So I
>> want to keep the interrupt_service_routine(). However, adding a gpio field
>> to the
>> platform_data that, if set, will tell the driver to request an irq and setup
>> a
>> workqueue that calls interrupt_service_routine() would be fine with me. That
>> will
>> benefit a lot of people since using gpios is much more common.
>
> I'll look into adding that since I'm using a GPIO for the interrupt on my
> platform as well.
>
>>
>>> The driver enables multiple interrupts on the chip, however, the
>>> adv7604_isr callback doesn't
>>> seem to handle them correctly.
>>> According to the docs:
>>> "If an interrupt event occurs, and then a second interrupt event occurs
>>> before the system controller
>>> has cleared or masked the first interrupt event, the ADV7611 does not
>>> generate a second interrupt signal."
>>>
>>> However, the interrupt_service_routine doesn't account for that.
>>> For example, in case fmt_change interrupt happens while fmt_change_digital
>>> interrupt is being
>>> processed by the adv7604_isr routine. If fmt_change status is set just
>>> before we clear fmt_change_digital,
>>> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed
>>> and therefore further interrupts disabled.
>>> I've tried to call the adv7604_isr routine in a loop and return from the
>>> worlqueue only when all interrupt status bits are cleared.
>>> This did help a bit, but sometimes I started getting lots of I2C read/write
>>> errors for some reason.
>>
>> I'm not sure if there is much that can be done about this. The code reads the
>> interrupt status, then clears the interrupts right after. There is always a
>> race condition there since this isn't atomic ('read and clear'). Unless
>> Lars-Peter
>> has a better idea?
>>
>
> As far as I understand it the interrupts lines are level triggered and will
> stay asserted as long as there are unmasked, non-acked IRQS.
You are correct. If you are using level interrupts, then the current
implementation works fine. However, when using edge interrupts (and we
have one system that apparently has only edge-triggered GPIOs), then
it will fail.
The only way to handle that is to mask all interrupts at the start of
the isr, process the interrupts as usual, and unmask the interrupts
at the end of the isr. AFAICT this method should be safe as well with
level triggered interrupts.
Disregard my comment about clearing all interrupts, that's bogus.
> So the
> interrupt handler should be re-entered if there are still pending
> interrupts. Is it possible that you setup a edge triggered interrupt, in
> that case the handler wouldn't be reentered if the signal stays asserted?
>
> But that's just my understanding from the manual, I'll have to verify
> whether the hardware does indeed work like that.
>
> - Lars
>
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
On 11/27/13 11:29, Valentine wrote:
> On 11/27/2013 12:21 PM, Hans Verkuil wrote:
>> On 11/26/2013 10:28 PM, Valentine wrote:
>>> On 11/20/2013 07:53 PM, Valentine wrote:
On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> Hi Valentine,
>
> Hi Hans,
>
>>>
>>> Hi Hans,
>>>
>
> Did you ever look at this adv7611 driver:
>
> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12
No, I missed that one somehow, although I did search for the adv7611/7612
before implementing this one.
I'm going to look closer at the patch and test it.
>>>
>>> I've tried the patch and I doubt that it was ever tested on adv7611.
>>> I haven't been able to make it work so far. Here's the description of some
>>> of the issues
>>> I've encountered.
>>>
>>> The patch does not apply cleanly so I had to make small adjustments just to
>>> make it apply
>>> without changing the functionality.
>>>
>>> First of all the driver (adv7604_dummy_client function) does not set
>>> default I2C slave addresses
>>> in the I/O map in case they are not set in the platform data.
>>> This is not needed for 7604, since the default addresses are already set in
>>> the I/O map after chip reset.
>>> However, the map is zeroed on 7611/7612 after power up, and we always have
>>> to set it manually.
>>
>> So, the platform data for the 7611/2 should always give i2c addresses. That
>> seems
>> reasonable.
>
> Yes, but currently the comment in include/media/adv7604.h says
> "i2c addresses: 0 == use default", and this is true for 7604, but
> it doesn't work for 7611.
>
> Probably the recommended value from the docs should be set by
> the driver in case an I2C address is zero in the platform data.
> This will help us to keep the same approach across all 76xx chips.
That would work for me.
>
>>
>>> I had to implement the IRQ handler since the soc_camera model does not use
>>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing
>>> about adv7612
>>> interrupt routed to a GPIO pin.
>>> So I had to schedule a workqueue and call adv7604_isr from there in case an
>>> interrupt happens.
>>
>> For our systems the adv7604 interrupts is not always hooked up to a gpio
>> irq, instead
>> a register has to be read to figure out which device actually produced the
>> irq. So I
>> want to keep the interrupt_service_routine(). However, adding a gpio field
>> to the
>> platform_data that, if set, will tell the driver to request an irq and setup
>> a
>> workqueue that calls interrupt_service_routine() would be fine with me. That
>> will
>> benefit a lot of people since using gpios is much more common.
>>
>>> The driver enables multiple interrupts on the chip, however, the
>>> adv7604_isr callback doesn't
>>> seem to handle them correctly.
>>> According to the docs:
>>> "If an interrupt event occurs, and then a second interrupt event occurs
>>> before the system controller
>>> has cleared or masked the first interrupt event, the ADV7611 does not
>>> generate a second interrupt signal."
>>>
>>> However, the interrupt_service_routine doesn't account for that.
>>> For example, in case fmt_change interrupt happens while fmt_change_digital
>>> interrupt is being
>>> processed by the adv7604_isr routine. If fmt_change status is set just
>>> before we clear fmt_change_digital,
>>> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed
>>> and therefore further interrupts disabled.
>>> I've tried to call the adv7604_isr routine in a loop and return from the
>>> worlqueue only when all interrupt status bits are cleared.
>>> This did help a bit, but sometimes I started getting lots of I2C read/write
>>> errors for some reason.
>>
>> I'm not sure if there is much that can be done about this. The code reads the
>> interrupt status, then clears the interrupts right after. There is always a
>> race condition there since this isn't atomic ('read and clear'). Unless
>> Lars-Peter
>> has a better idea?
>>
>> What can be improved, though, is to clear not just the interrupts that were
>> read, but all the interrupts that are unmasked. You are right, you could
>> loose an interrupt that way.
>>
>>> I'm also not sure how the dv_timing API should be used.
>>> The internal adv7604 state->timings structure is used when getting mbus
>>> format.
>>> However, the driver does not set the structure neither at start-up nor in
>>> the interrupt service callback when format changes.
>>> Is it supposed to be set by the upper level camera driver?
>>
>> It would be nice if the adv7604 would set up an initial timings format. In
>> our
>> case it is indeed the bridge driver that sets it up, but in the general case
>> it
>> is better if the i2c driver also sets an initial timings struct. 720p60 is
>> generally a good initial value.
>>
>> The irq certainly shouldn't change timings: changing timings will most likely
>> require changes in the video buffer sizes, which generally re
Re: [PATCH v2] drivers: staging: media: go7007: go7007-usb.c use pr_*() instead of dev_*() before 'go' initialized in go7007_usb_probe()
On Wed, Nov 27, 2013 at 12:24:22PM +0800, Chen Gang wrote: > On 11/27/2013 12:03 PM, Greg KH wrote: > > On Wed, Nov 27, 2013 at 11:48:08AM +0800, Chen Gang wrote: > >> dev_*() assumes 'go' is already initialized, so need use pr_*() instead > >> of before 'go' initialized. Related warning (with allmodconfig under > >> hexagon): > >> > >> CC [M] drivers/staging/media/go7007/go7007-usb.o > >> drivers/staging/media/go7007/go7007-usb.c: In function > >> 'go7007_usb_probe': > >> drivers/staging/media/go7007/go7007-usb.c:1060:2: warning: 'go' may be > >> used uninitialized in this function [-Wuninitialized] > >> > >> Also remove useless code after 'return' statement. > > > > This should all be fixed in my staging-linus branch already, right? No > > need for this anymore from what I can tell, sorry. > > > > That's all right (in fact don't need sorry). :-) > > And excuse me, I am not quite familiar upstream kernel version merging > and branches. Is it still better/suitable/possible to sync some bug fix > patches from staging brach to next brach? next syncs with everyone once a day. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/8] vb2: retry start_streaming in case of insufficient buffers.
Hi Hans,
Re-reading the code I can't see my original point anymore :-/ Let's assume I
was just wrong. I have two additional small comments though, please see below.
On Wednesday 27 November 2013 08:17:15 Hans Verkuil wrote:
> On 11/26/2013 04:46 PM, Laurent Pinchart wrote:
> > On Friday 22 November 2013 09:43:23 Hans Verkuil wrote:
> >> On 11/21/2013 08:09 PM, Laurent Pinchart wrote:
> >>> On Thursday 21 November 2013 16:22:02 Hans Verkuil wrote:
> From: Hans Verkuil
>
> If start_streaming returns -ENODATA, then it will be retried the next
> time a buffer is queued. This means applications no longer need to know
> how many buffers need to be queued before STREAMON can be called. This
> is particularly useful for output stream I/O.
>
> If a DMA engine needs at least X buffers before it can start streaming,
> then for applications to get a buffer out as soon as possible they need
> to know the minimum number of buffers to queue before STREAMON can be
> called. You can't just try STREAMON after every buffer since on failure
> STREAMON will dequeue all your buffers. (Is that a bug or a feature?
> Frankly, I'm not sure).
>
> This patch simplifies applications substantially: they can just call
> STREAMON at the beginning and then start queuing buffers and the DMA
> engine will kick in automagically once enough buffers are available.
>
> This also fixes using write() to stream video: the fileio
> implementation calls streamon without having any queued buffers, which
> will fail today for any driver that requires a minimum number of
> buffers.
>
> Signed-off-by: Hans Verkuil
> ---
>
> drivers/media/v4l2-core/videobuf2-core.c | 66 +---
> include/media/videobuf2-core.h | 15 ++--
> 2 files changed, 64 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 9ea3ae9..5bb91f7
> 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1332,6 +1332,39 @@ int vb2_prepare_buf(struct vb2_queue *q, struct
> v4l2_buffer *b) }
>
> EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>
> +/**
> + * vb2_start_streaming() - Attempt to start streaming.
> + * @q: videobuf2 queue
> + *
> + * If there are not enough buffers, then retry_start_streaming is set
> to
> + * true and 0 is returned. The next time a buffer is queued and
> + * retry_start_streaming is true, this function will be called again
> to
> + * retry starting the DMA engine.
> + */
> +static int vb2_start_streaming(struct vb2_queue *q)
> +{
> +int ret;
> +
> +/* Tell the driver to start streaming */
> +ret = call_qop(q, start_streaming, q,
> atomic_read(&q->queued_count));
> +
> +/*
> + * If there are not enough buffers queued to start streaming,
> then
> + * the start_streaming operation will return -ENODATA and you
> have to
> + * retry when the next buffer is queued.
> + */
> +if (ret == -ENODATA) {
> +dprintk(1, "qbuf: not enough buffers, retry when more
> buffers are
> queued.\n");
> +q->retry_start_streaming = true;
retry_start_streaming is an unsigned int, should you use 1 and 0 here ?
> +return 0;
> +}
> +if (ret)
> +dprintk(1, "qbuf: driver refused to start streaming\n");
> +else
> +q->retry_start_streaming = false;
> +return ret;
> +}
> +
> static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer
> *b)
> {
> int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> @@ -1377,6 +1410,12 @@ static int vb2_internal_qbuf(struct vb2_queue
> *q,
> struct v4l2_buffer *b) /* Fill buffer information for the userspace */
> __fill_v4l2_buffer(vb, b);
>
> +if (q->retry_start_streaming) {
> +ret = vb2_start_streaming(q);
> +if (ret)
> +return ret;
> +}
> +
> dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb-
> v4l2_buf.index);
> return 0;
> }
>
> @@ -1526,7 +1565,8 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
> return -EINVAL;
> }
>
> -wait_event(q->done_wq, !atomic_read(&q->queued_count));
> +if (!q->retry_start_streaming)
> +wait_event(q->done_wq, !atomic_read(&q->queued_count));
> retu
Re: [PATCH V2] media: i2c: Add ADV761X support
On 11/27/2013 12:21 PM, Hans Verkuil wrote:
On 11/26/2013 10:28 PM, Valentine wrote:
On 11/20/2013 07:53 PM, Valentine wrote:
On 11/20/2013 07:42 PM, Hans Verkuil wrote:
Hi Valentine,
Hi Hans,
Hi Hans,
Did you ever look at this adv7611 driver:
https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12
No, I missed that one somehow, although I did search for the adv7611/7612
before implementing this one.
I'm going to look closer at the patch and test it.
I've tried the patch and I doubt that it was ever tested on adv7611.
I haven't been able to make it work so far. Here's the description of some of
the issues
I've encountered.
The patch does not apply cleanly so I had to make small adjustments just to
make it apply
without changing the functionality.
First of all the driver (adv7604_dummy_client function) does not set default
I2C slave addresses
in the I/O map in case they are not set in the platform data.
This is not needed for 7604, since the default addresses are already set in the
I/O map after chip reset.
However, the map is zeroed on 7611/7612 after power up, and we always have to
set it manually.
So, the platform data for the 7611/2 should always give i2c addresses. That
seems
reasonable.
Yes, but currently the comment in include/media/adv7604.h says
"i2c addresses: 0 == use default", and this is true for 7604, but
it doesn't work for 7611.
Probably the recommended value from the docs should be set by
the driver in case an I2C address is zero in the platform data.
This will help us to keep the same approach across all 76xx chips.
I had to implement the IRQ handler since the soc_camera model does not use
interrupt_service_routine subdevice callback and R-Car VIN knows nothing about
adv7612
interrupt routed to a GPIO pin.
So I had to schedule a workqueue and call adv7604_isr from there in case an
interrupt happens.
For our systems the adv7604 interrupts is not always hooked up to a gpio irq,
instead
a register has to be read to figure out which device actually produced the irq.
So I
want to keep the interrupt_service_routine(). However, adding a gpio field to
the
platform_data that, if set, will tell the driver to request an irq and setup a
workqueue that calls interrupt_service_routine() would be fine with me. That
will
benefit a lot of people since using gpios is much more common.
The driver enables multiple interrupts on the chip, however, the adv7604_isr
callback doesn't
seem to handle them correctly.
According to the docs:
"If an interrupt event occurs, and then a second interrupt event occurs before
the system controller
has cleared or masked the first interrupt event, the ADV7611 does not generate a
second interrupt signal."
However, the interrupt_service_routine doesn't account for that.
For example, in case fmt_change interrupt happens while fmt_change_digital
interrupt is being
processed by the adv7604_isr routine. If fmt_change status is set just before
we clear fmt_change_digital,
we never clear fmt_change. Thus, we end up with fmt_change interrupt missed and
therefore further interrupts disabled.
I've tried to call the adv7604_isr routine in a loop and return from the
worlqueue only when all interrupt status bits are cleared.
This did help a bit, but sometimes I started getting lots of I2C read/write
errors for some reason.
I'm not sure if there is much that can be done about this. The code reads the
interrupt status, then clears the interrupts right after. There is always a
race condition there since this isn't atomic ('read and clear'). Unless
Lars-Peter
has a better idea?
What can be improved, though, is to clear not just the interrupts that were
read, but all the interrupts that are unmasked. You are right, you could
loose an interrupt that way.
I'm also not sure how the dv_timing API should be used.
The internal adv7604 state->timings structure is used when getting mbus format.
However, the driver does not set the structure neither at start-up nor in the
interrupt service callback when format changes.
Is it supposed to be set by the upper level camera driver?
It would be nice if the adv7604 would set up an initial timings format. In our
case it is indeed the bridge driver that sets it up, but in the general case it
is better if the i2c driver also sets an initial timings struct. 720p60 is
generally a good initial value.
The irq certainly shouldn't change timings: changing timings will most likely
require changes in the video buffer sizes, which generally requires stopping
streaming, reconfiguring the pipeline and restarting streaming. That's not
something the i2c driver can do.
The confusion you have with mbus vs dv_timings is that soc_camera lacks
dv_timings
support. It was designed for sensors, although there is now some support for
SDTV
receivers (s/g_std ioctls), but dv_timings support has to be added there as well
along the lines of what is done for s/g_std. Basically it is just a passt
RFC: add FMT_CHANGE event and VIDIOC_G/S_EDID ioctls
This RFC addresses some HDTV-related features that are missing in the API.
The reason they were missing is that there were no bridge drivers in the
kernel that needed them, but with the work done on soc_camera + adv7611/2
by Valentine this is now really needed.
The two missing pieces are how to inform the user that the format of an
input has changed, and how to get/set the EDID for simple pipelines
(i.e. one video node maps to one video receiver sub-device).
How does it work today: the subdev driver sends out a notification to
the bridge driver using v4l2_subdev_notify and a driver-specific notification
ID (see e.g. include/media/adv7604.h, ADV7604_FMT_CHANGE). This notification
needs to be standardized if this is to work for generic drivers like soc-camera.
When the bridge driver is notified it will pass it on as an event to the
application. This event needs to be standardized as well.
Note: there is also a notification if the hotplug pin needs to be pulled
up or down. Some receivers don't do that themselves, but rely on the
SoC to do that for them (usually through a gpio pin). The notification
for that should be standardized as well.
One question regarding the FMT_CHANGE event is if it should contain a
payload such as whether there is a video signal or not. Currently there
is no payload. I do not think a payload is useful. You do not know when
the application will finally dequeue the event, so any data you pass in
as payload might well be out of date. It is better to let the application
read the latest status directly.
The other issue is with setting and getting the EDID. There is an API to
set this through the v4l-subdev device node, which works fine, but it
is a hassle if you have just a simple pipeline and you want to avoid
having to open a v4l-subdev node just for the EDID. If you have a simple
pipeline, then it is unambiguous for which sub-device you set the EDID.
My proposal is the following:
Add two standard notifications to media/v4l2-subdev.h:
#define V4L2_SUBDEV_NOTIFY_HOTPLUG _IO('v', 0)
#define V4L2_SUBDEV_NOTIFY_FMT_CHANGE _IO('v', 1)
and switch adv7604 and adv7842 to use those new notifications.
Add a new event in videodev2.h:
#define V4L2_EVENT_FMT_CHANGE 5
and document it. When sent, the application should call QUERYSTD or
QUERY_DV_TIMINGS to find out the new format that is received.
For the EDID handling I propose to move the struct v4l2_subdev_edid
and VIDIOC_SUBDEV_G/S_EDID ioctl defines from v4l2-subdev.h to videodev2.h
and rename them to struct v4l2_edid and VIDIOC_G/S_EDID. The contents
remains the same, just the names change.
Currently there are no bridge drivers in the kernel that use these
ioctls, so I personally have no problem renaming it. It is possible
to add "#define v4l2_subdev_edid v4l2_edid" to v4l2-subdev.h (and
ditto for the ioctls) to, for the time being, keep backwards
compatibility. You can use the VIDIOC_G/S_EDID either through the
v4l-subdevX nodes or through the videoX nodes.
These additions would make it quite easy to support HDTV in soc-camera
and other bridge drivers in a standardized manner.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] v4l: sh_vou: Fix warnings due to improper casts and printk formats
Use the %zu printk specifier to print size_t variables, and cast
pointers to uintptr_t instead of unsigned int where applicable. This
fixes warnings on platforms where pointers and/or dma_addr_t have a
different size than int.
Cc: Guennadi Liakhovetski
Cc: Mauro Carvalho Chehab
Cc: linux-media@vger.kernel.org
Signed-off-by: Laurent Pinchart
---
drivers/media/platform/sh_vou.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Changes compared to v1:
- Cast to uintptr_t instead of unsigned long
diff --git a/drivers/media/platform/sh_vou.c b/drivers/media/platform/sh_vou.c
index 4f30341..69e2125 100644
--- a/drivers/media/platform/sh_vou.c
+++ b/drivers/media/platform/sh_vou.c
@@ -286,7 +286,7 @@ static int sh_vou_buf_prepare(struct videobuf_queue *vq,
vb->size = vb->height * bytes_per_line;
if (vb->baddr && vb->bsize < vb->size) {
/* User buffer too small */
- dev_warn(vq->dev, "User buffer too small: [%u] @ %lx\n",
+ dev_warn(vq->dev, "User buffer too small: [%zu] @ %lx\n",
vb->bsize, vb->baddr);
return -EINVAL;
}
@@ -302,9 +302,9 @@ static int sh_vou_buf_prepare(struct videobuf_queue *vq,
}
dev_dbg(vou_dev->v4l2_dev.dev,
- "%s(): fmt #%d, %u bytes per line, phys 0x%x, type %d, state
%d\n",
+ "%s(): fmt #%d, %u bytes per line, phys 0x%lx, type %d, state
%d\n",
__func__, vou_dev->pix_idx, bytes_per_line,
- videobuf_to_dma_contig(vb), vb->memory, vb->state);
+ (uintptr_t)videobuf_to_dma_contig(vb), vb->memory, vb->state);
return 0;
}
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
[...]
>> I had to implement the IRQ handler since the soc_camera model does not use
>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing
>> about adv7612
>> interrupt routed to a GPIO pin.
>> So I had to schedule a workqueue and call adv7604_isr from there in case an
>> interrupt happens.
>
> For our systems the adv7604 interrupts is not always hooked up to a gpio irq,
> instead
> a register has to be read to figure out which device actually produced the
> irq. So I
> want to keep the interrupt_service_routine(). However, adding a gpio field to
> the
> platform_data that, if set, will tell the driver to request an irq and setup a
> workqueue that calls interrupt_service_routine() would be fine with me. That
> will
> benefit a lot of people since using gpios is much more common.
I'll look into adding that since I'm using a GPIO for the interrupt on my
platform as well.
>
>> The driver enables multiple interrupts on the chip, however, the adv7604_isr
>> callback doesn't
>> seem to handle them correctly.
>> According to the docs:
>> "If an interrupt event occurs, and then a second interrupt event occurs
>> before the system controller
>> has cleared or masked the first interrupt event, the ADV7611 does not
>> generate a second interrupt signal."
>>
>> However, the interrupt_service_routine doesn't account for that.
>> For example, in case fmt_change interrupt happens while fmt_change_digital
>> interrupt is being
>> processed by the adv7604_isr routine. If fmt_change status is set just
>> before we clear fmt_change_digital,
>> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed
>> and therefore further interrupts disabled.
>> I've tried to call the adv7604_isr routine in a loop and return from the
>> worlqueue only when all interrupt status bits are cleared.
>> This did help a bit, but sometimes I started getting lots of I2C read/write
>> errors for some reason.
>
> I'm not sure if there is much that can be done about this. The code reads the
> interrupt status, then clears the interrupts right after. There is always a
> race condition there since this isn't atomic ('read and clear'). Unless
> Lars-Peter
> has a better idea?
>
As far as I understand it the interrupts lines are level triggered and will
stay asserted as long as there are unmasked, non-acked IRQS. So the
interrupt handler should be re-entered if there are still pending
interrupts. Is it possible that you setup a edge triggered interrupt, in
that case the handler wouldn't be reentered if the signal stays asserted?
But that's just my understanding from the manual, I'll have to verify
whether the hardware does indeed work like that.
- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/8] vb2: push the mmap semaphore down to __buf_prepare()
Hi Hans, On Wednesday 27 November 2013 08:12:24 Hans Verkuil wrote: > On 11/26/2013 04:42 PM, Laurent Pinchart wrote: > > On Friday 22 November 2013 10:02:49 Hans Verkuil wrote: > >> On 11/21/2013 08:04 PM, Laurent Pinchart wrote: > >>> On Thursday 21 November 2013 16:21:59 Hans Verkuil wrote: > From: Hans Verkuil > > Rather than taking the mmap semaphore at a relatively high-level > function, push it down to the place where it is really needed. > > It was placed in vb2_queue_or_prepare_buf() to prevent racing with > other vb2 calls, however, I see no way that any race can happen. > >>> > >>> What about the following scenario ? Both QBUF calls are performed on the > >>> same buffer. > >>> > >>> CPU 0 CPU 1 > >>> - > >>> QBUFQBUF > >>> locks the queue mutex waits for the > >>> queue mutex > >>> vb2_qbuf > >>> vb2_queue_or_prepare_buf > >>> __vb2_qbuf > >>> checks vb->state, calls > >>> __buf_prepare > >>> call_qop(q, wait_prepare, q); > >>> unlocks the queue mutex > >>> > >>> locks the queue mutex > >>> vb2_qbuf > >>> > >>> vb2_queue_or_prepare_buf > >>> > >>> __vb2_qbuf > >>> > >>> checks vb->state, calls > >>> > >>> __buf_prepare > >>> > >>> call_qop(q, wait_prepare, q); > >>> > >>> unlocks the queue mutex > >>> queue > >>> the buffer, set buffer > >>>state > >>> to queue > >>> queue the buffer, set buffer > >>>state to queue > >>> > >>> We would thus end up queueing the buffer twice. The vb->state check > >>> needs to be performed after the brief release of the queue mutex. > >> > >> Good point, I hadn't thought about that scenario. However, using mmap_sem > >> to introduce a large critical section just to protect against state > >> changes is IMHO not the right approach. Why not introduce a > >> VB2_BUF_STATE_PREPARING state? > > > > Note that we use the queue mutex to do so, not mmap_sem. The problem is > > that we can't release the queue mutex in the middle of a critical section > > without risking being preempted by another task. Introducing a new state > > might be possible if it effectively breaks the critical section in two > > independent parts. > > > >> That's set at the start of __buf_prepare while the queue mutex is still > >> held, and which prevents other threads of queuing the same buffer again. > >> If the prepare fails, then the state is reverted back to DEQUEUED. > >> > >> __fill_v4l2_buffer() will handle the PREPARING state as if it was the > >> DEQUEUED state. > >> > >> What do you think? > > > > I'll have to review that in details given the potential complexity of > > locking issues :-) I'm not opposed to the idea, if it works I believe we > > should do it. > > Do you want to think about this first, or shall I make a new patch that you > can then review? As the devil is in the details I'd prefer a patch. I would have to write one to think about this anyway :-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
On 11/26/2013 10:28 PM, Valentine wrote:
> On 11/20/2013 07:53 PM, Valentine wrote:
>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>> Hi Valentine,
>
> Hi Hans,
>
>>>
>>> Did you ever look at this adv7611 driver:
>>>
>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12
>>
>> No, I missed that one somehow, although I did search for the adv7611/7612
>> before implementing this one.
>> I'm going to look closer at the patch and test it.
>>
>
> I've tried the patch and I doubt that it was ever tested on adv7611.
> I haven't been able to make it work so far. Here's the description of some of
> the issues
> I've encountered.
>
> The patch does not apply cleanly so I had to make small adjustments just to
> make it apply
> without changing the functionality.
>
> First of all the driver (adv7604_dummy_client function) does not set default
> I2C slave addresses
> in the I/O map in case they are not set in the platform data.
> This is not needed for 7604, since the default addresses are already set in
> the I/O map after chip reset.
> However, the map is zeroed on 7611/7612 after power up, and we always have to
> set it manually.
So, the platform data for the 7611/2 should always give i2c addresses. That
seems
reasonable.
> I had to implement the IRQ handler since the soc_camera model does not use
> interrupt_service_routine subdevice callback and R-Car VIN knows nothing
> about adv7612
> interrupt routed to a GPIO pin.
> So I had to schedule a workqueue and call adv7604_isr from there in case an
> interrupt happens.
For our systems the adv7604 interrupts is not always hooked up to a gpio irq,
instead
a register has to be read to figure out which device actually produced the irq.
So I
want to keep the interrupt_service_routine(). However, adding a gpio field to
the
platform_data that, if set, will tell the driver to request an irq and setup a
workqueue that calls interrupt_service_routine() would be fine with me. That
will
benefit a lot of people since using gpios is much more common.
> The driver enables multiple interrupts on the chip, however, the adv7604_isr
> callback doesn't
> seem to handle them correctly.
> According to the docs:
> "If an interrupt event occurs, and then a second interrupt event occurs
> before the system controller
> has cleared or masked the first interrupt event, the ADV7611 does not
> generate a second interrupt signal."
>
> However, the interrupt_service_routine doesn't account for that.
> For example, in case fmt_change interrupt happens while fmt_change_digital
> interrupt is being
> processed by the adv7604_isr routine. If fmt_change status is set just before
> we clear fmt_change_digital,
> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed
> and therefore further interrupts disabled.
> I've tried to call the adv7604_isr routine in a loop and return from the
> worlqueue only when all interrupt status bits are cleared.
> This did help a bit, but sometimes I started getting lots of I2C read/write
> errors for some reason.
I'm not sure if there is much that can be done about this. The code reads the
interrupt status, then clears the interrupts right after. There is always a
race condition there since this isn't atomic ('read and clear'). Unless
Lars-Peter
has a better idea?
What can be improved, though, is to clear not just the interrupts that were
read, but all the interrupts that are unmasked. You are right, you could
loose an interrupt that way.
> I'm also not sure how the dv_timing API should be used.
> The internal adv7604 state->timings structure is used when getting mbus
> format.
> However, the driver does not set the structure neither at start-up nor in the
> interrupt service callback when format changes.
> Is it supposed to be set by the upper level camera driver?
It would be nice if the adv7604 would set up an initial timings format. In our
case it is indeed the bridge driver that sets it up, but in the general case it
is better if the i2c driver also sets an initial timings struct. 720p60 is
generally a good initial value.
The irq certainly shouldn't change timings: changing timings will most likely
require changes in the video buffer sizes, which generally requires stopping
streaming, reconfiguring the pipeline and restarting streaming. That's not
something the i2c driver can do.
The confusion you have with mbus vs dv_timings is that soc_camera lacks
dv_timings
support. It was designed for sensors, although there is now some support for
SDTV
receivers (s/g_std ioctls), but dv_timings support has to be added there as well
along the lines of what is done for s/g_std. Basically it is just a passthrough.
The way s_mbus_fmt is defined in adv7604 today is correct. s_dv_timings should
be
called to change the format, s_mbus_fmt should just return the current
width/height.
For HDTV there is more involved than just width and height when changing
formats,
just as SDTV.
So the r
