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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACRpkdbtwF-k3fHFvpXD+m8KN2XCt56X=iOXbideds0v+LWqWw@mail.gmail.com>
Date: Tue, 17 Sep 2024 21:25:56 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Lee Jones <lee@...nel.org>, Bartosz Golaszewski <brgl@...ev.pl>, Andi Shyti <andi.shyti@...nel.org>, 
	Wim Van Sebroeck <wim@...ux-watchdog.org>, Guenter Roeck <linux@...ck-us.net>, 
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org, 
	linux-i2c@...r.kernel.org, linux-watchdog@...r.kernel.org, 
	thomas.petazzoni@...tlin.com, blake.vermeer@...sight.com
Subject: Re: [PATCH v2 2/5] gpio: Congatec Board Controller gpio driver

Hi Thomas,

thanks for your patch!

Some comments:

On Tue, Sep 17, 2024 at 7:00 PM Thomas Richard
<thomas.richard@...tlin.com> wrote:

> Add gpio support for the Congatec Board Controller.
>
> Signed-off-by: Thomas Richard <thomas.richard@...tlin.com>

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@...aro.org>

Yours,
Linus Walleij