[patch] Re: HAL and hotplugging
Daniel Stone
daniel at fooishbar.org
Sun Mar 16 06:34:04 PDT 2008
On Sat, Mar 15, 2008 at 05:36:47PM -0400, Dustin Spicuzza wrote:
> Ok, this patch allows the HAL layer to send arbitrary options to the
> driver. I tested it using the evtouch driver that my touchscreen uses...
> but it'll most likely work with evdev or any other driver too. Test it
> and let me know.
>
> Also, I changed some of the output statements to ErrorF instead of
> DebugF, since I couldn't figure out how to display the DebugF
> statements, and it was annoying to not be able to see them (while right
> now my changes made it more verbose, at least it gives you the ability
> to figure out what isn't working, and that HAL is actually *trying* to
> do something, as opposed to how it was before. Theres probably a better
> solution...
>
> Also, I know strncasecmp isn't standard.. and I used it. Is that an issue?
When we hit a platform that doesn't have it, we can steal the BSD libc's
implementation. Until then, I wouldn't really worry -- I guess, just
add a configure check that looks for it and bombs out if not.
> - DebugF("[config/hal] removing device %s\n", dev->name);
> + /* this isn't an error, but the user should see this */
> + ErrorF("[config/hal] removing device %s\n", dev->name);
Hmm, do they really need the log filled with messages every time they
plug or unplug a device?
> + LibHalPropertySet *set = NULL;
> + char *psi_key = NULL, *tmp_val;
Please, no tabs. :)
> @@ -192,7 +200,7 @@ device_added(LibHalContext *hal_ctx, const char *udi)
> driver = get_prop_string(hal_ctx, udi, "input.x11_driver");
> path = get_prop_string(hal_ctx, udi, "input.device");
> if (!driver || !path) {
> - DebugF("[config/hal] no driver or path specified for %s\n", udi);
> + ErrorF("[config/hal] no driver or path specified for %s\n", udi);
Err, assuming you remove the capabilities check, this will trigger every
single time you hotplug any device that isn't an input device, e.g. a
thumbdrive, camera, external media, etc, etc.
> @@ -205,17 +213,22 @@ device_added(LibHalContext *hal_ctx, const char *udi)
> xkb_layout = get_prop_string(hal_ctx, udi, "input.xkb.layout");
> xkb_variant = get_prop_string(hal_ctx, udi, "input.xkb.variant");
> xkb_options = get_prop_string_array(hal_ctx, udi, "input.xkb.options");
> - }
> + }
Please don't add trailing whitespace.
> + /* most drivers use device.. not path. evdev uses both however, but the
> + * path version isn't documented apparently. support both for now. */
> add_option(&options, "path", path);
> + add_option(&options, "device", path);
Yeah. Given that there are so few hotplug-capable drivers, though
(evdev, evtouch, wacom, ... ?), I was hoping that we'd just be able to
change the drivers to take the slightly more explicit 'path'.
> + /* ok, grab any other options from hal.. iterate through all properties
> + * and lets see if any of them are options that we can add */
> + set = libhal_device_get_all_properties(hal_ctx, udi, &error);
Why not just fold all the above code into the iterator here, so we cut
down on round trips?
> + /* this isn't an error, but how else do you output something that the user can see? */
> + ErrorF("[config/hal]: Adding input device %s\n", name);
Yeah, ErrorF is a pretty spectacular misnomer.
> + <!-- The way this works:
Thanks for the commenting! :)
Looks pretty much good other than the above comments.
Cheers,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg/attachments/20080316/aa6acf23/attachment.pgp>
More information about the xorg
mailing list