* [PATCH 0/3] Add LTRF216A Driver @ 2022-03-25 10:30 Shreeya Patel 2022-03-25 10:30 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix Shreeya Patel ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Shreeya Patel @ 2022-03-25 10:30 UTC (permalink / raw) To: jic23, lars, robh+dt, Zhigang.Shi, krisman Cc: linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez, Shreeya Patel This patchset adds support for ltrf216a Ambient Light Sensor and documents the DT bindings for the same. Shreeya Patel (3): dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix dt-bindings: Document ltrf216a light sensor bindings iio: light: Add support for ltrf216a sensor .../bindings/iio/light/liteon,ltrf216a.yaml | 42 +++ .../devicetree/bindings/vendor-prefixes.yaml | 3 + drivers/iio/light/Kconfig | 10 + drivers/iio/light/Makefile | 1 + drivers/iio/light/ltrf216a.c | 334 ++++++++++++++++++ 5 files changed, 390 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml create mode 100644 drivers/iio/light/ltrf216a.c -- 2.30.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix 2022-03-25 10:30 [PATCH 0/3] Add LTRF216A Driver Shreeya Patel @ 2022-03-25 10:30 ` Shreeya Patel 2022-03-25 12:21 ` Krzysztof Kozlowski 2022-03-25 10:30 ` [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings Shreeya Patel 2022-03-25 10:30 ` [PATCH 3/3] iio: light: Add support for ltrf216a sensor Shreeya Patel 2 siblings, 1 reply; 16+ messages in thread From: Shreeya Patel @ 2022-03-25 10:30 UTC (permalink / raw) To: jic23, lars, robh+dt, Zhigang.Shi, krisman Cc: linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez, Shreeya Patel 'liteon' is the correct vendor prefix for devices released by LITE-ON Technology Corp. But one of the released device which uses ltr216a light sensor exposes the vendor prefix name as 'ltr' through ACPI. Hence, add 'ltr' as a deprecated vendor prefix which would suppress the following warning in case the compatible string used in ltrf216a driver is "ltr,ltrf216a" WARNING: DT compatible string vendor "ltr" appears un-documented -- check ./Documentation/devicetree/bindings/vendor-prefixes.yaml 364: FILE: drivers/iio/light/ltrf216a.c:313: + { .compatible = "ltr,ltrf216a" }, Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> --- Documentation/devicetree/bindings/vendor-prefixes.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index baed2b007d0e..e78091d51443 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -691,6 +691,9 @@ patternProperties: description: Linx Technologies "^liteon,.*": description: LITE-ON Technology Corp. + "^ltr,.*": + description: LITE-ON Technology Corp. + deprecated: true "^litex,.*": description: LiteX SoC builder "^lltc,.*": -- 2.30.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix 2022-03-25 10:30 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix Shreeya Patel @ 2022-03-25 12:21 ` Krzysztof Kozlowski 0 siblings, 0 replies; 16+ messages in thread From: Krzysztof Kozlowski @ 2022-03-25 12:21 UTC (permalink / raw) To: Shreeya Patel, jic23, lars, robh+dt, Zhigang.Shi, krisman Cc: linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez On 25/03/2022 11:30, Shreeya Patel wrote: > 'liteon' is the correct vendor prefix for devices released by > LITE-ON Technology Corp. But one of the released device which uses > ltr216a light sensor exposes the vendor prefix name as 'ltr' through > ACPI. > > Hence, add 'ltr' as a deprecated vendor prefix which would suppress the > following warning in case the compatible string used in ltrf216a driver > is "ltr,ltrf216a" > > WARNING: DT compatible string vendor "ltr" appears un-documented -- > check ./Documentation/devicetree/bindings/vendor-prefixes.yaml > 364: FILE: drivers/iio/light/ltrf216a.c:313: > + { .compatible = "ltr,ltrf216a" }, > > > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > --- > Documentation/devicetree/bindings/vendor-prefixes.yaml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml > index baed2b007d0e..e78091d51443 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml > @@ -691,6 +691,9 @@ patternProperties: > description: Linx Technologies > "^liteon,.*": > description: LITE-ON Technology Corp. > + "^ltr,.*": > + description: LITE-ON Technology Corp. > + deprecated: true Keep in alphabetical order. With the change: Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings 2022-03-25 10:30 [PATCH 0/3] Add LTRF216A Driver Shreeya Patel 2022-03-25 10:30 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix Shreeya Patel @ 2022-03-25 10:30 ` Shreeya Patel 2022-03-25 12:23 ` Krzysztof Kozlowski 2022-03-27 13:55 ` Jonathan Cameron 2022-03-25 10:30 ` [PATCH 3/3] iio: light: Add support for ltrf216a sensor Shreeya Patel 2 siblings, 2 replies; 16+ messages in thread From: Shreeya Patel @ 2022-03-25 10:30 UTC (permalink / raw) To: jic23, lars, robh+dt, Zhigang.Shi, krisman Cc: linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez, Shreeya Patel Add devicetree bindings for ltrf216a ambient light sensor Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> --- .../bindings/iio/light/liteon,ltrf216a.yaml | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml diff --git a/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml new file mode 100644 index 000000000000..275d86a0353a --- /dev/null +++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml @@ -0,0 +1,42 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/light/liteon,ltrf216a.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: LTRF216A Ambient Light Sensor + +maintainers: + - Zhigang Shi <Zhigang.Shi@liteon.com> + +description: | + Ambient sensing with an i2c interface. + +properties: + compatible: + enum: + - liteon,ltrf216a + - ltr,ltrf216a + + reg: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + + #address-cells = <1>; + #size-cells = <0>; + + light-sensor@53 { + compatible = "ltr,ltrf216a"; + reg = <0x53>; + }; + }; +... -- 2.30.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings 2022-03-25 10:30 ` [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings Shreeya Patel @ 2022-03-25 12:23 ` Krzysztof Kozlowski 2022-03-27 13:55 ` Jonathan Cameron 1 sibling, 0 replies; 16+ messages in thread From: Krzysztof Kozlowski @ 2022-03-25 12:23 UTC (permalink / raw) To: Shreeya Patel, jic23, lars, robh+dt, Zhigang.Shi, krisman Cc: linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez On 25/03/2022 11:30, Shreeya Patel wrote: > Add devicetree bindings for ltrf216a ambient light sensor Full stop. > > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > --- > .../bindings/iio/light/liteon,ltrf216a.yaml | 42 +++++++++++++++++++ > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml > > diff --git a/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml > new file mode 100644 > index 000000000000..275d86a0353a > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml > @@ -0,0 +1,42 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/light/liteon,ltrf216a.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: LTRF216A Ambient Light Sensor > + > +maintainers: > + - Zhigang Shi <Zhigang.Shi@liteon.com> > + > +description: | > + Ambient sensing with an i2c interface. > + > +properties: > + compatible: > + enum: > + - liteon,ltrf216a > + - ltr,ltrf216a Make it oneOf and then you can add "deprecated" to "ltr", because I assume it is deprecated since it is using deprecated vendor prefix and you keep it only because there is a real world device with it. > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + light-sensor@53 { > + compatible = "ltr,ltrf216a"; Don't use deprecated compatible/vendor prefix. > + reg = <0x53>; > + }; > + }; > +... Best regards, Krzysztof ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings 2022-03-25 10:30 ` [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings Shreeya Patel 2022-03-25 12:23 ` Krzysztof Kozlowski @ 2022-03-27 13:55 ` Jonathan Cameron 2022-03-28 2:49 ` Gabriel Krisman Bertazi 1 sibling, 1 reply; 16+ messages in thread From: Jonathan Cameron @ 2022-03-27 13:55 UTC (permalink / raw) To: Shreeya Patel Cc: lars, robh+dt, Zhigang.Shi, krisman, linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez On Fri, 25 Mar 2022 16:00:13 +0530 Shreeya Patel <shreeya.patel@collabora.com> wrote: > Add devicetree bindings for ltrf216a ambient light sensor > > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> Hi Shreeya, As we are making this Zhigang Shi's problem to maintain, I'm looking for an ack. Bit mean otherwise :) Except for the deprecated part this could just have gone in trivial-bindings.yaml. I guess you don't need it for your existing board, but best practice would probably include ensuring whatever supplies the device needs are here so that platforms that don't enable them by default can turn them on. Also, there is an interrupt according to the datasheet linked from patch 3 and that should definitely be in the binding even if the driver isn't using it. Jonathan > --- > .../bindings/iio/light/liteon,ltrf216a.yaml | 42 +++++++++++++++++++ > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml > > diff --git a/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml > new file mode 100644 > index 000000000000..275d86a0353a > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml > @@ -0,0 +1,42 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/light/liteon,ltrf216a.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: LTRF216A Ambient Light Sensor > + > +maintainers: > + - Zhigang Shi <Zhigang.Shi@liteon.com> > + > +description: | > + Ambient sensing with an i2c interface. > + > +properties: > + compatible: > + enum: > + - liteon,ltrf216a > + - ltr,ltrf216a > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + light-sensor@53 { > + compatible = "ltr,ltrf216a"; > + reg = <0x53>; > + }; > + }; > +... ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings 2022-03-27 13:55 ` Jonathan Cameron @ 2022-03-28 2:49 ` Gabriel Krisman Bertazi 0 siblings, 0 replies; 16+ messages in thread From: Gabriel Krisman Bertazi @ 2022-03-28 2:49 UTC (permalink / raw) To: Jonathan Cameron Cc: Shreeya Patel, lars, robh+dt, Zhigang.Shi, linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez Jonathan Cameron <jic23@kernel.org> writes: > On Fri, 25 Mar 2022 16:00:13 +0530 > Shreeya Patel <shreeya.patel@collabora.com> wrote: > >> Add devicetree bindings for ltrf216a ambient light sensor >> >> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > Hi Shreeya, > > As we are making this Zhigang Shi's problem to maintain, I'm > looking for an ack. Bit mean otherwise :) Alternatively, Shreeya could take over the maintainership of this schema, since she wrote it. :) > > Except for the deprecated part this could just have gone in > trivial-bindings.yaml. > > I guess you don't need it for your existing board, but best > practice would probably include ensuring whatever supplies > the device needs are here so that platforms that don't enable > them by default can turn them on. > > Also, there is an interrupt according to the datasheet linked > from patch 3 and that should definitely be in the binding > even if the driver isn't using it. > > Jonathan > > >> --- >> .../bindings/iio/light/liteon,ltrf216a.yaml | 42 +++++++++++++++++++ >> 1 file changed, 42 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml >> >> diff --git a/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml >> new file mode 100644 >> index 000000000000..275d86a0353a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml >> @@ -0,0 +1,42 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/iio/light/liteon,ltrf216a.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: LTRF216A Ambient Light Sensor >> + >> +maintainers: >> + - Zhigang Shi <Zhigang.Shi@liteon.com> >> + >> +description: | >> + Ambient sensing with an i2c interface. >> + >> +properties: >> + compatible: >> + enum: >> + - liteon,ltrf216a >> + - ltr,ltrf216a >> + >> + reg: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + i2c { >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + light-sensor@53 { >> + compatible = "ltr,ltrf216a"; >> + reg = <0x53>; >> + }; >> + }; >> +... > -- Gabriel Krisman Bertazi ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] iio: light: Add support for ltrf216a sensor 2022-03-25 10:30 [PATCH 0/3] Add LTRF216A Driver Shreeya Patel 2022-03-25 10:30 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix Shreeya Patel 2022-03-25 10:30 ` [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings Shreeya Patel @ 2022-03-25 10:30 ` Shreeya Patel 2022-03-25 12:25 ` Krzysztof Kozlowski ` (2 more replies) 2 siblings, 3 replies; 16+ messages in thread From: Shreeya Patel @ 2022-03-25 10:30 UTC (permalink / raw) To: jic23, lars, robh+dt, Zhigang.Shi, krisman Cc: linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez, Shreeya Patel From: Zhigang Shi <Zhigang.Shi@liteon.com> Add initial support for ltrf216a ambient light sensor. Datasheet :- https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf Co-developed-by: Shreeya Patel <shreeya.patel@collabora.com> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com> --- drivers/iio/light/Kconfig | 10 ++ drivers/iio/light/Makefile | 1 + drivers/iio/light/ltrf216a.c | 334 +++++++++++++++++++++++++++++++++++ 3 files changed, 345 insertions(+) create mode 100644 drivers/iio/light/ltrf216a.c diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index a62c7b4b8678..08fa383a8ca7 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -318,6 +318,16 @@ config SENSORS_LM3533 changes. The ALS-control output values can be set per zone for the three current output channels. +config LTRF216A + tristate "Liteon LTRF216A Light Sensor" + depends on I2C + help + If you say Y or M here, you get support for Liteon LTRF216A + Ambient Light Sensor. + + If built as a dynamically linked module, it will be called + ltrf216a. + config LTR501 tristate "LTR-501ALS-01 light sensor" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index d10912faf964..8fa91b9fe5b6 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o obj-$(CONFIG_ISL29125) += isl29125.o obj-$(CONFIG_JSA1212) += jsa1212.o obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o +obj-$(CONFIG_LTRF216A) += ltrf216a.o obj-$(CONFIG_LTR501) += ltr501.o obj-$(CONFIG_LV0104CS) += lv0104cs.o obj-$(CONFIG_MAX44000) += max44000.o diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c new file mode 100644 index 000000000000..99295358a7fe --- /dev/null +++ b/drivers/iio/light/ltrf216a.c @@ -0,0 +1,334 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * LTRF216A Ambient Light Sensor + * + * Copyright (C) 2021 Lite-On Technology Corp (Singapore) + * Author: Shi Zhigang <Zhigang.Shi@liteon.com> + * + * IIO driver for LTRF216A (7-bit I2C slave address 0x53). + */ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/i2c.h> +#include <linux/mutex.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/pm.h> +#include <linux/delay.h> + +#define LTRF216A_DRV_NAME "ltrf216a" + +#define LTRF216A_MAIN_CTRL 0x00 + +#define LTRF216A_ALS_MEAS_RATE 0x04 +#define LTRF216A_MAIN_STATUS 0x07 +#define LTRF216A_CLEAR_DATA_0 0x0A + +#define LTRF216A_ALS_DATA_0 0x0D + +static const int int_time_mapping[] = { 400000, 200000, 100000 }; + +struct ltrf216a_data { + struct i2c_client *client; + u32 int_time; + u8 int_time_fac; + u8 als_gain_fac; + struct mutex mutex; +}; + +/* open air. need to update based on TP transmission rate. */ +#define WIN_FAC 1 + +static const struct iio_chan_spec ltrf216a_channels[] = { + { + .type = IIO_LIGHT, + .info_mask_separate = + BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_INT_TIME), + } +}; + +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.1 0.2 0.4"); + +static struct attribute *ltrf216a_attributes[] = { + &iio_const_attr_integration_time_available.dev_attr.attr, + NULL +}; + +static const struct attribute_group ltrf216a_attribute_group = { + .attrs = ltrf216a_attributes, +}; + +static int ltrf216a_init(struct iio_dev *indio_dev) +{ + int ret; + struct ltrf216a_data *data = iio_priv(indio_dev); + + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_CTRL); + if (ret < 0) { + dev_err(&data->client->dev, "Error reading LTRF216A_MAIN_CTRL\n"); + return ret; + } + + /* enable sensor */ + ret |= 0x02; + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret); + if (ret < 0) { + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); + return ret; + } + + return 0; +} + +static int ltrf216a_disable(struct iio_dev *indio_dev) +{ + int ret; + struct ltrf216a_data *data = iio_priv(indio_dev); + + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0); + if (ret < 0) + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); + + return ret; +} + +static int ltrf216a_set_it_time(struct ltrf216a_data *data, int itime) +{ + int i, ret, index = -1; + u8 reg; + + for (i = 0; i < ARRAY_SIZE(int_time_mapping); i++) { + if (int_time_mapping[i] == itime) { + index = i; + break; + } + } + /* Make sure integration time index is valid */ + if (index < 0) + return -EINVAL; + + if (index == 0) { + reg = 0x03; + data->int_time_fac = 4; + } else if (index == 1) { + reg = 0x13; + data->int_time_fac = 2; + } else { + reg = (index << 4) | 0x02; + data->int_time_fac = 1; + } + + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RATE, reg); + if (ret < 0) + return ret; + + data->int_time = itime; + + return 0; +} + +static int ltrf216a_get_it_time(struct ltrf216a_data *data, int *val, int *val2) +{ + *val = 0; + *val2 = data->int_time; + + return IIO_VAL_INT_PLUS_MICRO; +} + +static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr) +{ + int ret; + int tries = 25; + int val_0, val_1, val_2; + + while (tries--) { + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS); + if (ret < 0) + return ret; + if (ret & 0x08) + break; + msleep(20); + } + + val_0 = i2c_smbus_read_byte_data(data->client, addr); + val_1 = i2c_smbus_read_byte_data(data->client, addr + 1); + val_2 = i2c_smbus_read_byte_data(data->client, addr + 2); + ret = (val_2 << 16) + (val_1 << 8) + val_0; + + return ret; +} + +static int ltrf216a_get_lux(struct ltrf216a_data *data) +{ + int greendata, cleardata, lux; + + greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0); + cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0); + + if (greendata < 0 || cleardata < 0) + lux = 0; + else + lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10; + + return lux; +} + +static int ltrf216a_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + int ret; + struct ltrf216a_data *data = iio_priv(indio_dev); + + mutex_lock(&data->mutex); + + switch (mask) { + case IIO_CHAN_INFO_PROCESSED: + ret = ltrf216a_get_lux(data); + *val = ret; + ret = IIO_VAL_INT; + break; + case IIO_CHAN_INFO_INT_TIME: + ret = ltrf216a_get_it_time(data, val, val2); + break; + default: + ret = -EINVAL; + } + + mutex_unlock(&data->mutex); + + return ret; +} + +static int ltrf216a_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long mask) +{ + struct ltrf216a_data *data = iio_priv(indio_dev); + int ret; + + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + if (val != 0) + return -EINVAL; + mutex_lock(&data->mutex); + ret = ltrf216a_set_it_time(data, val2); + mutex_unlock(&data->mutex); + return ret; + default: + return -EINVAL; + } +} + +static const struct iio_info ltrf216a_info = { + .read_raw = ltrf216a_read_raw, + .write_raw = ltrf216a_write_raw, + .attrs = <rf216a_attribute_group, +}; + +static int ltrf216a_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct ltrf216a_data *data; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); + if (!indio_dev) + return -ENOMEM; + + data = iio_priv(indio_dev); + i2c_set_clientdata(client, indio_dev); + data->client = client; + + mutex_init(&data->mutex); + + indio_dev->info = <rf216a_info; + indio_dev->name = LTRF216A_DRV_NAME; + indio_dev->channels = ltrf216a_channels; + indio_dev->num_channels = ARRAY_SIZE(ltrf216a_channels); + indio_dev->modes = INDIO_DIRECT_MODE; + + ret = ltrf216a_init(indio_dev); + if (ret < 0) { + dev_err(&client->dev, "ltrf216a chip init failed\n"); + return ret; + } + data->int_time = 100000; + data->int_time_fac = 1; + data->als_gain_fac = 3; + + ret = iio_device_register(indio_dev); + if (ret < 0) { + dev_err(&client->dev, "failed to register iio dev\n"); + goto err_init; + } + + return 0; +err_init: + ltrf216a_disable(indio_dev); + return ret; +} + +static int ltrf216a_remove(struct i2c_client *client) +{ + struct iio_dev *indio_dev = i2c_get_clientdata(client); + + iio_device_unregister(indio_dev); + ltrf216a_disable(indio_dev); + + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int ltrf216a_suspend(struct device *dev) +{ + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); + + return ltrf216a_disable(indio_dev); +} + +static int ltrf216a_resume(struct device *dev) +{ + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); + + return ltrf216a_init(indio_dev); +} + +static SIMPLE_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_suspend, ltrf216a_resume); +#define LTRF216A_PM_OPS (<rf216a_pm_ops) +#else +#define LTRF216A_PM_OPS NULL +#endif + +static const struct i2c_device_id ltrf216a_id[] = { + { LTRF216A_DRV_NAME, 0}, + {} +}; +MODULE_DEVICE_TABLE(i2c, ltrf216a_id); + +static const struct of_device_id ltrf216a_of_match[] = { + { .compatible = "ltr,ltrf216a", }, + { .compatible = "liteon,ltrf216a", }, + {} +}; +MODULE_DEVICE_TABLE(of, ltrf216a_of_match); + +static struct i2c_driver ltrf216a_driver = { + .driver = { + .name = LTRF216A_DRV_NAME, + .pm = LTRF216A_PM_OPS, + .of_match_table = ltrf216a_of_match, + }, + .probe = ltrf216a_probe, + .remove = ltrf216a_remove, + .id_table = ltrf216a_id, +}; + +module_i2c_driver(ltrf216a_driver); + +MODULE_AUTHOR("Shi Zhigang <Zhigang.Shi@liteon.com>"); +MODULE_DESCRIPTION("LTRF216A ambient light sensor driver"); +MODULE_LICENSE("GPL v2"); -- 2.30.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor 2022-03-25 10:30 ` [PATCH 3/3] iio: light: Add support for ltrf216a sensor Shreeya Patel @ 2022-03-25 12:25 ` Krzysztof Kozlowski 2022-03-27 14:30 ` Jonathan Cameron 2022-03-28 3:59 ` Gabriel Krisman Bertazi 2 siblings, 0 replies; 16+ messages in thread From: Krzysztof Kozlowski @ 2022-03-25 12:25 UTC (permalink / raw) To: Shreeya Patel, jic23, lars, robh+dt, Zhigang.Shi, krisman Cc: linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez On 25/03/2022 11:30, Shreeya Patel wrote: > From: Zhigang Shi <Zhigang.Shi@liteon.com> > > Add initial support for ltrf216a ambient light sensor. > > Datasheet :- > https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf > > > Co-developed-by: Shreeya Patel <shreeya.patel@collabora.com> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com> > --- > drivers/iio/light/Kconfig | 10 ++ > drivers/iio/light/Makefile | 1 + > drivers/iio/light/ltrf216a.c | 334 +++++++++++++++++++++++++++++++++++ > 3 files changed, 345 insertions(+) > create mode 100644 drivers/iio/light/ltrf216a.c > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > index a62c7b4b8678..08fa383a8ca7 100644 > --- a/drivers/iio/light/Kconfig > +++ b/drivers/iio/light/Kconfig > @@ -318,6 +318,16 @@ config SENSORS_LM3533 > changes. The ALS-control output values can be set per zone for the > three current output channels. > > +config LTRF216A > + tristate "Liteon LTRF216A Light Sensor" > + depends on I2C > + help > + If you say Y or M here, you get support for Liteon LTRF216A > + Ambient Light Sensor. > + > + If built as a dynamically linked module, it will be called > + ltrf216a. > + > config LTR501 > tristate "LTR-501ALS-01 light sensor" > depends on I2C > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile > index d10912faf964..8fa91b9fe5b6 100644 > --- a/drivers/iio/light/Makefile > +++ b/drivers/iio/light/Makefile > @@ -30,6 +30,7 @@ obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o > obj-$(CONFIG_ISL29125) += isl29125.o > obj-$(CONFIG_JSA1212) += jsa1212.o > obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o > +obj-$(CONFIG_LTRF216A) += ltrf216a.o > obj-$(CONFIG_LTR501) += ltr501.o > obj-$(CONFIG_LV0104CS) += lv0104cs.o > obj-$(CONFIG_MAX44000) += max44000.o > diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c > new file mode 100644 > index 000000000000..99295358a7fe > --- /dev/null > +++ b/drivers/iio/light/ltrf216a.c > @@ -0,0 +1,334 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * LTRF216A Ambient Light Sensor > + * > + * Copyright (C) 2021 Lite-On Technology Corp (Singapore) > + * Author: Shi Zhigang <Zhigang.Shi@liteon.com> > + * > + * IIO driver for LTRF216A (7-bit I2C slave address 0x53). > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/i2c.h> > +#include <linux/mutex.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/pm.h> > +#include <linux/delay.h> > + > +#define LTRF216A_DRV_NAME "ltrf216a" > + > +#define LTRF216A_MAIN_CTRL 0x00 > + > +#define LTRF216A_ALS_MEAS_RATE 0x04 > +#define LTRF216A_MAIN_STATUS 0x07 > +#define LTRF216A_CLEAR_DATA_0 0x0A > + > +#define LTRF216A_ALS_DATA_0 0x0D > + > +static const int int_time_mapping[] = { 400000, 200000, 100000 }; > + > +struct ltrf216a_data { > + struct i2c_client *client; > + u32 int_time; > + u8 int_time_fac; > + u8 als_gain_fac; > + struct mutex mutex; > +}; > + > +/* open air. need to update based on TP transmission rate. */ > +#define WIN_FAC 1 > + > +static const struct iio_chan_spec ltrf216a_channels[] = { > + { > + .type = IIO_LIGHT, > + .info_mask_separate = > + BIT(IIO_CHAN_INFO_PROCESSED) | > + BIT(IIO_CHAN_INFO_INT_TIME), > + } > +}; > + > +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.1 0.2 0.4"); > + > +static struct attribute *ltrf216a_attributes[] = { > + &iio_const_attr_integration_time_available.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group ltrf216a_attribute_group = { > + .attrs = ltrf216a_attributes, > +}; > + > +static int ltrf216a_init(struct iio_dev *indio_dev) > +{ > + int ret; > + struct ltrf216a_data *data = iio_priv(indio_dev); > + > + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_CTRL); > + if (ret < 0) { > + dev_err(&data->client->dev, "Error reading LTRF216A_MAIN_CTRL\n"); > + return ret; > + } > + > + /* enable sensor */ > + ret |= 0x02; > + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret); > + if (ret < 0) { > + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int ltrf216a_disable(struct iio_dev *indio_dev) > +{ > + int ret; > + struct ltrf216a_data *data = iio_priv(indio_dev); > + > + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0); > + if (ret < 0) > + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); > + > + return ret; > +} > + > +static int ltrf216a_set_it_time(struct ltrf216a_data *data, int itime) > +{ > + int i, ret, index = -1; > + u8 reg; > + > + for (i = 0; i < ARRAY_SIZE(int_time_mapping); i++) { > + if (int_time_mapping[i] == itime) { > + index = i; > + break; > + } > + } > + /* Make sure integration time index is valid */ > + if (index < 0) > + return -EINVAL; > + > + if (index == 0) { > + reg = 0x03; > + data->int_time_fac = 4; > + } else if (index == 1) { > + reg = 0x13; > + data->int_time_fac = 2; > + } else { > + reg = (index << 4) | 0x02; > + data->int_time_fac = 1; > + } > + > + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RATE, reg); > + if (ret < 0) > + return ret; > + > + data->int_time = itime; > + > + return 0; > +} > + > +static int ltrf216a_get_it_time(struct ltrf216a_data *data, int *val, int *val2) > +{ > + *val = 0; > + *val2 = data->int_time; > + > + return IIO_VAL_INT_PLUS_MICRO; > +} > + > +static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr) > +{ > + int ret; > + int tries = 25; > + int val_0, val_1, val_2; > + > + while (tries--) { > + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS); > + if (ret < 0) > + return ret; > + if (ret & 0x08) > + break; > + msleep(20); > + } > + > + val_0 = i2c_smbus_read_byte_data(data->client, addr); > + val_1 = i2c_smbus_read_byte_data(data->client, addr + 1); > + val_2 = i2c_smbus_read_byte_data(data->client, addr + 2); > + ret = (val_2 << 16) + (val_1 << 8) + val_0; > + > + return ret; > +} > + > +static int ltrf216a_get_lux(struct ltrf216a_data *data) > +{ > + int greendata, cleardata, lux; > + > + greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0); > + cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0); > + > + if (greendata < 0 || cleardata < 0) > + lux = 0; > + else > + lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10; > + > + return lux; > +} > + > +static int ltrf216a_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + int ret; > + struct ltrf216a_data *data = iio_priv(indio_dev); > + > + mutex_lock(&data->mutex); > + > + switch (mask) { > + case IIO_CHAN_INFO_PROCESSED: > + ret = ltrf216a_get_lux(data); > + *val = ret; > + ret = IIO_VAL_INT; > + break; > + case IIO_CHAN_INFO_INT_TIME: > + ret = ltrf216a_get_it_time(data, val, val2); > + break; > + default: > + ret = -EINVAL; > + } > + > + mutex_unlock(&data->mutex); > + > + return ret; > +} > + > +static int ltrf216a_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, > + int val2, long mask) > +{ > + struct ltrf216a_data *data = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_INT_TIME: > + if (val != 0) > + return -EINVAL; > + mutex_lock(&data->mutex); > + ret = ltrf216a_set_it_time(data, val2); > + mutex_unlock(&data->mutex); > + return ret; > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info ltrf216a_info = { > + .read_raw = ltrf216a_read_raw, > + .write_raw = ltrf216a_write_raw, > + .attrs = <rf216a_attribute_group, > +}; > + > +static int ltrf216a_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct ltrf216a_data *data; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + i2c_set_clientdata(client, indio_dev); > + data->client = client; > + > + mutex_init(&data->mutex); > + > + indio_dev->info = <rf216a_info; > + indio_dev->name = LTRF216A_DRV_NAME; > + indio_dev->channels = ltrf216a_channels; > + indio_dev->num_channels = ARRAY_SIZE(ltrf216a_channels); > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + ret = ltrf216a_init(indio_dev); > + if (ret < 0) { > + dev_err(&client->dev, "ltrf216a chip init failed\n"); > + return ret; > + } > + data->int_time = 100000; > + data->int_time_fac = 1; > + data->als_gain_fac = 3; > + > + ret = iio_device_register(indio_dev); Use devm- function, assuming no issues with removal-steps (disable will be before iio unregister). > + if (ret < 0) { > + dev_err(&client->dev, "failed to register iio dev\n"); > + goto err_init; > + } > + > + return 0; Blank line. > +err_init: > + ltrf216a_disable(indio_dev); > + return ret; > +} > + Best regards, Krzysztof ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor 2022-03-25 10:30 ` [PATCH 3/3] iio: light: Add support for ltrf216a sensor Shreeya Patel 2022-03-25 12:25 ` Krzysztof Kozlowski @ 2022-03-27 14:30 ` Jonathan Cameron 2022-03-29 20:03 ` Shreeya Patel 2022-04-11 17:06 ` Shreeya Patel 2022-03-28 3:59 ` Gabriel Krisman Bertazi 2 siblings, 2 replies; 16+ messages in thread From: Jonathan Cameron @ 2022-03-27 14:30 UTC (permalink / raw) To: Shreeya Patel Cc: lars, robh+dt, Zhigang.Shi, krisman, linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez On Fri, 25 Mar 2022 16:00:14 +0530 Shreeya Patel <shreeya.patel@collabora.com> wrote: Hi Zhigang, Shreeya, Comments inline. Thanks, Jonathan > From: Zhigang Shi <Zhigang.Shi@liteon.com> > > Add initial support for ltrf216a ambient light sensor. > > Datasheet :- > https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf We now have a Datasheet tag, so make this part of the tag block so automated tools can find it easily: > > Datasheet: https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf > Co-developed-by: Shreeya Patel <shreeya.patel@collabora.com> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com> > --- > drivers/iio/light/Kconfig | 10 ++ > drivers/iio/light/Makefile | 1 + > drivers/iio/light/ltrf216a.c | 334 +++++++++++++++++++++++++++++++++++ > 3 files changed, 345 insertions(+) > create mode 100644 drivers/iio/light/ltrf216a.c > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > index a62c7b4b8678..08fa383a8ca7 100644 > --- a/drivers/iio/light/Kconfig > +++ b/drivers/iio/light/Kconfig > @@ -318,6 +318,16 @@ config SENSORS_LM3533 > changes. The ALS-control output values can be set per zone for the > three current output channels. > > +config LTRF216A > + tristate "Liteon LTRF216A Light Sensor" > + depends on I2C > + help > + If you say Y or M here, you get support for Liteon LTRF216A > + Ambient Light Sensor. > + > + If built as a dynamically linked module, it will be called > + ltrf216a. > + > config LTR501 > tristate "LTR-501ALS-01 light sensor" > depends on I2C > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile > index d10912faf964..8fa91b9fe5b6 100644 > --- a/drivers/iio/light/Makefile > +++ b/drivers/iio/light/Makefile > @@ -30,6 +30,7 @@ obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o > obj-$(CONFIG_ISL29125) += isl29125.o > obj-$(CONFIG_JSA1212) += jsa1212.o > obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o > +obj-$(CONFIG_LTRF216A) += ltrf216a.o > obj-$(CONFIG_LTR501) += ltr501.o > obj-$(CONFIG_LV0104CS) += lv0104cs.o > obj-$(CONFIG_MAX44000) += max44000.o > diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c > new file mode 100644 > index 000000000000..99295358a7fe > --- /dev/null > +++ b/drivers/iio/light/ltrf216a.c > @@ -0,0 +1,334 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * LTRF216A Ambient Light Sensor > + * > + * Copyright (C) 2021 Lite-On Technology Corp (Singapore) > + * Author: Shi Zhigang <Zhigang.Shi@liteon.com> > + * > + * IIO driver for LTRF216A (7-bit I2C slave address 0x53). > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/i2c.h> > +#include <linux/mutex.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> mod_devicetable.h for the id tables > +#include <linux/pm.h> > +#include <linux/delay.h> > + > +#define LTRF216A_DRV_NAME "ltrf216a" > + > +#define LTRF216A_MAIN_CTRL 0x00 > + > +#define LTRF216A_ALS_MEAS_RATE 0x04 MEAS_RES seems more appropriate from datasheet. As mentioned below, also add defines for all the fields of the registers you will access and where the fields are obvious numerical things, add defines for the values they can take. > +#define LTRF216A_MAIN_STATUS 0x07 > +#define LTRF216A_CLEAR_DATA_0 0x0A > + > +#define LTRF216A_ALS_DATA_0 0x0D > + > +static const int int_time_mapping[] = { 400000, 200000, 100000 }; > + > +struct ltrf216a_data { > + struct i2c_client *client; > + u32 int_time; > + u8 int_time_fac; > + u8 als_gain_fac; > + struct mutex mutex; All locks need a comment to say exactly what they are protecting. > +}; > + > +/* open air. need to update based on TP transmission rate. */ > +#define WIN_FAC 1 > + > +static const struct iio_chan_spec ltrf216a_channels[] = { > + { > + .type = IIO_LIGHT, > + .info_mask_separate = > + BIT(IIO_CHAN_INFO_PROCESSED) | > + BIT(IIO_CHAN_INFO_INT_TIME), > + } > +}; > + > +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.1 0.2 0.4"); > + > +static struct attribute *ltrf216a_attributes[] = { > + &iio_const_attr_integration_time_available.dev_attr.attr, please use the read_avail callback and set the appropriate _available bit. That allows in kernel access to this information + is probably shorter in this case as you won't have an attribute group etc to deal wtih. > + NULL > +}; > + > +static const struct attribute_group ltrf216a_attribute_group = { > + .attrs = ltrf216a_attributes, > +}; > + > +static int ltrf216a_init(struct iio_dev *indio_dev) > +{ > + int ret; > + struct ltrf216a_data *data = iio_priv(indio_dev); > + > + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_CTRL); > + if (ret < 0) { > + dev_err(&data->client->dev, "Error reading LTRF216A_MAIN_CTRL\n"); > + return ret; > + } > + > + /* enable sensor */ > + ret |= 0x02; Needs a #define and preferably use ret |= FIELD_PREP()... > + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret); > + if (ret < 0) { > + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int ltrf216a_disable(struct iio_dev *indio_dev) > +{ > + int ret; > + struct ltrf216a_data *data = iio_priv(indio_dev); > + > + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0); > + if (ret < 0) > + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); > + > + return ret; > +} > + > +static int ltrf216a_set_it_time(struct ltrf216a_data *data, int itime) > +{ > + int i, ret, index = -1; > + u8 reg; > + > + for (i = 0; i < ARRAY_SIZE(int_time_mapping); i++) { > + if (int_time_mapping[i] == itime) { > + index = i; > + break; > + } > + } > + /* Make sure integration time index is valid */ > + if (index < 0) > + return -EINVAL; > + > + if (index == 0) { Switch statement seems more appropriate than this stack of if else > + reg = 0x03; reg isn't a great name as I assume this is the value, not the address which was my first thought... Perhaps reg_val? > + data->int_time_fac = 4; > + } else if (index == 1) { > + reg = 0x13; > + data->int_time_fac = 2; > + } else { > + reg = (index << 4) | 0x02; Unless I'm missing something index == 2 if we get here. So why the calculation? I'd suggest defining the two fields and using FIELD_PREP() to set up each part probably to one of a set of #define LTRF216A_ALS_MEAS_RATE_ > + data->int_time_fac = 1; > + } > + > + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RATE, reg); Called MEAS_RES on the datasheet, why this name for the register? > + if (ret < 0) > + return ret; > + > + data->int_time = itime; > + > + return 0; > +} > + > +static int ltrf216a_get_it_time(struct ltrf216a_data *data, int *val, int *val2) > +{ > + *val = 0; > + *val2 = data->int_time; I'd put this inline as it avoids a question I had at the call site on why you passed *val in as it would always be 0. > + > + return IIO_VAL_INT_PLUS_MICRO; > +} > + > +static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr) > +{ > + int ret; > + int tries = 25; > + int val_0, val_1, val_2; > + > + while (tries--) { > + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS); > + if (ret < 0) > + return ret; > + if (ret & 0x08) That 0x08 is a magic number and also better defined using BIT(3) Anyhow, use a define for that. > + break; > + msleep(20); > + } > + > + val_0 = i2c_smbus_read_byte_data(data->client, addr); All of these can return errors so you should check. Device doesn't support any larger reads? > + val_1 = i2c_smbus_read_byte_data(data->client, addr + 1); > + val_2 = i2c_smbus_read_byte_data(data->client, addr + 2); > + ret = (val_2 << 16) + (val_1 << 8) + val_0; This is a le24_to_cpu() conversion. Preferred choice would be to use something like u8 buf[3]; int i; for (i = 0; i < 3; i++) { ret = i2c_smbus_read_byte_data(data->client, addr); if (ret < 0) return ret; buf[i] = ret; } return le24_to_cpu(buf); > + > + return ret; > +} > + > +static int ltrf216a_get_lux(struct ltrf216a_data *data) > +{ > + int greendata, cleardata, lux; > + > + greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0); > + cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0); > + > + if (greendata < 0 || cleardata < 0) I'd rather see the error passed all the way up than eaten here. > + lux = 0; > + else > + lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10; This feels like it would be better reported as IIO_VAL_FRACTIONAL; Won't make any difference if the only user is sysfs, but if anyone ever wants to use the in kernel access to this device they will get access to better precision. Also possible the core handling for IIO_VAL_FRACTIONAL will do a better job on retaining precision. > + > + return lux; > +} > + > +static int ltrf216a_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + int ret; > + struct ltrf216a_data *data = iio_priv(indio_dev); > + > + mutex_lock(&data->mutex); > + > + switch (mask) { > + case IIO_CHAN_INFO_PROCESSED: > + ret = ltrf216a_get_lux(data); > + *val = ret; > + ret = IIO_VAL_INT; > + break; > + case IIO_CHAN_INFO_INT_TIME: > + ret = ltrf216a_get_it_time(data, val, val2); > + break; > + default: > + ret = -EINVAL; > + } > + > + mutex_unlock(&data->mutex); > + > + return ret; > +} > + > +static int ltrf216a_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, > + int val2, long mask) > +{ > + struct ltrf216a_data *data = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_INT_TIME: > + if (val != 0) > + return -EINVAL; > + mutex_lock(&data->mutex); > + ret = ltrf216a_set_it_time(data, val2); > + mutex_unlock(&data->mutex); > + return ret; > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info ltrf216a_info = { > + .read_raw = ltrf216a_read_raw, > + .write_raw = ltrf216a_write_raw, > + .attrs = <rf216a_attribute_group, > +}; > + > +static int ltrf216a_probe(struct i2c_client *client, > + const struct i2c_device_id *id) Whilst it's taking a long time to clean out the old approach, probe_new() callback preferred for new i2c drivers. > +{ > + struct ltrf216a_data *data; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + i2c_set_clientdata(client, indio_dev); > + data->client = client; > + > + mutex_init(&data->mutex); > + > + indio_dev->info = <rf216a_info; > + indio_dev->name = LTRF216A_DRV_NAME; > + indio_dev->channels = ltrf216a_channels; > + indio_dev->num_channels = ARRAY_SIZE(ltrf216a_channels); > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + ret = ltrf216a_init(indio_dev); > + if (ret < 0) { > + dev_err(&client->dev, "ltrf216a chip init failed\n"); > + return ret; As below, slightly preference for dev_err_probe(). It doesn't really matter when deferral isn't a possibility but using that function ends up slightly neater. > + } blank line here. Adding separation so we have "function call error handler next thing.." Just makes things a tiny bit easier to read quickly. > + data->int_time = 100000; > + data->int_time_fac = 1; > + data->als_gain_fac = 3; > + > + ret = iio_device_register(indio_dev); > + if (ret < 0) { > + dev_err(&client->dev, "failed to register iio dev\n"); Once using devm, this can be return dev_err_probe(&client->dev, ret, "failed to register iio dev\n"); Which is neater but only use this function to print errors for things that happen in probe. > + goto err_init; > + } > + > + return 0; > +err_init: > + ltrf216a_disable(indio_dev); > + return ret; > +} > + > +static int ltrf216a_remove(struct i2c_client *client) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > + > + iio_device_unregister(indio_dev); > + ltrf216a_disable(indio_dev); As Kryzstof mentioned, use devm versions and where they don't exist you can use devm_add_action_or_reset() (e.g. for this disable) That way we don't need error handling or a remove function. > + > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int ltrf216a_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + > + return ltrf216a_disable(indio_dev); > +} > + > +static int ltrf216a_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + > + return ltrf216a_init(indio_dev); > +} > + > +static SIMPLE_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_suspend, ltrf216a_resume); > +#define LTRF216A_PM_OPS (<rf216a_pm_ops) > +#else > +#define LTRF216A_PM_OPS NULL > +#endif Recently some 'magic' macros were added to pm.h that allow you to just specify this stuff without guards static DEFINE_SIMPLE_PM_OPS(ltrf216a_pm_ops, lt..._) and use .pm = pm_sleep_ptr(<rf216a_pm_ops), below and then the compiler can see enough to remove the unused code if CONFIG_PM_SLEEP isn't defined without the need for messy __maybe_unused etc. We are part way through driving this change through IIO and less of the way along with the rest of the kernel, but it's a nice cleanup so please apply it to new code like this. > + > +static const struct i2c_device_id ltrf216a_id[] = { > + { LTRF216A_DRV_NAME, 0}, Space after 0 > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, ltrf216a_id); > + > +static const struct of_device_id ltrf216a_of_match[] = { > + { .compatible = "ltr,ltrf216a", }, > + { .compatible = "liteon,ltrf216a", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, ltrf216a_of_match); > + > +static struct i2c_driver ltrf216a_driver = { > + .driver = { > + .name = LTRF216A_DRV_NAME, > + .pm = LTRF216A_PM_OPS, > + .of_match_table = ltrf216a_of_match, > + }, > + .probe = ltrf216a_probe, > + .remove = ltrf216a_remove, > + .id_table = ltrf216a_id, > +}; > + > +module_i2c_driver(ltrf216a_driver); > + > +MODULE_AUTHOR("Shi Zhigang <Zhigang.Shi@liteon.com>"); > +MODULE_DESCRIPTION("LTRF216A ambient light sensor driver"); > +MODULE_LICENSE("GPL v2"); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor 2022-03-27 14:30 ` Jonathan Cameron @ 2022-03-29 20:03 ` Shreeya Patel 2022-04-02 16:49 ` Jonathan Cameron 2022-04-11 17:06 ` Shreeya Patel 1 sibling, 1 reply; 16+ messages in thread From: Shreeya Patel @ 2022-03-29 20:03 UTC (permalink / raw) To: Jonathan Cameron Cc: lars, robh+dt, Zhigang.Shi, krisman, linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez On 27/03/22 20:00, Jonathan Cameron wrote: Hi Jonathan, Thanks for your detailed review. I am working on v2 with the modifications suggested by you. Just one comment inline. > On Fri, 25 Mar 2022 16:00:14 +0530 > Shreeya Patel <shreeya.patel@collabora.com> wrote: > > Hi Zhigang, Shreeya, > > Comments inline. > > Thanks, > > Jonathan >> From: Zhigang Shi <Zhigang.Shi@liteon.com> >> >> Add initial support for ltrf216a ambient light sensor. >> >> Datasheet :- >> https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf > We now have a Datasheet tag, so make this part of the tag block so automated > tools can find it easily: >> > Datasheet: https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf >> Co-developed-by: Shreeya Patel <shreeya.patel@collabora.com> >> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> >> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com> >> --- >> drivers/iio/light/Kconfig | 10 ++ >> drivers/iio/light/Makefile | 1 + >> drivers/iio/light/ltrf216a.c | 334 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 345 insertions(+) >> create mode 100644 drivers/iio/light/ltrf216a.c >> >> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig >> index a62c7b4b8678..08fa383a8ca7 100644 >> --- a/drivers/iio/light/Kconfig >> +++ b/drivers/iio/light/Kconfig >> @@ -318,6 +318,16 @@ config SENSORS_LM3533 >> changes. The ALS-control output values can be set per zone for the >> three current output channels. >> >> +config LTRF216A >> + tristate "Liteon LTRF216A Light Sensor" >> + depends on I2C >> + help >> + If you say Y or M here, you get support for Liteon LTRF216A >> + Ambient Light Sensor. >> + >> + If built as a dynamically linked module, it will be called >> + ltrf216a. >> + >> config LTR501 >> tristate "LTR-501ALS-01 light sensor" >> depends on I2C >> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile >> index d10912faf964..8fa91b9fe5b6 100644 >> --- a/drivers/iio/light/Makefile >> +++ b/drivers/iio/light/Makefile >> @@ -30,6 +30,7 @@ obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o >> obj-$(CONFIG_ISL29125) += isl29125.o >> obj-$(CONFIG_JSA1212) += jsa1212.o >> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o >> +obj-$(CONFIG_LTRF216A) += ltrf216a.o >> obj-$(CONFIG_LTR501) += ltr501.o >> obj-$(CONFIG_LV0104CS) += lv0104cs.o >> obj-$(CONFIG_MAX44000) += max44000.o >> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c >> new file mode 100644 >> index 000000000000..99295358a7fe >> --- /dev/null >> +++ b/drivers/iio/light/ltrf216a.c >> @@ -0,0 +1,334 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * LTRF216A Ambient Light Sensor >> + * >> + * Copyright (C) 2021 Lite-On Technology Corp (Singapore) >> + * Author: Shi Zhigang <Zhigang.Shi@liteon.com> >> + * >> + * IIO driver for LTRF216A (7-bit I2C slave address 0x53). >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/i2c.h> >> +#include <linux/mutex.h> >> +#include <linux/iio/iio.h> >> +#include <linux/iio/sysfs.h> > mod_devicetable.h for the id tables > >> +#include <linux/pm.h> >> +#include <linux/delay.h> >> + >> +#define LTRF216A_DRV_NAME "ltrf216a" >> + >> +#define LTRF216A_MAIN_CTRL 0x00 >> + >> +#define LTRF216A_ALS_MEAS_RATE 0x04 > MEAS_RES seems more appropriate from datasheet. > As mentioned below, also add defines for all the fields > of the registers you will access and where the fields are > obvious numerical things, add defines for the values they > can take. > > >> +#define LTRF216A_MAIN_STATUS 0x07 >> +#define LTRF216A_CLEAR_DATA_0 0x0A >> + >> +#define LTRF216A_ALS_DATA_0 0x0D >> + >> +static const int int_time_mapping[] = { 400000, 200000, 100000 }; >> + >> +struct ltrf216a_data { >> + struct i2c_client *client; >> + u32 int_time; >> + u8 int_time_fac; >> + u8 als_gain_fac; >> + struct mutex mutex; > All locks need a comment to say exactly what they are protecting. > >> +}; >> + >> +/* open air. need to update based on TP transmission rate. */ >> +#define WIN_FAC 1 >> + >> +static const struct iio_chan_spec ltrf216a_channels[] = { >> + { >> + .type = IIO_LIGHT, >> + .info_mask_separate = >> + BIT(IIO_CHAN_INFO_PROCESSED) | >> + BIT(IIO_CHAN_INFO_INT_TIME), >> + } >> +}; >> + >> +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.1 0.2 0.4"); >> + >> +static struct attribute *ltrf216a_attributes[] = { >> + &iio_const_attr_integration_time_available.dev_attr.attr, > please use the read_avail callback and set the appropriate > _available bit. > > That allows in kernel access to this information + is probably > shorter in this case as you won't have an attribute group etc > to deal wtih. > >> + NULL >> +}; >> + >> +static const struct attribute_group ltrf216a_attribute_group = { >> + .attrs = ltrf216a_attributes, >> +}; >> + >> +static int ltrf216a_init(struct iio_dev *indio_dev) >> +{ >> + int ret; >> + struct ltrf216a_data *data = iio_priv(indio_dev); >> + >> + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_CTRL); >> + if (ret < 0) { >> + dev_err(&data->client->dev, "Error reading LTRF216A_MAIN_CTRL\n"); >> + return ret; >> + } >> + >> + /* enable sensor */ >> + ret |= 0x02; > Needs a #define and preferably use > ret |= FIELD_PREP()... > >> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret); >> + if (ret < 0) { >> + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int ltrf216a_disable(struct iio_dev *indio_dev) >> +{ >> + int ret; >> + struct ltrf216a_data *data = iio_priv(indio_dev); >> + >> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0); >> + if (ret < 0) >> + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); >> + >> + return ret; >> +} >> + >> +static int ltrf216a_set_it_time(struct ltrf216a_data *data, int itime) >> +{ >> + int i, ret, index = -1; >> + u8 reg; >> + >> + for (i = 0; i < ARRAY_SIZE(int_time_mapping); i++) { >> + if (int_time_mapping[i] == itime) { >> + index = i; >> + break; >> + } >> + } >> + /* Make sure integration time index is valid */ >> + if (index < 0) >> + return -EINVAL; >> + >> + if (index == 0) { > Switch statement seems more appropriate than this stack of if else > >> + reg = 0x03; > reg isn't a great name as I assume this is the value, not the address > which was my first thought... Perhaps reg_val? >> + data->int_time_fac = 4; >> + } else if (index == 1) { >> + reg = 0x13; >> + data->int_time_fac = 2; >> + } else { >> + reg = (index << 4) | 0x02; > Unless I'm missing something index == 2 if we get here. > So why the calculation? I'd suggest defining the two fields and using > FIELD_PREP() to set up each part probably to one of a set of > #define LTRF216A_ALS_MEAS_RATE_ I think the calculation here is to set the default value when the integration time = 1. In this case, reg value will be 34 (0x22) which is the default value of ALS_MEAS_RATE register. I will still confirm it once from Zhigang before sending a v2. >> + data->int_time_fac = 1; >> + } >> + >> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RATE, reg); > Called MEAS_RES on the datasheet, why this name for the register? > >> + if (ret < 0) >> + return ret; >> + >> + data->int_time = itime; >> + >> + return 0; >> +} >> + >> +static int ltrf216a_get_it_time(struct ltrf216a_data *data, int *val, int *val2) >> +{ >> + *val = 0; >> + *val2 = data->int_time; > I'd put this inline as it avoids a question I had at the call site on why > you passed *val in as it would always be 0. > >> + >> + return IIO_VAL_INT_PLUS_MICRO; >> +} >> + >> +static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr) >> +{ >> + int ret; >> + int tries = 25; >> + int val_0, val_1, val_2; >> + >> + while (tries--) { >> + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS); >> + if (ret < 0) >> + return ret; >> + if (ret & 0x08) > That 0x08 is a magic number and also better defined using BIT(3) > Anyhow, use a define for that. > >> + break; >> + msleep(20); >> + } >> + >> + val_0 = i2c_smbus_read_byte_data(data->client, addr); > All of these can return errors so you should check. > Device doesn't support any larger reads? > >> + val_1 = i2c_smbus_read_byte_data(data->client, addr + 1); >> + val_2 = i2c_smbus_read_byte_data(data->client, addr + 2); >> + ret = (val_2 << 16) + (val_1 << 8) + val_0; > This is a le24_to_cpu() conversion. > Preferred choice would be to use something like > u8 buf[3]; > int i; > > for (i = 0; i < 3; i++) { > ret = i2c_smbus_read_byte_data(data->client, addr); > if (ret < 0) > return ret; > buf[i] = ret; > } > return le24_to_cpu(buf); > >> + >> + return ret; >> +} >> + >> +static int ltrf216a_get_lux(struct ltrf216a_data *data) >> +{ >> + int greendata, cleardata, lux; >> + >> + greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0); >> + cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0); >> + >> + if (greendata < 0 || cleardata < 0) > I'd rather see the error passed all the way up than eaten here. > >> + lux = 0; >> + else >> + lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10; > This feels like it would be better reported as > IIO_VAL_FRACTIONAL; > Won't make any difference if the only user is sysfs, but if anyone > ever wants to use the in kernel access to this device they will get > access to better precision. > > Also possible the core handling for IIO_VAL_FRACTIONAL will do a better > job on retaining precision. > >> + >> + return lux; >> +} >> + >> +static int ltrf216a_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, int *val, >> + int *val2, long mask) >> +{ >> + int ret; >> + struct ltrf216a_data *data = iio_priv(indio_dev); >> + >> + mutex_lock(&data->mutex); >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_PROCESSED: >> + ret = ltrf216a_get_lux(data); >> + *val = ret; >> + ret = IIO_VAL_INT; >> + break; >> + case IIO_CHAN_INFO_INT_TIME: >> + ret = ltrf216a_get_it_time(data, val, val2); >> + break; >> + default: >> + ret = -EINVAL; >> + } >> + >> + mutex_unlock(&data->mutex); >> + >> + return ret; >> +} >> + >> +static int ltrf216a_write_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, int val, >> + int val2, long mask) >> +{ >> + struct ltrf216a_data *data = iio_priv(indio_dev); >> + int ret; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_INT_TIME: >> + if (val != 0) >> + return -EINVAL; >> + mutex_lock(&data->mutex); >> + ret = ltrf216a_set_it_time(data, val2); >> + mutex_unlock(&data->mutex); >> + return ret; >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static const struct iio_info ltrf216a_info = { >> + .read_raw = ltrf216a_read_raw, >> + .write_raw = ltrf216a_write_raw, >> + .attrs = <rf216a_attribute_group, >> +}; >> + >> +static int ltrf216a_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) > Whilst it's taking a long time to clean out the old > approach, probe_new() callback preferred for new i2c drivers. > >> +{ >> + struct ltrf216a_data *data; >> + struct iio_dev *indio_dev; >> + int ret; >> + >> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); >> + if (!indio_dev) >> + return -ENOMEM; >> + >> + data = iio_priv(indio_dev); >> + i2c_set_clientdata(client, indio_dev); >> + data->client = client; >> + >> + mutex_init(&data->mutex); >> + >> + indio_dev->info = <rf216a_info; >> + indio_dev->name = LTRF216A_DRV_NAME; >> + indio_dev->channels = ltrf216a_channels; >> + indio_dev->num_channels = ARRAY_SIZE(ltrf216a_channels); >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + >> + ret = ltrf216a_init(indio_dev); >> + if (ret < 0) { >> + dev_err(&client->dev, "ltrf216a chip init failed\n"); >> + return ret; > As below, slightly preference for dev_err_probe(). > It doesn't really matter when deferral isn't a possibility but using > that function ends up slightly neater. > >> + } > blank line here. Adding separation so we have > "function call > error handler > > next thing.." > > Just makes things a tiny bit easier to read quickly. > > >> + data->int_time = 100000; >> + data->int_time_fac = 1; >> + data->als_gain_fac = 3; >> + >> + ret = iio_device_register(indio_dev); >> + if (ret < 0) { >> + dev_err(&client->dev, "failed to register iio dev\n"); > Once using devm, this can be > return dev_err_probe(&client->dev, ret, > "failed to register iio dev\n"); > > Which is neater but only use this function to print errors for things > that happen in probe. > >> + goto err_init; >> + } >> + >> + return 0; >> +err_init: >> + ltrf216a_disable(indio_dev); >> + return ret; >> +} >> + >> +static int ltrf216a_remove(struct i2c_client *client) >> +{ >> + struct iio_dev *indio_dev = i2c_get_clientdata(client); >> + >> + iio_device_unregister(indio_dev); >> + ltrf216a_disable(indio_dev); > As Kryzstof mentioned, use > devm versions and where they don't exist you can use > devm_add_action_or_reset() (e.g. for this disable) > > That way we don't need error handling or a remove function. >> + >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int ltrf216a_suspend(struct device *dev) >> +{ >> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >> + >> + return ltrf216a_disable(indio_dev); >> +} >> + >> +static int ltrf216a_resume(struct device *dev) >> +{ >> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >> + >> + return ltrf216a_init(indio_dev); >> +} >> + >> +static SIMPLE_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_suspend, ltrf216a_resume); >> +#define LTRF216A_PM_OPS (<rf216a_pm_ops) >> +#else >> +#define LTRF216A_PM_OPS NULL >> +#endif > Recently some 'magic' macros were added to pm.h that allow you to just > specify this stuff without guards > > static DEFINE_SIMPLE_PM_OPS(ltrf216a_pm_ops, lt..._) > > and use > > .pm = pm_sleep_ptr(<rf216a_pm_ops), > > below > > and then the compiler can see enough to remove the unused code > if CONFIG_PM_SLEEP isn't defined without the need for messy > __maybe_unused etc. > > We are part way through driving this change through IIO and > less of the way along with the rest of the kernel, but it's > a nice cleanup so please apply it to new code like this. > >> + >> +static const struct i2c_device_id ltrf216a_id[] = { >> + { LTRF216A_DRV_NAME, 0}, > Space after 0 > >> + {} >> +}; >> +MODULE_DEVICE_TABLE(i2c, ltrf216a_id); >> + >> +static const struct of_device_id ltrf216a_of_match[] = { >> + { .compatible = "ltr,ltrf216a", }, >> + { .compatible = "liteon,ltrf216a", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, ltrf216a_of_match); >> + >> +static struct i2c_driver ltrf216a_driver = { >> + .driver = { >> + .name = LTRF216A_DRV_NAME, >> + .pm = LTRF216A_PM_OPS, >> + .of_match_table = ltrf216a_of_match, >> + }, >> + .probe = ltrf216a_probe, >> + .remove = ltrf216a_remove, >> + .id_table = ltrf216a_id, >> +}; >> + >> +module_i2c_driver(ltrf216a_driver); >> + >> +MODULE_AUTHOR("Shi Zhigang <Zhigang.Shi@liteon.com>"); >> +MODULE_DESCRIPTION("LTRF216A ambient light sensor driver"); >> +MODULE_LICENSE("GPL v2"); > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor 2022-03-29 20:03 ` Shreeya Patel @ 2022-04-02 16:49 ` Jonathan Cameron 0 siblings, 0 replies; 16+ messages in thread From: Jonathan Cameron @ 2022-04-02 16:49 UTC (permalink / raw) To: Shreeya Patel Cc: lars, robh+dt, Zhigang.Shi, krisman, linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez On Wed, 30 Mar 2022 01:33:19 +0530 Shreeya Patel <shreeya.patel@collabora.com> wrote: > On 27/03/22 20:00, Jonathan Cameron wrote: > > Hi Jonathan, > > Thanks for your detailed review. I am working on v2 with the modifications > suggested by you. > > Just one comment inline. > ... > >> +static int ltrf216a_set_it_time(struct ltrf216a_data *data, int itime) > >> +{ > >> + int i, ret, index = -1; > >> + u8 reg; > >> + > >> + for (i = 0; i < ARRAY_SIZE(int_time_mapping); i++) { > >> + if (int_time_mapping[i] == itime) { > >> + index = i; > >> + break; > >> + } > >> + } > >> + /* Make sure integration time index is valid */ > >> + if (index < 0) > >> + return -EINVAL; > >> + > >> + if (index == 0) { > > Switch statement seems more appropriate than this stack of if else > > > >> + reg = 0x03; > > reg isn't a great name as I assume this is the value, not the address > > which was my first thought... Perhaps reg_val? > >> + data->int_time_fac = 4; > >> + } else if (index == 1) { > >> + reg = 0x13; > >> + data->int_time_fac = 2; > >> + } else { > >> + reg = (index << 4) | 0x02; > > Unless I'm missing something index == 2 if we get here. > > So why the calculation? I'd suggest defining the two fields and using > > FIELD_PREP() to set up each part probably to one of a set of > > #define LTRF216A_ALS_MEAS_RATE_ > > I think the calculation here is to set the default value when the > integration time = 1. 1 isn't a possible value in int_time_available. I guess you mean 100ms in which case if this were a switch statement switch (index) { case 0: /* 400msec */ reg = 0x03; data->int_time_fac = 4; break; case 1: /* 200msec */ reg = 0x13; data->int_time_fac = 2; break; case 2: /* 100sec */ reg = 0x22; data->int_time_fac = 1; break; } btw from datasheet, 50ms and 25ms also seem possible, why not support them? Note the switch might be better handled as a constant look up table of appropriate structures. > In this case, reg value will be 34 (0x22) which > is the default value of ALS_MEAS_RATE register. > > I will still confirm it once from Zhigang before sending a v2. > > >> + data->int_time_fac = 1; > >> + } > >> + > Jonathan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor 2022-03-27 14:30 ` Jonathan Cameron 2022-03-29 20:03 ` Shreeya Patel @ 2022-04-11 17:06 ` Shreeya Patel 2022-04-12 14:06 ` Gabriel Krisman Bertazi 1 sibling, 1 reply; 16+ messages in thread From: Shreeya Patel @ 2022-04-11 17:06 UTC (permalink / raw) To: Jonathan Cameron, krzk Cc: lars, robh+dt, Zhigang.Shi, krisman, linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez On 27/03/22 20:00, Jonathan Cameron wrote: Hi Jonathan, I have a question inline related to one of your comments. > On Fri, 25 Mar 2022 16:00:14 +0530 > Shreeya Patel <shreeya.patel@collabora.com> wrote: > > Hi Zhigang, Shreeya, > > Comments inline. > > Thanks, > > Jonathan >> From: Zhigang Shi <Zhigang.Shi@liteon.com> >> >> Add initial support for ltrf216a ambient light sensor. >> >> Datasheet :- >> https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf > We now have a Datasheet tag, so make this part of the tag block so automated > tools can find it easily: >> > Datasheet: https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf >> Co-developed-by: Shreeya Patel <shreeya.patel@collabora.com> >> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> >> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com> >> --- >> drivers/iio/light/Kconfig | 10 ++ >> drivers/iio/light/Makefile | 1 + >> drivers/iio/light/ltrf216a.c | 334 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 345 insertions(+) >> create mode 100644 drivers/iio/light/ltrf216a.c >> >> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig >> index a62c7b4b8678..08fa383a8ca7 100644 >> --- a/drivers/iio/light/Kconfig >> +++ b/drivers/iio/light/Kconfig >> @@ -318,6 +318,16 @@ config SENSORS_LM3533 >> changes. The ALS-control output values can be set per zone for the >> three current output channels. >> >> +config LTRF216A >> + tristate "Liteon LTRF216A Light Sensor" >> + depends on I2C >> + help >> + If you say Y or M here, you get support for Liteon LTRF216A >> + Ambient Light Sensor. >> + >> + If built as a dynamically linked module, it will be called >> + ltrf216a. >> + >> config LTR501 >> tristate "LTR-501ALS-01 light sensor" >> depends on I2C >> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile >> index d10912faf964..8fa91b9fe5b6 100644 >> --- a/drivers/iio/light/Makefile >> +++ b/drivers/iio/light/Makefile >> @@ -30,6 +30,7 @@ obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o >> obj-$(CONFIG_ISL29125) += isl29125.o >> obj-$(CONFIG_JSA1212) += jsa1212.o >> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o >> +obj-$(CONFIG_LTRF216A) += ltrf216a.o >> obj-$(CONFIG_LTR501) += ltr501.o >> obj-$(CONFIG_LV0104CS) += lv0104cs.o >> obj-$(CONFIG_MAX44000) += max44000.o >> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c >> new file mode 100644 >> index 000000000000..99295358a7fe >> --- /dev/null >> +++ b/drivers/iio/light/ltrf216a.c >> @@ -0,0 +1,334 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * LTRF216A Ambient Light Sensor >> + * >> + * Copyright (C) 2021 Lite-On Technology Corp (Singapore) >> + * Author: Shi Zhigang <Zhigang.Shi@liteon.com> >> + * >> + * IIO driver for LTRF216A (7-bit I2C slave address 0x53). >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/i2c.h> >> +#include <linux/mutex.h> >> +#include <linux/iio/iio.h> >> +#include <linux/iio/sysfs.h> > mod_devicetable.h for the id tables > >> +#include <linux/pm.h> >> +#include <linux/delay.h> >> + >> +#define LTRF216A_DRV_NAME "ltrf216a" >> + >> +#define LTRF216A_MAIN_CTRL 0x00 >> + >> +#define LTRF216A_ALS_MEAS_RATE 0x04 > MEAS_RES seems more appropriate from datasheet. > As mentioned below, also add defines for all the fields > of the registers you will access and where the fields are > obvious numerical things, add defines for the values they > can take. > > >> +#define LTRF216A_MAIN_STATUS 0x07 >> +#define LTRF216A_CLEAR_DATA_0 0x0A >> + >> +#define LTRF216A_ALS_DATA_0 0x0D >> + >> +static const int int_time_mapping[] = { 400000, 200000, 100000 }; >> + >> +struct ltrf216a_data { >> + struct i2c_client *client; >> + u32 int_time; >> + u8 int_time_fac; >> + u8 als_gain_fac; >> + struct mutex mutex; > All locks need a comment to say exactly what they are protecting. > >> +}; >> + >> +/* open air. need to update based on TP transmission rate. */ >> +#define WIN_FAC 1 >> + >> +static const struct iio_chan_spec ltrf216a_channels[] = { >> + { >> + .type = IIO_LIGHT, >> + .info_mask_separate = >> + BIT(IIO_CHAN_INFO_PROCESSED) | >> + BIT(IIO_CHAN_INFO_INT_TIME), >> + } >> +}; >> + >> +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.1 0.2 0.4"); >> + >> +static struct attribute *ltrf216a_attributes[] = { >> + &iio_const_attr_integration_time_available.dev_attr.attr, > please use the read_avail callback and set the appropriate > _available bit. > > That allows in kernel access to this information + is probably > shorter in this case as you won't have an attribute group etc > to deal wtih. > >> + NULL >> +}; >> + >> +static const struct attribute_group ltrf216a_attribute_group = { >> + .attrs = ltrf216a_attributes, >> +}; >> + >> +static int ltrf216a_init(struct iio_dev *indio_dev) >> +{ >> + int ret; >> + struct ltrf216a_data *data = iio_priv(indio_dev); >> + >> + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_CTRL); >> + if (ret < 0) { >> + dev_err(&data->client->dev, "Error reading LTRF216A_MAIN_CTRL\n"); >> + return ret; >> + } >> + >> + /* enable sensor */ >> + ret |= 0x02; > Needs a #define and preferably use > ret |= FIELD_PREP()... > >> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret); >> + if (ret < 0) { >> + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int ltrf216a_disable(struct iio_dev *indio_dev) >> +{ >> + int ret; >> + struct ltrf216a_data *data = iio_priv(indio_dev); >> + >> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0); >> + if (ret < 0) >> + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); >> + >> + return ret; >> +} >> + >> +static int ltrf216a_set_it_time(struct ltrf216a_data *data, int itime) >> +{ >> + int i, ret, index = -1; >> + u8 reg; >> + >> + for (i = 0; i < ARRAY_SIZE(int_time_mapping); i++) { >> + if (int_time_mapping[i] == itime) { >> + index = i; >> + break; >> + } >> + } >> + /* Make sure integration time index is valid */ >> + if (index < 0) >> + return -EINVAL; >> + >> + if (index == 0) { > Switch statement seems more appropriate than this stack of if else > >> + reg = 0x03; > reg isn't a great name as I assume this is the value, not the address > which was my first thought... Perhaps reg_val? > >> + data->int_time_fac = 4; >> + } else if (index == 1) { >> + reg = 0x13; >> + data->int_time_fac = 2; >> + } else { >> + reg = (index << 4) | 0x02; > Unless I'm missing something index == 2 if we get here. > So why the calculation? I'd suggest defining the two fields and using > FIELD_PREP() to set up each part probably to one of a set of > #define LTRF216A_ALS_MEAS_RATE_ > >> + data->int_time_fac = 1; >> + } >> + >> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RATE, reg); > Called MEAS_RES on the datasheet, why this name for the register? > >> + if (ret < 0) >> + return ret; >> + >> + data->int_time = itime; >> + >> + return 0; >> +} >> + >> +static int ltrf216a_get_it_time(struct ltrf216a_data *data, int *val, int *val2) >> +{ >> + *val = 0; >> + *val2 = data->int_time; > I'd put this inline as it avoids a question I had at the call site on why > you passed *val in as it would always be 0. > >> + >> + return IIO_VAL_INT_PLUS_MICRO; >> +} >> + >> +static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr) >> +{ >> + int ret; >> + int tries = 25; >> + int val_0, val_1, val_2; >> + >> + while (tries--) { >> + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS); >> + if (ret < 0) >> + return ret; >> + if (ret & 0x08) > That 0x08 is a magic number and also better defined using BIT(3) > Anyhow, use a define for that. > >> + break; >> + msleep(20); >> + } >> + >> + val_0 = i2c_smbus_read_byte_data(data->client, addr); > All of these can return errors so you should check. > Device doesn't support any larger reads? > >> + val_1 = i2c_smbus_read_byte_data(data->client, addr + 1); >> + val_2 = i2c_smbus_read_byte_data(data->client, addr + 2); >> + ret = (val_2 << 16) + (val_1 << 8) + val_0; > This is a le24_to_cpu() conversion. > Preferred choice would be to use something like > u8 buf[3]; > int i; > > for (i = 0; i < 3; i++) { > ret = i2c_smbus_read_byte_data(data->client, addr); > if (ret < 0) > return ret; > buf[i] = ret; > } > return le24_to_cpu(buf); > We do not have any le24_to_cpu() function in current kernel source code. I was thinking to use le32_to_cpu() instead but it requires an argument of type __le32 and our case we storing the values in u8 buf[3] so I'm not really sure if it's possible to use le32_to_cpu() or any other function. Thanks, Shreeya Patel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor 2022-04-11 17:06 ` Shreeya Patel @ 2022-04-12 14:06 ` Gabriel Krisman Bertazi 2022-04-12 14:53 ` Jonathan Cameron 0 siblings, 1 reply; 16+ messages in thread From: Gabriel Krisman Bertazi @ 2022-04-12 14:06 UTC (permalink / raw) To: Shreeya Patel Cc: Jonathan Cameron, krzk, lars, robh+dt, Zhigang.Shi, linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez Shreeya Patel <shreeya.patel@collabora.com> writes: >>> + val_1 = i2c_smbus_read_byte_data(data->client, addr + 1); >>> + val_2 = i2c_smbus_read_byte_data(data->client, addr + 2); >>> + ret = (val_2 << 16) + (val_1 << 8) + val_0; >> This is a le24_to_cpu() conversion. >> Preferred choice would be to use something like >> u8 buf[3]; >> int i; >> >> for (i = 0; i < 3; i++) { >> ret = i2c_smbus_read_byte_data(data->client, addr); >> if (ret < 0) >> return ret; >> buf[i] = ret; >> } >> return le24_to_cpu(buf); >> > > We do not have any le24_to_cpu() function in current kernel source code. > I was thinking to use le32_to_cpu() instead but it requires an argument of > type __le32 and our case we storing the values in u8 buf[3] so I'm not > really sure if it's possible to use le32_to_cpu() or any other function. I guess you could make it a 32-bit buffer, keep the most significant byte zeroed and return le32_to_cpu: u8 buf[4]; memset(buf, 0x0, sizeof(buf)); for (i = 0; i < 3; i++) { ret = i2c_smbus_read_byte_data(data->client, addr); if (ret < 0) return ret; buf[i] = ret; } return le32_to_cpu(buf); -- Gabriel Krisman Bertazi ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor 2022-04-12 14:06 ` Gabriel Krisman Bertazi @ 2022-04-12 14:53 ` Jonathan Cameron 0 siblings, 0 replies; 16+ messages in thread From: Jonathan Cameron @ 2022-04-12 14:53 UTC (permalink / raw) To: Gabriel Krisman Bertazi Cc: Shreeya Patel, Jonathan Cameron, krzk, lars, robh+dt, Zhigang.Shi, linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez On Tue, 12 Apr 2022 10:06:18 -0400 Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > Shreeya Patel <shreeya.patel@collabora.com> writes: > > >>> + val_1 = i2c_smbus_read_byte_data(data->client, addr + 1); > >>> + val_2 = i2c_smbus_read_byte_data(data->client, addr + 2); > >>> + ret = (val_2 << 16) + (val_1 << 8) + val_0; > >> This is a le24_to_cpu() conversion. > >> Preferred choice would be to use something like > >> u8 buf[3]; > >> int i; > >> > >> for (i = 0; i < 3; i++) { > >> ret = i2c_smbus_read_byte_data(data->client, addr); > >> if (ret < 0) > >> return ret; > >> buf[i] = ret; > >> } > >> return le24_to_cpu(buf); > >> > > > > We do not have any le24_to_cpu() function in current kernel source code. > > I was thinking to use le32_to_cpu() instead but it requires an argument of > > type __le32 and our case we storing the values in u8 buf[3] so I'm not > > really sure if it's possible to use le32_to_cpu() or any other function. > > I guess you could make it a 32-bit buffer, keep the most > significant byte zeroed and return le32_to_cpu: > > u8 buf[4]; > > memset(buf, 0x0, sizeof(buf)); > > for (i = 0; i < 3; i++) { > ret = i2c_smbus_read_byte_data(data->client, addr); > if (ret < 0) > return ret; > buf[i] = ret; > } > return le32_to_cpu(buf); > I was being silly. It's not aligned for obvious reasons that we don't do 24bit alignment, so you need get_unaligned_le24() Jonathan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor 2022-03-25 10:30 ` [PATCH 3/3] iio: light: Add support for ltrf216a sensor Shreeya Patel 2022-03-25 12:25 ` Krzysztof Kozlowski 2022-03-27 14:30 ` Jonathan Cameron @ 2022-03-28 3:59 ` Gabriel Krisman Bertazi 2 siblings, 0 replies; 16+ messages in thread From: Gabriel Krisman Bertazi @ 2022-03-28 3:59 UTC (permalink / raw) To: Shreeya Patel Cc: jic23, lars, robh+dt, Zhigang.Shi, linux-iio, devicetree, linux-kernel, kernel, alvaro.soliverez Shreeya Patel <shreeya.patel@collabora.com> writes: > From: Zhigang Shi <Zhigang.Shi@liteon.com> > > Add initial support for ltrf216a ambient light sensor. > > Datasheet :- > https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf > + struct ltrf216a_data *data = iio_priv(indio_dev); > + > + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0); > + if (ret < 0) > + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n"); > + > + return ret; > +} > + > +static int ltrf216a_set_it_time(struct ltrf216a_data *data, int itime) ltrf216a_set_int_time instad of it_time? although, ltr501 also uses "it" instead of "int" on the function name.. > + > +static int ltrf216a_get_lux(struct ltrf216a_data *data) > +{ > + int greendata, cleardata, lux; > + > + greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0); > + cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0); > + > + if (greendata < 0 || cleardata < 0) > + lux = 0; > + else > + lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10; This could be rewritten to avoid most of the divisions. But it also doesn't fit the calculation shown in page 20 on the datasheet. I suspect that 8 was calculated from a specific Window Factor (~1.77), which is specific to one device, but I'm not sure. The datasheet formula is: lux = (ALS_DATA_X * 0.45 * window_factor) / (gain * int_time) Shouldn't WIN_FAC be a configurable parameter, instead of constant? -- Gabriel Krisman Bertazi ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-04-12 14:53 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-25 10:30 [PATCH 0/3] Add LTRF216A Driver Shreeya Patel 2022-03-25 10:30 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix Shreeya Patel 2022-03-25 12:21 ` Krzysztof Kozlowski 2022-03-25 10:30 ` [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings Shreeya Patel 2022-03-25 12:23 ` Krzysztof Kozlowski 2022-03-27 13:55 ` Jonathan Cameron 2022-03-28 2:49 ` Gabriel Krisman Bertazi 2022-03-25 10:30 ` [PATCH 3/3] iio: light: Add support for ltrf216a sensor Shreeya Patel 2022-03-25 12:25 ` Krzysztof Kozlowski 2022-03-27 14:30 ` Jonathan Cameron 2022-03-29 20:03 ` Shreeya Patel 2022-04-02 16:49 ` Jonathan Cameron 2022-04-11 17:06 ` Shreeya Patel 2022-04-12 14:06 ` Gabriel Krisman Bertazi 2022-04-12 14:53 ` Jonathan Cameron 2022-03-28 3:59 ` Gabriel Krisman Bertazi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.