diff mbox series

[v5,02/11] mfd: Add max7360 support

Message ID 20250318-mdb-max7360-support-v5-2-fb20baf97da0@bootlin.com (mailing list archive)
State New
Headers show
Series Add support for MAX7360 | expand

Commit Message

Mathieu Dubois-Briand March 18, 2025, 4:26 p.m. UTC
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       | 185 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/max7360.h | 112 +++++++++++++++++++++++++++
 4 files changed, 312 insertions(+)

Comments

Andy Shevchenko March 19, 2025, 11:10 a.m. UTC | #1
On Tue, Mar 18, 2025 at 05:26:18PM +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.

...

+ array_size.h

> +#include <linux/bits.h>
> +#include <linux/delay.h>

> +#include <linux/device.h>

Since it won;t make v6.15-rc1 anyway the above can be better specified as

device/devres.h
dev_printk.h

as device.h is monstrous.

+ err.h

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

+ mod_devicetable.h

> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>

...

> +	ret = regmap_write(regmap, MAX7360_REG_GPIOCFG,
> +			   MAX7360_GPIO_CFG_GPIO_RST);

I would suggest to leave it as a single line. In this and similar cases
when it is ~83 characters, it's still fine (and even for strict 80 there is
a documented exception)

...

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

Just

	return ret;

?

> +	ret = max7360_mask_irqs(regmap);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Could not mask interrupts\n");

Hmm... As far as I can read this masks GPIO interrups. Does it do anything
else? If it's covered by the GPIO/pin control drivers, one want probably to
see that to be done there in the respective callback (init_hw_irq or alike,
I don't remember the name by heart).

...

> +#ifndef __LINUX_MFD_MAX7360_H
> +#define __LINUX_MFD_MAX7360_H
> +
> +#include <linux/bits.h>

> +#include <linux/types.h>

Do you need types.h here? I don't see for what...

> +struct device;

Neither this. Perhaps it's for the following changes? Then add when required,
not now.

> +#endif
Mathieu Dubois-Briand March 25, 2025, 4:26 p.m. UTC | #2
On Wed Mar 19, 2025 at 12:10 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:18PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> > From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > +	ret = max7360_mask_irqs(regmap);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Could not mask interrupts\n");
>
> Hmm... As far as I can read this masks GPIO interrups. Does it do anything
> else? If it's covered by the GPIO/pin control drivers, one want probably to
> see that to be done there in the respective callback (init_hw_irq or alike,
> I don't remember the name by heart).
>

Hum, I'm not sure I can do that.

So the "inti" interrupt line is shared across the GPIO and the rotary
encoder functionalities.

On reset, GPIO interrupts are not masked. This means, if we do the
masking in the GPIO driver and the GPIO driver is not loaded but the
rotary encoder driver is, the rotary encoder driver might get a lot of
spurious interrupts.

So I believe it makes sense to mask the interrupts here, setting the
chip in a sane configuration, whatever child drivers are present.

Any thought about that?
Andy Shevchenko March 25, 2025, 4:40 p.m. UTC | #3
On Tue, Mar 25, 2025 at 05:26:12PM +0100, Mathieu Dubois-Briand wrote:
> On Wed Mar 19, 2025 at 12:10 PM CET, Andy Shevchenko wrote:
> > On Tue, Mar 18, 2025 at 05:26:18PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> > > From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > +	ret = max7360_mask_irqs(regmap);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "Could not mask interrupts\n");
> >
> > Hmm... As far as I can read this masks GPIO interrups. Does it do anything
> > else? If it's covered by the GPIO/pin control drivers, one want probably to
> > see that to be done there in the respective callback (init_hw_irq or alike,
> > I don't remember the name by heart).
> 
> Hum, I'm not sure I can do that.
> 
> So the "inti" interrupt line is shared across the GPIO and the rotary
> encoder functionalities.
> 
> On reset, GPIO interrupts are not masked. This means, if we do the
> masking in the GPIO driver and the GPIO driver is not loaded but the
> rotary encoder driver is, the rotary encoder driver might get a lot of
> spurious interrupts.
> 
> So I believe it makes sense to mask the interrupts here, setting the
> chip in a sane configuration, whatever child drivers are present.
> 
> Any thought about that?

Okay, this makes sense. I forgot if you have any comment in the code
(probably not if I asked the question), but in any case the above can
be added on top of the function explaining this.
diff mbox series

Patch

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..9f489a798f94
--- /dev/null
+++ b/drivers/mfd/max7360.c
@@ -0,0 +1,185 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Maxim MAX7360 Core Driver
+ *
+ * Copyright 2025 Bootlin
+ *
+ * Author: Kamel Bouhara <kamel.bouhara@bootlin.com>
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/device.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/regmap.h>
+#include <linux/types.h>
+
+static const struct mfd_cell max7360_cells[] = {
+	{
+		.name           = "max7360-pinctrl",
+	},
+	{
+		.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 = MAX7360_REG_I2C_TIMEOUT,
+		.range_max = MAX7360_REG_RTR_CNT,
+	},
+};
+
+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 = MAX7360_REG_PWMCFG(MAX7360_PORT_PWM_COUNT - 1),
+	.volatile_table = &max7360_volatile_table,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int max7360_mask_irqs(struct regmap *regmap)
+{
+	struct device *dev = regmap_get_device(regmap);
+	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 (unsigned int i = 0; i < MAX7360_PORT_PWM_COUNT; i++) {
+		ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
+					MAX7360_PORT_CFG_INTERRUPT_MASK,
+					MAX7360_PORT_CFG_INTERRUPT_MASK);
+		if (ret) {
+			dev_err(dev, "Failed to write max7360 port configuration");
+			return ret;
+		}
+	}
+
+	/* Read GPIO in register, to ACK any pending IRQ. */
+	ret = regmap_read(regmap, MAX7360_REG_GPIOIN, &val);
+	if (ret)
+		dev_err(dev, "Failed to read gpio values: %d\n", ret);
+
+	return ret;
+}
+
+static int max7360_reset(struct regmap *regmap)
+{
+	struct device *dev = regmap_get_device(regmap);
+	int ret;
+
+	ret = regmap_write(regmap, MAX7360_REG_GPIOCFG,
+			   MAX7360_GPIO_CFG_GPIO_RST);
+	if (ret) {
+		dev_err(dev, "Failed to reset GPIO configuration: %x\n", ret);
+		return ret;
+	}
+
+	ret = regcache_drop_region(regmap, MAX7360_REG_GPIOCFG,
+				   MAX7360_REG_GPIO_LAST);
+	if (ret) {
+		dev_err(dev, "Failed to drop regmap cache: %x\n", ret);
+		return ret;
+	}
+
+	ret = regmap_write(regmap, MAX7360_REG_SLEEP, 0);
+	if (ret) {
+		dev_err(dev, "Failed to reset autosleep configuration: %x\n", ret);
+		return ret;
+	}
+
+	ret = regmap_write(regmap, MAX7360_REG_DEBOUNCE, 0);
+	if (ret) {
+		dev_err(dev, "Failed to reset GPO port count: %x\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max7360_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct regmap *regmap;
+	int ret;
+
+	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");
+
+	i2c_set_clientdata(client, regmap);
+
+	ret = max7360_reset(regmap);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to reset device\n");
+
+	/* Get the device out of shutdown mode. */
+	ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG,
+				MAX7360_GPIO_CFG_GPIO_EN,
+				MAX7360_GPIO_CFG_GPIO_EN);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable GPIO and PWM module\n");
+
+	ret = max7360_mask_irqs(regmap);
+	if (ret)
+		return dev_err_probe(dev, ret, "Could not mask interrupts\n");
+
+	ret =  devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+				    max7360_cells, ARRAY_SIZE(max7360_cells),
+				    NULL, 0, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret, "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..c5a90fa5b76b
--- /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/bits.h>
+#include <linux/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_I2C_TIMEOUT		0x48
+#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_PORT_CFG_COMMON_PWM		BIT(5)
+
+/*
+ * Autosleep register values
+ */
+#define MAX7360_AUTOSLEEP_8192MS	0x01
+#define MAX7360_AUTOSLEEP_4096MS	0x02
+#define MAX7360_AUTOSLEEP_2048MS	0x03
+#define MAX7360_AUTOSLEEP_1024MS	0x04
+#define MAX7360_AUTOSLEEP_512MS		0x05
+#define MAX7360_AUTOSLEEP_256MS		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
+
+#endif