Date: Sat, 2 May 2015 08:00:51 +0200
From: Ingo Molnar <mingo@kernel.org>
To: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, x86@kernel.org,
        linux-kernel@vger.kernel.org, pebolle@tiscali.nl,
        Borislav Petkov <bp@alien8.de>
Subject: Re: [PATCH v1] x86: punit_atom: punit device state debug driver
Message-ID: <20150502060050.GA10932@gmail.com>
References: <1430521630-19788-1-git-send-email-srinivas.pandruvada@linux.intel.com>
 <1430521630-19788-2-git-send-email-srinivas.pandruvada@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1430521630-19788-2-git-send-email-srinivas.pandruvada@linux.intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4283
Lines: 131


* Srinivas Pandruvada <srinivas.pandruvada@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 NC IPs which blocks the S0ix
> transitions on the platform. This is extremely useful during
> enabling PM on customer platforms and derivatives.

Looks useful.

> This submission not a complete rewrite from the original
> submission from Kumar P, Mahesh <mahesh.kumar.p.intel.com>.
> https://lkml.org/lkml/2014/11/5/367

This sentence does not parse. Did you mean 'is a complete rewrite'?

> +config PUNIT_ATOM_DEBUG
> +	tristate "ATOM Punit debug driver"
> +	depends on DEBUG_FS
> +	select IOSF_MBI
> +	---help---
> +	  This is a debug driver, which gets the power states
> +	  of all Punit North Complex devices.The power states of
> +	  each IP is exposed as part of the debugfs interface.

What is an 'IP'?

Also:

      s/devices.The
       /devices. The

> +
>  endmenu
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index cdb1b70..2ecbd55 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -110,6 +110,7 @@ obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o
>  obj-$(CONFIG_TRACING)			+= tracepoint.o
>  obj-$(CONFIG_IOSF_MBI)			+= iosf_mbi.o
>  obj-$(CONFIG_PMC_ATOM)			+= pmc_atom.o
> +obj-$(CONFIG_PUNIT_ATOM_DEBUG)		+= punit_atom_debug.o
>  
>  ###
>  # 64 bit specific files
> diff --git a/arch/x86/kernel/punit_atom_debug.c b/arch/x86/kernel/punit_atom_debug.c

Please put this somewhere suitable in arch/x86/platform/.

> +/*
> + * Intel SOC Punit device state debug driver
> + * Copyright (c) 2015, Intel Corporation.

Would be nice to add a blurb about what 'Intel SOC Punit' systems are. 
There doesn't seem to be anything in the kernel source about it, so 
you have a golden opportunity here.

> +#define PUNIT_PORT		0x04
> +#define PWRGT_STATUS		0x61 /* Power gate status reg */
> +#define VED_SS_PM0		0x32 /* Subsystem config/status Video */
> +#define ISP_SS_PM0		0x39 /* Subsystem config/status ISP */
> +#define MIO_SS_PM		0x3B /* Subsystem config/status MIO */
> +#define SSS_SHIFT		24
> +#define RENDER_POS		0
> +#define MEDIA_POS		2
> +#define VLV_DISPLAY_POS		6
> +#define CHT_DSP_SSS		0x36 /* Subsystem config/status DSP */
> +#define CHT_DSP_SSS_POS		16

All the magic constants need some explanation for humans, not just 
some.

> +static const struct punit_device punit_device_byt[] = {
> +	{ "GFX RENDER", PWRGT_STATUS, RENDER_POS },
> +	{ "GFX MEDIA", PWRGT_STATUS, MEDIA_POS },
> +	{ "DISPLAY", PWRGT_STATUS, VLV_DISPLAY_POS },
> +	{ "VED", VED_SS_PM0, SSS_SHIFT},
> +	{ "ISP", ISP_SS_PM0, SSS_SHIFT},
> +	{ "MIO", MIO_SS_PM, SSS_SHIFT},
> +	{ NULL}
> +};
> +
> +static const struct punit_device punit_device_cht[] = {
> +	{ "GFX RENDER", PWRGT_STATUS, RENDER_POS },
> +	{ "GFX MEDIA", PWRGT_STATUS, MEDIA_POS },
> +	{ "DSP", CHT_DSP_SSS,  CHT_DSP_SSS_POS },
> +	{ "VED", VED_SS_PM0, SSS_SHIFT},
> +	{ "ISP", ISP_SS_PM0, SSS_SHIFT},
> +	{ "MIO", MIO_SS_PM, SSS_SHIFT},
> +	{ NULL}
> +};

Please align such initialization tables vertically, like you did here:

> +static const struct file_operations punit_dev_state_ops = {
> +	.open		= punit_dev_state_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= single_release,
> +};


> +	punit_dbg_file = debugfs_create_dir("punit_atom", NULL);
> +	if (!punit_dbg_file)
> +		return -ENXIO;
> +
> +	dev_state = debugfs_create_file("dev_state", S_IFREG | S_IRUGO,
> +					 punit_dbg_file, NULL,
> +					 &punit_dev_state_ops);

So why isn't something that is declaradly a power state dump, called 
"power_state" or "dev_power_state"?

> +MODULE_AUTHOR("Kumar P, Mahesh <mahesh.kumar.p.intel.com>");
> +MODULE_DESCRIPTION("Driver for Punit devices states debugging");
> +MODULE_LICENSE("GPL v2");

Btw., you can have multiple MODULE_AUTHOR() lines, so you can credit
yourself as well. Take a look at drivers/net/wireless/at76c50x-usb.c's 
MODULE_AUTHOR() for grins.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/