On Wed, Jul 25, 2012 at 11:18 PM, Michael Krufky <mkrufky@xxxxxxxxxxx> wrote:
> On Tue, Jul 24, 2012 at 8:43 PM, Antti Palosaari <crope@xxxxxx> wrote:
>> On 07/25/2012 03:15 AM, Michael Krufky wrote:
>>>
>>> On Tue, Jul 24, 2012 at 6:17 PM, Michael Krufky <mkrufky@xxxxxxxxxxx>
>>> wrote:
>>>>
>>>> On Tue, Jul 24, 2012 at 6:12 PM, Antti Palosaari <crope@xxxxxx> wrote:
>>>>>
>>>>> On 07/25/2012 12:55 AM, Michael Krufky wrote:
>>>>>>
>>>>>>
>>>>>> On Sun, Jul 22, 2012 at 3:59 PM, Antti Palosaari <crope@xxxxxx> wrote:
>>>>>>>
>>>>>>>
>>>>>>> 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 :-)
>>>>>
>>>>>
>>>>>
>>>>> 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.
>>>>
>>>>
>>>> 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?
>>
>>
>> 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 onlỵ (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
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
Attachment:
0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch
Description: Binary data
