lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 26 Mar 2020 21:05:51 +0100 From: Michael Walle <michael@...le.cc> To: Bartosz Golaszewski <bgolaszewski@...libre.com> Cc: linux-gpio <linux-gpio@...r.kernel.org>, linux-devicetree <devicetree@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, linux-hwmon@...r.kernel.org, linux-pwm@...r.kernel.org, LINUXWATCHDOG <linux-watchdog@...r.kernel.org>, arm-soc <linux-arm-kernel@...ts.infradead.org>, Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh+dt@...nel.org>, Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>, Lee Jones <lee.jones@...aro.org>, Thierry Reding <thierry.reding@...il.com>, Uwe Kleine-König <u.kleine-koenig@...gutronix.de>, Wim Van Sebroeck <wim@...ux-watchdog.org>, Shawn Guo <shawnguo@...nel.org>, Li Yang <leoyang.li@....com>, Thomas Gleixner <tglx@...utronix.de>, Jason Cooper <jason@...edaemon.net>, Marc Zyngier <maz@...nel.org> Subject: Re: [PATCH 12/18] gpio: add support for the sl28cpld GPIO controller Hi Bartosz, Am 2020-03-25 12:50, schrieb Bartosz Golaszewski: > śr., 18 mar 2020 o 13:45 Michael Walle <michael@...le.cc> napisał(a): >> >> Hi Bartosz, >> >> Am 2020-03-18 10:14, schrieb Bartosz Golaszewski: >> > wt., 17 mar 2020 o 21:50 Michael Walle <michael@...le.cc> napisał(a): >> >> >> >> This adds support for the GPIO controller of the sl28 board management >> >> controller. This driver is part of a multi-function device. >> >> >> >> Signed-off-by: Michael Walle <michael@...le.cc> >> > >> > Hi Michael, >> > >> > thanks for the driver. Please take a look at some comments below. >> >> well, thank you for the very fast review! >> >> >> --- >> >> drivers/gpio/Kconfig | 11 ++ >> >> drivers/gpio/Makefile | 1 + >> >> drivers/gpio/gpio-sl28cpld.c | 332 >> >> +++++++++++++++++++++++++++++++++++ >> >> 3 files changed, 344 insertions(+) >> >> create mode 100644 drivers/gpio/gpio-sl28cpld.c >> >> >> >> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig >> >> index 3cbf8882a0dd..516e47017ef5 100644 >> >> --- a/drivers/gpio/Kconfig >> >> +++ b/drivers/gpio/Kconfig >> >> @@ -1211,6 +1211,17 @@ config GPIO_RC5T583 >> >> This driver provides the support for driving/reading the >> >> gpio pins >> >> of RC5T583 device through standard gpio library. >> >> >> >> +config GPIO_SL28CPLD >> >> + tristate "Kontron sl28 GPIO" >> >> + depends on MFD_SL28CPLD >> >> + depends on OF_GPIO >> >> + select GPIOLIB_IRQCHIP >> > >> > Please see below - I think both are not needed. >> > >> >> + help >> >> + This enables support for the GPIOs found on the Kontron sl28 >> >> CPLD. >> >> + >> >> + This driver can also be built as a module. If so, the module >> >> will be >> >> + called gpio-sl28cpld. >> >> + >> >> config GPIO_STMPE >> >> bool "STMPE GPIOs" >> >> depends on MFD_STMPE >> >> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile >> >> index 0b571264ddbc..0ca2d52c78e8 100644 >> >> --- a/drivers/gpio/Makefile >> >> +++ b/drivers/gpio/Makefile >> >> @@ -127,6 +127,7 @@ obj-$(CONFIG_GPIO_SCH311X) += >> >> gpio-sch311x.o >> >> obj-$(CONFIG_GPIO_SCH) += gpio-sch.o >> >> obj-$(CONFIG_GPIO_SIFIVE) += gpio-sifive.o >> >> obj-$(CONFIG_GPIO_SIOX) += gpio-siox.o >> >> +obj-$(CONFIG_GPIO_SL28CPLD) += gpio-sl28cpld.o >> >> obj-$(CONFIG_GPIO_SODAVILLE) += gpio-sodaville.o >> >> obj-$(CONFIG_GPIO_SPEAR_SPICS) += gpio-spear-spics.o >> >> obj-$(CONFIG_GPIO_SPRD) += gpio-sprd.o >> >> diff --git a/drivers/gpio/gpio-sl28cpld.c >> >> b/drivers/gpio/gpio-sl28cpld.c >> >> new file mode 100644 >> >> index 000000000000..94f82013882f >> >> --- /dev/null >> >> +++ b/drivers/gpio/gpio-sl28cpld.c >> >> @@ -0,0 +1,332 @@ >> >> +// SPDX-License-Identifier: GPL-2.0-only >> >> +/* >> >> + * SMARC-sAL28 GPIO driver. >> >> + * >> >> + * Copyright 2019 Kontron Europe GmbH >> >> + */ >> >> + >> >> +#include <linux/kernel.h> >> >> +#include <linux/module.h> >> >> +#include <linux/of.h> >> >> +#include <linux/of_device.h> >> >> +#include <linux/of_address.h> >> >> +#include <linux/interrupt.h> >> >> +#include <linux/regmap.h> >> >> +#include <linux/platform_device.h> >> >> +#include <linux/gpio/driver.h> >> >> + >> >> +#define GPIO_REG_DIR 0 >> >> +#define GPIO_REG_OUT 1 >> >> +#define GPIO_REG_IN 2 >> >> +#define GPIO_REG_IE 3 >> >> +#define GPIO_REG_IP 4 >> > >> > These values would be more clear if they were defined as hex. >> > >> >> + >> >> +#define GPI_REG_IN 0 >> >> + >> >> +#define GPO_REG_OUT 0 >> > >> > Please also use a common prefix even for defines. >> >> ok >> >> > >> >> + >> >> +enum sl28cpld_gpio_type { >> >> + sl28cpld_gpio, >> >> + sl28cpld_gpi, >> >> + sl28cpld_gpo, >> >> +}; >> > >> > Enum values should be all upper-case. >> >> ok >> >> >> + >> >> +struct sl28cpld_gpio { >> >> + struct gpio_chip gpio_chip; >> >> + struct irq_chip irq_chip; >> >> + struct regmap *regmap; >> >> + u32 offset; >> >> + struct mutex lock; >> >> + u8 ie; >> >> +}; >> >> + >> >> +static void sl28cpld_gpio_set_reg(struct gpio_chip *chip, unsigned >> >> int reg, >> >> + unsigned int offset, int value) >> >> +{ >> >> + struct sl28cpld_gpio *gpio = gpiochip_get_data(chip); >> >> + unsigned int mask = 1 << offset; >> >> + unsigned int val = value << offset; >> >> + >> >> + regmap_update_bits(gpio->regmap, gpio->offset + reg, mask, >> >> val); >> >> +} >> >> + >> >> +static void sl28cpld_gpio_set(struct gpio_chip *chip, unsigned int >> >> offset, >> >> + int value) >> >> +{ >> >> + sl28cpld_gpio_set_reg(chip, GPIO_REG_OUT, offset, value); >> >> +} >> >> + >> >> +static void sl28cpld_gpo_set(struct gpio_chip *chip, unsigned int >> >> offset, >> >> + int value) >> >> +{ >> >> + sl28cpld_gpio_set_reg(chip, GPO_REG_OUT, offset, value); >> >> +} >> >> + >> >> +static int sl28cpld_gpio_get_reg(struct gpio_chip *chip, unsigned int >> >> reg, >> >> + unsigned int offset) >> >> +{ >> >> + struct sl28cpld_gpio *gpio = gpiochip_get_data(chip); >> >> + unsigned int mask = 1 << offset; >> >> + unsigned int val; >> >> + int ret; >> >> + >> >> + ret = regmap_read(gpio->regmap, gpio->offset + reg, &val); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + return (val & mask) ? 1 : 0; >> >> +} >> >> + >> >> +static int sl28cpld_gpio_get(struct gpio_chip *chip, unsigned int >> >> offset) >> >> +{ >> >> + return sl28cpld_gpio_get_reg(chip, GPIO_REG_IN, offset); >> >> +} >> >> + >> >> +static int sl28cpld_gpi_get(struct gpio_chip *chip, unsigned int >> >> offset) >> >> +{ >> >> + return sl28cpld_gpio_get_reg(chip, GPI_REG_IN, offset); >> >> +} >> >> + >> >> +static int sl28cpld_gpio_get_direction(struct gpio_chip *chip, >> >> + unsigned int offset) >> >> +{ >> >> + struct sl28cpld_gpio *gpio = gpiochip_get_data(chip); >> >> + unsigned int reg; >> >> + int ret; >> >> + >> >> + ret = regmap_read(gpio->regmap, gpio->offset + GPIO_REG_DIR, >> >> ®); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + if (reg & (1 << offset)) >> >> + return GPIO_LINE_DIRECTION_OUT; >> >> + else >> >> + return GPIO_LINE_DIRECTION_IN; >> >> +} >> >> + >> >> +static int sl28cpld_gpio_set_direction(struct gpio_chip *chip, >> >> + unsigned int offset, >> >> + bool output) >> >> +{ >> >> + struct sl28cpld_gpio *gpio = gpiochip_get_data(chip); >> >> + unsigned int mask = 1 << offset; >> >> + unsigned int val = (output) ? mask : 0; >> >> + >> >> + return regmap_update_bits(gpio->regmap, gpio->offset + >> >> GPIO_REG_DIR, >> >> + mask, val); >> >> + >> > >> > Stray newline. >> ok >> >> > >> >> +} >> >> + >> >> +static int sl28cpld_gpio_direction_input(struct gpio_chip *chip, >> >> + unsigned int offset) >> >> +{ >> >> + return sl28cpld_gpio_set_direction(chip, offset, false); >> >> +} >> >> + >> >> +static int sl28cpld_gpio_direction_output(struct gpio_chip *chip, >> >> + unsigned int offset, int >> >> value) >> >> +{ >> >> + sl28cpld_gpio_set_reg(chip, GPIO_REG_OUT, offset, value); >> >> + return sl28cpld_gpio_set_direction(chip, offset, true); >> >> +} >> >> + >> >> +static void sl28cpld_gpio_irq_lock(struct irq_data *data) >> >> +{ >> >> + struct sl28cpld_gpio *gpio = >> >> + gpiochip_get_data(irq_data_get_irq_chip_data(data)); >> >> + >> >> + mutex_lock(&gpio->lock); >> > >> > How does that actually lock anything? >> >> TBH, I took that from gpio-pcf857x.c. But that >> (1) don't uses regmap >> (2) also uses that lock on other places. >> >> I'll dig deeper into that and try to understand why there is a lock at >> all and why this callback is actually called _irq_lock() because that >> made me wonder. >> >> > Regmap uses a different lock and >> > if you want to make sure nobody modifies the GPIO registers than you'd >> > need to use the same lock. Also: this looks a lot like a task for >> > regmap_irqchip - maybe you could use it here or in the core mfd >> > module? >> >> regmap_irqchip will register the interrupt controller on the device >> which owns the regmap, ie. the parent. So (1) the phandle would need >> to >> point to the parent device instead of the GPIO subnode and (2) I'm >> already using the regmap_irqchip for the interrupt controller. I don't >> know if you can actually have that multiple times. >> >> there was a discussion which might apply partly to (1): >> https://lore.kernel.org/patchwork/patch/802608/ >> > > In that case maybe you should use the disable_locking option in > regmap_config and provide your own callbacks that you can use in the > irqchip code too? But how would that solve problem (1). And keep in mind, that the reqmap_irqchip is actually used for the interrupt controller, which is not this gpio controller. Ie. the interrupt controller of the sl28cpld uses the regmap_irqchip and all interrupt phandles pointing to the interrupt controller will reference the toplevel node. Any phandles pointing to the gpio controller will reference the GPIO subnode. > >> > >> >> +} >> >> + >> >> +static void sl28cpld_gpio_irq_sync_unlock(struct irq_data *data) >> >> +{ >> >> + struct sl28cpld_gpio *gpio = >> >> + gpiochip_get_data(irq_data_get_irq_chip_data(data)); >> >> + >> >> + regmap_write(gpio->regmap, gpio->offset + GPIO_REG_IE, >> >> gpio->ie); >> >> + mutex_unlock(&gpio->lock); >> >> +} >> >> + >> >> +static void sl28cpld_gpio_irq_disable(struct irq_data *data) >> >> +{ >> >> + struct sl28cpld_gpio *gpio = >> >> + gpiochip_get_data(irq_data_get_irq_chip_data(data)); >> >> + >> >> + if (data->hwirq >= 8) >> >> + return; >> >> + >> >> + gpio->ie &= ~(1 << data->hwirq); >> >> +} >> >> + >> >> +static void sl28cpld_gpio_irq_enable(struct irq_data *data) >> >> +{ >> >> + struct sl28cpld_gpio *gpio = >> >> + gpiochip_get_data(irq_data_get_irq_chip_data(data)); >> >> + >> >> + if (data->hwirq >= 8) >> >> + return; >> >> + >> >> + gpio->ie |= (1 << data->hwirq); >> >> +} >> >> + >> >> +static int sl28cpld_gpio_irq_set_type(struct irq_data *data, unsigned >> >> int type) >> >> +{ >> >> + /* only edge triggered interrupts on both edges are supported >> >> */ >> >> + return (type == IRQ_TYPE_EDGE_BOTH) ? 0 : -EINVAL; >> >> +} >> >> + >> >> +static irqreturn_t sl28cpld_gpio_irq_thread(int irq, void *data) >> >> +{ >> >> + struct sl28cpld_gpio *gpio = data; >> >> + unsigned int ip; >> >> + unsigned int virq; >> >> + int pin; >> >> + int ret; >> >> + >> >> + ret = regmap_read(gpio->regmap, gpio->offset + GPIO_REG_IP, >> >> &ip); >> >> + if (ret) >> >> + return IRQ_NONE; >> >> + >> >> + /* mask other pending interrupts which are not enabled */ >> >> + ip &= gpio->ie; >> >> + >> >> + /* ack the interrupts */ >> >> + regmap_write(gpio->regmap, gpio->offset + GPIO_REG_IP, ip); >> >> + >> >> + /* and handle them */ >> >> + while (ip) { >> >> + pin = __ffs(ip); >> >> + ip &= ~BIT(pin); >> >> + >> >> + virq = irq_find_mapping(gpio->gpio_chip.irq.domain, >> >> pin); >> >> + if (virq) >> >> + handle_nested_irq(virq); >> >> + } >> >> + >> >> + return IRQ_HANDLED; >> >> +} >> > >> > This definitely looks like parts of regmap_irqchip reimplemented. >> > Please check if you could reuse it - it would save a lot of code. >> >> See above. I'd be happy to reuse the code though. >> >> > >> >> + >> >> +static int sl28_cpld_gpio_irq_init(struct platform_device *pdev, int >> >> irq) >> >> +{ >> >> + struct sl28cpld_gpio *gpio = platform_get_drvdata(pdev); >> >> + struct irq_chip *irq_chip = &gpio->irq_chip; >> >> + int ret; >> >> + >> >> + irq_chip->name = "sl28cpld-gpio-irq", >> >> + irq_chip->irq_bus_lock = sl28cpld_gpio_irq_lock, >> >> + irq_chip->irq_bus_sync_unlock = sl28cpld_gpio_irq_sync_unlock, >> >> + irq_chip->irq_disable = sl28cpld_gpio_irq_disable, >> >> + irq_chip->irq_enable = sl28cpld_gpio_irq_enable, >> >> + irq_chip->irq_set_type = sl28cpld_gpio_irq_set_type, >> >> + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE, >> >> + >> >> + ret = gpiochip_irqchip_add_nested(&gpio->gpio_chip, irq_chip, >> >> 0, >> >> + handle_simple_irq, >> >> IRQ_TYPE_NONE); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, >> >> + sl28cpld_gpio_irq_thread, >> >> + IRQF_SHARED | IRQF_ONESHOT, >> >> + pdev->name, gpio); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + gpiochip_set_nested_irqchip(&gpio->gpio_chip, irq_chip, irq); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static int sl28cpld_gpio_probe(struct platform_device *pdev) >> >> +{ >> >> + enum sl28cpld_gpio_type type = >> >> + platform_get_device_id(pdev)->driver_data; >> >> + struct device_node *np = pdev->dev.of_node; >> >> + struct sl28cpld_gpio *gpio; >> >> + struct gpio_chip *chip; >> >> + struct resource *res; >> >> + bool irq_support = false; >> >> + int ret; >> >> + int irq; >> >> + >> >> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); >> >> + if (!gpio) >> >> + return -ENOMEM; >> >> + >> >> + if (!pdev->dev.parent) >> >> + return -ENODEV; >> > >> > Why not check this before allocating any memory? >> >> I'll change that, you're not the first one which notices that. My >> reason >> was to have the check together with the dev_get_regmap() which uses >> the >> parent, expecting that the error case only happen exceptionally. >> >> > >> >> + >> >> + gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL); >> >> + if (!gpio->regmap) >> >> + return -ENODEV; >> >> + >> >> + res = platform_get_resource(pdev, IORESOURCE_REG, 0); >> >> + if (!res) >> >> + return -EINVAL; >> >> + gpio->offset = res->start; >> >> + >> > >> > This isn't how IO resources are used. What are you trying to achieve >> > here? >> >> Mh are you sure? The blueprint for this were the regulators in >> drivers/regulators/, eg the wm831x-ldo.c. IORESOURCE_REG isn't used >> that often. But here is what I want to achieve (for which I haven't >> found any existing drivers for now): >> (1) the individual blocks of the overall sl28cpld may be used >> multiple times, eg. this driver only has the offset to a >> base address. So if there are two blocks, this mfd core >> driver will register two devices for this driver with >> different base offsets, which are passed by IORESOURCE_REG >> (2) I wanted to avoid having a private mfd include with some >> kind of "proprietary" method how to get that offset >> (3) the mfd core driver is the one knowing the offset, thus it >> is possible to have different flavours of the sl28cpld >> > > Ok, now I see it's documented in the bindings. Thanks for the > explanation. > >> >> > >> >> + /* initialize struct gpio_chip */ >> >> + mutex_init(&gpio->lock); >> >> + chip = &gpio->gpio_chip; >> >> + chip->parent = &pdev->dev; >> >> + chip->label = dev_name(&pdev->dev); >> >> + chip->owner = THIS_MODULE; >> >> + chip->can_sleep = true; >> >> + chip->base = -1; >> >> + chip->ngpio = 8; >> >> + >> >> + switch (type) { >> >> + case sl28cpld_gpio: >> >> + chip->get_direction = sl28cpld_gpio_get_direction; >> >> + chip->direction_input = sl28cpld_gpio_direction_input; >> >> + chip->direction_output = >> >> sl28cpld_gpio_direction_output; >> >> + chip->get = sl28cpld_gpio_get; >> >> + chip->set = sl28cpld_gpio_set; >> >> + irq_support = true; >> >> + break; >> >> + case sl28cpld_gpo: >> >> + chip->set = sl28cpld_gpo_set; >> >> + chip->get = sl28cpld_gpi_get; >> >> + break; >> >> + case sl28cpld_gpi: >> >> + chip->get = sl28cpld_gpi_get; >> >> + break; >> >> + } >> >> + >> >> + ret = devm_gpiochip_add_data(&pdev->dev, chip, gpio); >> >> + if (ret < 0) >> >> + return ret; >> >> + >> >> + platform_set_drvdata(pdev, gpio); >> >> + >> >> + if (irq_support && of_property_read_bool(np, >> >> "interrupt-controller")) { >> > >> > You're depending on OF_GPIO for this one function. Please switch to >> > device_property_read_bool() instead. >> >> ok >> >> >> > >> >> + irq = platform_get_irq(pdev, 0); >> >> + if (irq < 0) >> >> + return ret; >> >> + >> >> + ret = sl28_cpld_gpio_irq_init(pdev, irq); >> >> + if (ret) >> >> + return ret; >> >> + } >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static const struct platform_device_id sl28cpld_gpio_id_table[] = { >> >> + {"sl28cpld-gpio", sl28cpld_gpio}, >> >> + {"sl28cpld-gpi", sl28cpld_gpi}, >> >> + {"sl28cpld-gpo", sl28cpld_gpo}, >> > >> > Could you explain this a bit more? Is this the same component with >> > input/output-only lines or three different components? >> >> These are actually three different components. Ie. you could have a >> flavour where you have one GPIO (sl28cpld-gpio) and two output-only >> ones (sl28cpld-gpo). Is that what you wanted to know? >> > > Yes, thanks. This could use some documentation in the bindings though. ok. thats easy ;) -michael > Bartosz > >> > >> >> +}; >> >> +MODULE_DEVICE_TABLE(platform, sl28cpld_gpio_id_table); >> >> + >> >> +static struct platform_driver sl28cpld_gpio_driver = { >> >> + .probe = sl28cpld_gpio_probe, >> >> + .id_table = sl28cpld_gpio_id_table, >> >> + .driver = { >> >> + .name = "sl28cpld-gpio", >> >> + }, >> >> +}; >> >> +module_platform_driver(sl28cpld_gpio_driver); >> >> + >> >> +MODULE_DESCRIPTION("sl28cpld GPIO Driver"); >> >> +MODULE_LICENSE("GPL"); >> > >> > I think you could use a MODULE_ALIAS() here if you want this module to >> > be loaded automatically by udev. >> >> ok, I'll look into that. >> >> thanks, >> -michael >> >> > >> >> -- >> >> 2.20.1 >> >> >> > >> > Best regards, >> > Bartosz Golaszewski
Powered by blists - more mailing lists