Re: [PATCH] pinctrl: remove unused 'pinconf-config' debugfs interface
Hi Linus, On 01/28/2019 02:36 PM, Linus Walleij wrote: > On Sun, Jan 20, 2019 at 4:14 PM Vladimir Zapolskiy 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 'e606.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 >> Cc: Laurent Meunier >> Cc: Masahiro Yamada >> Cc: Russell King > > This is fine, the build robot is complaining of some missing > prototype though, probably some implicit include that > disappeared when you removed > from the 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
Re: [PATCH] pinctrl: remove unused 'pinconf-config' debugfs interface
On Sun, Jan 20, 2019 at 4:14 PM Vladimir Zapolskiy 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 'e606.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 > Cc: Laurent Meunier > Cc: Masahiro Yamada > Cc: Russell King This is fine, the build robot is complaining of some missing prototype though, probably some implicit include that disappeared when you removed from the file. Will you send a v2 with this fixed? (I think just leaving the include in place would be a quickfix.) Yours, Linus Walleij
Re: [PATCH] pinctrl: remove unused 'pinconf-config' debugfs interface
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 26struct pinctrl_map **map, unsigned *reserved_maps, 1eb207a9e Laxman Dewangan 2013-08-06 27unsigned *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 29struct pinctrl_map **map, unsigned *reserved_maps, 1eb207a9e Laxman Dewangan 2013-08-06 30unsigned *num_maps, const char *group, 1eb207a9e Laxman Dewangan 2013-08-06 31const 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 33struct pinctrl_map **map, unsigned *reserved_maps, 1eb207a9e Laxman Dewangan 2013-08-06 34unsigned *num_maps, const char *group, 1eb207a9e Laxman Dewangan 2013-08-06 35unsigned long *configs, unsigned num_configs, 1eb207a9e Laxman Dewangan 2013-08-06 @36enum 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 38unsigned long **configs, unsigned *num_configs, 1eb207a9e Laxman Dewangan 2013-08-06 39unsigned long config); d32f7fd3b Irina Tirdea2016-03-31 40 void pinctrl_utils_free_map(struct pinctrl_dev *pctldev, 1eb207a9e Laxman Dewangan 2013-08-06 41struct 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 :: CC: Linus Walleij --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH] pinctrl: remove unused 'pinconf-config' debugfs interface
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 26struct pinctrl_map **map, unsigned *reserved_maps, 1eb207a9e Laxman Dewangan 2013-08-06 27unsigned *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 29struct pinctrl_map **map, unsigned *reserved_maps, 1eb207a9e Laxman Dewangan 2013-08-06 30unsigned *num_maps, const char *group, 1eb207a9e Laxman Dewangan 2013-08-06 31const 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 33struct pinctrl_map **map, unsigned *reserved_maps, 1eb207a9e Laxman Dewangan 2013-08-06 34unsigned *num_maps, const char *group, 1eb207a9e Laxman Dewangan 2013-08-06 35unsigned long *configs, unsigned num_configs, 1eb207a9e Laxman Dewangan 2013-08-06 @36enum 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 38unsigned long **configs, unsigned *num_configs, 1eb207a9e Laxman Dewangan 2013-08-06 39unsigned long config); d32f7fd3b Irina Tirdea2016-03-31 40 void pinctrl_utils_free_map(struct pinctrl_dev *pctldev, 1eb207a9e Laxman Dewangan 2013-08-06 41struct 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 :: CC: Linus Walleij --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip