On Sun, 2011-01-02 at 00:19 -0800, ext Dmitry Torokhov wrote:
Hi Tapio,
Hi Dmitry
Thank you for good comments. I sent v3 patch set right after this email.
--------8<-------
Please keep Makefile and Kconfig sorted alphabetically.
Corrected in patch set.
--------8<-------
Consider switching to sparse keymap library. You won't be needing ugly KEY_MAX + SW_XXX hacks and it will also support remapping keys from userspace.
Much nicer solution, thank you.
--------8<-------
Do not break strings on 80 column boundaries, wither align the arguments differently or just go past 80 columns, like this:
dev_err(eci->dev, "Unable to control headset microphone\n");
Corrected in patch set.
--------8<-------
+#define eci_initialize_debugfs(eci) 1
Should be 0 I think, and not have a parameter, preferably
static inline int eci_initialize_debugfs(void) { return 0; }
I take this static inline solution, but I needed the parameter.
--------8<-------
struct eci_mem_block *ekey = (void *)buf;int ret;/* Read always four bytes */ret = eci->eci_hw_ops->acc_read_direct(0, buf);if (ret)return ret;if (ekey->id != ECI_EKEY_BLOCK_ID)return -ENODEV;*key = cpu_to_be16(ekey->size);cpu_to_be16()??? This looks really wierd.
The ECI specification says that data in ECI accessory's memory is in big endian order. This simply get 16-bit size parameter right in any endianes machine.
--------8<-------
+static ssize_t show_cable_plugged(struct device *dev,
struct device_attribute *attr, char *buf)+{
struct eci_data *eci = dev_get_drvdata(dev);return snprintf(buf, sizeof(buf), "Cable plugged %s\n",eci->plugged ? "in" : "out");Should it be dsimple 0/1? How is this attribute supposed to be used?
+static ssize_t store_cable_plugged(struct device *dev,
struct device_attribute *attr,const char *buf, size_t len)+{
--------8<-------
Same here. Should it be in debugfs probably?
I made it now as 0/1. It's in sysfs as this is user space's interface for ECI acessories inserting.
The other way is via ALSA sound driver providing jack detection, but it's not there yet.
--------8<-------
/** Configure ECI buttons now as we have after parsed* enchancement features table*/I do not understand this comment.
I have loose some word somewhere... Now it goes: /* * Configure ECI buttons now as we know how after * enchancement features table has been parsed */
--------8<-------
set_bit(EV_KEY, eci->acc_input->evbit);set_bit(EV_SW, eci->acc_input->evbit);set_bit(EV_REP, eci->acc_input->evbit);__set_bit(), no neded to lock bus.
Corrected in patch set.
--------8<-------
+static struct miscdevice eci_device = {
.minor = MISC_DYNAMIC_MINOR,.name = ECI_DRIVERNAME,+};
What does this device do?
Nothing, and I can't even remember why I have put it there first place. Removed now.
--------8<-------
Thank you.
-- Dmitry
Thank you, now driver is better.