Hi, On Thu, Aug 07, 2014 at 11:01:04AM +0100, Jonathan Cameron wrote: > On 06/08/14 17:10, Felipe Balbi wrote: > > 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> > I don't suppose there is a datasheet available? There is a summary datasheet available. The device isn't released yet: http://www.ti.com/product/OPT3001?keyMatch=opt3001&tisearch=Search-EN > When in M_CONTINUOUS is it just providing event interrupts > (threshold etc)? It depends on the mode. If Latch bit is set, then it'll report event interrupts. Datasheet calls this a "window-style comparison operation). If Latch bit is cleared, then it'll continuously report the comparison result. Datasheet calls this a "hysteresis-style comoparison operation). > > 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 ? > As I guess has become clear from your thread with Peter, > what you have here in IIO terms are called events (separate > form the buffer which is for the main data flow). > > Anyhow, see drivers/staging/iio/Documentation/iio_event_monitor.c. > Some trickery with an anonymous file descriptor is used to avoid > using lots of chardevs for the same device. ok, will look at it. > > Also, there are a couple details about this device which > > I need to know if it looks for you: > > > > This device has a single enable bit which will enable both > > rising and falling edge triggers, but the limits are separate. > > > > The same limits are also used for hysteresis-style capture and > > that's controlled by a single bit flip. > With this on, it generates an event whenever the value changes by > more than a certain amount? If so this is better handled by > providing a trigger and a buffer. Note that the events path > carries no data other than an event code. It's still a bit weird to me. So, without Latch bit, then yeah, it'll be more of a trigger e.g: I can set low limit to 200 lux and high limit to 50 lux, then I'll get a rising edge IRQ when I have more than 50 lux and a falling edge when I get less than 200 lux. If Latch bit is set, however, then it's almost like it ignores low/high limit altogether and I get an IRQ ever $int_time ms or so. > > How do you want this to look on sysfs ? Currently, enable/disabling > > any of rising/falling edges, will disable both. > That's pretty common. The ABI basically allows for any setting to > change any other (as there are much weirder connections than this > in some devices). ok, so it's alright to have two "enable" files and have they refer to the same bit ? > > +static int opt3001_read(const struct opt3001 *opt, u8 reg) > > +{ > > + return i2c_smbus_read_word_swapped(opt->client, reg); > > +} > I'm very much against wrapper functions unless they add something. These > really do not. as I mentioned before, it helps a lot when using function tracer. I can just "echo opt3001* > set_trace_function". But if you guys are so much against it, I'll remove it. > > +static const struct iio_chan_spec opt3001_channels[] = { > > + { > > + .type = IIO_LIGHT, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE) | > > + BIT(IIO_CHAN_INFO_INT_TIME), > > + .channel = 0, > > No buffering at the moment so can drop scan_type and scan_index. will do > > +static int opt3001_read_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 = IIO_VAL_INT_PLUS_MICRO; > > + > > + mutex_lock(&opt->lock); > > + > So these will return the same value whether the value attribute is read > or the hysteresis one? yeah, the comparison done is the same. > > +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; > > + > > + u8 exponent; > > + > > + mutex_lock(&opt->lock); > > + > > + ret = opt3001_read(opt, OPT3001_CONFIGURATION); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > + OPT3001_CONFIGURATION); > > + goto err; > > + } > > + > > + reg = ret; > > + if (info == IIO_EV_INFO_HYSTERESIS) > Without a datasheet I'm not entirely sure how the hysteresis is > applied here and what it's connection to the thresholds is... yeah, that'll still take a while to be fully released. > > > + opt->hysteresis = true; > > + else > > + opt->hysteresis = false; > > + > > + switch (dir) { > > + case IIO_EV_DIR_RISING: > > + reg = OPT3001_HIGH_LIMIT; > Why bother with the indirection via a pointer. Just have a second > switch statement at the end of the function that sets the right ones. > Easier to follow than this. sure > > + thresh_mantissa = &opt->high_thresh_mantissa; > > + thresh_exp = &opt->high_thresh_exp; > > + break; > > + case IIO_EV_DIR_FALLING: > > + reg = OPT3001_LOW_LIMIT; > > + thresh_mantissa = &opt->low_thresh_mantissa; > > + thresh_exp = &opt->low_thresh_exp; > > + break; > > + default: > > + ret = -EINVAL; > > + goto err; > > + } > > + > > + ret = opt3001_find_scale(opt, val, val2, &exponent); > > + if (ret < 0) { > > + dev_err(opt->dev, "can't find scale for %d.%d\n", val, val2); > > + goto err; > > + } > > + > > + mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent; > > + value = exponent << 12 | mantissa; > > + ret = opt3001_write(opt, reg, value); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", reg); > > + goto err; > > + } > > + > As stated above an additional switch statement here with direct assignment > of the relevant threshold values would be clearer. > > + *thresh_mantissa = mantissa; > > + *thresh_exp = exponent; > > + > > +err: > > + mutex_unlock(&opt->lock); > > + > > + return ret; > > +} > > + > > +static int opt3001_read_event_config(struct iio_dev *iio, > > + const struct iio_chan_spec *chan, enum iio_event_type type, > > + enum iio_event_direction dir) > > +{ > > + struct opt3001 *opt = iio_priv(iio); > > + > > + return opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS; > > +} > > + > > +static int opt3001_write_event_config(struct iio_dev *iio, > > + const struct iio_chan_spec *chan, enum iio_event_type type, > > + enum iio_event_direction dir, int state) > > +{ > > + struct opt3001 *opt = iio_priv(iio); > > + int ret; > > + u16 mode; > > + u16 reg; > > + > > + if (state && opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) > > + return 0; > > + > > + if (!state && opt->mode == OPT3001_CONFIGURATION_M_SHUTDOWN) > > + return 0; > > + > > + mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS > > + : OPT3001_CONFIGURATION_M_SHUTDOWN; > > + > > + 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, ®, mode); > > + > > + if (opt->hysteresis) > > + reg |= OPT3001_CONFIGURATION_L; > > + else > > + reg &= ~OPT3001_CONFIGURATION_L; > > + > > + ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > + OPT3001_CONFIGURATION); > > + return ret; > > + } > > + > > + /* wait for mode change to go through */ > > + usleep_range(opt->int_time + 5000, opt->int_time + 10000); > > + > > + return 0; > > +} > > + > > +static const struct iio_info opt3001_info = { > > + .driver_module = THIS_MODULE, > > + .attrs = &opt3001_attribute_group, > > + .read_raw = opt3001_read_raw, > > + .write_raw = opt3001_write_raw, > > + .read_event_value = opt3001_read_event_value, > > + .write_event_value = opt3001_write_event_value, > > + .read_event_config = opt3001_read_event_config, > > + .write_event_config = opt3001_write_event_config, > > +}; > > + > > +static int opt3001_read_id(struct opt3001 *opt) > > +{ > > + char manufacturer[2]; > > + u16 device_id; > > + int ret; > > + > > + ret = opt3001_read(opt, OPT3001_MANUFACTURER_ID); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > + OPT3001_MANUFACTURER_ID); > > + return ret; > > + } > > + > > + manufacturer[0] = ret >> 8; > > + manufacturer[1] = ret & 0xff; > > + > > + ret = opt3001_read(opt, OPT3001_DEVICE_ID); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > + OPT3001_DEVICE_ID); > > + return ret; > > + } > > + > > + device_id = ret; > > + > > + dev_info(opt->dev, "Found %c%c OPT%04x\n", manufacturer[0], > > + manufacturer[1], device_id); > > + > > + return 0; > > +} > > + > > +static int opt3001_configure(struct opt3001 *opt) > > +{ > > + int ret; > > + u16 reg; > > + > > + 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_CT) > > + opt->int_time = 800000; > > + else > > + opt->int_time = 100000; > > + > > + reg &= ~OPT3001_CONFIGURATION_L; > > + reg &= ~OPT3001_CONFIGURATION_RN_MASK; > > + reg |= OPT3001_CONFIGURATION_RN_AUTO; > > + > > + ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to write register %02x\n", > > + OPT3001_CONFIGURATION); > > + return ret; > > + } > > + > > + ret = opt3001_read(opt, OPT3001_LOW_LIMIT); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > + OPT3001_LOW_LIMIT); > > + return ret; > > + } > > + > > + opt->low_thresh_mantissa = OPT3001_REG_MANTISSA(ret); > > + opt->low_thresh_exp = OPT3001_REG_EXPONENT(ret); > > + > > + ret = opt3001_read(opt, OPT3001_HIGH_LIMIT); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > + OPT3001_HIGH_LIMIT); > > + return ret; > > + } > > + > > + opt->high_thresh_mantissa = OPT3001_REG_MANTISSA(ret); > > + opt->high_thresh_exp = OPT3001_REG_EXPONENT(ret); > > + > > + return 0; > > +} > > + > > +static irqreturn_t opt3001_irq(int irq, void *_opt) > > +{ > > Pass the iio_dev in to the irq setup instead of opt and > you will then not need the opt reference to the struct iio_dev. What's the difference ? When used this way we have: struct opt3001 *opt = _opt; struct iio_dev *iio = opt->dev; Done your way we will have: struct iio_dev *iio = _iio; struct opt3001 *opt = iio_priv(iio); I don't see the benefit of changing this as I'll access to both opt and iio. > > +static int opt3001_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct device *dev = &client->dev; > > + > > + struct iio_dev *iio; > > + struct opt3001 *opt; > > + int irq = client->irq; > > + int ret; > > + > > + iio = devm_iio_device_alloc(dev, sizeof(*opt)); > > + if (!iio) > > + return -ENOMEM; > > + > > + opt = iio_priv(iio); > > + opt->client = client; > > + opt->dev = dev; > > This is very circular and it rarely makes sense to have > the device private structure have a reference to the > iio_dev. Here it is only used in the interrupt handler. > Avoid that by making the private data passed to the interrupt > handler the struct iio_dev instead of the struct opt3001. > > > + opt->iio = iio; > > + > > + mutex_init(&opt->lock); > > + i2c_set_clientdata(client, opt); > > + > > + ret = opt3001_read_id(opt); > > + if (ret) > > + return ret; > > + > > + ret = opt3001_configure(opt); > > + if (ret) > > + return ret; > > + > > + iio->name = client->name; > > + iio->channels = opt3001_channels; > > + iio->num_channels = ARRAY_SIZE(opt3001_channels); > > + iio->dev.parent = dev; > > + iio->modes = INDIO_DIRECT_MODE; > > + iio->info = &opt3001_info; > > + > > + ret = devm_iio_device_register(dev, iio); > > + if (ret) { > > + dev_err(dev, "failed to register IIO device\n"); > > + return ret; > > + } > > + > Normally we'd expect the devm_iio_device_register (that provides the > userspace interfaces) to be after the irq request. that's wrong. By the time the IRQ is requested, an IRQ can actually fire and you better have a valid e.g. iio_dev by then. -- balbi
Attachment:
signature.asc
Description: Digital signature
