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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y+Vs2WxqXVQmh4+z@smile.fi.intel.com>
Date:   Thu, 9 Feb 2023 23:59:53 +0200
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     David Thompson <davthompson@...dia.com>
Cc:     linus.walleij@...aro.org, brgl@...ev.pl,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        limings@...dia.com
Subject: Re: [PATCH v1 v/1] gpio: mlxbf: enable GPIO interface and ACPI event
 for host-gpio[7]

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