The main goal of the change is to remove .pin_config_dbg_parse_modify
callback before a driver with its support appears. So far the in-kernel
interface did not attract any users since its introduction 5 years ago.
Originally .pin_config_dbg_parse_modify callback and the associated
'pinconf-config' debugfs file were introduced in commit f07512e615dd
("pinctrl/pinconfig: add debug interface"), a short description of
'pinconf-config' usage for debugging can be expressed this way:
Write to 'pinconf-config' (see pinconf_dbg_config_write() function):
% echo -n modify $map_type $device_name $state_name $pin_name $config > \
/sys/kernel/debug/pinctrl/$pinctrl/pinconf-config
It supposes to update a global (therefore single!) 'pinconf_dbg_conf'
variable with an alternative setting, the arguments should match
an existing pinconf device and some registered pinctrl mapping 'map':
* $map_type is either 'config_pin' or 'config_group', it should match
'map->type' value of PIN_MAP_TYPE_CONFIGS_PIN or
PIN_MAP_TYPE_CONFIGS_GROUP accordingly,
* $device_name should match 'map->dev_name' string value,
* $state_name should match 'map->name' string value,
* $pin_name should match 'map->data.configs.group_or_pin' string value,
If all above has matched, then $config is a new value to be set by calling
pinconfops->pin_config_dbg_parse_modify(pctldev, config, matched_config).
After a successful write into 'pinconf-config' a user can read the file
to get information about that single modified pin configuration.
The fact is .pin_config_dbg_parse_modify callback has never been defined
in 'struct pinconf_ops' of any pinconf driver, thus an actual modification
of a pin or group state on any present pinconf controller does not happen,
and it declares that all related code is no more than dead code.
I discovered the issue while attempting to add .pin_config_dbg_parse_modify
support in some drivers and found that too short 'MAX_NAME_LEN' set by
drivers/pinctrl/pinconf.c:372:#define MAX_NAME_LEN 15
is practically insufficient to store a regular pinctrl device name,
which are like 'e6060000.pin-controller-sh-pfc' or pin names like
'MX6QDL_PAD_ENET_REF_CLK', thus it is another indicator that the code
is barely usable, insufficiently tested and unprepossessing.
Of course it might be possible to increase MAX_NAME_LEN, and then add
.pin_config_dbg_parse_modify callbacks to the drivers, but the whole
idea of such a limited debug option looks inviable. A more flexible
way to functionally substitute the original approach is to implicitly
or explicitly use pinctrl_select_state() function whenever needed.
Signed-off-by: Vladimir Zapolskiy <[email protected]>
Cc: Laurent Meunier <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Russell King <[email protected]>
---
The proposal is to remove the changes added by commit f07512e615dd
("pinctrl/pinconfig: add debug interface") for now, as described in
the commit message it may affect only non-vanilla kernels formally.
Note that still it is a debugfs UAPI change, however, since
'pinconf-config' file is plainly unusable on vanilla, I won't expect
any user reports or asks to restore it back, this my assumption
nudges me not to add an RFC tag to the published change.
drivers/pinctrl/pinconf.c | 222 --------------------------------
include/linux/pinctrl/pinconf.h | 6 -
2 files changed, 228 deletions(-)
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 2c7229380f08..2678603df14b 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -17,7 +17,6 @@
#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>
@@ -369,225 +368,6 @@ static int pinconf_groups_show(struct seq_file *s, void *what)
DEFINE_SHOW_ATTRIBUTE(pinconf_pins);
DEFINE_SHOW_ATTRIBUTE(pinconf_groups);
-#define MAX_NAME_LEN 15
-
-struct dbg_cfg {
- 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];
-};
-
-/*
- * Goal is to keep this structure as global in order to simply read the
- * pinconf-config file after a write to check config is as expected
- */
-static struct dbg_cfg pinconf_dbg_conf;
-
-/**
- * pinconf_dbg_config_print() - display the pinctrl config from the pinctrl
- * map, of the dev/pin/state that was last written to pinconf-config file.
- * @s: string filled in with config description
- * @d: not used
- */
-static int pinconf_dbg_config_print(struct seq_file *s, void *d)
-{
- struct pinctrl_maps *maps_node;
- const struct pinctrl_map *map;
- const struct pinctrl_map *found = NULL;
- struct pinctrl_dev *pctldev;
- struct dbg_cfg *dbg = &pinconf_dbg_conf;
- int i;
-
- mutex_lock(&pinctrl_maps_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 (strcmp(map->dev_name, dbg->dev_name))
- continue;
- if (strcmp(map->name, dbg->state_name))
- continue;
-
- if (!strcmp(map->data.configs.group_or_pin, dbg->pin_name)) {
- /* We found the right pin */
- found = map;
- break;
- }
- }
-
- if (!found) {
- seq_printf(s, "No config found for dev/state/pin, expected:\n");
- seq_printf(s, "Searched dev:%s\n", dbg->dev_name);
- seq_printf(s, "Searched state:%s\n", dbg->state_name);
- seq_printf(s, "Searched pin:%s\n", dbg->pin_name);
- seq_printf(s, "Use: modify config_pin <devname> "\
- "<state> <pinname> <value>\n");
- goto exit;
- }
-
- pctldev = get_pinctrl_dev_from_devname(found->ctrl_dev_name);
- seq_printf(s, "Dev %s has config of %s in state %s:\n",
- dbg->dev_name, dbg->pin_name, dbg->state_name);
- pinconf_show_config(s, pctldev, found->data.configs.configs,
- found->data.configs.num_configs);
-
-exit:
- mutex_unlock(&pinctrl_maps_mutex);
-
- return 0;
-}
-
-/**
- * pinconf_dbg_config_write() - modify the pinctrl config in the pinctrl
- * map, of a dev/pin/state entry based on user entries to pinconf-config
- * @user_buf: contains the modification request with expected format:
- * modify <config> <devicename> <state> <name> <newvalue>
- * modify is literal string, alternatives like add/delete not supported yet
- * <config> is the configuration to be changed. Supported configs are
- * "config_pin" or "config_group", alternatives like config_mux are not
- * supported yet.
- * <devicename> <state> <name> are values that should match the pinctrl-maps
- * <newvalue> reflects the new config and is driver dependent
- */
-static ssize_t pinconf_dbg_config_write(struct file *file,
- const char __user *user_buf, size_t count, loff_t *ppos)
-{
- struct pinctrl_maps *maps_node;
- const struct pinctrl_map *map;
- const struct pinctrl_map *found = NULL;
- struct pinctrl_dev *pctldev;
- const struct pinconf_ops *confops = NULL;
- struct dbg_cfg *dbg = &pinconf_dbg_conf;
- const struct pinctrl_map_configs *configs;
- char config[MAX_NAME_LEN + 1];
- char buf[128];
- char *b = &buf[0];
- int buf_size;
- char *token;
- 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' */
- token = strsep(&b, " ");
- if (!token)
- return -EINVAL;
- if (strcmp(token, "modify"))
- return -EINVAL;
-
- /*
- * Get arg type: "config_pin" and "config_group"
- * types are supported so far
- */
- token = strsep(&b, " ");
- if (!token)
- return -EINVAL;
- if (!strcmp(token, "config_pin"))
- dbg->map_type = PIN_MAP_TYPE_CONFIGS_PIN;
- else if (!strcmp(token, "config_group"))
- dbg->map_type = PIN_MAP_TYPE_CONFIGS_GROUP;
- else
- return -EINVAL;
-
- /* get arg 'device_name' */
- token = strsep(&b, " ");
- if (!token)
- return -EINVAL;
- if (strlen(token) >= MAX_NAME_LEN)
- return -EINVAL;
- strncpy(dbg->dev_name, token, MAX_NAME_LEN);
-
- /* get arg 'state_name' */
- token = strsep(&b, " ");
- if (!token)
- return -EINVAL;
- if (strlen(token) >= MAX_NAME_LEN)
- return -EINVAL;
- strncpy(dbg->state_name, token, MAX_NAME_LEN);
-
- /* get arg 'pin_name' */
- token = strsep(&b, " ");
- if (!token)
- return -EINVAL;
- if (strlen(token) >= MAX_NAME_LEN)
- return -EINVAL;
- strncpy(dbg->pin_name, token, MAX_NAME_LEN);
-
- /* get new_value of config' */
- token = strsep(&b, " ");
- if (!token)
- return -EINVAL;
- if (strlen(token) >= MAX_NAME_LEN)
- return -EINVAL;
- strncpy(config, token, MAX_NAME_LEN);
-
- mutex_lock(&pinctrl_maps_mutex);
-
- /* Parse the pinctrl map and look for the selected dev/state/pin */
- for_each_maps(maps_node, i, map) {
- if (strcmp(map->dev_name, dbg->dev_name))
- continue;
- if (map->type != dbg->map_type)
- continue;
- if (strcmp(map->name, dbg->state_name))
- continue;
-
- /* we found the right pin / state, so overwrite config */
- if (!strcmp(map->data.configs.group_or_pin, dbg->pin_name)) {
- found = map;
- break;
- }
- }
-
- if (!found) {
- count = -EINVAL;
- goto exit;
- }
-
- pctldev = get_pinctrl_dev_from_devname(found->ctrl_dev_name);
- if (pctldev)
- confops = pctldev->desc->confops;
-
- if (confops && confops->pin_config_dbg_parse_modify) {
- configs = &found->data.configs;
- for (i = 0; i < configs->num_configs; i++) {
- confops->pin_config_dbg_parse_modify(pctldev,
- config,
- &configs->configs[i]);
- }
- }
-
-exit:
- mutex_unlock(&pinctrl_maps_mutex);
-
- 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)
{
@@ -595,8 +375,6 @@ void pinconf_init_device_debugfs(struct dentry *devroot,
devroot, pctldev, &pinconf_pins_fops);
debugfs_create_file("pinconf-groups", S_IFREG | S_IRUGO,
devroot, pctldev, &pinconf_groups_fops);
- 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 8dd85d302b90..93c9dd133e9d 100644
--- a/include/linux/pinctrl/pinconf.h
+++ b/include/linux/pinctrl/pinconf.h
@@ -14,8 +14,6 @@
#ifdef CONFIG_PINCONF
-#include <linux/pinctrl/machine.h>
-
struct pinctrl_dev;
struct seq_file;
@@ -31,7 +29,6 @@ struct seq_file;
* @pin_config_group_get: get configurations for an entire pin group; should
* return -ENOTSUPP and -EINVAL using the same rules as pin_config_get.
* @pin_config_group_set: configure all pins in a group
- * @pin_config_dbg_parse_modify: 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
@@ -57,9 +54,6 @@ struct pinconf_ops {
unsigned selector,
unsigned long *configs,
unsigned num_configs);
- int (*pin_config_dbg_parse_modify) (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);
--
2.18.0
Hi Vladimir,
I love your patch! Yet something to improve:
[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v5.0-rc2 next-20190116]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Vladimir-Zapolskiy/pinctrl-remove-unused-pinconf-config-debugfs-interface/20190122-044655
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: i386-randconfig-i1-201903 (attached as .config)
compiler: gcc-8 (Debian 8.2.0-14) 8.2.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
In file included from drivers/pinctrl/cirrus/pinctrl-madera-core.c:25:
>> drivers/pinctrl/cirrus/../pinctrl-utils.h:36:8: warning: 'enum pinctrl_map_type' declared inside parameter list will not be visible outside of this definition or declaration
enum pinctrl_map_type type);
^~~~~~~~~~~~~~~~
drivers/pinctrl/cirrus/pinctrl-madera-core.c: In function 'madera_pin_probe':
>> drivers/pinctrl/cirrus/pinctrl-madera-core.c:1044:9: error: implicit declaration of function 'pinctrl_register_mappings'; did you mean 'pinctrl_register_and_init'? [-Werror=implicit-function-declaration]
ret = pinctrl_register_mappings(pdata->gpio_configs,
^~~~~~~~~~~~~~~~~~~~~~~~~
pinctrl_register_and_init
cc1: some warnings being treated as errors
vim +36 drivers/pinctrl/cirrus/../pinctrl-utils.h
1eb207a9e Laxman Dewangan 2013-08-06 24
1eb207a9e Laxman Dewangan 2013-08-06 25 int pinctrl_utils_reserve_map(struct pinctrl_dev *pctldev,
1eb207a9e Laxman Dewangan 2013-08-06 26 struct pinctrl_map **map, unsigned *reserved_maps,
1eb207a9e Laxman Dewangan 2013-08-06 27 unsigned *num_maps, unsigned reserve);
1eb207a9e Laxman Dewangan 2013-08-06 28 int pinctrl_utils_add_map_mux(struct pinctrl_dev *pctldev,
1eb207a9e Laxman Dewangan 2013-08-06 29 struct pinctrl_map **map, unsigned *reserved_maps,
1eb207a9e Laxman Dewangan 2013-08-06 30 unsigned *num_maps, const char *group,
1eb207a9e Laxman Dewangan 2013-08-06 31 const char *function);
1eb207a9e Laxman Dewangan 2013-08-06 32 int pinctrl_utils_add_map_configs(struct pinctrl_dev *pctldev,
1eb207a9e Laxman Dewangan 2013-08-06 33 struct pinctrl_map **map, unsigned *reserved_maps,
1eb207a9e Laxman Dewangan 2013-08-06 34 unsigned *num_maps, const char *group,
1eb207a9e Laxman Dewangan 2013-08-06 35 unsigned long *configs, unsigned num_configs,
1eb207a9e Laxman Dewangan 2013-08-06 @36 enum pinctrl_map_type type);
1eb207a9e Laxman Dewangan 2013-08-06 37 int pinctrl_utils_add_config(struct pinctrl_dev *pctldev,
1eb207a9e Laxman Dewangan 2013-08-06 38 unsigned long **configs, unsigned *num_configs,
1eb207a9e Laxman Dewangan 2013-08-06 39 unsigned long config);
d32f7fd3b Irina Tirdea 2016-03-31 40 void pinctrl_utils_free_map(struct pinctrl_dev *pctldev,
1eb207a9e Laxman Dewangan 2013-08-06 41 struct pinctrl_map *map, unsigned num_maps);
1eb207a9e Laxman Dewangan 2013-08-06 42
:::::: The code at line 36 was first introduced by commit
:::::: 1eb207a9ecaafb980704d8bc055a9a0269f62f8e pinctrl: add utility functions for add map/configs
:::::: TO: Laxman Dewangan <[email protected]>
:::::: CC: Linus Walleij <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Vladimir,
I love your patch! Yet something to improve:
[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v5.0-rc2 next-20190116]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Vladimir-Zapolskiy/pinctrl-remove-unused-pinconf-config-debugfs-interface/20190122-044655
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: i386-randconfig-x0-01221345 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
In file included from drivers/pinctrl/cirrus/pinctrl-madera-core.c:25:0:
>> drivers/pinctrl/cirrus/../pinctrl-utils.h:36:8: warning: 'enum pinctrl_map_type' declared inside parameter list
enum pinctrl_map_type type);
^
>> drivers/pinctrl/cirrus/../pinctrl-utils.h:36:8: warning: its scope is only this definition or declaration, which is probably not what you want
drivers/pinctrl/cirrus/pinctrl-madera-core.c: In function 'madera_pin_probe':
>> drivers/pinctrl/cirrus/pinctrl-madera-core.c:1044:9: error: implicit declaration of function 'pinctrl_register_mappings' [-Werror=implicit-function-declaration]
ret = pinctrl_register_mappings(pdata->gpio_configs,
^
cc1: some warnings being treated as errors
vim +36 drivers/pinctrl/cirrus/../pinctrl-utils.h
1eb207a9e Laxman Dewangan 2013-08-06 24
1eb207a9e Laxman Dewangan 2013-08-06 25 int pinctrl_utils_reserve_map(struct pinctrl_dev *pctldev,
1eb207a9e Laxman Dewangan 2013-08-06 26 struct pinctrl_map **map, unsigned *reserved_maps,
1eb207a9e Laxman Dewangan 2013-08-06 27 unsigned *num_maps, unsigned reserve);
1eb207a9e Laxman Dewangan 2013-08-06 28 int pinctrl_utils_add_map_mux(struct pinctrl_dev *pctldev,
1eb207a9e Laxman Dewangan 2013-08-06 29 struct pinctrl_map **map, unsigned *reserved_maps,
1eb207a9e Laxman Dewangan 2013-08-06 30 unsigned *num_maps, const char *group,
1eb207a9e Laxman Dewangan 2013-08-06 31 const char *function);
1eb207a9e Laxman Dewangan 2013-08-06 32 int pinctrl_utils_add_map_configs(struct pinctrl_dev *pctldev,
1eb207a9e Laxman Dewangan 2013-08-06 33 struct pinctrl_map **map, unsigned *reserved_maps,
1eb207a9e Laxman Dewangan 2013-08-06 34 unsigned *num_maps, const char *group,
1eb207a9e Laxman Dewangan 2013-08-06 35 unsigned long *configs, unsigned num_configs,
1eb207a9e Laxman Dewangan 2013-08-06 @36 enum pinctrl_map_type type);
1eb207a9e Laxman Dewangan 2013-08-06 37 int pinctrl_utils_add_config(struct pinctrl_dev *pctldev,
1eb207a9e Laxman Dewangan 2013-08-06 38 unsigned long **configs, unsigned *num_configs,
1eb207a9e Laxman Dewangan 2013-08-06 39 unsigned long config);
d32f7fd3b Irina Tirdea 2016-03-31 40 void pinctrl_utils_free_map(struct pinctrl_dev *pctldev,
1eb207a9e Laxman Dewangan 2013-08-06 41 struct pinctrl_map *map, unsigned num_maps);
1eb207a9e Laxman Dewangan 2013-08-06 42
:::::: The code at line 36 was first introduced by commit
:::::: 1eb207a9ecaafb980704d8bc055a9a0269f62f8e pinctrl: add utility functions for add map/configs
:::::: TO: Laxman Dewangan <[email protected]>
:::::: CC: Linus Walleij <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sun, Jan 20, 2019 at 4:14 PM Vladimir Zapolskiy <[email protected]> wrote:
> The main goal of the change is to remove .pin_config_dbg_parse_modify
> callback before a driver with its support appears. So far the in-kernel
> interface did not attract any users since its introduction 5 years ago.
>
> Originally .pin_config_dbg_parse_modify callback and the associated
> 'pinconf-config' debugfs file were introduced in commit f07512e615dd
> ("pinctrl/pinconfig: add debug interface"), a short description of
> 'pinconf-config' usage for debugging can be expressed this way:
>
> Write to 'pinconf-config' (see pinconf_dbg_config_write() function):
>
> % echo -n modify $map_type $device_name $state_name $pin_name $config > \
> /sys/kernel/debug/pinctrl/$pinctrl/pinconf-config
>
> It supposes to update a global (therefore single!) 'pinconf_dbg_conf'
> variable with an alternative setting, the arguments should match
> an existing pinconf device and some registered pinctrl mapping 'map':
>
> * $map_type is either 'config_pin' or 'config_group', it should match
> 'map->type' value of PIN_MAP_TYPE_CONFIGS_PIN or
> PIN_MAP_TYPE_CONFIGS_GROUP accordingly,
> * $device_name should match 'map->dev_name' string value,
> * $state_name should match 'map->name' string value,
> * $pin_name should match 'map->data.configs.group_or_pin' string value,
>
> If all above has matched, then $config is a new value to be set by calling
> pinconfops->pin_config_dbg_parse_modify(pctldev, config, matched_config).
>
> After a successful write into 'pinconf-config' a user can read the file
> to get information about that single modified pin configuration.
>
> The fact is .pin_config_dbg_parse_modify callback has never been defined
> in 'struct pinconf_ops' of any pinconf driver, thus an actual modification
> of a pin or group state on any present pinconf controller does not happen,
> and it declares that all related code is no more than dead code.
>
> I discovered the issue while attempting to add .pin_config_dbg_parse_modify
> support in some drivers and found that too short 'MAX_NAME_LEN' set by
>
> drivers/pinctrl/pinconf.c:372:#define MAX_NAME_LEN 15
>
> is practically insufficient to store a regular pinctrl device name,
> which are like 'e6060000.pin-controller-sh-pfc' or pin names like
> 'MX6QDL_PAD_ENET_REF_CLK', thus it is another indicator that the code
> is barely usable, insufficiently tested and unprepossessing.
>
> Of course it might be possible to increase MAX_NAME_LEN, and then add
> .pin_config_dbg_parse_modify callbacks to the drivers, but the whole
> idea of such a limited debug option looks inviable. A more flexible
> way to functionally substitute the original approach is to implicitly
> or explicitly use pinctrl_select_state() function whenever needed.
>
> Signed-off-by: Vladimir Zapolskiy <[email protected]>
> Cc: Laurent Meunier <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Russell King <[email protected]>
This is fine, the build robot is complaining of some missing
prototype though, probably some implicit include that
disappeared when you removed <linux/pinctrl/machine.h>
from the <linux/pinctrl/pinconf.h> file.
Will you send a v2 with this fixed? (I think just leaving the
include in place would be a quickfix.)
Yours,
Linus Walleij
Hi Linus,
On 01/28/2019 02:36 PM, Linus Walleij wrote:
> On Sun, Jan 20, 2019 at 4:14 PM Vladimir Zapolskiy <[email protected]> wrote:
>
>> The main goal of the change is to remove .pin_config_dbg_parse_modify
>> callback before a driver with its support appears. So far the in-kernel
>> interface did not attract any users since its introduction 5 years ago.
>>
>> Originally .pin_config_dbg_parse_modify callback and the associated
>> 'pinconf-config' debugfs file were introduced in commit f07512e615dd
>> ("pinctrl/pinconfig: add debug interface"), a short description of
>> 'pinconf-config' usage for debugging can be expressed this way:
>>
>> Write to 'pinconf-config' (see pinconf_dbg_config_write() function):
>>
>> % echo -n modify $map_type $device_name $state_name $pin_name $config > \
>> /sys/kernel/debug/pinctrl/$pinctrl/pinconf-config
>>
>> It supposes to update a global (therefore single!) 'pinconf_dbg_conf'
>> variable with an alternative setting, the arguments should match
>> an existing pinconf device and some registered pinctrl mapping 'map':
>>
>> * $map_type is either 'config_pin' or 'config_group', it should match
>> 'map->type' value of PIN_MAP_TYPE_CONFIGS_PIN or
>> PIN_MAP_TYPE_CONFIGS_GROUP accordingly,
>> * $device_name should match 'map->dev_name' string value,
>> * $state_name should match 'map->name' string value,
>> * $pin_name should match 'map->data.configs.group_or_pin' string value,
>>
>> If all above has matched, then $config is a new value to be set by calling
>> pinconfops->pin_config_dbg_parse_modify(pctldev, config, matched_config).
>>
>> After a successful write into 'pinconf-config' a user can read the file
>> to get information about that single modified pin configuration.
>>
>> The fact is .pin_config_dbg_parse_modify callback has never been defined
>> in 'struct pinconf_ops' of any pinconf driver, thus an actual modification
>> of a pin or group state on any present pinconf controller does not happen,
>> and it declares that all related code is no more than dead code.
>>
>> I discovered the issue while attempting to add .pin_config_dbg_parse_modify
>> support in some drivers and found that too short 'MAX_NAME_LEN' set by
>>
>> drivers/pinctrl/pinconf.c:372:#define MAX_NAME_LEN 15
>>
>> is practically insufficient to store a regular pinctrl device name,
>> which are like 'e6060000.pin-controller-sh-pfc' or pin names like
>> 'MX6QDL_PAD_ENET_REF_CLK', thus it is another indicator that the code
>> is barely usable, insufficiently tested and unprepossessing.
>>
>> Of course it might be possible to increase MAX_NAME_LEN, and then add
>> .pin_config_dbg_parse_modify callbacks to the drivers, but the whole
>> idea of such a limited debug option looks inviable. A more flexible
>> way to functionally substitute the original approach is to implicitly
>> or explicitly use pinctrl_select_state() function whenever needed.
>>
>> Signed-off-by: Vladimir Zapolskiy <[email protected]>
>> Cc: Laurent Meunier <[email protected]>
>> Cc: Masahiro Yamada <[email protected]>
>> Cc: Russell King <[email protected]>
>
> This is fine, the build robot is complaining of some missing
> prototype though, probably some implicit include that
> disappeared when you removed <linux/pinctrl/machine.h>
> from the <linux/pinctrl/pinconf.h> file.
>
> Will you send a v2 with this fixed? (I think just leaving the
> include in place would be a quickfix.)
>
Fortunately it has been already done, please check the sent v2 with one
earned Reviewed-by tag from Richard:
* https://patchwork.ozlabs.org/patch/1029536/
* https://patchwork.ozlabs.org/patch/1029537/
Thank you for your approval of the change in general!
--
Best wishes,
Vladimir