Re: [PATCH v3 2/7] mfd: Add max7360 support

[Lee Jones <lee@kernel.org>]

On Mon, 13 Jan 2025, 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.
> 
> 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         |  12 ++
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/max7360.c       | 296 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max7360.h | 109 ++++++++++++++++
>  4 files changed, 418 insertions(+)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ae23b317a64e..c1ddda480922 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2414,5 +2414,17 @@ config MFD_RSMU_SPI
>  	  Additional drivers must be enabled in order to use the functionality
>  	  of the device.
>  
> +config MFD_MAX7360
> +	tristate "Maxim MAX7360 Support"

What is it?

> +	depends on I2C
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	help
> +	  Say yes here to add support for Maxim MAX7360.

60 chars is an odd place to break.

> +	  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 e057d6d6faef..6cd55504106d 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..e2751b4f68b2
> --- /dev/null
> +++ b/drivers/mfd/max7360.c
> @@ -0,0 +1,296 @@
> +// 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_mfd {

Drop references to "mfd".

> +	struct regmap *regmap;
> +	unsigned int requested_ports;
> +	struct device *dev;

Pop dev at the top.

> +};
> +
> +#define GPO_COMPATIBLE "maxim,max7360-gpo"
> +#define GPIO_COMPATIBLE "maxim,max7360-gpio"

Please use the raw strings instead.

> +static const struct mfd_cell max7360_cells[] = {
> +	{
> +		.name           = MAX7360_DRVNAME_PWM,

Raw strings please.

> +	},
> +	{
> +		.name           = MAX7360_DRVNAME_GPO,
> +		.of_compatible	= GPO_COMPATIBLE,
> +	},
> +	{
> +		.name           = MAX7360_DRVNAME_GPIO,
> +		.of_compatible	= GPIO_COMPATIBLE,
> +	},
> +	{
> +		.name           = MAX7360_DRVNAME_KEYPAD,
> +	},
> +	{
> +		.name           = MAX7360_DRVNAME_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_RBTREE,

A lot of these are being replaced with REGCACHE_MAPLE.

Are you sure you shouldn't be using that too?

> +};
> +
> +static int max7360_set_gpos_count(struct max7360_mfd *max7360_mfd)

Pass the dev pointer around instead.

> +{
> +	/*
> +	 * Max7360 COL0 to COL7 pins can be used either as keypad columns,

MAX?

> +	 * general purpose output or a mix of both.
> +	 * Get the number of pins requested by the corresponding drivers, ensure
> +	 * they are compatible with each others and apply the corresponding
> +	 * configuration.
> +	 */

What are you documenting here?

Comments should go with their code snippets.

> +	struct device_node *np;

np usually implies our own node.

> +	u32 gpos = 0;
> +	u32 columns = 0;
> +	unsigned int val;
> +	int ret;
> +
> +	np = of_get_compatible_child(max7360_mfd->dev->of_node, GPO_COMPATIBLE);

Why don't you do all of this in the GPO driver?

> +	if (np) {
> +		ret = of_property_read_u32(np, "ngpios", &gpos);
> +		if (ret < 0) {
> +			dev_err(max7360_mfd->dev, "Failed to read gpos count\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = device_property_read_u32(max7360_mfd->dev,
> +				       "keypad,num-columns", &columns);
> +	if (ret < 0) {
> +		dev_err(max7360_mfd->dev, "Failed to read columns count\n");
> +		return ret;
> +	}
> +
> +	if (gpos > MAX7360_MAX_GPO ||
> +	    (gpos + columns > MAX7360_MAX_KEY_COLS)) {
> +		dev_err(max7360_mfd->dev,
> +			"Incompatible gpos and columns count (%u, %u)\n",
> +			gpos, 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, gpos);
> +	ret = regmap_write_bits(max7360_mfd->regmap, MAX7360_REG_DEBOUNCE,
> +				MAX7360_PORTS, val);
> +	if (ret) {
> +		dev_err(max7360_mfd->dev,
> +			"Failed to write max7360 columns/gpos configuration");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request)

This whole function is rough.  What are you trying to achieve?

> +{
> +	struct i2c_client *client;
> +	struct max7360_mfd *max7360_mfd;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	client = to_i2c_client(dev);
> +	max7360_mfd = i2c_get_clientdata(client);
> +
> +	spin_lock_irqsave(&request_lock, flags);
> +	if (request) {
> +		if (max7360_mfd->requested_ports & BIT(pin))
> +			ret = -EBUSY;
> +		else
> +			max7360_mfd->requested_ports |= BIT(pin);
> +	} else {
> +		max7360_mfd->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_mfd *max7360_mfd)
> +{
> +	unsigned int i;

Use `for (int i;` instead.

> +	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 (i = 0; i < MAX7360_PORT_PWM_COUNT; i++) {
> +		ret = regmap_write_bits(max7360_mfd->regmap,
> +					MAX7360_REG_PWMCFG + i,
> +					MAX7360_PORT_CFG_INTERRUPT_MASK,
> +					MAX7360_PORT_CFG_INTERRUPT_MASK);
> +		if (ret) {
> +			dev_err(max7360_mfd->dev,
> +				"failed to write max7360 port configuration");

You can use 100-chars to avoid the line break.

> +			return ret;
> +		}
> +	}
> +
> +	/* Read gpio in register, to ack any pending IRQ.

GPIO, ACK

> +	 */

Wrong format.

> +	ret = regmap_read(max7360_mfd->regmap, MAX7360_REG_GPIOIN, &val);
> +	if (ret) {
> +		dev_err(max7360_mfd->dev, "Failed to read gpio values: %d\n",

GPIO

> +			ret);

No wrap needed.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int max7360_reset(struct max7360_mfd *max7360_mfd)
> +{
> +	int err;
> +
> +	/*
> +	 * Set back the default values.
> +	 * We do not use GPIO reset function here, as it does not work reliably.

Why?  What's wrong with it?

> +	 */
> +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIODEB, 0x00);
> +	if (err) {
> +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");

Use a goto to only print this out once.

> +		return err;
> +	}
> +
> +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIOCURR, MAX7360_REG_GPIOCURR_FIXED);
> +	if (err) {
> +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
> +		return err;
> +	}
> +
> +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIOOUTM, 0x00);
> +	if (err) {
> +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
> +		return err;
> +	}
> +
> +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_PWMCOM, 0x00);
> +	if (err) {
> +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
> +		return err;
> +	}
> +
> +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_SLEEP, 0);
> +	if (err) {
> +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int max7360_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct regmap *regmap;
> +	struct max7360_mfd *max7360_mfd;
> +	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_mfd = devm_kzalloc(dev, sizeof(*max7360_mfd), GFP_KERNEL);
> +	if (!max7360_mfd)
> +		return -ENOMEM;
> +
> +	max7360_mfd->regmap = regmap;
> +	max7360_mfd->dev = dev;
> +	i2c_set_clientdata(client, max7360_mfd);
> +
> +	err = max7360_reset(max7360_mfd);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to reset device\n");
> +
> +	err = max7360_set_gpos_count(max7360_mfd);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to set GPOS pin count\n");
> +
> +	/*
> +	 * Get the device out of shutdown mode.
> +	 */

Single line comment please.

> +	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 set device out of shutdown\n");

This doesn't read well.

> +	err = max7360_mask_irqs(max7360_mfd);
> +	if (err)
> +		return dev_err_probe(dev, err, "could not mask interrupts\n");

Some messages start with an uppercase char, others do not.

I suggest you use one for consistency.

> +	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 MFD core driver");

There is no such thing as an MFD driver.  What does it do?

> +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..2665a8e6b0f0
> --- /dev/null
> +++ b/include/linux/mfd/max7360.h
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */

'\n'

> +#ifndef __LINUX_MFD_MAX7360_H
> +#define __LINUX_MFD_MAX7360_H

'\n'

> +#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)

'\n'

> +/*
> + * 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 registers
> + */

How is this different to the ones above?

> +#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_PWMCFG	0x58
> +
> +#define MAX7360_REG_PORTCFGBASE 0x58
> +
> +/*
> + * 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
> +
> +#define MAX7360_DRVNAME_PWM	"max7360-pwm"
> +#define MAX7360_DRVNAME_GPO	"max7360-gpo"
> +#define MAX7360_DRVNAME_GPIO	"max7360-gpio"
> +#define MAX7360_DRVNAME_KEYPAD	"max7360-keypad"
> +#define MAX7360_DRVNAME_ROTARY	"max7360-rotary"

Nope.

> +
> +int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request);
> +
> +#endif
> 
> -- 
> 2.39.5
> 

-- 
Lee Jones [李琼斯]