Messages in this thread | | | Date | Tue, 18 Apr 2006 15:54:27 -0700 | From | Patrick Mochel <> | Subject | Re: [patch 1/3] acpi: dock driver |
| |
On Tue, Apr 18, 2006 at 11:03:16AM -0700, Kristen Accardi wrote: > Create a driver which lives in the acpi subsystem to handle dock events. This > driver is not an acpi driver, because acpi drivers require that the object > be present when the driver is loaded.
A few comments..
> --- /dev/null > +++ 2.6-git-kca2/drivers/acpi/dock.c > @@ -0,0 +1,652 @@
> +#define ACPI_DOCK_COMPONENT 0x10000000 > +#define ACPI_DOCK_DRIVER_NAME "ACPI Dock Station Driver" > +#define _COMPONENT ACPI_DOCK_COMPONENT
These aren't necessary for code that is outside of the ACPI-CA.
> +struct dock_station { > + acpi_handle handle; > + unsigned long last_dock_time; > + u32 flags; > + spinlock_t dd_lock; > + spinlock_t hp_lock; > + struct list_head dependent_devices; > + struct list_head hotplug_devices; > +}; > + > +struct dock_dependent_device { > + struct list_head list; > + struct list_head hotplug_list; > + acpi_handle handle; > + acpi_notify_handler handler; > + void *context; > +}; > + > +#define DOCK_DOCKING 0x00000001 > + > +static struct dock_station *dock_station;
Does this need to be dynamically allocated? Static initialization would be a bit cleaner and obviate the need for the NULL checks in several of the functions below.
> +/** > + * eject_dock - respond to a dock eject request > + * @ds: the dock station > + * > + * This is called after _DCK is called, to execute the dock station's > + * _EJ0 method. > + */ > +static void eject_dock(struct dock_station *ds) > +{ > + struct acpi_object_list arg_list; > + union acpi_object arg; > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + union acpi_object *obj; > + > + acpi_get_name(ds->handle, ACPI_FULL_PATHNAME, &buffer); > + obj = buffer.pointer; > + > + arg_list.count = 1; > + arg_list.pointer = &arg; > + arg.type = ACPI_TYPE_INTEGER; > + arg.integer.value = 1;
Minor nit (that is replicated in many of the ACPI drivers). This can be done by just describing the data better:
struct acpi_object arg = { .type = ACPI_TYPE_INTEGER, .integer = { .value = 1, }, }; struct acpi_object_list arg_list = { .count = 1, .pointer = &arg, };
...
In the long run, since the same exact code exists in dozens of places in the ACPI drivers, there should just be a helper for it. E.g.:
int ret; unsigned long value;
ret = acpi_get_int(ds->handle, "_EJO", &value); if (!ret) /* Use Value */ else /* Error */
...and get rid of the awkward object/object list handling.
> +static inline void begin_dock(struct dock_station *ds) > +{ > + ds->flags |= DOCK_DOCKING; > +} > + > +static inline void complete_dock(struct dock_station *ds) > +{ > + ds->flags &= ~(DOCK_DOCKING); > + ds->last_dock_time = jiffies; > +} > + > +/** > + * dock_in_progress - see if we are in the middle of handling a dock event > + * @ds: the dock station > + * > + * Sometimes while docking, false dock events can be sent to the driver > + * because good connections aren't made or some other reason. Ignore these > + * if we are in the middle of doing something. > + */ > +static int dock_in_progress(struct dock_station *ds) > +{ > + if ((ds->flags & DOCK_DOCKING) || > + time_before(jiffies, (ds->last_dock_time + HZ))) > + return 1; > + return 0; > +}
These seem racy. It seems the flag should should at least be an atomic_t. But, if it's that, then it might as well be a mutex, eh? And, if it's a mutex, then do we need the other spinlocks?
> +acpi_status > +register_hotplug_dock_device(acpi_handle handle, acpi_notify_handler handler, > + void *context)
If this is called from outside drivers/acpi/, you should return an int with a real errno value. The AE_* values shouldn't be used outside of the ACPI CA.
> +acpi_status unregister_hotplug_dock_device(acpi_handle handle)
Does unregister need to return an error?
Thanks,
Pat - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |