af9013

GIT PULL Antti Palosaari 2 participants 5 messages Nov 30, 2011 – Dec 11, 2011 Media (V4L/DVB)

Share & tools

Antti Palosaari

author

2011-11-30 17:25 UTC #0 lore raw

Morjens Mauro,

I rewrote whole af9013 demodulator driver in order to decrease I2C load. Please pull that to the next Kernel merge window.

Antti

The following changes since commit a235af24a74a0fa03ece0a9f5e28a72e4d1e2cad:

ce168: remove experimental from Kconfig (2011-11-19 23:07:54 +0200)

are available in the git repository at:
git://linuxtv.org/anttip/media_tree.git misc

Antti Palosaari (1):
af9013: rewrite whole driver

 drivers/media/dvb/dvb-usb/af9015.c        |   82 +-
 drivers/media/dvb/frontends/af9013.c      | 1756 
++++++++++++++---------------
 drivers/media/dvb/frontends/af9013.h      |  113 +-
 drivers/media/dvb/frontends/af9013_priv.h |   93 +-
 4 files changed, 1017 insertions(+), 1027 deletions(-)

Signature · 1 line

http://palosaari.fi/

Mauro Carvalho Chehab

2011-12-11 10:59 UTC #1 lore raw

Quoted 13 lines · Antti Palosaari

Morjens Mauro,
I rewrote whole af9013 demodulator driver in order to decrease I2C load. Please pull that to the next Kernel merge window.
Antti
The following changes since commit a235af24a74a0fa03ece0a9f5e28a72e4d1e2cad:
ce168: remove experimental from Kconfig (2011-11-19 23:07:54 +0200)
are available in the git repository at:
git://linuxtv.org/anttip/media_tree.git misc

Quoted 9 lines

Antti Palosaari (1):
af9013: rewrite whole driver
drivers/media/dvb/dvb-usb/af9015.c | 82 +- drivers/media/dvb/frontends/af9013.c | 1756 ++++++++++++++--------------- drivers/media/dvb/frontends/af9013.h | 113 +- drivers/media/dvb/frontends/af9013_priv.h | 93 +-
4 files changed, 1017 insertions(+), 1027 deletions(-)

There was a minor context change here:

@@ -1156,7 +1158,7 @@ static int af9015_af9013_sleep(struct dvb_frontend *fe)
if (mutex_lock_interruptible(&adap->dev->usb_mutex))
return -EAGAIN;

- ret = priv->init[adap->id](fe);
+ ret = priv->sleep[adap->id](fe);

Basically, the current code doesn't have that mutex_lock_interruptible logic. It may be into the fixes we'll send for 3.2.

However, after this patch, compilation broke:

drivers/media/dvb/dvb-usb/af9015.c: In function ‘af9015_rc_query’:
drivers/media/dvb/dvb-usb/af9015.c:1089:12: error: ‘struct af9015_state’ has no member named ‘sleep’ drivers/media/dvb/dvb-usb/af9015.c:1089:20: error: ‘adap’ undeclared (first use in this function) drivers/media/dvb/dvb-usb/af9015.c:1089:20: note: each undeclared identifier is reported only once for each function it appears in drivers/media/dvb/dvb-usb/af9015.c:1089:30: error: ‘fe’ undeclared (first use in this function)

Regards,
Mauro

Antti Palosaari

author

2011-12-11 15:34 UTC #2 lore raw

Quoted 29 lines · Mauro Carvalho Chehab

Quoted 15 lines · Antti Palosaari
Morjens Mauro,
I rewrote whole af9013 demodulator driver in order to decrease I2C load. Please pull that to the next Kernel merge window.
Antti
The following changes since commit
a235af24a74a0fa03ece0a9f5e28a72e4d1e2cad:
ce168: remove experimental from Kconfig (2011-11-19 23:07:54 +0200)
are available in the git repository at:
git://linuxtv.org/anttip/media_tree.git misc
Quoted 9 lines
Antti Palosaari (1):
af9013: rewrite whole driver
drivers/media/dvb/dvb-usb/af9015.c | 82 +- drivers/media/dvb/frontends/af9013.c | 1756 ++++++++++++++--------------- drivers/media/dvb/frontends/af9013.h | 113 +- drivers/media/dvb/frontends/af9013_priv.h | 93 +-
4 files changed, 1017 insertions(+), 1027 deletions(-)
There was a minor context change here:

Review static int af9015_af9013_sleep(struct dvb_frontend · line 1162

Quoted 6 lines

(quoted diff) +1 -1

@@ -1156,7 +1158,7 @@ static int af9015_af9013_sleep(struct dvb_frontend
 *fe)
 if (mutex_lock_interruptible(&adap->dev->usb_mutex))
 return -EAGAIN;
 
- ret = priv->init[adap->id](fe);
+ ret = priv->sleep[adap->id](fe);

Correct, that is bug fix for the earlier patch I made. As a result we call demod .init() in case we should call .sleep() resulting demod will never sleep. I found that very late phase, when af9013 rewrite was almost complete. At that time I was too lazy at that point to made separate patch for fixing it because I had made so many changes for af9015 already.

But if you like, I can rebase whole thing and move that fix as own patch.

Quoted 3 lines

Basically, the current code doesn't have that mutex_lock_interruptible logic. It
may be into the fixes we'll send for 3.2.

Do you mean I should change all mutex_lock_interruptible => mutex_lock and send as bugfix to 3.2? Anyway, I asked you to push those mutex_lock_interruptible things to 3.3 and I think you haven't send those 3.2 so not fixes for 3.2 hopefully.

There is one old mutex_lock_interruptible inside af9015_rw_udev(). It have been there many years and it is copied from dvb_usb_generic_rw(). I think better to not change it as bugfix.

It is not even clear for me when to use mutex_lock_interruptible or mutex_lock. I suspect mutex_lock is cheaper and that's why it should be used when possible? I am happy to hear reasons and learn (and too lazy to look through docs and codes...).

Quoted 11 lines

However, after this patch, compilation broke:
drivers/media/dvb/dvb-usb/af9015.c: In function ‘af9015_rc_query’:
drivers/media/dvb/dvb-usb/af9015.c:1089:12: error: ‘struct af9015_state’ has no member named ‘sleep’ drivers/media/dvb/dvb-usb/af9015.c:1089:20: error: ‘adap’ undeclared (first use in this function) drivers/media/dvb/dvb-usb/af9015.c:1089:20: note: each undeclared identifier is reported only once for each function it appears in drivers/media/dvb/dvb-usb/af9015.c:1089:30: error: ‘fe’ undeclared (first use in this function)

Arg, how the I hell I missed af9015.h file from that change-set.

After all, I hope it was not TLDR :) What you think I should made in order to fix these issues correctly?

regards
Antti

Signature · 1 line

http://palosaari.fi/

Mauro Carvalho Chehab

2011-12-11 20:30 UTC #3 lore raw

Quoted 39 lines · Antti Palosaari

Quoted 29 lines · Mauro Carvalho Chehab
Quoted 15 lines · Antti Palosaari
Morjens Mauro,
I rewrote whole af9013 demodulator driver in order to decrease I2C load. Please pull that to the next Kernel merge window.
Antti
The following changes since commit
a235af24a74a0fa03ece0a9f5e28a72e4d1e2cad:
ce168: remove experimental from Kconfig (2011-11-19 23:07:54 +0200)
are available in the git repository at:
git://linuxtv.org/anttip/media_tree.git misc
Quoted 9 lines
Antti Palosaari (1):
af9013: rewrite whole driver
drivers/media/dvb/dvb-usb/af9015.c | 82 +- drivers/media/dvb/frontends/af9013.c | 1756 ++++++++++++++--------------- drivers/media/dvb/frontends/af9013.h | 113 +- drivers/media/dvb/frontends/af9013_priv.h | 93 +-
4 files changed, 1017 insertions(+), 1027 deletions(-)
There was a minor context change here:
Review static int af9015_af9013_sleep(struct dvb_frontend · line 1162
Quoted 6 lines
(quoted diff) +1 -1
@@ -1156,7 +1158,7 @@ static int af9015_af9013_sleep(struct dvb_frontend
 *fe)
 if (mutex_lock_interruptible(&adap->dev->usb_mutex))
 return -EAGAIN;
 
- ret = priv->init[adap->id](fe);
+ ret = priv->sleep[adap->id](fe);
Correct, that is bug fix for the earlier patch I made. As a result we call demod .init() in case we should call .sleep() resulting demod will never sleep. I found that very late phase, when af9013 rewrite was almost complete. At that time I was too lazy at that point to made separate patch for fixing it because I had made so many changes for af9015 already.

Quoted 7 lines

But if you like, I can rebase whole thing and move that fix as own patch.
Quoted 3 lines
Basically, the current code doesn't have that mutex_lock_interruptible logic. It
may be into the fixes we'll send for 3.2.
Do you mean I should change all mutex_lock_interruptible => mutex_lock and send as bugfix to 3.2?

No. I just meant to say that the current code (without the patches I'm preparing for -rc6) doesn't have the mutex_lock_interruptible() at the context.

Quoted 1 line

Anyway, I asked you to push those mutex_lock_interruptible things to 3.3 and I think you haven't send those 3.2 so not fixes for 3.2 hopefully.

Need to check on my linux-media tree, in order to be sure if those are there or not. Maybe I've merged it as a bug fix for 3.2.

Quoted 4 lines

There is one old mutex_lock_interruptible inside af9015_rw_udev(). It have been there many years and it is copied from dvb_usb_generic_rw(). I think better to not change it as bugfix.
It is not even clear for me when to use mutex_lock_interruptible or mutex_lock. I suspect mutex_lock is cheaper and that's why it should be used when possible? I am happy to hear reasons and learn (and too lazy to look through docs and codes...).

The mutex_lock_interruptible will return -EINTR if a signal is received while trying to handle it. Userspace application needs to be prepared for that.

Both are handled by the same routine at kernel/mutex.c.

Quoted 12 lines

Quoted 11 lines
However, after this patch, compilation broke:
drivers/media/dvb/dvb-usb/af9015.c: In function ‘af9015_rc_query’:
drivers/media/dvb/dvb-usb/af9015.c:1089:12: error: ‘struct af9015_state’ has no member named ‘sleep’ drivers/media/dvb/dvb-usb/af9015.c:1089:20: error: ‘adap’ undeclared (first use in this function) drivers/media/dvb/dvb-usb/af9015.c:1089:20: note: each undeclared identifier is reported only once for each function it appears in drivers/media/dvb/dvb-usb/af9015.c:1089:30: error: ‘fe’ undeclared (first use in this function)

Quoted 3 lines

Arg, how the I hell I missed af9015.h file from that change-set.
After all, I hope it was not TLDR :) What you think I should made in order to fix these issues correctly?

Rebase the patch ;) compilation errors break git bisect.

Quoted 3 lines

regards
Antti

Antti Palosaari

author

2011-12-11 21:03 UTC #4 lore raw

Quoted 94 lines · Mauro Carvalho Chehab

Quoted 45 lines · Antti Palosaari
Quoted 30 lines · Mauro Carvalho Chehab
Quoted 18 lines · Antti Palosaari
Morjens Mauro,
I rewrote whole af9013 demodulator driver in order to decrease I2C load. Please pull that to the next Kernel merge window.
Antti
The following changes since commit
a235af24a74a0fa03ece0a9f5e28a72e4d1e2cad:
ce168: remove experimental from Kconfig (2011-11-19 23:07:54 +0200)
are available in the git repository at:
git://linuxtv.org/anttip/media_tree.git misc
Antti Palosaari (1):
af9013: rewrite whole driver
Quoted 7 lines
drivers/media/dvb/dvb-usb/af9015.c | 82 +-
drivers/media/dvb/frontends/af9013.c | 1756
++++++++++++++---------------
drivers/media/dvb/frontends/af9013.h | 113 +-
drivers/media/dvb/frontends/af9013_priv.h | 93 +-
4 files changed, 1017 insertions(+), 1027 deletions(-)
There was a minor context change here:
Review static int af9015_af9013_sleep(struct dvb_frontend · line 1162
Quoted 6 lines
(quoted diff) +1 -1
@@ -1156,7 +1158,7 @@ static int af9015_af9013_sleep(struct dvb_frontend
 *fe)
 if (mutex_lock_interruptible(&adap->dev->usb_mutex))
 return -EAGAIN;
 
- ret = priv->init[adap->id](fe);
+ ret = priv->sleep[adap->id](fe);
Correct, that is bug fix for the earlier patch I made. As a result we call demod .init() in case we should call .sleep() resulting demod will never sleep. I found that very late phase, when af9013 rewrite was almost complete. At that time I was too lazy at that point to made separate patch for fixing it because I had made so many changes for af9015 already.
Quoted 8 lines
But if you like, I can rebase whole thing and move that fix as own patch.
Quoted 3 lines
Basically, the current code doesn't have that mutex_lock_interruptible logic. It
may be into the fixes we'll send for 3.2.
Do you mean I should change all mutex_lock_interruptible => mutex_lock and send as bugfix to 3.2?
No. I just meant to say that the current code (without the patches I'm preparing for -rc6) doesn't
have the mutex_lock_interruptible() at the context.
Quoted 3 lines
Anyway, I asked you to push those mutex_lock_interruptible things to 3.3 and I think you haven't send those 3.2 so not fixes for 3.2 hopefully.
Need to check on my linux-media tree, in order to be sure if those are there or not. Maybe I've
merged it as a bug fix for 3.2.
Quoted 9 lines
There is one old mutex_lock_interruptible inside af9015_rw_udev(). It have been there many years and it is copied from dvb_usb_generic_rw(). I think better to not change it as bugfix.
It is not even clear for me when to use mutex_lock_interruptible or mutex_lock. I suspect mutex_lock is cheaper and that's why it should be used when possible? I am happy to hear reasons and learn (and too lazy to look through docs and codes...).
The mutex_lock_interruptible will return -EINTR if a signal is received while
trying to handle it. Userspace application needs to be prepared for that.
Both are handled by the same routine at kernel/mutex.c.
Quoted 12 lines
Quoted 11 lines
However, after this patch, compilation broke:
drivers/media/dvb/dvb-usb/af9015.c: In function ‘af9015_rc_query’:
drivers/media/dvb/dvb-usb/af9015.c:1089:12: error: ‘struct af9015_state’ has no member named ‘sleep’ drivers/media/dvb/dvb-usb/af9015.c:1089:20: error: ‘adap’ undeclared (first use in this function) drivers/media/dvb/dvb-usb/af9015.c:1089:20: note: each undeclared identifier is reported only once for each function it appears in drivers/media/dvb/dvb-usb/af9015.c:1089:30: error: ‘fe’ undeclared (first use in this function)

Quoted 6 lines

Quoted 4 lines
Arg, how the I hell I missed af9015.h file from that change-set.
After all, I hope it was not TLDR :) What you think I should made in order to fix these issues correctly?
Rebase the patch ;) compilation errors break git bisect.

I suspect there is now something else wrong. Two person have reported that patch is working and no mention about compile errors you have.

So you have missing now some af9015 patches I have sent earlier. There patches are needed before that last one can be applied:
http://patchwork.linuxtv.org/patch/8410/
http://patchwork.linuxtv.org/patch/8411/

If you PULL all those starting from:
2011-11-19 af9015: limit I2C access to keep FW happy
http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/misc

you will get all what is needed. Some of those are applied but some not.

regards
Antti

Signature · 1 line

http://palosaari.fi/