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

From: Linus Walleij
Date: Tue Sep 17 2024 - 15:26:23 EST


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