Hi Greg,
We have modified our phub driver with sysfs I/F.
Please check below.
Best Regards, Ohtake(OKISEMI).
---
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 via sysfs I/F.
The driver creates a character device /dev/pch_phub. That device file
supports the following operations:
Signed-off-by: Masayuki Ohtake <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
---
drivers/misc/Kconfig | 9 +
drivers/misc/Makefile | 1 +
drivers/misc/pch_phub.c | 722 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 732 insertions(+), 0 deletions(-)
create mode 100755 drivers/misc/pch_phub.c
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 26386a9..c998521 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -353,6 +353,15 @@ config VMWARE_BALLOON
To compile this driver as a module, choose M here: the
module will be called vmware_balloon.
+config PCH_PHUB
+ tristate "PCH Packet Hub of Intel Topcliff"
+ 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 driver can access MAC address and Option ROM data in
+ SROM.
+
source "drivers/misc/c2port/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 6ed06a1..6389eb3 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -31,3 +31,4 @@ obj-$(CONFIG_IWMC3200TOP) += iwmc3200top/
obj-y += eeprom/
obj-y += cb710/
obj-$(CONFIG_VMWARE_BALLOON) += vmware_balloon.o
+obj-$(CONFIG_PCH_PHUB) += pch_phub.o
diff --git a/drivers/misc/pch_phub.c b/drivers/misc/pch_phub.c
new file mode 100755
index 0000000..d2e0409
--- /dev/null
+++ b/drivers/misc/pch_phub.c
@@ -0,0 +1,722 @@
+/*
+ * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include <linux/string.h>
+#include <linux/pci.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include <linux/if_ether.h>
+#include <linux/ctype.h>
+
+#define PHUB_STATUS 0x00 /* Status Register offset */
+#define PHUB_CONTROL 0x04 /* Control Register offset */
+#define PHUB_TIMEOUT 0x05 /* Time out value for Status Register */
+#define PCH_PHUB_ROM_WRITE_ENABLE 0x01 /* Enabling for writing ROM */
+#define PCH_PHUB_ROM_WRITE_DISABLE 0x00 /* Disabling for writing ROM */
+#define PCH_PHUB_ROM_START_ADDR 0x14 /* ROM data area start address offset */
+
+/* MAX number of INT_REDUCE_CONTROL registers */
+#define MAX_NUM_INT_REDUCE_CONTROL_REG 128
+#define PCI_DEVICE_ID_PCH1_PHUB 0x8801
+#define PCH_MINOR_NOS 1
+#define CLKCFG_CAN_50MHZ 0x12000000
+#define CLKCFG_CANCLK_MASK 0xFF000000
+#define MODULE_NAME "pch_phub"
+
+#define PHUB_IOCTL_MAGIC 0xf7
+
+/* Read GbE MAC address */
+#define IOCTL_PHUB_READ_MAC_ADDR (_IOR(PHUB_IOCTL_MAGIC, 1, __u8[ETH_ALEN]))
+
+/* Write GbE MAC address */
+#define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 2, __u8[ETH_ALEN]))
+
+/* SROM ACCESS Macro */
+#define PCH_WORD_ADDR_MASK (~((1 << 2) - 1))
+
+/* Registers address offset */
+#define PCH_PHUB_ID_REG 0x0000
+#define PCH_PHUB_QUEUE_PRI_VAL_REG 0x0004
+#define PCH_PHUB_RC_QUEUE_MAXSIZE_REG 0x0008
+#define PCH_PHUB_BRI_QUEUE_MAXSIZE_REG 0x000C
+#define PCH_PHUB_COMP_RESP_TIMEOUT_REG 0x0010
+#define PCH_PHUB_BUS_SLAVE_CONTROL_REG 0x0014
+#define PCH_PHUB_DEADLOCK_AVOID_TYPE_REG 0x0018
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG0 0x0020
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG1 0x0024
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG2 0x0028
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG3 0x002C
+#define PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE 0x0040
+#define CLKCFG_REG_OFFSET 0x500
+
+#define PCH_PHUB_OROM_SIZE 15360
+
+/**
+ * struct pch_phub_reg - PHUB register structure
+ * @phub_id_reg: PHUB_ID register val
+ * @q_pri_val_reg: QUEUE_PRI_VAL register val
+ * @rc_q_maxsize_reg: RC_QUEUE_MAXSIZE register val
+ * @bri_q_maxsize_reg: BRI_QUEUE_MAXSIZE register val
+ * @comp_resp_timeout_reg: COMP_RESP_TIMEOUT register val
+ * @bus_slave_control_reg: BUS_SLAVE_CONTROL_REG register val
+ * @deadlock_avoid_type_reg: DEADLOCK_AVOID_TYPE register val
+ * @intpin_reg_wpermit_reg0: INTPIN_REG_WPERMIT register 0 val
+ * @intpin_reg_wpermit_reg1: INTPIN_REG_WPERMIT register 1 val
+ * @intpin_reg_wpermit_reg2: INTPIN_REG_WPERMIT register 2 val
+ * @intpin_reg_wpermit_reg3: INTPIN_REG_WPERMIT register 3 val
+ * @int_reduce_control_reg: INT_REDUCE_CONTROL registers val
+ * @clkcfg_reg: CLK CFG register val
+ * @pch_phub_base_address: Register base address
+ * @pch_phub_extrom_base_address: external rom base address
+ * @pch_phub_suspended: PHUB status val
+ */
+struct pch_phub_reg {
+ u32 phub_id_reg;
+ u32 q_pri_val_reg;
+ u32 rc_q_maxsize_reg;
+ u32 bri_q_maxsize_reg;
+ u32 comp_resp_timeout_reg;
+ u32 bus_slave_control_reg;
+ u32 deadlock_avoid_type_reg;
+ u32 intpin_reg_wpermit_reg0;
+ u32 intpin_reg_wpermit_reg1;
+ u32 intpin_reg_wpermit_reg2;
+ u32 intpin_reg_wpermit_reg3;
+ u32 int_reduce_control_reg[MAX_NUM_INT_REDUCE_CONTROL_REG];
+ u32 clkcfg_reg;
+ void __iomem *pch_phub_base_address;
+ void __iomem *pch_phub_extrom_base_address;
+ int pch_phub_suspended;
+};
+
+/* SROM SPEC for MAC address assignment offset */
+static const int pch_phub_mac_offset[ETH_ALEN] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
+
+static DEFINE_MUTEX(pch_phub_mutex);
+static struct pch_phub_reg pch_phub_reg;
+
+/**
+ * pch_phub_read_modify_write_reg() - Reading modifying and writing register
+ * @reg_addr_offset: Register offset address value.
+ * @data: Writing value.
+ * @mask: Mask value.
+ */
+static void pch_phub_read_modify_write_reg(unsigned int reg_addr_offset,
+ unsigned int data, unsigned int mask)
+{
+ void __iomem *reg_addr = pch_phub_reg.pch_phub_base_address +
+ reg_addr_offset;
+ iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr);
+}
+
+/* pch_phub_save_reg_conf - saves register configuration */
+static void pch_phub_save_reg_conf(struct pci_dev *pdev)
+{
+ unsigned int i;
+ void __iomem *p = pch_phub_reg.pch_phub_base_address;
+
+ pch_phub_reg.phub_id_reg = ioread32(p + PCH_PHUB_ID_REG);
+ pch_phub_reg.q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
+ pch_phub_reg.rc_q_maxsize_reg =
+ ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
+ pch_phub_reg.bri_q_maxsize_reg =
+ ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
+ pch_phub_reg.comp_resp_timeout_reg =
+ ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
+ pch_phub_reg.bus_slave_control_reg =
+ ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
+ pch_phub_reg.deadlock_avoid_type_reg =
+ ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
+ pch_phub_reg.intpin_reg_wpermit_reg0 =
+ ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
+ pch_phub_reg.intpin_reg_wpermit_reg1 =
+ ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
+ pch_phub_reg.intpin_reg_wpermit_reg2 =
+ ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
+ pch_phub_reg.intpin_reg_wpermit_reg3 =
+ ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
+ dev_dbg(&pdev->dev, "%s : "
+ "pch_phub_reg.phub_id_reg=%x, "
+ "pch_phub_reg.q_pri_val_reg=%x, "
+ "pch_phub_reg.rc_q_maxsize_reg=%x, "
+ "pch_phub_reg.bri_q_maxsize_reg=%x, "
+ "pch_phub_reg.comp_resp_timeout_reg=%x, "
+ "pch_phub_reg.bus_slave_control_reg=%x, "
+ "pch_phub_reg.deadlock_avoid_type_reg=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n", __func__,
+ pch_phub_reg.phub_id_reg,
+ pch_phub_reg.q_pri_val_reg,
+ pch_phub_reg.rc_q_maxsize_reg,
+ pch_phub_reg.bri_q_maxsize_reg,
+ pch_phub_reg.comp_resp_timeout_reg,
+ pch_phub_reg.bus_slave_control_reg,
+ pch_phub_reg.deadlock_avoid_type_reg,
+ pch_phub_reg.intpin_reg_wpermit_reg0,
+ pch_phub_reg.intpin_reg_wpermit_reg1,
+ pch_phub_reg.intpin_reg_wpermit_reg2,
+ pch_phub_reg.intpin_reg_wpermit_reg3);
+ for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
+ pch_phub_reg.int_reduce_control_reg[i] =
+ ioread32(p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
+ dev_dbg(&pdev->dev, "%s : "
+ "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
+ __func__, i, pch_phub_reg.int_reduce_control_reg[i]);
+ }
+ pch_phub_reg.clkcfg_reg = ioread32(p + CLKCFG_REG_OFFSET);
+}
+
+/* pch_phub_restore_reg_conf - restore register configuration */
+static void pch_phub_restore_reg_conf(struct pci_dev *pdev)
+{
+ unsigned int i;
+ void __iomem *p = pch_phub_reg.pch_phub_base_address;
+
+ iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_ID_REG);
+ iowrite32(pch_phub_reg.q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
+ iowrite32(pch_phub_reg.rc_q_maxsize_reg,
+ p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
+ iowrite32(pch_phub_reg.bri_q_maxsize_reg,
+ p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
+ iowrite32(pch_phub_reg.comp_resp_timeout_reg,
+ p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
+ iowrite32(pch_phub_reg.bus_slave_control_reg,
+ p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
+ iowrite32(pch_phub_reg.deadlock_avoid_type_reg,
+ p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
+ iowrite32(pch_phub_reg.intpin_reg_wpermit_reg0,
+ p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
+ iowrite32(pch_phub_reg.intpin_reg_wpermit_reg1,
+ p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
+ iowrite32(pch_phub_reg.intpin_reg_wpermit_reg2,
+ p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
+ iowrite32(pch_phub_reg.intpin_reg_wpermit_reg3,
+ p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
+ dev_dbg(&pdev->dev, "%s : "
+ "pch_phub_reg.phub_id_reg=%x, "
+ "pch_phub_reg.q_pri_val_reg=%x, "
+ "pch_phub_reg.rc_q_maxsize_reg=%x, "
+ "pch_phub_reg.bri_q_maxsize_reg=%x, "
+ "pch_phub_reg.comp_resp_timeout_reg=%x, "
+ "pch_phub_reg.bus_slave_control_reg=%x, "
+ "pch_phub_reg.deadlock_avoid_type_reg=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n", __func__,
+ pch_phub_reg.phub_id_reg,
+ pch_phub_reg.q_pri_val_reg,
+ pch_phub_reg.rc_q_maxsize_reg,
+ pch_phub_reg.bri_q_maxsize_reg,
+ pch_phub_reg.comp_resp_timeout_reg,
+ pch_phub_reg.bus_slave_control_reg,
+ pch_phub_reg.deadlock_avoid_type_reg,
+ pch_phub_reg.intpin_reg_wpermit_reg0,
+ pch_phub_reg.intpin_reg_wpermit_reg1,
+ pch_phub_reg.intpin_reg_wpermit_reg2,
+ pch_phub_reg.intpin_reg_wpermit_reg3);
+ for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
+ iowrite32(pch_phub_reg.int_reduce_control_reg[i],
+ p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
+ dev_dbg(&pdev->dev, "%s : "
+ "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
+ __func__, i, pch_phub_reg.int_reduce_control_reg[i]);
+ }
+
+ iowrite32(pch_phub_reg.clkcfg_reg, p + CLKCFG_REG_OFFSET);
+}
+
+/**
+ * pch_phub_read_serial_rom() - Reading Serial ROM
+ * @offset_address: Serial ROM offset address to read.
+ * @data: Read buffer for specified Serial ROM value.
+ */
+static void pch_phub_read_serial_rom(unsigned int offset_address, u8 *data)
+{
+ void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
+ offset_address;
+
+ *data = ioread8(mem_addr);
+}
+
+/**
+ * pch_phub_write_serial_rom() - Writing Serial ROM
+ * @offset_address: Serial ROM offset address.
+ * @data: Serial ROM value to write.
+ */
+static int pch_phub_write_serial_rom(unsigned int offset_address, u8 data)
+{
+ void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
+ (offset_address & PCH_WORD_ADDR_MASK);
+ int i;
+ unsigned int word_data;
+ unsigned int pos;
+ unsigned int mask;
+ pos = (offset_address % 4) * 8;
+ mask = ~(0xFF << pos);
+
+ iowrite32(PCH_PHUB_ROM_WRITE_ENABLE,
+ pch_phub_reg.pch_phub_extrom_base_address +
+ PHUB_CONTROL);
+
+ word_data = ioread32(mem_addr);
+ iowrite32((word_data & mask) | (u32)data << pos, mem_addr);
+
+ i = 0;
+ while (ioread8(pch_phub_reg.pch_phub_extrom_base_address +
+ PHUB_STATUS) != 0x00) {
+ msleep(1);
+ if (PHUB_TIMEOUT == i)
+ return -EPERM;
+ i++;
+ }
+
+ iowrite32(PCH_PHUB_ROM_WRITE_DISABLE,
+ pch_phub_reg.pch_phub_extrom_base_address +
+ PHUB_CONTROL);
+
+ return 0;
+}
+
+/**
+ * pch_phub_read_serial_rom_val() - Read Serial ROM value
+ * @offset_address: Serial ROM address offset value.
+ * @data: Serial ROM value to read.
+ */
+static void pch_phub_read_serial_rom_val(unsigned int offset_address, u8 *data)
+{
+ unsigned int mem_addr;
+
+ mem_addr = PCH_PHUB_ROM_START_ADDR +
+ pch_phub_mac_offset[offset_address];
+
+ pch_phub_read_serial_rom(mem_addr, data);
+}
+
+/**
+ * pch_phub_write_serial_rom_val() - writing Serial ROM value
+ * @offset_address: Serial ROM address offset value.
+ * @data: Serial ROM value.
+ */
+static int pch_phub_write_serial_rom_val(unsigned int offset_address, u8 data)
+{
+ int retval;
+ unsigned int mem_addr;
+
+ mem_addr = PCH_PHUB_ROM_START_ADDR +
+ pch_phub_mac_offset[offset_address];
+
+ retval = pch_phub_write_serial_rom(mem_addr, data);
+
+ return retval;
+}
+
+/* pch_phub_gbe_serial_rom_conf - makes Serial ROM header format configuration
+ * for Gigabit Ethernet MAC address
+ */
+static int pch_phub_gbe_serial_rom_conf(void)
+{
+ int retval;
+
+ retval = pch_phub_write_serial_rom(0x0b, 0xbc);
+ retval |= pch_phub_write_serial_rom(0x0a, 0x10);
+ retval |= pch_phub_write_serial_rom(0x09, 0x01);
+ retval |= pch_phub_write_serial_rom(0x08, 0x02);
+
+ retval |= pch_phub_write_serial_rom(0x0f, 0x00);
+ retval |= pch_phub_write_serial_rom(0x0e, 0x00);
+ retval |= pch_phub_write_serial_rom(0x0d, 0x00);
+ retval |= pch_phub_write_serial_rom(0x0c, 0x80);
+
+ retval |= pch_phub_write_serial_rom(0x13, 0xbc);
+ retval |= pch_phub_write_serial_rom(0x12, 0x10);
+ retval |= pch_phub_write_serial_rom(0x11, 0x01);
+ retval |= pch_phub_write_serial_rom(0x10, 0x18);
+
+ retval |= pch_phub_write_serial_rom(0x1b, 0xbc);
+ retval |= pch_phub_write_serial_rom(0x1a, 0x10);
+ retval |= pch_phub_write_serial_rom(0x19, 0x01);
+ retval |= pch_phub_write_serial_rom(0x18, 0x19);
+
+ retval |= pch_phub_write_serial_rom(0x23, 0xbc);
+ retval |= pch_phub_write_serial_rom(0x22, 0x10);
+ retval |= pch_phub_write_serial_rom(0x21, 0x01);
+ retval |= pch_phub_write_serial_rom(0x20, 0x3a);
+
+ retval |= pch_phub_write_serial_rom(0x27, 0x01);
+ retval |= pch_phub_write_serial_rom(0x26, 0x00);
+ retval |= pch_phub_write_serial_rom(0x25, 0x00);
+ retval |= pch_phub_write_serial_rom(0x24, 0x00);
+
+ return retval;
+}
+
+/**
+ * pch_phub_read_gbe_mac_addr() - Read Gigabit Ethernet MAC address
+ * @offset_address: Gigabit Ethernet MAC address offset value.
+ * @data: Buffer of the Gigabit Ethernet MAC address value.
+ */
+static void pch_phub_read_gbe_mac_addr(u8 *data)
+{
+ int i;
+ for (i = 0; i < ETH_ALEN; i++)
+ pch_phub_read_serial_rom_val(i, &data[i]);
+}
+
+/**
+ * pch_phub_write_gbe_mac_addr() - Write MAC address
+ * @offset_address: Gigabit Ethernet MAC address offset value.
+ * @data: Gigabit Ethernet MAC address value.
+ */
+static int pch_phub_write_gbe_mac_addr(u8 *data)
+{
+ int retval;
+ int i;
+
+ retval = pch_phub_gbe_serial_rom_conf();
+ if (retval)
+ return retval;
+
+ for (i = 0; i < ETH_ALEN; i++) {
+ retval = pch_phub_write_serial_rom_val(i, data[i]);
+ if (retval)
+ return retval;
+ }
+
+ return retval;
+}
+
+static ssize_t pch_phub_bin_read(struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t off, size_t count)
+{
+ unsigned int rom_signature;
+ unsigned char rom_length;
+ unsigned int tmp;
+ unsigned char data;
+ unsigned int addr_offset;
+ unsigned int orom_size;
+ int ret;
+ int err;
+ int cnt;
+
+ ret = mutex_lock_interruptible(&pch_phub_mutex);
+ if (ret) {
+ err = -ERESTARTSYS;
+ goto return_err_nomutex;
+ }
+
+ /* Get Rom signature */
+ pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature);
+ rom_signature &= 0xff;
+ pch_phub_read_serial_rom(0x81, (unsigned char *)&tmp);
+ rom_signature |= (tmp & 0xff) << 8;
+ if (rom_signature == 0xAA55) {
+ pch_phub_read_serial_rom(0x82, &rom_length);
+ orom_size = rom_length * 512;
+ if (orom_size < off) {
+ addr_offset = 0;
+ goto return_ok;
+ }
+
+ for (addr_offset = 0; addr_offset < count; addr_offset++) {
+ pch_phub_read_serial_rom(0x80 + addr_offset + off,
+ &data);
+ cnt += sprintf(buf+addr_offset, "%c", data);
+ }
+ } else {
+ err = -ENODATA;
+ goto return_err;
+ }
+return_ok:
+ mutex_unlock(&pch_phub_mutex);
+ return count;
+
+return_err:
+ mutex_unlock(&pch_phub_mutex);
+return_err_nomutex:
+ return err;
+}
+
+static ssize_t pch_phub_bin_write(struct kobject *kobj,
+ struct bin_attribute *attr,
+ char *buf, loff_t off, size_t count)
+{
+ int ret_value2;
+ int err;
+ unsigned int addr_offset;
+ int ret;
+
+ ret = mutex_lock_interruptible(&pch_phub_mutex);
+ if (ret) {
+ err = -ERESTARTSYS;
+ goto return_err_nomutex;
+ }
+
+ for (addr_offset = 0; addr_offset < count; addr_offset++) {
+ if (PCH_PHUB_OROM_SIZE < off + addr_offset)
+ goto return_ok;
+
+ ret_value2 = pch_phub_write_serial_rom(0x80 + addr_offset + off,
+ buf[addr_offset]);
+ if (ret_value2) {
+ err = ret_value2;
+ goto return_err;
+ }
+ }
+
+return_ok:
+ mutex_unlock(&pch_phub_mutex);
+ return addr_offset;
+
+return_err:
+ mutex_unlock(&pch_phub_mutex);
+return_err_nomutex:
+ return err;
+}
+
+static ssize_t show_pch_mac(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ u8 mac[8];
+
+ pch_phub_read_gbe_mac_addr(mac);
+
+ return sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x\n",
+ mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
+}
+
+static ssize_t store_pch_mac(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+
+{
+ u8 mac[6];
+
+ if (count != 18)
+ return -1;
+
+ sscanf(buf, "%02x:%02x:%02x:%02x:%02x:%02x",
+ (u32 *)&mac[0], (u32 *)&mac[1], (u32 *)&mac[2], (u32 *)&mac[3],
+ (u32 *)&mac[4], (u32 *)&mac[5]);
+
+ pch_phub_write_gbe_mac_addr(mac);
+
+ return strlen(buf);
+}
+
+static DEVICE_ATTR(pch_mac, S_IRUGO | S_IWUSR, show_pch_mac, store_pch_mac);
+
+static struct attribute *pch_attrs[] = {
+ &dev_attr_pch_mac.attr,
+ NULL,
+};
+
+static struct bin_attribute pch_bin_attr = {
+ .attr = {
+ .name = "pch_firmware",
+ .mode = S_IRUGO | S_IWUSR,
+ },
+ .size = PCH_PHUB_OROM_SIZE + 1,
+ .read = pch_phub_bin_read,
+ .write = pch_phub_bin_write,
+};
+
+static struct attribute_group pch_attr_group = {
+ .attrs = pch_attrs,
+};
+
+static int __devinit pch_phub_probe(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ int retval;
+
+ int ret;
+ unsigned int rom_size;
+
+ ret = pci_enable_device(pdev);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "%s : pci_enable_device FAILED(ret=%d)", __func__, ret);
+ goto err_pci_enable_dev;
+ }
+ dev_dbg(&pdev->dev, "%s : pci_enable_device returns %d\n", __func__,
+ ret);
+
+ ret = pci_request_regions(pdev, MODULE_NAME);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "%s : pci_request_regions FAILED(ret=%d)", __func__, ret);
+ goto err_req_regions;
+ }
+ dev_dbg(&pdev->dev, "%s : "
+ "pci_request_regions returns %d\n", __func__, ret);
+
+ pch_phub_reg.pch_phub_base_address = pci_iomap(pdev, 1, 0);
+
+ if (pch_phub_reg.pch_phub_base_address == 0) {
+ dev_err(&pdev->dev, "%s : pci_iomap FAILED", __func__);
+ ret = -ENOMEM;
+ goto err_pci_iomap;
+ }
+ dev_dbg(&pdev->dev, "%s : pci_iomap SUCCESS and value "
+ "in pch_phub_base_address variable is 0x%08x\n", __func__,
+ (unsigned int)pch_phub_reg.pch_phub_base_address);
+
+ pch_phub_reg.pch_phub_extrom_base_address =
+ pci_map_rom(pdev, &rom_size);
+ if (pch_phub_reg.pch_phub_extrom_base_address == 0) {
+ dev_err(&pdev->dev, "%s : pci_map_rom FAILED", __func__);
+ ret = -ENOMEM;
+ goto err_pci_map;
+ }
+ dev_dbg(&pdev->dev, "%s : "
+ "pci_map_rom SUCCESS and value in "
+ "pch_phub_extrom_base_address variable is 0x%08x\n", __func__,
+ (unsigned int)pch_phub_reg.pch_phub_extrom_base_address);
+
+ retval = sysfs_create_group(&pdev->dev.kobj, &pch_attr_group);
+ if (retval)
+ goto err_sysfs_create;
+
+ retval = sysfs_create_bin_file(&pdev->dev.kobj, &pch_bin_attr);
+ if (retval)
+ goto exit_bin_attr;
+
+ return 0;
+
+ pch_phub_read_modify_write_reg((unsigned int)CLKCFG_REG_OFFSET,
+ CLKCFG_CAN_50MHZ, CLKCFG_CANCLK_MASK);
+
+ /* set the prefech value */
+ iowrite32(0x000ffffa, pch_phub_reg.pch_phub_base_address + 0x14);
+ /* set the interrupt delay value */
+ iowrite32(0x25, pch_phub_reg.pch_phub_base_address + 0x44);
+
+ return 0;
+exit_bin_attr:
+ sysfs_remove_group(&pdev->dev.kobj, &pch_attr_group);
+
+err_sysfs_create:
+ pci_unmap_rom(pdev, pch_phub_reg.pch_phub_extrom_base_address);
+err_pci_map:
+ pci_iounmap(pdev, pch_phub_reg.pch_phub_base_address);
+err_pci_iomap:
+ pci_release_regions(pdev);
+err_req_regions:
+ pci_disable_device(pdev);
+err_pci_enable_dev:
+ dev_err(&pdev->dev, "%s returns %d\n", __func__, ret);
+ return ret;
+}
+
+static void __devexit pch_phub_remove(struct pci_dev *pdev)
+{
+ pci_unmap_rom(pdev, pch_phub_reg.pch_phub_extrom_base_address);
+ pci_iounmap(pdev, pch_phub_reg.pch_phub_base_address);
+ pci_release_regions(pdev);
+ pci_disable_device(pdev);
+ sysfs_remove_bin_file(&pdev->dev.kobj, &pch_bin_attr);
+ sysfs_remove_group(&pdev->dev.kobj, &pch_attr_group);
+}
+
+#ifdef CONFIG_PM
+
+static int pch_phub_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+ int ret;
+
+ pch_phub_reg.pch_phub_suspended = true;/* For blocking further IOCTLs */
+ pch_phub_save_reg_conf(pdev);
+ ret = pci_save_state(pdev);
+ if (ret) {
+ dev_err(&pdev->dev,
+ " %s -pci_save_state returns %d\n", __func__, ret);
+ return ret;
+ }
+ pci_enable_wake(pdev, PCI_D3hot, 0);
+ pci_disable_device(pdev);
+ pci_set_power_state(pdev, pci_choose_state(pdev, state));
+
+ return 0;
+}
+
+static int pch_phub_resume(struct pci_dev *pdev)
+{
+ int ret;
+
+ pci_set_power_state(pdev, PCI_D0);
+ pci_restore_state(pdev);
+ ret = pci_enable_device(pdev);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "%s-pci_enable_device failed(ret=%d) ", __func__, ret);
+ return ret;
+ }
+
+ pci_enable_wake(pdev, PCI_D3hot, 0);
+ pch_phub_restore_reg_conf(pdev);
+ pch_phub_reg.pch_phub_suspended = false;
+
+ return 0;
+}
+#else
+#define pch_phub_suspend NULL
+#define pch_phub_resume NULL
+#endif /* CONFIG_PM */
+
+static struct pci_device_id pch_phub_pcidev_id[] = {
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH1_PHUB)},
+ {0,}
+};
+
+static struct pci_driver pch_phub_driver = {
+ .name = "pch_phub",
+ .id_table = pch_phub_pcidev_id,
+ .probe = pch_phub_probe,
+ .remove = __devexit_p(pch_phub_remove),
+ .suspend = pch_phub_suspend,
+ .resume = pch_phub_resume
+};
+
+static int __init pch_phub_pci_init(void)
+{
+ int ret;
+ ret = pci_register_driver(&pch_phub_driver);
+
+ return ret;
+}
+
+static void __exit pch_phub_pci_exit(void)
+{
+ pci_unregister_driver(&pch_phub_driver);
+}
+
+module_init(pch_phub_pci_init);
+module_exit(pch_phub_pci_exit);
+
+MODULE_DESCRIPTION("PCH PACKET HUB PCI Driver");
+MODULE_LICENSE("GPL");
--
1.6.0.6
On Wed, Aug 18, 2010 at 06:10:29PM +0900, Masayuki Ohtak wrote:
> Hi Greg,
>
> We have modified our phub driver with sysfs I/F.
What is "I/F"?
The driver is a lot better, thanks for doing this. It's also simpler,
right?
> Please check below.
>
> Best Regards, Ohtake(OKISEMI).
>
> ---
> 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 via sysfs I/F.
> The driver creates a character device /dev/pch_phub. That device file
> supports the following operations:
>
What operations?
> Signed-off-by: Masayuki Ohtake <[email protected]>
> Acked-by: Arnd Bergmann <[email protected]>
I don't think that Arnd has "acked" this version, right?
> ---
> drivers/misc/Kconfig | 9 +
> drivers/misc/Makefile | 1 +
> drivers/misc/pch_phub.c | 722 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 732 insertions(+), 0 deletions(-)
> create mode 100755 drivers/misc/pch_phub.c
You forgot to add documentation for your sysfs files in
Documentation/ABI/ which is a requiremend when you add new ones.
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..c998521 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -353,6 +353,15 @@ config VMWARE_BALLOON
> To compile this driver as a module, choose M here: the
> module will be called vmware_balloon.
>
> +config PCH_PHUB
> + tristate "PCH Packet Hub of Intel Topcliff"
> + depends on PCI
> + help
> + This driver is for PCH PHUB of Topcliff which is an IOH for x86
> + embedded processor.
Please spell out what "PCH" "PHUB" and "IOH" is.
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/fs.h>
> +#include <linux/uaccess.h>
> +#include <linux/string.h>
> +#include <linux/pci.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/cdev.h>
This isn't needed anymore.
> +#include <linux/device.h>
pci.h already includes this.
> +#include <linux/mutex.h>
> +#include <linux/if_ether.h>
> +#include <linux/ctype.h>
> +
> +#define PHUB_STATUS 0x00 /* Status Register offset */
> +#define PHUB_CONTROL 0x04 /* Control Register offset */
> +#define PHUB_TIMEOUT 0x05 /* Time out value for Status Register */
> +#define PCH_PHUB_ROM_WRITE_ENABLE 0x01 /* Enabling for writing ROM */
> +#define PCH_PHUB_ROM_WRITE_DISABLE 0x00 /* Disabling for writing ROM */
> +#define PCH_PHUB_ROM_START_ADDR 0x14 /* ROM data area start address offset */
> +
> +/* MAX number of INT_REDUCE_CONTROL registers */
> +#define MAX_NUM_INT_REDUCE_CONTROL_REG 128
> +#define PCI_DEVICE_ID_PCH1_PHUB 0x8801
> +#define PCH_MINOR_NOS 1
> +#define CLKCFG_CAN_50MHZ 0x12000000
> +#define CLKCFG_CANCLK_MASK 0xFF000000
> +#define MODULE_NAME "pch_phub"
This isn't needed, right? The kernel provides this to you in the build
service automatically.
> +
> +#define PHUB_IOCTL_MAGIC 0xf7
> +
> +/* Read GbE MAC address */
> +#define IOCTL_PHUB_READ_MAC_ADDR (_IOR(PHUB_IOCTL_MAGIC, 1, __u8[ETH_ALEN]))
> +
> +/* Write GbE MAC address */
> +#define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 2, __u8[ETH_ALEN]))
These 3 things are no longer needed, right?
> +
> +/* SROM ACCESS Macro */
> +#define PCH_WORD_ADDR_MASK (~((1 << 2) - 1))
> +
> +/* Registers address offset */
> +#define PCH_PHUB_ID_REG 0x0000
> +#define PCH_PHUB_QUEUE_PRI_VAL_REG 0x0004
> +#define PCH_PHUB_RC_QUEUE_MAXSIZE_REG 0x0008
> +#define PCH_PHUB_BRI_QUEUE_MAXSIZE_REG 0x000C
> +#define PCH_PHUB_COMP_RESP_TIMEOUT_REG 0x0010
> +#define PCH_PHUB_BUS_SLAVE_CONTROL_REG 0x0014
> +#define PCH_PHUB_DEADLOCK_AVOID_TYPE_REG 0x0018
> +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG0 0x0020
> +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG1 0x0024
> +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG2 0x0028
> +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG3 0x002C
> +#define PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE 0x0040
> +#define CLKCFG_REG_OFFSET 0x500
> +
> +#define PCH_PHUB_OROM_SIZE 15360
> +
> +/**
> + * struct pch_phub_reg - PHUB register structure
> + * @phub_id_reg: PHUB_ID register val
> + * @q_pri_val_reg: QUEUE_PRI_VAL register val
> + * @rc_q_maxsize_reg: RC_QUEUE_MAXSIZE register val
> + * @bri_q_maxsize_reg: BRI_QUEUE_MAXSIZE register val
> + * @comp_resp_timeout_reg: COMP_RESP_TIMEOUT register val
> + * @bus_slave_control_reg: BUS_SLAVE_CONTROL_REG register val
> + * @deadlock_avoid_type_reg: DEADLOCK_AVOID_TYPE register val
> + * @intpin_reg_wpermit_reg0: INTPIN_REG_WPERMIT register 0 val
> + * @intpin_reg_wpermit_reg1: INTPIN_REG_WPERMIT register 1 val
> + * @intpin_reg_wpermit_reg2: INTPIN_REG_WPERMIT register 2 val
> + * @intpin_reg_wpermit_reg3: INTPIN_REG_WPERMIT register 3 val
> + * @int_reduce_control_reg: INT_REDUCE_CONTROL registers val
> + * @clkcfg_reg: CLK CFG register val
> + * @pch_phub_base_address: Register base address
> + * @pch_phub_extrom_base_address: external rom base address
> + * @pch_phub_suspended: PHUB status val
> + */
> +struct pch_phub_reg {
> + u32 phub_id_reg;
> + u32 q_pri_val_reg;
> + u32 rc_q_maxsize_reg;
> + u32 bri_q_maxsize_reg;
> + u32 comp_resp_timeout_reg;
> + u32 bus_slave_control_reg;
> + u32 deadlock_avoid_type_reg;
> + u32 intpin_reg_wpermit_reg0;
> + u32 intpin_reg_wpermit_reg1;
> + u32 intpin_reg_wpermit_reg2;
> + u32 intpin_reg_wpermit_reg3;
> + u32 int_reduce_control_reg[MAX_NUM_INT_REDUCE_CONTROL_REG];
> + u32 clkcfg_reg;
> + void __iomem *pch_phub_base_address;
> + void __iomem *pch_phub_extrom_base_address;
> + int pch_phub_suspended;
You only ever set this value, not read it, so you can delete it.
> +};
> +
> +/* SROM SPEC for MAC address assignment offset */
> +static const int pch_phub_mac_offset[ETH_ALEN] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
> +
> +static DEFINE_MUTEX(pch_phub_mutex);
> +static struct pch_phub_reg pch_phub_reg;
So you can only have one of these devices in a system at the same time?
What happens when a machine ships with two of them?
> +
> +/**
> + * pch_phub_read_modify_write_reg() - Reading modifying and writing register
> + * @reg_addr_offset: Register offset address value.
> + * @data: Writing value.
> + * @mask: Mask value.
> + */
> +static void pch_phub_read_modify_write_reg(unsigned int reg_addr_offset,
> + unsigned int data, unsigned int mask)
> +{
> + void __iomem *reg_addr = pch_phub_reg.pch_phub_base_address +
> + reg_addr_offset;
> + iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr);
> +}
> +
> +/* pch_phub_save_reg_conf - saves register configuration */
> +static void pch_phub_save_reg_conf(struct pci_dev *pdev)
> +{
> + unsigned int i;
> + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> +
> + pch_phub_reg.phub_id_reg = ioread32(p + PCH_PHUB_ID_REG);
> + pch_phub_reg.q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
> + pch_phub_reg.rc_q_maxsize_reg =
> + ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
> + pch_phub_reg.bri_q_maxsize_reg =
> + ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
> + pch_phub_reg.comp_resp_timeout_reg =
> + ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
> + pch_phub_reg.bus_slave_control_reg =
> + ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
> + pch_phub_reg.deadlock_avoid_type_reg =
> + ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
> + pch_phub_reg.intpin_reg_wpermit_reg0 =
> + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
> + pch_phub_reg.intpin_reg_wpermit_reg1 =
> + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
> + pch_phub_reg.intpin_reg_wpermit_reg2 =
> + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
> + pch_phub_reg.intpin_reg_wpermit_reg3 =
> + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
> + dev_dbg(&pdev->dev, "%s : "
> + "pch_phub_reg.phub_id_reg=%x, "
> + "pch_phub_reg.q_pri_val_reg=%x, "
> + "pch_phub_reg.rc_q_maxsize_reg=%x, "
> + "pch_phub_reg.bri_q_maxsize_reg=%x, "
> + "pch_phub_reg.comp_resp_timeout_reg=%x, "
> + "pch_phub_reg.bus_slave_control_reg=%x, "
> + "pch_phub_reg.deadlock_avoid_type_reg=%x, "
> + "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
> + "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
> + "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
> + "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n", __func__,
> + pch_phub_reg.phub_id_reg,
> + pch_phub_reg.q_pri_val_reg,
> + pch_phub_reg.rc_q_maxsize_reg,
> + pch_phub_reg.bri_q_maxsize_reg,
> + pch_phub_reg.comp_resp_timeout_reg,
> + pch_phub_reg.bus_slave_control_reg,
> + pch_phub_reg.deadlock_avoid_type_reg,
> + pch_phub_reg.intpin_reg_wpermit_reg0,
> + pch_phub_reg.intpin_reg_wpermit_reg1,
> + pch_phub_reg.intpin_reg_wpermit_reg2,
> + pch_phub_reg.intpin_reg_wpermit_reg3);
> + for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
> + pch_phub_reg.int_reduce_control_reg[i] =
> + ioread32(p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
> + dev_dbg(&pdev->dev, "%s : "
> + "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
> + __func__, i, pch_phub_reg.int_reduce_control_reg[i]);
> + }
> + pch_phub_reg.clkcfg_reg = ioread32(p + CLKCFG_REG_OFFSET);
> +}
> +
> +/* pch_phub_restore_reg_conf - restore register configuration */
> +static void pch_phub_restore_reg_conf(struct pci_dev *pdev)
> +{
> + unsigned int i;
> + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> +
> + iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_ID_REG);
> + iowrite32(pch_phub_reg.q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
> + iowrite32(pch_phub_reg.rc_q_maxsize_reg,
> + p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
> + iowrite32(pch_phub_reg.bri_q_maxsize_reg,
> + p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
> + iowrite32(pch_phub_reg.comp_resp_timeout_reg,
> + p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
> + iowrite32(pch_phub_reg.bus_slave_control_reg,
> + p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
> + iowrite32(pch_phub_reg.deadlock_avoid_type_reg,
> + p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
> + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg0,
> + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
> + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg1,
> + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
> + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg2,
> + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
> + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg3,
> + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
> + dev_dbg(&pdev->dev, "%s : "
> + "pch_phub_reg.phub_id_reg=%x, "
> + "pch_phub_reg.q_pri_val_reg=%x, "
> + "pch_phub_reg.rc_q_maxsize_reg=%x, "
> + "pch_phub_reg.bri_q_maxsize_reg=%x, "
> + "pch_phub_reg.comp_resp_timeout_reg=%x, "
> + "pch_phub_reg.bus_slave_control_reg=%x, "
> + "pch_phub_reg.deadlock_avoid_type_reg=%x, "
> + "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
> + "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
> + "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
> + "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n", __func__,
> + pch_phub_reg.phub_id_reg,
> + pch_phub_reg.q_pri_val_reg,
> + pch_phub_reg.rc_q_maxsize_reg,
> + pch_phub_reg.bri_q_maxsize_reg,
> + pch_phub_reg.comp_resp_timeout_reg,
> + pch_phub_reg.bus_slave_control_reg,
> + pch_phub_reg.deadlock_avoid_type_reg,
> + pch_phub_reg.intpin_reg_wpermit_reg0,
> + pch_phub_reg.intpin_reg_wpermit_reg1,
> + pch_phub_reg.intpin_reg_wpermit_reg2,
> + pch_phub_reg.intpin_reg_wpermit_reg3);
> + for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
> + iowrite32(pch_phub_reg.int_reduce_control_reg[i],
> + p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
> + dev_dbg(&pdev->dev, "%s : "
> + "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
> + __func__, i, pch_phub_reg.int_reduce_control_reg[i]);
> + }
> +
> + iowrite32(pch_phub_reg.clkcfg_reg, p + CLKCFG_REG_OFFSET);
> +}
> +
> +/**
> + * pch_phub_read_serial_rom() - Reading Serial ROM
> + * @offset_address: Serial ROM offset address to read.
> + * @data: Read buffer for specified Serial ROM value.
> + */
> +static void pch_phub_read_serial_rom(unsigned int offset_address, u8 *data)
> +{
> + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
> + offset_address;
> +
> + *data = ioread8(mem_addr);
> +}
> +
> +/**
> + * pch_phub_write_serial_rom() - Writing Serial ROM
> + * @offset_address: Serial ROM offset address.
> + * @data: Serial ROM value to write.
> + */
> +static int pch_phub_write_serial_rom(unsigned int offset_address, u8 data)
> +{
> + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
> + (offset_address & PCH_WORD_ADDR_MASK);
> + int i;
> + unsigned int word_data;
> + unsigned int pos;
> + unsigned int mask;
> + pos = (offset_address % 4) * 8;
> + mask = ~(0xFF << pos);
> +
> + iowrite32(PCH_PHUB_ROM_WRITE_ENABLE,
> + pch_phub_reg.pch_phub_extrom_base_address +
> + PHUB_CONTROL);
> +
> + word_data = ioread32(mem_addr);
> + iowrite32((word_data & mask) | (u32)data << pos, mem_addr);
> +
> + i = 0;
> + while (ioread8(pch_phub_reg.pch_phub_extrom_base_address +
> + PHUB_STATUS) != 0x00) {
> + msleep(1);
> + if (PHUB_TIMEOUT == i)
Switch this the other way around. Linux usually puts the constants on
the right side of the if statement, not the left.
> + return -EPERM;
Not -ETIMEDOUT?
> + i++;
> + }
> +
> + iowrite32(PCH_PHUB_ROM_WRITE_DISABLE,
> + pch_phub_reg.pch_phub_extrom_base_address +
> + PHUB_CONTROL);
> +
> + return 0;
> +}
> +
> +/**
> + * pch_phub_read_serial_rom_val() - Read Serial ROM value
> + * @offset_address: Serial ROM address offset value.
> + * @data: Serial ROM value to read.
> + */
> +static void pch_phub_read_serial_rom_val(unsigned int offset_address, u8 *data)
> +{
> + unsigned int mem_addr;
> +
> + mem_addr = PCH_PHUB_ROM_START_ADDR +
> + pch_phub_mac_offset[offset_address];
> +
> + pch_phub_read_serial_rom(mem_addr, data);
> +}
> +
> +/**
> + * pch_phub_write_serial_rom_val() - writing Serial ROM value
> + * @offset_address: Serial ROM address offset value.
> + * @data: Serial ROM value.
> + */
> +static int pch_phub_write_serial_rom_val(unsigned int offset_address, u8 data)
> +{
> + int retval;
> + unsigned int mem_addr;
> +
> + mem_addr = PCH_PHUB_ROM_START_ADDR +
> + pch_phub_mac_offset[offset_address];
> +
> + retval = pch_phub_write_serial_rom(mem_addr, data);
> +
> + return retval;
> +}
> +
> +/* pch_phub_gbe_serial_rom_conf - makes Serial ROM header format configuration
> + * for Gigabit Ethernet MAC address
> + */
> +static int pch_phub_gbe_serial_rom_conf(void)
> +{
> + int retval;
> +
> + retval = pch_phub_write_serial_rom(0x0b, 0xbc);
> + retval |= pch_phub_write_serial_rom(0x0a, 0x10);
> + retval |= pch_phub_write_serial_rom(0x09, 0x01);
> + retval |= pch_phub_write_serial_rom(0x08, 0x02);
> +
> + retval |= pch_phub_write_serial_rom(0x0f, 0x00);
> + retval |= pch_phub_write_serial_rom(0x0e, 0x00);
> + retval |= pch_phub_write_serial_rom(0x0d, 0x00);
> + retval |= pch_phub_write_serial_rom(0x0c, 0x80);
> +
> + retval |= pch_phub_write_serial_rom(0x13, 0xbc);
> + retval |= pch_phub_write_serial_rom(0x12, 0x10);
> + retval |= pch_phub_write_serial_rom(0x11, 0x01);
> + retval |= pch_phub_write_serial_rom(0x10, 0x18);
> +
> + retval |= pch_phub_write_serial_rom(0x1b, 0xbc);
> + retval |= pch_phub_write_serial_rom(0x1a, 0x10);
> + retval |= pch_phub_write_serial_rom(0x19, 0x01);
> + retval |= pch_phub_write_serial_rom(0x18, 0x19);
> +
> + retval |= pch_phub_write_serial_rom(0x23, 0xbc);
> + retval |= pch_phub_write_serial_rom(0x22, 0x10);
> + retval |= pch_phub_write_serial_rom(0x21, 0x01);
> + retval |= pch_phub_write_serial_rom(0x20, 0x3a);
> +
> + retval |= pch_phub_write_serial_rom(0x27, 0x01);
> + retval |= pch_phub_write_serial_rom(0x26, 0x00);
> + retval |= pch_phub_write_serial_rom(0x25, 0x00);
> + retval |= pch_phub_write_serial_rom(0x24, 0x00);
> +
> + return retval;
> +}
> +
> +/**
> + * pch_phub_read_gbe_mac_addr() - Read Gigabit Ethernet MAC address
> + * @offset_address: Gigabit Ethernet MAC address offset value.
> + * @data: Buffer of the Gigabit Ethernet MAC address value.
> + */
> +static void pch_phub_read_gbe_mac_addr(u8 *data)
> +{
> + int i;
> + for (i = 0; i < ETH_ALEN; i++)
> + pch_phub_read_serial_rom_val(i, &data[i]);
> +}
> +
> +/**
> + * pch_phub_write_gbe_mac_addr() - Write MAC address
> + * @offset_address: Gigabit Ethernet MAC address offset value.
> + * @data: Gigabit Ethernet MAC address value.
> + */
> +static int pch_phub_write_gbe_mac_addr(u8 *data)
> +{
> + int retval;
> + int i;
> +
> + retval = pch_phub_gbe_serial_rom_conf();
> + if (retval)
> + return retval;
> +
> + for (i = 0; i < ETH_ALEN; i++) {
> + retval = pch_phub_write_serial_rom_val(i, data[i]);
> + if (retval)
> + return retval;
> + }
> +
> + return retval;
> +}
> +
> +static ssize_t pch_phub_bin_read(struct kobject *kobj,
> + struct bin_attribute *attr, char *buf,
> + loff_t off, size_t count)
> +{
> + unsigned int rom_signature;
> + unsigned char rom_length;
> + unsigned int tmp;
> + unsigned char data;
> + unsigned int addr_offset;
> + unsigned int orom_size;
> + int ret;
> + int err;
> + int cnt;
> +
> + ret = mutex_lock_interruptible(&pch_phub_mutex);
> + if (ret) {
> + err = -ERESTARTSYS;
> + goto return_err_nomutex;
> + }
> +
> + /* Get Rom signature */
> + pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature);
> + rom_signature &= 0xff;
> + pch_phub_read_serial_rom(0x81, (unsigned char *)&tmp);
> + rom_signature |= (tmp & 0xff) << 8;
> + if (rom_signature == 0xAA55) {
What is 0xAA55 for?
> + pch_phub_read_serial_rom(0x82, &rom_length);
> + orom_size = rom_length * 512;
> + if (orom_size < off) {
> + addr_offset = 0;
> + goto return_ok;
> + }
Nice that you check, but you can still read more data than you mean to
here, right? Shouldn't you be checking "count" as well?
> +
> + for (addr_offset = 0; addr_offset < count; addr_offset++) {
> + pch_phub_read_serial_rom(0x80 + addr_offset + off,
> + &data);
> + cnt += sprintf(buf+addr_offset, "%c", data);
Why not just:
buf[addr_offset] = data;
cnt++;
No need to call sprintf, right?
Or even better yet, do:
pch_phub_read_serial_rom(0x80 + addr_offset + off, &buf[addr_offset]);
cnt++;
to save a step.
And what's the "0x80" offset for?
> + }
> + } else {
> + err = -ENODATA;
> + goto return_err;
> + }
> +return_ok:
> + mutex_unlock(&pch_phub_mutex);
> + return count;
> +
> +return_err:
> + mutex_unlock(&pch_phub_mutex);
> +return_err_nomutex:
> + return err;
> +}
> +
> +static ssize_t pch_phub_bin_write(struct kobject *kobj,
> + struct bin_attribute *attr,
> + char *buf, loff_t off, size_t count)
> +{
> + int ret_value2;
> + int err;
> + unsigned int addr_offset;
> + int ret;
> +
> + ret = mutex_lock_interruptible(&pch_phub_mutex);
> + if (ret) {
> + err = -ERESTARTSYS;
> + goto return_err_nomutex;
> + }
You forgot to check count and off here also.
> +
> + for (addr_offset = 0; addr_offset < count; addr_offset++) {
> + if (PCH_PHUB_OROM_SIZE < off + addr_offset)
> + goto return_ok;
> +
> + ret_value2 = pch_phub_write_serial_rom(0x80 + addr_offset + off,
> + buf[addr_offset]);
> + if (ret_value2) {
> + err = ret_value2;
> + goto return_err;
You can use 'ret' instead of 'ret_value2' here, no need to have another
variable.
> + }
> + }
> +
> +return_ok:
> + mutex_unlock(&pch_phub_mutex);
> + return addr_offset;
> +
> +return_err:
> + mutex_unlock(&pch_phub_mutex);
> +return_err_nomutex:
> + return err;
This seems a bit complex, it can be cleaned up to not have 3 exit paths,
with two of them doing the unlocking.
> +}
> +
> +static ssize_t show_pch_mac(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + u8 mac[8];
> +
> + pch_phub_read_gbe_mac_addr(mac);
> +
> + return sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x\n",
> + mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> +}
> +
> +static ssize_t store_pch_mac(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +
> +{
> + u8 mac[6];
> +
> + if (count != 18)
> + return -1;
Why -1?
> +
> + sscanf(buf, "%02x:%02x:%02x:%02x:%02x:%02x",
> + (u32 *)&mac[0], (u32 *)&mac[1], (u32 *)&mac[2], (u32 *)&mac[3],
> + (u32 *)&mac[4], (u32 *)&mac[5]);
> +
> + pch_phub_write_gbe_mac_addr(mac);
> +
> + return strlen(buf);
No, return the length you read, not the larger value, that's only going
to confuse userspace a lot.
> +}
> +
> +static DEVICE_ATTR(pch_mac, S_IRUGO | S_IWUSR, show_pch_mac, store_pch_mac);
> +
> +static struct attribute *pch_attrs[] = {
> + &dev_attr_pch_mac.attr,
> + NULL,
> +};
> +
> +static struct bin_attribute pch_bin_attr = {
> + .attr = {
> + .name = "pch_firmware",
> + .mode = S_IRUGO | S_IWUSR,
> + },
> + .size = PCH_PHUB_OROM_SIZE + 1,
> + .read = pch_phub_bin_read,
> + .write = pch_phub_bin_write,
> +};
> +
> +static struct attribute_group pch_attr_group = {
> + .attrs = pch_attrs,
> +};
You only have one file, why a whole group?
> +
> +static int __devinit pch_phub_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + int retval;
> +
> + int ret;
> + unsigned int rom_size;
> +
> + ret = pci_enable_device(pdev);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "%s : pci_enable_device FAILED(ret=%d)", __func__, ret);
> + goto err_pci_enable_dev;
> + }
> + dev_dbg(&pdev->dev, "%s : pci_enable_device returns %d\n", __func__,
> + ret);
> +
> + ret = pci_request_regions(pdev, MODULE_NAME);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "%s : pci_request_regions FAILED(ret=%d)", __func__, ret);
> + goto err_req_regions;
> + }
> + dev_dbg(&pdev->dev, "%s : "
> + "pci_request_regions returns %d\n", __func__, ret);
> +
> + pch_phub_reg.pch_phub_base_address = pci_iomap(pdev, 1, 0);
> +
> + if (pch_phub_reg.pch_phub_base_address == 0) {
> + dev_err(&pdev->dev, "%s : pci_iomap FAILED", __func__);
> + ret = -ENOMEM;
> + goto err_pci_iomap;
> + }
> + dev_dbg(&pdev->dev, "%s : pci_iomap SUCCESS and value "
> + "in pch_phub_base_address variable is 0x%08x\n", __func__,
> + (unsigned int)pch_phub_reg.pch_phub_base_address);
> +
> + pch_phub_reg.pch_phub_extrom_base_address =
> + pci_map_rom(pdev, &rom_size);
> + if (pch_phub_reg.pch_phub_extrom_base_address == 0) {
> + dev_err(&pdev->dev, "%s : pci_map_rom FAILED", __func__);
> + ret = -ENOMEM;
> + goto err_pci_map;
> + }
> + dev_dbg(&pdev->dev, "%s : "
> + "pci_map_rom SUCCESS and value in "
> + "pch_phub_extrom_base_address variable is 0x%08x\n", __func__,
> + (unsigned int)pch_phub_reg.pch_phub_extrom_base_address);
> +
> + retval = sysfs_create_group(&pdev->dev.kobj, &pch_attr_group);
> + if (retval)
> + goto err_sysfs_create;
> +
> + retval = sysfs_create_bin_file(&pdev->dev.kobj, &pch_bin_attr);
> + if (retval)
> + goto exit_bin_attr;
> +
> + return 0;
> +
> + pch_phub_read_modify_write_reg((unsigned int)CLKCFG_REG_OFFSET,
> + CLKCFG_CAN_50MHZ, CLKCFG_CANCLK_MASK);
> +
> + /* set the prefech value */
> + iowrite32(0x000ffffa, pch_phub_reg.pch_phub_base_address + 0x14);
> + /* set the interrupt delay value */
> + iowrite32(0x25, pch_phub_reg.pch_phub_base_address + 0x44);
> +
> + return 0;
> +exit_bin_attr:
> + sysfs_remove_group(&pdev->dev.kobj, &pch_attr_group);
> +
> +err_sysfs_create:
> + pci_unmap_rom(pdev, pch_phub_reg.pch_phub_extrom_base_address);
> +err_pci_map:
> + pci_iounmap(pdev, pch_phub_reg.pch_phub_base_address);
> +err_pci_iomap:
> + pci_release_regions(pdev);
> +err_req_regions:
> + pci_disable_device(pdev);
> +err_pci_enable_dev:
> + dev_err(&pdev->dev, "%s returns %d\n", __func__, ret);
> + return ret;
> +}
> +
> +static void __devexit pch_phub_remove(struct pci_dev *pdev)
> +{
> + pci_unmap_rom(pdev, pch_phub_reg.pch_phub_extrom_base_address);
> + pci_iounmap(pdev, pch_phub_reg.pch_phub_base_address);
> + pci_release_regions(pdev);
> + pci_disable_device(pdev);
> + sysfs_remove_bin_file(&pdev->dev.kobj, &pch_bin_attr);
> + sysfs_remove_group(&pdev->dev.kobj, &pch_attr_group);
> +}
> +
> +#ifdef CONFIG_PM
> +
> +static int pch_phub_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> + int ret;
> +
> + pch_phub_reg.pch_phub_suspended = true;/* For blocking further IOCTLs */
> + pch_phub_save_reg_conf(pdev);
> + ret = pci_save_state(pdev);
> + if (ret) {
> + dev_err(&pdev->dev,
> + " %s -pci_save_state returns %d\n", __func__, ret);
> + return ret;
> + }
> + pci_enable_wake(pdev, PCI_D3hot, 0);
> + pci_disable_device(pdev);
> + pci_set_power_state(pdev, pci_choose_state(pdev, state));
> +
> + return 0;
> +}
> +
> +static int pch_phub_resume(struct pci_dev *pdev)
> +{
> + int ret;
> +
> + pci_set_power_state(pdev, PCI_D0);
> + pci_restore_state(pdev);
> + ret = pci_enable_device(pdev);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "%s-pci_enable_device failed(ret=%d) ", __func__, ret);
> + return ret;
> + }
> +
> + pci_enable_wake(pdev, PCI_D3hot, 0);
> + pch_phub_restore_reg_conf(pdev);
> + pch_phub_reg.pch_phub_suspended = false;
> +
> + return 0;
> +}
> +#else
> +#define pch_phub_suspend NULL
> +#define pch_phub_resume NULL
> +#endif /* CONFIG_PM */
> +
> +static struct pci_device_id pch_phub_pcidev_id[] = {
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH1_PHUB)},
> + {0,}
> +};
> +
> +static struct pci_driver pch_phub_driver = {
> + .name = "pch_phub",
> + .id_table = pch_phub_pcidev_id,
> + .probe = pch_phub_probe,
> + .remove = __devexit_p(pch_phub_remove),
> + .suspend = pch_phub_suspend,
> + .resume = pch_phub_resume
> +};
> +
> +static int __init pch_phub_pci_init(void)
> +{
> + int ret;
> + ret = pci_register_driver(&pch_phub_driver);
> +
> + return ret;
How about:
return pci_register_driver(&pch_phub_driver);
instead?
> +}
> +
> +static void __exit pch_phub_pci_exit(void)
> +{
> + pci_unregister_driver(&pch_phub_driver);
> +}
> +
> +module_init(pch_phub_pci_init);
> +module_exit(pch_phub_pci_exit);
> +
> +MODULE_DESCRIPTION("PCH PACKET HUB PCI Driver");
Why is "PACKET" and "HUB" all uppercase?
thanks,
greg k-h
Hi Greg,
Thank you for your comments.
> > +#define MODULE_NAME "pch_phub"
>
> This isn't needed, right? The kernel provides this to you in the build
> service automatically.
I can't understand above enough.
Is the above mean like below ?
* Delete the following line.
> > +#define MODULE_NAME "pch_phub"
* Modify the following line(Use "pch_phub" directly)
> > + ret = pci_request_regions(pdev, "pch_phub");
Thanks, Ohtake (OKISEMI)
----- Original Message -----
From: "Greg KH" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; <[email protected]>; <[email protected]>;
<[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Wednesday, August 18, 2010 10:14 PM
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_PHUB driver to 2.6.35
> On Wed, Aug 18, 2010 at 06:10:29PM +0900, Masayuki Ohtak wrote:
> > Hi Greg,
> >
> > We have modified our phub driver with sysfs I/F.
>
> What is "I/F"?
>
> The driver is a lot better, thanks for doing this. It's also simpler,
> right?
>
> > Please check below.
> >
> > Best Regards, Ohtake(OKISEMI).
> >
> > ---
> > 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 via sysfs I/F.
> > The driver creates a character device /dev/pch_phub. That device file
> > supports the following operations:
> >
>
> What operations?
>
> > Signed-off-by: Masayuki Ohtake <[email protected]>
> > Acked-by: Arnd Bergmann <[email protected]>
>
> I don't think that Arnd has "acked" this version, right?
>
> > ---
> > drivers/misc/Kconfig | 9 +
> > drivers/misc/Makefile | 1 +
> > drivers/misc/pch_phub.c | 722 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 732 insertions(+), 0 deletions(-)
> > create mode 100755 drivers/misc/pch_phub.c
>
> You forgot to add documentation for your sysfs files in
> Documentation/ABI/ which is a requiremend when you add new ones.
>
> >
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 26386a9..c998521 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -353,6 +353,15 @@ config VMWARE_BALLOON
> > To compile this driver as a module, choose M here: the
> > module will be called vmware_balloon.
> >
> > +config PCH_PHUB
> > + tristate "PCH Packet Hub of Intel Topcliff"
> > + depends on PCI
> > + help
> > + This driver is for PCH PHUB of Topcliff which is an IOH for x86
> > + embedded processor.
>
> Please spell out what "PCH" "PHUB" and "IOH" is.
>
>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/types.h>
> > +#include <linux/fs.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/string.h>
> > +#include <linux/pci.h>
> > +#include <linux/io.h>
> > +#include <linux/delay.h>
> > +#include <linux/cdev.h>
>
> This isn't needed anymore.
>
> > +#include <linux/device.h>
>
> pci.h already includes this.
>
> > +#include <linux/mutex.h>
> > +#include <linux/if_ether.h>
> > +#include <linux/ctype.h>
> > +
> > +#define PHUB_STATUS 0x00 /* Status Register offset */
> > +#define PHUB_CONTROL 0x04 /* Control Register offset */
> > +#define PHUB_TIMEOUT 0x05 /* Time out value for Status Register */
> > +#define PCH_PHUB_ROM_WRITE_ENABLE 0x01 /* Enabling for writing ROM */
> > +#define PCH_PHUB_ROM_WRITE_DISABLE 0x00 /* Disabling for writing ROM */
> > +#define PCH_PHUB_ROM_START_ADDR 0x14 /* ROM data area start address offset */
> > +
> > +/* MAX number of INT_REDUCE_CONTROL registers */
> > +#define MAX_NUM_INT_REDUCE_CONTROL_REG 128
> > +#define PCI_DEVICE_ID_PCH1_PHUB 0x8801
> > +#define PCH_MINOR_NOS 1
> > +#define CLKCFG_CAN_50MHZ 0x12000000
> > +#define CLKCFG_CANCLK_MASK 0xFF000000
> > +#define MODULE_NAME "pch_phub"
>
> This isn't needed, right? The kernel provides this to you in the build
> service automatically.
>
> > +
> > +#define PHUB_IOCTL_MAGIC 0xf7
> > +
> > +/* Read GbE MAC address */
> > +#define IOCTL_PHUB_READ_MAC_ADDR (_IOR(PHUB_IOCTL_MAGIC, 1, __u8[ETH_ALEN]))
> > +
> > +/* Write GbE MAC address */
> > +#define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 2, __u8[ETH_ALEN]))
>
> These 3 things are no longer needed, right?
>
>
> > +
> > +/* SROM ACCESS Macro */
> > +#define PCH_WORD_ADDR_MASK (~((1 << 2) - 1))
> > +
> > +/* Registers address offset */
> > +#define PCH_PHUB_ID_REG 0x0000
> > +#define PCH_PHUB_QUEUE_PRI_VAL_REG 0x0004
> > +#define PCH_PHUB_RC_QUEUE_MAXSIZE_REG 0x0008
> > +#define PCH_PHUB_BRI_QUEUE_MAXSIZE_REG 0x000C
> > +#define PCH_PHUB_COMP_RESP_TIMEOUT_REG 0x0010
> > +#define PCH_PHUB_BUS_SLAVE_CONTROL_REG 0x0014
> > +#define PCH_PHUB_DEADLOCK_AVOID_TYPE_REG 0x0018
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG0 0x0020
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG1 0x0024
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG2 0x0028
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG3 0x002C
> > +#define PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE 0x0040
> > +#define CLKCFG_REG_OFFSET 0x500
> > +
> > +#define PCH_PHUB_OROM_SIZE 15360
> > +
> > +/**
> > + * struct pch_phub_reg - PHUB register structure
> > + * @phub_id_reg: PHUB_ID register val
> > + * @q_pri_val_reg: QUEUE_PRI_VAL register val
> > + * @rc_q_maxsize_reg: RC_QUEUE_MAXSIZE register val
> > + * @bri_q_maxsize_reg: BRI_QUEUE_MAXSIZE register val
> > + * @comp_resp_timeout_reg: COMP_RESP_TIMEOUT register val
> > + * @bus_slave_control_reg: BUS_SLAVE_CONTROL_REG register val
> > + * @deadlock_avoid_type_reg: DEADLOCK_AVOID_TYPE register val
> > + * @intpin_reg_wpermit_reg0: INTPIN_REG_WPERMIT register 0 val
> > + * @intpin_reg_wpermit_reg1: INTPIN_REG_WPERMIT register 1 val
> > + * @intpin_reg_wpermit_reg2: INTPIN_REG_WPERMIT register 2 val
> > + * @intpin_reg_wpermit_reg3: INTPIN_REG_WPERMIT register 3 val
> > + * @int_reduce_control_reg: INT_REDUCE_CONTROL registers val
> > + * @clkcfg_reg: CLK CFG register val
> > + * @pch_phub_base_address: Register base address
> > + * @pch_phub_extrom_base_address: external rom base address
> > + * @pch_phub_suspended: PHUB status val
> > + */
> > +struct pch_phub_reg {
> > + u32 phub_id_reg;
> > + u32 q_pri_val_reg;
> > + u32 rc_q_maxsize_reg;
> > + u32 bri_q_maxsize_reg;
> > + u32 comp_resp_timeout_reg;
> > + u32 bus_slave_control_reg;
> > + u32 deadlock_avoid_type_reg;
> > + u32 intpin_reg_wpermit_reg0;
> > + u32 intpin_reg_wpermit_reg1;
> > + u32 intpin_reg_wpermit_reg2;
> > + u32 intpin_reg_wpermit_reg3;
> > + u32 int_reduce_control_reg[MAX_NUM_INT_REDUCE_CONTROL_REG];
> > + u32 clkcfg_reg;
> > + void __iomem *pch_phub_base_address;
> > + void __iomem *pch_phub_extrom_base_address;
> > + int pch_phub_suspended;
>
> You only ever set this value, not read it, so you can delete it.
>
> > +};
> > +
> > +/* SROM SPEC for MAC address assignment offset */
> > +static const int pch_phub_mac_offset[ETH_ALEN] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
> > +
> > +static DEFINE_MUTEX(pch_phub_mutex);
> > +static struct pch_phub_reg pch_phub_reg;
>
> So you can only have one of these devices in a system at the same time?
> What happens when a machine ships with two of them?
>
> > +
> > +/**
> > + * pch_phub_read_modify_write_reg() - Reading modifying and writing register
> > + * @reg_addr_offset: Register offset address value.
> > + * @data: Writing value.
> > + * @mask: Mask value.
> > + */
> > +static void pch_phub_read_modify_write_reg(unsigned int reg_addr_offset,
> > + unsigned int data, unsigned int mask)
> > +{
> > + void __iomem *reg_addr = pch_phub_reg.pch_phub_base_address +
> > + reg_addr_offset;
> > + iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr);
> > +}
> > +
> > +/* pch_phub_save_reg_conf - saves register configuration */
> > +static void pch_phub_save_reg_conf(struct pci_dev *pdev)
> > +{
> > + unsigned int i;
> > + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> > +
> > + pch_phub_reg.phub_id_reg = ioread32(p + PCH_PHUB_ID_REG);
> > + pch_phub_reg.q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
> > + pch_phub_reg.rc_q_maxsize_reg =
> > + ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
> > + pch_phub_reg.bri_q_maxsize_reg =
> > + ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
> > + pch_phub_reg.comp_resp_timeout_reg =
> > + ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
> > + pch_phub_reg.bus_slave_control_reg =
> > + ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
> > + pch_phub_reg.deadlock_avoid_type_reg =
> > + ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
> > + pch_phub_reg.intpin_reg_wpermit_reg0 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
> > + pch_phub_reg.intpin_reg_wpermit_reg1 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
> > + pch_phub_reg.intpin_reg_wpermit_reg2 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
> > + pch_phub_reg.intpin_reg_wpermit_reg3 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.phub_id_reg=%x, "
> > + "pch_phub_reg.q_pri_val_reg=%x, "
> > + "pch_phub_reg.rc_q_maxsize_reg=%x, "
> > + "pch_phub_reg.bri_q_maxsize_reg=%x, "
> > + "pch_phub_reg.comp_resp_timeout_reg=%x, "
> > + "pch_phub_reg.bus_slave_control_reg=%x, "
> > + "pch_phub_reg.deadlock_avoid_type_reg=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n", __func__,
> > + pch_phub_reg.phub_id_reg,
> > + pch_phub_reg.q_pri_val_reg,
> > + pch_phub_reg.rc_q_maxsize_reg,
> > + pch_phub_reg.bri_q_maxsize_reg,
> > + pch_phub_reg.comp_resp_timeout_reg,
> > + pch_phub_reg.bus_slave_control_reg,
> > + pch_phub_reg.deadlock_avoid_type_reg,
> > + pch_phub_reg.intpin_reg_wpermit_reg0,
> > + pch_phub_reg.intpin_reg_wpermit_reg1,
> > + pch_phub_reg.intpin_reg_wpermit_reg2,
> > + pch_phub_reg.intpin_reg_wpermit_reg3);
> > + for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
> > + pch_phub_reg.int_reduce_control_reg[i] =
> > + ioread32(p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
> > + __func__, i, pch_phub_reg.int_reduce_control_reg[i]);
> > + }
> > + pch_phub_reg.clkcfg_reg = ioread32(p + CLKCFG_REG_OFFSET);
> > +}
> > +
> > +/* pch_phub_restore_reg_conf - restore register configuration */
> > +static void pch_phub_restore_reg_conf(struct pci_dev *pdev)
> > +{
> > + unsigned int i;
> > + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> > +
> > + iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_ID_REG);
> > + iowrite32(pch_phub_reg.q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
> > + iowrite32(pch_phub_reg.rc_q_maxsize_reg,
> > + p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
> > + iowrite32(pch_phub_reg.bri_q_maxsize_reg,
> > + p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
> > + iowrite32(pch_phub_reg.comp_resp_timeout_reg,
> > + p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
> > + iowrite32(pch_phub_reg.bus_slave_control_reg,
> > + p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
> > + iowrite32(pch_phub_reg.deadlock_avoid_type_reg,
> > + p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg0,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg1,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg2,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg3,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.phub_id_reg=%x, "
> > + "pch_phub_reg.q_pri_val_reg=%x, "
> > + "pch_phub_reg.rc_q_maxsize_reg=%x, "
> > + "pch_phub_reg.bri_q_maxsize_reg=%x, "
> > + "pch_phub_reg.comp_resp_timeout_reg=%x, "
> > + "pch_phub_reg.bus_slave_control_reg=%x, "
> > + "pch_phub_reg.deadlock_avoid_type_reg=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n", __func__,
> > + pch_phub_reg.phub_id_reg,
> > + pch_phub_reg.q_pri_val_reg,
> > + pch_phub_reg.rc_q_maxsize_reg,
> > + pch_phub_reg.bri_q_maxsize_reg,
> > + pch_phub_reg.comp_resp_timeout_reg,
> > + pch_phub_reg.bus_slave_control_reg,
> > + pch_phub_reg.deadlock_avoid_type_reg,
> > + pch_phub_reg.intpin_reg_wpermit_reg0,
> > + pch_phub_reg.intpin_reg_wpermit_reg1,
> > + pch_phub_reg.intpin_reg_wpermit_reg2,
> > + pch_phub_reg.intpin_reg_wpermit_reg3);
> > + for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
> > + iowrite32(pch_phub_reg.int_reduce_control_reg[i],
> > + p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
> > + __func__, i, pch_phub_reg.int_reduce_control_reg[i]);
> > + }
> > +
> > + iowrite32(pch_phub_reg.clkcfg_reg, p + CLKCFG_REG_OFFSET);
> > +}
> > +
> > +/**
> > + * pch_phub_read_serial_rom() - Reading Serial ROM
> > + * @offset_address: Serial ROM offset address to read.
> > + * @data: Read buffer for specified Serial ROM value.
> > + */
> > +static void pch_phub_read_serial_rom(unsigned int offset_address, u8 *data)
> > +{
> > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
> > + offset_address;
> > +
> > + *data = ioread8(mem_addr);
> > +}
> > +
> > +/**
> > + * pch_phub_write_serial_rom() - Writing Serial ROM
> > + * @offset_address: Serial ROM offset address.
> > + * @data: Serial ROM value to write.
> > + */
> > +static int pch_phub_write_serial_rom(unsigned int offset_address, u8 data)
> > +{
> > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
> > + (offset_address & PCH_WORD_ADDR_MASK);
> > + int i;
> > + unsigned int word_data;
> > + unsigned int pos;
> > + unsigned int mask;
> > + pos = (offset_address % 4) * 8;
> > + mask = ~(0xFF << pos);
> > +
> > + iowrite32(PCH_PHUB_ROM_WRITE_ENABLE,
> > + pch_phub_reg.pch_phub_extrom_base_address +
> > + PHUB_CONTROL);
> > +
> > + word_data = ioread32(mem_addr);
> > + iowrite32((word_data & mask) | (u32)data << pos, mem_addr);
> > +
> > + i = 0;
> > + while (ioread8(pch_phub_reg.pch_phub_extrom_base_address +
> > + PHUB_STATUS) != 0x00) {
> > + msleep(1);
> > + if (PHUB_TIMEOUT == i)
>
> Switch this the other way around. Linux usually puts the constants on
> the right side of the if statement, not the left.
>
> > + return -EPERM;
>
> Not -ETIMEDOUT?
>
> > + i++;
> > + }
> > +
> > + iowrite32(PCH_PHUB_ROM_WRITE_DISABLE,
> > + pch_phub_reg.pch_phub_extrom_base_address +
> > + PHUB_CONTROL);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * pch_phub_read_serial_rom_val() - Read Serial ROM value
> > + * @offset_address: Serial ROM address offset value.
> > + * @data: Serial ROM value to read.
> > + */
> > +static void pch_phub_read_serial_rom_val(unsigned int offset_address, u8 *data)
> > +{
> > + unsigned int mem_addr;
> > +
> > + mem_addr = PCH_PHUB_ROM_START_ADDR +
> > + pch_phub_mac_offset[offset_address];
> > +
> > + pch_phub_read_serial_rom(mem_addr, data);
> > +}
> > +
> > +/**
> > + * pch_phub_write_serial_rom_val() - writing Serial ROM value
> > + * @offset_address: Serial ROM address offset value.
> > + * @data: Serial ROM value.
> > + */
> > +static int pch_phub_write_serial_rom_val(unsigned int offset_address, u8 data)
> > +{
> > + int retval;
> > + unsigned int mem_addr;
> > +
> > + mem_addr = PCH_PHUB_ROM_START_ADDR +
> > + pch_phub_mac_offset[offset_address];
> > +
> > + retval = pch_phub_write_serial_rom(mem_addr, data);
> > +
> > + return retval;
> > +}
> > +
> > +/* pch_phub_gbe_serial_rom_conf - makes Serial ROM header format configuration
> > + * for Gigabit Ethernet MAC address
> > + */
> > +static int pch_phub_gbe_serial_rom_conf(void)
> > +{
> > + int retval;
> > +
> > + retval = pch_phub_write_serial_rom(0x0b, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x0a, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x09, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x08, 0x02);
> > +
> > + retval |= pch_phub_write_serial_rom(0x0f, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x0e, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x0d, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x0c, 0x80);
> > +
> > + retval |= pch_phub_write_serial_rom(0x13, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x12, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x11, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x10, 0x18);
> > +
> > + retval |= pch_phub_write_serial_rom(0x1b, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x1a, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x19, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x18, 0x19);
> > +
> > + retval |= pch_phub_write_serial_rom(0x23, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x22, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x21, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x20, 0x3a);
> > +
> > + retval |= pch_phub_write_serial_rom(0x27, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x26, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x25, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x24, 0x00);
> > +
> > + return retval;
> > +}
> > +
> > +/**
> > + * pch_phub_read_gbe_mac_addr() - Read Gigabit Ethernet MAC address
> > + * @offset_address: Gigabit Ethernet MAC address offset value.
> > + * @data: Buffer of the Gigabit Ethernet MAC address value.
> > + */
> > +static void pch_phub_read_gbe_mac_addr(u8 *data)
> > +{
> > + int i;
> > + for (i = 0; i < ETH_ALEN; i++)
> > + pch_phub_read_serial_rom_val(i, &data[i]);
> > +}
> > +
> > +/**
> > + * pch_phub_write_gbe_mac_addr() - Write MAC address
> > + * @offset_address: Gigabit Ethernet MAC address offset value.
> > + * @data: Gigabit Ethernet MAC address value.
> > + */
> > +static int pch_phub_write_gbe_mac_addr(u8 *data)
> > +{
> > + int retval;
> > + int i;
> > +
> > + retval = pch_phub_gbe_serial_rom_conf();
> > + if (retval)
> > + return retval;
> > +
> > + for (i = 0; i < ETH_ALEN; i++) {
> > + retval = pch_phub_write_serial_rom_val(i, data[i]);
> > + if (retval)
> > + return retval;
> > + }
> > +
> > + return retval;
> > +}
> > +
> > +static ssize_t pch_phub_bin_read(struct kobject *kobj,
> > + struct bin_attribute *attr, char *buf,
> > + loff_t off, size_t count)
> > +{
> > + unsigned int rom_signature;
> > + unsigned char rom_length;
> > + unsigned int tmp;
> > + unsigned char data;
> > + unsigned int addr_offset;
> > + unsigned int orom_size;
> > + int ret;
> > + int err;
> > + int cnt;
> > +
> > + ret = mutex_lock_interruptible(&pch_phub_mutex);
> > + if (ret) {
> > + err = -ERESTARTSYS;
> > + goto return_err_nomutex;
> > + }
> > +
> > + /* Get Rom signature */
> > + pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature);
> > + rom_signature &= 0xff;
> > + pch_phub_read_serial_rom(0x81, (unsigned char *)&tmp);
> > + rom_signature |= (tmp & 0xff) << 8;
> > + if (rom_signature == 0xAA55) {
>
> What is 0xAA55 for?
>
> > + pch_phub_read_serial_rom(0x82, &rom_length);
> > + orom_size = rom_length * 512;
> > + if (orom_size < off) {
> > + addr_offset = 0;
> > + goto return_ok;
> > + }
>
> Nice that you check, but you can still read more data than you mean to
> here, right? Shouldn't you be checking "count" as well?
>
> > +
> > + for (addr_offset = 0; addr_offset < count; addr_offset++) {
> > + pch_phub_read_serial_rom(0x80 + addr_offset + off,
> > + &data);
> > + cnt += sprintf(buf+addr_offset, "%c", data);
>
> Why not just:
> buf[addr_offset] = data;
> cnt++;
>
> No need to call sprintf, right?
>
> Or even better yet, do:
> pch_phub_read_serial_rom(0x80 + addr_offset + off, &buf[addr_offset]);
> cnt++;
> to save a step.
>
> And what's the "0x80" offset for?
>
>
> > + }
> > + } else {
> > + err = -ENODATA;
> > + goto return_err;
> > + }
> > +return_ok:
> > + mutex_unlock(&pch_phub_mutex);
> > + return count;
> > +
> > +return_err:
> > + mutex_unlock(&pch_phub_mutex);
> > +return_err_nomutex:
> > + return err;
> > +}
> > +
> > +static ssize_t pch_phub_bin_write(struct kobject *kobj,
> > + struct bin_attribute *attr,
> > + char *buf, loff_t off, size_t count)
> > +{
> > + int ret_value2;
> > + int err;
> > + unsigned int addr_offset;
> > + int ret;
> > +
> > + ret = mutex_lock_interruptible(&pch_phub_mutex);
> > + if (ret) {
> > + err = -ERESTARTSYS;
> > + goto return_err_nomutex;
> > + }
>
> You forgot to check count and off here also.
>
> > +
> > + for (addr_offset = 0; addr_offset < count; addr_offset++) {
> > + if (PCH_PHUB_OROM_SIZE < off + addr_offset)
> > + goto return_ok;
> > +
> > + ret_value2 = pch_phub_write_serial_rom(0x80 + addr_offset + off,
> > + buf[addr_offset]);
> > + if (ret_value2) {
> > + err = ret_value2;
> > + goto return_err;
>
> You can use 'ret' instead of 'ret_value2' here, no need to have another
> variable.
>
> > + }
> > + }
> > +
> > +return_ok:
> > + mutex_unlock(&pch_phub_mutex);
> > + return addr_offset;
> > +
> > +return_err:
> > + mutex_unlock(&pch_phub_mutex);
> > +return_err_nomutex:
> > + return err;
>
> This seems a bit complex, it can be cleaned up to not have 3 exit paths,
> with two of them doing the unlocking.
>
>
> > +}
> > +
> > +static ssize_t show_pch_mac(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + u8 mac[8];
> > +
> > + pch_phub_read_gbe_mac_addr(mac);
> > +
> > + return sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x\n",
> > + mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> > +}
> > +
> > +static ssize_t store_pch_mac(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +
> > +{
> > + u8 mac[6];
> > +
> > + if (count != 18)
> > + return -1;
>
> Why -1?
>
> > +
> > + sscanf(buf, "%02x:%02x:%02x:%02x:%02x:%02x",
> > + (u32 *)&mac[0], (u32 *)&mac[1], (u32 *)&mac[2], (u32 *)&mac[3],
> > + (u32 *)&mac[4], (u32 *)&mac[5]);
> > +
> > + pch_phub_write_gbe_mac_addr(mac);
> > +
> > + return strlen(buf);
>
> No, return the length you read, not the larger value, that's only going
> to confuse userspace a lot.
>
> > +}
> > +
> > +static DEVICE_ATTR(pch_mac, S_IRUGO | S_IWUSR, show_pch_mac, store_pch_mac);
> > +
> > +static struct attribute *pch_attrs[] = {
> > + &dev_attr_pch_mac.attr,
> > + NULL,
> > +};
> > +
> > +static struct bin_attribute pch_bin_attr = {
> > + .attr = {
> > + .name = "pch_firmware",
> > + .mode = S_IRUGO | S_IWUSR,
> > + },
> > + .size = PCH_PHUB_OROM_SIZE + 1,
> > + .read = pch_phub_bin_read,
> > + .write = pch_phub_bin_write,
> > +};
> > +
> > +static struct attribute_group pch_attr_group = {
> > + .attrs = pch_attrs,
> > +};
>
> You only have one file, why a whole group?
>
> > +
> > +static int __devinit pch_phub_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *id)
> > +{
> > + int retval;
> > +
> > + int ret;
> > + unsigned int rom_size;
> > +
> > + ret = pci_enable_device(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "%s : pci_enable_device FAILED(ret=%d)", __func__, ret);
> > + goto err_pci_enable_dev;
> > + }
> > + dev_dbg(&pdev->dev, "%s : pci_enable_device returns %d\n", __func__,
> > + ret);
> > +
> > + ret = pci_request_regions(pdev, MODULE_NAME);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "%s : pci_request_regions FAILED(ret=%d)", __func__, ret);
> > + goto err_req_regions;
> > + }
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pci_request_regions returns %d\n", __func__, ret);
> > +
> > + pch_phub_reg.pch_phub_base_address = pci_iomap(pdev, 1, 0);
> > +
> > + if (pch_phub_reg.pch_phub_base_address == 0) {
> > + dev_err(&pdev->dev, "%s : pci_iomap FAILED", __func__);
> > + ret = -ENOMEM;
> > + goto err_pci_iomap;
> > + }
> > + dev_dbg(&pdev->dev, "%s : pci_iomap SUCCESS and value "
> > + "in pch_phub_base_address variable is 0x%08x\n", __func__,
> > + (unsigned int)pch_phub_reg.pch_phub_base_address);
> > +
> > + pch_phub_reg.pch_phub_extrom_base_address =
> > + pci_map_rom(pdev, &rom_size);
> > + if (pch_phub_reg.pch_phub_extrom_base_address == 0) {
> > + dev_err(&pdev->dev, "%s : pci_map_rom FAILED", __func__);
> > + ret = -ENOMEM;
> > + goto err_pci_map;
> > + }
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pci_map_rom SUCCESS and value in "
> > + "pch_phub_extrom_base_address variable is 0x%08x\n", __func__,
> > + (unsigned int)pch_phub_reg.pch_phub_extrom_base_address);
> > +
> > + retval = sysfs_create_group(&pdev->dev.kobj, &pch_attr_group);
> > + if (retval)
> > + goto err_sysfs_create;
> > +
> > + retval = sysfs_create_bin_file(&pdev->dev.kobj, &pch_bin_attr);
> > + if (retval)
> > + goto exit_bin_attr;
> > +
> > + return 0;
> > +
> > + pch_phub_read_modify_write_reg((unsigned int)CLKCFG_REG_OFFSET,
> > + CLKCFG_CAN_50MHZ, CLKCFG_CANCLK_MASK);
> > +
> > + /* set the prefech value */
> > + iowrite32(0x000ffffa, pch_phub_reg.pch_phub_base_address + 0x14);
> > + /* set the interrupt delay value */
> > + iowrite32(0x25, pch_phub_reg.pch_phub_base_address + 0x44);
> > +
> > + return 0;
> > +exit_bin_attr:
> > + sysfs_remove_group(&pdev->dev.kobj, &pch_attr_group);
> > +
> > +err_sysfs_create:
> > + pci_unmap_rom(pdev, pch_phub_reg.pch_phub_extrom_base_address);
> > +err_pci_map:
> > + pci_iounmap(pdev, pch_phub_reg.pch_phub_base_address);
> > +err_pci_iomap:
> > + pci_release_regions(pdev);
> > +err_req_regions:
> > + pci_disable_device(pdev);
> > +err_pci_enable_dev:
> > + dev_err(&pdev->dev, "%s returns %d\n", __func__, ret);
> > + return ret;
> > +}
> > +
> > +static void __devexit pch_phub_remove(struct pci_dev *pdev)
> > +{
> > + pci_unmap_rom(pdev, pch_phub_reg.pch_phub_extrom_base_address);
> > + pci_iounmap(pdev, pch_phub_reg.pch_phub_base_address);
> > + pci_release_regions(pdev);
> > + pci_disable_device(pdev);
> > + sysfs_remove_bin_file(&pdev->dev.kobj, &pch_bin_attr);
> > + sysfs_remove_group(&pdev->dev.kobj, &pch_attr_group);
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +
> > +static int pch_phub_suspend(struct pci_dev *pdev, pm_message_t state)
> > +{
> > + int ret;
> > +
> > + pch_phub_reg.pch_phub_suspended = true;/* For blocking further IOCTLs */
> > + pch_phub_save_reg_conf(pdev);
> > + ret = pci_save_state(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + " %s -pci_save_state returns %d\n", __func__, ret);
> > + return ret;
> > + }
> > + pci_enable_wake(pdev, PCI_D3hot, 0);
> > + pci_disable_device(pdev);
> > + pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > +
> > + return 0;
> > +}
> > +
> > +static int pch_phub_resume(struct pci_dev *pdev)
> > +{
> > + int ret;
> > +
> > + pci_set_power_state(pdev, PCI_D0);
> > + pci_restore_state(pdev);
> > + ret = pci_enable_device(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "%s-pci_enable_device failed(ret=%d) ", __func__, ret);
> > + return ret;
> > + }
> > +
> > + pci_enable_wake(pdev, PCI_D3hot, 0);
> > + pch_phub_restore_reg_conf(pdev);
> > + pch_phub_reg.pch_phub_suspended = false;
> > +
> > + return 0;
> > +}
> > +#else
> > +#define pch_phub_suspend NULL
> > +#define pch_phub_resume NULL
> > +#endif /* CONFIG_PM */
> > +
> > +static struct pci_device_id pch_phub_pcidev_id[] = {
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH1_PHUB)},
> > + {0,}
> > +};
> > +
> > +static struct pci_driver pch_phub_driver = {
> > + .name = "pch_phub",
> > + .id_table = pch_phub_pcidev_id,
> > + .probe = pch_phub_probe,
> > + .remove = __devexit_p(pch_phub_remove),
> > + .suspend = pch_phub_suspend,
> > + .resume = pch_phub_resume
> > +};
> > +
> > +static int __init pch_phub_pci_init(void)
> > +{
> > + int ret;
> > + ret = pci_register_driver(&pch_phub_driver);
> > +
> > + return ret;
>
> How about:
> return pci_register_driver(&pch_phub_driver);
> instead?
>
>
> > +}
> > +
> > +static void __exit pch_phub_pci_exit(void)
> > +{
> > + pci_unregister_driver(&pch_phub_driver);
> > +}
> > +
> > +module_init(pch_phub_pci_init);
> > +module_exit(pch_phub_pci_exit);
> > +
> > +MODULE_DESCRIPTION("PCH PACKET HUB PCI Driver");
>
> Why is "PACKET" and "HUB" all uppercase?
>
> thanks,
>
> greg k-h
>
Please find <MASA>
Thanks, Ohtake(OKISEMI)
----- Original Message -----
From: "Greg KH" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; <[email protected]>; <[email protected]>;
<[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Wednesday, August 18, 2010 10:14 PM
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_PHUB driver to 2.6.35
> On Wed, Aug 18, 2010 at 06:10:29PM +0900, Masayuki Ohtak wrote:
> > Hi Greg,
> >
> > We have modified our phub driver with sysfs I/F.
>
> What is "I/F"?
>
> The driver is a lot better, thanks for doing this. It's also simpler,
> right?
>
> > Please check below.
> >
> > Best Regards, Ohtake(OKISEMI).
> >
> > ---
> > 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 via sysfs I/F.
> > The driver creates a character device /dev/pch_phub. That device file
> > supports the following operations:
> >
>
> What operations?
>
> > Signed-off-by: Masayuki Ohtake <[email protected]>
> > Acked-by: Arnd Bergmann <[email protected]>
>
> I don't think that Arnd has "acked" this version, right?
>
> > ---
> > drivers/misc/Kconfig | 9 +
> > drivers/misc/Makefile | 1 +
> > drivers/misc/pch_phub.c | 722 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 732 insertions(+), 0 deletions(-)
> > create mode 100755 drivers/misc/pch_phub.c
>
> You forgot to add documentation for your sysfs files in
> Documentation/ABI/ which is a requiremend when you add new ones.
<MASA>
Which folder should we put stable/ or testing/ ?
>
> >
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 26386a9..c998521 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -353,6 +353,15 @@ config VMWARE_BALLOON
> > To compile this driver as a module, choose M here: the
> > module will be called vmware_balloon.
> >
> > +config PCH_PHUB
> > + tristate "PCH Packet Hub of Intel Topcliff"
> > + depends on PCI
> > + help
> > + This driver is for PCH PHUB of Topcliff which is an IOH for x86
> > + embedded processor.
>
> Please spell out what "PCH" "PHUB" and "IOH" is.
>
>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/types.h>
> > +#include <linux/fs.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/string.h>
> > +#include <linux/pci.h>
> > +#include <linux/io.h>
> > +#include <linux/delay.h>
> > +#include <linux/cdev.h>
>
> This isn't needed anymore.
>
> > +#include <linux/device.h>
>
> pci.h already includes this.
>
> > +#include <linux/mutex.h>
> > +#include <linux/if_ether.h>
> > +#include <linux/ctype.h>
> > +
> > +#define PHUB_STATUS 0x00 /* Status Register offset */
> > +#define PHUB_CONTROL 0x04 /* Control Register offset */
> > +#define PHUB_TIMEOUT 0x05 /* Time out value for Status Register */
> > +#define PCH_PHUB_ROM_WRITE_ENABLE 0x01 /* Enabling for writing ROM */
> > +#define PCH_PHUB_ROM_WRITE_DISABLE 0x00 /* Disabling for writing ROM */
> > +#define PCH_PHUB_ROM_START_ADDR 0x14 /* ROM data area start address offset */
> > +
> > +/* MAX number of INT_REDUCE_CONTROL registers */
> > +#define MAX_NUM_INT_REDUCE_CONTROL_REG 128
> > +#define PCI_DEVICE_ID_PCH1_PHUB 0x8801
> > +#define PCH_MINOR_NOS 1
> > +#define CLKCFG_CAN_50MHZ 0x12000000
> > +#define CLKCFG_CANCLK_MASK 0xFF000000
> > +#define MODULE_NAME "pch_phub"
>
> This isn't needed, right? The kernel provides this to you in the build
> service automatically.
>
> > +
> > +#define PHUB_IOCTL_MAGIC 0xf7
> > +
> > +/* Read GbE MAC address */
> > +#define IOCTL_PHUB_READ_MAC_ADDR (_IOR(PHUB_IOCTL_MAGIC, 1, __u8[ETH_ALEN]))
> > +
> > +/* Write GbE MAC address */
> > +#define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 2, __u8[ETH_ALEN]))
>
> These 3 things are no longer needed, right?
>
>
> > +
> > +/* SROM ACCESS Macro */
> > +#define PCH_WORD_ADDR_MASK (~((1 << 2) - 1))
> > +
> > +/* Registers address offset */
> > +#define PCH_PHUB_ID_REG 0x0000
> > +#define PCH_PHUB_QUEUE_PRI_VAL_REG 0x0004
> > +#define PCH_PHUB_RC_QUEUE_MAXSIZE_REG 0x0008
> > +#define PCH_PHUB_BRI_QUEUE_MAXSIZE_REG 0x000C
> > +#define PCH_PHUB_COMP_RESP_TIMEOUT_REG 0x0010
> > +#define PCH_PHUB_BUS_SLAVE_CONTROL_REG 0x0014
> > +#define PCH_PHUB_DEADLOCK_AVOID_TYPE_REG 0x0018
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG0 0x0020
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG1 0x0024
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG2 0x0028
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG3 0x002C
> > +#define PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE 0x0040
> > +#define CLKCFG_REG_OFFSET 0x500
> > +
> > +#define PCH_PHUB_OROM_SIZE 15360
> > +
> > +/**
> > + * struct pch_phub_reg - PHUB register structure
> > + * @phub_id_reg: PHUB_ID register val
> > + * @q_pri_val_reg: QUEUE_PRI_VAL register val
> > + * @rc_q_maxsize_reg: RC_QUEUE_MAXSIZE register val
> > + * @bri_q_maxsize_reg: BRI_QUEUE_MAXSIZE register val
> > + * @comp_resp_timeout_reg: COMP_RESP_TIMEOUT register val
> > + * @bus_slave_control_reg: BUS_SLAVE_CONTROL_REG register val
> > + * @deadlock_avoid_type_reg: DEADLOCK_AVOID_TYPE register val
> > + * @intpin_reg_wpermit_reg0: INTPIN_REG_WPERMIT register 0 val
> > + * @intpin_reg_wpermit_reg1: INTPIN_REG_WPERMIT register 1 val
> > + * @intpin_reg_wpermit_reg2: INTPIN_REG_WPERMIT register 2 val
> > + * @intpin_reg_wpermit_reg3: INTPIN_REG_WPERMIT register 3 val
> > + * @int_reduce_control_reg: INT_REDUCE_CONTROL registers val
> > + * @clkcfg_reg: CLK CFG register val
> > + * @pch_phub_base_address: Register base address
> > + * @pch_phub_extrom_base_address: external rom base address
> > + * @pch_phub_suspended: PHUB status val
> > + */
> > +struct pch_phub_reg {
> > + u32 phub_id_reg;
> > + u32 q_pri_val_reg;
> > + u32 rc_q_maxsize_reg;
> > + u32 bri_q_maxsize_reg;
> > + u32 comp_resp_timeout_reg;
> > + u32 bus_slave_control_reg;
> > + u32 deadlock_avoid_type_reg;
> > + u32 intpin_reg_wpermit_reg0;
> > + u32 intpin_reg_wpermit_reg1;
> > + u32 intpin_reg_wpermit_reg2;
> > + u32 intpin_reg_wpermit_reg3;
> > + u32 int_reduce_control_reg[MAX_NUM_INT_REDUCE_CONTROL_REG];
> > + u32 clkcfg_reg;
> > + void __iomem *pch_phub_base_address;
> > + void __iomem *pch_phub_extrom_base_address;
> > + int pch_phub_suspended;
>
> You only ever set this value, not read it, so you can delete it.
>
> > +};
> > +
> > +/* SROM SPEC for MAC address assignment offset */
> > +static const int pch_phub_mac_offset[ETH_ALEN] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
> > +
> > +static DEFINE_MUTEX(pch_phub_mutex);
> > +static struct pch_phub_reg pch_phub_reg;
>
> So you can only have one of these devices in a system at the same time?
> What happens when a machine ships with two of them?
<MASA>
I can't understand the above questioin meaning.
Give me more information, please.
What's does the above "these devices" mean?
>
> > +
> > +/**
> > + * pch_phub_read_modify_write_reg() - Reading modifying and writing register
> > + * @reg_addr_offset: Register offset address value.
> > + * @data: Writing value.
> > + * @mask: Mask value.
> > + */
> > +static void pch_phub_read_modify_write_reg(unsigned int reg_addr_offset,
> > + unsigned int data, unsigned int mask)
> > +{
> > + void __iomem *reg_addr = pch_phub_reg.pch_phub_base_address +
> > + reg_addr_offset;
> > + iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr);
> > +}
> > +
> > +/* pch_phub_save_reg_conf - saves register configuration */
> > +static void pch_phub_save_reg_conf(struct pci_dev *pdev)
> > +{
> > + unsigned int i;
> > + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> > +
> > + pch_phub_reg.phub_id_reg = ioread32(p + PCH_PHUB_ID_REG);
> > + pch_phub_reg.q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
> > + pch_phub_reg.rc_q_maxsize_reg =
> > + ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
> > + pch_phub_reg.bri_q_maxsize_reg =
> > + ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
> > + pch_phub_reg.comp_resp_timeout_reg =
> > + ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
> > + pch_phub_reg.bus_slave_control_reg =
> > + ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
> > + pch_phub_reg.deadlock_avoid_type_reg =
> > + ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
> > + pch_phub_reg.intpin_reg_wpermit_reg0 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
> > + pch_phub_reg.intpin_reg_wpermit_reg1 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
> > + pch_phub_reg.intpin_reg_wpermit_reg2 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
> > + pch_phub_reg.intpin_reg_wpermit_reg3 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.phub_id_reg=%x, "
> > + "pch_phub_reg.q_pri_val_reg=%x, "
> > + "pch_phub_reg.rc_q_maxsize_reg=%x, "
> > + "pch_phub_reg.bri_q_maxsize_reg=%x, "
> > + "pch_phub_reg.comp_resp_timeout_reg=%x, "
> > + "pch_phub_reg.bus_slave_control_reg=%x, "
> > + "pch_phub_reg.deadlock_avoid_type_reg=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n", __func__,
> > + pch_phub_reg.phub_id_reg,
> > + pch_phub_reg.q_pri_val_reg,
> > + pch_phub_reg.rc_q_maxsize_reg,
> > + pch_phub_reg.bri_q_maxsize_reg,
> > + pch_phub_reg.comp_resp_timeout_reg,
> > + pch_phub_reg.bus_slave_control_reg,
> > + pch_phub_reg.deadlock_avoid_type_reg,
> > + pch_phub_reg.intpin_reg_wpermit_reg0,
> > + pch_phub_reg.intpin_reg_wpermit_reg1,
> > + pch_phub_reg.intpin_reg_wpermit_reg2,
> > + pch_phub_reg.intpin_reg_wpermit_reg3);
> > + for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
> > + pch_phub_reg.int_reduce_control_reg[i] =
> > + ioread32(p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
> > + __func__, i, pch_phub_reg.int_reduce_control_reg[i]);
> > + }
> > + pch_phub_reg.clkcfg_reg = ioread32(p + CLKCFG_REG_OFFSET);
> > +}
> > +
> > +/* pch_phub_restore_reg_conf - restore register configuration */
> > +static void pch_phub_restore_reg_conf(struct pci_dev *pdev)
> > +{
> > + unsigned int i;
> > + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> > +
> > + iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_ID_REG);
> > + iowrite32(pch_phub_reg.q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
> > + iowrite32(pch_phub_reg.rc_q_maxsize_reg,
> > + p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
> > + iowrite32(pch_phub_reg.bri_q_maxsize_reg,
> > + p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
> > + iowrite32(pch_phub_reg.comp_resp_timeout_reg,
> > + p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
> > + iowrite32(pch_phub_reg.bus_slave_control_reg,
> > + p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
> > + iowrite32(pch_phub_reg.deadlock_avoid_type_reg,
> > + p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg0,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg1,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg2,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg3,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.phub_id_reg=%x, "
> > + "pch_phub_reg.q_pri_val_reg=%x, "
> > + "pch_phub_reg.rc_q_maxsize_reg=%x, "
> > + "pch_phub_reg.bri_q_maxsize_reg=%x, "
> > + "pch_phub_reg.comp_resp_timeout_reg=%x, "
> > + "pch_phub_reg.bus_slave_control_reg=%x, "
> > + "pch_phub_reg.deadlock_avoid_type_reg=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n", __func__,
> > + pch_phub_reg.phub_id_reg,
> > + pch_phub_reg.q_pri_val_reg,
> > + pch_phub_reg.rc_q_maxsize_reg,
> > + pch_phub_reg.bri_q_maxsize_reg,
> > + pch_phub_reg.comp_resp_timeout_reg,
> > + pch_phub_reg.bus_slave_control_reg,
> > + pch_phub_reg.deadlock_avoid_type_reg,
> > + pch_phub_reg.intpin_reg_wpermit_reg0,
> > + pch_phub_reg.intpin_reg_wpermit_reg1,
> > + pch_phub_reg.intpin_reg_wpermit_reg2,
> > + pch_phub_reg.intpin_reg_wpermit_reg3);
> > + for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
> > + iowrite32(pch_phub_reg.int_reduce_control_reg[i],
> > + p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
> > + __func__, i, pch_phub_reg.int_reduce_control_reg[i]);
> > + }
> > +
> > + iowrite32(pch_phub_reg.clkcfg_reg, p + CLKCFG_REG_OFFSET);
> > +}
> > +
> > +/**
> > + * pch_phub_read_serial_rom() - Reading Serial ROM
> > + * @offset_address: Serial ROM offset address to read.
> > + * @data: Read buffer for specified Serial ROM value.
> > + */
> > +static void pch_phub_read_serial_rom(unsigned int offset_address, u8 *data)
> > +{
> > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
> > + offset_address;
> > +
> > + *data = ioread8(mem_addr);
> > +}
> > +
> > +/**
> > + * pch_phub_write_serial_rom() - Writing Serial ROM
> > + * @offset_address: Serial ROM offset address.
> > + * @data: Serial ROM value to write.
> > + */
> > +static int pch_phub_write_serial_rom(unsigned int offset_address, u8 data)
> > +{
> > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
> > + (offset_address & PCH_WORD_ADDR_MASK);
> > + int i;
> > + unsigned int word_data;
> > + unsigned int pos;
> > + unsigned int mask;
> > + pos = (offset_address % 4) * 8;
> > + mask = ~(0xFF << pos);
> > +
> > + iowrite32(PCH_PHUB_ROM_WRITE_ENABLE,
> > + pch_phub_reg.pch_phub_extrom_base_address +
> > + PHUB_CONTROL);
> > +
> > + word_data = ioread32(mem_addr);
> > + iowrite32((word_data & mask) | (u32)data << pos, mem_addr);
> > +
> > + i = 0;
> > + while (ioread8(pch_phub_reg.pch_phub_extrom_base_address +
> > + PHUB_STATUS) != 0x00) {
> > + msleep(1);
> > + if (PHUB_TIMEOUT == i)
>
> Switch this the other way around. Linux usually puts the constants on
> the right side of the if statement, not the left.
>
> > + return -EPERM;
>
> Not -ETIMEDOUT?
>
> > + i++;
> > + }
> > +
> > + iowrite32(PCH_PHUB_ROM_WRITE_DISABLE,
> > + pch_phub_reg.pch_phub_extrom_base_address +
> > + PHUB_CONTROL);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * pch_phub_read_serial_rom_val() - Read Serial ROM value
> > + * @offset_address: Serial ROM address offset value.
> > + * @data: Serial ROM value to read.
> > + */
> > +static void pch_phub_read_serial_rom_val(unsigned int offset_address, u8 *data)
> > +{
> > + unsigned int mem_addr;
> > +
> > + mem_addr = PCH_PHUB_ROM_START_ADDR +
> > + pch_phub_mac_offset[offset_address];
> > +
> > + pch_phub_read_serial_rom(mem_addr, data);
> > +}
> > +
> > +/**
> > + * pch_phub_write_serial_rom_val() - writing Serial ROM value
> > + * @offset_address: Serial ROM address offset value.
> > + * @data: Serial ROM value.
> > + */
> > +static int pch_phub_write_serial_rom_val(unsigned int offset_address, u8 data)
> > +{
> > + int retval;
> > + unsigned int mem_addr;
> > +
> > + mem_addr = PCH_PHUB_ROM_START_ADDR +
> > + pch_phub_mac_offset[offset_address];
> > +
> > + retval = pch_phub_write_serial_rom(mem_addr, data);
> > +
> > + return retval;
> > +}
> > +
> > +/* pch_phub_gbe_serial_rom_conf - makes Serial ROM header format configuration
> > + * for Gigabit Ethernet MAC address
> > + */
> > +static int pch_phub_gbe_serial_rom_conf(void)
> > +{
> > + int retval;
> > +
> > + retval = pch_phub_write_serial_rom(0x0b, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x0a, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x09, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x08, 0x02);
> > +
> > + retval |= pch_phub_write_serial_rom(0x0f, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x0e, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x0d, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x0c, 0x80);
> > +
> > + retval |= pch_phub_write_serial_rom(0x13, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x12, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x11, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x10, 0x18);
> > +
> > + retval |= pch_phub_write_serial_rom(0x1b, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x1a, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x19, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x18, 0x19);
> > +
> > + retval |= pch_phub_write_serial_rom(0x23, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x22, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x21, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x20, 0x3a);
> > +
> > + retval |= pch_phub_write_serial_rom(0x27, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x26, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x25, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x24, 0x00);
> > +
> > + return retval;
> > +}
> > +
> > +/**
> > + * pch_phub_read_gbe_mac_addr() - Read Gigabit Ethernet MAC address
> > + * @offset_address: Gigabit Ethernet MAC address offset value.
> > + * @data: Buffer of the Gigabit Ethernet MAC address value.
> > + */
> > +static void pch_phub_read_gbe_mac_addr(u8 *data)
> > +{
> > + int i;
> > + for (i = 0; i < ETH_ALEN; i++)
> > + pch_phub_read_serial_rom_val(i, &data[i]);
> > +}
> > +
> > +/**
> > + * pch_phub_write_gbe_mac_addr() - Write MAC address
> > + * @offset_address: Gigabit Ethernet MAC address offset value.
> > + * @data: Gigabit Ethernet MAC address value.
> > + */
> > +static int pch_phub_write_gbe_mac_addr(u8 *data)
> > +{
> > + int retval;
> > + int i;
> > +
> > + retval = pch_phub_gbe_serial_rom_conf();
> > + if (retval)
> > + return retval;
> > +
> > + for (i = 0; i < ETH_ALEN; i++) {
> > + retval = pch_phub_write_serial_rom_val(i, data[i]);
> > + if (retval)
> > + return retval;
> > + }
> > +
> > + return retval;
> > +}
> > +
> > +static ssize_t pch_phub_bin_read(struct kobject *kobj,
> > + struct bin_attribute *attr, char *buf,
> > + loff_t off, size_t count)
> > +{
> > + unsigned int rom_signature;
> > + unsigned char rom_length;
> > + unsigned int tmp;
> > + unsigned char data;
> > + unsigned int addr_offset;
> > + unsigned int orom_size;
> > + int ret;
> > + int err;
> > + int cnt;
> > +
> > + ret = mutex_lock_interruptible(&pch_phub_mutex);
> > + if (ret) {
> > + err = -ERESTARTSYS;
> > + goto return_err_nomutex;
> > + }
> > +
> > + /* Get Rom signature */
> > + pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature);
> > + rom_signature &= 0xff;
> > + pch_phub_read_serial_rom(0x81, (unsigned char *)&tmp);
> > + rom_signature |= (tmp & 0xff) << 8;
> > + if (rom_signature == 0xAA55) {
>
> What is 0xAA55 for?
>
> > + pch_phub_read_serial_rom(0x82, &rom_length);
> > + orom_size = rom_length * 512;
> > + if (orom_size < off) {
> > + addr_offset = 0;
> > + goto return_ok;
> > + }
>
> Nice that you check, but you can still read more data than you mean to
> here, right? Shouldn't you be checking "count" as well?
>
> > +
> > + for (addr_offset = 0; addr_offset < count; addr_offset++) {
> > + pch_phub_read_serial_rom(0x80 + addr_offset + off,
> > + &data);
> > + cnt += sprintf(buf+addr_offset, "%c", data);
>
> Why not just:
> buf[addr_offset] = data;
> cnt++;
>
> No need to call sprintf, right?
>
> Or even better yet, do:
> pch_phub_read_serial_rom(0x80 + addr_offset + off, &buf[addr_offset]);
> cnt++;
> to save a step.
>
> And what's the "0x80" offset for?
>
>
> > + }
> > + } else {
> > + err = -ENODATA;
> > + goto return_err;
> > + }
> > +return_ok:
> > + mutex_unlock(&pch_phub_mutex);
> > + return count;
> > +
> > +return_err:
> > + mutex_unlock(&pch_phub_mutex);
> > +return_err_nomutex:
> > + return err;
> > +}
> > +
> > +static ssize_t pch_phub_bin_write(struct kobject *kobj,
> > + struct bin_attribute *attr,
> > + char *buf, loff_t off, size_t count)
> > +{
> > + int ret_value2;
> > + int err;
> > + unsigned int addr_offset;
> > + int ret;
> > +
> > + ret = mutex_lock_interruptible(&pch_phub_mutex);
> > + if (ret) {
> > + err = -ERESTARTSYS;
> > + goto return_err_nomutex;
> > + }
>
> You forgot to check count and off here also.
>
> > +
> > + for (addr_offset = 0; addr_offset < count; addr_offset++) {
> > + if (PCH_PHUB_OROM_SIZE < off + addr_offset)
> > + goto return_ok;
> > +
> > + ret_value2 = pch_phub_write_serial_rom(0x80 + addr_offset + off,
> > + buf[addr_offset]);
> > + if (ret_value2) {
> > + err = ret_value2;
> > + goto return_err;
>
> You can use 'ret' instead of 'ret_value2' here, no need to have another
> variable.
>
> > + }
> > + }
> > +
> > +return_ok:
> > + mutex_unlock(&pch_phub_mutex);
> > + return addr_offset;
> > +
> > +return_err:
> > + mutex_unlock(&pch_phub_mutex);
> > +return_err_nomutex:
> > + return err;
>
> This seems a bit complex, it can be cleaned up to not have 3 exit paths,
> with two of them doing the unlocking.
>
>
> > +}
> > +
> > +static ssize_t show_pch_mac(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + u8 mac[8];
> > +
> > + pch_phub_read_gbe_mac_addr(mac);
> > +
> > + return sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x\n",
> > + mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> > +}
> > +
> > +static ssize_t store_pch_mac(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +
> > +{
> > + u8 mac[6];
> > +
> > + if (count != 18)
> > + return -1;
>
> Why -1?
>
> > +
> > + sscanf(buf, "%02x:%02x:%02x:%02x:%02x:%02x",
> > + (u32 *)&mac[0], (u32 *)&mac[1], (u32 *)&mac[2], (u32 *)&mac[3],
> > + (u32 *)&mac[4], (u32 *)&mac[5]);
> > +
> > + pch_phub_write_gbe_mac_addr(mac);
> > +
> > + return strlen(buf);
>
> No, return the length you read, not the larger value, that's only going
> to confuse userspace a lot.
>
> > +}
> > +
> > +static DEVICE_ATTR(pch_mac, S_IRUGO | S_IWUSR, show_pch_mac, store_pch_mac);
> > +
> > +static struct attribute *pch_attrs[] = {
> > + &dev_attr_pch_mac.attr,
> > + NULL,
> > +};
> > +
> > +static struct bin_attribute pch_bin_attr = {
> > + .attr = {
> > + .name = "pch_firmware",
> > + .mode = S_IRUGO | S_IWUSR,
> > + },
> > + .size = PCH_PHUB_OROM_SIZE + 1,
> > + .read = pch_phub_bin_read,
> > + .write = pch_phub_bin_write,
> > +};
> > +
> > +static struct attribute_group pch_attr_group = {
> > + .attrs = pch_attrs,
> > +};
>
> You only have one file, why a whole group?
>
> > +
> > +static int __devinit pch_phub_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *id)
> > +{
> > + int retval;
> > +
> > + int ret;
> > + unsigned int rom_size;
> > +
> > + ret = pci_enable_device(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "%s : pci_enable_device FAILED(ret=%d)", __func__, ret);
> > + goto err_pci_enable_dev;
> > + }
> > + dev_dbg(&pdev->dev, "%s : pci_enable_device returns %d\n", __func__,
> > + ret);
> > +
> > + ret = pci_request_regions(pdev, MODULE_NAME);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "%s : pci_request_regions FAILED(ret=%d)", __func__, ret);
> > + goto err_req_regions;
> > + }
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pci_request_regions returns %d\n", __func__, ret);
> > +
> > + pch_phub_reg.pch_phub_base_address = pci_iomap(pdev, 1, 0);
> > +
> > + if (pch_phub_reg.pch_phub_base_address == 0) {
> > + dev_err(&pdev->dev, "%s : pci_iomap FAILED", __func__);
> > + ret = -ENOMEM;
> > + goto err_pci_iomap;
> > + }
> > + dev_dbg(&pdev->dev, "%s : pci_iomap SUCCESS and value "
> > + "in pch_phub_base_address variable is 0x%08x\n", __func__,
> > + (unsigned int)pch_phub_reg.pch_phub_base_address);
> > +
> > + pch_phub_reg.pch_phub_extrom_base_address =
> > + pci_map_rom(pdev, &rom_size);
> > + if (pch_phub_reg.pch_phub_extrom_base_address == 0) {
> > + dev_err(&pdev->dev, "%s : pci_map_rom FAILED", __func__);
> > + ret = -ENOMEM;
> > + goto err_pci_map;
> > + }
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pci_map_rom SUCCESS and value in "
> > + "pch_phub_extrom_base_address variable is 0x%08x\n", __func__,
> > + (unsigned int)pch_phub_reg.pch_phub_extrom_base_address);
> > +
> > + retval = sysfs_create_group(&pdev->dev.kobj, &pch_attr_group);
> > + if (retval)
> > + goto err_sysfs_create;
> > +
> > + retval = sysfs_create_bin_file(&pdev->dev.kobj, &pch_bin_attr);
> > + if (retval)
> > + goto exit_bin_attr;
> > +
> > + return 0;
> > +
> > + pch_phub_read_modify_write_reg((unsigned int)CLKCFG_REG_OFFSET,
> > + CLKCFG_CAN_50MHZ, CLKCFG_CANCLK_MASK);
> > +
> > + /* set the prefech value */
> > + iowrite32(0x000ffffa, pch_phub_reg.pch_phub_base_address + 0x14);
> > + /* set the interrupt delay value */
> > + iowrite32(0x25, pch_phub_reg.pch_phub_base_address + 0x44);
> > +
> > + return 0;
> > +exit_bin_attr:
> > + sysfs_remove_group(&pdev->dev.kobj, &pch_attr_group);
> > +
> > +err_sysfs_create:
> > + pci_unmap_rom(pdev, pch_phub_reg.pch_phub_extrom_base_address);
> > +err_pci_map:
> > + pci_iounmap(pdev, pch_phub_reg.pch_phub_base_address);
> > +err_pci_iomap:
> > + pci_release_regions(pdev);
> > +err_req_regions:
> > + pci_disable_device(pdev);
> > +err_pci_enable_dev:
> > + dev_err(&pdev->dev, "%s returns %d\n", __func__, ret);
> > + return ret;
> > +}
> > +
> > +static void __devexit pch_phub_remove(struct pci_dev *pdev)
> > +{
> > + pci_unmap_rom(pdev, pch_phub_reg.pch_phub_extrom_base_address);
> > + pci_iounmap(pdev, pch_phub_reg.pch_phub_base_address);
> > + pci_release_regions(pdev);
> > + pci_disable_device(pdev);
> > + sysfs_remove_bin_file(&pdev->dev.kobj, &pch_bin_attr);
> > + sysfs_remove_group(&pdev->dev.kobj, &pch_attr_group);
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +
> > +static int pch_phub_suspend(struct pci_dev *pdev, pm_message_t state)
> > +{
> > + int ret;
> > +
> > + pch_phub_reg.pch_phub_suspended = true;/* For blocking further IOCTLs */
> > + pch_phub_save_reg_conf(pdev);
> > + ret = pci_save_state(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + " %s -pci_save_state returns %d\n", __func__, ret);
> > + return ret;
> > + }
> > + pci_enable_wake(pdev, PCI_D3hot, 0);
> > + pci_disable_device(pdev);
> > + pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > +
> > + return 0;
> > +}
> > +
> > +static int pch_phub_resume(struct pci_dev *pdev)
> > +{
> > + int ret;
> > +
> > + pci_set_power_state(pdev, PCI_D0);
> > + pci_restore_state(pdev);
> > + ret = pci_enable_device(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "%s-pci_enable_device failed(ret=%d) ", __func__, ret);
> > + return ret;
> > + }
> > +
> > + pci_enable_wake(pdev, PCI_D3hot, 0);
> > + pch_phub_restore_reg_conf(pdev);
> > + pch_phub_reg.pch_phub_suspended = false;
> > +
> > + return 0;
> > +}
> > +#else
> > +#define pch_phub_suspend NULL
> > +#define pch_phub_resume NULL
> > +#endif /* CONFIG_PM */
> > +
> > +static struct pci_device_id pch_phub_pcidev_id[] = {
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH1_PHUB)},
> > + {0,}
> > +};
> > +
> > +static struct pci_driver pch_phub_driver = {
> > + .name = "pch_phub",
> > + .id_table = pch_phub_pcidev_id,
> > + .probe = pch_phub_probe,
> > + .remove = __devexit_p(pch_phub_remove),
> > + .suspend = pch_phub_suspend,
> > + .resume = pch_phub_resume
> > +};
> > +
> > +static int __init pch_phub_pci_init(void)
> > +{
> > + int ret;
> > + ret = pci_register_driver(&pch_phub_driver);
> > +
> > + return ret;
>
> How about:
> return pci_register_driver(&pch_phub_driver);
> instead?
>
>
> > +}
> > +
> > +static void __exit pch_phub_pci_exit(void)
> > +{
> > + pci_unregister_driver(&pch_phub_driver);
> > +}
> > +
> > +module_init(pch_phub_pci_init);
> > +module_exit(pch_phub_pci_exit);
> > +
> > +MODULE_DESCRIPTION("PCH PACKET HUB PCI Driver");
>
> Why is "PACKET" and "HUB" all uppercase?
>
> thanks,
>
> greg k-h
>
On Thu, Aug 19, 2010 at 03:27:23PM +0900, Masayuki Ohtake wrote:
> Hi Greg,
>
> Thank you for your comments.
> > > +#define MODULE_NAME "pch_phub"
> >
> > This isn't needed, right? The kernel provides this to you in the build
> > service automatically.
>
> I can't understand above enough.
> Is the above mean like below ?
>
> * Delete the following line.
> > > +#define MODULE_NAME "pch_phub"
>
> * Modify the following line(Use "pch_phub" directly)
> > > + ret = pci_request_regions(pdev, "pch_phub");
No, use the variable that the build system provides to you,
KBUILD_MODNAME
thanks,
greg k-h
On Thu, Aug 19, 2010 at 09:25:03PM +0900, Masayuki Ohtake wrote:
> Please find <MASA>
If someone takes the time to review your code and ask questions, it is
considered common courtesy to at least answer them all and not ignore
some of them. Please do so.
> > > drivers/misc/Kconfig | 9 +
> > > drivers/misc/Makefile | 1 +
> > > drivers/misc/pch_phub.c | 722 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 732 insertions(+), 0 deletions(-)
> > > create mode 100755 drivers/misc/pch_phub.c
> >
> > You forgot to add documentation for your sysfs files in
> > Documentation/ABI/ which is a requiremend when you add new ones.
>
> <MASA>
> Which folder should we put stable/ or testing/ ?
Which do you feel it should be in?
> > > +};
> > > +
> > > +/* SROM SPEC for MAC address assignment offset */
> > > +static const int pch_phub_mac_offset[ETH_ALEN] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
> > > +
> > > +static DEFINE_MUTEX(pch_phub_mutex);
> > > +static struct pch_phub_reg pch_phub_reg;
> >
> > So you can only have one of these devices in a system at the same time?
> > What happens when a machine ships with two of them?
>
> <MASA>
> I can't understand the above questioin meaning.
> Give me more information, please.
> What's does the above "these devices" mean?
The device the driver is controlling. What happens when this driver
runs on a system that has 2 of these devices? You need to be able to
handle multiple devices, and that doesn't happen with a single variable,
right? Please dynamically allocate it and make the lock associated to
the actual device, not the whole driver, if possible.
thanks,
greg k-h
----- Original Message -----
From: "Greg KH" <[email protected]>
To: "Masayuki Ohtake" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; <[email protected]>; <[email protected]>;
<[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Friday, August 20, 2010 12:17 AM
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_PHUB driver to 2.6.35
> On Thu, Aug 19, 2010 at 03:27:23PM +0900, Masayuki Ohtake wrote:
> > Hi Greg,
> >
> > Thank you for your comments.
> > > > +#define MODULE_NAME "pch_phub"
> > >
> > > This isn't needed, right? The kernel provides this to you in the build
> > > service automatically.
> >
> > I can't understand above enough.
> > Is the above mean like below ?
> >
> > * Delete the following line.
> > > > +#define MODULE_NAME "pch_phub"
> >
> > * Modify the following line(Use "pch_phub" directly)
> > > > + ret = pci_request_regions(pdev, "pch_phub");
>
> No, use the variable that the build system provides to you,
> KBUILD_MODNAME
I understand.
Thanks, Ohtake(OKISEMI)
>
> thanks,
>
> greg k-h
>
----- Original Message -----
From: "Greg KH" <[email protected]>
To: "Masayuki Ohtake" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; <[email protected]>; <[email protected]>;
<[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>;
"Morinaga" <[email protected]>
Sent: Friday, August 20, 2010 12:22 AM
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_PHUB driver to 2.6.35
> On Thu, Aug 19, 2010 at 09:25:03PM +0900, Masayuki Ohtake wrote:
> > Please find <MASA>
>
> If someone takes the time to review your code and ask questions, it is
> considered common courtesy to at least answer them all and not ignore
> some of them. Please do so.
>
> > > > drivers/misc/Kconfig | 9 +
> > > > drivers/misc/Makefile | 1 +
> > > > drivers/misc/pch_phub.c | 722 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 732 insertions(+), 0 deletions(-)
> > > > create mode 100755 drivers/misc/pch_phub.c
> > >
> > > You forgot to add documentation for your sysfs files in
> > > Documentation/ABI/ which is a requiremend when you add new ones.
> >
> > <MASA>
> > Which folder should we put stable/ or testing/ ?
>
> Which do you feel it should be in?
I think 'testing' is appropriate.
>
> > > > +};
> > > > +
> > > > +/* SROM SPEC for MAC address assignment offset */
> > > > +static const int pch_phub_mac_offset[ETH_ALEN] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
> > > > +
> > > > +static DEFINE_MUTEX(pch_phub_mutex);
> > > > +static struct pch_phub_reg pch_phub_reg;
> > >
> > > So you can only have one of these devices in a system at the same time?
> > > What happens when a machine ships with two of them?
> >
> > <MASA>
> > I can't understand the above questioin meaning.
> > Give me more information, please.
> > What's does the above "these devices" mean?
>
> The device the driver is controlling. What happens when this driver
> runs on a system that has 2 of these devices? You need to be able to
> handle multiple devices, and that doesn't happen with a single variable,
> right? Please dynamically allocate it and make the lock associated to
> the actual device, not the whole driver, if possible.
I can understand your saying.
But our driver for Topcliff doesn't support multiple device but single device only.
>From LSI structure point of view, I think, it is impossible that topcliff is used as multiple devices.
None the less, should our driver support multiple device ?
Thanks, Ohtake(OKISEMI)
On Mon, Aug 23, 2010 at 09:30:51PM +0900, Masayuki Ohtake wrote:
> ----- Original Message -----
> From: "Greg KH" <[email protected]>
> To: "Masayuki Ohtake" <[email protected]>
> Cc: <[email protected]>; "LKML" <[email protected]>; <[email protected]>; <[email protected]>;
> <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>;
> "Morinaga" <[email protected]>
> Sent: Friday, August 20, 2010 12:22 AM
> Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_PHUB driver to 2.6.35
>
>
> > On Thu, Aug 19, 2010 at 09:25:03PM +0900, Masayuki Ohtake wrote:
> > > Please find <MASA>
> >
> > If someone takes the time to review your code and ask questions, it is
> > considered common courtesy to at least answer them all and not ignore
> > some of them. Please do so.
> >
> > > > > drivers/misc/Kconfig | 9 +
> > > > > drivers/misc/Makefile | 1 +
> > > > > drivers/misc/pch_phub.c | 722 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > 3 files changed, 732 insertions(+), 0 deletions(-)
> > > > > create mode 100755 drivers/misc/pch_phub.c
> > > >
> > > > You forgot to add documentation for your sysfs files in
> > > > Documentation/ABI/ which is a requiremend when you add new ones.
> > >
> > > <MASA>
> > > Which folder should we put stable/ or testing/ ?
> >
> > Which do you feel it should be in?
> I think 'testing' is appropriate.
Ok, when will you feel they should move to "stable"?
> > > > > +};
> > > > > +
> > > > > +/* SROM SPEC for MAC address assignment offset */
> > > > > +static const int pch_phub_mac_offset[ETH_ALEN] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
> > > > > +
> > > > > +static DEFINE_MUTEX(pch_phub_mutex);
> > > > > +static struct pch_phub_reg pch_phub_reg;
> > > >
> > > > So you can only have one of these devices in a system at the same time?
> > > > What happens when a machine ships with two of them?
> > >
> > > <MASA>
> > > I can't understand the above questioin meaning.
> > > Give me more information, please.
> > > What's does the above "these devices" mean?
> >
> > The device the driver is controlling. What happens when this driver
> > runs on a system that has 2 of these devices? You need to be able to
> > handle multiple devices, and that doesn't happen with a single variable,
> > right? Please dynamically allocate it and make the lock associated to
> > the actual device, not the whole driver, if possible.
> I can understand your saying.
> But our driver for Topcliff doesn't support multiple device but single device only.
> From LSI structure point of view, I think, it is impossible that
> topcliff is used as multiple devices.
Are you sure that this is going to be true?
> None the less, should our driver support multiple device ?
Yes, it is trivial to make it support multiple devices, which makes it
easier in the future if you happen to have a machine with more than one.
thanks,
greg k-h
----- Original Message -----
From: "Greg KH" <[email protected]>
To: "Masayuki Ohtake" <[email protected]>
Cc: "Morinaga" <[email protected]>; <[email protected]>; <[email protected]>;
<[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; "LKML"
<[email protected]>; <[email protected]>
Sent: Tuesday, August 24, 2010 12:34 AM
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_PHUB driver to 2.6.35
> On Mon, Aug 23, 2010 at 09:30:51PM +0900, Masayuki Ohtake wrote:
> > ----- Original Message -----
> > From: "Greg KH" <[email protected]>
> > To: "Masayuki Ohtake" <[email protected]>
> > Cc: <[email protected]>; "LKML" <[email protected]>; <[email protected]>; <[email protected]>;
> > <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>;
> > "Morinaga" <[email protected]>
> > Sent: Friday, August 20, 2010 12:22 AM
> > Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_PHUB driver to 2.6.35
> >
> >
> > > On Thu, Aug 19, 2010 at 09:25:03PM +0900, Masayuki Ohtake wrote:
> > > > Please find <MASA>
> > >
> > > If someone takes the time to review your code and ask questions, it is
> > > considered common courtesy to at least answer them all and not ignore
> > > some of them. Please do so.
> > >
> > > > > > drivers/misc/Kconfig | 9 +
> > > > > > drivers/misc/Makefile | 1 +
> > > > > > drivers/misc/pch_phub.c | 722 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > 3 files changed, 732 insertions(+), 0 deletions(-)
> > > > > > create mode 100755 drivers/misc/pch_phub.c
> > > > >
> > > > > You forgot to add documentation for your sysfs files in
> > > > > Documentation/ABI/ which is a requiremend when you add new ones.
> > > >
> > > > <MASA>
> > > > Which folder should we put stable/ or testing/ ?
> > >
> > > Which do you feel it should be in?
> > I think 'testing' is appropriate.
>
> Ok, when will you feel they should move to "stable"?
After accepted by upstream and updating given comments,
we will move to stable.
>
> > > > > > +};
> > > > > > +
> > > > > > +/* SROM SPEC for MAC address assignment offset */
> > > > > > +static const int pch_phub_mac_offset[ETH_ALEN] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
> > > > > > +
> > > > > > +static DEFINE_MUTEX(pch_phub_mutex);
> > > > > > +static struct pch_phub_reg pch_phub_reg;
> > > > >
> > > > > So you can only have one of these devices in a system at the same time?
> > > > > What happens when a machine ships with two of them?
> > > >
> > > > <MASA>
> > > > I can't understand the above questioin meaning.
> > > > Give me more information, please.
> > > > What's does the above "these devices" mean?
> > >
> > > The device the driver is controlling. What happens when this driver
> > > runs on a system that has 2 of these devices? You need to be able to
> > > handle multiple devices, and that doesn't happen with a single variable,
> > > right? Please dynamically allocate it and make the lock associated to
> > > the actual device, not the whole driver, if possible.
> > I can understand your saying.
> > But our driver for Topcliff doesn't support multiple device but single device only.
> > From LSI structure point of view, I think, it is impossible that
> > topcliff is used as multiple devices.
>
> Are you sure that this is going to be true?
>
> > None the less, should our driver support multiple device ?
>
> Yes, it is trivial to make it support multiple devices, which makes it
> easier in the future if you happen to have a machine with more than one.
I understand. You are right.
I will modify for supporting multiple devices.
Thanks, Ohtake(OKISEMI)
----- Original Message -----
From: "Greg KH" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; <[email protected]>; <[email protected]>;
<[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Wednesday, August 18, 2010 10:14 PM
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_PHUB driver to 2.6.35
> On Wed, Aug 18, 2010 at 06:10:29PM +0900, Masayuki Ohtak wrote:
> > Hi Greg,
> >
> > We have modified our phub driver with sysfs I/F.
>
> What is "I/F"?
interface
>
> The driver is a lot better, thanks for doing this. It's also simpler,
> right?
>
> > Please check below.
> >
> > Best Regards, Ohtake(OKISEMI).
> >
> > ---
> > 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 via sysfs I/F.
> > The driver creates a character device /dev/pch_phub. That device file
> > supports the following operations:
> >
>
> What operations?
>
> > Signed-off-by: Masayuki Ohtake <[email protected]>
> > Acked-by: Arnd Bergmann <[email protected]>
>
> I don't think that Arnd has "acked" this version, right?
>
> > ---
> > drivers/misc/Kconfig | 9 +
> > drivers/misc/Makefile | 1 +
> > drivers/misc/pch_phub.c | 722 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 732 insertions(+), 0 deletions(-)
> > create mode 100755 drivers/misc/pch_phub.c
>
> You forgot to add documentation for your sysfs files in
> Documentation/ABI/ which is a requiremend when you add new ones.
>
> >
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 26386a9..c998521 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -353,6 +353,15 @@ config VMWARE_BALLOON
> > To compile this driver as a module, choose M here: the
> > module will be called vmware_balloon.
> >
> > +config PCH_PHUB
> > + tristate "PCH Packet Hub of Intel Topcliff"
> > + depends on PCI
> > + help
> > + This driver is for PCH PHUB of Topcliff which is an IOH for x86
> > + embedded processor.
>
> Please spell out what "PCH" "PHUB" and "IOH" is.
>
>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/types.h>
> > +#include <linux/fs.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/string.h>
> > +#include <linux/pci.h>
> > +#include <linux/io.h>
> > +#include <linux/delay.h>
> > +#include <linux/cdev.h>
>
> This isn't needed anymore.
>
> > +#include <linux/device.h>
>
> pci.h already includes this.
>
> > +#include <linux/mutex.h>
> > +#include <linux/if_ether.h>
> > +#include <linux/ctype.h>
> > +
> > +#define PHUB_STATUS 0x00 /* Status Register offset */
> > +#define PHUB_CONTROL 0x04 /* Control Register offset */
> > +#define PHUB_TIMEOUT 0x05 /* Time out value for Status Register */
> > +#define PCH_PHUB_ROM_WRITE_ENABLE 0x01 /* Enabling for writing ROM */
> > +#define PCH_PHUB_ROM_WRITE_DISABLE 0x00 /* Disabling for writing ROM */
> > +#define PCH_PHUB_ROM_START_ADDR 0x14 /* ROM data area start address offset */
> > +
> > +/* MAX number of INT_REDUCE_CONTROL registers */
> > +#define MAX_NUM_INT_REDUCE_CONTROL_REG 128
> > +#define PCI_DEVICE_ID_PCH1_PHUB 0x8801
> > +#define PCH_MINOR_NOS 1
> > +#define CLKCFG_CAN_50MHZ 0x12000000
> > +#define CLKCFG_CANCLK_MASK 0xFF000000
> > +#define MODULE_NAME "pch_phub"
>
> This isn't needed, right? The kernel provides this to you in the build
> service automatically.
>
> > +
> > +#define PHUB_IOCTL_MAGIC 0xf7
> > +
> > +/* Read GbE MAC address */
> > +#define IOCTL_PHUB_READ_MAC_ADDR (_IOR(PHUB_IOCTL_MAGIC, 1, __u8[ETH_ALEN]))
> > +
> > +/* Write GbE MAC address */
> > +#define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 2, __u8[ETH_ALEN]))
>
> These 3 things are no longer needed, right?
>
>
> > +
> > +/* SROM ACCESS Macro */
> > +#define PCH_WORD_ADDR_MASK (~((1 << 2) - 1))
> > +
> > +/* Registers address offset */
> > +#define PCH_PHUB_ID_REG 0x0000
> > +#define PCH_PHUB_QUEUE_PRI_VAL_REG 0x0004
> > +#define PCH_PHUB_RC_QUEUE_MAXSIZE_REG 0x0008
> > +#define PCH_PHUB_BRI_QUEUE_MAXSIZE_REG 0x000C
> > +#define PCH_PHUB_COMP_RESP_TIMEOUT_REG 0x0010
> > +#define PCH_PHUB_BUS_SLAVE_CONTROL_REG 0x0014
> > +#define PCH_PHUB_DEADLOCK_AVOID_TYPE_REG 0x0018
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG0 0x0020
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG1 0x0024
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG2 0x0028
> > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG3 0x002C
> > +#define PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE 0x0040
> > +#define CLKCFG_REG_OFFSET 0x500
> > +
> > +#define PCH_PHUB_OROM_SIZE 15360
> > +
> > +/**
> > + * struct pch_phub_reg - PHUB register structure
> > + * @phub_id_reg: PHUB_ID register val
> > + * @q_pri_val_reg: QUEUE_PRI_VAL register val
> > + * @rc_q_maxsize_reg: RC_QUEUE_MAXSIZE register val
> > + * @bri_q_maxsize_reg: BRI_QUEUE_MAXSIZE register val
> > + * @comp_resp_timeout_reg: COMP_RESP_TIMEOUT register val
> > + * @bus_slave_control_reg: BUS_SLAVE_CONTROL_REG register val
> > + * @deadlock_avoid_type_reg: DEADLOCK_AVOID_TYPE register val
> > + * @intpin_reg_wpermit_reg0: INTPIN_REG_WPERMIT register 0 val
> > + * @intpin_reg_wpermit_reg1: INTPIN_REG_WPERMIT register 1 val
> > + * @intpin_reg_wpermit_reg2: INTPIN_REG_WPERMIT register 2 val
> > + * @intpin_reg_wpermit_reg3: INTPIN_REG_WPERMIT register 3 val
> > + * @int_reduce_control_reg: INT_REDUCE_CONTROL registers val
> > + * @clkcfg_reg: CLK CFG register val
> > + * @pch_phub_base_address: Register base address
> > + * @pch_phub_extrom_base_address: external rom base address
> > + * @pch_phub_suspended: PHUB status val
> > + */
> > +struct pch_phub_reg {
> > + u32 phub_id_reg;
> > + u32 q_pri_val_reg;
> > + u32 rc_q_maxsize_reg;
> > + u32 bri_q_maxsize_reg;
> > + u32 comp_resp_timeout_reg;
> > + u32 bus_slave_control_reg;
> > + u32 deadlock_avoid_type_reg;
> > + u32 intpin_reg_wpermit_reg0;
> > + u32 intpin_reg_wpermit_reg1;
> > + u32 intpin_reg_wpermit_reg2;
> > + u32 intpin_reg_wpermit_reg3;
> > + u32 int_reduce_control_reg[MAX_NUM_INT_REDUCE_CONTROL_REG];
> > + u32 clkcfg_reg;
> > + void __iomem *pch_phub_base_address;
> > + void __iomem *pch_phub_extrom_base_address;
> > + int pch_phub_suspended;
>
> You only ever set this value, not read it, so you can delete it.
>
> > +};
> > +
> > +/* SROM SPEC for MAC address assignment offset */
> > +static const int pch_phub_mac_offset[ETH_ALEN] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
> > +
> > +static DEFINE_MUTEX(pch_phub_mutex);
> > +static struct pch_phub_reg pch_phub_reg;
>
> So you can only have one of these devices in a system at the same time?
> What happens when a machine ships with two of them?
>
> > +
> > +/**
> > + * pch_phub_read_modify_write_reg() - Reading modifying and writing register
> > + * @reg_addr_offset: Register offset address value.
> > + * @data: Writing value.
> > + * @mask: Mask value.
> > + */
> > +static void pch_phub_read_modify_write_reg(unsigned int reg_addr_offset,
> > + unsigned int data, unsigned int mask)
> > +{
> > + void __iomem *reg_addr = pch_phub_reg.pch_phub_base_address +
> > + reg_addr_offset;
> > + iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr);
> > +}
> > +
> > +/* pch_phub_save_reg_conf - saves register configuration */
> > +static void pch_phub_save_reg_conf(struct pci_dev *pdev)
> > +{
> > + unsigned int i;
> > + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> > +
> > + pch_phub_reg.phub_id_reg = ioread32(p + PCH_PHUB_ID_REG);
> > + pch_phub_reg.q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
> > + pch_phub_reg.rc_q_maxsize_reg =
> > + ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
> > + pch_phub_reg.bri_q_maxsize_reg =
> > + ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
> > + pch_phub_reg.comp_resp_timeout_reg =
> > + ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
> > + pch_phub_reg.bus_slave_control_reg =
> > + ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
> > + pch_phub_reg.deadlock_avoid_type_reg =
> > + ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
> > + pch_phub_reg.intpin_reg_wpermit_reg0 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
> > + pch_phub_reg.intpin_reg_wpermit_reg1 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
> > + pch_phub_reg.intpin_reg_wpermit_reg2 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
> > + pch_phub_reg.intpin_reg_wpermit_reg3 =
> > + ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.phub_id_reg=%x, "
> > + "pch_phub_reg.q_pri_val_reg=%x, "
> > + "pch_phub_reg.rc_q_maxsize_reg=%x, "
> > + "pch_phub_reg.bri_q_maxsize_reg=%x, "
> > + "pch_phub_reg.comp_resp_timeout_reg=%x, "
> > + "pch_phub_reg.bus_slave_control_reg=%x, "
> > + "pch_phub_reg.deadlock_avoid_type_reg=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n", __func__,
> > + pch_phub_reg.phub_id_reg,
> > + pch_phub_reg.q_pri_val_reg,
> > + pch_phub_reg.rc_q_maxsize_reg,
> > + pch_phub_reg.bri_q_maxsize_reg,
> > + pch_phub_reg.comp_resp_timeout_reg,
> > + pch_phub_reg.bus_slave_control_reg,
> > + pch_phub_reg.deadlock_avoid_type_reg,
> > + pch_phub_reg.intpin_reg_wpermit_reg0,
> > + pch_phub_reg.intpin_reg_wpermit_reg1,
> > + pch_phub_reg.intpin_reg_wpermit_reg2,
> > + pch_phub_reg.intpin_reg_wpermit_reg3);
> > + for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
> > + pch_phub_reg.int_reduce_control_reg[i] =
> > + ioread32(p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
> > + __func__, i, pch_phub_reg.int_reduce_control_reg[i]);
> > + }
> > + pch_phub_reg.clkcfg_reg = ioread32(p + CLKCFG_REG_OFFSET);
> > +}
> > +
> > +/* pch_phub_restore_reg_conf - restore register configuration */
> > +static void pch_phub_restore_reg_conf(struct pci_dev *pdev)
> > +{
> > + unsigned int i;
> > + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> > +
> > + iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_ID_REG);
> > + iowrite32(pch_phub_reg.q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
> > + iowrite32(pch_phub_reg.rc_q_maxsize_reg,
> > + p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
> > + iowrite32(pch_phub_reg.bri_q_maxsize_reg,
> > + p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
> > + iowrite32(pch_phub_reg.comp_resp_timeout_reg,
> > + p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
> > + iowrite32(pch_phub_reg.bus_slave_control_reg,
> > + p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
> > + iowrite32(pch_phub_reg.deadlock_avoid_type_reg,
> > + p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg0,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg1,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg2,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg3,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.phub_id_reg=%x, "
> > + "pch_phub_reg.q_pri_val_reg=%x, "
> > + "pch_phub_reg.rc_q_maxsize_reg=%x, "
> > + "pch_phub_reg.bri_q_maxsize_reg=%x, "
> > + "pch_phub_reg.comp_resp_timeout_reg=%x, "
> > + "pch_phub_reg.bus_slave_control_reg=%x, "
> > + "pch_phub_reg.deadlock_avoid_type_reg=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
> > + "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n", __func__,
> > + pch_phub_reg.phub_id_reg,
> > + pch_phub_reg.q_pri_val_reg,
> > + pch_phub_reg.rc_q_maxsize_reg,
> > + pch_phub_reg.bri_q_maxsize_reg,
> > + pch_phub_reg.comp_resp_timeout_reg,
> > + pch_phub_reg.bus_slave_control_reg,
> > + pch_phub_reg.deadlock_avoid_type_reg,
> > + pch_phub_reg.intpin_reg_wpermit_reg0,
> > + pch_phub_reg.intpin_reg_wpermit_reg1,
> > + pch_phub_reg.intpin_reg_wpermit_reg2,
> > + pch_phub_reg.intpin_reg_wpermit_reg3);
> > + for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
> > + iowrite32(pch_phub_reg.int_reduce_control_reg[i],
> > + p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
> > + __func__, i, pch_phub_reg.int_reduce_control_reg[i]);
> > + }
> > +
> > + iowrite32(pch_phub_reg.clkcfg_reg, p + CLKCFG_REG_OFFSET);
> > +}
> > +
> > +/**
> > + * pch_phub_read_serial_rom() - Reading Serial ROM
> > + * @offset_address: Serial ROM offset address to read.
> > + * @data: Read buffer for specified Serial ROM value.
> > + */
> > +static void pch_phub_read_serial_rom(unsigned int offset_address, u8 *data)
> > +{
> > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
> > + offset_address;
> > +
> > + *data = ioread8(mem_addr);
> > +}
> > +
> > +/**
> > + * pch_phub_write_serial_rom() - Writing Serial ROM
> > + * @offset_address: Serial ROM offset address.
> > + * @data: Serial ROM value to write.
> > + */
> > +static int pch_phub_write_serial_rom(unsigned int offset_address, u8 data)
> > +{
> > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
> > + (offset_address & PCH_WORD_ADDR_MASK);
> > + int i;
> > + unsigned int word_data;
> > + unsigned int pos;
> > + unsigned int mask;
> > + pos = (offset_address % 4) * 8;
> > + mask = ~(0xFF << pos);
> > +
> > + iowrite32(PCH_PHUB_ROM_WRITE_ENABLE,
> > + pch_phub_reg.pch_phub_extrom_base_address +
> > + PHUB_CONTROL);
> > +
> > + word_data = ioread32(mem_addr);
> > + iowrite32((word_data & mask) | (u32)data << pos, mem_addr);
> > +
> > + i = 0;
> > + while (ioread8(pch_phub_reg.pch_phub_extrom_base_address +
> > + PHUB_STATUS) != 0x00) {
> > + msleep(1);
> > + if (PHUB_TIMEOUT == i)
>
> Switch this the other way around. Linux usually puts the constants on
> the right side of the if statement, not the left.
>
> > + return -EPERM;
>
> Not -ETIMEDOUT?
>
> > + i++;
> > + }
> > +
> > + iowrite32(PCH_PHUB_ROM_WRITE_DISABLE,
> > + pch_phub_reg.pch_phub_extrom_base_address +
> > + PHUB_CONTROL);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * pch_phub_read_serial_rom_val() - Read Serial ROM value
> > + * @offset_address: Serial ROM address offset value.
> > + * @data: Serial ROM value to read.
> > + */
> > +static void pch_phub_read_serial_rom_val(unsigned int offset_address, u8 *data)
> > +{
> > + unsigned int mem_addr;
> > +
> > + mem_addr = PCH_PHUB_ROM_START_ADDR +
> > + pch_phub_mac_offset[offset_address];
> > +
> > + pch_phub_read_serial_rom(mem_addr, data);
> > +}
> > +
> > +/**
> > + * pch_phub_write_serial_rom_val() - writing Serial ROM value
> > + * @offset_address: Serial ROM address offset value.
> > + * @data: Serial ROM value.
> > + */
> > +static int pch_phub_write_serial_rom_val(unsigned int offset_address, u8 data)
> > +{
> > + int retval;
> > + unsigned int mem_addr;
> > +
> > + mem_addr = PCH_PHUB_ROM_START_ADDR +
> > + pch_phub_mac_offset[offset_address];
> > +
> > + retval = pch_phub_write_serial_rom(mem_addr, data);
> > +
> > + return retval;
> > +}
> > +
> > +/* pch_phub_gbe_serial_rom_conf - makes Serial ROM header format configuration
> > + * for Gigabit Ethernet MAC address
> > + */
> > +static int pch_phub_gbe_serial_rom_conf(void)
> > +{
> > + int retval;
> > +
> > + retval = pch_phub_write_serial_rom(0x0b, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x0a, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x09, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x08, 0x02);
> > +
> > + retval |= pch_phub_write_serial_rom(0x0f, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x0e, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x0d, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x0c, 0x80);
> > +
> > + retval |= pch_phub_write_serial_rom(0x13, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x12, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x11, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x10, 0x18);
> > +
> > + retval |= pch_phub_write_serial_rom(0x1b, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x1a, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x19, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x18, 0x19);
> > +
> > + retval |= pch_phub_write_serial_rom(0x23, 0xbc);
> > + retval |= pch_phub_write_serial_rom(0x22, 0x10);
> > + retval |= pch_phub_write_serial_rom(0x21, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x20, 0x3a);
> > +
> > + retval |= pch_phub_write_serial_rom(0x27, 0x01);
> > + retval |= pch_phub_write_serial_rom(0x26, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x25, 0x00);
> > + retval |= pch_phub_write_serial_rom(0x24, 0x00);
> > +
> > + return retval;
> > +}
> > +
> > +/**
> > + * pch_phub_read_gbe_mac_addr() - Read Gigabit Ethernet MAC address
> > + * @offset_address: Gigabit Ethernet MAC address offset value.
> > + * @data: Buffer of the Gigabit Ethernet MAC address value.
> > + */
> > +static void pch_phub_read_gbe_mac_addr(u8 *data)
> > +{
> > + int i;
> > + for (i = 0; i < ETH_ALEN; i++)
> > + pch_phub_read_serial_rom_val(i, &data[i]);
> > +}
> > +
> > +/**
> > + * pch_phub_write_gbe_mac_addr() - Write MAC address
> > + * @offset_address: Gigabit Ethernet MAC address offset value.
> > + * @data: Gigabit Ethernet MAC address value.
> > + */
> > +static int pch_phub_write_gbe_mac_addr(u8 *data)
> > +{
> > + int retval;
> > + int i;
> > +
> > + retval = pch_phub_gbe_serial_rom_conf();
> > + if (retval)
> > + return retval;
> > +
> > + for (i = 0; i < ETH_ALEN; i++) {
> > + retval = pch_phub_write_serial_rom_val(i, data[i]);
> > + if (retval)
> > + return retval;
> > + }
> > +
> > + return retval;
> > +}
> > +
> > +static ssize_t pch_phub_bin_read(struct kobject *kobj,
> > + struct bin_attribute *attr, char *buf,
> > + loff_t off, size_t count)
> > +{
> > + unsigned int rom_signature;
> > + unsigned char rom_length;
> > + unsigned int tmp;
> > + unsigned char data;
> > + unsigned int addr_offset;
> > + unsigned int orom_size;
> > + int ret;
> > + int err;
> > + int cnt;
> > +
> > + ret = mutex_lock_interruptible(&pch_phub_mutex);
> > + if (ret) {
> > + err = -ERESTARTSYS;
> > + goto return_err_nomutex;
> > + }
> > +
> > + /* Get Rom signature */
> > + pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature);
> > + rom_signature &= 0xff;
> > + pch_phub_read_serial_rom(0x81, (unsigned char *)&tmp);
> > + rom_signature |= (tmp & 0xff) << 8;
> > + if (rom_signature == 0xAA55) {
>
> What is 0xAA55 for?
'AA55' is signature.
In case read value is not 'AA55', firmware of SROM is invalid.
>
> > + pch_phub_read_serial_rom(0x82, &rom_length);
> > + orom_size = rom_length * 512;
> > + if (orom_size < off) {
> > + addr_offset = 0;
> > + goto return_ok;
> > + }
>
> Nice that you check, but you can still read more data than you mean to
> here, right? Shouldn't you be checking "count" as well?
>
> > +
> > + for (addr_offset = 0; addr_offset < count; addr_offset++) {
> > + pch_phub_read_serial_rom(0x80 + addr_offset + off,
> > + &data);
> > + cnt += sprintf(buf+addr_offset, "%c", data);
>
> Why not just:
> buf[addr_offset] = data;
> cnt++;
>
> No need to call sprintf, right?
>
> Or even better yet, do:
> pch_phub_read_serial_rom(0x80 + addr_offset + off, &buf[addr_offset]);
> cnt++;
> to save a step.
>
> And what's the "0x80" offset for?
"0x80" is for PCH firmware data offset.
>
>
> > + }
> > + } else {
> > + err = -ENODATA;
> > + goto return_err;
> > + }
> > +return_ok:
> > + mutex_unlock(&pch_phub_mutex);
> > + return count;
> > +
> > +return_err:
> > + mutex_unlock(&pch_phub_mutex);
> > +return_err_nomutex:
> > + return err;
> > +}
> > +
> > +static ssize_t pch_phub_bin_write(struct kobject *kobj,
> > + struct bin_attribute *attr,
> > + char *buf, loff_t off, size_t count)
> > +{
> > + int ret_value2;
> > + int err;
> > + unsigned int addr_offset;
> > + int ret;
> > +
> > + ret = mutex_lock_interruptible(&pch_phub_mutex);
> > + if (ret) {
> > + err = -ERESTARTSYS;
> > + goto return_err_nomutex;
> > + }
>
> You forgot to check count and off here also.
>
> > +
> > + for (addr_offset = 0; addr_offset < count; addr_offset++) {
> > + if (PCH_PHUB_OROM_SIZE < off + addr_offset)
> > + goto return_ok;
> > +
> > + ret_value2 = pch_phub_write_serial_rom(0x80 + addr_offset + off,
> > + buf[addr_offset]);
> > + if (ret_value2) {
> > + err = ret_value2;
> > + goto return_err;
>
> You can use 'ret' instead of 'ret_value2' here, no need to have another
> variable.
>
> > + }
> > + }
> > +
> > +return_ok:
> > + mutex_unlock(&pch_phub_mutex);
> > + return addr_offset;
> > +
> > +return_err:
> > + mutex_unlock(&pch_phub_mutex);
> > +return_err_nomutex:
> > + return err;
>
> This seems a bit complex, it can be cleaned up to not have 3 exit paths,
> with two of them doing the unlocking.
>
>
> > +}
> > +
> > +static ssize_t show_pch_mac(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + u8 mac[8];
> > +
> > + pch_phub_read_gbe_mac_addr(mac);
> > +
> > + return sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x\n",
> > + mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> > +}
> > +
> > +static ssize_t store_pch_mac(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +
> > +{
> > + u8 mac[6];
> > +
> > + if (count != 18)
> > + return -1;
>
> Why -1?
>
> > +
> > + sscanf(buf, "%02x:%02x:%02x:%02x:%02x:%02x",
> > + (u32 *)&mac[0], (u32 *)&mac[1], (u32 *)&mac[2], (u32 *)&mac[3],
> > + (u32 *)&mac[4], (u32 *)&mac[5]);
> > +
> > + pch_phub_write_gbe_mac_addr(mac);
> > +
> > + return strlen(buf);
>
> No, return the length you read, not the larger value, that's only going
> to confuse userspace a lot.
>
> > +}
> > +
> > +static DEVICE_ATTR(pch_mac, S_IRUGO | S_IWUSR, show_pch_mac, store_pch_mac);
> > +
> > +static struct attribute *pch_attrs[] = {
> > + &dev_attr_pch_mac.attr,
> > + NULL,
> > +};
> > +
> > +static struct bin_attribute pch_bin_attr = {
> > + .attr = {
> > + .name = "pch_firmware",
> > + .mode = S_IRUGO | S_IWUSR,
> > + },
> > + .size = PCH_PHUB_OROM_SIZE + 1,
> > + .read = pch_phub_bin_read,
> > + .write = pch_phub_bin_write,
> > +};
> > +
> > +static struct attribute_group pch_attr_group = {
> > + .attrs = pch_attrs,
> > +};
>
> You only have one file, why a whole group?
>
> > +
> > +static int __devinit pch_phub_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *id)
> > +{
> > + int retval;
> > +
> > + int ret;
> > + unsigned int rom_size;
> > +
> > + ret = pci_enable_device(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "%s : pci_enable_device FAILED(ret=%d)", __func__, ret);
> > + goto err_pci_enable_dev;
> > + }
> > + dev_dbg(&pdev->dev, "%s : pci_enable_device returns %d\n", __func__,
> > + ret);
> > +
> > + ret = pci_request_regions(pdev, MODULE_NAME);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "%s : pci_request_regions FAILED(ret=%d)", __func__, ret);
> > + goto err_req_regions;
> > + }
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pci_request_regions returns %d\n", __func__, ret);
> > +
> > + pch_phub_reg.pch_phub_base_address = pci_iomap(pdev, 1, 0);
> > +
> > + if (pch_phub_reg.pch_phub_base_address == 0) {
> > + dev_err(&pdev->dev, "%s : pci_iomap FAILED", __func__);
> > + ret = -ENOMEM;
> > + goto err_pci_iomap;
> > + }
> > + dev_dbg(&pdev->dev, "%s : pci_iomap SUCCESS and value "
> > + "in pch_phub_base_address variable is 0x%08x\n", __func__,
> > + (unsigned int)pch_phub_reg.pch_phub_base_address);
> > +
> > + pch_phub_reg.pch_phub_extrom_base_address =
> > + pci_map_rom(pdev, &rom_size);
> > + if (pch_phub_reg.pch_phub_extrom_base_address == 0) {
> > + dev_err(&pdev->dev, "%s : pci_map_rom FAILED", __func__);
> > + ret = -ENOMEM;
> > + goto err_pci_map;
> > + }
> > + dev_dbg(&pdev->dev, "%s : "
> > + "pci_map_rom SUCCESS and value in "
> > + "pch_phub_extrom_base_address variable is 0x%08x\n", __func__,
> > + (unsigned int)pch_phub_reg.pch_phub_extrom_base_address);
> > +
> > + retval = sysfs_create_group(&pdev->dev.kobj, &pch_attr_group);
> > + if (retval)
> > + goto err_sysfs_create;
> > +
> > + retval = sysfs_create_bin_file(&pdev->dev.kobj, &pch_bin_attr);
> > + if (retval)
> > + goto exit_bin_attr;
> > +
> > + return 0;
> > +
> > + pch_phub_read_modify_write_reg((unsigned int)CLKCFG_REG_OFFSET,
> > + CLKCFG_CAN_50MHZ, CLKCFG_CANCLK_MASK);
> > +
> > + /* set the prefech value */
> > + iowrite32(0x000ffffa, pch_phub_reg.pch_phub_base_address + 0x14);
> > + /* set the interrupt delay value */
> > + iowrite32(0x25, pch_phub_reg.pch_phub_base_address + 0x44);
> > +
> > + return 0;
> > +exit_bin_attr:
> > + sysfs_remove_group(&pdev->dev.kobj, &pch_attr_group);
> > +
> > +err_sysfs_create:
> > + pci_unmap_rom(pdev, pch_phub_reg.pch_phub_extrom_base_address);
> > +err_pci_map:
> > + pci_iounmap(pdev, pch_phub_reg.pch_phub_base_address);
> > +err_pci_iomap:
> > + pci_release_regions(pdev);
> > +err_req_regions:
> > + pci_disable_device(pdev);
> > +err_pci_enable_dev:
> > + dev_err(&pdev->dev, "%s returns %d\n", __func__, ret);
> > + return ret;
> > +}
> > +
> > +static void __devexit pch_phub_remove(struct pci_dev *pdev)
> > +{
> > + pci_unmap_rom(pdev, pch_phub_reg.pch_phub_extrom_base_address);
> > + pci_iounmap(pdev, pch_phub_reg.pch_phub_base_address);
> > + pci_release_regions(pdev);
> > + pci_disable_device(pdev);
> > + sysfs_remove_bin_file(&pdev->dev.kobj, &pch_bin_attr);
> > + sysfs_remove_group(&pdev->dev.kobj, &pch_attr_group);
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +
> > +static int pch_phub_suspend(struct pci_dev *pdev, pm_message_t state)
> > +{
> > + int ret;
> > +
> > + pch_phub_reg.pch_phub_suspended = true;/* For blocking further IOCTLs */
> > + pch_phub_save_reg_conf(pdev);
> > + ret = pci_save_state(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + " %s -pci_save_state returns %d\n", __func__, ret);
> > + return ret;
> > + }
> > + pci_enable_wake(pdev, PCI_D3hot, 0);
> > + pci_disable_device(pdev);
> > + pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > +
> > + return 0;
> > +}
> > +
> > +static int pch_phub_resume(struct pci_dev *pdev)
> > +{
> > + int ret;
> > +
> > + pci_set_power_state(pdev, PCI_D0);
> > + pci_restore_state(pdev);
> > + ret = pci_enable_device(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "%s-pci_enable_device failed(ret=%d) ", __func__, ret);
> > + return ret;
> > + }
> > +
> > + pci_enable_wake(pdev, PCI_D3hot, 0);
> > + pch_phub_restore_reg_conf(pdev);
> > + pch_phub_reg.pch_phub_suspended = false;
> > +
> > + return 0;
> > +}
> > +#else
> > +#define pch_phub_suspend NULL
> > +#define pch_phub_resume NULL
> > +#endif /* CONFIG_PM */
> > +
> > +static struct pci_device_id pch_phub_pcidev_id[] = {
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH1_PHUB)},
> > + {0,}
> > +};
> > +
> > +static struct pci_driver pch_phub_driver = {
> > + .name = "pch_phub",
> > + .id_table = pch_phub_pcidev_id,
> > + .probe = pch_phub_probe,
> > + .remove = __devexit_p(pch_phub_remove),
> > + .suspend = pch_phub_suspend,
> > + .resume = pch_phub_resume
> > +};
> > +
> > +static int __init pch_phub_pci_init(void)
> > +{
> > + int ret;
> > + ret = pci_register_driver(&pch_phub_driver);
> > +
> > + return ret;
>
> How about:
> return pci_register_driver(&pch_phub_driver);
> instead?
>
>
> > +}
> > +
> > +static void __exit pch_phub_pci_exit(void)
> > +{
> > + pci_unregister_driver(&pch_phub_driver);
> > +}
> > +
> > +module_init(pch_phub_pci_init);
> > +module_exit(pch_phub_pci_exit);
> > +
> > +MODULE_DESCRIPTION("PCH PACKET HUB PCI Driver");
>
> Why is "PACKET" and "HUB" all uppercase?
>
Thanks, Ohtake(OKISEMI)