RE: [PATCH v1 1/1] Add driver for Mellanox BlueField-3 GPIO controller

[Asmaa Mnebhi <asmaa@xxxxxxxxxx>]

Hi Linus,

Sorry for the long delay. I did work on adding the pinctrl driver besides the gpio driver and I am working on testing it.
I added the following to the gpio driver:
gc->set = mlxbf3_gpio_set;
gc->direction_input = mlxbf3_gpio_direction_input;
gc->direction_output = mlxbf3_gpio_direction_output;
gc->request = gpiochip_generic_request;
gc->free = gpiochip_generic_free;

In the pinctrl driver, I defined the following:
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,
};

During testing, I use the sysfs to change the gpio value as follows:
Cd /sys/class/gpio
echo 480 > export
When I do the export, I see that gpiochip_generic_request is being called which calls .gpio_request_enable = mlxbf_gpio_request_enable.

Is this how it also works in other driver? Or am I missing something? 
I wanted to disallow muxing from user space. I would like that to be controlled by the ACPI table only. For example, use devm_gpio_request from some other driver if needed.

Thanks.
Asmaa
-----Original Message-----
From: Linus Walleij <linus.walleij@xxxxxxxxxx> 
Sent: Wednesday, May 18, 2022 9:51 AM
To: Asmaa Mnebhi <asmaa@xxxxxxxxxx>
Cc: andy.shevchenko@xxxxxxxxx; bgolaszewski@xxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v1 1/1] Add driver for Mellanox BlueField-3 GPIO controller
Importance: High

On Mon, May 16, 2022 at 3:00 PM Asmaa Mnebhi <asmaa@xxxxxxxxxx> wrote:

> So these GPIO pins are assigned one specific HW functionality on the 
> boards  and software should never change them.
>
> By default, for security purposes, I think we shouldn't let the user 
> have the option to control the GPIO pins since they have a specific HW functionality.
>
> But for bringup/debug purposes, we would like to support the option of 
> software being able to change these pin values. We also might have 
> customers that choose to change the default HW connection of a certain 
> GPIO pin and connect it to control their LEDs for instance.

The fact that the usecase is bringup/debug does not mean we cut corners and do "quick fixes". The proper APIs have to be implemented, the alternative is to not submit the driver at all.

What I hear is that these pins have two modes:

1. Used for a device (I2C etc)
2. Used as GPIO by setting a bit in YU_GPIO_FW_CONTROL_SET

This is two pin control multiplexing states already.

So this should have a simple pin control driver as back-end with the GPIO as front end. A shortcut to enabling pins into GPIO mode can be provided using .gpio_request_enable() from struct pinmux_ops.

Please refer to
https://docs.kernel.org/driver-api/pin-control.html

I know this means more work and is kind of complex. But drivers/pinctrl has a lot of examples you can follow, for example drivers/pinctrl/pinctrl-sx150x.c and other simple multipurpose chips.

Yours,
Linus Walleij