| 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: > 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/ > Thanks, I was not aware of that. I tested it and I should be able to get rid of a lot of code using GPIO_REGMAP. My main concern so far is with the request()/free() functions, as I believe I will not be able to define them as callback anymore. I also saw the equivalent REGMAP_IRQ, but I'm not sure I will be able to use it, as I believe I would need to have registers identifying the exact GPIO source of the IRQ. > > +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. Just so remove any doubt, all this does is request the pin for the exclusive use of this driver, preventing the PWM or rotary encoder drivers to use it. There is no hardware configuration done here. Yet I agree that this does look a bit like some pin muxing. > > Yours, > Linus Walleij Thanks for your review.
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.
Hi Andy, Thanks for your review. I've been addressing most of your comments in this mail and the ones related to regmap-irq. I should be able to send a new version in a few days. However I have a few questions regarding some of the points. On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote: > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote: > > + 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? > I'm not sure to understand this correctly, what do you mean by propagating the fwnode to the children? Just a quick summary of the situation and what I try to do. The device tree looks like this, only keeping the interesting properties: io-expander@38 { ... interrupts = <23 IRQ_TYPE_LEVEL_LOW>, <24 IRQ_TYPE_LEVEL_LOW>; interrupt-names = "inti", "intk"; max7360_gpio: gpio { ... }; max7360_gpo: gpo { ... }; }; Our pdev fwnode points either to the "gpio" or "gpo" nodes, the one from our parent device points to "io-expander@38". Here we need to get the "inti" interrupt from the parent node. What would be the correct way to do it? > > + 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. > I believe this is still needed: - For gpos, we need the gpio count to correctly set the partition between gpo and keypad columns in max7360_set_gpos_count(). - For gpios, we need the gpio count to setup the IRQs. Best regards, Mathieu
On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote: > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote: > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote: ... > > > + 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? > > I'm not sure to understand this correctly, what do you mean by > propagating the fwnode to the children? > > Just a quick summary of the situation and what I try to do. The device > tree looks like this, only keeping the interesting properties: > > io-expander@38 { > ... > interrupts = <23 IRQ_TYPE_LEVEL_LOW>, > <24 IRQ_TYPE_LEVEL_LOW>; > interrupt-names = "inti", "intk"; > > max7360_gpio: gpio { > ... > }; > > max7360_gpo: gpo { > ... > }; > }; > > Our pdev fwnode points either to the "gpio" or "gpo" nodes, the one from > our parent device points to "io-expander@38". Here we need to get the > "inti" interrupt from the parent node. What would be the correct way to > do it? Ah, I see now. This is being used only for IRQs, but don't you want to call actually fwnode_irq_get_byname()? It will makes the intention clearer. ... > > > + 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. > > I believe this is still needed: > - For gpos, we need the gpio count to correctly set the partition > between gpo and keypad columns in max7360_set_gpos_count(). Shouldn't be that done somewhere in the GPIO valid mask initialisation? > - For gpios, we need the gpio count to setup the IRQs. Doesn't GPIOLIB parse the property before initializing the IRQ valid mask and other init callbacks?
On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote: > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote: > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote: > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote: > > ... > > > > > + 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? > > > > I'm not sure to understand this correctly, what do you mean by > > propagating the fwnode to the children? > > > > Just a quick summary of the situation and what I try to do. The device > > tree looks like this, only keeping the interesting properties: > > > > io-expander@38 { > > ... > > interrupts = <23 IRQ_TYPE_LEVEL_LOW>, > > <24 IRQ_TYPE_LEVEL_LOW>; > > interrupt-names = "inti", "intk"; > > > > max7360_gpio: gpio { > > ... > > }; > > > > max7360_gpo: gpo { > > ... > > }; > > }; > > > > Our pdev fwnode points either to the "gpio" or "gpo" nodes, the one from > > our parent device points to "io-expander@38". Here we need to get the > > "inti" interrupt from the parent node. What would be the correct way to > > do it? > > Ah, I see now. This is being used only for IRQs, but don't you want to call > actually fwnode_irq_get_byname()? It will makes the intention clearer. > Sure! I can definitely call fwnode_irq_get_byname(). > > > > > + 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. > > > > I believe this is still needed: > > - For gpos, we need the gpio count to correctly set the partition > > between gpo and keypad columns in max7360_set_gpos_count(). > > Shouldn't be that done somewhere in the GPIO valid mask initialisation? > > > - For gpios, we need the gpio count to setup the IRQs. > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask > and other init callbacks? No, I believe I have to register the IRQ before registering the GPIO, so I can get the IRQ domain. Right now I have something like: irq_chip->num_irqs = ngpios; devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap, irq, flags, 0, irq_chip, &irq_chip_data); gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data); devm_gpio_regmap_register(dev, &gpio_config); Also, gpiolib will store ngpios in the gpio_chip structure, but while using gpio-regmap, this structure is masked behind the opaque gpio_regmap structure. So I believe there is no easy way to retrieve its value. This part of the code changed a lot, maybe it would be easier if I push a new version of the series and we continue the discussion there?
On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote: > On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote: > > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote: > > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote: > > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote: ... > > > > > + 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. > > > > > > I believe this is still needed: > > > - For gpos, we need the gpio count to correctly set the partition > > > between gpo and keypad columns in max7360_set_gpos_count(). > > > > Shouldn't be that done somewhere in the GPIO valid mask initialisation? > > > > > - For gpios, we need the gpio count to setup the IRQs. > > > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask > > and other init callbacks? > > No, I believe I have to register the IRQ before registering the GPIO, so > I can get the IRQ domain. > > Right now I have something like: > > irq_chip->num_irqs = ngpios; > devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap, > irq, flags, 0, irq_chip, &irq_chip_data); > gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data); > devm_gpio_regmap_register(dev, &gpio_config); > > Also, gpiolib will store ngpios in the gpio_chip structure, but while > using gpio-regmap, this structure is masked behind the opaque > gpio_regmap structure. So I believe there is no easy way to retrieve its > value. > > This part of the code changed a lot, maybe it would be easier if I push > a new version of the series and we continue the discussion there? So, what seems need to be added is some flag to GPIO regmap configuration data structure and a code that is called after gpiochip_add_data() in gpio_regmap_register() to create a domain. This will solve the above issue and helps other drivers to get rid of potential duplication of devm_regmap_add_irq_chip_fwnode() calls. Have you researched this path?
On Wed Feb 12, 2025 at 5:17 PM CET, Andy Shevchenko wrote: > On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote: > > On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote: > > > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote: > > > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote: > > > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote: > > ... > > > > > > > + 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. > > > > > > > > I believe this is still needed: > > > > - For gpos, we need the gpio count to correctly set the partition > > > > between gpo and keypad columns in max7360_set_gpos_count(). > > > > > > Shouldn't be that done somewhere in the GPIO valid mask initialisation? > > > > > > > - For gpios, we need the gpio count to setup the IRQs. > > > > > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask > > > and other init callbacks? > > > > No, I believe I have to register the IRQ before registering the GPIO, so > > I can get the IRQ domain. > > > > Right now I have something like: > > > > irq_chip->num_irqs = ngpios; > > devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap, > > irq, flags, 0, irq_chip, &irq_chip_data); > > gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data); > > devm_gpio_regmap_register(dev, &gpio_config); > > > > Also, gpiolib will store ngpios in the gpio_chip structure, but while > > using gpio-regmap, this structure is masked behind the opaque > > gpio_regmap structure. So I believe there is no easy way to retrieve its > > value. > > > > This part of the code changed a lot, maybe it would be easier if I push > > a new version of the series and we continue the discussion there? > > So, what seems need to be added is some flag to GPIO regmap configuration > data structure and a code that is called after gpiochip_add_data() in > gpio_regmap_register() to create a domain. This will solve the above issue > and helps other drivers to get rid of potential duplication of > devm_regmap_add_irq_chip_fwnode() calls. > > Have you researched this path? OK, so looking at the code, I believe it would need to: - Add some flag in gpio_regmap_config structure, so gpio_regmap_register() creates a new IRQ domain. - Add a function allowing to retrieve this domain out of the gpio_regmap structure. - Allow to pass a domain in the regmap_irq_chip structure, so regmap_add_irq_chip_fwnode() use this domain instead of calling regmap_irq_create_domain(). - Make sure this domain is still populated with the IRQ data: number of IRQs, IRQ base but also a pointer on the regmap_irq_chip_data structure in .host_data. I believe this will be a bit tricky. - Add a function allowing to retrieve ngpio out of the gpio_regmap.gpio_chip structure, so it can be used for IRQ setup and other places of the driver. I'm sorry, but I feel like this is a lot of changes to solve this point. I've been thinking about it, and I can suggest a different solution. For gpios, I will remove the ngpios property of the device tree and use a fixed value: - For the today version of the chip, this is always 8. - I a chip variant or a similar chip ever arise later with a different number of gpios, the fixed value can be set according to the "compatible" value. - This removes any issue with the IRQ setup. For gpos, we have to keep ngpios, as it depends of the implementation on the board. That means ngpios will be used: - For the gpio chip configuration: we let gpiolib retrieve it from the device tree. - In gpio-regmap reg_mask_xlate callback: I can add a function allowing to retrieve it from gpio_regmap.gpio_chip, as suggested above. - In max7360_set_gpos_count() to validate the coherency between requested gpios and keypad columns and set the correct configuration on the chip: - I can also retrieve the value from gpio_regmap.gpio_chip, but that means the check is made after the call to devm_gpio_regmap_register(). - Or I will still need to retrieve it using device_property_read_u32() here. How do you feel about this solution?
On Thu Feb 13, 2025 at 11:59 AM CET, Mathieu Dubois-Briand wrote: > On Wed Feb 12, 2025 at 5:17 PM CET, Andy Shevchenko wrote: > > On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote: > > > On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote: > > > > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote: > > > > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote: > > > > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote: > > > > ... > > > > > > > > > + 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. > > > > > > > > > > I believe this is still needed: > > > > > - For gpos, we need the gpio count to correctly set the partition > > > > > between gpo and keypad columns in max7360_set_gpos_count(). > > > > > > > > Shouldn't be that done somewhere in the GPIO valid mask initialisation? > > > > > > > > > - For gpios, we need the gpio count to setup the IRQs. > > > > > > > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask > > > > and other init callbacks? > > > > > > No, I believe I have to register the IRQ before registering the GPIO, so > > > I can get the IRQ domain. > > > > > > Right now I have something like: > > > > > > irq_chip->num_irqs = ngpios; > > > devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap, > > > irq, flags, 0, irq_chip, &irq_chip_data); > > > gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data); > > > devm_gpio_regmap_register(dev, &gpio_config); > > > > > > Also, gpiolib will store ngpios in the gpio_chip structure, but while > > > using gpio-regmap, this structure is masked behind the opaque > > > gpio_regmap structure. So I believe there is no easy way to retrieve its > > > value. > > > > > > This part of the code changed a lot, maybe it would be easier if I push > > > a new version of the series and we continue the discussion there? > > > > So, what seems need to be added is some flag to GPIO regmap configuration > > data structure and a code that is called after gpiochip_add_data() in > > gpio_regmap_register() to create a domain. This will solve the above issue > > and helps other drivers to get rid of potential duplication of > > devm_regmap_add_irq_chip_fwnode() calls. > > > > Have you researched this path? > > OK, so looking at the code, I believe it would need to: > - Add some flag in gpio_regmap_config structure, so > gpio_regmap_register() creates a new IRQ domain. > - Add a function allowing to retrieve this domain out of the gpio_regmap > structure. > - Allow to pass a domain in the regmap_irq_chip structure, so > regmap_add_irq_chip_fwnode() use this domain instead of calling > regmap_irq_create_domain(). > - Make sure this domain is still populated with the IRQ data: number of > IRQs, IRQ base but also a pointer on the regmap_irq_chip_data > structure in .host_data. I believe this will be a bit tricky. > - Add a function allowing to retrieve ngpio out of the > gpio_regmap.gpio_chip structure, so it can be used for IRQ setup and > other places of the driver. > > I'm sorry, but I feel like this is a lot of changes to solve this point. > I've been thinking about it, and I can suggest a different solution. > > For gpios, I will remove the ngpios property of the device tree and use > a fixed value: > - For the today version of the chip, this is always 8. > - I a chip variant or a similar chip ever arise later with a different > number of gpios, the fixed value can be set according to the > "compatible" value. > - This removes any issue with the IRQ setup. > > For gpos, we have to keep ngpios, as it depends of the implementation on > the board. That means ngpios will be used: > - For the gpio chip configuration: we let gpiolib retrieve it from the > device tree. > - In gpio-regmap reg_mask_xlate callback: I can add a function allowing > to retrieve it from gpio_regmap.gpio_chip, as suggested above. > - In max7360_set_gpos_count() to validate the coherency between > requested gpios and keypad columns and set the correct configuration > on the chip: > - I can also retrieve the value from gpio_regmap.gpio_chip, but that > means the check is made after the call to > devm_gpio_regmap_register(). > - Or I will still need to retrieve it using device_property_read_u32() > here. > > How do you feel about this solution? Actually there is an additional issue: today, relying on gpiolib to parse the "ngpios" property does not work with gpio-regmap. The gpiochip_get_ngpios() function in gpiolib is called in gpiochip_add_data_with_key(), but when using gpio_regmap_register(), we first ensure ngpio is set correctly before calling anything. Yet I believe this check can safely be removed, allowing the magic in gpiolib happen as expected.
On Thu, Feb 13, 2025 at 02:45:31PM +0100, Mathieu Dubois-Briand wrote: > On Thu Feb 13, 2025 at 11:59 AM CET, Mathieu Dubois-Briand wrote: > > On Wed Feb 12, 2025 at 5:17 PM CET, Andy Shevchenko wrote: > > > On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote: > > > > On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote: > > > > > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote: > > > > > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote: > > > > > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote: ... > > > > > > > > + 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. > > > > > > > > > > > > I believe this is still needed: > > > > > > - For gpos, we need the gpio count to correctly set the partition > > > > > > between gpo and keypad columns in max7360_set_gpos_count(). > > > > > > > > > > Shouldn't be that done somewhere in the GPIO valid mask initialisation? > > > > > > > > > > > - For gpios, we need the gpio count to setup the IRQs. > > > > > > > > > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask > > > > > and other init callbacks? > > > > > > > > No, I believe I have to register the IRQ before registering the GPIO, so > > > > I can get the IRQ domain. > > > > > > > > Right now I have something like: > > > > > > > > irq_chip->num_irqs = ngpios; > > > > devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap, > > > > irq, flags, 0, irq_chip, &irq_chip_data); > > > > gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data); > > > > devm_gpio_regmap_register(dev, &gpio_config); > > > > > > > > Also, gpiolib will store ngpios in the gpio_chip structure, but while > > > > using gpio-regmap, this structure is masked behind the opaque > > > > gpio_regmap structure. So I believe there is no easy way to retrieve its > > > > value. Would it be needed in your driver ->probe() after all? (See also below) > > > > This part of the code changed a lot, maybe it would be easier if I push > > > > a new version of the series and we continue the discussion there? > > > > > > So, what seems need to be added is some flag to GPIO regmap configuration > > > data structure and a code that is called after gpiochip_add_data() in > > > gpio_regmap_register() to create a domain. This will solve the above issue > > > and helps other drivers to get rid of potential duplication of > > > devm_regmap_add_irq_chip_fwnode() calls. > > > > > > Have you researched this path? > > > > OK, so looking at the code, I believe it would need to: > > - Add some flag in gpio_regmap_config structure, so > > gpio_regmap_register() creates a new IRQ domain. Easy. > > - Add a function allowing to retrieve this domain out of the gpio_regmap > > structure. Easy, as there is an API available for regmaps, so it looks like one-liner. > > - Allow to pass a domain in the regmap_irq_chip structure, so > > regmap_add_irq_chip_fwnode() use this domain instead of calling > > regmap_irq_create_domain(). You need this because of...? (Please, remind me what the obstacle is there that requires this to be done) > > - Make sure this domain is still populated with the IRQ data: number of > > IRQs, IRQ base but also a pointer on the regmap_irq_chip_data > > structure in .host_data. I believe this will be a bit tricky. Hmm... But wouldn't gpio-regmap internals have all this information available? > > - Add a function allowing to retrieve ngpio out of the > > gpio_regmap.gpio_chip structure, so it can be used for IRQ setup and > > other places of the driver. I'm not sure where it can be needed. > > I'm sorry, but I feel like this is a lot of changes to solve this point. > > I've been thinking about it, and I can suggest a different solution. > > > > For gpios, I will remove the ngpios property of the device tree and use > > a fixed value: > > - For the today version of the chip, this is always 8. > > - I a chip variant or a similar chip ever arise later with a different > > number of gpios, the fixed value can be set according to the > > "compatible" value. > > - This removes any issue with the IRQ setup. > > > > For gpos, we have to keep ngpios, as it depends of the implementation on > > the board. That means ngpios will be used: > > - For the gpio chip configuration: we let gpiolib retrieve it from the > > device tree. > > - In gpio-regmap reg_mask_xlate callback: I can add a function allowing > > to retrieve it from gpio_regmap.gpio_chip, as suggested above. > > - In max7360_set_gpos_count() to validate the coherency between > > requested gpios and keypad columns and set the correct configuration > > on the chip: > > - I can also retrieve the value from gpio_regmap.gpio_chip, but that > > means the check is made after the call to > > devm_gpio_regmap_register(). > > - Or I will still need to retrieve it using device_property_read_u32() > > here. > > > > How do you feel about this solution? > > Actually there is an additional issue: today, relying on gpiolib to > parse the "ngpios" property does not work with gpio-regmap. > > The gpiochip_get_ngpios() function in gpiolib is called in > gpiochip_add_data_with_key(), but when using gpio_regmap_register(), > we first ensure ngpio is set correctly before calling anything. > > Yet I believe this check can safely be removed, allowing the magic in > gpiolib happen as expected. Not really. I'm about to send a series to address this issue. Please, test. ... P.S. Maybe it's time to send a new version based on this discussion even if not finished / working, so we can see the exact issues we still have and target them.
On Thu Feb 13, 2025 at 8:47 PM CET, Andy Shevchenko wrote: > On Thu, Feb 13, 2025 at 02:45:31PM +0100, Mathieu Dubois-Briand wrote: > > On Thu Feb 13, 2025 at 11:59 AM CET, Mathieu Dubois-Briand wrote: > > > On Wed Feb 12, 2025 at 5:17 PM CET, Andy Shevchenko wrote: > > > > On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote: > > > > > On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote: > > > > > > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote: > > > > > > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote: > > > > > > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote: > > ... > > > > > > > > > > + 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. > > > > > > > > > > > > > > I believe this is still needed: > > > > > > > - For gpos, we need the gpio count to correctly set the partition > > > > > > > between gpo and keypad columns in max7360_set_gpos_count(). > > > > > > > > > > > > Shouldn't be that done somewhere in the GPIO valid mask initialisation? > > > > > > > > > > > > > - For gpios, we need the gpio count to setup the IRQs. > > > > > > > > > > > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask > > > > > > and other init callbacks? > > > > > > > > > > No, I believe I have to register the IRQ before registering the GPIO, so > > > > > I can get the IRQ domain. > > > > > > > > > > Right now I have something like: > > > > > > > > > > irq_chip->num_irqs = ngpios; > > > > > devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap, > > > > > irq, flags, 0, irq_chip, &irq_chip_data); > > > > > gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data); > > > > > devm_gpio_regmap_register(dev, &gpio_config); > > > > > > > > > > Also, gpiolib will store ngpios in the gpio_chip structure, but while > > > > > using gpio-regmap, this structure is masked behind the opaque > > > > > gpio_regmap structure. So I believe there is no easy way to retrieve its > > > > > value. > > Would it be needed in your driver ->probe() after all? (See also below) > No necessarily in the probe with the changes previously described, but I will need it in other functions. So either I get it in the probe and store it, or I will need to retrieve it by other means. > > > > > This part of the code changed a lot, maybe it would be easier if I push > > > > > a new version of the series and we continue the discussion there? > > > > > > > > So, what seems need to be added is some flag to GPIO regmap configuration > > > > data structure and a code that is called after gpiochip_add_data() in > > > > gpio_regmap_register() to create a domain. This will solve the above issue > > > > and helps other drivers to get rid of potential duplication of > > > > devm_regmap_add_irq_chip_fwnode() calls. > > > > > > > > Have you researched this path? > > > > > > OK, so looking at the code, I believe it would need to: > > > - Add some flag in gpio_regmap_config structure, so > > > gpio_regmap_register() creates a new IRQ domain. > > Easy. > > > > - Add a function allowing to retrieve this domain out of the gpio_regmap > > > structure. > > Easy, as there is an API available for regmaps, so it looks like one-liner. > > > > - Allow to pass a domain in the regmap_irq_chip structure, so > > > regmap_add_irq_chip_fwnode() use this domain instead of calling > > > regmap_irq_create_domain(). > > You need this because of...? (Please, remind me what the obstacle is there > that requires this to be done) > OK, maybe I misunderstood you idea. Or maybe I misunderstood something about IRQ domains. So what I understood is, in the probe, we first call gpio_regmap_register(), that will create a new IRQ domain, and we call regmap_add_irq_chip_fwnode() in second. But this second call will again create an IRQ domain, so we would end-up with a second one. We have to prevent this second creation and make it use the one we created first. Did I miss something? > > > - Make sure this domain is still populated with the IRQ data: number of > > > IRQs, IRQ base but also a pointer on the regmap_irq_chip_data > > > structure in .host_data. I believe this will be a bit tricky. > > Hmm... But wouldn't gpio-regmap internals have all this information available? > I don't think so. It will not know the IRQ base nor the regmap_irq_chip_data as it is not created at this point. > > > - Add a function allowing to retrieve ngpio out of the > > > gpio_regmap.gpio_chip structure, so it can be used for IRQ setup and > > > other places of the driver. > > I'm not sure where it can be needed. > Let's discuss this on the next version, but yes, I do need to know ngpio in the driver. > > > I'm sorry, but I feel like this is a lot of changes to solve this point. > > > I've been thinking about it, and I can suggest a different solution. > > > > > > For gpios, I will remove the ngpios property of the device tree and use > > > a fixed value: > > > - For the today version of the chip, this is always 8. > > > - I a chip variant or a similar chip ever arise later with a different > > > number of gpios, the fixed value can be set according to the > > > "compatible" value. > > > - This removes any issue with the IRQ setup. > > > > > > For gpos, we have to keep ngpios, as it depends of the implementation on > > > the board. That means ngpios will be used: > > > - For the gpio chip configuration: we let gpiolib retrieve it from the > > > device tree. > > > - In gpio-regmap reg_mask_xlate callback: I can add a function allowing > > > to retrieve it from gpio_regmap.gpio_chip, as suggested above. > > > - In max7360_set_gpos_count() to validate the coherency between > > > requested gpios and keypad columns and set the correct configuration > > > on the chip: > > > - I can also retrieve the value from gpio_regmap.gpio_chip, but that > > > means the check is made after the call to > > > devm_gpio_regmap_register(). > > > - Or I will still need to retrieve it using device_property_read_u32() > > > here. > > > > > > How do you feel about this solution? > > > > Actually there is an additional issue: today, relying on gpiolib to > > parse the "ngpios" property does not work with gpio-regmap. > > > > The gpiochip_get_ngpios() function in gpiolib is called in > > gpiochip_add_data_with_key(), but when using gpio_regmap_register(), > > we first ensure ngpio is set correctly before calling anything. > > > > Yet I believe this check can safely be removed, allowing the magic in > > gpiolib happen as expected. > > Not really. I'm about to send a series to address this issue. > Please, test. I will test it today. > > ... > > P.S. > Maybe it's time to send a new version based on this discussion even > if not finished / working, so we can see the exact issues we still have > and target them. Sure, I will send a new version, probably today. Thanks again for your help.
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");