* [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_*
@ 2015-10-09 14:11 Andy Shevchenko
2015-10-09 14:11 ` [PATCH 2/5] intel_scu_ipc: propagate struct intel_scu_ipc_dev Andy Shevchenko
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Andy Shevchenko @ 2015-10-09 14:11 UTC (permalink / raw)
To: platform-driver-x86, Darren Hart; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/5] intel_scu_ipc: propagate struct intel_scu_ipc_dev
2015-10-09 14:11 [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_* Andy Shevchenko
@ 2015-10-09 14:11 ` Andy Shevchenko
2015-10-09 14:11 ` [PATCH 3/5] intel_scu_ipc: convert to use struct device * Andy Shevchenko
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2015-10-09 14:11 UTC (permalink / raw)
To: platform-driver-x86, Darren Hart; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/5] intel_scu_ipc: convert to use struct device *
2015-10-09 14:11 [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_* Andy Shevchenko
2015-10-09 14:11 ` [PATCH 2/5] intel_scu_ipc: propagate struct intel_scu_ipc_dev Andy Shevchenko
@ 2015-10-09 14:11 ` Andy Shevchenko
2015-10-09 14:11 ` [PATCH 4/5] intel_scu_ipc: switch to use module_pci_driver() macro Andy Shevchenko
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2015-10-09 14:11 UTC (permalink / raw)
To: platform-driver-x86, Darren Hart; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/5] intel_scu_ipc: switch to use module_pci_driver() macro
2015-10-09 14:11 [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_* Andy Shevchenko
2015-10-09 14:11 ` [PATCH 2/5] intel_scu_ipc: propagate struct intel_scu_ipc_dev Andy Shevchenko
2015-10-09 14:11 ` [PATCH 3/5] intel_scu_ipc: convert to use struct device * Andy Shevchenko
@ 2015-10-09 14:11 ` Andy Shevchenko
2015-10-11 4:39 ` Darren Hart
2015-10-09 14:11 ` [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev Andy Shevchenko
2015-10-11 4:21 ` [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_* Darren Hart
4 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2015-10-09 14:11 UTC (permalink / raw)
To: platform-driver-x86, Darren Hart; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev
2015-10-09 14:11 [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_* Andy Shevchenko
` (2 preceding siblings ...)
2015-10-09 14:11 ` [PATCH 4/5] intel_scu_ipc: switch to use module_pci_driver() macro Andy Shevchenko
@ 2015-10-09 14:11 ` Andy Shevchenko
2015-10-09 15:21 ` Andy Shevchenko
2015-10-11 4:21 ` [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_* Darren Hart
4 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2015-10-09 14:11 UTC (permalink / raw)
To: platform-driver-x86, Darren Hart; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev
2015-10-09 14:11 ` [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev Andy Shevchenko
@ 2015-10-09 15:21 ` Andy Shevchenko
2015-10-11 4:43 ` Darren Hart
0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2015-10-09 15:21 UTC (permalink / raw)
To: platform-driver-x86, Darren Hart
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_*
2015-10-09 14:11 [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_* Andy Shevchenko
` (3 preceding siblings ...)
2015-10-09 14:11 ` [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev Andy Shevchenko
@ 2015-10-11 4:21 ` Darren Hart
2015-10-12 8:00 ` Andy Shevchenko
4 siblings, 1 reply; 11+ messages in thread
From: Darren Hart @ 2015-10-11 4:21 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: platform-driver-x86
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/5] intel_scu_ipc: switch to use module_pci_driver() macro
2015-10-09 14:11 ` [PATCH 4/5] intel_scu_ipc: switch to use module_pci_driver() macro Andy Shevchenko
@ 2015-10-11 4:39 ` Darren Hart
0 siblings, 0 replies; 11+ messages in thread
From: Darren Hart @ 2015-10-11 4:39 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: platform-driver-x86
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev
2015-10-09 15:21 ` Andy Shevchenko
@ 2015-10-11 4:43 ` Darren Hart
2015-10-12 8:02 ` Andy Shevchenko
0 siblings, 1 reply; 11+ messages in thread
From: Darren Hart @ 2015-10-11 4:43 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: platform-driver-x86
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_*
2015-10-11 4:21 ` [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_* Darren Hart
@ 2015-10-12 8:00 ` Andy Shevchenko
0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2015-10-12 8:00 UTC (permalink / raw)
To: Darren Hart; +Cc: platform-driver-x86
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev
2015-10-11 4:43 ` Darren Hart
@ 2015-10-12 8:02 ` Andy Shevchenko
0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2015-10-12 8:02 UTC (permalink / raw)
To: Darren Hart; +Cc: platform-driver-x86
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-10-12 8:02 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-09 14:11 [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_* Andy Shevchenko
2015-10-09 14:11 ` [PATCH 2/5] intel_scu_ipc: propagate struct intel_scu_ipc_dev Andy Shevchenko
2015-10-09 14:11 ` [PATCH 3/5] intel_scu_ipc: convert to use struct device * Andy Shevchenko
2015-10-09 14:11 ` [PATCH 4/5] intel_scu_ipc: switch to use module_pci_driver() macro Andy Shevchenko
2015-10-11 4:39 ` Darren Hart
2015-10-09 14:11 ` [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev Andy Shevchenko
2015-10-09 15:21 ` Andy Shevchenko
2015-10-11 4:43 ` Darren Hart
2015-10-12 8:02 ` Andy Shevchenko
2015-10-11 4:21 ` [PATCH 1/5] intel_scu_ipc: fix error path by turning to devm_* / pcim_* Darren Hart
2015-10-12 8:00 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.