Re: [PATCH v8 1/2] Added AMS tsl2591 driver implementation
On Sun, Apr 18, 2021 at 10:08:30AM +0100, Jonathan Cameron wrote:
> On Fri, 16 Apr 2021 18:49:01 +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,
>
> I was in two minds about whether to just apply this and roll in the below
> suggestions + those Andy made or whether to ask you to do a v9.
>
> The naming and units things below are complex enough that I'm not 100% sure
> I'd get the right so please take a look and clean up those last few
> corners!
>
> Thanks,
>
> Jonathan
>
Thanks for the comments Jonathan. All good points! happy to clean up the
last few bits in v9. I'll aim to get this out over the next few days.
Thanks,
Joe
>
> > ---
> > Changes in v8;
> > - tsl2591_write_raw() - goto after tsl2591_set_als_gain_int_time() not
> > necessary
> > - tsl2591_read_event_value() and tsl2591_write_event_value() - break
> > instead of goto in default case
> > - Introduced tsl2591_info_no_irq iio_info structure which is used when the
> > interrupt is disabled.
> > This variant doesn't expose anything that can't be used when the
> > interrupt is disabled
> > - Corrected ordering of includes for
> > - Renamed TSL2591_FVAL_TO_ATIME to TSL2591_MSEC_TO_ATIME
> > - Renamed TSL2591_ATIME_TO_FVAL to TSL2591_ATIME_TO_MSEC
> > - Changed TSL2591_STS_ALS* defines to all use BIT for consistency
> > - Use (BIT(4) - 1) instead of value from list for
> > TSL2591_PRST_ALS_INT_CYCLE_MAX
> > - Cleaned up a few blank lines that were unneccesary
> > - Use sysfs_emit() instead of snprintf() in
> > tsl2591_in_illuminance_period_available_show()
> > - Use err_unlock branch instead of err to be clear on mutex_unlock cases
> > - Tidied up use of goto err_unlock in tsl2591_read_raw() to be consistent
> > with other functions
> >
> > NOTE;
> > - tsl2591_set_als_lower_threshold() and tsl2591_set_als_upper_threshold()
> > forward declaration needed
> > because tsl2591_set_als_lower_threshold() calls
> > tsl2591_set_als_upper_threshold() and vice versa
> >
> > drivers/iio/light/Kconfig | 11 +
> > drivers/iio/light/Makefile |1 +
> > drivers/iio/light/tsl2591.c | 1227 +++
> > 3 files changed, 1239 insertions(+)
> > create mode 100644 drivers/iio/light/tsl2591.c
> >
>
> ...
>
> > 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 ..4f9c5e19ee35
> > --- /dev/null
> > +++ b/drivers/iio/light/tsl2591.c
> > @@ -0,0 +1,1227 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2020 Joe Sandom
>
> Update perhaps?
>
> > + *
> > + * 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
> > +#include
> > +
> > +/* ADC integration time, field value to time in ms */
> > +#define TSL2591_MSEC_TO_ATIME(x) (((x) + 1) * 100)
>
> Naming here seems backwards. I'd assume that at MSEC_TO_ATIME
> function went msecs -> atime but it seems to be doing the opposite.
>
> > +/* ADC integration time, time in ms to field value */
> > +#define TSL2591_ATIME_TO_MSEC(x) (((x) / 100) - 1)
> > +
>
> ...
>
> > +
> > +/*
> > + * Period table is ALS persist cycle x integration time setting
> > + * Integration times: 100ms, 200ms, 300ms, 400ms, 500ms, 600ms
> > + * ALS cycles: 1, 2, 3, 5, 10, 20, 25, 30, 35, 40, 45, 50, 55, 60
> > + */
> > +static const char * const tsl2591_als_period_list[] = {
> > + "0.1 0.2 0.3 0.5 1.0 2.0 2.5
Re: [PATCH v8 1/2] Added AMS tsl2591 driver implementation
On Sat, Apr 17, 2021 at 03:50:16PM +0300, Andy Shevchenko wrote: > On Fri, Apr 16, 2021 at 8:49 PM 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. > > Hmm... It's v8 and the subject line is wrongly formatted. > Please add the corresponding prefix "iio: light: ..." > Thanks for pointing that out Andy. I'll be sure to correct this in v9. > Otherwise it's in very good shape. > > ... > > > +/* 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 > > Is it a bitfield? > > ... > > > + als_lower_l = als_lower_threshold; > > >> 0, but it's up to you. > > > + als_lower_h = als_lower_threshold >> 8; > > ... > > > + als_upper_l = als_upper_threshold; > > + als_upper_h = als_upper_threshold >> 8; > > Ditto. > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH v8 1/2] Added AMS tsl2591 driver implementation
On Fri, 16 Apr 2021 18:49:01 +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,
I was in two minds about whether to just apply this and roll in the below
suggestions + those Andy made or whether to ask you to do a v9.
The naming and units things below are complex enough that I'm not 100% sure
I'd get the right so please take a look and clean up those last few
corners!
Thanks,
Jonathan
> ---
> Changes in v8;
> - tsl2591_write_raw() - goto after tsl2591_set_als_gain_int_time() not
> necessary
> - tsl2591_read_event_value() and tsl2591_write_event_value() - break instead
> of goto in default case
> - Introduced tsl2591_info_no_irq iio_info structure which is used when the
> interrupt is disabled.
> This variant doesn't expose anything that can't be used when the interrupt
> is disabled
> - Corrected ordering of includes for
> - Renamed TSL2591_FVAL_TO_ATIME to TSL2591_MSEC_TO_ATIME
> - Renamed TSL2591_ATIME_TO_FVAL to TSL2591_ATIME_TO_MSEC
> - Changed TSL2591_STS_ALS* defines to all use BIT for consistency
> - Use (BIT(4) - 1) instead of value from list for
> TSL2591_PRST_ALS_INT_CYCLE_MAX
> - Cleaned up a few blank lines that were unneccesary
> - Use sysfs_emit() instead of snprintf() in
> tsl2591_in_illuminance_period_available_show()
> - Use err_unlock branch instead of err to be clear on mutex_unlock cases
> - Tidied up use of goto err_unlock in tsl2591_read_raw() to be consistent
> with other functions
>
> NOTE;
> - tsl2591_set_als_lower_threshold() and tsl2591_set_als_upper_threshold()
> forward declaration needed
> because tsl2591_set_als_lower_threshold() calls
> tsl2591_set_als_upper_threshold() and vice versa
>
> drivers/iio/light/Kconfig | 11 +
> drivers/iio/light/Makefile |1 +
> drivers/iio/light/tsl2591.c | 1227 +++
> 3 files changed, 1239 insertions(+)
> create mode 100644 drivers/iio/light/tsl2591.c
>
...
> 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 ..4f9c5e19ee35
> --- /dev/null
> +++ b/drivers/iio/light/tsl2591.c
> @@ -0,0 +1,1227 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Joe Sandom
Update perhaps?
> + *
> + * 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
> +#include
> +
> +/* ADC integration time, field value to time in ms */
> +#define TSL2591_MSEC_TO_ATIME(x) (((x) + 1) * 100)
Naming here seems backwards. I'd assume that at MSEC_TO_ATIME
function went msecs -> atime but it seems to be doing the opposite.
> +/* ADC integration time, time in ms to field value */
> +#define TSL2591_ATIME_TO_MSEC(x) (((x) / 100) - 1)
> +
...
> +
> +/*
> + * Period table is ALS persist cycle x integration time setting
> + * Integration times: 100ms, 200ms, 300ms, 400ms, 500ms, 600ms
> + * ALS cycles: 1, 2, 3, 5, 10, 20, 25, 30, 35, 40, 45, 50, 55, 60
> + */
> +static const char * const tsl2591_als_period_list[] = {
> + "0.1 0.2 0.3 0.5 1.0 2.0 2.5 3.0 3.5 4.0 4.5 5.0 5.5 6.0",
> + "0.2 0.4 0.6 1.0 2.0 4.0 5.0 6.0 7.0 8.0 9.0 10.0 11.0 12.0",
> + "0.3 0.6 0.9 1.5 3.0 6.0 7.5 9.0 10.5 12.0 13.5 15.0 16.5 18.0",
> + "0.4 0.8 1.2 2.0 4.0 8.0 10.0 12.0 14.0 16.0 18.0 20.0 22.0 24.0",
> + "0.5 1.0 1.5 2.5 5.0 10.0 12.5 15.0 17.5 20.0 22.5 25.0 27.5 30.0",
> + "0.6 1.2 1.8 3.0 6.0 12.0 15.0 18.0 21.0 24.0 27.0 30.0 33.0 36.0",
> +};
> +
> +static const int tsl2591_int_time_available[] = {
> + 100, 200, 300, 400, 500,
Re: [PATCH v8 1/2] Added AMS tsl2591 driver implementation
On Fri, Apr 16, 2021 at 8:49 PM 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. Hmm... It's v8 and the subject line is wrongly formatted. Please add the corresponding prefix "iio: light: ..." Otherwise it's in very good shape. ... > +/* 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 Is it a bitfield? ... > + als_lower_l = als_lower_threshold; >> 0, but it's up to you. > + als_lower_h = als_lower_threshold >> 8; ... > + als_upper_l = als_upper_threshold; > + als_upper_h = als_upper_threshold >> 8; Ditto. -- With Best Regards, Andy Shevchenko
[PATCH v8 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 v8; - tsl2591_write_raw() - goto after tsl2591_set_als_gain_int_time() not necessary - tsl2591_read_event_value() and tsl2591_write_event_value() - break instead of goto in default case - Introduced tsl2591_info_no_irq iio_info structure which is used when the interrupt is disabled. This variant doesn't expose anything that can't be used when the interrupt is disabled - Corrected ordering of includes for - Renamed TSL2591_FVAL_TO_ATIME to TSL2591_MSEC_TO_ATIME - Renamed TSL2591_ATIME_TO_FVAL to TSL2591_ATIME_TO_MSEC - Changed TSL2591_STS_ALS* defines to all use BIT for consistency - Use (BIT(4) - 1) instead of value from list for TSL2591_PRST_ALS_INT_CYCLE_MAX - Cleaned up a few blank lines that were unneccesary - Use sysfs_emit() instead of snprintf() in tsl2591_in_illuminance_period_available_show() - Use err_unlock branch instead of err to be clear on mutex_unlock cases - Tidied up use of goto err_unlock in tsl2591_read_raw() to be consistent with other functions NOTE; - tsl2591_set_als_lower_threshold() and tsl2591_set_als_upper_threshold() forward declaration needed because tsl2591_set_als_lower_threshold() calls tsl2591_set_als_upper_threshold() and vice versa drivers/iio/light/Kconfig | 11 + drivers/iio/light/Makefile |1 + drivers/iio/light/tsl2591.c | 1227 +++ 3 files changed, 1239 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 ..4f9c5e19ee35 --- /dev/null +++ b/drivers/iio/light/tsl2591.c @@ -0,0 +1,1227 @@ +// 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 +#include + +/* ADC integration time, field value to time in ms */ +#define TSL2591_MSEC_TO_ATIME(x) (((x) + 1) * 100) +/* ADC integration time, time in ms to field value */ +#define TSL2591_ATIME_TO_MSEC(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 */
