linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
@ 2012-09-05 11:36 Sourav Poddar
  2012-09-05 17:53 ` Vaibhav Hiremath
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Sourav Poddar @ 2012-09-05 11:36 UTC (permalink / raw)
  To: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
	linux-input, sourav.poddar
  Cc: b-cousson, balbi, santosh.shilimkar, sameo

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

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 v1:
 - Use Kconfig option correctly
 - Add regmap_config paramters
 - Modify formatting of logs for devid
 - Move read/write function to headed file as an inline
   function.
 Documentation/smsc_ece1099.txt |   56 ++++++++++++++++++++
 drivers/mfd/Kconfig            |   12 ++++
 drivers/mfd/Makefile           |    1 +
 drivers/mfd/smsc-ece1099.c     |  110 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/smsc.h       |  111 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 290 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 d1facef..991ef15 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..f587d91 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.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..73a6cb7
--- /dev/null
+++ b/drivers/mfd/smsc-ece1099.c
@@ -0,0 +1,110 @@
+/*
+ * 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_MAX_REGISTER - 1;
+		.cache_type = REGCACHE_COMPRESSED,
+};
+
+static const struct of_device_id of_smsc_match[] = {
+	{ .compatible = "smsc,ece1099", },
+	{ },
+};
+
+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 ret = 0;
+
+	smsc = devm_kzalloc(&i2c->dev, sizeof(struct smsc),
+				GFP_KERNEL);
+
+	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;
+
+	if (node)
+		ret = of_platform_populate(node, NULL, NULL, &i2c->dev);
+
+	return ret;
+
+err:
+	return ret;
+}
+
+static int smsc_i2c_remove(struct i2c_client *i2c)
+{
+	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",
+		   .of_match_table = of_smsc_match,
+		   .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..adfaec2
--- /dev/null
+++ b/include/linux/mfd/smsc.h
@@ -0,0 +1,111 @@
+/*
+ * 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
+
+#define SMSC_MAX_REG					 (SMSC_VEN_ID_H + 1)
+
+/* 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
  2012-09-05 11:36 [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver Sourav Poddar
@ 2012-09-05 17:53 ` Vaibhav Hiremath
  2012-09-06  5:50   ` Poddar, Sourav
  2012-09-08  3:12 ` Mark Brown
  2012-09-25 17:52 ` Mark Brown
  2 siblings, 1 reply; 6+ messages in thread
From: Vaibhav Hiremath @ 2012-09-05 17:53 UTC (permalink / raw)
  To: Sourav Poddar
  Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
	linux-input, santosh.shilimkar, b-cousson, balbi, sameo



On 9/5/2012 5:06 PM, 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.
> 
> 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 v1:
>  - Use Kconfig option correctly
>  - Add regmap_config paramters
>  - Modify formatting of logs for devid
>  - Move read/write function to headed file as an inline
>    function.
>  Documentation/smsc_ece1099.txt |   56 ++++++++++++++++++++
>  drivers/mfd/Kconfig            |   12 ++++
>  drivers/mfd/Makefile           |    1 +
>  drivers/mfd/smsc-ece1099.c     |  110 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/smsc.h       |  111 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 290 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 d1facef..991ef15 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..f587d91 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.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..73a6cb7
> --- /dev/null
> +++ b/drivers/mfd/smsc-ece1099.c
> @@ -0,0 +1,110 @@
> +/*
> + * 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_MAX_REGISTER - 1;
> +		.cache_type = REGCACHE_COMPRESSED,
> +};

You can remove one extra tab here.

> +
> +static const struct of_device_id of_smsc_match[] = {
> +	{ .compatible = "smsc,ece1099", },
> +	{ },
> +};
> +
> +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 ret = 0;
> +
> +	smsc = devm_kzalloc(&i2c->dev, sizeof(struct smsc),
> +				GFP_KERNEL);
> +

You must check the return value here?

> +	smsc->regmap = devm_regmap_init_i2c(i2c, &smsc_regmap_config);
> +	if (IS_ERR(smsc->regmap)) {
> +		ret = PTR_ERR(smsc->regmap);
> +		goto err;

You can simply return from here right?

> +	}
> +
> +	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);
> +

You also want to check return value here.

> +	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;
> +
> +	if (node)
> +		ret = of_platform_populate(node, NULL, NULL, &i2c->dev);
> +

of_xxx above is encapsulated in CONFIG_OF macro, same is not done here.

> +	return ret;
> +

This return is not required, as next instruction. Infact I would suggest
you can simply return from each places above.

Thanks,
Vaibhav
> +err:
> +	return ret;
> +}
> +
> +static int smsc_i2c_remove(struct i2c_client *i2c)
> +{
> +	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",
> +		   .of_match_table = of_smsc_match,
> +		   .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..adfaec2
> --- /dev/null
> +++ b/include/linux/mfd/smsc.h
> @@ -0,0 +1,111 @@
> +/*
> + * 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
> +
> +#define SMSC_MAX_REG					 (SMSC_VEN_ID_H + 1)
> +
> +/* 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 */
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
  2012-09-05 17:53 ` Vaibhav Hiremath
@ 2012-09-06  5:50   ` Poddar, Sourav
  0 siblings, 0 replies; 6+ messages in thread
From: Poddar, Sourav @ 2012-09-06  5:50 UTC (permalink / raw)
  To: Vaibhav Hiremath
  Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
	linux-input, santosh.shilimkar, b-cousson, balbi, sameo

Hi,

On Wed, Sep 5, 2012 at 11:23 PM, Vaibhav Hiremath <hvaibhav@ti.com> wrote:
>
>
> On 9/5/2012 5:06 PM, 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.
>>
>> 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 v1:
>>  - Use Kconfig option correctly
>>  - Add regmap_config paramters
>>  - Modify formatting of logs for devid
>>  - Move read/write function to headed file as an inline
>>    function.
>>  Documentation/smsc_ece1099.txt |   56 ++++++++++++++++++++
>>  drivers/mfd/Kconfig            |   12 ++++
>>  drivers/mfd/Makefile           |    1 +
>>  drivers/mfd/smsc-ece1099.c     |  110 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/smsc.h       |  111 ++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 290 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 d1facef..991ef15 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..f587d91 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.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..73a6cb7
>> --- /dev/null
>> +++ b/drivers/mfd/smsc-ece1099.c
>> @@ -0,0 +1,110 @@
>> +/*
>> + * 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_MAX_REGISTER - 1;
>> +             .cache_type = REGCACHE_COMPRESSED,
>> +};
>
> You can remove one extra tab here.
>
Ok.
>> +
>> +static const struct of_device_id of_smsc_match[] = {
>> +     { .compatible = "smsc,ece1099", },
>> +     { },
>> +};
>> +
>> +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 ret = 0;
>> +
>> +     smsc = devm_kzalloc(&i2c->dev, sizeof(struct smsc),
>> +                             GFP_KERNEL);
>> +
>
> You must check the return value here?
>
Yes, will include the check.
>> +     smsc->regmap = devm_regmap_init_i2c(i2c, &smsc_regmap_config);
>> +     if (IS_ERR(smsc->regmap)) {
>> +             ret = PTR_ERR(smsc->regmap);
>> +             goto err;
>
> You can simply return from here right?
>
hmmm, yes.
>> +     }
>> +
>> +     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);
>> +
>
> You also want to check return value here.
>
I dont see any point checking for the above ret values. These values
are read during
runtime and some  can be zero also. ?
>> +     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;
>> +
>> +     if (node)
>> +             ret = of_platform_populate(node, NULL, NULL, &i2c->dev);
>> +
>
> of_xxx above is encapsulated in CONFIG_OF macro, same is not done here.
>
Will include.
>> +     return ret;
>> +
>
> This return is not required, as next instruction. Infact I would suggest
> you can simply return from each places above.
>
Ok.
> Thanks,
> Vaibhav
>> +err:
>> +     return ret;
>> +}
>> +
>> +static int smsc_i2c_remove(struct i2c_client *i2c)
>> +{
>> +     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",
>> +                .of_match_table = of_smsc_match,
>> +                .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..adfaec2
>> --- /dev/null
>> +++ b/include/linux/mfd/smsc.h
>> @@ -0,0 +1,111 @@
>> +/*
>> + * 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
>> +
>> +#define SMSC_MAX_REG                                  (SMSC_VEN_ID_H + 1)
>> +
>> +/* 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 */
>>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
  2012-09-05 11:36 [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver Sourav Poddar
  2012-09-05 17:53 ` Vaibhav Hiremath
@ 2012-09-08  3:12 ` Mark Brown
  2012-09-25 17:52 ` Mark Brown
  2 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2012-09-08  3:12 UTC (permalink / raw)
  To: Sourav Poddar
  Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
	linux-input, b-cousson, balbi, santosh.shilimkar, sameo

On Wed, Sep 05, 2012 at 05:06:04PM +0530, Sourav Poddar wrote:

> +static struct regmap_config smsc_regmap_config = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +		.max_register = SMSC_MAX_REGISTER - 1;

That max_register setup looks very odd...

> +		.cache_type = REGCACHE_COMPRESSED,
> +};

Are you sure the compressed type is sensible?  It would normally only
make sense with a large number of closely packed registers but this
device has 8 bit register values.

> +#ifdef CONFIG_OF
> +	of_property_read_u32(node, "clock", &smsc->clk);
> +#endif

> +	ret = regmap_write(smsc->regmap, SMSC_CLK_CTRL, smsc->clk);
> +	if (ret)
> +		goto err;

What happens on non-DT systems?

> +static int smsc_i2c_remove(struct i2c_client *i2c)
> +{
> +	return 0;
> +}

Remove empty functions, though it's rather surprising that there's
nothing at all to do here..  Normally an MFD would at least remove its
children.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
  2012-09-05 11:36 [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver Sourav Poddar
  2012-09-05 17:53 ` Vaibhav Hiremath
  2012-09-08  3:12 ` Mark Brown
@ 2012-09-25 17:52 ` Mark Brown
  2012-09-25 18:01   ` Poddar, Sourav
  2 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2012-09-25 17:52 UTC (permalink / raw)
  To: Sourav Poddar
  Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
	linux-input, b-cousson, balbi, santosh.shilimkar, sameo

On Wed, Sep 05, 2012 at 05:06:04PM +0530, Sourav Poddar wrote:

> +static struct regmap_config smsc_regmap_config = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +		.max_register = SMSC_MAX_REGISTER - 1;
> +		.cache_type = REGCACHE_COMPRESSED,
> +};

That definition of max_register looks wrong - why are we subtracting 1
from a macro called MAX_REGISTER to get it?

Indentation here is a bit odd too.

> +static int smsc_i2c_remove(struct i2c_client *i2c)
> +{
> +	return 0;
> +}

Remove empty functions.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
  2012-09-25 17:52 ` Mark Brown
@ 2012-09-25 18:01   ` Poddar, Sourav
  0 siblings, 0 replies; 6+ messages in thread
From: Poddar, Sourav @ 2012-09-25 18:01 UTC (permalink / raw)
  To: Mark Brown
  Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
	linux-input, b-cousson, balbi, santosh.shilimkar, sameo

Hi,

On Tue, Sep 25, 2012 at 11:22 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Sep 05, 2012 at 05:06:04PM +0530, Sourav Poddar wrote:
>
>> +static struct regmap_config smsc_regmap_config = {
>> +             .reg_bits = 8,
>> +             .val_bits = 8,
>> +             .max_register = SMSC_MAX_REGISTER - 1;
>> +             .cache_type = REGCACHE_COMPRESSED,
>> +};
>
> That definition of max_register looks wrong - why are we subtracting 1
> from a macro called MAX_REGISTER to get it?
>
Yes, my bad.
Actually, I have define in .h file something like this..
#define SMSC_MAX_REG                                    (SMSC_VEN_ID_H
+ 1) where
SMSC_VEN_ID_H is the last register address which this chip supports.

I think I should directly assign max_address to SMSC_VEN_ID_H.   ?
+
+
> Indentation here is a bit odd too.
>
Will rectify.
>> +static int smsc_i2c_remove(struct i2c_client *i2c)
>> +{
>> +     return 0;
>> +}
>
> Remove empty functions.
Ok.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-09-25 18:01 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-05 11:36 [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver Sourav Poddar
2012-09-05 17:53 ` Vaibhav Hiremath
2012-09-06  5:50   ` Poddar, Sourav
2012-09-08  3:12 ` Mark Brown
2012-09-25 17:52 ` Mark Brown
2012-09-25 18:01   ` Poddar, Sourav

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).