Re: [3/3,v2] i2c-gpio: add devicetree support

[Grant Likely <grant.likely@secretlab.ca>]

On Mon, Jan 31, 2011 at 2:14 PM, Milton Miller <miltonm@bga.com> 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.