Patchwork [RESEND,1/5] pinctrl: allows not to define the get_group_pins operation

login
register
mail settings
Submitter Antoine Ténart
Date April 10, 2014, 1:07 p.m.
Message ID <1397135274-10764-2-git-send-email-antoine.tenart@free-electrons.com>
Download mbox | patch
Permalink /patch/338267/
State New
Headers show

Comments

Antoine Ténart - April 10, 2014, 1:07 p.m.
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(-)
Sebastian Hesselbarth - April 11, 2014, 9:21 a.m.
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;
>
>
Linus Walleij - April 22, 2014, 12:48 p.m.
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
Antoine Ténart - April 22, 2014, 3:58 p.m.
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

Patch

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;