[linux dev-6.6 v2 0/7] pinctrl: npcm8xx: pin configuration changes
Andrew Jeffery
andrew at codeconstruct.com.au
Fri Jul 26 15:19:39 AEST 2024
On Thu, 2024-07-25 at 20:56 +0300, Tomer Maimon wrote:
> Hi Andrew,
>
> Thanks for your comments
>
> On Thu, 25 Jul 2024 at 10:29, Andrew Jeffery
> <andrew at codeconstruct.com.au> wrote:
> >
> > Hi Tomer,
> >
> > On Thu, 2024-07-18 at 21:27 +0300, Tomer Maimon wrote:
> > > This patch set addresses various pin configuration changes for the
> > > Nuvoton NPCM8XX BMC SoC. The patches aim to enhance functionality,
> > > remove unused pins, and improve overall pin management.
> > >
> > > This patchset is on the upstream process to the Linux vanilla.
> > > https://lore.kernel.org/lkml/20240716194008.3502068-1-tmaimon77@gmail.com/
> > >
> > > Changes since version 1:
> > > - Squash the non-existent pins, groups and functions.
> > > - Remove non-existent groups and functions from dt-bindings.
> > > - Modify the commit message.
> > >
> > > Tomer Maimon (7):
> > > dt-bindings: pinctrl: npcm8xx: remove non-existent groups and
> > > functions
> > > pinctrl: nuvoton: npcm8xx: remove non-existent pins, groups, functions
> > > pinctrl: nuvoton: npcm8xx: clear polarity before set both edge
> > > pinctrl: nuvoton: npcm8xx: add gpi35 and gpi36
> > > pinctrl: nuvoton: npcm8xx: add pin 250 to DDR pins group
> > > pinctrl: nuvoton: npcm8xx: modify clkrun and serirq pin configuration
> > > pinctrl: nuvoton: npcm8xx: modify pins flags
> > >
> > > .../pinctrl/nuvoton,npcm845-pinctrl.yaml | 74 +++++++++----------
> > > drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c | 64 ++++++++--------
> > > 2 files changed, 67 insertions(+), 71 deletions(-)
> > >
> >
> > I looked at applying this series. A few notes:
> >
> > 1. I did get some whitespace warnings during patch application:
> >
> > ```
> > ...
> > Applying: pinctrl: nuvoton: npcm8xx: modify pins flags
> > /home/andrew/src/kernel.org/linux.git/worktrees/openbmc/rebase-apply/patch:47: trailing whitespace.
> > nprd_smi, smb0b, smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1
> > /home/andrew/src/kernel.org/linux.git/worktrees/openbmc/rebase-apply/patch:89: trailing whitespace.
> > nprd_smi, smb0b, smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1
> > warning: 2 lines add whitespace errors.
> > ```
> >
> will fixed in V3
> > 2. There's a reasonable difference to the patches upstream. The
> > devicetree binding in particular seems to have some odd differences in
> > the enums that make me wonder whether we're missing other patches
> > there. There are also some minor differences in the driver which I
> > assume are explained by intervening changes between v6.6 and upstream.
> > Here's the diff, can you give a quick comment on each hunk?
> >
> > ```
> > $ git diff <backported patches on dev-6.6> <patches proposed upstream on c33ffdb70cc6 ("Merge tag 'phy-for-6.11' of git://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy")> -- drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> > diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> > index 4496658006ab..8cd1f442240e 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> > +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> > @@ -35,6 +35,7 @@ properties:
> > patternProperties:
> > '^gpio@':
> > type: object
> > + additionalProperties: false
> >
> > description:
> > Eight GPIO banks that each contain 32 GPIOs.
> > @@ -80,37 +81,39 @@ patternProperties:
> > fanin7, fanin8, fanin9, fanin10, fanin11, fanin12, fanin13,
> > fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, r2, r2err, r2md,
> > r3rxer, ga20kbc, smb5d, lpc, espi, rg2, ddr, i3c0, i3c1,
> > - i3c2, i3c3, i3c4, i3c5, smb0, smb1, smb2, smb2c, smb2b,
> > - smb1c, smb1b, smb8, smb9, smb10, smb11, sd1, sd1pwr, pwm4,
> > - pwm5, pwm6, pwm7, pwm8, pwm9, pwm10, pwm11, mmc8, mmc, mmcwp,
> > - mmccd, mmcrst, clkout, serirq, scipme, smi, smb6, smb7, spi1,
> > - faninx, r1, spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3,
> > - nprd_smi, smb0b, smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1
> > - wdog2, smb12, smb13, spix, spixcs1, clkreq, hgpio0, hgpio1,
> > - hgpio2, hgpio3, hgpio4, hgpio5, hgpio6, hgpio7 ]
> > + i3c2, i3c3, i3c4, i3c5, smb0, smb1, smb2, smb2c, smb2b, smb1c,
> > + smb1b, smb8, smb9, smb10, smb11, sd1, sd1pwr, pwm4, pwm5,
> > + pwm6, pwm7, pwm8, pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd,
> > + mmcrst, clkout, serirq, scipme, smi, smb6, smb6b, smb6c,
> > + smb6d, smb7, smb7b, smb7c, smb7d, spi1, faninx, r1, spi3,
> > + spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, smb0c,
> > + smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12, smb13,
> > + spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, hgpio4,
> > + hgpio5, hgpio6, hgpio7, bu4, bu4b, bu5, bu5b, bu6, gpo187 ]
> >
> > function:
> > description:
> > The function that a group of pins is muxed to
> > - enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi,
> > - smb5b, smb5c, lkgpo0, pspi, jm1, jm2, smb4b, smb4c, smb15,
> > - smb16, smb17, smb18, smb19, smb20, smb21, smb22, smb23,
> > - smb23b, smb4d, smb14, smb5, smb4, smb3, spi0cs1, spi0cs2,
> > - spi0cs3, spi1cs0, spi1cs1, spi1cs2, spi1cs3, spi1cs23, smb3c,
> > - smb3b, bmcuart0a, uart1, jtag2, bmcuart1, uart2, sg1mdio,
> > - bmcuart0b, r1err, r1md, r1oen, r2oen, rmii3, r3oen, smb3d,
> > - fanin0, fanin1, fanin2, fanin3, fanin4, fanin5, fanin6,
> > - fanin7, fanin8, fanin9, fanin10, fanin11, fanin12, fanin13,
> > - fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, r2, r2err, r2md,
> > - r3rxer, ga20kbc, smb5d, lpc, espi, rg2, ddr, i3c0, i3c1,
> > - i3c2, i3c3, i3c4, i3c5, smb0, smb1, smb2, smb2c, smb2b,
> > - smb1c, smb1b, smb8, smb9, smb10, smb11, sd1, sd1pwr, pwm4,
> > - pwm5, pwm6, pwm7, pwm8, pwm9, pwm10, pwm11, mmc8, mmc, mmcwp,
> > - mmccd, mmcrst, clkout, serirq, scipme, smi, smb6, smb7, spi1,
> > - faninx, r1, spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3,
> > - nprd_smi, smb0b, smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1
> > - wdog2, smb12, smb13, spix, spixcs1, clkreq, hgpio0, hgpio1,
> > - hgpio2, hgpio3, hgpio4, hgpio5, hgpio6, hgpio7 ]
> > + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, smb5b,
> > + smb5c, lkgpo0, pspi, jm1, jm2, smb4b, smb4c, smb15, smb16,
> > + smb17, smb18, smb19, smb20, smb21, smb22, smb23, smb23b, smb4d,
> > + smb14, smb5, smb4, smb3, spi0cs1, spi0cs2, spi0cs3, spi1cs0,
> > + spi1cs1, spi1cs2, spi1cs3, spi1cs23, smb3c, smb3b, bmcuart0a,
> > + uart1, jtag2, bmcuart1, uart2, sg1mdio, bmcuart0b, r1err, r1md,
> > + r1oen, r2oen, rmii3, r3oen, smb3d, fanin0, fanin1, fanin2,
> > + fanin3, fanin4, fanin5, fanin6, fanin7, fanin8, fanin9, fanin10,
> > + fanin11, fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2,
> > + pwm3, r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg2,
> > + ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, smb0, smb1, smb2,
> > + smb2c, smb2b, smb1c, smb1b, smb8, smb9, smb10, smb11, sd1,
> > + sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, pwm9, pwm10, pwm11,
> > + mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, serirq, scipme, smi,
> > + smb6, smb6b, smb6c, smb6d, smb7, smb7b, smb7c, smb7d, spi1,
> > + faninx, r1, spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi,
> > + smb0b, smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2,
> > + smb12, smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2,
> > + hgpio3, hgpio4, hgpio5, hgpio6, hgpio7, bu4, bu4b, bu5, bu5b,
> > + bu6, gpo187 ]
> Thanks for raising I have an error here, I will fixed it in the
> vanilla and the OpenBMC, I should remove and add enums.
> >
> > dependencies:
> > groups: [ function ]
> > @@ -149,7 +152,6 @@ patternProperties:
> > description:
> > Debouncing periods in microseconds, one period per interrupt
> > bank found in the controller
> > - $ref: /schemas/types.yaml#/definitions/uint32-array
> it removed in the newer version, we can keep it
The outcome I'm after is we have the least difference in the patches
for dev-6.6 to those upstream. If it's correct to remove it, and that
is what you've done in the upstream patches, then we should remove it
in the dev-6.6 patches as well.
> > minItems: 1
> > maxItems: 4
> >
> > @@ -157,7 +159,6 @@ patternProperties:
> > description: |
> > 0: Low rate
> > 1: High rate
> > - $ref: /schemas/types.yaml#/definitions/uint32
> it removed in the newer version, we can keep it
> > enum: [0, 1]
> >
> > drive-strength:
> > diff --git a/drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c b/drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c
> > index 607960fdbc40..471f644c5eef 100644
> > --- a/drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c
> > +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c
> > @@ -173,7 +173,7 @@ static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> > struct npcm8xx_gpio *bank = gpiochip_get_data(chip);
> > int ret;
> >
> > - ret = pinctrl_gpio_direction_input(offset + chip->base);
> > + ret = pinctrl_gpio_direction_input(chip, offset);
> Modify in newer version, will revert the change in V3. since kernel
> 6.6 doesn't support it
Just to clarify, the dev-6.6 patches have the "-" content, and the
upstream patches have the "+" content. Perhaps how I generated the diff
is the confusing bit here. If you need me to unpack that a bit more
please ask, it's important that we both have the same understanding of
what's going on :D
Andrew
More information about the openbmc
mailing list