Hi Aniroop,
On Fri, Jan 10, 2014 at 04:05:44PM +0000, Aniroop Mathur wrote:
> <HTML><HEAD><TITLE>Samsung Enterprise Portal mySingle</TITLE>
> <META content="text/html; charset=windows-1252" http-equiv=Content-Type>
> <STYLE id=mysingle_style type=text/css>P {
Please no HTML mail as mainling lists drop it.
Thanks.
> MARGIN-TOP: 5px; FONT-FAMILY: Arial, arial; MARGIN-BOTTOM: 5px; FONT-SIZE: 9pt
> }
> TD {
> MARGIN-TOP: 5px; FONT-FAMILY: Arial, arial; MARGIN-BOTTOM: 5px; FONT-SIZE: 9pt
> }
> LI {
> MARGIN-TOP: 5px; FONT-FAMILY: Arial, arial; MARGIN-BOTTOM: 5px; FONT-SIZE: 9pt
> }
> BODY {
> LINE-HEIGHT: 1.4; MARGIN: 10px; FONT-FAMILY: Arial, arial; FONT-SIZE: 9pt
> }
> </STYLE>
>
> <META name=GENERATOR content=ActiveSquare></HEAD>
> <BODY>
> <P>Hello Mr. Torokhov,</P>
> <P>Greetings!</P>
> <P><BR>On Thu, Jan 09, 2014 at 10:27:56AM +0530, Aniroop Mathur wrote:<BR>> This patch allows user(driver) to set sysfs node name of input<BR>> devices. To set sysfs node name, user(driver) just needs to set<BR>> node_name_unique variable. If node_name_unique is not set, default<BR>> name is given(as before). So, this patch is completely<BR>> backward-compatible.<BR>> <BR>> Sysfs Input node name format is: input_<NAME><BR>> Sysfs Event node name format is: event_<NAME><BR>><BR>> This "name" is given by user and automatically, prefix(input and<BR>> event) is added by input core.<BR>> <BR>> This name must be unique among all input devices and driver(user) has<BR>> the responsibility to ensure it. If same name is used again for other<BR>> input device, registration of that input device will fail because two<BR>> input devices cannot have same name.<BR>> <BR>> Advantages of this patch are:<BR>> <BR>> 1. Reduces Booting Time of HAL/Upper-Layer because now HAL or<BR>> Upper-Layer do not need to search input/event number corresponding to<BR>> each input device in /dev/input/... This searching in /dev/input/ was<BR>> taking too much time. (Especially in mobile devices, where there are<BR>> many input devices (many sensors, touchscreen, etc), it reduces a lot<BR>> of booting time)<BR><BR>I am sorry, how much time does it take to scan a directory of what, 20<BR>devices? If it such a factor have udev create nodes that are easier for<BR>you to locate, similarly how we already create nodes by-id and by-path.<BR>For example you can encode major:minor in device name.</P>
> <P> </P>
> <P><STRONG><SPAN style="FONT-SIZE: 11pt">Re: (Aniroop Mathur)</SPAN></STRONG></P>
> <P>Its correct that we can set name of a device node using udev. <BR>Yes, this will change the name of device node(/dev/...) but not sysfs node.(/sys/class/input/...)<BR><STRONG>So now, the problem area will shift from dev path to sysfs path, <BR></STRONG>because now we dont know which sysfs node to refer for a particular input device<BR>and hence HAL/Upper-Layer will need to search in /sys/class/input/... instead of /dev/... directory.</P>
> <P> </P>
> <P>Moreover, as i know, udev is mainly for hot-pluggable devices, but my problem is for platform devices,<BR>which are already present on the board during boot up. (Like in Embedded devices)</P>
> <P> </P>
> <P>To avoid confusion and make the problem more clear, <BR>I would like to explain the problem and my suggestion by taking an example:</P>
> <P> </P>
> <P>Suppose in a mobile device, there are 10 embedded input devices as below:<BR><STRONG>Proximity --- /dev/input0 --- /sys/class/input/input0 --- /sys/class/input/event0<BR>Magnetometer --- /dev/input1 --- /sys/class/input/input1 --- /sys/class/input/event1<BR>Accelerometer --- /dev/input2 --- /sys/class/input/input2 --- /sys/class/input/event2<BR>Touchscreen --- /dev/input3 --- /sys/class/input/input3 --- /sys/class/input/event3 </STRONG></P>
> <P><STRONG>... 6 more like this<BR></STRONG>(All these are created during boot up time)</P>
> <P> </P>
> <P>Kernel has created all these nodes, so that HAL/UpperLayer can read or write values from it.<BR>HAL/Upper-Layer needs to do main tasks like:<BR>1. Read raw data - does through /dev/input<num><BR>2. Enable device - does through sys/class/input<num>/enable<BR>3. Set delay - does through sys/class/input<num>/delay<BR>and many more...</P>
> <P> </P>
> <P>Now, Lets suppose we need to do these tasks for Accelerometer.</P>
> <P>If dev node name is set, HAL can directly read value from it (no search required)<BR><STRONG>But for enabling the accelerometer device or set the delay of a hardware chip, <BR>there is no direct way, HAL can know which input node to refer for accelerometer<BR>because the input number is created dynamically as per device probe order,<BR>so this input number can be anything (0,1,2,3...)<BR></STRONG>So HAL will need to search every input node and read its name attribute<BR>and keep on searching until a match is found between the "attribute name" and "name passed as parameter". <BR>Like for accelerometer, this searching needs to be done for all other input devices.<BR>All of this part is done during booting and this takes a lot for time from booting perspective.</P>
> <P> </P>
> <P><STRONG>As I measured, if there are ten devices, it is taking 1 second to do all this searching. (for all devices)<BR>So for 20 devices, i guess, it could take upto 2 seconds.</STRONG></P>
> <P><STRONG></STRONG> </P>
> <P><STRONG>With naming convention, there is no need of search neither for dev path nor for sysfs path<BR>because HAL directly know which node to refer for which input device<BR>and hence this 1 second is reduced to 10ms or even less, therefore saving 990ms.<BR>I believe, this is a very good time saving. (from device booting perspective)</STRONG></P>
> <P> </P>
> <P>(Is there any direct way, without scanning all nodes for every input device ?)</P>
> <P><BR>> <BR>> 2. Improves Readabilty of input and event sysfs node paths because<BR>> names are used instead of numbers.<BR><BR>I do not see why it is that important. If one wants overview<BR>/proc/bus/input/devices gives nice picture.</P>
> <P> </P>
> <P><STRONG><SPAN style="FONT-SIZE: 11pt">Re: (Aniroop Mathur)</SPAN></STRONG></P>
> <P>Its correct, we can get an overview from /proc/bus/input/devices.<BR>And therefore using this, we can know input node number for every input device.<BR>But there are many input devices and input numbers are not fixed, <BR>so its quite difficult to memorize input number for all input devices.<BR><STRONG>Therefore, if a user needs to open some input node from sysfs path,<BR>he needs to check /proc/bus/input/devices before opening because <BR>he does not know the input number. Moreover, this applies for all other<BR>input devices and hence a user need to check this every time.</STRONG><STRONG><BR></P></STRONG>
> <P>It improves readabilty as below</P>
> <P> </P>
> <TABLE style="BACKGROUND-COLOR: #ffffff; WIDTH: 375px" border=1 cellSpacing=0 width=375>
> <TBODY>
> <TR>
> <TD height=26 width=151>
> <P> Before</P></TD>
> <TD height=26 width=214>
> <P> After Patch</P></TD></TR>
> <TR>
> <TD height=26 width=151>
> <P><STRONG>dev node paths:</STRONG></P>
> <P><STRONG></STRONG> </P>
> <P>/dev/input0</P>
> <P>/dev/input1</P>
> <P>... many more </P></TD>
> <TD height=26 width=214>
> <P><STRONG>dev node paths:</STRONG></P>
> <P><STRONG></STRONG> </P>
> <P>/dev/input_proximity</P>
> <P>/dev/input_accelerometer</P>
> <P>... many more </P></TD></TR>
> <TR>
> <TD height=26 width=151>
> <P><STRONG>sysfs node paths:</STRONG></P>
> <P><STRONG></STRONG> </P>
> <P>/sys/class/input/input0</P>
> <P>/sys/class/input/input1 </P>
> <P>... many more</P>
> <P> </P>
> <P>/sys/class/input/event0</P>
> <P>/sys/class/input/event1</P>
> <P>...many more</P></TD>
> <TD height=26 width=214>
> <P><STRONG>sysfs node paths:</STRONG></P>
> <P><STRONG></STRONG> </P>
> <P>/sys/class/input/input_proximity</P>
> <P>/sys/class/input/input_accelerometer</P>
> <P>... many more</P>
> <P> </P>
> <P>/sys/class/input/event_proximity</P>
> <P>/sys/class/input/event_accelerometer</P>
> <P>... many more</P></TD></TR></TBODY></TABLE>
> <P> </P>
> <P><STRONG>So, just by looking, user can directly open or refer any input node.<BR>(no need to refer any other path)</STRONG><STRONG></P>
> <P><BR></STRONG>> <BR>> 3. Removes Input Devices Dependency. If one input device probe fails,<BR>> other input devices still work. Before this patch, if one input<BR>> device probe fails before input_register_device, then input number of<BR>> other input devices changes and due to this permission settings are<BR>> disturbed and hence HAL or upper layer cannot open the required sysfs<BR>> node because permission denied error comes.<BR><BR>I have only one suggestion here: fix your userspace so that does not<BR>depend on device initialization ordering.<BR></P>
> <P><STRONG><SPAN style="FONT-SIZE: 11pt">Re: (Aniroop Mathur)</SPAN></STRONG></P>
> <P><STRONG>We cannot fix userspace because these input/event/dev number are decided/allocated in kernel<BR>as per device initialization ordering during boot up. (userspace has no role in it)<BR></STRONG>So, userspace is not aware, which exact input number corresponds to which input device <BR>so it ends up searching/scanning every input node untill a match is found.</P>
> <P>So, there is input device dependency which needs to be removed.</P>
> <P> </P>
> <P><BR>IOW I am totally unconvinced that this facility is needed.</P>
> <P><STRONG><SPAN style="FONT-SIZE: 11pt">Re: (Aniroop Mathur)</SPAN></STRONG></P>
> <P>I hope my problem and suggestion is more clear and convincing now.</P>
> <P> </P>
> <P> </P>
> <P><STRONG>For reference, copying Patch again:</STRONG></P>
> <P>---<BR>drivers/input/evdev.c | 11 ++++++++++-<BR>drivers/input/input.c | 12 +++++++++++-<BR>include/linux/input.h | 4 ++++<BR>3 files changed, 25 insertions(+), 2 deletions(-)<BR><BR>diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c<BR>index b6ded17..b6a5848 100644<BR>--- a/drivers/input/evdev.c<BR>+++ b/drivers/input/evdev.c<BR>@@ -1131,7 +1131,16 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,<BR>/* Normalize device number if it falls into legacy range */<BR>if (dev_no < EVDEV_MINOR_BASE + EVDEV_MINORS)<BR>dev_no -= EVDEV_MINOR_BASE;<BR>- dev_set_name(&evdev->dev, "event%d", dev_no);<BR>+<BR>+ /*<BR>+ * As per user choice (driver),<BR>+ * name of sysfs node is set, as mentioned in node_name_unique variable.<BR>+ * If node_name_unique is not set, default name is given.<BR>+ */<BR>+ if (dev->node_name_unique)<BR>+ dev_set_name(&evdev->dev, "event_%s", dev->node_name_unique);<BR>+ else<BR>+ dev_set_name(&evdev->dev, "event%d", dev_no);<BR><BR>evdev->handle.dev = input_get_device(dev);<BR>evdev->handle.name = dev_name(&evdev->dev);<BR>diff --git a/drivers/input/input.c b/drivers/input/input.c<BR>index c044699..c8126b3 100644<BR>--- a/drivers/input/input.c<BR>+++ b/drivers/input/input.c<BR>@@ -2073,7 +2073,17 @@ int input_register_device(struct input_dev *dev)<BR>if (!dev->setkeycode)<BR>dev->setkeycode = input_default_setkeycode;<BR><BR>- dev_set_name(&dev->dev, "input%ld",<BR>+ /*<BR>+ * As per user choice (driver),<BR>+ * name of sysfs node is set, as mentioned in node_name_unique variable.<BR>+ * If node_name_unique is not set, default name is given.<BR>+ */<BR>+ if (dev->node_name_unique) {<BR>+ atomic_inc_return(&input_no);<BR>+ dev_set_name(&dev->dev, "input_%s",<BR>+ dev->node_name_unique);<BR>+ } else<BR>+ dev_set_name(&dev->dev, "input%ld",<BR> (unsigned long) atomic_inc_return(&input_n
o) - 1);<BR><BR>error = device_add(&dev->dev);<BR>diff --git a/include/linux/input.h b/include/linux/input.h<BR>index 82ce323..fe44643 100644<BR>--- a/include/linux/input.h<BR>+++ b/include/linux/input.h<BR>@@ -43,6 +43,9 @@ struct input_value {<BR> * @uniq: unique identification code for the device (if device has it)<BR> * @id: id of the device (struct input_id)<BR> * @propbit: bitmap of device properties and quirks<BR>+ * @node_name_unique: name of input and event sysfs device node (char *).<BR>+ * This name must be unique among all input devices and driver(user)<BR>+ * has the responsibility to ensure it (if using).<BR> * @evbit: bitmap of types of events supported by the device (EV_KEY,<BR> * EV_REL, etc.)<BR> * @keybit: bitmap of keys/buttons this device has<BR>@@ -123,6 +126,7 @@ struct input_dev {<BR>const char *phys;<BR>const char *uniq;<BR>struct input_id id;<BR>+ char *node_name_unique;<BR><BR>unsigned long propbit[BITS_TO_LONGS(INPUT_PROP_CNT)];<BR><BR>-- <BR>1.7.0.4<BR><BR></P>
> <P>Thanks,</P>
> <P>Aniroop Mathur</P>
> <P> </P>
> <TABLE id=confidentialsignimg>
> <TBODY>
> <TR>
> <TD NAMO_LOCK>
> <P><IMG border=0 src="cid:QKNMBDIFBEI0@namo.co.kr" width=520></P></TD></TR></TBODY></TABLE></BODY></HTML><img src='http://ext.samsung.net/mailcheck/SeenTimeChecker?do=166e1c9f408a9352202b65e5c362a372ac21ff698029c65acf20909b63ed3e8c4e627947a285d7d76b91358bc02b4ef9c7b41e955949e5c8a728c55b39cc59eacf878f9a26ce15a0' border=0 width=0 height=0 style='display:none'>
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
