[PATCH v4 00/10] Add support for MAX7360

[PATCH v4 00/10] Add support for MAX7360

[Mathieu Dubois-Briand]

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,
-- 
Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>

[PATCH v4 01/10] dt-bindings: mfd: gpio: Add MAX7360

[Mathieu Dubois-Briand]

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          |  91 ++++++++++++++
 .../devicetree/bindings/mfd/maxim,max7360.yaml     | 139 +++++++++++++++++++++
 2 files changed, 230 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..bdaed7917051
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml
@@ -0,0 +1,91 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/maxim,max7360-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+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://www.analog.com/en/products/max7360.html
+
+  The device provide two series of GPIOs, referred here as GPIOs and GPOs.
+
+  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
+  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. This partioning must be described
+  here using the ngpios property.
+
+properties:
+  compatible:
+    enum:
+      - maxim,max7360-gpio
+      - maxim,max7360-gpo
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  ngpios:
+    minimum: 0
+    maximum: 6
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  maxim,constant-current-disable:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Bit field, each bit disables constant-current output of the associated
+      GPIO, starting from the least significant bit for the first GPIO.
+    maximum: 0xff
+
+required:
+  - compatible
+  - gpio-controller
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max7360-gpio
+        ngpios: false
+    then:
+      required:
+        - interrupt-controller
+    else:
+      properties:
+        interrupt-controller: false
+        maxim,constant-current-disable: false
+
+      required:
+        - ngpios
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio {
+      compatible = "maxim,max7360-gpio";
+
+      gpio-controller;
+      #gpio-cells = <2>;
+      maxim,constant-current-disable = <0x06>;
+
+      interrupt-controller;
+      #interrupt-cells = <2>;
+    };
diff --git a/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml b/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
new file mode 100644
index 000000000000..4b93a3dd8ae4
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/maxim,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 device, with following functions:
+    - keypad controller
+    - rotary controller
+    - GPIO and GPO controller
+    - PWM controller
+
+  https://www.analog.com/en/products/max7360.html
+
+allOf:
+  - $ref: /schemas/input/matrix-keymap.yaml#
+  - $ref: /schemas/input/input.yaml#
+
+properties:
+  compatible:
+    enum:
+      - maxim,max7360
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 2
+
+  interrupt-names:
+    items:
+      - const: inti
+      - const: intk
+
+  keypad-debounce-delay-ms:
+    description: Keypad debounce delay in ms
+    minimum: 9
+    maximum: 40
+    default: 9
+
+  rotary-debounce-delay-ms:
+    description: Rotary encoder debounce delay in ms
+    minimum: 0
+    maximum: 15
+    default: 0
+
+  linux,axis:
+    description: The input subsystem axis to map to this rotary encoder.
+
+  "#pwm-cells":
+    const: 3
+
+  gpio:
+    $ref: /schemas/gpio/maxim,max7360-gpio.yaml#
+    description:
+      PORT0 to PORT7 general purpose input/output pins configuration.
+
+  gpo:
+    $ref: /schemas/gpio/maxim,max7360-gpio.yaml#
+    description:
+      COL2 to COL7 general purpose output pins configuration.
+      Allows to use unused keypad columns as outputs.
+      The MAX7360 has 8 column lines and 6 of them can be used as GPOs. Value
+      of ngpios must be coherent with the value of keypad,num-columns, as their
+      sum must not exceed the number of physical lines.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - linux,keymap
+  - linux,axis
+  - "#pwm-cells"
+  - gpio
+  - gpo
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/input/input.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    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 */
+
+        #pwm-cells = <3>;
+
+        max7360_gpio: gpio {
+          compatible = "maxim,max7360-gpio";
+
+          gpio-controller;
+          #gpio-cells = <2>;
+          maxim,constant-current-disable = <0x06>;
+
+          interrupt-controller;
+          #interrupt-cells = <0x2>;
+        };
+
+        max7360_gpo: gpo {
+          compatible = "maxim,max7360-gpo";
+
+          gpio-controller;
+          #gpio-cells = <2>;
+          ngpios = <4>;
+        };
+      };
+    };

-- 
2.39.5

Re: [PATCH v4 01/10] dt-bindings: mfd: gpio: Add MAX7360

[Krzysztof Kozlowski]

On Fri, Feb 14, 2025 at 12:49:51PM +0100, 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          |  91 ++++++++++++++
>  .../devicetree/bindings/mfd/maxim,max7360.yaml     | 139 +++++++++++++++++++++
>  2 files changed, 230 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 01/10] dt-bindings: mfd: gpio: Add MAX7360

[Mathieu Dubois-Briand]

On Sun Feb 16, 2025 at 1:58 PM CET, Krzysztof Kozlowski wrote:
> On Fri, Feb 14, 2025 at 12:49:51PM +0100, 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          |  91 ++++++++++++++
> >  .../devicetree/bindings/mfd/maxim,max7360.yaml     | 139 +++++++++++++++++++++
> >  2 files changed, 230 insertions(+)
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> Best regards,
> Krzysztof

Thanks for the tag!

I've chosen not to include it so far in my newest series, as I have a
few changes that do impact device tree bindings:
- The ngpios property was removed from the GPIO bindings.
- Bindings for the pinctrl support were added.

New version is available here:
https://lore.kernel.org/all/20250318-mdb-max7360-support-v5-1-fb20baf97da0@bootlin.com/

Best regards,
Mathieu

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v4 02/10] mfd: Add max7360 support

[mathieu.dubois-briand]

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       | 218 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/max7360.h | 112 +++++++++++++++++++++++
 4 files changed, 345 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6b0682af6e32..ef02a1c4322c 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2439,5 +2439,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 9220eaf7cf12..db2bd232c150 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -163,6 +163,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..04ff8f5c811b
--- /dev/null
+++ b/drivers/mfd/max7360.c
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Maxim MAX7360 Core Driver
+ *
+ * Copyright (C) 2024 Kamel Bouhara
+ * Author: Kamel Bouhara <kamel.bouhara@bootlin.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/max7360.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+static DEFINE_SPINLOCK(request_lock);
+
+struct max7360 {
+	struct device *dev;
+	struct regmap *regmap;
+	unsigned int requested_ports;
+};
+
+static const struct mfd_cell max7360_cells[] = {
+	{
+		.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 = 0x48,
+		.range_max = 0x4a,
+	},
+};
+
+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 = 0xff,
+	.volatile_table = &max7360_volatile_table,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request)
+{
+	struct i2c_client *client;
+	struct max7360 *max7360;
+	unsigned long flags;
+	int ret = 0;
+
+	client = to_i2c_client(dev);
+	max7360 = i2c_get_clientdata(client);
+
+	spin_lock_irqsave(&request_lock, flags);
+	if (request) {
+		if (max7360->requested_ports & BIT(pin))
+			ret = -EBUSY;
+		else
+			max7360->requested_ports |= BIT(pin);
+	} else {
+		max7360->requested_ports &= ~BIT(pin);
+	}
+	spin_unlock_irqrestore(&request_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(max7360_port_pin_request);
+
+static int max7360_mask_irqs(struct max7360 *max7360)
+{
+	unsigned int val;
+	int ret;
+
+	/*
+	 * GPIO/PWM interrupts are not masked on reset: mask the during probe,
+	 * avoiding repeated spurious interrupts if the corresponding drivers
+	 * are not present.
+	 */
+	for (int i = 0; i < MAX7360_PORT_PWM_COUNT; i++) {
+		ret = regmap_write_bits(max7360->regmap, MAX7360_REG_PWMCFG(i),
+					MAX7360_PORT_CFG_INTERRUPT_MASK,
+					MAX7360_PORT_CFG_INTERRUPT_MASK);
+		if (ret) {
+			dev_err(max7360->dev, "Failed to write max7360 port configuration");
+			return ret;
+		}
+	}
+
+	/* Read GPIO in register, to ACK any pending IRQ. */
+	ret = regmap_read(max7360->regmap, MAX7360_REG_GPIOIN, &val);
+	if (ret) {
+		dev_err(max7360->dev, "Failed to read gpio values: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max7360_reset(struct max7360 *max7360)
+{
+	int err;
+
+	err = regmap_write(max7360->regmap, MAX7360_REG_GPIOCFG,
+			   MAX7360_GPIO_CFG_GPIO_RST);
+	if (err) {
+		dev_err(max7360->dev, "Failed to reset GPIO configuration: %x\n", err);
+		return err;
+	}
+
+	err = regcache_drop_region(max7360->regmap, MAX7360_REG_GPIOCFG,
+				   MAX7360_REG_GPIO_LAST);
+	if (err) {
+		dev_err(max7360->dev, "Failed to drop regmap cache: %x\n", err);
+		return err;
+	}
+
+	err = regmap_write(max7360->regmap, MAX7360_REG_SLEEP, 0);
+	if (err) {
+		dev_err(max7360->dev, "Failed to reset autosleep configuration: %x\n", err);
+		return err;
+	}
+
+	err = regmap_write(max7360->regmap, MAX7360_REG_DEBOUNCE, 0);
+	if (err) {
+		dev_err(max7360->dev, "Failed to reset GPO port count: %x\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int max7360_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct regmap *regmap;
+	struct max7360 *max7360;
+	int err;
+
+	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");
+
+	max7360 = devm_kzalloc(dev, sizeof(*max7360), GFP_KERNEL);
+	if (!max7360)
+		return -ENOMEM;
+
+	max7360->regmap = regmap;
+	max7360->dev = dev;
+	i2c_set_clientdata(client, max7360);
+
+	err = max7360_reset(max7360);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to reset device\n");
+
+	/* Get the device out of shutdown mode. */
+	err = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG,
+				MAX7360_GPIO_CFG_GPIO_EN,
+				MAX7360_GPIO_CFG_GPIO_EN);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to enable GPIO and PWM module\n");
+
+	err = max7360_mask_irqs(max7360);
+	if (err)
+		return dev_err_probe(dev, err, "Could not mask interrupts\n");
+
+	err =  devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+				    max7360_cells, ARRAY_SIZE(max7360_cells),
+				    NULL, 0, NULL);
+	if (err)
+		return dev_err_probe(dev, err, "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..6beff7405d10
--- /dev/null
+++ b/include/linux/mfd/max7360.h
@@ -0,0 +1,112 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __LINUX_MFD_MAX7360_H
+#define __LINUX_MFD_MAX7360_H
+
+#include <linux/bitfield.h>
+#include <linux/device.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_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_REG_GPIOCURR_FIXED 0xC0
+
+/*
+ * Autosleep register values (ms)
+ */
+#define MAX7360_AUTOSLEEP_8192	0x01
+#define MAX7360_AUTOSLEEP_4096	0x02
+#define MAX7360_AUTOSLEEP_2048	0x03
+#define MAX7360_AUTOSLEEP_1024	0x04
+#define MAX7360_AUTOSLEEP_512	0x05
+#define MAX7360_AUTOSLEEP_256	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
+
+int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request);
+
+#endif

-- 
2.39.5

Re: [PATCH v4 02/10] mfd: Add max7360 support

[Andy Shevchenko]

On Fri, Feb 14, 2025 at 12:49:52PM +0100, 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.

GPIO, PWM, GPO

...

> +/*
> + * Maxim MAX7360 Core Driver
> + *
> + * Copyright (C) 2024 Kamel Bouhara

Shouldn't you add yours / switch to 2025?

> + * Author: Kamel Bouhara <kamel.bouhara@bootlin.com>
> + */

...

+ bits.h

> +#include <linux/delay.h>

+ device.h // devm_kzalloc(), etc.

> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/module.h>

+ mod_devicetable.h // OF device ID table and so on...

> +#include <linux/of.h>

Is this being used? How?

> +#include <linux/regmap.h>

+ types.h // bool and so on...

...

> +struct max7360 {
> +	struct device *dev;

Same as dev in regmap?

> +	struct regmap *regmap;
> +	unsigned int requested_ports;
> +};

...

> +	}, {
> +		.range_min = 0x48,
> +		.range_max = 0x4a,

Hmm... No self-explanatory names for these addresses?

> +	},

> +static const struct regmap_config max7360_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,

> +	.max_register = 0xff,

Is it for real? I mean does the chip have 256 defined registers?

> +	.volatile_table = &max7360_volatile_table,
> +	.cache_type = REGCACHE_MAPLE,
> +};

...

> +int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request)
> +{
> +	struct i2c_client *client;
> +	struct max7360 *max7360;

> +	unsigned long flags;
> +	int ret = 0;

Unneeded (see below)

> +	client = to_i2c_client(dev);
> +	max7360 = i2c_get_clientdata(client);

What's wrong with dev_get_drvdata()?
And this can be done in the block definition above.

> +	spin_lock_irqsave(&request_lock, flags);

Consider use guard()() / scoped_guard(). This will make ret and flags unneeded
besides better and robust code. You will need to include cleanup.h.

> +	if (request) {
> +		if (max7360->requested_ports & BIT(pin))
> +			ret = -EBUSY;
> +		else
> +			max7360->requested_ports |= BIT(pin);
> +	} else {
> +		max7360->requested_ports &= ~BIT(pin);
> +	}
> +	spin_unlock_irqrestore(&request_lock, flags);
> +
> +	return ret;
> +}

...

> +EXPORT_SYMBOL_GPL(max7360_port_pin_request);

No namespace?

...

> +	for (int i = 0; i < MAX7360_PORT_PWM_COUNT; i++) {

Why signed?

> +		ret = regmap_write_bits(max7360->regmap, MAX7360_REG_PWMCFG(i),
> +					MAX7360_PORT_CFG_INTERRUPT_MASK,
> +					MAX7360_PORT_CFG_INTERRUPT_MASK);
> +		if (ret) {
> +			dev_err(max7360->dev, "Failed to write max7360 port configuration");
> +			return ret;
> +		}
> +	}

...

> +	/* Read GPIO in register, to ACK any pending IRQ. */
> +	ret = regmap_read(max7360->regmap, MAX7360_REG_GPIOIN, &val);
> +	if (ret) {
> +		dev_err(max7360->dev, "Failed to read gpio values: %d\n", ret);

> +		return ret;
> +	}
> +
> +	return 0;

	return ret;

instead of 4 LoCs.

> +}

...

> +static int max7360_reset(struct max7360 *max7360)
> +{
> +	int err;

Please, be consistent with the variable naming with the same semantics.

> +	err = regmap_write(max7360->regmap, MAX7360_REG_GPIOCFG,
> +			   MAX7360_GPIO_CFG_GPIO_RST);
> +	if (err) {
> +		dev_err(max7360->dev, "Failed to reset GPIO configuration: %x\n", err);
> +		return err;
> +	}
> +
> +	err = regcache_drop_region(max7360->regmap, MAX7360_REG_GPIOCFG,
> +				   MAX7360_REG_GPIO_LAST);
> +	if (err) {
> +		dev_err(max7360->dev, "Failed to drop regmap cache: %x\n", err);
> +		return err;
> +	}
> +
> +	err = regmap_write(max7360->regmap, MAX7360_REG_SLEEP, 0);
> +	if (err) {
> +		dev_err(max7360->dev, "Failed to reset autosleep configuration: %x\n", err);
> +		return err;
> +	}
> +
> +	err = regmap_write(max7360->regmap, MAX7360_REG_DEBOUNCE, 0);
> +	if (err) {
> +		dev_err(max7360->dev, "Failed to reset GPO port count: %x\n", err);

> +		return err;
> +	}
> +
> +	return 0;

	return ret;

> +}

...

> +static int max7360_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct regmap *regmap;
> +	struct max7360 *max7360;

> +	int err;

A bit of consistency, please.

> +}

...

> +static const struct of_device_id max7360_dt_match[] = {
> +	{ .compatible = "maxim,max7360" },
> +	{},

No comma for the terminator entry.

> +};

...

> +#ifndef __LINUX_MFD_MAX7360_H
> +#define __LINUX_MFD_MAX7360_H

> +#include <linux/bitfield.h>
> +#include <linux/device.h>

None of these are in use AFACS. Missing

bits.h
types.h

struct device;

> +#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_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_REG_GPIOCURR_FIXED 0xC0
> +
> +/*
> + * Autosleep register values (ms)

Instead of this ' (ms)' part, add a unit suffix, or where do the units apply?
To the number in the name? If so, extend comment to explain this.

> + */
> +#define MAX7360_AUTOSLEEP_8192	0x01
> +#define MAX7360_AUTOSLEEP_4096	0x02
> +#define MAX7360_AUTOSLEEP_2048	0x03
> +#define MAX7360_AUTOSLEEP_1024	0x04
> +#define MAX7360_AUTOSLEEP_512	0x05
> +#define MAX7360_AUTOSLEEP_256	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
> +
> +int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request);
> +
> +#endif

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 03/10] pwm: max7360: Add MAX7360 PWM support

[mathieu.dubois-briand]

From: Kamel Bouhara <kamel.bouhara@bootlin.com>

Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to
8 independent PWM outputs.

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/pwm/Kconfig       |  10 +++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-max7360.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0915c1e7df16..fac811ee46b5 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -745,4 +745,14 @@ config PWM_XILINX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-xilinx.
 
+config PWM_MAX7360
+	tristate "MAX7360 PWMs"
+	depends on MFD_MAX7360
+	help
+	  PWM driver for Maxim Integrated MAX7360 multifunction device, with
+	  support for up to 8 PWM outputs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-max7360.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9081e0c0e9e0..ae8908ffc892 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
 obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
 obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
+obj-$(CONFIG_PWM_MAX7360)	+= pwm-max7360.o
 obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
 obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
 obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
new file mode 100644
index 000000000000..f1257c20add2
--- /dev/null
+++ b/drivers/pwm/pwm-max7360.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Bootlin
+ *
+ * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com>
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ *
+ * Limitations:
+ * - Only supports normal polarity.
+ * - The period is fixed to 2 ms.
+ * - Only the duty cycle can be changed, new values are applied at the beginning
+ *   of the next cycle.
+ * - When disabled, the output is put in Hi-Z.
+ */
+#include <linux/err.h>
+#include <linux/math.h>
+#include <linux/mfd/max7360.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define MAX7360_NUM_PWMS			8
+#define MAX7360_PWM_MAX_RES			255
+#define MAX7360_PWM_PERIOD_NS			2000000 /* 500 Hz */
+#define MAX7360_PWM_COMMON_PWN			BIT(5)
+#define MAX7360_PWM_CTRL_ENABLE(n)		BIT(n)
+#define MAX7360_PWM_PORT(n)			BIT(n)
+
+struct max7360_pwm {
+	struct device *parent;
+	struct regmap *regmap;
+};
+
+struct max7360_pwm_waveform {
+	u8 duty_steps;
+};
+
+static inline struct max7360_pwm *max7360_pwm_from_chip(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct max7360_pwm *max7360_pwm;
+	int ret;
+
+	max7360_pwm = max7360_pwm_from_chip(chip);
+	ret = max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm, true);
+	if (ret) {
+		dev_warn(&chip->dev, "failed to request pwm-%d\n", pwm->hwpwm);
+		return ret;
+	}
+
+	ret = regmap_write_bits(max7360_pwm->regmap,
+				MAX7360_REG_PWMCFG(pwm->hwpwm),
+				MAX7360_PWM_COMMON_PWN,
+				0);
+	if (ret)
+		return ret;
+
+	return regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_PORTS,
+				 MAX7360_PWM_PORT(pwm->hwpwm),
+				 MAX7360_PWM_PORT(pwm->hwpwm));
+}
+
+static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct max7360_pwm *max7360_pwm;
+
+	max7360_pwm = max7360_pwm_from_chip(chip);
+	max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm, false);
+}
+
+static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip,
+					   struct pwm_device *pwm,
+					   const struct pwm_waveform *wf,
+					   void *_wfhw)
+{
+	struct max7360_pwm_waveform *wfhw = _wfhw;
+	u64 duty_steps;
+
+	/*
+	 * Ignore user provided values for period_length_ns and duty_offset_ns:
+	 * we only support fixed period of MAX7360_PWM_PERIOD_NS and offset of
+	 * 0.
+	 */
+
+	duty_steps = mul_u64_u64_div_u64(wf->duty_length_ns, MAX7360_PWM_MAX_RES,
+					 MAX7360_PWM_PERIOD_NS);
+
+	wfhw->duty_steps = min(MAX7360_PWM_MAX_RES, duty_steps);
+
+	return 0;
+}
+
+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 = MAX7360_PWM_PERIOD_NS;
+	wf->duty_offset_ns = 0;
+	wf->duty_length_ns = DIV64_U64_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS,
+						MAX7360_PWM_MAX_RES);
+
+	return 0;
+}
+
+static int max7360_pwm_write_waveform(struct pwm_chip *chip,
+				      struct pwm_device *pwm,
+				      const void *_wfhw)
+{
+	const struct max7360_pwm_waveform *wfhw = _wfhw;
+	struct max7360_pwm *max7360_pwm;
+	unsigned int val;
+	int ret;
+
+	max7360_pwm = max7360_pwm_from_chip(chip);
+
+	val = (wfhw->duty_steps == 0) ? 0 : MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm);
+	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
+				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm), val);
+
+	if (!ret && wfhw->duty_steps != 0) {
+		ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWM(pwm->hwpwm),
+				   wfhw->duty_steps);
+	}
+
+	return ret;
+}
+
+static int max7360_pwm_read_waveform(struct pwm_chip *chip,
+				     struct pwm_device *pwm,
+				     void *_wfhw)
+{
+	struct max7360_pwm_waveform *wfhw = _wfhw;
+	struct max7360_pwm *max7360_pwm;
+	unsigned int val;
+	int ret;
+
+	max7360_pwm = max7360_pwm_from_chip(chip);
+
+	ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL, &val);
+	if (ret)
+		return ret;
+
+	if (val & MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm)) {
+		ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_PWM(pwm->hwpwm),
+				  &val);
+		val = wfhw->duty_steps;
+	} else {
+		wfhw->duty_steps = 0;
+	}
+
+	return ret;
+}
+
+static const struct pwm_ops max7360_pwm_ops = {
+	.request = max7360_pwm_request,
+	.free = max7360_pwm_free,
+	.round_waveform_tohw = max7360_pwm_round_waveform_tohw,
+	.round_waveform_fromhw = max7360_pwm_round_waveform_fromhw,
+	.read_waveform = max7360_pwm_read_waveform,
+	.write_waveform = max7360_pwm_write_waveform,
+};
+
+static int max7360_pwm_probe(struct platform_device *pdev)
+{
+	struct max7360_pwm *max7360_pwm;
+	struct pwm_chip *chip;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return dev_err_probe(&pdev->dev, -ENODEV, "no parent device\n");
+
+	chip = devm_pwmchip_alloc(pdev->dev.parent, MAX7360_NUM_PWMS,
+				  sizeof(*max7360_pwm));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	chip->ops = &max7360_pwm_ops;
+
+	max7360_pwm = max7360_pwm_from_chip(chip);
+	max7360_pwm->parent = pdev->dev.parent;
+
+	max7360_pwm->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!max7360_pwm->regmap)
+		return dev_err_probe(&pdev->dev, -ENODEV,
+				     "could not get parent regmap\n");
+
+	ret = devm_pwmchip_add(&pdev->dev, chip);
+	if (ret != 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to add PWM chip\n");
+
+	return 0;
+}
+
+static struct platform_driver max7360_pwm_driver = {
+	.driver = {
+		.name = "max7360-pwm",
+	},
+	.probe = max7360_pwm_probe,
+};
+module_platform_driver(max7360_pwm_driver);
+
+MODULE_DESCRIPTION("MAX7360 PWM driver");
+MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>");
+MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH v4 03/10] pwm: max7360: Add MAX7360 PWM support

[Andy Shevchenko]

On Fri, Feb 14, 2025 at 12:49:53PM +0100, 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.

...

+ bits.h

+ dev_printk.h

> +#include <linux/err.h>


> +#include <linux/math.h>

Other way around, id est you need math64.h (see below).

> +#include <linux/mfd/max7360.h>

+ minmax.h

> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>

> +#include <linux/of.h>

Is this used? Cargo cult?

> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>

+ types.h

...

> +#define MAX7360_PWM_PERIOD_NS			2000000 /* 500 Hz */

Comment is superfluous, if you need HZ units, define the respective one.
Also you can use something like (2 * NSEC_PER_MSEC) which will immediately
gives a hint of how long this is and reduces potential 0:s miscalculations.
This will need time.h

...

> +#define MAX7360_PWM_CTRL_ENABLE(n)		BIT(n)
> +#define MAX7360_PWM_PORT(n)			BIT(n)

Personally I find these macros overkill. The value of them much shorter and
equally readable.

...

> +struct max7360_pwm {

> +	struct device *parent;

Is it not the same as you can derive from regmap?

> +	struct regmap *regmap;

Btw, have you checked the code generation if you place regmap the first in the
structure? It might affect it.

> +};

...

> +	/*
> +	 * Ignore user provided values for period_length_ns and duty_offset_ns:
> +	 * we only support fixed period of MAX7360_PWM_PERIOD_NS and offset of

> +	 * 0.

Easy to read with 0 be on previous line.

> +	 */

> +

No need for this blank line.

> +	duty_steps = mul_u64_u64_div_u64(wf->duty_length_ns, MAX7360_PWM_MAX_RES,
> +					 MAX7360_PWM_PERIOD_NS);

This comes from math64.h

> +
> +	wfhw->duty_steps = min(MAX7360_PWM_MAX_RES, duty_steps);

...

> +static int max7360_pwm_write_waveform(struct pwm_chip *chip,
> +				      struct pwm_device *pwm,
> +				      const void *_wfhw)
> +{
> +	const struct max7360_pwm_waveform *wfhw = _wfhw;
> +	struct max7360_pwm *max7360_pwm;
> +	unsigned int val;
> +	int ret;
> +
> +	max7360_pwm = max7360_pwm_from_chip(chip);
> +
> +	val = (wfhw->duty_steps == 0) ? 0 : MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm);
> +	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
> +				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm), val);

> +
> +	if (!ret && wfhw->duty_steps != 0) {
> +		ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWM(pwm->hwpwm),
> +				   wfhw->duty_steps);
> +	}
> +
> +	return ret;

Please, improve readability by rewriting like this:

	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm), val);
	if (ret)
		return ret;

	if (wfhw->duty_steps)
		return regmap_write(max7360_pwm->regmap, MAX7360_REG_PWM(pwm->hwpwm),
				    wfhw->duty_steps);

	return 0;

> +}

...

> +static int max7360_pwm_probe(struct platform_device *pdev)
> +{

With

	struct device *dev = &pdev->dev;

all below will look shorter and nicer.

> +	struct max7360_pwm *max7360_pwm;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	if (!pdev->dev.parent)
> +		return dev_err_probe(&pdev->dev, -ENODEV, "no parent device\n");
> +
> +	chip = devm_pwmchip_alloc(pdev->dev.parent, MAX7360_NUM_PWMS,
> +				  sizeof(*max7360_pwm));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +	chip->ops = &max7360_pwm_ops;
> +
> +	max7360_pwm = max7360_pwm_from_chip(chip);
> +	max7360_pwm->parent = pdev->dev.parent;
> +
> +	max7360_pwm->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!max7360_pwm->regmap)
> +		return dev_err_probe(&pdev->dev, -ENODEV,
> +				     "could not get parent regmap\n");

Will become one line (with the above suggestion).

> +	ret = devm_pwmchip_add(&pdev->dev, chip);

> +	if (ret != 0)

Please, be consistent with the style, and moreover this style is unusual.

> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to add PWM chip\n");
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 03/10] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Fri Feb 14, 2025 at 4:10 PM CET, Andy Shevchenko wrote:
> On Fri, Feb 14, 2025 at 12:49:53PM +0100, 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.
>
> ...
>
> > +#include <linux/of.h>
>
> Is this used? Cargo cult?
>

Right, not used anymore.

>
> > +struct max7360_pwm {
>
> > +	struct device *parent;
>
> Is it not the same as you can derive from regmap?
>

It is. I'm removing it.

Also, max7360_pwm structure will only contain the regmap, so I will
remove the structure and set the regmap directly as the pwm chip driver
data.

> ...
>

I have fixed all other points. Thanks for your review!


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 03/10] pwm: max7360: Add MAX7360 PWM support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4447 bytes --]

Hello Mathieu,

On Fri, Feb 14, 2025 at 12:49:53PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
> new file mode 100644
> index 000000000000..f1257c20add2
> --- /dev/null
> +++ b/drivers/pwm/pwm-max7360.c
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Bootlin
> + *
> + * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com>
> + * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> + *
> + * Limitations:
> + * - Only supports normal polarity.
> + * - The period is fixed to 2 ms.
> + * - Only the duty cycle can be changed, new values are applied at the beginning
> + *   of the next cycle.
> + * - When disabled, the output is put in Hi-Z.
> + */
> +#include <linux/err.h>
> +#include <linux/math.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define MAX7360_NUM_PWMS			8
> +#define MAX7360_PWM_MAX_RES			255
> +#define MAX7360_PWM_PERIOD_NS			2000000 /* 500 Hz */
> +#define MAX7360_PWM_COMMON_PWN			BIT(5)
> +#define MAX7360_PWM_CTRL_ENABLE(n)		BIT(n)
> +#define MAX7360_PWM_PORT(n)			BIT(n)
> +
> +struct max7360_pwm {
> +	struct device *parent;
> +	struct regmap *regmap;
> +};
> +
> +struct max7360_pwm_waveform {
> +	u8 duty_steps;
> +};
> +
> +static inline struct max7360_pwm *max7360_pwm_from_chip(struct pwm_chip *chip)
> +{
> +	return pwmchip_get_drvdata(chip);
> +}
> +
> +static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct max7360_pwm *max7360_pwm;
> +	int ret;
> +
> +	max7360_pwm = max7360_pwm_from_chip(chip);
> +	ret = max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm, true);
> +	if (ret) {
> +		dev_warn(&chip->dev, "failed to request pwm-%d\n", pwm->hwpwm);

Please drop this warning, just returning ret here is fine. (The rule of
thumb is: Emit runtime messages only in probe, not during usage.)

> +		return ret;
> +	}
> +
> +	ret = regmap_write_bits(max7360_pwm->regmap,
> +				MAX7360_REG_PWMCFG(pwm->hwpwm),
> +				MAX7360_PWM_COMMON_PWN,
> +				0);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_PORTS,
> +				 MAX7360_PWM_PORT(pwm->hwpwm),
> +				 MAX7360_PWM_PORT(pwm->hwpwm));
> +}
> +
> +static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct max7360_pwm *max7360_pwm;
> +
> +	max7360_pwm = max7360_pwm_from_chip(chip);
> +	max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm, false);

Would be nice if pinmuxing would be abstracted as a pinctrl driver. Not
sure how much effort that is. Maybe Linus has an idea?

> +}
> +
> [...]
> +
> +static int max7360_pwm_write_waveform(struct pwm_chip *chip,
> +				      struct pwm_device *pwm,
> +				      const void *_wfhw)
> +{
> +	const struct max7360_pwm_waveform *wfhw = _wfhw;
> +	struct max7360_pwm *max7360_pwm;
> +	unsigned int val;
> +	int ret;
> +
> +	max7360_pwm = max7360_pwm_from_chip(chip);
> +
> +	val = (wfhw->duty_steps == 0) ? 0 : MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm);

Does not setting MAX7360_PWM_CTRL_ENABLE result in the pin going to
Hi-Z? If yes: That's wrong. You're only supposed to do that if
period_length_ns = 0 was requested. If no: This needs a comment why
duty_steps = 0 is special here.

> +	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
> +				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm), val);
> +
> +	if (!ret && wfhw->duty_steps != 0) {
> +		ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWM(pwm->hwpwm),
> +				   wfhw->duty_steps);
> +	}
> +
> +	return ret;
> +}
> +
> +static int max7360_pwm_read_waveform(struct pwm_chip *chip,
> +				     struct pwm_device *pwm,
> +				     void *_wfhw)
> +{
> +	struct max7360_pwm_waveform *wfhw = _wfhw;
> +	struct max7360_pwm *max7360_pwm;
> +	unsigned int val;
> +	int ret;
> +
> +	max7360_pwm = max7360_pwm_from_chip(chip);
> +
> +	ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val & MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm)) {
> +		ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_PWM(pwm->hwpwm),
> +				  &val);
> +		val = wfhw->duty_steps;

wfhw->duty_steps = val;

> +	} else {
> +		wfhw->duty_steps = 0;
> +	}
> +
> +	return ret;
> +}

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 03/10] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Thu Mar 13, 2025 at 10:03 PM CET, Uwe Kleine-König wrote:
> Hello Mathieu,
>
> On Fri, Feb 14, 2025 at 12:49:53PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> > diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
> > new file mode 100644
> > index 000000000000..f1257c20add2
> ...
> > +
> > +static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct max7360_pwm *max7360_pwm;
> > +
> > +	max7360_pwm = max7360_pwm_from_chip(chip);
> > +	max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm, false);
>
> Would be nice if pinmuxing would be abstracted as a pinctrl driver. Not
> sure how much effort that is. Maybe Linus has an idea?
>

Yes, I got some similar comments previously and I've been working on it:
the next version will gain a pinctrl driver.

> > +}
> > +
> > [...]
> > +
> > +static int max7360_pwm_write_waveform(struct pwm_chip *chip,
> > +				      struct pwm_device *pwm,
> > +				      const void *_wfhw)
> > +{
> > +	const struct max7360_pwm_waveform *wfhw = _wfhw;
> > +	struct max7360_pwm *max7360_pwm;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	max7360_pwm = max7360_pwm_from_chip(chip);
> > +
> > +	val = (wfhw->duty_steps == 0) ? 0 : MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm);
>
> Does not setting MAX7360_PWM_CTRL_ENABLE result in the pin going to
> Hi-Z? If yes: That's wrong. You're only supposed to do that if
> period_length_ns = 0 was requested. If no: This needs a comment why
> duty_steps = 0 is special here.
>

Ok, I confirm this does set the pin in Hi-Z, I'm fixing it.

...

>
> Best regards
> Uwe

Thanks for your review.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v4 04/10] gpio: regmap: Allow to provide request and free callbacks

[Mathieu Dubois-Briand]

Allows to populate the gpio_regmap_config structure with request() and
free() callbacks to set on the final gpio_chip structure.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/gpio/gpio-regmap.c  | 2 ++
 include/linux/gpio/regmap.h | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 05f8781b5204..ba72c23bcf97 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -261,6 +261,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	chip->names = config->names;
 	chip->label = config->label ?: dev_name(config->parent);
 	chip->can_sleep = regmap_might_sleep(config->regmap);
+	chip->request = config->request;
+	chip->free = config->free;
 
 	chip->request = gpiochip_generic_request;
 	chip->free = gpiochip_generic_free;
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index a9f7b7faf57b..16f0c33df75d 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -6,6 +6,7 @@
 struct device;
 struct fwnode_handle;
 struct gpio_regmap;
+struct gpio_chip;
 struct irq_domain;
 struct regmap;
 
@@ -40,6 +41,10 @@ struct regmap;
  * @drvdata:		(Optional) Pointer to driver specific data which is
  *			not used by gpio-remap but is provided "as is" to the
  *			driver callback(s).
+ * @request:		(Optional) Hook for chip-specific activation, such as
+ *			enabling module power and clock
+ * @free:		(Optional) Hook for chip-specific deactivation, such as
+ *			disabling module power and clock
  *
  * The ->reg_mask_xlate translates a given base address and GPIO offset to
  * register and mask pair. The base address is one of the given register
@@ -83,6 +88,8 @@ struct gpio_regmap_config {
 			      unsigned int *mask);
 
 	void *drvdata;
+	int (*request)(struct gpio_chip *gc, unsigned int offset);
+	void (*free)(struct gpio_chip *gc, unsigned int offset);
 };
 
 struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config);

-- 
2.39.5

Re: [PATCH v4 04/10] gpio: regmap: Allow to provide request and free callbacks

[Andy Shevchenko]

On Fri, Feb 14, 2025 at 12:49:54PM +0100, Mathieu Dubois-Briand wrote:
> Allows to populate the gpio_regmap_config structure with request() and
> free() callbacks to set on the final gpio_chip structure.

Yeah, I understand the intention, but I have mixed feelings about this.
OOH it helps with the cases like yours, OTO it sounds like a hack instead
of proper implementation of the pin muxing. Yes, I know there are some
drivers in the kernel do similar things, but it most likely historically
and not always having the same HW capabilities as this chip.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 04/10] gpio: regmap: Allow to provide request and free callbacks

[Sander Vanheule]

Hi Mathieu,

On Fri, 2025-02-14 at 12:49 +0100, Mathieu Dubois-Briand wrote:
> Allows to populate the gpio_regmap_config structure with request() and
> free() callbacks to set on the final gpio_chip structure.
> 
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> ---
>  drivers/gpio/gpio-regmap.c  | 2 ++
>  include/linux/gpio/regmap.h | 7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index 05f8781b5204..ba72c23bcf97 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -261,6 +261,8 @@ struct gpio_regmap *gpio_regmap_register(const struct
> gpio_regmap_config *config
>  	chip->names = config->names;
>  	chip->label = config->label ?: dev_name(config->parent);
>  	chip->can_sleep = regmap_might_sleep(config->regmap);
> +	chip->request = config->request;
> +	chip->free = config->free;
>  
>  	chip->request = gpiochip_generic_request;
>  	chip->free = gpiochip_generic_free;

You probably have a bad rebase here, as the chip->{request,free} functions are immediately
overwritten by gpiochip_generic_{request,free}. Perhaps those can actually be used if you
provide a pinctrl driver for the MFD.


Best,
Sander

Re: [PATCH v4 04/10] gpio: regmap: Allow to provide request and free callbacks

[Mathieu Dubois-Briand]

On Sun Feb 16, 2025 at 2:17 PM CET, Sander Vanheule wrote:
> Hi Mathieu,
>
> On Fri, 2025-02-14 at 12:49 +0100, Mathieu Dubois-Briand wrote:
> > Allows to populate the gpio_regmap_config structure with request() and
> > free() callbacks to set on the final gpio_chip structure.
> > 
> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> > ---
> >  drivers/gpio/gpio-regmap.c  | 2 ++
> >  include/linux/gpio/regmap.h | 7 +++++++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> > index 05f8781b5204..ba72c23bcf97 100644
> > --- a/drivers/gpio/gpio-regmap.c
> > +++ b/drivers/gpio/gpio-regmap.c
> > @@ -261,6 +261,8 @@ struct gpio_regmap *gpio_regmap_register(const struct
> > gpio_regmap_config *config
> >  	chip->names = config->names;
> >  	chip->label = config->label ?: dev_name(config->parent);
> >  	chip->can_sleep = regmap_might_sleep(config->regmap);
> > +	chip->request = config->request;
> > +	chip->free = config->free;
> >  
> >  	chip->request = gpiochip_generic_request;
> >  	chip->free = gpiochip_generic_free;
>
> You probably have a bad rebase here, as the chip->{request,free} functions are immediately
> overwritten by gpiochip_generic_{request,free}. Perhaps those can actually be used if you
> provide a pinctrl driver for the MFD.
>

Thanks, indeed I missed this rebase issue. It's fixed now.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v4 05/10] gpio: regmap: Allow to retrieve ngpio

[Mathieu Dubois-Briand]

Drivers can leave the ngpio field of the gpio_regmap_config structure
uninitialized, letting gpio_regmap_register() retrieve its value from
the "ngpios" device property. Yet, the driver might still need to know
the ngpio value later: allow to extract this value from the gpio_regmap
structure.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/gpio/gpio-regmap.c  | 6 ++++++
 include/linux/gpio/regmap.h | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index ba72c23bcf97..7d8bfde386a4 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -195,6 +195,12 @@ void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio)
 }
 EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata);
 
+u16 gpio_regmap_get_ngpio(struct gpio_regmap *gpio)
+{
+	return gpio->gpio_chip.ngpio;
+}
+EXPORT_SYMBOL_GPL(gpio_regmap_get_ngpio);
+
 /**
  * gpio_regmap_register() - Register a generic regmap GPIO controller
  * @config: configuration for gpio_regmap
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index 16f0c33df75d..0fdc213178d1 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -3,6 +3,8 @@
 #ifndef _LINUX_GPIO_REGMAP_H
 #define _LINUX_GPIO_REGMAP_H
 
+#include <linux/types.h>
+
 struct device;
 struct fwnode_handle;
 struct gpio_regmap;
@@ -97,5 +99,6 @@ void gpio_regmap_unregister(struct gpio_regmap *gpio);
 struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
 					      const struct gpio_regmap_config *config);
 void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio);
+u16 gpio_regmap_get_ngpio(struct gpio_regmap *gpio);
 
 #endif /* _LINUX_GPIO_REGMAP_H */

-- 
2.39.5

Re: [PATCH v4 05/10] gpio: regmap: Allow to retrieve ngpio

[Andy Shevchenko]

On Fri, Feb 14, 2025 at 12:49:55PM +0100, Mathieu Dubois-Briand wrote:
> Drivers can leave the ngpio field of the gpio_regmap_config structure
> uninitialized, letting gpio_regmap_register() retrieve its value from
> the "ngpios" device property. Yet, the driver might still need to know
> the ngpio value later: allow to extract this value from the gpio_regmap
> structure.

I don't think it will be needed after looking at the user. Since we have
somewhat around ten of them already and no-one wanted it makes me feel
that this is a hack which can be avoided.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 06/10] regmap: irq: Add support for chips without separate IRQ status

[Mathieu Dubois-Briand]

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.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/base/regmap/regmap-irq.c | 83 ++++++++++++++++++++++++++++++----------
 include/linux/regmap.h           |  3 ++
 2 files changed, 66 insertions(+), 20 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 0bcd81389a29..531ea799383b 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -33,6 +33,7 @@ struct regmap_irq_chip_data {
 	void *status_reg_buf;
 	unsigned int *main_status_buf;
 	unsigned int *status_buf;
+	unsigned int *prev_status_buf;
 	unsigned int *mask_buf;
 	unsigned int *mask_buf_def;
 	unsigned int *wake_buf;
@@ -332,27 +333,13 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data,
 	return ret;
 }
 
-static irqreturn_t regmap_irq_thread(int irq, void *d)
+static int read_irq_data(struct regmap_irq_chip_data *data)
 {
-	struct regmap_irq_chip_data *data = d;
 	const struct regmap_irq_chip *chip = data->chip;
 	struct regmap *map = data->map;
 	int ret, i;
-	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);
-			goto exit;
-		}
-	}
-
 	/*
 	 * Read only registers with active IRQs if the chip has 'main status
 	 * register'. Else read in the statuses, using a single bulk read if
@@ -382,7 +369,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				dev_err(map->dev,
 					"Failed to read IRQ status %d\n",
 					ret);
-				goto exit;
+				return ret;
 			}
 		}
 
@@ -401,7 +388,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 					dev_err(map->dev,
 						"Failed to read IRQ status %d\n",
 						ret);
-					goto exit;
+					return ret;
 				}
 			}
 
@@ -420,7 +407,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		if (ret != 0) {
 			dev_err(map->dev, "Failed to read IRQ status: %d\n",
 				ret);
-			goto exit;
+			return ret;
 		}
 
 		for (i = 0; i < data->chip->num_regs; i++) {
@@ -436,7 +423,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				break;
 			default:
 				BUG();
-				goto exit;
+				return ret;
 			}
 		}
 
@@ -450,7 +437,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				dev_err(map->dev,
 					"Failed to read IRQ status: %d\n",
 					ret);
-				goto exit;
+				return ret;
 			}
 		}
 	}
@@ -459,6 +446,43 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		for (i = 0; i < data->chip->num_regs; i++)
 			data->status_buf[i] = ~data->status_buf[i];
 
+	return 0;
+}
+
+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;
+	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);
+			goto exit;
+		}
+	}
+
+	ret = read_irq_data(data);
+	if (ret < 0)
+		goto exit;
+
+	if (chip->status_is_level) {
+		for (i = 0; i < data->chip->num_regs; i++) {
+			unsigned int val = data->status_buf[i];
+
+			data->status_buf[i] ^= data->prev_status_buf[i];
+			data->prev_status_buf[i] = val;
+		}
+	}
+
 	/*
 	 * Ignore masked IRQs and ack if we need to; we ack early so
 	 * there is no race between handling and acknowledging the
@@ -705,6 +729,13 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	if (!d->status_buf)
 		goto err_alloc;
 
+	if (chip->status_is_level) {
+		d->prev_status_buf = kcalloc(chip->num_regs, sizeof(*d->prev_status_buf),
+					     GFP_KERNEL);
+		if (!d->prev_status_buf)
+			goto err_alloc;
+	}
+
 	d->mask_buf = kcalloc(chip->num_regs, sizeof(*d->mask_buf),
 			      GFP_KERNEL);
 	if (!d->mask_buf)
@@ -881,6 +912,16 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		}
 	}
 
+	/* Store current levels */
+	if (chip->status_is_level) {
+		ret = read_irq_data(d);
+		if (ret < 0)
+			goto err_alloc;
+
+		for (i = 0; i < d->chip->num_regs; i++)
+			d->prev_status_buf[i] = d->status_buf[i];
+	}
+
 	ret = regmap_irq_create_domain(fwnode, irq_base, chip, d);
 	if (ret)
 		goto err_alloc;
@@ -907,6 +948,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	kfree(d->mask_buf_def);
 	kfree(d->mask_buf);
 	kfree(d->status_buf);
+	kfree(d->prev_status_buf);
 	kfree(d->status_reg_buf);
 	if (d->config_buf) {
 		for (i = 0; i < chip->num_config_bases; i++)
@@ -983,6 +1025,7 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d)
 	kfree(d->mask_buf);
 	kfree(d->status_reg_buf);
 	kfree(d->status_buf);
+	kfree(d->prev_status_buf);
 	if (d->config_buf) {
 		for (i = 0; i < d->chip->num_config_bases; i++)
 			kfree(d->config_buf[i]);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 3a96d068915f..159527e97f00 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1640,6 +1640,8 @@ struct regmap_irq_chip_data;
  * @ack_invert:  Inverted ack register: cleared bits for ack.
  * @clear_ack:  Use this to set 1 and 0 or vice-versa to clear interrupts.
  * @status_invert: Inverted status register: cleared bits are active interrupts.
+ * @status_is_level: Status register is actuall signal level: Xor status
+ *		     register with previous value to get active interrupts.
  * @wake_invert: Inverted wake register: cleared bits are wake enabled.
  * @type_in_mask: Use the mask registers for controlling irq type. Use this if
  *		  the hardware provides separate bits for rising/falling edge
@@ -1703,6 +1705,7 @@ struct regmap_irq_chip {
 	unsigned int ack_invert:1;
 	unsigned int clear_ack:1;
 	unsigned int status_invert:1;
+	unsigned int status_is_level:1;
 	unsigned int wake_invert:1;
 	unsigned int type_in_mask:1;
 	unsigned int clear_on_unmask:1;

-- 
2.39.5

Re: [PATCH v4 06/10] regmap: irq: Add support for chips without separate IRQ status

[Andy Shevchenko]

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;
?

> +	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.

> +			goto exit;
> +		}
> +	}
> +
> +	ret = read_irq_data(data);
> +	if (ret < 0)
> +		goto exit;
> +
> +	if (chip->status_is_level) {
> +		for (i = 0; i < data->chip->num_regs; i++) {
> +			unsigned int val = data->status_buf[i];
> +
> +			data->status_buf[i] ^= data->prev_status_buf[i];
> +			data->prev_status_buf[i] = val;
> +		}
> +	}

...

> +		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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 06/10] regmap: irq: Add support for chips without separate IRQ status

[Mathieu Dubois-Briand]

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.


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 06/10] regmap: irq: Add support for chips without separate IRQ status

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 06/10] regmap: irq: Add support for chips without separate IRQ status

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 324 bytes --]

On Fri, Feb 14, 2025 at 05:18:17PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 14, 2025 at 12:49:56PM +0100, Mathieu Dubois-Briand wrote:

> > +	int ret, i;

> 	unsigned int i;
> ?

If it's just an iterator it's idiomatic to use signed ints.  IIRC if it
makes a difference to the code generation it's likely to be positive.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 06/10] regmap: irq: Add support for chips without separate IRQ status

[Andy Shevchenko]

On Wed, Feb 26, 2025 at 01:18:16PM +0000, Mark Brown wrote:
> On Fri, Feb 14, 2025 at 05:18:17PM +0200, Andy Shevchenko wrote:
> > On Fri, Feb 14, 2025 at 12:49:56PM +0100, Mathieu Dubois-Briand wrote:
> 
> > > +	int ret, i;
> 
> > 	unsigned int i;
> > ?
> 
> If it's just an iterator it's idiomatic to use signed ints.  IIRC if it
> makes a difference to the code generation it's likely to be positive.

It depends on the subsystem, V4L2, for example, is strict to unsigned types
when they are unsigned. It also helps to catch some strange conditionals at
some point when one checks for negative value in the (incrementing) counter.

But I'm not insisting as you may notice by question mark used.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

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>
---
 drivers/gpio/Kconfig        |  11 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-max7360.c | 253 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 265 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 98b4d1633b25..3b3babc1c029 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1445,6 +1445,17 @@ config GPIO_MADERA
 	help
 	  Support for GPIOs on Cirrus Logic Madera class codecs.
 
+config GPIO_MAX7360
+	tristate "MAX7360 GPIO support"
+	depends on MFD_MAX7360
+	select GPIO_REGMAP
+	help
+	  Allows to use MAX7360 I/O Expander PWM lines as GPIO and keypad COL
+	  lines as GPO.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-max7360.
+
 config GPIO_MAX77620
 	tristate "GPIO support for PMIC MAX77620 and MAX20024"
 	depends on MFD_MAX77620
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index af3ba4d81b58..581341b3e3e4 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -100,6 +100,7 @@ obj-$(CONFIG_GPIO_MAX7300)		+= gpio-max7300.o
 obj-$(CONFIG_GPIO_MAX7301)		+= gpio-max7301.o
 obj-$(CONFIG_GPIO_MAX730X)		+= gpio-max730x.o
 obj-$(CONFIG_GPIO_MAX732X)		+= gpio-max732x.o
+obj-$(CONFIG_GPIO_MAX7360)		+= gpio-max7360.o
 obj-$(CONFIG_GPIO_MAX77620)		+= gpio-max77620.o
 obj-$(CONFIG_GPIO_MAX77650)		+= gpio-max77650.o
 obj-$(CONFIG_GPIO_MB86S7X)		+= gpio-mb86s7x.o
diff --git a/drivers/gpio/gpio-max7360.c b/drivers/gpio/gpio-max7360.c
new file mode 100644
index 000000000000..99ccda687b88
--- /dev/null
+++ b/drivers/gpio/gpio-max7360.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Bootlin
+ *
+ * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com>
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/max7360.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define MAX7360_GPIO_PORT	1
+#define MAX7360_GPIO_COL	2
+
+static int max7360_gpo_reg_mask_xlate(struct gpio_regmap *gpio,
+				      unsigned int base, unsigned int offset,
+				      unsigned int *reg, unsigned int *mask)
+{
+	u16 ngpios = gpio_regmap_get_ngpio(gpio);
+
+	*reg = base;
+	*mask = BIT(MAX7360_MAX_KEY_COLS - (ngpios - offset));
+
+	return 0;
+}
+
+static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin)
+{
+	/*
+	 * GPIOs on PORT pins are shared with the PWM and rotary encoder
+	 * drivers: they have to be requested from the MFD driver.
+	 */
+	return max7360_port_pin_request(gc->parent->parent, pin, true);
+}
+
+static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin)
+{
+	max7360_port_pin_request(gc->parent->parent, pin, false);
+}
+
+static int max7360_set_gpos_count(struct device *dev, struct regmap *regmap)
+{
+	/*
+	 * MAX7360 COL0 to COL7 pins can be used either as keypad columns,
+	 * general purpose output or a mix of both.
+	 */
+	unsigned int val;
+	u32 columns;
+	u32 ngpios;
+	int ret;
+
+	ret = device_property_read_u32(dev, "ngpios", &ngpios);
+	if (ret < 0) {
+		dev_err(dev, "Missing ngpios OF property\n");
+		return ret;
+	}
+
+	/*
+	 * Get the number of pins requested by the keypad and ensure our own pin
+	 * count is compatible with it.
+	 */
+	ret = device_property_read_u32(dev->parent, "keypad,num-columns", &columns);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read columns count\n");
+		return ret;
+	}
+
+	if (ngpios > MAX7360_MAX_GPO ||
+	    (ngpios + columns > MAX7360_MAX_KEY_COLS)) {
+		dev_err(dev, "Incompatible gpos and columns count (%u, %u)\n",
+			ngpios, columns);
+		return -EINVAL;
+	}
+
+	/*
+	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
+	 * timings and gpos/keypad columns repartition. Only the later is
+	 * modified here.
+	 */
+	val = FIELD_PREP(MAX7360_PORTS, ngpios);
+	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
+	if (ret) {
+		dev_err(dev, "Failed to write max7360 columns/gpos configuration");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct regmap_irq max7360_regmap_irqs[MAX7360_MAX_GPIO] = {
+	REGMAP_IRQ_REG(0, 0, BIT(0)),
+	REGMAP_IRQ_REG(1, 0, BIT(1)),
+	REGMAP_IRQ_REG(2, 0, BIT(2)),
+	REGMAP_IRQ_REG(3, 0, BIT(3)),
+	REGMAP_IRQ_REG(4, 0, BIT(4)),
+	REGMAP_IRQ_REG(5, 0, BIT(5)),
+	REGMAP_IRQ_REG(6, 0, BIT(6)),
+	REGMAP_IRQ_REG(7, 0, BIT(7)),
+};
+
+static int max7360_handle_mask_sync(const int index,
+				    const unsigned int mask_buf_def,
+				    const unsigned int mask_buf,
+				    void *const irq_drv_data)
+{
+	struct regmap *regmap = irq_drv_data;
+	unsigned int val;
+
+	for (unsigned int i = 0; i < MAX7360_MAX_GPIO; ++i) {
+		val = (mask_buf & 1 << i) ? MAX7360_PORT_CFG_INTERRUPT_MASK : 0;
+		regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
+				  MAX7360_PORT_CFG_INTERRUPT_MASK, val);
+	}
+
+	return 0;
+}
+
+static int max7360_gpio_probe(struct platform_device *pdev)
+{
+	struct regmap_irq_chip *irq_chip;
+	struct regmap_irq_chip_data *irq_chip_data;
+	struct gpio_regmap_config gpio_config = { };
+	struct device *dev = &pdev->dev;
+	unsigned long gpio_function;
+	struct regmap *regmap;
+	unsigned int outconf;
+	unsigned long flags;
+	int irq;
+	int ret;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n");
+
+	gpio_function = (uintptr_t)device_get_match_data(dev);
+
+	if (gpio_function == MAX7360_GPIO_PORT &&
+	    (device_property_read_bool(dev, "interrupt-controller"))) {
+		/*
+		 * Port GPIOs with interrupt-controller property: add IRQ
+		 * controller.
+		 */
+		irq = fwnode_irq_get_byname(dev->parent->fwnode, "inti");
+		if (irq < 0)
+			return dev_err_probe(dev, irq, "Failed to get IRQ\n");
+
+		irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
+		if (!irq_chip)
+			return -ENOMEM;
+
+		irq_chip->name = dev_name(dev);
+		irq_chip->status_base = MAX7360_REG_GPIOIN;
+		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->status_is_level = true;
+		irq_chip->irq_drv_data = regmap;
+
+		for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) {
+			regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
+					  MAX7360_PORT_CFG_INTERRUPT_EDGES,
+					  MAX7360_PORT_CFG_INTERRUPT_EDGES);
+		}
+
+		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
+		ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0,
+						      irq_chip, &irq_chip_data);
+		if (ret)
+			return dev_err_probe(dev, ret, "IRQ registration failed\n");
+
+		gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data);
+	}
+
+	if (gpio_function == MAX7360_GPIO_PORT) {
+		/*
+		 * Port GPIOs: set output mode configuration (constant-current or not).
+		 * This property is optional.
+		 */
+		outconf = 0;
+		ret = device_property_read_u32(dev, "maxim,constant-current-disable", &outconf);
+		if (ret && (ret != -EINVAL))
+			return dev_err_probe(dev, -ENODEV,
+					     "Failed to read maxim,constant-current-disable OF property\n");
+
+		regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
+	}
+
+	/* Add gpio device. */
+	gpio_config.parent = dev;
+	gpio_config.regmap = regmap;
+	if (gpio_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.request = max7360_gpio_request;
+		gpio_config.free = max7360_gpio_free;
+	} 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;
+
+		ret = max7360_set_gpos_count(dev, regmap);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to set GPOS pin count\n");
+	}
+
+	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &gpio_config));
+}
+
+static const struct of_device_id max7360_gpio_of_match[] = {
+	{
+		.compatible = "maxim,max7360-gpo",
+		.data = (void *)MAX7360_GPIO_COL
+	}, {
+		.compatible = "maxim,max7360-gpio",
+		.data = (void *)MAX7360_GPIO_PORT
+	}, {
+	}
+};
+MODULE_DEVICE_TABLE(of, max7360_gpio_of_match);
+
+static struct platform_driver max7360_gpio_driver = {
+	.driver = {
+		.name	= "max7360-gpio",
+		.of_match_table = max7360_gpio_of_match,
+	},
+	.probe		= max7360_gpio_probe,
+};
+module_platform_driver(max7360_gpio_driver);
+
+MODULE_DESCRIPTION("MAX7360 GPIO driver");
+MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>");
+MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

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/

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote:
> Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.

...

> +static int max7360_gpo_reg_mask_xlate(struct gpio_regmap *gpio,
> +				      unsigned int base, unsigned int offset,
> +				      unsigned int *reg, unsigned int *mask)
> +{
> +	u16 ngpios = gpio_regmap_get_ngpio(gpio);
> +
> +	*reg = base;
> +	*mask = BIT(MAX7360_MAX_KEY_COLS - (ngpios - offset));
> +
> +	return 0;

Does this GPIO controller only capable of servicing keypads?
I think no, hence I'm not sure why this split is needed to be
here and not in the input driver.

Or you mean that there output only GPIO lines in HW after all?
Is there a link to the datasheet?

> +}
> +
> +static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin)
> +{
> +	/*
> +	 * GPIOs on PORT pins are shared with the PWM and rotary encoder
> +	 * drivers: they have to be requested from the MFD driver.
> +	 */

So, this sounds to me like a pin control approach is needed here.
This looks like an attempt to hack it in an "easy" way.

> +	return max7360_port_pin_request(gc->parent->parent, pin, true);
> +}
> +
> +static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin)
> +{
> +	max7360_port_pin_request(gc->parent->parent, pin, false);
> +}
> +
> +static int max7360_set_gpos_count(struct device *dev, struct regmap *regmap)
> +{
> +	/*
> +	 * MAX7360 COL0 to COL7 pins can be used either as keypad columns,
> +	 * general purpose output or a mix of both.
> +	 */
> +	unsigned int val;
> +	u32 columns;
> +	u32 ngpios;
> +	int ret;
> +
> +	ret = device_property_read_u32(dev, "ngpios", &ngpios);
> +	if (ret < 0) {
> +		dev_err(dev, "Missing ngpios OF property\n");

Clean messages from OF, "device property" is established term.

> +		return ret;
> +	}
> +
> +	/*
> +	 * Get the number of pins requested by the keypad and ensure our own pin
> +	 * count is compatible with it.
> +	 */
> +	ret = device_property_read_u32(dev->parent, "keypad,num-columns", &columns);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read columns count\n");
> +		return ret;
> +	}
> +
> +	if (ngpios > MAX7360_MAX_GPO ||
> +	    (ngpios + columns > MAX7360_MAX_KEY_COLS)) {
> +		dev_err(dev, "Incompatible gpos and columns count (%u, %u)\n",
> +			ngpios, columns);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
> +	 * timings and gpos/keypad columns repartition. Only the later is
> +	 * modified here.
> +	 */
> +	val = FIELD_PREP(MAX7360_PORTS, ngpios);
> +	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
> +	if (ret) {
> +		dev_err(dev, "Failed to write max7360 columns/gpos configuration");
> +		return ret;
> +	}

Shouldn't this be configured via ->set_config() callback?

> +	return 0;
> +}

...

> +static int max7360_handle_mask_sync(const int index,
> +				    const unsigned int mask_buf_def,
> +				    const unsigned int mask_buf,
> +				    void *const irq_drv_data)
> +{
> +	struct regmap *regmap = irq_drv_data;
> +	unsigned int val;
> +
> +	for (unsigned int i = 0; i < MAX7360_MAX_GPIO; ++i) {
> +		val = (mask_buf & 1 << i) ? MAX7360_PORT_CFG_INTERRUPT_MASK : 0;

(mask_buf & BIT(i)) ?

> +		regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
> +				  MAX7360_PORT_CFG_INTERRUPT_MASK, val);
> +	}
> +
> +	return 0;
> +}
> +
> +static int max7360_gpio_probe(struct platform_device *pdev)
> +{
> +	struct regmap_irq_chip *irq_chip;
> +	struct regmap_irq_chip_data *irq_chip_data;
> +	struct gpio_regmap_config gpio_config = { };
> +	struct device *dev = &pdev->dev;
> +	unsigned long gpio_function;
> +	struct regmap *regmap;
> +	unsigned int outconf;
> +	unsigned long flags;
> +	int irq;
> +	int ret;
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap)
> +		return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n");
> +
> +	gpio_function = (uintptr_t)device_get_match_data(dev);
> +
> +	if (gpio_function == MAX7360_GPIO_PORT &&
> +	    (device_property_read_bool(dev, "interrupt-controller"))) {
> +		/*
> +		 * Port GPIOs with interrupt-controller property: add IRQ
> +		 * controller.
> +		 */
> +		irq = fwnode_irq_get_byname(dev->parent->fwnode, "inti");

Use dev_fwnode() to get fwnode from struct device pointer.

> +		if (irq < 0)
> +			return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> +
> +		irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
> +		if (!irq_chip)
> +			return -ENOMEM;
> +
> +		irq_chip->name = dev_name(dev);
> +		irq_chip->status_base = MAX7360_REG_GPIOIN;
> +		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->status_is_level = true;
> +		irq_chip->irq_drv_data = regmap;
> +
> +		for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) {
> +			regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
> +					  MAX7360_PORT_CFG_INTERRUPT_EDGES,
> +					  MAX7360_PORT_CFG_INTERRUPT_EDGES);
> +		}
> +
> +		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
> +		ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0,
> +						      irq_chip, &irq_chip_data);

Right.

What I mean in previous discussion is to update gpio-regmap to call this from inside.
You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration
and if they are set (or the first one, I dunno if this is supported by IRQ chip core)
call this function and assign domain. This should be called after GPIO chip is
added, but before IRQ domain attachment.

> +		if (ret)
> +			return dev_err_probe(dev, ret, "IRQ registration failed\n");
> +
> +		gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data);
> +	}
> +
> +	if (gpio_function == MAX7360_GPIO_PORT) {
> +		/*
> +		 * Port GPIOs: set output mode configuration (constant-current or not).
> +		 * This property is optional.
> +		 */
> +		outconf = 0;
> +		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 :-)

> +
> +		regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
> +	}
> +
> +	/* Add gpio device. */
> +	gpio_config.parent = dev;
> +	gpio_config.regmap = regmap;

> +	if (gpio_function == MAX7360_GPIO_PORT) {
> +		gpio_config.ngpio = MAX7360_MAX_GPIO;

Why this case can't be managed also via ngpios property? Maybe at the end of
the day you rather need to have another property to tell where the split is?

This will help a lot and removes unneeded sharing of ngpios here and there.

What I read from this code is like you are trying to put _two_in_one_ semantics
on the shoulders of "ngpios".

> +		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.request = max7360_gpio_request;
> +		gpio_config.free = max7360_gpio_free;
> +	} 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;
> +
> +		ret = max7360_set_gpos_count(dev, regmap);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to set GPOS pin count\n");
> +	}
> +
> +	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &gpio_config));
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

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");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote:
> On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote:
> > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.
>
> ...
>
> > +static int max7360_gpo_reg_mask_xlate(struct gpio_regmap *gpio,
> > +				      unsigned int base, unsigned int offset,
> > +				      unsigned int *reg, unsigned int *mask)
> > +{
> > +	u16 ngpios = gpio_regmap_get_ngpio(gpio);
> > +
> > +	*reg = base;
> > +	*mask = BIT(MAX7360_MAX_KEY_COLS - (ngpios - offset));
> > +
> > +	return 0;
>
> Does this GPIO controller only capable of servicing keypads?
> I think no, hence I'm not sure why this split is needed to be
> here and not in the input driver.
>

I would say it's more a keypad controller able to support some GPIOs.
Some of the keypad columns, if unused, can be used as output-only gpios.
So I believe the split has its place here, because in the default
configuration, the split is set to have 8 keypad columns and no gpio. As
a consequence, the keypad driver can work without having to worry about
the split; the gpio driver needs to know about it.

To provide a bit more details, there is basically two set of pins usable
as GPIOs.

On one side we have what I refer to as GPIOs:
  - PORT0 to PORT7 pins of the chip.
  - Shared with PWM and rotary encoder functionalities. Functionality
    selection can be made independently for each pin. We have to ensure
    the same pin is not used by two drivers at the same time. E.g. we
    cannot have at the same time GPIO4 and PWM4.
  - Supports input and interrupts.
  - Outputs may be configured as constant current.
  - 8 GPIOS supported, so ngpios is fixed to MAX7360_MAX_GPIO.
  - maxim,max7360-gpio compatible, gpio_function == MAX7360_GPIO_PORT.

On the other side, we have what I refer to as GPOs:
  - COL2 to COL7 pins of the chip.
  - Shared with the keypad functionality. Selections is made by
    partitioning the pins: first pins for keypad columns, last pins for
    GPOs. Partition is described by the ngpios property.
  - Only support outputs.
  - maxim,max7360-gpo compatible, gpio_function == MAX7360_GPIO_COL.

> Or you mean that there output only GPIO lines in HW after all?
> Is there a link to the datasheet?

A datasheet is available on https://www.analog.com/en/products/max7360.html

>
> > +}
> > +
> > +static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin)
> > +{
> > +	/*
> > +	 * GPIOs on PORT pins are shared with the PWM and rotary encoder
> > +	 * drivers: they have to be requested from the MFD driver.
> > +	 */
>
> So, this sounds to me like a pin control approach is needed here.
> This looks like an attempt to hack it in an "easy" way.
>

Linus Walleij had a similar comment on v3, but said he thought it was
fine here. Still, I'm open to discussion.

> > +	return max7360_port_pin_request(gc->parent->parent, pin, true);
> > +}
> > +
> > +static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin)
> > +{
> > +	max7360_port_pin_request(gc->parent->parent, pin, false);
> > +}
> > +
> > +static int max7360_set_gpos_count(struct device *dev, struct regmap *regmap)
> > +{
> > +	/*
> > +	 * MAX7360 COL0 to COL7 pins can be used either as keypad columns,
> > +	 * general purpose output or a mix of both.
> > +	 */
> > +	unsigned int val;
> > +	u32 columns;
> > +	u32 ngpios;
> > +	int ret;
> > +
> > +	ret = device_property_read_u32(dev, "ngpios", &ngpios);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Missing ngpios OF property\n");
>
> Clean messages from OF, "device property" is established term.
>

Yes

> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * Get the number of pins requested by the keypad and ensure our own pin
> > +	 * count is compatible with it.
> > +	 */
> > +	ret = device_property_read_u32(dev->parent, "keypad,num-columns", &columns);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to read columns count\n");
> > +		return ret;
> > +	}
> > +
> > +	if (ngpios > MAX7360_MAX_GPO ||
> > +	    (ngpios + columns > MAX7360_MAX_KEY_COLS)) {
> > +		dev_err(dev, "Incompatible gpos and columns count (%u, %u)\n",
> > +			ngpios, columns);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
> > +	 * timings and gpos/keypad columns repartition. Only the later is
> > +	 * modified here.
> > +	 */
> > +	val = FIELD_PREP(MAX7360_PORTS, ngpios);
> > +	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to write max7360 columns/gpos configuration");
> > +		return ret;
> > +	}
>
> Shouldn't this be configured via ->set_config() callback?
>

My understanding of the set_config() callback is that it's meant to set
the configuration of a single line. Here the configuration applies to
the whole chip.

> > +	return 0;
> > +}
>
> ...
>
> > +		if (irq < 0)
> > +			return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> > +
> > +		irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
> > +		if (!irq_chip)
> > +			return -ENOMEM;
> > +
> > +		irq_chip->name = dev_name(dev);
> > +		irq_chip->status_base = MAX7360_REG_GPIOIN;
> > +		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->status_is_level = true;
> > +		irq_chip->irq_drv_data = regmap;
> > +
> > +		for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) {
> > +			regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
> > +					  MAX7360_PORT_CFG_INTERRUPT_EDGES,
> > +					  MAX7360_PORT_CFG_INTERRUPT_EDGES);
> > +		}
> > +
> > +		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
> > +		ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0,
> > +						      irq_chip, &irq_chip_data);
>
> Right.
>
> What I mean in previous discussion is to update gpio-regmap to call this from inside.
> You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration
> and if they are set (or the first one, I dunno if this is supported by IRQ chip core)
> call this function and assign domain. This should be called after GPIO chip is
> added, but before IRQ domain attachment.
>

OK, I believe I got it now. I will try to work on it in the coming days.

> > +
> > +		regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
> > +	}
> > +
> > +	/* Add gpio device. */
> > +	gpio_config.parent = dev;
> > +	gpio_config.regmap = regmap;
>
> > +	if (gpio_function == MAX7360_GPIO_PORT) {
> > +		gpio_config.ngpio = MAX7360_MAX_GPIO;
>
> Why this case can't be managed also via ngpios property? Maybe at the end of
> the day you rather need to have another property to tell where the split is?
>
> This will help a lot and removes unneeded sharing of ngpios here and there.
>
> What I read from this code is like you are trying to put _two_in_one_ semantics
> on the shoulders of "ngpios".
>

It could be managed with ngpios, just there is no specific need as we
will always have 8 gpios here. With (gpio_function ==
MAX7360_GPIO_PORT), there is no split and starting from this version of
my series, there is no reuse on ngpios property.

The split and reuse of ngpios is only used for GPO on keypad columns
(gpio_function == MAX7360_GPIO_COL).

We can introduce a new property to tell where the split is, but the
number of gpio is a direct consequence of the position of the split.


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

On Mon, Feb 17, 2025 at 12:20:13PM +0100, Mathieu Dubois-Briand wrote:
> On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote:
> > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote:
> > > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.

...

> > > +static int max7360_gpo_reg_mask_xlate(struct gpio_regmap *gpio,
> > > +				      unsigned int base, unsigned int offset,
> > > +				      unsigned int *reg, unsigned int *mask)
> > > +{
> > > +	u16 ngpios = gpio_regmap_get_ngpio(gpio);
> > > +
> > > +	*reg = base;
> > > +	*mask = BIT(MAX7360_MAX_KEY_COLS - (ngpios - offset));
> > > +
> > > +	return 0;
> >
> > Does this GPIO controller only capable of servicing keypads?
> > I think no, hence I'm not sure why this split is needed to be
> > here and not in the input driver.
> 
> I would say it's more a keypad controller able to support some GPIOs.
> Some of the keypad columns, if unused, can be used as output-only gpios.
> So I believe the split has its place here, because in the default
> configuration, the split is set to have 8 keypad columns and no gpio. As
> a consequence, the keypad driver can work without having to worry about
> the split; the gpio driver needs to know about it.
> 
> To provide a bit more details, there is basically two set of pins usable
> as GPIOs.
> 
> On one side we have what I refer to as GPIOs:
>   - PORT0 to PORT7 pins of the chip.
>   - Shared with PWM and rotary encoder functionalities. Functionality
>     selection can be made independently for each pin. We have to ensure
>     the same pin is not used by two drivers at the same time. E.g. we
>     cannot have at the same time GPIO4 and PWM4.
>   - Supports input and interrupts.
>   - Outputs may be configured as constant current.
>   - 8 GPIOS supported, so ngpios is fixed to MAX7360_MAX_GPIO.
>   - maxim,max7360-gpio compatible, gpio_function == MAX7360_GPIO_PORT.
> 
> On the other side, we have what I refer to as GPOs:
>   - COL2 to COL7 pins of the chip.
>   - Shared with the keypad functionality. Selections is made by
>     partitioning the pins: first pins for keypad columns, last pins for
>     GPOs. Partition is described by the ngpios property.
>   - Only support outputs.
>   - maxim,max7360-gpo compatible, gpio_function == MAX7360_GPIO_COL.
> 
> > Or you mean that there output only GPIO lines in HW after all?
> > Is there a link to the datasheet?
> 
> A datasheet is available on https://www.analog.com/en/products/max7360.html

Thank you for this good elaboration!
I will check on the datasheet later on, having one week off.

But what I have read above sounds to me like the following:

1) the PORT0-PORT7 should be just a regular pin control with the respective
function being provided (see pinctrl-cy8c95x0.c as an example);

2) the COL2 COL7 case can be modeled as a simplest GPIO (GPO) driver with
reserved lines property (this will set valid mask and let GPIOLIB to refuse any
use of the keypad connected pins.

So, with this approach the entire handling becomes less hackish and quite
straightforward!


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Mon Feb 17, 2025 at 9:08 PM CET, Andy Shevchenko wrote:
> On Mon, Feb 17, 2025 at 12:20:13PM +0100, Mathieu Dubois-Briand wrote:
> > To provide a bit more details, there is basically two set of pins usable
> > as GPIOs.
> > 
> > On one side we have what I refer to as GPIOs:
> >   - PORT0 to PORT7 pins of the chip.
> >   - Shared with PWM and rotary encoder functionalities. Functionality
> >     selection can be made independently for each pin. We have to ensure
> >     the same pin is not used by two drivers at the same time. E.g. we
> >     cannot have at the same time GPIO4 and PWM4.
> >   - Supports input and interrupts.
> >   - Outputs may be configured as constant current.
> >   - 8 GPIOS supported, so ngpios is fixed to MAX7360_MAX_GPIO.
> >   - maxim,max7360-gpio compatible, gpio_function == MAX7360_GPIO_PORT.
> > 
> > On the other side, we have what I refer to as GPOs:
> >   - COL2 to COL7 pins of the chip.
> >   - Shared with the keypad functionality. Selections is made by
> >     partitioning the pins: first pins for keypad columns, last pins for
> >     GPOs. Partition is described by the ngpios property.
> >   - Only support outputs.
> >   - maxim,max7360-gpo compatible, gpio_function == MAX7360_GPIO_COL.
> > 
> > > Or you mean that there output only GPIO lines in HW after all?
> > > Is there a link to the datasheet?
> > 
> > A datasheet is available on https://www.analog.com/en/products/max7360.html
>
> Thank you for this good elaboration!
> I will check on the datasheet later on, having one week off.
>

Thanks for your feedback! Sorry I haven't been able to work on this
series for the last few weeks, but I finally had the opportunity to
integrate your comments.

> But what I have read above sounds to me like the following:
>
> 1) the PORT0-PORT7 should be just a regular pin control with the respective
> function being provided (see pinctrl-cy8c95x0.c as an example);
>

Ok, so I created a pin control driver for the PORT pins. This will
effectively help to prevent concurrent use of pins in place of the
request()/free() callbacks.

My only concern is: as there is no real pin muxing on the chip, my
.set_mux callabck in pinmux_ops structure is not doing anything. It
looks like I'm not the only one
(drivers/pinctrl/pinctrl-microchip-sgpio.c does the same thing), but I
hope this is OK.

> 2) the COL2 COL7 case can be modeled as a simplest GPIO (GPO) driver with
> reserved lines property (this will set valid mask and let GPIOLIB to refuse any
> use of the keypad connected pins.
>

I mostly went that way, just a few notes.

I chose to not use the reserved lines property in the device tree, but
instead implemented a gpiolib init_valid_mask() callback. In believe
this is better, as:
- We can automatically generate the valid gpios mask, based on the
  number of columns used.
- It allows to get rid of the compatibility check between the number of
  columns and the number of GPIOs provided by the device tree: DT
  provides the number of columns, we deduct the number of GPIOs.

I chose to number GPIOs from 0 to 7.
- This might be a bit questionable, as GPIO 0 and 1 will always be
  invalid: pins 0 and 1 of the chip cannot be used as GPIOs. I'm
  definitely open to discussion on this point.
- Yet I believe it simplifies everything for the user: pin numbers and
  GPIO numbers are the same instead of having an offset of 2.
- It also simplifies a bit the GPIO driver code.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

Thu, Mar 13, 2025 at 05:43:00PM +0100, Mathieu Dubois-Briand kirjoitti:
> On Mon Feb 17, 2025 at 9:08 PM CET, Andy Shevchenko wrote:
> > On Mon, Feb 17, 2025 at 12:20:13PM +0100, Mathieu Dubois-Briand wrote:

...

> > > A datasheet is available on https://www.analog.com/en/products/max7360.html
> >
> > Thank you for this good elaboration!
> > I will check on the datasheet later on, having one week off.

Note, I have only briefly looked at it, not a deep study and TBH I am not sure
I will have time to invest into that.

> Thanks for your feedback! Sorry I haven't been able to work on this
> series for the last few weeks, but I finally had the opportunity to
> integrate your comments.

No rush, this will miss v6.15 anyway, so we still have a couple of months.

> > But what I have read above sounds to me like the following:
> >
> > 1) the PORT0-PORT7 should be just a regular pin control with the respective
> > function being provided (see pinctrl-cy8c95x0.c as an example);
> 
> Ok, so I created a pin control driver for the PORT pins. This will
> effectively help to prevent concurrent use of pins in place of the
> request()/free() callbacks.
> 
> My only concern is: as there is no real pin muxing on the chip, my
> .set_mux callabck in pinmux_ops structure is not doing anything. It
> looks like I'm not the only one
> (drivers/pinctrl/pinctrl-microchip-sgpio.c does the same thing), but I
> hope this is OK.

Hmm... This is strange. The PWM/GPIO block has 3 functions (GPIO/PWM/rotary),
How comes you have no switch between them?

As far as I read in the datasheet this is controlled by register 0x40
(and seems implicitly by other registers when it's in PWM mode).

> > 2) the COL2 COL7 case can be modeled as a simplest GPIO (GPO) driver with
> > reserved lines property (this will set valid mask and let GPIOLIB to refuse any
> > use of the keypad connected pins.
> 
> I mostly went that way, just a few notes.
> 
> I chose to not use the reserved lines property in the device tree, but
> instead implemented a gpiolib init_valid_mask() callback. In believe
> this is better, as:
> - We can automatically generate the valid gpios mask, based on the
>   number of columns used.
> - It allows to get rid of the compatibility check between the number of
>   columns and the number of GPIOs provided by the device tree: DT
>   provides the number of columns, we deduct the number of GPIOs.

If I understood it correctly it should work as well. But let's discuss that
when you issue a new version.

> I chose to number GPIOs from 0 to 7.
> - This might be a bit questionable, as GPIO 0 and 1 will always be
>   invalid: pins 0 and 1 of the chip cannot be used as GPIOs. I'm
>   definitely open to discussion on this point.
> - Yet I believe it simplifies everything for the user: pin numbers and
>   GPIO numbers are the same instead of having an offset of 2.
> - It also simplifies a bit the GPIO driver code.

In general you should follow the datasheet and mask the GPIOs that may not be
uses as a such due to HW limitation / specific configuration.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Fri Mar 14, 2025 at 9:02 AM CET, Andy Shevchenko wrote:
> Thu, Mar 13, 2025 at 05:43:00PM +0100, Mathieu Dubois-Briand kirjoitti:
> > On Mon Feb 17, 2025 at 9:08 PM CET, Andy Shevchenko wrote:
> > > On Mon, Feb 17, 2025 at 12:20:13PM +0100, Mathieu Dubois-Briand wrote:
>
> ...
>
> > > But what I have read above sounds to me like the following:
> > >
> > > 1) the PORT0-PORT7 should be just a regular pin control with the respective
> > > function being provided (see pinctrl-cy8c95x0.c as an example);
> > 
> > Ok, so I created a pin control driver for the PORT pins. This will
> > effectively help to prevent concurrent use of pins in place of the
> > request()/free() callbacks.
> > 
> > My only concern is: as there is no real pin muxing on the chip, my
> > .set_mux callabck in pinmux_ops structure is not doing anything. It
> > looks like I'm not the only one
> > (drivers/pinctrl/pinctrl-microchip-sgpio.c does the same thing), but I
> > hope this is OK.
>
> Hmm... This is strange. The PWM/GPIO block has 3 functions (GPIO/PWM/rotary),
> How comes you have no switch between them?
>
> As far as I read in the datasheet this is controlled by register 0x40
> (and seems implicitly by other registers when it's in PWM mode).
>

Yes, on pins 6 and 7, we do switch between rotary encoder and other
modes by writing in the register at 0x40, but that's all. My point was
more about all other modes. There is no difference between PWM and GPIO,
at least in output mode: GPIO level is just a PWM with duty cycle either
to 0% or 100%.


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote:
> On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote:
> > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.
>
> ...
> > +
> > +	/*
> > +	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
> > +	 * timings and gpos/keypad columns repartition. Only the later is
> > +	 * modified here.
> > +	 */
> > +	val = FIELD_PREP(MAX7360_PORTS, ngpios);
> > +	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to write max7360 columns/gpos configuration");
> > +		return ret;
> > +	}
>
> Shouldn't this be configured via ->set_config() callback?
>

I believe this comment has been a bit outdated by our discussion on
using GPIO valid mask, but I believe we could not use the ->set_config()
callback here: this callback is made to configure a single pin while the
gpos/keypad columns repartition is global.

>
> ...
>
> > +		if (irq < 0)
> > +			return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> > +
> > +		irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
> > +		if (!irq_chip)
> > +			return -ENOMEM;
> > +
> > +		irq_chip->name = dev_name(dev);
> > +		irq_chip->status_base = MAX7360_REG_GPIOIN;
> > +		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->status_is_level = true;
> > +		irq_chip->irq_drv_data = regmap;
> > +
> > +		for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) {
> > +			regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
> > +					  MAX7360_PORT_CFG_INTERRUPT_EDGES,
> > +					  MAX7360_PORT_CFG_INTERRUPT_EDGES);
> > +		}
> > +
> > +		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
> > +		ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0,
> > +						      irq_chip, &irq_chip_data);
>
> Right.
>
> What I mean in previous discussion is to update gpio-regmap to call this from inside.
> You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration
> and if they are set (or the first one, I dunno if this is supported by IRQ chip core)
> call this function and assign domain. This should be called after GPIO chip is
> added, but before IRQ domain attachment.
>

Ok, this is a bit more clear to me now. So I came up with something, it
will be part of the next iteration, probably during the next week.

This required to add a few additional fields to the gpio_regmap_config
structure, specifying the IRQ configuration:

+ * @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.
+ * @regmap_irq_irqno   (Optional) The IRQ the device uses to signal interrupts.
+ * @regmap_irq_flags   (Optional) The IRQF_ flags to use for the interrupt.

>
> ...
>
> > +
> > +		regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
> > +	}
> > +
> > +	/* Add gpio device. */
> > +	gpio_config.parent = dev;
> > +	gpio_config.regmap = regmap;
>
> > +	if (gpio_function == MAX7360_GPIO_PORT) {
> > +		gpio_config.ngpio = MAX7360_MAX_GPIO;
>
> Why this case can't be managed also via ngpios property? Maybe at the end of
> the day you rather need to have another property to tell where the split is?
>
> This will help a lot and removes unneeded sharing of ngpios here and there.
>
> What I read from this code is like you are trying to put _two_in_one_ semantics
> on the shoulders of "ngpios".
>

So as I reworked the keypad columns GPIOs, PORT GPIOs and the COL GPIOs
are a bit more similar on this point. So far I now use a constant value
assigned in the driver for both, as I believe there is no way the number
of GPIOs could be a different. Yet I can easily switch back to a value
provided by a device property.

Thanks again for your review.
Mathieu

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

Thu, Mar 13, 2025 at 06:07:03PM +0100, Mathieu Dubois-Briand kirjoitti:
> On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote:
> > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote:
> > > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.

...

> > > +	/*
> > > +	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
> > > +	 * timings and gpos/keypad columns repartition. Only the later is
> > > +	 * modified here.
> > > +	 */
> > > +	val = FIELD_PREP(MAX7360_PORTS, ngpios);
> > > +	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to write max7360 columns/gpos configuration");
> > > +		return ret;
> > > +	}
> >
> > Shouldn't this be configured via ->set_config() callback?
> 
> I believe this comment has been a bit outdated by our discussion on
> using GPIO valid mask, but I believe we could not use the ->set_config()
> callback here: this callback is made to configure a single pin while the
> gpos/keypad columns repartition is global.

Yeah, we have similar desing in Intel Bay Trail (see pinctrl-baytrail.c) and it
requires some software driven heuristics on how individual setting may affect
the global one. But the Q here is is the debounce affects only keypad? Then it
should be configured via keypad matrix driver. Btw, have you checked
drivers/input/keyboard/matrix_keypad.c? Is there anything that can be useful
here?

...

> > > +		if (irq < 0)
> > > +			return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> > > +
> > > +		irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
> > > +		if (!irq_chip)
> > > +			return -ENOMEM;
> > > +
> > > +		irq_chip->name = dev_name(dev);
> > > +		irq_chip->status_base = MAX7360_REG_GPIOIN;
> > > +		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->status_is_level = true;
> > > +		irq_chip->irq_drv_data = regmap;
> > > +
> > > +		for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) {
> > > +			regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
> > > +					  MAX7360_PORT_CFG_INTERRUPT_EDGES,
> > > +					  MAX7360_PORT_CFG_INTERRUPT_EDGES);
> > > +		}
> > > +
> > > +		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
> > > +		ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0,
> > > +						      irq_chip, &irq_chip_data);
> >
> > Right.
> >
> > What I mean in previous discussion is to update gpio-regmap to call this from inside.
> > You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration
> > and if they are set (or the first one, I dunno if this is supported by IRQ chip core)
> > call this function and assign domain. This should be called after GPIO chip is
> > added, but before IRQ domain attachment.
> >
> 
> Ok, this is a bit more clear to me now. So I came up with something, it
> will be part of the next iteration, probably during the next week.
> 
> This required to add a few additional fields to the gpio_regmap_config
> structure, specifying the IRQ configuration:
> 
> + * @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.
> + * @regmap_irq_irqno   (Optional) The IRQ the device uses to signal interrupts.
> + * @regmap_irq_flags   (Optional) The IRQF_ flags to use for the interrupt.

Okay, just make sure it's guarded by the same ifdeffery as the similar in the
GPIO:

#ifdef CONFIG_GPIOLIB_IRQCHIP

...

> > > +
> > > +		regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
> > > +	}
> > > +
> > > +	/* Add gpio device. */
> > > +	gpio_config.parent = dev;
> > > +	gpio_config.regmap = regmap;
> >
> > > +	if (gpio_function == MAX7360_GPIO_PORT) {
> > > +		gpio_config.ngpio = MAX7360_MAX_GPIO;
> >
> > Why this case can't be managed also via ngpios property? Maybe at the end of
> > the day you rather need to have another property to tell where the split is?
> >
> > This will help a lot and removes unneeded sharing of ngpios here and there.
> >
> > What I read from this code is like you are trying to put _two_in_one_ semantics
> > on the shoulders of "ngpios".
> 
> So as I reworked the keypad columns GPIOs, PORT GPIOs and the COL GPIOs
> are a bit more similar on this point. So far I now use a constant value
> assigned in the driver for both, as I believe there is no way the number
> of GPIOs could be a different. Yet I can easily switch back to a value
> provided by a device property.

Sounds good as long as ngpios is not overloaded with the additional meanings.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Fri Mar 14, 2025 at 9:14 AM CET, Andy Shevchenko wrote:
> Thu, Mar 13, 2025 at 06:07:03PM +0100, Mathieu Dubois-Briand kirjoitti:
> > On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote:
> > > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote:
> > > > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.
>
> ...
>
> > > > +	/*
> > > > +	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
> > > > +	 * timings and gpos/keypad columns repartition. Only the later is
> > > > +	 * modified here.
> > > > +	 */
> > > > +	val = FIELD_PREP(MAX7360_PORTS, ngpios);
> > > > +	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to write max7360 columns/gpos configuration");
> > > > +		return ret;
> > > > +	}
> > >
> > > Shouldn't this be configured via ->set_config() callback?
> > 
> > I believe this comment has been a bit outdated by our discussion on
> > using GPIO valid mask, but I believe we could not use the ->set_config()
> > callback here: this callback is made to configure a single pin while the
> > gpos/keypad columns repartition is global.
>
> Yeah, we have similar desing in Intel Bay Trail (see pinctrl-baytrail.c) and it
> requires some software driven heuristics on how individual setting may affect
> the global one. But the Q here is is the debounce affects only keypad? Then it
> should be configured via keypad matrix driver. Btw, have you checked
> drivers/input/keyboard/matrix_keypad.c? Is there anything that can be useful
> here?
>

Hum, maybe the comment is not clear enough? Not sure, but please tell
me.

So yes, this register is named "debounce" but controls two different
things:
- The keypad debounce: we do not touch it here.
- The partition between keypad columns and gpos. This is the value we do
  modify here.

I've been looking at drivers/input/keyboard/matrix_keypad.c, but I
believe the idea is completely different: it allows to use GPIOs to
create a keypad matrix, without the help of any controller. Here we have
a controller dedicated to keypad matrix, allowing to repurpose unused
columns as GPIOs. So except for some device tree similar properties and
input events, I believe there there isn't much we can reuse.

> ...
>
> > > > +		if (irq < 0)
> > > > +			return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> > > > +
> > > > +		irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
> > > > +		if (!irq_chip)
> > > > +			return -ENOMEM;
> > > > +
> > > > +		irq_chip->name = dev_name(dev);
> > > > +		irq_chip->status_base = MAX7360_REG_GPIOIN;
> > > > +		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->status_is_level = true;
> > > > +		irq_chip->irq_drv_data = regmap;
> > > > +
> > > > +		for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) {
> > > > +			regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
> > > > +					  MAX7360_PORT_CFG_INTERRUPT_EDGES,
> > > > +					  MAX7360_PORT_CFG_INTERRUPT_EDGES);
> > > > +		}
> > > > +
> > > > +		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
> > > > +		ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0,
> > > > +						      irq_chip, &irq_chip_data);
> > >
> > > Right.
> > >
> > > What I mean in previous discussion is to update gpio-regmap to call this from inside.
> > > You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration
> > > and if they are set (or the first one, I dunno if this is supported by IRQ chip core)
> > > call this function and assign domain. This should be called after GPIO chip is
> > > added, but before IRQ domain attachment.
> > >
> > 
> > Ok, this is a bit more clear to me now. So I came up with something, it
> > will be part of the next iteration, probably during the next week.
> > 
> > This required to add a few additional fields to the gpio_regmap_config
> > structure, specifying the IRQ configuration:
> > 
> > + * @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.
> > + * @regmap_irq_irqno   (Optional) The IRQ the device uses to signal interrupts.
> > + * @regmap_irq_flags   (Optional) The IRQF_ flags to use for the interrupt.
>
> Okay, just make sure it's guarded by the same ifdeffery as the similar in the
> GPIO:
>
> #ifdef CONFIG_GPIOLIB_IRQCHIP
>

Thanks!

> ...
>
> > > > +
> > > > +		regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
> > > > +	}
> > > > +
> > > > +	/* Add gpio device. */
> > > > +	gpio_config.parent = dev;
> > > > +	gpio_config.regmap = regmap;
> > >
> > > > +	if (gpio_function == MAX7360_GPIO_PORT) {
> > > > +		gpio_config.ngpio = MAX7360_MAX_GPIO;
> > >
> > > Why this case can't be managed also via ngpios property? Maybe at the end of
> > > the day you rather need to have another property to tell where the split is?
> > >
> > > This will help a lot and removes unneeded sharing of ngpios here and there.
> > >
> > > What I read from this code is like you are trying to put _two_in_one_ semantics
> > > on the shoulders of "ngpios".
> > 
> > So as I reworked the keypad columns GPIOs, PORT GPIOs and the COL GPIOs
> > are a bit more similar on this point. So far I now use a constant value
> > assigned in the driver for both, as I believe there is no way the number
> > of GPIOs could be a different. Yet I can easily switch back to a value
> > provided by a device property.
>
> Sounds good as long as ngpios is not overloaded with the additional meanings.

Thanks again for your review.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 03:13:07PM +0100, Mathieu Dubois-Briand wrote:
> On Fri Mar 14, 2025 at 9:14 AM CET, Andy Shevchenko wrote:
> > Thu, Mar 13, 2025 at 06:07:03PM +0100, Mathieu Dubois-Briand kirjoitti:
> > > On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote:
> > > > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote:

...

> > > > > +	/*
> > > > > +	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
> > > > > +	 * timings and gpos/keypad columns repartition. Only the later is
> > > > > +	 * modified here.
> > > > > +	 */
> > > > > +	val = FIELD_PREP(MAX7360_PORTS, ngpios);
> > > > > +	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
> > > > > +	if (ret) {
> > > > > +		dev_err(dev, "Failed to write max7360 columns/gpos configuration");
> > > > > +		return ret;
> > > > > +	}
> > > >
> > > > Shouldn't this be configured via ->set_config() callback?
> > > 
> > > I believe this comment has been a bit outdated by our discussion on
> > > using GPIO valid mask, but I believe we could not use the ->set_config()
> > > callback here: this callback is made to configure a single pin while the
> > > gpos/keypad columns repartition is global.
> >
> > Yeah, we have similar desing in Intel Bay Trail (see pinctrl-baytrail.c) and it
> > requires some software driven heuristics on how individual setting may affect
> > the global one. But the Q here is is the debounce affects only keypad? Then it
> > should be configured via keypad matrix driver. Btw, have you checked
> > drivers/input/keyboard/matrix_keypad.c? Is there anything that can be useful
> > here?
> >
> 
> Hum, maybe the comment is not clear enough? Not sure, but please tell
> me.

I see it now, yes, the comment seems point too much attention on the register
(and hence its name) then content.

I would start this comment with something like:
"Configure which GPIOs will be used for keypad."

> So yes, this register is named "debounce" but controls two different
> things:
> - The keypad debounce: we do not touch it here.
> - The partition between keypad columns and gpos. This is the value we do
>   modify here.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 08/10] input: keyboard: Add support for MAX7360 keypad

[Mathieu Dubois-Briand]

Add driver for Maxim Integrated MAX7360 keypad controller, providing
support for up to 64 keys, with a matrix of 8 columns and 8 rows.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/input/keyboard/Kconfig          |  12 ++
 drivers/input/keyboard/Makefile         |   1 +
 drivers/input/keyboard/max7360-keypad.c | 282 ++++++++++++++++++++++++++++++++
 3 files changed, 295 insertions(+)

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 721ab69e84ac..bba029f65cfa 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -421,6 +421,18 @@ config KEYBOARD_MAX7359
 	  To compile this driver as a module, choose M here: the
 	  module will be called max7359_keypad.
 
+config KEYBOARD_MAX7360
+	tristate "Maxim MAX7360 Key Switch Controller"
+	select INPUT_MATRIXKMAP
+	depends on I2C
+	depends on MFD_MAX7360
+	help
+	  If you say yes here you get support for the keypad controller on the
+	  Maxim MAX7360 I/O Expander.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called max7360_keypad.
+
 config KEYBOARD_MPR121
 	tristate "Freescale MPR121 Touchkey"
 	depends on I2C
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 1e0721c30709..b49d32d4003d 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_KEYBOARD_LPC32XX)		+= lpc32xx-keys.o
 obj-$(CONFIG_KEYBOARD_MAPLE)		+= maple_keyb.o
 obj-$(CONFIG_KEYBOARD_MATRIX)		+= matrix_keypad.o
 obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
+obj-$(CONFIG_KEYBOARD_MAX7360)		+= max7360-keypad.o
 obj-$(CONFIG_KEYBOARD_MPR121)		+= mpr121_touchkey.o
 obj-$(CONFIG_KEYBOARD_MT6779)		+= mt6779-keypad.o
 obj-$(CONFIG_KEYBOARD_MTK_PMIC) 	+= mtk-pmic-keys.o
diff --git a/drivers/input/keyboard/max7360-keypad.c b/drivers/input/keyboard/max7360-keypad.c
new file mode 100644
index 000000000000..da7a0faea873
--- /dev/null
+++ b/drivers/input/keyboard/max7360-keypad.c
@@ -0,0 +1,282 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Bootlin
+ *
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/max7360.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+struct max7360_keypad {
+	struct input_dev *input;
+	unsigned int rows;
+	unsigned int cols;
+	unsigned int debounce_ms;
+	int irq;
+	struct regmap *regmap;
+	unsigned short keycodes[MAX7360_MAX_KEY_ROWS * MAX7360_MAX_KEY_COLS];
+};
+
+static irqreturn_t max7360_keypad_irq(int irq, void *data)
+{
+	struct max7360_keypad *max7360_keypad = data;
+	unsigned int val;
+	unsigned int row, col;
+	unsigned int release;
+	unsigned int code;
+	int error;
+
+	do {
+		error = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO,
+				    &val);
+		if (error) {
+			dev_err(&max7360_keypad->input->dev,
+				"Failed to read max7360 FIFO");
+			return IRQ_NONE;
+		}
+
+		/* FIFO overflow: ignore it and get next event. */
+		if (val == MAX7360_FIFO_OVERFLOW)
+			dev_warn(&max7360_keypad->input->dev,
+				 "max7360 FIFO overflow");
+	} while (val == MAX7360_FIFO_OVERFLOW);
+
+	if (val == MAX7360_FIFO_EMPTY) {
+		dev_dbg(&max7360_keypad->input->dev,
+			"Got a spurious interrupt");
+
+		return IRQ_NONE;
+	}
+
+	row = FIELD_GET(MAX7360_FIFO_ROW, val);
+	col = FIELD_GET(MAX7360_FIFO_COL, val);
+	release = val & MAX7360_FIFO_RELEASE;
+
+	code = MATRIX_SCAN_CODE(row, col, MAX7360_ROW_SHIFT);
+
+	dev_dbg(&max7360_keypad->input->dev,
+		"key[%d:%d] %s\n", row, col, release ? "release" : "press");
+
+	input_event(max7360_keypad->input, EV_MSC, MSC_SCAN, code);
+	input_report_key(max7360_keypad->input, max7360_keypad->keycodes[code],
+			 !release);
+	input_sync(max7360_keypad->input);
+
+	return IRQ_HANDLED;
+}
+
+static int max7360_keypad_open(struct input_dev *pdev)
+{
+	struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
+	int error;
+
+	/*
+	 * Somebody is using the device: get out of sleep.
+	 */
+	error = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG,
+				  MAX7360_CFG_SLEEP, MAX7360_CFG_SLEEP);
+	if (error) {
+		dev_err(&max7360_keypad->input->dev,
+			"Failed to write max7360 configuration\n");
+		return error;
+	}
+
+	return 0;
+}
+
+static void max7360_keypad_close(struct input_dev *pdev)
+{
+	struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
+	int error;
+
+	/*
+	 * Nobody is using the device anymore: go to sleep.
+	 */
+	error = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG, MAX7360_CFG_SLEEP, 0);
+	if (error)
+		dev_err(&max7360_keypad->input->dev,
+			"Failed to write max7360 configuration\n");
+}
+
+static int max7360_keypad_hw_init(struct max7360_keypad *max7360_keypad)
+{
+	unsigned int val;
+	int error;
+
+	val = max7360_keypad->debounce_ms - MAX7360_DEBOUNCE_MIN;
+	error = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_DEBOUNCE,
+				  MAX7360_DEBOUNCE,
+				  FIELD_PREP(MAX7360_DEBOUNCE, val));
+	if (error) {
+		return dev_err_probe(&max7360_keypad->input->dev, error,
+			"Failed to write max7360 debounce configuration\n");
+	}
+
+	error = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_INTERRUPT,
+				  MAX7360_INTERRUPT_TIME_MASK,
+				  FIELD_PREP(MAX7360_INTERRUPT_TIME_MASK, 1));
+	if (error) {
+		return dev_err_probe(&max7360_keypad->input->dev, error,
+			"Failed to write max7360 keypad interrupt configuration\n");
+	}
+
+	return 0;
+}
+
+static int max7360_keypad_parse_dt(struct platform_device *pdev,
+				   struct max7360_keypad *max7360_keypad,
+				   bool *autorepeat)
+{
+	int error;
+
+	error = matrix_keypad_parse_properties(pdev->dev.parent,
+					       &max7360_keypad->rows,
+					       &max7360_keypad->cols);
+	if (error)
+		return error;
+
+	if (!max7360_keypad->rows || !max7360_keypad->cols ||
+	    max7360_keypad->rows > MAX7360_MAX_KEY_ROWS ||
+	    max7360_keypad->cols > MAX7360_MAX_KEY_COLS) {
+		dev_err(&pdev->dev,
+			"Invalid number of columns or rows (%ux%u)\n",
+			max7360_keypad->cols, max7360_keypad->rows);
+		return -EINVAL;
+	}
+
+	*autorepeat = device_property_read_bool(pdev->dev.parent, "autorepeat");
+
+	max7360_keypad->debounce_ms = MAX7360_DEBOUNCE_MIN;
+	error = device_property_read_u32(pdev->dev.parent,
+					 "keypad-debounce-delay-ms",
+					 &max7360_keypad->debounce_ms);
+	if (error == -EINVAL) {
+		dev_info(&pdev->dev, "Using default keypad-debounce-delay-ms: %u\n",
+			 max7360_keypad->debounce_ms);
+	} else if (error < 0) {
+		dev_err(&pdev->dev,
+			"Failed to read keypad-debounce-delay-ms property\n");
+		return error;
+	} else if (max7360_keypad->debounce_ms < MAX7360_DEBOUNCE_MIN ||
+		   max7360_keypad->debounce_ms > MAX7360_DEBOUNCE_MAX) {
+		dev_err(&pdev->dev,
+			"Invalid keypad-debounce-delay-ms: %u, should be between %u and %u.\n",
+			max7360_keypad->debounce_ms, MAX7360_DEBOUNCE_MIN,
+			MAX7360_DEBOUNCE_MAX);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int max7360_keypad_probe(struct platform_device *pdev)
+{
+	struct max7360_keypad *max7360_keypad;
+	struct input_dev *input;
+	bool autorepeat;
+	int error;
+	int irq;
+
+	if (!pdev->dev.parent)
+		return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n");
+
+	irq = platform_get_irq_byname(to_platform_device(pdev->dev.parent),
+				      "intk");
+	if (irq < 0)
+		return irq;
+
+	max7360_keypad = devm_kzalloc(&pdev->dev, sizeof(*max7360_keypad),
+				      GFP_KERNEL);
+	if (!max7360_keypad)
+		return -ENOMEM;
+
+	max7360_keypad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!max7360_keypad->regmap)
+		return dev_err_probe(&pdev->dev, -ENODEV,
+				     "Could not get parent regmap\n");
+
+	error = max7360_keypad_parse_dt(pdev, max7360_keypad, &autorepeat);
+	if (error)
+		return error;
+
+	input = devm_input_allocate_device(pdev->dev.parent);
+	if (!input)
+		return -ENOMEM;
+
+	max7360_keypad->input = input;
+
+	input->id.bustype = BUS_I2C;
+	input->name = pdev->name;
+	input->open = max7360_keypad_open;
+	input->close = max7360_keypad_close;
+
+	error = matrix_keypad_build_keymap(NULL, NULL,
+					   MAX7360_MAX_KEY_ROWS,
+					   MAX7360_MAX_KEY_COLS,
+					   max7360_keypad->keycodes, input);
+	if (error)
+		return dev_err_probe(&pdev->dev, error,
+				     "Failed to build keymap\n");
+
+	input_set_capability(input, EV_MSC, MSC_SCAN);
+	if (autorepeat)
+		__set_bit(EV_REP, input->evbit);
+
+	input_set_drvdata(input, max7360_keypad);
+
+	error = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					  max7360_keypad_irq,
+					  IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+					  "max7360-keypad", max7360_keypad);
+	if (error)
+		return dev_err_probe(&pdev->dev, error,
+				     "Failed to register interrupt\n");
+
+	error = input_register_device(input);
+	if (error)
+		return dev_err_probe(&pdev->dev, error,
+				     "Could not register input device\n");
+
+	platform_set_drvdata(pdev, max7360_keypad);
+
+	error = max7360_keypad_hw_init(max7360_keypad);
+	if (error)
+		return dev_err_probe(&pdev->dev, error,
+				     "Failed to initialize max7360 keypad\n");
+
+	device_init_wakeup(&pdev->dev, true);
+	error = dev_pm_set_wake_irq(&pdev->dev, irq);
+	if (error)
+		dev_warn(&pdev->dev, "Failed to set up wakeup irq: %d\n",
+			 error);
+
+	return 0;
+}
+
+static void max7360_keypad_remove(struct platform_device *pdev)
+{
+	dev_pm_clear_wake_irq(&pdev->dev);
+}
+
+static struct platform_driver max7360_keypad_driver = {
+	.driver = {
+		.name	= "max7360-keypad",
+	},
+	.probe		= max7360_keypad_probe,
+	.remove		= max7360_keypad_remove,
+};
+module_platform_driver(max7360_keypad_driver);
+
+MODULE_DESCRIPTION("MAX7360 Keypad driver");
+MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_LICENSE("GPL");

-- 
2.39.5

[PATCH v4 09/10] input: misc: Add support for MAX7360 rotary

[Mathieu Dubois-Briand]

Add driver for Maxim Integrated MAX7360 rotary encoder controller,
supporting a single rotary switch.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/input/misc/Kconfig          |  11 +++
 drivers/input/misc/Makefile         |   1 +
 drivers/input/misc/max7360-rotary.c | 182 ++++++++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+)

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 13d135257e06..96b183b4d3a5 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -230,6 +230,17 @@ config INPUT_M68K_BEEP
 	tristate "M68k Beeper support"
 	depends on M68K
 
+config INPUT_MAX7360_ROTARY
+	tristate "Maxim MAX7360 Rotary Encoder"
+	depends on I2C
+	depends on MFD_MAX7360
+	help
+	  If you say yes here you get support for the rotary encoder on the
+	  Maxim MAX7360 I/O Expander.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called max7360_rotary.
+
 config INPUT_MAX77650_ONKEY
 	tristate "Maxim MAX77650 ONKEY support"
 	depends on MFD_MAX77650
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 6d91804d0a6f..c454fba3a3ae 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_INPUT_IQS7222)		+= iqs7222.o
 obj-$(CONFIG_INPUT_KEYSPAN_REMOTE)	+= keyspan_remote.o
 obj-$(CONFIG_INPUT_KXTJ9)		+= kxtj9.o
 obj-$(CONFIG_INPUT_M68K_BEEP)		+= m68kspkr.o
+obj-$(CONFIG_INPUT_MAX7360_ROTARY)	+= max7360-rotary.o
 obj-$(CONFIG_INPUT_MAX77650_ONKEY)	+= max77650-onkey.o
 obj-$(CONFIG_INPUT_MAX77693_HAPTIC)	+= max77693-haptic.o
 obj-$(CONFIG_INPUT_MAX8925_ONKEY)	+= max8925_onkey.o
diff --git a/drivers/input/misc/max7360-rotary.c b/drivers/input/misc/max7360-rotary.c
new file mode 100644
index 000000000000..bea25203a5c1
--- /dev/null
+++ b/drivers/input/misc/max7360-rotary.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Bootlin
+ *
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/max7360.h>
+#include <linux/property.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+struct max7360_rotary {
+	u32 axis;
+	struct input_dev *input;
+	unsigned int debounce_ms;
+	struct regmap *regmap;
+};
+
+static irqreturn_t max7360_rotary_irq(int irq, void *data)
+{
+	struct max7360_rotary *max7360_rotary = data;
+	int val;
+	int ret;
+
+	ret = regmap_read(max7360_rotary->regmap, MAX7360_REG_RTR_CNT, &val);
+	if (ret < 0) {
+		dev_err(&max7360_rotary->input->dev,
+			"Failed to read rotary counter\n");
+		return IRQ_NONE;
+	}
+
+	if (val == 0) {
+		dev_dbg(&max7360_rotary->input->dev,
+			"Got a spurious interrupt\n");
+
+		return IRQ_NONE;
+	}
+
+	input_report_rel(max7360_rotary->input, max7360_rotary->axis,
+			 (int8_t)val);
+	input_sync(max7360_rotary->input);
+
+	return IRQ_HANDLED;
+}
+
+static int max7360_rotary_hw_init(struct max7360_rotary *max7360_rotary)
+{
+	int val;
+	int ret;
+
+	ret = regmap_write_bits(max7360_rotary->regmap, MAX7360_REG_GPIOCFG,
+				MAX7360_GPIO_CFG_RTR_EN,
+				MAX7360_GPIO_CFG_RTR_EN);
+	if (ret) {
+		dev_err(&max7360_rotary->input->dev,
+			"Failed to enable max7360 rotary encoder\n");
+		return ret;
+	}
+
+	val = FIELD_PREP(MAX7360_ROT_DEBOUNCE, max7360_rotary->debounce_ms) |
+		FIELD_PREP(MAX7360_ROT_INTCNT, 1) | MAX7360_ROT_INTCNT_DLY;
+	ret = regmap_write(max7360_rotary->regmap, MAX7360_REG_RTRCFG, val);
+	if (ret) {
+		dev_err(&max7360_rotary->input->dev,
+			"Failed to set max7360 rotary encoder configuration\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max7360_rotary_probe(struct platform_device *pdev)
+{
+	struct max7360_rotary *max7360_rotary;
+	struct input_dev *input;
+	int irq;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n");
+
+	ret = max7360_port_pin_request(pdev->dev.parent, MAX7360_PORT_RTR_PIN,
+				       true);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Could not request rotary pin\n");
+
+	irq = platform_get_irq_byname(to_platform_device(pdev->dev.parent),
+				      "inti");
+	if (irq < 0)
+		return irq;
+
+	max7360_rotary = devm_kzalloc(&pdev->dev, sizeof(*max7360_rotary),
+				      GFP_KERNEL);
+	if (!max7360_rotary)
+		return -ENOMEM;
+
+	max7360_rotary->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!max7360_rotary->regmap)
+		dev_err_probe(&pdev->dev, -ENODEV,
+			      "Could not get parent regmap\n");
+
+	device_property_read_u32(pdev->dev.parent, "linux,axis",
+				 &max7360_rotary->axis);
+	device_property_read_u32(pdev->dev.parent, "rotary-debounce-delay-ms",
+				 &max7360_rotary->debounce_ms);
+	if (max7360_rotary->debounce_ms > MAX7360_ROT_DEBOUNCE_MAX)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "Invalid debounce timing: %u\n",
+				     max7360_rotary->debounce_ms);
+
+	input = devm_input_allocate_device(&pdev->dev);
+	if (!input)
+		return -ENOMEM;
+
+	max7360_rotary->input = input;
+
+	input->id.bustype = BUS_I2C;
+	input->name = pdev->name;
+	input->dev.parent = &pdev->dev;
+
+	input_set_capability(input, EV_REL, max7360_rotary->axis);
+	input_set_drvdata(input, max7360_rotary);
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					max7360_rotary_irq,
+					IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED,
+					"max7360-rotary", max7360_rotary);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to register interrupt\n");
+
+	ret = input_register_device(input);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Could not register input device\n");
+
+	platform_set_drvdata(pdev, max7360_rotary);
+
+	device_init_wakeup(&pdev->dev, true);
+	ret = dev_pm_set_wake_irq(&pdev->dev, irq);
+	if (ret)
+		dev_warn(&pdev->dev, "Failed to set up wakeup irq: %d\n", ret);
+
+	ret = max7360_rotary_hw_init(max7360_rotary);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to initialize max7360 rotary\n");
+
+	return 0;
+}
+
+static void max7360_rotary_remove(struct platform_device *pdev)
+{
+	dev_pm_clear_wake_irq(&pdev->dev);
+
+	/*
+	 * Free the MAX7360_PORT_RTR_PIN pin, so it can be requested later by
+	 * this driver, the MAX7360 GPIO driver or the MAX7360 PWM driver.
+	 */
+	max7360_port_pin_request(pdev->dev.parent, MAX7360_PORT_RTR_PIN, false);
+}
+
+static struct platform_driver max7360_rotary_driver = {
+	.driver = {
+		.name	= "max7360-rotary",
+	},
+	.probe		= max7360_rotary_probe,
+	.remove		= max7360_rotary_remove,
+};
+module_platform_driver(max7360_rotary_driver);
+
+MODULE_DESCRIPTION("MAX7360 Rotary driver");
+MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_LICENSE("GPL");

-- 
2.39.5

[PATCH v4 10/10] MAINTAINERS: Add entry on MAX7360 driver

[Mathieu Dubois-Briand]

Add myself as maintainer of Maxim MAX7360 driver and device-tree bindings.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 MAINTAINERS | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 25c86f47353d..1916b41309f0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14259,6 +14259,18 @@ L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	drivers/iio/temperature/max30208.c
 
+MAXIM MAX7360 KEYPAD LED MFD DRIVER
+M:	Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml
+F:	Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
+F:	drivers/gpio/gpio-max7360.c
+F:	drivers/input/keyboard/max7360-keypad.c
+F:	drivers/input/misc/max7360-rotary.c
+F:	drivers/mfd/max7360.c
+F:	drivers/pwm/pwm-max7360.c
+F:	include/linux/mfd/max7360.h
+
 MAXIM MAX77650 PMIC MFD DRIVER
 M:	Bartosz Golaszewski <brgl@bgdev.pl>
 L:	linux-kernel@vger.kernel.org

-- 
2.39.5