Re: [PATCH v1 v/1] gpio: mlxbf: enable GPIO interface and ACPI event for host-gpio[7]

[Andy Shevchenko]

On Fri, Feb 03, 2023 at 01:49:07PM -0500, David Thompson wrote:
> This commit adds ACPI handling for host-gpio[7] to trigger
> the power-button event.

This has something to discuss...

...

>  /* Pad Electrical Controls. */
> -#define MLXBF_GPIO_PAD_CONTROL_FIRST_WORD 0x0700
> -#define MLXBF_GPIO_PAD_CONTROL_1_FIRST_WORD 0x0708
> -#define MLXBF_GPIO_PAD_CONTROL_2_FIRST_WORD 0x0710
> -#define MLXBF_GPIO_PAD_CONTROL_3_FIRST_WORD 0x0718
> +#define MLXBF_GPIO_PAD_CONTROL_FIRST_WORD	0x0700
> +#define MLXBF_GPIO_PAD_CONTROL_1_FIRST_WORD	0x0708
> +#define MLXBF_GPIO_PAD_CONTROL_2_FIRST_WORD	0x0710
> +#define MLXBF_GPIO_PAD_CONTROL_3_FIRST_WORD	0x0718

Unrelated change.

> +#define MLXBF_GPIO_PIN_DIR_I                    0x1040
> +#define MLXBF_GPIO_PIN_DIR_O                    0x1048
> +#define MLXBF_GPIO_PIN_STATE                    0x1000
> +#define MLXBF_GPIO_SCRATCHPAD                   0x20

Ditto. You already have it defined, why to change?

> -#define MLXBF_GPIO_PIN_DIR_I 0x1040
> -#define MLXBF_GPIO_PIN_DIR_O 0x1048
> -#define MLXBF_GPIO_PIN_STATE 0x1000
> -#define MLXBF_GPIO_SCRATCHPAD 0x20

...

> +	gs->hwirq = irq;
> +	gc->to_irq = mlxbf_gpio_to_irq;

This sounds incorrect. Seems like for _any_ interrupt you will give the same
result. Moreover, you should not use to_irq(). Try to model proper IRQ chip.

...

>  	platform_set_drvdata(pdev, gs);
> +	acpi_gpiochip_request_interrupts(gc);

This is done by GPIO library, no?

...

>  	dev_info(&pdev->dev, "registered Mellanox BlueField GPIO");
> +
>  	return 0;

Stray change.

-- 
With Best Regards,
Andy Shevchenko