Question regarding usage of pdev->id and platform_data

[Thomas Abraham]

Hi Grant,

On 23 February 2011 23:33, Grant Likely <grant.likely at secretlab.ca> wrote:
> Hi Thomas,
>
> On Wed, Feb 23, 2011 at 10:48:37PM +0530, Thomas Abraham wrote:
>> Hi,
>>
>> I am adding support for device tree based probe for the s5pv310 serial
>> driver for a platform that has 4 instances of the uart port. I have
>> few questions on this and appreciate any help for the following
>> questions.
>>
>> 1. The driver is based on the usage of pdev->id in several parts of
>> the driver. But the platform_device created using the
>> of_platform_bus_probe function assigns -1 to pdev->id. Is the use of
>> pdev->id not advisable or is it okay if the driver assigns a pdev->id
>> value during the probe.
>
> No, the driver should *not* write a value to pdev->id at probe time.
> Doing so breaks the device model.  Instead, any enumeration that the
> driver cares about should be stored in the driver's private data
> structure.  I would recommend modifying the driver to copy pdev->id
> into its private structure.  If it is -1, then dynamically assign an
> id.
>
>>
>> 2. The driver uses pdev->dev.platform_data even after the probe is
>> complete. I read in one of the posts from Grant Likely that drivers
>> should not assign pdev->dev.platfrom_data.
>
> Correct.
>
>> Is it okay if the driver
>> parse the platform data related information from the device node and
>> assign it to pdev->dev.platform_data.
>
> No.  The driver *must not* modify or assign platform_data.  That
> pointer provides information to the driver, but there are memory
> allocation lifecycle issues if it tries to store something there.
> (Okay, I it *can* be done if the driver very carefully cleaned up
> after itself; but I strongly recommend against it; that isn't what
> that pointer is for)  Modifying it also causes issues with drivers
> that can accept either platform_data or device tree data.
>
> Instead, the driver should store all data it needs in its private data
> structure.  That's exactly why drivers have private data structures.
>
> g.
>

Thanks for your explanation. It was very helpful. I am now modifying
the uart driver to keep a copy of pdev->dev.platform_data and use it.
Also, trying to eliminate all uses of pdev->id.

Could you help me with another question. The clk_get api for s5pv310
uses the pdev->id to find the clock defined in platform code. Since
the instance of 'struct platform_device' that the driver gets when
probed using device tree will have pdev->id as -1, the clk_get fails.
Is it okay to assign a instance value to pdev->id in the probe
function.

Thanks,
Thomas.