| Message ID | 1415041531-15520-4-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote: > --- a/include/linux/pinctrl/pinctrl.h > +++ b/include/linux/pinctrl/pinctrl.h > @@ -117,6 +118,8 @@ struct pinctrl_ops { > * @confops: pin config operations vtable, if you support pin configuration in > * your driver > * @owner: module providing the pin controller, used for refcounting > + * @num_dt_params: Number of driver-specifid DT parameters driver-specific > + * @params: List of DT parameters Missing @conf_items documentation. > */ > struct pinctrl_desc { > const char *name; > @@ -126,6 +129,11 @@ struct pinctrl_desc { > const struct pinmux_ops *pmxops; > const struct pinconf_ops *confops; > struct module *owner; > +#if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_OF) > + unsigned int num_dt_params; > + const struct pinconf_generic_dt_params *params; > + const struct pin_config_item *conf_items; > +#endif Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote: > Additionally to the generic DT parameters, allow drivers to provide > driver-specific DT parameters to be used with the generic parser > infrastructure. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> I like the looks of this, but the patch description is a bit terse. I'd like it to describe some of the refactorings being done to the intrinsics, because I have a hard time following the patch. First please rebase onto the "devel" branch in the pin control tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c which is merged there is actually doing this already: for_each_child_of_node(np_config, np) { ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map, &reserv, nmaps, type); if (ret) break; ret = pmic_gpio_dt_subnode_to_map(pctldev, np, map, &reserv, nmaps, type); if (ret) break; } So it should be patched to illustrate the point of this code. I'd like feedback from Ivan+Björn on the code too if possible. > - ret = pinconf_generic_parse_dt_config(np, &configs, &nconfigs); > + ret = pinconf_generic_parse_dt_config(np, pctldev, &configs, &nconfigs); > if (nconfigs) > has_config = 1; > np_config = of_parse_phandle(np, "ste,config", 0); > if (np_config) { > - ret = pinconf_generic_parse_dt_config(np_config, &configs, > - &nconfigs); > + ret = pinconf_generic_parse_dt_config(np_config, pctldev, > + &configs, &nconfigs); This code is patched upstream so that ABx500 only uses generic config. Again rebase on "devel" > -void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev, > - struct seq_file *s, unsigned pin) > +static void _pinconf_generic_dump(struct pinctrl_dev *pctldev, > + struct seq_file *s, const char *gname, > + unsigned pin, > + const struct pin_config_item *items, > + int nitems) Don't use functions named _foo, actually the underscore is for preprocessor and compiler things in my book, just give it an intuitive name instead. Like pinconf_generic_dump_one() if that is suitable or whatever. This changes the function signature from something quite intuitively understood to something pretty hard to understand, so you need to add kerneldoc to it. (That also enhance my understanding of the patch.) > -void pinconf_generic_dump_group(struct pinctrl_dev *pctldev, > - struct seq_file *s, const char *gname) > +static void pinconf_generic_dump(struct pinctrl_dev *pctldev, > + struct seq_file *s, const char *gname, > + unsigned pin) This looks intuitive and nice. > + _pinconf_generic_dump(pctldev, s, gname, pin, > + conf_items, ARRAY_SIZE(conf_items)); > + if (pctldev->desc->num_dt_params) { > + BUG_ON(!pctldev->desc->conf_items); Don't use BUG_ON() like that, it's nasty. Always try to recover and bail out instead. > +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev, > + struct seq_file *s, unsigned pin) > +{ > + pinconf_generic_dump(pctldev, s, NULL, pin); > +} > + > +void pinconf_generic_dump_group(struct pinctrl_dev *pctldev, > + struct seq_file *s, const char *gname) > +{ > + pinconf_generic_dump(pctldev, s, gname, 0); > +} Do you really need these helpers? Isn't it simpler just to call the generic function with the different arguments? > @@ -148,17 +132,22 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev, > seq_printf(s, "%s: 0x%x", conf_items[i].display, > pinconf_to_config_argument(config)); > } > + > + if (!pctldev->desc->num_dt_params) > + return; > + > + BUG_ON(!pctldev->desc->conf_items); No BUG_ON() dev_err() and exit. > +static void _parse_dt_cfg(struct device_node *np, > + const struct pinconf_generic_dt_params *params, > + unsigned int count, > + unsigned long *cfg, > + unsigned int *ncfg) Should return an error code right? Kerneldoc doesn't hurt either. > +{ > + int i; > + > + for (i = 0; i < count; i++) { > + u32 val; > + int ret; > + const struct pinconf_generic_dt_params *par = ¶ms[i]; > + > + ret = of_property_read_u32(np, par->property, &val); Not checking this return value. Alter the function to return an int value on success. > + > + /* property not found */ > + if (ret == -EINVAL) > + continue; > + > + /* use default value, when no value is specified */ > + if (ret) > + val = par->default_value; > + > + pr_debug("found %s with value %u\n", par->property, val); > + cfg[*ncfg] = pinconf_to_config_packed(par->param, val); > + (*ncfg)++; > + } > +} There is something very unintuitive about this loop. You pass two counter indexes (count, ncfg) in basically, that is looking weird, does it have to look like that? Especially since there is no bounds check on ncfg! Just use one index in the loop please. Assign *ncfg = ... after the loop has *successfully* iterated. > int pinconf_generic_parse_dt_config(struct device_node *np, > + struct pinctrl_dev *pctldev, > unsigned long **configs, > unsigned int *nconfigs) This is a good refactoring, but no _foo naming! > { > unsigned long *cfg; > - unsigned int ncfg = 0; > + unsigned int max_cfg, ncfg = 0; > int ret; > - int i; > - u32 val; > > if (!np) > return -EINVAL; > > /* allocate a temporary array big enough to hold one of each option */ > - cfg = kzalloc(sizeof(*cfg) * ARRAY_SIZE(dt_params), GFP_KERNEL); > + max_cfg = ARRAY_SIZE(dt_params); > + if (pctldev) > + max_cfg += pctldev->desc->num_dt_params; > + cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL); Aha this looks good... > + _parse_dt_cfg(np, dt_params, ARRAY_SIZE(dt_params), cfg, &ncfg); > + if (pctldev && pctldev->desc->num_dt_params) { > + BUG_ON(!pctldev->desc->params); No BUG_ON() > + _parse_dt_cfg(np, pctldev->desc->params, > + pctldev->desc->num_dt_params, cfg, &ncfg); This looks similar to what Qualcomm's driver is doing. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote: > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann > brinkmann@xilinx.com> wrote: > > > Additionally to the generic DT parameters, allow drivers to provide > > driver-specific DT parameters to be used with the generic parser > > infrastructure. > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com> > > I like the looks of this, but the patch description is a bit terse. > I'd like it to describe some of the refactorings being done > to the intrinsics, because I have a hard time following the patch. > > First please rebase onto the "devel" branch in the pin control > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > which is merged there is actually doing this already: > > > for_each_child_of_node(np_config, np) { > ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map, > &reserv, nmaps, type); > if (ret) > break; > > ret = pmic_gpio_dt_subnode_to_map(pctldev, np, map, &reserv, > nmaps, type); > if (ret) > break; > } > > So it should be patched to illustrate the point of this code. > I like the idea, but have issues with implementations :-). It is supposed that additional parameters are not generic, otherwise they will be part of enum pin_config_param, right? Probably it will be better if clients could pass array with driver specific dt bindings to pinconf_generic_dt_node_to_map()? Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-11-18 at 10:50AM +0200, Ivan T. Ivanov wrote: > > On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote: > > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann > > brinkmann@xilinx.com> wrote: > > > > > Additionally to the generic DT parameters, allow drivers to provide > > > driver-specific DT parameters to be used with the generic parser > > > infrastructure. > > > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com> > > > > I like the looks of this, but the patch description is a bit terse. > > I'd like it to describe some of the refactorings being done > > to the intrinsics, because I have a hard time following the patch. > > > > First please rebase onto the "devel" branch in the pin control > > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > which is merged there is actually doing this already: > > > > > > for_each_child_of_node(np_config, np) { > > ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map, > > &reserv, nmaps, type); > > if (ret) > > break; > > > > ret = pmic_gpio_dt_subnode_to_map(pctldev, np, map, &reserv, > > nmaps, type); > > if (ret) > > break; > > } > > > > So it should be patched to illustrate the point of this code. > > > > I like the idea, but have issues with implementations :-). > > It is supposed that additional parameters are not generic, > otherwise they will be part of enum pin_config_param, right? > > Probably it will be better if clients could pass array with > driver specific dt bindings to pinconf_generic_dt_node_to_map()? My idea was to hide that API from the driver. You just pass those parameters as part of the struct pctldev and the parser - whether this generic one or anything else - would do the right thing. I don't think calling the parser from the driver is the right approach. Thanks, Sören -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-11-18 at 09:25 -0800, Sören Brinkmann wrote: > On Tue, 2014-11-18 at 10:50AM +0200, Ivan T. Ivanov wrote: > > > > On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote: > > > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann > > > brinkmann@xilinx.com> wrote: > > > > > > > Additionally to the generic DT parameters, allow drivers to > > > > provide driver-specific DT parameters to be used with the > > > > generic parser infrastructure. > > > > > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com> > > > > > > I like the looks of this, but the patch description is a bit > > > terse. I'd like it to describe some of the refactorings being > > > done > > > to the intrinsics, because I have a hard time following the > > > patch. > > > > > > First please rebase onto the "devel" branch in the pin control > > > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > > which is merged there is actually doing this already: > > > > > > > > > for_each_child_of_node(np_config, np) { > > > ret = pinconf_generic_dt_subnode_to_map(pctldev, > > > np, map, > > > &reserv, > > > nmaps, type); > > > if (ret) > > > break; > > > > > > ret = pmic_gpio_dt_subnode_to_map(pctldev, np, > > > map, &reserv, > > > nmaps, type); > > > if (ret) > > > break; > > > } > > > > > > So it should be patched to illustrate the point of this code. > > > > > > > I like the idea, but have issues with implementations :-). > > > > It is supposed that additional parameters are not generic, > > otherwise they will be part of enum pin_config_param, right? > > > > Probably it will be better if clients could pass array with > > driver specific dt bindings to pinconf_generic_dt_node_to_map()? > > My idea was to hide that API from the driver. You just pass those > parameters as part of the struct pctldev and the parser - whether > this generic one or anything else - would do the right thing. I > don't think calling the parser from the driver is the right approach. Drivers already know about dt_node_to_map(). My proposal will make drivers, which register non-standard bindings, little bit simpler. With your approach probably we can remove dt_node_to_map() and dt_free_map() callbacks? Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ivan, On Wed, 2014-11-19 at 09:49AM +0200, Ivan T. Ivanov wrote: > > On Tue, 2014-11-18 at 09:25 -0800, Sören Brinkmann wrote: > > On Tue, 2014-11-18 at 10:50AM +0200, Ivan T. Ivanov wrote: > > > > > > On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote: > > > > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann > > > > brinkmann@xilinx.com> wrote: > > > > > > > > > Additionally to the generic DT parameters, allow drivers to > > > > > provide driver-specific DT parameters to be used with the > > > > > generic parser infrastructure. > > > > > > > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com> > > > > > > > > I like the looks of this, but the patch description is a bit > > > > terse. I'd like it to describe some of the refactorings being > > > > done > > > > to the intrinsics, because I have a hard time following the > > > > patch. > > > > > > > > First please rebase onto the "devel" branch in the pin control > > > > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > > > which is merged there is actually doing this already: > > > > > > > > > > > > for_each_child_of_node(np_config, np) { > > > > ret = pinconf_generic_dt_subnode_to_map(pctldev, > > > > np, map, > > > > &reserv, > > > > nmaps, type); > > > > if (ret) > > > > break; > > > > > > > > ret = pmic_gpio_dt_subnode_to_map(pctldev, np, > > > > map, &reserv, > > > > nmaps, type); > > > > if (ret) > > > > break; > > > > } > > > > > > > > So it should be patched to illustrate the point of this code. > > > > > > > > > > I like the idea, but have issues with implementations :-). > > > > > > It is supposed that additional parameters are not generic, > > > otherwise they will be part of enum pin_config_param, right? > > > > > > Probably it will be better if clients could pass array with > > > driver specific dt bindings to pinconf_generic_dt_node_to_map()? > > > > My idea was to hide that API from the driver. You just pass those > > parameters as part of the struct pctldev and the parser - whether > > this generic one or anything else - would do the right thing. I > > don't think calling the parser from the driver is the right approach. > > Drivers already know about dt_node_to_map(). My proposal will make > drivers, which register non-standard bindings, little bit simpler. And I think this is not the best solution. Those drivers essentially still do the DT parsing themselves, just call some common helpers. I think that should be well separated. The pinctrl driver just assembles some data structure that has the information regarding custom properties and the core handles the rest. I find the approach I have in the zynq driver - which does it that way - more elegant. The only reference to the core parser there is the function pointer to the generic node to map function. And even that could probably disappear in the long term when everything migrates to using the core parser and generic bindings. Also, why does it make the driver simpler? In my zynq driver I only have those mentioned data structs and nothing in regards of parsing the DT. With drivers calling the parser you duplicate exactly that all over the place in each driver. More code, more duplication. And I don't see where things become simpler. The core becomes a little more complex, but well, that's why it gets consolidated there, right? Thanks, Sören -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-11-19 at 07:35 -0800, Sören Brinkmann wrote: > Hi Ivan, > > On Wed, 2014-11-19 at 09:49AM +0200, Ivan T. Ivanov wrote: > > On Tue, 2014-11-18 at 09:25 -0800, Sören Brinkmann wrote: > > > On Tue, 2014-11-18 at 10:50AM +0200, Ivan T. Ivanov wrote: > > > > On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote: > > > > > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann > > > > > brinkmann@xilinx.com> wrote: > > > > > > > > > > > Additionally to the generic DT parameters, allow drivers to > > > > > > provide driver-specific DT parameters to be used with the > > > > > > generic parser infrastructure. > > > > > > > > > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com> > > > > > > > > > > I like the looks of this, but the patch description is a bit > > > > > terse. I'd like it to describe some of the refactorings being > > > > > done > > > > > to the intrinsics, because I have a hard time following the > > > > > patch. > > > > > > > > > > First please rebase onto the "devel" branch in the pin control > > > > > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > > > > which is merged there is actually doing this already: > > > > > > > > > > > > > > > for_each_child_of_node(np_config, np) { > > > > > ret = pinconf_generic_dt_subnode_to_map(pctldev, > > > > > np, map, > > > > > &reserv, > > > > > nmaps, type); > > > > > if (ret) > > > > > break; > > > > > > > > > > ret = pmic_gpio_dt_subnode_to_map(pctldev, np, > > > > > map, &reserv, > > > > > nmaps, type); > > > > > if (ret) > > > > > break; > > > > > } > > > > > > > > > > So it should be patched to illustrate the point of this code. > > > > > > > > > > > > > I like the idea, but have issues with implementations :-). > > > > > > > > It is supposed that additional parameters are not generic, > > > > otherwise they will be part of enum pin_config_param, right? > > > > > > > > Probably it will be better if clients could pass array with > > > > driver specific dt bindings to pinconf_generic_dt_node_to_map()? > > > > > > My idea was to hide that API from the driver. You just pass those > > > parameters as part of the struct pctldev and the parser - whether > > > this generic one or anything else - would do the right thing. I > > > don't think calling the parser from the driver is the right approach. > > > > Drivers already know about dt_node_to_map(). My proposal will make > > drivers, which register non-standard bindings, little bit simpler. > > And I think this is not the best solution. Those drivers essentially > still do the DT parsing themselves, Yes, and this could be avoided if there API which allow them to pass non-standard configuration maps. > just call some common helpers. I > think that should be well separated. Around 27 of 30 drivers are using custom dt_node_to_map(). And this is because most of them are using custom "x,pins", "x,functions" and 'x,groups" properties and I think that this is bigger issue, how this is addressed in this patch? > The pinctrl driver just assembles > some data structure that has the information regarding custom properties > and the core handles the rest. Yup, that is nice. What will be really nice if it also handle custom, "function", "groups" and "pins" properties. Otherwise most of the drivers will not be able to benefit from this. I just wanted to share my opinion. Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-11-20 at 10:06AM +0200, Ivan T. Ivanov wrote: > > On Wed, 2014-11-19 at 07:35 -0800, Sören Brinkmann wrote: > > Hi Ivan, > > > > On Wed, 2014-11-19 at 09:49AM +0200, Ivan T. Ivanov wrote: > > > On Tue, 2014-11-18 at 09:25 -0800, Sören Brinkmann wrote: > > > > On Tue, 2014-11-18 at 10:50AM +0200, Ivan T. Ivanov wrote: > > > > > On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote: > > > > > > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann > > > > > > brinkmann@xilinx.com> wrote: > > > > > > > > > > > > > Additionally to the generic DT parameters, allow drivers to > > > > > > > provide driver-specific DT parameters to be used with the > > > > > > > generic parser infrastructure. > > > > > > > > > > > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com> > > > > > > > > > > > > I like the looks of this, but the patch description is a bit > > > > > > terse. I'd like it to describe some of the refactorings being > > > > > > done > > > > > > to the intrinsics, because I have a hard time following the > > > > > > patch. > > > > > > > > > > > > First please rebase onto the "devel" branch in the pin control > > > > > > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > > > > > which is merged there is actually doing this already: > > > > > > > > > > > > > > > > > > for_each_child_of_node(np_config, np) { > > > > > > ret = pinconf_generic_dt_subnode_to_map(pctldev, > > > > > > np, map, > > > > > > &reserv, > > > > > > nmaps, type); > > > > > > if (ret) > > > > > > break; > > > > > > > > > > > > ret = pmic_gpio_dt_subnode_to_map(pctldev, np, > > > > > > map, &reserv, > > > > > > nmaps, type); > > > > > > if (ret) > > > > > > break; > > > > > > } > > > > > > > > > > > > So it should be patched to illustrate the point of this code. > > > > > > > > > > > > > > > > I like the idea, but have issues with implementations :-). > > > > > > > > > > It is supposed that additional parameters are not generic, > > > > > otherwise they will be part of enum pin_config_param, right? > > > > > > > > > > Probably it will be better if clients could pass array with > > > > > driver specific dt bindings to pinconf_generic_dt_node_to_map()? > > > > > > > > My idea was to hide that API from the driver. You just pass those > > > > parameters as part of the struct pctldev and the parser - whether > > > > this generic one or anything else - would do the right thing. I > > > > don't think calling the parser from the driver is the right approach. > > > > > > Drivers already know about dt_node_to_map(). My proposal will make > > > drivers, which register non-standard bindings, little bit simpler. > > > > And I think this is not the best solution. Those drivers essentially > > still do the DT parsing themselves, > > Yes, and this could be avoided if there API which allow them to pass > non-standard configuration maps. > > > just call some common helpers. I > > think that should be well separated. > > Around 27 of 30 drivers are using custom dt_node_to_map(). And this is > because most of them are using custom "x,pins", "x,functions" and 'x,groups" > properties and I think that this is bigger issue, how this is addressed > in this patch? Not really in this patch, but Linus recently added the 'groups' property. Somewhere in this thread he explained how he'd like to see things used. With mux nodes that contain 'groups' and 'function' and conf nodes that can contain 'groups' or 'pins' and the pinconf options. And pins/groups would actually refer to only pins or groups respectively. Also, I hope all my changes here don't break the current behavior. So, those 27 driver should still be able to do what they currently do. But I hope they could migrated over to use the generic bindings only in the longer term, so that these custom properties disappear. > > > The pinctrl driver just assembles > > some data structure that has the information regarding custom properties > > and the core handles the rest. > > Yup, that is nice. What will be really nice if it also handle custom, > "function", "groups" and "pins" properties. Otherwise most of the drivers > will not be able to benefit from this. Why would you still need those? I think the idea is to get rid of custom pins, groups and function properties. Could you explain why those are needed and why that couldn't be migrated to the amended bindings as outlined by Linus, please? Thanks, Sören -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-11-20 at 08:22 -0800, Sören Brinkmann wrote: > > > Also, I hope all my changes here don't break the current behavior. So, > those 27 driver should still be able to do what they currently do. But I > hope they could migrated over to use the generic bindings only in the > longer term, so that these custom properties disappear. > > > > The pinctrl driver just assembles > > > some data structure that has the information regarding custom properties > > > and the core handles the rest. > > > > Yup, that is nice. What will be really nice if it also handle custom, > > "function", "groups" and "pins" properties. Otherwise most of the drivers > > will not be able to benefit from this. > > Why would you still need those? I don't need them :-). The point was that still majority of the drivers will have custom parsing functions. It would be nice if we could fix that too. I do understand that using custom "pins", "functions"... is something which is deprecated, but if core parsing functions allow passing custom strings for above purposes, in a similar way as your proposal, it will be easier for those drivers to migrate, I believe. Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ivan, On Fri, 2014-11-21 at 09:35AM +0200, Ivan T. Ivanov wrote: > > On Thu, 2014-11-20 at 08:22 -0800, Sören Brinkmann wrote: > > > > > > Also, I hope all my changes here don't break the current behavior. So, > > those 27 driver should still be able to do what they currently do. But I > > hope they could migrated over to use the generic bindings only in the > > longer term, so that these custom properties disappear. > > > > > > The pinctrl driver just assembles > > > > some data structure that has the information regarding custom properties > > > > and the core handles the rest. > > > > > > Yup, that is nice. What will be really nice if it also handle custom, > > > "function", "groups" and "pins" properties. Otherwise most of the drivers > > > will not be able to benefit from this. > > > > Why would you still need those? > > I don't need them :-). The point was that still majority of the drivers > will have custom parsing functions. It would be nice if we could fix > that too. I do understand that using custom "pins", "functions"... is > something which is deprecated, but if core parsing functions allow > passing custom strings for above purposes, in a similar way as your > proposal, it will be easier for those drivers to migrate, I believe. This does sound much more like a feature request than a fundamental problem with the implementation, now. And like Laurent's feature request, I'd like to turn this down. Otherwise this just gets hold up by feature requests blocking pinctrl-zynq. I think the interesting questions are: 1. Does it break any current user? 2. Is there anything fundamentally preventing adding your feature at some later time as part of such a migration you describe? Thanks, Sören -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2014-11-22 at 08:06 -0800, Sören Brinkmann wrote: > Hi Ivan, > > On Fri, 2014-11-21 at 09:35AM +0200, Ivan T. Ivanov wrote: > > On Thu, 2014-11-20 at 08:22 -0800, Sören Brinkmann wrote: > > > > > > Also, I hope all my changes here don't break the current behavior. So, > > > those 27 driver should still be able to do what they currently do. But I > > > hope they could migrated over to use the generic bindings only in the > > > longer term, so that these custom properties disappear. > > > > > > > > The pinctrl driver just assembles > > > > > some data structure that has the information regarding custom properties > > > > > and the core handles the rest. > > > > > > > > Yup, that is nice. What will be really nice if it also handle custom, > > > > "function", "groups" and "pins" properties. Otherwise most of the drivers > > > > will not be able to benefit from this. > > > > > > Why would you still need those? > > > > I don't need them :-). The point was that still majority of the drivers > > will have custom parsing functions. It would be nice if we could fix > > that too. I do understand that using custom "pins", "functions"... is > > something which is deprecated, but if core parsing functions allow > > passing custom strings for above purposes, in a similar way as your > > proposal, it will be easier for those drivers to migrate, I believe. > > This does sound much more like a feature request than a fundamental > problem with the implementation, now. And like Laurent's feature > request, I'd like to turn this down. Otherwise this just gets hold up by > feature requests blocking pinctrl-zynq. > I think the interesting questions are: > 1. Does it break any current user? > 2. Is there anything fundamentally preventing adding your feature at > some later time as part of such a migration you describe? Well, as I said, this is just my opinion. Is up to Linus to decide. Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Tue, 2014-11-11 at 03:53PM +0100, Linus Walleij wrote: > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann > <soren.brinkmann@xilinx.com> wrote: > > > Additionally to the generic DT parameters, allow drivers to provide > > driver-specific DT parameters to be used with the generic parser > > infrastructure. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > I like the looks of this, but the patch description is a bit terse. > I'd like it to describe some of the refactorings being done > to the intrinsics, because I have a hard time following the patch. I'll be a little more verbose :) > > First please rebase onto the "devel" branch in the pin control > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > which is merged there is actually doing this already: > > > for_each_child_of_node(np_config, np) { > ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map, > &reserv, nmaps, type); > if (ret) > break; > > ret = pmic_gpio_dt_subnode_to_map(pctldev, np, map, &reserv, > nmaps, type); > if (ret) > break; > } > > So it should be patched to illustrate the point of this code. I'll look into this. > > I'd like feedback from Ivan+Björn on the code too if possible. > > > - ret = pinconf_generic_parse_dt_config(np, &configs, &nconfigs); > > + ret = pinconf_generic_parse_dt_config(np, pctldev, &configs, &nconfigs); > > if (nconfigs) > > has_config = 1; > > np_config = of_parse_phandle(np, "ste,config", 0); > > if (np_config) { > > - ret = pinconf_generic_parse_dt_config(np_config, &configs, > > - &nconfigs); > > + ret = pinconf_generic_parse_dt_config(np_config, pctldev, > > + &configs, &nconfigs); > > This code is patched upstream so that ABx500 only uses generic config. > Again rebase on "devel" Yeah, causes a conflict, but seems to be pretty much the same. > > > -void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev, > > - struct seq_file *s, unsigned pin) > > +static void _pinconf_generic_dump(struct pinctrl_dev *pctldev, > > + struct seq_file *s, const char *gname, > > + unsigned pin, > > + const struct pin_config_item *items, > > + int nitems) > > Don't use functions named _foo, actually the underscore is for > preprocessor and compiler things in my book, just give it an intuitive > name instead. Like pinconf_generic_dump_one() if that is suitable > or whatever. > > This changes the function signature from something quite intuitively > understood to something pretty hard to understand, so you need to > add kerneldoc to it. (That also enhance my understanding of the > patch.) I'll rename it and add some documentation. > > > -void pinconf_generic_dump_group(struct pinctrl_dev *pctldev, > > - struct seq_file *s, const char *gname) > > +static void pinconf_generic_dump(struct pinctrl_dev *pctldev, > > + struct seq_file *s, const char *gname, > > + unsigned pin) > > This looks intuitive and nice. > > > + _pinconf_generic_dump(pctldev, s, gname, pin, > > + conf_items, ARRAY_SIZE(conf_items)); > > + if (pctldev->desc->num_dt_params) { > > + BUG_ON(!pctldev->desc->conf_items); > > Don't use BUG_ON() like that, it's nasty. Always try to > recover and bail out instead. I merge the condition into the if. > > > +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev, > > + struct seq_file *s, unsigned pin) > > +{ > > + pinconf_generic_dump(pctldev, s, NULL, pin); > > +} > > + > > +void pinconf_generic_dump_group(struct pinctrl_dev *pctldev, > > + struct seq_file *s, const char *gname) > > +{ > > + pinconf_generic_dump(pctldev, s, gname, 0); > > +} > > Do you really need these helpers? Isn't it simpler just > to call the generic function with the different arguments? I'll remove the helpers and patch the users of these functions. > > > @@ -148,17 +132,22 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev, > > seq_printf(s, "%s: 0x%x", conf_items[i].display, > > pinconf_to_config_argument(config)); > > } > > + > > + if (!pctldev->desc->num_dt_params) > > + return; > > + > > + BUG_ON(!pctldev->desc->conf_items); > > No BUG_ON() dev_err() and exit. As above. > > > +static void _parse_dt_cfg(struct device_node *np, > > + const struct pinconf_generic_dt_params *params, > > + unsigned int count, > > + unsigned long *cfg, > > + unsigned int *ncfg) > > Should return an error code right? Kerneldoc doesn't hurt either. I don't see a need for an error return. It's currently not needed and this refactoring doesn't change that, IMHO. I'll add kerneldoc. > > > +{ > > + int i; > > + > > + for (i = 0; i < count; i++) { > > + u32 val; > > + int ret; > > + const struct pinconf_generic_dt_params *par = ¶ms[i]; > > + > > + ret = of_property_read_u32(np, par->property, &val); > > Not checking this return value. Alter the function to return an > int value on success. It's checked in the very next statement?! And it's all handled in this function. No need to report anything to the caller. > > > + > > + /* property not found */ > > + if (ret == -EINVAL) > > + continue; > > + > > + /* use default value, when no value is specified */ > > + if (ret) > > + val = par->default_value; > > + > > + pr_debug("found %s with value %u\n", par->property, val); > > + cfg[*ncfg] = pinconf_to_config_packed(par->param, val); > > + (*ncfg)++; > > + } > > +} > > There is something very unintuitive about this loop. You pass two > counter indexes (count, ncfg) in basically, that is looking weird, > does it have to look like that? Especially since there is no > bounds check on ncfg! > > Just use one index in the loop please. Assign *ncfg = ... after > the loop has *successfully* iterated. I think this needs to be as is. There are two arrays @cfg and @params. @params holding the DT params parsed with @count indicating the boundary. And @cfg, where the parsed options are put with @ncfg being a write pointer. @nfcg can be passed non-zero into this function. The caller is responsible to allocate enough memory to hold all possible entries. > > > int pinconf_generic_parse_dt_config(struct device_node *np, > > + struct pinctrl_dev *pctldev, > > unsigned long **configs, > > unsigned int *nconfigs) > > This is a good refactoring, but no _foo naming! Will be renamed. > > > { > > unsigned long *cfg; > > - unsigned int ncfg = 0; > > + unsigned int max_cfg, ncfg = 0; > > int ret; > > - int i; > > - u32 val; > > > > if (!np) > > return -EINVAL; > > > > /* allocate a temporary array big enough to hold one of each option */ > > - cfg = kzalloc(sizeof(*cfg) * ARRAY_SIZE(dt_params), GFP_KERNEL); > > + max_cfg = ARRAY_SIZE(dt_params); > > + if (pctldev) > > + max_cfg += pctldev->desc->num_dt_params; > > + cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL); > > Aha this looks good... > > > + _parse_dt_cfg(np, dt_params, ARRAY_SIZE(dt_params), cfg, &ncfg); > > + if (pctldev && pctldev->desc->num_dt_params) { > > + BUG_ON(!pctldev->desc->params); > > No BUG_ON() as above. Sören -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/nomadik/pinctrl-abx500.c b/drivers/pinctrl/nomadik/pinctrl-abx500.c index 228972827132..f0fe6555a557 100644 --- a/drivers/pinctrl/nomadik/pinctrl-abx500.c +++ b/drivers/pinctrl/nomadik/pinctrl-abx500.c @@ -915,13 +915,13 @@ static int abx500_dt_subnode_to_map(struct pinctrl_dev *pctldev, } } - ret = pinconf_generic_parse_dt_config(np, &configs, &nconfigs); + ret = pinconf_generic_parse_dt_config(np, pctldev, &configs, &nconfigs); if (nconfigs) has_config = 1; np_config = of_parse_phandle(np, "ste,config", 0); if (np_config) { - ret = pinconf_generic_parse_dt_config(np_config, &configs, - &nconfigs); + ret = pinconf_generic_parse_dt_config(np_config, pctldev, + &configs, &nconfigs); if (ret) goto exit; has_config |= nconfigs; diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c index 1e782a0d6e48..23b5359a8c39 100644 --- a/drivers/pinctrl/pinconf-generic.c +++ b/drivers/pinctrl/pinconf-generic.c @@ -27,17 +27,6 @@ #include "pinctrl-utils.h" #ifdef CONFIG_DEBUG_FS - -struct pin_config_item { - const enum pin_config_param param; - const char * const display; - const char * const format; - bool has_arg; -}; - -#define PCONFDUMP(a, b, c, d) { .param = a, .display = b, .format = c, \ - .has_arg = d } - static const struct pin_config_item conf_items[] = { PCONFDUMP(PIN_CONFIG_BIAS_DISABLE, "input bias disabled", NULL, false), PCONFDUMP(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, "input bias high impedance", NULL, false), @@ -60,22 +49,25 @@ static const struct pin_config_item conf_items[] = { PCONFDUMP(PIN_CONFIG_OUTPUT, "pin output", "level", true), }; -void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev, - struct seq_file *s, unsigned pin) +static void _pinconf_generic_dump(struct pinctrl_dev *pctldev, + struct seq_file *s, const char *gname, + unsigned pin, + const struct pin_config_item *items, + int nitems) { - const struct pinconf_ops *ops = pctldev->desc->confops; int i; - if (!ops->is_generic) - return; - - for (i = 0; i < ARRAY_SIZE(conf_items); i++) { + for (i = 0; i < nitems; i++) { unsigned long config; int ret; /* We want to check out this parameter */ - config = pinconf_to_config_packed(conf_items[i].param, 0); - ret = pin_config_get_for_pin(pctldev, pin, &config); + config = pinconf_to_config_packed(items[i].param, 0); + if (gname) + ret = pin_config_group_get(dev_name(pctldev->dev), + gname, &config); + else + ret = pin_config_get_for_pin(pctldev, pin, &config); /* These are legal errors */ if (ret == -EINVAL || ret == -ENOTSUPP) continue; @@ -85,58 +77,50 @@ void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev, } /* Space between multiple configs */ seq_puts(s, " "); - seq_puts(s, conf_items[i].display); + seq_puts(s, items[i].display); /* Print unit if available */ - if (conf_items[i].has_arg) { + if (items[i].has_arg) { seq_printf(s, " (%u", pinconf_to_config_argument(config)); - if (conf_items[i].format) - seq_printf(s, " %s)", conf_items[i].format); + if (items[i].format) + seq_printf(s, " %s)", items[i].format); else seq_puts(s, ")"); } } } -void pinconf_generic_dump_group(struct pinctrl_dev *pctldev, - struct seq_file *s, const char *gname) +static void pinconf_generic_dump(struct pinctrl_dev *pctldev, + struct seq_file *s, const char *gname, + unsigned pin) { const struct pinconf_ops *ops = pctldev->desc->confops; - int i; if (!ops->is_generic) return; - for (i = 0; i < ARRAY_SIZE(conf_items); i++) { - unsigned long config; - int ret; - - /* We want to check out this parameter */ - config = pinconf_to_config_packed(conf_items[i].param, 0); - ret = pin_config_group_get(dev_name(pctldev->dev), gname, - &config); - /* These are legal errors */ - if (ret == -EINVAL || ret == -ENOTSUPP) - continue; - if (ret) { - seq_printf(s, "ERROR READING CONFIG SETTING %d ", i); - continue; - } - /* Space between multiple configs */ - seq_puts(s, " "); - seq_puts(s, conf_items[i].display); - /* Print unit if available */ - if (conf_items[i].has_arg) { - seq_printf(s, " (%u", - pinconf_to_config_argument(config)); - if (conf_items[i].format) - seq_printf(s, " %s)", conf_items[i].format); - else - seq_puts(s, ")"); - } + _pinconf_generic_dump(pctldev, s, gname, pin, + conf_items, ARRAY_SIZE(conf_items)); + if (pctldev->desc->num_dt_params) { + BUG_ON(!pctldev->desc->conf_items); + _pinconf_generic_dump(pctldev, s, gname, pin, + pctldev->desc->conf_items, + pctldev->desc->num_dt_params); } } +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev, + struct seq_file *s, unsigned pin) +{ + pinconf_generic_dump(pctldev, s, NULL, pin); +} + +void pinconf_generic_dump_group(struct pinctrl_dev *pctldev, + struct seq_file *s, const char *gname) +{ + pinconf_generic_dump(pctldev, s, gname, 0); +} + void pinconf_generic_dump_config(struct pinctrl_dev *pctldev, struct seq_file *s, unsigned long config) { @@ -148,17 +132,22 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev, seq_printf(s, "%s: 0x%x", conf_items[i].display, pinconf_to_config_argument(config)); } + + if (!pctldev->desc->num_dt_params) + return; + + BUG_ON(!pctldev->desc->conf_items); + for (i = 0; i < pctldev->desc->num_dt_params; i++) { + if (pinconf_to_config_param(config) != pctldev->desc->conf_items[i].param) + continue; + seq_printf(s, "%s: 0x%x", pctldev->desc->conf_items[i].display, + pinconf_to_config_argument(config)); + } } EXPORT_SYMBOL_GPL(pinconf_generic_dump_config); #endif #ifdef CONFIG_OF -struct pinconf_generic_dt_params { - const char * const property; - enum pin_config_param param; - u32 default_value; -}; - static const struct pinconf_generic_dt_params dt_params[] = { { "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 }, { "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 }, @@ -183,6 +172,35 @@ static const struct pinconf_generic_dt_params dt_params[] = { { "slew-rate", PIN_CONFIG_SLEW_RATE, 0}, }; +static void _parse_dt_cfg(struct device_node *np, + const struct pinconf_generic_dt_params *params, + unsigned int count, + unsigned long *cfg, + unsigned int *ncfg) +{ + int i; + + for (i = 0; i < count; i++) { + u32 val; + int ret; + const struct pinconf_generic_dt_params *par = ¶ms[i]; + + ret = of_property_read_u32(np, par->property, &val); + + /* property not found */ + if (ret == -EINVAL) + continue; + + /* use default value, when no value is specified */ + if (ret) + val = par->default_value; + + pr_debug("found %s with value %u\n", par->property, val); + cfg[*ncfg] = pinconf_to_config_packed(par->param, val); + (*ncfg)++; + } +} + /** * pinconf_generic_parse_dt_config() * parse the config properties into generic pinconfig values. @@ -191,38 +209,30 @@ static const struct pinconf_generic_dt_params dt_params[] = { * @nconfigs: umber of configurations */ int pinconf_generic_parse_dt_config(struct device_node *np, + struct pinctrl_dev *pctldev, unsigned long **configs, unsigned int *nconfigs) { unsigned long *cfg; - unsigned int ncfg = 0; + unsigned int max_cfg, ncfg = 0; int ret; - int i; - u32 val; if (!np) return -EINVAL; /* allocate a temporary array big enough to hold one of each option */ - cfg = kzalloc(sizeof(*cfg) * ARRAY_SIZE(dt_params), GFP_KERNEL); + max_cfg = ARRAY_SIZE(dt_params); + if (pctldev) + max_cfg += pctldev->desc->num_dt_params; + cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL); if (!cfg) return -ENOMEM; - for (i = 0; i < ARRAY_SIZE(dt_params); i++) { - const struct pinconf_generic_dt_params *par = &dt_params[i]; - ret = of_property_read_u32(np, par->property, &val); - - /* property not found */ - if (ret == -EINVAL) - continue; - - /* use default value, when no value is specified */ - if (ret) - val = par->default_value; - - pr_debug("found %s with value %u\n", par->property, val); - cfg[ncfg] = pinconf_to_config_packed(par->param, val); - ncfg++; + _parse_dt_cfg(np, dt_params, ARRAY_SIZE(dt_params), cfg, &ncfg); + if (pctldev && pctldev->desc->num_dt_params) { + BUG_ON(!pctldev->desc->params); + _parse_dt_cfg(np, pctldev->desc->params, + pctldev->desc->num_dt_params, cfg, &ncfg); } ret = 0; @@ -274,7 +284,8 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, function = NULL; } - ret = pinconf_generic_parse_dt_config(np, &configs, &num_configs); + ret = pinconf_generic_parse_dt_config(np, pctldev, &configs, + &num_configs); if (ret < 0) { dev_err(dev, "could not parse node property\n"); return ret; diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h index a4a5417e1413..6a6c55864672 100644 --- a/drivers/pinctrl/pinconf.h +++ b/drivers/pinctrl/pinconf.h @@ -126,6 +126,7 @@ static inline void pinconf_generic_dump_config(struct pinctrl_dev *pctldev, #if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_OF) int pinconf_generic_parse_dt_config(struct device_node *np, + struct pinctrl_dev *pctldev, unsigned long **configs, unsigned int *nconfigs); #endif diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c index 016f4578e494..0e05febe80ba 100644 --- a/drivers/pinctrl/pinctrl-rockchip.c +++ b/drivers/pinctrl/pinctrl-rockchip.c @@ -1126,7 +1126,7 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np, return -EINVAL; np_config = of_find_node_by_phandle(be32_to_cpup(phandle)); - ret = pinconf_generic_parse_dt_config(np_config, + ret = pinconf_generic_parse_dt_config(np_config, NULL, &grp->data[j].configs, &grp->data[j].nconfigs); if (ret) return ret; diff --git a/drivers/pinctrl/pinctrl-tz1090-pdc.c b/drivers/pinctrl/pinctrl-tz1090-pdc.c index 3bb6a3b78864..0e3576ba1a7c 100644 --- a/drivers/pinctrl/pinctrl-tz1090-pdc.c +++ b/drivers/pinctrl/pinctrl-tz1090-pdc.c @@ -415,7 +415,7 @@ static int tz1090_pdc_pinctrl_dt_subnode_to_map(struct device *dev, function = NULL; } - ret = pinconf_generic_parse_dt_config(np, &configs, &num_configs); + ret = pinconf_generic_parse_dt_config(np, NULL, &configs, &num_configs); if (ret) return ret; diff --git a/drivers/pinctrl/pinctrl-tz1090.c b/drivers/pinctrl/pinctrl-tz1090.c index 48d36413b99f..0b36566db451 100644 --- a/drivers/pinctrl/pinctrl-tz1090.c +++ b/drivers/pinctrl/pinctrl-tz1090.c @@ -1131,7 +1131,7 @@ static int tz1090_pinctrl_dt_subnode_to_map(struct device *dev, function = NULL; } - ret = pinconf_generic_parse_dt_config(np, &configs, &num_configs); + ret = pinconf_generic_parse_dt_config(np, NULL, &configs, &num_configs); if (ret) return ret; diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c index 910deaefa0ac..072e7c62cab7 100644 --- a/drivers/pinctrl/sh-pfc/pinctrl.c +++ b/drivers/pinctrl/sh-pfc/pinctrl.c @@ -122,7 +122,7 @@ static int sh_pfc_dt_subnode_to_map(struct device *dev, struct device_node *np, return ret; } - ret = pinconf_generic_parse_dt_config(np, &configs, &num_configs); + ret = pinconf_generic_parse_dt_config(np, NULL, &configs, &num_configs); if (ret < 0) return ret; diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h index b6dedfbfce69..5731703b27c2 100644 --- a/include/linux/pinctrl/pinconf-generic.h +++ b/include/linux/pinctrl/pinconf-generic.h @@ -115,6 +115,18 @@ enum pin_config_param { PIN_CONFIG_END = 0x7FFF, }; +#ifdef CONFIG_DEBUG_FS +#define PCONFDUMP(a, b, c, d) { .param = a, .display = b, .format = c, \ + .has_arg = d } + +struct pin_config_item { + const enum pin_config_param param; + const char * const display; + const char * const format; + bool has_arg; +}; +#endif /* CONFIG_DEBUG_FS */ + /* * Helpful configuration macro to be used in tables etc. */ @@ -150,6 +162,12 @@ static inline unsigned long pinconf_to_config_packed(enum pin_config_param param struct pinctrl_dev; struct pinctrl_map; +struct pinconf_generic_dt_params { + const char * const property; + enum pin_config_param param; + u32 default_value; +}; + int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, struct device_node *np, struct pinctrl_map **map, unsigned *reserved_maps, unsigned *num_maps, diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h index cc8e1aff0e28..36f749775342 100644 --- a/include/linux/pinctrl/pinctrl.h +++ b/include/linux/pinctrl/pinctrl.h @@ -24,6 +24,7 @@ struct pinctrl_dev; struct pinctrl_map; struct pinmux_ops; struct pinconf_ops; +struct pin_config_item; struct gpio_chip; struct device_node; @@ -117,6 +118,8 @@ struct pinctrl_ops { * @confops: pin config operations vtable, if you support pin configuration in * your driver * @owner: module providing the pin controller, used for refcounting + * @num_dt_params: Number of driver-specifid DT parameters + * @params: List of DT parameters */ struct pinctrl_desc { const char *name; @@ -126,6 +129,11 @@ struct pinctrl_desc { const struct pinmux_ops *pmxops; const struct pinconf_ops *confops; struct module *owner; +#if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_OF) + unsigned int num_dt_params; + const struct pinconf_generic_dt_params *params; + const struct pin_config_item *conf_items; +#endif }; /* External interface to pin controller */
Additionally to the generic DT parameters, allow drivers to provide driver-specific DT parameters to be used with the generic parser infrastructure. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- drivers/pinctrl/nomadik/pinctrl-abx500.c | 6 +- drivers/pinctrl/pinconf-generic.c | 171 ++++++++++++++++--------------- drivers/pinctrl/pinconf.h | 1 + drivers/pinctrl/pinctrl-rockchip.c | 2 +- drivers/pinctrl/pinctrl-tz1090-pdc.c | 2 +- drivers/pinctrl/pinctrl-tz1090.c | 2 +- drivers/pinctrl/sh-pfc/pinctrl.c | 2 +- include/linux/pinctrl/pinconf-generic.h | 18 ++++ include/linux/pinctrl/pinctrl.h | 8 ++ 9 files changed, 125 insertions(+), 87 deletions(-)