From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> To: Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> Cc: Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Marc Zyngier <maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org>, Paul Shen <paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>, Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org> Subject: Re: [PATCH] mach-pxa/viper: Fix timeout usage for I2C Date: Mon, 12 Apr 2010 21:32:35 +0200 [thread overview] Message-ID: <20100412213235.1688dda8@hyperion.delvare> (raw) In-Reply-To: <20100412192010.GM3048-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 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
WARNING: multiple messages have this Message-ID (diff)
From: khali@linux-fr.org (Jean Delvare) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] mach-pxa/viper: Fix timeout usage for I2C Date: Mon, 12 Apr 2010 21:32:35 +0200 [thread overview] Message-ID: <20100412213235.1688dda8@hyperion.delvare> (raw) In-Reply-To: <20100412192010.GM3048@n2100.arm.linux.org.uk> 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
next prev parent reply other threads:[~2010-04-12 19:32 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-04-04 14:08 [PATCH] mach-pxa/viper: Fix timeout usage for I2C Wolfram Sang 2010-04-04 14:08 ` Wolfram Sang [not found] ` <1270390118-1802-1-git-send-email-w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 2010-04-12 17:57 ` Eric Miao 2010-04-12 17:57 ` Eric Miao [not found] ` <m2rf17812d71004121057n5131d025pb4965f45b61271f-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2010-04-12 19:13 ` Jean Delvare 2010-04-12 19:13 ` Jean Delvare [not found] ` <20100412211319.5a43c65b-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 2010-04-12 19:20 ` Russell King - ARM Linux 2010-04-12 19:20 ` Russell King - ARM Linux [not found] ` <20100412192010.GM3048-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2010-04-12 19:32 ` Jean Delvare [this message] 2010-04-12 19:32 ` Jean Delvare [not found] ` <20100412213235.1688dda8-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 2010-04-12 19:39 ` Russell King - ARM Linux 2010-04-12 19:39 ` Russell King - ARM Linux [not found] ` <20100412193943.GP3048-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2010-04-12 21:53 ` Jamie Lokier 2010-04-12 21:53 ` Jamie Lokier 2010-04-12 23:04 ` Eric Miao 2010-04-12 23:04 ` Eric Miao 2010-04-18 11:48 ` [PATCH V2] " Wolfram Sang 2010-04-18 11:48 ` Wolfram Sang [not found] ` <1271591309-22567-1-git-send-email-w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 2010-04-20 0:20 ` Eric Miao 2010-04-20 0:20 ` Eric Miao [not found] ` <z2xf17812d71004191720o3f8660a4kdd31ef9cdb6beae9-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2010-04-20 1:03 ` Wolfram Sang 2010-04-20 1:03 ` Wolfram Sang 2010-04-20 8:27 ` Marc Zyngier 2010-04-20 8:27 ` Marc Zyngier 2010-04-20 8:40 ` Marek Vasut 2010-04-20 8:40 ` Marek Vasut [not found] ` <201004201040.00739.marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2010-04-20 9:21 ` Eric Miao 2010-04-20 9:21 ` Eric Miao
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20100412213235.1688dda8@hyperion.delvare \ --to=khali-puyad+kwke1g9huczpvpmw@public.gmane.org \ --cc=eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \ --cc=linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org \ --cc=maz-20xNzvSXLT6hUMvJH42dtQ@public.gmane.org \ --cc=mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org \ --cc=paul.shen-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org \ --cc=w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.