login
Search
Hi, Arnd and Yong Y I have released Phub patch file. Please confirm below. Thanks, Ohtake. Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com> --- drivers/char/Kconfig | 12 + drivers/char/Makefile | 2 + drivers/char/pch_phub/Makefile | 7 + drivers/char/pch_phub/pch_phub.c | 937 +++++++++++++++++++++++ drivers/char/pch_phub/pch_phub.h | 58 ++ 5 files changed, 1016 insertions(+), 0 deletions(-) create mode 100644 drivers/char/pch_phub/Makefile create mode 100755 drivers/char/pch_phub/pch_phub.c create mode 100755 drivers/char/pch_phub/pch_phub.h diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig index e023682..7ff728a 100644 --- a/drivers/char/Kconfig +++ b/drivers/char/Kconfig @@ -4,6 +4,18 @@ menu "Character devices" +config PCH_PHUB + tristate "PCH PHUB" + depends on PCI + help + If you say yes to this option, support will be included for the + PCH Packet Hub Host controller. + This driver is for PCH Packet hub driver for Topcliff. + This driver is integrated as built-in. + This driver can access GbE MAC address. + This driver can access HW register. + You can also be integrated as module. + config VT bool "Virtual terminal" if EMBEDDED depends on !S390 diff --git a/drivers/char/Makefile b/drivers/char/Makefile index f957edf..1e3eb6c 100644 --- a/drivers/char/Makefile +++ b/drivers/char/Makefile @@ -111,6 +111,8 @@ obj-$(CONFIG_PS3_FLASH) += ps3flash.o obj-$(CONFIG_JS_RTC) += js-rtc.o js-rtc-y = rtc.o +obj-$(CONFIG_PCH_PHUB) += pch_phub/ + # Files generated that shall be removed upon make clean clean-files := consolemap_deftbl.c defkeymap.c diff --git a/drivers/char/pch_phub/Makefile b/drivers/char/pch_phub/Makefile new file mode 100644 index 0000000..51ce785 --- /dev/null +++ b/drivers/char/pch_phub/Makefile @@ -0,0 +1,7 @@ +ifeq ($(CONFIG_PHUB_DEBUG),y) +EXTRA_CFLAGS += -DDEBUG +endif + +obj-$(CONFIG_PCH_PHUB) += ...
Hi Arnd and Yong Y We have updated phub patch about the following. * Arnd's commnets * Delete PCH_READ_REG/PCH_WRITE_REG * Delete '_t' prefix * Modify basic type * Delete needless 'static' prefix * Modify returned value * Care returned value of get_user() * Add .llseek line * Yong Y's comments * Applying to the latest checkpatch(2.6.35) * Delete unused 'DEBUG' macro in Makefile * Delete IEEE1588 lines * Delete 'PCH_CAN_PCLK_50MHZ' Thanks, Ohtake. Kernel=2.6.33.1 Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com> --- drivers/char/Kconfig | 12 + drivers/char/Makefile | 2 + drivers/char/pch_phub/Makefile | 3 + drivers/char/pch_phub/pch_phub.c | 937 ++++++++++++++++++++++++++++++++++++++ drivers/char/pch_phub/pch_phub.h | 58 +++ 5 files changed, 1012 insertions(+), 0 deletions(-) create mode 100644 drivers/char/pch_phub/Makefile create mode 100755 drivers/char/pch_phub/pch_phub.c create mode 100755 drivers/char/pch_phub/pch_phub.h diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig index e023682..7ff728a 100644 --- a/drivers/char/Kconfig +++ b/drivers/char/Kconfig @@ -4,6 +4,18 @@ menu "Character devices" +config PCH_PHUB + tristate "PCH PHUB" + depends on PCI + help + If you say yes to this option, support will be included for the + PCH Packet Hub Host controller. + This driver is for PCH Packet hub driver for Topcliff. + This driver is integrated as built-in. + This driver can access GbE MAC address. + This driver can access HW register. + You can also be integrated as module. + config VT bool "Virtual terminal" if EMBEDDED depends on !S390 diff --git a/drivers/char/Makefile b/drivers/char/Makefile index f957edf..1e3eb6c 100644 --- a/drivers/char/Makefile +++ b/drivers/char/Makefile @@ -111,6 +111,8 @@ obj-$(CONFIG_PS3_FLASH) += ps3flash.o obj-$(CONFIG_JS_RTC) += js-rtc.o js-rtc-y = rtc.o +obj-$(CONFIG_PCH_PHUB) += pch_phub/ + # ...
On Tue, 22 Jun 2010 19:33:34 +0900 Please prepare a proper changelog for the patch - one which is suitabe for inclusion in the kernel's permanent git record. Include that changelog with each version of the patch, perhaps after alterations. Please describe the module parameters in that changelog. Please describe the major/minor number handling within the changelog - permitting the major number to be specified on the modprobe command line is unusual and should be fully described and justified, please. Please ensure that the changelog tells us what the driver actually kerneldoc comments are usually formatted as /** * foo() - do something * @arg1 ... I don't know whether the layout whcih this driver uses will be properly handled by the kerneldoc tools, but I'd suggest that it all be The driver has quite a lot of comments which use the kerneldoc toke "/**" but which don't really look like they wre intended to be kerneldoc comments. So I'd suggest converting these to plain old comments: /* * pch_phub_save_reg_conf - saves register configuration */ or /* pch_phub_save_reg_conf - saves register configuration */ or finish off the kerneldoc work by documenting the arguments (and the Strange layout. I don't know what this will look like after kerneldoc processing - it might make a mess. Conventional layout is /** * pch_phub_write_serial_rom - Implements the functionality of writing Serial ROM. or /** * pch_phub_write_serial_rom - Implements the functionality of writing Serial * ROM. The driver tends to do int foo = 0; ... foo = ... in quite a lot of places. The initialisation to zero is harmless - the compiler will take it out again. But it's unusual and jsut adds noise. The compiler will warn about use of uninitialised variables anyway. In fact this unnecessary initalisation will suppress compiler warnings which might have revealed real bugs. I'd suggest that they all be I think vfs_read() -> ...
Hi Andrew Thank you for your comments. We will resubmit modified our patch by tomorrow or day after tomorrow at the latest. Thanks. Ohtake ----- Original Message ----- From: "Andrew Morton" <akpm@linux-foundation.org> To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com> Cc: "Arnd Bergmann" <arnd@arndb.de>; "Wang, Yong Y" <yong.y.wang@intel.com>; <qi.wang@intel.com>; <joel.clark@intel.com>; <andrew.chih.howe.khor@intel.com>; "Alan Cox" <alan@lxorguk.ukuu.org.uk>; "LKML" <linux-kernel@vger.kernel.org> Sent: Wednesday, June 23, 2010 7:12 AM --
You have addressed all of my comments, the driver looks ready Acked-by: Arnd Bergmann <arnd@arndb.de> --
Hi Andrew, Are you going to take this patch? Or could you please let us know which maintainer will do? Thanks -Yong --
"Topcliff" is probably some kind of internal codename; please use a name that will be visible to customers of this product. References to what kind of device (IEEE standards it implements, what systems it might be I think you mean something more like "This driver allows access to the GbE MAC address and HW registers of the PCH Packet Hub." If this is a driver for an Ethernet MAC, what is it doing in We don't normally merge comment markup languages other than kernel-doc. Please read Documentation/kernel-doc-nano-HOWTO.txt and follow it. (Or, provide a pointer to documentation and tools for this mysterious markup I would format this as: #define PHUB_STATUS 0x00 /* Status Register offset */ #define PHUB_CONTROL 0x04 /* Control Register offset */ Since these are used to hold HW-defined data, you should use fixed-size types such as u32. Also, you should consider whether they should be If this is to remain a chardev, use register_chrdev(). You shouldn't That \ is superfluous. There are several of these in this file. The indentation on the second line is too large, and the fact that "a = b + c" spills onto a second line is a clue that your struct names The return is unneeded if this remains a void function. (many more Don't include comments that just duplicate the code. Also, rename your This comment is almost correctly formatted, but there are extra words and some whitespace problems, and it doesn't document what actually happens. +/** + * pch_phub_read_serial_rom - read PHUB Serial ROM. + * @offset_address: serial ROM offset to read. + * @data: pointer at which to store value that was read. + */ is more correct. The second line is indented too far. We use 8-space tabstops, so the "u" of unsigned is all the way over under the "t" of offset_address. It should either be two tabstops indented, or lined up with the previous argument. (This applies to several functions below too.) If this can't ...
Hi Andy Thank you for your comments. Please refer to the following inline. Thanks, Ohtake. ----- Original Message ----- From: "Andy Isaacson" <adi@hexapodia.org> To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com> Cc: "Arnd Bergmann" <arnd@arndb.de>; "Wang, Yong Y" <yong.y.wang@intel.com>; "Wang Qi" <qi.wang@intel.com>; "Intel OTC" <joel.clark@intel.com>; "Andrew" <andrew.chih.howe.khor@intel.com>; "Alan Cox" <alan@lxorguk.ukuu.org.uk>; "LKML" <linux-kernel@vger.kernel.org> Sent: Wednesday, June 30, 2010 8:31 AM Sorry, I can't understand your intention. This function is called from ioctl context(not interrput). Since SROM is not in GbE HW but Phub HW, Phub is not part of Ethernet driver. --
My mistake, I merged two comments into one paragraph, let me clarify. 1. When writing comments, do not write comments that duplicate the code. Instead of writing /* store PHUB_ID */ iowrite32(..._PHUB_ID_REG); /* store PHUB_FOO */ iowrite32(..._PHUB_FOO_REG); you should delete the line-by-line comments and just write iowrite32(..._PHUB_ID_REG); iowrite32(..._PHUB_FOO_REG); 2. your register names are very long. Since the #define names are private to this driver, there's no need to make them extremely descriptive. Instead of naming your registers PCH_PHUB_PHUB_ID_REG, you should change the names to be shorter, like PHUB_ID_REG or PCH_ID_REG. This will make your source code much more It sounds like PHUB is a system-level device which provides access to a SROM which contains GbE configuration data. If that is correct, then I have two comments: 1. There are many other systems with similar configurations -- MIPS SiByte, Alpha SRM, SPARC OpenFirmware, and some ARM systems, just to name a few. None of them expose the SROM as a custom /dev node AFAIK. Is there a shared infrastructure that you can implement? 2. How does your GbE driver get the MAC address from the SPROM? If there is an in-kernel user of the PHUB interface, it might be much easier to understand the design. Thanks for responding to my review so quickly! -andy --
This was our mistake. I have modified PCH_PHUB_PHUB_ID_REG to PCH_PHUB_ID_REG. Sorry, I can't understand your intension. PHUB HW transfers MAC address(in SROM) data to GbE register to set MAC address when boot processing. Thanks, Ohtake ----- Original Message ----- From: "Andy Isaacson" <adi@hexapodia.org> To: "Masayuki Ohtake" <masa-korg@dsn.okisemi.com> Cc: "Arnd Bergmann" <arnd@arndb.de>; "Wang, Yong Y" <yong.y.wang@intel.com>; "Wang Qi" <qi.wang@intel.com>; "Intel OTC" <joel.clark@intel.com>; "Andrew" <andrew.chih.howe.khor@intel.com>; "Alan Cox" <alan@lxorguk.ukuu.org.uk>; "LKML" <linux-kernel@vger.kernel.org> Sent: Thursday, July 01, 2010 3:28 AM --
Hi Andy and Andrew I have modified for your comments. Please confirm below. --- Packet hub driver of Topcliff PCH Topcliff PCH is the platform controller hub that is going to be used in Intel’s upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is a special converter device in Topcliff PCH that translate AMBA transactions to PCI Express transactions and vice versa. Thus packet hub helps present all IO peripherals in Topcliff PCH as PCIE devices to IA system. Topcliff PCH have MAC address and Option ROM data. These data are in SROM which is connected to PCIE bus. Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in SROM. Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com> Acked-by: Arnd <Arnd’s email address> --- drivers/char/Kconfig | 10 + drivers/char/Makefile | 2 + drivers/char/pch_phub/Makefile | 3 + drivers/char/pch_phub/pch_phub.c | 805 ++++++++++++++++++++++++++++++++++++++ drivers/char/pch_phub/pch_phub.h | 48 +++ 5 files changed, 868 insertions(+), 0 deletions(-) create mode 100644 drivers/char/pch_phub/Makefile create mode 100755 drivers/char/pch_phub/pch_phub.c create mode 100755 drivers/char/pch_phub/pch_phub.h diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig index e023682..1851e97 100644 --- a/drivers/char/Kconfig +++ b/drivers/char/Kconfig @@ -4,6 +4,16 @@ menu "Character devices" +config PCH_PHUB + tristate "PCH PHUB" + depends on PCI + help + Topcliff is a IOH for x86 embedded processor. This IOH is quite + different with the traditional IOH. Topcliff is a kind of ARM-based + processor and connected with PCIe bus (from x86 processor). + PHUB work as a gateway transform the PCIe transaction into the AMBA + transaction, and vise verse, and have several transform windows also. + config VT bool "Virtual terminal" if EMBEDDED depends on !S390 diff --git ...
vice versa, and has several Maybe lose some acromyns? Overall, this help text should be about *what* PCH_PHUB is, not *how* it accomplishes its work. If you want to document *how* it works, put that into the source code or Documentation/ somewhere. --- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** --
I will rewrite my help text. Thanks, Ohtake ----- Original Message ----- From: "Randy Dunlap" <randy.dunlap@oracle.com> To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com> Cc: "Andy Isaacson" <adi@hexapodia.org>; "Andrew Morton" <akpm@linux-foundation.org>; "Arnd Bergmann" <arnd@arndb.de>; "Wang, Yong Y" <yong.y.wang@intel.com>; <qi.wang@intel.com>; <joel.clark@intel.com>; <andrew.chih.howe.khor@intel.com>; "Alan Cox" <alan@lxorguk.ukuu.org.uk>; "LKML" <linux-kernel@vger.kernel.org> Sent: Thursday, July 01, 2010 3:05 AM --
Hi Andy and Randy I have modified for your comments. Please confirm below. Thanks, Ohtake. --- Packet hub driver of Topcliff PCH Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is a special converter device in Topcliff PCH that translate AMBA transactions to PCI Express transactions and vice versa. Thus packet hub helps present all IO peripherals in Topcliff PCH as PCIE devices to IA system. Topcliff PCH have MAC address and Option ROM data. These data are in SROM which is connected to PCIE bus. Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in SROM. Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com> Acked-by: Arnd Bergmann <arnd@arndb.de> --- drivers/char/Kconfig | 9 + drivers/char/Makefile | 2 + drivers/char/pch_phub/Makefile | 3 + drivers/char/pch_phub/pch_phub.c | 805 ++++++++++++++++++++++++++++++++++++++ drivers/char/pch_phub/pch_phub.h | 48 +++ 5 files changed, 867 insertions(+), 0 deletions(-) create mode 100644 drivers/char/pch_phub/Makefile create mode 100755 drivers/char/pch_phub/pch_phub.c create mode 100755 drivers/char/pch_phub/pch_phub.h diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig index e023682..9ef3c85 100644 --- a/drivers/char/Kconfig +++ b/drivers/char/Kconfig @@ -4,6 +4,15 @@ menu "Character devices" +config PCH_PHUB + tristate "PCH PHUB" + depends on PCI + help + This driver is for PCH PHUB of Topcliff which is an IOH for x86 + embedded processor. The Topcliff has MAC address and Option ROM data + in SROM. This dirver can access MAC address and Option ROM data in + SROM. + config VT bool "Virtual terminal" if EMBEDDED depends on !S390 diff --git a/drivers/char/Makefile b/drivers/char/Makefile index f957edf..1e3eb6c 100644 --- a/drivers/char/Makefile +++ ...
Your style is looking better, and the additional documentation is much appreciated. I still have concerns about the userland interface design. It still seems to me that the MAC interface should be used by something in drivers/net and there's no reason to expose the SROM in /dev unless it contains more than just the MAC address. A few more style issues I saw on a quick scan through -- this was not a Reformat this to fit on one line: + ret_value = copy_to_user(varg, mac_addr, sizeof(mac_addr)); Here we need to break: + break; + } ... we would pass garbage to pch_phub_write_gbe_mac_addr. Prefix \n is not correct. This should be + dev_dbg(&pdev->dev, "pch_phub_probe: pci_enable_device FAILED"); In general dev_dbg format strings should fit on one 80-char line. If your format strings are longer than that, it's a clue you're doing + dev_dbg(&pdev->dev, "pch_phub_probe: pci_enable_device returns %d\n", If you have a dev_dbg, please print ret. It may give an important clue. [snip a bunch more I don't have time to review right now] Thanks, -andy --
SROM has not only MAC address but also Option ROM data. We are now modifying. Thanks, Ohtake ----- Original Message ----- From: "Andy Isaacson" <adi@hexapodia.org> To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com> Cc: "Randy Dunlap" <randy.dunlap@oracle.com>; "Arnd Bergmann" <arnd@arndb.de>; "Wang, Yong Y" <yong.y.wang@intel.com>; <qi.wang@intel.com>; <joel.clark@intel.com>; <andrew.chih.howe.khor@intel.com>; "Alan Cox" <alan@lxorguk.ukuu.org.uk>; "LKML" <linux-kernel@vger.kernel.org> Sent: Thursday, July 01, 2010 3:58 PM --
Hi Andy I have modified for your comments. Please confirm below. Thanks, Ohtake. --- Packet hub driver of Topcliff PCH Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is a special converter device in Topcliff PCH that translate AMBA transactions to PCI Express transactions and vice versa. Thus packet hub helps present all IO peripherals in Topcliff PCH as PCIE devices to IA system. Topcliff PCH have MAC address and Option ROM data. These data are in SROM which is connected to PCIE bus. Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in SROM. Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com> Acked-by: Arnd Bergmann <arnd@arndb.de> --- drivers/char/Kconfig | 9 + drivers/char/Makefile | 2 + drivers/char/pch_phub/Makefile | 3 + drivers/char/pch_phub/pch_phub.c | 811 ++++++++++++++++++++++++++++++++++++++ drivers/char/pch_phub/pch_phub.h | 48 +++ 5 files changed, 873 insertions(+), 0 deletions(-) create mode 100644 drivers/char/pch_phub/Makefile create mode 100755 drivers/char/pch_phub/pch_phub.c create mode 100755 drivers/char/pch_phub/pch_phub.h diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig index e023682..9ef3c85 100644 --- a/drivers/char/Kconfig +++ b/drivers/char/Kconfig @@ -4,6 +4,15 @@ menu "Character devices" +config PCH_PHUB + tristate "PCH PHUB" + depends on PCI + help + This driver is for PCH PHUB of Topcliff which is an IOH for x86 + embedded processor. The Topcliff has MAC address and Option ROM data + in SROM. This dirver can access MAC address and Option ROM data in + SROM. + config VT bool "Virtual terminal" if EMBEDDED depends on !S390 diff --git a/drivers/char/Makefile b/drivers/char/Makefile index f957edf..1e3eb6c 100644 --- a/drivers/char/Makefile +++ ...
s/6/ETH_ALEN/ s/6/ETH_ALEN/ #include <linux/if_ether.h> All of these dev_dbg() calls should use __func__ to get the function name: dev_dbg(&pdev->dev, "%s returns: %d\n", __func__, 0); (same for other functions) -- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** --
Hi Randy I have modified for your comments. Please confirm below. Thanks, Ohtake. --- Packet hub driver of Topcliff PCH Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is a special converter device in Topcliff PCH that translate AMBA transactions to PCI Express transactions and vice versa. Thus packet hub helps present all IO peripherals in Topcliff PCH as PCIE devices to IA system. Topcliff PCH has MAC address and Option ROM data. These data are in SROM which is connected to PCIE bus. Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in SROM. Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com> Acked-by: Arnd Bergmann <arnd@arndb.de> --- Documentation/ioctl/ioctl-number.txt | 1 + drivers/char/Kconfig | 9 + drivers/char/Makefile | 2 + drivers/char/pch_phub/Makefile | 3 + drivers/char/pch_phub/pch_phub.c | 803 ++++++++++++++++++++++++++++++++++ drivers/char/pch_phub/pch_phub.h | 48 ++ 6 files changed, 866 insertions(+), 0 deletions(-) create mode 100644 drivers/char/pch_phub/Makefile create mode 100755 drivers/char/pch_phub/pch_phub.c create mode 100755 drivers/char/pch_phub/pch_phub.h diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt index 35cf64d..b700b17 100644 --- a/Documentation/ioctl/ioctl-number.txt +++ b/Documentation/ioctl/ioctl-number.txt @@ -320,4 +320,5 @@ Code Seq#(hex) Include File Comments <mailto:thomas@winischhofer.net> 0xF4 00-1F video/mbxfb.h mbxfb <mailto:raph@8d.com> +0xF7 00-0F drivers/char/pch_phub/pch_phub.h PCH Phub driver 0xFD all linux/dm-ioctl.h diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig index e023682..499902f 100644 --- a/drivers/char/Kconfig +++ b/drivers/char/Kconfig @@ -4,6 +4,15 @@ menu ...
I took another look and found some more things that should be improved.
Overall, the quality of this driver has improved enourmously, and I'm
optimistic that it will be a lot easier for you in your next device
driver with all the details you have learned about the coding style.
The variable you define here is in the global namespace, which it
should not be in. I'd suggest making it static and splitting the
type defintion from the variable definition to make that more obvious,
like:
struct pch_phub_reg {
...
};
Having three mutexes here means that you have effectively no
serialization between the functions at all. The mutex only
has an effect if you use the same one in all three functions!
Arnd
--
-- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** --
Hi Arnd I have modified for your comments. Please confirm below. Thanks, Ohtake. --- Packet hub driver of Topcliff PCH Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is a special converter device in Topcliff PCH that translate AMBA transactions to PCI Express transactions and vice versa. Thus packet hub helps present all IO peripherals in Topcliff PCH as PCIE devices to IA system. Topcliff PCH has MAC address and Option ROM data. These data are in SROM which is connected to PCIE bus. Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in SROM. Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com> Acked-by: Arnd Bergmann <arnd@arndb.de> --- Documentation/ioctl/ioctl-number.txt | 1 + drivers/char/Kconfig | 9 + drivers/char/Makefile | 2 + drivers/char/pch_phub/Makefile | 3 + drivers/char/pch_phub/pch_phub.c | 802 ++++++++++++++++++++++++++++++++++ drivers/char/pch_phub/pch_phub.h | 48 ++ 6 files changed, 865 insertions(+), 0 deletions(-) create mode 100644 drivers/char/pch_phub/Makefile create mode 100755 drivers/char/pch_phub/pch_phub.c create mode 100755 drivers/char/pch_phub/pch_phub.h diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt index 35cf64d..b700b17 100644 --- a/Documentation/ioctl/ioctl-number.txt +++ b/Documentation/ioctl/ioctl-number.txt @@ -320,4 +320,5 @@ Code Seq#(hex) Include File Comments <mailto:thomas@winischhofer.net> 0xF4 00-1F video/mbxfb.h mbxfb <mailto:raph@8d.com> +0xF7 00-0F drivers/char/pch_phub/pch_phub.h PCH Phub driver 0xFD all linux/dm-ioctl.h diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig index e023682..499902f 100644 --- a/drivers/char/Kconfig +++ b/drivers/char/Kconfig @@ -4,6 +4,15 @@ menu "Character ...
Yes, looks good. Thanks for the quick reply. Arnd --
Hi Andrew, Do you have any further comments on this patch? If so, please let us know. Thanks -Yong --
On Tue, 06 Jul 2010 15:20:52 +0900 That didn't describe the most important part of the driver: the userspace interface. We should add here something along the lines of The driver creates a character device /dev/pch_phub. That device file supports the following operations: read(): <document the read operation - seems to read a serial ROM?> write():<document the write operation - seems to write a serial ROM?> I suspect this function will do strange things if passed an initial *ppos which is outside the range of the ROM. It looks like it will write --
I understand OROM upper size check is not enough.
If the my understanding true, We will modify like below.
Can you accept the following our modification ?
+static ssize_t pch_phub_write(struct file *file, const char __user *buf,
+ size_t size, loff_t *ppos)
+{
+ unsigned int data;
+ int ret_value1;
+ int ret_value2;
+ int err;
+ unsigned int addr_offset;
+ loff_t pos = *ppos;
+ int ret;
+
+ ret = mutex_lock_interruptible(&pch_phub_mutex);
+ if (ret) {
+ err = -ERESTARTSYS;
+ goto return_err_nomutex;
+ }
+
+ for (addr_offset = 0; addr_offset < size; addr_offset++) {
+ if (PCH_PHUB_OROM_SIZE < pos + addr_offset) {
+ *ppos += addr_offset;
+ goto return_ok;
+ }
+ ret_value1 = get_user(data, &buf[addr_offset]);
+ if (ret_value1) {
+ err = -EFAULT;
+ goto return_err;
+ }
+
+ ret_value2 = pch_phub_write_serial_rom(0x80 + addr_offset + pos,
+ data);
+ if (ret_value2) {
+ err = ret_value2;
+ goto return_err;
+ }
+
Thanks, Ohtake
----- Original Message -----
From: "Andrew Morton" <akpm@linux-foundation.org>
To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com>
Cc: "Arnd Bergmann" <arnd@arndb.de>; "Wang, Yong Y" <yong.y.wang@intel.com>; <qi.wang@intel.com>;
<joel.clark@intel.com>; <andrew.chih.howe.khor@intel.com>; "Alan Cox" <alan@lxorguk.ukuu.org.uk>; "LKML"
<linux-kernel@vger.kernel.org>
Sent: Saturday, July 10, 2010 5:00 AM
--
Hi Andrew Morton We have modified our packet hub driver for your indication. Please confirm below. Thanks, Ohtake. --- Packet hub driver of Topcliff PCH Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is a special converter device in Topcliff PCH that translate AMBA transactions to PCI Express transactions and vice versa. Thus packet hub helps present all IO peripherals in Topcliff PCH as PCIE devices to IA system. Topcliff PCH has MAC address and Option ROM data. These data are in SROM which is connected to PCIE bus. Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in SROM. The driver creates a character device /dev/pch_phub. That device file supports the following operations: read() :Read Option ROM data of SROM write():Write Option ROM data of SROM ioctl():Read/Write MAC address of SROM Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com> Acked-by: Arnd Bergmann <arnd@arndb.de> --- Documentation/ioctl/ioctl-number.txt | 1 + drivers/char/Kconfig | 9 + drivers/char/Makefile | 2 + drivers/char/pch_phub/Makefile | 3 + drivers/char/pch_phub/pch_phub.c | 800 ++++++++++++++++++++++++++++++++++ drivers/char/pch_phub/pch_phub.h | 48 ++ 6 files changed, 863 insertions(+), 0 deletions(-) create mode 100644 drivers/char/pch_phub/Makefile create mode 100755 drivers/char/pch_phub/pch_phub.c create mode 100755 drivers/char/pch_phub/pch_phub.h diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt index 35cf64d..b700b17 100644 --- a/Documentation/ioctl/ioctl-number.txt +++ b/Documentation/ioctl/ioctl-number.txt @@ -320,4 +320,5 @@ Code Seq#(hex) Include File Comments <mailto:thomas@winischhofer.net> 0xF4 00-1F video/mbxfb.h mbxfb ...
I2C driver of Topcliff PCH Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Topcliff PCH has I2C I/F. Using this I/F, it is able to access system devices connected to I2C. Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com> --- drivers/i2c/busses/Kconfig | 8 + drivers/i2c/busses/Makefile | 3 + drivers/i2c/busses/i2c-pch.c | 1390 ++++++++++++++++++++++++++++++++++++++++++ drivers/i2c/busses/i2c-pch.h | 147 +++++ drivers/i2c/i2c-dev.c | 28 + 5 files changed, 1576 insertions(+), 0 deletions(-) create mode 100644 drivers/i2c/busses/i2c-pch.c create mode 100644 drivers/i2c/busses/i2c-pch.h diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index bceafbf..578fd42 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -7,6 +7,14 @@ menu "I2C Hardware Bus support" comment "PC SMBus host controller drivers" depends on PCI +config PCH_I2C + tristate "PCH I2C" + depends on PCI + help + This driver is for PCH I2C of Topcliff which is an IOH for x86 + embedded processor. + This driver can access PCH I2C bus device. + config I2C_ALI1535 tristate "ALI 1535" depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 936880b..53be4b3 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -78,3 +78,6 @@ obj-$(CONFIG_SCx200_I2C) += scx200_i2c.o ifeq ($(CONFIG_I2C_DEBUG_BUS),y) EXTRA_CFLAGS += -DDEBUG endif + +obj-$(CONFIG_PCH_I2C) += pch_i2c.o +pch_i2c-objs := i2c-pch.o diff --git a/drivers/i2c/busses/i2c-pch.c b/drivers/i2c/busses/i2c-pch.c new file mode 100644 index 0000000..58824cc --- /dev/null +++ b/drivers/i2c/busses/i2c-pch.c @@ -0,0 +1,1390 @@ +/* + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD. + * + * This program is free software; you can redistribute it and/or ...
If you want to wait for a maximum amount of time, a loop with
msleep(1) may end up waiting more than twice as long as you want,
because each msleep typically returns one milisecond too late.
Better use something like:
time_t timeout = ktime_add_ns(ktime_get(), MAX_NANOSECONDS);
do {
if (ioread32(p + PCH_I2CSR) & I2CMBB_BIT) == 0)
break;
msleep(1);
The control flow is much more complex than it needs to be here.
If you want to handle different kinds of error conditions, best
use goto. Also, it's very unusual to return positive values
on errors and to print dev_err messages on success.
int ret;
ret = wait_event_interruptible_timeout(...);
if (ret < 0)
goto out;
if (ret == 0) {
ret = -ETIMEOUT;
goto out;
}
ret = 0;
if (adap->pch_event_flag & I2C_ERROR_MASK) {
ret = -EIO;
dev_err(adap->pch_adapter.dev.parent, "error bits set: %lx\n", adap->pch_event_flag);
}
out:
Here you are multiplexing the interrupt yourself. If you don't
always use all the available channels, it may be better to
register the pch_cbr handler separately for each of the channels
that are actually used, so you don't have to invoke the callback
Similarly, you would create a new channel data structure for each channel here
and register it separately, and then request the interrupt with that
A read function should not do copy_from_user, only copy_to_user.
If you are worried about returning invalid data from kernel space,
better use kzalloc instead of kmalloc to get the buffer.
Arnd
--
Hi Arnd, Thank you for you comments. I will modify our i2c patch by tomorrow. Thanks, Ohtake ----- Original Message ----- From: "Arnd Bergmann" <arnd@arndb.de> To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com> Cc: "Jean Delvare (PC drivers, core)" <khali@linux-fr.org>; "Ben Dooks (embedded platforms)" <ben-linux@fluff.org>; <linux-i2c@vger.kernel.org>; "LKML" <linux-kernel@vger.kernel.org>; <qi.wang@intel.com>; "Wang, Yong Y" <yong.y.wang@intel.com>; <joel.clark@intel.com>; <andrew.chih.howe.khor@intel.com> Sent: Friday, July 16, 2010 4:35 AM --
With I2c multi-cahnnel IOH, IRQ number is in common. Thus, I think our PCH I2C driver can't be implemented like above. Thanks, Ohtake. ----- Original Message ----- From: "Arnd Bergmann" <arnd@arndb.de> To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com> Cc: "Jean Delvare (PC drivers, core)" <khali@linux-fr.org>; "Ben Dooks (embedded platforms)" <ben-linux@fluff.org>; <linux-i2c@vger.kernel.org>; "LKML" <linux-kernel@vger.kernel.org>; <qi.wang@intel.com>; "Wang, Yong Y" <yong.y.wang@intel.com>; <joel.clark@intel.com>; <andrew.chih.howe.khor@intel.com> Sent: Friday, July 16, 2010 4:35 AM --
If you pass IRQF_SHARED, you can register any number of handlers for the same IRQ number using different dev pointers. Arnd --
I will modify like above. Thanks, Ohtake. ----- Original Message ----- From: "Arnd Bergmann" <arnd@arndb.de> To: "Masayuki Ohtake" <masa-korg@dsn.okisemi.com> Cc: "Jean Delvare (PC drivers, core)" <khali@linux-fr.org>; "Ben Dooks (embedded platforms)" <ben-linux@fluff.org>; <linux-i2c@vger.kernel.org>; "LKML" <linux-kernel@vger.kernel.org>; <qi.wang@intel.com>; "Wang, Yong Y" <yong.y.wang@intel.com>; <joel.clark@intel.com>; <andrew.chih.howe.khor@intel.com> Sent: Tuesday, July 20, 2010 6:27 PM --
Our I2C HW has special mode. To control the mode, our i2c driver has copy_from_user. Thanks, Ohtake. ----- Original Message ----- From: "Arnd Bergmann" <arnd@arndb.de> To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com> Cc: "Jean Delvare (PC drivers, core)" <khali@linux-fr.org>; "Ben Dooks (embedded platforms)" <ben-linux@fluff.org>; <linux-i2c@vger.kernel.org>; "LKML" <linux-kernel@vger.kernel.org>; <qi.wang@intel.com>; "Wang, Yong Y" <yong.y.wang@intel.com>; <joel.clark@intel.com>; <andrew.chih.howe.khor@intel.com> Sent: Friday, July 16, 2010 4:35 AM --
That is a highly unusual interface and I would definitely not recommend doing this. Please use an ioctl operation for anything that has both input and output data. Arnd --
I will delete the special interface from our i2c driver. Thanks, Ohtake ----- Original Message ----- From: "Arnd Bergmann" <arnd@arndb.de> To: "Masayuki Ohtake" <masa-korg@dsn.okisemi.com> Cc: <andrew.chih.howe.khor@intel.com>; <joel.clark@intel.com>; "Wang, Yong Y" <yong.y.wang@intel.com>; <qi.wang@intel.com>; "LKML" <linux-kernel@vger.kernel.org>; <linux-i2c@vger.kernel.org>; "Ben Dooks (embedded platforms)" <ben-linux@fluff.org>; "Jean Delvare (PC drivers, core)" <khali@linux-fr.org> Sent: Tuesday, July 20, 2010 6:29 PM --
Hi Arnd, I have modified for your comments. Please confirm below. Thanks, Ohtake --- I2C driver of Topcliff PCH Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Topcliff PCH has I2C I/F. Using this I/F, it is able to access system devices connected to I2C. Signed-off-by: Masayuki Ohtake <masa-korg@dsn.okisemi.com> --- drivers/i2c/busses/Kconfig | 8 + drivers/i2c/busses/Makefile | 3 + drivers/i2c/busses/i2c-pch.c | 910 ++++++++++++++++++++++++++++++++++++++++++ drivers/i2c/busses/i2c-pch.h | 147 +++++++ drivers/i2c/i2c-dev.c | 21 + 5 files changed, 1089 insertions(+), 0 deletions(-) create mode 100644 drivers/i2c/busses/i2c-pch.c create mode 100644 drivers/i2c/busses/i2c-pch.h diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 5f318ce..98e7201 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -7,6 +7,14 @@ menu "I2C Hardware Bus support" comment "PC SMBus host controller drivers" depends on PCI +config PCH_I2C + tristate "PCH I2C" + depends on PCI + help + This driver is for PCH I2C of Topcliff which is an IOH for x86 + embedded processor. + This driver can access PCH I2C bus device. + config I2C_ALI1535 tristate "ALI 1535" depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 302c551..3e6b8d6 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -75,3 +75,6 @@ obj-$(CONFIG_SCx200_I2C) += scx200_i2c.o ifeq ($(CONFIG_I2C_DEBUG_BUS),y) EXTRA_CFLAGS += -DDEBUG endif + +obj-$(CONFIG_PCH_I2C) += pch_i2c.o +pch_i2c-objs := i2c-pch.o diff --git a/drivers/i2c/busses/i2c-pch.c b/drivers/i2c/busses/i2c-pch.c new file mode 100644 index 0000000..7939781 --- /dev/null +++ b/drivers/i2c/busses/i2c-pch.c @@ -0,0 +1,910 @@ +/* + * Copyright (C) 2010 OKI ...