On Mon, May 03, 2010 at 04:49:20PM -0300, Mauro Carvalho Chehab wrote:
> Hi David,
Hi, sorry for dropping off the radar...life has been hetic...daughter in
hospital (she's fine now).
> David Härdeman wrote:
> > With the current logic, each raw decoder needs to add a copy of the exact
> > same sysfs code. This is both unnecessary and also means that (re)loading
> > an IR driver after raw decoder modules have been loaded won't work as
> > expected.
> >
> > This patch moves that logic into ir-raw-event and adds a single sysfs
> > file per device.
> >
> > Reading that file returns something like:
> >
> > "rc5 [rc6] nec jvc [sony]"
> >
> > (with enabled protocols in [] brackets)
> >
> > Writing either "+protocol" or "-protocol" to that file will
> > enable or disable the according protocol decoder.
> >
> > An additional benefit is that the disabling of a decoder will be
> > remembered across module removal/insertion so a previously
> > disabled decoder won't suddenly be activated again. The default
> > setting is to enable all decoders.
> >
> > This is also necessary for the next patch which moves even more decoder
> > state into the central raw decoding structs.
>
> 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).
per-decoder or per-decoder-per-receiver sysfs entries? If you want the
latter, you'll need much more code than what is currently there to not
break random module load order (which doesn't work with the current
code). Additionally, *if* imon and lirc_dev really need something like
that, those modules should carry the burden.
> So, while we can implement some core support at the raw event core, we should
> keep the decoder attributes internally inside the driver.
Why?
> So, each decoder
> may have his own code like:
>
> static struct attribute *decoder_attributes[] = {
> &dev_attr_enabled.attr,
> &dev_attr_bar1.attr,
> &dev_attr_bar2.attr,
> &dev_attr_bar3.attr,
> NULL
> };
>
> static struct attribute_group decoder_attribute_group = {
> .name = "foo_decoder",
> .attrs = decoder_attributes,
> };
>
> As the attr_enabled is common to all, maybe we can have a default enable
> code at the .h or inside the core.
Smells like lots of duplicated code with no gain for the current set of
decoders.
--
David Härdeman
--
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
