Hi Thomas,
thanks for your patch!
Some comments:
On Tue, Sep 17, 2024 at 7:00 PM Thomas Richard
<thomas.richard@xxxxxxxxxxx> wrote:
> Add gpio support for the Congatec Board Controller.
>
> Signed-off-by: Thomas Richard <thomas.richard@xxxxxxxxxxx>
That's a bit terse commit message. At least mention that it sits under
the MFD.
> @@ -233,6 +233,16 @@ config GPIO_CADENCE
> help
> Say yes here to enable support for Cadence GPIO controller.
>
> +config GPIO_CGBC
> + tristate "Congatec Board Controller GPIO support"
> + depends on MFD_CGBC
> + help
> + Select this option to enable GPIO support for the Congatec Board
> + Controller.
> +
> + This driver can also be built as a module. If so, the module will be
> + called gpio-cgbc.
> +
> config GPIO_CLPS711X
This is in the middle of the memory-mapped GPIO drivers.
This is not a memory-mapped driver.
Move it down in the menu to the submenu for MFD GPIO
drivers please.
> +static void __cgbc_gpio_set(struct gpio_chip *chip,
> + unsigned int offset, int value)
> +{
> + struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
> + struct cgbc_device_data *cgbc = gpio->cgbc;
> + u8 val;
> + int ret;
> +
> + ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_GET, (offset > 7) ? 1 : 0, 0, &val);
> + if (ret)
> + return;
> +
> + if (value)
> + val |= BIT(offset % 8);
> + else
> + val &= ~((u8)BIT(offset % 8));
Is that cast really needed? (If you tried without and it
cause compilation problems, I believe you, if someone
smarter than me said it should be there, ignore me.)
> + if (direction == GPIO_LINE_DIRECTION_IN)
> + val &= ~((u8)BIT(offset % 8));
Dito.
Apart from this it looks very nice, so with the above
addressed:
Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
Yours,
Linus Walleij
