| 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 4:55 PM CEST, Mathieu Dubois-Briand wrote: > Add driver for Maxim Integrated MAX7360 pinctrl on the PORT pins. Pins > can be used either for GPIO, PWM or rotary encoder functionalities. > > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > ... > diff --git a/drivers/pinctrl/pinctrl-max7360.c b/drivers/pinctrl/pinctrl-max7360.c > ... > +static int max7360_pinctrl_probe(struct platform_device *pdev) > +{ > + struct regmap *regmap; > + struct pinctrl_desc *pd; > + struct max7360_pinctrl *chip; > + struct device *dev = &pdev->dev; > + > + regmap = dev_get_regmap(dev->parent, NULL); > + if (!regmap) > + dev_err_probe(dev, -ENODEV, "Could not get parent regmap\n"); > + > + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + pd = &chip->pinctrl_desc; > + > + pd->pctlops = &max7360_pinctrl_ops; > + pd->pmxops = &max7360_pmxops; > + pd->name = dev_name(dev); > + pd->pins = max7360_pins; > + pd->npins = MAX7360_MAX_GPIO; > + pd->owner = THIS_MODULE; > + > + device_set_of_node_from_dev(dev, dev->parent); Ok, so this goes a bit against what I said I was going to do on my previous series, let me explain why. Same reasoning applies for both uses, in PWM and pinctrl drivers. With my previous experiments, I came to the conclusion that: - Either we should use device_set_of_node_from_dev() as I do here. - Or we should add more subnodes in the device tree binding. - Also, copying the fwnode with device_set_node() was not possible, as the kernel would then try to apply pinctrl on both the parent and child device. I previously said the second solution was probably the way to go, but I changed my mind for two reasons. First having more subnodes in the device tree was already rejected in the past in the reviews of the dt-bindings patch. This do makes sense as it would be describing device internals (which should not be made in DT), just to ease one specific software implementation (which should also be avoided). So I believe this change would again be rejected. https://lore.kernel.org/lkml/58c80c2a-2532-4bc5-9c9f-52480b3af52a@kernel.org/ But the the second reason is, doing 'git grep "device_set_of_node_from_dev.*parent"', I found several drivers using device_set_of_node_from_dev() for a similar need. Some of these uses are also for MFD child devices: - gpio-adp5585.c / pwm-adp5585.c, - pwm-ntxec.c, - max77620-regulator.c / max77620_thermal.c. So, based on this, I believe using device_set_of_node_from_dev() in these two drivers is the way to go. > + chip->pctldev = devm_pinctrl_register(dev, pd, chip); > + if (IS_ERR(chip->pctldev)) > + return dev_err_probe(dev, PTR_ERR(chip->pctldev), "can't register controller\n"); > + > + return 0; > +} > +
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.
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. Thank you, this is the right change to do. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Wed, Apr 09, 2025 at 06:38:32PM +0300, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 04:19:27PM +0100, Mark Brown wrote: > > BUG() can be compiled out, CONFIG_BUG. > Yes, and it's still has unreachable() there. So, this change is correct. > See include/asm-generic/bug.h for the details of the implementation. > And yes, if we have an architecture that does not do this way, it has to > be fixed. unreachable() just annotates things, AFAICT it doesn't actually guarantee to do anything in particular if the annotation turns out to be incorrect.
On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > On Wed, Apr 09, 2025 at 06:38:32PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 09, 2025 at 04:19:27PM +0100, Mark Brown wrote: > > > > BUG() can be compiled out, CONFIG_BUG. > > > Yes, and it's still has unreachable() there. So, this change is correct. > > See include/asm-generic/bug.h for the details of the implementation. > > And yes, if we have an architecture that does not do this way, it has to > > be fixed. > > 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
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.
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.
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/ Thanks.
Hi Mathieu, On Wed, Apr 09, 2025 at 04:55:48PM +0200, Mathieu Dubois-Briand wrote: > Add device tree bindings for Maxim Integrated MAX7360 device with > support for keypad, rotary, gpios and pwm functionalities. > > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + io-expander@38 { > + compatible = "maxim,max7360"; > + reg = <0x38>; > + > + interrupt-parent = <&gpio1>; > + interrupts = <23 IRQ_TYPE_LEVEL_LOW>, > + <24 IRQ_TYPE_LEVEL_LOW>; > + interrupt-names = "inti", "intk"; > + > + keypad,num-rows = <8>; > + keypad,num-columns = <4>; > + linux,keymap = < > + MATRIX_KEY(0x00, 0x00, KEY_F5) > + MATRIX_KEY(0x01, 0x00, KEY_F4) > + MATRIX_KEY(0x02, 0x01, KEY_F6) > + >; > + keypad-debounce-delay-ms = <10>; > + autorepeat; > + > + rotary-debounce-delay-ms = <2>; > + linux,axis = <0>; /* REL_X */ Probably this has been already discussed, but shouldn't keyboard and rotary encoder be represented as sub-nodes here, similar to how GPIO block is represented? Thanks.
On Wed, 09 Apr 2025, 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 Please trim your responses. You comments should have blank lines above below your comments too please.
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 10:58 PM CEST, Dmitry Torokhov wrote: > Hi Mathieu, Hi Dmitry, > > On Wed, Apr 09, 2025 at 04:55:48PM +0200, Mathieu Dubois-Briand wrote: >> Add device tree bindings for Maxim Integrated MAX7360 device with >> support for keypad, rotary, gpios and pwm functionalities. >> >> + >> + i2c { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + io-expander@38 { >> + compatible = "maxim,max7360"; >> + reg = <0x38>; >> + >> + interrupt-parent = <&gpio1>; >> + interrupts = <23 IRQ_TYPE_LEVEL_LOW>, >> + <24 IRQ_TYPE_LEVEL_LOW>; >> + interrupt-names = "inti", "intk"; >> + >> + keypad,num-rows = <8>; >> + keypad,num-columns = <4>; >> + linux,keymap = < >> + MATRIX_KEY(0x00, 0x00, KEY_F5) >> + MATRIX_KEY(0x01, 0x00, KEY_F4) >> + MATRIX_KEY(0x02, 0x01, KEY_F6) >> + >; >> + keypad-debounce-delay-ms = <10>; >> + autorepeat; >> + >> + rotary-debounce-delay-ms = <2>; >> + linux,axis = <0>; /* REL_X */ > > Probably this has been already discussed, but shouldn't keyboard and > rotary encoder be represented as sub-nodes here, similar to how GPIO > block is represented? > > Thanks. Yes, this has been discussed on v1 and I was asked to remove most of the subnodes. https://lore.kernel.org/lkml/58c80c2a-2532-4bc5-9c9f-52480b3af52a@kernel.org/ Thanks for your review.
On Wed Apr 9, 2025 at 6:32 PM CEST, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 05:03:02PM +0200, Mathieu Dubois-Briand wrote: >> On Wed Apr 9, 2025 at 4:55 PM CEST, Mathieu Dubois-Briand wrote: >> > Add driver for Maxim Integrated MAX7360 pinctrl on the PORT pins. Pins >> > can be used either for GPIO, PWM or rotary encoder functionalities. > > ... > > The all the rest of the driver LGTM, but the below. > >> > + device_set_of_node_from_dev(dev, dev->parent); >> >> Ok, so this goes a bit against what I said I was going to do on my >> previous series, let me explain why. Same reasoning applies for both >> uses, in PWM and pinctrl drivers. >> >> With my previous experiments, I came to the conclusion that: >> - Either we should use device_set_of_node_from_dev() as I do here. >> - Or we should add more subnodes in the device tree binding. > >> - Also, copying the fwnode with device_set_node() was not possible, as >> the kernel would then try to apply pinctrl on both the parent and >> child device. > > Hmm... I need to refresh my memory with the old discussions. Can you point out > to the problem statement with that approach? > I mentioned here briefly in my previous series: https://lore.kernel.org/lkml/D8R4B2PKIWSU.2LWTN50YP7SMX@bootlin.com/ So the issue is, if I copy the parent fwnode using device_set_node(), the kernel is trying to apply any pinctrl defined on the node with pinctrl- properties on both the parent and the child node. Of course, only the first one will succeed, as two devices cannot request the same pins at the same time. >> I previously said the second solution was probably the way to go, but I >> changed my mind for two reasons. >> >> First having more subnodes in the device tree was already rejected in >> the past in the reviews of the dt-bindings patch. This do makes sense as >> it would be describing device internals (which should not be made in >> DT), just to ease one specific software implementation (which should >> also be avoided). So I believe this change would again be rejected. >> https://lore.kernel.org/lkml/58c80c2a-2532-4bc5-9c9f-52480b3af52a@kernel.org/ >> >> But the the second reason is, doing >> 'git grep "device_set_of_node_from_dev.*parent"', I found several >> drivers using device_set_of_node_from_dev() for a similar need. Some of >> these uses are also for MFD child devices: >> - gpio-adp5585.c / pwm-adp5585.c, >> - pwm-ntxec.c, >> - max77620-regulator.c / max77620_thermal.c. >> >> So, based on this, I believe using device_set_of_node_from_dev() in >> these two drivers is the way to go. > > The problem with this solution is that, It's OF-centric. Which shouldn't be > done in a new code (and I don't see impediments to avoid it). Yes, it does > the right thing for the case, but only on OF systems. Note, fwnode is a list > of maximum of two entries (yeah, designed like that right now), can you utilise > that somehow? Looking at MFD code, I believe ACPI MFD child devices already get the parent fwnode, except if a fwnode exists for them. https://elixir.bootlin.com/linux/v6.13.7/source/drivers/mfd/mfd-core.c#L90 Thanks for your review.
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
On Wed Apr 9, 2025 at 9:17 PM CEST, Dmitry Torokhov wrote: > Hi Mathieu, > > On Wed, Apr 09, 2025 at 04:55:58PM +0200, Mathieu Dubois-Briand wrote: >> Add driver for Maxim Integrated MAX7360 rotary encoder controller, >> supporting a single rotary switch. > > Largely same comments as for the keypad driver: use "int error" for erro > variable, selection of the device for logging. Also: > OK >> + >> + input = devm_input_allocate_device(dev); >> + if (!input) >> + return -ENOMEM; >> + >> + max7360_rotary->input = input; >> + >> + input->id.bustype = BUS_I2C; >> + input->name = pdev->name; >> + input->dev.parent = dev; > > No need to be setting/overriding this, devm_input_allocate_device() > already sets this up. > Ok, thanks! >> + >> + input_set_capability(input, EV_REL, max7360_rotary->axis); > > The event type should come from the DT data I believe. Could we use at > least parts of the regular rotary encoding bindings? Ok, I should be able to add "rotary-encoder,relative-axis" property, as for rotary_encoder.c. > > Thanks. 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,