On Tue, Nov 16, 2010 at 05:36:39PM +0530, Rudramuni, Vishwesh M wrote:
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> +config INTEL_MID_POWER
> + bool "Power Management driver for Intel Mid platform"
> + depends on X86 && CPU_IDLE && X86_PAE
Can you help me understand why it depends on X86_PAE? There are many
appearances of both #ifdef CONFIG_X86_PAE and #ifndef CONFIG_X86_PAE in
your patch series and it looks like the code runs in both cases.
> diff --git a/drivers/platform/x86/mid_power/mid_pmu.c
> b/drivers/platform/x86/mid_power/mid_pmu.c
...
> +/* the device object */
> +struct pci_dev *pmu_dev;
> +
> +/*virtual memory address for PMU base returned by ioremap_nocache().*/
> +struct mid_pmu_dev intel_mid_pmu_base;
> +unsigned long intel_mid_wakeup_address;
Better to encapsulate these variables and mark them static as well.
> +static int display_status;
> +
> +/* Create a work queue for sending events */
> +static struct workqueue_struct *event_work;
> +
> +/* stucture for event */
> +struct event {
> + u32 orig;
> + unsigned int type;
> + enum pmu_number pmu_num;
> + struct work_struct eventq;
> +};
> +
> +struct event *pmu_event;
static?
> +
> +static int pmu_pci_devices_suspend(struct pmu_ss_states *pmssc);
> +static int pmu_pci_devices_resume(struct pmu_ss_states *pmssc);
Why do you need these forward declarations? I saw all callers are
defined after them.
> +/**
> + * pmu_get_inactivity_source - This function is platform specific function
> + * for programming inactivity counters.
> + */
> +int pmu_get_inactivity_source(u32 *ina_source, int pmu_num)
Where is the user of this function?
> +void prepare_s0i3(void)
static?
> +/**
> + * pmu_sc_irq - pmu driver interrupt handler
> + * Context: interrupt context
> + *
> + * PMU interrupt handler based on the status of the interrupt
> + * will send net link event to PM Framework.
> + * - During inactivity interrupt it sends the information of inactivity
> + * counter that caused the interrupt as net link event.
> + * - During wake interrupt it sends the information of source of wake
> + * sub system that caused the wake interrupt as net link event.
> + */
> +static inline irqreturn_t pmu_sc_irq(int irq, void *pmu_unit)
Are you sure you want to inline this huge function? In addition, irq
handler typically is not called directly.
> +int pmu_program_inactivity_threshold(u32 sub_sys_num,
> + u32 thresh_value,
> + int pmu_num)
Where is the user of this function?
> +int pmu_read_inactivity_counter(u32 sub_sys_num,
> + u32 *counter_val,
> + int pmu_num)
ditto.
> +int check_sub_sys_validity(bool check_pmu, struct pmu_ss_states *pmssc)
static?
> +void copy_sub_systems(u32 config, int start, int end)
static?
> +static int get_index(struct pci_dev *pdev)
> +{
> + int pm, type;
> + unsigned int base_class;
> + unsigned int sub_class;
> + u8 ss;
> + u32 pmu1_max_devs;
> +
> + if (__mrst_cpu_chip == MRST_CPU_CHIP_PENWELL)
> + pmu1_max_devs = PMU1_MAX_PENWELL_DEVS;
> + else
> + pmu1_max_devs = PMU1_MAX_MRST_DEVS;
> +
> + base_class = pdev->class >> 16;
> + sub_class = (pdev->class & SUB_CLASS_MASK) >> 8;
> + pm = pci_find_capability(pdev, PCI_CAP_ID_VNDR);
> +
> + /* read the logical sub system id & cap if present */
> + pci_read_config_byte(pdev, pm + 4, &ss);
> +
> + type = ss & 0x80;
What does 0x80 mean? Better to have a macro definition for things like
this.
> + ss = ss & LOG_ID_MASK;
> +
> + if ((base_class == PCI_BASE_CLASS_DISPLAY) && !sub_class)
> + return 1;
> +
> + if (__mrst_cpu_chip == MRST_CPU_CHIP_PENWELL) {
> + if ((base_class == PCI_BASE_CLASS_MULTIMEDIA) &&
> + (sub_class == 0x4))
> + return 7;
> + else
> + return pmu1_max_devs + ss;
> + } else if (type)
> + return pmu1_max_devs + ss;
> +
> + return PMU_FAILED;
> +}
> +
> +void mid_pci_find_info(struct pci_dev *pdev)
static?
> +void pmu_enumerate(void)
static?
> +int pmu_mode_change(struct pmu_ss_states *pmssc, int mode, int ioc)
> +{
...
> +#if 0
> + /* Check the sub systems states validity */
> + status = check_sub_sys_validity(false, pmssc);
> + if (status != PMU_SUCCESS) {
> + dev_dbg(&pmu_dev->dev, "sub system validity check failed\n");
> + return status;
> + }
> +#endif
If this is dead code, please remove.
> +int pmu_init(void)
static?
> +EXPORT_SYMBOL(pmu_init);
Why do you need to export this?
> +EXPORT_SYMBOL(pmu_exit);
ditto.
> +
> +/**
> + * mid_pmu_probe - This is the function where most of the PMU driver
> + * initialization happens.
> + */
> +static int __devinit mid_pmu_probe(struct pci_dev *dev,
> + const struct pci_device_id *pci_id)
> +{
> +
> + int ret;
> + u32 pm_base, pm_len;
> + u32 bar_value;
> + u32 config;
> + struct pci_dev *pci_root = pci_get_bus_and_slot(0, 0);
> + u32 ctrl_reg = 0xD0;
> + u32 data_reg = 0xD4;
I saw you defined MSG_CMD_REG and MSG_DATA_REG. Why not use them?
> + /*Map PMU2 registers */
> + bar_value = 0;
> + pm_base = pci_read_config_dword(dev, PCI_BASE_ADDRESS_0, \
> + &bar_value);
> + pm_base = bar_value;
> + pm_len = PMU2_END_ADDRESS;
> +
> + /* Map the memory of pmu1 PMU reg base */
> + base_addr.pmu2_base = pci_iomap(dev, pm_base, pm_len);
Are you sure you tested your patches before submission? pmu2 is not mapped
properly and you will not be able to access pmu2 registers given how you
called pci_iomap. Please take a look at any PCI driver to see how to use
pci_iomap.
> +static void __devexit mid_pmu_remove(struct pci_dev *dev)
> +{
> + dev_dbg(&pmu_dev->dev, "Mid PM mid_pmu_remove called\n");
> +
> + /* Freeing up the irq */
> + free_irq(dev->irq, &pmu_sc_irq);
> +
> + /* Freeing the resources allocated in pmu_init() */
> + pmu_exit();
> +
> + /* Freeing up memory allocated for PMU1 & PMU2 */
> + pci_iounmap(dev, base_addr.pmu2_base);
> + base_addr.pmu1_base = NULL;
> + base_addr.pmu2_base = NULL;
> +
> + /* disable the current PCI device */
> + pci_release_region(dev, 0);
Have you ever called pci_request_regions?
> diff --git a/drivers/platform/x86/mid_power/mid_pmu.h
> b/drivers/platform/x86/mid_power/mid_pmu.h
> +#define MODE_ID_MAGIC_NUM 0
Better to use a nonzero value as magic number since the default value
of PM_STS after reset is 0.
> +/* Definitions for Message Bus Interface */
> +#define MSG_CMD_REG 0xD0
> +#define MSG_DATA_REG 0xD4
> +#define MSG_BUS_READ_OPCODE 0xD0
> +#define MSG_BUS_WRITE_OPCODE 0xE0
> +#define MSG_BUS_WRITE_ENABLES 0xF
> +#define PM_BLK_BASE_ADDR 0x70
> +#define PUNIT_PORT_NUM 0x04
> +#define PMU1_BUS_NUM 0
> +#define PMU1_DEV_NUM 0
> +#define PMU1_FUNC_NUM 0
> +#define PCNT_LEN 4
> +#define C4_STATE 4
> +#define C6_STATE 4
I do not see where the above macro definitions are used.
> +#define MID_PMU_DRV_VENDOR_ID 0x8086
This is redundant. I saw you already used the standard Intel vendor id.
> +struct cfg_trig_param_t {
> + u32 cmd:8;
> + u32 ioc:1;
> + u32 cfg_mode:4;
> + u32 mode_id:3;
> + u32 sys_state:3;
> + u32 cfg_trig_type:3;
> + u32 cfg_trig_val:8;
> + u32 cbmi:1;
Like I said earlier, please fix the naming. Having a bug somewhere else
is not the execuse of introducing another bug.
> diff --git a/include/linux/intel_mid.h b/include/linux/intel_mid.h
> +/* Error codes for pmu */
> +#define PMU_SUCCESS 0
> +#define PMU_FAILED -1
> +#define PMU_BUSY_STATUS 0
> +#define PMU_MODE_ID 1
These definitions can also be found in mid_pmu.h where you should
actually include intel_mid.h to avoid redefining them.
> +#define SET_MODE 1
> +#define SET_AOAC_S0i1 2
> +#define SET_AOAC_S0i3 3
> +#define SET_LPAUDIO 4
> +#define CONFIG_INACTIVITY 5
> +#define GET_SUBSYSTEM_INFO 6
Did not see where you use CONFIG_INACTIVITY and GET_SUBSYSTEM_INFO.
> +#define SET_AOAC_S0i2 7
> +
> +struct pci_dev_info {
> + u32 log_id;
> + u32 cap;
> + struct pci_dev *dev_driver;
This obviouly points to a device. Why do you name it driver?
-Yong
_______________________________________________
MeeGo-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
