[PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_*

[PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_*

[Andy Shevchenko]

The error handling is broken right now since it leaves resources unfreed.
Convert the code to use managed resources to fix the error handling.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_scu_ipc.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 187d108..7148535 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -563,7 +563,6 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int err;
 	struct intel_scu_ipc_pdata_t *pdata;
-	resource_size_t base;
 
 	if (ipcdev.pdev)		/* We support only one SCU */
 		return -EBUSY;
@@ -573,32 +572,26 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	ipcdev.pdev = pci_dev_get(dev);
 	ipcdev.irq_mode = pdata->irq_mode;
 
-	err = pci_enable_device(dev);
+	err = pcim_enable_device(dev);
 	if (err)
 		return err;
 
-	err = pci_request_regions(dev, "intel_scu_ipc");
+	err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev));
 	if (err)
 		return err;
 
-	base = pci_resource_start(dev, 0);
-	if (!base)
-		return -ENOMEM;
-
 	init_completion(&ipcdev.cmd_complete);
 
-	if (request_irq(dev->irq, ioc, 0, "intel_scu_ipc", &ipcdev))
-		return -EBUSY;
+	err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
+			       &ipcdev);
+	if (err)
+		return err;
 
-	ipcdev.ipc_base = ioremap_nocache(base, pci_resource_len(dev, 0));
-	if (!ipcdev.ipc_base)
-		return -ENOMEM;
+	ipcdev.ipc_base = pcim_iomap_table(dev)[0];
 
 	ipcdev.i2c_base = ioremap_nocache(pdata->i2c_base, pdata->i2c_len);
-	if (!ipcdev.i2c_base) {
-		iounmap(ipcdev.ipc_base);
+	if (!ipcdev.i2c_base)
 		return -ENOMEM;
-	}
 
 	intel_scu_devices_create();
 
@@ -617,10 +610,7 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
  */
 static void ipc_remove(struct pci_dev *pdev)
 {
-	free_irq(pdev->irq, &ipcdev);
-	pci_release_regions(pdev);
 	pci_dev_put(ipcdev.pdev);
-	iounmap(ipcdev.ipc_base);
 	iounmap(ipcdev.i2c_base);
 	ipcdev.pdev = NULL;
 	intel_scu_devices_destroy();
-- 
2.5.3

[PATCH 2/5] intel_scu_ipc: propagate struct intel_scu_ipc_dev

[Andy Shevchenko]

As much as poosible propagate struct intel_scu_ipc_dev.

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_scu_ipc.c | 134 +++++++++++++++++++----------------
 1 file changed, 74 insertions(+), 60 deletions(-)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 7148535..6c9367f 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -118,28 +118,30 @@ static struct intel_scu_ipc_dev  ipcdev; /* Only one for now */
 static DEFINE_MUTEX(ipclock); /* lock used to prevent multiple call to SCU */
 
 /*
+ * Send ipc command
  * Command Register (Write Only):
  * A write to this register results in an interrupt to the SCU core processor
  * Format:
  * |rfu2(8) | size(8) | command id(4) | rfu1(3) | ioc(1) | command(8)|
  */
-static inline void ipc_command(u32 cmd) /* Send ipc command */
+static inline void ipc_command(struct intel_scu_ipc_dev *scu, u32 cmd)
 {
-	if (ipcdev.irq_mode) {
-		reinit_completion(&ipcdev.cmd_complete);
-		writel(cmd | IPC_IOC, ipcdev.ipc_base);
+	if (scu->irq_mode) {
+		reinit_completion(&scu->cmd_complete);
+		writel(cmd | IPC_IOC, scu->ipc_base);
 	}
-	writel(cmd, ipcdev.ipc_base);
+	writel(cmd, scu->ipc_base);
 }
 
 /*
+ * Write ipc data
  * IPC Write Buffer (Write Only):
  * 16-byte buffer for sending data associated with IPC command to
  * SCU. Size of the data is specified in the IPC_COMMAND_REG register
  */
-static inline void ipc_data_writel(u32 data, u32 offset) /* Write ipc data */
+static inline void ipc_data_writel(struct intel_scu_ipc_dev *scu, u32 data, u32 offset)
 {
-	writel(data, ipcdev.ipc_base + 0x80 + offset);
+	writel(data, scu->ipc_base + 0x80 + offset);
 }
 
 /*
@@ -149,35 +151,37 @@ static inline void ipc_data_writel(u32 data, u32 offset) /* Write ipc data */
  * Format:
  * |rfu3(8)|error code(8)|initiator id(8)|cmd id(4)|rfu1(2)|error(1)|busy(1)|
  */
-static inline u8 ipc_read_status(void)
+static inline u8 ipc_read_status(struct intel_scu_ipc_dev *scu)
 {
-	return __raw_readl(ipcdev.ipc_base + 0x04);
+	return __raw_readl(scu->ipc_base + 0x04);
 }
 
-static inline u8 ipc_data_readb(u32 offset) /* Read ipc byte data */
+/* Read ipc byte data */
+static inline u8 ipc_data_readb(struct intel_scu_ipc_dev *scu, u32 offset)
 {
-	return readb(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
+	return readb(scu->ipc_base + IPC_READ_BUFFER + offset);
 }
 
-static inline u32 ipc_data_readl(u32 offset) /* Read ipc u32 data */
+/* Read ipc u32 data */
+static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
 {
-	return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
+	return readl(scu->ipc_base + IPC_READ_BUFFER + offset);
 }
 
 /* Wait till scu status is busy */
-static inline int busy_loop(void)
+static inline int busy_loop(struct intel_scu_ipc_dev *scu)
 {
-	u32 status = ipc_read_status();
+	u32 status = ipc_read_status(scu);
 	u32 loop_count = 100000;
 
 	/* break if scu doesn't reset busy bit after huge retry */
 	while ((status & BIT(0)) && --loop_count) {
 		udelay(1); /* scu processing time is in few u secods */
-		status = ipc_read_status();
+		status = ipc_read_status(scu);
 	}
 
 	if (status & BIT(0)) {
-		dev_err(&ipcdev.pdev->dev, "IPC timed out");
+		dev_err(&scu->pdev->dev, "IPC timed out");
 		return -ETIMEDOUT;
 	}
 
@@ -188,31 +192,32 @@ static inline int busy_loop(void)
 }
 
 /* Wait till ipc ioc interrupt is received or timeout in 3 HZ */
-static inline int ipc_wait_for_interrupt(void)
+static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev *scu)
 {
 	int status;
 
-	if (!wait_for_completion_timeout(&ipcdev.cmd_complete, 3 * HZ)) {
-		struct device *dev = &ipcdev.pdev->dev;
+	if (!wait_for_completion_timeout(&scu->cmd_complete, 3 * HZ)) {
+		struct device *dev = &scu->pdev->dev;
 		dev_err(dev, "IPC timed out\n");
 		return -ETIMEDOUT;
 	}
 
-	status = ipc_read_status();
+	status = ipc_read_status(scu);
 	if (status & BIT(1))
 		return -EIO;
 
 	return 0;
 }
 
-static int intel_scu_ipc_check_status(void)
+static int intel_scu_ipc_check_status(struct intel_scu_ipc_dev *scu)
 {
-	return ipcdev.irq_mode ? ipc_wait_for_interrupt() : busy_loop();
+	return scu->irq_mode ? ipc_wait_for_interrupt(scu) : busy_loop(scu);
 }
 
 /* Read/Write power control(PMIC in Langwell, MSIC in PenWell) registers */
 static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 {
+	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int nc;
 	u32 offset = 0;
 	int err;
@@ -223,7 +228,7 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 
 	mutex_lock(&ipclock);
 
-	if (ipcdev.pdev == NULL) {
+	if (scu->pdev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
@@ -235,27 +240,27 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 
 	if (id == IPC_CMD_PCNTRL_R) {
 		for (nc = 0, offset = 0; nc < count; nc++, offset += 4)
-			ipc_data_writel(wbuf[nc], offset);
-		ipc_command((count * 2) << 16 | id << 12 | 0 << 8 | op);
+			ipc_data_writel(scu, wbuf[nc], offset);
+		ipc_command(scu, (count * 2) << 16 | id << 12 | 0 << 8 | op);
 	} else if (id == IPC_CMD_PCNTRL_W) {
 		for (nc = 0; nc < count; nc++, offset += 1)
 			cbuf[offset] = data[nc];
 		for (nc = 0, offset = 0; nc < count; nc++, offset += 4)
-			ipc_data_writel(wbuf[nc], offset);
-		ipc_command((count * 3) << 16 | id << 12 | 0 << 8 | op);
+			ipc_data_writel(scu, wbuf[nc], offset);
+		ipc_command(scu, (count * 3) << 16 | id << 12 | 0 << 8 | op);
 	} else if (id == IPC_CMD_PCNTRL_M) {
 		cbuf[offset] = data[0];
 		cbuf[offset + 1] = data[1];
-		ipc_data_writel(wbuf[0], 0); /* Write wbuff */
-		ipc_command(4 << 16 | id << 12 | 0 << 8 | op);
+		ipc_data_writel(scu, wbuf[0], 0); /* Write wbuff */
+		ipc_command(scu, 4 << 16 | id << 12 | 0 << 8 | op);
 	}
 
-	err = intel_scu_ipc_check_status();
+	err = intel_scu_ipc_check_status(scu);
 	if (!err && id == IPC_CMD_PCNTRL_R) { /* Read rbuf */
 		/* Workaround: values are read as 0 without memcpy_fromio */
-		memcpy_fromio(cbuf, ipcdev.ipc_base + 0x90, 16);
+		memcpy_fromio(cbuf, scu->ipc_base + 0x90, 16);
 		for (nc = 0; nc < count; nc++)
-			data[nc] = ipc_data_readb(nc);
+			data[nc] = ipc_data_readb(scu, nc);
 	}
 	mutex_unlock(&ipclock);
 	return err;
@@ -436,15 +441,16 @@ EXPORT_SYMBOL(intel_scu_ipc_update_register);
  */
 int intel_scu_ipc_simple_command(int cmd, int sub)
 {
+	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int err;
 
 	mutex_lock(&ipclock);
-	if (ipcdev.pdev == NULL) {
+	if (scu->pdev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
-	ipc_command(sub << 12 | cmd);
-	err = intel_scu_ipc_check_status();
+	ipc_command(scu, sub << 12 | cmd);
+	err = intel_scu_ipc_check_status(scu);
 	mutex_unlock(&ipclock);
 	return err;
 }
@@ -465,23 +471,24 @@ EXPORT_SYMBOL(intel_scu_ipc_simple_command);
 int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
 			  u32 *out, int outlen)
 {
+	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int i, err;
 
 	mutex_lock(&ipclock);
-	if (ipcdev.pdev == NULL) {
+	if (scu->pdev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
 
 	for (i = 0; i < inlen; i++)
-		ipc_data_writel(*in++, 4 * i);
+		ipc_data_writel(scu, *in++, 4 * i);
 
-	ipc_command((inlen << 16) | (sub << 12) | cmd);
-	err = intel_scu_ipc_check_status();
+	ipc_command(scu, (inlen << 16) | (sub << 12) | cmd);
+	err = intel_scu_ipc_check_status(scu);
 
 	if (!err) {
 		for (i = 0; i < outlen; i++)
-			*out++ = ipc_data_readl(4 * i);
+			*out++ = ipc_data_readl(scu, 4 * i);
 	}
 
 	mutex_unlock(&ipclock);
@@ -507,25 +514,26 @@ EXPORT_SYMBOL(intel_scu_ipc_command);
  */
 int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 {
+	struct intel_scu_ipc_dev *scu = &ipcdev;
 	u32 cmd = 0;
 
 	mutex_lock(&ipclock);
-	if (ipcdev.pdev == NULL) {
+	if (scu->pdev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
 	cmd = (addr >> 24) & 0xFF;
 	if (cmd == IPC_I2C_READ) {
-		writel(addr, ipcdev.i2c_base + IPC_I2C_CNTRL_ADDR);
+		writel(addr, scu->i2c_base + IPC_I2C_CNTRL_ADDR);
 		/* Write not getting updated without delay */
 		mdelay(1);
-		*data = readl(ipcdev.i2c_base + I2C_DATA_ADDR);
+		*data = readl(scu->i2c_base + I2C_DATA_ADDR);
 	} else if (cmd == IPC_I2C_WRITE) {
-		writel(*data, ipcdev.i2c_base + I2C_DATA_ADDR);
+		writel(*data, scu->i2c_base + I2C_DATA_ADDR);
 		mdelay(1);
-		writel(addr, ipcdev.i2c_base + IPC_I2C_CNTRL_ADDR);
+		writel(addr, scu->i2c_base + IPC_I2C_CNTRL_ADDR);
 	} else {
-		dev_err(&ipcdev.pdev->dev,
+		dev_err(&scu->pdev->dev,
 			"intel_scu_ipc: I2C INVALID_CMD = 0x%x\n", cmd);
 
 		mutex_unlock(&ipclock);
@@ -545,8 +553,10 @@ EXPORT_SYMBOL(intel_scu_ipc_i2c_cntrl);
  */
 static irqreturn_t ioc(int irq, void *dev_id)
 {
-	if (ipcdev.irq_mode)
-		complete(&ipcdev.cmd_complete);
+	struct intel_scu_ipc_dev *scu = dev_id;
+
+	if (scu->irq_mode)
+		complete(&scu->cmd_complete);
 
 	return IRQ_HANDLED;
 }
@@ -562,15 +572,16 @@ static irqreturn_t ioc(int irq, void *dev_id)
 static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int err;
+	struct intel_scu_ipc_dev *scu = &ipcdev;
 	struct intel_scu_ipc_pdata_t *pdata;
 
-	if (ipcdev.pdev)		/* We support only one SCU */
+	if (scu->pdev)		/* We support only one SCU */
 		return -EBUSY;
 
 	pdata = (struct intel_scu_ipc_pdata_t *)id->driver_data;
 
-	ipcdev.pdev = pci_dev_get(dev);
-	ipcdev.irq_mode = pdata->irq_mode;
+	scu->pdev = pci_dev_get(dev);
+	scu->irq_mode = pdata->irq_mode;
 
 	err = pcim_enable_device(dev);
 	if (err)
@@ -580,21 +591,22 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (err)
 		return err;
 
-	init_completion(&ipcdev.cmd_complete);
+	init_completion(&scu->cmd_complete);
 
 	err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
-			       &ipcdev);
+			       scu);
 	if (err)
 		return err;
 
-	ipcdev.ipc_base = pcim_iomap_table(dev)[0];
+	scu->ipc_base = pcim_iomap_table(dev)[0];
 
-	ipcdev.i2c_base = ioremap_nocache(pdata->i2c_base, pdata->i2c_len);
-	if (!ipcdev.i2c_base)
+	scu->i2c_base = ioremap_nocache(pdata->i2c_base, pdata->i2c_len);
+	if (!scu->i2c_base)
 		return -ENOMEM;
 
 	intel_scu_devices_create();
 
+	pci_set_drvdata(dev, scu);
 	return 0;
 }
 
@@ -610,9 +622,11 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
  */
 static void ipc_remove(struct pci_dev *pdev)
 {
-	pci_dev_put(ipcdev.pdev);
-	iounmap(ipcdev.i2c_base);
-	ipcdev.pdev = NULL;
+	struct intel_scu_ipc_dev *scu = pci_get_drvdata(pdev);
+
+	pci_dev_put(scu->pdev);
+	scu->pdev = NULL;
+	iounmap(scu->i2c_base);
 	intel_scu_devices_destroy();
 }
 
-- 
2.5.3

[PATCH 3/5] intel_scu_ipc: convert to use struct device *

[Andy Shevchenko]

Switch the code to use struct device * instead of struct pci_dev * since there
is no reason to accee PCI related features in the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_scu_ipc.c | 41 ++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 6c9367f..69e39cb 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -92,11 +92,8 @@ static struct intel_scu_ipc_pdata_t intel_scu_ipc_tangier_pdata = {
 	.irq_mode = 0,
 };
 
-static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id);
-static void ipc_remove(struct pci_dev *pdev);
-
 struct intel_scu_ipc_dev {
-	struct pci_dev *pdev;
+	struct device *dev;
 	void __iomem *ipc_base;
 	void __iomem *i2c_base;
 	struct completion cmd_complete;
@@ -181,7 +178,7 @@ static inline int busy_loop(struct intel_scu_ipc_dev *scu)
 	}
 
 	if (status & BIT(0)) {
-		dev_err(&scu->pdev->dev, "IPC timed out");
+		dev_err(scu->dev, "IPC timed out");
 		return -ETIMEDOUT;
 	}
 
@@ -197,8 +194,7 @@ static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev *scu)
 	int status;
 
 	if (!wait_for_completion_timeout(&scu->cmd_complete, 3 * HZ)) {
-		struct device *dev = &scu->pdev->dev;
-		dev_err(dev, "IPC timed out\n");
+		dev_err(scu->dev, "IPC timed out\n");
 		return -ETIMEDOUT;
 	}
 
@@ -228,7 +224,7 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 
 	mutex_lock(&ipclock);
 
-	if (scu->pdev == NULL) {
+	if (scu->dev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
@@ -445,7 +441,7 @@ int intel_scu_ipc_simple_command(int cmd, int sub)
 	int err;
 
 	mutex_lock(&ipclock);
-	if (scu->pdev == NULL) {
+	if (scu->dev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
@@ -475,7 +471,7 @@ int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
 	int i, err;
 
 	mutex_lock(&ipclock);
-	if (scu->pdev == NULL) {
+	if (scu->dev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
@@ -518,7 +514,7 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 	u32 cmd = 0;
 
 	mutex_lock(&ipclock);
-	if (scu->pdev == NULL) {
+	if (scu->dev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
@@ -533,7 +529,7 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 		mdelay(1);
 		writel(addr, scu->i2c_base + IPC_I2C_CNTRL_ADDR);
 	} else {
-		dev_err(&scu->pdev->dev,
+		dev_err(scu->dev,
 			"intel_scu_ipc: I2C INVALID_CMD = 0x%x\n", cmd);
 
 		mutex_unlock(&ipclock);
@@ -563,42 +559,42 @@ static irqreturn_t ioc(int irq, void *dev_id)
 
 /**
  *	ipc_probe	-	probe an Intel SCU IPC
- *	@dev: the PCI device matching
+ *	@pdev: the PCI device matching
  *	@id: entry in the match table
  *
  *	Enable and install an intel SCU IPC. This appears in the PCI space
  *	but uses some hard coded addresses as well.
  */
-static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
+static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	int err;
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	struct intel_scu_ipc_pdata_t *pdata;
 
-	if (scu->pdev)		/* We support only one SCU */
+	if (scu->dev)		/* We support only one SCU */
 		return -EBUSY;
 
 	pdata = (struct intel_scu_ipc_pdata_t *)id->driver_data;
 
-	scu->pdev = pci_dev_get(dev);
+	scu->dev = &pdev->dev;
 	scu->irq_mode = pdata->irq_mode;
 
-	err = pcim_enable_device(dev);
+	err = pcim_enable_device(pdev);
 	if (err)
 		return err;
 
-	err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev));
+	err = pcim_iomap_regions(pdev, 1 << 0, pci_name(pdev));
 	if (err)
 		return err;
 
 	init_completion(&scu->cmd_complete);
 
-	err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
+	err = devm_request_irq(&pdev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
 			       scu);
 	if (err)
 		return err;
 
-	scu->ipc_base = pcim_iomap_table(dev)[0];
+	scu->ipc_base = pcim_iomap_table(pdev)[0];
 
 	scu->i2c_base = ioremap_nocache(pdata->i2c_base, pdata->i2c_len);
 	if (!scu->i2c_base)
@@ -606,7 +602,7 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	intel_scu_devices_create();
 
-	pci_set_drvdata(dev, scu);
+	pci_set_drvdata(pdev, scu);
 	return 0;
 }
 
@@ -624,8 +620,7 @@ static void ipc_remove(struct pci_dev *pdev)
 {
 	struct intel_scu_ipc_dev *scu = pci_get_drvdata(pdev);
 
-	pci_dev_put(scu->pdev);
-	scu->pdev = NULL;
+	scu->dev = NULL;
 	iounmap(scu->i2c_base);
 	intel_scu_devices_destroy();
 }
-- 
2.5.3

[PATCH 4/5] intel_scu_ipc: switch to use module_pci_driver() macro

[Andy Shevchenko]

The use of macro cleans up the code a bit.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_scu_ipc.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 69e39cb..9de2029 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -567,10 +567,15 @@ static irqreturn_t ioc(int irq, void *dev_id)
  */
 static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	int platform;		/* Platform type */
 	int err;
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	struct intel_scu_ipc_pdata_t *pdata;
 
+	platform = intel_mid_identify_cpu();
+	if (platform == 0)
+		return -ENODEV;
+
 	if (scu->dev)		/* We support only one SCU */
 		return -EBUSY;
 
@@ -589,7 +594,7 @@ static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	init_completion(&scu->cmd_complete);
 
-	err = devm_request_irq(&pdev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
+	err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_scu_ipc",
 			       scu);
 	if (err)
 		return err;
@@ -651,24 +656,8 @@ static struct pci_driver ipc_driver = {
 	.remove = ipc_remove,
 };
 
-static int __init intel_scu_ipc_init(void)
-{
-	int platform;		/* Platform type */
-
-	platform = intel_mid_identify_cpu();
-	if (platform == 0)
-		return -ENODEV;
-	return  pci_register_driver(&ipc_driver);
-}
-
-static void __exit intel_scu_ipc_exit(void)
-{
-	pci_unregister_driver(&ipc_driver);
-}
+module_pci_driver(ipc_driver);
 
 MODULE_AUTHOR("Sreedhara DS <sreedhara.ds@intel.com>");
 MODULE_DESCRIPTION("Intel SCU IPC driver");
 MODULE_LICENSE("GPL");
-
-module_init(intel_scu_ipc_init);
-module_exit(intel_scu_ipc_exit);
-- 
2.5.3

Re: [PATCH 4/5] intel_scu_ipc: switch to use module_pci_driver() macro

[Darren Hart]

On Fri, Oct 09, 2015 at 05:11:35PM +0300, Andy Shevchenko wrote:
> The use of macro cleans up the code a bit.

Thanks Andriy,

Minor point of feedback: it helps review to state specifics of how something is
cleaned up, etc. That way I am not assuming anything and can verify your
intentions with what I'm reading. This is a trivial example, but illustrative.

For this patch, you might have said:

  Eliminate some boilerplate code by using module_pci_driver() instead of
  init/exit, moving the salient bits from init into probe.

This provides more context for the reviewer and makes the intent explicit from
the commit message.

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel_scu_ipc.c | 25 +++++++------------------
>  1 file changed, 7 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index 69e39cb..9de2029 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -567,10 +567,15 @@ static irqreturn_t ioc(int irq, void *dev_id)
>   */
>  static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> +	int platform;		/* Platform type */
>  	int err;
>  	struct intel_scu_ipc_dev *scu = &ipcdev;
>  	struct intel_scu_ipc_pdata_t *pdata;
>  
> +	platform = intel_mid_identify_cpu();
> +	if (platform == 0)
> +		return -ENODEV;
> +
>  	if (scu->dev)		/* We support only one SCU */
>  		return -EBUSY;
>  
> @@ -589,7 +594,7 @@ static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	init_completion(&scu->cmd_complete);
>  
> -	err = devm_request_irq(&pdev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
> +	err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_scu_ipc",
>  			       scu);
>  	if (err)
>  		return err;
> @@ -651,24 +656,8 @@ static struct pci_driver ipc_driver = {
>  	.remove = ipc_remove,
>  };
>  
> -static int __init intel_scu_ipc_init(void)
> -{
> -	int platform;		/* Platform type */
> -
> -	platform = intel_mid_identify_cpu();
> -	if (platform == 0)
> -		return -ENODEV;
> -	return  pci_register_driver(&ipc_driver);
> -}
> -
> -static void __exit intel_scu_ipc_exit(void)
> -{
> -	pci_unregister_driver(&ipc_driver);
> -}
> +module_pci_driver(ipc_driver);
>  
>  MODULE_AUTHOR("Sreedhara DS <sreedhara.ds@intel.com>");
>  MODULE_DESCRIPTION("Intel SCU IPC driver");
>  MODULE_LICENSE("GPL");
> -
> -module_init(intel_scu_ipc_init);
> -module_exit(intel_scu_ipc_exit);
> -- 
> 2.5.3
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev

[Andy Shevchenko]

No need to use global variable for a mutex. Move it to be a member of SCU IPC
device structure.

While here, protect dev member assignment in ->remove() since user may
potentially call unbind from sysfs even if driver is built-in.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_scu_ipc.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 9de2029..16f1621 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -92,11 +92,13 @@ static struct intel_scu_ipc_pdata_t intel_scu_ipc_tangier_pdata = {
 	.irq_mode = 0,
 };
 
+/* @lock: lock used to prevent multiple call to SCU */
 struct intel_scu_ipc_dev {
 	struct device *dev;
 	void __iomem *ipc_base;
 	void __iomem *i2c_base;
 	struct completion cmd_complete;
+	struct mutex lock;
 	u8 irq_mode;
 };
 
@@ -112,8 +114,6 @@ static struct intel_scu_ipc_dev  ipcdev; /* Only one for now */
 #define IPC_I2C_CNTRL_ADDR	0
 #define I2C_DATA_ADDR		0x04
 
-static DEFINE_MUTEX(ipclock); /* lock used to prevent multiple call to SCU */
-
 /*
  * Send ipc command
  * Command Register (Write Only):
@@ -222,10 +222,10 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 
 	memset(cbuf, 0, sizeof(cbuf));
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}
 
@@ -258,7 +258,7 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 		for (nc = 0; nc < count; nc++)
 			data[nc] = ipc_data_readb(scu, nc);
 	}
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return err;
 }
 
@@ -440,14 +440,14 @@ int intel_scu_ipc_simple_command(int cmd, int sub)
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int err;
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}
 	ipc_command(scu, sub << 12 | cmd);
 	err = intel_scu_ipc_check_status(scu);
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return err;
 }
 EXPORT_SYMBOL(intel_scu_ipc_simple_command);
@@ -470,9 +470,9 @@ int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int i, err;
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}
 
@@ -487,7 +487,7 @@ int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
 			*out++ = ipc_data_readl(scu, 4 * i);
 	}
 
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return err;
 }
 EXPORT_SYMBOL(intel_scu_ipc_command);
@@ -513,9 +513,9 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	u32 cmd = 0;
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}
 	cmd = (addr >> 24) & 0xFF;
@@ -532,10 +532,10 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 		dev_err(scu->dev,
 			"intel_scu_ipc: I2C INVALID_CMD = 0x%x\n", cmd);
 
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -EIO;
 	}
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return 0;
 }
 EXPORT_SYMBOL(intel_scu_ipc_i2c_cntrl);
@@ -593,6 +593,7 @@ static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return err;
 
 	init_completion(&scu->cmd_complete);
+	mutex_init(&scu->lock);
 
 	err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_scu_ipc",
 			       scu);
@@ -625,7 +626,10 @@ static void ipc_remove(struct pci_dev *pdev)
 {
 	struct intel_scu_ipc_dev *scu = pci_get_drvdata(pdev);
 
+	mutex_lock(&scu->lock);
 	scu->dev = NULL;
+	mutex_unlock(&scu->lock);
+
 	iounmap(scu->i2c_base);
 	intel_scu_devices_destroy();
 }
-- 
2.5.3

Re: [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev

[Andy Shevchenko]

On Fri, 2015-10-09 at 17:11 +0300, Andy Shevchenko wrote:
> No need to use global variable for a mutex. Move it to be a member of 
> SCU IPC
> device structure.

This one is wrong, sorry.

> 
> While here, protect dev member assignment in ->remove() since user 
> may
> potentially call unbind from sysfs even if driver is built-in.

This part is still right.

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel_scu_ipc.c | 34 +++++++++++++++++++-------
> --------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_scu_ipc.c 
> b/drivers/platform/x86/intel_scu_ipc.c
> index 9de2029..16f1621 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -92,11 +92,13 @@ static struct intel_scu_ipc_pdata_t 
> intel_scu_ipc_tangier_pdata = {
>  	.irq_mode = 0,
>  };
>  
> +/* @lock: lock used to prevent multiple call to SCU */
>  struct intel_scu_ipc_dev {
>  	struct device *dev;
>  	void __iomem *ipc_base;
>  	void __iomem *i2c_base;
>  	struct completion cmd_complete;
> +	struct mutex lock;
>  	u8 irq_mode;
>  };
>  
> @@ -112,8 +114,6 @@ static struct intel_scu_ipc_dev  ipcdev; /* Only 
> one for now */
>  #define IPC_I2C_CNTRL_ADDR	0
>  #define I2C_DATA_ADDR		0x04
>  
> -static DEFINE_MUTEX(ipclock); /* lock used to prevent multiple call 
> to SCU */
> -
>  /*
>   * Send ipc command
>   * Command Register (Write Only):
> @@ -222,10 +222,10 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, 
> u32 count, u32 op, u32 id)
>  
>  	memset(cbuf, 0, sizeof(cbuf));
>  
> -	mutex_lock(&ipclock);
> +	mutex_lock(&scu->lock);
>  
>  	if (scu->dev == NULL) {
> -		mutex_unlock(&ipclock);
> +		mutex_unlock(&scu->lock);
>  		return -ENODEV;
>  	}
>  
> @@ -258,7 +258,7 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 
> count, u32 op, u32 id)
>  		for (nc = 0; nc < count; nc++)
>  			data[nc] = ipc_data_readb(scu, nc);
>  	}
> -	mutex_unlock(&ipclock);
> +	mutex_unlock(&scu->lock);
>  	return err;
>  }
>  
> @@ -440,14 +440,14 @@ int intel_scu_ipc_simple_command(int cmd, int 
> sub)
>  	struct intel_scu_ipc_dev *scu = &ipcdev;
>  	int err;
>  
> -	mutex_lock(&ipclock);
> +	mutex_lock(&scu->lock);
>  	if (scu->dev == NULL) {
> -		mutex_unlock(&ipclock);
> +		mutex_unlock(&scu->lock);
>  		return -ENODEV;
>  	}
>  	ipc_command(scu, sub << 12 | cmd);
>  	err = intel_scu_ipc_check_status(scu);
> -	mutex_unlock(&ipclock);
> +	mutex_unlock(&scu->lock);
>  	return err;
>  }
>  EXPORT_SYMBOL(intel_scu_ipc_simple_command);
> @@ -470,9 +470,9 @@ int intel_scu_ipc_command(int cmd, int sub, u32 
> *in, int inlen,
>  	struct intel_scu_ipc_dev *scu = &ipcdev;
>  	int i, err;
>  
> -	mutex_lock(&ipclock);
> +	mutex_lock(&scu->lock);
>  	if (scu->dev == NULL) {
> -		mutex_unlock(&ipclock);
> +		mutex_unlock(&scu->lock);
>  		return -ENODEV;
>  	}
>  
> @@ -487,7 +487,7 @@ int intel_scu_ipc_command(int cmd, int sub, u32 
> *in, int inlen,
>  			*out++ = ipc_data_readl(scu, 4 * i);
>  	}
>  
> -	mutex_unlock(&ipclock);
> +	mutex_unlock(&scu->lock);
>  	return err;
>  }
>  EXPORT_SYMBOL(intel_scu_ipc_command);
> @@ -513,9 +513,9 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
>  	struct intel_scu_ipc_dev *scu = &ipcdev;
>  	u32 cmd = 0;
>  
> -	mutex_lock(&ipclock);
> +	mutex_lock(&scu->lock);
>  	if (scu->dev == NULL) {
> -		mutex_unlock(&ipclock);
> +		mutex_unlock(&scu->lock);
>  		return -ENODEV;
>  	}
>  	cmd = (addr >> 24) & 0xFF;
> @@ -532,10 +532,10 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 
> *data)
>  		dev_err(scu->dev,
>  			"intel_scu_ipc: I2C INVALID_CMD = 0x%x\n", 
> cmd);
>  
> -		mutex_unlock(&ipclock);
> +		mutex_unlock(&scu->lock);
>  		return -EIO;
>  	}
> -	mutex_unlock(&ipclock);
> +	mutex_unlock(&scu->lock);
>  	return 0;
>  }
>  EXPORT_SYMBOL(intel_scu_ipc_i2c_cntrl);
> @@ -593,6 +593,7 @@ static int ipc_probe(struct pci_dev *pdev, const 
> struct pci_device_id *id)
>  		return err;
>  
>  	init_completion(&scu->cmd_complete);
> +	mutex_init(&scu->lock);
>  
>  	err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, 
> "intel_scu_ipc",
>  			       scu);
> @@ -625,7 +626,10 @@ static void ipc_remove(struct pci_dev *pdev)
>  {
>  	struct intel_scu_ipc_dev *scu = pci_get_drvdata(pdev);
>  
> +	mutex_lock(&scu->lock);
>  	scu->dev = NULL;
> +	mutex_unlock(&scu->lock);
> +
>  	iounmap(scu->i2c_base);
>  	intel_scu_devices_destroy();
>  }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev

[Darren Hart]

On Fri, Oct 09, 2015 at 06:21:37PM +0300, Andy Shevchenko wrote:
> On Fri, 2015-10-09 at 17:11 +0300, Andy Shevchenko wrote:
> > No need to use global variable for a mutex. Move it to be a member of 
> > SCU IPC
> > device structure.
> 
> This one is wrong, sorry.
> 
> > 
> > While here, protect dev member assignment in ->remove() since user 
> > may
> > potentially call unbind from sysfs even if driver is built-in.
> 
> This part is still right.

OK.

With my minor comment to 1/5 and this change, care to send a v2?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev

[Andy Shevchenko]

On Sat, 2015-10-10 at 21:43 -0700, Darren Hart wrote:
> On Fri, Oct 09, 2015 at 06:21:37PM +0300, Andy Shevchenko wrote:
> > On Fri, 2015-10-09 at 17:11 +0300, Andy Shevchenko wrote:
> > > No need to use global variable for a mutex. Move it to be a 
> > > member of 
> > > SCU IPC
> > > device structure.
> > 
> > This one is wrong, sorry.
> > 
> > > 
> > > While here, protect dev member assignment in ->remove() since 
> > > user 
> > > may
> > > potentially call unbind from sysfs even if driver is built-in.
> > 
> > This part is still right.
> 
> OK.
> 
> With my minor comment to 1/5 and this change, care to send a v2?

Will do later this week.
Also I will update message of the patch 4 as you suggested.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_*

[Darren Hart]

On Fri, Oct 09, 2015 at 05:11:32PM +0300, Andy Shevchenko wrote:
> The error handling is broken right now since it leaves resources unfreed.
> Convert the code to use managed resources to fix the error handling.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks Andriy,

Please always explicitly Cc LKML.

Is this hardware you able to test explicitly?

> ---
>  drivers/platform/x86/intel_scu_ipc.c | 26 ++++++++------------------
>  1 file changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index 187d108..7148535 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c

...

> -	err = pci_request_regions(dev, "intel_scu_ipc");
> +	err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev));
>  	if (err)
>  		return err;
>  
> -	base = pci_resource_start(dev, 0);
> -	if (!base)
> -		return -ENOMEM;
> -
>  	init_completion(&ipcdev.cmd_complete);
>  
> -	if (request_irq(dev->irq, ioc, 0, "intel_scu_ipc", &ipcdev))
> -		return -EBUSY;
> +	err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, "intel_scu_ipc",

You switched to using pci_name(dev) above, seems to me the same rationale should
apply here. Any reason not to use pci_name(dev) here instead of "intel_scu_ipc"?

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_*

[Andy Shevchenko]

On Sat, 2015-10-10 at 21:21 -0700, Darren Hart wrote:
> On Fri, Oct 09, 2015 at 05:11:32PM +0300, Andy Shevchenko wrote:
> > The error handling is broken right now since it leaves resources 
> > unfreed.
> > Convert the code to use managed resources to fix the error 
> > handling.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Thanks Andriy,
> 
> Please always explicitly Cc LKML.
> 
> Is this hardware you able to test explicitly?

Yes, the patch series has been tested on Intel Medfield and Intel
Edison  (Merrifield) boards.

> 
> > ---
> >  drivers/platform/x86/intel_scu_ipc.c | 26 ++++++++----------------
> > --
> >  1 file changed, 8 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_scu_ipc.c 
> > b/drivers/platform/x86/intel_scu_ipc.c
> > index 187d108..7148535 100644
> > --- a/drivers/platform/x86/intel_scu_ipc.c
> > +++ b/drivers/platform/x86/intel_scu_ipc.c
> 
> ...
> 
> > -	err = pci_request_regions(dev, "intel_scu_ipc");
> > +	err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev));
> >  	if (err)
> >  		return err;
> >  
> > -	base = pci_resource_start(dev, 0);
> > -	if (!base)
> > -		return -ENOMEM;
> > -
> >  	init_completion(&ipcdev.cmd_complete);
> >  
> > -	if (request_irq(dev->irq, ioc, 0, "intel_scu_ipc", 
> > &ipcdev))
> > -		return -EBUSY;
> > +	err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, 
> > "intel_scu_ipc",
> 
> You switched to using pci_name(dev) above, seems to me the same 
> rationale should
> apply here. Any reason not to use pci_name(dev) here instead of 
> "intel_scu_ipc"?
> 
> Thanks,
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy