Thank you for review! But I don't completely understand one of your comment:
>> +static int als_probe(struct i2c_client *client, const struct i2c_device_id *id)
[...]
>> + if (client->irq) {
>> + ret = devm_request_threaded_irq(&client->dev, client->irq,
>> + NULL, als_interrupt_handler,
>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> + ALS_IRQ_NAME, indio_dev);
>
> This is a bit racy, you access memory in the irq handler that is freed
> before the irq is freed.
Do you mean than that indio_dev may be used in interrupt handler after
iio_device_free(indio_dev) called in als_remove() function?
If so, can I use disable_irq() in als_remove() before iio_device_free()
to avoid this problem?
On Fri, Jul 12, 2013 at 8:04 PM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> On 07/10/2013 03:08 PM, Oleksandr Kravchenko wrote:
>> From: Oleksandr Kravchenko <o.v.kravchenko@xxxxxxxxxxxxxxx>
>>
>> This patch adds IIO driver for APDS9300 ambilent light sensor (ALS).
>
> s/ambilent/ambient/
>
>> http://www.avagotech.com/docs/AV02-1077EN
>>
>> The driver allows to read raw data from ADC registers or calculate
>> lux value. It also can handle threshold inrerrupt.
>
> s/inrerrupt/interrupt/
>
> The patch looks very good in general, a couple of comment inline.
>
>>
>> Signed-off-by: Oleksandr Kravchenko <o.v.kravchenko@xxxxxxxxxxxxxxx>
>> ---
>> .../devicetree/bindings/iio/light/apds9300.txt | 22 +
>> drivers/iio/light/Kconfig | 10 +
>> drivers/iio/light/Makefile | 1 +
>> drivers/iio/light/apds9300.c | 528 ++++++++++++++++++++
>> 4 files changed, 561 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/light/apds9300.txt
>> create mode 100644 drivers/iio/light/apds9300.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/light/apds9300.txt b/Documentation/devicetree/bindings/iio/light/apds9300.txt
>> new file mode 100644
>> index 0000000..d6f66c7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/light/apds9300.txt
>> @@ -0,0 +1,22 @@
>> +* Avago APDS9300 ambient light sensor
>> +
>> +http://www.avagotech.com/docs/AV02-1077EN
>> +
>> +Required properties:
>> +
>> + - compatible : should be "avago,apds9300"
>
> You should also add the avago vendor prefix to
> Documentation/devicetree/bindings/vendor-prefixes.txt. Preferably in a
> separate patch.
>
>> + - reg : the I2C address of the sensor
>> +
>> +Optional properties:
>> +
>> + - interrupt-parent : should be the phandle for the interrupt controller
>> + - interrupts : interrupt mapping for GPIO IRQ
>> +
>> +Example:
>> +
>> +apds9300@39 {
>> + compatible = "avago,apds9300";
>> + reg = <0x39>;
>> + interrupt-parent = <&gpio2>;
>> + interrupts = <29 8>;
>> +};
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index 5ef1a39..08a6742 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -52,6 +52,16 @@ config VCNL4000
>> To compile this driver as a module, choose M here: the
>> module will be called vcnl4000.
>>
>> +config APDS9300
>> + tristate "APDS9300 ambient light sensor"
>> + depends on I2C
>> + help
>> + Say Y here if you want to build a driver for the Avago APDS9300
>> + ambient light sensor.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called apds9300.
>> +
>
> Keeps this in alphabetical order
>
>> config HID_SENSOR_ALS
>> depends on HID_SENSOR_HUB
>> select IIO_BUFFER
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index 040d9c7..da58e12 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -6,4 +6,5 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
>> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
>> obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
>> obj-$(CONFIG_VCNL4000) += vcnl4000.o
>> +obj-$(CONFIG_APDS9300) += apds9300.o
>
> Same here
>
>> obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
>> diff --git a/drivers/iio/light/apds9300.c b/drivers/iio/light/apds9300.c
>> new file mode 100644
>> index 0000000..2275ecc
>> --- /dev/null
>> +++ b/drivers/iio/light/apds9300.c
>> @@ -0,0 +1,528 @@
>> +/*
>> + * apds9300.c - IIO driver for Avago APDS9300 ambient light sensor
>> + *
>> + * Copyright 2013 Oleksandr Kravchenko <o.v.kravchenko@xxxxxxxxxxxxxxx>
>> + *
>> + * This file is subject to the terms and conditions of version 2 of
>> + * the GNU General Public License. See the file COPYING in the main
>> + * directory of this archive for more details.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/pm.h>
>> +#include <linux/i2c.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>
> No device driver should ever need to include irq.h
>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/events.h>
>> +
> [...]
>> +
>> +static unsigned long als_calculate_lux(u16 ch0, u16 ch1)
>> +{
>> + unsigned long lux, tmp;
>> + u64 tmp64;
>> +
>> + /* avoid division by zero */
>> + if (ch0 == 0)
>> + return 0;
>> +
>> + tmp = ch1 * 100 / ch0;
>> + if (tmp <= 52) {
>> + /*
>> + * Variable tmp64 need to avoid overflow of this part of lux
>> + * calculation formula.
>> + */
>
> If you want to avoid the overflow you have to do the math as 64bit math. As
> it is right now it will do 32bit math and only store the result in a 64 bit
> variable.
>
>> + tmp64 = ch0 * lux_ratio[tmp] * 5930 / 1000;
>> + lux = 3150 * ch0 - (unsigned long)tmp64;
>> + }
>> + else if (tmp <= 65)
>> + lux = 2290 * ch0 - 2910 * ch1;
>> + else if (tmp <= 80)
>> + lux = 1570 * ch0 - 1800 * ch1;
>> + else if (tmp <= 130)
>> + lux = 338 * ch0 - 260 * ch1;
>> + else
>> + lux = 0;
>> +
>> + return lux / 100000;
>> +}
>> +
> [...]
>> +static int als_get_adc_val(struct als_data *data, int adc_number)
>> +{
>> + int ret;
>> + u8 flags = ALS_CMD | ALS_WORD;
>> +
>> + if (!data->power_state)
>> + return -EAGAIN;
>
> EAGAIN is probably not the right error code, maybe EBUSY or ENODEV.
>
>> +
>> + /* Select ADC0 or ADC1 data register */
>> + flags |= adc_number ? ALS_DATA1LOW : ALS_DATA0LOW;
>> +
>> + ret = i2c_smbus_read_word_data(data->client, flags);
>> + if (ret < 0)
>> + dev_err(&data->client->dev,
>> + "failed to read ADC%d value\n", adc_number);
>> +
>> + return ret;
>> +}
>> +
> [...]
>
>> +static irqreturn_t als_interrupt_handler(int irq, void *private)
>> +{
>> + struct iio_dev *dev_info = private;
>> + struct als_data *data = iio_priv(dev_info);
>> +
>> + iio_push_event(dev_info,
>> + IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, 0,
>> + IIO_EV_TYPE_THRESH,
>> + IIO_EV_DIR_FALLING),
>> + iio_get_time_ns());
>
> In the event mask you specify support for both falling and rising threshold
> events, yet the only event ever triggered is a falling event.
>
>> +
>> + als_clear_intr(data);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +/* Probe/remove functions */
>
> I don't think we need the comment to know that als_probe is the probe
> function ;)
>
>> +
>> +static int als_probe(struct i2c_client *client, const struct i2c_device_id *id)
>> +{
>> + struct als_data *data;
>> + struct iio_dev *indio_dev;
>> + int ret;
>> +
>> + indio_dev = iio_device_alloc(sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(indio_dev);
>> + i2c_set_clientdata(client, indio_dev);
>> + data->client = client;
>> +
>> + ret = als_chip_init(data);
>> + if (ret < 0)
>> + goto err;
>> +
>> + mutex_init(&data->mutex);
>> +
>> + indio_dev->dev.parent = &client->dev;
>> + indio_dev->channels = als_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(als_channels);
>> + indio_dev->name = ALS_DRV_NAME;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + if (client->irq)
>> + indio_dev->info = &als_info;
>> + else
>> + indio_dev->info = &als_info_no_irq;
>> +
>> + if (client->irq) {
>> + ret = devm_request_threaded_irq(&client->dev, client->irq,
>> + NULL, als_interrupt_handler,
>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> + ALS_IRQ_NAME, indio_dev);
>
> This is a bit racy, you access memory in the irq handler that is freed
> before the irq is freed.
>
>> + if (ret) {
>> + dev_err(&client->dev, "irq request error %d\n", -ret);
>> + goto err;
>> + }
>> + }
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret < 0)
>> + goto err;
>> +
>> + dev_info(&client->dev, "ambient light sensor\n");
>
> This line is just noise in the bootlog, please remove it.
>
>> +
>> + return 0;
>> +
>> +err:
>> + /* Ensure that power off in case of error */
>> + als_set_power_state(data, 0);
>> + iio_device_free(indio_dev);
>> + return ret;
>> +}
>> +
>> +static int als_remove(struct i2c_client *client)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> + struct als_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + iio_device_unregister(indio_dev);
>> +
>> + /* Ensure that power off and interrupts are disabled */
>> + ret = als_set_intr_state(data, 0);
>> + if (!ret)
>> + ret = als_set_power_state(data, 0);
>> +
>> + iio_device_free(indio_dev);
>> +
>> + return ret;
>
> The remove callback must always return 0.
>
>> +}
> [...]
>
--
Oleksandr Kravchenko
GlobalLogic
P +380633213187
P +380994930248
www.globallogic.com
http://www.globallogic.com/email_disclaimer.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
