[PATCH 0/2] add support for partial pin space

[PATCH 0/2] add support for partial pin space

[Chanho Park]

A pin space of current pinctrl driver always start with zero-based pin
number. And pins are enumerated from 0 to a max pin number. If there are
two pins (one is 0, another is 1000), we always enumerate from 0 to 1000
for just only two pins. This patch enables to start non-zero based pin
number and to enumerate actually owned pins.

For example, there is a system which has huge(500 or 1000) pins. If muxing
or controlling pins are small, we just define such small pins instead of
whole pins.

Chanho Park (2):
  pinctrl: correct a offset while enumerating pins
  pinctrl: remove unnecessary max pin number

 drivers/pinctrl/core.c          |   19 ++++++++++---------
 drivers/pinctrl/pinconf.c       |    9 +++++----
 drivers/pinctrl/pinctrl-sirf.c  |    1 -
 drivers/pinctrl/pinctrl-u300.c  |    1 -
 drivers/pinctrl/pinmux.c        |    9 +++++----
 include/linux/pinctrl/pinctrl.h |    5 -----
 6 files changed, 20 insertions(+), 24 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] pinctrl: correct a offset while enumerating pins

[Chanho Park]

This patch modifies a offset while enumerating pins to support a
partial pin space. If we use a pin number for enumerating pins,
the pin space always starts with zero base. Indeed, we always check
the pin is in the pin space. An extreme example, there is only two pins.
One is 0. Another is 1000. We always enumerate whole offsets until 1000.
For solving this problem, we use the offset of the pin array instead
of the zero-based pin number.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/core.c    |   18 ++++++++++--------
 drivers/pinctrl/pinconf.c |    9 +++++----
 drivers/pinctrl/pinmux.c  |    9 +++++----
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 4a65d83..03a1dd6 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -102,14 +102,15 @@ struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, unsigned int pin)
  */
 int pin_get_from_name(struct pinctrl_dev *pctldev, const char *name)
 {
-	unsigned pin;
+	unsigned i, pin;
 
-	/* The highest pin number need to be included in the loop, thus <= */
-	for (pin = 0; pin <= pctldev->desc->maxpin; pin++) {
+	/* The pin number can be retrived from the pin controller descriptor */
+	for (i = 0; i < pctldev->desc->npins; i++) {
 		struct pin_desc *desc;
 
+		pin = pctldev->desc->pins[i].number;
 		desc = pin_desc_get(pctldev, pin);
-		/* Pin space may be sparse */
+		/* Skip if we cannot search the pin */
 		if (desc == NULL)
 			continue;
 		if (desc->name && !strcmp(name, desc->name))
@@ -378,17 +379,18 @@ static int pinctrl_pins_show(struct seq_file *s, void *what)
 {
 	struct pinctrl_dev *pctldev = s->private;
 	const struct pinctrl_ops *ops = pctldev->desc->pctlops;
-	unsigned pin;
+	unsigned i, pin;
 
 	seq_printf(s, "registered pins: %d\n", pctldev->desc->npins);
 	seq_printf(s, "max pin number: %d\n", pctldev->desc->maxpin);
 
-	/* The highest pin number need to be included in the loop, thus <= */
-	for (pin = 0; pin <= pctldev->desc->maxpin; pin++) {
+	/* The pin number can be retrived from the pin controller descriptor */
+	for (i = 0; i < pctldev->desc->npins; i++) {
 		struct pin_desc *desc;
 
+		pin = pctldev->desc->pins[i].number;
 		desc = pin_desc_get(pctldev, pin);
-		/* Pin space may be sparse */
+		/* Skip if we cannot search the pin */
 		if (desc == NULL)
 			continue;
 
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 950b3cb..cd6647e 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -232,17 +232,18 @@ static void pinconf_dump_pin(struct pinctrl_dev *pctldev,
 static int pinconf_pins_show(struct seq_file *s, void *what)
 {
 	struct pinctrl_dev *pctldev = s->private;
-	unsigned pin;
+	unsigned i, pin;
 
 	seq_puts(s, "Pin config settings per pin\n");
 	seq_puts(s, "Format: pin (name): pinmux setting array\n");
 
-	/* The highest pin number need to be included in the loop, thus <= */
-	for (pin = 0; pin <= pctldev->desc->maxpin; pin++) {
+	/* The pin number can be retrived from the pin controller descriptor */
+	for (i = 0; pin < pctldev->desc->npins; i++) {
 		struct pin_desc *desc;
 
+		pin = pctldev->desc->pins[i].number;
 		desc = pin_desc_get(pctldev, pin);
-		/* Pin space may be sparse */
+		/* Skip if we cannot search the pin */
 		if (desc == NULL)
 			continue;
 
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index f29a9d2..59bdf90 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -1097,18 +1097,19 @@ static int pinmux_functions_show(struct seq_file *s, void *what)
 static int pinmux_pins_show(struct seq_file *s, void *what)
 {
 	struct pinctrl_dev *pctldev = s->private;
-	unsigned pin;
+	unsigned i, pin;
 
 	seq_puts(s, "Pinmux settings per pin\n");
 	seq_puts(s, "Format: pin (name): pinmuxfunction\n");
 
-	/* The highest pin number need to be included in the loop, thus <= */
-	for (pin = 0; pin <= pctldev->desc->maxpin; pin++) {
+	/* The pin number can be retrived from the pin controller descriptor */
+	for (i = 0; i < pctldev->desc->npins; i++) {
 
 		struct pin_desc *desc;
 
+		pin = pctldev->desc->pins[i].number;
 		desc = pin_desc_get(pctldev, pin);
-		/* Pin space may be sparse */
+		/* Skip if we cannot search the pin */
 		if (desc == NULL)
 			continue;
 
-- 
1.7.5.4

Re: [PATCH 1/2] pinctrl: correct a offset while enumerating pins

[Linus Walleij]

On Tue, Jan 3, 2012 at 8:47 AM, Chanho Park <chanho61.park@samsung.com> wrote:

This:

> -               /* Pin space may be sparse */
> +               /* Skip if we cannot search the pin */
(...)
> -               /* Pin space may be sparse */
> +               /* Skip if we cannot search the pin */

I don't see why you're changing that comment, it is still true that
the pin space may be sparse and that is what your patch confirms.

So I will restore that comment...

Yours,
Linus Walleij

RE: [PATCH 1/2] pinctrl: correct a offset while enumerating pins

[Chanho Park]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Tuesday, January 03, 2012 4:59 PM
> To: Chanho Park
> Cc: swarren@nvidia.com; Baohua.Song@csr.com; linux-
> kernel@vger.kernel.org; Kyungmin Park
> Subject: Re: [PATCH 1/2] pinctrl: correct a offset while enumerating
> pins
> 
> On Tue, Jan 3, 2012 at 8:47 AM, Chanho Park <chanho61.park@samsung.com>
> wrote:
> 
> This:
> 
> > -               /* Pin space may be sparse */
> > +               /* Skip if we cannot search the pin */
> (...)
> > -               /* Pin space may be sparse */
> > +               /* Skip if we cannot search the pin */
> 
> I don't see why you're changing that comment, it is still true that
> the pin space may be sparse and that is what your patch confirms.
> 
> So I will restore that comment...

We do not concern that pin space is sparse
because we enumerate the pin space using the offset of array.
This situation is that we cannot search the pin from radix tree.
It is BUGON() or just skip this pin :)

Best regards,
Chanho Park

Re: [PATCH 1/2] pinctrl: correct a offset while enumerating pins

[Linus Walleij]

On Tue, Jan 3, 2012 at 9:37 AM, Chanho Park <chanho61.park@samsung.com> wrote:
>> From: Linus Walleij [mailto:linus.walleij@linaro.org]
>> On Tue, Jan 3, 2012 at 8:47 AM, Chanho Park <chanho61.park@samsung.com>
>> This:
>>
>> > -               /* Pin space may be sparse */
>> > +               /* Skip if we cannot search the pin */
>> (...)
>> > -               /* Pin space may be sparse */
>> > +               /* Skip if we cannot search the pin */
>>
>> I don't see why you're changing that comment, it is still true that
>> the pin space may be sparse and that is what your patch confirms.
>>
>> So I will restore that comment...
>
> We do not concern that pin space is sparse
> because we enumerate the pin space using the offset of array.
> This situation is that we cannot search the pin from radix tree.
> It is BUGON() or just skip this pin :)

Yes but the comment there is about why we are searching
the radix tree instead of just inserting the new pin. If the
radix tree was not sparse, we would not need to search
it.

Yours,
Linus Walleij

[PATCH 2/2] pinctrl: remove unnecessary max pin number

[Chanho Park]

This patch removes maxpin member in the pin control descriptor
because we don't need this value as we enumerate a pin space
using offset.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/core.c          |    1 -
 drivers/pinctrl/pinctrl-sirf.c  |    1 -
 drivers/pinctrl/pinctrl-u300.c  |    1 -
 include/linux/pinctrl/pinctrl.h |    5 -----
 4 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 03a1dd6..dfd7232 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -382,7 +382,6 @@ static int pinctrl_pins_show(struct seq_file *s, void *what)
 	unsigned i, pin;
 
 	seq_printf(s, "registered pins: %d\n", pctldev->desc->npins);
-	seq_printf(s, "max pin number: %d\n", pctldev->desc->maxpin);
 
 	/* The pin number can be retrived from the pin controller descriptor */
 	for (i = 0; i < pctldev->desc->npins; i++) {
diff --git a/drivers/pinctrl/pinctrl-sirf.c b/drivers/pinctrl/pinctrl-sirf.c
index 99e688e..6b3534c 100644
--- a/drivers/pinctrl/pinctrl-sirf.c
+++ b/drivers/pinctrl/pinctrl-sirf.c
@@ -1086,7 +1086,6 @@ static struct pinctrl_desc sirfsoc_pinmux_desc = {
 	.name = DRIVER_NAME,
 	.pins = sirfsoc_pads,
 	.npins = ARRAY_SIZE(sirfsoc_pads),
-	.maxpin = SIRFSOC_NUM_PADS - 1,
 	.pctlops = &sirfsoc_pctrl_ops,
 	.pmxops = &sirfsoc_pinmux_ops,
 	.owner = THIS_MODULE,
diff --git a/drivers/pinctrl/pinctrl-u300.c b/drivers/pinctrl/pinctrl-u300.c
index 5a01da1..03d3cb8 100644
--- a/drivers/pinctrl/pinctrl-u300.c
+++ b/drivers/pinctrl/pinctrl-u300.c
@@ -1108,7 +1108,6 @@ static struct pinctrl_desc u300_pmx_desc = {
 	.name = DRIVER_NAME,
 	.pins = u300_pads,
 	.npins = ARRAY_SIZE(u300_pads),
-	.maxpin = U300_NUM_PADS-1,
 	.pctlops = &u300_pctrl_ops,
 	.pmxops = &u300_pmx_ops,
 	.confops = &u300_pconf_ops,
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index ac22827..5904725 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -114,10 +114,6 @@ struct pinctrl_ops {
  *	this pin controller
  * @npins: number of descriptors in the array, usually just ARRAY_SIZE()
  *	of the pins field above
- * @maxpin: since pin spaces may be sparse, there can he "holes" in the
- *	pin range, this attribute gives the maximum pin number in the
- *	total range. This should not be lower than npins for example,
- *	but may be equal to npins if you have no holes in the pin range.
  * @pctlops: pin control operation vtable, to support global concepts like
  *	grouping of pins, this is optional.
  * @pmxops: pinmux operations vtable, if you support pinmuxing in your driver
@@ -129,7 +125,6 @@ struct pinctrl_desc {
 	const char *name;
 	struct pinctrl_pin_desc const *pins;
 	unsigned int npins;
-	unsigned int maxpin;
 	struct pinctrl_ops *pctlops;
 	struct pinmux_ops *pmxops;
 	struct pinconf_ops *confops;
-- 
1.7.5.4

Re: [PATCH 0/2] add support for partial pin space

[Linus Walleij]

On Tue, Jan 3, 2012 at 8:47 AM, Chanho Park <chanho61.park@samsung.com> wrote:

> A pin space of current pinctrl driver always start with zero-based pin
> number. And pins are enumerated from 0 to a max pin number. If there are
> two pins (one is 0, another is 1000), we always enumerate from 0 to 1000
> for just only two pins. This patch enables to start non-zero based pin
> number and to enumerate actually owned pins.
>
> For example, there is a system which has huge(500 or 1000) pins. If muxing
> or controlling pins are small, we just define such small pins instead of
> whole pins.

Seems like you're fixing my code to do what was originally intended :-)

The maxpin member was something I introduced for sparse ranges
which would auto-register "dummy pins" for those not included in the
enumeration.

That doesn't quite work, so was left behind.

I'll try applying the patches and see what happens.

Yours,
Linus Walleij

RE: [PATCH 0/2] add support for partial pin space

[Chanho Park]

Hello Linus W.,
Original code always enumerate from 0 to maxpin because you wrote
as an offset equals to a pin number. It's inefficient because we always
define and enumerate whole pins in the system even though we do not use
the pin.
The pin space was already stored as array in the pin controller descriptor.
And we also have a number of pins.

Please see below example,

static const struct pinctrl_pin_desc pads[] = {
	PINCTRL_PIN(100, "PIN100"),
	PINCTRL_PIN(9999, "PIN9999"),
};
In this extreme system, the system has two pins.
We do not support non-zero based pin starting number.
And we always do silly loop until 0 to 9999. :)

This patch enables to enumerate the pin space by using the offset of array.
Thus, we don't need maxpin to enumerate the pin space because it is
sufficient
to use npins.

Best Regards,
Chanho Park


> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Tuesday, January 03, 2012 4:57 PM
> To: Chanho Park
> Cc: swarren@nvidia.com; Baohua.Song@csr.com; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 0/2] add support for partial pin space
> 
> On Tue, Jan 3, 2012 at 8:47 AM, Chanho Park <chanho61.park@samsung.com>
> wrote:
> 
> > A pin space of current pinctrl driver always start with zero-based
> pin
> > number. And pins are enumerated from 0 to a max pin number. If there
> are
> > two pins (one is 0, another is 1000), we always enumerate from 0 to
> 1000
> > for just only two pins. This patch enables to start non-zero based
> pin
> > number and to enumerate actually owned pins.
> >
> > For example, there is a system which has huge(500 or 1000) pins. If
> muxing
> > or controlling pins are small, we just define such small pins instead
> of
> > whole pins.
> 
> Seems like you're fixing my code to do what was originally intended :-)
> 
> The maxpin member was something I introduced for sparse ranges
> which would auto-register "dummy pins" for those not included in the
> enumeration.
> 
> That doesn't quite work, so was left behind.
> 
> I'll try applying the patches and see what happens.
> 
> Yours,
> Linus Walleij