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 |
