| Message ID | 1397135274-10764-2-git-send-email-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/10/2014 03:07 PM, Antoine Ténart wrote: > When using a group only pinctrl driver, which does not have any > information on the pins it is useless to define a get_group_pins > always returning an empty list of pins. > > When not using get_group_pin[1], a driver must implement it so > pins = NULL and num_pins = 0. This patch makes it the default > behaviour if not defined in the pinctrl driver when used in > pinmux enable and disable funtions and in pinctrl_groups_show. > > It also adds a check in pinctrl_get_group_pins and return -EINVAL if > not defined. This function is called in the gpiolib when adding when > pingroup range. It cannot be used if no group is defined, so this seams > reasonable. I don't have a strong opinion about making pins optional for pinctrl drivers. I'll leave it for Linus to decide on it. But have you checked all the other places pins are referred to? One place that jumps into my mind would be the DEBUGFS files... Sebastian > [1] get_group_pin(struct pinctrl_dev *pctldev, > unsigned selector, > const unsigned **pins, > unsigned *num_pins); > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > --- > drivers/pinctrl/core.c | 17 ++++++++++------- > drivers/pinctrl/pinmux.c | 23 +++++++++++++---------- > 2 files changed, 23 insertions(+), 17 deletions(-) > > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index c0fe6091566a..e09474ecde23 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -468,6 +468,9 @@ int pinctrl_get_group_pins(struct pinctrl_dev *pctldev, const char *pin_group, > const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; > int gs; > > + if (!pctlops->get_group_pins) > + return -EINVAL; > + > gs = pinctrl_get_group_selector(pctldev, pin_group); > if (gs < 0) > return gs; > @@ -1362,15 +1365,16 @@ static int pinctrl_groups_show(struct seq_file *s, void *what) > > seq_puts(s, "registered pin groups:\n"); > while (selector < ngroups) { > - const unsigned *pins; > - unsigned num_pins; > + const unsigned *pins = NULL; > + unsigned num_pins = 0; > const char *gname = ops->get_group_name(pctldev, selector); > const char *pname; > - int ret; > + int ret = 0; > int i; > > - ret = ops->get_group_pins(pctldev, selector, > - &pins, &num_pins); > + if (ops->get_group_pins) > + ret = ops->get_group_pins(pctldev, selector, > + &pins, &num_pins); > if (ret) > seq_printf(s, "%s [ERROR GETTING PINS]\n", > gname); > @@ -1694,8 +1698,7 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev) > > if (!ops || > !ops->get_groups_count || > - !ops->get_group_name || > - !ops->get_group_pins) > + !ops->get_group_name) > return -EINVAL; > > if (ops->dt_node_to_map && !ops->dt_free_map) > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c > index 9248ce4efed4..051e8592990e 100644 > --- a/drivers/pinctrl/pinmux.c > +++ b/drivers/pinctrl/pinmux.c > @@ -391,14 +391,16 @@ int pinmux_enable_setting(struct pinctrl_setting const *setting) > struct pinctrl_dev *pctldev = setting->pctldev; > const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; > const struct pinmux_ops *ops = pctldev->desc->pmxops; > - int ret; > - const unsigned *pins; > - unsigned num_pins; > + int ret = 0; > + const unsigned *pins = NULL; > + unsigned num_pins = 0; > int i; > struct pin_desc *desc; > > - ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, > - &pins, &num_pins); > + if (pctlops->get_group_pins) > + ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, > + &pins, &num_pins); > + > if (ret) { > const char *gname; > > @@ -470,14 +472,15 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) > struct pinctrl_dev *pctldev = setting->pctldev; > const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; > const struct pinmux_ops *ops = pctldev->desc->pmxops; > - int ret; > - const unsigned *pins; > - unsigned num_pins; > + int ret = 0; > + const unsigned *pins = NULL; > + unsigned num_pins = 0; > int i; > struct pin_desc *desc; > > - ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, > - &pins, &num_pins); > + if (pctlops->get_group_pins) > + ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, > + &pins, &num_pins); > if (ret) { > const char *gname; > >
On Thu, Apr 10, 2014 at 3:07 PM, Antoine Ténart <antoine.tenart@free-electrons.com> wrote: > When using a group only pinctrl driver, which does not have any > information on the pins it is useless to define a get_group_pins > always returning an empty list of pins. > > When not using get_group_pin[1], a driver must implement it so > pins = NULL and num_pins = 0. This patch makes it the default > behaviour if not defined in the pinctrl driver when used in > pinmux enable and disable funtions and in pinctrl_groups_show. > > It also adds a check in pinctrl_get_group_pins and return -EINVAL if > not defined. This function is called in the gpiolib when adding when > pingroup range. It cannot be used if no group is defined, so this seams > reasonable. > > [1] get_group_pin(struct pinctrl_dev *pctldev, > unsigned selector, > const unsigned **pins, > unsigned *num_pins); > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> OK makes perfect sense. Patch applied. Can you please check the Documentation/pinctrl.txt to see if anything needs to be updated due to this? Yours, Linus Walleij
Linus, On Tue, Apr 22, 2014 at 02:48:04PM +0200, Linus Walleij wrote: > On Thu, Apr 10, 2014 at 3:07 PM, Antoine Ténart > <antoine.tenart@free-electrons.com> wrote: > > > When using a group only pinctrl driver, which does not have any > > information on the pins it is useless to define a get_group_pins > > always returning an empty list of pins. > > > > When not using get_group_pin[1], a driver must implement it so > > pins = NULL and num_pins = 0. This patch makes it the default > > behaviour if not defined in the pinctrl driver when used in > > pinmux enable and disable funtions and in pinctrl_groups_show. > > > > It also adds a check in pinctrl_get_group_pins and return -EINVAL if > > not defined. This function is called in the gpiolib when adding when > > pingroup range. It cannot be used if no group is defined, so this seams > > reasonable. > > > > [1] get_group_pin(struct pinctrl_dev *pctldev, > > unsigned selector, > > const unsigned **pins, > > unsigned *num_pins); > > > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > > OK makes perfect sense. > > Patch applied. Thanks! > Can you please check the Documentation/pinctrl.txt to see > if anything needs to be updated due to this? I just checked and did not see needed updates due to this patch. Antoine
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index c0fe6091566a..e09474ecde23 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -468,6 +468,9 @@ int pinctrl_get_group_pins(struct pinctrl_dev *pctldev, const char *pin_group, const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; int gs; + if (!pctlops->get_group_pins) + return -EINVAL; + gs = pinctrl_get_group_selector(pctldev, pin_group); if (gs < 0) return gs; @@ -1362,15 +1365,16 @@ static int pinctrl_groups_show(struct seq_file *s, void *what) seq_puts(s, "registered pin groups:\n"); while (selector < ngroups) { - const unsigned *pins; - unsigned num_pins; + const unsigned *pins = NULL; + unsigned num_pins = 0; const char *gname = ops->get_group_name(pctldev, selector); const char *pname; - int ret; + int ret = 0; int i; - ret = ops->get_group_pins(pctldev, selector, - &pins, &num_pins); + if (ops->get_group_pins) + ret = ops->get_group_pins(pctldev, selector, + &pins, &num_pins); if (ret) seq_printf(s, "%s [ERROR GETTING PINS]\n", gname); @@ -1694,8 +1698,7 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev) if (!ops || !ops->get_groups_count || - !ops->get_group_name || - !ops->get_group_pins) + !ops->get_group_name) return -EINVAL; if (ops->dt_node_to_map && !ops->dt_free_map) diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 9248ce4efed4..051e8592990e 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -391,14 +391,16 @@ int pinmux_enable_setting(struct pinctrl_setting const *setting) struct pinctrl_dev *pctldev = setting->pctldev; const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; const struct pinmux_ops *ops = pctldev->desc->pmxops; - int ret; - const unsigned *pins; - unsigned num_pins; + int ret = 0; + const unsigned *pins = NULL; + unsigned num_pins = 0; int i; struct pin_desc *desc; - ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, - &pins, &num_pins); + if (pctlops->get_group_pins) + ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, + &pins, &num_pins); + if (ret) { const char *gname; @@ -470,14 +472,15 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) struct pinctrl_dev *pctldev = setting->pctldev; const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; const struct pinmux_ops *ops = pctldev->desc->pmxops; - int ret; - const unsigned *pins; - unsigned num_pins; + int ret = 0; + const unsigned *pins = NULL; + unsigned num_pins = 0; int i; struct pin_desc *desc; - ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, - &pins, &num_pins); + if (pctlops->get_group_pins) + ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, + &pins, &num_pins); if (ret) { const char *gname;
When using a group only pinctrl driver, which does not have any information on the pins it is useless to define a get_group_pins always returning an empty list of pins. When not using get_group_pin[1], a driver must implement it so pins = NULL and num_pins = 0. This patch makes it the default behaviour if not defined in the pinctrl driver when used in pinmux enable and disable funtions and in pinctrl_groups_show. It also adds a check in pinctrl_get_group_pins and return -EINVAL if not defined. This function is called in the gpiolib when adding when pingroup range. It cannot be used if no group is defined, so this seams reasonable. [1] get_group_pin(struct pinctrl_dev *pctldev, unsigned selector, const unsigned **pins, unsigned *num_pins); Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> --- drivers/pinctrl/core.c | 17 ++++++++++------- drivers/pinctrl/pinmux.c | 23 +++++++++++++---------- 2 files changed, 23 insertions(+), 17 deletions(-)