| Message ID | 20200604211039.12689-7-michael@walle.cc |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
On Fri, Jun 5, 2020 at 12:14 AM Michael Walle <michael@walle.cc> wrote: > Add support for the GPIO controller of the sl28 board management > controller. This driver is part of a multi-function device. > > A controller has 8 lines. There are three different flavors: > full-featured GPIO with interrupt support, input-only and output-only. ... > +#include <linux/of_device.h> I think also not needed. But wait... > + return devm_regmap_add_irq_chip_np(dev, dev_of_node(dev), regmap, It seems regmap needs to be converted to use fwnode. > + irq, IRQF_SHARED | IRQF_ONESHOT, 0, > + irq_chip, &gpio->irq_data); ... > + if (!pdev->dev.parent) > + return -ENODEV; Are we expecting to get shot into foot? I mean why we need this check? > + dev_id = platform_get_device_id(pdev); > + if (dev_id) > + type = dev_id->driver_data; Oh, no. In new code we don't need this. We have facilities to provide platform data in a form of fwnode. > + else > + type = (uintptr_t)of_device_get_match_data(&pdev->dev); So does this. device_get_match_data(). > + if (!type) > + return -ENODEV; ... > + if (irq_support && Why do you need this flag? Can't simple IRQ number be sufficient? > + device_property_read_bool(&pdev->dev, "interrupt-controller")) { > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + ret = sl28cpld_gpio_irq_init(&pdev->dev, gpio, regmap, > + base, irq); > + if (ret) > + return ret; > + > + config.irq_domain = regmap_irq_get_domain(gpio->irq_data); > + } ... > +static const struct of_device_id sl28cpld_gpio_of_match[] = { > + { .compatible = "kontron,sl28cpld-gpio", > + .data = (void *)SL28CPLD_GPIO }, > + { .compatible = "kontron,sl28cpld-gpi", > + .data = (void *)SL28CPLD_GPI }, > + { .compatible = "kontron,sl28cpld-gpo", > + .data = (void *)SL28CPLD_GPO }, All above can be twice less LOCs. > + {}, No comma. > +}; ... > + .name = KBUILD_MODNAME, This actually not good idea in long term. File name can change and break an ABI.
Am 2020-06-05 14:00, schrieb Andy Shevchenko: > On Fri, Jun 5, 2020 at 12:14 AM Michael Walle <michael@walle.cc> wrote: > >> Add support for the GPIO controller of the sl28 board management >> controller. This driver is part of a multi-function device. >> >> A controller has 8 lines. There are three different flavors: >> full-featured GPIO with interrupt support, input-only and output-only. > > ... > >> +#include <linux/of_device.h> > > I think also not needed. > But wait... > >> + return devm_regmap_add_irq_chip_np(dev, dev_of_node(dev), >> regmap, > > It seems regmap needs to be converted to use fwnode. Mhh, this _np functions was actually part of this series in the beginning. >> + irq, IRQF_SHARED | >> IRQF_ONESHOT, 0, >> + irq_chip, &gpio->irq_data); > > ... > >> + if (!pdev->dev.parent) >> + return -ENODEV; > > Are we expecting to get shot into foot? I mean why we need this check? Can we be sure, that we always have a parent node? You are the first which complains about this ;) There were some other comments to move this to the beginning of the function. > >> + dev_id = platform_get_device_id(pdev); >> + if (dev_id) >> + type = dev_id->driver_data; > > Oh, no. In new code we don't need this. We have facilities to provide > platform data in a form of fwnode. Ok I'll look into that. But I already have a question, so there are of_property_read_xx(), which seems to be the old functions, then there is device_property_read_xx() and fwnode_property_read_xx(). What is the difference between the latter two? > >> + else >> + type = >> (uintptr_t)of_device_get_match_data(&pdev->dev); > > So does this. device_get_match_data(). ok > >> + if (!type) >> + return -ENODEV; > > ... > >> + if (irq_support && > > Why do you need this flag? Can't simple IRQ number be sufficient? I want to make sure, the is no misconfiguration. Eg. only GPIO flavors which has irq_support set, have the additional interrupt registers. > >> + device_property_read_bool(&pdev->dev, >> "interrupt-controller")) { >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) >> + return irq; >> + >> + ret = sl28cpld_gpio_irq_init(&pdev->dev, gpio, regmap, >> + base, irq); >> + if (ret) >> + return ret; >> + >> + config.irq_domain = >> regmap_irq_get_domain(gpio->irq_data); >> + } > > ... > >> +static const struct of_device_id sl28cpld_gpio_of_match[] = { > >> + { .compatible = "kontron,sl28cpld-gpio", >> + .data = (void *)SL28CPLD_GPIO }, >> + { .compatible = "kontron,sl28cpld-gpi", >> + .data = (void *)SL28CPLD_GPI }, >> + { .compatible = "kontron,sl28cpld-gpo", >> + .data = (void *)SL28CPLD_GPO }, > > All above can be twice less LOCs. They are longer than 80 chars. Or do I miss something? > >> + {}, > > No comma. ok >> +}; > > > ... > >> + .name = KBUILD_MODNAME, > > This actually not good idea in long term. File name can change and > break an ABI. Ahh an explanation, why this is bad. Ok makes sense, although to be fair, .id_table should be used for the driver name matching. I'm not sure if this is used somewhere else, though.
On Fri, Jun 05, 2020 at 02:42:53PM +0200, Michael Walle wrote: > Am 2020-06-05 14:00, schrieb Andy Shevchenko: > > On Fri, Jun 5, 2020 at 12:14 AM Michael Walle <michael@walle.cc> wrote: > > > + return devm_regmap_add_irq_chip_np(dev, dev_of_node(dev), > > > regmap, > > > > It seems regmap needs to be converted to use fwnode. > > Mhh, this _np functions was actually part of this series in the > beginning. Then, please, make them fwnode aware rather than OF centric. > > > IRQF_ONESHOT, 0, > > > + irq_chip, &gpio->irq_data); ... > > > + dev_id = platform_get_device_id(pdev); > > > + if (dev_id) > > > + type = dev_id->driver_data; > > > > Oh, no. In new code we don't need this. We have facilities to provide > > platform data in a form of fwnode. > > Ok I'll look into that. > > But I already have a question, so there are of_property_read_xx(), which > seems to be the old functions, then there is device_property_read_xx() and > fwnode_property_read_xx(). What is the difference between the latter two? It's easy. device_*() requires struct device to be established for this, so, operates only against devices, while the fwnode_*() operates on pure data which might or might not be related to any devices. If you understand OF examples better, consider device node vs. child of such node. ... > > > + if (irq_support && > > > > Why do you need this flag? Can't simple IRQ number be sufficient? > > I want to make sure, the is no misconfiguration. Eg. only GPIO > flavors which has irq_support set, have the additional interrupt > registers. In gpio-dwapb, for example, we simple check two things: a) hardware limitation (if IRQ is assigned to a proper port) and b) if there is any IRQ comes from DT, ACPI, etc. > > > + device_property_read_bool(&pdev->dev, > > > "interrupt-controller")) { > > > + irq = platform_get_irq(pdev, 0); > > > + if (irq < 0) > > > + return irq; > > > + > > > + ret = sl28cpld_gpio_irq_init(&pdev->dev, gpio, regmap, > > > + base, irq); > > > + if (ret) > > > + return ret; > > > + > > > + config.irq_domain = > > > regmap_irq_get_domain(gpio->irq_data); > > > + } ... > > > + { .compatible = "kontron,sl28cpld-gpio", > > > + .data = (void *)SL28CPLD_GPIO }, > > > + { .compatible = "kontron,sl28cpld-gpi", > > > + .data = (void *)SL28CPLD_GPI }, > > > + { .compatible = "kontron,sl28cpld-gpo", > > > + .data = (void *)SL28CPLD_GPO }, > > > > All above can be twice less LOCs. > > They are longer than 80 chars. Or do I miss something? We have 100 :-) ... > > > + .name = KBUILD_MODNAME, > > > > This actually not good idea in long term. File name can change and break > > an ABI. > > Ahh an explanation, why this is bad. Ok makes sense, although to be fair, > .id_table should be used for the driver name matching. I'm not sure if > this is used somewhere else, though. I saw in my practice chain of renames for a driver. Now, if somebody somewhere would like to instantiate a platform driver by its name... Oops, ABI breakage. And of course using platform data for such device makes less sense.
Am 2020-06-05 15:15, schrieb Andy Shevchenko: > On Fri, Jun 05, 2020 at 02:42:53PM +0200, Michael Walle wrote: >> Am 2020-06-05 14:00, schrieb Andy Shevchenko: >> > On Fri, Jun 5, 2020 at 12:14 AM Michael Walle <michael@walle.cc> wrote: > >> > > + return devm_regmap_add_irq_chip_np(dev, dev_of_node(dev), >> > > regmap, >> > >> > It seems regmap needs to be converted to use fwnode. >> >> Mhh, this _np functions was actually part of this series in the >> beginning. > > Then, please, make them fwnode aware rather than OF centric. ok > >> > > IRQF_ONESHOT, 0, >> > > + irq_chip, &gpio->irq_data); > > ... > >> > > + dev_id = platform_get_device_id(pdev); >> > > + if (dev_id) >> > > + type = dev_id->driver_data; >> > >> > Oh, no. In new code we don't need this. We have facilities to provide >> > platform data in a form of fwnode. >> >> Ok I'll look into that. >> >> But I already have a question, so there are of_property_read_xx(), >> which >> seems to be the old functions, then there is device_property_read_xx() >> and >> fwnode_property_read_xx(). What is the difference between the latter >> two? > > It's easy. device_*() requires struct device to be established for > this, so, > operates only against devices, while the fwnode_*() operates on pure > data which > might or might not be related to any devices. If you understand OF > examples > better, consider device node vs. child of such node. Ahh thanks, got it. > > ... > >> > > + if (irq_support && >> > >> > Why do you need this flag? Can't simple IRQ number be sufficient? >> >> I want to make sure, the is no misconfiguration. Eg. only GPIO >> flavors which has irq_support set, have the additional interrupt >> registers. > > In gpio-dwapb, for example, we simple check two things: a) hardware > limitation > (if IRQ is assigned to a proper port) and b) if there is any IRQ comes > from DT, > ACPI, etc. I can't follow you here. irq_support is like your (a); or the "pp->idx == 0" in your example. >> > > + device_property_read_bool(&pdev->dev, >> > > "interrupt-controller")) { >> > > + irq = platform_get_irq(pdev, 0); >> > > + if (irq < 0) >> > > + return irq; >> > > + >> > > + ret = sl28cpld_gpio_irq_init(&pdev->dev, gpio, regmap, >> > > + base, irq); >> > > + if (ret) >> > > + return ret; >> > > + >> > > + config.irq_domain = >> > > regmap_irq_get_domain(gpio->irq_data); >> > > + } > > ... > >> > > + { .compatible = "kontron,sl28cpld-gpio", >> > > + .data = (void *)SL28CPLD_GPIO }, >> > > + { .compatible = "kontron,sl28cpld-gpi", >> > > + .data = (void *)SL28CPLD_GPI }, >> > > + { .compatible = "kontron,sl28cpld-gpo", >> > > + .data = (void *)SL28CPLD_GPO }, >> > >> > All above can be twice less LOCs. >> >> They are longer than 80 chars. Or do I miss something? > > We have 100 :-) oh come on, since 6 days *g* >> > > + .name = KBUILD_MODNAME, >> > >> > This actually not good idea in long term. File name can change and break >> > an ABI. >> >> Ahh an explanation, why this is bad. Ok makes sense, although to be >> fair, >> .id_table should be used for the driver name matching. I'm not sure if >> this is used somewhere else, though. > > I saw in my practice chain of renames for a driver. Now, if somebody > somewhere would like to instantiate a platform driver by its name... > Oops, ABI breakage. > > And of course using platform data for such device makes less sense. i just removed the id_table from all drivers anyways. -michael
On Fri, Jun 5, 2020 at 9:44 PM Michael Walle <michael@walle.cc> wrote: > Am 2020-06-05 15:15, schrieb Andy Shevchenko: > > On Fri, Jun 05, 2020 at 02:42:53PM +0200, Michael Walle wrote: > >> Am 2020-06-05 14:00, schrieb Andy Shevchenko: > >> > On Fri, Jun 5, 2020 at 12:14 AM Michael Walle <michael@walle.cc> wrote: ... > >> > > + if (irq_support && > >> > > >> > Why do you need this flag? Can't simple IRQ number be sufficient? > >> > >> I want to make sure, the is no misconfiguration. Eg. only GPIO > >> flavors which has irq_support set, have the additional interrupt > >> registers. > > > > In gpio-dwapb, for example, we simple check two things: a) hardware > > limitation > > (if IRQ is assigned to a proper port) and b) if there is any IRQ comes > > from DT, > > ACPI, etc. > > I can't follow you here. irq_support is like your (a); or the > "pp->idx == 0" in your example. And you have type already. Why do you need to duplicate it? Moreover, is it protection from wrong type to have interrupts? You can move this all stuff under corresponding switch-case. > >> > > + device_property_read_bool(&pdev->dev, > >> > > "interrupt-controller")) { > >> > > + irq = platform_get_irq(pdev, 0); > >> > > + if (irq < 0) > >> > > + return irq; > >> > > + > >> > > + ret = sl28cpld_gpio_irq_init(&pdev->dev, gpio, regmap, > >> > > + base, irq); > >> > > + if (ret) > >> > > + return ret; > >> > > + > >> > > + config.irq_domain = > >> > > regmap_irq_get_domain(gpio->irq_data);
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index bcacd9c74aa8..a325d2d619a8 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1215,6 +1215,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 + select GPIO_REGMAP + select GPIOLIB_IRQCHIP + 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 1e4894e0bf0f..152127a9b339 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -130,6 +130,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..800e218ee624 --- /dev/null +++ b/drivers/gpio/gpio-sl28cpld.c @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * sl28cpld GPIO driver. + * + * Copyright 2019 Michael Walle <michael@walle.cc> + */ + +#include <linux/device.h> +#include <linux/gpio/regmap.h> +#include <linux/gpio/driver.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +/* GPIO flavor */ +#define GPIO_REG_DIR 0x00 +#define GPIO_REG_OUT 0x01 +#define GPIO_REG_IN 0x02 +#define GPIO_REG_IE 0x03 +#define GPIO_REG_IP 0x04 + +/* input-only flavor */ +#define GPI_REG_IN 0x00 + +/* output-only flavor */ +#define GPO_REG_OUT 0x00 + +enum sl28cpld_gpio_type { + SL28CPLD_GPIO = 1, + SL28CPLD_GPI, + SL28CPLD_GPO, +}; + +struct sl28cpld_gpio { + struct regmap_irq_chip irq_chip; + struct regmap_irq_chip_data *irq_data; +}; + +static const struct regmap_irq sl28cpld_gpio_irqs[] = { + REGMAP_IRQ_REG_LINE(0, 8), + REGMAP_IRQ_REG_LINE(1, 8), + REGMAP_IRQ_REG_LINE(2, 8), + REGMAP_IRQ_REG_LINE(3, 8), + REGMAP_IRQ_REG_LINE(4, 8), + REGMAP_IRQ_REG_LINE(5, 8), + REGMAP_IRQ_REG_LINE(6, 8), + REGMAP_IRQ_REG_LINE(7, 8), +}; + +static int sl28cpld_gpio_irq_init(struct device *dev, + struct sl28cpld_gpio *gpio, + struct regmap *regmap, unsigned int base, + int irq) +{ + struct regmap_irq_chip *irq_chip = &gpio->irq_chip; + + irq_chip->name = "sl28cpld-gpio-irq", + irq_chip->irqs = sl28cpld_gpio_irqs; + irq_chip->num_irqs = ARRAY_SIZE(sl28cpld_gpio_irqs); + irq_chip->num_regs = 1; + irq_chip->status_base = base + GPIO_REG_IP; + irq_chip->mask_base = base + GPIO_REG_IE; + irq_chip->mask_invert = true, + irq_chip->ack_base = base + GPIO_REG_IP; + + return devm_regmap_add_irq_chip_np(dev, dev_of_node(dev), regmap, + irq, IRQF_SHARED | IRQF_ONESHOT, 0, + irq_chip, &gpio->irq_data); +} + +static int sl28cpld_gpio_probe(struct platform_device *pdev) +{ + const struct platform_device_id *dev_id; + struct gpio_regmap_config config = {0}; + enum sl28cpld_gpio_type type; + struct sl28cpld_gpio *gpio; + bool irq_support = false; + struct regmap *regmap; + int irq, ret; + u32 base; + + if (!pdev->dev.parent) + return -ENODEV; + + dev_id = platform_get_device_id(pdev); + if (dev_id) + type = dev_id->driver_data; + else + type = (uintptr_t)of_device_get_match_data(&pdev->dev); + if (!type) + return -ENODEV; + + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); + if (!gpio) + return -ENOMEM; + + ret = device_property_read_u32(&pdev->dev, "reg", &base); + if (ret) + return -EINVAL; + + regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!regmap) + return -ENODEV; + + config.regmap = regmap; + config.parent = &pdev->dev; + config.ngpio = 8; + + switch (type) { + case SL28CPLD_GPIO: + config.reg_dat_base = base + GPIO_REG_IN; + config.reg_set_base = base + GPIO_REG_OUT; + /* reg_dir_out_base might be zero */ + config.reg_dir_out_base = GPIO_REGMAP_ADDR(base + + GPIO_REG_DIR); + irq_support = true; + break; + case SL28CPLD_GPO: + config.reg_set_base = base + GPO_REG_OUT; + break; + case SL28CPLD_GPI: + config.reg_dat_base = base + GPI_REG_IN; + break; + default: + dev_err(&pdev->dev, "unknown type %d\n", type); + return -ENODEV; + } + + if (irq_support && + device_property_read_bool(&pdev->dev, "interrupt-controller")) { + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + ret = sl28cpld_gpio_irq_init(&pdev->dev, gpio, regmap, + base, irq); + if (ret) + return ret; + + config.irq_domain = regmap_irq_get_domain(gpio->irq_data); + } + + return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config)); +} + +static const struct of_device_id sl28cpld_gpio_of_match[] = { + { .compatible = "kontron,sl28cpld-gpio", + .data = (void *)SL28CPLD_GPIO }, + { .compatible = "kontron,sl28cpld-gpi", + .data = (void *)SL28CPLD_GPI }, + { .compatible = "kontron,sl28cpld-gpo", + .data = (void *)SL28CPLD_GPO }, + {}, +}; +MODULE_DEVICE_TABLE(of, sl28cpld_gpio_of_match); + +static const struct platform_device_id sl28cpld_gpio_id_table[] = { + { "sl28cpld-gpio", SL28CPLD_GPIO }, + { "sl28cpld-gpi", SL28CPLD_GPI }, + { "sl28cpld-gpo", SL28CPLD_GPO }, + {} +}; +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 = KBUILD_MODNAME, + .of_match_table = sl28cpld_gpio_of_match, + }, +}; +module_platform_driver(sl28cpld_gpio_driver); + +MODULE_DESCRIPTION("sl28cpld GPIO Driver"); +MODULE_AUTHOR("Michael Walle <michael@walle.cc>"); +MODULE_LICENSE("GPL");
Add support for the GPIO controller of the sl28 board management controller. This driver is part of a multi-function device. A controller has 8 lines. There are three different flavors: full-featured GPIO with interrupt support, input-only and output-only. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/gpio/Kconfig | 11 +++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-sl28cpld.c | 180 +++++++++++++++++++++++++++++++++++ 3 files changed, 192 insertions(+) create mode 100644 drivers/gpio/gpio-sl28cpld.c