On Tue, Jun 08, 2010 at 11:46:36PM -0400, Jarod Wilson wrote:
> On Tue, Jun 8, 2010 at 1:50 PM, David Härdeman <david@xxxxxxxxxxx> wrote:
> > On Mon, Jun 07, 2010 at 04:15:30PM -0400, Jarod Wilson wrote:
> >> On Mon, Jun 07, 2010 at 09:00:03PM +0200, David Härdeman wrote:
> >> > On Mon, May 03, 2010 at 05:00:05PM -0300, Mauro Carvalho Chehab wrote:
> >> > > David Härdeman wrote:
> >> > > > This patch moves the state from each raw decoder into the
> >> > > > ir_raw_event_ctrl struct.
> >> > > >
> >> > > > This allows the removal of code like this:
> >> > > >
> >> > > > spin_lock(&decoder_lock);
> >> > > > list_for_each_entry(data, &decoder_list, list) {
> >> > > > if (data->ir_dev == ir_dev)
> >> > > > break;
> >> > > > }
> >> > > > spin_unlock(&decoder_lock);
> >> > > > return data;
> >> > > >
> >> > > > which is currently run for each decoder on each event in order
> >> > > > to get the client-specific decoding state data.
> >> > > >
> >> > > > In addition, ir decoding modules and ir driver module load
> >> > > > order is now independent. Centralizing the data also allows
> >> > > > for a nice code reduction of about 30% per raw decoder as
> >> > > > client lists and client registration callbacks are no longer
> >> > > > necessary.
> >> > >
> >> > > The registration callbacks will likely still be needed by lirc,
> >> > > as you need to create/delete lirc_dev interfaces, when the module
> >> > > is registered, but I might be wrong. It would be interesting to
> >> > > add lirc_dev first, in order to be sure about the better interfaces
> >> > > for it.
> >> >
> >> > Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the
> >> > current interfaces are not good enough since it'll break if lirc_dev is
> >> > loaded after the hardware modules.
> >>
> >> This is something I've been meaning to mention myself. On system boot, if
> >> an mceusb device is connected, it pretty regularly only has the NEC
> >> decoder available to use. I have to reload mceusb, or make sure ir-core is
> >> explicitly loaded, wait a bit, then load mceusb, if I want to have all of
> >> the protocol handlers available -- which includes the needed-by-default
> >> rc6 one. I've only briefly tinkered with trying to fix it, sounds like you
> >> may already have fixage within this patchset.
> >
> > The problem is that without the patchset, each decoder is expected to
> > carry it's own list of datastructures for each hardware receiver.
> > Hardware receiver addition/removal is signalled through a callback to
> > the decoder, but the callback will (naturally) not be invoked if the
> > hardware driver is already loaded when the decoder is. So loading a
> > decoder "late" or reloading a decoder will mean that it doesn't know
> > about pre-existing hardware.
> >
> >> > In addition, random module load order is currently broken (try loading
> >> > decoders first and hardware later and you'll see). With this patch, it
> >> > works again.
> >>
> >> Want.
> >
> > Then please help me with two things:
> >
> > a) Test the patches I just sent (especially 6/8 and 7/8, they should
> > be independent from the rest)
>
> Working on merging them into a tree here locally. There's been a bit
> of churn, so the last few didn't apply cleanly, but I'm almost there.
>
> > b) Mauro mentioned in <4BDF28C0.4060102@xxxxxxxxxx> that:
> >
> > I liked the idea of your redesign, but I didn't like the removal
> > of a per-decoder sysfs entry. As already discussed, there are
> > cases where we'll need a per-decoder sysfs entry (lirc_dev is
> > probably one of those cases - also Jarod's imon driver is
> > currently implementing a modprobe parameter that needs to be
> > moved to the driver).
> >
> > could you please confirm if your lirc and/or imon drivers would be
> > negatively affected by the proposed patches?
>
> Will do so once I get them wedged in on top.
Got it all merged and compiling, but not yet runtime tested. Compiling
alone sheds some light on things though...
So this definitely negatively impacts my ir-core-to-lirc_dev
(ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device
registration work in its register function. However, if (after your
patchset) we add a new pair of callbacks replacing raw_register and
raw_unregister, which are optional, that work could be done there instead,
so I don't think this is an insurmountable obstacle for the lirc bits.
As for the imon driver, the modprobe parameter in question (iirc) was
already removed from the driver, as its functionality is replaced by
implementing a change_protocol callback. However, there's a request to
add it (or something like it) back to the driver to allow disabling the
IR part altogether, and there are a few other modparams that might be
better suited as sysfs entries. However, its actually not relevant to the
case of registering raw protocol handlers, as the imon devices do their
decoding in hardware. I can see the possibility for protocol-specific
knobs in sysfs though. But I think the same optional callbacks I'd use to
keep the lirc bits working could also be used for this. Can't think of a
good name for these yet, probably need more coffee first... ;)
--
Jarod Wilson
jarod@xxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
