| Message ID | 1417137993-8337-8-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2014-11-27 at 17:26 -0800, Soren Brinkmann wrote: > Instead of the driver caring about implementation details like device > tree, just provide information about driver specific pinconf parameters > to pinconf-generic which takes care of parsing the DT. > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com> > --- > This is compile tested only. So, it's likely that it needs more tweaking > to make it actually work on HW. But it illustrates the potential > benefits of the pinconf-generic changes in this series. > --- > drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 125 +++---------------------------- > 1 file changed, 11 insertions(+), 114 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > index b863b5080890..2db85e53ef73 100644 > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > @@ -131,14 +131,14 @@ struct pmic_gpio_state { > struct gpio_chip chip; > }; > > -struct pmic_gpio_bindings { > - const char*property; > - unsigned param; > +static const struct pinconf_generic_dt_params pmic_gpio_bindings[] = { > + {"qcom,pull-up-strength",PMIC_GPIO_CONF_PULL_UP,0}, > + {"qcom,drive-strength",PMIC_GPIO_CONF_STRENGTH,0}, > }; > > -static struct pmic_gpio_bindings pmic_gpio_bindings[] = { > - {"qcom,pull-up-strength",PMIC_GPIO_CONF_PULL_UP}, > - {"qcom,drive-strength",PMIC_GPIO_CONF_STRENGTH}, > +static const struct pin_config_item pmic_conf_items[] = { > + PCONFDUMP(PMIC_GPIO_CONF_PULL_UP, "pull-up-strength", NULL, true), s/pull-up-strength/qcom,pull-up-strength/ > + PCONFDUMP(PMIC_GPIO_CONF_STRENGTH, "drive-strength", NULL, true), > }; > I am not happy that we have to define the same think two times. Regards, Ivan
On Wed, 2014-12-03 at 03:03PM +0200, Ivan T. Ivanov wrote: > > On Thu, 2014-11-27 at 17:26 -0800, Soren Brinkmann wrote: > > Instead of the driver caring about implementation details like device > > tree, just provide information about driver specific pinconf parameters > > to pinconf-generic which takes care of parsing the DT. > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com> > > --- > > This is compile tested only. So, it's likely that it needs more tweaking > > to make it actually work on HW. But it illustrates the potential > > benefits of the pinconf-generic changes in this series. > > --- > > drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 125 +++---------------------------- > > 1 file changed, 11 insertions(+), 114 deletions(-) > > > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > index b863b5080890..2db85e53ef73 100644 > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > @@ -131,14 +131,14 @@ struct pmic_gpio_state { > > struct gpio_chip chip; > > }; > > > > -struct pmic_gpio_bindings { > > - const char*property; > > - unsigned param; > > +static const struct pinconf_generic_dt_params pmic_gpio_bindings[] = { > > + {"qcom,pull-up-strength",PMIC_GPIO_CONF_PULL_UP,0}, > > + {"qcom,drive-strength",PMIC_GPIO_CONF_STRENGTH,0}, > > }; > > > > -static struct pmic_gpio_bindings pmic_gpio_bindings[] = { > > - {"qcom,pull-up-strength",PMIC_GPIO_CONF_PULL_UP}, > > - {"qcom,drive-strength",PMIC_GPIO_CONF_STRENGTH}, > > +static const struct pin_config_item pmic_conf_items[] = { > > + PCONFDUMP(PMIC_GPIO_CONF_PULL_UP, "pull-up-strength", NULL, true), > > s/pull-up-strength/qcom,pull-up-strength/ This is used for the debugfs output. I don't think you want the qcom prefix here, do you? > > > + PCONFDUMP(PMIC_GPIO_CONF_STRENGTH, "drive-strength", NULL, true), > > }; > > > > > > I am not happy that we have to define the same think two times. It's not really the same thing. One is the actual DT binding the other how the property is displayed in debugfs. Two different things. It's how the core does it. That's not new. Sören
On Wed, 2014-12-03 at 09:38 -0800, Sören Brinkmann wrote: > On Wed, 2014-12-03 at 03:03PM +0200, Ivan T. Ivanov wrote: > > On Thu, 2014-11-27 at 17:26 -0800, Soren Brinkmann wrote: > > > Instead of the driver caring about implementation details like device > > > tree, just provide information about driver specific pinconf parameters > > > to pinconf-generic which takes care of parsing the DT. > > > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com> > > > --- > > > This is compile tested only. So, it's likely that it needs more tweaking > > > to make it actually work on HW. But it illustrates the potential > > > benefits of the pinconf-generic changes in this series. > > > --- > > > drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 125 +++---------------------------- > > > 1 file changed, 11 insertions(+), 114 deletions(-) > > > > > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi- > > > gpio.c > > > index b863b5080890..2db85e53ef73 100644 > > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > > @@ -131,14 +131,14 @@ struct pmic_gpio_state { > > > struct gpio_chip chip; > > > }; > > > > > > -struct pmic_gpio_bindings { > > > - const char*property; > > > - unsigned param; > > > +static const struct pinconf_generic_dt_params pmic_gpio_bindings[] = { > > > + {"qcom,pull-up-strength",PMIC_GPIO_CONF_PULL_UP,0}, > > > + {"qcom,drive-strength",PMIC_GPIO_CONF_STRENGTH,0}, > > > }; > > > > > > -static struct pmic_gpio_bindings pmic_gpio_bindings[] = { > > > - {"qcom,pull-up-strength",PMIC_GPIO_CONF_PULL_UP}, > > > - {"qcom,drive-strength",PMIC_GPIO_CONF_STRENGTH}, > > > +static const struct pin_config_item pmic_conf_items[] = { > > > + PCONFDUMP(PMIC_GPIO_CONF_PULL_UP, "pull-up-strength", NULL, true), > > > > s/pull-up-strength/qcom,pull-up-strength/ > > This is used for the debugfs output. I don't think you want the qcom > prefix here, do you? Then at lest lets make it "pull up strength". > > > > + PCONFDUMP(PMIC_GPIO_CONF_STRENGTH, "drive-strength", NULL, true), > > > }; > > > > > > > > > > > I am not happy that we have to define the same think two times. > > It's not really the same thing. One is the actual DT binding the other > how the property is displayed in debugfs. Two different things. It's how > the core does it. That's not new. This didn't make me more happy :-). Could both be merged? Regards, Ivan
On Thu, 2014-12-04 at 11:30AM +0200, Ivan T. Ivanov wrote: > > On Wed, 2014-12-03 at 09:38 -0800, Sören Brinkmann wrote: > > On Wed, 2014-12-03 at 03:03PM +0200, Ivan T. Ivanov wrote: > > > On Thu, 2014-11-27 at 17:26 -0800, Soren Brinkmann wrote: > > > > Instead of the driver caring about implementation details like device > > > > tree, just provide information about driver specific pinconf parameters > > > > to pinconf-generic which takes care of parsing the DT. > > > > > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com> > > > > --- > > > > This is compile tested only. So, it's likely that it needs more tweaking > > > > to make it actually work on HW. But it illustrates the potential > > > > benefits of the pinconf-generic changes in this series. > > > > --- > > > > drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 125 +++---------------------------- > > > > 1 file changed, 11 insertions(+), 114 deletions(-) > > > > > > > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi- > > > > gpio.c > > > > index b863b5080890..2db85e53ef73 100644 > > > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > > > @@ -131,14 +131,14 @@ struct pmic_gpio_state { > > > > struct gpio_chip chip; > > > > }; > > > > > > > > -struct pmic_gpio_bindings { > > > > - const char*property; > > > > - unsigned param; > > > > +static const struct pinconf_generic_dt_params pmic_gpio_bindings[] = { > > > > + {"qcom,pull-up-strength",PMIC_GPIO_CONF_PULL_UP,0}, > > > > + {"qcom,drive-strength",PMIC_GPIO_CONF_STRENGTH,0}, > > > > }; > > > > > > > > -static struct pmic_gpio_bindings pmic_gpio_bindings[] = { > > > > - {"qcom,pull-up-strength",PMIC_GPIO_CONF_PULL_UP}, > > > > - {"qcom,drive-strength",PMIC_GPIO_CONF_STRENGTH}, > > > > +static const struct pin_config_item pmic_conf_items[] = { > > > > + PCONFDUMP(PMIC_GPIO_CONF_PULL_UP, "pull-up-strength", NULL, true), > > > > > > s/pull-up-strength/qcom,pull-up-strength/ > > > > This is used for the debugfs output. I don't think you want the qcom > > prefix here, do you? > > Then at lest lets make it "pull up strength". Ok. Will change it. > > > > > > > + PCONFDUMP(PMIC_GPIO_CONF_STRENGTH, "drive-strength", NULL, true), > > > > }; > > > > > > > > > > > > > > > > I am not happy that we have to define the same think two times. > > > > It's not really the same thing. One is the actual DT binding the other > > how the property is displayed in debugfs. Two different things. It's how > > the core does it. That's not new. > > This didn't make me more happy :-). Could both be merged? That's definitely beyond the scope of this series. Soren
On Thu, 2014-11-27 at 17:26 -0800, Soren Brinkmann wrote: > > static const struct pinconf_ops pmic_gpio_pinconf_ops = { > + .is_generic= true, > .pin_config_group_get= pmic_gpio_config_get, > .pin_config_group_set= pmic_gpio_config_set, > .pin_config_group_dbg_show= pmic_gpio_config_dbg_show, > @@ -848,6 +742,9 @@ static int pmic_gpio_probe(struct platform_device *pdev) > pctrldesc->name = dev_name(dev); > pctrldesc->pins = pindesc; > pctrldesc->npins = npins; > + pctrldesc->num_dt_params = ARRAY_SIZE(pmic_gpio_bindings); > + pctrldesc->params = pmic_gpio_bindings; > + pctrldesc->conf_items = pmic_conf_items; > What will happen if number of conf_items is less than number of params? Ivan
On Fri, 2014-12-05 at 09:59AM +0200, Ivan T. Ivanov wrote: > > On Thu, 2014-11-27 at 17:26 -0800, Soren Brinkmann wrote: > > > > static const struct pinconf_ops pmic_gpio_pinconf_ops = { > > + .is_generic= true, > > .pin_config_group_get= pmic_gpio_config_get, > > .pin_config_group_set= pmic_gpio_config_set, > > .pin_config_group_dbg_show= pmic_gpio_config_dbg_show, > > @@ -848,6 +742,9 @@ static int pmic_gpio_probe(struct platform_device *pdev) > > pctrldesc->name = dev_name(dev); > > pctrldesc->pins = pindesc; > > pctrldesc->npins = npins; > > + pctrldesc->num_dt_params = ARRAY_SIZE(pmic_gpio_bindings); > > + pctrldesc->params = pmic_gpio_bindings; > > + pctrldesc->conf_items = pmic_conf_items; > > > > What will happen if number of conf_items is less than number of params? I suppose bad things :) You're right, that should probably be checked somewhere. Let me see whether I find a good place to put such a check. Thanks, Sören
On Fri, 2014-12-05 at 09:08AM -0800, Sören Brinkmann wrote: > On Fri, 2014-12-05 at 09:59AM +0200, Ivan T. Ivanov wrote: > > > > On Thu, 2014-11-27 at 17:26 -0800, Soren Brinkmann wrote: > > > > > > static const struct pinconf_ops pmic_gpio_pinconf_ops = { > > > + .is_generic= true, > > > .pin_config_group_get= pmic_gpio_config_get, > > > .pin_config_group_set= pmic_gpio_config_set, > > > .pin_config_group_dbg_show= pmic_gpio_config_dbg_show, > > > @@ -848,6 +742,9 @@ static int pmic_gpio_probe(struct platform_device *pdev) > > > pctrldesc->name = dev_name(dev); > > > pctrldesc->pins = pindesc; > > > pctrldesc->npins = npins; > > > + pctrldesc->num_dt_params = ARRAY_SIZE(pmic_gpio_bindings); > > > + pctrldesc->params = pmic_gpio_bindings; > > > + pctrldesc->conf_items = pmic_conf_items; > > > > > > > What will happen if number of conf_items is less than number of params? > > I suppose bad things :) You're right, that should probably be checked > somewhere. Let me see whether I find a good place to put such a check. I looked at merging these arrays. Unfortunately that doesn't work that easy. There are two arrays. The first one establishes a mapping from strings used in DT to specify pinconf parameters to a pinconf parameter. This array has some entries which map to the same pinconf parameter. E.g. 'input-enable' and 'input-disable', the only difference in those entries is the resulting value of the parameter. The second array establishes a debugfs representation for each pinconf parameter. So, for the core parameters, the number of entries in these arrays differ. For this patch, I was assuming that these arrays match in size. I'd keep it the way it is now and if it turns out that this is not sufficient, passing the size of the second array too is an easy addition. Thanks, Sören
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c index b863b5080890..2db85e53ef73 100644 --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c @@ -131,14 +131,14 @@ struct pmic_gpio_state { struct gpio_chip chip; }; -struct pmic_gpio_bindings { - const char *property; - unsigned param; +static const struct pinconf_generic_dt_params pmic_gpio_bindings[] = { + {"qcom,pull-up-strength", PMIC_GPIO_CONF_PULL_UP, 0}, + {"qcom,drive-strength", PMIC_GPIO_CONF_STRENGTH, 0}, }; -static struct pmic_gpio_bindings pmic_gpio_bindings[] = { - {"qcom,pull-up-strength", PMIC_GPIO_CONF_PULL_UP}, - {"qcom,drive-strength", PMIC_GPIO_CONF_STRENGTH}, +static const struct pin_config_item pmic_conf_items[] = { + PCONFDUMP(PMIC_GPIO_CONF_PULL_UP, "pull-up-strength", NULL, true), + PCONFDUMP(PMIC_GPIO_CONF_STRENGTH, "drive-strength", NULL, true), }; static const char *const pmic_gpio_groups[] = { @@ -209,118 +209,11 @@ static int pmic_gpio_get_group_pins(struct pinctrl_dev *pctldev, unsigned pin, return 0; } -static int pmic_gpio_parse_dt_config(struct device_node *np, - struct pinctrl_dev *pctldev, - unsigned long **configs, - unsigned int *nconfs) -{ - struct pmic_gpio_bindings *par; - unsigned long cfg; - int ret, i; - u32 val; - - for (i = 0; i < ARRAY_SIZE(pmic_gpio_bindings); i++) { - par = &pmic_gpio_bindings[i]; - ret = of_property_read_u32(np, par->property, &val); - - /* property not found */ - if (ret == -EINVAL) - continue; - - /* use zero as default value */ - if (ret) - val = 0; - - dev_dbg(pctldev->dev, "found %s with value %u\n", - par->property, val); - - cfg = pinconf_to_config_packed(par->param, val); - - ret = pinctrl_utils_add_config(pctldev, configs, nconfs, cfg); - if (ret) - return ret; - } - - return 0; -} - -static int pmic_gpio_dt_subnode_to_map(struct pinctrl_dev *pctldev, - struct device_node *np, - struct pinctrl_map **map, - unsigned *reserv, unsigned *nmaps, - enum pinctrl_map_type type) -{ - unsigned long *configs = NULL; - unsigned nconfs = 0; - struct property *prop; - const char *group; - int ret; - - ret = pmic_gpio_parse_dt_config(np, pctldev, &configs, &nconfs); - if (ret < 0) - return ret; - - if (!nconfs) - return 0; - - ret = of_property_count_strings(np, "pins"); - if (ret < 0) - goto exit; - - ret = pinctrl_utils_reserve_map(pctldev, map, reserv, nmaps, ret); - if (ret < 0) - goto exit; - - of_property_for_each_string(np, "pins", prop, group) { - ret = pinctrl_utils_add_map_configs(pctldev, map, - reserv, nmaps, group, - configs, nconfs, type); - if (ret < 0) - break; - } -exit: - kfree(configs); - return ret; -} - -static int pmic_gpio_dt_node_to_map(struct pinctrl_dev *pctldev, - struct device_node *np_config, - struct pinctrl_map **map, unsigned *nmaps) -{ - enum pinctrl_map_type type; - struct device_node *np; - unsigned reserv; - int ret; - - ret = 0; - *map = NULL; - *nmaps = 0; - reserv = 0; - type = PIN_MAP_TYPE_CONFIGS_GROUP; - - 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; - } - - if (ret < 0) - pinctrl_utils_dt_free_map(pctldev, *map, *nmaps); - - return ret; -} - static const struct pinctrl_ops pmic_gpio_pinctrl_ops = { .get_groups_count = pmic_gpio_get_groups_count, .get_group_name = pmic_gpio_get_group_name, .get_group_pins = pmic_gpio_get_group_pins, - .dt_node_to_map = pmic_gpio_dt_node_to_map, + .dt_node_to_map = pinconf_generic_dt_node_to_map_group, .dt_free_map = pinctrl_utils_dt_free_map, }; @@ -590,6 +483,7 @@ static void pmic_gpio_config_dbg_show(struct pinctrl_dev *pctldev, } static const struct pinconf_ops pmic_gpio_pinconf_ops = { + .is_generic = true, .pin_config_group_get = pmic_gpio_config_get, .pin_config_group_set = pmic_gpio_config_set, .pin_config_group_dbg_show = pmic_gpio_config_dbg_show, @@ -848,6 +742,9 @@ static int pmic_gpio_probe(struct platform_device *pdev) pctrldesc->name = dev_name(dev); pctrldesc->pins = pindesc; pctrldesc->npins = npins; + pctrldesc->num_dt_params = ARRAY_SIZE(pmic_gpio_bindings); + pctrldesc->params = pmic_gpio_bindings; + pctrldesc->conf_items = pmic_conf_items; for (i = 0; i < npins; i++, pindesc++) { pad = &pads[i];
Instead of the driver caring about implementation details like device tree, just provide information about driver specific pinconf parameters to pinconf-generic which takes care of parsing the DT. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- This is compile tested only. So, it's likely that it needs more tweaking to make it actually work on HW. But it illustrates the potential benefits of the pinconf-generic changes in this series. --- drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 125 +++---------------------------- 1 file changed, 11 insertions(+), 114 deletions(-)