| Message ID | 20250428-mdb-max7360-support-v7-0-4e0608d0a7ff@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for MAX7360 | expand |
On Mon, 28 Apr 2025, mathieu.dubois-briand@bootlin.com wrote: > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Add core driver to support MAX7360 i2c chip, multi function device > with keypad, GPIO, PWM, GPO and rotary encoder submodules. > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > --- > drivers/mfd/Kconfig | 14 ++++ > drivers/mfd/Makefile | 1 + > drivers/mfd/max7360.c | 184 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/max7360.h | 109 ++++++++++++++++++++++++++ > 4 files changed, 308 insertions(+) Getting there. Couple of nits. Last push! > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 22b936310039..c2998c6ce54c 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -2422,5 +2422,19 @@ config MFD_UPBOARD_FPGA > To compile this driver as a module, choose M here: the module will be > called upboard-fpga. > > +config MFD_MAX7360 > + tristate "Maxim MAX7360 I2C IO Expander" > + depends on I2C > + select MFD_CORE > + select REGMAP_I2C > + select REGMAP_IRQ > + help > + Say yes here to add support for Maxim MAX7360 device, embedding > + keypad, rotary encoder, PWM and GPIO features. > + > + This driver provides common support for accessing the device; > + additional drivers must be enabled in order to use the functionality > + of the device. > + > endmenu > endif > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 948cbdf42a18..add9ff58eb25 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -162,6 +162,7 @@ obj-$(CONFIG_MFD_DA9063) += da9063.o > obj-$(CONFIG_MFD_DA9150) += da9150-core.o > > obj-$(CONFIG_MFD_MAX14577) += max14577.o > +obj-$(CONFIG_MFD_MAX7360) += max7360.o > obj-$(CONFIG_MFD_MAX77541) += max77541.o > obj-$(CONFIG_MFD_MAX77620) += max77620.o > obj-$(CONFIG_MFD_MAX77650) += max77650.o > diff --git a/drivers/mfd/max7360.c b/drivers/mfd/max7360.c > new file mode 100644 > index 000000000000..9a223a9b409d > --- /dev/null > +++ b/drivers/mfd/max7360.c > @@ -0,0 +1,184 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Maxim MAX7360 Core Driver > + * > + * Copyright 2025 Bootlin > + * > + * Author: Kamel Bouhara <kamel.bouhara@bootlin.com> > + * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > + */ > + > +#include <linux/array_size.h> > +#include <linux/bits.h> > +#include <linux/delay.h> > +#include <linux/device/devres.h> > +#include <linux/dev_printk.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/max7360.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/types.h> > + > +static const struct mfd_cell max7360_cells[] = { > + { > + .name = "max7360-pinctrl", > + }, All of these single line entries should be placed on a single line. { .name = "max7360-pinctrl" }, { .name = "max7360-pwm" }, If ordering is not important. Please group them. > + { > + .name = "max7360-pwm", > + }, > + { > + .name = "max7360-gpo", > + .of_compatible = "maxim,max7360-gpo", > + }, > + { > + .name = "max7360-gpio", > + .of_compatible = "maxim,max7360-gpio", > + }, > + { > + .name = "max7360-keypad", > + }, > + { > + .name = "max7360-rotary", > + }, > +}; > + > +static const struct regmap_range max7360_volatile_ranges[] = { > + { > + .range_min = MAX7360_REG_KEYFIFO, > + .range_max = MAX7360_REG_KEYFIFO, > + }, { > + .range_min = MAX7360_REG_I2C_TIMEOUT, > + .range_max = MAX7360_REG_RTR_CNT, > + }, > +}; Use regmap_reg_range() > +static const struct regmap_access_table max7360_volatile_table = { > + .yes_ranges = max7360_volatile_ranges, > + .n_yes_ranges = ARRAY_SIZE(max7360_volatile_ranges), > +}; > + > +static const struct regmap_config max7360_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = MAX7360_REG_PWMCFG(MAX7360_PORT_PWM_COUNT - 1), > + .volatile_table = &max7360_volatile_table, > + .cache_type = REGCACHE_MAPLE, > +}; > + > +static int max7360_mask_irqs(struct regmap *regmap) > +{ > + struct device *dev = regmap_get_device(regmap); > + unsigned int val; > + int ret; > + > + /* > + * GPIO/PWM interrupts are not masked on reset: as the MAX7360 "INTI" > + * interrupt line is shared between GPIOs and rotary encoder, this could > + * result in repeated spurious interrupts on the rotary encoder driver > + * if the GPIO driver is not loaded. Mask them now to avoid this > + * situation. > + */ > + for (unsigned int i = 0; i < MAX7360_PORT_PWM_COUNT; i++) { > + ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i), > + MAX7360_PORT_CFG_INTERRUPT_MASK, > + MAX7360_PORT_CFG_INTERRUPT_MASK); > + if (ret) { > + dev_err(dev, "Failed to write max7360 port configuration"); MAX7360 > + return ret; > + } > + } > + > + /* Read GPIO in register, to ACK any pending IRQ. */ > + ret = regmap_read(regmap, MAX7360_REG_GPIOIN, &val); > + if (ret) > + dev_err(dev, "Failed to read gpio values: %d\n", ret); GPIO > + > + return ret; > +} > + > +static int max7360_reset(struct regmap *regmap) > +{ > + struct device *dev = regmap_get_device(regmap); > + int ret; > + > + ret = regmap_write(regmap, MAX7360_REG_GPIOCFG, MAX7360_GPIO_CFG_GPIO_RST); > + if (ret) { > + dev_err(dev, "Failed to reset GPIO configuration: %x\n", ret); > + return ret; > + } > + > + ret = regcache_drop_region(regmap, MAX7360_REG_GPIOCFG, MAX7360_REG_GPIO_LAST); > + if (ret) { > + dev_err(dev, "Failed to drop regmap cache: %x\n", ret); > + return ret; > + } > + > + ret = regmap_write(regmap, MAX7360_REG_SLEEP, 0); > + if (ret) { > + dev_err(dev, "Failed to reset autosleep configuration: %x\n", ret); > + return ret; > + } > + > + ret = regmap_write(regmap, MAX7360_REG_DEBOUNCE, 0); > + if (ret) > + dev_err(dev, "Failed to reset GPO port count: %x\n", ret); > + > + return ret; > +} > + > +static int max7360_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct regmap *regmap; > + int ret; > + > + regmap = devm_regmap_init_i2c(client, &max7360_regmap_config); > + if (IS_ERR(regmap)) > + return dev_err_probe(dev, PTR_ERR(regmap), "Failed to initialise regmap\n"); dev_err_ptr_probe() > + > + ret = max7360_reset(regmap); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to reset device\n"); > + > + /* Get the device out of shutdown mode. */ > + ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG, > + MAX7360_GPIO_CFG_GPIO_EN, > + MAX7360_GPIO_CFG_GPIO_EN); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to enable GPIO and PWM module\n"); > + > + ret = max7360_mask_irqs(regmap); > + if (ret) > + return dev_err_probe(dev, ret, "Could not mask interrupts\n"); > + > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, > + max7360_cells, ARRAY_SIZE(max7360_cells), > + NULL, 0, NULL); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to register child devices\n"); > + > + return 0; > +} > + > +static const struct of_device_id max7360_dt_match[] = { > + { .compatible = "maxim,max7360" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, max7360_dt_match); > + > +static struct i2c_driver max7360_driver = { > + .driver = { > + .name = "max7360", > + .of_match_table = max7360_dt_match, > + }, > + .probe = max7360_probe, > +}; > +module_i2c_driver(max7360_driver); > + > +MODULE_DESCRIPTION("Maxim MAX7360 I2C IO Expander core driver"); > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/max7360.h b/include/linux/mfd/max7360.h > new file mode 100644 > index 000000000000..b1d4cbee2385 > --- /dev/null > +++ b/include/linux/mfd/max7360.h > @@ -0,0 +1,109 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef __LINUX_MFD_MAX7360_H > +#define __LINUX_MFD_MAX7360_H > + > +#include <linux/bits.h> > + > +#define MAX7360_MAX_KEY_ROWS 8 > +#define MAX7360_MAX_KEY_COLS 8 > +#define MAX7360_MAX_KEY_NUM (MAX7360_MAX_KEY_ROWS * MAX7360_MAX_KEY_COLS) > +#define MAX7360_ROW_SHIFT 3 > + > +#define MAX7360_MAX_GPIO 8 > +#define MAX7360_MAX_GPO 6 > +#define MAX7360_PORT_PWM_COUNT 8 > +#define MAX7360_PORT_RTR_PIN (MAX7360_PORT_PWM_COUNT - 1) > + > +/* > + * MAX7360 registers > + */ > +#define MAX7360_REG_KEYFIFO 0x00 > +#define MAX7360_REG_CONFIG 0x01 > +#define MAX7360_REG_DEBOUNCE 0x02 > +#define MAX7360_REG_INTERRUPT 0x03 > +#define MAX7360_REG_PORTS 0x04 > +#define MAX7360_REG_KEYREP 0x05 > +#define MAX7360_REG_SLEEP 0x06 > + > +/* > + * MAX7360 GPIO registers > + * > + * All these registers are reset together when writing bit 3 of > + * MAX7360_REG_GPIOCFG. > + */ > +#define MAX7360_REG_GPIOCFG 0x40 > +#define MAX7360_REG_GPIOCTRL 0x41 > +#define MAX7360_REG_GPIODEB 0x42 > +#define MAX7360_REG_GPIOCURR 0x43 > +#define MAX7360_REG_GPIOOUTM 0x44 > +#define MAX7360_REG_PWMCOM 0x45 > +#define MAX7360_REG_RTRCFG 0x46 > +#define MAX7360_REG_I2C_TIMEOUT 0x48 > +#define MAX7360_REG_GPIOIN 0x49 > +#define MAX7360_REG_RTR_CNT 0x4A > +#define MAX7360_REG_PWMBASE 0x50 > +#define MAX7360_REG_PWMCFGBASE 0x58 > + > +#define MAX7360_REG_GPIO_LAST 0x5F > + > +#define MAX7360_REG_PWM(x) (MAX7360_REG_PWMBASE + (x)) > +#define MAX7360_REG_PWMCFG(x) (MAX7360_REG_PWMCFGBASE + (x)) > + > +/* > + * Configuration register bits > + */ > +#define MAX7360_FIFO_EMPTY 0x3f > +#define MAX7360_FIFO_OVERFLOW 0x7f > +#define MAX7360_FIFO_RELEASE BIT(6) > +#define MAX7360_FIFO_COL GENMASK(5, 3) > +#define MAX7360_FIFO_ROW GENMASK(2, 0) > + > +#define MAX7360_CFG_SLEEP BIT(7) > +#define MAX7360_CFG_INTERRUPT BIT(5) > +#define MAX7360_CFG_KEY_RELEASE BIT(3) > +#define MAX7360_CFG_WAKEUP BIT(1) > +#define MAX7360_CFG_TIMEOUT BIT(0) > + > +#define MAX7360_DEBOUNCE GENMASK(4, 0) > +#define MAX7360_DEBOUNCE_MIN 9 > +#define MAX7360_DEBOUNCE_MAX 40 > +#define MAX7360_PORTS GENMASK(8, 5) > + > +#define MAX7360_INTERRUPT_TIME_MASK GENMASK(4, 0) > +#define MAX7360_INTERRUPT_FIFO_MASK GENMASK(7, 5) > + > +#define MAX7360_PORT_CFG_INTERRUPT_MASK BIT(7) > +#define MAX7360_PORT_CFG_INTERRUPT_EDGES BIT(6) > +#define MAX7360_PORT_CFG_COMMON_PWM BIT(5) > + > +/* > + * Autosleep register values > + */ > +#define MAX7360_AUTOSLEEP_8192MS 0x01 > +#define MAX7360_AUTOSLEEP_4096MS 0x02 > +#define MAX7360_AUTOSLEEP_2048MS 0x03 > +#define MAX7360_AUTOSLEEP_1024MS 0x04 > +#define MAX7360_AUTOSLEEP_512MS 0x05 > +#define MAX7360_AUTOSLEEP_256MS 0x06 > + > +#define MAX7360_GPIO_CFG_RTR_EN BIT(7) > +#define MAX7360_GPIO_CFG_GPIO_EN BIT(4) > +#define MAX7360_GPIO_CFG_GPIO_RST BIT(3) > + > +#define MAX7360_ROT_DEBOUNCE GENMASK(3, 0) > +#define MAX7360_ROT_DEBOUNCE_MIN 0 > +#define MAX7360_ROT_DEBOUNCE_MAX 15 > +#define MAX7360_ROT_INTCNT GENMASK(6, 4) > +#define MAX7360_ROT_INTCNT_DLY BIT(7) > + > +#define MAX7360_INT_INTI 0 > +#define MAX7360_INT_INTK 1 > + > +#define MAX7360_INT_GPIO 0 > +#define MAX7360_INT_KEYPAD 1 > +#define MAX7360_INT_ROTARY 2 > + > +#define MAX7360_NR_INTERNAL_IRQS 3 > + > +#endif > > -- > 2.39.5 >
On Thu May 1, 2025 at 2:59 PM CEST, Lee Jones wrote: > On Mon, 28 Apr 2025, mathieu.dubois-briand@bootlin.com wrote: > >> From: Kamel Bouhara <kamel.bouhara@bootlin.com> >> +static int max7360_probe(struct i2c_client *client) >> +{ >> + struct device *dev = &client->dev; >> + struct regmap *regmap; >> + int ret; >> + >> + regmap = devm_regmap_init_i2c(client, &max7360_regmap_config); >> + if (IS_ERR(regmap)) >> + return dev_err_probe(dev, PTR_ERR(regmap), "Failed to initialise regmap\n"); > > dev_err_ptr_probe() > I believe dev_err_ptr_probe() is meant to be used for the opposite case, where the variable holding the error is an int but the function has to return a pointer. Here regmap is a pointer but we have to return an int, so I believe neither dev_err_ptr_probe() or any similar macro can really help us. OK for all other points. Thanks for your review, Mathieu
On Fri, 02 May 2025, Mathieu Dubois-Briand wrote: > On Thu May 1, 2025 at 2:59 PM CEST, Lee Jones wrote: > > On Mon, 28 Apr 2025, mathieu.dubois-briand@bootlin.com wrote: > > > >> From: Kamel Bouhara <kamel.bouhara@bootlin.com> > >> +static int max7360_probe(struct i2c_client *client) > >> +{ > >> + struct device *dev = &client->dev; > >> + struct regmap *regmap; > >> + int ret; > >> + > >> + regmap = devm_regmap_init_i2c(client, &max7360_regmap_config); > >> + if (IS_ERR(regmap)) > >> + return dev_err_probe(dev, PTR_ERR(regmap), "Failed to initialise regmap\n"); > > > > dev_err_ptr_probe() > > > > I believe dev_err_ptr_probe() is meant to be used for the opposite case, > where the variable holding the error is an int but the function has to > return a pointer. Here regmap is a pointer but we have to return an int, > so I believe neither dev_err_ptr_probe() or any similar macro can really > help us. Ah yes, you're right. Disregard.
On Thu, May 01, 2025 at 01:59:43PM +0100, Lee Jones wrote: > On Mon, 28 Apr 2025, mathieu.dubois-briand@bootlin.com wrote: > > > > Add core driver to support MAX7360 i2c chip, multi function device > > with keypad, GPIO, PWM, GPO and rotary encoder submodules. ... > > +static int max7360_probe(struct i2c_client *client) > > +{ > > + struct device *dev = &client->dev; > > + struct regmap *regmap; > > + int ret; > > + > > + regmap = devm_regmap_init_i2c(client, &max7360_regmap_config); > > + if (IS_ERR(regmap)) > > + return dev_err_probe(dev, PTR_ERR(regmap), "Failed to initialise regmap\n"); > > dev_err_ptr_probe() Hmm... This one to return an error pointer casted to the needed type. I think the given code is correct as is. > > + ret = max7360_reset(regmap); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to reset device\n"); > > + > > + /* Get the device out of shutdown mode. */ > > + ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG, > > + MAX7360_GPIO_CFG_GPIO_EN, > > + MAX7360_GPIO_CFG_GPIO_EN); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to enable GPIO and PWM module\n"); > > + > > + ret = max7360_mask_irqs(regmap); > > + if (ret) > > + return dev_err_probe(dev, ret, "Could not mask interrupts\n"); > > + > > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, > > + max7360_cells, ARRAY_SIZE(max7360_cells), > > + NULL, 0, NULL); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to register child devices\n"); > > + > > + return 0; > > +}
On Mon, Apr 28, 2025 at 01:57:20PM +0200, mathieu.dubois-briand@bootlin.com wrote: > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Add core driver to support MAX7360 i2c chip, multi function device > with keypad, GPIO, PWM, GPO and rotary encoder submodules. ... > +static int max7360_mask_irqs(struct regmap *regmap) So, AFAICS this is used only at probe stage... > +{ > + struct device *dev = regmap_get_device(regmap); > + unsigned int val; > + int ret; > + > + /* > + * GPIO/PWM interrupts are not masked on reset: as the MAX7360 "INTI" > + * interrupt line is shared between GPIOs and rotary encoder, this could > + * result in repeated spurious interrupts on the rotary encoder driver > + * if the GPIO driver is not loaded. Mask them now to avoid this > + * situation. > + */ > + for (unsigned int i = 0; i < MAX7360_PORT_PWM_COUNT; i++) { > + ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i), > + MAX7360_PORT_CFG_INTERRUPT_MASK, > + MAX7360_PORT_CFG_INTERRUPT_MASK); > + if (ret) { > + dev_err(dev, "Failed to write max7360 port configuration"); > + return ret; ...if it's the case, use return dev_err_probe(...) here... > + } > + } > + > + /* Read GPIO in register, to ACK any pending IRQ. */ > + ret = regmap_read(regmap, MAX7360_REG_GPIOIN, &val); > + if (ret) > + dev_err(dev, "Failed to read gpio values: %d\n", ret); > + > + return ret; ...and here. > +}
On Mon, Apr 28, 2025 at 01:57:22PM +0200, mathieu.dubois-briand@bootlin.com wrote: > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to > 8 independent PWM outputs. ... > +static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct regmap *regmap; > + int ret; > + regmap = pwmchip_get_drvdata(chip); Just unify this assignment with the definition. Will save you 1 LoC. > + ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(pwm->hwpwm), > + MAX7360_PORT_CFG_COMMON_PWM, 0); > + if (ret) > + return ret; > + > + return regmap_write_bits(regmap, MAX7360_REG_PORTS, BIT(pwm->hwpwm), BIT(pwm->hwpwm)); > +} ... > +static int max7360_pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm, > + const void *_wfhw, struct pwm_waveform *wf) > +{ > + const struct max7360_pwm_waveform *wfhw = _wfhw; > + > + wf->period_length_ns = wfhw->enabled ? MAX7360_PWM_PERIOD_NS : 0; > + wf->duty_offset_ns = 0; > + wf->duty_length_ns = DIV64_U64_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS, Does the numerator have already 64-bit type? Otherwise (u)int*(u)int will be just an (u)int. > + MAX7360_PWM_MAX_RES); > + > + return 0; > +} ... > +static int max7360_pwm_read_waveform(struct pwm_chip *chip, > + struct pwm_device *pwm, > + void *_wfhw) > +{ > + struct max7360_pwm_waveform *wfhw = _wfhw; > + struct regmap *regmap; > + unsigned int val; > + int ret; > + regmap = pwmchip_get_drvdata(chip); > + Save 2 LoCs. > + ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val); > + if (ret) > + return ret; > + > + if (val & BIT(pwm->hwpwm)) { > + wfhw->enabled = true; > + ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val); > + if (ret) > + return ret; > + > + wfhw->duty_steps = val; > + } else { > + wfhw->enabled = false; > + wfhw->duty_steps = 0; > + } > + > + return 0; > +} ... > +static int max7360_pwm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct pwm_chip *chip; > + struct regmap *regmap; > + int ret; > + > + regmap = dev_get_regmap(dev->parent, NULL); > + if (!regmap) > + return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n"); > + device_set_of_node_from_dev(dev, dev->parent); Probably same comment as per pin control driver case? > + chip = devm_pwmchip_alloc(dev, MAX7360_NUM_PWMS, 0); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + chip->ops = &max7360_pwm_ops; > + pwmchip_set_drvdata(chip, regmap); > + ret = devm_pwmchip_add(dev, chip); > + if (ret) > + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); > + > + return 0; > +}
On Mon, Apr 28, 2025 at 01:57:26PM +0200, 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. ... > +#include <linux/bitmap.h> > +#include <linux/err.h> > +#include <linux/gpio/driver.h> Is this still needed? > +#include <linux/gpio/regmap.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/max7360.h> + minmax.h > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/property.h> > +#include <linux/regmap.h> ... > +static int max7360_gpio_probe(struct platform_device *pdev) > +{ > + const struct max7360_gpio_plat_data *plat_data; > + struct gpio_regmap_config gpio_config = { }; > + struct regmap_irq_chip *irq_chip; > + struct device *dev = &pdev->dev; > + struct regmap *regmap; > + unsigned int outconf; > + int ret; > + > + regmap = dev_get_regmap(dev->parent, NULL); > + if (!regmap) > + return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n"); > + > + plat_data = device_get_match_data(dev); > + if (plat_data->function == MAX7360_GPIO_PORT) { > + if (device_property_read_bool(dev, "interrupt-controller")) { > + /* > + * Port GPIOs with interrupt-controller property: add IRQ > + * controller. > + */ > + gpio_config.regmap_irq_flags = IRQF_ONESHOT | IRQF_SHARED; > + gpio_config.regmap_irq_line = > + fwnode_irq_get_byname(dev_fwnode(dev->parent), "inti"); > + if (gpio_config.regmap_irq_line < 0) > + return dev_err_probe(dev, gpio_config.regmap_irq_line, > + "Failed to get IRQ\n"); > + irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL); Can this be made static const instead? > + gpio_config.regmap_irq_chip = irq_chip; > + if (!irq_chip) > + return -ENOMEM; > + > + irq_chip->name = dev_name(dev); > + irq_chip->status_base = MAX7360_REG_GPIOIN; > + irq_chip->status_is_level = true; > + 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->irq_drv_data = regmap; > + for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) { > + ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i), > + MAX7360_PORT_CFG_INTERRUPT_EDGES, > + MAX7360_PORT_CFG_INTERRUPT_EDGES); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to enable interrupts\n"); > + } > + } > + > + /* > + * Port GPIOs: set output mode configuration (constant-current or not). > + * This property is optional. > + */ > + ret = device_property_read_u32(dev, "maxim,constant-current-disable", &outconf); > + if (!ret) { > + ret = regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to set constant-current configuration\n"); > + } > + } > + > + /* Add gpio device. */ > + gpio_config.parent = dev; > + gpio_config.regmap = regmap; > + if (plat_data->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.reg_mask_xlate = max7360_gpio_reg_mask_xlate; > + } else { > + ret = max7360_set_gpos_count(dev, regmap); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to set GPOS pin count\n"); > + > + gpio_config.reg_set_base = GPIO_REGMAP_ADDR(MAX7360_REG_PORTS); > + gpio_config.ngpio = MAX7360_MAX_KEY_COLS; > + gpio_config.init_valid_mask = max7360_gpo_init_valid_mask; > + } > + > + return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &gpio_config)); > +}
On Mon, Apr 28, 2025 at 01:57:28PM +0200, Mathieu Dubois-Briand wrote: > Add driver for Maxim Integrated MAX7360 rotary encoder controller, > supporting a single rotary switch. ... > +struct max7360_rotary { > + struct input_dev *input; > + unsigned int debounce_ms; > + struct regmap *regmap; > + > + u32 steps; > + u32 axis; > + bool relative_axis; > + bool rollover; > + > + unsigned int pos; > +}; I believe `pahole` can recommend better layout (look for the better position of debounce_ms). ... > +static void max7360_rotaty_report_event(struct max7360_rotary *max7360_rotary, int steps) > +{ > + if (max7360_rotary->relative_axis) { > + input_report_rel(max7360_rotary->input, max7360_rotary->axis, steps); > + } else { > + unsigned int pos = max7360_rotary->pos; > + > + if (steps < 0) { > + /* turning counter-clockwise */ > + if (max7360_rotary->rollover) > + pos += max7360_rotary->steps + steps; > + else > + pos = max(0, (int)pos + steps); Please, no castings for min()/max()/clamp(). It diminishes the use of those macros. > + } else { > + /* turning clockwise */ > + if (max7360_rotary->rollover) > + pos += steps; > + else > + pos = min(max7360_rotary->steps, pos + steps); > + } > + > + if (max7360_rotary->rollover) > + pos %= max7360_rotary->steps; > + > + max7360_rotary->pos = pos; > + input_report_abs(max7360_rotary->input, max7360_rotary->axis, max7360_rotary->pos); > + } > + > + input_sync(max7360_rotary->input); > +} ... > +static irqreturn_t max7360_rotary_irq(int irq, void *data) > +{ > + struct max7360_rotary *max7360_rotary = data; > + struct device *dev = max7360_rotary->input->dev.parent; > + unsigned int val; > + int error; > + > + error = regmap_read(max7360_rotary->regmap, MAX7360_REG_RTR_CNT, &val); > + if (error < 0) { > + dev_err(dev, "Failed to read rotary counter\n"); > + return IRQ_NONE; > + } > + > + if (val == 0) { > + dev_dbg(dev, "Got a spurious interrupt\n"); This contradicts with the IRQF_SHARED. Drop one of them. > + return IRQ_NONE; > + } > + > + max7360_rotaty_report_event(max7360_rotary, sign_extend32(val, 7)); > + > + return IRQ_HANDLED; > +} ... > +static int max7360_rotary_hw_init(struct max7360_rotary *max7360_rotary) > +{ > + struct device *dev = max7360_rotary->input->dev.parent; > + int val; > + int error; > + > + val = FIELD_PREP(MAX7360_ROT_DEBOUNCE, max7360_rotary->debounce_ms) | > + FIELD_PREP(MAX7360_ROT_INTCNT, 1) | MAX7360_ROT_INTCNT_DLY; Indentation is incorrect. > + error = regmap_write(max7360_rotary->regmap, MAX7360_REG_RTRCFG, val); > + if (error) > + dev_err(dev, "Failed to set max7360 rotary encoder configuration\n"); > + > + return error; > +} ... > +static int max7360_rotary_probe(struct platform_device *pdev) > +{ > + struct max7360_rotary *max7360_rotary; > + struct device *dev = &pdev->dev; > + struct input_dev *input; > + struct regmap *regmap; > + int irq; > + int error; > + > + regmap = dev_get_regmap(dev->parent, NULL); > + if (!regmap) > + dev_err_probe(dev, -ENODEV, "Could not get parent regmap\n"); Missing return. Copy'n'paste error over all drivers? > + irq = fwnode_irq_get_byname(dev_fwnode(dev->parent), "inti"); > + if (irq < 0) > + return dev_err_probe(dev, irq, "Failed to get IRQ\n"); > + > + max7360_rotary = devm_kzalloc(dev, sizeof(*max7360_rotary), GFP_KERNEL); > + if (!max7360_rotary) > + return -ENOMEM; > + > + max7360_rotary->regmap = regmap; > + > + device_property_read_u32(dev->parent, "linux,axis", &max7360_rotary->axis); > + max7360_rotary->rollover = device_property_read_bool(dev->parent, > + "rotary-encoder,rollover"); > + max7360_rotary->relative_axis = > + device_property_read_bool(dev->parent, "rotary-encoder,relative-axis"); > + > + error = device_property_read_u32(dev->parent, "rotary-encoder,steps", > + &max7360_rotary->steps); > + if (error) > + max7360_rotary->steps = MAX7360_ROTARY_DEFAULT_STEPS; > + > + device_property_read_u32(dev->parent, "rotary-debounce-delay-ms", > + &max7360_rotary->debounce_ms); > + if (max7360_rotary->debounce_ms > MAX7360_ROT_DEBOUNCE_MAX) > + return dev_err_probe(dev, -EINVAL, "Invalid debounce timing: %u\n", > + max7360_rotary->debounce_ms); > + > + input = devm_input_allocate_device(dev); > + if (!input) > + return -ENOMEM; > + > + max7360_rotary->input = input; > + > + input->id.bustype = BUS_I2C; > + input->name = pdev->name; > + > + if (max7360_rotary->relative_axis) > + input_set_capability(input, EV_REL, max7360_rotary->axis); > + else > + input_set_abs_params(input, max7360_rotary->axis, 0, max7360_rotary->steps, 0, 1); > + > + error = devm_request_threaded_irq(dev, irq, NULL, max7360_rotary_irq, > + IRQF_ONESHOT | IRQF_SHARED, > + "max7360-rotary", max7360_rotary); > + if (error) > + return dev_err_probe(dev, error, "Failed to register interrupt\n"); > + > + error = input_register_device(input); > + if (error) > + return dev_err_probe(dev, error, "Could not register input device\n"); > + > + error = max7360_rotary_hw_init(max7360_rotary); > + if (error) > + return dev_err_probe(dev, error, "Failed to initialize max7360 rotary\n"); > + > + device_init_wakeup(dev, true); > + error = dev_pm_set_wake_irq(dev, irq); > + if (error) > + dev_warn(dev, "Failed to set up wakeup irq: %d\n", error); > + > + return 0; > +} > + > +static void max7360_rotary_remove(struct platform_device *pdev) > +{ > + dev_pm_clear_wake_irq(&pdev->dev); Shouldn't be here device_init_wakeup(dev, false); ? > +}
On Fri May 2, 2025 at 12:19 PM CEST, Andy Shevchenko wrote: > On Mon, Apr 28, 2025 at 01:57:22PM +0200, mathieu.dubois-briand@bootlin.com wrote: >> From: Kamel Bouhara <kamel.bouhara@bootlin.com> >> >> +static int max7360_pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm, >> + const void *_wfhw, struct pwm_waveform *wf) >> +{ >> + const struct max7360_pwm_waveform *wfhw = _wfhw; >> + >> + wf->period_length_ns = wfhw->enabled ? MAX7360_PWM_PERIOD_NS : 0; >> + wf->duty_offset_ns = 0; >> + wf->duty_length_ns = DIV64_U64_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS, > > Does the numerator have already 64-bit type? Otherwise (u)int*(u)int will be > just an (u)int. > Err no, this section has been modified back and forth, but today we have u8 * 2 * 1000000L, so we will always fit in a u32. I will use DIV_ROUND_UP() instead. > ... OK with all other comments. Thanks for your review. Mathieu
On Fri May 2, 2025 at 12:39 PM CEST, Andy Shevchenko wrote: > On Mon, Apr 28, 2025 at 01:57:26PM +0200, Mathieu Dubois-Briand wrote: >> +static int max7360_gpio_probe(struct platform_device *pdev) >> +{ >> + const struct max7360_gpio_plat_data *plat_data; >> + struct gpio_regmap_config gpio_config = { }; >> + struct regmap_irq_chip *irq_chip; >> + struct device *dev = &pdev->dev; >> + struct regmap *regmap; >> + unsigned int outconf; >> + int ret; >> + >> + regmap = dev_get_regmap(dev->parent, NULL); >> + if (!regmap) >> + return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n"); >> + >> + plat_data = device_get_match_data(dev); >> + if (plat_data->function == MAX7360_GPIO_PORT) { >> + if (device_property_read_bool(dev, "interrupt-controller")) { >> + /* >> + * Port GPIOs with interrupt-controller property: add IRQ >> + * controller. >> + */ >> + gpio_config.regmap_irq_flags = IRQF_ONESHOT | IRQF_SHARED; >> + gpio_config.regmap_irq_line = >> + fwnode_irq_get_byname(dev_fwnode(dev->parent), "inti"); >> + if (gpio_config.regmap_irq_line < 0) >> + return dev_err_probe(dev, gpio_config.regmap_irq_line, >> + "Failed to get IRQ\n"); > >> + irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL); > > Can this be made static const instead? > Sorry, I don't think we can. We do have a few data that will vary: ->name, but above all ->irq_drv_data, as it will point on the regmap of the specific device. > ... OK with all other comments. Thanks for your review. Mathieu
On Fri, May 02, 2025 at 02:31:13PM +0200, Mathieu Dubois-Briand wrote: > On Fri May 2, 2025 at 12:39 PM CEST, Andy Shevchenko wrote: > > On Mon, Apr 28, 2025 at 01:57:26PM +0200, Mathieu Dubois-Briand wrote: ... > >> + irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL); > > > > Can this be made static const instead? > > Sorry, I don't think we can. We do have a few data that will vary: > ->name, but above all ->irq_drv_data, as it will point on the regmap of > the specific device. I see, perhaps a (oneline) comment?
On Fri May 2, 2025 at 12:52 PM CEST, Andy Shevchenko wrote: > On Mon, Apr 28, 2025 at 01:57:28PM +0200, Mathieu Dubois-Briand wrote: >> Add driver for Maxim Integrated MAX7360 rotary encoder controller, >> supporting a single rotary switch. > > ... > >> +struct max7360_rotary { >> + struct input_dev *input; >> + unsigned int debounce_ms; >> + struct regmap *regmap; >> + >> + u32 steps; >> + u32 axis; >> + bool relative_axis; >> + bool rollover; >> + >> + unsigned int pos; >> +}; > > I believe `pahole` can recommend better layout (look for the better position > of debounce_ms). > > ... > Oh yes it does on arm64. Moving it after the regmap pointer, it should be better. >> +static void max7360_rotaty_report_event(struct max7360_rotary *max7360_rotary, int steps) >> +{ >> + if (max7360_rotary->relative_axis) { >> + input_report_rel(max7360_rotary->input, max7360_rotary->axis, steps); >> + } else { >> + unsigned int pos = max7360_rotary->pos; >> + >> + if (steps < 0) { >> + /* turning counter-clockwise */ >> + if (max7360_rotary->rollover) >> + pos += max7360_rotary->steps + steps; >> + else > >> + pos = max(0, (int)pos + steps); > > Please, no castings for min()/max()/clamp(). It diminishes the use of those > macros. > Sorry, I'm not sure to get the point. Should I use MIN_T() instead? > > ... > >> +static int max7360_rotary_probe(struct platform_device *pdev) >> +{ >> + struct max7360_rotary *max7360_rotary; >> + struct device *dev = &pdev->dev; >> + struct input_dev *input; >> + struct regmap *regmap; >> + int irq; >> + int error; >> + >> + regmap = dev_get_regmap(dev->parent, NULL); >> + if (!regmap) >> + dev_err_probe(dev, -ENODEV, "Could not get parent regmap\n"); > > Missing return. Copy'n'paste error over all drivers? > Probably more an error while replacing all dev_err()+return by dev_err_probe(), but yes. I will look for similar issues. > ... OK with all other comments. Thanks for your review. Mathieu
On Fri, May 02, 2025 at 03:58:04PM +0200, Mathieu Dubois-Briand wrote: > On Fri May 2, 2025 at 12:52 PM CEST, Andy Shevchenko wrote: > > On Mon, Apr 28, 2025 at 01:57:28PM +0200, Mathieu Dubois-Briand wrote: ... > >> + pos = max(0, (int)pos + steps); > > > > Please, no castings for min()/max()/clamp(). It diminishes the use of those > > macros. > > Sorry, I'm not sure to get the point. Should I use MIN_T() instead? Are the second argument is compile-time constant? I don't think so. Hence no use for MIN*()/MAX*(). First of all, try to answer to the Q: Why is the explicit casting being used? The second Q: How can it be easily fixed without using _t() variants of the macros?
On Fri May 2, 2025 at 4:09 PM CEST, Andy Shevchenko wrote: > On Fri, May 02, 2025 at 03:58:04PM +0200, Mathieu Dubois-Briand wrote: >> On Fri May 2, 2025 at 12:52 PM CEST, Andy Shevchenko wrote: >> > On Mon, Apr 28, 2025 at 01:57:28PM +0200, Mathieu Dubois-Briand wrote: > > ... > >> >> + pos = max(0, (int)pos + steps); >> > >> > Please, no castings for min()/max()/clamp(). It diminishes the use of those >> > macros. >> >> Sorry, I'm not sure to get the point. Should I use MIN_T() instead? > > Are the second argument is compile-time constant? I don't think so. Hence no > use for MIN*()/MAX*(). First of all, try to answer to the Q: Why is the explicit > casting being used? The second Q: How can it be easily fixed without using _t() > variants of the macros? Err right, no MIN/MAX of course. Explicit cast is here because the unsigned int + int sum would result in an unsigned int. I will use an intermediate signed value. Also the whole logic here can be simplified a bit, so I will rework the whole block. Best regards, Mathieu
This series implements a set of drivers allowing to support the Maxim Integrated MAX7360 device. The MAX7360 is an I2C key-switch and led controller, with following functionalities: - Keypad controller for a key matrix of up to 8 rows and 8 columns. - Rotary encoder support, for a single rotary encoder. - Up to 8 PWM outputs. - Up to 8 GPIOs with support for interrupts and 6 GPOs. Chipset pins are shared between all functionalities, so all cannot be used at the same time. Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> --- Changes in v7: - Add rotary encoder absolute axis support in device tree bindings and driver. - Lot of small changes in keypad, rotary encoder and GPIO drivers. - Rebased on v6.15-rc4 - Link to v6: https://lore.kernel.org/r/20250409-mdb-max7360-support-v6-0-7a2535876e39@bootlin.com Changes in v6: - Rebased on v6.15-rc1. - Use device_set_of_node_from_dev() instead of creating PWM and Pinctrl on parent device. - Various small fixes in all drivers. - Fix pins property pattern in pinctrl dt bindings. - Link to v5: https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-0-fb20baf97da0@bootlin.com Changes in v5: - Add pinctrl driver to replace the previous use of request()/free() callbacks for PORT pins. - Remove ngpios property from GPIO device tree bindings. - Use GPIO valid_mask to mark unusable keypad columns GPOs, instead of changing ngpios. - Drop patches adding support for request()/free() callbacks in GPIO regmap and gpio_regmap_get_ngpio(). - Allow gpio_regmap_register() to create the associated regmap IRQ. - Various fixes in MFD, PWM, GPIO and KEYPAD drivers. - Link to v4: https://lore.kernel.org/r/20250214-mdb-max7360-support-v4-0-8a35c6dbb966@bootlin.com Changes in v4: - Modified the GPIO driver to use gpio-regmap and regmap-irq. - Add support for request()/free() callbacks in gpio-regmap. - Add support for status_is_level in regmap-irq. - Switched the PWM driver to waveform callbacks. - Various small fixes in MFD, PWM, GPIO drivers and dt bindings. - Rebased on v6.14-rc2. - Link to v3: https://lore.kernel.org/r/20250113-mdb-max7360-support-v3-0-9519b4acb0b1@bootlin.com Changes in v3: - Fix MFD device tree binding to add gpio child nodes. - Fix various small issues in device tree bindings. - Add missing line returns in error messages. - Use dev_err_probe() when possible. - Link to v2: https://lore.kernel.org/r/20241223-mdb-max7360-support-v2-0-37a8d22c36ed@bootlin.com Changes in v2: - Removing device tree subnodes for keypad, rotary encoder and pwm functionalities. - Fixed dt-bindings syntax and naming. - Fixed missing handling of requested period in PWM driver. - Cleanup of the code - Link to v1: https://lore.kernel.org/r/20241219-mdb-max7360-support-v1-0-8e8317584121@bootlin.com --- Kamel Bouhara (2): mfd: Add max7360 support pwm: max7360: Add MAX7360 PWM support Mathieu Dubois-Briand (9): dt-bindings: mfd: gpio: Add MAX7360 pinctrl: Add MAX7360 pinctrl driver regmap: irq: Add support for chips without separate IRQ status gpio: regmap: Allow to allocate regmap-irq device gpio: regmap: Allow to provide init_valid_mask callback gpio: max7360: Add MAX7360 gpio support input: keyboard: Add support for MAX7360 keypad input: misc: Add support for MAX7360 rotary MAINTAINERS: Add entry on MAX7360 driver .../bindings/gpio/maxim,max7360-gpio.yaml | 83 ++++++ .../devicetree/bindings/mfd/maxim,max7360.yaml | 191 +++++++++++++ MAINTAINERS | 13 + drivers/base/regmap/regmap-irq.c | 98 ++++--- drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-max7360.c | 255 +++++++++++++++++ drivers/gpio/gpio-regmap.c | 22 +- drivers/input/keyboard/Kconfig | 12 + drivers/input/keyboard/Makefile | 1 + drivers/input/keyboard/max7360-keypad.c | 302 +++++++++++++++++++++ drivers/input/misc/Kconfig | 10 + drivers/input/misc/Makefile | 1 + drivers/input/misc/max7360-rotary.c | 198 ++++++++++++++ drivers/mfd/Kconfig | 14 + drivers/mfd/Makefile | 1 + drivers/mfd/max7360.c | 184 +++++++++++++ drivers/pinctrl/Kconfig | 11 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-max7360.c | 208 ++++++++++++++ drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-max7360.c | 181 ++++++++++++ include/linux/gpio/regmap.h | 18 ++ include/linux/mfd/max7360.h | 109 ++++++++ include/linux/regmap.h | 3 + 26 files changed, 1907 insertions(+), 33 deletions(-) --- base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e change-id: 20241219-mdb-max7360-support-223a8ce45ba3 Best regards,