mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: Tomas Marek <tomas.marek@elrest.cz>, barebox@lists.infradead.org
Subject: Re: [PATCH] gpio: Add Intel gpio controller support
Date: Tue, 9 Apr 2024 09:41:28 +0200	[thread overview]
Message-ID: <46db4b10-73d7-4f33-9fc9-e383326bfe5d@pengutronix.de> (raw)
In-Reply-To: <20240409071422.5934-1-tomas.marek@elrest.cz>

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 |




  reply	other threads:[~2024-04-09  7:42 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-09  7:14 Tomas Marek
2024-04-09  7:41 ` Ahmad Fatoum [this message]
2024-04-10  7:39   ` Tomas Marek
2024-04-10  8:37     ` Ahmad Fatoum

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=46db4b10-73d7-4f33-9fc9-e383326bfe5d@pengutronix.de \
    --to=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=tomas.marek@elrest.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox