Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <cover.1582647809.git.Asmaa@mellanox.com> <7ef84476a00e8771cf1edf5745c378273b760f5d.1582647809.git.Asmaa@mellanox.com>
 <DB6PR0501MB27121431EA6DCCB476B73F1DDAED0@DB6PR0501MB2712.eurprd05.prod.outlook.com>
In-Reply-To: <DB6PR0501MB27121431EA6DCCB476B73F1DDAED0@DB6PR0501MB2712.eurprd05.prod.outlook.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Sat, 29 Feb 2020 00:14:00 +0100
Message-ID: <CACRpkdZhs=ZYkcYb5bNqK_ayWEVk9=J0w--ELW-vcvoSvG9cxg@mail.gmail.com>
Subject: Re: [PATCH v2 1/1] gpio: add driver for Mellanox BlueField 2 GPIO controller
To:     Asmaa Mnebhi <Asmaa@mellanox.com>
Cc:     "bgolaszewski@baylibre.com" <bgolaszewski@baylibre.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Feb 25, 2020 at 5:42 PM Asmaa Mnebhi <Asmaa@mellanox.com> wrote:

> I have addressed your comments and tested the code. Please note
>  that the YU_ARM_GPIO_LOCK is a shared resource between the 3
>  gpio blocks instances.
> So now that we have split up the gpio_chip into 3 instances, we need to share
>  that LOCK resource accordingly. I have created a global variable for that purpose.

Fair enough, all looks so much nicer now :)

This:

+       /*
+        * Although the arm_gpio_lock was set in the probe function,
+        * check again it is still enabled to be able to write to the
+        * ModeX registers.
+        */
+       spin_lock(&gs->gc.bgpio_lock);
+       ret = mlxbf2_gpio_lock_acquire();
+       if (ret < 0) {
+               spin_unlock(&gs->gc.bgpio_lock);
+               return ret;
+       }

Is open-coded in two places, please create a helper function for this
or just merge it into mlbx2_gpio_lock_acquire() since it is done all the
time when that is called.

> Also note, that although it is not supported in this driver at the moment, some
> gpio interrupt registers will be similar to that LOCK i.e. they are shared among
> the  3 gpio block instances.

It's a good start like this.
The robot complains about devm_ioremap_nocache
which is now deleted, use devm_ioremap() simply, they are
all write-through.

Yours,
Linus Walleij