Hi Grant,

On 23 February 2011 23:33, Grant Likely [off-list ref] wrote:

Hi Thomas,

On Wed, Feb 23, 2011 at 10:48:37PM +0530, Thomas Abraham wrote:

quoted

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.

quoted

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.

quoted

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.