Message ID | 1549559639-9447-1-git-send-email-info@metux.net |
---|---|
State | New |
Headers | show |
Series | [1/2] x86: gpio: AMD G-Series pch gpio platform driver | expand |
On Thu, Feb 7, 2019 at 7:14 PM Enrico Weigelt, metux IT consult <info@metux.net> wrote: > > GPIO platform driver for the AMD G-series PCH (eg. on GX-412TC) > > This driver doesn't registers itself automatically, as it needs to > be provided with platform specific configuration, provided by some > board driver setup code. > > Didn't implement oftree probing yet, as it's rarely found on x86. Thanks for the patch, see my comments below. Overall I have a feeling that this driver can be replaced with existing generic one where one register per pin is allocated. Unfortunately, I didn't look deep into this and hope Linus will help to figure this out. > @@ -0,0 +1,171 @@ > +/* > + * GPIO driver for the AMD G series FCH (eg. GX-412TC) > + * > + * Copyright (C) 2018 metux IT consult > + * Author: Enrico Weigelt <info@metux.net> > + * > + * SPDX-License-Identifier: GPL+ SPDX should go as a separate first line in a proper format. > + */ > +// FIXME: add spinlocks Then fix them and come again. > +#include <linux/init.h> > +#include <linux/module.h> One of them should be present, another one dropped. > +#define GPIO_BIT_DIR 23 > +#define GPIO_BIT_WRITE 22 > +#define GPIO_BIT_READ 16 Oh, namespace issues. What about using BIT() macro? > + > + Why two blank lines? > +static uint32_t *amd_fch_gpio_addr(struct gpio_chip *gc, unsigned gpio) > +{ > + struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc); > + > + if (gpio > priv->pdata->gpio_num) { > + dev_err(&priv->pdev->dev, "gpio number %d out of range\n", gpio); > + return NULL; > + } On which circumstances it may happen? > + > + return priv->base + priv->pdata->gpio_reg[gpio].reg*sizeof(u32); > +} > + > +static int amd_fch_gpio_direction_input(struct gpio_chip *gc, unsigned offset) > +{ > + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, offset); volatile?! I think you need to use readl()/writel() (or their _relaxed variants) instead. Same applies for entire code. > + if (!ptr) return -EINVAL; This code has style issues. Check your entire file. > + > + *ptr &= ~(1 << GPIO_BIT_DIR); > + return 0; > +} > +static void amd_fch_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc) > +{ > + struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc); > + (void)priv; > + > + seq_printf(s, "debug info not implemented yet\n"); > +} Remove whatever is not implemented and not required to have a stub. > +static int amd_fch_gpio_request(struct gpio_chip *chip, unsigned gpio_pin) > +{ > + if (gpio_pin < chip->ngpio) > + return 0; Is it even possible? > + > + return -EINVAL; > +} > + > +static int amd_fch_gpio_probe(struct platform_device *pdev) > +{ > + struct amd_fch_gpio_priv *priv; > + struct amd_fch_gpio_pdata *pdata = pdev->dev.platform_data; We have a helper to get this. platform_get_data() IIRC. > + int err; > + > + if (!pdata) { > + dev_err(&pdev->dev, "no platform_data\n"); > + return -ENOENT; > + } > + > + if (!(priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL))) { Should be two lines. > + dev_err(&pdev->dev, "failed to allocate priv struct\n"); Noise. > + return -ENOMEM; > + } > + > + if (IS_ERR(priv->base = devm_ioremap_resource(&pdev->dev, &priv->pdata->res))) { > + dev_err(&pdev->dev, "failed to map iomem\n"); Noise (that function will print a message) > + return -ENXIO; Shadowed error code. > + } > + > + dev_info(&pdev->dev, "initializing on my own II\n"); Noise. > + > + if (IS_ENABLED(CONFIG_DEBUG_FS)) { Do you really care? > + dev_info(&pdev->dev, "enabling debugfs\n"); Noise. > + priv->gc.dbg_show = amd_fch_gpio_dbg_show; > + } > + > + platform_set_drvdata(pdev, priv); > + > + err = devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv); > + dev_info(&pdev->dev, "probe finished\n"); > + return err; return devm_gpiochip_add_data(...); > +} > +MODULE_LICENSE("GPL"); License mismatch. I really don't look what 'GPL+' means. OTOH I know this one corresponds to GPL-2.0+. > +++ b/include/linux/platform_data/x86/amd-fch-gpio-pdata.h > @@ -0,0 +1,41 @@ > +/* > + * AMD FCH gpio driver platform-data > + * > + * Copyright (C) 2018 metux IT consult > + * Author: Enrico Weigelt <info@metux.net> > + * > + * SPDX-License-Identifier: GPL Same comments. > + */ > +/* It's not marked as kernel doc. > + * struct amd_fch_gpio_reg - GPIO register definition > + * @reg: register index > + * @name: signal name > + */ > +struct amd_fch_gpio_reg { > + int reg; > + const char* name; > +}; Isn't this provided by GPIO library? We have so called labels. > +/* > + * struct amd_fch_gpio_pdata - GPIO chip platform data > + * @resource: iomem range > + * @gpio_reg: array of gpio registers > + * @gpio_num: number of entries > + */ > +struct amd_fch_gpio_pdata { > + struct resource res; > + int gpio_num; > + struct amd_fch_gpio_reg *gpio_reg; > + int gpio_base; > +};
On 07.02.19 19:06, Andy Shevchenko wrote: Hi, > Overall I have a feeling that this driver can be replaced with> existing generic one where one register per pin is allocated.> Unfortunately, I didn't look deep into this and hope Linus will help> to figure this out. this also was my first thought, but i recall the generic one copes w/ devices that have per-flag- instead of per channel-registers (IOW: all direction flags in one register, all level flags in another). correct me if I'm wrong .. I actually considered writing a generic per-register driver, where one just configures which bit does what. But it didn't feel worth the extra effort yet. OTOH, if we would have lots of consumers, the situation would be different. >> @@ -0,0 +1,171 @@ >> +/* >> + * GPIO driver for the AMD G series FCH (eg. GX-412TC) >> + * >> + * Copyright (C) 2018 metux IT consult >> + * Author: Enrico Weigelt <info@metux.net> >> + * > >> + * SPDX-License-Identifier: GPL+ > > SPDX should go as a separate first line in a proper format. Fixed. Maybe I should write an automatic check for that ;-) By the way: are there already some tools that actually operate on that ? >> +#define GPIO_BIT_DIR 23 >> +#define GPIO_BIT_WRITE 22 >> +#define GPIO_BIT_READ 16 > > Oh, namespace issues. > What about using BIT() macro? Ah, thanks, forgot that. By the way: would it be a good idea to define a struct w/ bitfields for the registers, instead of directly doing bitmask operations ? >> +static uint32_t *amd_fch_gpio_addr(struct gpio_chip *gc, unsigned gpio) >> +{ >> + struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc); >> + > >> + if (gpio > priv->pdata->gpio_num) { >> + dev_err(&priv->pdev->dev, "gpio number %d out of range\n", gpio); >> + return NULL; >> + } > > On which circumstances it may happen? hopefully never, but I'm a bit paranoid ;-) Shall I kick out that check ? >> + return priv->base + priv->pdata->gpio_reg[gpio].reg*sizeof(u32); >> +} >> + >> +static int amd_fch_gpio_direction_input(struct gpio_chip *gc, unsigned offset) >> +{ > >> + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, offset); > > volatile?! > > I think you need to use readl()/writel() (or their _relaxed variants) instead. I assumed the compiler would already emit the correct code (and not try any optimizations) if the field is declared volatile. But I'll change it to readl()/writel(). By the way: do we already have helpers for doing such bit operations on mmapped registers ? (eg. similar to those in bitops.h) > Same applies for entire code. > >> + if (!ptr) return -EINVAL; > > This code has style issues. > Check your entire file. Should it be written in two lines - like that ? if (!ptr) return -EINVAL; >> +static int amd_fch_gpio_request(struct gpio_chip *chip, unsigned gpio_pin) >> +{ > >> + if (gpio_pin < chip->ngpio) >> + return 0; > > Is it even possible? Not sure. AFAIK, this function should check whether the requested pin is available. Feels safer to me having this check. >> +static int amd_fch_gpio_probe(struct platform_device *pdev) >> +{ >> + struct amd_fch_gpio_priv *priv; > >> + struct amd_fch_gpio_pdata *pdata = pdev->dev.platform_data; > > We have a helper to get this. platform_get_data() IIRC. found dev_get_platdata(const struct device *dev). but nothing for working on struct platform_device directly - should we introduce one ? >> + if (IS_ERR(priv->base = devm_ioremap_resource(&pdev->dev, &priv->pdata->res))) { > >> + dev_err(&pdev->dev, "failed to map iomem\n"); > > Noise (that function will print a message) > >> + return -ENXIO; > > Shadowed error code. Which one shall I use instead ? >> +MODULE_LICENSE("GPL"); > > License mismatch. I really don't look what 'GPL+' means. OTOH I know > this one corresponds to GPL-2.0+. Typo, should have been GPL-2.0+. >> + * struct amd_fch_gpio_reg - GPIO register definition >> + * @reg: register index >> + * @name: signal name >> + */ >> +struct amd_fch_gpio_reg { >> + int reg; >> + const char* name; >> +}; > > Isn't this provided by GPIO library? We have so called labels. hmm, haven't found a proper struct yet. struct gpio indeed has a label and two int fields. but it doesn't seem to be designed for holding register addresses ... using this one here feels quite abusive. (and a waste of memory, too). for consistency, I could rename 'name' to 'label', if you wish. thanks for your review. --mtx
diff --git a/MAINTAINERS b/MAINTAINERS index 8c68de3c..a693f39 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -766,6 +766,13 @@ S: Supported F: Documentation/hwmon/fam15h_power F: drivers/hwmon/fam15h_power.c +AMD FCH GPIO DRIVER +M: Enrico Weigelt, metux IT consult <info@metux.net> +L: linux-gpio@vger.kernel.org +S: Maintanced +F: drivers/gpio/gpio-amd-fch.c +F: include/linux/platform_data/x86/amd-fch-gpio-pdata.h + AMD GEODE CS5536 USB DEVICE CONTROLLER DRIVER L: linux-geode@lists.infradead.org (moderated for non-subscribers) S: Orphan diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index b5a2845..a3e47c8 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -654,6 +654,16 @@ config GPIO_LOONGSON1 help Say Y or M here to support GPIO on Loongson1 SoCs. +config GPIO_AMD_FCH + tristate "GPIO support for AMD Fusion Controller Hub (G-series SOCs)" + select GPIO_GENERIC + help + This option enables driver for GPIO on AMDs Fusion Controller Hub, + as found on G-series SOCs (eg. GX-412TC) + + Note: This driver doesn't registers itself automatically, as it + needs to be provided with platform specific configuration. + (See eg. CONFIG_PCENGINES_APU2.) endmenu menu "Port-mapped I/O GPIO drivers" diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 37628f8..bb48fd2 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -27,6 +27,7 @@ obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o obj-$(CONFIG_GPIO_ALTERA_A10SR) += gpio-altera-a10sr.o +obj-$(CONFIG_GPIO_AMD_FCH) += gpio-amd-fch.o obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o obj-$(CONFIG_GPIO_AMDPT) += gpio-amdpt.o obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o diff --git a/drivers/gpio/gpio-amd-fch.c b/drivers/gpio/gpio-amd-fch.c new file mode 100644 index 0000000..8a002453 --- /dev/null +++ b/drivers/gpio/gpio-amd-fch.c @@ -0,0 +1,171 @@ +/* + * GPIO driver for the AMD G series FCH (eg. GX-412TC) + * + * Copyright (C) 2018 metux IT consult + * Author: Enrico Weigelt <info@metux.net> + * + * SPDX-License-Identifier: GPL+ + */ + +// FIXME: add spinlocks + +#include <linux/err.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/gpio/driver.h> +#include <linux/platform_data/x86/amd-fch-gpio-pdata.h> + + +#define GPIO_BIT_DIR 23 +#define GPIO_BIT_WRITE 22 +#define GPIO_BIT_READ 16 + + +struct amd_fch_gpio_priv { + struct platform_device *pdev; + struct gpio_chip gc; + void __iomem *base; + struct amd_fch_gpio_pdata *pdata; +}; + +static uint32_t *amd_fch_gpio_addr(struct gpio_chip *gc, unsigned gpio) +{ + struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc); + + if (gpio > priv->pdata->gpio_num) { + dev_err(&priv->pdev->dev, "gpio number %d out of range\n", gpio); + return NULL; + } + + return priv->base + priv->pdata->gpio_reg[gpio].reg*sizeof(u32); +} + +static int amd_fch_gpio_direction_input(struct gpio_chip *gc, unsigned offset) +{ + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, offset); + if (!ptr) return -EINVAL; + + *ptr &= ~(1 << GPIO_BIT_DIR); + return 0; +} + +static int amd_fch_gpio_direction_output(struct gpio_chip *gc, unsigned gpio, int value) +{ + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, gpio); + if (!ptr) return -EINVAL; + + *ptr |= (1 << GPIO_BIT_DIR); + return 0; +} + +static int amd_fch_gpio_get_direction(struct gpio_chip *gc, unsigned gpio) +{ + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, gpio); + if (!ptr) return -EINVAL; + + return (*ptr >> GPIO_BIT_DIR) & 1; +} + +static void amd_fch_gpio_set(struct gpio_chip *gc, unsigned gpio, int value) +{ + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, gpio); + if (!ptr) return; + + if (value) + *ptr |= (1 << GPIO_BIT_WRITE); + else + *ptr &= ~(1 << GPIO_BIT_WRITE); +} + +static int amd_fch_gpio_get(struct gpio_chip *gc, unsigned offset) +{ + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, offset); + if (!ptr) return -EINVAL; + + return ((*ptr) >> GPIO_BIT_READ) & 1; +} + +static void amd_fch_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc) +{ + struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc); + (void)priv; + + seq_printf(s, "debug info not implemented yet\n"); +} + +static int amd_fch_gpio_request(struct gpio_chip *chip, unsigned gpio_pin) +{ + if (gpio_pin < chip->ngpio) + return 0; + + return -EINVAL; +} + +static int amd_fch_gpio_probe(struct platform_device *pdev) +{ + struct amd_fch_gpio_priv *priv; + struct amd_fch_gpio_pdata *pdata = pdev->dev.platform_data; + int err; + + if (!pdata) { + dev_err(&pdev->dev, "no platform_data\n"); + return -ENOENT; + } + + if (!(priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL))) { + dev_err(&pdev->dev, "failed to allocate priv struct\n"); + return -ENOMEM; + } + + priv->pdata = pdata; + priv->pdev = pdev; + + priv->gc.owner = THIS_MODULE; + priv->gc.parent = &pdev->dev; + priv->gc.label = dev_name(&pdev->dev); + priv->gc.base = priv->pdata->gpio_base; + priv->gc.ngpio = priv->pdata->gpio_num; + priv->gc.request = amd_fch_gpio_request; + priv->gc.direction_input = amd_fch_gpio_direction_input; + priv->gc.direction_output = amd_fch_gpio_direction_output; + priv->gc.get_direction = amd_fch_gpio_get_direction; + priv->gc.get = amd_fch_gpio_get; + priv->gc.set = amd_fch_gpio_set; + + spin_lock_init(&priv->gc.bgpio_lock); + + if (IS_ERR(priv->base = devm_ioremap_resource(&pdev->dev, &priv->pdata->res))) { + dev_err(&pdev->dev, "failed to map iomem\n"); + return -ENXIO; + } + + dev_info(&pdev->dev, "initializing on my own II\n"); + + if (IS_ENABLED(CONFIG_DEBUG_FS)) { + dev_info(&pdev->dev, "enabling debugfs\n"); + priv->gc.dbg_show = amd_fch_gpio_dbg_show; + } + + platform_set_drvdata(pdev, priv); + + err = devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv); + dev_info(&pdev->dev, "probe finished\n"); + return err; +} + +static struct platform_driver amd_fch_gpio_driver = { + .driver = { + .name = AMD_FCH_GPIO_DRIVER_NAME, + }, + .probe = amd_fch_gpio_probe, +}; + +module_platform_driver(amd_fch_gpio_driver); + +MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>"); +MODULE_DESCRIPTION("AMD G-series FCH GPIO driver"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:gpio_amd_fch"); diff --git a/include/linux/platform_data/x86/amd-fch-gpio-pdata.h b/include/linux/platform_data/x86/amd-fch-gpio-pdata.h new file mode 100644 index 0000000..68c1730 --- /dev/null +++ b/include/linux/platform_data/x86/amd-fch-gpio-pdata.h @@ -0,0 +1,41 @@ +/* + * AMD FCH gpio driver platform-data + * + * Copyright (C) 2018 metux IT consult + * Author: Enrico Weigelt <info@metux.net> + * + * SPDX-License-Identifier: GPL + */ + +#ifndef AMD_FCH_PDATA_H +#define AMD_FCH_PDATA_H + + +#include <linux/ioport.h> + +#define AMD_FCH_GPIO_DRIVER_NAME "gpio_amd_fch" + +/* + * struct amd_fch_gpio_reg - GPIO register definition + * @reg: register index + * @name: signal name + */ +struct amd_fch_gpio_reg { + int reg; + const char* name; +}; + +/* + * struct amd_fch_gpio_pdata - GPIO chip platform data + * @resource: iomem range + * @gpio_reg: array of gpio registers + * @gpio_num: number of entries + */ +struct amd_fch_gpio_pdata { + struct resource res; + int gpio_num; + struct amd_fch_gpio_reg *gpio_reg; + int gpio_base; +}; + +#endif /* AMD_FCH_PDATA_H */
GPIO platform driver for the AMD G-series PCH (eg. on GX-412TC) This driver doesn't registers itself automatically, as it needs to be provided with platform specific configuration, provided by some board driver setup code. Didn't implement oftree probing yet, as it's rarely found on x86. Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net> --- MAINTAINERS | 7 + drivers/gpio/Kconfig | 10 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-amd-fch.c | 171 +++++++++++++++++++++ .../linux/platform_data/x86/amd-fch-gpio-pdata.h | 41 +++++ 5 files changed, 230 insertions(+) create mode 100644 drivers/gpio/gpio-amd-fch.c create mode 100644 include/linux/platform_data/x86/amd-fch-gpio-pdata.h