On 09/08/13 16:20, Peter Meerwald wrote: > chip has four 16-bit channels for red, green, blue, clear color > intensity; driver supports the TCS3x7x family of devices and was > tested with a TCS34725 chip; further information here: > http://www.ams.com/eng/Products/Light-Sensors/Color-Sensor/TCS34725 > > Signed-off-by: Peter Meerwald <pmeerw@xxxxxxxxxx> > Cc: Jon Brenner <jon.brenner@xxxxxxx> A few trivial bits: I2C does not have any alignment constraints on the buffers used (these are handled in the i2c driver by using a bounce buffer if needed). That means you can just embed you data->buffer as an array directly inside data. Saves an allocation and 2 lines of code, hence whilst nicer it can be in a follow up patch if you like. For long writes to sysfs buffers make sure you use snprintf. It's entirely possible your write will always fit (and indeed it should) but as a reviewer I'm happier letting it go with the added protection that it won't overrun the buffer. Anyhow, must be all of 2 mins work for you to roll a new version of with these trivial bits cleaned up. (I very nearly just made the changes and merged, but they are just a tiny bit too invasive given I can't test!) Another nice driver. Thanks Peter. Jonathan ... > +struct tcs3472_data { > + struct i2c_client *client; > + u8 enable; > + u8 control; > + u8 atime; As mentioned below, u16 buffer[8]; > + u16 *buffer; > +}; > + ... > +static int tcs3472_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct tcs3472_data *data = iio_priv(indio_dev); > + int i; > + > + switch (mask) { > + case IIO_CHAN_INFO_CALIBSCALE: > + for (i = 0; i < ARRAY_SIZE(tcs3472_agains); i++) { Do you want to check val2 here as well just to confirm it is 0? You could otherwise implement the write_raw_get_fmt callback to specify that this is IIO_VAL_INT to let the front end handle any unwanted decimals. > + if (val == tcs3472_agains[i]) { > + data->control &= ~TCS3472_CONTROL_AGAIN_MASK; > + data->control |= i; > + return i2c_smbus_write_byte_data( > + data->client, TCS3472_CONTROL, > + data->control); > + } > + } > + return -EINVAL; > + case IIO_CHAN_INFO_INT_TIME: > + if (val != 0) > + return -EINVAL; > + for (i = 0; i < 256; i++) { > + if (val2 == (256 - i) * 2400) { > + data->atime = i; > + return i2c_smbus_write_word_data( > + data->client, TCS3472_ATIME, > + data->atime); > + } > + > + } > + return -EINVAL; > + } > + return -EINVAL; > +} ... > +static ssize_t tcs3472_show_int_time_available(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + ssize_t total_n = 0; > + int i; > + > + for (i = 1; i <= 256; i++) { > + ssize_t n = sprintf(buf, "0.%06d ", 2400 * i); Ouch, that is quite a long text string. Handling this sort of available case nicely has been on the todo list for ages. I've been meaning to at least get proposed docs on how to do this out there and it might happen one day soon! For this I'd propose something like 2400..2400..614400 to indicate that it is even steps size 2400 between 2400 and 614400. Right now what you have is all the ABI allows unfortunately. Snprintf limited to a page would certainly be wise here. > + buf += n; > + total_n += n; > + } > + /* replace trailing space by newline */ > + buf[-1] = '\n'; > + > + return total_n; > +} > + > +static IIO_CONST_ATTR(calibscale_available, "1 4 16 60"); > +static IIO_DEV_ATTR_INT_TIME_AVAIL(tcs3472_show_int_time_available); > + > +static struct attribute *tcs3472_attributes[] = { > + &iio_const_attr_calibscale_available.dev_attr.attr, > + &iio_dev_attr_integration_time_available.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group tcs3472_attribute_group = { > + .attrs = tcs3472_attributes, > +}; > + > +static const struct iio_info tcs3472_info = { > + .read_raw = tcs3472_read_raw, > + .write_raw = tcs3472_write_raw, > + .attrs = &tcs3472_attribute_group, > + .driver_module = THIS_MODULE, > +}; > + > +static int tcs3472_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct tcs3472_data *data; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > + if (indio_dev == NULL) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + i2c_set_clientdata(client, indio_dev); > + data->client = client; I2C has no alignment requirements (and even if there were there are ways of ensuring all is fine - see spi drivers). Hence just put the buffer array directly inside data and lose this separate allocation. > + data->buffer = devm_kzalloc(&indio_dev->dev, TSL3472_BUFFER_SIZE, > + GFP_KERNEL); > + if (data->buffer == NULL) > + return -ENOMEM; ... -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
