| Message ID | 20250214-mdb-max7360-support-v4-0-8a35c6dbb966@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for MAX7360 | expand |
On Fri Feb 14, 2025 at 12:49 PM CET, 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. > > Co-developed-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > --- ... > +static int max7360_gpio_probe(struct platform_device *pdev) ... > + } 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; The device_property_read_u32() and setting of gpio_config.ngpio here will be removed, once the "gpio: regmap: Make use of 'ngpios' property" series gets merged. https://lore.kernel.org/linux-gpio/20250213195621.3133406-1-andriy.shevchenko@linux.intel.com/
On Fri Feb 14, 2025 at 4:18 PM CET, Andy Shevchenko wrote: > On Fri, Feb 14, 2025 at 12:49:56PM +0100, Mathieu Dubois-Briand wrote: > > Some GPIO chips allow to rise an IRQ on GPIO level changes but do not > > provide an IRQ status for each separate line: only the current gpio > > level can be retrieved. > > > > Add support for these chips, emulating IRQ status by comparing GPIO > > levels with the levels during the previous interrupt. > > Thanks, this will help to convert more drivers to regmap > (e.g., gpio-pca953x that seems use similar approach). > > ... > > > +static irqreturn_t regmap_irq_thread(int irq, void *d) > > +{ > > + struct regmap_irq_chip_data *data = d; > > + const struct regmap_irq_chip *chip = data->chip; > > + struct regmap *map = data->map; > > + int ret, i; > > unsigned int i; > ? I agree, but signed int index variables are used in all functions of this file. What would be the best approach here? Only fix it on code parts I modified? On the whole file? > > > + bool handled = false; > > + u32 reg; > > + > > + if (chip->handle_pre_irq) > > + chip->handle_pre_irq(chip->irq_drv_data); > > + > > + if (chip->runtime_pm) { > > + ret = pm_runtime_get_sync(map->dev); > > + if (ret < 0) { > > > + dev_err(map->dev, "IRQ thread failed to resume: %d\n", > > + ret); > > Can be one line. > Yes. Kind of the same question here: should I fix only the code close to the parts I modified or the whole file? > ... > > > + for (i = 0; i < d->chip->num_regs; i++) > > + d->prev_status_buf[i] = d->status_buf[i]; > > Hmm... Wouldn't memcpy() suffice? > But okay, this seems to be not a hot path and the intention is clear. Yes... I don't know why I didn't use memcpy. I will fix it.
On Fri, Feb 14, 2025 at 04:49:57PM +0100, Mathieu Dubois-Briand wrote: > On Fri Feb 14, 2025 at 4:18 PM CET, Andy Shevchenko wrote: > > On Fri, Feb 14, 2025 at 12:49:56PM +0100, Mathieu Dubois-Briand wrote: ... > > > +static irqreturn_t regmap_irq_thread(int irq, void *d) > > > +{ > > > + struct regmap_irq_chip_data *data = d; > > > + const struct regmap_irq_chip *chip = data->chip; > > > + struct regmap *map = data->map; > > > + int ret, i; > > > > unsigned int i; > > ? > > I agree, but signed int index variables are used in all functions of > this file. What would be the best approach here? Only fix it on code > parts I modified? On the whole file? I would change in the code you touched, > > > + bool handled = false; > > > + u32 reg; > > > + > > > + if (chip->handle_pre_irq) > > > + chip->handle_pre_irq(chip->irq_drv_data); > > > + > > > + if (chip->runtime_pm) { > > > + ret = pm_runtime_get_sync(map->dev); > > > + if (ret < 0) { > > > > > + dev_err(map->dev, "IRQ thread failed to resume: %d\n", > > > + ret); > > > > Can be one line. > > Yes. Kind of the same question here: should I fix only the code close to > the parts I modified or the whole file? Same, it falls under the "common sense" category.
On Fri, Feb 14, 2025 at 05:59:40PM +0200, Andy Shevchenko wrote: > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote: ... > > + ret = device_property_read_u32(dev, "maxim,constant-current-disable", &outconf); > > + if (ret && (ret != -EINVAL)) > > + return dev_err_probe(dev, -ENODEV, > > Why shadowing the real error code? > > > + "Failed to read maxim,constant-current-disable OF property\n"); > > It may be not only OF :-) Btw, can you compare this approach and the below in terms of bloat-o-meter against the object file size? // can be done without this as well, just the same string as a parameter const char *propname; propname = "maxim,constant-current-disable"; ret = device_property_read_u32(dev, propname, &outconf); if (ret && (ret != -EINVAL)) return dev_err_probe(dev, ret, "Failed to read %s device property\n", propname); While the above is strong hint to the compiler, the below should give similar result but by the duplicates elimination: ret = device_property_read_u32(dev, "maxim,constant-current-disable", &outconf); if (ret && (ret != -EINVAL)) return dev_err_probe(dev, ret, "Failed to read %s device property\n", "maxim,constant-current-disable");
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 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 (8): dt-bindings: mfd: gpio: Add MAX7360 gpio: regmap: Allow to provide request and free callbacks gpio: regmap: Allow to retrieve ngpio regmap: irq: Add support for chips without separate IRQ status 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 | 91 +++++++ .../devicetree/bindings/mfd/maxim,max7360.yaml | 139 ++++++++++ MAINTAINERS | 12 + drivers/base/regmap/regmap-irq.c | 83 ++++-- drivers/gpio/Kconfig | 11 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-max7360.c | 253 ++++++++++++++++++ drivers/gpio/gpio-regmap.c | 8 + drivers/input/keyboard/Kconfig | 12 + drivers/input/keyboard/Makefile | 1 + drivers/input/keyboard/max7360-keypad.c | 282 +++++++++++++++++++++ drivers/input/misc/Kconfig | 11 + drivers/input/misc/Makefile | 1 + drivers/input/misc/max7360-rotary.c | 182 +++++++++++++ drivers/mfd/Kconfig | 14 + drivers/mfd/Makefile | 1 + drivers/mfd/max7360.c | 218 ++++++++++++++++ drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-max7360.c | 213 ++++++++++++++++ include/linux/gpio/regmap.h | 10 + include/linux/mfd/max7360.h | 112 ++++++++ include/linux/regmap.h | 3 + 23 files changed, 1649 insertions(+), 20 deletions(-) --- base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3 change-id: 20241219-mdb-max7360-support-223a8ce45ba3 Best regards,