* Srinivas Pandruvada <srinivas.pandruv...@linux.intel.com> wrote:
> The patch adds a debug driver, which dumps the power states of all
> the North complex (NC) devices. This debug interface is useful to
> figure out the devices, which blocks the S0ix transitions on the
> platform. This is extremely useful during enabling PM on customer
> platforms and derivatives.
Looks mostly good. Small nits:
> +config PUNIT_ATOM_DEBUG
> + tristate "ATOM Punit debug driver"
> + depends on DEBUG_FS
> + select IOSF_MBI
I suspect you could select DEBUG_FS as well? Half of the drivers seem
to do that.
> + ---help---
> + This is a debug driver, which gets the power states
> + of all Punit North Complex devices. The power states of
> + each device is exposed as part of the debugfs interface.
Might as well mention the path of the file? To keep people from
guessing and so.
> +static int punit_dev_state_show(struct seq_file *seq_file, void *unused)
> +{
> + u32 punit_pwr_status;
> + struct punit_device *punit_devp = punit_device;
You could stick stick 'punit_device' into s->private? You do that by
passing it to debugfs_create_file(). That way you could avoid the
fugly static allocation of 'punit_device' and its global setting in
punit_atom_debug_init().
> + int index;
> + int status;
> +
> + seq_puts(seq_file, "\n\nPUNIT NORTH COMPLEX DEVICES :\n");
> + while (punit_devp->name) {
> + status = iosf_mbi_read(PUNIT_PORT, BT_MBI_PMC_READ,
> + punit_devp->reg,
> + &punit_pwr_status);
> + if (status)
> + seq_printf(seq_file, "%9s : Read Failed\n",
> + punit_devp->name);
> + else {
> + index = (punit_pwr_status >> punit_devp->sss_pos) & 3;
> + seq_printf(seq_file, "%9s : %s\n", punit_devp->name,
> + dstates[index]);
> + }
We only use symmetric curly braces in the kernel.
> +#define ICPU(model, drv_data) \
> + { X86_VENDOR_INTEL, 6, model, X86_FEATURE_MWAIT,\
> + (kernel_ulong_t)&drv_data }
> +
> +static const struct x86_cpu_id intel_punit_cpu_ids[] = {
> + ICPU(0x4c, punit_device_cht),
> + ICPU(0x37, punit_device_byt),
> + {}
> +};
So should the models be listed in increasing order?
Also, I'd use decimal, as we do for models typically. Also, might as
well mention which Intel Atom models those are: 22nm Atom "Silvermont"
and 14nm Atom "Airmont", right?
Thanks,
Ingo
--
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/
