On Thu, Apr 21, 2016 at 04:33:25PM +0530, Rajneesh Bhardwaj wrote:
[...]
Just some minor things I've spotted and one error is probably unchecked.
[...]
> diff --git a/arch/x86/include/asm/pmc_core.h b/arch/x86/include/asm/pmc_core.h
> new file mode 100644
> index 0000000..3ea61bf
> --- /dev/null
> +++ b/arch/x86/include/asm/pmc_core.h
> @@ -0,0 +1,53 @@
> +/*
> + * Intel Core SOC Power Management Controller Header File
> + *
> + * Copyright (c) 2016, Intel Corporation.
> + * Author: Rajneesh Bhardwaj (rajneesh.bhardwaj@xxxxxxxxx)
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + */
> +
> +#ifndef PMC_CORE_H
> +#define PMC_CORE_H
> +
> +/* Skylake Power Management Controller PCI Device ID */
> +#define PCI_DEVICE_ID_SKL_PMC 0x9d21
> +#define PMC_BASE_ADDR_OFFSET 0x48
> +#define PMC_SLP_S0_RESIDENCY_COUNTER 0x13c
> +#define PMC_MMIO_REG_LEN 0x100
> +#define PMC_REG_BIT_WIDTH 0x20
> +#define SLP_S0_RESIDENCY_COUNTER_GRANULARITY 0x64
Inconsistent spaces. Maybe just use one space.
[...]
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> new file mode 100644
> index 0000000..42cee87
> --- /dev/null
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -0,0 +1,200 @@
> +/*
> + * Intel Core SOC Power Management Controller Driver
> + *
> + * Copyright (c) 2016, Intel Corporation.
> + * Author: Rajneesh Bhardwaj (rajneesh.bhardwaj@xxxxxxxxx)
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <asm/cpu_device_id.h>
> +#include <asm/pmc_core.h>
> +
> +static struct pmc_dev pmc;
> +
> +static const struct pci_device_id pmc_pci_ids[] = {
> + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_SKL_PMC), (kernel_ulong_t)NULL },
Minor thing here but maybe just use a simple 0 here instead of
(kernel_ulong_t)NULL? In the other places as well.
[...]
> +static int pmc_core_dbgfs_register(struct pmc_dev *pmc)
> +{
> + return 0; /* nothing to register */
> +}
> +
> +#endif /* CONFIG_DEBUG_FS */
> +
> +static const struct x86_cpu_id intel_pmc_core_ids[] = {
> + { X86_VENDOR_INTEL, 6, 0x4e, X86_FEATURE_MWAIT,
> + (kernel_ulong_t)NULL}, /* SKL CPU*/
> + { X86_VENDOR_INTEL, 6, 0x5e, X86_FEATURE_MWAIT,
> + (kernel_ulong_t)NULL}, /* SKL CPU*/
> + {}
A space after CPU? SKL? I assume this means skylake so perhaps add the whole
name.
> +};
> +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
> +
> +static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id)
> +{
> + int err;
> + const struct x86_cpu_id *cpu_id;
> +
> + cpu_id = x86_match_cpu(intel_pmc_core_ids);
> + if (!cpu_id)
> + return -EINVAL;
> +
> + err = pci_enable_device(dev);
> + if (err) {
> + dev_err(&dev->dev, "PMC Core: failed to enable Power Management Controller.\n");
> + goto exit;
> + }
> +
> + pci_read_config_dword(dev, PMC_BASE_ADDR_OFFSET, &pmc.base_addr);
Shouldn't the return value of this function be checked if an error occured? Also
while we are at it maybe the label names could be improved. For example:
err_disable_dev; err_return
> + err = pmc_core_dbgfs_register(&pmc);
> + if (err) {
> + dev_err(&dev->dev, "PMC Core: debugfs register failed\n");
> + iounmap(pmc.regmap);
> + goto disable;
> + }
> +
> + pmc.feature_available = 1;
> + return 0;
> +
> +disable:
> + pci_disable_device(dev);
> +exit:
> + return err;
> +}
> +
--
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
