RE: [PATCH V3]TAOS 258x: Device Driver

From: Shubhrajyoti Datta
Date: Tue Mar 29 2011 - 01:38:18 EST

> -----Original Message-----
> From: Jon Brenner [mailto:jbrenner@xxxxxxxxxxx]
> Sent: Tuesday, March 29, 2011 12:21 AM
> To: Shubhrajyoti
> Cc: Jonathan Cameron; linux-iio; Linux Kernel
> Subject: Re: [PATCH V3]TAOS 258x: Device Driver
>
> HI Shubhrajyoti:
> Thanks for taking time to review the code.
> Please see my response to your comments throughout.
>
>
> Jon
>
> On Sat, 2011-03-26 at 22:47 +0530, Shubhrajyoti wrote:
> > Hi Jon,
> > Some minor comments.
> >
> > On Saturday 26 March 2011 03:11 AM, Jon Brenner wrote:
> > > [PATCH v3] for TAOS tsl2580/81/83 Device driver
> > >
> > > Added suspend/resume functions.
> > > Changed attribute names to match existing where applicable and updated
> or documented new ABI as discussed.
> > > Changed integration time ABI from using index (0 to 3) to use actual
> gain values (1x,8x, etc.).
> > > Removed various unused variables, declarations, and functions.
> > > Revised code to accommodate different endianess (le16_to_cpu).
> > > Updated error return codes in various functions.
> > > Changed from mdelay to msleep after determining that longer wait would
> be acceptable.
> > >
> > > Signed-off-by:Jon August Brenner<jbrenner@xxxxxxxxxxx>
> > >
> > > ---
> > > drivers/staging/iio/Documentation/sysfs-bus-iio | 6 +
> > > .../staging/iio/Documentation/sysfs-bus-iio-light | 13 +
> > > .../iio/Documentation/sysfs-bus-iio-light-tsl2583 | 20 +
> > > drivers/staging/iio/light/Kconfig | 9 +
> > > drivers/staging/iio/light/Makefile | 1 +
> > > drivers/staging/iio/light/tsl2583.c | 962
> ++++++++++++++++++++
> > > 6 files changed, 1011 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio
> b/drivers/staging/iio/Documentation/sysfs-bus-iio
> > > index 2dde97d..6a86ad2 100644
> > > --- a/drivers/staging/iio/Documentation/sysfs-bus-iio
> > > +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio
> > > @@ -6,6 +6,12 @@ Description:
> > > Corresponds to a grouping of sensor channels. X is the
> IIO
> > > index of the device.
> > >
> > > +What: /sys/bus/iio/devices/device[n]/power_state
> > > +KernelVersion: 2.6.37
> > > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > > +Description:
> > > + This property gets/sets the device power state.
> > > +
> > > What: /sys/bus/iio/devices/triggerX
> > > KernelVersion: 2.6.35
> > > Contact: linux-iio@xxxxxxxxxxxxxxx
> > > diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> > > index 5d84856..21d2774 100644
> > > --- a/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> > > +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light
> > > @@ -62,3 +62,16 @@ Description:
> > > sensing mode. This value should be the output from a
> reading
> > > and if expressed in SI units, should include _input. If
> this
> > > value is not in SI units, then it should include _raw.
> > > +
> > > +What: /sys/bus/iio/devices/device[n]/illuminance0_target
> > > +KernelVersion: 2.6.37
> > > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > > +Description:
> > > + This property gets/sets the last known external
> > > + lux measurement used in/for calibration.
> > > +
> > > +What:
> /sys/bus/iio/devices/device[n]/illuminance0_integration_time
> > > +KernelVersion: 2.6.37
> > > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > > +Description:
> > > + This property gets/sets the sensors ADC analog integration
> time.
> > > diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-
> tsl2583 b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-tsl2583
> > > new file mode 100644
> > > index 0000000..660781d
> > > --- /dev/null
> > > +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-tsl2583
> > > @@ -0,0 +1,20 @@
> > > +What: /sys/bus/iio/devices/device[n]/lux_table
> > > +KernelVersion: 2.6.37
> > > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > > +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@xxxxxxxxxxxxxxx
> > > +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_input_target
> > > +KernelVersion: 2.6.37
> > > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > > +Description:
> > > + This property is the known externally illuminance (in lux).
> > > + It is used in the process of calibrating the device accuracy.
> > > diff --git a/drivers/staging/iio/light/Kconfig
> b/drivers/staging/iio/light/Kconfig
> > > index 36d8bbe..55c146c 100644
> > > --- a/drivers/staging/iio/light/Kconfig
> > > +++ b/drivers/staging/iio/light/Kconfig
> > > @@ -13,6 +13,15 @@ config SENSORS_TSL2563
> > > This driver can also be built as a module. If so, the module
> > > will be called tsl2563.
> > >
> > > +config TSL2583
> > > + tristate "TAOS TSL2580, TSL2581, and TSL2583 light-to-digital
> converters"
> > > + depends on I2C
> > > + help
> > > + Y = in kernel.
> > > + M = as module.
> > > + Provides support for the TAOS tsl2580, tsl2581, and tsl2583
> devices.
> > > + Access ALS data via iio, sysfs.
> > > +
> > > config SENSORS_ISL29018
> > > tristate "ISL 29018 light and proximity sensor"
> > > depends on I2C
> > > diff --git a/drivers/staging/iio/light/Makefile
> b/drivers/staging/iio/light/Makefile
> > > index 9142c0e..8a10815 100644
> > > --- a/drivers/staging/iio/light/Makefile
> > > +++ b/drivers/staging/iio/light/Makefile
> > > @@ -3,4 +3,5 @@
> > > #
> > >
> > > obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
> > > +obj-$(CONFIG_TSL2583) += tsl2583.o
> > > obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o
> > > diff --git a/drivers/staging/iio/light/tsl2583.c
> b/drivers/staging/iio/light/tsl2583.c
> > > new file mode 100644
> > > index 0000000..013a742
> > > --- /dev/null
> > > +++ b/drivers/staging/iio/light/tsl2583.c
> > > @@ -0,0 +1,962 @@
> > > +/*
> > > + * Device driver for monitoring ambient light intensity (lux)
> > > + * within the TAOS tsl258x family of devices (tsl2580, tsl2581).
> > > + *
> > > + * Copyright (c) 2011, TAOS Corporation.
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> modify
> > > + * it under the terms of the GNU General Public License as published
> by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> but WITHOUT
> > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> or
> > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> License for
> > > + * more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> along
> > > + * with this program; if not, write to the Free Software Foundation,
> Inc.,
> > > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> > > + */
> > > +
> > > +#include<linux/kernel.h>
> > > +#include<linux/i2c.h>
> > > +#include<linux/errno.h>
> > > +#include<linux/delay.h>
> > > +#include<linux/string.h>
> > > +#include<linux/mutex.h>
> > > +#include<linux/unistd.h>
> > > +#include "../iio.h"
> > > +
> > > +
> > > +#define TSL258X_MAX_DEVICE_REGS 32
> > > +
> > > +/* Triton register offsets */
> > > +#define TSL258X_REG_MAX 8
> > > +
> > > +/* Device Registers and Masks */
> > > +#define TSL258X_CNTRL 0x00
> > > +#define TSL258X_ALS_TIME 0X01
> > > +#define TSL258X_INTERRUPT 0x02
> > > +#define TSL258X_GAIN 0x07
> > > +#define TSL258X_REVID 0x11
> > > +#define TSL258X_CHIPID 0x12
> > > +#define TSL258X_ALS_CHAN0LO 0x14
> > > +#define TSL258X_ALS_CHAN0HI 0x15
> > > +#define TSL258X_ALS_CHAN1LO 0x16
> > > +#define TSL258X_ALS_CHAN1HI 0x17
> > > +#define TSL258X_TMR_LO 0x18
> > > +#define TSL258X_TMR_HI 0x19
> > > +
> > > +/* tsl2583 cmd reg masks */
> > > +#define TSL258X_CMD_REG 0x80
> > > +#define TSL258X_CMD_SPL_FN 0x60
> > > +#define TSL258X_CMD_ALS_INT_CLR 0X01
> > > +
> > > +/* tsl2583 cntrl reg masks */
> > > +#define TSL258X_CNTL_ADC_ENBL 0x02
> > > +#define TSL258X_CNTL_PWR_ON 0x01
> > > +
> > > +/* tsl2583 status reg masks */
> > > +#define TSL258X_STA_ADC_VALID 0x01
> > > +#define TSL258X_STA_ADC_INTR 0x10
> > > +
> > > +/* Lux calculation constants */
> > > +#define TSL258X_LUX_CALC_OVER_FLOW 65535
> > > +
> > > +enum {
> > > + TSL258X_CHIP_UNKNOWN = 0,
> > > + TSL258X_CHIP_WORKING = 1,
> > > + TSL258X_CHIP_SUSPENDED = 2
> > > +} TSL258X_CHIP_WORKING_STATUS;
> > > +
> > > +/* Per-device data */
> > > +struct taos_als_info {
> > > + u16 als_ch0;
> > > + u16 als_ch1;
> > > + u16 lux;
> > > +};
> > > +
> > > +struct taos_settings {
> > > + int als_time;
> > > + int als_gain;
> > > + int als_gain_trim;
> > > + int als_cal_target;
> > > +};
> > > +
> > > +struct tsl2583_chip {
> > > + struct mutex als_mutex;
> > > + struct i2c_client *client;
> > > + struct iio_dev *iio_dev;
> > > + struct taos_als_info als_cur_info;
> > > + struct taos_settings taos_settings;
> > > + int als_time_scale;
> > > + int als_saturation;
> > > + int taos_chip_status;
> > > + u8 taos_config[8];
> > > +};
> > > +
> > > +/*
> > > + * Initial values for device - this values can/will be changed by
> driver.
> > > + * and applications as needed.
> > > + * These values are dynamic.
> > > + */
> > > +static const u8 taos_config[8] = {
> > > + 0x00, 0xee, 0x00, 0x03, 0x00, 0xFF, 0xFF, 0x00
> > > +}; /* cntrl atime intC Athl0 Athl1 Athh0 Athh1 gain */
> > > +
> > > +struct taos_lux {
> > > + unsigned int ratio;
> > > + unsigned int ch0;
> > > + unsigned int ch1;
> > > +};
> > > +
> > > +/* This structure is intentionally large to accommodate updates via
> sysfs. */
> > > +/* Sized to 11 = max 10 segments + 1 termination segment */
> > > +/* Assumption is is one and only one type of glass used */
> > > +struct taos_lux taos_device_lux[11] = {
> > > + { 9830, 8520, 15729 },
> > > + { 12452, 10807, 23344 },
> > > + { 14746, 6383, 11705 },
> > > + { 17695, 4063, 6554 },
> > > +};
> > > +
> > > +struct gainadj {
> > > + s16 ch0;
> > > + s16 ch1;
> > > +};
> > > +
> > > +/* Index = (0 - 3) Used to validate the gain selection index */
> > > +static const struct gainadj gainadj[] = {
> > > + { 1, 1 },
> > > + { 8, 8 },
> > > + { 16, 16 },
> > > + { 107, 115 }
> > > +};
> > > +
> > > +/*
> > > + * Provides initial operational parameter defaults.
> > > + * These defaults may be changed through the device's sysfs files.
> > > + */
> > > +static void taos_defaults(struct tsl2583_chip *chip)
> > > +{
> > > + /* Operational parameters */
> > > + chip->taos_settings.als_time = 450;
> > > + /* must be a multiple of 50mS */
> > > + chip->taos_settings.als_gain = 2;
> > > + /* this is actually an index into the gain table */
> > > + /* assume clear glass as default */
> > > + chip->taos_settings.als_gain_trim = 1000;
> > > + /* default gain trim to account for aperture effects */
> > > + chip->taos_settings.als_cal_target = 130;
> > > + /* Known external ALS reading used for calibration */
> > > +}
> > > +
> > > +/*
> > > + * Read a number of bytes starting at register (reg) location.
> > > + * Return 0, or i2c_smbus_write_byte ERROR code.
> > > + */
> > Could we use i2c_smbus_read_block_data ?
> NO -Many controllers no longer seem to support block mode read/write.
> To use block mode reads could exclude some.
By controller you mean the i2c controller?

>
> > > +static int
> > > +taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned
> int len)
> > > +{
> > > + int ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/