On Fri, 2014-03-07 at 10:08 +0800, Li, Aubrey wrote: > The Power Management Controller (PMC) controls many of the power > management features present in the SoC. This driver provides > interface to configure the Power Management Controller (PMC).
More trivial notes. Nothing really that should stop this from being applied. > diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c I still think that #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt is useful here. > +#include <linux/module.h> > +#include <linux/init.h> [] > +#define DRIVER_NAME "pmc_atom" Maybe #define DRIVER_NAME KBUILD_MODNAME too > +struct pmc_dev_map { > + char *name; > + u32 bit_mask; > +}; > > +static struct pmc_dev_map dev_map[] = { static const to reduce data? struct pmc_dev_map { const char *name; u32 bit_mask; }; > +static struct pmc_dev_map dev_map[] = { static const struct pmc_dev_map dev_map[] = { > + {"0 - LPSS1_F0_DMA", BIT_LPSS1_F0_DMA}, > + {"1 - LPSS1_F1_PWM1", BIT_LPSS1_F1_PWM1}, [] > +static void pmc_power_off(void) > +{ [] > + pr_info("%s: Preparing to enter system sleep state S5\n", DRIVER_NAME); This is larger code than using pr_fmt > +#ifdef CONFIG_DEBUG_FS > +static int pmc_dev_state_show(struct seq_file *s, void *unused) > +{ [] > + seq_printf(s, "Dev: %-32s\tState: %s [%s]\n", > + dev_map[dev_index].name, > + dev_map[dev_index].bit_mask & func_dis_index ? > + "Disabled" : "Enabled ", > + (d3_sts_index & dev_map[dev_index].bit_mask) ? > + "D3" : "D0"); This reverses the order of the 1st and 2nd trigraph test and to me would be easier to read like: seq_printf(s, "Dev: %-32s\tState: %s [%s]\n", dev_map[dev_index].name, dev_map[dev_index].bit_mask & func_dis_index ? "Disabled" : "Enabled ", dev_map[dev_index].bit_mask & d3_sts_index ? "D3" : "D0"); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/