[PATCH 00/05] resource: type, size and IORESOURCE_CLK patches V2

[PATCH 00/05] resource: type, size and IORESOURCE_CLK patches V2

[Magnus Damm]

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

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

 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 01/05] resource: add resource_size()

[Magnus Damm]

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

Avoid one-off errors by introducing a resource_size() function.

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

 include/linux/ioport.h |    4 ++++
 kernel/resource.c      |    2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

--- 0001/include/linux/ioport.h
+++ work/include/linux/ioport.h	2008-07-09 12:59:04.000000000 +0900
@@ -113,6 +113,10 @@ extern int allocate_resource(struct reso
 int 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))
--- 0001/kernel/resource.c
+++ work/kernel/resource.c	2008-07-09 12:59:41.000000000 +0900
@@ -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()

[Ben Dooks]

On Fri, Jul 18, 2008 at 04:40:10PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> Avoid one-off errors by introducing a resource_size() function.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>

A resource_size definition is a good idea, given the number of
times it has been re-implemented throughout the kernel.

> ---
> 
>  include/linux/ioport.h |    4 ++++
>  kernel/resource.c      |    2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> --- 0001/include/linux/ioport.h
> +++ work/include/linux/ioport.h	2008-07-09 12:59:04.000000000 +0900
> @@ -113,6 +113,10 @@ extern int allocate_resource(struct reso
>  int 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))
> --- 0001/kernel/resource.c
> +++ work/kernel/resource.c	2008-07-09 12:59:41.000000000 +0900
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

[PATCH 02/05] resource: add resource_type() and IORESOURCE_TYPE_BITS

[Magnus Damm]

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

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.

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

 drivers/base/platform.c |   31 +++++++++++++++++--------------
 include/linux/ioport.h  |    7 ++++++-
 2 files changed, 23 insertions(+), 15 deletions(-)

--- 0001/drivers/base/platform.c
+++ work/drivers/base/platform.c	2008-07-09 20:19:16.000000000 +0900
@@ -42,10 +42,8 @@ struct resource *platform_get_resource(s
 	for (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_b
 	for (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 platform
 
 		for (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);
 		}
 	}
--- 0002/include/linux/ioport.h
+++ work/include/linux/ioport.h	2008-07-09 15:16:48.000000000 +0900
@@ -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

[Ben Dooks]

On Fri, Jul 18, 2008 at 04:40:18PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> 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.

This looks ok to me, with one concern as shown below.

> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  drivers/base/platform.c |   31 +++++++++++++++++--------------
>  include/linux/ioport.h  |    7 ++++++-
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 
> --- 0001/drivers/base/platform.c
> +++ work/drivers/base/platform.c	2008-07-09 20:19:16.000000000 +0900
> @@ -42,10 +42,8 @@ struct resource *platform_get_resource(s
>  	for (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_b
>  	for (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;
>  		}

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?
  
> @@ -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 platform
>  
>  		for (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);
>  		}
>  	}
> --- 0002/include/linux/ioport.h
> +++ work/include/linux/ioport.h	2008-07-09 15:16:48.000000000 +0900
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH 02/05] resource: add resource_type() and IORESOURCE_TYPE_BITS

[Magnus Damm]

On Fri, Jul 18, 2008 at 4:56 PM, Ben Dooks <ben-linux@fluff.org> wrote:
> 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?

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

[Ben Dooks]

On Fri, Jul 18, 2008 at 05:24:59PM +0900, Magnus Damm wrote:
> On Fri, Jul 18, 2008 at 4:56 PM, Ben Dooks <ben-linux@fluff.org> wrote:
> > 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?
> 
> 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... 

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH 02/05] resource: add resource_type() and IORESOURCE_TYPE_BITS

[Magnus Damm]

On Fri, Jul 18, 2008 at 5:33 PM, Ben Dooks <ben-linux@fluff.org> wrote:
> On Fri, Jul 18, 2008 at 05:24:59PM +0900, Magnus Damm wrote:
>> On Fri, Jul 18, 2008 at 4:56 PM, Ben Dooks <ben-linux@fluff.org> wrote:
>> > 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?
>>
>> 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 03/05] resource: add new IORESOURCE_CLK type V2

[Magnus Damm]

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

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.

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

 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(-)

--- 0003/include/linux/ioport.h
+++ work/include/linux/ioport.h	2008-07-18 14:29:48.000000000 +0900
@@ -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

[Ben Dooks]

On Fri, Jul 18, 2008 at 04:40:27PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> 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.

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.
 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  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(-)
> 
> --- 0003/include/linux/ioport.h
> +++ work/include/linux/ioport.h	2008-07-18 14:29:48.000000000 +0900
> @@ -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
> i2c@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/i2c

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [i2c] [PATCH 03/05] resource: add new IORESOURCE_CLK type V2

[Magnus Damm]

On Fri, Jul 18, 2008 at 4:53 PM, Ben Dooks <ben-linux@fluff.org> wrote:
> On Fri, Jul 18, 2008 at 04:40:27PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> 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.
>
> 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.

> 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 04/05] i2c-sh_mobile: IORESOURCE_CLK support

[Magnus Damm]

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

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.

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

 drivers/i2c/busses/i2c-sh_mobile.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--- 0001/drivers/i2c/busses/i2c-sh_mobile.c
+++ work/drivers/i2c/busses/i2c-sh_mobile.c	2008-07-18 14:56:40.000000000 +0900
@@ -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;
 	}
 
-	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

[Ben Dooks]

On Fri, Jul 18, 2008 at 04:40:36PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> 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.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  drivers/i2c/busses/i2c-sh_mobile.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> --- 0001/drivers/i2c/busses/i2c-sh_mobile.c
> +++ work/drivers/i2c/busses/i2c-sh_mobile.c	2008-07-18 14:56:40.000000000 +0900
> @@ -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;
>  	}
>  
> -	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.

> +	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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH 04/05] i2c-sh_mobile: IORESOURCE_CLK support

[Magnus Damm]

On Fri, Jul 18, 2008 at 5:04 PM, Ben Dooks <ben-linux@fluff.org> wrote:
> On Fri, Jul 18, 2008 at 04:40:36PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> 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.
>>
>> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> ---
>>
>>  drivers/i2c/busses/i2c-sh_mobile.c |    8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> --- 0001/drivers/i2c/busses/i2c-sh_mobile.c
>> +++ work/drivers/i2c/busses/i2c-sh_mobile.c   2008-07-18 14:56:40.000000000 +0900
>> @@ -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;
>>       }
>>
>> -     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.

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

[Ben Dooks]

On Fri, Jul 18, 2008 at 06:18:06PM +0900, Magnus Damm wrote:
> On Fri, Jul 18, 2008 at 5:04 PM, Ben Dooks <ben-linux@fluff.org> wrote:
> > On Fri, Jul 18, 2008 at 04:40:36PM +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm@igel.co.jp>
> >>
> >> 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.
> >>
> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> ---
> >>
> >>  drivers/i2c/busses/i2c-sh_mobile.c |    8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> --- 0001/drivers/i2c/busses/i2c-sh_mobile.c
> >> +++ work/drivers/i2c/busses/i2c-sh_mobile.c   2008-07-18 14:56:40.000000000 +0900
> >> @@ -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;
> >> +     }

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.

> >>       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.
> 
> 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.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [i2c] [PATCH 04/05] i2c-sh_mobile: IORESOURCE_CLK support

[Russell King]

On Wed, Aug 13, 2008 at 06:54:53AM +0100, Ben Dooks wrote:
> On Fri, Jul 18, 2008 at 06:18:06PM +0900, Magnus Damm wrote:
> > On Fri, Jul 18, 2008 at 5:04 PM, Ben Dooks <ben-linux@fluff.org> wrote:
> > > On Fri, Jul 18, 2008 at 04:40:36PM +0900, Magnus Damm wrote:
> > >> From: Magnus Damm <damm@igel.co.jp>
> > >>
> > >> 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.
> > >>
> > >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> > >> ---
> > >>
> > >>  drivers/i2c/busses/i2c-sh_mobile.c |    8 +++++++-
> > >>  1 file changed, 7 insertions(+), 1 deletion(-)
> > >>
> > >> --- 0001/drivers/i2c/busses/i2c-sh_mobile.c
> > >> +++ work/drivers/i2c/busses/i2c-sh_mobile.c   2008-07-18 14:56:40.000000000 +0900
> > >> @@ -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;
> > >> +     }
> 
> 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.
> 
> > >>       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.
> > 
> > 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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

[PATCH 05/05] sh: add IORESOURCE_CLK to SuperH Mobile I2C platform data

[Magnus Damm]

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

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.

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

 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(-)

--- 0001/arch/sh/kernel/cpu/sh4a/setup-sh7343.c
+++ work/arch/sh/kernel/cpu/sh4a/setup-sh7343.c	2008-07-18 14:37:33.000000000 +0900
@@ -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(v
 	clk_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 */
 
--- 0001/arch/sh/kernel/cpu/sh4a/setup-sh7366.c
+++ work/arch/sh/kernel/cpu/sh4a/setup-sh7366.c	2008-07-18 14:37:06.000000000 +0900
@@ -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(v
 	clk_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 */
--- 0001/arch/sh/kernel/cpu/sh4a/setup-sh7722.c
+++ work/arch/sh/kernel/cpu/sh4a/setup-sh7722.c	2008-07-18 14:35:43.000000000 +0900
@@ -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(v
 	clk_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 */
--- 0001/arch/sh/kernel/cpu/sh4a/setup-sh7723.c
+++ work/arch/sh/kernel/cpu/sh4a/setup-sh7723.c	2008-07-18 14:36:23.000000000 +0900
@@ -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(v
 	clk_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

[Andrew Morton]

On Fri, 18 Jul 2008 16:40:44 +0900
Magnus Damm <magnus.damm@gmail.com> wrote:

> 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.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Applies to 6b4b0121a22649640e997b17dd00bb40e97e254d of sh-2.6 git

I can't get this to even vaguely apply to linux-next from 20 minutes ago.