On Wed, 2019-08-28 at 14:37 +0200, Stefan Roese wrote:
On 28.08.19 14:26, Stefan Roese wrote:
On 28.08.19 08:37, Weijie Gao wrote:
This patch adds pinctrl support for mt7628, with a file for common pinmux functions and a file for mt7628 which has additional support for pinconf.
Signed-off-by: Weijie Gao weijie.gao@mediatek.com
drivers/pinctrl/Kconfig | 1 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/mtmips/Kconfig | 13 + drivers/pinctrl/mtmips/Makefile | 7 + drivers/pinctrl/mtmips/pinctrl-mt7628.c | 585 ++++++++++++++++++ .../pinctrl/mtmips/pinctrl-mtmips-common.c | 87 +++ .../pinctrl/mtmips/pinctrl-mtmips-common.h | 53 ++ 7 files changed, 747 insertions(+) create mode 100644 drivers/pinctrl/mtmips/Kconfig create mode 100644 drivers/pinctrl/mtmips/Makefile create mode 100644 drivers/pinctrl/mtmips/pinctrl-mt7628.c create mode 100644 drivers/pinctrl/mtmips/pinctrl-mtmips-common.c create mode 100644 drivers/pinctrl/mtmips/pinctrl-mtmips-common.h
Nice patch. I do have 2 questions though:
a) Why are you introducing a new "mtmips" directory and don't re-use the already available "mediatek" directory? Is there nothing in common with these "mediatek" drivers?
b) Somewhat related: You introduce a mtmips-common file. For which platforms is this targeted (non-mt7628)? Is there nothing in common with the "mediatek" files already available?
Other than that I've tested this on my MT7688 board and it works just fine. Thanks a lot!
I do have another comment though:
I've used the common "pinctrl-single" driver in Linux a few weeks ago as there is no need for a separate MT7628 specific pin-mux driver [1][2] etc. Frankly, I don't know that status of the "pinctrl-single" U-Boot driver in depth. If its compatible with the Linux one (which I really hope), then we don't need a MT7628 specific pinctrl driver but can use the "pinctrl-single" driver as I've done in the Linux [1][2].
It would be great if you could check this and change this pinctrl support to the common "single" driver is possible.
Thanks, Stefan
[1] https://github.com/torvalds/linux/commit/380f072c57a590d7593050b8533d88e18b6... [2] https://github.com/torvalds/linux/commit/6394de396ed36f3e8043734676eaa9c26f8...
Although I am trying to write the pinctrl driver to be similar with the "mediatek" one, they cannot share the same codebase because the register map and definition are totally different. The "mtmips" chips have separate pinmux registers, pinconf registers and gpio registers, while the "mediatek" chips have uniformed registers for all these three functionalities.
I introduced the common file to reserve space for mt7620.
If I use pinctrl-single, I still have to write a new driver for the pinconf function and I'd rather just write them into one driver.
Besides the pinmux function takes advantages from the OpenWrt source. Since some pinmux groups have 4 different function, I think is more readable using the string configuration. And the users don't need to read the programming guide to set the registers/shift/mask for a single pinmux function.
Please see https://github.com/torvalds/linux/blob/master/arch/mips/ralink/mt7620.c
Best Regards,
Weijie