resource: type, size and IORESOURCE_CLK patches V2
Subsystems (4)
cover resource: type, size and IORESOURCE_CLK patches V2
This is V2 of the resource type, size and IORESOURCE_CLK patches.
[PATCH 01/05] resource: add resource_size()
[PATCH 02/05] resource: add resource_type() and IORESOURCE_TYPE_BITS [PATCH 03/05] resource: add new IORESOURCE_CLK type V2 [PATCH 04/05] i2c-sh_mobile: IORESOURCE_CLK support
[PATCH 05/05] sh: add IORESOURCE_CLK to SuperH Mobile I2C platform data
Changes since V1
- Just add a single bit for IORESOURCE_CLK
- Dropped "[PATCH 03/04] resource: use new resource type changes" for now
- Added IORESOURCE_CLK driver patch for i2c-sh_mobile
- Added IORESOURCE_CLK platform data patch
---
arch/sh/kernel/cpu/sh4a/setup-sh7343.c | 14 ++++++++++---- arch/sh/kernel/cpu/sh4a/setup-sh7366.c | 7 +++++-- arch/sh/kernel/cpu/sh4a/setup-sh7722.c | 7 +++++-- arch/sh/kernel/cpu/sh4a/setup-sh7723.c | 7 +++++-- drivers/base/platform.c | 31 +++++++++++++++++-------------- drivers/i2c/busses/i2c-sh_mobile.c | 8 +++++++- include/linux/ioport.h | 28 +++++++++++++++++++--------- kernel/resource.c | 2 +- 8 files changed, 69 insertions(+), 35 deletions(-)
PATCH 1/5 resource: add resource_size()
From: Magnus Damm <[email protected]>
Avoid one-off errors by introducing a resource_size() function.
---
include/linux/ioport.h | 4 ++++ kernel/resource.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)
include/linux/ioport.h +4 -0
@@ -113,6 +113,10 @@ extern int allocate_resource(struct resoint adjust_resource (struct resource *res,resource_size_t start,resource_size_t size);resource_size_t resource_alignment (struct resource *res); +static inline resource_size_t resource_size (struct resource *res) +{ +return res->end - res->start +1 ; +}/* Convenience shorthand with allocation */ #define request_region (start,n,name) __request_region(&ioport_resource, (start), (n), (name))
kernel/resource.c +1 -1
@@ -490,7 +490,7 @@ resource_size_t resource_alignment(struc {switch (res->flags & (IORESOURCE_SIZEALIGN |IORESOURCE_STARTALIGN )) {case IORESOURCE_SIZEALIGN : -return res->end - res->start +1 ; +return resource_size (res);case IORESOURCE_STARTALIGN :return res->start;default :
Re: [PATCH 01/05] resource: add resource_size()
Quoted 5 lines · Magnus Damm
From: Magnus Damm <[email protected]>
Avoid one-off errors by introducing a
resource_size()function.Signed-off-by Magnus Damm <[email protected]>
A resource_size definition is a good idea, given the number of times it has been re-implemented throughout the kernel.
Quoted 35 lines
---
include/linux/ioport.h | 4 ++++ kernel/resource.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)include/linux/ioport.h +4
-0@@ -113,6 +113,10 @@ extern int allocate_resource(struct resoint adjust_resource (struct resource *res,resource_size_t start,resource_size_t size);resource_size_t resource_alignment (struct resource *res); +static inline resource_size_t resource_size (struct resource *res) +{ +return res->end - res->start +1 ; +}/* Convenience shorthand with allocation */ #define request_region (start,n,name) __request_region(&ioport_resource, (start), (n), (name))kernel/resource.c +1
-2@@ -490,7 +490,7 @@ resource_size_t resource_alignment(struc {switch (res->flags & (IORESOURCE_SIZEALIGN |IORESOURCE_STARTALIGN )) {case IORESOURCE_SIZEALIGN : -return res->end - res->start +1 ; +return resource_size (res);case IORESOURCE_STARTALIGN :return res->start;default : --To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes'
PATCH 2/5 resource: add resource_type() and IORESOURCE_TYPE_BITS
In reply to Magnus DammFrom: Magnus Damm <[email protected]>
This patch adds resource_type() and IORESOURCE_TYPE_BITS. They make it easier to add more resource types without having to rewrite tons of code.
---
drivers/base/platform.c | 31 +++++++++++++++++-------------- include/linux/ioport.h | 7 ++++++- 2 files changed, 23 insertions(+), 15 deletions(-)
drivers/base/platform.c +17 -14
@@ -42,10 +42,8 @@ struct resource *platform_get_resource(sfor (i =0 ; i < dev->num_resources; i++) {struct resource *r = &dev->resource[i]; -if ((r->flags & (IORESOURCE_IO |IORESOURCE_MEM | -IORESOURCE_IRQ |IORESOURCE_DMA )) == type) -if (num-- ==0 ) -return r; +if (type ==resource_type (r) && num-- ==0 ) +return r; }return NULL ; }
@@ -78,10 +76,8 @@ struct resource *platform_get_resource_bfor (i =0 ; i < dev->num_resources; i++) {struct resource *r = &dev->resource[i]; -if ((r->flags & (IORESOURCE_IO |IORESOURCE_MEM | -IORESOURCE_IRQ |IORESOURCE_DMA )) == type) -if (!strcmp (r->name, name)) -return r; +if (type ==resource_type (r) && !strcmp (r->name, name)) +return r; }return NULL ; }
@@ -259,9 +255,9 @@ int platform_device_add(struct platform_ p = r->parent;if (!p) { -if (r->flags &IORESOURCE_MEM ) +if (resource_type (r) ==IORESOURCE_MEM ) p = &iomem_resource; -else if (r->flags &IORESOURCE_IO ) +else if (resource_type (r) ==IORESOURCE_IO ) p = &ioport_resource; }
@@ -282,9 +278,14 @@ int platform_device_add(struct platform_return ret; failed: -while (--i >=0 ) -if (pdev->resource[i].flags & (IORESOURCE_MEM |IORESOURCE_IO )) -release_resource (&pdev->resource[i]); +while (--i >=0 ) { +struct resource *r = &pdev->resource[i]; +unsigned long type =resource_type (r); + +if (type ==IORESOURCE_MEM || type ==IORESOURCE_IO ) +release_resource (r); + } +return ret; }EXPORT_SYMBOL_GPL (platform_device_add);
@@ -306,7 +307,9 @@ void platform_device_del(struct platformfor (i =0 ; i < pdev->num_resources; i++) {struct resource *r = &pdev->resource[i]; -if (r->flags & (IORESOURCE_MEM |IORESOURCE_IO )) +unsigned long type =resource_type (r); + +if (type ==IORESOURCE_MEM || type ==IORESOURCE_IO )release_resource (r); } }
include/linux/ioport.h +6 -1
@@ -34,7 +34,8 @@ struct resource_list {*/ #define IORESOURCE_BITS 0x000000ff/* Bus-specific bits */ -#define IORESOURCE_IO 0x00000100/* Resource type */ +#define IORESOURCE_TYPE_BITS 0x00000f00/* Resource type */ +#define IORESOURCE_IO 0x00000100#define IORESOURCE_MEM 0x00000200#define IORESOURCE_IRQ 0x00000400#define IORESOURCE_DMA 0x00000800
@@ -117,6 +118,10 @@ static inline resource_size_t resource_s {return res->end - res->start +1 ; } +static inline unsigned long resource_type (struct resource *res) +{ +return res->flags &IORESOURCE_TYPE_BITS ; +}/* Convenience shorthand with allocation */ #define request_region (start,n,name) __request_region(&ioport_resource, (start), (n), (name))
Re: [PATCH 02/05] resource: add resource_type() and IORESOURCE_TYPE_BITS
Quoted 4 lines · Magnus Damm
From: Magnus Damm <[email protected]>
This patch adds
resource_type()andIORESOURCE_TYPE_BITS. They make it easier to add more resource types without having to rewrite tons of code.
This looks ok to me, with one concern as shown below.
Quoted 36 lines
Signed-off-by Magnus Damm <[email protected]>---
drivers/base/platform.c | 31 +++++++++++++++++-------------- include/linux/ioport.h | 7 ++++++- 2 files changed, 23 insertions(+), 15 deletions(-)drivers/base/platform.c +4
-8@@ -42,10 +42,8 @@ struct resource *platform_get_resource(sfor (i =0 ; i < dev->num_resources; i++) {struct resource *r = &dev->resource[i]; -if ((r->flags & (IORESOURCE_IO |IORESOURCE_MEM | -IORESOURCE_IRQ |IORESOURCE_DMA )) == type) -if (num-- ==0 ) -return r; +if (type ==resource_type (r) && num-- ==0 ) +return r; }return NULL ; }@@ -78,10 +76,8 @@ struct resource *platform_get_resource_bfor (i =0 ; i < dev->num_resources; i++) {struct resource *r = &dev->resource[i]; -if ((r->flags & (IORESOURCE_IO |IORESOURCE_MEM | -IORESOURCE_IRQ |IORESOURCE_DMA )) == type) -if (!strcmp (r->name, name)) -return r; +if (type ==resource_type (r) && !strcmp (r->name, name)) +return r; }return NULL ; }
drivers/base/platform.c int platform_device_add(struct platform_ · line 262
Quoted 10 lines
drivers/base/platform.c +2
-2@@ -259,9 +255,9 @@ int platform_device_add(struct platform_ p = r->parent;if (!p) { -if (r->flags &IORESOURCE_MEM ) +if (resource_type (r) ==IORESOURCE_MEM ) p = &iomem_resource; -else if (r->flags &IORESOURCE_IO ) +else if (resource_type (r) ==IORESOURCE_IO ) p = &ioport_resource; }
You are changing a simple test to a mask and compare, is anyone going to produce resources with an IORESOURCE_MEM and an IORESOURCE_IO together?
Quoted 57 lines
drivers/base/platform.c +11
-4@@ -282,9 +278,14 @@ int platform_device_add(struct platform_return ret; failed: -while (--i >=0 ) -if (pdev->resource[i].flags & (IORESOURCE_MEM |IORESOURCE_IO )) -release_resource (&pdev->resource[i]); +while (--i >=0 ) { +struct resource *r = &pdev->resource[i]; +unsigned long type =resource_type (r); + +if (type ==IORESOURCE_MEM || type ==IORESOURCE_IO ) +release_resource (r); + } +return ret; }EXPORT_SYMBOL_GPL (platform_device_add);@@ -306,7 +307,9 @@ void platform_device_del(struct platformfor (i =0 ; i < pdev->num_resources; i++) {struct resource *r = &pdev->resource[i]; -if (r->flags & (IORESOURCE_MEM |IORESOURCE_IO )) +unsigned long type =resource_type (r); + +if (type ==IORESOURCE_MEM || type ==IORESOURCE_IO )release_resource (r); } }include/linux/ioport.h +6
-2@@ -34,7 +34,8 @@ struct resource_list {*/ #define IORESOURCE_BITS 0x000000ff/* Bus-specific bits */ -#define IORESOURCE_IO 0x00000100/* Resource type */ +#define IORESOURCE_TYPE_BITS 0x00000f00/* Resource type */ +#define IORESOURCE_IO 0x00000100#define IORESOURCE_MEM 0x00000200#define IORESOURCE_IRQ 0x00000400#define IORESOURCE_DMA 0x00000800@@ -117,6 +118,10 @@ static inline resource_size_t resource_s {return res->end - res->start +1 ; } +static inline unsigned long resource_type (struct resource *res) +{ +return res->flags &IORESOURCE_TYPE_BITS ; +}/* Convenience shorthand with allocation */ #define request_region (start,n,name) __request_region(&ioport_resource, (start), (n), (name)) --To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes'
Re: [PATCH 02/05] resource: add resource_type() and IORESOURCE_TYPE_BITS
Quoted 3 lines · Ben Dooks
You are changing a simple test to a mask and compare, is anyone going to produce resources with an
IORESOURCE_MEMand anIORESOURCE_IOtogether?
Actually, I'd like to replace the one-bit-per-type strategy with a N-bit counter. But that is not very compatible with the case you are pointing out. I'm not sure if that's a combination we really want to support though. Both IRQ and DMA doesn't make much sense to me. =)
/ magnus
Re: [PATCH 02/05] resource: add resource_type() and IORESOURCE_TYPE_BITS
Quoted 9 lines · Magnus Damm
Quoted 3 lines · Ben Dooks
You are changing a simple test to a mask and compare, is anyone going to produce resources with an
IORESOURCE_MEMand anIORESOURCE_IOtogether?Actually, I'd like to replace the one-bit-per-type strategy with a N-bit counter. But that is not very compatible with the case you are pointing out. I'm not sure if that's a combination we really want to support though. Both IRQ and DMA doesn't make much sense to me. =)
I'm not saying it is a bad idea, I just do not know if anyone is currently relying on this to work...
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes'
Re: [PATCH 02/05] resource: add resource_type() and IORESOURCE_TYPE_BITS
Quoted 6 lines · Ben Dooks
Quoted 5 lines · Magnus Damm
Quoted 3 lines · Ben Dooks
You are changing a simple test to a mask and compare, is anyone going to produce resources with an
IORESOURCE_MEMand anIORESOURCE_IOtogether?
Quoted 7 lines
Quoted 4 lines
Actually, I'd like to replace the one-bit-per-type strategy with a N-bit counter. But that is not very compatible with the case you are pointing out. I'm not sure if that's a combination we really want to support though. Both IRQ and DMA doesn't make much sense to me. =)
I'm not saying it is a bad idea, I just do not know if anyone is currently relying on this to work...
In V1 I posted both a mega patch that went through and converted arch/ and also a patch that converted the type into a N-bit counter. In V2 I've taken more of a step-by-step approach and not converted into a N-bit counter.
I'm thinking that this patch shouldn't break anything since the bits are left exactly like before. And the code in drivers/base/platform.c seems to treat the bits as only one should be set anyway. For instance platform_device_add() seems to prioritize IORESOURCE_MEM over IORESOURCE_IO.
Or did I change the logic in drivers/base/platform without realizing it?
Cheers,
/ magnus
PATCH 3/5 resource: add new IORESOURCE_CLK type V2
In reply to Magnus DammFrom: Magnus Damm <[email protected]>
So far struct resource has been used with the types IORESOURCE_MEM, IORESOURCE_IO and IORESOUCE_IRQ to pass I/O and interrupt parameters to platform drivers. This patch extends this with IORESOURCE_CLK which should be used to pass a clock string to the platform driver. This string points out which specific clock that should be used with clk_get() for a certain driver instance.
Using a hard coded strings in the device driver won't do since we may have multiple instances of drivers that need to use different clocks.
---
Changes since V1:
- Use a new bit for IORESOURCE_CLK instead of switching to a counter
include/linux/ioport.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
include/linux/ioport.h +9 -8
@@ -34,20 +34,21 @@ struct resource_list {*/ #define IORESOURCE_BITS 0x000000ff/* Bus-specific bits */ -#define IORESOURCE_TYPE_BITS 0x00000f00/* Resource type */ +#define IORESOURCE_TYPE_BITS 0x00001f00/* Resource type */ #define IORESOURCE_IO 0x00000100#define IORESOURCE_MEM 0x00000200#define IORESOURCE_IRQ 0x00000400#define IORESOURCE_DMA 0x00000800 +#define IORESOURCE_CLK 0x00001000 -#define IORESOURCE_PREFETCH 0x00001000/* No side effects */ -#define IORESOURCE_READONLY 0x00002000 -#define IORESOURCE_CACHEABLE 0x00004000 -#define IORESOURCE_RANGELENGTH 0x00008000 -#define IORESOURCE_SHADOWABLE 0x00010000 +#define IORESOURCE_PREFETCH 0x00002000/* No side effects */ +#define IORESOURCE_READONLY 0x00004000 +#define IORESOURCE_CACHEABLE 0x00008000 +#define IORESOURCE_RANGELENGTH 0x00010000 +#define IORESOURCE_SHADOWABLE 0x00020000 -#define IORESOURCE_SIZEALIGN 0x00020000/* size indicates alignment */ -#define IORESOURCE_STARTALIGN 0x00040000/* start field is alignment */ +#define IORESOURCE_SIZEALIGN 0x00040000/* size indicates alignment */ +#define IORESOURCE_STARTALIGN 0x00080000/* start field is alignment */ #define IORESOURCE_DISABLED 0x10000000#define IORESOURCE_UNSET 0x20000000
Re: [i2c] [PATCH 03/05] resource: add new IORESOURCE_CLK type V2
Quoted 11 lines · Magnus Damm
From: Magnus Damm <[email protected]>
So far struct resource has been used with the types
IORESOURCE_MEM,IORESOURCE_IOandIORESOUCE_IRQto pass I/O and interrupt parameters to platform drivers. This patch extends this withIORESOURCE_CLKwhich should be used to pass a clock string to the platform driver. This string points out which specific clock that should be used withclk_get()for a certain driver instance.Using a hard coded strings in the device driver won't do since we may have multiple instances of drivers that need to use different clocks.
It works already. The S3C24XX has n-number of H and P clocks fed to each driver, differentiated by the device being supplied. This is why clk_get() has two arguments, a device pointer and a name.
This might be useful to allow optional clocks to be passed, but I'm not sure if it is necessary.
Quoted 46 lines
Signed-off-by Magnus Damm <[email protected]>---
Changes since V1:
- Use a new bit forIORESOURCE_CLKinstead of switching to a counterinclude/linux/ioport.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)include/linux/ioport.h +9
-8@@ -34,20 +34,21 @@ struct resource_list {*/ #define IORESOURCE_BITS 0x000000ff/* Bus-specific bits */ -#define IORESOURCE_TYPE_BITS 0x00000f00/* Resource type */ +#define IORESOURCE_TYPE_BITS 0x00001f00/* Resource type */ #define IORESOURCE_IO 0x00000100#define IORESOURCE_MEM 0x00000200#define IORESOURCE_IRQ 0x00000400#define IORESOURCE_DMA 0x00000800 +#define IORESOURCE_CLK 0x00001000 -#define IORESOURCE_PREFETCH 0x00001000/* No side effects */ -#define IORESOURCE_READONLY 0x00002000 -#define IORESOURCE_CACHEABLE 0x00004000 -#define IORESOURCE_RANGELENGTH 0x00008000 -#define IORESOURCE_SHADOWABLE 0x00010000 +#define IORESOURCE_PREFETCH 0x00002000/* No side effects */ +#define IORESOURCE_READONLY 0x00004000 +#define IORESOURCE_CACHEABLE 0x00008000 +#define IORESOURCE_RANGELENGTH 0x00010000 +#define IORESOURCE_SHADOWABLE 0x00020000 -#define IORESOURCE_SIZEALIGN 0x00020000/* size indicates alignment */ -#define IORESOURCE_STARTALIGN 0x00040000/* start field is alignment */ +#define IORESOURCE_SIZEALIGN 0x00040000/* size indicates alignment */ +#define IORESOURCE_STARTALIGN 0x00080000/* start field is alignment */ #define IORESOURCE_DISABLED 0x10000000#define IORESOURCE_UNSET 0x20000000_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes'
Re: [i2c] [PATCH 03/05] resource: add new IORESOURCE_CLK type V2
Quoted 10 lines · Ben Dooks
Quoted 9 lines · Magnus Damm
From: Magnus Damm <[email protected]>
So far struct resource has been used with the types
IORESOURCE_MEM,IORESOURCE_IOandIORESOUCE_IRQto pass I/O and interrupt parameters to platform drivers. This patch extends this withIORESOURCE_CLKwhich should be used to pass a clock string to the platform driver. This string points out which specific clock that should be used withclk_get()for a certain driver instance.
Quoted 6 lines
Quoted 2 lines
Using a hard coded strings in the device driver won't do since we may have multiple instances of drivers that need to use different clocks.
It works already. The S3C24XX has n-number of H and P clocks fed to each driver, differentiated by the device being supplied. This is why
clk_get()has two arguments, a device pointer and a name.
Oh, I see. Both the platform device id and the string seems to be used in the comparison.
Quoted 2 lines
This might be useful to allow optional clocks to be passed, but I'm not sure if it is necessary.
No, you are right. It is possible to use platform device id to differentiate between devices. I'm not sure if it maps well to our SuperH Mobile clock stop bits though...
Thanks for your comments!
/ magnus
PATCH 4/5 i2c-sh_mobile: IORESOURCE_CLK support
In reply to Magnus DammFrom: Magnus Damm <[email protected]>
This patch makes the i2c-sh_mobile driver get the clock name from the struct resource with type IORESOURCE_CLK provided by the platform data.
---
drivers/i2c/busses/i2c-sh_mobile.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/i2c/busses/i2c-sh_mobile.c +7 -1
@@ -390,13 +390,19 @@ static int sh_mobile_i2c_probe(struct plint size;int ret; + res =platform_get_resource (dev,IORESOURCE_CLK ,0 ); +if (res ==NULL || res->name ==NULL ) { +dev_err (&dev->dev,"cannot find CLK resource\n" ); +return -ENOENT ; + } + pd =kzalloc (sizeof (struct sh_mobile_i2c_data ),GFP_KERNEL );if (pd ==NULL ) {dev_err (&dev->dev,"cannot allocate private data\n" );return -ENOMEM ; } - pd->clk =clk_get (&dev->dev,"peripheral_clk" ); + pd->clk =clk_get (&dev->dev, res->name);if (IS_ERR (pd->clk)) {dev_err (&dev->dev,"cannot get peripheral clock\n" ); ret =PTR_ERR (pd->clk);
Re: [PATCH 04/05] i2c-sh_mobile: IORESOURCE_CLK support
Quoted 20 lines · Magnus Damm
From: Magnus Damm <[email protected]>
This patch makes the i2c-sh_mobile driver get the clock name from the struct resource with type
IORESOURCE_CLKprovided by the platform data.Signed-off-by Magnus Damm <[email protected]>---
drivers/i2c/busses/i2c-sh_mobile.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/i2c/busses/i2c-sh_mobile.c +3
-0@@ -390,13 +390,19 @@ static int sh_mobile_i2c_probe(struct plint size;int ret; + res =platform_get_resource (dev,IORESOURCE_CLK ,0 ); +if (res ==NULL || res->name ==NULL ) { +dev_err (&dev->dev,"cannot find CLK resource\n" );
drivers/i2c/busses/i2c-sh_mobile.c static int sh_mobile_i2c_probe(struct pl · line 399
Quoted 10 lines
drivers/i2c/busses/i2c-sh_mobile.c +3
-1+return -ENOENT ; + } + pd =kzalloc (sizeof (struct sh_mobile_i2c_data ),GFP_KERNEL );if (pd ==NULL ) {dev_err (&dev->dev,"cannot allocate private data\n" );return -ENOMEM ; } - pd->clk =clk_get (&dev->dev,"peripheral_clk" );
I think that is working correctly and there isn't really any need to change this. The clk_get is supplied the device that it needs the clock for, and the name of the clock needed.
Quoted 9 lines
drivers/i2c/busses/i2c-sh_mobile.c +1
-1+ pd->clk =clk_get (&dev->dev, res->name);if (IS_ERR (pd->clk)) {dev_err (&dev->dev,"cannot get peripheral clock\n" ); ret =PTR_ERR (pd->clk); --To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes'
Re: [PATCH 04/05] i2c-sh_mobile: IORESOURCE_CLK support
Quoted 30 lines · Ben Dooks
Quoted 29 lines · Magnus Damm
From: Magnus Damm <[email protected]>
This patch makes the i2c-sh_mobile driver get the clock name from the struct resource with type
IORESOURCE_CLKprovided by the platform data.Signed-off-by Magnus Damm <[email protected]>---
drivers/i2c/busses/i2c-sh_mobile.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 renamed from drivers/i2c/busses/i2c-sh_mobile.c +6
-0@@ -390,13 +390,19 @@ static int sh_mobile_i2c_probe(struct pl int size; int ret; + res = platform_get_resource(dev, IORESOURCE_CLK, 0); + if (res == NULL || res->name == NULL) { + dev_err(&dev->dev, "cannot find CLK resource\n"); + return -ENOENT; + } + pd = kzalloc(sizeof(struct sh_mobile_i2c_data), GFP_KERNEL); if (pd == NULL) { dev_err(&dev->dev, "cannot allocate private data\n"); return -ENOMEM; }
Quoted 5 lines
drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 static int sh_mobile_i2c_probe(struct pl · line 399
Quoted 1 line
drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 renamed from drivers/i2c/busses/i2c-sh_mobile.c +0
-1- pd->clk = clk_get(&dev->dev, "peripheral_clk");I think that is working correctly and there isn't really any need to change this. The
clk_getis supplied the device that it needs the clock for, and the name of the clock needed.
Right, we could handle this "under the hood" of the clock frame work implementation, or we could deal with it together with the rest of the platform data and have one unique string per hardware block. On SuperH Mobile we currently have one shared clock implementation that supports multiple processors. Which clock that is assigned to what hardware block is currently handled by per-cpu platform data, and that's where this patch comes in.
Thanks!
/ magnus
Re: [i2c] [PATCH 04/05] i2c-sh_mobile: IORESOURCE_CLK support
Quoted 19 lines · Magnus Damm
Quoted 18 lines · Ben Dooks
Quoted 17 lines · Magnus Damm
From: Magnus Damm <[email protected]>
This patch makes the i2c-sh_mobile driver get the clock name from the struct resource with type
IORESOURCE_CLKprovided by the platform data.Signed-off-by Magnus Damm <[email protected]>---
drivers/i2c/busses/i2c-sh_mobile.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 renamed from drivers/i2c/busses/i2c-sh_mobile.c +0
-0@@ -390,13 +390,19 @@ static int sh_mobile_i2c_probe(struct pl int size; int ret;
Quoted 5 lines
Quoted 5 lines
drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 static int sh_mobile_i2c_probe(struct pl · line 397
Quoted 5 lines
drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 renamed from drivers/i2c/busses/i2c-sh_mobile.c +5
-0+ res = platform_get_resource(dev, IORESOURCE_CLK, 0); + if (res == NULL || res->name == NULL) { + dev_err(&dev->dev, "cannot find CLK resource\n"); + return -ENOENT; + }
I note in this one you are re-using the resource.name field in the way it was not intended to be used. This field is for use to differentiate resources where this is more than one of them and they may not all be present in the list. This is not a good course of action.
Quoted 12 lines
Quoted 11 lines
drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 static int sh_mobile_i2c_probe(struct pl · line 399
Quoted 7 lines
drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 renamed from drivers/i2c/busses/i2c-sh_mobile.c +0
-1pd = kzalloc(sizeof(struct sh_mobile_i2c_data), GFP_KERNEL); if (pd == NULL) { dev_err(&dev->dev, "cannot allocate private data\n"); return -ENOMEM; } - pd->clk = clk_get(&dev->dev, "peripheral_clk");I think that is working correctly and there isn't really any need to change this. The
clk_getis supplied the device that it needs the clock for, and the name of the clock needed.
Quoted 7 lines
Right, we could handle this "under the hood" of the clock frame work implementation, or we could deal with it together with the rest of the platform data and have one unique string per hardware block. On SuperH Mobile we currently have one shared clock implementation that supports multiple processors. Which clock that is assigned to what hardware block is currently handled by per-cpu platform data, and that's where this patch comes in.
We have a shared implementation for the s3c24xx, which all have subtly different clock requirements and we change the clock registration for the chip, instead of trying to subvert the platform system. I do not see this as being "under the hood", changing resources at registration time is done all over the place.
Techincally, if there is only one clock per block, then you don't actually need a name, just supply the device that you are looking up a clock for.
Note, added Russell King to the discussion, as he is the original authour of the clock framework anyway.
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes'
Re: [i2c] [PATCH 04/05] i2c-sh_mobile: IORESOURCE_CLK support
Quoted 20 lines · Ben Dooks
Quoted 19 lines · Magnus Damm
Quoted 18 lines · Ben Dooks
Quoted 17 lines · Magnus Damm
From: Magnus Damm <[email protected]>
This patch makes the i2c-sh_mobile driver get the clock name from the struct resource with type
IORESOURCE_CLKprovided by the platform data.Signed-off-by Magnus Damm <[email protected]>---
drivers/i2c/busses/i2c-sh_mobile.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 renamed from drivers/i2c/busses/i2c-sh_mobile.c +0
-0@@ -390,13 +390,19 @@ static int sh_mobile_i2c_probe(struct pl int size; int ret;
Quoted 30 lines
Quoted 5 lines
Quoted 5 lines
drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 static int sh_mobile_i2c_probe(struct pl · line 397
Quoted 5 lines
drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 renamed from drivers/i2c/busses/i2c-sh_mobile.c +5
-0+ res = platform_get_resource(dev, IORESOURCE_CLK, 0); + if (res == NULL || res->name == NULL) { + dev_err(&dev->dev, "cannot find CLK resource\n"); + return -ENOENT; + }I note in this one you are re-using the resource.name field in the way it was not intended to be used. This field is for use to differentiate resources where this is more than one of them and they may not all be present in the list. This is not a good course of action.
Quoted 19 lines
Quoted 11 lines
drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 static int sh_mobile_i2c_probe(struct pl · line 399
Quoted 7 lines
drivers/i2c/busses/i2c-sh_mobile.c 2008-07-18 14:56:40.000000000 +0900 renamed from drivers/i2c/busses/i2c-sh_mobile.c +0
-1pd = kzalloc(sizeof(struct sh_mobile_i2c_data), GFP_KERNEL); if (pd == NULL) { dev_err(&dev->dev, "cannot allocate private data\n"); return -ENOMEM; } - pd->clk = clk_get(&dev->dev, "peripheral_clk");I think that is working correctly and there isn't really any need to change this. The
clk_getis supplied the device that it needs the clock for, and the name of the clock needed.Right, we could handle this "under the hood" of the clock frame work implementation, or we could deal with it together with the rest of the platform data and have one unique string per hardware block. On SuperH Mobile we currently have one shared clock implementation that supports multiple processors. Which clock that is assigned to what hardware block is currently handled by per-cpu platform data, and that's where this patch comes in.
The basic problem is that you're using names to identify clock sources, not clock consumers. If you consult the API documentation, it is made pretty clear that using the ID string to identify clock sources is wrong:
* clk_get - lookup and obtain a reference to a clock producer.
* @dev: device for clock "consumer"
* @id: clock comsumer ID
*
* Returns a struct clk corresponding to the clock producer, or
* valid IS_ERR() condition containing errno. The implementation
* uses @dev and @id to determine the clock consumer, and thereby
* the clock producer. (IOW, @id may be identical strings, but
* clk_get may return different clock producers depending on @dev.)
The problem with using the ID as a clock source is what you're finding - you have to pass names around through various structures.
Instead, the way we do this on ARM is to use the device and ID to determine the clock source, using aliases if necessary.
So, for example, all UART clocks may be called 'UARTCLK' but may actually be different clock sources, which are told apart by the struct device passed in.
Really, the struct device is the _primary_ distinguishing parameter between clock sources. The ID is meant to distinguish between different inputs on the device itself.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
PATCH 5/5 sh: add IORESOURCE_CLK to SuperH Mobile I2C platform data
In reply to Magnus DammFrom: Magnus Damm <[email protected]>
This patch passes processor specific IORESOURCE_CLK information to the i2c-sh_mobile driver. With this change the clock to the IIC block will be disabled while the I2C bus is inactive. Without this patch the clock is always on.
---
Applies to 6b4b0121a22649640e997b17dd00bb40e97e254d of sh-2.6 git
arch/sh/kernel/cpu/sh4a/setup-sh7343.c | 14 ++++++++++---- arch/sh/kernel/cpu/sh4a/setup-sh7366.c | 7 +++++-- arch/sh/kernel/cpu/sh4a/setup-sh7722.c | 7 +++++-- arch/sh/kernel/cpu/sh4a/setup-sh7723.c | 7 +++++-- 4 files changed, 25 insertions(+), 10 deletions(-)
arch/sh/kernel/cpu/sh4a/setup-sh7343.c +10 -4
@@ -25,7 +25,11 @@ static struct resource iic0_resources[] .start = 96, .end = 99, .flags = IORESOURCE_IRQ, - }, + }, + [2] = { + .name = "mstp109", + .flags = IORESOURCE_CLK, + }, }; static struct platform_device iic0_device = {
@@ -45,7 +49,11 @@ static struct resource iic1_resources[] .start = 44, .end = 47, .flags = IORESOURCE_IRQ, - }, + }, + [2] = { + .name = "mstp108", + .flags = IORESOURCE_CLK, + }, }; static struct platform_device iic1_device = {
@@ -147,8 +155,6 @@ static int __init sh7343_devices_setup(vclk_always_enable ("mstp023" );/* INTC3 */ clk_always_enable ("mstp022" );/* INTC */ clk_always_enable ("mstp020" );/* SuperHyway */ -clk_always_enable ("mstp109" );/* I2C0 */ -clk_always_enable ("mstp108" );/* I2C1 */ clk_always_enable ("mstp202" );/* VEU */ clk_always_enable ("mstp201" );/* VPU */
arch/sh/kernel/cpu/sh4a/setup-sh7366.c +5 -2
@@ -27,7 +27,11 @@ static struct resource iic_resources[] = .start = 96, .end = 99, .flags = IORESOURCE_IRQ, - }, + }, + [2] = { + .name = "mstp109", + .flags = IORESOURCE_CLK, + }, }; static struct platform_device iic_device = {
@@ -157,7 +161,6 @@ static int __init sh7366_devices_setup(vclk_always_enable ("mstp023" );/* INTC3 */ clk_always_enable ("mstp022" );/* INTC */ clk_always_enable ("mstp020" );/* SuperHyway */ -clk_always_enable ("mstp109" );/* I2C */ clk_always_enable ("mstp207" );/* VEU-2 */ clk_always_enable ("mstp202" );/* VEU-1 */ clk_always_enable ("mstp201" );/* VPU */
arch/sh/kernel/cpu/sh4a/setup-sh7722.c +5 -2
@@ -52,7 +52,11 @@ static struct resource iic_resources[] = .start = 96, .end = 99, .flags = IORESOURCE_IRQ, - }, + }, + [2] = { + .name = "mstp109", + .flags = IORESOURCE_CLK, + }, }; static struct platform_device iic_device = {
@@ -166,7 +170,6 @@ static int __init sh7722_devices_setup(vclk_always_enable ("mstp026" );/* XYMEM */ clk_always_enable ("mstp022" );/* INTC */ clk_always_enable ("mstp020" );/* SuperHyway */ -clk_always_enable ("mstp109" );/* I2C */ clk_always_enable ("mstp211" );/* USB */ clk_always_enable ("mstp202" );/* VEU */ clk_always_enable ("mstp201" );/* VPU */
arch/sh/kernel/cpu/sh4a/setup-sh7723.c +5 -2
@@ -210,7 +210,11 @@ static struct resource iic_resources[] = .start = 96, .end = 99, .flags = IORESOURCE_IRQ, - }, + }, + [2] = { + .name = "mstp109", + .flags = IORESOURCE_CLK, + }, }; static struct platform_device iic_device = {
@@ -238,7 +242,6 @@ static int __init sh7723_devices_setup(vclk_always_enable ("mstp022" );/* INTC */ clk_always_enable ("mstp020" );/* SuperHyway */ clk_always_enable ("mstp000" );/* MERAM */ -clk_always_enable ("mstp109" );/* I2C */ clk_always_enable ("mstp108" );/* RTC */ clk_always_enable ("mstp211" );/* USB */ clk_always_enable ("mstp206" );/* VEU2H1 */
Re: [PATCH 05/05] sh: add IORESOURCE_CLK to SuperH Mobile I2C platform data
Quoted 9 lines · On Fri, 18 Jul 2008 16:40:44 +0900 Magnus Damm <[email protected]> wrote:
This patch passes processor specific
IORESOURCE_CLKinformation to the i2c-sh_mobile driver. With this change the clock to the IIC block will be disabled while the I2C bus is inactive. Without this patch the clock is always on.Signed-off-by Magnus Damm <[email protected]>---
Applies to
6b4b0121a22649640e997b17dd00bb40e97e254dof sh-2.6 git
I can't get this to even vaguely apply to linux-next from 20 minutes ago.