Re: [PATCH v3] i2c:amd I2C Driver based on PCI Interface for upcoming, platform

[Elie Morisse]

Hi,

I did some work to improve Shah's driver and make it work on laptops
where the two buses of the MP2 are enabled (e.g Lenovo Yoga
530-14ARR):

https://marc.info/?l=linux-i2c&m=154039677815809

v3 had some issues preventing both buses to work:

- enabling the second adapter (bus 1) was overriding amd_mp2_dev.ops
so events intended for the first adapter (bus 0) would be handled by
the second adapter
- in the interrupt handler checking whether the content of
AMD_P2C_MSG1 or AMD_P2C_MSG2 was != zero to determine which bus the
IRQ was for was incorrect since they were not getting zero'd

Tomorrow I'll submit an updated patch that takes Bjorn's remarks that
still apply to my version into account.

Elie Morisse
Le mer. 24 oct. 2018 Ã 16:16, Bjorn Helgaas <helgaas@xxxxxxxxxx> a Ãcrit :
>
> On Thu, Oct 25, 2018 at 01:26:51AM +0800, Kai Heng Feng wrote:
> > > On Sep 17, 2018, at 16:19, Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> wrote:
> > > at 18:54, Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@xxxxxxx> wrote:
> > >
> > >> From: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@xxxxxxx>
> > >>
> > >> This contains two drivers.
> > >> 1)i2c-amd-platdrv: This is based on I2C framework of
> > >> linux kernel. So any i2c read write call or commands
> > >> to this driver is routed to PCI Interface driver.
> > >> 2) i2c-amd-platdrv: This driver is responsible to
> > >> talk with Mp2 and does all C2P/P2C communication
> > >> or reading/writing from DRAM in case of more
> > >> data.
> > >>
> > >> Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@xxxxxxx>
> > >> Reviewed-by: Sandeep Singh <sandeep.singh@xxxxxxx>
> > >> Signed-off-by: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@xxxxxxx>
> > >
> > > The I2C touchpad on Latitude 5495 works with this patch.
> >
> > From PCIâs point of view, do you think this driver is good?
>
> Speaking for myself, I usually only review things that *change* the
> PCI core, not things like drivers that simply *use* the PCI core
> services.
>
> But since you made the mistake of asking, I do have a few comments
> below :)
>
> > >> --- /dev/null
> > >> +++ b/drivers/i2c/busses/i2c-amd-pci-mp2.c
> > >> @@ -0,0 +1,580 @@
> > >> +// SPDX-License-Identifier: GPL-2.0
> > >> +/*
> > >> + * Copyright (C) 2018 Advanced Micro Devices, Inc. All Rights Reserved.
> > >> + *
> > >> + *
>
> Extra blank line above.
>
> > >> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> > >> + * AMD PCIe MP2 Communication Driver
> > >> + */
> > >> +
> > >> +#include <linux/debugfs.h>
> > >> +#include <linux/interrupt.h>
> > >> +#include <linux/module.h>
> > >> +#include <linux/pci.h>
> > >> +#include <linux/slab.h>
> > >> +
> > >> +#include "i2c-amd-pci-mp2.h"
> > >> +
> > >> +#define DRIVER_NAME       "pcie_mp2_amd"
> > >> +#define DRIVER_DESC       "AMD(R) PCI-E MP2 Communication Driver"
>
> "PCIe" is the usual spelling.
>
> > >> +#define DRIVER_VER        "1.0"
> > >> +
> > >> +MODULE_DESCRIPTION(DRIVER_DESC);
> > >> +MODULE_VERSION(DRIVER_VER);
> > >> +MODULE_LICENSE("Dual BSD/GPL");
>
> This doesn't match the SPDX tag above.
> See https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
>
> > >> +MODULE_AUTHOR("Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>");
> > >> +
> > >> +static const struct file_operations amd_mp2_debugfs_info;
> > >> +static struct dentry *debugfs_dir;
> > >> +
> > >> +int amd_mp2_connect(struct pci_dev *dev, struct i2c_config i2c_cfg)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata = pci_get_drvdata(dev);
> > >> +  union i2c_cmd_base i2c_cmd_base;
> > >> +  unsigned  long  flags;
>
> Extra spaces in the "flags" declaration.
>
> > >> +
> > >> +  raw_spin_lock_irqsave(&privdata->lock, flags);
> > >> +  dev_dbg(ndev_dev(privdata), "%s addr: %x id: %d\n", __func__,
> > >> +          i2c_cfg.dev_addr, i2c_cfg.bus_id);
> > >> +
> > >> +  i2c_cmd_base.ul = 0;
> > >> +  i2c_cmd_base.s.i2c_cmd = i2c_enable;
> > >> +  i2c_cmd_base.s.bus_id = i2c_cfg.bus_id;
> > >> +  i2c_cmd_base.s.i2c_speed = i2c_cfg.i2c_speed;
> > >> +
> > >> +  if (i2c_cmd_base.s.bus_id == i2c_bus_1) {
> > >> +          writel(i2c_cmd_base.ul, privdata->mmio + AMD_C2P_MSG1);
> > >> +  } else if (i2c_cmd_base.s.bus_id == i2c_bus_0) {
> > >> +          writel(i2c_cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
> > >> +  } else {
> > >> +          dev_err(ndev_dev(privdata), "%s Invalid bus id\n", __func__);
> > >> +          return -EINVAL;
> > >> +  }
> > >> +  raw_spin_unlock_irqrestore(&privdata->lock, flags);
> > >> +  return 0;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(amd_mp2_connect);
> > >> +
> > >> +int amd_mp2_read(struct pci_dev *dev, struct i2c_config i2c_cfg)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata = pci_get_drvdata(dev);
> > >> +  union i2c_cmd_base i2c_cmd_base;
> > >> +
> > >> +  dev_dbg(ndev_dev(privdata), "%s addr: %x id: %d\n", __func__,
> > >> +          i2c_cfg.dev_addr, i2c_cfg.bus_id);
> > >> +
> > >> +  privdata->requested = true;
> > >> +  i2c_cmd_base.ul = 0;
> > >> +  i2c_cmd_base.s.i2c_cmd = i2c_read;
> > >> +  i2c_cmd_base.s.dev_addr = i2c_cfg.dev_addr;
> > >> +  i2c_cmd_base.s.length = i2c_cfg.length;
> > >> +  i2c_cmd_base.s.bus_id = i2c_cfg.bus_id;
>
> This block is repeated and could be factored out to a helper function
> (parameterized with i2c_read/i2c_write).
>
> > >> +
> > >> +  if (i2c_cfg.length <= 32) {
> > >> +          i2c_cmd_base.s.mem_type = use_c2pmsg;
> > >> +          privdata->eventval.buf = (u32 *)i2c_cfg.read_buf;
> > >> +          if (!privdata->eventval.buf) {
> > >> +                  dev_err(ndev_dev(privdata), "%s no mem for buf received\n",
> > >> +                          __func__);
> > >> +                  return -ENOMEM;
> > >> +          }
> > >> +  } else {
> > >> +          i2c_cmd_base.s.mem_type = use_dram;
> > >> +          privdata->i2c_cfg.phy_addr = i2c_cfg.phy_addr;
> > >> +          privdata->i2c_cfg.read_buf = i2c_cfg.read_buf;
> > >> +          write64((u64)privdata->i2c_cfg.phy_addr,
> > >> +                  privdata->mmio + AMD_C2P_MSG2);
> > >> +  }
> > >> +
> > >> +  switch (i2c_cfg.i2c_speed) {
> > >> +  case 0:
> > >> +          i2c_cmd_base.s.i2c_speed = speed100k;
> > >> +          break;
> > >> +  case 1:
> > >> +          i2c_cmd_base.s.i2c_speed = speed400k;
> > >> +          break;
> > >> +  case 2:
> > >> +          i2c_cmd_base.s.i2c_speed = speed1000k;
> > >> +          break;
> > >> +  case 3:
> > >> +          i2c_cmd_base.s.i2c_speed = speed1400k;
> > >> +          break;
> > >> +  case 4:
> > >> +          i2c_cmd_base.s.i2c_speed = speed3400k;
> > >> +          break;
> > >> +  default:
> > >> +          dev_err(ndev_dev(privdata), "Invalid ConnectionSpeed\n");
> > >> +  }
>
> This switch statement is repeated below and could be factored out into
> a helper function.
>
> > >> +  if (i2c_cmd_base.s.bus_id == i2c_bus_1) {
> > >> +          writel(i2c_cmd_base.ul, privdata->mmio + AMD_C2P_MSG1);
> > >> +  } else if (i2c_cmd_base.s.bus_id == i2c_bus_0) {
> > >> +          writel(i2c_cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
> > >> +  } else {
> > >> +          dev_err(ndev_dev(privdata), "%s Invalid bus id\n", __func__);
> > >> +          return -EINVAL;
> > >> +  }
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(amd_mp2_read);
> > >> +
> > >> +int amd_mp2_write(struct pci_dev *dev, struct i2c_config i2c_cfg)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata = pci_get_drvdata(dev);
> > >> +  union i2c_cmd_base i2c_cmd_base;
> > >> +  int i = 0;
>
> This initialization is unnecessary.
>
> > >> +  privdata->requested = true;
> > >> +  dev_dbg(ndev_dev(privdata), "%s addr: %x id: %d\n", __func__,
> > >> +          i2c_cfg.dev_addr, i2c_cfg.bus_id);
> > >> +
> > >> +  i2c_cmd_base.ul = 0;
> > >> +  i2c_cmd_base.s.i2c_cmd = i2c_write;
> > >> +  i2c_cmd_base.s.dev_addr = i2c_cfg.dev_addr;
> > >> +  i2c_cmd_base.s.length = i2c_cfg.length;
> > >> +  i2c_cmd_base.s.bus_id = i2c_cfg.bus_id;
> > >> +
> > >> +  switch (i2c_cfg.i2c_speed) {
> > >> +  case 0:
> > >> +          i2c_cmd_base.s.i2c_speed = speed100k;
> > >> +          break;
> > >> +  case 1:
> > >> +          i2c_cmd_base.s.i2c_speed = speed400k;
> > >> +          break;
> > >> +  case 2:
> > >> +          i2c_cmd_base.s.i2c_speed = speed1000k;
> > >> +          break;
> > >> +  case 3:
> > >> +          i2c_cmd_base.s.i2c_speed = speed1400k;
> > >> +          break;
> > >> +  case 4:
> > >> +          i2c_cmd_base.s.i2c_speed = speed3400k;
> > >> +          break;
> > >> +  default:
> > >> +          dev_err(ndev_dev(privdata), "Invalid ConnectionSpeed\n");
> > >> +  }
> > >> +
> > >> +  if (i2c_cfg.length <= 32) {
> > >> +          i2c_cmd_base.s.mem_type = use_c2pmsg;
> > >> +          for (i = 0; i < ((i2c_cfg.length + 3) / 4); i++) {
> > >> +                  writel(i2c_cfg.write_buf[i],
> > >> +                         privdata->mmio + (AMD_C2P_MSG2 + i * 4));
> > >> +          }
> > >> +  } else {
> > >> +          i2c_cmd_base.s.mem_type = use_dram;
> > >> +          privdata->i2c_cfg.phy_addr = i2c_cfg.phy_addr;
> > >> +          write64((u64)privdata->i2c_cfg.phy_addr,
> > >> +                  privdata->mmio + AMD_C2P_MSG2);
> > >> +  }
> > >> +
> > >> +  if (i2c_cmd_base.s.bus_id == i2c_bus_1) {
> > >> +          writel(i2c_cmd_base.ul, privdata->mmio + AMD_C2P_MSG1);
> > >> +  } else if (i2c_cmd_base.s.bus_id == i2c_bus_0) {
> > >> +          writel(i2c_cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
> > >> +  } else {
> > >> +          dev_err(ndev_dev(privdata), "%s Invalid bus id\n", __func__);
> > >> +          return -EINVAL;
> > >> +  }
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(amd_mp2_write);
> > >> +
> > >> +int amd_i2c_register_cb(struct pci_dev *dev, const struct amd_i2c_pci_ops *ops,
> > >> +                  void *dev_ctx)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata = pci_get_drvdata(dev);
> > >> +
> > >> +  privdata->ops = ops;
> > >> +  privdata->i2c_dev_ctx = dev_ctx;
> > >> +
> > >> +  if (!privdata->ops || !privdata->i2c_dev_ctx)
> > >> +          return -EINVAL;
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(amd_i2c_register_cb);
> > >> +
> > >> +static void amd_mp2_pci_work(struct work_struct *work)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata = mp2_dev(work);
> > >> +  u32 readdata = 0;
> > >> +  int i = 0;
>
> The above two initializations (readdata, i) are unnecessary.
>
> > >> +  int sts = privdata->eventval.base.r.status;
> > >> +  int res = privdata->eventval.base.r.response;
> > >> +  int len = privdata->eventval.base.r.length;
> > >> +
> > >> +  if (res == command_success && sts == i2c_readcomplete_event) {
> > >> +          if (privdata->ops->read_complete) {
> > >> +                  if (len <= 32) {
> > >> +                          for (i = 0; i < ((len + 3) / 4); i++) {
> > >> +                                  readdata = readl(privdata->mmio +
> > >> +                                                  (AMD_C2P_MSG2 + i * 4));
> > >> +                                  privdata->eventval.buf[i] = readdata;
> > >> +                          }
> > >> +                          privdata->ops->read_complete(privdata->eventval,
> > >> +                                          privdata->i2c_dev_ctx);
> > >> +                  } else {
> > >> +                          privdata->ops->read_complete(privdata->eventval,
> > >> +                                          privdata->i2c_dev_ctx);
> > >> +                  }
> > >> +          }
> > >> +  } else if (res == command_success && sts == i2c_writecomplete_event) {
> > >> +          if (privdata->ops->write_complete)
> > >> +                  privdata->ops->write_complete(privdata->eventval,
> > >> +                                  privdata->i2c_dev_ctx);
> > >> +  } else if (res == command_success && sts == i2c_busenable_complete) {
> > >> +          if (privdata->ops->connect_complete)
> > >> +                  privdata->ops->connect_complete(privdata->eventval,
> > >> +                                  privdata->i2c_dev_ctx);
> > >> +  } else {
> > >> +          dev_err(ndev_dev(privdata), "ERROR!!nothing to be handled !\n");
> > >> +  }
> > >> +}
> > >> +
> > >> +static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata = dev;
> > >> +  u32 val = 0;
>
> This initialization is unnecessary.
>
> > >> +  unsigned long  flags;
>
> Extra space before "flags".
>
> > >> +
> > >> +  raw_spin_lock_irqsave(&privdata->lock, flags);
> > >> +  val = readl(privdata->mmio + AMD_P2C_MSG1);
> > >> +  if (val != 0) {
> > >> +          writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
> > >> +          privdata->eventval.base.ul = val;
> > >> +  } else {
> > >> +          val = readl(privdata->mmio + AMD_P2C_MSG2);
> > >> +          if (val != 0) {
> > >> +                  writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
> > >> +                  privdata->eventval.base.ul = val;
> > >> +          }
> > >> +  }
> > >> +
> > >> +  raw_spin_unlock_irqrestore(&privdata->lock, flags);
> > >> +  if (!privdata->ops)
> > >> +          return IRQ_NONE;
> > >> +
> > >> +  if (!privdata->requested)
> > >> +          return IRQ_HANDLED;
> > >> +
> > >> +  privdata->requested = false;
> > >> +  schedule_delayed_work(&privdata->work, 0);
> > >> +
> > >> +  return IRQ_HANDLED;
> > >> +}
> > >> +
> > >> +static ssize_t amd_mp2_debugfs_read(struct file *filp, char __user *ubuf,
> > >> +                              size_t count, loff_t *offp)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata;
> > >> +  void __iomem *mmio;
> > >> +  u8 *buf;
> > >> +  u32 data;
> > >> +  size_t buf_size;
> > >> +  ssize_t ret, off;
> > >> +
> > >> +  privdata = filp->private_data;
> > >> +  mmio = privdata->mmio;
> > >> +  buf_size = min(count, 0x800ul);
> > >> +  buf = kmalloc(buf_size, GFP_KERNEL);
> > >> +
> > >> +  if (!buf)
> > >> +          return -ENOMEM;
> > >> +
> > >> +  off = 0;
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "Mp2 Device Information:\n");
> > >> +
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "========================\n");
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "\tMP2 C2P Message Register Dump:\n\n");
> > >> +  data = readl(privdata->mmio + AMD_C2P_MSG0);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_C2P_MSG0 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_C2P_MSG1);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_C2P_MSG1 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_C2P_MSG2);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_C2P_MSG2 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_C2P_MSG3);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_C2P_MSG3 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_C2P_MSG4);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_C2P_MSG4 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_C2P_MSG5);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_C2P_MSG5 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_C2P_MSG6);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_C2P_MSG6 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_C2P_MSG7);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_C2P_MSG7 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_C2P_MSG8);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_C2P_MSG8 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_C2P_MSG9);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_C2P_MSG9 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "\n\tMP2 P2C Message Register Dump:\n\n");
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_P2C_MSG1);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_P2C_MSG1 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_P2C_MSG2);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_P2C_MSG2 -\t\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_P2C_MSG_INTEN);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_P2C_MSG_INTEN -\t\t%#06x\n", data);
> > >> +
> > >> +  data = readl(privdata->mmio + AMD_P2C_MSG_INTSTS);
> > >> +  off += scnprintf(buf + off, buf_size - off,
> > >> +                  "AMD_P2C_MSG_INTSTS -\t\t%#06x\n", data);
> > >> +
> > >> +  ret = simple_read_from_buffer(ubuf, count, offp, buf, off);
> > >> +  kfree(buf);
> > >> +  return ret;
> > >> +}
> > >> +
> > >> +static void amd_mp2_init_debugfs(struct amd_mp2_dev *privdata)
> > >> +{
> > >> +  if (!debugfs_dir) {
> > >> +          privdata->debugfs_dir = NULL;
> > >> +          privdata->debugfs_info = NULL;
>
> Just return here and unindent the following block, like this:
>
>   if (!debugfs_dir) {
>     ...
>     return;
>   }
>
>   privdata->debugfs_dir = ...
>
> > >> +  } else {
> > >> +          privdata->debugfs_dir = debugfs_create_dir(ndev_name(privdata),
> > >> +                                                     debugfs_dir);
> > >> +          if (!privdata->debugfs_dir) {
> > >> +                  privdata->debugfs_info = NULL;
> > >> +          } else {
> > >> +                  privdata->debugfs_info = debugfs_create_file
> > >> +                          ("info", 0400, privdata->debugfs_dir,
> > >> +                                  privdata, &amd_mp2_debugfs_info);
> > >> +          }
> > >> +  }
> > >> +}
> > >> +
> > >> +static void amd_mp2_deinit_debugfs(struct amd_mp2_dev *privdata)
> > >> +{
> > >> +  debugfs_remove_recursive(privdata->debugfs_dir);
> > >> +}
> > >> +
> > >> +static void amd_mp2_clear_reg(struct amd_mp2_dev *privdata)
> > >> +{
> > >> +  int reg = 0;
>
> Unnecessary initialization.
>
> > >> +
> > >> +  for (reg = AMD_C2P_MSG0; reg <= AMD_C2P_MSG9; reg += 4)
> > >> +          writel(0, privdata->mmio + reg);
> > >> +
> > >> +  for (reg = AMD_P2C_MSG0; reg <= AMD_P2C_MSG2; reg += 4)
> > >> +          writel(0, privdata->mmio + reg);
> > >> +}
> > >> +
> > >> +static int amd_mp2_pci_init(struct amd_mp2_dev *privdata, struct pci_dev *pdev)
> > >> +{
> > >> +  int rc;
> > >> +  int bar_index = 2;
>
> Better to hardcode "2" at the point where bar_index used so the reader
> doesn't have to scan back up to the top of the function to see which
> it is.
>
> > >> +  resource_size_t size, base;
> > >> +
> > >> +  pci_set_drvdata(pdev, privdata);
> > >> +
> > >> +  rc = pci_enable_device(pdev);
> > >> +  if (rc)
> > >> +          goto err_pci_enable;
> > >> +
> > >> +  rc = pci_request_regions(pdev, DRIVER_NAME);
> > >> +  if (rc)
> > >> +          goto err_pci_regions;
> > >> +
> > >> +  pci_set_master(pdev);
> > >> +
> > >> +  rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
> > >> +  if (rc) {
> > >> +          rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> > >> +          if (rc)
> > >> +                  goto err_dma_mask;
> > >> +          dev_warn(ndev_dev(privdata), "Cannot DMA highmem\n");
> > >> +  }
> > >> +
> > >> +  rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
> > >> +  if (rc) {
> > >> +          rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
> > >> +          if (rc)
> > >> +                  goto err_dma_mask;
> > >> +          dev_warn(ndev_dev(privdata), "Cannot DMA consistent highmem\n");
> > >> +  }
> > >> +
> > >> +  base = pci_resource_start(pdev, bar_index);
> > >> +  size = pci_resource_len(pdev, bar_index);
> > >> +
> > >> +  privdata->mmio = ioremap(base, size);
> > >> +  if (!privdata->mmio) {
> > >> +          rc = -EIO;
> > >> +          goto err_dma_mask;
> > >> +  }
> > >> +
> > >> +  /* Try to set up intx irq */
> > >> +  pci_intx(pdev, 1);
> > >> +  privdata->eventval.buf = NULL;
> > >> +  privdata->requested = false;
> > >> +  raw_spin_lock_init(&privdata->lock);
> > >> +  rc = request_irq(pdev->irq, amd_mp2_irq_isr, IRQF_SHARED, "mp2_irq_isr",
> > >> +                   privdata);
> > >> +  if (rc)
> > >> +          goto err_intx_request;
> > >> +
> > >> +  return 0;
> > >> +
> > >> +err_intx_request:
> > >> +  return rc;
> > >> +err_dma_mask:
> > >> +  pci_clear_master(pdev);
> > >> +  pci_release_regions(pdev);
> > >> +err_pci_regions:
> > >> +  pci_disable_device(pdev);
> > >> +err_pci_enable:
> > >> +  pci_set_drvdata(pdev, NULL);
> > >> +  return rc;
> > >> +}
> > >> +
> > >> +static void amd_mp2_pci_deinit(struct amd_mp2_dev *privdata)
> > >> +{
> > >> +  struct pci_dev *pdev = ndev_pdev(privdata);
> > >> +
> > >> +  pci_iounmap(pdev, privdata->mmio);
> > >> +
> > >> +  pci_clear_master(pdev);
> > >> +  pci_release_regions(pdev);
> > >> +  pci_disable_device(pdev);
> > >> +  pci_set_drvdata(pdev, NULL);
> > >> +}
> > >> +
> > >> +static int amd_mp2_pci_probe(struct pci_dev *pdev,
> > >> +                       const struct pci_device_id *id)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata;
> > >> +  int rc;
> > >> +
> > >> +  dev_info(&pdev->dev, "MP2 device found [%04x:%04x] (rev %x)\n",
> > >> +           (int)pdev->vendor, (int)pdev->device, (int)pdev->revision);
> > >> +
> > >> +  privdata = kzalloc(sizeof(*privdata), GFP_KERNEL);
> > >> +  privdata->pdev = pdev;
> > >> +
> > >> +  if (!privdata) {
> > >> +          rc = -ENOMEM;
> > >> +          goto err_dev;
> > >> +  }
> > >> +
> > >> +  rc = amd_mp2_pci_init(privdata, pdev);
> > >> +  if (rc)
> > >> +          goto err_pci_init;
> > >> +  dev_dbg(&pdev->dev, "pci init done.\n");
> > >> +
> > >> +  INIT_DELAYED_WORK(&privdata->work, amd_mp2_pci_work);
> > >> +
> > >> +  amd_mp2_init_debugfs(privdata);
> > >> +  dev_info(&pdev->dev, "MP2 device registered.\n");
> > >> +  return 0;
> > >> +
> > >> +err_pci_init:
> > >> +  kfree(privdata);
> > >> +err_dev:
> > >> +  dev_err(&pdev->dev, "Memory Allocation Failed\n");
> > >> +  return rc;
> > >> +}
> > >> +
> > >> +static void amd_mp2_pci_remove(struct pci_dev *pdev)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata = pci_get_drvdata(pdev);
> > >> +
> > >> +  amd_mp2_deinit_debugfs(privdata);
> > >> +  amd_mp2_clear_reg(privdata);
> > >> +  cancel_delayed_work_sync(&privdata->work);
> > >> +  free_irq(pdev->irq, privdata);
> > >> +  pci_intx(pdev, 0);
> > >> +  amd_mp2_pci_deinit(privdata);
> > >> +  kfree(privdata);
> > >> +}
> > >> +
> > >> +static const struct file_operations amd_mp2_debugfs_info = {
> > >> +  .owner = THIS_MODULE,
> > >> +  .open = simple_open,
> > >> +  .read = amd_mp2_debugfs_read,
> > >> +};
> > >> +
> > >> +static const struct pci_device_id amd_mp2_pci_tbl[] = {
> > >> +  {PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_MP2)},
> > >> +  {0}
> > >> +};
> > >> +MODULE_DEVICE_TABLE(pci, amd_mp2_pci_tbl);
> > >> +
> > >> +#ifdef CONFIG_PM_SLEEP
> > >> +static int amd_mp2_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata = pci_get_drvdata(pdev);
> > >> +
> > >> +  if (!privdata)
> > >> +          return -EINVAL;
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +
> > >> +static int amd_mp2_pci_device_resume(struct pci_dev *pdev)
> > >> +{
> > >> +  struct amd_mp2_dev *privdata = pci_get_drvdata(pdev);
> > >> +
> > >> +  if (!privdata)
> > >> +          return -EINVAL;
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +#endif
> > >> +
> > >> +static struct pci_driver amd_mp2_pci_driver = {
> > >> +  .name           = DRIVER_NAME,
> > >> +  .id_table       = amd_mp2_pci_tbl,
> > >> +  .probe          = amd_mp2_pci_probe,
> > >> +  .remove         = amd_mp2_pci_remove,
> > >> +#ifdef CONFIG_PM_SLEEP
> > >> +  .suspend                = amd_mp2_pci_device_suspend,
> > >> +  .resume                 = amd_mp2_pci_device_resume,
> > >> +#endif
> > >> +};
> > >> +
> > >> +static int __init amd_mp2_pci_driver_init(void)
> > >> +{
> > >> +  pr_info("%s: %s Version: %s\n", DRIVER_NAME, DRIVER_DESC, DRIVER_VER);
>
> This sort of thing is annoying because if this driver is built into
> the kernel statically (not as a module), it *always* prints stuff,
> regardless of whether the device is present.  There *are* examples
> of that in the kernel, but I don't think they're good examples to
> follow.
>
> For example, I get the following useless messages on my laptop despite
> the fact that they apparently only apply to IBM xSeries x366 and x460
> systems.  I think this is just anti-social.
>
>   Calgary: detecting Calgary via BIOS EBDA area
>   Calgary: Unable to locate Rio Grande table in EBDA - bailing!
>
> If you need the driver name/desc/ver, you can always print it in
> amd_mp2_pci_probe() (just the first time through).
>
> > >> +
> > >> +  if (debugfs_initialized())
> > >> +          debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> > >> +
> > >> +  return pci_register_driver(&amd_mp2_pci_driver);
> > >> +}
> > >> +module_init(amd_mp2_pci_driver_init);
> > >> +
> > >> +static void __exit amd_mp2_pci_driver_exit(void)
> > >> +{
> > >> +  pci_unregister_driver(&amd_mp2_pci_driver);
> > >> +  debugfs_remove_recursive(debugfs_dir);
> > >> +}
> > >> +module_exit(amd_mp2_pci_driver_exit);
> > >> diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.h b/drivers/i2c/busses/i2c-amd-pci-mp2.h
> > >> new file mode 100644
> > >> index 0000000..787484e
> > >> --- /dev/null
> > >> +++ b/drivers/i2c/busses/i2c-amd-pci-mp2.h
> > >> @@ -0,0 +1,196 @@
> > >> +/* SPDX-License-Identifier: GPL-2.0
>
> Doesn't match the MODULE_LICENSE in the .c files.
>
> > >> + *
> > >> + * Copyright (C) 2018 Advanced Micro Devices, Inc. All Rights Reserved
> > >> + *
> > >> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> > >> + * AMD PCIe MP2 Communication Interface Driver
> > >> + */
> > >> +
> > >> +#ifndef I2C_AMD_PCI_MP2_H
> > >> +#define I2C_AMD_PCI_MP2_H
> > >> +
> > >> +#include <linux/pci.h>
> > >> +
> > >> +#define PCI_DEVICE_ID_AMD_MP2     0x15E6
> > >> +
> > >> +#define write64 _write64
> > >> +static inline void _write64(u64 val, void __iomem *mmio)
> > >> +{
> > >> +  writel(val, mmio);
> > >> +  writel(val >> 32, mmio + sizeof(u32));
> > >> +}
> > >> +
> > >> +#define read64 _read64
> > >> +static inline u64 _read64(void __iomem *mmio)
> > >> +{
> > >> +  u64 low, high;
> > >> +
> > >> +  low = readl(mmio);
> > >> +  high = readl(mmio + sizeof(u32));
> > >> +  return low | (high << 32);
> > >> +}
> > >> +
> > >> +enum {
> > >> +  /* MP2 C2P Message Registers */
> > >> +  AMD_C2P_MSG0 = 0x10500, /*MP2 Message for I2C0*/
>
> Generally people use a space after "/*" and another before "*/".
> You should at least be consistent for all your comments (some of them
> have the spaces and some don't).
>
> > >> +  AMD_C2P_MSG1 = 0x10504, /*MP2 Message for I2C1*/
> > >> +  AMD_C2P_MSG2 = 0x10508, /*DRAM Address Lo / Data 0*/
> > >> +  AMD_C2P_MSG3 = 0x1050c, /*DRAM Address HI / Data 1*/
> > >> +  AMD_C2P_MSG4 = 0x10510, /*Data 2*/
> > >> +  AMD_C2P_MSG5 = 0x10514, /*Data 3*/
> > >> +  AMD_C2P_MSG6 = 0x10518, /*Data 4*/
> > >> +  AMD_C2P_MSG7 = 0x1051c, /*Data 5*/
> > >> +  AMD_C2P_MSG8 = 0x10520, /*Data 6*/
> > >> +  AMD_C2P_MSG9 = 0x10524, /*Data 7*/
> > >> +
> > >> +  /* MP2 P2C Message Registers */
> > >> +  AMD_P2C_MSG0 = 0x10680, /*Do not use*/
> > >> +  AMD_P2C_MSG1 = 0x10684, /*I2c0 int reg*/
> > >> +  AMD_P2C_MSG2 = 0x10688, /*I2c1 int reg*/
> > >> +  AMD_P2C_MSG3 = 0x1068C, /*MP2 debug info*/
> > >> +  AMD_P2C_MSG_INTEN = 0x10690, /*MP2 int gen register*/
> > >> +  AMD_P2C_MSG_INTSTS = 0x10694, /*Interrupt sts*/
> > >> +};
> > >> +
> > >> +/* Command register data structures */
> > >> +
> > >> +enum i2c_cmd {
> > >> +  i2c_read,
> > >> +  i2c_write,
> > >> +  i2c_enable,
> > >> +  i2c_disable,
> > >> +  number_of_sensor_discovered,
> > >> +  is_mp2_active,
> > >> +  invalid_cmd = 0xF,
> > >> +};
> > >> +
> > >> +enum i2c_bus_index {
> > >> +  i2c_bus_0 = 0,
> > >> +  i2c_bus_1 = 1,
> > >> +  i2c_bus_max
> > >> +};
> > >> +
> > >> +enum speed_enum {
> > >> +  speed100k = 0,
> > >> +  speed400k = 1,
> > >> +  speed1000k = 2,
> > >> +  speed1400k = 3,
> > >> +  speed3400k = 4
> > >> +};
> > >> +
> > >> +enum mem_type {
> > >> +  use_dram = 0,
> > >> +  use_c2pmsg = 1,
> > >> +};
> > >> +
> > >> +union i2c_cmd_base {
> > >> +  u32 ul;
> > >> +  struct {
> > >> +          enum i2c_cmd i2c_cmd : 4; /*!< bit: 0..3 i2c R/W command */
> > >> +          enum i2c_bus_index bus_id : 4; /*!< bit: 4..7 i2c bus index */
> > >> +          u32 dev_addr : 8; /*!< bit: 8..15 device address or Bus Speed*/
> > >> +          u32 length : 12; /*!< bit: 16..29 read/write length */
> > >> +          enum speed_enum i2c_speed : 3; /*!< bit: 30 register address*/
> > >> +          enum mem_type mem_type : 1; /*!< bit: 15 mem type*/
> > >> +  } s; /*!< Structure used for bit access */
> > >> +};
> > >> +
> > >> +/* Response register data structures */
> > >> +
> > >> +/*Response - Response of SFI*/
> > >> +enum response_type {
> > >> +  invalid_response = 0,
> > >> +  command_success = 1,
> > >> +  command_failed = 2,
> > >> +};
> > >> +
> > >> +/*Status - Command ID to indicate a command*/
> > >> +enum status_type {
> > >> +  i2c_readcomplete_event = 0,
> > >> +  i2c_readfail_event = 1,
> > >> +  i2c_writecomplete_event = 2,
> > >> +  i2c_writefail_event = 3,
> > >> +  i2c_busenable_complete = 4,
> > >> +  i2c_busenable_failed = 5,
> > >> +  i2c_busdisable_complete = 6,
> > >> +  i2c_busdisable_failed = 7,
> > >> +  invalid_data_length = 8,
> > >> +  invalid_slave_address = 9,
> > >> +  invalid_i2cbus_id = 10,
> > >> +  invalid_dram_addr = 11,
> > >> +  invalid_command = 12,
> > >> +  mp2_active = 13,
> > >> +  numberof_sensors_discovered_resp = 14,
> > >> +  i2C_bus_notinitialized
> > >> +};
> > >> +
> > >> +struct i2c_event {
> > >> +  union {
> > >> +          u32 ul;
> > >> +          struct {
> > >> +                  enum response_type response : 2; /*!< bit: 0..1 I2C res type */
> > >> +                  enum status_type status : 5; /*!< bit: 2..6 status_type */
> > >> +                  enum mem_type mem_type : 1; /*!< bit: 7 0-DRAM;1- C2PMsg o/p */
> > >> +                  enum i2c_bus_index bus_id : 4; /*!< bit: 8..11 I2C Bus ID */
> > >> +                  u32 length : 12; /*!< bit:16..29 length */
> > >> +                  u32 slave_addr : 8; /*!< bit: 15 debug msg include in p2c msg */
> > >> +          } r; /*!< Structure used for bit access */
> > >> +  } base;
> > >> +  u32 *buf;
> > >> +};
> > >> +
> > >> +/* data structures for communication with I2c*/
> > >> +
> > >> +struct i2c_config {
> > >> +  enum i2c_bus_index bus_id;
> > >> +  u64 i2c_speed;
> > >> +  u16 dev_addr;
> > >> +  u32 length;
> > >> +  phys_addr_t phy_addr;
> > >> +  u8 *read_buf;
> > >> +  u32 *write_buf;
> > >> +};
> > >> +
> > >> +/* struct to send/receive data b/w pci and i2c drivers*/
> > >> +struct amd_i2c_pci_ops {
> > >> +  int (*read_complete)(struct i2c_event event, void *dev_ctx);
> > >> +  int (*write_complete)(struct i2c_event event, void *dev_ctx);
> > >> +  int (*connect_complete)(struct i2c_event event, void *dev_ctx);
> > >> +};
> > >> +
> > >> +struct amd_i2c_common {
> > >> +  struct i2c_config i2c_cfg;
> > >> +  const struct amd_i2c_pci_ops *ops;
> > >> +  struct pci_dev *pdev;
> > >> +};
> > >> +
> > >> +struct amd_mp2_dev {
> > >> +  struct pci_dev *pdev;
> > >> +  struct dentry *debugfs_dir;
> > >> +  struct dentry *debugfs_info;
> > >> +  void __iomem *mmio;
> > >> +  struct i2c_event eventval;
> > >> +  enum i2c_cmd reqcmd;
> > >> +  struct i2c_config i2c_cfg;
> > >> +  union i2c_cmd_base i2c_cmd_base;
> > >> +  const struct amd_i2c_pci_ops *ops;
> > >> +  struct delayed_work work;
> > >> +  void *i2c_dev_ctx;
> > >> +  bool requested;
> > >> +  raw_spinlock_t lock;
> > >> +};
> > >> +
> > >> +int amd_mp2_read(struct pci_dev *pdev, struct i2c_config i2c_cfg);
> > >> +int amd_mp2_write(struct pci_dev *pdev,
> > >> +            struct i2c_config i2c_cfg);
> > >> +int amd_mp2_connect(struct pci_dev *pdev,
> > >> +              struct i2c_config i2c_cfg);
> > >> +int amd_i2c_register_cb(struct pci_dev *pdev, const struct amd_i2c_pci_ops *ops,
> > >> +                  void *dev_ctx);
> > >> +
> > >> +#define ndev_pdev(ndev) ((ndev)->pdev)
> > >> +#define ndev_name(ndev) pci_name(ndev_pdev(ndev))
> > >> +#define ndev_dev(ndev) (&ndev_pdev(ndev)->dev)
> > >> +#define mp2_dev(__work) container_of(__work, struct amd_mp2_dev, work.work)
> > >> +
> > >> +#endif
> > >> diff --git a/drivers/i2c/busses/i2c-amd-platdrv.c b/drivers/i2c/busses/i2c-amd-platdrv.c
> > >> new file mode 100644
> > >> index 0000000..95e3b37
> > >> --- /dev/null
> > >> +++ b/drivers/i2c/busses/i2c-amd-platdrv.c
> > >> @@ -0,0 +1,277 @@
> > >> +// SPDX-License-Identifier: GPL-2.0
>
> Doesn't match MODULE_LICENSE.
>
> > >> +/*
> > >> + *
> > >> + * Copyright (C) 2018 Advanced Micro Devices, Inc. All Rights Reserved.
> > >> + *
> > >> + *
>
> Extra blank lines above.
>
> > >> + * Author: Nehal Bakulchandra Shah <Nehal-bakulchandra.shah@xxxxxxx>
> > >> + * AMD I2C Platform Driver
> > >> + */
> > >> +
> > >> +#include <linux/kernel.h>
> > >> +#include <linux/module.h>
> > >> +#include <linux/types.h>
> > >> +#include <linux/slab.h>
> > >> +#include <linux/i2c.h>
> > >> +#include <linux/platform_device.h>
> > >> +#include <linux/dma-mapping.h>
> > >> +#include <linux/acpi.h>
> > >> +#include <linux/delay.h>
> > >> +#include "i2c-amd-pci-mp2.h"
> > >> +
> > >> +struct amd_i2c_dev {
> > >> +  struct platform_device *pdev;
> > >> +  struct i2c_adapter adapter;
> > >> +  struct amd_i2c_common i2c_common;
> > >> +  struct completion msg_complete;
> > >> +  struct i2c_msg *msg_buf;
> > >> +  bool is_configured;
> > >> +  u8 bus_id;
> > >> +  u8 *buf;
> > >> +};
> > >> +
> > >> +static int i2c_amd_read_completion(struct i2c_event event, void *dev_ctx)
> > >> +{
> > >> +  struct amd_i2c_dev *i2c_dev = (struct amd_i2c_dev *)dev_ctx;
>
> I don't think this cast is necessary.
>
> > >> +  struct amd_i2c_common *commond = &i2c_dev->i2c_common;
> > >> +  int i = 0;
>
> Unnecessary initialization.
>
> > >> +
> > >> +  if (event.base.r.status == i2c_readcomplete_event) {
> > >> +          if (event.base.r.length <= 32) {
> > >> +                  pr_devel(" in %s i2c_dev->msg_buf :%p\n",
> > >> +                           __func__, i2c_dev->msg_buf);
> > >> +
> > >> +                  memcpy(i2c_dev->msg_buf->buf,
> > >> +                         (unsigned char *)event.buf, event.base.r.length);
> > >> +
> > >> +                  for (i = 0; i < ((event.base.r.length + 3) / 4); i++)
> > >> +                          pr_devel("%s: readdata:%x\n",
> > >> +                                   __func__, event.buf[i]);
> > >> +
> > >> +          } else {
> > >> +                  memcpy(i2c_dev->msg_buf->buf,
> > >> +                         (unsigned char *)commond->i2c_cfg.read_buf,
> > >> +                          event.base.r.length);
> > >> +
> > >> +                  for (i = 0; i < ((event.base.r.length + 3) / 4); i++)
> > >> +                          pr_devel("%s: readdata:%x\n", __func__,
> > >> +                                   ((unsigned int *)commond->i2c_cfg.read_buf)[i]);
> > >> +          }
> > >> +
> > >> +                          complete(&i2c_dev->msg_complete);
> > >> +  }
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +
> > >> +static int i2c_amd_write_completion(struct i2c_event event, void *dev_ctx)
> > >> +{
> > >> +  struct amd_i2c_dev *i2c_dev = (struct amd_i2c_dev *)dev_ctx;
>
> Unnecessary cast.
>
> > >> +
> > >> +  if (event.base.r.status == i2c_writecomplete_event)
> > >> +                  complete(&i2c_dev->msg_complete);
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +
> > >> +static int i2c_amd_connect_completion(struct i2c_event event, void *dev_ctx)
> > >> +{
> > >> +  struct amd_i2c_dev *i2c_dev = (struct amd_i2c_dev *)dev_ctx;
>
> Unnecessary cast.
>
> > >> +
> > >> +  if (event.base.r.status == i2c_busenable_complete)
> > >> +                  complete(&i2c_dev->msg_complete);
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +
> > >> +static const struct amd_i2c_pci_ops data_handler = {
> > >> +          .read_complete = i2c_amd_read_completion,
> > >> +          .write_complete = i2c_amd_write_completion,
> > >> +          .connect_complete = i2c_amd_connect_completion,
> > >> +};
> > >> +
> > >> +static int i2c_amd_pci_configure(struct amd_i2c_dev *i2c_dev, int slaveaddr)
> > >> +{
> > >> +  struct amd_i2c_common *i2c_common = &i2c_dev->i2c_common;
> > >> +  int ret;
> > >> +
> > >> +  amd_i2c_register_cb(i2c_common->pdev, &data_handler, (void *)i2c_dev);
> > >> +  i2c_common->i2c_cfg.bus_id = i2c_dev->bus_id;
> > >> +  i2c_common->i2c_cfg.dev_addr = slaveaddr;
> > >> +  i2c_common->i2c_cfg.i2c_speed = speed400k;
> > >> +  ret = amd_mp2_connect(i2c_common->pdev, i2c_common->i2c_cfg);
> > >> +
> > >> +  if (ret)
> > >> +          return  -EIO;
> > >> +
> > >> +  mdelay(100);
>
> This deserves a comment about why the delay is needed.
>
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +
> > >> +static int i2c_amd_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> > >> +{
> > >> +  struct amd_i2c_dev *dev = i2c_get_adapdata(adap);
> > >> +  struct amd_i2c_common *i2c_common = &dev->i2c_common;
> > >> +
>
> Remove this blank line.
>
> > >> +  int i;
> > >> +  unsigned long timeout;
> > >> +  struct i2c_msg *pmsg;
> > >> +  unsigned char *dma_buf = NULL;
> > >> +  dma_addr_t phys;
> > >> +
> > >> +  reinit_completion(&dev->msg_complete);
> > >> +  if (dev->is_configured == 0) {
> > >> +          i2c_amd_pci_configure(dev, msgs->addr);
> > >> +          timeout = wait_for_completion_timeout(&dev->msg_complete, 50);
> > >> +          dev->is_configured = 1;
> > >> +  }
> > >> +
> > >> +  for (i = 0; i < num; i++) {
> > >> +          pmsg = &msgs[i];
> > >> +          i2c_common->i2c_cfg.length = pmsg->len;
> > >> +          if (pmsg->len > 32) {
> > >> +                  dma_buf = (u8 *)dma_alloc_coherent(&i2c_common->pdev->dev,
> > >> +                                  pmsg->len, &phys, GFP_KERNEL);
> > >> +                  i2c_common->i2c_cfg.phy_addr = phys;
> > >> +          }
> > >> +
> > >> +          if (pmsg->flags & I2C_M_RD) {
> > >> +                  if (pmsg->len <= 32)
> > >> +                          i2c_common->i2c_cfg.read_buf = dev->buf;
> > >> +                  else
> > >> +                          i2c_common->i2c_cfg.read_buf = dma_buf;
> > >> +
> > >> +                  dev->msg_buf = pmsg;
> > >> +                  amd_mp2_read(i2c_common->pdev,
> > >> +                               i2c_common->i2c_cfg);
> > >> +
> > >> +          } else {
> > >> +                  if (pmsg->len <= 32)
> > >> +                          i2c_common->i2c_cfg.write_buf = (unsigned int *)pmsg->buf;
> > >> +                  else
> > >> +                          i2c_common->i2c_cfg.write_buf = (unsigned int *)dma_buf;
> > >> +
> > >> +                  amd_mp2_write(i2c_common->pdev,
> > >> +                                i2c_common->i2c_cfg);
> > >> +          }
> > >> +
> > >> +          timeout = wait_for_completion_timeout
> > >> +                                  (&dev->msg_complete, 50);
> > >> +          if (dma_buf)
> > >> +                  dma_free_coherent(&i2c_common->pdev->dev,
> > >> +                                    pmsg->len, dma_buf, phys);
> > >> +
> > >> +          if (timeout == 0)
> > >> +                  return -ETIMEDOUT;
> > >> +  }
> > >> +  return num;
> > >> +}
> > >> +
> > >> +static u32 i2c_amd_func(struct i2c_adapter *a)
> > >> +{
> > >> +  return I2C_FUNC_I2C;
> > >> +}
> > >> +
> > >> +static const struct i2c_algorithm i2c_amd_algorithm = {
> > >> +  .master_xfer = i2c_amd_xfer,
> > >> +  .functionality = i2c_amd_func,
> > >> +};
> > >> +
> > >> +static int i2c_amd_probe(struct platform_device *pdev)
> > >> +{
> > >> +  int ret;
> > >> +  struct amd_i2c_dev *i2c_dev;
> > >> +  struct device *dev = &pdev->dev;
> > >> +  acpi_handle handle = ACPI_HANDLE(&pdev->dev);
> > >> +  struct acpi_device *adev;
> > >> +  const char *uid = NULL;
> > >> +
> > >> +  i2c_dev = devm_kzalloc(dev, sizeof(*i2c_dev), GFP_KERNEL);
> > >> +  if (!i2c_dev)
> > >> +          return -ENOMEM;
> > >> +
> > >> +  i2c_dev->pdev = pdev;
> > >> +
> > >> +  if (!acpi_bus_get_device(handle, &adev)) {
> > >> +          pr_err(" i2c0  pdev->id=%s\n", adev->pnp.unique_id);
>
> All the printks here should be some form of dev_printk(), probably
> using the platform_device.  All driver printks should use dev_printk()
> so the messages can be associated with a specific device.
>
> > >> +          uid = adev->pnp.unique_id;
> > >> +  }
> > >> +
> > >> +  if (strcmp(uid, "0") == 0) {
>
> This is unsafe: uid may be NULL here, and strcmp will dereference it.
>
> > >> +          pr_err(" bus id is 0\n");
> > >> +          i2c_dev->bus_id = 0;
> > >> +  }
> > >> +
> > >> +  pr_devel(" i2c1  pdev->id=%s\n", uid);
> > >> +  if (strcmp(uid, "1") == 0) {
> > >> +          pr_err(" bus id is 1\n");
> > >> +          i2c_dev->bus_id = 1;
> > >> +  }
> > >> +  /* setup i2c adapter description */
> > >> +  i2c_dev->adapter.owner = THIS_MODULE;
> > >> +  i2c_dev->adapter.algo = &i2c_amd_algorithm;
> > >> +  i2c_dev->adapter.dev.parent = dev;
> > >> +  i2c_dev->adapter.algo_data = i2c_dev;
> > >> +  ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev));
>
> I don't understand what's going on here.  You seem to be claiming
> AMDI0011 devices, but I can't see that you ever *do* anything with
> that.  I don't see any methods on that device being called or any
> information from the ACPI namespace being used.  What am I missing?
>
> > >> +  i2c_dev->adapter.dev.of_node = dev->of_node;
> > >> +  /*buffer for 32 bytes of read  data*/
>
> Extra space in "read  data", and not enough at beginning and end.
>
> > >> +  i2c_dev->buf = kzalloc(32, GFP_KERNEL);
> > >> +
> > >> +  if (!i2c_dev->buf)
> > >> +          return -ENOMEM;
> > >> +
> > >> +  snprintf(i2c_dev->adapter.name, sizeof(i2c_dev->adapter.name), "%s-%s",
> > >> +           "i2c_dev-i2c", dev_name(pdev->dev.parent));
> > >> +
> > >> +  i2c_dev->i2c_common.pdev = pci_get_device(PCI_VENDOR_ID_AMD,
> > >> +                                            PCI_DEVICE_ID_AMD_MP2, NULL);
>
> I really don't like this.  pci_get_device() basically subverts the PCI
> driver registration mechanism.  If a driver has claimed the MP2 device
> using pci_register_driver(), then *it* completely owns the device and
> no other code should touch it.  pci_get_device() is a way for
> arbitrary other code to get a pointer to the device and muck with it.
>
> In this case, I'm guessing the driver claiming the MP2 device is
> probably another piece of *this* driver, and you should have some
> mechanism internal to the driver to coordinate knowledge of the device
> and access to it.
>
> > >> +
> > >> +  if (!i2c_dev->i2c_common.pdev) {
> > >> +          pr_err("%s Could not find pdev in i2c\n", __func__);
> > >> +          return -EINVAL;
> > >> +  }
> > >> +  platform_set_drvdata(pdev, i2c_dev);
> > >> +  i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
> > >> +
> > >> +  init_completion(&i2c_dev->msg_complete);
> > >> +  /* and finally attach to i2c layer */
> > >> +  ret = i2c_add_adapter(&i2c_dev->adapter);
> > >> +
> > >> +  if (ret < 0)
> > >> +          pr_err(" i2c add adpater failed =%d", ret);
>
> s/adpater/adapter/
>
> > >> +
> > >> +  return ret;
> > >> +}
> > >> +
> > >> +static int i2c_amd_remove(struct platform_device *pdev)
> > >> +{
> > >> +  struct amd_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> > >> +
> > >> +  kfree(i2c_dev->buf);
> > >> +
> > >> +  i2c_del_adapter(&i2c_dev->adapter);
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +
> > >> +static const struct acpi_device_id i2c_amd_acpi_match[] = {
> > >> +          { "AMDI0011" },
> > >> +          { },
> > >> +};
> > >> +
> > >> +static struct platform_driver amd_i2c_plat_driver = {
> > >> +          .probe = i2c_amd_probe,
> > >> +          .remove = i2c_amd_remove,
> > >> +          .driver = {
> > >> +                          .name = "i2c_amd_platdrv",
> > >> +                          .acpi_match_table = ACPI_PTR
> > >> +                                          (i2c_amd_acpi_match),
> > >> +          },
> > >> +};
> > >> +
> > >> +module_platform_driver(amd_i2c_plat_driver);
> > >> +
> > >> +MODULE_AUTHOR("Nehal Shah <nehal-bakulchandra.shah@xxxxxxx>");
> > >> +MODULE_DESCRIPTION("AMD I2C Platform Driver");
> > >> +MODULE_LICENSE("Dual BSD/GPL");
> > >> --
> > >> 2.7.4
> > >
> > >
> >