tda18271 driver power consumption

PATCH In review Antti Palosaari 5 participants 36 messages Jul 22, 2012 – Oct 7, 2012 2 messages not in the archive

Subsystems (2)

Media (V4L/DVB) TDA18271 media driver

lore

Share & tools

lore

Antti Palosaari

author

2012-07-22 19:59 UTC #0 lore raw

Moi Michael,
I just realized tda18271 driver eats 160mA too much current after attach. This means, there is power management bug.

When I plug my nanoStick it eats total 240mA, after tda18271 sleep is called it eats only 80mA total which is reasonable. If I use Digital Devices tda18271c2dd driver it is total 110mA after attach, which is also quite OK.

regards
Antti

Signature · 1 line

http://palosaari.fi/

Michael Krufky

maintainer

2012-07-24 21:55 UTC #1 lore raw

Quoted 7 lines · Antti Palosaari

Moi Michael,
I just realized tda18271 driver eats 160mA too much current after attach. This means, there is power management bug.
When I plug my nanoStick it eats total 240mA, after tda18271 sleep is called it eats only 80mA total which is reasonable. If I use Digital Devices tda18271c2dd driver it is total 110mA after attach, which is also quite OK.

Thanks for the report -- I will take a look at it.

...patches are welcome, of course :-)

-Mike

Antti Palosaari

author

2012-07-24 22:12 UTC #2 lore raw

Quoted 5 lines · Michael Krufky

Quoted 4 lines · Antti Palosaari
Moi Michael,
I just realized tda18271 driver eats 160mA too much current after attach. This means, there is power management bug.

Quoted 7 lines

Quoted 3 lines
When I plug my nanoStick it eats total 240mA, after tda18271 sleep is called it eats only 80mA total which is reasonable. If I use Digital Devices tda18271c2dd driver it is total 110mA after attach, which is also quite OK.
Thanks for the report -- I will take a look at it.
...patches are welcome, of course :-)

I suspect it does some tweaking on attach() and chip leaves powered (I saw demod debugs at calls I2C-gate control quite many times thus this suspicion). When chip is powered-up it is usually in some sleep state by default. Also, on attach() there should be no I/O unless very good reason. For example chip ID is allowed to read and download firmware in case it is really needed to continue - like for tuner communication.

What I found quickly testing few DVB USB sticks there seems to be very much power management problems... I am now waiting for new multimeter in order to make better measurements and likely return fixing these issues later.

regards
Antti

Signature · 1 line

http://palosaari.fi/

Michael Krufky

maintainer

2012-07-24 22:17 UTC #3 lore raw

Quoted 28 lines · Antti Palosaari

Quoted 14 lines · Michael Krufky
Quoted 10 lines · Antti Palosaari
Moi Michael,
I just realized tda18271 driver eats 160mA too much current after attach. This means, there is power management bug.
When I plug my nanoStick it eats total 240mA, after tda18271 sleep is called
it eats only 80mA total which is reasonable. If I use Digital Devices tda18271c2dd driver it is total 110mA after attach, which is also quite OK.
Quoted 3 lines
Thanks for the report -- I will take a look at it.
...patches are welcome, of course :-)
I suspect it does some tweaking on attach() and chip leaves powered (I saw demod debugs at calls I2C-gate control quite many times thus this suspicion). When chip is powered-up it is usually in some sleep state by default. Also, on attach() there should be no I/O unless very good reason. For example chip ID is allowed to read and download firmware in case it is really needed to continue - like for tuner communication.

Quoted 3 lines

What I found quickly testing few DVB USB sticks there seems to be very much power management problems... I am now waiting for new multimeter in order to make better measurements and likely return fixing these issues later.

The driver does some calibration during attach, some of which is a one-time initialization to determine a temperature differential for tune calculation later on, which can take some time on slower USB buses. The "fix" for the power usage issue would just be to make sure to sleep the device before exiting the attach() function.

I'm not looking to remove the calibration from the attach -- this was done on purpose.

-Mike

Michael Krufky

maintainer

2012-07-25 00:15 UTC #4 lore raw

Quoted 33 lines · Michael Krufky

Quoted 28 lines · Antti Palosaari
Quoted 14 lines · Michael Krufky
Quoted 10 lines · Antti Palosaari
Moi Michael,
I just realized tda18271 driver eats 160mA too much current after attach. This means, there is power management bug.
When I plug my nanoStick it eats total 240mA, after tda18271 sleep is called
it eats only 80mA total which is reasonable. If I use Digital Devices tda18271c2dd driver it is total 110mA after attach, which is also quite OK.
Quoted 3 lines
Thanks for the report -- I will take a look at it.
...patches are welcome, of course :-)
I suspect it does some tweaking on attach() and chip leaves powered (I saw demod debugs at calls I2C-gate control quite many times thus this suspicion). When chip is powered-up it is usually in some sleep state by default. Also, on attach() there should be no I/O unless very good reason. For example chip ID is allowed to read and download firmware in case it is really needed to continue - like for tuner communication.
Quoted 3 lines
What I found quickly testing few DVB USB sticks there seems to be very much power management problems... I am now waiting for new multimeter in order to make better measurements and likely return fixing these issues later.

Quoted 9 lines

The driver does some calibration during attach, some of which is a one-time initialization to determine a temperature differential for tune calculation later on, which can take some time on slower USB buses. The "fix" for the power usage issue would just be to make sure to sleep the device before exiting the attach() function.
I'm not looking to remove the calibration from the attach -- this was done on purpose.

Antti,

After looking again, I realize that we are purposefully not sleeping the device before we exit the attach() function.

The tda18271 is commonly found in multi-chip designs that may or may not include an analog demodulator and / or other tda18271 tuners. In such designs, the chips tend to be daisy-chained to each other, using the xtal output and loop-thru features of the tda18271. We set the required features in the attach-time configuration structure. However, we must keep in mind that this is a hybrid tuner chip, and the analog side of the bridge driver may actually come up before the digital side. Since the actual configuration tends to be done in the digital bring-up, the analog side is brought up within tuner.ko using the most generic one-size-fits all configuration, which gets overridden when the digital side initializes.

It is absolutely crucial that if we actually need the xtal output feature enabled, that it must *never* be turned off, otherwise the i2c bus may get wedged unrecoverably. So, we make sure to leave this feature enabled during the attach function, since we don't yet know at that point whether there is another "instance" of this same tuner yet to be initialized. It is not safe to power off that feature until after we are sure that the bridge has completely initialized.

In order to rectify this issue from within your driver, you should call sleep after you complete the attach. For instance, this is what we do in the cx23885 driver:

if (fe0->dvb.frontend->ops.analog_ops.standby)

                 fe0->dvb.frontend->ops.analog_ops.standby(fe0->dvb.frontend);

...except you should call into the tuner_ops->sleep() function instead of analog_demod_ops->standby()

Does this clear things up for you?

Regards,

Mike Krufky

Antti Palosaari

author

2012-07-25 00:43 UTC #5 lore raw

Quoted 73 lines · Michael Krufky

Quoted 33 lines · Michael Krufky
Quoted 28 lines · Antti Palosaari
Quoted 14 lines · Michael Krufky
Quoted 10 lines · Antti Palosaari
Moi Michael,
I just realized tda18271 driver eats 160mA too much current after attach. This means, there is power management bug.
When I plug my nanoStick it eats total 240mA, after tda18271 sleep is called
it eats only 80mA total which is reasonable. If I use Digital Devices tda18271c2dd driver it is total 110mA after attach, which is also quite OK.
Quoted 3 lines
Thanks for the report -- I will take a look at it.
...patches are welcome, of course :-)
I suspect it does some tweaking on attach() and chip leaves powered (I saw demod debugs at calls I2C-gate control quite many times thus this suspicion). When chip is powered-up it is usually in some sleep state by default. Also, on attach() there should be no I/O unless very good reason. For example chip ID is allowed to read and download firmware in case it is really needed to continue - like for tuner communication.
Quoted 3 lines
What I found quickly testing few DVB USB sticks there seems to be very much power management problems... I am now waiting for new multimeter in order to make better measurements and likely return fixing these issues later.
Quoted 9 lines
The driver does some calibration during attach, some of which is a one-time initialization to determine a temperature differential for tune calculation later on, which can take some time on slower USB buses. The "fix" for the power usage issue would just be to make sure to sleep the device before exiting the attach() function.
I'm not looking to remove the calibration from the attach -- this was done on purpose.
Antti,
After looking again, I realize that we are purposefully not sleeping the device before we exit the attach() function.
The tda18271 is commonly found in multi-chip designs that may or may not include an analog demodulator and / or other tda18271 tuners. In such designs, the chips tend to be daisy-chained to each other, using the xtal output and loop-thru features of the tda18271. We set the required features in the attach-time configuration structure. However, we must keep in mind that this is a hybrid tuner chip, and the analog side of the bridge driver may actually come up before the digital side. Since the actual configuration tends to be done in the digital bring-up, the analog side is brought up within tuner.ko using the most generic one-size-fits all configuration, which gets overridden when the digital side initializes.
It is absolutely crucial that if we actually need the xtal output feature enabled, that it must *never* be turned off, otherwise the i2c bus may get wedged unrecoverably. So, we make sure to leave this feature enabled during the attach function, since we don't yet know at that point whether there is another "instance" of this same tuner yet to be initialized. It is not safe to power off that feature until after we are sure that the bridge has completely initialized.
In order to rectify this issue from within your driver, you should call sleep after you complete the attach. For instance, this is what we do in the cx23885 driver:

Quoted 8 lines

if (fe0->dvb.frontend->ops.analog_ops.standby)
                  fe0->dvb.frontend->ops.analog_ops.standby(fe0->dvb.frontend);
...except you should call into the tuner_ops->sleep() function instead of analog_demod_ops->standby()
Does this clear things up for you?

Surely this is possible and it will resolve power drain issue. But it is not nice looking and causes more deviation compared to others.

Could you add configuration option "bool do_not_powerdown_on_attach" ?

I have quite many tda18271 devices here and all those are DVB onlỵ (OK, PCTV 520e is DVB + analog, but analog is not supported). Having configuration parameter sounds like better plan.

regards
Antti

Signature · 1 line

http://palosaari.fi/

Michael Krufky

maintainer

2012-07-26 03:18 UTC #6 lore raw

Quoted 101 lines · Antti Palosaari

Quoted 91 lines · Michael Krufky
Quoted 47 lines · Michael Krufky
Quoted 38 lines · Antti Palosaari
Quoted 19 lines · Michael Krufky
Quoted 7 lines · Antti Palosaari
Moi Michael,
I just realized tda18271 driver eats 160mA too much current after attach.
This means, there is power management bug.
Quoted 6 lines
When I plug my nanoStick it eats total 240mA, after tda18271 sleep is called
it eats only 80mA total which is reasonable. If I use Digital Devices tda18271c2dd driver it is total 110mA after attach, which is also quite
OK.
Quoted 3 lines
Thanks for the report -- I will take a look at it.
...patches are welcome, of course :-)
I suspect it does some tweaking on attach() and chip leaves powered (I saw
demod debugs at calls I2C-gate control quite many times thus this suspicion). When chip is powered-up it is usually in some sleep state by default. Also, on attach() there should be no I/O unless very good reason.
For example chip ID is allowed to read and download firmware in case it is
really needed to continue - like for tuner communication.
Quoted 5 lines
What I found quickly testing few DVB USB sticks there seems to be very much
power management problems... I am now waiting for new multimeter in order to
make better measurements and likely return fixing these issues later.
Quoted 9 lines
The driver does some calibration during attach, some of which is a one-time initialization to determine a temperature differential for tune calculation later on, which can take some time on slower USB buses. The "fix" for the power usage issue would just be to make sure to sleep the device before exiting the attach() function.
I'm not looking to remove the calibration from the attach -- this was done on purpose.
Antti,
After looking again, I realize that we are purposefully not sleeping the device before we exit the attach() function.
The tda18271 is commonly found in multi-chip designs that may or may not include an analog demodulator and / or other tda18271 tuners. In such designs, the chips tend to be daisy-chained to each other, using the xtal output and loop-thru features of the tda18271. We set the required features in the attach-time configuration structure. However, we must keep in mind that this is a hybrid tuner chip, and the analog side of the bridge driver may actually come up before the digital side. Since the actual configuration tends to be done in the digital bring-up, the analog side is brought up within tuner.ko using the most generic one-size-fits all configuration, which gets overridden when the digital side initializes.
It is absolutely crucial that if we actually need the xtal output feature enabled, that it must *never* be turned off, otherwise the i2c bus may get wedged unrecoverably. So, we make sure to leave this feature enabled during the attach function, since we don't yet know at that point whether there is another "instance" of this same tuner yet to be initialized. It is not safe to power off that feature until after we are sure that the bridge has completely initialized.
In order to rectify this issue from within your driver, you should call sleep after you complete the attach. For instance, this is what we do in the cx23885 driver:
if (fe0->dvb.frontend->ops.analog_ops.standby)
Quoted 7 lines
fe0->dvb.frontend->ops.analog_ops.standby(fe0->dvb.frontend);
...except you should call into the tuner_ops->sleep() function instead of analog_demod_ops->standby()
Does this clear things up for you?

Quoted 8 lines

Surely this is possible and it will resolve power drain issue. But it is not nice looking and causes more deviation compared to others.
Could you add configuration option "bool do_not_powerdown_on_attach" ?
I have quite many tda18271 devices here and all those are DVB onlỵ (OK, PCTV 520e is DVB + analog, but analog is not supported). Having configuration parameter sounds like better plan.

Come to think of it, since the generic "one-size-fits-all" configuration leaves the loop thru and xtal output enabled, it should be safe to go to the lowest power level allowed (based on the configuration) at the end of the attach() function. I'll put up a patch within the next few days... Thanks for noticing this, Antti. :-)

We wont need to add any new configuration option :-)

-Mike

Michael Krufky

maintainer

2012-07-26 12:49 UTC #7 lore raw

Quoted 118 lines · Michael Krufky

Quoted 101 lines · Antti Palosaari
Quoted 91 lines · Michael Krufky
Quoted 47 lines · Michael Krufky
Quoted 38 lines · Antti Palosaari
Quoted 19 lines · Michael Krufky
Quoted 7 lines · Antti Palosaari
Moi Michael,
I just realized tda18271 driver eats 160mA too much current after attach.
This means, there is power management bug.
Quoted 6 lines
When I plug my nanoStick it eats total 240mA, after tda18271 sleep is called
it eats only 80mA total which is reasonable. If I use Digital Devices tda18271c2dd driver it is total 110mA after attach, which is also quite
OK.
Quoted 3 lines
Thanks for the report -- I will take a look at it.
...patches are welcome, of course :-)
I suspect it does some tweaking on attach() and chip leaves powered (I saw
demod debugs at calls I2C-gate control quite many times thus this suspicion). When chip is powered-up it is usually in some sleep state by default. Also, on attach() there should be no I/O unless very good reason.
For example chip ID is allowed to read and download firmware in case it is
really needed to continue - like for tuner communication.
Quoted 5 lines
What I found quickly testing few DVB USB sticks there seems to be very much
power management problems... I am now waiting for new multimeter in order to
make better measurements and likely return fixing these issues later.
Quoted 9 lines
The driver does some calibration during attach, some of which is a one-time initialization to determine a temperature differential for tune calculation later on, which can take some time on slower USB buses. The "fix" for the power usage issue would just be to make sure to sleep the device before exiting the attach() function.
I'm not looking to remove the calibration from the attach -- this was done on purpose.
Antti,
After looking again, I realize that we are purposefully not sleeping the device before we exit the attach() function.
The tda18271 is commonly found in multi-chip designs that may or may not include an analog demodulator and / or other tda18271 tuners. In such designs, the chips tend to be daisy-chained to each other, using the xtal output and loop-thru features of the tda18271. We set the required features in the attach-time configuration structure. However, we must keep in mind that this is a hybrid tuner chip, and the analog side of the bridge driver may actually come up before the digital side. Since the actual configuration tends to be done in the digital bring-up, the analog side is brought up within tuner.ko using the most generic one-size-fits all configuration, which gets overridden when the digital side initializes.
It is absolutely crucial that if we actually need the xtal output feature enabled, that it must *never* be turned off, otherwise the i2c bus may get wedged unrecoverably. So, we make sure to leave this feature enabled during the attach function, since we don't yet know at that point whether there is another "instance" of this same tuner yet to be initialized. It is not safe to power off that feature until after we are sure that the bridge has completely initialized.
In order to rectify this issue from within your driver, you should call sleep after you complete the attach. For instance, this is what we do in the cx23885 driver:
if (fe0->dvb.frontend->ops.analog_ops.standby)
Quoted 7 lines
fe0->dvb.frontend->ops.analog_ops.standby(fe0->dvb.frontend);
...except you should call into the tuner_ops->sleep() function instead of analog_demod_ops->standby()
Does this clear things up for you?
Quoted 8 lines
Surely this is possible and it will resolve power drain issue. But it is not nice looking and causes more deviation compared to others.
Could you add configuration option "bool do_not_powerdown_on_attach" ?
I have quite many tda18271 devices here and all those are DVB onlỵ (OK, PCTV 520e is DVB + analog, but analog is not supported). Having configuration parameter sounds like better plan.
Come to think of it, since the generic "one-size-fits-all" configuration leaves the loop thru and xtal output enabled, it should be safe to go to the lowest power level allowed (based on the configuration) at the end of the attach() function. I'll put up a patch within the next few days... Thanks for noticing this, Antti. :-)

Quoted 3 lines

We wont need to add any new configuration option :-)
-Mike

Antti,

This small patch should do the trick -- can you test it?

The following changes since commit 0c7d5a6da75caecc677be1fda207b7578936770d:

Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)

are available in the git repository at:

git://git.linuxtv.org/mkrufky/tuners tda18271

for you to fetch changes up to 782b28e20d3b253d317cc71879639bf3c108b200:

tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)

----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of tda18271_attach()

 drivers/media/common/tuners/tda18271-fe.c |    3 +++
 1 file changed, 3 insertions(+)

Cheers,

Mike

From 782b28e20d3b253d317cc71879639bf3c108b200 Mon Sep 17 00:00:00 2001
From: Michael Krufky <[email protected]>
Date: Thu, 26 Jul 2012 08:34:37 -0400
Subject: [PATCH] tda18271: enter low-power standby mode at the end of
tda18271_attach()

Ensure that unnecessary features are powered down at the end of the attach() function. If the configuration requires the loop thru or xtout features, they will remain enabled.

Thanks to Antti Palosaari for noticing the additional power consumption.

Cc Antti Palosaari <[email protected]>

Signed-off-by Michael Krufky <[email protected]>

---
 drivers/media/common/tuners/tda18271-fe.c |    3 +++
 1 file changed, 3 insertions(+)

drivers/media/common/tuners/tda18271-fe.c +3 -0

index 2e67f44..5f5d866 100644
@@ -1323,6 +1323,9 @@ struct dvb_frontend *tda18271_attach(struct dvb_frontend *fe, u8 addr,
 	if (tda18271_debug & (DBG_MAP | DBG_ADV))
 		tda18271_dump_std_map(fe);
 
+	ret = tda18271_sleep(fe);
+	tda_fail(ret);
+
 	return fe;
 fail:
 	mutex_unlock(&tda18271_list_mutex);

Signature · 1 line

1.7.9.5

Antti Palosaari

author Tested-by

2012-08-06 13:30 UTC #8 lore raw

Quoted 119 lines · Michael Krufky

Quoted 118 lines · Michael Krufky
Quoted 101 lines · Antti Palosaari
Quoted 91 lines · Michael Krufky
Quoted 47 lines · Michael Krufky
Quoted 38 lines · Antti Palosaari
Quoted 19 lines · Michael Krufky
Quoted 7 lines · Antti Palosaari
Moi Michael,
I just realized tda18271 driver eats 160mA too much current after attach.
This means, there is power management bug.
Quoted 6 lines
When I plug my nanoStick it eats total 240mA, after tda18271 sleep is called
it eats only 80mA total which is reasonable. If I use Digital Devices tda18271c2dd driver it is total 110mA after attach, which is also quite
OK.
Quoted 3 lines
Thanks for the report -- I will take a look at it.
...patches are welcome, of course :-)
I suspect it does some tweaking on attach() and chip leaves powered (I saw
demod debugs at calls I2C-gate control quite many times thus this suspicion). When chip is powered-up it is usually in some sleep state by default. Also, on attach() there should be no I/O unless very good reason.
For example chip ID is allowed to read and download firmware in case it is
really needed to continue - like for tuner communication.
Quoted 5 lines
What I found quickly testing few DVB USB sticks there seems to be very much
power management problems... I am now waiting for new multimeter in order to
make better measurements and likely return fixing these issues later.
Quoted 9 lines
The driver does some calibration during attach, some of which is a one-time initialization to determine a temperature differential for tune calculation later on, which can take some time on slower USB buses. The "fix" for the power usage issue would just be to make sure to sleep the device before exiting the attach() function.
I'm not looking to remove the calibration from the attach -- this was done on purpose.
Antti,
After looking again, I realize that we are purposefully not sleeping the device before we exit the attach() function.
The tda18271 is commonly found in multi-chip designs that may or may not include an analog demodulator and / or other tda18271 tuners. In such designs, the chips tend to be daisy-chained to each other, using the xtal output and loop-thru features of the tda18271. We set the required features in the attach-time configuration structure. However, we must keep in mind that this is a hybrid tuner chip, and the analog side of the bridge driver may actually come up before the digital side. Since the actual configuration tends to be done in the digital bring-up, the analog side is brought up within tuner.ko using the most generic one-size-fits all configuration, which gets overridden when the digital side initializes.
It is absolutely crucial that if we actually need the xtal output feature enabled, that it must *never* be turned off, otherwise the i2c bus may get wedged unrecoverably. So, we make sure to leave this feature enabled during the attach function, since we don't yet know at that point whether there is another "instance" of this same tuner yet to be initialized. It is not safe to power off that feature until after we are sure that the bridge has completely initialized.
In order to rectify this issue from within your driver, you should call sleep after you complete the attach. For instance, this is what we do in the cx23885 driver:
if (fe0->dvb.frontend->ops.analog_ops.standby)
Quoted 7 lines
fe0->dvb.frontend->ops.analog_ops.standby(fe0->dvb.frontend);
...except you should call into the tuner_ops->sleep() function instead of analog_demod_ops->standby()
Does this clear things up for you?
Quoted 8 lines
Surely this is possible and it will resolve power drain issue. But it is not nice looking and causes more deviation compared to others.
Could you add configuration option "bool do_not_powerdown_on_attach" ?
I have quite many tda18271 devices here and all those are DVB onlỵ (OK, PCTV 520e is DVB + analog, but analog is not supported). Having configuration parameter sounds like better plan.
Come to think of it, since the generic "one-size-fits-all" configuration leaves the loop thru and xtal output enabled, it should be safe to go to the lowest power level allowed (based on the configuration) at the end of the attach() function. I'll put up a patch within the next few days... Thanks for noticing this, Antti. :-)

Quoted 7 lines

Quoted 3 lines
We wont need to add any new configuration option :-)
-Mike
Antti,
This small patch should do the trick -- can you test it?

Tested-by Antti Palosaari <[email protected]>

Quoted 30 lines

The following changes since commit 0c7d5a6da75caecc677be1fda207b7578936770d:
   Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
   git://git.linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to 782b28e20d3b253d317cc71879639bf3c108b200:
tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)
----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of tda18271_attach()
  drivers/media/common/tuners/tda18271-fe.c |    3 +++
  1 file changed, 3 insertions(+)
Cheers,
Mike

Signature · 1 line

http://palosaari.fi/

A message that is not in the archive (sent off-list or not yet received): [email protected]

Antti Palosaari

author

2012-09-27 19:59 UTC #10 lore raw

Quoted 71 lines · Mauro Carvalho Chehab

Em Thu, 26 Jul 2012 08:48:58 -0400
Michael Krufky <[email protected]> escreveu:
Quoted 18 lines
Antti,
This small patch should do the trick -- can you test it?
The following changes since commit 0c7d5a6da75caecc677be1fda207b7578936770d:
   Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
   git://git.linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to 782b28e20d3b253d317cc71879639bf3c108b200:
tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)
Quoted 6 lines
----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of tda18271_attach()
  drivers/media/common/tuners/tda18271-fe.c |    3 +++
  1 file changed, 3 insertions(+)
Mike,
Despite patchwork's way of handling, thinking that this is a pull request, I suspect that your intention here were simply offer some patches for Antti to test.
In any case, please always send the patches via email to the ML before sending a pull request. This was always a rule, but some developers are lazy with this duty, and, as I didn't use to have a tool to double check, bad things happen.
I'm now finally able to check with a simple script if weather a patch went to the ML or not. My script checks both reply-to/references email tags and it looks for the same patch subject at the ML Inbox. So, I'll be now be more grumpy with that ;) [1]
So, please be sure to post those patches at the ML, with Antti's tested-by:
tag, before sending a pull request.
Thanks!
Mauro
[1] Side note: it is not actually a matter of being grumpy; posted patches receive a lot more attention/review than simple pull requests. From time to time, patches that went via the wrong way (e. g. without a previous post) caused troubles for other developers. So, enforcing it is actually a matter of improving Kernel quality and avoiding regressions.
-
$ test_patch
testing if patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch applies
patch -p1 -i patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch --dry-run -t -N
patching file drivers/media/tuners/tda18271-fe.c
  drivers/media/tuners/tda18271-fe.c |    3 +++
  1 file changed, 3 insertions(+)
Subject: tda18271: enter low-power standby mode at the end of  tda18271_attach()
From: Michael Krufky <[email protected]>
Date: Thu, 26 Jul 2012 08:34:37 -0400
Patch applies OK
total: 0 errors, 0 warnings, 9 lines checked

Quoted 4 lines

patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch has no obvious style problems and is ready for submission. Didn't find any message with subject equal to 'tda18271: enter low-power standby mode at the end of tda18271_attach()' Duplicated md5sum patches
Likely duplicated patches (need manual check)

If that tda18271 patch is not applied then these two should be:

https://patchwork.kernel.org/patch/1481901/
https://patchwork.kernel.org/patch/1481911/

regards
Antti

Signature · 1 line

http://palosaari.fi/

Michael Krufky

maintainer

2012-09-27 21:20 UTC #11 lore raw

Quoted 99 lines · Antti Palosaari

Quoted 84 lines · Mauro Carvalho Chehab
Em Thu, 26 Jul 2012 08:48:58 -0400
Michael Krufky <[email protected]> escreveu:
Quoted 19 lines
Antti,
This small patch should do the trick -- can you test it?
The following changes since commit
0c7d5a6da75caecc677be1fda207b7578936770d:
   Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
   git://git.linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to 782b28e20d3b253d317cc71879639bf3c108b200:
tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)
Quoted 7 lines
----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of
tda18271_attach()
  drivers/media/common/tuners/tda18271-fe.c |    3 +++
  1 file changed, 3 insertions(+)
Mike,
Despite patchwork's way of handling, thinking that this is a pull request, I suspect that your intention here were simply offer some patches for Antti
to test.
In any case, please always send the patches via email to the ML before sending a pull request. This was always a rule, but some developers are lazy with this duty, and, as I didn't use to have a tool to double check, bad things happen.
I'm now finally able to check with a simple script if weather a patch went to the ML or not. My script checks both reply-to/references email tags and it looks for the same patch subject at the ML Inbox. So, I'll be now be more grumpy with that ;) [1]
So, please be sure to post those patches at the ML, with Antti's tested-by:
tag, before sending a pull request.
Thanks!
Mauro
[1] Side note: it is not actually a matter of being grumpy; posted patches receive a lot more attention/review than simple pull requests. From time to time, patches that went via the wrong way (e. g. without a previous post)
caused troubles for other developers. So, enforcing it is actually a matter
of improving Kernel quality and avoiding regressions.
-
$ test_patch
testing if
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch
applies
patch -p1 -i
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch
--dry-run -t -N
patching file drivers/media/tuners/tda18271-fe.c
  drivers/media/tuners/tda18271-fe.c |    3 +++
  1 file changed, 3 insertions(+)
Subject: tda18271: enter low-power standby mode at the end of
tda18271_attach()
From: Michael Krufky <[email protected]>
Date: Thu, 26 Jul 2012 08:34:37 -0400
Patch applies OK
total: 0 errors, 0 warnings, 9 lines checked
Quoted 6 lines
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch has no obvious style problems and is ready for submission. Didn't find any message with subject equal to 'tda18271: enter low-power standby mode at the end of tda18271_attach()'
Duplicated md5sum patches
Likely duplicated patches (need manual check)
If that tda18271 patch is not applied then these two should be:
https://patchwork.kernel.org/patch/1481901/
https://patchwork.kernel.org/patch/1481911/

Quoted 5 lines

regards
Antti
--
http://palosaari.fi/

The tda18271 patch should indeed be applied -- I will send it to the ML later on today and follow up with a pull request. Thanks to all who have commented :-)

-Mike

Antti Palosaari

author

2012-09-27 21:38 UTC #12 lore raw

Quoted 103 lines · Michael Krufky

Quoted 102 lines · Antti Palosaari
Quoted 84 lines · Mauro Carvalho Chehab
Em Thu, 26 Jul 2012 08:48:58 -0400
Michael Krufky <[email protected]> escreveu:
Quoted 19 lines
Antti,
This small patch should do the trick -- can you test it?
The following changes since commit
0c7d5a6da75caecc677be1fda207b7578936770d:
    Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
    git://git.linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to 782b28e20d3b253d317cc71879639bf3c108b200:
tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)
Quoted 7 lines
----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of
tda18271_attach()
   drivers/media/common/tuners/tda18271-fe.c |    3 +++
   1 file changed, 3 insertions(+)
Mike,
Despite patchwork's way of handling, thinking that this is a pull request, I suspect that your intention here were simply offer some patches for Antti
to test.
In any case, please always send the patches via email to the ML before sending a pull request. This was always a rule, but some developers are lazy with this duty, and, as I didn't use to have a tool to double check, bad things happen.
I'm now finally able to check with a simple script if weather a patch went to the ML or not. My script checks both reply-to/references email tags and it looks for the same patch subject at the ML Inbox. So, I'll be now be more grumpy with that ;) [1]
So, please be sure to post those patches at the ML, with Antti's tested-by:
tag, before sending a pull request.
Thanks!
Mauro
[1] Side note: it is not actually a matter of being grumpy; posted patches receive a lot more attention/review than simple pull requests. From time to time, patches that went via the wrong way (e. g. without a previous post)
caused troubles for other developers. So, enforcing it is actually a matter
of improving Kernel quality and avoiding regressions.
-
$ test_patch
testing if
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch
applies
patch -p1 -i
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch
--dry-run -t -N
patching file drivers/media/tuners/tda18271-fe.c
   drivers/media/tuners/tda18271-fe.c |    3 +++
   1 file changed, 3 insertions(+)
Subject: tda18271: enter low-power standby mode at the end of
tda18271_attach()
From: Michael Krufky <[email protected]>
Date: Thu, 26 Jul 2012 08:34:37 -0400
Patch applies OK
total: 0 errors, 0 warnings, 9 lines checked
Quoted 6 lines
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch has no obvious style problems and is ready for submission. Didn't find any message with subject equal to 'tda18271: enter low-power standby mode at the end of tda18271_attach()'
Duplicated md5sum patches
Likely duplicated patches (need manual check)
If that tda18271 patch is not applied then these two should be:
https://patchwork.kernel.org/patch/1481901/
https://patchwork.kernel.org/patch/1481911/
regards
Antti

Quoted 6 lines

Quoted 2 lines
--
http://palosaari.fi/
The tda18271 patch should indeed be applied -- I will send it to the ML later on today and follow up with a pull request. Thanks to all who have commented :-)

Mike, There is other problem too. PCTV 520e, which is Em28xx + DRX-K + TDA18271, fails to attach tuner now. Tuner is wired behind DRX-K I2C bus. TDA18271 driver does very much I/O during attach and I2C error is raised during attach now. Earlier it worked as DRX-K firmware was downloaded before tuner was attached, but now both DRX-K fw download and tuner attach happens same time leading that error.

regards
Antti

Signature · 1 line

http://palosaari.fi/

Michael Krufky

maintainer

2012-09-27 21:58 UTC #13 lore raw

Quoted 122 lines · Antti Palosaari

Quoted 112 lines · Michael Krufky
Quoted 110 lines · Antti Palosaari
Quoted 90 lines · Mauro Carvalho Chehab
Em Thu, 26 Jul 2012 08:48:58 -0400
Michael Krufky <[email protected]> escreveu:
Quoted 20 lines
Antti,
This small patch should do the trick -- can you test it?
The following changes since commit
0c7d5a6da75caecc677be1fda207b7578936770d:
    Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
    git://git.linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to
782b28e20d3b253d317cc71879639bf3c108b200:
tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)
Quoted 7 lines
----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of
tda18271_attach()
   drivers/media/common/tuners/tda18271-fe.c |    3 +++
   1 file changed, 3 insertions(+)
Mike,
Despite patchwork's way of handling, thinking that this is a pull request,
I suspect that your intention here were simply offer some patches for Antti
to test.
In any case, please always send the patches via email to the ML before sending a pull request. This was always a rule, but some developers are lazy with this duty, and, as I didn't use to have a tool to double check,
bad things happen.
I'm now finally able to check with a simple script if weather a patch went to the ML or not. My script checks both reply-to/references email tags and it looks for the same patch subject at the ML Inbox. So, I'll be now be more grumpy with that ;) [1]
So, please be sure to post those patches at the ML, with Antti's tested-by:
tag, before sending a pull request.
Thanks!
Mauro
[1] Side note: it is not actually a matter of being grumpy; posted patches
receive a lot more attention/review than simple pull requests. From time to time, patches that went via the wrong way (e. g. without a previous post)
caused troubles for other developers. So, enforcing it is actually a matter
of improving Kernel quality and avoiding regressions.
-
$ test_patch
testing if
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch
applies
patch -p1 -i
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch
--dry-run -t -N
patching file drivers/media/tuners/tda18271-fe.c
   drivers/media/tuners/tda18271-fe.c |    3 +++
   1 file changed, 3 insertions(+)
Subject: tda18271: enter low-power standby mode at the end of
tda18271_attach()
From: Michael Krufky <[email protected]>
Date: Thu, 26 Jul 2012 08:34:37 -0400
Patch applies OK
total: 0 errors, 0 warnings, 9 lines checked
Quoted 6 lines
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch has no obvious style problems and is ready for submission. Didn't find any message with subject equal to 'tda18271: enter low-power standby mode at the end of tda18271_attach()'
Duplicated md5sum patches
Likely duplicated patches (need manual check)
If that tda18271 patch is not applied then these two should be:
https://patchwork.kernel.org/patch/1481901/
https://patchwork.kernel.org/patch/1481911/
regards
Antti
Quoted 7 lines
Quoted 2 lines
--
http://palosaari.fi/
The tda18271 patch should indeed be applied -- I will send it to the ML later on today and follow up with a pull request. Thanks to all who have commented :-)

Quoted 6 lines

Mike, There is other problem too. PCTV 520e, which is Em28xx + DRX-K + TDA18271, fails to attach tuner now. Tuner is wired behind DRX-K I2C bus. TDA18271 driver does very much I/O during attach and I2C error is raised during attach now. Earlier it worked as DRX-K firmware was downloaded before tuner was attached, but now both DRX-K fw download and tuner attach happens same time leading that error.

Why is the DRX-K firmware downloading at the same time as tuner attach? Shouldn't the demod attach be finished before the tuner attach begins?

-Mike

Antti Palosaari

author

2012-09-27 22:26 UTC #14 lore raw

Quoted 130 lines · Michael Krufky

Quoted 122 lines · Antti Palosaari
Quoted 112 lines · Michael Krufky
Quoted 110 lines · Antti Palosaari
Quoted 90 lines · Mauro Carvalho Chehab
Em Thu, 26 Jul 2012 08:48:58 -0400
Michael Krufky <[email protected]> escreveu:
Quoted 20 lines
Antti,
This small patch should do the trick -- can you test it?
The following changes since commit
0c7d5a6da75caecc677be1fda207b7578936770d:
     Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
     git://git.linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to
782b28e20d3b253d317cc71879639bf3c108b200:
tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)
Quoted 7 lines
----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of
tda18271_attach()
    drivers/media/common/tuners/tda18271-fe.c |    3 +++
    1 file changed, 3 insertions(+)
Mike,
Despite patchwork's way of handling, thinking that this is a pull request,
I suspect that your intention here were simply offer some patches for Antti
to test.
In any case, please always send the patches via email to the ML before sending a pull request. This was always a rule, but some developers are lazy with this duty, and, as I didn't use to have a tool to double check,
bad things happen.
I'm now finally able to check with a simple script if weather a patch went to the ML or not. My script checks both reply-to/references email tags and it looks for the same patch subject at the ML Inbox. So, I'll be now be more grumpy with that ;) [1]
So, please be sure to post those patches at the ML, with Antti's tested-by:
tag, before sending a pull request.
Thanks!
Mauro
[1] Side note: it is not actually a matter of being grumpy; posted patches
receive a lot more attention/review than simple pull requests. From time to time, patches that went via the wrong way (e. g. without a previous post)
caused troubles for other developers. So, enforcing it is actually a matter
of improving Kernel quality and avoiding regressions.
-
$ test_patch
testing if
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch
applies
patch -p1 -i
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch
--dry-run -t -N
patching file drivers/media/tuners/tda18271-fe.c
    drivers/media/tuners/tda18271-fe.c |    3 +++
    1 file changed, 3 insertions(+)
Subject: tda18271: enter low-power standby mode at the end of
tda18271_attach()
From: Michael Krufky <[email protected]>
Date: Thu, 26 Jul 2012 08:34:37 -0400
Patch applies OK
total: 0 errors, 0 warnings, 9 lines checked
Quoted 6 lines
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch has no obvious style problems and is ready for submission. Didn't find any message with subject equal to 'tda18271: enter low-power standby mode at the end of tda18271_attach()'
Duplicated md5sum patches
Likely duplicated patches (need manual check)
If that tda18271 patch is not applied then these two should be:
https://patchwork.kernel.org/patch/1481901/
https://patchwork.kernel.org/patch/1481911/
regards
Antti
Quoted 7 lines
Quoted 2 lines
--
http://palosaari.fi/
The tda18271 patch should indeed be applied -- I will send it to the ML later on today and follow up with a pull request. Thanks to all who have commented :-)
Quoted 6 lines
Mike, There is other problem too. PCTV 520e, which is Em28xx + DRX-K + TDA18271, fails to attach tuner now. Tuner is wired behind DRX-K I2C bus. TDA18271 driver does very much I/O during attach and I2C error is raised during attach now. Earlier it worked as DRX-K firmware was downloaded before tuner was attached, but now both DRX-K fw download and tuner attach happens same time leading that error.

Quoted 3 lines

Why is the DRX-K firmware downloading at the same time as tuner attach? Shouldn't the demod attach be finished before the tuner attach begins?

What I think all these should go in order bridge => demod => tuner. Attach and fw loading. I cannot see how it will never work generally unless those firmwares are loaded in that order.
1) bridge needs firmware up and running before demod could be attached. That is mostly because I2C adapter is behind bridge.

2) demod needs firmware up and running before tuner could be attached. That is mostly because I2C adapter/bus for tuner is behind the demod.

3) tuner needs firmware up and running as there could be firmware controlled GPIO bus behind tuner. There is many times LNA, LNB controller, LED, antenna switch. I am not surprised if there is even I2C bus behind the tuner to control LNB or other equipment near antenna connector and tuner.

Of course situation is not that bad usually - but surely there could be some existing device which is very near that. But as we *want* to do things as general as possible to avoid driver / device specific hacks that is the only reasonable model.

regards
Antti

Signature · 1 line

http://palosaari.fi/

Michael Krufky

maintainer

2012-09-27 22:43 UTC #15 lore raw

Quoted 168 lines · Antti Palosaari

Quoted 148 lines · Michael Krufky
Quoted 136 lines · Antti Palosaari
Quoted 123 lines · Michael Krufky
Quoted 120 lines · Antti Palosaari
Quoted 97 lines · Mauro Carvalho Chehab
Em Thu, 26 Jul 2012 08:48:58 -0400
Michael Krufky <[email protected]> escreveu:
Quoted 20 lines
Antti,
This small patch should do the trick -- can you test it?
The following changes since commit
0c7d5a6da75caecc677be1fda207b7578936770d:
     Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
     git://git.linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to
782b28e20d3b253d317cc71879639bf3c108b200:
tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)
Quoted 7 lines
----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of
tda18271_attach()
    drivers/media/common/tuners/tda18271-fe.c |    3 +++
    1 file changed, 3 insertions(+)
Mike,
Despite patchwork's way of handling, thinking that this is a pull request,
I suspect that your intention here were simply offer some patches for Antti
to test.
In any case, please always send the patches via email to the ML before sending a pull request. This was always a rule, but some developers are
lazy with this duty, and, as I didn't use to have a tool to double check,
bad things happen.
I'm now finally able to check with a simple script if weather a patch went to the ML or not. My script checks both reply-to/references email tags and it looks for the same patch subject at the ML Inbox. So, I'll be now be more grumpy with that ;) [1]
So, please be sure to post those patches at the ML, with Antti's tested-by:
tag, before sending a pull request.
Thanks!
Mauro
[1] Side note: it is not actually a matter of being grumpy; posted patches
receive a lot more attention/review than simple pull requests. From time
to time, patches that went via the wrong way (e. g. without a previous post)
caused troubles for other developers. So, enforcing it is actually a matter
of improving Kernel quality and avoiding regressions.
-
$ test_patch
testing if
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch applies
patch -p1 -i
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch --dry-run -t -N
patching file drivers/media/tuners/tda18271-fe.c
    drivers/media/tuners/tda18271-fe.c |    3 +++
    1 file changed, 3 insertions(+)
Subject: tda18271: enter low-power standby mode at the end of tda18271_attach()
From: Michael Krufky <[email protected]>
Date: Thu, 26 Jul 2012 08:34:37 -0400
Patch applies OK
total: 0 errors, 0 warnings, 9 lines checked
Quoted 7 lines
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch has no obvious style problems and is ready for submission. Didn't find any message with subject equal to 'tda18271: enter low-power
standby mode at the end of tda18271_attach()'
Duplicated md5sum patches
Likely duplicated patches (need manual check)
If that tda18271 patch is not applied then these two should be:
https://patchwork.kernel.org/patch/1481901/
https://patchwork.kernel.org/patch/1481911/
regards
Antti
Quoted 8 lines
Quoted 2 lines
--
http://palosaari.fi/
The tda18271 patch should indeed be applied -- I will send it to the ML later on today and follow up with a pull request. Thanks to all who have commented :-)
Quoted 8 lines
Mike, There is other problem too. PCTV 520e, which is Em28xx + DRX-K + TDA18271, fails to attach tuner now. Tuner is wired behind DRX-K I2C bus. TDA18271 driver does very much I/O during attach and I2C error is raised during attach now. Earlier it worked as DRX-K firmware was downloaded before
tuner was attached, but now both DRX-K fw download and tuner attach happens
same time leading that error.
Quoted 3 lines
Why is the DRX-K firmware downloading at the same time as tuner attach? Shouldn't the demod attach be finished before the tuner attach begins?
What I think all these should go in order bridge => demod => tuner. Attach and fw loading. I cannot see how it will never work generally unless those firmwares are loaded in that order.
1) bridge needs firmware up and running before demod could be attached. That is mostly because I2C adapter is behind bridge.
2) demod needs firmware up and running before tuner could be attached. That is mostly because I2C adapter/bus for tuner is behind the demod.
3) tuner needs firmware up and running as there could be firmware controlled GPIO bus behind tuner. There is many times LNA, LNB controller, LED, antenna switch. I am not surprised if there is even I2C bus behind the tuner to control LNB or other equipment near antenna connector and tuner.

Quoted 5 lines

Of course situation is not that bad usually - but surely there could be some existing device which is very near that. But as we *want* to do things as general as possible to avoid driver / device specific hacks that is the only reasonable model.

I'm not sure how that relates to the problem you brought up..... back to the issue:

I don't have the PCTV 520e schematics handy, but... it's possible that the DRX-K depends on XTOUT from the tda18271 --

In your struct tda18271_config, are you using the .output_opt configuration? for example:

struct tda18271_config hauppauge_tda18271_config = {
        .std_map = &hauppauge_tda18271_std_map,
        .gate    = TDA18271_GATE_ANALOG,
        .output_opt = TDA18271_OUTPUT_LT_OFF,
};

If so, try deleting the .output_opt line - see if that helps.

-Mike

Antti Palosaari

author

2012-09-27 22:46 UTC #16 lore raw

Quoted 185 lines · Michael Krufky

Quoted 168 lines · Antti Palosaari
Quoted 148 lines · Michael Krufky
Quoted 136 lines · Antti Palosaari
Quoted 123 lines · Michael Krufky
Quoted 120 lines · Antti Palosaari
Quoted 97 lines · Mauro Carvalho Chehab
Em Thu, 26 Jul 2012 08:48:58 -0400
Michael Krufky <[email protected]> escreveu:
Quoted 20 lines
Antti,
This small patch should do the trick -- can you test it?
The following changes since commit
0c7d5a6da75caecc677be1fda207b7578936770d:
      Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
      git://git.linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to
782b28e20d3b253d317cc71879639bf3c108b200:
tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)
Quoted 7 lines
----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of
tda18271_attach()
     drivers/media/common/tuners/tda18271-fe.c |    3 +++
     1 file changed, 3 insertions(+)
Mike,
Despite patchwork's way of handling, thinking that this is a pull request,
I suspect that your intention here were simply offer some patches for Antti
to test.
In any case, please always send the patches via email to the ML before sending a pull request. This was always a rule, but some developers are
lazy with this duty, and, as I didn't use to have a tool to double check,
bad things happen.
I'm now finally able to check with a simple script if weather a patch went to the ML or not. My script checks both reply-to/references email tags and it looks for the same patch subject at the ML Inbox. So, I'll be now be more grumpy with that ;) [1]
So, please be sure to post those patches at the ML, with Antti's tested-by:
tag, before sending a pull request.
Thanks!
Mauro
[1] Side note: it is not actually a matter of being grumpy; posted patches
receive a lot more attention/review than simple pull requests. From time
to time, patches that went via the wrong way (e. g. without a previous post)
caused troubles for other developers. So, enforcing it is actually a matter
of improving Kernel quality and avoiding regressions.
-
$ test_patch
testing if
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch applies
patch -p1 -i
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch --dry-run -t -N
patching file drivers/media/tuners/tda18271-fe.c
     drivers/media/tuners/tda18271-fe.c |    3 +++
     1 file changed, 3 insertions(+)
Subject: tda18271: enter low-power standby mode at the end of tda18271_attach()
From: Michael Krufky <[email protected]>
Date: Thu, 26 Jul 2012 08:34:37 -0400
Patch applies OK
total: 0 errors, 0 warnings, 9 lines checked
Quoted 7 lines
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch has no obvious style problems and is ready for submission. Didn't find any message with subject equal to 'tda18271: enter low-power
standby mode at the end of tda18271_attach()'
Duplicated md5sum patches
Likely duplicated patches (need manual check)
If that tda18271 patch is not applied then these two should be:
https://patchwork.kernel.org/patch/1481901/
https://patchwork.kernel.org/patch/1481911/
regards
Antti
Quoted 8 lines
Quoted 2 lines
--
http://palosaari.fi/
The tda18271 patch should indeed be applied -- I will send it to the ML later on today and follow up with a pull request. Thanks to all who have commented :-)
Quoted 8 lines
Mike, There is other problem too. PCTV 520e, which is Em28xx + DRX-K + TDA18271, fails to attach tuner now. Tuner is wired behind DRX-K I2C bus. TDA18271 driver does very much I/O during attach and I2C error is raised during attach now. Earlier it worked as DRX-K firmware was downloaded before
tuner was attached, but now both DRX-K fw download and tuner attach happens
same time leading that error.
Quoted 3 lines
Why is the DRX-K firmware downloading at the same time as tuner attach? Shouldn't the demod attach be finished before the tuner attach begins?
What I think all these should go in order bridge => demod => tuner. Attach and fw loading. I cannot see how it will never work generally unless those firmwares are loaded in that order.
1) bridge needs firmware up and running before demod could be attached. That is mostly because I2C adapter is behind bridge.
2) demod needs firmware up and running before tuner could be attached. That is mostly because I2C adapter/bus for tuner is behind the demod.
3) tuner needs firmware up and running as there could be firmware controlled GPIO bus behind tuner. There is many times LNA, LNB controller, LED, antenna switch. I am not surprised if there is even I2C bus behind the tuner to control LNB or other equipment near antenna connector and tuner.
Quoted 5 lines
Of course situation is not that bad usually - but surely there could be some existing device which is very near that. But as we *want* to do things as general as possible to avoid driver / device specific hacks that is the only reasonable model.
I'm not sure how that relates to the problem you brought up..... back to the issue:
I don't have the PCTV 520e schematics handy, but... it's possible that the DRX-K depends on XTOUT from the tda18271 --
In your struct tda18271_config, are you using the .output_opt configuration? for example:

Quoted 8 lines

struct tda18271_config hauppauge_tda18271_config = {
         .std_map = &hauppauge_tda18271_std_map,
         .gate    = TDA18271_GATE_ANALOG,
         .output_opt = TDA18271_OUTPUT_LT_OFF,
};

If so, try deleting the .output_opt line - see if that helps.

I suspect it does not have nothing to do with tuner outputs. If I add 2 second sleep after demod attach and before tuner attach - it works. Also if I use DRX-K internal firmware (not download newer) it works.

Here is the debug (drx-k & tda18271):
http://palosaari.fi/linux/v4l-dvb/em28xx_drxk_tda18271.txt

regards
Antti

Signature · 1 line

http://palosaari.fi/

Michael Krufky

maintainer

2012-09-27 22:55 UTC #17 lore raw

Quoted 227 lines · Antti Palosaari

Quoted 216 lines · Michael Krufky
Quoted 196 lines · Antti Palosaari
Quoted 168 lines · Michael Krufky
Quoted 152 lines · Antti Palosaari
Quoted 142 lines · Michael Krufky
Quoted 129 lines · Antti Palosaari
Quoted 107 lines · Mauro Carvalho Chehab
Em Thu, 26 Jul 2012 08:48:58 -0400
Michael Krufky <[email protected]> escreveu:
Quoted 20 lines
Antti,
This small patch should do the trick -- can you test it?
The following changes since commit
0c7d5a6da75caecc677be1fda207b7578936770d:
      Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
      git://git.linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to
782b28e20d3b253d317cc71879639bf3c108b200:
tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)
Quoted 7 lines
----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of
tda18271_attach()
     drivers/media/common/tuners/tda18271-fe.c |    3 +++
     1 file changed, 3 insertions(+)
Mike,
Despite patchwork's way of handling, thinking that this is a pull request,
I suspect that your intention here were simply offer some patches for
Antti
to test.
In any case, please always send the patches via email to the ML before
sending a pull request. This was always a rule, but some developers are
lazy with this duty, and, as I didn't use to have a tool to double check,
bad things happen.
I'm now finally able to check with a simple script if weather a patch
went to the ML or not. My script checks both reply-to/references email
tags and it looks for the same patch subject at the ML Inbox. So, I'll be now be more grumpy with that ;) [1]
So, please be sure to post those patches at the ML, with Antti's tested-by:
tag, before sending a pull request.
Thanks!
Mauro
[1] Side note: it is not actually a matter of being grumpy; posted patches
receive a lot more attention/review than simple pull requests. From time
to time, patches that went via the wrong way (e. g. without a previous
post)
caused troubles for other developers. So, enforcing it is actually a matter
of improving Kernel quality and avoiding regressions.
-
$ test_patch
testing if
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch applies
patch -p1 -i
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch --dry-run -t -N
patching file drivers/media/tuners/tda18271-fe.c
     drivers/media/tuners/tda18271-fe.c |    3 +++
     1 file changed, 3 insertions(+)
Subject: tda18271: enter low-power standby mode at the end of tda18271_attach()
From: Michael Krufky <[email protected]>
Date: Thu, 26 Jul 2012 08:34:37 -0400
Patch applies OK
total: 0 errors, 0 warnings, 9 lines checked
Quoted 7 lines
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch has no obvious style problems and is ready for submission. Didn't find any message with subject equal to 'tda18271: enter low-power
standby mode at the end of tda18271_attach()'
Duplicated md5sum patches
Likely duplicated patches (need manual check)
If that tda18271 patch is not applied then these two should be:
https://patchwork.kernel.org/patch/1481901/
https://patchwork.kernel.org/patch/1481911/
Quoted 5 lines
regards
Antti
--
http://palosaari.fi/
Quoted 3 lines
The tda18271 patch should indeed be applied -- I will send it to the ML later on today and follow up with a pull request. Thanks to all who have commented :-)
Quoted 10 lines
Mike, There is other problem too. PCTV 520e, which is Em28xx + DRX-K + TDA18271, fails to attach tuner now. Tuner is wired behind DRX-K I2C bus.
TDA18271 driver does very much I/O during attach and I2C error is raised
during attach now. Earlier it worked as DRX-K firmware was downloaded before
tuner was attached, but now both DRX-K fw download and tuner attach happens
same time leading that error.
Quoted 3 lines
Why is the DRX-K firmware downloading at the same time as tuner attach? Shouldn't the demod attach be finished before the tuner attach begins?
What I think all these should go in order bridge => demod => tuner. Attach
and fw loading. I cannot see how it will never work generally unless those
firmwares are loaded in that order.
1) bridge needs firmware up and running before demod could be attached. That
is mostly because I2C adapter is behind bridge.
2) demod needs firmware up and running before tuner could be attached. That
is mostly because I2C adapter/bus for tuner is behind the demod.
3) tuner needs firmware up and running as there could be firmware controlled
GPIO bus behind tuner. There is many times LNA, LNB controller, LED, antenna
switch. I am not surprised if there is even I2C bus behind the tuner to control LNB or other equipment near antenna connector and tuner.
Quoted 7 lines
Of course situation is not that bad usually - but surely there could be some
existing device which is very near that. But as we *want* to do things as general as possible to avoid driver / device specific hacks that is the only
reasonable model.
I'm not sure how that relates to the problem you brought up..... back to the issue:
I don't have the PCTV 520e schematics handy, but... it's possible that the DRX-K depends on XTOUT from the tda18271 --
In your struct tda18271_config, are you using the .output_opt configuration? for example:
Quoted 8 lines
struct tda18271_config hauppauge_tda18271_config = {
         .std_map = &hauppauge_tda18271_std_map,
         .gate    = TDA18271_GATE_ANALOG,
         .output_opt = TDA18271_OUTPUT_LT_OFF,
};
If so, try deleting the .output_opt line - see if that helps.

Quoted 6 lines

I suspect it does not have nothing to do with tuner outputs. If I add 2 second sleep after demod attach and before tuner attach - it works. Also if I use DRX-K internal firmware (not download newer) it works.
Here is the debug (drx-k & tda18271):
http://palosaari.fi/linux/v4l-dvb/em28xx_drxk_tda18271.txt

Antti,

This is not about tuner *output* -- The tda18271 has a crystal output feature to drive another demod. If the demod needs this crystal output, then it will lockup if the tuner disables the xtout.

Please try my suggestion -- it seems to me that it's quite likely that the DRX-K could be using the XTOUT from the tda18271. That's why the driver leaves XTOUT on by default.

Try rebuilding with the TDA18271_OUTPUT_LT_OFF, removed.

-Mike

Antti Palosaari

author

2012-09-27 23:05 UTC #18 lore raw

Quoted 241 lines · Michael Krufky

Quoted 227 lines · Antti Palosaari
Quoted 216 lines · Michael Krufky
Quoted 196 lines · Antti Palosaari
Quoted 168 lines · Michael Krufky
Quoted 152 lines · Antti Palosaari
Quoted 142 lines · Michael Krufky
Quoted 129 lines · Antti Palosaari
Quoted 107 lines · Mauro Carvalho Chehab
Em Thu, 26 Jul 2012 08:48:58 -0400
Michael Krufky <[email protected]> escreveu:
Quoted 20 lines
Antti,
This small patch should do the trick -- can you test it?
The following changes since commit
0c7d5a6da75caecc677be1fda207b7578936770d:
       Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
       git://git.linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to
782b28e20d3b253d317cc71879639bf3c108b200:
tda18271: enter low-power standby mode at the end of
tda18271_attach() (2012-07-26 08:34:37 -0400)
Quoted 7 lines
----------------------------------------------------------------
Michael Krufky (1):
tda18271: enter low-power standby mode at the end of
tda18271_attach()
      drivers/media/common/tuners/tda18271-fe.c |    3 +++
      1 file changed, 3 insertions(+)
Mike,
Despite patchwork's way of handling, thinking that this is a pull request,
I suspect that your intention here were simply offer some patches for
Antti
to test.
In any case, please always send the patches via email to the ML before
sending a pull request. This was always a rule, but some developers are
lazy with this duty, and, as I didn't use to have a tool to double check,
bad things happen.
I'm now finally able to check with a simple script if weather a patch
went to the ML or not. My script checks both reply-to/references email
tags and it looks for the same patch subject at the ML Inbox. So, I'll be now be more grumpy with that ;) [1]
So, please be sure to post those patches at the ML, with Antti's tested-by:
tag, before sending a pull request.
Thanks!
Mauro
[1] Side note: it is not actually a matter of being grumpy; posted patches
receive a lot more attention/review than simple pull requests. From time
to time, patches that went via the wrong way (e. g. without a previous
post)
caused troubles for other developers. So, enforcing it is actually a matter
of improving Kernel quality and avoiding regressions.
-
$ test_patch
testing if
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch applies
patch -p1 -i
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch --dry-run -t -N
patching file drivers/media/tuners/tda18271-fe.c
      drivers/media/tuners/tda18271-fe.c |    3 +++
      1 file changed, 3 insertions(+)
Subject: tda18271: enter low-power standby mode at the end of tda18271_attach()
From: Michael Krufky <[email protected]>
Date: Thu, 26 Jul 2012 08:34:37 -0400
Patch applies OK
total: 0 errors, 0 warnings, 9 lines checked
Quoted 7 lines
patches/0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch has no obvious style problems and is ready for submission. Didn't find any message with subject equal to 'tda18271: enter low-power
standby mode at the end of tda18271_attach()'
Duplicated md5sum patches
Likely duplicated patches (need manual check)
If that tda18271 patch is not applied then these two should be:
https://patchwork.kernel.org/patch/1481901/
https://patchwork.kernel.org/patch/1481911/
Quoted 5 lines
regards
Antti
--
http://palosaari.fi/
Quoted 3 lines
The tda18271 patch should indeed be applied -- I will send it to the ML later on today and follow up with a pull request. Thanks to all who have commented :-)
Quoted 10 lines
Mike, There is other problem too. PCTV 520e, which is Em28xx + DRX-K + TDA18271, fails to attach tuner now. Tuner is wired behind DRX-K I2C bus.
TDA18271 driver does very much I/O during attach and I2C error is raised
during attach now. Earlier it worked as DRX-K firmware was downloaded before
tuner was attached, but now both DRX-K fw download and tuner attach happens
same time leading that error.
Quoted 3 lines
Why is the DRX-K firmware downloading at the same time as tuner attach? Shouldn't the demod attach be finished before the tuner attach begins?
What I think all these should go in order bridge => demod => tuner. Attach
and fw loading. I cannot see how it will never work generally unless those
firmwares are loaded in that order.
1) bridge needs firmware up and running before demod could be attached. That
is mostly because I2C adapter is behind bridge.
2) demod needs firmware up and running before tuner could be attached. That
is mostly because I2C adapter/bus for tuner is behind the demod.
3) tuner needs firmware up and running as there could be firmware controlled
GPIO bus behind tuner. There is many times LNA, LNB controller, LED, antenna
switch. I am not surprised if there is even I2C bus behind the tuner to control LNB or other equipment near antenna connector and tuner.
Quoted 7 lines
Of course situation is not that bad usually - but surely there could be some
existing device which is very near that. But as we *want* to do things as general as possible to avoid driver / device specific hacks that is the only
reasonable model.
I'm not sure how that relates to the problem you brought up..... back to the issue:
I don't have the PCTV 520e schematics handy, but... it's possible that the DRX-K depends on XTOUT from the tda18271 --
In your struct tda18271_config, are you using the .output_opt configuration? for example:
Quoted 8 lines
struct tda18271_config hauppauge_tda18271_config = {
          .std_map = &hauppauge_tda18271_std_map,
          .gate    = TDA18271_GATE_ANALOG,
          .output_opt = TDA18271_OUTPUT_LT_OFF,
};
If so, try deleting the .output_opt line - see if that helps.
Quoted 6 lines
I suspect it does not have nothing to do with tuner outputs. If I add 2 second sleep after demod attach and before tuner attach - it works. Also if I use DRX-K internal firmware (not download newer) it works.
Here is the debug (drx-k & tda18271):
http://palosaari.fi/linux/v4l-dvb/em28xx_drxk_tda18271.txt
Antti,
This is not about tuner *output* -- The tda18271 has a crystal output feature to drive another demod. If the demod needs this crystal output, then it will lockup if the tuner disables the xtout.

Quoted 5 lines

Please try my suggestion -- it seems to me that it's quite likely that the DRX-K could be using the XTOUT from the tda18271. That's why the driver leaves XTOUT on by default.
Try rebuilding with the TDA18271_OUTPUT_LT_OFF, removed.

Tested (all possible) values: 0/1/2. No effect.

regards
Antti

Signature · 1 line

http://palosaari.fi/

A message that is not in the archive (sent off-list or not yet received): [email protected]

Mauro Carvalho Chehab

2012-09-28 15:04 UTC #20 lore raw

PATCH 1/1 tda18271-common: hold the I2C adapter during write transfers

The tda18271 datasheet says:

"The image rejection calibration and RF tracking filter
calibration must be launched exactly as described in the flowchart, otherwise bad calibration or even blocking of the TDA18211HD can result making it impossible to communicate via the I2C-bus."

(yeah, tda18271 refers there to tda18211 - likely a typo at their
datasheets)

That likely explains why sometimes tda18271 stops answering. That is now happening more often on designs with drx-k chips, as the firmware is now loaded asyncrousnly there.

While the above text doesn't explicitly tell that the I2C bus couldn't be used by other devices during such initialization, that seems to be a requirement there.

So, let's explicitly use the I2C lock there, avoiding I2C bus share during those critical moments.

Compile-tested only. Please test.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/tuners/tda18271-common.c | 104 ++++++++++++++++++++++-----------
 1 file changed, 71 insertions(+), 33 deletions(-)

drivers/media/tuners/tda18271-common.c +71 ~~-33~~

index 221171e..18c77af 100644
@@ -187,7 +187,8 @@ int tda18271_read_extended(struct dvb_frontend *fe)
 	return (ret == 2 ? 0 : ret);
 }
 
-int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+static int __tda18271_write_regs(struct dvb_frontend *fe, int idx, int len,
+			bool lock_i2c)
 {
 	struct tda18271_priv *priv = fe->tuner_priv;
 	unsigned char *regs = priv->tda18271_regs;

@@ -198,7 +199,6 @@ int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
 
 	BUG_ON((len == 0) || (idx + len > sizeof(buf)));
 
-
 	switch (priv->small_i2c) {
 	case TDA18271_03_BYTE_CHUNK_INIT:
 		max = 3;

@@ -214,7 +214,19 @@ int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
 		max = 39;
 	}
 
-	tda18271_i2c_gate_ctrl(fe, 1);
+
+	/*
+	 * If lock_i2c is true, it will take the I2C bus for tda18271 private
+	 * usage during the entire write ops, as otherwise, bad things could
+	 * happen.
+	 * During device init, several write operations will happen. So,
+	 * tda18271_init_regs controls the I2C lock directly,
+	 * disabling lock_i2c here.
+	 */
+	if (lock_i2c) {
+		tda18271_i2c_gate_ctrl(fe, 1);
+		i2c_lock_adapter(priv->i2c_props.adap);
+	}
 	while (len) {
 		if (max > len)
 			max = len;

@@ -226,14 +238,17 @@ int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
 		msg.len = max + 1;
 
 		/* write registers */
-		ret = i2c_transfer(priv->i2c_props.adap, &msg, 1);
+		ret = __i2c_transfer(priv->i2c_props.adap, &msg, 1);
 		if (ret != 1)
 			break;
 
 		idx += max;
 		len -= max;
 	}
-	tda18271_i2c_gate_ctrl(fe, 0);
+	if (lock_i2c) {
+		i2c_unlock_adapter(priv->i2c_props.adap);
+		tda18271_i2c_gate_ctrl(fe, 0);
+	}
 
 	if (ret != 1)
 		tda_err("ERROR: idx = 0x%x, len = %d, "

@@ -242,10 +257,16 @@ int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
 	return (ret == 1 ? 0 : ret);
 }
 
+int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+{
+	return __tda18271_write_regs(fe, idx, len, true);
+}
+
 /*---------------------------------------------------------------------*/
 
-int tda18271_charge_pump_source(struct dvb_frontend *fe,
-				enum tda18271_pll pll, int force)
+static int __tda18271_charge_pump_source(struct dvb_frontend *fe,
+					 enum tda18271_pll pll, int force,
+					 bool lock_i2c)
 {
 	struct tda18271_priv *priv = fe->tuner_priv;
 	unsigned char *regs = priv->tda18271_regs;

@@ -255,9 +276,16 @@ int tda18271_charge_pump_source(struct dvb_frontend *fe,
 	regs[r_cp] &= ~0x20;
 	regs[r_cp] |= ((force & 1) << 5);
 
-	return tda18271_write_regs(fe, r_cp, 1);
+	return __tda18271_write_regs(fe, r_cp, 1, lock_i2c);
+}
+
+int tda18271_charge_pump_source(struct dvb_frontend *fe,
+				enum tda18271_pll pll, int force)
+{
+	return __tda18271_charge_pump_source(fe, pll, force, true);
 }
 
+
 int tda18271_init_regs(struct dvb_frontend *fe)
 {
 	struct tda18271_priv *priv = fe->tuner_priv;

@@ -267,6 +295,13 @@ int tda18271_init_regs(struct dvb_frontend *fe)
 		i2c_adapter_id(priv->i2c_props.adap),
 		priv->i2c_props.addr);
 
+	/*
+	 * Don't let any other I2C transfer to happen at adapter during init,
+	 * as those could cause bad things
+	 */
+	tda18271_i2c_gate_ctrl(fe, 1);
+	i2c_lock_adapter(priv->i2c_props.adap);
+
 	/* initialize registers */
 	switch (priv->id) {
 	case TDA18271HDC1:

@@ -352,28 +387,28 @@ int tda18271_init_regs(struct dvb_frontend *fe)
 	regs[R_EB22] = 0x48;
 	regs[R_EB23] = 0xb0;
 
-	tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS);
+	__tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS, false);
 
 	/* setup agc1 gain */
 	regs[R_EB17] = 0x00;
-	tda18271_write_regs(fe, R_EB17, 1);
+	__tda18271_write_regs(fe, R_EB17, 1, false);
 	regs[R_EB17] = 0x03;
-	tda18271_write_regs(fe, R_EB17, 1);
+	__tda18271_write_regs(fe, R_EB17, 1, false);
 	regs[R_EB17] = 0x43;
-	tda18271_write_regs(fe, R_EB17, 1);
+	__tda18271_write_regs(fe, R_EB17, 1, false);
 	regs[R_EB17] = 0x4c;
-	tda18271_write_regs(fe, R_EB17, 1);
+	__tda18271_write_regs(fe, R_EB17, 1, false);
 
 	/* setup agc2 gain */
 	if ((priv->id) == TDA18271HDC1) {
 		regs[R_EB20] = 0xa0;
-		tda18271_write_regs(fe, R_EB20, 1);
+		__tda18271_write_regs(fe, R_EB20, 1, false);
 		regs[R_EB20] = 0xa7;
-		tda18271_write_regs(fe, R_EB20, 1);
+		__tda18271_write_regs(fe, R_EB20, 1, false);
 		regs[R_EB20] = 0xe7;
-		tda18271_write_regs(fe, R_EB20, 1);
+		__tda18271_write_regs(fe, R_EB20, 1, false);
 		regs[R_EB20] = 0xec;
-		tda18271_write_regs(fe, R_EB20, 1);
+		__tda18271_write_regs(fe, R_EB20, 1, false);
 	}
 
 	/* image rejection calibration */

@@ -391,21 +426,21 @@ int tda18271_init_regs(struct dvb_frontend *fe)
 	regs[R_MD2] = 0x08;
 	regs[R_MD3] = 0x00;
 
-	tda18271_write_regs(fe, R_EP3, 11);
+	__tda18271_write_regs(fe, R_EP3, 11, false);
 
 	if ((priv->id) == TDA18271HDC2) {
 		/* main pll cp source on */
-		tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1);
+		__tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1, false);
 		msleep(1);
 
 		/* main pll cp source off */
-		tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0);
+		__tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0, false);
 	}
 
 	msleep(5); /* pll locking */
 
 	/* launch detector */
-	tda18271_write_regs(fe, R_EP1, 1);
+	__tda18271_write_regs(fe, R_EP1, 1, false);
 	msleep(5); /* wanted low measurement */
 
 	regs[R_EP5] = 0x85;

@@ -413,11 +448,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
 	regs[R_CD1] = 0x66;
 	regs[R_CD2] = 0x70;
 
-	tda18271_write_regs(fe, R_EP3, 7);
+	__tda18271_write_regs(fe, R_EP3, 7, false);
 	msleep(5); /* pll locking */
 
 	/* launch optimization algorithm */
-	tda18271_write_regs(fe, R_EP2, 1);
+	__tda18271_write_regs(fe, R_EP2, 1, false);
 	msleep(30); /* image low optimization completion */
 
 	/* mid-band */

@@ -428,11 +463,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
 	regs[R_MD1] = 0x73;
 	regs[R_MD2] = 0x1a;
 
-	tda18271_write_regs(fe, R_EP3, 11);
+	__tda18271_write_regs(fe, R_EP3, 11, false);
 	msleep(5); /* pll locking */
 
 	/* launch detector */
-	tda18271_write_regs(fe, R_EP1, 1);
+	__tda18271_write_regs(fe, R_EP1, 1, false);
 	msleep(5); /* wanted mid measurement */
 
 	regs[R_EP5] = 0x86;

@@ -440,11 +475,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
 	regs[R_CD1] = 0x66;
 	regs[R_CD2] = 0xa0;
 
-	tda18271_write_regs(fe, R_EP3, 7);
+	__tda18271_write_regs(fe, R_EP3, 7, false);
 	msleep(5); /* pll locking */
 
 	/* launch optimization algorithm */
-	tda18271_write_regs(fe, R_EP2, 1);
+	__tda18271_write_regs(fe, R_EP2, 1, false);
 	msleep(30); /* image mid optimization completion */
 
 	/* high-band */

@@ -456,30 +491,33 @@ int tda18271_init_regs(struct dvb_frontend *fe)
 	regs[R_MD1] = 0x71;
 	regs[R_MD2] = 0xcd;
 
-	tda18271_write_regs(fe, R_EP3, 11);
+	__tda18271_write_regs(fe, R_EP3, 11, false);
 	msleep(5); /* pll locking */
 
 	/* launch detector */
-	tda18271_write_regs(fe, R_EP1, 1);
+	__tda18271_write_regs(fe, R_EP1, 1, false);
 	msleep(5); /* wanted high measurement */
 
 	regs[R_EP5] = 0x87;
 	regs[R_CD1] = 0x65;
 	regs[R_CD2] = 0x50;
 
-	tda18271_write_regs(fe, R_EP3, 7);
+	__tda18271_write_regs(fe, R_EP3, 7, false);
 	msleep(5); /* pll locking */
 
 	/* launch optimization algorithm */
-	tda18271_write_regs(fe, R_EP2, 1);
+	__tda18271_write_regs(fe, R_EP2, 1, false);
 	msleep(30); /* image high optimization completion */
 
 	/* return to normal mode */
 	regs[R_EP4] = 0x64;
-	tda18271_write_regs(fe, R_EP4, 1);
+	__tda18271_write_regs(fe, R_EP4, 1, false);
 
 	/* synchronize */
-	tda18271_write_regs(fe, R_EP1, 1);
+	__tda18271_write_regs(fe, R_EP1, 1, false);
+
+	i2c_unlock_adapter(priv->i2c_props.adap);
+	tda18271_i2c_gate_ctrl(fe, 0);
 
 	return 0;
 }

Signature · 1 line

1.7.11.4

Antti Palosaari

author

2012-09-28 18:31 UTC #21 lore raw

Re: [PATCH] tda18271-common: hold the I2C adapter during write transfers

Hello,
Did not fix the issue. Problem remains same. With the sleep + that patch it works still.

Quoted 10 lines · Mauro Carvalho Chehab

The tda18271 datasheet says:
"The image rejection calibration and RF tracking filter
calibration must be launched exactly as described in the flowchart, otherwise bad calibration or even blocking of the TDA18211HD can result making it impossible to communicate via the I2C-bus."
(yeah, tda18271 refers there to tda18211 - likely a typo at their
datasheets)

tda18211 is just same than tda18271 but without a analog.

Quoted 279 lines

That likely explains why sometimes tda18271 stops answering. That is now happening more often on designs with drx-k chips, as the firmware is now loaded asyncrousnly there.

While the above text doesn't explicitly tell that the I2C bus couldn't be used by other devices during such initialization, that seems to be a requirement there.

So, let's explicitly use the I2C lock there, avoiding I2C bus share during those critical moments.

Compile-tested only. Please test.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
  drivers/media/tuners/tda18271-common.c | 104 ++++++++++++++++++++++-----------
  1 file changed, 71 insertions(+), 33 deletions(-)

drivers/media/tuners/tda18271-common.c +71 ~~-33~~

index 221171e..18c77af 100644
@@ -187,7 +187,8 @@ int tda18271_read_extended(struct dvb_frontend *fe)
  	return (ret == 2 ? 0 : ret);
  }
 
-int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+static int __tda18271_write_regs(struct dvb_frontend *fe, int idx, int len,
+			bool lock_i2c)
  {
  	struct tda18271_priv *priv = fe->tuner_priv;
  	unsigned char *regs = priv->tda18271_regs;

@@ -198,7 +199,6 @@ int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
 
  	BUG_ON((len == 0) || (idx + len > sizeof(buf)));
 
-
  	switch (priv->small_i2c) {
  	case TDA18271_03_BYTE_CHUNK_INIT:
  		max = 3;

@@ -214,7 +214,19 @@ int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
  		max = 39;
  	}
 
-	tda18271_i2c_gate_ctrl(fe, 1);
+
+	/*
+	 * If lock_i2c is true, it will take the I2C bus for tda18271 private
+	 * usage during the entire write ops, as otherwise, bad things could
+	 * happen.
+	 * During device init, several write operations will happen. So,
+	 * tda18271_init_regs controls the I2C lock directly,
+	 * disabling lock_i2c here.
+	 */
+	if (lock_i2c) {
+		tda18271_i2c_gate_ctrl(fe, 1);
+		i2c_lock_adapter(priv->i2c_props.adap);
+	}
  	while (len) {
  		if (max > len)
  			max = len;

@@ -226,14 +238,17 @@ int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
  		msg.len = max + 1;
 
  		/* write registers */
-		ret = i2c_transfer(priv->i2c_props.adap, &msg, 1);
+		ret = __i2c_transfer(priv->i2c_props.adap, &msg, 1);
  		if (ret != 1)
  			break;
 
  		idx += max;
  		len -= max;
  	}
-	tda18271_i2c_gate_ctrl(fe, 0);
+	if (lock_i2c) {
+		i2c_unlock_adapter(priv->i2c_props.adap);
+		tda18271_i2c_gate_ctrl(fe, 0);
+	}
 
  	if (ret != 1)
  		tda_err("ERROR: idx = 0x%x, len = %d, "

@@ -242,10 +257,16 @@ int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
  	return (ret == 1 ? 0 : ret);
  }
 
+int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+{
+	return __tda18271_write_regs(fe, idx, len, true);
+}
+
  /*---------------------------------------------------------------------*/
 
-int tda18271_charge_pump_source(struct dvb_frontend *fe,
-				enum tda18271_pll pll, int force)
+static int __tda18271_charge_pump_source(struct dvb_frontend *fe,
+					 enum tda18271_pll pll, int force,
+					 bool lock_i2c)
  {
  	struct tda18271_priv *priv = fe->tuner_priv;
  	unsigned char *regs = priv->tda18271_regs;

@@ -255,9 +276,16 @@ int tda18271_charge_pump_source(struct dvb_frontend *fe,
  	regs[r_cp] &= ~0x20;
  	regs[r_cp] |= ((force & 1) << 5);
 
-	return tda18271_write_regs(fe, r_cp, 1);
+	return __tda18271_write_regs(fe, r_cp, 1, lock_i2c);
+}
+
+int tda18271_charge_pump_source(struct dvb_frontend *fe,
+				enum tda18271_pll pll, int force)
+{
+	return __tda18271_charge_pump_source(fe, pll, force, true);
  }
 
+
  int tda18271_init_regs(struct dvb_frontend *fe)
  {
  	struct tda18271_priv *priv = fe->tuner_priv;

@@ -267,6 +295,13 @@ int tda18271_init_regs(struct dvb_frontend *fe)
  		i2c_adapter_id(priv->i2c_props.adap),
  		priv->i2c_props.addr);
 
+	/*
+	 * Don't let any other I2C transfer to happen at adapter during init,
+	 * as those could cause bad things
+	 */
+	tda18271_i2c_gate_ctrl(fe, 1);
+	i2c_lock_adapter(priv->i2c_props.adap);
+
  	/* initialize registers */
  	switch (priv->id) {
  	case TDA18271HDC1:

@@ -352,28 +387,28 @@ int tda18271_init_regs(struct dvb_frontend *fe)
  	regs[R_EB22] = 0x48;
  	regs[R_EB23] = 0xb0;
 
-	tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS);
+	__tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS, false);
 
  	/* setup agc1 gain */
  	regs[R_EB17] = 0x00;
-	tda18271_write_regs(fe, R_EB17, 1);
+	__tda18271_write_regs(fe, R_EB17, 1, false);
  	regs[R_EB17] = 0x03;
-	tda18271_write_regs(fe, R_EB17, 1);
+	__tda18271_write_regs(fe, R_EB17, 1, false);
  	regs[R_EB17] = 0x43;
-	tda18271_write_regs(fe, R_EB17, 1);
+	__tda18271_write_regs(fe, R_EB17, 1, false);
  	regs[R_EB17] = 0x4c;
-	tda18271_write_regs(fe, R_EB17, 1);
+	__tda18271_write_regs(fe, R_EB17, 1, false);
 
  	/* setup agc2 gain */
  	if ((priv->id) == TDA18271HDC1) {
  		regs[R_EB20] = 0xa0;
-		tda18271_write_regs(fe, R_EB20, 1);
+		__tda18271_write_regs(fe, R_EB20, 1, false);
  		regs[R_EB20] = 0xa7;
-		tda18271_write_regs(fe, R_EB20, 1);
+		__tda18271_write_regs(fe, R_EB20, 1, false);
  		regs[R_EB20] = 0xe7;
-		tda18271_write_regs(fe, R_EB20, 1);
+		__tda18271_write_regs(fe, R_EB20, 1, false);
  		regs[R_EB20] = 0xec;
-		tda18271_write_regs(fe, R_EB20, 1);
+		__tda18271_write_regs(fe, R_EB20, 1, false);
  	}
 
  	/* image rejection calibration */

@@ -391,21 +426,21 @@ int tda18271_init_regs(struct dvb_frontend *fe)
  	regs[R_MD2] = 0x08;
  	regs[R_MD3] = 0x00;
 
-	tda18271_write_regs(fe, R_EP3, 11);
+	__tda18271_write_regs(fe, R_EP3, 11, false);
 
  	if ((priv->id) == TDA18271HDC2) {
  		/* main pll cp source on */
-		tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1);
+		__tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1, false);
  		msleep(1);
 
  		/* main pll cp source off */
-		tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0);
+		__tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0, false);
  	}
 
  	msleep(5); /* pll locking */
 
  	/* launch detector */
-	tda18271_write_regs(fe, R_EP1, 1);
+	__tda18271_write_regs(fe, R_EP1, 1, false);
  	msleep(5); /* wanted low measurement */
 
  	regs[R_EP5] = 0x85;

@@ -413,11 +448,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
  	regs[R_CD1] = 0x66;
  	regs[R_CD2] = 0x70;
 
-	tda18271_write_regs(fe, R_EP3, 7);
+	__tda18271_write_regs(fe, R_EP3, 7, false);
  	msleep(5); /* pll locking */
 
  	/* launch optimization algorithm */
-	tda18271_write_regs(fe, R_EP2, 1);
+	__tda18271_write_regs(fe, R_EP2, 1, false);
  	msleep(30); /* image low optimization completion */
 
  	/* mid-band */

@@ -428,11 +463,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
  	regs[R_MD1] = 0x73;
  	regs[R_MD2] = 0x1a;
 
-	tda18271_write_regs(fe, R_EP3, 11);
+	__tda18271_write_regs(fe, R_EP3, 11, false);
  	msleep(5); /* pll locking */
 
  	/* launch detector */
-	tda18271_write_regs(fe, R_EP1, 1);
+	__tda18271_write_regs(fe, R_EP1, 1, false);
  	msleep(5); /* wanted mid measurement */
 
  	regs[R_EP5] = 0x86;

@@ -440,11 +475,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
  	regs[R_CD1] = 0x66;
  	regs[R_CD2] = 0xa0;
 
-	tda18271_write_regs(fe, R_EP3, 7);
+	__tda18271_write_regs(fe, R_EP3, 7, false);
  	msleep(5); /* pll locking */
 
  	/* launch optimization algorithm */
-	tda18271_write_regs(fe, R_EP2, 1);
+	__tda18271_write_regs(fe, R_EP2, 1, false);
  	msleep(30); /* image mid optimization completion */
 
  	/* high-band */

@@ -456,30 +491,33 @@ int tda18271_init_regs(struct dvb_frontend *fe)
  	regs[R_MD1] = 0x71;
  	regs[R_MD2] = 0xcd;
 
-	tda18271_write_regs(fe, R_EP3, 11);
+	__tda18271_write_regs(fe, R_EP3, 11, false);
  	msleep(5); /* pll locking */
 
  	/* launch detector */
-	tda18271_write_regs(fe, R_EP1, 1);
+	__tda18271_write_regs(fe, R_EP1, 1, false);
  	msleep(5); /* wanted high measurement */
 
  	regs[R_EP5] = 0x87;
  	regs[R_CD1] = 0x65;
  	regs[R_CD2] = 0x50;
 
-	tda18271_write_regs(fe, R_EP3, 7);
+	__tda18271_write_regs(fe, R_EP3, 7, false);
  	msleep(5); /* pll locking */
 
  	/* launch optimization algorithm */
-	tda18271_write_regs(fe, R_EP2, 1);
+	__tda18271_write_regs(fe, R_EP2, 1, false);
  	msleep(30); /* image high optimization completion */
 
  	/* return to normal mode */
  	regs[R_EP4] = 0x64;
-	tda18271_write_regs(fe, R_EP4, 1);
+	__tda18271_write_regs(fe, R_EP4, 1, false);
 
  	/* synchronize */
-	tda18271_write_regs(fe, R_EP1, 1);
+	__tda18271_write_regs(fe, R_EP1, 1, false);
+
+	i2c_unlock_adapter(priv->i2c_props.adap);
+	tda18271_i2c_gate_ctrl(fe, 0);
 
  	return 0;
  }

Signature · 1 line

http://palosaari.fi/

Michael Krufky

maintainer

2012-09-28 18:56 UTC #22 lore raw

Re: [PATCH] tda18271-common: hold the I2C adapter during write transfers

Quoted 310 lines · Antti Palosaari

Hello,
Did not fix the issue. Problem remains same. With the sleep + that patch it works still.

Quoted 11 lines · Mauro Carvalho Chehab

The tda18271 datasheet says:
"The image rejection calibration and RF tracking filter
calibration must be launched exactly as described in the flowchart, otherwise bad calibration or even blocking of the TDA18211HD can result making it impossible to communicate via the I2C-bus."
(yeah, tda18271 refers there to tda18211 - likely a typo at their
datasheets)

tda18211 is just same than tda18271 but without a analog.

Quoted 290 lines

That likely explains why sometimes tda18271 stops answering. That is now happening more often on designs with drx-k chips, as the firmware is now loaded asyncrousnly there.

While the above text doesn't explicitly tell that the I2C bus couldn't be used by other devices during such initialization, that seems to be a requirement there.

So, let's explicitly use the I2C lock there, avoiding I2C bus share during those critical moments.

Compile-tested only. Please test.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
  drivers/media/tuners/tda18271-common.c | 104
++++++++++++++++++++++-----------
  1 file changed, 71 insertions(+), 33 deletions(-)

drivers/media/tuners/tda18271-common.c +2 -1

index 221171e..18c77af 100644
@@ -187,7 +187,8 @@ int tda18271_read_extended(struct dvb_frontend *fe)
        return (ret == 2 ? 0 : ret);
  }
 
-int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+static int __tda18271_write_regs(struct dvb_frontend *fe, int idx, int
 len,
+                       bool lock_i2c)
  {
        struct tda18271_priv *priv = fe->tuner_priv;

        unsigned char *regs = priv->tda18271_regs;

drivers/media/tuners/tda18271-common.c +0 -1

@@ -198,7 +199,6 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
 
        BUG_ON((len == 0) || (idx + len > sizeof(buf)));
 
-
        switch (priv->small_i2c) {
        case TDA18271_03_BYTE_CHUNK_INIT:

                max = 3;

drivers/media/tuners/tda18271-common.c +3 -1

@@ -214,7 +214,19 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
                max = 39;
        }
 
-       tda18271_i2c_gate_ctrl(fe, 1);
+
+       /*
+        * If lock_i2c is true, it will take the I2C bus for tda18271

private
+ * usage during the entire write ops, as otherwise, bad things
could

drivers/media/tuners/tda18271-common.c +14 -2

+        * happen.
+        * During device init, several write operations will happen. So,
+        * tda18271_init_regs controls the I2C lock directly,
+        * disabling lock_i2c here.
+        */
+       if (lock_i2c) {
+               tda18271_i2c_gate_ctrl(fe, 1);
+               i2c_lock_adapter(priv->i2c_props.adap);
+       }
        while (len) {
                if (max > len)
                        max = len;

@@ -226,14 +238,17 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
                msg.len = max + 1;
 
                /* write registers */
-               ret = i2c_transfer(priv->i2c_props.adap, &msg, 1);
+               ret = __i2c_transfer(priv->i2c_props.adap, &msg, 1);
                if (ret != 1)
                        break;
 
                idx += max;
                len -= max;
        }
-       tda18271_i2c_gate_ctrl(fe, 0);
+       if (lock_i2c) {
+               i2c_unlock_adapter(priv->i2c_props.adap);
+               tda18271_i2c_gate_ctrl(fe, 0);
+       }
 
        if (ret != 1)

                tda_err("ERROR: idx = 0x%x, len = %d, "

drivers/media/tuners/tda18271-common.c +8 -2

@@ -242,10 +257,16 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
        return (ret == 1 ? 0 : ret);
  }
 
+int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+{
+       return __tda18271_write_regs(fe, idx, len, true);
+}
+
 
 /*---------------------------------------------------------------------*/
 
-int tda18271_charge_pump_source(struct dvb_frontend *fe,
-                               enum tda18271_pll pll, int force)
+static int __tda18271_charge_pump_source(struct dvb_frontend *fe,
+                                        enum tda18271_pll pll, int force,
+                                        bool lock_i2c)
  {

        struct tda18271_priv *priv = fe->tuner_priv;
        unsigned char *regs = priv->tda18271_regs;

drivers/media/tuners/tda18271-common.c +8 -1

@@ -255,9 +276,16 @@ int tda18271_charge_pump_source(struct dvb_frontend
 *fe,
        regs[r_cp] &= ~0x20;
        regs[r_cp] |= ((force & 1) << 5);
 
-       return tda18271_write_regs(fe, r_cp, 1);
+       return __tda18271_write_regs(fe, r_cp, 1, lock_i2c);
+}
+
+int tda18271_charge_pump_source(struct dvb_frontend *fe,
+                               enum tda18271_pll pll, int force)
+{
+       return __tda18271_charge_pump_source(fe, pll, force, true);
  }
 
+
  int tda18271_init_regs(struct dvb_frontend *fe)
  {

        struct tda18271_priv *priv = fe->tuner_priv;

drivers/media/tuners/tda18271-common.c +7 -0

@@ -267,6 +295,13 @@ int tda18271_init_regs(struct dvb_frontend *fe)
                i2c_adapter_id(priv->i2c_props.adap),
                priv->i2c_props.addr);
 
+       /*
+        * Don't let any other I2C transfer to happen at adapter during
 init,
+        * as those could cause bad things
+        */
+       tda18271_i2c_gate_ctrl(fe, 1);
+       i2c_lock_adapter(priv->i2c_props.adap);
+
        /* initialize registers */
        switch (priv->id) {

        case TDA18271HDC1:

drivers/media/tuners/tda18271-common.c +13 ~~-13~~

@@ -352,28 +387,28 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_EB22] = 0x48;
        regs[R_EB23] = 0xb0;
 
-       tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS);
+       __tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS, false);
 
        /* setup agc1 gain */
        regs[R_EB17] = 0x00;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x03;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x43;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x4c;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
 
        /* setup agc2 gain */
        if ((priv->id) == TDA18271HDC1) {
                regs[R_EB20] = 0xa0;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xa7;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xe7;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xec;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
        }
 
        /* image rejection calibration */

@@ -391,21 +426,21 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD2] = 0x08;
        regs[R_MD3] = 0x00;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
 
        if ((priv->id) == TDA18271HDC2) {
                /* main pll cp source on */
-               tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1);
+               __tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1,
 false);
                msleep(1);
 
                /* main pll cp source off */
-               tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0);
+               __tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0,
 false);
        }
 
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted low measurement */

        regs[R_EP5] = 0x85;

drivers/media/tuners/tda18271-common.c +15 ~~-12~~

@@ -413,11 +448,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_CD1] = 0x66;
        regs[R_CD2] = 0x70;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image low optimization completion */
 
        /* mid-band */

@@ -428,11 +463,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD1] = 0x73;
        regs[R_MD2] = 0x1a;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted mid measurement */
 
        regs[R_EP5] = 0x86;

@@ -440,11 +475,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_CD1] = 0x66;
        regs[R_CD2] = 0xa0;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image mid optimization completion */
 
        /* high-band */

@@ -456,30 +491,33 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD1] = 0x71;
        regs[R_MD2] = 0xcd;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted high measurement */
 
        regs[R_EP5] = 0x87;
        regs[R_CD1] = 0x65;
        regs[R_CD2] = 0x50;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image high optimization completion */
 
        /* return to normal mode */
        regs[R_EP4] = 0x64;
-       tda18271_write_regs(fe, R_EP4, 1);
+       __tda18271_write_regs(fe, R_EP4, 1, false);
 
        /* synchronize */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
+
+       i2c_unlock_adapter(priv->i2c_props.adap);
+       tda18271_i2c_gate_ctrl(fe, 0);

Quoted 7 lines

drivers/media/tuners/tda18271-common.c int tda18271_init_regs(struct dvb_frontend *fe) · line 523
Quoted 3 lines
drivers/media/tuners/tda18271-common.c +0 -0
        return 0;
  }
--
http://palosaari.fi/

I have to NACK this particular patch -- I saw Mauro's email about the locked i2c -- it's a great idea. In fact, it is the original use case for adding this functionality into i2c, I just never got around to
implementing it in the tda18271 driver. However, it shouldnt be
around *all* i2c transactions -- it should only lock the i2c bus during the *critical* section. Please wait for me to send a new patch for testing. I'll try to get to it before the end of the weekend. (hopefully tonight or tomorrow morning)

-Mike

Michael Krufky

maintainer

2012-09-29 19:20 UTC #23 lore raw

Re: [PATCH] tda18271-common: hold the I2C adapter during write transfers

Quoted 319 lines · Michael Krufky

Quoted 310 lines · Antti Palosaari

Hello,
Did not fix the issue. Problem remains same. With the sleep + that patch it works still.

Quoted 11 lines · Mauro Carvalho Chehab

The tda18271 datasheet says:
"The image rejection calibration and RF tracking filter
calibration must be launched exactly as described in the flowchart, otherwise bad calibration or even blocking of the TDA18211HD can result making it impossible to communicate via the I2C-bus."
(yeah, tda18271 refers there to tda18211 - likely a typo at their
datasheets)

tda18211 is just same than tda18271 but without a analog.

Quoted 290 lines

That likely explains why sometimes tda18271 stops answering. That is now happening more often on designs with drx-k chips, as the firmware is now loaded asyncrousnly there.

While the above text doesn't explicitly tell that the I2C bus couldn't be used by other devices during such initialization, that seems to be a requirement there.

So, let's explicitly use the I2C lock there, avoiding I2C bus share during those critical moments.

Compile-tested only. Please test.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
  drivers/media/tuners/tda18271-common.c | 104
++++++++++++++++++++++-----------
  1 file changed, 71 insertions(+), 33 deletions(-)

drivers/media/tuners/tda18271-common.c +2 -1

index 221171e..18c77af 100644
@@ -187,7 +187,8 @@ int tda18271_read_extended(struct dvb_frontend *fe)
        return (ret == 2 ? 0 : ret);
  }
 
-int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+static int __tda18271_write_regs(struct dvb_frontend *fe, int idx, int
 len,
+                       bool lock_i2c)
  {
        struct tda18271_priv *priv = fe->tuner_priv;

        unsigned char *regs = priv->tda18271_regs;

drivers/media/tuners/tda18271-common.c +0 -1

@@ -198,7 +199,6 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
 
        BUG_ON((len == 0) || (idx + len > sizeof(buf)));
 
-
        switch (priv->small_i2c) {
        case TDA18271_03_BYTE_CHUNK_INIT:

                max = 3;

drivers/media/tuners/tda18271-common.c +3 -1

@@ -214,7 +214,19 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
                max = 39;
        }
 
-       tda18271_i2c_gate_ctrl(fe, 1);
+
+       /*
+        * If lock_i2c is true, it will take the I2C bus for tda18271

private
+ * usage during the entire write ops, as otherwise, bad things
could

drivers/media/tuners/tda18271-common.c +14 -2

+        * happen.
+        * During device init, several write operations will happen. So,
+        * tda18271_init_regs controls the I2C lock directly,
+        * disabling lock_i2c here.
+        */
+       if (lock_i2c) {
+               tda18271_i2c_gate_ctrl(fe, 1);
+               i2c_lock_adapter(priv->i2c_props.adap);
+       }
        while (len) {
                if (max > len)
                        max = len;

@@ -226,14 +238,17 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
                msg.len = max + 1;
 
                /* write registers */
-               ret = i2c_transfer(priv->i2c_props.adap, &msg, 1);
+               ret = __i2c_transfer(priv->i2c_props.adap, &msg, 1);
                if (ret != 1)
                        break;
 
                idx += max;
                len -= max;
        }
-       tda18271_i2c_gate_ctrl(fe, 0);
+       if (lock_i2c) {
+               i2c_unlock_adapter(priv->i2c_props.adap);
+               tda18271_i2c_gate_ctrl(fe, 0);
+       }
 
        if (ret != 1)

                tda_err("ERROR: idx = 0x%x, len = %d, "

drivers/media/tuners/tda18271-common.c +8 -2

@@ -242,10 +257,16 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
        return (ret == 1 ? 0 : ret);
  }
 
+int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+{
+       return __tda18271_write_regs(fe, idx, len, true);
+}
+
 
 /*---------------------------------------------------------------------*/
 
-int tda18271_charge_pump_source(struct dvb_frontend *fe,
-                               enum tda18271_pll pll, int force)
+static int __tda18271_charge_pump_source(struct dvb_frontend *fe,
+                                        enum tda18271_pll pll, int force,
+                                        bool lock_i2c)
  {

        struct tda18271_priv *priv = fe->tuner_priv;
        unsigned char *regs = priv->tda18271_regs;

drivers/media/tuners/tda18271-common.c +8 -1

@@ -255,9 +276,16 @@ int tda18271_charge_pump_source(struct dvb_frontend
 *fe,
        regs[r_cp] &= ~0x20;
        regs[r_cp] |= ((force & 1) << 5);
 
-       return tda18271_write_regs(fe, r_cp, 1);
+       return __tda18271_write_regs(fe, r_cp, 1, lock_i2c);
+}
+
+int tda18271_charge_pump_source(struct dvb_frontend *fe,
+                               enum tda18271_pll pll, int force)
+{
+       return __tda18271_charge_pump_source(fe, pll, force, true);
  }
 
+
  int tda18271_init_regs(struct dvb_frontend *fe)
  {

        struct tda18271_priv *priv = fe->tuner_priv;

drivers/media/tuners/tda18271-common.c +7 -0

@@ -267,6 +295,13 @@ int tda18271_init_regs(struct dvb_frontend *fe)
                i2c_adapter_id(priv->i2c_props.adap),
                priv->i2c_props.addr);
 
+       /*
+        * Don't let any other I2C transfer to happen at adapter during
 init,
+        * as those could cause bad things
+        */
+       tda18271_i2c_gate_ctrl(fe, 1);
+       i2c_lock_adapter(priv->i2c_props.adap);
+
        /* initialize registers */
        switch (priv->id) {

        case TDA18271HDC1:

drivers/media/tuners/tda18271-common.c +13 ~~-13~~

@@ -352,28 +387,28 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_EB22] = 0x48;
        regs[R_EB23] = 0xb0;
 
-       tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS);
+       __tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS, false);
 
        /* setup agc1 gain */
        regs[R_EB17] = 0x00;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x03;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x43;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x4c;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
 
        /* setup agc2 gain */
        if ((priv->id) == TDA18271HDC1) {
                regs[R_EB20] = 0xa0;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xa7;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xe7;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xec;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
        }
 
        /* image rejection calibration */

@@ -391,21 +426,21 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD2] = 0x08;
        regs[R_MD3] = 0x00;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
 
        if ((priv->id) == TDA18271HDC2) {
                /* main pll cp source on */
-               tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1);
+               __tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1,
 false);
                msleep(1);
 
                /* main pll cp source off */
-               tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0);
+               __tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0,
 false);
        }
 
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted low measurement */

        regs[R_EP5] = 0x85;

drivers/media/tuners/tda18271-common.c +15 ~~-12~~

@@ -413,11 +448,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_CD1] = 0x66;
        regs[R_CD2] = 0x70;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image low optimization completion */
 
        /* mid-band */

@@ -428,11 +463,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD1] = 0x73;
        regs[R_MD2] = 0x1a;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted mid measurement */
 
        regs[R_EP5] = 0x86;

@@ -440,11 +475,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_CD1] = 0x66;
        regs[R_CD2] = 0xa0;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image mid optimization completion */
 
        /* high-band */

@@ -456,30 +491,33 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD1] = 0x71;
        regs[R_MD2] = 0xcd;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted high measurement */
 
        regs[R_EP5] = 0x87;
        regs[R_CD1] = 0x65;
        regs[R_CD2] = 0x50;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image high optimization completion */
 
        /* return to normal mode */
        regs[R_EP4] = 0x64;
-       tda18271_write_regs(fe, R_EP4, 1);
+       __tda18271_write_regs(fe, R_EP4, 1, false);
 
        /* synchronize */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
+
+       i2c_unlock_adapter(priv->i2c_props.adap);
+       tda18271_i2c_gate_ctrl(fe, 0);

Quoted 7 lines

drivers/media/tuners/tda18271-common.c int tda18271_init_regs(struct dvb_frontend *fe) · line 523
Quoted 3 lines
drivers/media/tuners/tda18271-common.c +0 -0
        return 0;
  }
--
http://palosaari.fi/

Quoted 8 lines

I have to NACK this particular patch -- I saw Mauro's email about the locked i2c -- it's a great idea. In fact, it is the original use case for adding this functionality into i2c, I just never got around to
implementing it in the tda18271 driver. However, it shouldnt be
around *all* i2c transactions -- it should only lock the i2c bus during the *critical* section. Please wait for me to send a new patch for testing. I'll try to get to it before the end of the weekend. (hopefully tonight or tomorrow morning)

On further inspection of the patch, I see that it *does* attempt to only lock the i2c bus during the initialization of the device. The patch could be optimized a bit to specifically only lock the bus during the critical section of the initialization itself, but that is no reason to block this patch. So, I retract my NACK. If we decide to merge this, then we can optimize it afterwards.

-Mike

Mauro Carvalho Chehab

2012-10-07 12:42 UTC #24 lore raw

Re: [PATCH] tda18271-common: hold the I2C adapter during write transfers

Em Sat, 29 Sep 2012 15:20:23 -0400
Michael Krufky <[email protected]> escreveu:

Quoted 329 lines · Michael Krufky

Quoted 319 lines · Antti Palosaari

Quoted 310 lines

Hello,
Did not fix the issue. Problem remains same. With the sleep + that patch it works still.

Quoted 11 lines · Mauro Carvalho Chehab

The tda18271 datasheet says:
"The image rejection calibration and RF tracking filter
calibration must be launched exactly as described in the flowchart, otherwise bad calibration or even blocking of the TDA18211HD can result making it impossible to communicate via the I2C-bus."
(yeah, tda18271 refers there to tda18211 - likely a typo at their
datasheets)

tda18211 is just same than tda18271 but without a analog.

Quoted 290 lines

That likely explains why sometimes tda18271 stops answering. That is now happening more often on designs with drx-k chips, as the firmware is now loaded asyncrousnly there.

While the above text doesn't explicitly tell that the I2C bus couldn't be used by other devices during such initialization, that seems to be a requirement there.

So, let's explicitly use the I2C lock there, avoiding I2C bus share during those critical moments.

Compile-tested only. Please test.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
  drivers/media/tuners/tda18271-common.c | 104
++++++++++++++++++++++-----------
  1 file changed, 71 insertions(+), 33 deletions(-)

drivers/media/tuners/tda18271-common.c +2 -1

index 221171e..18c77af 100644
@@ -187,7 +187,8 @@ int tda18271_read_extended(struct dvb_frontend *fe)
        return (ret == 2 ? 0 : ret);
  }
 
-int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+static int __tda18271_write_regs(struct dvb_frontend *fe, int idx, int
 len,
+                       bool lock_i2c)
  {
        struct tda18271_priv *priv = fe->tuner_priv;

        unsigned char *regs = priv->tda18271_regs;

drivers/media/tuners/tda18271-common.c +0 -1

@@ -198,7 +199,6 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
 
        BUG_ON((len == 0) || (idx + len > sizeof(buf)));
 
-
        switch (priv->small_i2c) {
        case TDA18271_03_BYTE_CHUNK_INIT:

                max = 3;

drivers/media/tuners/tda18271-common.c +3 -1

@@ -214,7 +214,19 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
                max = 39;
        }
 
-       tda18271_i2c_gate_ctrl(fe, 1);
+
+       /*
+        * If lock_i2c is true, it will take the I2C bus for tda18271

private
+ * usage during the entire write ops, as otherwise, bad things
could

drivers/media/tuners/tda18271-common.c +14 -2

+        * happen.
+        * During device init, several write operations will happen. So,
+        * tda18271_init_regs controls the I2C lock directly,
+        * disabling lock_i2c here.
+        */
+       if (lock_i2c) {
+               tda18271_i2c_gate_ctrl(fe, 1);
+               i2c_lock_adapter(priv->i2c_props.adap);
+       }
        while (len) {
                if (max > len)
                        max = len;

@@ -226,14 +238,17 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
                msg.len = max + 1;
 
                /* write registers */
-               ret = i2c_transfer(priv->i2c_props.adap, &msg, 1);
+               ret = __i2c_transfer(priv->i2c_props.adap, &msg, 1);
                if (ret != 1)
                        break;
 
                idx += max;
                len -= max;
        }
-       tda18271_i2c_gate_ctrl(fe, 0);
+       if (lock_i2c) {
+               i2c_unlock_adapter(priv->i2c_props.adap);
+               tda18271_i2c_gate_ctrl(fe, 0);
+       }
 
        if (ret != 1)

                tda_err("ERROR: idx = 0x%x, len = %d, "

drivers/media/tuners/tda18271-common.c +8 -2

@@ -242,10 +257,16 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
        return (ret == 1 ? 0 : ret);
  }
 
+int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+{
+       return __tda18271_write_regs(fe, idx, len, true);
+}
+
 
 /*---------------------------------------------------------------------*/
 
-int tda18271_charge_pump_source(struct dvb_frontend *fe,
-                               enum tda18271_pll pll, int force)
+static int __tda18271_charge_pump_source(struct dvb_frontend *fe,
+                                        enum tda18271_pll pll, int force,
+                                        bool lock_i2c)
  {

        struct tda18271_priv *priv = fe->tuner_priv;
        unsigned char *regs = priv->tda18271_regs;

drivers/media/tuners/tda18271-common.c +8 -1

@@ -255,9 +276,16 @@ int tda18271_charge_pump_source(struct dvb_frontend
 *fe,
        regs[r_cp] &= ~0x20;
        regs[r_cp] |= ((force & 1) << 5);
 
-       return tda18271_write_regs(fe, r_cp, 1);
+       return __tda18271_write_regs(fe, r_cp, 1, lock_i2c);
+}
+
+int tda18271_charge_pump_source(struct dvb_frontend *fe,
+                               enum tda18271_pll pll, int force)
+{
+       return __tda18271_charge_pump_source(fe, pll, force, true);
  }
 
+
  int tda18271_init_regs(struct dvb_frontend *fe)
  {

        struct tda18271_priv *priv = fe->tuner_priv;

drivers/media/tuners/tda18271-common.c +7 -0

@@ -267,6 +295,13 @@ int tda18271_init_regs(struct dvb_frontend *fe)
                i2c_adapter_id(priv->i2c_props.adap),
                priv->i2c_props.addr);
 
+       /*
+        * Don't let any other I2C transfer to happen at adapter during
 init,
+        * as those could cause bad things
+        */
+       tda18271_i2c_gate_ctrl(fe, 1);
+       i2c_lock_adapter(priv->i2c_props.adap);
+
        /* initialize registers */
        switch (priv->id) {

        case TDA18271HDC1:

drivers/media/tuners/tda18271-common.c +13 ~~-13~~

@@ -352,28 +387,28 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_EB22] = 0x48;
        regs[R_EB23] = 0xb0;
 
-       tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS);
+       __tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS, false);
 
        /* setup agc1 gain */
        regs[R_EB17] = 0x00;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x03;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x43;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x4c;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
 
        /* setup agc2 gain */
        if ((priv->id) == TDA18271HDC1) {
                regs[R_EB20] = 0xa0;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xa7;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xe7;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xec;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
        }
 
        /* image rejection calibration */

@@ -391,21 +426,21 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD2] = 0x08;
        regs[R_MD3] = 0x00;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
 
        if ((priv->id) == TDA18271HDC2) {
                /* main pll cp source on */
-               tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1);
+               __tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1,
 false);
                msleep(1);
 
                /* main pll cp source off */
-               tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0);
+               __tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0,
 false);
        }
 
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted low measurement */

        regs[R_EP5] = 0x85;

drivers/media/tuners/tda18271-common.c +15 ~~-12~~

@@ -413,11 +448,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_CD1] = 0x66;
        regs[R_CD2] = 0x70;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image low optimization completion */
 
        /* mid-band */

@@ -428,11 +463,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD1] = 0x73;
        regs[R_MD2] = 0x1a;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted mid measurement */
 
        regs[R_EP5] = 0x86;

@@ -440,11 +475,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_CD1] = 0x66;
        regs[R_CD2] = 0xa0;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image mid optimization completion */
 
        /* high-band */

@@ -456,30 +491,33 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD1] = 0x71;
        regs[R_MD2] = 0xcd;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted high measurement */
 
        regs[R_EP5] = 0x87;
        regs[R_CD1] = 0x65;
        regs[R_CD2] = 0x50;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image high optimization completion */
 
        /* return to normal mode */
        regs[R_EP4] = 0x64;
-       tda18271_write_regs(fe, R_EP4, 1);
+       __tda18271_write_regs(fe, R_EP4, 1, false);
 
        /* synchronize */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
+
+       i2c_unlock_adapter(priv->i2c_props.adap);
+       tda18271_i2c_gate_ctrl(fe, 0);

Quoted 7 lines

drivers/media/tuners/tda18271-common.c int tda18271_init_regs(struct dvb_frontend *fe) · line 523
Quoted 3 lines
drivers/media/tuners/tda18271-common.c +0 -0
        return 0;
  }
--
http://palosaari.fi/

Quoted 8 lines

I have to NACK this particular patch -- I saw Mauro's email about the locked i2c -- it's a great idea. In fact, it is the original use case for adding this functionality into i2c, I just never got around to
implementing it in the tda18271 driver. However, it shouldnt be
around *all* i2c transactions -- it should only lock the i2c bus during the *critical* section. Please wait for me to send a new patch for testing. I'll try to get to it before the end of the weekend. (hopefully tonight or tomorrow morning)

Quoted 5 lines

On further inspection of the patch, I see that it *does* attempt to only lock the i2c bus during the initialization of the device. The patch could be optimized a bit to specifically only lock the bus during the critical section of the initialization itself, but that is no reason to block this patch.

Yesh, I took the care of not locking all transactions, locking just the init code.

Quoted 1 line

So, I retract my NACK.

Ok, I'll apply it then.

Quoted 2 lines

If we decide
to merge this, then we can optimize it afterwards.

Yeah, for sure optimization there is very welcome.

Quoted 6 lines

-Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Regards,
Mauro

Michael Krufky

maintainer

2012-10-07 13:18 UTC #25 lore raw

Re: [PATCH] tda18271-common: hold the I2C adapter during write transfers

Quoted 341 lines · Mauro Carvalho Chehab

Em Sat, 29 Sep 2012 15:20:23 -0400
Michael Krufky <[email protected]> escreveu:

Quoted 329 lines · Michael Krufky

Quoted 319 lines · Antti Palosaari

Quoted 310 lines

Hello,
Did not fix the issue. Problem remains same. With the sleep + that patch it works still.

Quoted 11 lines · Mauro Carvalho Chehab

The tda18271 datasheet says:
"The image rejection calibration and RF tracking filter
calibration must be launched exactly as described in the flowchart, otherwise bad calibration or even blocking of the TDA18211HD can result making it impossible to communicate via the I2C-bus."
(yeah, tda18271 refers there to tda18211 - likely a typo at their
datasheets)

tda18211 is just same than tda18271 but without a analog.

Quoted 290 lines

That likely explains why sometimes tda18271 stops answering. That is now happening more often on designs with drx-k chips, as the firmware is now loaded asyncrousnly there.

While the above text doesn't explicitly tell that the I2C bus couldn't be used by other devices during such initialization, that seems to be a requirement there.

So, let's explicitly use the I2C lock there, avoiding I2C bus share during those critical moments.

Compile-tested only. Please test.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
  drivers/media/tuners/tda18271-common.c | 104
++++++++++++++++++++++-----------
  1 file changed, 71 insertions(+), 33 deletions(-)

drivers/media/tuners/tda18271-common.c +2 -1

index 221171e..18c77af 100644
@@ -187,7 +187,8 @@ int tda18271_read_extended(struct dvb_frontend *fe)
        return (ret == 2 ? 0 : ret);
  }
 
-int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+static int __tda18271_write_regs(struct dvb_frontend *fe, int idx, int
 len,
+                       bool lock_i2c)
  {
        struct tda18271_priv *priv = fe->tuner_priv;

        unsigned char *regs = priv->tda18271_regs;

drivers/media/tuners/tda18271-common.c +0 -1

@@ -198,7 +199,6 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
 
        BUG_ON((len == 0) || (idx + len > sizeof(buf)));
 
-
        switch (priv->small_i2c) {
        case TDA18271_03_BYTE_CHUNK_INIT:

                max = 3;

drivers/media/tuners/tda18271-common.c +3 -1

@@ -214,7 +214,19 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
                max = 39;
        }
 
-       tda18271_i2c_gate_ctrl(fe, 1);
+
+       /*
+        * If lock_i2c is true, it will take the I2C bus for tda18271

private
+ * usage during the entire write ops, as otherwise, bad things
could

drivers/media/tuners/tda18271-common.c +14 -2

+        * happen.
+        * During device init, several write operations will happen. So,
+        * tda18271_init_regs controls the I2C lock directly,
+        * disabling lock_i2c here.
+        */
+       if (lock_i2c) {
+               tda18271_i2c_gate_ctrl(fe, 1);
+               i2c_lock_adapter(priv->i2c_props.adap);
+       }
        while (len) {
                if (max > len)
                        max = len;

@@ -226,14 +238,17 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
                msg.len = max + 1;
 
                /* write registers */
-               ret = i2c_transfer(priv->i2c_props.adap, &msg, 1);
+               ret = __i2c_transfer(priv->i2c_props.adap, &msg, 1);
                if (ret != 1)
                        break;
 
                idx += max;
                len -= max;
        }
-       tda18271_i2c_gate_ctrl(fe, 0);
+       if (lock_i2c) {
+               i2c_unlock_adapter(priv->i2c_props.adap);
+               tda18271_i2c_gate_ctrl(fe, 0);
+       }
 
        if (ret != 1)

                tda_err("ERROR: idx = 0x%x, len = %d, "

drivers/media/tuners/tda18271-common.c +8 -2

@@ -242,10 +257,16 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
 idx, int len)
        return (ret == 1 ? 0 : ret);
  }
 
+int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
+{
+       return __tda18271_write_regs(fe, idx, len, true);
+}
+
 
 /*---------------------------------------------------------------------*/
 
-int tda18271_charge_pump_source(struct dvb_frontend *fe,
-                               enum tda18271_pll pll, int force)
+static int __tda18271_charge_pump_source(struct dvb_frontend *fe,
+                                        enum tda18271_pll pll, int force,
+                                        bool lock_i2c)
  {

        struct tda18271_priv *priv = fe->tuner_priv;
        unsigned char *regs = priv->tda18271_regs;

drivers/media/tuners/tda18271-common.c +8 -1

@@ -255,9 +276,16 @@ int tda18271_charge_pump_source(struct dvb_frontend
 *fe,
        regs[r_cp] &= ~0x20;
        regs[r_cp] |= ((force & 1) << 5);
 
-       return tda18271_write_regs(fe, r_cp, 1);
+       return __tda18271_write_regs(fe, r_cp, 1, lock_i2c);
+}
+
+int tda18271_charge_pump_source(struct dvb_frontend *fe,
+                               enum tda18271_pll pll, int force)
+{
+       return __tda18271_charge_pump_source(fe, pll, force, true);
  }
 
+
  int tda18271_init_regs(struct dvb_frontend *fe)
  {

        struct tda18271_priv *priv = fe->tuner_priv;

drivers/media/tuners/tda18271-common.c +7 -0

@@ -267,6 +295,13 @@ int tda18271_init_regs(struct dvb_frontend *fe)
                i2c_adapter_id(priv->i2c_props.adap),
                priv->i2c_props.addr);
 
+       /*
+        * Don't let any other I2C transfer to happen at adapter during
 init,
+        * as those could cause bad things
+        */
+       tda18271_i2c_gate_ctrl(fe, 1);
+       i2c_lock_adapter(priv->i2c_props.adap);
+
        /* initialize registers */
        switch (priv->id) {

        case TDA18271HDC1:

drivers/media/tuners/tda18271-common.c +13 ~~-13~~

@@ -352,28 +387,28 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_EB22] = 0x48;
        regs[R_EB23] = 0xb0;
 
-       tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS);
+       __tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS, false);
 
        /* setup agc1 gain */
        regs[R_EB17] = 0x00;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x03;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x43;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
        regs[R_EB17] = 0x4c;
-       tda18271_write_regs(fe, R_EB17, 1);
+       __tda18271_write_regs(fe, R_EB17, 1, false);
 
        /* setup agc2 gain */
        if ((priv->id) == TDA18271HDC1) {
                regs[R_EB20] = 0xa0;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xa7;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xe7;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
                regs[R_EB20] = 0xec;
-               tda18271_write_regs(fe, R_EB20, 1);
+               __tda18271_write_regs(fe, R_EB20, 1, false);
        }
 
        /* image rejection calibration */

@@ -391,21 +426,21 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD2] = 0x08;
        regs[R_MD3] = 0x00;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
 
        if ((priv->id) == TDA18271HDC2) {
                /* main pll cp source on */
-               tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1);
+               __tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1,
 false);
                msleep(1);
 
                /* main pll cp source off */
-               tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0);
+               __tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0,
 false);
        }
 
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted low measurement */

        regs[R_EP5] = 0x85;

drivers/media/tuners/tda18271-common.c +15 ~~-12~~

@@ -413,11 +448,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_CD1] = 0x66;
        regs[R_CD2] = 0x70;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image low optimization completion */
 
        /* mid-band */

@@ -428,11 +463,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD1] = 0x73;
        regs[R_MD2] = 0x1a;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted mid measurement */
 
        regs[R_EP5] = 0x86;

@@ -440,11 +475,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_CD1] = 0x66;
        regs[R_CD2] = 0xa0;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image mid optimization completion */
 
        /* high-band */

@@ -456,30 +491,33 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_MD1] = 0x71;
        regs[R_MD2] = 0xcd;
 
-       tda18271_write_regs(fe, R_EP3, 11);
+       __tda18271_write_regs(fe, R_EP3, 11, false);
        msleep(5); /* pll locking */
 
        /* launch detector */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
        msleep(5); /* wanted high measurement */
 
        regs[R_EP5] = 0x87;
        regs[R_CD1] = 0x65;
        regs[R_CD2] = 0x50;
 
-       tda18271_write_regs(fe, R_EP3, 7);
+       __tda18271_write_regs(fe, R_EP3, 7, false);
        msleep(5); /* pll locking */
 
        /* launch optimization algorithm */
-       tda18271_write_regs(fe, R_EP2, 1);
+       __tda18271_write_regs(fe, R_EP2, 1, false);
        msleep(30); /* image high optimization completion */
 
        /* return to normal mode */
        regs[R_EP4] = 0x64;
-       tda18271_write_regs(fe, R_EP4, 1);
+       __tda18271_write_regs(fe, R_EP4, 1, false);
 
        /* synchronize */
-       tda18271_write_regs(fe, R_EP1, 1);
+       __tda18271_write_regs(fe, R_EP1, 1, false);
+
+       i2c_unlock_adapter(priv->i2c_props.adap);
+       tda18271_i2c_gate_ctrl(fe, 0);

Quoted 7 lines

drivers/media/tuners/tda18271-common.c int tda18271_init_regs(struct dvb_frontend *fe) · line 523
Quoted 3 lines
drivers/media/tuners/tda18271-common.c +0 -0
        return 0;
  }
--
http://palosaari.fi/

Quoted 8 lines

I have to NACK this particular patch -- I saw Mauro's email about the locked i2c -- it's a great idea. In fact, it is the original use case for adding this functionality into i2c, I just never got around to
implementing it in the tda18271 driver. However, it shouldnt be
around *all* i2c transactions -- it should only lock the i2c bus during the *critical* section. Please wait for me to send a new patch for testing. I'll try to get to it before the end of the weekend. (hopefully tonight or tomorrow morning)

Quoted 5 lines

On further inspection of the patch, I see that it *does* attempt to only lock the i2c bus during the initialization of the device. The patch could be optimized a bit to specifically only lock the bus during the critical section of the initialization itself, but that is no reason to block this patch.

Yesh, I took the care of not locking all transactions, locking just the init code.

Quoted 15 lines

Quoted 1 line
So, I retract my NACK.
Ok, I'll apply it then.
Quoted 2 lines
If we decide
to merge this, then we can optimize it afterwards.
Yeah, for sure optimization there is very welcome.
Quoted 6 lines
-Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mauro,

If the patch isn't needed then lets not apply it -- i don't think it's entirely necessary, and it adds more locking that actually required.

-Mike

Mauro Carvalho Chehab

2012-10-01 10:42 UTC #26 lore raw

Re: [PATCH] tda18271-common: hold the I2C adapter during write transfers

In reply to Antti Palosaari

Em Fri, 28 Sep 2012 21:31:07 +0300
Antti Palosaari <[email protected]> escreveu:

Quoted 2 lines

Hello,
Did not fix the issue. Problem remains same.

Ok, that's what I was afraid: there's likely something at drxk firmware that it is needed for tda18271 to be visible - maybe gpio settings.

Quoted 2 lines

With the sleep + that patch
it works still.

Good, no regressions added.

IMO, we should add a defer job at dvb_attach, that will postpone the tuner attach to happen after drxk firmware is loaded, or add there a wait queue. Still, I think that this patch is needed anyway, in order to avoid race conditions with CI and Remote Controller polls that may affect devices with tda18271.

It should be easy to check if the firmware is loaded: all it is needed is to, for example, call:
drxk_ops.get_tune_settings()

This function returns -ENODEV if firmware load fails; -EAGAIN if firmware was not loaded yet and 0 or -EINVAL if firmware is OK.

So, instead of using sleep, you could do:

static bool is_drxk_firmware_loaded(struct dvb_frontend *fe) {
	struct dvb_frontend_tune_settings sets;
	int ret = fe->ops.get_tune_settings(fe, &sets);

	if (ret == -ENODEV || ret == -EAGAIN)
		return false;
	else
		return true;
};

and, at the place you coded the sleep(), replace it by:

	ret = wait_event_interruptible(waitq, is_drxk_firmware_loaded(dev->dvb->fe[0]));
	if (ret < 0) {
		dvb_frontend_detach(dev->dvb->fe[0]);
		dev->dvb->fe[0] = NULL;
		return -EINVAL;
	}

It might have sense to add an special callback to return the tuner state (firmware not loaded, firmware loaded, firmware load failed).

Regards,
Mauro

Antti Palosaari

author

2012-10-01 11:31 UTC #27 lore raw

Re: [PATCH] tda18271-common: hold the I2C adapter during write transfers

Quoted 6 lines · Mauro Carvalho Chehab

Em Fri, 28 Sep 2012 21:31:07 +0300
Antti Palosaari <[email protected]> escreveu:
Quoted 2 lines
Hello,
Did not fix the issue. Problem remains same.

Quoted 7 lines

Ok, that's what I was afraid: there's likely something at drxk firmware that it is needed for tda18271 to be visible - maybe gpio settings.
Quoted 2 lines
With the sleep + that patch
it works still.
Good, no regressions added.

Currently there is regression as you haven't committed that sleep patch.

Quoted 34 lines

IMO, we should add a defer job at dvb_attach, that will postpone the tuner attach to happen after drxk firmware is loaded, or add there a wait queue. Still, I think that this patch is needed anyway, in order to avoid race conditions with CI and Remote Controller polls that may affect devices with tda18271.
It should be easy to check if the firmware is loaded: all it is needed is to, for example, call:
drxk_ops.get_tune_settings()
This function returns -ENODEV if firmware load fails; -EAGAIN if firmware was not loaded yet and 0 or -EINVAL if firmware is OK.
So, instead of using sleep, you could do:
static bool is_drxk_firmware_loaded(struct dvb_frontend *fe) {
	struct dvb_frontend_tune_settings sets;
	int ret = fe->ops.get_tune_settings(fe, &sets);

	if (ret == -ENODEV || ret == -EAGAIN)
		return false;
	else
		return true;
};
and, at the place you coded the sleep(), replace it by:
	ret = wait_event_interruptible(waitq, is_drxk_firmware_loaded(dev->dvb->fe[0]));
	if (ret < 0) {
		dvb_frontend_detach(dev->dvb->fe[0]);
		dev->dvb->fe[0] = NULL;
		return -EINVAL;
	}

Quoted 2 lines

It might have sense to add an special callback to return the tuner state (firmware not loaded, firmware loaded, firmware load failed).

This is stupid approach. It does not change the original behavior which was we are not allowed to block module init path. It blocks module init just as long as earlier, even longer, with increased code complexity!

Why the hell you want add this kind of hacks every single chip driver that downloads firmware? Instead, put it to the bridge and leave demod, tuner, sec, etc, drivers free.

If you put that asyncronous stuff to em28xx (with possible dev unregister if you wish to be elegant) then all the rest sub-drivers could be hack free.

regards
Antti

Signature · 1 line

http://palosaari.fi/

Mauro Carvalho Chehab

2012-10-01 12:37 UTC #28 lore raw

Re: [PATCH] tda18271-common: hold the I2C adapter during write transfers

Em 01-10-2012 08:31, Antti Palosaari escreveu:

Quoted 10 lines · Mauro Carvalho Chehab

Quoted 9 lines
Em Fri, 28 Sep 2012 21:31:07 +0300
Antti Palosaari <[email protected]> escreveu:
Quoted 2 lines
Hello,
Did not fix the issue. Problem remains same.
Ok, that's what I was afraid: there's likely something at drxk firmware that it is needed for tda18271 to be visible - maybe gpio settings.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
With the sleep + that patch
it works still.
Good, no regressions added.
Currently there is regression as you haven't committed that sleep patch.

Yes, I won't apply it before we finish those discussions.

Quoted 34 lines

Quoted 34 lines
IMO, we should add a defer job at dvb_attach, that will postpone the tuner attach to happen after drxk firmware is loaded, or add there a wait queue. Still, I think that this patch is needed anyway, in order to avoid race conditions with CI and Remote Controller polls that may affect devices with tda18271.
It should be easy to check if the firmware is loaded: all it is needed is to, for example, call:
drxk_ops.get_tune_settings()
This function returns -ENODEV if firmware load fails; -EAGAIN if firmware was not loaded yet and 0 or -EINVAL if firmware is OK.
So, instead of using sleep, you could do:
static bool is_drxk_firmware_loaded(struct dvb_frontend *fe) {
    struct dvb_frontend_tune_settings sets;
    int ret = fe->ops.get_tune_settings(fe, &sets);

    if (ret == -ENODEV || ret == -EAGAIN)
        return false;
    else
        return true;
};
and, at the place you coded the sleep(), replace it by:
    ret = wait_event_interruptible(waitq, is_drxk_firmware_loaded(dev->dvb->fe[0]));
    if (ret < 0) {
        dvb_frontend_detach(dev->dvb->fe[0]);
        dev->dvb->fe[0] = NULL;
        return -EINVAL;
    }

Quoted 5 lines

Quoted 2 lines
It might have sense to add an special callback to return the tuner state (firmware not loaded, firmware loaded, firmware load failed).
This is stupid approach. It does not change the original behavior which was we are not allowed to block module init path. It blocks module init just as long as earlier, even longer, with increased code complexity!

Not really. em28xx-dvb is loaded/attached asynchronously, already:

static void request_module_async(struct work_struct *work)

{
	struct em28xx *dev = container_of(work,
			     struct em28xx, request_module_wk);

	if (dev->has_audio_class)
		request_module("snd-usb-audio");
	else if (dev->has_alsa_audio)
		request_module("em28xx-alsa");

	if (dev->board.has_dvb)
		request_module("em28xx-dvb");
	if (dev->board.ir_codes && !disable_ir)
		request_module("em28xx-rc");
}

static void request_modules(struct em28xx *dev)

{
	INIT_WORK(&dev->request_module_wk, request_module_async);
	schedule_work(&dev->request_module_wk);
}

(one small change there is actually needed, for the case where the
driver is built-in)

Quoted 1 line

Why the hell you want add this kind of hacks every single chip driver that downloads firmware? Instead, put it to the bridge and leave demod, tuner, sec, etc, drivers free.

A sleep() hack is worse. Firmware load can take up to 60 seconds.

Btw, I know one atom-based hardware where firmware load can actually take a lot more than 2 seconds to happen, when the root fs is mounted via nfs.

Quoted 5 lines

If you put that asyncronous stuff to em28xx (with possible dev unregister if you wish to be elegant) then all the rest sub-drivers could be hack free.
regards
Antti

Antti Palosaari

author

2012-09-28 16:19 UTC #29 lore raw

In reply to a message not in the archive

Quoted 17 lines · Mauro Carvalho Chehab

Em Thu, 27 Sep 2012 17:58:24 -0400
Michael Krufky <[email protected]> escreveu:
Quoted 10 lines · Antti Palosaari
Quoted 8 lines · Michael Krufky
Mike, There is other problem too. PCTV 520e, which is Em28xx + DRX-K + TDA18271, fails to attach tuner now. Tuner is wired behind DRX-K I2C bus. TDA18271 driver does very much I/O during attach and I2C error is raised during attach now. Earlier it worked as DRX-K firmware was downloaded before tuner was attached, but now both DRX-K fw download and tuner attach happens same time leading that error.
Quoted 3 lines
Why is the DRX-K firmware downloading at the same time as tuner attach? Shouldn't the demod attach be finished before the tuner attach begins?

Quoted 6 lines

Michael,
What happens is that udev changes forced drivers to load firmware asynchronously, as, otherwise, udev won't load any firmware at all. Also, there's no warranty that the firmware will be loaded on 2 seconds or so (Anti's hack were to add a 2 seconds wait after drxk atttach, to wait for firmware load).

IMHO whole current DRX-K FW is broken by design.
1) if fw is not really needed it should not be loaded on attach() instead first use case at .init()
2) if fw is needed then it must be loaded and wait chip is up and running and after that return from attach()

When you done it asynchronously like that, there is big you hit more problems depending on hardware configuration etc.

I explained that earlier too. But lets take more "real world" example.

There is USB DVB-S device. USB-bridge needs first firmware in order to offer I2C adapter. We need USB-bridge I2C-adapter to probe demodulator which sits on bridge I2C-bus. After demodulator is found we need to download firmware fir demodulator in order to find out tuner. Tuner sits behind demod I2C-bus. OK, download fw and start demod => get access for demod I2C-bus. Then probe used tuner. Download FW for tuner in order to get access for tuner GPIOs which are in that case controlled by tuner firmware. Finally there is LNB voltage controller which is controlled by tuner GPIO. We ask tuner firmware to disable LNB voltage supply.

Quite worst possible scenario, but highly possible. And it cannot be performed until firmware are loaded for each chip one by one.

The only place this kind "asynchronous" hack is allowed is bridge driver - leaving the rest as those are. And my real opinion is that this kind of functionality does not belong to drivers at all but somewhere more lower levels like USB/PCI core routines.

Quoted 6 lines

What I suspect is that tda18271 init is being interruped in the middle, by the drxk firmware load. If this is the case, the solution is clean and quick: just use the new i2c_lock_adapter() way to lock the I2C bus to tda18271 during the critical part of the code where the register init happens.

regards
Antti

Signature · 1 line

http://palosaari.fi/

Antti Palosaari

author

2012-08-06 18:20 UTC #30 lore raw

In reply to Michael Krufky

Quoted 33 lines · Michael Krufky

Quoted 28 lines · Antti Palosaari
Quoted 14 lines · Michael Krufky
Quoted 10 lines · Antti Palosaari
Moi Michael,
I just realized tda18271 driver eats 160mA too much current after attach. This means, there is power management bug.
When I plug my nanoStick it eats total 240mA, after tda18271 sleep is called
it eats only 80mA total which is reasonable. If I use Digital Devices tda18271c2dd driver it is total 110mA after attach, which is also quite OK.
Quoted 3 lines
Thanks for the report -- I will take a look at it.
...patches are welcome, of course :-)
I suspect it does some tweaking on attach() and chip leaves powered (I saw demod debugs at calls I2C-gate control quite many times thus this suspicion). When chip is powered-up it is usually in some sleep state by default. Also, on attach() there should be no I/O unless very good reason. For example chip ID is allowed to read and download firmware in case it is really needed to continue - like for tuner communication.
Quoted 3 lines
What I found quickly testing few DVB USB sticks there seems to be very much power management problems... I am now waiting for new multimeter in order to make better measurements and likely return fixing these issues later.

Quoted 8 lines

The driver does some calibration during attach, some of which is a one-time initialization to determine a temperature differential for tune calculation later on, which can take some time on slower USB buses. The "fix" for the power usage issue would just be to make sure to sleep the device before exiting the attach() function.
I'm not looking to remove the calibration from the attach -- this was done on purpose.

You should understand from DVB driver model:
* attach() called only once when driver is loaded
* init() called to wake-up device
* sleep() called to sleep device

What I would like to say is that there is very big risk to shoot own leg when doing some initialization on attach(). For example resuming from the suspend could cause device reset and if you rely some settings that are done during attach() you will likely fail as Kernel / USB-host controller has reset your device.

See reset_resume from Kernel documentation:
Documentation/usb/power-management.txt

regards
Antti

Signature · 1 line

http://palosaari.fi/

Devin Heitmueller

2012-08-06 18:35 UTC #31 lore raw

Quoted 5 lines · Antti Palosaari

You should understand from DVB driver model:
* attach() called only once when driver is loaded
* init() called to wake-up device
* sleep() called to sleep device

Quoted 8 lines

What I would like to say is that there is very big risk to shoot own leg when doing some initialization on attach(). For example resuming from the suspend could cause device reset and if you rely some settings that are done during attach() you will likely fail as Kernel / USB-host controller has reset your device.
See reset_resume from Kernel documentation:
Documentation/usb/power-management.txt

Be forewarned: there is a very high likelihood that this patch will cause regressions on hybrid devices due to known race conditions related to dvb_frontend sleeping the tuner asynchronously on close. This is a hybrid tuner, and unless code is specifically added to the bridge or tuner driver, going from digital to analog mode too quickly will cause the tuner to be shutdown while it's actively in analog mode.

(I discovered this the hard way when working on problems MythTV users reported against the HVR-950q).

Description of race:

1.  User opens DVB frontend tunes
2.  User closes DVB frontend
3.  User *immediately* opens V4L device using same tuner
4.  User performs tuning request for analog
5.  DVB frontend issues sleep() call to tuner, causing analog tuning to fail.

This class of problem isn't seen on DVB only devices or those that have dedicated digital tuners not shared for analog usage. And in some cases it isn't noticed because a delay between closing the DVB device and opening the analog devices causes the sleep() call to happen before the analog tune (in which case you don't hit the race).

I'm certainly not against improved power management, but it will require the race conditions to be fixed first in order to avoid regressions.

Devin

Signature · 2 lines

Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Michael Krufky

maintainer

2012-08-06 18:57 UTC #32 lore raw

Quoted 44 lines · Devin Heitmueller

Quoted 5 lines · Antti Palosaari
You should understand from DVB driver model:
* attach() called only once when driver is loaded
* init() called to wake-up device
* sleep() called to sleep device
Quoted 8 lines
What I would like to say is that there is very big risk to shoot own leg when doing some initialization on attach(). For example resuming from the suspend could cause device reset and if you rely some settings that are done during attach() you will likely fail as Kernel / USB-host controller has reset your device.
See reset_resume from Kernel documentation:
Documentation/usb/power-management.txt
Be forewarned: there is a very high likelihood that this patch will cause regressions on hybrid devices due to known race conditions related to dvb_frontend sleeping the tuner asynchronously on close. This is a hybrid tuner, and unless code is specifically added to the bridge or tuner driver, going from digital to analog mode too quickly will cause the tuner to be shutdown while it's actively in analog mode.
(I discovered this the hard way when working on problems MythTV users reported against the HVR-950q).
Description of race:
1.  User opens DVB frontend tunes
2.  User closes DVB frontend
3.  User *immediately* opens V4L device using same tuner
4.  User performs tuning request for analog
5.  DVB frontend issues sleep() call to tuner, causing analog tuning to fail.
This class of problem isn't seen on DVB only devices or those that have dedicated digital tuners not shared for analog usage. And in some cases it isn't noticed because a delay between closing the DVB device and opening the analog devices causes the sleep() call to happen before the analog tune (in which case you don't hit the race).
I'm certainly not against improved power management, but it will require the race conditions to be fixed first in order to avoid regressions.

Quoted 5 lines

Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Devin's right. I'm sorry, please *don't* merge the patch, Mauro. Antti, you should just call sleep from your driver after attach(), as I had previously suggested. We can revisit this some time in the future after we can be sure that these race conditions wont occur.

-Mike

Antti Palosaari

author

2012-08-06 19:13 UTC #33 lore raw

Quoted 48 lines · Michael Krufky

Quoted 46 lines · Devin Heitmueller
Quoted 5 lines · Antti Palosaari
You should understand from DVB driver model:
* attach() called only once when driver is loaded
* init() called to wake-up device
* sleep() called to sleep device
Quoted 8 lines
What I would like to say is that there is very big risk to shoot own leg when doing some initialization on attach(). For example resuming from the suspend could cause device reset and if you rely some settings that are done during attach() you will likely fail as Kernel / USB-host controller has reset your device.
See reset_resume from Kernel documentation:
Documentation/usb/power-management.txt
Be forewarned: there is a very high likelihood that this patch will cause regressions on hybrid devices due to known race conditions related to dvb_frontend sleeping the tuner asynchronously on close. This is a hybrid tuner, and unless code is specifically added to the bridge or tuner driver, going from digital to analog mode too quickly will cause the tuner to be shutdown while it's actively in analog mode.
(I discovered this the hard way when working on problems MythTV users reported against the HVR-950q).
Description of race:
1.  User opens DVB frontend tunes
2.  User closes DVB frontend
3.  User *immediately* opens V4L device using same tuner
4.  User performs tuning request for analog
5.  DVB frontend issues sleep() call to tuner, causing analog tuning to fail.
This class of problem isn't seen on DVB only devices or those that have dedicated digital tuners not shared for analog usage. And in some cases it isn't noticed because a delay between closing the DVB device and opening the analog devices causes the sleep() call to happen before the analog tune (in which case you don't hit the race).
I'm certainly not against improved power management, but it will require the race conditions to be fixed first in order to avoid regressions.
Devin

Quoted 8 lines

Quoted 3 lines
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
Devin's right. I'm sorry, please *don't* merge the patch, Mauro. Antti, you should just call sleep from your driver after attach(), as I had previously suggested. We can revisit this some time in the future after we can be sure that these race conditions wont occur.

OK, maybe it is safer then. I have no any hybrid hardware to test...

Anyhow, I wonder how many years it will take to resolve that V4L2/DVB API hybrid usage pŕoblem. I ran thinking that recently when looked how to implement DVB SDR for V4L2 API... I could guess problem will not disappear near future even analog TV disappears, because there is surely coming new nasty features which spreads over both V4L2 and DVB APIs.

regards
Antti

Signature · 1 line

http://palosaari.fi/

Manu Abraham

2012-08-06 20:19 UTC #34 lore raw

On Tue, Aug 7, 2012 at 12:43 AM, Antti Palosaari <[email protected]> wrote:
..
I ran thinking that recently when looked how to

Quoted 1 line

implement DVB SDR for V4L2 API...

If you try to fit an apple into an orange, you run into that sort of a problem. If you are working with DVB, the Linux-DVB is the way to go, If you are working with analog/webcams then V4L should be used. If you are mixing both worlds, then you land into all sorts of nastiness.

You might need extensions to fit hardware needs as time passes by, but still you have the same basic fundamental thing working in there ..

One comes across demodulators running on a DSP/FPGA or a DSP-FPGA and so on ..

Regards,
Manu

Michael Krufky

maintainer

2012-09-20 17:47 UTC #35 lore raw

In reply to Antti Palosaari

Quoted 67 lines · Antti Palosaari

Quoted 60 lines · Michael Krufky
Quoted 50 lines · Devin Heitmueller
Quoted 14 lines · Antti Palosaari
You should understand from DVB driver model:
* attach() called only once when driver is loaded
* init() called to wake-up device
* sleep() called to sleep device
What I would like to say is that there is very big risk to shoot own leg when doing some initialization on attach(). For example resuming from the
suspend could cause device reset and if you rely some settings that are done
during attach() you will likely fail as Kernel / USB-host controller has reset your device.
Quoted 2 lines
See reset_resume from Kernel documentation:
Documentation/usb/power-management.txt
Be forewarned: there is a very high likelihood that this patch will cause regressions on hybrid devices due to known race conditions related to dvb_frontend sleeping the tuner asynchronously on close. This is a hybrid tuner, and unless code is specifically added to the bridge or tuner driver, going from digital to analog mode too quickly will cause the tuner to be shutdown while it's actively in analog mode.
(I discovered this the hard way when working on problems MythTV users reported against the HVR-950q).
Description of race:
1.  User opens DVB frontend tunes
2.  User closes DVB frontend
3.  User *immediately* opens V4L device using same tuner
4.  User performs tuning request for analog
5.  DVB frontend issues sleep() call to tuner, causing analog tuning to
fail.
This class of problem isn't seen on DVB only devices or those that have dedicated digital tuners not shared for analog usage. And in some cases it isn't noticed because a delay between closing the DVB device and opening the analog devices causes the sleep() call to happen before the analog tune (in which case you don't hit the race).
I'm certainly not against improved power management, but it will require the race conditions to be fixed first in order to avoid regressions.
Quoted 5 lines
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
Quoted 4 lines
Devin's right. I'm sorry, please *don't* merge the patch, Mauro. Antti, you should just call sleep from your driver after attach(), as I had previously suggested. We can revisit this some time in the future after we can be sure that these race conditions wont occur.

Quoted 7 lines

OK, maybe it is safer then. I have no any hybrid hardware to test...
Anyhow, I wonder how many years it will take to resolve that V4L2/DVB API hybrid usage pŕoblem. I ran thinking that recently when looked how to implement DVB SDR for V4L2 API... I could guess problem will not disappear near future even analog TV disappears, because there is surely coming new nasty features which spreads over both V4L2 and DVB APIs.

Guys,

Please take another look at this branch and test if possible -- I pushed an additional patch that takes Devin's concerns into account. After applying these patches, the tda18271 driver will behave as it always has, but it will sleep the tuner after attaching the first instance. If there is only one instance, then this works exactly as Antti desires. If there are more instances, then the tuner will only be woken up again during attach if the tda18271_need_cal_on_startup() returns non-zero. The driver does not attempt to re-sleep the hardware again during successive attach() calls.

I have not tested this yet myself, but I believe it resolves the matter -- please comment.

Regards,

Mike Krufky

Michael Krufky

maintainer

2012-09-20 17:49 UTC #36 lore raw

Quoted 88 lines · Michael Krufky

Quoted 67 lines · Antti Palosaari
Quoted 60 lines · Michael Krufky
Quoted 50 lines · Devin Heitmueller
Quoted 14 lines · Antti Palosaari
You should understand from DVB driver model:
* attach() called only once when driver is loaded
* init() called to wake-up device
* sleep() called to sleep device
What I would like to say is that there is very big risk to shoot own leg when doing some initialization on attach(). For example resuming from the
suspend could cause device reset and if you rely some settings that are done
during attach() you will likely fail as Kernel / USB-host controller has reset your device.
Quoted 2 lines
See reset_resume from Kernel documentation:
Documentation/usb/power-management.txt
Be forewarned: there is a very high likelihood that this patch will cause regressions on hybrid devices due to known race conditions related to dvb_frontend sleeping the tuner asynchronously on close. This is a hybrid tuner, and unless code is specifically added to the bridge or tuner driver, going from digital to analog mode too quickly will cause the tuner to be shutdown while it's actively in analog mode.
(I discovered this the hard way when working on problems MythTV users reported against the HVR-950q).
Description of race:
1.  User opens DVB frontend tunes
2.  User closes DVB frontend
3.  User *immediately* opens V4L device using same tuner
4.  User performs tuning request for analog
5.  DVB frontend issues sleep() call to tuner, causing analog tuning to
fail.
This class of problem isn't seen on DVB only devices or those that have dedicated digital tuners not shared for analog usage. And in some cases it isn't noticed because a delay between closing the DVB device and opening the analog devices causes the sleep() call to happen before the analog tune (in which case you don't hit the race).
I'm certainly not against improved power management, but it will require the race conditions to be fixed first in order to avoid regressions.
Quoted 5 lines
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
Quoted 4 lines
Devin's right. I'm sorry, please *don't* merge the patch, Mauro. Antti, you should just call sleep from your driver after attach(), as I had previously suggested. We can revisit this some time in the future after we can be sure that these race conditions wont occur.
Quoted 7 lines
OK, maybe it is safer then. I have no any hybrid hardware to test...
Anyhow, I wonder how many years it will take to resolve that V4L2/DVB API hybrid usage pŕoblem. I ran thinking that recently when looked how to implement DVB SDR for V4L2 API... I could guess problem will not disappear near future even analog TV disappears, because there is surely coming new nasty features which spreads over both V4L2 and DVB APIs.
Guys,
Please take another look at this branch and test if possible -- I pushed an additional patch that takes Devin's concerns into account. After applying these patches, the tda18271 driver will behave as it always has, but it will sleep the tuner after attaching the first instance. If there is only one instance, then this works exactly as Antti desires. If there are more instances, then the tuner will only be woken up again during attach if the tda18271_need_cal_on_startup() returns non-zero. The driver does not attempt to re-sleep the hardware again during successive attach() calls.

Quoted 6 lines

I have not tested this yet myself, but I believe it resolves the matter -- please comment.
Regards,
Mike Krufky

...in case the URL got lost:

The following changes since commit 0c7d5a6da75caecc677be1fda207b7578936770d:

Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)

are available in the git repository at:

git://linuxtv.org/mkrufky/tuners tda18271

for you to fetch changes up to 4e46c5d1bbb920165fecfe7de18b2c01d9787230:

tda18271: make 'low-power standby mode after attach' multi-instance
safe (2012-09-20 13:34:29 -0400)

----------------------------------------------------------------
Michael Krufky (2):
tda18271: enter low-power standby mode at the end of tda18271_attach()
tda18271: make 'low-power standby mode after attach' multi-instance safe

 drivers/media/common/tuners/tda18271-fe.c |    4 ++++
 1 file changed, 4 insertions(+)

Best regards,

Mike

Antti Palosaari

author Tested-by

2012-09-22 17:21 UTC #37 lore raw

Quoted 117 lines · Michael Krufky

Quoted 88 lines · Michael Krufky
Quoted 67 lines · Antti Palosaari
Quoted 60 lines · Michael Krufky
Quoted 50 lines · Devin Heitmueller
Quoted 14 lines · Antti Palosaari
You should understand from DVB driver model:
* attach() called only once when driver is loaded
* init() called to wake-up device
* sleep() called to sleep device
What I would like to say is that there is very big risk to shoot own leg when doing some initialization on attach(). For example resuming from the
suspend could cause device reset and if you rely some settings that are done
during attach() you will likely fail as Kernel / USB-host controller has reset your device.
Quoted 2 lines
See reset_resume from Kernel documentation:
Documentation/usb/power-management.txt
Be forewarned: there is a very high likelihood that this patch will cause regressions on hybrid devices due to known race conditions related to dvb_frontend sleeping the tuner asynchronously on close. This is a hybrid tuner, and unless code is specifically added to the bridge or tuner driver, going from digital to analog mode too quickly will cause the tuner to be shutdown while it's actively in analog mode.
(I discovered this the hard way when working on problems MythTV users reported against the HVR-950q).
Description of race:
1.  User opens DVB frontend tunes
2.  User closes DVB frontend
3.  User *immediately* opens V4L device using same tuner
4.  User performs tuning request for analog
5.  DVB frontend issues sleep() call to tuner, causing analog tuning to
fail.
This class of problem isn't seen on DVB only devices or those that have dedicated digital tuners not shared for analog usage. And in some cases it isn't noticed because a delay between closing the DVB device and opening the analog devices causes the sleep() call to happen before the analog tune (in which case you don't hit the race).
I'm certainly not against improved power management, but it will require the race conditions to be fixed first in order to avoid regressions.
Quoted 5 lines
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
Quoted 4 lines
Devin's right. I'm sorry, please *don't* merge the patch, Mauro. Antti, you should just call sleep from your driver after attach(), as I had previously suggested. We can revisit this some time in the future after we can be sure that these race conditions wont occur.
Quoted 7 lines
OK, maybe it is safer then. I have no any hybrid hardware to test...
Anyhow, I wonder how many years it will take to resolve that V4L2/DVB API hybrid usage pŕoblem. I ran thinking that recently when looked how to implement DVB SDR for V4L2 API... I could guess problem will not disappear near future even analog TV disappears, because there is surely coming new nasty features which spreads over both V4L2 and DVB APIs.
Guys,
Please take another look at this branch and test if possible -- I pushed an additional patch that takes Devin's concerns into account. After applying these patches, the tda18271 driver will behave as it always has, but it will sleep the tuner after attaching the first instance. If there is only one instance, then this works exactly as Antti desires. If there are more instances, then the tuner will only be woken up again during attach if the tda18271_need_cal_on_startup() returns non-zero. The driver does not attempt to re-sleep the hardware again during successive attach() calls.
Quoted 6 lines
I have not tested this yet myself, but I believe it resolves the matter -- please comment.
Regards,
Mike Krufky
...in case the URL got lost:
The following changes since commit 0c7d5a6da75caecc677be1fda207b7578936770d:
   Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
are available in the git repository at:
   git://linuxtv.org/mkrufky/tuners tda18271
for you to fetch changes up to 4e46c5d1bbb920165fecfe7de18b2c01d9787230:
tda18271: make 'low-power standby mode after attach' multi-instance
safe (2012-09-20 13:34:29 -0400)
----------------------------------------------------------------
Michael Krufky (2):
tda18271: enter low-power standby mode at the end of tda18271_attach()
tda18271: make 'low-power standby mode after attach' multi-instance safe

Quoted 7 lines

  drivers/media/common/tuners/tda18271-fe.c |    4 ++++
  1 file changed, 4 insertions(+)

Best regards,

Mike

Tested-by Antti Palosaari <[email protected]>

Tested with PCTV 290e, but I cannot test multi-instance. I tried to plug PCTV 520e as a second stick, but it crashed as there is now that DRX-K firmware download problem....

regards
Antti

Signature · 1 line

http://palosaari.fi/