i2c: Renesas Highlander FPGA SMBus support.
Subsystems (2)
PATCH 1/1 i2c: Renesas Highlander FPGA SMBus support.
This adds support for the SMBus adapter found in the various FPGAs on the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and R0P7785LC0011RL FPGAs.
Functionality is fairly restricted, in that only byte and block data transfers are supported. Normal/fast mode and IRQ/polling are also supported. Primarily used for various RTCs and thermal sensors.
---
drivers/i2c/busses/Kconfig | 12 drivers/i2c/busses/Makefile | 1 drivers/i2c/busses/i2c-highlander.c | 500 ++++++++++++++++++++++++++++++++++++ 3 files changed, 513 insertions(+)
drivers/i2c/busses/Kconfig +12 -0
@@ -146,6 +146,18 @@ config I2C_GPIO This is a very simple bitbanging I2C driver utilizing the arch-neutral GPIO API to control the SCL and SDA lines. +config I2C_HIGHLANDER + tristate "Highlander FPGA SMBus interface" + depends on SH_HIGHLANDER + help + If you say yes to this option, support will be included for + the SMBus interface located in the FPGA on various Highlander + boards, particularly the R0P7780LC0011RL and R0P7785LC0011RL + FPGAs. This is wholly unrelated to the SoC I2C. + + This driver can also be built as a module. If so, the module + will be called i2c-highlander. + config I2C_HYDRA tristate "CHRP Apple Hydra Mac I/O I2C interface" depends on PCI && PPC_CHRP && EXPERIMENTAL
drivers/i2c/busses/Makefile +1 -0
@@ -14,6 +14,7 @@ obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o obj-$(CONFIG_I2C_DAVINCI) += i2c-davinci.o obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o +obj-$(CONFIG_I2C_HIGHLANDER) += i2c-highlander.o obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o obj-$(CONFIG_I2C_I801) += i2c-i801.o obj-$(CONFIG_I2C_I810) += i2c-i810.o
drivers/i2c/busses/i2c-highlander.c new file +500 -0
500 lines not on this page · raw message
Quoted 9 lines · Paul Mundt
This adds support for the SMBus adapter found in the various FPGAs on the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and R0P7785LC0011RL FPGAs.
Functionality is fairly restricted, in that only byte and block data transfers are supported. Normal/fast mode and IRQ/polling are also supported. Primarily used for various RTCs and thermal sensors.
Signed-off-by Paul Mundt <[email protected]>
No volunteer to review this driver? Manuel or Magnus maybe?
Thanks,
Signature · 1 line
Jean Delvare
Hi Jean,
Quoted 5 lines · Jean Delvare
Quoted 4 lines · Paul Mundt
This adds support for the SMBus adapter found in the various FPGAs on the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and R0P7785LC0011RL FPGAs.
Quoted 7 lines
Quoted 5 lines
Functionality is fairly restricted, in that only byte and block data transfers are supported. Normal/fast mode and IRQ/polling are also supported. Primarily used for various RTCs and thermal sensors.
Signed-off-by Paul Mundt <[email protected]>No volunteer to review this driver? Manuel or Magnus maybe?
I don't think I'm qualified to review other peoples' code (it looks fine to me). The other thing is Renesas demoboards are impossible to get outside of Japan so I think Paul is the only user of this hardware who is also interested in linux support. And I'm convinced he knows what he's doing ;-)
Thanks,
Manuel Lauss
Quoted 14 lines · Manuel Lauss
Hi Jean,
Quoted 11 lines · Jean Delvare
Quoted 9 lines · Paul Mundt
This adds support for the SMBus adapter found in the various FPGAs on the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and R0P7785LC0011RL FPGAs.
Functionality is fairly restricted, in that only byte and block data transfers are supported. Normal/fast mode and IRQ/polling are also supported. Primarily used for various RTCs and thermal sensors.
Signed-off-by Paul Mundt <[email protected]>
Quoted 7 lines
Quoted 1 line
No volunteer to review this driver? Manuel or Magnus maybe?
I don't think I'm qualified to review other peoples' code (it looks fine to me). The other thing is Renesas demoboards are impossible to get outside of Japan so I think Paul is the only user of this hardware who is also interested in linux support. And I'm convinced he knows what he's doing ;-)
I guess that there are more users than just Paul, otherwise he wouldn't bother pushing his driver upstream, would he?
Signature · 1 line
Jean Delvare
Quoted 15 lines · Jean Delvare
Quoted 14 lines · Manuel Lauss
Quoted 12 lines · Jean Delvare
Quoted 9 lines · Paul Mundt
This adds support for the SMBus adapter found in the various FPGAs on the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and R0P7785LC0011RL FPGAs.
Functionality is fairly restricted, in that only byte and block data transfers are supported. Normal/fast mode and IRQ/polling are also supported. Primarily used for various RTCs and thermal sensors.
Signed-off-by Paul Mundt <[email protected]>No volunteer to review this driver? Manuel or Magnus maybe?
Quoted 9 lines
Quoted 5 lines
I don't think I'm qualified to review other peoples' code (it looks fine to me). The other thing is Renesas demoboards are impossible to get outside of Japan so I think Paul is the only user of this hardware who is also interested in linux support. And I'm convinced he knows what he's doing ;-)
I guess that there are more users than just Paul, otherwise he wouldn't bother pushing his driver upstream, would he?
Even if I were the only user, I would still push it upstream. Though in this case, there are plenty of other users. This is a fairly common reference board in Japan, which we've supported in the kernel in various forms for a few years already. The FPGA SMBus implementation is one of the outstanding bits of support for the platform.
Andrew Morton
Reviewed-byQuoted 13 lines · On Fri, 25 Apr 2008 10:30:11 +0900 Paul Mundt <[email protected]> wrote:
Quoted 11 lines · Jean Delvare
Quoted 10 lines · Manuel Lauss
Quoted 9 lines · Jean Delvare
Quoted 8 lines · Paul Mundt
This adds support for the SMBus adapter found in the various FPGAs on the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and R0P7785LC0011RL FPGAs.
Functionality is fairly restricted, in that only byte and block data transfers are supported. Normal/fast mode and IRQ/polling are also supported. Primarily used for various RTCs and thermal sensors.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Paul Mundt <[email protected]>No volunteer to review this driver? Manuel or Magnus maybe?
I don't think I'm qualified to review other peoples' code (it looks fine to me).
I looked through it when I merged it - believe it or not, I always do (well, except for some dopey mechanical code transformation patches where I'll just believe the changelog). I saw nothing worth commenting on. As is always the case when I don't comment ;)
So here's a
Although that is of course of limited use, coming from a person who isn't terribly sure what an i2c is.
Quoted 11 lines · Andrew Morton
Quoted 5 lines · On Fri, 25 Apr 2008 10:30:11 +0900 Paul Mundt <[email protected]> wrote:
Quoted 3 lines · Jean Delvare
Quoted 2 lines · Manuel Lauss
I don't think I'm qualified to review other peoples' code (it looks fine to me).
I looked through it when I merged it - believe it or not, I always do (well, except for some dopey mechanical code transformation patches where I'll just believe the changelog). I saw nothing worth commenting on. As is always the case when I don't comment ;)
Quoted 5 lines
So here's a
Reviewed-by Andrew Morton <[email protected]>Although that is of course of limited use, coming from a person who isn't terribly sure what an i2c is.
This is the root of the issue, none of the people asked to review the code are i2c people either. This is a pretty sad state for the subsystem if the subsystem maintainer needs to defer to people with little to no knowledge of the subsystem to "review" a driver before it can be merged.
While Manuel, Magnus, and I can easily review and ack our patches, none of this changes the fact that outside of the platform and architecture specific bits in the driver, there's very little we can generally comment on. The reason for soliciting feedback from the i2c list in the first place was to get review and comments on the subsystem-specific bits from the people who are obviously far more familiar with these things. I understand that Jean isn't an embedded person and therefore isn't comfortable reviewing those sorts of drivers, but in these cases it's the bus-specific stuff where the review really matters, which obviously the rest of us aren't in the best position to self-review.
If it's not possible to get a subsystem maintainer to review a patch, what's the point of having a centralized subsystem in the first place?
Quoted 23 lines · Paul Mundt
Quoted 11 lines · Andrew Morton
Quoted 5 lines · On Fri, 25 Apr 2008 10:30:11 +0900 Paul Mundt <[email protected]> wrote:
Quoted 3 lines · Jean Delvare
Quoted 2 lines · Manuel Lauss
I don't think I'm qualified to review other peoples' code (it looks fine to me).
I looked through it when I merged it - believe it or not, I always do (well, except for some dopey mechanical code transformation patches where I'll just believe the changelog). I saw nothing worth commenting on. As is always the case when I don't comment ;)
Quoted 5 lines
So here's a
Reviewed-by Andrew Morton <[email protected]>Although that is of course of limited use, coming from a person who isn't terribly sure what an i2c is.
This is the root of the issue, none of the people asked to review the code are i2c people either. This is a pretty sad state for the subsystem if the subsystem maintainer needs to defer to people with little to no knowledge of the subsystem to "review" a driver before it can be merged.
Quoted 10 lines
While Manuel, Magnus, and I can easily review and ack our patches, none of this changes the fact that outside of the platform and architecture specific bits in the driver, there's very little we can generally comment on. The reason for soliciting feedback from the i2c list in the first place was to get review and comments on the subsystem-specific bits from the people who are obviously far more familiar with these things. I understand that Jean isn't an embedded person and therefore isn't comfortable reviewing those sorts of drivers, but in these cases it's the bus-specific stuff where the review really matters, which obviously the rest of us aren't in the best position to self-review.
OK, I just reviewed your driver. I had 20 comments, only 6 of them required knowledge about the i2c subsystem. The 14 remaining comments could have been made by about anybody with some experience with Linux kernel development.
Given the limited time I have to review new i2c drivers, my hope was some other people could take care of the first review catching most of the non-i2c-related issues, and then I could just focus on the i2c side of things. But I guess I'm asking for too much.
Quoted 2 lines
If it's not possible to get a subsystem maintainer to review a patch, what's the point of having a centralized subsystem in the first place?
I don't even understand your question, but I doubt it deserves an answer anyway.
Signature · 1 line
Jean Delvare
Hi Paul,
Quoted 10 lines · Paul Mundt
This adds support for the SMBus adapter found in the various FPGAs on the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and R0P7785LC0011RL FPGAs.
Functionality is fairly restricted, in that only byte and block data transfers are supported. Normal/fast mode and IRQ/polling are also supported. Primarily used for various RTCs and thermal sensors.
Signed-off-by Paul Mundt <[email protected]>
Review:
Quoted 88 lines
---
drivers/i2c/busses/Kconfig | 12 drivers/i2c/busses/Makefile | 1 drivers/i2c/busses/i2c-highlander.c | 500 ++++++++++++++++++++++++++++++++++++ 3 files changed, 513 insertions(+)drivers/i2c/busses/Kconfig +12
-0@@ -146,6 +146,18 @@ config I2C_GPIO This is a very simple bitbanging I2C driver utilizing the arch-neutral GPIO API to control the SCL and SDA lines. +config I2C_HIGHLANDER + tristate "Highlander FPGA SMBus interface" + depends on SH_HIGHLANDER + help + If you say yes to this option, support will be included for + the SMBus interface located in the FPGA on various Highlander + boards, particularly the R0P7780LC0011RL and R0P7785LC0011RL + FPGAs. This is wholly unrelated to the SoC I2C. + + This driver can also be built as a module. If so, the module + will be called i2c-highlander. + config I2C_HYDRA tristate "CHRP Apple Hydra Mac I/O I2C interface" depends on PCI && PPC_CHRP && EXPERIMENTALdrivers/i2c/busses/Makefile +1
-0@@ -14,6 +14,7 @@ obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o obj-$(CONFIG_I2C_DAVINCI) += i2c-davinci.o obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o +obj-$(CONFIG_I2C_HIGHLANDER) += i2c-highlander.o obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o obj-$(CONFIG_I2C_I801) += i2c-i801.o obj-$(CONFIG_I2C_I810) += i2c-i810.odrivers/i2c/busses/i2c-highlander.c new file +40
-0@@ -0,0 +1,500 @@ +/* +* Renesas Solutions Highlander FPGA I2C/SMBus support. +* +* Supported devices: R0P7780LC0011RL, R0P7785LC0011RL +* +* Copyright (C) 2008 Paul Mundt +* Copyright (C) 2008 Renesas Solutions Corp. +* Copyright (C) 2008 Atom Create Engineering Co., Ltd. +* +* This file is subject to the terms and conditions of the GNU General +* Public License version 2. See the file "COPYING" in the main directory +* of this archive for more details. +*/ +#include <linux/module.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/i2c.h> +#include <linux/platform_device.h> +#include <linux/completion.h> +#include <linux/io.h> +#include <linux/delay.h> + +#define SMCR 0x00 +#define SMCR_START (1 << 0) +#define SMCR_IRIC (1 << 1) +#define SMCR_BBSY (1 << 2) +#define SMCR_ACKE (1 << 3) +#define SMCR_RST (1 << 4) +#define SMCR_IEIC (1 << 6) + +#define SMSMADR 0x02 + +#define SMMR 0x04 +#define SMMR_MODE0 (1 << 0) +#define SMMR_MODE1 (1 << 1) +#define SMMR_CAP (1 << 3) +#define SMMR_TMMD (1 << 4) +#define SMMR_SP (1 << 7) + +#define SMSADR 0x06
drivers/i2c/busses/i2c-highlander.c line 50
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+#define SMTRDR 0x46 + +struct highlander_i2c_dev { +struct device *dev; +void __iomem *base; +struct i2c_adapter adapter; +struct completion cmd_complete; +int irq; +u8 *buf; +size_t buf_len;
Your driver is a bit inconsistent with these two last struct members. You only access them in highlander_i2c_smbus_xfer(), the rest of the time you pass them to functions as extra parameters (while you are _also_ passing a pointer to the struct highlander_i2c_dev.) Please choose a strategy and stick to it: either use these struct members to carry the values around, or drop these members and use local variables instead. Mixing both is inefficient and confusing.
Quoted 15 lines
drivers/i2c/busses/i2c-highlander.c new file +15
-0+}; + +static int iic_force_poll, iic_force_normal, iic_timeout =1000 ; + +static inline void highlander_i2c_irq_enable (struct highlander_i2c_dev *dev) +{ +iowrite16 (ioread16 (dev->base +SMCR ) |SMCR_IEIC , dev->base +SMCR ); +} + +static inline void highlander_i2c_irq_disable (struct highlander_i2c_dev *dev) +{ +iowrite16 (ioread16 (dev->base +SMCR ) & ~SMCR_IEIC , dev->base +SMCR ); +} + +static inline void highlander_i2c_start (struct highlander_i2c_dev *dev)
drivers/i2c/busses/i2c-highlander.c line 75
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+{ +iowrite16 (ioread16 (dev->base +SMCR ) |SMCR_START , dev->base +SMCR ); +} + +static inline void highlander_i2c_done (struct highlander_i2c_dev *dev) +{ +iowrite16 (ioread16 (dev->base +SMCR ) |SMCR_IRIC , dev->base +SMCR ); +} + +static inline void highlander_i2c_setup (struct highlander_i2c_dev *dev)
There's no point in forcing this function to be inlined, it's definitely not performance critical.
Quoted 4 lines
drivers/i2c/busses/i2c-highlander.c new file +4
-0+{ +u16 smmr; + + smmr =ioread16 (dev->base +SMMR );
drivers/i2c/busses/i2c-highlander.c line 89
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+ smmr |=SMMR_TMMD ; + +if (iic_force_normal) + smmr &= ~SMMR_SP ; +else + smmr |=SMMR_SP ; + +iowrite16 (smmr, dev->base +SMMR ); +} +
A comment about why the following 2 functions are needed and/or what they are doing would be nice to have. Also, the order of the parameters is rather error-prone, most C functions copying things from one buffer to another have the destination as first parameter, for example memcpy.
These functions seem to only support even values for len (except for 1), this should be mentioned.
Quoted 20 lines
drivers/i2c/busses/i2c-highlander.c new file +20
-0+static void smbus_write_data (u8 *src,u16 *dst,int len) +{ +int i, j; + +if (len ==1 ) + *dst = *src <<8 ; +else { + j =0 ; +for (i =0 ; i < len; i +=2 ) { + *(dst + j) = *(src + i) <<8 | *(src + i +1 ); + j++; + } + } +} + +static void smbus_read_data (u16 *src,u8 *dst,int len) +{ +int i, j; + +if (len ==1 )
drivers/i2c/busses/i2c-highlander.c line 119
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+ *dst = *src >>8 ; + else { + j =0 ; +for (i =0 ; i < len; i +=2 ) { + *(dst + i) = *(src + j) >>8 ; + *(dst + i +1 ) = *(src + j) &0x00ff ; + j++; + } + } +}
If I read the code above correctly, you are merely converting 16-bit words from cpu-endian to long-endian and back, so using be16_to_cpu and cpu_to_be16 would perform better. If the Highlander is big-endian, you should be able to let the compiler optimize out most of the code.
Quoted 101 lines
drivers/i2c/busses/i2c-highlander.c new file +101
-0+ +static void highlander_i2c_command (struct highlander_i2c_dev *dev,u8 command,int len) +{ +u16 cmd[32 ]; +int i, j; + + j =0 ; +if (len ==1 ) + cmd[j++] = (command <<8 ); +else +for (i =0 ; i < len; i +=2 ) + cmd[j++] = (command <<8 ) | command; + +for (i =0 ; i < j; i++) { +iowrite16 (cmd[i], dev->base +SMSADR + (i *sizeof (u16))); +dev_dbg (dev->dev,"command data[%x] 0x%04x\n" , i, cmd[i]); + } +} + +static int highlander_i2c_wait_for_bbsy (struct highlander_i2c_dev *dev) +{ +unsigned long timeout; + + timeout = jiffies +msecs_to_jiffies (iic_timeout); +while (ioread16 (dev->base +SMCR ) &SMCR_BBSY ) { +if (time_after (jiffies, timeout)) { +dev_warn (dev->dev,"timeout waiting for bus ready\n" ); +return -ETIMEDOUT ; + } + +msleep (1 ); + } + +return 0 ; +} + +static int highlander_i2c_reset (struct highlander_i2c_dev *dev) +{ +iowrite16 (ioread16 (dev->base +SMCR ) |SMCR_RST , dev->base +SMCR ); +return highlander_i2c_wait_for_bbsy (dev); +} + +static int highlander_i2c_wait_for_ack (struct highlander_i2c_dev *dev) +{ +u16 tmp =ioread16 (dev->base +SMCR ); + +if ((tmp & (SMCR_IRIC |SMCR_ACKE )) ==SMCR_ACKE ) { +dev_warn (dev->dev,"ack abnormality\n" ); +return highlander_i2c_reset (dev); + } + +return 0 ; +} + +static irqreturn_t highlander_i2c_irq (int irq,void *dev_id) +{ +struct highlander_i2c_dev *dev = dev_id; + +highlander_i2c_done (dev); +complete (&dev->cmd_complete); + +return IRQ_HANDLED ; +} + +static void highlander_i2c_poll (struct highlander_i2c_dev *dev) +{ +unsigned long timeout; +u16 smcr; + + timeout = jiffies +msecs_to_jiffies (iic_timeout); +for (;;) { + smcr =ioread16 (dev->base +SMCR ); + +/* +* Don't bother checking ACKE here, this and the reset +* are handled in highlander_i2c_wait_xfer_done() when +* waiting for the ACK. +*/ + +if (smcr &SMCR_IRIC ) +return ; +if (time_after (jiffies, timeout)) +break ; + +cpu_relax (); +cond_resched (); + } + +dev_err (dev->dev,"polling timed out\n" ); +} + +static inline int highlander_i2c_wait_xfer_done (struct highlander_i2c_dev *dev) +{ +if (dev->irq) +wait_for_completion_interruptible_timeout (&dev->cmd_complete, +msecs_to_jiffies (iic_timeout)); +else +/* busy looping, the IRQ of champions */ +highlander_i2c_poll (dev); + +return highlander_i2c_wait_for_ack (dev);
drivers/i2c/busses/i2c-highlander.c line 230
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+} + +static int highlander_i2c_read (struct highlander_i2c_dev *dev, +u8 *buf,unsigned int len) +{ +int i, cnt; +u16 data[16 ]; + +if (highlander_i2c_wait_for_bbsy (dev)) { +dev_warn (dev->dev,"timed out\n" );
highlander_i2c_wait_for_bbsy already prints a warning message.
drivers/i2c/busses/i2c-highlander.c line 238
Quoted 8 lines
drivers/i2c/busses/i2c-highlander.c new file +8
-0+return -EAGAIN ; + } + +highlander_i2c_start (dev); + +if (highlander_i2c_wait_xfer_done (dev)) { +dev_err (dev->dev,"Arbitration loss\n" ); +return -EIO ;
I think you should return -EAGAIN here as well... On arbitration loss the caller will certainly want to retry later, same as if the bus was busy?
Quoted 7 lines
drivers/i2c/busses/i2c-highlander.c new file +7
-0+ } + + cnt = (len +1 ) >>1 ; +for (i =0 ; i < cnt; i++) { + data[i] =ioread16 (dev->base +SMTRDR + (i *sizeof (u16))); +dev_dbg (dev->dev,"read data[%x] 0x%04x\n" , i, data[i]); + }
drivers/i2c/busses/i2c-highlander.c line 255
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+ +smbus_read_data (data, buf, len); + +/* +* The R0P7780LC0011RL FPGA on the SH7780-based Highlanders +* needs a significant delay in the read path. SH7785 Highlanders +* don't have this issue, so restrict it entirely to those. +*/ +if (mach_is_r7780rp () ||mach_is_r7780mp ()) +mdelay (1000 );
A one second busy-wait is nasty. Can't you use msleep here instead?
Having to wait for 1 second after each read makes this driver pretty unusable on these machines anyway, doesn't it? :(
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+ +return 0 ; +} + +static int highlander_i2c_write (struct highlander_i2c_dev *dev, +u8 *buf,unsigned int len) +{ +int i, cnt; +u16 data[16 ]; +
drivers/i2c/busses/i2c-highlander.c line 275
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+smbus_write_data (buf, data, len); + + cnt = (len +1 ) >>1 ; +for (i =0 ; i < cnt; i++) { +iowrite16 (data[i], dev->base +SMTRDR + (i *sizeof (u16))); +dev_dbg (dev->dev,"write data[%x] 0x%04x\n" , i, data[i]); + } + +if (highlander_i2c_wait_for_bbsy (dev)) { +dev_warn (dev->dev,"timed out\n" );
highlander_i2c_wait_for_bbsy already prints a warning message.
Quoted 15 lines
drivers/i2c/busses/i2c-highlander.c new file +15
-0+return -EAGAIN ; + } + +highlander_i2c_start (dev); + +return highlander_i2c_wait_xfer_done (dev); +} + +static int highlander_i2c_smbus_xfer (struct i2c_adapter *adap,u16 addr, +unsigned short flags,char read_write, +u8 command,int size, +union i2c_smbus_data *data) +{ +struct highlander_i2c_dev *dev =i2c_get_adapdata (adap); +int read = read_write &I2C_SMBUS_READ ;
drivers/i2c/busses/i2c-highlander.c line 300
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+u16 tmp; + +init_completion (&dev->cmd_complete); + +dev_dbg (dev->dev,"addr %04x, command %02x, read_write %d, size %d\n" , + addr, command, read_write, size); + +/* Defaults */ + dev->buf =NULL ; + dev->buf_len =0 ;
These initializations shouldn't be needed.
drivers/i2c/busses/i2c-highlander.c line 308
Quoted 8 lines
drivers/i2c/busses/i2c-highlander.c new file +8
-0+ +/* +* Set up the buffer and transfer size +*/ +switch (size) { +case I2C_SMBUS_BYTE_DATA : +if (data) + dev->buf = &data->byte;
It is not valid for data to be null for I2C_SMBUS_BYTE_DATA, so this test isn't needed.
drivers/i2c/busses/i2c-highlander.c line 315
Quoted 7 lines
drivers/i2c/busses/i2c-highlander.c new file +7
-0+ dev->buf_len =1 ; +break ; +case I2C_SMBUS_BLOCK_DATA : +case I2C_SMBUS_I2C_BLOCK_DATA : + dev->buf = &data->block[1 ]; + dev->buf_len = data->block[0 ]; +break ;
That's definitely not correct. Given that you don't differentiate between I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_I2C_BLOCK_DATA anywhere in the code, I take it that the device implements either I2C block transactions or SMBus block transactions but not both. Which one is it?
Quoted 18 lines
drivers/i2c/busses/i2c-highlander.c new file +18
-0+default : +dev_err (dev->dev,"bogus command %d\n" , size); +return -EINVAL ; + } + +/* +* Encode the mode setting +*/ + tmp =ioread16 (dev->base +SMMR ); + tmp &= ~(SMMR_MODE0 |SMMR_MODE1 ); + +switch (dev->buf_len) { +case 1 : +/* default */ +break ; +case 8 : + tmp |=SMMR_MODE0 ; +break ;
drivers/i2c/busses/i2c-highlander.c line 343
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+case 16 : + tmp |=SMMR_MODE1 ; +break ; +case 32 : + tmp |= (SMMR_MODE0 |SMMR_MODE1 ); +break ; +default : +dev_err (dev->dev,"bogus xfer size %d\n" , dev->buf_len); +return -EINVAL ; + }
Wait, do you mean that this device only supports block sizes of 8, 16 and 32. Oh my. At least please change "bogus" to "unsupported" in the message above - if something is bogus here, that's the hardware.
drivers/i2c/busses/i2c-highlander.c line 351
Quoted 8 lines
drivers/i2c/busses/i2c-highlander.c new file +8
-0+ +iowrite16 (tmp, dev->base +SMMR ); + +/* Ensure we're in a sane state */ +highlander_i2c_done (dev); + +/* Set slave address */ +iowrite16 (((addr &0x7f ) <<1 ) | read, dev->base +SMSMADR );
Masking is not needed, addr is a 7-bit value to start with.
drivers/i2c/busses/i2c-highlander.c line 363
Quoted 12 lines
drivers/i2c/busses/i2c-highlander.c new file +12
-0+ +highlander_i2c_command (dev, command, dev->buf_len); + +if (read) +return highlander_i2c_read (dev, dev->buf, dev->buf_len); +else +return highlander_i2c_write (dev, dev->buf, dev->buf_len); +} + +static u32 highlander_i2c_func (struct i2c_adapter *adapter) +{ +return I2C_FUNC_SMBUS_BYTE_DATA |I2C_FUNC_SMBUS_BLOCK_DATA ;
Doesn't match the cases handled in highlander_i2c_smbus_xfer.
Quoted 15 lines
drivers/i2c/busses/i2c-highlander.c new file +15
-0+} + +static const struct i2c_algorithm highlander_i2c_algo = { + .smbus_xfer = highlander_i2c_smbus_xfer, + .functionality = highlander_i2c_func, +}; + +static int highlander_i2c_probe (struct platform_device *pdev) +{ +struct highlander_i2c_dev *dev; +struct i2c_adapter *adap; +struct resource *res; +int ret; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 );
drivers/i2c/busses/i2c-highlander.c line 388
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+if (unlikely (!res)) { +dev_err (&pdev->dev,"no mem resource\n" ); +return -ENODEV ; + } + + dev =kzalloc (sizeof (struct highlander_i2c_dev ),GFP_KERNEL ); +if (unlikely (!dev)) +return -ENOMEM ; + + dev->base =ioremap_nocache (res->start, res->end - res ->start +1 );
No spaces around ->.
Quoted 18 lines
drivers/i2c/busses/i2c-highlander.c new file +18
-0+if (unlikely (!dev->base)) { + ret = -ENXIO ; + goto err; + } + + dev->dev = &pdev->dev; +platform_set_drvdata (pdev, dev); + + dev->irq =platform_get_irq (pdev,0 ); +if (dev->irq && !iic_force_poll) { + ret =request_irq (dev->irq, highlander_i2c_irq,IRQF_DISABLED , + pdev->name, dev); +if (unlikely (ret)) + goto err_unmap; + +highlander_i2c_irq_enable (dev); + }else { +dev_notice (&pdev->dev,"no IRQ, using polling mode\n" );
drivers/i2c/busses/i2c-highlander.c line 416
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+highlander_i2c_irq_disable (dev); + } + +highlander_i2c_setup (dev); + + adap = &dev->adapter; +i2c_set_adapdata (adap, dev); + adap->owner =THIS_MODULE ; + adap->class =I2C_CLASS_HWMON ; +strncpy (adap->name,"HL FPGA I2C adapter" ,sizeof (adap->name));
strncpy is broken, please use strlcpy instead.
Quoted 7 lines
drivers/i2c/busses/i2c-highlander.c new file +7
-0+ adap->algo = &highlander_i2c_algo; + adap->dev.parent = &pdev->dev; + + adap->nr = pdev->id; + ret =i2c_add_numbered_adapter (adap); +if (unlikely (ret)) { +dev_err (&pdev->dev,"failure adding adapter\n" );
drivers/i2c/busses/i2c-highlander.c line 433
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+ goto err_free_irq; + } + +/* +* Reset the adapter +*/ + ret =highlander_i2c_reset (dev); +if (unlikely (ret)) { +dev_err (&pdev->dev,"controller didn't come up\n" ); + goto err_free_irq;
The error path is broken here, you return with an i2c adapter registered but all its resources freed.
In fact, you should reset the adapter before registering it, otherwise there's a race where someone could use the adapter before it's operational.
Quoted 14 lines
drivers/i2c/busses/i2c-highlander.c new file +14
-0+ } + +return 0 ; + +err_free_irq: +free_irq (dev->irq, dev); +err_unmap: +iounmap (dev->base); +err: +kfree (dev); + +platform_set_drvdata (pdev,NULL ); + +return ret;
drivers/i2c/busses/i2c-highlander.c line 457
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+} + +static int highlander_i2c_remove (struct platform_device *pdev) +{ +struct highlander_i2c_dev *dev =platform_get_drvdata (pdev); + +i2c_del_adapter (&dev->adapter); + +if (dev->irq) +free_irq (dev->irq, dev);
Here you test for dev->irq before calling free_irq(), but in the error path above you didn't?
Quoted 7 lines
drivers/i2c/busses/i2c-highlander.c new file +7
-0+ +iounmap (dev->base); +kfree (dev); + +platform_set_drvdata (pdev,NULL ); + +return 0 ;
drivers/i2c/busses/i2c-highlander.c line 474
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+} + +static struct platform_driver highlander_i2c_driver = { + .driver = { + .name ="i2c-highlander" , + .owner =THIS_MODULE , + }, + + .probe = highlander_i2c_probe, + .remove = highlander_i2c_remove,
I guess that the remove function could be made __devexit or even __exit?
Quoted 16 lines
drivers/i2c/busses/i2c-highlander.c new file +16
-0+}; + +static int __init highlander_i2c_init (void ) +{ +return platform_driver_register (&highlander_i2c_driver); +} + +static void __exit highlander_i2c_exit(void) +{ +platform_driver_unregister (&highlander_i2c_driver); +} + +module_init (highlander_i2c_init); +module_exit (highlander_i2c_exit); + +MODULE_AUTHOR ("Paul Mundt" );
drivers/i2c/busses/i2c-highlander.c line 500
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +10
-0+MODULE_DESCRIPTION ("Renesas Highlander FPGA I2C/SMBus adapter" ); +MODULE_LICENSE ("GPL v2" ); + +module_param (iic_force_poll, bool,0 ); +module_param (iic_force_normal, bool,0 ); +module_param (iic_timeout, int,0 ); + +MODULE_PARM_DESC (iic_force_poll,"Force polling mode" ); +MODULE_PARM_DESC (iic_force_normal,"Force normal mode (100 kHz), default is fast mode (400 kHz)" ); +MODULE_PARM_DESC (iic_timeout,"Force timeout value in msecs (default 1000 ms)" );
This is just setting the timeout value, not "forcing" it.
Signature · 1 line
Jean Delvare
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 6 lines · Jean Delvare
Quoted 6 lines
drivers/i2c/busses/i2c-highlander.c new file +6
-0+static void smbus_write_data (u8 *src,u16 *dst,int len) +{ +int i, j; + +if (len ==1 ) + *dst = *src <<8 ;
Quoted 8 lines
drivers/i2c/busses/i2c-highlander.c line 103
Quoted 8 lines
drivers/i2c/busses/i2c-highlander.c new file +8
-0+else { + j =0 ; +for (i =0 ; i < len; i +=2 ) { + *(dst + j) = *(src + i) <<8 | *(src + i +1 ); + j++; + } + } +}
for (; len > 1; len -= 2) {
*dst++ = be16_to_cpup((u16*)src);
src += 2;
}
if (len)
*dst = *src << 8;Quoted 12 lines
Quoted 12 lines
drivers/i2c/busses/i2c-highlander.c new file +12
-0+static void smbus_read_data (u16 *src,u8 *dst,int len) +{ +int i, j; + +if (len ==1 ) + *dst = *src >>8 ; +else { + j =0 ; +for (i =0 ; i < len; i +=2 ) { + *(dst + i) = *(src + j) >>8 ; + *(dst + i +1 ) = *(src + j) &0x00ff ; + j++;
Quoted 8 lines
drivers/i2c/busses/i2c-highlander.c line 68
Quoted 3 lines
drivers/i2c/busses/i2c-highlander.c new file +3
-0+ } + } +}If I read the code above correctly, you are merely converting 16-bit words from cpu-endian to long-endian and back, so using
be16_to_cpuandcpu_to_be16would perform better. If the Highlander is big-endian, you should be able to let the compiler optimize out most of the code.
for (; len > 1; len -= 2) {
*(u16*)dst = cpu_to_be16p(src++);
dst += 2;
}
if (len)
*dst = *src >> 8;Quoted 9 lines
Quoted 9 lines
drivers/i2c/busses/i2c-highlander.c new file +9
-0+static void highlander_i2c_command (struct highlander_i2c_dev *dev,u8 command,int len) +{ +u16 cmd[32 ]; +int i, j; + + j =0 ; +if (len ==1 ) + cmd[j++] = (command <<8 ); +else
Quoted 8 lines
drivers/i2c/busses/i2c-highlander.c line 137
Quoted 8 lines
drivers/i2c/busses/i2c-highlander.c new file +8
-0+for (i =0 ; i < len; i +=2 ) + cmd[j++] = (command <<8 ) | command; + +for (i =0 ; i < j; i++) { +iowrite16 (cmd[i], dev->base +SMSADR + (i *sizeof (u16))); +dev_dbg (dev->dev,"command data[%x] 0x%04x\n" , i, cmd[i]); + } +}
Is there a reason to have a 32 element array for cmd? Each element is the same.
unsigned int i;
u16 cmd = (command << 8) | command;
for (i = 0; i < len; i += 2) {
if (len - i == 1)
cmd = command << 8;
iowrite16(cmd, dev->base + SMSADR + i);
dev_dbg(dev->dev, "command data[%x] 0x%04x\n", i/2, cmd);
}These versions will work for odd values of len. If the hardware can't handle this, except when len == 1, it would probably make more sense to catch the error sooner and never call them with unsupported values of len.
Quoted 6 lines
drivers/i2c/busses/i2c-highlander.c line 144
Quoted 6 lines
drivers/i2c/busses/i2c-highlander.c new file +6
-0+static int highlander_i2c_wait_for_bbsy (struct highlander_i2c_dev *dev) +{ +unsigned long timeout; + + timeout = jiffies +msecs_to_jiffies (iic_timeout); +while (ioread16 (dev->base +SMCR ) &SMCR_BBSY ) {
If there is some delay here (interrupt or kernel preemption for example), then the timeout could be triggered incorrectly.
Quoted 21 lines
Quoted 8 lines
drivers/i2c/busses/i2c-highlander.c new file +8
-0+if (time_after (jiffies, timeout)) { +dev_warn (dev->dev,"timeout waiting for bus ready\n" ); +return -ETIMEDOUT ; + } + +msleep (1 ); + } +drivers/i2c/busses/i2c-highlander.c line 255
Quoted 10 lines
drivers/i2c/busses/i2c-highlander.c new file +9
-0+return 0 ; +} +/* +* The R0P7780LC0011RL FPGA on the SH7780-based Highlanders +* needs a significant delay in the read path. SH7785 Highlanders +* don't have this issue, so restrict it entirely to those. +*/ +if (mach_is_r7780rp () ||mach_is_r7780mp ()) +mdelay (1000 );A one second busy-wait is nasty. Can't you use msleep here instead?
Quoted 2 lines
Having to wait for 1 second after each read makes this driver pretty unusable on these machines anyway, doesn't it? :(
Does it need to wait 1 sec after each read, or does it really need a 1 sec delay _between_ reads? If it's the later, you would be much better off storing the time of the last read, and delaying one second from this time before starting a new read. If you're only trying to poll a sensor chip every second, you won't need to busy wait at all this way.
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c