From: Arturs Artamonovs <arturs.artamonovs@analog.com>
Add ADSP-SC5xx GPIO driver.
- Support all GPIO ports
- Each gpio support seperate PINT interrupt controller
Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
Co-developed-by: Greg Malysa <greg.malysa@timesys.com>
Signed-off-by: Greg Malysa <greg.malysa@timesys.com>
---
drivers/gpio/Kconfig | 8 +++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-adi-adsp-port.c | 145 ++++++++++++++++++++++++++++++++++++++
3 files changed, 154 insertions(+)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 58f43bcced7c1f29fad5960771817f500ef67ce1..b02693f5b4cec95a59f19aa1bacf7ed72236865a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -147,6 +147,14 @@ config GPIO_74XX_MMIO
8 bits: 74244 (Input), 74273 (Output)
16 bits: 741624 (Input), 7416374 (Output)
+config GPIO_ADI_ADSP_PORT
+ bool "ADI ADSP PORT GPIO driver"
+ depends on OF_GPIO
+ select GPIO_GENERIC
+ help
+ Say Y to enable the ADSP PORT-based GPIO driver for Analog Devices
+ ADSP chips.
+
config GPIO_ALTERA
tristate "Altera GPIO"
depends on OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 64dd6d9d730d5a22564821df71375113e31fe057..fb02c7807a674c8a38d1128e6a25bb7c7f1f4aab 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_GPIO_104_IDI_48) += gpio-104-idi-48.o
obj-$(CONFIG_GPIO_104_IDIO_16) += gpio-104-idio-16.o
obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_74XX_MMIO) += gpio-74xx-mmio.o
+obj-$(CONFIG_GPIO_ADI_ADSP_PORT) += gpio-adi-adsp-port.o
obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_AGGREGATOR) += gpio-aggregator.o
diff --git a/drivers/gpio/gpio-adi-adsp-port.c b/drivers/gpio/gpio-adi-adsp-port.c
new file mode 100644
index 0000000000000000000000000000000000000000..a7a1867495bbdd121cda9b99991865a035dfa117
--- /dev/null
+++ b/drivers/gpio/gpio-adi-adsp-port.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ADSP PORT gpio driver
+ *
+ * (C) Copyright 2022-2024 - Analog Devices, Inc.
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/soc/adi/adsp-gpio-port.h>
+#include "gpiolib.h"
+
+static int adsp_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+ struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
+
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DIR_CLEAR);
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_INEN_SET);
+ return 0;
+}
+
+static int adsp_gpio_direction_output(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
+
+ /*
+ * For open drain ports, they've already been configured by pinctrl and
+ * we should not modify their output characteristics
+ */
+ if (port->open_drain & BIT(offset))
+ return 0;
+
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_INEN_CLEAR);
+
+ if (value)
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DATA_SET);
+ else
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DATA_CLEAR);
+
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DIR_SET);
+ return 0;
+}
+
+static void adsp_gpio_set_value(struct gpio_chip *chip, unsigned int offset, int value)
+{
+ struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
+
+ /*
+ * For open drain ports, set as input if driving a 1, set as output
+ * if driving a 0
+ */
+ if (port->open_drain & BIT(offset)) {
+ if (value) {
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DIR_CLEAR);
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_INEN_SET);
+ } else {
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_INEN_CLEAR);
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DATA_CLEAR);
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DIR_SET);
+ }
+ } else {
+ if (value)
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DATA_SET);
+ else
+ __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DATA_CLEAR);
+ }
+}
+
+static int adsp_gpio_get_value(struct gpio_chip *chip, unsigned int offset)
+{
+ struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
+
+ return !!(__adsp_gpio_readw(port, ADSP_PORT_REG_DATA) & BIT(offset));
+}
+
+static int adsp_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+ struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
+ irq_hw_number_t irq = offset + port->irq_offset;
+ int map = irq_find_mapping(port->irq_domain, irq);
+
+ if (map)
+ return map;
+
+ return irq_create_mapping(port->irq_domain, irq);
+}
+
+static int adsp_gpio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct adsp_gpio_port *gpio;
+ int ret;
+
+ gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ gpio->regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(gpio->regs))
+ return PTR_ERR(gpio->regs);
+
+ gpio->dev = dev;
+
+ ret = adsp_attach_pint_to_gpio(gpio);
+ if (ret)
+ dev_err_probe(gpio->dev, ret, "error attaching interupt to gpio pin\n");
+
+ spin_lock_init(&gpio->lock);
+
+ gpio->gpio.label = "adsp-gpio";
+ gpio->gpio.direction_input = adsp_gpio_direction_input;
+ gpio->gpio.direction_output = adsp_gpio_direction_output;
+ gpio->gpio.get = adsp_gpio_get_value;
+ gpio->gpio.set = adsp_gpio_set_value;
+ gpio->gpio.to_irq = adsp_gpio_to_irq;
+ gpio->gpio.request = gpiochip_generic_request;
+ gpio->gpio.free = gpiochip_generic_free;
+ gpio->gpio.ngpio = ADSP_PORT_NGPIO;
+ gpio->gpio.parent = dev;
+ gpio->gpio.base = -1;
+ return devm_gpiochip_add_data(dev, &gpio->gpio, gpio);
+}
+
+static const struct of_device_id adsp_gpio_of_match[] = {
+ { .compatible = "adi,adsp-port-gpio", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, adsp_gpio_of_match);
+
+static struct platform_driver adsp_gpio_driver = {
+ .driver = {
+ .name = "adsp-port-gpio",
+ .of_match_table = adsp_gpio_of_match,
+ },
+ .probe = adsp_gpio_probe,
+};
+
+module_platform_driver(adsp_gpio_driver);
+
+MODULE_AUTHOR("Greg Malysa <greg.malysa@timesys.com>");
+MODULE_DESCRIPTION("Analog Devices GPIO driver");
+MODULE_LICENSE("GPL v2");
\ No newline at end of file
--
2.25.1Hi Arturs,
thanks for your patch!
On Thu, Sep 12, 2024 at 8:20 PM Arturs Artamonovs via B4 Relay
<devnull+arturs.artamonovs.analog.com@kernel.org> wrote:
> From: Arturs Artamonovs <arturs.artamonovs@analog.com>
>
> Add ADSP-SC5xx GPIO driver.
> - Support all GPIO ports
> - Each gpio support seperate PINT interrupt controller
>
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Co-developed-by: Greg Malysa <greg.malysa@timesys.com>
> Signed-off-by: Greg Malysa <greg.malysa@timesys.com>
(...)
> +config GPIO_ADI_ADSP_PORT
> + bool "ADI ADSP PORT GPIO driver"
> + depends on OF_GPIO
> + select GPIO_GENERIC
If you select this then you need to use it in the idiomatic way.
+#include <linux/soc/adi/adsp-gpio-port.h>
Drop this, just bring the contents into this file all register defines
etc.
+#include "gpiolib.h"
No way, do this:
#include <linux/gpio/driver.h>
> +static int adsp_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
> +
> + __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DIR_CLEAR);
Ah these __adsp_gpio_writew/readw things are too idiomatic. Just
use the base and common writew() please.
> + __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_INEN_SET);
Interrupt enable in the direction function? No thanks, poke the
interrupt registers in your irqchip if you make one (you currently
do not) in this case I'd say just disable all interrupts in probe()
using something like writew(base + ADSP_PORT_REG_INEN_SET, 0xffff)
and be done with it.
> +static int adsp_gpio_get_value(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
> +
> + return !!(__adsp_gpio_readw(port, ADSP_PORT_REG_DATA) & BIT(offset));
> +}
This becomes a reimplemenation of generic GPIO.
> +static int adsp_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
> + irq_hw_number_t irq = offset + port->irq_offset;
> + int map = irq_find_mapping(port->irq_domain, irq);
> +
> + if (map)
> + return map;
> +
> + return irq_create_mapping(port->irq_domain, irq);
> +}
This irqdomain in the "port" looks weird.
Implement the irqchip in the GPIO driver instead.
If the domain *has* to be external to the GPIO driver then
you need to use hierarchical irqdomains.
> +static int adsp_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct adsp_gpio_port *gpio;
> + int ret;
> +
> + gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + gpio->regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(gpio->regs))
> + return PTR_ERR(gpio->regs);
So you have gpio->regs which is the base.
> + gpio->dev = dev;
> +
> + ret = adsp_attach_pint_to_gpio(gpio);
> + if (ret)
> + dev_err_probe(gpio->dev, ret, "error attaching interupt to gpio pin\n");
> +
> + spin_lock_init(&gpio->lock);
> +
> + gpio->gpio.label = "adsp-gpio";
> + gpio->gpio.direction_input = adsp_gpio_direction_input;
> + gpio->gpio.direction_output = adsp_gpio_direction_output;
> + gpio->gpio.get = adsp_gpio_get_value;
> + gpio->gpio.set = adsp_gpio_set_value;
> + gpio->gpio.to_irq = adsp_gpio_to_irq;
> + gpio->gpio.request = gpiochip_generic_request;
> + gpio->gpio.free = gpiochip_generic_free;
> + gpio->gpio.ngpio = ADSP_PORT_NGPIO;
> + gpio->gpio.parent = dev;
> + gpio->gpio.base = -1;
> + return devm_gpiochip_add_data(dev, &gpio->gpio, gpio);
Look in e.g. drivers/gpio/gpio-ftgpio010.c for an example of
how to use generic GPIO (with an irqchip!). It will be something like:
ret = bgpio_init(&g->gc, dev, 2,
gpio->regs + ADSP_PORT_REG_DATA,
gpio->regs + ADSP_PORT_REG_DATA_SET,
gpio->regs + ADSP_PORT_REG_DATA_CLEAR,
gpio->regs + ADSP_PORT_REG_DIR_SET,
gpio->regs + ADSP_PORT_REG_DIR_CLEAR,
0);
if (ret) {
dev_err(dev, "unable to init generic GPIO\n");
goto dis_clk;
}
g->gc.label = dev_name(dev);
g->gc.base = -1;
g->gc.parent = dev;
g->gc.owner = THIS_MODULE;
/* ngpio is set by bgpio_init() */
You can augment the generic driver instance with an extra config function
to set the special open drain bits.
Yours,
Linus Walleij
Hi Linus, Thanks for the review. > > + __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_INEN_SET); > > Interrupt enable in the direction function? No thanks, poke the > interrupt registers in your irqchip if you make one (you currently > do not) in this case I'd say just disable all interrupts in probe() > using something like writew(base + ADSP_PORT_REG_INEN_SET, 0xffff) > and be done with it. > This will come up next time too so I wanted to mention that INEN here means "input enable." The PORT design has two registers for controlling pin direction, one to enable/disable output drivers (DIR) and one to enable input drivers (INEN) to be able to read the pin state from the gpio data register. If I recall the bare metal reference code toggled both but we can review if setting INEN for all pins and leaving it is acceptable as well to simplify things. Thanks, Greg -- Greg Malysa Timesys Corporation
On Tue, Oct 1, 2024 at 11:58 PM Greg Malysa <greg.malysa@timesys.com> wrote: > > Interrupt enable in the direction function? No thanks, poke the > > interrupt registers in your irqchip if you make one (you currently > > do not) in this case I'd say just disable all interrupts in probe() > > using something like writew(base + ADSP_PORT_REG_INEN_SET, 0xffff) > > and be done with it. > > > > This will come up next time too so I wanted to mention that INEN here > means "input enable." The PORT design has two registers for > controlling pin direction, one to enable/disable output drivers (DIR) > and one to enable input drivers (INEN) to be able to read the pin > state from the gpio data register. If I recall the bare metal > reference code toggled both but we can review if setting INEN for all > pins and leaving it is acceptable as well to simplify things. Aha so that's what it means! Yeah play around with it and see what you can come up with. Perhaps you need to override the input/output enable callbacks with local versions rather than the gpio-mmio ones to set all bits. (This is possible to do after bgpio_init() but before adding the gpio_chip if necessary.) Yours, Linus Walleij
On 12/09/2024 20:24, Arturs Artamonovs via B4 Relay wrote:
> From: Arturs Artamonovs <arturs.artamonovs@analog.com>
>
> Add ADSP-SC5xx GPIO driver.
> - Support all GPIO ports
> - Each gpio support seperate PINT interrupt controller
>
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Co-developed-by: Greg Malysa <greg.malysa@timesys.com>
> Signed-off-by: Greg Malysa <greg.malysa@timesys.com>
Your SoB chain is odd. Author is Greg, but Greg is not the first person
in the chain? And no final SoB? This is really odd and not correct. I am
not sure what you even want to say here.
...
> +
> +module_platform_driver(adsp_gpio_driver);
> +
> +MODULE_AUTHOR("Greg Malysa <greg.malysa@timesys.com>");
> +MODULE_DESCRIPTION("Analog Devices GPIO driver");
> +MODULE_LICENSE("GPL v2");
> \ No newline at end of file
Please review all your patches before sending for such stuff.
Best regards,
Krzysztof
Hi Arturs, kernel test robot noticed the following build errors: [auto build test ERROR on da3ea35007d0af457a0afc87e84fddaebc4e0b63] url: https://github.com/intel-lab-lkp/linux/commits/Arturs-Artamonovs-via-B4-Relay/arm64-Add-ADI-ADSP-SC598-SoC/20240913-022308 base: da3ea35007d0af457a0afc87e84fddaebc4e0b63 patch link: https://lore.kernel.org/r/20240912-test-v1-9-458fa57c8ccf%40analog.com patch subject: [PATCH 09/21] gpio: add driver for ADI ADSP-SC5xx platform config: arm64-randconfig-r071-20240914 (https://download.01.org/0day-ci/archive/20240914/202409142215.olyOwnPE-lkp@intel.com/config) compiler: aarch64-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409142215.olyOwnPE-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409142215.olyOwnPE-lkp@intel.com/ All errors (new ones prefixed by >>): aarch64-linux-ld: Unexpected GOT/PLT entries detected! aarch64-linux-ld: Unexpected run-time procedure linkages detected! aarch64-linux-ld: drivers/gpio/gpio-adi-adsp-port.o: in function `adsp_gpio_probe': >> gpio-adi-adsp-port.c:(.text+0x1cc): undefined reference to `adsp_attach_pint_to_gpio' -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
On Thu, Sep 12, 2024, at 18:24, Arturs Artamonovs via B4 Relay wrote:
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/soc/adi/adsp-gpio-port.h>
> +#include "gpiolib.h"
> +
> +static int adsp_gpio_direction_input(struct gpio_chip *chip, unsigned
> int offset)
> +{
> + struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
> +
> + __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DIR_CLEAR);
> + __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_INEN_SET);
> + return 0;
What is the purpose of these __adsp_gpio_writew() in a global header?
Can't those be moved into this file directly?
> \ No newline at end of file
Whitespace damage?
Arnd
© 2016 - 2024 Red Hat, Inc.