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