Re: [PATCH v1 2/2] pinctrl: Add Intel Thunder Bay pinctrl driver

[Linus Walleij]

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@xxxxxxxxx> 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