linux-kernel - Re: [PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4ba11985-c71e-e526-8478-8bfe0e208429@emutex.com>
Date:   Wed, 31 Oct 2018 19:55:39 +0000
From:   Dan O'Donovan <dan.odonovan@...tex.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Dan O'Donovan <dan.odonovan@...tex.com>
CC:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        "Krogerus, Heikki" <heikki.krogerus@...ux.intel.com>,
        Lee Jones <lee.jones@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Pavel Machek <pavel@....cz>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        Carlos Iglesias <carlos.iglesias@...tex.com>,
        Javier Arteaga <javier@...tex.com>
Subject: Re: [PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

Thanks for your review feedback, Andy!  I'll send a v3 shortly with those changes you suggested.  I've added some comments inline below.

On 10/20/2018 12:40 PM, Andy Shevchenko wrote:
> On Fri, Oct 19, 2018 at 8:24 PM Dan O'Donovan <dan@...tex.com> wrote:
>
>> +static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
>> +{
>> +       return 0;
>> +}
>> +
>> +static int upboard_get_function_groups(struct pinctrl_dev *pctldev,
>> +                                      unsigned int selector,
>> +                                      const char * const **groups,
>> +                                      unsigned int *num_groups)
>> +{
>> +       *groups = NULL;
>> +       *num_groups = 0;
>> +       return 0;
>> +}
>> +
>> +static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
>> +                                            unsigned int selector)
>> +{
>> +       return NULL;
>> +}
>> +
>> +static int upboard_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
>> +                          unsigned int group)
>> +{
>> +       return 0;
>> +}
> Hmm... Do you need those stubs? Same Q for other stubs in the file.
It looks like they're required by pinctrl core, which returns an error if they're not provided.

>> +static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
>> +                                      struct pinctrl_gpio_range *range,
>> +                                      unsigned int pin)
>> +{
>> +       const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
>> +       const struct upboard_pin *p;
>> +       int ret;
>> +
>> +       if (!pd)
>> +               return -EINVAL;
> When it possible to happen?
> Same Q for the rest same excerpts.
Agreed, it shouldn't be possible.  I will remove these checks.

>> +
>> +       if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
>> +               return ERR_PTR(-ENODEV);
> offset >= ?
> Is it even possible?
Agreed, it shouldn't be possible.  I will remove these checks.

>> +static int upboard_pinctrl_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct pinctrl_desc *pctldesc;
>> +       struct upboard_pinctrl *pctrl;
>> +       struct upboard_pin *pins;
>> +       struct acpi_device *adev;
>> +       struct regmap *regmap;
>> +       unsigned int i;
>> +       int ret;
>> +       adev = ACPI_COMPANION(dev);
>> +       if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01"))
>> +               return -ENODEV;
> Same Q as per LED driver.
I agree.  I will remove this check, both here and in the LED driver.

>> +               if (pd->drv_data) {
>> +                       struct reg_field *field = pd->drv_data;
>> +
>> +                       pin->func_en = devm_regmap_field_alloc(dev, regmap,
>> +                                                              *field);
>> +                       if (IS_ERR(pin->func_en))
>> +                               return PTR_ERR(pin->func_en);
>> +               }
>> +
>> +               pin->gpio_en = upboard_field_alloc(dev, regmap,
>> +                                                  UPBOARD_REG_GPIO_EN0, i);
>> +               if (IS_ERR(pin->gpio_en))
>> +                       return PTR_ERR(pin->gpio_en);
>> +
>> +               pin->gpio_dir = upboard_field_alloc(dev, regmap,
>> +                                                   UPBOARD_REG_GPIO_DIR0, i);
>> +               if (IS_ERR(pin->gpio_dir))
>> +                       return PTR_ERR(pin->gpio_dir);
>> +
>> +               ((struct pinctrl_pin_desc *)pd)->drv_data = pin;
> I'm not sure I understand the purpose of this casting.
When the pd pointer is retrieved from struct pinctrl_desc, it has a const constraint.  The purpose of the cast is to bypass that constraint for this use case, because this code is allocating and setting drv_data dynamically here at run-time rather than at compile-time.