Messages in this thread | | | Date | Mon, 13 Feb 2012 15:28:49 +0000 | From | Jonathan Cameron <> | Subject | Re: [PATCH V1] TAOS tsl2x7x |
| |
On 2/13/2012 3:19 PM, Jonathan Cameron wrote: > On 2/10/2012 7:06 PM, Jon Brenner wrote: >> TAOS device driver for tsl/tmd 2771 and 2772 device families (inc all >> variants). >> >> Signed-off-by: Jon Brenner<jbrenner@taosinc.com> > Hi Jon, > Not enough coffee.. Reasonably clean. ... > Good to see another driver from you. Reasonably keen but couple of > medium sized issues > a) Use iio_chan_spec and callbacks in iio_info structure where > possible. Propose additional > elements where they make sense (such as your integration time). > b) Documentation should be slotted in relevant places in more general > files. > c) I'm unconvinced the calibration stuff should be in kernel. > > Thanks, > > Jonathan >> >> --- >> .../iio/Documentation/sysfs-bus-iio-light-tsl2x7x | 71 + >> drivers/staging/iio/light/Kconfig | 11 + >> drivers/staging/iio/light/Makefile | 1 + >> drivers/staging/iio/light/tsl2x7x_core.c | 2053 >> ++++++++++++++++++++ >> drivers/staging/iio/light/tsl2x7x_core.h | 138 ++ >> 5 files changed, 2274 insertions(+), 0 deletions(-) >> >> diff --git >> a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-tsl2x7x >> b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-tsl2x7x >> new file mode 100644 >> index 0000000..f1bb5f5 >> --- /dev/null >> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-tsl2x7x >> @@ -0,0 +1,71 @@ > Most of these are fairly general purpose so should be sysfs-bus-iio or > sysfs-bus-iio-light. > Some are also shared with the tsl2583 doc so maybe move those into the > light one. > > Generally if the last bit (after the channel type) is restricted to > the actual device then > it wants to be in this file. > If it's restricted to the class (light / proximity) then it wants to be > in sysfs-bus-iio-light or sysfs-bus-iio-proximity > If like proximity_raw or the thresh_value ones then it wants to be > in sysfs-bus-iio. > > Please do fix any cases where this isn't currently the case. Our > documentation outside the top > level file is far from well written or organized. > >> +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. >> + >> +What: /sys/bus/iio/devices/device[n]/illuminance0_calibrate >> +KernelVersion: 2.6.37 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + This property causes an internal calibration of the als gain >> trim >> + value which is later used in calculating illuminance in lux. >> + >> +What: >> /sys/bus/iio/devices/device[n]/illuminance0_thresh_falling_value >> +KernelVersion: 3.3-rc1 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + Low (falling) trigger point for the internal ALS comparison >> + function for interrupt generation. > This one just needs adding to sysfs-bus-iio. >> + >> +What: >> /sys/bus/iio/devices/device[n]/illuminance0_thresh_rising_value >> +KernelVersion: 3.3-rc1 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + high (rising) trigger point for the internal ALS comparison >> + function for interrupt generation. >> + >> +What: /sys/bus/iio/devices/device[n]/illuminance0_both_raw >> +KernelVersion: 3.3-rc1 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + Simultainious ALS channel 1 and channel 2 data represented as >> + a 32 bit integer. > err. previously the both has referred to the light frequencies > covered. Please don't do what you > describe here. If you really need to be sure they are pared then you > want to use the buffered > infrastructure not sysfs. > >> + >> +What: /sys/bus/iio/devices/device[n]/proximity_calibrate >> +KernelVersion: 3.3-rc1 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + Causes an recalculation and adjustment to the >> + proximity_thresh_rising_value. >> + >> +What: >> /sys/bus/iio/devices/device[n]/proximity_thresh_falling_value >> +KernelVersion: 3.3-rc1 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + Low (falling) trigger point for the internal proximity >> comparison >> + function for interrupt generation. >> + > Again, standard form so add to sysfs-bus-iio. >> +What: >> /sys/bus/iio/devices/device[n]/proximity_thresh_rising_value >> +KernelVersion: 3.3-rc1 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + high (rising) trigger point for the internal proximity >> comparison >> + function for interrupt generation. >> + >> +What: /sys/bus/iio/devices/device[n]/proximity_calibscale >> +KernelVersion: 3.3-rc1 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + Hardware or software applied calibration scale factor assumed >> + to account for attenuation due to industrial design (glass >> + filters or aperture holes). >> + > All of these are standard so add to sysfs-bus-iio lists for calibscale >> +What: /sys/bus/iio/devices/device[n]/proximity_raw >> +KernelVersion: 3.3-rc1 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + State of proximity detection based on the >> + proximity_thresh_rising_value. > If you wouldn't mind can you lift he proximity documentation out of > sysfs-bus-iio-ilght into sysfs-bus-iio > then we can take all the standard definitions without adding them yet > again. (as a separate patch). > > I'm still kind of regretting ever introducing multiple sysfs > documentatino files. >> + >> diff --git a/drivers/staging/iio/light/Kconfig >> b/drivers/staging/iio/light/Kconfig >> index e7e9159..29de333 100644 >> --- a/drivers/staging/iio/light/Kconfig >> +++ b/drivers/staging/iio/light/Kconfig >> @@ -31,4 +31,15 @@ config TSL2583 >> Provides support for the TAOS tsl2580, tsl2581 and tsl2583 >> devices. >> Access ALS data via iio, sysfs. >> >> + This driver can also be built as a module. If so, the module >> + will be called tsl2583. > errr. This change to a comment about a completely different driver > should not be in this patch. >> + >> +config TSL2x7x >> + tristate "TAOS TSL/TMD2x71 and TSL/TMD2x72 Family of light and >> proximity sensors" >> + depends on I2C >> + help >> + Support for: tsl2571, tsl2671, tmd2671, tsl2771, tmd2771, >> tsl2572, tsl2672, >> + tmd2672, tsl2772, tmd2772 devices. >> + Provides iio_events and direct access via sysfs. >> + >> endmenu >> diff --git a/drivers/staging/iio/light/Makefile >> b/drivers/staging/iio/light/Makefile >> index 3011fbf..ff12c4b 100644 >> --- a/drivers/staging/iio/light/Makefile >> +++ b/drivers/staging/iio/light/Makefile >> @@ -5,3 +5,4 @@ >> obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o >> obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o >> obj-$(CONFIG_TSL2583) += tsl2583.o >> +obj-$(CONFIG_TSL2x7x) += tsl2x7x_core.o >> diff --git a/drivers/staging/iio/light/tsl2x7x_core.c >> b/drivers/staging/iio/light/tsl2x7x_core.c >> new file mode 100644 >> index 0000000..671f476 >> --- /dev/null >> +++ b/drivers/staging/iio/light/tsl2x7x_core.c >> @@ -0,0 +1,2053 @@ >> +/* >> + * Device driver for monitoring ambient light intensity in (lux) >> + * and proximity detection (prox) within the TAOS TSL2X7X family of >> devices. >> + * >> + * Copyright (c) 2012, 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/platform_device.h> >> +#include<linux/input.h> >> +#include<linux/slab.h> >> +#include<linux/pm.h> >> +#include<linux/module.h> >> +#include<linux/version.h> >> + >> +#include<linux/cdev.h> >> +#include<linux/stat.h> >> +#include<linux/module.h> >> +#include "tsl2x7x_core.h" >> +#include "../events.h" >> +#include "../buffer.h" >> +#include "../iio.h" >> +#include "../sysfs.h" >> + >> +/* Cal defs*/ >> +#define PROX_STAT_CAL 0 >> +#define PROX_STAT_SAMP 1 >> +#define MAX_SAMPLES_CAL 200 >> + >> +/* TSL2X7X Device ID */ >> +#define TRITON_ID 0x00 >> +#define SWORDFISH_ID 0x30 >> +#define HALIBUT_ID 0x20 >> + >> +/* Lux calculation constants */ >> +#define TSL2X7X_LUX_CALC_OVER_FLOW 65535 >> + >> +/* TAOS txx2x7x Device family members */ >> +enum { >> + tsl2571, >> + tsl2671, >> + tmd2671, >> + tsl2771, >> + tmd2771, >> + tsl2572, >> + tsl2672, >> + tmd2672, >> + tsl2772, >> + tmd2772 >> +}; >> + >> +enum { >> + TSL2X7X_CHIP_UNKNOWN = 0, >> + TSL2X7X_CHIP_WORKING = 1, >> + TSL2X7X_CHIP_SUSPENDED = 2 >> +}; >> + >> +/* Per-device data */ >> +struct tsl2x7x_als_info { >> + u16 als_ch0; >> + u16 als_ch1; >> + u16 lux; >> +}; >> + >> +/* proximity data */ >> +struct tsl2x7x_prox_info { >> + u16 prox_data; >> + int prox_event; >> +}; >> + >> +struct prox_stat { >> + u16 min; >> + u16 max; >> + u16 mean; >> + unsigned long stddev; >> +}; >> + >> +struct tsl2x7x_settings { >> + int als_time; >> + int als_gain; >> + int als_gain_trim; >> + int wait_time; >> + int prx_time; >> + int prox_gain; >> + int prox_config; >> + int als_cal_target; >> + u8 interrupts_en; >> + u8 als_persistence; >> + int als_thresh_low; >> + int als_thresh_high; >> + int prox_thres_low; >> + int prox_thres_high; >> + int prox_pulse_count; >> + int prox_max_samples_cal;/* for calibration mode*/ >> + >> +}; >> + >> +struct tsl2X7X_chip_info { >> + struct iio_chan_spec channel[0]; >> + const struct iio_info *info; >> +}; > err. what is channel[0] for? Please do the iio_chan_spec stuff fully. >> + >> +struct tsl2X7X_chip { >> + kernel_ulong_t id; >> + struct mutex prox_mutex; >> + struct mutex als_mutex; >> + struct i2c_client *client; >> + struct iio_dev *iio_dev; >> + struct tsl2x7x_prox_info prox_cur_info; >> + struct tsl2x7x_als_info als_cur_info; >> + struct tsl2x7x_settings tsl2x7x_settings; >> + struct tsl2X7X_platform_data *pdata; >> + int als_time_scale; >> + int als_saturation; >> + int tsl2x7x_chip_status; >> + u8 tsl2x7x_config[TSL2X7X_REG_MAX]; >> + bool init_done; >> + struct work_struct work_thresh; >> + s64 event_timestamp; >> + /* This structure is intentionally large to accommodate >> + * updates via sysfs. */ >> + /* Sized to 9 = max 8 segments + 1 termination segment */ >> + /* Assumption is one and only one type of glass used */ >> + struct tsl2x7x_lux tsl2x7x_device_lux[MAX_TABLE_SIZE]; >> +}; >> + >> +/* Different devices require different coefficents */ >> +static const struct tsl2x7x_lux tsl2x71_lux_table[] = { >> + { 14461, 611, 1211 }, >> + { 18540, 352, 623 }, >> + { 0, 0, 0 }, >> +}; >> + >> +static const struct tsl2x7x_lux tmd2x71_lux_table[] = { >> + { 11635, 115, 256 }, >> + { 15536, 87, 179 }, >> + { 0, 0, 0 }, >> +}; >> + >> +static const struct tsl2x7x_lux tsl2x72_lux_table[] = { >> + { 14013, 466, 917 }, >> + { 18222, 310, 552 }, >> + { 0, 0, 0 }, >> +}; >> + >> +static const struct tsl2x7x_lux tmd2x72_lux_table[] = { >> + { 13218, 130, 262 }, >> + { 17592, 92, 169 }, >> + { 0, 0, 0 }, >> +}; >> + >> +static const struct tsl2x7x_lux *tsl2x7x_default_lux_table_group[] = { >> + [tsl2571] = tsl2x71_lux_table, >> + [tsl2671] = tsl2x71_lux_table, >> + [tmd2671] = tmd2x71_lux_table, >> + [tsl2771] = tsl2x71_lux_table, >> + [tmd2771] = tmd2x71_lux_table, >> + [tsl2572] = tsl2x72_lux_table, >> + [tsl2672] = tsl2x72_lux_table, > Check your spacing vs tabs. >> + [tmd2672] = tmd2x72_lux_table, >> + [tsl2772] = tsl2x72_lux_table, >> + [tmd2772] = tmd2x72_lux_table, >> +}; >> + >> +struct als_gainadj { >> + s16 ch0; >> + s16 ch1; >> +}; >> + >> +/* Used to validate the ALS gain selection index */ >> +static const struct als_gainadj tsl2X7X_als_gainadj[] = { >> + { 1, 1 }, >> + { 8, 8 }, >> + { 16, 16 }, >> + { 120, 120 } >> +}; > This is a somewhat novel structure.... Any real point in the two > columns? >> + >> + >> +/* Not using channels */ >> +static const struct iio_chan_spec tsl2X7X_channels[] = {}; >> + >> +/* >> + * Read a number of bytes starting at register (reg) location. >> + * Return 0, or i2c_smbus_write_byte ERROR code. >> + */ >> +static int >> +tsl2x7x_i2c_read(struct i2c_client *client, u8 reg, u8 *val, >> unsigned int len) >> +{ >> + int ret; >> + int i; >> + >> + for (i = 0; i< len; i++) { > Just to check, does this correspond to > i2c_smbus_read_byte_data(client, (TSL2X7X_CMD_REG | REG))? > Also, a quick look makes me think you only ever use this for one byte > at a time... > >> + /* select register to write */ >> + ret = i2c_smbus_write_byte(client, (TSL2X7X_CMD_REG | reg)); >> + if (ret< 0) { >> + dev_err(&client->dev, "tsl2x7x_i2c_read failed to write" >> + " register %x\n", reg); >> + return ret; >> + } >> + /* read the data */ >> + *val = i2c_smbus_read_byte(client); >> + val++; >> + reg++; >> + } >> + return 0; >> +} >> + >> +/* > Kernel doc please. >> + * 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 >> tsl2x7x_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 tsl2x7x_get_lux(struct iio_dev *indio_dev) >> +{ >> + u16 ch0, ch1; /* separated ch0/ch1 data from device */ >> + u32 lux; /* raw lux calculated from device data */ >> + u64 lux64; >> + u32 ratio; >> +#pragma pack(4) > ? that needs some explanation... >> + u8 buf[4]; >> + struct tsl2x7x_lux *p; >> + struct tsl2X7X_chip *chip = iio_priv(indio_dev); >> + int i, ret; >> + u32 ch0lux = 0; >> + u32 ch1lux = 0; >> + > as below, I find these try locks rather suspicious. Please justify. >> + if (mutex_trylock(&chip->als_mutex) == 0) { >> + dev_info(&chip->client->dev, "tsl2x7x_get_lux device is >> busy\n"); >> + return chip->als_cur_info.lux; /* busy, so return LAST VALUE */ >> + } >> + >> + if (chip->tsl2x7x_chip_status != TSL2X7X_CHIP_WORKING) { >> + /* device is not enabled */ >> + dev_err(&chip->client->dev, "tsl2x7x_get_lux device is not >> enabled\n"); >> + ret = -EBUSY ; >> + goto out_unlock; >> + } >> + >> + ret = tsl2x7x_i2c_read(chip->client, >> + (TSL2X7X_CMD_REG | TSL2X7X_STATUS),&buf[0], 1); >> + if (ret< 0) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_get_lux failed to read CMD_REG\n"); >> + goto out_unlock; >> + } >> + /* is data new& valid */ >> + if (!(buf[0]& TSL2X7X_STA_ADC_VALID)) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_get_lux data not valid yet\n"); >> + ret = chip->als_cur_info.lux; /* return LAST VALUE */ >> + goto out_unlock; >> + } >> + >> + for (i = 0; i< 4; i++) { >> + ret = tsl2x7x_i2c_read(chip->client, >> + (TSL2X7X_CMD_REG | (TSL2X7X_ALS_CHAN0LO + i)), >> + &buf[i], 1); >> + if (ret< 0) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_get_lux failed to read" >> + " ret: %x\n", ret); >> + goto out_unlock; >> + } >> + } >> + >> + /* clear status, really interrupt status ( are off), >> + but we use the bit anyway */ >> + ret = i2c_smbus_write_byte(chip->client, >> + (TSL2X7X_CMD_REG | >> + TSL2X7X_CMD_SPL_FN | >> + TSL2X7X_CMD_ALS_INT_CLR)); >> + >> + if (ret< 0) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_i2c_write_command failed in tsl2x7x_get_lux, err = >> %d\n", >> + ret); >> + goto out_unlock; /* have no data, so return failure */ >> + } >> + >> + /* extract ALS/lux data */ >> + ch0 = le16_to_cpup((const __le16 *)&buf[0]); >> + ch1 = le16_to_cpup((const __le16 *)&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 tsl2x7x_lux *) chip->tsl2x7x_device_lux; >> + p->ratio != 0&& p->ratio< ratio; p++) > stray semi colon. >> + ; >> + >> + if (p->ratio == 0) { >> + lux = 0; >> + } else { >> + ch0lux = ((ch0 * p->ch0) + >> + (tsl2X7X_als_gainadj[chip->tsl2x7x_settings.als_gain].ch0>> >> 1)) >> + / tsl2X7X_als_gainadj[chip->tsl2x7x_settings.als_gain].ch0; >> + >> + ch1lux = ((ch1 * p->ch1) + >> + (tsl2X7X_als_gainadj[chip->tsl2x7x_settings.als_gain].ch1>> >> 1)) >> + / tsl2X7X_als_gainadj[chip->tsl2x7x_settings.als_gain].ch1; >> + >> + lux = ch0lux - ch1lux; >> + } >> + >> + /* note: lux is 31 bit max at this point */ >> + if (ch1lux> ch0lux) { >> + dev_dbg(&chip->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 >> + * The tsl2x7x_device_lux tables have a factor of 256 built-in. >> + * User-specified gain provides a multiplier. >> + * Apply user-specified gain before shifting right to retain >> precision. >> + * Use 64 bits to avoid overflow on multiplication. >> + * Then go back to 32 bits before division to avoid using >> div_u64(). >> + */ >> + lux64 = lux; >> + lux64 = lux64 * chip->tsl2x7x_settings.als_gain_trim; >> + lux64>>= 8; >> + lux = lux64; >> + lux = (lux + 500) / 1000; >> + >> + if (lux> TSL2X7X_LUX_CALC_OVER_FLOW) { /* check for overflow */ >> +return_max: >> + lux = TSL2X7X_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; >> + >> +} >> + >> +/* >> + * 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. >> + */ >> +static int tsl2x7x_prox_poll(struct iio_dev *indio_dev) >> +{ >> +#define CONSECUTIVE_RETRIES 50 >> + >> + int i; >> + int ret; >> + u8 status; >> + u8 chdata[2]; >> + int err_cnt; >> + struct tsl2X7X_chip *chip = iio_priv(indio_dev); >> + >> + if (mutex_trylock(&chip->prox_mutex) == 0) { >> + dev_err(&chip->client->dev, "Can't get prox mutex\n"); >> + return -EBUSY; >> + } > Trylocks always need an explanation as far as I am concerned. why not > wait? >> + >> + err_cnt = 0; >> + >> +try_again: >> + ret = tsl2x7x_i2c_read(chip->client, >> + (TSL2X7X_CMD_REG | TSL2X7X_STATUS),&status, 1); >> + if (ret< 0) { >> + dev_err(&chip->client->dev, >> + "Read regs failed in tsl2x7x_prox_poll() - A\n"); >> + mutex_unlock(&chip->prox_mutex); >> + return ret; >> + } >> + >> + /*Prox interrupt asserted*/ >> + if (((chip->tsl2x7x_settings.interrupts_en<< 4) >> + & CNTL_PROX_INT_ENBL)) { >> + if (!(status& TSL2X7X_STA_ADC_VALID)) { > I've not read the datasheet but this seems unusual. You get an > interrupt but the > value isn't valid? >> + err_cnt++; >> + if (err_cnt> CONSECUTIVE_RETRIES) { >> + mutex_unlock(&chip->prox_mutex); >> + dev_err(&chip->client->dev, >> + "Consec. retries exceeded\n"); >> + return chip->prox_cur_info.prox_event; >> + } >> + goto try_again; >> + } >> + } >> + >> + for (i = 0; i< 2; i++) { >> + ret = tsl2x7x_i2c_read(chip->client, >> + (TSL2X7X_CMD_REG | >> + (TSL2X7X_PRX_LO + i)),&chdata[i], 1); >> + if (ret< 0) { > Errors like this are extremely unlikely (I hope) so I'd drop the > message. Also use a goto > and have the mutex_unlock in only one place as it's easier to read. >> + dev_err(&chip->client->dev, >> + "Read regs failed in tsl2x7x_prox_poll() - B\n"); >> + mutex_unlock(&chip->prox_mutex); >> + return ret; >> + } >> + } >> + >> + chip->prox_cur_info.prox_data = (chdata[1]<<8)|chdata[0]; >> + >> + if (chip->prox_cur_info.prox_data>= >> + chip->tsl2x7x_settings.prox_thres_high) >> + chip->prox_cur_info.prox_event = 1; >> + else >> + chip->prox_cur_info.prox_event = 0; > use chip->proc_cur_info.prox_event = chip->prox_cur_info.prox_data >= > chip->tsl2x7x_settings.prox_thresh_high; >> + >> + mutex_unlock(&chip->prox_mutex); >> + return chip->prox_cur_info.prox_event; >> +} >> + >> +/* >> + * Provides initial operational parameter defaults. >> + * These defaults may be changed through the device's sysfs files. >> + */ > Have a > const static struct tsl2x7x_settings tsl2x7x_defaults = > { > .als_time = 200, > ... > }; > then memcpy it into place as necessary. >> +static void tsl2x7x_defaults(struct tsl2X7X_chip *chip) >> +{ >> + /* Operational parameters */ >> + chip->tsl2x7x_settings.als_time = 200; >> + /* must be a multiple of 50mS */ >> + chip->tsl2x7x_settings.als_gain = 0; >> + /* this is actually an index into the gain table */ >> + chip->tsl2x7x_settings.prx_time = 0xfe; /*5.4 mS */ >> + /* 2.7ms prox integration time - decrease to increase time */ >> + /* decreases in 2.7 ms intervals */ >> + chip->tsl2x7x_settings.prox_gain = 1; >> + /* these are bits 3:2 of reg 0x0f: 0=x1,1=x2,2=x4,3=x8 */ >> + /* assume clear glass as default */ >> + chip->tsl2x7x_settings.wait_time = 245; >> + /* Time between PRX and ALS cycles -decrease to increase time */ >> + /* decreases in 2.7 ms intervals */ >> + chip->tsl2x7x_settings.prox_config = 0; >> + /* Prox configuration filters */ >> + chip->tsl2x7x_settings.als_gain_trim = 1000; >> + /* default gain trim to account for aperture effects */ >> + chip->tsl2x7x_settings.als_cal_target = 150; >> + /* Known external ALS reading used for calibration */ >> + chip->tsl2x7x_settings.als_thresh_low = 200; >> + /* CH0 'low' count to trigger interrupt */ >> + chip->tsl2x7x_settings.als_thresh_high = 256; >> + /* CH0 'high' count to trigger interrupt */ >> + chip->tsl2x7x_settings.als_persistence = 0xFF; >> + /* Number of 'out of limits' ADC readings PRX/ALS*/ >> + chip->tsl2x7x_settings.interrupts_en = 0x00; >> + /* Default interrupt(s) enabled. >> + * 0x00 = none, 0x10 = als, 0x20 = prx 0x30 = bth */ >> + chip->tsl2x7x_settings.prox_thres_low = 0; >> + chip->tsl2x7x_settings.prox_thres_high = 512; >> + /*default threshold (adjust either manually or with cal routine*/ >> + chip->tsl2x7x_settings.prox_max_samples_cal = 30; >> + chip->tsl2x7x_settings.prox_pulse_count = 8; >> + >> + /* Load up the lux table. Can be changed later via ABI */ >> + if (chip->pdata&& chip->pdata->platform_lux_table[0].ratio != 0) >> + memcpy(chip->tsl2x7x_device_lux, >> + chip->pdata->platform_lux_table, >> + sizeof(chip->pdata->platform_lux_table)); >> + else >> + memcpy(chip->tsl2x7x_device_lux, >> + (struct tsl2x7x_lux >> *)tsl2x7x_default_lux_table_group[chip->id], >> + MAX_DEFAULT_TABLE_BYTES); >> + >> +} >> + > As I've stated below, you really need to justify why we have > calibration in the kernel rather > than userspace.. >> +/* >> + * Obtain single reading and calculate the als_gain_trim >> + * (later used to derive actual lux). >> + * Return updated gain_trim value. >> + */ >> +static int tsl2x7x_als_calibrate(struct iio_dev *indio_dev) >> +{ >> + struct tsl2X7X_chip *chip = iio_priv(indio_dev); >> + u8 reg_val; >> + int gain_trim_val; >> + int ret; >> + int lux_val; >> + >> + ret = i2c_smbus_write_byte(chip->client, >> + (TSL2X7X_CMD_REG | TSL2X7X_CNTRL)); >> + if (ret< 0) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_als_calibrate failed to write CNTRL register, >> ret=%d\n", >> + ret); >> + return ret; >> + } >> + >> + reg_val = i2c_smbus_read_byte(chip->client); >> + if ((reg_val& (TSL2X7X_CNTL_ADC_ENBL | TSL2X7X_CNTL_PWR_ON)) >> + != (TSL2X7X_CNTL_ADC_ENBL | TSL2X7X_CNTL_PWR_ON)) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_als_calibrate failed: ADC not enabled\n"); >> + return -1; >> + } >> + >> + ret = i2c_smbus_write_byte(chip->client, >> + (TSL2X7X_CMD_REG | TSL2X7X_CNTRL)); >> + if (ret< 0) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_als_calibrate failed to write ctrl reg: ret=%d\n", >> + ret); >> + return ret; >> + } >> + reg_val = i2c_smbus_read_byte(chip->client); >> + >> + if ((reg_val& TSL2X7X_STA_ADC_VALID) != TSL2X7X_STA_ADC_VALID) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_als_calibrate failed: STATUS - ADC not valid.\n"); >> + return -ENODATA; >> + } >> + lux_val = tsl2x7x_get_lux(indio_dev); >> + if (lux_val< 0) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_als_calibrate failed to get lux\n"); >> + return lux_val; >> + } >> + gain_trim_val = (((chip->tsl2x7x_settings.als_cal_target) >> + * chip->tsl2x7x_settings.als_gain_trim) / lux_val); >> + >> + if ((gain_trim_val< 250) || (gain_trim_val> 4000)) >> + return -ERANGE; >> + >> + chip->tsl2x7x_settings.als_gain_trim = gain_trim_val; >> + >> + dev_info(&chip->client->dev, >> + "%s als_calibrate completed\n", chip->client->name); >> + >> + return (int) gain_trim_val; >> +} >> + >> +/* >> + * Turn the device on. >> + * Configuration must be set before calling this function. >> + */ >> +static int tsl2x7x_chip_on(struct iio_dev *indio_dev) >> +{ >> + int i; >> + int ret = 0; >> + u8 *dev_reg; >> + u8 utmp; >> + int als_count; >> + int als_time; >> + struct tsl2X7X_chip *chip = iio_priv(indio_dev); >> + u8 reg_val = 0; >> + >> + if (chip->pdata&& chip->pdata->power_on) >> + chip->pdata->power_on(indio_dev); >> + >> + /* Non calculated parameters */ >> + chip->tsl2x7x_config[TSL2X7X_PRX_TIME] = >> + chip->tsl2x7x_settings.prx_time; >> + chip->tsl2x7x_config[TSL2X7X_WAIT_TIME] = >> + chip->tsl2x7x_settings.wait_time; >> + chip->tsl2x7x_config[TSL2X7X_PRX_CONFIG] = >> + chip->tsl2x7x_settings.prox_config; >> + >> + chip->tsl2x7x_config[TSL2X7X_ALS_MINTHRESHLO] = >> + (chip->tsl2x7x_settings.als_thresh_low)& 0xFF; >> + chip->tsl2x7x_config[TSL2X7X_ALS_MINTHRESHHI] = >> + (chip->tsl2x7x_settings.als_thresh_low>> 8)& 0xFF; >> + chip->tsl2x7x_config[TSL2X7X_ALS_MAXTHRESHLO] = >> + (chip->tsl2x7x_settings.als_thresh_high)& 0xFF; >> + chip->tsl2x7x_config[TSL2X7X_ALS_MAXTHRESHHI] = >> + (chip->tsl2x7x_settings.als_thresh_high>> 8)& 0xFF; >> + chip->tsl2x7x_config[TSL2X7X_PERSISTENCE] = >> + chip->tsl2x7x_settings.als_persistence; >> + >> + chip->tsl2x7x_config[TSL2X7X_PRX_COUNT] = >> + chip->tsl2x7x_settings.prox_pulse_count; >> + chip->tsl2x7x_config[TSL2X7X_PRX_MINTHRESHLO] = >> + chip->tsl2x7x_settings.prox_thres_low; >> + chip->tsl2x7x_config[TSL2X7X_PRX_MAXTHRESHLO] = >> + chip->tsl2x7x_settings.prox_thres_high; >> + >> + /* and make sure we're not already on */ >> + if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) { >> + /* if forcing a register update - turn off, then on */ >> + dev_info(&chip->client->dev, "device is already enabled\n"); >> + return -EINVAL; >> + } >> + >> + /* determine als integration regster */ >> + als_count = (chip->tsl2x7x_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->tsl2x7x_config[TSL2X7X_ALS_TIME] = 256 - als_count; >> + >> + /* Set the gain based on tsl2x7x_settings struct */ >> + chip->tsl2x7x_config[TSL2X7X_GAIN] = >> + (chip->tsl2x7x_settings.als_gain | (mA100 | DIODE1) >> + | ((chip->tsl2x7x_settings.prox_gain)<< 2)); >> + >> + /* 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; >> + >> + /* TSL2X7X Specific power-on / adc enable sequence >> + * Power on the device 1st. */ >> + utmp = TSL2X7X_CNTL_PWR_ON; >> + ret = i2c_smbus_write_byte_data(chip->client, >> + TSL2X7X_CMD_REG | TSL2X7X_CNTRL, utmp); >> + if (ret< 0) { >> + dev_err(&chip->client->dev, "tsl2x7x_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, dev_reg = chip->tsl2x7x_config; i< TSL2X7X_REG_MAX; >> i++) { >> + ret = i2c_smbus_write_byte_data(chip->client, >> + TSL2X7X_CMD_REG + i, *dev_reg++); >> + if (ret< 0) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_chip_on failed on write to reg %d.\n", i); >> + return ret; >> + } >> + } >> + >> + udelay(3000); /* Power-on settling time */ >> + >> + /* NOW enable the ADC >> + * initialize the desired mode of operation */ >> + utmp = TSL2X7X_CNTL_PWR_ON | TSL2X7X_CNTL_ADC_ENBL | >> CNTL_PROX_DET_ENBL; >> + ret = i2c_smbus_write_byte_data(chip->client, >> + TSL2X7X_CMD_REG | TSL2X7X_CNTRL, utmp); >> + if (ret< 0) { >> + dev_err(&chip->client->dev, "tsl2x7x_chip_on failed on 2nd >> CTRL reg.\n"); >> + return ret; >> + } >> + >> + chip->tsl2x7x_chip_status = TSL2X7X_CHIP_WORKING; >> + >> + if (chip->tsl2x7x_settings.interrupts_en != 0) { >> + dev_info(&chip->client->dev, "Setting Up Interrupt(s)\n"); >> + /* first time interrupt */ >> + chip->init_done = 0; >> + >> + >> + reg_val = TSL2X7X_CNTL_PWR_ON; >> + >> + if (chip->tsl2x7x_settings.interrupts_en == 0x10) >> + reg_val |= CNTL_ADC_ENBL; >> + >> + if (chip->tsl2x7x_settings.interrupts_en == 0x20) >> + reg_val |= CNTL_PROX_DET_ENBL; >> + >> + if (chip->tsl2x7x_settings.interrupts_en == 0x30) >> + reg_val |= (CNTL_ADC_ENBL | CNTL_PROX_DET_ENBL); >> + >> + reg_val |= chip->tsl2x7x_settings.interrupts_en; >> + >> + ret = i2c_smbus_write_byte_data(chip->client, >> + (TSL2X7X_CMD_REG | TSL2X7X_CNTRL), reg_val); >> + if (ret< 0) >> + dev_err(&chip->client->dev, >> + "tsl2x7x_i2c_write to device failed in >> tsl2x7x_IOCTL_INT_SET.\n"); >> + >> + /* Clear out any initial interrupts */ >> + ret = i2c_smbus_write_byte(chip->client, >> + TSL2X7X_CMD_REG | TSL2X7X_CMD_SPL_FN | >> + TSL2X7X_CMD_PROXALS_INTCLR); >> + if (ret< 0) { >> + dev_err(&chip->client->dev, >> + "tsl2x7x_i2c_write_command failed in tsl2x7x_chip_on\n"); >> + return ret; >> + } >> + } >> + return ret; >> + >> +} >> + >> +static int tsl2x7x_chip_off(struct iio_dev *indio_dev) >> +{ >> + int ret; >> + struct tsl2X7X_chip *chip = iio_priv(indio_dev); >> + >> + /* turn device off */ >> + chip->tsl2x7x_chip_status = TSL2X7X_CHIP_SUSPENDED; >> + >> + ret = i2c_smbus_write_byte_data(chip->client, >> + TSL2X7X_CMD_REG | TSL2X7X_CNTRL, 0x00); >> + >> + if (chip->pdata&& chip->pdata->power_off) >> + chip->pdata->power_off(chip->client); >> + >> + return ret; >> +} >> + >> +/** >> + * 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. > It's exactly this sort of thing that supports my argument below that > this ought to be > in userspace... >> + */ >> +static unsigned long tsl2x7x_isqrt(unsigned long x) >> +{ >> + 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. >> + */ >> +static >> +void tsl2x7x_prox_calculate(u16 *data, int length, struct prox_stat >> *statP) >> +{ >> + int i; >> + int min, max, sum, mean; >> + unsigned long stddev; >> + int tmp; >> + >> + if (length == 0) >> + length = 1; >> + >> + sum = 0; >> + min = 0xffff; >> + max = 0; >> + for (i = 0; i< length; i++) { >> + sum += data[i]; >> + 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 = tsl2x7x_isqrt((long)sum)/length; >> + statP->stddev = stddev; >> + >> +} >> + >> +/** >> + * Proximity calibration - collects a number of samples, >> + * calculates a standard deviation based on the samples, and >> + * sets the threshold accordingly. >> + */ > I guess you have a demand for this, but generally I'd feel this had more > of a place in userspace than down in the kernel... >> +static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) >> +{ >> + u16 prox_history[MAX_SAMPLES_CAL+1]; >> + int i; >> + struct prox_stat prox_stat_data[2]; >> + struct prox_stat *calP; >> + struct tsl2X7X_chip *chip = iio_priv(indio_dev); >> + u8 tmp_irq_settings; >> + u8 current_state = chip->tsl2x7x_chip_status; >> + >> + if (chip->tsl2x7x_settings.prox_max_samples_cal> >> MAX_SAMPLES_CAL) { >> + dev_err(&chip->client->dev, >> + "max prox samples cal is too big: %d\n", >> + chip->tsl2x7x_settings.prox_max_samples_cal); >> + chip->tsl2x7x_settings.prox_max_samples_cal = MAX_SAMPLES_CAL; >> + } >> + >> + /* have to stop to change settings */ >> + tsl2x7x_chip_off(indio_dev); >> + >> + /* Enable proximity detection save just in case prox not wanted >> yet*/ >> + tmp_irq_settings = chip->tsl2x7x_settings.interrupts_en; >> + chip->tsl2x7x_settings.interrupts_en |= CNTL_PROX_INT_ENBL; >> + >> + /*turn on device if not already on*/ >> + tsl2x7x_chip_on(indio_dev); >> + >> + /*gather the samples*/ >> + for (i = 0; i< chip->tsl2x7x_settings.prox_max_samples_cal; i++) { >> + mdelay(15); >> + tsl2x7x_prox_poll(indio_dev); >> + prox_history[i] = chip->prox_cur_info.prox_data; >> + dev_info(&chip->client->dev, "2 i=%d prox data= %d\n", >> + i, chip->prox_cur_info.prox_data); >> + >> + } >> + >> + tsl2x7x_chip_off(indio_dev); >> + >> + calP =&prox_stat_data[PROX_STAT_CAL]; >> + tsl2x7x_prox_calculate(prox_history, >> + chip->tsl2x7x_settings.prox_max_samples_cal, calP); >> + chip->tsl2x7x_settings.prox_thres_high = (calP->max<< 1) - >> calP->mean; >> + >> + dev_info(&chip->client->dev, " cal min=%d mean=%d max=%d\n", >> + calP->min, calP->mean, calP->max); >> + dev_info(&chip->client->dev, >> + "%s proximity threshold set to %d\n", >> + chip->client->name, chip->tsl2x7x_settings.prox_thres_high); >> + >> + /* back to the way they were */ >> + chip->tsl2x7x_settings.interrupts_en = tmp_irq_settings; >> + if (current_state == TSL2X7X_CHIP_WORKING) >> + tsl2x7x_chip_on(indio_dev); >> +} >> + >> +/* ---------- Sysfs Interface Functions ------------- */ > Don't bother with the structure comments. It's readily apparent and they > have a nasty habit of not being true after a year or two. >> + >> + >> +static ssize_t tsl2x7x_power_state_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", chip->tsl2x7x_chip_status); >> +} >> + >> +static ssize_t tsl2x7x_power_state_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; > strtobool >> + >> + if (value == 0) >> + tsl2x7x_chip_off(dev_info); >> + else >> + tsl2x7x_chip_on(dev_info); >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_proximity_detect_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + tsl2x7x_prox_poll(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + chip->prox_cur_info.prox_event); >> +} >> + >> +static ssize_t tsl2x7x_gain_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + tsl2X7X_als_gainadj[chip->tsl2x7x_settings.als_gain].ch0); >> +} >> + >> +static ssize_t tsl2x7x_gain_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + switch (value) { >> + case 1: >> + chip->tsl2x7x_settings.als_gain = 0; >> + break; >> + case 8: >> + chip->tsl2x7x_settings.als_gain = 1; >> + break; >> + case 16: >> + chip->tsl2x7x_settings.als_gain = 2; >> + break; >> + case 120: >> + chip->tsl2x7x_settings.als_gain = 3; >> + break; > It's unlikely but you probably want to verify that 120 is only used > for parts where it is valid. (for consistency sake). >> + case 128: >> + chip->tsl2x7x_settings.als_gain = 3; >> + break; >> + >> + default: >> + dev_err(dev, >> + "Invalid Gain Index\n"); >> + return -EINVAL; >> + } >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_gain_available_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + if (strncmp(chip->client->name, "tsl2772", 7) == 0) >> + return snprintf(buf, PAGE_SIZE, "%s\n", "1 8 16 128"); >> + else >> + return snprintf(buf, PAGE_SIZE, "%s\n", "1 8 16 120"); >> +} >> + >> +static ssize_t tsl2x7x_prox_gain_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + (1<< chip->tsl2x7x_settings.prox_gain)); >> +} >> + >> +static ssize_t tsl2x7x_prox_gain_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + switch (value) { >> + case 1: > maybe use the relevant log2? Then you just need to check if it's too big. >> + chip->tsl2x7x_settings.prox_gain = 0; >> + break; >> + case 2: >> + chip->tsl2x7x_settings.prox_gain = 1; >> + break; >> + case 4: >> + chip->tsl2x7x_settings.prox_gain = 2; >> + break; >> + case 8: >> + chip->tsl2x7x_settings.prox_gain = 3; >> + break; >> + default: >> + dev_err(dev, >> + "Invalid Prox Gain Index\n"); >> + return -EINVAL; >> + } >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_prox_gain_available_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + return snprintf(buf, PAGE_SIZE, "%s\n", "1 2 4 8"); >> +} >> + >> +static ssize_t tsl2x7x_als_time_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + chip->tsl2x7x_settings.als_time); >> +} >> + >> +static ssize_t tsl2x7x_als_time_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + if ((value< 50) || (value> 650)) >> + return -EINVAL; >> + >> + if (value % 50) >> + return -EINVAL; >> + >> + chip->tsl2x7x_settings.als_time = value; >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_als_time_available_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + return snprintf(buf, PAGE_SIZE, "%s\n", >> + "50 100 150 200 250 300 350 400 450 500 550 600 650"); >> +} >> + >> +static ssize_t tsl2x7x_als_trim_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + chip->tsl2x7x_settings.als_gain_trim); >> +} >> + >> +static ssize_t tsl2x7x_als_trim_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + if (value) > I'd have thought 0 was a valid gain_trim? >> + chip->tsl2x7x_settings.als_gain_trim = value; >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_als_cal_target_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + chip->tsl2x7x_settings.als_cal_target); >> +} >> + >> +static ssize_t tsl2x7x_als_cal_target_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + if (value) >> + chip->tsl2x7x_settings.als_cal_target = value; >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_interrupts_en_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + if (chip->tsl2x7x_settings.interrupts_en& 0x10) >> + return snprintf(buf, PAGE_SIZE, "%d\n", 1); >> + else >> + return snprintf(buf, PAGE_SIZE, "%d\n", 0); > snprintf(buf, PAGE_SIZE, "%d\n", > !!(chip->tsl2x7x_settings.interrupts_en & 0x10)); >> +} >> + >> +static ssize_t tsl2x7x_interrupts_en_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + > strotobool (and use the iio_chan_spec and relevant callbacks please.) >> + if (value> 1) >> + return -EINVAL; >> + if (value) >> + chip->tsl2x7x_settings.interrupts_en |= 0x10; >> + else >> + chip->tsl2x7x_settings.interrupts_en&= 0x20; >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_prox_interrupt_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + if (chip->tsl2x7x_settings.interrupts_en& 0x20) >> + return snprintf(buf, PAGE_SIZE, "%d\n", 1); >> + else >> + return snprintf(buf, PAGE_SIZE, "%d\n", 0); >> +} >> + >> +static ssize_t tsl2x7x_prox_interrupt_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + if (value> 1) >> + return -EINVAL; >> + if (value) >> + chip->tsl2x7x_settings.interrupts_en |= 0x20; >> + else >> + chip->tsl2x7x_settings.interrupts_en&= 0x10; >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_als_thresh_low_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + chip->tsl2x7x_settings.als_thresh_low); >> +} >> + >> +static ssize_t tsl2x7x_als_thresh_low_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + chip->tsl2x7x_settings.als_thresh_low = value; >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_als_thresh_high_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + chip->tsl2x7x_settings.als_thresh_high); >> +} >> + >> +static ssize_t tsl2x7x_als_thresh_high_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + chip->tsl2x7x_settings.als_thresh_high = value; >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_prox_thresh_low_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + chip->tsl2x7x_settings.prox_thres_low); >> +} >> + >> +static ssize_t tsl2x7x_prox_thresh_low_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + chip->tsl2x7x_settings.prox_thres_low = value; >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_prox_thresh_high_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + chip->tsl2x7x_settings.prox_thres_high); >> +} >> + >> +static ssize_t tsl2x7x_prox_thresh_high_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + chip->tsl2x7x_settings.prox_thres_high = value; >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_prox_data_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + tsl2x7x_prox_poll(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> + chip->prox_cur_info.prox_data); >> +} >> + >> +/* sampling_frequency AKA persistence in data sheet */ >> +static ssize_t tsl2x7x_als_persistence_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "0x%02X\n", >> + chip->tsl2x7x_settings.als_persistence); >> +} >> + >> +static ssize_t tsl2x7x_als_persistence_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; >> + >> + chip->tsl2x7x_settings.als_persistence = value; >> + >> + return len; >> +} >> + >> +static >> +ssize_t tsl2x7x_als_persistence_available_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + return snprintf(buf, PAGE_SIZE, "0x00 - 0xFF (0 - 255)\n"); >> +} >> + >> +static >> +ssize_t tsl2x7x_lux_show(struct device *dev, struct device_attribute >> *attr, >> + char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + int lux; >> + >> + lux = tsl2x7x_get_lux(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", lux); >> +} >> + >> +static >> +ssize_t tsl2x7x_adc_show(struct device *dev, struct device_attribute >> *attr, >> + char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + tsl2x7x_get_lux(dev_info); >> + >> + return snprintf(buf, PAGE_SIZE, "0x%08x\n", >> + ((chip->als_cur_info.als_ch0<< 16) | >> + chip->als_cur_info.als_ch1)); > Your outputing in hex? Please don't and please do this through an > iio_chan_spec > structure and read_raw callback. >> +} >> + >> +static ssize_t tsl2x7x_do_calibrate(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; > strtobool? >> + >> + if (value == 1) >> + tsl2x7x_als_calibrate(dev_info); >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_luxtable_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + int i; >> + int offset = 0; >> + >> + i = 0; >> + while (i< (MAX_TABLE_SIZE * 3)) { >> + offset += snprintf(buf + offset, PAGE_SIZE, "%d,%d,%d,", >> + chip->tsl2x7x_device_lux[i].ratio, >> + chip->tsl2x7x_device_lux[i].ch0, >> + chip->tsl2x7x_device_lux[i].ch1); >> + if (chip->tsl2x7x_device_lux[i].ratio == 0) { >> + /* We just printed the first "0" entry. >> + * Now get rid of the extra "," and break. */ >> + offset--; >> + break; >> + } >> + i++; >> + } >> + >> + offset += snprintf(buf + offset, PAGE_SIZE, "\n"); >> + return offset; >> +} >> + >> +static ssize_t tsl2x7x_luxtable_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + int value[ARRAY_SIZE(chip->tsl2x7x_device_lux)*3 + 1]; >> + 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(chip->tsl2x7x_device_lux) - 1) * 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->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) >> + tsl2x7x_chip_off(dev_info); >> + >> + /* Zero out the table */ >> + memset(chip->tsl2x7x_device_lux, 0, >> sizeof(chip->tsl2x7x_device_lux)); >> + memcpy(chip->tsl2x7x_device_lux,&value[1], (value[0] * 4)); >> + >> + return len; >> +} >> + >> +static ssize_t tsl2x7x_do_prox_calibrate(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t len) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + unsigned long value; >> + >> + if (kstrtoul(buf, 0,&value)) >> + return -EINVAL; > strtobool probably? >> + >> + if (value == 1) >> + tsl2x7x_prox_cal(dev_info); >> + >> + return len; >> +} >> + >> +static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR, >> + tsl2x7x_power_state_show, tsl2x7x_power_state_store); >> + >> +static DEVICE_ATTR(proximity_raw, S_IRUGO, >> + tsl2x7x_proximity_detect_show, NULL); >> + >> +static DEVICE_ATTR(intensity_infrared_raw, S_IRUGO, >> + tsl2x7x_prox_data_show, NULL); >> + >> + >> +static DEVICE_ATTR(proximity_calibscale, S_IRUGO | S_IWUSR, >> + tsl2x7x_prox_gain_show, tsl2x7x_prox_gain_store); >> +static DEVICE_ATTR(proximity_calibscale_available, S_IRUGO, >> + tsl2x7x_prox_gain_available_show, NULL); >> + >> +static DEVICE_ATTR(illuminance0_calibscale, S_IRUGO | S_IWUSR, >> + tsl2x7x_gain_show, tsl2x7x_gain_store); >> +static DEVICE_ATTR(illuminance0_calibscale_available, S_IRUGO, >> + tsl2x7x_gain_available_show, NULL); >> + >> +static DEVICE_ATTR(illuminance0_integration_time, S_IRUGO | S_IWUSR, >> + tsl2x7x_als_time_show, tsl2x7x_als_time_store); >> +static DEVICE_ATTR(illuminance0_integration_time_available, S_IRUGO, >> + tsl2x7x_als_time_available_show, NULL); >> + >> +static DEVICE_ATTR(illuminance0_calibbias, S_IRUGO | S_IWUSR, >> + tsl2x7x_als_trim_show, tsl2x7x_als_trim_store); >> + >> +static DEVICE_ATTR(illuminance0_input_target, S_IRUGO | S_IWUSR, >> + tsl2x7x_als_cal_target_show, tsl2x7x_als_cal_target_store); >> + >> +static DEVICE_ATTR(illuminance0_both_raw, S_IRUGO, tsl2x7x_adc_show, >> + NULL); >> + >> +static DEVICE_ATTR(illuminance0_input, S_IRUGO, tsl2x7x_lux_show, >> + NULL); >> +static DEVICE_ATTR(illuminance0_calibrate, S_IWUSR, NULL, >> + tsl2x7x_do_calibrate); >> +static DEVICE_ATTR(proximity_calibrate, S_IWUSR, NULL, >> + tsl2x7x_do_prox_calibrate); >> + >> +static DEVICE_ATTR(illuminance0_lux_table, S_IRUGO | S_IWUSR, >> + tsl2x7x_luxtable_show, tsl2x7x_luxtable_store); >> + >> +static DEVICE_ATTR(illuminance0_thresh_falling_value, S_IRUGO | >> S_IWUSR, >> + tsl2x7x_als_thresh_low_show, tsl2x7x_als_thresh_low_store); >> + >> +static DEVICE_ATTR(illuminance0_thresh_rising_value, S_IRUGO | S_IWUSR, >> + tsl2x7x_als_thresh_high_show, tsl2x7x_als_thresh_high_store); >> + >> +static DEVICE_ATTR(sampling_frequency, S_IRUGO | S_IWUSR, >> + tsl2x7x_als_persistence_show, tsl2x7x_als_persistence_store); >> + >> +static DEVICE_ATTR(sampling_frequency_available, S_IRUGO, >> + tsl2x7x_als_persistence_available_show, NULL); >> + >> +static DEVICE_ATTR(proximity_thresh_rising_value, S_IRUGO | S_IWUSR, >> + tsl2x7x_prox_thresh_high_show, tsl2x7x_prox_thresh_high_store); >> + >> +static DEVICE_ATTR(proximity_thresh_falling_value, S_IRUGO | S_IWUSR, >> + tsl2x7x_prox_thresh_low_show, tsl2x7x_prox_thresh_low_store); >> + > A lot of what we having in here is standard and should be done via a > iio_chan_spec > definition. That'll cover calibscal, calibbias, input, raw, > thresh_rising_value, thresh_falling_value > (which incidentally should be in the event attributes). > That leaves us with > power_state - ideally not here but handled via a kernel wide control > (it gets suggested, > everyone agrees it's a good idea and no one implements it...) but such > is life. > calibscale_available - fine > integration_time - Propose adding this to the iio_chan_spec via > info_mask. > integration_time_available - fine. > input_target -> target_input but otherwise fine. > calibrate -> fine > lux_table -> fine > sampling_frequency, frequency_available fine. > > Just for reference, we really need to come up with a way of handing > 'available' within > the iio_chan_spec structures and the same for sampling frequency. Not > your problem > but if I say it often enough hopefully someone else will do it ;) >> +static struct attribute *tsl2571_device_attrs[] = { >> + &dev_attr_power_state.attr, >> + &dev_attr_illuminance0_calibscale.attr, >> + &dev_attr_illuminance0_calibscale_available.attr, >> + &dev_attr_illuminance0_integration_time.attr, >> + &dev_attr_illuminance0_integration_time_available.attr, >> + &dev_attr_illuminance0_calibbias.attr, >> + &dev_attr_illuminance0_input_target.attr, >> + &dev_attr_illuminance0_both_raw.attr, >> + &dev_attr_illuminance0_input.attr, >> + &dev_attr_illuminance0_calibrate.attr, >> + &dev_attr_illuminance0_lux_table.attr, >> + &dev_attr_illuminance0_thresh_falling_value.attr, >> + &dev_attr_illuminance0_thresh_rising_value.attr, >> + &dev_attr_sampling_frequency.attr, >> + &dev_attr_sampling_frequency_available.attr, >> + NULL >> +}; >> + >> +static struct attribute *tsl2671_device_attrs[] = { >> + &dev_attr_power_state.attr, >> + &dev_attr_proximity_raw.attr, >> + &dev_attr_intensity_infrared_raw.attr, >> + &dev_attr_sampling_frequency.attr, >> + &dev_attr_sampling_frequency_available.attr, >> + &dev_attr_proximity_thresh_rising_value.attr, >> + &dev_attr_proximity_thresh_falling_value.attr, >> + &dev_attr_proximity_calibrate.attr, >> + NULL >> +}; >> + >> +static struct attribute *tsl2771_device_attrs[] = { >> + &dev_attr_power_state.attr, >> + &dev_attr_proximity_raw.attr, >> + &dev_attr_intensity_infrared_raw.attr, >> + &dev_attr_illuminance0_calibscale.attr, >> + &dev_attr_illuminance0_calibscale_available.attr, >> + &dev_attr_illuminance0_integration_time.attr, >> + &dev_attr_illuminance0_integration_time_available.attr, >> + &dev_attr_illuminance0_calibbias.attr, >> + &dev_attr_illuminance0_input_target.attr, >> + &dev_attr_illuminance0_both_raw.attr, >> + &dev_attr_illuminance0_input.attr, >> + &dev_attr_illuminance0_calibrate.attr, >> + &dev_attr_illuminance0_lux_table.attr, >> + &dev_attr_illuminance0_thresh_falling_value.attr, >> + &dev_attr_illuminance0_thresh_rising_value.attr, >> + &dev_attr_sampling_frequency.attr, >> + &dev_attr_sampling_frequency_available.attr, >> + &dev_attr_proximity_thresh_rising_value.attr, >> + &dev_attr_proximity_thresh_falling_value.attr, >> + &dev_attr_proximity_calibrate.attr, >> + NULL >> +}; >> + >> +static struct attribute *tsl2572_device_attrs[] = { >> + &dev_attr_power_state.attr, >> + &dev_attr_illuminance0_calibscale.attr, >> + &dev_attr_illuminance0_calibscale_available.attr, >> + &dev_attr_illuminance0_integration_time.attr, >> + &dev_attr_illuminance0_integration_time_available.attr, >> + &dev_attr_illuminance0_calibbias.attr, >> + &dev_attr_illuminance0_input_target.attr, >> + &dev_attr_illuminance0_both_raw.attr, >> + &dev_attr_illuminance0_input.attr, >> + &dev_attr_illuminance0_calibrate.attr, >> + &dev_attr_illuminance0_lux_table.attr, >> + &dev_attr_illuminance0_thresh_falling_value.attr, >> + &dev_attr_illuminance0_thresh_rising_value.attr, >> + &dev_attr_sampling_frequency.attr, >> + &dev_attr_sampling_frequency_available.attr, >> + NULL >> +}; >> + >> +static struct attribute *tsl2672_device_attrs[] = { >> + &dev_attr_power_state.attr, >> + &dev_attr_proximity_raw.attr, >> + &dev_attr_intensity_infrared_raw.attr, >> + &dev_attr_proximity_calibscale.attr, >> + &dev_attr_sampling_frequency.attr, >> + &dev_attr_sampling_frequency_available.attr, >> + &dev_attr_proximity_thresh_rising_value.attr, >> + &dev_attr_proximity_thresh_falling_value.attr, >> + &dev_attr_proximity_calibrate.attr, >> + NULL >> +}; >> + > Indent the same for all of these. >> +static struct attribute *tsl2772_device_attrs[] = { >> + &dev_attr_power_state.attr, >> + &dev_attr_proximity_raw.attr, >> + &dev_attr_proximity_calibscale.attr, >> + &dev_attr_intensity_infrared_raw.attr, >> + &dev_attr_illuminance0_calibscale.attr, >> + &dev_attr_illuminance0_calibscale_available.attr, >> + &dev_attr_illuminance0_integration_time.attr, >> + &dev_attr_illuminance0_integration_time_available.attr, >> + &dev_attr_illuminance0_calibbias.attr, >> + &dev_attr_illuminance0_input_target.attr, >> + &dev_attr_illuminance0_both_raw.attr, >> + &dev_attr_illuminance0_input.attr, >> + &dev_attr_illuminance0_calibrate.attr, >> + &dev_attr_illuminance0_lux_table.attr, >> + &dev_attr_illuminance0_thresh_falling_value.attr, >> + &dev_attr_illuminance0_thresh_rising_value.attr, >> + &dev_attr_sampling_frequency.attr, >> + &dev_attr_sampling_frequency_available.attr, >> + &dev_attr_proximity_thresh_rising_value.attr, >> + &dev_attr_proximity_thresh_falling_value.attr, >> + &dev_attr_proximity_calibrate.attr, >> + &dev_attr_proximity_calibscale_available.attr, >> + NULL >> +}; >> + >> +static struct attribute_group tsl2X7X_dev_attr_group_tbl[] = { >> + [tsl2571] = { >> + .attrs = tsl2571_device_attrs, >> + }, >> + [tsl2671] = { >> + .attrs = tsl2671_device_attrs, >> + }, >> + [tmd2671] = { >> + .attrs = tsl2671_device_attrs, >> + }, >> + [tsl2771] = { >> + .attrs = tsl2771_device_attrs, >> + }, >> + [tmd2771] = { >> + .attrs = tsl2771_device_attrs, >> + }, >> + [tsl2572] = { >> + .attrs = tsl2572_device_attrs, >> + }, >> + [tsl2672] = { >> + .attrs = tsl2672_device_attrs, >> + }, >> + [tmd2672] = { >> + .attrs = tsl2672_device_attrs, >> + }, >> + [tsl2772] = { >> + .attrs = tsl2772_device_attrs, >> + }, >> + [tmd2772] = { >> + .attrs = tsl2772_device_attrs, >> + }, >> + >> +}; >> + >> +static IIO_DEVICE_ATTR(illuminance_thresh_both_en, >> + S_IRUGO | S_IWUSR, >> + tsl2x7x_interrupts_en_show, tsl2x7x_interrupts_en_store, 0); >> + >> +static IIO_DEVICE_ATTR(proximity_thresh_both_en, >> + S_IRUGO | S_IWUSR, >> + tsl2x7x_prox_interrupt_show, tsl2x7x_prox_interrupt_store, 0); >> + >> +static struct attribute *tsl2X7X_prox_event_attributes[] = { >> + &iio_dev_attr_proximity_thresh_both_en.dev_attr.attr, >> + NULL, >> +}; >> + >> +static struct attribute *tsl2X7X_als_event_attributes[] = { >> + &iio_dev_attr_illuminance_thresh_both_en.dev_attr.attr, >> + NULL, >> +}; >> + >> +static struct attribute *tsl2X7X_proxals_event_attributes[] = { >> + &iio_dev_attr_illuminance_thresh_both_en.dev_attr.attr, >> + &iio_dev_attr_proximity_thresh_both_en.dev_attr.attr, >> + NULL, >> +}; > The way we have handled this for some similar devices is to just have > the two enables done > via the iio_chan_spec and chain them both to read / write the same > value. (By which I mean > the rising enable and falling enable). It slightly clunkier as an > interface, but saves code and will > ultimately enable them to be accessed by consumer drivers within the > kernel (not that we've > even started looking at event pass >> + >> +static struct attribute_group tsl2X7X_event_attr_group_tbl[] = { >> + [tsl2571] = { >> + .attrs = tsl2X7X_als_event_attributes, >> + .name = "events", >> + }, >> + [tsl2671] = { >> + .attrs = tsl2X7X_prox_event_attributes, >> + .name = "events", >> + }, >> + [tmd2671] = { >> + .attrs = tsl2X7X_prox_event_attributes, >> + .name = "events", >> + }, >> + [tsl2771] = { >> + .attrs = tsl2X7X_proxals_event_attributes, >> + .name = "events", >> + }, >> + [tmd2771] = { >> + .attrs = tsl2X7X_proxals_event_attributes, >> + .name = "events", >> + }, >> + >> + [tsl2572] = { >> + .attrs = tsl2X7X_als_event_attributes, >> + .name = "events", >> + }, >> + [tsl2672] = { >> + .attrs = tsl2X7X_prox_event_attributes, >> + .name = "events", >> + }, >> + [tmd2672] = { >> + .attrs = tsl2X7X_prox_event_attributes, >> + .name = "events", >> + }, >> + [tsl2772] = { >> + .attrs = tsl2X7X_proxals_event_attributes, >> + .name = "events", >> + }, >> + [tmd2772] = { >> + .attrs = tsl2X7X_proxals_event_attributes, >> + .name = "events", >> + } >> +}; >> + > Being fussy, but only one blank line please. >> + >> +/* Use the default register values to identify the Taos device */ >> +static int tsl2x7x_device_id(unsigned char *bufp, int target) >> +{ >> + switch (target) { >> + case tsl2571: >> + case tsl2671: >> + case tsl2771: >> + return ((bufp[TSL2X7X_CHIPID]& 0xf0) == TRITON_ID); >> + break; >> + case tmd2671: >> + case tmd2771: >> + return ((bufp[TSL2X7X_CHIPID]& 0xf0) == HALIBUT_ID); >> + break; >> + case tsl2572: >> + case tsl2672: >> + case tmd2672: >> + case tsl2772: >> + case tmd2772: >> + return ((bufp[TSL2X7X_CHIPID]& 0xf0) == SWORDFISH_ID); >> + break; >> + } >> + >> + return -EINVAL; >> +} >> + >> +static const struct iio_info tsl2X7X_info[] = { >> + [tsl2571] = { >> + .attrs =&tsl2X7X_dev_attr_group_tbl[tsl2571], >> + .event_attrs =&tsl2X7X_event_attr_group_tbl[tsl2571], >> + .driver_module = THIS_MODULE, >> + }, >> + [tsl2671] = { >> + .attrs =&tsl2X7X_dev_attr_group_tbl[tsl2671], >> + .event_attrs =&tsl2X7X_event_attr_group_tbl[tsl2671], >> + .driver_module = THIS_MODULE, >> + }, >> + [tmd2671] = { >> + .attrs =&tsl2X7X_dev_attr_group_tbl[tsl2671], >> + .event_attrs =&tsl2X7X_event_attr_group_tbl[tsl2671], >> + .driver_module = THIS_MODULE, >> + }, >> + [tsl2771] = { >> + .attrs =&tsl2X7X_dev_attr_group_tbl[tsl2771], >> + .event_attrs =&tsl2X7X_event_attr_group_tbl[tsl2771], >> + .driver_module = THIS_MODULE, >> + }, >> + [tmd2771] = { >> + .attrs =&tsl2X7X_dev_attr_group_tbl[tsl2771], >> + .event_attrs =&tsl2X7X_event_attr_group_tbl[tsl2771], >> + .driver_module = THIS_MODULE, >> + }, >> + [tsl2572] = { >> + .attrs =&tsl2X7X_dev_attr_group_tbl[tsl2572], >> + .event_attrs =&tsl2X7X_event_attr_group_tbl[tsl2572], >> + .driver_module = THIS_MODULE, >> + }, >> + [tsl2672] = { >> + .attrs =&tsl2X7X_dev_attr_group_tbl[tsl2672], >> + .event_attrs =&tsl2X7X_event_attr_group_tbl[tsl2672], >> + .driver_module = THIS_MODULE, >> + }, >> + [tmd2672] = { >> + .attrs =&tsl2X7X_dev_attr_group_tbl[tsl2672], >> + .event_attrs =&tsl2X7X_event_attr_group_tbl[tsl2672], >> + .driver_module = THIS_MODULE, >> + }, >> + [tsl2772] = { >> + .attrs =&tsl2X7X_dev_attr_group_tbl[tsl2772], >> + .event_attrs =&tsl2X7X_event_attr_group_tbl[tsl2772], >> + .driver_module = THIS_MODULE, >> + }, >> + [tmd2772] = { >> + .attrs =&tsl2X7X_dev_attr_group_tbl[tsl2772], >> + .event_attrs =&tsl2X7X_event_attr_group_tbl[tsl2772], >> + .driver_module = THIS_MODULE, > A lot of repeats in here, could you not have fewer entries? Just > reuse the enum values > in the i2c id table below. >> + }, >> + >> +}; >> + >> +/* >> + * Run-time interrupt handler - depending on whether the device is >> in ambient >> + * light sensing interrupt mode, this handler can queue up >> + * a thread, to handle valid interrupts. >> + */ >> +static irqreturn_t tsl2x7x_event_handler(int irq, void *private) >> +{ >> + struct iio_dev *indio_dev = private; >> + struct tsl2X7X_chip *chip = iio_priv(indio_dev); >> + s64 timestamp = iio_get_time_ns(); >> + int ret; >> + int value; >> + >> + value = i2c_smbus_read_byte_data(chip->client, >> + TSL2X7X_CMD_REG | TSL2X7X_STATUS); >> + >> + /* What type of interrupt do we need to process */ >> + if (value& TSL2X7X_STA_PRX_INTR) { >> + tsl2x7x_prox_poll(indio_dev); >> + iio_push_event(indio_dev, >> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, >> + 0, >> + IIO_EV_TYPE_THRESH, >> + IIO_EV_DIR_EITHER), >> + timestamp); >> + } >> + >> + if (value& TSL2X7X_STA_ALS_INTR) { >> + tsl2x7x_get_lux(indio_dev); >> + iio_push_event(indio_dev, >> + IIO_UNMOD_EVENT_CODE(IIO_LIGHT, >> + 0, >> + IIO_EV_TYPE_THRESH, >> + IIO_EV_DIR_EITHER), >> + timestamp); >> + } >> + /* Clear interrupt now that we have the status */ >> + ret = i2c_smbus_write_byte(chip->client, >> + TSL2X7X_CMD_REG | TSL2X7X_CMD_SPL_FN | >> + TSL2X7X_CMD_PROXALS_INTCLR); >> + if (ret< 0) >> + dev_err(&chip->client->dev, >> + "Failed to clear irq from event handler. err = %d\n", ret); >> + >> + return IRQ_HANDLED; >> +} >> + >> +/* >> + * Client probe function. >> + */ >> +static int __devinit tsl2x7x_probe(struct i2c_client *clientp, >> + const struct i2c_device_id *id) >> +{ >> + int i, ret; >> + unsigned char buf[TSL2X7X_MAX_DEVICE_REGS]; >> + struct iio_dev *indio_dev = >> + iio_allocate_device(sizeof(struct tsl2X7X_chip)); > Ugly to do non trivial allocation in the variable definitions. Have > struct iio_dev *indio_dev; > struct tsl2x7x_chip *chip; > > indio_dev = iio_allocate_device(sizeof(*chip)); > if (indio_dev == NULL) { > } > chip = iio_priv(indio_dev); > etc. >> + struct tsl2X7X_chip *chip = iio_priv(indio_dev); >> + >> + if (indio_dev == NULL) { >> + ret = -ENOMEM; >> + dev_err(&clientp->dev, "iio allocation failed\n"); >> + goto fail1; >> + } >> + >> + chip = iio_priv(indio_dev); >> + chip->client = clientp; >> + i2c_set_clientdata(clientp, indio_dev); >> + >> + mutex_init(&chip->als_mutex); >> + mutex_init(&chip->prox_mutex); >> + >> + chip->tsl2x7x_chip_status = TSL2X7X_CHIP_UNKNOWN; >> + >> + chip->pdata = clientp->dev.platform_data; >> + if (chip->pdata&& chip->pdata->init) >> + chip->pdata->init(clientp); >> + >> + for (i = 0; i< TSL2X7X_MAX_DEVICE_REGS; i++) { >> + ret = i2c_smbus_write_byte(clientp, >> + (TSL2X7X_CMD_REG | (TSL2X7X_CNTRL + i))); >> + if (ret< 0) { >> + dev_err(&clientp->dev, "i2c_smbus_write_bytes() to cmd " >> + "reg failed in tsl2x7x_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 tsl2x7x_probe(), err = %d\n", ret); >> + >> + goto fail1; >> + } >> + buf[i] = ret; >> + } >> + >> + if ((!tsl2x7x_device_id(buf, id->driver_data)) || >> + (tsl2x7x_device_id(buf, id->driver_data) == -EINVAL)) { >> + dev_info(&chip->client->dev, "i2c device found but does not >> match " >> + "expected id in tsl2x7x_probe()\n"); >> + goto fail1; >> + } >> + >> + chip->id = id->driver_data; >> + >> + ret = i2c_smbus_write_byte(clientp, (TSL2X7X_CMD_REG | >> TSL2X7X_CNTRL)); >> + if (ret< 0) { >> + dev_err(&clientp->dev, "i2c_smbus_write_byte() to cmd reg " >> + "failed in tsl2x7x_probe(), err = %d\n", ret); >> + goto fail1; >> + } >> + >> + indio_dev->info =&tsl2X7X_info[id->driver_data]; >> + indio_dev->dev.parent =&clientp->dev; >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + indio_dev->name = chip->client->name; >> + indio_dev->channels = tsl2X7X_channels; >> + indio_dev->num_channels = ARRAY_SIZE(tsl2X7X_channels); >> + >> + ret = iio_device_register(indio_dev); >> + if (ret) { >> + dev_err(&clientp->dev, "iio registration failed\n"); >> + goto fail2; > one would normally expect a later failure to result in more needing to > be undone, > but it appears to be the other way around here. >> + } >> + >> + if (clientp->irq) { >> + ret = request_threaded_irq(clientp->irq, >> + NULL, >> + &tsl2x7x_event_handler, >> + IRQF_TRIGGER_RISING | IRQF_ONESHOT, >> + "TSL2X7X_event", >> + indio_dev); >> + if (ret) >> + dev_err(&clientp->dev, "irq request failed"); > Unwinding after this error? Generally set the irq up and make the > iio_device_register > pretty much the last thing to happen rather than this way around. > Nothing can > touch the device before the iio_device_register so it avoids the > chance of race > conditions without having to be careful! >> + } >> + >> + /* Load up the defaults */ >> + tsl2x7x_defaults(chip); >> + >> + /* Make sure the chip is on */ >> + tsl2x7x_chip_on(indio_dev); >> + >> + dev_info(&clientp->dev, "%s Light sensor found.\n", id->name); >> + >> + return 0; >> + >> +fail1: >> + if (chip->pdata&& chip->pdata->teardown) >> + chip->pdata->teardown(clientp); >> + iio_free_device(indio_dev); >> + >> +fail2: >> + return ret; > loose the bonus blank line. >> + >> +} >> + >> + >> +static int tsl2x7x_suspend(struct device *dev) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); > standardize the naming please. indio_dev preferably but consistency > is most > important. >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + >> + int ret = 0; >> + >> + /* Blocks if work is active */ >> + cancel_work_sync(&chip->work_thresh); >> + >> + if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) { >> + ret = tsl2x7x_chip_off(dev_info); >> + chip->tsl2x7x_chip_status = TSL2X7X_CHIP_SUSPENDED; >> + } >> + >> + if (chip->pdata&& chip->pdata->platform_power) { >> + pm_message_t pmm = {PM_EVENT_SUSPEND}; >> + chip->pdata->platform_power(dev, pmm); >> + } >> + >> + return ret; >> +} >> + >> +static int tsl2x7x_resume(struct device *dev) >> +{ >> + struct iio_dev *dev_info = dev_get_drvdata(dev); >> + struct tsl2X7X_chip *chip = iio_priv(dev_info); >> + int ret = 0; >> + >> + if (chip->pdata&& chip->pdata->platform_power) { >> + pm_message_t pmm = {PM_EVENT_RESUME}; >> + chip->pdata->platform_power(dev, pmm); >> + } >> + >> + if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_SUSPENDED) >> + ret = tsl2x7x_chip_on(dev_info); >> + >> + return ret; >> +} >> + >> +static int __devexit tsl2x7x_remove(struct i2c_client *client) >> +{ >> + struct iio_dev *indio_dev = i2c_get_clientdata(client); >> + struct tsl2X7X_chip *chip = i2c_get_clientdata(client); > Err, the two lines above seem a little implausible. >> + >> + tsl2x7x_chip_off(indio_dev); >> + >> + if (client->irq) >> + free_irq(client->irq, chip->client->name); >> + >> + flush_scheduled_work(); >> + >> + iio_device_unregister(chip->iio_dev); > or use the indio_dev pointer you already have. There is rarely a reason > to have a pointer to the iio_dev in the driver private bit. >> + kfree(chip); >> + >> + if (chip->pdata&& chip->pdata->teardown) >> + chip->pdata->teardown(client); >> + >> + return 0; >> +} >> + >> +static struct i2c_device_id tsl2x7x_idtable[] = { > I'm guessing there is a magic ordering here, but might be > nicer just to go with alphabetical order.. >> + { "tsl2571", tsl2571 }, >> + { "tsl2671", tsl2671 }, >> + { "tmd2671", tmd2671 }, >> + { "tsl2771", tsl2771 }, >> + { "tmd2771", tmd2771 }, >> + { "tsl2572", tsl2572 }, >> + { "tsl2672", tsl2672 }, >> + { "tmd2672", tmd2672 }, >> + { "tsl2772", tsl2772 }, >> + { "tmd2772", tmd2772 }, >> + {} >> +}; >> + >> +MODULE_DEVICE_TABLE(i2c, tsl2x7x_idtable); >> + >> +static const struct dev_pm_ops tsl2x7x_pm_ops = { >> + .suspend = tsl2x7x_suspend, >> + .resume = tsl2x7x_resume, >> +}; >> + >> +/* Driver definition */ >> +static struct i2c_driver tsl2x7x_driver = { >> + .driver = { >> + .name = "tsl2X7X", >> + .pm =&tsl2x7x_pm_ops, >> + >> + }, >> + .id_table = tsl2x7x_idtable, >> + .probe = tsl2x7x_probe, >> + .remove = __devexit_p(tsl2x7x_remove), >> +}; >> + >> +static int __init tsl2x7x_init(void) >> +{ >> + return i2c_add_driver(&tsl2x7x_driver); >> +} >> + >> +static void __exit tsl2x7x_exit(void) >> +{ >> + i2c_del_driver(&tsl2x7x_driver); >> +} >> + >> +module_init(tsl2x7x_init); >> +module_exit(tsl2x7x_exit); > Some nice new macros to handle this boiler plate. > see module_i2c_driver in include/linux/i2c.h > >> + >> +MODULE_AUTHOR("J. August Brenner<jbrenner@taosinc.com>"); >> +MODULE_DESCRIPTION("TAOS tsl2X7X ambient light sensor driver"); >> +MODULE_LICENSE("GPL"); >> diff --git a/drivers/staging/iio/light/tsl2x7x_core.h >> b/drivers/staging/iio/light/tsl2x7x_core.h >> new file mode 100644 >> index 0000000..9a64412 >> --- /dev/null >> +++ b/drivers/staging/iio/light/tsl2x7x_core.h >> @@ -0,0 +1,138 @@ >> +/* >> + * Device driver for monitoring ambient light intensity (lux) >> + * and proximity (prox) within the TAOS TSL2X7X family of devices. >> + * >> + * Copyright (c) 2012, 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. >> + */ >> + >> +#ifndef __TSL2X7X_H >> +#define __TSL2X7X_H >> +#include<linux/pm.h> >> + >> +#include "../iio.h" > loose this header and just forward declare the pointer. > e.g. struct iio_dev; > >> + >> +/* TAOS Register definitions - note: >> + * depending on device, some of these register are not used and the >> + * register address is benign. >> + */ >> +/* 2X7X register offsets */ >> +#define TSL2X7X_MAX_DEVICE_REGS 32 >> +#define TSL2X7X_REG_MAX 16 >> + >> +/* Device Registers and Masks */ >> +#define TSL2X7X_CNTRL 0x00 >> +#define TSL2X7X_ALS_TIME 0X01 >> +#define TSL2X7X_PRX_TIME 0x02 >> +#define TSL2X7X_WAIT_TIME 0x03 >> +#define TSL2X7X_ALS_MINTHRESHLO 0X04 >> +#define TSL2X7X_ALS_MINTHRESHHI 0X05 >> +#define TSL2X7X_ALS_MAXTHRESHLO 0X06 >> +#define TSL2X7X_ALS_MAXTHRESHHI 0X07 >> +#define TSL2X7X_PRX_MINTHRESHLO 0X08 >> +#define TSL2X7X_PRX_MINTHRESHHI 0X09 >> +#define TSL2X7X_PRX_MAXTHRESHLO 0X0A >> +#define TSL2X7X_PRX_MAXTHRESHHI 0X0B >> +#define TSL2X7X_PERSISTENCE 0x0C >> +#define TSL2X7X_PRX_CONFIG 0x0D >> +#define TSL2X7X_PRX_COUNT 0x0E >> +#define TSL2X7X_GAIN 0x0F >> +#define TSL2X7X_NOTUSED 0x10 >> +#define TSL2X7X_REVID 0x11 >> +#define TSL2X7X_CHIPID 0x12 >> +#define TSL2X7X_STATUS 0x13 >> +#define TSL2X7X_ALS_CHAN0LO 0x14 >> +#define TSL2X7X_ALS_CHAN0HI 0x15 >> +#define TSL2X7X_ALS_CHAN1LO 0x16 >> +#define TSL2X7X_ALS_CHAN1HI 0x17 >> +#define TSL2X7X_PRX_LO 0x18 >> +#define TSL2X7X_PRX_HI 0x19 >> + >> +/* tsl2X7X cmd reg masks */ >> +#define TSL2X7X_CMD_REG 0x80 >> +#define TSL2X7X_CMD_SPL_FN 0x60 >> + >> +#define TSL2X7X_CMD_PROX_INT_CLR 0X05 >> +#define TSL2X7X_CMD_ALS_INT_CLR 0x06 >> +#define CMD_PROXALS_INT_CLR 0X07 >> + >> +/* tsl2X7X cntrl reg masks */ >> +#define TSL2X7X_CNTL_ADC_ENBL 0x02 >> +#define TSL2X7X_CNTL_PWR_ON 0x01 >> + >> +/* tsl2X7X status reg masks */ >> +#define TSL2X7X_STA_ADC_VALID 0x01 >> +#define TSL2X7X_STA_PRX_VALID 0x02 >> +#define TSL2X7X_STA_ADC_PRX_VALID 0x03 >> +#define TSL2X7X_STA_ALS_INTR 0x10 >> +#define TSL2X7X_STA_ADC_INTR 0x10 >> +#define TSL2X7X_STA_PRX_INTR 0x20 >> + >> +#define TSL2X7X_STA_ADC_INTR 0x10 >> + >> +/* tsl2X7X cntrl reg masks */ >> +#define CNTL_REG_CLEAR 0x00 >> +#define CNTL_PROX_INT_ENBL 0X20 >> +#define CNTL_ALS_INT_ENBL 0X10 >> +#define TSL2X7X_CNTL_WAIT_TMR_ENBL 0X08 >> +#define CNTL_PROX_DET_ENBL 0X04 >> +#define CNTL_ADC_ENBL 0x02 >> +#define TSL2X7X_CNTL_PWRON 0x01 >> +#define CNTL_ALSPON_ENBL 0x03 >> +#define CNTL_INTALSPON_ENBL 0x13 >> +#define CNTL_PROXPON_ENBL 0x0F >> +#define CNTL_INTPROXPON_ENBL 0x2F >> +#define TSL2X7X_CMD_PROXALS_INTCLR 0X07 >> + > Chances of a clash on these definitions is extremely high, > Stick a prefix in front of them to avoid that issue. >> +/*Prox diode to use */ >> +#define DIODE0 0x10 >> +#define DIODE1 0x20 >> +#define DIODE_BOTH 0x30 >> + >> +/* LED Power */ >> +#define mA100 0x00 >> +#define mA50 0x40 >> +#define mA25 0x80 >> +#define mA13 0xD0 >> + >> +/* Max number of segments allowable in LUX table */ >> +#define MAX_TABLE_SIZE 9 >> +#define MAX_DEFAULT_TABLE_BYTES (sizeof(int) * MAX_TABLE_SIZE) >> + >> +struct tsl2x7x_lux { >> + unsigned int ratio; >> + unsigned int ch0; >> + unsigned int ch1; >> +}; >> + >> + > Kernel doc for comments would be preferable. Can you also give a > description of what > actually tends to occur in these callbacks? I can't help feeling > there are rather more of > them than we'd normally expect. >> +/* struct tsl2x7x_platform_data - Assume all varients will have this */ >> +struct tsl2X7X_platform_data { >> + int (*platform_power)(struct device *dev, pm_message_t); >> + /* The following callback gets called when the TSL2772 is >> powered on */ >> + int (*power_on) (struct iio_dev *indio_dev); >> + /* The following callback gets called when the TSL2772 is >> powered off */ >> + int (*power_off) (struct i2c_client *dev); >> + /* The following callback gets called when the driver is added */ >> + int (*init) (struct i2c_client *dev); >> + /* The following callback gets called when the driver is removed */ >> + int (*teardown) (struct i2c_client *dev); >> + /* These are the device specific glass coefficents used to >> + * calculate Lux */ >> + struct tsl2x7x_lux platform_lux_table[MAX_TABLE_SIZE]; >> +}; >> + >> +#endif /* __TSL2X7X_H */ >> -- >> > > -- > 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
| |