Hello Tomas,
On 09.04.24 09:14, Tomas Marek wrote:
> Signed-off-by: Tomas Marek <tomas.marek@xxxxxxxxx>
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 |
