Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Christoph Bartelmus]

Hi!

Jon Smirl "jonsm...@gmail.com" wrote:

> On Sun, Aug 1, 2010 at 5:50 AM, Christoph Bartelmus 
> wrote:
>> Hi Jon,
>>
>> on 31 Jul 10 at 14:14, Jon Smirl wrote:
>>> On Sat, Jul 31, 2010 at 1:47 PM, Christoph Bartelmus 
>>> wrote:
 Hi Jon,

 on 31 Jul 10 at 12:25, Jon Smirl wrote:
> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
> wrote:
>> I think you won't be able to fix the problem conclusively either way.
>>  A lot of how the chip's clocks should be programmed depends on how the
>> GPIOs are used and what crystal is used.
>>
>> I suspect many designers will use some reference design layout from
>> ENE, but it won't be good in every case.  The wire-up of the ENE of
>> various motherboards is likely something you'll have to live with as
>> unknowns.
>>
>> This is a case where looser tolerances in the in kernel decoders could
>> reduce this driver's complexity and/or get rid of arbitrary fudge
>> factors in the driver.

> The tolerances are as loose as they can be. The NEC protocol uses
> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
> (50% of 4%).  The crystals used in electronics are accurate to
> 0.0001%+.

 But the standard IR receivers are far from being accurate enough to allow
 tolerance windows of only 2%.
 I'm surprised that this works for you. LIRC uses a standard tolerance of
 30% / 100 us and even this is not enough sometimes.

 For the NEC protocol one signal consists of 22 individual pulses at
 38kHz.. If the receiver just misses one pulse, you already have an error
 of 1/22
> 4%.
>>
>>> There are different types of errors. The decoders can take large
>>> variations in bit times. The problem is with cumulative errors. In
>>> this case the error had accumulated up to 450us in the lead pulse.
>>> That's just too big of an error and caused the JVC code to be
>>> misclassified as NEC.
>>>
>>> I think he said lirc was misclassifying it too. So we both did the same
>>> thing.
>>
>> No way. JVC is a 16 bit code. NEC uses 32 bits. How can you ever confuse
>> JVC with NEC signals?
>>
>> LIRC will work if there is a 4% or 40% or 400% error. Because irrecord
>> generates the config file using your receiver it will compensate for any

> At the end of the process we can build a record and match raw mode if
> we have to.

I'm not talking about raw mode here. lircd will happily decode the signals  
despite of any timing error as long it's consistent.

I'm still interested how JVC can be confused with NEC codes.

>> timing error. It will work with pulses cut down to 50 us like IrDA
>> hardware does and it will work when half of the bits are swallowed like
>> the IgorPlug USB receiver does.

> The code for fixing IrDA and IgorPLug should live inside their low
> level device drivers.  The characteristics of the errors produced by
> this hardware are known so a fix can be written to compensate.

The function f(x) = 50 is not bijective. No way to compensate.

Missing bits cannot be magically regenerated by the driver.

> The
> IgorPlug people might find it easier to fix their firmware.

There is a firmware patch available? Do you have a pointer?

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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Sun, Aug 1, 2010 at 10:00 AM, Jon Smirl  wrote:
> On Sun, Aug 1, 2010 at 5:50 AM, Christoph Bartelmus  wrote:
>> Hi Jon,
>>
>> on 31 Jul 10 at 14:14, Jon Smirl wrote:
>>> On Sat, Jul 31, 2010 at 1:47 PM, Christoph Bartelmus 
>>> wrote:
 Hi Jon,

 on 31 Jul 10 at 12:25, Jon Smirl wrote:
> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
> wrote:
>> I think you won't be able to fix the problem conclusively either way.  A
>> lot of how the chip's clocks should be programmed depends on how the
>> GPIOs are used and what crystal is used.
>>
>> I suspect many designers will use some reference design layout from ENE,
>> but it won't be good in every case.  The wire-up of the ENE of various
>> motherboards is likely something you'll have to live with as unknowns.
>>
>> This is a case where looser tolerances in the in kernel decoders could
>> reduce this driver's complexity and/or get rid of arbitrary fudge
>> factors in the driver.

> The tolerances are as loose as they can be. The NEC protocol uses
> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
> (50% of 4%).  The crystals used in electronics are accurate to
> 0.0001%+.

 But the standard IR receivers are far from being accurate enough to allow
 tolerance windows of only 2%.
 I'm surprised that this works for you. LIRC uses a standard tolerance of
 30% / 100 us and even this is not enough sometimes.

 For the NEC protocol one signal consists of 22 individual pulses at 38kHz..
 If the receiver just misses one pulse, you already have an error of 1/22
> 4%.
>>
>>> There are different types of errors. The decoders can take large
>>> variations in bit times. The problem is with cumulative errors. In
>>> this case the error had accumulated up to 450us in the lead pulse.
>>> That's just too big of an error and caused the JVC code to be
>>> misclassified as NEC.
>>>
>>> I think he said lirc was misclassifying it too. So we both did the same
>>> thing.
>>
>> No way. JVC is a 16 bit code. NEC uses 32 bits. How can you ever confuse
>> JVC with NEC signals?
>>
>> LIRC will work if there is a 4% or 40% or 400% error. Because irrecord
>> generates the config file using your receiver it will compensate for any
>
> At the end of the process we can build a record and match raw mode if
> we have to.
>
>> timing error. It will work with pulses cut down to 50 us like IrDA
>> hardware does and it will work when half of the bits are swallowed like
>> the IgorPlug USB receiver does.
>
> The code for fixing IrDA and IgorPLug should live inside their low
> level device drivers.  The characteristics of the errors produced by
> this hardware are known so a fix can be written to compensate.  The
> IgorPlug people might find it easier to fix their firmware. You don't
> have to get the timing exactly right for the protocol engines to work,
> you just need to get the big systematic errors out.

There is a real benefit to strict protocol engines. If the IgorPlus
people had strict protocol engines to test against they would have
discovered their firmware bugs during the initial development process.

>
> Before doing raw the low level IR drivers should be fixed to provide
> the most accurate data possible. I don't think it is good design for a
> low level driver to be injecting systematic errors and then have the
> next layer of code remove them.  The source of the systematic errors
> should be characterized and fixed in the low level driver. That also
> allows the fixed to be constrained to fixing data from only a single
> receiver type.
>
> I have been burnt twice in the past from fixing errors in a low level
> driver higher in the stack. I have learned the hard way that it is a
> bad thing to do. As the fixes accumulate in the higher layers you will
> reach a point where they conflict and no solution is possible. Bugs in
> the low level drivers need to be fixed in that driver.
>
> Show me a case that can't be fixed in the low level driver and we can
> explore the options.
>
>>
>> But of course the driver should try to generate timings as accurate as
>> possible.
>>
>> Christoph
>>
>
>
>
> --
> Jon Smirl
> jonsm...@gmail.com
>



-- 
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Sun, Aug 1, 2010 at 5:50 AM, Christoph Bartelmus  wrote:
> Hi Jon,
>
> on 31 Jul 10 at 14:14, Jon Smirl wrote:
>> On Sat, Jul 31, 2010 at 1:47 PM, Christoph Bartelmus 
>> wrote:
>>> Hi Jon,
>>>
>>> on 31 Jul 10 at 12:25, Jon Smirl wrote:
 On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
 wrote:
> I think you won't be able to fix the problem conclusively either way.  A
> lot of how the chip's clocks should be programmed depends on how the
> GPIOs are used and what crystal is used.
>
> I suspect many designers will use some reference design layout from ENE,
> but it won't be good in every case.  The wire-up of the ENE of various
> motherboards is likely something you'll have to live with as unknowns.
>
> This is a case where looser tolerances in the in kernel decoders could
> reduce this driver's complexity and/or get rid of arbitrary fudge
> factors in the driver.
>>>
 The tolerances are as loose as they can be. The NEC protocol uses
 pulses that are 4% longer than JVC. The decoders allow errors up to 2%
 (50% of 4%).  The crystals used in electronics are accurate to
 0.0001%+.
>>>
>>> But the standard IR receivers are far from being accurate enough to allow
>>> tolerance windows of only 2%.
>>> I'm surprised that this works for you. LIRC uses a standard tolerance of
>>> 30% / 100 us and even this is not enough sometimes.
>>>
>>> For the NEC protocol one signal consists of 22 individual pulses at 38kHz..
>>> If the receiver just misses one pulse, you already have an error of 1/22
 4%.
>
>> There are different types of errors. The decoders can take large
>> variations in bit times. The problem is with cumulative errors. In
>> this case the error had accumulated up to 450us in the lead pulse.
>> That's just too big of an error and caused the JVC code to be
>> misclassified as NEC.
>>
>> I think he said lirc was misclassifying it too. So we both did the same
>> thing.
>
> No way. JVC is a 16 bit code. NEC uses 32 bits. How can you ever confuse
> JVC with NEC signals?
>
> LIRC will work if there is a 4% or 40% or 400% error. Because irrecord
> generates the config file using your receiver it will compensate for any

At the end of the process we can build a record and match raw mode if
we have to.

> timing error. It will work with pulses cut down to 50 us like IrDA
> hardware does and it will work when half of the bits are swallowed like
> the IgorPlug USB receiver does.

The code for fixing IrDA and IgorPLug should live inside their low
level device drivers.  The characteristics of the errors produced by
this hardware are known so a fix can be written to compensate.  The
IgorPlug people might find it easier to fix their firmware. You don't
have to get the timing exactly right for the protocol engines to work,
you just need to get the big systematic errors out.

Before doing raw the low level IR drivers should be fixed to provide
the most accurate data possible. I don't think it is good design for a
low level driver to be injecting systematic errors and then have the
next layer of code remove them.  The source of the systematic errors
should be characterized and fixed in the low level driver. That also
allows the fixed to be constrained to fixing data from only a single
receiver type.

I have been burnt twice in the past from fixing errors in a low level
driver higher in the stack. I have learned the hard way that it is a
bad thing to do. As the fixes accumulate in the higher layers you will
reach a point where they conflict and no solution is possible. Bugs in
the low level drivers need to be fixed in that driver.

Show me a case that can't be fixed in the low level driver and we can
explore the options.

>
> But of course the driver should try to generate timings as accurate as
> possible.
>
> Christoph
>



-- 
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Christoph Bartelmus]

Hi Jon,

on 31 Jul 10 at 14:14, Jon Smirl wrote:
> On Sat, Jul 31, 2010 at 1:47 PM, Christoph Bartelmus 
> wrote:
>> Hi Jon,
>>
>> on 31 Jul 10 at 12:25, Jon Smirl wrote:
>>> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
>>> wrote:
 I think you won't be able to fix the problem conclusively either way.  A
 lot of how the chip's clocks should be programmed depends on how the
 GPIOs are used and what crystal is used.

 I suspect many designers will use some reference design layout from ENE,
 but it won't be good in every case.  The wire-up of the ENE of various
 motherboards is likely something you'll have to live with as unknowns.

 This is a case where looser tolerances in the in kernel decoders could
 reduce this driver's complexity and/or get rid of arbitrary fudge
 factors in the driver.
>>
>>> The tolerances are as loose as they can be. The NEC protocol uses
>>> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
>>> (50% of 4%).  The crystals used in electronics are accurate to
>>> 0.0001%+.
>>
>> But the standard IR receivers are far from being accurate enough to allow
>> tolerance windows of only 2%.
>> I'm surprised that this works for you. LIRC uses a standard tolerance of
>> 30% / 100 us and even this is not enough sometimes.
>>
>> For the NEC protocol one signal consists of 22 individual pulses at 38kHz..
>> If the receiver just misses one pulse, you already have an error of 1/22
>>> 4%.

> There are different types of errors. The decoders can take large
> variations in bit times. The problem is with cumulative errors. In
> this case the error had accumulated up to 450us in the lead pulse.
> That's just too big of an error and caused the JVC code to be
> misclassified as NEC.
>
> I think he said lirc was misclassifying it too. So we both did the same
> thing.

No way. JVC is a 16 bit code. NEC uses 32 bits. How can you ever confuse  
JVC with NEC signals?

LIRC will work if there is a 4% or 40% or 400% error. Because irrecord  
generates the config file using your receiver it will compensate for any  
timing error. It will work with pulses cut down to 50 us like IrDA  
hardware does and it will work when half of the bits are swollowed like  
the IgorPlug USB receiver does.

But of course the driver should try to generate timings as accurate as  
possible.

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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Christoph Bartelmus]

Hi Jon,

on 31 Jul 10 at 17:53, Jon Smirl wrote:
> On Sat, Jul 31, 2010 at 2:51 PM, Andy Walls  wrote:
>> On Sat, 2010-07-31 at 14:14 -0400, Jon Smirl wrote:
>>> On Sat, Jul 31, 2010 at 1:47 PM, Christoph Bartelmus 
>>> wrote:
 Hi Jon,

 on 31 Jul 10 at 12:25, Jon Smirl wrote:
> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
> wrote:
>> I think you won't be able to fix the problem conclusively either way..
>>  A lot of how the chip's clocks should be programmed depends on how the
>> GPIOs are used and what crystal is used.
>>
>> I suspect many designers will use some reference design layout from
>> ENE, but it won't be good in every case.  The wire-up of the ENE of
>> various motherboards is likely something you'll have to live with as
>> unknowns.
>>
>> This is a case where looser tolerances in the in kernel decoders could
>> reduce this driver's complexity and/or get rid of arbitrary fudge
>> factors in the driver.

> The tolerances are as loose as they can be. The NEC protocol uses
> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
> (50% of 4%).  The crystals used in electronics are accurate to
> 0.0001%+.

 But the standard IR receivers are far from being accurate enough to allow
 tolerance windows of only 2%.
 I'm surprised that this works for you. LIRC uses a standard tolerance of
 30% / 100 us and even this is not enough sometimes.

 For the NEC protocol one signal consists of 22 individual pulses at
 38kHz. If the receiver just misses one pulse, you already have an error
 of 1/22
> 4%.
>>>
>>> There are different types of errors. The decoders can take large
>>> variations in bit times. The problem is with cumulative errors. In
>>> this case the error had accumulated up to 450us in the lead pulse.
>>> That's just too big of an error
>>
>> Hi Jon,
>>
>> Hmmm.  Leader marks are, by protocol design, there to give time for the
>> receiver's AGC to settle.  We should make it OK to miss somewhat large
>> portions of leader marks.  I'm not sure what the harm is in accepting
>> too long of a leader mark, but I'm pretty sure a leader mark that is too
>> long will always be due to systematic error and not noise errors.
>>
>> However, if we know we have systematic errors caused by unknowns, we
>> should be designing and implementing a decoding system that reasonably
>> deals with those systematic errors.  Again the part of the system almost
>> completely out of our control is the remote controls, and we *have no
>> control* over systematic errors introduced by remotes.

> We haven't encountered remotes with systematic errors. If remotes had
> large errors in them they wouldn't be able to reliably control their
> target devices. Find a remote that won't work with the protocol
> engines and a reasonably accurate receiver.

>>
>> Obviously we want to reduce or eliminate systematic errors by
>> determining the unknowns and undoing their effects or by compensating
>> for their overall effect.  But in the case of the ENE receiver driver,
>> you didn't seem to like the Maxim's software compensation for the
>> systematic receiver errors.

> I would be happier if we could track down the source of the error
> instead of sticking a bandaid on at the end of the process.

>>> and caused the JVC code to be
>>> misclassified as NEC.
>>
>> I still have not heard why we need protocol discrimination/classifcation
>> in the kernel.  Doing discrimination between two protocols with such
>> close timings is whose requirement again?

> If we don't do protocol engines we have to revert back to raw
> recording and having everyone train the system with their remotes. The
> goal is to eliminate the training step. We would also have to have
> large files (LIRC configs) for building the keymaps and a new API to
> deal with them. With the engines the key presses are identified by
> short strings.

Only 437 of 3486 config files on lirc.org use raw mode (probably what you  
mean with large files). Many of them are recorded with an very old  
irrecord version. Current versions of irrecord wouldn't create a raw mode  
config file for these remotes.

> A use case: install mythtv, add an IR receiver. Myth UI says to set
> your universal remote to a Motorola DVR profile. Remote works - no
> training step needed.

+ Myth UI reconfigures lircd with an existing Motorola DVR config file.
Where's the difference?

> LIRC has protocol engines too. irrecord first tries to fit the remote
> into a protocol engine.

With the sublte difference to your approach that LIRC does not make any  
assumptions on any timings in contrast to hardcoded values in the kernel.

> If it can't it reverts to raw mode. Let's
> analyze those cases where lirc ends up in raw mode and see if we can
> figure out what's going wrong.
>
> For example I know of two things that will trip up irrecord that are
> fixed in the kernel system
> 

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

On Sat, 2010-07-31 at 17:53 -0400, Jon Smirl wrote: 
> On Sat, Jul 31, 2010 at 2:51 PM, Andy Walls  wrote:
> > On Sat, 2010-07-31 at 14:14 -0400, Jon Smirl wrote:
> >> On Sat, Jul 31, 2010 at 1:47 PM, Christoph Bartelmus  
> >> wrote:
> >> > Hi Jon,
> >> >
> >> > on 31 Jul 10 at 12:25, Jon Smirl wrote:
> >> >> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
> >> >> wrote:
> >> >>> I think you won't be able to fix the problem conclusively either way.  
> >> >>> A
> >> >>> lot of how the chip's clocks should be programmed depends on how the
> >> >>> GPIOs are used and what crystal is used.
> >> >>>
> >> >>> I suspect many designers will use some reference design layout from 
> >> >>> ENE,
> >> >>> but it won't be good in every case.  The wire-up of the ENE of various
> >> >>> motherboards is likely something you'll have to live with as unknowns.
> >> >>>
> >> >>> This is a case where looser tolerances in the in kernel decoders could
> >> >>> reduce this driver's complexity and/or get rid of arbitrary fudge
> >> >>> factors in the driver.
> >> >
> >> >> The tolerances are as loose as they can be. The NEC protocol uses
> >> >> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
> >> >> (50% of 4%).  The crystals used in electronics are accurate to
> >> >> 0.0001%+.
> >> >
> >> > But the standard IR receivers are far from being accurate enough to allow
> >> > tolerance windows of only 2%.
> >> > I'm surprised that this works for you. LIRC uses a standard tolerance of
> >> > 30% / 100 us and even this is not enough sometimes.
> >> >
> >> > For the NEC protocol one signal consists of 22 individual pulses at 
> >> > 38kHz.
> >> > If the receiver just misses one pulse, you already have an error of 1/22
> >> >> 4%.
> >>
> >> There are different types of errors. The decoders can take large
> >> variations in bit times. The problem is with cumulative errors. In
> >> this case the error had accumulated up to 450us in the lead pulse.
> >> That's just too big of an error
> >
> > Hi Jon,
> >
> > Hmmm.  Leader marks are, by protocol design, there to give time for the
> > receiver's AGC to settle.  We should make it OK to miss somewhat large
> > portions of leader marks.  I'm not sure what the harm is in accepting
> > too long of a leader mark, but I'm pretty sure a leader mark that is too
> > long will always be due to systematic error and not noise errors.
> >
> > However, if we know we have systematic errors caused by unknowns, we
> > should be designing and implementing a decoding system that reasonably
> > deals with those systematic errors.  Again the part of the system almost
> > completely out of our control is the remote controls, and we *have no
> > control* over systematic errors introduced by remotes.
> 
> We haven't encountered remotes with systematic errors. If remotes had
> large errors in them they wouldn't be able to reliably control their
> target devices. Find a remote that won't work with the protocol
> engines and a reasonably accurate receiver.
> 
> >
> > Obviously we want to reduce or eliminate systematic errors by
> > determining the unknowns and undoing their effects or by compensating
> > for their overall effect.  But in the case of the ENE receiver driver,
> > you didn't seem to like the Maxim's software compensation for the
> > systematic receiver errors.
> 
> I would be happier if we could track down the source of the error
> instead of sticking a bandaid on at the end of the process.
This isn't a bandaid.
Windows driver programs the period to 52 but treats it as a 50.
(I don't do that because I set period to 75 - otherwise leading pulse of
NEC/JVC is almost missing. I know the reason for that, and it isn't
important).




> 
> >> and caused the JVC code to be
> >> misclassified as NEC.
> >
> > I still have not heard why we need protocol discrimination/classifcation
> > in the kernel.  Doing discrimination between two protocols with such
> > close timings is whose requirement again?
> 
> If we don't do protocol engines we have to revert back to raw
> recording and having everyone train the system with their remotes. The
> goal is to eliminate the training step. We would also have to have
> large files (LIRC configs) for building the keymaps and a new API to
> deal with them. With the engines the key presses are identified by
> short strings.
> 
> A use case: install mythtv, add an IR receiver. Myth UI says to set
> your universal remote to a Motorola DVR profile. Remote works - no
> training step needed.
> 
> LIRC has protocol engines too. irrecord first tries to fit the remote
> into a protocol engine. If it can't it reverts to raw mode. Let's
> analyze those cases where lirc ends up in raw mode and see if we can
> figure out what's going wrong.
> 
> For example I know of two things that will trip up irrecord that are
> fixed in the kernel system
> 1) the ene driver. we know now it had a 4% error in the reported periods
No it doesn't
It even works if leading large pulse is

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Sat, Jul 31, 2010 at 2:51 PM, Andy Walls  wrote:
> On Sat, 2010-07-31 at 14:14 -0400, Jon Smirl wrote:
>> On Sat, Jul 31, 2010 at 1:47 PM, Christoph Bartelmus  
>> wrote:
>> > Hi Jon,
>> >
>> > on 31 Jul 10 at 12:25, Jon Smirl wrote:
>> >> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
>> >> wrote:
>> >>> I think you won't be able to fix the problem conclusively either way.  A
>> >>> lot of how the chip's clocks should be programmed depends on how the
>> >>> GPIOs are used and what crystal is used.
>> >>>
>> >>> I suspect many designers will use some reference design layout from ENE,
>> >>> but it won't be good in every case.  The wire-up of the ENE of various
>> >>> motherboards is likely something you'll have to live with as unknowns.
>> >>>
>> >>> This is a case where looser tolerances in the in kernel decoders could
>> >>> reduce this driver's complexity and/or get rid of arbitrary fudge
>> >>> factors in the driver.
>> >
>> >> The tolerances are as loose as they can be. The NEC protocol uses
>> >> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
>> >> (50% of 4%).  The crystals used in electronics are accurate to
>> >> 0.0001%+.
>> >
>> > But the standard IR receivers are far from being accurate enough to allow
>> > tolerance windows of only 2%.
>> > I'm surprised that this works for you. LIRC uses a standard tolerance of
>> > 30% / 100 us and even this is not enough sometimes.
>> >
>> > For the NEC protocol one signal consists of 22 individual pulses at 38kHz.
>> > If the receiver just misses one pulse, you already have an error of 1/22
>> >> 4%.
>>
>> There are different types of errors. The decoders can take large
>> variations in bit times. The problem is with cumulative errors. In
>> this case the error had accumulated up to 450us in the lead pulse.
>> That's just too big of an error
>
> Hi Jon,
>
> Hmmm.  Leader marks are, by protocol design, there to give time for the
> receiver's AGC to settle.  We should make it OK to miss somewhat large
> portions of leader marks.  I'm not sure what the harm is in accepting
> too long of a leader mark, but I'm pretty sure a leader mark that is too
> long will always be due to systematic error and not noise errors.
>
> However, if we know we have systematic errors caused by unknowns, we
> should be designing and implementing a decoding system that reasonably
> deals with those systematic errors.  Again the part of the system almost
> completely out of our control is the remote controls, and we *have no
> control* over systematic errors introduced by remotes.

We haven't encountered remotes with systematic errors. If remotes had
large errors in them they wouldn't be able to reliably control their
target devices. Find a remote that won't work with the protocol
engines and a reasonably accurate receiver.

>
> Obviously we want to reduce or eliminate systematic errors by
> determining the unknowns and undoing their effects or by compensating
> for their overall effect.  But in the case of the ENE receiver driver,
> you didn't seem to like the Maxim's software compensation for the
> systematic receiver errors.

I would be happier if we could track down the source of the error
instead of sticking a bandaid on at the end of the process.

>> and caused the JVC code to be
>> misclassified as NEC.
>
> I still have not heard why we need protocol discrimination/classifcation
> in the kernel.  Doing discrimination between two protocols with such
> close timings is whose requirement again?

If we don't do protocol engines we have to revert back to raw
recording and having everyone train the system with their remotes. The
goal is to eliminate the training step. We would also have to have
large files (LIRC configs) for building the keymaps and a new API to
deal with them. With the engines the key presses are identified by
short strings.

A use case: install mythtv, add an IR receiver. Myth UI says to set
your universal remote to a Motorola DVR profile. Remote works - no
training step needed.

LIRC has protocol engines too. irrecord first tries to fit the remote
into a protocol engine. If it can't it reverts to raw mode. Let's
analyze those cases where lirc ends up in raw mode and see if we can
figure out what's going wrong.

For example I know of two things that will trip up irrecord that are
fixed in the kernel system
1) the ene driver. we know now it had a 4% error in the reported periods
2) Sony remotes - they mix protocols on a single remote.

> Since these two protocols have such close timings that systematic errors
> can cause misclassification when using simple mark or space measurements
> against fixed thresholds, it indicates that a more sophisticated
> discrimination mechanism would be needed.  Perhaps one that takes multiple
> successive measurements into account?

If we get to the point where we need more sophisticated
classifications we can build it. But are we at that point yet? I'd
prefer to initially leave everything pretty strict as a

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Andy Walls]

On Sat, 2010-07-31 at 14:14 -0400, Jon Smirl wrote:
> On Sat, Jul 31, 2010 at 1:47 PM, Christoph Bartelmus  
> wrote:
> > Hi Jon,
> >
> > on 31 Jul 10 at 12:25, Jon Smirl wrote:
> >> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
> >> wrote:
> >>> I think you won't be able to fix the problem conclusively either way.  A
> >>> lot of how the chip's clocks should be programmed depends on how the
> >>> GPIOs are used and what crystal is used.
> >>>
> >>> I suspect many designers will use some reference design layout from ENE,
> >>> but it won't be good in every case.  The wire-up of the ENE of various
> >>> motherboards is likely something you'll have to live with as unknowns.
> >>>
> >>> This is a case where looser tolerances in the in kernel decoders could
> >>> reduce this driver's complexity and/or get rid of arbitrary fudge
> >>> factors in the driver.
> >
> >> The tolerances are as loose as they can be. The NEC protocol uses
> >> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
> >> (50% of 4%).  The crystals used in electronics are accurate to
> >> 0.0001%+.
> >
> > But the standard IR receivers are far from being accurate enough to allow
> > tolerance windows of only 2%.
> > I'm surprised that this works for you. LIRC uses a standard tolerance of
> > 30% / 100 us and even this is not enough sometimes.
> >
> > For the NEC protocol one signal consists of 22 individual pulses at 38kHz.
> > If the receiver just misses one pulse, you already have an error of 1/22
> >> 4%.
> 
> There are different types of errors. The decoders can take large
> variations in bit times. The problem is with cumulative errors. In
> this case the error had accumulated up to 450us in the lead pulse.
> That's just too big of an error 

Hi Jon,

Hmmm.  Leader marks are, by protocol design, there to give time for the
receiver's AGC to settle.  We should make it OK to miss somewhat large
portions of leader marks.  I'm not sure what the harm is in accepting
too long of a leader mark, but I'm pretty sure a leader mark that is too
long will always be due to systematic error and not noise errors.

However, if we know we have systematic errors caused by unknowns, we
should be designing and imlpementing a decoding system that reasonably
deals with those systematic errors.  Again the part of the system almost
completely out of our control is the remote controls, and we *have no
control* over systematic errors introduced by remotes.

Obviously we want to reduce or elimiinate systematic errors by
determining the unknowns and undoing their effects or by compensating
for their overall effect.  But in the case of the ENE receiver driver,
you didn't seem to like the Maxim's software compensation for the
systematic receiver errors.


> and caused the JVC code to be
> misclassified as NEC.

I still have not heard why we need protocol discrimination/classifcation
in the kernel.  Doing discrimination between two protocols with such
close timings is whose requirement again?

Since these two protocols have such close timings that systematic errors
can cause misclassifcation when using simple mark or space measurments
against fixed thresholds, it indicates that a more sophisticated
discrimation mechanism would be needed.  Perhaps one that takes multiple
successive measurments into account?

Regards,
Andy

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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Sat, Jul 31, 2010 at 2:14 PM, Jon Smirl  wrote:
> On Sat, Jul 31, 2010 at 1:47 PM, Christoph Bartelmus  
> wrote:
>> Hi Jon,
>>
>> on 31 Jul 10 at 12:25, Jon Smirl wrote:
>>> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
>>> wrote:
 I think you won't be able to fix the problem conclusively either way.  A
 lot of how the chip's clocks should be programmed depends on how the
 GPIOs are used and what crystal is used.

 I suspect many designers will use some reference design layout from ENE,
 but it won't be good in every case.  The wire-up of the ENE of various
 motherboards is likely something you'll have to live with as unknowns.

 This is a case where looser tolerances in the in kernel decoders could
 reduce this driver's complexity and/or get rid of arbitrary fudge
 factors in the driver.
>>
>>> The tolerances are as loose as they can be. The NEC protocol uses
>>> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
>>> (50% of 4%).  The crystals used in electronics are accurate to
>>> 0.0001%+.
>>
>> But the standard IR receivers are far from being accurate enough to allow
>> tolerance windows of only 2%.
>> I'm surprised that this works for you. LIRC uses a standard tolerance of
>> 30% / 100 us and even this is not enough sometimes.
>>
>> For the NEC protocol one signal consists of 22 individual pulses at 38kHz.
>> If the receiver just misses one pulse, you already have an error of 1/22
>>> 4%.
>
> There are different types of errors. The decoders can take large
> variations in bit times. The problem is with cumulative errors. In
> this case the error had accumulated up to 450us in the lead pulse.
> That's just too big of an error and caused the JVC code to be
> misclassified as NEC.

I only see two solutions to this problem:

1) fix the driver to semi-accurately report correct measurements. A
consistent off by 4% error is simply too much since the NEC protocol
is a 4% stretched version of the JVC protocol. If the driver is
stretching JVC by 4% it has effectively converted it into a broken NEC
message. And that's what the decoders detected.  Given that the NEC
protocol is a 4% stretched JVC the largest safe timing variance is 2%
(half of 4%).  That 2% number is nothing to do with the code, it is
caused by the definitions of the JVC and NEC protocol timings.

2) Implement a record and match mode that knows nothing about
protocols. LIRC has this in the raw protocol. That would fix this
problem, but we're treating the symptom not the disease. The disease
is the broken IR driver.

I'd rather hold off on the raw protocol and try to fix the base IR
drivers first.


>
> I think he said lirc was misclassifying it too. So we both did the same thing.
>
>
>>
>> Christoph
>>
>
>
>
> --
> Jon Smirl
> jonsm...@gmail.com
>



-- 
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Sat, Jul 31, 2010 at 1:47 PM, Christoph Bartelmus  wrote:
> Hi Jon,
>
> on 31 Jul 10 at 12:25, Jon Smirl wrote:
>> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
>> wrote:
>>> I think you won't be able to fix the problem conclusively either way.  A
>>> lot of how the chip's clocks should be programmed depends on how the
>>> GPIOs are used and what crystal is used.
>>>
>>> I suspect many designers will use some reference design layout from ENE,
>>> but it won't be good in every case.  The wire-up of the ENE of various
>>> motherboards is likely something you'll have to live with as unknowns.
>>>
>>> This is a case where looser tolerances in the in kernel decoders could
>>> reduce this driver's complexity and/or get rid of arbitrary fudge
>>> factors in the driver.
>
>> The tolerances are as loose as they can be. The NEC protocol uses
>> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
>> (50% of 4%).  The crystals used in electronics are accurate to
>> 0.0001%+.
>
> But the standard IR receivers are far from being accurate enough to allow
> tolerance windows of only 2%.
> I'm surprised that this works for you. LIRC uses a standard tolerance of
> 30% / 100 us and even this is not enough sometimes.
>
> For the NEC protocol one signal consists of 22 individual pulses at 38kHz.
> If the receiver just misses one pulse, you already have an error of 1/22
>> 4%.

There are different types of errors. The decoders can take large
variations in bit times. The problem is with cumulative errors. In
this case the error had accumulated up to 450us in the lead pulse.
That's just too big of an error and caused the JVC code to be
misclassified as NEC.

I think he said lirc was misclassifying it too. So we both did the same thing.


>
> Christoph
>



-- 
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Christoph Bartelmus]

Hi Jon,

on 31 Jul 10 at 12:25, Jon Smirl wrote:
> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls 
> wrote:
>> I think you won't be able to fix the problem conclusively either way.  A
>> lot of how the chip's clocks should be programmed depends on how the
>> GPIOs are used and what crystal is used.
>>
>> I suspect many designers will use some reference design layout from ENE,
>> but it won't be good in every case.  The wire-up of the ENE of various
>> motherboards is likely something you'll have to live with as unknowns.
>>
>> This is a case where looser tolerances in the in kernel decoders could
>> reduce this driver's complexity and/or get rid of arbitrary fudge
>> factors in the driver.

> The tolerances are as loose as they can be. The NEC protocol uses
> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
> (50% of 4%).  The crystals used in electronics are accurate to
> 0.0001%+.

But the standard IR receivers are far from being accurate enough to allow
tolerance windows of only 2%.
I'm surprised that this works for you. LIRC uses a standard tolerance of
30% / 100 us and even this is not enough sometimes.

For the NEC protocol one signal consists of 22 individual pulses at 38kHz.
If the receiver just misses one pulse, you already have an error of 1/22
> 4%.

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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

On Sat, 2010-07-31 at 12:25 -0400, Jon Smirl wrote: 
> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls  wrote:
> > I think you won't be able to fix the problem conclusively either way.  A
> > lot of how the chip's clocks should be programmed depends on how the
> > GPIOs are used and what crystal is used.
> >
> > I suspect many designers will use some reference design layout from ENE,
> > but it won't be good in every case.  The wire-up of the ENE of various
> > motherboards is likely something you'll have to live with as unknowns.
> >
> > This is a case where looser tolerances in the in kernel decoders could
> > reduce this driver's complexity and/or get rid of arbitrary fudge
> > factors in the driver.
> 
> The tolerances are as loose as they can be. The NEC protocol uses
> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
> (50% of 4%).  The crystals used in electronics are accurate to
> 0.0001%+.  The 4% error in this driver is because the hardware is not
> being programmed accurately. This needs to be fixed in the driver and
> not in the upper layers.
> 
> How is sample period being computed, where is the complete source to
> this driver?
> 
>dev->tx_period = 32;
> 
> Where is sample_period computed?
> 
> @@ -672,13 +583,25 @@ static irqreturn_t ene_isr(int irq, void *data)
>pulse = !(hw_value & ENE_SAMPLE_SPC_MASK);
>hw_value &= ENE_SAMPLE_VALUE_MASK;
>hw_sample = hw_value * sample_period;
> +
> +   if (dev->rx_period_adjust) {
> +   hw_sample *= (100 - dev->rx_period_adjust);
> +   hw_sample /= 100;
> +   }
>}
> 
> I suspect sample_period is set to 32us. For 32.768Mhz the period needs
> to be 30.5us. I don't see the code for how it was computed.
> 
> You have to be careful with rounding errors when doing this type of
> computation. What looks like a minor error can amplify into a large
> error. Sometimes I do the math in 64b ints just to keep the round off
> errors from accumulating.  Instead of doing the math in calculator and
> plugging in 32. Use #defines and do the math in the
There is no reason to worry about rounding here.

hw_sample is maximum of 127 * 50, so when I muliply by 100 I get exact
result.
Then I do one divide.

Best regards,
Maxim Levitsky



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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

On Sat, 2010-07-31 at 12:25 -0400, Jon Smirl wrote: 
> On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls  wrote:
> > I think you won't be able to fix the problem conclusively either way.  A
> > lot of how the chip's clocks should be programmed depends on how the
> > GPIOs are used and what crystal is used.
> >
> > I suspect many designers will use some reference design layout from ENE,
> > but it won't be good in every case.  The wire-up of the ENE of various
> > motherboards is likely something you'll have to live with as unknowns.
> >
> > This is a case where looser tolerances in the in kernel decoders could
> > reduce this driver's complexity and/or get rid of arbitrary fudge
> > factors in the driver.
> 
> The tolerances are as loose as they can be. The NEC protocol uses
> pulses that are 4% longer than JVC. The decoders allow errors up to 2%
> (50% of 4%).  The crystals used in electronics are accurate to
> 0.0001%+.  The 4% error in this driver is because the hardware is not
> being programmed accurately. This needs to be fixed in the driver and
> not in the upper layers.

Let me explain again.

I get samples in 4 byte buffer. each sample is a count of sample
periods.
Sample period is programmed into hardware, at 'ENE_CIR_SAMPLE_PERIOD'
(it is in us)

Default sample period is 50 us.

The error source isn't 'electronics' fault.
The device is microprocessor.
I don't read the samples 'directly' from hardware, but rather from ram
of that microprocessor.
I don't know how it samples the input.
A expiration of sample period might just cause a IRQ inside that
microprocessor, and it can't process it instantly. That is probably the
source of the delay.
Or something like that.

Best regards,
Maxim Levitsky

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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Sat, Jul 31, 2010 at 11:12 AM, Andy Walls  wrote:
> I think you won't be able to fix the problem conclusively either way.  A
> lot of how the chip's clocks should be programmed depends on how the
> GPIOs are used and what crystal is used.
>
> I suspect many designers will use some reference design layout from ENE,
> but it won't be good in every case.  The wire-up of the ENE of various
> motherboards is likely something you'll have to live with as unknowns.
>
> This is a case where looser tolerances in the in kernel decoders could
> reduce this driver's complexity and/or get rid of arbitrary fudge
> factors in the driver.

The tolerances are as loose as they can be. The NEC protocol uses
pulses that are 4% longer than JVC. The decoders allow errors up to 2%
(50% of 4%).  The crystals used in electronics are accurate to
0.0001%+.  The 4% error in this driver is because the hardware is not
being programmed accurately. This needs to be fixed in the driver and
not in the upper layers.

How is sample period being computed, where is the complete source to
this driver?

   dev->tx_period = 32;

Where is sample_period computed?

@@ -672,13 +583,25 @@ static irqreturn_t ene_isr(int irq, void *data)
   pulse = !(hw_value & ENE_SAMPLE_SPC_MASK);
   hw_value &= ENE_SAMPLE_VALUE_MASK;
   hw_sample = hw_value * sample_period;
+
+   if (dev->rx_period_adjust) {
+   hw_sample *= (100 - dev->rx_period_adjust);
+   hw_sample /= 100;
+   }
   }

I suspect sample_period is set to 32us. For 32.768Mhz the period needs
to be 30.5us. I don't see the code for how it was computed.

You have to be careful with rounding errors when doing this type of
computation. What looks like a minor error can amplify into a large
error. Sometimes I do the math in 64b ints just to keep the round off
errors from accumulating.  Instead of doing the math in calculator and
plugging in 32. Use #defines and do the math in the code.

Maybe something like
#define sample_period  (1 / (32768 * 1000))

Then don't store this constant in a variable since it will cause a
round off. Just use it directly in the computation.

>
> Regards,
> Andy
>
>



-- 
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Andy Walls]

On Fri, 2010-07-30 at 15:45 +0300, Maxim Levitsky wrote:
> On Fri, 2010-07-30 at 08:07 -0400, Jon Smirl wrote: 
> > On Fri, Jul 30, 2010 at 8:02 AM, Jon Smirl  wrote:

> > >>> >
> > >>>
> > >>> Should that be a <= or >= instead of !=?
> > >>> +   if (pll_freq != 1000)
> > >>
> > >> This is how its done in windows driver.
> > >
> > > That doesn't mean it is bug free.
> 
> This PLL frequency is likely to be chip internal frequency.
> And windows driver doesn't touch it.
> Its embedded controller, so I don't want to touch things I am not sure
> about.


The KB3700 datasheet states there are 4 clock domains in the chip.

One of the clock domains is a PLL LOW domain, used to clock
miscellaneous peripherials (which probably includes CIR on similar
chips).   The defualt for this clock appears to be 32.768 kHz clock
derived from a 32.768 MHz clock from which a 32.768 kHz clock is
derived.  It seems to be set up in the EC (ACPI 2.0 Embedded Controller)
register bank of the KB3700 chip.

That 1000 (0x3e8) is the default divider value to go from 32.768 MHz to
32.768 kHz.  I suspect it could be off by one - 0x3e7 might be the right
value - but that is only a 30 ns difference in the 30 us clock period.


So the check for 1000 by the Windows driver is likely a check for the
chip being in it's default configuration.  Looking at the CLKCFG2
register, the PLL can apparently output a 25 MHz clock instead of a
32.768 MHz clock.

While I'm looking at CLKCFG2, I note the default divider value of 0x1f
(31) for 1000 ns is wrong as well:

32 / 32.768 MHz ~= 977 ns = 0.977 us   (-2.3%)

where as

33 / 32.768 MHz ~= 1007 ns = 1.007 us  (+0.7%)

so the CLKCFG2 register programmed with 0x20 (32) would a better divisor
for a 1 us time period, if the functions in the chip can tolerate being
a little late vs. early.

I also read that the PLL reference comes from the LPC portion of the
chip which is the PCI clock domain.  So if a 33 MHz reference is used
instead of 32.768 MHz, then the default CLKCFG2 value yields this for a
nominal 1 us:

32 / 33.333 MHz ~= 960 ns = 0.960 us   (-4.0%)
 





> > > Experimenting with changing the PLL frequency register may correct the
> > > error.  Try taking 96% of pll_freq and write it back into these
> > > register. This would be easy to fix with a manual. The root problem is
> > > almost certainly a bug in the way the PLLs were programmed.
> > >
> > > I don't like putting in fudge factors like the 4% correction. What
> > > happens if a later version of the hardware has fixed firmware? I
> > > normal user is never going to figure out that they need to change the
> > > fudge factor.
> I don't think that is a hardware bug, rather a limitation.
> 
> Lets leave it as is.
> I will soon publish the driver on launchpad or something like that and
> try to contact users I debugged that driver with, and then see what
> ranges PLL register takes.

I think you won't be able to fix the problem conclusively either way.  A
lot of how the chip's clocks should be programmed depends on how the
GPIOs are used and what crystal is used.

I suspect many designers will use some reference design layout from ENE,
but it won't be good in every case.  The wire-up of the ENE of various
motherboards is likely something you'll have to live with as unknowns.

This is a case where looser tolerances in the in kernel decoders could
reduce this driver's complexity and/or get rid of arbitrary fudge
factors in the driver.

Regards,
Andy

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

[PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 MAINTAINERS   |6 +
 drivers/media/IR/Kconfig  |   14 +
 drivers/media/IR/Makefile |1 +
 drivers/media/IR/ene_ir.c |  595 +
 drivers/media/IR/ene_ir.h |   51 ++---
 5 files changed, 265 insertions(+), 402 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 56a36d7..587785a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2188,6 +2188,12 @@ F:   drivers/misc/cb710/
 F: drivers/mmc/host/cb710-mmc.*
 F: include/linux/cb710.h
 
+ENE KB2426 (ENE0100/ENE020XX) INFRARED RECEIVER
+M: Maxim Levitsky 
+S: Maintained
+F: drivers/media/IR/ene_ir.c
+F: drivers/media/IR/ene_ir.h
+
 EPSON 1355 FRAMEBUFFER DRIVER
 M: Christopher Hoover 
 M: Christopher Hoover 
diff --git a/drivers/media/IR/Kconfig b/drivers/media/IR/Kconfig
index fc48a3f..3f62bf9 100644
--- a/drivers/media/IR/Kconfig
+++ b/drivers/media/IR/Kconfig
@@ -105,4 +105,18 @@ config IR_MCEUSB
   To compile this driver as a module, choose M here: the
   module will be called mceusb.
 
+config IR_ENE
+   tristate "ENE eHome Receiver/Transciever (pnp id: ENE0100/ENE02xxx)"
+   depends on PNP
+   depends on IR_CORE
+   ---help---
+  Say Y here to enable support for integrated infrared receiver
+  /transciever made by ENE.
+
+  You can see if you have it by looking at lspnp output.
+  Output should include ENE0100 ENE0200 or something similiar.
+
+  To compile this driver as a module, choose M here: the
+  module will be called ene_ir.
+
 endif #IR_CORE
diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
index 2ae4f3a..3262a68 100644
--- a/drivers/media/IR/Makefile
+++ b/drivers/media/IR/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_IR_LIRC_CODEC) += ir-lirc-codec.o
 # stand-alone IR receivers/transmitters
 obj-$(CONFIG_IR_IMON) += imon.o
 obj-$(CONFIG_IR_MCEUSB) += mceusb.o
+obj-$(CONFIG_IR_ENE) += ene_ir.o
diff --git a/drivers/media/IR/ene_ir.c b/drivers/media/IR/ene_ir.c
index 9d11caf..5447750 100644
--- a/drivers/media/IR/ene_ir.c
+++ b/drivers/media/IR/ene_ir.c
@@ -1,5 +1,5 @@
 /*
- * driver for ENE KB3926 B/C/D CIR (also known as ENE0100/ENE0200/ENE0201)
+ * driver for ENE KB3926 B/C/D CIR (pnp id: ENE0XXX)
  *
  * Copyright (C) 2010 Maxim Levitsky 
  *
@@ -25,20 +25,20 @@
 #include 
 #include 
 #include 
-#include 
-#include "lirc_ene0100.h"
+#include 
+#include 
+#include 
+#include 
+#include "ene_ir.h"
 
 
 static int sample_period = -1;
 static int enable_idle = 1;
-static int enable_duty_carrier;
 static int input = 1;
 static int debug;
 static int txsim;
 
-static void ene_rx_set_idle(struct ene_device *dev, int idle);
 static int ene_irq_status(struct ene_device *dev);
-static void ene_send_sample(struct ene_device *dev, unsigned long sample);
 
 /* read a hardware register */
 static u8 ene_hw_read_reg(struct ene_device *dev, u16 reg)
@@ -85,6 +85,7 @@ static int ene_hw_detect(struct ene_device *dev)
u8 hw_revision, old_ver;
u8 tmp;
u8 fw_capabilities;
+   int pll_freq;
 
tmp = ene_hw_read_reg(dev, ENE_HW_UNK);
ene_hw_write_reg(dev, ENE_HW_UNK, tmp & ~ENE_HW_UNK_CLR);
@@ -96,6 +97,17 @@ static int ene_hw_detect(struct ene_device *dev)
hw_revision = ene_hw_read_reg(dev, ENE_HW_VERSION);
old_ver = ene_hw_read_reg(dev, ENE_HW_VER_OLD);
 
+   pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
+   (ene_hw_read_reg(dev, ENE_PLLFRL) >> 4);
+
+   if (pll_freq != 1000)
+   dev->rx_period_adjust = 4;
+   else
+   dev->rx_period_adjust = 2;
+
+
+   ene_printk(KERN_NOTICE, "PLL freq = %d\n", pll_freq);
+
if (hw_revision == 0xFF) {
 
ene_printk(KERN_WARNING, "device seems to be disabled\n");
@@ -160,7 +172,7 @@ static int ene_hw_detect(struct ene_device *dev)
 }
 
 /* this enables/disables IR input via gpio40*/
-static void ene_enable_gpio40_recieve(struct ene_device *dev, int enable)
+static void ene_enable_gpio40_receive(struct ene_device *dev, int enable)
 {
ene_hw_write_reg_mask(dev, ENE_CIR_CONF2, enable ?
  0 : ENE_CIR_CONF2_GPIO40DIS,
@@ -168,13 +180,13 @@ static void ene_enable_gpio40_recieve(struct ene_device 
*dev, int enable)
 }
 
 /* this enables/disables IR via standard input */
-static void ene_enable_normal_recieve(struct ene_device *dev, int enable)
+static void ene_enable_normal_receive(struct ene_device *dev, int enable)
 {
ene_hw_write_reg(dev, ENE_CIR_CONF1, enable ? ENE_CIR_CONF1_RX_ON : 0);
 }
 
 /* this enables/disables IR input via unused fan tachtometer input */
-static void ene_enable_fan_recieve(struct ene_device *dev, int enable)
+static void ene_enable_fan_receive(struct ene_device *dev, int enable)
 {
if (!enable)
ene_hw_write_reg(dev, ENE_FAN_AS_IN1, 0);
@@ -186,7 +198,7 @@ static void ene_enable_fan_recieve

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

On Sat, 2010-07-31 at 10:37 -0400, Jon Smirl wrote: 
> On Sat, Jul 31, 2010 at 10:28 AM, Maxim Levitsky
>  wrote:
> > On Sat, 2010-07-31 at 09:55 -0400, Andy Walls wrote:
> >> On Fri, 2010-07-30 at 15:45 +0300, Maxim Levitsky wrote:
> >> > On Fri, 2010-07-30 at 08:07 -0400, Jon Smirl wrote:
> >> > > On Fri, Jul 30, 2010 at 8:02 AM, Jon Smirl  wrote:
> >> > > > On Fri, Jul 30, 2010 at 7:54 AM, Maxim Levitsky 
> >> > > >  wrote:
> >>
> >> >
> >> > > >
> >> > > > +   pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
> >> > > > +   (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
> >> > >
> >> >
> >> >
> >> > > I can understand the shift of the high bits, but that shift of the low
> >> > > bits is unlikely.  A manual would tell us if it is right.
> >> > >
> >> > This shift is correct (according to datasheet, which contains mostly
> >> > useless info, but it does dociment this reg briefly.)
> >>
> >> The KB3700 series datasheet indicates that the value from ENE_PLLFRL
> >> should be shifted by >> 4 bits, not by >> 2.  Of course, the KB3700
> >> isn't the exact same chip.
> > You are right about that, thanks!
> 
> I looked at KB3700 manual. It says it is trying to make a 32Mhz clock
> by multiplying 32.768Khz * 1000.
> 
> 32,768 * 1000 = 32.768Mhz is a 2.4% error.
> 
> When you are computing the timings of the pulses did you assume a
> 32Mhz clock? It looks like the clock is actuall 32.768Mhz.
No, I just take the samples hardware give me.
Lets just leave this as is.


Best regards,
Maxim Levitsky

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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Sat, Jul 31, 2010 at 10:28 AM, Maxim Levitsky
 wrote:
> On Sat, 2010-07-31 at 09:55 -0400, Andy Walls wrote:
>> On Fri, 2010-07-30 at 15:45 +0300, Maxim Levitsky wrote:
>> > On Fri, 2010-07-30 at 08:07 -0400, Jon Smirl wrote:
>> > > On Fri, Jul 30, 2010 at 8:02 AM, Jon Smirl  wrote:
>> > > > On Fri, Jul 30, 2010 at 7:54 AM, Maxim Levitsky 
>> > > >  wrote:
>>
>> >
>> > > >
>> > > > +       pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
>> > > > +               (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
>> > >
>> >
>> >
>> > > I can understand the shift of the high bits, but that shift of the low
>> > > bits is unlikely.  A manual would tell us if it is right.
>> > >
>> > This shift is correct (according to datasheet, which contains mostly
>> > useless info, but it does dociment this reg briefly.)
>>
>> The KB3700 series datasheet indicates that the value from ENE_PLLFRL
>> should be shifted by >> 4 bits, not by >> 2.  Of course, the KB3700
>> isn't the exact same chip.
> You are right about that, thanks!

I looked at KB3700 manual. It says it is trying to make a 32Mhz clock
by multiplying 32.768Khz * 1000.

32,768 * 1000 = 32.768Mhz is a 2.4% error.

When you are computing the timings of the pulses did you assume a
32Mhz clock? It looks like the clock is actuall 32.768Mhz.


>
> Best regards,
> Maxim Levitsky
>
>



-- 
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

On Sat, 2010-07-31 at 09:55 -0400, Andy Walls wrote: 
> On Fri, 2010-07-30 at 15:45 +0300, Maxim Levitsky wrote:
> > On Fri, 2010-07-30 at 08:07 -0400, Jon Smirl wrote: 
> > > On Fri, Jul 30, 2010 at 8:02 AM, Jon Smirl  wrote:
> > > > On Fri, Jul 30, 2010 at 7:54 AM, Maxim Levitsky 
> > > >  wrote:
> 
> > 
> > > >
> > > > +   pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
> > > > +   (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
> > > 
> > 
> > 
> > > I can understand the shift of the high bits, but that shift of the low
> > > bits is unlikely.  A manual would tell us if it is right.
> > > 
> > This shift is correct (according to datasheet, which contains mostly
> > useless info, but it does dociment this reg briefly.)
> 
> The KB3700 series datasheet indicates that the value from ENE_PLLFRL
> should be shifted by >> 4 bits, not by >> 2.  Of course, the KB3700
> isn't the exact same chip.
You are right about that, thanks!

Best regards,
Maxim Levitsky

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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Andy Walls]

On Fri, 2010-07-30 at 15:45 +0300, Maxim Levitsky wrote:
> On Fri, 2010-07-30 at 08:07 -0400, Jon Smirl wrote: 
> > On Fri, Jul 30, 2010 at 8:02 AM, Jon Smirl  wrote:
> > > On Fri, Jul 30, 2010 at 7:54 AM, Maxim Levitsky  
> > > wrote:

> 
> > >
> > > +   pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
> > > +   (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
> > 
> 
> 
> > I can understand the shift of the high bits, but that shift of the low
> > bits is unlikely.  A manual would tell us if it is right.
> > 
> This shift is correct (according to datasheet, which contains mostly
> useless info, but it does dociment this reg briefly.)

The KB3700 series datasheet indicates that the value from ENE_PLLFRL
should be shifted by >> 4 bits, not by >> 2.  Of course, the KB3700
isn't the exact same chip.

Regards,
Andy


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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

On Fri, 2010-07-30 at 08:07 -0400, Jon Smirl wrote: 
> On Fri, Jul 30, 2010 at 8:02 AM, Jon Smirl  wrote:
> > On Fri, Jul 30, 2010 at 7:54 AM, Maxim Levitsky  
> > wrote:
> >> On Fri, 2010-07-30 at 07:51 -0400, Jon Smirl wrote:
> >>> On Fri, Jul 30, 2010 at 7:36 AM, Maxim Levitsky  
> >>> wrote:
> >>> > On Thu, 2010-07-29 at 23:46 -0400, Andy Walls wrote:
> >>> >> On Thu, 2010-07-29 at 22:39 -0400, Jon Smirl wrote:
> >>> >> > On Thu, Jul 29, 2010 at 10:17 PM, Maxim Levitsky
> >>> >> >  wrote:
> >>> >> > > note that error_adjustment module option is added.
> >>> >> > > This allows to reduce input samples by a percent.
> >>> >> > > This makes input on my system more correct.
> >>> >> > >
> >>> >> > > Default is 4% as it works best here.
> >>> >> > >
> >>> >> > > Note that only normal input is adjusted. I don't know
> >>> >> > > what adjustments to apply to fan tachometer input.
> >>> >> > > Maybe it is accurate already.
> >>> >> >
> >>> >> > Do you have the manual for the ENE chip in English? or do you read 
> >>> >> > Chinese?
> >>> >>
> >>> >> The datasheet for a similar chip, the KB3700, is out there in English,
> >>> >> but it doesn't have CIR.
> >>> >>
> >>> >> You might find these links mildly interesting:
> >>> >>
> >>> >> http://www.coreboot.org/Embedded_controller
> >>> >> http://wiki.laptop.org/go/Embedded_controller
> >>> >> http://lists.laptop.org/pipermail/openec/2008-July/000108.html
> >>> >
> >>> > Nope, I have read that.
> >>> >>
> >>> >> Regards,
> >>> >> Andy
> >>> >>
> >>> >> > Maybe you can figure out why the readings are off by 4%. I suspect
> >>> >> > that someone has set a clock divider wrong when programming the chip.
> >>> >> > For example setting the divider for a 25Mhz clock when the clock is
> >>> >> > actually 26Mhz would cause the error you are seeing. Or they just 
> >>> >> > made
> >>> >> > a mistake in computing the divisor. It is probably a bug in the BIOS
> >>> >> > of your laptop.  If that's the case you could add a quirk in the
> >>> >> > system boot code to fix the register setting.
> >>> >
> >>> > I figured out how windows driver compensates for the offset, and do the
> >>> > same in my driver. I think the problem is solved.
> >>> >
> >>>
> >>> Should that be a <= or >= instead of !=?
> >>> +   if (pll_freq != 1000)
> >>
> >> This is how its done in windows driver.
> >
> > That doesn't mean it is bug free.

This PLL frequency is likely to be chip internal frequency.
And windows driver doesn't touch it.
Its embedded controller, so I don't want to touch things I am not sure
about.

> >
> > Experimenting with changing the PLL frequency register may correct the
> > error.  Try taking 96% of pll_freq and write it back into these
> > register. This would be easy to fix with a manual. The root problem is
> > almost certainly a bug in the way the PLLs were programmed.
> >
> > I don't like putting in fudge factors like the 4% correction. What
> > happens if a later version of the hardware has fixed firmware? I
> > normal user is never going to figure out that they need to change the
> > fudge factor.
I don't think that is a hardware bug, rather a limitation.

Lets leave it as is.
I will soon publish the driver on launchpad or something like that and
try to contact users I debugged that driver with, and then see what
ranges PLL register takes.



> >
> > +   pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
> > +   (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
> 


> I can understand the shift of the high bits, but that shift of the low
> bits is unlikely.  A manual would tell us if it is right.
> 
This shift is correct (according to datasheet, which contains mostly
useless info, but it does dociment this reg briefly.)


Best regards,
Maxim Levitsky

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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Fri, Jul 30, 2010 at 8:02 AM, Jon Smirl  wrote:
> On Fri, Jul 30, 2010 at 7:54 AM, Maxim Levitsky  
> wrote:
>> On Fri, 2010-07-30 at 07:51 -0400, Jon Smirl wrote:
>>> On Fri, Jul 30, 2010 at 7:36 AM, Maxim Levitsky  
>>> wrote:
>>> > On Thu, 2010-07-29 at 23:46 -0400, Andy Walls wrote:
>>> >> On Thu, 2010-07-29 at 22:39 -0400, Jon Smirl wrote:
>>> >> > On Thu, Jul 29, 2010 at 10:17 PM, Maxim Levitsky
>>> >> >  wrote:
>>> >> > > note that error_adjustment module option is added.
>>> >> > > This allows to reduce input samples by a percent.
>>> >> > > This makes input on my system more correct.
>>> >> > >
>>> >> > > Default is 4% as it works best here.
>>> >> > >
>>> >> > > Note that only normal input is adjusted. I don't know
>>> >> > > what adjustments to apply to fan tachometer input.
>>> >> > > Maybe it is accurate already.
>>> >> >
>>> >> > Do you have the manual for the ENE chip in English? or do you read 
>>> >> > Chinese?
>>> >>
>>> >> The datasheet for a similar chip, the KB3700, is out there in English,
>>> >> but it doesn't have CIR.
>>> >>
>>> >> You might find these links mildly interesting:
>>> >>
>>> >> http://www.coreboot.org/Embedded_controller
>>> >> http://wiki.laptop.org/go/Embedded_controller
>>> >> http://lists.laptop.org/pipermail/openec/2008-July/000108.html
>>> >
>>> > Nope, I have read that.
>>> >>
>>> >> Regards,
>>> >> Andy
>>> >>
>>> >> > Maybe you can figure out why the readings are off by 4%. I suspect
>>> >> > that someone has set a clock divider wrong when programming the chip.
>>> >> > For example setting the divider for a 25Mhz clock when the clock is
>>> >> > actually 26Mhz would cause the error you are seeing. Or they just made
>>> >> > a mistake in computing the divisor. It is probably a bug in the BIOS
>>> >> > of your laptop.  If that's the case you could add a quirk in the
>>> >> > system boot code to fix the register setting.
>>> >
>>> > I figured out how windows driver compensates for the offset, and do the
>>> > same in my driver. I think the problem is solved.
>>> >
>>>
>>> Should that be a <= or >= instead of !=?
>>> +       if (pll_freq != 1000)
>>
>> This is how its done in windows driver.
>
> That doesn't mean it is bug free.
>
> Experimenting with changing the PLL frequency register may correct the
> error.  Try taking 96% of pll_freq and write it back into these
> register. This would be easy to fix with a manual. The root problem is
> almost certainly a bug in the way the PLLs were programmed.
>
> I don't like putting in fudge factors like the 4% correction. What
> happens if a later version of the hardware has fixed firmware? I
> normal user is never going to figure out that they need to change the
> fudge factor.
>
> +       pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
> +               (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);

I can understand the shift of the high bits, but that shift of the low
bits is unlikely.  A manual would tell us if it is right.


> +
>
>
>>>
>>> Programming the PLL wrong would cause the 4% error.
>>>
>>>        hw_revision = ene_hw_read_reg(dev, ENE_HW_VERSION);
>>>        old_ver = ene_hw_read_reg(dev, ENE_HW_VER_OLD);
>>>
>>> +       pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
>>> +               (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
>>> +
>>> +       if (pll_freq != 1000)
>>> +               dev->rx_period_adjust = 4;
>>> +       else
>>> +               dev->rx_period_adjust = 2;
>>> +
>>> +
>>> +       ene_printk(KERN_NOTICE, "PLL freq = %d\n", pll_freq);
>>> +
>>>        if (hw_revision == 0xFF) {
>>>
>>>
>>>
>>> >
>>> > Best regards,
>>> > Maxim Levitsky
>>> >
>>> >
>>>
>>>
>>>
>>
>>
>>
>
>
>
> --
> Jon Smirl
> jonsm...@gmail.com
>



-- 
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Fri, Jul 30, 2010 at 7:54 AM, Maxim Levitsky  wrote:
> On Fri, 2010-07-30 at 07:51 -0400, Jon Smirl wrote:
>> On Fri, Jul 30, 2010 at 7:36 AM, Maxim Levitsky  
>> wrote:
>> > On Thu, 2010-07-29 at 23:46 -0400, Andy Walls wrote:
>> >> On Thu, 2010-07-29 at 22:39 -0400, Jon Smirl wrote:
>> >> > On Thu, Jul 29, 2010 at 10:17 PM, Maxim Levitsky
>> >> >  wrote:
>> >> > > note that error_adjustment module option is added.
>> >> > > This allows to reduce input samples by a percent.
>> >> > > This makes input on my system more correct.
>> >> > >
>> >> > > Default is 4% as it works best here.
>> >> > >
>> >> > > Note that only normal input is adjusted. I don't know
>> >> > > what adjustments to apply to fan tachometer input.
>> >> > > Maybe it is accurate already.
>> >> >
>> >> > Do you have the manual for the ENE chip in English? or do you read 
>> >> > Chinese?
>> >>
>> >> The datasheet for a similar chip, the KB3700, is out there in English,
>> >> but it doesn't have CIR.
>> >>
>> >> You might find these links mildly interesting:
>> >>
>> >> http://www.coreboot.org/Embedded_controller
>> >> http://wiki.laptop.org/go/Embedded_controller
>> >> http://lists.laptop.org/pipermail/openec/2008-July/000108.html
>> >
>> > Nope, I have read that.
>> >>
>> >> Regards,
>> >> Andy
>> >>
>> >> > Maybe you can figure out why the readings are off by 4%. I suspect
>> >> > that someone has set a clock divider wrong when programming the chip.
>> >> > For example setting the divider for a 25Mhz clock when the clock is
>> >> > actually 26Mhz would cause the error you are seeing. Or they just made
>> >> > a mistake in computing the divisor. It is probably a bug in the BIOS
>> >> > of your laptop.  If that's the case you could add a quirk in the
>> >> > system boot code to fix the register setting.
>> >
>> > I figured out how windows driver compensates for the offset, and do the
>> > same in my driver. I think the problem is solved.
>> >
>>
>> Should that be a <= or >= instead of !=?
>> +       if (pll_freq != 1000)
>
> This is how its done in windows driver.

That doesn't mean it is bug free.

Experimenting with changing the PLL frequency register may correct the
error.  Try taking 96% of pll_freq and write it back into these
register. This would be easy to fix with a manual. The root problem is
almost certainly a bug in the way the PLLs were programmed.

I don't like putting in fudge factors like the 4% correction. What
happens if a later version of the hardware has fixed firmware? I
normal user is never going to figure out that they need to change the
fudge factor.

+   pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
+   (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
+


>>
>> Programming the PLL wrong would cause the 4% error.
>>
>>        hw_revision = ene_hw_read_reg(dev, ENE_HW_VERSION);
>>        old_ver = ene_hw_read_reg(dev, ENE_HW_VER_OLD);
>>
>> +       pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
>> +               (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
>> +
>> +       if (pll_freq != 1000)
>> +               dev->rx_period_adjust = 4;
>> +       else
>> +               dev->rx_period_adjust = 2;
>> +
>> +
>> +       ene_printk(KERN_NOTICE, "PLL freq = %d\n", pll_freq);
>> +
>>        if (hw_revision == 0xFF) {
>>
>>
>>
>> >
>> > Best regards,
>> > Maxim Levitsky
>> >
>> >
>>
>>
>>
>
>
>



-- 
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

On Fri, 2010-07-30 at 07:51 -0400, Jon Smirl wrote: 
> On Fri, Jul 30, 2010 at 7:36 AM, Maxim Levitsky  
> wrote:
> > On Thu, 2010-07-29 at 23:46 -0400, Andy Walls wrote:
> >> On Thu, 2010-07-29 at 22:39 -0400, Jon Smirl wrote:
> >> > On Thu, Jul 29, 2010 at 10:17 PM, Maxim Levitsky
> >> >  wrote:
> >> > > note that error_adjustment module option is added.
> >> > > This allows to reduce input samples by a percent.
> >> > > This makes input on my system more correct.
> >> > >
> >> > > Default is 4% as it works best here.
> >> > >
> >> > > Note that only normal input is adjusted. I don't know
> >> > > what adjustments to apply to fan tachometer input.
> >> > > Maybe it is accurate already.
> >> >
> >> > Do you have the manual for the ENE chip in English? or do you read 
> >> > Chinese?
> >>
> >> The datasheet for a similar chip, the KB3700, is out there in English,
> >> but it doesn't have CIR.
> >>
> >> You might find these links mildly interesting:
> >>
> >> http://www.coreboot.org/Embedded_controller
> >> http://wiki.laptop.org/go/Embedded_controller
> >> http://lists.laptop.org/pipermail/openec/2008-July/000108.html
> >
> > Nope, I have read that.
> >>
> >> Regards,
> >> Andy
> >>
> >> > Maybe you can figure out why the readings are off by 4%. I suspect
> >> > that someone has set a clock divider wrong when programming the chip.
> >> > For example setting the divider for a 25Mhz clock when the clock is
> >> > actually 26Mhz would cause the error you are seeing. Or they just made
> >> > a mistake in computing the divisor. It is probably a bug in the BIOS
> >> > of your laptop.  If that's the case you could add a quirk in the
> >> > system boot code to fix the register setting.
> >
> > I figured out how windows driver compensates for the offset, and do the
> > same in my driver. I think the problem is solved.
> >
> 
> Should that be a <= or >= instead of !=?
> +   if (pll_freq != 1000)

This is how its done in windows driver. 
> 
> Programming the PLL wrong would cause the 4% error.
> 
>hw_revision = ene_hw_read_reg(dev, ENE_HW_VERSION);
>old_ver = ene_hw_read_reg(dev, ENE_HW_VER_OLD);
> 
> +   pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
> +   (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
> +
> +   if (pll_freq != 1000)
> +   dev->rx_period_adjust = 4;
> +   else
> +   dev->rx_period_adjust = 2;
> +
> +
> +   ene_printk(KERN_NOTICE, "PLL freq = %d\n", pll_freq);
> +
>if (hw_revision == 0xFF) {
> 
> 
> 
> >
> > Best regards,
> > Maxim Levitsky
> >
> >
> 
> 
> 


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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Fri, Jul 30, 2010 at 7:36 AM, Maxim Levitsky  wrote:
> On Thu, 2010-07-29 at 23:46 -0400, Andy Walls wrote:
>> On Thu, 2010-07-29 at 22:39 -0400, Jon Smirl wrote:
>> > On Thu, Jul 29, 2010 at 10:17 PM, Maxim Levitsky
>> >  wrote:
>> > > note that error_adjustment module option is added.
>> > > This allows to reduce input samples by a percent.
>> > > This makes input on my system more correct.
>> > >
>> > > Default is 4% as it works best here.
>> > >
>> > > Note that only normal input is adjusted. I don't know
>> > > what adjustments to apply to fan tachometer input.
>> > > Maybe it is accurate already.
>> >
>> > Do you have the manual for the ENE chip in English? or do you read Chinese?
>>
>> The datasheet for a similar chip, the KB3700, is out there in English,
>> but it doesn't have CIR.
>>
>> You might find these links mildly interesting:
>>
>> http://www.coreboot.org/Embedded_controller
>> http://wiki.laptop.org/go/Embedded_controller
>> http://lists.laptop.org/pipermail/openec/2008-July/000108.html
>
> Nope, I have read that.
>>
>> Regards,
>> Andy
>>
>> > Maybe you can figure out why the readings are off by 4%. I suspect
>> > that someone has set a clock divider wrong when programming the chip.
>> > For example setting the divider for a 25Mhz clock when the clock is
>> > actually 26Mhz would cause the error you are seeing. Or they just made
>> > a mistake in computing the divisor. It is probably a bug in the BIOS
>> > of your laptop.  If that's the case you could add a quirk in the
>> > system boot code to fix the register setting.
>
> I figured out how windows driver compensates for the offset, and do the
> same in my driver. I think the problem is solved.
>

Should that be a <= or >= instead of !=?
+   if (pll_freq != 1000)

Programming the PLL wrong would cause the 4% error.

   hw_revision = ene_hw_read_reg(dev, ENE_HW_VERSION);
   old_ver = ene_hw_read_reg(dev, ENE_HW_VER_OLD);

+   pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
+   (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
+
+   if (pll_freq != 1000)
+   dev->rx_period_adjust = 4;
+   else
+   dev->rx_period_adjust = 2;
+
+
+   ene_printk(KERN_NOTICE, "PLL freq = %d\n", pll_freq);
+
   if (hw_revision == 0xFF) {



>
> Best regards,
> Maxim Levitsky
>
>



-- 
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 MAINTAINERS   |6 +
 drivers/media/IR/Kconfig  |   14 +
 drivers/media/IR/Makefile |1 +
 drivers/media/IR/ene_ir.c |  595 +
 drivers/media/IR/ene_ir.h |   51 ++---
 5 files changed, 265 insertions(+), 402 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 56a36d7..587785a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2188,6 +2188,12 @@ F:   drivers/misc/cb710/
 F: drivers/mmc/host/cb710-mmc.*
 F: include/linux/cb710.h
 
+ENE KB2426 (ENE0100/ENE020XX) INFRARED RECEIVER
+M: Maxim Levitsky 
+S: Maintained
+F: drivers/media/IR/ene_ir.c
+F: drivers/media/IR/ene_ir.h
+
 EPSON 1355 FRAMEBUFFER DRIVER
 M: Christopher Hoover 
 M: Christopher Hoover 
diff --git a/drivers/media/IR/Kconfig b/drivers/media/IR/Kconfig
index fc48a3f..3f62bf9 100644
--- a/drivers/media/IR/Kconfig
+++ b/drivers/media/IR/Kconfig
@@ -105,4 +105,18 @@ config IR_MCEUSB
   To compile this driver as a module, choose M here: the
   module will be called mceusb.
 
+config IR_ENE
+   tristate "ENE eHome Receiver/Transciever (pnp id: ENE0100/ENE02xxx)"
+   depends on PNP
+   depends on IR_CORE
+   ---help---
+  Say Y here to enable support for integrated infrared receiver
+  /transciever made by ENE.
+
+  You can see if you have it by looking at lspnp output.
+  Output should include ENE0100 ENE0200 or something similiar.
+
+  To compile this driver as a module, choose M here: the
+  module will be called ene_ir.
+
 endif #IR_CORE
diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
index 2ae4f3a..3262a68 100644
--- a/drivers/media/IR/Makefile
+++ b/drivers/media/IR/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_IR_LIRC_CODEC) += ir-lirc-codec.o
 # stand-alone IR receivers/transmitters
 obj-$(CONFIG_IR_IMON) += imon.o
 obj-$(CONFIG_IR_MCEUSB) += mceusb.o
+obj-$(CONFIG_IR_ENE) += ene_ir.o
diff --git a/drivers/media/IR/ene_ir.c b/drivers/media/IR/ene_ir.c
index 9d11caf..de1e5c4 100644
--- a/drivers/media/IR/ene_ir.c
+++ b/drivers/media/IR/ene_ir.c
@@ -1,5 +1,5 @@
 /*
- * driver for ENE KB3926 B/C/D CIR (also known as ENE0100/ENE0200/ENE0201)
+ * driver for ENE KB3926 B/C/D CIR (pnp id: ENE0XXX)
  *
  * Copyright (C) 2010 Maxim Levitsky 
  *
@@ -25,20 +25,20 @@
 #include 
 #include 
 #include 
-#include 
-#include "lirc_ene0100.h"
+#include 
+#include 
+#include 
+#include 
+#include "ene_ir.h"
 
 
 static int sample_period = -1;
 static int enable_idle = 1;
-static int enable_duty_carrier;
 static int input = 1;
 static int debug;
 static int txsim;
 
-static void ene_rx_set_idle(struct ene_device *dev, int idle);
 static int ene_irq_status(struct ene_device *dev);
-static void ene_send_sample(struct ene_device *dev, unsigned long sample);
 
 /* read a hardware register */
 static u8 ene_hw_read_reg(struct ene_device *dev, u16 reg)
@@ -85,6 +85,7 @@ static int ene_hw_detect(struct ene_device *dev)
u8 hw_revision, old_ver;
u8 tmp;
u8 fw_capabilities;
+   int pll_freq;
 
tmp = ene_hw_read_reg(dev, ENE_HW_UNK);
ene_hw_write_reg(dev, ENE_HW_UNK, tmp & ~ENE_HW_UNK_CLR);
@@ -96,6 +97,17 @@ static int ene_hw_detect(struct ene_device *dev)
hw_revision = ene_hw_read_reg(dev, ENE_HW_VERSION);
old_ver = ene_hw_read_reg(dev, ENE_HW_VER_OLD);
 
+   pll_freq = (ene_hw_read_reg(dev, ENE_PLLFRH) << 4) +
+   (ene_hw_read_reg(dev, ENE_PLLFRL) >> 2);
+
+   if (pll_freq != 1000)
+   dev->rx_period_adjust = 4;
+   else
+   dev->rx_period_adjust = 2;
+
+
+   ene_printk(KERN_NOTICE, "PLL freq = %d\n", pll_freq);
+
if (hw_revision == 0xFF) {
 
ene_printk(KERN_WARNING, "device seems to be disabled\n");
@@ -160,7 +172,7 @@ static int ene_hw_detect(struct ene_device *dev)
 }
 
 /* this enables/disables IR input via gpio40*/
-static void ene_enable_gpio40_recieve(struct ene_device *dev, int enable)
+static void ene_enable_gpio40_receive(struct ene_device *dev, int enable)
 {
ene_hw_write_reg_mask(dev, ENE_CIR_CONF2, enable ?
  0 : ENE_CIR_CONF2_GPIO40DIS,
@@ -168,13 +180,13 @@ static void ene_enable_gpio40_recieve(struct ene_device 
*dev, int enable)
 }
 
 /* this enables/disables IR via standard input */
-static void ene_enable_normal_recieve(struct ene_device *dev, int enable)
+static void ene_enable_normal_receive(struct ene_device *dev, int enable)
 {
ene_hw_write_reg(dev, ENE_CIR_CONF1, enable ? ENE_CIR_CONF1_RX_ON : 0);
 }
 
 /* this enables/disables IR input via unused fan tachtometer input */
-static void ene_enable_fan_recieve(struct ene_device *dev, int enable)
+static void ene_enable_fan_receive(struct ene_device *dev, int enable)
 {
if (!enable)
ene_hw_write_reg(dev, ENE_FAN_AS_IN1, 0);
@@ -186,7 +198,7 @@ static void ene_enable_fan_recieve

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

On Thu, 2010-07-29 at 23:46 -0400, Andy Walls wrote: 
> On Thu, 2010-07-29 at 22:39 -0400, Jon Smirl wrote:
> > On Thu, Jul 29, 2010 at 10:17 PM, Maxim Levitsky
> >  wrote:
> > > note that error_adjustment module option is added.
> > > This allows to reduce input samples by a percent.
> > > This makes input on my system more correct.
> > >
> > > Default is 4% as it works best here.
> > >
> > > Note that only normal input is adjusted. I don't know
> > > what adjustments to apply to fan tachometer input.
> > > Maybe it is accurate already.
> > 
> > Do you have the manual for the ENE chip in English? or do you read Chinese?
> 
> The datasheet for a similar chip, the KB3700, is out there in English,
> but it doesn't have CIR.
> 
> You might find these links mildly interesting:
> 
> http://www.coreboot.org/Embedded_controller
> http://wiki.laptop.org/go/Embedded_controller
> http://lists.laptop.org/pipermail/openec/2008-July/000108.html

Nope, I have read that. 
> 
> Regards,
> Andy
> 
> > Maybe you can figure out why the readings are off by 4%. I suspect
> > that someone has set a clock divider wrong when programming the chip.
> > For example setting the divider for a 25Mhz clock when the clock is
> > actually 26Mhz would cause the error you are seeing. Or they just made
> > a mistake in computing the divisor. It is probably a bug in the BIOS
> > of your laptop.  If that's the case you could add a quirk in the
> > system boot code to fix the register setting.

I figured out how windows driver compensates for the offset, and do the
same in my driver. I think the problem is solved.


Best regards,
Maxim Levitsky

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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Andy Walls]

On Thu, 2010-07-29 at 22:39 -0400, Jon Smirl wrote:
> On Thu, Jul 29, 2010 at 10:17 PM, Maxim Levitsky
>  wrote:
> > note that error_adjustment module option is added.
> > This allows to reduce input samples by a percent.
> > This makes input on my system more correct.
> >
> > Default is 4% as it works best here.
> >
> > Note that only normal input is adjusted. I don't know
> > what adjustments to apply to fan tachometer input.
> > Maybe it is accurate already.
> 
> Do you have the manual for the ENE chip in English? or do you read Chinese?

The datasheet for a similar chip, the KB3700, is out there in English,
but it doesn't have CIR.

You might find these links mildly interesting:

http://www.coreboot.org/Embedded_controller
http://wiki.laptop.org/go/Embedded_controller
http://lists.laptop.org/pipermail/openec/2008-July/000108.html

Regards,
Andy

> Maybe you can figure out why the readings are off by 4%. I suspect
> that someone has set a clock divider wrong when programming the chip.
> For example setting the divider for a 25Mhz clock when the clock is
> actually 26Mhz would cause the error you are seeing. Or they just made
> a mistake in computing the divisor. It is probably a bug in the BIOS
> of your laptop.  If that's the case you could add a quirk in the
> system boot code to fix the register setting.
> 


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

Re: [PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Jon Smirl]

On Thu, Jul 29, 2010 at 10:17 PM, Maxim Levitsky
 wrote:
> note that error_adjustment module option is added.
> This allows to reduce input samples by a percent.
> This makes input on my system more correct.
>
> Default is 4% as it works best here.
>
> Note that only normal input is adjusted. I don't know
> what adjustments to apply to fan tachometer input.
> Maybe it is accurate already.

Do you have the manual for the ENE chip in English? or do you read Chinese?

Maybe you can figure out why the readings are off by 4%. I suspect
that someone has set a clock divider wrong when programming the chip.
For example setting the divider for a 25Mhz clock when the clock is
actually 26Mhz would cause the error you are seeing. Or they just made
a mistake in computing the divisor. It is probably a bug in the BIOS
of your laptop.  If that's the case you could add a quirk in the
system boot code to fix the register setting.

-- 
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/13] IR: Port ene driver to new IR subsystem and enable it.

[Maxim Levitsky]

note that error_adjustment module option is added.
This allows to reduce input samples by a percent.
This makes input on my system more correct.

Default is 4% as it works best here.

Note that only normal input is adjusted. I don't know
what ajustements to apply to fan tachtometer input.
Maybe it is accurate already.

Signed-off-by: Maxim Levitsky 
---
 MAINTAINERS   |6 +
 drivers/media/IR/Kconfig  |   14 +
 drivers/media/IR/Makefile |1 +
 drivers/media/IR/ene_ir.c |  591 +
 drivers/media/IR/ene_ir.h |   47 ++---
 5 files changed, 258 insertions(+), 401 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 56a36d7..587785a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2188,6 +2188,12 @@ F:   drivers/misc/cb710/
 F: drivers/mmc/host/cb710-mmc.*
 F: include/linux/cb710.h
 
+ENE KB2426 (ENE0100/ENE020XX) INFRARED RECEIVER
+M: Maxim Levitsky 
+S: Maintained
+F: drivers/media/IR/ene_ir.c
+F: drivers/media/IR/ene_ir.h
+
 EPSON 1355 FRAMEBUFFER DRIVER
 M: Christopher Hoover 
 M: Christopher Hoover 
diff --git a/drivers/media/IR/Kconfig b/drivers/media/IR/Kconfig
index fc48a3f..3f62bf9 100644
--- a/drivers/media/IR/Kconfig
+++ b/drivers/media/IR/Kconfig
@@ -105,4 +105,18 @@ config IR_MCEUSB
   To compile this driver as a module, choose M here: the
   module will be called mceusb.
 
+config IR_ENE
+   tristate "ENE eHome Receiver/Transciever (pnp id: ENE0100/ENE02xxx)"
+   depends on PNP
+   depends on IR_CORE
+   ---help---
+  Say Y here to enable support for integrated infrared receiver
+  /transciever made by ENE.
+
+  You can see if you have it by looking at lspnp output.
+  Output should include ENE0100 ENE0200 or something similiar.
+
+  To compile this driver as a module, choose M here: the
+  module will be called ene_ir.
+
 endif #IR_CORE
diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
index 2ae4f3a..3262a68 100644
--- a/drivers/media/IR/Makefile
+++ b/drivers/media/IR/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_IR_LIRC_CODEC) += ir-lirc-codec.o
 # stand-alone IR receivers/transmitters
 obj-$(CONFIG_IR_IMON) += imon.o
 obj-$(CONFIG_IR_MCEUSB) += mceusb.o
+obj-$(CONFIG_IR_ENE) += ene_ir.o
diff --git a/drivers/media/IR/ene_ir.c b/drivers/media/IR/ene_ir.c
index 9d11caf..297cfed 100644
--- a/drivers/media/IR/ene_ir.c
+++ b/drivers/media/IR/ene_ir.c
@@ -1,5 +1,5 @@
 /*
- * driver for ENE KB3926 B/C/D CIR (also known as ENE0100/ENE0200/ENE0201)
+ * driver for ENE KB3926 B/C/D CIR (pnp id: ENE0XXX)
  *
  * Copyright (C) 2010 Maxim Levitsky 
  *
@@ -25,20 +25,21 @@
 #include 
 #include 
 #include 
-#include 
-#include "lirc_ene0100.h"
+#include 
+#include 
+#include 
+#include 
+#include "ene_ir.h"
 
 
 static int sample_period = -1;
 static int enable_idle = 1;
-static int enable_duty_carrier;
+static int error_adjustment = 4;
 static int input = 1;
 static int debug;
 static int txsim;
 
-static void ene_rx_set_idle(struct ene_device *dev, int idle);
 static int ene_irq_status(struct ene_device *dev);
-static void ene_send_sample(struct ene_device *dev, unsigned long sample);
 
 /* read a hardware register */
 static u8 ene_hw_read_reg(struct ene_device *dev, u16 reg)
@@ -160,7 +161,7 @@ static int ene_hw_detect(struct ene_device *dev)
 }
 
 /* this enables/disables IR input via gpio40*/
-static void ene_enable_gpio40_recieve(struct ene_device *dev, int enable)
+static void ene_enable_gpio40_receive(struct ene_device *dev, int enable)
 {
ene_hw_write_reg_mask(dev, ENE_CIR_CONF2, enable ?
  0 : ENE_CIR_CONF2_GPIO40DIS,
@@ -168,13 +169,13 @@ static void ene_enable_gpio40_recieve(struct ene_device 
*dev, int enable)
 }
 
 /* this enables/disables IR via standard input */
-static void ene_enable_normal_recieve(struct ene_device *dev, int enable)
+static void ene_enable_normal_receive(struct ene_device *dev, int enable)
 {
ene_hw_write_reg(dev, ENE_CIR_CONF1, enable ? ENE_CIR_CONF1_RX_ON : 0);
 }
 
 /* this enables/disables IR input via unused fan tachtometer input */
-static void ene_enable_fan_recieve(struct ene_device *dev, int enable)
+static void ene_enable_fan_receive(struct ene_device *dev, int enable)
 {
if (!enable)
ene_hw_write_reg(dev, ENE_FAN_AS_IN1, 0);
@@ -186,7 +187,7 @@ static void ene_enable_fan_recieve(struct ene_device *dev, 
int enable)
 }
 
 
-/* Sense current recieved carrier */
+/* Sense current received carrier */
 static int ene_rx_sense_carrier(struct ene_device *dev)
 {
int period = ene_hw_read_reg(dev, ENE_RX_CARRIER);
@@ -209,37 +210,37 @@ static int ene_rx_sense_carrier(struct ene_device *dev)
 /* determine which input to use*/
 static void ene_rx_set_inputs(struct ene_device *dev)
 {
-   int learning_mode = dev->learning_enabled || dev->rx_carrier_sense;
+   int learning_mode = dev->learning_enabled;
 
-