| 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: <20100521120347.GP30801@buzzloop.caiaq.de>
Date: Fri, 21 May 2010 14:03:48 +0200
From: Daniel Mack <daniel@...aq.de>
To: Hemanth V <hemanthv@...com>
Cc: linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
linux-input@...r.kernel.org
Subject: Re: [RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver
Hi,
On Fri, May 21, 2010 at 05:05:50PM +0530, Hemanth V wrote:
> Corrected the mailing list, should be linux-kernel
>
> This patch adds support for ROHM BH1780GLI Ambient light sensor.
>
> BH1780 supports I2C interface. Driver supports read/update of power state and
> read of lux value (through SYSFS). Writing value 3 to power_state enables the
> sensor and current lux value could be read.
FWIW, this looks pretty good to me, just some minor things below.
Daniel
> Signed-off-by: Hemanth V <hemanthv@...com>
> ---
> drivers/misc/Kconfig | 11 ++
> drivers/misc/Makefile | 1 +
> drivers/misc/bh1780gli.c | 278 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 290 insertions(+), 0 deletions(-)
> create mode 100644 drivers/misc/bh1780gli.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 0d0d625..0687a0c 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -278,6 +278,17 @@ config SENSORS_TSL2550
> This driver can also be built as a module. If so, the module
> will be called tsl2550.
>
> +config SENSORS_BH1780
> + tristate "ROHM BH1780GLI ambient light sensor"
> + depends on I2C && SYSFS
> + help
> + If you say yes here you get support for the ROHM BH1780GLI
> + ambient light sensor.
> +
> + This driver can also be built as a module. If so, the module
> + will be called bh1780gli.
> +
> +
> config EP93XX_PWM
> tristate "EP93xx PWM support"
> depends on ARCH_EP93XX
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 7b6f7ee..c479d91 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -30,3 +30,4 @@ obj-$(CONFIG_IWMC3200TOP) += iwmc3200top/
> obj-y += eeprom/
> obj-y += cb710/
> obj-$(CONFIG_VMWARE_BALLOON) += vmware_balloon.o
> +obj-$(CONFIG_SENSORS_BH1780) += bh1780gli.o
> diff --git a/drivers/misc/bh1780gli.c b/drivers/misc/bh1780gli.c
> new file mode 100644
> index 0000000..9b4137d
> --- /dev/null
> +++ b/drivers/misc/bh1780gli.c
> @@ -0,0 +1,278 @@
> +/*
> + * bh1780gli.c
> + * ROHM Ambient Light Sensor Driver
> + *
> + * Copyright (C) 2010 Texas Instruments
> + * Author: Hemanth V <hemanthv@...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.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +
> +#define BH1780_REG_CONTROL 0x80
> +#define BH1780_REG_PARTID 0x8A
> +#define BH1780_REG_MANFID 0x8B
> +#define BH1780_REG_DLOW 0x8C
> +#define BH1780_REG_DHIGH 0x8D
> +
> +#define BH1780_REVMASK (0xf)
> +#define BH1780_POWMASK (0x3)
> +#define BH1780_POFF (0x0)
> +#define BH1780_PON (0x3)
Just a nit ... something's wrong with the indentation here.
> +/* power on settling time in ms */
> +#define BH1780_PON_DELAY 2
> +
> +struct bh1780_data {
> + struct i2c_client *client;
> + int power_state;
> + /* lock for sysfs operations */
> + struct mutex lock;
> +};
> +
> +int bh1780_write(struct bh1780_data *ddata, u8 reg, u8 val, char *msg)
Should be static.
> +{
> + int ret = i2c_smbus_write_byte_data(ddata->client, reg, val);
> + if (ret < 0)
> + dev_err(&ddata->client->dev,
> + "i2c_smbus_write_byte_data failed error %d\
> + Register (%s)\n", ret, msg);
> + return ret;
> +}
> +
> +int bh1780_read(struct bh1780_data *ddata, u8 reg, char *msg)
Dito.
> +{
> + int ret = i2c_smbus_read_byte_data(ddata->client, reg);
> + if (ret < 0)
> + dev_err(&ddata->client->dev,
> + "i2c_smbus_read_byte_data failed error %d\
> + Register (%s)\n", ret, msg);
> + return ret;
> +}
> +
> +static ssize_t bh1780_show_lux(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> +
> + struct platform_device *pdev = to_platform_device(dev);
> + struct bh1780_data *ddata = platform_get_drvdata(pdev);
> + int lsb, msb;
> +
> + lsb = bh1780_read(ddata, BH1780_REG_DLOW, "DLOW");
> + if (lsb < 0)
> + return lsb;
> +
> + msb = bh1780_read(ddata, BH1780_REG_DHIGH, "DHIGH");
> + if (msb < 0)
> + return msb;
> +
> + return sprintf(buf, "%d\n", (msb << 8) | lsb);
> +}
> +
> +static ssize_t bh1780_show_power_state(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct bh1780_data *ddata = platform_get_drvdata(pdev);
> + int state;
> +
> + state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
> + if (state < 0)
> + return state;
> +
> + return sprintf(buf, "%d\n", state & BH1780_POWMASK);
> +}
> +
> +static ssize_t bh1780_store_power_state(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct bh1780_data *ddata = platform_get_drvdata(pdev);
> + unsigned long val;
> + int error;
> +
> + error = strict_strtoul(buf, 0, &val);
> + if (error)
> + return error;
> +
> + if (val < BH1780_POFF || val > BH1780_PON)
> + return -EINVAL;
> +
> + mutex_lock(&ddata->lock);
> +
> + error = bh1780_write(ddata, BH1780_REG_CONTROL, val, "CONTROL");
> + if (error < 0) {
> + mutex_unlock(&ddata->lock);
> + return error;
> + }
> +
> + msleep(BH1780_PON_DELAY);
Hmm, what do you wait for here?
> + ddata->power_state = val;
> + mutex_unlock(&ddata->lock);
And I don't get the usage of the mutex here. Can you explain?
> + return count;
> +
> +}
Remove the empty line. Or move it above the return statement.
> +
> +static DEVICE_ATTR(lux, S_IRUGO, bh1780_show_lux, NULL);
> +
> +static DEVICE_ATTR(power_state, S_IWUSR | S_IRUGO,
> + bh1780_show_power_state, bh1780_store_power_state);
> +
> +static struct attribute *bh1780_attributes[] = {
> + &dev_attr_power_state.attr,
> + &dev_attr_lux.attr,
> + NULL
> +};
> +
> +static const struct attribute_group bh1780_attr_group = {
> + .attrs = bh1780_attributes,
> +};
> +
> +static int __devinit bh1780_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + int ret;
> + struct bh1780_data *ddata = NULL;
The initialization isn't needed.
> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) {
> + ret = -EIO;
> + goto err_op_failed;
> + }
> +
> + ddata = kzalloc(sizeof(struct bh1780_data), GFP_KERNEL);
> + if (ddata == NULL) {
> + ret = -ENOMEM;
> + goto err_op_failed;
> + }
> +
> + ddata->client = client;
> + i2c_set_clientdata(client, ddata);
> +
> + ret = bh1780_read(ddata, BH1780_REG_PARTID, "PART ID");
> + if (ret < 0)
> + goto err_op_failed;
> +
> + dev_info(&client->dev, "Ambient Light Sensor, Rev : %d\n",
> + (ret & BH1780_REVMASK));
> +
> + mutex_init(&ddata->lock);
> +
> + ret = sysfs_create_group(&client->dev.kobj, &bh1780_attr_group);
> + if (ret)
> + goto err_op_failed;
> +
> + return 0;
> +
> +err_op_failed:
> + if (ddata != NULL)
> + kfree(ddata);
kfree() is resitant against NULL pointers, so you can call it
unconditionally.
> +
> + return ret;
> +}
> +
> +static int __devexit bh1780_remove(struct i2c_client *client)
> +{
> + struct bh1780_data *ddata;
> +
> + ddata = i2c_get_clientdata(client);
> + sysfs_remove_group(&client->dev.kobj, &bh1780_attr_group);
> +
> + if (ddata != NULL) {
> + i2c_set_clientdata(client, NULL);
> + kfree(ddata);
> + }
Same here.
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int bh1780_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> + struct bh1780_data *ddata;
> + int state, ret;
> +
> + ddata = i2c_get_clientdata(client);
> + state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
> + if (state < 0)
> + return state;
> +
> + ddata->power_state = state & BH1780_POWMASK;
> +
> + ret = bh1780_write(ddata, BH1780_REG_CONTROL, BH1780_POFF,
> + "CONTROL");
> +
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int bh1780_resume(struct i2c_client *client)
> +{
> + struct bh1780_data *ddata;
> + int state, ret;
> +
> + ddata = i2c_get_clientdata(client);
> + state = ddata->power_state;
> +
> + ret = bh1780_write(ddata, BH1780_REG_CONTROL, state,
> + "CONTROL");
> +
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
#else
static inline int bh1780_suspend(struct i2c_client *client, pm_message_t mesg) {}
static inline int bh1780_resume(struct i2c_client *client) {}
> +#endif
> +
> +static const struct i2c_device_id bh1780_id[] = {
> + { "bh1780", 0 },
> + { },
> +};
> +
> +static struct i2c_driver bh1780_driver = {
> + .probe = bh1780_probe,
> + .remove = bh1780_remove,
> + .id_table = bh1780_id,
> +#ifdef CONFIG_PM
... then this #ifdef can go away.
> + .suspend = bh1780_suspend,
> + .resume = bh1780_resume,
> +#endif
> + .driver = {
> + .name = "bh1780"
> + },
> +};
> +
> +static int __init bh1780_init(void)
> +{
> + return i2c_add_driver(&bh1780_driver);
> +}
> +
> +static void __exit bh1780_exit(void)
> +{
> + i2c_del_driver(&bh1780_driver);
> +}
> +
> +module_init(bh1780_init)
> +module_exit(bh1780_exit)
> +
> +MODULE_DESCRIPTION("BH1780GLI Ambient Light Sensor Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Hemanth V <hemanthv@...com>");
> --
> 1.5.6.3
>
> --
> 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/
--
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