On Thu, Apr 21, 2016 at 03:47:29PM +0300, Giedrius Statkevičius wrote:
> 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.
>
> [...]
>
Please provide the rational for a request for change. What do you think is wrong
with the above? There is precedent in the kernel today for this usage. Is it
wrong? Unnecessary? Subjectively ugly?
> > +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.
>
The three letter CPU code is very common throughout the kernel of Intel CPUs.
NHM,WSM,SNB,IVB,BYT,HSW,BDW,SKL, etc.
Those are becoming more and more difficult to grep for though, so a full
expansion now and then would certainly be nice :-)
> > +};
> > +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
Yes please.
> while we are at it maybe the label names could be improved. For example:
> err_disable_dev; err_return
>
This is more typical, although "out" or "err_out" if only used in the error
case. However, if the label does nothing but return with an error code, be sure
to be consistent in it's usage. Don't do "return -EINVAL" and "ret = -EINVAL;
goto err_out;" in the same function.
Thanks,
> > + 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;
> > +}
> > +
>
--
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
