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

[Kristen Accardi <kristen.c.accardi@intel.com>]

On Wed, 2006-04-12 at 22:27 -0700, Andrew Morton wrote:
> Kristen Accardi <kristen.c.accardi@intel.com> 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.
> > 
> > ...
> >
> > +/**
> > + * add_dock_dependent_device - associate a device with the dock station
> > + * @ds: The dock station
> > + * @dd: The dependent device
> > + *
> > + * Add the dependent device to the dock's dependent device list.
> > + */
> > +static void
> > +add_dock_dependent_device(struct dock_station *ds,
> > +			  struct dock_dependent_device *dd)
> > +{
> > +	list_add_tail(&dd->list, &ds->dependent_devices);
> > +}
> > +
> 
> Does this not need any locking?

yes, I'll fix this.

> 
> > +/**
> > + * find_dock_dependent_device - get a device dependent on this dock
> > + * @ds: the dock station
> > + * @handle: the acpi_handle of the device we want
> > + *
> > + * iterate over the dependent device list for this dock.  If the
> > + * dependent device matches the handle, return.
> > + */
> > +static struct dock_dependent_device *
> > +find_dock_dependent_device(struct dock_station *ds, acpi_handle handle)
> > +{
> > +	struct dock_dependent_device *dd;
> > +
> > +	list_for_each_entry(dd, &ds->dependent_devices, list) {
> > +		if (handle == dd->handle)
> > +			return dd;
> > +	}
> > +	return NULL;
> > +}
> 
> Nor this?

yes, I'll fix this too.

> 
> > +
> > +
> > +
> > +
> 
> The driver has a lot of blank lines between functions.  I don't think this
> adds any benefit - it just makes less of the code visible.
> 
> > +EXPORT_SYMBOL_GPL(is_dock_device);
> 
> I assume all these exports are used?
> 

Yes, they are.

> > +
> > +
> > +
> > +
> > +/**
> > + * hotplug_devices - insert or remove devices on the dock station
> > + * @ds: the dock station
> > + * @event: either bus check or eject request
> > + *
> > + * Some devices on the dock station need to have drivers called
> > + * to perform hotplug operations after a dock event has occurred.
> > + * Traverse the list of dock devices that have registered a
> > + * hotplug handler, and call the handler.
> > + */
> > +static void hotplug_devices(struct dock_station *ds, u32 event)
> > +{
> > +	struct dock_dependent_device *dd;
> > +
> > +	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list) {
> > +		if (dd->handler)
> > +			dd->handler(dd->handle, event, dd->context);
> > +	}
> > +}
> > +
> > +
> > +
> > +
> 
> There's a reasonable chance that someone else will choose identifiers such
> as `hotplug_devices' and `hotplug_list'.  Let's hope they're not put in a
> header file which gets included here...
> 

No, they aren't included, however, I'll change the function name just in
case the function is ever exported.

> 
> > +/**
> > + * 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 ||
> > +		(jiffies < (ds->last_dock_time + 10))) {
> 
> Peculiar mixture of paranoid and trusting parenthesisation there..
> 
> It'll malfunction if jiffies happens to wrap - use time_before() or
> time_after() to fix.
> 

Thanks, I'll fix this.

> > +static void acpi_dock_notify(acpi_handle handle, u32 event, void *data)
> > +{
> > +	struct dock_station *ds = (struct dock_station *)data;
> > +	struct acpi_device *device;
> > +
> > +	ACPI_FUNCTION_TRACE("acpi_dock_notify");
> > +
> > +	switch (event) {
> > +		case ACPI_NOTIFY_BUS_CHECK:
> 
> We normally indent thusly:
> 
> 	switch (event) {
> 	case ACPI_NOTIFY_BUS_CHECK:
> 
> > +			if (!dock_in_progress(ds) && dock_present(ds)) {
> > +				begin_dock(ds);
> > +				dock(ds);
> > +				if (!dock_present(ds)) {
> > +					printk(KERN_ERR PREFIX "Unable to dock!\n");
> > +					break;
> > +				}
> > +				atomic_notifier_call_chain(&dock_notifier_list,
> > +					 event, NULL);
> > +				hotplug_devices(ds, event);
> > +				complete_dock(ds);
> > +				if (acpi_bus_get_device(ds->handle, &device))
> > +					acpi_bus_generate_event(device,
> > +						event, 0);
> > +			}
> 
> and if you do that here, this code will look nicer.
> 
> > +
> > +/**
> > + * acpi_dock_add - add a new dock station
> > + * @handle: the dock station handle
> > + *
> > + * allocated and initialize a new dock station device.  Find all devices
> > + * that are on the dock station, and register for dock event notifications.
> > + */
> > +static int acpi_dock_add(acpi_handle handle)
> > +{
> > +	int ret;
> > +	acpi_status status;
> > +
> > +	ACPI_FUNCTION_TRACE("acpi_dock_add");
> > +
> > +	/* allocate & initialize the dock_station private data */
> > +	ds = kzalloc(sizeof(*ds), GFP_KERNEL);
> 
> <wonders what ds is>
> 
> Oh, it's a file-wide `struct dock_station *'.
> 
> Suggest that it be given a more file-widey name.
> 
> Would it be better if `ds' be defined at compile time?  Perhaps not..
> 

Probably not.

> > +	if (!ds)
> > +		return_VALUE(-ENOMEM);
> > +	ds->handle = handle;
> > +	INIT_LIST_HEAD(&ds->dependent_devices);
> > +	INIT_LIST_HEAD(&ds->hotplug_devices);
> > +
> > +	/* Find dependent devices */
> > +	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> > +			ACPI_UINT32_MAX, find_dock_devices, ds, NULL);
> > +
> > +	/* register for dock events */
> > +	status = acpi_install_notify_handler(ds->handle, ACPI_SYSTEM_NOTIFY,
> > +				acpi_dock_notify, ds);
> > +	if (ACPI_FAILURE(status)) {
> > +		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
> > +			"Error installing notify handler\n"));
> > +		ret = -ENODEV;
> > +		goto dock_add_err;
> > +	}
> > +
> > +	printk(KERN_INFO PREFIX "%s \n", ACPI_DOCK_DRIVER_NAME);
> > +
> > +	return_VALUE(0);
> > +dock_add_err:
> > +	kfree(ds);
> > +	return_VALUE(ret);
> > +}
> > +
> >
> > ...
> >
> > --- 2.6-git-kca2.orig/drivers/acpi/Kconfig
> > +++ 2.6-git-kca2/drivers/acpi/Kconfig
> > @@ -134,6 +134,12 @@ config ACPI_FAN
> >  	  This driver adds support for ACPI fan devices, allowing user-mode 
> >  	  applications to perform basic fan control (on, off, status).
> >  
> > +config ACPI_DOCK
> > +	tristate "Dock"
> > +	default y
> > +	help
> > +	  This driver adds support for ACPI controlled docking stations
> 
> It doesn't depend upon anything else?

It doesn't have to.  If you want to be able to hotplug any PCI devices
on the dock station, then it does depend on HOTPLUG_PCI, however, there
are dock stations which have no PCI devices on them which would not need
to do this.

I'll send a new version which incorporates your feedback soon.  Thanks
for reviewing, I really appreciate it.

Kristen