Re: [PATCH] gpio:gpio-amdpt:add new device and that 24-pin support

[Andy Shevchenko]

On Tue, Dec 07, 2021 at 05:42:39PM +0800, Richard Hsu wrote:
> From: RichardHsu <Richard_Hsu@xxxxxxxxxxxxxx>

Thanks for an update, my comments below.

First of all, don't forget versioning of the patch (in the Subject it should
be PATCH v2). Besides that, don't forget to include changelog (see below).

Subject should be:

	gpio: amdpt: add new device ID and 24-pin support

> New ACPI gpio device(AMDIF031) support 24 gpio pins. We add new device id and pin number to .driver_data of acpi_device_id structure
> and then retrieve it by device_get_match_data() that Andy suggest it.

Please, make sure your text is wrapped at ~72-75 characters.

> Signed-off-by: RichardHsu <Richard_Hsu@xxxxxxxxxxxxxx>

Is it name out of one work like this? Otherwise put your Real Name here.

> ---

Changelog should be here, after '--- ' cutter line.

>  drivers/gpio/gpio-amdpt.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-amdpt.c b/drivers/gpio/gpio-amdpt.c
> index bbf53e289141..a45693423a07 100644
> --- a/drivers/gpio/gpio-amdpt.c
> +++ b/drivers/gpio/gpio-amdpt.c
> @@ -14,6 +14,7 @@
>  #include <linux/platform_device.h>
> 
>  #define PT_TOTAL_GPIO 8
> +#define PT_TOTAL_GPIO_EX 24
> 
>  /* PCI-E MMIO register offsets */
>  #define PT_DIRECTION_REG   0x00
> @@ -103,7 +104,8 @@ static int pt_gpio_probe(struct platform_device *pdev)
>  	pt_gpio->gc.owner            = THIS_MODULE;
>  	pt_gpio->gc.request          = pt_gpio_request;
>  	pt_gpio->gc.free             = pt_gpio_free;
> -	pt_gpio->gc.ngpio            = PT_TOTAL_GPIO;

> +	/* retrieve pin number from .driver_data of acpi_device_id structure */

Everybody understands or can easily get what the below API call does. No need
to have a comment.

> +	pt_gpio->gc.ngpio            = (uintptr_t)device_get_match_data(dev);
>  #if defined(CONFIG_OF_GPIO)
>  	pt_gpio->gc.of_node          = dev->of_node;
>  #endif
> @@ -133,8 +135,9 @@ static int pt_gpio_remove(struct platform_device *pdev)
>  }
> 
>  static const struct acpi_device_id pt_gpio_acpi_match[] = {
> -	{ "AMDF030", 0 },
> -	{ "AMDIF030", 0 },
> +	{ "AMDF030", PT_TOTAL_GPIO },
> +	{ "AMDIF030", PT_TOTAL_GPIO },
> +	{ "AMDIF031", PT_TOTAL_GPIO_EX },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, pt_gpio_acpi_match);

The code itself looks good!

-- 
With Best Regards,
Andy Shevchenko