* [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
@ 2011-03-14 19:45 Jon Brenner
2011-03-21 19:07 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Jon Brenner @ 2011-03-14 19:45 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, Linux Kernel
From: Jon Brenner <jbrenner@taosinc.com>
Support for TAOS tsl2580/01/03 ALS devices.
Uses sysfs/iio methods.
Signed-off-by: Jon August Brenner <jbrenner@taosinc.com>
---
drivers/staging/iio/Documentation/sysfs-bus-iio | 6 +
.../staging/iio/Documentation/sysfs-bus-iio-light | 7 +
drivers/staging/iio/light/Kconfig | 9 +
drivers/staging/iio/light/Makefile | 1 +
drivers/staging/iio/light/tsl2583.c | 943 ++++++++++++++++++++
5 files changed, 966 insertions(+), 0 deletions(-)
diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio b/drivers/staging/iio/Documentation/sysfs-bus-iio
index 2dde97d..6a86ad2 100644
--- a/drivers/staging/iio/Documentation/sysfs-bus-iio
+++ b/drivers/staging/iio/Documentation/sysfs-bus-iio
@@ -6,6 +6,12 @@ Description:
Corresponds to a grouping of sensor channels. X is the IIO
index of the device.
+What: /sys/bus/iio/devices/device[n]/power_state
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets/sets the device power state.
+
What: /sys/bus/iio/devices/triggerX
KernelVersion: 2.6.35
Contact: linux-iio@vger.kernel.org
diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
index 5d84856..b59cdb4 100644
--- a/drivers/staging/iio/Documentation/sysfs-bus-iio-light
+++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
@@ -62,3 +62,10 @@ Description:
sensing mode. This value should be the output from a reading
and if expressed in SI units, should include _input. If this
value is not in SI units, then it should include _raw.
+
+What: /sys/bus/iio/devices/device[n]/lux_table
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets/sets the table of coefficients
+ used in calculating illuminance in lux.
diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
index 36d8bbe..a295e82 100644
--- a/drivers/staging/iio/light/Kconfig
+++ b/drivers/staging/iio/light/Kconfig
@@ -13,6 +13,15 @@ config SENSORS_TSL2563
This driver can also be built as a module. If so, the module
will be called tsl2563.
+config SENSORS_TSL2583
+ tristate "TAOS TSL2580, TSL2581, and TSL2583 light-to-digital converters"
+ depends on I2C
+ help
+ Y = in kernel.
+ M = as module.
+ Provides support for the TAOS tsl2580, tsl2581, and tsl2583 devices.
+ Access ALS data via iio, sysfs.
+
config SENSORS_ISL29018
tristate "ISL 29018 light and proximity sensor"
depends on I2C
diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile
index 9142c0e..9d13b8d 100644
--- a/drivers/staging/iio/light/Makefile
+++ b/drivers/staging/iio/light/Makefile
@@ -3,4 +3,5 @@
#
obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
+obj-$(CONFIG_SENSORS_TSL2583) += tsl2583.o
obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o
diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
new file mode 100644
index 0000000..96e4487
--- /dev/null
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -0,0 +1,943 @@
+/*
+ * Device driver for monitoring ambient light intensity (lux)
+ * within the TAOS tsl258x family of devices (tsl2580, tsl2581).
+ *
+ * Copyright (c) 2011, TAOS Corporation.
+ *
+ * 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.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/i2c.h>
+#include <linux/errno.h>
+#include <linux/delay.h>
+#include <linux/string.h>
+#include <linux/mutex.h>
+#include "../iio.h"
+
+
+#define MAX_DEVICE_REGS 32
+
+/* Triton register offsets */
+#define TAOS_REG_MAX 8
+
+/* Device Registers and Masks */
+#define TSL258X_CNTRL 0x00
+#define TSL258X_ALS_TIME 0X01
+#define TSL258X_INTERRUPT 0x02
+#define TSL258X_GAIN 0x07
+#define TSL258X_REVID 0x11
+#define TSL258X_CHIPID 0x12
+#define TSL258X_ALS_CHAN0LO 0x14
+#define TSL258X_ALS_CHAN0HI 0x15
+#define TSL258X_ALS_CHAN1LO 0x16
+#define TSL258X_ALS_CHAN1HI 0x17
+#define TSL258X_TMR_LO 0x18
+#define TSL258X_TMR_HI 0x19
+
+/* tsl2583 cmd reg masks */
+#define TSL258X_CMD_REG 0x80
+#define TSL258X_CMD_SPL_FN 0x60
+#define TSL258X_CMD_ALS_INT_CLR 0X01
+
+/* tsl2583 cntrl reg masks */
+#define TSL258X_CNTL_ADC_ENBL 0x02
+#define TSL258X_CNTL_PWR_ON 0x01
+
+/* tsl2583 status reg masks */
+#define TSL258X_STA_ADC_VALID 0x01
+#define TSL258X_STA_ADC_INTR 0x10
+
+/* Lux calculation constants */
+#define LUX_CALC_OVER_FLOW 65535
+
+enum {
+ TAOS_CHIP_UNKNOWN = 0, TAOS_CHIP_WORKING = 1, TAOS_CHIP_SLEEP = 2
+} TAOS_CHIP_WORKING_STATUS;
+
+/* Per-device data */
+struct taos_als_info {
+ u16 als_ch0;
+ u16 als_ch1;
+ u16 lux;
+};
+
+struct taos_settings {
+ int als_time;
+ int als_gain;
+ int als_gain_trim;
+ int als_cal_target;
+};
+
+struct tsl2583_chip {
+ struct mutex als_mutex;
+ struct i2c_client *client;
+ struct iio_dev *iio_dev;
+ struct delayed_work update_lux;
+ unsigned int addr;
+ char taos_id;
+ char valid;
+ unsigned long last_updated;
+ struct taos_als_info als_cur_info;
+ struct taos_settings taos_settings;
+ int als_time_scale;
+ int als_saturation;
+ int taos_chip_status;
+ u8 taos_config[8];
+};
+
+static int taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val,
+ unsigned int len);
+static int taos_i2c_write_command(struct i2c_client *client, u8 reg);
+
+/*
+ * Initial values for device - this values can/will be changed by driver.
+ * and applications as needed.
+ * These values are dynamic.
+ */
+static const u8 taos_config[8] = {
+ 0x00, 0xee, 0x00, 0x03, 0x00, 0xFF, 0xFF, 0x00
+}; /* cntrl atime intC Athl0 Athl1 Athh0 Athh1 gain */
+
+struct taos_lux {
+ unsigned int ratio;
+ unsigned int ch0;
+ unsigned int ch1;
+};
+
+/* This structure is intentionally large to accommodate updates via sysfs. */
+/* Sized to 11 = max 10 segments + 1 termination segment */
+/* Assumption is is one and only one type of glass used */
+struct taos_lux taos_device_lux[11] = {
+ { 9830, 8520, 15729 },
+ { 12452, 10807, 23344 },
+ { 14746, 6383, 11705 },
+ { 17695, 4063, 6554 },
+};
+
+struct taos_lux taos_lux;
+
+struct gainadj {
+ s16 ch0;
+ s16 ch1;
+};
+
+/* Index = (0 - 3) Used to validate the gain selection index */
+static const struct gainadj gainadj[] = {
+ { 1, 1 },
+ { 8, 8 },
+ { 16, 16 },
+ { 107, 115 }
+};
+
+/*
+ * Provides initial operational parameter defaults.
+ * These defaults may be changed through the device's sysfs files.
+ */
+static void taos_defaults(struct tsl2583_chip *chip)
+{
+ /* Operational parameters */
+ chip->taos_settings.als_time = 450;
+ /* must be a multiple of 50mS */
+ chip->taos_settings.als_gain = 2;
+ /* this is actually an index into the gain table */
+ /* assume clear glass as default */
+ chip->taos_settings.als_gain_trim = 1000;
+ /* default gain trim to account for aperture effects */
+ chip->taos_settings.als_cal_target = 130;
+ /* Known external ALS reading used for calibration */
+}
+
+/*
+ * Read a number of bytes starting at register (reg) location.
+ * Return 0, or i2c_smbus_write_byte ERROR code.
+ */
+static int
+taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned int len)
+{
+ int ret;
+ int i;
+
+ for (i = 0; i < len; i++) {
+ /* select register to write */
+ ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | reg));
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_i2c_read failed to write"
+ " register %x\n", reg);
+ return ret;
+ }
+ /* read the data */
+ *val = i2c_smbus_read_byte(client);
+ val++;
+ reg++;
+ }
+ return 0;
+}
+
+/*
+ * This function is used to send a command to device command/control register
+ * All bytes sent using this command have their MSBit set - it's a command!
+ * Return 0, or i2c_smbus_write_byte error code.
+ */
+static int taos_i2c_write_command(struct i2c_client *client, u8 reg)
+{
+ int ret;
+
+ /* write the data */
+ ret = i2c_smbus_write_byte(client, (reg |= TSL258X_CMD_REG));
+ if (ret < 0) {
+ dev_err(&client->dev, "FAILED: i2c_smbus_write_byte\n");
+ return ret;
+ }
+ return 0;
+}
+
+/*
+ * Reads and calculates current lux value.
+ * The raw ch0 and ch1 values of the ambient light sensed in the last
+ * integration cycle are read from the device.
+ * Time scale factor array values are adjusted based on the integration time.
+ * The raw values are multiplied by a scale factor, and device gain is obtained
+ * using gain index. Limit checks are done next, then the ratio of a multiple
+ * of ch1 value, to the ch0 value, is calculated. The array taos_device_lux[]
+ * declared above is then scanned to find the first ratio value that is just
+ * above the ratio we just calculated. The ch0 and ch1 multiplier constants in
+ * the array are then used along with the time scale factor array values, to
+ * calculate the lux.
+ */
+static int taos_get_lux(struct i2c_client *client)
+{
+ u32 ch0, ch1; /* separated ch0/ch1 data from device */
+ u32 lux; /* raw lux calculated from device data */
+ u32 ratio;
+ u8 buf[5];
+ struct taos_lux *p;
+ struct tsl2583_chip *chip = i2c_get_clientdata(client);
+ int i, ret;
+ u32 ch0lux = 0;
+ u32 ch1lux = 0;
+
+ if (mutex_trylock(&chip->als_mutex) == 0) {
+ dev_info(&client->dev, "taos_get_lux device is busy\n");
+ return chip->als_cur_info.lux; /* busy, so return LAST VALUE */
+ }
+
+ if (chip->taos_chip_status != TAOS_CHIP_WORKING) {
+ /* device is not enabled */
+ dev_err(&client->dev, "taos_get_lux device is not enabled\n");
+ ret = -EBUSY ;
+ goto out_unlock;
+ }
+
+ ret = taos_i2c_read(client, (TSL258X_CMD_REG), &buf[0], 1);
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_get_lux failed to read CMD_REG\n");
+ goto out_unlock;
+ }
+ /* is data new & valid */
+ if (!(buf[0] & TSL258X_STA_ADC_INTR)) {
+ dev_err(&client->dev, "taos_get_lux data not valid\n");
+ ret = chip->als_cur_info.lux; /* return LAST VALUE */
+ goto out_unlock;
+ }
+
+ for (i = 0; i < 4; i++) {
+ int reg = TSL258X_CMD_REG | (TSL258X_ALS_CHAN0LO + i);
+ ret = taos_i2c_read(client, reg, &buf[i], 1);
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_get_lux failed to read"
+ " register %x\n", reg);
+ goto out_unlock;
+ }
+ }
+
+ /* clear status, really interrupt status (interrupts are off), but
+ * we use the bit anyway */
+ ret = taos_i2c_write_command(client,
+ TSL258X_CMD_REG | TSL258X_CMD_SPL_FN | TSL258X_CMD_ALS_INT_CLR);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "taos_i2c_write_command failed in taos_get_lux, err = %d\n",
+ ret);
+ goto out_unlock; /* have no data, so return failure */
+ }
+
+ /* extract ALS/lux data */
+ ch0 = (buf[1] << 8) | buf[0];
+ ch1 = (buf[3] << 8) | buf[2];
+
+ chip->als_cur_info.als_ch0 = ch0;
+ chip->als_cur_info.als_ch1 = ch1;
+
+ if ((ch0 >= chip->als_saturation) || (ch1 >= chip->als_saturation))
+ goto return_max;
+
+ if (ch0 == 0) {
+ /* have no data, so return LAST VALUE */
+ ret = chip->als_cur_info.lux = 0;
+ goto out_unlock;
+ }
+ /* calculate ratio */
+ ratio = (ch1 << 15) / ch0;
+ /* convert to unscaled lux using the pointer to the table */
+ for (p = (struct taos_lux *) taos_device_lux;
+ p->ratio != 0 && p->ratio < ratio; p++)
+ ;
+
+ if (p->ratio == 0) {
+ lux = 0;
+ } else {
+ ch0lux = ((ch0 * p->ch0) +
+ (gainadj[chip->taos_settings.als_gain].ch0 >> 1))
+ / gainadj[chip->taos_settings.als_gain].ch0;
+ ch1lux = ((ch1 * p->ch1) +
+ (gainadj[chip->taos_settings.als_gain].ch1 >> 1))
+ / gainadj[chip->taos_settings.als_gain].ch1;
+ lux = ch0lux - ch1lux;
+ }
+
+ /* note: lux is 31 bit max at this point */
+ if (ch1lux > ch0lux) {
+ dev_dbg(&client->dev, "No Data - Return last value\n");
+ ret = chip->als_cur_info.lux = 0;
+ goto out_unlock;
+ }
+
+ /* adjust for active time scale */
+ if (chip->als_time_scale == 0)
+ lux = 0;
+ else
+ lux = (lux + (chip->als_time_scale >> 1)) /
+ chip->als_time_scale;
+
+ /* adjust for active gain scale */
+ lux >>= 13; /* tables have factor of 8192 builtin for accuracy */
+ lux = (lux * chip->taos_settings.als_gain_trim + 500) / 1000;
+ if (lux > LUX_CALC_OVER_FLOW) { /* check for overflow */
+return_max:
+ lux = LUX_CALC_OVER_FLOW;
+ }
+
+ /* Update the structure with the latest VALID lux. */
+ chip->als_cur_info.lux = lux;
+ ret = lux;
+
+out_unlock:
+ mutex_unlock(&chip->als_mutex);
+ return ret;
+}
+
+/*
+ * Obtain single reading and calculate the als_gain_trim (later used
+ * to derive actual lux).
+ * Return updated gain_trim value.
+ */
+int taos_als_calibrate(struct i2c_client *client)
+{
+ struct tsl2583_chip *chip = i2c_get_clientdata(client);
+ u8 reg_val;
+ unsigned int gain_trim_val;
+ int ret;
+ int lux_val;
+
+ ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | TSL258X_CNTRL));
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "taos_als_calibrate failed to reach the CNTRL register, ret=%d\n",
+ ret);
+ return ret;
+ }
+
+ reg_val = i2c_smbus_read_byte(client);
+ if ((reg_val & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON))
+ != (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON)) {
+ dev_err(&client->dev,
+ "taos_als_calibrate failed: device not powered on with ADC enabled\n");
+ return -ENODATA;
+ }
+
+ ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | TSL258X_CNTRL));
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "taos_als_calibrate failed to reach the STATUS register, ret=%d\n",
+ ret);
+ return ret;
+ }
+ reg_val = i2c_smbus_read_byte(client);
+
+ if ((reg_val & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
+ dev_err(&client->dev,
+ "taos_als_calibrate failed: STATUS - ADC not valid.\n");
+ return -ENODATA;
+ }
+ lux_val = taos_get_lux(client);
+ if (lux_val < 0) {
+ dev_err(&client->dev, "taos_als_calibrate failed to get lux\n");
+ return lux_val;
+ }
+ gain_trim_val = (unsigned int) (((chip->taos_settings.als_cal_target)
+ * chip->taos_settings.als_gain_trim) / lux_val);
+
+ dev_info(&client->dev, "taos_settings.als_cal_target = %d\n"
+ "taos_settings.als_gain_trim = %d\nlux_val = %d\n",
+ chip->taos_settings.als_cal_target,
+ chip->taos_settings.als_gain_trim,
+ lux_val);
+
+ if ((gain_trim_val < 250) || (gain_trim_val > 4000)) {
+ dev_err(&client->dev,
+ "taos_als_calibrate failed: trim_val of %d is out of range\n",
+ gain_trim_val);
+ return -ENODATA;
+ }
+ chip->taos_settings.als_gain_trim = (int) gain_trim_val;
+
+ return (int) gain_trim_val;
+}
+
+/*
+ * Turn the device on.
+ * Configuration must be set before calling this function.
+ */
+static int taos_chip_on(struct i2c_client *client)
+{
+ int i;
+ int ret = 0;
+ u8 *uP;
+ u8 utmp;
+ int als_count;
+ int als_time;
+ struct tsl2583_chip *chip = i2c_get_clientdata(client);
+
+ /* and make sure we're not already on */
+ if (chip->taos_chip_status == TAOS_CHIP_WORKING) {
+ /* if forcing a register update - turn off, then on */
+ dev_info(&client->dev, "device is already enabled\n");
+ return -1;
+ }
+
+ /* determine als integration regster */
+ als_count = (chip->taos_settings.als_time * 100 + 135) / 270;
+ if (als_count == 0)
+ als_count = 1; /* ensure at least one cycle */
+
+ /* convert back to time (encompasses overrides) */
+ als_time = (als_count * 27 + 5) / 10;
+ chip->taos_config[TSL258X_ALS_TIME] = 256 - als_count;
+
+ /* Set the gain based on taos_settings struct */
+ chip->taos_config[TSL258X_GAIN] = chip->taos_settings.als_gain;
+
+ /* set chip struct re scaling and saturation */
+ chip->als_saturation = als_count * 922; /* 90% of full scale */
+ chip->als_time_scale = (als_time + 25) / 50;
+
+ /* SKATE Specific power-on / adc enable sequence
+ * Power on the device 1st. */
+ utmp = TSL258X_CNTL_PWR_ON;
+ ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
+ utmp);
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_chip_on failed on CNTRL reg.\n");
+ return -1;
+ }
+
+ /* Use the following shadow copy for our delay before enabling ADC.
+ * Write all the registers. */
+ for (i = 0, uP = chip->taos_config; i < TAOS_REG_MAX; i++) {
+ ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG + i,
+ *uP++);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "taos_chip_on failed on reg %d.\n", i);
+ return -1;
+ }
+ }
+
+ mdelay(3);
+ /* NOW enable the ADC
+ * initialize the desired mode of operation */
+ utmp = TSL258X_CNTL_PWR_ON | TSL258X_CNTL_ADC_ENBL;
+ ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
+ utmp);
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_chip_on failed on 2nd CTRL reg.\n");
+ return -1;
+ }
+ chip->taos_chip_status = TAOS_CHIP_WORKING;
+ return ret;
+}
+
+/* Turn the device OFF. */
+static int taos_chip_off(struct i2c_client *client)
+{
+ struct tsl2583_chip *chip = i2c_get_clientdata(client);
+ int ret;
+ u8 utmp;
+
+ /* turn device off */
+ chip->taos_chip_status = TAOS_CHIP_SLEEP;
+ utmp = 0x00;
+ ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
+ utmp);
+ return ret;
+}
+
+/* Sysfs Interface Functions */
+static ssize_t taos_device_id(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%s\n", chip->client->name);
+}
+
+static ssize_t taos_power_state_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%d\n", chip->taos_chip_status);
+}
+
+static ssize_t taos_power_state_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value == 0)
+ taos_chip_off(chip->client);
+ else
+ taos_chip_on(chip->client);
+
+ return len;
+}
+
+static ssize_t taos_gain_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%d\n", chip->taos_settings.als_gain);
+}
+
+static ssize_t taos_gain_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value > 3) {
+ dev_err(dev, "Invalid Gain Index: Enter 0-3\n");
+ return -1;
+ } else {
+ chip->taos_settings.als_gain = value;
+ }
+ return len;
+}
+
+static ssize_t taos_gain_available_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%s\n", "0 1 2 3");
+}
+
+static ssize_t taos_als_time_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%d\n", chip->taos_settings.als_time);
+}
+
+static ssize_t taos_als_time_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if ((value < 50) || (value > 650))
+ return -EINVAL;
+
+ if (value % 50)
+ return -EINVAL;
+
+ chip->taos_settings.als_time = value;
+
+ return len;
+}
+
+static ssize_t taos_als_time_available_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%s\n", "50 100 150 200 250 300 350 400 450 500 550 600 650");
+}
+
+static ssize_t taos_als_trim_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%d\n", chip->taos_settings.als_gain_trim);
+}
+
+static ssize_t taos_als_trim_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value)
+ chip->taos_settings.als_gain_trim = value;
+
+ return len;
+}
+
+static ssize_t taos_als_cal_target_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%d\n", chip->taos_settings.als_cal_target);
+}
+
+static ssize_t taos_als_cal_target_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value)
+ chip->taos_settings.als_cal_target = value;
+
+ return len;
+}
+
+static ssize_t taos_lux_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+ int lux;
+
+ lux = taos_get_lux(chip->client);
+
+ return sprintf(buf, "%d\n", lux);
+}
+
+static ssize_t taos_do_calibrate(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value == 1)
+ taos_als_calibrate(chip->client);
+
+ return len;
+}
+
+static ssize_t taos_luxtable_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int i;
+ int offset = 0;
+
+ for (i = 0; i < ARRAY_SIZE(taos_device_lux); i++) {
+ offset += sprintf(buf + offset, "%d,%d,%d,",
+ taos_device_lux[i].ratio,
+ taos_device_lux[i].ch0,
+ taos_device_lux[i].ch1);
+ if (taos_device_lux[i].ratio == 0) {
+ /* We just printed the first "0" entry.
+ * Now get rid of the extra "," and break. */
+ offset--;
+ break;
+ }
+ }
+
+ offset += sprintf(buf + offset, "\n");
+ return offset;
+}
+
+static ssize_t taos_luxtable_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl2583_chip *chip = indio_dev->dev_data;
+ int value[ARRAY_SIZE(taos_device_lux)];
+ int n;
+
+ get_options(buf, ARRAY_SIZE(value), value);
+
+ /* We now have an array of ints starting at value[1], and
+ * enumerated by value[0].
+ * We expect each group of three ints is one table entry,
+ * and the last table entry is all 0.
+ */
+ n = value[0];
+ if ((n % 3) || n < 6 || n > (ARRAY_SIZE(taos_device_lux) - 3)) {
+ dev_info(dev, "LUX TABLE INPUT ERROR 1 Value[0]=%d\n", n);
+ return -EINVAL;
+ }
+ if ((value[(n - 2)] | value[(n - 1)] | value[n]) != 0) {
+ dev_info(dev, "LUX TABLE INPUT ERROR 2 Value[0]=%d\n", n);
+ return -EINVAL;
+ }
+
+ if (chip->taos_chip_status == TAOS_CHIP_WORKING)
+ taos_chip_off(chip->client);
+
+ /* Zero out the table */
+ memset(taos_device_lux, 0, sizeof(taos_device_lux));
+ memcpy(taos_device_lux, &value[1], (value[0] * 4));
+
+ taos_chip_on(chip->client);
+
+ return len;
+}
+
+static DEVICE_ATTR(name, S_IRUGO, taos_device_id, NULL);
+static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
+ taos_power_state_show, taos_power_state_store);
+
+static DEVICE_ATTR(illuminance0_calibscale, S_IRUGO | S_IWUSR,
+ taos_gain_show, taos_gain_store);
+static DEVICE_ATTR(illuminance0_calibscale_available, S_IRUGO,
+ taos_gain_available_show, NULL);
+
+static DEVICE_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
+ taos_als_time_show, taos_als_time_store);
+static DEVICE_ATTR(sampling_frequency_available, S_IRUGO,
+ taos_als_time_available_show, NULL);
+
+static DEVICE_ATTR(scale, S_IRUGO | S_IWUSR,
+ taos_als_trim_show, taos_als_trim_store);
+
+static DEVICE_ATTR(illuminance0_target, S_IRUGO | S_IWUSR,
+ taos_als_cal_target_show, taos_als_cal_target_store);
+
+static DEVICE_ATTR(illuminance0_input, S_IRUGO, taos_lux_show, NULL);
+static DEVICE_ATTR(calibrate, S_IWUSR, NULL, taos_do_calibrate);
+static DEVICE_ATTR(lux_table, S_IRUGO | S_IWUSR,
+ taos_luxtable_show, taos_luxtable_store);
+
+static struct attribute *sysfs_attrs_ctrl[] = {
+ &dev_attr_name.attr,
+ &dev_attr_power_state.attr,
+ &dev_attr_illuminance0_calibscale.attr,
+ &dev_attr_illuminance0_calibscale_available.attr,
+ &dev_attr_sampling_frequency.attr,
+ &dev_attr_sampling_frequency_available.attr,
+ &dev_attr_scale.attr,
+ &dev_attr_illuminance0_target.attr,
+ &dev_attr_illuminance0_input.attr,
+ &dev_attr_calibrate.attr,
+ &dev_attr_lux_table.attr,
+ NULL
+};
+
+static struct attribute_group tsl2583_attribute_group = {
+ .attrs = sysfs_attrs_ctrl,
+};
+
+/* Use the default register values to identify the Taos device */
+static int taos_skate_device(unsigned char *bufp)
+{
+ if (((bufp[TSL258X_CHIPID] & 0xf0) == 0x90))
+ /* tsl2583 */
+ return 1;
+ /* else unknown */
+ return 0;
+}
+
+/*
+ * Client probe function - When a valid device is found, the driver's device
+ * data structure is updated, and initialization completes successfully.
+ */
+static int __devinit taos_probe(struct i2c_client *clientp,
+ const struct i2c_device_id *idp)
+{
+ int i, ret = 0;
+ unsigned char buf[MAX_DEVICE_REGS];
+ static struct tsl2583_chip *chip;
+
+ if (!i2c_check_functionality(clientp->adapter,
+ I2C_FUNC_SMBUS_BYTE_DATA)) {
+ dev_err(&clientp->dev,
+ "taos_probe() - i2c smbus byte data "
+ "functions unsupported\n");
+ return -EOPNOTSUPP;
+ }
+ if (!i2c_check_functionality(clientp->adapter,
+ I2C_FUNC_SMBUS_WORD_DATA)) {
+ dev_warn(&clientp->dev,
+ "taos_probe() - i2c smbus word data "
+ "functions unsupported\n");
+ }
+ if (!i2c_check_functionality(clientp->adapter,
+ I2C_FUNC_SMBUS_BLOCK_DATA)) {
+ dev_err(&clientp->dev,
+ "taos_probe() - i2c smbus block data "
+ "functions unsupported\n");
+ }
+
+ chip = kzalloc(sizeof(struct tsl2583_chip), GFP_KERNEL);
+ if (!chip) {
+ dev_err(&clientp->dev, "taos_device_drvr: kmalloc for "
+ "struct tsl2583_chip failed in taos_probe()\n");
+ return -ENOMEM;
+ }
+
+ chip->client = clientp;
+ i2c_set_clientdata(clientp, chip);
+
+ mutex_init(&chip->als_mutex);
+ chip->taos_chip_status = TAOS_CHIP_UNKNOWN; /* uninitialized to start */
+ memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
+
+ for (i = 0; i < MAX_DEVICE_REGS; i++) {
+ ret = i2c_smbus_write_byte(clientp,
+ (TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
+ if (ret < 0) {
+ dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd "
+ "reg failed in taos_probe(), err = %d\n", ret);
+ goto fail1;
+ }
+ buf[i] = i2c_smbus_read_byte(clientp);
+ }
+ if (!taos_skate_device(buf)) {
+ dev_info(&clientp->dev, "i2c device found but does not match "
+ "expected id in taos_probe()\n");
+ goto fail1;
+ } else {
+ dev_info(&clientp->dev, "i2c device match in probe\n");
+ }
+ ret = i2c_smbus_write_byte(clientp, (TSL258X_CMD_REG | TSL258X_CNTRL));
+ if (ret < 0) {
+ dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd reg "
+ "failed in taos_probe(), err = %d\n", ret);
+ goto fail1;
+ }
+ chip->valid = 0;
+
+ chip->iio_dev = iio_allocate_device();
+ if (!chip->iio_dev) {
+ ret = -ENOMEM;
+ dev_err(&clientp->dev, "iio allocation failed\n");
+ goto fail1;
+ }
+
+ chip->iio_dev->attrs = &tsl2583_attribute_group;
+ chip->iio_dev->dev.parent = &clientp->dev;
+ chip->iio_dev->dev_data = (void *)(chip);
+ chip->iio_dev->driver_module = THIS_MODULE;
+ chip->iio_dev->modes = INDIO_DIRECT_MODE;
+ ret = iio_device_register(chip->iio_dev);
+ if (ret) {
+ dev_err(&clientp->dev, "iio registration failed\n");
+ goto fail1;
+ }
+
+ /* Load up the V2 defaults (these are hard coded defaults for now) */
+ taos_defaults(chip);
+
+ /* Make sure the chip is on */
+ taos_chip_on(clientp);
+
+ dev_info(&clientp->dev, "Light sensor found.\n");
+ return 0;
+
+fail1:
+ kfree(chip);
+
+ return ret;
+}
+
+static int __devexit taos_remove(struct i2c_client *client)
+{
+ struct tsl2583_chip *chip = i2c_get_clientdata(client);
+
+ iio_device_unregister(chip->iio_dev);
+
+ kfree(chip);
+ return 0;
+}
+
+static struct i2c_device_id taos_idtable[] = {
+ { "tsl2580", 0 },
+ { "tsl2581", 1 },
+ { "tsl2583", 2 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, taos_idtable);
+
+/* Driver definition */
+static struct i2c_driver taos_driver = {
+ .driver = {
+ .name = "tsl2583",
+ },
+ .id_table = taos_idtable,
+ .probe = taos_probe,
+ .remove = __devexit_p(taos_remove),
+};
+
+static int __init taos_init(void)
+{
+ return i2c_add_driver(&taos_driver);
+}
+
+static void __exit taos_exit(void)
+{
+ i2c_del_driver(&taos_driver);
+}
+
+module_init(taos_init);
+module_exit(taos_exit);
+
+MODULE_AUTHOR("J. August Brenner<jbrenner@taosinc.com>");
+MODULE_DESCRIPTION("TAOS tsl2583 ambient light sensor driver");
+MODULE_LICENSE("GPL");
+
--
1.7.0.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
2011-03-14 19:45 [PATCH 2.6.38-rc7]TAOS 258x: Device Driver Jon Brenner
@ 2011-03-21 19:07 ` Jonathan Cameron
2011-03-23 1:07 ` Jon Brenner
0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2011-03-21 19:07 UTC (permalink / raw)
To: Jon Brenner; +Cc: linux-iio, Linux Kernel
On 03/14/11 19:45, Jon Brenner wrote:
> From: Jon Brenner <jbrenner@taosinc.com>
>
> Support for TAOS tsl2580/01/03 ALS devices.
> Uses sysfs/iio methods.
Hi Jon,
As I pm'd you the other day, please remember to make version of patch clear
in title, and include details of what has changed since previous version.
The 'scale' attribute is a problem. I've suggested one possible solution
but I'm open to others. It ought to be handled in a generalizable way
but isn't currently.
Please document the _target and calibrate attributes in part specific
documentation file.
Otherwise, various small points inline.
Jonathan
>
> Signed-off-by: Jon August Brenner <jbrenner@taosinc.com>
>
> ---
> drivers/staging/iio/Documentation/sysfs-bus-iio | 6 +
> .../staging/iio/Documentation/sysfs-bus-iio-light | 7 +
> drivers/staging/iio/light/Kconfig | 9 +
> drivers/staging/iio/light/Makefile | 1 +
> drivers/staging/iio/light/tsl2583.c | 943 ++++++++++++++++++++
> 5 files changed, 966 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio b/drivers/staging/iio/Documentation/sysfs-bus-iio
> index 2dde97d..6a86ad2 100644
> --- a/drivers/staging/iio/Documentation/sysfs-bus-iio
> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio
> @@ -6,6 +6,12 @@ Description:
> Corresponds to a grouping of sensor channels. X is the IIO
> index of the device.
>
> +What: /sys/bus/iio/devices/device[n]/power_state
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the device power state.
Where the options are?
There is some on going debate about how best to handle explicit userspace
power control. For now I'll let this go in, but we will need to clean
it up in the long run. Basically this is an issue that effects several
subsystems and their ought to be one coherent solution.
> +
> What: /sys/bus/iio/devices/triggerX
> KernelVersion: 2.6.35
> Contact: linux-iio@vger.kernel.org
> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> index 5d84856..b59cdb4 100644
> --- a/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> @@ -62,3 +62,10 @@ Description:
> sensing mode. This value should be the output from a reading
> and if expressed in SI units, should include _input. If this
> value is not in SI units, then it should include _raw.
> +
> +What: /sys/bus/iio/devices/device[n]/lux_table
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the table of coefficients
> + used in calculating illuminance in lux.
This one probably wants to be in a separate file as we only have one user
so far and it's not as though a device agnostic userspace program is going
to know what to do with it. Put it in syfsf-bus-iio-light-tsl2583 please.
> diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
> index 36d8bbe..a295e82 100644
> --- a/drivers/staging/iio/light/Kconfig
> +++ b/drivers/staging/iio/light/Kconfig
> @@ -13,6 +13,15 @@ config SENSORS_TSL2563
> This driver can also be built as a module. If so, the module
> will be called tsl2563.
>
ouch, how the heck did that SENSORS prefix creep into this file. That's
the convention for hwmon, not IIO. Guess I wasn't keeping a close
eye on this. Please drop the prefix. We may well introduce an IIO one
shortly as part of the move out of staging, but for now no prefix is the norm.
> +config SENSORS_TSL2583
> + tristate "TAOS TSL2580, TSL2581, and TSL2583 light-to-digital converters"
> + depends on I2C
> + help
> + Y = in kernel.
> + M = as module.
> + Provides support for the TAOS tsl2580, tsl2581, and tsl2583 devices.
> + Access ALS data via iio, sysfs.
> +
> config SENSORS_ISL29018
> tristate "ISL 29018 light and proximity sensor"
> depends on I2C
> diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile
> index 9142c0e..9d13b8d 100644
> --- a/drivers/staging/iio/light/Makefile
> +++ b/drivers/staging/iio/light/Makefile
> @@ -3,4 +3,5 @@
> #
>
> obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
> +obj-$(CONFIG_SENSORS_TSL2583) += tsl2583.o
> obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> new file mode 100644
> index 0000000..96e4487
> --- /dev/null
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -0,0 +1,943 @@
> +/*
> + * Device driver for monitoring ambient light intensity (lux)
> + * within the TAOS tsl258x family of devices (tsl2580, tsl2581).
> + *
> + * Copyright (c) 2011, TAOS Corporation.
> + *
> + * 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.
> + *
> + * 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, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/i2c.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/string.h>
> +#include <linux/mutex.h>
> +#include "../iio.h"
> +
> +
Please prefix this with TSL258X to avoid possible namespace
clashes in future.
> +#define MAX_DEVICE_REGS 32
> +
> +/* Triton register offsets */
> +#define TAOS_REG_MAX 8
> +
> +/* Device Registers and Masks */
> +#define TSL258X_CNTRL 0x00
> +#define TSL258X_ALS_TIME 0X01
> +#define TSL258X_INTERRUPT 0x02
> +#define TSL258X_GAIN 0x07
> +#define TSL258X_REVID 0x11
> +#define TSL258X_CHIPID 0x12
> +#define TSL258X_ALS_CHAN0LO 0x14
> +#define TSL258X_ALS_CHAN0HI 0x15
> +#define TSL258X_ALS_CHAN1LO 0x16
> +#define TSL258X_ALS_CHAN1HI 0x17
> +#define TSL258X_TMR_LO 0x18
> +#define TSL258X_TMR_HI 0x19
> +
> +/* tsl2583 cmd reg masks */
> +#define TSL258X_CMD_REG 0x80
> +#define TSL258X_CMD_SPL_FN 0x60
> +#define TSL258X_CMD_ALS_INT_CLR 0X01
> +
> +/* tsl2583 cntrl reg masks */
> +#define TSL258X_CNTL_ADC_ENBL 0x02
> +#define TSL258X_CNTL_PWR_ON 0x01
> +
> +/* tsl2583 status reg masks */
> +#define TSL258X_STA_ADC_VALID 0x01
> +#define TSL258X_STA_ADC_INTR 0x10
> +
> +/* Lux calculation constants */
Again please prefix.
> +#define LUX_CALC_OVER_FLOW 65535
> +
Ideally part names in these. You might have another TAOS driver where
these are defined differently which will make life confusing!
> +enum {
> + TAOS_CHIP_UNKNOWN = 0, TAOS_CHIP_WORKING = 1, TAOS_CHIP_SLEEP = 2
> +} TAOS_CHIP_WORKING_STATUS;
> +
> +/* Per-device data */
> +struct taos_als_info {
> + u16 als_ch0;
> + u16 als_ch1;
> + u16 lux;
> +};
> +
> +struct taos_settings {
> + int als_time;
> + int als_gain;
> + int als_gain_trim;
> + int als_cal_target;
> +};
> +
> +struct tsl2583_chip {
> + struct mutex als_mutex;
> + struct i2c_client *client;
> + struct iio_dev *iio_dev;
unused so remove
> + struct delayed_work update_lux;
unused.
> + unsigned int addr;
unused.
> + char taos_id;
I think this is set to 0 then never checked anywhere? Hence remove
> + char valid;
never used
> + unsigned long last_updated;
> + struct taos_als_info als_cur_info;
> + struct taos_settings taos_settings;
> + int als_time_scale;
> + int als_saturation;
> + int taos_chip_status;
> + u8 taos_config[8];
> +};
> +
> +static int taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val,
> + unsigned int len);
> +static int taos_i2c_write_command(struct i2c_client *client, u8 reg);
Don't need these definitions.
> +
> +/*
> + * Initial values for device - this values can/will be changed by driver.
> + * and applications as needed.
> + * These values are dynamic.
> + */
> +static const u8 taos_config[8] = {
> + 0x00, 0xee, 0x00, 0x03, 0x00, 0xFF, 0xFF, 0x00
> +}; /* cntrl atime intC Athl0 Athl1 Athh0 Athh1 gain */
> +
> +struct taos_lux {
> + unsigned int ratio;
> + unsigned int ch0;
> + unsigned int ch1;
> +};
> +
> +/* This structure is intentionally large to accommodate updates via sysfs. */
> +/* Sized to 11 = max 10 segments + 1 termination segment */
> +/* Assumption is is one and only one type of glass used */
> +struct taos_lux taos_device_lux[11] = {
> + { 9830, 8520, 15729 },
> + { 12452, 10807, 23344 },
> + { 14746, 6383, 11705 },
> + { 17695, 4063, 6554 },
> +};
> +
I don't think this is used?
> +struct taos_lux taos_lux;
> +
> +struct gainadj {
> + s16 ch0;
> + s16 ch1;
> +};
> +
> +/* Index = (0 - 3) Used to validate the gain selection index */
> +static const struct gainadj gainadj[] = {
> + { 1, 1 },
> + { 8, 8 },
> + { 16, 16 },
> + { 107, 115 }
> +};
> +
> +/*
> + * Provides initial operational parameter defaults.
> + * These defaults may be changed through the device's sysfs files.
> + */
How about pasing in taos_settings and making all of this func a bit more
readable?
> +static void taos_defaults(struct tsl2583_chip *chip)
> +{
> + /* Operational parameters */
> + chip->taos_settings.als_time = 450;
> + /* must be a multiple of 50mS */
> + chip->taos_settings.als_gain = 2;
> + /* this is actually an index into the gain table */
> + /* assume clear glass as default */
> + chip->taos_settings.als_gain_trim = 1000;
> + /* default gain trim to account for aperture effects */
> + chip->taos_settings.als_cal_target = 130;
> + /* Known external ALS reading used for calibration */
> +}
> +
> +/*
> + * Read a number of bytes starting at register (reg) location.
> + * Return 0, or i2c_smbus_write_byte ERROR code.
> + */
> +static int
> +taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned int len)
> +{
> + int ret;
> + int i;
> +
> + for (i = 0; i < len; i++) {
> + /* select register to write */
> + ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | reg));
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_i2c_read failed to write"
> + " register %x\n", reg);
> + return ret;
> + }
> + /* read the data */
> + *val = i2c_smbus_read_byte(client);
> + val++;
> + reg++;
> + }
> + return 0;
> +}
> +
> +/*
> + * This function is used to send a command to device command/control register
> + * All bytes sent using this command have their MSBit set - it's a command!
> + * Return 0, or i2c_smbus_write_byte error code.
> + */
> +static int taos_i2c_write_command(struct i2c_client *client, u8 reg)
> +{
> + int ret;
> +
> + /* write the data */
Why update reg? reg | TSL258X_CMD_REG.
Also this is only used once in the code. I'd just roll this line in
directly there and get rid of this wrapper function.
> + ret = i2c_smbus_write_byte(client, (reg |= TSL258X_CMD_REG));
> + if (ret < 0) {
The error is pretty well reported by the one caller anyway
> + dev_err(&client->dev, "FAILED: i2c_smbus_write_byte\n");
> + return ret;
> + }
Could just return ret as you check this for <0 anyway.
> + return 0;
> +}
> +
> +/*
> + * Reads and calculates current lux value.
> + * The raw ch0 and ch1 values of the ambient light sensed in the last
> + * integration cycle are read from the device.
> + * Time scale factor array values are adjusted based on the integration time.
> + * The raw values are multiplied by a scale factor, and device gain is obtained
> + * using gain index. Limit checks are done next, then the ratio of a multiple
> + * of ch1 value, to the ch0 value, is calculated. The array taos_device_lux[]
> + * declared above is then scanned to find the first ratio value that is just
> + * above the ratio we just calculated. The ch0 and ch1 multiplier constants in
> + * the array are then used along with the time scale factor array values, to
> + * calculate the lux.
> + */
> +static int taos_get_lux(struct i2c_client *client)
> +{
> + u32 ch0, ch1; /* separated ch0/ch1 data from device */
> + u32 lux; /* raw lux calculated from device data */
> + u32 ratio;
> + u8 buf[5];
> + struct taos_lux *p;
> + struct tsl2583_chip *chip = i2c_get_clientdata(client);
> + int i, ret;
> + u32 ch0lux = 0;
> + u32 ch1lux = 0;
> +
> + if (mutex_trylock(&chip->als_mutex) == 0) {
> + dev_info(&client->dev, "taos_get_lux device is busy\n");
> + return chip->als_cur_info.lux; /* busy, so return LAST VALUE */
> + }
> +
> + if (chip->taos_chip_status != TAOS_CHIP_WORKING) {
> + /* device is not enabled */
> + dev_err(&client->dev, "taos_get_lux device is not enabled\n");
> + ret = -EBUSY ;
> + goto out_unlock;
> + }
> +
> + ret = taos_i2c_read(client, (TSL258X_CMD_REG), &buf[0], 1);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_get_lux failed to read CMD_REG\n");
> + goto out_unlock;
> + }
> + /* is data new & valid */
> + if (!(buf[0] & TSL258X_STA_ADC_INTR)) {
> + dev_err(&client->dev, "taos_get_lux data not valid\n");
> + ret = chip->als_cur_info.lux; /* return LAST VALUE */
> + goto out_unlock;
> + }
> +
> + for (i = 0; i < 4; i++) {
> + int reg = TSL258X_CMD_REG | (TSL258X_ALS_CHAN0LO + i);
> + ret = taos_i2c_read(client, reg, &buf[i], 1);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_get_lux failed to read"
> + " register %x\n", reg);
> + goto out_unlock;
> + }
> + }
> +
> + /* clear status, really interrupt status (interrupts are off), but
> + * we use the bit anyway */
> + ret = taos_i2c_write_command(client,
> + TSL258X_CMD_REG | TSL258X_CMD_SPL_FN | TSL258X_CMD_ALS_INT_CLR);
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "taos_i2c_write_command failed in taos_get_lux, err = %d\n",
> + ret);
> + goto out_unlock; /* have no data, so return failure */
> + }
> +
> + /* extract ALS/lux data */
These appear to be endianness convesions. ch0 = le16tocpu(&buf[0])? That way they compile
out to a copy if we happen to be on a little endian machine. Also these seem to be explicitly
16 bits, so u16 makes more sense.
> + ch0 = (buf[1] << 8) | buf[0];
> + ch1 = (buf[3] << 8) | buf[2];
> +
> + chip->als_cur_info.als_ch0 = ch0;
> + chip->als_cur_info.als_ch1 = ch1;
> +
> + if ((ch0 >= chip->als_saturation) || (ch1 >= chip->als_saturation))
would be easier to read if you set ret to lux max here an avoid jumping into
the if statement below.
> + goto return_max;
> +
> + if (ch0 == 0) {
> + /* have no data, so return LAST VALUE */
> + ret = chip->als_cur_info.lux = 0;
> + goto out_unlock;
> + }
> + /* calculate ratio */
> + ratio = (ch1 << 15) / ch0;
> + /* convert to unscaled lux using the pointer to the table */
&taos_device_lux[0] perhaps?
> + for (p = (struct taos_lux *) taos_device_lux;
> + p->ratio != 0 && p->ratio < ratio; p++)
> + ;
> +
> + if (p->ratio == 0) {
> + lux = 0;
> + } else {
> + ch0lux = ((ch0 * p->ch0) +
> + (gainadj[chip->taos_settings.als_gain].ch0 >> 1))
> + / gainadj[chip->taos_settings.als_gain].ch0;
> + ch1lux = ((ch1 * p->ch1) +
> + (gainadj[chip->taos_settings.als_gain].ch1 >> 1))
> + / gainadj[chip->taos_settings.als_gain].ch1;
> + lux = ch0lux - ch1lux;
> + }
> +
> + /* note: lux is 31 bit max at this point */
> + if (ch1lux > ch0lux) {
> + dev_dbg(&client->dev, "No Data - Return last value\n");
> + ret = chip->als_cur_info.lux = 0;
> + goto out_unlock;
> + }
> +
> + /* adjust for active time scale */
> + if (chip->als_time_scale == 0)
> + lux = 0;
> + else
> + lux = (lux + (chip->als_time_scale >> 1)) /
> + chip->als_time_scale;
> +
> + /* adjust for active gain scale */
> + lux >>= 13; /* tables have factor of 8192 builtin for accuracy */
> + lux = (lux * chip->taos_settings.als_gain_trim + 500) / 1000;
> + if (lux > LUX_CALC_OVER_FLOW) { /* check for overflow */
> +return_max:
> + lux = LUX_CALC_OVER_FLOW;
> + }
> +
> + /* Update the structure with the latest VALID lux. */
> + chip->als_cur_info.lux = lux;
> + ret = lux;
> +
> +out_unlock:
> + mutex_unlock(&chip->als_mutex);
> + return ret;
> +}
> +
> +/*
> + * Obtain single reading and calculate the als_gain_trim (later used
> + * to derive actual lux).
> + * Return updated gain_trim value.
> + */
> +int taos_als_calibrate(struct i2c_client *client)
> +{
> + struct tsl2583_chip *chip = i2c_get_clientdata(client);
> + u8 reg_val;
> + unsigned int gain_trim_val;
> + int ret;
> + int lux_val;
> +
> + ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | TSL258X_CNTRL));
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "taos_als_calibrate failed to reach the CNTRL register, ret=%d\n",
> + ret);
> + return ret;
> + }
> +
> + reg_val = i2c_smbus_read_byte(client);
> + if ((reg_val & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON))
> + != (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON)) {
> + dev_err(&client->dev,
> + "taos_als_calibrate failed: device not powered on with ADC enabled\n");
> + return -ENODATA;
> + }
> +
> + ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | TSL258X_CNTRL));
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "taos_als_calibrate failed to reach the STATUS register, ret=%d\n",
> + ret);
> + return ret;
> + }
> + reg_val = i2c_smbus_read_byte(client);
> +
what if it's an error? Then we want to return that rather than -ENODATA.
> + if ((reg_val & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
> + dev_err(&client->dev,
> + "taos_als_calibrate failed: STATUS - ADC not valid.\n");
> + return -ENODATA;
> + }
> + lux_val = taos_get_lux(client);
> + if (lux_val < 0) {
> + dev_err(&client->dev, "taos_als_calibrate failed to get lux\n");
> + return lux_val;
> + }
> + gain_trim_val = (unsigned int) (((chip->taos_settings.als_cal_target)
> + * chip->taos_settings.als_gain_trim) / lux_val);
> +
If it is worth printing this to the log, we don't care about where it
is stored just what it is. Otherwise, shouldn't be here.
> + dev_info(&client->dev, "taos_settings.als_cal_target = %d\n"
> + "taos_settings.als_gain_trim = %d\nlux_val = %d\n",
> + chip->taos_settings.als_cal_target,
> + chip->taos_settings.als_gain_trim,
> + lux_val);
> +
> + if ((gain_trim_val < 250) || (gain_trim_val > 4000)) {
> + dev_err(&client->dev,
> + "taos_als_calibrate failed: trim_val of %d is out of range\n",
> + gain_trim_val);
> + return -ENODATA;
> + }
> + chip->taos_settings.als_gain_trim = (int) gain_trim_val;
> +
> + return (int) gain_trim_val;
> +}
> +
> +/*
> + * Turn the device on.
> + * Configuration must be set before calling this function.
> + */
> +static int taos_chip_on(struct i2c_client *client)
> +{
> + int i;
> + int ret = 0;
> + u8 *uP;
> + u8 utmp;
> + int als_count;
> + int als_time;
> + struct tsl2583_chip *chip = i2c_get_clientdata(client);
> +
> + /* and make sure we're not already on */
> + if (chip->taos_chip_status == TAOS_CHIP_WORKING) {
> + /* if forcing a register update - turn off, then on */
> + dev_info(&client->dev, "device is already enabled\n");
-EINVAL perhaps?
> + return -1;
> + }
> +
> + /* determine als integration regster */
> + als_count = (chip->taos_settings.als_time * 100 + 135) / 270;
> + if (als_count == 0)
> + als_count = 1; /* ensure at least one cycle */
> +
> + /* convert back to time (encompasses overrides) */
> + als_time = (als_count * 27 + 5) / 10;
> + chip->taos_config[TSL258X_ALS_TIME] = 256 - als_count;
> +
> + /* Set the gain based on taos_settings struct */
> + chip->taos_config[TSL258X_GAIN] = chip->taos_settings.als_gain;
> +
> + /* set chip struct re scaling and saturation */
> + chip->als_saturation = als_count * 922; /* 90% of full scale */
> + chip->als_time_scale = (als_time + 25) / 50;
> +
We still don't know what SKATE is... Any publically available terms possible?
> + /* SKATE Specific power-on / adc enable sequence
> + * Power on the device 1st. */
> + utmp = TSL258X_CNTL_PWR_ON;
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
> + utmp);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_chip_on failed on CNTRL reg.\n");
> + return -1;
> + }
> +
> + /* Use the following shadow copy for our delay before enabling ADC.
> + * Write all the registers. */
> + for (i = 0, uP = chip->taos_config; i < TAOS_REG_MAX; i++) {
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG + i,
> + *uP++);
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "taos_chip_on failed on reg %d.\n", i);
> + return -1;
> + }
> + }
> +
If not time critical lest make that a sleep.
> + mdelay(3);
> + /* NOW enable the ADC
> + * initialize the desired mode of operation */
> + utmp = TSL258X_CNTL_PWR_ON | TSL258X_CNTL_ADC_ENBL;
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
> + utmp);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_chip_on failed on 2nd CTRL reg.\n");
> + return -1;
> + }
> + chip->taos_chip_status = TAOS_CHIP_WORKING;
conventionally blank line here.
> + return ret;
> +}
> +
> +/* Turn the device OFF. */
> +static int taos_chip_off(struct i2c_client *client)
> +{
> + struct tsl2583_chip *chip = i2c_get_clientdata(client);
> + int ret;
> + u8 utmp;
> +
> + /* turn device off */
Kind of obvious comment in this function ;) Please remove.
> + chip->taos_chip_status = TAOS_CHIP_SLEEP;
> + utmp = 0x00;
Why not just put this value into the function call and get rid of utmp as a variable?
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
> + utmp);
> + return ret;
> +}
> +
> +/* Sysfs Interface Functions */
> +static ssize_t taos_device_id(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%s\n", chip->client->name);
> +}
> +
> +static ssize_t taos_power_state_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_chip_status);
> +}
> +
> +static ssize_t taos_power_state_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
(wishful thought of the day)
One day someone will write a nice standard 'boolean' test function
that everyone will agree on... This works for me though ;)
> + if (value == 0)
> + taos_chip_off(chip->client);
> + else
> + taos_chip_on(chip->client);
> +
> + return len;
> +}
> +
> +static ssize_t taos_gain_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_gain);
> +}
> +
> +static ssize_t taos_gain_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
I still wonder if there isn't a way of avoiding this index issue.
It's horrible and really feels like something we ought to able
to handle reasonably well...
> + if (value > 3) {
> + dev_err(dev, "Invalid Gain Index: Enter 0-3\n");
> + return -1;
> + } else {
> + chip->taos_settings.als_gain = value;
> + }
> + return len;
> +}
> +
> +static ssize_t taos_gain_available_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%s\n", "0 1 2 3");
> +}
> +
Just to confirm, is this a frequency in Hz? If it is
can we renaming it to taos_als_frequency_show to
avoid confusing me?
> +static ssize_t taos_als_time_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_time);
> +}
> +
> +static ssize_t taos_als_time_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if ((value < 50) || (value > 650))
> + return -EINVAL;
> +
> + if (value % 50)
> + return -EINVAL;
> +
> + chip->taos_settings.als_time = value;
> +
> + return len;
> +}
> +
> +static ssize_t taos_als_time_available_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%s\n", "50 100 150 200 250 300 350 400 450 500 550 600 650");
> +}
> +
> +static ssize_t taos_als_trim_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_gain_trim);
> +}
> +
> +static ssize_t taos_als_trim_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value)
> + chip->taos_settings.als_gain_trim = value;
> +
> + return len;
> +}
> +
> +static ssize_t taos_als_cal_target_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_cal_target);
> +}
> +
> +static ssize_t taos_als_cal_target_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value)
> + chip->taos_settings.als_cal_target = value;
> +
> + return len;
> +}
> +
> +static ssize_t taos_lux_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> + int lux;
> +
> + lux = taos_get_lux(chip->client);
> +
> + return sprintf(buf, "%d\n", lux);
> +}
> +
> +static ssize_t taos_do_calibrate(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value == 1)
> + taos_als_calibrate(chip->client);
> +
> + return len;
> +}
> +
> +static ssize_t taos_luxtable_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int i;
> + int offset = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(taos_device_lux); i++) {
> + offset += sprintf(buf + offset, "%d,%d,%d,",
> + taos_device_lux[i].ratio,
> + taos_device_lux[i].ch0,
> + taos_device_lux[i].ch1);
> + if (taos_device_lux[i].ratio == 0) {
> + /* We just printed the first "0" entry.
> + * Now get rid of the extra "," and break. */
> + offset--;
> + break;
> + }
> + }
> +
> + offset += sprintf(buf + offset, "\n");
> + return offset;
> +}
> +
> +static ssize_t taos_luxtable_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> + int value[ARRAY_SIZE(taos_device_lux)];
> + int n;
> +
> + get_options(buf, ARRAY_SIZE(value), value);
> +
> + /* We now have an array of ints starting at value[1], and
> + * enumerated by value[0].
> + * We expect each group of three ints is one table entry,
> + * and the last table entry is all 0.
> + */
> + n = value[0];
> + if ((n % 3) || n < 6 || n > (ARRAY_SIZE(taos_device_lux) - 3)) {
> + dev_info(dev, "LUX TABLE INPUT ERROR 1 Value[0]=%d\n", n);
> + return -EINVAL;
> + }
> + if ((value[(n - 2)] | value[(n - 1)] | value[n]) != 0) {
> + dev_info(dev, "LUX TABLE INPUT ERROR 2 Value[0]=%d\n", n);
> + return -EINVAL;
> + }
> +
> + if (chip->taos_chip_status == TAOS_CHIP_WORKING)
> + taos_chip_off(chip->client);
> +
> + /* Zero out the table */
> + memset(taos_device_lux, 0, sizeof(taos_device_lux));
> + memcpy(taos_device_lux, &value[1], (value[0] * 4));
> +
> + taos_chip_on(chip->client);
> +
> + return len;
> +}
> +
> +static DEVICE_ATTR(name, S_IRUGO, taos_device_id, NULL);
> +static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
> + taos_power_state_show, taos_power_state_store);
> +
> +static DEVICE_ATTR(illuminance0_calibscale, S_IRUGO | S_IWUSR,
> + taos_gain_show, taos_gain_store);
> +static DEVICE_ATTR(illuminance0_calibscale_available, S_IRUGO,
> + taos_gain_available_show, NULL);
> +
> +static DEVICE_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
> + taos_als_time_show, taos_als_time_store);
> +static DEVICE_ATTR(sampling_frequency_available, S_IRUGO,
> + taos_als_time_available_show, NULL);
> +
scale as a stand alone attribute isn't defined. Please document this.
Looking at what you do with it, it's another factor effecting the overal
gain on the reading that reaches userspace. Ideally you'd roll this
into your calibscale parameter, but I can see that would get complex
to manage. Will have a think about this. In devices with a simple conversion
function (adc's etc) we handle this by leaving this software value
be applied by userspace (and output it as in_scale). The issue here that
as far as userspace is concerned both of your scales have been applied before
it sees the data and at different more or less random looking points in
the calculation.
Actually, looking at the calculation you could output illuminance0_raw
and let userspace apply a multiplier based on your trim value and offset
+0.5? If you want to hold trim in driver then just implement
read and write to
illuminance0_scale
and have read only
illuminance0_offset (rather tediously for this device, offset is applied
before scale, so you'll need to divide 0.5 by whatever your trim is).
We'll have to do pin down how to do this before moving out of staging
so best to get it right now.
> +static DEVICE_ATTR(scale, S_IRUGO | S_IWUSR,
> + taos_als_trim_show, taos_als_trim_store);
> +
> +static DEVICE_ATTR(illuminance0_target, S_IRUGO | S_IWUSR,
> + taos_als_cal_target_show, taos_als_cal_target_store);
This needs documenting. Also if it is calculated units (e.g. lux)
needs to be illuminance0_input_target to make that explicit.
> +
> +static DEVICE_ATTR(illuminance0_input, S_IRUGO, taos_lux_show, NULL);
> +static DEVICE_ATTR(calibrate, S_IWUSR, NULL, taos_do_calibrate);
calibrate needs documentation.
> +static DEVICE_ATTR(lux_table, S_IRUGO | S_IWUSR,
> + taos_luxtable_show, taos_luxtable_store);
> +
> +static struct attribute *sysfs_attrs_ctrl[] = {
> + &dev_attr_name.attr,
> + &dev_attr_power_state.attr,
> + &dev_attr_illuminance0_calibscale.attr,
> + &dev_attr_illuminance0_calibscale_available.attr,
> + &dev_attr_sampling_frequency.attr,
> + &dev_attr_sampling_frequency_available.attr,
> + &dev_attr_scale.attr,
> + &dev_attr_illuminance0_target.attr,
> + &dev_attr_illuminance0_input.attr,
> + &dev_attr_calibrate.attr,
> + &dev_attr_lux_table.attr,
> + NULL
> +};
> +
> +static struct attribute_group tsl2583_attribute_group = {
> + .attrs = sysfs_attrs_ctrl,
> +};
> +
> +/* Use the default register values to identify the Taos device */
> +static int taos_skate_device(unsigned char *bufp)
> +{
return ((bufp[TSL258X_CHIPID] & 0xf0) == 0x90);
> + if (((bufp[TSL258X_CHIPID] & 0xf0) == 0x90))
> + /* tsl2583 */
> + return 1;
> + /* else unknown */
> + return 0;
> +}
> +
> +/*
> + * Client probe function - When a valid device is found, the driver's device
> + * data structure is updated, and initialization completes successfully.
> + */
> +static int __devinit taos_probe(struct i2c_client *clientp,
> + const struct i2c_device_id *idp)
> +{
> + int i, ret = 0;
> + unsigned char buf[MAX_DEVICE_REGS];
> + static struct tsl2583_chip *chip;
> +
> + if (!i2c_check_functionality(clientp->adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA)) {
> + dev_err(&clientp->dev,
> + "taos_probe() - i2c smbus byte data "
> + "functions unsupported\n");
> + return -EOPNOTSUPP;
> + }
> + if (!i2c_check_functionality(clientp->adapter,
> + I2C_FUNC_SMBUS_WORD_DATA)) {
> + dev_warn(&clientp->dev,
> + "taos_probe() - i2c smbus word data "
> + "functions unsupported\n");
Why do you care? I'm not seeing them being used anyway.
> + }
> + if (!i2c_check_functionality(clientp->adapter,
> + I2C_FUNC_SMBUS_BLOCK_DATA)) {
> + dev_err(&clientp->dev,
> + "taos_probe() - i2c smbus block data "
> + "functions unsupported\n");
Nor these.
> + }
> +
> + chip = kzalloc(sizeof(struct tsl2583_chip), GFP_KERNEL);
> + if (!chip) {
Malloc failure would be pretty unusual and knowing what it was probably
won't be helpful, so no need to have this err printing here.
> + dev_err(&clientp->dev, "taos_device_drvr: kmalloc for "
> + "struct tsl2583_chip failed in taos_probe()\n");
> + return -ENOMEM;
> + }
> +
> + chip->client = clientp;
> + i2c_set_clientdata(clientp, chip);
> +
> + mutex_init(&chip->als_mutex);
> + chip->taos_chip_status = TAOS_CHIP_UNKNOWN; /* uninitialized to start */
> + memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
> +
> + for (i = 0; i < MAX_DEVICE_REGS; i++) {
> + ret = i2c_smbus_write_byte(clientp,
> + (TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
> + if (ret < 0) {
> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd "
> + "reg failed in taos_probe(), err = %d\n", ret);
> + goto fail1;
> + }
> + buf[i] = i2c_smbus_read_byte(clientp);
error handling for this read?
> + }
> + if (!taos_skate_device(buf)) {
> + dev_info(&clientp->dev, "i2c device found but does not match "
> + "expected id in taos_probe()\n");
> + goto fail1;
> + } else {
The sort of debug info you want to get rid of for production drivers. Doesn't
tell anyone anything helpful.
> + dev_info(&clientp->dev, "i2c device match in probe\n");
> + }
> + ret = i2c_smbus_write_byte(clientp, (TSL258X_CMD_REG | TSL258X_CNTRL));
> + if (ret < 0) {
> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd reg "
> + "failed in taos_probe(), err = %d\n", ret);
> + goto fail1;
> + }
> + chip->valid = 0;
> +
> + chip->iio_dev = iio_allocate_device();
> + if (!chip->iio_dev) {
> + ret = -ENOMEM;
> + dev_err(&clientp->dev, "iio allocation failed\n");
> + goto fail1;
> + }
> +
> + chip->iio_dev->attrs = &tsl2583_attribute_group;
> + chip->iio_dev->dev.parent = &clientp->dev;
> + chip->iio_dev->dev_data = (void *)(chip);
> + chip->iio_dev->driver_module = THIS_MODULE;
> + chip->iio_dev->modes = INDIO_DIRECT_MODE;
> + ret = iio_device_register(chip->iio_dev);
> + if (ret) {
> + dev_err(&clientp->dev, "iio registration failed\n");
> + goto fail1;
> + }
> +
> + /* Load up the V2 defaults (these are hard coded defaults for now) */
> + taos_defaults(chip);
> +
> + /* Make sure the chip is on */
> + taos_chip_on(clientp);
> +
> + dev_info(&clientp->dev, "Light sensor found.\n");
> + return 0;
> +
> +fail1:
> + kfree(chip);
> +
> + return ret;
> +}
> +
> +static int __devexit taos_remove(struct i2c_client *client)
> +{
> + struct tsl2583_chip *chip = i2c_get_clientdata(client);
> +
> + iio_device_unregister(chip->iio_dev);
> +
> + kfree(chip);
> + return 0;
> +}
> +
> +static struct i2c_device_id taos_idtable[] = {
> + { "tsl2580", 0 },
> + { "tsl2581", 1 },
> + { "tsl2583", 2 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, taos_idtable);
> +
> +/* Driver definition */
> +static struct i2c_driver taos_driver = {
> + .driver = {
> + .name = "tsl2583",
> + },
> + .id_table = taos_idtable,
> + .probe = taos_probe,
> + .remove = __devexit_p(taos_remove),
> +};
> +
> +static int __init taos_init(void)
> +{
> + return i2c_add_driver(&taos_driver);
> +}
> +
> +static void __exit taos_exit(void)
> +{
> + i2c_del_driver(&taos_driver);
> +}
> +
> +module_init(taos_init);
> +module_exit(taos_exit);
> +
> +MODULE_AUTHOR("J. August Brenner<jbrenner@taosinc.com>");
> +MODULE_DESCRIPTION("TAOS tsl2583 ambient light sensor driver");
> +MODULE_LICENSE("GPL");
> +
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
2011-03-21 19:07 ` Jonathan Cameron
@ 2011-03-23 1:07 ` Jon Brenner
2011-03-23 11:06 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Jon Brenner @ 2011-03-23 1:07 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, Linux Kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 5994 bytes --]
Jonathan, et al,
Again, thank you for taking the time to review the latest submission.
I have implemented all recommendations made (with the exceptions noted below).
I have also added two new functions (not seen here):
'taos_suspend', and 'taos_resume'
These changes and comments will be submitted in the next patch [PATCH V3] submission which I will submit shortly (after testing/verification).
Respectfully,
Jon
-----Original Message-----
As I pm'd you the other day, please remember to make version of patch clear in title, and include details of what has changed since previous version.
OK - NEXT PATCH WILL INDICATE VERSION 3
WILL REMOVE SENSORS_ PREFIX FROM KCONFIG & MAKEFILE
ADDED/MOVED/CREATED DOCUMENTATION AS RECOMMENDED
----- snip ------------
> +/*
> + * Provides initial operational parameter defaults.
> + * These defaults may be changed through the device's sysfs files.
> + */
How about pasing in taos_settings and making all of this func a bit more readable?
PREFER TO USE DEFAULTS METHOD
> +static void taos_defaults(struct tsl2583_chip *chip) {
> + /* Operational parameters */
> + chip->taos_settings.als_time = 450;
> + /* must be a multiple of 50mS */
> + chip->taos_settings.als_gain = 2;
> + /* this is actually an index into the gain table */
> + /* assume clear glass as default */
> + chip->taos_settings.als_gain_trim = 1000;
> + /* default gain trim to account for aperture effects */
> + chip->taos_settings.als_cal_target = 130;
> + /* Known external ALS reading used for calibration */ }
> +
----- snip ------------
> + if ((ch0 >= chip->als_saturation) || (ch1 >= chip->als_saturation))
would be easier to read if you set ret to lux max here an avoid jumping into
the if statement below.
NO - TSL258X_LUX_CALC_OVER_FLOW NEEDS TO PASSED UP.
MUTEX NEEDS TO BE UNLOCKED.
THEN RETURN.
> + goto return_max;
----- snip ------------
&taos_device_lux[0] perhaps?
NO - CAST MAKES IT MORE OBVIOUS - at least to me ;-)
> + for (p = (struct taos_lux *) taos_device_lux;
----- snip ------------
> +
If not time critical lest make that a sleep.
NO - TIME IS REQUIRED FOR DEVICE TO SETTLE BETWEEN POWERON AND ADC ENABLE - DON'T WANT TO MAKE IT ANY LONGER THAN NECESSARY.
> + mdelay(3);
> + /* NOW enable the ADC
----- snip ------------
I still wonder if there isn't a way of avoiding this index issue.
It's horrible and really feels like something we ought to able
to handle reasonably well...
BECAUSE THE CH0 AND CH1 DIVERGE AT HIGH GAINS -- INDEX IS BETTER THAN USING CHO OR CH1 VALUE
( AS EITHER ONE OVER THE OTHER WOULD BE WRONG :-{ )
> + if (value > 3) {
> + dev_err(dev, "Invalid Gain Index: Enter 0-3\n");
> + return -1;
> + } else {
> + chip->taos_settings.als_gain = value;
> + }
> + return len;
> +}
----- snip ------------
Just to confirm, is this a frequency in Hz? If it is
can we renaming it to taos_als_frequency_show to
avoid confusing me?
THIS IS THE INTERNAL ALS INTEGRATION TIME FOR THE ADC CHANNELS
I SHOE HORNED IT INTO SAMPLING FREQUENCY TO HELP ALEVIATE THE ABI ANGST
> +static ssize_t taos_als_time_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl2583_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_time);
> +}
----- snip ------------
scale as a stand alone attribute isn't defined. Please document this.
Looking at what you do with it, it's another factor effecting the overal
gain on the reading that reaches userspace. Ideally you'd roll this
into your calibscale parameter, but I can see that would get complex
to manage. Will have a think about this. In devices with a simple conversion
function (adc's etc) we handle this by leaving this software value
be applied by userspace (and output it as in_scale). The issue here that
as far as userspace is concerned both of your scales have been applied before
it sees the data and at different more or less random looking points in
the calculation.
Actually, looking at the calculation you could output illuminance0_raw
and let userspace apply a multiplier based on your trim value and offset
+0.5? If you want to hold trim in driver then just implement
read and write to
illuminance0_scale
and have read only
illuminance0_offset (rather tediously for this device, offset is applied
before scale, so you'll need to divide 0.5 by whatever your trim is).
We'll have to do pin down how to do this before moving out of staging
so best to get it right now.
CHANGED TO ILLUMINANCE0_SCALE
LIFE WOULE BE NICE IF WE COULD JUST PASS THE RAW DATA UP TO USERSPACE
BUT WE HAVE CUSTOMERS WHO REQUIRE IT TO COME UP CALCULATED WITH
THE MULTIPLIER FACTORED IN
----- snip ------------
> + for (i = 0; i < MAX_DEVICE_REGS; i++) {
> + ret = i2c_smbus_write_byte(clientp,
> + (TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
> + if (ret < 0) {
> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd "
> + "reg failed in taos_probe(), err = %d\n", ret);
> + goto fail1;
> + }
> + buf[i] = i2c_smbus_read_byte(clientp);
error handling for this read?
VALUE IS UNIMPORTANT - BUT MAKING SURE WE CAN JUST READ ALL 32 REGISTER LOCATIONS IS..
----- snip ------------
> + }
> + if (!taos_skate_device(buf)) {
> + dev_info(&clientp->dev, "i2c device found but does not match "
> + "expected id in taos_probe()\n");
> + goto fail1;
> + } else {
The sort of debug info you want to get rid of for production drivers. Doesn't
tell anyone anything helpful.
NO - DO NOT AGREE - IF A CUSTOMER EXPECTS ONE DEVICE BUT HAS SOMEHOW INSTALLED A DIFFERENT ONE
AT PRODUCTION TIME, THIS WILL HELP IDENTIFY THE ISSUE.
(IE. WANTED A TAOS ALS DEVICE BUT INSTALLED A TAOS PROX/ALS DEVICE)
FIRST PART INDICATES THAT. SECOND PART IS CONFIRMATION THAT DEVICE WAS IDENTIFIED TO SYSTEM.
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
2011-03-23 1:07 ` Jon Brenner
@ 2011-03-23 11:06 ` Jonathan Cameron
2011-03-23 22:38 ` Jon Brenner
0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2011-03-23 11:06 UTC (permalink / raw)
To: Jon Brenner; +Cc: linux-iio, Linux Kernel
On 03/23/11 01:07, Jon Brenner wrote:
> Jonathan, et al,
>
> Again, thank you for taking the time to review the latest submission.
> I have implemented all recommendations made (with the exceptions noted below).
>
> I have also added two new functions (not seen here):
> 'taos_suspend', and 'taos_resume'
>
> These changes and comments will be submitted in the next patch [PATCH V3] submission which I will submit shortly (after testing/verification).
>
> Respectfully,
> Jon
>
> -----Original Message-----
> As I pm'd you the other day, please remember to make version of patch clear in title, and include details of what has changed since previous version.
> OK - NEXT PATCH WILL INDICATE VERSION 3
>
> WILL REMOVE SENSORS_ PREFIX FROM KCONFIG & MAKEFILE
>
> ADDED/MOVED/CREATED DOCUMENTATION AS RECOMMENDED
>
> ----- snip ------------
>> +/*
>> + * Provides initial operational parameter defaults.
>> + * These defaults may be changed through the device's sysfs files.
>> + */
> How about pasing in taos_settings and making all of this func a bit more readable?
> PREFER TO USE DEFAULTS METHOD
Ah, I didn't make clear what I meant here. It was simply an
observation that having
static void taos_defaults(struct taos_settings *settings)
{
settings->als_time = 450;
etc.
}
would be easier to read.
>
>> +static void taos_defaults(struct tsl2583_chip *chip) {
>> + /* Operational parameters */
>> + chip->taos_settings.als_time = 450;
>> + /* must be a multiple of 50mS */
>> + chip->taos_settings.als_gain = 2;
>> + /* this is actually an index into the gain table */
>> + /* assume clear glass as default */
>> + chip->taos_settings.als_gain_trim = 1000;
>> + /* default gain trim to account for aperture effects */
>> + chip->taos_settings.als_cal_target = 130;
>> + /* Known external ALS reading used for calibration */ }
>> +
>
> ----- snip ------------
>> + if ((ch0 >= chip->als_saturation) || (ch1 >= chip->als_saturation))
> would be easier to read if you set ret to lux max here an avoid jumping into
> the if statement below.
> NO - TSL258X_LUX_CALC_OVER_FLOW NEEDS TO PASSED UP.
> MUTEX NEEDS TO BE UNLOCKED.
> THEN RETURN.
Sorry, should have said jump to out_unlock having set ret = LUX_CALC_OVER_FLOW;
Just makes for marginally cleaner program flow to my mind (though I don't
really care about this one).
>> + goto return_max;
>
> ----- snip ------------
> &taos_device_lux[0] perhaps?
> NO - CAST MAKES IT MORE OBVIOUS - at least to me ;-)
>> + for (p = (struct taos_lux *) taos_device_lux;
>
> ----- snip ------------
>> +
> If not time critical lest make that a sleep.
> NO - TIME IS REQUIRED FOR DEVICE TO SETTLE BETWEEN POWERON AND ADC ENABLE - DON'T WANT TO MAKE IT ANY LONGER THAN NECESSARY.
But do you want to spin the processor during this time instead of getting on with something useful?
I guess it depends on how critical speed is in the taos_chip_on function. I'd say this
device is slow enough that a possible extra delay doesn't really matter.
>> + mdelay(3);
>> + /* NOW enable the ADC
>
>
> ----- snip ------------
> I still wonder if there isn't a way of avoiding this index issue.
> It's horrible and really feels like something we ought to able
> to handle reasonably well...
> BECAUSE THE CH0 AND CH1 DIVERGE AT HIGH GAINS -- INDEX IS BETTER THAN USING CHO OR CH1 VALUE
> ( AS EITHER ONE OVER THE OTHER WOULD BE WRONG :-{ )
I can see your point to a certain extent. The counter argument is that doing
it with an index precludes ever touching this with any remotely general purpose
user space code. Given it's an internal gain anyway is the precise value that
critical? Basically is user space ever going to care about the difference between
those those values?
Another option would be to have a gain for each channel as separate attributes
(clearly writing to one would change the other).
>> + if (value > 3) {
>> + dev_err(dev, "Invalid Gain Index: Enter 0-3\n");
>> + return -1;
>> + } else {
>> + chip->taos_settings.als_gain = value;
>> + }
>> + return len;
>> +}
>
> ----- snip ------------
> Just to confirm, is this a frequency in Hz? If it is
> can we renaming it to taos_als_frequency_show to
> avoid confusing me?
>
> THIS IS THE INTERNAL ALS INTEGRATION TIME FOR THE ADC CHANNELS
> I SHOE HORNED IT INTO SAMPLING FREQUENCY TO HELP ALEVIATE THE ABI ANGST
That's good but it does have to match the units specified in the ABI
as well.
>
>> +static ssize_t taos_als_time_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct tsl2583_chip *chip = indio_dev->dev_data;
>> +
>> + return sprintf(buf, "%d\n", chip->taos_settings.als_time);
>> +}
>
> ----- snip ------------
> scale as a stand alone attribute isn't defined. Please document this.
> Looking at what you do with it, it's another factor effecting the overal
> gain on the reading that reaches userspace. Ideally you'd roll this
> into your calibscale parameter, but I can see that would get complex
> to manage. Will have a think about this. In devices with a simple conversion
> function (adc's etc) we handle this by leaving this software value
> be applied by userspace (and output it as in_scale). The issue here that
> as far as userspace is concerned both of your scales have been applied before
> it sees the data and at different more or less random looking points in
> the calculation.
>
> Actually, looking at the calculation you could output illuminance0_raw
> and let userspace apply a multiplier based on your trim value and offset
> +0.5? If you want to hold trim in driver then just implement
> read and write to
>
> illuminance0_scale
> and have read only
> illuminance0_offset (rather tediously for this device, offset is applied
> before scale, so you'll need to divide 0.5 by whatever your trim is).
>
> We'll have to do pin down how to do this before moving out of staging
> so best to get it right now.
>
> CHANGED TO ILLUMINANCE0_SCALE
>
> LIFE WOULE BE NICE IF WE COULD JUST PASS THE RAW DATA UP TO USERSPACE
> BUT WE HAVE CUSTOMERS WHO REQUIRE IT TO COME UP CALCULATED WITH
> THE MULTIPLIER FACTORED IN
Sure. It's a fiddly calculation so I can kind of see their point.
These light sensors are all a pain :)
>
> ----- snip ------------
>> + for (i = 0; i < MAX_DEVICE_REGS; i++) {
>> + ret = i2c_smbus_write_byte(clientp,
>> + (TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
>> + if (ret < 0) {
>> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd "
>> + "reg failed in taos_probe(), err = %d\n", ret);
>> + goto fail1;
>> + }
>> + buf[i] = i2c_smbus_read_byte(clientp);
> error handling for this read?
> VALUE IS UNIMPORTANT - BUT MAKING SURE WE CAN JUST READ ALL 32 REGISTER LOCATIONS IS..
>
> ----- snip ------------
>
>> + }
>> + if (!taos_skate_device(buf)) {
>> + dev_info(&clientp->dev, "i2c device found but does not match "
>> + "expected id in taos_probe()\n");
>> + goto fail1;
>> + } else {
> The sort of debug info you want to get rid of for production drivers. Doesn't
> tell anyone anything helpful.
> NO - DO NOT AGREE - IF A CUSTOMER EXPECTS ONE DEVICE BUT HAS SOMEHOW INSTALLED A DIFFERENT ONE
> AT PRODUCTION TIME, THIS WILL HELP IDENTIFY THE ISSUE.
> (IE. WANTED A TAOS ALS DEVICE BUT INSTALLED A TAOS PROX/ALS DEVICE)
> FIRST PART INDICATES THAT. SECOND PART IS CONFIRMATION THAT DEVICE WAS IDENTIFIED TO SYSTEM.
I'm happy with the error one. Just don't see the point in saying 'everything is as expected'.
The mere absence of the error indicates that just fine!
Jonathan
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
2011-03-23 11:06 ` Jonathan Cameron
@ 2011-03-23 22:38 ` Jon Brenner
2011-03-24 11:15 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Jon Brenner @ 2011-03-23 22:38 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 9363 bytes --]
Hi Jonathan,
Below are a few comments/questions to your latest response.
Any direction is greatly appreciated.
Jon
-----Original Message-----
From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
Sent: Wednesday, March 23, 2011 6:07 AM
To: Jon Brenner
Cc: linux-iio; Linux Kernel
Subject: Re: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
On 03/23/11 01:07, Jon Brenner wrote:
> Jonathan, et al,
>
> Again, thank you for taking the time to review the latest submission.
> I have implemented all recommendations made (with the exceptions noted below).
>
> I have also added two new functions (not seen here):
> 'taos_suspend', and 'taos_resume'
>
> These changes and comments will be submitted in the next patch [PATCH V3] submission which I will submit shortly (after testing/verification).
>
> Respectfully,
> Jon
>
> -----Original Message-----
> As I pm'd you the other day, please remember to make version of patch clear in title, and include details of what has changed since previous version.
> OK - NEXT PATCH WILL INDICATE VERSION 3
>
> WILL REMOVE SENSORS_ PREFIX FROM KCONFIG & MAKEFILE
>
> ADDED/MOVED/CREATED DOCUMENTATION AS RECOMMENDED
>
> ----- snip ------------
>> +/*
>> + * Provides initial operational parameter defaults.
>> + * These defaults may be changed through the device's sysfs files.
>> + */
> How about pasing in taos_settings and making all of this func a bit more readable?
> PREFER TO USE DEFAULTS METHOD
Ah, I didn't make clear what I meant here. It was simply an observation that having
static void taos_defaults(struct taos_settings *settings) {
settings->als_time = 450;
etc.
}
would be easier to read.
>
>> +static void taos_defaults(struct tsl2583_chip *chip) {
>> + /* Operational parameters */
>> + chip->taos_settings.als_time = 450;
>> + /* must be a multiple of 50mS */
>> + chip->taos_settings.als_gain = 2;
>> + /* this is actually an index into the gain table */
>> + /* assume clear glass as default */
>> + chip->taos_settings.als_gain_trim = 1000;
>> + /* default gain trim to account for aperture effects */
>> + chip->taos_settings.als_cal_target = 130;
>> + /* Known external ALS reading used for calibration */ }
>> +
>
> ----- snip ------------
>> + if ((ch0 >= chip->als_saturation) || (ch1 >= chip->als_saturation))
> would be easier to read if you set ret to lux max here an avoid
> jumping into the if statement below.
> NO - TSL258X_LUX_CALC_OVER_FLOW NEEDS TO PASSED UP.
> MUTEX NEEDS TO BE UNLOCKED.
> THEN RETURN.
Sorry, should have said jump to out_unlock having set ret = LUX_CALC_OVER_FLOW;
Just makes for marginally cleaner program flow to my mind (though I don't really care about this one).
I UNDERSTAND BUT I FORGOT TO MENTION THAT IN ADDITION TO THE ABOVE, ALSO NEEDS TO UPDATE THE ALS_CUR_INFO STRUCT.
SO, ALL SAID - EASIER TO SIMPLY JUMP TO RETURN_MAX, AS THIS IS ALL DONE THERE.
>> + goto return_max;
>
> ----- snip ------------
> &taos_device_lux[0] perhaps?
> NO - CAST MAKES IT MORE OBVIOUS - at least to me ;-)
>> + for (p = (struct taos_lux *) taos_device_lux;
>
> ----- snip ------------
>> +
> If not time critical lest make that a sleep.
> NO - TIME IS REQUIRED FOR DEVICE TO SETTLE BETWEEN POWERON AND ADC ENABLE - DON'T WANT TO MAKE IT ANY LONGER THAN NECESSARY.
But do you want to spin the processor during this time instead of getting on with something useful?
I guess it depends on how critical speed is in the taos_chip_on function. I'd say this device is slow enough that a possible extra delay doesn't really matter.
>> + mdelay(3);
AGREED - CHANGED TO MSLEEP(3);
>> + /* NOW enable the ADC
>
>
> ----- snip ------------
> I still wonder if there isn't a way of avoiding this index issue.
> It's horrible and really feels like something we ought to able to
> handle reasonably well...
> BECAUSE THE CH0 AND CH1 DIVERGE AT HIGH GAINS -- INDEX IS BETTER THAN
> USING CHO OR CH1 VALUE ( AS EITHER ONE OVER THE OTHER WOULD BE WRONG
> :-{ )
I can see your point to a certain extent. The counter argument is that doing it with an index precludes ever touching this with any remotely general purpose user space code. Given it's an internal gain anyway is the precise value that critical? Basically is user space ever going to care about the difference between those those values?
Another option would be to have a gain for each channel as separate attributes (clearly writing to one would change the other).
I SEE YOUR POINT TOO - BUT WHAT ELSE CAN I DO (ASIDE FROM A SEPARATE GAIN FOR EACH CHANNEL - WHICH WOULD BE CONFUSING TO MANAGE)?
I GUESS I COULD BUCKETIZE ANYTHING > GAIN 16 - BUT THAT SEEMS RATHER KLUDGED?
AGAIN SEEMS LIKE MOST REASONABLE (FOR NOW ANYWAY) IS AN INDEX.
>> + if (value > 3) {
>> + dev_err(dev, "Invalid Gain Index: Enter 0-3\n");
>> + return -1;
>> + } else {
>> + chip->taos_settings.als_gain = value;
>> + }
>> + return len;
>> +}
>
> ----- snip ------------
> Just to confirm, is this a frequency in Hz? If it is can we renaming
> it to taos_als_frequency_show to avoid confusing me?
>
> THIS IS THE INTERNAL ALS INTEGRATION TIME FOR THE ADC CHANNELS I SHOE
> HORNED IT INTO SAMPLING FREQUENCY TO HELP ALEVIATE THE ABI ANGST
That's good but it does have to match the units specified in the ABI as well.
OK - MAYBE I'M MISSING SOME INFO.
ALL I HAVE FOR THE ABI IS THIS:
what: /sys/bus/iio/devices/deviceX/sampling_frequency
KernelVersion: 2.6.35
Contact: linux-iio@vger.kernel.org
Description:
Some devices have internal clocks. This parameter sets the
resulting sampling frequency. In many devices this
parameter has an effect on input filters etc rather than
simply controlling when the input is sampled. As this
effects datardy triggers, hardware buffers and the sysfs
direct access interfaces, it may be found in any of the
relevant directories. If it effects all of the above
then it is to be found in the base device directory as here.
SO I AM UNSURE OF WHAT UNITS NEED TO MATCH?
>
>> +static ssize_t taos_als_time_show(struct device *dev,
>> + struct device_attribute *attr, char *buf) {
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct tsl2583_chip *chip = indio_dev->dev_data;
>> +
>> + return sprintf(buf, "%d\n", chip->taos_settings.als_time); }
>
> ----- snip ------------
> scale as a stand alone attribute isn't defined. Please document this.
> Looking at what you do with it, it's another factor effecting the
> overal gain on the reading that reaches userspace. Ideally you'd roll
> this into your calibscale parameter, but I can see that would get
> complex to manage. Will have a think about this. In devices with a
> simple conversion function (adc's etc) we handle this by leaving this
> software value be applied by userspace (and output it as in_scale).
> The issue here that as far as userspace is concerned both of your
> scales have been applied before it sees the data and at different more
> or less random looking points in the calculation.
>
> Actually, looking at the calculation you could output illuminance0_raw
> and let userspace apply a multiplier based on your trim value and
> offset
> +0.5? If you want to hold trim in driver then just implement
> read and write to
>
> illuminance0_scale
> and have read only
> illuminance0_offset (rather tediously for this device, offset is
> applied before scale, so you'll need to divide 0.5 by whatever your trim is).
>
> We'll have to do pin down how to do this before moving out of staging
> so best to get it right now.
>
> CHANGED TO ILLUMINANCE0_SCALE
>
> LIFE WOULE BE NICE IF WE COULD JUST PASS THE RAW DATA UP TO USERSPACE
> BUT WE HAVE CUSTOMERS WHO REQUIRE IT TO COME UP CALCULATED WITH THE
> MULTIPLIER FACTORED IN
Sure. It's a fiddly calculation so I can kind of see their point.
These light sensors are all a pain :)
SO - WE ARE OK?
>
> ----- snip ------------
>> + for (i = 0; i < MAX_DEVICE_REGS; i++) {
>> + ret = i2c_smbus_write_byte(clientp,
>> + (TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
>> + if (ret < 0) {
>> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd "
>> + "reg failed in taos_probe(), err = %d\n", ret);
>> + goto fail1;
>> + }
>> + buf[i] = i2c_smbus_read_byte(clientp);
> error handling for this read?
> VALUE IS UNIMPORTANT - BUT MAKING SURE WE CAN JUST READ ALL 32 REGISTER LOCATIONS IS..
>
> ----- snip ------------
>
>> + }
>> + if (!taos_skate_device(buf)) {
>> + dev_info(&clientp->dev, "i2c device found but does not match "
>> + "expected id in taos_probe()\n");
>> + goto fail1;
>> + } else {
> The sort of debug info you want to get rid of for production drivers.
> Doesn't tell anyone anything helpful.
> NO - DO NOT AGREE - IF A CUSTOMER EXPECTS ONE DEVICE BUT HAS SOMEHOW
> INSTALLED A DIFFERENT ONE AT PRODUCTION TIME, THIS WILL HELP IDENTIFY THE ISSUE.
> (IE. WANTED A TAOS ALS DEVICE BUT INSTALLED A TAOS PROX/ALS DEVICE)
> FIRST PART INDICATES THAT. SECOND PART IS CONFIRMATION THAT DEVICE WAS IDENTIFIED TO SYSTEM.
I'm happy with the error one. Just don't see the point in saying 'everything is as expected'.
The mere absence of the error indicates that just fine!
AGREED - REMOVED 2ND PART (THE OK MESSAGE)
Jonathan
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
2011-03-23 22:38 ` Jon Brenner
@ 2011-03-24 11:15 ` Jonathan Cameron
2011-03-24 19:04 ` Jon Brenner
0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2011-03-24 11:15 UTC (permalink / raw)
To: Jon Brenner; +Cc: linux-iio, linux-kernel
On 03/23/11 22:38, Jon Brenner wrote:
> Hi Jonathan,
> Below are a few comments/questions to your latest response.
> Any direction is greatly appreciated.
>
> Jon
No chance you can persuade your email client to do conventional
indenting? Ah well, I guess this works more or less.
>>
>> ----- snip ------------
>>> + if ((ch0 >= chip->als_saturation) || (ch1 >= chip->als_saturation))
>> would be easier to read if you set ret to lux max here an avoid
>> jumping into the if statement below.
>> NO - TSL258X_LUX_CALC_OVER_FLOW NEEDS TO PASSED UP.
>> MUTEX NEEDS TO BE UNLOCKED.
>> THEN RETURN.
> Sorry, should have said jump to out_unlock having set ret = LUX_CALC_OVER_FLOW;
>
> Just makes for marginally cleaner program flow to my mind (though I don't really care about this one).
>
> I UNDERSTAND BUT I FORGOT TO MENTION THAT IN ADDITION TO THE ABOVE, ALSO NEEDS TO UPDATE THE ALS_CUR_INFO STRUCT.
> SO, ALL SAID - EASIER TO SIMPLY JUMP TO RETURN_MAX, AS THIS IS ALL DONE THERE.
Fair enough. Was just personal preference anyway!
...
>
>>> + /* NOW enable the ADC
>>
>>
>> ----- snip ------------
>> I still wonder if there isn't a way of avoiding this index issue.
>> It's horrible and really feels like something we ought to able to
>> handle reasonably well...
>> BECAUSE THE CH0 AND CH1 DIVERGE AT HIGH GAINS -- INDEX IS BETTER THAN
>> USING CHO OR CH1 VALUE ( AS EITHER ONE OVER THE OTHER WOULD BE WRONG
>> :-{ )
> I can see your point to a certain extent. The counter argument is that doing it with an index precludes ever touching this with any remotely general purpose user space code. Given it's an internal gain anyway is the precise value that critical? Basically is user space ever going to care about the difference between those those values?
>
> Another option would be to have a gain for each channel as separate attributes (clearly writing to one would change the other).
>
> I SEE YOUR POINT TOO - BUT WHAT ELSE CAN I DO (ASIDE FROM A SEPARATE GAIN FOR EACH CHANNEL - WHICH WOULD BE CONFUSING TO MANAGE)?
> I GUESS I COULD BUCKETIZE ANYTHING > GAIN 16 - BUT THAT SEEMS RATHER KLUDGED?
> AGAIN SEEMS LIKE MOST REASONABLE (FOR NOW ANYWAY) IS AN INDEX.
It may be reasonable, but it isn't generalizable which means it probably isn't viable
long term. The parameter simply won't be used by anything that isn't custom coded
for this particular sensor. Even Taos' next generation of sensors probably won't
have an index which has the same effect.
If it can be coherently blugeoned into existing interfaces, then we should do so.
There are quite a few other bits of IIO interface where changing one value will
effect others. This means (and we could emphasise it more) that there are sets
of values user space MUST check after making a given change if it wants to know
the full device state. Believe me, things are much more complex with devices
that do partial scans of channels only in some combinations!
>> Just to confirm, is this a frequency in Hz? If it is can we renaming
>> it to taos_als_frequency_show to avoid confusing me?
>>
>> THIS IS THE INTERNAL ALS INTEGRATION TIME FOR THE ADC CHANNELS I SHOE
>> HORNED IT INTO SAMPLING FREQUENCY TO HELP ALEVIATE THE ABI ANGST
> That's good but it does have to match the units specified in the ABI as well.
>
> OK - MAYBE I'M MISSING SOME INFO.
> ALL I HAVE FOR THE ABI IS THIS:
> what: /sys/bus/iio/devices/deviceX/sampling_frequency
> KernelVersion: 2.6.35
> Contact: linux-iio@vger.kernel.org
> Description:
> Some devices have internal clocks. This parameter sets the
> resulting sampling frequency. In many devices this
> parameter has an effect on input filters etc rather than
> simply controlling when the input is sampled. As this
> effects datardy triggers, hardware buffers and the sysfs
> direct access interfaces, it may be found in any of the
> relevant directories. If it effects all of the above
> then it is to be found in the base device directory as here.
>
> SO I AM UNSURE OF WHAT UNITS NEED TO MATCH?
Fair point. That documentation is clearly lacking. One for the TODO list.
Anyhow, it does say sampling frequency so the units can't be a measure of
time. What it should say is 'in Hz' somewhere.
>
>>
>>> +static ssize_t taos_als_time_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf) {
>>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>> + struct tsl2583_chip *chip = indio_dev->dev_data;
>>> +
>>> + return sprintf(buf, "%d\n", chip->taos_settings.als_time); }
>>
>> ----- snip ------------
>> scale as a stand alone attribute isn't defined. Please document this.
>> Looking at what you do with it, it's another factor effecting the
>> overal gain on the reading that reaches userspace. Ideally you'd roll
>> this into your calibscale parameter, but I can see that would get
>> complex to manage. Will have a think about this. In devices with a
>> simple conversion function (adc's etc) we handle this by leaving this
>> software value be applied by userspace (and output it as in_scale).
>> The issue here that as far as userspace is concerned both of your
>> scales have been applied before it sees the data and at different more
>> or less random looking points in the calculation.
>>
>> Actually, looking at the calculation you could output illuminance0_raw
>> and let userspace apply a multiplier based on your trim value and
>> offset
>> +0.5? If you want to hold trim in driver then just implement
>> read and write to
>>
>> illuminance0_scale
>> and have read only
>> illuminance0_offset (rather tediously for this device, offset is
>> applied before scale, so you'll need to divide 0.5 by whatever your trim is).
>>
>> We'll have to do pin down how to do this before moving out of staging
>> so best to get it right now.
>>
>> CHANGED TO ILLUMINANCE0_SCALE
>>
>> LIFE WOULE BE NICE IF WE COULD JUST PASS THE RAW DATA UP TO USERSPACE
>> BUT WE HAVE CUSTOMERS WHO REQUIRE IT TO COME UP CALCULATED WITH THE
>> MULTIPLIER FACTORED IN
> Sure. It's a fiddly calculation so I can kind of see their point.
> These light sensors are all a pain :)
> SO - WE ARE OK?
Err. I've kind of lost track, but I think so. Will take one last look
at your next revision! Sounds like it may have some interesting new features
anyway given the other thread!
>>
>> ----- snip ------------
>>> + for (i = 0; i < MAX_DEVICE_REGS; i++) {
>>> + ret = i2c_smbus_write_byte(clientp,
>>> + (TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
>>> + if (ret < 0) {
>>> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd "
>>> + "reg failed in taos_probe(), err = %d\n", ret);
>>> + goto fail1;
>>> + }
>>> + buf[i] = i2c_smbus_read_byte(clientp);
>> error handling for this read?
>> VALUE IS UNIMPORTANT - BUT MAKING SURE WE CAN JUST READ ALL 32 REGISTER LOCATIONS IS..
That's fine, but you haven't verified that if you don't check for a negative return
from that read.
>>
>> ----- snip ------------
>>
>>> + }
>>> + if (!taos_skate_device(buf)) {
>>> + dev_info(&clientp->dev, "i2c device found but does not match "
>>> + "expected id in taos_probe()\n");
>>> + goto fail1;
>>> + } else {
>> The sort of debug info you want to get rid of for production drivers.
>> Doesn't tell anyone anything helpful.
>> NO - DO NOT AGREE - IF A CUSTOMER EXPECTS ONE DEVICE BUT HAS SOMEHOW
>> INSTALLED A DIFFERENT ONE AT PRODUCTION TIME, THIS WILL HELP IDENTIFY THE ISSUE.
>> (IE. WANTED A TAOS ALS DEVICE BUT INSTALLED A TAOS PROX/ALS DEVICE)
>> FIRST PART INDICATES THAT. SECOND PART IS CONFIRMATION THAT DEVICE WAS IDENTIFIED TO SYSTEM.
>
> I'm happy with the error one. Just don't see the point in saying 'everything is as expected'.
> The mere absence of the error indicates that just fine!
> AGREED - REMOVED 2ND PART (THE OK MESSAGE)
Cool
Coming together nicely.
Jonathan
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
2011-03-24 11:15 ` Jonathan Cameron
@ 2011-03-24 19:04 ` Jon Brenner
2011-03-24 20:10 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Jon Brenner @ 2011-03-24 19:04 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel
Hi Jonathan,
The remaining issue that I am still trying to resolve is dealing with a
proper ABI for the device integration time.
If 'illuminance0_sampling_frequency' is not the proper ABI, should I
create a new ABI
called illuminance0_integration_time and add it to the
sys-bus-iio-lisgt-tsl2583 document as you had me do with
/sys/bus/iio/devices/device[n]/lux_table?
Jon
-----Original Message-----
From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
Sent: Thursday, March 24, 2011 6:16 AM
To: Jon Brenner
Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
On 03/23/11 22:38, Jon Brenner wrote:
> Hi Jonathan,
> Below are a few comments/questions to your latest response.
> Any direction is greatly appreciated.
>
> Jon
No chance you can persuade your email client to do conventional
indenting? Ah well, I guess this works more or less.
>>
>> ----- snip ------------
>>> + if ((ch0 >= chip->als_saturation) || (ch1 >=
>>> +chip->als_saturation))
>> would be easier to read if you set ret to lux max here an avoid
>> jumping into the if statement below.
>> NO - TSL258X_LUX_CALC_OVER_FLOW NEEDS TO PASSED UP.
>> MUTEX NEEDS TO BE UNLOCKED.
>> THEN RETURN.
> Sorry, should have said jump to out_unlock having set ret =
> LUX_CALC_OVER_FLOW;
>
> Just makes for marginally cleaner program flow to my mind (though I
don't really care about this one).
>
> I UNDERSTAND BUT I FORGOT TO MENTION THAT IN ADDITION TO THE ABOVE,
ALSO NEEDS TO UPDATE THE ALS_CUR_INFO STRUCT.
> SO, ALL SAID - EASIER TO SIMPLY JUMP TO RETURN_MAX, AS THIS IS ALL
DONE THERE.
Fair enough. Was just personal preference anyway!
...
>
>>> + /* NOW enable the ADC
>>
>>
>> ----- snip ------------
>> I still wonder if there isn't a way of avoiding this index issue.
>> It's horrible and really feels like something we ought to able to
>> handle reasonably well...
>> BECAUSE THE CH0 AND CH1 DIVERGE AT HIGH GAINS -- INDEX IS BETTER THAN
>> USING CHO OR CH1 VALUE ( AS EITHER ONE OVER THE OTHER WOULD BE WRONG
>> :-{ )
> I can see your point to a certain extent. The counter argument is
that doing it with an index precludes ever touching this with any
remotely general purpose user space code. Given it's an internal gain
anyway is the precise value that critical? Basically is user space ever
going to care about the difference between those those values?
>
> Another option would be to have a gain for each channel as separate
attributes (clearly writing to one would change the other).
>
> I SEE YOUR POINT TOO - BUT WHAT ELSE CAN I DO (ASIDE FROM A SEPARATE
GAIN FOR EACH CHANNEL - WHICH WOULD BE CONFUSING TO MANAGE)?
> I GUESS I COULD BUCKETIZE ANYTHING > GAIN 16 - BUT THAT SEEMS RATHER
KLUDGED?
> AGAIN SEEMS LIKE MOST REASONABLE (FOR NOW ANYWAY) IS AN INDEX.
It may be reasonable, but it isn't generalizable which means it probably
isn't viable long term. The parameter simply won't be used by anything
that isn't custom coded
for this particular sensor. Even Taos' next generation of sensors
probably won't
have an index which has the same effect.
If it can be coherently blugeoned into existing interfaces, then we
should do so.
There are quite a few other bits of IIO interface where changing one
value will effect others. This means (and we could emphasise it more)
that there are sets of values user space MUST check after making a given
change if it wants to know the full device state. Believe me, things
are much more complex with devices that do partial scans of channels
only in some combinations!
>> Just to confirm, is this a frequency in Hz? If it is can we renaming
>> it to taos_als_frequency_show to avoid confusing me?
>>
>> THIS IS THE INTERNAL ALS INTEGRATION TIME FOR THE ADC CHANNELS I SHOE
>> HORNED IT INTO SAMPLING FREQUENCY TO HELP ALEVIATE THE ABI ANGST
> That's good but it does have to match the units specified in the ABI
as well.
>
> OK - MAYBE I'M MISSING SOME INFO.
> ALL I HAVE FOR THE ABI IS THIS:
> what: /sys/bus/iio/devices/deviceX/sampling_frequency
> KernelVersion: 2.6.35
> Contact: linux-iio@vger.kernel.org
> Description:
> Some devices have internal clocks. This parameter sets
the
> resulting sampling frequency. In many devices this
> parameter has an effect on input filters etc rather than
> simply controlling when the input is sampled. As this
> effects datardy triggers, hardware buffers and the sysfs
> direct access interfaces, it may be found in any of the
> relevant directories. If it effects all of the above
> then it is to be found in the base device directory as
here.
>
> SO I AM UNSURE OF WHAT UNITS NEED TO MATCH?
Fair point. That documentation is clearly lacking. One for the TODO
list.
Anyhow, it does say sampling frequency so the units can't be a measure
of time. What it should say is 'in Hz' somewhere.
>
>>
>>> +static ssize_t taos_als_time_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf) {
>>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>> + struct tsl2583_chip *chip = indio_dev->dev_data;
>>> +
>>> + return sprintf(buf, "%d\n", chip->taos_settings.als_time); }
>>
>> ----- snip ------------
>> scale as a stand alone attribute isn't defined. Please document this.
>> Looking at what you do with it, it's another factor effecting the
>> overal gain on the reading that reaches userspace. Ideally you'd
>> roll this into your calibscale parameter, but I can see that would
>> get complex to manage. Will have a think about this. In devices with
>> a simple conversion function (adc's etc) we handle this by leaving
>> this software value be applied by userspace (and output it as
in_scale).
>> The issue here that as far as userspace is concerned both of your
>> scales have been applied before it sees the data and at different
>> more or less random looking points in the calculation.
>>
>> Actually, looking at the calculation you could output
>> illuminance0_raw and let userspace apply a multiplier based on your
>> trim value and offset
>> +0.5? If you want to hold trim in driver then just implement
>> read and write to
>>
>> illuminance0_scale
>> and have read only
>> illuminance0_offset (rather tediously for this device, offset is
>> applied before scale, so you'll need to divide 0.5 by whatever your
trim is).
>>
>> We'll have to do pin down how to do this before moving out of staging
>> so best to get it right now.
>>
>> CHANGED TO ILLUMINANCE0_SCALE
>>
>> LIFE WOULE BE NICE IF WE COULD JUST PASS THE RAW DATA UP TO USERSPACE
>> BUT WE HAVE CUSTOMERS WHO REQUIRE IT TO COME UP CALCULATED WITH THE
>> MULTIPLIER FACTORED IN
> Sure. It's a fiddly calculation so I can kind of see their point.
> These light sensors are all a pain :)
> SO - WE ARE OK?
Err. I've kind of lost track, but I think so. Will take one last look
at your next revision! Sounds like it may have some interesting new
features anyway given the other thread!
>>
>> ----- snip ------------
>>> + for (i = 0; i < MAX_DEVICE_REGS; i++) {
>>> + ret = i2c_smbus_write_byte(clientp,
>>> + (TSL258X_CMD_REG | (TSL258X_CNTRL +
i)));
>>> + if (ret < 0) {
>>> + dev_err(&clientp->dev, "i2c_smbus_write_byte()
to cmd "
>>> + "reg failed in taos_probe(), err =
%d\n", ret);
>>> + goto fail1;
>>> + }
>>> + buf[i] = i2c_smbus_read_byte(clientp);
>> error handling for this read?
>> VALUE IS UNIMPORTANT - BUT MAKING SURE WE CAN JUST READ ALL 32
REGISTER LOCATIONS IS..
That's fine, but you haven't verified that if you don't check for a
negative return from that read.
AGREED - ADDED CHECK FOR NEGATIVE RETURN
>>
>> ----- snip ------------
>>
>>> + }
>>> + if (!taos_skate_device(buf)) {
>>> + dev_info(&clientp->dev, "i2c device found but does not
match "
>>> + "expected id in taos_probe()\n");
>>> + goto fail1;
>>> + } else {
>> The sort of debug info you want to get rid of for production drivers.
>> Doesn't tell anyone anything helpful.
>> NO - DO NOT AGREE - IF A CUSTOMER EXPECTS ONE DEVICE BUT HAS SOMEHOW
>> INSTALLED A DIFFERENT ONE AT PRODUCTION TIME, THIS WILL HELP IDENTIFY
THE ISSUE.
>> (IE. WANTED A TAOS ALS DEVICE BUT INSTALLED A TAOS PROX/ALS DEVICE)
>> FIRST PART INDICATES THAT. SECOND PART IS CONFIRMATION THAT DEVICE
WAS IDENTIFIED TO SYSTEM.
>
> I'm happy with the error one. Just don't see the point in saying
'everything is as expected'.
> The mere absence of the error indicates that just fine!
> AGREED - REMOVED 2ND PART (THE OK MESSAGE)
Cool
Coming together nicely.
Jonathan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
2011-03-24 19:04 ` Jon Brenner
@ 2011-03-24 20:10 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2011-03-24 20:10 UTC (permalink / raw)
To: Jon Brenner; +Cc: linux-iio, linux-kernel
On 03/24/11 19:04, Jon Brenner wrote:
> Hi Jonathan,
> The remaining issue that I am still trying to resolve is dealing with a
> proper ABI for the device integration time.
> If 'illuminance0_sampling_frequency' is not the proper ABI, should I
> create a new ABI
> called illuminance0_integration_time and add it to the
> sys-bus-iio-lisgt-tsl2583 document as you had me do with
> /sys/bus/iio/devices/device[n]/lux_table?
Hmm. It really isn't fitting well in sampling frequency is it?
Obviously it is connected to the frequency at which one can sample,
but isn't quite the same.
Throw an email to linux-iio proposing such an attribute and lets
see what people come back with. This sort of integration time
is pretty specific to light sensors so for now add it to the
light docs - it is however a general thing on light sensors
so we want something that isn't device specific.
Make sure it is in sensible units though (seconds).
>
> Jon
> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
> Sent: Thursday, March 24, 2011 6:16 AM
> To: Jon Brenner
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
>
> On 03/23/11 22:38, Jon Brenner wrote:
>> Hi Jonathan,
>> Below are a few comments/questions to your latest response.
>> Any direction is greatly appreciated.
>>
>> Jon
> No chance you can persuade your email client to do conventional
> indenting? Ah well, I guess this works more or less.
>
>>>
>>> ----- snip ------------
>>>> + if ((ch0 >= chip->als_saturation) || (ch1 >=
>>>> +chip->als_saturation))
>>> would be easier to read if you set ret to lux max here an avoid
>>> jumping into the if statement below.
>>> NO - TSL258X_LUX_CALC_OVER_FLOW NEEDS TO PASSED UP.
>>> MUTEX NEEDS TO BE UNLOCKED.
>>> THEN RETURN.
>> Sorry, should have said jump to out_unlock having set ret =
>> LUX_CALC_OVER_FLOW;
>>
>> Just makes for marginally cleaner program flow to my mind (though I
> don't really care about this one).
>>
>> I UNDERSTAND BUT I FORGOT TO MENTION THAT IN ADDITION TO THE ABOVE,
> ALSO NEEDS TO UPDATE THE ALS_CUR_INFO STRUCT.
>> SO, ALL SAID - EASIER TO SIMPLY JUMP TO RETURN_MAX, AS THIS IS ALL
> DONE THERE.
> Fair enough. Was just personal preference anyway!
> ...
>>
>>>> + /* NOW enable the ADC
>>>
>>>
>>> ----- snip ------------
>>> I still wonder if there isn't a way of avoiding this index issue.
>>> It's horrible and really feels like something we ought to able to
>>> handle reasonably well...
>>> BECAUSE THE CH0 AND CH1 DIVERGE AT HIGH GAINS -- INDEX IS BETTER THAN
>
>>> USING CHO OR CH1 VALUE ( AS EITHER ONE OVER THE OTHER WOULD BE WRONG
>>> :-{ )
>> I can see your point to a certain extent. The counter argument is
> that doing it with an index precludes ever touching this with any
> remotely general purpose user space code. Given it's an internal gain
> anyway is the precise value that critical? Basically is user space ever
> going to care about the difference between those those values?
>>
>> Another option would be to have a gain for each channel as separate
> attributes (clearly writing to one would change the other).
>>
>> I SEE YOUR POINT TOO - BUT WHAT ELSE CAN I DO (ASIDE FROM A SEPARATE
> GAIN FOR EACH CHANNEL - WHICH WOULD BE CONFUSING TO MANAGE)?
>> I GUESS I COULD BUCKETIZE ANYTHING > GAIN 16 - BUT THAT SEEMS RATHER
> KLUDGED?
>> AGAIN SEEMS LIKE MOST REASONABLE (FOR NOW ANYWAY) IS AN INDEX.
> It may be reasonable, but it isn't generalizable which means it probably
> isn't viable long term. The parameter simply won't be used by anything
> that isn't custom coded
> for this particular sensor. Even Taos' next generation of sensors
> probably won't
> have an index which has the same effect.
>
> If it can be coherently blugeoned into existing interfaces, then we
> should do so.
> There are quite a few other bits of IIO interface where changing one
> value will effect others. This means (and we could emphasise it more)
> that there are sets of values user space MUST check after making a given
> change if it wants to know the full device state. Believe me, things
> are much more complex with devices that do partial scans of channels
> only in some combinations!
>
>>> Just to confirm, is this a frequency in Hz? If it is can we renaming
>>> it to taos_als_frequency_show to avoid confusing me?
>>>
>>> THIS IS THE INTERNAL ALS INTEGRATION TIME FOR THE ADC CHANNELS I SHOE
>
>>> HORNED IT INTO SAMPLING FREQUENCY TO HELP ALEVIATE THE ABI ANGST
>> That's good but it does have to match the units specified in the ABI
> as well.
>>
>> OK - MAYBE I'M MISSING SOME INFO.
>> ALL I HAVE FOR THE ABI IS THIS:
>> what: /sys/bus/iio/devices/deviceX/sampling_frequency
>> KernelVersion: 2.6.35
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> Some devices have internal clocks. This parameter sets
> the
>> resulting sampling frequency. In many devices this
>> parameter has an effect on input filters etc rather than
>> simply controlling when the input is sampled. As this
>> effects datardy triggers, hardware buffers and the sysfs
>> direct access interfaces, it may be found in any of the
>> relevant directories. If it effects all of the above
>> then it is to be found in the base device directory as
> here.
>>
>> SO I AM UNSURE OF WHAT UNITS NEED TO MATCH?
> Fair point. That documentation is clearly lacking. One for the TODO
> list.
> Anyhow, it does say sampling frequency so the units can't be a measure
> of time. What it should say is 'in Hz' somewhere.
>>
>>>
>>>> +static ssize_t taos_als_time_show(struct device *dev,
>>>> + struct device_attribute *attr, char *buf) {
>>>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>>> + struct tsl2583_chip *chip = indio_dev->dev_data;
>>>> +
>>>> + return sprintf(buf, "%d\n", chip->taos_settings.als_time); }
>>>
>>> ----- snip ------------
>>> scale as a stand alone attribute isn't defined. Please document this.
>>> Looking at what you do with it, it's another factor effecting the
>>> overal gain on the reading that reaches userspace. Ideally you'd
>>> roll this into your calibscale parameter, but I can see that would
>>> get complex to manage. Will have a think about this. In devices with
>>> a simple conversion function (adc's etc) we handle this by leaving
>>> this software value be applied by userspace (and output it as
> in_scale).
>>> The issue here that as far as userspace is concerned both of your
>>> scales have been applied before it sees the data and at different
>>> more or less random looking points in the calculation.
>>>
>>> Actually, looking at the calculation you could output
>>> illuminance0_raw and let userspace apply a multiplier based on your
>>> trim value and offset
>>> +0.5? If you want to hold trim in driver then just implement
>>> read and write to
>>>
>>> illuminance0_scale
>>> and have read only
>>> illuminance0_offset (rather tediously for this device, offset is
>>> applied before scale, so you'll need to divide 0.5 by whatever your
> trim is).
>>>
>>> We'll have to do pin down how to do this before moving out of staging
>
>>> so best to get it right now.
>>>
>>> CHANGED TO ILLUMINANCE0_SCALE
>>>
>>> LIFE WOULE BE NICE IF WE COULD JUST PASS THE RAW DATA UP TO USERSPACE
>
>>> BUT WE HAVE CUSTOMERS WHO REQUIRE IT TO COME UP CALCULATED WITH THE
>>> MULTIPLIER FACTORED IN
>> Sure. It's a fiddly calculation so I can kind of see their point.
>> These light sensors are all a pain :)
>> SO - WE ARE OK?
> Err. I've kind of lost track, but I think so. Will take one last look
> at your next revision! Sounds like it may have some interesting new
> features anyway given the other thread!
>>>
>>> ----- snip ------------
>>>> + for (i = 0; i < MAX_DEVICE_REGS; i++) {
>>>> + ret = i2c_smbus_write_byte(clientp,
>>>> + (TSL258X_CMD_REG | (TSL258X_CNTRL +
> i)));
>>>> + if (ret < 0) {
>>>> + dev_err(&clientp->dev, "i2c_smbus_write_byte()
> to cmd "
>>>> + "reg failed in taos_probe(), err =
> %d\n", ret);
>>>> + goto fail1;
>>>> + }
>>>> + buf[i] = i2c_smbus_read_byte(clientp);
>>> error handling for this read?
>>> VALUE IS UNIMPORTANT - BUT MAKING SURE WE CAN JUST READ ALL 32
> REGISTER LOCATIONS IS..
> That's fine, but you haven't verified that if you don't check for a
> negative return from that read.
>
> AGREED - ADDED CHECK FOR NEGATIVE RETURN
>
>>>
>>> ----- snip ------------
>>>
>>>> + }
>>>> + if (!taos_skate_device(buf)) {
>>>> + dev_info(&clientp->dev, "i2c device found but does not
> match "
>>>> + "expected id in taos_probe()\n");
>>>> + goto fail1;
>>>> + } else {
>>> The sort of debug info you want to get rid of for production drivers.
>
>>> Doesn't tell anyone anything helpful.
>>> NO - DO NOT AGREE - IF A CUSTOMER EXPECTS ONE DEVICE BUT HAS SOMEHOW
>>> INSTALLED A DIFFERENT ONE AT PRODUCTION TIME, THIS WILL HELP IDENTIFY
> THE ISSUE.
>>> (IE. WANTED A TAOS ALS DEVICE BUT INSTALLED A TAOS PROX/ALS DEVICE)
>>> FIRST PART INDICATES THAT. SECOND PART IS CONFIRMATION THAT DEVICE
> WAS IDENTIFIED TO SYSTEM.
>>
>> I'm happy with the error one. Just don't see the point in saying
> 'everything is as expected'.
>> The mere absence of the error indicates that just fine!
>> AGREED - REMOVED 2ND PART (THE OK MESSAGE)
> Cool
>
> Coming together nicely.
>
> Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
2011-03-09 19:14 ` Jonathan Cameron
@ 2011-03-12 1:16 ` Jon Brenner
0 siblings, 0 replies; 11+ messages in thread
From: Jon Brenner @ 2011-03-12 1:16 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio; +Cc: linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 46156 bytes --]
Johathan , et al.,
Thank you for taking the time to review.
Please see response (IN ALL CAPS) to various comments inline.
Jon Brenner
-----Original Message-----
From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
Sent: Wednesday, March 09, 2011 1:14 PM
To: Jon Brenner; linux-iio@vger.kernel.org
Cc: Linux Kernel
Subject: Re: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
On 03/09/11 01:32, Jon Brenner wrote:
> From: J. August Brenner <jbrenner@taosinc.com>
>
> This driver supports the TAOS tsl258x family of ALS sensors.
> This driver provides ALS data (lux), and device configuration via
> isysfs/iio.
> More significantly, this driver provides the capability to be fed
> 'glass coefficients' to accommodate varying system designs (bezels,
> faceplates, etc.).
>
Jon,
Please cc all the relevant subsystem lists...(done)
Sorry to say I'm pretty militant about attribute names.
My job is to ensure we end up with a generalizable set, consistent across lots of different sensor types. That consistency does restrict what is acceptable a lot more than would be true if we were only interested in light sensors. That's not say I'm not happy to have futher discussions on these points.
Otherwise, getting there. Various comments inline.
> Signed-off-by: Jon August Brenner <jbrenner@taosinc.com>
>
> ---
>
>>From 5b8364f9828dbad5fbfff96385e3fc2a6a6d56ed Mon Sep 17 00:00:00 2001
> From: Jon Brenner <jbrenner@taosinc.com>
> Date: Tue, 8 Mar 2011 18:37:13 -0600
> Subject: [PATCH] TAOS driver for the tsl258x family of devices.
This should be in the patch body. Guessing output of git format-patch?
It's for use with git send-email and forms the email header.
WILL BE REMOVED FROM NEXT PATCH SUBMISSION - FOLLOWING THIS RESPONSE TO COMMENT
> ---
> Makefile | 2 +-
> .../staging/iio/Documentation/sysfs-bus-iio-light | 63 ++
> drivers/staging/iio/light/Kconfig | 7 +
> drivers/staging/iio/light/Makefile | 1 +
> drivers/staging/iio/light/tsl258x.c | 1010 ++++++++++++++++++++
> 5 files changed, 1082 insertions(+), 1 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 26d7d82..2f7d922 100644
> --- a/Makefile
> +++ b/Makefile
This shouldn't be in a driver patch.
> @@ -1,7 +1,7 @@
> VERSION = 2
> PATCHLEVEL = 6
> SUBLEVEL = 38
> -EXTRAVERSION = -rc6
> +EXTRAVERSION = -rc7
> NAME = Flesh-Eating Bats with Fangs
>
> # *DOCUMENTATION*
> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> index 5d84856..5affc2a 100644
> --- a/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> @@ -62,3 +62,66 @@ Description:
> sensing mode. This value should be the output from a reading
> and if expressed in SI units, should include _input. If this
> value is not in SI units, then it should include _raw.
> +
> +What: /sys/bus/iio/devices/device[n]/device_state
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x power state.
This certainly looks more general than that. It's not light sensor specific so should be in sysfs-bus-iio rather than the light related doc. Having said that it's also a rather non specific description / name.
I've been generally pushing against having this sort of power control as an explicit attribute in the iio abi simply because the right way to handle this is probably runtime power management. We did let a few drivers do this early on simply because they predated runtime PM. That framework has the advantage that it also allows for the bus to be turned off if no devices on it need to be talked to. It may be time to reopen the previous discussion of how to allow userspace to explicitly say it doesn't care if a device is powered up...
MOVED TO SYSFS-BUS-IIO DOC.
CHANGED TO POWER_STATE
NEED ABILITY FROM USERS SPACE TO TURN DEVICE ON/OFF (ALLOWS USERS TO CHANGE MULTIPLE NOMINAL OPS THEN OFF/ON vs OFF/ON FOR EACH CHANGE)
REMOVED "TAOS tsl258X" FROM ALL DOCUMENTATION REFERENCES.
> +
> +What: /sys/bus/iio/devices/device[n]/als_gain
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x analog gain settings
> + of the device.
Again, please get the mention of particular part out of the description.
Although it isn't explicitly specified (yet) for light sensors, we do have an equivalent (I think) of this assuming it is a linear relationship?
(google found me a datasheet so at least superficially I think it is).
That attribute is illuminance_calibscale (calib bit means it is applied on device or in driver rather than telling userspace what needs to be applied by it). (yikes, just noticed that this is wrong in tsl2563 where we have calibgain due to some inconsistency on my part a while back).
DONE - CHANGED NAME TO ILLUMANCE_CALIBSCALE
REMOVED THIS TEXT FROM DOC
Also avoid 'magic' numbers as seen here. There are a finite range of possible values, so have an extra attributes - here illuminance_calibscale_available which is read only and gives the string "1 8 16 111" and make illuminance_calibscale return -EINVAL if the value written is not one of these. sysfs_streq is really handy for this sort of matching.
Ok - HOWEVER, WITH RESPECT TO THIS: ONLY ALLOWED VALUES ARE 0-3 (INDEX RATHER THAN SCALE FACTOR - SINCE WE'RE USING A 2 PART/CHANNEL SCALE)
ADDED ILLUMINANCE_CALIBSCALE_AVAILABLE
> +
> +What: /sys/bus/iio/devices/device[n]/als_time
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x ADC channel integration
> + time in 2.7ms increments.
Again, this is more general than just working for the tsl258x driver. That 2.7ms isn't though. Please make this a value in millisecs (fixed point) then deal with conversion to this base in the driver. On this one, it is closely related to the 'period' attributes that exist for various events. Perhaps illuminance_period is a more consistent name?
CHANGED TO SAMPLING_FREQUENCY
REMOVED FOR LIGHT DOC
ADDED SAMPLING_FREQUENCY_AVAILIABLE
> +
> +What: /sys/bus/iio/devices/device[n]/als_trim
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x ADC active gain scale.
This one needs more detail before I can comment. I can't figure out what it actually is!
CHANGED TO SCALE
REMOVED FROM LIGHT DOC
> +
> +What: /sys/bus/iio/devices/device[n]/als_target
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x ADC last known external
> + lux measurement used in calibration.
als_target -> illuminance0_target
CHANGED TO ILLUMINANCE0_INPUT_TARGET
lux -> illuminance.
What units?
Sounds more general than the tsl258x to me so perhaps say that chip is an example of its use?
> +
> +What: /sys/bus/iio/devices/device[n]/lux
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets the TAOS tsl258x calculated lux value.
Sorry, lux is a unit. Attributes are named after what is being measured. Hence illuminance. Might seem reasonable to use the unit, but then what would we do for acceleration (m/s^2)? I guess we can reopen this debate again if you are really attached to lux... Also needs to be illuminance0_input. The input specifices that it is in the base unit (lux) rather than raw. We stick to this form to maintain compatiblity with hwmon which has been around a lot longer than us!
CHANGED TO ILLUMINANCE0_TARGET
> +
> +What: /sys/bus/iio/devices/device[n]/lux_table
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x lux table of coefficients
> + that are used to calculate lux.
> +
> +What: /sys/bus/iio/devices/device[n]/reg_offset
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets an index the TAOS tsl258x internel register
> + for r/w of selected register.
Sorry, not letting direct register write attributes in. What do you need these for? Can it really not be replaced by more informative attributes?
> +
REMOVED REMOVED REGISTER R/W ATTRIBUTE FUNCTIONS FROM DOC & CODE
> +What: /sys/bus/iio/devices/device[n]/reg_offset
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets an TAOS tsl258x internel register value
> + indexed by reg_offset.
> +
REMOVED ALL REGISTER READ / WRITE ATTRIBUTE ABI FUNCTIONS
Extra empty lines. Please remove. Also please run checkpatch over this patch. I think there are some extra tabs on these lines that won't have gotten through that.
> +
> +
> \ No newline at end of file
> diff --git a/drivers/staging/iio/light/Kconfig
> b/drivers/staging/iio/light/Kconfig
> index 36d8bbe..ae499b8 100644
> --- a/drivers/staging/iio/light/Kconfig
> +++ b/drivers/staging/iio/light/Kconfig
> @@ -13,6 +13,13 @@ config SENSORS_TSL2563
> This driver can also be built as a module. If so, the module
> will be called tsl2563.
>
> +config TAOS_258x
> + tristate "TAOS TSL258x device driver"
> + default m
> + help
> + Provides support for the TAOS tsl258x family of devices.
> + Access ALS data/control via sysfs/iio.
Please list devices. People tend to grep the tree for the part number of the device that they actually have hence it is useful to have them all explicitly listed here. In combination with the id table listing them all this also tends to tell people if the developer thinks it will work with the part they have.
OK - CHANGED TO tsl2581
> +
> config SENSORS_ISL29018
> tristate "ISL 29018 light and proximity sensor"
> depends on I2C
> diff --git a/drivers/staging/iio/light/Makefile
> b/drivers/staging/iio/light/Makefile
> index 9142c0e..4395db8 100644
> --- a/drivers/staging/iio/light/Makefile
> +++ b/drivers/staging/iio/light/Makefile
> @@ -3,4 +3,5 @@
> #
>
> obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
> +obj-$(CONFIG_TAOS_258x) += tsl258x.o
> obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o
> diff --git a/drivers/staging/iio/light/tsl258x.c
> b/drivers/staging/iio/light/tsl258x.c
> new file mode 100644
> index 0000000..225d85b
> --- /dev/null
> +++ b/drivers/staging/iio/light/tsl258x.c
> @@ -0,0 +1,1010 @@
> +/*
> + * Device driver for monitoring ambient light intensity (lux)
> + * within the TAOS tsl258x family of devices
> + *
> + * Copyright (c) 2011, TAOS Corporation.
> + *
> + * 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.
> + *
> + * 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, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/i2c.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/string.h>
> +#include <linux/mutex.h>
> +#include "../iio.h"
> +
> +#define DEVICE_ID "tsl258x"
> +
> +#define MAX_DEVICE_REGS 32
> +
I'm guessing this is an internal part name? If possible please replace with the numbers people will see on datasheets.
CHANGED TO TSL2583
> +/* Triton register offsets */
> +#define TAOS_REG_MAX 8
> +
> +/* Device Registers and Masks */
> +#define TSL258X_CNTRL 0x00
This needs a a comment. Why two regs with the same address? - DONE - REMOVED STATUS
> +#define TSL258X_STATUS 0x00
> +#define TSL258X_ALS_TIME 0X01
> +#define TSL258X_INTERRUPT 0x02
> +#define TSL258X_GAIN 0x07
> +#define TSL258X_REVID 0x11
> +#define TSL258X_CHIPID 0x12
Cryptic name and never used. Please remove. - DONE - REMOVED
> +#define TSL258X_SMB_4 0x13
> +#define TSL258X_ALS_CHAN0LO 0x14
> +#define TSL258X_ALS_CHAN0HI 0x15
> +#define TSL258X_ALS_CHAN1LO 0x16
> +#define TSL258X_ALS_CHAN1HI 0x17
> +#define TSL258X_TMR_LO 0x18
> +#define TSL258X_TMR_HI 0x19
> +
> +/* Skate cmd reg masks */
> +#define TSL258X_CMD_REG 0x80
> +#define TSL258X_CMD_BYTE_RW 0x00
This name confuses me. Looks like it sets word protocol so why the block bit? - DONE REMOVED BLK
Also not used so could just get rid of it.
> +#define TSL258X_CMD_WORD_BLK_RW 0x20
> +#define TSL258X_CMD_SPL_FN 0x60
Nitpick. Later reference to INT have an _ after them. Perhaps add one for consistency? - DONE
> +#define TSL258X_CMD_ALS_INTCLR 0X01
> +
Err. Skate?
> +/* Skate cntrl reg masks */
Not used and rather pointless. Default would be to assume writing 0 cleared a register? (or does this mean something else?) - DONE
> +#define TSL258X_CNTL_REG_CLEAR 0x00
> +#define TSL258X_CNTL_ALS_INT_ENBL 0x10
> +#define TSL258X_CNTL_WAIT_TMR_ENBL 0x08
> +#define TSL258X_CNTL_ADC_ENBL 0x02
> +#define TSL258X_CNTL_PWRON 0x01
> +#define TSL258X_CNTL_ALSPON_ENBL 0x03
Define this in terms of PWRON and ADC_ENBL to make it clear what it is. - CHANGED NAME PWR_ON (POWER ON VS ENABLING THE ADC ARE DIFFERENT)
> +#define TSL258X_CNTL_INTALSPON_ENBL 0x13 - REMOVED
also define this in terms of its sub parts.
> +
> +/* Skate status reg masks */
> +#define TSL258X_STA_ADCVALID 0x01
> +#define TSL258X_STA_ALSINTR 0x10
> +#define TSL258X_STA_ADCINTR 0x10
> +
> +/* Lux constants */
> +#define MAX_LUX 65535 -DONE NAME CHANGED
Err. Rename that unless it really does mean 65535 lux
> +
> +enum {
> + TAOS_CHIP_UNKNOWN = 0, TAOS_CHIP_WORKING = 1, TAOS_CHIP_SLEEP = 2 }
> +TAOS_CHIP_WORKING_STATUS;
> +
> +/* Per-device data */
> +struct taos_als_info {
> + u16 als_ch0;
> + u16 als_ch1;
> + u16 lux;
> +};
> +
> +struct taos_settings {
> + int als_time;
> + int als_gain;
> + int als_gain_trim;
> + int als_cal_target;
> +};
> +
> +struct tsl258x_chip {
> + struct mutex als_mutex;
> + struct i2c_client *client;
> + struct iio_dev *iio_dev;
> + struct delayed_work update_lux;
> + unsigned int addr;
> + char taos_id;
> + char valid;
> + unsigned long last_updated;
> + struct taos_als_info als_cur_info;
> + struct taos_settings taos_settings;
> + int als_time_scale;
> + int als_saturation;
> + int taos_chip_status;
> + u8 taos_config[8];
> +};
> +
> +static int taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val,
> + unsigned int len);
> +static int taos_i2c_write_command(struct i2c_client *client, u8 reg);
> +
> +/*
> + * Initial values for device - this values can/will be changed by driver.
> + * and applications as needed.
> + * These values are dynamic.
> + */
> +static const u8 taos_config[8] = {
> + 0x00, 0xee, 0x00, 0x03, 0x00, 0xFF, 0xFF, 0x00
> +}; /* cntrl atime intC Athl0 Athl1 Athh0 Athh1 gain */
> +
> +struct taos_lux {
> + unsigned int ratio;
> + unsigned int ch0;
> + unsigned int ch1;
> +};
> +
> +/* This structure is intentionally large to accommodate updates via
> +sysfs. */
> +/* Sized to 11 = max 10 segments + 1 termination segment */
Any chance of two sensors on one device, each of which has different values for this? - NO , LUX TABLE IS FOR GLASS NOT DEVICE
> +struct taos_lux taos_device_lux[11] = {
> + { 9830, 8520, 15729 },
> + { 12452, 10807, 23344 },
> + { 14746, 6383, 11705 },
> + { 17695, 4063, 6554 },
> +};
> +
> +struct taos_lux taos_lux;
> +
> +struct gainadj {
> + s16 ch0;
> + s16 ch1;
> +};
> +
> +/* Index = (0 - 3) Used to validate the gain selection index */
> +static const struct gainadj gainadj[] = {
> + { 1, 1 },
> + { 8, 8 },
> + { 16, 16 },
That's 'interesting'. This will make the calibscale discussion above more complex. I guess no userspace is going to care about the precise internal multipliers so a 'rough' value would probably do in that attribute. What do you think?
THIS ACCOMMODATES A DIFFERENTIAL CHARACTERISTIC (IN THIS FAMILY OF DEVICE) BETWEEN ADC CHANNELS AT HIGHER GAIN. THIS IS AN INTERNAL/DRIVER CALCULATION. USERSPACE SIMPLY SELECTS GAIN INDEX 0 - 3.
> + { 107, 115 }
> +};
> +
> +/*
> + * Provides initial operational parameter defaults.
> + * These defaults may be changed through the device's sysfs files.
> + */
> +static void taos_defaults(struct tsl258x_chip *chip) {
> + /* Operational parameters */
> + chip->taos_settings.als_time = 450;
> + /* must be a multiple of 50mS */
> + chip->taos_settings.als_gain = 2;
> + /* this is actually an index into the gain table */
> + /* assume clear glass as default */
> + chip->taos_settings.als_gain_trim = 1000;
> + /* default gain trim to account for aperture effects */
> + chip->taos_settings.als_cal_target = 130;
> + /* Known external ALS reading used for calibration */
> +
> + /* Initialize ALS data to defaults */
> + chip->als_cur_info.als_ch0 = 0;
> + chip->als_cur_info.als_ch1 = 0;
> + chip->als_cur_info.lux = 0;
Already zero via the memset (soon to be kzalloc) hence don't bother setting these three. - DONE
> +}
> +
> +/*
> + * Read a number of bytes starting at register (reg) location.
> + * Return 0, or i2c_smbus_write_byte ERROR code.
> + */
> +static int
> +taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned
> +int len) {
> + int ret;
> + int i;
> +
> + for (i = 0; i < len; i++) {
> + /* select register to write */
> + ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | reg));
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_i2c_read failed to write"
> + " register %x\n", reg);
> + return ret;
> + }
> + /* read the data */
> + *val = i2c_smbus_read_byte(client);
I'd use val[i] without this increment for clarity.
> + val++;
> + if ((reg & 0x1f) == 0x1f)
Can this ever occcur in the driver? I would imagine it is a bug if it does as you'll read back a different number of values than you expect to do. - DONE REMOVED
If not it's an over enthusiastic bit of debug code so get rid of it.
> + break;
> + reg++;
> + }
> + return 0;
> +}
> +
> +/*
> + * This function is used to send a command to device command/control
> +register
> + * All bytes sent using this command have their MSBit set - it's a command!
> + * Return 0, or i2c_smbus_write_byte error code.
> + */
> +static int taos_i2c_write_command(struct i2c_client *client, u8 reg)
> +{
> + int ret;
> +
> + /* write the data */
> + ret = i2c_smbus_write_byte(client, (reg |= TSL258X_CMD_REG));
> + if (ret < 0) {
> + dev_err(&client->dev, "FAILED: i2c_smbus_write_byte\n");
> + return ret;
> + }
> + return 0;
> +}
> +
> +/*
> + * Reads and calculates current lux value.
> + * The raw ch0 and ch1 values of the ambient light sensed in the last
> + * integration cycle are read from the device.
> + * Time scale factor array values are adjusted based on the integration time.
> + * The raw values are multiplied by a scale factor, and device gain
> +is obtained
> + * using gain index. Limit checks are done next, then the ratio of a
> +multiple
> + * of ch1 value, to the ch0 value, is calculated. The array
> +taos_device_lux[]
> + * declared above is then scanned to find the first ratio value that
> +is just
> + * above the ratio we just calculated. The ch0 and ch1 multiplier
> +constants in
> + * the array are then used along with the time scale factor array
> +values, to
> + * calculate the lux.
> + */
> +static int taos_get_lux(struct i2c_client *client) {
> + u32 ch0, ch1; /* separated ch0/ch1 data from device */
> + u32 lux; /* raw lux calculated from device data */
And here we have a great example of why lux is a bad name for this.
lux could only ever have units of lux. OK - GOT YOUR POINT
> + u32 ratio;
> + u8 buf[5];
> + struct taos_lux *p;
> + struct tsl258x_chip *chip = i2c_get_clientdata(client);
> + int i, ret;
> + u32 ch0lux = 0;
> + u32 ch1lux = 0;
> +
> + if (mutex_trylock(&chip->als_mutex) == 0) {
> + dev_info(&client->dev, "taos_get_lux device is busy\n");
> + return chip->als_cur_info.lux; /* busy, so return LAST VALUE */
> + }
> +
> + if (chip->taos_chip_status != TAOS_CHIP_WORKING) {
> + /* device is not enabled */
> + dev_err(&client->dev, "taos_get_lux device is not enabled\n");
> + ret = -ENODEV;
-EBUSY probably. The device exists, it's just turned off. - DONE CHANGED
> + goto out_unlock;
> + }
> +
> + ret = taos_i2c_read(client, (TSL258X_CMD_REG), &buf[0], 1);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_get_lux failed to read CMD_REG\n");
> + goto out_unlock;
> + }
> + /* is data new & valid */
> + if (!(buf[0] & TSL258X_STA_ADCINTR)) {
> + dev_err(&client->dev, "taos_get_lux data not valid\n");
I'd error out at this point. Something has gone wrong and you want to indicate it to your userspace code. - NO: ADC MAY JUST NOT HAVE FINISHED. IF USERSPACE IS POLLING BETTER TO PASS BACK PREVIOUS VALUE THAN TO ERROR
- BASED ON CUSTOMER FEEDBACK
> + ret = chip->als_cur_info.lux; /* return LAST VALUE */
> + goto out_unlock;
> + }
> +
> + for (i = 0; i < 4; i++) {
> + int reg = TSL258X_CMD_REG | (TSL258X_ALS_CHAN0LO + i);
> + ret = taos_i2c_read(client, reg, &buf[i], 1);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_get_lux failed to read"
> + " register %x\n", reg);
> + goto out_unlock;
> + }
> + }
> +
> + /* clear status, really interrupt status (interrupts are off), but
> + * we use the bit anyway */
> + ret = taos_i2c_write_command(client,
> + TSL258X_CMD_REG | TSL258X_CMD_SPL_FN | TSL258X_CMD_ALS_INTCLR);
> + if (ret < 0) {
> + dev_err(&client->dev,
This is the one case where checkpatch warnings should be ignored.
Please keep strings like this on a single line as people who see them in a log will grep for them in the source code. Artificial breaks like this make them a lot harder to find! - DONE
> + "taos_i2c_write_command failed in "
> + "taos_get_lux, err = %d\n", ret);
> + goto out_unlock; /* have no data, so return failure */
> + }
> +
> + /* extract ALS/lux data */
Should be the relevant endian conversion function. be16tocpu or similar (haven't thought about which). That way it's free on one endianness of machine.
> + ch0 = (buf[1] << 8) | buf[0];
> + ch1 = (buf[3] << 8) | buf[2];
> +
> + chip->als_cur_info.als_ch0 = ch0;
> + chip->als_cur_info.als_ch1 = ch1;
> +
> + if ((ch0 >= chip->als_saturation) || (ch1 >= chip->als_saturation))
> + goto return_max;
> +
> + if (ch0 == 0) {
> + /* have no data, so return LAST VALUE */
> + ret = chip->als_cur_info.lux = 0;
> + goto out_unlock;
> + }
> + /* calculate ratio */
> + ratio = (ch1 << 15) / ch0;
> + /* convert to unscaled lux using the pointer to the table */
> + for (p = (struct taos_lux *) taos_device_lux;
> + p->ratio != 0 && p->ratio < ratio; p++)
> + ;
> +
> + if (p->ratio == 0) {
> + lux = 0;
> + } else {
> + ch0lux = ((ch0 * p->ch0) +
> + (gainadj[chip->taos_settings.als_gain].ch0 >> 1))
> + / gainadj[chip->taos_settings.als_gain].ch0;
> + ch1lux = ((ch1 * p->ch1) +
> + (gainadj[chip->taos_settings.als_gain].ch1 >> 1))
> + / gainadj[chip->taos_settings.als_gain].ch1;
> + lux = ch0lux - ch1lux;
> + }
> +
> + /* note: lux is 31 bit max at this point */
> + if (ch1lux > ch0lux) {
> + dev_dbg(&client->dev, "No Data - Return last value\n");
Again, do we want userspace to know the sensor isn't returning valid values? - NO - NOT NECESSARLY - JUST MEANS CH1 (IR) WENT HIGHER THAN CH0 (VISABLE)
> + ret = chip->als_cur_info.lux = 0;
> + goto out_unlock;
> + }
> +
> + /* adjust for active time scale */
> + if (chip->als_time_scale == 0)
> + lux = 0;
> + else
> + lux = (lux + (chip->als_time_scale >> 1)) /
> + chip->als_time_scale;
> +
> + /* adjust for active gain scale */
> + lux >>= 13; /* tables have factor of 8192 builtin for accuracy */
> + lux = (lux * chip->taos_settings.als_gain_trim + 500) / 1000;
> + if (lux > MAX_LUX) { /* check for overflow */
> +return_max:
> + lux = MAX_LUX;
Really the right thing to do? Surely you want to return an out of range error? Perhaps ERANGE so userspace knows the value is garbage. - NO - COULD BE DUE TO SATURATION - IN WHICH CASE USERSPACE SOULD KNOW IT'S AT LIMIT.
> + }
> +
> + /* Update the structure with the latest VALID lux. */
> + chip->als_cur_info.lux = lux;
> + ret = lux;
> +
> +out_unlock:
> + mutex_unlock(&chip->als_mutex);
> + return ret;
> +}
> +
> +/*
> + * Obtain single reading and calculate the als_gain_trim (later used
> + * to derive actual lux).
> + * Return updated gain_trim value.
> + */
> +int taos_als_calibrate(struct i2c_client *client) {
> + struct tsl258x_chip *chip = i2c_get_clientdata(client);
> + u8 reg_val;
> + unsigned int gain_trim_val;
> + int ret;
> + int lux_val;
> +
> + ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | TSL258X_CNTRL));
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_als_calibrate failed to reach the"
Again, don't break this sort of string up however much checkpatch rants at you. - OK - JUST REMEMBER YOU SAID SO
> + " CNTRL register, ret=%d\n", ret);
> + return ret;
> + }
> +
> + reg_val = i2c_smbus_read_byte(client);
> + if ((reg_val & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWRON))
> + != (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWRON)) {
> + dev_err(&client->dev, "taos_als_calibrate failed because the"
> + " device is not powered on with ADC enabled\n");
> + return -ENODATA;
> + }
> +
> + ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | TSL258X_STATUS));
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_als_calibrate failed to reach the"
> + " STATUS register, ret=%d\n", ret);
> + return ret;
> + }
> + reg_val = i2c_smbus_read_byte(client);
> +
> + if ((reg_val & TSL258X_STA_ADCVALID) != TSL258X_STA_ADCVALID) {
> + dev_err(&client->dev, "taos_als_calibrate failed because the"
> + " STATUS did not indicate ADC valid.\n");
> + return -ENODATA;
> + }
> + lux_val = taos_get_lux(client);
> + if (lux_val < 0) {
> + dev_err(&client->dev, "taos_als_calibrate failed to get lux\n");
> + return lux_val;
> + }
> + gain_trim_val = (unsigned int) (((chip->taos_settings.als_cal_target)
> + * chip->taos_settings.als_gain_trim) / lux_val);
> +
> + dev_info(&client->dev, "taos_settings.als_cal_target = %d\n"
> + "taos_settings.als_gain_trim = %d\nlux_val = %d\n",
> + chip->taos_settings.als_cal_target,
> + chip->taos_settings.als_gain_trim,
> + lux_val);
> +
> + if ((gain_trim_val < 250) || (gain_trim_val > 4000)) {
> + dev_err(&client->dev, "taos_als_calibrate failed because"
> + "trim_val of %d is out of range\n", gain_trim_val);
> + return -ENODATA;
> + }
> + chip->taos_settings.als_gain_trim = (int) gain_trim_val;
> +
> + return (int) gain_trim_val;
> +}
> +
> +/*
> + * Turn the device on.
> + * Configuration must be set before calling this function.
> + */
> +static int taos_chip_on(struct i2c_client *client) {
> + int i;
> + int ret = 0;
> + u8 *uP;
> + u8 utmp;
> + int als_count;
> + int als_time;
> + struct tsl258x_chip *chip = i2c_get_clientdata(client);
> +
and?
> + /* and make sure we're not already on */
> + if (chip->taos_chip_status == TAOS_CHIP_WORKING) {
> + /* if forcing a register update - turn off, then on */
> + dev_info(&client->dev, "device is already enabled\n");
Not sure what the right error is here, but should be more specific than that. - OK - BUT MESSAGE TO INDICATE IT, USERSPACE COULD HAVE BEEN TRYING TO TURN IT ON FOR A NUMBER OF REASONS
> + return -1;
> + }
> +
> + /* determine als integration regster */
> + als_count = (chip->taos_settings.als_time * 100 + 135) / 270;
> + if (als_count == 0)
> + als_count = 1; /* ensure at least one cycle */
> +
> +
> + /* convert back to time (encompasses overrides) */
> + als_time = (als_count * 27 + 5) / 10;
> + chip->taos_config[TSL258X_ALS_TIME] = 256 - als_count;
> +
> +
Bonus blank lines. One is plenty to break up code. - FIXED
> + /* Set the gain based on taos_settings struct */
> + chip->taos_config[TSL258X_GAIN] = chip->taos_settings.als_gain;
> +
> +
> + /* set globals re scaling and saturation */
They aren't globals - CORRECT CHANGED COMMENT
> + chip->als_saturation = als_count * 922; /* 90% of full scale */
> + chip->als_time_scale = (als_time + 25) / 50;
> +
> + /* SKATE Specific power-on / adc enable sequence
> + * Power on the device 1st. */
> + utmp = TSL258X_CNTL_PWRON;
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
> + utmp);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_chip_on failed on CNTRL reg.\n");
> + return -1;
> + }
> +
> + /* Use the following shadow copy for our delay before enabling ADC.
> + * Write all the registers. */
> + for (i = 0, uP = chip->taos_config; i < TAOS_REG_MAX; i++) {
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG + i,
> + *uP++);
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "taos_chip_on failed on reg %d.\n", i);
> + return -1;
> + }
> + }
> +
> + mdelay(3);
> + /* NOW enable the ADC
> + * initialize the desired mode of operation */
> + utmp = TSL258X_CNTL_PWRON | TSL258X_CNTL_ADC_ENBL;
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
> + utmp);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_chip_on failed on 2nd CTRL reg.\n");
> + return -1;
> + }
> + chip->taos_chip_status = TAOS_CHIP_WORKING;
Comment is rather obvious... - DONE REMOVED COMMENT
> + return ret; /* returns result of last i2cwrite */ }
> +
> +/* Turn the device OFF. */
> +static int taos_chip_off(struct i2c_client *client) {
> + struct tsl258x_chip *chip = i2c_get_clientdata(client);
> + int ret;
> + u8 utmp;
> +
> + /* turn device off */
> + chip->taos_chip_status = TAOS_CHIP_SLEEP;
> + utmp = 0x00;
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
> + utmp);
> + return ret;
> +}
> +
> +/* Sysfs Interface Functions */
> +static ssize_t taos_device_id(struct device *dev,
> + struct device_attribute *attr, char *buf) {
> + return sprintf(buf, "%s\n", DEVICE_ID); }
> +
> +static ssize_t taos_power_state_show(struct device *dev,
> + struct device_attribute *attr, char *buf) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_chip_status); }
> +
> +static ssize_t taos_power_state_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value == 0)
> + taos_chip_off(chip->client);
> + else
> + taos_chip_on(chip->client);
> +
> + return len;
> +}
> +
> +static ssize_t taos_gain_show(struct device *dev,
> + struct device_attribute *attr, char *buf) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_gain); }
> +
> +static ssize_t taos_gain_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> + if (value) {
> + if (value > 4) {
> + dev_err(dev, "Invalid Gain Index\n");
> + return -1;
EINVAL
> + } else {
> + chip->taos_settings.als_gain = value;
> + }
So this sets the value in the local cache. When does it get written to the device? - EVERYTIME CHIP_ON IS CALLED
> + }
> + return len;
> +}
> +
> +static ssize_t taos_als_time_show(struct device *dev,
> + struct device_attribute *attr, char *buf) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_time); }
> +
> +static ssize_t taos_als_time_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value)
> + chip->taos_settings.als_time = value;
else? It's not been sucessfuly set so return -EINVAL - DONE
> +
> + return len;
> +}
> +
> +static ssize_t taos_als_trim_show(struct device *dev,
> + struct device_attribute *attr, char *buf) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_gain_trim);
> +}
> +
> +static ssize_t taos_als_trim_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value)
> + chip->taos_settings.als_gain_trim = value;
else return -EINVAL - UNLESS I'M MISSING SOMETHING, TAKEN CARE IN IN 'IF' ABOVE
> +
> + return len;
> +}
> +
> +static ssize_t taos_als_cal_target_show(struct device *dev,
> + struct device_attribute *attr, char *buf) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_cal_target);
> +}
> +
> +static ssize_t taos_als_cal_target_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value)
> + chip->taos_settings.als_cal_target = value;
again, else return -EINVAL; Userspace really wants to know this failed. - TAKEN CARE IN IN 'IF' ABOVE
> +
> + return len;
> +}
> +
> +static ssize_t taos_lux_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + int lux = 0;
no need to assign. - DONE
> +
> + lux = taos_get_lux(chip->client);
> +
> + return sprintf(buf, "%d\n", lux);
> +}
> +
> +static ssize_t taos_do_calibrate(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value == 1)
> + taos_als_calibrate(chip->client);
else return -EINVAL; - TAKEN CARE IN IN 'IF' ABOVE
> +
> + return len;
> +}
> +
> +static ssize_t taos_luxtable_show(struct device *dev,
> + struct device_attribute *attr, char *buf) {
> + int i;
> + int offset = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(taos_device_lux); i++) {
> + offset += sprintf(buf + offset, "%d,%d,%d,",
> + taos_device_lux[i].ratio,
> + taos_device_lux[i].ch0,
> + taos_device_lux[i].ch1);
> + if (taos_device_lux[i].ratio == 0) {
> + /* We just printed the first "0" entry.
> + * Now get rid of the extra "," and break. */
> + offset--;
> + break;
> + }
> + }
> +
> + offset += sprintf(buf + offset, "\n");
> + return offset;
> +}
> +
> +static ssize_t taos_luxtable_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data; #define
> +MAX_LUX_TABLE_FIELDS 33 - DONE
I'd loose this define.
> + int value[MAX_LUX_TABLE_FIELDS];
> + int n;
> +
> + get_options(buf, ARRAY_SIZE(value), value);
> +
> + /* We now have an array of ints starting at value[1], and
> + * enumerated by value[0].
> + * We expect each group of three ints is one table entry,
> + * and the last table entry is all 0.
> + */
> + n = value[0];
prefer to see ARRAY_SIZE(value) than the define. - AGREED - DONE
> + if ((n % 3) || n < 6 || n > (MAX_LUX_TABLE_FIELDS - 3)) {
> + dev_info(dev, "LUX TABLE INPUT ERROR 1 Value[0]=%d\n", n);
> + return -EINVAL;
> + }
> + if ((value[(n - 2)] | value[(n - 1)] | value[n]) != 0) {
> + dev_info(dev, "LUX TABLE INPUT ERROR 2 Value[0]=%d\n", n);
> + return -EINVAL;
> + }
> +
> + if (chip->taos_chip_status == TAOS_CHIP_WORKING)
> + taos_chip_off(chip->client);
> +
> + /* Zero out the table */
> + memset(taos_device_lux, 0, sizeof(taos_device_lux));
> + memcpy(taos_device_lux, &value[1], (value[0] * 4));
> +
> + taos_chip_on(chip->client);
> +
> + return len;
> +}
> +
> +static u8 reg_index;
> +
As stated above these need to go before we take this driver. - DONE - BUT THESE WERE NEEDED BY USERSPACE APP FOR CREATING NEW LUX EQUATION COEFFs PRIOR TO PRODUCTION
NEVER THE LESS - GONE.
> +/* Sets a pointer to a register for R/W via sysfs */ static ssize_t
> +taos_reg_offset_show(struct device *dev,
> + struct device_attribute *attr, char *buf) {
> + return sprintf(buf, "%d\n", reg_index);
> + return 0;
> +}
> +
> +static ssize_t taos_reg_offset_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len) {
> + unsigned long value;
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value > MAX_DEVICE_REGS) {
> + dev_err(dev, "register offset exceeds MAX\n");
> + return -1;
> + } else {
> + reg_index = value;
> + }
> +return len;
> +}
> +
> +/* R/W a register for R/W via sysfs */ static ssize_t
> +taos_reg_show(struct device *dev,
> + struct device_attribute *attr, char *buf) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + u8 value = 0;
> + int ret = 0;
> +
> + ret = i2c_smbus_write_byte(chip->client,
> + (TSL258X_CMD_REG | reg_index));
> + if (ret < 0) {
> + dev_err(dev, "i2c_smbus_write_byte to cmd reg failed "
> + "in taos_reg_offset_show(), err = %d\n", ret);
> + return ret;
> + }
> + value = i2c_smbus_read_byte(chip->client);
> + return sprintf(buf, "%d\n", value);
> +
> +return 0;
> +}
> +
> +static ssize_t taos_reg_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len) {
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + unsigned long value = 0;
> + int ret = 0;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + ret = i2c_smbus_write_byte_data(chip->client,
> + (TSL258X_CMD_REG | reg_index), value);
> + if (ret < 0) {
> + dev_err(dev, "i2c_smbus_write_byte_data failed in "
> + "taos_reg_store()\n");
> + return ret;
> + }
> +
> +return len;
> +}
> +
> +static DEVICE_ATTR(name, S_IRUGO, taos_device_id, NULL); static
> +DEVICE_ATTR(device_state, S_IRUGO | S_IWUSR,
> + taos_power_state_show, taos_power_state_store); static
> +DEVICE_ATTR(als_gain, S_IRUGO | S_IWUSR,
> + taos_gain_show, taos_gain_store);
> +static DEVICE_ATTR(als_time, S_IRUGO | S_IWUSR,
> + taos_als_time_show, taos_als_time_store); static
> +DEVICE_ATTR(als_trim, S_IRUGO | S_IWUSR,
> + taos_als_trim_show, taos_als_trim_store); static
> +DEVICE_ATTR(als_target, S_IRUGO | S_IWUSR,
> + taos_als_cal_target_show, taos_als_cal_target_store); static
> +DEVICE_ATTR(lux, S_IRUGO, taos_lux_show, NULL); static
> +DEVICE_ATTR(calibrate, S_IWUSR, NULL, taos_do_calibrate); static
> +DEVICE_ATTR(lux_table, S_IRUGO | S_IWUSR,
> + taos_luxtable_show, taos_luxtable_store); static
> +DEVICE_ATTR(reg_offset, S_IRUGO | S_IWUSR,
> + taos_reg_offset_show,
> + taos_reg_offset_store);
> +static DEVICE_ATTR(reg, S_IRUGO | S_IWUSR,
> + taos_reg_show,
> + taos_reg_store);
> +
> +
> +static struct attribute *sysfs_attrs_ctrl[] = {
> + &dev_attr_name.attr,
> + &dev_attr_device_state.attr,
> + &dev_attr_als_gain.attr,
> + &dev_attr_als_time.attr,
> + &dev_attr_als_trim.attr,
> + &dev_attr_als_target.attr,
> + &dev_attr_lux.attr,
> + &dev_attr_calibrate.attr,
> + &dev_attr_lux_table.attr,
Spaces instead of tab here.
> + &dev_attr_reg_offset.attr,
> + &dev_attr_reg.attr,
> + NULL
> +};
> +
> +static struct attribute_group tsl258x_attribute_group = {
> + .attrs = sysfs_attrs_ctrl,
> +};
> +
> +/* Use the default register values to identify the Taos device */
> +static int taos_skate_device(unsigned char *bufp) {
> + if (((bufp[TSL258X_CHIPID] & 0xf0) == 0x90))
> + /* tsl258x */
> + return 1;
> + /* else unknown */
> + return 0;
> +}
> +
> +/*
> + * Client probe function - When a valid device is found, the driver's
> +device
> + * data structure is updated, and initialization completes successfully.
> + */
> +static int __devinit taos_probe(struct i2c_client *clientp,
> + const struct i2c_device_id *idp) {
> + int i, ret = 0;
> + unsigned char buf[MAX_DEVICE_REGS];
> + static struct tsl258x_chip *chip;
> +
> + if (!i2c_check_functionality(clientp->adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA)) {
> + dev_err(&clientp->dev,
> + "taos_probe() - i2c smbus byte data "
> + "functions unsupported\n");
> + return -EOPNOTSUPP;
> + }
> + if (!i2c_check_functionality(clientp->adapter,
> + I2C_FUNC_SMBUS_WORD_DATA)) {
> + dev_warn(&clientp->dev,
> + "taos_probe() - i2c smbus word data "
> + "functions unsupported\n");
> + }
> + if (!i2c_check_functionality(clientp->adapter,
> + I2C_FUNC_SMBUS_BLOCK_DATA)) {
> + dev_err(&clientp->dev,
> + "taos_probe() - i2c smbus block data "
> + "functions unsupported\n");
> + }
> +
> + chip = kmalloc(sizeof(struct tsl258x_chip), GFP_KERNEL);
> + if (!chip) {
> + dev_err(&clientp->dev, "taos_device_drvr: kmalloc for "
> + "struct tsl258x_chip failed in taos_probe()\n");
> + return -ENOMEM;
> + }
> + memset(chip, 0, sizeof(struct tsl258x_chip));
Use kzalloc instead of kmalloc then you don't need the memset. - GREAT - DONE
> + chip->client = clientp;
> + i2c_set_clientdata(clientp, chip);
> +
> + mutex_init(&chip->als_mutex);
> + chip->taos_chip_status = TAOS_CHIP_UNKNOWN; /* uninitialized to start */
> + memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
> +
> + for (i = 0; i < MAX_DEVICE_REGS; i++) {
> + ret = i2c_smbus_write_byte(clientp,
> + (TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
> + if (ret < 0) {
> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd "
> + "reg failed in taos_probe(), err = %d\n", ret);
> + goto fail1;
> + }
> + buf[i] = i2c_smbus_read_byte(clientp);
> + }
> + if (!taos_skate_device(buf)) {
> + dev_info(&clientp->dev, "i2c device found but does not match "
> + "expected id in taos_probe()\n");
> + goto fail1;
> + } else {
> + dev_info(&clientp->dev, "i2c device match in probe\n");
> + }
> + ret = i2c_smbus_write_byte(clientp, (TSL258X_CMD_REG | TSL258X_CNTRL));
> + if (ret < 0) {
> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd reg "
> + "failed in taos_probe(), err = %d\n", ret);
> + goto fail1;
> + }
> + chip->valid = 0;
> +
> + chip->iio_dev = iio_allocate_device();
> + if (!chip->iio_dev) {
> + ret = -ENOMEM;
> + dev_err(&clientp->dev, "iio allocation failed\n");
> + goto fail1;
> + }
> +
> + chip->iio_dev->attrs = &tsl258x_attribute_group;
> + chip->iio_dev->dev.parent = &clientp->dev;
> + chip->iio_dev->dev_data = (void *)(chip);
> + chip->iio_dev->driver_module = THIS_MODULE;
> + chip->iio_dev->modes = INDIO_DIRECT_MODE;
> + ret = iio_device_register(chip->iio_dev);
> + if (ret) {
> + dev_err(&clientp->dev, "iio registration failed\n");
> + goto fail1;
> + }
> +
> + /* Load up the V2 defaults (these are hard coded defaults for now) */
> + taos_defaults(chip);
> +
> + /* Make sure the chip is on */
> + taos_chip_on(clientp);
> +
> + dev_info(&clientp->dev, "Light sensor found.\n");
> + return 0;
> +
> +fail1:
> + kfree(chip);
> +
> + return ret;
> +}
> +
> +static int __devexit taos_remove(struct i2c_client *client) {
> + struct tsl258x_chip *chip = i2c_get_clientdata(client);
> +
> + iio_device_unregister(chip->iio_dev);
> +
> + kfree(chip);
> + return 0;
> +}
> +
> +static struct i2c_device_id taos_idtable[] = {
> + { DEVICE_ID, 0 },
> + {}
Please can we have an explicit list if ID's here. If a person has a tsl2580 then they'll want to say that in their board file. - DONE INCLUDES ALL FAMILY DEVICES
On that note, I skipped this originally as there were bigger fish to fry, but can we be sure that there will never be a value of x that this driver won't work for? If not, please follow convention and pick your favourite part number from the range and replace all x's in the driver appropriately. Too many part manufacturers have really weird naming schemes that tend to break wild cards like this!
DONE - DRIVER NAME TO TSL2583 - ALSO SUPPORTS 2580,81 - AS STATED IN KCONFIG HELP
> +};
> +MODULE_DEVICE_TABLE(i2c, taos_idtable);
> +
> +/* Driver definition */
> +static struct i2c_driver taos_driver = {
> + .driver = {
> + .name = "tsl258x",
> + },
> + .id_table = taos_idtable,
> + .probe = taos_probe,
> + .remove = __devexit_p(taos_remove),
> +};
> +
> +static int __init taos_init(void)
> +{
> + return i2c_add_driver(&taos_driver); }
> +
> +static void __exit taos_exit(void)
> +{
> + i2c_del_driver(&taos_driver);
> +}
> +
> +module_init(taos_init);
> +module_exit(taos_exit);
> +
> +MODULE_AUTHOR("J. August Brenner<jbrenner@taosinc.com>");
> +MODULE_DESCRIPTION("TAOS tsl258x ambient light sensor driver");
> +MODULE_LICENSE("GPL");
> +
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
2011-03-09 1:32 Jon Brenner
@ 2011-03-09 19:14 ` Jonathan Cameron
2011-03-12 1:16 ` Jon Brenner
0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2011-03-09 19:14 UTC (permalink / raw)
To: Jon Brenner, linux-iio; +Cc: Linux Kernel
On 03/09/11 01:32, Jon Brenner wrote:
> From: J. August Brenner <jbrenner@taosinc.com>
>
> This driver supports the TAOS tsl258x family of ALS sensors.
> This driver provides ALS data (lux), and device configuration via
> isysfs/iio.
> More significantly, this driver provides the capability to be fed 'glass
> coefficients' to accommodate varying system designs (bezels, faceplates,
> etc.).
>
Jon,
Please cc all the relevant subsystem lists...(done)
Sorry to say I'm pretty militant about attribute names.
My job is to ensure we end up with a generalizable set,
consistent across lots of different sensor types. That
consistency does restrict what is acceptable a lot more
than would be true if we were only interested in light
sensors. That's not say I'm not happy to have futher
discussions on these points.
Otherwise, getting there. Various comments inline.
> Signed-off-by: Jon August Brenner <jbrenner@taosinc.com>
>
> ---
>
>>From 5b8364f9828dbad5fbfff96385e3fc2a6a6d56ed Mon Sep 17 00:00:00 2001
> From: Jon Brenner <jbrenner@taosinc.com>
> Date: Tue, 8 Mar 2011 18:37:13 -0600
> Subject: [PATCH] TAOS driver for the tsl258x family of devices.
This should be in the patch body. Guessing output of git format-patch?
It's for use with git send-email and forms the email header.
> ---
> Makefile | 2 +-
> .../staging/iio/Documentation/sysfs-bus-iio-light | 63 ++
> drivers/staging/iio/light/Kconfig | 7 +
> drivers/staging/iio/light/Makefile | 1 +
> drivers/staging/iio/light/tsl258x.c | 1010 ++++++++++++++++++++
> 5 files changed, 1082 insertions(+), 1 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 26d7d82..2f7d922 100644
> --- a/Makefile
> +++ b/Makefile
This shouldn't be in a driver patch.
> @@ -1,7 +1,7 @@
> VERSION = 2
> PATCHLEVEL = 6
> SUBLEVEL = 38
> -EXTRAVERSION = -rc6
> +EXTRAVERSION = -rc7
> NAME = Flesh-Eating Bats with Fangs
>
> # *DOCUMENTATION*
> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> index 5d84856..5affc2a 100644
> --- a/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> @@ -62,3 +62,66 @@ Description:
> sensing mode. This value should be the output from a reading
> and if expressed in SI units, should include _input. If this
> value is not in SI units, then it should include _raw.
> +
> +What: /sys/bus/iio/devices/device[n]/device_state
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x power state.
This certainly looks more general than that. It's not light sensor specific
so should be in sysfs-bus-iio rather than the light related doc. Having said
that it's also a rather non specific description / name.
I've been generally pushing against having this sort of power control as
an explicit attribute in the iio abi simply because the right way to handle
this is probably runtime power management. We did let a few drivers do
this early on simply because they predated runtime PM. That framework has
the advantage that it also allows for the bus to be turned off if no
devices on it need to be talked to. It may be time to reopen the previous
discussion of how to allow userspace to explicitly say it doesn't care
if a device is powered up...
> +
> +What: /sys/bus/iio/devices/device[n]/als_gain
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x analog gain settings
> + of the device.
Again, please get the mention of particular part out of the description.
Although it isn't explicitly specified (yet) for light sensors, we do have
an equivalent (I think) of this assuming it is a linear relationship?
(google found me a datasheet so at least superficially I think it is).
That attribute is illuminance_calibscale (calib bit means it is applied
on device or in driver rather than telling userspace what needs to be
applied by it). (yikes, just noticed that this is wrong in tsl2563 where
we have calibgain due to some inconsistency on my part a while back).
Also avoid 'magic' numbers as seen here. There are a finite range of
possible values, so have an extra attributes - here illuminance_calibscale_available
which is read only and gives the string "1 8 16 111" and make illuminance_calibscale
return -EINVAL if the value written is not one of these. sysfs_streq is really
handy for this sort of matching.
> +
> +What: /sys/bus/iio/devices/device[n]/als_time
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x ADC channel integration
> + time in 2.7ms increments.
Again, this is more general than just working for the tsl258x driver. That 2.7ms
isn't though. Please make this a value in millisecs (fixed point) then deal
with conversion to this base in the driver. On this one, it is closely related
to the 'period' attributes that exist for various events. Perhaps illuminance_period
is a more consistent name?
> +
> +What: /sys/bus/iio/devices/device[n]/als_trim
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x ADC active gain scale.
This one needs more detail before I can comment. I can't figure out what it
actually is!
> +
> +What: /sys/bus/iio/devices/device[n]/als_target
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x ADC last known external
> + lux measurement used in calibration.
als_target -> illuminance0_target
lux -> illuminance.
What units?
Sounds more general than the tsl258x to me so perhaps say that chip is an example
of its use?
> +
> +What: /sys/bus/iio/devices/device[n]/lux
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets the TAOS tsl258x calculated lux value.
Sorry, lux is a unit. Attributes are named after what is being measured. Hence
illuminance. Might seem reasonable to use the unit, but then what would we
do for acceleration (m/s^2)? I guess we can reopen this debate again if you are
really attached to lux... Also needs to be illuminance0_input. The input specifices
that it is in the base unit (lux) rather than raw. We stick to this form
to maintain compatiblity with hwmon which has been around a lot longer than us!
> +
> +What: /sys/bus/iio/devices/device[n]/lux_table
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets the TAOS tsl258x lux table of coefficients
> + that are used to calculate lux.
> +
> +What: /sys/bus/iio/devices/device[n]/reg_offset
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets an index the TAOS tsl258x internel register
> + for r/w of selected register.
Sorry, not letting direct register write attributes in. What do you need these
for? Can it really not be replaced by more informative attributes?
> +
> +What: /sys/bus/iio/devices/device[n]/reg_offset
> +KernelVersion: 2.6.37
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + This property gets/sets an TAOS tsl258x internel register value
> + indexed by reg_offset.
> +
Extra empty lines. Please remove. Also please run checkpatch over this patch. I think
there are some extra tabs on these lines that won't have gotten through that.
> +
> +
> \ No newline at end of file
> diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
> index 36d8bbe..ae499b8 100644
> --- a/drivers/staging/iio/light/Kconfig
> +++ b/drivers/staging/iio/light/Kconfig
> @@ -13,6 +13,13 @@ config SENSORS_TSL2563
> This driver can also be built as a module. If so, the module
> will be called tsl2563.
>
> +config TAOS_258x
> + tristate "TAOS TSL258x device driver"
> + default m
> + help
> + Provides support for the TAOS tsl258x family of devices.
> + Access ALS data/control via sysfs/iio.
Please list devices. People tend to grep the tree for the part number of the
device that they actually have hence it is useful to have them all explicitly
listed here. In combination with the id table listing them all this also tends
to tell people if the developer thinks it will work with the part they have.
> +
> config SENSORS_ISL29018
> tristate "ISL 29018 light and proximity sensor"
> depends on I2C
> diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile
> index 9142c0e..4395db8 100644
> --- a/drivers/staging/iio/light/Makefile
> +++ b/drivers/staging/iio/light/Makefile
> @@ -3,4 +3,5 @@
> #
>
> obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
> +obj-$(CONFIG_TAOS_258x) += tsl258x.o
> obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o
> diff --git a/drivers/staging/iio/light/tsl258x.c b/drivers/staging/iio/light/tsl258x.c
> new file mode 100644
> index 0000000..225d85b
> --- /dev/null
> +++ b/drivers/staging/iio/light/tsl258x.c
> @@ -0,0 +1,1010 @@
> +/*
> + * Device driver for monitoring ambient light intensity (lux)
> + * within the TAOS tsl258x family of devices
> + *
> + * Copyright (c) 2011, TAOS Corporation.
> + *
> + * 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.
> + *
> + * 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, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/i2c.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/string.h>
> +#include <linux/mutex.h>
> +#include "../iio.h"
> +
> +#define DEVICE_ID "tsl258x"
> +
> +#define MAX_DEVICE_REGS 32
> +
I'm guessing this is an internal part name? If possible please replace
with the numbers people will see on datasheets.
> +/* Triton register offsets */
> +#define TAOS_REG_MAX 8
> +
> +/* Device Registers and Masks */
> +#define TSL258X_CNTRL 0x00
This needs a a comment. Why two regs with the same address?
> +#define TSL258X_STATUS 0x00
> +#define TSL258X_ALS_TIME 0X01
> +#define TSL258X_INTERRUPT 0x02
> +#define TSL258X_GAIN 0x07
> +#define TSL258X_REVID 0x11
> +#define TSL258X_CHIPID 0x12
Cryptic name and never used. Please remove.
> +#define TSL258X_SMB_4 0x13
> +#define TSL258X_ALS_CHAN0LO 0x14
> +#define TSL258X_ALS_CHAN0HI 0x15
> +#define TSL258X_ALS_CHAN1LO 0x16
> +#define TSL258X_ALS_CHAN1HI 0x17
> +#define TSL258X_TMR_LO 0x18
> +#define TSL258X_TMR_HI 0x19
> +
> +/* Skate cmd reg masks */
> +#define TSL258X_CMD_REG 0x80
> +#define TSL258X_CMD_BYTE_RW 0x00
This name confuses me. Looks like it sets word protocol so why the block bit?
Also not used so could just get rid of it.
> +#define TSL258X_CMD_WORD_BLK_RW 0x20
> +#define TSL258X_CMD_SPL_FN 0x60
Nitpick. Later reference to INT have an _ after them. Perhaps add one for consistency?
> +#define TSL258X_CMD_ALS_INTCLR 0X01
> +
Err. Skate?
> +/* Skate cntrl reg masks */
Not used and rather pointless. Default would be to assume writing 0
cleared a register? (or does this mean something else?)
> +#define TSL258X_CNTL_REG_CLEAR 0x00
> +#define TSL258X_CNTL_ALS_INT_ENBL 0x10
> +#define TSL258X_CNTL_WAIT_TMR_ENBL 0x08
> +#define TSL258X_CNTL_ADC_ENBL 0x02
> +#define TSL258X_CNTL_PWRON 0x01
> +#define TSL258X_CNTL_ALSPON_ENBL 0x03
Define this in terms of PWRON and ADC_ENBL to make it clear what it is.
> +#define TSL258X_CNTL_INTALSPON_ENBL 0x13
also define this in terms of its sub parts.
> +
> +/* Skate status reg masks */
> +#define TSL258X_STA_ADCVALID 0x01
> +#define TSL258X_STA_ALSINTR 0x10
> +#define TSL258X_STA_ADCINTR 0x10
> +
> +/* Lux constants */
> +#define MAX_LUX 65535
Err. Rename that unless it really does mean 65535 lux
> +
> +enum {
> + TAOS_CHIP_UNKNOWN = 0, TAOS_CHIP_WORKING = 1, TAOS_CHIP_SLEEP = 2
> +} TAOS_CHIP_WORKING_STATUS;
> +
> +/* Per-device data */
> +struct taos_als_info {
> + u16 als_ch0;
> + u16 als_ch1;
> + u16 lux;
> +};
> +
> +struct taos_settings {
> + int als_time;
> + int als_gain;
> + int als_gain_trim;
> + int als_cal_target;
> +};
> +
> +struct tsl258x_chip {
> + struct mutex als_mutex;
> + struct i2c_client *client;
> + struct iio_dev *iio_dev;
> + struct delayed_work update_lux;
> + unsigned int addr;
> + char taos_id;
> + char valid;
> + unsigned long last_updated;
> + struct taos_als_info als_cur_info;
> + struct taos_settings taos_settings;
> + int als_time_scale;
> + int als_saturation;
> + int taos_chip_status;
> + u8 taos_config[8];
> +};
> +
> +static int taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val,
> + unsigned int len);
> +static int taos_i2c_write_command(struct i2c_client *client, u8 reg);
> +
> +/*
> + * Initial values for device - this values can/will be changed by driver.
> + * and applications as needed.
> + * These values are dynamic.
> + */
> +static const u8 taos_config[8] = {
> + 0x00, 0xee, 0x00, 0x03, 0x00, 0xFF, 0xFF, 0x00
> +}; /* cntrl atime intC Athl0 Athl1 Athh0 Athh1 gain */
> +
> +struct taos_lux {
> + unsigned int ratio;
> + unsigned int ch0;
> + unsigned int ch1;
> +};
> +
> +/* This structure is intentionally large to accommodate updates via sysfs. */
> +/* Sized to 11 = max 10 segments + 1 termination segment */
Any chance of two sensors on one device, each of which has different
values for this?
> +struct taos_lux taos_device_lux[11] = {
> + { 9830, 8520, 15729 },
> + { 12452, 10807, 23344 },
> + { 14746, 6383, 11705 },
> + { 17695, 4063, 6554 },
> +};
> +
> +struct taos_lux taos_lux;
> +
> +struct gainadj {
> + s16 ch0;
> + s16 ch1;
> +};
> +
> +/* Index = (0 - 3) Used to validate the gain selection index */
> +static const struct gainadj gainadj[] = {
> + { 1, 1 },
> + { 8, 8 },
> + { 16, 16 },
That's 'interesting'. This will make the calibscale discussion above more
complex. I guess no userspace is going to care about the precise internal
multipliers so a 'rough' value would probably do in that attribute. What
do you think?
> + { 107, 115 }
> +};
> +
> +/*
> + * Provides initial operational parameter defaults.
> + * These defaults may be changed through the device's sysfs files.
> + */
> +static void taos_defaults(struct tsl258x_chip *chip)
> +{
> + /* Operational parameters */
> + chip->taos_settings.als_time = 450;
> + /* must be a multiple of 50mS */
> + chip->taos_settings.als_gain = 2;
> + /* this is actually an index into the gain table */
> + /* assume clear glass as default */
> + chip->taos_settings.als_gain_trim = 1000;
> + /* default gain trim to account for aperture effects */
> + chip->taos_settings.als_cal_target = 130;
> + /* Known external ALS reading used for calibration */
> +
> + /* Initialize ALS data to defaults */
> + chip->als_cur_info.als_ch0 = 0;
> + chip->als_cur_info.als_ch1 = 0;
> + chip->als_cur_info.lux = 0;
Already zero via the memset (soon to be kzalloc) hence don't bother
setting these three.
> +}
> +
> +/*
> + * Read a number of bytes starting at register (reg) location.
> + * Return 0, or i2c_smbus_write_byte ERROR code.
> + */
> +static int
> +taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned int len)
> +{
> + int ret;
> + int i;
> +
> + for (i = 0; i < len; i++) {
> + /* select register to write */
> + ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | reg));
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_i2c_read failed to write"
> + " register %x\n", reg);
> + return ret;
> + }
> + /* read the data */
> + *val = i2c_smbus_read_byte(client);
I'd use val[i] without this increment for clarity.
> + val++;
> + if ((reg & 0x1f) == 0x1f)
Can this ever occcur in the driver? I would imagine it is a bug if it does
as you'll read back a different number of values than you expect to do.
If not it's an over enthusiastic bit of debug code so get rid of it.
> + break;
> + reg++;
> + }
> + return 0;
> +}
> +
> +/*
> + * This function is used to send a command to device command/control register
> + * All bytes sent using this command have their MSBit set - it's a command!
> + * Return 0, or i2c_smbus_write_byte error code.
> + */
> +static int taos_i2c_write_command(struct i2c_client *client, u8 reg)
> +{
> + int ret;
> +
> + /* write the data */
> + ret = i2c_smbus_write_byte(client, (reg |= TSL258X_CMD_REG));
> + if (ret < 0) {
> + dev_err(&client->dev, "FAILED: i2c_smbus_write_byte\n");
> + return ret;
> + }
> + return 0;
> +}
> +
> +/*
> + * Reads and calculates current lux value.
> + * The raw ch0 and ch1 values of the ambient light sensed in the last
> + * integration cycle are read from the device.
> + * Time scale factor array values are adjusted based on the integration time.
> + * The raw values are multiplied by a scale factor, and device gain is obtained
> + * using gain index. Limit checks are done next, then the ratio of a multiple
> + * of ch1 value, to the ch0 value, is calculated. The array taos_device_lux[]
> + * declared above is then scanned to find the first ratio value that is just
> + * above the ratio we just calculated. The ch0 and ch1 multiplier constants in
> + * the array are then used along with the time scale factor array values, to
> + * calculate the lux.
> + */
> +static int taos_get_lux(struct i2c_client *client)
> +{
> + u32 ch0, ch1; /* separated ch0/ch1 data from device */
> + u32 lux; /* raw lux calculated from device data */
And here we have a great example of why lux is a bad name for this.
lux could only ever have units of lux.
> + u32 ratio;
> + u8 buf[5];
> + struct taos_lux *p;
> + struct tsl258x_chip *chip = i2c_get_clientdata(client);
> + int i, ret;
> + u32 ch0lux = 0;
> + u32 ch1lux = 0;
> +
> + if (mutex_trylock(&chip->als_mutex) == 0) {
> + dev_info(&client->dev, "taos_get_lux device is busy\n");
> + return chip->als_cur_info.lux; /* busy, so return LAST VALUE */
> + }
> +
> + if (chip->taos_chip_status != TAOS_CHIP_WORKING) {
> + /* device is not enabled */
> + dev_err(&client->dev, "taos_get_lux device is not enabled\n");
> + ret = -ENODEV;
-EBUSY probably. The device exists, it's just turned off.
> + goto out_unlock;
> + }
> +
> + ret = taos_i2c_read(client, (TSL258X_CMD_REG), &buf[0], 1);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_get_lux failed to read CMD_REG\n");
> + goto out_unlock;
> + }
> + /* is data new & valid */
> + if (!(buf[0] & TSL258X_STA_ADCINTR)) {
> + dev_err(&client->dev, "taos_get_lux data not valid\n");
I'd error out at this point. Something has gone wrong and you want to
indicate it to your userspace code.
> + ret = chip->als_cur_info.lux; /* return LAST VALUE */
> + goto out_unlock;
> + }
> +
> + for (i = 0; i < 4; i++) {
> + int reg = TSL258X_CMD_REG | (TSL258X_ALS_CHAN0LO + i);
> + ret = taos_i2c_read(client, reg, &buf[i], 1);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_get_lux failed to read"
> + " register %x\n", reg);
> + goto out_unlock;
> + }
> + }
> +
> + /* clear status, really interrupt status (interrupts are off), but
> + * we use the bit anyway */
> + ret = taos_i2c_write_command(client,
> + TSL258X_CMD_REG | TSL258X_CMD_SPL_FN | TSL258X_CMD_ALS_INTCLR);
> + if (ret < 0) {
> + dev_err(&client->dev,
This is the one case where checkpatch warnings should be ignored.
Please keep strings like this on a single line as people who see them
in a log will grep for them in the source code. Artificial breaks
like this make them a lot harder to find!
> + "taos_i2c_write_command failed in "
> + "taos_get_lux, err = %d\n", ret);
> + goto out_unlock; /* have no data, so return failure */
> + }
> +
> + /* extract ALS/lux data */
Should be the relevant endian conversion function. be16tocpu or similar
(haven't thought about which). That way it's free on one endianness of
machine.
> + ch0 = (buf[1] << 8) | buf[0];
> + ch1 = (buf[3] << 8) | buf[2];
> +
> + chip->als_cur_info.als_ch0 = ch0;
> + chip->als_cur_info.als_ch1 = ch1;
> +
> + if ((ch0 >= chip->als_saturation) || (ch1 >= chip->als_saturation))
> + goto return_max;
> +
> + if (ch0 == 0) {
> + /* have no data, so return LAST VALUE */
> + ret = chip->als_cur_info.lux = 0;
> + goto out_unlock;
> + }
> + /* calculate ratio */
> + ratio = (ch1 << 15) / ch0;
> + /* convert to unscaled lux using the pointer to the table */
> + for (p = (struct taos_lux *) taos_device_lux;
> + p->ratio != 0 && p->ratio < ratio; p++)
> + ;
> +
> + if (p->ratio == 0) {
> + lux = 0;
> + } else {
> + ch0lux = ((ch0 * p->ch0) +
> + (gainadj[chip->taos_settings.als_gain].ch0 >> 1))
> + / gainadj[chip->taos_settings.als_gain].ch0;
> + ch1lux = ((ch1 * p->ch1) +
> + (gainadj[chip->taos_settings.als_gain].ch1 >> 1))
> + / gainadj[chip->taos_settings.als_gain].ch1;
> + lux = ch0lux - ch1lux;
> + }
> +
> + /* note: lux is 31 bit max at this point */
> + if (ch1lux > ch0lux) {
> + dev_dbg(&client->dev, "No Data - Return last value\n");
Again, do we want userspace to know the sensor isn't returning valid values?
> + ret = chip->als_cur_info.lux = 0;
> + goto out_unlock;
> + }
> +
> + /* adjust for active time scale */
> + if (chip->als_time_scale == 0)
> + lux = 0;
> + else
> + lux = (lux + (chip->als_time_scale >> 1)) /
> + chip->als_time_scale;
> +
> + /* adjust for active gain scale */
> + lux >>= 13; /* tables have factor of 8192 builtin for accuracy */
> + lux = (lux * chip->taos_settings.als_gain_trim + 500) / 1000;
> + if (lux > MAX_LUX) { /* check for overflow */
> +return_max:
> + lux = MAX_LUX;
Really the right thing to do? Surely you want to return an out of range
error? Perhaps ERANGE so userspace knows the value is garbage.
> + }
> +
> + /* Update the structure with the latest VALID lux. */
> + chip->als_cur_info.lux = lux;
> + ret = lux;
> +
> +out_unlock:
> + mutex_unlock(&chip->als_mutex);
> + return ret;
> +}
> +
> +/*
> + * Obtain single reading and calculate the als_gain_trim (later used
> + * to derive actual lux).
> + * Return updated gain_trim value.
> + */
> +int taos_als_calibrate(struct i2c_client *client)
> +{
> + struct tsl258x_chip *chip = i2c_get_clientdata(client);
> + u8 reg_val;
> + unsigned int gain_trim_val;
> + int ret;
> + int lux_val;
> +
> + ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | TSL258X_CNTRL));
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_als_calibrate failed to reach the"
Again, don't break this sort of string up however much checkpatch rants at you.
> + " CNTRL register, ret=%d\n", ret);
> + return ret;
> + }
> +
> + reg_val = i2c_smbus_read_byte(client);
> + if ((reg_val & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWRON))
> + != (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWRON)) {
> + dev_err(&client->dev, "taos_als_calibrate failed because the"
> + " device is not powered on with ADC enabled\n");
> + return -ENODATA;
> + }
> +
> + ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | TSL258X_STATUS));
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_als_calibrate failed to reach the"
> + " STATUS register, ret=%d\n", ret);
> + return ret;
> + }
> + reg_val = i2c_smbus_read_byte(client);
> +
> + if ((reg_val & TSL258X_STA_ADCVALID) != TSL258X_STA_ADCVALID) {
> + dev_err(&client->dev, "taos_als_calibrate failed because the"
> + " STATUS did not indicate ADC valid.\n");
> + return -ENODATA;
> + }
> + lux_val = taos_get_lux(client);
> + if (lux_val < 0) {
> + dev_err(&client->dev, "taos_als_calibrate failed to get lux\n");
> + return lux_val;
> + }
> + gain_trim_val = (unsigned int) (((chip->taos_settings.als_cal_target)
> + * chip->taos_settings.als_gain_trim) / lux_val);
> +
> + dev_info(&client->dev, "taos_settings.als_cal_target = %d\n"
> + "taos_settings.als_gain_trim = %d\nlux_val = %d\n",
> + chip->taos_settings.als_cal_target,
> + chip->taos_settings.als_gain_trim,
> + lux_val);
> +
> + if ((gain_trim_val < 250) || (gain_trim_val > 4000)) {
> + dev_err(&client->dev, "taos_als_calibrate failed because"
> + "trim_val of %d is out of range\n", gain_trim_val);
> + return -ENODATA;
> + }
> + chip->taos_settings.als_gain_trim = (int) gain_trim_val;
> +
> + return (int) gain_trim_val;
> +}
> +
> +/*
> + * Turn the device on.
> + * Configuration must be set before calling this function.
> + */
> +static int taos_chip_on(struct i2c_client *client)
> +{
> + int i;
> + int ret = 0;
> + u8 *uP;
> + u8 utmp;
> + int als_count;
> + int als_time;
> + struct tsl258x_chip *chip = i2c_get_clientdata(client);
> +
and?
> + /* and make sure we're not already on */
> + if (chip->taos_chip_status == TAOS_CHIP_WORKING) {
> + /* if forcing a register update - turn off, then on */
> + dev_info(&client->dev, "device is already enabled\n");
Not sure what the right error is here, but should be more specific than
that.
> + return -1;
> + }
> +
> + /* determine als integration regster */
> + als_count = (chip->taos_settings.als_time * 100 + 135) / 270;
> + if (als_count == 0)
> + als_count = 1; /* ensure at least one cycle */
> +
> +
> + /* convert back to time (encompasses overrides) */
> + als_time = (als_count * 27 + 5) / 10;
> + chip->taos_config[TSL258X_ALS_TIME] = 256 - als_count;
> +
> +
Bonus blank lines. One is plenty to break up code.
> + /* Set the gain based on taos_settings struct */
> + chip->taos_config[TSL258X_GAIN] = chip->taos_settings.als_gain;
> +
> +
> + /* set globals re scaling and saturation */
They aren't globals
> + chip->als_saturation = als_count * 922; /* 90% of full scale */
> + chip->als_time_scale = (als_time + 25) / 50;
> +
> + /* SKATE Specific power-on / adc enable sequence
> + * Power on the device 1st. */
> + utmp = TSL258X_CNTL_PWRON;
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
> + utmp);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_chip_on failed on CNTRL reg.\n");
> + return -1;
> + }
> +
> + /* Use the following shadow copy for our delay before enabling ADC.
> + * Write all the registers. */
> + for (i = 0, uP = chip->taos_config; i < TAOS_REG_MAX; i++) {
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG + i,
> + *uP++);
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "taos_chip_on failed on reg %d.\n", i);
> + return -1;
> + }
> + }
> +
> + mdelay(3);
> + /* NOW enable the ADC
> + * initialize the desired mode of operation */
> + utmp = TSL258X_CNTL_PWRON | TSL258X_CNTL_ADC_ENBL;
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
> + utmp);
> + if (ret < 0) {
> + dev_err(&client->dev, "taos_chip_on failed on 2nd CTRL reg.\n");
> + return -1;
> + }
> + chip->taos_chip_status = TAOS_CHIP_WORKING;
Comment is rather obvious...
> + return ret; /* returns result of last i2cwrite */
> +}
> +
> +/* Turn the device OFF. */
> +static int taos_chip_off(struct i2c_client *client)
> +{
> + struct tsl258x_chip *chip = i2c_get_clientdata(client);
> + int ret;
> + u8 utmp;
> +
> + /* turn device off */
> + chip->taos_chip_status = TAOS_CHIP_SLEEP;
> + utmp = 0x00;
> + ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
> + utmp);
> + return ret;
> +}
> +
> +/* Sysfs Interface Functions */
> +static ssize_t taos_device_id(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%s\n", DEVICE_ID);
> +}
> +
> +static ssize_t taos_power_state_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_chip_status);
> +}
> +
> +static ssize_t taos_power_state_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value == 0)
> + taos_chip_off(chip->client);
> + else
> + taos_chip_on(chip->client);
> +
> + return len;
> +}
> +
> +static ssize_t taos_gain_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_gain);
> +}
> +
> +static ssize_t taos_gain_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> + if (value) {
> + if (value > 4) {
> + dev_err(dev, "Invalid Gain Index\n");
> + return -1;
EINVAL
> + } else {
> + chip->taos_settings.als_gain = value;
> + }
So this sets the value in the local cache. When does it get written to the
device?
> + }
> + return len;
> +}
> +
> +static ssize_t taos_als_time_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_time);
> +}
> +
> +static ssize_t taos_als_time_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value)
> + chip->taos_settings.als_time = value;
else? It's not been sucessfuly set so return -EINVAL
> +
> + return len;
> +}
> +
> +static ssize_t taos_als_trim_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_gain_trim);
> +}
> +
> +static ssize_t taos_als_trim_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value)
> + chip->taos_settings.als_gain_trim = value;
else return -EINVAL
> +
> + return len;
> +}
> +
> +static ssize_t taos_als_cal_target_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + return sprintf(buf, "%d\n", chip->taos_settings.als_cal_target);
> +}
> +
> +static ssize_t taos_als_cal_target_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value)
> + chip->taos_settings.als_cal_target = value;
again, else return -EINVAL; Userspace really wants to know this failed.
> +
> + return len;
> +}
> +
> +static ssize_t taos_lux_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + int lux = 0;
no need to assign.
> +
> + lux = taos_get_lux(chip->client);
> +
> + return sprintf(buf, "%d\n", lux);
> +}
> +
> +static ssize_t taos_do_calibrate(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> + unsigned long value;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value == 1)
> + taos_als_calibrate(chip->client);
else return -EINVAL;
> +
> + return len;
> +}
> +
> +static ssize_t taos_luxtable_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int i;
> + int offset = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(taos_device_lux); i++) {
> + offset += sprintf(buf + offset, "%d,%d,%d,",
> + taos_device_lux[i].ratio,
> + taos_device_lux[i].ch0,
> + taos_device_lux[i].ch1);
> + if (taos_device_lux[i].ratio == 0) {
> + /* We just printed the first "0" entry.
> + * Now get rid of the extra "," and break. */
> + offset--;
> + break;
> + }
> + }
> +
> + offset += sprintf(buf + offset, "\n");
> + return offset;
> +}
> +
> +static ssize_t taos_luxtable_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +#define MAX_LUX_TABLE_FIELDS 33
I'd loose this define.
> + int value[MAX_LUX_TABLE_FIELDS];
> + int n;
> +
> + get_options(buf, ARRAY_SIZE(value), value);
> +
> + /* We now have an array of ints starting at value[1], and
> + * enumerated by value[0].
> + * We expect each group of three ints is one table entry,
> + * and the last table entry is all 0.
> + */
> + n = value[0];
prefer to see ARRAY_SIZE(value) than the define.
> + if ((n % 3) || n < 6 || n > (MAX_LUX_TABLE_FIELDS - 3)) {
> + dev_info(dev, "LUX TABLE INPUT ERROR 1 Value[0]=%d\n", n);
> + return -EINVAL;
> + }
> + if ((value[(n - 2)] | value[(n - 1)] | value[n]) != 0) {
> + dev_info(dev, "LUX TABLE INPUT ERROR 2 Value[0]=%d\n", n);
> + return -EINVAL;
> + }
> +
> + if (chip->taos_chip_status == TAOS_CHIP_WORKING)
> + taos_chip_off(chip->client);
> +
> + /* Zero out the table */
> + memset(taos_device_lux, 0, sizeof(taos_device_lux));
> + memcpy(taos_device_lux, &value[1], (value[0] * 4));
> +
> + taos_chip_on(chip->client);
> +
> + return len;
> +}
> +
> +static u8 reg_index;
> +
As stated above these need to go before we take this driver.
> +/* Sets a pointer to a register for R/W via sysfs */
> +static ssize_t taos_reg_offset_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%d\n", reg_index);
> + return 0;
> +}
> +
> +static ssize_t taos_reg_offset_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + unsigned long value;
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + if (value > MAX_DEVICE_REGS) {
> + dev_err(dev, "register offset exceeds MAX\n");
> + return -1;
> + } else {
> + reg_index = value;
> + }
> +return len;
> +}
> +
> +/* R/W a register for R/W via sysfs */
> +static ssize_t taos_reg_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + u8 value = 0;
> + int ret = 0;
> +
> + ret = i2c_smbus_write_byte(chip->client,
> + (TSL258X_CMD_REG | reg_index));
> + if (ret < 0) {
> + dev_err(dev, "i2c_smbus_write_byte to cmd reg failed "
> + "in taos_reg_offset_show(), err = %d\n", ret);
> + return ret;
> + }
> + value = i2c_smbus_read_byte(chip->client);
> + return sprintf(buf, "%d\n", value);
> +
> +return 0;
> +}
> +
> +static ssize_t taos_reg_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tsl258x_chip *chip = indio_dev->dev_data;
> +
> + unsigned long value = 0;
> + int ret = 0;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + ret = i2c_smbus_write_byte_data(chip->client,
> + (TSL258X_CMD_REG | reg_index), value);
> + if (ret < 0) {
> + dev_err(dev, "i2c_smbus_write_byte_data failed in "
> + "taos_reg_store()\n");
> + return ret;
> + }
> +
> +return len;
> +}
> +
> +static DEVICE_ATTR(name, S_IRUGO, taos_device_id, NULL);
> +static DEVICE_ATTR(device_state, S_IRUGO | S_IWUSR,
> + taos_power_state_show, taos_power_state_store);
> +static DEVICE_ATTR(als_gain, S_IRUGO | S_IWUSR,
> + taos_gain_show, taos_gain_store);
> +static DEVICE_ATTR(als_time, S_IRUGO | S_IWUSR,
> + taos_als_time_show, taos_als_time_store);
> +static DEVICE_ATTR(als_trim, S_IRUGO | S_IWUSR,
> + taos_als_trim_show, taos_als_trim_store);
> +static DEVICE_ATTR(als_target, S_IRUGO | S_IWUSR,
> + taos_als_cal_target_show, taos_als_cal_target_store);
> +static DEVICE_ATTR(lux, S_IRUGO, taos_lux_show, NULL);
> +static DEVICE_ATTR(calibrate, S_IWUSR, NULL, taos_do_calibrate);
> +static DEVICE_ATTR(lux_table, S_IRUGO | S_IWUSR,
> + taos_luxtable_show, taos_luxtable_store);
> +static DEVICE_ATTR(reg_offset, S_IRUGO | S_IWUSR,
> + taos_reg_offset_show,
> + taos_reg_offset_store);
> +static DEVICE_ATTR(reg, S_IRUGO | S_IWUSR,
> + taos_reg_show,
> + taos_reg_store);
> +
> +
> +static struct attribute *sysfs_attrs_ctrl[] = {
> + &dev_attr_name.attr,
> + &dev_attr_device_state.attr,
> + &dev_attr_als_gain.attr,
> + &dev_attr_als_time.attr,
> + &dev_attr_als_trim.attr,
> + &dev_attr_als_target.attr,
> + &dev_attr_lux.attr,
> + &dev_attr_calibrate.attr,
> + &dev_attr_lux_table.attr,
Spaces instead of tab here.
> + &dev_attr_reg_offset.attr,
> + &dev_attr_reg.attr,
> + NULL
> +};
> +
> +static struct attribute_group tsl258x_attribute_group = {
> + .attrs = sysfs_attrs_ctrl,
> +};
> +
> +/* Use the default register values to identify the Taos device */
> +static int taos_skate_device(unsigned char *bufp)
> +{
> + if (((bufp[TSL258X_CHIPID] & 0xf0) == 0x90))
> + /* tsl258x */
> + return 1;
> + /* else unknown */
> + return 0;
> +}
> +
> +/*
> + * Client probe function - When a valid device is found, the driver's device
> + * data structure is updated, and initialization completes successfully.
> + */
> +static int __devinit taos_probe(struct i2c_client *clientp,
> + const struct i2c_device_id *idp)
> +{
> + int i, ret = 0;
> + unsigned char buf[MAX_DEVICE_REGS];
> + static struct tsl258x_chip *chip;
> +
> + if (!i2c_check_functionality(clientp->adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA)) {
> + dev_err(&clientp->dev,
> + "taos_probe() - i2c smbus byte data "
> + "functions unsupported\n");
> + return -EOPNOTSUPP;
> + }
> + if (!i2c_check_functionality(clientp->adapter,
> + I2C_FUNC_SMBUS_WORD_DATA)) {
> + dev_warn(&clientp->dev,
> + "taos_probe() - i2c smbus word data "
> + "functions unsupported\n");
> + }
> + if (!i2c_check_functionality(clientp->adapter,
> + I2C_FUNC_SMBUS_BLOCK_DATA)) {
> + dev_err(&clientp->dev,
> + "taos_probe() - i2c smbus block data "
> + "functions unsupported\n");
> + }
> +
> + chip = kmalloc(sizeof(struct tsl258x_chip), GFP_KERNEL);
> + if (!chip) {
> + dev_err(&clientp->dev, "taos_device_drvr: kmalloc for "
> + "struct tsl258x_chip failed in taos_probe()\n");
> + return -ENOMEM;
> + }
> + memset(chip, 0, sizeof(struct tsl258x_chip));
Use kzalloc instead of kmalloc then you don't need the memset.
> + chip->client = clientp;
> + i2c_set_clientdata(clientp, chip);
> +
> + mutex_init(&chip->als_mutex);
> + chip->taos_chip_status = TAOS_CHIP_UNKNOWN; /* uninitialized to start */
> + memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
> +
> + for (i = 0; i < MAX_DEVICE_REGS; i++) {
> + ret = i2c_smbus_write_byte(clientp,
> + (TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
> + if (ret < 0) {
> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd "
> + "reg failed in taos_probe(), err = %d\n", ret);
> + goto fail1;
> + }
> + buf[i] = i2c_smbus_read_byte(clientp);
> + }
> + if (!taos_skate_device(buf)) {
> + dev_info(&clientp->dev, "i2c device found but does not match "
> + "expected id in taos_probe()\n");
> + goto fail1;
> + } else {
> + dev_info(&clientp->dev, "i2c device match in probe\n");
> + }
> + ret = i2c_smbus_write_byte(clientp, (TSL258X_CMD_REG | TSL258X_CNTRL));
> + if (ret < 0) {
> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd reg "
> + "failed in taos_probe(), err = %d\n", ret);
> + goto fail1;
> + }
> + chip->valid = 0;
> +
> + chip->iio_dev = iio_allocate_device();
> + if (!chip->iio_dev) {
> + ret = -ENOMEM;
> + dev_err(&clientp->dev, "iio allocation failed\n");
> + goto fail1;
> + }
> +
> + chip->iio_dev->attrs = &tsl258x_attribute_group;
> + chip->iio_dev->dev.parent = &clientp->dev;
> + chip->iio_dev->dev_data = (void *)(chip);
> + chip->iio_dev->driver_module = THIS_MODULE;
> + chip->iio_dev->modes = INDIO_DIRECT_MODE;
> + ret = iio_device_register(chip->iio_dev);
> + if (ret) {
> + dev_err(&clientp->dev, "iio registration failed\n");
> + goto fail1;
> + }
> +
> + /* Load up the V2 defaults (these are hard coded defaults for now) */
> + taos_defaults(chip);
> +
> + /* Make sure the chip is on */
> + taos_chip_on(clientp);
> +
> + dev_info(&clientp->dev, "Light sensor found.\n");
> + return 0;
> +
> +fail1:
> + kfree(chip);
> +
> + return ret;
> +}
> +
> +static int __devexit taos_remove(struct i2c_client *client)
> +{
> + struct tsl258x_chip *chip = i2c_get_clientdata(client);
> +
> + iio_device_unregister(chip->iio_dev);
> +
> + kfree(chip);
> + return 0;
> +}
> +
> +static struct i2c_device_id taos_idtable[] = {
> + { DEVICE_ID, 0 },
> + {}
Please can we have an explicit list if ID's here. If a person has
a tsl2580 then they'll want to say that in their board file.
On that note, I skipped this originally as there were bigger fish
to fry, but can we be sure that there will never be a value of x
that this driver won't work for? If not, please follow convention
and pick your favourite part number from the range and replace all
x's in the driver appropriately. Too many part manufacturers have
really weird naming schemes that tend to break wild cards like this!
> +};
> +MODULE_DEVICE_TABLE(i2c, taos_idtable);
> +
> +/* Driver definition */
> +static struct i2c_driver taos_driver = {
> + .driver = {
> + .name = "tsl258x",
> + },
> + .id_table = taos_idtable,
> + .probe = taos_probe,
> + .remove = __devexit_p(taos_remove),
> +};
> +
> +static int __init taos_init(void)
> +{
> + return i2c_add_driver(&taos_driver);
> +}
> +
> +static void __exit taos_exit(void)
> +{
> + i2c_del_driver(&taos_driver);
> +}
> +
> +module_init(taos_init);
> +module_exit(taos_exit);
> +
> +MODULE_AUTHOR("J. August Brenner<jbrenner@taosinc.com>");
> +MODULE_DESCRIPTION("TAOS tsl258x ambient light sensor driver");
> +MODULE_LICENSE("GPL");
> +
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2.6.38-rc7]TAOS 258x: Device Driver
@ 2011-03-09 1:32 Jon Brenner
2011-03-09 19:14 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Jon Brenner @ 2011-03-09 1:32 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Linux Kernel
From: J. August Brenner <jbrenner@taosinc.com>
This driver supports the TAOS tsl258x family of ALS sensors.
This driver provides ALS data (lux), and device configuration via
isysfs/iio.
More significantly, this driver provides the capability to be fed 'glass
coefficients' to accommodate varying system designs (bezels, faceplates,
etc.).
Signed-off-by: Jon August Brenner <jbrenner@taosinc.com>
---
>From 5b8364f9828dbad5fbfff96385e3fc2a6a6d56ed Mon Sep 17 00:00:00 2001
From: Jon Brenner <jbrenner@taosinc.com>
Date: Tue, 8 Mar 2011 18:37:13 -0600
Subject: [PATCH] TAOS driver for the tsl258x family of devices.
---
Makefile | 2 +-
.../staging/iio/Documentation/sysfs-bus-iio-light | 63 ++
drivers/staging/iio/light/Kconfig | 7 +
drivers/staging/iio/light/Makefile | 1 +
drivers/staging/iio/light/tsl258x.c | 1010 ++++++++++++++++++++
5 files changed, 1082 insertions(+), 1 deletions(-)
diff --git a/Makefile b/Makefile
index 26d7d82..2f7d922 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
VERSION = 2
PATCHLEVEL = 6
SUBLEVEL = 38
-EXTRAVERSION = -rc6
+EXTRAVERSION = -rc7
NAME = Flesh-Eating Bats with Fangs
# *DOCUMENTATION*
diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
index 5d84856..5affc2a 100644
--- a/drivers/staging/iio/Documentation/sysfs-bus-iio-light
+++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
@@ -62,3 +62,66 @@ Description:
sensing mode. This value should be the output from a reading
and if expressed in SI units, should include _input. If this
value is not in SI units, then it should include _raw.
+
+What: /sys/bus/iio/devices/device[n]/device_state
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets/sets the TAOS tsl258x power state.
+
+What: /sys/bus/iio/devices/device[n]/als_gain
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets/sets the TAOS tsl258x analog gain settings
+ of the device.
+
+What: /sys/bus/iio/devices/device[n]/als_time
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets/sets the TAOS tsl258x ADC channel integration
+ time in 2.7ms increments.
+
+What: /sys/bus/iio/devices/device[n]/als_trim
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets/sets the TAOS tsl258x ADC active gain scale.
+
+What: /sys/bus/iio/devices/device[n]/als_target
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets/sets the TAOS tsl258x ADC last known external
+ lux measurement used in calibration.
+
+What: /sys/bus/iio/devices/device[n]/lux
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets the TAOS tsl258x calculated lux value.
+
+What: /sys/bus/iio/devices/device[n]/lux_table
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets/sets the TAOS tsl258x lux table of coefficients
+ that are used to calculate lux.
+
+What: /sys/bus/iio/devices/device[n]/reg_offset
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets/sets an index the TAOS tsl258x internel register
+ for r/w of selected register.
+
+What: /sys/bus/iio/devices/device[n]/reg_offset
+KernelVersion: 2.6.37
+Contact: linux-iio@vger.kernel.org
+Description:
+ This property gets/sets an TAOS tsl258x internel register value
+ indexed by reg_offset.
+
+
+
\ No newline at end of file
diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
index 36d8bbe..ae499b8 100644
--- a/drivers/staging/iio/light/Kconfig
+++ b/drivers/staging/iio/light/Kconfig
@@ -13,6 +13,13 @@ config SENSORS_TSL2563
This driver can also be built as a module. If so, the module
will be called tsl2563.
+config TAOS_258x
+ tristate "TAOS TSL258x device driver"
+ default m
+ help
+ Provides support for the TAOS tsl258x family of devices.
+ Access ALS data/control via sysfs/iio.
+
config SENSORS_ISL29018
tristate "ISL 29018 light and proximity sensor"
depends on I2C
diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile
index 9142c0e..4395db8 100644
--- a/drivers/staging/iio/light/Makefile
+++ b/drivers/staging/iio/light/Makefile
@@ -3,4 +3,5 @@
#
obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
+obj-$(CONFIG_TAOS_258x) += tsl258x.o
obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o
diff --git a/drivers/staging/iio/light/tsl258x.c b/drivers/staging/iio/light/tsl258x.c
new file mode 100644
index 0000000..225d85b
--- /dev/null
+++ b/drivers/staging/iio/light/tsl258x.c
@@ -0,0 +1,1010 @@
+/*
+ * Device driver for monitoring ambient light intensity (lux)
+ * within the TAOS tsl258x family of devices
+ *
+ * Copyright (c) 2011, TAOS Corporation.
+ *
+ * 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.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/i2c.h>
+#include <linux/errno.h>
+#include <linux/delay.h>
+#include <linux/string.h>
+#include <linux/mutex.h>
+#include "../iio.h"
+
+#define DEVICE_ID "tsl258x"
+
+#define MAX_DEVICE_REGS 32
+
+/* Triton register offsets */
+#define TAOS_REG_MAX 8
+
+/* Device Registers and Masks */
+#define TSL258X_CNTRL 0x00
+#define TSL258X_STATUS 0x00
+#define TSL258X_ALS_TIME 0X01
+#define TSL258X_INTERRUPT 0x02
+#define TSL258X_GAIN 0x07
+#define TSL258X_REVID 0x11
+#define TSL258X_CHIPID 0x12
+#define TSL258X_SMB_4 0x13
+#define TSL258X_ALS_CHAN0LO 0x14
+#define TSL258X_ALS_CHAN0HI 0x15
+#define TSL258X_ALS_CHAN1LO 0x16
+#define TSL258X_ALS_CHAN1HI 0x17
+#define TSL258X_TMR_LO 0x18
+#define TSL258X_TMR_HI 0x19
+
+/* Skate cmd reg masks */
+#define TSL258X_CMD_REG 0x80
+#define TSL258X_CMD_BYTE_RW 0x00
+#define TSL258X_CMD_WORD_BLK_RW 0x20
+#define TSL258X_CMD_SPL_FN 0x60
+#define TSL258X_CMD_ALS_INTCLR 0X01
+
+/* Skate cntrl reg masks */
+#define TSL258X_CNTL_REG_CLEAR 0x00
+#define TSL258X_CNTL_ALS_INT_ENBL 0x10
+#define TSL258X_CNTL_WAIT_TMR_ENBL 0x08
+#define TSL258X_CNTL_ADC_ENBL 0x02
+#define TSL258X_CNTL_PWRON 0x01
+#define TSL258X_CNTL_ALSPON_ENBL 0x03
+#define TSL258X_CNTL_INTALSPON_ENBL 0x13
+
+/* Skate status reg masks */
+#define TSL258X_STA_ADCVALID 0x01
+#define TSL258X_STA_ALSINTR 0x10
+#define TSL258X_STA_ADCINTR 0x10
+
+/* Lux constants */
+#define MAX_LUX 65535
+
+enum {
+ TAOS_CHIP_UNKNOWN = 0, TAOS_CHIP_WORKING = 1, TAOS_CHIP_SLEEP = 2
+} TAOS_CHIP_WORKING_STATUS;
+
+/* Per-device data */
+struct taos_als_info {
+ u16 als_ch0;
+ u16 als_ch1;
+ u16 lux;
+};
+
+struct taos_settings {
+ int als_time;
+ int als_gain;
+ int als_gain_trim;
+ int als_cal_target;
+};
+
+struct tsl258x_chip {
+ struct mutex als_mutex;
+ struct i2c_client *client;
+ struct iio_dev *iio_dev;
+ struct delayed_work update_lux;
+ unsigned int addr;
+ char taos_id;
+ char valid;
+ unsigned long last_updated;
+ struct taos_als_info als_cur_info;
+ struct taos_settings taos_settings;
+ int als_time_scale;
+ int als_saturation;
+ int taos_chip_status;
+ u8 taos_config[8];
+};
+
+static int taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val,
+ unsigned int len);
+static int taos_i2c_write_command(struct i2c_client *client, u8 reg);
+
+/*
+ * Initial values for device - this values can/will be changed by driver.
+ * and applications as needed.
+ * These values are dynamic.
+ */
+static const u8 taos_config[8] = {
+ 0x00, 0xee, 0x00, 0x03, 0x00, 0xFF, 0xFF, 0x00
+}; /* cntrl atime intC Athl0 Athl1 Athh0 Athh1 gain */
+
+struct taos_lux {
+ unsigned int ratio;
+ unsigned int ch0;
+ unsigned int ch1;
+};
+
+/* This structure is intentionally large to accommodate updates via sysfs. */
+/* Sized to 11 = max 10 segments + 1 termination segment */
+struct taos_lux taos_device_lux[11] = {
+ { 9830, 8520, 15729 },
+ { 12452, 10807, 23344 },
+ { 14746, 6383, 11705 },
+ { 17695, 4063, 6554 },
+};
+
+struct taos_lux taos_lux;
+
+struct gainadj {
+ s16 ch0;
+ s16 ch1;
+};
+
+/* Index = (0 - 3) Used to validate the gain selection index */
+static const struct gainadj gainadj[] = {
+ { 1, 1 },
+ { 8, 8 },
+ { 16, 16 },
+ { 107, 115 }
+};
+
+/*
+ * Provides initial operational parameter defaults.
+ * These defaults may be changed through the device's sysfs files.
+ */
+static void taos_defaults(struct tsl258x_chip *chip)
+{
+ /* Operational parameters */
+ chip->taos_settings.als_time = 450;
+ /* must be a multiple of 50mS */
+ chip->taos_settings.als_gain = 2;
+ /* this is actually an index into the gain table */
+ /* assume clear glass as default */
+ chip->taos_settings.als_gain_trim = 1000;
+ /* default gain trim to account for aperture effects */
+ chip->taos_settings.als_cal_target = 130;
+ /* Known external ALS reading used for calibration */
+
+ /* Initialize ALS data to defaults */
+ chip->als_cur_info.als_ch0 = 0;
+ chip->als_cur_info.als_ch1 = 0;
+ chip->als_cur_info.lux = 0;
+}
+
+/*
+ * Read a number of bytes starting at register (reg) location.
+ * Return 0, or i2c_smbus_write_byte ERROR code.
+ */
+static int
+taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned int len)
+{
+ int ret;
+ int i;
+
+ for (i = 0; i < len; i++) {
+ /* select register to write */
+ ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | reg));
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_i2c_read failed to write"
+ " register %x\n", reg);
+ return ret;
+ }
+ /* read the data */
+ *val = i2c_smbus_read_byte(client);
+ val++;
+ if ((reg & 0x1f) == 0x1f)
+ break;
+ reg++;
+ }
+ return 0;
+}
+
+/*
+ * This function is used to send a command to device command/control register
+ * All bytes sent using this command have their MSBit set - it's a command!
+ * Return 0, or i2c_smbus_write_byte error code.
+ */
+static int taos_i2c_write_command(struct i2c_client *client, u8 reg)
+{
+ int ret;
+
+ /* write the data */
+ ret = i2c_smbus_write_byte(client, (reg |= TSL258X_CMD_REG));
+ if (ret < 0) {
+ dev_err(&client->dev, "FAILED: i2c_smbus_write_byte\n");
+ return ret;
+ }
+ return 0;
+}
+
+/*
+ * Reads and calculates current lux value.
+ * The raw ch0 and ch1 values of the ambient light sensed in the last
+ * integration cycle are read from the device.
+ * Time scale factor array values are adjusted based on the integration time.
+ * The raw values are multiplied by a scale factor, and device gain is obtained
+ * using gain index. Limit checks are done next, then the ratio of a multiple
+ * of ch1 value, to the ch0 value, is calculated. The array taos_device_lux[]
+ * declared above is then scanned to find the first ratio value that is just
+ * above the ratio we just calculated. The ch0 and ch1 multiplier constants in
+ * the array are then used along with the time scale factor array values, to
+ * calculate the lux.
+ */
+static int taos_get_lux(struct i2c_client *client)
+{
+ u32 ch0, ch1; /* separated ch0/ch1 data from device */
+ u32 lux; /* raw lux calculated from device data */
+ u32 ratio;
+ u8 buf[5];
+ struct taos_lux *p;
+ struct tsl258x_chip *chip = i2c_get_clientdata(client);
+ int i, ret;
+ u32 ch0lux = 0;
+ u32 ch1lux = 0;
+
+ if (mutex_trylock(&chip->als_mutex) == 0) {
+ dev_info(&client->dev, "taos_get_lux device is busy\n");
+ return chip->als_cur_info.lux; /* busy, so return LAST VALUE */
+ }
+
+ if (chip->taos_chip_status != TAOS_CHIP_WORKING) {
+ /* device is not enabled */
+ dev_err(&client->dev, "taos_get_lux device is not enabled\n");
+ ret = -ENODEV;
+ goto out_unlock;
+ }
+
+ ret = taos_i2c_read(client, (TSL258X_CMD_REG), &buf[0], 1);
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_get_lux failed to read CMD_REG\n");
+ goto out_unlock;
+ }
+ /* is data new & valid */
+ if (!(buf[0] & TSL258X_STA_ADCINTR)) {
+ dev_err(&client->dev, "taos_get_lux data not valid\n");
+ ret = chip->als_cur_info.lux; /* return LAST VALUE */
+ goto out_unlock;
+ }
+
+ for (i = 0; i < 4; i++) {
+ int reg = TSL258X_CMD_REG | (TSL258X_ALS_CHAN0LO + i);
+ ret = taos_i2c_read(client, reg, &buf[i], 1);
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_get_lux failed to read"
+ " register %x\n", reg);
+ goto out_unlock;
+ }
+ }
+
+ /* clear status, really interrupt status (interrupts are off), but
+ * we use the bit anyway */
+ ret = taos_i2c_write_command(client,
+ TSL258X_CMD_REG | TSL258X_CMD_SPL_FN | TSL258X_CMD_ALS_INTCLR);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "taos_i2c_write_command failed in "
+ "taos_get_lux, err = %d\n", ret);
+ goto out_unlock; /* have no data, so return failure */
+ }
+
+ /* extract ALS/lux data */
+ ch0 = (buf[1] << 8) | buf[0];
+ ch1 = (buf[3] << 8) | buf[2];
+
+ chip->als_cur_info.als_ch0 = ch0;
+ chip->als_cur_info.als_ch1 = ch1;
+
+ if ((ch0 >= chip->als_saturation) || (ch1 >= chip->als_saturation))
+ goto return_max;
+
+ if (ch0 == 0) {
+ /* have no data, so return LAST VALUE */
+ ret = chip->als_cur_info.lux = 0;
+ goto out_unlock;
+ }
+ /* calculate ratio */
+ ratio = (ch1 << 15) / ch0;
+ /* convert to unscaled lux using the pointer to the table */
+ for (p = (struct taos_lux *) taos_device_lux;
+ p->ratio != 0 && p->ratio < ratio; p++)
+ ;
+
+ if (p->ratio == 0) {
+ lux = 0;
+ } else {
+ ch0lux = ((ch0 * p->ch0) +
+ (gainadj[chip->taos_settings.als_gain].ch0 >> 1))
+ / gainadj[chip->taos_settings.als_gain].ch0;
+ ch1lux = ((ch1 * p->ch1) +
+ (gainadj[chip->taos_settings.als_gain].ch1 >> 1))
+ / gainadj[chip->taos_settings.als_gain].ch1;
+ lux = ch0lux - ch1lux;
+ }
+
+ /* note: lux is 31 bit max at this point */
+ if (ch1lux > ch0lux) {
+ dev_dbg(&client->dev, "No Data - Return last value\n");
+ ret = chip->als_cur_info.lux = 0;
+ goto out_unlock;
+ }
+
+ /* adjust for active time scale */
+ if (chip->als_time_scale == 0)
+ lux = 0;
+ else
+ lux = (lux + (chip->als_time_scale >> 1)) /
+ chip->als_time_scale;
+
+ /* adjust for active gain scale */
+ lux >>= 13; /* tables have factor of 8192 builtin for accuracy */
+ lux = (lux * chip->taos_settings.als_gain_trim + 500) / 1000;
+ if (lux > MAX_LUX) { /* check for overflow */
+return_max:
+ lux = MAX_LUX;
+ }
+
+ /* Update the structure with the latest VALID lux. */
+ chip->als_cur_info.lux = lux;
+ ret = lux;
+
+out_unlock:
+ mutex_unlock(&chip->als_mutex);
+ return ret;
+}
+
+/*
+ * Obtain single reading and calculate the als_gain_trim (later used
+ * to derive actual lux).
+ * Return updated gain_trim value.
+ */
+int taos_als_calibrate(struct i2c_client *client)
+{
+ struct tsl258x_chip *chip = i2c_get_clientdata(client);
+ u8 reg_val;
+ unsigned int gain_trim_val;
+ int ret;
+ int lux_val;
+
+ ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | TSL258X_CNTRL));
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_als_calibrate failed to reach the"
+ " CNTRL register, ret=%d\n", ret);
+ return ret;
+ }
+
+ reg_val = i2c_smbus_read_byte(client);
+ if ((reg_val & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWRON))
+ != (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWRON)) {
+ dev_err(&client->dev, "taos_als_calibrate failed because the"
+ " device is not powered on with ADC enabled\n");
+ return -ENODATA;
+ }
+
+ ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | TSL258X_STATUS));
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_als_calibrate failed to reach the"
+ " STATUS register, ret=%d\n", ret);
+ return ret;
+ }
+ reg_val = i2c_smbus_read_byte(client);
+
+ if ((reg_val & TSL258X_STA_ADCVALID) != TSL258X_STA_ADCVALID) {
+ dev_err(&client->dev, "taos_als_calibrate failed because the"
+ " STATUS did not indicate ADC valid.\n");
+ return -ENODATA;
+ }
+ lux_val = taos_get_lux(client);
+ if (lux_val < 0) {
+ dev_err(&client->dev, "taos_als_calibrate failed to get lux\n");
+ return lux_val;
+ }
+ gain_trim_val = (unsigned int) (((chip->taos_settings.als_cal_target)
+ * chip->taos_settings.als_gain_trim) / lux_val);
+
+ dev_info(&client->dev, "taos_settings.als_cal_target = %d\n"
+ "taos_settings.als_gain_trim = %d\nlux_val = %d\n",
+ chip->taos_settings.als_cal_target,
+ chip->taos_settings.als_gain_trim,
+ lux_val);
+
+ if ((gain_trim_val < 250) || (gain_trim_val > 4000)) {
+ dev_err(&client->dev, "taos_als_calibrate failed because"
+ "trim_val of %d is out of range\n", gain_trim_val);
+ return -ENODATA;
+ }
+ chip->taos_settings.als_gain_trim = (int) gain_trim_val;
+
+ return (int) gain_trim_val;
+}
+
+/*
+ * Turn the device on.
+ * Configuration must be set before calling this function.
+ */
+static int taos_chip_on(struct i2c_client *client)
+{
+ int i;
+ int ret = 0;
+ u8 *uP;
+ u8 utmp;
+ int als_count;
+ int als_time;
+ struct tsl258x_chip *chip = i2c_get_clientdata(client);
+
+ /* and make sure we're not already on */
+ if (chip->taos_chip_status == TAOS_CHIP_WORKING) {
+ /* if forcing a register update - turn off, then on */
+ dev_info(&client->dev, "device is already enabled\n");
+ return -1;
+ }
+
+ /* determine als integration regster */
+ als_count = (chip->taos_settings.als_time * 100 + 135) / 270;
+ if (als_count == 0)
+ als_count = 1; /* ensure at least one cycle */
+
+
+ /* convert back to time (encompasses overrides) */
+ als_time = (als_count * 27 + 5) / 10;
+ chip->taos_config[TSL258X_ALS_TIME] = 256 - als_count;
+
+
+ /* Set the gain based on taos_settings struct */
+ chip->taos_config[TSL258X_GAIN] = chip->taos_settings.als_gain;
+
+
+ /* set globals re scaling and saturation */
+ chip->als_saturation = als_count * 922; /* 90% of full scale */
+ chip->als_time_scale = (als_time + 25) / 50;
+
+ /* SKATE Specific power-on / adc enable sequence
+ * Power on the device 1st. */
+ utmp = TSL258X_CNTL_PWRON;
+ ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
+ utmp);
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_chip_on failed on CNTRL reg.\n");
+ return -1;
+ }
+
+ /* Use the following shadow copy for our delay before enabling ADC.
+ * Write all the registers. */
+ for (i = 0, uP = chip->taos_config; i < TAOS_REG_MAX; i++) {
+ ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG + i,
+ *uP++);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "taos_chip_on failed on reg %d.\n", i);
+ return -1;
+ }
+ }
+
+ mdelay(3);
+ /* NOW enable the ADC
+ * initialize the desired mode of operation */
+ utmp = TSL258X_CNTL_PWRON | TSL258X_CNTL_ADC_ENBL;
+ ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
+ utmp);
+ if (ret < 0) {
+ dev_err(&client->dev, "taos_chip_on failed on 2nd CTRL reg.\n");
+ return -1;
+ }
+ chip->taos_chip_status = TAOS_CHIP_WORKING;
+ return ret; /* returns result of last i2cwrite */
+}
+
+/* Turn the device OFF. */
+static int taos_chip_off(struct i2c_client *client)
+{
+ struct tsl258x_chip *chip = i2c_get_clientdata(client);
+ int ret;
+ u8 utmp;
+
+ /* turn device off */
+ chip->taos_chip_status = TAOS_CHIP_SLEEP;
+ utmp = 0x00;
+ ret = i2c_smbus_write_byte_data(client, TSL258X_CMD_REG | TSL258X_CNTRL,
+ utmp);
+ return ret;
+}
+
+/* Sysfs Interface Functions */
+static ssize_t taos_device_id(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%s\n", DEVICE_ID);
+}
+
+static ssize_t taos_power_state_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%d\n", chip->taos_chip_status);
+}
+
+static ssize_t taos_power_state_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value == 0)
+ taos_chip_off(chip->client);
+ else
+ taos_chip_on(chip->client);
+
+ return len;
+}
+
+static ssize_t taos_gain_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%d\n", chip->taos_settings.als_gain);
+}
+
+static ssize_t taos_gain_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+ if (value) {
+ if (value > 4) {
+ dev_err(dev, "Invalid Gain Index\n");
+ return -1;
+ } else {
+ chip->taos_settings.als_gain = value;
+ }
+ }
+ return len;
+}
+
+static ssize_t taos_als_time_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%d\n", chip->taos_settings.als_time);
+}
+
+static ssize_t taos_als_time_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value)
+ chip->taos_settings.als_time = value;
+
+ return len;
+}
+
+static ssize_t taos_als_trim_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%d\n", chip->taos_settings.als_gain_trim);
+}
+
+static ssize_t taos_als_trim_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value)
+ chip->taos_settings.als_gain_trim = value;
+
+ return len;
+}
+
+static ssize_t taos_als_cal_target_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+
+ return sprintf(buf, "%d\n", chip->taos_settings.als_cal_target);
+}
+
+static ssize_t taos_als_cal_target_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value)
+ chip->taos_settings.als_cal_target = value;
+
+ return len;
+}
+
+static ssize_t taos_lux_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+ int lux = 0;
+
+ lux = taos_get_lux(chip->client);
+
+ return sprintf(buf, "%d\n", lux);
+}
+
+static ssize_t taos_do_calibrate(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+ unsigned long value;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value == 1)
+ taos_als_calibrate(chip->client);
+
+ return len;
+}
+
+static ssize_t taos_luxtable_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int i;
+ int offset = 0;
+
+ for (i = 0; i < ARRAY_SIZE(taos_device_lux); i++) {
+ offset += sprintf(buf + offset, "%d,%d,%d,",
+ taos_device_lux[i].ratio,
+ taos_device_lux[i].ch0,
+ taos_device_lux[i].ch1);
+ if (taos_device_lux[i].ratio == 0) {
+ /* We just printed the first "0" entry.
+ * Now get rid of the extra "," and break. */
+ offset--;
+ break;
+ }
+ }
+
+ offset += sprintf(buf + offset, "\n");
+ return offset;
+}
+
+static ssize_t taos_luxtable_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+#define MAX_LUX_TABLE_FIELDS 33
+ int value[MAX_LUX_TABLE_FIELDS];
+ int n;
+
+ get_options(buf, ARRAY_SIZE(value), value);
+
+ /* We now have an array of ints starting at value[1], and
+ * enumerated by value[0].
+ * We expect each group of three ints is one table entry,
+ * and the last table entry is all 0.
+ */
+ n = value[0];
+ if ((n % 3) || n < 6 || n > (MAX_LUX_TABLE_FIELDS - 3)) {
+ dev_info(dev, "LUX TABLE INPUT ERROR 1 Value[0]=%d\n", n);
+ return -EINVAL;
+ }
+ if ((value[(n - 2)] | value[(n - 1)] | value[n]) != 0) {
+ dev_info(dev, "LUX TABLE INPUT ERROR 2 Value[0]=%d\n", n);
+ return -EINVAL;
+ }
+
+ if (chip->taos_chip_status == TAOS_CHIP_WORKING)
+ taos_chip_off(chip->client);
+
+ /* Zero out the table */
+ memset(taos_device_lux, 0, sizeof(taos_device_lux));
+ memcpy(taos_device_lux, &value[1], (value[0] * 4));
+
+ taos_chip_on(chip->client);
+
+ return len;
+}
+
+static u8 reg_index;
+
+/* Sets a pointer to a register for R/W via sysfs */
+static ssize_t taos_reg_offset_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%d\n", reg_index);
+ return 0;
+}
+
+static ssize_t taos_reg_offset_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ unsigned long value;
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ if (value > MAX_DEVICE_REGS) {
+ dev_err(dev, "register offset exceeds MAX\n");
+ return -1;
+ } else {
+ reg_index = value;
+ }
+return len;
+}
+
+/* R/W a register for R/W via sysfs */
+static ssize_t taos_reg_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+
+ u8 value = 0;
+ int ret = 0;
+
+ ret = i2c_smbus_write_byte(chip->client,
+ (TSL258X_CMD_REG | reg_index));
+ if (ret < 0) {
+ dev_err(dev, "i2c_smbus_write_byte to cmd reg failed "
+ "in taos_reg_offset_show(), err = %d\n", ret);
+ return ret;
+ }
+ value = i2c_smbus_read_byte(chip->client);
+ return sprintf(buf, "%d\n", value);
+
+return 0;
+}
+
+static ssize_t taos_reg_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tsl258x_chip *chip = indio_dev->dev_data;
+
+ unsigned long value = 0;
+ int ret = 0;
+
+ if (strict_strtoul(buf, 0, &value))
+ return -EINVAL;
+
+ ret = i2c_smbus_write_byte_data(chip->client,
+ (TSL258X_CMD_REG | reg_index), value);
+ if (ret < 0) {
+ dev_err(dev, "i2c_smbus_write_byte_data failed in "
+ "taos_reg_store()\n");
+ return ret;
+ }
+
+return len;
+}
+
+static DEVICE_ATTR(name, S_IRUGO, taos_device_id, NULL);
+static DEVICE_ATTR(device_state, S_IRUGO | S_IWUSR,
+ taos_power_state_show, taos_power_state_store);
+static DEVICE_ATTR(als_gain, S_IRUGO | S_IWUSR,
+ taos_gain_show, taos_gain_store);
+static DEVICE_ATTR(als_time, S_IRUGO | S_IWUSR,
+ taos_als_time_show, taos_als_time_store);
+static DEVICE_ATTR(als_trim, S_IRUGO | S_IWUSR,
+ taos_als_trim_show, taos_als_trim_store);
+static DEVICE_ATTR(als_target, S_IRUGO | S_IWUSR,
+ taos_als_cal_target_show, taos_als_cal_target_store);
+static DEVICE_ATTR(lux, S_IRUGO, taos_lux_show, NULL);
+static DEVICE_ATTR(calibrate, S_IWUSR, NULL, taos_do_calibrate);
+static DEVICE_ATTR(lux_table, S_IRUGO | S_IWUSR,
+ taos_luxtable_show, taos_luxtable_store);
+static DEVICE_ATTR(reg_offset, S_IRUGO | S_IWUSR,
+ taos_reg_offset_show,
+ taos_reg_offset_store);
+static DEVICE_ATTR(reg, S_IRUGO | S_IWUSR,
+ taos_reg_show,
+ taos_reg_store);
+
+
+static struct attribute *sysfs_attrs_ctrl[] = {
+ &dev_attr_name.attr,
+ &dev_attr_device_state.attr,
+ &dev_attr_als_gain.attr,
+ &dev_attr_als_time.attr,
+ &dev_attr_als_trim.attr,
+ &dev_attr_als_target.attr,
+ &dev_attr_lux.attr,
+ &dev_attr_calibrate.attr,
+ &dev_attr_lux_table.attr,
+ &dev_attr_reg_offset.attr,
+ &dev_attr_reg.attr,
+ NULL
+};
+
+static struct attribute_group tsl258x_attribute_group = {
+ .attrs = sysfs_attrs_ctrl,
+};
+
+/* Use the default register values to identify the Taos device */
+static int taos_skate_device(unsigned char *bufp)
+{
+ if (((bufp[TSL258X_CHIPID] & 0xf0) == 0x90))
+ /* tsl258x */
+ return 1;
+ /* else unknown */
+ return 0;
+}
+
+/*
+ * Client probe function - When a valid device is found, the driver's device
+ * data structure is updated, and initialization completes successfully.
+ */
+static int __devinit taos_probe(struct i2c_client *clientp,
+ const struct i2c_device_id *idp)
+{
+ int i, ret = 0;
+ unsigned char buf[MAX_DEVICE_REGS];
+ static struct tsl258x_chip *chip;
+
+ if (!i2c_check_functionality(clientp->adapter,
+ I2C_FUNC_SMBUS_BYTE_DATA)) {
+ dev_err(&clientp->dev,
+ "taos_probe() - i2c smbus byte data "
+ "functions unsupported\n");
+ return -EOPNOTSUPP;
+ }
+ if (!i2c_check_functionality(clientp->adapter,
+ I2C_FUNC_SMBUS_WORD_DATA)) {
+ dev_warn(&clientp->dev,
+ "taos_probe() - i2c smbus word data "
+ "functions unsupported\n");
+ }
+ if (!i2c_check_functionality(clientp->adapter,
+ I2C_FUNC_SMBUS_BLOCK_DATA)) {
+ dev_err(&clientp->dev,
+ "taos_probe() - i2c smbus block data "
+ "functions unsupported\n");
+ }
+
+ chip = kmalloc(sizeof(struct tsl258x_chip), GFP_KERNEL);
+ if (!chip) {
+ dev_err(&clientp->dev, "taos_device_drvr: kmalloc for "
+ "struct tsl258x_chip failed in taos_probe()\n");
+ return -ENOMEM;
+ }
+ memset(chip, 0, sizeof(struct tsl258x_chip));
+ chip->client = clientp;
+ i2c_set_clientdata(clientp, chip);
+
+ mutex_init(&chip->als_mutex);
+ chip->taos_chip_status = TAOS_CHIP_UNKNOWN; /* uninitialized to start */
+ memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
+
+ for (i = 0; i < MAX_DEVICE_REGS; i++) {
+ ret = i2c_smbus_write_byte(clientp,
+ (TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
+ if (ret < 0) {
+ dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd "
+ "reg failed in taos_probe(), err = %d\n", ret);
+ goto fail1;
+ }
+ buf[i] = i2c_smbus_read_byte(clientp);
+ }
+ if (!taos_skate_device(buf)) {
+ dev_info(&clientp->dev, "i2c device found but does not match "
+ "expected id in taos_probe()\n");
+ goto fail1;
+ } else {
+ dev_info(&clientp->dev, "i2c device match in probe\n");
+ }
+ ret = i2c_smbus_write_byte(clientp, (TSL258X_CMD_REG | TSL258X_CNTRL));
+ if (ret < 0) {
+ dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd reg "
+ "failed in taos_probe(), err = %d\n", ret);
+ goto fail1;
+ }
+ chip->valid = 0;
+
+ chip->iio_dev = iio_allocate_device();
+ if (!chip->iio_dev) {
+ ret = -ENOMEM;
+ dev_err(&clientp->dev, "iio allocation failed\n");
+ goto fail1;
+ }
+
+ chip->iio_dev->attrs = &tsl258x_attribute_group;
+ chip->iio_dev->dev.parent = &clientp->dev;
+ chip->iio_dev->dev_data = (void *)(chip);
+ chip->iio_dev->driver_module = THIS_MODULE;
+ chip->iio_dev->modes = INDIO_DIRECT_MODE;
+ ret = iio_device_register(chip->iio_dev);
+ if (ret) {
+ dev_err(&clientp->dev, "iio registration failed\n");
+ goto fail1;
+ }
+
+ /* Load up the V2 defaults (these are hard coded defaults for now) */
+ taos_defaults(chip);
+
+ /* Make sure the chip is on */
+ taos_chip_on(clientp);
+
+ dev_info(&clientp->dev, "Light sensor found.\n");
+ return 0;
+
+fail1:
+ kfree(chip);
+
+ return ret;
+}
+
+static int __devexit taos_remove(struct i2c_client *client)
+{
+ struct tsl258x_chip *chip = i2c_get_clientdata(client);
+
+ iio_device_unregister(chip->iio_dev);
+
+ kfree(chip);
+ return 0;
+}
+
+static struct i2c_device_id taos_idtable[] = {
+ { DEVICE_ID, 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, taos_idtable);
+
+/* Driver definition */
+static struct i2c_driver taos_driver = {
+ .driver = {
+ .name = "tsl258x",
+ },
+ .id_table = taos_idtable,
+ .probe = taos_probe,
+ .remove = __devexit_p(taos_remove),
+};
+
+static int __init taos_init(void)
+{
+ return i2c_add_driver(&taos_driver);
+}
+
+static void __exit taos_exit(void)
+{
+ i2c_del_driver(&taos_driver);
+}
+
+module_init(taos_init);
+module_exit(taos_exit);
+
+MODULE_AUTHOR("J. August Brenner<jbrenner@taosinc.com>");
+MODULE_DESCRIPTION("TAOS tsl258x ambient light sensor driver");
+MODULE_LICENSE("GPL");
+
--
1.7.0.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-03-24 20:09 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-14 19:45 [PATCH 2.6.38-rc7]TAOS 258x: Device Driver Jon Brenner
2011-03-21 19:07 ` Jonathan Cameron
2011-03-23 1:07 ` Jon Brenner
2011-03-23 11:06 ` Jonathan Cameron
2011-03-23 22:38 ` Jon Brenner
2011-03-24 11:15 ` Jonathan Cameron
2011-03-24 19:04 ` Jon Brenner
2011-03-24 20:10 ` Jonathan Cameron
-- strict thread matches above, loose matches on Subject: below --
2011-03-09 1:32 Jon Brenner
2011-03-09 19:14 ` Jonathan Cameron
2011-03-12 1:16 ` Jon Brenner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).