| Message ID | 20250214-mdb-max7360-support-v4-7-8a35c6dbb966@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for MAX7360 | expand |
On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote: > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote: > > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller. > > ... > > > +static int max7360_gpo_reg_mask_xlate(struct gpio_regmap *gpio, > > + unsigned int base, unsigned int offset, > > + unsigned int *reg, unsigned int *mask) > > +{ > > + u16 ngpios = gpio_regmap_get_ngpio(gpio); > > + > > + *reg = base; > > + *mask = BIT(MAX7360_MAX_KEY_COLS - (ngpios - offset)); > > + > > + return 0; > > Does this GPIO controller only capable of servicing keypads? > I think no, hence I'm not sure why this split is needed to be > here and not in the input driver. > I would say it's more a keypad controller able to support some GPIOs. Some of the keypad columns, if unused, can be used as output-only gpios. So I believe the split has its place here, because in the default configuration, the split is set to have 8 keypad columns and no gpio. As a consequence, the keypad driver can work without having to worry about the split; the gpio driver needs to know about it. To provide a bit more details, there is basically two set of pins usable as GPIOs. On one side we have what I refer to as GPIOs: - PORT0 to PORT7 pins of the chip. - Shared with PWM and rotary encoder functionalities. Functionality selection can be made independently for each pin. We have to ensure the same pin is not used by two drivers at the same time. E.g. we cannot have at the same time GPIO4 and PWM4. - Supports input and interrupts. - Outputs may be configured as constant current. - 8 GPIOS supported, so ngpios is fixed to MAX7360_MAX_GPIO. - maxim,max7360-gpio compatible, gpio_function == MAX7360_GPIO_PORT. On the other side, we have what I refer to as GPOs: - COL2 to COL7 pins of the chip. - Shared with the keypad functionality. Selections is made by partitioning the pins: first pins for keypad columns, last pins for GPOs. Partition is described by the ngpios property. - Only support outputs. - maxim,max7360-gpo compatible, gpio_function == MAX7360_GPIO_COL. > Or you mean that there output only GPIO lines in HW after all? > Is there a link to the datasheet? A datasheet is available on https://www.analog.com/en/products/max7360.html > > > +} > > + > > +static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin) > > +{ > > + /* > > + * GPIOs on PORT pins are shared with the PWM and rotary encoder > > + * drivers: they have to be requested from the MFD driver. > > + */ > > So, this sounds to me like a pin control approach is needed here. > This looks like an attempt to hack it in an "easy" way. > Linus Walleij had a similar comment on v3, but said he thought it was fine here. Still, I'm open to discussion. > > + return max7360_port_pin_request(gc->parent->parent, pin, true); > > +} > > + > > +static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin) > > +{ > > + max7360_port_pin_request(gc->parent->parent, pin, false); > > +} > > + > > +static int max7360_set_gpos_count(struct device *dev, struct regmap *regmap) > > +{ > > + /* > > + * MAX7360 COL0 to COL7 pins can be used either as keypad columns, > > + * general purpose output or a mix of both. > > + */ > > + unsigned int val; > > + u32 columns; > > + u32 ngpios; > > + int ret; > > + > > + ret = device_property_read_u32(dev, "ngpios", &ngpios); > > + if (ret < 0) { > > + dev_err(dev, "Missing ngpios OF property\n"); > > Clean messages from OF, "device property" is established term. > Yes > > + return ret; > > + } > > + > > + /* > > + * Get the number of pins requested by the keypad and ensure our own pin > > + * count is compatible with it. > > + */ > > + ret = device_property_read_u32(dev->parent, "keypad,num-columns", &columns); > > + if (ret < 0) { > > + dev_err(dev, "Failed to read columns count\n"); > > + return ret; > > + } > > + > > + if (ngpios > MAX7360_MAX_GPO || > > + (ngpios + columns > MAX7360_MAX_KEY_COLS)) { > > + dev_err(dev, "Incompatible gpos and columns count (%u, %u)\n", > > + ngpios, columns); > > + return -EINVAL; > > + } > > + > > + /* > > + * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce > > + * timings and gpos/keypad columns repartition. Only the later is > > + * modified here. > > + */ > > + val = FIELD_PREP(MAX7360_PORTS, ngpios); > > + ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val); > > + if (ret) { > > + dev_err(dev, "Failed to write max7360 columns/gpos configuration"); > > + return ret; > > + } > > Shouldn't this be configured via ->set_config() callback? > My understanding of the set_config() callback is that it's meant to set the configuration of a single line. Here the configuration applies to the whole chip. > > + return 0; > > +} > > ... > > > + if (irq < 0) > > + return dev_err_probe(dev, irq, "Failed to get IRQ\n"); > > + > > + irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL); > > + if (!irq_chip) > > + return -ENOMEM; > > + > > + irq_chip->name = dev_name(dev); > > + irq_chip->status_base = MAX7360_REG_GPIOIN; > > + irq_chip->num_regs = 1; > > + irq_chip->num_irqs = MAX7360_MAX_GPIO; > > + irq_chip->irqs = max7360_regmap_irqs; > > + irq_chip->handle_mask_sync = max7360_handle_mask_sync; > > + irq_chip->status_is_level = true; > > + irq_chip->irq_drv_data = regmap; > > + > > + for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) { > > + regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i), > > + MAX7360_PORT_CFG_INTERRUPT_EDGES, > > + MAX7360_PORT_CFG_INTERRUPT_EDGES); > > + } > > + > > + flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED; > > + ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0, > > + irq_chip, &irq_chip_data); > > Right. > > What I mean in previous discussion is to update gpio-regmap to call this from inside. > You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration > and if they are set (or the first one, I dunno if this is supported by IRQ chip core) > call this function and assign domain. This should be called after GPIO chip is > added, but before IRQ domain attachment. > OK, I believe I got it now. I will try to work on it in the coming days. > > + > > + regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf); > > + } > > + > > + /* Add gpio device. */ > > + gpio_config.parent = dev; > > + gpio_config.regmap = regmap; > > > + if (gpio_function == MAX7360_GPIO_PORT) { > > + gpio_config.ngpio = MAX7360_MAX_GPIO; > > Why this case can't be managed also via ngpios property? Maybe at the end of > the day you rather need to have another property to tell where the split is? > > This will help a lot and removes unneeded sharing of ngpios here and there. > > What I read from this code is like you are trying to put _two_in_one_ semantics > on the shoulders of "ngpios". > It could be managed with ngpios, just there is no specific need as we will always have 8 gpios here. With (gpio_function == MAX7360_GPIO_PORT), there is no split and starting from this version of my series, there is no reuse on ngpios property. The split and reuse of ngpios is only used for GPO on keypad columns (gpio_function == MAX7360_GPIO_COL). We can introduce a new property to tell where the split is, but the number of gpio is a direct consequence of the position of the split.
On Mon, Feb 17, 2025 at 12:20:13PM +0100, Mathieu Dubois-Briand wrote: > On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote: > > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote: > > > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller. ... > > > +static int max7360_gpo_reg_mask_xlate(struct gpio_regmap *gpio, > > > + unsigned int base, unsigned int offset, > > > + unsigned int *reg, unsigned int *mask) > > > +{ > > > + u16 ngpios = gpio_regmap_get_ngpio(gpio); > > > + > > > + *reg = base; > > > + *mask = BIT(MAX7360_MAX_KEY_COLS - (ngpios - offset)); > > > + > > > + return 0; > > > > Does this GPIO controller only capable of servicing keypads? > > I think no, hence I'm not sure why this split is needed to be > > here and not in the input driver. > > I would say it's more a keypad controller able to support some GPIOs. > Some of the keypad columns, if unused, can be used as output-only gpios. > So I believe the split has its place here, because in the default > configuration, the split is set to have 8 keypad columns and no gpio. As > a consequence, the keypad driver can work without having to worry about > the split; the gpio driver needs to know about it. > > To provide a bit more details, there is basically two set of pins usable > as GPIOs. > > On one side we have what I refer to as GPIOs: > - PORT0 to PORT7 pins of the chip. > - Shared with PWM and rotary encoder functionalities. Functionality > selection can be made independently for each pin. We have to ensure > the same pin is not used by two drivers at the same time. E.g. we > cannot have at the same time GPIO4 and PWM4. > - Supports input and interrupts. > - Outputs may be configured as constant current. > - 8 GPIOS supported, so ngpios is fixed to MAX7360_MAX_GPIO. > - maxim,max7360-gpio compatible, gpio_function == MAX7360_GPIO_PORT. > > On the other side, we have what I refer to as GPOs: > - COL2 to COL7 pins of the chip. > - Shared with the keypad functionality. Selections is made by > partitioning the pins: first pins for keypad columns, last pins for > GPOs. Partition is described by the ngpios property. > - Only support outputs. > - maxim,max7360-gpo compatible, gpio_function == MAX7360_GPIO_COL. > > > Or you mean that there output only GPIO lines in HW after all? > > Is there a link to the datasheet? > > A datasheet is available on https://www.analog.com/en/products/max7360.html Thank you for this good elaboration! I will check on the datasheet later on, having one week off. But what I have read above sounds to me like the following: 1) the PORT0-PORT7 should be just a regular pin control with the respective function being provided (see pinctrl-cy8c95x0.c as an example); 2) the COL2 COL7 case can be modeled as a simplest GPIO (GPO) driver with reserved lines property (this will set valid mask and let GPIOLIB to refuse any use of the keypad connected pins. So, with this approach the entire handling becomes less hackish and quite straightforward!
On Mon Feb 17, 2025 at 9:08 PM CET, Andy Shevchenko wrote: > On Mon, Feb 17, 2025 at 12:20:13PM +0100, Mathieu Dubois-Briand wrote: > > To provide a bit more details, there is basically two set of pins usable > > as GPIOs. > > > > On one side we have what I refer to as GPIOs: > > - PORT0 to PORT7 pins of the chip. > > - Shared with PWM and rotary encoder functionalities. Functionality > > selection can be made independently for each pin. We have to ensure > > the same pin is not used by two drivers at the same time. E.g. we > > cannot have at the same time GPIO4 and PWM4. > > - Supports input and interrupts. > > - Outputs may be configured as constant current. > > - 8 GPIOS supported, so ngpios is fixed to MAX7360_MAX_GPIO. > > - maxim,max7360-gpio compatible, gpio_function == MAX7360_GPIO_PORT. > > > > On the other side, we have what I refer to as GPOs: > > - COL2 to COL7 pins of the chip. > > - Shared with the keypad functionality. Selections is made by > > partitioning the pins: first pins for keypad columns, last pins for > > GPOs. Partition is described by the ngpios property. > > - Only support outputs. > > - maxim,max7360-gpo compatible, gpio_function == MAX7360_GPIO_COL. > > > > > Or you mean that there output only GPIO lines in HW after all? > > > Is there a link to the datasheet? > > > > A datasheet is available on https://www.analog.com/en/products/max7360.html > > Thank you for this good elaboration! > I will check on the datasheet later on, having one week off. > Thanks for your feedback! Sorry I haven't been able to work on this series for the last few weeks, but I finally had the opportunity to integrate your comments. > But what I have read above sounds to me like the following: > > 1) the PORT0-PORT7 should be just a regular pin control with the respective > function being provided (see pinctrl-cy8c95x0.c as an example); > Ok, so I created a pin control driver for the PORT pins. This will effectively help to prevent concurrent use of pins in place of the request()/free() callbacks. My only concern is: as there is no real pin muxing on the chip, my .set_mux callabck in pinmux_ops structure is not doing anything. It looks like I'm not the only one (drivers/pinctrl/pinctrl-microchip-sgpio.c does the same thing), but I hope this is OK. > 2) the COL2 COL7 case can be modeled as a simplest GPIO (GPO) driver with > reserved lines property (this will set valid mask and let GPIOLIB to refuse any > use of the keypad connected pins. > I mostly went that way, just a few notes. I chose to not use the reserved lines property in the device tree, but instead implemented a gpiolib init_valid_mask() callback. In believe this is better, as: - We can automatically generate the valid gpios mask, based on the number of columns used. - It allows to get rid of the compatibility check between the number of columns and the number of GPIOs provided by the device tree: DT provides the number of columns, we deduct the number of GPIOs. I chose to number GPIOs from 0 to 7. - This might be a bit questionable, as GPIO 0 and 1 will always be invalid: pins 0 and 1 of the chip cannot be used as GPIOs. I'm definitely open to discussion on this point. - Yet I believe it simplifies everything for the user: pin numbers and GPIO numbers are the same instead of having an offset of 2. - It also simplifies a bit the GPIO driver code.
On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote: > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote: > > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller. > > ... > > + > > + /* > > + * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce > > + * timings and gpos/keypad columns repartition. Only the later is > > + * modified here. > > + */ > > + val = FIELD_PREP(MAX7360_PORTS, ngpios); > > + ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val); > > + if (ret) { > > + dev_err(dev, "Failed to write max7360 columns/gpos configuration"); > > + return ret; > > + } > > Shouldn't this be configured via ->set_config() callback? > I believe this comment has been a bit outdated by our discussion on using GPIO valid mask, but I believe we could not use the ->set_config() callback here: this callback is made to configure a single pin while the gpos/keypad columns repartition is global. > > ... > > > + if (irq < 0) > > + return dev_err_probe(dev, irq, "Failed to get IRQ\n"); > > + > > + irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL); > > + if (!irq_chip) > > + return -ENOMEM; > > + > > + irq_chip->name = dev_name(dev); > > + irq_chip->status_base = MAX7360_REG_GPIOIN; > > + irq_chip->num_regs = 1; > > + irq_chip->num_irqs = MAX7360_MAX_GPIO; > > + irq_chip->irqs = max7360_regmap_irqs; > > + irq_chip->handle_mask_sync = max7360_handle_mask_sync; > > + irq_chip->status_is_level = true; > > + irq_chip->irq_drv_data = regmap; > > + > > + for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) { > > + regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i), > > + MAX7360_PORT_CFG_INTERRUPT_EDGES, > > + MAX7360_PORT_CFG_INTERRUPT_EDGES); > > + } > > + > > + flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED; > > + ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0, > > + irq_chip, &irq_chip_data); > > Right. > > What I mean in previous discussion is to update gpio-regmap to call this from inside. > You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration > and if they are set (or the first one, I dunno if this is supported by IRQ chip core) > call this function and assign domain. This should be called after GPIO chip is > added, but before IRQ domain attachment. > Ok, this is a bit more clear to me now. So I came up with something, it will be part of the next iteration, probably during the next week. This required to add a few additional fields to the gpio_regmap_config structure, specifying the IRQ configuration: + * @regmap_irq_chip: (Optional) Pointer on an regmap_irq_chip structure. If + * set, a regmap-irq device will be created and the IRQ + * domain will be set accordingly. + * @regmap_irq_chip_data: (Optional) Pointer on an regmap_irq_chip_data + * structure pointer. If set, it will be populated with a + * pointer on allocated regmap_irq data. + * @regmap_irq_irqno (Optional) The IRQ the device uses to signal interrupts. + * @regmap_irq_flags (Optional) The IRQF_ flags to use for the interrupt. > > ... > > > + > > + regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf); > > + } > > + > > + /* Add gpio device. */ > > + gpio_config.parent = dev; > > + gpio_config.regmap = regmap; > > > + if (gpio_function == MAX7360_GPIO_PORT) { > > + gpio_config.ngpio = MAX7360_MAX_GPIO; > > Why this case can't be managed also via ngpios property? Maybe at the end of > the day you rather need to have another property to tell where the split is? > > This will help a lot and removes unneeded sharing of ngpios here and there. > > What I read from this code is like you are trying to put _two_in_one_ semantics > on the shoulders of "ngpios". > So as I reworked the keypad columns GPIOs, PORT GPIOs and the COL GPIOs are a bit more similar on this point. So far I now use a constant value assigned in the driver for both, as I believe there is no way the number of GPIOs could be a different. Yet I can easily switch back to a value provided by a device property. Thanks again for your review. Mathieu
Thu, Mar 13, 2025 at 05:43:00PM +0100, Mathieu Dubois-Briand kirjoitti: > On Mon Feb 17, 2025 at 9:08 PM CET, Andy Shevchenko wrote: > > On Mon, Feb 17, 2025 at 12:20:13PM +0100, Mathieu Dubois-Briand wrote: ... > > > A datasheet is available on https://www.analog.com/en/products/max7360.html > > > > Thank you for this good elaboration! > > I will check on the datasheet later on, having one week off. Note, I have only briefly looked at it, not a deep study and TBH I am not sure I will have time to invest into that. > Thanks for your feedback! Sorry I haven't been able to work on this > series for the last few weeks, but I finally had the opportunity to > integrate your comments. No rush, this will miss v6.15 anyway, so we still have a couple of months. > > But what I have read above sounds to me like the following: > > > > 1) the PORT0-PORT7 should be just a regular pin control with the respective > > function being provided (see pinctrl-cy8c95x0.c as an example); > > Ok, so I created a pin control driver for the PORT pins. This will > effectively help to prevent concurrent use of pins in place of the > request()/free() callbacks. > > My only concern is: as there is no real pin muxing on the chip, my > .set_mux callabck in pinmux_ops structure is not doing anything. It > looks like I'm not the only one > (drivers/pinctrl/pinctrl-microchip-sgpio.c does the same thing), but I > hope this is OK. Hmm... This is strange. The PWM/GPIO block has 3 functions (GPIO/PWM/rotary), How comes you have no switch between them? As far as I read in the datasheet this is controlled by register 0x40 (and seems implicitly by other registers when it's in PWM mode). > > 2) the COL2 COL7 case can be modeled as a simplest GPIO (GPO) driver with > > reserved lines property (this will set valid mask and let GPIOLIB to refuse any > > use of the keypad connected pins. > > I mostly went that way, just a few notes. > > I chose to not use the reserved lines property in the device tree, but > instead implemented a gpiolib init_valid_mask() callback. In believe > this is better, as: > - We can automatically generate the valid gpios mask, based on the > number of columns used. > - It allows to get rid of the compatibility check between the number of > columns and the number of GPIOs provided by the device tree: DT > provides the number of columns, we deduct the number of GPIOs. If I understood it correctly it should work as well. But let's discuss that when you issue a new version. > I chose to number GPIOs from 0 to 7. > - This might be a bit questionable, as GPIO 0 and 1 will always be > invalid: pins 0 and 1 of the chip cannot be used as GPIOs. I'm > definitely open to discussion on this point. > - Yet I believe it simplifies everything for the user: pin numbers and > GPIO numbers are the same instead of having an offset of 2. > - It also simplifies a bit the GPIO driver code. In general you should follow the datasheet and mask the GPIOs that may not be uses as a such due to HW limitation / specific configuration.
Thu, Mar 13, 2025 at 06:07:03PM +0100, Mathieu Dubois-Briand kirjoitti: > On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote: > > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote: > > > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller. ... > > > + /* > > > + * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce > > > + * timings and gpos/keypad columns repartition. Only the later is > > > + * modified here. > > > + */ > > > + val = FIELD_PREP(MAX7360_PORTS, ngpios); > > > + ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val); > > > + if (ret) { > > > + dev_err(dev, "Failed to write max7360 columns/gpos configuration"); > > > + return ret; > > > + } > > > > Shouldn't this be configured via ->set_config() callback? > > I believe this comment has been a bit outdated by our discussion on > using GPIO valid mask, but I believe we could not use the ->set_config() > callback here: this callback is made to configure a single pin while the > gpos/keypad columns repartition is global. Yeah, we have similar desing in Intel Bay Trail (see pinctrl-baytrail.c) and it requires some software driven heuristics on how individual setting may affect the global one. But the Q here is is the debounce affects only keypad? Then it should be configured via keypad matrix driver. Btw, have you checked drivers/input/keyboard/matrix_keypad.c? Is there anything that can be useful here? ... > > > + if (irq < 0) > > > + return dev_err_probe(dev, irq, "Failed to get IRQ\n"); > > > + > > > + irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL); > > > + if (!irq_chip) > > > + return -ENOMEM; > > > + > > > + irq_chip->name = dev_name(dev); > > > + irq_chip->status_base = MAX7360_REG_GPIOIN; > > > + irq_chip->num_regs = 1; > > > + irq_chip->num_irqs = MAX7360_MAX_GPIO; > > > + irq_chip->irqs = max7360_regmap_irqs; > > > + irq_chip->handle_mask_sync = max7360_handle_mask_sync; > > > + irq_chip->status_is_level = true; > > > + irq_chip->irq_drv_data = regmap; > > > + > > > + for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) { > > > + regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i), > > > + MAX7360_PORT_CFG_INTERRUPT_EDGES, > > > + MAX7360_PORT_CFG_INTERRUPT_EDGES); > > > + } > > > + > > > + flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED; > > > + ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0, > > > + irq_chip, &irq_chip_data); > > > > Right. > > > > What I mean in previous discussion is to update gpio-regmap to call this from inside. > > You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration > > and if they are set (or the first one, I dunno if this is supported by IRQ chip core) > > call this function and assign domain. This should be called after GPIO chip is > > added, but before IRQ domain attachment. > > > > Ok, this is a bit more clear to me now. So I came up with something, it > will be part of the next iteration, probably during the next week. > > This required to add a few additional fields to the gpio_regmap_config > structure, specifying the IRQ configuration: > > + * @regmap_irq_chip: (Optional) Pointer on an regmap_irq_chip structure. If > + * set, a regmap-irq device will be created and the IRQ > + * domain will be set accordingly. > + * @regmap_irq_chip_data: (Optional) Pointer on an regmap_irq_chip_data > + * structure pointer. If set, it will be populated with a > + * pointer on allocated regmap_irq data. > + * @regmap_irq_irqno (Optional) The IRQ the device uses to signal interrupts. > + * @regmap_irq_flags (Optional) The IRQF_ flags to use for the interrupt. Okay, just make sure it's guarded by the same ifdeffery as the similar in the GPIO: #ifdef CONFIG_GPIOLIB_IRQCHIP ... > > > + > > > + regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf); > > > + } > > > + > > > + /* Add gpio device. */ > > > + gpio_config.parent = dev; > > > + gpio_config.regmap = regmap; > > > > > + if (gpio_function == MAX7360_GPIO_PORT) { > > > + gpio_config.ngpio = MAX7360_MAX_GPIO; > > > > Why this case can't be managed also via ngpios property? Maybe at the end of > > the day you rather need to have another property to tell where the split is? > > > > This will help a lot and removes unneeded sharing of ngpios here and there. > > > > What I read from this code is like you are trying to put _two_in_one_ semantics > > on the shoulders of "ngpios". > > So as I reworked the keypad columns GPIOs, PORT GPIOs and the COL GPIOs > are a bit more similar on this point. So far I now use a constant value > assigned in the driver for both, as I believe there is no way the number > of GPIOs could be a different. Yet I can easily switch back to a value > provided by a device property. Sounds good as long as ngpios is not overloaded with the additional meanings.
On Fri Mar 14, 2025 at 9:14 AM CET, Andy Shevchenko wrote: > Thu, Mar 13, 2025 at 06:07:03PM +0100, Mathieu Dubois-Briand kirjoitti: > > On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote: > > > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote: > > > > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller. > > ... > > > > > + /* > > > > + * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce > > > > + * timings and gpos/keypad columns repartition. Only the later is > > > > + * modified here. > > > > + */ > > > > + val = FIELD_PREP(MAX7360_PORTS, ngpios); > > > > + ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val); > > > > + if (ret) { > > > > + dev_err(dev, "Failed to write max7360 columns/gpos configuration"); > > > > + return ret; > > > > + } > > > > > > Shouldn't this be configured via ->set_config() callback? > > > > I believe this comment has been a bit outdated by our discussion on > > using GPIO valid mask, but I believe we could not use the ->set_config() > > callback here: this callback is made to configure a single pin while the > > gpos/keypad columns repartition is global. > > Yeah, we have similar desing in Intel Bay Trail (see pinctrl-baytrail.c) and it > requires some software driven heuristics on how individual setting may affect > the global one. But the Q here is is the debounce affects only keypad? Then it > should be configured via keypad matrix driver. Btw, have you checked > drivers/input/keyboard/matrix_keypad.c? Is there anything that can be useful > here? > Hum, maybe the comment is not clear enough? Not sure, but please tell me. So yes, this register is named "debounce" but controls two different things: - The keypad debounce: we do not touch it here. - The partition between keypad columns and gpos. This is the value we do modify here. I've been looking at drivers/input/keyboard/matrix_keypad.c, but I believe the idea is completely different: it allows to use GPIOs to create a keypad matrix, without the help of any controller. Here we have a controller dedicated to keypad matrix, allowing to repurpose unused columns as GPIOs. So except for some device tree similar properties and input events, I believe there there isn't much we can reuse. > ... > > > > > + if (irq < 0) > > > > + return dev_err_probe(dev, irq, "Failed to get IRQ\n"); > > > > + > > > > + irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL); > > > > + if (!irq_chip) > > > > + return -ENOMEM; > > > > + > > > > + irq_chip->name = dev_name(dev); > > > > + irq_chip->status_base = MAX7360_REG_GPIOIN; > > > > + irq_chip->num_regs = 1; > > > > + irq_chip->num_irqs = MAX7360_MAX_GPIO; > > > > + irq_chip->irqs = max7360_regmap_irqs; > > > > + irq_chip->handle_mask_sync = max7360_handle_mask_sync; > > > > + irq_chip->status_is_level = true; > > > > + irq_chip->irq_drv_data = regmap; > > > > + > > > > + for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) { > > > > + regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i), > > > > + MAX7360_PORT_CFG_INTERRUPT_EDGES, > > > > + MAX7360_PORT_CFG_INTERRUPT_EDGES); > > > > + } > > > > + > > > > + flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED; > > > > + ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0, > > > > + irq_chip, &irq_chip_data); > > > > > > Right. > > > > > > What I mean in previous discussion is to update gpio-regmap to call this from inside. > > > You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration > > > and if they are set (or the first one, I dunno if this is supported by IRQ chip core) > > > call this function and assign domain. This should be called after GPIO chip is > > > added, but before IRQ domain attachment. > > > > > > > Ok, this is a bit more clear to me now. So I came up with something, it > > will be part of the next iteration, probably during the next week. > > > > This required to add a few additional fields to the gpio_regmap_config > > structure, specifying the IRQ configuration: > > > > + * @regmap_irq_chip: (Optional) Pointer on an regmap_irq_chip structure. If > > + * set, a regmap-irq device will be created and the IRQ > > + * domain will be set accordingly. > > + * @regmap_irq_chip_data: (Optional) Pointer on an regmap_irq_chip_data > > + * structure pointer. If set, it will be populated with a > > + * pointer on allocated regmap_irq data. > > + * @regmap_irq_irqno (Optional) The IRQ the device uses to signal interrupts. > > + * @regmap_irq_flags (Optional) The IRQF_ flags to use for the interrupt. > > Okay, just make sure it's guarded by the same ifdeffery as the similar in the > GPIO: > > #ifdef CONFIG_GPIOLIB_IRQCHIP > Thanks! > ... > > > > > + > > > > + regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf); > > > > + } > > > > + > > > > + /* Add gpio device. */ > > > > + gpio_config.parent = dev; > > > > + gpio_config.regmap = regmap; > > > > > > > + if (gpio_function == MAX7360_GPIO_PORT) { > > > > + gpio_config.ngpio = MAX7360_MAX_GPIO; > > > > > > Why this case can't be managed also via ngpios property? Maybe at the end of > > > the day you rather need to have another property to tell where the split is? > > > > > > This will help a lot and removes unneeded sharing of ngpios here and there. > > > > > > What I read from this code is like you are trying to put _two_in_one_ semantics > > > on the shoulders of "ngpios". > > > > So as I reworked the keypad columns GPIOs, PORT GPIOs and the COL GPIOs > > are a bit more similar on this point. So far I now use a constant value > > assigned in the driver for both, as I believe there is no way the number > > of GPIOs could be a different. Yet I can easily switch back to a value > > provided by a device property. > > Sounds good as long as ngpios is not overloaded with the additional meanings. Thanks again for your review.
On Mon, Mar 17, 2025 at 03:13:07PM +0100, Mathieu Dubois-Briand wrote: > On Fri Mar 14, 2025 at 9:14 AM CET, Andy Shevchenko wrote: > > Thu, Mar 13, 2025 at 06:07:03PM +0100, Mathieu Dubois-Briand kirjoitti: > > > On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote: > > > > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote: ... > > > > > + /* > > > > > + * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce > > > > > + * timings and gpos/keypad columns repartition. Only the later is > > > > > + * modified here. > > > > > + */ > > > > > + val = FIELD_PREP(MAX7360_PORTS, ngpios); > > > > > + ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val); > > > > > + if (ret) { > > > > > + dev_err(dev, "Failed to write max7360 columns/gpos configuration"); > > > > > + return ret; > > > > > + } > > > > > > > > Shouldn't this be configured via ->set_config() callback? > > > > > > I believe this comment has been a bit outdated by our discussion on > > > using GPIO valid mask, but I believe we could not use the ->set_config() > > > callback here: this callback is made to configure a single pin while the > > > gpos/keypad columns repartition is global. > > > > Yeah, we have similar desing in Intel Bay Trail (see pinctrl-baytrail.c) and it > > requires some software driven heuristics on how individual setting may affect > > the global one. But the Q here is is the debounce affects only keypad? Then it > > should be configured via keypad matrix driver. Btw, have you checked > > drivers/input/keyboard/matrix_keypad.c? Is there anything that can be useful > > here? > > > > Hum, maybe the comment is not clear enough? Not sure, but please tell > me. I see it now, yes, the comment seems point too much attention on the register (and hence its name) then content. I would start this comment with something like: "Configure which GPIOs will be used for keypad." > So yes, this register is named "debounce" but controls two different > things: > - The keypad debounce: we do not touch it here. > - The partition between keypad columns and gpos. This is the value we do > modify here.
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 98b4d1633b25..3b3babc1c029 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1445,6 +1445,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 + select GPIO_REGMAP + 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..99ccda687b88 --- /dev/null +++ b/drivers/gpio/gpio-max7360.c @@ -0,0 +1,253 @@ +// 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/driver.h> +#include <linux/gpio/regmap.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/mfd/max7360.h> +#include <linux/module.h> +#include <linux/property.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#define MAX7360_GPIO_PORT 1 +#define MAX7360_GPIO_COL 2 + +static int max7360_gpo_reg_mask_xlate(struct gpio_regmap *gpio, + unsigned int base, unsigned int offset, + unsigned int *reg, unsigned int *mask) +{ + u16 ngpios = gpio_regmap_get_ngpio(gpio); + + *reg = base; + *mask = BIT(MAX7360_MAX_KEY_COLS - (ngpios - offset)); + + return 0; +} + +static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin) +{ + /* + * GPIOs on PORT pins are shared with the PWM and rotary encoder + * drivers: they have to be requested from the MFD driver. + */ + return max7360_port_pin_request(gc->parent->parent, pin, true); +} + +static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin) +{ + max7360_port_pin_request(gc->parent->parent, pin, false); +} + +static int max7360_set_gpos_count(struct device *dev, struct regmap *regmap) +{ + /* + * MAX7360 COL0 to COL7 pins can be used either as keypad columns, + * general purpose output or a mix of both. + */ + unsigned int val; + u32 columns; + u32 ngpios; + int ret; + + ret = device_property_read_u32(dev, "ngpios", &ngpios); + if (ret < 0) { + dev_err(dev, "Missing ngpios OF property\n"); + return ret; + } + + /* + * Get the number of pins requested by the keypad and ensure our own pin + * count is compatible with it. + */ + ret = device_property_read_u32(dev->parent, "keypad,num-columns", &columns); + if (ret < 0) { + dev_err(dev, "Failed to read columns count\n"); + return ret; + } + + if (ngpios > MAX7360_MAX_GPO || + (ngpios + columns > MAX7360_MAX_KEY_COLS)) { + dev_err(dev, "Incompatible gpos and columns count (%u, %u)\n", + ngpios, columns); + return -EINVAL; + } + + /* + * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce + * timings and gpos/keypad columns repartition. Only the later is + * modified here. + */ + val = FIELD_PREP(MAX7360_PORTS, ngpios); + ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val); + if (ret) { + dev_err(dev, "Failed to write max7360 columns/gpos configuration"); + return ret; + } + + return 0; +} + +static const struct regmap_irq max7360_regmap_irqs[MAX7360_MAX_GPIO] = { + REGMAP_IRQ_REG(0, 0, BIT(0)), + REGMAP_IRQ_REG(1, 0, BIT(1)), + REGMAP_IRQ_REG(2, 0, BIT(2)), + REGMAP_IRQ_REG(3, 0, BIT(3)), + REGMAP_IRQ_REG(4, 0, BIT(4)), + REGMAP_IRQ_REG(5, 0, BIT(5)), + REGMAP_IRQ_REG(6, 0, BIT(6)), + REGMAP_IRQ_REG(7, 0, BIT(7)), +}; + +static int max7360_handle_mask_sync(const int index, + const unsigned int mask_buf_def, + const unsigned int mask_buf, + void *const irq_drv_data) +{ + struct regmap *regmap = irq_drv_data; + unsigned int val; + + for (unsigned int i = 0; i < MAX7360_MAX_GPIO; ++i) { + val = (mask_buf & 1 << i) ? MAX7360_PORT_CFG_INTERRUPT_MASK : 0; + regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i), + MAX7360_PORT_CFG_INTERRUPT_MASK, val); + } + + return 0; +} + +static int max7360_gpio_probe(struct platform_device *pdev) +{ + struct regmap_irq_chip *irq_chip; + struct regmap_irq_chip_data *irq_chip_data; + struct gpio_regmap_config gpio_config = { }; + struct device *dev = &pdev->dev; + unsigned long gpio_function; + struct regmap *regmap; + unsigned int outconf; + unsigned long flags; + int irq; + int ret; + + regmap = dev_get_regmap(dev->parent, NULL); + if (!regmap) + return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n"); + + gpio_function = (uintptr_t)device_get_match_data(dev); + + if (gpio_function == MAX7360_GPIO_PORT && + (device_property_read_bool(dev, "interrupt-controller"))) { + /* + * Port GPIOs with interrupt-controller property: add IRQ + * controller. + */ + irq = fwnode_irq_get_byname(dev->parent->fwnode, "inti"); + if (irq < 0) + return dev_err_probe(dev, irq, "Failed to get IRQ\n"); + + irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL); + if (!irq_chip) + return -ENOMEM; + + irq_chip->name = dev_name(dev); + irq_chip->status_base = MAX7360_REG_GPIOIN; + irq_chip->num_regs = 1; + irq_chip->num_irqs = MAX7360_MAX_GPIO; + irq_chip->irqs = max7360_regmap_irqs; + irq_chip->handle_mask_sync = max7360_handle_mask_sync; + irq_chip->status_is_level = true; + irq_chip->irq_drv_data = regmap; + + for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) { + regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i), + MAX7360_PORT_CFG_INTERRUPT_EDGES, + MAX7360_PORT_CFG_INTERRUPT_EDGES); + } + + flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED; + ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0, + irq_chip, &irq_chip_data); + if (ret) + return dev_err_probe(dev, ret, "IRQ registration failed\n"); + + gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data); + } + + if (gpio_function == MAX7360_GPIO_PORT) { + /* + * Port GPIOs: set output mode configuration (constant-current or not). + * This property is optional. + */ + outconf = 0; + ret = device_property_read_u32(dev, "maxim,constant-current-disable", &outconf); + if (ret && (ret != -EINVAL)) + return dev_err_probe(dev, -ENODEV, + "Failed to read maxim,constant-current-disable OF property\n"); + + regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf); + } + + /* Add gpio device. */ + gpio_config.parent = dev; + gpio_config.regmap = regmap; + if (gpio_function == MAX7360_GPIO_PORT) { + gpio_config.ngpio = MAX7360_MAX_GPIO; + gpio_config.reg_dat_base = GPIO_REGMAP_ADDR(MAX7360_REG_GPIOIN); + gpio_config.reg_set_base = GPIO_REGMAP_ADDR(MAX7360_REG_PWMBASE); + gpio_config.reg_dir_out_base = GPIO_REGMAP_ADDR(MAX7360_REG_GPIOCTRL); + gpio_config.ngpio_per_reg = MAX7360_MAX_GPIO; + gpio_config.request = max7360_gpio_request; + gpio_config.free = max7360_gpio_free; + } else { + u32 ngpios; + + ret = device_property_read_u32(dev, "ngpios", &ngpios); + if (ret < 0) { + dev_err(dev, "Missing ngpios OF property\n"); + return ret; + } + + gpio_config.reg_set_base = GPIO_REGMAP_ADDR(MAX7360_REG_PORTS); + gpio_config.reg_mask_xlate = max7360_gpo_reg_mask_xlate; + gpio_config.ngpio = ngpios; + + ret = max7360_set_gpos_count(dev, regmap); + if (ret) + return dev_err_probe(dev, ret, "Failed to set GPOS pin count\n"); + } + + return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &gpio_config)); +} + +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-gpio", + .of_match_table = 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_LICENSE("GPL");