Date:   Thu, 9 Feb 2023 23:59:53 +0200
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     David Thompson <davthompson@nvidia.com>
Cc:     linus.walleij@linaro.org, brgl@bgdev.pl,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        limings@nvidia.com
Subject: Re: [PATCH v1 v/1] gpio: mlxbf: enable GPIO interface and ACPI event
 for host-gpio[7]
Message-ID: <Y+Vs2WxqXVQmh4+z@smile.fi.intel.com>
References: <20230203184907.18786-1-davthompson@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230203184907.18786-1-davthompson@nvidia.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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