| Message ID | 20250113-mdb-max7360-support-v3-4-9519b4acb0b1@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for MAX7360 | expand |
Hi Mathieu, thanks for your patch! On Mon, Jan 13, 2025 at 1:43 PM Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> wrote: > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller. > > Two sets of GPIOs are provided by the device: > - Up to 8 GPIOs, shared with the PWM and rotary encoder functionalities. > These GPIOs also provide interrupts on input changes. > - Up to 6 GPOs, on unused keypad columns pins. > > Co-developed-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> (...) > +#include <linux/gpio/consumer.h> Why? My most generic feedback is if you have looked at using select GPIO_REGMAP for this driver? The regmap utility library is very helpful, look how other driver selecting GPIO_REGMAP gets default implementations from the library just git grep GPIO_REGMAP drivers/gpio/ > +static void max7360_gpio_set_value(struct gpio_chip *gc, > + unsigned int pin, int state) > +{ > + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); > + int ret; > + > + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) { OK some custom stuff... > + int off = MAX7360_MAX_GPIO - (gc->ngpio - pin); > + > + ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PORTS, > + BIT(off), state ? BIT(off) : 0); Fairly standard. > + } else { > + ret = regmap_write(max7360_gpio->regmap, > + MAX7360_REG_PWMBASE + pin, state ? 0xFF : 0); > + } Some custom stuff. > +static int max7360_gpio_get_value(struct gpio_chip *gc, unsigned int pin) > +{ > + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); > + unsigned int val; > + int off; > + int ret; > + > + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) { > + off = MAX7360_MAX_GPIO - (gc->ngpio - pin); > + > + ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_PORTS, &val); > + } else { > + off = pin; > + ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOIN, &val); > + } > + > + if (ret) { > + dev_err(max7360_gpio->dev, "failed to read gpio-%d", pin); > + return ret; > + } > + > + return !!(val & BIT(off)); > +} Looks like stock template regmap-gpio. > +static int max7360_gpio_get_direction(struct gpio_chip *gc, unsigned int pin) > +{ > + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); > + unsigned int val; > + int ret; > + > + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) > + return GPIO_LINE_DIRECTION_OUT; > + > + ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL, &val); > + if (ret) { > + dev_err(max7360_gpio->dev, "failed to read gpio-%d direction", > + pin); > + return ret; > + } > + > + if (val & BIT(pin)) > + return GPIO_LINE_DIRECTION_OUT; > + > + return GPIO_LINE_DIRECTION_IN; > +} Dito. > +static int max7360_gpio_direction_input(struct gpio_chip *gc, unsigned int pin) > +{ > + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); > + int ret; > + > + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) > + return -EIO; > + > + ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL, > + BIT(pin), 0); > + if (ret) { > + dev_err(max7360_gpio->dev, "failed to set gpio-%d direction", > + pin); > + return ret; > + } > + > + return 0; > +} Dito. > +static int max7360_gpio_direction_output(struct gpio_chip *gc, unsigned int pin, > + int state) > +{ > + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); > + int ret; > + > + if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) { > + ret = regmap_write_bits(max7360_gpio->regmap, > + MAX7360_REG_GPIOCTRL, BIT(pin), > + BIT(pin)); > + if (ret) { > + dev_err(max7360_gpio->dev, > + "failed to set gpio-%d direction", pin); > + return ret; > + } > + } > + > + max7360_gpio_set_value(gc, pin, state); > + > + return 0; > +} Dito. > +static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin) > +{ > + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); > + > + /* > + * GPOs on COL pins (keypad columns) can always be requested: this > + * driver has full access to them, up to the number set in chip.ngpio. > + * GPIOs on PORT pins are shared with the PWM and rotary encoder > + * drivers: they have to be requested from the MFD driver. > + */ > + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) > + return 0; > + > + return max7360_port_pin_request(max7360_gpio->dev->parent, pin, true); > +} > + > +static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin) > +{ > + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); > + > + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) > + return; > + > + max7360_port_pin_request(max7360_gpio->dev->parent, pin, false); > +} The pin request looks a bit like a custom pin control implementation... But I think it's fine, pin control can be a bit heavy to implement on simple devices, but if there is elaborate muxing and config going on, pin control should be used. Yours, Linus Walleij
On Tue Jan 14, 2025 at 3:33 PM CET, Linus Walleij wrote: > On Mon, Jan 13, 2025 at 1:43 PM Mathieu Dubois-Briand > My most generic feedback is if you have looked at using > select GPIO_REGMAP for this driver? > > The regmap utility library is very helpful, look how other driver > selecting GPIO_REGMAP gets default implementations > from the library just git grep GPIO_REGMAP drivers/gpio/ > I tried to switch to GPIO_REGMAP and I really like it overall, as it does simplify a lot the code. However, I identified two features that I was not able to port so far: the request()/free() callbacks and the interrupts. So for the request()/free() callbacks, I cannot add them anymore, as they are set on the gpio_chip structure, and this structure is hidden behind the gpio_regmap structure. I could easily modify the gpio_regmap_config structure and gpio_regmap_register() to allow to provide these callbacks, but is this acceptable? Or should I switch to a different way to prevent concurrent use of the same pin? I saw you mentioned the possibility of defining pin control. On the IRQ side, before switching to GPIO_REGMAP, I was able to define the IRQ configuration using the irq member of the gpio_chip structure. This does create the IRQ domain for me in a quite straightforward way. Again, I will not be able to do that anymore, as the gpio_chip structure is hidden. I saw I can specify my own irq_domain in gpio_regmap_config, so that would be a way, but I was wondering if there is any way to have something as easy as previously. I had a quick look at existing drivers using GPIO_REGMAP and providing IRQ support: I believe they are all using REGMAP_IRQ. And I believe I cannot use REGMAP_IRQ here, as if I understood correctly, I would need to have a register telling me exactly on which GPIO I have a pending interrupt and I don't have such a thing: all I know is there was an interrupt related to the GPIOs, and then I have to compare each GPIO with the previous known state to know which pin is affected. Do you have any thought about this? Best regards, Mathieu
Hi Mathieu, On Fri, Jan 17, 2025 at 4:22 PM Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> wrote: > I tried to switch to GPIO_REGMAP and I really like it overall, as it > does simplify a lot the code. However, I identified two features that I > was not able to port so far: the request()/free() callbacks and the > interrupts. Thanks for looking into this! > So for the request()/free() callbacks, I cannot add them anymore, as > they are set on the gpio_chip structure, and this structure is hidden > behind the gpio_regmap structure. I could easily modify the > gpio_regmap_config structure and gpio_regmap_register() to allow to > provide these callbacks, but is this acceptable? Of course. The template is to be used for setting it up, just modify it if we need more from it. > On the IRQ side, before switching to GPIO_REGMAP, I was able to define > the IRQ configuration using the irq member of the gpio_chip structure. > This does create the IRQ domain for me in a quite straightforward way. > Again, I will not be able to do that anymore, as the gpio_chip structure > is hidden. (...) > I had a quick look at existing drivers using GPIO_REGMAP and providing > IRQ support: I believe they are all using REGMAP_IRQ. And I believe I > cannot use REGMAP_IRQ here, Then we need to modify GPIO_REGMAP so it's possible to pass in our own gpio_irq_chip, maybe another member in the config, simply? Yours, Linus Walleij
On Fri, Jan 17, 2025 at 04:22:33PM +0100, Mathieu Dubois-Briand wrote: > On Tue Jan 14, 2025 at 3:33 PM CET, Linus Walleij wrote: > > On Mon, Jan 13, 2025 at 1:43 PM Mathieu Dubois-Briand > > My most generic feedback is if you have looked at using > > select GPIO_REGMAP for this driver? > > > > The regmap utility library is very helpful, look how other driver > > selecting GPIO_REGMAP gets default implementations > > from the library just git grep GPIO_REGMAP drivers/gpio/ > > I tried to switch to GPIO_REGMAP and I really like it overall, as it > does simplify a lot the code. However, I identified two features that I > was not able to port so far: the request()/free() callbacks and the > interrupts. You can easily extend the config to provide both if needed. So, update gpio-regmap.c itself as a prerequisite. > So for the request()/free() callbacks, I cannot add them anymore, as > they are set on the gpio_chip structure, and this structure is hidden > behind the gpio_regmap structure. I could easily modify the > gpio_regmap_config structure and gpio_regmap_register() to allow to > provide these callbacks, but is this acceptable? Or should I switch to a > different way to prevent concurrent use of the same pin? I saw you > mentioned the possibility of defining pin control. > > On the IRQ side, before switching to GPIO_REGMAP, I was able to define > the IRQ configuration using the irq member of the gpio_chip structure. > This does create the IRQ domain for me in a quite straightforward way. > Again, I will not be able to do that anymore, as the gpio_chip structure > is hidden. Look how it's done in, e.g., drivers/gpio/gpio-104-idi-48.c. It's pretty straightforward. You create and register an IRQ chip with devm_regmap_add_irq_chip(). It creates a domain for you. > I saw I can specify my own irq_domain in gpio_regmap_config, so that > would be a way, but I was wondering if there is any way to have > something as easy as previously. > > I had a quick look at existing drivers using GPIO_REGMAP and providing > IRQ support: I believe they are all using REGMAP_IRQ. And I believe I > cannot use REGMAP_IRQ here, as if I understood correctly, I would need > to have a register telling me exactly on which GPIO I have a pending > interrupt and I don't have such a thing: all I know is there was an > interrupt related to the GPIOs, and then I have to compare each GPIO > with the previous known state to know which pin is affected. > > Do you have any thought about this? I briefly looked at the code of regmap IRQ and I don't see big impediments for your case. So, you seems to have mask, unmask, and input registers. What you most likely want to do is to use input as status (regmap IRQ supports configuration without status registers). What seems to be needed is the logic that uses previous state to handle a new one. This is not only this chip that may need this type of handling, so it will be beneficial for others that may be converted to use gpio-regmap.c. So, I don't think we need full bypass of the GPIO IRQ chip through gpio-regmap.c.
On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote: > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller. > > Two sets of GPIOs are provided by the device: > - Up to 8 GPIOs, shared with the PWM and rotary encoder functionalities. > These GPIOs also provide interrupts on input changes. > - Up to 6 GPOs, on unused keypad columns pins. ... > + depends on OF_GPIO This is wrong. You don't use it. ... > +#include <linux/of.h> Shouldn't be here. Instead it should be linux/property.h. ... > + /* MAX7360 generates interrupts but does not report which pins changed: > + * compare the pin value with the value they had in previous interrupt > + * and report interrupt if the change match the set IRQ type. > + */ /* * Wrong multi-line comment style. Consider using * this one as an example. This applies to all the comments * in the entire series. */ > + pending = val ^ max7360_gpio->in_values; > + for_each_set_bit(irqn, &pending, max7360_gpio->chip.ngpio) { > + type = max7360_gpio->irq_types[irqn]; > + if (max7360_gpio->masked_interrupts & BIT(irqn)) > + continue; > + if ((val & BIT(irqn)) && type == IRQ_TYPE_EDGE_FALLING) > + continue; > + if (!(val & BIT(irqn)) && type == IRQ_TYPE_EDGE_RISING) > + continue; > + gpio_irq = irq_find_mapping(max7360_gpio->chip.irq.domain, irqn); > + handle_nested_irq(gpio_irq); > + count++; You can also look how gpio-pca953x.c does this. It uses bitmaps all over the place. But with the gpio-regmap.c in use this should be much more simpler. > + } > + max7360_gpio->in_values = val; > + if (count == 0) count is redundant, Checking pending against 0 is enough (or in case of bitmaps, if it's longer than unsigned long, bitmap_empty() is what is here). > + return IRQ_NONE; > + > + return IRQ_HANDLED; ... > +static int max7360_gpio_probe(struct platform_device *pdev) > +{ > + struct max7360_gpio *max7360_gpio; > + struct platform_device *parent; > + unsigned int ngpios; > + unsigned int outconf; > + struct gpio_irq_chip *girq; > + unsigned long flags; > + int irq; > + int ret; > + if (!pdev->dev.parent) { > + dev_err(&pdev->dev, "no parent device\n"); > + return -ENODEV; > + } Probably redundant as one of the calls below may fail if parent is absent (see below for more). > + parent = to_platform_device(pdev->dev.parent); Why do you need this? Can't the fwnode be propagated to the children and then the respective APIs to be used? > + max7360_gpio = devm_kzalloc(&pdev->dev, sizeof(*max7360_gpio), > + GFP_KERNEL); With struct device *dev = &pdev->dev; at the beginning of the function this and other lines will become neater and shorter, in particular this becomes one line even for the strict 80 characters limit (however we have it being relaxed to 100 nowadays). > + if (!max7360_gpio) > + return -ENOMEM; > + if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) { > + dev_err(&pdev->dev, "Missing ngpios OF property\n"); > + return -ENODEV; > + } This is not needed, it is already done in GPIOLIB core. > + max7360_gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL); > + if (!max7360_gpio->regmap) { > + dev_err(&pdev->dev, "could not get parent regmap\n"); > + return -ENODEV; Here and everywhere in the ->probe() and related use return dev_err_probe(...); And drop unneeded curly braces. > + } > + max7360_gpio->dev = &pdev->dev; So, why do you need this dup? You can easily get it via GPIO chip, no? > + max7360_gpio->chip = max7360_gpio_chip; > + max7360_gpio->chip.ngpio = ngpios; > + max7360_gpio->chip.parent = &pdev->dev; > + max7360_gpio->chip.base = -1; > + max7360_gpio->gpio_function = (uintptr_t)device_get_match_data(&pdev->dev); > + > + dev_dbg(&pdev->dev, "gpio count: %d\n", max7360_gpio->chip.ngpio); > + > + if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) { > + /* Port GPIOs: set output mode configuration (constant-current > + * or not). > + * This property is optional. > + */ > + outconf = 0; > + ret = of_property_read_u32(pdev->dev.of_node, > + "maxim,constant-current-disable", > + &outconf); device_property_read_u32() > + if (ret && (ret != -EINVAL)) { > + dev_err(&pdev->dev, > + "Failed to read maxim,constant-current-disable OF property\n"); > + return -ENODEV; > + } > + > + regmap_write(max7360_gpio->regmap, MAX7360_REG_GPIOOUTM, outconf); > + } > + > + if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT && > + of_property_read_bool(pdev->dev.of_node, "interrupt-controller")) { > + /* Port GPIOs: declare IRQ chip, if configuration was provided. > + */ > + irq = platform_get_irq_byname(parent, "inti"); fwnode_irq_get_byname() with the propagated firmware node will give you the same result. > + if (irq < 0) > + return dev_err_probe(&pdev->dev, irq, > + "Failed to get IRQ\n"); > + > + flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED; > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, > + max7360_gpio_irq, flags, > + "max7360-gpio", max7360_gpio); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "Failed to register interrupt\n"); > + > + girq = &max7360_gpio->chip.irq; > + gpio_irq_chip_set_chip(girq, &max7360_gpio_irqchip); > + girq->parent_handler = NULL; > + girq->num_parents = 0; > + girq->parents = NULL; > + girq->threaded = true; > + girq->default_type = IRQ_TYPE_NONE; > + girq->handler = handle_simple_irq; > + } > + > + ret = devm_gpiochip_add_data(&pdev->dev, &max7360_gpio->chip, max7360_gpio); > + if (ret) { > + return dev_err_probe(&pdev->dev, ret, > + "unable to add gpiochip\n"); > + } > + > + return 0; > +} ... > +static struct platform_driver max7360_gpio_driver = { > + .driver = { > + .name = MAX7360_DRVNAME_GPIO, > + .of_match_table = of_match_ptr(max7360_gpio_of_match), No of_match_ptr() or ACPI_PTR() in new code, please. It appears to be more harmful than helpful. > + }, > + .probe = max7360_gpio_probe, > +}; > +module_platform_driver(max7360_gpio_driver); ... > +MODULE_ALIAS("platform:" MAX7360_DRVNAME_GPIO); Why? It doesn't look like it can be instantiated via board files. ... Overall seems many of my comments are applicable to your entire series (all patches), please address and feel free to Cc me on v4 for review.
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 93ee3aa092f8..efe07e21c442 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1444,6 +1444,17 @@ config GPIO_MADERA help Support for GPIOs on Cirrus Logic Madera class codecs. +config GPIO_MAX7360 + tristate "MAX7360 GPIO support" + depends on MFD_MAX7360 + depends on OF_GPIO + help + Allows to use MAX7360 I/O Expander PWM lines as GPIO and keypad COL + lines as GPO. + + This driver can also be built as a module. If so, the module will be + called gpio-max7360. + config GPIO_MAX77620 tristate "GPIO support for PMIC MAX77620 and MAX20024" depends on MFD_MAX77620 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index af3ba4d81b58..581341b3e3e4 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -100,6 +100,7 @@ obj-$(CONFIG_GPIO_MAX7300) += gpio-max7300.o obj-$(CONFIG_GPIO_MAX7301) += gpio-max7301.o obj-$(CONFIG_GPIO_MAX730X) += gpio-max730x.o obj-$(CONFIG_GPIO_MAX732X) += gpio-max732x.o +obj-$(CONFIG_GPIO_MAX7360) += gpio-max7360.o obj-$(CONFIG_GPIO_MAX77620) += gpio-max77620.o obj-$(CONFIG_GPIO_MAX77650) += gpio-max77650.o obj-$(CONFIG_GPIO_MB86S7X) += gpio-mb86s7x.o diff --git a/drivers/gpio/gpio-max7360.c b/drivers/gpio/gpio-max7360.c new file mode 100644 index 000000000000..9b701246689c --- /dev/null +++ b/drivers/gpio/gpio-max7360.c @@ -0,0 +1,454 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2024 Bootlin + * + * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com> + * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> + */ + +#include <linux/gpio/consumer.h> +#include <linux/gpio/driver.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/mfd/max7360.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#define MAX7360_GPIO_PORT 1 +#define MAX7360_GPIO_COL 2 + +struct max7360_gpio { + struct gpio_chip chip; + struct device *dev; + struct regmap *regmap; + unsigned long gpio_function; + + /* + * Interrupts handling data: only used when gpio_function is + * MAX7360_GPIO_PORT. + */ + u8 masked_interrupts; + u8 in_values; + unsigned int irq_types[MAX7360_MAX_GPIO]; +}; + +static void max7360_gpio_set_value(struct gpio_chip *gc, + unsigned int pin, int state) +{ + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); + int ret; + + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) { + int off = MAX7360_MAX_GPIO - (gc->ngpio - pin); + + ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PORTS, + BIT(off), state ? BIT(off) : 0); + } else { + ret = regmap_write(max7360_gpio->regmap, + MAX7360_REG_PWMBASE + pin, state ? 0xFF : 0); + } + + if (ret) + dev_err(max7360_gpio->dev, + "failed to set value %d on gpio-%d", state, pin); +} + +static int max7360_gpio_get_value(struct gpio_chip *gc, unsigned int pin) +{ + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); + unsigned int val; + int off; + int ret; + + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) { + off = MAX7360_MAX_GPIO - (gc->ngpio - pin); + + ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_PORTS, &val); + } else { + off = pin; + ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOIN, &val); + } + + if (ret) { + dev_err(max7360_gpio->dev, "failed to read gpio-%d", pin); + return ret; + } + + return !!(val & BIT(off)); +} + +static int max7360_gpio_get_direction(struct gpio_chip *gc, unsigned int pin) +{ + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); + unsigned int val; + int ret; + + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) + return GPIO_LINE_DIRECTION_OUT; + + ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL, &val); + if (ret) { + dev_err(max7360_gpio->dev, "failed to read gpio-%d direction", + pin); + return ret; + } + + if (val & BIT(pin)) + return GPIO_LINE_DIRECTION_OUT; + + return GPIO_LINE_DIRECTION_IN; +} + +static int max7360_gpio_direction_input(struct gpio_chip *gc, unsigned int pin) +{ + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); + int ret; + + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) + return -EIO; + + ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL, + BIT(pin), 0); + if (ret) { + dev_err(max7360_gpio->dev, "failed to set gpio-%d direction", + pin); + return ret; + } + + return 0; +} + +static int max7360_gpio_direction_output(struct gpio_chip *gc, unsigned int pin, + int state) +{ + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); + int ret; + + if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) { + ret = regmap_write_bits(max7360_gpio->regmap, + MAX7360_REG_GPIOCTRL, BIT(pin), + BIT(pin)); + if (ret) { + dev_err(max7360_gpio->dev, + "failed to set gpio-%d direction", pin); + return ret; + } + } + + max7360_gpio_set_value(gc, pin, state); + + return 0; +} + +static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin) +{ + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); + + /* + * GPOs on COL pins (keypad columns) can always be requested: this + * driver has full access to them, up to the number set in chip.ngpio. + * GPIOs on PORT pins are shared with the PWM and rotary encoder + * drivers: they have to be requested from the MFD driver. + */ + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) + return 0; + + return max7360_port_pin_request(max7360_gpio->dev->parent, pin, true); +} + +static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin) +{ + struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc); + + if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) + return; + + max7360_port_pin_request(max7360_gpio->dev->parent, pin, false); +} + +static struct gpio_chip max7360_gpio_chip = { + .label = "max7360", + .request = max7360_gpio_request, + .free = max7360_gpio_free, + .get_direction = max7360_gpio_get_direction, + .direction_input = max7360_gpio_direction_input, + .direction_output = max7360_gpio_direction_output, + .get = max7360_gpio_get_value, + .set = max7360_gpio_set_value, + .base = -1, + .can_sleep = true, +}; + +static irqreturn_t max7360_gpio_irq(int irq, void *data) +{ + struct max7360_gpio *max7360_gpio = data; + unsigned long pending; + unsigned int gpio_irq; + unsigned int type; + unsigned int count = 0; + int val; + int irqn; + int ret; + + ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOIN, &val); + if (ret) { + dev_err(max7360_gpio->dev, "Failed to read gpio values: %d\n", + ret); + return IRQ_NONE; + } + + /* MAX7360 generates interrupts but does not report which pins changed: + * compare the pin value with the value they had in previous interrupt + * and report interrupt if the change match the set IRQ type. + */ + pending = val ^ max7360_gpio->in_values; + for_each_set_bit(irqn, &pending, max7360_gpio->chip.ngpio) { + type = max7360_gpio->irq_types[irqn]; + if (max7360_gpio->masked_interrupts & BIT(irqn)) + continue; + if ((val & BIT(irqn)) && type == IRQ_TYPE_EDGE_FALLING) + continue; + if (!(val & BIT(irqn)) && type == IRQ_TYPE_EDGE_RISING) + continue; + gpio_irq = irq_find_mapping(max7360_gpio->chip.irq.domain, irqn); + handle_nested_irq(gpio_irq); + count++; + } + + max7360_gpio->in_values = val; + + if (count == 0) + return IRQ_NONE; + + return IRQ_HANDLED; +} + +static void max7360_gpio_irq_unmask(struct irq_data *data) +{ + struct max7360_gpio *max7360_gpio; + unsigned int pin = irqd_to_hwirq(data); + unsigned int val; + int ret; + + max7360_gpio = gpiochip_get_data(irq_data_get_irq_chip_data(data)); + + /* Read current pin value, so we know if the pin changed in the next + * interrupt. + * No lock should be needed regarding the interrupt handler: as long as + * the corresponding bit has not been cleared in masked_interrupts, this + * gpio is ignored. + */ + ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOIN, &val); + if (ret) + dev_err(max7360_gpio->dev, "Failed to read gpio values: %d\n", + ret); + + max7360_gpio->in_values &= ~BIT(pin); + max7360_gpio->in_values |= val & BIT(pin); + + ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PWMCFG + pin, + MAX7360_PORT_CFG_INTERRUPT_MASK, 0); + + if (ret) + dev_err(max7360_gpio->dev, "failed to unmask gpio-%d", pin); + + max7360_gpio->masked_interrupts &= ~BIT(pin); +} + +static void max7360_gpio_irq_mask(struct irq_data *data) +{ + struct max7360_gpio *max7360_gpio; + unsigned int pin = irqd_to_hwirq(data); + int ret; + + max7360_gpio = gpiochip_get_data(irq_data_get_irq_chip_data(data)); + + max7360_gpio->masked_interrupts |= BIT(pin); + + ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PWMCFG + pin, + MAX7360_PORT_CFG_INTERRUPT_MASK, + MAX7360_PORT_CFG_INTERRUPT_MASK); + + if (ret) + dev_err(max7360_gpio->dev, "failed to mask gpio-%d", pin); +} + +static void max7360_gpio_irq_enable(struct irq_data *data) +{ + max7360_gpio_irq_unmask(data); +} + +static void max7360_gpio_irq_disable(struct irq_data *data) +{ + max7360_gpio_irq_mask(data); +} + +static int max7360_gpio_irq_set_type(struct irq_data *data, + unsigned int flow_type) +{ + struct max7360_gpio *max7360_gpio; + unsigned int pin; + unsigned int val; + int ret; + + pin = irqd_to_hwirq(data); + max7360_gpio = gpiochip_get_data(irq_data_get_irq_chip_data(data)); + + switch (flow_type) { + case IRQ_TYPE_EDGE_RISING: + val = 0; + break; + case IRQ_TYPE_EDGE_FALLING: + case IRQ_TYPE_EDGE_BOTH: + val = MAX7360_PORT_CFG_INTERRUPT_EDGES; + break; + default: + return -EINVAL; + } + ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PWMCFG + pin, + MAX7360_PORT_CFG_INTERRUPT_EDGES, val); + + if (ret) + dev_err(max7360_gpio->dev, "failed to unmask gpio-%d", pin); + + max7360_gpio->irq_types[pin] = flow_type; + + return 0; +} + +static const struct irq_chip max7360_gpio_irqchip = { + .name = "max7360", + .irq_enable = max7360_gpio_irq_enable, + .irq_disable = max7360_gpio_irq_disable, + .irq_mask = max7360_gpio_irq_mask, + .irq_unmask = max7360_gpio_irq_unmask, + .irq_set_type = max7360_gpio_irq_set_type, + .flags = IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, +}; + +static int max7360_gpio_probe(struct platform_device *pdev) +{ + struct max7360_gpio *max7360_gpio; + struct platform_device *parent; + unsigned int ngpios; + unsigned int outconf; + struct gpio_irq_chip *girq; + unsigned long flags; + int irq; + int ret; + + if (!pdev->dev.parent) { + dev_err(&pdev->dev, "no parent device\n"); + return -ENODEV; + } + parent = to_platform_device(pdev->dev.parent); + + max7360_gpio = devm_kzalloc(&pdev->dev, sizeof(*max7360_gpio), + GFP_KERNEL); + if (!max7360_gpio) + return -ENOMEM; + + if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) { + dev_err(&pdev->dev, "Missing ngpios OF property\n"); + return -ENODEV; + } + + max7360_gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!max7360_gpio->regmap) { + dev_err(&pdev->dev, "could not get parent regmap\n"); + return -ENODEV; + } + + max7360_gpio->dev = &pdev->dev; + max7360_gpio->chip = max7360_gpio_chip; + max7360_gpio->chip.ngpio = ngpios; + max7360_gpio->chip.parent = &pdev->dev; + max7360_gpio->chip.base = -1; + max7360_gpio->gpio_function = (uintptr_t)device_get_match_data(&pdev->dev); + + dev_dbg(&pdev->dev, "gpio count: %d\n", max7360_gpio->chip.ngpio); + + if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) { + /* Port GPIOs: set output mode configuration (constant-current + * or not). + * This property is optional. + */ + outconf = 0; + ret = of_property_read_u32(pdev->dev.of_node, + "maxim,constant-current-disable", + &outconf); + if (ret && (ret != -EINVAL)) { + dev_err(&pdev->dev, + "Failed to read maxim,constant-current-disable OF property\n"); + return -ENODEV; + } + + regmap_write(max7360_gpio->regmap, MAX7360_REG_GPIOOUTM, outconf); + } + + if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT && + of_property_read_bool(pdev->dev.of_node, "interrupt-controller")) { + /* Port GPIOs: declare IRQ chip, if configuration was provided. + */ + irq = platform_get_irq_byname(parent, "inti"); + if (irq < 0) + return dev_err_probe(&pdev->dev, irq, + "Failed to get IRQ\n"); + + flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED; + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, + max7360_gpio_irq, flags, + "max7360-gpio", max7360_gpio); + if (ret) + return dev_err_probe(&pdev->dev, ret, + "Failed to register interrupt\n"); + + girq = &max7360_gpio->chip.irq; + gpio_irq_chip_set_chip(girq, &max7360_gpio_irqchip); + girq->parent_handler = NULL; + girq->num_parents = 0; + girq->parents = NULL; + girq->threaded = true; + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_simple_irq; + } + + ret = devm_gpiochip_add_data(&pdev->dev, &max7360_gpio->chip, max7360_gpio); + if (ret) { + return dev_err_probe(&pdev->dev, ret, + "unable to add gpiochip\n"); + } + + return 0; +} + +static const struct of_device_id max7360_gpio_of_match[] = { + { + .compatible = "maxim,max7360-gpo", + .data = (void *)MAX7360_GPIO_COL + }, { + .compatible = "maxim,max7360-gpio", + .data = (void *)MAX7360_GPIO_PORT + }, { + } +}; +MODULE_DEVICE_TABLE(of, max7360_gpio_of_match); + +static struct platform_driver max7360_gpio_driver = { + .driver = { + .name = MAX7360_DRVNAME_GPIO, + .of_match_table = of_match_ptr(max7360_gpio_of_match), + }, + .probe = max7360_gpio_probe, +}; +module_platform_driver(max7360_gpio_driver); + +MODULE_DESCRIPTION("MAX7360 GPIO driver"); +MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>"); +MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>"); +MODULE_ALIAS("platform:" MAX7360_DRVNAME_GPIO); +MODULE_LICENSE("GPL");