On Wed, Aug 06, 2014 at 11:11:52PM +0200, Peter Meerwald wrote: > Hello Felipe, > > in subject: light sensor > > please see my minor comments below > > p. > > > TI's opt3001 light sensor is a simple and yet powerful > > little device. The device provides 99% IR rejection, > > Automatic full-scale, very low power consumption and > > measurements from 0.01 to 83k lux. > > > > This patch adds support for that device using the IIO > > framework. > > > > Signed-off-by: Felipe Balbi <balbi@xxxxxx> > > --- > > > > Jonathan, I have a few doubts on how can I test buffered > > mode properly. I can see my IRQs triggering now problem, > > but where is the data pushed ? Can it be read from sysfs > > or /dev somehwere ? > > there should be /dev/iio:deviceX > > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > > index bf05ca5..e4582d7 100644 > > --- a/drivers/iio/light/Kconfig > > +++ b/drivers/iio/light/Kconfig > > @@ -128,6 +128,18 @@ config LTR501 > > This driver can also be built as a module. If so, the module > > will be called ltr501. > > > > +config OPT3001 > > + tristate "Texas Instruments OPT3001 Light Sensor" > > + depends on I2C > > + select IIO_BUFFER > > + select IIO_TRIGGERED_BUFFER > > + help > > + If you say Y or M here, you get support for Texas Instruments > > + OPT3001 Ambient Light Sensor. > > + > > + If built as a dynamically linked module, it will be called > > + opt3001. > > + > > config TCS3414 > > tristate "TAOS TCS3414 digital color sensor" > > depends on I2C > > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile > > index 8b8c09f..898ef13 100644 > > --- a/drivers/iio/light/Makefile > > +++ b/drivers/iio/light/Makefile > > @@ -13,6 +13,7 @@ obj-$(CONFIG_HID_SENSOR_PROX) += hid-sensor-prox.o > > obj-$(CONFIG_ISL29125) += isl29125.o > > obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o > > obj-$(CONFIG_LTR501) += ltr501.o > > +obj-$(CONFIG_OPT3001) += opt3001.o > > obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o > > obj-$(CONFIG_TCS3414) += tcs3414.o > > obj-$(CONFIG_TCS3472) += tcs3472.o > > diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c > > new file mode 100644 > > index 0000000..1d925fd1 > > --- /dev/null > > +++ b/drivers/iio/light/opt3001.c > > @@ -0,0 +1,765 @@ > > +/** > > + * opt3001.c - Texas Instruments OPT3001 Light Sensor > > + * > > + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com > > + * > > + * Author: Felipe Balbi <balbi@xxxxxx> > > + * > > + * This program is free software: you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License version 2 of the License > > + * as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, but WITHOUT > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > > + * more details. > > + */ > > + > > +#include <linux/bitops.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/i2c.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/slab.h> > > +#include <linux/types.h> > > + > > +#include <linux/iio/events.h> > > +#include <linux/iio/iio.h> > > +#include <linux/iio/sysfs.h> > > + > > +#define OPT3001_RESULT 0x00 > > +#define OPT3001_CONFIGURATION 0x01 > > +#define OPT3001_LOW_LIMIT 0x02 > > +#define OPT3001_HIGH_LIMIT 0x03 > > +#define OPT3001_MANUFACTURER_ID 0x7e > > +#define OPT3001_DEVICE_ID 0x7f > > + > > +#define OPT3001_CONFIGURATION_RN_MASK (0xf << 12) > > +#define OPT3001_CONFIGURATION_RN_AUTO (0xc << 12) > > + > > +#define OPT3001_CONFIGURATION_CT (1 << 11) > > could use BIT() from bitops.h sure, easy enough. > > +static int opt3001_read(const struct opt3001 *opt, u8 reg) > > +{ > > + return i2c_smbus_read_word_swapped(opt->client, reg); > > +} > > + > > +static int opt3001_write(const struct opt3001 *opt, u8 reg, u16 value) > > +{ > > + return i2c_smbus_write_word_swapped(opt->client, reg, value); > > +} > > + > > many small helper function; worth it to wrap a one-liner? why not ? I think it looks better and it'll get inlined anyway. It also helps a lot when using kernel function tracer. > > + .scan_type = { > > + .sign = 'u', > > .realbits / .storagebits missing? yeah, couldn't figure this out. The HW gives me a 16-bit value where 12-bits are the mantissa and 4-bits are for the exponent. Should I call realbits 16 and storagebits 32 ? > > > + .endianness = IIO_BE, > > + }, > > + .scan_index = 0, > > + .event_spec = opt3001_event_spec, > > + .num_event_specs = ARRAY_SIZE(opt3001_event_spec), > > + }, > > + IIO_CHAN_SOFT_TIMESTAMP(1), > > +}; > > + > > +static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2) > > +{ > > + int ret; > > + u16 mantissa; > > + u16 reg; > > + u8 exponent; > > + > > + ret = opt3001_read(opt, OPT3001_CONFIGURATION); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > + OPT3001_CONFIGURATION); > > + return ret; > > + } > > + > > + reg = ret; > > + opt3001_set_mode(opt, ®, OPT3001_CONFIGURATION_M_SINGLE); > > + > > + ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > "failed to write register ..." yeah thanks. copy&paste error. > > + OPT3001_CONFIGURATION); > > + return ret; > > + } > > + > > + /* wait for conversion and give it an extra 5ms */ > > + usleep_range(opt->int_time + 5000, opt->int_time + 10000); > > + > > + ret = opt3001_read(opt, OPT3001_CONFIGURATION); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > + OPT3001_CONFIGURATION); > > + return ret; > > + } > > + > > + reg = ret; > > + if (!(reg & OPT3001_CONFIGURATION_CRF)) > > + return -EPIPE; > > + > > + ret = opt3001_read(opt, OPT3001_RESULT); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > + OPT3001_RESULT); > > + return ret; > > + } > > + > > + exponent = OPT3001_REG_EXPONENT(ret); > > + mantissa = OPT3001_REG_MANTISSA(ret); > > + > > + opt3001_to_iio_ret(opt, exponent, mantissa, val, val2); > > + > > + return IIO_VAL_INT_PLUS_MICRO; > > +} > > + > > +static int opt3001_get_scale(struct opt3001 *opt, int *val, int *val2) > > +{ > > + int ret; > > + u8 exponent; > > + > > I guess OPT3001_RESULT is only valid AFTER performing opt3001_get_lux() right. I'm only interested in the exponent part. It'll latch the exponent from last conversion, 0 being default. > > +static int opt3001_read_raw(struct iio_dev *iio, > > + struct iio_chan_spec const *chan, int *val, int *val2, > > + long mask) > > +{ > > + struct opt3001 *opt = iio_priv(iio); > > + int ret = 0; > > initialization not needed I'll drop > > +static int opt3001_write_raw(struct iio_dev *iio, > > + struct iio_chan_spec const *chan, int val, int val2, > > + long mask) > > +{ > > + struct opt3001 *opt = iio_priv(iio); > > + int ret = 0; > > initialization not needed I'll drop > > + > > + if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) > > + return -EBUSY; > > + > > + if (chan->type != IIO_LIGHT) > > + return -EINVAL; > > + > > + mutex_lock(&opt->lock); > > + > > + switch (mask) { > > is there no way to control the scale? yeah, but the HW does auto-scaling which works pretty well. Why would you want to control the scale manually ? > > +static int opt3001_write_event_value(struct iio_dev *iio, > > + const struct iio_chan_spec *chan, enum iio_event_type type, > > + enum iio_event_direction dir, enum iio_event_info info, > > + int val, int val2) > > +{ > > + struct opt3001 *opt = iio_priv(iio); > > + int ret = 0; > > + > > + u16 *thresh_mantissa; > > + u16 mantissa; > > + u16 value; > > + u16 reg; > > + > > + u8 *thresh_exp; > > why extra newlines? why not ? I generally separate the types as it aids readability. > > +static int opt3001_remove(struct i2c_client *client) > > +{ > > + /* nothing to do here */ > > shutdown the device probably? it's always in shutdown unless it's doing a transfer. The only exception being if we enable continuous mode and remove the module. Might be better to check that and shut it down, sure. -- balbi
Attachment:
signature.asc
Description: Digital signature
