All of lore.kernel.org
 help / color / mirror / Atom feed
From: swarren@wwwdotorg.org (Stephen Warren)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2] pinctrl/pinconfig: add debug interface
Date: Wed, 17 Apr 2013 09:21:30 -0600	[thread overview]
Message-ID: <516EBDFA.90201@wwwdotorg.org> (raw)
In-Reply-To: <516EB2B7.30607@st.com>

On 04/17/2013 08:33 AM, Laurent MEUNIER wrote:
> On 04/04/2013 11:11 PM, Stephen Warren wrote:
>> On 04/03/2013 01:47 PM, Linus Walleij wrote:
>>> From: Laurent Meunier <laurent.meunier@st.com>
>>>
>>> This update adds a debugfs interface to modify a pin configuration
>>> for a given state in the pinctrl map. This allows to modify the
>>> configuration for a non-active state, typically sleep state.
>>> This configuration is not applied right away, but only when the state
>>> will be entered.
>>>
>>> This solution is mandated for us by HW validation: in order
>>> to test and verify several pin configurations during sleep without
>>> recompiling the software.
>>>
>>> diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
>>> +enum pinconf_dbg_request_type {
>>> +	PINCONF_DBG_REQ_TYPE_INVALID,
>>> +	PINCONF_DBG_REQ_TYPE_MODIFY,
>>> +};
>> I'm not sure why that really needs an enum yet, since only one operation
>> is supported.
>
> I agree the enum does not bring a lot of added value, for now.
> It is there following one earlier comment that the debugfs
> could be later extended to support ADD / DELETE. This is
> a preparation step to this, so I've kept it in in my V3.
> Hope this is fine.

That seems like something that could easily be added later. right now,
it's just dead code. I'll let LinusW make the call though.

>>> +struct dbg_cfg {
>>> +	enum pinconf_dbg_request_type req_type;
>>> +	enum pinctrl_map_type map_type;
>>> +	char dev_name[MAX_NAME_LEN+1];
>>> +	char state_name[MAX_NAME_LEN+1];
>>> +	char pin_name[MAX_NAME_LEN+1];
>>> +	char config[MAX_NAME_LEN+1];
>>> +};
>
>> Many of those fields are only used internally to
>> pinconf_dbg_config_write(). Can't they be stored on the stack there
>> rather than globally.
>
> I'd like to get the context as a global to share it between _print()
> and _write() functions. I find it quite useful in practice
> basically when using the debugfs, after the write request,
> you can simply cat pinconf-config and check the status of
> what you've last written to. This is the rationale at least.

Data that truly is shared between print() and write() would have to be
global. My point is that many of those fields are not shared, and hence
there's no point making them global.

WARNING: multiple messages have this Message-ID (diff)
From: Stephen Warren <swarren@wwwdotorg.org>
To: Laurent MEUNIER <laurent.meunier@st.com>
Cc: Linus WALLEIJ <linus.walleij@stericsson.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Stephen Warren <swarren@nvidia.com>,
	Anmar Oueja <anmar.oueja@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH v2] pinctrl/pinconfig: add debug interface
Date: Wed, 17 Apr 2013 09:21:30 -0600	[thread overview]
Message-ID: <516EBDFA.90201@wwwdotorg.org> (raw)
In-Reply-To: <516EB2B7.30607@st.com>

On 04/17/2013 08:33 AM, Laurent MEUNIER wrote:
> On 04/04/2013 11:11 PM, Stephen Warren wrote:
>> On 04/03/2013 01:47 PM, Linus Walleij wrote:
>>> From: Laurent Meunier <laurent.meunier@st.com>
>>>
>>> This update adds a debugfs interface to modify a pin configuration
>>> for a given state in the pinctrl map. This allows to modify the
>>> configuration for a non-active state, typically sleep state.
>>> This configuration is not applied right away, but only when the state
>>> will be entered.
>>>
>>> This solution is mandated for us by HW validation: in order
>>> to test and verify several pin configurations during sleep without
>>> recompiling the software.
>>>
>>> diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
>>> +enum pinconf_dbg_request_type {
>>> +	PINCONF_DBG_REQ_TYPE_INVALID,
>>> +	PINCONF_DBG_REQ_TYPE_MODIFY,
>>> +};
>> I'm not sure why that really needs an enum yet, since only one operation
>> is supported.
>
> I agree the enum does not bring a lot of added value, for now.
> It is there following one earlier comment that the debugfs
> could be later extended to support ADD / DELETE. This is
> a preparation step to this, so I've kept it in in my V3.
> Hope this is fine.

That seems like something that could easily be added later. right now,
it's just dead code. I'll let LinusW make the call though.

>>> +struct dbg_cfg {
>>> +	enum pinconf_dbg_request_type req_type;
>>> +	enum pinctrl_map_type map_type;
>>> +	char dev_name[MAX_NAME_LEN+1];
>>> +	char state_name[MAX_NAME_LEN+1];
>>> +	char pin_name[MAX_NAME_LEN+1];
>>> +	char config[MAX_NAME_LEN+1];
>>> +};
>
>> Many of those fields are only used internally to
>> pinconf_dbg_config_write(). Can't they be stored on the stack there
>> rather than globally.
>
> I'd like to get the context as a global to share it between _print()
> and _write() functions. I find it quite useful in practice
> basically when using the debugfs, after the write request,
> you can simply cat pinconf-config and check the status of
> what you've last written to. This is the rationale at least.

Data that truly is shared between print() and write() would have to be
global. My point is that many of those fields are not shared, and hence
there's no point making them global.

  reply	other threads:[~2013-04-17 15:21 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-03 19:47 [PATCH v2] pinctrl/pinconfig: add debug interface Linus Walleij
2013-04-03 19:47 ` Linus Walleij
2013-04-04 21:11 ` Stephen Warren
2013-04-04 21:11   ` Stephen Warren
2013-04-17 14:33   ` Laurent MEUNIER
2013-04-17 14:33     ` Laurent MEUNIER
2013-04-17 15:21     ` Stephen Warren [this message]
2013-04-17 15:21       ` Stephen Warren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=516EBDFA.90201@wwwdotorg.org \
    --to=swarren@wwwdotorg.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.