| Message ID | 1366841010-2823-1-git-send-email-syin@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 25, 2013 at 12:03 AM, Sherman Yin <syin@broadcom.com> wrote: > Currently, when setting pin configuration in the pinctrl framework, > pin_config_set() or pin_config_group_set() is called in a loop to set one > configuration at a time for the specified pin or group. > > pin_config_set_all() and pin_config_group_set_all() are defined to set all > configurations for the specified pin or group at the same time. These optional > APIs allow pinctrl drivers to optimize and combine pin configurations to reduce > the number of register writes required to configure a pin. > > If both pin_config_set() and pin_config_set_all() are defined by a driver, the > latter takes precedence. Similarly for pin_config_group_set_all(). > > Signed-off-by: Sherman Yin <syin@broadcom.com> NAK, sorry. We discussed this already at the creation of the pinctrl framework. What you should do is to add an optional pin_config_set_complete() and pin_config_group_set_complete() callback that is called after the framework has iterated over all configs. This way the responsibility to resolve the sequence of config calls and validate the validity of that sequence is pushed down to the driver, it may do this incrementally or at pin_config_set_complete() and then write the resulting config to the hardware as an effect of pin_config_set_complete(). The driver takes care of caching intermediate settings for the pin until the complete() callback is called. Can you devise a patch like this instead? I am also very hesitant to merge this if there is no code using it, i.e. if you don't have a corresponding driver to submit at the same time, it will not be merged, as I don't like to maintain dead code. Yours, Linus Walleij
Hi Linus, > We discussed this already at the creation of the pinctrl framework. > > What you should do is to add an optional pin_config_set_complete() > and pin_config_group_set_complete() > callback that is called after the framework has iterated over all > configs. Perhaps I have misunderstood you previously, as I thought you are ok with letting the driver take care of the loop. The difference being instead of just moving the for loop inside the driver's pin_config_set(), I followed Stephen's suggestion due to concern about drivers duplicating the loop: (quote again here since I didn't send to the mail list last time) [quote] >>>>> pin_config_set() is called for each element of the array, one at a time: >>>>> >>>>> 328int pinconf_apply_setting(struct pinctrl_setting const *setting) >>>>> ... >>>>> 345 for (i = 0; i < setting->data.configs.num_configs; i++) { >>>>> 346 ret = ops->pin_config_set(pctldev, >>>>> 347 setting->data.configs.group_or_pin, >>>>> 348 setting->data.configs.configs[i]); >>>>> >>>>> Wouldn't it be more flexible if the for loop is moved inside the custom >>>>> pin_config_set() function, and the core code pass in the whole >>>>> configs[] array and num_configs? This allows the driver to possibly >>>>> combine different configs and make less write() calls to the registers. >>>> >>>> What you are saying is making a lot of sense. >>>> >>>> And I even think I have that problem where the batch of writes >>>> may cause spurious IRQs that can be suppressed by disabling >>>> IRQs in the driver when processing a queue of configs. >>>> >>>> Stephen what do you think? >>>> >>>> Sherman, would you be interested in writing a patch for this? >>>> >>>> You would have to patch all current users of the interface in >>>> the same patch. >>>> >>>> You could actually provide a static inline helper to iterate over >>>> the array to make it hard to do mistakes. >>> >>> I can see that applying all the configs at once might be useful. >>> >>> However, I don't think it makes sense to force all the drivers to >>> iterate over all the configs themselves; that would just be duplicating >>> the for loop in many drivers without any purpose. >>> >>> Instead, perhaps add a new "set_configs" operation in the driver >>> structure. The core can call set_configs once if exists, and if not, the >>> core can loop over each individual config and call the existing >>> set_config function. This is just like the set_function operation, which >>> can either be called once on a group, or once per pin, by the core. >> >> I don't mind writing the patch, once we agree on the method. I can't test >> the other platforms though. >> >> Stephen, I'm not quite sure about duplicating the for loop - I was thinking >> just to remove the for loop in pinconf_apply_setting() and add the loop in >> the driver-defined function, so in the end there is still only 1 for loop. >> Functionally it shouldn't affect existing drivers, but it gives new drivers >> an opportunity to optimize based on their register design. > >I understand, but this forces all drivers to implement the loop >themselves, and hence causes duplication. By making the new feature >optional, you avoid that except where it's useful. > >But I suppose the loop itself is pretty small either way. [/quote] > This way the responsibility to resolve the sequence of config > calls and validate the validity of that sequence is pushed down > to the driver, it may do this incrementally or at pin_config_set_complete() > and then write the resulting config to the hardware as an > effect of pin_config_set_complete(). Please correctly if I'm wrong, but I think with the functions in my patch, the driver still have full control of / responsibility to resolve config sequence and perform validation. The difference is instead of feeding the driver one config at time and telling it to write when the complete() function is called, my patch just gives the driver all configs in one call and expects the driver to parse, validate and write accordingly. > The driver takes care of caching intermediate settings for the pin > until the complete() callback is called. > Can you devise a patch like this instead? I do have some concerns about the driver caching all the configs until complete() is called... I'm ok either way as long as it provides a way to consolidate pin config register writes. Just want to clarify things with you before I go ahead and modify my patch and driver. > I am also very hesitant to merge this if there is no code using > it, i.e. if you don't have a corresponding driver to submit > at the same time, it will not be merged, as I don't like to > maintain dead code. I do plan to upstream a pinctrl driver that makes use of the new function, so I can make this patch (or one that implements the pin_config_set_complete()) part of that commit. Regards, Sherman
On Sat, Apr 27, 2013 at 12:02 AM, Sherman Yin <syin@broadcom.com> wrote: > Perhaps I have misunderstood you previously, as I thought you are ok with letting > the driver take care of the loop. The difference being instead of just moving the for > loop inside the driver's pin_config_set(), I followed Stephen's suggestion due to > concern about drivers duplicating the loop: (...) > Please correctly if I'm wrong, but I think with the functions in my patch, the > driver still have full control of / responsibility to resolve config sequence > and perform validation. The difference is instead of feeding the driver one > config at time and telling it to write when the complete() function is called, > my patch just gives the driver all configs in one call and expects the driver > to parse, validate and write accordingly. (...) >> The driver takes care of caching intermediate settings for the pin >> until the complete() callback is called. >> Can you devise a patch like this instead? > > I do have some concerns about the driver caching all the configs until complete() > is called... I'm ok either way as long as it provides a way to consolidate pin config register > writes. Just want to clarify things with you before I go ahead and modify > my patch and driver. OK I thought a bit about this too and I see the problem. However I think it's cluttery to have both pin_config_set() and pin_config_set_all(). If you want it to be done this way, I think you should modify pin_config_set() like so instead: - int (*pin_config_set) (struct pinctrl_dev *pctldev, - unsigned pin, - unsigned long config); + int (*pin_config_set) (struct pinctrl_dev *pctldev, + const struct pinctrl_setting_configs *configs, unsigned num_configs); And then change all the drivers. (And vice versa mutatis mutandis for the group function.) Notice especially the added num_configs parameter that was missing from pin_config_set_all() in your patch, please tell me how else you deduct the size of the configs array. To the drivers to iterate over the array of configs, please introduce a static helper function: for_each_config() in something like <linux/pinctrl/pinconf.h> so it's easy for driver writers to get this right. (You'll be much helped by this function when refactoring the existing drivers as well I think.) Doing the same thing with two different functions isn't helpful, refactoring like this is much better, it's just more work :-D Yours, Linus Walleij
>OK I thought a bit about this too and I see the problem. > >However I think it's cluttery to have both >pin_config_set() and pin_config_set_all(). > >If you want it to be done this way, I think you should modify >pin_config_set() like so instead: > >- int (*pin_config_set) (struct pinctrl_dev *pctldev, >- unsigned pin, >- unsigned long config); >+ int (*pin_config_set) (struct pinctrl_dev *pctldev, >+ const struct >pinctrl_setting_configs *configs, unsigned num_configs); > >And then change all the drivers. > >(And vice versa mutatis mutandis for the group function.) So that's basically back to the initial suggestion of moving the loop inside the driver's function instead of creating a separate API? Stephen said he is concerned about the duplicated loop but did say it's not a big deal. >Notice especially the added num_configs parameter that was >missing from pin_config_set_all() in your patch, please tell >me how else you deduct the size of the configs array. We are passing a pointer to struct pinctrl_setting_configs, which should have the num_configs already: 105struct pinctrl_setting_configs { 106 unsigned group_or_pin; 107 unsigned long *configs; 108 unsigned num_configs; 109}; I want to be sure you are ok with exposing this struct to consumers (ie. drivers). If not, we could pass the 3 components in the API instead. >To the drivers to iterate over the array of configs, >please introduce a static helper function: > >for_each_config() in something like <linux/pinctrl/pinconf.h> >so it's easy for driver writers to get this right. (You'll be much >helped by this function when refactoring the existing drivers as well >I think.) Yup ok. >Doing the same thing with two different functions isn't >helpful, refactoring like this is much better, it's just more >work :-D Yes it's more work, and a bit more risk of breaking stuff. Out of the 2 methods, I do like this better since there are less APIs. I will probably have to get this patch in after my pinctrl driver since it is almost ready. Thanks! Sherman
On Thu, May 2, 2013 at 2:07 AM, Sherman Yin <syin@broadcom.com> wrote: > [Me]: >>Notice especially the added num_configs parameter that was >>missing from pin_config_set_all() in your patch, please tell >>me how else you deduct the size of the configs array. > > We are passing a pointer to struct pinctrl_setting_configs, > which should have the num_configs already: > > 105struct pinctrl_setting_configs { > 106 unsigned group_or_pin; > 107 unsigned long *configs; > 108 unsigned num_configs; > 109}; > > I want to be sure you are ok with exposing this struct to > consumers (ie. drivers). If not, we could pass the 3 > components in the API instead. Damned I got it wrong. I was totally confused by the passing of the struct to the driver and not really grasping what it contained. The idea was to pass the array of configs (since this is dealing with configs, not multuiplexing) and then a number telling how many configs there are in the array. Pass struct pinctrl_setting_configs is not OK because then all of a sudden we're starting to expose a lot of pinctrl internals to the drivers and that will invariably be abused: one day someone will make a pinconfig function that look at the mux group just because it's convenient... So I was after something like this: int (*pin_config_set) (struct pinctrl_dev *pctldev, unsigned pin, unsigned long *configs, unsigned num_configs); (And same for groups.) So we get an array of configs and the driver can iterate over this. Since these are unsigned longs we do not need a special helper as I thought. And then you should refactor the drivers to account for this. Yours, Linus Walleij
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c index d611ecf..568e4f2 100644 --- a/drivers/pinctrl/pinconf.c +++ b/drivers/pinctrl/pinconf.c @@ -34,7 +34,8 @@ int pinconf_check_ops(struct pinctrl_dev *pctldev) return -EINVAL; } /* We have to be able to config the pins in SOME way */ - if (!ops->pin_config_set && !ops->pin_config_group_set) { + if (!ops->pin_config_set && !ops->pin_config_group_set && + !ops->pin_config_set_all && !ops->pin_config_group_set_all) { dev_err(pctldev->dev, "pinconf has to be able to set a pins config\n"); return -EINVAL; @@ -338,40 +339,65 @@ int pinconf_apply_setting(struct pinctrl_setting const *setting) switch (setting->type) { case PIN_MAP_TYPE_CONFIGS_PIN: - if (!ops->pin_config_set) { - dev_err(pctldev->dev, "missing pin_config_set op\n"); - return -EINVAL; - } - for (i = 0; i < setting->data.configs.num_configs; i++) { - ret = ops->pin_config_set(pctldev, - setting->data.configs.group_or_pin, - setting->data.configs.configs[i]); + if (ops->pin_config_set_all) { + ret = ops->pin_config_set_all(pctldev, + &setting->data.configs); if (ret < 0) { dev_err(pctldev->dev, - "pin_config_set op failed for pin %d config %08lx\n", - setting->data.configs.group_or_pin, - setting->data.configs.configs[i]); + "pin_config_set_all op failed for pin %d\n", + setting->data.configs.group_or_pin); return ret; } - } - break; - case PIN_MAP_TYPE_CONFIGS_GROUP: - if (!ops->pin_config_group_set) { + } else if (ops->pin_config_set) { + for (i = 0; i < setting->data.configs.num_configs; i++) { + ret = ops->pin_config_set(pctldev, + setting->data.configs.group_or_pin, + setting->data.configs.configs[i]); + if (ret < 0) { + dev_err(pctldev->dev, + "pin_config_set op failed for " + "pin %d config %08lx\n", + setting->data.configs.group_or_pin, + setting->data.configs.configs[i]); + return ret; + } + } + } else { dev_err(pctldev->dev, - "missing pin_config_group_set op\n"); + "missing pin_config_set or " + "pin_config_set_all op\n"); return -EINVAL; } - for (i = 0; i < setting->data.configs.num_configs; i++) { - ret = ops->pin_config_group_set(pctldev, - setting->data.configs.group_or_pin, - setting->data.configs.configs[i]); + break; + case PIN_MAP_TYPE_CONFIGS_GROUP: + if (ops->pin_config_group_set_all) { + ret = ops->pin_config_group_set_all(pctldev, + &setting->data.configs); if (ret < 0) { dev_err(pctldev->dev, - "pin_config_group_set op failed for group %d config %08lx\n", - setting->data.configs.group_or_pin, - setting->data.configs.configs[i]); + "pin_config_group_set_all op failed for group %d\n", + setting->data.configs.group_or_pin); return ret; } + } else if (ops->pin_config_group_set) { + for (i = 0; i < setting->data.configs.num_configs; i++) { + ret = ops->pin_config_group_set(pctldev, + setting->data.configs.group_or_pin, + setting->data.configs.configs[i]); + if (ret < 0) { + dev_err(pctldev->dev, + "pin_config_group_set op failed " + "for group %d config %08lx\n", + setting->data.configs.group_or_pin, + setting->data.configs.configs[i]); + return ret; + } + } + } else { + dev_err(pctldev->dev, + "missing pin_config_group_set or " + "pin_config_group_set_all op\n"); + return -EINVAL; } break; default: diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h index e7a7201..b838756 100644 --- a/include/linux/pinctrl/pinconf.h +++ b/include/linux/pinctrl/pinconf.h @@ -16,6 +16,7 @@ struct pinctrl_dev; struct seq_file; +struct pinctrl_setting_configs; /** * struct pinconf_ops - pin config operations, to be implemented by @@ -25,9 +26,11 @@ struct seq_file; * @pin_config_get: get the config of a certain pin, if the requested config * is not available on this controller this should return -ENOTSUPP * and if it is available but disabled it should return -EINVAL - * @pin_config_set: configure an individual pin + * @pin_config_set: apply one configuration to an individual pin + * @pin_config_set_all: apply all configurations to 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_set: apply one configuration to all pins in a group + * @pin_config_group_set_all: apply all configurations to all pins in a group * @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 @@ -45,12 +48,16 @@ struct pinconf_ops { int (*pin_config_set) (struct pinctrl_dev *pctldev, unsigned pin, unsigned long config); + int (*pin_config_set_all) (struct pinctrl_dev *pctldev, + const struct pinctrl_setting_configs *configs); int (*pin_config_group_get) (struct pinctrl_dev *pctldev, unsigned selector, unsigned long *config); int (*pin_config_group_set) (struct pinctrl_dev *pctldev, unsigned selector, unsigned long config); + int (*pin_config_group_set_all) (struct pinctrl_dev *pctldev, + const struct pinctrl_setting_configs *configs); void (*pin_config_dbg_show) (struct pinctrl_dev *pctldev, struct seq_file *s, unsigned offset);
Currently, when setting pin configuration in the pinctrl framework, pin_config_set() or pin_config_group_set() is called in a loop to set one configuration at a time for the specified pin or group. pin_config_set_all() and pin_config_group_set_all() are defined to set all configurations for the specified pin or group at the same time. These optional APIs allow pinctrl drivers to optimize and combine pin configurations to reduce the number of register writes required to configure a pin. If both pin_config_set() and pin_config_set_all() are defined by a driver, the latter takes precedence. Similarly for pin_config_group_set_all(). Signed-off-by: Sherman Yin <syin@broadcom.com> --- drivers/pinctrl/pinconf.c | 74 ++++++++++++++++++++++++++------------- include/linux/pinctrl/pinconf.h | 11 ++++-- 2 files changed, 59 insertions(+), 26 deletions(-)