| Message ID | 1365018447-28062-1-git-send-email-linus.walleij@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > +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. > +/** > + * 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 > + */ This comment seems stale; I don't believe there are any pinconf-name and pinconf-state files; aren't they part of the data written to the one debugfs file each time? > +static int pinconf_dbg_config_print(struct seq_file *s, void *d) > + if (strncmp(map->dev_name, dbg->dev_name, MAX_NAME_LEN) != 0) Wouldn't it be simpler to always ensure dbg->dev_name was NULL-terminated, and just use !strcmp() here? Same for dbg->state_name below. Same comment for the other function below. > + mutex_unlock(&pinctrl_mutex); Don't you need the lock held to call confops->xxx() below, to make sure that the driver isn't unregistered between finding it, and calling the confops function? > + 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"); > + } Hmmm. At least including the parsed values that were being searched for might be useful? > + confops->pin_config_config_dbg_show(pctldev, > + s, config); I think that function name is wrong; two "config_" in a row. Does this compile? > +/** > + * 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 Similar stale comment. > + * @user_buf: contains 'modify configs_pin devicename state pinname newvalue' It might be useful to say which parts of that are literal strings and which are variables/values to be changed? > +static int pinconf_dbg_config_write(struct file *file, > + const char __user *user_buf, size_t count, loff_t *ppos) > + /* Get arg: 'modify' */ > + if (!strncmp(b, "modify ", strlen("modify "))) { > + dbg->req_type = PINCONF_DBG_REQ_TYPE_MODIFY; > + b += strlen("modify "); > + } else { > + return -EINVAL; > + } There's rather a lot of duplication of strings and strlen calls there. Why not: a) Start using strsep() for the very first fields too, so everything is consistent. b) Put the error-path first. In other words: token = strsep(&b, " "); if (!token) return -EINVAL; if (!strcmp(token, "modify")) 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; > + } That could be transformed the same way. > + /* 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; It might make sense to put the error-handling first, and why sscanf not strcpy? So: token = strsep(&b, " "); if (token == NULL) return -EINVAL; if (strlen(token) >= MAX_NAME_LEN) return -EINVAL; strcpy(dbg->dev_name, token); (or even just use strncpy followed by explicit NULL termination and truncate text that's too long, or kstrdup() it to avoid limits?) > + if (confops && confops->pin_config_dbg_parse) { > + for (i = 0; i < configs->num_configs; i++) { > + confops->pin_config_dbg_parse(pctldev, > + dbg->config, > + &configs->configs[i]); I assume that "parse" function is intended to actually write the new result into configs->configs[i]. That's more than parsing. s/dbg_parse/dbg_modify/ perhaps to make this more explicit, and also mention this in the kerneldoc for the confops structure in the header?
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. >> +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. >> +/** >> + * 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 >> + */ > This comment seems stale; I don't believe there are any pinconf-name and > pinconf-state files; aren't they part of the data written to the one > debugfs file each time? Right. Comment definitely needs update. > >> +static int pinconf_dbg_config_print(struct seq_file *s, void *d) >> + if (strncmp(map->dev_name, dbg->dev_name, MAX_NAME_LEN) != 0) > Wouldn't it be simpler to always ensure dbg->dev_name was > NULL-terminated, and just use !strcmp() here? Same for dbg->state_name > below. Same comment for the other function below. Yes that looks better. >> + mutex_unlock(&pinctrl_mutex); > Don't you need the lock held to call confops->xxx() below, to make sure > that the driver isn't unregistered between finding it, and calling the > confops function? Right as well. > >> + 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"); >> + } > Hmmm. At least including the parsed values that were being searched for > might be useful? Right as well. > >> + confops->pin_config_config_dbg_show(pctldev, >> + s, config); > I think that function name is wrong; two "config_" in a row. Does this > compile? Yes it compiles. The pinconf_ops contains an operation named like this (pin_config_config_dbg_show) and I think this is the one we need here. > >> +/** >> + * 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 > Similar stale comment. Yes, right. > >> + * @user_buf: contains 'modify configs_pin devicename state pinname newvalue' > It might be useful to say which parts of that are literal strings and > which are variables/values to be changed? Yes also part of update needed in the comment > >> +static int pinconf_dbg_config_write(struct file *file, >> + const char __user *user_buf, size_t count, loff_t *ppos) >> + /* Get arg: 'modify' */ >> + if (!strncmp(b, "modify ", strlen("modify "))) { >> + dbg->req_type = PINCONF_DBG_REQ_TYPE_MODIFY; >> + b += strlen("modify "); >> + } else { >> + return -EINVAL; >> + } > There's rather a lot of duplication of strings and strlen calls there. > Why not: > > a) Start using strsep() for the very first fields too, so everything is > consistent. > > b) Put the error-path first. > > In other words: > > token = strsep(&b, " "); > if (!token) > return -EINVAL; > if (!strcmp(token, "modify")) > return -EINVAL; Thanks for the proposal, code looks indeed better like this >> + /* 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; >> + } > That could be transformed the same way. > >> + /* 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; > It might make sense to put the error-handling first, and why sscanf not > strcpy? So: > > token = strsep(&b, " "); > if (token == NULL) > return -EINVAL; > if (strlen(token) >= MAX_NAME_LEN) > return -EINVAL; > strcpy(dbg->dev_name, token); > > (or even just use strncpy followed by explicit NULL termination and > truncate text that's too long, or kstrdup() it to avoid limits?) Same here. Looks better > >> + if (confops && confops->pin_config_dbg_parse) { >> + for (i = 0; i < configs->num_configs; i++) { >> + confops->pin_config_dbg_parse(pctldev, >> + dbg->config, >> + &configs->configs[i]); > I assume that "parse" function is intended to actually write the new > result into configs->configs[i]. That's more than parsing. > s/dbg_parse/dbg_modify/ perhaps to make this more explicit, and also > mention this in the kerneldoc for the confops structure in the header? I ended up with ->pin_config_dbg_parse_modify(). Hope this is ok. Linus will post a V3 version including above needed updates.
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.
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c index baee2cc..faabc25 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_parse) { + for (i = 0; i < configs->num_configs; i++) { + confops->pin_config_dbg_parse(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..fbe02de 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_dbg_parse: optional debugfs to parse a string 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_parse) (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);