[PATCHv2] Input: omap4-keypad: Add pinctrl support

[PATCHv2] Input: omap4-keypad: Add pinctrl support

[Sourav Poddar]

Adapt keypad to use pinctrl framework.

Tested on omap4430 sdp with 3.7-rc1 kernel.

Cc: Felipe Balbi <balbi@ti.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
v1->v2
- Added "PROBE_DEFER" check 
 drivers/input/keyboard/omap4-keypad.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index c05f98c..502b832 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -31,6 +31,7 @@
 #include <linux/input.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <linux/platform_data/omap4-keypad.h>
 
@@ -76,6 +77,7 @@ enum {
 
 struct omap4_keypad {
 	struct input_dev *input;
+	struct pinctrl	*pins;
 
 	void __iomem *base;
 	unsigned int irq;
@@ -298,6 +300,15 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev)
 		goto err_release_mem;
 	}
 
+	keypad_data->pins = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(keypad_data->pins)) {
+		if (PTR_ERR(keypad_data->pins) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		dev_warn(&pdev->dev, "did not get pins for keypad error: %li\n",
+					PTR_ERR(keypad_data->pins));
+		keypad_data->pins = NULL;
+	}
 
 	/*
 	 * Enable clocks for the keypad module so that we can read
-- 
1.7.1

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Dmitry Torokhov]

Hi Sourav,

On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote:
> Adapt keypad to use pinctrl framework.
> 
> Tested on omap4430 sdp with 3.7-rc1 kernel.

I do not see anything in the driver that would directly use pinctrl. Is
there a better place to select default pin configuration; maybe when
instantiating platform device?

Thanks.

> 
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> v1->v2
> - Added "PROBE_DEFER" check 
>  drivers/input/keyboard/omap4-keypad.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index c05f98c..502b832 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -31,6 +31,7 @@
>  #include <linux/input.h>
>  #include <linux/slab.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include <linux/platform_data/omap4-keypad.h>
>  
> @@ -76,6 +77,7 @@ enum {
>  
>  struct omap4_keypad {
>  	struct input_dev *input;
> +	struct pinctrl	*pins;
>  
>  	void __iomem *base;
>  	unsigned int irq;
> @@ -298,6 +300,15 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev)
>  		goto err_release_mem;
>  	}
>  
> +	keypad_data->pins = devm_pinctrl_get_select_default(&pdev->dev);
> +	if (IS_ERR(keypad_data->pins)) {
> +		if (PTR_ERR(keypad_data->pins) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		dev_warn(&pdev->dev, "did not get pins for keypad error: %li\n",
> +					PTR_ERR(keypad_data->pins));
> +		keypad_data->pins = NULL;
> +	}
>  
>  	/*
>  	 * Enable clocks for the keypad module so that we can read
> -- 
> 1.7.1
> 

-- 
Dmitry

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Mon, Oct 22, 2012 at 5:50 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Sourav,
>
> On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote:
>> Adapt keypad to use pinctrl framework.
>>
>> Tested on omap4430 sdp with 3.7-rc1 kernel.
>
> I do not see anything in the driver that would directly use pinctrl. Is
> there a better place to select default pin configuration; maybe when
> instantiating platform device?

The option is to use pinctrl hogs. Then the pins will be taken,
muxed and configured by the pin controller itself.

Another option (not implemented) is to use bus notifiers.

(I wrote about this in some other thread but can't find it now.)

Each approach above come with its own set of problems.

If the driver need to handle multiple states like default/idle/sleep
it is IMO better to put the handling into the driver, so if that
is what is going to happen also to this driver it could be a good
idea to actually implement that code upfront and include in
this submission so as to show that this driver is really going
to exercise its pins.

But it's also a question of conformity: if other drivers in the
system is using different states and this is the only one
using a single "default" state, then it doesn't make sense
to have just one driver get its pins using hogs, it's just
inconsistent.

So Sourav, please tell us a bit about your plans for this
and other drivers!

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Benoit Cousson]

Hi Linus,

On 10/23/2012 11:13 AM, Linus Walleij wrote:
> On Mon, Oct 22, 2012 at 5:50 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> Hi Sourav,
>>
>> On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote:
>>> Adapt keypad to use pinctrl framework.
>>>
>>> Tested on omap4430 sdp with 3.7-rc1 kernel.
>>
>> I do not see anything in the driver that would directly use pinctrl. Is
>> there a better place to select default pin configuration; maybe when
>> instantiating platform device?
> 
> The option is to use pinctrl hogs. Then the pins will be taken,
> muxed and configured by the pin controller itself.
> 
> Another option (not implemented) is to use bus notifiers.
> 
> (I wrote about this in some other thread but can't find it now.)
> 
> Each approach above come with its own set of problems.
> 
> If the driver need to handle multiple states like default/idle/sleep
> it is IMO better to put the handling into the driver, so if that
> is what is going to happen also to this driver it could be a good
> idea to actually implement that code upfront and include in
> this submission so as to show that this driver is really going
> to exercise its pins.
> 
> But it's also a question of conformity: if other drivers in the
> system is using different states and this is the only one
> using a single "default" state, then it doesn't make sense
> to have just one driver get its pins using hogs, it's just
> inconsistent.
> 
> So Sourav, please tell us a bit about your plans for this
> and other drivers!

Yeah, this idea is to handle pinctrl from all the drivers, and
potentially change the mode during suspend when it is relevant.

Regards,
Benoit

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Tue, Oct 23, 2012 at 11:35 AM, Benoit Cousson <b-cousson@ti.com> wrote:
> On 10/23/2012 11:13 AM, Linus Walleij wrote:

>> So Sourav, please tell us a bit about your plans for this
>> and other drivers!
>
> Yeah, this idea is to handle pinctrl from all the drivers, and
> potentially change the mode during suspend when it is relevant.

I'm leaning toward the same approach for ux500.

But it appears that shmobile prefer to get all resources using
bus notifiers.

So we need to form some kind of consensus ... or live with
the fact that different systems do it different ways. Which will
explode the day we need to use a driver on two systems,
each using the other approach :-)

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1057 bytes --]

Hi,

On Tue, Oct 23, 2012 at 12:04:01PM +0200, Linus Walleij wrote:
> On Tue, Oct 23, 2012 at 11:35 AM, Benoit Cousson <b-cousson@ti.com> wrote:
> > On 10/23/2012 11:13 AM, Linus Walleij wrote:
> 
> >> So Sourav, please tell us a bit about your plans for this
> >> and other drivers!
> >
> > Yeah, this idea is to handle pinctrl from all the drivers, and
> > potentially change the mode during suspend when it is relevant.
> 
> I'm leaning toward the same approach for ux500.
> 
> But it appears that shmobile prefer to get all resources using
> bus notifiers.
> 
> So we need to form some kind of consensus ... or live with
> the fact that different systems do it different ways. Which will
> explode the day we need to use a driver on two systems,
> each using the other approach :-)

I much prefer having drivers explicitly manage all their resources,
which would mean that pinctrl calls need to be done on probe() and, if
necessary, during suspend()/resume().

Using bus notifiers for that is quite a hack IMHO.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Thomas Petazzoni]

On Tue, 23 Oct 2012 13:03:33 +0300, Felipe Balbi wrote:

> > But it appears that shmobile prefer to get all resources using
> > bus notifiers.
> > 
> > So we need to form some kind of consensus ... or live with
> > the fact that different systems do it different ways. Which will
> > explode the day we need to use a driver on two systems,
> > each using the other approach :-)
> 
> I much prefer having drivers explicitly manage all their resources,
> which would mean that pinctrl calls need to be done on probe() and, if
> necessary, during suspend()/resume().
> 
> Using bus notifiers for that is quite a hack IMHO.

Agreed. Just like drivers do their ioremap, request_irq and others,
they should also request their pin resources using the pinctrl API.
Hiding this behind a bus notifier is not nice.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Tue, Oct 23, 2012 at 12:23 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
>
> On Tue, 23 Oct 2012 13:03:33 +0300, Felipe Balbi wrote:
>
>> > But it appears that shmobile prefer to get all resources using
>> > bus notifiers.
>> >
>> > So we need to form some kind of consensus ... or live with
>> > the fact that different systems do it different ways. Which will
>> > explode the day we need to use a driver on two systems,
>> > each using the other approach :-)
>>
>> I much prefer having drivers explicitly manage all their resources,
>> which would mean that pinctrl calls need to be done on probe() and, if
>> necessary, during suspend()/resume().
>>
>> Using bus notifiers for that is quite a hack IMHO.
>
> Agreed. Just like drivers do their ioremap, request_irq and others,
> they should also request their pin resources using the pinctrl API.
> Hiding this behind a bus notifier is not nice.

So the biggest implementation of the notifier approach to resource
handling is the SH clock thing:
drivers/base/power/clock_ops.c

It's made to be generic but AFAICT only SH is using this.

So according to that paradigm most device resources should
be handled that way if I understand correctly the basic idea.

So let's get Rafael, Paul and Magnus in here to beat us up
a bit :-)

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

Hi,

On Tue, Oct 23, 2012 at 12:29:28PM +0200, Linus Walleij wrote:
> On Tue, Oct 23, 2012 at 12:23 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> >
> > On Tue, 23 Oct 2012 13:03:33 +0300, Felipe Balbi wrote:
> >
> >> > But it appears that shmobile prefer to get all resources using
> >> > bus notifiers.
> >> >
> >> > So we need to form some kind of consensus ... or live with
> >> > the fact that different systems do it different ways. Which will
> >> > explode the day we need to use a driver on two systems,
> >> > each using the other approach :-)
> >>
> >> I much prefer having drivers explicitly manage all their resources,
> >> which would mean that pinctrl calls need to be done on probe() and, if
> >> necessary, during suspend()/resume().
> >>
> >> Using bus notifiers for that is quite a hack IMHO.
> >
> > Agreed. Just like drivers do their ioremap, request_irq and others,
> > they should also request their pin resources using the pinctrl API.
> > Hiding this behind a bus notifier is not nice.
> 
> So the biggest implementation of the notifier approach to resource
> handling is the SH clock thing:
> drivers/base/power/clock_ops.c

that's different right ? It's just creating the list of clocks, device
drivers still have to call pm_clk_add().

That's ok, I guess, otherwise all struct device would allocate memory
which hardly ever used (so far).

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Tue, Oct 23, 2012 at 12:29 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Tue, Oct 23, 2012 at 12:29:28PM +0200, Linus Walleij wrote:

>> So the biggest implementation of the notifier approach to resource
>> handling is the SH clock thing:
>> drivers/base/power/clock_ops.c
>
> that's different right ? It's just creating the list of clocks, device
> drivers still have to call pm_clk_add().
>
> That's ok, I guess, otherwise all struct device would allocate memory
> which hardly ever used (so far).

Hm so I have had this idea of runtime PM core helping out
with pins, so I could add something like

pm_pins_fetch()
pm_pins_default()
pm_pins_idle()
pm_pins_sleep()

So if one is using the pin states defined in <linux/pinctrl/pinctrl-state.h>
then the PM core can help out in keeping track of the pins
and states, and the driver will just tell the PM core what
to do and when.

Would this fit the bill for everyone's code consolidation needs?
It would sure work for us...

It however require that no custom states are used and that we
keep to the state semantics I just happen to think is most
common.

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

Hi,

On Tue, Oct 23, 2012 at 12:45:33PM +0200, Linus Walleij wrote:
> On Tue, Oct 23, 2012 at 12:29 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Tue, Oct 23, 2012 at 12:29:28PM +0200, Linus Walleij wrote:
> 
> >> So the biggest implementation of the notifier approach to resource
> >> handling is the SH clock thing:
> >> drivers/base/power/clock_ops.c
> >
> > that's different right ? It's just creating the list of clocks, device
> > drivers still have to call pm_clk_add().
> >
> > That's ok, I guess, otherwise all struct device would allocate memory
> > which hardly ever used (so far).
> 
> Hm so I have had this idea of runtime PM core helping out
> with pins, so I could add something like
> 
> pm_pins_fetch()
> pm_pins_default()
> pm_pins_idle()
> pm_pins_sleep()
> 
> So if one is using the pin states defined in <linux/pinctrl/pinctrl-state.h>
> then the PM core can help out in keeping track of the pins
> and states, and the driver will just tell the PM core what
> to do and when.
> 
> Would this fit the bill for everyone's code consolidation needs?
> It would sure work for us...
> 
> It however require that no custom states are used and that we
> keep to the state semantics I just happen to think is most
> common.

From a quick read, it looks ok. I guess problems will only how up when
we actually end up with a silicon errata or something similar which
mandates that we change pin's state at a particular location. Not sure
if we have those yet.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Thomas Petazzoni]

On Tue, 23 Oct 2012 12:45:33 +0200, Linus Walleij wrote:

> Hm so I have had this idea of runtime PM core helping out
> with pins, so I could add something like
> 
> pm_pins_fetch()
> pm_pins_default()
> pm_pins_idle()
> pm_pins_sleep()
> 
> So if one is using the pin states defined in
> <linux/pinctrl/pinctrl-state.h> then the PM core can help out in
> keeping track of the pins and states, and the driver will just tell
> the PM core what to do and when.
> 
> Would this fit the bill for everyone's code consolidation needs?
> It would sure work for us...

That surely would work but is kind of non-obvious when reading a
driver's code: that's the problem with bus notifier, they do things a
bit "behind your back" without you noticing. Having the driver request
its own pinctrl state, and switch between states upon suspend/resume is
a lot more explicit, IMO.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mitch Bradley]

On 10/23/2012 12:03 AM, Felipe Balbi wrote:
> Hi,
> 
> I much prefer having drivers explicitly manage all their resources,
> which would mean that pinctrl calls need to be done on probe() and, if
> necessary, during suspend()/resume().


Per-driver resource management is certainly convenient when you are
dealing with a single system, but it becomes difficult to maintain for
drivers that are shared among many platforms.

The industry trend for many years has been consolidation around a single
programming model per class of device.  For example, SDHCI, EHCI, ATA.
This trend will only accelerate, as the cost of developing controller IP
and associated drivers increases.  Such drivers need to be as
platform-agnostic as possible.

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

HI,

On Tue, Oct 23, 2012 at 07:02:09AM -1000, Mitch Bradley wrote:
> On 10/23/2012 12:03 AM, Felipe Balbi wrote:
> > Hi,
> > 
> > I much prefer having drivers explicitly manage all their resources,
> > which would mean that pinctrl calls need to be done on probe() and, if
> > necessary, during suspend()/resume().
> 
> 
> Per-driver resource management is certainly convenient when you are
> dealing with a single system, but it becomes difficult to maintain for
> drivers that are shared among many platforms.

why ? look at drivers/usb/dwc3/, we're using that on OMAP, exynos, PCIe
and a couple of different FPGA implementations inside TI. Not to mention
what other licensees of that IP core might have internally.

So far no problesm with resources at all.

We have frameworks exactly to hide the differences.

> The industry trend for many years has been consolidation around a single
> programming model per class of device.  For example, SDHCI, EHCI, ATA.
> This trend will only accelerate, as the cost of developing controller IP
> and associated drivers increases.  Such drivers need to be as
> platform-agnostic as possible.

that's why we have pinctrl framework to abstract the details about pin
muxing.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mitch Bradley]

Perhaps I misunderstood what you were suggesting.  I thought that, when
you said "explicitly manage all their resources", you meant that the
driver should know the platform-specific details about clocks and power
domains.  That is one possible interpretation of the word "explicit".

Now I see that you meant that the driver should explicitly call
abstracted functions.


On 10/23/2012 7:20 AM, Felipe Balbi wrote:
> HI,
> 
> On Tue, Oct 23, 2012 at 07:02:09AM -1000, Mitch Bradley wrote:
>> On 10/23/2012 12:03 AM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> I much prefer having drivers explicitly manage all their resources,
>>> which would mean that pinctrl calls need to be done on probe() and, if
>>> necessary, during suspend()/resume().
>>
>>
>> Per-driver resource management is certainly convenient when you are
>> dealing with a single system, but it becomes difficult to maintain for
>> drivers that are shared among many platforms.
> 
> why ? look at drivers/usb/dwc3/, we're using that on OMAP, exynos, PCIe
> and a couple of different FPGA implementations inside TI. Not to mention
> what other licensees of that IP core might have internally.
> 
> So far no problesm with resources at all.
> 
> We have frameworks exactly to hide the differences.
> 
>> The industry trend for many years has been consolidation around a single
>> programming model per class of device.  For example, SDHCI, EHCI, ATA.
>> This trend will only accelerate, as the cost of developing controller IP
>> and associated drivers increases.  Such drivers need to be as
>> platform-agnostic as possible.
> 
> that's why we have pinctrl framework to abstract the details about pin
> muxing.
> 

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 599 bytes --]

Hi,

(please don't top-post)

On Tue, Oct 23, 2012 at 07:51:22AM -1000, Mitch Bradley wrote:
> Perhaps I misunderstood what you were suggesting.  I thought that, when
> you said "explicitly manage all their resources", you meant that the
> driver should know the platform-specific details about clocks and power
> domains.  That is one possible interpretation of the word "explicit".

we had two suggestions in the thread:

1) handle it in driver source code (explict)
2) handle at bus notifiers level (hidden)

archives would've helped you clear up that confusion ;-)

-- 
balbi

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Benoit Cousson]

Hi Dimitry,

On 10/22/2012 05:50 PM, Dmitry Torokhov wrote:
> Hi Sourav,
> 
> On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote:
>> Adapt keypad to use pinctrl framework.
>>
>> Tested on omap4430 sdp with 3.7-rc1 kernel.
> 
> I do not see anything in the driver that would directly use pinctrl. Is
> there a better place to select default pin configuration; maybe when
> instantiating platform device?

Why?
The devm_pinctrl_get_select_default is using the pinctrl.
That's why it is named "get_select_default" and not "get" only.
This API ensure that the pin will be set to the default state, and this
is all we need to do during the probe.

There is no point to change the mux if the driver is not probed, because
potentially the pin can be use be another driver.
So probe is the right place to do that for my point of view. Moreover
with DT we don't have that board static config like we had before to do
that kind of pin mux init.

But, maybe I'm missing your point.

Regards,
Benoit


> 
> Thanks.
> 
>>
>> Cc: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
>> Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Sourav Poddar <sourav.poddar-l0cyMroinI0@public.gmane.org>
>> ---
>> v1->v2
>> - Added "PROBE_DEFER" check 
>>  drivers/input/keyboard/omap4-keypad.c |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
>> index c05f98c..502b832 100644
>> --- a/drivers/input/keyboard/omap4-keypad.c
>> +++ b/drivers/input/keyboard/omap4-keypad.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/input.h>
>>  #include <linux/slab.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/pinctrl/consumer.h>
>>  
>>  #include <linux/platform_data/omap4-keypad.h>
>>  
>> @@ -76,6 +77,7 @@ enum {
>>  
>>  struct omap4_keypad {
>>  	struct input_dev *input;
>> +	struct pinctrl	*pins;
>>  
>>  	void __iomem *base;
>>  	unsigned int irq;
>> @@ -298,6 +300,15 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev)
>>  		goto err_release_mem;
>>  	}
>>  
>> +	keypad_data->pins = devm_pinctrl_get_select_default(&pdev->dev);
>> +	if (IS_ERR(keypad_data->pins)) {
>> +		if (PTR_ERR(keypad_data->pins) == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;
>> +
>> +		dev_warn(&pdev->dev, "did not get pins for keypad error: %li\n",
>> +					PTR_ERR(keypad_data->pins));
>> +		keypad_data->pins = NULL;
>> +	}
>>  
>>  	/*
>>  	 * Enable clocks for the keypad module so that we can read
>> -- 
>> 1.7.1
>>
> 

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Dmitry Torokhov]

On Tue, Oct 23, 2012 at 11:18:12AM +0200, Benoit Cousson wrote:
> Hi Dimitry,
> 
> On 10/22/2012 05:50 PM, Dmitry Torokhov wrote:
> > Hi Sourav,
> > 
> > On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote:
> >> Adapt keypad to use pinctrl framework.
> >>
> >> Tested on omap4430 sdp with 3.7-rc1 kernel.
> > 
> > I do not see anything in the driver that would directly use pinctrl. Is
> > there a better place to select default pin configuration; maybe when
> > instantiating platform device?
> 
> Why?
> The devm_pinctrl_get_select_default is using the pinctrl.

No, I guess we ihave different understanding of what "directly use" means.
This particular driver does directly use interrupts: it requests it and
performs some actions when interrupt arrives. Same goes for IO memory -
it gets requested, then we access it. With pinctrl we do not do anything
- we just ask another layer to configure it and that is it.

I have seen just in a few days 3 or 4 drivers having exactly the same
change - call to devm_pinctrl_get_select_default(), and I guess I will
receive the same patches for the rest of input drivers shortly.
This suggests that the operation is done at the wrong level. Do the
pin configuration as you parse DT data, the same way you set up i2c
devices registers in of_i2c.c, and leave the individual drivers that do
not care about specifics alone.

> That's why it is named "get_select_default" and not "get" only.
> This API ensure that the pin will be set to the default state, and this
> is all we need to do during the probe.

Why during the probe and not by default? Realistically, the driver will
be loaded as long as there is a matching device and pins will need to be
configured.

> 
> There is no point to change the mux if the driver is not probed, because
> potentially the pin can be use be another driver.

What other driver would use it? Who would chose what driver to load?

Thanks.

-- 
Dmitry

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 3764 bytes --]

Hi,

On Tue, Oct 23, 2012 at 01:02:49PM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 23, 2012 at 11:18:12AM +0200, Benoit Cousson wrote:
> > Hi Dimitry,
> > 
> > On 10/22/2012 05:50 PM, Dmitry Torokhov wrote:
> > > Hi Sourav,
> > > 
> > > On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote:
> > >> Adapt keypad to use pinctrl framework.
> > >>
> > >> Tested on omap4430 sdp with 3.7-rc1 kernel.
> > > 
> > > I do not see anything in the driver that would directly use pinctrl. Is
> > > there a better place to select default pin configuration; maybe when
> > > instantiating platform device?
> > 
> > Why?
> > The devm_pinctrl_get_select_default is using the pinctrl.
> 
> No, I guess we ihave different understanding of what "directly use" means.
> This particular driver does directly use interrupts: it requests it and
> performs some actions when interrupt arrives. Same goes for IO memory -
> it gets requested, then we access it. With pinctrl we do not do anything
> - we just ask another layer to configure it and that is it.

this is true for almost anything we do:

- we ask another layer to allocate memory for us
- we ask another layer to call our ISR once the IRQ line is asserted
- we ask another layer to handle the input events we just received
- we ask another layer to transfer data through DMA for us
- we ask another layer to turn regulators on and off.

and so on. This is just how abstractions work, we group common parts in
a framework so that users don't need to know the details, but still need
to tell the framework when to fiddle with those resources.

> I have seen just in a few days 3 or 4 drivers having exactly the same
> change - call to devm_pinctrl_get_select_default(), and I guess I will
> receive the same patches for the rest of input drivers shortly.
> This suggests that the operation is done at the wrong level. Do the
> pin configuration as you parse DT data, the same way you set up i2c
> devices registers in of_i2c.c, and leave the individual drivers that do
> not care about specifics alone.

Makes no sense to hide that from drivers. The idea here is that driver
should know when it needs its pins to muxed correctly. 95% of the time
it will be done during probe() but then again, so what ?

doing that when parsing DT, or on bus notifiers is just plain wrong.
Drivers should be required to handle all of their resources.

> > That's why it is named "get_select_default" and not "get" only.
> > This API ensure that the pin will be set to the default state, and this
> > is all we need to do during the probe.
> 
> Why during the probe and not by default? Realistically, the driver will
> be loaded as long as there is a matching device and pins will need to be
> configured.

likewise memory will be allocated when matching happens, IRQs will be
allocated, regulators will be turned on. So why don't we do all that by
default ? Because it is wrong.

> > There is no point to change the mux if the driver is not probed, because
> > potentially the pin can be use be another driver.
> 
> What other driver would use it? Who would chose what driver to load?

Well, you _do_ know that on a SoC we have a limited amount of pins
right ?

Considering the amont of features which are packed inside a single die,
it's not farfetched to assume we will have a lot less pins then we
actually need, so we need muxers behind each pin in order to choose
which functionality we want.

If it happens that keypad's pins are shared with another IP (e.g. GPIO),
we need to give the final user (a OEM/ODM) the choice of using those
pins as either keypad or GPIOs, thus the need for pinctrl framework and
the calls in the drivers.

-- 
balbi

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Dmitry Torokhov]

On Wed, Oct 24, 2012 at 11:37:04AM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Oct 23, 2012 at 01:02:49PM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 23, 2012 at 11:18:12AM +0200, Benoit Cousson wrote:
> > > Hi Dimitry,
> > > 
> > > On 10/22/2012 05:50 PM, Dmitry Torokhov wrote:
> > > > Hi Sourav,
> > > > 
> > > > On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote:
> > > >> Adapt keypad to use pinctrl framework.
> > > >>
> > > >> Tested on omap4430 sdp with 3.7-rc1 kernel.
> > > > 
> > > > I do not see anything in the driver that would directly use pinctrl. Is
> > > > there a better place to select default pin configuration; maybe when
> > > > instantiating platform device?
> > > 
> > > Why?
> > > The devm_pinctrl_get_select_default is using the pinctrl.
> > 
> > No, I guess we ihave different understanding of what "directly use" means.
> > This particular driver does directly use interrupts: it requests it and
> > performs some actions when interrupt arrives. Same goes for IO memory -
> > it gets requested, then we access it. With pinctrl we do not do anything
> > - we just ask another layer to configure it and that is it.
> 
> this is true for almost anything we do:
> 
> - we ask another layer to allocate memory for us
> - we ask another layer to call our ISR once the IRQ line is asserted
> - we ask another layer to handle the input events we just received
> - we ask another layer to transfer data through DMA for us
> - we ask another layer to turn regulators on and off.

But we are _directly_ _using_ all of these. You allocate memory and you
(the driver) stuff data into that memory. You ask for DMA and you take
the DMAed data and work with it. Not so with pinctrl in omap keypad and
other drivers I have seen so far.

> 
> and so on. This is just how abstractions work, we group common parts in
> a framework so that users don't need to know the details, but still need
> to tell the framework when to fiddle with those resources.
> 
> > I have seen just in a few days 3 or 4 drivers having exactly the same
> > change - call to devm_pinctrl_get_select_default(), and I guess I will
> > receive the same patches for the rest of input drivers shortly.
> > This suggests that the operation is done at the wrong level. Do the
> > pin configuration as you parse DT data, the same way you set up i2c
> > devices registers in of_i2c.c, and leave the individual drivers that do
> > not care about specifics alone.
> 
> Makes no sense to hide that from drivers. The idea here is that driver
> should know when it needs its pins to muxed correctly.

The driver also needs memory controller to be initialized, gpio chip be
ready and registered, DMA subsystem ready, input core reade, etc, etc,
etc. You however do not have every driver explicitly initialize any of
that; you expect certain working environment to be already operable. The
driver does manage resources it controls, it has ultimate knowledge
about, pin configuration is normally is not it. We just need to know
that we wired/muxed properly, otherwise we won't work. So please let
parent layers deal with it.

> 95% of the time
> it will be done during probe() but then again, so what ?
> 
> doing that when parsing DT, or on bus notifiers is just plain wrong.
> Drivers should be required to handle all of their resources.

All of _their_ resources, exactly. They do not own nor control pins so
they should not be bothered with them either. Look, when you see that
potentially _every_ driver in the system needs to set up the same object
that it doe snot use otherwise you should realize that individual driver
is not the proper place to do that.

> 
> > > That's why it is named "get_select_default" and not "get" only.
> > > This API ensure that the pin will be set to the default state, and this
> > > is all we need to do during the probe.
> > 
> > Why during the probe and not by default? Realistically, the driver will
> > be loaded as long as there is a matching device and pins will need to be
> > configured.
> 
> likewise memory will be allocated when matching happens, IRQs will be
> allocated, regulators will be turned on. So why don't we do all that by
> default ? Because it is wrong.

No, because we do not know how. The generic layer does not know the ISR
to install, how much memory to allocate, etc. Having regulator turned on
before getting to probe might not be a bad idea.

> 
> > > There is no point to change the mux if the driver is not probed, because
> > > potentially the pin can be use be another driver.
> > 
> > What other driver would use it? Who would chose what driver to load?
> 
> Well, you _do_ know that on a SoC we have a limited amount of pins
> right ?
> 
> Considering the amont of features which are packed inside a single die,
> it's not farfetched to assume we will have a lot less pins then we
> actually need, so we need muxers behind each pin in order to choose
> which functionality we want.
> 
> If it happens that keypad's pins are shared with another IP (e.g. GPIO),
> we need to give the final user (a OEM/ODM) the choice of using those
> pins as either keypad or GPIOs, thus the need for pinctrl framework and
> the calls in the drivers.

Right, so please walk me through, step by step, how an OEM/ODM woudl
select a particular configuration. Do you expect it to happen at
runtime, or do you expect relevant data be put in DT?

Thanks.

-- 
Dmitry

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Wed, Oct 24, 2012 at 6:14 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Oct 24, 2012 at 11:37:04AM +0300, Felipe Balbi wrote:

>> - we ask another layer to allocate memory for us
>> - we ask another layer to call our ISR once the IRQ line is asserted
>> - we ask another layer to handle the input events we just received
>> - we ask another layer to transfer data through DMA for us
>> - we ask another layer to turn regulators on and off.
>
> But we are _directly_ _using_ all of these. You allocate memory and you
> (the driver) stuff data into that memory. You ask for DMA and you take
> the DMAed data and work with it. Not so with pinctrl in omap keypad and
> other drivers I have seen so far.

Consult:
drivers/tty/serial/amba-pl011.c
drivers/spi/spi-pl022.c
drivers/i2c/busses/i2c-nomadik.c

for more complex pinctrl use cases. These are my dogfood drivers ...
Most of these will request more than one state and switch the driver
between these different states at runtime, in these examples for power
saving there are states named "default", "sleep" and in the I2C driver
also "idle".

These examples are more typical to how the ux500 platform will
look, also the SKE input driver will move the devise to sleep/default
states but we need to merge PM code before we can do that.

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Dmitry Torokhov]

On Wednesday, October 24, 2012 06:51:47 PM Linus Walleij wrote:
> On Wed, Oct 24, 2012 at 6:14 PM, Dmitry Torokhov
> 
> <dmitry.torokhov@gmail.com> wrote:
> > On Wed, Oct 24, 2012 at 11:37:04AM +0300, Felipe Balbi wrote:
> >> - we ask another layer to allocate memory for us
> >> - we ask another layer to call our ISR once the IRQ line is asserted
> >> - we ask another layer to handle the input events we just received
> >> - we ask another layer to transfer data through DMA for us
> >> - we ask another layer to turn regulators on and off.
> > 
> > But we are _directly_ _using_ all of these. You allocate memory and you
> > (the driver) stuff data into that memory. You ask for DMA and you take
> > the DMAed data and work with it. Not so with pinctrl in omap keypad and
> > other drivers I have seen so far.
> 
> Consult:
> drivers/tty/serial/amba-pl011.c

OK.

> drivers/spi/spi-pl022.c

Default/sleep transitions could be moved into bus code.

> drivers/i2c/busses/i2c-nomadik.c

Don't see pinctrl in linux-next.

 
> for more complex pinctrl use cases. These are my dogfood drivers ...
> Most of these will request more than one state and switch the driver
> between these different states at runtime, in these examples for power
> saving there are states named "default", "sleep" and in the I2C driver
> also "idle".
> 
> These examples are more typical to how the ux500 platform will
> look, also the SKE input driver will move the devise to sleep/default
> states but we need to merge PM code before we can do that.

I do not say that no drivers should ever touch pinctrl, just that most
of them do not have to if you have other layers to the right thing for
them.

Thanks.

-- 
Dmitry

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

Hi,

On Wed, Oct 24, 2012 at 10:28:46AM -0700, Dmitry Torokhov wrote:
> > for more complex pinctrl use cases. These are my dogfood drivers ...
> > Most of these will request more than one state and switch the driver
> > between these different states at runtime, in these examples for power
> > saving there are states named "default", "sleep" and in the I2C driver
> > also "idle".
> > 
> > These examples are more typical to how the ux500 platform will
> > look, also the SKE input driver will move the devise to sleep/default
> > states but we need to merge PM code before we can do that.
> 
> I do not say that no drivers should ever touch pinctrl, just that most
> of them do not have to if you have other layers to the right thing for
> them.

It will be a much bigger mess. Some drivers don't need to care about
pinctrl because drivers/base handle it for them, while some others will
need a way to tell drivers/base "hey, don't touch pinctrl at all because
I know what I'm doing" and that has to happen before probe() too,
otherwise it's already too late and, according to what you suggest,
drivers/base will already have touched pinctrl. The only way I see would
be to add an extra "dont_touch_my_pins" field to every driver structure
in the kernel. Clearly what you say is nonsense.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

On Wed, Oct 24, 2012 at 09:58:19PM +0300, Felipe Balbi wrote:

> need a way to tell drivers/base "hey, don't touch pinctrl at all because
> I know what I'm doing" and that has to happen before probe() too,
> otherwise it's already too late and, according to what you suggest,
> drivers/base will already have touched pinctrl. The only way I see would
> be to add an extra "dont_touch_my_pins" field to every driver structure
> in the kernel. Clearly what you say is nonsense.

I suspect that's not actually a big deal and that if we went down this
route we'd have the driver take over control from the core code during
probe() with the core still setting up the default state.

Personally I do think we want to be factoring bolierplate out of
drivers, if they're not doing anything constructive with pinctrl they
should be able to avoid having code for it.  There definitely are issues
to work through but it seems like we ought to be able to do something.

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

Hi,

On Thu, Oct 25, 2012 at 09:59:01PM +0100, Mark Brown wrote:
> On Wed, Oct 24, 2012 at 09:58:19PM +0300, Felipe Balbi wrote:
> 
> > need a way to tell drivers/base "hey, don't touch pinctrl at all because
> > I know what I'm doing" and that has to happen before probe() too,
> > otherwise it's already too late and, according to what you suggest,
> > drivers/base will already have touched pinctrl. The only way I see would
> > be to add an extra "dont_touch_my_pins" field to every driver structure
> > in the kernel. Clearly what you say is nonsense.
> 
> I suspect that's not actually a big deal and that if we went down this
> route we'd have the driver take over control from the core code during
> probe() with the core still setting up the default state.
> 
> Personally I do think we want to be factoring bolierplate out of
> drivers, if they're not doing anything constructive with pinctrl they
> should be able to avoid having code for it.  There definitely are issues
> to work through but it seems like we ought to be able to do something.

IMHO this will come back to bite you in the *ss. Specially when the same
driver is shared among multiple revisions of the same SoC or multiple
different SoCs.

Hypothetical situation: OMAP4 has keypad as the default pin mode and low
power is handled by the HW, so keypad could have pinctlr "boilerplate"
factored out. Then comes OMAP5 and low power mode has to be handled by
SW for whatever reason (maybe there are more than one low power mode).
Then we will need to patch omap4-keypad.c to remove "dont_touch_my_pins"
flag and add pinctrl support.

When you think of the possibilities of every single driver going
throught that it sounds a lot nicer to not make that decision IMHO and
keep pinctrl explicit.

This is not like module_*_driver() macro.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]

On Fri, Oct 26, 2012 at 09:20:36AM +0300, Felipe Balbi wrote:
> On Thu, Oct 25, 2012 at 09:59:01PM +0100, Mark Brown wrote:

> > I suspect that's not actually a big deal and that if we went down this
> > route we'd have the driver take over control from the core code during
> > probe() with the core still setting up the default state.

> > Personally I do think we want to be factoring bolierplate out of
> > drivers, if they're not doing anything constructive with pinctrl they
> > should be able to avoid having code for it.  There definitely are issues
> > to work through but it seems like we ought to be able to do something.

> IMHO this will come back to bite you in the *ss. Specially when the same
> driver is shared among multiple revisions of the same SoC or multiple
> different SoCs.

I'm not entirely sure you fully understood the proposal...

> Hypothetical situation: OMAP4 has keypad as the default pin mode and low
> power is handled by the HW, so keypad could have pinctlr "boilerplate"
> factored out. Then comes OMAP5 and low power mode has to be handled by
> SW for whatever reason (maybe there are more than one low power mode).
> Then we will need to patch omap4-keypad.c to remove "dont_touch_my_pins"
> flag and add pinctrl support.

This isn't going to make any practical difference at all, as soon as the
driver starts using pinctrl explicitly a flag gets set in the driver and
the default code does nothing more.  The only difference will be that we
may get a default configuration applied prior to probe.

You could have the driver explicitly set the flag, it would just be one
extra line, but it seems marginally nicer to just do it.

> When you think of the possibilities of every single driver going
> throught that it sounds a lot nicer to not make that decision IMHO and
> keep pinctrl explicit.

I'm just not seeing any impact on drivers that do something interesting
here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4147 bytes --]

Hi,

On Fri, Oct 26, 2012 at 05:03:16PM +0100, Mark Brown wrote:
> On Fri, Oct 26, 2012 at 09:20:36AM +0300, Felipe Balbi wrote:
> > On Thu, Oct 25, 2012 at 09:59:01PM +0100, Mark Brown wrote:
> 
> > > I suspect that's not actually a big deal and that if we went down this
> > > route we'd have the driver take over control from the core code during
> > > probe() with the core still setting up the default state.
> 
> > > Personally I do think we want to be factoring bolierplate out of
> > > drivers, if they're not doing anything constructive with pinctrl they
> > > should be able to avoid having code for it.  There definitely are issues
> > > to work through but it seems like we ought to be able to do something.
> 
> > IMHO this will come back to bite you in the *ss. Specially when the same
> > driver is shared among multiple revisions of the same SoC or multiple
> > different SoCs.
> 
> I'm not entirely sure you fully understood the proposal...
> 
> > Hypothetical situation: OMAP4 has keypad as the default pin mode and low
> > power is handled by the HW, so keypad could have pinctlr "boilerplate"
> > factored out. Then comes OMAP5 and low power mode has to be handled by
> > SW for whatever reason (maybe there are more than one low power mode).
> > Then we will need to patch omap4-keypad.c to remove "dont_touch_my_pins"
> > flag and add pinctrl support.
> 
> This isn't going to make any practical difference at all, as soon as the
> driver starts using pinctrl explicitly a flag gets set in the driver and
> the default code does nothing more.  The only difference will be that we
> may get a default configuration applied prior to probe.
>
> You could have the driver explicitly set the flag, it would just be one
> extra line, but it seems marginally nicer to just do it.

You didn't get the whole picture I'm afraid. Consider the situation
where the same e.g. keypad driver is being used on OMAP4 and OMAP5 and
those have different requirements towards pinctrl.

Let's just assume for the sake of argument that OMAP4 would set the flag
on the driver structure which would tell drivers/base/ to handle pinctrl
default automatically and let's assume that's just fine for OMAP4 since
sleep mode is handled by HW (all of this is just for the sake of
argument, I'm not claiming this is how OMAP4/5 work).

Now, we need to add OMAP5 support *to the same keypad driver*.
Unfortunately, OMAP5 needs to handle pinctrl explicitly for whatever
reason (SW-controlled sleep mode, errata fix, whatever).

This will mean that we will have to remove the flag from the keypad
driver because that's not valid anymore for OMAP5.

What I'm claiming below is that quite possibly all drivers will go
through that:

- start with transparent pinctrl default mode by letting drivers/base/
  automagically take care of that for us.

- remove the 'handle-pinctrl-automatically-for-me' flag because of new
  requirements on different version of the IP.

This is why I think hiding things from drivers makes no sense. Also
consider the situations Linus W exposed on another subthread. If you
change ordering of certain calls, you will really break the
functionality of the IP. Because we can't make sure this won't work
automagically in all cases (just like we can't make sure $size memory
allocation is enough for all drivers) we don't hide that from the
driver. We require driver to manage its resources properly.

> > When you think of the possibilities of every single driver going
> > throught that it sounds a lot nicer to not make that decision IMHO and
> > keep pinctrl explicit.
> 
> I'm just not seeing any impact on drivers that do something interesting
> here.

that's because you didn't consider enough possibilities. See above.

How can you make sure that this will work for at least 50% of the
drivers ? You just can't. We don't know the implementation details of
every arch/soc/platform supported by Linux today to make that decision.

IMHO, it's best to require drivers to explicitly setup pin muxing by
calling into pinctrl framework.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]

On Mon, Oct 29, 2012 at 09:49:01PM +0200, Felipe Balbi wrote:
> On Fri, Oct 26, 2012 at 05:03:16PM +0100, Mark Brown wrote:

> > You could have the driver explicitly set the flag, it would just be one
> > extra line, but it seems marginally nicer to just do it.

> You didn't get the whole picture I'm afraid. Consider the situation
> where the same e.g. keypad driver is being used on OMAP4 and OMAP5 and
> those have different requirements towards pinctrl.

No, I'm pretty certain that I do.

> Now, we need to add OMAP5 support *to the same keypad driver*.
> Unfortunately, OMAP5 needs to handle pinctrl explicitly for whatever
> reason (SW-controlled sleep mode, errata fix, whatever).

> This will mean that we will have to remove the flag from the keypad
> driver because that's not valid anymore for OMAP5.

This isn't a problem; either the pinctrl code for OMAP5 will do
something sensible for OMAP4 in which case there's no difference to the
resulting code or you're going to have to have conditional code for the
two devices anyway and you're no worse off.

> This is why I think hiding things from drivers makes no sense. Also
> consider the situations Linus W exposed on another subthread. If you
> change ordering of certain calls, you will really break the
> functionality of the IP. Because we can't make sure this won't work
> automagically in all cases (just like we can't make sure $size memory
> allocation is enough for all drivers) we don't hide that from the
> driver. We require driver to manage its resources properly.

We need some place to put the SoC integration; power domains seem like
the obvious place to me but YMMV.  Nothing about having this out of the
drivers requires that this be done by individual subsystems in isolation
from each other.  Half the point here is that for the reusable IPs this
stuff often isn't driver specific at all, it's often more about the SoC
integration than it is about the driver and so you'll get a consistent
pattern for most IPs on the SoC.

> How can you make sure that this will work for at least 50% of the
> drivers ? You just can't. We don't know the implementation details of
> every arch/soc/platform supported by Linux today to make that decision.

Well, we've managed to get along for rather a long time with essentially
all architectures implementing this stuff by doing static setup for the
pins on boot.  That does suggest we can get a reasonably long way with
something simple, and it does seem to match up with how things usually
look at an electrical level too.

It seems fairly obvious that if we need to add identical bolier plate
code to lots of drivers we're doing something wrong, it's just churn for
little practical gain and a problem if we ever decide to change how this
stuff works in the future.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3545 bytes --]

Hi,

On Tue, Oct 30, 2012 at 11:24:10AM +0000, Mark Brown wrote:
> > This is why I think hiding things from drivers makes no sense. Also
> > consider the situations Linus W exposed on another subthread. If you
> > change ordering of certain calls, you will really break the
> > functionality of the IP. Because we can't make sure this won't work
> > automagically in all cases (just like we can't make sure $size memory
> > allocation is enough for all drivers) we don't hide that from the
> > driver. We require driver to manage its resources properly.
> 
> We need some place to put the SoC integration; power domains seem like
> the obvious place to me but YMMV.  Nothing about having this out of the

except that pin muxing has nothing to do with power domain. To me that
sounds like an abuse of the API.

> drivers requires that this be done by individual subsystems in isolation
> from each other.  Half the point here is that for the reusable IPs this
> stuff often isn't driver specific at all, it's often more about the SoC
> integration than it is about the driver and so you'll get a consistent
> pattern for most IPs on the SoC.

and all of that SoC-specific detail is already hidden behind power
domains, runtime PM, pinctrl, clk API, regulator framework, etc.

> > How can you make sure that this will work for at least 50% of the
> > drivers ? You just can't. We don't know the implementation details of
> > every arch/soc/platform supported by Linux today to make that decision.
> 
> Well, we've managed to get along for rather a long time with essentially
> all architectures implementing this stuff by doing static setup for the
> pins on boot.  That does suggest we can get a reasonably long way with

and this is one of the issues we're all trying to solve today so we have
single zImage approach for the ARM port.

> something simple, and it does seem to match up with how things usually
> look at an electrical level too.

simple ? I really doubt it. Just look at the amount of code duplication
the ARM port had (still has in some places) to handle platform-specific
details.

It turned out that drivers weren't very portable when they had to do
platform-specific initialization, we were all abusing platform_data to
pass strings and/or function pointers down to drivers and so on.

I'm concerned if we hide pinctrl under e.g. power domains (as said
above, it sounds like an abuse of the API to me) we will end up with a
situation like above. Maybe not as bad, but still weird-looking.

> It seems fairly obvious that if we need to add identical bolier plate
> code to lots of drivers we're doing something wrong, it's just churn for
> little practical gain and a problem if we ever decide to change how this
> stuff works in the future.

I wouldn't consider it boilerplate if you remember that each driver
might have different requirements regarding how all of those details
need to be handled.

We have to consider power consumption, ordering of calls, proper IP
setup, IP reuse on multiple platforms (even multiple ARCHes), etc etc,
and to get that right outside of the driver - who's the only class that
actually knows what it needs to do with its resources - will just be too
complex and error-prone.

I would strongly suggest starting with explicit calls to pinctrl, clk
API, etc and if we can really prove later that 95% of the users are
"standard", then we can factor code out, but making that assumption now
is quite risky IMHO.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4702 bytes --]

On Tue, Oct 30, 2012 at 01:49:49PM +0200, Felipe Balbi wrote:
> On Tue, Oct 30, 2012 at 11:24:10AM +0000, Mark Brown wrote:

> > We need some place to put the SoC integration; power domains seem like
> > the obvious place to me but YMMV.  Nothing about having this out of the

> except that pin muxing has nothing to do with power domain. To me that
> sounds like an abuse of the API.

Well, we can call the API Archibald if we like...  what I mean is
something that sits in the system below the device in pretty much
exactly the way that power domains do.

> > drivers requires that this be done by individual subsystems in isolation
> > from each other.  Half the point here is that for the reusable IPs this
> > stuff often isn't driver specific at all, it's often more about the SoC
> > integration than it is about the driver and so you'll get a consistent
> > pattern for most IPs on the SoC.

> and all of that SoC-specific detail is already hidden behind power
> domains, runtime PM, pinctrl, clk API, regulator framework, etc.

That's all getting rather open coded, especially if you're going to get
down to a situation where you have varying ordering constraints between
the different APIs on different SoCs.

> > > How can you make sure that this will work for at least 50% of the
> > > drivers ? You just can't. We don't know the implementation details of
> > > every arch/soc/platform supported by Linux today to make that decision.

> > Well, we've managed to get along for rather a long time with essentially
> > all architectures implementing this stuff by doing static setup for the
> > pins on boot.  That does suggest we can get a reasonably long way with

> and this is one of the issues we're all trying to solve today so we have
> single zImage approach for the ARM port.

I don't see the relevance of single zImage here; device tree is supposed
to solve that one.

> > something simple, and it does seem to match up with how things usually
> > look at an electrical level too.

> simple ? I really doubt it. Just look at the amount of code duplication
> the ARM port had (still has in some places) to handle platform-specific
> details.

What I'm saying here is that I'm concerned about us ending up with more
code duplication...

> It turned out that drivers weren't very portable when they had to do
> platform-specific initialization, we were all abusing platform_data to
> pass strings and/or function pointers down to drivers and so on.

> I'm concerned if we hide pinctrl under e.g. power domains (as said
> above, it sounds like an abuse of the API to me) we will end up with a
> situation like above. Maybe not as bad, but still weird-looking.

Well, nothing's going to stop that happening if people are determined
enough - one could equally see that there'll be flags getting passed to
control the ordering of calls if things are open coded.  I would expect
that with a power domain style approach any data would be being passed
directly and bypassing the driver completely.

> > It seems fairly obvious that if we need to add identical bolier plate
> > code to lots of drivers we're doing something wrong, it's just churn for
> > little practical gain and a problem if we ever decide to change how this
> > stuff works in the future.

> I wouldn't consider it boilerplate if you remember that each driver
> might have different requirements regarding how all of those details
> need to be handled.

Essentially all the patches I'm seeing adding pinctrl are totally
mindless, most of them are just doing the initial setup on boot and not
doing any active management at runtime at all.

> We have to consider power consumption, ordering of calls, proper IP
> setup, IP reuse on multiple platforms (even multiple ARCHes), etc etc,
> and to get that right outside of the driver - who's the only class that
> actually knows what it needs to do with its resources - will just be too
> complex and error-prone.

A big part of my point here is that it's not at all clear to me that it
is the driver which knows what's going on.  For SoC-specific IPs you can
be confident about how the SoC integration looks but for IPs that get
reused widely this becomes less true.  

> I would strongly suggest starting with explicit calls to pinctrl, clk
> API, etc and if we can really prove later that 95% of the users are
> "standard", then we can factor code out, but making that assumption now
> is quite risky IMHO.

Like I say I think we're already seeing a pattern here, the code going
into most drivers looks *very* similar with lots of the drivers simply
calling get_select_default().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Tue, Oct 30, 2012 at 3:07 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> Essentially all the patches I'm seeing adding pinctrl are totally
> mindless, most of them are just doing the initial setup on boot and not
> doing any active management at runtime at all.

None of the Ux500 pinctrl patches are like that.

I concludes in some other mails that all Ux500 drivers
will need to handle atleast two states (default and sleep)
and some a third state (idle).

And this is what we're doing in our patches.

Arguably it can all be pushed to power domains, but that
will as said mandate all affected systems to implement
power domains, and effectively moving code from
drivers/* to arch/arm/* in our case atleast.

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

On Tue, Oct 30, 2012 at 03:16:02PM +0100, Linus Walleij wrote:
> On Tue, Oct 30, 2012 at 3:07 PM, Mark Brown

> > Essentially all the patches I'm seeing adding pinctrl are totally
> > mindless, most of them are just doing the initial setup on boot and not
> > doing any active management at runtime at all.

> I concludes in some other mails that all Ux500 drivers
> will need to handle atleast two states (default and sleep)
> and some a third state (idle).

Right, that's the other common option and obviously it's just an
extension of the simple hogs which maps very nicely onto the existing PM
states for devices.

> And this is what we're doing in our patches.

> Arguably it can all be pushed to power domains, but that
> will as said mandate all affected systems to implement
> power domains, and effectively moving code from
> drivers/* to arch/arm/* in our case atleast.

As soon as they start adding clock support and so on, yes.  Obviously if
they don't want to use any of the features that are offloaded like this
then they could happily ignore it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6237 bytes --]

Hi,

On Tue, Oct 30, 2012 at 02:07:15PM +0000, Mark Brown wrote:
> On Tue, Oct 30, 2012 at 01:49:49PM +0200, Felipe Balbi wrote:
> > On Tue, Oct 30, 2012 at 11:24:10AM +0000, Mark Brown wrote:
> 
> > > We need some place to put the SoC integration; power domains seem like
> > > the obvious place to me but YMMV.  Nothing about having this out of the
> 
> > except that pin muxing has nothing to do with power domain. To me that
> > sounds like an abuse of the API.
> 
> Well, we can call the API Archibald if we like...  what I mean is

I'm sure you know it's not at all about the name and much more about the
semantics ;-)

> something that sits in the system below the device in pretty much
> exactly the way that power domains do.
> 
> > > drivers requires that this be done by individual subsystems in isolation
> > > from each other.  Half the point here is that for the reusable IPs this
> > > stuff often isn't driver specific at all, it's often more about the SoC
> > > integration than it is about the driver and so you'll get a consistent
> > > pattern for most IPs on the SoC.
> 
> > and all of that SoC-specific detail is already hidden behind power
> > domains, runtime PM, pinctrl, clk API, regulator framework, etc.
> 
> That's all getting rather open coded, especially if you're going to get
> down to a situation where you have varying ordering constraints between
> the different APIs on different SoCs.

however we need to consider those cases right ? Otherwise we will endup
pushing something to mainline which will have to be reverted couple
weeks later after a big rant from Linus ;-)

> > > > How can you make sure that this will work for at least 50% of the
> > > > drivers ? You just can't. We don't know the implementation details of
> > > > every arch/soc/platform supported by Linux today to make that decision.
> 
> > > Well, we've managed to get along for rather a long time with essentially
> > > all architectures implementing this stuff by doing static setup for the
> > > pins on boot.  That does suggest we can get a reasonably long way with
> 
> > and this is one of the issues we're all trying to solve today so we have
> > single zImage approach for the ARM port.
> 
> I don't see the relevance of single zImage here; device tree is supposed
> to solve that one.

DT is part of the deal. DT alone will solve nothing.

> > > something simple, and it does seem to match up with how things usually
> > > look at an electrical level too.
> 
> > simple ? I really doubt it. Just look at the amount of code duplication
> > the ARM port had (still has in some places) to handle platform-specific
> > details.
> 
> What I'm saying here is that I'm concerned about us ending up with more
> code duplication...

a fair concern.

> > It turned out that drivers weren't very portable when they had to do
> > platform-specific initialization, we were all abusing platform_data to
> > pass strings and/or function pointers down to drivers and so on.
> 
> > I'm concerned if we hide pinctrl under e.g. power domains (as said
> > above, it sounds like an abuse of the API to me) we will end up with a
> > situation like above. Maybe not as bad, but still weird-looking.
> 
> Well, nothing's going to stop that happening if people are determined
> enough - one could equally see that there'll be flags getting passed to
> control the ordering of calls if things are open coded.  I would expect
> that with a power domain style approach any data would be being passed
> directly and bypassing the driver completely.

situations like that would be a lot more rare in open coded case, don't
you think ? Also a lot more local, since they will show up on a driver
source code which is used in a small set of use cases, instead of being
part of the pm domain implementation for the entire platform.

> > > It seems fairly obvious that if we need to add identical bolier plate
> > > code to lots of drivers we're doing something wrong, it's just churn for
> > > little practical gain and a problem if we ever decide to change how this
> > > stuff works in the future.
> 
> > I wouldn't consider it boilerplate if you remember that each driver
> > might have different requirements regarding how all of those details
> > need to be handled.
> 
> Essentially all the patches I'm seeing adding pinctrl are totally
> mindless, most of them are just doing the initial setup on boot and not
> doing any active management at runtime at all.

have you considered that might be just a first step ? I have mentioned
this before. We first add the bare minimum and work on PM optimizations
later. You can be sure most likely those mindless patches you're seeing
are probably building blocks for upcoming patches adding sleep/idle
modes.

> > We have to consider power consumption, ordering of calls, proper IP
> > setup, IP reuse on multiple platforms (even multiple ARCHes), etc etc,
> > and to get that right outside of the driver - who's the only class that
> > actually knows what it needs to do with its resources - will just be too
> > complex and error-prone.
> 
> A big part of my point here is that it's not at all clear to me that it
> is the driver which knows what's going on.  For SoC-specific IPs you can
> be confident about how the SoC integration looks but for IPs that get
> reused widely this becomes less true.  

I don't think so. As long as we keep the meaning of the 'default'
pinctrl state to mean that this is the working state for the IP,
underlying pinctrl-$arch implementation should know how to set muxing up
properly, no ?

> > I would strongly suggest starting with explicit calls to pinctrl, clk
> > API, etc and if we can really prove later that 95% of the users are
> > "standard", then we can factor code out, but making that assumption now
> > is quite risky IMHO.
> 
> Like I say I think we're already seeing a pattern here, the code going
> into most drivers looks *very* similar with lots of the drivers simply
> calling get_select_default().

might be true, but we don't really know if those aren't building blocks
for upcoming PM optimizations IMO.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3734 bytes --]

On Tue, Oct 30, 2012 at 05:16:42PM +0200, Felipe Balbi wrote:
> On Tue, Oct 30, 2012 at 02:07:15PM +0000, Mark Brown wrote:

> > > and all of that SoC-specific detail is already hidden behind power
> > > domains, runtime PM, pinctrl, clk API, regulator framework, etc.

> > That's all getting rather open coded, especially if you're going to get
> > down to a situation where you have varying ordering constraints between
> > the different APIs on different SoCs.

> however we need to consider those cases right ? Otherwise we will endup
> pushing something to mainline which will have to be reverted couple
> weeks later after a big rant from Linus ;-)

I'm not sure there's much risk of that either way; if anything it seems
like it ought to be cleaner to have the stuff owned by the SoCs.

> > > and this is one of the issues we're all trying to solve today so we have
> > > single zImage approach for the ARM port.

> > I don't see the relevance of single zImage here; device tree is supposed
> > to solve that one.

> DT is part of the deal. DT alone will solve nothing.

If DT isn't relevant I'm not sure what you're saying about single
zImage?  The only relevance I can see for that is the data and
configuration bloating the kernel, something that DT is supposed to
address - this is the main use case where DT has benefits.

> > Well, nothing's going to stop that happening if people are determined
> > enough - one could equally see that there'll be flags getting passed to
> > control the ordering of calls if things are open coded.  I would expect
> > that with a power domain style approach any data would be being passed
> > directly and bypassing the driver completely.

> situations like that would be a lot more rare in open coded case, don't
> you think ? Also a lot more local, since they will show up on a driver
> source code which is used in a small set of use cases, instead of being
> part of the pm domain implementation for the entire platform.

I don't see how open coding helps prevent people doing silly things, it
seems like it'd have at most neutral impact (and of course it does
require going round all the drivers every time someone comes up with a
new idea for doing things which is a bit tedious).

> > Essentially all the patches I'm seeing adding pinctrl are totally
> > mindless, most of them are just doing the initial setup on boot and not
> > doing any active management at runtime at all.

> have you considered that might be just a first step ? I have mentioned
> this before. We first add the bare minimum and work on PM optimizations
> later. You can be sure most likely those mindless patches you're seeing
> are probably building blocks for upcoming patches adding sleep/idle
> modes.

The sleep/idle modes are just a basic extension of the same idea, I'd
not anticipate much difference there (and indeed anything where pinmux
power saving makes a meaningful impact will I suspect need to be using
runtime PM to allow SoC wide power savings anyway).

> > A big part of my point here is that it's not at all clear to me that it
> > is the driver which knows what's going on.  For SoC-specific IPs you can
> > be confident about how the SoC integration looks but for IPs that get
> > reused widely this becomes less true.  

> I don't think so. As long as we keep the meaning of the 'default'
> pinctrl state to mean that this is the working state for the IP,
> underlying pinctrl-$arch implementation should know how to set muxing up
> properly, no ?

But then this comes round to the mindless code that ought to be factored
out :)  Only the more interesting cases that do something unusual really
register here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3651 bytes --]

Hi,

On Tue, Oct 30, 2012 at 03:58:21PM +0000, Mark Brown wrote:
> > > > and this is one of the issues we're all trying to solve today so we have
> > > > single zImage approach for the ARM port.
> 
> > > I don't see the relevance of single zImage here; device tree is supposed
> > > to solve that one.
> 
> > DT is part of the deal. DT alone will solve nothing.
> 
> If DT isn't relevant I'm not sure what you're saying about single

I didn't say DT is irrelevant. I said it's not the *only* thing.

> zImage?  The only relevance I can see for that is the data and
> configuration bloating the kernel, something that DT is supposed to
> address - this is the main use case where DT has benefits.
> 
> > > Well, nothing's going to stop that happening if people are determined
> > > enough - one could equally see that there'll be flags getting passed to
> > > control the ordering of calls if things are open coded.  I would expect
> > > that with a power domain style approach any data would be being passed
> > > directly and bypassing the driver completely.
> 
> > situations like that would be a lot more rare in open coded case, don't
> > you think ? Also a lot more local, since they will show up on a driver
> > source code which is used in a small set of use cases, instead of being
> > part of the pm domain implementation for the entire platform.
> 
> I don't see how open coding helps prevent people doing silly things, it
> seems like it'd have at most neutral impact (and of course it does
> require going round all the drivers every time someone comes up with a
> new idea for doing things which is a bit tedious).
> 
> > > Essentially all the patches I'm seeing adding pinctrl are totally
> > > mindless, most of them are just doing the initial setup on boot and not
> > > doing any active management at runtime at all.
> 
> > have you considered that might be just a first step ? I have mentioned
> > this before. We first add the bare minimum and work on PM optimizations
> > later. You can be sure most likely those mindless patches you're seeing
> > are probably building blocks for upcoming patches adding sleep/idle
> > modes.
> 
> The sleep/idle modes are just a basic extension of the same idea, I'd
> not anticipate much difference there (and indeed anything where pinmux
> power saving makes a meaningful impact will I suspect need to be using
> runtime PM to allow SoC wide power savings anyway).
> 
> > > A big part of my point here is that it's not at all clear to me that it
> > > is the driver which knows what's going on.  For SoC-specific IPs you can
> > > be confident about how the SoC integration looks but for IPs that get
> > > reused widely this becomes less true.  
> 
> > I don't think so. As long as we keep the meaning of the 'default'
> > pinctrl state to mean that this is the working state for the IP,
> > underlying pinctrl-$arch implementation should know how to set muxing up
> > properly, no ?
> 
> But then this comes round to the mindless code that ought to be factored
> out :)  Only the more interesting cases that do something unusual really
> register here.

fair enough. I see your point. Not saying I agree though, just that this
discussion has been flying for far too long, so feel free to provide
patches implementing what you're defending here ;-)

Guess code will speak for itself. On way or another, we need OMAP keypad
driver working in mainline and I don't think your arguments are strong
enough to keep $SUBJECT from being merged, unless you can provide
something stable/tested for v3.8 merge window.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Dmitry Torokhov]

On Tue, Oct 30, 2012 at 07:25:13PM +0200, Felipe Balbi wrote:
> On Tue, Oct 30, 2012 at 03:58:21PM +0000, Mark Brown wrote:
> > 
> > But then this comes round to the mindless code that ought to be factored
> > out :)  Only the more interesting cases that do something unusual really
> > register here.
> 
> fair enough. I see your point. Not saying I agree though, just that this
> discussion has been flying for far too long, so feel free to provide
> patches implementing what you're defending here ;-)
> 
> Guess code will speak for itself. On way or another, we need OMAP keypad
> driver working in mainline

Are you saying that introducing pincrtl infrastructure actually _broke_
the driver in mainline?

Thanks.

-- 
Dmitry

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

Hi,

On Tue, Oct 30, 2012 at 11:20:07AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 30, 2012 at 07:25:13PM +0200, Felipe Balbi wrote:
> > On Tue, Oct 30, 2012 at 03:58:21PM +0000, Mark Brown wrote:
> > > 
> > > But then this comes round to the mindless code that ought to be factored
> > > out :)  Only the more interesting cases that do something unusual really
> > > register here.
> > 
> > fair enough. I see your point. Not saying I agree though, just that this
> > discussion has been flying for far too long, so feel free to provide
> > patches implementing what you're defending here ;-)
> > 
> > Guess code will speak for itself. On way or another, we need OMAP keypad
> > driver working in mainline
> 
> Are you saying that introducing pincrtl infrastructure actually _broke_
> the driver in mainline?

no dude, I'm saying we need pinctrl working for this driver because we
need to remove OMAP-specific MUX settings. One way or another, this has
to work.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

On Tue, Oct 30, 2012 at 07:25:13PM +0200, Felipe Balbi wrote:
> On Tue, Oct 30, 2012 at 03:58:21PM +0000, Mark Brown wrote:

> > But then this comes round to the mindless code that ought to be factored
> > out :)  Only the more interesting cases that do something unusual really
> > register here.

> fair enough. I see your point. Not saying I agree though, just that this
> discussion has been flying for far too long, so feel free to provide
> patches implementing what you're defending here ;-)

> Guess code will speak for itself. On way or another, we need OMAP keypad
> driver working in mainline and I don't think your arguments are strong
> enough to keep $SUBJECT from being merged, unless you can provide
> something stable/tested for v3.8 merge window.

Ship me an OMAP5 system and I might see what I can do :)

More seriously the amount of time we seem to have been spending recently
on changes which end up requiring us to go through essentially every
driver and add code to them (often several times) doesn't seem like
we're doing a good job here.  pinctrl is really noticable because it's
new but it's not the only thing.  As a subsystem maintainer this code
just makes me want to add new subsystem features to pull the code out of
drivers but obviously that's not something that should be being done at
the subsystem level.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Tue, Oct 30, 2012 at 7:37 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> More seriously the amount of time we seem to have been spending recently
> on changes which end up requiring us to go through essentially every
> driver and add code to them (often several times) doesn't seem like
> we're doing a good job here.

If this is your main concern you should be made aware that there
are people out there planning to supplant the existing DT probe paths
that are now being added to each and every ARM-related driver
with an ACPI probe path as ARM servers come into the picture.

> pinctrl is really noticable because it's
> new but it's not the only thing.  As a subsystem maintainer this code
> just makes me want to add new subsystem features to pull the code out of
> drivers but obviously that's not something that should be being done at
> the subsystem level.

We did manage to drag the power/voltage domain per se out
of the AMBA bus, and recommend that people (like us) do that
business using the power domains.

I think most people (including OMAP) have bought
into the concept of using the runtime PM framework and power
domains to control the power domain switches.

It's this wider concept of using the loose concept "PM resource
domains" to control also clocks and pins that is at stake, and so
far the runtime PM core people (Rafael and Magnus) has not said
much so I think we need some kind of indication from them as to
what is to happen, long-term, with drivers handling their own clocks
and pins. Should it be centralized or not? If it's to be centralized it
needs to become a large piece of infrastructure refactoring and
needs the attention of Linaro and the like to happen.

I've CC:ed a few people into this thread so we get some traction,
we need more subsystem maintainers in here.

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Rafael J. Wysocki]

On Tuesday, October 30, 2012 10:51:11 PM Linus Walleij wrote:
> On Tue, Oct 30, 2012 at 7:37 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> 
> > More seriously the amount of time we seem to have been spending recently
> > on changes which end up requiring us to go through essentially every
> > driver and add code to them (often several times) doesn't seem like
> > we're doing a good job here.
> 
> If this is your main concern you should be made aware that there
> are people out there planning to supplant the existing DT probe paths
> that are now being added to each and every ARM-related driver
> with an ACPI probe path as ARM servers come into the picture.

That's correct.

> > pinctrl is really noticable because it's
> > new but it's not the only thing.  As a subsystem maintainer this code
> > just makes me want to add new subsystem features to pull the code out of
> > drivers but obviously that's not something that should be being done at
> > the subsystem level.
> 
> We did manage to drag the power/voltage domain per se out
> of the AMBA bus, and recommend that people (like us) do that
> business using the power domains.
> 
> I think most people (including OMAP) have bought
> into the concept of using the runtime PM framework and power
> domains to control the power domain switches.
> 
> It's this wider concept of using the loose concept "PM resource
> domains" to control also clocks and pins that is at stake, and so
> far the runtime PM core people (Rafael and Magnus) has not said
> much so I think we need some kind of indication from them as to
> what is to happen, long-term, with drivers handling their own clocks
> and pins. Should it be centralized or not? If it's to be centralized it
> needs to become a large piece of infrastructure refactoring and
> needs the attention of Linaro and the like to happen.

Well, I personally think it should be centralized somehow.  I'm not quite
sure how to achieve that, though.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On Tue, Oct 30, 2012 at 10:51:11PM +0100, Linus Walleij wrote:
> On Tue, Oct 30, 2012 at 7:37 PM, Mark Brown

> > More seriously the amount of time we seem to have been spending recently
> > on changes which end up requiring us to go through essentially every
> > driver and add code to them (often several times) doesn't seem like
> > we're doing a good job here.

> If this is your main concern you should be made aware that there
> are people out there planning to supplant the existing DT probe paths
> that are now being added to each and every ARM-related driver
> with an ACPI probe path as ARM servers come into the picture.

That's different as we're adding support for a new external interface
which will need per device configuration parsing rather than
reorganising things within the kernel; I'd expect it won't supplant DT
but rather sit alongside it as it's more a requirement for the server
market than for the embedded market.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Tue, Oct 30, 2012 at 12:49 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Tue, Oct 30, 2012 at 11:24:10AM +0000, Mark Brown wrote:

>> We need some place to put the SoC integration; power domains seem like
>> the obvious place to me but YMMV.  Nothing about having this out of the
>
> except that pin muxing has nothing to do with power domain. To me that
> sounds like an abuse of the API.

It could be renamed to "power resources" or something as long as
it's related to resource handling related to the PM calls.

But I worry that it violates the Unix principle to do one thing and one
thing only.

A device power resource framework goes in the opposite direction,
trying to do a lot of unrelated things in a central place as opposed
to distributing the task.

>> drivers requires that this be done by individual subsystems in isolation
>> from each other.  Half the point here is that for the reusable IPs this
>> stuff often isn't driver specific at all, it's often more about the SoC
>> integration than it is about the driver and so you'll get a consistent
>> pattern for most IPs on the SoC.
>
> and all of that SoC-specific detail is already hidden behind power
> domains, runtime PM, pinctrl, clk API, regulator framework, etc.

I agree.

pinctrl has already done a fair job at trying to be abstract in the
states requested from the core, in <linux/pinctrl/pinctrl-state.h>.

And I accept the idea to  try to centralize more as well, maybe
as a helpful struct and some inlines for the pinctrl core. I think
this is enough, and pushing all handles into central code creates
a problem elsewhere.

(But I'm not so certain ... so I might just
change opinion one of those days depending on what
arguments will be made.)

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Wed, Oct 24, 2012 at 7:28 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

>> drivers/spi/spi-pl022.c
>
> Default/sleep transitions could be moved into bus code.

No that's not a good idea as long as we have both the platform bus
and the AMBA bus doing essentially the same thing. We will then be
having two copies of the same code in two different busses running
out of sync. There may be other busses too.

But I could prepare static helpers in <linux/pinctrl/consumer.h>
that any bus could use. Or any driver. Probably any driver,
because of this:

As noted the bus cannot really execute the pinctrl calls to
e.g. put a drivers pins into "sleep". Since if e.g. the bus is walking
the suspend() ladder, shall it put the pins into sleep *before*
or *after* calling the suspend() hook in the driver?

The answer is that it does not know. Because drivers have
different needs. Depending on how the hardware and
system is done.

I already tried to make this point:

pinctrl_set_state(state_sleep);
clk_disable();
power_off_voltage_domain();

May for some drivers have to be:

clk_disable();
power_off_voltage_domain();
pinctrl_set_state(state_sleep);

(etc)

I'm not making this up, it is a very real phenomenon on the
Ux500 and I don't think we are unique.

Moving this handling to bus code or anywhere else
invariably implies that resource acquisition/release order
does not matter, and my point is that it does.

>> drivers/i2c/busses/i2c-nomadik.c
>
> Don't see pinctrl in linux-next.

This code is here:
http://marc.info/?l=linux-i2c&m=134986995731695&w=2

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

On Sun, Oct 28, 2012 at 09:12:52PM +0100, Linus Walleij wrote:

> The answer is that it does not know. Because drivers have
> different needs. Depending on how the hardware and
> system is done.

...

> I'm not making this up, it is a very real phenomenon on the
> Ux500 and I don't think we are unique.

> Moving this handling to bus code or anywhere else
> invariably implies that resource acquisition/release order
> does not matter, and my point is that it does.

Doing this in the buses is definitely wrong, as you say it's not bus
specific.  I do however think we can usefully do this stuff in a SoC
specific place like a power domain, keeping the SoC integration code
together and out of the drivers.  IME the SoCs where you need to do
different things for different IPs shoudl mostly still get some reuse
out of such an approach.

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Tue, Oct 30, 2012 at 12:34 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sun, Oct 28, 2012 at 09:12:52PM +0100, Linus Walleij wrote:

>> Moving this handling to bus code or anywhere else
>> invariably implies that resource acquisition/release order
>> does not matter, and my point is that it does.
>
> Doing this in the buses is definitely wrong, as you say it's not bus
> specific.  I do however think we can usefully do this stuff in a SoC
> specific place like a power domain, keeping the SoC integration code
> together and out of the drivers.  IME the SoCs where you need to do
> different things for different IPs shoudl mostly still get some reuse
> out of such an approach.

Talking to Kevin Hilman today he was also stressing that
power domains is a good thing for handling resources, especially
when replacing prior hacks in the custom clk code. However
he pointed specifically to clocks and voltages, which may
be true.

What worries me is when knowledge of the hardware which
is traditionally a concern for the device driver start to
bubble up to the power domain (or better renamed resource
domain if use like this, as Felipe points out).

I worry that we will end up with power/resource domain
code that start to look like this:

suspend()
switch (device.id) {
case DEV_FOO:
  clk_disable();
  pinctrl_set_state();
  power_domain_off();
case DEV_BAR:
  pinctrl_set_state();
  clk_disable();
  // Always-on domain
case DEV_BAZ:
  pinctrl_set_state();
  clk_disable();
  power_domain_off();
case ...

Mutate the above with silicon errata, specific tweaks etc that
Felipe was mentioning.

What is happening is that device-specific behaviour which
traditionally handled in the driver is now inside the
power/resource domain.

I agree that if the domain was doing the same thing for each
piece of hardware, this would be the right thing to do,
and I think the in-kernel examples are all "simple",
e.g. arch/arm/mach-omap2/powerdomain* is all about
power domains and nothing else, and the notifiers used
by SHmobile is all about clock gating and nothing else.

Another effect is that moving all resource handling to
power domains is that if we do that for a widely shared
device driver like the PL011 that mandates that power
domains need to be implemented for U300, Ux500,
Nomadik, SPEAr 13xx, 3xx, 6xx, Versatile, Versatile
Express, Integrator (AP & CP) and BCM2835. Probably
more.

None of which has power domains (upstream) as
of today. Some of which (U300, Ux500, Nomadik,
SPEAr, Integrator, BCM2835) implement pin control.

Basically power (resource) domains have the side-effect
of in this light not doing one thing (power domains) but
instead imposing all-or-nothing imperialistic characteristics.

While avoiding a set of distributed, optional pinctrl hooks,
it mandates a central piece of upfront powerdomain
boilerplate to be present in each and every platform that
wants to control its pins.

I think the lesser of two evils is the distributed approach,
and then I'm talking about pinctrl only, disregarding the
fact that clocks and power domains are basically subject to
the same kind of argument. I still buy into the concept of
using power domains for exactly power domains only.
Arguably this is an elegance opinion...

I worry that the per-SoC power domain implementation
which will live in arch/arm/mach-* as of today will become
the new board file problem, overburdening the arch/* tree.
Maybe I'm mistaken as to the size of these things,
but just doing ls arch/arm/mach-omap2/powerdomain*
makes me start worrying.

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2127 bytes --]

On Tue, Oct 30, 2012 at 03:02:23PM +0100, Linus Walleij wrote:

> I worry that we will end up with power/resource domain
> code that start to look like this:

> suspend()
> switch (device.id) {
> case DEV_FOO:
>   clk_disable();
>   pinctrl_set_state();
>   power_domain_off();
> case DEV_BAR:

Well, if we're doing that then either we'd presumably either get the
same result if we open code it in the drivers or whoever wrote the
resource domain code for the platform should be creating multiple
domains.

> Another effect is that moving all resource handling to
> power domains is that if we do that for a widely shared
> device driver like the PL011 that mandates that power
> domains need to be implemented for U300, Ux500,
> Nomadik, SPEAr 13xx, 3xx, 6xx, Versatile, Versatile
> Express, Integrator (AP & CP) and BCM2835. Probably
> more.

> Basically power (resource) domains have the side-effect
> of in this light not doing one thing (power domains) but
> instead imposing all-or-nothing imperialistic characteristics.

For me that's happening anyway with explicit control, just in different
places - for example the Freescale guys have had issues with IPs shared
between m68k and i.MX requiring that stub clocks be provided on m68k for
things that are always on there and similarly with all the platforms
that get affected when some widely used chip acquires regulator support.

It seems easier to organise things if the platform has responsibility
for coordinating this stuff than if we add stuff in the drivers.

> I worry that the per-SoC power domain implementation
> which will live in arch/arm/mach-* as of today will become
> the new board file problem, overburdening the arch/* tree.
> Maybe I'm mistaken as to the size of these things,
> but just doing ls arch/arm/mach-omap2/powerdomain*
> makes me start worrying.

One thing to remember is that OMAP has some of the most featureful
hardware out there in terms of software control for power so it's
unlikely to ever get much more complex than that.  IME most SoCs
are very much simpler than that and should be able to punt lots of
stuff to device tree data.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Kevin Hilman]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Tue, Oct 30, 2012 at 12:34 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Sun, Oct 28, 2012 at 09:12:52PM +0100, Linus Walleij wrote:
>
>>> Moving this handling to bus code or anywhere else
>>> invariably implies that resource acquisition/release order
>>> does not matter, and my point is that it does.
>>
>> Doing this in the buses is definitely wrong, as you say it's not bus
>> specific.  I do however think we can usefully do this stuff in a SoC
>> specific place like a power domain, keeping the SoC integration code
>> together and out of the drivers.  IME the SoCs where you need to do
>> different things for different IPs shoudl mostly still get some reuse
>> out of such an approach.
>
> Talking to Kevin Hilman today he was also stressing that
> power domains is a good thing for handling resources, especially
> when replacing prior hacks in the custom clk code. However
> he pointed specifically to clocks and voltages, which may
> be true.
>
> What worries me is when knowledge of the hardware which
> is traditionally a concern for the device driver start to
> bubble up to the power domain (or better renamed resource
> domain if use like this, as Felipe points out).
>
> I worry that we will end up with power/resource domain
> code that start to look like this:
>
> suspend()
> switch (device.id) {
> case DEV_FOO:
>   clk_disable();
>   pinctrl_set_state();
>   power_domain_off();
> case DEV_BAR:
>   pinctrl_set_state();
>   clk_disable();
>   // Always-on domain
> case DEV_BAZ:
>   pinctrl_set_state();
>   clk_disable();
>   power_domain_off();
> case ...
>
> Mutate the above with silicon errata, specific tweaks etc that
> Felipe was mentioning.


like this, as well as a bunch more.  This is why we have a generic
description of IP blocks (omap_hwmod) which abstracts all of these
differences and keeps the PM domain layer rather simple.

I agree with Mark.  Either you have to take care of this with
conditional code in the driver, and the drivers become bloated with a
mess of SoC integration details, or you hide it away in SoC-specific
code that can handle this, and keep the drivers portable. 

Now that we have PM domains (PM domains didn't exist when we created
omap_device/omap_hwmod), I suspect the cleanest way to do this is to
create separate PM domains for each "class" of devices that have
different set of behavior.

> What is happening is that device-specific behaviour which
> traditionally handled in the driver is now inside the
> power/resource domain.
>
> piece of hardware, this would be the right thing to do,
> and I think the in-kernel examples are all "simple",
> e.g. arch/arm/mach-omap2/powerdomain* is all about
> power domains and nothing else, 

FYI... that code isn't the same as PM domain.  That code is for the
*hardware* powerdomains, not the software concept of "PM domain."  In
OMAP, PM domain is implmented at the omap_device level.  And omap_device
is the abstraction of an IP block that knows about all the PM related
register settings, clocks, HW powerdomain, voltage domain, PM related
pin-muxing etc. etc.    All of these things are abstracted in an
omap_device, so that the PM domain implementation for OMAP looks rather
simple (c.f. omap_device_pm_domain in arch/arm/plat-omap/omap_device.c.)

Note that the callbacks just call omap_device_enable(),
omap_device_disable() and all the HW ugliness, SoC-specific integration
mess is hidden away.

[...]

> I think the lesser of two evils is the distributed approach,
> and then I'm talking about pinctrl only, disregarding the
> fact that clocks and power domains are basically subject to
> the same kind of argument. I still buy into the concept of
> using power domains for exactly power domains only.
> Arguably this is an elegance opinion...

The pinctrl examples I've seen mentioned so far are all PM related
(sleep, idle, wakeup, etc.) so to me I think they still belong in
PM domains (and that's how we handle the PM related pins in OMAP.)

> I worry that the per-SoC power domain implementation
> which will live in arch/arm/mach-* as of today will become
> the new board file problem, overburdening the arch/* tree.
> Maybe I'm mistaken as to the size of these things,
> but just doing ls arch/arm/mach-omap2/powerdomain*
> makes me start worrying.

Yes, I agree that this means more code/data in arch/arm/mach-*, but
IMO, that's really where it belongs.  It really is SoC integration
details, and driver should really not know about it.

Kevin

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

[-- Attachment #1.1: Type: text/plain, Size: 3372 bytes --]

On Wed, Oct 31, 2012 at 9:10 PM, Kevin Hilman
<khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>wrote:

> Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
>


> > piece of hardware, this would be the right thing to do,
> > and I think the in-kernel examples are all "simple",
> > e.g. arch/arm/mach-omap2/powerdomain* is all about
> > power domains and nothing else,
>
> FYI... that code isn't the same as PM domain.


This sort of points to a core problem here. Our terminologies are
ambiguous that we cannot understand each others code. As long
as <linux/pm_domain.h> begins:

/*
 * pm_domain.h - Definitions and headers related to device power domains.
 *

But arguably that should just be patched (I think there are a few
remnants in the code still implying that these things are only about
power).


>  That code is for the
> *hardware* powerdomains, not the software concept of "PM domain."  In
> OMAP, PM domain is implmented at the omap_device level.  And omap_device
> is the abstraction of an IP block that knows about all the PM related
> register settings, clocks, HW powerdomain, voltage domain, PM related
> pin-muxing etc. etc.    All of these things are abstracted in an
> omap_device, so that the PM domain implementation for OMAP looks rather
> simple (c.f. omap_device_pm_domain in arch/arm/plat-omap/omap_device.c.)
>

OK following now...


> > I think the lesser of two evils is the distributed approach,
>
The pinctrl examples I've seen mentioned so far are all PM related
> (sleep, idle, wakeup, etc.) so to me I think they still belong in
> PM domains (and that's how we handle the PM related pins in OMAP.)
>

Well, the pinctrl grabbers in these drivers are using these states also
for platforms that do not even select CONFIG_PM.  For example
mach-nomadik is quite happy that the PL011 driver is thusly
muxing in its pins. And would require refactoring to use PM
domains.

So basically this requirement comes down to:

- When dealing with a SoC IP block driver

- That need to multiplex pins

- Then your SoC must select CONFIG_PM and
  CONFIG_PM_RUNTIME and
  CONFIG_PM_GENERIC_DOMAINS and implement
  proper domain handling hooks.

Is this correct? And for Mark, Dmitry, does this correspond to
your view?

It's actually something that needs to be acknowledged by the
ARM SoC maintainers, because they will be the ones telling
all subarch maintainers to go implement full PM handling
with these three frameworks whenever an SoC driver want
to handle pins.

Bascially all subsystem maintainers dealing with embedded
SoCs need to be aligned on this as well...

And IIUC not only pins but also silicon block clocks?

I can surely fix these for "my" systems, but it really needs
to be enforced widely or it will be a mess.


>  > I worry that the per-SoC power domain implementation
> > which will live in arch/arm/mach-* as of today will become
> > the new board file problem, overburdening the arch/* tree.
> > Maybe I'm mistaken as to the size of these things,
> > but just doing ls arch/arm/mach-omap2/powerdomain*
> > makes me start worrying.
>
> Yes, I agree that this means more code/data in arch/arm/mach-*, but
> IMO, that's really where it belongs.  It really is SoC integration
> details, and driver should really not know about it.


OK we need feedback from ARM SoC on this.

Yours,
Linus Walleij

[-- Attachment #1.2: Type: text/html, Size: 4808 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Fwd: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Wed, Oct 31, 2012 at 9:10 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:

> Linus Walleij <linus.walleij@linaro.org> writes:


>
> > piece of hardware, this would be the right thing to do,
> > and I think the in-kernel examples are all "simple",
> > e.g. arch/arm/mach-omap2/powerdomain* is all about
> > power domains and nothing else,
>
> FYI... that code isn't the same as PM domain.


This sort of points to a core problem here. Our terminologies are
ambiguous that we cannot understand each others code. As long
as <linux/pm_domain.h> begins:

/*
 * pm_domain.h - Definitions and headers related to device power domains.
 *

But arguably that should just be patched (I think there are a few
remnants in the code still implying that these things are only about
power).

>
>  That code is for the
> *hardware* powerdomains, not the software concept of "PM domain."  In
> OMAP, PM domain is implmented at the omap_device level.  And omap_device
> is the abstraction of an IP block that knows about all the PM related
> register settings, clocks, HW powerdomain, voltage domain, PM related
> pin-muxing etc. etc.    All of these things are abstracted in an
> omap_device, so that the PM domain implementation for OMAP looks rather
> simple (c.f. omap_device_pm_domain in arch/arm/plat-omap/omap_device.c.)


OK following now...

>
> > I think the lesser of two evils is the distributed approach,
>
> The pinctrl examples I've seen mentioned so far are all PM related
>
> (sleep, idle, wakeup, etc.) so to me I think they still belong in
> PM domains (and that's how we handle the PM related pins in OMAP.)


Well, the pinctrl grabbers in these drivers are using these states also
for platforms that do not even select CONFIG_PM.  For example
mach-nomadik is quite happy that the PL011 driver is thusly
muxing in its pins. And would require refactoring to use PM
domains.

So basically this requirement comes down to:

- When dealing with a SoC IP block driver

- That need to multiplex pins

- Then your SoC must select CONFIG_PM and
  CONFIG_PM_RUNTIME and
  CONFIG_PM_GENERIC_DOMAINS and implement
  proper domain handling hooks.

Is this correct? And for Mark, Dmitry, does this correspond to
your view?

It's actually something that needs to be acknowledged by the
ARM SoC maintainers, because they will be the ones telling
all subarch maintainers to go implement full PM handling
with these three frameworks whenever an SoC driver want
to handle pins.

Bascially all subsystem maintainers dealing with embedded
SoCs need to be aligned on this as well...

And IIUC not only pins but also silicon block clocks?

I can surely fix these for "my" systems, but it really needs
to be enforced widely or it will be a mess.

>
> > I worry that the per-SoC power domain implementation
> > which will live in arch/arm/mach-* as of today will become
> > the new board file problem, overburdening the arch/* tree.
> > Maybe I'm mistaken as to the size of these things,
> > but just doing ls arch/arm/mach-omap2/powerdomain*
> > makes me start worrying.
>
> Yes, I agree that this means more code/data in arch/arm/mach-*, but
> IMO, that's really where it belongs.  It really is SoC integration
> details, and driver should really not know about it.


OK we need feedback from ARM SoC on this.

Yours,
Linus Walleij

Re: Fwd: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Kevin Hilman]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Wed, Oct 31, 2012 at 9:10 PM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>
>> Linus Walleij <linus.walleij@linaro.org> writes:
>
>
>>
>> > piece of hardware, this would be the right thing to do,
>> > and I think the in-kernel examples are all "simple",
>> > e.g. arch/arm/mach-omap2/powerdomain* is all about
>> > power domains and nothing else,
>>
>> FYI... that code isn't the same as PM domain.
>
>
> This sort of points to a core problem here. Our terminologies are
> ambiguous that we cannot understand each others code. As long
> as <linux/pm_domain.h> begins:
>
> /*
>  * pm_domain.h - Definitions and headers related to device power domains.
>  *
>
> But arguably that should just be patched (I think there are a few
> remnants in the code still implying that these things are only about
> power).

Agreed.  The terminology is confusing, and any situations like this in
the code/comments/docs should be patched.

When PM domains were introduced, I was the first to complain that we
shouldn't use the term power domain so as not to be confused with HW
concepts, so we settled on the term 'PM domain.'  Ultimately, it's just
a configurable grouping of devices whose callbacks happen during PM
transitions.

>>
>>  That code is for the
>> *hardware* powerdomains, not the software concept of "PM domain."  In
>> OMAP, PM domain is implmented at the omap_device level.  And omap_device
>> is the abstraction of an IP block that knows about all the PM related
>> register settings, clocks, HW powerdomain, voltage domain, PM related
>> pin-muxing etc. etc.    All of these things are abstracted in an
>> omap_device, so that the PM domain implementation for OMAP looks rather
>> simple (c.f. omap_device_pm_domain in arch/arm/plat-omap/omap_device.c.)
>
>
> OK following now...
>
>>
>> > I think the lesser of two evils is the distributed approach,
>>
>> The pinctrl examples I've seen mentioned so far are all PM related
>>
>> (sleep, idle, wakeup, etc.) so to me I think they still belong in
>> PM domains (and that's how we handle the PM related pins in OMAP.)
>
>
> Well, the pinctrl grabbers in these drivers are using these states also
> for platforms that do not even select CONFIG_PM.  For example
> mach-nomadik is quite happy that the PL011 driver is thusly
> muxing in its pins. And would require refactoring to use PM
> domains.

If CONFIG_PM is disabled, then is it safe to assume that the pins in
question are probably only done once at init time.  I assume during
->probe(). ?

>
> So basically this requirement comes down to:
>
> - When dealing with a SoC IP block driver
>
> - That need to multiplex pins
>
> - Then your SoC must select CONFIG_PM and
>   CONFIG_PM_RUNTIME andb
>   CONFIG_PM_GENERIC_DOMAINS and implement
>   proper domain handling hooks.
>
> Is this correct? 

I would say yes.  Currently, PM domains are the way to hook SoC-specific
integration details into PM transitions.

However, if what we want/need are only ways to introduce SoC-specific
integration details into non-PM transitions (e.g. probe/remove), maybe
bus notifiers would suffice here.  e.g. you'd get a bus notifier when
the device is added/attached and any init-time pinctrl setup could be
done then.  This still keeps drivers clean of SoC-specific integration
data/code, and also allows that to happen whether or not PM features are
enabled.

Kevin

Re: Fwd: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Thu, Nov 1, 2012 at 12:42 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> [Me]
>> Well, the pinctrl grabbers in these drivers are using these states also
>> for platforms that do not even select CONFIG_PM.  For example
>> mach-nomadik is quite happy that the PL011 driver is thusly
>> muxing in its pins. And would require refactoring to use PM
>> domains.
>
> If CONFIG_PM is disabled, then is it safe to assume that the pins in
> question are probably only done once at init time.  I assume during
> ->probe(). ?

Sadly no.

Consider drivers/tty/serial/amba-pl011.c

Many ARM platforms have several instances of PL011, and not all of
them have CONFIG_PM & friends, so it's a good example.

Here the driver will probe and currently fetch a pinctrl handle and
looks up two states: "default", which refers to the situation you
describe, and optionally "sleep" which will put pins into a
low-power state.

The driver will currently put the pins into the "sleep" state when
.shutdown() is called by something (userspace or in-kernel users).

So in the new suggested scheme using runtime PM, this
would have to be replaced by pm_runtime_get[_sync]()
and pm_runtime_put() hints and the current pin handling
deleted, and for each platform using this driver instead
implement a PM domain to do the same thing.

Else you loose this runtime power optimization.

This is what I refer to the all-or-nothing charcter of
runtime PM domains... but maybe it's a good thing,
I haven't quite made my mind up about it.

> (...) if what we want/need are only ways to introduce SoC-specific
> integration details into non-PM transitions (e.g. probe/remove), maybe
> bus notifiers would suffice here.  e.g. you'd get a bus notifier when
> the device is added/attached and any init-time pinctrl setup could be
> done then.  This still keeps drivers clean of SoC-specific integration
> data/code, and also allows that to happen whether or not PM features are
> enabled.

It doesn't cut it for any of our drivers as shown above,
but it would work for the patch in $SUBJECT.

It sounds like the way silicon clocks are handled on
SH am I right?

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

On Thu, Nov 01, 2012 at 09:54:00AM +0100, Linus Walleij wrote:

> Well, the pinctrl grabbers in these drivers are using these states also
> for platforms that do not even select CONFIG_PM.  For example
> mach-nomadik is quite happy that the PL011 driver is thusly
> muxing in its pins. And would require refactoring to use PM
> domains.

> So basically this requirement comes down to:

> - When dealing with a SoC IP block driver

> - That need to multiplex pins

> - Then your SoC must select CONFIG_PM and
>   CONFIG_PM_RUNTIME and
>   CONFIG_PM_GENERIC_DOMAINS and implement
>   proper domain handling hooks.

> Is this correct? And for Mark, Dmitry, does this correspond to
> your view?

For the pin hogging I'd actually been thinking separately that we should
just have the device core do a devm_pinctrl_get_set_default() prior to
probing the device and store the result in the struct device.  That
would immediately remove almost all of the current pinctrl users, users
that do need to do things with the data or check the result can then
pick up the pinctrl pointer from the device struct.

> It's actually something that needs to be acknowledged by the
> ARM SoC maintainers, because they will be the ones telling
> all subarch maintainers to go implement full PM handling
> with these three frameworks whenever an SoC driver want
> to handle pins.

Well, they're going to have to implement it somewhere anyway - either in
the drivers or in the SoC stuff.

> And IIUC not only pins but also silicon block clocks?

> I can surely fix these for "my" systems, but it really needs
> to be enforced widely or it will be a mess.

We definitely need to decide if it's something that should be open coded
everywhere.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Thu, Nov 1, 2012 at 1:07 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Nov 01, 2012 at 09:54:00AM +0100, Linus Walleij wrote:

> For the pin hogging I'd actually been thinking separately that we should
> just have the device core do a devm_pinctrl_get_set_default() prior to
> probing the device and store the result in the struct device.  That
> would immediately remove almost all of the current pinctrl users, users
> that do need to do things with the data or check the result can then
> pick up the pinctrl pointer from the device struct.

I never thought of that. This sounds like it would work.

And the good thing is that this is a clean cut so we
will centralized code without having to decide right now
how to handle the pm idle/sleep cases.

Talking here with Kevin Hilman on my left and Grant
Likely on my right (they're physically here) there is some
worry about stashing stuff into struct device.

What if I retrieve this in the pinctrl subsystem using
bus notifiers and then expose the struct pinctrl * to
the clients by using pinctrl_get() and when you get
such a handle in your probe() you know that the
pinctrl subsystem has already fetched the handle and
set it to "default" at that point?

I just worry whether there is a fringe case where the default
state is not be be default-selected in probe(), the API
semantics does not mandate that. But I think this is the case
for all in-kernel drivers so we wouldn't be breaking anything
by doing this right now. And platforms can just leave the
"default" state undefined in that case.

>> It's actually something that needs to be acknowledged by the
>> ARM SoC maintainers, because they will be the ones telling
>> all subarch maintainers to go implement full PM handling
>> with these three frameworks whenever an SoC driver want
>> to handle pins.
>
> Well, they're going to have to implement it somewhere anyway - either in
> the drivers or in the SoC stuff.

Sure I just worry about it being done is several different ways
and creating a mess so they need to be involved to block
other approaches.

>> I can surely fix these for "my" systems, but it really needs
>> to be enforced widely or it will be a mess.
>
> We definitely need to decide if it's something that should be open coded
> everywhere.

If I prepare a patch to move the fetch+set defaul to the pinctrl
core using notifiers, we centralize one piece and we get the
currently floating patches out of the way.

Then what to do with sleep and idle is a question we need
to handle next. Requiring PM domains for this is one
approach, albeit a bit controversial.

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

On Thu, Nov 01, 2012 at 03:01:28PM +0100, Linus Walleij wrote:
> On Thu, Nov 1, 2012 at 1:07 PM, Mark Brown
> 
> > For the pin hogging I'd actually been thinking separately that we should
> > just have the device core do a devm_pinctrl_get_set_default() prior to
> > probing the device and store the result in the struct device.  That

> What if I retrieve this in the pinctrl subsystem using
> bus notifiers and then expose the struct pinctrl * to
> the clients by using pinctrl_get() and when you get
> such a handle in your probe() you know that the
> pinctrl subsystem has already fetched the handle and
> set it to "default" at that point?

I'm not sure I parse a problem from the above?

> I just worry whether there is a fringe case where the default
> state is not be be default-selected in probe(), the API
> semantics does not mandate that. But I think this is the case
> for all in-kernel drivers so we wouldn't be breaking anything
> by doing this right now. And platforms can just leave the
> "default" state undefined in that case.

Yes, that had been my thinking too though I'd really expect that the
platform ought to be able to think of something sensible to do by
default.

> Then what to do with sleep and idle is a question we need
> to handle next. Requiring PM domains for this is one
> approach, albeit a bit controversial.

Yup.  Notifiers are another option again I guess.  There's far fewer
drivers doing anything at all with that so it's a bit less urgent.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Thu, Nov 1, 2012 at 3:01 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Nov 1, 2012 at 1:07 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Thu, Nov 01, 2012 at 09:54:00AM +0100, Linus Walleij wrote:
>
>> For the pin hogging I'd actually been thinking separately that we should
>> just have the device core do a devm_pinctrl_get_set_default() prior to
>> probing the device and store the result in the struct device.  That
>> would immediately remove almost all of the current pinctrl users, users
>> that do need to do things with the data or check the result can then
>> pick up the pinctrl pointer from the device struct.
>
> I never thought of that. This sounds like it would work.

So I've looked closer at this.

> And the good thing is that this is a clean cut so we
> will centralized code without having to decide right now
> how to handle the pm idle/sleep cases.
>
> Talking here with Kevin Hilman on my left and Grant
> Likely on my right (they're physically here) there is some
> worry about stashing stuff into struct device.
>
> What if I retrieve this in the pinctrl subsystem using
> bus notifiers and then expose the struct pinctrl * to
> the clients by using pinctrl_get() and when you get
> such a handle in your probe() you know that the
> pinctrl subsystem has already fetched the handle and
> set it to "default" at that point?

I have sent out an RFC for this approach. It actually
works quite well on the U300 for example.

However as stated in the patch we run into another
problem: what if the pinctrl provider returns
-EDEFER_PROBE?

(The same will be true for clocks, regulators etc I
guess...)

If I use notifiers like this, I cannot return -EPROBE_DEFER
to the core. :-(

The PM domains seem to be built on the notification
mechanism as well (AFAICT), so it will not allow drivers to
defer their probes, as it is an optional layer after all.

As a matter of fact it seems that there is an implicit
assumption in PM domains that the resources that
are taken there cannot defer any probing, so they are
assumed to always be present. Is this correct?
If so, any resources that may be deferred (such as
pinctrl) can never be handled in PM domains. Only
simple stuff that the SoC controls directly e.g through
some fixed register pokes to voltage domains.

So then the only option that remains to centralize
pinctrl handling is indeed to do that in the device core,
before probe(). I need to know Greg's feelings about that.

At the same time this sort of give me the feeling that
we might be doing something wrong. The distributed
nature of deferred probes will become quite elusive
if the deferral request comes from some place
before probe() is even called on the driver.

Please check my RFC patch and tell me above errors
in the above reasoning....

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Jean-Christophe PLAGNIOL-VILLARD]

On 21:12 Sun 28 Oct     , Linus Walleij wrote:
> On Wed, Oct 24, 2012 at 7:28 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> >> drivers/spi/spi-pl022.c
> >
> > Default/sleep transitions could be moved into bus code.
> 
> No that's not a good idea as long as we have both the platform bus
> and the AMBA bus doing essentially the same thing. We will then be
> having two copies of the same code in two different busses running
> out of sync. There may be other busses too.
> 
> But I could prepare static helpers in <linux/pinctrl/consumer.h>
> that any bus could use. Or any driver. Probably any driver,
> because of this:
> 
> As noted the bus cannot really execute the pinctrl calls to
> e.g. put a drivers pins into "sleep". Since if e.g. the bus is walking
> the suspend() ladder, shall it put the pins into sleep *before*
> or *after* calling the suspend() hook in the driver?
> 
> The answer is that it does not know. Because drivers have
> different needs. Depending on how the hardware and
> system is done.
> 
> I already tried to make this point:
> 
> pinctrl_set_state(state_sleep);
> clk_disable();
> power_off_voltage_domain();
> 
> May for some drivers have to be:
> 
> clk_disable();
> power_off_voltage_domain();
> pinctrl_set_state(state_sleep);
> 
> (etc)
> 
> I'm not making this up, it is a very real phenomenon on the
> Ux500 and I don't think we are unique.
I agree with Linus

you can not handle pinctrl as bus levell

as each driver need different requirement before enabling the pin

as example you may need to clock the ip before muxing the pin and invertly

on some IP the pinctrl is optionnal, on other mandatory

you can not expect every driver to have the same need and requirement

yes we will have a few code duplication

but today it's few lines and those few lines will be place at different init
stage on the drivers ditto for remove or sleep/idle


> 
> Moving this handling to bus code or anywhere else
> invariably implies that resource acquisition/release order
> does not matter, and my point is that it does.

100% agreed

Best Regards,
J.
> 
> >> drivers/i2c/busses/i2c-nomadik.c
> >
> > Don't see pinctrl in linux-next.
> 
> This code is here:
> http://marc.info/?l=linux-i2c&m=134986995731695&w=2
> 
> Yours,
> Linus Walleij
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 6775 bytes --]

Hi,

On Wed, Oct 24, 2012 at 09:14:29AM -0700, Dmitry Torokhov wrote:

<snip>

> > > No, I guess we ihave different understanding of what "directly use" means.
> > > This particular driver does directly use interrupts: it requests it and
> > > performs some actions when interrupt arrives. Same goes for IO memory -
> > > it gets requested, then we access it. With pinctrl we do not do anything
> > > - we just ask another layer to configure it and that is it.
> > 
> > this is true for almost anything we do:
> > 
> > - we ask another layer to allocate memory for us
> > - we ask another layer to call our ISR once the IRQ line is asserted
> > - we ask another layer to handle the input events we just received
> > - we ask another layer to transfer data through DMA for us
> > - we ask another layer to turn regulators on and off.
> 
> But we are _directly_ _using_ all of these. You allocate memory and you
> (the driver) stuff data into that memory. You ask for DMA and you take
> the DMAed data and work with it. Not so with pinctrl in omap keypad and
> other drivers I have seen so far.

of course we are. If we don't mux the pins to their correct setting, we
can't realy use the HW. So while you don't see any SW control of the
requested pins, we're still making use of them.

> > and so on. This is just how abstractions work, we group common parts in
> > a framework so that users don't need to know the details, but still need
> > to tell the framework when to fiddle with those resources.
> > 
> > > I have seen just in a few days 3 or 4 drivers having exactly the same
> > > change - call to devm_pinctrl_get_select_default(), and I guess I will
> > > receive the same patches for the rest of input drivers shortly.
> > > This suggests that the operation is done at the wrong level. Do the
> > > pin configuration as you parse DT data, the same way you set up i2c
> > > devices registers in of_i2c.c, and leave the individual drivers that do
> > > not care about specifics alone.
> > 
> > Makes no sense to hide that from drivers. The idea here is that driver
> > should know when it needs its pins to muxed correctly.
> 
> The driver also needs memory controller to be initialized, gpio chip be
> ready and registered, DMA subsystem ready, input core reade, etc, etc,
> etc. You however do not have every driver explicitly initialize any of
> that; you expect certain working environment to be already operable. The
> driver does manage resources it controls, it has ultimate knowledge
> about, pin configuration is normally is not it. We just need to know
> that we wired/muxed properly, otherwise we won't work. So please let
> parent layers deal with it.
> 
> > 95% of the time
> > it will be done during probe() but then again, so what ?
> > 
> > doing that when parsing DT, or on bus notifiers is just plain wrong.
> > Drivers should be required to handle all of their resources.
> 
> All of _their_ resources, exactly. They do not own nor control pins so
> they should not be bothered with them either. Look, when you see that

except that they *own* the pins. Now that the muxer has been setup
properly, this particular IP owns the pins.

> potentially _every_ driver in the system needs to set up the same object
> that it doe snot use otherwise you should realize that individual driver
> is not the proper place to do that.

fair enough, but IMHO, we're not there yet. We can't make that claim
yet. Besides, we don't know what's the default pin state in a system. It
might be that certain pins start out in a way which consumes less power
due to the internal construction of the SoC. If we set pins up before
driver probes, and probe fails or the driver is never really used, then
we could be falling into a situation where we're wasting power.

Granted, you can undo everything you did before, but I guess keeping
track of everything we setup before probe() just to remove a couple of
lines from drivers is wrong.

> > > > That's why it is named "get_select_default" and not "get" only.
> > > > This API ensure that the pin will be set to the default state, and this
> > > > is all we need to do during the probe.
> > > 
> > > Why during the probe and not by default? Realistically, the driver will
> > > be loaded as long as there is a matching device and pins will need to be
> > > configured.
> > 
> > likewise memory will be allocated when matching happens, IRQs will be
> > allocated, regulators will be turned on. So why don't we do all that by
> > default ? Because it is wrong.
> 
> No, because we do not know how. The generic layer does not know the ISR
> to install, how much memory to allocate, etc. Having regulator turned on
> before getting to probe might not be a bad idea.

what if your driver never probes ? Will you really leave regulators on
consuming extra, valuable power ?

> > > > There is no point to change the mux if the driver is not probed, because
> > > > potentially the pin can be use be another driver.
> > > 
> > > What other driver would use it? Who would chose what driver to load?
> > 
> > Well, you _do_ know that on a SoC we have a limited amount of pins
> > right ?
> > 
> > Considering the amont of features which are packed inside a single die,
> > it's not farfetched to assume we will have a lot less pins then we
> > actually need, so we need muxers behind each pin in order to choose
> > which functionality we want.
> > 
> > If it happens that keypad's pins are shared with another IP (e.g. GPIO),
> > we need to give the final user (a OEM/ODM) the choice of using those
> > pins as either keypad or GPIOs, thus the need for pinctrl framework and
> > the calls in the drivers.
> 
> Right, so please walk me through, step by step, how an OEM/ODM woudl
> select a particular configuration. Do you expect it to happen at
> runtime, or do you expect relevant data be put in DT?

It depends, I've seen both happening, really. Also note that DT
migration is still not complete, meaning that most (all ?) OEM/ODMs are
still using the legacy board-file-based approach and it will still take
them a few years to move to DT-based boot.

Another point to consider is community boards such as beaglebone which
have tens of different "capes" to support. Depending on the cape, pins
might have to be remuxed, so instead of adding all that code to platform
support, just leave it all in drivers. Depending on the "cape" different
drivers will probe() and those drivers should know how to mux pins for
themselves.

Note that these are only the two easy examples that came to my mind, I'm
sure we can discuss this for a long, but is it valid ? For a single line
of code ?

-- 
balbi

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Wed, Oct 24, 2012 at 6:52 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Oct 24, 2012 at 09:14:29AM -0700, Dmitry Torokhov wrote:

>> All of _their_ resources, exactly. They do not own nor control pins so
>> they should not be bothered with them either. Look, when you see that
>
> except that they *own* the pins. Now that the muxer has been setup
> properly, this particular IP owns the pins.

This is true. It is then also reflected in the device model.
And in debugfs, which is very helpful when debugging. If I
cat
/sys/kernel/debug/pinctrl/pinctrl-db8500/pins

I can see a list of all pins on this ASIC and what device
is using it. It is all due to the fact that each driver use
[devm_]pinctrl_get(&dev) ad the struct device * pointer
is used with dev_name() to print the corresponding
device name.

When using hogs it just says the name of the pin
controller and the pins aren't really connected to their
real consumers :-P

Example:

Pinmux settings per pin
Format: pin (name): mux_owner gpio_owner hog?
pin 0 (GPIO0_AJ5): (MUX UNCLAIMED) (GPIO UNCLAIMED)
pin 1 (GPIO1_AJ3): (MUX UNCLAIMED) (GPIO UNCLAIMED)
pin 2 (GPIO2_AH4): (MUX UNCLAIMED) (GPIO UNCLAIMED)
pin 3 (GPIO3_AH3): (MUX UNCLAIMED) (GPIO UNCLAIMED)
pin 4 (GPIO4_AH6): (MUX UNCLAIMED) (GPIO UNCLAIMED)
pin 5 (GPIO5_AG6): (MUX UNCLAIMED) DB8500:5
pin 6 (GPIO6_AF6): pinctrl-db8500 (GPIO UNCLAIMED) function ipgpio
group ipgpio0_c_1
pin 7 (GPIO7_AG5): pinctrl-db8500 (GPIO UNCLAIMED) function ipgpio
group ipgpio1_c_1
pin 8 (GPIO8_AD5): (MUX UNCLAIMED) (GPIO UNCLAIMED)
pin 9 (GPIO9_AE4): (MUX UNCLAIMED) (GPIO UNCLAIMED)
pin 10 (GPIO10_AF5): nmk-i2c.2 (GPIO UNCLAIMED) function i2c2 group i2c2_b_2
pin 11 (GPIO11_AG4): nmk-i2c.2 (GPIO UNCLAIMED) function i2c2 group i2c2_b_2
pin 12 (GPIO12_AC4): pinctrl-db8500 (GPIO UNCLAIMED) function msp0
group msp0txrx_a_1
pin 13 (GPIO13_AF3): pinctrl-db8500 (GPIO UNCLAIMED) function msp0
group msp0tfstck_a_1
pin 14 (GPIO14_AE3): pinctrl-db8500 (GPIO UNCLAIMED) function msp0
group msp0tfstck_a_1
pin 15 (GPIO15_AC3): pinctrl-db8500 (GPIO UNCLAIMED) function msp0
group msp0txrx_a_1
pin 16 (GPIO16_AD3): nmk-i2c.1 (GPIO UNCLAIMED) function i2c1 group i2c1_b_2
pin 17 (GPIO17_AD4): nmk-i2c.1 (GPIO UNCLAIMED) function i2c1 group i2c1_b_2

As you can see pins 6,7,12-15 are using hogs. Sure I can see
the name of the function but that is just a string albeit
helpful.

Pins 10,11,16,17 are requested directly from the i2c driver
and shows up connected to its device.

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Dmitry Torokhov]

[-- Attachment #1: Type: text/plain, Size: 7687 bytes --]

On Wednesday, October 24, 2012 07:52:16 PM Felipe Balbi wrote:
> Hi,
> 
> On Wed, Oct 24, 2012 at 09:14:29AM -0700, Dmitry Torokhov wrote:
> 
> <snip>
> 
> > > > No, I guess we ihave different understanding of what "directly use"
> > > > means.
> > > > This particular driver does directly use interrupts: it requests it
> > > > and
> > > > performs some actions when interrupt arrives. Same goes for IO memory
> > > > -
> > > > it gets requested, then we access it. With pinctrl we do not do
> > > > anything
> > > > - we just ask another layer to configure it and that is it.
> > > 
> > > this is true for almost anything we do:
> > > 
> > > - we ask another layer to allocate memory for us
> > > - we ask another layer to call our ISR once the IRQ line is asserted
> > > - we ask another layer to handle the input events we just received
> > > - we ask another layer to transfer data through DMA for us
> > > - we ask another layer to turn regulators on and off.
> > 
> > But we are _directly_ _using_ all of these. You allocate memory and you
> > (the driver) stuff data into that memory. You ask for DMA and you take
> > the DMAed data and work with it. Not so with pinctrl in omap keypad and
> > other drivers I have seen so far.
> 
> of course we are. If we don't mux the pins to their correct setting, we
> can't realy use the HW. So while you don't see any SW control of the
> requested pins, we're still making use of them.

So we make use of CPU too, and the main power supply, and memory chips.

> 
> > > and so on. This is just how abstractions work, we group common parts in
> > > a framework so that users don't need to know the details, but still need
> > > to tell the framework when to fiddle with those resources.
> > > 
> > > > I have seen just in a few days 3 or 4 drivers having exactly the same
> > > > change - call to devm_pinctrl_get_select_default(), and I guess I will
> > > > receive the same patches for the rest of input drivers shortly.
> > > > This suggests that the operation is done at the wrong level. Do the
> > > > pin configuration as you parse DT data, the same way you set up i2c
> > > > devices registers in of_i2c.c, and leave the individual drivers that
> > > > do
> > > > not care about specifics alone.
> > > 
> > > Makes no sense to hide that from drivers. The idea here is that driver
> > > should know when it needs its pins to muxed correctly.
> > 
> > The driver also needs memory controller to be initialized, gpio chip be
> > ready and registered, DMA subsystem ready, input core reade, etc, etc,
> > etc. You however do not have every driver explicitly initialize any of
> > that; you expect certain working environment to be already operable. The
> > driver does manage resources it controls, it has ultimate knowledge
> > about, pin configuration is normally is not it. We just need to know
> > that we wired/muxed properly, otherwise we won't work. So please let
> > parent layers deal with it.
> > 
> > > 95% of the time
> > > it will be done during probe() but then again, so what ?
> > > 
> > > doing that when parsing DT, or on bus notifiers is just plain wrong.
> > > Drivers should be required to handle all of their resources.
> > 
> > All of _their_ resources, exactly. They do not own nor control pins so
> > they should not be bothered with them either. Look, when you see that
> 
> except that they *own* the pins. Now that the muxer has been setup
> properly, this particular IP owns the pins.
> 
> > potentially _every_ driver in the system needs to set up the same object
> > that it doe snot use otherwise you should realize that individual driver
> > is not the proper place to do that.
> 
> fair enough, but IMHO, we're not there yet. We can't make that claim
> yet. Besides, we don't know what's the default pin state in a system. It
> might be that certain pins start out in a way which consumes less power
> due to the internal construction of the SoC. If we set pins up before
> driver probes, and probe fails or the driver is never really used, then
> we could be falling into a situation where we're wasting power.

So what about moving this into bus code - have bus's probe() request
default pin config before probe, revert to original setup when unbinding
or probe fails. You can even plug PM switching into bus code as well.

> 
> Granted, you can undo everything you did before,

Right, the same way as we undo every other initialization when something
goes wrong.

> but I guess keeping
> track of everything we setup before probe() just to remove a couple of
> lines from drivers is wrong.

If it was just about a couple lines in a couple of drivers that would
be fine. But the way I see it eventually every driver will need to do
this.

> 
> > > > > That's why it is named "get_select_default" and not "get" only.
> > > > > This API ensure that the pin will be set to the default state, and
> > > > > this
> > > > > is all we need to do during the probe.
> > > > 
> > > > Why during the probe and not by default? Realistically, the driver
> > > > will
> > > > be loaded as long as there is a matching device and pins will need to
> > > > be
> > > > configured.
> > > 
> > > likewise memory will be allocated when matching happens, IRQs will be
> > > allocated, regulators will be turned on. So why don't we do all that by
> > > default ? Because it is wrong.
> > 
> > No, because we do not know how. The generic layer does not know the ISR
> > to install, how much memory to allocate, etc. Having regulator turned on
> > before getting to probe might not be a bad idea.
> 
> what if your driver never probes ? Will you really leave regulators on
> consuming extra, valuable power ?

If we do it right in probe() we won't consume unless the dirver is bound.

> 
> > > > > There is no point to change the mux if the driver is not probed,
> > > > > because
> > > > > potentially the pin can be use be another driver.
> > > > 
> > > > What other driver would use it? Who would chose what driver to load?
> > > 
> > > Well, you _do_ know that on a SoC we have a limited amount of pins
> > > right ?
> > > 
> > > Considering the amont of features which are packed inside a single die,
> > > it's not farfetched to assume we will have a lot less pins then we
> > > actually need, so we need muxers behind each pin in order to choose
> > > which functionality we want.
> > > 
> > > If it happens that keypad's pins are shared with another IP (e.g. GPIO),
> > > we need to give the final user (a OEM/ODM) the choice of using those
> > > pins as either keypad or GPIOs, thus the need for pinctrl framework and
> > > the calls in the drivers.
> > 
> > Right, so please walk me through, step by step, how an OEM/ODM woudl
> > select a particular configuration. Do you expect it to happen at
> > runtime, or do you expect relevant data be put in DT?
> 
> It depends, I've seen both happening, really. Also note that DT
> migration is still not complete, meaning that most (all ?) OEM/ODMs are
> still using the legacy board-file-based approach and it will still take
> them a few years to move to DT-based boot.
> 
> Another point to consider is community boards such as beaglebone which
> have tens of different "capes" to support. Depending on the cape, pins
> might have to be remuxed, so instead of adding all that code to platform
> support, just leave it all in drivers. Depending on the "cape" different
> drivers will probe() and those drivers should know how to mux pins for
> themselves.
> 
> Note that these are only the two easy examples that came to my mind, I'm
> sure we can discuss this for a long, but is it valid ? For a single line
> of code ?

Multiply by hundred of drivers - yes.

-- 
Dmitry

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Benoit Cousson]

Hi Dmitry,

On 10/24/2012 06:14 PM, Dmitry Torokhov wrote:
> On Wed, Oct 24, 2012 at 11:37:04AM +0300, Felipe Balbi wrote:
>> Hi,
>>
>> On Tue, Oct 23, 2012 at 01:02:49PM -0700, Dmitry Torokhov wrote:
>>> On Tue, Oct 23, 2012 at 11:18:12AM +0200, Benoit Cousson wrote:
>>>> Hi Dimitry,
>>>>
>>>> On 10/22/2012 05:50 PM, Dmitry Torokhov wrote:
>>>>> Hi Sourav,
>>>>>
>>>>> On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote:
>>>>>> Adapt keypad to use pinctrl framework.
>>>>>>
>>>>>> Tested on omap4430 sdp with 3.7-rc1 kernel.
>>>>>
>>>>> I do not see anything in the driver that would directly use pinctrl. Is
>>>>> there a better place to select default pin configuration; maybe when
>>>>> instantiating platform device?
>>>>
>>>> Why?
>>>> The devm_pinctrl_get_select_default is using the pinctrl.
>>>
>>> No, I guess we ihave different understanding of what "directly use" means.
>>> This particular driver does directly use interrupts: it requests it and
>>> performs some actions when interrupt arrives. Same goes for IO memory -
>>> it gets requested, then we access it. With pinctrl we do not do anything
>>> - we just ask another layer to configure it and that is it.
>>
>> this is true for almost anything we do:
>>
>> - we ask another layer to allocate memory for us
>> - we ask another layer to call our ISR once the IRQ line is asserted
>> - we ask another layer to handle the input events we just received
>> - we ask another layer to transfer data through DMA for us
>> - we ask another layer to turn regulators on and off.
> 
> But we are _directly_ _using_ all of these. You allocate memory and you
> (the driver) stuff data into that memory. You ask for DMA and you take
> the DMAed data and work with it. Not so with pinctrl in omap keypad and
> other drivers I have seen so far.

That's not really true. You select a pin mode and thanks to that you get
the signal from an external pin that goes to your IP.
And thanks to that the IP is doing what your are expecting it to do.

Without that your IP will not get any input signal, which will make it a
little bit useless, isn't it?

>> and so on. This is just how abstractions work, we group common parts in
>> a framework so that users don't need to know the details, but still need
>> to tell the framework when to fiddle with those resources.
>>
>>> I have seen just in a few days 3 or 4 drivers having exactly the same
>>> change - call to devm_pinctrl_get_select_default(), and I guess I will
>>> receive the same patches for the rest of input drivers shortly.
>>> This suggests that the operation is done at the wrong level. Do the
>>> pin configuration as you parse DT data, the same way you set up i2c
>>> devices registers in of_i2c.c, and leave the individual drivers that do
>>> not care about specifics alone.
>>
>> Makes no sense to hide that from drivers. The idea here is that driver
>> should know when it needs its pins to muxed correctly.
> 
> The driver also needs memory controller to be initialized, gpio chip be
> ready and registered, DMA subsystem ready, input core reade, etc, etc,
> etc. You however do not have every driver explicitly initialize any of
> that; you expect certain working environment to be already operable. The
> driver does manage resources it controls, it has ultimate knowledge
> about, pin configuration is normally is not it. We just need to know
> that we wired/muxed properly, otherwise we won't work. So please let
> parent layers deal with it.
> 
>> 95% of the time
>> it will be done during probe() but then again, so what ?
>>
>> doing that when parsing DT, or on bus notifiers is just plain wrong.
>> Drivers should be required to handle all of their resources.
> 
> All of _their_ resources, exactly. They do not own nor control pins so
> they should not be bothered with them either. Look, when you see that
> potentially _every_ driver in the system needs to set up the same object
> that it doe snot use otherwise you should realize that individual driver
> is not the proper place to do that.

What your are missing as well in that case is the explicit dependency
that this API is creating with the pinctrl driver that we are going to
miss otherwise.

Hence the following code.

+		if (PTR_ERR(keypad_data->pins) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;

If the pinctrl is not already there you defer the probe until it is there.

Moreover, as already said, we are probably at some point going to handle
as well the low power mode and thus change the pin mode upon idle/suspend.

And again, selecting a pin mode during probe is doing something with the
pins when and only when it is useful. It is not different than getting
an IRQ or DMA request at probe time.

You get it, use it for registration and that all you are doing with it.
It is no different here.

Doing that during device creation does not make sense, since that device
might never be used.

Is it like allocating the memory by default for every devices at boot
time just in case a driver will probe it at some time or registering
every IRQs at boot time just in case a driver will use it...

That's just pointless. We are wasting resources for nothing and thus
potentially power and that will not help the Earth getting any better.

Regards,
Benoit

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Tue, Oct 23, 2012 at 10:02 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> I have seen just in a few days 3 or 4 drivers having exactly the same
> change - call to devm_pinctrl_get_select_default(), and I guess I will
> receive the same patches for the rest of input drivers shortly.
> This suggests that the operation is done at the wrong level. Do the
> pin configuration as you parse DT data, the same way you set up i2c
> devices registers in of_i2c.c, and leave the individual drivers that do
> not care about specifics alone.

Exactly this can be done with pinctrl hogs.

The problem with that is that it removes the cross-reference
between the device and it's pinctrl handle (also from the device
tree). Instead the pinctrl handle gets referenced to the pin controller
itself. So from a modelling perpective this looks a bit ugly.

So we have two kinds of ugly:

- Sprinke devm_pinctrl_get_select_default() over all drivers
  which makes pinctrl handles properly reference their devices

- Use hogs and loose coupling between pinctrl handles and their
  devices

A third alternative as outlined is to use notifiers and some
resource core in drivers/base/*

>> That's why it is named "get_select_default" and not "get" only.
>> This API ensure that the pin will be set to the default state, and this
>> is all we need to do during the probe.
>
> Why during the probe and not by default? Realistically, the driver will
> be loaded as long as there is a matching device and pins will need to be
> configured.

Hogs will do it by default but will disassociate the pinctrl from its
device as described.

>> There is no point to change the mux if the driver is not probed, because
>> potentially the pin can be use be another driver.
>
> What other driver would use it? Who would chose what driver to load?

I don't know about this specific driver, but for the SKE
keypad driver we have a runtime case switching around the
pins.

We recently patched the pinctrl core for a specific usecase like
this, and in that case both drivers are loaded, but one will be
disabled at runtime and the other become active.

In the ux500, if you need to perform deep debugging on a running
system (an ordinary cell phone, say) you can at runtime convert
the SD card output into an STM trace port and start monitoring
different messages coming out on a MIPI-type bus.

So basically it is not an SD card slot anymore, it turns into
something else, the silicon core for MMC/SD is decoupled from its
pins, and they are re-routed to the STM tracer. And you plug
a special piece of hardware into the SD-card slot and it
has a USB cord or something collection standard MIPI
traces.

We have the same for the SKE keypad actually. We can shunt
the STM tracing signals out on this as well, you "just" unmount
the physical keypad and start using the lines on the PCB as
a trace collecting mechanism.

Thus we need - at runtime, in systems produced in the the
millions, it's not "just for fun" - to recouple pins from one IP
block to another and turn it into a totally different thing.

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Dmitry Torokhov]

On Wed, Oct 24, 2012 at 02:54:23PM +0200, Linus Walleij wrote:
> On Tue, Oct 23, 2012 at 10:02 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > I have seen just in a few days 3 or 4 drivers having exactly the same
> > change - call to devm_pinctrl_get_select_default(), and I guess I will
> > receive the same patches for the rest of input drivers shortly.
> > This suggests that the operation is done at the wrong level. Do the
> > pin configuration as you parse DT data, the same way you set up i2c
> > devices registers in of_i2c.c, and leave the individual drivers that do
> > not care about specifics alone.
> 
> Exactly this can be done with pinctrl hogs.
> 
> The problem with that is that it removes the cross-reference
> between the device and it's pinctrl handle (also from the device
> tree). Instead the pinctrl handle gets referenced to the pin controller
> itself. So from a modelling perpective this looks a bit ugly.
> 
> So we have two kinds of ugly:
> 
> - Sprinke devm_pinctrl_get_select_default() over all drivers
>   which makes pinctrl handles properly reference their devices
> 
> - Use hogs and loose coupling between pinctrl handles and their
>   devices
> 
> A third alternative as outlined is to use notifiers and some
> resource core in drivers/base/*

OK, so with drivers/base/, have you considered doing default pinctrl
selection in bus's probe() methods? Yo would select the default
configuration before starting probing the device and maybe select idle
when probe fails or device is unbound? That would still keep the link
between device object and pinctrl and there less busses than device
drivers out there.

Thanks.

-- 
Dmitry

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2396 bytes --]

Hi,

On Wed, Oct 24, 2012 at 09:18:01AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 24, 2012 at 02:54:23PM +0200, Linus Walleij wrote:
> > On Tue, Oct 23, 2012 at 10:02 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > 
> > > I have seen just in a few days 3 or 4 drivers having exactly the same
> > > change - call to devm_pinctrl_get_select_default(), and I guess I will
> > > receive the same patches for the rest of input drivers shortly.
> > > This suggests that the operation is done at the wrong level. Do the
> > > pin configuration as you parse DT data, the same way you set up i2c
> > > devices registers in of_i2c.c, and leave the individual drivers that do
> > > not care about specifics alone.
> > 
> > Exactly this can be done with pinctrl hogs.
> > 
> > The problem with that is that it removes the cross-reference
> > between the device and it's pinctrl handle (also from the device
> > tree). Instead the pinctrl handle gets referenced to the pin controller
> > itself. So from a modelling perpective this looks a bit ugly.
> > 
> > So we have two kinds of ugly:
> > 
> > - Sprinke devm_pinctrl_get_select_default() over all drivers
> >   which makes pinctrl handles properly reference their devices
> > 
> > - Use hogs and loose coupling between pinctrl handles and their
> >   devices
> > 
> > A third alternative as outlined is to use notifiers and some
> > resource core in drivers/base/*
> 
> OK, so with drivers/base/, have you considered doing default pinctrl
> selection in bus's probe() methods? Yo would select the default
> configuration before starting probing the device and maybe select idle
> when probe fails or device is unbound? That would still keep the link
> between device object and pinctrl and there less busses than device
> drivers out there.

it starts to become confusing after a while. I mean, there's a reason
why all drivers explictly call pm_runtim_enable(), right ?

From a first thought, one could think of just yanking that into bus'
probe() as you may suggest, but sometimes the device is already enabled,
so we need extra tricks:

pm_runtime_set_active();
pm_runtime_enable();
pm_runtime_get();

the same could happen with pinctrl eventually. What if a device needs to
do something else (an errata fix as an example) before requesting
pinctrl's default state ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Wed, Oct 24, 2012 at 6:57 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Oct 24, 2012 at 09:18:01AM -0700, Dmitry Torokhov wrote:
>> OK, so with drivers/base/, have you considered doing default pinctrl
>> selection in bus's probe() methods? Yo would select the default
>> configuration before starting probing the device and maybe select idle
>> when probe fails or device is unbound? That would still keep the link
>> between device object and pinctrl and there less busses than device
>> drivers out there.
>
> it starts to become confusing after a while. I mean, there's a reason
> why all drivers explictly call pm_runtim_enable(), right ?
>
> From a first thought, one could think of just yanking that into bus'
> probe() as you may suggest, but sometimes the device is already enabled,
> so we need extra tricks:
>
> pm_runtime_set_active();
> pm_runtime_enable();
> pm_runtime_get();
>
> the same could happen with pinctrl eventually. What if a device needs to
> do something else (an errata fix as an example) before requesting
> pinctrl's default state ?

I can confirm this. Just the ordering between enabling/disabling
resources like clock/pins/powerdomain screw things up for us
and even if we can surely centralize parts of this code as such
into the drivers/base or pm_* namespace we would still need
explicit calls from the driver.

I'm thinking that maybe the best helpers are actually static
inline functions in the <linux/pinctrl/consumer.h> header
rather than moving anything into drivers/base/* if the
code duplication is the real problem.

Yours,
Linus Walleij

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Dmitry Torokhov]

On Wednesday, October 24, 2012 07:57:49 PM Felipe Balbi wrote:
> Hi,
> 
> On Wed, Oct 24, 2012 at 09:18:01AM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 24, 2012 at 02:54:23PM +0200, Linus Walleij wrote:
> > > On Tue, Oct 23, 2012 at 10:02 PM, Dmitry Torokhov
> > > 
> > > <dmitry.torokhov@gmail.com> wrote:
> > > > I have seen just in a few days 3 or 4 drivers having exactly the same
> > > > change - call to devm_pinctrl_get_select_default(), and I guess I will
> > > > receive the same patches for the rest of input drivers shortly.
> > > > This suggests that the operation is done at the wrong level. Do the
> > > > pin configuration as you parse DT data, the same way you set up i2c
> > > > devices registers in of_i2c.c, and leave the individual drivers that
> > > > do
> > > > not care about specifics alone.
> > > 
> > > Exactly this can be done with pinctrl hogs.
> > > 
> > > The problem with that is that it removes the cross-reference
> > > between the device and it's pinctrl handle (also from the device
> > > tree). Instead the pinctrl handle gets referenced to the pin controller
> > > itself. So from a modelling perpective this looks a bit ugly.
> > > 
> > > So we have two kinds of ugly:
> > > 
> > > - Sprinke devm_pinctrl_get_select_default() over all drivers
> > > 
> > >   which makes pinctrl handles properly reference their devices
> > > 
> > > - Use hogs and loose coupling between pinctrl handles and their
> > > 
> > >   devices
> > > 
> > > A third alternative as outlined is to use notifiers and some
> > > resource core in drivers/base/*
> > 
> > OK, so with drivers/base/, have you considered doing default pinctrl
> > selection in bus's probe() methods? Yo would select the default
> > configuration before starting probing the device and maybe select idle
> > when probe fails or device is unbound? That would still keep the link
> > between device object and pinctrl and there less busses than device
> > drivers out there.
> 
> it starts to become confusing after a while. I mean, there's a reason
> why all drivers explictly call pm_runtim_enable(), right ?

Right. Because not all of them support runtime PM and quite usually their
PM methods are not ready to go until initialization is complete. And again,
the driver here controls its own behavior.

> 
> From a first thought, one could think of just yanking that into bus'
> probe() as you may suggest, but sometimes the device is already enabled,
> so we need extra tricks:
> 
> pm_runtime_set_active();
> pm_runtime_enable();
> pm_runtime_get();
> 
> the same could happen with pinctrl eventually. What if a device needs to
> do something else (an errata fix as an example) before requesting
> pinctrl's default state ?

That is a valid concern and we'll need to find a compromise here. As I said,
I am not saying that no driver should ever touch pinctrl. I can see, for
example, input drivers actually disabling pins until they are ->open()ed,
in addition of doing that at [runtime] suspend/resume. But it would be nice
if we could have "dumb" drivers not care about pins.

Thanks.

-- 
Dmitry

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5701 bytes --]

Hi,

On Wed, Oct 24, 2012 at 10:58:53AM -0700, Dmitry Torokhov wrote:
> On Wednesday, October 24, 2012 07:57:49 PM Felipe Balbi wrote:
> > Hi,
> > 
> > On Wed, Oct 24, 2012 at 09:18:01AM -0700, Dmitry Torokhov wrote:
> > > On Wed, Oct 24, 2012 at 02:54:23PM +0200, Linus Walleij wrote:
> > > > On Tue, Oct 23, 2012 at 10:02 PM, Dmitry Torokhov
> > > > 
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > I have seen just in a few days 3 or 4 drivers having exactly the same
> > > > > change - call to devm_pinctrl_get_select_default(), and I guess I will
> > > > > receive the same patches for the rest of input drivers shortly.
> > > > > This suggests that the operation is done at the wrong level. Do the
> > > > > pin configuration as you parse DT data, the same way you set up i2c
> > > > > devices registers in of_i2c.c, and leave the individual drivers that
> > > > > do
> > > > > not care about specifics alone.
> > > > 
> > > > Exactly this can be done with pinctrl hogs.
> > > > 
> > > > The problem with that is that it removes the cross-reference
> > > > between the device and it's pinctrl handle (also from the device
> > > > tree). Instead the pinctrl handle gets referenced to the pin controller
> > > > itself. So from a modelling perpective this looks a bit ugly.
> > > > 
> > > > So we have two kinds of ugly:
> > > > 
> > > > - Sprinke devm_pinctrl_get_select_default() over all drivers
> > > > 
> > > >   which makes pinctrl handles properly reference their devices
> > > > 
> > > > - Use hogs and loose coupling between pinctrl handles and their
> > > > 
> > > >   devices
> > > > 
> > > > A third alternative as outlined is to use notifiers and some
> > > > resource core in drivers/base/*
> > > 
> > > OK, so with drivers/base/, have you considered doing default pinctrl
> > > selection in bus's probe() methods? Yo would select the default
> > > configuration before starting probing the device and maybe select idle
> > > when probe fails or device is unbound? That would still keep the link
> > > between device object and pinctrl and there less busses than device
> > > drivers out there.
> > 
> > it starts to become confusing after a while. I mean, there's a reason
> > why all drivers explictly call pm_runtim_enable(), right ?
> 
> Right. Because not all of them support runtime PM and quite usually their
> PM methods are not ready to go until initialization is complete. And again,
> the driver here controls its own behavior.

likewise not all devices will need pin muxing, those which do (granted,
an increasing number of them since transistor size continue to shrink,
allowing chip manufacturers to pack more features inside a single die,
while the number of external pins/balls remain the same), will call
pinctrl to setup muxing right.

> > From a first thought, one could think of just yanking that into bus'
> > probe() as you may suggest, but sometimes the device is already enabled,
> > so we need extra tricks:
> > 
> > pm_runtime_set_active();
> > pm_runtime_enable();
> > pm_runtime_get();
> > 
> > the same could happen with pinctrl eventually. What if a device needs to
> > do something else (an errata fix as an example) before requesting
> > pinctrl's default state ?
> 
> That is a valid concern and we'll need to find a compromise here. As I said,

WHAT ?? Silicon erratas are not a valid concern ? Power waste isn't a
valid concern ? Tell that to the millions of devices shipped with Linux
everyday. Power usage if it's the top concern in any product, is right
there as the top five. Likewise for silicon erratas.

Let's face it, just like SW, HW has bugs; the difference is that no
company will continue to do several spins of an ASIC just because some
SW engineer doesn't get concerned about a silicon bug. It's just too
expensive to re-spin the ASIC. And even if we get another revision of
the ASIC, we still need to support the older version as there might be
cellphones, laser welding machines, IPTVs and whatever product already
shipped.

> I am not saying that no driver should ever touch pinctrl. I can see, for
> example, input drivers actually disabling pins until they are ->open()ed,
> in addition of doing that at [runtime] suspend/resume. But it would be nice
> if we could have "dumb" drivers not care about pins.

Like I replied on another sub-thread, this will just create exceptions
to the rule which is far more messy than having a couple of extra lines
of code in a few drivers. We can even argue that eventually all drivers
will need to toggle pins in runtime in order to save that extra
microwatt of runtime power consumption, so why bother adding exceptions ?

In fact, we already have the exception: drivers which don't need to
fiddle with pin muxing, just don't use pinctrl. The ones you're
receiving today are the one which, for whatever reason, need to make
sure pin muxing is right. If it's not toggling in runtime, it might just
be because $AUTHOR decided that it would be best to do thing in small
steps (don't we all agree with that ?). Maybe he thought that changing
pins in runtime could cause problems, so let's get bare minimum in
mainline and work towards optimizations in parallel.

All in all, I don't see why you're complaining so much about a couple of
lines of code. Even if it needs to be added to all drivers in tree. So
what ? As long as pinctrl works fine in multiple platforms, what is done
for e.g. OMAP2 should work fine in OMAP3/4/5. An even more complex
example: what works on OMAP, should work on Exynos, Tegra, etc, if the
same driver is used accross those platforms.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Dmitry Torokhov]

On Wednesday, October 24, 2012 10:10:42 PM Felipe Balbi wrote:
> > 
> >
> > That is a valid concern and we'll need to find a compromise here. As I
> > said,
> WHAT ?? Silicon erratas are not a valid concern ? Power waste isn't a
> valid concern ? Tell that to the millions of devices shipped with Linux
> everyday. Power usage if it's the top concern in any product, is right
> there as the top five. Likewise for silicon erratas.

I think we should come back to this discussion when you get more coffee
and start parsing other party e-mails properly.

-- 
Dmitry

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

Hi,

On Wed, Oct 24, 2012 at 12:38:44PM -0700, Dmitry Torokhov wrote:
> On Wednesday, October 24, 2012 10:10:42 PM Felipe Balbi wrote:
> > > 
> > >
> > > That is a valid concern and we'll need to find a compromise here. As I
> > > said,
> > WHAT ?? Silicon erratas are not a valid concern ? Power waste isn't a
> > valid concern ? Tell that to the millions of devices shipped with Linux
> > everyday. Power usage if it's the top concern in any product, is right
> > there as the top five. Likewise for silicon erratas.
> 
> I think we should come back to this discussion when you get more coffee
> and start parsing other party e-mails properly.

indeed. I really misparsed it. My bad. comments withdrawn.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] Input: omap4-keypad: Add pinctrl support

[Linus Walleij]

On Wed, Oct 24, 2012 at 6:18 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Oct 24, 2012 at 02:54:23PM +0200, Linus Walleij wrote:
>>
>> A third alternative as outlined is to use notifiers and some
>> resource core in drivers/base/*
>
> OK, so with drivers/base/, have you considered doing default pinctrl
> selection in bus's probe() methods? Yo would select the default
> configuration before starting probing the device and maybe select idle
> when probe fails or device is unbound? That would still keep the link
> between device object and pinctrl and there less busses than device
> drivers out there.

One of our major important busses is the AMBA (PrimeCell) bus.

As it happens, this bus actually already do limited resource handling
by requesting the silicon block clock for the device, which is necessary
to perform auto-probing on the bus level. (You won't be able to read
the auto-detect registers unless the silicon is clocked...)

When it comes to pin control is turns out in the AMBA drivers we
have that we need to do more complex stuff than just select a
default configuration. (I assure this is not just for fun, it is saving
considerable amounts of power).

So the examples I outlined just in the previous mail:
drivers/tty/serial/amba-pl011.c
drivers/spi/spi-pl022.c
drivers/i2c/busses/i2c-nomadik.c

Hm it turns out that Wolfram has not yet merged the i2c patch,
here it is:
http://marc.info/?l=linux-i2c&m=134986995731695&w=2

There are complex state switches involved. It can arguably be
centralized, but then it needs to go into drivers/base/[power]
or similar, not into a specific piece of bus code, because the
needs won't be any different for e.g. a platform device.

Yours,
Linus Walleij