From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752786Ab2IFFuW (ORCPT ); Thu, 6 Sep 2012 01:50:22 -0400 Received: from na3sys009aog112.obsmtp.com ([74.125.149.207]:42530 "EHLO na3sys009aog112.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751649Ab2IFFuR (ORCPT ); Thu, 6 Sep 2012 01:50:17 -0400 MIME-Version: 1.0 In-Reply-To: <5047918D.3000305@ti.com> References: <1346844964-24839-1-git-send-email-sourav.poddar@ti.com> <5047918D.3000305@ti.com> Date: Thu, 6 Sep 2012 11:20:15 +0530 Message-ID: Subject: Re: [PATCHv2 1/4] mfd: smsc: Add support for smsc gpio io/keypad driver From: "Poddar, Sourav" To: Vaibhav Hiremath Cc: devicetree-discuss@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org, linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org, linux-input@vger.kernel.org, santosh.shilimkar@ti.com, b-cousson@ti.com, balbi@ti.com, sameo@linux.intel.com Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Wed, Sep 5, 2012 at 11:23 PM, Vaibhav Hiremath wrote: > > > On 9/5/2012 5:06 PM, Sourav Poddar wrote: >> smsc ece1099 is a keyboard scan or gpio expansion device. >> The patch create keypad and gpio expander child for this >> multi function smsc driver. >> >> Cc: Samuel Ortiz >> Cc: Benoit Cousson >> Cc: Felipe Balbi >> Cc: Santosh Shilimkar >> Signed-off-by: Sourav Poddar >> --- >> Changes since v1: >> - Use Kconfig option correctly >> - Add regmap_config paramters >> - Modify formatting of logs for devid >> - Move read/write function to headed file as an inline >> function. >> Documentation/smsc_ece1099.txt | 56 ++++++++++++++++++++ >> drivers/mfd/Kconfig | 12 ++++ >> drivers/mfd/Makefile | 1 + >> drivers/mfd/smsc-ece1099.c | 110 +++++++++++++++++++++++++++++++++++++++ >> include/linux/mfd/smsc.h | 111 ++++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 290 insertions(+), 0 deletions(-) >> create mode 100644 Documentation/smsc_ece1099.txt >> create mode 100644 drivers/mfd/smsc-ece1099.c >> create mode 100644 include/linux/mfd/smsc.h >> >> diff --git a/Documentation/smsc_ece1099.txt b/Documentation/smsc_ece1099.txt >> new file mode 100644 >> index 0000000..6b492e8 >> --- /dev/null >> +++ b/Documentation/smsc_ece1099.txt >> @@ -0,0 +1,56 @@ >> +What is smsc-ece1099? >> +---------------------- >> + >> +The ECE1099 is a 40-Pin 3.3V Keyboard Scan Expansion >> +or GPIO Expansion device. The device supports a keyboard >> +scan matrix of 23x8. The device is connected to a Master >> +via the SMSC BC-Link interface or via the SMBus. >> +Keypad scan Input(KSI) and Keypad Scan Output(KSO) signals >> +are multiplexed with GPIOs. >> + >> +Interrupt generation >> +-------------------- >> + >> +Interrupts can be generated by an edge detection on a GPIO >> +pin or an edge detection on one of the bus interface pins. >> +Interrupts can also be detected on the keyboard scan interface. >> +The bus interrupt pin (BC_INT# or SMBUS_INT#) is asserted if >> +any bit in one of the Interrupt Status registers is 1 and >> +the corresponding Interrupt Mask bit is also 1. >> + >> +In order for software to determine which device is the source >> +of an interrupt, it should first read the Group Interrupt Status Register >> +to determine which Status register group is a source for the interrupt. >> +Software should read both the Status register and the associated Mask register, >> +then AND the two values together. Bits that are 1 in the result of the AND >> +are active interrupts. Software clears an interrupt by writing a 1 to the >> +corresponding bit in the Status register. >> + >> +Communication Protocol >> +---------------------- >> + >> +- SMbus slave Interface >> + The host processor communicates with the ECE1099 device >> + through a series of read/write registers via the SMBus >> + interface. SMBus is a serial communication protocol between >> + a computer host and its peripheral devices. The SMBus data >> + rate is 10KHz minimum to 400 KHz maximum >> + >> +- Slave Bus Interface >> + The ECE1099 device SMBus implementation is a subset of the >> + SMBus interface to the host. The device is a slave-only SMBus device. >> + The implementation in the device is a subset of SMBus since it >> + only supports four protocols. >> + >> + The Write Byte, Read Byte, Send Byte, and Receive Byte protocols are the >> + only valid SMBus protocols for the device. >> + >> +- BC-LinkTM Interface >> + The BC-Link is a proprietary bus that allows communication >> + between a Master device and a Companion device. The Master >> + device uses this serial bus to read and write registers >> + located on the Companion device. The bus comprises three signals, >> + BC_CLK, BC_DAT and BC_INT#. The Master device always provides the >> + clock, BC_CLK, and the Companion device is the source for an >> + independent asynchronous interrupt signal, BC_INT#. The ECE1099 >> + supports BC-Link speeds up to 24MHz. >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index d1facef..991ef15 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -385,6 +385,18 @@ config MFD_T7L66XB >> help >> Support for Toshiba Mobile IO Controller T7L66XB >> >> +config MFD_SMSC >> + bool "Support for the SMSC ECE1099 series chips" >> + depends on I2C=y >> + select MFD_CORE >> + select REGMAP_I2C >> + help >> + If you say yes here you get support for the >> + ece1099 chips from SMSC. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called smsc. >> + >> config MFD_TC6387XB >> bool "Support Toshiba TC6387XB" >> depends on ARM && HAVE_CLK >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index 79dd22d..f587d91 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -77,6 +77,7 @@ obj-$(CONFIG_EZX_PCAP) += ezx-pcap.o >> obj-$(CONFIG_MCP) += mcp-core.o >> obj-$(CONFIG_MCP_SA11X0) += mcp-sa11x0.o >> obj-$(CONFIG_MCP_UCB1200) += ucb1x00-core.o >> +obj-$(CONFIG_MFD_SMSC) += smsc.o >> obj-$(CONFIG_MCP_UCB1200_TS) += ucb1x00-ts.o >> >> ifeq ($(CONFIG_SA1100_ASSABET),y) >> diff --git a/drivers/mfd/smsc-ece1099.c b/drivers/mfd/smsc-ece1099.c >> new file mode 100644 >> index 0000000..73a6cb7 >> --- /dev/null >> +++ b/drivers/mfd/smsc-ece1099.c >> @@ -0,0 +1,110 @@ >> +/* >> + * TI SMSC MFD Driver >> + * >> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com >> + * >> + * Author: Sourav Poddar >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +static struct regmap_config smsc_regmap_config = { >> + .reg_bits = 8, >> + .val_bits = 8, >> + .max_register = SMSC_MAX_REGISTER - 1; >> + .cache_type = REGCACHE_COMPRESSED, >> +}; > > You can remove one extra tab here. > Ok. >> + >> +static const struct of_device_id of_smsc_match[] = { >> + { .compatible = "smsc,ece1099", }, >> + { }, >> +}; >> + >> +static int smsc_i2c_probe(struct i2c_client *i2c, >> + const struct i2c_device_id *id) >> +{ >> + struct device_node *node = i2c->dev.of_node; >> + struct smsc *smsc; >> + int ret = 0; >> + >> + smsc = devm_kzalloc(&i2c->dev, sizeof(struct smsc), >> + GFP_KERNEL); >> + > > You must check the return value here? > Yes, will include the check. >> + smsc->regmap = devm_regmap_init_i2c(i2c, &smsc_regmap_config); >> + if (IS_ERR(smsc->regmap)) { >> + ret = PTR_ERR(smsc->regmap); >> + goto err; > > You can simply return from here right? > hmmm, yes. >> + } >> + >> + i2c_set_clientdata(i2c, smsc); >> + smsc->dev = &i2c->dev; >> + >> +#ifdef CONFIG_OF >> + of_property_read_u32(node, "clock", &smsc->clk); >> +#endif >> + >> + regmap_read(smsc->regmap, SMSC_DEV_ID, &devid); >> + regmap_read(smsc->regmap, SMSC_DEV_REV, &rev); >> + regmap_read(smsc->regmap, SMSC_VEN_ID_L, &venid_l); >> + regmap_read(smsc->regmap, SMSC_VEN_ID_H, &venid_h); >> + > > You also want to check return value here. > I dont see any point checking for the above ret values. These values are read during runtime and some can be zero also. ? >> + dev_info(&i2c->dev, "SMSCxxx devid: %02x rev: %02x venid: %02x\n", >> + devid, rev, (venid_h << 8) | venid_l); >> + >> + ret = regmap_write(smsc->regmap, SMSC_CLK_CTRL, smsc->clk); >> + if (ret) >> + goto err; >> + >> + if (node) >> + ret = of_platform_populate(node, NULL, NULL, &i2c->dev); >> + > > of_xxx above is encapsulated in CONFIG_OF macro, same is not done here. > Will include. >> + return ret; >> + > > This return is not required, as next instruction. Infact I would suggest > you can simply return from each places above. > Ok. > Thanks, > Vaibhav >> +err: >> + return ret; >> +} >> + >> +static int smsc_i2c_remove(struct i2c_client *i2c) >> +{ >> + return 0; >> +} >> + >> +static const struct i2c_device_id smsc_i2c_id[] = { >> + { "smscece1099", 0}, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(i2c, smsc_i2c_id); >> + >> +static struct i2c_driver smsc_i2c_driver = { >> + .driver = { >> + .name = "smsc", >> + .of_match_table = of_smsc_match, >> + .owner = THIS_MODULE, >> + }, >> + .probe = smsc_i2c_probe, >> + .remove = smsc_i2c_remove, >> + .id_table = smsc_i2c_id, >> +}; >> + >> +module_i2c_driver(smsc_i2c_driver); >> + >> +MODULE_AUTHOR("Sourav Poddar "); >> +MODULE_DESCRIPTION("SMSC chip multi-function driver"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/include/linux/mfd/smsc.h b/include/linux/mfd/smsc.h >> new file mode 100644 >> index 0000000..adfaec2 >> --- /dev/null >> +++ b/include/linux/mfd/smsc.h >> @@ -0,0 +1,111 @@ >> +/* >> + * SMSC ECE1099 >> + * >> + * Copyright 2012 Texas Instruments Inc. >> + * >> + * Author: Sourav Poddar >> + * >> + * 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 >> + >> +#define SMSC_ID_ECE1099 1 >> +#define SMSC_NUM_CLIENTS 2 >> + >> +#define SMSC_BASE_ADDR 0x38 >> +#define OMAP_GPIO_SMSC_IRQ 151 >> + >> +#define SMSC_MAXGPIO 32 >> +#define SMSC_BANK(offs) ((offs) >> 3) >> +#define SMSC_BIT(offs) (1u << ((offs) & 0x7)) >> + >> +struct smsc { >> + struct device *dev; >> + struct i2c_client *i2c_clients[SMSC_NUM_CLIENTS]; >> + struct regmap *regmap; >> + int clk; >> + /* Stored chip id */ >> + int id; >> +}; >> + >> +struct smsc_gpio; >> +struct smsc_keypad; >> + >> +static inline int smsc_read(struct device *child, unsigned int reg, >> + unsigned int *dest) >> +{ >> + struct smsc *smsc = dev_get_drvdata(child->parent); >> + >> + return regmap_read(smsc->regmap, reg, dest); >> +} >> + >> +static inline int smsc_write(struct device *child, unsigned int reg, >> + unsigned int value) >> +{ >> + struct smsc *smsc = dev_get_drvdata(child->parent); >> + >> + return regmap_write(smsc->regmap, reg, value); >> +} >> + >> +/* Registers for SMSC */ >> +#define SMSC_RESET 0xF5 >> +#define SMSC_GRP_INT 0xF9 >> +#define SMSC_CLK_CTRL 0xFA >> +#define SMSC_WKUP_CTRL 0xFB >> +#define SMSC_DEV_ID 0xFC >> +#define SMSC_DEV_REV 0xFD >> +#define SMSC_VEN_ID_L 0xFE >> +#define SMSC_VEN_ID_H 0xFF >> + >> +#define SMSC_MAX_REG (SMSC_VEN_ID_H + 1) >> + >> +/* CLK VALUE */ >> +#define SMSC_CLK_VALUE 0x13 >> + >> +/* Registers for function GPIO INPUT */ >> +#define SMSC_GPIO_DATA_IN_START 0x00 >> + >> +/* Registers for function GPIO OUPUT */ >> +#define SMSC_GPIO_DATA_OUT_START 0x05 >> + >> +/* Definitions for SMSC GPIO CONFIGURATION REGISTER*/ >> +#define SMSC_GPIO_INPUT_LOW 0x01 >> +#define SMSC_GPIO_INPUT_RISING 0x09 >> +#define SMSC_GPIO_INPUT_FALLING 0x11 >> +#define SMSC_GPIO_INPUT_BOTH_EDGE 0x19 >> +#define SMSC_GPIO_OUTPUT_PP 0x21 >> +#define SMSC_GPIO_OUTPUT_OP 0x31 >> + >> +#define GRP_INT_STAT 0xf9 >> +#define SMSC_GPI_INT 0x0f >> +#define SMSC_CFG_START 0x0A >> + >> +/* Registers for SMSC GPIO INTERRUPT STATUS REGISTER*/ >> +#define SMSC_GPIO_INT_STAT_START 0x32 >> + >> +/* Registers for SMSC GPIO INTERRUPT MASK REGISTER*/ >> +#define SMSC_GPIO_INT_MASK_START 0x37 >> + >> +/* Registers for SMSC function KEYPAD*/ >> +#define SMSC_KP_OUT 0x40 >> +#define SMSC_KP_IN 0x41 >> +#define SMSC_KP_INT_STAT 0x42 >> +#define SMSC_KP_INT_MASK 0x43 >> + >> +/* Definitions for keypad */ >> +#define SMSC_KP_KSO 0x70 >> +#define SMSC_KP_KSI 0x51 >> +#define SMSC_KSO_ALL_LOW 0x20 >> +#define SMSC_KP_SET_LOW_PWR 0x0B >> +#define SMSC_KP_SET_HIGH 0xFF >> +#define SMSC_KSO_EVAL 0x00 >> + >> +#endif /* __LINUX_MFD_SMSC_H */ >>