Re: [PATCH -mm] Blackfin: blackfin i2c driver
On Fri, 9 Mar 2007 12:04:59 +0800, Sonic Zhang wrote: > On 3/8/07, Jean Delvare <[EMAIL PROTECTED]> wrote: > > i2c-core can emulate SMBus transactions using master_xfer, so in > > general when you have a complete master_xfer implementation you do not > > need to define a separate smbus_xfer function. This would save a lot of > > code. > > Actually the i2c-core can't emulate SMBus transactions using the > master_xfer function, because the blackfin TWI controller provide > hardware support to the SMBus transactions and the combination of > master_xfer operations can't generate proper signal for SMBus. Did you try? I can't think of any valid reason why it wouldn't work. All SMBus transactions are compatible with I2C by definition. Now performance might be better with dedicated code if the hardware accelerate SMBus transactions, that's a different issue. If you prefer to keep the smbus_xfer method for performance reasons, that's OK with me. After all you're the one who will have to maintain the driver ;) -- Jean Delvare - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On 3/8/07, Jean Delvare <[EMAIL PROTECTED]> wrote: Hi Brian, Thanks for the quick update. > + > + rc = (iface->result >= 0) ? 0 : -1; > + > + /* Release mutex */ > + mutex_unlock(&iface->twi_lock); > + > + return rc; > +} i2c-core can emulate SMBus transactions using master_xfer, so in general when you have a complete master_xfer implementation you do not need to define a separate smbus_xfer function. This would save a lot of code. Actually the i2c-core can't emulate SMBus transactions using the master_xfer function, because the blackfin TWI controller provide hardware support to the SMBus transactions and the combination of master_xfer operations can't generate proper signal for SMBus. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
Hi Brian,
Thanks for the quick update.
> [PATCH] Blackfin: blackfin i2c driver
>
> The i2c linux driver for blackfin architecture which supports both GPIO
> i2c operation and blackfin on-chip TWI controller i2c operation.
>
> Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
> Reviewed-by: Andrew Morton <[EMAIL PROTECTED]>
> Reviewed-by: Alexey Dobriyan <[EMAIL PROTECTED]>
> Reviewed-by: Jean Delvare <[EMAIL PROTECTED]>
Actually I had only reviewed the Kconfig part, not the driver itself.
Here's a more complete review:
> ---
>
> drivers/i2c/busses/Kconfig | 47
> drivers/i2c/busses/Makefile|2
> drivers/i2c/busses/i2c-bfin-gpio.c | 100 +
> drivers/i2c/busses/i2c-bfin-twi.c | 589
>
> 4 files changed, 738 insertions(+)
>
> Index: linux-2.6/drivers/i2c/busses/Kconfig
> ===
> --- linux-2.6.orig/drivers/i2c/busses/Kconfig 2007-03-07 17:18:49.0
> +0800
> +++ linux-2.6/drivers/i2c/busses/Kconfig 2007-03-07 17:23:05.0
> +0800
> @@ -91,6 +91,53 @@
> This driver can also be built as a module. If so, the module
> will be called i2c-au1550.
>
> +config I2C_BLACKFIN_GPIO
> + tristate "Blackfin GPIO based I2C interface"
> + depends on I2C && BLACKFIN && EXPERIMENTAL
> + select I2C_ALGOBIT
> + help
> + Say Y here if you have an Blackfin BF5xx based
> + system and are using GPIO lines for an I2C bus.
> +
> +menu "Blackfin I2C SDA/SCL Selection"
> + depends on I2C_BLACKFIN_GPIO
> +config I2C_BLACKFIN_GPIO_SDA
> + int "SDA GPIO pin number"
> + range 0 15 if (BF533 || BF532 || BF531)
> + range 0 47 if (BF534 || BF536 || BF537)
> + range 0 47 if BF561
> + default 2
> +
> +config I2C_BLACKFIN_GPIO_SCL
> + int "SCL GPIO pin number"
> + range 0 15 if (BF533 || BF532 || BF531)
> + range 0 47 if (BF534 || BF536 || BF537)
> + range 0 47 if BF561
> + default 3
> +endmenu
> +
> +config I2C_BLACKFIN_GPIO_CYCLE_DELAY
> + int "Cycle Delay in uSec"
The proper symbol is "us".
> + depends on I2C_BLACKFIN_GPIO
> + range 5 100
> + default 40
> +
> +config I2C_BLACKFIN_TWI
> + tristate "Blackfin TWI I2C support"
> + depends on I2C && (BF534 || BF536 || BF537)
> + help
> + This is the TWI I2C device driver for Blackfin 534/536/537.
> + This driver can also be built as a module. If so, the module
> + will be called i2c-bfin-twi.
> +
> +config I2C_BLACKFIN_TWI_CLK_KHZ
> + int "Blackfin TWI I2C clock (kHz)"
> + depends on I2C_BLACKFIN_TWI
> + range 10 400
> + default 50
> + help
> + The unit of the TWI clock is kilo HZ.
kHz
> +
> config I2C_ELEKTOR
> tristate "Elektor ISA card"
> depends on I2C && ISA && BROKEN_ON_SMP
> Index: linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c
> ===
> --- /dev/null 1970-01-01 00:00:00.0 +
> +++ linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c 2007-03-07
> 17:32:30.0 +0800
> @@ -0,0 +1,100 @@
> +/
> + * Description: *
> + * *
> + * Maintainer: Meihui Fan <[EMAIL PROTECTED]>*
Indentation problem. Also, maintainer information goes in MAINTAINERS,
not individual drivers.
> + * *
> + * CopyRight (c) 2004 HHTech *
> + * www.hhcn.com, www.hhcn.org *
> + * All rights reserved. *
> + * *
> + * This file is free software; *
> + * you are free to modify and/or redistribute it *
Indentation problem here too.
> + * under the terms of the GNU General Public Licence (GPL). *
> + * *
> + /
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#define I2C_HW_B_HHBF 0x13
Holy god, no! This define goes in include/linux/i2c-id.h.
> +
> +static void hhbf_setsda(void *data, int state)
> +{
> + if (state) {
> + gpio_direction_input(CONFIG_I2C_BLACKFIN_GPIO_SDA);
> +
Extra blank line.
> + } else {
> + gpio_direction_output(CONFIG_I2C_BLACKFIN_GPIO_SDA);
> + gpio_set_value(CONFIG_I2C_BLACKFIN_GPIO_SDA, 0);
> + }
> +}
> +
> +static void hhbf_setscl(void *data, int state)
> +{
> + gpio_set_value(CONFIG_I2C_BLACKFIN_GPIO_SCL, state);
> +}
> +
> +static int hhbf
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On Tue, 6 Mar 2007 23:45:29 -0800, Andrew Morton wrote:
> On Wed, 07 Mar 2007 15:39:27 +0800 "Wu, Bryan" <[EMAIL PROTECTED]> wrote:
>
> > Thanks a lot, could you please give me a script just to kill this
> > whitespace? So I can do it before sending you patches.
>
>
> Is pretty simple:
>
> #!/bin/sh
> #
> # Strip any trailing whitespace which a unified diff adds.
> #
>
> strip1()
> {
> TMP=$(mktemp /tmp/XX)
> cp $1 $TMP
> sed -e '/^+/s/[ ]*$//' < $TMP > $1
> rm $TMP
> }
>
> for i in $*
> do
> strip1 $i
> done
>
>
> that'll be in
> http://www.zip.com.au/~akpm/linux/patches/patch-scripts-0.20/patch-scripts-0.20.tar.gz
> too
Alternatively, you can use quilt [1] to manage your patches and enable
the --strip-trailing-whitespace option by default.
[1] http://savannah.nongnu.org/projects/quilt/
--
Jean Delvare
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
trailing whitespace killing (Re: [PATCH -mm] Blackfin: blackfin i2c driver)
> From: Andrew Morton
> Newsgroups: gmane.linux.kernel
> Subject: Re: [PATCH -mm] Blackfin: blackfin i2c driver
> Date: Tue, 6 Mar 2007 23:45:29 -0800
[]
> On Wed, 07 Mar 2007 15:39:27 +0800 "Wu, Bryan" <[EMAIL PROTECTED]> wrote:
>
>> Thanks a lot, could you please give me a script just to kill this
>> whitespace? So I can do it before sending you patches.
>
>
> Is pretty simple:
>
> #!/bin/sh
> #
> # Strip any trailing whitespace which a unified diff adds.
> #
>
> strip1()
> {
> TMP=$(mktemp /tmp/XX)
> cp $1 $TMP
> sed -e '/^+/s/[ ]*$//' < $TMP > $1
> rm $TMP
> }
>
> for i in $*
> do
> strip1 $i
> done
>
>
> that'll be in
> http://www.zip.com.au/~akpm/linux/patches/patch-scripts-0.20/patch-scripts-0.20.tar.gz
> too
It doesn't work for me. Maybe i can't understand what you are trying to
do, anyway.
General suggestion is can be:
sed -e 's_[ \t]*$__'
(i.e any line on stdin with space/tab mixed tails is stripped on stdout)
You can use it as wrapper for diff, sending patch bombs, etc.
(very nice with pipes):
shell$ diff -Npu2 old new | sed -e 's_[ \t]*$__' > patch.diff
shell$ < patch-set.mbox sed -e 's_[ \t]*$__' | formail -s /usr/sbin/sendmail
-bm -t
similar in scripts; quilt (patch sets manager) notices about them.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
> > Signed-off-by: Bryan Wu <[EMAIL PROTECTED]> > > --- > > drivers/i2c/busses/Kconfig | 47 > > drivers/i2c/busses/i2c-bfin-gpio.c | 98 + > > drivers/i2c/busses/i2c-bfin-twi.c | 589 > > > > I'd prefer i2c-blackfin-gpio and i2c-blackfin-twi. Abreviations tend to > confuse newcomers. > There are tons of code using bfin abreviations for functions/files/variables names. So we prefer to follow the name scheme. > > 3 files changed, 734 insertions(+) > > > > Index: linux-2.6/drivers/i2c/busses/Kconfig > > === > > --- linux-2.6.orig/drivers/i2c/busses/Kconfig 2007-03-07 > > 13:32:02.0 +0800 > > +++ linux-2.6/drivers/i2c/busses/Kconfig2007-03-07 13:44:19.0 > > +0800 > > @@ -5,6 +5,53 @@ > > menu "I2C Hardware Bus support" > > depends on I2C > > > > +config I2C_BFIN_GPIO > > I2C_BLACKFIN_GPIO > In the Kconfig, we are trying to using CONFIG_BLACKFIN. So, I changed all the BFIN to BLACKFIN as you mentioned. > Please move the entries to the right location. The list is sorted > alphabetically if you didn't notice. > > > + tristate "Generic Blackfin and HHBF533/561 development board I2C > > support" > > You can drop the trailing "I2C support", the user is in a menu named > "I2C hardware bus support" so it's pretty clear what we're talking > about. > > > + depends on I2C && EXPERIMENTAL > > + select I2C_ALGOBIT > > + help > > + -- > > + > > +menu "BFIN I2C SDA/SCL Selection" > > + depends on I2C_BFIN_GPIO > > +config BFIN_SDA > > I2C_BLACKFIN_SDA > > > + int "SDA is GPIO Number" > > "SDA GPIO pin number" > > > + range 0 15 if (BF533 || BF532 || BF531) > > Trailing whitespace. > > > + range 0 47 if (BF534 || BF536 || BF537) > > + range 0 47 if BF561 > > + default 2 if (BF533 || BF532 || BF531) > > Trailing whitespace. > > No default for the other cases? > > > + > > +config BFIN_SCL > > I2C_BLACKFIN_SCL > Etc etc, all the options should start with I2C_BLACKFIN. > > > + int "SCL is GPIO Number" > > "SCL GPIO pin number" > > > + range 0 15 if (BF533 || BF532 || BF531) > > Trailing whitespace, and many more after that. Please fix them all! > All above fixed. > > + range 0 47 if (BF534 || BF536 || BF537) > > + range 0 47 if BF561 > > + default 3 > > +endmenu > > + > > +config I2C_BFIN_GPIO_CYCLE_DELAY > > + int "Cycle Delay in usec" > > + depends on I2C_BFIN_GPIO > > + range 1 100 > > + default 40 > > This should really not be a kernel configuration option. Please turn it > into a kernel module parameter or a sysfs attribute if you really need > it. Also note that we already have an interface to change this > value from user-space (using an ioctl on /dev/i2c-N) and that might be > sufficient for your needs. > Actually, for some customer's requirement, they just want to set the configuration in kernel config time not in the runtime. > And allowing 1 usec delay is probably not a good idea, I don't > recommend values below 6 usec with i2c-algo-bit. I add a range here from 5 to 100, default is 40. > > + > > +config I2C_BFIN_TWI > > + tristate "Blackfin TWI I2C support" > > + depends on I2C && (BF534 || BF536 || BF537) > > + help > > + This the TWI I2C device driver for Blackfin 534/536/537. > > + > > + This driver can also be built as a module. If so, the module > > + will be called i2c-bfin-twi. > > + > > +config TWICLK_KHZ > > + int "TWI clock (kHZ)" > > kHz > > > + depends on I2C_BFIN_TWI > > + default 50 > > + help > > + The unit of the TWI clock is kilo HZ. Please divide the clock > > + by 1024 if you count it in HZ. The value should be less than 400. > > Why don't you use "range" here too to ensure that the value is actually > less than 400? Either way, same as above, IMHO this should not be a > compilation-time decision. > > A kHz is really 1000 Hz, not 1024. And everybody skilled enough to > configure a kernel should know that, I doubt it's worth reminding. > All above fixed. > > + > > config I2C_ALI1535 > > tristate "ALI 1535" > > depends on I2C && PCI > > All these options won't work really well until you also change > drivers/i2c/busses/Makefile to make something useful with them... > Sorry for missing the Makefile in this patch. Thanks for your review and this is the latest one. Andrew, I also fixed Kconfig bug in your blackfin-blackfin-i2c-driver-fix.patch Please add this one to -mm tree and remove old one/update.patch/fix.patch from -mm tree. [PATCH] Blackfin: blackfin i2c driver The i2c linux driver for blackfin architecture which supports both GPIO i2c operation and blackfin on-chip TWI controller i2c operation. Signed-off-by: Bryan Wu <[EMAIL PROTECTED]> Reviewed-by: Andrew Morton <[EMAIL PROTECTED]> Reviewed-by: Alexey Dobriyan <[EMAIL PROTECTED]> Reviewed-by: Jean Delvare <[EMAIL PROTE
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On Wed, 07 Mar 2007 15:39:27 +0800 "Wu, Bryan" <[EMAIL PROTECTED]> wrote:
> Thanks a lot, could you please give me a script just to kill this
> whitespace? So I can do it before sending you patches.
Is pretty simple:
#!/bin/sh
#
# Strip any trailing whitespace which a unified diff adds.
#
strip1()
{
TMP=$(mktemp /tmp/XX)
cp $1 $TMP
sed -e '/^+/s/[ ]*$//' < $TMP > $1
rm $TMP
}
for i in $*
do
strip1 $i
done
that'll be in
http://www.zip.com.au/~akpm/linux/patches/patch-scripts-0.20/patch-scripts-0.20.tar.gz
too
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On Tue, 2007-03-06 at 23:14 -0800, Andrew Morton wrote: > On Wed, 7 Mar 2007 07:58:22 +0100 Jean Delvare <[EMAIL PROTECTED]> wrote: > > > > +config BFIN_SDA > > > > I2C_BLACKFIN_SDA > > The blackfin architecture uses "bfin" pretty much universally, so this > usage is consistent. > > box:/usr/src/25> grep -i blackfin patches/blackfin*|wc -l >1608 > box:/usr/src/25> grep -i bfin patches/blackfin*|wc -l >6198 > Thanks for you understanding, but now we want to move to use CONFIG_BLACKFIN options. There is a new task in our development plan to change things to CONFIG_BLACKFIN. At this moment, we both provide CONFIG_BFIN and CONFIG_BLACKFIN. When all the things relied on CONFIG_BFIN/bfin are changed to CONFIG_BLACKFIN/blackfin, the CONFIG_BFIN will be removed. So here I will follow Jean's comments. > Let's just hope nobody makes a bluefin. So it is ok for both blackfin and bluefin. But I think Black is cooler than Blue. -:) > > > > + range 0 15 if (BF533 || BF532 || BF531) > > > > Trailing whitespace. > > I always remove that when merging a patch. Thanks a lot, could you please give me a script just to kill this whitespace? So I can do it before sending you patches. Thanks Jean and Andrew. -Bryan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On Wed, 7 Mar 2007 07:58:22 +0100 Jean Delvare <[EMAIL PROTECTED]> wrote: > > +config BFIN_SDA > > I2C_BLACKFIN_SDA The blackfin architecture uses "bfin" pretty much universally, so this usage is consistent. box:/usr/src/25> grep -i blackfin patches/blackfin*|wc -l 1608 box:/usr/src/25> grep -i bfin patches/blackfin*|wc -l 6198 Let's just hope nobody makes a bluefin. > > + range 0 15 if (BF533 || BF532 || BF531) > > Trailing whitespace. I always remove that when merging a patch. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On Wed, 07 Mar 2007 13:57:58 +0800 "Wu, Bryan" <[EMAIL PROTECTED]> wrote: > Here is the updated blackfin i2c driver. > > [PATCH] Blackfin: blackfin i2c driver > > The i2c linux driver for blackfin architecture which supports both GPIO > i2c operation and blackfin on-chip TWI controller i2c operation. > > Signed-off-by: Bryan Wu <[EMAIL PROTECTED]> > --- > drivers/i2c/busses/Kconfig | 47 > drivers/i2c/busses/i2c-bfin-gpio.c | 98 + > drivers/i2c/busses/i2c-bfin-twi.c | 589 > > 3 files changed, 734 insertions(+) > > Index: linux-2.6/drivers/i2c/busses/Kconfig > === > --- linux-2.6.orig/drivers/i2c/busses/Kconfig 2007-03-07 13:32:02.0 > +0800 > +++ linux-2.6/drivers/i2c/busses/Kconfig 2007-03-07 13:44:19.0 > +0800 > @@ -5,6 +5,53 @@ > menu "I2C Hardware Bus support" > depends on I2C > > +config I2C_BFIN_GPIO > + tristate "Generic Blackfin and HHBF533/561 development board I2C > support" > + depends on I2C && EXPERIMENTAL > + select I2C_ALGOBIT > + help > + -- > + > +menu "BFIN I2C SDA/SCL Selection" > + depends on I2C_BFIN_GPIO > +config BFIN_SDA > + int "SDA is GPIO Number" > + range 0 15 if (BF533 || BF532 || BF531) > + range 0 47 if (BF534 || BF536 || BF537) > + range 0 47 if BF561 > + default 2 if (BF533 || BF532 || BF531) > + > +config BFIN_SCL > + int "SCL is GPIO Number" > + range 0 15 if (BF533 || BF532 || BF531) > + range 0 47 if (BF534 || BF536 || BF537) > + range 0 47 if BF561 > + default 3 > +endmenu > + > +config I2C_BFIN_GPIO_CYCLE_DELAY > + int "Cycle Delay in usec" > + depends on I2C_BFIN_GPIO > + range 1 100 > + default 40 > + > +config I2C_BFIN_TWI > + tristate "Blackfin TWI I2C support" > + depends on I2C && (BF534 || BF536 || BF537) > + help > + This the TWI I2C device driver for Blackfin 534/536/537. > + > + This driver can also be built as a module. If so, the module > + will be called i2c-bfin-twi. > + > +config TWICLK_KHZ > + int "TWI clock (kHZ)" > + depends on I2C_BFIN_TWI > + default 50 > + help > + The unit of the TWI clock is kilo HZ. Please divide the clock > + by 1024 if you count it in HZ. The value should be less than 400. > + Well that's cute. This patch causes an i386 `make allmodconfig' to spew these: SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) SDA is GPIO Number (BFIN_SDA) [] (NEW) out at about 1,000,000/sec, infinitely. I'll put a `depends on BFIN' in there to shut it up, but I think you've tickled a Kconfig bug. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
Hi Bryan, On Wed, 07 Mar 2007 13:57:58 +0800, Wu, Bryan wrote: > Here is the updated blackfin i2c driver. > > [PATCH] Blackfin: blackfin i2c driver > > The i2c linux driver for blackfin architecture which supports both GPIO > i2c operation and blackfin on-chip TWI controller i2c operation. > > Signed-off-by: Bryan Wu <[EMAIL PROTECTED]> > --- > drivers/i2c/busses/Kconfig | 47 > drivers/i2c/busses/i2c-bfin-gpio.c | 98 + > drivers/i2c/busses/i2c-bfin-twi.c | 589 > I'd prefer i2c-blackfin-gpio and i2c-blackfin-twi. Abreviations tend to confuse newcomers. > 3 files changed, 734 insertions(+) > > Index: linux-2.6/drivers/i2c/busses/Kconfig > === > --- linux-2.6.orig/drivers/i2c/busses/Kconfig 2007-03-07 13:32:02.0 > +0800 > +++ linux-2.6/drivers/i2c/busses/Kconfig 2007-03-07 13:44:19.0 > +0800 > @@ -5,6 +5,53 @@ > menu "I2C Hardware Bus support" > depends on I2C > > +config I2C_BFIN_GPIO I2C_BLACKFIN_GPIO Please move the entries to the right location. The list is sorted alphabetically if you didn't notice. > + tristate "Generic Blackfin and HHBF533/561 development board I2C > support" You can drop the trailing "I2C support", the user is in a menu named "I2C hardware bus support" so it's pretty clear what we're talking about. > + depends on I2C && EXPERIMENTAL > + select I2C_ALGOBIT > + help > + -- > + > +menu "BFIN I2C SDA/SCL Selection" > + depends on I2C_BFIN_GPIO > +config BFIN_SDA I2C_BLACKFIN_SDA > + int "SDA is GPIO Number" "SDA GPIO pin number" > + range 0 15 if (BF533 || BF532 || BF531) Trailing whitespace. > + range 0 47 if (BF534 || BF536 || BF537) > + range 0 47 if BF561 > + default 2 if (BF533 || BF532 || BF531) Trailing whitespace. No default for the other cases? > + > +config BFIN_SCL I2C_BLACKFIN_SCL Etc etc, all the options should start with I2C_BLACKFIN. > + int "SCL is GPIO Number" "SCL GPIO pin number" > + range 0 15 if (BF533 || BF532 || BF531) Trailing whitespace, and many more after that. Please fix them all! > + range 0 47 if (BF534 || BF536 || BF537) > + range 0 47 if BF561 > + default 3 > +endmenu > + > +config I2C_BFIN_GPIO_CYCLE_DELAY > + int "Cycle Delay in usec" > + depends on I2C_BFIN_GPIO > + range 1 100 > + default 40 This should really not be a kernel configuration option. Please turn it into a kernel module parameter or a sysfs attribute if you really need it. Also note that we already have an interface to change this value from user-space (using an ioctl on /dev/i2c-N) and that might be sufficient for your needs. And allowing 1 usec delay is probably not a good idea, I don't recommend values below 6 usec with i2c-algo-bit. > + > +config I2C_BFIN_TWI > + tristate "Blackfin TWI I2C support" > + depends on I2C && (BF534 || BF536 || BF537) > + help > + This the TWI I2C device driver for Blackfin 534/536/537. > + > + This driver can also be built as a module. If so, the module > + will be called i2c-bfin-twi. > + > +config TWICLK_KHZ > + int "TWI clock (kHZ)" kHz > + depends on I2C_BFIN_TWI > + default 50 > + help > + The unit of the TWI clock is kilo HZ. Please divide the clock > + by 1024 if you count it in HZ. The value should be less than 400. Why don't you use "range" here too to ensure that the value is actually less than 400? Either way, same as above, IMHO this should not be a compilation-time decision. A kHz is really 1000 Hz, not 1024. And everybody skilled enough to configure a kernel should know that, I doubt it's worth reminding. > + > config I2C_ALI1535 > tristate "ALI 1535" > depends on I2C && PCI All these options won't work really well until you also change drivers/i2c/busses/Makefile to make something useful with them... -- Jean Delvare - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
> >
> > OK, I change it into yield(). So, current process will be move to the
> > tail of the run queue. Is that OK with you?
>
> Nope, yield is terribly bad when there are busy processes running: it can
> stall for a very long time indeed,
>
> Is this hardware not capable of generating an interrupt when BUSBUSY gets
> negated?
>
> I guess not, in which case you're stuck with having to poll it - probably
> use a cond_resched() in the loop, and an angry comment.
Thanks, we fix it. please pick up this one.
[PATCH] Blackfin: blackfin i2c driver
The i2c linux driver for blackfin architecture which supports both GPIO
i2c operation and blackfin on-chip TWI controller i2c operation.
Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
---
drivers/i2c/busses/Kconfig | 47
drivers/i2c/busses/i2c-bfin-gpio.c | 98 +
drivers/i2c/busses/i2c-bfin-twi.c | 589
3 files changed, 734 insertions(+)
Index: linux-2.6/drivers/i2c/busses/Kconfig
===
--- linux-2.6.orig/drivers/i2c/busses/Kconfig 2007-03-07 13:32:02.0
+0800
+++ linux-2.6/drivers/i2c/busses/Kconfig2007-03-07 13:44:19.0
+0800
@@ -5,6 +5,53 @@
menu "I2C Hardware Bus support"
depends on I2C
+config I2C_BFIN_GPIO
+ tristate "Generic Blackfin and HHBF533/561 development board I2C
support"
+ depends on I2C && EXPERIMENTAL
+ select I2C_ALGOBIT
+ help
+ --
+
+menu "BFIN I2C SDA/SCL Selection"
+ depends on I2C_BFIN_GPIO
+config BFIN_SDA
+ int "SDA is GPIO Number"
+ range 0 15 if (BF533 || BF532 || BF531)
+ range 0 47 if (BF534 || BF536 || BF537)
+ range 0 47 if BF561
+ default 2 if (BF533 || BF532 || BF531)
+
+config BFIN_SCL
+ int "SCL is GPIO Number"
+ range 0 15 if (BF533 || BF532 || BF531)
+ range 0 47 if (BF534 || BF536 || BF537)
+ range 0 47 if BF561
+ default 3
+endmenu
+
+config I2C_BFIN_GPIO_CYCLE_DELAY
+ int "Cycle Delay in usec"
+ depends on I2C_BFIN_GPIO
+ range 1 100
+ default 40
+
+config I2C_BFIN_TWI
+ tristate "Blackfin TWI I2C support"
+ depends on I2C && (BF534 || BF536 || BF537)
+ help
+ This the TWI I2C device driver for Blackfin 534/536/537.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-bfin-twi.
+
+config TWICLK_KHZ
+ int "TWI clock (kHZ)"
+ depends on I2C_BFIN_TWI
+ default 50
+ help
+ The unit of the TWI clock is kilo HZ. Please divide the clock
+ by 1024 if you count it in HZ. The value should be less than 400.
+
config I2C_ALI1535
tristate "ALI 1535"
depends on I2C && PCI
Index: linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c2007-03-07
13:44:19.0 +0800
@@ -0,0 +1,98 @@
+/
+ * Description: *
+ * *
+ * Maintainer: Meihui Fan <[EMAIL PROTECTED]> *
+ * *
+ * CopyRight (c) 2004 HHTech *
+ * www.hhcn.com, www.hhcn.org *
+ * All rights reserved. *
+ * *
+ * This file is free software; *
+ * you are free to modify and/or redistribute it *
+ * under the terms of the GNU General Public Licence (GPL). *
+ * *
+ /
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#defineI2C_HW_B_HHBF 0x13
+
+static void hhbf_setsda(void *data, int state)
+{
+ if (state) {
+ gpio_direction_input(CONFIG_BFIN_SDA);
+
+ } else {
+ gpio_direction_output(CONFIG_BFIN_SDA);
+ gpio_set_value(CONFIG_BFIN_SDA, 0);
+ }
+}
+
+static void hhbf_setscl(void *data, int state)
+{
+ gpio_set_value(CONFIG_BFIN_SCL, state);
+}
+
+static int hhbf_getsda(void *data)
+{
+ return (gpio_get_value(CONFIG_BFIN_SDA) != 0);
+}
+
+
+static struct i2c_algo_bit_data bit_hhbf_data = {
+ .setsda = hhbf_setsda,
+ .setscl = hhbf_setscl,
+ .getsda = hhbf_getsda,
+ .udelay = CONFIG_I2C_BFIN_GPIO_CYCLE_DELAY,
+ .timeout = HZ
+};
+
+static struct i2c_adapter hhbf_ops = {
+ .owner = THIS_MODULE,
+ .id = I2C_HW_B_HHBF
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On Wed, 7 Mar 2007 13:17:57 +0800 "Sonic Zhang" <[EMAIL PROTECTED]> wrote:
> On 3/6/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
> > On Tue, 06 Mar 2007 14:54:18 +0800 "Wu, Bryan" <[EMAIL PROTECTED]> wrote:
> >
> > > Hi folks,
> > >
> > > [PATCH] Blackfin: blackfin i2c driver
> > >
>
> > > + struct i2c_msg *pmsg;
> > > + int i, ret;
> > > + int rc = 0;
> > > +
> > > + if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> > > + return -ENXIO;
> > > +
> > > + down(&iface->twi_lock);
> > > +
> > > + while (bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> > > + up(&iface->twi_lock);
> > > + schedule();
> > > + down(&iface->twi_lock);
> > > + }
> >
> > That's a busy loop until this task's timeslice has expired. It'll work,
> > but it'll suck a bit. (Repeated in several places)
> >
>
> OK, I change it into yield(). So, current process will be move to the
> tail of the run queue. Is that OK with you?
Nope, yield is terribly bad when there are busy processes running: it can
stall for a very long time indeed,
Is this hardware not capable of generating an interrupt when BUSBUSY gets
negated?
I guess not, in which case you're stuck with having to poll it - probably
use a cond_resched() in the loop, and an angry comment.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
Dear Andrew and Alexey:
Thanks a lot for the review.
Here is the updated blackfin i2c driver.
[PATCH] Blackfin: blackfin i2c driver
The i2c linux driver for blackfin architecture which supports both GPIO
i2c operation and blackfin on-chip TWI controller i2c operation.
Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
---
drivers/i2c/busses/Kconfig | 47
drivers/i2c/busses/i2c-bfin-gpio.c | 98 +
drivers/i2c/busses/i2c-bfin-twi.c | 589
3 files changed, 734 insertions(+)
Index: linux-2.6/drivers/i2c/busses/Kconfig
===
--- linux-2.6.orig/drivers/i2c/busses/Kconfig 2007-03-07 13:32:02.0
+0800
+++ linux-2.6/drivers/i2c/busses/Kconfig2007-03-07 13:44:19.0
+0800
@@ -5,6 +5,53 @@
menu "I2C Hardware Bus support"
depends on I2C
+config I2C_BFIN_GPIO
+ tristate "Generic Blackfin and HHBF533/561 development board I2C
support"
+ depends on I2C && EXPERIMENTAL
+ select I2C_ALGOBIT
+ help
+ --
+
+menu "BFIN I2C SDA/SCL Selection"
+ depends on I2C_BFIN_GPIO
+config BFIN_SDA
+ int "SDA is GPIO Number"
+ range 0 15 if (BF533 || BF532 || BF531)
+ range 0 47 if (BF534 || BF536 || BF537)
+ range 0 47 if BF561
+ default 2 if (BF533 || BF532 || BF531)
+
+config BFIN_SCL
+ int "SCL is GPIO Number"
+ range 0 15 if (BF533 || BF532 || BF531)
+ range 0 47 if (BF534 || BF536 || BF537)
+ range 0 47 if BF561
+ default 3
+endmenu
+
+config I2C_BFIN_GPIO_CYCLE_DELAY
+ int "Cycle Delay in usec"
+ depends on I2C_BFIN_GPIO
+ range 1 100
+ default 40
+
+config I2C_BFIN_TWI
+ tristate "Blackfin TWI I2C support"
+ depends on I2C && (BF534 || BF536 || BF537)
+ help
+ This the TWI I2C device driver for Blackfin 534/536/537.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-bfin-twi.
+
+config TWICLK_KHZ
+ int "TWI clock (kHZ)"
+ depends on I2C_BFIN_TWI
+ default 50
+ help
+ The unit of the TWI clock is kilo HZ. Please divide the clock
+ by 1024 if you count it in HZ. The value should be less than 400.
+
config I2C_ALI1535
tristate "ALI 1535"
depends on I2C && PCI
Index: linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c2007-03-07
13:44:19.0 +0800
@@ -0,0 +1,98 @@
+/
+ * Description: *
+ * *
+ * Maintainer: Meihui Fan <[EMAIL PROTECTED]> *
+ * *
+ * CopyRight (c) 2004 HHTech *
+ * www.hhcn.com, www.hhcn.org *
+ * All rights reserved. *
+ * *
+ * This file is free software; *
+ * you are free to modify and/or redistribute it *
+ * under the terms of the GNU General Public Licence (GPL). *
+ * *
+ /
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#defineI2C_HW_B_HHBF 0x13
+
+static void hhbf_setsda(void *data, int state)
+{
+ if (state) {
+ gpio_direction_input(CONFIG_BFIN_SDA);
+
+ } else {
+ gpio_direction_output(CONFIG_BFIN_SDA);
+ gpio_set_value(CONFIG_BFIN_SDA, 0);
+ }
+}
+
+static void hhbf_setscl(void *data, int state)
+{
+ gpio_set_value(CONFIG_BFIN_SCL, state);
+}
+
+static int hhbf_getsda(void *data)
+{
+ return (gpio_get_value(CONFIG_BFIN_SDA) != 0);
+}
+
+
+static struct i2c_algo_bit_data bit_hhbf_data = {
+ .setsda = hhbf_setsda,
+ .setscl = hhbf_setscl,
+ .getsda = hhbf_getsda,
+ .udelay = CONFIG_I2C_BFIN_GPIO_CYCLE_DELAY,
+ .timeout = HZ
+};
+
+static struct i2c_adapter hhbf_ops = {
+ .owner = THIS_MODULE,
+ .id = I2C_HW_B_HHBF,
+ .algo_data = &bit_hhbf_data,
+ .name = "HHBF I2C driver",
+};
+
+static int __init i2c_hhbf_init(void)
+{
+
+ if (gpio_request(CONFIG_BFIN_SCL, NULL)) {
+ printk(KERN_ERR "%s: gpio_request GPIO %d failed \n",__func__,
CONFIG_BFIN_SCL);
+ return -1;
+ }
+
+ if (gpio_request(CONFIG_BFIN_SDA, NULL)) {
+ printk(KERN_ERR "%s: gpio_request
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On 3/6/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
On Tue, 06 Mar 2007 14:54:18 +0800 "Wu, Bryan" <[EMAIL PROTECTED]> wrote:
> Hi folks,
>
> [PATCH] Blackfin: blackfin i2c driver
>
> + struct i2c_msg *pmsg;
> + int i, ret;
> + int rc = 0;
> +
> + if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> + return -ENXIO;
> +
> + down(&iface->twi_lock);
> +
> + while (bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> + up(&iface->twi_lock);
> + schedule();
> + down(&iface->twi_lock);
> + }
That's a busy loop until this task's timeslice has expired. It'll work,
but it'll suck a bit. (Repeated in several places)
OK, I change it into yield(). So, current process will be move to the
tail of the run queue. Is that OK with you?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On 3/6/07, Alexey Dobriyan <[EMAIL PROTECTED]> wrote:
> +static int __init i2c_bfin_twi_init(void)
> +{
> + rc = request_irq(twi_iface.irq, bfin_twi_interrupt_entry, SA_INTERRUPT,
"i2c-bfin-twi", &twi_iface);
> + if (rc) {
> + printk(KERN_ERR "i2c-bfin-twi: can't get IRQ %d !\n",
twi_iface.irq);
> + return -ENODEV;
> + }
> +
> + /* Set TWI internal clock as 10MHz */
> + bfin_write_TWI_CONTROL(((get_sclk() / 1024 / 1024 + 5) / 10) & 0x7F);
> +
> + /* Set Twi interface clock as specified */
> + if (CONFIG_TWICLK_KHZ > 400)
> + bfin_write_TWI_CLKDIV((( 5*1024 / 400 ) << 8) | (( 5*1024 / 400 )
& 0xFF));
> + else
> + bfin_write_TWI_CLKDIV((( 5*1024 / CONFIG_TWICLK_KHZ ) << 8) | ((
5*1024 / CONFIG_TWICLK_KHZ ) & 0xFF));
> +
> + /* Enable TWI */
> + bfin_write_TWI_CONTROL(bfin_read_TWI_CONTROL() | TWI_ENA);
> + SSYNC();
> +
> + return i2c_add_adapter(p_adap);
free_irq on error?
what error ? do you mean i2c_add_adapter() ?
-mike
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On Tue, Mar 06, 2007 at 02:54:18PM +0800, Wu, Bryan wrote:
> [PATCH] Blackfin: blackfin i2c driver
>
> The i2c linux driver for blackfin architecture which supports both GPIO
> i2c operation and blackfin on-chip TWI controller i2c operation.
> +config TWICLK_KHZ
> + int "TWI clock (kHZ)"
> + depends on I2C_BFIN_TWI
> + default 50
> + help
> + The unit of the TWI clock is kilo HZ. Please divide the clock
> + by 1024 if you count it in HZ. The value should be less than
> 400.
Indentation in Kconfig files is Tab-Space-Space.
> --- /dev/null
> +++ linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c
> +static int __init i2c_hhbf_init(void)
> +{
> +
> +if(gpio_request(CONFIG_BFIN_SCL, NULL)) {
> + printk(KERN_ERR "%s: gpio_request GPIO %d failed \n",__FUNCTION__,
> CONFIG_BFIN_SCL);
I may be first to ask this, but, please, use __func__ in all new code.
__FUNCTION__ is gcc-2.95-ism, which is no longer supported.
> + return -1;
> + }
> +
> +if(gpio_request(CONFIG_BFIN_SDA, NULL)) {
> + printk(KERN_ERR "%s: gpio_request GPIO %d failed \n",__FUNCTION__,
> CONFIG_BFIN_SDA);
__func__, Do you need to gpio_free() ?
> + return -1;
> + }
> +
> +
> +gpio_direction_output(CONFIG_BFIN_SCL);
> +gpio_direction_input(CONFIG_BFIN_SDA);
> +gpio_set_value(CONFIG_BFIN_SCL, 1);
> +
> +return i2c_bit_add_bus(&hhbf_ops);
Do you need unwinding with gpio_free() like below?
> +static void __exit i2c_hhbf_exit(void)
> +{
> +gpio_free(CONFIG_BFIN_SCL);
> +gpio_free(CONFIG_BFIN_SDA);
trailing whitespace.
> +i2c_bit_del_bus(&hhbf_ops);
> +}
> --- /dev/null
> +++ linux-2.6/drivers/i2c/busses/i2c-bfin-twi.c
> +static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
> +{
> + unsigned short twi_int_stat = bfin_read_TWI_INT_STAT();
> + unsigned short mast_stat = bfin_read_TWI_MASTER_STAT();
> +
> + if (XMTSERV & twi_int_stat) {
Please, put bitmasks on the right. Much easier to read. See below for
other places.
if (twi_int_stat & XMTSERV)
Also, I'd rename variable to twi_intr_status, to not confuse with
statistics or something.
> + /* Transmit next data */
> + if (iface->writeNum > 0) {
> + bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
> + iface->writeNum--;
StudlyCaps.
> +static int bfin_twi_master_xfer(struct i2c_adapter *adap, struct i2c_msg
> msgs[], int num)
> +{
> + struct bfin_twi_iface* iface = (struct bfin_twi_iface*)adap->algo_data;
> + struct i2c_msg *pmsg;
> + int i, ret;
> + int rc = 0;
> +
> + if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> + return -ENXIO;
> +
> + down(&iface->twi_lock);
> +
> + while (bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> + up(&iface->twi_lock);
> + schedule();
> + down(&iface->twi_lock);
> + }
> +int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write,
> + u8 command, int size, union i2c_smbus_data * data)
> +{
> + struct bfin_twi_iface* iface = (struct bfin_twi_iface*)adap->algo_data;
> + int rc = 0;
> +
> + if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> + return -ENXIO;
> +
> + down(&iface->twi_lock);
> +
> + while(bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> + up(&iface->twi_lock);
> + schedule();
> + down(&iface->twi_lock);
> + }
Smells like schedule() abuse for delays.
> + switch (iface->cur_mode) {
> + case TWI_I2C_MODE_STANDARDSUB:
> + bfin_write_TWI_XMT_DATA8(iface->command);
> + bfin_write_TWI_INT_MASK(MCOMP | MERR | ((iface->read_write ==
> I2C_SMBUS_READ) ? RCVSERV : XMTSERV));
> + SSYNC();
> +
> + if (iface->writeNum + 1 <= 255)
> + bfin_write_TWI_MASTER_CTL(((iface->writeNum+1) << 6));
Too many ():
bfin_write_TWI_MASTER_CTL((iface->writeNum + 1) << 6);
> + else {
> + bfin_write_TWI_MASTER_CTL(( 0xff << 6 ));
ditto
> + iface->manual_stop = 1;
> + }
> + /* Master enable */
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> ((CONFIG_TWICLK_KHZ>100) ? FAST : 0));
> + break;
> + case TWI_I2C_MODE_COMBINED:
> + bfin_write_TWI_XMT_DATA8(iface->command);
> + bfin_write_TWI_INT_MASK(MCOMP | MERR | RCVSERV | XMTSERV);
> + SSYNC();
> +
> + if (iface->writeNum > 0)
> + bfin_write_TWI_MASTER_CTL(((iface->writeNum+1) << 6));
ditto
> + else
> + bfin_write_TWI_MASTER_CTL(( 0x1 << 6 ));
ditto, etc
> +static int __init i2c_bfin_twi_init(void)
> +{
> + rc = request_irq(twi_iface.irq, bfin_twi_interrupt_entry, SA_INTERRUPT,
> "i2c-bfin-tw
Re: [PATCH -mm] Blackfin: blackfin i2c driver
On Tue, 06 Mar 2007 14:54:18 +0800 "Wu, Bryan" <[EMAIL PROTECTED]> wrote:
> Hi folks,
>
> [PATCH] Blackfin: blackfin i2c driver
>
> The i2c linux driver for blackfin architecture which supports both GPIO
> i2c operation and blackfin on-chip TWI controller i2c operation.
>
Little things...
> +static int __init i2c_hhbf_init(void)
> +{
> +
> +if(gpio_request(CONFIG_BFIN_SCL, NULL)) {
> + printk(KERN_ERR "%s: gpio_request GPIO %d failed \n",__FUNCTION__,
> CONFIG_BFIN_SCL);
> + return -1;
> + }
> +
> +if(gpio_request(CONFIG_BFIN_SDA, NULL)) {
> + printk(KERN_ERR "%s: gpio_request GPIO %d failed \n",__FUNCTION__,
> CONFIG_BFIN_SDA);
> + return -1;
> + }
whitespace breakage there
> +
> +gpio_direction_output(CONFIG_BFIN_SCL);
> +gpio_direction_input(CONFIG_BFIN_SDA);
> +gpio_set_value(CONFIG_BFIN_SCL, 1);
> +
> +return i2c_bit_add_bus(&hhbf_ops);
> +}
> +
> +#define TWI_I2C_MODE_COMBINED0x04
> +
> +struct bfin_twi_iface
> +{
struct bfin_twi_iface {
> + struct semaphoretwi_lock;
Can this be converted to a mutex?
> + int irq;
> + spinlock_t lock;
> + */
>
> ...
>
> +static int bfin_twi_master_xfer(struct i2c_adapter *adap, struct i2c_msg
> msgs[], int num)
> +{
> + struct bfin_twi_iface* iface = (struct bfin_twi_iface*)adap->algo_data;
This code has zillions of unneeded casts of void*
> + struct i2c_msg *pmsg;
> + int i, ret;
> + int rc = 0;
> +
> + if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> + return -ENXIO;
> +
> + down(&iface->twi_lock);
> +
> + while (bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> + up(&iface->twi_lock);
> + schedule();
> + down(&iface->twi_lock);
> + }
That's a busy loop until this task's timeslice has expired. It'll work,
but it'll suck a bit. (Repeated in several places)
> + ret = 0;
> + for (i = 0; rc >= 0 && i < num;) {
> + pmsg = &msgs[i++];
Strange. Why not do the i++ in the `for' statement?
> + if (pmsg->flags & I2C_M_TEN) {
> + printk(KERN_ERR "i2c-bfin-twi: 10 bits addr not
> supported !\n");
> + rc = -EINVAL;
> + break;
> + }
> +
>
> ...
>
> + switch (iface->cur_mode) {
> + case TWI_I2C_MODE_STANDARDSUB:
> + bfin_write_TWI_XMT_DATA8(iface->command);
> + bfin_write_TWI_INT_MASK(MCOMP | MERR | ((iface->read_write ==
> I2C_SMBUS_READ) ? RCVSERV : XMTSERV));
It's preferred if code is readable in an 80-col display.
> + SSYNC();
> +
> + if (iface->writeNum + 1 <= 255)
> + bfin_write_TWI_MASTER_CTL(((iface->writeNum+1) << 6));
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH -mm] Blackfin: blackfin i2c driver
Hi folks,
[PATCH] Blackfin: blackfin i2c driver
The i2c linux driver for blackfin architecture which supports both GPIO
i2c operation and blackfin on-chip TWI controller i2c operation.
Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
---
drivers/i2c/busses/Kconfig | 47
drivers/i2c/busses/i2c-bfin-gpio.c | 98 ++
drivers/i2c/busses/i2c-bfin-twi.c | 530
3 files changed, 675 insertions(+)
Index: linux-2.6/drivers/i2c/busses/Kconfig
===
--- linux-2.6.orig/drivers/i2c/busses/Kconfig 2007-03-05 11:20:04.0
+0800
+++ linux-2.6/drivers/i2c/busses/Kconfig2007-03-06 14:46:46.0
+0800
@@ -5,6 +5,53 @@
menu "I2C Hardware Bus support"
depends on I2C
+config I2C_BFIN_GPIO
+ tristate "Generic Blackfin and HHBF533/561 development board I2C
support"
+ depends on I2C && EXPERIMENTAL
+ select I2C_ALGOBIT
+ help
+ --
+
+menu "BFIN I2C SDA/SCL Selection"
+ depends on I2C_BFIN_GPIO
+config BFIN_SDA
+ int "SDA is GPIO Number"
+ range 0 15 if (BF533 || BF532 || BF531)
+ range 0 47 if (BF534 || BF536 || BF537)
+ range 0 47 if BF561
+ default 2 if (BF533 || BF532 || BF531)
+
+config BFIN_SCL
+ int "SCL is GPIO Number"
+ range 0 15 if (BF533 || BF532 || BF531)
+ range 0 47 if (BF534 || BF536 || BF537)
+ range 0 47 if BF561
+ default 3
+endmenu
+
+config I2C_BFIN_GPIO_CYCLE_DELAY
+ int "Cycle Delay in usec"
+ depends on I2C_BFIN_GPIO
+ range 1 100
+ default 40
+
+config I2C_BFIN_TWI
+ tristate "Blackfin TWI I2C support"
+ depends on I2C && (BF534 || BF536 || BF537)
+ help
+ This the TWI I2C device driver for Blackfin 534/536/537.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-bfin-twi.
+
+config TWICLK_KHZ
+ int "TWI clock (kHZ)"
+ depends on I2C_BFIN_TWI
+ default 50
+ help
+ The unit of the TWI clock is kilo HZ. Please divide the clock
+ by 1024 if you count it in HZ. The value should be less than
400.
+
config I2C_ALI1535
tristate "ALI 1535"
depends on I2C && PCI
Index: linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c2007-03-06
14:47:31.0 +0800
@@ -0,0 +1,98 @@
+/
+ * Description: *
+ * *
+ * Maintainer: Meihui Fan <[EMAIL PROTECTED]> *
+ * *
+ * CopyRight (c) 2004 HHTech *
+ * www.hhcn.com, www.hhcn.org *
+ * All rights reserved. *
+ * *
+ * This file is free software; *
+ * you are free to modify and/or redistribute it *
+ * under the terms of the GNU General Public Licence (GPL). *
+ * *
+ /
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#defineI2C_HW_B_HHBF 0x13
+
+static void hhbf_setsda(void *data, int state)
+{
+if (state) {
+ gpio_direction_input(CONFIG_BFIN_SDA);
+
+} else {
+ gpio_direction_output(CONFIG_BFIN_SDA);
+ gpio_set_value(CONFIG_BFIN_SDA, 0);
+}
+}
+
+static void hhbf_setscl(void *data, int state)
+{
+ gpio_set_value(CONFIG_BFIN_SCL, state);
+}
+
+static int hhbf_getsda(void *data)
+{
+ return (gpio_get_value(CONFIG_BFIN_SDA) != 0);
+}
+
+
+static struct i2c_algo_bit_data bit_hhbf_data = {
+.setsda = hhbf_setsda,
+.setscl = hhbf_setscl,
+.getsda = hhbf_getsda,
+.udelay = CONFIG_I2C_BFIN_GPIO_CYCLE_DELAY,
+.timeout = HZ
+};
+
+static struct i2c_adapter hhbf_ops = {
+.owner = THIS_MODULE,
+.id= I2C_HW_B_HHBF,
+.algo_data = &bit_hhbf_data,
+.name = "HHBF I2C driver",
+};
+
+static int __init i2c_hhbf_init(void)
+{
+
+if(gpio_request(CONFIG_BFIN_SCL, NULL)) {
+ printk(KERN_ERR "%s: gpio_request GPIO %d failed \n",__FUNCTION__,
CONFIG_BFIN_SCL);
+ return -1;
+ }
+
+if(gpio_request(CONFIG_BFIN_SDA, NULL)) {
+ printk(KERN_ERR "%s: gpio_request GPIO %d failed \n",__FUNCTION__,
CONFIG_BFIN_SDA);
+ return -1;
+ }
+
+
+gpio_direction_output(CONFIG_BFIN_SCL);
+gpio_direction
