Messages in this thread Patch in this message | | | From | Laurent Pinchart <> | Subject | Re: [PATCH] pinctrl: sh-pfc: Initial r7s72100 support | Date | Wed, 27 Nov 2013 11:54:42 +0100 |
| |
Hi Magnus,
Thank you for the patch.
On Wednesday 27 November 2013 16:41:22 Magnus Damm wrote: > From: Magnus Damm <damm@opensource.se> > > Add r7s72100 PINCTRL support via sh-pfc. At this point this code > provides enough support to allow use together with the gpio-rz > driver. Incremental per-device patches will in the future be > submitted on top of this base patch to allow PINCTRL enablement > for each individual device. > > Signed-off-by: Magnus Damm <damm@opensource.se> > --- > > drivers/pinctrl/sh-pfc/Kconfig | 5 > drivers/pinctrl/sh-pfc/Makefile | 1 > drivers/pinctrl/sh-pfc/core.c | 9 + > drivers/pinctrl/sh-pfc/core.h | 1 > drivers/pinctrl/sh-pfc/pfc-r7s72100.c | 166 ++++++++++++++++++++++++++++++ > 5 files changed, 182 insertions(+)
[snip]
> --- /dev/null > +++ work/drivers/pinctrl/sh-pfc/pfc-r7s72100.c 2013-11-27 15:20:28.000000000 > +0900 @@ -0,0 +1,166 @@
[snip]
> +#define RZ_PORT_PIN(bank, pin) (((bank) * 16) + (pin))
Could you move this below closer to the location where it will be used, right after the pinmux_pins array ?
> + > +#define PORT_P_1(bank, pin, fn, sfx) fn(bank, pin, P_##bank##_##pin, sfx)
I'm tempted to call the pins GP_ instead of P_ to be able to reuse the existing macros. On the other hand I suppose the datasheet uses P, not GP. What's your opinion on that ?
> + > +#define PORT_P_16(bank, fn, sfx) \ > + PORT_P_1(bank, 0, fn, sfx), PORT_P_1(bank, 1, fn, sfx), \ > + PORT_P_1(bank, 2, fn, sfx), PORT_P_1(bank, 3, fn, sfx), \ > + PORT_P_1(bank, 4, fn, sfx), PORT_P_1(bank, 5, fn, sfx), \ > + PORT_P_1(bank, 6, fn, sfx), PORT_P_1(bank, 7, fn, sfx), \ > + PORT_P_1(bank, 8, fn, sfx), PORT_P_1(bank, 9, fn, sfx), \ > + PORT_P_1(bank, 10, fn, sfx), PORT_P_1(bank, 11, fn, sfx), \ > + PORT_P_1(bank, 12, fn, sfx), PORT_P_1(bank, 13, fn, sfx), \ > + PORT_P_1(bank, 14, fn, sfx), PORT_P_1(bank, 15, fn, sfx) > + > +#define CPU_ALL_PORT(fn, sfx) \ > + PORT_P_16(0, fn, sfx), PORT_P_16(1, fn, sfx), \ > + PORT_P_16(2, fn, sfx), PORT_P_16(3, fn, sfx), \ > + PORT_P_16(4, fn, sfx), PORT_P_16(5, fn, sfx), \ > + PORT_P_16(6, fn, sfx), PORT_P_16(7, fn, sfx), \ > + PORT_P_16(8, fn, sfx), PORT_P_16(9, fn, sfx), \ > + PORT_P_16(10, fn, sfx), PORT_P_16(11, fn, sfx), \ > + PORT_P_16(12, fn, sfx) > + > +#define P_ALL(n) GP_ALL(n) > + > +enum { > + PINMUX_RESERVED = 0, > + > + PINMUX_DATA_BEGIN, > + P_ALL(DATA), > + PINMUX_DATA_END, > + > + PINMUX_FUNCTION_BEGIN, > + P_ALL(PMC_0), P_ALL(PMC_1), > + P_ALL(PFC_0), P_ALL(PFC_1), > + P_ALL(PFCE_0), P_ALL(PFCE_1), > + P_ALL(PFCAE_0), P_ALL(PFCAE_1), > + P_ALL(PIBC_0), P_ALL(PIBC_1), > + P_ALL(PBDC_0), P_ALL(PBDC_1), > + P_ALL(PIPC_0), P_ALL(PIPC_1), > + PINMUX_FUNCTION_END,
Only the PMC_1 and PFC* registers seem to be used in your later patches. What are the other registers for ? Shouldn't they be omitted if unused ? > + > + PINMUX_MARK_BEGIN, > + P_ALL(MARK_FN1), P_ALL(MARK_FN2), P_ALL(MARK_FN3), P_ALL(MARK_FN4), > + P_ALL(MARK_FN5), P_ALL(MARK_FN6), P_ALL(MARK_FN7), P_ALL(MARK_FN8), > + PINMUX_MARK_END, > +}; > + > +#define _P_ALL(n) CPU_ALL_PORT(n, unused) > + > +#define _P_GPIO(bank, _pin, _name, sfx) \ > + [(bank * 16) + _pin] = { \ > + .pin = (bank * 16) + _pin, \ > + .name = __stringify(_name), \ > + .enum_id = _name##_DATA, \ > + }
What about renaming _GP_GPIO to _GP_GPIO32 in sh_pfc.h, and move this macro to that header, calling it _GP_GPIO16 ?
> + > +#define _P_DATA(bank, pin, name, sfx) \ > + PINMUX_DATA(name##_DATA, name##_PMC_0, \ > + name##_PIBC_1, name##_PBDC_1) > + > +#define _P_FN(n, fn, pfcae, pfce, pfc) \ > + PINMUX_DATA(n##_MARK_FN##fn, n##_PMC_1, \ > + n##_PFCAE_##pfcae, n##_PFCE_##pfce, n##_PFC_##pfc) > + > +#define _P_MARK_FN1(bank, pin, name, sfx) _P_FN(name, 1, 0, 0, 0) > +#define _P_MARK_FN2(bank, pin, name, sfx) _P_FN(name, 2, 0, 0, 1) > +#define _P_MARK_FN3(bank, pin, name, sfx) _P_FN(name, 3, 0, 1, 0) > +#define _P_MARK_FN4(bank, pin, name, sfx) _P_FN(name, 4, 0, 1, 1) > +#define _P_MARK_FN5(bank, pin, name, sfx) _P_FN(name, 5, 1, 0, 0) > +#define _P_MARK_FN6(bank, pin, name, sfx) _P_FN(name, 6, 1, 0, 1) > +#define _P_MARK_FN7(bank, pin, name, sfx) _P_FN(name, 7, 1, 1, 0) > +#define _P_MARK_FN8(bank, pin, name, sfx) _P_FN(name, 8, 1, 1, 1) > + > +static const u16 pinmux_data[] = { > + _P_ALL(_P_DATA), /* PINMUX_DATA(P_M_N_DATA, P_M_N_PMC_0), */ > + _P_ALL(_P_MARK_FN1), _P_ALL(_P_MARK_FN2), > + _P_ALL(_P_MARK_FN3), _P_ALL(_P_MARK_FN4), > + _P_ALL(_P_MARK_FN5), _P_ALL(_P_MARK_FN6), > + _P_ALL(_P_MARK_FN7), _P_ALL(_P_MARK_FN8), > +}; > + > +static struct sh_pfc_pin pinmux_pins[] = { > + _P_ALL(_P_GPIO), > +}; > + > +static const struct sh_pfc_pin_group pinmux_groups[] = { > +}; > + > +static const struct sh_pfc_function pinmux_functions[] = { > +}; > + > +#define PFC_REG(idx, name, reg) \ > + { PINMUX_CFG_REG(__stringify(name), reg, 16, 1) { \ > + P_##idx##_15_##name##_0, P_##idx##_15_##name##_1, \ > + P_##idx##_14_##name##_0, P_##idx##_14_##name##_1, \ > + P_##idx##_13_##name##_0, P_##idx##_13_##name##_1, \ > + P_##idx##_12_##name##_0, P_##idx##_12_##name##_1, \ > + P_##idx##_11_##name##_0, P_##idx##_11_##name##_1, \ > + P_##idx##_10_##name##_0, P_##idx##_10_##name##_1, \ > + P_##idx##_9_##name##_0, P_##idx##_9_##name##_1, \ > + P_##idx##_8_##name##_0, P_##idx##_8_##name##_1, \ > + P_##idx##_7_##name##_0, P_##idx##_7_##name##_1, \ > + P_##idx##_6_##name##_0, P_##idx##_6_##name##_1, \ > + P_##idx##_5_##name##_0, P_##idx##_5_##name##_1, \ > + P_##idx##_4_##name##_0, P_##idx##_4_##name##_1, \ > + P_##idx##_3_##name##_0, P_##idx##_3_##name##_1, \ > + P_##idx##_2_##name##_0, P_##idx##_2_##name##_1, \ > + P_##idx##_1_##name##_0, P_##idx##_1_##name##_1, \ > + P_##idx##_0_##name##_0, P_##idx##_0_##name##_1 } \ > + } > + > +#define PFC_REGS(idx) \ > + PFC_REG(idx, PMC, (0xfcfe3400 + (idx * 4))), \ > + PFC_REG(idx, PFC, (0xfcfe3500 + (idx * 4))), \ > + PFC_REG(idx, PFCE, (0xfcfe3600 + (idx * 4))), \ > + PFC_REG(idx, PFCAE, (0xfcfe3a00 + (idx * 4))), \ > + PFC_REG(idx, PIBC, (0xfcfe7000 + (idx * 4))), \ > + PFC_REG(idx, PBDC, (0xfcfe7100 + (idx * 4))), \ > + PFC_REG(idx, PIPC, (0xfcfe7200 + (idx * 4))) > + > +static struct pinmux_cfg_reg pinmux_config_regs[] = { > + PFC_REGS(0), PFC_REGS(1), PFC_REGS(2), PFC_REGS(3), > + PFC_REGS(4), PFC_REGS(5), PFC_REGS(6), PFC_REGS(7), > + PFC_REGS(8), PFC_REGS(9), PFC_REGS(10), PFC_REGS(11), > + PFC_REG(12, PMC, 0xfcfe7b40), > + PFC_REG(12, PIBC, 0xfcfe7f00), > + { }, > +}; > + > +const struct sh_pfc_soc_info r7s72100_pinmux_info = { > + .name = "r7s72100_pfc", > + > + .function = { PINMUX_FUNCTION_BEGIN, PINMUX_FUNCTION_END }, > + > + .pins = pinmux_pins, > + .nr_pins = ARRAY_SIZE(pinmux_pins), > + .groups = pinmux_groups, > + .nr_groups = ARRAY_SIZE(pinmux_groups), > + .functions = pinmux_functions, > + .nr_functions = ARRAY_SIZE(pinmux_functions), > + > + .cfg_regs = pinmux_config_regs, > + > + .gpio_data = pinmux_data, > + .gpio_data_size = ARRAY_SIZE(pinmux_data), > +};
-- Regards,
Laurent Pinchart
| |