* [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
2012-08-21 10:45 [PATCH 0/4] Add mfd driver for smsc-ece1099 chip Sourav Poddar
@ 2012-08-21 10:45 ` Sourav Poddar
2012-08-21 12:41 ` Mark Brown
2012-08-21 10:45 ` [PATCH 2/4] Input: keypad: Add smsc ece1099 keypad driver Sourav Poddar
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: Sourav Poddar @ 2012-08-21 10:45 UTC (permalink / raw)
To: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input, 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.
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>
---
Documentation/smsc_ece1099.txt | 56 ++++++++++++++++
drivers/mfd/Kconfig | 10 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/smsc-ece1099.c | 139 ++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/smsc.h | 89 +++++++++++++++++++++++++
5 files changed, 295 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..3b81c17 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -385,6 +385,16 @@ 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 && MFD_CORE && 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..6ba06b8
--- /dev/null
+++ b/drivers/mfd/smsc-ece1099.c
@@ -0,0 +1,139 @@
+/*
+ * 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>
+
+struct smsc {
+ struct device *dev;
+ struct i2c_client *i2c_clients[SMSC_NUM_CLIENTS];
+ struct regmap *regmap;
+ int clk;
+ /* Stored chip id */
+ int id;
+};
+
+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);
+}
+EXPORT_SYMBOL_GPL(smsc_read);
+
+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);
+}
+EXPORT_SYMBOL_GPL(smsc_write);
+
+static struct regmap_config smsc_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .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;
+
+ of_property_read_u32(node, "clock", &smsc->clk);
+
+ regmap_read(smsc->regmap, SMSC_DEV_ID, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
+
+ regmap_read(smsc->regmap, SMSC_DEV_REV, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
+
+ regmap_read(smsc->regmap, SMSC_VEN_ID_L, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
+
+ regmap_read(smsc->regmap, SMSC_VEN_ID_H, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
+
+ 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:
+ kfree(smsc);
+ return ret;
+}
+
+static int smsc_i2c_remove(struct i2c_client *i2c)
+{
+ return 0;
+}
+
+static const struct i2c_device_id smsc_i2c_id[] = {
+ { "smsc", 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..0b048c6
--- /dev/null
+++ b/include/linux/mfd/smsc.h
@@ -0,0 +1,89 @@
+/*
+ * 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_gpio;
+struct smsc_keypad;
+
+int smsc_read(struct device *child, unsigned int reg,
+ unsigned int *dest);
+int smsc_write(struct device *child, unsigned int reg,
+ unsigned int 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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
2012-08-21 10:45 ` [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver Sourav Poddar
@ 2012-08-21 12:41 ` Mark Brown
2012-08-21 12:42 ` Felipe Balbi
0 siblings, 1 reply; 26+ messages in thread
From: Mark Brown @ 2012-08-21 12:41 UTC (permalink / raw)
To: Sourav Poddar
Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input
On Tue, Aug 21, 2012 at 04:15:37PM +0530, Sourav Poddar wrote:
> +config MFD_SMSC
> + bool "Support for the SMSC ECE1099 series chips"
> + depends on I2C=y && MFD_CORE && REGMAP_I2C
This needs to select REGMAP_I2C not depend on it. REGMAP_I2C will only
be enabled by being selected.
> +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);
> +}
> +EXPORT_SYMBOL_GPL(smsc_read);
I'd suggest making these static inlines in the header given that they're
so trivial.
> +static struct regmap_config smsc_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .cache_type = REGCACHE_COMPRESSED,
> +};
Indentation is weird here. For the cache we should have at least
.max_register defined and given the functionality there must surely be
some volatile registers (I'm surprised this works at all as it is, the
cache should break things).
> + of_property_read_u32(node, "clock", &smsc->clk);
> +
This is all unconditional, there should be a dependency on this in
Kconfig.
> + regmap_read(smsc->regmap, SMSC_DEV_ID, &ret);
> + dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
I'd make these log messages dev_info() or something.
> +err:
> + kfree(smsc);
Use devm_kzalloc() for this.
> +static const struct i2c_device_id smsc_i2c_id[] = {
> + { "smsc", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(i2c, smsc_i2c_id);
This should probably list the part name rather than "smsc" - that seems
far too generic a name to use, obviously smsc produce more than one
part!
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
2012-08-21 12:41 ` Mark Brown
@ 2012-08-21 12:42 ` Felipe Balbi
2012-08-21 13:22 ` Mark Brown
0 siblings, 1 reply; 26+ messages in thread
From: Felipe Balbi @ 2012-08-21 12:42 UTC (permalink / raw)
To: Mark Brown
Cc: Sourav Poddar, devicetree-discuss, linux-arm-kernel, linux-omap,
linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 403 bytes --]
On Tue, Aug 21, 2012 at 01:41:46PM +0100, Mark Brown wrote:
> > + regmap_read(smsc->regmap, SMSC_DEV_ID, &ret);
> > + dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
>
> I'd make these log messages dev_info() or something.
dev_info() ? It'lll just make boot noisier for no good reason. Which
user wants to see this during boot up ? That's a debugging feature for
develop IMHO.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
2012-08-21 12:42 ` Felipe Balbi
@ 2012-08-21 13:22 ` Mark Brown
2012-08-21 13:27 ` Felipe Balbi
0 siblings, 1 reply; 26+ messages in thread
From: Mark Brown @ 2012-08-21 13:22 UTC (permalink / raw)
To: Felipe Balbi
Cc: Sourav Poddar, devicetree-discuss, linux-arm-kernel, linux-omap,
linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 858 bytes --]
On Tue, Aug 21, 2012 at 03:42:45PM +0300, Felipe Balbi wrote:
> On Tue, Aug 21, 2012 at 01:41:46PM +0100, Mark Brown wrote:
> > > + regmap_read(smsc->regmap, SMSC_DEV_ID, &ret);
> > > + dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
> > I'd make these log messages dev_info() or something.
> dev_info() ? It'lll just make boot noisier for no good reason. Which
> user wants to see this during boot up ? That's a debugging feature for
> develop IMHO.
Most of the registers appeared to be chip revision information which is
most definitely useful to tell people about, though possibly with neater
formatting ("why is this batch of boards failing... oh, right"). If
they're fixed device IDs then the driver should instead be verifying
that the registers contain the expected values and bombing out if they
don't. Either way dev_dbg() isn't too helpful.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
2012-08-21 13:22 ` Mark Brown
@ 2012-08-21 13:27 ` Felipe Balbi
2012-08-21 13:49 ` Mark Brown
0 siblings, 1 reply; 26+ messages in thread
From: Felipe Balbi @ 2012-08-21 13:27 UTC (permalink / raw)
To: Mark Brown
Cc: Felipe Balbi, Sourav Poddar, devicetree-discuss,
linux-arm-kernel, linux-omap, linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]
On Tue, Aug 21, 2012 at 02:22:22PM +0100, Mark Brown wrote:
> On Tue, Aug 21, 2012 at 03:42:45PM +0300, Felipe Balbi wrote:
> > On Tue, Aug 21, 2012 at 01:41:46PM +0100, Mark Brown wrote:
> > > > + regmap_read(smsc->regmap, SMSC_DEV_ID, &ret);
> > > > + dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
>
> > > I'd make these log messages dev_info() or something.
>
> > dev_info() ? It'lll just make boot noisier for no good reason. Which
> > user wants to see this during boot up ? That's a debugging feature for
> > develop IMHO.
>
> Most of the registers appeared to be chip revision information which is
> most definitely useful to tell people about, though possibly with neater
> formatting ("why is this batch of boards failing... oh, right"). If
> they're fixed device IDs then the driver should instead be verifying
> that the registers contain the expected values and bombing out if they
> don't. Either way dev_dbg() isn't too helpful.
I still beg to differ. Even if it fails, dmesg will still contain the
message (provided you have it enabled). I really don't think we want
this to print to console on every boot.
If you're still testing your new batch of boards, you're not just a
simple user and you will have debugging enabled anyway. dev_info() will
be visible to anyone who's got a console running. Not sure how useful
that would be to my neighbor.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
2012-08-21 13:27 ` Felipe Balbi
@ 2012-08-21 13:49 ` Mark Brown
2012-08-21 13:52 ` Felipe Balbi
0 siblings, 1 reply; 26+ messages in thread
From: Mark Brown @ 2012-08-21 13:49 UTC (permalink / raw)
To: Felipe Balbi
Cc: Sourav Poddar, devicetree-discuss, linux-arm-kernel, linux-omap,
linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 991 bytes --]
On Tue, Aug 21, 2012 at 04:27:44PM +0300, Felipe Balbi wrote:
> I still beg to differ. Even if it fails, dmesg will still contain the
> message (provided you have it enabled). I really don't think we want
> this to print to console on every boot.
Only if it's enabled which is the trick...
> If you're still testing your new batch of boards, you're not just a
> simple user and you will have debugging enabled anyway. dev_info() will
> be visible to anyone who's got a console running. Not sure how useful
> that would be to my neighbor.
Also think about hobbyists and so on, and ideally at some point the
people using distros. We shouldn't be requiring kernel rebuilds for
this sort of diagnostic information. I guess sysfs is another option
but frankly the overhead on boot just doesn't seem meaningful in the
context of the overall kernel boot style - I'd really expect people who
are bothered by this sort of output would be raising the minimum log
level appearing on the console.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
2012-08-21 13:49 ` Mark Brown
@ 2012-08-21 13:52 ` Felipe Balbi
2012-08-21 14:08 ` Mark Brown
0 siblings, 1 reply; 26+ messages in thread
From: Felipe Balbi @ 2012-08-21 13:52 UTC (permalink / raw)
To: Mark Brown
Cc: Felipe Balbi, Sourav Poddar, devicetree-discuss,
linux-arm-kernel, linux-omap, linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]
On Tue, Aug 21, 2012 at 02:49:37PM +0100, Mark Brown wrote:
> On Tue, Aug 21, 2012 at 04:27:44PM +0300, Felipe Balbi wrote:
>
> > I still beg to differ. Even if it fails, dmesg will still contain the
> > message (provided you have it enabled). I really don't think we want
> > this to print to console on every boot.
>
> Only if it's enabled which is the trick...
>
> > If you're still testing your new batch of boards, you're not just a
> > simple user and you will have debugging enabled anyway. dev_info() will
> > be visible to anyone who's got a console running. Not sure how useful
> > that would be to my neighbor.
>
> Also think about hobbyists and so on, and ideally at some point the
> people using distros. We shouldn't be requiring kernel rebuilds for
> this sort of diagnostic information. I guess sysfs is another option
but we don't. We have dynamic printk for that, right ?
> but frankly the overhead on boot just doesn't seem meaningful in the
> context of the overall kernel boot style - I'd really expect people who
> are bothered by this sort of output would be raising the minimum log
> level appearing on the console.
Well, if you consider a single driver then surely it doesn't make a
difference. But when you add many drivers, each with its own dev_info()
output, it can delay bootup rather significantly, actually.
Fair enough, we have "quiet", but I'm not sure that's enough argument to
allow any simple driver to start poluting dmesg with whatever random
messages.
my 2 cents
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
2012-08-21 13:52 ` Felipe Balbi
@ 2012-08-21 14:08 ` Mark Brown
2012-08-21 14:09 ` Felipe Balbi
0 siblings, 1 reply; 26+ messages in thread
From: Mark Brown @ 2012-08-21 14:08 UTC (permalink / raw)
To: Felipe Balbi
Cc: Sourav Poddar, devicetree-discuss, linux-arm-kernel, linux-omap,
linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 548 bytes --]
On Tue, Aug 21, 2012 at 04:52:41PM +0300, Felipe Balbi wrote:
> Fair enough, we have "quiet", but I'm not sure that's enough argument to
> allow any simple driver to start poluting dmesg with whatever random
> messages.
I think if the driver is just logging to say "I'm running" that's noise
and I do push back on that routinely myself; if the driver is providing
information it's discovered from the running system then that seems much
more useful and we should have a sensible way of getting that out in a
place where users are likely to find.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
2012-08-21 14:08 ` Mark Brown
@ 2012-08-21 14:09 ` Felipe Balbi
2012-08-21 14:20 ` Mark Brown
0 siblings, 1 reply; 26+ messages in thread
From: Felipe Balbi @ 2012-08-21 14:09 UTC (permalink / raw)
To: Mark Brown
Cc: Felipe Balbi, Sourav Poddar, devicetree-discuss,
linux-arm-kernel, linux-omap, linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]
Hi,
On Tue, Aug 21, 2012 at 03:08:03PM +0100, Mark Brown wrote:
> On Tue, Aug 21, 2012 at 04:52:41PM +0300, Felipe Balbi wrote:
>
> > Fair enough, we have "quiet", but I'm not sure that's enough argument to
> > allow any simple driver to start poluting dmesg with whatever random
> > messages.
>
> I think if the driver is just logging to say "I'm running" that's noise
> and I do push back on that routinely myself; if the driver is providing
> information it's discovered from the running system then that seems much
> more useful and we should have a sensible way of getting that out in a
> place where users are likely to find.
fair enough. But look at the messages which that driver is printing:
+ regmap_read(smsc->regmap, SMSC_DEV_ID, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
+
+ regmap_read(smsc->regmap, SMSC_DEV_REV, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
+
+ regmap_read(smsc->regmap, SMSC_VEN_ID_L, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
+
+ regmap_read(smsc->regmap, SMSC_VEN_ID_H, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
You can't possibly understand what that'll print. First of all, VEN_ID_H
and VEN_ID_L should be ORed together. Second, the user will see the same
message four times in a row, with different values, but see that driver
claims that all four values refer to the device id. What this should do,
is at least combine all four messages into a single one of the format:
dev_(dbg|info)(&i2c->dev, "SMSCxxx devid: %02x rev: %02x venid: %02x\n",
devid, rev, (venid_h << 8) | venid_l);
or something similar.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver
2012-08-21 14:09 ` Felipe Balbi
@ 2012-08-21 14:20 ` Mark Brown
0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2012-08-21 14:20 UTC (permalink / raw)
To: Felipe Balbi
Cc: Sourav Poddar, devicetree-discuss, linux-arm-kernel, linux-omap,
linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 808 bytes --]
On Tue, Aug 21, 2012 at 05:09:24PM +0300, Felipe Balbi wrote:
> You can't possibly understand what that'll print. First of all, VEN_ID_H
> and VEN_ID_L should be ORed together. Second, the user will see the same
> message four times in a row, with different values, but see that driver
> claims that all four values refer to the device id. What this should do,
> is at least combine all four messages into a single one of the format:
> dev_(dbg|info)(&i2c->dev, "SMSCxxx devid: %02x rev: %02x venid: %02x\n",
> devid, rev, (venid_h << 8) | venid_l);
> or something similar.
Yes, I agree that the formatting here should also be improved - as I
said in one of my earlier mails if any of these things are fixed things
that should never change the driver should instead verify the value
rather than log it.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/4] Input: keypad: Add smsc ece1099 keypad driver
2012-08-21 10:45 [PATCH 0/4] Add mfd driver for smsc-ece1099 chip Sourav Poddar
2012-08-21 10:45 ` [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver Sourav Poddar
@ 2012-08-21 10:45 ` Sourav Poddar
2012-08-21 10:45 ` Felipe Balbi
2012-08-21 10:46 ` Felipe Balbi
2012-08-21 10:45 ` [PATCH 3/4] arm/dts: omap5-evm: Add keypad support Sourav Poddar
2012-08-21 10:45 ` [RFC/PATCH 4/4] gpio: smscece: Add support for gpio IO expander feature Sourav Poddar
3 siblings, 2 replies; 26+ messages in thread
From: Sourav Poddar @ 2012-08-21 10:45 UTC (permalink / raw)
To: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input, sourav.poddar
From: G, Manjunath Kondaiah <manjugk@ti.com>
SMSC ECE1099 is a keyboard scan or GPIO expansion device.The device
supports a keypad scan matrix of 23*8.This driver uses this
device as a keypad driver.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
drivers/input/keyboard/Kconfig | 11 +
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/smsc-ece1099-keypad.c | 308 ++++++++++++++++++++++++++
3 files changed, 320 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/keyboard/smsc-ece1099-keypad.c
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index c50fa75..2a2d374 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -593,6 +593,17 @@ config KEYBOARD_TWL4030
To compile this driver as a module, choose M here: the
module will be called twl4030_keypad.
+config KEYBOARD_SMSC
+ tristate "SMSC ECE1099 keypad support"
+ depends on I2C=y
+ help
+ Say Y here if your board use the smsc keypad controller
+ for omap5 defconfig. It's safe to say enable this
+ even on boards that don't use the keypad controller.
+
+ To compile this driver as a module, choose M here: the
+ module will be called smsc-ece1099-keypad.
+
config KEYBOARD_XTKBD
tristate "XT keyboard"
select SERIO
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 44e7600..0f2aa26 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -52,5 +52,6 @@ obj-$(CONFIG_KEYBOARD_TC3589X) += tc3589x-keypad.o
obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
obj-$(CONFIG_KEYBOARD_TNETV107X) += tnetv107x-keypad.o
obj-$(CONFIG_KEYBOARD_TWL4030) += twl4030_keypad.o
+obj-$(CONFIG_KEYBOARD_SMSC) += smsc-ece1099-keypad.o
obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o
obj-$(CONFIG_KEYBOARD_W90P910) += w90p910_keypad.o
diff --git a/drivers/input/keyboard/smsc-ece1099-keypad.c b/drivers/input/keyboard/smsc-ece1099-keypad.c
new file mode 100644
index 0000000..8a77878
--- /dev/null
+++ b/drivers/input/keyboard/smsc-ece1099-keypad.c
@@ -0,0 +1,308 @@
+/*
+ * SMSC_ECE1099 Keypad driver
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program 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.
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/delay.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/smsc.h>
+#include <linux/of_gpio.h>
+#include <linux/of.h>
+
+#define KEYPRESS_TIME 200
+
+struct smsc_keypad {
+ struct smsc *smsc;
+ struct matrix_keymap_data *keymap_data;
+ unsigned int last_key_state[16];
+ unsigned int last_col;
+ unsigned int last_key_ms[16];
+ unsigned short *keymap;
+ struct i2c_client *client;
+ struct input_dev *input;
+ int rows, cols;
+ int row_shift;
+ bool no_autorepeat;
+ unsigned irq;
+ struct device *dev;
+};
+
+static void smsc_kp_scan(struct smsc_keypad *kp)
+{
+ struct input_dev *input = kp->input;
+ int i, j;
+ int row, col;
+ int temp, code;
+ unsigned int new_state[16];
+ unsigned int bits_changed;
+ int this_ms;
+
+ smsc_write(kp->dev, SMSC_KP_INT_MASK, 0x00);
+ smsc_write(kp->dev, SMSC_KP_INT_STAT, 0xFF);
+
+ /* Scan for row and column */
+ for (i = 0; i < kp->cols; i++) {
+ smsc_write(kp->dev, SMSC_KP_OUT, SMSC_KSO_EVAL + i);
+ /* Read Row Status */
+ smsc_read(kp->dev, SMSC_KP_IN, &temp);
+ if (temp == 0xFF)
+ continue;
+
+ col = i;
+ for (j = 0; j < kp->rows; j++) {
+ if ((temp & 0x01) != 0x00) {
+ temp = temp >> 1;
+ continue;
+ }
+
+ row = j;
+ new_state[col] = (1 << row);
+ bits_changed = kp->last_key_state[col] ^ new_state[col];
+ this_ms = jiffies_to_msecs(jiffies);
+ if (bits_changed != 0 || (!bits_changed &&
+ ((this_ms - kp->last_key_ms[col]) >= KEYPRESS_TIME))) {
+ code = MATRIX_SCAN_CODE(row, col, kp->row_shift);
+ input_event(input, EV_MSC, MSC_SCAN, code);
+ input_report_key(input, kp->keymap[code], 1);
+ input_report_key(input, kp->keymap[code], 0);
+ kp->last_key_state[col] = new_state[col];
+ if (kp->last_col != col)
+ kp->last_key_state[kp->last_col] = 0;
+ kp->last_key_ms[col] = this_ms;
+ }
+ temp = temp >> 1;
+ }
+ }
+ input_sync(input);
+
+ smsc_write(kp->dev, SMSC_KP_INT_MASK, 0xFF);
+
+ /* Set up Low Power Mode (Wake-up) (0xFB) */
+ smsc_write(kp->dev, SMSC_WKUP_CTRL, SMSC_KP_SET_LOW_PWR);
+
+ /*Enable Keypad Scan (generate interrupt on key press) (0x40)*/
+ smsc_write(kp->dev, SMSC_KP_OUT, SMSC_KSO_ALL_LOW);
+}
+
+static irqreturn_t do_kp_irq(int irq, void *_kp)
+{
+ struct smsc_keypad *kp = _kp;
+ int int_status;
+
+ smsc_read(kp->dev, SMSC_KP_INT_STAT, &int_status);
+ if (int_status)
+ smsc_kp_scan(kp);
+
+ return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_OF
+static int __devinit smsc_keypad_parse_dt(struct device *dev,
+ struct smsc_keypad *kp)
+{
+ struct device_node *np = dev->of_node;
+
+ if (!np) {
+ dev_err(dev, "missing DT data");
+ return -EINVAL;
+ }
+
+ of_property_read_u32(np, "keypad,num-rows", &kp->rows);
+ of_property_read_u32(np, "keypad,num-columns", &kp->cols);
+ if (!kp->rows || !kp->cols) {
+ dev_err(dev, "number of keypad rows/columns not specified\n");
+ return -EINVAL;
+ }
+
+ if (of_get_property(np, "linux,input-no-autorepeat", NULL))
+ kp->no_autorepeat = true;
+
+ return 0;
+}
+#else
+static inline int smsc_keypad_parse_dt(struct device *dev,
+ struct smsc_keypad *kp)
+{
+ return -ENOSYS;
+}
+#endif
+
+static int __devinit
+smsc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct smsc *smsc = dev_get_drvdata(pdev->dev.parent);
+ struct input_dev *input;
+ struct smsc_keypad *kp;
+ int ret = 0, error;
+ int col, i, max_keys, row_shift;
+ int irq;
+ int addr_start, addr;
+
+ kp = devm_kzalloc(dev, sizeof(*kp), GFP_KERNEL);
+
+ input = input_allocate_device();
+ if (!kp || !input) {
+ error = -ENOMEM;
+ goto err1;
+ }
+
+ error = smsc_keypad_parse_dt(&pdev->dev, kp);
+ if (error)
+ return error;
+
+ /* Get the debug Device */
+ kp->input = input;
+ kp->smsc = smsc;
+ kp->irq = platform_get_irq(pdev, 0);
+ kp->dev = dev;
+
+ for (col = 0; col < 16; col++) {
+ kp->last_key_state[col] = 0;
+ kp->last_key_ms[col] = 0;
+ }
+
+ /* setup input device */
+ __set_bit(EV_KEY, input->evbit);
+
+ /* Enable auto repeat feature of Linux input subsystem */
+ if (!(kp->no_autorepeat))
+ __set_bit(EV_REP, input->evbit);
+
+ input_set_capability(input, EV_MSC, MSC_SCAN);
+ input->name = "SMSC Keypad";
+ input->phys = "smsc_keypad/input0";
+ input->dev.parent = &pdev->dev;
+ input->id.bustype = BUS_HOST;
+ input->id.vendor = 0x0001;
+ input->id.product = 0x0001;
+ input->id.version = 0x0003;
+
+ error = input_register_device(input);
+ if (error) {
+ dev_err(kp->dev,
+ "Unable to register twl4030 keypad device\n");
+ goto err1;
+ }
+
+ /* Mask all GPIO interrupts (0x37-0x3B) */
+ for (addr = 0x37; addr < 0x3B; addr++)
+ smsc_write(dev, addr, 0);
+
+ /* Set all outputs high (0x05-0x09) */
+ for (addr = 0x05; addr < 0x09; addr++)
+ smsc_write(dev, addr, 0xff);
+
+ /* Clear all GPIO interrupts (0x32-0x36) */
+ for (addr = 0x32; addr < 0x36; addr++)
+ smsc_write(dev, addr, 0xff);
+
+ addr_start = 0x12;
+ for (i = 0; i <= kp->rows; i++) {
+ addr = 0x12 + i;
+ smsc_write(dev, addr, SMSC_KP_KSI);
+ }
+
+ addr_start = 0x1A;
+ for (i = 0; i <= kp->cols; i++) {
+ addr = 0x1A + i;
+ smsc_write(dev, addr, SMSC_KP_KSO);
+ }
+
+ addr = SMSC_KP_INT_STAT;
+ smsc_write(dev, addr, SMSC_KP_SET_HIGH);
+
+ addr = SMSC_WKUP_CTRL;
+ smsc_write(dev, addr, SMSC_KP_SET_LOW_PWR);
+
+ addr = SMSC_KP_OUT;
+ smsc_write(dev, addr, SMSC_KSO_ALL_LOW);
+
+ row_shift = get_count_order(kp->cols);
+ max_keys = kp->rows << row_shift;
+
+ kp->row_shift = row_shift;
+ kp->keymap = kzalloc(max_keys * sizeof(kp->keymap[0]),
+ GFP_KERNEL);
+ if (!kp->keymap) {
+ dev_err(&pdev->dev, "Not enough memory for keymap\n");
+ error = -ENOMEM;
+ }
+
+ matrix_keypad_build_keymap(NULL, NULL, kp->rows,
+ kp->cols, kp->keymap, input);
+
+ /*
+ * This ISR will always execute in kernel thread context because of
+ * the need to access the SMSC over the I2C bus.
+ */
+ ret = devm_request_threaded_irq(dev, kp->irq, NULL, do_kp_irq,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT, pdev->name, kp);
+ if (ret) {
+ dev_dbg(&pdev->dev, "request_irq failed for irq no=%d\n",
+ irq);
+ goto err2;
+ }
+
+ /* Enable smsc keypad interrupts */
+ ret = smsc_write(dev, SMSC_KP_INT_MASK, 0xff);
+ if (ret < 0)
+ goto err2;
+
+ return 0;
+
+err2:
+ input_unregister_device(input);
+ free_irq(kp->irq, NULL);
+err1:
+ input_free_device(input);
+ return ret;
+}
+
+static int smsc_remove(struct platform_device *pdev)
+{
+ struct smsc_keypad *kp = platform_get_drvdata(pdev);
+ free_irq(kp->irq, kp);
+ input_unregister_device(kp->input);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id smsc_keypad_dt_match[] = {
+ { .compatible = "smsc,keypad" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, smsc_keypad_dt_match);
+#endif
+
+static struct platform_driver smsc_driver = {
+ .driver = {
+ .name = "smsc-keypad",
+ .of_match_table = of_match_ptr(smsc_keypad_dt_match),
+ .owner = THIS_MODULE,
+ },
+ .probe = smsc_probe,
+ .remove = smsc_remove,
+};
+
+module_platform_driver(smsc_driver);
+
+MODULE_AUTHOR("G Kondaiah Manjunath <manjugk@ti.com>");
+MODULE_DESCRIPTION("SMSC ECE1099 Keypad driver");
+MODULE_LICENSE("GPL v2");
--
1.7.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 2/4] Input: keypad: Add smsc ece1099 keypad driver
2012-08-21 10:45 ` [PATCH 2/4] Input: keypad: Add smsc ece1099 keypad driver Sourav Poddar
@ 2012-08-21 10:45 ` Felipe Balbi
2012-08-21 11:30 ` Poddar, Sourav
2012-08-21 10:46 ` Felipe Balbi
1 sibling, 1 reply; 26+ messages in thread
From: Felipe Balbi @ 2012-08-21 10:45 UTC (permalink / raw)
To: Sourav Poddar
Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input
[-- Attachment #1: Type: text/plain, Size: 4560 bytes --]
On Tue, Aug 21, 2012 at 04:15:38PM +0530, Sourav Poddar wrote:
> From: G, Manjunath Kondaiah <manjugk@ti.com>
>
> SMSC ECE1099 is a keyboard scan or GPIO expansion device.The device
> supports a keypad scan matrix of 23*8.This driver uses this
> device as a keypad driver.
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
looks good. If you just fix my comment on free_irq() below, you can add:
Acked-by: Felipe Balbi <balbi@ti.com>
> +static int __devinit
> +smsc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct smsc *smsc = dev_get_drvdata(pdev->dev.parent);
> + struct input_dev *input;
> + struct smsc_keypad *kp;
> + int ret = 0, error;
> + int col, i, max_keys, row_shift;
> + int irq;
> + int addr_start, addr;
> +
> + kp = devm_kzalloc(dev, sizeof(*kp), GFP_KERNEL);
> +
> + input = input_allocate_device();
> + if (!kp || !input) {
> + error = -ENOMEM;
> + goto err1;
> + }
> +
> + error = smsc_keypad_parse_dt(&pdev->dev, kp);
> + if (error)
> + return error;
> +
> + /* Get the debug Device */
> + kp->input = input;
> + kp->smsc = smsc;
> + kp->irq = platform_get_irq(pdev, 0);
> + kp->dev = dev;
> +
> + for (col = 0; col < 16; col++) {
> + kp->last_key_state[col] = 0;
> + kp->last_key_ms[col] = 0;
> + }
> +
> + /* setup input device */
> + __set_bit(EV_KEY, input->evbit);
> +
> + /* Enable auto repeat feature of Linux input subsystem */
> + if (!(kp->no_autorepeat))
> + __set_bit(EV_REP, input->evbit);
> +
> + input_set_capability(input, EV_MSC, MSC_SCAN);
> + input->name = "SMSC Keypad";
> + input->phys = "smsc_keypad/input0";
> + input->dev.parent = &pdev->dev;
> + input->id.bustype = BUS_HOST;
> + input->id.vendor = 0x0001;
> + input->id.product = 0x0001;
> + input->id.version = 0x0003;
> +
> + error = input_register_device(input);
> + if (error) {
> + dev_err(kp->dev,
> + "Unable to register twl4030 keypad device\n");
> + goto err1;
> + }
> +
> + /* Mask all GPIO interrupts (0x37-0x3B) */
> + for (addr = 0x37; addr < 0x3B; addr++)
> + smsc_write(dev, addr, 0);
> +
> + /* Set all outputs high (0x05-0x09) */
> + for (addr = 0x05; addr < 0x09; addr++)
> + smsc_write(dev, addr, 0xff);
> +
> + /* Clear all GPIO interrupts (0x32-0x36) */
> + for (addr = 0x32; addr < 0x36; addr++)
> + smsc_write(dev, addr, 0xff);
> +
> + addr_start = 0x12;
> + for (i = 0; i <= kp->rows; i++) {
> + addr = 0x12 + i;
> + smsc_write(dev, addr, SMSC_KP_KSI);
> + }
> +
> + addr_start = 0x1A;
> + for (i = 0; i <= kp->cols; i++) {
> + addr = 0x1A + i;
> + smsc_write(dev, addr, SMSC_KP_KSO);
> + }
> +
> + addr = SMSC_KP_INT_STAT;
> + smsc_write(dev, addr, SMSC_KP_SET_HIGH);
> +
> + addr = SMSC_WKUP_CTRL;
> + smsc_write(dev, addr, SMSC_KP_SET_LOW_PWR);
> +
> + addr = SMSC_KP_OUT;
> + smsc_write(dev, addr, SMSC_KSO_ALL_LOW);
> +
> + row_shift = get_count_order(kp->cols);
> + max_keys = kp->rows << row_shift;
> +
> + kp->row_shift = row_shift;
> + kp->keymap = kzalloc(max_keys * sizeof(kp->keymap[0]),
> + GFP_KERNEL);
> + if (!kp->keymap) {
> + dev_err(&pdev->dev, "Not enough memory for keymap\n");
> + error = -ENOMEM;
> + }
> +
> + matrix_keypad_build_keymap(NULL, NULL, kp->rows,
> + kp->cols, kp->keymap, input);
> +
> + /*
> + * This ISR will always execute in kernel thread context because of
> + * the need to access the SMSC over the I2C bus.
> + */
> + ret = devm_request_threaded_irq(dev, kp->irq, NULL, do_kp_irq,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, pdev->name, kp);
> + if (ret) {
> + dev_dbg(&pdev->dev, "request_irq failed for irq no=%d\n",
> + irq);
> + goto err2;
> + }
> +
> + /* Enable smsc keypad interrupts */
> + ret = smsc_write(dev, SMSC_KP_INT_MASK, 0xff);
> + if (ret < 0)
> + goto err2;
> +
> + return 0;
> +
> +err2:
> + input_unregister_device(input);
> + free_irq(kp->irq, NULL);
you're using devm_request_threaded_irq, this is unnecessary
> +err1:
> + input_free_device(input);
> + return ret;
> +}
> +
> +static int smsc_remove(struct platform_device *pdev)
> +{
> + struct smsc_keypad *kp = platform_get_drvdata(pdev);
> + free_irq(kp->irq, kp);
ditto.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/4] Input: keypad: Add smsc ece1099 keypad driver
2012-08-21 10:45 ` Felipe Balbi
@ 2012-08-21 11:30 ` Poddar, Sourav
0 siblings, 0 replies; 26+ messages in thread
From: Poddar, Sourav @ 2012-08-21 11:30 UTC (permalink / raw)
To: balbi
Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input
Hi,
On Tue, Aug 21, 2012 at 4:15 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Tue, Aug 21, 2012 at 04:15:38PM +0530, Sourav Poddar wrote:
>> From: G, Manjunath Kondaiah <manjugk@ti.com>
>>
>> SMSC ECE1099 is a keyboard scan or GPIO expansion device.The device
>> supports a keypad scan matrix of 23*8.This driver uses this
>> device as a keypad driver.
>>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benoit Cousson <b-cousson@ti.com>
>> Cc: Felipe Balbi <balbi@ti.com>
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
>> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
>
> looks good. If you just fix my comment on free_irq() below, you can add:
>
> Acked-by: Felipe Balbi <balbi@ti.com>
>
>> +static int __devinit
>> +smsc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct smsc *smsc = dev_get_drvdata(pdev->dev.parent);
>> + struct input_dev *input;
>> + struct smsc_keypad *kp;
>> + int ret = 0, error;
>> + int col, i, max_keys, row_shift;
>> + int irq;
>> + int addr_start, addr;
>> +
>> + kp = devm_kzalloc(dev, sizeof(*kp), GFP_KERNEL);
>> +
>> + input = input_allocate_device();
>> + if (!kp || !input) {
>> + error = -ENOMEM;
>> + goto err1;
>> + }
>> +
>> + error = smsc_keypad_parse_dt(&pdev->dev, kp);
>> + if (error)
>> + return error;
>> +
>> + /* Get the debug Device */
>> + kp->input = input;
>> + kp->smsc = smsc;
>> + kp->irq = platform_get_irq(pdev, 0);
>> + kp->dev = dev;
>> +
>> + for (col = 0; col < 16; col++) {
>> + kp->last_key_state[col] = 0;
>> + kp->last_key_ms[col] = 0;
>> + }
>> +
>> + /* setup input device */
>> + __set_bit(EV_KEY, input->evbit);
>> +
>> + /* Enable auto repeat feature of Linux input subsystem */
>> + if (!(kp->no_autorepeat))
>> + __set_bit(EV_REP, input->evbit);
>> +
>> + input_set_capability(input, EV_MSC, MSC_SCAN);
>> + input->name = "SMSC Keypad";
>> + input->phys = "smsc_keypad/input0";
>> + input->dev.parent = &pdev->dev;
>> + input->id.bustype = BUS_HOST;
>> + input->id.vendor = 0x0001;
>> + input->id.product = 0x0001;
>> + input->id.version = 0x0003;
>> +
>> + error = input_register_device(input);
>> + if (error) {
>> + dev_err(kp->dev,
>> + "Unable to register twl4030 keypad device\n");
>> + goto err1;
>> + }
>> +
>> + /* Mask all GPIO interrupts (0x37-0x3B) */
>> + for (addr = 0x37; addr < 0x3B; addr++)
>> + smsc_write(dev, addr, 0);
>> +
>> + /* Set all outputs high (0x05-0x09) */
>> + for (addr = 0x05; addr < 0x09; addr++)
>> + smsc_write(dev, addr, 0xff);
>> +
>> + /* Clear all GPIO interrupts (0x32-0x36) */
>> + for (addr = 0x32; addr < 0x36; addr++)
>> + smsc_write(dev, addr, 0xff);
>> +
>> + addr_start = 0x12;
>> + for (i = 0; i <= kp->rows; i++) {
>> + addr = 0x12 + i;
>> + smsc_write(dev, addr, SMSC_KP_KSI);
>> + }
>> +
>> + addr_start = 0x1A;
>> + for (i = 0; i <= kp->cols; i++) {
>> + addr = 0x1A + i;
>> + smsc_write(dev, addr, SMSC_KP_KSO);
>> + }
>> +
>> + addr = SMSC_KP_INT_STAT;
>> + smsc_write(dev, addr, SMSC_KP_SET_HIGH);
>> +
>> + addr = SMSC_WKUP_CTRL;
>> + smsc_write(dev, addr, SMSC_KP_SET_LOW_PWR);
>> +
>> + addr = SMSC_KP_OUT;
>> + smsc_write(dev, addr, SMSC_KSO_ALL_LOW);
>> +
>> + row_shift = get_count_order(kp->cols);
>> + max_keys = kp->rows << row_shift;
>> +
>> + kp->row_shift = row_shift;
>> + kp->keymap = kzalloc(max_keys * sizeof(kp->keymap[0]),
>> + GFP_KERNEL);
>> + if (!kp->keymap) {
>> + dev_err(&pdev->dev, "Not enough memory for keymap\n");
>> + error = -ENOMEM;
>> + }
>> +
>> + matrix_keypad_build_keymap(NULL, NULL, kp->rows,
>> + kp->cols, kp->keymap, input);
>> +
>> + /*
>> + * This ISR will always execute in kernel thread context because of
>> + * the need to access the SMSC over the I2C bus.
>> + */
>> + ret = devm_request_threaded_irq(dev, kp->irq, NULL, do_kp_irq,
>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, pdev->name, kp);
>> + if (ret) {
>> + dev_dbg(&pdev->dev, "request_irq failed for irq no=%d\n",
>> + irq);
>> + goto err2;
>> + }
>> +
>> + /* Enable smsc keypad interrupts */
>> + ret = smsc_write(dev, SMSC_KP_INT_MASK, 0xff);
>> + if (ret < 0)
>> + goto err2;
>> +
>> + return 0;
>> +
>> +err2:
>> + input_unregister_device(input);
>> + free_irq(kp->irq, NULL);
>
> you're using devm_request_threaded_irq, this is unnecessary
>
True. Will remove "free_irq" from the code.
>> +err1:
>> + input_free_device(input);
>> + return ret;
>> +}
>> +
>> +static int smsc_remove(struct platform_device *pdev)
>> +{
>> + struct smsc_keypad *kp = platform_get_drvdata(pdev);
>> + free_irq(kp->irq, kp);
>
> ditto.
>
>
> --
> balbi
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/4] Input: keypad: Add smsc ece1099 keypad driver
2012-08-21 10:45 ` [PATCH 2/4] Input: keypad: Add smsc ece1099 keypad driver Sourav Poddar
2012-08-21 10:45 ` Felipe Balbi
@ 2012-08-21 10:46 ` Felipe Balbi
2012-08-21 11:31 ` Poddar, Sourav
1 sibling, 1 reply; 26+ messages in thread
From: Felipe Balbi @ 2012-08-21 10:46 UTC (permalink / raw)
To: Sourav Poddar
Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input
[-- Attachment #1: Type: text/plain, Size: 460 bytes --]
Hi,
On Tue, Aug 21, 2012 at 04:15:38PM +0530, Sourav Poddar wrote:
> +static struct platform_driver smsc_driver = {
> + .driver = {
> + .name = "smsc-keypad",
> + .of_match_table = of_match_ptr(smsc_keypad_dt_match),
> + .owner = THIS_MODULE,
> + },
> + .probe = smsc_probe,
> + .remove = smsc_remove,
one extra comment which I forgot. You probably should put your remove on
__devexit and add __devexit_p(smsc_remove) here.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/4] Input: keypad: Add smsc ece1099 keypad driver
2012-08-21 10:46 ` Felipe Balbi
@ 2012-08-21 11:31 ` Poddar, Sourav
0 siblings, 0 replies; 26+ messages in thread
From: Poddar, Sourav @ 2012-08-21 11:31 UTC (permalink / raw)
To: balbi
Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input
Hi,
On Tue, Aug 21, 2012 at 4:16 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Tue, Aug 21, 2012 at 04:15:38PM +0530, Sourav Poddar wrote:
>> +static struct platform_driver smsc_driver = {
>> + .driver = {
>> + .name = "smsc-keypad",
>> + .of_match_table = of_match_ptr(smsc_keypad_dt_match),
>> + .owner = THIS_MODULE,
>> + },
>> + .probe = smsc_probe,
>> + .remove = smsc_remove,
>
> one extra comment which I forgot. You probably should put your remove on
> __devexit and add __devexit_p(smsc_remove) here.
>
yes, Will do it.
> --
> balbi
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/4] arm/dts: omap5-evm: Add keypad support
2012-08-21 10:45 [PATCH 0/4] Add mfd driver for smsc-ece1099 chip Sourav Poddar
2012-08-21 10:45 ` [PATCH 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver Sourav Poddar
2012-08-21 10:45 ` [PATCH 2/4] Input: keypad: Add smsc ece1099 keypad driver Sourav Poddar
@ 2012-08-21 10:45 ` Sourav Poddar
2012-08-21 10:47 ` Felipe Balbi
2012-08-21 10:45 ` [RFC/PATCH 4/4] gpio: smscece: Add support for gpio IO expander feature Sourav Poddar
3 siblings, 1 reply; 26+ messages in thread
From: Sourav Poddar @ 2012-08-21 10:45 UTC (permalink / raw)
To: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input, sourav.poddar
Add keypad data node in omap5-evm.
Based on I2C support patch for omap5, which has been
already posted as a different series.
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Tested on omap5430 sdp with 3.5 custom kernel.
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
arch/arm/boot/dts/omap5-evm.dts | 95 +++++++++++++++++++++++++++++++++++++++
1 files changed, 95 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/omap5-evm.dts b/arch/arm/boot/dts/omap5-evm.dts
index 200c39a..6473983 100644
--- a/arch/arm/boot/dts/omap5-evm.dts
+++ b/arch/arm/boot/dts/omap5-evm.dts
@@ -18,3 +18,98 @@
reg = <0x80000000 0x40000000>; /* 1 GB */
};
};
+
+&i2c5 {
+ clock-frequency = <400000>;
+
+ smsc@38 {
+ compatible = "smsc";
+ reg = <0x38>;
+ clock = <0x13>;
+ keypad {
+ compatible = "smsc,keypad";
+ interrupt-parent = <&gpio5>;
+ interrupts = <23>; /* gpio line 151 */
+ keypad,num-rows = <8>;
+ keypad,num-columns = <16>;
+ linux,keymap = < 0x20041 /*KEY_F7*/
+ 0x30001 /*KEY_ESC*/
+ 0x4003e /*KEY_F4*/
+ 0x50022 /*KEY_G*/
+ 0x70023 /*KEY_H*/
+ 0x9009a /*KEY_CYCLEWINDOWS*/
+ 0xc000e /*KEY_BACKSPACE*/
+ 0xd0057 /*KEY_F11*/
+ 0xe009f /*KEY_FORWARD*/
+ 0xf006e /*KEY_INSERT*/
+ 0x1020036 /*KEY_RIGHTSHIFT*/
+ 0x1030011 /*KEY_W*/
+ 0x1040010 /*KEY_Q*/
+ 0x1050012 /*KEY_E*/
+ 0x1070013 /*KEY_R*/
+ 0x1080016 /*KEY_U*/
+ 0x10c0017 /*KEY_I*/
+ 0x10d0067 /*KEY_UP*/
+ 0x10e0018 /*KEY_O*/
+ 0x10f0019 /*KEY_LEFT*/
+ 0x2020003 /*KEY_2*/
+ 0x2040004 /*KEY_1*/
+ 0x2050005 /*KEY_3*/
+ 0x2070008 /*KEY_4*/
+ 0x2080009 /*KEY_7*/
+ 0x20b0064 /*KEY_8*/
+ 0x20c006c /*KEY_RIGHTALT*/
+ 0x20d000a /*KEY_DOWN*/
+ 0x20e0001 /*KEY_0*/
+ 0x20f006a /*KEY_RIGHT*/
+ 0x3010061 /*KEY_RIGHTCTRL*/
+ 0x302001f /*KEY_S*/
+ 0x303001e /*KEY_A*/
+ 0x3040020 /*KEY_D*/
+ 0x3050021 /*KEY_F*/
+ 0x3070024 /*KEY_J*/
+ 0x3080025 /*KEY_K*/
+ 0x30c001c /*KEY_ENTER*/
+ 0x30d0026 /*KEY_L*/
+ 0x30e0027 /*KEY_SEMICOLON*/
+ 0x400002a /*KEY_LEFTSHIFT*/
+ 0x402002d /*KEY_X*/
+ 0x403002c /*KEY_Z*/
+ 0x404002e /*KEY_C*/
+ 0x405002f /*KEY_V/
+ 0x4070032 /*KEY_M*/
+ 0x4080033 /*KEY_COMMA*/
+ 0x40c0039 /*KEY_SPACE*/
+ 0x40d0033 /*KEY_DOT*/
+ 0x40e0035 /*KEY_SLASH*/
+ 0x40f006b /*KEY_END*/
+ 0x501001d /*KEY_LEFTCTRL*/
+ 0x5020040 /*KEY_F6*/
+ 0x503000f /*KEY_TAB*/
+ 0x504003d /*KEY_F3*/
+ 0x5050014 /*KEY_T*/
+ 0x5070015 /*KEY_Y*/
+ 0x508001a /*KEY_LEFTBRACE*/
+ 0x50d0044 /*KEY_F10*/
+ 0x50e001b /*KEY_RIGHTBRACE*/
+ 0x50f0066 /*KEY_HOME*/
+ 0x602003f /*KEY_F5*/
+ 0x604003c /*KEY_F2*/
+ 0x6050006 /*KEY_5*/
+ 0x60601d0 /*KEY_FN*/
+ 0x6070007 /*KEY_6*/
+ 0x60a008b /*KEY_MENU*/
+ 0x60c002b /*KEY_BACKSLASH*/
+ 0x60d0043 /*KEY_F9*/
+ 0x7020042 /*KEY_F8*/
+ 0x703003a /*KEY_CAPSLOCK*/
+ 0x704003b /*KEY_F1*/
+ 0x7050030 /*KEY_B*/
+ 0x7070031 /*KEY_N*/
+ 0x70b0038 /*KEY_LEFTALT*/
+ 0x70d0058 /*KEY_F12*/
+ 0x70f006f >; /*KEY_DELETE*/
+ linux,input-no-autorepeat;
+ };
+ };
+};
--
1.7.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 3/4] arm/dts: omap5-evm: Add keypad support
2012-08-21 10:45 ` [PATCH 3/4] arm/dts: omap5-evm: Add keypad support Sourav Poddar
@ 2012-08-21 10:47 ` Felipe Balbi
2012-08-21 11:20 ` Poddar, Sourav
0 siblings, 1 reply; 26+ messages in thread
From: Felipe Balbi @ 2012-08-21 10:47 UTC (permalink / raw)
To: Sourav Poddar
Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input
[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]
On Tue, Aug 21, 2012 at 04:15:39PM +0530, Sourav Poddar wrote:
> Add keypad data node in omap5-evm.
>
> Based on I2C support patch for omap5, which has been
> already posted as a different series.
>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Tested on omap5430 sdp with 3.5 custom kernel.
>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
after fixing my only comment below, you can add my:
Acked-by: Felipe Balbi <balbi@ti.com>
> ---
> arch/arm/boot/dts/omap5-evm.dts | 95 +++++++++++++++++++++++++++++++++++++++
> 1 files changed, 95 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/omap5-evm.dts b/arch/arm/boot/dts/omap5-evm.dts
> index 200c39a..6473983 100644
> --- a/arch/arm/boot/dts/omap5-evm.dts
> +++ b/arch/arm/boot/dts/omap5-evm.dts
> @@ -18,3 +18,98 @@
> reg = <0x80000000 0x40000000>; /* 1 GB */
> };
> };
> +
> +&i2c5 {
> + clock-frequency = <400000>;
> +
> + smsc@38 {
> + compatible = "smsc";
> + reg = <0x38>;
> + clock = <0x13>;
> + keypad {
> + compatible = "smsc,keypad";
> + interrupt-parent = <&gpio5>;
> + interrupts = <23>; /* gpio line 151 */
> + keypad,num-rows = <8>;
> + keypad,num-columns = <16>;
> + linux,keymap = < 0x20041 /*KEY_F7*/
please add spaces around /* and */ so it's easier to read. Ditto to all
others.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/4] arm/dts: omap5-evm: Add keypad support
2012-08-21 10:47 ` Felipe Balbi
@ 2012-08-21 11:20 ` Poddar, Sourav
0 siblings, 0 replies; 26+ messages in thread
From: Poddar, Sourav @ 2012-08-21 11:20 UTC (permalink / raw)
To: balbi
Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input
Hi,
On Tue, Aug 21, 2012 at 4:17 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Tue, Aug 21, 2012 at 04:15:39PM +0530, Sourav Poddar wrote:
>> Add keypad data node in omap5-evm.
>>
>> Based on I2C support patch for omap5, which has been
>> already posted as a different series.
>>
>> Cc: Benoit Cousson <b-cousson@ti.com>
>> Cc: Felipe Balbi <balbi@ti.com>
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Tested on omap5430 sdp with 3.5 custom kernel.
>>
>> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
>
> after fixing my only comment below, you can add my:
>
> Acked-by: Felipe Balbi <balbi@ti.com>
>
>> ---
>> arch/arm/boot/dts/omap5-evm.dts | 95 +++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 95 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/omap5-evm.dts b/arch/arm/boot/dts/omap5-evm.dts
>> index 200c39a..6473983 100644
>> --- a/arch/arm/boot/dts/omap5-evm.dts
>> +++ b/arch/arm/boot/dts/omap5-evm.dts
>> @@ -18,3 +18,98 @@
>> reg = <0x80000000 0x40000000>; /* 1 GB */
>> };
>> };
>> +
>> +&i2c5 {
>> + clock-frequency = <400000>;
>> +
>> + smsc@38 {
>> + compatible = "smsc";
>> + reg = <0x38>;
>> + clock = <0x13>;
>> + keypad {
>> + compatible = "smsc,keypad";
>> + interrupt-parent = <&gpio5>;
>> + interrupts = <23>; /* gpio line 151 */
>> + keypad,num-rows = <8>;
>> + keypad,num-columns = <16>;
>> + linux,keymap = < 0x20041 /*KEY_F7*/
>
> please add spaces around /* and */ so it's easier to read. Ditto to all
> others.
>
Yes, will add spaces in the new version.
> --
> balbi
^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC/PATCH 4/4] gpio: smscece: Add support for gpio IO expander feature
2012-08-21 10:45 [PATCH 0/4] Add mfd driver for smsc-ece1099 chip Sourav Poddar
` (2 preceding siblings ...)
2012-08-21 10:45 ` [PATCH 3/4] arm/dts: omap5-evm: Add keypad support Sourav Poddar
@ 2012-08-21 10:45 ` Sourav Poddar
2012-08-21 10:53 ` Felipe Balbi
3 siblings, 1 reply; 26+ messages in thread
From: Sourav Poddar @ 2012-08-21 10:45 UTC (permalink / raw)
To: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input, sourav.poddar
smsc can be used as an gpio io expander device also. So adding
support for configuring smsc pins as a gpio.
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>
---
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-smscece.c | 373 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 381 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/gpio-smscece.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b16c8a7..e883929 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -444,6 +444,13 @@ config GPIO_ADP5588_IRQ
Say yes here to enable the adp5588 to be used as an interrupt
controller. It requires the driver to be built in the kernel.
+config GPIO_SMSCECE
+ tristate "SMSCECE 1099 I2C GPIO expander"
+ depends on I2C
+ help
+ This option enables support for 18 GPIOs found
+ on SMSC ECE 1099 GPIO Expanders.
+
comment "PCI GPIO expanders:"
config GPIO_CS5535
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 153cace..7c803c5 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_AB8500) += gpio-ab8500.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
+obj-$(CONFIG_GPIO_SMSCECE) += gpio-smscece.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
diff --git a/drivers/gpio/gpio-smscece.c b/drivers/gpio/gpio-smscece.c
new file mode 100644
index 0000000..0cb0959
--- /dev/null
+++ b/drivers/gpio/gpio-smscece.c
@@ -0,0 +1,373 @@
+/*
+ * GPIO Chip driver for smsc
+ * SMSC I/O Expander and QWERTY Keypad Controller
+ *
+ * Copyright 2012 Texas Instruments Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/irq.h>
+#include <linux/mfd/smsc.h>
+#include <linux/err.h>
+
+struct smsc_gpio {
+ struct device *dev;
+ struct smsc *smsc;
+ struct gpio_chip gpio_chip;
+ struct mutex lock; /* protect cached dir, dat_out */
+ /* protect serialized access to the interrupt controller bus */
+ struct mutex irq_lock;
+ unsigned gpio_start;
+ int type;
+ int flags;
+ int irq;
+ int irq_base;
+ unsigned int gpio_base;
+ unsigned int dat_out[5];
+ unsigned int dir[5];
+ unsigned int int_lvl[5];
+ unsigned int int_en[5];
+ unsigned int irq_mask[5];
+ unsigned int irq_stat[5];
+};
+
+static int smsc_gpio_get_value(struct gpio_chip *chip, unsigned off)
+{
+ struct smsc_gpio *sg =
+ container_of(chip, struct smsc_gpio, gpio_chip);
+ unsigned int get;
+ return !!(smsc_read(sg->dev,
+ (SMSC_GPIO_DATA_IN_START + SMSC_BANK(off)) & SMSC_BIT(off),
+ &get));
+}
+
+static void smsc_gpio_set_value(struct gpio_chip *chip,
+ unsigned off, int val)
+{
+ unsigned bank, bit;
+ struct smsc_gpio *sg =
+ container_of(chip, struct smsc_gpio, gpio_chip);
+
+ bank = SMSC_BANK(off);
+ bit = SMSC_BIT(off);
+
+ mutex_lock(&sg->lock);
+ if (val)
+ sg->dat_out[bank] |= bit;
+ else
+ sg->dat_out[bank] &= ~bit;
+
+ smsc_write(sg->dev, SMSC_GPIO_DATA_OUT_START + bank,
+ sg->dat_out[bank]);
+ mutex_unlock(&sg->lock);
+}
+
+static int smsc_gpio_direction_input(struct gpio_chip *chip, unsigned off)
+{
+ unsigned int reg;
+ struct smsc_gpio *sg =
+ container_of(chip, struct smsc_gpio, gpio_chip);
+ int reg_dir;
+
+ mutex_lock(&sg->lock);
+ reg_dir = SMSC_CFG_START + off;
+ smsc_read(sg->dev, reg_dir, ®);
+ reg |= SMSC_GPIO_INPUT_LOW;
+ mutex_unlock(&sg->lock);
+
+ return smsc_write(sg->dev, reg_dir, reg);
+}
+
+static int smsc_gpio_direction_output(struct gpio_chip *chip,
+ unsigned off, int val)
+{
+ unsigned int reg;
+ struct smsc_gpio *sg =
+ container_of(chip, struct smsc_gpio, gpio_chip);
+ int reg_dir;
+
+ mutex_lock(&sg->lock);
+ reg_dir = SMSC_CFG_START + off;
+ smsc_read(sg->dev, reg_dir, ®);
+ reg |= SMSC_GPIO_OUTPUT_PP;
+ mutex_unlock(&sg->lock);
+
+ return smsc_write(sg->dev, reg_dir, reg);
+}
+
+static int smsc_gpio_to_irq(struct gpio_chip *chip, unsigned off)
+{
+ struct smsc_gpio *sg =
+ container_of(chip, struct smsc_gpio, gpio_chip);
+ return sg->irq_base + off;
+}
+
+static void smsc_irq_bus_lock(struct irq_data *d)
+{
+ struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
+
+ mutex_lock(&sg->irq_lock);
+}
+
+static void smsc_irq_bus_sync_unlock(struct irq_data *d)
+{
+ struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
+ int i;
+
+ for (i = 0; i < SMSC_BANK(SMSC_MAXGPIO); i++)
+ if (sg->int_en[i] ^ sg->irq_mask[i]) {
+ sg->int_en[i] = sg->irq_mask[i];
+ smsc_write(sg->dev, SMSC_GPIO_INT_MASK_START + i,
+ sg->int_en[i]);
+ }
+
+ mutex_unlock(&sg->irq_lock);
+}
+
+static void smsc_irq_mask(struct irq_data *d)
+{
+ struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
+ unsigned gpio = d->irq - sg->irq_base;
+
+ sg->irq_mask[SMSC_BANK(gpio)] &= ~SMSC_BIT(gpio);
+}
+
+static void smsc_irq_unmask(struct irq_data *d)
+{
+ struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
+ unsigned gpio = d->irq - sg->irq_base;
+
+ sg->irq_mask[SMSC_BANK(gpio)] |= SMSC_BIT(gpio);
+}
+
+static int smsc_irq_set_type(struct irq_data *d, unsigned int type)
+{
+ struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
+ uint16_t gpio = d->irq - sg->irq_base;
+ unsigned bank, bit;
+
+ if ((type & IRQ_TYPE_EDGE_BOTH)) {
+ dev_err(sg->dev, "irq %d: unsupported type %d\n",
+ d->irq, type);
+ return -EINVAL;
+ }
+
+ bank = SMSC_BANK(gpio);
+ bit = SMSC_BIT(gpio);
+
+ if (type & IRQ_TYPE_LEVEL_HIGH)
+ sg->int_lvl[bank] |= bit;
+ else if (type & IRQ_TYPE_LEVEL_LOW)
+ sg->int_lvl[bank] &= ~bit;
+ else
+ return -EINVAL;
+
+ smsc_gpio_direction_input(&sg->gpio_chip, gpio);
+ smsc_write(sg->dev, SMSC_CFG_START + gpio,
+ sg->int_lvl[bank]);
+
+ return 0;
+}
+
+static struct irq_chip smsc_irq_chip = {
+ .name = "smsc",
+ .irq_mask = smsc_irq_mask,
+ .irq_unmask = smsc_irq_unmask,
+ .irq_bus_lock = smsc_irq_bus_lock,
+ .irq_bus_sync_unlock = smsc_irq_bus_sync_unlock,
+ .irq_set_type = smsc_irq_set_type,
+};
+
+static int smsc_gpio_read_intstat(struct smsc_gpio *sg,
+ unsigned int *buf, int i)
+{
+ int ret = smsc_read(sg->dev,
+ SMSC_GPIO_INT_STAT_START + i, buf);
+
+ if (ret < 0)
+ dev_err(sg->dev, "Read INT_STAT Error\n");
+
+ return ret;
+}
+
+static irqreturn_t smsc_irq_handler(int irq, void *devid)
+{
+ struct smsc_gpio *sg = devid;
+ unsigned int status, bank, pending;
+ int ret;
+ smsc_read(sg->dev, GRP_INT_STAT, &status);
+
+ if (!(status & SMSC_GPI_INT))
+ goto out;
+
+ for (bank = 0; bank <= SMSC_BANK(SMSC_MAXGPIO);
+ bank++) {
+ pending = sg->irq_stat[bank] & sg->irq_mask[bank];
+ ret = smsc_gpio_read_intstat(sg,
+ &sg->irq_stat[bank], bank);
+ if (ret < 0)
+ memset(&sg->irq_stat[bank], 0,
+ ARRAY_SIZE(sg->irq_stat));
+
+ while (pending) {
+ unsigned long bit = __ffs(pending);
+ unsigned int irq;
+
+ pending &= ~BIT(bit);
+ irq = bit + sg->irq_base;
+ handle_nested_irq(irq);
+ }
+ }
+
+out:
+ smsc_write(sg->dev, GRP_INT_STAT, status); /* Status is W1C */
+
+ return IRQ_HANDLED;
+}
+
+static int smsc_irq_setup(struct smsc_gpio *sg)
+{
+ unsigned gpio;
+ int ret;
+
+ mutex_init(&sg->irq_lock);
+
+ for (gpio = 0; gpio < sg->gpio_chip.ngpio; gpio++) {
+ int irq = gpio + sg->irq_base;
+ irq_set_chip_data(irq, sg);
+ irq_set_chip_and_handler(irq, &smsc_irq_chip,
+ handle_level_irq);
+ irq_set_nested_thread(irq, 1);
+#ifdef CONFIG_ARM
+ set_irq_flags(irq, IRQF_VALID);
+#else
+ irq_set_noprobe(irq);
+#endif
+ }
+
+ ret = request_threaded_irq(sg->irq, NULL,
+ smsc_irq_handler, sg->flags,
+ "smsc_gpio", sg);
+ if (ret) {
+ dev_err(sg->dev, "failed to request irq %d\n",
+ sg->irq);
+ }
+
+ sg->gpio_chip.to_irq = smsc_gpio_to_irq;
+
+ return 0;
+}
+
+static int __devinit smsc_gpio_probe(struct platform_device *pdev)
+{
+ struct smsc_gpio *sg;
+ struct gpio_chip *gc;
+ struct smsc *smsc = dev_get_drvdata(pdev->dev.parent);
+ int ret, i, temp;
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ int irq_base;
+
+ sg = devm_kzalloc(dev, sizeof(*sg), GFP_KERNEL);
+ if (sg == NULL) {
+ dev_err(&pdev->dev, "failed to alloc memory\n");
+ return -ENOMEM;
+ }
+
+ sg->irq = platform_get_irq(pdev, 0);
+ if (np) {
+ of_property_read_u32(np, "gpio,base", &temp);
+ of_property_read_u32(np, "flags", &sg->flags);
+ }
+
+ gc = &sg->gpio_chip;
+ gc->direction_input = smsc_gpio_direction_input;
+ gc->direction_output = smsc_gpio_direction_output;
+ gc->get = smsc_gpio_get_value;
+ gc->set = smsc_gpio_set_value;
+ gc->can_sleep = 1;
+
+ gc->base = temp;
+ gc->ngpio = SMSC_MAXGPIO;
+ gc->owner = THIS_MODULE;
+
+ sg->smsc = smsc;
+ sg->dev = dev;
+ mutex_init(&sg->lock);
+
+ for (i = 0; i <= SMSC_BANK(SMSC_MAXGPIO); i++)
+ smsc_read(sg->dev, SMSC_GPIO_DATA_OUT_START + i,
+ &sg->dat_out[i]);
+
+ for (i = 0; i < SMSC_MAXGPIO; i++)
+ smsc_read(sg->dev, SMSC_CFG_START + i, &sg->dir[i]);
+
+ irq_base = irq_alloc_descs(-1, 0, sg->gpio_chip.ngpio, 0);
+ if (IS_ERR_VALUE(irq_base)) {
+ dev_err(sg->dev, "Fail to allocate IRQ descs\n");
+ return irq_base;
+ }
+ sg->irq_base = irq_base;
+
+ irq_domain_add_legacy(pdev->dev.of_node, sg->gpio_chip.ngpio,
+ sg->irq_base, 0, &irq_domain_simple_ops, NULL);
+
+ ret = smsc_irq_setup(sg);
+ if (ret)
+ goto err;
+
+ ret = gpiochip_add(&sg->gpio_chip);
+ if (ret)
+ goto err;
+
+ return 0;
+
+err:
+ kfree(sg);
+
+ return ret;
+}
+
+static int __devexit smsc_gpio_remove(struct platform_device *pdev)
+{
+ struct smsc_gpio *sg = dev_get_drvdata(&pdev->dev);
+ int ret;
+
+ ret = gpiochip_remove(&sg->gpio_chip);
+ if (ret) {
+ dev_err(&pdev->dev, "gpiochip_remove failed %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id smsc_gpio_dt_match[] = {
+ { .compatible = "smsc,gpio" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, smsc_gpio_dt_match);
+
+static struct platform_driver smsc_gpio_driver = {
+ .driver = {
+ .name = "smsc_gpio",
+ .of_match_table = of_match_ptr(smsc_gpio_dt_match),
+ },
+ .probe = smsc_gpio_probe,
+ .remove = __devexit_p(smsc_gpio_remove),
+};
+
+module_platform_driver(smsc_gpio_driver);
+
+MODULE_AUTHOR("Sourav Poddar <sourav.poddar@ti.com>");
+MODULE_DESCRIPTION("GPIO SMSC Driver");
+MODULE_LICENSE("GPL v2");
--
1.7.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [RFC/PATCH 4/4] gpio: smscece: Add support for gpio IO expander feature
2012-08-21 10:45 ` [RFC/PATCH 4/4] gpio: smscece: Add support for gpio IO expander feature Sourav Poddar
@ 2012-08-21 10:53 ` Felipe Balbi
2012-08-21 11:47 ` Poddar, Sourav
0 siblings, 1 reply; 26+ messages in thread
From: Felipe Balbi @ 2012-08-21 10:53 UTC (permalink / raw)
To: Sourav Poddar
Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input
[-- Attachment #1: Type: text/plain, Size: 11752 bytes --]
Hi,
On Tue, Aug 21, 2012 at 04:15:40PM +0530, Sourav Poddar wrote:
> smsc can be used as an gpio io expander device also. So adding
> support for configuring smsc pins as a gpio.
>
> 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>
> ---
> drivers/gpio/Kconfig | 7 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-smscece.c | 373 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 381 insertions(+), 0 deletions(-)
> create mode 100644 drivers/gpio/gpio-smscece.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index b16c8a7..e883929 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -444,6 +444,13 @@ config GPIO_ADP5588_IRQ
> Say yes here to enable the adp5588 to be used as an interrupt
> controller. It requires the driver to be built in the kernel.
>
> +config GPIO_SMSCECE
> + tristate "SMSCECE 1099 I2C GPIO expander"
> + depends on I2C
> + help
> + This option enables support for 18 GPIOs found
> + on SMSC ECE 1099 GPIO Expanders.
> +
> comment "PCI GPIO expanders:"
>
> config GPIO_CS5535
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 153cace..7c803c5 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
> obj-$(CONFIG_GPIO_AB8500) += gpio-ab8500.o
> obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
> obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
> +obj-$(CONFIG_GPIO_SMSCECE) += gpio-smscece.o
> obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
> obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
> obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
> diff --git a/drivers/gpio/gpio-smscece.c b/drivers/gpio/gpio-smscece.c
> new file mode 100644
> index 0000000..0cb0959
> --- /dev/null
> +++ b/drivers/gpio/gpio-smscece.c
> @@ -0,0 +1,373 @@
> +/*
> + * GPIO Chip driver for smsc
> + * SMSC I/O Expander and QWERTY Keypad Controller
> + *
> + * Copyright 2012 Texas Instruments Inc.
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irq.h>
> +#include <linux/mfd/smsc.h>
> +#include <linux/err.h>
> +
> +struct smsc_gpio {
> + struct device *dev;
> + struct smsc *smsc;
> + struct gpio_chip gpio_chip;
> + struct mutex lock; /* protect cached dir, dat_out */
> + /* protect serialized access to the interrupt controller bus */
> + struct mutex irq_lock;
> + unsigned gpio_start;
> + int type;
> + int flags;
> + int irq;
> + int irq_base;
> + unsigned int gpio_base;
> + unsigned int dat_out[5];
> + unsigned int dir[5];
> + unsigned int int_lvl[5];
> + unsigned int int_en[5];
> + unsigned int irq_mask[5];
> + unsigned int irq_stat[5];
> +};
> +
> +static int smsc_gpio_get_value(struct gpio_chip *chip, unsigned off)
> +{
> + struct smsc_gpio *sg =
> + container_of(chip, struct smsc_gpio, gpio_chip);
> + unsigned int get;
> + return !!(smsc_read(sg->dev,
> + (SMSC_GPIO_DATA_IN_START + SMSC_BANK(off)) & SMSC_BIT(off),
> + &get));
> +}
> +
> +static void smsc_gpio_set_value(struct gpio_chip *chip,
> + unsigned off, int val)
> +{
> + unsigned bank, bit;
> + struct smsc_gpio *sg =
> + container_of(chip, struct smsc_gpio, gpio_chip);
> +
> + bank = SMSC_BANK(off);
> + bit = SMSC_BIT(off);
> +
> + mutex_lock(&sg->lock);
> + if (val)
> + sg->dat_out[bank] |= bit;
> + else
> + sg->dat_out[bank] &= ~bit;
> +
> + smsc_write(sg->dev, SMSC_GPIO_DATA_OUT_START + bank,
> + sg->dat_out[bank]);
> + mutex_unlock(&sg->lock);
> +}
> +
> +static int smsc_gpio_direction_input(struct gpio_chip *chip, unsigned off)
> +{
> + unsigned int reg;
> + struct smsc_gpio *sg =
> + container_of(chip, struct smsc_gpio, gpio_chip);
> + int reg_dir;
> +
> + mutex_lock(&sg->lock);
> + reg_dir = SMSC_CFG_START + off;
> + smsc_read(sg->dev, reg_dir, ®);
> + reg |= SMSC_GPIO_INPUT_LOW;
> + mutex_unlock(&sg->lock);
> +
> + return smsc_write(sg->dev, reg_dir, reg);
> +}
> +
> +static int smsc_gpio_direction_output(struct gpio_chip *chip,
> + unsigned off, int val)
> +{
> + unsigned int reg;
> + struct smsc_gpio *sg =
> + container_of(chip, struct smsc_gpio, gpio_chip);
> + int reg_dir;
> +
> + mutex_lock(&sg->lock);
> + reg_dir = SMSC_CFG_START + off;
> + smsc_read(sg->dev, reg_dir, ®);
> + reg |= SMSC_GPIO_OUTPUT_PP;
> + mutex_unlock(&sg->lock);
> +
> + return smsc_write(sg->dev, reg_dir, reg);
> +}
> +
> +static int smsc_gpio_to_irq(struct gpio_chip *chip, unsigned off)
> +{
> + struct smsc_gpio *sg =
> + container_of(chip, struct smsc_gpio, gpio_chip);
> + return sg->irq_base + off;
> +}
> +
> +static void smsc_irq_bus_lock(struct irq_data *d)
> +{
> + struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
> +
> + mutex_lock(&sg->irq_lock);
> +}
> +
> +static void smsc_irq_bus_sync_unlock(struct irq_data *d)
> +{
> + struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
> + int i;
> +
> + for (i = 0; i < SMSC_BANK(SMSC_MAXGPIO); i++)
> + if (sg->int_en[i] ^ sg->irq_mask[i]) {
> + sg->int_en[i] = sg->irq_mask[i];
> + smsc_write(sg->dev, SMSC_GPIO_INT_MASK_START + i,
> + sg->int_en[i]);
> + }
> +
> + mutex_unlock(&sg->irq_lock);
> +}
> +
> +static void smsc_irq_mask(struct irq_data *d)
> +{
> + struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
> + unsigned gpio = d->irq - sg->irq_base;
> +
> + sg->irq_mask[SMSC_BANK(gpio)] &= ~SMSC_BIT(gpio);
> +}
> +
> +static void smsc_irq_unmask(struct irq_data *d)
> +{
> + struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
> + unsigned gpio = d->irq - sg->irq_base;
> +
> + sg->irq_mask[SMSC_BANK(gpio)] |= SMSC_BIT(gpio);
> +}
> +
> +static int smsc_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> + struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
> + uint16_t gpio = d->irq - sg->irq_base;
> + unsigned bank, bit;
> +
> + if ((type & IRQ_TYPE_EDGE_BOTH)) {
> + dev_err(sg->dev, "irq %d: unsupported type %d\n",
> + d->irq, type);
> + return -EINVAL;
> + }
> +
> + bank = SMSC_BANK(gpio);
> + bit = SMSC_BIT(gpio);
> +
> + if (type & IRQ_TYPE_LEVEL_HIGH)
> + sg->int_lvl[bank] |= bit;
> + else if (type & IRQ_TYPE_LEVEL_LOW)
> + sg->int_lvl[bank] &= ~bit;
> + else
> + return -EINVAL;
this looks wrong. You could have a user who wants to trigger on both
HIGH and LOW levels, no ?
> + smsc_gpio_direction_input(&sg->gpio_chip, gpio);
> + smsc_write(sg->dev, SMSC_CFG_START + gpio,
> + sg->int_lvl[bank]);
> +
> + return 0;
> +}
> +
> +static struct irq_chip smsc_irq_chip = {
> + .name = "smsc",
> + .irq_mask = smsc_irq_mask,
> + .irq_unmask = smsc_irq_unmask,
> + .irq_bus_lock = smsc_irq_bus_lock,
> + .irq_bus_sync_unlock = smsc_irq_bus_sync_unlock,
> + .irq_set_type = smsc_irq_set_type,
> +};
> +
> +static int smsc_gpio_read_intstat(struct smsc_gpio *sg,
> + unsigned int *buf, int i)
> +{
> + int ret = smsc_read(sg->dev,
> + SMSC_GPIO_INT_STAT_START + i, buf);
> +
> + if (ret < 0)
> + dev_err(sg->dev, "Read INT_STAT Error\n");
> +
> + return ret;
> +}
> +
> +static irqreturn_t smsc_irq_handler(int irq, void *devid)
> +{
> + struct smsc_gpio *sg = devid;
> + unsigned int status, bank, pending;
> + int ret;
> + smsc_read(sg->dev, GRP_INT_STAT, &status);
> +
> + if (!(status & SMSC_GPI_INT))
> + goto out;
> +
> + for (bank = 0; bank <= SMSC_BANK(SMSC_MAXGPIO);
> + bank++) {
> + pending = sg->irq_stat[bank] & sg->irq_mask[bank];
> + ret = smsc_gpio_read_intstat(sg,
> + &sg->irq_stat[bank], bank);
> + if (ret < 0)
> + memset(&sg->irq_stat[bank], 0,
> + ARRAY_SIZE(sg->irq_stat));
> +
> + while (pending) {
> + unsigned long bit = __ffs(pending);
> + unsigned int irq;
> +
> + pending &= ~BIT(bit);
> + irq = bit + sg->irq_base;
> + handle_nested_irq(irq);
> + }
> + }
> +
> +out:
> + smsc_write(sg->dev, GRP_INT_STAT, status); /* Status is W1C */
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int smsc_irq_setup(struct smsc_gpio *sg)
> +{
> + unsigned gpio;
> + int ret;
> +
> + mutex_init(&sg->irq_lock);
> +
> + for (gpio = 0; gpio < sg->gpio_chip.ngpio; gpio++) {
> + int irq = gpio + sg->irq_base;
> + irq_set_chip_data(irq, sg);
> + irq_set_chip_and_handler(irq, &smsc_irq_chip,
> + handle_level_irq);
> + irq_set_nested_thread(irq, 1);
> +#ifdef CONFIG_ARM
> + set_irq_flags(irq, IRQF_VALID);
> +#else
> + irq_set_noprobe(irq);
> +#endif
> + }
> +
> + ret = request_threaded_irq(sg->irq, NULL,
> + smsc_irq_handler, sg->flags,
> + "smsc_gpio", sg);
would be nice to stick to devm_request_threaded_irq() like on the other
patch.
> + if (ret) {
> + dev_err(sg->dev, "failed to request irq %d\n",
> + sg->irq);
> + }
> +
> + sg->gpio_chip.to_irq = smsc_gpio_to_irq;
> +
> + return 0;
> +}
> +
> +static int __devinit smsc_gpio_probe(struct platform_device *pdev)
> +{
> + struct smsc_gpio *sg;
> + struct gpio_chip *gc;
> + struct smsc *smsc = dev_get_drvdata(pdev->dev.parent);
> + int ret, i, temp;
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + int irq_base;
> +
> + sg = devm_kzalloc(dev, sizeof(*sg), GFP_KERNEL);
> + if (sg == NULL) {
> + dev_err(&pdev->dev, "failed to alloc memory\n");
> + return -ENOMEM;
> + }
> +
> + sg->irq = platform_get_irq(pdev, 0);
> + if (np) {
> + of_property_read_u32(np, "gpio,base", &temp);
> + of_property_read_u32(np, "flags", &sg->flags);
> + }
> +
> + gc = &sg->gpio_chip;
> + gc->direction_input = smsc_gpio_direction_input;
> + gc->direction_output = smsc_gpio_direction_output;
> + gc->get = smsc_gpio_get_value;
> + gc->set = smsc_gpio_set_value;
> + gc->can_sleep = 1;
> +
> + gc->base = temp;
> + gc->ngpio = SMSC_MAXGPIO;
> + gc->owner = THIS_MODULE;
> +
> + sg->smsc = smsc;
> + sg->dev = dev;
> + mutex_init(&sg->lock);
> +
> + for (i = 0; i <= SMSC_BANK(SMSC_MAXGPIO); i++)
> + smsc_read(sg->dev, SMSC_GPIO_DATA_OUT_START + i,
> + &sg->dat_out[i]);
> +
> + for (i = 0; i < SMSC_MAXGPIO; i++)
> + smsc_read(sg->dev, SMSC_CFG_START + i, &sg->dir[i]);
> +
> + irq_base = irq_alloc_descs(-1, 0, sg->gpio_chip.ngpio, 0);
> + if (IS_ERR_VALUE(irq_base)) {
> + dev_err(sg->dev, "Fail to allocate IRQ descs\n");
> + return irq_base;
> + }
> + sg->irq_base = irq_base;
> +
> + irq_domain_add_legacy(pdev->dev.of_node, sg->gpio_chip.ngpio,
> + sg->irq_base, 0, &irq_domain_simple_ops, NULL);
> +
> + ret = smsc_irq_setup(sg);
> + if (ret)
> + goto err;
> +
> + ret = gpiochip_add(&sg->gpio_chip);
> + if (ret)
> + goto err;
> +
> + return 0;
> +
> +err:
> + kfree(sg);
using devm_kzalloc(), not needed to free.
> +
> + return ret;
> +}
> +
> +static int __devexit smsc_gpio_remove(struct platform_device *pdev)
> +{
> + struct smsc_gpio *sg = dev_get_drvdata(&pdev->dev);
> + int ret;
> +
> + ret = gpiochip_remove(&sg->gpio_chip);
> + if (ret) {
> + dev_err(&pdev->dev, "gpiochip_remove failed %d\n", ret);
> + return ret;
> + }
this will leak the irq (please move to devm_request_threaded_irq).
It's also leaking irq_descs and irq_domain, you need to free those
resources by calling irq_domain_remove and irq_free_descs.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [RFC/PATCH 4/4] gpio: smscece: Add support for gpio IO expander feature
2012-08-21 10:53 ` Felipe Balbi
@ 2012-08-21 11:47 ` Poddar, Sourav
2012-08-21 12:00 ` Felipe Balbi
0 siblings, 1 reply; 26+ messages in thread
From: Poddar, Sourav @ 2012-08-21 11:47 UTC (permalink / raw)
To: balbi
Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input
Hi,
On Tue, Aug 21, 2012 at 4:23 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Tue, Aug 21, 2012 at 04:15:40PM +0530, Sourav Poddar wrote:
>> smsc can be used as an gpio io expander device also. So adding
>> support for configuring smsc pins as a gpio.
>>
>> 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>
>> ---
>> drivers/gpio/Kconfig | 7 +
>> drivers/gpio/Makefile | 1 +
>> drivers/gpio/gpio-smscece.c | 373 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 381 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/gpio/gpio-smscece.c
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index b16c8a7..e883929 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -444,6 +444,13 @@ config GPIO_ADP5588_IRQ
>> Say yes here to enable the adp5588 to be used as an interrupt
>> controller. It requires the driver to be built in the kernel.
>>
>> +config GPIO_SMSCECE
>> + tristate "SMSCECE 1099 I2C GPIO expander"
>> + depends on I2C
>> + help
>> + This option enables support for 18 GPIOs found
>> + on SMSC ECE 1099 GPIO Expanders.
>> +
>> comment "PCI GPIO expanders:"
>>
>> config GPIO_CS5535
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index 153cace..7c803c5 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
>> obj-$(CONFIG_GPIO_AB8500) += gpio-ab8500.o
>> obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
>> obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
>> +obj-$(CONFIG_GPIO_SMSCECE) += gpio-smscece.o
>> obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
>> obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
>> obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
>> diff --git a/drivers/gpio/gpio-smscece.c b/drivers/gpio/gpio-smscece.c
>> new file mode 100644
>> index 0000000..0cb0959
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-smscece.c
>> @@ -0,0 +1,373 @@
>> +/*
>> + * GPIO Chip driver for smsc
>> + * SMSC I/O Expander and QWERTY Keypad Controller
>> + *
>> + * Copyright 2012 Texas Instruments Inc.
>> + *
>> + * Licensed under the GPL-2 or later.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/init.h>
>> +#include <linux/i2c.h>
>> +#include <linux/gpio.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/irq.h>
>> +#include <linux/mfd/smsc.h>
>> +#include <linux/err.h>
>> +
>> +struct smsc_gpio {
>> + struct device *dev;
>> + struct smsc *smsc;
>> + struct gpio_chip gpio_chip;
>> + struct mutex lock; /* protect cached dir, dat_out */
>> + /* protect serialized access to the interrupt controller bus */
>> + struct mutex irq_lock;
>> + unsigned gpio_start;
>> + int type;
>> + int flags;
>> + int irq;
>> + int irq_base;
>> + unsigned int gpio_base;
>> + unsigned int dat_out[5];
>> + unsigned int dir[5];
>> + unsigned int int_lvl[5];
>> + unsigned int int_en[5];
>> + unsigned int irq_mask[5];
>> + unsigned int irq_stat[5];
>> +};
>> +
>> +static int smsc_gpio_get_value(struct gpio_chip *chip, unsigned off)
>> +{
>> + struct smsc_gpio *sg =
>> + container_of(chip, struct smsc_gpio, gpio_chip);
>> + unsigned int get;
>> + return !!(smsc_read(sg->dev,
>> + (SMSC_GPIO_DATA_IN_START + SMSC_BANK(off)) & SMSC_BIT(off),
>> + &get));
>> +}
>> +
>> +static void smsc_gpio_set_value(struct gpio_chip *chip,
>> + unsigned off, int val)
>> +{
>> + unsigned bank, bit;
>> + struct smsc_gpio *sg =
>> + container_of(chip, struct smsc_gpio, gpio_chip);
>> +
>> + bank = SMSC_BANK(off);
>> + bit = SMSC_BIT(off);
>> +
>> + mutex_lock(&sg->lock);
>> + if (val)
>> + sg->dat_out[bank] |= bit;
>> + else
>> + sg->dat_out[bank] &= ~bit;
>> +
>> + smsc_write(sg->dev, SMSC_GPIO_DATA_OUT_START + bank,
>> + sg->dat_out[bank]);
>> + mutex_unlock(&sg->lock);
>> +}
>> +
>> +static int smsc_gpio_direction_input(struct gpio_chip *chip, unsigned off)
>> +{
>> + unsigned int reg;
>> + struct smsc_gpio *sg =
>> + container_of(chip, struct smsc_gpio, gpio_chip);
>> + int reg_dir;
>> +
>> + mutex_lock(&sg->lock);
>> + reg_dir = SMSC_CFG_START + off;
>> + smsc_read(sg->dev, reg_dir, ®);
>> + reg |= SMSC_GPIO_INPUT_LOW;
>> + mutex_unlock(&sg->lock);
>> +
>> + return smsc_write(sg->dev, reg_dir, reg);
>> +}
>> +
>> +static int smsc_gpio_direction_output(struct gpio_chip *chip,
>> + unsigned off, int val)
>> +{
>> + unsigned int reg;
>> + struct smsc_gpio *sg =
>> + container_of(chip, struct smsc_gpio, gpio_chip);
>> + int reg_dir;
>> +
>> + mutex_lock(&sg->lock);
>> + reg_dir = SMSC_CFG_START + off;
>> + smsc_read(sg->dev, reg_dir, ®);
>> + reg |= SMSC_GPIO_OUTPUT_PP;
>> + mutex_unlock(&sg->lock);
>> +
>> + return smsc_write(sg->dev, reg_dir, reg);
>> +}
>> +
>> +static int smsc_gpio_to_irq(struct gpio_chip *chip, unsigned off)
>> +{
>> + struct smsc_gpio *sg =
>> + container_of(chip, struct smsc_gpio, gpio_chip);
>> + return sg->irq_base + off;
>> +}
>> +
>> +static void smsc_irq_bus_lock(struct irq_data *d)
>> +{
>> + struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
>> +
>> + mutex_lock(&sg->irq_lock);
>> +}
>> +
>> +static void smsc_irq_bus_sync_unlock(struct irq_data *d)
>> +{
>> + struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
>> + int i;
>> +
>> + for (i = 0; i < SMSC_BANK(SMSC_MAXGPIO); i++)
>> + if (sg->int_en[i] ^ sg->irq_mask[i]) {
>> + sg->int_en[i] = sg->irq_mask[i];
>> + smsc_write(sg->dev, SMSC_GPIO_INT_MASK_START + i,
>> + sg->int_en[i]);
>> + }
>> +
>> + mutex_unlock(&sg->irq_lock);
>> +}
>> +
>> +static void smsc_irq_mask(struct irq_data *d)
>> +{
>> + struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
>> + unsigned gpio = d->irq - sg->irq_base;
>> +
>> + sg->irq_mask[SMSC_BANK(gpio)] &= ~SMSC_BIT(gpio);
>> +}
>> +
>> +static void smsc_irq_unmask(struct irq_data *d)
>> +{
>> + struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
>> + unsigned gpio = d->irq - sg->irq_base;
>> +
>> + sg->irq_mask[SMSC_BANK(gpio)] |= SMSC_BIT(gpio);
>> +}
>> +
>> +static int smsc_irq_set_type(struct irq_data *d, unsigned int type)
>> +{
>> + struct smsc_gpio *sg = irq_data_get_irq_chip_data(d);
>> + uint16_t gpio = d->irq - sg->irq_base;
>> + unsigned bank, bit;
>> +
>> + if ((type & IRQ_TYPE_EDGE_BOTH)) {
>> + dev_err(sg->dev, "irq %d: unsupported type %d\n",
>> + d->irq, type);
>> + return -EINVAL;
>> + }
>> +
>> + bank = SMSC_BANK(gpio);
>> + bit = SMSC_BIT(gpio);
>> +
>> + if (type & IRQ_TYPE_LEVEL_HIGH)
>> + sg->int_lvl[bank] |= bit;
>> + else if (type & IRQ_TYPE_LEVEL_LOW)
>> + sg->int_lvl[bank] &= ~bit;
>> + else
>> + return -EINVAL;
>
> this looks wrong. You could have a user who wants to trigger on both
> HIGH and LOW levels, no ?
>
Yes, I think there can be a scenario where gpio_keys are attached
to this driver and signals a "key press" at low and "key release" at
high. ?
Will figure out a way to add support to check for case where
both High and low levels are used.
>> + smsc_gpio_direction_input(&sg->gpio_chip, gpio);
>> + smsc_write(sg->dev, SMSC_CFG_START + gpio,
>> + sg->int_lvl[bank]);
>> +
>> + return 0;
>> +}
>> +
>> +static struct irq_chip smsc_irq_chip = {
>> + .name = "smsc",
>> + .irq_mask = smsc_irq_mask,
>> + .irq_unmask = smsc_irq_unmask,
>> + .irq_bus_lock = smsc_irq_bus_lock,
>> + .irq_bus_sync_unlock = smsc_irq_bus_sync_unlock,
>> + .irq_set_type = smsc_irq_set_type,
>> +};
>> +
>> +static int smsc_gpio_read_intstat(struct smsc_gpio *sg,
>> + unsigned int *buf, int i)
>> +{
>> + int ret = smsc_read(sg->dev,
>> + SMSC_GPIO_INT_STAT_START + i, buf);
>> +
>> + if (ret < 0)
>> + dev_err(sg->dev, "Read INT_STAT Error\n");
>> +
>> + return ret;
>> +}
>> +
>> +static irqreturn_t smsc_irq_handler(int irq, void *devid)
>> +{
>> + struct smsc_gpio *sg = devid;
>> + unsigned int status, bank, pending;
>> + int ret;
>> + smsc_read(sg->dev, GRP_INT_STAT, &status);
>> +
>> + if (!(status & SMSC_GPI_INT))
>> + goto out;
>> +
>> + for (bank = 0; bank <= SMSC_BANK(SMSC_MAXGPIO);
>> + bank++) {
>> + pending = sg->irq_stat[bank] & sg->irq_mask[bank];
>> + ret = smsc_gpio_read_intstat(sg,
>> + &sg->irq_stat[bank], bank);
>> + if (ret < 0)
>> + memset(&sg->irq_stat[bank], 0,
>> + ARRAY_SIZE(sg->irq_stat));
>> +
>> + while (pending) {
>> + unsigned long bit = __ffs(pending);
>> + unsigned int irq;
>> +
>> + pending &= ~BIT(bit);
>> + irq = bit + sg->irq_base;
>> + handle_nested_irq(irq);
>> + }
>> + }
>> +
>> +out:
>> + smsc_write(sg->dev, GRP_INT_STAT, status); /* Status is W1C */
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int smsc_irq_setup(struct smsc_gpio *sg)
>> +{
>> + unsigned gpio;
>> + int ret;
>> +
>> + mutex_init(&sg->irq_lock);
>> +
>> + for (gpio = 0; gpio < sg->gpio_chip.ngpio; gpio++) {
>> + int irq = gpio + sg->irq_base;
>> + irq_set_chip_data(irq, sg);
>> + irq_set_chip_and_handler(irq, &smsc_irq_chip,
>> + handle_level_irq);
>> + irq_set_nested_thread(irq, 1);
>> +#ifdef CONFIG_ARM
>> + set_irq_flags(irq, IRQF_VALID);
>> +#else
>> + irq_set_noprobe(irq);
>> +#endif
>> + }
>> +
>> + ret = request_threaded_irq(sg->irq, NULL,
>> + smsc_irq_handler, sg->flags,
>> + "smsc_gpio", sg);
>
> would be nice to stick to devm_request_threaded_irq() like on the other
> patch.
>
Yes, will change.
>> + if (ret) {
>> + dev_err(sg->dev, "failed to request irq %d\n",
>> + sg->irq);
>> + }
>> +
>> + sg->gpio_chip.to_irq = smsc_gpio_to_irq;
>> +
>> + return 0;
>> +}
>> +
>> +static int __devinit smsc_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct smsc_gpio *sg;
>> + struct gpio_chip *gc;
>> + struct smsc *smsc = dev_get_drvdata(pdev->dev.parent);
>> + int ret, i, temp;
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + int irq_base;
>> +
>> + sg = devm_kzalloc(dev, sizeof(*sg), GFP_KERNEL);
>> + if (sg == NULL) {
>> + dev_err(&pdev->dev, "failed to alloc memory\n");
>> + return -ENOMEM;
>> + }
>> +
>> + sg->irq = platform_get_irq(pdev, 0);
>> + if (np) {
>> + of_property_read_u32(np, "gpio,base", &temp);
>> + of_property_read_u32(np, "flags", &sg->flags);
>> + }
>> +
>> + gc = &sg->gpio_chip;
>> + gc->direction_input = smsc_gpio_direction_input;
>> + gc->direction_output = smsc_gpio_direction_output;
>> + gc->get = smsc_gpio_get_value;
>> + gc->set = smsc_gpio_set_value;
>> + gc->can_sleep = 1;
>> +
>> + gc->base = temp;
>> + gc->ngpio = SMSC_MAXGPIO;
>> + gc->owner = THIS_MODULE;
>> +
>> + sg->smsc = smsc;
>> + sg->dev = dev;
>> + mutex_init(&sg->lock);
>> +
>> + for (i = 0; i <= SMSC_BANK(SMSC_MAXGPIO); i++)
>> + smsc_read(sg->dev, SMSC_GPIO_DATA_OUT_START + i,
>> + &sg->dat_out[i]);
>> +
>> + for (i = 0; i < SMSC_MAXGPIO; i++)
>> + smsc_read(sg->dev, SMSC_CFG_START + i, &sg->dir[i]);
>> +
>> + irq_base = irq_alloc_descs(-1, 0, sg->gpio_chip.ngpio, 0);
>> + if (IS_ERR_VALUE(irq_base)) {
>> + dev_err(sg->dev, "Fail to allocate IRQ descs\n");
>> + return irq_base;
>> + }
>> + sg->irq_base = irq_base;
>> +
>> + irq_domain_add_legacy(pdev->dev.of_node, sg->gpio_chip.ngpio,
>> + sg->irq_base, 0, &irq_domain_simple_ops, NULL);
>> +
>> + ret = smsc_irq_setup(sg);
>> + if (ret)
>> + goto err;
>> +
>> + ret = gpiochip_add(&sg->gpio_chip);
>> + if (ret)
>> + goto err;
>> +
>> + return 0;
>> +
>> +err:
>> + kfree(sg);
>
> using devm_kzalloc(), not needed to free.
>
True, will change.
>> +
>> + return ret;
>> +}
>> +
>> +static int __devexit smsc_gpio_remove(struct platform_device *pdev)
>> +{
>> + struct smsc_gpio *sg = dev_get_drvdata(&pdev->dev);
>> + int ret;
>> +
>> + ret = gpiochip_remove(&sg->gpio_chip);
>> + if (ret) {
>> + dev_err(&pdev->dev, "gpiochip_remove failed %d\n", ret);
>> + return ret;
>> + }
>
> this will leak the irq (please move to devm_request_threaded_irq).
>
> It's also leaking irq_descs and irq_domain, you need to free those
> resources by calling irq_domain_remove and irq_free_descs.
>
Ok. will add the above apis in the next version.
> --
> balbi
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [RFC/PATCH 4/4] gpio: smscece: Add support for gpio IO expander feature
2012-08-21 11:47 ` Poddar, Sourav
@ 2012-08-21 12:00 ` Felipe Balbi
2012-08-21 12:20 ` Poddar, Sourav
0 siblings, 1 reply; 26+ messages in thread
From: Felipe Balbi @ 2012-08-21 12:00 UTC (permalink / raw)
To: Poddar, Sourav
Cc: balbi, devicetree-discuss, linux-arm-kernel, linux-omap,
linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 809 bytes --]
Hi,
On Tue, Aug 21, 2012 at 05:17:37PM +0530, Poddar, Sourav wrote:
> >> + if (type & IRQ_TYPE_LEVEL_HIGH)
> >> + sg->int_lvl[bank] |= bit;
> >> + else if (type & IRQ_TYPE_LEVEL_LOW)
> >> + sg->int_lvl[bank] &= ~bit;
> >> + else
> >> + return -EINVAL;
> >
> > this looks wrong. You could have a user who wants to trigger on both
> > HIGH and LOW levels, no ?
> >
> Yes, I think there can be a scenario where gpio_keys are attached
> to this driver and signals a "key press" at low and "key release" at
> high. ?
> Will figure out a way to add support to check for case where
> both High and low levels are used.
could probably be done on a separate patch, maybe... Just now I saw that
HIGH and LOW levels use the same bit.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH 4/4] gpio: smscece: Add support for gpio IO expander feature
2012-08-21 12:00 ` Felipe Balbi
@ 2012-08-21 12:20 ` Poddar, Sourav
2012-08-21 12:22 ` Felipe Balbi
0 siblings, 1 reply; 26+ messages in thread
From: Poddar, Sourav @ 2012-08-21 12:20 UTC (permalink / raw)
To: balbi
Cc: devicetree-discuss, linux-arm-kernel, linux-omap, linux-kernel,
linux-input
Hi,
On Tue, Aug 21, 2012 at 5:30 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Tue, Aug 21, 2012 at 05:17:37PM +0530, Poddar, Sourav wrote:
>> >> + if (type & IRQ_TYPE_LEVEL_HIGH)
>> >> + sg->int_lvl[bank] |= bit;
>> >> + else if (type & IRQ_TYPE_LEVEL_LOW)
>> >> + sg->int_lvl[bank] &= ~bit;
>> >> + else
>> >> + return -EINVAL;
>> >
>> > this looks wrong. You could have a user who wants to trigger on both
>> > HIGH and LOW levels, no ?
>> >
>> Yes, I think there can be a scenario where gpio_keys are attached
>> to this driver and signals a "key press" at low and "key release" at
>> high. ?
>> Will figure out a way to add support to check for case where
>> both High and low levels are used.
>
> could probably be done on a separate patch, maybe... Just now I saw that
> HIGH and LOW levels use the same bit.
>
If I am understanding correctly, if they both uses the same bit we cannot
use both for a particular user. ?
> --
> balbi
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH 4/4] gpio: smscece: Add support for gpio IO expander feature
2012-08-21 12:20 ` Poddar, Sourav
@ 2012-08-21 12:22 ` Felipe Balbi
2012-08-21 14:50 ` Mark Brown
0 siblings, 1 reply; 26+ messages in thread
From: Felipe Balbi @ 2012-08-21 12:22 UTC (permalink / raw)
To: Poddar, Sourav
Cc: balbi, devicetree-discuss, linux-arm-kernel, linux-omap,
linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]
On Tue, Aug 21, 2012 at 05:50:28PM +0530, Poddar, Sourav wrote:
> Hi,
>
> On Tue, Aug 21, 2012 at 5:30 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Tue, Aug 21, 2012 at 05:17:37PM +0530, Poddar, Sourav wrote:
> >> >> + if (type & IRQ_TYPE_LEVEL_HIGH)
> >> >> + sg->int_lvl[bank] |= bit;
> >> >> + else if (type & IRQ_TYPE_LEVEL_LOW)
> >> >> + sg->int_lvl[bank] &= ~bit;
> >> >> + else
> >> >> + return -EINVAL;
> >> >
> >> > this looks wrong. You could have a user who wants to trigger on both
> >> > HIGH and LOW levels, no ?
> >> >
> >> Yes, I think there can be a scenario where gpio_keys are attached
> >> to this driver and signals a "key press" at low and "key release" at
> >> high. ?
> >> Will figure out a way to add support to check for case where
> >> both High and low levels are used.
> >
> > could probably be done on a separate patch, maybe... Just now I saw that
> > HIGH and LOW levels use the same bit.
> >
> If I am understanding correctly, if they both uses the same bit we cannot
> use both for a particular user. ?
we can, it's just a bit more complex. If a user request both LOW and
HIGH, then you start with HIGH, once it triggers, before calling the
nested IRQ handler, you need to change it LOW. When low triggers, before
calling the nested IRQ handler, you need to change it to HIGH again. And
so on. I'm just not sure if that's valid on linux IRQ subsystem.
Anyone ?
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH 4/4] gpio: smscece: Add support for gpio IO expander feature
2012-08-21 12:22 ` Felipe Balbi
@ 2012-08-21 14:50 ` Mark Brown
0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2012-08-21 14:50 UTC (permalink / raw)
To: Felipe Balbi
Cc: Poddar, Sourav, devicetree-discuss, linux-arm-kernel, linux-omap,
linux-kernel, linux-input
On Tue, Aug 21, 2012 at 03:22:18PM +0300, Felipe Balbi wrote:
> On Tue, Aug 21, 2012 at 05:50:28PM +0530, Poddar, Sourav wrote:
> > If I am understanding correctly, if they both uses the same bit we cannot
> > use both for a particular user. ?
> we can, it's just a bit more complex. If a user request both LOW and
> HIGH, then you start with HIGH, once it triggers, before calling the
> nested IRQ handler, you need to change it LOW. When low triggers, before
> calling the nested IRQ handler, you need to change it to HIGH again. And
> so on. I'm just not sure if that's valid on linux IRQ subsystem.
The given example was for keypress - usually a system would use edge
triggered interrupts in combination with reading the GPIO state rather
than level triggered interrupts.
^ permalink raw reply [flat|nested] 26+ messages in thread