On Mon, Jan 31, 2011 at 2:14 PM, Milton Miller <miltonm@xxxxxxx> wrote: > On Mon, 31 Jan 2011 about 15:25:51 -0000, Thomas Chou wrote: > >> This patch is based on an earlier patch from Albert Herranz, >> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/ >> 9854eb78607c641ab5ae85bcbe3c9d14ac113733 >> >> The dts binding is modified as Grant suggested. The of probing >> is merged inline instead of a separate file. It uses the newer >> of gpio probe. > >> @@ -172,6 +227,8 @@ err_request_sda: >> err_alloc_bit_data: >> kfree(adap); >> err_alloc_adap: >> + if (!pdev->dev.platform_data) >> + kfree(pdata); > > This looks better with the code move to the function above, but > I don't like the condition on this free here ... > >> return ret; >> } >> >> @@ -179,23 +236,33 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev) >> { >> struct i2c_gpio_platform_data *pdata; >> struct i2c_adapter *adap; >> + struct i2c_algo_bit_data *bit_data; >> >> adap = platform_get_drvdata(pdev); >> - pdata = pdev->dev.platform_data; >> + bit_data = adap->algo_data; >> + pdata = bit_data->data; >> >> i2c_del_adapter(adap); >> gpio_free(pdata->scl_pin); >> gpio_free(pdata->sda_pin); >> kfree(adap->algo_data); >> kfree(adap); >> + if (!pdev->dev.platform_data) >> + kfree(pdata); > > and especially here ... it seems unrelated. > > Looking at devices/base/platform.c, how about just allocating the > initial platform data struct on the stack in i2c_gpio_of_probe > and then calling platform_device_add_data? That way the pdev > is responsible for freeing the data. No, *do not* use platform_device_add_data(). It stores the pointer in pdev->dev.platform_data which gets freed when the last reference to the device is dropped (it gets removed from the bus). Thomas wants something that gets freed when the driver is unbound. Driver code must never modify the pdev->dev.platform_data pointer. Doing so causes all kinds of lifecycle ambiguity which messes up driver unbinding/rebinding and module load/unload. When a driver needs a modifiable copy of platform data, then it must include a copy of the platform_data in the driver private data structure. It should be straight forward to refactor this driver to do so. g. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
