[PATCH][RFC] sh-pfc: add new PINMUX_IPSR_MODS() macro

[PATCH][RFC] sh-pfc: add new PINMUX_IPSR_MODS() macro

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Actually, PINMUX_IPSR_MODS() macro is same as PINMUX_IPSR_MODSEL_DATA().
Current PFC driver is very difficult to read, because macro names are
using different lenth. Especially PINMUX_IPSR_MODSEL_DATA() is well
used macro

	PINMUX_IPSR_NOGP(ispr, ...)
	PINMUX_IPSR_DATA(ipsr, ...)
	PINMUX_IPSR_NOGM(ispr, ...)
	PINMUX_IPSR_NOFN(ipsr, ...)
	PINMUX_IPSR_MSEL(ipsr, ...)
	PINMUX_IPSR_MODSEL_DATA(ipsr, ...)

These are readable

	PINMUX_IPSR_NOGP(ispr, ...)
	PINMUX_IPSR_DATA(ipsr, ...)
	PINMUX_IPSR_NOGM(ispr, ...)
	PINMUX_IPSR_NOFN(ipsr, ...)
	PINMUX_IPSR_MSEL(ipsr, ...)
	PINMUX_IPSR_MODS(ipsr, ...)

We can replace all PINMUX_IPSR_MODSEL_DATA() to PINMUX_IPSR_MODS(),
and remove PINMUX_IPSR_MODSEL_DATA() from header.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
> LinusW, Laurent

As I mentioned above, if this patch was accepted, I can replace all PINMUX_IPSR_MODSEL_DATA()
to PINMUX_IPSR_MODS(). But I didn't it since it is [RFC] now

 drivers/pinctrl/sh-pfc/sh_pfc.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index 0874cfe..40447d54 100644
--- a/drivers/pinctrl/sh-pfc/sh_pfc.h
+++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
@@ -173,6 +173,8 @@ struct sh_pfc_soc_info {
 	PINMUX_DATA(fn##_MARK, FN_##ipsr, FN_##ms)
 #define PINMUX_IPSR_MSEL(ipsr, fn, ms)					\
 	PINMUX_DATA(fn##_MARK, FN_##fn, FN_##ipsr, FN_##ms)
+#define PINMUX_IPSR_MODS(ipsr, fn, ms)			\
+	PINMUX_DATA(fn##_MARK, FN_##ms, FN_##ipsr, FN_##fn)
 #define PINMUX_IPSR_MODSEL_DATA(ipsr, fn, ms)				\
 	PINMUX_DATA(fn##_MARK, FN_##ms, FN_##ipsr, FN_##fn)
 
-- 
1.9.1

Re: [PATCH][RFC] sh-pfc: add new PINMUX_IPSR_MODS() macro

[Laurent Pinchart]

Hi Morimoto-san,

On Friday 28 August 2015 14:20:41 Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Actually, PINMUX_IPSR_MODS() macro is same as PINMUX_IPSR_MODSEL_DATA().
> Current PFC driver is very difficult to read, because macro names are
> using different lenth. Especially PINMUX_IPSR_MODSEL_DATA() is well
> used macro
> 
> 	PINMUX_IPSR_NOGP(ispr, ...)
> 	PINMUX_IPSR_DATA(ipsr, ...)
> 	PINMUX_IPSR_NOGM(ispr, ...)
> 	PINMUX_IPSR_NOFN(ipsr, ...)
> 	PINMUX_IPSR_MSEL(ipsr, ...)
> 	PINMUX_IPSR_MODSEL_DATA(ipsr, ...)
> 
> These are readable
> 
> 	PINMUX_IPSR_NOGP(ispr, ...)
> 	PINMUX_IPSR_DATA(ipsr, ...)
> 	PINMUX_IPSR_NOGM(ispr, ...)
> 	PINMUX_IPSR_NOFN(ipsr, ...)
> 	PINMUX_IPSR_MSEL(ipsr, ...)
> 	PINMUX_IPSR_MODS(ipsr, ...)
> 
> We can replace all PINMUX_IPSR_MODSEL_DATA() to PINMUX_IPSR_MODS(),
> and remove PINMUX_IPSR_MODSEL_DATA() from header.

I agree that the PINMUX_IPSR_MODSEL_DATA() name makes code harder to read. 
However, PINMUX_IPSR_MODS() isn't very descriptive, I think it would make the 
code confusing (not that it isn't already...).

The only difference between  PINMUX_IPSR_MSEL and PINMUX_IPSR_MODSEL_DATA is 
the order in which the MODSEL, GSPR and IPSR registers are written. I wonder 
if that's actually important, or if we could merge both macros into a single 
one. What do you think ?


> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> 
> > LinusW, Laurent
> 
> As I mentioned above, if this patch was accepted, I can replace all
> PINMUX_IPSR_MODSEL_DATA() to PINMUX_IPSR_MODS(). But I didn't it since it
> is [RFC] now
> 
>  drivers/pinctrl/sh-pfc/sh_pfc.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h
> b/drivers/pinctrl/sh-pfc/sh_pfc.h index 0874cfe..40447d54 100644
> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> @@ -173,6 +173,8 @@ struct sh_pfc_soc_info {
>  	PINMUX_DATA(fn##_MARK, FN_##ipsr, FN_##ms)
>  #define PINMUX_IPSR_MSEL(ipsr, fn, ms)					\
>  	PINMUX_DATA(fn##_MARK, FN_##fn, FN_##ipsr, FN_##ms)
> +#define PINMUX_IPSR_MODS(ipsr, fn, ms)			\
> +	PINMUX_DATA(fn##_MARK, FN_##ms, FN_##ipsr, FN_##fn)
>  #define PINMUX_IPSR_MODSEL_DATA(ipsr, fn, ms)				\
>  	PINMUX_DATA(fn##_MARK, FN_##ms, FN_##ipsr, FN_##fn)

-- 
Regards,

Laurent Pinchart

Re: [PATCH][RFC] sh-pfc: add new PINMUX_IPSR_MODS() macro

[Kuninori Morimoto]

Hi Laurent

> > 	PINMUX_IPSR_NOGP(ispr, ...)
> > 	PINMUX_IPSR_DATA(ipsr, ...)
> > 	PINMUX_IPSR_NOGM(ispr, ...)
> > 	PINMUX_IPSR_NOFN(ipsr, ...)
> > 	PINMUX_IPSR_MSEL(ipsr, ...)
> > 	PINMUX_IPSR_MODSEL_DATA(ipsr, ...)
> > 
> > These are readable
> > 
> > 	PINMUX_IPSR_NOGP(ispr, ...)
> > 	PINMUX_IPSR_DATA(ipsr, ...)
> > 	PINMUX_IPSR_NOGM(ispr, ...)
> > 	PINMUX_IPSR_NOFN(ipsr, ...)
> > 	PINMUX_IPSR_MSEL(ipsr, ...)
> > 	PINMUX_IPSR_MODS(ipsr, ...)
> > 
> > We can replace all PINMUX_IPSR_MODSEL_DATA() to PINMUX_IPSR_MODS(),
> > and remove PINMUX_IPSR_MODSEL_DATA() from header.
> 
> I agree that the PINMUX_IPSR_MODSEL_DATA() name makes code harder to read. 
> However, PINMUX_IPSR_MODS() isn't very descriptive, I think it would make the 
> code confusing (not that it isn't already...).
> 
> The only difference between  PINMUX_IPSR_MSEL and PINMUX_IPSR_MODSEL_DATA is 
> the order in which the MODSEL, GSPR and IPSR registers are written. I wonder 
> if that's actually important, or if we could merge both macros into a single 
> one. What do you think ?

Yes, I agree about this.
But, PINMUX_IPSR_MSEL is used on r8a7778 only, and my concern is we don't know what happen
if we merged PINMUX_IPSR_MODSEL_DATA() and PINMUX_IPSR_MSEL()
And, I don't have bockw board now...

Re: [PATCH][RFC] sh-pfc: add new PINMUX_IPSR_MODS() macro

[Kuninori Morimoto]

Hi Ulrich

> > > 	PINMUX_IPSR_NOGP(ispr, ...)
> > > 	PINMUX_IPSR_DATA(ipsr, ...)
> > > 	PINMUX_IPSR_NOGM(ispr, ...)
> > > 	PINMUX_IPSR_NOFN(ipsr, ...)
> > > 	PINMUX_IPSR_MSEL(ipsr, ...)
> > > 	PINMUX_IPSR_MODSEL_DATA(ipsr, ...)
> > > 
> > > These are readable
> > > 
> > > 	PINMUX_IPSR_NOGP(ispr, ...)
> > > 	PINMUX_IPSR_DATA(ipsr, ...)
> > > 	PINMUX_IPSR_NOGM(ispr, ...)
> > > 	PINMUX_IPSR_NOFN(ipsr, ...)
> > > 	PINMUX_IPSR_MSEL(ipsr, ...)
> > > 	PINMUX_IPSR_MODS(ipsr, ...)
> > > 
> > > We can replace all PINMUX_IPSR_MODSEL_DATA() to PINMUX_IPSR_MODS(),
> > > and remove PINMUX_IPSR_MODSEL_DATA() from header.
> > 
> > I agree that the PINMUX_IPSR_MODSEL_DATA() name makes code harder to read. 
> > However, PINMUX_IPSR_MODS() isn't very descriptive, I think it would make the 
> > code confusing (not that it isn't already...).
> > 
> > The only difference between  PINMUX_IPSR_MSEL and PINMUX_IPSR_MODSEL_DATA is 
> > the order in which the MODSEL, GSPR and IPSR registers are written. I wonder 
> > if that's actually important, or if we could merge both macros into a single 
> > one. What do you think ?
> 
> Yes, I agree about this.
> But, PINMUX_IPSR_MSEL is used on r8a7778 only, and my concern is we don't know what happen
> if we merged PINMUX_IPSR_MODSEL_DATA() and PINMUX_IPSR_MSEL()
> And, I don't have bockw board now...

Can you check this ?
Is r8a7778 can work correctly if PINMUX_IPSR_MSEL() and PINMUX_IPSR_MODSEL_DATA() are merged ?

  #define PINMUX_IPSR_MSEL(ipsr, fn, ms)				\
- 	PINMUX_DATA(fn##_MARK, FN_##fn, FN_##ipsr, FN_##ms)
+ 	PINMUX_DATA(fn##_MARK, FN_##ms, FN_##ipsr, FN_##fn)
  #define PINMUX_IPSR_MODSEL_DATA(ipsr, fn, ms)				\
  	PINMUX_DATA(fn##_MARK, FN_##ms, FN_##ipsr, FN_##fn)

If it works, we can use PINMUX_IPSR_MSEL() for all SoC