On 10/9/18 11:55 AM, Lee Jones wrote:
> On Thu, 27 Sep 2018, Amelie Delaunay wrote:
>
>> STMicroelectronics Multi-Function eXpander (STMFX) is a slave controller
>> using I2C for communication with the main MCU. Main features are:
>> - 16 fast GPIOs individually configurable in input/output
>> - 8 alternate GPIOs individually configurable in input/output when other
>> STMFX functions are not used
>> - Main MCU IDD measurement
>> - Resistive touchscreen controller
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@xxxxxx>
>> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> ---
>> drivers/mfd/Kconfig | 14 ++
>> drivers/mfd/Makefile | 2 +-
>> drivers/mfd/stmfx.c | 626 ++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/stmfx.h | 27 ++
>> 4 files changed, 668 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/mfd/stmfx.c
>> create mode 100644 include/linux/mfd/stmfx.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index f3a5f8d..8c41342 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1862,6 +1862,20 @@ config MFD_STM32_TIMERS
>> for PWM and IIO Timer. This driver allow to share the
>> registers between the others drivers.
>>
>> +config MFD_STMFX
>> + tristate "Support for STMicroelectronics Multi-Function eXpander (STMFX)"
>> + depends on I2C
>> + depends on OF || COMPILE_TEST
>> + select MFD_CORE
>> + select REGMAP_I2C
>> + help
>> + Support for the STMicroelectronics Multi-Function eXpander.
>> +
>> + This driver provides common support for accessing the device,
>> + additional drivers must be enabled in order to use the functionality
>> + of the device.
>> +
>> +
>> menu "Multimedia Capabilities Port drivers"
>> depends on ARCH_SA1100
>>
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 5856a94..282323b 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -240,4 +240,4 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
>> obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
>> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
>> obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
>> -
>> +obj-$(CONFIG_MFD_STMFX) += stmfx.o
>> diff --git a/drivers/mfd/stmfx.c b/drivers/mfd/stmfx.c
>> new file mode 100644
>> index 0000000..cfd4fca
>> --- /dev/null
>> +++ b/drivers/mfd/stmfx.c
>> @@ -0,0 +1,626 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for STMicroelectronics Multi-Function eXpander (STMFX) core
>> + *
>> + * Copyright (C) 2018 STMicroelectronics
>> + * Author(s): Amelie Delaunay <amelie.delaunay@xxxxxx>.
>> + */
>> +#include <linux/bitfield.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/stmfx.h>
>> +#include <linux/module.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +/* General */
>> +#define STMFX_REG_CHIP_ID 0x00 /* R */
>> +#define STMFX_REG_FW_VERSION_MSB 0x01 /* R */
>> +#define STMFX_REG_FW_VERSION_LSB 0x02 /* R */
>> +#define STMFX_REG_SYS_CTRL 0x40 /* RW */
>> +/* IRQ output management */
>> +#define STMFX_REG_IRQ_OUT_PIN 0x41 /* RW */
>> +#define STMFX_REG_IRQ_SRC_EN 0x42 /* RW */
>> +#define STMFX_REG_IRQ_PENDING 0x08 /* R */
>> +#define STMFX_REG_IRQ_ACK 0x44 /* RW */
>> +/* GPIO management */
>> +#define STMFX_REG_IRQ_GPI_PENDING1 0x0C /* R */
>> +#define STMFX_REG_IRQ_GPI_PENDING2 0x0D /* R */
>> +#define STMFX_REG_IRQ_GPI_PENDING3 0x0E /* R */
>> +#define STMFX_REG_GPIO_STATE1 0x10 /* R */
>> +#define STMFX_REG_GPIO_STATE2 0x11 /* R */
>> +#define STMFX_REG_GPIO_STATE3 0x12 /* R */
>> +#define STMFX_REG_IRQ_GPI_SRC1 0x48 /* RW */
>> +#define STMFX_REG_IRQ_GPI_SRC2 0x49 /* RW */
>> +#define STMFX_REG_IRQ_GPI_SRC3 0x4A /* RW */
>> +#define STMFX_REG_GPO_SET1 0x6C /* RW */
>> +#define STMFX_REG_GPO_SET2 0x6D /* RW */
>> +#define STMFX_REG_GPO_SET3 0x6E /* RW */
>> +#define STMFX_REG_GPO_CLR1 0x70 /* RW */
>> +#define STMFX_REG_GPO_CLR2 0x71 /* RW */
>> +#define STMFX_REG_GPO_CLR3 0x72 /* RW */
>> +
>> +#define STMFX_REG_MAX 0xB0
>> +
>> +/* MFX boot time is around 10ms, so after reset, we have to wait this delay */
>> +#define STMFX_BOOT_TIME_MS 10
>> +
>> +/* STMFX_REG_CHIP_ID bitfields */
>> +#define STMFX_REG_CHIP_ID_MASK GENMASK(7, 0)
>> +
>> +/* STMFX_REG_SYS_CTRL bitfields */
>> +#define STMFX_REG_SYS_CTRL_GPIO_EN BIT(0)
>> +#define STMFX_REG_SYS_CTRL_TS_EN BIT(1)
>> +#define STMFX_REG_SYS_CTRL_IDD_EN BIT(2)
>> +#define STMFX_REG_SYS_CTRL_ALTGPIO_EN BIT(3)
>> +#define STMFX_REG_SYS_CTRL_SWRST BIT(7)
>> +
>> +/* STMFX_REG_IRQ_OUT_PIN bitfields */
>> +#define STMFX_REG_IRQ_OUT_PIN_TYPE BIT(0) /* 0-OD 1-PP */
>> +#define STMFX_REG_IRQ_OUT_PIN_POL BIT(1) /* 0-active LOW 1-active HIGH */
>> +
>> +/* STMFX_REG_IRQ_(SRC_EN/PENDING/ACK) bit shift */
>> +enum stmfx_irqs {
>> + STMFX_REG_IRQ_SRC_EN_GPIO = 0,
>> + STMFX_REG_IRQ_SRC_EN_IDD,
>> + STMFX_REG_IRQ_SRC_EN_ERROR,
>> + STMFX_REG_IRQ_SRC_EN_TS_DET,
>> + STMFX_REG_IRQ_SRC_EN_TS_NE,
>> + STMFX_REG_IRQ_SRC_EN_TS_TH,
>> + STMFX_REG_IRQ_SRC_EN_TS_FULL,
>> + STMFX_REG_IRQ_SRC_EN_TS_OVF,
>> + STMFX_REG_IRQ_SRC_MAX,
>> +};
>
> I'd prefer from here up to the includes to be placed into the header.
>
OK.
>> +/**
>> + * struct stmfx_ddata - STMFX MFD private structure
>
> You can drop '_ddata' from here (read next comment).
>
>> + * @stmfx: state holder with device for logs and register map
>
> I don't understand why this requires an additional struct.
>
>> + * @vdd: STMFX power supply
>> + * @irq_domain: IRQ domain
>> + * @lock: IRQ bus lock
>> + * @irq_src: cache of IRQ_SRC_EN register for bus_lock
>> + * @bkp_sysctrl: backup of SYS_CTRL register for suspend/resume
>> + * @bkp_irqoutpin: backup of IRQ_OUT_PIN register for suspend/resume
>> + */
>> +struct stmfx_ddata {
>> + struct stmfx *stmfx;
>> + struct regulator *vdd;
>> + struct irq_domain *irq_domain;
>> + struct mutex lock; /* IRQ bus lock */
>> + u8 irq_src;
>> +#ifdef CONFIG_PM
>> + u8 bkp_sysctrl;
>> + u8 bkp_irqoutpin;
>> +#endif
>
> I haven't seen this before.
>
> Why is this necessary for *this* device and no others?
>
Not necessary in fact. Private structure was here to reflect the mfd
core specific members because I thought it was useless for his children
to have access to these struct members.
>> +};
>> +
>> +static u8 stmfx_func_to_mask(u32 func)
>> +{
>> + u8 mask = 0;
>> +
>> + if (func & STMFX_FUNC_GPIO)
>> + mask |= STMFX_REG_SYS_CTRL_GPIO_EN;
>> +
>> + if ((func & STMFX_FUNC_ALTGPIO_LOW) || (func & STMFX_FUNC_ALTGPIO_HIGH))
>> + mask |= STMFX_REG_SYS_CTRL_ALTGPIO_EN;
>> +
>> + if (func & STMFX_FUNC_TS)
>> + mask |= STMFX_REG_SYS_CTRL_TS_EN;
>> +
>> + if (func & STMFX_FUNC_IDD)
>> + mask |= STMFX_REG_SYS_CTRL_IDD_EN;
>> +
>> + return mask;
>> +}
>> +
>> +int stmfx_function_enable(struct stmfx *stmfx, u32 func)
>> +{
>> + u32 sys_ctrl;
>> + u8 mask;
>> + int ret;
>> +
>> + ret = regmap_read(stmfx->map, STMFX_REG_SYS_CTRL, &sys_ctrl);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * IDD and TS have priority in STMFX FW, so if IDD and TS are enabled,
>> + * ALTGPIO function is disabled by STMFX FW. If IDD or TS is enabled,
>> + * the number of aGPIO available decreases. To avoid GPIO management
>> + * disturbance, abort IDD or TS function enable in this case.
>> + */
>> + if (((func & STMFX_FUNC_IDD) || (func & STMFX_FUNC_TS)) &&
>> + (sys_ctrl & STMFX_REG_SYS_CTRL_ALTGPIO_EN)) {
>> + dev_err(stmfx->dev, "ALTGPIO function already enabled\n");
>> + return -EBUSY;
>> + }
>> +
>> + /* If TS is enabled, aGPIO[3:0] cannot be used */
>> + if ((func & STMFX_FUNC_ALTGPIO_LOW) &&
>> + (sys_ctrl & STMFX_REG_SYS_CTRL_TS_EN)) {
>> + dev_err(stmfx->dev, "TS in use, aGPIO[3:0] unavailable\n");
>> + return -EBUSY;
>> + }
>> +
>> + /* If IDD is enabled, aGPIO[7:4] cannot be used */
>> + if ((func & STMFX_FUNC_ALTGPIO_HIGH) &&
>> + (sys_ctrl & STMFX_REG_SYS_CTRL_IDD_EN)) {
>> + dev_err(stmfx->dev, "IDD in use, aGPIO[7:4] unavailable\n");
>> + return -EBUSY;
>> + }
>> +
>> + mask = stmfx_func_to_mask(func);
>> +
>> + return regmap_update_bits(stmfx->map, STMFX_REG_SYS_CTRL, mask, mask);
>> +}
>> +EXPORT_SYMBOL_GPL(stmfx_function_enable);
>> +
>> +int stmfx_function_disable(struct stmfx *stmfx, u32 func)
>> +{
>> + u8 mask = stmfx_func_to_mask(func);
>> +
>> + return regmap_update_bits(stmfx->map, STMFX_REG_SYS_CTRL, mask, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(stmfx_function_disable);
>> +
>> +static void stmfx_irq_bus_lock(struct irq_data *data)
>> +{
>> + struct stmfx_ddata *ddata = irq_data_get_irq_chip_data(data);
>> +
>> + mutex_lock(&ddata->lock);
>> +}
>> +
>> +static void stmfx_irq_bus_sync_unlock(struct irq_data *data)
>> +{
>> + struct stmfx_ddata *ddata = irq_data_get_irq_chip_data(data);
>> +
>> + regmap_write(ddata->stmfx->map, STMFX_REG_IRQ_SRC_EN, ddata->irq_src);
>> +
>> + mutex_unlock(&ddata->lock);
>> +}
>> +
>> +static void stmfx_irq_mask(struct irq_data *data)
>> +{
>> + struct stmfx_ddata *ddata = irq_data_get_irq_chip_data(data);
>> +
>> + ddata->irq_src &= ~BIT(data->hwirq % 8);
>> +}
>> +
>> +static void stmfx_irq_unmask(struct irq_data *data)
>> +{
>> + struct stmfx_ddata *ddata = irq_data_get_irq_chip_data(data);
>> +
>> + ddata->irq_src |= BIT(data->hwirq % 8);
>> +}
>> +
>> +static struct irq_chip stmfx_irq_chip = {
>> + .name = "stmfx-core",
>> + .irq_bus_lock = stmfx_irq_bus_lock,
>> + .irq_bus_sync_unlock = stmfx_irq_bus_sync_unlock,
>> + .irq_mask = stmfx_irq_mask,
>> + .irq_unmask = stmfx_irq_unmask,
>> +};
>> +
>> +static irqreturn_t stmfx_irq_handler(int irq, void *data)
>> +{
>> + struct stmfx_ddata *ddata = data;
>> + unsigned long n, pending;
>> + u32 ack;
>> + int ret;
>> +
>> + ret = regmap_read(ddata->stmfx->map, STMFX_REG_IRQ_PENDING,
>> + (u32 *)&pending);
>> + if (ret)
>> + return IRQ_NONE;
>> +
>> + /*
>> + * There is no ACK for GPIO, MFX_REG_IRQ_PENDING_GPIO is a logical OR
>> + * of MFX_REG_IRQ_GPI _PENDING1/_PENDING2/_PENDING3
>> + */
>> + ack = pending & ~BIT(STMFX_REG_IRQ_SRC_EN_GPIO);
>> + if (ack) {
>> + ret = regmap_write(ddata->stmfx->map, STMFX_REG_IRQ_ACK, ack);
>> + if (ret)
>> + return IRQ_NONE;
>> + }
>> +
>> + for_each_set_bit(n, &pending, STMFX_REG_IRQ_SRC_MAX)
>> + handle_nested_irq(irq_find_mapping(ddata->irq_domain, n));
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int stmfx_irq_map(struct irq_domain *d, unsigned int virq,
>> + irq_hw_number_t hwirq)
>> +{
>> + irq_set_chip_data(virq, d->host_data);
>> + irq_set_chip_and_handler(virq, &stmfx_irq_chip, handle_simple_irq);
>> + irq_set_nested_thread(virq, 1);
>> + irq_set_noprobe(virq);
>> +
>> + return 0;
>> +}
>> +
>> +static void stmfx_irq_unmap(struct irq_domain *d, unsigned int virq)
>> +{
>> + irq_set_chip_and_handler(virq, NULL, NULL);
>> + irq_set_chip_data(virq, NULL);
>> +}
>> +
>> +static const struct irq_domain_ops stmfx_irq_ops = {
>> + .map = stmfx_irq_map,
>> + .unmap = stmfx_irq_unmap,
>> +};
>> +
>> +static void stmfx_irq_exit(struct stmfx_ddata *ddata)
>> +{
>> + int hwirq;
>> +
>> + for (hwirq = 0; hwirq < STMFX_REG_IRQ_SRC_MAX; hwirq++)
>> + irq_dispose_mapping(irq_find_mapping(ddata->irq_domain, hwirq));
>
> '\n' here please.
>
>> + irq_domain_remove(ddata->irq_domain);
>> +}
>> +
>> +static int stmfx_irq_init(struct stmfx_ddata *ddata, int irq)
>> +{
>> + struct device *dev = ddata->stmfx->dev;
>
> Better to pass either 'dev' or 'pdev' around and extrapolate using the
> dev_get_drvdata() helpers, rather than the other way around.
>
I agree. I can pass i2c client and extrapolate using the
i2c_get_clientdata helper, here and in _irq_exit, _chip_init,
_chip_exit, to homogenize.
>> + u32 irqoutpin = 0, irqtrigger;
>> + int ret;
>> +
>> + ddata->irq_domain = irq_domain_add_simple(dev->of_node,
>> + STMFX_REG_IRQ_SRC_MAX, 0,
>> + &stmfx_irq_ops, ddata);
>> + if (!ddata->irq_domain) {
>> + dev_err(dev, "failed to create irq domain\n");
>
> s/irq/IRQ/
>
> I also prefer capital letters to start sentences, but it's not
> critical here.
>
OK, I will fix all typos.
>> + return -EINVAL;
>> + }
>> +
>> + if (!of_property_read_bool(dev->of_node, "drive-open-drain"))
>> + irqoutpin |= STMFX_REG_IRQ_OUT_PIN_TYPE;
>> +
>> + irqtrigger = irq_get_trigger_type(irq);
>> + if ((irqtrigger & IRQ_TYPE_EDGE_RISING) ||
>> + (irqtrigger & IRQ_TYPE_LEVEL_HIGH))
>> + irqoutpin |= STMFX_REG_IRQ_OUT_PIN_POL;
>> +
>> + ret = regmap_write(ddata->stmfx->map, STMFX_REG_IRQ_OUT_PIN, irqoutpin);
>> + if (ret)
>> + return ret;
>> +
>> + ret = devm_request_threaded_irq(dev, irq, NULL, stmfx_irq_handler,
>> + irqtrigger | IRQF_ONESHOT,
>> + "stmfx", ddata);
>> + if (ret)
>> + stmfx_irq_exit(ddata);
>> +
>> + return ret;
>> +}
>> +
>> +static int stmfx_chip_reset(struct stmfx_ddata *ddata)
>> +{
>> + int ret;
>> +
>> + ret = regmap_write(ddata->stmfx->map, STMFX_REG_SYS_CTRL,
>> + STMFX_REG_SYS_CTRL_SWRST);
>> + if (ret)
>> + return ret;
>> +
>> + msleep(STMFX_BOOT_TIME_MS);
>> +
>> + return ret;
>> +}
>> +
>> +static int stmfx_chip_init(struct stmfx_ddata *ddata, struct i2c_client *client)
>
> No need to provide both ddata and client, you can extrapolate the
> former from the latter.
>
>> +{
>> + u32 id;
>> + u8 version[2];
>> + int ret;
>> +
>> + ddata->vdd = devm_regulator_get_optional(&client->dev, "vdd");
>> + if (IS_ERR(ddata->vdd)) {
>> + ret = PTR_ERR(ddata->vdd);
>> + if (ret != -ENODEV) {
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(&client->dev,
>> + "no vdd regulator found:%d\n", ret);
>
> s/vdd/VDD/?
>
>> + return ret;
>> + }
>> + }
>> +
>> + if (!IS_ERR(ddata->vdd)) {
>
> Why not just 'else'?
>
>> + ret = regulator_enable(ddata->vdd);
>> + if (ret) {
>> + dev_err(&client->dev, "vdd enable failed: %d\n", ret);
>
> s/vdd/VDD/?
>
>> + return ret;
>> + }
>> + }
>> +
>> + ret = regmap_read(ddata->stmfx->map, STMFX_REG_CHIP_ID, &id);
>> + if (ret) {
>> + dev_err(&client->dev, "error reading chip id: %d\n", ret);
>
> s/id/ID/
>
>> + goto err;
>> + }
>> +
>> + /*
>> + * Check that ID is the complement of the I2C address:
>> + * STMFX I2C address follows the 7-bit format (MSB), that's why
>> + * client->addr is shifted.
>> + *
>> + * STMFX_I2C_ADDR| STMFX | Linux
>> + * input pin | I2C device address | I2C device address
>> + *---------------------------------------------------------
>> + * 0 | b: 1000 010x h:0x84 | 0x42
>> + * 1 | b: 1000 011x h:0x86 | 0x43
>> + */
>> + if (FIELD_GET(STMFX_REG_CHIP_ID_MASK, ~id) != (client->addr << 1)) {
>> + dev_err(&client->dev, "unknown chip id: %#x\n", id);
>> + ret = -EINVAL;
>> + goto err;
>> + }
>> +
>> + ret = regmap_bulk_read(ddata->stmfx->map, STMFX_REG_FW_VERSION_MSB,
>> + version, ARRAY_SIZE(version));
>> + if (ret) {
>> + dev_err(&client->dev, "error reading fw version: %d\n", ret);
>> + goto err;
>> + }
>> +
>> + dev_info(&client->dev, "STMFX id: %#x, fw version: %x.%02x\n",
>> + id, version[0], version[1]);
>> +
>> + return stmfx_chip_reset(ddata);
>> +
>> +err:
>> + if (!IS_ERR(ddata->vdd))
>> + return regulator_disable(ddata->vdd);
>> +
>> + return ret;
>> +}
>> +
>> +static int stmfx_chip_exit(struct stmfx_ddata *ddata)
>> +{
>> + regmap_write(ddata->stmfx->map, STMFX_REG_IRQ_SRC_EN, 0);
>> + regmap_write(ddata->stmfx->map, STMFX_REG_SYS_CTRL, 0);
>> +
>> + if (!IS_ERR(ddata->vdd))
>> + return regulator_disable(ddata->vdd);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct resource stmfx_pinctrl_resources[] = {
>> + DEFINE_RES_IRQ(STMFX_REG_IRQ_SRC_EN_GPIO),
>> +};
>> +
>> +static struct mfd_cell stmfx_cells[] = {
>> + {
>> + .of_compatible = "st,stmfx-0300-pinctrl",
>> + .name = "stmfx-pinctrl",
>> + .resources = stmfx_pinctrl_resources,
>> + .num_resources = ARRAY_SIZE(stmfx_pinctrl_resources),
>> + }
>> +};
>
> What other devices make up this MFD?
>
I can add two other cells for TouchScreen feature with TouchScreen
interrupts, and IDD feature with IDD and Error interrupt (used by IDD
module to raise default/error encountered during IDD measurement).
>> +static bool stmfx_reg_volatile(struct device *dev, unsigned int reg)
>> +{
>> + switch (reg) {
>> + case STMFX_REG_SYS_CTRL:
>> + case STMFX_REG_IRQ_SRC_EN:
>> + case STMFX_REG_IRQ_PENDING:
>> + case STMFX_REG_IRQ_GPI_PENDING1:
>> + case STMFX_REG_IRQ_GPI_PENDING2:
>> + case STMFX_REG_IRQ_GPI_PENDING3:
>> + case STMFX_REG_GPIO_STATE1:
>> + case STMFX_REG_GPIO_STATE2:
>> + case STMFX_REG_GPIO_STATE3:
>> + case STMFX_REG_IRQ_GPI_SRC1:
>> + case STMFX_REG_IRQ_GPI_SRC2:
>> + case STMFX_REG_IRQ_GPI_SRC3:
>> + case STMFX_REG_GPO_SET1:
>> + case STMFX_REG_GPO_SET2:
>> + case STMFX_REG_GPO_SET3:
>> + case STMFX_REG_GPO_CLR1:
>> + case STMFX_REG_GPO_CLR2:
>> + case STMFX_REG_GPO_CLR3:
>> + return true;
>> + default:
>> + return false;
>> + }
>> +}
>> +
>> +static bool stmfx_reg_writeable(struct device *dev, unsigned int reg)
>> +{
>> + return (reg >= STMFX_REG_SYS_CTRL);
>> +}
>> +
>> +static const struct regmap_config stmfx_regmap_config = {
>> + .reg_bits = 8,
>> + .reg_stride = 1,
>> + .val_bits = 8,
>> + .max_register = STMFX_REG_MAX,
>> + .volatile_reg = stmfx_reg_volatile,
>> + .writeable_reg = stmfx_reg_writeable,
>> + .cache_type = REGCACHE_RBTREE,
>> +};
>> +
>> +static int stmfx_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct device *dev = &client->dev;
>> + struct stmfx_ddata *ddata;
>> + int i, ret;
>> +
>> + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
>> + if (!ddata)
>> + return -ENOMEM;
>
> '\n' here please.
>
>> + /* State holder */
>> + ddata->stmfx = devm_kzalloc(dev, sizeof(*ddata->stmfx), GFP_KERNEL);
>> + if (!ddata->stmfx)
>> + return -ENOMEM;
>> +
>> + i2c_set_clientdata(client, ddata);
>> +
>> + ddata->stmfx->dev = dev;
>> +
>> + ddata->stmfx->map = devm_regmap_init_i2c(client, &stmfx_regmap_config);
>
> Just put this straight into the parent struct and do away with the
> extra complication.
>
>> + if (IS_ERR(ddata->stmfx->map)) {
>> + ret = PTR_ERR(ddata->stmfx->map);
>> + dev_err(dev, "failed to allocate register map: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + mutex_init(&ddata->lock);
>> +
>> + ret = stmfx_chip_init(ddata, client);
>> + if (ret) {
>> + if (ret == -ETIMEDOUT)
>> + return -EPROBE_DEFER;
>> + return ret;
>> + }
>> +
>> + if (client->irq < 0) {
>> + dev_err(dev, "failed to get irq: %d\n", client->irq);
>> + ret = client->irq;
>> + goto err_chip_exit;
>> + }
>> +
>> + ret = stmfx_irq_init(ddata, client->irq);
>> + if (ret)
>> + goto err_chip_exit;
>> +
>> + for (i = 0; i < ARRAY_SIZE(stmfx_cells); i++) {
>> + stmfx_cells[i].platform_data = ddata->stmfx;
>> + stmfx_cells[i].pdata_size = sizeof(struct stmfx);
>> + }
>> +
>> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
>> + stmfx_cells, ARRAY_SIZE(stmfx_cells), NULL,
>> + 0, ddata->irq_domain);
>> + if (ret)
>> + goto err_irq_exit;
>> +
>> + return 0;
>> +
>> +err_irq_exit:
>> + stmfx_irq_exit(ddata);
>> +err_chip_exit:
>> + stmfx_chip_exit(ddata);
>> +
>> + return ret;
>> +}
>> +
>> +static int stmfx_remove(struct i2c_client *client)
>> +{
>> + struct stmfx_ddata *ddata = i2c_get_clientdata(client);
>> +
>> + stmfx_irq_exit(ddata);
>> +
>> + return stmfx_chip_exit(ddata);
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int stmfx_backup_regs(struct stmfx_ddata *ddata)
>> +{
>> + int ret;
>> +
>> + ret = regmap_raw_read(ddata->stmfx->map, STMFX_REG_SYS_CTRL,
>> + &ddata->bkp_sysctrl, sizeof(ddata->bkp_sysctrl));
>> + if (ret)
>> + return ret;
>> + ret = regmap_raw_read(ddata->stmfx->map, STMFX_REG_IRQ_OUT_PIN,
>> + &ddata->bkp_irqoutpin,
>> + sizeof(ddata->bkp_irqoutpin));
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int stmfx_restore_regs(struct stmfx_ddata *ddata)
>> +{
>> + int ret;
>> +
>> + ret = regmap_raw_write(ddata->stmfx->map, STMFX_REG_SYS_CTRL,
>> + &ddata->bkp_sysctrl, sizeof(ddata->bkp_sysctrl));
>> + if (ret)
>> + return ret;
>> + ret = regmap_raw_write(ddata->stmfx->map, STMFX_REG_IRQ_OUT_PIN,
>> + &ddata->bkp_irqoutpin,
>> + sizeof(ddata->bkp_irqoutpin));
>> + if (ret)
>> + return ret;
>> + ret = regmap_raw_write(ddata->stmfx->map, STMFX_REG_IRQ_SRC_EN,
>> + &ddata->irq_src, sizeof(ddata->irq_src));
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int stmfx_suspend(struct device *dev)
>> +{
>> + struct stmfx_ddata *ddata = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + ret = stmfx_backup_regs(ddata);
>> + if (ret) {
>> + dev_err(ddata->stmfx->dev, "registers backup failure\n");
>> + return ret;
>> + }
>> +
>> + if (!IS_ERR(ddata->vdd)) {
>> + ret = regulator_disable(ddata->vdd);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int stmfx_resume(struct device *dev)
>> +{
>> + struct stmfx_ddata *ddata = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + if (!IS_ERR(ddata->vdd)) {
>> + ret = regulator_enable(ddata->vdd);
>> + if (ret) {
>> + dev_err(ddata->stmfx->dev,
>> + "vdd enable failed: %d\n", ret);
>> + return ret;
>> + }
>> + }
>> +
>> + ret = stmfx_restore_regs(ddata);
>> + if (ret) {
>> + dev_err(ddata->stmfx->dev, "registers restoration failure\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(stmfx_dev_pm_ops, stmfx_suspend, stmfx_resume);
>> +
>> +static const struct of_device_id stmfx_of_match[] = {
>> + { .compatible = "st,stmfx-0300", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, stmfx_of_match);
>> +
>> +static struct i2c_driver stmfx_driver = {
>> + .driver = {
>> + .name = "stmfx-core",
>> + .of_match_table = of_match_ptr(stmfx_of_match),
>> + .pm = &stmfx_dev_pm_ops,
>> + },
>> + .probe = stmfx_probe,
>> + .remove = stmfx_remove,
>> +};
>> +module_i2c_driver(stmfx_driver);
>> +
>> +MODULE_DESCRIPTION("STMFX core driver");
>> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@xxxxxx>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/stmfx.h b/include/linux/mfd/stmfx.h
>> new file mode 100644
>> index 0000000..35c3d42
>> --- /dev/null
>> +++ b/include/linux/mfd/stmfx.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2018 STMicroelectronics
>> + * Author(s): Amelie Delaunay <amelie.delaunay@xxxxxx>.
>> + */
>> +
>> +#ifndef MFD_STMFX_H
>> +#define MFX_STMFX_H
>> +
>> +#include <linux/regmap.h>
>> +
>> +enum stmfx_functions {
>> + STMFX_FUNC_GPIO = BIT(0), /* GPIO[15:0] */
>> + STMFX_FUNC_ALTGPIO_LOW = BIT(1), /* aGPIO[3:0] */
>> + STMFX_FUNC_ALTGPIO_HIGH = BIT(2), /* aGPIO[7:4] */
>> + STMFX_FUNC_TS = BIT(3),
>> + STMFX_FUNC_IDD = BIT(4),
>> +};
>> +
>> +struct stmfx {
>> + struct device *dev;
>> + struct regmap *map;
>> +};
>> +
>> +int stmfx_function_enable(struct stmfx *stmfx, u32 func);
>> +int stmfx_function_disable(struct stmfx *stmfx, u32 func);
>> +#endif
>
V4 is coming, based on your comments. Thanks for the review.
Regards,
Amelie
