[PATCH] Driver Core: Add platform device arch data V2

[PATCH] Driver Core: Add platform device arch data V2

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

Allow architecture specific data in struct platform_device V2.
The structure pdev_archdata is added to struct platform_device,
similar to struct dev_archdata in struct device.

Useful for architecture code that needs to keep extra data
associated with each platform device. This data shall not
be accessed by platform drivers, only architecture code.

Needed for platform device runtime PM.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Applies to next-20090529.

 Changes since V1:
 - post to lkml, keep linux-pm cc:ed
 - add struct pdev_archdata to asm-generic
 - add struct pdev_archdata to non-generic architectures
 - drop Kconfig bits

 arch/arm/include/asm/device.h        |    3 +++
 arch/ia64/include/asm/device.h       |    3 +++
 arch/microblaze/include/asm/device.h |    3 +++
 arch/powerpc/include/asm/device.h    |    3 +++
 arch/sparc/include/asm/device.h      |    3 +++
 arch/x86/include/asm/device.h        |    3 +++
 include/asm-generic/device.h         |    3 +++
 include/linux/platform_device.h      |    3 +++
 8 files changed, 24 insertions(+)

--- 0001/arch/arm/include/asm/device.h
+++ work/arch/arm/include/asm/device.h	2009-06-01 12:19:51.000000000 +0900
@@ -12,4 +12,7 @@ struct dev_archdata {
 #endif
 };
 
+struct pdev_archdata {
+};
+
 #endif
--- 0001/arch/ia64/include/asm/device.h
+++ work/arch/ia64/include/asm/device.h	2009-06-01 12:18:11.000000000 +0900
@@ -15,4 +15,7 @@ struct dev_archdata {
 #endif
 };
 
+struct pdev_archdata {
+};
+
 #endif /* _ASM_IA64_DEVICE_H */
--- 0001/arch/microblaze/include/asm/device.h
+++ work/arch/microblaze/include/asm/device.h	2009-06-01 12:19:32.000000000 +0900
@@ -16,6 +16,9 @@ struct dev_archdata {
 	struct device_node	*of_node;
 };
 
+struct pdev_archdata {
+};
+
 #endif /* _ASM_MICROBLAZE_DEVICE_H */
 
 
--- 0001/arch/powerpc/include/asm/device.h
+++ work/arch/powerpc/include/asm/device.h	2009-06-01 12:16:28.000000000 +0900
@@ -30,4 +30,7 @@ dev_archdata_get_node(const struct dev_a
 	return ad->of_node;
 }
 
+struct pdev_archdata {
+};
+
 #endif /* _ASM_POWERPC_DEVICE_H */
--- 0001/arch/sparc/include/asm/device.h
+++ work/arch/sparc/include/asm/device.h	2009-06-01 12:18:57.000000000 +0900
@@ -32,4 +32,7 @@ dev_archdata_get_node(const struct dev_a
 	return ad->prom_node;
 }
 
+struct pdev_archdata {
+};
+
 #endif /* _ASM_SPARC_DEVICE_H */
--- 0001/arch/x86/include/asm/device.h
+++ work/arch/x86/include/asm/device.h	2009-06-01 12:17:28.000000000 +0900
@@ -13,4 +13,7 @@ struct dma_map_ops *dma_ops;
 #endif
 };
 
+struct pdev_archdata {
+};
+
 #endif /* _ASM_X86_DEVICE_H */
--- 0001/include/asm-generic/device.h
+++ work/include/asm-generic/device.h	2009-06-01 12:16:20.000000000 +0900
@@ -9,4 +9,7 @@
 struct dev_archdata {
 };
 
+struct pdev_archdata {
+};
+
 #endif /* _ASM_GENERIC_DEVICE_H */
--- 0001/include/linux/platform_device.h
+++ work/include/linux/platform_device.h	2009-06-01 12:14:43.000000000 +0900
@@ -22,6 +22,9 @@ struct platform_device {
 	struct resource	* resource;
 
 	struct platform_device_id	*id_entry;
+
+	/* arch specific additions */
+	struct pdev_archdata	archdata;
 };
 
 #define platform_get_device_id(pdev)	((pdev)->id_entry)

Re: [PATCH] Driver Core: Add platform device arch data V2

[Rafael J. Wysocki]

On Monday 01 June 2009, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> Allow architecture specific data in struct platform_device V2.
> The structure pdev_archdata is added to struct platform_device,
> similar to struct dev_archdata in struct device.
> 
> Useful for architecture code that needs to keep extra data
> associated with each platform device. This data shall not
> be accessed by platform drivers, only architecture code.
> 
> Needed for platform device runtime PM.

What exactly do you need this data for?

Rafael


> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Applies to next-20090529.
> 
>  Changes since V1:
>  - post to lkml, keep linux-pm cc:ed
>  - add struct pdev_archdata to asm-generic
>  - add struct pdev_archdata to non-generic architectures
>  - drop Kconfig bits
> 
>  arch/arm/include/asm/device.h        |    3 +++
>  arch/ia64/include/asm/device.h       |    3 +++
>  arch/microblaze/include/asm/device.h |    3 +++
>  arch/powerpc/include/asm/device.h    |    3 +++
>  arch/sparc/include/asm/device.h      |    3 +++
>  arch/x86/include/asm/device.h        |    3 +++
>  include/asm-generic/device.h         |    3 +++
>  include/linux/platform_device.h      |    3 +++
>  8 files changed, 24 insertions(+)
> 
> --- 0001/arch/arm/include/asm/device.h
> +++ work/arch/arm/include/asm/device.h	2009-06-01 12:19:51.000000000 +0900
> @@ -12,4 +12,7 @@ struct dev_archdata {
>  #endif
>  };
>  
> +struct pdev_archdata {
> +};
> +
>  #endif
> --- 0001/arch/ia64/include/asm/device.h
> +++ work/arch/ia64/include/asm/device.h	2009-06-01 12:18:11.000000000 +0900
> @@ -15,4 +15,7 @@ struct dev_archdata {
>  #endif
>  };
>  
> +struct pdev_archdata {
> +};
> +
>  #endif /* _ASM_IA64_DEVICE_H */
> --- 0001/arch/microblaze/include/asm/device.h
> +++ work/arch/microblaze/include/asm/device.h	2009-06-01 12:19:32.000000000 +0900
> @@ -16,6 +16,9 @@ struct dev_archdata {
>  	struct device_node	*of_node;
>  };
>  
> +struct pdev_archdata {
> +};
> +
>  #endif /* _ASM_MICROBLAZE_DEVICE_H */
>  
>  
> --- 0001/arch/powerpc/include/asm/device.h
> +++ work/arch/powerpc/include/asm/device.h	2009-06-01 12:16:28.000000000 +0900
> @@ -30,4 +30,7 @@ dev_archdata_get_node(const struct dev_a
>  	return ad->of_node;
>  }
>  
> +struct pdev_archdata {
> +};
> +
>  #endif /* _ASM_POWERPC_DEVICE_H */
> --- 0001/arch/sparc/include/asm/device.h
> +++ work/arch/sparc/include/asm/device.h	2009-06-01 12:18:57.000000000 +0900
> @@ -32,4 +32,7 @@ dev_archdata_get_node(const struct dev_a
>  	return ad->prom_node;
>  }
>  
> +struct pdev_archdata {
> +};
> +
>  #endif /* _ASM_SPARC_DEVICE_H */
> --- 0001/arch/x86/include/asm/device.h
> +++ work/arch/x86/include/asm/device.h	2009-06-01 12:17:28.000000000 +0900
> @@ -13,4 +13,7 @@ struct dma_map_ops *dma_ops;
>  #endif
>  };
>  
> +struct pdev_archdata {
> +};
> +
>  #endif /* _ASM_X86_DEVICE_H */
> --- 0001/include/asm-generic/device.h
> +++ work/include/asm-generic/device.h	2009-06-01 12:16:20.000000000 +0900
> @@ -9,4 +9,7 @@
>  struct dev_archdata {
>  };
>  
> +struct pdev_archdata {
> +};
> +
>  #endif /* _ASM_GENERIC_DEVICE_H */
> --- 0001/include/linux/platform_device.h
> +++ work/include/linux/platform_device.h	2009-06-01 12:14:43.000000000 +0900
> @@ -22,6 +22,9 @@ struct platform_device {
>  	struct resource	* resource;
>  
>  	struct platform_device_id	*id_entry;
> +
> +	/* arch specific additions */
> +	struct pdev_archdata	archdata;
>  };
>  
>  #define platform_get_device_id(pdev)	((pdev)->id_entry)
> 
> 

Re: [PATCH] Driver Core: Add platform device arch data V2

[Rafael J. Wysocki]

On Monday 01 June 2009, Rafael J. Wysocki wrote:
> On Monday 01 June 2009, Magnus Damm wrote:
> > From: Magnus Damm <damm@igel.co.jp>
> > 
> > Allow architecture specific data in struct platform_device V2.
> > The structure pdev_archdata is added to struct platform_device,
> > similar to struct dev_archdata in struct device.
> > 
> > Useful for architecture code that needs to keep extra data
> > associated with each platform device. This data shall not
> > be accessed by platform drivers, only architecture code.
> > 
> > Needed for platform device runtime PM.
> 
> What exactly do you need this data for?

Anyway, I think you can introduce something like:

struct <your arch>_platform_device {
    struct platform_device dev;
    <some type> <your arch data>;
    ...
};

define your platform devices using the struct above and pass its dev member to
the functions that need 'struct platform_device' as an argument.

Then you won't need to add arch members to 'struct platform_device' itself.

Best,
Rafael

 
> > Signed-off-by: Magnus Damm <damm@igel.co.jp>
> > ---
> > 
> >  Applies to next-20090529.
> > 
> >  Changes since V1:
> >  - post to lkml, keep linux-pm cc:ed
> >  - add struct pdev_archdata to asm-generic
> >  - add struct pdev_archdata to non-generic architectures
> >  - drop Kconfig bits
> > 
> >  arch/arm/include/asm/device.h        |    3 +++
> >  arch/ia64/include/asm/device.h       |    3 +++
> >  arch/microblaze/include/asm/device.h |    3 +++
> >  arch/powerpc/include/asm/device.h    |    3 +++
> >  arch/sparc/include/asm/device.h      |    3 +++
> >  arch/x86/include/asm/device.h        |    3 +++
> >  include/asm-generic/device.h         |    3 +++
> >  include/linux/platform_device.h      |    3 +++
> >  8 files changed, 24 insertions(+)
> > 
> > --- 0001/arch/arm/include/asm/device.h
> > +++ work/arch/arm/include/asm/device.h	2009-06-01 12:19:51.000000000 +0900
> > @@ -12,4 +12,7 @@ struct dev_archdata {
> >  #endif
> >  };
> >  
> > +struct pdev_archdata {
> > +};
> > +
> >  #endif
> > --- 0001/arch/ia64/include/asm/device.h
> > +++ work/arch/ia64/include/asm/device.h	2009-06-01 12:18:11.000000000 +0900
> > @@ -15,4 +15,7 @@ struct dev_archdata {
> >  #endif
> >  };
> >  
> > +struct pdev_archdata {
> > +};
> > +
> >  #endif /* _ASM_IA64_DEVICE_H */
> > --- 0001/arch/microblaze/include/asm/device.h
> > +++ work/arch/microblaze/include/asm/device.h	2009-06-01 12:19:32.000000000 +0900
> > @@ -16,6 +16,9 @@ struct dev_archdata {
> >  	struct device_node	*of_node;
> >  };
> >  
> > +struct pdev_archdata {
> > +};
> > +
> >  #endif /* _ASM_MICROBLAZE_DEVICE_H */
> >  
> >  
> > --- 0001/arch/powerpc/include/asm/device.h
> > +++ work/arch/powerpc/include/asm/device.h	2009-06-01 12:16:28.000000000 +0900
> > @@ -30,4 +30,7 @@ dev_archdata_get_node(const struct dev_a
> >  	return ad->of_node;
> >  }
> >  
> > +struct pdev_archdata {
> > +};
> > +
> >  #endif /* _ASM_POWERPC_DEVICE_H */
> > --- 0001/arch/sparc/include/asm/device.h
> > +++ work/arch/sparc/include/asm/device.h	2009-06-01 12:18:57.000000000 +0900
> > @@ -32,4 +32,7 @@ dev_archdata_get_node(const struct dev_a
> >  	return ad->prom_node;
> >  }
> >  
> > +struct pdev_archdata {
> > +};
> > +
> >  #endif /* _ASM_SPARC_DEVICE_H */
> > --- 0001/arch/x86/include/asm/device.h
> > +++ work/arch/x86/include/asm/device.h	2009-06-01 12:17:28.000000000 +0900
> > @@ -13,4 +13,7 @@ struct dma_map_ops *dma_ops;
> >  #endif
> >  };
> >  
> > +struct pdev_archdata {
> > +};
> > +
> >  #endif /* _ASM_X86_DEVICE_H */
> > --- 0001/include/asm-generic/device.h
> > +++ work/include/asm-generic/device.h	2009-06-01 12:16:20.000000000 +0900
> > @@ -9,4 +9,7 @@
> >  struct dev_archdata {
> >  };
> >  
> > +struct pdev_archdata {
> > +};
> > +
> >  #endif /* _ASM_GENERIC_DEVICE_H */
> > --- 0001/include/linux/platform_device.h
> > +++ work/include/linux/platform_device.h	2009-06-01 12:14:43.000000000 +0900
> > @@ -22,6 +22,9 @@ struct platform_device {
> >  	struct resource	* resource;
> >  
> >  	struct platform_device_id	*id_entry;
> > +
> > +	/* arch specific additions */
> > +	struct pdev_archdata	archdata;
> >  };
> >  
> >  #define platform_get_device_id(pdev)	((pdev)->id_entry)
> > 
> > 
> --

Re: [PATCH] Driver Core: Add platform device arch data V2

[Magnus Damm]

2009/6/2 Rafael J. Wysocki <rjw@sisk.pl>:
> On Monday 01 June 2009, Rafael J. Wysocki wrote:
>> On Monday 01 June 2009, Magnus Damm wrote:
>> > From: Magnus Damm <damm@igel.co.jp>
>> >
>> > Allow architecture specific data in struct platform_device V2.
>> > The structure pdev_archdata is added to struct platform_device,
>> > similar to struct dev_archdata in struct device.
>> >
>> > Useful for architecture code that needs to keep extra data
>> > associated with each platform device. This data shall not
>> > be accessed by platform drivers, only architecture code.
>> >
>> > Needed for platform device runtime PM.
>>
>> What exactly do you need this data for?

I'd like to keep a hardware block id associated with each platform
device on our SoC.
Please have a look at "PATCH [04/04] sh: Runtime platform device PM mockup",
http://patchwork.kernel.org/patch/26421/

> Anyway, I think you can introduce something like:
>
> struct <your arch>_platform_device {
>    struct platform_device dev;
>    <some type> <your arch data>;
>    ...
> };
>
> define your platform devices using the struct above and pass its dev member to
> the functions that need 'struct platform_device' as an argument.
>
> Then you won't need to add arch members to 'struct platform_device' itself.

Thanks for your suggestion. I'm usually a friend of wrapping
structures and using offsetof(), but in this case I don't think it
will work very well.

I'd like to keep a SoC specific hardware block id in this architecture
specific struct. Then let the arch specific functions
platform_device_idle() and platform_device_wakeup() use this hardware
block id to locate which clocks to stop and which power domains to
fiddle with within the SoC. If we only consider this on-SoC case then
wrapping and offsetof() works well.

However, a typical embedded system has a wide range of platform
devices. Some are for the SoC itself and some are for external
devices, like on board ethernet controlllers (not on chip like the SoC
platform devices). And since idle() and wakeup() work with struct
platform device, with a wrapped data structure we need some way to
check if the platform data is actually wrapped and offsetof() is
valid. I guess we could use some platform device specific flag for
this, but that seems overly complicated in my opinion. And modifying
idle() and wakup() to take arch specific structures is not so good
since we want to use the same platform driver on multiple
architectures.

My mockup code that keeps keeps the hardware block id in the platform
device arch specific data works well since the hardware block id with
value zero is a special case. The value zero means "external non-soc
device", so a "regular" board specific struct platform_device that do
not setup arch specific data can just be skipped in idle()/wakeup().

If you guys dislike adding arch specific data to struct platform
device then for SuperH we can just (mis)use the arch specific data in
struct device instead. I'm afraid that solution wastes memory since
the data will only be used for platform devices anyway. So I prefer
adding arch specific data to struct platform_device instead of struct
device if possible.

Maybe there are better ways to solve this? I think arch specific data
in struct platform_device is pretty straight forward though.

Thanks,

/ magnus

Re: [PATCH] Driver Core: Add platform device arch data V2

[Kevin Hilman]

Magnus Damm <magnus.damm@gmail.com> writes:

> 2009/6/2 Rafael J. Wysocki <rjw@sisk.pl>:
>> On Monday 01 June 2009, Rafael J. Wysocki wrote:
>>> On Monday 01 June 2009, Magnus Damm wrote:
>>> > From: Magnus Damm <damm@igel.co.jp>
>>> >
>>> > Allow architecture specific data in struct platform_device V2.
>>> > The structure pdev_archdata is added to struct platform_device,
>>> > similar to struct dev_archdata in struct device.
>>> >
>>> > Useful for architecture code that needs to keep extra data
>>> > associated with each platform device. This data shall not
>>> > be accessed by platform drivers, only architecture code.
>>> >
>>> > Needed for platform device runtime PM.
>>>
>>> What exactly do you need this data for?
>
> I'd like to keep a hardware block id associated with each platform
> device on our SoC.
> Please have a look at "PATCH [04/04] sh: Runtime platform device PM mockup",
> http://patchwork.kernel.org/patch/26421/

And in OMAP, we will keep a pointer to an SoC-specific struct of
HW specific data to be used in idle/wakeup decision making.

>> Anyway, I think you can introduce something like:
>>
>> struct <your arch>_platform_device {
>>    struct platform_device dev;
>>    <some type> <your arch data>;
>>    ...
>> };
>>
>> define your platform devices using the struct above and pass its dev member to
>> the functions that need 'struct platform_device' as an argument.
>>
>> Then you won't need to add arch members to 'struct platform_device' itself.
>
> Thanks for your suggestion. I'm usually a friend of wrapping
> structures and using offsetof(), but in this case I don't think it
> will work very well.

Neither do I in this case...

> I'd like to keep a SoC specific hardware block id in this architecture
> specific struct. Then let the arch specific functions
> platform_device_idle() and platform_device_wakeup() use this hardware
> block id to locate which clocks to stop and which power domains to
> fiddle with within the SoC. If we only consider this on-SoC case then
> wrapping and offsetof() works well.
>
> However, a typical embedded system has a wide range of platform
> devices. Some are for the SoC itself and some are for external
> devices, like on board ethernet controlllers (not on chip like the SoC
> platform devices). And since idle() and wakeup() work with struct
> platform device, with a wrapped data structure we need some way to
> check if the platform data is actually wrapped and offsetof() is
> valid. I guess we could use some platform device specific flag for
> this, but that seems overly complicated in my opinion. And modifying
> idle() and wakup() to take arch specific structures is not so good
> since we want to use the same platform driver on multiple
> architectures.

Also, there many cases where platform_devices are not declared
statically and using the wrapper method doesn't work well if you are
using platform_device_alloc().  In addition to not being able to use
container_of() etc.  the memory allocated potentially lasts longer than
the existence of the platform_device.

I have a patch lying around that extended platform_device_alloc() to
take an extra size arg for platform-specific extentions (like
netdev_alloc() and some others like it) but I never got ambitious
enough to change all the users of platform_device_alloc().

Kevin


> My mockup code that keeps keeps the hardware block id in the platform
> device arch specific data works well since the hardware block id with
> value zero is a special case. The value zero means "external non-soc
> device", so a "regular" board specific struct platform_device that do
> not setup arch specific data can just be skipped in idle()/wakeup().
>
> If you guys dislike adding arch specific data to struct platform
> device then for SuperH we can just (mis)use the arch specific data in
> struct device instead. I'm afraid that solution wastes memory since
> the data will only be used for platform devices anyway. So I prefer
> adding arch specific data to struct platform_device instead of struct
> device if possible.
>
> Maybe there are better ways to solve this? I think arch specific data
> in struct platform_device is pretty straight forward though.

Re: [PATCH] Driver Core: Add platform device arch data V2

[Rafael J. Wysocki]

On Tuesday 02 June 2009, Kevin Hilman wrote:
> Magnus Damm <magnus.damm@gmail.com> writes:
> 
> > 2009/6/2 Rafael J. Wysocki <rjw@sisk.pl>:
> >> On Monday 01 June 2009, Rafael J. Wysocki wrote:
> >>> On Monday 01 June 2009, Magnus Damm wrote:
> >>> > From: Magnus Damm <damm@igel.co.jp>
> >>> >
> >>> > Allow architecture specific data in struct platform_device V2.
> >>> > The structure pdev_archdata is added to struct platform_device,
> >>> > similar to struct dev_archdata in struct device.
> >>> >
> >>> > Useful for architecture code that needs to keep extra data
> >>> > associated with each platform device. This data shall not
> >>> > be accessed by platform drivers, only architecture code.
> >>> >
> >>> > Needed for platform device runtime PM.
> >>>
> >>> What exactly do you need this data for?
> >
> > I'd like to keep a hardware block id associated with each platform
> > device on our SoC.
> > Please have a look at "PATCH [04/04] sh: Runtime platform device PM mockup",
> > http://patchwork.kernel.org/patch/26421/
> 
> And in OMAP, we will keep a pointer to an SoC-specific struct of
> HW specific data to be used in idle/wakeup decision making.
> 
> >> Anyway, I think you can introduce something like:
> >>
> >> struct <your arch>_platform_device {
> >>    struct platform_device dev;
> >>    <some type> <your arch data>;
> >>    ...
> >> };
> >>
> >> define your platform devices using the struct above and pass its dev member to
> >> the functions that need 'struct platform_device' as an argument.
> >>
> >> Then you won't need to add arch members to 'struct platform_device' itself.
> >
> > Thanks for your suggestion. I'm usually a friend of wrapping
> > structures and using offsetof(), but in this case I don't think it
> > will work very well.
> 
> Neither do I in this case...
> 
> > I'd like to keep a SoC specific hardware block id in this architecture
> > specific struct. Then let the arch specific functions
> > platform_device_idle() and platform_device_wakeup() use this hardware
> > block id to locate which clocks to stop and which power domains to
> > fiddle with within the SoC. If we only consider this on-SoC case then
> > wrapping and offsetof() works well.
> >
> > However, a typical embedded system has a wide range of platform
> > devices. Some are for the SoC itself and some are for external
> > devices, like on board ethernet controlllers (not on chip like the SoC
> > platform devices). And since idle() and wakeup() work with struct
> > platform device, with a wrapped data structure we need some way to
> > check if the platform data is actually wrapped and offsetof() is
> > valid. I guess we could use some platform device specific flag for
> > this, but that seems overly complicated in my opinion. And modifying
> > idle() and wakup() to take arch specific structures is not so good
> > since we want to use the same platform driver on multiple
> > architectures.
> 
> Also, there many cases where platform_devices are not declared
> statically and using the wrapper method doesn't work well if you are
> using platform_device_alloc().  In addition to not being able to use
> container_of() etc.  the memory allocated potentially lasts longer than
> the existence of the platform_device.

OK, that is a valid point.

Best,
Rafael

Re: [PATCH] Driver Core: Add platform device arch data V2

[Rafael J. Wysocki]

On Tuesday 02 June 2009, Magnus Damm wrote:
> 2009/6/2 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Monday 01 June 2009, Rafael J. Wysocki wrote:
> >> On Monday 01 June 2009, Magnus Damm wrote:
> >> > From: Magnus Damm <damm@igel.co.jp>
> >> >
> >> > Allow architecture specific data in struct platform_device V2.
> >> > The structure pdev_archdata is added to struct platform_device,
> >> > similar to struct dev_archdata in struct device.
> >> >
> >> > Useful for architecture code that needs to keep extra data
> >> > associated with each platform device. This data shall not
> >> > be accessed by platform drivers, only architecture code.
> >> >
> >> > Needed for platform device runtime PM.
> >>
> >> What exactly do you need this data for?
> 
> I'd like to keep a hardware block id associated with each platform
> device on our SoC.
> Please have a look at "PATCH [04/04] sh: Runtime platform device PM mockup",
> http://patchwork.kernel.org/patch/26421/
> 
> > Anyway, I think you can introduce something like:
> >
> > struct <your arch>_platform_device {
> >    struct platform_device dev;
> >    <some type> <your arch data>;
> >    ...
> > };
> >
> > define your platform devices using the struct above and pass its dev member to
> > the functions that need 'struct platform_device' as an argument.
> >
> > Then you won't need to add arch members to 'struct platform_device' itself.
> 
> Thanks for your suggestion. I'm usually a friend of wrapping
> structures and using offsetof(), but in this case I don't think it
> will work very well.
> 
> I'd like to keep a SoC specific hardware block id in this architecture
> specific struct. Then let the arch specific functions
> platform_device_idle() and platform_device_wakeup() use this hardware
> block id to locate which clocks to stop and which power domains to
> fiddle with within the SoC. If we only consider this on-SoC case then
> wrapping and offsetof() works well.
> 
> However, a typical embedded system has a wide range of platform
> devices. Some are for the SoC itself and some are for external
> devices, like on board ethernet controlllers (not on chip like the SoC
> platform devices).

Why don't we use the arch-specific wrapping for all platform devices on this
arch, the SoC ones and the non-SoC ones?  Is there any particular reason not
to do that? 

If idle() and wakeup() are supposed to be platform-specific, and quite frankly
I don't see how they could be implemented in a generic way, they don't even
need to operate on struct platform_device objects.

> And since idle() and wakeup() work with struct
> platform device, with a wrapped data structure we need some way to
> check if the platform data is actually wrapped and offsetof() is
> valid. I guess we could use some platform device specific flag for
> this, but that seems overly complicated in my opinion. And modifying
> idle() and wakup() to take arch specific structures is not so good
> since we want to use the same platform driver on multiple
> architectures.
> 
> My mockup code that keeps keeps the hardware block id in the platform
> device arch specific data works well since the hardware block id with
> value zero is a special case. The value zero means "external non-soc
> device", so a "regular" board specific struct platform_device that do
> not setup arch specific data can just be skipped in idle()/wakeup().
> 
> If you guys dislike adding arch specific data to struct platform
> device then for SuperH we can just (mis)use the arch specific data in
> struct device instead. I'm afraid that solution wastes memory since
> the data will only be used for platform devices anyway. So I prefer
> adding arch specific data to struct platform_device instead of struct
> device if possible.

That's generally OK, but I'd like to get convinced that there's no better
way indeed.

What I personally don't like about the patch is the duplication of empty
struct pdev_archdata for quite a number of architectures.  It's not nice and
I wonder if there's a way to avoid it.

Thanks,
Rafael

Re: [PATCH] Driver Core: Add platform device arch data V2

[Magnus Damm]

On Wed, Jun 3, 2009 at 5:45 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Tuesday 02 June 2009, Magnus Damm wrote:
>> However, a typical embedded system has a wide range of platform
>> devices. Some are for the SoC itself and some are for external
>> devices, like on board ethernet controlllers (not on chip like the SoC
>> platform devices).
>
> Why don't we use the arch-specific wrapping for all platform devices on this
> arch, the SoC ones and the non-SoC ones?  Is there any particular reason not
> to do that?

There are quite a few in-tree platform device drivers for SuperH that
can run on both ARM and SuperH. Maybe even MIPS in the future, who
knows. And we already have a perfectly fine platform device interface
that we can share, so I don't see any reason why we should invent
something new all of a sudden.

> If idle() and wakeup() are supposed to be platform-specific, and quite frankly
> I don't see how they could be implemented in a generic way, they don't even
> need to operate on struct platform_device objects.

The implementation can be platform specific, but the interface should
be shared across architectures. This so we can use the same device
driver on multiple architectures.

If you dislike the idea of operating on struct platform_device then
how about letting idle()/wakeup() or enable()/disable() take a pointer
to struct device instead of struct platform_device?

In my opinion it makes sense to allow architectures to add data to
struct platform_device since it's platform devices we're going to do
runtime PM on. For architectures not using struct pdev_archdata there
is really no additional runtime cost.

>> If you guys dislike adding arch specific data to struct platform
>> device then for SuperH we can just (mis)use the arch specific data in
>> struct device instead. I'm afraid that solution wastes memory since
>> the data will only be used for platform devices anyway. So I prefer
>> adding arch specific data to struct platform_device instead of struct
>> device if possible.
>
> That's generally OK, but I'd like to get convinced that there's no better
> way indeed.

The earlier version did something ugly with Kconfig. That's even worse IMO.

> What I personally don't like about the patch is the duplication of empty
> struct pdev_archdata for quite a number of architectures.  It's not nice and
> I wonder if there's a way to avoid it.

I don't think there's any other way around that than Kconfig. The per
arch duplication of empty structures is already done for struct device
so it can't be that bad.

Thanks for your help!

/ magnus

Re: [PATCH] Driver Core: Add platform device arch data V2

[Rafael J. Wysocki]

On Tuesday 02 June 2009, Magnus Damm wrote:
> 2009/6/2 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Monday 01 June 2009, Rafael J. Wysocki wrote:
> >> On Monday 01 June 2009, Magnus Damm wrote:
> >> > From: Magnus Damm <damm@igel.co.jp>
[--snip--]
> 
> If you guys dislike adding arch specific data to struct platform
> device then for SuperH we can just (mis)use the arch specific data in
> struct device instead. I'm afraid that solution wastes memory since
> the data will only be used for platform devices anyway. So I prefer
> adding arch specific data to struct platform_device instead of struct
> device if possible.

BTW, what is the difference really?  You can always put
dev.platform_data = NULL for devices that don't have any platform data,
can't you?

Rafael

Re: [PATCH] Driver Core: Add platform device arch data V2

[Magnus Damm]

On Wed, Jun 3, 2009 at 6:01 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Tuesday 02 June 2009, Magnus Damm wrote:
>> If you guys dislike adding arch specific data to struct platform
>> device then for SuperH we can just (mis)use the arch specific data in
>> struct device instead. I'm afraid that solution wastes memory since
>> the data will only be used for platform devices anyway. So I prefer
>> adding arch specific data to struct platform_device instead of struct
>> device if possible.
>
> BTW, what is the difference really?  You can always put
> dev.platform_data = NULL for devices that don't have any platform data,
> can't you?

So the convention is that dev.platform_data points to driver-specific
data. It may or may not be required by the driver. The format of this
data is driver specific and should be the same across architectures.

What I'm trying to add with struct pdev_archdata is a place for
architecture specific data. This data is needed by architecture
specific code (for example runtime PM), and since it's architecture
specific it should _never_ be touched by device driver code. Exactly
like struct dev_archdata but for platform devices.

Like I said, we _could_ use struct device for this purpose, but it
sounds like suboptimal software design to me. Using struct device
means that we put data where it doesn't belong. I'd like to add
_platform_ _device_ _specific_ data, not data that should be present
in all struct devices in the system but only used in some cases.

Cheers,

/ magnus

Re: [PATCH] Driver Core: Add platform device arch data V2

[Rafael J. Wysocki]

On Friday 05 June 2009, Magnus Damm wrote:
> On Wed, Jun 3, 2009 at 6:01 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Tuesday 02 June 2009, Magnus Damm wrote:
> >> If you guys dislike adding arch specific data to struct platform
> >> device then for SuperH we can just (mis)use the arch specific data in
> >> struct device instead. I'm afraid that solution wastes memory since
> >> the data will only be used for platform devices anyway. So I prefer
> >> adding arch specific data to struct platform_device instead of struct
> >> device if possible.
> >
> > BTW, what is the difference really?  You can always put
> > dev.platform_data = NULL for devices that don't have any platform data,
> > can't you?
> 
> So the convention is that dev.platform_data points to driver-specific
> data. It may or may not be required by the driver. The format of this
> data is driver specific and should be the same across architectures.
> 
> What I'm trying to add with struct pdev_archdata is a place for
> architecture specific data. This data is needed by architecture
> specific code (for example runtime PM), and since it's architecture
> specific it should _never_ be touched by device driver code. Exactly
> like struct dev_archdata but for platform devices.
> 
> Like I said, we _could_ use struct device for this purpose, but it
> sounds like suboptimal software design to me. Using struct device
> means that we put data where it doesn't belong. I'd like to add
> _platform_ _device_ _specific_ data, not data that should be present
> in all struct devices in the system but only used in some cases.

OK, that explains the idea.  Perhaps it's woth putting into the changelog?

Best,
Rafael