On Wed, Aug 05, 2015 at 02:25:13PM -0700, Darren Hart wrote:
> On Fri, Jul 31, 2015 at 11:58:56PM +0800, Qipeng Zha wrote:
> > This driver provides support for Punit mailbox IPC on Intel platforms.
> > The heart of the Punit is the Foxton microcontroller and its firmware,
> > which provide mailbox interface for power management usage.
> >
>
> Mika, Andriy:
>
> I'm traveling over the next few days and could use some help ensuring this new
> driver receives some additional review. If you can, I'd appreciate your
> thoughts.
Sure, no problem.
> > Signed-off-by: Qipeng Zha <qipeng.zha@xxxxxxxxx>
> > ---
> > arch/x86/include/asm/intel_punit_ipc.h | 101 +++++++++
> > drivers/platform/x86/Kconfig | 6 +
> > drivers/platform/x86/Makefile | 1 +
> > drivers/platform/x86/intel_punit_ipc.c | 388 +++++++++++++++++++++++++++++++++
> > 4 files changed, 496 insertions(+)
> > create mode 100644 arch/x86/include/asm/intel_punit_ipc.h
> > create mode 100644 drivers/platform/x86/intel_punit_ipc.c
> >
> > diff --git a/arch/x86/include/asm/intel_punit_ipc.h b/arch/x86/include/asm/intel_punit_ipc.h
> > new file mode 100644
> > index 0000000..08e3e14
> > --- /dev/null
> > +++ b/arch/x86/include/asm/intel_punit_ipc.h
> > @@ -0,0 +1,101 @@
> > +#ifndef _ASM_X86_INTEL_PUNIT_IPC_H_
> > +#define _ASM_X86_INTEL_PUNIT_IPC_H_
> > +
> > +/* Commands supported on GLM core, are handled by different bars,
> > + * unify these commands together here
> > + */
> > +#define IPC_BIOS_PUNIT_CMD_BASE 0x00
> > +#define IPC_GTD_PUNIT_CMD_BASE 0x20
> > +#define IPC_ISPD_PUNIT_CMD_BASE 0x40
> > +
> > +/* BIOS => Pcode commands */
> > +#define IPC_BIOS_PUNIT_CMD_ZERO (IPC_BIOS_PUNIT_CMD_BASE + 0x00)
> > +#define IPC_BIOS_PUNIT_CMD_VR_INTERFACE (IPC_BIOS_PUNIT_CMD_BASE + 0x01)
> > +#define IPC_BIOS_PUNIT_CMD_READ_PCS (IPC_BIOS_PUNIT_CMD_BASE + 0x02)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_PCS (IPC_BIOS_PUNIT_CMD_BASE + 0x03)
> > +#define IPC_BIOS_PUNIT_CMD_READ_PCU_CONFIG (IPC_BIOS_PUNIT_CMD_BASE + 0x04)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_PCU_CONFIG (IPC_BIOS_PUNIT_CMD_BASE + 0x05)
> > +#define IPC_BIOS_PUNIT_CMD_READ_PL1_SETTING (IPC_BIOS_PUNIT_CMD_BASE + 0x06)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_PL1_SETTING (IPC_BIOS_PUNIT_CMD_BASE + 0x07)
> > +#define IPC_BIOS_PUNIT_CMD_TRIGGER_VDD_RAM (IPC_BIOS_PUNIT_CMD_BASE + 0x08)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_INFO (IPC_BIOS_PUNIT_CMD_BASE + 0x09)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_TRACE_CTRL (IPC_BIOS_PUNIT_CMD_BASE + 0x0a)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_TRACE_CTRL \
> > + (IPC_BIOS_PUNIT_CMD_BASE + 0x0b)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_EVENT_CTRL (IPC_BIOS_PUNIT_CMD_BASE + 0x0c)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_EVENT_CTRL \
> > + (IPC_BIOS_PUNIT_CMD_BASE + 0x0d)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_TRACE (IPC_BIOS_PUNIT_CMD_BASE + 0x0e)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_TRACE (IPC_BIOS_PUNIT_CMD_BASE + 0x0f)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_EVENT (IPC_BIOS_PUNIT_CMD_BASE + 0x10)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_EVENT (IPC_BIOS_PUNIT_CMD_BASE + 0x11)
> > +#define IPC_BIOS_PUNIT_CMD_READ_MODULE_TEMP (IPC_BIOS_PUNIT_CMD_BASE + 0x12)
> > +#define IPC_BIOS_PUNIT_CMD_RESERVED (IPC_BIOS_PUNIT_CMD_BASE + 0x13)
> > +#define IPC_BIOS_PUNIT_CMD_READ_VOLTAGE_OVER (IPC_BIOS_PUNIT_CMD_BASE + 0x14)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_VOLTAGE_OVER (IPC_BIOS_PUNIT_CMD_BASE + 0x15)
> > +#define IPC_BIOS_PUNIT_CMD_READ_RATIO_OVER (IPC_BIOS_PUNIT_CMD_BASE + 0x16)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_RATIO_OVER (IPC_BIOS_PUNIT_CMD_BASE + 0x17)
> > +#define IPC_BIOS_PUNIT_CMD_READ_VF_GL_CTRL (IPC_BIOS_PUNIT_CMD_BASE + 0x18)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_VF_GL_CTRL (IPC_BIOS_PUNIT_CMD_BASE + 0x19)
> > +#define IPC_BIOS_PUNIT_CMD_READ_FM_SOC_TEMP_THRESH \
> > + (IPC_BIOS_PUNIT_CMD_BASE + 0x1a)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_FM_SOC_TEMP_THRESH \
> > + (IPC_BIOS_PUNIT_CMD_BASE + 0x1b)
> > +
> > +/*GT Driver => Pcode commands*/
> > +#define IPC_GTD_PUNIT_CMD_ZERO (IPC_GTD_PUNIT_CMD_BASE + 0x00)
> > +#define IPC_GTD_PUNIT_CMD_CONFIG (IPC_GTD_PUNIT_CMD_BASE + 0x01)
> > +#define IPC_GTD_PUNIT_CMD_READ_ICCP_LIC_CDYN_SCAL \
> > + (IPC_GTD_PUNIT_CMD_BASE + 0x02)
> > +#define IPC_GTD_PUNIT_CMD_WRITE_ICCP_LIC_CDYN_SCAL \
> > + (IPC_GTD_PUNIT_CMD_BASE + 0x03)
> > +#define IPC_GTD_PUNIT_CMD_GET_WM_VAL (IPC_GTD_PUNIT_CMD_BASE + 0x06)
> > +#define IPC_GTD_PUNIT_CMD_WRITE_CONFIG_WISHREQ (IPC_GTD_PUNIT_CMD_BASE + 0x07)
> > +#define IPC_GTD_PUNIT_CMD_READ_REQ_DUTY_CYCLE (IPC_GTD_PUNIT_CMD_BASE + 0x16)
> > +#define IPC_GTD_PUNIT_CMD_DIS_VOL_FREQ_CHANGE_REQUEST \
> > + (IPC_GTD_PUNIT_CMD_BASE + 0x17)
> > +#define IPC_GTD_PUNIT_CMD_DYNA_DUTY_CYCLE_CTRL (IPC_GTD_PUNIT_CMD_BASE + 0x1a)
> > +#define IPC_GTD_PUNIT_CMD_DYNA_DUTY_CYCLE_TUNING \
> > + (IPC_GTD_PUNIT_CMD_BASE + 0x1c)
> > +
> > +/* ISP Driver => Pcode commands */
> > +#define IPC_ISPD_PUNIT_CMD_ZERO (IPC_ISPD_PUNIT_CMD_BASE + 0x00)
> > +#define IPC_ISPD_PUNIT_CMD_CONFIG (IPC_ISPD_PUNIT_CMD_BASE + 0x01)
> > +#define IPC_ISPD_PUNIT_CMD_GET_ISP_LTR_VAL (IPC_ISPD_PUNIT_CMD_BASE + 0x02)
> > +#define IPC_ISPD_PUNIT_CMD_ACCESS_IU_FREQ_BOUNDS \
> > + (IPC_ISPD_PUNIT_CMD_BASE + 0x03)
> > +#define IPC_ISPD_PUNIT_CMD_READ_CDYN_LEVEL (IPC_ISPD_PUNIT_CMD_BASE + 0x04)
> > +#define IPC_ISPD_PUNIT_CMD_WRITE_CDYN_LEVEL (IPC_ISPD_PUNIT_CMD_BASE + 0x05)
> > +#define IPC_ISPD_PUNIT_CMD_MAX (IPC_ISPD_PUNIT_CMD_BASE + 0x06)
> > +
> > +/* Error codes */
> > +#define IPC_ERR_SUCCESS 0
> > +#define IPC_ERR_INVALID_CMD 1
> > +#define IPC_ERR_INVALID_PARAMETER 2
> > +#define IPC_ERR_CMD_TIMEOUT 3
> > +#define IPC_ERR_CMD_LOCKED 4
> > +#define IPC_ERR_INVALID_VR_ID 5
> > +#define IPC_ERR_VR_ERR 6
> > +
> > +#if IS_ENABLED(CONFIG_INTEL_PUNIT_IPC)
> > +
> > +int intel_punit_ipc_simple_command(int cmd, int para1, int para2);
> > +int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2, u32 *in, u32 *out);
So is there going to be in-kernel user for this driver?
> > +
> > +#else
> > +
> > +static intline int intel_punit_ipc_simple_command(int cmd,
> > + int para1, int para2);
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static inline int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2,
> > + u32 *in, u32 *out);
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +#endif /*CONFIG_INTEL_PUNIT_IPC*/
> > +
> > +#endif
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 346f1fd..42ada9b 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -891,4 +891,10 @@ config INTEL_PMC_IPC
> > The PMC is an ARC processor which defines IPC commands for communication
> > with other entities in the CPU.
> >
> > +config INTEL_PUNIT_IPC
> > + tristate "Intel PUNIT IPC Driver"
> > + default y
I'm sure it does not need to be enabled by default.
> > + ---help---
> > + IPC is used to bridge the communications between kernel and PUNIT
> > +
> > endif # X86_PLATFORM_DEVICES
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index 1051372..eea765f 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -59,3 +59,4 @@ obj-$(CONFIG_INTEL_SMARTCONNECT) += intel-smartconnect.o
> > obj-$(CONFIG_PVPANIC) += pvpanic.o
> > obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
> > obj-$(CONFIG_INTEL_PMC_IPC) += intel_pmc_ipc.o
> > +obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o
> > diff --git a/drivers/platform/x86/intel_punit_ipc.c b/drivers/platform/x86/intel_punit_ipc.c
> > new file mode 100644
> > index 0000000..78cb794
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_punit_ipc.c
> > @@ -0,0 +1,388 @@
> > +/*
> > + * intel_punit_ipc.c: Driver for the Intel Punit Mailbox IPC mechanism
Drop the file name here.
> > + *
> > + * (C) Copyright 2015 Intel Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * The heart of the Punit is the Foxton microcontroller and its firmware,
> > + * which provide mailbox interface for power management usage.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/device.h>
> > +#include <linux/pm.h>
> > +#include <linux/acpi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/bitops.h>
> > +#include <linux/atomic.h>
> > +#include <linux/notifier.h>
> > +#include <linux/suspend.h>
> > +#include <linux/platform_device.h>
Are you sure you need all the above headers?
> > +#include <asm/intel_punit_ipc.h>
> > +
> > +/* Mailbox registers */
> > +#define MAILBOX_DATA_LOW 0x0
> > +#define MAILBOX_INTERFACE 0x4
> > +#define CMD_RUN (1 << 31)
> > +#define CMD_ERRCODE_MASK 0xFF
> > +#define CMD_PARA1_SHIFT 8
> > +#define CMD_PARA2_SHIFT 16
> > +#define CMD_MASK 0xFF
> > +#define MAILBOX_DATA_HIGH 0x8
> > +
> > +#define MAILBOX_REGISTER_SPACE 0x10
> > +
> > +#define CMD_TIMEOUT_SECONDS 3
> > +
> > +/* Three external mailbox */
> > +enum mailbox_type {
> > + BIOS_MAILBOX,
> > + GTDRIVER_MAILBOX,
> > + ISPDRIVER_MAILBOX,
> > + RESERVED_MAILBOX,
> > +};
> > +
> > +struct intel_punit_ipc_controller {
> > + struct platform_device *pdev;
> > + struct mutex lock;
> > + void __iomem *base[RESERVED_MAILBOX];
> > + struct completion cmd_complete;
> > + int irq;
> > +
> > + int cmd;
> > + enum mailbox_type type;
> > +};
> > +
> > +static char *ipc_err_sources[] = {
> > + [IPC_ERR_SUCCESS] =
> > + "no error",
> > + [IPC_ERR_INVALID_CMD] =
> > + "invalid command",
> > + [IPC_ERR_INVALID_PARAMETER] =
> > + "invalid parameter",
> > + [IPC_ERR_CMD_TIMEOUT] =
> > + "command timeout",
> > + [IPC_ERR_CMD_LOCKED] =
> > + "command locked",
> > + [IPC_ERR_INVALID_VR_ID] =
> > + "invalid vr id",
> > + [IPC_ERR_VR_ERR] =
> > + "vr error",
> > +};
> > +
> > +static struct intel_punit_ipc_controller ipcdev;
Why not to allocate the private structure at probe time?
> > +
> > +static inline u32 ipc_read_status(void)
> > +{
> > + return readl(ipcdev.base[ipcdev.type] + MAILBOX_INTERFACE);
> > +}
> > +
> > +static inline void ipc_write_cmd(u32 cmd)
> > +{
> > + writel(cmd, ipcdev.base[ipcdev.type] + MAILBOX_INTERFACE);
> > +}
> > +
> > +static inline u32 ipc_read_data_low(void)
> > +{
> > + return readl(ipcdev.base[ipcdev.type] + MAILBOX_DATA_LOW);
> > +}
> > +
> > +static inline u32 ipc_read_data_high(void)
> > +{
> > + return readl(ipcdev.base[ipcdev.type] + MAILBOX_DATA_HIGH);
> > +}
> > +
> > +static inline void ipc_write_data_low(u32 data)
> > +{
> > + writel(data, ipcdev.base[ipcdev.type] + MAILBOX_DATA_LOW);
> > +}
> > +
> > +static inline void ipc_write_data_high(u32 data)
> > +{
> > + writel(data, ipcdev.base[ipcdev.type] + MAILBOX_DATA_HIGH);
> > +}
> > +
> > +static int intel_punit_init_cmd(u32 cmd)
> > +{
> > + cmd &= CMD_MASK;
> > + if (cmd < IPC_BIOS_PUNIT_CMD_BASE)
> > + return -EINVAL;
> > + else if (cmd < IPC_GTD_PUNIT_CMD_BASE)
> > + ipcdev.type = BIOS_MAILBOX;
> > + else if (cmd < IPC_ISPD_PUNIT_CMD_BASE)
> > + ipcdev.type = GTDRIVER_MAILBOX;
> > + else if (cmd < IPC_ISPD_PUNIT_CMD_MAX)
> > + ipcdev.type = ISPDRIVER_MAILBOX;
> > + else
> > + return -EINVAL;
> > +
> > + return 0;
return -EINVAL and drop else
> > +}
> > +
> > +static int intel_punit_ipc_send_command(u32 cmd)
> > +{
> > + int ret;
> > +
> > + ret = intel_punit_init_cmd(cmd);
> > + if (ret)
> > + return ret;
> > + ipcdev.cmd = cmd;
> > + reinit_completion(&ipcdev.cmd_complete);
> > + ipc_write_cmd(cmd);
> > + return 0;
> > +}
> > +
> > +static int intel_punit_ipc_check_status(void)
> > +{
> > + int loops = CMD_TIMEOUT_SECONDS * USEC_PER_SEC;
> > + int ret = 0;
> > + int status;
> > + int errcode;
> > +
> > + if (ipcdev.irq) {
> > + if (0 == wait_for_completion_timeout(
if (!wait_for_completion_timeout(
> > + &ipcdev.cmd_complete,
> > + CMD_TIMEOUT_SECONDS * HZ)) {
> > + dev_err(&ipcdev.pdev->dev,
> > + "IPC timed out, IPC_CMD=0x%x\n", ipcdev.cmd);
> > + return -ETIMEDOUT;
> > + }
> > + } else {
> > + while ((ipc_read_status() & CMD_RUN) && --loops)
> > + udelay(1);
> > + if (!loops) {
> > + dev_err(&ipcdev.pdev->dev,
> > + "IPC timed out, IPC_CMD=0x%x\n", ipcdev.cmd);
> > + return -ETIMEDOUT;
> > + }
> > + }
> > +
> > + status = ipc_read_status();
> > + errcode = status & CMD_ERRCODE_MASK;
> > + if (errcode) {
> > + ret = -EIO;
> > + if (errcode < ARRAY_SIZE(ipc_err_sources))
> > + dev_err(&ipcdev.pdev->dev,
> > + "IPC failed: %s, IPC_STS=0x%x, IPC_CMD=0x%x\n",
> > + ipc_err_sources[errcode], status, ipcdev.cmd);
> > + else
> > + dev_err(&ipcdev.pdev->dev,
> > + "IPC failed: unknown err,STS=0x%x, CMD=0x%x\n",
> > + status, ipcdev.cmd);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * intel_punit_ipc_simple_command() - Simple IPC command
> > + * @cmd: IPC command code.
> > + * @para1: First 8bit parameter, set 0 if invalid.
> > + * @para2: Second 8bit parameter, set 0 if invalid.
> > + *
> > + * Send a IPC command to Punit when there is no data transaction
> > + *
> > + * Return: an IPC error code or 0 on success.
> > + */
> > +int intel_punit_ipc_simple_command(int cmd, int para1, int para2)
> > +{
> > + int ret;
> > +
> > + mutex_lock(&ipcdev.lock);
> > + ret = intel_punit_ipc_send_command(CMD_RUN |
> > + para2 << CMD_PARA2_SHIFT |
> > + para1 << CMD_PARA1_SHIFT | cmd);
> > + if (ret)
> > + goto out;
> > + ret = intel_punit_ipc_check_status();
> > +out:
> > + mutex_unlock(&ipcdev.lock);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(intel_punit_ipc_simple_command);
> > +
> > +/**
> > + * intel_punit_ipc_command() - IPC command with data and pointers
> > + * @cmd: IPC command code.
> > + * @para1: First 8bit parameter, set 0 if invalid.
> > + * @para2: Second 8bit parameter, set 0 if invalid.
> > + * @in: Input data, 32bit for BIOS cmd, two 32bit for GTD and ISPD.
> > + * @out: Output data.
> > + *
> > + * Send a IPC command to Punit with data transaction
> > + *
> > + * Return: an IPC error code or 0 on success.
> > + */
> > +int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2, u32 *in, u32 *out)
> > +{
> > + int ret;
> > +
> > + mutex_lock(&ipcdev.lock);
> > + ipc_write_data_low(*in);
> > + if (ipcdev.type == GTDRIVER_MAILBOX ||
> > + ipcdev.type == ISPDRIVER_MAILBOX) {
> > + in++;
> > + ipc_write_data_high(*in);
> > + }
> > + ret = intel_punit_ipc_send_command(CMD_RUN |
> > + para2 << CMD_PARA2_SHIFT |
> > + para1 << CMD_PARA1_SHIFT | cmd);
> > + if (ret)
> > + goto out;
> > + ret = intel_punit_ipc_check_status();
> > + *out = ipc_read_data_low();
> > + if (ipcdev.type == GTDRIVER_MAILBOX ||
> > + ipcdev.type == ISPDRIVER_MAILBOX) {
> > + out++;
> > + *out = ipc_read_data_high();
> > + }
> > +out:
> > + mutex_unlock(&ipcdev.lock);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(intel_punit_ipc_command);
> > +
> > +static irqreturn_t intel_punit_ioc(int irq, void *dev_id)
> > +{
> > + complete(&ipcdev.cmd_complete);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int intel_punit_get_bars(struct platform_device *pdev)
> > +{
> > + struct resource *res0, *res1;
> > + void __iomem *addr;
> > + int size;
> > + int ret;
> > +
> > + res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res0) {
> > + dev_err(&pdev->dev, "Failed to get iomem resource\n");
> > + return -EINVAL;
> > + }
> > + size = resource_size(res0);
> > + if (!request_mem_region(res0->start, size, pdev->name)) {
> > + dev_err(&pdev->dev, "Failed to request iomem resouce\n");
> > + return -EBUSY;
> > + }
Why not use devm_ here, like:
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
addr = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(addr))
return PTR_ERR(addr);
> > +
> > + res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!res1) {
> > + dev_err(&pdev->dev, "Failed to get iomem resource1\n");
> > + return -EINVAL;
> > + }
> > + size = resource_size(res1);
> > + if (!request_mem_region(res1->start, size, pdev->name)) {
> > + dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
> > + ret = -EBUSY;
> > + goto err_res1;
> > + }
> > +
> > + addr = ioremap_nocache(res0->start,
> > + resource_size(res0) + resource_size(res1));
> > + if (!addr) {
> > + dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
> > + ret = -ENOMEM;
> > + goto err_map;
> > + }
> > + ipcdev.base[BIOS_MAILBOX] = addr;
> > + addr += MAILBOX_REGISTER_SPACE;
> > + ipcdev.base[GTDRIVER_MAILBOX] = addr;
> > + addr += MAILBOX_REGISTER_SPACE;
> > + ipcdev.base[ISPDRIVER_MAILBOX] = addr;
> > +
> > + return 0;
> > +
> > +err_map:
> > + release_mem_region(res1->start, resource_size(res1));
> > +err_res1:
> > + release_mem_region(res0->start, resource_size(res0));
> > + return ret;
> > +}
> > +
> > +static int intel_punit_ipc_probe(struct platform_device *pdev)
> > +{
> > + int irq;
> > + int ret;
Looks better if you do
int irq, ret;
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + ipcdev.irq = 0;
> > + dev_warn(&pdev->dev, "Could not get irq number\n");
> > + } else {
> > + if (request_irq(irq, intel_punit_ioc, IRQF_NO_SUSPEND,
> > + "intel_punit_ipc", &ipcdev)) {
> > + dev_err(&pdev->dev, "request irq %d\n", irq);
> > + return -EBUSY;
> > + }
> > + ipcdev.irq = irq;
> > + }
devm_request_irq()?
> > +
> > + ret = intel_punit_get_bars(pdev);
> > + if (ret) {
> > + if (ipcdev.irq)
> > + free_irq(ipcdev.irq, &ipcdev);
Then you don't need all this...
> > + return ret;
> > + }
> > +
> > + ipcdev.pdev = pdev;
> > + mutex_init(&ipcdev.lock);
> > + init_completion(&ipcdev.cmd_complete);
> > + return ret;
> > +}
> > +
> > +static int intel_punit_ipc_remove(struct platform_device *pdev)
> > +{
> > + struct resource *res;
> > +
> > + if (ipcdev.irq)
> > + free_irq(ipcdev.irq, &ipcdev);
> > + iounmap(ipcdev.base[BIOS_MAILBOX]);
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (res)
> > + release_mem_region(res->start, resource_size(res));
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (res)
> > + release_mem_region(res->start, resource_size(res));
...nor this.
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id punit_ipc_acpi_ids[] = {
> > + {"INT34D4", 0}
> > +};
> > +#endif
> > +
> > +static struct platform_driver intel_punit_ipc_driver = {
> > + .probe = intel_punit_ipc_probe,
> > + .remove = intel_punit_ipc_remove,
> > + .driver = {
> > + .name = "intel_punit_ipc",
> > + .acpi_match_table = ACPI_PTR(punit_ipc_acpi_ids),
> > + },
> > +};
> > +
> > +static int __init intel_punit_ipc_init(void)
> > +{
> > + return platform_driver_register(&intel_punit_ipc_driver);
> > +}
> > +
> > +static void __exit intel_punit_ipc_exit(void)
> > +{
> > + platform_driver_unregister(&intel_punit_ipc_driver);
> > +}
> > +
> > +MODULE_AUTHOR("Zha Qipeng <qipeng.zha@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("Intel Punit IPC driver");
> > +MODULE_LICENSE("GPL");
GPLv2
> > +
> > +/* Some modules are dependent on this, so init earlier */
> > +fs_initcall(intel_punit_ipc_init);
> > +module_exit(intel_punit_ipc_exit);
> > --
> > 1.8.3.2
> >
> >
>
> --
> Darren Hart
> Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
