[PATCH 0/2] pinctrl: meson: use one uniform 'function' name

[PATCH 0/2] pinctrl: meson: use one uniform 'function' name

[Yixun Lan]

These two patches are general improvement for meson pinctrl driver.
It make the two pinctrl trees (ee/ao) to share one uniform 'function' name for
one hardware block even its pin groups live inside two differet hardware domains,
which for example EE vs AO domain here.

This idea is motivated by Martin's question at [1]

[1]
 http://lkml.kernel.org/r/CAFBinCCuQ-NK747+GHDkhZty_UMMgzCYOYFcNTrRDJgU8OM=Gw@mail.gmail.com


Yixun Lan (2):
  pinctrl: meson: introduce a macro to have name/groups seperated
  pinctrl: meson-axg: correct the pin expansion of UART_AO_B

 drivers/pinctrl/meson/pinctrl-meson-axg.c | 4 ++--
 drivers/pinctrl/meson/pinctrl-meson.h     | 8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.15.1

[PATCH 1/2] pinctrl: meson: introduce a macro to have name/groups seperated

[Yixun Lan]

We introduce a macro FUNCTION_EX here, the main motivation is
trying to have the possibility to expand the macro with the same of the
'.name' number but different multiple '.groups/.num_groups' numbers.

With this change, the meson pinctrl drivr is capable of have one uniform
'function' name but with different pin 'groups', as we face the sitiuation
that two pin groups may live inside different hardware domain (EE vs AO domain),
which mean we couldn't put them in one single group.

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/pinctrl/meson/pinctrl-meson.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
index 12a391109329..d8f705098810 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.h
+++ b/drivers/pinctrl/meson/pinctrl-meson.h
@@ -124,13 +124,15 @@ struct meson_pinctrl {
 	struct device_node *of_node;
 };
 
-#define FUNCTION(fn)							\
+#define FUNCTION_EX(fn, ex)						\
 	{								\
 		.name = #fn,						\
-		.groups = fn ## _groups,				\
-		.num_groups = ARRAY_SIZE(fn ## _groups),		\
+		.groups = fn ## ex ## _groups,				\
+		.num_groups = ARRAY_SIZE(fn ## ex ## _groups),		\
 	}
 
+#define FUNCTION(fn)	FUNCTION_EX(fn, )
+
 #define BANK(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib)	\
 	{								\
 		.name		= n,					\
-- 
2.15.1

[PATCH 2/2] pinctrl: meson-axg: correct the pin expansion of UART_AO_B

[Yixun Lan]

The 'uart_ao_b_groups' for the UART_AO_B pins is already defined which is
living inside the AO domain, for these pins which are routed out from EE domain,
we need to correct them with the 'FUNCTION_EX' macro, otherwise there is
a conflict in the code level.

Also slightly adjust the name to make it short and more consistent.

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/pinctrl/meson/pinctrl-meson-axg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/meson/pinctrl-meson-axg.c b/drivers/pinctrl/meson/pinctrl-meson-axg.c
index 1fda9d6c7ea3..308e5433bd04 100644
--- a/drivers/pinctrl/meson/pinctrl-meson-axg.c
+++ b/drivers/pinctrl/meson/pinctrl-meson-axg.c
@@ -716,7 +716,7 @@ static const char * const uart_b_groups[] = {
 	"uart_tx_b_x", "uart_rx_b_x", "uart_cts_b_x", "uart_rts_b_x",
 };
 
-static const char * const uart_ao_b_gpioz_groups[] = {
+static const char * const uart_ao_b_z_groups[] = {
 	"uart_ao_tx_b_z", "uart_ao_rx_b_z",
 	"uart_ao_cts_b_z", "uart_ao_rts_b_z",
 };
@@ -855,7 +855,7 @@ static struct meson_pmx_func meson_axg_periphs_functions[] = {
 	FUNCTION(nand),
 	FUNCTION(uart_a),
 	FUNCTION(uart_b),
-	FUNCTION(uart_ao_b_gpioz),
+	FUNCTION_EX(uart_ao_b, _z),
 	FUNCTION(i2c0),
 	FUNCTION(i2c1),
 	FUNCTION(i2c2),
-- 
2.15.1

Re: [PATCH 0/2] pinctrl: meson: use one uniform 'function' name

[Jerome Brunet]

On Mon, 2018-01-08 at 15:33 +0800, Yixun Lan wrote:
> These two patches are general improvement for meson pinctrl driver.
> It make the two pinctrl trees (ee/ao) to share one uniform 'function' name for
> one hardware block even its pin groups live inside two differet hardware domains,
> which for example EE vs AO domain here.
> 
> This idea is motivated by Martin's question at [1]
> 
> [1]
>  http://lkml.kernel.org/r/CAFBinCCuQ-NK747+GHDkhZty_UMMgzCYOYFcNTrRDJgU8OM=Gw@mail.gmail.com
> 
> 
> Yixun Lan (2):
>   pinctrl: meson: introduce a macro to have name/groups seperated
>   pinctrl: meson-axg: correct the pin expansion of UART_AO_B
> 
>  drivers/pinctrl/meson/pinctrl-meson-axg.c | 4 ++--
>  drivers/pinctrl/meson/pinctrl-meson.h     | 8 +++++---
>  2 files changed, 7 insertions(+), 5 deletions(-)

Hi Yixun,

Honestly, I don't like the idea. I think it adds an unnecessary complexity.
I don't see the point of FUNCTION_EX(uart_ao_b, _z) when you could simply write 
FUNCTION(uart_ao_b_z) ... especially when there is just a couple of function per
SoC available on different domains.

A pinctrl driver can already be challenging to understand at first, let's keep
it simple and avoid adding more macros.

Regards
Jerome

Re: [PATCH 0/2] pinctrl: meson: use one uniform 'function' name

[Yixun Lan]

On 01/08/18 16:52, Jerome Brunet wrote:
> On Mon, 2018-01-08 at 15:33 +0800, Yixun Lan wrote:
>> These two patches are general improvement for meson pinctrl driver.
>> It make the two pinctrl trees (ee/ao) to share one uniform 'function' name for
>> one hardware block even its pin groups live inside two differet hardware domains,
>> which for example EE vs AO domain here.
>>
>> This idea is motivated by Martin's question at [1]
>>
>> [1]
>>  http://lkml.kernel.org/r/CAFBinCCuQ-NK747+GHDkhZty_UMMgzCYOYFcNTrRDJgU8OM=Gw@mail.gmail.com
>>
>>
>> Yixun Lan (2):
>>   pinctrl: meson: introduce a macro to have name/groups seperated
>>   pinctrl: meson-axg: correct the pin expansion of UART_AO_B
>>
>>  drivers/pinctrl/meson/pinctrl-meson-axg.c | 4 ++--
>>  drivers/pinctrl/meson/pinctrl-meson.h     | 8 +++++---
>>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> Hi Yixun,
> 
> Honestly, I don't like the idea. I think it adds an unnecessary complexity.
> I don't see the point of FUNCTION_EX(uart_ao_b, _z) when you could simply write 
> FUNCTION(uart_ao_b_z) ... especially when there is just a couple of function per
> SoC available on different domains.
> 
> A pinctrl driver can already be challenging to understand at first, let's keep
> it simple and avoid adding more macros.
> 

Hi Jerome：
  In my opinion, the idea of keeping one uniform 'function' in DT (thus
introducing another macro) is worth considering. It would make the DT
part much clean.
  And yes, it's a trade-off here, either we 1) do more in code to make
DT clean or 2) do nothing in the code level to make DT live with it.

Yixun

Re: [PATCH 0/2] pinctrl: meson: use one uniform 'function' name

[Jerome Brunet]

On Wed, 2018-01-10 at 10:12 +0800, Yixun Lan wrote:
> 
> On 01/08/18 16:52, Jerome Brunet wrote:
> > On Mon, 2018-01-08 at 15:33 +0800, Yixun Lan wrote:
> > > These two patches are general improvement for meson pinctrl driver.
> > > It make the two pinctrl trees (ee/ao) to share one uniform 'function' name for
> > > one hardware block even its pin groups live inside two differet hardware domains,
> > > which for example EE vs AO domain here.
> > > 
> > > This idea is motivated by Martin's question at [1]
> > > 
> > > [1]
> > >  http://lkml.kernel.org/r/CAFBinCCuQ-NK747+GHDkhZty_UMMgzCYOYFcNTrRDJgU8OM=Gw@mail.gmail.com
> > > 
> > > 
> > > Yixun Lan (2):
> > >   pinctrl: meson: introduce a macro to have name/groups seperated
> > >   pinctrl: meson-axg: correct the pin expansion of UART_AO_B
> > > 
> > >  drivers/pinctrl/meson/pinctrl-meson-axg.c | 4 ++--
> > >  drivers/pinctrl/meson/pinctrl-meson.h     | 8 +++++---
> > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > Hi Yixun,
> > 
> > Honestly, I don't like the idea. I think it adds an unnecessary complexity.
> > I don't see the point of FUNCTION_EX(uart_ao_b, _z) when you could simply write 
> > FUNCTION(uart_ao_b_z) ... especially when there is just a couple of function per
> > SoC available on different domains.
> > 
> > A pinctrl driver can already be challenging to understand at first, let's keep
> > it simple and avoid adding more macros.
> > 
> 
> Hi Jerome：
>   In my opinion, the idea of keeping one uniform 'function' in DT (thus
> introducing another macro) is worth considering. It would make the DT
> part much clean.

Ok this is your opinion. I don't share it. Keeping function names tidy is good,
I don't think we need another macro to do so.

>   And yes, it's a trade-off here, either we 1) do more in code to make
> DT clean or 2) do nothing in the code level to make DT live with it.

I don't see how adding a macro doing just string concatenation is going to make
anything more clean. It does not prevent one to write FUNCTION_EX(uart_ao_b,
_gpioz), resulting in uart_ao_b_gpioz, which is what is apparently considered
'not clean'

BTW, there no cleanness issue here, the name is just out of the 'usual scheme'
but there is no problem with. If you want to change this, and
s/uart_ao_b_gpioz/uart_ao_b_z/, now is the time to change it. 

> 
> Yixun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] pinctrl: meson: use one uniform 'function' name

[Yixun Lan]

Hi Jerome:

On 01/10/2018 03:28 PM, Jerome Brunet wrote:
> On Wed, 2018-01-10 at 10:12 +0800, Yixun Lan wrote:
>>
>> On 01/08/18 16:52, Jerome Brunet wrote:
>>> On Mon, 2018-01-08 at 15:33 +0800, Yixun Lan wrote:
>>>> These two patches are general improvement for meson pinctrl driver.
>>>> It make the two pinctrl trees (ee/ao) to share one uniform 'function' name for
>>>> one hardware block even its pin groups live inside two differet hardware domains,
>>>> which for example EE vs AO domain here.
>>>>
>>>> This idea is motivated by Martin's question at [1]
>>>>
>>>> [1]
>>>>  http://lkml.kernel.org/r/CAFBinCCuQ-NK747+GHDkhZty_UMMgzCYOYFcNTrRDJgU8OM=Gw@mail.gmail.com
>>>>
>>>>
>>>> Yixun Lan (2):
>>>>   pinctrl: meson: introduce a macro to have name/groups seperated
>>>>   pinctrl: meson-axg: correct the pin expansion of UART_AO_B
>>>>
>>>>  drivers/pinctrl/meson/pinctrl-meson-axg.c | 4 ++--
>>>>  drivers/pinctrl/meson/pinctrl-meson.h     | 8 +++++---
>>>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>>
>>> Hi Yixun,
>>>
>>> Honestly, I don't like the idea. I think it adds an unnecessary complexity.
>>> I don't see the point of FUNCTION_EX(uart_ao_b, _z) when you could simply write 
>>> FUNCTION(uart_ao_b_z) ... especially when there is just a couple of function per
>>> SoC available on different domains.
>>>
>>> A pinctrl driver can already be challenging to understand at first, let's keep
>>> it simple and avoid adding more macros.
>>>
>>
>> Hi Jerome：
>>   In my opinion, the idea of keeping one uniform 'function' in DT (thus
>> introducing another macro) is worth considering. It would make the DT
>> part much clean.
> 
> Ok this is your opinion. I don't share it. Keeping function names tidy is good,
> I don't think we need another macro to do so.
> 
>>   And yes, it's a trade-off here, either we 1) do more in code to make
>> DT clean or 2) do nothing in the code level to make DT live with it.
> 
> I don't see how adding a macro doing just string concatenation is going to make
> anything more clean. It does not prevent one to write FUNCTION_EX(uart_ao_b,
> _gpioz), resulting in uart_ao_b_gpioz, which is what is apparently considered
> 'not clean'
> 
for the benefits of introducing macro 'FUNCTION_EX', it will end with
 .name = "uart_ao_b", -> same for both EE, AO domain, and it will match
the DT part (although still different for '.groups')


> BTW, there no cleanness issue here, the name is just out of the 'usual scheme'
> but there is no problem with. If you want to change this, and
> s/uart_ao_b_gpioz/uart_ao_b_z/, now is the time to change it. 
> 
I'd rather *NOT* to push a pinctrl patch for just changing
'uart_ao_b_gpioz' to 'uart_ao_b_z' (it's a cosmetic change, and still
end with two different name - 'uart_ao_b_gpioz/z' & 'uart_ao_b' in DT)

>>
>> Yixun
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> 

Re: [PATCH 0/2] pinctrl: meson: use one uniform 'function' name

[Kevin Hilman]

Yixun Lan <yixun.lan@amlogic.com> writes:

> Hi Jerome:
>
> On 01/10/2018 03:28 PM, Jerome Brunet wrote:
>> On Wed, 2018-01-10 at 10:12 +0800, Yixun Lan wrote:
>>>
>>> On 01/08/18 16:52, Jerome Brunet wrote:
>>>> On Mon, 2018-01-08 at 15:33 +0800, Yixun Lan wrote:
>>>>> These two patches are general improvement for meson pinctrl driver.
>>>>> It make the two pinctrl trees (ee/ao) to share one uniform 'function' name for
>>>>> one hardware block even its pin groups live inside two differet hardware domains,
>>>>> which for example EE vs AO domain here.
>>>>>
>>>>> This idea is motivated by Martin's question at [1]
>>>>>
>>>>> [1]
>>>>>  http://lkml.kernel.org/r/CAFBinCCuQ-NK747+GHDkhZty_UMMgzCYOYFcNTrRDJgU8OM=Gw@mail.gmail.com
>>>>>
>>>>>
>>>>> Yixun Lan (2):
>>>>>   pinctrl: meson: introduce a macro to have name/groups seperated
>>>>>   pinctrl: meson-axg: correct the pin expansion of UART_AO_B
>>>>>
>>>>>  drivers/pinctrl/meson/pinctrl-meson-axg.c | 4 ++--
>>>>>  drivers/pinctrl/meson/pinctrl-meson.h     | 8 +++++---
>>>>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> Hi Yixun,
>>>>
>>>> Honestly, I don't like the idea. I think it adds an unnecessary complexity.
>>>> I don't see the point of FUNCTION_EX(uart_ao_b, _z) when you could simply write 
>>>> FUNCTION(uart_ao_b_z) ... especially when there is just a couple of function per
>>>> SoC available on different domains.
>>>>
>>>> A pinctrl driver can already be challenging to understand at first, let's keep
>>>> it simple and avoid adding more macros.
>>>>
>>>
>>> Hi Jerome：
>>>   In my opinion, the idea of keeping one uniform 'function' in DT (thus
>>> introducing another macro) is worth considering. It would make the DT
>>> part much clean.
>> 
>> Ok this is your opinion. I don't share it. Keeping function names tidy is good,
>> I don't think we need another macro to do so.
>> 
>>>   And yes, it's a trade-off here, either we 1) do more in code to make
>>> DT clean or 2) do nothing in the code level to make DT live with it.
>> 
>> I don't see how adding a macro doing just string concatenation is going to make
>> anything more clean. It does not prevent one to write FUNCTION_EX(uart_ao_b,
>> _gpioz), resulting in uart_ao_b_gpioz, which is what is apparently considered
>> 'not clean'
>> 
> for the benefits of introducing macro 'FUNCTION_EX', it will end with
>  .name = "uart_ao_b", -> same for both EE, AO domain, and it will match
> the DT part (although still different for '.groups')
>
>
>> BTW, there no cleanness issue here, the name is just out of the 'usual scheme'
>> but there is no problem with. If you want to change this, and
>> s/uart_ao_b_gpioz/uart_ao_b_z/, now is the time to change it. 
>> 
> I'd rather *NOT* to push a pinctrl patch for just changing
> 'uart_ao_b_gpioz' to 'uart_ao_b_z' (it's a cosmetic change, and still
> end with two different name - 'uart_ao_b_gpioz/z' & 'uart_ao_b' in DT)

FWIW, I agree with Jerome.

Rather than this patch adding a(nother) hard-to-understand macro, please
submit a pinctrl rename/cleanup to s/uart_ao_b_gpioz/uart_ao_b_z/.
Since there are not any users of the _gpioz name, now is the time to do
it.

We're already using the _x _y _z suffixes all over the place, and IMO,
adding this new macro would make that even more confusing that it
already is.

Kevin

Re: [PATCH 0/2] pinctrl: meson: use one uniform 'function' name

[Yixun Lan]

On 01/17/2018 08:14 AM, Kevin Hilman wrote:
> Yixun Lan <yixun.lan@amlogic.com> writes:
> 
>> Hi Jerome:
>>
>> On 01/10/2018 03:28 PM, Jerome Brunet wrote:
>>> On Wed, 2018-01-10 at 10:12 +0800, Yixun Lan wrote:
>>>>
>>>> On 01/08/18 16:52, Jerome Brunet wrote:
>>>>> On Mon, 2018-01-08 at 15:33 +0800, Yixun Lan wrote:
>>>>>> These two patches are general improvement for meson pinctrl driver.
>>>>>> It make the two pinctrl trees (ee/ao) to share one uniform 'function' name for
>>>>>> one hardware block even its pin groups live inside two differet hardware domains,
>>>>>> which for example EE vs AO domain here.
>>>>>>
>>>>>> This idea is motivated by Martin's question at [1]
>>>>>>
>>>>>> [1]
>>>>>>  http://lkml.kernel.org/r/CAFBinCCuQ-NK747+GHDkhZty_UMMgzCYOYFcNTrRDJgU8OM=Gw@mail.gmail.com
>>>>>>
>>>>>>
>>>>>> Yixun Lan (2):
>>>>>>   pinctrl: meson: introduce a macro to have name/groups seperated
>>>>>>   pinctrl: meson-axg: correct the pin expansion of UART_AO_B
>>>>>>
>>>>>>  drivers/pinctrl/meson/pinctrl-meson-axg.c | 4 ++--
>>>>>>  drivers/pinctrl/meson/pinctrl-meson.h     | 8 +++++---
>>>>>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>>>>
>>>>> Hi Yixun,
>>>>>
>>>>> Honestly, I don't like the idea. I think it adds an unnecessary complexity.
>>>>> I don't see the point of FUNCTION_EX(uart_ao_b, _z) when you could simply write 
>>>>> FUNCTION(uart_ao_b_z) ... especially when there is just a couple of function per
>>>>> SoC available on different domains.
>>>>>
>>>>> A pinctrl driver can already be challenging to understand at first, let's keep
>>>>> it simple and avoid adding more macros.
>>>>>
>>>>
>>>> Hi Jerome：
>>>>   In my opinion, the idea of keeping one uniform 'function' in DT (thus
>>>> introducing another macro) is worth considering. It would make the DT
>>>> part much clean.
>>>
>>> Ok this is your opinion. I don't share it. Keeping function names tidy is good,
>>> I don't think we need another macro to do so.
>>>
>>>>   And yes, it's a trade-off here, either we 1) do more in code to make
>>>> DT clean or 2) do nothing in the code level to make DT live with it.
>>>
>>> I don't see how adding a macro doing just string concatenation is going to make
>>> anything more clean. It does not prevent one to write FUNCTION_EX(uart_ao_b,
>>> _gpioz), resulting in uart_ao_b_gpioz, which is what is apparently considered
>>> 'not clean'
>>>
>> for the benefits of introducing macro 'FUNCTION_EX', it will end with
>>  .name = "uart_ao_b", -> same for both EE, AO domain, and it will match
>> the DT part (although still different for '.groups')
>>
>>
>>> BTW, there no cleanness issue here, the name is just out of the 'usual scheme'
>>> but there is no problem with. If you want to change this, and
>>> s/uart_ao_b_gpioz/uart_ao_b_z/, now is the time to change it. 
>>>
>> I'd rather *NOT* to push a pinctrl patch for just changing
>> 'uart_ao_b_gpioz' to 'uart_ao_b_z' (it's a cosmetic change, and still
>> end with two different name - 'uart_ao_b_gpioz/z' & 'uart_ao_b' in DT)
> 
> FWIW, I agree with Jerome.
> 
> Rather than this patch adding a(nother) hard-to-understand macro, please
> submit a pinctrl rename/cleanup to s/uart_ao_b_gpioz/uart_ao_b_z/.
> Since there are not any users of the _gpioz name, now is the time to do
> it.
> 
> We're already using the _x _y _z suffixes all over the place, and IMO,
> adding this new macro would make that even more confusing that it
> already is.
> 
> Kevin
> 

HI Linus
  In this case, please drop this series, and I will send another patch
which simply adjust the pin group name.
  Thanks

Yixun

Re: [PATCH 0/2] pinctrl: meson: use one uniform 'function' name

[Linus Walleij]

On Mon, Jan 8, 2018 at 8:33 AM, Yixun Lan <yixun.lan@amlogic.com> wrote:

> These two patches are general improvement for meson pinctrl driver.
> It make the two pinctrl trees (ee/ao) to share one uniform 'function' name for
> one hardware block even its pin groups live inside two differet hardware domains,
> which for example EE vs AO domain here.
>
> This idea is motivated by Martin's question at [1]
>
> [1]
>  http://lkml.kernel.org/r/CAFBinCCuQ-NK747+GHDkhZty_UMMgzCYOYFcNTrRDJgU8OM=Gw@mail.gmail.com

There seems to be controversy here so I'd like input from Carlo
and/or Beniamino if possible.

Yours,
Linus Walleij