On Tue, 12 Sep 2023 13:04:56 +0200 Linus Walleij linus.walleij@linaro.org wrote:
Hi Herve,
thanks for your patch!
On Tue, Sep 12, 2023 at 12:15 PM Herve Codina herve.codina@bootlin.com wrote:
The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.
This kind of component can be found in old telecommunication system. It was used to digital transmission of many simultaneous telephone calls by time-division multiplexing. Also using HDLC protocol, WAN networks can be reached through the framer.
This pinmux support handles the pin muxing part (pins RP(A..D) and pins XP(A..D)) of the PEF2256.
Signed-off-by: Herve Codina herve.codina@bootlin.com Reviewed-by: Christophe Leroy christophe.leroy@csgroup.eu Signed-off-by: Christophe Leroy christophe.leroy@csgroup.eu
Nice to see this as a proper pin control driver!
drivers/pinctrl/pinctrl-pef2256-regs.h | 65 ++++++ drivers/pinctrl/pinctrl-pef2256.c | 308 +++++++++++++++++++++++++
Do you really need a separate header just for some registers? But it's a matter of taste so I'm not gonna complain if you want it this way.
Will be move to the .c file in the next iteration.
+config PINCTRL_PEF2256
tristate "Lantiq PEF2256 (FALC56) pin controller driver"depends on OF && FRAMER_PEF2256select PINMUXselect PINCONF
Will be added in the next iteration.
select GENERIC_PINCONFThis brings it in implicitly but I prefer that you just select it.
+/* SPDX-License-Identifier: GPL-2.0 */ +/*
I think SPDX mandates that you start the tag with C99 comments
Already replied by Mark, C style comment is correct -> No change.
// SPDX-License-Identifier: GPL-2.0-only
/* We map 1 group <-> 1 pin */Also known as "the qualcomm trick", but hey: it's fine.
+static int pef2256_register_pinctrl(struct pef2256_pinctrl *pef2256) +{
struct pinctrl_dev *pctrl;pef2256->pctrl_desc.name = dev_name(pef2256->dev);pef2256->pctrl_desc.owner = THIS_MODULE;pef2256->pctrl_desc.pctlops = &pef2256_pctlops;pef2256->pctrl_desc.pmxops = &pef2256_pmxops;if (pef2256->version == PEF2256_VERSION_1_2) {pef2256->pctrl_desc.pins = pef2256_v12_pins;pef2256->pctrl_desc.npins = ARRAY_SIZE(pef2256_v12_pins);pef2256->functions = pef2256_v12_functions;pef2256->nfunctions = ARRAY_SIZE(pef2256_v12_functions);} else {pef2256->pctrl_desc.pins = pef2256_v2x_pins;pef2256->pctrl_desc.npins = ARRAY_SIZE(pef2256_v2x_pins);pef2256->functions = pef2256_v2x_functions;pef2256->nfunctions = ARRAY_SIZE(pef2256_v2x_functions);}pctrl = devm_pinctrl_register(pef2256->dev, &pef2256->pctrl_desc, pef2256);if (IS_ERR(pctrl)) {dev_err(pef2256->dev, "pinctrl driver registration failed\n");return PTR_ERR(pctrl);}return 0;You could use return dev_err_probe(...);
Indeed, I will change.
pef2256_reset_pinmux(pef2256_pinctrl);ret = pef2256_register_pinctrl(pef2256_pinctrl);if (ret)return ret;Or you could use it down here.
With or without these changes (because they are nitpicks) Reviewed-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
Thanks for your comment.
Best regards, Hervé