Message ID | 1345545940-2232-2-git-send-email-sourav.poddar@ti.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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! -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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.
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.
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
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.
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.
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.
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 */
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