Re: [PATCH v2 2/5] gpio: Congatec Board Controller gpio driver

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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