Re: [PATCH v5 28/31] pinctrl: Add support for the Lantic PEF2256 pinmux
On Tue, 12 Sep 2023 13:04:56 +0200
Linus Walleij wrote:
> Hi Herve,
>
> thanks for your patch!
>
> On Tue, Sep 12, 2023 at 12:15 PM Herve Codina
> 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
> > Reviewed-by: Christophe Leroy
> > Signed-off-by: Christophe Leroy
>
> 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_PEF2256
> > + select PINMUX
>
> select PINCONF
Will be added in the next iteration.
>
> > + select GENERIC_PINCONF
>
> This 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 = _pctlops;
> > + pef2256->pctrl_desc.pmxops = _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, >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
>
> Yours,
> Linus Walleij
Thanks for your comment.
Best regards,
Hervé
Re: [PATCH v5 28/31] pinctrl: Add support for the Lantic PEF2256 pinmux
On Tue, Sep 12, 2023 at 4:31 PM Mark Brown wrote: > On Tue, Sep 12, 2023 at 01:04:56PM +0200, Linus Walleij wrote: > > On Tue, Sep 12, 2023 at 12:15 PM Herve Codina > > wrote: > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +/* > > > I think SPDX mandates that you start the tag with C99 comments > > > // SPDX-License-Identifier: GPL-2.0-only > > Not for headers, they should use C style since they might be included in > contexts where C++ isn't supported. Oh right. Thanks Mark! Yours, Linus Walleij
Re: [PATCH v5 28/31] pinctrl: Add support for the Lantic PEF2256 pinmux
On Tue, Sep 12, 2023 at 01:04:56PM +0200, Linus Walleij wrote: > On Tue, Sep 12, 2023 at 12:15 PM Herve Codina > wrote: > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > I think SPDX mandates that you start the tag with C99 comments > // SPDX-License-Identifier: GPL-2.0-only Not for headers, they should use C style since they might be included in contexts where C++ isn't supported. signature.asc Description: PGP signature
Re: [PATCH v5 28/31] pinctrl: Add support for the Lantic PEF2256 pinmux
Hi Herve,
thanks for your patch!
On Tue, Sep 12, 2023 at 12:15 PM Herve Codina 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
> Reviewed-by: Christophe Leroy
> Signed-off-by: Christophe Leroy
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.
> +config PINCTRL_PEF2256
> + tristate "Lantiq PEF2256 (FALC56) pin controller driver"
> + depends on OF && FRAMER_PEF2256
> + select PINMUX
select PINCONF
> + select GENERIC_PINCONF
This 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
// 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 = _pctlops;
> + pef2256->pctrl_desc.pmxops = _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, >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(...);
> + 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
Yours,
Linus Walleij
[PATCH v5 28/31] pinctrl: Add support for the Lantic PEF2256 pinmux
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 Reviewed-by: Christophe Leroy Signed-off-by: Christophe Leroy --- drivers/pinctrl/Kconfig| 14 ++ drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-pef2256-regs.h | 65 ++ drivers/pinctrl/pinctrl-pef2256.c | 308 + 4 files changed, 388 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-pef2256-regs.h create mode 100644 drivers/pinctrl/pinctrl-pef2256.c diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 7dfb7190580e..2e8687310c61 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -366,6 +366,20 @@ config PINCTRL_PALMAS open drain configuration for the Palmas series devices like TPS65913, TPS80036 etc. +config PINCTRL_PEF2256 + tristate "Lantiq PEF2256 (FALC56) pin controller driver" + depends on OF && FRAMER_PEF2256 + select PINMUX + select GENERIC_PINCONF + help + This option enables the pin controller support for the Lantiq PEF2256 + framer, also known as FALC56. + + If unsure, say N. + + To compile this driver as a module, choose M here: the + module will be called pinctrl-pef2256. + config PINCTRL_PIC32 bool "Microchip PIC32 pin controller driver" depends on OF diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index dd6cda270294..800c4219fcc1 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -39,6 +39,7 @@ obj-$(CONFIG_PINCTRL_MICROCHIP_SGPIO) += pinctrl-microchip-sgpio.o obj-$(CONFIG_PINCTRL_MLXBF3) += pinctrl-mlxbf3.o obj-$(CONFIG_PINCTRL_OCELOT) += pinctrl-ocelot.o obj-$(CONFIG_PINCTRL_PALMAS) += pinctrl-palmas.o +obj-$(CONFIG_PINCTRL_PEF2256) += pinctrl-pef2256.o obj-$(CONFIG_PINCTRL_PIC32)+= pinctrl-pic32.o obj-$(CONFIG_PINCTRL_PISTACHIO)+= pinctrl-pistachio.o obj-$(CONFIG_PINCTRL_RK805)+= pinctrl-rk805.o diff --git a/drivers/pinctrl/pinctrl-pef2256-regs.h b/drivers/pinctrl/pinctrl-pef2256-regs.h new file mode 100644 index ..cacc492a1623 --- /dev/null +++ b/drivers/pinctrl/pinctrl-pef2256-regs.h @@ -0,0 +1,65 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * PEF2256 pinctrl registers definition + * + * Copyright 2023 CS GROUP France + * + * Author: Herve Codina + */ +#ifndef __PEF2256_PINCTRL_REGS_H__ +#define __PEF2256_PINCTRL_REGS_H__ + +#include + +/* Port Configuration 1..4 */ +#define PEF2256_PC1 0x80 +#define PEF2256_PC2 0x81 +#define PEF2256_PC3 0x82 +#define PEF2256_PC4 0x83 +#define PEF2256_12_PC_RPC_MASK GENMASK(6, 4) +#define PEF2256_12_PC_RPC_SYPR FIELD_PREP_CONST(PEF2256_12_PC_RPC_MASK, 0x0) +#define PEF2256_12_PC_RPC_RFMFIELD_PREP_CONST(PEF2256_12_PC_RPC_MASK, 0x1) +#define PEF2256_12_PC_RPC_RFMB FIELD_PREP_CONST(PEF2256_12_PC_RPC_MASK, 0x2) +#define PEF2256_12_PC_RPC_RSIGM FIELD_PREP_CONST(PEF2256_12_PC_RPC_MASK, 0x3) +#define PEF2256_12_PC_RPC_RSIG FIELD_PREP_CONST(PEF2256_12_PC_RPC_MASK, 0x4) +#define PEF2256_12_PC_RPC_DLRFIELD_PREP_CONST(PEF2256_12_PC_RPC_MASK, 0x5) +#define PEF2256_12_PC_RPC_FREEZE FIELD_PREP_CONST(PEF2256_12_PC_RPC_MASK, 0x6) +#define PEF2256_12_PC_RPC_RFSP FIELD_PREP_CONST(PEF2256_12_PC_RPC_MASK, 0x7) +#define PEF2256_12_PC_XPC_MASKGENMASK(4, 0) +#define PEF2256_12_PC_XPC_SYPX FIELD_PREP_CONST(PEF2256_12_PC_XPC_MASK, 0x0) +#define PEF2256_12_PC_XPC_XFMS FIELD_PREP_CONST(PEF2256_12_PC_XPC_MASK, 0x1) +#define PEF2256_12_PC_XPC_XSIG FIELD_PREP_CONST(PEF2256_12_PC_XPC_MASK, 0x2) +#define PEF2256_12_PC_XPC_TCLK FIELD_PREP_CONST(PEF2256_12_PC_XPC_MASK, 0x3) +#define PEF2256_12_PC_XPC_XMFB FIELD_PREP_CONST(PEF2256_12_PC_XPC_MASK, 0x4) +#define PEF2256_12_PC_XPC_XSIGM FIELD_PREP_CONST(PEF2256_12_PC_XPC_MASK, 0x5) +#define PEF2256_12_PC_XPC_DLXFIELD_PREP_CONST(PEF2256_12_PC_XPC_MASK, 0x6) +#define PEF2256_12_PC_XPC_XCLK FIELD_PREP_CONST(PEF2256_12_PC_XPC_MASK, 0x7) +#define PEF2256_12_PC_XPC_XLTFIELD_PREP_CONST(PEF2256_12_PC_XPC_MASK, 0x8) +#define PEF2256_2X_PC_RPC_MASK GENMASK(7, 4) +#define PEF2256_2X_PC_RPC_SYPR FIELD_PREP_CONST(PEF2256_2X_PC_RPC_MASK, 0x0) +#define PEF2256_2X_PC_RPC_RFMFIELD_PREP_CONST(PEF2256_2X_PC_RPC_MASK, 0x1) +#define PEF2256_2X_PC_RPC_RFMB FIELD_PREP_CONST(PEF2256_2X_PC_RPC_MASK, 0x2) +#define PEF2256_2X_PC_RPC_RSIGM
