Hi Torsten,
On 2026-09-25T10:54:32, Torsten Duwe <[email protected]> wrote:
> gpio: add support for RPi5 RP1 chip GPIO
>
> RP1 GPIO driver, derived from Linux drivers/pinctrl/pinctrl-rp1.c;
> mainly stripped the pins' alternate function handling and fine-grained
> driving current controls.
>
> Signed-off-by: Torsten Duwe <[email protected]>
> Co-authored-by: Oleksii Moisieiev <[email protected]>
>
> configs/rpi_arm64_defconfig | 1 +
> drivers/gpio/Kconfig | 7 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/rp1_gpio.c | 374 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 383 insertions(+)
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> @@ -413,6 +413,13 @@ config RCAR_GPIO
> +config RP1_GPIO
> + bool "Raspberry PR1 GPIO driver"
> + help
> + This driver supports the GPIO banks on Raspberry RP1 chip.
> + Raspberry PI5 has an external chip RP1 installed, which
> + provides it's own GPIO controller.
> +
Two typos in the help text: 'Raspberry PR1' should be 'Raspberry RP1',
and 'it's own' should be 'its own'. Please also add 'depends on
DM_GPIO' as the neighbouring GPIO entries do - the driver will not
build without it.
> diff --git a/drivers/gpio/rp1_gpio.c b/drivers/gpio/rp1_gpio.c
> @@ -0,0 +1,374 @@
> +const struct rp1_iobank_desc rp1_iobanks[RP1_NUM_BANKS] = {
> + /* gpio inte ints rio pads */
> + { 0, 28, 0x0000, 0x011c, 0x0124, 0x0000, 0x0004 },
> + { 28, 6, 0x4000, 0x411c, 0x4124, 0x4000, 0x4004 },
> + { 34, 20, 0x8000, 0x811c, 0x8124, 0x8000, 0x8004 },
> +};
Please make this 'static const' - it is only used within this translation unit.
> diff --git a/drivers/gpio/rp1_gpio.c b/drivers/gpio/rp1_gpio.c
> @@ -0,0 +1,374 @@
> +static int rp1_gpio_set(struct udevice *dev, unsigned int offset, int value)
> +{
> + struct rp1_pin_info *pin = rp1_get_pin(dev, offset);
> +
> + if (pin)
> + rp1_set_value(pin, value);
> +
> + return 0;
> +}
Silently returning 0 on a bad offset is inconsistent with the
direction_input/direction_output ops, which return -EINVAL. Please
return -EINVAL here too.
> diff --git a/drivers/gpio/rp1_gpio.c b/drivers/gpio/rp1_gpio.c
> @@ -0,0 +1,374 @@
> +static int rp1_gpio_get_direction(struct udevice *dev, unsigned int offset)
> +{
> + struct rp1_pin_info *pin = rp1_get_pin(dev, offset);
> + u32 fsel;
> +
> + if (!pin)
> + return -EINVAL;
> +
> + fsel = rp1_get_fsel(pin);
> + if (fsel != RP1_FSEL_GPIO)
> + return -EINVAL;
> +
> + return (rp1_get_dir(pin) == RP1_DIR_OUTPUT) ?
> + GPIOF_OUTPUT :
> + GPIOF_INPUT;
> +}
This is wired up as .get_function, so it must return one of the
GPIOF_* values, not an errno. When fsel != RP1_FSEL_GPIO the pin is in
an alternate function - please return GPIOF_FUNC to match
intel_gpio/at91_gpio/stm32_gpio. Otherwise 'gpio status' will report a
stray error for every non-GPIO pin.
> diff --git a/drivers/gpio/rp1_gpio.c b/drivers/gpio/rp1_gpio.c
> @@ -0,0 +1,374 @@
> +struct rp1_pin_info {
> + u8 num;
> + u8 bank;
> + u8 offset;
> + u8 fsel;
> +
> + void __iomem *gpio;
> + void __iomem *rio;
> + void __iomem *inte;
> + void __iomem *ints;
> + void __iomem *pad;
> +};
num, fsel, inte and ints are populated in probe but never read - the
IRQ paths from the Linux driver were dropped. How about dropping the
dead fields?
Regards,
Simon
