lkml.org 
[lkml]   [2021]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v1 2/2] pinctrl: Add Intel Thunder Bay pinctrl driver
Date
Thanks Linus!

>-----Original Message-----
>From: Linus Walleij <linus.walleij@linaro.org>
>Sent: Friday, November 26, 2021 6:04 AM
>To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@intel.com>
>Cc: linux-gpio@vger.kernel.org; bgolaszewski@baylibre.com; linux-
>kernel@vger.kernel.org; andriy.shevchenko@linux.intel.com; Saha, Tamal
><tamal.saha@intel.com>; N, Pandith <pandith.n@intel.com>; Demakkanavar,
>Kenchappa <kenchappa.demakkanavar@intel.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@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?
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
\
 
 \ /
  Last update: 2021-11-29 16:25    [W:0.200 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site