Hi Jean,
> On Tue, 2012-12-04 at 16:23 -0700, Brown, Bill E wrote:
> -----Original Message-----
> From: Jean Delvare [mailto:[email protected]]
> Sent: Sunday, September 30, 2012 1:45 PM
> To: Brown, Bill E
> Cc: [email protected]
> Subject: Re: [PATCH v2] i2c: Adding support for Intel iSMT SMBus 2.0 host
> controller
>
> Hi Bill,
>
> On Thu, 6 Sep 2012 16:32:58 -0700, Bill Brown wrote:
> > The iSMT (Intel SMBus Message Transport) supports multi-master
> > I2C/SMBus, as well as IPMI. It's operation is DMA-based and utilizes
> > descriptors to initiate transactions on the bus.
> >
> > The iSMT hardware can act as both a master and a target, although this
> > driver only supports being a master.
> >
> > Signed-off-by: Bill Brown <[email protected]>
> > ---
> > drivers/i2c/busses/Kconfig | 10 +
> > drivers/i2c/busses/Makefile | 1 +
> > drivers/i2c/busses/i2c-ismt.c | 845
> > +++++++++++++++++++++++++++++++++++++++++
> > drivers/i2c/busses/i2c-ismt.h | 111 ++++++
> > 4 files changed, 967 insertions(+), 0 deletions(-) create mode
> > 100644 drivers/i2c/busses/i2c-ismt.c create mode 100644
> > drivers/i2c/busses/i2c-ismt.h
>
> Here's my review, sorry for the delay. The driver looks overall good and most
> of my comments are suggestions for improvements, either for code readability
> or performance. For these it's up to you if you want to follow my advice or
> not.
>
> There are a few real bugs too though, which you will _have_ to fix. If you're
> quick we can be in time for kernel 3.7.
>
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 7244c8b..64d5756 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -119,6 +119,16 @@ config I2C_ISCH
> > This driver can also be built as a module. If so, the module
> > will be called i2c-isch.
> >
> > +config I2C_ISMT
> > + tristate "Intel iSMT SMBus Controller"
> > + depends on PCI
> > + help
> > + If you say yes to this option, support will be included for the Intel
> > + iSMT SMBus host controller interface.
> > +
> > + This driver can also be built as a module. If so, the module will be
> > + called i2c-ismt.
> > +
> > config I2C_PIIX4
> > tristate "Intel PIIX4 and compatible
> > (ATI/AMD/Serverworks/Broadcom/SMSC)"
> > depends on PCI
> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index ce3c2be..694711a 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -22,6 +22,7 @@ obj-$(CONFIG_I2C_SIS630) += i2c-sis630.o
> > obj-$(CONFIG_I2C_SIS96X) += i2c-sis96x.o
> > obj-$(CONFIG_I2C_VIA) += i2c-via.o
> > obj-$(CONFIG_I2C_VIAPRO) += i2c-viapro.o
> > +obj-$(CONFIG_I2C_ISMT) += i2c-ismt.o
>
> In alphabetical order please.
done
>
> >
> > # Mac SMBus host controller drivers
> > obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
> > diff --git a/drivers/i2c/busses/i2c-ismt.c
> > b/drivers/i2c/busses/i2c-ismt.c new file mode 100644 index
> > 0000000..46df042
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-ismt.c
> > @@ -0,0 +1,845 @@
> > +/*
> > + * This file is provided under a dual BSD/GPLv2 license. When using
> > +or
> > + * redistributing this file, you may do so under either license.
> > + *
> > + * GPL LICENSE SUMMARY
> > + *
> > + * Copyright(c) 2012 Intel Corporation. All rights reserved.
>
> Copyright is independent from licensing, so please move the copyright
> statement outside of the license blocks. This will avoid duplicating the
> copyright statement.
>
done
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + modify
> > + * it under the terms of version 2 of the GNU General Public License
> > + as
> > + * published by the Free Software Foundation.
> > + *
> > + * 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., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301
> > USA.
> > + * The full GNU General Public License is included in this
> > + distribution
> > + * in the file called LICENSE.GPL.
> > + *
> > + * BSD LICENSE
> > + *
> > + * Copyright(c) 2012 Intel Corporation. All rights reserved.
> > + * All rights reserved.
>
> Very reserved it seems ;)
Not by my choice... :)
>
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + * * Redistributions of source code must retain the above copyright
> > + * notice, this list of conditions and the following disclaimer.
> > + * * Redistributions in binary form must reproduce the above copyright
> > + * notice, this list of conditions and the following disclaimer in
> > + * the documentation and/or other materials provided with the
> > + * distribution.
> > + * * Neither the name of Intel Corporation nor the names of its
> > + * contributors may be used to endorse or promote products derived
> > + * from this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> > + CONTRIBUTORS
> > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> > + FOR
> > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> > + COPYRIGHT
> > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> > + INCIDENTAL,
> > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> > + USE,
> > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> > + ANY
> > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> > + TORT
> > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
> > + USE
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +
> > +/*
> > + Supports the SMBus Message Transport (SMT) on the following Intel SOCs:
>
> Please use standard comment style for consistency (as above.)
done
>
> > + BWD
> > + CTN
>
> Very cryptic names. Can you use something more user-friendly?
done
>
> > +
> > + Features supported by this driver:
> > + Software PEC no
>
> Does the hardware support this at all? If not, there's no point in mentioning
> it.
The hardware supports it, so I there's no real reason to mention this.
>
> > + Hardware PEC yes
> > + Block buffer yes
> > + Block process call transaction no
>
> Same question here.
The SOC supports it, but the driver doesn't.
>
> > + Slave mode no
> > +*/
> > +
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/pci.h>
> > +#include <linux/kernel.h>
> > +#include <linux/stddef.h>
> > +#include <linux/delay.h>
>
> It doesn't look like you need this one. OTOH you need <linux/completion.h>,
> which is missing. Including <linux/dma-mapping.h> would probably be a good
> idea too.
done
>
> > +#include <linux/i2c.h>
> > +#include <linux/acpi.h>
> > +#include <linux/interrupt.h>
> > +#include "i2c-ismt.h"
> > +
> > +/**
> > + * DEFINE_PCI_DEVICE_TABLE - PCI device IDs supported by this driver
>
> You're describing ismt_ids not DEFINE_PCI_DEVICE_TABLE.
Oops, fixed. :)
>
> > + */
> > +static const DEFINE_PCI_DEVICE_TABLE(ismt_ids) = {
> > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BWD_SMBUS_SMT0) },
> > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BWD_SMBUS_SMT1) },
> > + { 0, }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(pci, ismt_ids);
> > +
> > +/* Master Descriptor control bits */
> > +static unsigned int stop_on_error = 1; module_param(stop_on_error,
> > +uint, S_IRUGO); MODULE_PARM_DESC(stop_on_error, "Stop on Error");
>
> I'd write "error" with no leading capital, for consistency.
>
> > +
> > +static unsigned int fair = 1;
> > +module_param(fair, uint, S_IRUGO);
> > +MODULE_PARM_DESC(fair, "Enable fairness on the SMBus");
>
> It would make sense to make these parameters writable by root in sysfs, so
> that the settings can be changed at run-time.
>
> You could also make these bools rather than uints, I think this gets you
> stricter input validation, and maybe some optimizations too.
>
> Both options would be good to document in Documentation/i2c/busses/i2c-ismt,
> as their effect isn't obvious. See
> i2c-i801 in this directory for the formatting. You don't have to be extra
> verbose if you don't have the time, just write the information that you think
> is useful for other users and developers.
I removed both parameters as they weren't really needed. I added a new
one though for changing the bus frequency and documented it as you
suggested. Refer to the #ifdef DEBUG_SLOW_HW comment you made below.
>
> > +
> > +#ifdef DEBUG
> > +/**
> > + * ismt_desc_dump() - dump the contents of a descriptor for debug
> > +purposes
> > + * @adap: the I2C host adapter
> > + */
> > +static void ismt_desc_dump(struct i2c_adapter *adap) {
> > + struct ismt_priv *priv = i2c_get_adapdata(adap);
>
> In this function and the 2 other debug functions below, you don't need adap,
> so you better pass priv as the function parameter.
done
>
> > + struct device *dev = &priv->pci_dev->dev;
> > + struct ismt_desc *desc = &priv->hw[priv->head];
> > +
> > + dev_dbg(dev, "Dump of the descriptor struct: 0x%X\n", priv->head);
> > + dev_dbg(dev, "\ttgtaddr_rw=0x%02X\n", desc->tgtaddr_rw);
> > + dev_dbg(dev, "\twr_len_cmd=0x%02X\n", desc->wr_len_cmd);
> > + dev_dbg(dev, "\trd_len= 0x%02X\n", desc->rd_len);
> > + dev_dbg(dev, "\tcontrol= 0x%02X\n", desc->control);
> > + dev_dbg(dev, "\tstatus= 0x%02X\n", desc->status);
> > + dev_dbg(dev, "\tretry= 0x%02X\n", desc->retry);
> > + dev_dbg(dev, "\trxbytes= 0x%02X\n", desc->rxbytes);
> > + dev_dbg(dev, "\ttxbytes= 0x%02X\n", desc->txbytes);
> > + dev_dbg(dev, "\tdptr_low= 0x%08X\n", desc->dptr_low);
> > + dev_dbg(dev, "\tdptr_high= 0x%08X\n", desc->dptr_high); }
> > +
> > +/**
> > + * ismt_gen_reg_dump() - dump the iSMT General Registers
> > + * @adap: the I2C host adapter
> > + */
> > +static void ismt_gen_reg_dump(struct i2c_adapter *adap) {
> > + struct ismt_priv *priv = i2c_get_adapdata(adap);
> > + struct device *dev = &priv->pci_dev->dev;
> > +
> > + dev_dbg(dev, "Dump of the iSMT General Registers\n");
> > + dev_dbg(dev, " GCTRL.... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_GR_GCTRL,
> > + readl(priv->smba + ISMT_GR_GCTRL));
> > + dev_dbg(dev, " SMTICL... : (0x%p)=0x%016lX\n",
> > + priv->smba + ISMT_GR_SMTICL,
> > + readq(priv->smba + ISMT_GR_SMTICL));
> > + dev_dbg(dev, " ERRINTMSK : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_GR_ERRINTMSK,
> > + readl(priv->smba + ISMT_GR_ERRINTMSK));
> > + dev_dbg(dev, " ERRAERMSK : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_GR_ERRAERMSK,
> > + readl(priv->smba + ISMT_GR_ERRAERMSK));
> > + dev_dbg(dev, " ERRSTS... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_GR_ERRSTS,
> > + readl(priv->smba + ISMT_GR_ERRSTS));
> > + dev_dbg(dev, " ERRINFO.. : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_GR_ERRINFO,
> > + readl(priv->smba + ISMT_GR_ERRINFO)); }
> > +
> > +/**
> > + * ismt_mstr_reg_dump() - dump the iSMT Master Registers
> > + * @adap: the I2C host adapter
> > + */
> > +static void ismt_mstr_reg_dump(struct i2c_adapter *adap) {
> > + struct ismt_priv *priv = i2c_get_adapdata(adap);
> > + struct device *dev = &priv->pci_dev->dev;
> > +
> > + dev_dbg(dev, "Dump of the iSMT Master Registers\n");
> > + dev_dbg(dev, " MDBA..... : (0x%p)=0x%016lX\n",
> > + priv->smba + ISMT_MSTR_MDBA,
> > + readq(priv->smba + ISMT_MSTR_MDBA));
> > + dev_dbg(dev, " MCTRL.... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_MSTR_MCTRL,
> > + readl(priv->smba + ISMT_MSTR_MCTRL));
> > + dev_dbg(dev, " MSTS..... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_MSTR_MSTS,
> > + readl(priv->smba + ISMT_MSTR_MSTS));
> > + dev_dbg(dev, " MDS...... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_MSTR_MDS,
> > + readl(priv->smba + ISMT_MSTR_MDS));
> > + dev_dbg(dev, " RPOLICY.. : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_MSTR_RPOLICY,
> > + readl(priv->smba + ISMT_MSTR_RPOLICY));
> > + dev_dbg(dev, " SPGT..... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_SPGT,
> > + readl(priv->smba + ISMT_SPGT));
> > +}
> > +
> > +#else
> > +static void ismt_desc_dump(struct i2c_adapter *adap) {} static void
> > +ismt_gen_reg_dump(struct i2c_adapter *adap) {} static void
> > +ismt_mstr_reg_dump(struct i2c_adapter *adap) {} #endif
>
> This prevents using the dynamic debug feature. The 3 debug functions do
> nothing by default if DEBUG isn't set, so I think they should depend on
> defined(DEBUG) || defined(CONFIG_DYNAMIC_DEBUG). That way they can be
> included but disabled by default under DYNAMIC_DEBUG, and you can enable
> specific debugging messages at run-time. Same is done in
> net/netfilter/nf_conntrack_pptp.c for example.
Good idea, done.
>
> > +
> > +/**
> > + * ismt_insert_cmd() - stuff a command into the head of the data
> > +buffer
>
> Extra space after dash.
fixed
>
> > + * @data: data buffer
> > + * @command: command to insert
> > + */
> > +static void ismt_insert_cmd(union i2c_smbus_data *data, u8 command) {
> > + memmove(&data->block[1], &data->block[0], I2C_SMBUS_BLOCK_MAX);
> > + data->block[0] = command;
>
> Ouch, no, you can't do that, sorry. This union is not under your full
> control. The caller must be able to re-use the same union with no change to
> issue the same SMBus transaction. As a matter of fact,
> i2c-core.c:i2c_smbus_xfer() will do exactly this if it detects arbitration
> loss.
>
> Obviously if you modify the contents like that, it will break. So, to put it
> short: if you need buffers for your own use, allocate them as part of your
> private data structure.
>
Ouch is right, I didn't know that. I fixed it by creating a temp buffer.
>
> As a side note, it may be faster anyway. The memmove() above had to move 32
> bytes one by one because the areas overlap, that was certainly slow.
>
> Also note that you're only supposed to access data->block for block
> transactions. For other transactions it's ->byte or ->word. You probably
> don't care much about endianness in this specific driver, but if you had to,
> the above would inevitably fail for word transactions on either endianness.
Noted. I don't have to worry about it here since this is x86 only.
>
> > +}
> > +
> > +/**
> > + * ismt_submit_desc() - add a descriptor to the ring
> > + * @adap: the i2c host adapter
> > + */
> > +static void ismt_submit_desc(struct i2c_adapter *adap) {
> > + int fmhp;
> > + int val;
>
> These should be unsigned, as you use them for readl()/writel().
fixed
>
> > + struct ismt_priv *priv = i2c_get_adapdata(adap);
> > +
> > + ismt_desc_dump(adap);
> > + ismt_gen_reg_dump(adap);
> > + ismt_mstr_reg_dump(adap);
>
> I fail to see the rationale for putting these debug calls here. In this
> function you don't need adap, so by moving the debug calls back to
> ismt_access(), you could pass priv directly as a parameter and save a few
> bytes.
I used this during debug to verify that the descriptor and the
controller are correct before I start a transaction (it came in quite
handy for me :) ).
Switched to passing priv as the parameter per your suggestion.
>
> > +
> > + /* Set the FMHP (Firmware Master Head Pointer)*/
> > + fmhp = ((priv->head + 1) % ISMT_DESC_ENTRIES) << 16;
> > + val = readl(priv->smba + ISMT_MSTR_MCTRL);
> > + writel((val & ~(ISMT_MCTRL_FMHP)) | fmhp,
> > + (priv->smba + ISMT_MSTR_MCTRL));
>
> Alignment is off by one, and parentheses around (ISMT_MCTRL_FMHP) and
> (priv->smba + ISMT_MSTR_MCTRL) aren't needed.
fixed
>
> > +
> > + /* Set the start bit */
> > + val = readl(priv->smba + ISMT_MSTR_MCTRL);
> > + writel((val | ISMT_MCTRL_SS),
> > + (priv->smba + ISMT_MSTR_MCTRL));
>
> Many unneeded parentheses here too.
fixed
>
> > +}
> > +
> > +/**
> > + * ismt_process_desc() - handle the completion of the descriptor
> > + * @adap: the i2c host adapter
> > + */
> > +static int ismt_process_desc(struct i2c_adapter *adap) {
> > + struct ismt_desc *desc;
> > + struct ismt_priv *priv = i2c_get_adapdata(adap);
>
> All you really need in this function is desc, and you have it at hand on the
> calling side. So just pass "desc" as the function parameter.
> This saves 32 binary bytes on x86_64. You can even make desc const as the
> function doesn't modify it.
done and done
>
> > +
> > + desc = &priv->hw[priv->head];
> > +
> > + if (desc->status & ISMT_DESC_SCS)
> > + return 0;
>
> We can probably assume that this is the most frequent case, and use
> likely() to help gcc optimize the most frequent path.
I just learned something new, done. :)
>
> > +
> > + if ((desc->status & ISMT_DESC_NAK) || (desc->status & ISMT_DESC_CRC))
> > + return -ENXIO;
>
> According to Documentation/i2c/fault-codes, CRC (PEC) errors are to be
> reported with -EBADMSG.
fixed
>
> NAK is probably the second most likely case, as it happens on device probing,
> so again likely() may be good to use.
done
>
> > +
> > + if (desc->status & ISMT_DESC_COL)
> > + return -EAGAIN;
> > +
> > + return 0;
>
> Can it really be a success if ISMT_DESC_SCS wasn't set? I doubt it.
Ugh, nice catch. Fixed and added some other failure cases.
>
> > +}
> > +
> > +/**
> > + * ismt_access() - process an SMBus command
> > + * @adap: the i2c host adapter
> > + * @addr: address of the i2c/SMBus target
> > + * @flags: command options
> > + * @read_write: read from or write to device
> > + * @command: the i2c/SMBus command to issue
> > + * @size: SMBus transaction type
> > + * @data: read/write data buffer
> > + */
> > +static int ismt_access(struct i2c_adapter *adap, u16 addr,
> > + unsigned short flags, char read_write, u8 command,
> > + int size, union i2c_smbus_data *data) {
> > + unsigned int ret = 0; /* return code */
>
> You won't catch (negative!) error codes with an unsigned int.
fixed
>
> The comment /* return code */ is pretty redundant, BTW, it's quite clear what
> this variable is for. And initialization isn't needed either.
I get a little verbose sometimes...fixed. :)
>
> > + dma_addr_t dma_addr = 0; /* address of the data buffer */
> > + u8 dma_size = 0;
> > + enum dma_data_direction dma_direction = 0;
> > + bool map_dma_flag = 0;
>
> Could you do without this flag? AFAICS map_dma_flag always evaluates to the
> same as dma_size != 0.
I looked at it, and you are right. I removed the flag and modified
accordingly.
>
> > + struct ismt_desc *desc;
> > + struct ismt_priv *priv = i2c_get_adapdata(adap);
>
> I'd suggest that you also declare:
> struct device *dev = &priv->pci_dev->dev; and use it in all debug and
> error messages, this will make the code much more readable!
That is much better, done.
>
> > +
> > + desc = &priv->hw[priv->head];
> > +
> > + /* Initialize the descriptor */
> > + memset(desc, 0, sizeof(struct ismt_desc));
> > + desc->tgtaddr_rw = ISMT_DESC_ADDR_RW(addr, read_write);
> > +
> > + /* Initialize common control bits */
> > + desc->control |= ISMT_DESC_INT;
>
> You just zeroed this field with memset(), so "=" will do the same as "|=",
> and is probably more efficient.
done
>
> > +
> > + if (stop_on_error)
> > + desc->control |= ISMT_DESC_SOE;
> > +
> > + if ((flags & I2C_CLIENT_PEC) && (size != I2C_SMBUS_QUICK)
> > + && (size != I2C_SMBUS_I2C_BLOCK_DATA))
>
> Please align the && on opening parenthesis to avoid confusion between the
> condition and the action.
done
>
> > + desc->control |= ISMT_DESC_PEC;
> > +
> > + if (fair)
> > + desc->control |= ISMT_DESC_FAIR;
> > +
> > + switch (size) {
> > + case I2C_SMBUS_QUICK:
> > + dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_QUICK\n");
> > + break;
> > +
> > + case I2C_SMBUS_BYTE:
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + /*
> > + * Send Byte
> > + * The command field contains the write data
> > + */
> > + dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE:
> > WRITE\n");
> > + desc->control |= ISMT_DESC_CWRL;
> > + desc->wr_len_cmd = command;
> > + } else {
> > + /* Receive Byte */
> > + dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE: READ\n");
> > + dma_size = 1;
> > + dma_direction = DMA_FROM_DEVICE;
> > + map_dma_flag = 1;
> > + desc->rd_len = 1;
> > + }
> > +
> > + break;
> > +
> > + case I2C_SMBUS_BYTE_DATA:
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + /*
> > + * Write Byte
> > + * Command plus 1 data byte
> > + */
> > + dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE_DATA:
> > WRITE\n");
> > + desc->wr_len_cmd = 2;
> > +
> > + /* Stuff the command ahead of the data in the buffer */
> > + ismt_insert_cmd(data, command);
> > + dma_size = 2;
> > + dma_direction = DMA_TO_DEVICE;
> > + } else {
> > + /* Read Byte */
> > + dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE_DATA:
> > READ\n");
> > + desc->control |= ISMT_DESC_CWRL;
> > + desc->wr_len_cmd = command;
> > + desc->rd_len = 1;
> > + dma_size = 1;
> > + dma_direction = DMA_FROM_DEVICE;
> > + }
> > +
> > + map_dma_flag = 1;
> > + break;
> > +
> > + case I2C_SMBUS_WORD_DATA:
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + /* Write Word */
> > + dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_WORD_DATA:
> > WRITE\n");
> > + desc->wr_len_cmd = 3;
> > +
> > + /* Stuff the command ahead of the data in the buffer */
> > + ismt_insert_cmd(data, command);
> > + dma_size = 3;
> > + dma_direction = DMA_TO_DEVICE;
> > + } else {
> > + /* Read Word */
> > + dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_WORD_DATA:
> > READ\n");
> > + desc->wr_len_cmd = command;
> > + desc->control |= ISMT_DESC_CWRL;
> > + desc->rd_len = 2;
> > + dma_size = 2;
> > + dma_direction = DMA_FROM_DEVICE;
> > + }
> > +
> > + map_dma_flag = 1;
> > + break;
> > +
> > + case I2C_SMBUS_PROC_CALL:
> > + dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_PROC_CALL\n");
> > + desc->wr_len_cmd = 3;
> > + desc->rd_len = 2;
> > +
> > + /* Stuff the command ahead of the data in the buffer */
> > + ismt_insert_cmd(data, command);
> > + dma_size = 3;
> > + dma_direction = DMA_BIDIRECTIONAL;
> > + map_dma_flag = 1;
> > + break;
> > +
> > + case I2C_SMBUS_BLOCK_DATA:
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + /* Block Write */
> > + dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BLOCK_DATA:
> > WRITE\n");
> > + dma_size = data->block[0] + 1;
> > + dma_direction = DMA_TO_DEVICE;
> > + desc->wr_len_cmd = dma_size;
> > + desc->control |= ISMT_DESC_BLK;
> > + ismt_insert_cmd(data, command);
> > + } else {
> > + /* Block Read */
> > + dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BLOCK_DATA:
> > READ\n");
> > + dma_size = data->block[0] + 1;
>
> Probably not. For SMBus block reads, the block length is _received_ from the
> slave, so at this point you don't know it. So you probably have to map the
> maximum (I2C_SMBUS_BLOCK_MAX == 32 bytes) always.
Yep, fixed.
>
> > + dma_direction = DMA_FROM_DEVICE;
> > + desc->rd_len = dma_size;
> > + desc->wr_len_cmd = command;
> > + desc->control |= (ISMT_DESC_BLK | ISMT_DESC_CWRL);
> > + }
> > +
> > + map_dma_flag = 1;
> > + break;
> > +
> > + default:
> > + dev_err(&priv->pci_dev->dev, "Unsupported transaction %d\n",
> > + size);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + /* map the data buffer */
> > + if (map_dma_flag) {
> > + dev_dbg(&priv->pci_dev->dev,
> > + " &priv->pci_dev->dev=%p\n", &priv->pci_dev->dev);
> > + dev_dbg(&priv->pci_dev->dev, " data=%p\n", data);
> > + dev_dbg(&priv->pci_dev->dev, " dma_size=%d\n", dma_size);
> > + dev_dbg(&priv->pci_dev->dev,
> > + " dma_direction=%d\n", dma_direction);
> > +
> > + dma_addr = dma_map_single(&priv->pci_dev->dev,
> > + data,
> > + dma_size,
> > + dma_direction);
> > +
> > + dev_dbg(&priv->pci_dev->dev, " dma_addr = 0x%016llX\n",
> > + dma_addr);
> > +
> > + desc->dptr_low = lower_32_bits(dma_addr);
> > + desc->dptr_high = upper_32_bits(dma_addr);
> > + }
> > +
> > + INIT_COMPLETION(priv->cmp);
> > +
> > + /* Add the descriptor */
> > + ismt_submit_desc(adap);
> > +
> > + /* Now we wait for interrupt completion, 1s */
> > + ret = wait_for_completion_interruptible_timeout(&priv->cmp, HZ*1);
> > +
> > + /* unmap the data buffer */
> > + if (map_dma_flag)
> > + dma_unmap_single(&adap->dev, dma_addr, dma_size, dma_direction);
> > +
> > + if (ret < 0) {
> > + dev_err(&priv->pci_dev->dev, "completion wait interrupted\n");
> > + ret = -EIO;
> > + } else if (ret == 0) {
> > + dev_err(&priv->pci_dev->dev, "completion wait timed out\n");
> > + ret = -ETIMEDOUT;
>
> Both are unlikely and you could help the compiler optimize the most common
> case.
done
>
> > + } else
> > + /* do any post processing of the descriptor here */
> > + ret = ismt_process_desc(adap);
> > +
> > + /* Update the ring pointer */
> > + priv->head++;
> > + priv->head %= ISMT_DESC_ENTRIES;
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * ismt_func() - report which i2c commands are supported by this
> > +adapter
> > + * @adap: the i2c host adapter
> > + */
> > +static u32 ismt_func(struct i2c_adapter *adap) {
> > + return I2C_FUNC_SMBUS_QUICK |
> > + I2C_FUNC_SMBUS_BYTE |
> > + I2C_FUNC_SMBUS_BYTE_DATA |
> > + I2C_FUNC_SMBUS_WORD_DATA |
> > + I2C_FUNC_SMBUS_PROC_CALL |
> > + I2C_FUNC_SMBUS_BLOCK_DATA |
> > + I2C_FUNC_SMBUS_PEC;
>
> Unusual spacing... There should be a single space between return and
> I2C_FUNC_SMBUS_QUICK, and if you really want to align the "|"s that should be
> done with tabs, not spaces.
done
>
> > +}
> > +
> > +/**
> > + * struct i2c_algorithm - the adapter algorithm and supported
> > +functionality
>
> Your structure is actually named smbus_algorithm. Anyway this syntax is meant
> to describe structures as you define them, not as you instantiate them. I
> think you have to omit the "struct" here.
done
>
> > + * @smbus_xfer: the adapter algorithm
> > + * @functionality: functionality supported by the adapter */ static
> > +const struct i2c_algorithm smbus_algorithm = {
> > + .smbus_xfer = ismt_access,
> > + .functionality = ismt_func,
> > +};
> > +
> > +/**
> > + * ismt_handle_isr() - interrupt handler bottom half
> > + * @priv: iSMT private data
> > + */
> > +static irqreturn_t ismt_handle_isr(struct ismt_priv *priv) {
> > + complete(&priv->cmp);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +
> > +/**
> > + * ismt_do_interrupt() - IRQ interrupt handler
> > + * @vec: interrupt vector
> > + * @data: iSMT private data
>
> Extra space after colon.
fixed
>
> > + */
> > +static irqreturn_t ismt_do_interrupt(int vec, void *data) {
> > + u32 val;
> > + struct ismt_priv *priv = (struct ismt_priv *)data;
>
> You never need to explicitly cast a void * pointer.
fixed
>
> > +
> > + /*
> > + * check to see it's our interrupt, return IRQ_NONE if not ours
> > + * since we are sharing interrupt
> > + */
> > + val = readl(priv->smba + ISMT_MSTR_MSTS);
> > +
> > + if (!(val & (ISMT_MSTS_MIS | ISMT_MSTS_MEIS)))
> > + return IRQ_NONE;
> > +
> > + if (val & ISMT_MSTS_MIS) {
> > + /* completed successfully */
> > + writel((val | ISMT_MSTS_MIS), priv->smba + ISMT_MSTR_MSTS);
>
> Parentheses not needed. Plus I doubt this is what you want to do... if (val &
> ISMT_MSTS_MIS) then (val | ISMT_MSTS_MIS) == val. Maybe you really meant "&"
> instead of "|"?
>
>
Nice find, thanks!
> Could it be that this code was never tested because MSI interrupts were
> always available? Please inject a pci_enable_msi error in
> ismt_int_init() to test this code path.
>
fixed and tested out okay with legacy ints.
> > + } else {
> > + /* completed with errors */
> > + writel((val | ISMT_MSTS_MEIS), priv->smba + ISMT_MSTR_MSTS);
>
> Same here.
done
>
> Furthermore, I would like to see the conditional go. I'm sure you can do
> without it, and this will speed up the code. Isn't it OK to write back both
> ISMT_MSTS_MIS and ISMT_MSTS_MEIS unconditionally? This would simplify the
> code a lot. If this isn't OK, then you can still remember which flag was set
> and write only that flag back, with:
>
> val = readl(priv->smba + ISMT_MSTR_MSTS) &
> (ISMT_MSTS_MIS | ISMT_MSTS_MEIS);
>
> if (!val)
> return IRQ_NONE;
>
> writel(val, priv->smba + ISMT_MSTR_MSTS);
>
> This too simplifies the code a lot (-27 binary bytes on x86_64) which is good
> to keep the latency low.
>
> I was also wondering if it was possible that both flags were set at the same
> time? My code would deal with this case properly, yours did not.
I rewrote it pretty much the way you described, with the exception that
there are other bits that I don't want changed.
>
> > + }
> > +
> > + return ismt_handle_isr(priv);
> > +}
> > +
> > +/**
> > + * ismt_do_msi_interrupt() - MSI interrupt handler
> > + * @vec: interrupt vector
> > + * @data: iSMT private data
>
> Extra space after colon.
fixed
>
> > + */
> > +static irqreturn_t ismt_do_msi_interrupt(int vec, void *data) {
> > + struct ismt_priv *priv = (struct ismt_priv *)data;
>
> Here again the cast isn't needed. In fact you can pass data to
> ismt_handle_isr() directly, so you don't even need to introduce priv.
done
>
> > +
> > + return ismt_handle_isr(priv);
> > +}
> > +
> > +/**
> > + * ismt_hw_init() - initialize the iSMT hardware
> > + * @pdev: PCI-Express device
> > + */
> > +static void __devinit ismt_hw_init(struct pci_dev *pdev) {
> > + u32 val;
> > + struct ismt_priv *priv = pci_get_drvdata(pdev);
> > +
> > + /* initialize the Master Descriptor Base Address (MDBA) */
> > + writeq(priv->io_rng_dma, priv->smba + ISMT_MSTR_MDBA);
>
> writeq() doesn't exist on 32-bit architectures, causing a build failure.
>
> If this driver has to work on 32-bit systems, you'll have to come up with a
> workaround. If not, I suggest we make it depend on CONFIG_X86_64. I doubt
> this driver is of any use besides X86 anyway, right?
Yep, x86 only.
fixed
>
> > +
> > + /* initialize the Master Control Register (MCTRL) */
> > + writel(ISMT_MCTRL_MEIE, priv->smba + ISMT_MSTR_MCTRL);
> > +
> > + /* initialize the Master Status Register (MSTS) */
> > + writel(0, priv->smba + ISMT_MSTR_MSTS);
> > +
> > + /* initialize the Master Descriptor Size (MDS) */
> > + val = readl(priv->smba + ISMT_MSTR_MDS);
> > + writel((val & ~(ISMT_MDS_MDS)) | (ISMT_DESC_ENTRIES - 1),
>
> Parentheses around ISMT_MDS_MDS aren't needed.
done
>
> > + priv->smba + ISMT_MSTR_MDS);
> > +
> > +#ifdef DEBUG_SLOW_HW
> > + /*
> > + * initialize the SMBus speed to 80KHz for slow HW debuggers
> > + */
> > + dev_dbg(&pdev->dev, " Setting SMBus clock to 80KHz\n");
> > + val = readl(priv->smba + ISMT_SPGT);
> > + writel(((val & ~(ISMT_SPGT_SPD)) | ISMT_SPGT_SPD_80K),
>
> Same here. But I don't think we want to keep this in the upstream driver, at
> least not in this form. If it's still useful it should be controlled by a
> module parameter, not a hidden define.
I made this a module parameter and documented it
in /Documentation/i2c/busses/i2c-ismt
>
> > + priv->smba + ISMT_SPGT);
> > +#endif
> > +
> > + dev_dbg(&pdev->dev, " priv->smba=%p\n", priv->smba);
>
> This would rather belong to the ismt_probe function IMHO. If needed at all...
It was extraneous, so I removed it.
>
> > +}
> > +
> > +/**
> > + * ismt_init() - initialize the iSMT data structures
> > + * @pdev: PCI-Express Device
> > + */
> > +static int __devinit ismt_init(struct pci_dev *pdev)
>
> This name is a little confusing with i2c_ismt_init later in the driver.
> What about ismt_dev_init?
changed
>
> > +{
> > + struct ismt_priv *priv = pci_get_drvdata(pdev);
> > +
> > + priv->entries = ISMT_DESC_ENTRIES;
> > +
> > + /* allocate memory for the descriptor */
> > + priv->hw = dmam_alloc_coherent(&pdev->dev,
> > + (ISMT_DESC_ENTRIES
> > + * sizeof(struct ismt_desc)),
> > + &priv->io_rng_dma,
> > + GFP_KERNEL);
> > + if (!priv->hw)
> > + return -ENOMEM;
> > +
> > + memset(priv->hw, 0, (ISMT_DESC_ENTRIES * sizeof(struct ismt_desc)));
> > +
> > + priv->head = 0;
> > + priv->tail = 0;
> > + init_completion(&priv->cmp);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * ismt_int_init() - initialize interrupts
> > + * @pdev: PCI-Express device
> > + * @priv: iSMT private data
> > + */
> > +static int __devinit ismt_int_init(struct pci_dev *pdev, struct
> > +ismt_priv *priv)
>
> That's not consistent with the two previous functions where you derive priv
> from pdev. Please do the same in all 3 functions. Given that gcc is very
> likely to inline the functions, I suggest passing the priv parameter always,
> as this is simplified at inlining time.
done
>
> > +{
> > + int err;
> > +
> > + /* Try using MSI interrupts */
> > + err = pci_enable_msi(pdev);
> > + if (err)
>
> Maybe a warning message would be good to have, indicating that we're falling
> back to legacy interrupts? Or is it supposed to happen on many systems?
Good idea, done.
>
> > + goto intx;
> > +
> > + err = devm_request_irq(&pdev->dev,
> > + pdev->irq,
> > + ismt_do_msi_interrupt,
> > + 0,
> > + "ismt-msi",
> > + priv);
> > +
> > + if (err) {
> > + pci_disable_msi(pdev);
> > + goto intx;
> > + }
> > +
> > + goto done;
> > +
> > + /* Try using legacy interrupts */
> > +intx:
>
> Out of curiosity, what's the "x" for?
For whatever reason, the docs refer to legacy interrupts as "INTx" :)
>
> > + err = devm_request_irq(&pdev->dev,
> > + pdev->irq,
> > + ismt_do_interrupt,
> > + IRQF_SHARED,
> > + "ismt-intx",
> > + priv);
> > + if (err) {
> > + dev_err(&pdev->dev, "no usable interrupts\n");
> > + return -ENODEV;
> > + }
> > +
> > +done:
> > + return 0;
> > +}
> > +
> > +static struct pci_driver ismt_driver;
> > +
> > +/**
> > + * ismt_probe() - probe for iSMT devices
> > + * @pdev: PCI-Express device
> > + * @id: PCI-Express device ID
> > + */
> > +static int __devinit
> > +ismt_probe(struct pci_dev *pdev, const struct pci_device_id *id) {
> > + int err;
> > + struct ismt_priv *priv;
> > + unsigned long start, len;
> > +
> > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + pci_set_drvdata(pdev, priv);
> > + i2c_set_adapdata(&priv->adapter, priv);
> > + priv->adapter.owner = THIS_MODULE;
> > +
> > + priv->adapter.class = I2C_CLASS_HWMON;
>
> Can't there be SPD EEPROMs on this bus?
Unfortunately, not on these controllers. Memory isn't available for the
descriptors during memory initialization in BIOS, so this controller
cannot be used.
>
> > +
> > + priv->adapter.algo = &smbus_algorithm;
> > + priv->pci_dev = pdev;
> > +
> > + err = pcim_enable_device(pdev);
> > + if (err) {
> > + dev_err(&pdev->dev, "Failed to enable SMBus PCI device (%d)\n",
> > + err);
> > + return err;
> > + }
> > +
> > + /* enable bus mastering */
> > + pci_set_master(pdev);
> > +
> > + /* Determine the address of the SMBus area */
> > + start = pci_resource_start(pdev, SMBBAR);
> > + if (!start) {
> > + dev_err(&pdev->dev,
> > + "SMBus base address uninitialized, upgrade BIOS\n");
> > + return -ENODEV;
> > + }
> > +
> > + len = pci_resource_len(pdev, SMBBAR);
> > + if (len == 0) {
> > + dev_err(&pdev->dev,
> > + "SMBus base address uninitialized, upgrade BIOS\n");
> > + return -ENODEV;
> > + }
>
> As you are printing the same error message, you can refactor the two tests to
> make the code more simple.
done
>
> > +
> > + dev_dbg(&priv->pci_dev->dev, " start=0x%lX\n", start);
> > + dev_dbg(&priv->pci_dev->dev, " len=0x%lX\n", len);
> > +
> > + err = acpi_check_resource_conflict(&pdev->resource[SMBBAR]);
> > + if (err) {
> > + dev_err(&pdev->dev, "ACPI resource conflict!\n");
> > + return err;
> > + }
> > +
> > + err = pci_request_region(pdev, SMBBAR, ismt_driver.name);
> > + if (err) {
> > + dev_err(&pdev->dev,
> > + "Failed to request SMBus region 0x%lx-0x%lx\n",
> > + start, start + len);
> > + return err;
> > + }
> > +
> > + priv->smba = pcim_iomap(pdev, SMBBAR, len);
> > + if (!priv->smba) {
> > + dev_err(&pdev->dev, "Unable to ioremap SMBus BAR\n");
> > + err = -ENODEV;
> > + goto fail;
> > + }
> > +
> > + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
> > + if (err) {
> > + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> > + if (err)
> > + goto fail;
> > + dev_warn(&pdev->dev, "Cannot DMA highmem\n");
> > + }
> > +
> > + err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
> > + if (err) {
> > + err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
> > + if (err)
> > + goto fail;
> > + dev_warn(&pdev->dev, "Cannot DMA consistent highmem\n");
> > + }
>
> This doesn't look safe to me. What if
> pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) succeeds but
> pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)) fails, or vice-versa? I'd
> rather do (with a disclaimer: I don't know anything about this "consistent
> DMA" thing):
>
> err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
> if (err) {
> /* Fall back to 32-bit - lowmem */
> err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> if (err)
> goto fail;
> err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
> if (err)
> goto fail;
> dev_warn(&pdev->dev, "Cannot DMA highmem, falling back to
> lowmem\n");
> } else {
> err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
> if (err)
> goto fail;
> }
>
> (Or the more compact variants found in drivers/scsi/vmw_pvscsi.c or
> drivers/scsi/bfa/bfad.c for example.)
>
> What do you think? To my candid eyes, setting a different value for
> pci_set_consistent_dma_mask() than for pci_set_dma_mask() seems incorrect.
> OTOH it seems that some other drivers are doing exactly that so maybe I'm
> just wrong.
I agree, it doesn't look right. I asked around, and heard that there
are no known issues with the way I implemented it, but that doesn't make
it right. I liked the implementation in bfad.c and used it, thanks!
>
> > +
> > + err = ismt_init(pdev);
> > + if (err) {
> > + err = -ENODEV;
>
> Please just return the err value returned by the called function.
fixed
>
> > + goto fail;
> > + }
> > +
> > + ismt_hw_init(pdev);
> > +
> > + err = ismt_int_init(pdev, priv);
> > + if (err)
> > + goto fail;
> > +
> > + /* set up the sysfs linkage to our parent device */
> > + priv->adapter.dev.parent = &pdev->dev;
> > +
> > + /* number of retries on lost arbitration */
> > + priv->adapter.retries = ISMT_MAX_RETRIES;
> > +
> > + snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> > + "SMBus iSMT adapter at %p", priv->smba);
>
> A good I2C adapter name is stable across reboots, this one is not. It's
> better to use "start" as the unique value, as it will stay the same across
> reboots.
Thanks for the help here!
done
>
> As a side note, it may make sense, for performance and clarity, to group the
> initialization of all priv->adapter fields in one place. You initialize 4
> fields at the top of the function and 3 fields at the bottom, for no obvious
> reason.
done
>
> > + err = i2c_add_adapter(&priv->adapter);
> > + if (err) {
> > + dev_err(&pdev->dev, "Failed to add SMBus iSMT adapter\n");
> > + err = -ENODEV;
> > + goto fail;
> > + }
> > + return 0;
> > +
> > +fail:
> > + pci_release_region(pdev, SMBBAR);
> > + return err;
> > +}
> > +
> > +/**
> > + * ismt_remove() - release driver resources
> > + * @pdev: PCI-Express device
> > + */
> > +static void __devexit ismt_remove(struct pci_dev *pdev) {
> > + struct ismt_priv *priv = pci_get_drvdata(pdev);
> > +
> > + writel(ISMT_GCTRL_SRST, priv->smba + ISMT_GR_GCTRL);
> > + i2c_del_adapter(&priv->adapter);
>
> This is most certainly racy, I think you want to delete the I2C adapter
> first. BTW, what's the rationale for unconditionally resetting the bus here?
I removed the unconditional reset. It was a leftover from a polling
implementation I had done that required the reset.
>
> > + pci_release_region(pdev, SMBBAR);
> > +}
> > +
> > +/**
> > + * ismt_suspend() - place the device in suspend
> > + * @pdev: PCI-Express device
> > + * @mesg: PM message
> > + */
> > +#ifdef CONFIG_PM
> > +static int ismt_suspend(struct pci_dev *pdev, pm_message_t mesg) {
> > + pci_save_state(pdev);
> > + pci_set_power_state(pdev, pci_choose_state(pdev, mesg));
> > + return 0;
> > +}
> > +
> > +/**
> > + * ismt_resume() - PCI resume code
> > + * @pdev: PCI-Express device
> > + */
> > +static int ismt_resume(struct pci_dev *pdev) {
> > + pci_set_power_state(pdev, PCI_D0);
> > + pci_restore_state(pdev);
> > + return pci_enable_device(pdev);
> > +}
> > +
> > +#else
> > +
> > +#define ismt_suspend NULL
> > +#define ismt_resume NULL
> > +
> > +#endif
> > +
> > +static struct pci_driver ismt_driver = {
> > + .name = "ismt_smbus",
> > + .id_table = ismt_ids,
> > + .probe = ismt_probe,
> > + .remove = __devexit_p(ismt_remove),
> > + .suspend = ismt_suspend,
> > + .resume = ismt_resume,
> > +};
> > +
> > +/**
> > + * i2c_ismt_init() - iSMT driver initialization */ static int __init
> > +i2c_ismt_init(void) {
> > + pr_debug("Loading the iSMT SMBus driver\n");
> > + return pci_register_driver(&ismt_driver);
> > +}
> > +
> > +/**
> > + * i2c_ismt_exit() - iSMT driver exit code */ static void __exit
> > +i2c_ismt_exit(void) {
> > + pr_debug("Unloading iSMT SMBus driver\n");
> > + pci_unregister_driver(&ismt_driver);
> > +}
>
> If you can live without the two debugging messages above (and they don't
> strike me as terribly useful) you can use module_pci_driver().
I learned something new, thanks! I went with module_pci_driver().
>
> > +
> > +MODULE_LICENSE("Dual BSD/GPL");
> > +MODULE_AUTHOR("Bill E. Brown <[email protected]>");
> > +MODULE_DESCRIPTION("Intel SMBus Message Transport (iSMT) driver");
> > +
> > +module_init(i2c_ismt_init);
> > +module_exit(i2c_ismt_exit);
> > +
>
> No blank line at end of file please.
done
>
> > diff --git a/drivers/i2c/busses/i2c-ismt.h
> > b/drivers/i2c/busses/i2c-ismt.h new file mode 100644 index
> > 0000000..e9c10e1
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-ismt.h
>
> Is there any reason why you'd ever need to include this file from another
> driver file than i2c-ismt.c? If not, then please just move all these
> definitions at the top of i2c-ismt.c. This speeds up the build.
Good point. I like a separate header file when they start to get long,
but this isn't too bad so I moved it to the top of i2c-ismt.c.
>
> > @@ -0,0 +1,111 @@
> > +#ifndef _I2C_ISMT_H_
> > +#define _I2C_ISMT_H_
> > +
> > +/* PCI Address Constants */
> > +#define SMBBAR 0
> > +
> > +/* PCI DIDs for Briarwood's pair of SMBus Message Transport (SMT)
> > +Devices */ #define PCI_DEVICE_ID_INTEL_BWD_SMBUS_SMT0 0x0c59 #define
> > +PCI_DEVICE_ID_INTEL_BWD_SMBUS_SMT1 0x0c5a
>
> Please align IDs using tabs, it will improve readability especially in case
> we ever have to add more (and history suggests we will.) Same everywhere
> below, all values and comments (at least within a given
> block) should be aligned, using tabs.
done
>
> > +
> > +#define ISMT_DESC_ENTRIES 32 /* number of descritor entries */
>
> Typo: descriptor
fixed
>
> > +#define ISMT_MAX_RETRIES 3 /* number of SMBus retries to attempt */
> > +
> > +/* Hardware Descriptor Constants - Control Field */
> > +#define ISMT_DESC_CWRL 0x01 /* Command/Write Length */
> > +#define ISMT_DESC_BLK 0X04 /* Perform Block Transaction */
> > +#define ISMT_DESC_FAIR 0x08 /* Set fairness flag upon successful
> > arbit. */
> > +#define ISMT_DESC_PEC 0x10 /* Packet Error Code */
> > +#define ISMT_DESC_I2C 0x20 /* I2C Enable */
> > +#define ISMT_DESC_INT 0x40 /* Interrupt */
> > +#define ISMT_DESC_SOE 0x80 /* Stop On Error */
> > +
> > +/* Hardware Descriptor Constants - Status Field */
> > +#define ISMT_DESC_SCS 0x01 /* Success */
> > +#define ISMT_DESC_DLTO 0x04 /* Data Low Time Out */
> > +#define ISMT_DESC_NAK 0x08 /* NAK Received */
> > +#define ISMT_DESC_CRC 0x10 /* CRC Error */
> > +#define ISMT_DESC_CLTO 0x20 /* Clock Low Time Out */
> > +#define ISMT_DESC_COL 0x40 /* Collisions */
> > +#define ISMT_DESC_LPR 0x80 /* Large Packet Received */
> > +
> > +/* Hardware Descriptor Masks */
> > +#define ISMT_DESC_ADDR 0x7f /* I2C/SMB address mask */
> > +#define ISMT_DESC_RW 0x01 /* Read/Write bit mask */
>
> I don't think it's worth introducing these for a single use where the
> constants would be just as clear.
Agreed, done.
>
> > +
> > +/* Macros */
> > +#define ISMT_DESC_ADDR_RW(addr, rw) (((addr & ISMT_DESC_ADDR) << 1)
> > \
> > + | (rw & ISMT_DESC_RW))
>
> The masking doesn't seem to be terribly needed anyway, it would work the same
> (and be marginally faster) without. Also note that addr and rw must be
> enclosed in parentheses for your macro to be safe.
done
>
> > +
> > +/* iSMT General Register address offsets (SMBAR + <addr>) */
>
> Typo: SMBBAR
fixed
>
> > +#define ISMT_GR_GCTRL 0x000 /* General Control */
> > +#define ISMT_GR_SMTICL 0x008 /* SMT Interrupt Cause Location */
> > +#define ISMT_GR_ERRINTMSK 0x010 /* Error Interrupt Mask */ #define
> > +ISMT_GR_ERRAERMSK 0x014 /* Error AER Mask */
> > +#define ISMT_GR_ERRSTS 0x018 /* Error Status */
> > +#define ISMT_GR_ERRINFO 0x01c /* Error Information */
> > +
> > +/* iSMT Master Registers */
> > +#define ISMT_MSTR_MDBA 0x100 /* Master Descriptor Base Address */
> > +#define ISMT_MSTR_MCTRL 0x108 /* Master Control */
> > +#define ISMT_MSTR_MSTS 0x10c /* Master Status */
> > +#define ISMT_MSTR_MDS 0x110 /* Master Descriptor Size */
> > +#define ISMT_MSTR_RPOLICY 0x114 /* Retry Policy */
> > +
> > +/* iSMT Miscellaneous Registers */
> > +#define ISMT_SPGT 0x300 /* SMBus PHY Global Timing */
> > +
> > +/* General Control Register (GCTRL) bit definitions */
> > +#define ISMT_GCTRL_TRST 0x04 /* Target Reset */
> > +#define ISMT_GCTRL_KILL 0x08 /* Kill */
> > +#define ISMT_GCTRL_SRST 0x40 /* Soft Reset */
> > +
> > +/* Master Control Register (MCTRL) bit definitions */
> > +#define ISMT_MCTRL_SS 0x01 /* Start/Stop */
> > +#define ISMT_MCTRL_MEIE 0x10 /* Master Error Interrupt Enable */
> > +#define ISMT_MCTRL_FMHP 0x00ff0000 /* Firmware Master Head Pointer
> > +(FMHP) */
> > +
> > +/* Master Status Register (MSTS) bit definitions */ #define
> > +ISMT_MSTS_HMTP 0xff0000 /* HW Master Tail Pointer (HMTP) */
> > +#define ISMT_MSTS_MIS 0x20 /* Master Interrupt Status (MIS) */
> > +#define ISMT_MSTS_MEIS 0x10 /* Master Error Interrupt Status (MEIS) */
> > +#define ISMT_MSTS_IP 0x01 /* In Progress */
> > +
> > +/* Master Descriptor Size (MDS) bit definitions */ #define
> > +ISMT_MDS_MDS 0xFF /* Master Descriptor Size mask (MDS) */
>
> I tend to prefer when masks are clearly identified as such with a trailing
> _MASK.
done
>
> > +
> > +/* SMBus PHY Global Timing Register (SPGT) bit definitions */
> > +#define ISMT_SPGT_SPD 0xc0000000 /* SMBus Speed mask */
>
> Same here.
done
>
> > +#define ISMT_SPGT_SPD_80K (0x01 << 30) /* 80 KHz */
>
> kHz with lower-case k, please.
changed
>
> Please define the other possible speeds, in case people without the datasheet
> need to experiment or debug.
I added them in. They came in handy for the module parameter.
>
> > +
> > +/* MSI Control Register (MSICTL) bit definitions */ #define
> > +ISMT_MSICTL_MSIE 0x01 /* MSI Enable */
> > +
> > +/* iSMT Hardware Descriptor */
> > +struct ismt_desc {
> > + u8 tgtaddr_rw; /* target address & r/w bit */
> > + u8 wr_len_cmd; /* write length in bytes or a command */
> > + u8 rd_len; /* read length */
> > + u8 control; /* control bits */
> > + u8 status; /* status bits */
> > + u8 retry; /* collision retry and retry count */
> > + u8 rxbytes; /* received bytes */
> > + u8 txbytes; /* transmitted bytes */
> > + u32 dptr_low; /* lower 32 bit of the data pointer */
> > + u32 dptr_high; /* upper 32 bit of the data pointer */ };
>
> Aren't you supposed to declare it with __packed to guarantee it matches the
> hardware's idea of the descriptor?
Good idea, done/
>
> > +
> > +struct ismt_priv {
> > + struct i2c_adapter adapter;
> > + void *smba; /* PCI BAR */
> > + struct pci_dev *pci_dev;
> > + struct ismt_ring_ent **ring; /* housekeeping struct pointer */
>
> This structure isn't defined anywhere, and this member is never used.
removed
>
> > + struct ismt_desc *hw; /* virtual base address of the descriptor */
> > + dma_addr_t io_rng_dma; /* hardware base address of the descriptor */
> > + int entries; /* number of descriptor entries */
>
> This member is assigned once and never used after that. You hard-coded
> ISMT_DESC_ENTRIES everywhere in the driver. If there's no reason to use
> another value, just drop priv->entries. If there is a good reason to ever
> change it, keep and consistently use priv->entries everywhere.
removed
>
> > + u8 head; /* ring buffer head pointer */
> > + u8 tail; /* ring buffer tail pointer */
>
> Same here, assigned once and never used after that.
removed
>
> > + struct completion cmp; /* interrupt completion */ };
> > +
> > +#endif
>
> --
> Jean Delvare
I'll be submitting v3 shortly. :)
Many thanks for the thorough review Jean, I really appreciate it!!
Bill Brown
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
