On Sun, Feb 14, 2021 at 12:23:53PM +0000, Jonathan Cameron wrote: > On Sat, 13 Feb 2021 13:22:41 +0000 > Joe Sandom <joe.g.sandom@xxxxxxxxx> 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, raw visible light, > > raw combined light and combined lux value. > > 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 Available at: https://ams.com/tsl25911 > > > > Signed-off-by: Joe Sandom <joe.g.sandom@xxxxxxxxx> > > Various bits and bobs inline. Note that before we can apply this > you will have to figure out how to deal with your email issues. > gmail will alow smtp I believe, so I'd use that and > git send-email if possible. > > Jonathan Hi Jonathan, Thanks again for the comprehensive review. A lot of valuable points made! all looks fine now with get send-email + gmail, thanks for that. I'll work on getting a new revision out this week. Thanks, Joe > > > --- > > Changes in v3: > > - Cleaned up descriptions in binding file and Kconfig > > - Changed macros to be uppercase > > - Cleaned up comment formatting for capitalisation and block comments > > - Changed tsl2591_settings to tsl2591_als_settings as settings only > > related to als > > - Return -EINVAL directly in default case to save some lines > > - Consistent use of const in "compatible" check functions > > - Removed mutex use in _show functions as not necessary > > - Removed print's which contribute little value/have little meaning > > > > NOTES; > > - Where spaces are seen at the end of some lines, it looks like gmail > > has mangled things slightly. > Ah. You need to work out how to fix that unfortunately otherwise it > won't be possible to apply patches. > > There are some hints in Documentation/process/email-clients.rst > > > > - checkpatch.pl --strict remarks that mutex definition should have a > > comment above it. I agree it has little meaning, but just added it to > > satisfy checkpatch.pl :) > > Hmm. Comment on that below. It should have a meaningful comment defining > the scope of the lock. > > > - For sysfs functions e.g. "in_illuminance_*", they're not currently > > prefixed with "tsl2591" because I wanted to keep things consistent > > with the other light drivers. Is this something we're looking to > > change with the other drivers too? > Most of these shouldn't exist anyway as they are handled by the core > of IIO via read_raw() and read_avail() callbacks. > > The one remainder is the event _available one as we haven't yet added > infrastructure to build those in the core. I'd prefer it was prefixed > but not enough to suggest we add noise fixing other drivers that do > it differently. Key rule of kernel development, never assume another > driver is right - it may just be we decided it wasn't worth the pain > of tidying it up! > > > > > REASON FOR RESEND; > > - Mailing lists were rejecting my outlook email, so switched to gmail as > > mailing lists seem to accept without issues. > > > > .../bindings/iio/light/amstaos,tsl2591.yaml | 50 + > > drivers/iio/light/Kconfig | 11 + > > drivers/iio/light/Makefile | 1 + > > drivers/iio/light/tsl2591.c | 1220 +++++++++++++++++ > > 4 files changed, 1282 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml > > create mode 100644 drivers/iio/light/tsl2591.c > > > > diff --git a/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml > > new file mode 100644 > > index 000000000000..29a53e0019dd > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml > > @@ -0,0 +1,50 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > Dt bindings should be a in a separate patch titled > dt-bindings: * > to make them easy for Rob and devicetree specialists to spot. > > > +$id: http://devicetree.org/schemas/iio/light/tsl2591.yaml# > Try building this binding as per instructions in > Documentation/devicetree/writing-schema.rst > > The line above needs the vendor prefix. > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: AMS/TAOS TSL2591 Ambient Light Sensor (ALS) > > + > > +maintainers: > > + - Joe Sandom <joe.g.sandom@xxxxxxxxx> > > + > > +description: | > > + AMS/TAOS TSL2591 is a very-high sensitivity > > + light-to-digital converter that transforms light intensity into a digital > > + signal. > > + > > +properties: > > + compatible: > > + const: amstaos,tsl2591 > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + description: > > + Interrupt (INT:Pin 2) Active low. Should be set to IRQ_TYPE_EDGE_FALLING. > > + interrupt is used to detect if the light intensity has fallen below > > + or reached above the configured threshold values. > > + > > +required: > > + - compatible > > + - reg > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + tsl2591@29 { > > + compatible = "amstaos,tsl2591"; > > + reg = <0x29>; > > + interrupts = <20 IRQ_TYPE_EDGE_FALLING>; > > + }; > > + }; > > +... > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > > index 33ad4dd0b5c7..07550f1a1783 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 als 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 000000000000..fafa1f11c1f9 > > --- /dev/null > > +++ b/drivers/iio/light/tsl2591.c > > @@ -0,0 +1,1220 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (C) 2020 Joe Sandom <joe.g.sandom@xxxxxxxxx> > > + * > > + * Datasheet available at: https://ams.com/tsl25911 > > + * > > + * 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 <linux/debugfs.h> > > +#include <linux/delay.h> > > +#include <linux/i2c.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/pm_runtime.h> > > + > > +#include <linux/iio/events.h> > > +#include <linux/iio/iio.h> > > +#include <linux/iio/sysfs.h> > > + > > +/* ALS integration time conversion macros */ > > +#define ALS_TIME_SECS_TO_MS(x) (((x) + 1) * 100) > > Prefix as TSL2591_TIME_SECS_TO_MS > > Though it's also confusing because * 100 doesn't normally > do seconds to milliseconds, so what is this doing? > > > +#define ALS_TIME_MS_TO_SECS(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_AILTL 0x08 > > +#define TSL2591_NP_AILTH 0x09 > > +#define TSL2591_NP_AIHTL 0x0A > > +#define TSL2591_NP_AIHTH 0x0B > > +#define TSL2591_PERSIST 0x0C > > +#define TSL2591_PACKAGE_ID 0x11 > > +#define TSL2591_DEVICE_ID 0x12 > > +#define TSL2591_STATUS 0x13 > > +#define TSL2591_C0_DATAL 0x14 > > +#define TSL2591_C0_DATAH 0x15 > > +#define TSL2591_C1_DATAL 0x16 > > +#define TSL2591_C1_DATAH 0x17 > > + > > +/* 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_INT 0x40 > > +#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 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 > > + > > +/* TSL2591 persist register definitions */ > > +#define TSL2591_PRST_ALS_INT_CYCLE_0 0x00 > > +#define TSL2591_PRST_ALS_INT_CYCLE_ANY 0x01 > > +#define TSL2591_PRST_ALS_INT_CYCLE_2 0x02 > > +#define TSL2591_PRST_ALS_INT_CYCLE_3 0x03 > > +#define TSL2591_PRST_ALS_INT_CYCLE_5 0x04 > > +#define TSL2591_PRST_ALS_INT_CYCLE_10 0x05 > > +#define TSL2591_PRST_ALS_INT_CYCLE_15 0x06 > > +#define TSL2591_PRST_ALS_INT_CYCLE_20 0x07 > > +#define TSL2591_PRST_ALS_INT_CYCLE_25 0x08 > > +#define TSL2591_PRST_ALS_INT_CYCLE_30 0x09 > > +#define TSL2591_PRST_ALS_INT_CYCLE_35 0x0A > > +#define TSL2591_PRST_ALS_INT_CYCLE_40 0x0B > > +#define TSL2591_PRST_ALS_INT_CYCLE_45 0x0C > > +#define TSL2591_PRST_ALS_INT_CYCLE_50 0x0D > > +#define TSL2591_PRST_ALS_INT_CYCLE_55 0x0E > > +#define TSL2591_PRST_ALS_INT_CYCLE_60 0x0F > > +#define TSL2591_PRST_ALS_INT_CYCLE_MAX TSL2591_PRST_ALS_INT_CYCLE_60 > > + > > +/* TSL2591 persist interrupt cycle literals */ > > +#define TSL2591_PRST_ALS_INT_CYCLE_1_LIT 1 > > +#define TSL2591_PRST_ALS_INT_CYCLE_2_LIT 2 > > +#define TSL2591_PRST_ALS_INT_CYCLE_3_LIT 3 > > +#define TSL2591_PRST_ALS_INT_CYCLE_5_LIT 5 > > +#define TSL2591_PRST_ALS_INT_CYCLE_10_LIT 10 > > +#define TSL2591_PRST_ALS_INT_CYCLE_15_LIT 15 > > +#define TSL2591_PRST_ALS_INT_CYCLE_20_LIT 20 > > +#define TSL2591_PRST_ALS_INT_CYCLE_25_LIT 25 > > +#define TSL2591_PRST_ALS_INT_CYCLE_30_LIT 30 > > +#define TSL2591_PRST_ALS_INT_CYCLE_35_LIT 35 > > +#define TSL2591_PRST_ALS_INT_CYCLE_40_LIT 40 > > +#define TSL2591_PRST_ALS_INT_CYCLE_45_LIT 45 > > +#define TSL2591_PRST_ALS_INT_CYCLE_50_LIT 50 > > +#define TSL2591_PRST_ALS_INT_CYCLE_55_LIT 55 > > +#define TSL2591_PRST_ALS_INT_CYCLE_60_LIT 60 > > + > > +/* TSL2591 PID register mask */ > > +#define TSL2591_PACKAGE_ID_MASK 0x30 > > + > > +/* TSL2591 ID register mask */ > > +#define TSL2591_DEVICE_ID_MASK 0xFF > > + > > +/* TSL2591 status register masks */ > > +#define TSL2591_STS_ALS_VALID_MASK 0x01 > > BIT() or GENMASK() for masks. > > > +#define TSL2591_STS_ALS_INT_MASK 0x10 > > +#define TSL2591_STS_NPERS_INT_MASK 0x20 > > +#define TSL2591_STS_VAL_HIGH_MASK 0x01 > > + > > +/* TSL2591 constant values */ > > +#define TSL2591_PACKAGE_ID_VAL 0x00 > > +#define TSL2591_DEVICE_ID_VAL 0x50 > > + > > +/* Power off suspend delay time MS */ > > +#define TSL2591_POWER_OFF_DELAY_MS 2000 > > + > > +/* TSL2591 default values */ > > +#define TSL2591_DEFAULT_ALS_INT_TIME TSL2591_CTRL_ALS_INTEGRATION_300MS > > +#define TSL2591_DEFAULT_ALS_GAIN TSL2591_CTRL_ALS_MED_GAIN > > +#define TSL2591_DEFAULT_ALS_PERSIST TSL2591_PRST_ALS_INT_CYCLE_ANY > > +#define TSL2591_DEFAULT_ALS_LOWER_THRESHOLD 100 > > +#define TSL2591_DEFAULT_ALS_UPPER_THRESHOLD 1500 > > + > > +/* TSL2591 number of data channels */ > > +#define TSL2591_NUM_DATA_CHANNELS 4 > That's not true. There are 4 bytes, but it's not the number of channels. > > + > > +/* TSL2591 number of valid status reads on ADC complete */ > > +#define TSL2591_ALS_STS_VALID_COUNT 10 > > + > > +/* TSL2591 maximum values */ > > +#define TSL2591_MAX_ALS_INT_TIME_MS 600 > > +#define TSL2591_ALS_MAX_VALUE 65535 > > + > > +/* > > + * LUX calculations; > > + * AGAIN values from Adafruits TSL2591 Arduino library > > + * https://github.com/adafruit/Adafruit_TSL2591_Library > > + */ > > +#define TSL2591_CTRL_ALS_LOW_GAIN_MULTIPLIER 1 > > +#define TSL2591_CTRL_ALS_MED_GAIN_MULTIPLIER 25 > > +#define TSL2591_CTRL_ALS_HIGH_GAIN_MULTIPLIER 428 > > +#define TSL2591_CTRL_ALS_MAX_GAIN_MULTIPLIER 9876 > > +#define TSL2591_LUX_COEFFICIENT 408 > > + > > +static const u8 tsl2591_data_channels[] = { > > They aren't channels, they are registers so rename as such. > > > + TSL2591_C0_DATAL, > > + TSL2591_C0_DATAH, > > + TSL2591_C1_DATAL, > > + TSL2591_C1_DATAH, > > +}; > > + > > +struct tsl2591_als_readings { > As mentioned below, I don't think this structure is useful. > Rewrite the function that fills it to return only the value > or values of interest to a particular call rather than burying this > data inside the chip state structure. > > > + u16 als_ch0; > > + u16 als_ch1; > > + u16 als_lux_int; > > + u16 als_lux_decimal; > > +}; > > + > > +struct tsl2591_als_settings { > > + u8 als_int_time; > > + u8 als_gain; > > + u8 als_persist; > > + u16 als_lower_threshold; > > + u16 als_upper_threshold; > > +}; > > + > > +struct tsl2591_chip { > > + /* ALS mutex */ > > Not informative. For a lock what we want is a statement of > what state it is protecting. > > /* > * Keep als_settings in sync with hardware state > * and ensure multiple readers are serialized. > */ > > Though more detail that that would be even better. > > > + struct mutex als_mutex; > > + struct i2c_client *client; > > + struct tsl2591_als_settings als_settings; > > + struct tsl2591_als_readings als_readings; > > + > > + bool events_enabled; > > +}; > > + > > +/* > > + * 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 int tsl2591_gain_to_multiplier(const u8 als_gain) > > +{ > > + switch (als_gain) { > > + case TSL2591_CTRL_ALS_LOW_GAIN: > > + return TSL2591_CTRL_ALS_LOW_GAIN_MULTIPLIER; > > + case TSL2591_CTRL_ALS_MED_GAIN: > > + return TSL2591_CTRL_ALS_MED_GAIN_MULTIPLIER; > > + case TSL2591_CTRL_ALS_HIGH_GAIN: > > + return TSL2591_CTRL_ALS_HIGH_GAIN_MULTIPLIER; > > + case TSL2591_CTRL_ALS_MAX_GAIN: > > + return TSL2591_CTRL_ALS_MAX_GAIN_MULTIPLIER; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static u8 tsl2591_multiplier_to_gain(const u32 multiplier) > > +{ > > + switch (multiplier) { > > + case TSL2591_CTRL_ALS_LOW_GAIN_MULTIPLIER: > > + return TSL2591_CTRL_ALS_LOW_GAIN; > > + case TSL2591_CTRL_ALS_MED_GAIN_MULTIPLIER: > > + return TSL2591_CTRL_ALS_MED_GAIN; > > + case TSL2591_CTRL_ALS_HIGH_GAIN_MULTIPLIER: > > + return TSL2591_CTRL_ALS_HIGH_GAIN; > > + case TSL2591_CTRL_ALS_MAX_GAIN_MULTIPLIER: > > + return TSL2591_CTRL_ALS_MAX_GAIN; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int tsl2591_persist_cycle_to_lit(const u8 als_persist) > > +{ > > + switch (als_persist) { > > + case TSL2591_PRST_ALS_INT_CYCLE_ANY: > > + return TSL2591_PRST_ALS_INT_CYCLE_1_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_2: > > + return TSL2591_PRST_ALS_INT_CYCLE_2_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_3: > > + return TSL2591_PRST_ALS_INT_CYCLE_3_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_5: > > + return TSL2591_PRST_ALS_INT_CYCLE_5_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_10: > > + return TSL2591_PRST_ALS_INT_CYCLE_10_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_15: > > + return TSL2591_PRST_ALS_INT_CYCLE_15_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_20: > > + return TSL2591_PRST_ALS_INT_CYCLE_20_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_25: > > + return TSL2591_PRST_ALS_INT_CYCLE_25_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_30: > > + return TSL2591_PRST_ALS_INT_CYCLE_30_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_35: > > + return TSL2591_PRST_ALS_INT_CYCLE_35_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_40: > > + return TSL2591_PRST_ALS_INT_CYCLE_40_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_45: > > + return TSL2591_PRST_ALS_INT_CYCLE_45_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_50: > > + return TSL2591_PRST_ALS_INT_CYCLE_50_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_55: > > + return TSL2591_PRST_ALS_INT_CYCLE_55_LIT; > > + case TSL2591_PRST_ALS_INT_CYCLE_60: > > + return TSL2591_PRST_ALS_INT_CYCLE_60_LIT; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int tsl2591_persist_lit_to_cycle(const u8 als_persist) > > +{ > > + switch (als_persist) { > > + case TSL2591_PRST_ALS_INT_CYCLE_1_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_ANY; > > + case TSL2591_PRST_ALS_INT_CYCLE_2_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_2; > > + case TSL2591_PRST_ALS_INT_CYCLE_3_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_3; > > + case TSL2591_PRST_ALS_INT_CYCLE_5_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_5; > > + case TSL2591_PRST_ALS_INT_CYCLE_10_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_10; > > + case TSL2591_PRST_ALS_INT_CYCLE_15_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_15; > > + case TSL2591_PRST_ALS_INT_CYCLE_20_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_20; > > + case TSL2591_PRST_ALS_INT_CYCLE_25_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_25; > > + case TSL2591_PRST_ALS_INT_CYCLE_30_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_30; > > + case TSL2591_PRST_ALS_INT_CYCLE_35_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_35; > > + case TSL2591_PRST_ALS_INT_CYCLE_40_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_40; > > + case TSL2591_PRST_ALS_INT_CYCLE_45_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_45; > > + case TSL2591_PRST_ALS_INT_CYCLE_50_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_50; > > + case TSL2591_PRST_ALS_INT_CYCLE_55_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_55; > > + case TSL2591_PRST_ALS_INT_CYCLE_60_LIT: > > + return TSL2591_PRST_ALS_INT_CYCLE_60; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int tsl2591_compatible_int_time(struct tsl2591_chip *chip, > > + const u32 als_integration_time) > > +{ > > + switch (als_integration_time) { > > + case TSL2591_CTRL_ALS_INTEGRATION_100MS: > > + case TSL2591_CTRL_ALS_INTEGRATION_200MS: > > + case TSL2591_CTRL_ALS_INTEGRATION_300MS: > > + case TSL2591_CTRL_ALS_INTEGRATION_400MS: > > + case TSL2591_CTRL_ALS_INTEGRATION_500MS: > > + case TSL2591_CTRL_ALS_INTEGRATION_600MS: > > + chip->als_settings.als_int_time = als_integration_time; > > + return 0; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int tsl2591_compatible_gain(struct tsl2591_chip *chip, const u8 als_gain) > > +{ > > + switch (als_gain) { > > + case TSL2591_CTRL_ALS_LOW_GAIN: > > + case TSL2591_CTRL_ALS_MED_GAIN: > > + case TSL2591_CTRL_ALS_HIGH_GAIN: > > + case TSL2591_CTRL_ALS_MAX_GAIN: > > + chip->als_settings.als_gain = als_gain; > > As below, either set at caller rather than hiding in here, or > possibly rename the function to indicate it's setting this. > > > + return 0; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int tsl2591_compatible_als_persist(struct tsl2591_chip *chip, > > + const u32 als_persist) > > +{ > > + switch (als_persist) { > > + case TSL2591_PRST_ALS_INT_CYCLE_ANY: > > + case TSL2591_PRST_ALS_INT_CYCLE_2: > > + case TSL2591_PRST_ALS_INT_CYCLE_3: > > + case TSL2591_PRST_ALS_INT_CYCLE_5: > > + case TSL2591_PRST_ALS_INT_CYCLE_10: > > + case TSL2591_PRST_ALS_INT_CYCLE_15: > > + case TSL2591_PRST_ALS_INT_CYCLE_20: > > + case TSL2591_PRST_ALS_INT_CYCLE_25: > > + case TSL2591_PRST_ALS_INT_CYCLE_30: > > + case TSL2591_PRST_ALS_INT_CYCLE_35: > > + case TSL2591_PRST_ALS_INT_CYCLE_40: > > + case TSL2591_PRST_ALS_INT_CYCLE_45: > > + case TSL2591_PRST_ALS_INT_CYCLE_50: > > + case TSL2591_PRST_ALS_INT_CYCLE_55: > > + case TSL2591_PRST_ALS_INT_CYCLE_60: > > + chip->als_settings.als_persist = als_persist; > > Don't hid this in a function that appears to be simply checking it is > a valid value. Return the value and set > chip->als_settings.als_persist at the caller instead. > > > + return 0; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int tsl2591_wait_adc_complete(struct tsl2591_chip *chip) > > +{ > > + struct i2c_client *client = chip->client; > > + struct tsl2591_als_settings settings = chip->als_settings; > > + int delay = ALS_TIME_SECS_TO_MS(settings.als_int_time); > > + int ret; > > + int i; > > + > > + if (!delay) > > + return -EINVAL; > > + > > + /* > > + * Sleep for ALS integration time to allow enough time > > + * for an ADC read cycle to complete. Check status after > > + * delay for ALS valid > > + */ > > + msleep(delay); > > + > > + /* Check for status ALS valid flag for up to 100ms */ > > + for (i = 0; i < TSL2591_ALS_STS_VALID_COUNT; ++i) { > > + ret = i2c_smbus_read_byte_data(client, TSL2591_CMD_NOP | > > + TSL2591_STATUS); > > + if (ret < 0) { > > + dev_err(&client->dev, "Failed to read register\n"); > > + return -EINVAL; > > + } > > + > > + if ((ret & TSL2591_STS_ALS_VALID_MASK) == TSL2591_STS_VAL_HIGH_MASK) > > + break; > > + > > + if (i == (TSL2591_ALS_STS_VALID_COUNT - 1)) > > + return -ENODATA; > > + > > + usleep_range(9000, 10000); > > + } > > + > > + return 0; > > +} > > + > > +/* > > + * tsl2591_read_channel_data() - Reads raw channel data and calculates lux > > + * > > + * Formula for lux calculation; > > + * Derived from Adafruit's TSL2591 library > > + * Link: https://github.com/adafruit/Adafruit_TSL2591_Library > > + * Counts Per Lux (CPL) = (ATIME_ms * AGAIN) / LUX DF > > + * lux = ((C0DATA - C1DATA) * (1 - (C1DATA / C0DATA))) / CPL > > + * > > + * Scale values to get more representative value of lux i.e. > > + * lux = ((C0DATA - C1DATA) * (1000 - ((C1DATA * 1000) / C0DATA))) / CPL > > + * > > + * Channel 0 = IR + Visible > > + * Channel 1 = IR only > > + * Visible = Channel 0 - Channel 1 > > + * > > + */ > > +static int tsl2591_read_channel_data(struct iio_dev *indio_dev) > > I think this would end up simpler if you had this function return the reading > requested directly rather than putting it in a structure from where > it is later fetched. > > If we also specified which channel was being read it would cut down > on the amount of work if it's not the lux channel. > > > +{ > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + struct tsl2591_als_settings *settings = &chip->als_settings; > > + struct i2c_client *client = chip->client; > > + int i; > > + int ret; > > + u8 channel_data[TSL2591_NUM_DATA_CHANNELS]; > > + > > + int counts_per_lux; > > + int lux; > > + int gain_multiplier; > > + > > + ret = tsl2591_wait_adc_complete(chip); > > + if (ret < 0) { > > + dev_err(&client->dev, "No data available. Err: %d\n", ret); > > + return ret; > > + } > > + > > + for (i = 0; i < TSL2591_NUM_DATA_CHANNELS; ++i) { > > + int reg = TSL2591_CMD_NOP | tsl2591_data_channels[i]; > > + > > + ret = i2c_smbus_read_byte_data(client, TSL2591_CMD_NOP | reg); > > + if (ret < 0) { > > + dev_err(&client->dev, > > + "Failed to read register %#04x\n", reg); > > + return -EINVAL; > > + } > > + channel_data[i] = ret; > > This loop looked unusual, so I had a look at the datasheet. > Can we not use the autoincrement and 4 byte read mentioned in the docs > for these registers? > > Mind you, the random datasheet google found for me doesn't actually say how > to do this - so perhaps better playing it safe and doing 4 single byte reads. > > > + } > > + > > + chip->als_readings.als_ch0 = > > + le16_to_cpup((const __le16 *)&channel_data[0]); > > + chip->als_readings.als_ch1 = > > + le16_to_cpup((const __le16 *)&channel_data[2]); > > + > > + dev_dbg(&client->dev, "both: %d\n", chip->als_readings.als_ch0); > > + dev_dbg(&client->dev, "ir: %d\n", chip->als_readings.als_ch1); > > Given we can read those both directly seems pointless to have this debug > info here in a production driver. > > > + > > + gain_multiplier = tsl2591_gain_to_multiplier(settings->als_gain); > > + if (gain_multiplier < 0) { > > + dev_err(&client->dev, "Invalid multiplier. Err: %d\n", > > + gain_multiplier); > > + return -EINVAL; > > return gain_multiplier; > > Obviously it's current the same thing but better to not give impression of > eating a potentially useful error code. > > > + } > > + > > + /* Calculate counts per lux value */ > > + counts_per_lux = (ALS_TIME_SECS_TO_MS(settings->als_int_time) * > > + gain_multiplier) / TSL2591_LUX_COEFFICIENT; > > + > > + dev_dbg(&client->dev, "Counts Per Lux (CPL): %d\n", counts_per_lux); > > + > > + /* Calculate lux value */ > > + lux = ((chip->als_readings.als_ch0 - chip->als_readings.als_ch1) * > > + (1000 - ((chip->als_readings.als_ch1 * 1000) / > > + chip->als_readings.als_ch0))) / counts_per_lux; > > + > > + /* Divide by 1000 to get real lux value before scaling */ > > + chip->als_readings.als_lux_int = lux / 1000; > > + > > + /* Get the decimal part of lux reading */ > > + chip->als_readings.als_lux_decimal = > > + (lux - (chip->als_readings.als_lux_int * 1000)); > > + > > + dev_dbg(&client->dev, "Raw lux calculation: %d\n", lux); > > + > > + return ret; > > +} > > + > > +static int tsl2591_set_als_gain_int_time(struct tsl2591_chip *chip) > > +{ > > + struct i2c_client *client = chip->client; > > + struct tsl2591_als_settings als_settings = chip->als_settings; > > + int ret; > > + > > + ret = i2c_smbus_write_byte_data(client, > > + TSL2591_CMD_NOP | TSL2591_CONTROL, > > + als_settings.als_int_time | > > + als_settings.als_gain); > > + if (ret < 0) > > + dev_err(&client->dev, "Failed to set als gain & int time\n"); > > + > > + return ret; > > +} > > + > > +static int tsl2591_set_als_thresholds(struct tsl2591_chip *chip) > > +{ > > + struct i2c_client *client = chip->client; > > + struct tsl2591_als_settings als_settings = chip->als_settings; > > + int ret; > > + > > + u8 als_lower_l; > > + u8 als_lower_h; > > + u8 als_upper_l; > > + u8 als_upper_h; > > + > > + if (als_settings.als_lower_threshold >= als_settings.als_upper_threshold) { > > + dev_warn(&client->dev, "Lower threshold higher than upper\n"); > This looks like it can be triggered directly from userspace. Don't print to the > kernel logs in that case, just return an error if the driver can't cope with the > value provided. > > Note however, that this may mean thresholds need to be set in a particular order > and we almost certainly want to avoid that. Various solutions exist. > 1) Only update thresholds at event enable. - may not be wise on this device > as we frequently want to adjust light thresholds during normal usecases. > 2) Enforce the constraint by changing the other threshold as well if needed. > > > > + return -EINVAL; > > + } > > + > > + if (als_settings.als_upper_threshold > TSL2591_ALS_MAX_VALUE) { > > + dev_warn(&client->dev, "Upper threshold higher than max\n"); > > + return -EINVAL; > > + } > > + > > + als_lower_l = (als_settings.als_lower_threshold & 0x00FF); > > + als_lower_h = ((als_settings.als_lower_threshold >> 8) & 0x00FF); > > + als_upper_l = (als_settings.als_upper_threshold & 0x00FF); > > + als_upper_h = ((als_settings.als_upper_threshold >> 8) & 0x00FF); > > + > > + dev_dbg(&client->dev, "Setting configuration - als lower l: %#04x\n", > > + als_lower_l); > > + dev_dbg(&client->dev, "Setting configuration - als lower h: %#04x\n", > > + als_lower_h); > > + dev_dbg(&client->dev, "Setting configuration - als upper l: %#04x\n", > > + als_upper_l); > > + dev_dbg(&client->dev, "Setting configuration - als upper h: %#04x\n", > > + als_upper_h); > > This level of debug doesn't belong in an a driver in upstream. It's useful > stuff whilst debugging perhaps, but not remote under normal use conditions. > > > + > > + ret = i2c_smbus_write_byte_data(client, TSL2591_CMD_NOP | > > + TSL2591_AILTL, als_lower_l); > As with all the other cases like this reformat to put line breaks > in places that are better for readability + align with opening brackets > e.g. > > ret = i2c_smbus_write_byte_data(client, > TSL2591_CMD_NOP | TSL2591_AILTL, > als_lower_l); > > > + if (ret < 0) > > + dev_err(&client->dev, "Failed to set als lower threshold\n"); > > Don't carry on. Always return on first error. > > > + > > + ret = i2c_smbus_write_byte_data(client, TSL2591_CMD_NOP | > > + TSL2591_AILTH, als_lower_h); > > + if (ret < 0) > > + dev_err(&client->dev, "Failed to set als lower threshold\n"); > > + > > + ret = i2c_smbus_write_byte_data(client, TSL2591_CMD_NOP | > > + TSL2591_AIHTL, als_upper_l); > > + if (ret < 0) > > + dev_err(&client->dev, "Failed to set als upper threshold\n"); > > + > > + ret = i2c_smbus_write_byte_data(client, TSL2591_CMD_NOP | > > + TSL2591_AIHTH, als_upper_h); > > + if (ret < 0) > > + dev_err(&client->dev, "Failed to set als upper threshold\n"); > > + > > + return ret; > > +} > > + > > +static int tsl2591_set_als_persist_cycle(struct tsl2591_chip *chip) > > +{ > > + struct i2c_client *client = chip->client; > > + struct tsl2591_als_settings als_settings = chip->als_settings; > > + int ret; > > + > > + ret = i2c_smbus_write_byte_data(client, TSL2591_CMD_NOP | > > + TSL2591_PERSIST, als_settings.als_persist); > > Please align parameters with opening bracket where possible (we are a bit > flexible on this where it would lead to very long lines. Also, avoid unusual > line breaks mid parameter if you can. > > ret = i2c_smbus_write_byte_data(client, > TSL2591_CMD_NOP | TSL2591_PERSIST, > als_settings.als_persist); > > Is the best we can do with this case. > > > > + if (ret < 0) > > + dev_err(&client->dev, "Failed to set als persist cycle\n"); > > + > > + return ret; > > +} > > + > > +static int tsl2591_set_power_state(struct tsl2591_chip *chip, u8 state) > > +{ > > + struct i2c_client *client = chip->client; > > + int ret; > > + > > + ret = i2c_smbus_write_byte_data(client, TSL2591_CMD_NOP | > > + TSL2591_ENABLE, state); > > + if (ret < 0) > > + dev_err(&client->dev, > > + "Failed to set the power state to %#04x\n", state); > > + > > + return ret; > > +} > > + > > +static int tsl2591_get_power_state(struct tsl2591_chip *chip) > > +{ > > + struct i2c_client *client = chip->client; > > + int ret; > > + > > + ret = i2c_smbus_read_byte_data(client, TSL2591_CMD_NOP | > > + TSL2591_ENABLE); > > + if (ret < 0) > > + dev_err(&client->dev, "Failed to get power state\n"); > > + > > + return ret; > > +} > > + > > +static ssize_t in_illuminance_integration_time_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); > > + > > + int als_int_time = ALS_TIME_SECS_TO_MS(chip->als_settings.als_int_time); > > + > > + return sprintf(buf, "%d\n", als_int_time); > > +} > > + > > +static ssize_t in_illuminance_integration_time_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t len) > > +{ > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + struct i2c_client *client = chip->client; > > + > > + u32 int_time; > > + int value; > > + > > + if (kstrtoint(buf, 0, &value) || !value) > > + return -EINVAL; > > + > > + mutex_lock(&chip->als_mutex); > > + > > + int_time = ALS_TIME_MS_TO_SECS(value); > > + > > + if (tsl2591_compatible_int_time(chip, int_time)) > > + goto calibrate_error; > > + > > + if (tsl2591_set_als_gain_int_time(chip)) > > + goto calibrate_error; > > + > > + mutex_unlock(&chip->als_mutex); > > + > > + return len; > > + > > +calibrate_error: > > + dev_err(&client->dev, "Failed to calibrate sensor\n"); > > + mutex_unlock(&chip->als_mutex); > > + return -EINVAL; > > +} > > + > > +static ssize_t in_illuminance_calibscale_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 sprintf(buf, "%d\n", tsl2591_gain_to_multiplier > > + (chip->als_settings.als_gain)); > > +} > > + > > +static ssize_t in_illuminance_calibscale_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t len) > > +{ > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + struct i2c_client *client = chip->client; > > + > > + u8 gain; > > + int value; > > + > > + if (kstrtoint(buf, 0, &value) || !value) > > + return -EINVAL; > > + > > + mutex_lock(&chip->als_mutex); > > + > > + gain = tsl2591_multiplier_to_gain(value); > > + > > + if (tsl2591_compatible_gain(chip, gain)) > > + goto calibrate_error; > > + > > + if (tsl2591_set_als_gain_int_time(chip)) > > + goto calibrate_error; > > + > > + mutex_unlock(&chip->als_mutex); > > + > > + return len; > > + > > +calibrate_error: > > + dev_err(&client->dev, "Failed to calibrate sensor\n"); > > + mutex_unlock(&chip->als_mutex); > > + return -EINVAL; > > +} > > + > > +static ssize_t 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", > > + tsl2591_als_period_list[chip->als_settings.als_int_time]); > > +} > > + > > +static IIO_CONST_ATTR(in_illuminance_integration_time_available, > > + "100 200 300 400 500 600"); > > +static IIO_CONST_ATTR(in_illuminance_calibscale_available, > > + "1 25 428 9876"); > > +static IIO_DEVICE_ATTR_RW(in_illuminance_integration_time, 0); > > +static IIO_DEVICE_ATTR_RW(in_illuminance_calibscale, 0); > > + > > +static struct attribute *tsl2591_sysfs_attrs_ctrl[] = { > > We should only see attrs like this for obscure bits of interface that > don't form part of the standard IIO interfaces. These are all > handled via the info_mask system. > That brings both advantages in reviewability (as we don't have to check > the naming etc is as per the ABI) and also make them available for use > by in kernel consumers of the IIO channels (though for light sensors I don't > think there are any yet). > > > + &iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr, > > We have the read_avail() callback to provide these _available elements. > Note that it's not universally used yet in IIO as it's a relatively recent > addition, but I would prefer that new drivers use it. > Also add BIT(IIO_CHAN_INFO_INT_TIME) to info_mask_shared_by_all_available > > > + &iio_const_attr_in_illuminance_calibscale_available.dev_attr.attr, > > + &iio_dev_attr_in_illuminance_integration_time.dev_attr.attr, > > This is a standard IIO element you can do by adding > BIT(IIO_CHAN_INFO_INT_TIME) to info_mask_shared_by_all > > > + &iio_dev_attr_in_illuminance_calibscale.dev_attr.attr, > > + NULL > > +}; > > + > > +static const struct attribute_group tsl2591_sysfs_attribute_group = { > > + .attrs = tsl2591_sysfs_attrs_ctrl, > > +}; > > + > > +static IIO_DEVICE_ATTR_RO(in_illuminance_period_available, 0); > > + > > +static struct attribute *tsl2591_event_attrs_ctrl[] = { > > + &iio_dev_attr_in_illuminance_period_available.dev_attr.attr, > > + NULL > > +}; > > + > > +static const struct attribute_group tsl2591_event_attribute_group = { > > + .attrs = tsl2591_event_attrs_ctrl, > > +}; > > + > > +static const struct iio_event_spec tsl2591_events[] = { > > + { > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_RISING, > > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > > + }, { > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_FALLING, > > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > > + }, { > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_EITHER, > > + .mask_separate = BIT(IIO_EV_INFO_PERIOD) | > > + BIT(IIO_EV_INFO_ENABLE), > > + }, > > +}; > > + > > +static const struct iio_chan_spec tsl2591_channels[] = { > > + { > > + .type = IIO_INTENSITY, > > + .modified = 1, > > + .channel2 = IIO_MOD_LIGHT_IR, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > > + }, > > + { > > + .type = IIO_INTENSITY, > > + .modified = 1, > > + .channel2 = IIO_MOD_LIGHT_BOTH, > > + .event_spec = tsl2591_events, > > + .num_event_specs = ARRAY_SIZE(tsl2591_events), > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > > + }, > > + { > > + .type = IIO_LIGHT, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > > + }, > > +}; > > + > > +static int tsl2591_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + struct i2c_client *client = chip->client; > > + int ret; > > + > > + dev_dbg(&client->dev, "Reading from sensor"); > > Drop this low level debug. It isn't useful in a production driver. > It just tells you the function was called and ftrace etc can do that > much better than a debug print. > > > + > > + pm_runtime_get_sync(&chip->client->dev); > > + > > + mutex_lock(&chip->als_mutex); > > + > > + ret = -EINVAL; > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + if (chan->type == IIO_INTENSITY) { > > Flip condition to reduce deep indenting. > > if (chan->type != IIO_INTENSITY) > break; > > ret = tsl... > > > + ret = tsl2591_read_channel_data(indio_dev); > > + if (ret < 0) > > + break; > > + > > + if (chan->channel2 == IIO_MOD_LIGHT_BOTH) { > > + *val = chip->als_readings.als_ch0; > > + ret = IIO_VAL_INT; > > + } else if (chan->channel2 == IIO_MOD_LIGHT_IR) { > > + *val = chip->als_readings.als_ch1; > > + ret = IIO_VAL_INT; > > + } else { > > + ret = -EINVAL; > > + } > > + } > > + break; > > + case IIO_CHAN_INFO_PROCESSED: > > + if (chan->type == IIO_LIGHT) { > > + ret = tsl2591_read_channel_data(indio_dev); > > + if (ret < 0) > > + break; > > + *val = chip->als_readings.als_lux_int; > > + *val2 = (chip->als_readings.als_lux_decimal * 1000); > > + ret = IIO_VAL_INT_PLUS_MICRO; > > + } > > + break; > > + } > > + > > + mutex_unlock(&chip->als_mutex); > > + > > + pm_runtime_mark_last_busy(&chip->client->dev); > > + pm_runtime_put_autosuspend(&chip->client->dev); > > + > > + return ret; > > +} > > + > > +static int tsl2591_read_event_value(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > Fix all this indentation. > > > + enum iio_event_type type, > > + enum iio_event_direction dir, > > + enum iio_event_info info, int *val, > > + int *val2) > > +{ > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + struct i2c_client *client = chip->client; > > + int als_persist; > > + int period; > > + int ret; > > + int int_time; > > + > > + switch (info) { > > + case IIO_EV_INFO_VALUE: > > + switch (dir) { > > + case IIO_EV_DIR_RISING: > > + mutex_lock(&chip->als_mutex); > > As below, I'd take the lock outside of the switch statements. > > > + *val = chip->als_settings.als_upper_threshold; > > + mutex_unlock(&chip->als_mutex); > > + break; > > + case IIO_EV_DIR_FALLING: > > + mutex_lock(&chip->als_mutex); > > + *val = chip->als_settings.als_lower_threshold; > > + mutex_unlock(&chip->als_mutex); > > + break; > > + default: > > + return -EINVAL; > > + } > > + ret = IIO_VAL_INT; > > + break; > > + case IIO_EV_INFO_PERIOD: > > + mutex_lock(&chip->als_mutex); > > + > > + ret = i2c_smbus_read_byte_data(client, > > + TSL2591_CMD_NOP | TSL2591_PERSIST); > > + if (ret <= 0 || ret > TSL2591_PRST_ALS_INT_CYCLE_MAX) > > + goto err; > > + > > + als_persist = tsl2591_persist_cycle_to_lit(ret); > > + int_time = ALS_TIME_SECS_TO_MS(chip->als_settings.als_int_time); > > + period = als_persist * (int_time * MSEC_PER_SEC); > > + > > + *val = period / USEC_PER_SEC; > > + *val2 = period % USEC_PER_SEC; > > + > > + mutex_unlock(&chip->als_mutex); > > + ret = IIO_VAL_INT_PLUS_MICRO; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return ret; > > + > > +err: > > + mutex_unlock(&chip->als_mutex); > > + return -EINVAL; > > +} > > + > > +static int tsl2591_write_event_value(struct iio_dev *indio_dev, > > + 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 tsl2591_chip *chip = iio_priv(indio_dev); > > + int period, int_time, als_cycle; > > + > > + if (val < 0 || val2 < 0) > > + return -EINVAL; > > + > > + switch (info) { > > + case IIO_EV_INFO_VALUE: > > + if (val > TSL2591_ALS_MAX_VALUE) > > + return -EINVAL; > > + /* Lower threshold should not be higher than upper */ > > I'm guessing that's true in both directions, so if the upper is set > after the lower (and to a lower value) should fault out here as well. > > As mentioned elsewhere, I wonder if you are better off 'pushing' the other > threshold as that will look more intuitive to userspace if it is messing > with these in a silly order (particularly if the event isn't even enabled). > > > + if (dir == IIO_EV_DIR_FALLING) > > + if (val > chip->als_settings.als_upper_threshold) > > + return -EINVAL; > > + > > + switch (dir) { > > + case IIO_EV_DIR_RISING: > > + mutex_lock(&chip->als_mutex); > > + chip->als_settings.als_upper_threshold = val; > > + if (tsl2591_set_als_thresholds(chip)) > > Never eat the return code from a function and reorg this a tiny > bit and you can directly return from in here, simplifying the flow > > ret = tsl2591_set_als_thresholds(chip); > mutex_unlock(&chip->als_mutex); > return ret; > > Though see below, I think you are better off moving locking outside > of the switch statement in this case. > > > > + goto err; > > + mutex_unlock(&chip->als_mutex); > > + break; > > + case IIO_EV_DIR_FALLING: > > + mutex_lock(&chip->als_mutex); > > + chip->als_settings.als_lower_threshold = val; > > + if (tsl2591_set_als_thresholds(chip)) > > + goto err; > > + mutex_unlock(&chip->als_mutex); > > As above, don't eat error return and reorder to unlock before checking the > error code so that you can return directly. > > > + break; > > + default: > > + return -EINVAL; > > + } > > + break; > > + case IIO_EV_INFO_PERIOD: > > + mutex_lock(&chip->als_mutex); > > Given marginally fiddly lock handling, I'd factor this out as as separate > function. > > > + int_time = ALS_TIME_SECS_TO_MS(chip->als_settings.als_int_time); > > + if (!val && val2) > > + period = (val2 / MSEC_PER_SEC) / int_time; > > + else if (val && !val2) > > + period = (val * MSEC_PER_SEC) / int_time; > > No need for multiple cases. It's fine to divide 0 by whatever so final > case is fine in all paths. > > Alternative is to take the local outside of the switch statement. > All real paths you are going to hit in here will take it anyway and > if you do that the lock and unlock are more obviously correct. > > > + else if (val && val2) > > + period = ((val * MSEC_PER_SEC) + > > + (val2 / MSEC_PER_SEC)) / int_time; > > + else > > + goto err; > > + > > + als_cycle = tsl2591_persist_lit_to_cycle(period); > > + if (als_cycle < 0) > > + goto err; > > + if (tsl2591_compatible_als_persist(chip, als_cycle)) > > + goto err; > > + if (tsl2591_set_als_persist_cycle(chip)) > > + goto err; > > + mutex_unlock(&chip->als_mutex); > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +err: > > + mutex_unlock(&chip->als_mutex); > > + return -EINVAL; > return err, not -EINVAL so we get more useful specific errors. > > > +} > > + > > +static int tsl2591_read_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir) > > +{ > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + int ret; > > + > > + mutex_lock(&chip->als_mutex); > > + ret = tsl2591_get_power_state(chip); > > + mutex_unlock(&chip->als_mutex); > > + if (ret < 0) > > + return ret; > > + > > + return !!(ret & TSL2591_ENABLE_ALS_INT); > > Given you have chip->events_enabled, why not just return that > instead of reading the register? > > > +} > > + > > +static int tsl2591_write_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, > > + int state) > > +{ > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + struct i2c_client *client = chip->client; > > + > > + if (state && !chip->events_enabled) { > > + chip->events_enabled = true; > > + pm_runtime_get_sync(&client->dev); > > + } else if (!state && chip->events_enabled) { > > + chip->events_enabled = false; > > + pm_runtime_mark_last_busy(&client->dev); > > + pm_runtime_put_autosuspend(&client->dev); > > + } > > + > > + return 0; > > +} > > + > > +static const struct iio_info tsl2591_info = { > > + .attrs = &tsl2591_sysfs_attribute_group, > > + .event_attrs = &tsl2591_event_attribute_group, > > + .read_raw = tsl2591_read_raw, > > + .read_event_value = tsl2591_read_event_value, > > + .write_event_value = tsl2591_write_event_value, > > + .read_event_config = tsl2591_read_event_config, > > + .write_event_config = tsl2591_write_event_config, > > +}; > > + > > +static int __maybe_unused tsl2591_suspend(struct device *dev) > > +{ > > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + int ret; > > + > > + mutex_lock(&chip->als_mutex); > > + ret = tsl2591_set_power_state(chip, TSL2591_PWR_OFF); > > + mutex_unlock(&chip->als_mutex); > > + > > + return ret; > > +} > > + > > +static int __maybe_unused tsl2591_resume(struct device *dev) > > +{ > > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + int ret; > > + int power_state; > > + > > + if (chip->events_enabled) > > + power_state = TSL2591_PWR_ON | TSL2591_ENABLE_ALS_INT | > > + TSL2591_ENABLE_ALS; > > + else > > + power_state = TSL2591_PWR_ON | TSL2591_ENABLE_ALS; > > + > > + mutex_lock(&chip->als_mutex); > > + ret = tsl2591_set_power_state(chip, power_state); > > + mutex_unlock(&chip->als_mutex); > > + > > + return ret; > > +} > > + > > +static const struct dev_pm_ops tsl2591_pm_ops = { > > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > > + pm_runtime_force_resume) > > + SET_RUNTIME_PM_OPS(tsl2591_suspend, tsl2591_resume, NULL) > > +}; > > + > > +static irqreturn_t tsl2591_event_handler(int irq, void *private) > > +{ > > + struct iio_dev *dev_info = private; > > + struct tsl2591_chip *chip = iio_priv(dev_info); > > + struct i2c_client *client = chip->client; > > + int ret; > > + > > + if (!chip->events_enabled) > > + return IRQ_HANDLED; > > + > > + iio_push_event(dev_info, > > + IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0, > > + IIO_EV_TYPE_THRESH, > > + IIO_EV_DIR_EITHER), > > Fix alignment so all parameters align with opening bracket. > > > + iio_get_time_ns(dev_info)); > > + > > + /* Clear ALS irq */ > > + ret = i2c_smbus_write_byte(client, TSL2591_CMD_SF_CALS_NPI); > > + if (ret < 0) > > + dev_err(&client->dev, "Failed to clear als irq\n"); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int tsl2591_load_defaults(struct tsl2591_chip *chip) > > +{ > > + struct i2c_client *client = chip->client; > > + > > + chip->als_settings.als_int_time = TSL2591_DEFAULT_ALS_INT_TIME; > > + chip->als_settings.als_gain = TSL2591_DEFAULT_ALS_GAIN; > > + chip->als_settings.als_persist = TSL2591_DEFAULT_ALS_PERSIST; > > + chip->als_settings.als_lower_threshold = > > + TSL2591_DEFAULT_ALS_LOWER_THRESHOLD; > > + chip->als_settings.als_upper_threshold = > > + TSL2591_DEFAULT_ALS_UPPER_THRESHOLD; > > + > ret = tsl2591_set_als_gain_int_time(chip); > if (ret) > reutrn ret; > > That way we avoid removing information on the error that occurred which > has been passed up from lower level functions. > > Please adopt this style throughout. > > > + if (tsl2591_set_als_gain_int_time(chip)) { > > + dev_err(&client->dev, "Failed to set als gain and int time\n"); > > tsl2591_set_als_gain_int_time() has already printed an equivalent error message > in this path so no point in doing so again. > > > + return -EINVAL; > > + } > > + > > + if (tsl2591_set_als_persist_cycle(chip)) { > > + dev_err(&client->dev, "Failed to set als persist cycle\n"); > > As previous, both on message and returning error reported by lower levels. > > > + return -EINVAL; > > + } > > + > > + if (tsl2591_set_als_thresholds(chip)) { > > + dev_err(&client->dev, "Failed to set als thresholds\n"); > > + return -EINVAL; > > As above. > > > + } > > + > > + return 0; > > +} > > + > > +static void tsl2591_chip_off(void *data) > > +{ > > + struct iio_dev *indio_dev = data; > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + struct i2c_client *client = chip->client; > > + > > + pm_runtime_disable(&client->dev); > > + pm_runtime_set_suspended(&client->dev); > > + pm_runtime_put_noidle(&client->dev); > > + > > + tsl2591_set_power_state(chip, TSL2591_PWR_OFF); > > +} > > + > > +static int tsl2591_probe(struct i2c_client *client) > > +{ > > + struct tsl2591_chip *chip; > > + struct iio_dev *indio_dev; > > + int ret; > > + > > + if (!i2c_check_functionality(client->adapter, > > + I2C_FUNC_SMBUS_BYTE_DATA)) { > > + dev_err(&client->dev, > > + "I2C smbus byte data functionality is not supported\n"); > > + return -EOPNOTSUPP; > > + } > > + > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + chip = iio_priv(indio_dev); > > + chip->client = client; > > + i2c_set_clientdata(client, indio_dev); > > + > > + if (client->irq) { > > + ret = devm_request_threaded_irq(&client->dev, client->irq, > > + NULL, tsl2591_event_handler, > > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > + "tsl2591_irq", indio_dev); > > + if (ret) { > > + dev_err(&client->dev, "IRQ request error %d\n", -ret); > > + return -EINVAL; > > + } > > + } > > + > > + mutex_init(&chip->als_mutex); > > + > > + ret = i2c_smbus_read_byte_data(client, > > + TSL2591_CMD_NOP | TSL2591_DEVICE_ID); > > + if (ret < 0) { > > + dev_err(&client->dev, > > + "Failed to read the device ID register\n"); > > + return ret; > > + } > > + > > + if ((ret & TSL2591_DEVICE_ID_MASK) != TSL2591_DEVICE_ID_VAL) { > > + dev_err(&client->dev, "Device ID: %#04x unknown\n", ret); > > + return -EINVAL; > > + } > > + > > + indio_dev->info = &tsl2591_info; > > + indio_dev->channels = tsl2591_channels; > > + indio_dev->num_channels = ARRAY_SIZE(tsl2591_channels); > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->name = chip->client->name; > > + chip->events_enabled = false; > > + > > + pm_runtime_enable(&client->dev); > > + pm_runtime_set_autosuspend_delay(&client->dev, > > + TSL2591_POWER_OFF_DELAY_MS); > > + pm_runtime_use_autosuspend(&client->dev); > > + > > + if (tsl2591_load_defaults(chip)) { > > + dev_err(&client->dev, "Failed to load sensor defaults\n"); > > + return -EINVAL; > > + } > > + > > + ret = i2c_smbus_write_byte(client, TSL2591_CMD_SF_CALS_NPI); > > + if (ret < 0) { > > + dev_err(&client->dev, "Failed to clear als irq\n"); > > + return -EINVAL; > > + } > > + > > + /* Add chip off to automatically managed path */ > > + ret = devm_add_action_or_reset(&client->dev, > > + tsl2591_chip_off, > > + indio_dev); > > Some uneven indentation here. Also, try to keep things on minimum lines > possible whilst respecting limit of 80 chars (100 chars if readability > is helped by being a bit more flexible. > > ret = devm_add_action_or_reset(&client->dev, tsl2591_chip_off, > indio_dev); > > Is appropriate in this case. I'd also expect clear matching between > what happens in probe and what happens in remove. Where it is necessary > to do a little more a comment on why is useful. For example, it's not > obviously that the power is going to need disabling in the *_chip_off() > function. > > > + if (ret < 0) > > + return -EINVAL; > > + > > + return devm_iio_device_register(&client->dev, indio_dev); > > +} > > + > > +static const struct of_device_id tsl2591_of_match[] = { > > + { .compatible = "amstaos,tsl2591"}, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, tsl2591_of_match); > > + > > +static struct i2c_driver tsl2591_driver = { > > + .driver = { > > + .name = "tsl2591", > > + .pm = &tsl2591_pm_ops, > > + .of_match_table = tsl2591_of_match, > > + }, > > + .probe_new = tsl2591_probe, > > +}; > > +module_i2c_driver(tsl2591_driver); > > + > > +MODULE_AUTHOR("Joe Sandom <joe.g.sandom@xxxxxxxxx>"); > > +MODULE_DESCRIPTION("TAOS tsl2591 ambient light sensor driver"); > > +MODULE_LICENSE("GPL"); >
