Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200721103634.2939493-1-abanoubsameh@protonmail.com> <20200721103634.2939493-2-abanoubsameh@protonmail.com>
In-Reply-To: <20200721103634.2939493-2-abanoubsameh@protonmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 21 Jul 2020 13:55:39 +0300
Message-ID: <CAHp75VehVVJ+1Lvo2EAGpp-e7wAG6qnCUmkKQ6Bd-CXx0VfBtw@mail.gmail.com>
Subject: Re: [PATCH] gpio: fixed some coding style issues in gpiolib files
To:     Abanoub Sameh <abanoubsameh8@gmail.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Abanoub Sameh <abanoubsameh@protonmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks, my comments below.


On Tue, Jul 21, 2020 at 1:41 PM Abanoub Sameh <abanoubsameh8@gmail.com> wrote:

Commit message is missed on why you did all these changes.

...

>         if (pin <= 255) {
>                 char ev_name[5];
> +
>                 sprintf(ev_name, "_%c%02hhX",
>                         agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
>                         pin);

Should be a separate patch.

...

>                 dev_err(chip->parent,
> -                       "Failed to install GPIO OpRegion handler\n");
> +                       "Failed to install GPIO OpRegion handler\n");
Make it simple in one line.

Also a separate patch.

...

> @@ -316,9 +316,8 @@ struct gpio_desc *gpiod_get_from_of_node(struct device_node *node,
>         desc = of_get_named_gpiod_flags(node, propname,
>                                         index, &flags);
>
> -       if (!desc || IS_ERR(desc)) {
> +       if (!desc || IS_ERR(desc))
>                 return desc;
> -       }

Also a separate patch.

...

> @@ -974,6 +974,7 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
>         if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
>             && le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE) {
>                 int level = gpiod_get_value_cansleep(le->desc);
> +
>                 if (level)
>                         /* Emit low-to-high event */
>                         ge.id = GPIOEVENT_EVENT_RISING_EDGE;

This can be unified with a patch against gpiolib-acpi.

...

> -       if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
> +       if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2)
>                 return irq_domain_translate_twocell(d, fwspec, hwirq, type);
> -       }

Unify with the patch above removing {}.

>         /* If a parent irqdomain is provided, let's build a hierarchy */
>         if (gpiochip_hierarchy_is_hierarchical(gc)) {
>                 int ret = gpiochip_hierarchy_add_domain(gc);
> +
>                 if (ret)
>                         return ret;

Similar as above.

And so on...

-- 
With Best Regards,
Andy Shevchenko