Re: [PATCH v7 1/2] Added AMS tsl2591 driver implementation
On Fri, Apr 2, 2021 at 3:45 AM Joe Sandom wrote:
>
> Driver implementation for AMS/TAOS tsl2591 ambient light sensor.
>
> This driver supports configuration via device tree and sysfs.
> Supported channels for raw infrared light intensity,
> raw combined light intensity and illuminance in lux.
> The driver additionally supports iio events on lower and
> upper thresholds.
>
> This is a very-high sensitivity light-to-digital converter that
> transforms light intensity into a digital signal.
> Datasheet: https://ams.com/tsl25911#tab/documents
>
Shouldn't be blank lines in the tag block.
> Signed-off-by: Joe Sandom
...
> +#include
Better to put this in a separate group after linux/* , but before linux/iio*
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
...
> +/* ADC integration time, field value to time in ms*/
Missed space.
> +#define TSL2591_FVAL_TO_ATIME(x) (((x) + 1) * 100)
> +/* ADC integration time, time in ms to field value */
> +#define TSL2591_ATIME_TO_FVAL(x) (((x) / 100) - 1)
Taking comments into consideration I would simply rename
ATIME -> MSEC
...
> +#define TSL2591_CTRL_ALS_INTEGRATION_100MS 0x00
> +#define TSL2591_CTRL_ALS_INTEGRATION_200MS 0x01
> +#define TSL2591_CTRL_ALS_INTEGRATION_300MS 0x02
> +#define TSL2591_CTRL_ALS_INTEGRATION_400MS 0x03
> +#define TSL2591_CTRL_ALS_INTEGRATION_500MS 0x04
> +#define TSL2591_CTRL_ALS_INTEGRATION_600MS 0x05
> +#define TSL2591_CTRL_ALS_LOW_GAIN 0x00
> +#define TSL2591_CTRL_ALS_MED_GAIN 0x10
> +#define TSL2591_CTRL_ALS_HIGH_GAIN 0x20
> +#define TSL2591_CTRL_ALS_MAX_GAIN 0x30
> +#define TSL2591_CTRL_SYS_RESET 0x80
I would do it differently, since they are bit fields, e.g. for GAIN
(0 << 4)
(1 << 4)
(2 << 4)
(3 << 4)
and so on for the rest.
But there are pros and cons for yours and proposed variants, so I
leave it for you and maintainers.
...
> +#define TSL2591_PRST_ALS_INT_CYCLE_MAX TSL2591_PRST_ALS_INT_CYCLE_60
I guess it's a bitfield, so instead of using value from the list, show
explicitly the size in bits of the field
(BIT(x) - 1), or here looks like (BIT(4) - 1)
...
> +/* TSL2591 status register masks */
> +#define TSL2591_STS_ALS_VALID_MASK BIT(0)
> +#define TSL2591_STS_ALS_INT_MASK 0x10
> +#define TSL2591_STS_NPERS_INT_MASK 0x20
> +#define TSL2591_STS_VAL_HIGH_MASK0x01
Why inconsistent ? Use BIT() for all here, or switch to left shifts or
plain numbers.
...
> +/*
> + * LUX calculations;
> + * AGAIN values from Adafruits TSL2591 Arduino library
Adafruit's
> + * https://github.com/adafruit/Adafruit_TSL2591_Library
> + */
...
> +static int tsl2591_set_als_lower_threshold(struct tsl2591_chip *chip,
> + u16 als_lower_threshold);
> +static int tsl2591_set_als_upper_threshold(struct tsl2591_chip *chip,
> + u16 als_upper_threshold);
Why forward declarations? Do you have recursion in use?
Otherwise, rearrange functions to be ordered and drop these.
...
> +static int tsl2591_check_als_valid(struct i2c_client *client)
> +{
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(client,
> + TSL2591_CMD_NOP | TSL2591_STATUS);
One line?
> + if (ret < 0) {
> + dev_err(>dev, "Failed to read register\n");
> + return -EINVAL;
> + }
> + ret = FIELD_GET(TSL2591_STS_ALS_VALID_MASK, ret);
> +
> + return ret;
return FIELD_GET(...);
> +}
...
> + struct tsl2591_chip *chip = iio_priv(indio_dev);
> + struct tsl2591_als_settings *settings = >als_settings;
> + struct i2c_client *client = chip->client;
> +
Extra unneeded blank line.
> + u8 als_data[TSL2591_NUM_DATA_REGISTERS];
> + int counts_per_lux, int_time_fval, gain_multi, lux;
> + u16 als_ch0, als_ch1;
> + int ret;
...
> + struct tsl2591_als_settings als_settings = chip->als_settings;
> + struct i2c_client *client = chip->client;
> +
Ditto.
> + u16 als_upper_threshold;
> + u8 als_lower_l;
> + u8 als_lower_h;
> + int ret;
...
> + struct tsl2591_als_settings als_settings = chip->als_settings;
> + struct i2c_client *client = chip->client;
> +
Ditto.
> + u16 als_lower_threshold;
> + u8 als_upper_l;
> + u8 als_upper_h;
> + int ret;
...
> +static ssize_t tsl2591_in_illuminance_period_available_show(struct device
> *dev,
> + struct
> device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct tsl2591_chip *chip = iio_priv(indio_dev);
> + return snprintf(buf, PAGE_SIZE, "%s\n",
> +
>
Re: [PATCH v7 1/2] Added AMS tsl2591 driver implementation
On Fri, 2 Apr 2021 01:45:46 +0100 Joe Sandom wrote: > Driver implementation for AMS/TAOS tsl2591 ambient light sensor. > > This driver supports configuration via device tree and sysfs. > Supported channels for raw infrared light intensity, > raw combined light intensity and illuminance in lux. > The driver additionally supports iio events on lower and > upper thresholds. > > This is a very-high sensitivity light-to-digital converter that > transforms light intensity into a digital signal. > > Datasheet: https://ams.com/tsl25911#tab/documents > > Signed-off-by: Joe Sandom Hi Joe, One thing I noticed whilst reading this that I'd missed before is we present userspace with the interface for events whether or not we have an IRQ. It would be nice to tidy that up. If nothing else comes up in review, I can do that whilst applying as it's pretty straight forward. I'd like to leave this a little longer on list though to see if anyone else would like to take a last look. Thanks, Jonathan > --- > > Changes in v7; > - Revert back to using plain numbers in register defines instead of BIT and > GENMASK > - Changed from pre-increment of 'i' in for loops to post-increment > - Use iopoll.h - readx_poll_timeout macro for periodic poll on ALS status > flag valid > - Remove the 0xFF masks on u8 types as they are redundant > > drivers/iio/light/Kconfig | 11 + > drivers/iio/light/Makefile |1 + > drivers/iio/light/tsl2591.c | 1224 +++ > 3 files changed, 1236 insertions(+) > create mode 100644 drivers/iio/light/tsl2591.c > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > index 33ad4dd0b5c7..6a69a9a3577a 100644 > --- a/drivers/iio/light/Kconfig > +++ b/drivers/iio/light/Kconfig > @@ -501,6 +501,17 @@ config TSL2583 > Provides support for the TAOS tsl2580, tsl2581 and tsl2583 devices. > Access ALS data via iio, sysfs. > > +config TSL2591 > +tristate "TAOS TSL2591 ambient light sensor" > +depends on I2C > +help > + Select Y here for support of the AMS/TAOS TSL2591 ambient light > sensor, > + featuring channels for combined visible + IR intensity and lux > illuminance. > + Access data via iio and sysfs. Supports iio_events. > + > + To compile this driver as a module, select M: the > + module will be called tsl2591. > + > config TSL2772 > tristate "TAOS TSL/TMD2x71 and TSL/TMD2x72 Family of light and > proximity sensors" > depends on I2C > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile > index ea376deaca54..d10912faf964 100644 > --- a/drivers/iio/light/Makefile > +++ b/drivers/iio/light/Makefile > @@ -48,6 +48,7 @@ obj-$(CONFIG_ST_UVIS25_SPI) += st_uvis25_spi.o > obj-$(CONFIG_TCS3414)+= tcs3414.o > obj-$(CONFIG_TCS3472)+= tcs3472.o > obj-$(CONFIG_TSL2583)+= tsl2583.o > +obj-$(CONFIG_TSL2591)+= tsl2591.o > obj-$(CONFIG_TSL2772)+= tsl2772.o > obj-$(CONFIG_TSL4531)+= tsl4531.o > obj-$(CONFIG_US5182D)+= us5182d.o > diff --git a/drivers/iio/light/tsl2591.c b/drivers/iio/light/tsl2591.c > new file mode 100644 > index ..14a405a96e3a > --- /dev/null > +++ b/drivers/iio/light/tsl2591.c > @@ -0,0 +1,1224 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2020 Joe Sandom > + * > + * Datasheet: https://ams.com/tsl25911#tab/documents > + * > + * Device driver for the TAOS TSL2591. This is a very-high sensitivity > + * light-to-digital converter that transforms light intensity into a digital > + * signal. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +/* ADC integration time, field value to time in ms*/ > +#define TSL2591_FVAL_TO_ATIME(x) (((x) + 1) * 100) > +/* ADC integration time, time in ms to field value */ > +#define TSL2591_ATIME_TO_FVAL(x) (((x) / 100) - 1) > + > +/* TSL2591 register set */ > +#define TSL2591_ENABLE 0x00 > +#define TSL2591_CONTROL 0x01 > +#define TSL2591_AILTL 0x04 > +#define TSL2591_AILTH 0x05 > +#define TSL2591_AIHTL 0x06 > +#define TSL2591_AIHTH 0x07 > +#define TSL2591_NP_AILTL0x08 > +#define TSL2591_NP_AILTH0x09 > +#define TSL2591_NP_AIHTL0x0A > +#define TSL2591_NP_AIHTH0x0B > +#define TSL2591_PERSIST 0x0C > +#define TSL2591_PACKAGE_ID 0x11 > +#define TSL2591_DEVICE_ID 0x12 > +#define TSL2591_STATUS 0x13 > +#define TSL2591_C0_DATAL0x14 > +#define TSL2591_C0_DATAH0x15 > +#define TSL2591_C1_DATAL0x16 > +#define TSL2591_C1_DATAH0x17 > + > +/* TSL2591 command register definitions */ > +#define TSL2591_CMD_NOP 0xA0 > +#define TSL2591_CMD_SF_INTSET 0xE4 > +#define TSL2591_CMD_SF_CALS_I 0xE5 > +#define
[PATCH v7 1/2] Added AMS tsl2591 driver implementation
Driver implementation for AMS/TAOS tsl2591 ambient light sensor. This driver supports configuration via device tree and sysfs. Supported channels for raw infrared light intensity, raw combined light intensity and illuminance in lux. The driver additionally supports iio events on lower and upper thresholds. This is a very-high sensitivity light-to-digital converter that transforms light intensity into a digital signal. Datasheet: https://ams.com/tsl25911#tab/documents Signed-off-by: Joe Sandom --- Changes in v7; - Revert back to using plain numbers in register defines instead of BIT and GENMASK - Changed from pre-increment of 'i' in for loops to post-increment - Use iopoll.h - readx_poll_timeout macro for periodic poll on ALS status flag valid - Remove the 0xFF masks on u8 types as they are redundant drivers/iio/light/Kconfig | 11 + drivers/iio/light/Makefile |1 + drivers/iio/light/tsl2591.c | 1224 +++ 3 files changed, 1236 insertions(+) create mode 100644 drivers/iio/light/tsl2591.c diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 33ad4dd0b5c7..6a69a9a3577a 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -501,6 +501,17 @@ config TSL2583 Provides support for the TAOS tsl2580, tsl2581 and tsl2583 devices. Access ALS data via iio, sysfs. +config TSL2591 +tristate "TAOS TSL2591 ambient light sensor" +depends on I2C +help + Select Y here for support of the AMS/TAOS TSL2591 ambient light sensor, + featuring channels for combined visible + IR intensity and lux illuminance. + Access data via iio and sysfs. Supports iio_events. + + To compile this driver as a module, select M: the + module will be called tsl2591. + config TSL2772 tristate "TAOS TSL/TMD2x71 and TSL/TMD2x72 Family of light and proximity sensors" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index ea376deaca54..d10912faf964 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -48,6 +48,7 @@ obj-$(CONFIG_ST_UVIS25_SPI) += st_uvis25_spi.o obj-$(CONFIG_TCS3414) += tcs3414.o obj-$(CONFIG_TCS3472) += tcs3472.o obj-$(CONFIG_TSL2583) += tsl2583.o +obj-$(CONFIG_TSL2591) += tsl2591.o obj-$(CONFIG_TSL2772) += tsl2772.o obj-$(CONFIG_TSL4531) += tsl4531.o obj-$(CONFIG_US5182D) += us5182d.o diff --git a/drivers/iio/light/tsl2591.c b/drivers/iio/light/tsl2591.c new file mode 100644 index ..14a405a96e3a --- /dev/null +++ b/drivers/iio/light/tsl2591.c @@ -0,0 +1,1224 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 Joe Sandom + * + * Datasheet: https://ams.com/tsl25911#tab/documents + * + * Device driver for the TAOS TSL2591. This is a very-high sensitivity + * light-to-digital converter that transforms light intensity into a digital + * signal. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +/* ADC integration time, field value to time in ms*/ +#define TSL2591_FVAL_TO_ATIME(x) (((x) + 1) * 100) +/* ADC integration time, time in ms to field value */ +#define TSL2591_ATIME_TO_FVAL(x) (((x) / 100) - 1) + +/* TSL2591 register set */ +#define TSL2591_ENABLE 0x00 +#define TSL2591_CONTROL 0x01 +#define TSL2591_AILTL 0x04 +#define TSL2591_AILTH 0x05 +#define TSL2591_AIHTL 0x06 +#define TSL2591_AIHTH 0x07 +#define TSL2591_NP_AILTL0x08 +#define TSL2591_NP_AILTH0x09 +#define TSL2591_NP_AIHTL0x0A +#define TSL2591_NP_AIHTH0x0B +#define TSL2591_PERSIST 0x0C +#define TSL2591_PACKAGE_ID 0x11 +#define TSL2591_DEVICE_ID 0x12 +#define TSL2591_STATUS 0x13 +#define TSL2591_C0_DATAL0x14 +#define TSL2591_C0_DATAH0x15 +#define TSL2591_C1_DATAL0x16 +#define TSL2591_C1_DATAH0x17 + +/* TSL2591 command register definitions */ +#define TSL2591_CMD_NOP 0xA0 +#define TSL2591_CMD_SF_INTSET 0xE4 +#define TSL2591_CMD_SF_CALS_I 0xE5 +#define TSL2591_CMD_SF_CALS_NPI 0xE7 +#define TSL2591_CMD_SF_CNP_ALSI 0xEA + +/* TSL2591 enable register definitions */ +#define TSL2591_PWR_ON 0x01 +#define TSL2591_PWR_OFF 0x00 +#define TSL2591_ENABLE_ALS 0x02 +#define TSL2591_ENABLE_ALS_INT 0x10 +#define TSL2591_ENABLE_SLEEP_INT0x40 +#define TSL2591_ENABLE_NP_INT 0x80 + +/* TSL2591 control register definitions */ +#define TSL2591_CTRL_ALS_INTEGRATION_100MS 0x00 +#define TSL2591_CTRL_ALS_INTEGRATION_200MS 0x01 +#define TSL2591_CTRL_ALS_INTEGRATION_300MS 0x02 +#define TSL2591_CTRL_ALS_INTEGRATION_400MS 0x03 +#define TSL2591_CTRL_ALS_INTEGRATION_500MS 0x04 +#define TSL2591_CTRL_ALS_INTEGRATION_600MS 0x05 +#define TSL2591_CTRL_ALS_LOW_GAIN 0x00 +#define
