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

[D, Lakshmi Sowjanya]

Thanks Linus!

>-----Original Message-----
>From: Linus Walleij <linus.walleij@xxxxxxxxxx>
>Sent: Friday, November 26, 2021 6:04 AM
>To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@xxxxxxxxx>
>Cc: linux-gpio@xxxxxxxxxxxxxxx; bgolaszewski@xxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx; Saha, Tamal
><tamal.saha@xxxxxxxxx>; N, Pandith <pandith.n@xxxxxxxxx>; Demakkanavar,
>Kenchappa <kenchappa.demakkanavar@xxxxxxxxx>
>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@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?
Yes it's hw setup.
This will be removed in next version of the patchset. These changes are accommodated in the BIOS, so not required in kernel code.
>
>> +       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)

Will be added in the next version of patchset.
>
>> +       /* 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

Thanks,
This will be taken care in next version.

>
>> +#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.
Will be removed.
>
>Yours,
>Linus Walleij

Regards,
Sowjanya