[RFC] pinctrl: Replace list_groups() with get_groups_count()

[RFC] pinctrl: Replace list_groups() with get_groups_count()

[viresh kumar]

Most of the SoC drivers implement list_*() routines for pinctrl, pinmux,
pinconf, etc, And these routines continue returning zero until the second
argument is greater than total count of available groups, function, etc.

I will take struct pinctrl_ops as an example to elaborate my concern/doubts.

There are few concerns here:
- list_groups() is called multiple times. If the purpose of this routine is to
  only check the number of groups available, then why not remove it and create
  another routine which returns number of total groups. Over which, we can run a
  simple loop.
- All client drivers have checks in their ops routines to check the range of
  groups, which is just not required if we pass the correct number to pinctrl
  framework. Because these ops are going to be called via pinctrl, and we can
  trust on it and place these range checkers there only to make SoC drivers
  simple.

I just wanted to have different views about my concerns/idea before i start
coding. If this idea looks fine, i would do this change across pinmux framework.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 include/linux/pinctrl/pinctrl.h |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 4e9f078..740fb99 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -64,9 +64,7 @@ struct pinctrl_gpio_range {
 /**
  * struct pinctrl_ops - global pin control operations, to be implemented by
  * pin controller drivers.
- * @list_groups: list the number of selectable named groups available
- *	in this pinmux driver, the core will begin on 0 and call this
- *	repeatedly as long as it returns >= 0 to enumerate the groups
+ * @get_group_count: Returns the count of total number of groups registered.
  * @get_group_name: return the group name of the pin group
  * @get_group_pins: return an array of pins corresponding to a certain
  *	group selector @pins, and the size of the array in @num_pins
@@ -74,7 +72,7 @@ struct pinctrl_gpio_range {
  *	info for a certain pin in debugfs
  */
 struct pinctrl_ops {
-	int (*list_groups) (struct pinctrl_dev *pctldev, unsigned selector);
+	int (*get_group_count) (struct pinctrl_dev *pctldev);
 	const char *(*get_group_name) (struct pinctrl_dev *pctldev,
 				       unsigned selector);
 	int (*get_group_pins) (struct pinctrl_dev *pctldev,
-- 
1.7.9

[RFC] pinctrl: Replace list_groups() with get_groups_count()

[Stephen Warren]

On 03/29/2012 10:34 AM, viresh kumar wrote:
> Most of the SoC drivers implement list_*() routines for pinctrl, pinmux,
> pinconf, etc, And these routines continue returning zero until the second
> argument is greater than total count of available groups, function, etc.
> 
> I will take struct pinctrl_ops as an example to elaborate my concern/doubts.
> 
> There are few concerns here:
> - list_groups() is called multiple times. If the purpose of this routine is to
>   only check the number of groups available, then why not remove it and create
>   another routine which returns number of total groups. Over which, we can run a
>   simple loop.
> - All client drivers have checks in their ops routines to check the range of
>   groups, which is just not required if we pass the correct number to pinctrl
>   framework. Because these ops are going to be called via pinctrl, and we can
>   trust on it and place these range checkers there only to make SoC drivers
>   simple.
> 
> I just wanted to have different views about my concerns/idea before i start
> coding. If this idea looks fine, i would do this change across pinmux framework.

The concept is fine by me.

I vaguely recall raising the same issue before, and Linus saying he
wanted to be consistent with similar list_*() functions in other
subsystems, but personally I'd prefer to evaluate this aspect of the
pinctrl subsystem on its own.

[RFC] pinctrl: Replace list_groups() with get_groups_count()

[Stephen Warren]

On 03/29/2012 11:31 AM, viresh kumar wrote:
> On Mar 29, 2012 10:56 PM, Stephen Warren wrote:
>> On 03/29/2012 10:34 AM, viresh kumar wrote:
...
>> I vaguely recall raising the same issue before, and Linus saying he
>> wanted to be consistent with similar list_*() functions in other
>> subsystems, but personally I'd prefer to evaluate this aspect of the
>> pinctrl subsystem on its own.
> 
> Which other subsystems have such stuff?

For example. include/linux/regulator/driver.h struct regulator_ops field
list_voltage().

[RFC] pinctrl: Replace list_groups() with get_groups_count()

[Viresh Kumar]

On 3/29/2012 11:18 PM, Stephen Warren wrote:
>>> >> I vaguely recall raising the same issue before, and Linus saying he
>>> >> wanted to be consistent with similar list_*() functions in other
>>> >> subsystems, but personally I'd prefer to evaluate this aspect of the
>>> >> pinctrl subsystem on its own.
>> > 
>> > Which other subsystems have such stuff?
> For example. include/linux/regulator/driver.h struct regulator_ops field
> list_voltage().

Thanks.

Anyway, if others implement it, that doesn't mean we also do that, even
if we are not happy with it. It is not only about cleaning of code, but
it will save boot time too.

@Linus: Can i have your viewpoint before i start doing this?

-- 
viresh

[RFC] pinctrl: Replace list_groups() with get_groups_count()

[Linus Walleij]

On Thu, Mar 29, 2012 at 7:48 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/29/2012 11:31 AM, viresh kumar wrote:
>> On Mar 29, 2012 10:56 PM, Stephen Warren wrote:
>>> On 03/29/2012 10:34 AM, viresh kumar wrote:
> ...
>>> I vaguely recall raising the same issue before, and Linus saying he
>>> wanted to be consistent with similar list_*() functions in other
>>> subsystems, but personally I'd prefer to evaluate this aspect of the
>>> pinctrl subsystem on its own.
>>
>> Which other subsystems have such stuff?
>
> For example. include/linux/regulator/driver.h struct regulator_ops field
> list_voltage().

Well, actually .list_voltage() returns the voltage per se for each
enumerator, so it works like this quick way to iterate over a list
of integers.

But since we're not really getting anything back from the .list_groups()
callback, just zero or error, there is no specific point in having
it engineered like this.

So please go ahead and patch it in whatever way makes more
sense!

Thanks,
Linus Walleij

[RFC] pinctrl: Replace list_groups() with get_groups_count()

[Linus Walleij]

On Fri, Mar 30, 2012 at 6:01 AM, Viresh Kumar <viresh.kumar@st.com> wrote:

> Anyway, if others implement it, that doesn't mean we also do that, even
> if we are not happy with it. It is not only about cleaning of code, but
> it will save boot time too.
>
> @Linus: Can i have your viewpoint before i start doing this?

Just patch it, as detailed in the other letter there is really no point
for having it like this so go ahead. As long as you also patch all
the users and remove the old interface, just rewrite it. If you could
also test it on something it'd be nice ...

Yours,
Linus Walleij

[RFC] pinctrl: Replace list_groups() with get_groups_count()

[Viresh Kumar]

On 3/30/2012 10:34 AM, Linus Walleij wrote:
> Just patch it, as detailed in the other letter there is really no point
> for having it like this so go ahead. As long as you also patch all
> the users and remove the old interface, just rewrite it.

I have this in mind. Will fix all users.

> If you could also test it on something it'd be nice ...

I don't have any board to test it. But after i am done with this, i
will complete pinctrl support for SPEAr and there i can test it.

I hope before that lot of other people would be able to test it. :)

BTW, i am planning not to add another call, get_groups_count(),
but add another field like ngroups, which would be set by users.

-- 
viresh

[RFC] pinctrl: Replace list_groups() with get_groups_count()

[Linus Walleij]

On Fri, Mar 30, 2012 at 7:09 AM, Viresh Kumar <viresh.kumar@st.com> wrote:

> BTW, i am planning not to add another call, get_groups_count(),
> but add another field like ngroups, which would be set by users.

get_groups_count() may make some sense if the number of
available groups could in some instance change at runtime.
Maybe a bit overcautious and pedantic but you never know...

But do it any way you like, I don't worry very much :-)

Yours,
Linus Walleij

[RFC] pinctrl: Replace list_groups() with get_groups_count()

[Viresh Kumar]

On 3/30/2012 10:44 AM, Linus Walleij wrote:
> get_groups_count() may make some sense if the number of
> available groups could in some instance change at runtime.
> Maybe a bit overcautious and pedantic but you never know...

Will go with get_groups_count(); :)

-- 
viresh

[RFC] pinctrl: Replace list_groups() with get_groups_count()

[Mark Brown]

On Thu, Mar 29, 2012 at 11:48:14AM -0600, Stephen Warren wrote:
> On 03/29/2012 11:31 AM, viresh kumar wrote:

> > Which other subsystems have such stuff?

> For example. include/linux/regulator/driver.h struct regulator_ops field
> list_voltage().

The reason regulator does this for list voltage is because we
idiomatically have gaps in the space of selectors as they should
normally map directly onto register values but sometimes some of the
register values are not specified so we need to skip them.  This can
happen both due to chip limitations or due to constraints imposed by
the system.