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] [day] [month] [year] [list]

Message-ID: <DM6PR11MB3660B018DC1826415CBD23F7C4669@DM6PR11MB3660.namprd11.prod.outlook.com>
Date:   Mon, 29 Nov 2021 15:22:42 +0000
From:   "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@...el.com>
To:     Linus Walleij <linus.walleij@...aro.org>
CC:     "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "Saha, Tamal" <tamal.saha@...el.com>,
        "N, Pandith" <pandith.n@...el.com>,
        "Demakkanavar, Kenchappa" <kenchappa.demakkanavar@...el.com>
Subject: RE: [PATCH v1 2/2] pinctrl: Add Intel Thunder Bay pinctrl driver

Thanks Linus!

>-----Original Message-----
>From: Linus Walleij <linus.walleij@...aro.org>
>Sent: Friday, November 26, 2021 6:04 AM
>To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@...el.com>
>Cc: linux-gpio@...r.kernel.org; bgolaszewski@...libre.com; linux-
>kernel@...r.kernel.org; andriy.shevchenko@...ux.intel.com; Saha, Tamal
><tamal.saha@...el.com>; N, Pandith <pandith.n@...el.com>; Demakkanavar,
>Kenchappa <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?
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