| 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!
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");