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

[Tomas Marek <tomas.marek@elrest.cz>]

Hello Ahmad,

many thanks for the review.

On Tue, Apr 09, 2024 at 09:41:28AM +0200, Ahmad Fatoum wrote:
> 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. :-)

Nice to hear someone is interested in :-).

> 
> 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.

Make sense, I’ll do so.

> 
> > +	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.

I understand. I will change 'tristate' to 'bool' and update the help
to avoid any confusion.

> 
> > +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?

Silly me. No, it was not intentional; it's a mistake. The last statement
should return GPIOF_DIR_OUT. Thank you for pointing that out. I'll fix it.

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

Thanks for hit.

> 
> > +		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.

I have registered the device in the board code now. In theory, it is
definitely possible to register the device using ACPI match, and I agree
with you that it's useful.

Unfortunately, the GPIO community resource definition is inside the DSDT
ACPI table for my device. I might be wrong here, but I think that Barebox
parses root tables but doesn’t delve into the nested DSDT at the moment.
So it's getting a bit complicated here. I can take a closer look at it
later (without any guarantee of success, of course :-)), but I would
consider it a different patch if you don't mind.

Needless to say, I am coming from the ARM world and haven't found my way
the ACPI just yet :-).

> 
> > 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.

Is following code what you have in mind?

include/platform_data/gpio-intel.h:

	struct gpio_intel_platform_data {
		resource_size_t community_base;
		resource_size_t community_size;
		unsigned int	ngpios;
	};

	static inline struct device *add_intel_gpio_device(
		struct gpio_intel_platform_data *pdata
	)
	{
		return add_generic_device("intel-gpio", DEVICE_ID_DYNAMIC, NULL,
			pdata->community_base, pdata->community_size,
			IORESOURCE_MEM, pdata);
	}


Best regards

Tomas

> 
> 
> 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 |
>