Question regarding usage of pdev->id and platform_data

Question regarding usage of pdev->id and platform_data

[Thomas Abraham]

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.

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. Is it okay if the driver
parse the platform data related information from the device node and
assign it to pdev->dev.platform_data.

Thanks,
Thomas.

Re: Question regarding usage of pdev->id and platform_data

[Grant Likely]

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.

Re: Question regarding usage of pdev->id and platform_data

[Thomas Abraham]

Hi Grant,

On 23 February 2011 23:33, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> 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.

Re: Question regarding usage of pdev->id and platform_data

[Grant Likely]

On Thu, Feb 24, 2011 at 05:43:31PM +0530, Thomas Abraham wrote:
> Hi Grant,
> 
> On 23 February 2011 23:33, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> 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.

When clock connections are described in the device tree, it is
intended (hoped?) that code in drivers/of/clock.c will take care of
any decoding device tree references to struct clks.  Unfortunately,
the design of the clk api is such that it really is non-trivial and I
don't have an easy answer for you about how to reconcile the different
way that DT and non-DT platforms want to get their clock information.

However, I do have a workaround.  Take a look at the
of_platform_prepare() patch that I sent around a couple of days ago.
That patch will allow DT platforms to continue using the static
platform_device registrations used by non-DT platforms with the same
SoC, while still getting links to the device tree nodes.  It may not
be the long term solution, but I'm comfortable merging code that uses
it because it sidesteps a lot of these issues (the static
registrations can keep using the current .id and clk_get() code).

g.

Re: Question regarding usage of pdev->id and platform_data

[Thomas Abraham]

Hi Grant,

On 18 March 2011 00:49, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Thu, Feb 24, 2011 at 05:43:31PM +0530, Thomas Abraham wrote:
>> Hi Grant,
>>
>> On 23 February 2011 23:33, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> 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.
>
> When clock connections are described in the device tree, it is
> intended (hoped?) that code in drivers/of/clock.c will take care of
> any decoding device tree references to struct clks.  Unfortunately,
> the design of the clk api is such that it really is non-trivial and I
> don't have an easy answer for you about how to reconcile the different
> way that DT and non-DT platforms want to get their clock information.
>
> However, I do have a workaround.  Take a look at the
> of_platform_prepare() patch that I sent around a couple of days ago.
> That patch will allow DT platforms to continue using the static
> platform_device registrations used by non-DT platforms with the same
> SoC, while still getting links to the device tree nodes.  It may not
> be the long term solution, but I'm comfortable merging code that uses
> it because it sidesteps a lot of these issues (the static
> registrations can keep using the current .id and clk_get() code).

Thanks. I will check with of_platform_prepare() and of_platform_populate()

Regards,
Thomas.

>
> g.
>
>