On Fri, Feb 14, 2025 at 12:49:52PM +0100, 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.
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@xxxxxxxxxxx>
> + */
...
+ 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
Re: [PATCH v4 02/10] mfd: Add max7360 support
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
- Subject: Re: [PATCH v4 02/10] mfd: Add max7360 support
- From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
- Date: Tue, 18 Feb 2025 12:09:42 +0200
- In-reply-to: <20250214-mdb-max7360-support-v4-2-8a35c6dbb966@bootlin.com>
- Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
- References: <20250214-mdb-max7360-support-v4-0-8a35c6dbb966@bootlin.com> <20250214-mdb-max7360-support-v4-2-8a35c6dbb966@bootlin.com>
- References:
- [PATCH v4 00/10] Add support for MAX7360
- From: Mathieu Dubois-Briand
- [PATCH v4 02/10] mfd: Add max7360 support
- From: mathieu . dubois-briand
- [PATCH v4 00/10] Add support for MAX7360
- Prev by Date: Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC
- Next by Date: Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC
- Previous by thread: [PATCH v4 02/10] mfd: Add max7360 support
- Next by thread: [PATCH v4 06/10] regmap: irq: Add support for chips without separate IRQ status
- Index(es):
 |