Re: [PATCH] gpio: Add Intel gpio controller support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Tomas,

On 09.04.24 09:14, Tomas Marek wrote:
> Signed-off-by: Tomas Marek <tomas.marek@elrest.cz>

Thanks for your patch. I have a soft spot for barebox-as-efi-payload,
so it's cool to see you contributing new features.

It also makes me curious what more drivers are you intending to
contribute. :-)

Some review below. 

> ---
>  drivers/gpio/Kconfig               |   5 +
>  drivers/gpio/Makefile              |   1 +
>  drivers/gpio/gpio-intel.c          | 198 +++++++++++++++++++++++++++++
>  include/platform_data/gpio-intel.h |  10 ++
>  4 files changed, 214 insertions(+)
>  create mode 100644 drivers/gpio/gpio-intel.c
>  create mode 100644 include/platform_data/gpio-intel.h
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 9f27addaa2..094c9b7fd4 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -219,6 +219,11 @@ config GPIO_LATCH
>  	  Say yes here to enable a driver for GPIO multiplexers based on latches
>  	  connected to other GPIOs.
>  
> +config GPIO_INTEL
> +	tristate "Intel GPIO driver"

Please add a depends on X86 || COMPILE_TEST here, so other architectures
aren't prompted for this driver by default.

> +	help
> +	  Say Y or M here to build support for the Intel GPIO driver.

Nitpick: We only have [M]odule support for ARM, so tristate == bool in your
case and one couldn't set M here, despite what the help text suggests.

> +static int intel_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> +{
> +	struct intel_gpio_chip *chip = to_intel_gpio(gc);
> +	u32 padcfg0;
> +
> +	padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
> +
> +	if (padcfg0 & PADCFG0_PMODE_MASK)
> +		return -EINVAL;
> +
> +	if (padcfg0 & PADCFG0_GPIOTXDIS)
> +		return GPIOF_DIR_IN;
> +
> +	return GPIOF_DIR_IN;

Your never return GPIOF_DIR_OUT. Is this intended?

> +	ret = gpiochip_add(&intel_gpio->chip);
> +
> +	if (ret) {
> +		dev_err(dev, "Couldn't add gpiochip: %d\n", ret);

Nitpick: %pe\n", ERR_PTR(ret)

> +		kfree(intel_gpio);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct driver_d intel_gpio_driver = {
> +	.name = "intel-gpio",
> +	.probe = intel_gpio_probe,
> +};
> +
> +coredevice_platform_driver(intel_gpio_driver);

Who will register this device? Is it possible to add an ACPI table match
(like itco_wdt does for example) for your SoC and then register the device
there like Linux does?

This would make extension for more SoCs easier in future.

> diff --git a/include/platform_data/gpio-intel.h b/include/platform_data/gpio-intel.h
> new file mode 100644
> index 0000000000..f04baadd4d
> --- /dev/null
> +++ b/include/platform_data/gpio-intel.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __GPIO_INTEL_H
> +#define __GPIO_INTEL_H
> +
> +struct gpio_intel_platform_data {
> +	unsigned int	ngpios;
> +};

I'd suggest you add a add_intel_gpio_device helper here that would create a suitable
device. This could be then called from the ACPI driver probe or from board code if
discoverability is not possible.


Cheers,
Ahmad

> +
> +#endif /* __GPIO_INTEL_H */

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |