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

From: Lee Jones
Date: Wed Jan 15 2025 - 10:43:10 EST

Next message: Ulf Hansson: "Re: [PATCH 2/6] thermal: of: Export non-devres helper to register/unregister thermal zone"
Previous message: Jonathan Cameron: "Re: [PATCH 1/2] iio: adc: rzg2l_adc: Drop devm_pm_runtime_enable()"
In reply to: mathieu . dubois-briand: "[PATCH v3 2/7] mfd: Add max7360 support"
Next in thread: mathieu . dubois-briand: "[PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, 13 Jan 2025, mathieu.dubois-briand@xxxxxxxxxxx wrote:

> From: Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@xxxxxxxxxxx>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>
> + */
> +
> +#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@xxxxxxxxxxx>");
> +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 [李琼斯]

Next message: Ulf Hansson: "Re: [PATCH 2/6] thermal: of: Export non-devres helper to register/unregister thermal zone"
Previous message: Jonathan Cameron: "Re: [PATCH 1/2] iio: adc: rzg2l_adc: Drop devm_pm_runtime_enable()"
In reply to: mathieu . dubois-briand: "[PATCH v3 2/7] mfd: Add max7360 support"
Next in thread: mathieu . dubois-briand: "[PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]