[PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[Sourav Poddar]

smsc ece1099 is a keyboard scan or gpio expansion device.
The patch create keypad and gpio expander child for this
multi function smsc driver.

Tested on omap5430 evm with 3.6-rc6 custom kernel.

Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
Changes since v2:
 - Change the cache type, max register assignment
 - remove of_device_table, since i2c based device gets 
probed according to i2c_device_id through dt.
 - Modify the remove function
 - Minor return patch cleanups.
 Documentation/smsc_ece1099.txt |   56 ++++++++++++++++++++
 drivers/mfd/Kconfig            |   12 ++++
 drivers/mfd/Makefile           |    1 +
 drivers/mfd/smsc-ece1099.c     |  113 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/smsc.h       |  109 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 291 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/smsc_ece1099.txt
 create mode 100644 drivers/mfd/smsc-ece1099.c
 create mode 100644 include/linux/mfd/smsc.h

diff --git a/Documentation/smsc_ece1099.txt b/Documentation/smsc_ece1099.txt
new file mode 100644
index 0000000..6b492e8
--- /dev/null
+++ b/Documentation/smsc_ece1099.txt
@@ -0,0 +1,56 @@
+What is smsc-ece1099?
+----------------------
+
+The ECE1099 is a 40-Pin 3.3V Keyboard Scan Expansion
+or GPIO Expansion device. The device supports a keyboard
+scan matrix of 23x8. The device is connected to a Master
+via the SMSC BC-Link interface or via the SMBus.
+Keypad scan Input(KSI) and Keypad Scan Output(KSO) signals
+are multiplexed with GPIOs.
+
+Interrupt generation
+--------------------
+
+Interrupts can be generated by an edge detection on a GPIO
+pin or an edge detection on one of the bus interface pins.
+Interrupts can also be detected on the keyboard scan interface.
+The bus interrupt pin (BC_INT# or SMBUS_INT#) is asserted if
+any bit in one of the Interrupt Status registers is 1 and
+the corresponding Interrupt Mask bit is also 1.
+
+In order for software to determine which device is the source
+of an interrupt, it should first read the Group Interrupt Status Register
+to determine which Status register group is a source for the interrupt.
+Software should read both the Status register and the associated Mask register,
+then AND the two values together. Bits that are 1 in the result of the AND
+are active interrupts. Software clears an interrupt by writing a 1 to the
+corresponding bit in the Status register.
+
+Communication Protocol
+----------------------
+
+- SMbus slave Interface
+	The host processor communicates with the ECE1099 device
+	through a series of read/write registers via the SMBus
+	interface. SMBus is a serial communication protocol between
+	a computer host and its peripheral devices. The SMBus data
+	rate is 10KHz minimum to 400 KHz maximum
+
+- Slave Bus Interface
+	The ECE1099 device SMBus implementation is a subset of the
+	SMBus interface to the host. The device is a slave-only SMBus device.
+	The implementation in the device is a subset of SMBus since it
+	only supports four protocols.
+
+	The Write Byte, Read Byte, Send Byte, and Receive Byte protocols are the
+	only valid SMBus protocols for the device.
+
+- BC-LinkTM Interface
+	The BC-Link is a proprietary bus that allows communication
+	between a Master device and a Companion device. The Master
+	device uses this serial bus to read and write registers
+	located on the Companion device. The bus comprises three signals,
+	BC_CLK, BC_DAT and BC_INT#. The Master device always provides the
+	clock, BC_CLK, and the Companion device is the source for an
+	independent asynchronous interrupt signal, BC_INT#. The ECE1099
+	supports BC-Link speeds up to 24MHz.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b1a1462..dedc874 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -385,6 +385,18 @@ config MFD_T7L66XB
 	help
 	  Support for Toshiba Mobile IO Controller T7L66XB
 
+config MFD_SMSC
+       bool "Support for the SMSC ECE1099 series chips"
+       depends on I2C=y
+       select MFD_CORE
+       select REGMAP_I2C
+       help
+        If you say yes here you get support for the
+        ece1099 chips from SMSC.
+
+        To compile this driver as a module, choose M here: the
+        module will be called smsc.
+
 config MFD_TC6387XB
 	bool "Support Toshiba TC6387XB"
 	depends on ARM && HAVE_CLK
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 79dd22d..3323345 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_EZX_PCAP)		+= ezx-pcap.o
 obj-$(CONFIG_MCP)		+= mcp-core.o
 obj-$(CONFIG_MCP_SA11X0)	+= mcp-sa11x0.o
 obj-$(CONFIG_MCP_UCB1200)	+= ucb1x00-core.o
+obj-$(CONFIG_MFD_SMSC)        += smsc-ece1099.o
 obj-$(CONFIG_MCP_UCB1200_TS)	+= ucb1x00-ts.o
 
 ifeq ($(CONFIG_SA1100_ASSABET),y)
diff --git a/drivers/mfd/smsc-ece1099.c b/drivers/mfd/smsc-ece1099.c
new file mode 100644
index 0000000..220aec2
--- /dev/null
+++ b/drivers/mfd/smsc-ece1099.c
@@ -0,0 +1,113 @@
+/*
+ * TI SMSC MFD Driver
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Author: Sourav Poddar <sourav.poddar@ti.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under  the terms of the GNU General  Public License as published by the
+ *  Free Software Foundation;  GPL v2.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/workqueue.h>
+#include <linux/irq.h>
+#include <linux/regmap.h>
+#include <linux/err.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/smsc.h>
+#include <linux/of_platform.h>
+
+static struct regmap_config smsc_regmap_config = {
+		.reg_bits = 8,
+		.val_bits = 8,
+		.max_register = SMSC_VEN_ID_H,
+		.cache_type = REGCACHE_RBTREE,
+};
+
+static int smsc_i2c_probe(struct i2c_client *i2c,
+			const struct i2c_device_id *id)
+{
+	struct device_node              *node = i2c->dev.of_node;
+	struct smsc *smsc;
+	int devid, rev, venid_l, venid_h;
+	int ret = 0;
+
+	smsc = devm_kzalloc(&i2c->dev, sizeof(struct smsc),
+				GFP_KERNEL);
+	if (!smsc) {
+		dev_err(&i2c->dev, "smsc mfd driver memory allocation failed\n");
+		return -ENOMEM;
+	}
+
+	smsc->regmap = devm_regmap_init_i2c(i2c, &smsc_regmap_config);
+	if (IS_ERR(smsc->regmap)) {
+		ret = PTR_ERR(smsc->regmap);
+		goto err;
+	}
+
+	i2c_set_clientdata(i2c, smsc);
+	smsc->dev = &i2c->dev;
+
+#ifdef CONFIG_OF
+	of_property_read_u32(node, "clock", &smsc->clk);
+#endif
+
+	regmap_read(smsc->regmap, SMSC_DEV_ID, &devid);
+	regmap_read(smsc->regmap, SMSC_DEV_REV, &rev);
+	regmap_read(smsc->regmap, SMSC_VEN_ID_L, &venid_l);
+	regmap_read(smsc->regmap, SMSC_VEN_ID_H, &venid_h);
+
+	dev_info(&i2c->dev, "SMSCxxx devid: %02x rev: %02x venid: %02x\n",
+		devid, rev, (venid_h << 8) | venid_l);
+
+	ret = regmap_write(smsc->regmap, SMSC_CLK_CTRL, smsc->clk);
+	if (ret)
+		goto err;
+
+#ifdef CONFIG_OF
+	if (node)
+		ret = of_platform_populate(node, NULL, NULL, &i2c->dev);
+#endif
+
+err:
+	return ret;
+}
+
+static int smsc_i2c_remove(struct i2c_client *i2c)
+{
+	struct smsc *smsc = i2c_get_clientdata(i2c);
+
+	mfd_remove_devices(smsc->dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id smsc_i2c_id[] = {
+	{ "smscece1099", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, smsc_i2c_id);
+
+static struct i2c_driver smsc_i2c_driver = {
+	.driver = {
+		   .name = "smsc",
+		   .owner = THIS_MODULE,
+	},
+	.probe = smsc_i2c_probe,
+	.remove = smsc_i2c_remove,
+	.id_table = smsc_i2c_id,
+};
+
+module_i2c_driver(smsc_i2c_driver);
+
+MODULE_AUTHOR("Sourav Poddar <sourav.poddar@ti.com>");
+MODULE_DESCRIPTION("SMSC chip multi-function driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/smsc.h b/include/linux/mfd/smsc.h
new file mode 100644
index 0000000..9747b29
--- /dev/null
+++ b/include/linux/mfd/smsc.h
@@ -0,0 +1,109 @@
+/*
+ * SMSC ECE1099
+ *
+ * Copyright 2012 Texas Instruments Inc.
+ *
+ * Author: Sourav Poddar <sourav.poddar@ti.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under  the terms of the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the License, or (at your
+ *  option) any later version.
+ *
+ */
+
+#ifndef __LINUX_MFD_SMSC_H
+#define __LINUX_MFD_SMSC_H
+
+#include <linux/regmap.h>
+
+#define SMSC_ID_ECE1099			1
+#define SMSC_NUM_CLIENTS		2
+
+#define SMSC_BASE_ADDR			0x38
+#define OMAP_GPIO_SMSC_IRQ		151
+
+#define SMSC_MAXGPIO         32
+#define SMSC_BANK(offs)      ((offs) >> 3)
+#define SMSC_BIT(offs)       (1u << ((offs) & 0x7))
+
+struct smsc {
+	struct device *dev;
+	struct i2c_client *i2c_clients[SMSC_NUM_CLIENTS];
+	struct regmap *regmap;
+	int clk;
+	/* Stored chip id */
+	int id;
+};
+
+struct smsc_gpio;
+struct smsc_keypad;
+
+static inline int smsc_read(struct device *child, unsigned int reg,
+	unsigned int *dest)
+{
+	struct smsc     *smsc = dev_get_drvdata(child->parent);
+
+	return regmap_read(smsc->regmap, reg, dest);
+}
+
+static inline int smsc_write(struct device *child, unsigned int reg,
+	unsigned int value)
+{
+	struct smsc     *smsc = dev_get_drvdata(child->parent);
+
+	return regmap_write(smsc->regmap, reg, value);
+}
+
+/* Registers for SMSC */
+#define SMSC_RESET						0xF5
+#define SMSC_GRP_INT						0xF9
+#define SMSC_CLK_CTRL						0xFA
+#define SMSC_WKUP_CTRL						0xFB
+#define SMSC_DEV_ID						0xFC
+#define SMSC_DEV_REV						0xFD
+#define SMSC_VEN_ID_L						0xFE
+#define SMSC_VEN_ID_H						0xFF
+
+/* CLK VALUE */
+#define SMSC_CLK_VALUE						0x13
+
+/* Registers for function GPIO INPUT */
+#define SMSC_GPIO_DATA_IN_START					0x00
+
+/* Registers for function GPIO OUPUT */
+#define SMSC_GPIO_DATA_OUT_START                                       0x05
+
+/* Definitions for SMSC GPIO CONFIGURATION REGISTER*/
+#define SMSC_GPIO_INPUT_LOW					0x01
+#define SMSC_GPIO_INPUT_RISING					0x09
+#define SMSC_GPIO_INPUT_FALLING					0x11
+#define SMSC_GPIO_INPUT_BOTH_EDGE				0x19
+#define SMSC_GPIO_OUTPUT_PP					0x21
+#define SMSC_GPIO_OUTPUT_OP					0x31
+
+#define GRP_INT_STAT						0xf9
+#define	SMSC_GPI_INT						0x0f
+#define SMSC_CFG_START						0x0A
+
+/* Registers for SMSC GPIO INTERRUPT STATUS REGISTER*/
+#define SMSC_GPIO_INT_STAT_START                                  0x32
+
+/* Registers for SMSC GPIO INTERRUPT MASK REGISTER*/
+#define SMSC_GPIO_INT_MASK_START                               0x37
+
+/* Registers for SMSC function KEYPAD*/
+#define SMSC_KP_OUT						0x40
+#define SMSC_KP_IN						0x41
+#define SMSC_KP_INT_STAT					0x42
+#define SMSC_KP_INT_MASK					0x43
+
+/* Definitions for keypad */
+#define SMSC_KP_KSO           0x70
+#define SMSC_KP_KSI           0x51
+#define SMSC_KSO_ALL_LOW        0x20
+#define SMSC_KP_SET_LOW_PWR        0x0B
+#define SMSC_KP_SET_HIGH           0xFF
+#define SMSC_KSO_EVAL           0x00
+
+#endif /*  __LINUX_MFD_SMSC_H */
-- 
1.7.1

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[ABRAHAM, KISHON VIJAY]

Hi,

On Mon, Oct 1, 2012 at 4:31 PM, Sourav Poddar <sourav.poddar-l0cyMroinI0@public.gmane.org> wrote:
> smsc ece1099 is a keyboard scan or gpio expansion device.
> The patch create keypad and gpio expander child for this
> multi function smsc driver.
>
> Tested on omap5430 evm with 3.6-rc6 custom kernel.
Can we have this line in comment section and not in commit log?

>
> Cc: Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: Benoit Cousson <b-cousson-l0cyMroinI0@public.gmane.org>
> Cc: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> Cc: Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Sourav Poddar <sourav.poddar-l0cyMroinI0@public.gmane.org>
> ---
> Changes since v2:
>  - Change the cache type, max register assignment
>  - remove of_device_table, since i2c based device gets
> probed according to i2c_device_id through dt.
>  - Modify the remove function
>  - Minor return patch cleanups.
>  Documentation/smsc_ece1099.txt |   56 ++++++++++++++++++++
>  drivers/mfd/Kconfig            |   12 ++++
>  drivers/mfd/Makefile           |    1 +
>  drivers/mfd/smsc-ece1099.c     |  113 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/smsc.h       |  109 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 291 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/smsc_ece1099.txt
>  create mode 100644 drivers/mfd/smsc-ece1099.c
>  create mode 100644 include/linux/mfd/smsc.h
>
> diff --git a/Documentation/smsc_ece1099.txt b/Documentation/smsc_ece1099.txt
> new file mode 100644
> index 0000000..6b492e8
> --- /dev/null
> +++ b/Documentation/smsc_ece1099.txt
> @@ -0,0 +1,56 @@
> +What is smsc-ece1099?
> +----------------------
> +
> +The ECE1099 is a 40-Pin 3.3V Keyboard Scan Expansion
> +or GPIO Expansion device. The device supports a keyboard
> +scan matrix of 23x8. The device is connected to a Master
> +via the SMSC BC-Link interface or via the SMBus.
> +Keypad scan Input(KSI) and Keypad Scan Output(KSO) signals
> +are multiplexed with GPIOs.
> +
> +Interrupt generation
> +--------------------
> +
> +Interrupts can be generated by an edge detection on a GPIO
> +pin or an edge detection on one of the bus interface pins.
> +Interrupts can also be detected on the keyboard scan interface.
> +The bus interrupt pin (BC_INT# or SMBUS_INT#) is asserted if
> +any bit in one of the Interrupt Status registers is 1 and
> +the corresponding Interrupt Mask bit is also 1.
> +
> +In order for software to determine which device is the source
> +of an interrupt, it should first read the Group Interrupt Status Register
> +to determine which Status register group is a source for the interrupt.
> +Software should read both the Status register and the associated Mask register,
> +then AND the two values together. Bits that are 1 in the result of the AND
> +are active interrupts. Software clears an interrupt by writing a 1 to the
> +corresponding bit in the Status register.
> +
> +Communication Protocol
> +----------------------
> +
> +- SMbus slave Interface
> +       The host processor communicates with the ECE1099 device
> +       through a series of read/write registers via the SMBus
> +       interface. SMBus is a serial communication protocol between
> +       a computer host and its peripheral devices. The SMBus data
> +       rate is 10KHz minimum to 400 KHz maximum
> +
> +- Slave Bus Interface
> +       The ECE1099 device SMBus implementation is a subset of the
> +       SMBus interface to the host. The device is a slave-only SMBus device.
> +       The implementation in the device is a subset of SMBus since it
> +       only supports four protocols.
> +
> +       The Write Byte, Read Byte, Send Byte, and Receive Byte protocols are the
> +       only valid SMBus protocols for the device.
> +
> +- BC-LinkTM Interface
> +       The BC-Link is a proprietary bus that allows communication
> +       between a Master device and a Companion device. The Master
> +       device uses this serial bus to read and write registers
> +       located on the Companion device. The bus comprises three signals,
> +       BC_CLK, BC_DAT and BC_INT#. The Master device always provides the
> +       clock, BC_CLK, and the Companion device is the source for an
> +       independent asynchronous interrupt signal, BC_INT#. The ECE1099
> +       supports BC-Link speeds up to 24MHz.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b1a1462..dedc874 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -385,6 +385,18 @@ config MFD_T7L66XB
>         help
>           Support for Toshiba Mobile IO Controller T7L66XB
>
> +config MFD_SMSC
> +       bool "Support for the SMSC ECE1099 series chips"
> +       depends on I2C=y
> +       select MFD_CORE
> +       select REGMAP_I2C
> +       help
> +        If you say yes here you get support for the
> +        ece1099 chips from SMSC.
> +
> +        To compile this driver as a module, choose M here: the
> +        module will be called smsc.
*bool* attribute for MFD_SMSC, does not allow to select the driver as
module. use *trisate* instead.
> +
>  config MFD_TC6387XB
>         bool "Support Toshiba TC6387XB"
>         depends on ARM && HAVE_CLK
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 79dd22d..3323345 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_EZX_PCAP)                += ezx-pcap.o
>  obj-$(CONFIG_MCP)              += mcp-core.o
>  obj-$(CONFIG_MCP_SA11X0)       += mcp-sa11x0.o
>  obj-$(CONFIG_MCP_UCB1200)      += ucb1x00-core.o
> +obj-$(CONFIG_MFD_SMSC)        += smsc-ece1099.o
>  obj-$(CONFIG_MCP_UCB1200_TS)   += ucb1x00-ts.o
>
>  ifeq ($(CONFIG_SA1100_ASSABET),y)
> diff --git a/drivers/mfd/smsc-ece1099.c b/drivers/mfd/smsc-ece1099.c
> new file mode 100644
> index 0000000..220aec2
> --- /dev/null
> +++ b/drivers/mfd/smsc-ece1099.c
> @@ -0,0 +1,113 @@
> +/*
> + * TI SMSC MFD Driver
> + *
> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com
> + *
> + * Author: Sourav Poddar <sourav.poddar-l0cyMroinI0@public.gmane.org>
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under  the terms of the GNU General  Public License as published by the
> + *  Free Software Foundation;  GPL v2.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +#include <linux/workqueue.h>
> +#include <linux/irq.h>
> +#include <linux/regmap.h>
> +#include <linux/err.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/smsc.h>
> +#include <linux/of_platform.h>
> +
> +static struct regmap_config smsc_regmap_config = {
> +               .reg_bits = 8,
> +               .val_bits = 8,
> +               .max_register = SMSC_VEN_ID_H,
> +               .cache_type = REGCACHE_RBTREE,
> +};
> +
> +static int smsc_i2c_probe(struct i2c_client *i2c,
> +                       const struct i2c_device_id *id)
> +{
> +       struct device_node              *node = i2c->dev.of_node;
> +       struct smsc *smsc;
> +       int devid, rev, venid_l, venid_h;
> +       int ret = 0;
> +
> +       smsc = devm_kzalloc(&i2c->dev, sizeof(struct smsc),
> +                               GFP_KERNEL);
> +       if (!smsc) {
> +               dev_err(&i2c->dev, "smsc mfd driver memory allocation failed\n");
> +               return -ENOMEM;
> +       }
> +
> +       smsc->regmap = devm_regmap_init_i2c(i2c, &smsc_regmap_config);
> +       if (IS_ERR(smsc->regmap)) {
> +               ret = PTR_ERR(smsc->regmap);
> +               goto err;
> +       }
> +
> +       i2c_set_clientdata(i2c, smsc);
> +       smsc->dev = &i2c->dev;
> +
> +#ifdef CONFIG_OF
> +       of_property_read_u32(node, "clock", &smsc->clk);
This functions need not come under macro.
> +#endif
> +
> +       regmap_read(smsc->regmap, SMSC_DEV_ID, &devid);
> +       regmap_read(smsc->regmap, SMSC_DEV_REV, &rev);
> +       regmap_read(smsc->regmap, SMSC_VEN_ID_L, &venid_l);
> +       regmap_read(smsc->regmap, SMSC_VEN_ID_H, &venid_h);
> +
> +       dev_info(&i2c->dev, "SMSCxxx devid: %02x rev: %02x venid: %02x\n",
> +               devid, rev, (venid_h << 8) | venid_l);
dev_info makes the boot noisy.
> +
> +       ret = regmap_write(smsc->regmap, SMSC_CLK_CTRL, smsc->clk);
> +       if (ret)
> +               goto err;
> +
> +#ifdef CONFIG_OF
> +       if (node)
> +               ret = of_platform_populate(node, NULL, NULL, &i2c->dev);
of_platform_populate() need not come under macro.
> +#endif
> +
> +err:
> +       return ret;
> +}
> +
> +static int smsc_i2c_remove(struct i2c_client *i2c)
> +{
> +       struct smsc *smsc = i2c_get_clientdata(i2c);
> +
> +       mfd_remove_devices(smsc->dev);
IMO, this should have resulted in an abort (hint: see
mfd_remove_devices_fn()). Trying to do mfd_remove_devices without
mfd_add_devices(). Use device_for_each_child here to remove the
device.

Thanks
Kishon

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[Mark Brown]

On Mon, Oct 01, 2012 at 05:00:06PM +0530, ABRAHAM, KISHON VIJAY wrote:
> On Mon, Oct 1, 2012 at 4:31 PM, Sourav Poddar <sourav.poddar@ti.com> wrote:

Delete irrelevant context from your replies, it makes it much easier to
find new text.

> > +static int smsc_i2c_remove(struct i2c_client *i2c)
> > +{
> > +       struct smsc *smsc = i2c_get_clientdata(i2c);
> > +
> > +       mfd_remove_devices(smsc->dev);

> IMO, this should have resulted in an abort (hint: see
> mfd_remove_devices_fn()). Trying to do mfd_remove_devices without
> mfd_add_devices(). Use device_for_each_child here to remove the
> device.

No, this is not at all sensible - if there's an mfd_remove_devices()
function we really ought to be able to use it to remove the children.
If we can't do that we should fix the API usability, not open code the
same stuff in every user.

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[ABRAHAM, KISHON VIJAY]

Hi,

On Mon, Oct 1, 2012 at 5:14 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Mon, Oct 01, 2012 at 05:00:06PM +0530, ABRAHAM, KISHON VIJAY wrote:
>> On Mon, Oct 1, 2012 at 4:31 PM, Sourav Poddar <sourav.poddar-l0cyMroinI0@public.gmane.org> wrote:
>
> Delete irrelevant context from your replies, it makes it much easier to
> find new text.
>
>> > +static int smsc_i2c_remove(struct i2c_client *i2c)
>> > +{
>> > +       struct smsc *smsc = i2c_get_clientdata(i2c);
>> > +
>> > +       mfd_remove_devices(smsc->dev);
>
>> IMO, this should have resulted in an abort (hint: see
>> mfd_remove_devices_fn()). Trying to do mfd_remove_devices without
>> mfd_add_devices(). Use device_for_each_child here to remove the
>> device.
>
> No, this is not at all sensible - if there's an mfd_remove_devices()
> function we really ought to be able to use it to remove the children.
> If we can't do that we should fix the API usability, not open code the
> same stuff in every user.

What makes more sense to me is extend mfd_add_devices to create child
devices from dt data. Then for removing devices, one can use
mfd_remove_devices().

Thanks
Kishon

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[Mark Brown]

On Mon, Oct 01, 2012 at 05:23:15PM +0530, ABRAHAM, KISHON VIJAY wrote:
> On Mon, Oct 1, 2012 at 5:14 PM, Mark Brown

> > No, this is not at all sensible - if there's an mfd_remove_devices()
> > function we really ought to be able to use it to remove the children.
> > If we can't do that we should fix the API usability, not open code the
> > same stuff in every user.

> What makes more sense to me is extend mfd_add_devices to create child
> devices from dt data. Then for removing devices, one can use
> mfd_remove_devices().

Why would that be helpful?  Most platforms don't support DT at all, and
having the API behave differently depending on the platform doesn't seem
like a step forwards.  I would really expect any usage of DT to be
totally orthogonal here.

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[ABRAHAM, KISHON VIJAY]

Hi,

On Mon, Oct 1, 2012 at 5:39 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Oct 01, 2012 at 05:23:15PM +0530, ABRAHAM, KISHON VIJAY wrote:
>> On Mon, Oct 1, 2012 at 5:14 PM, Mark Brown
>
>> > No, this is not at all sensible - if there's an mfd_remove_devices()
>> > function we really ought to be able to use it to remove the children.
>> > If we can't do that we should fix the API usability, not open code the
>> > same stuff in every user.
>
>> What makes more sense to me is extend mfd_add_devices to create child
>> devices from dt data. Then for removing devices, one can use
>> mfd_remove_devices().
>
> Why would that be helpful?  Most platforms don't support DT at all, and
I'm not sure how to put it correctly. All I'm trying to tell is mfd is
a framework that exposes a set of API's to create and remove a device
among others. If a mfd child device is not created using mfd API,
it'll be unfair to expect that child be removed properly using mfd
API. Like in this patch, the device is created using
of_platform_populate (not a mfd API) but is removed using
mfd_remove_devices (mfd API) [which should result in an abort].
This means mfd framework does not have an API to create a device from
dt data or so do I think since of_platform_populate() is used. Thats
why I suggested the idea of extending mfd_add_devices() (or adding a
new API in mfd framework) to create child devices from dt data so that
we'll have API's in mfd framework to both create and delete a device.

Thanks
Kishon

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[Mark Brown]

On Mon, Oct 01, 2012 at 08:54:23PM +0530, ABRAHAM, KISHON VIJAY wrote:
> On Mon, Oct 1, 2012 at 5:39 PM, Mark Brown

> > Why would that be helpful?  Most platforms don't support DT at all, and

> I'm not sure how to put it correctly. All I'm trying to tell is mfd is
> a framework that exposes a set of API's to create and remove a device
> among others. If a mfd child device is not created using mfd API,
> it'll be unfair to expect that child be removed properly using mfd
> API. Like in this patch, the device is created using
> of_platform_populate (not a mfd API) but is removed using
> mfd_remove_devices (mfd API) [which should result in an abort].

That doesn't sound terribly clever, no, though it's not immediately
clear to me if the non-clever bit is using mfd_remove_devices() or
of_platform_populate().

> This means mfd framework does not have an API to create a device from
> dt data or so do I think since of_platform_populate() is used. Thats
> why I suggested the idea of extending mfd_add_devices() (or adding a
> new API in mfd framework) to create child devices from dt data so that
> we'll have API's in mfd framework to both create and delete a device.

The trouble that always exists with representing MFD children in DT is
that unless you've got a usefully reusable IP block which is clearly
separate from the chip integration you end up essentially just dumping
the Linux data structures into DT which often doesn't leave you with
something which describes the hardware.

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 1354 bytes --]

Hi,

coming late to the discussion, so bear with me

On Tue, Oct 02, 2012 at 01:38:56PM +0100, Mark Brown wrote:
> > This means mfd framework does not have an API to create a device from
> > dt data or so do I think since of_platform_populate() is used. Thats
> > why I suggested the idea of extending mfd_add_devices() (or adding a
> > new API in mfd framework) to create child devices from dt data so that
> > we'll have API's in mfd framework to both create and delete a device.
> 
> The trouble that always exists with representing MFD children in DT is
> that unless you've got a usefully reusable IP block which is clearly
> separate from the chip integration you end up essentially just dumping
> the Linux data structures into DT which often doesn't leave you with
> something which describes the hardware.

do you mean to say that children creation should be left into the driver
(outside dt) ? That should be doable, indeed.

BTW, OTOH writing all children into the DT actually describes the HW,
no ? And depending on the device I feel it'd be better to write that
data to DT. Think of twlxxxx (TI's PMICs), we might have completely
unrelated drivers using one of TWL's GPIO lines as an interrupt source.

If that particular children isn't listed in DT, it can't be used as an
interrupt-parent, right ?

-- 
balbi

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[Mark Brown]

On Tue, Oct 02, 2012 at 03:43:22PM +0300, Felipe Balbi wrote:

> BTW, OTOH writing all children into the DT actually describes the HW,
> no ? And depending on the device I feel it'd be better to write that

Well, it depends on the hardware.  Some hardware has a bunch of nice,
neat IPs which can usefully be reproduced and which map sensibly onto OS
abstractions but a lot of it doesn't and frequently the abstractions
which Linux wants to use don't bear a huge resemblance to the hardware
(and Linux's ideas can change over time, as with the clock API being
factored out for example).

> data to DT. Think of twlxxxx (TI's PMICs), we might have completely
> unrelated drivers using one of TWL's GPIO lines as an interrupt source.

> If that particular children isn't listed in DT, it can't be used as an
> interrupt-parent, right ?

You can have the interrupt controller there without having to list every
IP in the device, just make the parent device the interrupt controller
to DT.

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 1157 bytes --]

Hi,

On Tue, Oct 02, 2012 at 01:58:20PM +0100, Mark Brown wrote:
> On Tue, Oct 02, 2012 at 03:43:22PM +0300, Felipe Balbi wrote:
> 
> > BTW, OTOH writing all children into the DT actually describes the HW,
> > no ? And depending on the device I feel it'd be better to write that
> 
> Well, it depends on the hardware.  Some hardware has a bunch of nice,
> neat IPs which can usefully be reproduced and which map sensibly onto OS
> abstractions but a lot of it doesn't and frequently the abstractions
> which Linux wants to use don't bear a huge resemblance to the hardware
> (and Linux's ideas can change over time, as with the clock API being
> factored out for example).
> 
> > data to DT. Think of twlxxxx (TI's PMICs), we might have completely
> > unrelated drivers using one of TWL's GPIO lines as an interrupt source.
> 
> > If that particular children isn't listed in DT, it can't be used as an
> > interrupt-parent, right ?
> 
> You can have the interrupt controller there without having to list every
> IP in the device, just make the parent device the interrupt controller
> to DT.

fair enough, thanks ;-)

-- 
balbi

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[ABRAHAM, KISHON VIJAY]

Hi,

On Tue, Oct 2, 2012 at 6:08 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Oct 01, 2012 at 08:54:23PM +0530, ABRAHAM, KISHON VIJAY wrote:
>> On Mon, Oct 1, 2012 at 5:39 PM, Mark Brown
>
>> > Why would that be helpful?  Most platforms don't support DT at all, and
>
>> I'm not sure how to put it correctly. All I'm trying to tell is mfd is
>> a framework that exposes a set of API's to create and remove a device
>> among others. If a mfd child device is not created using mfd API,
>> it'll be unfair to expect that child be removed properly using mfd
>> API. Like in this patch, the device is created using
>> of_platform_populate (not a mfd API) but is removed using
>> mfd_remove_devices (mfd API) [which should result in an abort].
>
> That doesn't sound terribly clever, no, though it's not immediately
> clear to me if the non-clever bit is using mfd_remove_devices() or
> of_platform_populate().
>
>> This means mfd framework does not have an API to create a device from
>> dt data or so do I think since of_platform_populate() is used. Thats
>> why I suggested the idea of extending mfd_add_devices() (or adding a
>> new API in mfd framework) to create child devices from dt data so that
>> we'll have API's in mfd framework to both create and delete a device.
>
> The trouble that always exists with representing MFD children in DT is
> that unless you've got a usefully reusable IP block which is clearly
> separate from the chip integration you end up essentially just dumping
> the Linux data structures into DT which often doesn't leave you with
> something which describes the hardware.

indeed!

Thanks
Kishon

Re: [PATCHv3 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver

[Samuel Ortiz]

Hi Sourav,

On Mon, Oct 01, 2012 at 04:31:22PM +0530, Sourav Poddar wrote:
> smsc ece1099 is a keyboard scan or gpio expansion device.
> The patch create keypad and gpio expander child for this
> multi function smsc driver.
> 
> Tested on omap5430 evm with 3.6-rc6 custom kernel.
> 
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> Changes since v2:
>  - Change the cache type, max register assignment
>  - remove of_device_table, since i2c based device gets 
> probed according to i2c_device_id through dt.
>  - Modify the remove function
>  - Minor return patch cleanups.
>  Documentation/smsc_ece1099.txt |   56 ++++++++++++++++++++
>  drivers/mfd/Kconfig            |   12 ++++
>  drivers/mfd/Makefile           |    1 +
>  drivers/mfd/smsc-ece1099.c     |  113 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/smsc.h       |  109 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 291 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/smsc_ece1099.txt
>  create mode 100644 drivers/mfd/smsc-ece1099.c
>  create mode 100644 include/linux/mfd/smsc.h
Applied with a warning fix for the !CONFIG_OF case.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/