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

[Asmaa Mnebhi <asmaa@xxxxxxxxxx>]

-----Original Message-----
From: Linus Walleij <linus.walleij@xxxxxxxxxx> 
Sent: Thursday, May 5, 2022 11:12 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

> Could you please have a look at this patch?

Sure!

> +static void mlxbf3_gpio_set(struct gpio_chip *chip, unsigned int 
> +offset, int val) {

Put opening bracket on new line.
Run your code through scripts/checkpatch.pl before submitting.

It is odd that it got sent this way in the email. In my patch and my local file, the bracket is aligned correctly as follows:
 +static void mlxbf3_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)
 +{

I did run checkpatch.pl before sending this. I will resend the email , hopefully it will be sent properly this time.

> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +
> +       /* Software can only control GPIO pins defined by ctrl_gpio_mask */
> +       if (!(BIT(offset) & gs->ctrl_gpio_mask))
> +               return;
> +
> +       if (val)
> +               writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_DATA_OUT_SET);
> +       else
> +               writel(BIT(offset), gs->gpio_io + 
> + YU_GPIO_FW_DATA_OUT_CLEAR);
> +
> +       /* Make sure all previous writes are done before changing YU_GPIO_FW_CONTROL_SET */
> +       wmb();
> +
> +       /* This needs to be done last to avoid glitches */
> +       writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_CONTROL_SET); }

Bracket on new line. This coding style is very odd.

Same comment as above. Not sure why the email prints it this way.

The hardware is a bit odd too but I see why you can't use GPIO_GENERIC properly with this FW_CONTROL_SET business :/

> +static int mlxbf3_gpio_direction_input(struct gpio_chip *chip,
> +                                      unsigned int offset) {
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_OUTPUT_ENABLE_CLEAR);
> +       writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_CONTROL_CLEAR);
> +
> +       spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}
> +
> +static int mlxbf3_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned int offset,
> +                                       int value) {
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + 
> + YU_GPIO_FW_OUTPUT_ENABLE_SET);
> +
> +       spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}

Why isn't FW_CONTROL_CLEAR/SET used when making  a line into an output? Seems unsymmetric? At least put a comment in the code why this is different from making a line into input.

Ok I will add a comment.

All GPIOs are always controlled by HW by default.
FW_CONTROL_SET is used when we want to release control of the GPIO by HW and give the control to software instead. We want to give control to software only after changing the value of a GPIO pin to avoid any glitches in the HW, so this is the only case where YU_GPIO_FW_CONTROL_SET is set. This is why we don’t do it in mlxbf3_gpio_direction_output. Anyways, the user cannot set a gpio pin to a certain value from linux, if they haven't changed the direction of the gpio to out in the first place. 
It is a bit an odd implementation but basically even if we set this bit in mlxbf3_gpio_direction_output:
writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_OUTPUT_ENABLE_SET);
It doesn’t actually take effect until we also set the corresponding bit in YU_GPIO_FW_CONTROL_SET. But it was advised by our HW team to set YU_GPIO_FW_CONTROL_SET only after setting the desired GPIO value to avoid HW glitches.
 

> +       /* To set the direction to input, just give control to HW by setting
> +        * YU_GPIO_FW_CONTROL_CLEAR.
> +        * If the GPIO is controlled by HW, read its value via read_data_in register.
> +        *
> +        * When the direction = output, the GPIO is controlled by SW and
> +        * datain=dataout. If software modifies the value of the GPIO pin,
> +        * the value can be read from read_data_in without changing the direction.
> +        */
> +       ret = bgpio_init(gc, dev, 4,
> +                       gs->gpio_io + YU_GPIO_READ_DATA_IN,
> +                       NULL,
> +                       NULL,
> +                       NULL,
> +                       NULL,
> +                       0);

Hm. Is it still worth it?

MVH
Linus Walleij