[PATCH] i2c: i2c-sh_mobile bus speed platform data

[PATCH] i2c: i2c-sh_mobile bus speed platform data

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Add support to the i2c-sh_mobile driver for setting
the I2C bus speed using platform data.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/i2c/busses/i2c-sh_mobile.c |   13 +++++++++++--
 include/linux/i2c-sh_mobile.h      |   10 ++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

--- 0013/drivers/i2c/busses/i2c-sh_mobile.c
+++ work/drivers/i2c/busses/i2c-sh_mobile.c	2011-04-21 20:20:23.000000000 +0900
@@ -32,6 +32,7 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/i2c-sh_mobile.h>
 
 /* Transmit operation:                                                      */
 /*                                                                          */
@@ -117,7 +118,7 @@ struct sh_mobile_i2c_data {
 	struct device *dev;
 	void __iomem *reg;
 	struct i2c_adapter adap;
-
+	unsigned long bus_speed;
 	struct clk *clk;
 	u_int8_t icic;
 	u_int8_t iccl;
@@ -205,7 +206,7 @@ static void activate_ch(struct sh_mobile
 	 * We also round off the result.
 	 */
 	num = i2c_clk * 5;
-	denom = NORMAL_SPEED * 9;
+	denom = pd->bus_speed * 9;
 	tmp = num * 10 / denom;
 	if (tmp % 10 >= 5)
 		pd->iccl = (u_int8_t)((num/denom) + 1);
@@ -574,6 +575,7 @@ static int sh_mobile_i2c_hook_irqs(struc
 
 static int sh_mobile_i2c_probe(struct platform_device *dev)
 {
+	struct i2c_sh_mobile_platform_data *pdata = dev->dev.platform_data;
 	struct sh_mobile_i2c_data *pd;
 	struct i2c_adapter *adap;
 	struct resource *res;
@@ -618,6 +620,11 @@ static int sh_mobile_i2c_probe(struct pl
 		goto err_irq;
 	}
 
+	/* Use platformd data bus speed or NORMAL_SPEED */
+	pd->bus_speed = NORMAL_SPEED;
+	if (pdata && pdata->bus_speed)
+		pd->bus_speed = pdata->bus_speed;
+
 	/* The IIC blocks on SH-Mobile ARM processors
 	 * come with two new bits in ICIC.
 	 */
@@ -658,6 +665,8 @@ static int sh_mobile_i2c_probe(struct pl
 		goto err_all;
 	}
 
+	dev_info(&dev->dev, "I2C adapter %d with bus speed %lu Hz\n",
+		 adap->nr, pd->bus_speed);
 	return 0;
 
  err_all:
--- /dev/null
+++ work/include/linux/i2c-sh_mobile.h	2011-04-21 20:06:59.000000000 +0900
@@ -0,0 +1,10 @@
+#ifndef __I2C_SH_MOBILE_H__
+#define __I2C_SH_MOBILE_H__
+
+#include <linux/platform_device.h>
+
+struct i2c_sh_mobile_platform_data {
+	unsigned long bus_speed;
+};
+
+#endif /* __I2C_SH_MOBILE_H__ */

Re: [PATCH] i2c: i2c-sh_mobile bus speed platform data

[Simon Horman]

Hi Magnus,

On Thu, Apr 21, 2011 at 10:22:54PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Add support to the i2c-sh_mobile driver for setting
> the I2C bus speed using platform data.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  drivers/i2c/busses/i2c-sh_mobile.c |   13 +++++++++++--
>  include/linux/i2c-sh_mobile.h      |   10 ++++++++++
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> --- 0013/drivers/i2c/busses/i2c-sh_mobile.c
> +++ work/drivers/i2c/busses/i2c-sh_mobile.c	2011-04-21 20:20:23.000000000 +0900
> @@ -32,6 +32,7 @@
>  #include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> +#include <linux/i2c-sh_mobile.h>
>  
>  /* Transmit operation:                                                      */
>  /*                                                                          */
> @@ -117,7 +118,7 @@ struct sh_mobile_i2c_data {
>  	struct device *dev;
>  	void __iomem *reg;
>  	struct i2c_adapter adap;
> -
> +	unsigned long bus_speed;
>  	struct clk *clk;
>  	u_int8_t icic;
>  	u_int8_t iccl;
> @@ -205,7 +206,7 @@ static void activate_ch(struct sh_mobile
>  	 * We also round off the result.
>  	 */
>  	num = i2c_clk * 5;
> -	denom = NORMAL_SPEED * 9;
> +	denom = pd->bus_speed * 9;

I believe that on the sh73a0 and so far only the sh73a0
denom needs to be doubled.

>  	tmp = num * 10 / denom;
>  	if (tmp % 10 >= 5)
>  		pd->iccl = (u_int8_t)((num/denom) + 1);
> @@ -574,6 +575,7 @@ static int sh_mobile_i2c_hook_irqs(struc
>  
>  static int sh_mobile_i2c_probe(struct platform_device *dev)
>  {
> +	struct i2c_sh_mobile_platform_data *pdata = dev->dev.platform_data;
>  	struct sh_mobile_i2c_data *pd;
>  	struct i2c_adapter *adap;
>  	struct resource *res;
> @@ -618,6 +620,11 @@ static int sh_mobile_i2c_probe(struct pl
>  		goto err_irq;
>  	}
>  
> +	/* Use platformd data bus speed or NORMAL_SPEED */
> +	pd->bus_speed = NORMAL_SPEED;
> +	if (pdata && pdata->bus_speed)
> +		pd->bus_speed = pdata->bus_speed;
> +
>  	/* The IIC blocks on SH-Mobile ARM processors
>  	 * come with two new bits in ICIC.
>  	 */
> @@ -658,6 +665,8 @@ static int sh_mobile_i2c_probe(struct pl
>  		goto err_all;
>  	}
>  
> +	dev_info(&dev->dev, "I2C adapter %d with bus speed %lu Hz\n",
> +		 adap->nr, pd->bus_speed);
>  	return 0;
>  
>   err_all:
> --- /dev/null
> +++ work/include/linux/i2c-sh_mobile.h	2011-04-21 20:06:59.000000000 +0900
> @@ -0,0 +1,10 @@
> +#ifndef __I2C_SH_MOBILE_H__
> +#define __I2C_SH_MOBILE_H__
> +
> +#include <linux/platform_device.h>
> +
> +struct i2c_sh_mobile_platform_data {
> +	unsigned long bus_speed;
> +};
> +
> +#endif /* __I2C_SH_MOBILE_H__ */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] i2c: i2c-sh_mobile bus speed platform data

[Magnus Damm]

On Thu, Apr 28, 2011 at 7:14 AM, Simon Horman <horms@verge.net.au> wrote:
> Hi Magnus,
>
> On Thu, Apr 21, 2011 at 10:22:54PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> Add support to the i2c-sh_mobile driver for setting
>> the I2C bus speed using platform data.
>>
>> Signed-off-by: Magnus Damm <damm@opensource.se>
>> ---
>>
>>  drivers/i2c/busses/i2c-sh_mobile.c |   13 +++++++++++--
>>  include/linux/i2c-sh_mobile.h      |   10 ++++++++++
>>  2 files changed, 21 insertions(+), 2 deletions(-)
>>
>> --- 0013/drivers/i2c/busses/i2c-sh_mobile.c
>> +++ work/drivers/i2c/busses/i2c-sh_mobile.c   2011-04-21 20:20:23.000000000 +0900
>> @@ -32,6 +32,7 @@
>>  #include <linux/clk.h>
>>  #include <linux/io.h>
>>  #include <linux/slab.h>
>> +#include <linux/i2c-sh_mobile.h>
>>
>>  /* Transmit operation:                                                      */
>>  /*                                                                          */
>> @@ -117,7 +118,7 @@ struct sh_mobile_i2c_data {
>>       struct device *dev;
>>       void __iomem *reg;
>>       struct i2c_adapter adap;
>> -
>> +     unsigned long bus_speed;
>>       struct clk *clk;
>>       u_int8_t icic;
>>       u_int8_t iccl;
>> @@ -205,7 +206,7 @@ static void activate_ch(struct sh_mobile
>>        * We also round off the result.
>>        */
>>       num = i2c_clk * 5;
>> -     denom = NORMAL_SPEED * 9;
>> +     denom = pd->bus_speed * 9;
>
> I believe that on the sh73a0 and so far only the sh73a0
> denom needs to be doubled.

Uhm, I don't think this patch is specific to any SoC type. It may of
course be used on sh73a0 to adjust the denom value, but setting the
I2C bus speed is something that can be used on any SoC. So I'd say
that this is a fairly generic feature.

Thanks,

/ magnus

Re: [PATCH] i2c: i2c-sh_mobile bus speed platform data

[Simon Horman]

On Thu, Apr 28, 2011 at 10:06:38AM +0900, Magnus Damm wrote:
> On Thu, Apr 28, 2011 at 7:14 AM, Simon Horman <horms@verge.net.au> wrote:
> > Hi Magnus,
> >
> > On Thu, Apr 21, 2011 at 10:22:54PM +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >>
> >> Add support to the i2c-sh_mobile driver for setting
> >> the I2C bus speed using platform data.
> >>
> >> Signed-off-by: Magnus Damm <damm@opensource.se>
> >> ---
> >>
> >>  drivers/i2c/busses/i2c-sh_mobile.c |   13 +++++++++++--
> >>  include/linux/i2c-sh_mobile.h      |   10 ++++++++++
> >>  2 files changed, 21 insertions(+), 2 deletions(-)
> >>
> >> --- 0013/drivers/i2c/busses/i2c-sh_mobile.c
> >> +++ work/drivers/i2c/busses/i2c-sh_mobile.c   2011-04-21 20:20:23.000000000 +0900
> >> @@ -32,6 +32,7 @@
> >>  #include <linux/clk.h>
> >>  #include <linux/io.h>
> >>  #include <linux/slab.h>
> >> +#include <linux/i2c-sh_mobile.h>
> >>
> >>  /* Transmit operation:                                                      */
> >>  /*                                                                          */
> >> @@ -117,7 +118,7 @@ struct sh_mobile_i2c_data {
> >>       struct device *dev;
> >>       void __iomem *reg;
> >>       struct i2c_adapter adap;
> >> -
> >> +     unsigned long bus_speed;
> >>       struct clk *clk;
> >>       u_int8_t icic;
> >>       u_int8_t iccl;
> >> @@ -205,7 +206,7 @@ static void activate_ch(struct sh_mobile
> >>        * We also round off the result.
> >>        */
> >>       num = i2c_clk * 5;
> >> -     denom = NORMAL_SPEED * 9;
> >> +     denom = pd->bus_speed * 9;
> >
> > I believe that on the sh73a0 and so far only the sh73a0
> > denom needs to be doubled.
> 
> Uhm, I don't think this patch is specific to any SoC type. It may of
> course be used on sh73a0 to adjust the denom value, but setting the
> I2C bus speed is something that can be used on any SoC. So I'd say
> that this is a fairly generic feature.

I'm just saying that that I've observed the value being doubled for sh73a0.

Re: [PATCH] i2c: i2c-sh_mobile bus speed platform data

[Paul Mundt]

On Thu, Apr 28, 2011 at 10:18:01AM +0900, Simon Horman wrote:
> On Thu, Apr 28, 2011 at 10:06:38AM +0900, Magnus Damm wrote:
> > On Thu, Apr 28, 2011 at 7:14 AM, Simon Horman <horms@verge.net.au> wrote:
> > > I believe that on the sh73a0 and so far only the sh73a0
> > > denom needs to be doubled.
> > 
> > Uhm, I don't think this patch is specific to any SoC type. It may of
> > course be used on sh73a0 to adjust the denom value, but setting the
> > I2C bus speed is something that can be used on any SoC. So I'd say
> > that this is a fairly generic feature.
> 
> I'm just saying that that I've observed the value being doubled for sh73a0.
> 
The general rule of thumb is that whatever unusual behaviour is observed
in the latest CPU we will see become the standard for future ones.
Abstracting the denom value seems reasonable as a cautionary measure, and
it does do the NORMAL_SPEED as a default fallback so there is no adverse
impact for normalized platforms.

This really should be going in to include/linux/i2c/ or so though, no
need to dump these tiny stubs in to include/linux/ directly.

Re: [PATCH] i2c: i2c-sh_mobile bus speed platform data

[Magnus Damm]

On Thu, Apr 28, 2011 at 11:22 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Thu, Apr 28, 2011 at 10:18:01AM +0900, Simon Horman wrote:
>> On Thu, Apr 28, 2011 at 10:06:38AM +0900, Magnus Damm wrote:
>> > On Thu, Apr 28, 2011 at 7:14 AM, Simon Horman <horms@verge.net.au> wrote:
>> > > I believe that on the sh73a0 and so far only the sh73a0
>> > > denom needs to be doubled.
>> >
>> > Uhm, I don't think this patch is specific to any SoC type. It may of
>> > course be used on sh73a0 to adjust the denom value, but setting the
>> > I2C bus speed is something that can be used on any SoC. So I'd say
>> > that this is a fairly generic feature.
>>
>> I'm just saying that that I've observed the value being doubled for sh73a0.
>>
> The general rule of thumb is that whatever unusual behaviour is observed
> in the latest CPU we will see become the standard for future ones.

This may also be an attempt to simply double the I2C bus speed on that
particular platform for that particular application. So in the end it
may have nothing to do with sh73a0. Actually, now when I think about
it, I recall hacking up a prototype to control the LCD backlight via
I2C on sh73a0 and AG5EVM, and I did not have to modify any part of the
I2C bus driver to get that going as expected.

> Abstracting the denom value seems reasonable as a cautionary measure, and
> it does do the NORMAL_SPEED as a default fallback so there is no adverse
> impact for normalized platforms.
>
> This really should be going in to include/linux/i2c/ or so though, no
> need to dump these tiny stubs in to include/linux/ directly.

I guess I just followed what other i2c drivers do, but yes, putting
the header file in include/linux/i2c/ makes more sense.

I'll cook up a V2.

Thanks,

/ magnus

Re: [PATCH] i2c: i2c-sh_mobile bus speed platform data

[Paul Mundt]

On Thu, Apr 28, 2011 at 11:46:37AM +0900, Magnus Damm wrote:
> On Thu, Apr 28, 2011 at 11:22 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> > On Thu, Apr 28, 2011 at 10:18:01AM +0900, Simon Horman wrote:
> >> On Thu, Apr 28, 2011 at 10:06:38AM +0900, Magnus Damm wrote:
> >> > On Thu, Apr 28, 2011 at 7:14 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > > I believe that on the sh73a0 and so far only the sh73a0
> >> > > denom needs to be doubled.
> >> >
> >> > Uhm, I don't think this patch is specific to any SoC type. It may of
> >> > course be used on sh73a0 to adjust the denom value, but setting the
> >> > I2C bus speed is something that can be used on any SoC. So I'd say
> >> > that this is a fairly generic feature.
> >>
> >> I'm just saying that that I've observed the value being doubled for sh73a0.
> >>
> > The general rule of thumb is that whatever unusual behaviour is observed
> > in the latest CPU we will see become the standard for future ones.
> 
> This may also be an attempt to simply double the I2C bus speed on that
> particular platform for that particular application. So in the end it
> may have nothing to do with sh73a0. Actually, now when I think about
> it, I recall hacking up a prototype to control the LCD backlight via
> I2C on sh73a0 and AG5EVM, and I did not have to modify any part of the
> I2C bus driver to get that going as expected.
> 
Ok, that bit of information was missing from your patch. That's obviously
a bit more dodgy. We don't want to have the default behaviour out of spec
for some specific application.

> > Abstracting the denom value seems reasonable as a cautionary measure, and
> > it does do the NORMAL_SPEED as a default fallback so there is no adverse
> > impact for normalized platforms.
> >
> > This really should be going in to include/linux/i2c/ or so though, no
> > need to dump these tiny stubs in to include/linux/ directly.
> 
> I guess I just followed what other i2c drivers do, but yes, putting
> the header file in include/linux/i2c/ makes more sense.
> 
Other i2c drivers put things in include/linux/i2c, too. There's also an
include/linux/platform_data/ now as well for things that don't fit
anywhere else. "This is the way we used to do it" tends to be a pretty
pointless argument going forward.

Re: [PATCH] i2c: i2c-sh_mobile bus speed platform data

[Magnus Damm]

On Thu, Apr 28, 2011 at 12:11 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Thu, Apr 28, 2011 at 11:46:37AM +0900, Magnus Damm wrote:
>> On Thu, Apr 28, 2011 at 11:22 AM, Paul Mundt <lethal@linux-sh.org> wrote:
>> > On Thu, Apr 28, 2011 at 10:18:01AM +0900, Simon Horman wrote:
>> >> On Thu, Apr 28, 2011 at 10:06:38AM +0900, Magnus Damm wrote:
>> >> > On Thu, Apr 28, 2011 at 7:14 AM, Simon Horman <horms@verge.net.au> wrote:
>> >> > > I believe that on the sh73a0 and so far only the sh73a0
>> >> > > denom needs to be doubled.
>> >> >
>> >> > Uhm, I don't think this patch is specific to any SoC type. It may of
>> >> > course be used on sh73a0 to adjust the denom value, but setting the
>> >> > I2C bus speed is something that can be used on any SoC. So I'd say
>> >> > that this is a fairly generic feature.
>> >>
>> >> I'm just saying that that I've observed the value being doubled for sh73a0.
>> >>
>> > The general rule of thumb is that whatever unusual behaviour is observed
>> > in the latest CPU we will see become the standard for future ones.
>>
>> This may also be an attempt to simply double the I2C bus speed on that
>> particular platform for that particular application. So in the end it
>> may have nothing to do with sh73a0. Actually, now when I think about
>> it, I recall hacking up a prototype to control the LCD backlight via
>> I2C on sh73a0 and AG5EVM, and I did not have to modify any part of the
>> I2C bus driver to get that going as expected.
>>
> Ok, that bit of information was missing from your patch. That's obviously
> a bit more dodgy. We don't want to have the default behaviour out of spec
> for some specific application.

I suspect that I may have chosen my words poorly. By writing "This may
also be an attempt to simply double the I2C bus speed on that
particular platform for that particular application." I am not
referring to this patch. I am commenting on something Simon may see
when he is going though kernel source that has been mangled by people
doing integration.

As for configuring the i2c bus speed, I wouldn't call it dodgy. It is
something that is fairly common but of course needs to be handled with
care. Other drivers handle this through module parameters, but since
we want to configure this with per-device instance granularity
platform data is a better fit.

>> > Abstracting the denom value seems reasonable as a cautionary measure, and
>> > it does do the NORMAL_SPEED as a default fallback so there is no adverse
>> > impact for normalized platforms.
>> >
>> > This really should be going in to include/linux/i2c/ or so though, no
>> > need to dump these tiny stubs in to include/linux/ directly.
>>
>> I guess I just followed what other i2c drivers do, but yes, putting
>> the header file in include/linux/i2c/ makes more sense.
>>
> Other i2c drivers put things in include/linux/i2c, too. There's also an
> include/linux/platform_data/ now as well for things that don't fit
> anywhere else. "This is the way we used to do it" tends to be a pretty
> pointless argument going forward.

I looks like I have been unclear in my writing once again. I'm not
arguing that the header should be kept in include/linux/.

/ magnus

[PATCH] i2c: i2c-sh_mobile bus speed platform data V2

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Add support to the i2c-sh_mobile driver for setting
the I2C bus speed using platform data.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 Changes since V1:
 - Moved header file from include/linux/ to include/linux/i2c/

 drivers/i2c/busses/i2c-sh_mobile.c |   13 +++++++++++--
 include/linux/i2c/i2c-sh_mobile.h  |   10 ++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

--- 0003/drivers/i2c/busses/i2c-sh_mobile.c
+++ work/drivers/i2c/busses/i2c-sh_mobile.c	2011-04-28 13:21:51.000000000 +0900
@@ -32,6 +32,7 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/i2c/i2c-sh_mobile.h>
 
 /* Transmit operation:                                                      */
 /*                                                                          */
@@ -117,7 +118,7 @@ struct sh_mobile_i2c_data {
 	struct device *dev;
 	void __iomem *reg;
 	struct i2c_adapter adap;
-
+	unsigned long bus_speed;
 	struct clk *clk;
 	u_int8_t icic;
 	u_int8_t iccl;
@@ -205,7 +206,7 @@ static void activate_ch(struct sh_mobile
 	 * We also round off the result.
 	 */
 	num = i2c_clk * 5;
-	denom = NORMAL_SPEED * 9;
+	denom = pd->bus_speed * 9;
 	tmp = num * 10 / denom;
 	if (tmp % 10 >= 5)
 		pd->iccl = (u_int8_t)((num/denom) + 1);
@@ -574,6 +575,7 @@ static int sh_mobile_i2c_hook_irqs(struc
 
 static int sh_mobile_i2c_probe(struct platform_device *dev)
 {
+	struct i2c_sh_mobile_platform_data *pdata = dev->dev.platform_data;
 	struct sh_mobile_i2c_data *pd;
 	struct i2c_adapter *adap;
 	struct resource *res;
@@ -618,6 +620,11 @@ static int sh_mobile_i2c_probe(struct pl
 		goto err_irq;
 	}
 
+	/* Use platformd data bus speed or NORMAL_SPEED */
+	pd->bus_speed = NORMAL_SPEED;
+	if (pdata && pdata->bus_speed)
+		pd->bus_speed = pdata->bus_speed;
+
 	/* The IIC blocks on SH-Mobile ARM processors
 	 * come with two new bits in ICIC.
 	 */
@@ -658,6 +665,8 @@ static int sh_mobile_i2c_probe(struct pl
 		goto err_all;
 	}
 
+	dev_info(&dev->dev, "I2C adapter %d with bus speed %lu Hz\n",
+		 adap->nr, pd->bus_speed);
 	return 0;
 
  err_all:
--- /dev/null
+++ work/include/linux/i2c/i2c-sh_mobile.h	2011-04-28 12:21:45.000000000 +0900
@@ -0,0 +1,10 @@
+#ifndef __I2C_SH_MOBILE_H__
+#define __I2C_SH_MOBILE_H__
+
+#include <linux/platform_device.h>
+
+struct i2c_sh_mobile_platform_data {
+	unsigned long bus_speed;
+};
+
+#endif /* __I2C_SH_MOBILE_H__ */

Re: [PATCH] i2c: i2c-sh_mobile bus speed platform data

[Simon Horman]

On Thu, Apr 28, 2011 at 12:25:30PM +0900, Magnus Damm wrote:
> On Thu, Apr 28, 2011 at 12:11 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > On Thu, Apr 28, 2011 at 11:46:37AM +0900, Magnus Damm wrote:
> >> On Thu, Apr 28, 2011 at 11:22 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> >> > On Thu, Apr 28, 2011 at 10:18:01AM +0900, Simon Horman wrote:
> >> >> On Thu, Apr 28, 2011 at 10:06:38AM +0900, Magnus Damm wrote:
> >> >> > On Thu, Apr 28, 2011 at 7:14 AM, Simon Horman <horms@verge.net.au> wrote:
> >> >> > > I believe that on the sh73a0 and so far only the sh73a0
> >> >> > > denom needs to be doubled.
> >> >> >
> >> >> > Uhm, I don't think this patch is specific to any SoC type. It may of
> >> >> > course be used on sh73a0 to adjust the denom value, but setting the
> >> >> > I2C bus speed is something that can be used on any SoC. So I'd say
> >> >> > that this is a fairly generic feature.
> >> >>
> >> >> I'm just saying that that I've observed the value being doubled for sh73a0.
> >> >>
> >> > The general rule of thumb is that whatever unusual behaviour is observed
> >> > in the latest CPU we will see become the standard for future ones.
> >>
> >> This may also be an attempt to simply double the I2C bus speed on that
> >> particular platform for that particular application. So in the end it
> >> may have nothing to do with sh73a0. Actually, now when I think about
> >> it, I recall hacking up a prototype to control the LCD backlight via
> >> I2C on sh73a0 and AG5EVM, and I did not have to modify any part of the
> >> I2C bus driver to get that going as expected.
> >>
> > Ok, that bit of information was missing from your patch. That's obviously
> > a bit more dodgy. We don't want to have the default behaviour out of spec
> > for some specific application.
> 
> I suspect that I may have chosen my words poorly. By writing "This may
> also be an attempt to simply double the I2C bus speed on that
> particular platform for that particular application." I am not
> referring to this patch. I am commenting on something Simon may see
> when he is going though kernel source that has been mangled by people
> doing integration.
> 
> As for configuring the i2c bus speed, I wouldn't call it dodgy. It is
> something that is fairly common but of course needs to be handled with
> care. Other drivers handle this through module parameters, but since
> we want to configure this with per-device instance granularity
> platform data is a better fit.

To clarify my position on this.

I merely observed a different implementation of essentially the same code
that doubles the value for the sh73a0. I am not entirely sure why it was
doubled. Nor am I sure why it was implemented outside of platform code.

However, I am sure that the exception that I noted is nowhere
in upstream code. So I think it would be fine to merge Magnus's changes.

If there is interest in resolving this sh73a0 anomaly then I guess
enquires will need to be made to the people with the alternate
implementation. That is probably a discussion best had off-list.

Re: [PATCH] i2c: i2c-sh_mobile bus speed platform data V2

[Simon Horman]

On Thu, Apr 28, 2011 at 01:25:36PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Add support to the i2c-sh_mobile driver for setting
> the I2C bus speed using platform data.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>

Acked-by: Simon Horman <horms@verge.net.au>

Re: [PATCH] i2c: i2c-sh_mobile bus speed platform data

[dong chen]

Magnus Damm <magnus.damm@gmail.com>编写：


On Thu, Apr 28, 2011 at 12:11 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Thu, Apr 28, 2011 at 11:46:37AM +0900, Magnus Damm wrote:
>> On Thu, Apr 28, 2011 at 11:22 AM, Paul Mundt <lethal@linux-sh.org> wrote:
>> > On Thu, Apr 28, 2011 at 10:18:01AM +0900, Simon Horman wrote:
>> >> On Thu, Apr 28, 2011 at 10:06:38AM +0900, Magnus Damm wrote:
>> >> > On Thu, Apr 28, 2011 at 7:14 AM, Simon Horman <horms@verge.net.au> wrote:
>> >> > > I believe that on the sh73a0 and so far only the sh73a0
>> >> > > denom needs to be doubled.
>> >> >
>> >> > Uhm, I don't think this patch is specific to any SoC type. It may of
>> >> > course be used on sh73a0 to adjust the denom value, but setting the
>> >> > I2C bus speed is something that can be used on any SoC. So I'd say
>> >> > that this is a fairly generic feature.
>> >>
>> >> I'm just saying that that I've observed the value being doubled for sh73a0.
>> >>
>> > The general rule of thumb is that whatever unusual behaviour is observed
>> > in the latest CPU we will see become the standard for future ones.
>>
>> This may also be an attempt to simply double the I2C bus speed on that
>> particular platform for that particular application. So in the end it
>> may have nothing to do with sh73a0. Actually, now when I think about
>> it, I recall hacking up a prototype to control the LCD backlight via
>> I2C on sh73a0 and AG5EVM, and I did not have to modify any part of the
>> I2C bus driver to get that going as expected.
>>
> Ok, that bit of information was missing from your patch. That's obviously
> a bit more dodgy. We don't want to have the default behaviour out of spec
> for some specific application.

I suspect that I may have chosen my words poorly. By writing "This may
also be an attempt to simply double the I2C bus speed on that
particular platform for that particular application." I am not
referring to this patch. I am commenting on something Simon may see
when he is going though kernel source that has been mangled by people
doing integration.

As for configuring the i2c bus speed, I wouldn't call it dodgy. It is
something that is fairly common but of course needs to be handled with
care. Other drivers handle this through module parameters, but since
we want to configure this with per-device instance granularity
platform data is a better fit.

>> > Abstracting the denom value seems reasonable as a cautionary measure, and
>> > it does do the NORMAL_SPEED as a default fallback so there is no adverse
>> > impact for normalized platforms.
>> >
>> > This really should be going in to include/linux/i2c/ or so though, no
>> > need to dump these tiny stubs in to include/linux/ directly.
>>
>> I guess I just followed what other i2c drivers do, but yes, putting
>> the header file in include/linux/i2c/ makes more sense.
>>
> Other i2c drivers put things in include/linux/i2c, too. There's also an
> include/linux/platform_data/ now as well for things that don't fit
> anywhere else. "This is the way we used to do it" tends to be a pretty
> pointless argument going forward.

I looks like I have been unclear in my writing once again. I'm not
arguing that the header should be kept in include/linux/.

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html