tda18271 driver power consumption

tda18271 driver power consumption

[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.

regards
Antti


-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Michael Krufky]

On Sun, Jul 22, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> 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 :-)

-Mike

Re: tda18271 driver power consumption

[Antti Palosaari]

On 07/25/2012 12:55 AM, Michael Krufky wrote:
> On Sun, Jul 22, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> 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.

regards
Antti

-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Michael Krufky]

On Tue, Jul 24, 2012 at 6:12 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 07/25/2012 12:55 AM, Michael Krufky wrote:
>>
>> On Sun, Jul 22, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> 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.

-Mike

Re: tda18271 driver power consumption

[Michael Krufky]

On Tue, Jul 24, 2012 at 6:17 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
> On Tue, Jul 24, 2012 at 6:12 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 07/25/2012 12:55 AM, Michael Krufky wrote:
>>>
>>> On Sun, Jul 22, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> 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?

Regards,

Mike Krufky

Re: tda18271 driver power consumption

[Antti Palosaari]

On 07/25/2012 03:15 AM, Michael Krufky wrote:
> On Tue, Jul 24, 2012 at 6:17 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
>> On Tue, Jul 24, 2012 at 6:12 PM, Antti Palosaari <crope@iki.fi> wrote:
>>> On 07/25/2012 12:55 AM, Michael Krufky wrote:
>>>>
>>>> On Sun, Jul 22, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> 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.

regards
Antti

-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Michael Krufky]

On Tue, Jul 24, 2012 at 8:43 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 07/25/2012 03:15 AM, Michael Krufky wrote:
>>
>> On Tue, Jul 24, 2012 at 6:17 PM, Michael Krufky <mkrufky@linuxtv.org>
>> wrote:
>>>
>>> On Tue, Jul 24, 2012 at 6:12 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>
>>>> On 07/25/2012 12:55 AM, Michael Krufky wrote:
>>>>>
>>>>>
>>>>> On Sun, Jul 22, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> 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

Re: tda18271 driver power consumption

[Michael Krufky]

[-- Attachment #1: Type: text/plain, Size: 5944 bytes --]

On Wed, Jul 25, 2012 at 11:18 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
> On Tue, Jul 24, 2012 at 8:43 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 07/25/2012 03:15 AM, Michael Krufky wrote:
>>>
>>> On Tue, Jul 24, 2012 at 6:17 PM, Michael Krufky <mkrufky@linuxtv.org>
>>> wrote:
>>>>
>>>> On Tue, Jul 24, 2012 at 6:12 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>
>>>>> On 07/25/2012 12:55 AM, Michael Krufky wrote:
>>>>>>
>>>>>>
>>>>>> On Sun, Jul 22, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> 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 #2: 0001-tda18271-enter-low-power-standby-mode-at-the-end-of-.patch --]
[-- Type: application/octet-stream, Size: 1185 bytes --]

From 782b28e20d3b253d317cc71879639bf3c108b200 Mon Sep 17 00:00:00 2001
From: Michael Krufky <mkrufky@linuxtv.org>
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 <crope@iki.fi>
Signed-off-by: Michael Krufky <mkrufky@linuxtv.org>
---
 drivers/media/common/tuners/tda18271-fe.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
index 2e67f44..5f5d866 100644
--- a/drivers/media/common/tuners/tda18271-fe.c
+++ b/drivers/media/common/tuners/tda18271-fe.c
@@ -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);
-- 
1.7.9.5

Re: tda18271 driver power consumption

[Antti Palosaari]

On 07/26/2012 03:48 PM, Michael Krufky wrote:
> On Wed, Jul 25, 2012 at 11:18 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
>> On Tue, Jul 24, 2012 at 8:43 PM, Antti Palosaari <crope@iki.fi> wrote:
>>> On 07/25/2012 03:15 AM, Michael Krufky wrote:
>>>>
>>>> On Tue, Jul 24, 2012 at 6:17 PM, Michael Krufky <mkrufky@linuxtv.org>
>>>> wrote:
>>>>>
>>>>> On Tue, Jul 24, 2012 at 6:12 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>
>>>>>> On 07/25/2012 12:55 AM, Michael Krufky wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Sun, Jul 22, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> 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?


Tested-by: Antti Palosaari <crope@iki.fi>

>
>
> 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
>


-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Antti Palosaari]

On 07/25/2012 01:17 AM, Michael Krufky wrote:
> On Tue, Jul 24, 2012 at 6:12 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 07/25/2012 12:55 AM, Michael Krufky wrote:
>>>
>>> On Sun, Jul 22, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> 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.


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

-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Devin Heitmueller]

On Mon, Aug 6, 2012 at 2:19 PM, Antti Palosaari <crope@iki.fi> wrote:
> 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

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

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

Re: tda18271 driver power consumption

[Michael Krufky]

On Mon, Aug 6, 2012 at 2:35 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> On Mon, Aug 6, 2012 at 2:19 PM, Antti Palosaari <crope@iki.fi> wrote:
>> 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
>
> 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
>
> --
> 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

Re: tda18271 driver power consumption

[Antti Palosaari]

On 08/06/2012 09:57 PM, Michael Krufky wrote:
> On Mon, Aug 6, 2012 at 2:35 PM, Devin Heitmueller
> <dheitmueller@kernellabs.com> wrote:
>> On Mon, Aug 6, 2012 at 2:19 PM, Antti Palosaari <crope@iki.fi> wrote:
>>> 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
>>
>> 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
>>
>> --
>> 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

-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Manu Abraham]

On Tue, Aug 7, 2012 at 12:43 AM, Antti Palosaari <crope@iki.fi> wrote:
..
 I ran thinking that recently when looked how to
> 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

Re: tda18271 driver power consumption

[Michael Krufky]

On Mon, Aug 6, 2012 at 3:13 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 08/06/2012 09:57 PM, Michael Krufky wrote:
>>
>> On Mon, Aug 6, 2012 at 2:35 PM, Devin Heitmueller
>> <dheitmueller@kernellabs.com> wrote:
>>>
>>> On Mon, Aug 6, 2012 at 2:19 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>
>>>> 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
>>>
>>>
>>> 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
>>>
>>> --
>>> 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.

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

Re: tda18271 driver power consumption

[Michael Krufky]

On Thu, Sep 20, 2012 at 1:47 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
> On Mon, Aug 6, 2012 at 3:13 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 08/06/2012 09:57 PM, Michael Krufky wrote:
>>>
>>> On Mon, Aug 6, 2012 at 2:35 PM, Devin Heitmueller
>>> <dheitmueller@kernellabs.com> wrote:
>>>>
>>>> On Mon, Aug 6, 2012 at 2:19 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>
>>>>> 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
>>>>
>>>>
>>>> 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
>>>>
>>>> --
>>>> 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.
>
> 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

...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

Re: tda18271 driver power consumption

[Antti Palosaari]

On 09/20/2012 08:49 PM, Michael Krufky wrote:
> On Thu, Sep 20, 2012 at 1:47 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
>> On Mon, Aug 6, 2012 at 3:13 PM, Antti Palosaari <crope@iki.fi> wrote:
>>> On 08/06/2012 09:57 PM, Michael Krufky wrote:
>>>>
>>>> On Mon, Aug 6, 2012 at 2:35 PM, Devin Heitmueller
>>>> <dheitmueller@kernellabs.com> wrote:
>>>>>
>>>>> On Mon, Aug 6, 2012 at 2:19 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>
>>>>>> 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
>>>>>
>>>>>
>>>>> 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
>>>>>
>>>>> --
>>>>> 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.
>>
>> 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
>
> ...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
>
Tested-by: Antti Palosaari <crope@iki.fi>

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

-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Antti Palosaari]

On 09/27/2012 10:19 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 26 Jul 2012 08:48:58 -0400
> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>
>> 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(+)
>
>
> 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 <mkrufky@linuxtv.org>
> Date: Thu, 26 Jul 2012 08:34:37 -0400
> Patch applies OK
> total: 0 errors, 0 warnings, 9 lines checked
>
> 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

-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Michael Krufky]

On Thu, Sep 27, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 09/27/2012 10:19 PM, Mauro Carvalho Chehab wrote:
>>
>> Em Thu, 26 Jul 2012 08:48:58 -0400
>> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>>
>>> 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(+)
>>
>>
>>
>> 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 <mkrufky@linuxtv.org>
>> Date: Thu, 26 Jul 2012 08:34:37 -0400
>> Patch applies OK
>> total: 0 errors, 0 warnings, 9 lines checked
>>
>> 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
>
> --
> 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

Re: tda18271 driver power consumption

[Antti Palosaari]

On 09/28/2012 12:20 AM, Michael Krufky wrote:
> On Thu, Sep 27, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 09/27/2012 10:19 PM, Mauro Carvalho Chehab wrote:
>>>
>>> Em Thu, 26 Jul 2012 08:48:58 -0400
>>> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>>>
>>>> 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(+)
>>>
>>>
>>>
>>> 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 <mkrufky@linuxtv.org>
>>> Date: Thu, 26 Jul 2012 08:34:37 -0400
>>> Patch applies OK
>>> total: 0 errors, 0 warnings, 9 lines checked
>>>
>>> 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
>>
>> --
>> 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


-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Michael Krufky]

On Thu, Sep 27, 2012 at 5:38 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 09/28/2012 12:20 AM, Michael Krufky wrote:
>>
>> On Thu, Sep 27, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>
>>> On 09/27/2012 10:19 PM, Mauro Carvalho Chehab wrote:
>>>>
>>>>
>>>> Em Thu, 26 Jul 2012 08:48:58 -0400
>>>> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>>>>
>>>>> 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(+)
>>>>
>>>>
>>>>
>>>>
>>>> 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 <mkrufky@linuxtv.org>
>>>> Date: Thu, 26 Jul 2012 08:34:37 -0400
>>>> Patch applies OK
>>>> total: 0 errors, 0 warnings, 9 lines checked
>>>>
>>>> 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
>>>
>>> --
>>> 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.

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

Re: tda18271 driver power consumption

[Antti Palosaari]

On 09/28/2012 12:58 AM, Michael Krufky wrote:
> On Thu, Sep 27, 2012 at 5:38 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 09/28/2012 12:20 AM, Michael Krufky wrote:
>>>
>>> On Thu, Sep 27, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>
>>>> On 09/27/2012 10:19 PM, Mauro Carvalho Chehab wrote:
>>>>>
>>>>>
>>>>> Em Thu, 26 Jul 2012 08:48:58 -0400
>>>>> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>>>>>
>>>>>> 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(+)
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> 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 <mkrufky@linuxtv.org>
>>>>> Date: Thu, 26 Jul 2012 08:34:37 -0400
>>>>> Patch applies OK
>>>>> total: 0 errors, 0 warnings, 9 lines checked
>>>>>
>>>>> 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
>>>>
>>>> --
>>>> 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.
>
> 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

-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Michael Krufky]

On Thu, Sep 27, 2012 at 6:26 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 09/28/2012 12:58 AM, Michael Krufky wrote:
>>
>> On Thu, Sep 27, 2012 at 5:38 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>
>>> On 09/28/2012 12:20 AM, Michael Krufky wrote:
>>>>
>>>>
>>>> On Thu, Sep 27, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>
>>>>>
>>>>> On 09/27/2012 10:19 PM, Mauro Carvalho Chehab wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Em Thu, 26 Jul 2012 08:48:58 -0400
>>>>>> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>>>>>>
>>>>>>> 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(+)
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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 <mkrufky@linuxtv.org>
>>>>>> Date: Thu, 26 Jul 2012 08:34:37 -0400
>>>>>> Patch applies OK
>>>>>> total: 0 errors, 0 warnings, 9 lines checked
>>>>>>
>>>>>>
>>>>>> 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
>>>>>
>>>>> --
>>>>> 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.
>>
>>
>> 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.
>


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

Re: tda18271 driver power consumption

[Antti Palosaari]

On 09/28/2012 01:43 AM, Michael Krufky wrote:
> On Thu, Sep 27, 2012 at 6:26 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 09/28/2012 12:58 AM, Michael Krufky wrote:
>>>
>>> On Thu, Sep 27, 2012 at 5:38 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>
>>>> On 09/28/2012 12:20 AM, Michael Krufky wrote:
>>>>>
>>>>>
>>>>> On Thu, Sep 27, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>
>>>>>>
>>>>>> On 09/27/2012 10:19 PM, Mauro Carvalho Chehab wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Em Thu, 26 Jul 2012 08:48:58 -0400
>>>>>>> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>>>>>>>
>>>>>>>> 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(+)
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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 <mkrufky@linuxtv.org>
>>>>>>> Date: Thu, 26 Jul 2012 08:34:37 -0400
>>>>>>> Patch applies OK
>>>>>>> total: 0 errors, 0 warnings, 9 lines checked
>>>>>>>
>>>>>>>
>>>>>>> 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
>>>>>>
>>>>>> --
>>>>>> 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.
>>>
>>>
>>> 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.
>>
>
>
> 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.

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

-- 
http://palosaari.fi/

Re: tda18271 driver power consumption

[Michael Krufky]

On Thu, Sep 27, 2012 at 6:46 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 09/28/2012 01:43 AM, Michael Krufky wrote:
>>
>> On Thu, Sep 27, 2012 at 6:26 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>
>>> On 09/28/2012 12:58 AM, Michael Krufky wrote:
>>>>
>>>>
>>>> On Thu, Sep 27, 2012 at 5:38 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>
>>>>>
>>>>> On 09/28/2012 12:20 AM, Michael Krufky wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Thu, Sep 27, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 09/27/2012 10:19 PM, Mauro Carvalho Chehab wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Em Thu, 26 Jul 2012 08:48:58 -0400
>>>>>>>> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>>>>>>>>
>>>>>>>>> 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(+)
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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 <mkrufky@linuxtv.org>
>>>>>>>> Date: Thu, 26 Jul 2012 08:34:37 -0400
>>>>>>>> Patch applies OK
>>>>>>>> total: 0 errors, 0 warnings, 9 lines checked
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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
>>>>>>>
>>>>>>> --
>>>>>>> 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.
>>>>
>>>>
>>>>
>>>> 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.
>>>
>>
>>
>> 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.
>
>
> 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

Re: tda18271 driver power consumption

[Antti Palosaari]

On 09/28/2012 01:55 AM, Michael Krufky wrote:
> On Thu, Sep 27, 2012 at 6:46 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 09/28/2012 01:43 AM, Michael Krufky wrote:
>>>
>>> On Thu, Sep 27, 2012 at 6:26 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>
>>>> On 09/28/2012 12:58 AM, Michael Krufky wrote:
>>>>>
>>>>>
>>>>> On Thu, Sep 27, 2012 at 5:38 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>
>>>>>>
>>>>>> On 09/28/2012 12:20 AM, Michael Krufky wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Sep 27, 2012 at 3:59 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 09/27/2012 10:19 PM, Mauro Carvalho Chehab wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Em Thu, 26 Jul 2012 08:48:58 -0400
>>>>>>>>> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>>>>>>>>>
>>>>>>>>>> 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(+)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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 <mkrufky@linuxtv.org>
>>>>>>>>> Date: Thu, 26 Jul 2012 08:34:37 -0400
>>>>>>>>> Patch applies OK
>>>>>>>>> total: 0 errors, 0 warnings, 9 lines checked
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>
>>>>>>>> --
>>>>>>>> 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.
>>>>>
>>>>>
>>>>>
>>>>> 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.
>>>>
>>>
>>>
>>> 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.
>>
>>
>> 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.

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

regards
Antti


-- 
http://palosaari.fi/

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

[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)

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 <mchehab@redhat.com>
---
 drivers/media/tuners/tda18271-common.c | 104 ++++++++++++++++++++++-----------
 1 file changed, 71 insertions(+), 33 deletions(-)

diff --git a/drivers/media/tuners/tda18271-common.c b/drivers/media/tuners/tda18271-common.c
index 221171e..18c77af 100644
--- a/drivers/media/tuners/tda18271-common.c
+++ b/drivers/media/tuners/tda18271-common.c
@@ -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;
 }
-- 
1.7.11.4

Re: tda18271 driver power consumption

[Antti Palosaari]

On 09/28/2012 02:43 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 27 Sep 2012 17:58:24 -0400
> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>
>> On Thu, Sep 27, 2012 at 5:38 PM, Antti Palosaari <crope@iki.fi> wrote:
>>> On 09/28/2012 12:20 AM, Michael Krufky wrote:
>
>>> 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?
>
> 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.

> 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
-- 
http://palosaari.fi/

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

[Antti Palosaari]

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

On 09/28/2012 06:04 PM, Mauro Carvalho Chehab wrote:
> 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.

> 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 <mchehab@redhat.com>
> ---
>   drivers/media/tuners/tda18271-common.c | 104 ++++++++++++++++++++++-----------
>   1 file changed, 71 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/tuners/tda18271-common.c b/drivers/media/tuners/tda18271-common.c
> index 221171e..18c77af 100644
> --- a/drivers/media/tuners/tda18271-common.c
> +++ b/drivers/media/tuners/tda18271-common.c
> @@ -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;
>   }
>


-- 
http://palosaari.fi/

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

[Michael Krufky]

On Fri, Sep 28, 2012 at 2:31 PM, Antti Palosaari <crope@iki.fi> wrote:
> Hello,
> Did not fix the issue. Problem remains same. With the sleep + that patch it
> works still.
>
> On 09/28/2012 06:04 PM, Mauro Carvalho Chehab wrote:
>>
>> 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.
>
>> 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 <mchehab@redhat.com>
>> ---
>>   drivers/media/tuners/tda18271-common.c | 104
>> ++++++++++++++++++++++-----------
>>   1 file changed, 71 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/media/tuners/tda18271-common.c
>> b/drivers/media/tuners/tda18271-common.c
>> index 221171e..18c77af 100644
>> --- a/drivers/media/tuners/tda18271-common.c
>> +++ b/drivers/media/tuners/tda18271-common.c
>> @@ -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;
>>   }
>>
>
>
> --
> 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

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

[Michael Krufky]

On Fri, Sep 28, 2012 at 2:56 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
> On Fri, Sep 28, 2012 at 2:31 PM, Antti Palosaari <crope@iki.fi> wrote:
>> Hello,
>> Did not fix the issue. Problem remains same. With the sleep + that patch it
>> works still.
>>
>> On 09/28/2012 06:04 PM, Mauro Carvalho Chehab wrote:
>>>
>>> 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.
>>
>>> 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 <mchehab@redhat.com>
>>> ---
>>>   drivers/media/tuners/tda18271-common.c | 104
>>> ++++++++++++++++++++++-----------
>>>   1 file changed, 71 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/media/tuners/tda18271-common.c
>>> b/drivers/media/tuners/tda18271-common.c
>>> index 221171e..18c77af 100644
>>> --- a/drivers/media/tuners/tda18271-common.c
>>> +++ b/drivers/media/tuners/tda18271-common.c
>>> @@ -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;
>>>   }
>>>
>>
>>
>> --
>> 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)

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

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

[Mauro Carvalho Chehab]

Em Fri, 28 Sep 2012 21:31:07 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> 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.

> 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

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

[Antti Palosaari]

On 10/01/2012 01:42 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 28 Sep 2012 21:31:07 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> 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.
>
>> With the sleep + that patch
>> it works still.
>
> Good, no regressions added.

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

> 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).

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
-- 
http://palosaari.fi/

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

[Mauro Carvalho Chehab]

Em 01-10-2012 08:31, Antti Palosaari escreveu:
> On 10/01/2012 01:42 PM, Mauro Carvalho Chehab wrote:
>> Em Fri, 28 Sep 2012 21:31:07 +0300
>> Antti Palosaari <crope@iki.fi> escreveu:
>>
>>> 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.
>>
>>> 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.

>> 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).
> 
> 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)

> 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.

> 
> 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

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

[Mauro Carvalho Chehab]

Em Sat, 29 Sep 2012 15:20:23 -0400
Michael Krufky <mkrufky@linuxtv.org> escreveu:

> On Fri, Sep 28, 2012 at 2:56 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
> > On Fri, Sep 28, 2012 at 2:31 PM, Antti Palosaari <crope@iki.fi> wrote:
> >> Hello,
> >> Did not fix the issue. Problem remains same. With the sleep + that patch it
> >> works still.
> >>
> >> On 09/28/2012 06:04 PM, Mauro Carvalho Chehab wrote:
> >>>
> >>> 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.
> >>
> >>> 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 <mchehab@redhat.com>
> >>> ---
> >>>   drivers/media/tuners/tda18271-common.c | 104
> >>> ++++++++++++++++++++++-----------
> >>>   1 file changed, 71 insertions(+), 33 deletions(-)
> >>>
> >>> diff --git a/drivers/media/tuners/tda18271-common.c
> >>> b/drivers/media/tuners/tda18271-common.c
> >>> index 221171e..18c77af 100644
> >>> --- a/drivers/media/tuners/tda18271-common.c
> >>> +++ b/drivers/media/tuners/tda18271-common.c
> >>> @@ -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;
> >>>   }
> >>>
> >>
> >>
> >> --
> >> 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)
> 
> 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.

> So, I retract my NACK. 

Ok, I'll apply it then.

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

Yeah, for sure optimization there is very welcome.

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


-- 
Regards,
Mauro

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

[Michael Krufky]

On Sun, Oct 7, 2012 at 8:42 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em Sat, 29 Sep 2012 15:20:23 -0400
> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>
>> On Fri, Sep 28, 2012 at 2:56 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
>> > On Fri, Sep 28, 2012 at 2:31 PM, Antti Palosaari <crope@iki.fi> wrote:
>> >> Hello,
>> >> Did not fix the issue. Problem remains same. With the sleep + that patch it
>> >> works still.
>> >>
>> >> On 09/28/2012 06:04 PM, Mauro Carvalho Chehab wrote:
>> >>>
>> >>> 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.
>> >>
>> >>> 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 <mchehab@redhat.com>
>> >>> ---
>> >>>   drivers/media/tuners/tda18271-common.c | 104
>> >>> ++++++++++++++++++++++-----------
>> >>>   1 file changed, 71 insertions(+), 33 deletions(-)
>> >>>
>> >>> diff --git a/drivers/media/tuners/tda18271-common.c
>> >>> b/drivers/media/tuners/tda18271-common.c
>> >>> index 221171e..18c77af 100644
>> >>> --- a/drivers/media/tuners/tda18271-common.c
>> >>> +++ b/drivers/media/tuners/tda18271-common.c
>> >>> @@ -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;
>> >>>   }
>> >>>
>> >>
>> >>
>> >> --
>> >> 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)
>>
>> 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.
>
>> So, I retract my NACK.
>
> Ok, I'll apply it then.
>
>> If we decide
>> to merge this, then we can optimize it afterwards.
>
> Yeah, for sure optimization there is very welcome.
>
>>
>> -Mike
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> 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