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@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. :-)
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 |
>
