[PATCH v4 0/3] Add LTRF216A Driver

[PATCH v4 0/3] Add LTRF216A Driver

[Shreeya Patel]

This patchset adds support for ltrf216a Ambient Light Sensor
and documents the DT bindings for the same.


Changes in v4
  - Add more descriptive comment for mutex lock
  - Fix mutex locking in read_raw()
  - Use i2c_smbus_read_i2c_block_data()

Changes in v3
  - Use u16 instead of u8 for int_time_fac
  - Reorder headers in ltrf216a.c file
  - Remove int_time_mapping table and use int_time_available
  - Fix indentation in the bindings file.

Changes in v2
  - Add support for 25ms and 50ms integration time.
  - Rename some of the macros as per names given in datasheet
  - Add a comment for the mutex lock
  - Use read_avail callback instead of attributes and set the
    appropriate _available bit.
  - Use FIELD_PREP() at appropriate places.
  - Add a constant lookup table for integration time and reg val
  - Use BIT() macro for magic numbers.
  - Improve error handling at few places.
  - Use get_unaligned_le24() and div_u64()
  - Use probe_new() callback and devm functions
  - Return errors in probe using dev_err_probe()
  - Use DEFINE_SIMPLE_DEV_PM_OPS()
  - Correct the formula for lux to use 0.45 instead of 0.8
  - Add interrupt and power supply property in DT bindings
  - Add vendor prefix name as per the alphabetical order.


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   |  51 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   3 +
 drivers/iio/light/Kconfig                     |  10 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/ltrf216a.c                  | 346 ++++++++++++++++++
 5 files changed, 411 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
 create mode 100644 drivers/iio/light/ltrf216a.c

-- 
2.30.2

[PATCH v4 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix

[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" },

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---

Changes in v2
  - Add vendor prefix name as per the alphabetical order.

 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 01430973ecec..02f94fba03b6 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -716,6 +716,9 @@ patternProperties:
     description: Loongson Technology Corporation Limited
   "^lsi,.*":
     description: LSI Corp. (LSI Logic)
+  "^ltr,.*":
+    description: LITE-ON Technology Corp.
+    deprecated: true
   "^lwn,.*":
     description: Liebherr-Werk Nenzing GmbH
   "^lxa,.*":
-- 
2.30.2

Re: [PATCH v4 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix

[Rob Herring]

On Wed, May 11, 2022 at 03:10:22PM +0530, 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.

ACPI? NAK.

There are no cases of 'ltr' for DT, so fix 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" },
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
> 
> Changes in v2
>   - Add vendor prefix name as per the alphabetical order.
> 
>  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 01430973ecec..02f94fba03b6 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -716,6 +716,9 @@ patternProperties:
>      description: Loongson Technology Corporation Limited
>    "^lsi,.*":
>      description: LSI Corp. (LSI Logic)
> +  "^ltr,.*":
> +    description: LITE-ON Technology Corp.
> +    deprecated: true
>    "^lwn,.*":
>      description: Liebherr-Werk Nenzing GmbH
>    "^lxa,.*":
> -- 
> 2.30.2
> 
> 

Re: [PATCH v4 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix

[Shreeya Patel]

On 16/05/22 22:30, Rob Herring wrote:
> On Wed, May 11, 2022 at 03:10:22PM +0530, 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.
> ACPI? NAK.
>
> There are no cases of 'ltr' for DT, so fix ACPI.

Hi Rob,

Yes, we understand there are no cases of 'ltr', but we have released devices
which uses this string for probing the ltrf216a light sensor driver ( 
x86 with DT )

If we don't document this in vendor-prefixes.yaml, then the following 
warning
is generated.

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" },


Can you suggest us what would be the right way to fix this warning if 
not documenting
in vendor-prefixes.yaml?



Thanks,
Shreeya Patel

>
>> 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" },
>>
>> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
>> ---
>>
>> Changes in v2
>>    - Add vendor prefix name as per the alphabetical order.
>>
>>   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 01430973ecec..02f94fba03b6 100644
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> @@ -716,6 +716,9 @@ patternProperties:
>>       description: Loongson Technology Corporation Limited
>>     "^lsi,.*":
>>       description: LSI Corp. (LSI Logic)
>> +  "^ltr,.*":
>> +    description: LITE-ON Technology Corp.
>> +    deprecated: true
>>     "^lwn,.*":
>>       description: Liebherr-Werk Nenzing GmbH
>>     "^lxa,.*":
>> -- 
>> 2.30.2
>>
>>

Re: [PATCH v4 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix

[Rob Herring]

On Tue, May 17, 2022 at 04:07:33PM +0530, Shreeya Patel wrote:
> 
> On 16/05/22 22:30, Rob Herring wrote:
> > On Wed, May 11, 2022 at 03:10:22PM +0530, 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.
> > ACPI? NAK.
> > 
> > There are no cases of 'ltr' for DT, so fix ACPI.
> 
> Hi Rob,
> 
> Yes, we understand there are no cases of 'ltr', but we have released devices
> which uses this string for probing the ltrf216a light sensor driver ( x86
> with DT )

That's not what your commit message says.

Even if this is DT based, given an undocumented vendor string is used, 
it seems doubtful the rest of the binding would match upstream. What 
about the rest of the DTB? Got a pointer to it or want to publish it?

> If we don't document this in vendor-prefixes.yaml, then the following
> warning
> is generated.
> 
> 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" },
> 
> 
> Can you suggest us what would be the right way to fix this warning if not
> documenting
> in vendor-prefixes.yaml?

Fix the DT. We don't accept bindings simply because they are already 
used in the field. If this was the only issue, it would be fine, but I 
suspect it's the tip of the iceberg.

Rob

Re: [PATCH v4 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix

[Shreeya Patel]

On 18/05/22 22:02, Rob Herring wrote:
> On Tue, May 17, 2022 at 04:07:33PM +0530, Shreeya Patel wrote:
>> On 16/05/22 22:30, Rob Herring wrote:
>>> On Wed, May 11, 2022 at 03:10:22PM +0530, 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.
>>> ACPI? NAK.
>>>
>>> There are no cases of 'ltr' for DT, so fix ACPI.
>> Hi Rob,
>>
>> Yes, we understand there are no cases of 'ltr', but we have released devices
>> which uses this string for probing the ltrf216a light sensor driver ( x86
>> with DT )
> That's not what your commit message says.
>
> Even if this is DT based, given an undocumented vendor string is used,
> it seems doubtful the rest of the binding would match upstream. What
> about the rest of the DTB? Got a pointer to it or want to publish it?
>
>> If we don't document this in vendor-prefixes.yaml, then the following
>> warning
>> is generated.
>>
>> 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" },
>>
>>
>> Can you suggest us what would be the right way to fix this warning if not
>> documenting
>> in vendor-prefixes.yaml?
> Fix the DT. We don't accept bindings simply because they are already
> used in the field. If this was the only issue, it would be fine, but I
> suspect it's the tip of the iceberg.


Hi Rob,

To make things more clear, following is the modalias info of the device.

(B+)(root@linux iio:device0)# cat 
/sys/bus/i2c/devices/i2c-PRP0001\:01/modalias
of:NltrfTCltr,ltrf216a

It's a dt namespace on an ACPI based device. We used an of_device_id 
table to be able to probe the driver
using the vendor prefix and compatible string.

But when we compile the driver, we get the following warning and hence 
we documented it in vendor-prefixes.yaml
and also added a complete device tree file [Patch 3/3] just to get rid 
of the warning. In real life we are not using
the device tree file at all.

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" },

There are many existing devices used by people which has the vendor 
prefix name as 'ltr'
and it won't be possible to change that hence we are trying to upstream it.


Thanks,
Shreeya Patel

>
> Rob
>

Re: [PATCH v4 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix

[Rob Herring]

On Mon, May 23, 2022 at 08:27:56PM +0530, Shreeya Patel wrote:
> 
> On 18/05/22 22:02, Rob Herring wrote:
> > On Tue, May 17, 2022 at 04:07:33PM +0530, Shreeya Patel wrote:
> > > On 16/05/22 22:30, Rob Herring wrote:
> > > > On Wed, May 11, 2022 at 03:10:22PM +0530, 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.
> > > > ACPI? NAK.
> > > > 
> > > > There are no cases of 'ltr' for DT, so fix ACPI.
> > > Hi Rob,
> > > 
> > > Yes, we understand there are no cases of 'ltr', but we have released devices
> > > which uses this string for probing the ltrf216a light sensor driver ( x86
> > > with DT )
> > That's not what your commit message says.
> > 
> > Even if this is DT based, given an undocumented vendor string is used,
> > it seems doubtful the rest of the binding would match upstream. What
> > about the rest of the DTB? Got a pointer to it or want to publish it?
> > 
> > > If we don't document this in vendor-prefixes.yaml, then the following
> > > warning
> > > is generated.
> > > 
> > > 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" },
> > > 
> > > 
> > > Can you suggest us what would be the right way to fix this warning if not
> > > documenting
> > > in vendor-prefixes.yaml?
> > Fix the DT. We don't accept bindings simply because they are already
> > used in the field. If this was the only issue, it would be fine, but I
> > suspect it's the tip of the iceberg.
> 
> 
> Hi Rob,
> 
> To make things more clear, following is the modalias info of the device.
> 
> (B+)(root@linux iio:device0)# cat
> /sys/bus/i2c/devices/i2c-PRP0001\:01/modalias
> of:NltrfTCltr,ltrf216a
> 
> It's a dt namespace on an ACPI based device. We used an of_device_id table
> to be able to probe the driver
> using the vendor prefix and compatible string.

Again, it's ACPI so I don't care. If someone cares about using DT 
bindings in ACPI they can step up and help maintain them. It's not a DT 
vs. ACPI thing, but just that I can only maintain so much and have to 
draw the line somewhere.

> But when we compile the driver, we get the following warning and hence we
> documented it in vendor-prefixes.yaml
> and also added a complete device tree file [Patch 3/3] just to get rid of
> the warning. In real life we are not using
> the device tree file at all.
> 
> 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" },

So, is someone telling you to fix this?


> There are many existing devices used by people which has the vendor prefix
> name as 'ltr'
> and it won't be possible to change that hence we are trying to upstream it.

There are millions if not billions of DT based devices using 
undocumented bindings. If those used "ltr,ltrf216a", I wouldn't accept 
it either.

Rob

Re: [PATCH v4 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix

[Shreeya Patel]

On 24/05/22 21:17, Rob Herring wrote:
> On Mon, May 23, 2022 at 08:27:56PM +0530, Shreeya Patel wrote:
>> On 18/05/22 22:02, Rob Herring wrote:
>>> On Tue, May 17, 2022 at 04:07:33PM +0530, Shreeya Patel wrote:
>>>> On 16/05/22 22:30, Rob Herring wrote:
>>>>> On Wed, May 11, 2022 at 03:10:22PM +0530, 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.
>>>>> ACPI? NAK.
>>>>>
>>>>> There are no cases of 'ltr' for DT, so fix ACPI.
>>>> Hi Rob,
>>>>
>>>> Yes, we understand there are no cases of 'ltr', but we have released devices
>>>> which uses this string for probing the ltrf216a light sensor driver ( x86
>>>> with DT )
>>> That's not what your commit message says.
>>>
>>> Even if this is DT based, given an undocumented vendor string is used,
>>> it seems doubtful the rest of the binding would match upstream. What
>>> about the rest of the DTB? Got a pointer to it or want to publish it?
>>>
>>>> If we don't document this in vendor-prefixes.yaml, then the following
>>>> warning
>>>> is generated.
>>>>
>>>> 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" },
>>>>
>>>>
>>>> Can you suggest us what would be the right way to fix this warning if not
>>>> documenting
>>>> in vendor-prefixes.yaml?
>>> Fix the DT. We don't accept bindings simply because they are already
>>> used in the field. If this was the only issue, it would be fine, but I
>>> suspect it's the tip of the iceberg.
>>
>> Hi Rob,
>>
>> To make things more clear, following is the modalias info of the device.
>>
>> (B+)(root@linux iio:device0)# cat
>> /sys/bus/i2c/devices/i2c-PRP0001\:01/modalias
>> of:NltrfTCltr,ltrf216a
>>
>> It's a dt namespace on an ACPI based device. We used an of_device_id table
>> to be able to probe the driver
>> using the vendor prefix and compatible string.
> Again, it's ACPI so I don't care. If someone cares about using DT
> bindings in ACPI they can step up and help maintain them. It's not a DT
> vs. ACPI thing, but just that I can only maintain so much and have to
> draw the line somewhere.
>
>> But when we compile the driver, we get the following warning and hence we
>> documented it in vendor-prefixes.yaml
>> and also added a complete device tree file [Patch 3/3] just to get rid of
>> the warning. In real life we are not using
>> the device tree file at all.
>>
>> 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" },
> So, is someone telling you to fix this?


So will it be right to just keep the warning and remove this patch?
Is there a way you know to silent this warning?


Thanks,
Shreeya Patel

>
>
>> There are many existing devices used by people which has the vendor prefix
>> name as 'ltr'
>> and it won't be possible to change that hence we are trying to upstream it.
> There are millions if not billions of DT based devices using
> undocumented bindings. If those used "ltr,ltrf216a", I wouldn't accept
> it either.
>
> Rob
>

[PATCH v4 2/3] dt-bindings: Document ltrf216a light sensor bindings

[Shreeya Patel]

Add devicetree bindings for ltrf216a ambient light sensor.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---

Changes in v3
  - Fix indentation in the example section

Changes in v2
  - Take over the maintainership for the bindings
  - Add interrupt and power supply property in DT bindings


 .../bindings/iio/light/liteon,ltrf216a.yaml   | 51 +++++++++++++++++++
 1 file changed, 51 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..1389639cd7fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
@@ -0,0 +1,51 @@
+# 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:
+  - Shreeya Patel <shreeya.patel@collabora.com>
+
+description:
+  Ambient light sensing with an i2c interface.
+
+properties:
+  compatible:
+    oneOf:
+      - const: liteon,ltrf216a
+      - const: ltr,ltrf216a
+        deprecated: true
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply:
+    description: Regulator that provides power to the sensor.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@53 {
+            compatible = "liteon,ltrf216a";
+            reg = <0x53>;
+            vdd-supply = <&vdd_regulator>;
+            interrupt-parent = <&gpio0>;
+            interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
+        };
+    };
-- 
2.30.2

Re: [PATCH v4 2/3] dt-bindings: Document ltrf216a light sensor bindings

[Rob Herring]

On Wed, May 11, 2022 at 03:10:23PM +0530, Shreeya Patel wrote:
> Add devicetree bindings for ltrf216a ambient light sensor.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
> 
> Changes in v3
>   - Fix indentation in the example section
> 
> Changes in v2
>   - Take over the maintainership for the bindings
>   - Add interrupt and power supply property in DT bindings
> 
> 
>  .../bindings/iio/light/liteon,ltrf216a.yaml   | 51 +++++++++++++++++++
>  1 file changed, 51 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..1389639cd7fd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
> @@ -0,0 +1,51 @@
> +# 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:
> +  - Shreeya Patel <shreeya.patel@collabora.com>
> +
> +description:
> +  Ambient light sensing with an i2c interface.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: liteon,ltrf216a
> +      - const: ltr,ltrf216a
> +        deprecated: true

As this is for ACPI, NAK. There's not really any point in having this in 
schema as you can't use the schema with ACPI.

> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: Regulator that provides power to the sensor.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        light-sensor@53 {
> +            compatible = "liteon,ltrf216a";
> +            reg = <0x53>;
> +            vdd-supply = <&vdd_regulator>;
> +            interrupt-parent = <&gpio0>;
> +            interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
> +        };
> +    };
> -- 
> 2.30.2
> 
> 

[PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Shreeya Patel]

From: Zhigang Shi <Zhigang.Shi@liteon.com>

Add initial support for ltrf216a ambient light sensor.

Datasheet: gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTRF216A.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>
---

Changes in v4
  - Add more descriptive comment for mutex lock
  - Fix mutex locking in read_raw()
  - Use i2c_smbus_read_i2c_block_data()

Changes in v3
  - Use u16 instead of u8 for int_time_fac
  - Reorder headers in ltrf216a.c file
  - Remove int_time_mapping table and use int_time_available

Changes in v2
  - Add support for 25ms and 50ms integration time.
  - Rename some of the macros as per names given in datasheet
  - Add a comment for the mutex lock
  - Use read_avail callback instead of attributes and set the
    appropriate _available bit.
  - Use FIELD_PREP() at appropriate places.
  - Add a constant lookup table for integration time and reg val
  - Use BIT() macro for magic numbers.
  - Improve error handling at few places.
  - Use get_unaligned_le24() and div_u64()
  - Use probe_new() callback and devm functions
  - Return errors in probe using dev_err_probe()
  - Use DEFINE_SIMPLE_DEV_PM_OPS()
  - Correct the formula for lux to use 0.45 instead of 0.8

 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/ltrf216a.c | 346 +++++++++++++++++++++++++++++++++++
 3 files changed, 357 insertions(+)
 create mode 100644 drivers/iio/light/ltrf216a.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index a62c7b4b8678..33d2b24ba1da 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..05ae9912c854
--- /dev/null
+++ b/drivers/iio/light/ltrf216a.c
@@ -0,0 +1,346 @@
+// 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/bitfield.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/iio/iio.h>
+#include <asm/unaligned.h>
+
+#define LTRF216A_DRV_NAME "ltrf216a"
+
+#define LTRF216A_MAIN_CTRL		0x00
+
+#define LTRF216A_ALS_DATA_STATUS	BIT(3)
+#define LTRF216A_ALS_ENABLE_MASK	BIT(1)
+
+#define LTRF216A_ALS_MEAS_RES		0x04
+#define LTRF216A_MAIN_STATUS		0x07
+#define LTRF216A_CLEAR_DATA_0		0x0A
+
+#define LTRF216A_ALS_DATA_0		0x0D
+
+static const int ltrf216a_int_time_available[5][2] = {
+	{0, 400000},
+	{0, 200000},
+	{0, 100000},
+	{0, 50000},
+	{0, 25000},
+};
+
+static const int ltrf216a_int_time_reg[5][2] = {
+	{400, 0x03},
+	{200, 0x13},
+	{100, 0x22},
+	{50, 0x31},
+	{25, 0x40},
+};
+
+struct ltrf216a_data {
+	struct i2c_client *client;
+	u32 int_time;
+	u16 int_time_fac;
+	u8 als_gain_fac;
+	/*
+	 * Ensure cached value of integration time is consistent
+	 * with hardware setting and remains constant during a
+	 * measurement of Lux.
+	 */
+	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),
+		.info_mask_separate_available =
+			BIT(IIO_CHAN_INFO_INT_TIME),
+	}
+};
+
+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 |= FIELD_PREP(LTRF216A_ALS_ENABLE_MASK, 1);
+	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 void als_ltrf216a_disable(void *data)
+{
+	struct iio_dev *indio_dev = data;
+
+	ltrf216a_disable(indio_dev);
+}
+
+static int ltrf216a_set_int_time(struct ltrf216a_data *data, int itime)
+{
+	int i, ret, index = -1;
+	u8 reg_val;
+
+	for (i = 0; i < ARRAY_SIZE(ltrf216a_int_time_available); i++) {
+		if (ltrf216a_int_time_available[i][1] == itime) {
+			index = i;
+			break;
+		}
+	}
+
+	if (index < 0)
+		return -EINVAL;
+
+	reg_val = ltrf216a_int_time_reg[index][1];
+	data->int_time_fac = ltrf216a_int_time_reg[index][0];
+
+	ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RES, reg_val);
+	if (ret < 0)
+		return ret;
+
+	data->int_time = itime;
+
+	return 0;
+}
+
+static int ltrf216a_get_int_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 = -1, tries = 25;
+	u8 buf[3];
+
+	while (tries--) {
+		ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS);
+		if (ret < 0)
+			return ret;
+		if (ret & LTRF216A_ALS_DATA_STATUS)
+			break;
+		msleep(20);
+	}
+
+	ret = i2c_smbus_read_i2c_block_data(data->client, addr, sizeof(buf), buf);
+	if (ret < 0)
+		return ret;
+
+	return get_unaligned_le24(&buf[0]);
+}
+
+static int ltrf216a_get_lux(struct ltrf216a_data *data)
+{
+	int greendata, cleardata;
+	u64 lux, div;
+
+	greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
+	cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
+
+	if (greendata < 0 || cleardata < 0)
+		return -EINVAL;
+
+	lux = greendata * 45 * WIN_FAC * 100;
+	div = data->als_gain_fac * data->int_time_fac * 100;
+
+	return div_u64(lux, div);
+}
+
+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);
+		if (ret < 0) {
+			mutex_unlock(&data->mutex);
+			return ret;
+		}
+		*val = ret;
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = ltrf216a_get_int_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_int_time(data, val2);
+		mutex_unlock(&data->mutex);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltrf216a_read_available(struct iio_dev *indio_dev,
+				   struct iio_chan_spec const *chan,
+				   const int **vals, int *type, int *length,
+				   long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*length = ARRAY_SIZE(ltrf216a_int_time_available) * 2;
+		*vals = (const int *)ltrf216a_int_time_available;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ltrf216a_info = {
+	.read_raw	= ltrf216a_read_raw,
+	.write_raw	= ltrf216a_write_raw,
+	.read_avail	= ltrf216a_read_available,
+};
+
+static int ltrf216a_probe(struct i2c_client *client)
+{
+	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 = &ltrf216a_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)
+		return dev_err_probe(&client->dev, ret,
+				     "ltrf216a chip init failed\n");
+
+	data->int_time = 100000;
+	data->int_time_fac = 100;
+	data->als_gain_fac = 3;
+
+	ret = devm_add_action_or_reset(&client->dev, als_ltrf216a_disable,
+				       indio_dev);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+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 DEFINE_SIMPLE_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_suspend, ltrf216a_resume);
+
+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 = "liteon,ltrf216a", },
+	{ .compatible = "ltr,ltrf216a", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
+
+static struct i2c_driver ltrf216a_driver = {
+	.driver = {
+		.name = LTRF216A_DRV_NAME,
+		.pm = pm_sleep_ptr(&ltrf216a_pm_ops),
+		.of_match_table = ltrf216a_of_match,
+	},
+	.probe_new	= ltrf216a_probe,
+	.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");
-- 
2.30.2

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Andy Shevchenko]

On Wed, May 11, 2022 at 1:11 PM Shreeya Patel
<shreeya.patel@collabora.com> wrote:
>
> From: Zhigang Shi <Zhigang.Shi@liteon.com>
>
> Add initial support for ltrf216a ambient light sensor.

> Datasheet: gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTRF216A.pdf

What is the protocol? https? git? ssh?

...

> +obj-$(CONFIG_LTRF216A)         += ltrf216a.o

I believe alphabetically it goes after the below one.

>  obj-$(CONFIG_LTR501)           += ltr501.o

...

> +#include <linux/bitfield.h>

+ bits.h

...

> +static const int ltrf216a_int_time_available[5][2] = {

You may drop 5 here and below, correct?

> +       {0, 400000},
> +       {0, 200000},
> +       {0, 100000},
> +       {0, 50000},
> +       {0, 25000},
> +};

...

> +/* open air. need to update based on TP transmission rate. */

I didn't get the small letters in conjunction with the period in the
middle of the phrase. Can you update grammar or spell the shortened
form of "air." fully?

> +#define WIN_FAC        1

Can it be namespaced?

...

> +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),
> +               .info_mask_separate_available =
> +                       BIT(IIO_CHAN_INFO_INT_TIME),
> +       }

+ Comma.

> +};

...

> +static int ltrf216a_set_int_time(struct ltrf216a_data *data, int itime)
> +{
> +       int i, ret, index = -1;

Redundant assignment, and actually not needed variable 'index', see below.

> +       u8 reg_val;
> +
> +       for (i = 0; i < ARRAY_SIZE(ltrf216a_int_time_available); i++) {
> +               if (ltrf216a_int_time_available[i][1] == itime) {
> +                       index = i;
> +                       break;
> +               }

if (...)
  break;

> +       }

> +

You can drop this blank line in order to show the tough connection
between for-loop and if-cond.

> +       if (index < 0)
> +               return -EINVAL;

if (i == ARRAY_SIZE(...))
  return ...

And use i here, which actually can be typed as unsigned int above.

> +       reg_val = ltrf216a_int_time_reg[index][1];
> +       data->int_time_fac = ltrf216a_int_time_reg[index][0];
> +
> +       ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RES, reg_val);
> +       if (ret < 0)
> +               return ret;
> +
> +       data->int_time = itime;
> +
> +       return 0;
> +}

...

> +static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr)
> +{
> +       int ret = -1, tries = 25;

No need to assign ret. And please avoid assigning returned values to
some negative values that may be misinterpreted as error codes.

> +       u8 buf[3];
> +
> +       while (tries--) {
> +               ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS);
> +               if (ret < 0)
> +                       return ret;
> +               if (ret & LTRF216A_ALS_DATA_STATUS)
> +                       break;
> +               msleep(20);
> +       }

NIH of a macro from iopoll.h. Use the appropriate one.

> +       ret = i2c_smbus_read_i2c_block_data(data->client, addr, sizeof(buf), buf);
> +       if (ret < 0)
> +               return ret;
> +
> +       return get_unaligned_le24(&buf[0]);
> +}

...

> +       greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
> +       cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
> +
> +       if (greendata < 0 || cleardata < 0)

> +               return -EINVAL;

I am expecting that each of them may contain the actual error code,
so, please decouple the condition and return the actual error codes.

...

> +       switch (mask) {
> +       case IIO_CHAN_INFO_PROCESSED:
> +               ret = ltrf216a_get_lux(data);
> +               if (ret < 0) {

> +                       mutex_unlock(&data->mutex);
> +                       return ret;

Wouldn't 'break' suffice?

> +               }
> +               *val = ret;
> +               ret = IIO_VAL_INT;
> +               break;
> +       case IIO_CHAN_INFO_INT_TIME:
> +               ret = ltrf216a_get_int_time(data, val, val2);
> +               break;
> +       default:
> +               ret = -EINVAL;

Missed break;.

> +       }
> +
> +       mutex_unlock(&data->mutex);
> +
> +       return ret;

...

> +       ret = ltrf216a_init(indio_dev);
> +       if (ret < 0)
> +               return dev_err_probe(&client->dev, ret,
> +                                    "ltrf216a chip init failed\n");

One line? Esp. if you create a temporary variable to hold a device pointer.

...

> +       ret = devm_add_action_or_reset(&client->dev, als_ltrf216a_disable,
> +                                      indio_dev);

Ditto.

> +       if (ret < 0)
> +               return ret;

...

> +static struct i2c_driver ltrf216a_driver = {
> +       .driver = {
> +               .name = LTRF216A_DRV_NAME,
> +               .pm = pm_sleep_ptr(&ltrf216a_pm_ops),
> +               .of_match_table = ltrf216a_of_match,
> +       },
> +       .probe_new      = ltrf216a_probe,
> +       .id_table       = ltrf216a_id,
> +};

> +

Redundant blank line.

> +module_i2c_driver(ltrf216a_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Dmitry Osipenko]

Hello Shreeya,

...
> +#include <linux/init.h>
> +#include <linux/mod_devicetable.h>

You may safely remove these includes because module.h always provides them.

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm.h>
> +#include <linux/iio/iio.h>
> +#include <asm/unaligned.h>
> +
> +#define LTRF216A_DRV_NAME "ltrf216a"

This define doesn't bring much benefits, you may use "ltrf216a" directly
in the code.

> +#define LTRF216A_MAIN_CTRL		0x00
> +
> +#define LTRF216A_ALS_DATA_STATUS	BIT(3)
> +#define LTRF216A_ALS_ENABLE_MASK	BIT(1)
> +
> +#define LTRF216A_ALS_MEAS_RES		0x04
> +#define LTRF216A_MAIN_STATUS		0x07
> +#define LTRF216A_CLEAR_DATA_0		0x0A
> +

Is any reason for separating these regs with a newline? If no, then you
may remove this newline.

> +#define LTRF216A_ALS_DATA_0		0x0D
> +

...
> +struct ltrf216a_data {
> +	struct i2c_client *client;
> +	u32 int_time;
> +	u16 int_time_fac;
> +	u8 als_gain_fac;
> +	/*
> +	 * Ensure cached value of integration time is consistent
> +	 * with hardware setting and remains constant during a
> +	 * measurement of Lux.
> +	 */
> +	struct mutex mutex;

I'd rename the "mutex" -> "lock".

> +};
> +
> +/* open air. need to update based on TP transmission rate. */
> +#define WIN_FAC	1

Usually all defines are placed before the code. You may move this define
before struct ltrf216a_data.

...
> +static int ltrf216a_init(struct iio_dev *indio_dev)
> +{
> +	int ret;
> +	struct ltrf216a_data *data = iio_priv(indio_dev);

Reverse Xmas tree is a common coding style in kernel for the function
variables.

******
****
**

Will be great if you could use the same coding style everywhere in this
source file, i.e. like this:

struct ltrf216a_data *data = iio_priv(indio_dev);
int ret;

> +	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 |= FIELD_PREP(LTRF216A_ALS_ENABLE_MASK, 1);
> +	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");

This message doesn't tell us the error code, which usually is very handy
to know.

I'd also change all the error messages in this driver to tell which
action failed, like this:

dev_err(dev, "Failed to enable sensor: %d\n", ret);

> +		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");

Please add error code to the message here too and reword it. This is
exactly the same error message as in ltrf216a_init(), so you can't
distinguish whether it's enabling or disabling that failed.

> +	return ret;
> +}
> +
> +static void als_ltrf216a_disable(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +
> +	ltrf216a_disable(indio_dev);
> +}
> +
> +static int ltrf216a_set_int_time(struct ltrf216a_data *data, int itime)
> +{
> +	int i, ret, index = -1;
> +	u8 reg_val;
> +
> +	for (i = 0; i < ARRAY_SIZE(ltrf216a_int_time_available); i++) {
> +		if (ltrf216a_int_time_available[i][1] == itime) {
> +			index = i;
> +			break;
> +		}
> +	}
> +
> +	if (index < 0)
> +		return -EINVAL;
> +
> +	reg_val = ltrf216a_int_time_reg[index][1];
> +	data->int_time_fac = ltrf216a_int_time_reg[index][0];
> +
> +	ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RES, reg_val);
> +	if (ret < 0)
> +		return ret;

Won't hurt to add error message here for consistency.

> +	data->int_time = itime;
> +
> +	return 0;
> +}
> +
> +static int ltrf216a_get_int_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 = -1, tries = 25;

No need to initialize the ret variable.

> +	u8 buf[3];
> +
> +	while (tries--) {
> +		ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS);
> +		if (ret < 0)
> +			return ret;

Need error message

> +		if (ret & LTRF216A_ALS_DATA_STATUS)
> +			break;
> +		msleep(20);
> +	}
> +
> +	ret = i2c_smbus_read_i2c_block_data(data->client, addr, sizeof(buf), buf);
> +	if (ret < 0)
> +		return ret;

Same here

..
> +MODULE_AUTHOR("Shi Zhigang <Zhigang.Shi@liteon.com>");

Driver could have multiple MODULE_AUTHOR(), you may add yourself here:

MODULE_AUTHOR("Shreeya Patel <shreeya.patel@collabora.com>")

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Jonathan Cameron]

On Fri, 13 May 2022 02:40:41 +0300
Dmitry Osipenko <digetx@gmail.com> wrote:

> Hello Shreeya,
> 
> ...
> > +#include <linux/init.h>
> > +#include <linux/mod_devicetable.h>  
> 
> You may safely remove these includes because module.h always provides them.

Safely yes, but generally I'd prefer direct include
of mod_devicetable.h because of the direct use of
elements from it in the driver.

Jonathan

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Dmitry Osipenko]

11.05.2022 12:40, Shreeya Patel пишет:
> +static int ltrf216a_probe(struct i2c_client *client)
> +{
> +	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 = &ltrf216a_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)
> +		return dev_err_probe(&client->dev, ret,
> +				     "ltrf216a chip init failed\n");

Is it possible to enable sensor only when measurement is made for more
power savings? Light sensor shouldn't consume much power, but nevertheless.

You'll need to add msleep(power_on_delay + resolution_rate_delay) after
enabling sensor and before reading the measurement to wait until
measurement is made by h/w.

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Dmitry Osipenko]

11.05.2022 12:40, Shreeya Patel пишет:
> +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 |= FIELD_PREP(LTRF216A_ALS_ENABLE_MASK, 1);
> +	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;
> +	}

Couldn't you write "1" directly without reading?

What about doing SW reset?

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Shreeya Patel]

On 13/05/22 05:24, Dmitry Osipenko wrote:

Hi Dmitry,

> 11.05.2022 12:40, Shreeya Patel пишет:
>> +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 |= FIELD_PREP(LTRF216A_ALS_ENABLE_MASK, 1);
>> +	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;
>> +	}
> Couldn't you write "1" directly without reading?
>
> What about doing SW reset?

I think we are doing a read here just to make sure device registers are 
ready and accessible
without any issues.

Also, why would we want to do a SW reset here?

In the datasheet, I could see the following steps to enable the sensor
Supply VDD to Sensor (Sensor in Standby Mode) ---> Wait 100 ms (min) - 
initial startup time
---> I2C Command (Write) To enable sensor to Active Mode

Thanks,
Shreeya Patel

>

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Shreeya Patel]

On 13/05/22 19:10, Shreeya Patel wrote:
>
> On 13/05/22 05:24, Dmitry Osipenko wrote:
>
> Hi Dmitry,
>
>> 11.05.2022 12:40, Shreeya Patel пишет:
>>> +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 |= FIELD_PREP(LTRF216A_ALS_ENABLE_MASK, 1);
>>> +    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;
>>> +    }
>> Couldn't you write "1" directly without reading?
>>
>> What about doing SW reset?
>
> I think we are doing a read here just to make sure device registers 
> are ready and accessible
> without any issues.
>
I just came to know that in I2C communication, writing a single bit 
requires reading the old value (whole byte),
modifying the result (i.e. set or clear the bit one is interested in) 
and then write it back. So the above code writes
the enable bit without modifying the other bits in the 
LTRF216A_MAIN_CTRL register. ( Thanks to Sebastian )

And you are right, we don't need to do a read here since we anyway want 
all other bits of LTRF216A_MAIN_CTRL
to be 0.

Thanks,
Shreeya Patel


> Also, why would we want to do a SW reset here?
>
> In the datasheet, I could see the following steps to enable the sensor
> Supply VDD to Sensor (Sensor in Standby Mode) ---> Wait 100 ms (min) - 
> initial startup time
> ---> I2C Command (Write) To enable sensor to Active Mode
>
> Thanks,
> Shreeya Patel
>
>>
>

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Dmitry Osipenko]

13.05.2022 16:40, Shreeya Patel пишет:
> Also, why would we want to do a SW reset here?
> 
> In the datasheet, I could see the following steps to enable the sensor
> Supply VDD to Sensor (Sensor in Standby Mode) ---> Wait 100 ms (min) -
> initial startup time
> ---> I2C Command (Write) To enable sensor to Active Mode

For example, if you'll do kexec from other kernel, say downstream
kernel, then the h/w state is undetermined for us. It's a common problem
with downstream drivers that they rely on a specific state left from
bootloader, which is often unacceptable for upstream.

If we'll do the SW reset on probe, then we'll bring h/w into the
predictable state and won't depend on state left from bootloader or
anything else that touched h/w before us.

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Dmitry Osipenko]

11.05.2022 12:40, Shreeya Patel пишет:
> +static int ltrf216a_get_lux(struct ltrf216a_data *data)
> +{
> +	int greendata, cleardata;
> +	u64 lux, div;
> +
> +	greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
> +	cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);

Could you please explain what is cleardata?

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Dmitry Osipenko]

11.05.2022 12:40, Shreeya Patel пишет:
> +static int ltrf216a_get_lux(struct ltrf216a_data *data)
> +{
> +	int greendata, cleardata;
> +	u64 lux, div;
> +
> +	greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
> +	cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
> +
> +	if (greendata < 0 || cleardata < 0)
> +		return -EINVAL;

-EIO should be more appropriate error code here

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Dmitry Osipenko]

11.05.2022 12:40, Shreeya Patel пишет:
> +static int ltrf216a_set_int_time(struct ltrf216a_data *data, int itime)
> +{
> +	int i, ret, index = -1;
> +	u8 reg_val;
> +
> +	for (i = 0; i < ARRAY_SIZE(ltrf216a_int_time_available); i++) {
> +		if (ltrf216a_int_time_available[i][1] == itime) {
> +			index = i;
> +			break;
> +		}
> +	}
> +
> +	if (index < 0)
> +		return -EINVAL;
> +
> +	reg_val = ltrf216a_int_time_reg[index][1];
> +	data->int_time_fac = ltrf216a_int_time_reg[index][0];
> +
> +	ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RES, reg_val);
> +	if (ret < 0)
> +		return ret;

Should the data->int_time_fac be updated only if I2C write was successful?

I'm not sure what reading of LTRF216A_CLEAR_DATA reg does, but if it
clears the measured data, then shouldn't the data be cleared after
changing the config?

> +	data->int_time = itime;
> +
> +	return 0;
> +}

Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Shreeya Patel]

On 13/05/22 06:00, Dmitry Osipenko wrote:
> 11.05.2022 12:40, Shreeya Patel пишет:
>> +static int ltrf216a_set_int_time(struct ltrf216a_data *data, int itime)
>> +{
>> +	int i, ret, index = -1;
>> +	u8 reg_val;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ltrf216a_int_time_available); i++) {
>> +		if (ltrf216a_int_time_available[i][1] == itime) {
>> +			index = i;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (index < 0)
>> +		return -EINVAL;
>> +
>> +	reg_val = ltrf216a_int_time_reg[index][1];
>> +	data->int_time_fac = ltrf216a_int_time_reg[index][0];
>> +
>> +	ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RES, reg_val);
>> +	if (ret < 0)
>> +		return ret;
> Should the data->int_time_fac be updated only if I2C write was successful?

Yes, thanks for pointing it out. It should be updated only if write is 
successful.

> I'm not sure what reading of LTRF216A_CLEAR_DATA reg does, but if it
> clears the measured data, then shouldn't the data be cleared after
> changing the config?

LTRF216A_CLEAR_DATA isn't used for clearing the measured data. Name is a
bit confusing and even I assumed it is being used for clearing the data. 
But from the
datasheet, it seems that clear data registers provides light intensity 
data related to infrared
and ultravoilet.
We are currently not using this anywhere in the driver so we could 
remove the cleardata readings
for now.


Thanks,
Shreeya Patel
>> +	data->int_time = itime;
>> +
>> +	return 0;
>> +}
>