Message-ID: <000b01cb126b$70d7e9b0$66f8800a@maildom.okisemi.com>
Date:	Wed, 23 Jun 2010 09:31:31 +0900
From:	"Masayuki Ohtake" <masa-korg@....okisemi.com>
To:	"Andrew Morton" <akpm@...ux-foundation.org>
Cc:	"Arnd Bergmann" <arnd@...db.de>,
	"Wang, Yong Y" <yong.y.wang@...el.com>, <qi.wang@...el.com>,
	<joel.clark@...el.com>, <andrew.chih.howe.khor@...el.com>,
	"Alan Cox" <alan@...rguk.ukuu.org.uk>,
	"LKML" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver

Hi Andrew

Thank you for your comments.
We will resubmit modified our patch by tomorrow or day after tomorrow at the latest.

Thanks.
Ohtake
----- Original Message ----- 
From: "Andrew Morton" <akpm@...ux-foundation.org>
To: "Masayuki Ohtak" <masa-korg@....okisemi.com>
Cc: "Arnd Bergmann" <arnd@...db.de>; "Wang, Yong Y" <yong.y.wang@...el.com>; <qi.wang@...el.com>;
<joel.clark@...el.com>; <andrew.chih.howe.khor@...el.com>; "Alan Cox" <alan@...rguk.ukuu.org.uk>; "LKML"
<linux-kernel@...r.kernel.org>
Sent: Wednesday, June 23, 2010 7:12 AM
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver


> On Tue, 22 Jun 2010 19:33:34 +0900
> Masayuki Ohtak <masa-korg@....okisemi.com> wrote:
>
> > Hi Arnd and Yong Y
> >
> > We have updated phub patch about the following.
> >  * Arnd's commnets
> > * Delete PCH_READ_REG/PCH_WRITE_REG
> > * Delete '_t' prefix
> > * Modify basic type
> > * Delete needless 'static' prefix
> > * Modify returned value
> > * Care returned value of get_user()
> > * Add .llseek line
> >
> >  * Yong Y's comments
> > * Applying to the latest checkpatch(2.6.35)
> > * Delete unused 'DEBUG' macro in Makefile
> > * Delete IEEE1588 lines
> > * Delete 'PCH_CAN_PCLK_50MHZ'
> >
> > Thanks, Ohtake.
> >
> > Kernel=2.6.33.1
> > Signed-off-by: Masayuki Ohtake <masa-korg@....okisemi.com>
>
> Please prepare a proper changelog for the patch - one which is suitabe
> for inclusion in the kernel's permanent git record.  Include that
> changelog with each version of the patch, perhaps after alterations.
>
> Please describe the module parameters in that changelog.  Please
> describe the major/minor number handling within the changelog -
> permitting the major number to be specified on the modprobe command
> line is unusual and should be fully described and justified, please.
>
> Please ensure that the changelog tells us what the driver actually
> does!  I don't even know what a "PCH packet hub" _is_ :(
>
> >
> > ...
> >
> > --- /dev/null
> > +++ b/drivers/char/pch_phub/pch_phub.c
> > @@ -0,0 +1,937 @@
> > +/*!
> > + * @file pch_phub.c
> > + * @brief Provides all the implementation of the interfaces pertaining to
> > + * the Packet Hub module.
> > + * @version 1.0.0.0
> > + * @section
>
> This appears to use a markup format whcih the kernel doesn't implement.
>
> > + * 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.
> > + */
> >
> > ...
> >
> > +/** pch_phub_read_modify_write_reg - Implements the functionality of reading,
> > + * modifying and writing register.
> > + *  @reg_addr_offset:  Contains the register offset address value
> > + *  @data:             Contains the writing value
> > + *  @mask:             Contains the mask value
> > + */
>
> kerneldoc comments are usually formatted as
>
> /**
>  *  foo() - do something
>  *  @arg1 ...
>
> I don't know whether the layout whcih this driver uses will be properly
> handled by the kerneldoc tools, but I'd suggest that it all be
> converted to the more usual style for consistency.
>
> > +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);
> > + return;
> > +}
> > +
> > +
> > +/** pch_phub_save_reg_conf - saves register configuration
> > + */
>
> The driver has quite a lot of comments which use the kerneldoc toke
> "/**" but which don't really look like they wre intended to be
> kerneldoc comments.  So I'd suggest converting these to plain old comments:
>
> /*
>  * pch_phub_save_reg_conf - saves register configuration
>  */
>
> or
>
> /* pch_phub_save_reg_conf - saves register configuration */
>
>
> or finish off the kerneldoc work by documenting the arguments (and the
> return values, please).
>
> > +static void pch_phub_save_reg_conf(struct pci_dev *pdev)
> > +{
> > + unsigned int i = 0;
> > + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> > +
> > + dev_dbg(&pdev->dev, "pch_phub_save_reg_conf ENTRY\n");
> > + /* to store contents of PHUB_ID register */
> > + pch_phub_reg.phub_id_reg = ioread32(p + PCH_PHUB_PHUB_ID_REG);
> > + /* to store contents of QUEUE_PRI_VAL register */
> > + pch_phub_reg.q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
> > + /* to store contents of RC_QUEUE_MAXSIZE register */
> > + pch_phub_reg.rc_q_maxsize_reg =
> > +     ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
> > + /* to store contents of BRI_QUEUE_MAXSIZE register */
> > + pch_phub_reg.bri_q_maxsize_reg =
> > +     ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
> > + /* to store contents of COMP_RESP_TIMEOUT register */
> > + pch_phub_reg.comp_resp_timeout_reg =
> > +     ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
> > + /* to store contents of BUS_SLAVE_CONTROL_REG register */
> > + pch_phub_reg.bus_slave_control_reg =
> > +     ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
> > + /* to store contents of DEADLOCK_AVOID_TYPE register */
> > + pch_phub_reg.deadlock_avoid_type_reg =
> > +     ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
> > + /* to store contents of INTPIN_REG_WPERMIT register 0 */
> > + pch_phub_reg.intpin_reg_wpermit_reg0 =
> > +     ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
> > + /* to store contents of INTPIN_REG_WPERMIT register 1 */
> > + pch_phub_reg.intpin_reg_wpermit_reg1 =
> > +     ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
> > + /* to store contents of INTPIN_REG_WPERMIT register 2 */
> > + pch_phub_reg.intpin_reg_wpermit_reg2 =
> > +     ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
> > + /* to store contents of INTPIN_REG_WPERMIT register 3 */
>
> s/to //g
>
> > + pch_phub_reg.intpin_reg_wpermit_reg3 =
> > +     ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
> > + dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
> > + "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",
> > + 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);
> > + /* to store contents of INT_REDUCE_CONTROL registers */
> > + 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, "pch_phub_save_reg_conf : "
> > + "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
> > + i, pch_phub_reg.int_reduce_control_reg[i]);
> > + }
> > + /* save clk cfg register */
> > + pch_phub_reg.clkcfg_reg = ioread32(p + CLKCFG_REG_OFFSET);
> > +
> > + return;
> > +}
> > +
> >
> > ...
> >
> > +static void pch_phub_restore_reg_conf(struct pci_dev *pdev)
> > +{
> > + unsigned int i;
> > + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> > +
> > + dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n");
> > + /* to store contents of PHUB_ID register */
> > + iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG);
> > + /* to store contents of QUEUE_PRI_VAL register */
> > + iowrite32(pch_phub_reg.q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
> > + /* to store contents of RC_QUEUE_MAXSIZE register */
> > + iowrite32(pch_phub_reg.rc_q_maxsize_reg,
> > + p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
> > + /* to store contents of BRI_QUEUE_MAXSIZE register */
> > + iowrite32(pch_phub_reg.bri_q_maxsize_reg,
> > + p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
> > + /* to store contents of COMP_RESP_TIMEOUT register */
> > + iowrite32(pch_phub_reg.comp_resp_timeout_reg,
> > + p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
> > + /* to store contents of BUS_SLAVE_CONTROL_REG register */
> > + iowrite32(pch_phub_reg.bus_slave_control_reg,
> > + p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
> > + /* to store contents of DEADLOCK_AVOID_TYPE register */
> > + iowrite32(pch_phub_reg.deadlock_avoid_type_reg,
> > + p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
> > + /* to store contents of INTPIN_REG_WPERMIT register 0 */
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg0,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
> > + /* to store contents of INTPIN_REG_WPERMIT register 1 */
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg1,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
> > + /* to store contents of INTPIN_REG_WPERMIT register 2 */
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg2,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
> > + /* to store contents of INTPIN_REG_WPERMIT register 3 */
> > + iowrite32(pch_phub_reg.intpin_reg_wpermit_reg3,
> > + p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
> > + dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
> > + "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",
> > + 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);
> > + /* to store contents of INT_REDUCE_CONTROL register */
> > + 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, "pch_phub_save_reg_conf : "
> > + "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
> > + i, pch_phub_reg.int_reduce_control_reg[i]);
> > + }
> > +
> > + /*restore the clock config reg */
>
> One space after the /*, please.
>
> > + iowrite32(pch_phub_reg.clkcfg_reg, p + CLKCFG_REG_OFFSET);
> > +
> > + return;
> > +}
> > +
> > +/** pch_phub_read_serial_rom - Implements the functionality of reading Serial
> > + * ROM.
> > + *  @offset_address: Contains the Serial ROM address offset value
> > + *  @data: Contains the Serial ROM value
> > + */
> > +static int pch_phub_read_serial_rom(unsigned int offset_address,
> > + unsigned char *data)
> > +{
> > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\
> > + offset_address;
>
> Unneeded \.  There are several instances of this in the driver.
>
> > + *data = ioread8(mem_addr);
> > +
> > + return 0;
> > +}
> > +
> > +/** pch_phub_write_serial_rom - Implements the functionality of writing Serial
> > + * ROM.
>
> Strange layout.  I don't know what this will look like after kerneldoc
> processing - it might make a mess.  Conventional layout is
>
> /**
>  * pch_phub_write_serial_rom - Implements the functionality of writing Serial ROM.
>
> or
>
> /**
>  * pch_phub_write_serial_rom - Implements the functionality of writing Serial
>  * ROM.
>
> (there are several instances of this).
>
>
> > + *  @offset_address: Contains the Serial ROM address offset value
> > + *  @data: Contains the Serial ROM value
> > + */
> > +static int pch_phub_write_serial_rom(unsigned int offset_address,
> > + unsigned char data)
> > +{
> > + int retval = 0;
> > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\
> > + (offset_address & 0xFFFFFFFC);
> > + int i = 0;
> > + unsigned int word_data = 0;
> > +
> > + iowrite32(PCH_PHUB_ROM_WRITE_ENABLE,
> > + pch_phub_reg.pch_phub_extrom_base_address +\
> > + PHUB_CONTROL);
> > +
> > + word_data = ioread32(mem_addr);
>
> The driver tends to do
>
> int foo = 0;
>
> ...
>
> foo = ...
>
> in quite a lot of places.  The initialisation to zero is harmless - the
> compiler will take it out again.  But it's unusual and jsut adds noise.
>
> The compiler will warn about use of uninitialised variables anyway.  In
> fact this unnecessary initalisation will suppress compiler warnings
> which might have revealed real bugs.  I'd suggest that they all be
> removed (there are quite a lot in this driver).
>
> > + switch (offset_address % 4) {
> > + case 0:
> > + word_data &= 0xFFFFFF00;
> > + iowrite32((word_data | (unsigned int)data),
> > + mem_addr);
> > + break;
> > + case 1:
> > + word_data &= 0xFFFF00FF;
> > + iowrite32((word_data | ((unsigned int)data << 8)),
> > + mem_addr);
> > + break;
> > + case 2:
> > + word_data &= 0xFF00FFFF;
> > + iowrite32((word_data | ((unsigned int)data << 16)),
> > + mem_addr);
> > + break;
> > + case 3:
> > + word_data &= 0x00FFFFFF;
> > + iowrite32((word_data | ((unsigned int)data << 24)),
> > + mem_addr);
> > + break;
> > + }
> > + while (0x00 !=
> > +        ioread8(pch_phub_reg.pch_phub_extrom_base_address +\
> > + PHUB_STATUS)) {
> > + msleep(1);
> > + if (PHUB_TIMEOUT == i) {
> > + retval = -EPERM;
> > + break;
> > + }
> > + i++;
> > + }
> > +
> > + iowrite32(PCH_PHUB_ROM_WRITE_DISABLE,
> > + pch_phub_reg.pch_phub_extrom_base_address +\
> > + PHUB_CONTROL);
> > +
> > + return retval;
> > +}
> > +
> >
> > ...
> >
> > +static ssize_t pch_phub_read(struct file *file, char __user *buf, size_t size,
> > + loff_t *ppos)
> > +{
> > + unsigned int rom_signature = 0;
> > + unsigned char rom_length;
> > + int ret_value;
> > + unsigned int tmp;
> > + unsigned char data;
> > + unsigned int addr_offset = 0;
> > + unsigned int orom_size;
> > + loff_t pos = *ppos;
> > +
> > + if (pos < 0)
> > + return -EINVAL;
>
> I think vfs_read() -> rw_verify_area() already did that, so indovidual
> ->read() implementations don't need to check for negative offset.
>
> > + /*Get Rom signature*/
>
> Spaces after /* and before */, please.
>
> > + pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature);
> > + 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 < pos)
> > + return 0;
> > +
> > + for (addr_offset = 0; addr_offset < size; addr_offset++) {
> > + pch_phub_read_serial_rom(0x80 + addr_offset + pos,
> > + &data);
> > + ret_value = copy_to_user(&buf[addr_offset], &data, 1);
> > + if (ret_value)
> > + return -EFAULT;
> > +
> > + if (orom_size < pos + addr_offset) {
> > + *ppos += addr_offset;
> > + return addr_offset;
> > + }
> > +
> > + }
> > + } else {
> > + return -ENOEXEC;
>
> ENOEXEC means "your executable file doesn't have suitable contents".
> If this error gets propagated to userspace the poor user will be
> wondering who corrupted his ELF files and would be surprised to find
> out that the error in fact came from his packethub driver.
>
> So please use a more appropriate errno here.
>
> > + }
> > + *ppos += addr_offset;
> > + return addr_offset;
> > +}
> > +
> > +/** pch_phub_write - Implements the write functionality of the Packet Hub
> > + * module.
> > + *  @file: Contains the reference of the file structure
> > + *  @buf: Usermode buffer pointer
> > + *  @size: Usermode buffer size
> > + *  @ppos: Contains the reference of the file structure
> > + */
> > +static ssize_t pch_phub_write(struct file *file, const char __user *buf,
> > + size_t size, loff_t *ppos)
> > +{
> > + unsigned int data;
> > + int ret_value;
> > + unsigned int addr_offset = 0;
> > + loff_t pos = *ppos;
> > +
> > + if (pos < 0)
> > + return -EINVAL;
>
> vfs_write() already did that.
>
> > + for (addr_offset = 0; addr_offset < size; addr_offset++) {
> > + ret_value = get_user(data, &buf[addr_offset]);
> > + if (ret_value)
> > + return -EFAULT;
> > +
> > + ret_value = pch_phub_write_serial_rom(0x80 + addr_offset + pos,
> > + data);
> > + if (ret_value)
> > + return -EIO;
>
> This overwrites the pch_phub_write_serial_rom() return value.  it's
> generally better to propagate error return values instead of replacing
> them with different ones.
>
> otoh pch_phub_write_serial_rom() can return -EPERM.  "Operation not
> permitted.  An attempt was made to perform an operation limited to
> processes with appropriate privileges or to the owner of a file or
> other resource.".  That doesn't sound like an appropriate errno for a
> driver to use?
>
> > + if (PCH_PHUB_OROM_SIZE < pos + addr_offset) {
> > + *ppos += addr_offset;
> > + return addr_offset;
> > + }
> > +
> > + }
> > +
> > + *ppos += addr_offset;
> > + return addr_offset;
> > +}
> > +
> > +
> > +/** pch_phub_ioctl - Implements the various ioctl functionalities of the Packet
> > + * Hub module.
> > + *  @inode: Contains the reference of the inode structure
> > + *  @file: Contains the reference of the file structure
> > + *  @cmd: Contains the command value
> > + *  @arg: Contains the command argument value
> > + */
> > +
> > +
> > +static long pch_phub_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + int ret_value = 0;
> > + __u8 mac_addr[6];
> > + int ret;
> > + unsigned int i;
> > + void __user *varg = (void __user *)arg;
> > +
> > + ret = mutex_lock_interruptible(&pch_phub_ioctl_mutex);
> > + if (ret) {
> > + ret_value = -ERESTARTSYS;
> > + goto return_nomutex;
> > + }
>
> A plain old `return -ERESTARTSYS' is OK here.
>
> > + if (pch_phub_reg.pch_phub_suspended == true) {
> > + ret_value = -EPERM;
>
> EPERM?
>
> > + goto return_ioctrl;
> > + }
> > +
> > + switch (cmd) {
> > + case IOCTL_PHUB_READ_MAC_ADDR:
> > + for (i = 0; i < 6; i++)
> > + pch_phub_read_gbe_mac_addr(i, &mac_addr[i]);
> > +
> > + ret_value = copy_to_user(varg,
> > + mac_addr, sizeof(mac_addr));
> > + if (ret_value) {
> > + ret_value = -EFAULT;
> > + goto return_ioctrl;
> > + }
>
> The goto is unneeded.
>
> > + break;
> > +
> > + case IOCTL_PHUB_WRITE_MAC_ADDR:
> > + ret_value = copy_from_user(mac_addr, varg, sizeof(mac_addr));
> > +
> > + if (ret_value) {
> > + ret_value = -EFAULT;
> > + goto return_ioctrl;
> > + }
>
> Could do a `break'.
>
> > + for (i = 0; i < 6; i++)
> > + pch_phub_write_gbe_mac_addr(i, mac_addr[i]);
> > + break;
> > +
> > + default:
> > + ret_value = -EINVAL;
> > + break;
> > + }
> > +return_ioctrl:
> > + mutex_unlock(&pch_phub_ioctl_mutex);
> > +return_nomutex:
> > + return ret_value;
> > +}
> > +
> > +
> > +/**
> > + * file_operations structure initialization
> > + */
>
> That's not a kerneldoc comment, but it has the /** kerneldoc token.
>
> > +static const struct file_operations pch_phub_fops = {
> > + .owner = THIS_MODULE,
> > + .read = pch_phub_read,
> > + .write = pch_phub_write,
> > + .unlocked_ioctl = pch_phub_ioctl,
> > + .llseek = default_llseek
> > +};
> > +
> > +
> > +/** pch_phub_probe - Implements the probe functionality of the module.
> > + *  @pdev: Contains the reference of the pci_dev structure
> > + *  @id: Contains the reference of the pci_device_id structure
> > + */
> > +static int __devinit pch_phub_probe(struct pci_dev *pdev,
> > +        const struct pci_device_id *id)
> > +{
> > + char *DRIVER_NAME = "pch_phub";
>
> hm, why was that upper case?
>
> Maybe use MODULE_NAME instead?  Or
>
> static const char driver_name[] = "pch_phub";
>
> > + int ret;
> > + unsigned int rom_size;
> > +
> > + pch_phub_major_no = (pch_phub_major_no < 0 || pch_phub_major_no > 254) ?
> > + 0 : pch_phub_major_no;
>
> This looks odd - should be explained in code comments and/or changelog.
>
> > + ret = pci_enable_device(pdev);
> > + if (ret) {
> > + dev_dbg(&pdev->dev,
> > + "\npch_phub_probe : pci_enable_device FAILED");
> > + goto err_probe;
> > + }
> > + dev_dbg(&pdev->dev, "pch_phub_probe : "
> > + "pci_enable_device returns %d\n", ret);
> > +
> > + ret = pci_request_regions(pdev, DRIVER_NAME);
> > + if (ret) {
> > + dev_dbg(&pdev->dev,
> > + "pch_phub_probe : pci_request_regions FAILED");
> > + pci_disable_device(pdev);
> > + goto err_probe;
> > + }
> > + dev_dbg(&pdev->dev, "pch_phub_probe : "
> > + "pci_request_regions returns %d\n", ret);
> > +
> > + pch_phub_reg.pch_phub_base_address = \
> > + (void __iomem *)pci_iomap(pdev, 1, 0);
>
> Unneeded and undesirable typeast.
>
> > + if (pch_phub_reg.pch_phub_base_address == 0) {
> > + dev_dbg(&pdev->dev, "pch_phub_probe : pci_iomap FAILED");
> > + pci_release_regions(pdev);
> > + pci_disable_device(pdev);
> > + ret = -ENOMEM;
> > + goto err_probe;
> > + }
> > + dev_dbg(&pdev->dev, "pch_phub_probe : pci_iomap SUCCESS and value "
> > + "in pch_phub_base_address variable is 0x%08x\n",
> > + (unsigned int)pch_phub_reg.pch_phub_base_address);
> > +
> > + pch_phub_reg.pch_phub_extrom_base_address =
> > +     (void __iomem *)pci_map_rom(pdev, &rom_size);
>
> Ditto
>
> > + if (pch_phub_reg.pch_phub_extrom_base_address == 0) {
> > + dev_dbg(&pdev->dev, "pch_phub_probe : pci_map_rom FAILED");
> > + pci_iounmap(pdev, (void *)pch_phub_reg.pch_phub_base_address);
> > + pci_release_regions(pdev);
> > + pci_disable_device(pdev);
>
> This is getting painful.  I'd suggest removal of the duplicated code
> via the standard `goto out_iounmap' unwinding approach.
>
> > + ret = -ENOMEM;
> > + goto err_probe;
> > + }
> > + dev_dbg(&pdev->dev, "pch_phub_probe : "
> > + "pci_map_rom SUCCESS and value in "
> > + "pch_phub_extrom_base_address variable is 0x%08x\n",
> > + (unsigned int)pch_phub_reg.pch_phub_extrom_base_address);
> > +
> > + if (pch_phub_major_no) {
> > + pch_phub_dev_no = MKDEV(pch_phub_major_no, 0);
> > + ret = register_chrdev_region(pch_phub_dev_no,
> > +    PCH_MINOR_NOS, DRIVER_NAME);
> > + if (ret) {
> > + dev_dbg(&pdev->dev, "pch_phub_probe : "
> > + "register_chrdev_region FAILED");
> > + pci_unmap_rom(pdev,
> > + (void *)pch_phub_reg.pch_phub_extrom_base_address);
> > + pci_iounmap(pdev,
> > + (void *)pch_phub_reg.pch_phub_base_address);
>
> More unneeded typecasts.
>
> > + pci_release_regions(pdev);
> > + pci_disable_device(pdev);
> > + goto err_probe;
> > + }
> > + dev_dbg(&pdev->dev, "pch_phub_probe : "
> > + "register_chrdev_region returns %d\n", ret);
> > + } else {
> > + ret = alloc_chrdev_region(&pch_phub_dev_no, 0,
> > + PCH_MINOR_NOS, DRIVER_NAME);
> > + if (ret) {
> > + dev_dbg(&pdev->dev, "pch_phub_probe : "
> > + "alloc_chrdev_region FAILED");
> > + pci_unmap_rom(pdev,
> > + (void *)pch_phub_reg.pch_phub_extrom_base_address);
> > + pci_iounmap(pdev,
> > +     (void *)pch_phub_reg.pch_phub_base_address);
>
> more..
>
> > + pci_release_regions(pdev);
> > + pci_disable_device(pdev);
> > + goto err_probe;
> > + }
> > + dev_dbg(&pdev->dev, "pch_phub_probe : "
> > + "alloc_chrdev_region returns %d\n", ret);
> > + }
> > +
> > + cdev_init(&pch_phub_dev, &pch_phub_fops);
> > + dev_dbg(&pdev->dev,
> > + "pch_phub_probe :  cdev_init invoked successfully\n");
> > +
> > + pch_phub_dev.owner = THIS_MODULE;
> > + pch_phub_dev.ops = &pch_phub_fops;
> > +
> > + ret = cdev_add(&pch_phub_dev, pch_phub_dev_no, PCH_MINOR_NOS);
> > + if (ret) {
> > + dev_dbg(&pdev->dev, "pch_phub_probe :  cdev_add FAILED");
> > + unregister_chrdev_region(pch_phub_dev_no, PCH_MINOR_NOS);
> > + pci_unmap_rom(pdev,
> > + (void *)pch_phub_reg.pch_phub_extrom_base_address);
> > + pci_iounmap(pdev, (void *)pch_phub_reg.pch_phub_base_address);
> > + pci_release_regions(pdev);
> > + pci_disable_device(pdev);
> > + goto err_probe;
> > + }
> > + dev_dbg(&pdev->dev, "pch_phub_probe :  cdev_add returns %d\n", ret);
> > +
> > + /*set the clock config reg if CAN clock is 50Mhz */
> > + dev_dbg(&pdev->dev, "pch_phub_probe : invoking "
> > + "pch_phub_read_modify_write_reg "
> > + "to set CLKCFG reg for CAN clk 50Mhz\n");
> > + 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;
> > +
> > +err_probe:
> > + dev_dbg(&pdev->dev, "pch_phub_probe returns %d\n", ret);
> > + return ret;
> > +}
> > +
> >
> > ...
> >
> > +#ifdef CONFIG_PM
> > +
> > +/** pch_phub_suspend - Implements the suspend functionality of the module.
> > + *  @pdev: Contains the reference of the pci_dev structure
> > + *  @state: Contains the reference of the pm_message_t structure
> > + */
> > +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);
> > + dev_dbg(&pdev->dev, "pch_phub_suspend - "
> > + "pch_phub_save_reg_conf Invoked successfully\n");
> > +
> > + ret = pci_save_state(pdev);
> > + if (ret) {
> > + dev_dbg(&pdev->dev,
> > + " pch_phub_suspend -pci_save_state returns-%d\n", ret);
> > + return ret;
> > + }
> > + dev_dbg(&pdev->dev,
> > + "pch_phub_suspend - pci_save_state returns %d\n", ret);
> > + pci_enable_wake(pdev, PCI_D3hot, 0);
> > + dev_dbg(&pdev->dev, "pch_phub_suspend - "
> > + "pci_enable_wake Invoked successfully\n");
> > +
> > + pci_disable_device(pdev);
> > + dev_dbg(&pdev->dev, "pch_phub_suspend - "
> > + "pci_disable_device Invoked successfully\n");
> > +
> > + pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > + dev_dbg(&pdev->dev, "pch_phub_suspend - "
> > + "pci_set_power_state Invoked successfully   "
> > + "return = %d\n", 0);
> > +
> > + return 0;
> > +}
> > +
> > +/** pch_phub_resume - Implements the resume functionality of the module.
> > + *  @pdev: Contains the reference of the pci_dev structure
> > + */
> > +static int pch_phub_resume(struct pci_dev *pdev)
> > +{
> > +
> > + int ret;
> > +
> > + pci_set_power_state(pdev, PCI_D0);
> > + dev_dbg(&pdev->dev, "pch_phub_resume - "
> > + "pci_set_power_state Invoked successfully\n");
> > +
> > + pci_restore_state(pdev);
> > + dev_dbg(&pdev->dev, "pch_phub_resume - "
> > + "pci_restore_state Invoked successfully\n");
> > +
> > + ret = pci_enable_device(pdev);
> > + if (ret) {
> > + dev_dbg(&pdev->dev,
> > + "pch_phub_resume-pci_enable_device failed ");
> > + return ret;
> > + }
> > +
> > + dev_dbg(&pdev->dev, "pch_phub_resume - "
> > + "pci_enable_device returns -%d\n", ret);
> > +
> > + pci_enable_wake(pdev, PCI_D3hot, 0);
> > + dev_dbg(&pdev->dev, "pch_phub_resume - "
> > + "pci_enable_wake Invoked successfully\n");
> > +
> > + pch_phub_restore_reg_conf(pdev);
> > + dev_dbg(&pdev->dev, "pch_phub_resume - "
> > + "pch_phub_restore_reg_conf Invoked successfully\n");
> > +
> > + pch_phub_reg.pch_phub_suspended = false;
> > +
> > + dev_dbg(&pdev->dev, "pch_phub_resume  returns- %d\n", 0);
> > + return 0;
> > +}
>
> Here you can put
>
> #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),
> > +#ifdef CONFIG_PM
> > + .suspend = pch_phub_suspend,
> > + .resume = pch_phub_resume
> > +#endif
>
> and then remove this ifdef.
>
> >
> > ...
> >
> > --- /dev/null
> > +++ b/drivers/char/pch_phub/pch_phub.h
> > @@ -0,0 +1,58 @@
> > +#ifndef __PCH_PHUB_H__
> > +#define __PCH_PHUB_H__
> > +/*!
> > + * @file pch_phub.h
> > + * @brief Provides all the interfaces pertaining to the Packet Hub module.
> > + * @version 1.0.0.0
> > + * @section
>
> ?
>
> > + * 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.
> > + */
> > +
> > +/*
> > + * History:
> > + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
> > + *
> > + * created:
> > + * OKI SEMICONDUCTOR 04/14/2010
> > + * modified:
> > + *
> > + */
> > +
> > +#define PHUB_IOCTL_MAGIC (0xf7)
> > +
> > +/*Outlines the read mac address function signature. */
>
> Space after /*, please (check the whole patch)
>
> > +#define IOCTL_PHUB_READ_MAC_ADDR (_IOR(PHUB_IOCTL_MAGIC, 6, __u8[6]))
> > +
> > +/*brief Outlines the write mac address function signature. */
> > +#define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 7, __u8[6]))
>
> "brief"?
>
> I didn't notice any documentation for these ioctls anywhere?
>
> > +
> > +/* Registers address offset */
> > +#define PCH_PHUB_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
> > +
> > +#endif
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists