[PATCH 1/2] pinmux: Remove double pin validity check.

[PATCH 1/2] pinmux: Remove double pin validity check.

[Marek Belisko]

Function pin_is_valid just call pin_desc_get which is in pin_request
call some line below. Remove pin_is_valid() check.

Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com>
---
 drivers/pinctrl/pinmux.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index a5467f8..b6d964f 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -112,16 +112,6 @@ static int pin_request(struct pinctrl_dev *pctldev,
 
 	dev_dbg(&pctldev->dev, "request pin %d for %s\n", pin, function);
 
-	if (!pin_is_valid(pctldev, pin)) {
-		dev_err(&pctldev->dev, "pin is invalid\n");
-		return -EINVAL;
-	}
-
-	if (!function) {
-		dev_err(&pctldev->dev, "no function name given\n");
-		return -EINVAL;
-	}
-
 	desc = pin_desc_get(pctldev, pin);
 	if (desc == NULL) {
 		dev_err(&pctldev->dev,
@@ -129,6 +119,11 @@ static int pin_request(struct pinctrl_dev *pctldev,
 		goto out;
 	}
 
+	if (!function) {
+		dev_err(&pctldev->dev, "no function name given\n");
+		return -EINVAL;
+	}
+
 	spin_lock(&desc->lock);
 	if (desc->mux_function) {
 		spin_unlock(&desc->lock);
-- 
1.7.5.4

[PATCH 2/2] pinmux: Unify pin type from signed to unsigned.

[Marek Belisko]

Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com>
---
 drivers/pinctrl/core.c          |    7 ++-----
 drivers/pinctrl/core.h          |    2 +-
 drivers/pinctrl/pinmux.c        |    4 ++--
 include/linux/pinctrl/pinctrl.h |    4 ++--
 4 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 423522d..11e2ec5 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -87,7 +87,7 @@ struct pinctrl_dev *get_pinctrl_dev_from_dev(struct device *dev,
 	return found ? pctldev : NULL;
 }
 
-struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, int pin)
+struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, unsigned int pin)
 {
 	struct pin_desc *pindesc;
 	unsigned long flags;
@@ -107,13 +107,10 @@ struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, int pin)
  * This tells us whether a certain pin exist on a certain pin controller or
  * not. Pin lists may be sparse, so some pins may not exist.
  */
-bool pin_is_valid(struct pinctrl_dev *pctldev, int pin)
+bool pin_is_valid(struct pinctrl_dev *pctldev, unsigned int pin)
 {
 	struct pin_desc *pindesc;
 
-	if (pin < 0)
-		return false;
-
 	pindesc = pin_desc_get(pctldev, pin);
 	if (pindesc == NULL)
 		return false;
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 472fa13..759dc1f 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -65,7 +65,7 @@ struct pin_desc {
 
 struct pinctrl_dev *get_pinctrl_dev_from_dev(struct device *dev,
 					     const char *dev_name);
-struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, int pin);
+struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, unsigned int pin);
 int pinctrl_get_device_gpio_range(unsigned gpio,
 				  struct pinctrl_dev **outdev,
 				  struct pinctrl_gpio_range **outrange);
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index b6d964f..a53d36a 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -103,7 +103,7 @@ struct pinmux_hog {
  *	single GPIO pin
  */
 static int pin_request(struct pinctrl_dev *pctldev,
-		       int pin, const char *function, bool gpio,
+		       unsigned int pin, const char *function, bool gpio,
 		       struct pinctrl_gpio_range *gpio_range)
 {
 	struct pin_desc *desc;
@@ -213,7 +213,7 @@ int pinmux_request_gpio(unsigned gpio)
 	struct pinctrl_dev *pctldev;
 	struct pinctrl_gpio_range *range;
 	int ret;
-	int pin;
+	unsigned int pin;
 
 	ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range);
 	if (ret)
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 3605e94..34c33c7 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -112,7 +112,7 @@ struct pinctrl_desc {
 extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 				struct device *dev, void *driver_data);
 extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
-extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin);
+extern bool pin_is_valid(struct pinctrl_dev *pctldev, unsigned int pin);
 extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
 				struct pinctrl_gpio_range *range);
 extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
@@ -123,7 +123,7 @@ extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
 
 
 /* Sufficiently stupid default function when pinctrl is not in use */
-static inline bool pin_is_valid(struct pinctrl_dev *pctldev, int pin)
+static inline bool pin_is_valid(struct pinctrl_dev *pctldev, unsigned int pin)
 {
 	return pin >= 0;
 }
-- 
1.7.5.4

Re: [PATCH 2/2] pinmux: Unify pin type from signed to unsigned.

[David Rientjes]

On Wed, 26 Oct 2011, Marek Belisko wrote:

> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
> index 3605e94..34c33c7 100644
> --- a/include/linux/pinctrl/pinctrl.h
> +++ b/include/linux/pinctrl/pinctrl.h
> @@ -112,7 +112,7 @@ struct pinctrl_desc {
>  extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
>  				struct device *dev, void *driver_data);
>  extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
> -extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin);
> +extern bool pin_is_valid(struct pinctrl_dev *pctldev, unsigned int pin);
>  extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
>  				struct pinctrl_gpio_range *range);
>  extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
> @@ -123,7 +123,7 @@ extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
>  
>  
>  /* Sufficiently stupid default function when pinctrl is not in use */
> -static inline bool pin_is_valid(struct pinctrl_dev *pctldev, int pin)
> +static inline bool pin_is_valid(struct pinctrl_dev *pctldev, unsigned int pin)
>  {
>  	return pin >= 0;
>  }


So this would just become return true, right?

Re: [PATCH 2/2] pinmux: Unify pin type from signed to unsigned.

[Belisko Marek]

On Wed, Oct 26, 2011 at 11:06 PM, David Rientjes <rientjes@google.com> wrote:
> On Wed, 26 Oct 2011, Marek Belisko wrote:
>
>> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
>> index 3605e94..34c33c7 100644
>> --- a/include/linux/pinctrl/pinctrl.h
>> +++ b/include/linux/pinctrl/pinctrl.h
>> @@ -112,7 +112,7 @@ struct pinctrl_desc {
>>  extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
>>                               struct device *dev, void *driver_data);
>>  extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
>> -extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin);
>> +extern bool pin_is_valid(struct pinctrl_dev *pctldev, unsigned int pin);
>>  extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
>>                               struct pinctrl_gpio_range *range);
>>  extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
>> @@ -123,7 +123,7 @@ extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
>>
>>
>>  /* Sufficiently stupid default function when pinctrl is not in use */
>> -static inline bool pin_is_valid(struct pinctrl_dev *pctldev, int pin)
>> +static inline bool pin_is_valid(struct pinctrl_dev *pctldev, unsigned int pin)
>>  {
>>       return pin >= 0;
>>  }
>
>
> So this would just become return true, right?
Right I've overlooked it. Will send update version.
>

thanks,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

RE: [PATCH 2/2] pinmux: Unify pin type from signed to unsigned.

[Stephen Warren]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2286 bytes --]

Belisko Marek wrote at Thursday, October 27, 2011 12:13 AM:
> On Wed, Oct 26, 2011 at 11:06 PM, David Rientjes <rientjes@google.com> wrote:
> > On Wed, 26 Oct 2011, Marek Belisko wrote:
> >
> >> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
> >> index 3605e94..34c33c7 100644
> >> --- a/include/linux/pinctrl/pinctrl.h
> >> +++ b/include/linux/pinctrl/pinctrl.h
> >> @@ -112,7 +112,7 @@ struct pinctrl_desc {
> >>  extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
> >>                               struct device *dev, void *driver_data);
> >>  extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
> >> -extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin);
> >> +extern bool pin_is_valid(struct pinctrl_dev *pctldev, unsigned int pin);
> >>  extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
> >>                               struct pinctrl_gpio_range *range);
> >>  extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
> >> @@ -123,7 +123,7 @@ extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
> >>
> >>
> >>  /* Sufficiently stupid default function when pinctrl is not in use */
> >> -static inline bool pin_is_valid(struct pinctrl_dev *pctldev, int pin)
> >> +static inline bool pin_is_valid(struct pinctrl_dev *pctldev, unsigned int pin)
> >>  {
> >>       return pin >= 0;
> >>  }
> >
> >
> > So this would just become return true, right?
>
> Right I've overlooked it. Will send update version.

I wonder if the type shouldn't be signed consistently rather than unsigned,
since a signed value, -1, is typically used to indicate "invalid pin".

I wonder if the dummy pin_is_valid() should actually return false rather
than true; that's what <linux/gpio.h> does, but asm-generic/gpio.h always
returns true, like your updated pinctrl patch would.

Looking at include/linux/gpio.h, gpiolib has the same issue with signed/
unsigned inconsistency. Whatever solution we pick for pinctrl should be
consistent with gpiolib; Grant should comment here.

-- 
nvpublic

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] pinmux: Unify pin type from signed to unsigned.

[Linus Walleij]

On Thu, Oct 27, 2011 at 8:13 AM, Belisko Marek <marek.belisko@gmail.com> wrote:
> On Wed, Oct 26, 2011 at 11:06 PM, David Rientjes <rientjes@google.com> wrote:
>> On Wed, 26 Oct 2011, Marek Belisko wrote:
>>
>>>  /* Sufficiently stupid default function when pinctrl is not in use */
>>> -static inline bool pin_is_valid(struct pinctrl_dev *pctldev, int pin)
>>> +static inline bool pin_is_valid(struct pinctrl_dev *pctldev, unsigned int pin)
>>>  {
>>>       return pin >= 0;
>>>  }
>>
>>
>> So this would just become return true, right?
>
> Right I've overlooked it. Will send update version.

Well, actually the idea was that the external interface to the pin control
should have a signed value, so as to detect negative pin numbers as
"invalid".

Internally, and towards the pin control driver backends it is supposed
to be unsigned, because we don't want to have to check it for being
negative all the time.

So can you please make a patch only patching pin_desc_get()
to begin with?

For pin_is_valid() we rather should find out if it will have
any external users (like for GPIO) or if we should just delete
that function (now that pin control doesn't use it internally
anymore).

Yours,
Linus Walleij

[PATH v2 2/2] pinmux: Unify pin type from signed to unsigned.

[Marek Belisko]

Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com>
---
 drivers/pinctrl/core.c |    2 +-
 drivers/pinctrl/core.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 423522d..18a7dda 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -87,7 +87,7 @@ struct pinctrl_dev *get_pinctrl_dev_from_dev(struct device *dev,
 	return found ? pctldev : NULL;
 }
 
-struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, int pin)
+struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, unsigned int pin)
 {
 	struct pin_desc *pindesc;
 	unsigned long flags;
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 472fa13..759dc1f 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -65,7 +65,7 @@ struct pin_desc {
 
 struct pinctrl_dev *get_pinctrl_dev_from_dev(struct device *dev,
 					     const char *dev_name);
-struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, int pin);
+struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, unsigned int pin);
 int pinctrl_get_device_gpio_range(unsigned gpio,
 				  struct pinctrl_dev **outdev,
 				  struct pinctrl_gpio_range **outrange);
-- 
1.7.5.4

Re: [PATH v2 2/2] pinmux: Unify pin type from signed to unsigned.

[Linus Walleij]

On Mon, Oct 31, 2011 at 9:27 PM, Marek Belisko
<marek.belisko@open-nandra.com> wrote:

> Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com>
> ---
>  drivers/pinctrl/core.c |    2 +-
>  drivers/pinctrl/core.h |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Thanks, applied.

Linus Walleij

RE: [PATCH 1/2] pinmux: Remove double pin validity check.

[Stephen Warren]

Belisko Marek wrote at Wednesday, October 26, 2011 2:57 PM:
> Function pin_is_valid just call pin_desc_get which is in pin_request
> call some line below. Remove pin_is_valid() check.
...
> @@ -112,16 +112,6 @@ static int pin_request(struct pinctrl_dev *pctldev,
> 
>  	dev_dbg(&pctldev->dev, "request pin %d for %s\n", pin, function);
> 
> -	if (!pin_is_valid(pctldev, pin)) {
> -		dev_err(&pctldev->dev, "pin is invalid\n");
> -		return -EINVAL;
> -	}

So that makes sense.

> -
> -	if (!function) {
> -		dev_err(&pctldev->dev, "no function name given\n");
> -		return -EINVAL;
> -	}

But I don't think you want to remove that? Oh actually, perhaps removing
that /is/ valid, since it's an internal function and can't be called with
a NULL function parameter. But, you should mention why in the changelog
so it doesn't look like a mistake.

-- 
nvpublic

Re: [PATCH 1/2] pinmux: Remove double pin validity check.

[Belisko Marek]

On Thu, Oct 27, 2011 at 6:37 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Belisko Marek wrote at Wednesday, October 26, 2011 2:57 PM:
>> Function pin_is_valid just call pin_desc_get which is in pin_request
>> call some line below. Remove pin_is_valid() check.
> ...
>> @@ -112,16 +112,6 @@ static int pin_request(struct pinctrl_dev *pctldev,
>>
>>       dev_dbg(&pctldev->dev, "request pin %d for %s\n", pin, function);
>>
>> -     if (!pin_is_valid(pctldev, pin)) {
>> -             dev_err(&pctldev->dev, "pin is invalid\n");
>> -             return -EINVAL;
>> -     }
>
> So that makes sense.
>
>> -
>> -     if (!function) {
>> -             dev_err(&pctldev->dev, "no function name given\n");
>> -             return -EINVAL;
>> -     }
>
> But I don't think you want to remove that? Oh actually, perhaps removing
> that /is/ valid, since it's an internal function and can't be called with
> a NULL function parameter. But, you should mention why in the changelog
> so it doesn't look like a mistake.
Well it's not removed. It's moved below pin_desc_get call.
>
> --
> nvpublic
>
>

regards,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

RE: [PATCH 1/2] pinmux: Remove double pin validity check.

[Stephen Warren]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1474 bytes --]

Belisko Marek wrote at Thursday, October 27, 2011 12:40 PM:
> On Thu, Oct 27, 2011 at 6:37 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Belisko Marek wrote at Wednesday, October 26, 2011 2:57 PM:
> >> Function pin_is_valid just call pin_desc_get which is in pin_request
> >> call some line below. Remove pin_is_valid() check.
> > ...
> >> @@ -112,16 +112,6 @@ static int pin_request(struct pinctrl_dev *pctldev,
> >>
> >>       dev_dbg(&pctldev->dev, "request pin %d for %s\n", pin, function);
> >>
> >> -     if (!pin_is_valid(pctldev, pin)) {
> >> -             dev_err(&pctldev->dev, "pin is invalid\n");
> >> -             return -EINVAL;
> >> -     }
> >
> > So that makes sense.
> >
> >> -
> >> -     if (!function) {
> >> -             dev_err(&pctldev->dev, "no function name given\n");
> >> -             return -EINVAL;
> >> -     }
> >
> > But I don't think you want to remove that? Oh actually, perhaps removing
> > that /is/ valid, since it's an internal function and can't be called with
> > a NULL function parameter. But, you should mention why in the changelog
> > so it doesn't look like a mistake.
>
> Well it's not removed. It's moved below pin_desc_get call.

Sigh, so it is. In that case,

Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] pinmux: Remove double pin validity check.

[Linus Walleij]

On Wed, Oct 26, 2011 at 10:57 PM, Marek Belisko
<marek.belisko@open-nandra.com> wrote:

> Function pin_is_valid just call pin_desc_get which is in pin_request
> call some line below. Remove pin_is_valid() check.
>
> Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com>

So it is. Applied patch with Stephen's ACK.
Thanks Marek!

Linus Walleij