| Message ID | 20241219-mdb-max7360-support-v1-0-8e8317584121@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for MAX7360 multifunction device | expand |
On 19/12/2024 17:21, Mathieu Dubois-Briand wrote: > Add device tree bindings for Maxim Integrated MAX7360 MFD device with > support for keypad, rotary, gpios and pwm functionalities. Please use subject prefixes matching the subsystem. You can get them for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching. For bindings, the preferred subjects are explained here: https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters Subject/commit msg: drop MFD and explain what the hardware is. MFD is Linuxism. > > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > --- > Documentation/devicetree/bindings/mfd/max7360.yaml | 56 ++++++++++++++++++++++ Use compatible as filename. > 1 file changed, 56 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/max7360.yaml b/Documentation/devicetree/bindings/mfd/max7360.yaml > new file mode 100644 > index 000000000000..49dd437fd313 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/max7360.yaml > @@ -0,0 +1,56 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/max7360.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Maxim MAX7360 Keypad, Rotary encoder, PWM and GPIO controller > + > +maintainers: > + - Kamel Bouhara <kamel.bouhara@bootlin.com> > + - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > + > +description: | > + Maxim MAX7360 MFD device, with following functions: > + - keypad controller > + - rotary controller > + - GPIO and GPO controller > + - PWM controller > + > + https://www.analog.com/en/products/max7360.html > + > +properties: > + compatible: > + enum: > + - maxim,max7360 > + > + reg: > + maxItems: 1 > + > + interrupts: > + description: The interrupt line the device is connected to. Drop description, > + maxItems: 1 I don't think this was tested at all. It is heavily incomplete, considering this is sort of MFD device. Or you split patches in odd way. Look how other PMIC-style things are upstreamed. > + > +required: > + - compatible > + - reg > + - interrupts > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + max7360@38 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "maxim,max7360"; > + reg = <0x38>; > + Best regards, Krzysztof
On 19/12/2024 17:21, 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. > ... > + > +#ifdef CONFIG_OF > +static const struct of_device_id max7360_pwm_of_match[] = { > + { .compatible = "maxim,max7360-pwm", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, max7360_pwm_of_match); > +#endif > + > +static struct platform_driver max7360_pwm_driver = { > + .driver = { > + .name = "max7360-pwm", > + .of_match_table = of_match_ptr(max7360_pwm_of_match), Better to drop of_match_ptr and #ifdef earlier, so ACPI could use it. > + }, > + .probe = max7360_pwm_probe, > +}; > +module_platform_driver(max7360_pwm_driver); > + > +MODULE_DESCRIPTION("MAX7360 PWM driver"); > +MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>"); > +MODULE_ALIAS("platform:max7360-pwm"); You should not need MODULE_ALIAS() in normal cases. If you need it, usually it means your device ID table is wrong (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute for incomplete ID table. Maybe you need it because OF table will get dropped, then it would be fine. But in current code it's unnecessary and copy-pasta from other drivers. Best regards, Krzysztof
On Thu Dec 19, 2024 at 10:53 PM CET, Uwe Kleine-König wrote: > Hello, > > On Thu, Dec 19, 2024 at 05:21:21PM +0100, mathieu.dubois-briand@bootlin.com wrote: > > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > > > + int ret; > > + > > + if (state->polarity != PWM_POLARITY_NORMAL) > > + return -EINVAL; > > + > > + max7360_pwm = to_max7360_pwm(chip); > > + ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL, > > + MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm), > > + MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm)); > > + if (ret) { > > + dev_err(&chip->dev, "failed to enable pwm-%d , error %d\n", > > + pwm->hwpwm, ret); > > + return ret; > > + } > > + > > + do_div(duty_steps, MAX7360_PWM_PERIOD_NS); > > + > > + ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWMBASE + pwm->hwpwm, > > + duty_steps >= 255 ? 255 : duty_steps); > > + if (ret) { > > + dev_err(&chip->dev, > > + "failed to apply pwm duty_cycle %llu on pwm-%d, error %d\n", > > + duty_steps, pwm->hwpwm, ret); > > + return ret; > > + } > > Huh, state->period isn't used at all. That is wrong for sure. > Yes this was definitely missing. Period is fixed by the chip, so I will make sure the requested one is valid or return -EINVAL. Thanks a lot for your review, I am preparing a new version of this series that should address all your comments.
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> --- Kamel Bouhara (2): mfd: Add max7360 support pwm: max7360: Add MAX7360 PWM support Mathieu Dubois-Briand (6): dt-bindings: Add MAX7360 MFD device dt-bindings: Add MAX7360 subdevices 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 .../devicetree/bindings/gpio/max7360-gpio.yaml | 96 +++++ .../devicetree/bindings/input/max7360-keypad.yaml | 67 +++ .../devicetree/bindings/input/max7360-rotary.yaml | 52 +++ Documentation/devicetree/bindings/mfd/max7360.yaml | 56 +++ .../devicetree/bindings/pwm/max7360-pwm.yaml | 35 ++ MAINTAINERS | 12 + drivers/gpio/Kconfig | 11 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-max7360.c | 453 +++++++++++++++++++++ drivers/input/keyboard/Kconfig | 12 + drivers/input/keyboard/Makefile | 1 + drivers/input/keyboard/max7360-keypad.c | 297 ++++++++++++++ drivers/input/misc/Kconfig | 11 + drivers/input/misc/Makefile | 1 + drivers/input/misc/max7360-rotary.c | 194 +++++++++ drivers/mfd/Kconfig | 12 + drivers/mfd/Makefile | 1 + drivers/mfd/max7360.c | 302 ++++++++++++++ drivers/pwm/Kconfig | 11 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-max7360.c | 173 ++++++++ include/linux/mfd/max7360.h | 103 +++++ 22 files changed, 1902 insertions(+) --- base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8 change-id: 20241219-mdb-max7360-support-223a8ce45ba3 Best regards,