[PATCH] mach-pxa/viper: Fix timeout usage for I2C

[PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Wolfram Sang]

The timeout value is in jiffies, so it should be using HZ, not a plain
number. Assume '100' means 100ms here and adapt accordingly.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
---

Janitorial fix, not tested due to no hardware.

 arch/arm/mach-pxa/viper.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
index 1dd1334..c25921f 100644
--- a/arch/arm/mach-pxa/viper.c
+++ b/arch/arm/mach-pxa/viper.c
@@ -33,6 +33,7 @@
 #include <linux/pm.h>
 #include <linux/sched.h>
 #include <linux/gpio.h>
+#include <linux/jiffies.h>
 #include <linux/i2c-gpio.h>
 #include <linux/serial_8250.h>
 #include <linux/smc91x.h>
@@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
 	.sda_pin = VIPER_RTC_I2C_SDA_GPIO,
 	.scl_pin = VIPER_RTC_I2C_SCL_GPIO,
 	.udelay  = 10,
-	.timeout = 100,
+	.timeout = HZ / 10,
 };
 
 static struct platform_device i2c_bus_device = {
@@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
 		.sda_pin = VIPER_TPM_I2C_SDA_GPIO,
 		.scl_pin = VIPER_TPM_I2C_SCL_GPIO,
 		.udelay  = 10,
-		.timeout = 100,
+		.timeout = HZ / 10,
 	};
 	char *errstr;
 
-- 
1.7.0

[PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Wolfram Sang]

The timeout value is in jiffies, so it should be using HZ, not a plain
number. Assume '100' means 100ms here and adapt accordingly.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Marc Zyngier <maz@misterjones.org>
Cc: Paul Shen <paul.shen@marvell.com>
Cc: Mike Rapoport <mike@compulab.co.il>
---

Janitorial fix, not tested due to no hardware.

 arch/arm/mach-pxa/viper.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
index 1dd1334..c25921f 100644
--- a/arch/arm/mach-pxa/viper.c
+++ b/arch/arm/mach-pxa/viper.c
@@ -33,6 +33,7 @@
 #include <linux/pm.h>
 #include <linux/sched.h>
 #include <linux/gpio.h>
+#include <linux/jiffies.h>
 #include <linux/i2c-gpio.h>
 #include <linux/serial_8250.h>
 #include <linux/smc91x.h>
@@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
 	.sda_pin = VIPER_RTC_I2C_SDA_GPIO,
 	.scl_pin = VIPER_RTC_I2C_SCL_GPIO,
 	.udelay  = 10,
-	.timeout = 100,
+	.timeout = HZ / 10,
 };
 
 static struct platform_device i2c_bus_device = {
@@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
 		.sda_pin = VIPER_TPM_I2C_SDA_GPIO,
 		.scl_pin = VIPER_TPM_I2C_SCL_GPIO,
 		.udelay  = 10,
-		.timeout = 100,
+		.timeout = HZ / 10,
 	};
 	char *errstr;
 
-- 
1.7.0

Re: [PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Eric Miao]

On Sun, Apr 4, 2010 at 10:08 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> The timeout value is in jiffies, so it should be using HZ, not a plain
> number. Assume '100' means 100ms here and adapt accordingly.
>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
> Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
> ---
>
> Janitorial fix, not tested due to no hardware.
>
>  arch/arm/mach-pxa/viper.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
> index 1dd1334..c25921f 100644
> --- a/arch/arm/mach-pxa/viper.c
> +++ b/arch/arm/mach-pxa/viper.c
> @@ -33,6 +33,7 @@
>  #include <linux/pm.h>
>  #include <linux/sched.h>
>  #include <linux/gpio.h>
> +#include <linux/jiffies.h>
>  #include <linux/i2c-gpio.h>
>  #include <linux/serial_8250.h>
>  #include <linux/smc91x.h>
> @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
>        .sda_pin = VIPER_RTC_I2C_SDA_GPIO,
>        .scl_pin = VIPER_RTC_I2C_SCL_GPIO,
>        .udelay  = 10,
> -       .timeout = 100,
> +       .timeout = HZ / 10,
>  };
>
>  static struct platform_device i2c_bus_device = {
> @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
>                .sda_pin = VIPER_TPM_I2C_SDA_GPIO,
>                .scl_pin = VIPER_TPM_I2C_SCL_GPIO,
>                .udelay  = 10,
> -               .timeout = 100,
> +               .timeout = HZ / 10,
>        };
>        char *errstr;
>

One other better and cleaner approach to such inconsistency issue is
to have a timeout_ms field, and having i2c-gpio.c driver to convert this
to jiffies using msec_to_jiffies() at run-time.

[PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Eric Miao]

On Sun, Apr 4, 2010 at 10:08 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> The timeout value is in jiffies, so it should be using HZ, not a plain
> number. Assume '100' means 100ms here and adapt accordingly.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Eric Miao <eric.y.miao@gmail.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Marc Zyngier <maz@misterjones.org>
> Cc: Paul Shen <paul.shen@marvell.com>
> Cc: Mike Rapoport <mike@compulab.co.il>
> ---
>
> Janitorial fix, not tested due to no hardware.
>
> ?arch/arm/mach-pxa/viper.c | ? ?5 +++--
> ?1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
> index 1dd1334..c25921f 100644
> --- a/arch/arm/mach-pxa/viper.c
> +++ b/arch/arm/mach-pxa/viper.c
> @@ -33,6 +33,7 @@
> ?#include <linux/pm.h>
> ?#include <linux/sched.h>
> ?#include <linux/gpio.h>
> +#include <linux/jiffies.h>
> ?#include <linux/i2c-gpio.h>
> ?#include <linux/serial_8250.h>
> ?#include <linux/smc91x.h>
> @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
> ? ? ? ?.sda_pin = VIPER_RTC_I2C_SDA_GPIO,
> ? ? ? ?.scl_pin = VIPER_RTC_I2C_SCL_GPIO,
> ? ? ? ?.udelay ?= 10,
> - ? ? ? .timeout = 100,
> + ? ? ? .timeout = HZ / 10,
> ?};
>
> ?static struct platform_device i2c_bus_device = {
> @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
> ? ? ? ? ? ? ? ?.sda_pin = VIPER_TPM_I2C_SDA_GPIO,
> ? ? ? ? ? ? ? ?.scl_pin = VIPER_TPM_I2C_SCL_GPIO,
> ? ? ? ? ? ? ? ?.udelay ?= 10,
> - ? ? ? ? ? ? ? .timeout = 100,
> + ? ? ? ? ? ? ? .timeout = HZ / 10,
> ? ? ? ?};
> ? ? ? ?char *errstr;
>

One other better and cleaner approach to such inconsistency issue is
to have a timeout_ms field, and having i2c-gpio.c driver to convert this
to jiffies using msec_to_jiffies() at run-time.

Re: [PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Jean Delvare]

On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
> On Sun, Apr 4, 2010 at 10:08 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > The timeout value is in jiffies, so it should be using HZ, not a plain
> > number. Assume '100' means 100ms here and adapt accordingly.
> >
> > Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> > Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
> > Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
> > ---
> >
> > Janitorial fix, not tested due to no hardware.
> >
> >  arch/arm/mach-pxa/viper.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
> > index 1dd1334..c25921f 100644
> > --- a/arch/arm/mach-pxa/viper.c
> > +++ b/arch/arm/mach-pxa/viper.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/pm.h>
> >  #include <linux/sched.h>
> >  #include <linux/gpio.h>
> > +#include <linux/jiffies.h>
> >  #include <linux/i2c-gpio.h>
> >  #include <linux/serial_8250.h>
> >  #include <linux/smc91x.h>
> > @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
> >        .sda_pin = VIPER_RTC_I2C_SDA_GPIO,
> >        .scl_pin = VIPER_RTC_I2C_SCL_GPIO,
> >        .udelay  = 10,
> > -       .timeout = 100,
> > +       .timeout = HZ / 10,
> >  };
> >
> >  static struct platform_device i2c_bus_device = {
> > @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
> >                .sda_pin = VIPER_TPM_I2C_SDA_GPIO,
> >                .scl_pin = VIPER_TPM_I2C_SCL_GPIO,
> >                .udelay  = 10,
> > -               .timeout = 100,
> > +               .timeout = HZ / 10,
> >        };
> >        char *errstr;
> >
> 
> One other better and cleaner approach to such inconsistency issue is
> to have a timeout_ms field, and having i2c-gpio.c driver to convert this
> to jiffies using msec_to_jiffies() at run-time.

With what benefit? Expressing time values in units of HZ is very
frequent in the kernel code and shouldn't actually surprise anyone.

-- 
Jean Delvare

[PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Jean Delvare]

On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
> On Sun, Apr 4, 2010 at 10:08 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> > The timeout value is in jiffies, so it should be using HZ, not a plain
> > number. Assume '100' means 100ms here and adapt accordingly.
> >
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > Cc: Eric Miao <eric.y.miao@gmail.com>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Marc Zyngier <maz@misterjones.org>
> > Cc: Paul Shen <paul.shen@marvell.com>
> > Cc: Mike Rapoport <mike@compulab.co.il>
> > ---
> >
> > Janitorial fix, not tested due to no hardware.
> >
> > ?arch/arm/mach-pxa/viper.c | ? ?5 +++--
> > ?1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
> > index 1dd1334..c25921f 100644
> > --- a/arch/arm/mach-pxa/viper.c
> > +++ b/arch/arm/mach-pxa/viper.c
> > @@ -33,6 +33,7 @@
> > ?#include <linux/pm.h>
> > ?#include <linux/sched.h>
> > ?#include <linux/gpio.h>
> > +#include <linux/jiffies.h>
> > ?#include <linux/i2c-gpio.h>
> > ?#include <linux/serial_8250.h>
> > ?#include <linux/smc91x.h>
> > @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
> > ? ? ? ?.sda_pin = VIPER_RTC_I2C_SDA_GPIO,
> > ? ? ? ?.scl_pin = VIPER_RTC_I2C_SCL_GPIO,
> > ? ? ? ?.udelay ?= 10,
> > - ? ? ? .timeout = 100,
> > + ? ? ? .timeout = HZ / 10,
> > ?};
> >
> > ?static struct platform_device i2c_bus_device = {
> > @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
> > ? ? ? ? ? ? ? ?.sda_pin = VIPER_TPM_I2C_SDA_GPIO,
> > ? ? ? ? ? ? ? ?.scl_pin = VIPER_TPM_I2C_SCL_GPIO,
> > ? ? ? ? ? ? ? ?.udelay ?= 10,
> > - ? ? ? ? ? ? ? .timeout = 100,
> > + ? ? ? ? ? ? ? .timeout = HZ / 10,
> > ? ? ? ?};
> > ? ? ? ?char *errstr;
> >
> 
> One other better and cleaner approach to such inconsistency issue is
> to have a timeout_ms field, and having i2c-gpio.c driver to convert this
> to jiffies using msec_to_jiffies() at run-time.

With what benefit? Expressing time values in units of HZ is very
frequent in the kernel code and shouldn't actually surprise anyone.

-- 
Jean Delvare

Re: [PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Russell King - ARM Linux]

On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
> On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
> > On Sun, Apr 4, 2010 at 10:08 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> > > The timeout value is in jiffies, so it should be using HZ, not a plain
> > > number. Assume '100' means 100ms here and adapt accordingly.
> > >
> > > Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> > > Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
> > > Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > > Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
> > > ---
> > >
> > > Janitorial fix, not tested due to no hardware.
> > >
> > >  arch/arm/mach-pxa/viper.c |    5 +++--
> > >  1 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
> > > index 1dd1334..c25921f 100644
> > > --- a/arch/arm/mach-pxa/viper.c
> > > +++ b/arch/arm/mach-pxa/viper.c
> > > @@ -33,6 +33,7 @@
> > >  #include <linux/pm.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/gpio.h>
> > > +#include <linux/jiffies.h>
> > >  #include <linux/i2c-gpio.h>
> > >  #include <linux/serial_8250.h>
> > >  #include <linux/smc91x.h>
> > > @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
> > >        .sda_pin = VIPER_RTC_I2C_SDA_GPIO,
> > >        .scl_pin = VIPER_RTC_I2C_SCL_GPIO,
> > >        .udelay  = 10,
> > > -       .timeout = 100,
> > > +       .timeout = HZ / 10,
> > >  };
> > >
> > >  static struct platform_device i2c_bus_device = {
> > > @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
> > >                .sda_pin = VIPER_TPM_I2C_SDA_GPIO,
> > >                .scl_pin = VIPER_TPM_I2C_SCL_GPIO,
> > >                .udelay  = 10,
> > > -               .timeout = 100,
> > > +               .timeout = HZ / 10,
> > >        };
> > >        char *errstr;
> > >
> > 
> > One other better and cleaner approach to such inconsistency issue is
> > to have a timeout_ms field, and having i2c-gpio.c driver to convert this
> > to jiffies using msec_to_jiffies() at run-time.
> 
> With what benefit? Expressing time values in units of HZ is very
> frequent in the kernel code and shouldn't actually surprise anyone.

Actually, this patch shows there is confusion.

"Assume '100' means 100ms here and adapt accordingly."

Since this patch is for ARM, where HZ=100, the above patch is not a
simple "convert how we derive this constant" patch - it's a functional
change, reducing the timeouts by a factor of 10.

Could that be because the patch author misinterpreted the HZ-based
values?

I suspect I'm not the only one who thinks that the latter of "HZ / 10"
"100ms" is easier to read and comprehend without mistake.

[PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Russell King - ARM Linux]

On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
> On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
> > On Sun, Apr 4, 2010 at 10:08 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> > > The timeout value is in jiffies, so it should be using HZ, not a plain
> > > number. Assume '100' means 100ms here and adapt accordingly.
> > >
> > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > > Cc: Eric Miao <eric.y.miao@gmail.com>
> > > Cc: Russell King <linux@arm.linux.org.uk>
> > > Cc: Marc Zyngier <maz@misterjones.org>
> > > Cc: Paul Shen <paul.shen@marvell.com>
> > > Cc: Mike Rapoport <mike@compulab.co.il>
> > > ---
> > >
> > > Janitorial fix, not tested due to no hardware.
> > >
> > > ?arch/arm/mach-pxa/viper.c | ? ?5 +++--
> > > ?1 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
> > > index 1dd1334..c25921f 100644
> > > --- a/arch/arm/mach-pxa/viper.c
> > > +++ b/arch/arm/mach-pxa/viper.c
> > > @@ -33,6 +33,7 @@
> > > ?#include <linux/pm.h>
> > > ?#include <linux/sched.h>
> > > ?#include <linux/gpio.h>
> > > +#include <linux/jiffies.h>
> > > ?#include <linux/i2c-gpio.h>
> > > ?#include <linux/serial_8250.h>
> > > ?#include <linux/smc91x.h>
> > > @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
> > > ? ? ? ?.sda_pin = VIPER_RTC_I2C_SDA_GPIO,
> > > ? ? ? ?.scl_pin = VIPER_RTC_I2C_SCL_GPIO,
> > > ? ? ? ?.udelay ?= 10,
> > > - ? ? ? .timeout = 100,
> > > + ? ? ? .timeout = HZ / 10,
> > > ?};
> > >
> > > ?static struct platform_device i2c_bus_device = {
> > > @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
> > > ? ? ? ? ? ? ? ?.sda_pin = VIPER_TPM_I2C_SDA_GPIO,
> > > ? ? ? ? ? ? ? ?.scl_pin = VIPER_TPM_I2C_SCL_GPIO,
> > > ? ? ? ? ? ? ? ?.udelay ?= 10,
> > > - ? ? ? ? ? ? ? .timeout = 100,
> > > + ? ? ? ? ? ? ? .timeout = HZ / 10,
> > > ? ? ? ?};
> > > ? ? ? ?char *errstr;
> > >
> > 
> > One other better and cleaner approach to such inconsistency issue is
> > to have a timeout_ms field, and having i2c-gpio.c driver to convert this
> > to jiffies using msec_to_jiffies() at run-time.
> 
> With what benefit? Expressing time values in units of HZ is very
> frequent in the kernel code and shouldn't actually surprise anyone.

Actually, this patch shows there is confusion.

"Assume '100' means 100ms here and adapt accordingly."

Since this patch is for ARM, where HZ=100, the above patch is not a
simple "convert how we derive this constant" patch - it's a functional
change, reducing the timeouts by a factor of 10.

Could that be because the patch author misinterpreted the HZ-based
values?

I suspect I'm not the only one who thinks that the latter of "HZ / 10"
"100ms" is easier to read and comprehend without mistake.

Re: [PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Jean Delvare]

On Mon, 12 Apr 2010 20:20:10 +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
> > On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
> > > On Sun, Apr 4, 2010 at 10:08 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> > > > The timeout value is in jiffies, so it should be using HZ, not a plain
> > > > number. Assume '100' means 100ms here and adapt accordingly.
> > > >
> > > > Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> > > > Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
> > > > Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > > > Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
> > > > ---
> > > >
> > > > Janitorial fix, not tested due to no hardware.
> > > >
> > > >  arch/arm/mach-pxa/viper.c |    5 +++--
> > > >  1 files changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
> > > > index 1dd1334..c25921f 100644
> > > > --- a/arch/arm/mach-pxa/viper.c
> > > > +++ b/arch/arm/mach-pxa/viper.c
> > > > @@ -33,6 +33,7 @@
> > > >  #include <linux/pm.h>
> > > >  #include <linux/sched.h>
> > > >  #include <linux/gpio.h>
> > > > +#include <linux/jiffies.h>
> > > >  #include <linux/i2c-gpio.h>
> > > >  #include <linux/serial_8250.h>
> > > >  #include <linux/smc91x.h>
> > > > @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
> > > >        .sda_pin = VIPER_RTC_I2C_SDA_GPIO,
> > > >        .scl_pin = VIPER_RTC_I2C_SCL_GPIO,
> > > >        .udelay  = 10,
> > > > -       .timeout = 100,
> > > > +       .timeout = HZ / 10,
> > > >  };
> > > >
> > > >  static struct platform_device i2c_bus_device = {
> > > > @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
> > > >                .sda_pin = VIPER_TPM_I2C_SDA_GPIO,
> > > >                .scl_pin = VIPER_TPM_I2C_SCL_GPIO,
> > > >                .udelay  = 10,
> > > > -               .timeout = 100,
> > > > +               .timeout = HZ / 10,
> > > >        };
> > > >        char *errstr;
> > > >
> > > 
> > > One other better and cleaner approach to such inconsistency issue is
> > > to have a timeout_ms field, and having i2c-gpio.c driver to convert this
> > > to jiffies using msec_to_jiffies() at run-time.
> > 
> > With what benefit? Expressing time values in units of HZ is very
> > frequent in the kernel code and shouldn't actually surprise anyone.
> 
> Actually, this patch shows there is confusion.
> 
> "Assume '100' means 100ms here and adapt accordingly."
> 
> Since this patch is for ARM, where HZ=100, the above patch is not a
> simple "convert how we derive this constant" patch - it's a functional
> change, reducing the timeouts by a factor of 10.
> 
> Could that be because the patch author misinterpreted the HZ-based
> values?

I admit I would have assumed 100 -> HZ, as hard-coded HZ-dependent
value typically assume HZ=100.

> I suspect I'm not the only one who thinks that the latter of "HZ / 10"
> "100ms" is easier to read and comprehend without mistake.

OTOH, converting from ms to jiffies each time you need the value has a
cost.

-- 
Jean Delvare

[PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Jean Delvare]

On Mon, 12 Apr 2010 20:20:10 +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
> > On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
> > > On Sun, Apr 4, 2010 at 10:08 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> > > > The timeout value is in jiffies, so it should be using HZ, not a plain
> > > > number. Assume '100' means 100ms here and adapt accordingly.
> > > >
> > > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > > > Cc: Eric Miao <eric.y.miao@gmail.com>
> > > > Cc: Russell King <linux@arm.linux.org.uk>
> > > > Cc: Marc Zyngier <maz@misterjones.org>
> > > > Cc: Paul Shen <paul.shen@marvell.com>
> > > > Cc: Mike Rapoport <mike@compulab.co.il>
> > > > ---
> > > >
> > > > Janitorial fix, not tested due to no hardware.
> > > >
> > > > ?arch/arm/mach-pxa/viper.c | ? ?5 +++--
> > > > ?1 files changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
> > > > index 1dd1334..c25921f 100644
> > > > --- a/arch/arm/mach-pxa/viper.c
> > > > +++ b/arch/arm/mach-pxa/viper.c
> > > > @@ -33,6 +33,7 @@
> > > > ?#include <linux/pm.h>
> > > > ?#include <linux/sched.h>
> > > > ?#include <linux/gpio.h>
> > > > +#include <linux/jiffies.h>
> > > > ?#include <linux/i2c-gpio.h>
> > > > ?#include <linux/serial_8250.h>
> > > > ?#include <linux/smc91x.h>
> > > > @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
> > > > ? ? ? ?.sda_pin = VIPER_RTC_I2C_SDA_GPIO,
> > > > ? ? ? ?.scl_pin = VIPER_RTC_I2C_SCL_GPIO,
> > > > ? ? ? ?.udelay ?= 10,
> > > > - ? ? ? .timeout = 100,
> > > > + ? ? ? .timeout = HZ / 10,
> > > > ?};
> > > >
> > > > ?static struct platform_device i2c_bus_device = {
> > > > @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
> > > > ? ? ? ? ? ? ? ?.sda_pin = VIPER_TPM_I2C_SDA_GPIO,
> > > > ? ? ? ? ? ? ? ?.scl_pin = VIPER_TPM_I2C_SCL_GPIO,
> > > > ? ? ? ? ? ? ? ?.udelay ?= 10,
> > > > - ? ? ? ? ? ? ? .timeout = 100,
> > > > + ? ? ? ? ? ? ? .timeout = HZ / 10,
> > > > ? ? ? ?};
> > > > ? ? ? ?char *errstr;
> > > >
> > > 
> > > One other better and cleaner approach to such inconsistency issue is
> > > to have a timeout_ms field, and having i2c-gpio.c driver to convert this
> > > to jiffies using msec_to_jiffies() at run-time.
> > 
> > With what benefit? Expressing time values in units of HZ is very
> > frequent in the kernel code and shouldn't actually surprise anyone.
> 
> Actually, this patch shows there is confusion.
> 
> "Assume '100' means 100ms here and adapt accordingly."
> 
> Since this patch is for ARM, where HZ=100, the above patch is not a
> simple "convert how we derive this constant" patch - it's a functional
> change, reducing the timeouts by a factor of 10.
> 
> Could that be because the patch author misinterpreted the HZ-based
> values?

I admit I would have assumed 100 -> HZ, as hard-coded HZ-dependent
value typically assume HZ=100.

> I suspect I'm not the only one who thinks that the latter of "HZ / 10"
> "100ms" is easier to read and comprehend without mistake.

OTOH, converting from ms to jiffies each time you need the value has a
cost.

-- 
Jean Delvare

Re: [PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Russell King - ARM Linux]

On Mon, Apr 12, 2010 at 09:32:35PM +0200, Jean Delvare wrote:
> On Mon, 12 Apr 2010 20:20:10 +0100, Russell King - ARM Linux wrote:
> > On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
> > > On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
> > > > One other better and cleaner approach to such inconsistency issue is
> > > > to have a timeout_ms field, and having i2c-gpio.c driver to convert this
> > > > to jiffies using msec_to_jiffies() at run-time.
> > > 
> > > With what benefit? Expressing time values in units of HZ is very
> > > frequent in the kernel code and shouldn't actually surprise anyone.
> > 
> > Actually, this patch shows there is confusion.
> > 
> > "Assume '100' means 100ms here and adapt accordingly."
> > 
> > Since this patch is for ARM, where HZ=100, the above patch is not a
> > simple "convert how we derive this constant" patch - it's a functional
> > change, reducing the timeouts by a factor of 10.
> > 
> > Could that be because the patch author misinterpreted the HZ-based
> > values?
> 
> I admit I would have assumed 100 -> HZ, as hard-coded HZ-dependent
> value typically assume HZ=100.
> 
> > I suspect I'm not the only one who thinks that the latter of "HZ / 10"
> > "100ms" is easier to read and comprehend without mistake.
> 
> OTOH, converting from ms to jiffies each time you need the value has a
> cost.

True; what I did for MMC stuff is converted it from ms to jiffies at
initialization time when copying it in from platform data in the
driver's probe function.

I'm not saying that I care either way, I'm merely showing that dealing
with HZ-based values can be (maybe unexpectedly) more error prone.

[PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Russell King - ARM Linux]

On Mon, Apr 12, 2010 at 09:32:35PM +0200, Jean Delvare wrote:
> On Mon, 12 Apr 2010 20:20:10 +0100, Russell King - ARM Linux wrote:
> > On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
> > > On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
> > > > One other better and cleaner approach to such inconsistency issue is
> > > > to have a timeout_ms field, and having i2c-gpio.c driver to convert this
> > > > to jiffies using msec_to_jiffies() at run-time.
> > > 
> > > With what benefit? Expressing time values in units of HZ is very
> > > frequent in the kernel code and shouldn't actually surprise anyone.
> > 
> > Actually, this patch shows there is confusion.
> > 
> > "Assume '100' means 100ms here and adapt accordingly."
> > 
> > Since this patch is for ARM, where HZ=100, the above patch is not a
> > simple "convert how we derive this constant" patch - it's a functional
> > change, reducing the timeouts by a factor of 10.
> > 
> > Could that be because the patch author misinterpreted the HZ-based
> > values?
> 
> I admit I would have assumed 100 -> HZ, as hard-coded HZ-dependent
> value typically assume HZ=100.
> 
> > I suspect I'm not the only one who thinks that the latter of "HZ / 10"
> > "100ms" is easier to read and comprehend without mistake.
> 
> OTOH, converting from ms to jiffies each time you need the value has a
> cost.

True; what I did for MMC stuff is converted it from ms to jiffies at
initialization time when copying it in from platform data in the
driver's probe function.

I'm not saying that I care either way, I'm merely showing that dealing
with HZ-based values can be (maybe unexpectedly) more error prone.

Re: [PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Jamie Lokier]

Russell King - ARM Linux wrote:
> On Mon, Apr 12, 2010 at 09:32:35PM +0200, Jean Delvare wrote:
> > On Mon, 12 Apr 2010 20:20:10 +0100, Russell King - ARM Linux wrote:
> > > On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
> > > > On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
> > > > > One other better and cleaner approach to such inconsistency issue is
> > > > > to have a timeout_ms field, and having i2c-gpio.c driver to convert this
> > > > > to jiffies using msec_to_jiffies() at run-time.
> > > > 
> > > > With what benefit? Expressing time values in units of HZ is very
> > > > frequent in the kernel code and shouldn't actually surprise anyone.
> > > 
> > > Actually, this patch shows there is confusion.
> > > 
> > > "Assume '100' means 100ms here and adapt accordingly."
> > > 
> > > Since this patch is for ARM, where HZ=100, the above patch is not a
> > > simple "convert how we derive this constant" patch - it's a functional
> > > change, reducing the timeouts by a factor of 10.
> > > 
> > > Could that be because the patch author misinterpreted the HZ-based
> > > values?
> > 
> > I admit I would have assumed 100 -> HZ, as hard-coded HZ-dependent
> > value typically assume HZ=100.
> > 
> > > I suspect I'm not the only one who thinks that the latter of "HZ / 10"
> > > "100ms" is easier to read and comprehend without mistake.
> > 
> > OTOH, converting from ms to jiffies each time you need the value has a
> > cost.
> 
> True; what I did for MMC stuff is converted it from ms to jiffies at
> initialization time when copying it in from platform data in the
> driver's probe function.
> 
> I'm not saying that I care either way, I'm merely showing that dealing
> with HZ-based values can be (maybe unexpectedly) more error prone.

HZ is used a lot in kernel timeouts, so even though it's confusing, it
is something everyone ought to get used to.

But I agree it is too confusing.  An obvious remedy is:

#define milliseconds(ms) (((ms) * HZ + 999) / 1000)
#define seconds(s)       ((s) * HZ)

-- Jamie

[PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Jamie Lokier]

Russell King - ARM Linux wrote:
> On Mon, Apr 12, 2010 at 09:32:35PM +0200, Jean Delvare wrote:
> > On Mon, 12 Apr 2010 20:20:10 +0100, Russell King - ARM Linux wrote:
> > > On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
> > > > On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
> > > > > One other better and cleaner approach to such inconsistency issue is
> > > > > to have a timeout_ms field, and having i2c-gpio.c driver to convert this
> > > > > to jiffies using msec_to_jiffies() at run-time.
> > > > 
> > > > With what benefit? Expressing time values in units of HZ is very
> > > > frequent in the kernel code and shouldn't actually surprise anyone.
> > > 
> > > Actually, this patch shows there is confusion.
> > > 
> > > "Assume '100' means 100ms here and adapt accordingly."
> > > 
> > > Since this patch is for ARM, where HZ=100, the above patch is not a
> > > simple "convert how we derive this constant" patch - it's a functional
> > > change, reducing the timeouts by a factor of 10.
> > > 
> > > Could that be because the patch author misinterpreted the HZ-based
> > > values?
> > 
> > I admit I would have assumed 100 -> HZ, as hard-coded HZ-dependent
> > value typically assume HZ=100.
> > 
> > > I suspect I'm not the only one who thinks that the latter of "HZ / 10"
> > > "100ms" is easier to read and comprehend without mistake.
> > 
> > OTOH, converting from ms to jiffies each time you need the value has a
> > cost.
> 
> True; what I did for MMC stuff is converted it from ms to jiffies at
> initialization time when copying it in from platform data in the
> driver's probe function.
> 
> I'm not saying that I care either way, I'm merely showing that dealing
> with HZ-based values can be (maybe unexpectedly) more error prone.

HZ is used a lot in kernel timeouts, so even though it's confusing, it
is something everyone ought to get used to.

But I agree it is too confusing.  An obvious remedy is:

#define milliseconds(ms) (((ms) * HZ + 999) / 1000)
#define seconds(s)       ((s) * HZ)

-- Jamie

Re: [PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Eric Miao]

On Tue, Apr 13, 2010 at 5:53 AM, Jamie Lokier <jamie@shareable.org> wrote:
> Russell King - ARM Linux wrote:
>> On Mon, Apr 12, 2010 at 09:32:35PM +0200, Jean Delvare wrote:
>> > On Mon, 12 Apr 2010 20:20:10 +0100, Russell King - ARM Linux wrote:
>> > > On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
>> > > > On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
>> > > > > One other better and cleaner approach to such inconsistency issue is
>> > > > > to have a timeout_ms field, and having i2c-gpio.c driver to convert this
>> > > > > to jiffies using msec_to_jiffies() at run-time.
>> > > >
>> > > > With what benefit? Expressing time values in units of HZ is very
>> > > > frequent in the kernel code and shouldn't actually surprise anyone.
>> > >
>> > > Actually, this patch shows there is confusion.
>> > >
>> > > "Assume '100' means 100ms here and adapt accordingly."
>> > >
>> > > Since this patch is for ARM, where HZ=100, the above patch is not a
>> > > simple "convert how we derive this constant" patch - it's a functional
>> > > change, reducing the timeouts by a factor of 10.
>> > >
>> > > Could that be because the patch author misinterpreted the HZ-based
>> > > values?
>> >
>> > I admit I would have assumed 100 -> HZ, as hard-coded HZ-dependent
>> > value typically assume HZ=100.
>> >
>> > > I suspect I'm not the only one who thinks that the latter of "HZ / 10"
>> > > "100ms" is easier to read and comprehend without mistake.
>> >
>> > OTOH, converting from ms to jiffies each time you need the value has a
>> > cost.
>>
>> True; what I did for MMC stuff is converted it from ms to jiffies at
>> initialization time when copying it in from platform data in the
>> driver's probe function.
>>
>> I'm not saying that I care either way, I'm merely showing that dealing
>> with HZ-based values can be (maybe unexpectedly) more error prone.
>
> HZ is used a lot in kernel timeouts, so even though it's confusing, it
> is something everyone ought to get used to.

I don't understand why people has to live with confusions where there is
apparently a cleaner way to go. HZ is everywhere, and as long as they
live within their own driver code, that's OK.

>
> But I agree it is too confusing.  An obvious remedy is:
>
> #define milliseconds(ms) (((ms) * HZ + 999) / 1000)
> #define seconds(s)       ((s) * HZ)
>

This is to reinvent the wheel as there are already msecs_to_jiffies()
and usecs_to_jiffies(), and vice versa. The only benefit of using macros
instead of a function is for constants. This, however, can be worked
around as Russell suggested. The additional run-time cost by this
function, compared with a unit of jiffies, is insignificant.

> -- Jamie
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Eric Miao]

On Tue, Apr 13, 2010 at 5:53 AM, Jamie Lokier <jamie@shareable.org> wrote:
> Russell King - ARM Linux wrote:
>> On Mon, Apr 12, 2010 at 09:32:35PM +0200, Jean Delvare wrote:
>> > On Mon, 12 Apr 2010 20:20:10 +0100, Russell King - ARM Linux wrote:
>> > > On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
>> > > > On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
>> > > > > One other better and cleaner approach to such inconsistency issue is
>> > > > > to have a timeout_ms field, and having i2c-gpio.c driver to convert this
>> > > > > to jiffies using msec_to_jiffies() at run-time.
>> > > >
>> > > > With what benefit? Expressing time values in units of HZ is very
>> > > > frequent in the kernel code and shouldn't actually surprise anyone.
>> > >
>> > > Actually, this patch shows there is confusion.
>> > >
>> > > "Assume '100' means 100ms here and adapt accordingly."
>> > >
>> > > Since this patch is for ARM, where HZ=100, the above patch is not a
>> > > simple "convert how we derive this constant" patch - it's a functional
>> > > change, reducing the timeouts by a factor of 10.
>> > >
>> > > Could that be because the patch author misinterpreted the HZ-based
>> > > values?
>> >
>> > I admit I would have assumed 100 -> HZ, as hard-coded HZ-dependent
>> > value typically assume HZ=100.
>> >
>> > > I suspect I'm not the only one who thinks that the latter of "HZ / 10"
>> > > "100ms" is easier to read and comprehend without mistake.
>> >
>> > OTOH, converting from ms to jiffies each time you need the value has a
>> > cost.
>>
>> True; what I did for MMC stuff is converted it from ms to jiffies at
>> initialization time when copying it in from platform data in the
>> driver's probe function.
>>
>> I'm not saying that I care either way, I'm merely showing that dealing
>> with HZ-based values can be (maybe unexpectedly) more error prone.
>
> HZ is used a lot in kernel timeouts, so even though it's confusing, it
> is something everyone ought to get used to.

I don't understand why people has to live with confusions where there is
apparently a cleaner way to go. HZ is everywhere, and as long as they
live within their own driver code, that's OK.

>
> But I agree it is too confusing. ?An obvious remedy is:
>
> #define milliseconds(ms) (((ms) * HZ + 999) / 1000)
> #define seconds(s) ? ? ? ((s) * HZ)
>

This is to reinvent the wheel as there are already msecs_to_jiffies()
and usecs_to_jiffies(), and vice versa. The only benefit of using macros
instead of a function is for constants. This, however, can be worked
around as Russell suggested. The additional run-time cost by this
function, compared with a unit of jiffies, is insignificant.

> -- Jamie
>

[PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Wolfram Sang]

The timeout value is in jiffies, so it should be using HZ, not a plain
number. Assume with HZ=100 '100' means 1s here and adapt accordingly.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---

Changes since V1:

* Don't assume 100 means 100ms

Thanks for the comments!

Admitted, the first try was a really bad guess. Maybe I got distracted from the
previous patch for another arch where the value was 10000.

While this may still not be the favoured solution for Eric, I think it is at
least better than before, so it might be worth applying after all?

Janitorial fix, not tested due to no hardware.

 arch/arm/mach-pxa/viper.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
index 1dd1334..12cf38c 100644
--- a/arch/arm/mach-pxa/viper.c
+++ b/arch/arm/mach-pxa/viper.c
@@ -33,6 +33,7 @@
 #include <linux/pm.h>
 #include <linux/sched.h>
 #include <linux/gpio.h>
+#include <linux/jiffies.h>
 #include <linux/i2c-gpio.h>
 #include <linux/serial_8250.h>
 #include <linux/smc91x.h>
@@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
 	.sda_pin = VIPER_RTC_I2C_SDA_GPIO,
 	.scl_pin = VIPER_RTC_I2C_SCL_GPIO,
 	.udelay  = 10,
-	.timeout = 100,
+	.timeout = HZ,
 };
 
 static struct platform_device i2c_bus_device = {
@@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
 		.sda_pin = VIPER_TPM_I2C_SDA_GPIO,
 		.scl_pin = VIPER_TPM_I2C_SCL_GPIO,
 		.udelay  = 10,
-		.timeout = 100,
+		.timeout = HZ,
 	};
 	char *errstr;
 
-- 
1.7.0

[PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Wolfram Sang]

The timeout value is in jiffies, so it should be using HZ, not a plain
number. Assume with HZ=100 '100' means 1s here and adapt accordingly.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Marc Zyngier <maz@misterjones.org>
Cc: Paul Shen <paul.shen@marvell.com>
Cc: Mike Rapoport <mike@compulab.co.il>
Cc: Jean Delvare <khali@linux-fr.org>
---

Changes since V1:

* Don't assume 100 means 100ms

Thanks for the comments!

Admitted, the first try was a really bad guess. Maybe I got distracted from the
previous patch for another arch where the value was 10000.

While this may still not be the favoured solution for Eric, I think it is at
least better than before, so it might be worth applying after all?

Janitorial fix, not tested due to no hardware.

 arch/arm/mach-pxa/viper.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
index 1dd1334..12cf38c 100644
--- a/arch/arm/mach-pxa/viper.c
+++ b/arch/arm/mach-pxa/viper.c
@@ -33,6 +33,7 @@
 #include <linux/pm.h>
 #include <linux/sched.h>
 #include <linux/gpio.h>
+#include <linux/jiffies.h>
 #include <linux/i2c-gpio.h>
 #include <linux/serial_8250.h>
 #include <linux/smc91x.h>
@@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
 	.sda_pin = VIPER_RTC_I2C_SDA_GPIO,
 	.scl_pin = VIPER_RTC_I2C_SCL_GPIO,
 	.udelay  = 10,
-	.timeout = 100,
+	.timeout = HZ,
 };
 
 static struct platform_device i2c_bus_device = {
@@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
 		.sda_pin = VIPER_TPM_I2C_SDA_GPIO,
 		.scl_pin = VIPER_TPM_I2C_SCL_GPIO,
 		.udelay  = 10,
-		.timeout = 100,
+		.timeout = HZ,
 	};
 	char *errstr;
 
-- 
1.7.0

Re: [PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Eric Miao]

On Sun, Apr 18, 2010 at 7:48 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> The timeout value is in jiffies, so it should be using HZ, not a plain
> number. Assume with HZ=100 '100' means 1s here and adapt accordingly.
>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
> Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>
> Changes since V1:
>
> * Don't assume 100 means 100ms
>
> Thanks for the comments!
>
> Admitted, the first try was a really bad guess. Maybe I got distracted from the
> previous patch for another arch where the value was 10000.
>
> While this may still not be the favoured solution for Eric, I think it is at
> least better than before, so it might be worth applying after all?

Ack. The clearest fix would involve more code to be updated, which
can be postponed. This fix, at least, makes the time quantity clearer.

>
> Janitorial fix, not tested due to no hardware.
>
>  arch/arm/mach-pxa/viper.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
> index 1dd1334..12cf38c 100644
> --- a/arch/arm/mach-pxa/viper.c
> +++ b/arch/arm/mach-pxa/viper.c
> @@ -33,6 +33,7 @@
>  #include <linux/pm.h>
>  #include <linux/sched.h>
>  #include <linux/gpio.h>
> +#include <linux/jiffies.h>
>  #include <linux/i2c-gpio.h>
>  #include <linux/serial_8250.h>
>  #include <linux/smc91x.h>
> @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
>        .sda_pin = VIPER_RTC_I2C_SDA_GPIO,
>        .scl_pin = VIPER_RTC_I2C_SCL_GPIO,
>        .udelay  = 10,
> -       .timeout = 100,
> +       .timeout = HZ,
>  };
>
>  static struct platform_device i2c_bus_device = {
> @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
>                .sda_pin = VIPER_TPM_I2C_SDA_GPIO,
>                .scl_pin = VIPER_TPM_I2C_SCL_GPIO,
>                .udelay  = 10,
> -               .timeout = 100,
> +               .timeout = HZ,
>        };
>        char *errstr;
>
> --
> 1.7.0
>
>

[PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Eric Miao]

On Sun, Apr 18, 2010 at 7:48 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> The timeout value is in jiffies, so it should be using HZ, not a plain
> number. Assume with HZ=100 '100' means 1s here and adapt accordingly.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Eric Miao <eric.y.miao@gmail.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Marc Zyngier <maz@misterjones.org>
> Cc: Paul Shen <paul.shen@marvell.com>
> Cc: Mike Rapoport <mike@compulab.co.il>
> Cc: Jean Delvare <khali@linux-fr.org>
> ---
>
> Changes since V1:
>
> * Don't assume 100 means 100ms
>
> Thanks for the comments!
>
> Admitted, the first try was a really bad guess. Maybe I got distracted from the
> previous patch for another arch where the value was 10000.
>
> While this may still not be the favoured solution for Eric, I think it is at
> least better than before, so it might be worth applying after all?

Ack. The clearest fix would involve more code to be updated, which
can be postponed. This fix, at least, makes the time quantity clearer.

>
> Janitorial fix, not tested due to no hardware.
>
> ?arch/arm/mach-pxa/viper.c | ? ?5 +++--
> ?1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
> index 1dd1334..12cf38c 100644
> --- a/arch/arm/mach-pxa/viper.c
> +++ b/arch/arm/mach-pxa/viper.c
> @@ -33,6 +33,7 @@
> ?#include <linux/pm.h>
> ?#include <linux/sched.h>
> ?#include <linux/gpio.h>
> +#include <linux/jiffies.h>
> ?#include <linux/i2c-gpio.h>
> ?#include <linux/serial_8250.h>
> ?#include <linux/smc91x.h>
> @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
> ? ? ? ?.sda_pin = VIPER_RTC_I2C_SDA_GPIO,
> ? ? ? ?.scl_pin = VIPER_RTC_I2C_SCL_GPIO,
> ? ? ? ?.udelay ?= 10,
> - ? ? ? .timeout = 100,
> + ? ? ? .timeout = HZ,
> ?};
>
> ?static struct platform_device i2c_bus_device = {
> @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
> ? ? ? ? ? ? ? ?.sda_pin = VIPER_TPM_I2C_SDA_GPIO,
> ? ? ? ? ? ? ? ?.scl_pin = VIPER_TPM_I2C_SCL_GPIO,
> ? ? ? ? ? ? ? ?.udelay ?= 10,
> - ? ? ? ? ? ? ? .timeout = 100,
> + ? ? ? ? ? ? ? .timeout = HZ,
> ? ? ? ?};
> ? ? ? ?char *errstr;
>
> --
> 1.7.0
>
>

Re: [PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

On Tue, Apr 20, 2010 at 08:20:46AM +0800, Eric Miao wrote:
> On Sun, Apr 18, 2010 at 7:48 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > The timeout value is in jiffies, so it should be using HZ, not a plain
> > number. Assume with HZ=100 '100' means 1s here and adapt accordingly.
> >
> > Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> > Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
> > Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
> > Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > ---
> >
> > Changes since V1:
> >
> > * Don't assume 100 means 100ms
> >
> > Thanks for the comments!
> >
> > Admitted, the first try was a really bad guess. Maybe I got distracted from the
> > previous patch for another arch where the value was 10000.
> >
> > While this may still not be the favoured solution for Eric, I think it is at
> > least better than before, so it might be worth applying after all?
> 
> Ack. The clearest fix would involve more code to be updated, which
> can be postponed. This fix, at least, makes the time quantity clearer.

Thanks. Can you pick it up? Because it is a change to a board-file.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Wolfram Sang]

On Tue, Apr 20, 2010 at 08:20:46AM +0800, Eric Miao wrote:
> On Sun, Apr 18, 2010 at 7:48 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> > The timeout value is in jiffies, so it should be using HZ, not a plain
> > number. Assume with HZ=100 '100' means 1s here and adapt accordingly.
> >
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > Cc: Eric Miao <eric.y.miao@gmail.com>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Marc Zyngier <maz@misterjones.org>
> > Cc: Paul Shen <paul.shen@marvell.com>
> > Cc: Mike Rapoport <mike@compulab.co.il>
> > Cc: Jean Delvare <khali@linux-fr.org>
> > ---
> >
> > Changes since V1:
> >
> > * Don't assume 100 means 100ms
> >
> > Thanks for the comments!
> >
> > Admitted, the first try was a really bad guess. Maybe I got distracted from the
> > previous patch for another arch where the value was 10000.
> >
> > While this may still not be the favoured solution for Eric, I think it is at
> > least better than before, so it might be worth applying after all?
> 
> Ack. The clearest fix would involve more code to be updated, which
> can be postponed. This fix, at least, makes the time quantity clearer.

Thanks. Can you pick it up? Because it is a change to a board-file.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100420/8019b103/attachment.sig>

Re: [PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Marc Zyngier]

On Sun, 18 Apr 2010 13:48:29 +0200, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
wrote:
> The timeout value is in jiffies, so it should be using HZ, not a plain
> number. Assume with HZ=100 '100' means 1s here and adapt accordingly.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
> Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

Acked-by: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>

Eric, can you merge this directly via your -devel branch?

Thanks,

        M.
-- 
Who you jivin' with that Cosmik Debris?

[PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Marc Zyngier]

On Sun, 18 Apr 2010 13:48:29 +0200, Wolfram Sang <w.sang@pengutronix.de>
wrote:
> The timeout value is in jiffies, so it should be using HZ, not a plain
> number. Assume with HZ=100 '100' means 1s here and adapt accordingly.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Eric Miao <eric.y.miao@gmail.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Marc Zyngier <maz@misterjones.org>
> Cc: Paul Shen <paul.shen@marvell.com>
> Cc: Mike Rapoport <mike@compulab.co.il>
> Cc: Jean Delvare <khali@linux-fr.org>

Acked-by: Marc Zyngier <maz@misterjones.org>

Eric, can you merge this directly via your -devel branch?

Thanks,

        M.
-- 
Who you jivin' with that Cosmik Debris?

Re: [PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Marek Vasut]

Dne Út 20. dubna 2010 10:27:00 Marc Zyngier napsal(a):
> On Sun, 18 Apr 2010 13:48:29 +0200, Wolfram Sang <w.sang@pengutronix.de>
> 
> wrote:
> > The timeout value is in jiffies, so it should be using HZ, not a plain
> > number. Assume with HZ=100 '100' means 1s here and adapt accordingly.
> > 
> > Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> > Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
> > Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
> > Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> 
> Acked-by: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
> 
> Eric, can you merge this directly via your -devel branch?
> 
> Thanks,
> 
>         M.

And possibly updating your git tree? There are a few patches in -next that will 
need it's arm counterpart.

Cheers

[PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Marek Vasut]

Dne ?t 20. dubna 2010 10:27:00 Marc Zyngier napsal(a):
> On Sun, 18 Apr 2010 13:48:29 +0200, Wolfram Sang <w.sang@pengutronix.de>
> 
> wrote:
> > The timeout value is in jiffies, so it should be using HZ, not a plain
> > number. Assume with HZ=100 '100' means 1s here and adapt accordingly.
> > 
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > Cc: Eric Miao <eric.y.miao@gmail.com>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Marc Zyngier <maz@misterjones.org>
> > Cc: Paul Shen <paul.shen@marvell.com>
> > Cc: Mike Rapoport <mike@compulab.co.il>
> > Cc: Jean Delvare <khali@linux-fr.org>
> 
> Acked-by: Marc Zyngier <maz@misterjones.org>
> 
> Eric, can you merge this directly via your -devel branch?
> 
> Thanks,
> 
>         M.

And possibly updating your git tree? There are a few patches in -next that will 
need it's arm counterpart.

Cheers

Re: [PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Eric Miao]

On Tue, Apr 20, 2010 at 4:40 PM, Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Dne Út 20. dubna 2010 10:27:00 Marc Zyngier napsal(a):
>> On Sun, 18 Apr 2010 13:48:29 +0200, Wolfram Sang <w.sang@pengutronix.de>
>>
>> wrote:
>> > The timeout value is in jiffies, so it should be using HZ, not a plain
>> > number. Assume with HZ=100 '100' means 1s here and adapt accordingly.
>> >
>> > Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> > Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> > Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>> > Cc: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
>> > Cc: Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>> > Cc: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
>> > Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
>>
>> Acked-by: Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>
>>
>> Eric, can you merge this directly via your -devel branch?
>>
>> Thanks,
>>
>>         M.
>
> And possibly updating your git tree? There are a few patches in -next that will
> need it's arm counterpart.
>

Merged and updated.

[PATCH V2] mach-pxa/viper: Fix timeout usage for I2C

[Eric Miao]

On Tue, Apr 20, 2010 at 4:40 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Dne ?t 20. dubna 2010 10:27:00 Marc Zyngier napsal(a):
>> On Sun, 18 Apr 2010 13:48:29 +0200, Wolfram Sang <w.sang@pengutronix.de>
>>
>> wrote:
>> > The timeout value is in jiffies, so it should be using HZ, not a plain
>> > number. Assume with HZ=100 '100' means 1s here and adapt accordingly.
>> >
>> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
>> > Cc: Eric Miao <eric.y.miao@gmail.com>
>> > Cc: Russell King <linux@arm.linux.org.uk>
>> > Cc: Marc Zyngier <maz@misterjones.org>
>> > Cc: Paul Shen <paul.shen@marvell.com>
>> > Cc: Mike Rapoport <mike@compulab.co.il>
>> > Cc: Jean Delvare <khali@linux-fr.org>
>>
>> Acked-by: Marc Zyngier <maz@misterjones.org>
>>
>> Eric, can you merge this directly via your -devel branch?
>>
>> Thanks,
>>
>> ? ? ? ? M.
>
> And possibly updating your git tree? There are a few patches in -next that will
> need it's arm counterpart.
>

Merged and updated.