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

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

[Asmaa Mnebhi]

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.

The BlueField-3 SoC gpio-mlxbf3.c driver handles different hardware registers
and logic that from gpio-mlxbf.c and gpio-mlxbf2.c.
For that reason, we have separate drivers for each generation.

Changes from v2->v3:
Addressed the following comments from maintainers:
- bgpio_init can handle direction_input and direction_output
- Update pinctrl Kconfig to select GPIO_MLXBF3
- remove unnecessary #includes from gpio-mlxbf3.c and pinctrl-mlxbf.c

Asmaa Mnebhi (2):
  Support NVIDIA BlueField-3 GPIO controller
  Support NVIDIA BlueField-3 pinctrl driver

 drivers/gpio/Kconfig            |   7 +
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-mlxbf3.c      | 262 ++++++++++++++++++++++++
 drivers/pinctrl/Kconfig         |  10 +
 drivers/pinctrl/Makefile        |   1 +
 drivers/pinctrl/pinctrl-mlxbf.c | 341 ++++++++++++++++++++++++++++++++
 6 files changed, 622 insertions(+)
 create mode 100644 drivers/gpio/gpio-mlxbf3.c
 create mode 100644 drivers/pinctrl/pinctrl-mlxbf.c

-- 
2.30.1

[PATCH v3 1/2] Support NVIDIA BlueField-3 GPIO controller

[Asmaa Mnebhi]

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

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>
---
 drivers/gpio/Kconfig       |   7 +
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-mlxbf3.c | 262 +++++++++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+)
 create mode 100644 drivers/gpio/gpio-mlxbf3.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ec7cfd4f52b1..3d56a83db284 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1534,6 +1534,13 @@ config GPIO_MLXBF2
 	help
 	  Say Y here if you want GPIO support on Mellanox BlueField 2 SoC.
 
+config GPIO_MLXBF3
+	tristate "Mellanox BlueField 3 SoC GPIO"
+	depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || (64BIT && COMPILE_TEST)
+	select GPIO_GENERIC
+	help
+	  Say Y here if you want GPIO support on Mellanox BlueField 3 SoC.
+
 config GPIO_ML_IOH
 	tristate "OKI SEMICONDUCTOR ML7213 IOH GPIO support"
 	depends on X86 || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 010587025fc8..76545ca31457 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_GPIO_MERRIFIELD)		+= gpio-merrifield.o
 obj-$(CONFIG_GPIO_ML_IOH)		+= gpio-ml-ioh.o
 obj-$(CONFIG_GPIO_MLXBF)		+= gpio-mlxbf.o
 obj-$(CONFIG_GPIO_MLXBF2)		+= gpio-mlxbf2.o
+obj-$(CONFIG_GPIO_MLXBF3)		+= gpio-mlxbf3.o
 obj-$(CONFIG_GPIO_MM_LANTIQ)		+= gpio-mm-lantiq.o
 obj-$(CONFIG_GPIO_MOCKUP)		+= gpio-mockup.o
 obj-$(CONFIG_GPIO_MOXTET)		+= gpio-moxtet.o
diff --git a/drivers/gpio/gpio-mlxbf3.c b/drivers/gpio/gpio-mlxbf3.c
new file mode 100644
index 000000000000..ddd27c316035
--- /dev/null
+++ b/drivers/gpio/gpio-mlxbf3.c
@@ -0,0 +1,262 @@
+// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause
+
+/*
+ * Copyright (C) 2022 NVIDIA CORPORATION & AFFILIATES
+ */
+
+#include <linux/acpi.h>
+#include <linux/gpio/driver.h>
+#include <linux/platform_device.h>
+
+/*
+ * There are 2 YU GPIO blocks:
+ * gpio[0]: HOST_GPIO0->HOST_GPIO31
+ * gpio[1]: HOST_GPIO32->HOST_GPIO55
+ */
+#define MLXBF3_GPIO_MAX_PINS_PER_BLOCK 32
+
+/*
+ * fw_gpio[x] block registers and their offset
+ */
+#define MLXBF_GPIO_FW_OUTPUT_ENABLE_SET	  0x04
+#define MLXBF_GPIO_FW_DATA_OUT_SET        0x08
+#define MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR 0x18
+#define MLXBF_GPIO_FW_DATA_OUT_CLEAR      0x1c
+#define MLXBF_GPIO_CAUSE_RISE_EN          0x28
+#define MLXBF_GPIO_CAUSE_FALL_EN          0x2c
+#define MLXBF_GPIO_READ_DATA_IN           0x30
+
+#define MLXBF_GPIO_CAUSE_OR_CAUSE_EVTEN0  0x00
+#define MLXBF_GPIO_CAUSE_OR_EVTEN0        0x14
+#define MLXBF_GPIO_CAUSE_OR_CLRCAUSE      0x18
+
+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;
+
+	/* Mask of valid gpios that can be accessed by software */
+	unsigned int valid_mask;
+};
+
+static void mlxbf3_gpio_irq_enable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	writel(BIT(offset), gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CLRCAUSE);
+
+	val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+	val |= BIT(offset);
+	writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+	raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+}
+
+static void mlxbf3_gpio_irq_disable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+	val &= ~BIT(offset);
+	writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+	raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+}
+
+static irqreturn_t mlxbf3_gpio_irq_handler(int irq, void *ptr)
+{
+	struct mlxbf3_gpio_context *gs = ptr;
+	struct gpio_chip *gc = &gs->gc;
+	unsigned long pending;
+	u32 level;
+
+	pending = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CAUSE_EVTEN0);
+	writel(pending, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CLRCAUSE);
+
+	for_each_set_bit(level, &pending, gc->ngpio)
+		generic_handle_irq(irq_find_mapping(gc->irq.domain, level));
+
+	return IRQ_RETVAL(pending);
+}
+
+static int
+mlxbf3_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	bool fall = false;
+	bool rise = false;
+	u32 val;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_BOTH:
+		fall = true;
+		rise = true;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		rise = true;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		fall = true;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	if (fall) {
+		val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
+		val |= BIT(offset);
+		writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
+	}
+
+	if (rise) {
+		val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
+		val |= BIT(offset);
+		writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
+	}
+	raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+
+	return 0;
+}
+
+static int mlxbf3_gpio_init_valid_mask(struct gpio_chip *gc,
+				       unsigned long *valid_mask,
+				       unsigned int ngpios)
+{
+	struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
+
+	*valid_mask = gs->valid_mask;
+
+	return 0;
+}
+
+static int
+mlxbf3_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mlxbf3_gpio_context *gs;
+	unsigned int npins, valid_mask;
+	struct gpio_irq_chip *girq;
+	struct gpio_chip *gc;
+	struct resource *res;
+	const char *name;
+	int ret, irq;
+
+	name = dev_name(dev);
+
+	gs = devm_kzalloc(dev, sizeof(*gs), GFP_KERNEL);
+	if (!gs)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	/* Resource shared with pinctrl driver */
+	gs->gpio_io = devm_ioremap(dev, res->start, resource_size(res));
+	if (!gs->gpio_io)
+		return -ENOMEM;
+
+	/* YU GPIO block address */
+	gs->gpio_cause_io = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(gs->gpio_cause_io))
+		return PTR_ERR(gs->gpio_cause_io);
+
+	if (device_property_read_u32(dev, "npins", &npins))
+		npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
+
+	if (device_property_read_u32(dev, "valid_mask", &valid_mask))
+		valid_mask = 0x0;
+
+	gs->valid_mask = valid_mask;
+
+	gc = &gs->gc;
+
+	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,
+			gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_SET,
+			gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR, 0);
+
+	gc->request = gpiochip_generic_request;
+	gc->free = gpiochip_generic_free;
+	gc->init_valid_mask = mlxbf3_gpio_init_valid_mask;
+
+	gc->ngpio = npins;
+	gc->owner = THIS_MODULE;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0) {
+		gs->irq_chip.name = name;
+		gs->irq_chip.irq_set_type = mlxbf3_gpio_irq_set_type;
+		gs->irq_chip.irq_enable = mlxbf3_gpio_irq_enable;
+		gs->irq_chip.irq_disable = mlxbf3_gpio_irq_disable;
+
+		girq = &gs->gc.irq;
+		girq->chip = &gs->irq_chip;
+		girq->handler = handle_simple_irq;
+		girq->default_type = IRQ_TYPE_NONE;
+		/* This will let us handle the parent IRQ in the driver */
+		girq->num_parents = 0;
+		girq->parents = NULL;
+		girq->parent_handler = NULL;
+
+		/*
+		 * Directly request the irq here instead of passing
+		 * a flow-handler because the irq is shared.
+		 */
+		ret = devm_request_irq(dev, irq, mlxbf3_gpio_irq_handler,
+				       IRQF_SHARED, name, gs);
+		if (ret) {
+			dev_err(dev, "failed to request IRQ");
+			return ret;
+		}
+	}
+
+	platform_set_drvdata(pdev, gs);
+
+	ret = devm_gpiochip_add_data(dev, &gs->gc, gs);
+	if (ret) {
+		dev_err(dev, "Failed adding memory mapped gpiochip\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct acpi_device_id __maybe_unused mlxbf3_gpio_acpi_match[] = {
+	{ "MLNXBF33", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, mlxbf3_gpio_acpi_match);
+
+static struct platform_driver mlxbf3_gpio_driver = {
+	.driver = {
+		.name = "mlxbf3_gpio",
+		.acpi_match_table = mlxbf3_gpio_acpi_match,
+	},
+	.probe    = mlxbf3_gpio_probe,
+};
+
+module_platform_driver(mlxbf3_gpio_driver);
+
+MODULE_DESCRIPTION("NVIDIA BlueField-3 GPIO Driver");
+MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.30.1

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

[kernel test robot]

Hi Asmaa,

I love your patch! Yet something to improve:

[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on linusw-pinctrl/devel linusw-pinctrl/for-next linus/master v6.2-rc7 next-20230210]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Asmaa-Mnebhi/Support-NVIDIA-BlueField-3-GPIO-controller/20230210-234043
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/c1bf45a868edcd3df5263fa76a32b28e6c9ca3d1.1676042188.git.asmaa%40nvidia.com
patch subject: [PATCH v3 1/2] Support NVIDIA BlueField-3 GPIO controller
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20230211/202302111542.y6a7wZeb-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ea731fc5718b591e6c84ee33049e46b60882009d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Asmaa-Mnebhi/Support-NVIDIA-BlueField-3-GPIO-controller/20230210-234043
        git checkout ea731fc5718b591e6c84ee33049e46b60882009d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=loongarch olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=loongarch SHELL=/bin/bash drivers/gpio/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302111542.y6a7wZeb-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-mlxbf3.c: In function 'mlxbf3_gpio_probe':
>> drivers/gpio/gpio-mlxbf3.c:225:23: error: implicit declaration of function 'devm_request_irq'; did you mean 'can_request_irq'? [-Werror=implicit-function-declaration]
     225 |                 ret = devm_request_irq(dev, irq, mlxbf3_gpio_irq_handler,
         |                       ^~~~~~~~~~~~~~~~
         |                       can_request_irq
>> drivers/gpio/gpio-mlxbf3.c:226:40: error: 'IRQF_SHARED' undeclared (first use in this function); did you mean 'VM_SHARED'?
     226 |                                        IRQF_SHARED, name, gs);
         |                                        ^~~~~~~~~~~
         |                                        VM_SHARED
   drivers/gpio/gpio-mlxbf3.c:226:40: note: each undeclared identifier is reported only once for each function it appears in
   cc1: some warnings being treated as errors


vim +225 drivers/gpio/gpio-mlxbf3.c

   148	
   149	static int
   150	mlxbf3_gpio_probe(struct platform_device *pdev)
   151	{
   152		struct device *dev = &pdev->dev;
   153		struct mlxbf3_gpio_context *gs;
   154		unsigned int npins, valid_mask;
   155		struct gpio_irq_chip *girq;
   156		struct gpio_chip *gc;
   157		struct resource *res;
   158		const char *name;
   159		int ret, irq;
   160	
   161		name = dev_name(dev);
   162	
   163		gs = devm_kzalloc(dev, sizeof(*gs), GFP_KERNEL);
   164		if (!gs)
   165			return -ENOMEM;
   166	
   167		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   168		if (!res)
   169			return -ENODEV;
   170	
   171		/* Resource shared with pinctrl driver */
   172		gs->gpio_io = devm_ioremap(dev, res->start, resource_size(res));
   173		if (!gs->gpio_io)
   174			return -ENOMEM;
   175	
   176		/* YU GPIO block address */
   177		gs->gpio_cause_io = devm_platform_ioremap_resource(pdev, 1);
   178		if (IS_ERR(gs->gpio_cause_io))
   179			return PTR_ERR(gs->gpio_cause_io);
   180	
   181		if (device_property_read_u32(dev, "npins", &npins))
   182			npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
   183	
   184		if (device_property_read_u32(dev, "valid_mask", &valid_mask))
   185			valid_mask = 0x0;
   186	
   187		gs->valid_mask = valid_mask;
   188	
   189		gc = &gs->gc;
   190	
   191		ret = bgpio_init(gc, dev, 4,
   192				gs->gpio_io + MLXBF_GPIO_READ_DATA_IN,
   193				gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_SET,
   194				gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_CLEAR,
   195				gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_SET,
   196				gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR, 0);
   197	
   198		gc->request = gpiochip_generic_request;
   199		gc->free = gpiochip_generic_free;
   200		gc->init_valid_mask = mlxbf3_gpio_init_valid_mask;
   201	
   202		gc->ngpio = npins;
   203		gc->owner = THIS_MODULE;
   204	
   205		irq = platform_get_irq(pdev, 0);
   206		if (irq >= 0) {
   207			gs->irq_chip.name = name;
   208			gs->irq_chip.irq_set_type = mlxbf3_gpio_irq_set_type;
   209			gs->irq_chip.irq_enable = mlxbf3_gpio_irq_enable;
   210			gs->irq_chip.irq_disable = mlxbf3_gpio_irq_disable;
   211	
   212			girq = &gs->gc.irq;
   213			girq->chip = &gs->irq_chip;
   214			girq->handler = handle_simple_irq;
   215			girq->default_type = IRQ_TYPE_NONE;
   216			/* This will let us handle the parent IRQ in the driver */
   217			girq->num_parents = 0;
   218			girq->parents = NULL;
   219			girq->parent_handler = NULL;
   220	
   221			/*
   222			 * Directly request the irq here instead of passing
   223			 * a flow-handler because the irq is shared.
   224			 */
 > 225			ret = devm_request_irq(dev, irq, mlxbf3_gpio_irq_handler,
 > 226					       IRQF_SHARED, name, gs);
   227			if (ret) {
   228				dev_err(dev, "failed to request IRQ");
   229				return ret;
   230			}
   231		}
   232	
   233		platform_set_drvdata(pdev, gs);
   234	
   235		ret = devm_gpiochip_add_data(dev, &gs->gc, gs);
   236		if (ret) {
   237			dev_err(dev, "Failed adding memory mapped gpiochip\n");
   238			return ret;
   239		}
   240	
   241		return 0;
   242	}
   243	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Andy Shevchenko]

On Fri, Feb 10, 2023 at 10:39:40AM -0500, Asmaa Mnebhi wrote:
> This patch adds support for the BlueField-3 SoC GPIO driver

The Submitting Patches states that imperative speech should be used.

> which allows:
> - setting certain GPIOs as interrupts from other dependent drivers
> - ability to manipulate certain GPIO pins via libgpiod tools
> 
> 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.

...

> +	help
> +	  Say Y here if you want GPIO support on Mellanox BlueField 3 SoC.

Have you run checkpatch? Nowadays 3+ lines of help is recommended.
I would suggest to add a standard phrase about module name in case
the module build is chosen.

...

> +// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause

> +

Redundant blank line

> +/*
> + * Copyright (C) 2022 NVIDIA CORPORATION & AFFILIATES
> + */

This can be on one line.

...

> +#include <linux/acpi.h>

No user of this header.

> +#include <linux/gpio/driver.h>
> +#include <linux/platform_device.h>

Approx dozen of header inclusions are missing.
(bits.h, types.h, spinlock.h, ...)

...

> +struct mlxbf3_gpio_context {
> +	struct gpio_chip gc;

> +	struct irq_chip irq_chip;

Have you run it on v6.1+ kernels? This should not be here, i.e. it must be
static and const.

> +	/* YU GPIO block address */
> +	void __iomem *gpio_io;
> +
> +	/* YU GPIO cause block address */
> +	void __iomem *gpio_cause_io;
> +
> +	/* Mask of valid gpios that can be accessed by software */
> +	unsigned int valid_mask;
> +};

...

> +		generic_handle_irq(irq_find_mapping(gc->irq.domain, level));

There is a helper that unites these two calls together.

...

> +	bool fall = false;
> +	bool rise = false;

Instead, just assign each of the in the corresponding switch-case.

> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_BOTH:
> +		fall = true;
> +		rise = true;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		rise = true;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		fall = true;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

...

> +	raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +	if (fall) {
> +		val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> +		val |= BIT(offset);
> +		writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> +	}
> +
> +	if (rise) {
> +		val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> +		val |= BIT(offset);
> +		writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> +	}
> +	raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);

Don't you need to choose and lock the IRQ handler here?

> +}

...

> +static int mlxbf3_gpio_init_valid_mask(struct gpio_chip *gc,
> +				       unsigned long *valid_mask,
> +				       unsigned int ngpios)
> +{
> +	struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
> +
> +	*valid_mask = gs->valid_mask;
> +
> +	return 0;
> +}

Why do you need this?


> +	struct resource *res;

Useless variable, see below.

...

> +	const char *name;


> +	name = dev_name(dev);

Useless, just call dev_name() where it's needed.

...

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	/* Resource shared with pinctrl driver */
> +	gs->gpio_io = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!gs->gpio_io)
> +		return -ENOMEM;
> +
> +	/* YU GPIO block address */
> +	gs->gpio_cause_io = devm_platform_ioremap_resource(pdev, 1);
> +	if (IS_ERR(gs->gpio_cause_io))
> +		return PTR_ERR(gs->gpio_cause_io);

These can be folded in a single devm_platform_ioremap_resource() call.

...


> +	if (device_property_read_u32(dev, "npins", &npins))
> +		npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;

You can get of conditional.

	npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
	device_property_read_u32(dev, "npins", &npins);

...

> +	if (device_property_read_u32(dev, "valid_mask", &valid_mask))
> +		valid_mask = 0x0;

Besides that you can move this directly to the respective call and drop
redundant private copy of valid mask, you mean that without that property
no valid GPIOs?

> +	gs->valid_mask = valid_mask;

...

> +		girq->handler = handle_simple_irq;

Should be handle_bad_irq() to avoid some subtle issues that hard to debug.

...

> +		ret = devm_request_irq(dev, irq, mlxbf3_gpio_irq_handler,
> +				       IRQF_SHARED, name, gs);
> +		if (ret) {

> +			dev_err(dev, "failed to request IRQ");
> +			return ret;

return dev_err_probe(...);

> +		}
> +	}

...

> +	ret = devm_gpiochip_add_data(dev, &gs->gc, gs);
> +	if (ret) {
> +		dev_err(dev, "Failed adding memory mapped gpiochip\n");
> +		return ret;

Ditto.

> +	}

...

> +static const struct acpi_device_id __maybe_unused mlxbf3_gpio_acpi_match[] = {
> +	{ "MLNXBF33", 0 },

> +	{},

No comma for termination entry.

> +};

...

> +	.probe    = mlxbf3_gpio_probe,
> +};

> +

Redundant blank line.

> +module_platform_driver(mlxbf3_gpio_driver);

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 1/2] Support NVIDIA BlueField-3 GPIO controller

[Asmaa Mnebhi]

Thanks Andy for the feedback! Will address your comment in v4.

> +		generic_handle_irq(irq_find_mapping(gc->irq.domain, level));

There is a helper that unites these two calls together.

I didn't find any helper in v6.2. Could you please point me to it?

> +static int mlxbf3_gpio_init_valid_mask(struct gpio_chip *gc,
> +				       unsigned long *valid_mask,
> +				       unsigned int ngpios)
> +{
> +	struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
> +
> +	*valid_mask = gs->valid_mask;
> +
> +	return 0;
> +}

Why do you need this?

Since we only use ACPI tables and we want user space (libgpiod tool) or possibly (in the future)
other kernel drivers to have access to certain GPIO pins, we use this valid_mask to do so.
Linus previously explained that if we ask for a GPIO then it will be muxed in using .gpio_request_enable().
And so we would use .valid_mask to restrict the use of certain gpios.

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

[andy.shevchenko]

Mon, Feb 13, 2023 at 11:14:06PM +0000, Asmaa Mnebhi kirjoitti:
> Thanks Andy for the feedback! Will address your comment in v4.
> 
> > +		generic_handle_irq(irq_find_mapping(gc->irq.domain, level));
> 
> There is a helper that unites these two calls together.
> 
> I didn't find any helper in v6.2. Could you please point me to it?

It's available even longer than just in v6.2:
generic_handle_domain_irq().

...

> > +static int mlxbf3_gpio_init_valid_mask(struct gpio_chip *gc,
> > +				       unsigned long *valid_mask,
> > +				       unsigned int ngpios)
> > +{
> > +	struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
> > +
> > +	*valid_mask = gs->valid_mask;
> > +
> > +	return 0;
> > +}
> 
> Why do you need this?
> 
> Since we only use ACPI tables and we want user space (libgpiod tool) or possibly (in the future)
> other kernel drivers to have access to certain GPIO pins, we use this valid_mask to do so.
> Linus previously explained that if we ask for a GPIO then it will be muxed in using .gpio_request_enable().
> And so we would use .valid_mask to restrict the use of certain gpios.

So, why you can't use gpio-reserved-ranges property?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/2] Support NVIDIA BlueField-3 pinctrl driver

[Asmaa Mnebhi]

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>
---
 drivers/pinctrl/Kconfig         |  10 +
 drivers/pinctrl/Makefile        |   1 +
 drivers/pinctrl/pinctrl-mlxbf.c | 341 ++++++++++++++++++++++++++++++++
 3 files changed, 352 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-mlxbf.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 7d5f5458c72e..7040d79a757a 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -523,6 +523,16 @@ config PINCTRL_ZYNQMP
 	  This driver can also be built as a module. If so, the module
 	  will be called pinctrl-zynqmp.
 
+config PINCTRL_MLXBF
+	tristate "NVIDIA BlueField-3 SoC Pinctrl driver"
+	depends on (MELLANOX_PLATFORM && ARM64 && ACPI)
+	select PINMUX
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
+	select GPIO_MLXBF3
+	help
+	  This selects the pinctrl driver for BlueField-3 SoCs.
+
 source "drivers/pinctrl/actions/Kconfig"
 source "drivers/pinctrl/aspeed/Kconfig"
 source "drivers/pinctrl/bcm/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index d5939840bb2a..67252469e76b 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_PINCTRL_MCP23S08_I2C)	+= pinctrl-mcp23s08_i2c.o
 obj-$(CONFIG_PINCTRL_MCP23S08_SPI)	+= pinctrl-mcp23s08_spi.o
 obj-$(CONFIG_PINCTRL_MCP23S08)	+= pinctrl-mcp23s08.o
 obj-$(CONFIG_PINCTRL_MICROCHIP_SGPIO)	+= pinctrl-microchip-sgpio.o
+obj-$(CONFIG_PINCTRL_MLXBF)	+= pinctrl-mlxbf.o
 obj-$(CONFIG_PINCTRL_OCELOT)	+= pinctrl-ocelot.o
 obj-$(CONFIG_PINCTRL_OXNAS)	+= pinctrl-oxnas.o
 obj-$(CONFIG_PINCTRL_PALMAS)	+= pinctrl-palmas.o
diff --git a/drivers/pinctrl/pinctrl-mlxbf.c b/drivers/pinctrl/pinctrl-mlxbf.c
new file mode 100644
index 000000000000..405e5a906ee4
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-mlxbf.c
@@ -0,0 +1,341 @@
+// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause
+
+/*
+ * Copyright (C) 2022 NVIDIA CORPORATION & AFFILIATES
+ */
+
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#define MLXBF_GPIO0_FW_CONTROL_SET   0
+#define MLXBF_GPIO0_FW_CONTROL_CLEAR 0x14
+#define MLXBF_GPIO1_FW_CONTROL_SET   0x80
+#define MLXBF_GPIO1_FW_CONTROL_CLEAR 0x94
+
+#define MLXBF_NGPIOS_GPIO0    32
+
+enum {
+	MLXBF_GPIO_HW_MODE,
+	MLXBF_GPIO_SW_MODE
+};
+
+struct mlxbf_pinctrl {
+	void __iomem *base;
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+	struct pinctrl_gpio_range gpio_range;
+};
+
+#define MLXBF_GPIO_RANGE(_id, _pinbase, _gpiobase, _npins)	\
+	{							\
+		.name = "mlxbf_gpio_range",			\
+		.id = _id,					\
+		.base = _gpiobase,				\
+		.pin_base = _pinbase,				\
+		.npins = _npins,				\
+	}
+
+static struct pinctrl_gpio_range mlxbf_pinctrl_gpio_ranges[] = {
+	MLXBF_GPIO_RANGE(0, 0,  480, 32),
+	MLXBF_GPIO_RANGE(1,  32, 456, 24),
+};
+
+static const struct pinctrl_pin_desc mlxbf_pins[] = {
+	PINCTRL_PIN(0, "gpio0"),
+	PINCTRL_PIN(1, "gpio1"),
+	PINCTRL_PIN(2, "gpio2"),
+	PINCTRL_PIN(3, "gpio3"),
+	PINCTRL_PIN(4, "gpio4"),
+	PINCTRL_PIN(5, "gpio5"),
+	PINCTRL_PIN(6, "gpio6"),
+	PINCTRL_PIN(7, "gpio7"),
+	PINCTRL_PIN(8, "gpio8"),
+	PINCTRL_PIN(9, "gpio9"),
+	PINCTRL_PIN(10, "gpio10"),
+	PINCTRL_PIN(11, "gpio11"),
+	PINCTRL_PIN(12, "gpio12"),
+	PINCTRL_PIN(13, "gpio13"),
+	PINCTRL_PIN(14, "gpio14"),
+	PINCTRL_PIN(15, "gpio15"),
+	PINCTRL_PIN(16, "gpio16"),
+	PINCTRL_PIN(17, "gpio17"),
+	PINCTRL_PIN(18, "gpio18"),
+	PINCTRL_PIN(19, "gpio19"),
+	PINCTRL_PIN(20, "gpio20"),
+	PINCTRL_PIN(21, "gpio21"),
+	PINCTRL_PIN(22, "gpio22"),
+	PINCTRL_PIN(23, "gpio23"),
+	PINCTRL_PIN(24, "gpio24"),
+	PINCTRL_PIN(25, "gpio25"),
+	PINCTRL_PIN(26, "gpio26"),
+	PINCTRL_PIN(27, "gpio27"),
+	PINCTRL_PIN(28, "gpio28"),
+	PINCTRL_PIN(29, "gpio29"),
+	PINCTRL_PIN(30, "gpio30"),
+	PINCTRL_PIN(31, "gpio31"),
+	PINCTRL_PIN(32, "gpio32"),
+	PINCTRL_PIN(33, "gpio33"),
+	PINCTRL_PIN(34, "gpio34"),
+	PINCTRL_PIN(35, "gpio35"),
+	PINCTRL_PIN(36, "gpio36"),
+	PINCTRL_PIN(37, "gpio37"),
+	PINCTRL_PIN(38, "gpio38"),
+	PINCTRL_PIN(39, "gpio39"),
+	PINCTRL_PIN(40, "gpio40"),
+	PINCTRL_PIN(41, "gpio41"),
+	PINCTRL_PIN(42, "gpio42"),
+	PINCTRL_PIN(43, "gpio43"),
+	PINCTRL_PIN(44, "gpio44"),
+	PINCTRL_PIN(45, "gpio45"),
+	PINCTRL_PIN(46, "gpio46"),
+	PINCTRL_PIN(47, "gpio47"),
+	PINCTRL_PIN(48, "gpio48"),
+	PINCTRL_PIN(49, "gpio49"),
+	PINCTRL_PIN(50, "gpio50"),
+	PINCTRL_PIN(51, "gpio51"),
+	PINCTRL_PIN(52, "gpio52"),
+	PINCTRL_PIN(53, "gpio53"),
+	PINCTRL_PIN(54, "gpio54"),
+	PINCTRL_PIN(55, "gpio55"),
+};
+
+/*
+ * All single-pin functions can be mapped to any GPIO, however pinmux applies
+ * functions to pin groups and only those groups declared as supporting that
+ * function. To make this work we must put each pin in its own dummy group so
+ * that the functions can be described as applying to all pins.
+ * We use the same name as in the datasheet.
+ */
+static const char * const mlxbf_pinctrl_single_group_names[] = {
+	"gpio0", "gpio1",  "gpio2",  "gpio3",  "gpio4",  "gpio5",  "gpio6",
+	"gpio7", "gpio8",  "gpio9",  "gpio10", "gpio11", "gpio12", "gpio13",
+	"gpio14", "gpio15", "gpio16", "gpio17", "gpio18", "gpio19", "gpio20",
+	"gpio21", "gpio22", "gpio23", "gpio24", "gpio25", "gpio26", "gpio27",
+	"gpio28", "gpio29", "gpio30", "gpio31", "gpio32", "gpio33", "gpio34",
+	"gpio35", "gpio36", "gpio37", "gpio38", "gpio39", "gpio40", "gpio41",
+	"gpio42", "gpio43", "gpio44", "gpio45", "gpio46", "gpio47", "gpio48",
+	"gpio49", "gpio50", "gpio51", "gpio52", "gpio53", "gpio54", "gpio55"
+};
+
+/* Set of pin numbers for single-pin groups */
+static const unsigned int mlxbf_pinctrl_single_group_pins[] = {
+	0,  1,  2,  3,  4,  5,  6,
+	7,  8,  9, 10, 11, 12, 13,
+	14, 15, 16, 17, 18, 19, 20,
+	21, 22, 23, 24, 25, 26, 27,
+	28, 29, 30, 31, 32, 33, 34,
+	35, 36, 37, 38, 39, 40, 41,
+	42, 43, 44, 45, 46, 47, 48,
+	49, 50, 51, 52, 53, 54, 55,
+};
+
+static int mlxbf_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	/* Number single-pin groups */
+	return ARRAY_SIZE(mlxbf_pinctrl_single_group_pins);
+}
+
+static const char *mlxbf_get_group_name(struct pinctrl_dev *pctldev,
+					 unsigned int selector)
+{
+	return mlxbf_pinctrl_single_group_names[selector];
+}
+
+static int mlxbf_get_group_pins(struct pinctrl_dev *pctldev,
+				 unsigned int selector,
+				 const unsigned int **pins,
+				 unsigned int *num_pins)
+{
+	/* return the dummy group for a single pin */
+	*pins = &mlxbf_pinctrl_single_group_pins[selector];
+	*num_pins = 1;
+
+	return 0;
+}
+
+static const struct pinctrl_ops mlxbf_pinctrl_group_ops = {
+	.get_groups_count = mlxbf_get_groups_count,
+	.get_group_name = mlxbf_get_group_name,
+	.get_group_pins = mlxbf_get_group_pins,
+};
+
+static const char * const mlxbf_gpiofunc_group_names[] = { "swctrl" };
+static const char * const mlxbf_hwfunc_group_names[]   = { "hwctrl" };
+
+/*
+ * Only 2 functions are supported and they apply to all pins:
+ * 1) Default hardware functionality
+ * 2) Software controlled GPIO
+ */
+static const struct {
+	const char *name;
+	const char * const *group_names;
+} mlxbf_pmx_funcs[] = {
+	{
+		.name = "hwfunc",
+		.group_names = mlxbf_hwfunc_group_names
+	},
+	{
+		.name = "gpiofunc",
+		.group_names = mlxbf_gpiofunc_group_names
+	},
+};
+
+static int mlxbf_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(mlxbf_pmx_funcs);
+}
+
+static const char *mlxbf_pmx_get_func_name(struct pinctrl_dev *pctldev,
+					   unsigned int selector)
+{
+	return mlxbf_pmx_funcs[selector].name;
+}
+
+static int mlxbf_pmx_get_groups(struct pinctrl_dev *pctldev,
+				 unsigned int selector,
+				 const char * const **groups,
+				 unsigned int * const num_groups)
+{
+	*groups = mlxbf_pmx_funcs[selector].group_names;
+	*num_groups = ARRAY_SIZE(mlxbf_pinctrl_single_group_pins);
+
+	return 0;
+}
+
+static int mlxbf_pmx_set(struct pinctrl_dev *pctldev,
+			      unsigned int selector,
+			      unsigned int group)
+{
+	struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	if (selector == MLXBF_GPIO_HW_MODE) {
+		if (group < MLXBF_NGPIOS_GPIO0)
+			writel(BIT(group), priv->base + MLXBF_GPIO0_FW_CONTROL_CLEAR);
+		else
+			writel(BIT(group % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_CLEAR);
+	}
+
+	if (selector == MLXBF_GPIO_SW_MODE) {
+		if (group < MLXBF_NGPIOS_GPIO0)
+			writel(BIT(group), priv->base + MLXBF_GPIO0_FW_CONTROL_SET);
+		else
+			writel(BIT(group % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_SET);
+	}
+
+	return 0;
+}
+
+static int mlxbf_gpio_request_enable(struct pinctrl_dev *pctldev,
+				     struct pinctrl_gpio_range *range,
+				     unsigned int offset)
+{
+	struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	if (offset < MLXBF_NGPIOS_GPIO0)
+		writel(BIT(offset), priv->base + MLXBF_GPIO0_FW_CONTROL_SET);
+	else
+		writel(BIT(offset % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_SET);
+
+	return 0;
+}
+
+static void mlxbf_gpio_disable_free(struct pinctrl_dev *pctldev,
+				    struct pinctrl_gpio_range *range,
+				    unsigned int offset)
+{
+	struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	/* disable GPIO functionality by giving control back to hardware */
+	if (offset < MLXBF_NGPIOS_GPIO0)
+		writel(BIT(offset), priv->base + MLXBF_GPIO0_FW_CONTROL_CLEAR);
+	else
+		writel(BIT(offset % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_CLEAR);
+
+}
+
+static const struct pinmux_ops mlxbf_pmx_ops = {
+	.get_functions_count = mlxbf_pmx_get_funcs_count,
+	.get_function_name = mlxbf_pmx_get_func_name,
+	.get_function_groups = mlxbf_pmx_get_groups,
+	.set_mux = mlxbf_pmx_set,
+	.gpio_request_enable = mlxbf_gpio_request_enable,
+	.gpio_disable_free = mlxbf_gpio_disable_free,
+};
+
+static struct pinctrl_desc mlxbf_pin_desc = {
+	.name = "pinctrl-mlxbf",
+	.pins = mlxbf_pins,
+	.npins = ARRAY_SIZE(mlxbf_pins),
+	.pctlops = &mlxbf_pinctrl_group_ops,
+	.pmxops = &mlxbf_pmx_ops,
+	.owner = THIS_MODULE,
+};
+
+static int mlxbf_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mlxbf_pinctrl *priv;
+	struct resource *res;
+	int ret;
+
+	BUILD_BUG_ON(ARRAY_SIZE(mlxbf_pinctrl_single_group_names) !=
+		     ARRAY_SIZE(mlxbf_pinctrl_single_group_pins));
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	priv->base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!priv->base)
+		return -ENOMEM;
+
+	ret = devm_pinctrl_register_and_init(priv->dev,
+					     &mlxbf_pin_desc,
+					     priv,
+					     &priv->pctl);
+	if (ret) {
+		dev_err(priv->dev, "Failed pinctrl register (%d)\n", ret);
+		return ret;
+	}
+
+	ret = pinctrl_enable(priv->pctl);
+	if (ret) {
+		dev_err(priv->dev, "Failed to enable pinctrl (%d)\n", ret);
+		return ret;
+	}
+
+	pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[0]);
+	pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[1]);
+
+	return 0;
+}
+
+static const struct acpi_device_id mlxbf_pinctrl_acpi_ids[] = {
+	{ "MLNXBF34", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, mlxbf_pinctrl_acpi_ids);
+
+static struct platform_driver mlxbf_pinctrl_driver = {
+	.driver = {
+		.name = "pinctrl-mlxbf",
+		.acpi_match_table = mlxbf_pinctrl_acpi_ids,
+	},
+	.probe = mlxbf_pinctrl_probe,
+};
+
+module_platform_driver(mlxbf_pinctrl_driver);
+
+MODULE_DESCRIPTION("NVIDIA pinctrl driver");
+MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.30.1

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

[Andy Shevchenko]

On Fri, Feb 10, 2023 at 10:39:41AM -0500, Asmaa Mnebhi wrote:
> This patch adds support to the BlueField-3 SoC pin controller.

Please read Submitting Patches on how to create better commit message.

> It allows muxing individual GPIOs to switch from the default
> hardware mode to software controlled mode.

...

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

This is wrong.
Please make sure you cover more testing.
Also, do you really need an ACPI dependency?

> +	select PINMUX
> +	select GPIOLIB
> +	select GPIOLIB_IRQCHIP
> +	select GPIO_MLXBF3
> +	help
> +	  This selects the pinctrl driver for BlueField-3 SoCs.

Same comment as per previous patch.

> +// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause
> +
> +/*
> + * Copyright (C) 2022 NVIDIA CORPORATION & AFFILIATES
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>

Same comments as per previous patch.

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

Based on the above, I stop here and wait for next version where comments given
anywhere are applied to all appropriate places.

...

> +	BUILD_BUG_ON(ARRAY_SIZE(mlxbf_pinctrl_single_group_names) !=
> +		     ARRAY_SIZE(mlxbf_pinctrl_single_group_pins));

static_assert().

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 2/2] Support NVIDIA BlueField-3 pinctrl driver

[Asmaa Mnebhi]

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

This is wrong.
Please make sure you cover more testing.
Also, do you really need an ACPI dependency?

Could you please provide more details on why this is wrong? All our upstreamed drivers use the same "depends on"
Our pinctrl driver only applies to Mellanox platforms, ARM64 and use ACPI tables.  

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

[Andy Shevchenko]

On Thu, Feb 16, 2023 at 05:50:56PM +0000, Asmaa Mnebhi wrote:
> > +config PINCTRL_MLXBF
> > +	tristate "NVIDIA BlueField-3 SoC Pinctrl driver"
> > +	depends on (MELLANOX_PLATFORM && ARM64 && ACPI)
> 
> This is wrong.
> Please make sure you cover more testing.
> Also, do you really need an ACPI dependency?
> 
> Could you please provide more details on why this is wrong? All our upstreamed drivers use the same "depends on"
> Our pinctrl driver only applies to Mellanox platforms, ARM64 and use ACPI tables.

This is wrong because it narrows down testing coverage.

Besides that you need to define functional and build dependencies separately.

ACPI probably is not what you are using in the driver. I do not believe you
have at all dependency on it.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 2/2] Support NVIDIA BlueField-3 pinctrl driver

[Asmaa Mnebhi]

> > +config PINCTRL_MLXBF
> > +	tristate "NVIDIA BlueField-3 SoC Pinctrl driver"
> > +	depends on (MELLANOX_PLATFORM && ARM64 && ACPI)
> 
> This is wrong.
> Please make sure you cover more testing.
> Also, do you really need an ACPI dependency?
> 
> Could you please provide more details on why this is wrong? All our upstreamed drivers use the same "depends on"
> Our pinctrl driver only applies to Mellanox platforms, ARM64 and use ACPI tables.

This is wrong because it narrows down testing coverage.

Besides that you need to define functional and build dependencies separately.

ACPI probably is not what you are using in the driver. I do not believe you have at all dependency on it.

Noted, I will define function and build dependencies separately.
We have our own custom UEFI for BlueField SoCs so ACPI tables are our only options (for users/customer etc... as well) 

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

[Andy Shevchenko]

On Thu, Feb 16, 2023 at 06:44:58PM +0000, Asmaa Mnebhi wrote:
> > > +config PINCTRL_MLXBF
> > > +	tristate "NVIDIA BlueField-3 SoC Pinctrl driver"
> > > +	depends on (MELLANOX_PLATFORM && ARM64 && ACPI)
> > 
> > This is wrong.
> > Please make sure you cover more testing.
> > Also, do you really need an ACPI dependency?
> > 
> > Could you please provide more details on why this is wrong? All our
> > upstreamed drivers use the same "depends on" Our pinctrl driver only
> > applies to Mellanox platforms, ARM64 and use ACPI tables.
> 
> This is wrong because it narrows down testing coverage.
> 
> Besides that you need to define functional and build dependencies separately.
> 
> ACPI probably is not what you are using in the driver. I do not believe you
> have at all dependency on it.
> 
> Noted, I will define function and build dependencies separately.
> We have our own custom UEFI for BlueField SoCs so ACPI tables are our only
> options (for users/customer etc... as well)

I understand that, but I'm pretty sure that driver can be compiled with ACPI=n
which is good for testing coverage.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 2/2] Support NVIDIA BlueField-3 pinctrl driver

[Asmaa Mnebhi]

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@intel.com> 
Sent: Thursday, February 16, 2023 1:54 PM
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
Subject: Re: [PATCH v3 2/2] Support NVIDIA BlueField-3 pinctrl driver
Importance: High

On Thu, Feb 16, 2023 at 06:44:58PM +0000, Asmaa Mnebhi wrote:
> > > +config PINCTRL_MLXBF
> > > +	tristate "NVIDIA BlueField-3 SoC Pinctrl driver"
> > > +	depends on (MELLANOX_PLATFORM && ARM64 && ACPI)
> > 
> > This is wrong.
> > Please make sure you cover more testing.
> > Also, do you really need an ACPI dependency?
> > 
> > Could you please provide more details on why this is wrong? All our 
> > upstreamed drivers use the same "depends on" Our pinctrl driver only 
> > applies to Mellanox platforms, ARM64 and use ACPI tables.
> 
> This is wrong because it narrows down testing coverage.
> 
> Besides that you need to define functional and build dependencies separately.
> 
> ACPI probably is not what you are using in the driver. I do not 
> believe you have at all dependency on it.
> 
> Noted, I will define function and build dependencies separately.
> We have our own custom UEFI for BlueField SoCs so ACPI tables are our 
> only options (for users/customer etc... as well)

I understand that, but I'm pretty sure that driver can be compiled with ACPI=n which is good for testing coverage.

Ah! OK, understood. Will remove it then.

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

[Andy Shevchenko]

On Fri, Feb 10, 2023 at 10:39:39AM -0500, Asmaa Mnebhi 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.
> 
> The BlueField-3 SoC gpio-mlxbf3.c driver handles different hardware registers
> and logic that from gpio-mlxbf.c and gpio-mlxbf2.c.
> For that reason, we have separate drivers for each generation.

It seems you neglected to include maintainers and reviewers of the previous
version(s) of your series. Don't do this. Please, respect people who invested
their time in your code.

Hint: I have a "smart" script [1] which helps to collect proper people (except
reviewers that you need to add manually via --cc command line parameter) and
mailing lists. Fell free to re-use, modify, send feedback.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 0/2] Support Nvidia BlueField-3 GPIO driver and pin controller

[Asmaa Mnebhi]

Support the BlueField-3 SoC GPIO driver for handling interrupts and
providing the option to change the direction and value of a GPIO.
Support the BlueField-3 SoC pin controller driver for allowing a
select number of GPIO pins to be manipulated from userspace or
the kernel.

The gpio-mlxbf3.c driver handles hardware registers and logic
that are different from gpio-mlxbf.c and gpio-mlxbf2.c.
For that reason, we have separate drivers for each generation.

Changes from v3->v4:
gpio-mlxbf3.c:
- Update the Kconfig file so that it is conform with checkpatch
- Remove unncessary headers and add missing header inclusions
- Make irq_chip struct static and const
- Replace generic_handle_irq(irq_find_mapping) with
  generic_handle_domain_irq
- Simplify logic in irq_set_type
- Replace valid_mask with gpio-reserved-ranges
- Cleanup code

pinctrl-mlxbf.c:
- Cleanup code
- Update the Kconfig file so that it is conform with checkpatch
- Remove unncessary headers and add missing header inclusions

Asmaa Mnebhi (2):
  gpio: gpio-mlxbf3: Add gpio driver support
  pinctrl: pinctrl-mlxbf: Add pinctrl driver support

 drivers/gpio/Kconfig            |  12 ++
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-mlxbf3.c      | 230 ++++++++++++++++++++++
 drivers/pinctrl/Kconfig         |  14 ++
 drivers/pinctrl/Makefile        |   1 +
 drivers/pinctrl/pinctrl-mlxbf.c | 338 ++++++++++++++++++++++++++++++++
 6 files changed, 596 insertions(+)
 create mode 100644 drivers/gpio/gpio-mlxbf3.c
 create mode 100644 drivers/pinctrl/pinctrl-mlxbf.c

-- 
2.30.1

[PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Asmaa Mnebhi]

Add support for the BlueField-3 SoC GPIO driver.
This driver configures and handles GPIO interrupts. It also enables a user
to manipulate certain GPIO pins via libgpiod tools or other kernel drivers.
The usables pins are defined via the gpio-reserved-ranges property.

Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 drivers/gpio/Kconfig       |  12 ++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-mlxbf3.c | 230 +++++++++++++++++++++++++++++++++++++
 3 files changed, 243 insertions(+)
 create mode 100644 drivers/gpio/gpio-mlxbf3.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ec7cfd4f52b1..5cddcb56353d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1534,6 +1534,18 @@ config GPIO_MLXBF2
 	help
 	  Say Y here if you want GPIO support on Mellanox BlueField 2 SoC.
 
+config GPIO_MLXBF3
+	tristate "Mellanox BlueField 3 SoC GPIO"
+	depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || (64BIT && COMPILE_TEST)
+	select GPIO_GENERIC
+	help
+	  Say Y if you want GPIO support on Mellanox BlueField 3 SoC.
+	  This GPIO controller supports interrupt handling and enables the
+	  manipulation of certain GPIO pins.
+	  This controller should be used in parallel with pinctrl-mlxbf to
+	  control the desired gpios.
+	  This driver can also be built as a module called mlxbf3-gpio.
+
 config GPIO_ML_IOH
 	tristate "OKI SEMICONDUCTOR ML7213 IOH GPIO support"
 	depends on X86 || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 010587025fc8..76545ca31457 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_GPIO_MERRIFIELD)		+= gpio-merrifield.o
 obj-$(CONFIG_GPIO_ML_IOH)		+= gpio-ml-ioh.o
 obj-$(CONFIG_GPIO_MLXBF)		+= gpio-mlxbf.o
 obj-$(CONFIG_GPIO_MLXBF2)		+= gpio-mlxbf2.o
+obj-$(CONFIG_GPIO_MLXBF3)		+= gpio-mlxbf3.o
 obj-$(CONFIG_GPIO_MM_LANTIQ)		+= gpio-mm-lantiq.o
 obj-$(CONFIG_GPIO_MOCKUP)		+= gpio-mockup.o
 obj-$(CONFIG_GPIO_MOXTET)		+= gpio-moxtet.o
diff --git a/drivers/gpio/gpio-mlxbf3.c b/drivers/gpio/gpio-mlxbf3.c
new file mode 100644
index 000000000000..cdc93c8e77ff
--- /dev/null
+++ b/drivers/gpio/gpio-mlxbf3.c
@@ -0,0 +1,230 @@
+// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause
+/* Copyright (C) 2022 NVIDIA CORPORATION & AFFILIATES */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/version.h>
+
+/*
+ * There are 2 YU GPIO blocks:
+ * gpio[0]: HOST_GPIO0->HOST_GPIO31
+ * gpio[1]: HOST_GPIO32->HOST_GPIO55
+ */
+#define MLXBF3_GPIO_MAX_PINS_PER_BLOCK 32
+
+/*
+ * fw_gpio[x] block registers and their offset
+ */
+#define MLXBF_GPIO_FW_OUTPUT_ENABLE_SET	  0x04
+#define MLXBF_GPIO_FW_DATA_OUT_SET        0x08
+#define MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR 0x18
+#define MLXBF_GPIO_FW_DATA_OUT_CLEAR      0x1c
+#define MLXBF_GPIO_CAUSE_RISE_EN          0x28
+#define MLXBF_GPIO_CAUSE_FALL_EN          0x2c
+#define MLXBF_GPIO_READ_DATA_IN           0x30
+
+#define MLXBF_GPIO_CAUSE_OR_CAUSE_EVTEN0  0x00
+#define MLXBF_GPIO_CAUSE_OR_EVTEN0        0x14
+#define MLXBF_GPIO_CAUSE_OR_CLRCAUSE      0x18
+
+struct mlxbf3_gpio_context {
+	struct gpio_chip gc;
+
+	/* YU GPIO block address */
+	void __iomem *gpio_io;
+
+	/* YU GPIO cause block address */
+	void __iomem *gpio_cause_io;
+};
+
+static void mlxbf3_gpio_irq_enable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	writel(BIT(offset), gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CLRCAUSE);
+
+	val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+	val |= BIT(offset);
+	writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+	raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+}
+
+static void mlxbf3_gpio_irq_disable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+	val &= ~BIT(offset);
+	writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+	raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+}
+
+static irqreturn_t mlxbf3_gpio_irq_handler(int irq, void *ptr)
+{
+	struct mlxbf3_gpio_context *gs = ptr;
+	struct gpio_chip *gc = &gs->gc;
+	unsigned long pending;
+	u32 level;
+
+	pending = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CAUSE_EVTEN0);
+	writel(pending, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CLRCAUSE);
+
+	for_each_set_bit(level, &pending, gc->ngpio)
+		generic_handle_domain_irq(gc->irq.domain, level);
+
+	return IRQ_RETVAL(pending);
+}
+
+static int
+mlxbf3_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_BOTH:
+		val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
+		val |= BIT(offset);
+		writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
+		val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
+		val |= BIT(offset);
+		writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
+		val |= BIT(offset);
+		writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
+		val |= BIT(offset);
+		writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+
+	irq_set_handler_locked(irqd, handle_simple_irq);
+
+	return 0;
+}
+
+static const struct irq_chip gpio_mlxbf3_irqchip = {
+	.name = "MLNXBF33",
+	.irq_set_type = mlxbf3_gpio_irq_set_type,
+	.irq_enable = mlxbf3_gpio_irq_enable,
+	.irq_disable = mlxbf3_gpio_irq_disable,
+};
+
+static int
+mlxbf3_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mlxbf3_gpio_context *gs;
+	struct gpio_irq_chip *girq;
+	struct gpio_chip *gc;
+	unsigned int npins;
+	int ret, irq;
+
+	gs = devm_kzalloc(dev, sizeof(*gs), GFP_KERNEL);
+	if (!gs)
+		return -ENOMEM;
+
+	gs->gpio_io = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(gs->gpio_io))
+		return PTR_ERR(gs->gpio_io);
+
+	gs->gpio_cause_io = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(gs->gpio_cause_io))
+		return PTR_ERR(gs->gpio_cause_io);
+
+	npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
+	device_property_read_u32(dev, "npins", &npins);
+
+	gc = &gs->gc;
+
+	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,
+			gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_SET,
+			gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR, 0);
+
+	gc->request = gpiochip_generic_request;
+	gc->free = gpiochip_generic_free;
+
+	gc->ngpio = npins;
+	gc->owner = THIS_MODULE;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0) {
+		girq = &gs->gc.irq;
+		gpio_irq_chip_set_chip(girq, &gpio_mlxbf3_irqchip);
+		girq->default_type = IRQ_TYPE_NONE;
+		/* This will let us handle the parent IRQ in the driver */
+		girq->num_parents = 0;
+		girq->parents = NULL;
+		girq->parent_handler = NULL;
+
+		/*
+		 * Directly request the irq here instead of passing
+		 * a flow-handler because the irq is shared.
+		 */
+		ret = devm_request_irq(dev, irq, mlxbf3_gpio_irq_handler,
+				       IRQF_SHARED, dev_name(dev), gs);
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request IRQ");
+	}
+
+	platform_set_drvdata(pdev, gs);
+
+	ret = devm_gpiochip_add_data(dev, &gs->gc, gs);
+	if (ret)
+		dev_err_probe(dev, ret, "Failed adding memory mapped gpiochip\n");
+
+	return 0;
+}
+
+static const struct acpi_device_id __maybe_unused mlxbf3_gpio_acpi_match[] = {
+	{ "MLNXBF33", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, mlxbf3_gpio_acpi_match);
+
+static struct platform_driver mlxbf3_gpio_driver = {
+	.driver = {
+		.name = "mlxbf3_gpio",
+		.acpi_match_table = mlxbf3_gpio_acpi_match,
+	},
+	.probe    = mlxbf3_gpio_probe,
+};
+module_platform_driver(mlxbf3_gpio_driver);
+
+MODULE_DESCRIPTION("NVIDIA BlueField-3 GPIO Driver");
+MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.30.1

Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Andy Shevchenko]

On Fri, Feb 17, 2023 at 11:27 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> Add support for the BlueField-3 SoC GPIO driver.
> This driver configures and handles GPIO interrupts. It also enables a user
> to manipulate certain GPIO pins via libgpiod tools or other kernel drivers.
> The usables pins are defined via the gpio-reserved-ranges property.

Probably

"gpio-reserved-ranges"

(in double quotes).

...

> +       depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || (64BIT && COMPILE_TEST)

But do you really need the ACPI dependency? I don't see a single bit
of it in the driver.
(Yes, I know about IDs.)

Also, how 64BIT is needed for testing?

...

> +#include <linux/version.h>

I believe it's included automatically. Please, double check the header
inclusions to make sure you don't have some spurious ones in the list.

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

...

> +       int offset = irqd_to_hwirq(irqd);

It returns irq_hw_number_t IIRC. It's an unsigned type. Otherwise you
may have interesting effects.

...

> +       int offset = irqd_to_hwirq(irqd);

Ditto.

...

> +       raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       switch (type & IRQ_TYPE_SENSE_MASK) {
> +       case IRQ_TYPE_EDGE_BOTH:
> +               val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> +               val |= BIT(offset);
> +               writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> +               val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> +               val |= BIT(offset);
> +               writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> +               break;
> +       case IRQ_TYPE_EDGE_RISING:
> +               val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> +               val |= BIT(offset);
> +               writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> +               val |= BIT(offset);
> +               writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> +               break;
> +       default:

Dead lock starts here.

> +               return -EINVAL;
> +       }
> +
> +       raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);

...

> +       irq_set_handler_locked(irqd, handle_simple_irq);

Why not using propert handler type (edge)?

...

> +static const struct irq_chip gpio_mlxbf3_irqchip = {
> +       .name = "MLNXBF33",
> +       .irq_set_type = mlxbf3_gpio_irq_set_type,
> +       .irq_enable = mlxbf3_gpio_irq_enable,
> +       .irq_disable = mlxbf3_gpio_irq_disable,
> +};

Seems missing two things (dunno if bgpio_init() helps with that):
- IMMUTABLE flag
- actual calls to enable and disable IRQs for internal GPIO library usage

See other drivers how it's done. There are even plenty of patches to
enable this thing separately.

...

> +static int
> +mlxbf3_gpio_probe(struct platform_device *pdev)

Doesn't it perfectly fit one line?

...

> +       npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> +       device_property_read_u32(dev, "npins", &npins);

I don't see DT bindings for this property (being added in this
series). Is it already established one?

...

> +               girq->default_type = IRQ_TYPE_NONE;

Assigning handle_bad_irq() is missing.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Asmaa Mnebhi]

> +static const struct irq_chip gpio_mlxbf3_irqchip = {
> +       .name = "MLNXBF33",
> +       .irq_set_type = mlxbf3_gpio_irq_set_type,
> +       .irq_enable = mlxbf3_gpio_irq_enable,
> +       .irq_disable = mlxbf3_gpio_irq_disable, };

Seems missing two things (dunno if bgpio_init() helps with that):
- IMMUTABLE flag
- actual calls to enable and disable IRQs for internal GPIO library usage
See other drivers how it's done. There are even plenty of patches to enable this thing separately.

I saw that in other drivers only irq_enable and irq_disable are defined. Example in gpio-davinci.c:
static struct irq_chip gpio_irqchip = {
.name           = "GPIO",
.irq_enable     = gpio_irq_enable,
.irq_disable    = gpio_irq_disable,
.irq_set_type   = gpio_irq_type,
.flags          = IRQCHIP_SET_TYPE_MASKED,
};

Which I think is ok due to the following logic:

gpiochip_add_irqchip calls
gpiochip_set_irq_hooks which contains the following logic:

if (irqchip->irq_disable) {
                 gc->irq.irq_disable = irqchip->irq_disable;
                 irqchip->irq_disable = gpiochip_irq_disable;
} else {
                 gc->irq.irq_mask = irqchip->irq_mask;
                 irqchip->irq_mask = gpiochip_irq_mask;
}
if (irqchip->irq_enable) {
                 gc->irq.irq_enable = irqchip->irq_enable;
                 irqchip->irq_enable = gpiochip_irq_enable;
} else {
                 gc->irq.irq_unmask = irqchip->irq_unmask;
                 irqchip->irq_unmask = gpiochip_irq_unmask;
}

So it doesn’t seem like we need to define irq_mask/unmask if we have irq_enable/disable?

> +       npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> +       device_property_read_u32(dev, "npins", &npins);

I don't see DT bindings for this property (being added in this series). Is it already established one?

Ah that’s my bad. The property should be called "ngpios" like in the DT documentation. Will fix.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Andy Shevchenko]

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

First of all, please, please, fix your email client!
It's so hard to distinguish my own words from yours.

> > +static const struct irq_chip gpio_mlxbf3_irqchip = {
> > +       .name = "MLNXBF33",
> > +       .irq_set_type = mlxbf3_gpio_irq_set_type,
> > +       .irq_enable = mlxbf3_gpio_irq_enable,
> > +       .irq_disable = mlxbf3_gpio_irq_disable, };
>
> Seems missing two things (dunno if bgpio_init() helps with that):
> - IMMUTABLE flag
> - actual calls to enable and disable IRQs for internal GPIO library usage
> See other drivers how it's done. There are even plenty of patches to enable this thing separately.
>
> I saw that in other drivers only irq_enable and irq_disable are defined. Example in gpio-davinci.c:
> static struct irq_chip gpio_irqchip = {
> .name           = "GPIO",
> .irq_enable     = gpio_irq_enable,
> .irq_disable    = gpio_irq_disable,
> .irq_set_type   = gpio_irq_type,
> .flags          = IRQCHIP_SET_TYPE_MASKED,
> };
>
> Which I think is ok due to the following logic:
>
> gpiochip_add_irqchip calls
> gpiochip_set_irq_hooks which contains the following logic:
>
> if (irqchip->irq_disable) {
>                  gc->irq.irq_disable = irqchip->irq_disable;
>                  irqchip->irq_disable = gpiochip_irq_disable;
> } else {
>                  gc->irq.irq_mask = irqchip->irq_mask;
>                  irqchip->irq_mask = gpiochip_irq_mask;
> }
> if (irqchip->irq_enable) {
>                  gc->irq.irq_enable = irqchip->irq_enable;
>                  irqchip->irq_enable = gpiochip_irq_enable;
> } else {
>                  gc->irq.irq_unmask = irqchip->irq_unmask;
>                  irqchip->irq_unmask = gpiochip_irq_unmask;
> }

The chips have another flag there:

        .flags          = IRQCHIP_IMMUTABLE,
       GPIOCHIP_IRQ_RESOURCE_HELPERS,

> So it doesn’t seem like we need to define irq_mask/unmask if we have irq_enable/disable?
>
> > +       npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> > +       device_property_read_u32(dev, "npins", &npins);
>
> I don't see DT bindings for this property (being added in this series). Is it already established one?
>
> Ah that’s my bad. The property should be called "ngpios" like in the DT documentation. Will fix.

And why do you need it? What's a corner case that the GPIO library
doesn't handle yet?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Asmaa Mnebhi]

> > > +       npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> > > +       device_property_read_u32(dev, "npins", &npins);
> >
> > I don't see DT bindings for this property (being added in this series). Is it
> already established one?
> >
> > Ah that’s my bad. The property should be called "ngpios" like in the DT
> documentation. Will fix.
> 
> And why do you need it? What's a corner case that the GPIO library doesn't
> handle yet?

We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip 1 supports only 24 pins.
If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will correctly set the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip 1:

gpiochip_add_data_with_key {
	[...]
	ngpios = gc->ngpio;
	if (ngpios == 0) {
		ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios);
		if (ret == -ENODATA)
			/*
			 * -ENODATA means that there is no property found and
			 * we want to issue the error message to the user.
			 * Besides that, we want to return different error code
			 * to state that supplied value is not valid.
			 */
			ngpios = 0;
		else if (ret)
			goto err_free_dev_name;

		gc->ngpio = ngpios;
	}
	[...]
}

bgpio_init {
	gc->bgpio_bits = sz * 8;
	gc->ngpio = gc->bgpio_bits;
}

Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Andy Shevchenko]

On Thu, Feb 23, 2023 at 9:08 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> > > > +       npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> > > > +       device_property_read_u32(dev, "npins", &npins);
> > >
> > > I don't see DT bindings for this property (being added in this series). Is it
> > already established one?
> > >
> > > Ah that’s my bad. The property should be called "ngpios" like in the DT
> > documentation. Will fix.
> >
> > And why do you need it? What's a corner case that the GPIO library doesn't
> > handle yet?
>
> We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip 1 supports only 24 pins.
> If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will correctly set the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip 1:

So, either you need to have two entries in DT per chip or ngpios should be 56.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Asmaa Mnebhi]

> >
> > > > > +       npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> > > > > +       device_property_read_u32(dev, "npins", &npins);
> > > >
> > > > I don't see DT bindings for this property (being added in this
> > > > series). Is it
> > > already established one?
> > > >
> > > > Ah that’s my bad. The property should be called "ngpios" like in
> > > > the DT
> > > documentation. Will fix.
> > >
> > > And why do you need it? What's a corner case that the GPIO library
> > > doesn't handle yet?
> >
> > We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip 1
> supports only 24 pins.
> > If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will correctly set
> the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip 1:
> 
> So, either you need to have two entries in DT per chip or ngpios should be 56.
> 
I already have 2 entries in my ACPI table, in the first entry, ngpios = 32 and in the second entry ngpios = 24.
Gpiochip_add_data_with_keys only reads the ngpios property if (ngpios == 0) which is not the case when 
bgpio_init is called. bgpio_init uses "sz" argument to populate the ngpio in bgpio_init, which is not what we want.

Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Andy Shevchenko]

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

...

> > > > > Ah that’s my bad. The property should be called "ngpios" like in
> > > > > the DT
> > > > documentation. Will fix.
> > > >
> > > > And why do you need it? What's a corner case that the GPIO library
> > > > doesn't handle yet?
> > >
> > > We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip 1
> > supports only 24 pins.
> > > If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will correctly set
> > the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip 1:
> >
> > So, either you need to have two entries in DT per chip or ngpios should be 56.
> >
> I already have 2 entries in my ACPI table, in the first entry, ngpios = 32 and in the second entry ngpios = 24.

Can you show the DSDT excerpt of this device? (Also including the
pieces for pin control)

Is this a table of the device in the wild?

> Gpiochip_add_data_with_keys only reads the ngpios property if (ngpios == 0) which is not the case when
> bgpio_init is called. bgpio_init uses "sz" argument to populate the ngpio in bgpio_init, which is not what we want.

Maybe bgpio_init() is not a good API for your case?

--
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Asmaa Mnebhi]

> > > > > > Ah that’s my bad. The property should be called "ngpios" like
> > > > > > in the DT
> > > > > documentation. Will fix.
> > > > >
> > > > > And why do you need it? What's a corner case that the GPIO
> > > > > library doesn't handle yet?
> > > >
> > > > We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip
> > > > 1
> > > supports only 24 pins.
> > > > If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will
> > > > correctly set
> > > the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip
> 1:
> > >
> > > So, either you need to have two entries in DT per chip or ngpios should be
> 56.
> > >
> > I already have 2 entries in my ACPI table, in the first entry, ngpios = 32 and
> in the second entry ngpios = 24.
> 
> Can you show the DSDT excerpt of this device? (Also including the pieces for
> pin control)

Our ACPI tables are internal:

Device(GPI0) {
        Name(_HID, "MLNXBF33")
        Name(_UID, Zero)
        Name(_CCA, 1)
        Name(_CRS, ResourceTemplate() {
          // for fw_gpio[0] yu block
         Memory32Fixed(ReadWrite, 0x13401100, 0x00000080)
         // for yu_gpio_causereg0 block
         Memory32Fixed(ReadWrite, 0x13401c00, 0x00000020)
         Interrupt(ResourceConsumer, Edge, ActiveHigh, Shared)
           { BF_RSH0_YU }
       })
       Name(_DSD, Package() {
         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
         Package() {
           Package () { "ngpios", 32 }, // Number of GPIO pins on gpio block 0
           Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 3, 11, 31}},
         }
       })
     }
 Device(GPI1) {
       Name(_HID, "MLNXBF33")
       Name(_UID, 1)
       Name(_CCA, 1)
       Name(_CRS, ResourceTemplate() {
         // for fw_gpio[1] yu block
         Memory32Fixed(ReadWrite, 0x13401180, 0x00000080)
         // for yu_gpio_causereg1 block
         Memory32Fixed(ReadWrite, 0x13401c20, 0x00000020)
       })
       Name(_DSD, Package() {
         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
         Package() {
           Package () { "ngpios", 24 }, // Number of GPIO pins on gpio block 1
           Package () { "gpio-reserved-ranges", Package () {0, 21}},
         }
       })
  }

    Device(PIN0) {
      Name(_HID, "MLNXBF34")
      Name(_UID, Zero)
      Name(_CCA, 1)
      Name(_CRS, ResourceTemplate() {
        // for fw_gpio[0] and fw_gpio[1] yu blocks
        Memory32Fixed(ReadWrite, 0x13401100, 0x00000100)
      })
     }

> 
> > Gpiochip_add_data_with_keys only reads the ngpios property if (ngpios
> > == 0) which is not the case when bgpio_init is called. bgpio_init uses "sz"
> argument to populate the ngpio in bgpio_init, which is not what we want.
> 
> Maybe bgpio_init() is not a good API for your case?


At the moment, bgpio_init handles all the direction in/out get/set gpio value for us.
So I can either remove bgpio_init so that we get the correct ngpios property, but will have to define get_gpio, set_gpio, dir in and dir out.
Or I can keep bgpio_init and device_property_read_u32 in the gpio-mlxbf3.c driver.
Please let me know which approach you prefer.

Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Andy Shevchenko]

On Fri, Feb 24, 2023 at 4:42 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> > > > > > > Ah that’s my bad. The property should be called "ngpios" like
> > > > > > > in the DT
> > > > > > documentation. Will fix.
> > > > > >
> > > > > > And why do you need it? What's a corner case that the GPIO
> > > > > > library doesn't handle yet?
> > > > >
> > > > > We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip
> > > > > 1
> > > > supports only 24 pins.
> > > > > If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will
> > > > > correctly set
> > > > the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip
> > 1:
> > > >
> > > > So, either you need to have two entries in DT per chip or ngpios should be
> > 56.
> > > >
> > > I already have 2 entries in my ACPI table, in the first entry, ngpios = 32 and
> > in the second entry ngpios = 24.
> >
> > Can you show the DSDT excerpt of this device? (Also including the pieces for
> > pin control)
>
> Our ACPI tables are internal:
>
> Device(GPI0) {
>         Name(_HID, "MLNXBF33")
>         Name(_UID, Zero)
>         Name(_CCA, 1)
>         Name(_CRS, ResourceTemplate() {
>           // for fw_gpio[0] yu block
>          Memory32Fixed(ReadWrite, 0x13401100, 0x00000080)
>          // for yu_gpio_causereg0 block
>          Memory32Fixed(ReadWrite, 0x13401c00, 0x00000020)
>          Interrupt(ResourceConsumer, Edge, ActiveHigh, Shared)
>            { BF_RSH0_YU }
>        })
>        Name(_DSD, Package() {
>          ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>          Package() {
>            Package () { "ngpios", 32 }, // Number of GPIO pins on gpio block 0
>            Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 3, 11, 31}},
>          }
>        })
>      }
>  Device(GPI1) {
>        Name(_HID, "MLNXBF33")
>        Name(_UID, 1)
>        Name(_CCA, 1)
>        Name(_CRS, ResourceTemplate() {
>          // for fw_gpio[1] yu block
>          Memory32Fixed(ReadWrite, 0x13401180, 0x00000080)
>          // for yu_gpio_causereg1 block
>          Memory32Fixed(ReadWrite, 0x13401c20, 0x00000020)
>        })
>        Name(_DSD, Package() {
>          ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>          Package() {
>            Package () { "ngpios", 24 }, // Number of GPIO pins on gpio block 1
>            Package () { "gpio-reserved-ranges", Package () {0, 21}},
>          }
>        })
>   }
>
>     Device(PIN0) {
>       Name(_HID, "MLNXBF34")
>       Name(_UID, Zero)
>       Name(_CCA, 1)
>       Name(_CRS, ResourceTemplate() {
>         // for fw_gpio[0] and fw_gpio[1] yu blocks
>         Memory32Fixed(ReadWrite, 0x13401100, 0x00000100)
>       })
>      }

So far I see no issues with the tables. Thanks for sharing!

> > > Gpiochip_add_data_with_keys only reads the ngpios property if (ngpios
> > > == 0) which is not the case when bgpio_init is called. bgpio_init uses "sz"
> > argument to populate the ngpio in bgpio_init, which is not what we want.
> >
> > Maybe bgpio_init() is not a good API for your case?
>
> At the moment, bgpio_init handles all the direction in/out get/set gpio value for us.
> So I can either remove bgpio_init so that we get the correct ngpios property, but will have to define get_gpio, set_gpio, dir in and dir out.
> Or I can keep bgpio_init and device_property_read_u32 in the gpio-mlxbf3.c driver.

So far it seems the issue is in bgpio_init() that doesn't handle
ngpios properly. Maybe we need to fix this first?

Btw, have you considered using the gpio-regmap approach?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Asmaa Mnebhi]

> > > > Gpiochip_add_data_with_keys only reads the ngpios property if
> > > > (ngpios == 0) which is not the case when bgpio_init is called. bgpio_init
> uses "sz"
> > > argument to populate the ngpio in bgpio_init, which is not what we want.
> > >
> > > Maybe bgpio_init() is not a good API for your case?
> >
> > At the moment, bgpio_init handles all the direction in/out get/set gpio value
> for us.
> > So I can either remove bgpio_init so that we get the correct ngpios
> property, but will have to define get_gpio, set_gpio, dir in and dir out.
> > Or I can keep bgpio_init and device_property_read_u32 in the gpio-
> mlxbf3.c driver.
> 
> So far it seems the issue is in bgpio_init() that doesn't handle ngpios properly.
> Maybe we need to fix this first?

Ok I will send a sperate patch shortly addressing this.

> Btw, have you considered using the gpio-regmap approach?

Thanks!  I see examples of drivers already using this (gpio-tn48m.c). Will take a look.

Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Andy Shevchenko]

On Fri, Feb 24, 2023 at 4:42 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

>            Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 3, 11, 31}},

Side note, doesn't it the same to

           Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 34}},

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Andy Shevchenko]

On Fri, Feb 24, 2023 at 5:13 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Feb 24, 2023 at 4:42 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> >            Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 3, 11, 31}},
>
> Side note, doesn't it the same to
>
>            Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 34}},
>
> ?

Ah, now I got it, the 10 is OK, while 7-9 and 11+ are not.

Sorry for the noise.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[kernel test robot]

Hi Asmaa,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next brgl/gpio/for-next linus/master v6.2-rc8 next-20230217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Asmaa-Mnebhi/pinctrl-pinctrl-mlxbf-Add-pinctrl-driver-support/20230218-062744
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link:    https://lore.kernel.org/r/28f0d670407c127614b64d9c382b11c795f5077d.1676668853.git.asmaa%40nvidia.com
patch subject: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support
reproduce:
        make versioncheck

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302180821.a8TRkiIS-lkp@intel.com/

versioncheck warnings: (new ones prefixed by >>)
   INFO PATH=/opt/cross/clang/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
   /usr/bin/timeout -k 100 3h /usr/bin/make W=1 --keep-going HOSTCC=gcc-11 CC=gcc-11 -j32 ARCH=x86_64 versioncheck
   find ./* \( -name SCCS -o -name BitKeeper -o -name .svn -o -name CVS -o -name .pc -o -name .hg -o -name .git \) -prune -o \
   	-name '*.[hcS]' -type f -print | sort \
   	| xargs perl -w ./scripts/checkversion.pl
   ./drivers/accessibility/speakup/genmap.c: 13 linux/version.h not needed.
   ./drivers/accessibility/speakup/makemapdata.c: 13 linux/version.h not needed.
>> ./drivers/gpio/gpio-mlxbf3.c: 14 linux/version.h not needed.
   ./drivers/net/ethernet/qlogic/qede/qede.h: 10 linux/version.h not needed.
   ./drivers/net/ethernet/qlogic/qede/qede_ethtool.c: 7 linux/version.h not needed.
   ./drivers/scsi/cxgbi/libcxgbi.h: 27 linux/version.h not needed.
   ./drivers/scsi/mpi3mr/mpi3mr.h: 32 linux/version.h not needed.
   ./drivers/scsi/qedi/qedi_dbg.h: 14 linux/version.h not needed.
   ./drivers/soc/tegra/cbb/tegra-cbb.c: 19 linux/version.h not needed.
   ./drivers/soc/tegra/cbb/tegra194-cbb.c: 26 linux/version.h not needed.
   ./drivers/soc/tegra/cbb/tegra234-cbb.c: 27 linux/version.h not needed.
   ./drivers/staging/media/atomisp/include/linux/atomisp.h: 23 linux/version.h not needed.
   ./init/version-timestamp.c: 5 linux/version.h not needed.
   ./samples/trace_events/trace_custom_sched.c: 11 linux/version.h not needed.
   ./sound/soc/codecs/cs42l42.c: 14 linux/version.h not needed.
   ./tools/lib/bpf/bpf_helpers.h: 289: need linux/version.h
   ./tools/perf/tests/bpf-script-example.c: 60: need linux/version.h
   ./tools/perf/tests/bpf-script-test-kbuild.c: 21: need linux/version.h
   ./tools/perf/tests/bpf-script-test-prologue.c: 47: need linux/version.h
   ./tools/perf/tests/bpf-script-test-relocation.c: 51: need linux/version.h
   ./tools/testing/selftests/bpf/progs/dev_cgroup.c: 9 linux/version.h not needed.
   ./tools/testing/selftests/bpf/progs/netcnt_prog.c: 3 linux/version.h not needed.
   ./tools/testing/selftests/bpf/progs/test_map_lock.c: 4 linux/version.h not needed.
   ./tools/testing/selftests/bpf/progs/test_send_signal_kern.c: 4 linux/version.h not needed.
   ./tools/testing/selftests/bpf/progs/test_spin_lock.c: 4 linux/version.h not needed.
   ./tools/testing/selftests/bpf/progs/test_tcp_estats.c: 37 linux/version.h not needed.
   ./tools/testing/selftests/wireguard/qemu/init.c: 27 linux/version.h not needed.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Linus Walleij]

Hi Asamaa,

thanks for your patch!

getting a lot better all the time.

On Fri, Feb 17, 2023 at 10:27 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> Add support for the BlueField-3 SoC GPIO driver.
> This driver configures and handles GPIO interrupts. It also enables a user
> to manipulate certain GPIO pins via libgpiod tools or other kernel drivers.
> The usables pins are defined via the gpio-reserved-ranges property.
>
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>

(...)
> +config GPIO_MLXBF3
> +       tristate "Mellanox BlueField 3 SoC GPIO"
> +       depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || (64BIT && COMPILE_TEST)
> +       select GPIO_GENERIC

select GPIOLIB_IRQCHIP

since you use it.

(...)
> +static void mlxbf3_gpio_irq_enable(struct irq_data *irqd)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
> +       int offset = irqd_to_hwirq(irqd);
> +       unsigned long flags;
> +       u32 val;
> +
> +       raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +       writel(BIT(offset), gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CLRCAUSE);
> +
> +       val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
> +       val |= BIT(offset);
> +       writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
> +       raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);

Here, at the end (*after* writing registers) call:

gpiochip_disable_irq(gc, offset);

> +static void mlxbf3_gpio_irq_disable(struct irq_data *irqd)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
> +       int offset = irqd_to_hwirq(irqd);
> +       unsigned long flags;
> +       u32 val;
> +

Here, at the beginning (*before* writing registers) call:

gpiochip_disable_irq(gc, offset);

> +       raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +       val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
> +       val &= ~BIT(offset);
> +       writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
> +       raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +}

(...)

> +static const struct irq_chip gpio_mlxbf3_irqchip = {
> +       .name = "MLNXBF33",
> +       .irq_set_type = mlxbf3_gpio_irq_set_type,
> +       .irq_enable = mlxbf3_gpio_irq_enable,
> +       .irq_disable = mlxbf3_gpio_irq_disable,

Like Andy said:

.flags = IRQCHIP_IMMUTABLE,
GPIOCHIP_IRQ_RESOURCE_HELPERS,

Apart from this it looks all right.

Yours,
Linus Walleij

[PATCH v4 2/2] pinctrl: pinctrl-mlxbf: Add pinctrl driver support

[Asmaa Mnebhi]

NVIDIA BlueField-3 SoC has a few pins that can be used as GPIOs
or take the default hardware functionality. Add a driver for
the pinmuxing.

Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 drivers/pinctrl/Kconfig         |  14 ++
 drivers/pinctrl/Makefile        |   1 +
 drivers/pinctrl/pinctrl-mlxbf.c | 338 ++++++++++++++++++++++++++++++++
 3 files changed, 353 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-mlxbf.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 7d5f5458c72e..900fa17635c9 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -523,6 +523,20 @@ config PINCTRL_ZYNQMP
 	  This driver can also be built as a module. If so, the module
 	  will be called pinctrl-zynqmp.
 
+config PINCTRL_MLXBF
+	tristate "NVIDIA BlueField-3 SoC Pinctrl driver"
+	depends on (MELLANOX_PLATFORM && ARM64) || (64BIT && COMPILE_TEST)
+	select PINMUX
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
+	select GPIO_MLXBF3
+	help
+	  Say Y to select the pinctrl driver for BlueField-3 SoCs.
+	  This pin controller works hand in hand with the gpio-mlxbf3
+	  driver and allows selecting the mux function for each
+	  pin. This driver can also be built as a module called
+	  pinctrl-mlxbf.
+
 source "drivers/pinctrl/actions/Kconfig"
 source "drivers/pinctrl/aspeed/Kconfig"
 source "drivers/pinctrl/bcm/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index d5939840bb2a..67252469e76b 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_PINCTRL_MCP23S08_I2C)	+= pinctrl-mcp23s08_i2c.o
 obj-$(CONFIG_PINCTRL_MCP23S08_SPI)	+= pinctrl-mcp23s08_spi.o
 obj-$(CONFIG_PINCTRL_MCP23S08)	+= pinctrl-mcp23s08.o
 obj-$(CONFIG_PINCTRL_MICROCHIP_SGPIO)	+= pinctrl-microchip-sgpio.o
+obj-$(CONFIG_PINCTRL_MLXBF)	+= pinctrl-mlxbf.o
 obj-$(CONFIG_PINCTRL_OCELOT)	+= pinctrl-ocelot.o
 obj-$(CONFIG_PINCTRL_OXNAS)	+= pinctrl-oxnas.o
 obj-$(CONFIG_PINCTRL_PALMAS)	+= pinctrl-palmas.o
diff --git a/drivers/pinctrl/pinctrl-mlxbf.c b/drivers/pinctrl/pinctrl-mlxbf.c
new file mode 100644
index 000000000000..95fc5b8f2dc7
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-mlxbf.c
@@ -0,0 +1,338 @@
+// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause
+/* Copyright (C) 2022 NVIDIA CORPORATION & AFFILIATES */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#define MLXBF_GPIO0_FW_CONTROL_SET   0
+#define MLXBF_GPIO0_FW_CONTROL_CLEAR 0x14
+#define MLXBF_GPIO1_FW_CONTROL_SET   0x80
+#define MLXBF_GPIO1_FW_CONTROL_CLEAR 0x94
+
+#define MLXBF_NGPIOS_GPIO0    32
+
+enum {
+	MLXBF_GPIO_HW_MODE,
+	MLXBF_GPIO_SW_MODE
+};
+
+struct mlxbf_pinctrl {
+	void __iomem *base;
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+	struct pinctrl_gpio_range gpio_range;
+};
+
+#define MLXBF_GPIO_RANGE(_id, _pinbase, _gpiobase, _npins)	\
+	{							\
+		.name = "mlxbf_gpio_range",			\
+		.id = _id,					\
+		.base = _gpiobase,				\
+		.pin_base = _pinbase,				\
+		.npins = _npins,				\
+	}
+
+static struct pinctrl_gpio_range mlxbf_pinctrl_gpio_ranges[] = {
+	MLXBF_GPIO_RANGE(0, 0,  480, 32),
+	MLXBF_GPIO_RANGE(1,  32, 456, 24),
+};
+
+static const struct pinctrl_pin_desc mlxbf_pins[] = {
+	PINCTRL_PIN(0, "gpio0"),
+	PINCTRL_PIN(1, "gpio1"),
+	PINCTRL_PIN(2, "gpio2"),
+	PINCTRL_PIN(3, "gpio3"),
+	PINCTRL_PIN(4, "gpio4"),
+	PINCTRL_PIN(5, "gpio5"),
+	PINCTRL_PIN(6, "gpio6"),
+	PINCTRL_PIN(7, "gpio7"),
+	PINCTRL_PIN(8, "gpio8"),
+	PINCTRL_PIN(9, "gpio9"),
+	PINCTRL_PIN(10, "gpio10"),
+	PINCTRL_PIN(11, "gpio11"),
+	PINCTRL_PIN(12, "gpio12"),
+	PINCTRL_PIN(13, "gpio13"),
+	PINCTRL_PIN(14, "gpio14"),
+	PINCTRL_PIN(15, "gpio15"),
+	PINCTRL_PIN(16, "gpio16"),
+	PINCTRL_PIN(17, "gpio17"),
+	PINCTRL_PIN(18, "gpio18"),
+	PINCTRL_PIN(19, "gpio19"),
+	PINCTRL_PIN(20, "gpio20"),
+	PINCTRL_PIN(21, "gpio21"),
+	PINCTRL_PIN(22, "gpio22"),
+	PINCTRL_PIN(23, "gpio23"),
+	PINCTRL_PIN(24, "gpio24"),
+	PINCTRL_PIN(25, "gpio25"),
+	PINCTRL_PIN(26, "gpio26"),
+	PINCTRL_PIN(27, "gpio27"),
+	PINCTRL_PIN(28, "gpio28"),
+	PINCTRL_PIN(29, "gpio29"),
+	PINCTRL_PIN(30, "gpio30"),
+	PINCTRL_PIN(31, "gpio31"),
+	PINCTRL_PIN(32, "gpio32"),
+	PINCTRL_PIN(33, "gpio33"),
+	PINCTRL_PIN(34, "gpio34"),
+	PINCTRL_PIN(35, "gpio35"),
+	PINCTRL_PIN(36, "gpio36"),
+	PINCTRL_PIN(37, "gpio37"),
+	PINCTRL_PIN(38, "gpio38"),
+	PINCTRL_PIN(39, "gpio39"),
+	PINCTRL_PIN(40, "gpio40"),
+	PINCTRL_PIN(41, "gpio41"),
+	PINCTRL_PIN(42, "gpio42"),
+	PINCTRL_PIN(43, "gpio43"),
+	PINCTRL_PIN(44, "gpio44"),
+	PINCTRL_PIN(45, "gpio45"),
+	PINCTRL_PIN(46, "gpio46"),
+	PINCTRL_PIN(47, "gpio47"),
+	PINCTRL_PIN(48, "gpio48"),
+	PINCTRL_PIN(49, "gpio49"),
+	PINCTRL_PIN(50, "gpio50"),
+	PINCTRL_PIN(51, "gpio51"),
+	PINCTRL_PIN(52, "gpio52"),
+	PINCTRL_PIN(53, "gpio53"),
+	PINCTRL_PIN(54, "gpio54"),
+	PINCTRL_PIN(55, "gpio55"),
+};
+
+/*
+ * All single-pin functions can be mapped to any GPIO, however pinmux applies
+ * functions to pin groups and only those groups declared as supporting that
+ * function. To make this work we must put each pin in its own dummy group so
+ * that the functions can be described as applying to all pins.
+ * We use the same name as in the datasheet.
+ */
+static const char * const mlxbf_pinctrl_single_group_names[] = {
+	"gpio0", "gpio1",  "gpio2",  "gpio3",  "gpio4",  "gpio5",  "gpio6",
+	"gpio7", "gpio8",  "gpio9",  "gpio10", "gpio11", "gpio12", "gpio13",
+	"gpio14", "gpio15", "gpio16", "gpio17", "gpio18", "gpio19", "gpio20",
+	"gpio21", "gpio22", "gpio23", "gpio24", "gpio25", "gpio26", "gpio27",
+	"gpio28", "gpio29", "gpio30", "gpio31", "gpio32", "gpio33", "gpio34",
+	"gpio35", "gpio36", "gpio37", "gpio38", "gpio39", "gpio40", "gpio41",
+	"gpio42", "gpio43", "gpio44", "gpio45", "gpio46", "gpio47", "gpio48",
+	"gpio49", "gpio50", "gpio51", "gpio52", "gpio53", "gpio54", "gpio55"
+};
+
+/* Set of pin numbers for single-pin groups */
+static const unsigned int mlxbf_pinctrl_single_group_pins[] = {
+	0,  1,  2,  3,  4,  5,  6,
+	7,  8,  9, 10, 11, 12, 13,
+	14, 15, 16, 17, 18, 19, 20,
+	21, 22, 23, 24, 25, 26, 27,
+	28, 29, 30, 31, 32, 33, 34,
+	35, 36, 37, 38, 39, 40, 41,
+	42, 43, 44, 45, 46, 47, 48,
+	49, 50, 51, 52, 53, 54, 55,
+};
+
+static int mlxbf_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	/* Number single-pin groups */
+	return ARRAY_SIZE(mlxbf_pinctrl_single_group_pins);
+}
+
+static const char *mlxbf_get_group_name(struct pinctrl_dev *pctldev,
+					 unsigned int selector)
+{
+	return mlxbf_pinctrl_single_group_names[selector];
+}
+
+static int mlxbf_get_group_pins(struct pinctrl_dev *pctldev,
+				 unsigned int selector,
+				 const unsigned int **pins,
+				 unsigned int *num_pins)
+{
+	/* return the dummy group for a single pin */
+	*pins = &mlxbf_pinctrl_single_group_pins[selector];
+	*num_pins = 1;
+
+	return 0;
+}
+
+static const struct pinctrl_ops mlxbf_pinctrl_group_ops = {
+	.get_groups_count = mlxbf_get_groups_count,
+	.get_group_name = mlxbf_get_group_name,
+	.get_group_pins = mlxbf_get_group_pins,
+};
+
+static const char * const mlxbf_gpiofunc_group_names[] = { "swctrl" };
+static const char * const mlxbf_hwfunc_group_names[]   = { "hwctrl" };
+
+/*
+ * Only 2 functions are supported and they apply to all pins:
+ * 1) Default hardware functionality
+ * 2) Software controlled GPIO
+ */
+static const struct {
+	const char *name;
+	const char * const *group_names;
+} mlxbf_pmx_funcs[] = {
+	{
+		.name = "hwfunc",
+		.group_names = mlxbf_hwfunc_group_names
+	},
+	{
+		.name = "gpiofunc",
+		.group_names = mlxbf_gpiofunc_group_names
+	},
+};
+
+static int mlxbf_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(mlxbf_pmx_funcs);
+}
+
+static const char *mlxbf_pmx_get_func_name(struct pinctrl_dev *pctldev,
+					   unsigned int selector)
+{
+	return mlxbf_pmx_funcs[selector].name;
+}
+
+static int mlxbf_pmx_get_groups(struct pinctrl_dev *pctldev,
+				 unsigned int selector,
+				 const char * const **groups,
+				 unsigned int * const num_groups)
+{
+	*groups = mlxbf_pmx_funcs[selector].group_names;
+	*num_groups = ARRAY_SIZE(mlxbf_pinctrl_single_group_pins);
+
+	return 0;
+}
+
+static int mlxbf_pmx_set(struct pinctrl_dev *pctldev,
+			      unsigned int selector,
+			      unsigned int group)
+{
+	struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	if (selector == MLXBF_GPIO_HW_MODE) {
+		if (group < MLXBF_NGPIOS_GPIO0)
+			writel(BIT(group), priv->base + MLXBF_GPIO0_FW_CONTROL_CLEAR);
+		else
+			writel(BIT(group % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_CLEAR);
+	}
+
+	if (selector == MLXBF_GPIO_SW_MODE) {
+		if (group < MLXBF_NGPIOS_GPIO0)
+			writel(BIT(group), priv->base + MLXBF_GPIO0_FW_CONTROL_SET);
+		else
+			writel(BIT(group % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_SET);
+	}
+
+	return 0;
+}
+
+static int mlxbf_gpio_request_enable(struct pinctrl_dev *pctldev,
+				     struct pinctrl_gpio_range *range,
+				     unsigned int offset)
+{
+	struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	if (offset < MLXBF_NGPIOS_GPIO0)
+		writel(BIT(offset), priv->base + MLXBF_GPIO0_FW_CONTROL_SET);
+	else
+		writel(BIT(offset % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_SET);
+
+	return 0;
+}
+
+static void mlxbf_gpio_disable_free(struct pinctrl_dev *pctldev,
+				    struct pinctrl_gpio_range *range,
+				    unsigned int offset)
+{
+	struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	/* disable GPIO functionality by giving control back to hardware */
+	if (offset < MLXBF_NGPIOS_GPIO0)
+		writel(BIT(offset), priv->base + MLXBF_GPIO0_FW_CONTROL_CLEAR);
+	else
+		writel(BIT(offset % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_CLEAR);
+
+}
+
+static const struct pinmux_ops mlxbf_pmx_ops = {
+	.get_functions_count = mlxbf_pmx_get_funcs_count,
+	.get_function_name = mlxbf_pmx_get_func_name,
+	.get_function_groups = mlxbf_pmx_get_groups,
+	.set_mux = mlxbf_pmx_set,
+	.gpio_request_enable = mlxbf_gpio_request_enable,
+	.gpio_disable_free = mlxbf_gpio_disable_free,
+};
+
+static struct pinctrl_desc mlxbf_pin_desc = {
+	.name = "pinctrl-mlxbf",
+	.pins = mlxbf_pins,
+	.npins = ARRAY_SIZE(mlxbf_pins),
+	.pctlops = &mlxbf_pinctrl_group_ops,
+	.pmxops = &mlxbf_pmx_ops,
+	.owner = THIS_MODULE,
+};
+
+static_assert(ARRAY_SIZE(mlxbf_pinctrl_single_group_names) ==
+	      ARRAY_SIZE(mlxbf_pinctrl_single_group_pins));
+
+static int mlxbf_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mlxbf_pinctrl *priv;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+
+	/* This resource is shared so use devm_ioremap */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	priv->base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!priv->base)
+		return -ENOMEM;
+
+	ret = devm_pinctrl_register_and_init(priv->dev,
+					     &mlxbf_pin_desc,
+					     priv,
+					     &priv->pctl);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
+
+	ret = pinctrl_enable(priv->pctl);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable pinctrl\n");
+
+	pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[0]);
+	pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[1]);
+
+	return 0;
+}
+
+static const struct acpi_device_id mlxbf_pinctrl_acpi_ids[] = {
+	{ "MLNXBF34", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, mlxbf_pinctrl_acpi_ids);
+
+static struct platform_driver mlxbf_pinctrl_driver = {
+	.driver = {
+		.name = "pinctrl-mlxbf",
+		.acpi_match_table = mlxbf_pinctrl_acpi_ids,
+	},
+	.probe = mlxbf_pinctrl_probe,
+};
+module_platform_driver(mlxbf_pinctrl_driver);
+
+MODULE_DESCRIPTION("NVIDIA pinctrl driver");
+MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.30.1

Re: [PATCH v4 2/2] pinctrl: pinctrl-mlxbf: Add pinctrl driver support

[Andy Shevchenko]

On Fri, Feb 17, 2023 at 11:27 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> NVIDIA BlueField-3 SoC has a few pins that can be used as GPIOs
> or take the default hardware functionality. Add a driver for
> the pinmuxing.

pin muxing

...

> +++ b/drivers/pinctrl/pinctrl-mlxbf.c

Wondering if it would be better to match the GPIO driver naming
schema, i.e. by adding 3. In this case the additional explanation in
Kconfig help won't be necessary.

...

> +#define MLXBF_GPIO0_FW_CONTROL_SET   0
> +#define MLXBF_GPIO0_FW_CONTROL_CLEAR 0x14
> +#define MLXBF_GPIO1_FW_CONTROL_SET   0x80
> +#define MLXBF_GPIO1_FW_CONTROL_CLEAR 0x94

Unclear if these are commands or register offsets. If they are of the
same type (semantically), make them fixed width, e.g., 0x00.

...

> +enum {
> +       MLXBF_GPIO_HW_MODE,
> +       MLXBF_GPIO_SW_MODE

I would leave a comma here as it might be extended in the future.

> +};

...

> +static const char * const mlxbf_pinctrl_single_group_names[] = {
> +       "gpio0", "gpio1",  "gpio2",  "gpio3",  "gpio4",  "gpio5",  "gpio6",
> +       "gpio7", "gpio8",  "gpio9",  "gpio10", "gpio11", "gpio12", "gpio13",
> +       "gpio14", "gpio15", "gpio16", "gpio17", "gpio18", "gpio19", "gpio20",
> +       "gpio21", "gpio22", "gpio23", "gpio24", "gpio25", "gpio26", "gpio27",
> +       "gpio28", "gpio29", "gpio30", "gpio31", "gpio32", "gpio33", "gpio34",
> +       "gpio35", "gpio36", "gpio37", "gpio38", "gpio39", "gpio40", "gpio41",
> +       "gpio42", "gpio43", "gpio44", "gpio45", "gpio46", "gpio47", "gpio48",
> +       "gpio49", "gpio50", "gpio51", "gpio52", "gpio53", "gpio54", "gpio55"

Ditto.
Can you group by 8?

> +};
> +
> +/* Set of pin numbers for single-pin groups */
> +static const unsigned int mlxbf_pinctrl_single_group_pins[] = {
> +       0,  1,  2,  3,  4,  5,  6,
> +       7,  8,  9, 10, 11, 12, 13,
> +       14, 15, 16, 17, 18, 19, 20,
> +       21, 22, 23, 24, 25, 26, 27,
> +       28, 29, 30, 31, 32, 33, 34,
> +       35, 36, 37, 38, 39, 40, 41,
> +       42, 43, 44, 45, 46, 47, 48,
> +       49, 50, 51, 52, 53, 54, 55,

Group by 8 which is the more natural length of subarray per line.

Is it just 1:1 to the index? If so, why do you need this table at all?

> +};

...

> +static const struct {
> +       const char *name;
> +       const char * const *group_names;

Use this instead
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215
and this
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222

> +} mlxbf_pmx_funcs[] = {

> +};

...

> +{
> +       struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
> +
> +       /* disable GPIO functionality by giving control back to hardware */
> +       if (offset < MLXBF_NGPIOS_GPIO0)
> +               writel(BIT(offset), priv->base + MLXBF_GPIO0_FW_CONTROL_CLEAR);
> +       else
> +               writel(BIT(offset % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_CLEAR);

> +

Redundant blank line.

> +}

...

> +static_assert(ARRAY_SIZE(mlxbf_pinctrl_single_group_names) ==
> +             ARRAY_SIZE(mlxbf_pinctrl_single_group_pins));

I would put it on a single line, but it's up to you.

...

> +       struct resource *res;

Useless?

...

> +       /* This resource is shared so use devm_ioremap */

Can you elaborate on who actually requests the region? And why is it
not _this_ driver?

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return -ENODEV;

...

> +       ret = devm_pinctrl_register_and_init(priv->dev,

Is the priv->dev different from dev?

> +                                            &mlxbf_pin_desc,
> +                                            priv,
> +                                            &priv->pctl);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to register pinctrl\n");

...

> +       pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[0]);
> +       pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[1]);

pinctrl_add_gpio_ranges() ?

--
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 2/2] pinctrl: pinctrl-mlxbf: Add pinctrl driver support

[Asmaa Mnebhi]

> > +static const struct {
> > +       const char *name;
> > +       const char * const *group_names;
> 
> Use this instead
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-
> pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215
> and this
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-
> pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222
> 
> > +} mlxbf_pmx_funcs[] = {
> 
> > +};

so copy that struct definition and macro to my driver? (I don’t see these code changes in master)  

> 
> > +       /* This resource is shared so use devm_ioremap */
> 
> Can you elaborate on who actually requests the region? And why is it not
> _this_ driver?

This resource is shared with the gpio-mlxbf3.c driver. The gpio-mlxbf3.c driver does not access the same offsets as the pinctrl-mlxbf3.c driver, but it accesses several other registers offsets in between.

Re: [PATCH v4 2/2] pinctrl: pinctrl-mlxbf: Add pinctrl driver support

[Andy Shevchenko]

On Thu, Feb 23, 2023 at 11:07 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> > > +static const struct {
> > > +       const char *name;
> > > +       const char * const *group_names;
> >
> > Use this instead
> > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-
> > pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215
> > and this
> > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-
> > pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222
> >
> > > +} mlxbf_pmx_funcs[] = {
> >
> > > +};
>
> so copy that struct definition and macro to my driver? (I don’t see these code changes in master)

Which master?

First of all, you should do your development based on the "for-next"
of the respective subsystem (okay, for pin control Linus Walleij
called his published branch "devel"). So, the above mentioned
functionality was there a while ago.

Second, a couple of days ago Linus Torvalds pulled PR, so it's part of
upstream now.

TL;DR: just use those types and macros in your code.

> > > +       /* This resource is shared so use devm_ioremap */
> >
> > Can you elaborate on who actually requests the region? And why is it not
> > _this_ driver?
>
> This resource is shared with the gpio-mlxbf3.c driver. The gpio-mlxbf3.c driver does not access the same offsets as the pinctrl-mlxbf3.c driver, but it accesses several other registers offsets in between.

Okay, so in such a case you need a common denominator that actually
does this all for you via, for example, regmap. If the region is not
requested, bad things may happen.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 2/2] pinctrl: pinctrl-mlxbf: Add pinctrl driver support

[Asmaa Mnebhi]

 
> > > > +       /* This resource is shared so use devm_ioremap */
> > >
> > > Can you elaborate on who actually requests the region? And why is it
> > > not _this_ driver?
> >
> > This resource is shared with the gpio-mlxbf3.c driver. The gpio-mlxbf3.c
> driver does not access the same offsets as the pinctrl-mlxbf3.c driver, but it
> accesses several other registers offsets in between.
> 
> Okay, so in such a case you need a common denominator that actually does
> this all for you via, for example, regmap. If the region is not requested, bad
> things may happen.

Hi Andy,

I have taken a look at the regmap driver and decided we will maintain the
Current implementation with devm_platform_ioremap_resource.
The main reason is for backward compatibility with older kernels. Although we
Do support the latest kernel, we have some customers who are still using old
Kernels which don’t have regmap support. But I have found a solution which
Involves using  devm_platform_ioremap_resource here as you initially
suggested.
patch v5 coming soon!

Thanks.
Asmaa

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

[Linus Walleij]

On Fri, Feb 10, 2023 at 4:03 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
>
> 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>

Excellent work Asmaa,
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

RE: [PATCH v3 1/2] Support NVIDIA BlueField-3 GPIO controller

[Asmaa Mnebhi]

> 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>

Excellent work Asmaa,
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thank you very much Linus!
Andy added few comments which I will address shortly in v4.