On Thu, June 25, 2009 00:13, Jesse Barnes wrote: > On Wed, 24 Jun 2009 14:36:45 -0700 > David Härdeman <david@xxxxxxxxxxx> wrote: > >> I've written a driver for the ... >> Winbond WPCD376I chipset > > Yay, glad I could get these released for you. I just did a quick scan > of the driver (notes below) Two more things that Intel could provide: a) Publish the datasheet (I know you mentioned doing this but I can't find it on the Intel website) b) Make the hardware needed to actually use the CIR functionality available for purchase. http://www.easy-cir.com seems to be more or less dead (which is curious since an ad for the website seems to be included with every CIR-enabled Intel motherboard). I had to solder my own IR receiver in order to write the driver. >> I'd appreciate having the driver reviewed...and in addition I have >> some questions for the list: >> >> 1) SuperI/O concurrency >> ... > > Yeah, often multifunction devices like this have higher level "bus > drivers" that take care of managing the global parts, and drivers that > attach to it to manage individual functions. If you were feeling > really ambitious you could do that for the superio chip and port any > sub-drivers... :) My ambitions are more directed towards some kind of IR-subsystem into the kernel at the moment :) Besides, the Intel mainboards doesn't actually seem to use any of the other logical devices (which are mostly supported by existing drivers anyway). >> Where should this driver go in the tree? drivers/platform/x86/? > > drivers/char is probably fine. I'm leaning towards drivers/input/misc now... >> #define dprintk(fmt, arg...) \ >> do { \ >> if (debug) \ >> printk(KERN_DEBUG DRVNAME fmt , ## arg); \ >> } while (0) > > Maybe you could use the generic debug functions instead (pr_debug iirc)? Yes > ... > There are a few magic numbers above here you could possibly make into > #defines just to make things more readable. I'll try > The key up/down timeout handling seems like a pretty general problem, > maybe the input layer has some helpers for it? Dunno. drivers/media/common/ir-functions.c is the closest thing I could find while writing the driver. The functions there aren't usable because they do not properly implement the toggle/repeat handling and it forces the use of a small, fixed-size keymap. The same problem existed when I improved the IR functionality in drivers/media/dvb/ttpci/budget-ci.c by the way, so a generic version could probably be added to ir-functions in the future. >> static ssize_t >> wbcir_show_last_scancode(struct device *dev, >> struct device_attribute *attr, char *buf) >> { >> struct acpi_device *device = container_of(dev, struct >> acpi_device, dev); struct wbcir_data *data = acpi_driver_data(device); >> return sprintf(buf, "0x%08X\n", data->last_scancode); >> } >> >> static struct device_attribute dev_attr_last_scancode = { >> .attr = { >> .name = "last_scancode", >> .mode = 0444, >> }, >> .show = wbcir_show_last_scancode, >> .store = NULL, >> >> }; >> >> static struct attribute *wbcir_attributes[] = { >> &dev_attr_last_scancode.attr, >> NULL, >> }; >> >> static struct attribute_group wbcir_attribute_group = { >> .attrs = wbcir_attributes, >> }; > > Are these just for debugging? If so, you could put them in debugfs > instead... No, they are there to help the user when generating a keymap for an unknown remote. Press key on remote, read value from /sys/.../last_scancode, add line saying "0x12345678 = KEY_EXPLODE" to keymap file, repeat...there aren't any user-friendly tools for this yet though. (Dropped Terry from the CC, I just saw that he had requested a driver for this chip earlier but I'm not sure he's that interested in the rest of the discussion) -- David Härdeman -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
