Messages in this thread | | | From | Linus Walleij <> | Date | Fri, 26 Nov 2021 01:33:31 +0100 | 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@intel.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
| |