Re: [patch 1/3] acpi: dock driver

[Patrick Mochel <mochel@linux.intel.com>]

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