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

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

Hello Tomas,

On 10.04.24 09:39, Tomas Marek wrote:
> On Tue, Apr 09, 2024 at 09:41:28AM +0200, Ahmad Fatoum wrote:
>>> +static int intel_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
>>> +		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

Typo: s/itco_wdt/WDAT/

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

We indeed do not do DSDT parsing yet.

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

I was thinking of matching against the SoC type (e.g. Alderlake) and just
register hardcoded values for the addresses/ngpios. I am not sure what's the
easiest way to get this information though (maybe SMBIOS?).

This shouldn't delay applying this driver though.

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

Same here ^^

>>> +#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);
> 	}

This looks ok.

Cheers,
Ahmad

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

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