Patchwork [Resend,v6,2/2] ASoC: atmel-ssc: add pinctrl consumer

login
register
mail settings
Submitter Bo Shen
Date Nov. 16, 2012, 6:03 a.m.
Message ID <1353045837-19474-2-git-send-email-voice.shen@atmel.com>
Download mbox | patch
Permalink /patch/1753091/
State New, archived
Headers show

Comments

Bo Shen - Nov. 16, 2012, 6:03 a.m.
Add pinctrl consumer for atmel ssc peripheral

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---
Resend:
  Split one patch into two.
    - one add pinctrl nodes
    - one add pinctrl consumer
---
 drivers/misc/atmel-ssc.c |    8 ++++++++
 1 file changed, 8 insertions(+)
Mark Brown - Nov. 16, 2012, 6:12 a.m.
On Fri, Nov 16, 2012 at 02:03:57PM +0800, Bo Shen wrote:
> Add pinctrl consumer for atmel ssc peripheral
> 
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> ---
> Resend:
>   Split one patch into two.
>     - one add pinctrl nodes
>     - one add pinctrl consumer

No, this isn't what was meant - the idea is to send only the addition of
pinctrl data as one patch, based off the ASoC branch instead of -next.
Bo Shen - Nov. 16, 2012, 6:33 a.m.
Hi Mark,

On 11/16/2012 14:12, Mark Brown wrote:
> On Fri, Nov 16, 2012 at 02:03:57PM +0800, Bo Shen wrote:
>> Add pinctrl consumer for atmel ssc peripheral
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>> ---
>> Resend:
>>    Split one patch into two.
>>      - one add pinctrl nodes
>>      - one add pinctrl consumer
>
> No, this isn't what was meant - the idea is to send only the addition of
> pinctrl data as one patch, based off the ASoC branch instead of -next.

What is the pinctrl data? (This patch can be applied on 
sound/topic/atmel branch without any conflicts)
The other one add pinctrl nodes, must based on -next, or else I don't 
know where should I add the pinctrl nodes.

Best Regards,
Bo Shen
Mark Brown - Nov. 16, 2012, 6:41 a.m.
On Fri, Nov 16, 2012 at 02:33:50PM +0800, Bo Shen wrote:
> On 11/16/2012 14:12, Mark Brown wrote:

> >No, this isn't what was meant - the idea is to send only the addition of
> >pinctrl data as one patch, based off the ASoC branch instead of -next.

> What is the pinctrl data? (This patch can be applied on

The data you're adding in the device tree!

> sound/topic/atmel branch without any conflicts)

That's not helpful to anyone doing bisection if there's nothing defining
the pin states, it means that the system won't be able to start the
driver as the API call will fail.

> The other one add pinctrl nodes, must based on -next, or else I
> don't know where should I add the pinctrl nodes.

What makes you say this?
Bo Shen - Nov. 16, 2012, 6:59 a.m.
On 11/16/2012 14:41, Mark Brown wrote:
> On Fri, Nov 16, 2012 at 02:33:50PM +0800, Bo Shen wrote:
>> On 11/16/2012 14:12, Mark Brown wrote:
>
>>> No, this isn't what was meant - the idea is to send only the addition of
>>> pinctrl data as one patch, based off the ASoC branch instead of -next.
>
>> What is the pinctrl data? (This patch can be applied on
>
> The data you're adding in the device tree!
>
>> sound/topic/atmel branch without any conflicts)
>
> That's not helpful to anyone doing bisection if there's nothing defining
> the pin states, it means that the system won't be able to start the
> driver as the API call will fail.
>
>> The other one add pinctrl nodes, must based on -next, or else I
>> don't know where should I add the pinctrl nodes.
>
> What makes you say this?

For example, if I want to add pinctrl node
---<8---
  ssc0 {
	pinctrl_ssc0_tx: ssc0_tx-0 {
         	atmel,pins = <1 16 0x1 0x0   /* PB16 periph A */
                 	      1 17 0x1 0x0   /* PB17 periph A */
                       	      1 18 0x1 0x0>; /* PB18 periph A */
         };
--->8---
This should be add into dtsi file as following
---<8---
ahb {
	apb {
		pinctrl {
			ssc0 {
				pinctrl_ssc0_tx
			}
		}
	}
}
--->8---

In the ASoC branch tree, I don't see any pinctrl related information. 
So, I say I don't know where should I add the pinctrl nodes.

May be I misunderstanding, do you mean I should only add as followign 
based on ASoC tree? And the upper go into pinctrl tree?
---<8---
ssc0: ssc@fffbc000 {
	compatible = "atmel,at91rm9200-ssc";
         reg = <0xfffbc000 0x4000>;
         interrupts = <14 4 5>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_ssc0_tx &pinctrl_ssc0_rx>;
         status = "disable";
};
--->8---

Best Regards
Bo Shen
Bo Shen - Nov. 20, 2012, 9:31 a.m.
Hi Mark,

On 11/16/2012 14:59, Bo Shen wrote:
> On 11/16/2012 14:41, Mark Brown wrote:
>> On Fri, Nov 16, 2012 at 02:33:50PM +0800, Bo Shen wrote:
>>> On 11/16/2012 14:12, Mark Brown wrote:
>>
>>>> No, this isn't what was meant - the idea is to send only the
>>>> addition of
>>>> pinctrl data as one patch, based off the ASoC branch instead of -next.
>>
>>> What is the pinctrl data? (This patch can be applied on
>>
>> The data you're adding in the device tree!
>>
>>> sound/topic/atmel branch without any conflicts)
>>
>> That's not helpful to anyone doing bisection if there's nothing defining
>> the pin states, it means that the system won't be able to start the
>> driver as the API call will fail.
>>
>>> The other one add pinctrl nodes, must based on -next, or else I
>>> don't know where should I add the pinctrl nodes.
>>
>> What makes you say this?
>
> For example, if I want to add pinctrl node
> ---<8---
>   ssc0 {
>      pinctrl_ssc0_tx: ssc0_tx-0 {
>              atmel,pins = <1 16 0x1 0x0   /* PB16 periph A */
>                            1 17 0x1 0x0   /* PB17 periph A */
>                                  1 18 0x1 0x0>; /* PB18 periph A */
>          };
> --->8---
> This should be add into dtsi file as following
> ---<8---
> ahb {
>      apb {
>          pinctrl {
>              ssc0 {
>                  pinctrl_ssc0_tx
>              }
>          }
>      }
> }
> --->8---
>
> In the ASoC branch tree, I don't see any pinctrl related information.
> So, I say I don't know where should I add the pinctrl nodes.
>
> May be I misunderstanding, do you mean I should only add as followign
> based on ASoC tree? And the upper go into pinctrl tree?
> ---<8---
> ssc0: ssc@fffbc000 {
>      compatible = "atmel,at91rm9200-ssc";
>          reg = <0xfffbc000 0x4000>;
>          interrupts = <14 4 5>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_ssc0_tx &pinctrl_ssc0_rx>;
>          status = "disable";
> };
> --->8---

Any suggestion for this? what should I do with this patch for next?

> Best Regards
> Bo Shen
>
>
Mark Brown - Nov. 20, 2012, 10:27 a.m.
On Tue, Nov 20, 2012 at 05:31:22PM +0800, Bo Shen wrote:

> >based on ASoC tree? And the upper go into pinctrl tree?
> >---<8---
> >ssc0: ssc@fffbc000 {
> >     compatible = "atmel,at91rm9200-ssc";
> >         reg = <0xfffbc000 0x4000>;
> >         interrupts = <14 4 5>;
> >+        pinctrl-names = "default";
> >+        pinctrl-0 = <&pinctrl_ssc0_tx &pinctrl_ssc0_rx>;
> >         status = "disable";
> >};
> >--->8---

> Any suggestion for this? what should I do with this patch for next?

Send it to pinctrl and we'll deal with the add/add.
Bo Shen - Nov. 21, 2012, 1:43 a.m.
Hi Mark,

On 11/20/2012 18:27, Mark Brown wrote:
> On Tue, Nov 20, 2012 at 05:31:22PM +0800, Bo Shen wrote:
>
>>> based on ASoC tree? And the upper go into pinctrl tree?
>>> ---<8---
>>> ssc0: ssc@fffbc000 {
>>>      compatible = "atmel,at91rm9200-ssc";
>>>          reg = <0xfffbc000 0x4000>;
>>>          interrupts = <14 4 5>;
>>> +        pinctrl-names = "default";
>>> +        pinctrl-0 = <&pinctrl_ssc0_tx &pinctrl_ssc0_rx>;
>>>          status = "disable";
>>> };
>>> --->8---
>
>> Any suggestion for this? what should I do with this patch for next?
>
> Send it to pinctrl and we'll deal with the add/add.

So, should I keep these two patches [1] and [2] and then send to pinctrl 
again, or I need keep it in one patch as [3] which i sent first.

1. [Resend,v6,1/2] ARM: at91: atmel-ssc: add pinctrl nodes
    https://patchwork.kernel.org/patch/1753101/

2. [Resend,v6,2/2] ASoC: atmel-ssc: add pinctrl consumer
    https://patchwork.kernel.org/patch/1753091/

3. [v6,1/4] ARM: at91: atmel-ssc: add pinctrl support
    https://patchwork.kernel.org/patch/1740391/

Best regards,
Bo Shen
Mark Brown - Nov. 21, 2012, 1:46 a.m.
On Wed, Nov 21, 2012 at 09:43:33AM +0800, Bo Shen wrote:

> So, should I keep these two patches [1] and [2] and then send to
> pinctrl again, or I need keep it in one patch as [3] which i sent
> first.

Please send something coherent which can be applied without breaking the
branch it is applied to.

Patch

diff --git a/drivers/misc/atmel-ssc.c b/drivers/misc/atmel-ssc.c
index a769719..3b3d0e0 100644
--- a/drivers/misc/atmel-ssc.c
+++ b/drivers/misc/atmel-ssc.c
@@ -19,6 +19,7 @@ 
 #include <linux/module.h>
 
 #include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
 
 /* Serialize access to ssc_list and user count */
 static DEFINE_SPINLOCK(user_lock);
@@ -131,6 +132,13 @@  static int ssc_probe(struct platform_device *pdev)
 	struct resource *regs;
 	struct ssc_device *ssc;
 	const struct atmel_ssc_platform_data *plat_dat;
+	struct pinctrl *pinctrl;
+
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl)) {
+		dev_err(&pdev->dev, "Failed to request pinctrl\n");
+		return PTR_ERR(pinctrl);
+	}
 
 	ssc = devm_kzalloc(&pdev->dev, sizeof(struct ssc_device), GFP_KERNEL);
 	if (!ssc) {