Re: [PATCH v2 1/4] mfd: add viperboard driver

[Lars-Peter Clausen <lars@metafoo.de>]

On 10/16/2012 11:43 AM, Lars Poeschel wrote:
> On Tuesday 16 October 2012 at 10:40:26, Lars-Peter Clausen wrote:
>> On 10/12/2012 04:34 PM, Lars Poeschel wrote:
>>> [...]
>>> +static void vprbrd_dev_release(struct device *dev)
>>> +{
>>> +	return;
>>
>> A empty release callback is usually a good indicator that something is
>> wrong. The release callback will be called once the last reference to the
>> device has been called, so the memory associated with the device should not
>> be freed before the release callback has been called, otherwise the memory
>> might be accessed after it has been freed...
>>
>>> +}
>>> +
>>> +static void vprbrd_free(struct vprbrd *dev)
>>> +{
>>> +	usb_put_dev(dev->usb_dev);
>>> +	kfree(dev);
>>
>> ..., so this kfree should be moved from here to the release callback.
> 
> Thank you for catching that one!
>  
>> Btw. I'm wondering why is the extra platform device required? Can't you not
>> just use the usb device as the parent device for the mfd cells?
> 
> This is what I first did, but this does not work. You can read about my first 
> thoughts why this is not working here: (To sum it up: The device is housed in 
> an usb_device, not a platform_device and This usb_device has no mfd_cell 
> member.)
> 
> https://lkml.org/lkml/2012/9/28/327
> 
> As I got a bit more deeper I also noticed, that mfd_add_devices (obviously) 
> adds the devices "as childs" to the parent device. mfd_remove_devices then 
> removes ALL "child" devices from the parent, not only those added by 
> mfd_add_devices before. This does not work in the case of the usb parent 
> device, because it has other childs that the usb layer added before (some 
> endpoints and stuff). So I had to construct an "empty" (in sense of childs) 
> mock platform_device between the usb and mfd.

Ah, ok that makes sense. I was a bit confused, because there are other mfd
drivers with for example i2c or spi devices as parents and these work fine,
but I guess this is because non of them registers any additional child
devices. I guess it makes sense to create a mfd cell device type and assign
this type to newly created mfd cells and only unregister a device in
mfd_remove_devices if it has the correct device type.

E.g. something along the lines of:


--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -21,6 +21,10 @@
 #include <linux/irqdomain.h>
 #include <linux/of.h>

+static struct device_type mfd_device_type = {
+	.name = "mfd-cell",
+};
+
 int mfd_cell_enable(struct platform_device *pdev)
 {
 	const struct mfd_cell *cell = mfd_get_cell(pdev);
@@ -91,6 +95,7 @@ static int mfd_add_device(struct device *parent, int id,
 		goto fail_device;

 	pdev->dev.parent = parent;
+	pdev->dev.type = &mfd_device_type;

 	if (parent->of_node && cell->of_compatible) {
 		for_each_child_of_node(parent->of_node, np) {
@@ -204,10 +209,16 @@ EXPORT_SYMBOL(mfd_add_devices);

 static int mfd_remove_devices_fn(struct device *dev, void *c)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct platform_device *pdev;
+	const struct mfd_cell *cell;
 	atomic_t **usage_count = c;

+	if (dev->type != &mfd_device_type)
+		return 0;
+
+	pdev = to_platform_device(dev);
+	cell = mfd_get_cell(pdev);
+
 	/* find the base address of usage_count pointers (for freeing) */
 	if (!*usage_count || (cell->usage_count < *usage_count))
 		*usage_count = cell->usage_count;