[PATCH] pinctrl/pinconfig: add debug interface

[PATCH] pinctrl/pinconfig: add debug interface

[Linus Walleij]

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.

Signed-off-by: Laurent Meunier <laurent.meunier@st.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/core.c    |  18 +---
 drivers/pinctrl/core.h    |  21 +++++
 drivers/pinctrl/pinconf.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 229 insertions(+), 17 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index b0a8e9a..b0de6e7 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -32,17 +32,6 @@
 #include "pinmux.h"
 #include "pinconf.h"
 
-/**
- * struct pinctrl_maps - a list item containing part of the mapping table
- * @node: mapping table list node
- * @maps: array of mapping table entries
- * @num_maps: the number of entries in @maps
- */
-struct pinctrl_maps {
-	struct list_head node;
-	struct pinctrl_map const *maps;
-	unsigned num_maps;
-};
 
 static bool pinctrl_dummy_state;
 
@@ -56,13 +45,8 @@ LIST_HEAD(pinctrldev_list);
 static LIST_HEAD(pinctrl_list);
 
 /* List of pinctrl maps (struct pinctrl_maps) */
-static LIST_HEAD(pinctrl_maps);
+LIST_HEAD(pinctrl_maps);
 
-#define for_each_maps(_maps_node_, _i_, _map_) \
-	list_for_each_entry(_maps_node_, &pinctrl_maps, node) \
-		for (_i_ = 0, _map_ = &_maps_node_->maps[_i_]; \
-			_i_ < _maps_node_->num_maps; \
-			_i_++, _map_ = &_maps_node_->maps[_i_])
 
 /**
  * pinctrl_provide_dummies() - indicate if pinctrl provides dummy state support
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index fdd350d..0aacc27 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -155,6 +155,18 @@ struct pin_desc {
 #endif
 };
 
+/**
+ * struct pinctrl_maps - a list item containing part of the mapping table
+ * @node: mapping table list node
+ * @maps: array of mapping table entries
+ * @num_maps: the number of entries in @maps
+ */
+struct pinctrl_maps {
+	struct list_head node;
+	struct pinctrl_map const *maps;
+	unsigned num_maps;
+};
+
 struct pinctrl_dev *get_pinctrl_dev_from_devname(const char *dev_name);
 int pin_get_from_name(struct pinctrl_dev *pctldev, const char *name);
 const char *pin_get_name(struct pinctrl_dev *pctldev, const unsigned pin);
@@ -176,3 +188,12 @@ extern int pinctrl_force_default(struct pinctrl_dev *pctldev);
 
 extern struct mutex pinctrl_mutex;
 extern struct list_head pinctrldev_list;
+extern struct list_head pinctrl_maps;
+
+#define for_each_maps(_maps_node_, _i_, _map_) \
+	list_for_each_entry(_maps_node_, &pinctrl_maps, node) \
+		for (_i_ = 0, _map_ = &_maps_node_->maps[_i_]; \
+			_i_ < _maps_node_->num_maps; \
+			_i_++, _map_ = &_maps_node_->maps[_i_])
+
+
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index baee2cc..ac8d382 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -574,6 +574,207 @@ static const struct file_operations pinconf_groups_ops = {
 	.release	= single_release,
 };
 
+/* 32bit read/write ressources */
+#define MAX_NAME_LEN 16
+char dbg_pinname[MAX_NAME_LEN]; /* shared: name of the state of the pin*/
+char dbg_state_name[MAX_NAME_LEN]; /* shared: state of the pin*/
+static u32 dbg_config; /* shared: config to be read/set for the pin & state*/
+
+static int pinconf_dbg_pinname_print(struct seq_file *s, void *d)
+{
+	if (strlen(dbg_pinname))
+		seq_printf(s, "%s\n", dbg_pinname);
+	else
+		seq_printf(s, "No pin name set\n");
+	return 0;
+}
+
+static int pinconf_dbg_pinname_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pinconf_dbg_pinname_print, inode->i_private);
+}
+
+static int pinconf_dbg_pinname_write(struct file *file,
+	const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	int err;
+
+	if (count > MAX_NAME_LEN)
+		return -EINVAL;
+
+	err = sscanf(user_buf, "%15s", dbg_pinname);
+
+	if (err != 1)
+		return -EINVAL;
+
+	return count;
+}
+
+static const struct file_operations pinconf_dbg_pinname_fops = {
+	.open = pinconf_dbg_pinname_open,
+	.write = pinconf_dbg_pinname_write,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.owner = THIS_MODULE,
+};
+
+static int pinconf_dbg_state_print(struct seq_file *s, void *d)
+{
+	if (strlen(dbg_state_name))
+		seq_printf(s, "%s\n", dbg_pinname);
+	else
+		seq_printf(s, "No pin state set\n");
+	return 0;
+}
+
+static int pinconf_dbg_state_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pinconf_dbg_state_print, inode->i_private);
+}
+
+static int pinconf_dbg_state_write(struct file *file,
+	const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	int err;
+
+	if (count > MAX_NAME_LEN)
+		return -EINVAL;
+
+	err = sscanf(user_buf, "%15s", dbg_state_name);
+
+	if (err != 1)
+		return -EINVAL;
+
+	return count;
+}
+
+static const struct file_operations pinconf_dbg_pinstate_fops = {
+	.open = pinconf_dbg_state_open,
+	.write = pinconf_dbg_state_write,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.owner = THIS_MODULE,
+};
+
+/**
+ * pinconf_dbg_config_print() - display the pinctrl config from the pinctrl
+ * map, of a pin/state pair based on pinname and state that have been
+ * selected with the debugfs entries pinconf-name and pinconf-state
+ * @s: contains the 32bits config to be written
+ * @d: not used
+ */
+static int pinconf_dbg_config_print(struct seq_file *s, void *d)
+{
+	struct pinctrl_maps *maps_node;
+	struct pinctrl_map const *map;
+	struct pinctrl_dev *pctldev = NULL;
+	struct pinconf_ops *confops = NULL;
+	int i, j;
+	bool found = false;
+
+	mutex_lock(&pinctrl_mutex);
+
+	/* Parse the pinctrl map and look for the elected pin/state */
+	for_each_maps(maps_node, i, map) {
+		if (map->type != PIN_MAP_TYPE_CONFIGS_PIN)
+			continue;
+
+		if (strncmp(map->name, dbg_state_name, MAX_NAME_LEN) > 0)
+			continue;
+
+		for (j = 0; j < map->data.configs.num_configs; j++) {
+			if (0 == strncmp(map->data.configs.group_or_pin,
+						dbg_pinname, MAX_NAME_LEN)) {
+				/* We found the right pin / state, read the
+				 * config and store the pctldev */
+				dbg_config = map->data.configs.configs[j];
+				pctldev = get_pinctrl_dev_from_devname
+					(map->ctrl_dev_name);
+				found = true;
+				break;
+			}
+		}
+	}
+
+	mutex_unlock(&pinctrl_mutex);
+
+	if (found) {
+		seq_printf(s, "Config of %s in state %s: 0x%08X\n", dbg_pinname,
+				 dbg_state_name, dbg_config);
+
+		if (pctldev)
+			confops = pctldev->desc->confops;
+
+		if (confops && confops->pin_config_config_dbg_show)
+			confops->pin_config_config_dbg_show(pctldev,
+					s, dbg_config);
+	} else {
+		seq_printf(s, "No pin found for defined name/state\n");
+	}
+
+	return 0;
+}
+
+static int pinconf_dbg_config_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pinconf_dbg_config_print, inode->i_private);
+}
+
+/**
+ * pinconf_dbg_config_write() - overwrite the pinctrl config in thepinctrl
+ * map, of a pin/state pair based on pinname and state that have been
+ * selected with the debugfs entries pinconf-name and pinconf-state
+ */
+static int pinconf_dbg_config_write(struct file *file,
+	const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	int err;
+	unsigned long config;
+	struct pinctrl_maps *maps_node;
+	struct pinctrl_map const *map;
+	int i, j;
+
+	err = kstrtoul_from_user(user_buf, count, 0, &config);
+
+	if (err)
+		return err;
+
+	dbg_config = config;
+
+	mutex_lock(&pinctrl_mutex);
+
+	/* Parse the pinctrl map and look for the selected pin/state */
+	for_each_maps(maps_node, i, map) {
+		if (map->type != PIN_MAP_TYPE_CONFIGS_PIN)
+			continue;
+
+		if (strncmp(map->name, dbg_state_name, MAX_NAME_LEN) > 0)
+			continue;
+
+		/*  we found the right pin / state, so overwrite config */
+		for (j = 0; j < map->data.configs.num_configs; j++) {
+			if (strncmp(map->data.configs.group_or_pin, dbg_pinname,
+						MAX_NAME_LEN) == 0)
+				map->data.configs.configs[j] = dbg_config;
+		}
+	}
+
+	mutex_unlock(&pinctrl_mutex);
+
+	return count;
+}
+
+static const struct file_operations pinconf_dbg_pinconfig_fops = {
+	.open = pinconf_dbg_config_open,
+	.write = pinconf_dbg_config_write,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.owner = THIS_MODULE,
+};
+
 void pinconf_init_device_debugfs(struct dentry *devroot,
 			 struct pinctrl_dev *pctldev)
 {
@@ -581,6 +782,12 @@ void pinconf_init_device_debugfs(struct dentry *devroot,
 			    devroot, pctldev, &pinconf_pins_ops);
 	debugfs_create_file("pinconf-groups", S_IFREG | S_IRUGO,
 			    devroot, pctldev, &pinconf_groups_ops);
+	debugfs_create_file("pinconf-name", (S_IRUGO | S_IWUSR | S_IWGRP),
+			    devroot, pctldev, &pinconf_dbg_pinname_fops);
+	debugfs_create_file("pinconf-state",  (S_IRUGO | S_IWUSR | S_IWGRP),
+			    devroot, pctldev, &pinconf_dbg_pinstate_fops);
+	debugfs_create_file("pinconf-config",  (S_IRUGO | S_IWUSR | S_IWGRP),
+			    devroot, pctldev, &pinconf_dbg_pinconfig_fops);
 }
 
 #endif
-- 
1.7.11.3

Re: [PATCH] pinctrl/pinconfig: add debug interface

[Stephen Warren]

On 02/10/2013 01:11 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.

I never understood why HW engineers can't just recompile the kernel.
Besides, it's just a device tree change these days - no recompile even
required, right?

> diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c

To be worth including, hadn't this feature also better also allow
editing of pin mux settings too, and even addition/removal of entries,
so some more core file would be a better place?

> +/* 32bit read/write ressources */
> +#define MAX_NAME_LEN 16
> +char dbg_pinname[MAX_NAME_LEN]; /* shared: name of the state of the pin*/
> +char dbg_state_name[MAX_NAME_LEN]; /* shared: state of the pin*/
> +static u32 dbg_config; /* shared: config to be read/set for the pin & state*/

Rather than setting pin name, state name, and config separately,
wouldn't it be better to accept a single write() with all those
parameters contained in it at once, so no persistent state was required.
Say something like:

modify configs_pin devicename state pinname newvalue

That would allow "add", "delete" to be implemented in addition to modify
later if desired.

> +static int pinconf_dbg_pinname_write(struct file *file,
> +	const char __user *user_buf, size_t count, loff_t *ppos)
> +{
> +	int err;
> +
> +	if (count > MAX_NAME_LEN)
> +		return -EINVAL;
> +
> +	err = sscanf(user_buf, "%15s", dbg_pinname);

That %15 had better somehow be related to MAX_NAME_LEN.

I'd suggest making MAX_NAME_LEN 15, and the variable declarations above
use MAX_NAME_LEN + 1, so that MAX_NAME_LEN can be used as part of the
scanf parameter here.

> +/**
> + * pinconf_dbg_config_write() - overwrite the pinctrl config in thepinctrl
> + * map, of a pin/state pair based on pinname and state that have been
> + * selected with the debugfs entries pinconf-name and pinconf-state
> + */
> +static int pinconf_dbg_config_write(struct file *file,
> +	const char __user *user_buf, size_t count, loff_t *ppos)
> +{
> +	int err;
> +	unsigned long config;
> +	struct pinctrl_maps *maps_node;
> +	struct pinctrl_map const *map;
> +	int i, j;
> +
> +	err = kstrtoul_from_user(user_buf, count, 0, &config);
> +
> +	if (err)
> +		return err;
> +
> +	dbg_config = config;

That assumes that pin config values are a plain u32. While this is
commonly true in existing pinctrl drivers, it certainly isn't something
that the pinctrl core mandates; that's the whole point of
dt_node_to_map() for example, to allow the individual pinctrl drivers to
use whatever type (scalar, pointer-to-struct, ...) they want to
represent their config values.

> +
> +	mutex_lock(&pinctrl_mutex);
> +
> +	/* Parse the pinctrl map and look for the selected pin/state */
> +	for_each_maps(maps_node, i, map) {
> +		if (map->type != PIN_MAP_TYPE_CONFIGS_PIN)
> +			continue;
> +
> +		if (strncmp(map->name, dbg_state_name, MAX_NAME_LEN) > 0)
> +			continue;

What about the device name; this changes that state in every device's
map table entry.

> +
> +		/*  we found the right pin / state, so overwrite config */
> +		for (j = 0; j < map->data.configs.num_configs; j++) {
> +			if (strncmp(map->data.configs.group_or_pin, dbg_pinname,
> +						MAX_NAME_LEN) == 0)

Why compare this inside the per-config loop;
map->data.configs.group_or_pin is something "global" to the entire
struct, and not specific to each configs[] entry.

> +				map->data.configs.configs[j] = dbg_config;

Given that dbg_config is written to by this function, then used by this
function, why even make it a global? The only other use is
pinconf_dbg_config_print(), which can also make it a local variable.

Overall, I'm not convinced of the utility of this patch upstream. Sorry.

Re: [PATCH] pinctrl/pinconfig: add debug interface

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130211 12:57]:
> On 02/10/2013 01:11 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.
> 
> I never understood why HW engineers can't just recompile the kernel.
> Besides, it's just a device tree change these days - no recompile even
> required, right?

Typically when bringing up a new board you do not have the driver
specific mux settings verified. For developers, it's easiest to tweak
the muxing during runtime do the drivers as a loadable module, then
export the verified mux configuration into a .dts file.

Regards,

Tony

Re: [PATCH] pinctrl/pinconfig: add debug interface

[Stephen Warren]

On 02/11/2013 02:00 PM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [130211 12:57]:
>> On 02/10/2013 01:11 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.
>>
>> I never understood why HW engineers can't just recompile the kernel.
>> Besides, it's just a device tree change these days - no recompile even
>> required, right?
> 
> Typically when bringing up a new board you do not have the driver
> specific mux settings verified. For developers, it's easiest to tweak
> the muxing during runtime do the drivers as a loadable module, then
> export the verified mux configuration into a .dts file.

Well HW engineers typically just write to the HW registers directly
rather than screwing around with the Linux pinctrl tables and
unloading/reloading drivers.

Re: [PATCH] pinctrl/pinconfig: add debug interface

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130211 13:17]:
> On 02/11/2013 02:00 PM, Tony Lindgren wrote:
> > * Stephen Warren <swarren@wwwdotorg.org> [130211 12:57]:
> >> On 02/10/2013 01:11 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.
> >>
> >> I never understood why HW engineers can't just recompile the kernel.
> >> Besides, it's just a device tree change these days - no recompile even
> >> required, right?
> > 
> > Typically when bringing up a new board you do not have the driver
> > specific mux settings verified. For developers, it's easiest to tweak
> > the muxing during runtime do the drivers as a loadable module, then
> > export the verified mux configuration into a .dts file.
> 
> Well HW engineers typically just write to the HW registers directly
> rather than screwing around with the Linux pinctrl tables and
> unloading/reloading drivers.

I've seen cases where most of the mux register configuration are
done correctly by HW engineers.. But there's always been some pieces
of the pin configuration wrong initially and that needs to be fixed by
the driver people to get things working.

Being able to set the mux configuration via debugfs is also very
useful for the case of adding external devices to your proto boards
like beagle etc.

Regards,

Tony

Re: [PATCH] pinctrl/pinconfig: add debug interface

[Stephen Warren]

On 02/11/2013 02:21 PM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [130211 13:17]:
>> On 02/11/2013 02:00 PM, Tony Lindgren wrote:
>>> * Stephen Warren <swarren@wwwdotorg.org> [130211 12:57]:
>>>> On 02/10/2013 01:11 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.
>>>>
>>>> I never understood why HW engineers can't just recompile the kernel.
>>>> Besides, it's just a device tree change these days - no recompile even
>>>> required, right?
>>>
>>> Typically when bringing up a new board you do not have the driver
>>> specific mux settings verified. For developers, it's easiest to tweak
>>> the muxing during runtime do the drivers as a loadable module, then
>>> export the verified mux configuration into a .dts file.
>>
>> Well HW engineers typically just write to the HW registers directly
>> rather than screwing around with the Linux pinctrl tables and
>> unloading/reloading drivers.
> 
> I've seen cases where most of the mux register configuration are
> done correctly by HW engineers.. But there's always been some pieces
> of the pin configuration wrong initially and that needs to be fixed by
> the driver people to get things working.
> 
> Being able to set the mux configuration via debugfs is also very
> useful for the case of adding external devices to your proto boards
> like beagle etc.

This patch won't work for that, since it doesn't support adding new
entries to the mux table, just editing existing entries.

Re: [PATCH] pinctrl/pinconfig: add debug interface

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130211 13:27]:
> On 02/11/2013 02:21 PM, Tony Lindgren wrote:
> > * Stephen Warren <swarren@wwwdotorg.org> [130211 13:17]:
> >> On 02/11/2013 02:00 PM, Tony Lindgren wrote:
> >>> * Stephen Warren <swarren@wwwdotorg.org> [130211 12:57]:
> >>>> On 02/10/2013 01:11 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.
> >>>>
> >>>> I never understood why HW engineers can't just recompile the kernel.
> >>>> Besides, it's just a device tree change these days - no recompile even
> >>>> required, right?
> >>>
> >>> Typically when bringing up a new board you do not have the driver
> >>> specific mux settings verified. For developers, it's easiest to tweak
> >>> the muxing during runtime do the drivers as a loadable module, then
> >>> export the verified mux configuration into a .dts file.
> >>
> >> Well HW engineers typically just write to the HW registers directly
> >> rather than screwing around with the Linux pinctrl tables and
> >> unloading/reloading drivers.
> > 
> > I've seen cases where most of the mux register configuration are
> > done correctly by HW engineers.. But there's always been some pieces
> > of the pin configuration wrong initially and that needs to be fixed by
> > the driver people to get things working.
> > 
> > Being able to set the mux configuration via debugfs is also very
> > useful for the case of adding external devices to your proto boards
> > like beagle etc.
> 
> This patch won't work for that, since it doesn't support adding new
> entries to the mux table, just editing existing entries.

Being able to change the existing mux entries still helps developers to
debug things like pull values needed for i2c device impedance etc.

Regards,

Tony

Re: [PATCH] pinctrl/pinconfig: add debug interface

[Linus Walleij]

On Mon, Feb 11, 2013 at 9:53 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/10/2013 01:11 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.
>
> I never understood why HW engineers can't just recompile the kernel.
> Besides, it's just a device tree change these days - no recompile even
> required, right?

Oh well, they have their habits, like testing a range of different
HW configs after each other, say you have a reference design
with a nailed down mux+config, but you want to loop over a few
possible configurations (not for that hardware, but applicable for
other designs) and make sure they actually work too. The number
of recompiles and reboots soon goes through the roof.

So the HW people hack the kernel a lot to do this kind of things,
and then someone get to maintain their hacks. So this is a step
in the direction of letting them have a generic tool to do the trick.

>> diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
>
> To be worth including, hadn't this feature also better also allow
> editing of pin mux settings too, and even addition/removal of entries,
> so some more core file would be a better place?

I think it'll be orthogonal: one file for the pinconf stuff (something
like in this patch) and then another patch for the pinmux
stuff. I'm trying to keep these two separate.

>> +/* 32bit read/write ressources */
>> +#define MAX_NAME_LEN 16
>> +char dbg_pinname[MAX_NAME_LEN]; /* shared: name of the state of the pin*/
>> +char dbg_state_name[MAX_NAME_LEN]; /* shared: state of the pin*/
>> +static u32 dbg_config; /* shared: config to be read/set for the pin & state*/
>
> Rather than setting pin name, state name, and config separately,
> wouldn't it be better to accept a single write() with all those
> parameters contained in it at once, so no persistent state was required.
> Say something like:
>
> modify configs_pin devicename state pinname newvalue
>
> That would allow "add", "delete" to be implemented in addition to modify
> later if desired.

Hm. Sounds useful.

Laurent what do you say about this?

Could you augment the patch like that?

>> +static int pinconf_dbg_pinname_write(struct file *file,
>> +     const char __user *user_buf, size_t count, loff_t *ppos)
>> +{
>> +     int err;
>> +
>> +     if (count > MAX_NAME_LEN)
>> +             return -EINVAL;
>> +
>> +     err = sscanf(user_buf, "%15s", dbg_pinname);
>
> That %15 had better somehow be related to MAX_NAME_LEN.
>
> I'd suggest making MAX_NAME_LEN 15, and the variable declarations above
> use MAX_NAME_LEN + 1, so that MAX_NAME_LEN can be used as part of the
> scanf parameter here.

OK -> Laurent.

>> +/**
>> + * pinconf_dbg_config_write() - overwrite the pinctrl config in thepinctrl
>> + * map, of a pin/state pair based on pinname and state that have been
>> + * selected with the debugfs entries pinconf-name and pinconf-state
>> + */
>> +static int pinconf_dbg_config_write(struct file *file,
>> +     const char __user *user_buf, size_t count, loff_t *ppos)
>> +{
>> +     int err;
>> +     unsigned long config;
>> +     struct pinctrl_maps *maps_node;
>> +     struct pinctrl_map const *map;
>> +     int i, j;
>> +
>> +     err = kstrtoul_from_user(user_buf, count, 0, &config);
>> +
>> +     if (err)
>> +             return err;
>> +
>> +     dbg_config = config;
>
> That assumes that pin config values are a plain u32. While this is
> commonly true in existing pinctrl drivers, it certainly isn't something
> that the pinctrl core mandates;

So there is this unsigned long and it has a value.

The interface should support altering that unsigned long not
really caring for what it contains. It seems pretty straight-forward
to me.

Of course, if this unsigned long is a pointer, this is just a
fantastic recipe for shooting oneself in the foot, but again
debugfs is just one big gun aimed at ones foot anyway, that's
the whole point of debugfs.

> that's the whole point of
> dt_node_to_map() for example, to allow the individual pinctrl drivers to
> use whatever type (scalar, pointer-to-struct, ...) they want to
> represent their config values.

Yes. But it is an unsigned long, and we support altering it.

And the whole point of debugfs is to do crazy stuff like that.

>> +     mutex_lock(&pinctrl_mutex);
>> +
>> +     /* Parse the pinctrl map and look for the selected pin/state */
>> +     for_each_maps(maps_node, i, map) {
>> +             if (map->type != PIN_MAP_TYPE_CONFIGS_PIN)
>> +                     continue;
>> +
>> +             if (strncmp(map->name, dbg_state_name, MAX_NAME_LEN) > 0)
>> +                     continue;
>
> What about the device name; this changes that state in every device's
> map table entry.

OK.

Laurent, can you augment this to also take the device name into
account? (Shouldn't be a big deal.)

>> +             /*  we found the right pin / state, so overwrite config */
>> +             for (j = 0; j < map->data.configs.num_configs; j++) {
>> +                     if (strncmp(map->data.configs.group_or_pin, dbg_pinname,
>> +                                             MAX_NAME_LEN) == 0)
>
> Why compare this inside the per-config loop;
> map->data.configs.group_or_pin is something "global" to the entire
> struct, and not specific to each configs[] entry.

OK Laurent can you fix this?

>> +                             map->data.configs.configs[j] = dbg_config;
>
> Given that dbg_config is written to by this function, then used by this
> function, why even make it a global? The only other use is
> pinconf_dbg_config_print(), which can also make it a local variable.

OK Laurent can you fix this?

> Overall, I'm not convinced of the utility of this patch upstream. Sorry.

The utility is that it reduces the amount of code needed to be kept
out-of-tree, and it provides a nice gun for shooting oneself in the
foot using debugfs.

Yours,
Linus Walleij

Re: [PATCH] pinctrl/pinconfig: add debug interface

[Haojian Zhuang]

On Tue, Feb 12, 2013 at 8:54 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Feb 11, 2013 at 9:53 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/10/2013 01:11 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.
>>
>> I never understood why HW engineers can't just recompile the kernel.
>> Besides, it's just a device tree change these days - no recompile even
>> required, right?
>
> Oh well, they have their habits, like testing a range of different
> HW configs after each other, say you have a reference design
> with a nailed down mux+config, but you want to loop over a few
> possible configurations (not for that hardware, but applicable for
> other designs) and make sure they actually work too. The number
> of recompiles and reboots soon goes through the roof.
>
> So the HW people hack the kernel a lot to do this kind of things,
> and then someone get to maintain their hacks. So this is a step
> in the direction of letting them have a generic tool to do the trick.
>
>>> diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
>>
>> To be worth including, hadn't this feature also better also allow
>> editing of pin mux settings too, and even addition/removal of entries,
>> so some more core file would be a better place?
>
> I think it'll be orthogonal: one file for the pinconf stuff (something
> like in this patch) and then another patch for the pinmux
> stuff. I'm trying to keep these two separate.
>
>>> +/* 32bit read/write ressources */
>>> +#define MAX_NAME_LEN 16
>>> +char dbg_pinname[MAX_NAME_LEN]; /* shared: name of the state of the pin*/
>>> +char dbg_state_name[MAX_NAME_LEN]; /* shared: state of the pin*/
>>> +static u32 dbg_config; /* shared: config to be read/set for the pin & state*/
>>
>> Rather than setting pin name, state name, and config separately,
>> wouldn't it be better to accept a single write() with all those
>> parameters contained in it at once, so no persistent state was required.
>> Say something like:
>>
>> modify configs_pin devicename state pinname newvalue
>>
>> That would allow "add", "delete" to be implemented in addition to modify
>> later if desired.
>
> Hm. Sounds useful.
>
> Laurent what do you say about this?
>
> Could you augment the patch like that?
>
>>> +static int pinconf_dbg_pinname_write(struct file *file,
>>> +     const char __user *user_buf, size_t count, loff_t *ppos)
>>> +{
>>> +     int err;
>>> +
>>> +     if (count > MAX_NAME_LEN)
>>> +             return -EINVAL;
>>> +
>>> +     err = sscanf(user_buf, "%15s", dbg_pinname);
>>
>> That %15 had better somehow be related to MAX_NAME_LEN.
>>
>> I'd suggest making MAX_NAME_LEN 15, and the variable declarations above
>> use MAX_NAME_LEN + 1, so that MAX_NAME_LEN can be used as part of the
>> scanf parameter here.
>
> OK -> Laurent.
>
>From this patch, we need to input "debug state" & "debug name" first. Then we
could program the new updated config. Do we really use these new nodes & these
steps in debugfs?

"pinconf-pins" can dump all configs of all pins. But it could only
dump the run-time
configs. Could we extend it to support dumping idle configs? Could we append
the write interface on pinconf-pins?

If we dump "pinconf-pins", we can find that all pins are listed in
number sequence.
It seems that we can make use the number directly. "debug name" is unnecessary.

By the way, program pinconf really help software guy to tune power consumption
on pins. Only programming pinconf in idle state may be still limited.
It's better to
program the mux setting in idle state.

Regards
Haojian

Re: [PATCH] pinctrl/pinconfig: add debug interface

[Stephen Warren]

On 02/12/2013 05:54 AM, Linus Walleij wrote:
> On Mon, Feb 11, 2013 at 9:53 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/10/2013 01:11 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.
...
>>> +/**
>>> + * pinconf_dbg_config_write() - overwrite the pinctrl config in thepinctrl
>>> + * map, of a pin/state pair based on pinname and state that have been
>>> + * selected with the debugfs entries pinconf-name and pinconf-state
>>> + */
>>> +static int pinconf_dbg_config_write(struct file *file,
>>> +     const char __user *user_buf, size_t count, loff_t *ppos)
>>> +{
>>> +     int err;
>>> +     unsigned long config;
>>> +     struct pinctrl_maps *maps_node;
>>> +     struct pinctrl_map const *map;
>>> +     int i, j;
>>> +
>>> +     err = kstrtoul_from_user(user_buf, count, 0, &config);
>>> +
>>> +     if (err)
>>> +             return err;
>>> +
>>> +     dbg_config = config;
>>
>> That assumes that pin config values are a plain u32. While this is
>> commonly true in existing pinctrl drivers, it certainly isn't something
>> that the pinctrl core mandates;
> 
> So there is this unsigned long and it has a value.
> 
> The interface should support altering that unsigned long not
> really caring for what it contains. It seems pretty straight-forward
> to me.

But it's not an unsigned long - it's an opaque value that just happens
to be stored in an unsigned long.

> Of course, if this unsigned long is a pointer, this is just a
> fantastic recipe for shooting oneself in the foot, but again
> debugfs is just one big gun aimed at ones foot anyway, that's
> the whole point of debugfs.

But it'd be possible to handle this if the code to modify the map called
into the individual pinctrl driver to convert the data the user wrote
into the value to store in the map, just like it does for DT.

Then, it'd work in all cases.

Plus, it could convert e.g. "pull up" to 0x10001 rather than requiring
the user to manually encode the 0x10001 themselves; much more useful.

>> that's the whole point of
>> dt_node_to_map() for example, to allow the individual pinctrl drivers to
>> use whatever type (scalar, pointer-to-struct, ...) they want to
>> represent their config values.
> 
> Yes. But it is an unsigned long, and we support altering it.
> 
> And the whole point of debugfs is to do crazy stuff like that.

Well to some extent, but there's little point in creating a debugfs file
that makes different assumptions about what its doing than any other
code that uses the same data structures.

Re: [PATCH] pinctrl/pinconfig: add debug interface

[Linus Walleij]

On Tue, Feb 12, 2013 at 5:57 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/12/2013 05:54 AM, Linus Walleij wrote:

>> Of course, if this unsigned long is a pointer, this is just a
>> fantastic recipe for shooting oneself in the foot, but again
>> debugfs is just one big gun aimed at ones foot anyway, that's
>> the whole point of debugfs.
>
> But it'd be possible to handle this if the code to modify the map called
> into the individual pinctrl driver to convert the data the user wrote
> into the value to store in the map, just like it does for DT.
>
> Then, it'd work in all cases.

You are right, this is way more elegant. So Laurent, can you add
some optional function pointer to struct pinconf_ops to translate
the passed parameter, and if that function is not assigned we do
not open the debugfs interface for config at all. This will be
quite elegant.

Something like:

int (*pin_config_group_dbg_set) (struct pinctrl_dev *pctldev,
                                           const char *arg,
                                           unsigned long *config);

> Plus, it could convert e.g. "pull up" to 0x10001 rather than requiring
> the user to manually encode the 0x10001 themselves; much more useful.

You are right.

Reconfiguring muxes from debugfs will be different, I guess
that may be handled entirely by the core. But it's a separate thing.

Yours,
Linus Walleij

[PATCH] pinctrl/pinconfig: add debug interface

[Linus Walleij]

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.

Signed-off-by: Laurent Meunier <laurent.meunier@st.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/pinconf.c       | 231 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pinctrl/pinconf.h |   4 +
 2 files changed, 235 insertions(+)

diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index baee2cc..a95ac8d 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
+#include <linux/uaccess.h>
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinconf.h>
@@ -574,6 +575,234 @@ static const struct file_operations pinconf_groups_ops = {
 	.release	= single_release,
 };
 
+/* 32bit read/write resources */
+#define MAX_NAME_LEN 15
+
+enum pinconf_dbg_request_type {
+	PINCONF_DBG_REQ_TYPE_INVALID,
+	PINCONF_DBG_REQ_TYPE_MODIFY,
+};
+
+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];
+};
+
+static struct dbg_cfg pinconf_dbg_conf;
+
+/**
+ * pinconf_dbg_config_print() - display the pinctrl config from the pinctrl
+ * map, of a pin/state pair based on pinname and state that have been
+ * selected with the debugfs entries pinconf-name and pinconf-state
+ * @s: contains the 32bits config to be written
+ * @d: not used
+ */
+static int pinconf_dbg_config_print(struct seq_file *s, void *d)
+{
+	struct pinctrl_maps *maps_node;
+	struct pinctrl_map const *map;
+	struct pinctrl_dev *pctldev = NULL;
+	struct pinconf_ops const *confops = NULL;
+	int i, j;
+	bool found = false;
+	struct dbg_cfg *dbg = &pinconf_dbg_conf;
+	unsigned long config = 0;
+
+	mutex_lock(&pinctrl_mutex);
+
+	/* Parse the pinctrl map and look for the elected pin/state */
+	for_each_maps(maps_node, i, map) {
+		if (map->type != dbg->map_type)
+			continue;
+		if (strncmp(map->dev_name, dbg->dev_name, MAX_NAME_LEN) != 0)
+			continue;
+		if (strncmp(map->name, dbg->state_name, MAX_NAME_LEN) != 0)
+			continue;
+
+		for (j = 0; j < map->data.configs.num_configs; j++) {
+			if (0 == strncmp(map->data.configs.group_or_pin,
+						dbg->pin_name, MAX_NAME_LEN)) {
+				/*
+				 * We found the right pin / state, read the
+				 * config and store the pctldev
+				 */
+				config = map->data.configs.configs[j];
+				pctldev = get_pinctrl_dev_from_devname
+					(map->ctrl_dev_name);
+				found = true;
+				break;
+			}
+		}
+	}
+
+	mutex_unlock(&pinctrl_mutex);
+
+	if (!found) {
+		seq_printf(s, "No config found for dev/state/pin, expected:\n");
+		seq_printf(s, "modify config_pins devname " \
+			   "state pinname value\n");
+	}
+
+	seq_printf(s, "Dev %s has config of %s in state %s: 0x%08lX\n",
+		   dbg->dev_name, dbg->pin_name,
+		   dbg->state_name, config);
+
+	if (pctldev)
+		confops = pctldev->desc->confops;
+
+	if (confops && confops->pin_config_config_dbg_show)
+		confops->pin_config_config_dbg_show(pctldev,
+						    s, config);
+	return 0;
+}
+
+/**
+ * pinconf_dbg_config_write() - overwrite the pinctrl config in the pinctrl
+ * map, of a pin/state pair based on pinname and state that have been
+ * selected with the debugfs entries pinconf-name and pinconf-state
+ * @user_buf: contains 'modify configs_pin devicename state pinname newvalue'
+ */
+static int pinconf_dbg_config_write(struct file *file,
+	const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	struct pinctrl_maps *maps_node;
+	struct pinctrl_map const *map;
+	struct pinctrl_dev *pctldev = NULL;
+	struct pinconf_ops const *confops = NULL;
+	struct dbg_cfg *dbg = &pinconf_dbg_conf;
+	bool found = false;
+	char buf[128];
+	char *b = &buf[0];
+	int buf_size;
+	char *token;
+	struct pinctrl_map_configs const *configs;
+	int i;
+
+	/* Get userspace string and assure termination */
+	buf_size = min(count, (sizeof(buf)-1));
+	if (copy_from_user(buf, user_buf, buf_size))
+		return -EFAULT;
+
+	buf[buf_size] = 0;
+
+	/*
+	 * need to parse entry and extract parameters:
+	 * modify configs_pin devicename state pinname newvalue
+	 */
+
+	/* Get arg: 'modify' */
+	if (!strncmp(b, "modify ", strlen("modify "))) {
+		dbg->req_type = PINCONF_DBG_REQ_TYPE_MODIFY;
+		b += strlen("modify ");
+	} else {
+		return -EINVAL;
+	}
+
+	/* Get arg type: "config_pin" type supported so far */
+	if (!strncmp(b, "config_pins ", strlen("config_pins "))) {
+		dbg->map_type = PIN_MAP_TYPE_CONFIGS_PIN;
+		b += strlen("config_pins ");
+	} else {
+		return -EINVAL;
+	}
+
+	/* get arg 'device_name' */
+	token = strsep(&b, " ");
+	if (token == NULL)
+		return -EINVAL;
+	if (strlen(token) < MAX_NAME_LEN)
+		sscanf(token, "%s", dbg->dev_name);
+	else
+		return -EINVAL;
+
+	/* get arg 'state_name' */
+	token = strsep(&b, " ");
+	if (token == NULL)
+		return -EINVAL;
+	if (strlen(token) < MAX_NAME_LEN)
+		sscanf(token, "%s", dbg->state_name);
+	else
+		return -EINVAL;
+
+	/* get arg 'pin_name' */
+	token = strsep(&b, " ");
+	if (token == NULL)
+		return -EINVAL;
+	if (strlen(token) < MAX_NAME_LEN)
+		sscanf(token, "%s", dbg->pin_name);
+	else
+		return -EINVAL;
+
+	/* get new_value of config' */
+	token = strsep(&b, " ");
+	if (token == NULL)
+		return -EINVAL;
+
+	if (strlen(token) < MAX_NAME_LEN)
+		sscanf(token, "%s", dbg->config);
+	else
+		return -EINVAL;
+
+	mutex_lock(&pinctrl_mutex);
+
+	/* Parse the pinctrl map and look for the selected dev/state/pin */
+	for_each_maps(maps_node, i, map) {
+		if (strncmp(map->dev_name, dbg->dev_name, MAX_NAME_LEN) != 0)
+			continue;
+		if (map->type != dbg->map_type)
+			continue;
+		if (strncmp(map->name, dbg->state_name, MAX_NAME_LEN) != 0)
+			continue;
+
+		/*  we found the right pin / state, so overwrite config */
+		if (strncmp(map->data.configs.group_or_pin,
+			dbg->pin_name, MAX_NAME_LEN) == 0) {
+
+			found = true;
+			pctldev = get_pinctrl_dev_from_devname(
+					map->ctrl_dev_name);
+			configs = &map->data.configs;
+			break;
+		}
+	}
+
+	mutex_unlock(&pinctrl_mutex);
+
+	if (!found)
+		return -EINVAL;
+
+	if (pctldev)
+		confops = pctldev->desc->confops;
+
+	if (confops && confops->pin_config_dbg_set) {
+		for (i = 0; i < configs->num_configs; i++) {
+			confops->pin_config_dbg_set(pctldev,
+						    dbg->config,
+						    configs->configs[i]);
+		}
+	}
+
+	return count;
+}
+
+static int pinconf_dbg_config_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pinconf_dbg_config_print, inode->i_private);
+}
+
+static const struct file_operations pinconf_dbg_pinconfig_fops = {
+	.open = pinconf_dbg_config_open,
+	.write = pinconf_dbg_config_write,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.owner = THIS_MODULE,
+};
+
 void pinconf_init_device_debugfs(struct dentry *devroot,
 			 struct pinctrl_dev *pctldev)
 {
@@ -581,6 +810,8 @@ void pinconf_init_device_debugfs(struct dentry *devroot,
 			    devroot, pctldev, &pinconf_pins_ops);
 	debugfs_create_file("pinconf-groups", S_IFREG | S_IRUGO,
 			    devroot, pctldev, &pinconf_groups_ops);
+	debugfs_create_file("pinconf-config",  (S_IRUGO | S_IWUSR | S_IWGRP),
+			    devroot, pctldev, &pinconf_dbg_pinconfig_fops);
 }
 
 #endif
diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h
index e7a7201..2cc8d2c 100644
--- a/include/linux/pinctrl/pinconf.h
+++ b/include/linux/pinctrl/pinconf.h
@@ -28,6 +28,7 @@ struct seq_file;
  * @pin_config_set: configure an individual pin
  * @pin_config_group_get: get configurations for an entire pin group
  * @pin_config_group_set: configure all pins in a group
+ * @pin_config_group_dbg_set: optional debugfs to modify a pin configuration
  * @pin_config_dbg_show: optional debugfs display hook that will provide
  *	per-device info for a certain pin in debugfs
  * @pin_config_group_dbg_show: optional debugfs display hook that will provide
@@ -51,6 +52,9 @@ struct pinconf_ops {
 	int (*pin_config_group_set) (struct pinctrl_dev *pctldev,
 				     unsigned selector,
 				     unsigned long config);
+	int (*pin_config_dbg_set) (struct pinctrl_dev *pctldev,
+				   const char *arg,
+				   unsigned long config);
 	void (*pin_config_dbg_show) (struct pinctrl_dev *pctldev,
 				     struct seq_file *s,
 				     unsigned offset);
-- 
1.7.11.3