| Message ID | 20250409-mdb-max7360-support-v6-0-7a2535876e39@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for MAX7360 | expand |
On Wed Apr 9, 2025 at 5:22 PM CEST, ALOK TIWARI wrote: > > > On 09-04-2025 20:25, Mathieu Dubois-Briand wrote: >> Add device tree bindings for Maxim Integrated MAX7360 device with >> support for keypad, rotary, gpios and pwm functionalities. >> >> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> >> --- >> .../bindings/gpio/maxim,max7360-gpio.yaml | 83 ++++++++++ >> .../devicetree/bindings/mfd/maxim,max7360.yaml | 171 +++++++++++++++++++++ >> 2 files changed, 254 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml >> new file mode 100644 >> index 000000000000..21d603d9504c >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml >> @@ -0,0 +1,83 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: https://urldefense.com/v3/__http://devicetree.org/schemas/gpio/maxim,max7360-gpio.yaml*__;Iw!!ACWV5N9M2RV99hQ!LySDuQZdU3DANTEmkRlntMCbFm69zp24O0wAwuujlnN1Zh9-xPEHZu7fj5d_O7vIxUHn9b6gqg9MHtd9ntPvXQvakCad_v0$ >> +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!ACWV5N9M2RV99hQ!LySDuQZdU3DANTEmkRlntMCbFm69zp24O0wAwuujlnN1Zh9-xPEHZu7fj5d_O7vIxUHn9b6gqg9MHtd9ntPvXQvacsB3d9k$ >> + >> +title: Maxim MAX7360 GPIO controller >> + >> +maintainers: >> + - Kamel Bouhara <kamel.bouhara@bootlin.com> >> + - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> >> + >> +description: | >> + Maxim MAX7360 GPIO controller, in MAX7360 chipset >> + https://urldefense.com/v3/__https://www.analog.com/en/products/max7360.html__;!!ACWV5N9M2RV99hQ!LySDuQZdU3DANTEmkRlntMCbFm69zp24O0wAwuujlnN1Zh9-xPEHZu7fj5d_O7vIxUHn9b6gqg9MHtd9ntPvXQvavZnHZJk$ >> + >> + The device provide two series of GPIOs, referred here as GPIOs and GPOs. > typo: The device provides two series of GPIOs, >> + >> + PORT0 to PORT7 pins can be used as GPIOs, with support for interrupts and >> + constant-current mode. These pins will also be used by the torary encoder and > typo: ie rotary encoder ? >> + PWM functionalities. >> + >> + COL2 to COL7 pins can be used as GPOs, there is no input capability. COL pins >> + will be partitionned, with the first pins being affected to the keypad >> + functionality and the last ones as GPOs. >> + > typo: partitionned -> partitioned Thanks for your review, I fixed all 3 typos.
On Wed Apr 9, 2025 at 5:21 PM CEST, Mark Brown wrote: > On Wed, Apr 09, 2025 at 04:19:34PM +0100, Mark Brown wrote: >> On Wed, Apr 09, 2025 at 04:55:52PM +0200, Mathieu Dubois-Briand wrote: >> > BUG() never returns, so code after it is unreachable: remove it. >> >> BUG() can be compiled out, CONFIG_BUG. > > Also, please don't mix irrelevant patches into random serieses. It just > makes everything noisier for everyone. Hi Mark, Just to provide the context about why this change is part of this series: this goto, if left unmodified, would have to replaced by a return. This is how the topic of dropping it came in the previous iteration of this series. Thanks for your review. Mathieu
On Wed Apr 9, 2025 at 6:39 PM CEST, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 04:55:54PM +0200, Mathieu Dubois-Briand wrote: >> GPIO controller often have support for IRQ: allow to easily allocate >> both gpio-regmap and regmap-irq in one operation. > >> >> - memcpy(d->prev_status_buf, d->status_buf, array_size(d->prev_status_buf)); >> + memcpy(d->prev_status_buf, d->status_buf, >> + array_size(d->chip->num_regs, sizeof(d->prev_status_buf[0]))); > > ... > >> +#ifdef CONFIG_REGMAP_IRQ >> + if (config->regmap_irq_chip) { >> + struct regmap_irq_chip_data *irq_chip_data; >> + >> + ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent), >> + config->regmap, config->regmap_irq_irqno, >> + config->regmap_irq_flags, 0, >> + config->regmap_irq_chip, &irq_chip_data); >> + if (ret) >> + goto err_free_gpio; >> + >> + irq_domain = regmap_irq_get_domain(irq_chip_data); >> + } else >> +#endif >> + irq_domain = config->irq_domain; > >> + > > This is blank line is not needed, but I not mind either way. > I can remove it, but as the line above is potentially part of the "else", I have a small preference for keeping it. >> + if (irq_domain) { >> + ret = gpiochip_irqchip_add_domain(chip, irq_domain); >> if (ret) >> goto err_remove_gpiochip; >> } > > ... > >> + * @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. > > Leftover? Yes, sorry... > >> + * @regmap_irq_irqno (Optional) The IRQ the device uses to signal interrupts. >> + * @regmap_irq_flags (Optional) The IRQF_ flags to use for the interrupt. > > ... > >> +#ifdef CONFIG_REGMAP_IRQ >> + struct regmap_irq_chip *regmap_irq_chip; >> + int regmap_irq_irqno; > > Perhaps call it line? > > int regmap_irq_line; > Makes sense, thanks. >> + unsigned long regmap_irq_flags; >> +#endif Thanks for your review. Mathieu
Wed, Apr 09, 2025 at 10:16:40AM -0700, Dmitry Torokhov kirjoitti: > On Wed, Apr 09, 2025 at 07:45:42PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 09, 2025 at 05:32:55PM +0100, Mark Brown wrote: > > > On Wed, Apr 09, 2025 at 07:00:24PM +0300, Andy Shevchenko wrote: > > > > On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > > > > > > > > unreachable() just annotates things, AFAICT it doesn't actually > > > > > guarantee to do anything in particular if the annotation turns out to be > > > > > incorrect. > > > > > > > I;m not sure I follow. unreachable is a wrapper on top of > > > > __builtin_unreachable() which is intrinsic of the compiler. > > > > > > > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable > > > > > > That just says that the program is undefined if we get to the > > > __builtin_undefined() and documents some behaviour around warnings. One > > > example of undefined behaviour would be doing nothing. > > > > Theoretically yes, practically return after a BUG() makes no sense. Note, > > that compiler effectively removes 'goto exit;' here (that's also mentioned > > in the documentation independently on the control flow behaviour), so > > I don't know what you expect from it. > > So unreachable() sometimes lears to weird behavior from compiler, for > example as mentioned here where we ended up removing it to prevent > miscompilations: > > https://lore.kernel.org/all/20241010222451.GA3571761@thelio-3990X/ How does it affect the BUG()?
On Wed, 09 Apr 2025 16:55:48 +0200, Mathieu Dubois-Briand wrote: > Add device tree bindings for Maxim Integrated MAX7360 device with > support for keypad, rotary, gpios and pwm functionalities. > > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > --- > .../bindings/gpio/maxim,max7360-gpio.yaml | 83 ++++++++++ > .../devicetree/bindings/mfd/maxim,max7360.yaml | 171 +++++++++++++++++++++ > 2 files changed, 254 insertions(+) > With the typos fixed, Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
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 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 (10): dt-bindings: mfd: gpio: Add MAX7360 pinctrl: Add MAX7360 pinctrl driver regmap: irq: Remove unreachable goto 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 | 171 ++++++++++++ MAINTAINERS | 13 + drivers/base/regmap/regmap-irq.c | 97 ++++--- drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-max7360.c | 250 +++++++++++++++++ drivers/gpio/gpio-regmap.c | 22 +- drivers/input/keyboard/Kconfig | 12 + drivers/input/keyboard/Makefile | 1 + drivers/input/keyboard/max7360-keypad.c | 299 +++++++++++++++++++++ drivers/input/misc/Kconfig | 10 + drivers/input/misc/Makefile | 1 + drivers/input/misc/max7360-rotary.c | 148 ++++++++++ drivers/mfd/Kconfig | 14 + drivers/mfd/Makefile | 1 + drivers/mfd/max7360.c | 186 +++++++++++++ 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 | 190 +++++++++++++ include/linux/gpio/regmap.h | 21 ++ include/linux/mfd/max7360.h | 109 ++++++++ include/linux/regmap.h | 3 + 26 files changed, 1842 insertions(+), 33 deletions(-) --- base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 change-id: 20241219-mdb-max7360-support-223a8ce45ba3 Best regards,