[PATCH 0/6] ACPI: dock: code hygiene

[PATCH 0/6] ACPI: dock: code hygiene

[Alex Chiang]

While poking around in the dock driver debugging the NULL ptr that
Rafael fixed, I found the driver a little hard to read.

Here is a modest attempt to clean it up a little, and is intended as
2.6.33 material.

Compile-tested only. I have access to an HP nc6220 and nc6230, but neither
of them seem to provide the _DCK method, which is needed for the dock
driver.

Thanks.

/ac

---

Alex Chiang (6):
      ACPI: dock: clean up error handling paths in dock_add()
      ACPI: dock: rename local variable 'dock_station' in dock_add()
      ACPI: dock: clean up one more error path in dock_add()
      ACPI: dock: add struct dock_station * directly to platform device data
      ACPI: dock: combine add|alloc_dock_dependent_device
      ACPI: dock: minor whitespace and style cleanups


 drivers/acpi/dock.c |  259 ++++++++++++++++++++-------------------------------
 1 files changed, 104 insertions(+), 155 deletions(-)

[PATCH 1/6] ACPI: dock: clean up error handling paths in dock_add()

[Alex Chiang]

Remove some copy/paste code in our error handling paths, which makes
the function smaller and slightly easier to read.

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |   66 +++++++++++++++++++--------------------------------
 1 files changed, 24 insertions(+), 42 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 7338b6a..a8f81c0 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -987,46 +987,24 @@ static int dock_add(acpi_handle handle)
 		dock_station->flags |= DOCK_IS_BAT;
 
 	ret = device_create_file(&dock_device->dev, &dev_attr_docked);
-	if (ret) {
-		printk(KERN_ERR "Error %d adding sysfs file\n", ret);
-		platform_device_unregister(dock_device);
-		kfree(dock_station);
-		dock_station = NULL;
-		return ret;
-	}
+	if (ret)
+		goto err_unregister;
+
 	ret = device_create_file(&dock_device->dev, &dev_attr_undock);
-	if (ret) {
-		printk(KERN_ERR "Error %d adding sysfs file\n", ret);
-		device_remove_file(&dock_device->dev, &dev_attr_docked);
-		platform_device_unregister(dock_device);
-		kfree(dock_station);
-		dock_station = NULL;
-		return ret;
-	}
+	if (ret)
+		goto err_unregister1;
+
 	ret = device_create_file(&dock_device->dev, &dev_attr_uid);
-	if (ret) {
-		printk(KERN_ERR "Error %d adding sysfs file\n", ret);
-		device_remove_file(&dock_device->dev, &dev_attr_docked);
-		device_remove_file(&dock_device->dev, &dev_attr_undock);
-		platform_device_unregister(dock_device);
-		kfree(dock_station);
-		dock_station = NULL;
-		return ret;
-	}
+	if (ret)
+		goto err_unregister2;
+
 	ret = device_create_file(&dock_device->dev, &dev_attr_flags);
-	if (ret) {
-		printk(KERN_ERR "Error %d adding sysfs file\n", ret);
-		device_remove_file(&dock_device->dev, &dev_attr_docked);
-		device_remove_file(&dock_device->dev, &dev_attr_undock);
-		device_remove_file(&dock_device->dev, &dev_attr_uid);
-		platform_device_unregister(dock_device);
-		kfree(dock_station);
-		dock_station = NULL;
-		return ret;
-	}
+	if (ret)
+		goto err_unregister3;
+
 	ret = device_create_file(&dock_device->dev, &dev_attr_type);
 	if (ret)
-		printk(KERN_ERR"Error %d adding sysfs file\n", ret);
+		goto err_unregister4;
 
 	/* Find dependent devices */
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
@@ -1036,10 +1014,8 @@ static int dock_add(acpi_handle handle)
 	/* add the dock station as a device dependent on itself */
 	dd = alloc_dock_dependent_device(handle);
 	if (!dd) {
-		kfree(dock_station);
-		dock_station = NULL;
 		ret = -ENOMEM;
-		goto dock_add_err_unregister;
+		goto err_unregister5;
 	}
 	add_dock_dependent_device(dock_station, dd);
 
@@ -1047,12 +1023,18 @@ static int dock_add(acpi_handle handle)
 	list_add(&dock_station->sibling, &dock_stations);
 	return 0;
 
-dock_add_err_unregister:
+err_unregister5:
 	device_remove_file(&dock_device->dev, &dev_attr_type);
-	device_remove_file(&dock_device->dev, &dev_attr_docked);
-	device_remove_file(&dock_device->dev, &dev_attr_undock);
-	device_remove_file(&dock_device->dev, &dev_attr_uid);
+err_unregister4:
 	device_remove_file(&dock_device->dev, &dev_attr_flags);
+err_unregister3:
+	device_remove_file(&dock_device->dev, &dev_attr_uid);
+err_unregister2:
+	device_remove_file(&dock_device->dev, &dev_attr_undock);
+err_unregister1:
+	device_remove_file(&dock_device->dev, &dev_attr_docked);
+err_unregister:
+	printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
 	platform_device_unregister(dock_device);
 	kfree(dock_station);
 	dock_station = NULL;

[PATCH 2/6] ACPI: dock: rename local variable 'dock_station' in dock_add()

[Alex Chiang]

Shorten the name of 'dock_station' to 'ds'.

No functional change.

It will help later on with some whitespace and legibility issues.

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |   46 +++++++++++++++++++++++-----------------------
 1 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index a8f81c0..390adee 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -947,44 +947,44 @@ static int dock_add(acpi_handle handle)
 {
 	int ret;
 	struct dock_dependent_device *dd;
-	struct dock_station *dock_station;
+	struct dock_station *ds;
 	struct platform_device *dock_device;
 
 	/* allocate & initialize the dock_station private data */
-	dock_station = kzalloc(sizeof(*dock_station), GFP_KERNEL);
-	if (!dock_station)
+	ds = kzalloc(sizeof(struct dock_station), GFP_KERNEL);
+	if (!ds)
 		return -ENOMEM;
-	dock_station->handle = handle;
-	dock_station->last_dock_time = jiffies - HZ;
-	INIT_LIST_HEAD(&dock_station->dependent_devices);
-	INIT_LIST_HEAD(&dock_station->hotplug_devices);
-	INIT_LIST_HEAD(&dock_station->sibling);
-	spin_lock_init(&dock_station->dd_lock);
-	mutex_init(&dock_station->hp_lock);
+	ds->handle = handle;
+	ds->last_dock_time = jiffies - HZ;
+	INIT_LIST_HEAD(&ds->dependent_devices);
+	INIT_LIST_HEAD(&ds->hotplug_devices);
+	INIT_LIST_HEAD(&ds->sibling);
+	spin_lock_init(&ds->dd_lock);
+	mutex_init(&ds->hp_lock);
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 
 	/* initialize platform device stuff */
-	dock_station->dock_device =
+	ds->dock_device =
 		platform_device_register_simple(dock_device_name,
 			dock_station_count, NULL, 0);
-	dock_device = dock_station->dock_device;
+	dock_device = ds->dock_device;
 	if (IS_ERR(dock_device)) {
-		kfree(dock_station);
-		dock_station = NULL;
+		kfree(ds);
+		ds = NULL;
 		return PTR_ERR(dock_device);
 	}
-	platform_device_add_data(dock_device, &dock_station,
+	platform_device_add_data(dock_device, &ds,
 		sizeof(struct dock_station *));
 
 	/* we want the dock device to send uevents */
 	dev_set_uevent_suppress(&dock_device->dev, 0);
 
 	if (is_dock(handle))
-		dock_station->flags |= DOCK_IS_DOCK;
+		ds->flags |= DOCK_IS_DOCK;
 	if (is_ata(handle))
-		dock_station->flags |= DOCK_IS_ATA;
+		ds->flags |= DOCK_IS_ATA;
 	if (is_battery(handle))
-		dock_station->flags |= DOCK_IS_BAT;
+		ds->flags |= DOCK_IS_BAT;
 
 	ret = device_create_file(&dock_device->dev, &dev_attr_docked);
 	if (ret)
@@ -1008,7 +1008,7 @@ static int dock_add(acpi_handle handle)
 
 	/* Find dependent devices */
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-			    ACPI_UINT32_MAX, find_dock_devices, dock_station,
+			    ACPI_UINT32_MAX, find_dock_devices, ds,
 			    NULL);
 
 	/* add the dock station as a device dependent on itself */
@@ -1017,10 +1017,10 @@ static int dock_add(acpi_handle handle)
 		ret = -ENOMEM;
 		goto err_unregister5;
 	}
-	add_dock_dependent_device(dock_station, dd);
+	add_dock_dependent_device(ds, dd);
 
 	dock_station_count++;
-	list_add(&dock_station->sibling, &dock_stations);
+	list_add(&ds->sibling, &dock_stations);
 	return 0;
 
 err_unregister5:
@@ -1036,8 +1036,8 @@ err_unregister1:
 err_unregister:
 	printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
 	platform_device_unregister(dock_device);
-	kfree(dock_station);
-	dock_station = NULL;
+	kfree(ds);
+	ds = NULL;
 	return ret;
 }
 

[PATCH 3/6] ACPI: dock: clean up one more error path in dock_add()

[Alex Chiang]

Remove one final redundant error handling path and reduce repetitiveness.

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 390adee..1d693a1 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -968,11 +968,10 @@ static int dock_add(acpi_handle handle)
 		platform_device_register_simple(dock_device_name,
 			dock_station_count, NULL, 0);
 	dock_device = ds->dock_device;
-	if (IS_ERR(dock_device)) {
-		kfree(ds);
-		ds = NULL;
-		return PTR_ERR(dock_device);
-	}
+	ret = IS_ERR(dock_device) ? PTR_ERR(dock_device) : 0;
+	if (ret)
+		goto err_free;
+
 	platform_device_add_data(dock_device, &ds,
 		sizeof(struct dock_station *));
 
@@ -1036,6 +1035,7 @@ err_unregister1:
 err_unregister:
 	printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
 	platform_device_unregister(dock_device);
+err_free:
 	kfree(ds);
 	ds = NULL;
 	return ret;

[PATCH 4/6] ACPI: dock: add struct dock_station * directly to platform device data

[Alex Chiang]

Instead of adding a (struct dock_station **) to our dock device's
platform data, we can add the (struct dock_station *) directly.

This change saves us some silly casting.

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 1d693a1..e53d49b 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -857,8 +857,7 @@ static ssize_t show_docked(struct device *dev,
 {
 	struct acpi_device *tmp;
 
-	struct dock_station *dock_station = *((struct dock_station **)
-		dev->platform_data);
+	struct dock_station *dock_station = dev->platform_data;
 
 	if (ACPI_SUCCESS(acpi_bus_get_device(dock_station->handle, &tmp)))
 		return snprintf(buf, PAGE_SIZE, "1\n");
@@ -872,8 +871,7 @@ static DEVICE_ATTR(docked, S_IRUGO, show_docked, NULL);
 static ssize_t show_flags(struct device *dev,
 			  struct device_attribute *attr, char *buf)
 {
-	struct dock_station *dock_station = *((struct dock_station **)
-		dev->platform_data);
+	struct dock_station *dock_station = dev->platform_data;
 	return snprintf(buf, PAGE_SIZE, "%d\n", dock_station->flags);
 
 }
@@ -886,8 +884,7 @@ static ssize_t write_undock(struct device *dev, struct device_attribute *attr,
 			   const char *buf, size_t count)
 {
 	int ret;
-	struct dock_station *dock_station = *((struct dock_station **)
-		dev->platform_data);
+	struct dock_station *dock_station = dev->platform_data;
 
 	if (!count)
 		return -EINVAL;
@@ -905,8 +902,7 @@ static ssize_t show_dock_uid(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
 	unsigned long long lbuf;
-	struct dock_station *dock_station = *((struct dock_station **)
-		dev->platform_data);
+	struct dock_station *dock_station = dev->platform_data;
 	acpi_status status = acpi_evaluate_integer(dock_station->handle,
 					"_UID", NULL, &lbuf);
 	if (ACPI_FAILURE(status))
@@ -919,8 +915,7 @@ static DEVICE_ATTR(uid, S_IRUGO, show_dock_uid, NULL);
 static ssize_t show_dock_type(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct dock_station *dock_station = *((struct dock_station **)
-		dev->platform_data);
+	struct dock_station *dock_station = dev->platform_data;
 	char *type;
 
 	if (dock_station->flags & DOCK_IS_DOCK)
@@ -972,8 +967,8 @@ static int dock_add(acpi_handle handle)
 	if (ret)
 		goto err_free;
 
-	platform_device_add_data(dock_device, &ds,
-		sizeof(struct dock_station *));
+	platform_device_add_data(dock_device, ds,
+		sizeof(struct dock_station));
 
 	/* we want the dock device to send uevents */
 	dev_set_uevent_suppress(&dock_device->dev, 0);

[PATCH 5/6] ACPI: dock: combine add|alloc_dock_dependent_device

[Alex Chiang]

There's no real need to have a separate allocation step when adding
a dock dependent device.

Combining the two functions is both logical and helps with legibility.

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |   55 ++++++++++++++++++---------------------------------
 1 files changed, 19 insertions(+), 36 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index e53d49b..ce36b6c 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -93,40 +93,30 @@ struct dock_dependent_device {
  *                         Dock Dependent device functions                   *
  *****************************************************************************/
 /**
- *  alloc_dock_dependent_device - allocate and init a dependent device
- *  @handle: the acpi_handle of the dependent device
+ * add_dock_dependent_device - associate a device with the dock station
+ * @handle: handle of the dependent device
+ * @ds: The dock station
  *
- *  Allocate memory for a dependent device structure for a device referenced
- *  by the acpi handle
+ * Add the dependent device to the dock's dependent device list.
  */
-static struct dock_dependent_device *
-alloc_dock_dependent_device(acpi_handle handle)
+static int
+add_dock_dependent_device(acpi_handle handle, struct dock_station *ds)
 {
 	struct dock_dependent_device *dd;
 
 	dd = kzalloc(sizeof(*dd), GFP_KERNEL);
-	if (dd) {
-		dd->handle = handle;
-		INIT_LIST_HEAD(&dd->list);
-		INIT_LIST_HEAD(&dd->hotplug_list);
-	}
-	return dd;
-}
+	if (!dd)
+		return -ENOMEM;
+
+	dd->handle = handle;
+	INIT_LIST_HEAD(&dd->list);
+	INIT_LIST_HEAD(&dd->hotplug_list);
 
-/**
- * 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)
-{
 	spin_lock(&ds->dd_lock);
 	list_add_tail(&dd->list, &ds->dependent_devices);
 	spin_unlock(&ds->dd_lock);
+
+	return 0;
 }
 
 /**
@@ -826,7 +816,6 @@ find_dock_devices(acpi_handle handle, u32 lvl, void *context, void **rv)
 	acpi_status status;
 	acpi_handle tmp, parent;
 	struct dock_station *ds = context;
-	struct dock_dependent_device *dd;
 
 	status = acpi_bus_get_ejd(handle, &tmp);
 	if (ACPI_FAILURE(status)) {
@@ -840,11 +829,9 @@ find_dock_devices(acpi_handle handle, u32 lvl, void *context, void **rv)
 			goto fdd_out;
 	}
 
-	if (tmp == ds->handle) {
-		dd = alloc_dock_dependent_device(handle);
-		if (dd)
-			add_dock_dependent_device(ds, dd);
-	}
+	if (tmp == ds->handle)
+		add_dock_dependent_device(ds, handle);
+
 fdd_out:
 	return AE_OK;
 }
@@ -941,7 +928,6 @@ static DEVICE_ATTR(type, S_IRUGO, show_dock_type, NULL);
 static int dock_add(acpi_handle handle)
 {
 	int ret;
-	struct dock_dependent_device *dd;
 	struct dock_station *ds;
 	struct platform_device *dock_device;
 
@@ -1006,12 +992,9 @@ static int dock_add(acpi_handle handle)
 			    NULL);
 
 	/* add the dock station as a device dependent on itself */
-	dd = alloc_dock_dependent_device(handle);
-	if (!dd) {
-		ret = -ENOMEM;
+	ret = add_dock_dependent_device(ds, handle);
+	if (ret)
 		goto err_unregister5;
-	}
-	add_dock_dependent_device(ds, dd);
 
 	dock_station_count++;
 	list_add(&ds->sibling, &dock_stations);

[PATCH 6/6] ACPI: dock: minor whitespace and style cleanups

[Alex Chiang]

Removed some stray whitespaces
Added whitespace when needed for legibility
Removed unneeded curly braces
Removed useless void casts
Removed unnecessary local variable initialization
Renamed variables to help out with 80-column fixes

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |   79 ++++++++++++++++++++++-----------------------------
 1 files changed, 34 insertions(+), 45 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index ce36b6c..1598fb4 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -239,6 +239,7 @@ static int is_battery(acpi_handle handle)
 static int is_ejectable_bay(acpi_handle handle)
 {
 	acpi_handle phandle;
+
 	if (!is_ejectable(handle))
 		return 0;
 	if (is_battery(handle) || is_ata(handle))
@@ -265,14 +266,13 @@ int is_dock_device(acpi_handle handle)
 
 	if (is_dock(handle))
 		return 1;
-	list_for_each_entry(dock_station, &dock_stations, sibling) {
+
+	list_for_each_entry(dock_station, &dock_stations, sibling)
 		if (find_dock_dependent_device(dock_station, handle))
 			return 1;
-	}
 
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(is_dock_device);
 
 /**
@@ -295,8 +295,6 @@ static int dock_present(struct dock_station *ds)
 	return 0;
 }
 
-
-
 /**
  * dock_create_acpi_device - add new devices to acpi
  * @handle - handle of the device to add
@@ -310,7 +308,7 @@ static int dock_present(struct dock_station *ds)
  */
 static struct acpi_device * dock_create_acpi_device(acpi_handle handle)
 {
-	struct acpi_device *device = NULL;
+	struct acpi_device *device;
 	struct acpi_device *parent_device;
 	acpi_handle parent;
 	int ret;
@@ -327,8 +325,7 @@ static struct acpi_device * dock_create_acpi_device(acpi_handle handle)
 		ret = acpi_bus_add(&device, parent_device, handle,
 			ACPI_BUS_TYPE_DEVICE);
 		if (ret) {
-			pr_debug("error adding bus, %x\n",
-				-ret);
+			pr_debug("error adding bus, %x\n", -ret);
 			return NULL;
 		}
 	}
@@ -354,7 +351,6 @@ static void dock_remove_acpi_device(acpi_handle handle)
 	}
 }
 
-
 /**
  * hotplug_dock_devices - insert or remove devices on the dock station
  * @ds: the dock station
@@ -374,10 +370,9 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event)
 	/*
 	 * First call driver specific hotplug functions
 	 */
-	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list) {
+	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
 		if (dd->ops && dd->ops->handler)
 			dd->ops->handler(dd->handle, event, dd->context);
-	}
 
 	/*
 	 * Now make sure that an acpi_device is created for each
@@ -416,6 +411,7 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
 	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
 		if (dd->ops && dd->ops->uevent)
 			dd->ops->uevent(dd->handle, event, dd->context);
+
 	if (num != DOCK_EVENT)
 		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
 }
@@ -446,8 +442,8 @@ static void eject_dock(struct dock_station *ds)
 	arg.type = ACPI_TYPE_INTEGER;
 	arg.integer.value = 1;
 
-	if (ACPI_FAILURE(acpi_evaluate_object(ds->handle, "_EJ0",
-					      &arg_list, NULL)))
+	status = acpi_evaluate_object(ds->handle, "_EJ0", &arg_list, NULL);
+	if (ACPI_FAILURE(status))
 		pr_debug("Failed to evaluate _EJ0!\n");
 }
 
@@ -567,7 +563,6 @@ int register_dock_notifier(struct notifier_block *nb)
 
 	return atomic_notifier_chain_register(&dock_notifier_list, nb);
 }
-
 EXPORT_SYMBOL_GPL(register_dock_notifier);
 
 /**
@@ -581,7 +576,6 @@ void unregister_dock_notifier(struct notifier_block *nb)
 
 	atomic_notifier_chain_unregister(&dock_notifier_list, nb);
 }
-
 EXPORT_SYMBOL_GPL(unregister_dock_notifier);
 
 /**
@@ -626,7 +620,6 @@ register_hotplug_dock_device(acpi_handle handle, struct acpi_dock_ops *ops,
 
 	return ret;
 }
-
 EXPORT_SYMBOL_GPL(register_hotplug_dock_device);
 
 /**
@@ -647,7 +640,6 @@ void unregister_hotplug_dock_device(acpi_handle handle)
 			dock_del_hotplug_device(dock_station, dd);
 	}
 }
-
 EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
 
 /**
@@ -762,7 +754,7 @@ struct dock_data {
 
 static void acpi_dock_deferred_cb(void *context)
 {
-	struct dock_data *data = (struct dock_data *)context;
+	struct dock_data *data = context;
 
 	dock_notify(data->handle, data->event, data->ds);
 	kfree(data);
@@ -772,23 +764,22 @@ static int acpi_dock_notifier_call(struct notifier_block *this,
 	unsigned long event, void *data)
 {
 	struct dock_station *dock_station;
-	acpi_handle handle = (acpi_handle)data;
+	acpi_handle handle = data;
 
 	if (event != ACPI_NOTIFY_BUS_CHECK && event != ACPI_NOTIFY_DEVICE_CHECK
 	   && event != ACPI_NOTIFY_EJECT_REQUEST)
 		return 0;
 	list_for_each_entry(dock_station, &dock_stations, sibling) {
 		if (dock_station->handle == handle) {
-			struct dock_data *dock_data;
+			struct dock_data *dd;
 
-			dock_data = kmalloc(sizeof(*dock_data), GFP_KERNEL);
-			if (!dock_data)
+			dd = kmalloc(sizeof(*dd), GFP_KERNEL);
+			if (!dd)
 				return 0;
-			dock_data->handle = handle;
-			dock_data->event = event;
-			dock_data->ds = dock_station;
-			acpi_os_hotplug_execute(acpi_dock_deferred_cb,
-				dock_data);
+			dd->handle = handle;
+			dd->event = event;
+			dd->ds = dock_station;
+			acpi_os_hotplug_execute(acpi_dock_deferred_cb, dd);
 			return 0 ;
 		}
 	}
@@ -935,8 +926,10 @@ static int dock_add(acpi_handle handle)
 	ds = kzalloc(sizeof(struct dock_station), GFP_KERNEL);
 	if (!ds)
 		return -ENOMEM;
+
 	ds->handle = handle;
 	ds->last_dock_time = jiffies - HZ;
+
 	INIT_LIST_HEAD(&ds->dependent_devices);
 	INIT_LIST_HEAD(&ds->hotplug_devices);
 	INIT_LIST_HEAD(&ds->sibling);
@@ -945,16 +938,15 @@ static int dock_add(acpi_handle handle)
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 
 	/* initialize platform device stuff */
-	ds->dock_device =
-		platform_device_register_simple(dock_device_name,
-			dock_station_count, NULL, 0);
+	ds->dock_device = platform_device_register_simple(dock_device_name,
+				dock_station_count, NULL, 0);
 	dock_device = ds->dock_device;
+
 	ret = IS_ERR(dock_device) ? PTR_ERR(dock_device) : 0;
 	if (ret)
 		goto err_free;
 
-	platform_device_add_data(dock_device, ds,
-		sizeof(struct dock_station));
+	platform_device_add_data(dock_device, ds, sizeof(struct dock_station));
 
 	/* we want the dock device to send uevents */
 	dev_set_uevent_suppress(&dock_device->dev, 0);
@@ -988,8 +980,7 @@ static int dock_add(acpi_handle handle)
 
 	/* Find dependent devices */
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-			    ACPI_UINT32_MAX, find_dock_devices, ds,
-			    NULL);
+			    ACPI_UINT32_MAX, find_dock_devices, ds, NULL);
 
 	/* add the dock station as a device dependent on itself */
 	ret = add_dock_dependent_device(ds, handle);
@@ -1022,18 +1013,17 @@ err_free:
 /**
  * dock_remove - free up resources related to the dock station
  */
-static int dock_remove(struct dock_station *dock_station)
+static int dock_remove(struct dock_station *ds)
 {
 	struct dock_dependent_device *dd, *tmp;
-	struct platform_device *dock_device = dock_station->dock_device;
+	struct platform_device *dock_device = ds->dock_device;
 
 	if (!dock_station_count)
 		return 0;
 
 	/* remove dependent devices */
-	list_for_each_entry_safe(dd, tmp, &dock_station->dependent_devices,
-				 list)
-	    kfree(dd);
+	list_for_each_entry_safe(dd, tmp, &ds->dependent_devices, list)
+		kfree(dd);
 
 	/* cleanup sysfs */
 	device_remove_file(&dock_device->dev, &dev_attr_type);
@@ -1044,8 +1034,8 @@ static int dock_remove(struct dock_station *dock_station)
 	platform_device_unregister(dock_device);
 
 	/* free dock station memory */
-	kfree(dock_station);
-	dock_station = NULL;
+	kfree(ds);
+	ds = NULL;
 	return 0;
 }
 
@@ -1063,11 +1053,10 @@ find_dock(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
 	acpi_status status = AE_OK;
 
-	if (is_dock(handle)) {
-		if (dock_add(handle) >= 0) {
+	if (is_dock(handle))
+		if (dock_add(handle) >= 0)
 			status = AE_CTRL_TERMINATE;
-		}
-	}
+
 	return status;
 }
 

Re: [PATCH 0/6] ACPI: dock: code hygiene

[Len Brown]

series applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center

On Wed, 7 Oct 2009, Alex Chiang wrote:

> While poking around in the dock driver debugging the NULL ptr that
> Rafael fixed, I found the driver a little hard to read.
> 
> Here is a modest attempt to clean it up a little, and is intended as
> 2.6.33 material.
> 
> Compile-tested only. I have access to an HP nc6220 and nc6230, but neither
> of them seem to provide the _DCK method, which is needed for the dock
> driver.
> 
> Thanks.
> 
> /ac
> 
> ---
> 
> Alex Chiang (6):
>       ACPI: dock: clean up error handling paths in dock_add()
>       ACPI: dock: rename local variable 'dock_station' in dock_add()
>       ACPI: dock: clean up one more error path in dock_add()
>       ACPI: dock: add struct dock_station * directly to platform device data
>       ACPI: dock: combine add|alloc_dock_dependent_device
>       ACPI: dock: minor whitespace and style cleanups
> 
> 
>  drivers/acpi/dock.c |  259 ++++++++++++++++++++-------------------------------
>  1 files changed, 104 insertions(+), 155 deletions(-)
> 

Re: [PATCH 4/6] ACPI: dock: add struct dock_station * directly to platform device data

[Shaohua Li]

On Thu, Oct 08, 2009 at 06:07:47AM +0800, Alex Chiang wrote:
> Instead of adding a (struct dock_station **) to our dock device's
> platform data, we can add the (struct dock_station *) directly.
> 
> This change saves us some silly casting.
> 
> Signed-off-by: Alex Chiang <achiang@hp.com>
> ---
> 
>  drivers/acpi/dock.c |   19 +++++++------------
>  1 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index 1d693a1..e53d49b 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -857,8 +857,7 @@ static ssize_t show_docked(struct device *dev,
>  {
>  	struct acpi_device *tmp;
>  
> -	struct dock_station *dock_station = *((struct dock_station **)
> -		dev->platform_data);
> +	struct dock_station *dock_station = dev->platform_data;
>  
>  	if (ACPI_SUCCESS(acpi_bus_get_device(dock_station->handle, &tmp)))
>  		return snprintf(buf, PAGE_SIZE, "1\n");
> @@ -872,8 +871,7 @@ static DEVICE_ATTR(docked, S_IRUGO, show_docked, NULL);
>  static ssize_t show_flags(struct device *dev,
>  			  struct device_attribute *attr, char *buf)
>  {
> -	struct dock_station *dock_station = *((struct dock_station **)
> -		dev->platform_data);
> +	struct dock_station *dock_station = dev->platform_data;
>  	return snprintf(buf, PAGE_SIZE, "%d\n", dock_station->flags);
>  
>  }
> @@ -886,8 +884,7 @@ static ssize_t write_undock(struct device *dev, struct device_attribute *attr,
>  			   const char *buf, size_t count)
>  {
>  	int ret;
> -	struct dock_station *dock_station = *((struct dock_station **)
> -		dev->platform_data);
> +	struct dock_station *dock_station = dev->platform_data;
>  
>  	if (!count)
>  		return -EINVAL;
> @@ -905,8 +902,7 @@ static ssize_t show_dock_uid(struct device *dev,
>  			     struct device_attribute *attr, char *buf)
>  {
>  	unsigned long long lbuf;
> -	struct dock_station *dock_station = *((struct dock_station **)
> -		dev->platform_data);
> +	struct dock_station *dock_station = dev->platform_data;
>  	acpi_status status = acpi_evaluate_integer(dock_station->handle,
>  					"_UID", NULL, &lbuf);
>  	if (ACPI_FAILURE(status))
> @@ -919,8 +915,7 @@ static DEVICE_ATTR(uid, S_IRUGO, show_dock_uid, NULL);
>  static ssize_t show_dock_type(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	struct dock_station *dock_station = *((struct dock_station **)
> -		dev->platform_data);
> +	struct dock_station *dock_station = dev->platform_data;
>  	char *type;
>  
>  	if (dock_station->flags & DOCK_IS_DOCK)
> @@ -972,8 +967,8 @@ static int dock_add(acpi_handle handle)
>  	if (ret)
>  		goto err_free;
>  
> -	platform_device_add_data(dock_device, &ds,
> -		sizeof(struct dock_station *));
> +	platform_device_add_data(dock_device, ds,
> +		sizeof(struct dock_station));

This patch isn't good. platform_device_add_data will malloc a new
'struct dock_station' and copy old to it, and later the two copy aren't
consistent. So NACK this one.

ACK other 5 patches.

Re: [PATCH 4/6] ACPI: dock: add struct dock_station * directly to platform device data

[Alex Chiang]

Hi Len,

* Shaohua Li <shaohua.li@intel.com>:
> 
> This patch isn't good. platform_device_add_data will malloc a new
> 'struct dock_station' and copy old to it, and later the two copy aren't
> consistent. So NACK this one.

If you can drop this patch without requiring a respin from me, go
ahead.

If you can't, go ahead and drop what you need, and I'll respin.

I'm going to chew on this a bit and see if I can still salvage
it.

> ACK other 5 patches.

Thanks, Shaohua.

/ac