Re: [PATCH v2 1/2] Support NVIDIA BlueField-3 GPIO controller

Re: [PATCH v2 1/2] Support NVIDIA BlueField-3 GPIO controller

[Linus Walleij]

Hi Asmaa,

thanks for your patch!

On Wed, Feb 8, 2023 at 7:57 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> This patch adds support for the BlueField-3 SoC GPIO driver
> which allows:
> - setting certain GPIOs as interrupts from other dependent drivers
> - ability to manipulate certain GPIO pins via libgpiod tools for instance
>
> BlueField-3 has 56 GPIOs but the user is only allowed to change some
> of them into GPIO mode. Use valid_mask to make it impossible to alter
> the rest of the GPIOs.
>
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>

(...)

> +struct mlxbf3_gpio_context {
> +       struct gpio_chip gc;
> +       struct irq_chip irq_chip;
> +
> +       /* YU GPIO block address */
> +       void __iomem *gpio_io;
> +
> +       /* YU GPIO cause block address */
> +       void __iomem *gpio_cause_io;

I especially like that you use the word "cause" (as in cause-and-effect)
over the imprecise and overused word "reason" (brrrr)

> +static int mlxbf3_gpio_direction_input(struct gpio_chip *chip,
> +                                      unsigned int offset)
> +{
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR);
> +
> +       raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}
> +
> +static int mlxbf3_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned int offset,
> +                                       int value)
> +{
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_SET);
> +
> +       if (value)
> +               writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_SET);
> +       else
> +               writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_CLEAR);
> +
> +       raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}

GPIO_GENERIC / bgpio should be able to handle these too.

> +       ret = bgpio_init(gc, dev, 4,
> +                       gs->gpio_io + MLXBF_GPIO_READ_DATA_IN,
> +                       gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_SET,
> +                       gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_CLEAR,
> +                       NULL, NULL, 0);

Instead of NULL, NULL, 0

use

gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_SET,
gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR,
0);

The generic library will make sure to also set the output value when
changing direction to output.

Yours,
Linus Walleij

Re: [PATCH v2 2/2] Support NVIDIA BlueField-3 pinctrl driver

[Linus Walleij]

Hi Asmaa,

thanks for your patch!

(Adding my colleague Niyas so he can have a look at this quite typical ACPI
embedded pinctrl driver.)

On Wed, Feb 8, 2023 at 7:57 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> This patch adds support to the BlueField-3 SoC pin controller.
> It allows muxing individual GPIOs to switch from the default
> hardware mode to software controlled mode.
>
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>

(...)
> +config PINCTRL_MLXBF
> +       tristate "NVIDIA BlueField-3 SoC Pinctrl driver"
> +       depends on (MELLANOX_PLATFORM && ARM64 && ACPI)
> +       select PINMUX
> +       select GPIOLIB
> +       select GPIOLIB_IRQCHIP

Since you're selecting these you could as well
select GPIO_MLXBF3
too.

> +#include <linux/acpi.h>

Oh ACPI, I hope Andy has time to look at this.

> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>

Drop this include, this is a legacy API, also this is not a GPIO driver.

Go through the list and see if there are some more unnecessary headers.

> +#include <linux/pinctrl/machine.h>

Why?

> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +
> +#include "core.h"

Do you need this? (I'm uncertain myself, so try without.)

Yours,
Linus Walleij

Re: [PATCH v2 0/2] Add NVIDIA BlueField-3 GPIO driver and pin controller

[Andy Shevchenko]

On Wed, Feb 8, 2023 at 8:57 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> This series of patches creates a pin controller driver and GPIO
> driver for NVIDIA BlueField-3 SoC.
> The first patch creates a GPIO driver for handling interrupts and
> allowing the change of direction and value of a GPIO if needed.
> The second patch creates a pin controller driver for allowing a
> select number of GPIO pins to be manipulated from userspace or
> the kernel.

Please, make sure your patches are available on lore.kernel.org. I
can't find this at all.

>  drivers/gpio/gpio-mlxbf3.c      | 312 ++++++++++++++++++++++++++++

I'm wondering if you have anything in common  with gpio-mlxbf and/or
gpio-mlxbf2 drivers. If so, can you split a common library code?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 0/2] Add NVIDIA BlueField-3 GPIO driver and pin controller

[Asmaa Mnebhi]

Hi Andy,

Thank you for your response. I was just going to contact you regarding this. I successfully sent these patches to you, Linus and Bartosz while they failed to get delivered to the "*.kernel.org" emails below:
linux-gpio@vger.kernel.org;
linux-kernel@vger.kernel.org;
linux-acpi@vger.kernel.org

Have these emails changed?

These patches don't have much in common with gpio-mlxbf or gpio-mlxbf2 because the hardware registers and logic have changed across generations. The only similar code between gpio-mlxbf2.c and gpio-mlxbf3.c is the irq handling.

Thanks.
Asmaa

-----Original Message-----
From: Andy Shevchenko <andy.shevchenko@gmail.com> 
Sent: Thursday, February 9, 2023 5:28 PM
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: linus.walleij@linaro.org; 

Re: [PATCH v2 0/2] Add NVIDIA BlueField-3 GPIO driver and pin controller

[Andy Shevchenko]

On Fri, Feb 10, 2023 at 12:48 AM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

First of all, do not top-post!

> Thank you for your response. I was just going to contact you regarding this. I successfully sent these patches to you, Linus and Bartosz while they failed to get delivered to the "*.kernel.org" emails below:
> linux-gpio@vger.kernel.org;
> linux-kernel@vger.kernel.org;
> linux-acpi@vger.kernel.org
>
> Have these emails changed?

No, but you need to work with your company's IT to understand what's
going on. Your mails are only available privately and not in the
archives on lore.kernel.org. This is an issue and I'm not going to
comment on something that was not in public.

> These patches don't have much in common with gpio-mlxbf or gpio-mlxbf2 because the hardware registers and logic have changed across generations. The only similar code between gpio-mlxbf2.c and gpio-mlxbf3.c is the irq handling.

I see, don't forget to put it in the cover letter for the next version
(you will send it when you will be sure that emails are going to the
kernel.org archives).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] Add NVIDIA BlueField-3 GPIO driver and pin controller

[Andy Shevchenko]

On Fri, Feb 10, 2023 at 12:15:15PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 10, 2023 at 12:48 AM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
> 
> First of all, do not top-post!
> 
> > Thank you for your response. I was just going to contact you regarding this. I successfully sent these patches to you, Linus and Bartosz while they failed to get delivered to the "*.kernel.org" emails below:
> > linux-gpio@vger.kernel.org;
> > linux-kernel@vger.kernel.org;
> > linux-acpi@vger.kernel.org
> >
> > Have these emails changed?
> 
> No, but you need to work with your company's IT to understand what's
> going on. Your mails are only available privately and not in the
> archives on lore.kernel.org. This is an issue and I'm not going to
> comment on something that was not in public.

To illustrate what I'm talking about:

https://lore.kernel.org/linux-gpio/CACRpkdZ_sPCa+Q6MWUKj1ytCe5AxTp--bMmbRTUfsNO0kZTMpQ@mail.gmail.com/T/#t


> > These patches don't have much in common with gpio-mlxbf or gpio-mlxbf2 because the hardware registers and logic have changed across generations. The only similar code between gpio-mlxbf2.c and gpio-mlxbf3.c is the irq handling.
> 
> I see, don't forget to put it in the cover letter for the next version
> (you will send it when you will be sure that emails are going to the
> kernel.org archives).

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 0/2] Add NVIDIA BlueField-3 GPIO driver and pin controller

[Asmaa Mnebhi]

To illustrate what I'm talking about:

https://lore.kernel.org/linux-gpio/CACRpkdZ_sPCa+Q6MWUKj1ytCe5AxTp--bMmbRTUfsNO0kZTMpQ@mail.gmail.com/T/#t

Thanks for your help! Here is the v3 patch: 
https://lore.kernel.org/lkml/cover.1676042188.git.asmaa@nvidia.com/

Best,
Asmaa