Re: [PATCH v3] x86: punit_atom: punit device state debug driver

[Ingo Molnar]

* Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/