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

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 [off-list ref]
+ */

...

+ 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