linux-kernel - Re: [PATCH v1 2/2] pinctrl: Add Intel Thunder Bay pinctrl 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdanKspORKBa2ETRvQyBkzgCssca-fFE+QcybZY=GYmMAQ@mail.gmail.com>
Date:   Fri, 26 Nov 2021 01:33:31 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     lakshmi.sowjanya.d@...el.com
Cc:     linux-gpio@...r.kernel.org, bgolaszewski@...libre.com,
        linux-kernel@...r.kernel.org, andriy.shevchenko@...ux.intel.com,
        tamal.saha@...el.com, pandith.n@...el.com,
        kenchappa.demakkanavar@...el.com
Subject: Re: [PATCH v1 2/2] pinctrl: Add Intel Thunder Bay pinctrl driver

Hi Lakshmi,

thanks for your patch! Interesting chip! This is looking very good.

I bet Andy will also give you some attention to details unless you
had some already internally at Intel.

On Tue, Nov 23, 2021 at 4:52 PM <lakshmi.sowjanya.d@...el.com> wrote:

This caught my eye:

> +#define THB_BOARD_SPECIFIC_GPIO_REQUIREMENTS_HANDLE    (1u)

We don't do this kind of conditionals, either it is there or not.
Drop this define since it is always 1

> +#if (THB_BOARD_SPECIFIC_GPIO_REQUIREMENTS_HANDLE)
> +
> +static u32 thb_gpio_board_requirements_handle(struct gpio_chip *chip)
> +{
> +       u32 offset, reg;
> +
> +       /* 0x43 = register Offset for gpio_power_int_setup/4u */
> +       offset = 0x43;
> +       reg = thb_gpio_read_reg(chip, offset);
> +
> +       /* Keeping all power interrupts to Level-High triggered as suggested by HW team */
> +       reg |= 0x1E;
> +
> +       return thb_gpio_write_reg(chip, offset, reg);
> +}

This looks like something that can just be inlined into probe(), some HW
set-up?

> +       struct gpio_chip *chip = &tpc->chip;
(..)
> +       chip->get               = thunderbay_gpio_get_value;
> +       chip->set               = thunderbay_gpio_set_value;

It should be trivial to also implement

.set_config = gpiochip_generic_config

just like the other Intel drivers. This is great because it will make
things like the generic bit-banged GPIO I2C bus use the open drain
hardware support from the chip. (Etc)

> +       /* Register pin mapping between GPIO and PinControl */
> +       ret = gpiochip_add_pin_range(chip, dev_name(tpc->dev), 0, 0, chip->ngpio);
> +       if (ret) {
> +               dev_err(tpc->dev, "Failed to add gpiochip pin range\n");
> +               return ret;
> +       }

It's usually better to put these ranges into the device tree. The
gpiolib core will handle it. See gpio-ranges in
Documentation/devicetree/bindings/gpio/gpio.txt

> +#if (THB_BOARD_SPECIFIC_GPIO_REQUIREMENTS_HANDLE)
> +       /* function to handle THB board specific requirements */
> +       ret = thb_gpio_board_requirements_handle(chip);
> +#endif

Just inline that function's code in here.

Yours,
Linus Walleij