Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 12 Feb 2018 12:06:00 +0000
From:   Lee Jones <lee.jones@linaro.org>
To:     Amelie Delaunay <amelie.delaunay@st.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Russell King <linux@armlinux.org.uk>,
        Alexandre Torgue <alexandre.torgue@st.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/6] mfd: Add ST Multi-Function eXpander core driver
Message-ID: <20180212120600.5cghrzz4jqvvkgz6@dell>
References: <1518100057-23234-1-git-send-email-amelie.delaunay@st.com>
 <1518100057-23234-3-git-send-email-amelie.delaunay@st.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1518100057-23234-3-git-send-email-amelie.delaunay@st.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 08 Feb 2018, Amelie Delaunay wrote:

> ST Multi-Function eXpander (MFX) 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
> - Main MCU IDD measurement
> - Resistive touchscreen controller
> 
> Only GPIO expander (16 fast GPIOs + 8 alternate) feature is
> supported for the moment.
> 
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> ---
>  drivers/mfd/Kconfig        |  15 ++
>  drivers/mfd/Makefile       |   1 +
>  drivers/mfd/st-mfx.c       | 526 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/st-mfx.h | 116 ++++++++++
>  4 files changed, 658 insertions(+)
>  create mode 100644 drivers/mfd/st-mfx.c
>  create mode 100644 include/linux/mfd/st-mfx.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1d20a80..e78e818 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1823,6 +1823,21 @@ config MFD_STM32_TIMERS
>  	  for PWM and IIO Timer. This driver allow to share the
>  	  registers between the others drivers.
>  
> +config MFD_ST_MFX
> +	bool "STMicroelectronics MFX"
> +	depends on I2C
> +	depends on OF || COMPILE_TEST
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	help
> +	  Support for the STMicroelectronics Multi-Function eXpander.

Is that really what this device is called?

Is there a datasheet?

> +	  This driver provides common support for accessing the device,
> +	  additional drivers must be enabled in order to use the functionality
> +	  of the device. Currently available sub drivers are:
> +
> +		GPIO: mfx-gpio

This driver would be easier to review if I could picture the device as
a whole.  What other functionality does it have?  What is it that
makes this an MFD?

>  menu "Multimedia Capabilities Port drivers"
>  	depends on ARCH_SA1100
>  
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index d9474ad..1379a18 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -230,3 +230,4 @@ obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
>  obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
>  obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
>  obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
> +obj-$(CONFIG_MFD_ST_MFX) 	+= st-mfx.o
> diff --git a/drivers/mfd/st-mfx.c b/drivers/mfd/st-mfx.c
> new file mode 100644
> index 0000000..5bee5d3
> --- /dev/null
> +++ b/drivers/mfd/st-mfx.c
> @@ -0,0 +1,526 @@
> +/*
> + * STMicroelectronics Multi-Function eXpander (ST-MFX) Core Driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.

You don't need to put "for STMicroelectronics".  This was something we
made up when submitting from a different (!st.com) email address.

> + * License terms: GPL V2.0.
> + *
> + * st-mfx Core Driver is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * st-mfx Core Driver is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * st-mfx Core Driver. If not, see <http://www.gnu.org/licenses/>.

You should be able to use the short version of the licensing
agreement.  Also, please grep for "SPDX".

> + */
> +#include <linux/bitfield.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/st-mfx.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/regmap.h>
> +
> +/**
> + * struct mfx_priv - MFX MFD private structure
> + * @dev: device, mostly for logs
> + * @regmap: register map
> + * @mfx: MFX MFD public structure, to share information with subdrivers
> + * @irq_domain: IRQ domain
> + * @irq: IRQ number for mfx
> + * @irq_lock: IRQ bus lock
> + * @irqen: cache of IRQ_SRC_EN register for bus_lock
> + * @oldirqen: cache of IRQ_SRC_EN register for bus_lock
> + */

/**
 * struct mfx_priv - MFX MFD private structure
 * @dev:  	   device, mostly for logs
 * @regmap: 	   register map
 * @mfx: 	   MFX MFD public structure, to share information with subdrivers
 * @irq_domain:    IRQ domain
 * @irq: 	   IRQ number for mfx
 * @irq_lock: 	   IRQ bus lock
 * @irqen: 	   cache of IRQ_SRC_EN register for bus_lock
 * @oldirqen: 	   cache of IRQ_SRC_EN register for bus_lock
 */

Easier to read?

> +struct mfx_priv {

mfx_ddata

> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct mfx mfx;
> +	struct irq_domain *irq_domain;
> +	int irq;
> +	struct mutex irq_lock; /* IRQ bus lock */
> +	u8 irqen;
> +	u8 oldirqen;
> +};
> +
> +#define to_mfx_priv(_mfx) container_of(_mfx, struct mfx_priv, mfx)

FYI: I don't like this idea.  More to follow.

> +/* MFX boot time is around 10ms, so after reset, we have to wait this delay */
> +#define MFX_BOOT_TIME 10

MFX_BOOT_TIME_MS

> +static u8 mfx_blocks_to_mask(u32 blocks)

I think it would be better to prepend a vendor tag to everything too.

  st_mfx_*

> +{
> +	u8 mask = 0;
> +
> +	if (blocks & MFX_BLOCK_GPIO)
> +		mask |= MFX_REG_SYS_CTRL_GPIO_EN;
> +	else
> +		mask &= ~MFX_REG_SYS_CTRL_GPIO_EN;
> +
> +	if (blocks & MFX_BLOCK_TS)
> +		mask |= MFX_REG_SYS_CTRL_TS_EN;
> +	else
> +		mask &= ~MFX_REG_SYS_CTRL_TS_EN;
> +
> +	if (blocks & MFX_BLOCK_IDD)
> +		mask |= MFX_REG_SYS_CTRL_IDD_EN;
> +	else
> +		mask &= ~MFX_REG_SYS_CTRL_IDD_EN;
> +
> +	if (blocks & MFX_BLOCK_ALTGPIO)
> +		mask |= MFX_REG_SYS_CTRL_ALTGPIO_EN;
> +	else
> +		mask &= ~MFX_REG_SYS_CTRL_ALTGPIO_EN;
> +
> +	return mask;
> +}
> +
> +static int mfx_reset(struct mfx *mfx)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +	int ret;
> +
> +	ret = regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
> +				 MFX_REG_SYS_CTRL_SWRST,
> +				 MFX_REG_SYS_CTRL_SWRST);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(MFX_BOOT_TIME);
> +
> +	return ret;
> +}
> +
> +int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +
> +	return regmap_bulk_read(mfx_priv->regmap, reg, values, length);
> +}
> +EXPORT_SYMBOL_GPL(mfx_block_read);
> +
> +int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +
> +	return regmap_bulk_write(mfx_priv->regmap, reg, values, length);
> +}
> +EXPORT_SYMBOL_GPL(mfx_block_write);
> +
> +int mfx_reg_read(struct mfx *mfx, u8 reg)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(mfx_priv->regmap, reg, &val);
> +
> +	return ret ? ret : val;
> +}
> +EXPORT_SYMBOL_GPL(mfx_reg_read);
> +
> +int mfx_reg_write(struct mfx *mfx, u8 reg, u8 val)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +
> +	return regmap_write(mfx_priv->regmap, reg, val);
> +}
> +EXPORT_SYMBOL_GPL(mfx_reg_write);
> +
> +int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +
> +	return regmap_update_bits(mfx_priv->regmap, reg, mask, val);
> +}
> +EXPORT_SYMBOL_GPL(mfx_set_bits);
> +
> +int mfx_enable(struct mfx *mfx, u32 blocks)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +	u8 mask = mfx_blocks_to_mask(blocks);
> +
> +	return regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
> +				  mask, mask);
> +}
> +EXPORT_SYMBOL_GPL(mfx_enable);
> +
> +int mfx_disable(struct mfx *mfx, u32 blocks)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +	u8 mask = mfx_blocks_to_mask(blocks);
> +
> +	return regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
> +				  mask, 0);
> +}
> +EXPORT_SYMBOL_GPL(mfx_disable);

The remap infrastructure doesn't need further abstraction.  Please
pass the pointer to any child devices and have them use it directly.

> +static void mfx_irq_lock(struct irq_data *data)
> +{
> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
> +
> +	mutex_lock(&mfx_priv->irq_lock);
> +}
> +
> +static void mfx_irq_sync_unlock(struct irq_data *data)
> +{
> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
> +	u8 new = mfx_priv->irqen;
> +	u8 old = mfx_priv->oldirqen;
> +
> +	if (new == old)
> +		goto unlock;
> +
> +	mfx_priv->oldirqen = new;
> +	mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_SRC_EN, new);
> +unlock:
> +	mutex_unlock(&mfx_priv->irq_lock);
> +}
> +
> +static void mfx_irq_mask(struct irq_data *data)
> +{
> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
> +
> +	mfx_priv->irqen &= ~BIT(data->hwirq % 8);
> +}
> +
> +static void mfx_irq_unmask(struct irq_data *data)
> +{
> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
> +
> +	mfx_priv->irqen |= BIT(data->hwirq % 8);
> +}
> +
> +static struct irq_chip mfx_irq_chip = {
> +	.name			= "mfx",
> +	.irq_bus_lock		= mfx_irq_lock,
> +	.irq_bus_sync_unlock	= mfx_irq_sync_unlock,
> +	.irq_mask		= mfx_irq_mask,
> +	.irq_unmask		= mfx_irq_unmask,
> +};
> +
> +static irqreturn_t mfx_irq(int irq, void *data)
> +{
> +	struct mfx_priv *mfx_priv = data;

'ddata' is a more consistent naming approach.

> +	unsigned long status, bit;
> +	u8 ack;
> +	int ret;
> +
> +	ret = mfx_reg_read(&mfx_priv->mfx, MFX_REG_IRQ_PENDING);
> +	if (ret < 0) {
> +		dev_err(mfx_priv->dev, "can't get IRQ_PENDING: %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	/* It can be GPIO, IDD, ERROR, TS* IRQs */
> +	status = ret & mfx_priv->irqen;
> +
> +	/*
> +	 * There is no ACK for GPIO, MFX_REG_IRQ_PENDING_GPIO is a logical OR
> +	 * of MFX_REG_IRQ_GPI _PENDING1/_PENDING2/_PENDING3
> +	 */
> +	ack = status & ~MFX_REG_IRQ_PENDING_GPIO;
> +
> +	for_each_set_bit(bit, &status, 8)
> +		handle_nested_irq(irq_find_mapping(mfx_priv->irq_domain, bit));
> +
> +	if (ack)
> +		mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_ACK, ack);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mfx_irq_map(struct irq_domain *d, unsigned int virq,
> +		       irq_hw_number_t hwirq)
> +{
> +	struct mfx_priv *mfx_priv = d->host_data;
> +
> +	irq_set_chip_data(virq, mfx_priv);
> +	irq_set_chip(virq, &mfx_irq_chip);
> +	irq_set_nested_thread(virq, 1);
> +	irq_set_parent(virq, mfx_priv->irq);
> +	irq_set_noprobe(virq);
> +
> +	return 0;
> +}
> +
> +static void mfx_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 mfx_irq_ops = {
> +	.map = mfx_irq_map,
> +	.unmap = mfx_irq_unmap,
> +	.xlate = irq_domain_xlate_onecell,
> +};
> +
> +static int mfx_irq_init(struct mfx_priv *mfx_priv, struct device_node *np)
> +{
> +	int irqoutpin = MFX_REG_IRQ_OUT_PIN_TYPE; /* Push-Pull */
> +	int irqtrigger, ret;
> +
> +	mfx_priv->irq = of_irq_get(np, 0);
> +	if (mfx_priv->irq > 0) {
> +		irqtrigger = irq_get_trigger_type(mfx_priv->irq);
> +	} else {
> +		dev_err(mfx_priv->dev, "failed to get irq: %d\n",
> +			mfx_priv->irq);
> +		return mfx_priv->irq;
> +	}
> +
> +	if ((irqtrigger & IRQ_TYPE_EDGE_FALLING) ||
> +	    (irqtrigger & IRQ_TYPE_LEVEL_LOW))
> +		irqoutpin &= ~MFX_REG_IRQ_OUT_PIN_POL; /* Active Low */
> +	else
> +		irqoutpin |= MFX_REG_IRQ_OUT_PIN_POL; /* Active High */
> +
> +	mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_OUT_PIN, irqoutpin);
> +
> +	mfx_priv->irq_domain = irq_domain_add_linear(np, MFX_IRQ_SRC_NR,
> +						     &mfx_irq_ops, mfx_priv);
> +	if (!mfx_priv->irq_domain) {
> +		dev_err(mfx_priv->dev, "failed to create irq domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = devm_request_threaded_irq(mfx_priv->dev, mfx_priv->irq,
> +					NULL, mfx_irq,
> +					irqtrigger | IRQF_ONESHOT, "mfx",
> +					mfx_priv);
> +	if (ret) {
> +		dev_err(mfx_priv->dev, "failed to request irq: %d\n", ret);
> +		irq_domain_remove(mfx_priv->irq_domain);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mfx_irq_remove(struct mfx_priv *mfx_priv)
> +{
> +	int hwirq;
> +
> +	for (hwirq = 0; hwirq < MFX_IRQ_SRC_NR; hwirq++)
> +		irq_dispose_mapping(irq_find_mapping(mfx_priv->irq_domain,
> +						     hwirq));
> +	irq_domain_remove(mfx_priv->irq_domain);
> +}

Is there any reason why this IRQ stuff can't live in the GPIO driver?

> +static int mfx_chip_init(struct mfx_priv *mfx_priv, u16 i2c_addr)
> +{
> +	struct mfx *mfx = &mfx_priv->mfx;
> +	int ret;
> +	int id;
> +	u8 version[2];
> +
> +	id = mfx_reg_read(mfx, MFX_REG_CHIP_ID);
> +	if (id < 0) {
> +		dev_err(mfx_priv->dev, "error reading chip id: %d\n", id);
> +		return id;
> +	}
> +
> +	ret = mfx_block_read(mfx, MFX_REG_FW_VERSION_MSB,
> +			     ARRAY_SIZE(version), version);
> +	if (ret < 0) {
> +		dev_err(mfx_priv->dev, "error reading fw version: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Check that ID is the complement of the I2C address:
> +	 * MFX I2C address follows the 7-bit format (MSB), that's why
> +	 * i2c_addr is shifted.
> +	 *
> +	 * MFX_I2C_ADDR |         MFX         |        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(MFX_REG_CHIP_ID_MASK, ~id) != (i2c_addr << 1)) {
> +		dev_err(mfx_priv->dev, "unknown chip id: %#x\n", id);
> +		return -EINVAL;
> +	}
> +
> +	dev_info(mfx_priv->dev, "ST-MFX chip id: %#x, fw version: %x.%02x\n",
> +		 id, version[0], version[1]);
> +
> +	/* Disable all features, subdrivers should enable what they need */
> +	ret = mfx_disable(mfx, ~0);
> +	if (ret)
> +		return ret;
> +
> +	return mfx_reset(mfx);
> +}
> +
> +static const struct regmap_config mfx_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static const struct of_device_id mfx_of_match[] = {
> +	{ .compatible = "st,mfx" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, mfx_of_match);
> +
> +static int mfx_of_probe(struct device_node *np, struct mfx_priv *mfx_priv)
> +{
> +	struct device_node *child;
> +
> +	if (!np)
> +		return -ENODEV;

Is this even possible?

Do you support !OF?

> +	for_each_child_of_node(np, child) {
> +		if (of_device_is_compatible(child, "st,mfx-gpio")) {
> +			mfx_priv->mfx.blocks |= MFX_BLOCK_GPIO;
> +			mfx_priv->mfx.num_gpio = 16;

This is ugly.

Where is num_gpio used?

> +		}
> +		/*
> +		 * Here we could find other children like "st,mfx-ts" or
> +		 * "st,mfx-idd.
> +		 */
> +	}
> +
> +	if (!(mfx_priv->mfx.blocks & MFX_BLOCK_TS) &&
> +	    !(mfx_priv->mfx.blocks & MFX_BLOCK_IDD)) {
> +		mfx_priv->mfx.blocks |= MFX_BLOCK_ALTGPIO;
> +		mfx_priv->mfx.num_gpio += 8;
> +	}

What are TS and IDD?

This is starting to look like a GPIO driver, and nothing more.

> +	/*
> +	 * TODO: aGPIO[3:0] and aGPIO[7:4] can be used independently:
> +	 * - if IDD is used but not TS, aGPIO[3:0] can be used as GPIO,
> +	 * - if TS is used but not IDD: aGPIO[7:4] can be used as GPIO.
> +	 */
> +
> +	return of_platform_populate(np, NULL, NULL, mfx_priv->dev);
> +}
> +
> +int mfx_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct device_node *np = client->dev.of_node;
> +	struct mfx_priv *mfx_priv;
> +	int ret;
> +
> +	mfx_priv = devm_kzalloc(&client->dev, sizeof(struct mfx_priv),
> +				GFP_KERNEL);
> +	if (!mfx_priv)
> +		return -ENOMEM;
> +
> +	mfx_priv->regmap = devm_regmap_init_i2c(client, &mfx_regmap_config);
> +	if (IS_ERR(mfx_priv->regmap)) {
> +		ret = PTR_ERR(mfx_priv->regmap);
> +		dev_err(&client->dev, "failed to allocate register map: %d\n",
> +			ret);

Nit: Prefer if you broke the line after '&client->dev,'.

> +		return ret;
> +	}
> +
> +	mfx_priv->dev = &client->dev;


> +	i2c_set_clientdata(client, &mfx_priv->mfx);
> +
> +	mutex_init(&mfx_priv->irq_lock);
> +
> +	ret = mfx_chip_init(mfx_priv, client->addr);
> +	if (ret) {
> +		if (ret == -ETIMEDOUT)
> +			return -EPROBE_DEFER;

Return -EPROBE_DEFER from reset() instead.

> +		return ret;
> +	}
> +
> +	ret = mfx_irq_init(mfx_priv, np);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mfx_of_probe(np, mfx_priv);
> +	if (ret < 0)
> +		return ret;

Is it possible to build with !OF?

If not, move all probe code into here.

> +	dev_info(mfx_priv->dev, "ST-MFX (CORE) initialized\n");

These kinds of messages are usually frowned upon.

> +	return 0;
> +}
> +
> +static int mfx_remove(struct i2c_client *client)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(i2c_get_clientdata(client));
> +
> +	mfx_irq_remove(mfx_priv);
> +	of_platform_depopulate(mfx_priv->dev);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mfx_suspend(struct device *dev)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(dev_get_drvdata(dev));
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(mfx_priv->irq);
> +
> +	/*
> +	 * TODO: Do we put MFX in STANDBY mode ?
> +	 * (Wakeup by rising edge on MFX_wakeup pin)
> +	 */

How long before you answer this question?

Better do just enable it right away or make a mental note and remove
the comment from the upstream driver.

> +	return 0;
> +}
> +
> +static int mfx_resume(struct device *dev)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(dev_get_drvdata(dev));
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(mfx_priv->irq);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(mfx_dev_pm_ops, mfx_suspend, mfx_resume);
> +
> +static const struct i2c_device_id mfx_i2c_id[] = {
> +	{ "mfx", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, mfx_id);

I don't think this is required anymore.

> +static struct i2c_driver mfx_driver = {
> +	.driver = {
> +		.name = "st-mfx",
> +		.pm = &mfx_dev_pm_ops,
> +		.of_match_table = mfx_of_match,
> +	},
> +	.probe = mfx_probe,
> +	.remove = mfx_remove,
> +	.id_table = mfx_i2c_id,
> +};
> +
> +static int __init mfx_init(void)
> +{
> +	return i2c_add_driver(&mfx_driver);
> +}
> +subsys_initcall(mfx_init);
> +
> +static void __exit mfx_exit(void)
> +{
> +	i2c_del_driver(&mfx_driver);
> +}
> +module_exit(mfx_exit);
> +
> +MODULE_DESCRIPTION("STMicroelectronics Multi-Function eXpander MFD core driver");
> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/st-mfx.h b/include/linux/mfd/st-mfx.h
> new file mode 100644
> index 0000000..1bf7e65
> --- /dev/null
> +++ b/include/linux/mfd/st-mfx.h
> @@ -0,0 +1,116 @@
> +/*
> + * STMicroelectronics Multi-Function eXpander (ST-MFX)
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.
> + *
> + * License terms: GPL V2.0.
> + *
> + * st-mfx is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * st-mfx is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * st-mfx. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __MFD_ST_MFX_H
> +#define __MFD_ST_MFX_H
> +
> +enum mfx_block {
> +	MFX_BLOCK_GPIO		= BIT(0),
> +	MFX_BLOCK_TS		= BIT(1),
> +	MFX_BLOCK_IDD		= BIT(2),
> +	MFX_BLOCK_ALTGPIO	= BIT(3),
> +};
> +
> +/*
> + * 8 events can activate the MFX_IRQ_OUT signal,
> + * but for the moment, only GPIO event is used
> + */
> +enum mfx_irq {
> +	MFX_IRQ_SRC_GPIO,
> +
> +	MFX_IRQ_SRC_NR,
> +};
> +
> +/**
> + * struct mfx - MFX MFD structure
> + * @blocks: mask of mfx_block to be enabled
> + * @num_gpio: number of gpios
> + */
> +struct mfx {
> +	u32 blocks;
> +	u32 num_gpio;
> +};
> +
> +int mfx_reg_write(struct mfx *mfx, u8 reg, u8 data);
> +int mfx_reg_read(struct mfx *mfx, u8 reg);
> +int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values);
> +int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values);
> +int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val);
> +int mfx_enable(struct mfx *mfx, unsigned int blocks);
> +int mfx_disable(struct mfx *mfx, unsigned int blocks);
> +
> +/* General */
> +#define MFX_REG_CHIP_ID			0x00 /* R */
> +#define MFX_REG_FW_VERSION_MSB		0x01 /* R */
> +#define MFX_REG_FW_VERSION_LSB		0x02 /* R */
> +#define MFX_REG_SYS_CTRL		0x40 /* RW */
> +/* IRQ output management */
> +#define MFX_REG_IRQ_OUT_PIN		0x41 /* RW */
> +#define MFX_REG_IRQ_SRC_EN		0x42 /* RW */
> +#define MFX_REG_IRQ_PENDING		0x08 /* R */
> +#define MFX_REG_IRQ_ACK			0x44 /* RW */
> +/* GPIOs expander */
> +/* GPIO_STATE1 0x10, GPIO_STATE2 0x11, GPIO_STATE3 0x12 */
> +#define MFX_REG_GPIO_STATE		0x10 /* R */
> +/* GPIO_DIR1 0x60, GPIO_DIR2 0x61, GPIO_DIR3 0x63 */
> +#define MFX_REG_GPIO_DIR		0x60 /* RW */
> +/* GPIO_TYPE1 0x64, GPIO_TYPE2 0x65, GPIO_TYPE3 0x66 */
> +#define MFX_REG_GPIO_TYPE		0x64 /* RW */
> +/* GPIO_PUPD1 0x68, GPIO_PUPD2 0x69, GPIO_PUPD3 0x6A */
> +#define MFX_REG_GPIO_PUPD		0x68 /* RW */
> +/* GPO_SET1 0x6C, GPO_SET2 0x6D, GPO_SET3 0x6E */
> +#define MFX_REG_GPO_SET			0x6C /* RW */
> +/* GPO_CLR1 0x70, GPO_CLR2 0x71, GPO_CLR3 0x72 */
> +#define MFX_REG_GPO_CLR			0x70 /* RW */
> +/* IRQ_GPI_SRC1 0x48, IRQ_GPI_SRC2 0x49, IRQ_GPI_SRC3 0x4A */
> +#define MFX_REG_IRQ_GPI_SRC		0x48 /* RW */
> +/* IRQ_GPI_EVT1 0x4C, IRQ_GPI_EVT2 0x4D, IRQ_GPI_EVT3 0x4E */
> +#define MFX_REG_IRQ_GPI_EVT		0x4C /* RW */
> +/* IRQ_GPI_TYPE1 0x50, IRQ_GPI_TYPE2 0x51, IRQ_GPI_TYPE3 0x52 */
> +#define MFX_REG_IRQ_GPI_TYPE		0x50 /* RW */
> +/* IRQ_GPI_PENDING1 0x0C, IRQ_GPI_PENDING2 0x0D, IRQ_GPI_PENDING3 0x0E*/
> +#define MFX_REG_IRQ_GPI_PENDING		0x0C /* R */
> +/* IRQ_GPI_ACK1 0x54, IRQ_GPI_ACK2 0x55, IRQ_GPI_ACK3 0x56 */
> +#define MFX_REG_IRQ_GPI_ACK		0x54 /* RW */
> +
> +/* MFX_REG_CHIP_ID bitfields */
> +#define MFX_REG_CHIP_ID_MASK		GENMASK(7, 0)
> +
> +/* MFX_REG_SYS_CTRL bitfields */
> +#define MFX_REG_SYS_CTRL_GPIO_EN	BIT(0)
> +#define MFX_REG_SYS_CTRL_TS_EN		BIT(1)
> +#define MFX_REG_SYS_CTRL_IDD_EN		BIT(2)
> +#define MFX_REG_SYS_CTRL_ALTGPIO_EN	BIT(3)
> +#define MFX_REG_SYS_CTRL_STANDBY	BIT(6)
> +#define MFX_REG_SYS_CTRL_SWRST		BIT(7)
> +
> +/* MFX_REG_IRQ_OUT_PIN bitfields */
> +#define MFX_REG_IRQ_OUT_PIN_TYPE	BIT(0) /* 0-OD 1-PP */
> +#define MFX_REG_IRQ_OUT_PIN_POL		BIT(1) /* 0-active LOW 1-active HIGH */
> +
> +/* MFX_REG_IRQ_SRC_EN bitfields */
> +#define MFX_REG_IRQ_SRC_EN_GPIO		BIT(0)
> +
> +/* MFX_REG_IRQ_PENDING bitfields */
> +#define MFX_REG_IRQ_PENDING_GPIO	BIT(0)
> +
> +#endif /* __MFD_ST_MFX_H */
> +

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog