On 04/27/11 20:41, Jon Brenner wrote: > [PATCH v1] for TAOS tsl2671 (Proximity Only) Device driver > > Driver includes (iio_event/interrupt). Hi Jon, Mostly fine, but quite a few minor bits and pieces need cleaning up. This will clash with some of the changes to IIO that are working their way through review. Hence you may well have to do a fair bit of simple fixing up before this merges (or I can do it if you can test). Jonathan > > Signed-off-by: Jon Brenner <jbrenner@xxxxxxxxxxx> > > --- > drivers/staging/iio/light/Kconfig | 9 + > drivers/staging/iio/light/Makefile | 1 + > drivers/staging/iio/light/tsl2671.c | 1080 +++++++++++++++++++++++++++++++++++ > 3 files changed, 1090 insertions(+), 0 deletions(-) > > diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig > index 36d8bbe..3ead1c5 100644 > --- a/drivers/staging/iio/light/Kconfig > +++ b/drivers/staging/iio/light/Kconfig > @@ -24,3 +24,12 @@ config SENSORS_ISL29018 > in lux, proximity infrared sensing and normal infrared sensing. > Data from sensor is accessible via sysfs. > > +config SENSORS_TSL2671 > + tristate "TAOS TSL2671 proximity sensor" > + depends on I2C > + help > + Y = in kernel. > + M = as module. > + Provides support for TAOS tsl2671 devices. > + Access PROXIMITY data via iio, sysfs. Just curious, but why put proximity in capitals? > + > diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile > index 9142c0e..0528443 100644 > --- a/drivers/staging/iio/light/Makefile > +++ b/drivers/staging/iio/light/Makefile > @@ -4,3 +4,4 @@ > > obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o > obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o > +obj-$(CONFIG_SENSORS_TSL2671) += tsl2671.o > diff --git a/drivers/staging/iio/light/tsl2671.c b/drivers/staging/iio/light/tsl2671.c > new file mode 100644 > index 0000000..a521130 > --- /dev/null > +++ b/drivers/staging/iio/light/tsl2671.c > @@ -0,0 +1,1080 @@ > +/* > + * Device driver for the TAOS TSL2671 proximity sensor. > + * 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 <linux/unistd.h> > +#include <linux/interrupt.h> > +#include <linux/input.h> > +#include <linux/slab.h> > +#include "../iio.h" > +#include "../sysfs.h" > + > +/* 2571 register offsets */ Wrong part number in comment. > +#define TSL2671_MAX_DEVICE_REGS 32 > +#define TSL2671_REG_MAX 16 Err. THose two definitions aren't exactly informative Also, given they are only used in two places each, I'd just hard code them into the relevant places. Having them here just adds to the noise... e.g. unsigned char buf[32]; ... for (i = 0; i < ARRAY_SIZE(buf); i++) { ... > + > +/* Device Registers and Masks */ > +#define TSL2671_CNTRL 0x00 > +#define TSL2671_PRX_MINTHRESHLO 0X08 Next 3 aren't used. Perhaps remain the above to TSL2671_PRX_THRESH_BASE given that is how you use it. > +#define TSL2671_PRX_MINTHRESHHI 0X09 > +#define TSL2671_PRX_MAXTHRESHLO 0X0A > +#define TSL2671_PRX_MAXTHRESHHI 0X0B > + > +#define TSL2671_PERSISTENCE 0x0c > + > +#define TSL2671_PRX_COUNT 0x0E > +#define TSL2671_GAIN 0x0f > +#define TSL2671_STATUS 0x13 Inconsistent tab / space here. > +#define TSL2671_REVID 0x11 Not used, if not intending to use, get rid of it. > +#define TSL2671_CHIPID 0x12 > +#define TSL2671_PRX_LO 0x18 > +#define TSL2671_PRX_HI 0x19 > + > +/* tsl2671 cmd reg masks */ > +#define TSL2671_CMD_REG 0x80 > +#define TSL2671_CMD_SPL_FN 0x60 > + > +#define CMD_PROXALS_INT_CLR 0X07 > + > +/* tsl2671 cntrl reg masks */ > +#define TSL2671_CNTL_ADC_ENBL 0x02 > +#define TSL2671_CNTL_PWR_ON 0x01 > + > +/* tsl2671 status reg masks */ > +#define TSL2671_STA_ADC_VALID 0x01 > +#define TSL2671_STA_PRX_VALID 0x02 > +#define TSL2671_STA_ADC_PRX_VALID 0x03 > +#define STA_ADCINTR 0x10 > +#define STA_PRXINTR 0x20 > + > +#define TSL2671_STA_ADC_INTR 0x10 > + > +/* Triton cntrl reg masks */ Please prefix all of these with TSL2671 rather than just some of them! Er, not only is this unused, it's also kind of obvious! > +#define CNTL_REG_CLEAR 0x00 > +#define CNTL_PROX_INT_ENBL 0X20 > +#define TSL2671_CNTL_WAIT_TMR_ENBL 0X08 Not used.. Any intention to add whatever this is for? > +#define CNTL_PROX_DET_ENBL 0X04 > +#define CNTL_ADC_ENBL 0x02 This is a repeat of TSL2671_CNTL_ADC_ENBL above. > +#define TSL2671_CNTL_PWRON 0x01 Repeat of TSL2671_CNTRL_PWR_ON above. Neither of next two is used. > +#define CNTL_PROXPON_ENBL 0x0F > +#define CNTL_INTPROXPON_ENBL 0x2F > +#define TSL2671_CMD_PROXALS_INTCLR 0X07 > + > +/*Prox diode to use */ > +#define DIODE0 0x10 Not used and needs a prefix anyway > +#define DIODE1 0x20 > +#define DIODE_BOTH 0x30 Reasonable to have first two, but then have #define TSL2671_DIODE_BOTH (TSL2671_DIODE0, TSL2671_DIODE2) > + > +/* LED Power */ These defintely need prefixes. > +#define mA100 0x00 > +#define mA50 0x40 > +#define mA25 0x80 > +#define mA13 0xD0 > + > +/* Cal defs*/ > +#define PROX_STAT_CAL 0 Not used > +#define PROX_STAT_SAMP 1 > +#define MAX_SAMPLES_CAL 200 > + > +enum { > + TSL2671_CHIP_UNKNOWN = 0, > + TSL2671_CHIP_WORKING = 1, > + TSL2671_CHIP_SUSPENDED = 2 > +} TSL2671_CHIP_WORKING_STATUS; > + > +/* proximity data */ > +struct taos_prox_info { > + u16 prox_data; > + int prox_event; > +}; > + > +struct prox_stat { > + u16 min; > + u16 max; > + u16 mean; > + unsigned long stddev; > +} ; > + > +struct taos_settings { > + u8 interrupt; > + u8 persistence; > + int prox_thres; > + int prox_pulse_count; > + int prox_max_samples_cal; /* for calibration mode*/ > + > +}; > + > +#define MAXPROXFILTER 100 > + > +u16 taos_prox_filter_data[MAXPROXFILTER]; /*history buffer*/ > + > +struct tsl2671_chip { > + struct mutex prox_mutex; > + struct i2c_client *client; > + struct iio_dev *iio_dev; > + struct taos_prox_info prox_cur_info; > + struct taos_settings taos_settings; > + int taos_chip_status; > + u8 taos_config[TSL2671_REG_MAX]; > + bool init_done; > + struct work_struct work_thresh; > + s64 event_timestamp; > + unsigned int irq_no; Why mirror client->irq? > +}; > + > +/* > + * Initial values for device - this values can/will be changed by driver. > + * and applications as needed. > + * These values are dynamic. > + */ > +static u8 tsl2671_taos_config[] = { > + 0x00, 0xee, 0xFF, 0xF5, 0x00, 0x00, 0x00, 0x00, > + /* Enabl itime ptime wtime NA NA NA NA */ > + 0x00, 0x00, 0x00, 0x03, 0x10, 0x00, 0x0a, 0x20 > + /* PtL0 PtL1 PtH0 PtH1 Pers CFG Pcnt CTRL */ > +}; > + > +/* > + * 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) As far as I can see len is always 1. Hence this function can be greatly simplified. Either that, or in the one paired read below, actually use this function with a length of 2! > +{ > + int ret; > + int i; > + > + for (i = 0; i < len; i++) { > + /* select register to write */ > + ret = i2c_smbus_write_byte(client, (TSL2671_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; > +} > + > +/* > + * Proximity poll function - if valid data is available, read and form the ch0 > + * and prox data values, check for limits on the ch0 value, and check the prox > + * data against the current thresholds, to set the event status accordingly. > + */ > +int taos_prox_poll(struct i2c_client *client) > +{ > +#define CONSECUTIVE_RETRIES 50 > + > +int i; > +int ret; > +u8 status; > +u8 chdata[2]; > +int err_cnt; > +struct tsl2671_chip *chip = i2c_get_clientdata(client); > + > + if (mutex_trylock(&chip->prox_mutex) == 0) { > + dev_err(&client->dev, "Can't get prox mutex\n"); > + return -1; > + } > + > + err_cnt = 0; > + > +tryAgain: > + ret = taos_i2c_read(client, > + (TSL2671_CMD_REG | TSL2671_STATUS), &status, 1); > + if (ret < 0) { > + dev_err(&client->dev, > + "Read regs failed in taos_prox_poll() - A\n"); > + mutex_unlock(&chip->prox_mutex); > + return ret; > + } > + > + /*Prox interrupt asserted*/ > + if (((chip->taos_settings.interrupt << 4) & CNTL_PROX_INT_ENBL)) { > + if (!(status & TSL2671_STA_ADC_VALID)) { > + err_cnt++; > + if (err_cnt > CONSECUTIVE_RETRIES) { > + mutex_unlock(&chip->prox_mutex); > + dev_err(&client->dev, "Consec. retries exceeded\n"); > + return chip->prox_cur_info.prox_event; I don't understand why you are returning this here... > + } > + goto tryAgain; Any retry type condiitions like this needs some explanatory comments. Why might it fail? > + } > + } > + > + for (i = 0; i < 2; i++) { > + ret = taos_i2c_read(client, > + (TSL2671_CMD_REG | > + (TSL2671_PRX_LO + i)), &chdata[i], 1); > + if (ret < 0) { > + dev_err(&client->dev, > + "Read regs failed in taos_prox_poll() - B\n"); > + mutex_unlock(&chip->prox_mutex); > + return ret; > + } > + } > + This in combination with the above is clearly a 16 bit read. I'd therefore make chdata a u16 and read directly into it - type casting pointers as needed. Then you can be sure it is 16 bit aligned and hence use the be16_to_cpu or similar (that's free if you happen to be on the correct endianness) Or more to the point, just transfer directly into chip->prox_cur_info.prox_data. Also, why not use the fact your taos_i2c_read takes a length of 2. Something like (possibly with wrong endian conversion, I haven't woken up yet today!) ret = taos_i2c_read(client, (TSL2671_CMD_REG |(TSL2671_PRX_LO + i)), &chip->prox_cur_info.prox_data, 2); if (ret < 0) return ret; /* Note I wouldn't bother with error message in something that should never happen on a sensible hardware setup. */ chip->prox_cur_info.prox_data = be16_to_cpu(chip->prox_cur_info.prox_data); > + chip->prox_cur_info.prox_data = (chdata[1]<<8)|chdata[0]; > + > + if (chip->prox_cur_info.prox_data >= chip->taos_settings.prox_thres) > + chip->prox_cur_info.prox_event = 1; chip->prox_cur_info.prox_event = chip->prox_cur_info.prox_data >= chip->taos_settings.prox_thres; > + else > + chip->prox_cur_info.prox_event = 0; > + > + mutex_unlock(&chip->prox_mutex); > + return chip->prox_cur_info.prox_event; > +} > + > +/* > + * Proximity detect interrupt BH - called when proximity of an object > + * to the sensor is detected (event = 1), or, once detected, it has moved away > + * from the sensor (event = 0). > + * Prox info is stored into the structure "prox_cur_inf", err, prox_cur_info seems to exist. > + * and signal is issued to any waiting user mode threads - > + * which must be (of course) registered to be signaled. > + * This is the called from the 'bottom half' of IRQ. > + */ > +void taos_prox_adjust_level(struct work_struct *prox) > +{ > + int ret = 0, i; > + u16 cdelta; > + u8 prox_low[2]; > + u8 prox_high[2]; These look like 16 bit values to me. Probably best to treat them as such. > + struct tsl2671_chip *chip > + = container_of(prox, > + struct tsl2671_chip, work_thresh); > + > + taos_prox_poll(chip->client); > + > + if (chip->prox_cur_info.prox_data > chip->taos_settings.prox_thres) { > + dev_info(&chip->client->dev, > + "prox_data > prox_thres"); No reason to break the above line. > + /*Rail the threshold so we don't keep interrupting*/ Rail? > + prox_high[0] = 0xFF; > + prox_high[1] = 0xFF; > + for (i = 0; i < 2; i++) { > + ret = i2c_smbus_write_byte_data(chip->client, > + (TSL2671_CMD_REG | > + (TSL2671_PRX_MAXTHRESHLO + i)), > + prox_high[i]); > + > + if (ret < 0) > + dev_err(&chip->client->dev, > + "FAILED: to update PROX HIGH THRESH (A).\n"); > + } > + Perhaps we have a call for a tsl2671_write_data function similar to your read one. > + cdelta = chip->taos_settings.prox_thres - 100; > + > + if (cdelta < 10) > + cdelta = chip->taos_settings.prox_thres - 1; This black magic could do with some explanation. > + > + prox_low[0] = (cdelta) & 0xFF; > + prox_low[1] = (cdelta >> 8) & 0xFF; Definitely a 16 bit value, don't pretend it's 8 bit. > + for (i = 0; i < 2; i++) { > + i2c_smbus_write_byte_data(chip->client, > + (TSL2671_CMD_REG | > + (TSL2671_PRX_MINTHRESHLO + i)), > + prox_low[i]); > + > + if (ret < 0) > + dev_err(&chip->client->dev, > + "FAILED: to update the PROX LOW THRESH (B).\n"); > + } > + } else if (chip->prox_cur_info.prox_data < > + chip->taos_settings.prox_thres) { > + dev_info(&chip->client->dev, > + "prox_data <= prox_thres"); Why break line? > + prox_low[0] = 0x00; > + prox_low[1] = 0x00; > + for (i = 0; i < 2; i++) { > + ret = i2c_smbus_write_byte_data(chip->client, > + (TSL2671_CMD_REG | > + (TSL2671_PRX_MINTHRESHLO + i)), > + prox_low[i]); > + > + if (ret < 0) > + dev_err(&chip->client->dev, > + "FAILED: to update the PROX LOW THRESH (C).\n"); Again, don't muddle through. Drop out with an error. > + } > + /*Lastly we put the high threshols back to where we started*/ threshols->thresholds > + prox_high[0] = (chip->taos_settings.prox_thres) & 0xFF; > + prox_high[1] = (chip->taos_settings.prox_thres >> 8) & 0xFF; > + for (i = 0; i < 2; i++) { > + ret = i2c_smbus_write_byte_data(chip->client, > + (TSL2671_CMD_REG | > + (TSL2671_PRX_MAXTHRESHLO + i)), > + prox_high[i]); > + > + if (ret < 0) > + dev_err(&chip->client->dev, > + "FAILED: to update PROX HIGH THRESH (D).\n"); > + } > + > + } > + > +return; > +} > + > +/* > + * Proximity interrupt BH - called when the proximity threshold > + * falls above or below a set value. > + */ > +void taos_interrupt_bh(struct work_struct *prox) > +{ > +int ret; > +int value; > +u8 reg_val; > + > +struct tsl2671_chip *chip > + = container_of(prox, > + struct tsl2671_chip, work_thresh); > + > + value = i2c_smbus_read_byte_data(chip->client, > + TSL2671_CMD_REG | TSL2671_STATUS); > + > + if ((value & STA_PRXINTR) && > + (chip->taos_settings.interrupt & CNTL_PROX_INT_ENBL)) { > + iio_push_event(chip->iio_dev, 0, > + IIO_UNMOD_EVENT_CODE(IIO_EV_CLASS_PROXIMITY, > + 0, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_EITHER), I haven't looked at datasheet, but just to confirm, this will trigger if we cross the threshold in either direction? Eg. proximity rises or proximity falls? > + chip->event_timestamp); > + taos_prox_adjust_level(prox); > + } > + Please explain why we want this init in here? > + if (!chip->init_done) { > + /* Maintain the persistence value */ > + ret = taos_i2c_read(chip->client, > + (TSL2671_CMD_REG | (TSL2671_PERSISTENCE)), > + ®_val, 1); > + if (ret < 0) > + dev_info(&chip->client->dev, > + "Failed to get the persistence register value\n"); Why do we read this? We if it is required to clear an interrupt or similar, then please document. > + Tabbing looks wrong here. > + reg_val = chip->taos_settings.persistence; > + > + ret = i2c_smbus_write_byte_data(chip->client, > + (TSL2671_CMD_REG | TSL2671_PERSISTENCE), reg_val); > + if (ret < 0) > + dev_info(&chip->client->dev, > + "FAILED: update the persistence (B).\n"); > + } > + > + chip->init_done = 1; Indentation is wrong / confusing... > + > + /* Clear out any initial irq status */ > + ret = i2c_smbus_write_byte(chip->client, > + (TSL2671_CMD_REG | Odd parameter indentation. > + TSL2671_CMD_SPL_FN | > + TSL2671_CMD_PROXALS_INTCLR)); > + if (ret < 0) > + dev_info(&chip->client->dev, > + "taos_interrupt_bh FAILED to clear irqs: err = %d\n", ret); I'd drop out without rennabling the interrupt on any error. If the device isn't talking any result may be rubbish. > + > + enable_irq(chip->irq_no); > + > +return; > +} > + > +/* > + * Provides initial operational parameter defaults. > + * These defaults may be changed through the device's ABI. > + */ > +static void taos_defaults(struct tsl2671_chip *chip) > +{ > + /* Operational parameters */ > + chip->taos_settings.persistence = 0x10; > + /* Number of 'out of limits' ADC readings */ > + chip->taos_settings.interrupt = 0x30; > + /* Default interrupt enabled. */ > + chip->taos_settings.prox_thres = 512; > + /*default threshold (adjust either manually or with cal routine*/ > + chip->taos_settings.prox_max_samples_cal = 100; > + chip->taos_settings.prox_pulse_count = 10; Unwanted white space. All of these look like they should be platform data to me. > + > +} > + > +/* > + * 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; > + struct tsl2671_chip *chip = i2c_get_clientdata(client); > + u8 reg_val; > + > + /* Non calculated parameters */ > + chip->taos_config[TSL2671_PERSISTENCE] = > + chip->taos_settings.persistence; > + > + chip->taos_config[TSL2671_PRX_COUNT] = > + chip->taos_settings.prox_pulse_count; > + chip->taos_config[TSL2671_PRX_MINTHRESHLO] = 0; > + chip->taos_config[TSL2671_PRX_MAXTHRESHLO] = > + chip->taos_settings.prox_thres; > + > + /* and make sure we're not already on */ > + if (chip->taos_chip_status == TSL2671_CHIP_WORKING) { > + /* if forcing a register update - turn off, then on */ > + dev_info(&client->dev, "device is already enabled\n"); > + return -EINVAL; > + } > + > + /* Set the emitter and gain based on taos_settings struct */ > + chip->taos_config[TSL2671_GAIN] = (mA100 | DIODE_BOTH); > + > + /* TSL2671 Specific power-on / adc enable sequence > + * Power on the device 1st. */ > + utmp = TSL2671_CNTL_PWR_ON; > + ret = i2c_smbus_write_byte_data(client, > + TSL2671_CMD_REG | TSL2671_CNTRL, utmp); > + if (ret < 0) { > + dev_err(&client->dev, "taos_chip_on failed on CNTRL reg.\n"); return ret; > + 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 < TSL2671_REG_MAX; i++) { > + ret = i2c_smbus_write_byte_data(client, TSL2671_CMD_REG + i, > + *uP++); I'd not bother introducing uP. Just use chip->taos_config[i]. > + if (ret < 0) { > + dev_err(&client->dev, > + "taos_chip_on failed on write to reg %d.\n", i); > + return ret; > + } > + } > + > + msleep(3); > + /* NOW enable the ADC */ > + utmp = TSL2671_CNTL_PWR_ON | TSL2671_CNTL_ADC_ENBL; > + ret = i2c_smbus_write_byte_data(client, TSL2671_CMD_REG | TSL2671_CNTRL, > + utmp); Why introduce utmp? Just put value in directly. > + if (ret < 0) { > + dev_err(&client->dev, "taos_chip_on failed on 2nd CTRL reg.\n"); > + return ret; > + } > + > + chip->taos_chip_status = TSL2671_CHIP_WORKING; > + > + /* If interrupts are enabled */ > + chip->init_done = 0; > + > + if (chip->taos_settings.interrupt) { > + dev_info(&client->dev, "Setting Up Interrupt(s)\n"); > + /* first time interrupt */ You just did this a couple of lines up. > + chip->init_done = 0; > + > + /*First make sure we have persistence > 0 > + else we'll interrupt continuously.*/ > + ret = taos_i2c_read(client, > + (TSL2671_CMD_REG | TSL2671_PERSISTENCE), ®_val, 1); > + if (ret < 0) > + dev_err(&client->dev, > + "Failed to get the persistence register value\n"); > + > + /*Prox Interrupt after 1 reading out of range */ > + if ((reg_val & 0x0F) == 0) { > + reg_val |= 0x10; > + ret = i2c_smbus_write_byte_data(client, > + (TSL2671_CMD_REG | (TSL2671_PERSISTENCE)), reg_val); > + > + if (ret < 0) > + dev_err(&client->dev, > + "taos_i2c_write to update the persistance register.\n"); Do not continue after such an error. It's all gone wrong, so pass this up all the way. > + } > + > + reg_val = TSL2671_CNTL_PWR_ON | CNTL_ADC_ENBL | CNTL_PROX_DET_ENBL; > + reg_val |= chip->taos_settings.interrupt; > + > + ret = i2c_smbus_write_byte_data(client, > + (TSL2671_CMD_REG | TSL2671_CNTRL), reg_val); > + if (ret < 0) > + dev_err(&client->dev, > + "taos_i2c_write to device failed in TAOS_IOCTL_INT_SET.\n"); Again, please don't eat errros. > + > + /* Clear out any initial interrupt */ > + ret = i2c_smbus_write_byte(client, > + TSL2671_CMD_REG | TSL2671_CMD_SPL_FN | > + TSL2671_CMD_PROXALS_INTCLR); > + if (ret < 0) { > + dev_err(&client->dev, > + "taos_i2c_write_command failed in taos_chip_on\n"); > + return ret; Full through to the return ret below. > + } > + } > + > + return ret; Unwanted blank line here. > + > +} > + > +static int taos_chip_off(struct i2c_client *client) > +{ > + struct tsl2671_chip *chip = i2c_get_clientdata(client); > + > + /* turn device off */ > + chip->taos_chip_status = TSL2671_CHIP_SUSPENDED; > + return i2c_smbus_write_byte_data(client, > + TSL2671_CMD_REG | TSL2671_CNTRL, 0x00); > +} > + > +/** > + * Integer Square Root > + * We need an integer version since 1st Floating point is not allowed > + * in driver world, 2nd, cannot count on the devices having a FPU, and > + * 3rd software FP emulation may be excessive. > + */ > +unsigned long taos_isqrt(unsigned long x) In kernel.h unsigned long int_sqrt(unsigned long); Do the job? It's pretty much identical to this. > +{ > +register unsigned long op, res, one; > +op = x; > +res = 0; > + > + one = 1 << 30; > + while (one > op) > + one >>= 2; > + > + while (one != 0) { > + if (op >= res + one) { > + op -= res + one; > + res += one << 1; > + } > + res >>= 1; > + one >>= 2; > + } > + return res; > +} > + > +/* > + * Proximity calibration helper function > + * runs through a collection of data samples, > + * sets the min, max, mean, and std dev. Probably wants a name to indicate it is a helper that doesn't actually calcualte the proximity as it's name might suggest... > + */ > +void taos_prox_calculate(u16 *data, int length, struct prox_stat *statP) > +{ > + > +int i; > +int min, max, sum, mean; > +unsigned long stddev; > +int tmp; Checkpatch.pl on this patch please. Fair few indent issues. > + > + if (length == 0) > + length = 1; > + > + sum = 0; > + min = 0xffff; > + max = 0; Set these when they are defined. > + for (i = 0; i < length; i++) { > + sum += data[i]; min = min(data[i], min); max = max(data[i], max); > + if (data[i] < min) > + min = data[i]; > + if (data[i] > max) > + max = data[i]; > + } > + mean = sum/length; > + statP->min = min; > + statP->max = max; > + statP->mean = mean; > + > + sum = 0; > + for (i = 0; i < length; i++) { > + tmp = data[i]-mean; > + sum += tmp * tmp; > + } > + stddev = taos_isqrt((long)sum)/length; > + statP->stddev = stddev; > + Unwanted blank line at end of function. > +} > + > +/* > + * Proximity calibration - collects a number of samples, > + * calculates a standard deviation based on the samples, > + * and sets the threshold accordingly. Funky. This may well be a trick we want to pull out to a library function at some point. Seems quite a useful bit of code to me. > + */ > +void taos_prox_cal(struct i2c_client *client) > +{ > +u16 prox_history[MAX_SAMPLES_CAL+1]; > +int i; > +struct prox_stat prox_stat_data[2]; > +struct prox_stat *calP; > +struct tsl2671_chip *chip = i2c_get_clientdata(client); > +u8 tmp_irq_settings; > + > + if (chip->taos_settings.prox_max_samples_cal > MAX_SAMPLES_CAL) { > + dev_err(&client->dev, > + "max prox samples cal is too big: %d\n", > + chip->taos_settings.prox_max_samples_cal); > + chip->taos_settings.prox_max_samples_cal = MAX_SAMPLES_CAL; > + } > + > + /* have to stop to change settings */ > + taos_chip_off(chip->client); > + > + /* Enable proximity detection save just in case prox not wanted yet*/ > + tmp_irq_settings = chip->taos_settings.interrupt; > + chip->taos_settings.interrupt |= CNTL_PROX_INT_ENBL; > + > + /*turn on device if not already on*/ > + taos_chip_on(chip->client); > + > + /*gather the samples*/ > + for (i = 0; i < chip->taos_settings.prox_max_samples_cal; i++) { > + mdelay(15); Why this particular delay? That's a fair time to hog the processor. Particularly with a default giving 1.5 seconds! Perhaps we need to do this asynchronously? Maybe leave that for another time. This will cause issues when we move out of staging though. > + taos_prox_poll(chip->client); > + prox_history[i] = chip->prox_cur_info.prox_data; > + dev_info(&client->dev, "2 i=%d prox data= %d\n", > + i, chip->prox_cur_info.prox_data); > + Unwanted blank line. > + } > + > + taos_chip_off(chip->client); > + > + calP = &prox_stat_data[PROX_STAT_CAL]; > + taos_prox_calculate(prox_history, > + chip->taos_settings.prox_max_samples_cal, calP); > + chip->taos_settings.prox_thres = (calP->max << 1) - calP->mean; > + > + dev_info(&client->dev, " cal min=%d mean=%d max=%d\n", > + calP->min, calP->mean, calP->max); > + dev_info(&client->dev, > + "TAOS: proximity threshold set to %d, basic mode\n", > + chip->taos_settings.prox_thres); > + > + /* back to the way they were */ > + chip->taos_settings.interrupt = tmp_irq_settings; > +} > + > +/* ---------- Sysfs Interface Functions ------------- */ I'm not in general keen on rather obvious comments like this one. They just add noise. > + > +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 tsl2671_chip *chip = indio_dev->dev_data; > + > + return sprintf(buf, "%s\n", chip->client->name); > +} This will be wrapped into the core. This may happen before or after this driver merges. > + > +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 tsl2671_chip *chip = indio_dev->dev_data; > + > + return sprintf(buf, "%d\n", chip->taos_chip_status); > +} Hmm.. we really need to bully the runtime pm guys into putting this in the core. It doesn't belong in a driver at all. > + > +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 tsl2671_chip *chip = indio_dev->dev_data; > + unsigned long value; > + > + if (strict_strtoul(buf, 0, &value)) > + return -EINVAL; strict_strtoul returns a useful error code. Please return that not -EINVAL. > + > + if (value == 0) > + taos_chip_off(chip->client); > + else > + taos_chip_on(chip->client); > + > + return len; > +} > + > + > +static ssize_t taos_prox_interrupt_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct tsl2671_chip *chip = indio_dev->dev_data; The iio_priv stuff has now been merged. It's much cleaner than this old approach. I will be getting rid of dev_data when we move out of staging. > + if (chip->taos_settings.interrupt & 0x20) > + return sprintf(buf, "%d\n", 1); > + else > + return sprintf(buf, "%d\n", 0); return sprintf(buf, "%d\n", !!chip->taos_settings.interrupt); > +} > + > +static ssize_t taos_prox_interrupt_store(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t len) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct tsl2671_chip *chip = indio_dev->dev_data; > + unsigned long value; > + > + if (strict_strtoul(buf, 0, &value)) error from strict_strtoul please. > + return -EINVAL; > + > + if (value > 1) > + return -EINVAL; > + if (value) > + chip->taos_settings.interrupt |= 0x20; > + else > + chip->taos_settings.interrupt &= 0x10; > + > + return len; > +} > + > +static ssize_t taos_prox_thresh_high_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct tsl2671_chip *chip = indio_dev->dev_data; > + > + return sprintf(buf, "%d\n", chip->taos_settings.prox_thres); > +} > + > +static ssize_t taos_prox_thresh_high_store(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t len) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct tsl2671_chip *chip = indio_dev->dev_data; > + unsigned long value; > + > + if (strict_strtoul(buf, 0, &value)) > + return -EINVAL; > + > + chip->taos_settings.prox_thres = value; > + > + return len; > +} So this stores it in a cached value. Surely you want to write that to the device? If you can't because of unpredictable results whilst it is enabled, then please return -EBUSY until it is not enabled. > + > +/* sampling_frequency AKA persistence in data sheet */ > +static ssize_t taos_persistence_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct tsl2671_chip *chip = indio_dev->dev_data; > + > + return sprintf(buf, "0x%02X\n", chip->taos_settings.persistence); > +} > + > +static ssize_t taos_persistence_store(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t len) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct tsl2671_chip *chip = indio_dev->dev_data; > + unsigned long value; > + > + if (strict_strtoul(buf, 0, &value)) > + return -EINVAL; > + > + chip->taos_settings.persistence = value; > + > + return len; > +} > + > +static ssize_t taos_persistence_available_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "0x0. - 0xF. (0 - 240)\n"); We should pin down this standard cleanly. This is not trivially machine readable so isn't ideal. If you want to do this, propose it in a separate RFC to the list for discussion. > +} > + > +static ssize_t taos_do_prox_calibrate(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t len) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct tsl2671_chip *chip = indio_dev->dev_data; > + unsigned long value; > + > + if (strict_strtoul(buf, 0, &value)) > + return -EINVAL; > + > + if (value == 1) > + taos_prox_cal(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(proximity_calibrate, S_IWUSR, NULL, taos_do_prox_calibrate); > + > +static DEVICE_ATTR(sampling_frequency, S_IRUGO | S_IWUSR, > + taos_persistence_show, taos_persistence_store); Is it in Hz? I have my doubts > + > +static DEVICE_ATTR(sampling_frequency_available, S_IRUGO, > + taos_persistence_available_show, NULL); > + > +static DEVICE_ATTR(proximity_thresh_value, S_IRUGO | S_IWUSR, > + taos_prox_thresh_high_show, taos_prox_thresh_high_store); > + > +static struct attribute *sysfs_attrs_ctrl[] = { Silly question, but don't we want to be able to read the raw value? > + &dev_attr_name.attr, > + &dev_attr_power_state.attr, > + &dev_attr_sampling_frequency.attr, /* persist*/ > + &dev_attr_sampling_frequency_available.attr, > + &dev_attr_proximity_thresh_value.attr, /*Prox thresh */ > + &dev_attr_proximity_calibrate.attr, > + NULL > +}; > + > +static struct attribute_group tsl2671_attribute_group = { > + .attrs = sysfs_attrs_ctrl, > +}; > + > +/* > + * Run-time interrupt handler - this handler queues up > + * the bottom-half tasklet, to handle all valid interrupts. > + */ > +static int taos_interrupt_th(struct iio_dev *dev_info, > + int index, > + s64 timestamp, > + int not_test) > +{ > + struct tsl2671_chip *chip = dev_info->dev_data; > + > + schedule_work(&chip->work_thresh); > + return 0; > +} > + > +IIO_EVENT_SH(threshold, &taos_interrupt_th); > + > +IIO_EVENT_ATTR_SH(proximity_thresh_en, > + iio_event_threshold, > + taos_prox_interrupt_show, > + taos_prox_interrupt_store, > + STA_PRXINTR); > + > +static struct attribute *tsl2671_event_attributes[] = { > + &iio_event_attr_proximity_thresh_en.dev_attr.attr, > + NULL, > +}; > + > +static struct attribute_group tsl2671_event_attribute_group = { > + .attrs = tsl2671_event_attributes, > +}; > + > +/* Use the default register values to identify the Taos device */ > +static int taos_TSL2671_device(unsigned char *bufp) > +{ > + return ((bufp[TSL2671_CHIPID] & 0xf0) == 0x00); > +} It's a bit of a micro function. I'd be tempted to roll this into the calling site. > + > +/* > + * 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[TSL2671_MAX_DEVICE_REGS]; > + static struct tsl2671_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; > + } > + > + chip = kzalloc(sizeof(struct tsl2671_chip), GFP_KERNEL); > + > + chip->client = clientp; > + i2c_set_clientdata(clientp, chip); > + > + mutex_init(&chip->prox_mutex); > + > + chip->taos_chip_status = TSL2671_CHIP_UNKNOWN; > + memcpy(chip->taos_config, tsl2671_taos_config, > + sizeof(chip->taos_config)); > + You wrote a nice general purpose read function, why not use that? > + for (i = 0; i < TSL2671_MAX_DEVICE_REGS; i++) { > + ret = i2c_smbus_write_byte(clientp, > + (TSL2671_CMD_REG | (TSL2671_CNTRL + i))); > + if (ret < 0) { > + dev_err(&clientp->dev, "i2c_smbus_write_bytes() to cmd " > + "reg failed in taos_probe(), err = %d\n", ret); > + goto fail1; > + } > + ret = i2c_smbus_read_byte(clientp); > + if (ret < 0) { > + dev_err(&clientp->dev, "i2c_smbus_read_byte from " > + "reg failed in taos_probe(), err = %d\n", ret); > + > + goto fail1; > + } > + buf[i] = ret; > + } > + > + if (!taos_TSL2671_device(buf)) { > + dev_info(&clientp->dev, "i2c device found but does not match " > + "expected id in taos_probe()\n"); > + goto fail1; > + } > + > + ret = i2c_smbus_write_byte(clientp, (TSL2671_CMD_REG | TSL2671_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->iio_dev = iio_allocate_device(); > + if (!chip->iio_dev) > + goto fail1; > + > + chip->iio_dev->attrs = &tsl2671_attribute_group; > + chip->iio_dev->dev.parent = &clientp->dev; > + chip->iio_dev->dev_data = (void *)(chip); > + chip->iio_dev->num_interrupt_lines = 1; > + chip->iio_dev->event_attrs = &tsl2671_event_attribute_group; > + chip->iio_dev->driver_module = THIS_MODULE; > + chip->iio_dev->modes = INDIO_DIRECT_MODE; > + > + ret = iio_device_register(chip->iio_dev); > + if (ret) > + goto fail1; > + > + if (chip->irq_no) { > + ret = iio_register_interrupt_line(chip->irq_no, > + chip->iio_dev, > + 0, > + IRQF_TRIGGER_FALLING, > + "tsl2671"); This will soon be gone. You could preempt that by doing the same as in one of the 'move from old to current event handling' patches in the iio-onwards tree. I don't think they have any prerequists. Makes all this code much simpler. The old approach was massively over engineered (oops). Basically you just handle the events entirely yourself. > + if (ret) > + goto fail1; > + > + iio_add_event_to_list(&iio_event_threshold, > + &chip->iio_dev->interrupts[0]->ev_list); > + } > + > + /* Load up the defaults > + * (these are can be changed in the device[x]/ABI) */ > + taos_defaults(chip); > + > + /* Assume board info already established > + chip->irq_no = IRQ_EINT4; Indeed, so get rid of this comment! > + */ > + > + INIT_WORK(&chip->work_thresh, taos_interrupt_bh); > + > + /* Make sure the chip is on */ > + taos_chip_on(clientp); > + Not a useful message. If it's found, the directory will be created and this is obvious. > + dev_info(&clientp->dev, "Light sensor found.\n"); > + > + return 0; > + > +fail1: > + kfree(chip); > + > + return ret; > +} > + > +static int taos_suspend(struct i2c_client *client, pm_message_t state) > +{ > + struct tsl2671_chip *chip = i2c_get_clientdata(client); > + int ret = 0; > + > + mutex_lock(&chip->prox_mutex); > + Silly question, but what in here will fail if TSL2671_CHIP_WORKING isn't true? If nothing, then don't bother checking the condition. > + if (chip->taos_chip_status == TSL2671_CHIP_WORKING) { > + ret = taos_chip_off(client); > + chip->taos_chip_status = TSL2671_CHIP_SUSPENDED; > + } > + > + mutex_unlock(&chip->prox_mutex); > + return ret; > +} > + > +static int taos_resume(struct i2c_client *client) > +{ > + struct tsl2671_chip *chip = i2c_get_clientdata(client); > + int ret = 0; > + > + mutex_lock(&chip->prox_mutex); > + Can't see any way this won't be true... > + if (chip->taos_chip_status == TSL2671_CHIP_SUSPENDED) > + ret = taos_chip_on(client); > + > + mutex_unlock(&chip->prox_mutex); > + return ret; > +} > + > + > +static int __devexit taos_remove(struct i2c_client *client) > +{ > + struct tsl2671_chip *chip = i2c_get_clientdata(client); > + > + taos_chip_off(client); > + > + if (chip->irq_no) > + free_irq(chip->irq_no, chip->client->name); > + > + flush_scheduled_work(); > + > + iio_device_unregister(chip->iio_dev); > + > + kfree(chip); > + return 0; > +} > + > +static struct i2c_device_id taos_idtable[] = { > + { "tsl2671", 0 }, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, taos_idtable); > + > +/* Driver definition */ Pointless comment of the day :) > +static struct i2c_driver taos_driver = { > + .driver = { > + .name = "tsl2671", > + }, > + .id_table = taos_idtable, > + .suspend = taos_suspend, > + .resume = taos_resume, > + .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@xxxxxxxxxxx>"); > +MODULE_DESCRIPTION("TAOS tsl2671 proximity 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html