Re: [PATCH 1/3] IR: add core lirc device interface
On Fri, Jun 4, 2010 at 2:38 PM, Mauro Carvalho Chehab wrote: ... > From my side, as I said before, I'd like to see a documentation of the > defined API bits, > and the removal of the currently unused ioctls (they can be added later, > together > with the patches that will introduce the code that handles them) to give my > final ack. Thanks to Christoph and me spending an exciting Friday night doing docbook formatting for the first time ever, I've finally got a start of a LIRC device interface API docbook doc together, along with all the lirc_dev bits and ir-lirc-codec bridge, all me-tested-and-approved w/four different mceusb devices, both send and receive. I'm about to head out of town for the weekend, but hope to get patches in flight either before then, or from the road. In the interim, the interested can take a look here: http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches Nb: this branch has been non-fast-forward-ably updated against latest linuxtv staging/rc. The prior variant there left too much lirc-specific tx bits in mceusb, which have now all been moved into ir-lirc-codec. -- Jarod Wilson ja...@wilsonet.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 1/3] IR: add core lirc device interface
On Mon, Jun 07, 2010 at 03:45:38PM -0400, Jarod Wilson wrote: > On Mon, Jun 7, 2010 at 2:44 PM, David Härdeman wrote: > > On Fri, Jun 04, 2010 at 03:38:35PM -0300, Mauro Carvalho Chehab wrote: > >> Em 04-06-2010 12:51, Christoph Bartelmus escreveu: > >> > Hi Mauro, > >> > > >> > on 04 Jun 10 at 01:10, Mauro Carvalho Chehab wrote: > >> >> Em 03-06-2010 19:06, Jarod Wilson escreveu: > >> > [...] > >> >>> As for the compat bits... I actually pulled them out of the Fedora > >> >>> kernel > >> >>> and userspace for a while, and there were only a few people who really > >> >>> ran > >> >>> into issues with it, but I think if the new userspace and kernel are > >> >>> rolled > >> >>> out at the same time in a new distro release (i.e., Fedora 14, in our > >> >>> particular case), it should be mostly transparent to users. > >> > > >> >> For sure this will happen on all distros that follows upstream: they'll > >> >> update lirc to fulfill the minimal requirement at Documentation/Changes. > >> >> > >> >> The issue will appear only to people that manually compile kernel and > >> >> lirc. > >> >> Those users are likely smart enough to upgrade to a newer lirc version > >> >> if > >> >> they notice a trouble, and to check at the forums. > >> > > >> >>> Christoph > >> >>> wasn't a fan of the change, and actually asked me to revert it, so I'm > >> >>> cc'ing him here for further feedback, but I'm inclined to say that if > >> >>> this > >> >>> is the price we pay to get upstream, so be it. > >> > > >> >> I understand Christoph view, but I think that having to deal with compat > >> >> stuff forever is a high price to pay, as the impact of this change is > >> >> transitory and shouldn't be hard to deal with. > >> > > >> > I'm not against doing this change, but it has to be coordinated between > >> > drivers and user-space. > >> > Just changing lirc.h is not enough. You also have to change all > >> > user-space > >> > applications that use the affected ioctls to use the correct types. > >> > That's what Jarod did not address last time so I asked him to revert the > >> > change. > >> > >> For sure coordination between kernel and userspace is very important. I'm > >> sure > >> that Jarod can help with this sync. Also, after having the changes > >> implemented > >> on userspace, I expect one patch from you adding the minimal lirc > >> requirement > >> at Documentation/Changes. > >> > >> > And I'd also like to collect all other change request to the API > >> > if there are any and do all changes in one go. > >> > >> You and Jarod are the most indicated people to point for such needs. Also, > >> Jon > >> and David may have some comments. > > > > David (who has been absent, sorry about that, life got in the way) > > thinks that the lirc raw decoder should implement the minimum amount of > > ioctl's possible while still being usable by the lirc userspace and > > without going beyond being a raw pulse/space "decoder" or we'll risk > > having to extend the entire decoder API just to support the lirc > > compatability decoder. > > Thus far, I can get 100% feature-parity w/lirc_mceusb in my ir-core > mceusb driver by adding only 3 tx-specific callbacks to ir_dev_props, > and they're all callbacks (set output mask, set carrier and ir tx > function) that any rc-core native tx solution is also going to need. Yes, but I hope...think...that we're going to have one set-RX-parameters and one set-TX-parameters callback when we're done. Setting parameters could be a costly operation so all settings should be passed to the driver in one go (whether they come from an ioctl or some fancy sysfs-with-transactions scheme). On the other hand, changing the callbacks you're proposing once we actually have support in rc-core shouldn't be too much work, so I won't oppose those changes. -- David Härdeman -- 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 1/3] IR: add core lirc device interface
On Mon, Jun 7, 2010 at 2:44 PM, David Härdeman wrote:
> On Fri, Jun 04, 2010 at 03:38:35PM -0300, Mauro Carvalho Chehab wrote:
>> Em 04-06-2010 12:51, Christoph Bartelmus escreveu:
>> > Hi Mauro,
>> >
>> > on 04 Jun 10 at 01:10, Mauro Carvalho Chehab wrote:
>> >> Em 03-06-2010 19:06, Jarod Wilson escreveu:
>> > [...]
>> >>> As for the compat bits... I actually pulled them out of the Fedora kernel
>> >>> and userspace for a while, and there were only a few people who really
>> >>> ran
>> >>> into issues with it, but I think if the new userspace and kernel are
>> >>> rolled
>> >>> out at the same time in a new distro release (i.e., Fedora 14, in our
>> >>> particular case), it should be mostly transparent to users.
>> >
>> >> For sure this will happen on all distros that follows upstream: they'll
>> >> update lirc to fulfill the minimal requirement at Documentation/Changes.
>> >>
>> >> The issue will appear only to people that manually compile kernel and
>> >> lirc.
>> >> Those users are likely smart enough to upgrade to a newer lirc version if
>> >> they notice a trouble, and to check at the forums.
>> >
>> >>> Christoph
>> >>> wasn't a fan of the change, and actually asked me to revert it, so I'm
>> >>> cc'ing him here for further feedback, but I'm inclined to say that if
>> >>> this
>> >>> is the price we pay to get upstream, so be it.
>> >
>> >> I understand Christoph view, but I think that having to deal with compat
>> >> stuff forever is a high price to pay, as the impact of this change is
>> >> transitory and shouldn't be hard to deal with.
>> >
>> > I'm not against doing this change, but it has to be coordinated between
>> > drivers and user-space.
>> > Just changing lirc.h is not enough. You also have to change all user-space
>> > applications that use the affected ioctls to use the correct types.
>> > That's what Jarod did not address last time so I asked him to revert the
>> > change.
>>
>> For sure coordination between kernel and userspace is very important. I'm
>> sure
>> that Jarod can help with this sync. Also, after having the changes
>> implemented
>> on userspace, I expect one patch from you adding the minimal lirc requirement
>> at Documentation/Changes.
>>
>> > And I'd also like to collect all other change request to the API
>> > if there are any and do all changes in one go.
>>
>> You and Jarod are the most indicated people to point for such needs. Also,
>> Jon
>> and David may have some comments.
>
> David (who has been absent, sorry about that, life got in the way)
> thinks that the lirc raw decoder should implement the minimum amount of
> ioctl's possible while still being usable by the lirc userspace and
> without going beyond being a raw pulse/space "decoder" or we'll risk
> having to extend the entire decoder API just to support the lirc
> compatability decoder.
Thus far, I can get 100% feature-parity w/lirc_mceusb in my ir-core
mceusb driver by adding only 3 tx-specific callbacks to ir_dev_props,
and they're all callbacks (set output mask, set carrier and ir tx
function) that any rc-core native tx solution is also going to need.
On the receive side, zero modifications were made to enable the bridge
driver, outside of the minimal bits to load and register the
"decoder". I definitely don't want to burden {ir,rc}-core with
anything that is lirc-specific. Andy's cx23888 driver seems to have a
whole lot more functionality that it might be desirable to adjust via
userspace than mceusb does, but I think that can still be done w/o any
hooks in the core that are inherently lirc-specific.
For reference, this is the entire diff for what was added to ir-core
to enable tx on the mceusb driver:
http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=commitdiff;h=73d631214ed75003bb73e3303819748b47303fd6
Most of the heavy lifting was done in ir-lirc-codec and mceusb, and
the mceusb parts are all non-lirc-specific (hopefully), and thus ready
to be utilized by a "native" tx solution as well.
> Over time, I hope that rc-core will grow it's own chardev with a clean
> set of ioctls (or sysfs controls, the jury hasn't returned a verdict
> yet). lircd can then be upgraded to support both the in-kernel native
> mode and the legacy lirc mode, and with time, the lirc raw decoder can
> be phased out.
Works for me.
--
Jarod Wilson
ja...@wilsonet.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 1/3] IR: add core lirc device interface
On Fri, Jun 04, 2010 at 03:38:35PM -0300, Mauro Carvalho Chehab wrote: > Em 04-06-2010 12:51, Christoph Bartelmus escreveu: > > Hi Mauro, > > > > on 04 Jun 10 at 01:10, Mauro Carvalho Chehab wrote: > >> Em 03-06-2010 19:06, Jarod Wilson escreveu: > > [...] > >>> As for the compat bits... I actually pulled them out of the Fedora kernel > >>> and userspace for a while, and there were only a few people who really ran > >>> into issues with it, but I think if the new userspace and kernel are > >>> rolled > >>> out at the same time in a new distro release (i.e., Fedora 14, in our > >>> particular case), it should be mostly transparent to users. > > > >> For sure this will happen on all distros that follows upstream: they'll > >> update lirc to fulfill the minimal requirement at Documentation/Changes. > >> > >> The issue will appear only to people that manually compile kernel and lirc. > >> Those users are likely smart enough to upgrade to a newer lirc version if > >> they notice a trouble, and to check at the forums. > > > >>> Christoph > >>> wasn't a fan of the change, and actually asked me to revert it, so I'm > >>> cc'ing him here for further feedback, but I'm inclined to say that if this > >>> is the price we pay to get upstream, so be it. > > > >> I understand Christoph view, but I think that having to deal with compat > >> stuff forever is a high price to pay, as the impact of this change is > >> transitory and shouldn't be hard to deal with. > > > > I'm not against doing this change, but it has to be coordinated between > > drivers and user-space. > > Just changing lirc.h is not enough. You also have to change all user-space > > applications that use the affected ioctls to use the correct types. > > That's what Jarod did not address last time so I asked him to revert the > > change. > > For sure coordination between kernel and userspace is very important. I'm sure > that Jarod can help with this sync. Also, after having the changes implemented > on userspace, I expect one patch from you adding the minimal lirc requirement > at Documentation/Changes. > > > And I'd also like to collect all other change request to the API > > if there are any and do all changes in one go. > > You and Jarod are the most indicated people to point for such needs. Also, Jon > and David may have some comments. David (who has been absent, sorry about that, life got in the way) thinks that the lirc raw decoder should implement the minimum amount of ioctl's possible while still being usable by the lirc userspace and without going beyond being a raw pulse/space "decoder" or we'll risk having to extend the entire decoder API just to support the lirc compatability decoder. Over time, I hope that rc-core will grow it's own chardev with a clean set of ioctls (or sysfs controls, the jury hasn't returned a verdict yet). lircd can then be upgraded to support both the in-kernel native mode and the legacy lirc mode, and with time, the lirc raw decoder can be phased out. -- David Härdeman -- 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 1/3] IR: add core lirc device interface
On Sat, Jun 5, 2010 at 1:24 PM, Andy Walls wrote:
> On Fri, 2010-06-04 at 14:57 -0400, Jon Smirl wrote:
>> On Fri, Jun 4, 2010 at 2:38 PM, Mauro Carvalho Chehab
>> wrote:
>> > Em 04-06-2010 12:51, Christoph Bartelmus escreveu:
>> >> Hi Mauro,
>> >>
>> >> on 04 Jun 10 at 01:10, Mauro Carvalho Chehab wrote:
>> >>> Em 03-06-2010 19:06, Jarod Wilson escreveu:
>> >> [...]
>> As for the compat bits... I actually pulled them out of the Fedora
>> kernel
>> and userspace for a while, and there were only a few people who really
>> ran
>> into issues with it, but I think if the new userspace and kernel are
>> rolled
>> out at the same time in a new distro release (i.e., Fedora 14, in our
>> particular case), it should be mostly transparent to users.
>> >>
>> >>> For sure this will happen on all distros that follows upstream: they'll
>> >>> update lirc to fulfill the minimal requirement at Documentation/Changes.
>> >>>
>> >>> The issue will appear only to people that manually compile kernel and
>> >>> lirc.
>> >>> Those users are likely smart enough to upgrade to a newer lirc version if
>> >>> they notice a trouble, and to check at the forums.
>> >>
>> Christoph
>> wasn't a fan of the change, and actually asked me to revert it, so I'm
>> cc'ing him here for further feedback, but I'm inclined to say that if
>> this
>> is the price we pay to get upstream, so be it.
>> >>
>> >>> I understand Christoph view, but I think that having to deal with compat
>> >>> stuff forever is a high price to pay, as the impact of this change is
>> >>> transitory and shouldn't be hard to deal with.
>> >>
>> >> I'm not against doing this change, but it has to be coordinated between
>> >> drivers and user-space.
>> >> Just changing lirc.h is not enough. You also have to change all user-space
>> >> applications that use the affected ioctls to use the correct types.
>> >> That's what Jarod did not address last time so I asked him to revert the
>> >> change.
>> >
>> > For sure coordination between kernel and userspace is very important. I'm
>> > sure
>> > that Jarod can help with this sync. Also, after having the changes
>> > implemented
>> > on userspace, I expect one patch from you adding the minimal lirc
>> > requirement
>> > at Documentation/Changes.
>>
>> Keep the get_version() ioctl stable. The first thing user space would
>> do it call the get_version() ioctl. If user space doesn't support the
>> kernel version error out and print a message with the url of the
>> project web site and tell the user to upgrade.
>>
>> If you have the ability to upgrade your kernel you should also have
>> the ability to upgrade user space too. I'm no fan of keeping backwards
>> compatibility code around. It just becomes piles of clutter that
>> nobody maintains. A reliable mechanism to determine version mismatch
>> is all that is needed.
>>
>> PS - I really don't like having to fix bug reports in compatibility
>> code. That is an incredible waste of support time that can be easily
>> fixed by upgrading the user.
>>
>> >
>> >> And I'd also like to collect all other change request to the API
>> >> if there are any and do all changes in one go.
>> >
>> > You and Jarod are the most indicated people to point for such needs. Also,
>> > Jon
>> > and David may have some comments.
>> >
>> > From my side, as I said before, I'd like to see a documentation of the
>> > defined API bits,
>> > and the removal of the currently unused ioctls (they can be added later,
>> > together
>> > with the patches that will introduce the code that handles them) to give
>> > my final ack.
>> >
>> > From what I'm seeing, those are the current used ioctls:
>> >
>> > +#define LIRC_GET_FEATURES _IOR('i', 0x, unsigned
>> > long)
>> >
>> > +#define LIRC_GET_LENGTH _IOR('i', 0x000f, unsigned
>> > long)
>>
>> Has this been set into stone yet? if not a 64b word would be more future
>> proof.
>> Checking the existence of features might be better as sysfs
>> attributes. You can have as many sysfs attributes as you want.
>>
>> > +#define LIRC_GET_MIN_TIMEOUT _IOR('i', 0x0008, uint32_t)
>> > +#define LIRC_GET_MAX_TIMEOUT _IOR('i', 0x0009, uint32_t)
>> >
>> > There is also a defined LIRC_GET_REC_MODE that just returns a mask of
>> > GET_FEATURES
>> > bits, and a LIRC_SET_REC_MODE that do nothing.
>> >
>> > I can't comment about the other ioctls, as currently there's no code using
>> > them, but
>> > it seems that some of them would be better implemented as ioctl, like the
>> > ones that
>> > send carrier/burst, etc.
>
> So I'll comment on the LIRC ioctl()'s that cx23885 driver/cx23888-ir.c
> implementation will support. These ioctl()'s are from the
> lirc-0.8.7pre1 CVS snapshot I just downloaded.
>
>
> LIRC_GET_FEATURES
> The driver will support this.
>
> LIRC_GET_SEND_MODE
> LIRC_GET_REC_MODE
> The driver will support these. It will return hard-code
Re: [PATCH 1/3] IR: add core lirc device interface
Jon Smirl wrote: > On Fri, Jun 4, 2010 at 5:17 PM, Jarod Wilson wrote: >> Hrm, struct file_operations specifies an unsigned long for the ioctl >> args, so doesn't that mean we're pretty much stuck with only 32-bit >> for the ioctls? > > I haven't written an IOCTL in a while, but how would you pass a 64b > memory address? On architectures with 64 bits wide memory addresses, unsigned long happens to be 64 bits wide too. :-) IOW unsigned long is defined such that casts from and to void * are lossless. This is not universal though. E.g. Microsoft compilers define unsigned long always only as 32 bits wide AFAIK. -- Stefan Richter -=-==-=- -==- --=-= http://arcgraph.de/sr/ -- 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 1/3] IR: add core lirc device interface
On Fri, 2010-06-04 at 14:57 -0400, Jon Smirl wrote:
> On Fri, Jun 4, 2010 at 2:38 PM, Mauro Carvalho Chehab
> wrote:
> > Em 04-06-2010 12:51, Christoph Bartelmus escreveu:
> >> Hi Mauro,
> >>
> >> on 04 Jun 10 at 01:10, Mauro Carvalho Chehab wrote:
> >>> Em 03-06-2010 19:06, Jarod Wilson escreveu:
> >> [...]
> As for the compat bits... I actually pulled them out of the Fedora kernel
> and userspace for a while, and there were only a few people who really
> ran
> into issues with it, but I think if the new userspace and kernel are
> rolled
> out at the same time in a new distro release (i.e., Fedora 14, in our
> particular case), it should be mostly transparent to users.
> >>
> >>> For sure this will happen on all distros that follows upstream: they'll
> >>> update lirc to fulfill the minimal requirement at Documentation/Changes.
> >>>
> >>> The issue will appear only to people that manually compile kernel and
> >>> lirc.
> >>> Those users are likely smart enough to upgrade to a newer lirc version if
> >>> they notice a trouble, and to check at the forums.
> >>
> Christoph
> wasn't a fan of the change, and actually asked me to revert it, so I'm
> cc'ing him here for further feedback, but I'm inclined to say that if
> this
> is the price we pay to get upstream, so be it.
> >>
> >>> I understand Christoph view, but I think that having to deal with compat
> >>> stuff forever is a high price to pay, as the impact of this change is
> >>> transitory and shouldn't be hard to deal with.
> >>
> >> I'm not against doing this change, but it has to be coordinated between
> >> drivers and user-space.
> >> Just changing lirc.h is not enough. You also have to change all user-space
> >> applications that use the affected ioctls to use the correct types.
> >> That's what Jarod did not address last time so I asked him to revert the
> >> change.
> >
> > For sure coordination between kernel and userspace is very important. I'm
> > sure
> > that Jarod can help with this sync. Also, after having the changes
> > implemented
> > on userspace, I expect one patch from you adding the minimal lirc
> > requirement
> > at Documentation/Changes.
>
> Keep the get_version() ioctl stable. The first thing user space would
> do it call the get_version() ioctl. If user space doesn't support the
> kernel version error out and print a message with the url of the
> project web site and tell the user to upgrade.
>
> If you have the ability to upgrade your kernel you should also have
> the ability to upgrade user space too. I'm no fan of keeping backwards
> compatibility code around. It just becomes piles of clutter that
> nobody maintains. A reliable mechanism to determine version mismatch
> is all that is needed.
>
> PS - I really don't like having to fix bug reports in compatibility
> code. That is an incredible waste of support time that can be easily
> fixed by upgrading the user.
>
> >
> >> And I'd also like to collect all other change request to the API
> >> if there are any and do all changes in one go.
> >
> > You and Jarod are the most indicated people to point for such needs. Also,
> > Jon
> > and David may have some comments.
> >
> > From my side, as I said before, I'd like to see a documentation of the
> > defined API bits,
> > and the removal of the currently unused ioctls (they can be added later,
> > together
> > with the patches that will introduce the code that handles them) to give my
> > final ack.
> >
> > From what I'm seeing, those are the current used ioctls:
> >
> > +#define LIRC_GET_FEATURES _IOR('i', 0x, unsigned long)
> >
> > +#define LIRC_GET_LENGTH_IOR('i', 0x000f, unsigned long)
>
> Has this been set into stone yet? if not a 64b word would be more future
> proof.
> Checking the existence of features might be better as sysfs
> attributes. You can have as many sysfs attributes as you want.
>
> > +#define LIRC_GET_MIN_TIMEOUT _IOR('i', 0x0008, uint32_t)
> > +#define LIRC_GET_MAX_TIMEOUT _IOR('i', 0x0009, uint32_t)
> >
> > There is also a defined LIRC_GET_REC_MODE that just returns a mask of
> > GET_FEATURES
> > bits, and a LIRC_SET_REC_MODE that do nothing.
> >
> > I can't comment about the other ioctls, as currently there's no code using
> > them, but
> > it seems that some of them would be better implemented as ioctl, like the
> > ones that
> > send carrier/burst, etc.
So I'll comment on the LIRC ioctl()'s that cx23885 driver/cx23888-ir.c
implementation will support. These ioctl()'s are from the
lirc-0.8.7pre1 CVS snapshot I just downloaded.
LIRC_GET_FEATURES
The driver will support this.
LIRC_GET_SEND_MODE
LIRC_GET_REC_MODE
The driver will support these. It will return hard-coded values, as the
hardware and cx23888-ir.c only deal with raw pulse time values because
there is no hardware decoding.
LIRC_SET_SEND_MODE
LIRC_SET_REC_MODE
The driver won't support these.
Re: [PATCH 1/3] IR: add core lirc device interface
On Jun 4, 2010, at 10:45 PM, Jarod Wilson wrote:
On Fri, Jun 4, 2010 at 7:16 PM, Jon Smirl wrote:
On Fri, Jun 4, 2010 at 5:17 PM, Jarod Wilson
wrote:
On Fri, Jun 4, 2010 at 4:17 PM, Jarod Wilson
wrote:
On Fri, Jun 04, 2010 at 02:57:04PM -0400, Jon Smirl wrote:
...
From what I'm seeing, those are the current used ioctls:
+#define LIRC_GET_FEATURES _IOR('i', 0x,
unsigned long)
+#define LIRC_GET_LENGTH_IOR('i', 0x000f,
unsigned long)
Has this been set into stone yet? if not a 64b word would be
more future proof.
Nope, not set in stone at all, nothing has been merged. A patch I
was
carrying in Fedora changed all unsigned long to u64 and unsigned
int to
u32, and my current ir wip tree has all u32, but I don't see a
reason why
if we're going to make a change, it couldn't be to all u64, for
as much
future-proofing as possible.
Hrm, struct file_operations specifies an unsigned long for the ioctl
args, so doesn't that mean we're pretty much stuck with only 32-bit
for the ioctls?
I haven't written an IOCTL in a while, but how would you pass a 64b
memory address?
Well, you wouldn't use struct file_operations' ioctl definition if you
wanted to do so on a 32-bit host. :)
Its definitely possible using a different ioctl definition (see
gdth_ioctl_free in drivers/scsi/gdth_proc.c, for example), but we're
currently bound by what's there for file_operations.
There's also the two-pass approach. Just split the address (or feature
flags) across two slightly different ioctl cmds.
--
Jarod Wilson
ja...@wilsonet.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 1/3] IR: add core lirc device interface
On Fri, Jun 4, 2010 at 7:16 PM, Jon Smirl wrote:
> On Fri, Jun 4, 2010 at 5:17 PM, Jarod Wilson wrote:
>> On Fri, Jun 4, 2010 at 4:17 PM, Jarod Wilson wrote:
>>> On Fri, Jun 04, 2010 at 02:57:04PM -0400, Jon Smirl wrote:
>> ...
> From what I'm seeing, those are the current used ioctls:
>
> +#define LIRC_GET_FEATURES _IOR('i', 0x, unsigned
> long)
> +#define LIRC_GET_LENGTH _IOR('i', 0x000f, unsigned
> long)
Has this been set into stone yet? if not a 64b word would be more future
proof.
>>>
>>> Nope, not set in stone at all, nothing has been merged. A patch I was
>>> carrying in Fedora changed all unsigned long to u64 and unsigned int to
>>> u32, and my current ir wip tree has all u32, but I don't see a reason why
>>> if we're going to make a change, it couldn't be to all u64, for as much
>>> future-proofing as possible.
>>
>> Hrm, struct file_operations specifies an unsigned long for the ioctl
>> args, so doesn't that mean we're pretty much stuck with only 32-bit
>> for the ioctls?
>
> I haven't written an IOCTL in a while, but how would you pass a 64b
> memory address?
Well, you wouldn't use struct file_operations' ioctl definition if you
wanted to do so on a 32-bit host. :)
Its definitely possible using a different ioctl definition (see
gdth_ioctl_free in drivers/scsi/gdth_proc.c, for example), but we're
currently bound by what's there for file_operations.
--
Jarod Wilson
ja...@wilsonet.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 1/3] IR: add core lirc device interface
On Fri, Jun 4, 2010 at 5:17 PM, Jarod Wilson wrote:
> On Fri, Jun 4, 2010 at 4:17 PM, Jarod Wilson wrote:
>> On Fri, Jun 04, 2010 at 02:57:04PM -0400, Jon Smirl wrote:
> ...
>>> > From what I'm seeing, those are the current used ioctls:
>>> >
>>> > +#define LIRC_GET_FEATURES _IOR('i', 0x, unsigned
>>> > long)
>>> > +#define LIRC_GET_LENGTH _IOR('i', 0x000f, unsigned
>>> > long)
>>>
>>> Has this been set into stone yet? if not a 64b word would be more future
>>> proof.
>>
>> Nope, not set in stone at all, nothing has been merged. A patch I was
>> carrying in Fedora changed all unsigned long to u64 and unsigned int to
>> u32, and my current ir wip tree has all u32, but I don't see a reason why
>> if we're going to make a change, it couldn't be to all u64, for as much
>> future-proofing as possible.
>
> Hrm, struct file_operations specifies an unsigned long for the ioctl
> args, so doesn't that mean we're pretty much stuck with only 32-bit
> for the ioctls?
I haven't written an IOCTL in a while, but how would you pass a 64b
memory address?
>
> Even with "only" 32 feature flags, I think we'd still be just fine,
> there appear to be only 15 feature flags at present, and I doubt many
> more features need to be added, given how long lirc has been around.
>
> --
> Jarod Wilson
> ja...@wilsonet.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 1/3] IR: add core lirc device interface
On Fri, Jun 4, 2010 at 4:17 PM, Jarod Wilson wrote:
> On Fri, Jun 04, 2010 at 02:57:04PM -0400, Jon Smirl wrote:
...
>> > From what I'm seeing, those are the current used ioctls:
>> >
>> > +#define LIRC_GET_FEATURES _IOR('i', 0x, unsigned
>> > long)
>> > +#define LIRC_GET_LENGTH _IOR('i', 0x000f, unsigned
>> > long)
>>
>> Has this been set into stone yet? if not a 64b word would be more future
>> proof.
>
> Nope, not set in stone at all, nothing has been merged. A patch I was
> carrying in Fedora changed all unsigned long to u64 and unsigned int to
> u32, and my current ir wip tree has all u32, but I don't see a reason why
> if we're going to make a change, it couldn't be to all u64, for as much
> future-proofing as possible.
Hrm, struct file_operations specifies an unsigned long for the ioctl
args, so doesn't that mean we're pretty much stuck with only 32-bit
for the ioctls?
Even with "only" 32 feature flags, I think we'd still be just fine,
there appear to be only 15 feature flags at present, and I doubt many
more features need to be added, given how long lirc has been around.
--
Jarod Wilson
ja...@wilsonet.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 1/3] IR: add core lirc device interface
On Fri, Jun 04, 2010 at 02:57:04PM -0400, Jon Smirl wrote:
> On Fri, Jun 4, 2010 at 2:38 PM, Mauro Carvalho Chehab
> wrote:
> > Em 04-06-2010 12:51, Christoph Bartelmus escreveu:
> >> Hi Mauro,
> >>
> >> on 04 Jun 10 at 01:10, Mauro Carvalho Chehab wrote:
> >>> Em 03-06-2010 19:06, Jarod Wilson escreveu:
> >> [...]
> As for the compat bits... I actually pulled them out of the Fedora kernel
> and userspace for a while, and there were only a few people who really
> ran
> into issues with it, but I think if the new userspace and kernel are
> rolled
> out at the same time in a new distro release (i.e., Fedora 14, in our
> particular case), it should be mostly transparent to users.
> >>
> >>> For sure this will happen on all distros that follows upstream: they'll
> >>> update lirc to fulfill the minimal requirement at Documentation/Changes.
> >>>
> >>> The issue will appear only to people that manually compile kernel and
> >>> lirc.
> >>> Those users are likely smart enough to upgrade to a newer lirc version if
> >>> they notice a trouble, and to check at the forums.
> >>
> Christoph
> wasn't a fan of the change, and actually asked me to revert it, so I'm
> cc'ing him here for further feedback, but I'm inclined to say that if
> this
> is the price we pay to get upstream, so be it.
> >>
> >>> I understand Christoph view, but I think that having to deal with compat
> >>> stuff forever is a high price to pay, as the impact of this change is
> >>> transitory and shouldn't be hard to deal with.
> >>
> >> I'm not against doing this change, but it has to be coordinated between
> >> drivers and user-space.
> >> Just changing lirc.h is not enough. You also have to change all user-space
> >> applications that use the affected ioctls to use the correct types.
> >> That's what Jarod did not address last time so I asked him to revert the
> >> change.
> >
> > For sure coordination between kernel and userspace is very important. I'm
> > sure
> > that Jarod can help with this sync. Also, after having the changes
> > implemented
> > on userspace, I expect one patch from you adding the minimal lirc
> > requirement
> > at Documentation/Changes.
>
> Keep the get_version() ioctl stable. The first thing user space would
> do it call the get_version() ioctl. If user space doesn't support the
> kernel version error out and print a message with the url of the
> project web site and tell the user to upgrade.
>
> If you have the ability to upgrade your kernel you should also have
> the ability to upgrade user space too. I'm no fan of keeping backwards
> compatibility code around. It just becomes piles of clutter that
> nobody maintains. A reliable mechanism to determine version mismatch
> is all that is needed.
That we can definitely do.
> > From what I'm seeing, those are the current used ioctls:
> >
> > +#define LIRC_GET_FEATURES _IOR('i', 0x, unsigned long)
> > +#define LIRC_GET_LENGTH _IOR('i', 0x000f, unsigned long)
>
> Has this been set into stone yet? if not a 64b word would be more future
> proof.
Nope, not set in stone at all, nothing has been merged. A patch I was
carrying in Fedora changed all unsigned long to u64 and unsigned int to
u32, and my current ir wip tree has all u32, but I don't see a reason why
if we're going to make a change, it couldn't be to all u64, for as much
future-proofing as possible.
> Checking the existence of features might be better as sysfs
> attributes. You can have as many sysfs attributes as you want.
I suspect we won't run out of feature bits if we go u64 for the get
features ioctl, and sticking with the ioctl minimizes churn and
potential destabilization.
A perhaps-as-yet unstated desire here is to keep even the out-of-tree,
not-yet-ported-to-ir-core lirc drivers functional with a kernel-provided
lirc_dev. I do plan on porting as many lirc drivers as possible to
ir-core, but it'll take some time, and I don't have hardware for several
of the less common drivers. (For the record, the streamzap, i2c and
zilog drivers are next up for porting, probably zilog first).
> > +#define LIRC_GET_MIN_TIMEOUT _IOR('i', 0x0008, uint32_t)
> > +#define LIRC_GET_MAX_TIMEOUT _IOR('i', 0x0009, uint32_t)
> >
> > There is also a defined LIRC_GET_REC_MODE that just returns a mask of
> > GET_FEATURES
> > bits, and a LIRC_SET_REC_MODE that do nothing.
> >
> > I can't comment about the other ioctls, as currently there's no code using
> > them, but
> > it seems that some of them would be better implemented as ioctl, like the
> > ones that
> > send carrier/burst, etc.
>
> If the ioctls haven't been implemented, leave them as comments in the
> h file. Don't implement them with stubs. Stubs will set the parameters
> into stone and the parameters may be wrong.
I'd either go with commented out or omitted entirely. I think I like
commented out better than omitted entirely though.
Re: [PATCH 1/3] IR: add core lirc device interface
On Fri, Jun 4, 2010 at 2:38 PM, Mauro Carvalho Chehab
wrote:
> Em 04-06-2010 12:51, Christoph Bartelmus escreveu:
>> Hi Mauro,
>>
>> on 04 Jun 10 at 01:10, Mauro Carvalho Chehab wrote:
>>> Em 03-06-2010 19:06, Jarod Wilson escreveu:
>> [...]
As for the compat bits... I actually pulled them out of the Fedora kernel
and userspace for a while, and there were only a few people who really ran
into issues with it, but I think if the new userspace and kernel are rolled
out at the same time in a new distro release (i.e., Fedora 14, in our
particular case), it should be mostly transparent to users.
>>
>>> For sure this will happen on all distros that follows upstream: they'll
>>> update lirc to fulfill the minimal requirement at Documentation/Changes.
>>>
>>> The issue will appear only to people that manually compile kernel and lirc.
>>> Those users are likely smart enough to upgrade to a newer lirc version if
>>> they notice a trouble, and to check at the forums.
>>
Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm
cc'ing him here for further feedback, but I'm inclined to say that if this
is the price we pay to get upstream, so be it.
>>
>>> I understand Christoph view, but I think that having to deal with compat
>>> stuff forever is a high price to pay, as the impact of this change is
>>> transitory and shouldn't be hard to deal with.
>>
>> I'm not against doing this change, but it has to be coordinated between
>> drivers and user-space.
>> Just changing lirc.h is not enough. You also have to change all user-space
>> applications that use the affected ioctls to use the correct types.
>> That's what Jarod did not address last time so I asked him to revert the
>> change.
>
> For sure coordination between kernel and userspace is very important. I'm sure
> that Jarod can help with this sync. Also, after having the changes implemented
> on userspace, I expect one patch from you adding the minimal lirc requirement
> at Documentation/Changes.
Keep the get_version() ioctl stable. The first thing user space would
do it call the get_version() ioctl. If user space doesn't support the
kernel version error out and print a message with the url of the
project web site and tell the user to upgrade.
If you have the ability to upgrade your kernel you should also have
the ability to upgrade user space too. I'm no fan of keeping backwards
compatibility code around. It just becomes piles of clutter that
nobody maintains. A reliable mechanism to determine version mismatch
is all that is needed.
PS - I really don't like having to fix bug reports in compatibility
code. That is an incredible waste of support time that can be easily
fixed by upgrading the user.
>
>> And I'd also like to collect all other change request to the API
>> if there are any and do all changes in one go.
>
> You and Jarod are the most indicated people to point for such needs. Also, Jon
> and David may have some comments.
>
> From my side, as I said before, I'd like to see a documentation of the
> defined API bits,
> and the removal of the currently unused ioctls (they can be added later,
> together
> with the patches that will introduce the code that handles them) to give my
> final ack.
>
> From what I'm seeing, those are the current used ioctls:
>
> +#define LIRC_GET_FEATURES _IOR('i', 0x, unsigned long)
>
> +#define LIRC_GET_LENGTH _IOR('i', 0x000f, unsigned long)
Has this been set into stone yet? if not a 64b word would be more future proof.
Checking the existence of features might be better as sysfs
attributes. You can have as many sysfs attributes as you want.
> +#define LIRC_GET_MIN_TIMEOUT _IOR('i', 0x0008, uint32_t)
> +#define LIRC_GET_MAX_TIMEOUT _IOR('i', 0x0009, uint32_t)
>
> There is also a defined LIRC_GET_REC_MODE that just returns a mask of
> GET_FEATURES
> bits, and a LIRC_SET_REC_MODE that do nothing.
>
> I can't comment about the other ioctls, as currently there's no code using
> them, but
> it seems that some of them would be better implemented as ioctl, like the
> ones that
> send carrier/burst, etc.
If the ioctls haven't been implemented, leave them as comments in the
h file. Don't implement them with stubs. Stubs will set the parameters
into stone and the parameters may be wrong.
> One discussion that may be pertinent is if we should add ioctls for those
> controls,
> or if it would be better to just add sysfs nodes for them.
>
> As all those ioctls are related to config stuff, IMO, using sysfs would be
> better, but
> I haven't a closed opinion about that matter.
In general sysfs is used to set options that are static or only change
infrequently.
If the parameter is set on every request, use an IOCTL.
>
> Btw, a lirc userspace that would work with both the out-of-tree and in-tree
> lirc-dev
> can easily check for the proper sysfs nodes to know what version of the
> driver is b
Re: [PATCH 1/3] IR: add core lirc device interface
Em 04-06-2010 12:51, Christoph Bartelmus escreveu:
> Hi Mauro,
>
> on 04 Jun 10 at 01:10, Mauro Carvalho Chehab wrote:
>> Em 03-06-2010 19:06, Jarod Wilson escreveu:
> [...]
>>> As for the compat bits... I actually pulled them out of the Fedora kernel
>>> and userspace for a while, and there were only a few people who really ran
>>> into issues with it, but I think if the new userspace and kernel are rolled
>>> out at the same time in a new distro release (i.e., Fedora 14, in our
>>> particular case), it should be mostly transparent to users.
>
>> For sure this will happen on all distros that follows upstream: they'll
>> update lirc to fulfill the minimal requirement at Documentation/Changes.
>>
>> The issue will appear only to people that manually compile kernel and lirc.
>> Those users are likely smart enough to upgrade to a newer lirc version if
>> they notice a trouble, and to check at the forums.
>
>>> Christoph
>>> wasn't a fan of the change, and actually asked me to revert it, so I'm
>>> cc'ing him here for further feedback, but I'm inclined to say that if this
>>> is the price we pay to get upstream, so be it.
>
>> I understand Christoph view, but I think that having to deal with compat
>> stuff forever is a high price to pay, as the impact of this change is
>> transitory and shouldn't be hard to deal with.
>
> I'm not against doing this change, but it has to be coordinated between
> drivers and user-space.
> Just changing lirc.h is not enough. You also have to change all user-space
> applications that use the affected ioctls to use the correct types.
> That's what Jarod did not address last time so I asked him to revert the
> change.
For sure coordination between kernel and userspace is very important. I'm sure
that Jarod can help with this sync. Also, after having the changes implemented
on userspace, I expect one patch from you adding the minimal lirc requirement
at Documentation/Changes.
> And I'd also like to collect all other change request to the API
> if there are any and do all changes in one go.
You and Jarod are the most indicated people to point for such needs. Also, Jon
and David may have some comments.
>From my side, as I said before, I'd like to see a documentation of the defined
>API bits,
and the removal of the currently unused ioctls (they can be added later,
together
with the patches that will introduce the code that handles them) to give my
final ack.
>From what I'm seeing, those are the current used ioctls:
+#define LIRC_GET_FEATURES _IOR('i', 0x, unsigned long)
+#define LIRC_GET_LENGTH_IOR('i', 0x000f, unsigned long)
+#define LIRC_GET_MIN_TIMEOUT _IOR('i', 0x0008, uint32_t)
+#define LIRC_GET_MAX_TIMEOUT _IOR('i', 0x0009, uint32_t)
There is also a defined LIRC_GET_REC_MODE that just returns a mask of
GET_FEATURES
bits, and a LIRC_SET_REC_MODE that do nothing.
I can't comment about the other ioctls, as currently there's no code using
them, but
it seems that some of them would be better implemented as ioctl, like the ones
that
send carrier/burst, etc.
One discussion that may be pertinent is if we should add ioctls for those
controls,
or if it would be better to just add sysfs nodes for them.
As all those ioctls are related to config stuff, IMO, using sysfs would be
better, but
I haven't a closed opinion about that matter.
Btw, a lirc userspace that would work with both the out-of-tree and in-tree
lirc-dev
can easily check for the proper sysfs nodes to know what version of the driver
is being
used. It will need access sysfs anyway, to enable the lirc decoder.
Cheers,
Mauro.
--
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 1/3] IR: add core lirc device interface
Hi Mauro, on 04 Jun 10 at 01:10, Mauro Carvalho Chehab wrote: > Em 03-06-2010 19:06, Jarod Wilson escreveu: [...] >> As for the compat bits... I actually pulled them out of the Fedora kernel >> and userspace for a while, and there were only a few people who really ran >> into issues with it, but I think if the new userspace and kernel are rolled >> out at the same time in a new distro release (i.e., Fedora 14, in our >> particular case), it should be mostly transparent to users. > For sure this will happen on all distros that follows upstream: they'll > update lirc to fulfill the minimal requirement at Documentation/Changes. > > The issue will appear only to people that manually compile kernel and lirc. > Those users are likely smart enough to upgrade to a newer lirc version if > they notice a trouble, and to check at the forums. >> Christoph >> wasn't a fan of the change, and actually asked me to revert it, so I'm >> cc'ing him here for further feedback, but I'm inclined to say that if this >> is the price we pay to get upstream, so be it. > I understand Christoph view, but I think that having to deal with compat > stuff forever is a high price to pay, as the impact of this change is > transitory and shouldn't be hard to deal with. I'm not against doing this change, but it has to be coordinated between drivers and user-space. Just changing lirc.h is not enough. You also have to change all user-space applications that use the affected ioctls to use the correct types. That's what Jarod did not address last time so I asked him to revert the change. And I'd also like to collect all other change request to the API if there are any and do all changes in one go. 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 1/3] IR: add core lirc device interface
Em 03-06-2010 19:06, Jarod Wilson escreveu:
> On Thu, Jun 03, 2010 at 03:02:11AM -0300, Mauro Carvalho Chehab wrote:
>> Em 01-06-2010 17:51, Jarod Wilson escreveu:
>>> Add the core lirc device interface (http://lirc.org/).
>>>
>>> This is a 99.9% unaltered lirc_dev device interface (only change being
>>> the path to lirc.h), which has been carried in the Fedora kernels and
>>> has been built for numerous other distro kernels for years. In the
>>> next two patches in this series, ir-core will be wired up to make use
>>> of the lirc interface as one of many IR protocol decoder options. In
>>> this case, raw IR will be delivered to the lirc userspace daemon, which
>>> will then handle the decoding.
>>>
>>> Signed-off-by: Jarod Wilson
>>> ---
>>> drivers/media/IR/Kconfig| 11 +
>>> drivers/media/IR/Makefile |1 +
>>> drivers/media/IR/lirc_dev.c | 850
>>> +++
>>> drivers/media/IR/lirc_dev.h | 228
>>> include/media/lirc.h| 159
>>> 5 files changed, 1249 insertions(+), 0 deletions(-)
>>> create mode 100644 drivers/media/IR/lirc_dev.c
>>> create mode 100644 drivers/media/IR/lirc_dev.h
>>> create mode 100644 include/media/lirc.h
>>>
> ...
>>> +#ifdef CONFIG_COMPAT
>>> +#define LIRC_GET_FEATURES_COMPAT32 _IOR('i', 0x, __u32)
>>> +
>>> +#define LIRC_GET_SEND_MODE_COMPAT32_IOR('i', 0x0001, __u32)
>>> +#define LIRC_GET_REC_MODE_COMPAT32 _IOR('i', 0x0002, __u32)
>>> +
>>> +#define LIRC_GET_LENGTH_COMPAT32 _IOR('i', 0x000f, __u32)
>>> +
>>> +#define LIRC_SET_SEND_MODE_COMPAT32_IOW('i', 0x0011, __u32)
>>> +#define LIRC_SET_REC_MODE_COMPAT32 _IOW('i', 0x0012, __u32)
>>> +
>>> +long lirc_dev_fop_compat_ioctl(struct file *file,
>>> + unsigned int cmd32,
>>> + unsigned long arg)
>>> +{
>>> + mm_segment_t old_fs;
>>> + int ret;
>>> + unsigned long val;
>>> + unsigned int cmd;
>>> +
>>> + switch (cmd32) {
>>> + case LIRC_GET_FEATURES_COMPAT32:
>>> + case LIRC_GET_SEND_MODE_COMPAT32:
>>> + case LIRC_GET_REC_MODE_COMPAT32:
>>> + case LIRC_GET_LENGTH_COMPAT32:
>>> + case LIRC_SET_SEND_MODE_COMPAT32:
>>> + case LIRC_SET_REC_MODE_COMPAT32:
>>> + /*
>>> +* These commands expect (unsigned long *) arg
>>> +* but the 32-bit app supplied (__u32 *).
>>> +* Conversion is required.
>>> +*/
>>> + if (get_user(val, (__u32 *)compat_ptr(arg)))
>>> + return -EFAULT;
>>> + lock_kernel();
>>
>> Hmm... BKL? Are you sure you need it here? As BKL are being removed, please
>> rewrite
>> it to use another kind of lock.
>>
>> On a first glance, I suspect that you should be locking &ir->irctl_lock
>> inside
>> lirc_dev_fop_ioctl() and just remove the BKL calls on
>> lirc_dev_fop_compat_ioctl().
>
> Ew, yeah, looks like there's some missing locking in lirc_dev_fop_ioctl(),
> though its never been a problem in practice, from what I've seen... Okay,
> will add locking there.
Ok, thanks. Well, in the past, the ioctl where already blocked by BKL. So, the
lock
is/will probably needed only with newer kernels.
> As for the compat bits... I actually pulled them out of the Fedora kernel
> and userspace for a while, and there were only a few people who really ran
> into issues with it, but I think if the new userspace and kernel are rolled
> out at the same time in a new distro release (i.e., Fedora 14, in our
> particular case), it should be mostly transparent to users.
For sure this will happen on all distros that follows upstream: they'll update
lirc
to fulfill the minimal requirement at Documentation/Changes.
The issue will appear only to people that manually compile kernel and lirc.
Those
users are likely smart enough to upgrade to a newer lirc version if they notice
a
trouble, and to check at the forums.
> Christoph
> wasn't a fan of the change, and actually asked me to revert it, so I'm
> cc'ing him here for further feedback, but I'm inclined to say that if this
> is the price we pay to get upstream, so be it.
I understand Christoph view, but I think that having to deal with compat stuff
forever
is a high price to pay, as the impact of this change is transitory and shouldn't
be hard to deal with.
>> Btw, as this is the first in-tree kernel driver for lirc, I would just
>> define the
>> lirc ioctls with __u32 and remove the entire compat stuff.
>
> I've pushed a patch into my ir-wip tree, ir branch, that does this:
>
> http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/ir
>
> I've tested it out, and things still work as expected w/my mceusb port and
> the ir-lirc-codec ir-core bridge driver, after rebuilding the lirc
> userspace (64-bit host) to match up the ioctl definitions.
Patches look sane on my eyes.
>
>>> +/*
>>> + * from the next key press on the driver will send
>>> + * LIRC_MODE2_FREQUENCY packets
Re: [PATCH 1/3] IR: add core lirc device interface
On Thu, Jun 03, 2010 at 03:02:11AM -0300, Mauro Carvalho Chehab wrote:
> Em 01-06-2010 17:51, Jarod Wilson escreveu:
> > Add the core lirc device interface (http://lirc.org/).
> >
> > This is a 99.9% unaltered lirc_dev device interface (only change being
> > the path to lirc.h), which has been carried in the Fedora kernels and
> > has been built for numerous other distro kernels for years. In the
> > next two patches in this series, ir-core will be wired up to make use
> > of the lirc interface as one of many IR protocol decoder options. In
> > this case, raw IR will be delivered to the lirc userspace daemon, which
> > will then handle the decoding.
> >
> > Signed-off-by: Jarod Wilson
> > ---
> > drivers/media/IR/Kconfig| 11 +
> > drivers/media/IR/Makefile |1 +
> > drivers/media/IR/lirc_dev.c | 850
> > +++
> > drivers/media/IR/lirc_dev.h | 228
> > include/media/lirc.h| 159
> > 5 files changed, 1249 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/media/IR/lirc_dev.c
> > create mode 100644 drivers/media/IR/lirc_dev.h
> > create mode 100644 include/media/lirc.h
> >
...
> > +#ifdef CONFIG_COMPAT
> > +#define LIRC_GET_FEATURES_COMPAT32 _IOR('i', 0x, __u32)
> > +
> > +#define LIRC_GET_SEND_MODE_COMPAT32_IOR('i', 0x0001, __u32)
> > +#define LIRC_GET_REC_MODE_COMPAT32 _IOR('i', 0x0002, __u32)
> > +
> > +#define LIRC_GET_LENGTH_COMPAT32 _IOR('i', 0x000f, __u32)
> > +
> > +#define LIRC_SET_SEND_MODE_COMPAT32_IOW('i', 0x0011, __u32)
> > +#define LIRC_SET_REC_MODE_COMPAT32 _IOW('i', 0x0012, __u32)
> > +
> > +long lirc_dev_fop_compat_ioctl(struct file *file,
> > + unsigned int cmd32,
> > + unsigned long arg)
> > +{
> > + mm_segment_t old_fs;
> > + int ret;
> > + unsigned long val;
> > + unsigned int cmd;
> > +
> > + switch (cmd32) {
> > + case LIRC_GET_FEATURES_COMPAT32:
> > + case LIRC_GET_SEND_MODE_COMPAT32:
> > + case LIRC_GET_REC_MODE_COMPAT32:
> > + case LIRC_GET_LENGTH_COMPAT32:
> > + case LIRC_SET_SEND_MODE_COMPAT32:
> > + case LIRC_SET_REC_MODE_COMPAT32:
> > + /*
> > +* These commands expect (unsigned long *) arg
> > +* but the 32-bit app supplied (__u32 *).
> > +* Conversion is required.
> > +*/
> > + if (get_user(val, (__u32 *)compat_ptr(arg)))
> > + return -EFAULT;
> > + lock_kernel();
>
> Hmm... BKL? Are you sure you need it here? As BKL are being removed, please
> rewrite
> it to use another kind of lock.
>
> On a first glance, I suspect that you should be locking &ir->irctl_lock inside
> lirc_dev_fop_ioctl() and just remove the BKL calls on
> lirc_dev_fop_compat_ioctl().
Ew, yeah, looks like there's some missing locking in lirc_dev_fop_ioctl(),
though its never been a problem in practice, from what I've seen... Okay,
will add locking there.
As for the compat bits... I actually pulled them out of the Fedora kernel
and userspace for a while, and there were only a few people who really ran
into issues with it, but I think if the new userspace and kernel are rolled
out at the same time in a new distro release (i.e., Fedora 14, in our
particular case), it should be mostly transparent to users. Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm
cc'ing him here for further feedback, but I'm inclined to say that if this
is the price we pay to get upstream, so be it.
> Btw, as this is the first in-tree kernel driver for lirc, I would just define
> the
> lirc ioctls with __u32 and remove the entire compat stuff.
I've pushed a patch into my ir-wip tree, ir branch, that does this:
http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/ir
I've tested it out, and things still work as expected w/my mceusb port and
the ir-lirc-codec ir-core bridge driver, after rebuilding the lirc
userspace (64-bit host) to match up the ioctl definitions.
> > +/*
> > + * from the next key press on the driver will send
> > + * LIRC_MODE2_FREQUENCY packets
> > + */
> > +#define LIRC_MEASURE_CARRIER_ENABLE_IO('i', 0x0021)
> > +#define LIRC_MEASURE_CARRIER_DISABLE _IO('i', 0x0022)
> > +
> > +#endif
>
> Wow! There are lots of ioctls that aren't currently used. IMO, the better is
> to add
> at the file just the ioctls that are currently in usage, and to put some
> documentation
> about them at /Documentation.
Several of the ioctls were only very recently (past 4 months) added, but I
haven't a clue what they're actually used for... Adding something to
Documentation/ would definitely be prudent in any case.
--
Jarod Wilson
ja...@redhat.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 1/3] IR: add core lirc device interface
Em 01-06-2010 17:51, Jarod Wilson escreveu:
> Add the core lirc device interface (http://lirc.org/).
>
> This is a 99.9% unaltered lirc_dev device interface (only change being
> the path to lirc.h), which has been carried in the Fedora kernels and
> has been built for numerous other distro kernels for years. In the
> next two patches in this series, ir-core will be wired up to make use
> of the lirc interface as one of many IR protocol decoder options. In
> this case, raw IR will be delivered to the lirc userspace daemon, which
> will then handle the decoding.
>
> Signed-off-by: Jarod Wilson
> ---
> drivers/media/IR/Kconfig| 11 +
> drivers/media/IR/Makefile |1 +
> drivers/media/IR/lirc_dev.c | 850
> +++
> drivers/media/IR/lirc_dev.h | 228
> include/media/lirc.h| 159
> 5 files changed, 1249 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/IR/lirc_dev.c
> create mode 100644 drivers/media/IR/lirc_dev.h
> create mode 100644 include/media/lirc.h
>
> diff --git a/drivers/media/IR/Kconfig b/drivers/media/IR/Kconfig
> index 7ffa86f..c3010fb 100644
> --- a/drivers/media/IR/Kconfig
> +++ b/drivers/media/IR/Kconfig
> @@ -8,6 +8,17 @@ config VIDEO_IR
> depends on IR_CORE
> default IR_CORE
>
> +config LIRC
> + tristate
> + default y
> +
> + ---help---
> +Enable this option to build the Linux Infrared Remote
> +Control (LIRC) core device interface driver. The LIRC
> +interface passes raw IR to and from userspace, where the
> +LIRC daemon handles protocol decoding for IR reception ann
> +encoding for IR transmitting (aka "blasting").
> +
> source "drivers/media/IR/keymaps/Kconfig"
>
> config IR_NEC_DECODER
> diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
> index b43fe36..3ba00bb 100644
> --- a/drivers/media/IR/Makefile
> +++ b/drivers/media/IR/Makefile
> @@ -5,6 +5,7 @@ obj-y += keymaps/
>
> obj-$(CONFIG_IR_CORE) += ir-core.o
> obj-$(CONFIG_VIDEO_IR) += ir-common.o
> +obj-$(CONFIG_LIRC) += lirc_dev.o
> obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
> obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
> obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o
> diff --git a/drivers/media/IR/lirc_dev.c b/drivers/media/IR/lirc_dev.c
> new file mode 100644
> index 000..7e45800
> --- /dev/null
> +++ b/drivers/media/IR/lirc_dev.c
> @@ -0,0 +1,850 @@
> +/*
> + * LIRC base driver
> + *
> + * by Artur Lipowski
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#ifdef CONFIG_COMPAT
> +#include
> +#endif
> +
> +#include
> +#include "lirc_dev.h"
> +
> +static int debug;
> +
> +#define IRCTL_DEV_NAME "BaseRemoteCtl"
> +#define NOPLUG -1
> +#define LOGHEAD "lirc_dev (%s[%d]): "
> +
> +static dev_t lirc_base_dev;
> +
> +struct irctl {
> + struct lirc_driver d;
> + int attached;
> + int open;
> +
> + struct mutex irctl_lock;
> + struct lirc_buffer *buf;
> + unsigned int chunk_size;
> +
> + struct task_struct *task;
> + long jiffies_to_wait;
> +
> + struct cdev cdev;
> +};
> +
> +static DEFINE_MUTEX(lirc_dev_lock);
> +
> +static struct irctl *irctls[MAX_IRCTL_DEVICES];
> +
> +/* Only used for sysfs but defined to void otherwise */
> +static struct class *lirc_class;
> +
> +/* helper function
> + * initializes the irctl structure
> + */
> +static void init_irctl(struct irctl *ir)
> +{
> + dev_dbg(ir->d.dev, LOGHEAD "initializing irctl\n",
> + ir->d.name, ir->d.minor);
> + mutex_init(&ir->irctl_lock);
> + ir->d.minor = NOPLUG;
> +}
> +
> +static void cleanup(struct irctl *ir)
> +{
> + dev_dbg(ir->d.dev, LOGHEAD "cleaning up\n", ir->d.name, ir->d.minor);
> +
> + device_destroy(lirc_class, MKDEV(MAJOR(lirc_base_dev), ir->d.minor));
> +
> + if (ir->buf != ir->d.rbuf) {
> + lirc_buffer_free(ir->buf);
> + kfree(ir->buf);
> + }
> + ir->buf = NULL;
> +}
> +
> +/*
[PATCH 1/3] IR: add core lirc device interface
Add the core lirc device interface (http://lirc.org/).
This is a 99.9% unaltered lirc_dev device interface (only change being
the path to lirc.h), which has been carried in the Fedora kernels and
has been built for numerous other distro kernels for years. In the
next two patches in this series, ir-core will be wired up to make use
of the lirc interface as one of many IR protocol decoder options. In
this case, raw IR will be delivered to the lirc userspace daemon, which
will then handle the decoding.
Signed-off-by: Jarod Wilson
---
drivers/media/IR/Kconfig| 11 +
drivers/media/IR/Makefile |1 +
drivers/media/IR/lirc_dev.c | 850 +++
drivers/media/IR/lirc_dev.h | 228
include/media/lirc.h| 159
5 files changed, 1249 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/IR/lirc_dev.c
create mode 100644 drivers/media/IR/lirc_dev.h
create mode 100644 include/media/lirc.h
diff --git a/drivers/media/IR/Kconfig b/drivers/media/IR/Kconfig
index 7ffa86f..c3010fb 100644
--- a/drivers/media/IR/Kconfig
+++ b/drivers/media/IR/Kconfig
@@ -8,6 +8,17 @@ config VIDEO_IR
depends on IR_CORE
default IR_CORE
+config LIRC
+ tristate
+ default y
+
+ ---help---
+ Enable this option to build the Linux Infrared Remote
+ Control (LIRC) core device interface driver. The LIRC
+ interface passes raw IR to and from userspace, where the
+ LIRC daemon handles protocol decoding for IR reception ann
+ encoding for IR transmitting (aka "blasting").
+
source "drivers/media/IR/keymaps/Kconfig"
config IR_NEC_DECODER
diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
index b43fe36..3ba00bb 100644
--- a/drivers/media/IR/Makefile
+++ b/drivers/media/IR/Makefile
@@ -5,6 +5,7 @@ obj-y += keymaps/
obj-$(CONFIG_IR_CORE) += ir-core.o
obj-$(CONFIG_VIDEO_IR) += ir-common.o
+obj-$(CONFIG_LIRC) += lirc_dev.o
obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o
diff --git a/drivers/media/IR/lirc_dev.c b/drivers/media/IR/lirc_dev.c
new file mode 100644
index 000..7e45800
--- /dev/null
+++ b/drivers/media/IR/lirc_dev.c
@@ -0,0 +1,850 @@
+/*
+ * LIRC base driver
+ *
+ * by Artur Lipowski
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#ifdef CONFIG_COMPAT
+#include
+#endif
+
+#include
+#include "lirc_dev.h"
+
+static int debug;
+
+#define IRCTL_DEV_NAME "BaseRemoteCtl"
+#define NOPLUG -1
+#define LOGHEAD"lirc_dev (%s[%d]): "
+
+static dev_t lirc_base_dev;
+
+struct irctl {
+ struct lirc_driver d;
+ int attached;
+ int open;
+
+ struct mutex irctl_lock;
+ struct lirc_buffer *buf;
+ unsigned int chunk_size;
+
+ struct task_struct *task;
+ long jiffies_to_wait;
+
+ struct cdev cdev;
+};
+
+static DEFINE_MUTEX(lirc_dev_lock);
+
+static struct irctl *irctls[MAX_IRCTL_DEVICES];
+
+/* Only used for sysfs but defined to void otherwise */
+static struct class *lirc_class;
+
+/* helper function
+ * initializes the irctl structure
+ */
+static void init_irctl(struct irctl *ir)
+{
+ dev_dbg(ir->d.dev, LOGHEAD "initializing irctl\n",
+ ir->d.name, ir->d.minor);
+ mutex_init(&ir->irctl_lock);
+ ir->d.minor = NOPLUG;
+}
+
+static void cleanup(struct irctl *ir)
+{
+ dev_dbg(ir->d.dev, LOGHEAD "cleaning up\n", ir->d.name, ir->d.minor);
+
+ device_destroy(lirc_class, MKDEV(MAJOR(lirc_base_dev), ir->d.minor));
+
+ if (ir->buf != ir->d.rbuf) {
+ lirc_buffer_free(ir->buf);
+ kfree(ir->buf);
+ }
+ ir->buf = NULL;
+}
+
+/* helper function
+ * reads key codes from driver and puts them into buffer
+ * returns 0 on success
+ */
+static int add_to_buf(struct irctl *ir)
+{
+ if (ir->d.add_to_buf) {
+ int res = -ENODATA;
+ int got_data = 0;
+
+ /*
+* service the device as long
