[PATCH v1 0/4] Update APDS990x ALS to support IIO

[PATCH v1 0/4] Update APDS990x ALS to support IIO

[Svyatoslav Ryhel]

Add apds990x binding scheme, convert it to get basic data from
dts and use common IIO API. Since it works with IIO now, move from
/misc to /iio.

Svyatoslav Ryhel (4):
  dt-bindings: iio: light: add apds990x binding
  misc: adps990x: convert to OF
  misc: apds990x: convert to IIO
  iio: light: move apds990x into proper place

 .../bindings/iio/light/avago,apds990x.yaml    |  76 ++
 drivers/iio/light/Kconfig                     |  10 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/{misc => iio/light}/apds990x.c        | 802 +++++++++---------
 drivers/misc/Kconfig                          |  10 -
 drivers/misc/Makefile                         |   1 -
 6 files changed, 509 insertions(+), 391 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml
 rename drivers/{misc => iio/light}/apds990x.c (67%)

-- 
2.37.2

[PATCH v1 1/4] dt-bindings: iio: light: add apds990x binding

[Svyatoslav Ryhel]

Add dt-binding for apds990x ALS/proximity sensor.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../bindings/iio/light/avago,apds990x.yaml    | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml
new file mode 100644
index 000000000000..9b47e13f88e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/avago,apds990x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Avago APDS990x ALS and proximity sensor
+
+maintainers:
+  - Samu Onkalo <samu.p.onkalo@nokia.com>
+
+description: |
+  APDS990x is a combined ambient light and proximity sensor. ALS and
+  proximity functionality are highly connected. ALS measurement path
+  must be running while the proximity functionality is enabled.
+
+properties:
+  compatible:
+    const: avago,apds990x
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+  vled-supply: true
+
+  avago,pdrive:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 32
+    description: |
+      Drive value used in configuring control register.
+
+  avago,ppcount:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 32
+    description: |
+      Number of pulses used for proximity sensor calibration.
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupt
+  - vdd-supply
+  - vled-supply
+  - avago,pdrive
+  - avago,ppcount
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@39 {
+            compatible = "avago,apds990x";
+            reg = <0x39>;
+
+            interrupt-parent = <&gpio>;
+            interrupts = <82 IRQ_TYPE_EDGE_RISING>;
+
+            vdd-supply = <&vdd_3v0_proxi>;
+            vled-supply = <&vdd_1v8_sen>;
+
+            avago,pdrive = <0x00>;
+            avago,ppcount = <0x03>;
+        };
+    };
+...
-- 
2.37.2

Re: [PATCH v1 1/4] dt-bindings: iio: light: add apds990x binding

[Rob Herring]

On Wed, 08 Mar 2023 11:02:16 +0200, Svyatoslav Ryhel wrote:
> Add dt-binding for apds990x ALS/proximity sensor.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../bindings/iio/light/avago,apds990x.yaml    | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/light/avago,apds990x.example.dtb: light-sensor@39: 'interrupt' is a required property
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230308090219.12710-2-clamor95@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 1/4] dt-bindings: iio: light: add apds990x binding

[Jonathan Cameron]

On Wed,  8 Mar 2023 11:02:16 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> Add dt-binding for apds990x ALS/proximity sensor.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../bindings/iio/light/avago,apds990x.yaml    | 76 +++++++++++++++++++
I'm not a fan of wild cards. It breaks far too often.  Can we name this
instead after a particular supported part - same for compatible.
I'm not sure what parts are supported by this, but you may want multiple
compatibles.


>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml
> new file mode 100644
> index 000000000000..9b47e13f88e3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/avago,apds990x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Avago APDS990x ALS and proximity sensor
> +
> +maintainers:
> +  - Samu Onkalo <samu.p.onkalo@nokia.com>
> +
> +description: |
> +  APDS990x is a combined ambient light and proximity sensor. ALS and
> +  proximity functionality are highly connected. ALS measurement path
> +  must be running while the proximity functionality is enabled.
> +
> +properties:
> +  compatible:
> +    const: avago,apds990x
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +  vled-supply: true
> +
> +  avago,pdrive:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 32
> +    description: |
> +      Drive value used in configuring control register.

Is this something where there is a reasonable default?
If so I'd prefer it was optional so that the device is easier to
use without needing firmware description.

> +
> +  avago,ppcount:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 32
> +    description: |
> +      Number of pulses used for proximity sensor calibration.
Same for this - if there is a reasonable default it would be good to
have that specified.

> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt
It would nice to relax the need for an interrupt if the device is still useable
with timeouts etc.  Board folk have a habit of deciding they don't need to wire
up interrupts.  We can relax that a later date though if you prefer not to do
it now.
> +  - vdd-supply
> +  - vled-supply

Whilst true that the supplies need to be connected, that doesn't
mean they need to provided in the device tree binding.  If they are
always powered up I think we can fallback to stub regulators.

> +  - avago,pdrive
> +  - avago,ppcount
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        light-sensor@39 {
> +            compatible = "avago,apds990x";
> +            reg = <0x39>;
> +
> +            interrupt-parent = <&gpio>;
> +            interrupts = <82 IRQ_TYPE_EDGE_RISING>;
> +
> +            vdd-supply = <&vdd_3v0_proxi>;
> +            vled-supply = <&vdd_1v8_sen>;
> +
> +            avago,pdrive = <0x00>;
> +            avago,ppcount = <0x03>;
> +        };
> +    };
> +...

Re: [PATCH v1 1/4] dt-bindings: iio: light: add apds990x binding

[Krzysztof Kozlowski]

On 11/03/2023 20:34, Jonathan Cameron wrote:

>> +
>> +additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupt
> It would nice to relax the need for an interrupt if the device is still useable
> with timeouts etc.  Board folk have a habit of deciding they don't need to wire
> up interrupts.  We can relax that a later date though if you prefer not to do
> it now.
>> +  - vdd-supply
>> +  - vled-supply
> 
> Whilst true that the supplies need to be connected, that doesn't
> mean they need to provided in the device tree binding.  If they are
> always powered up I think we can fallback to stub regulators.

We can, but others might not. The binding should still require them if
they are required for device to work. Mark also made it clear recently:

https://lore.kernel.org/all/31ca0ede-012c-4849-bf25-d0492b116681@sirena.org.uk/
https://lore.kernel.org/all/5cd6764c-9b04-42ea-932d-9f14aa465605@sirena.org.uk/
https://lore.kernel.org/all/f6f02138-8ef9-4a33-9b51-0b7cd371230f@sirena.org.uk/

Best regards,
Krzysztof

Re: [PATCH v1 1/4] dt-bindings: iio: light: add apds990x binding

[Jonathan Cameron]

On Sun, 12 Mar 2023 11:47:19 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 11/03/2023 20:34, Jonathan Cameron wrote:
> 
> >> +
> >> +additionalProperties: false
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - interrupt  
> > It would nice to relax the need for an interrupt if the device is still useable
> > with timeouts etc.  Board folk have a habit of deciding they don't need to wire
> > up interrupts.  We can relax that a later date though if you prefer not to do
> > it now.  
> >> +  - vdd-supply
> >> +  - vled-supply  
> > 
> > Whilst true that the supplies need to be connected, that doesn't
> > mean they need to provided in the device tree binding.  If they are
> > always powered up I think we can fallback to stub regulators.  
> 
> We can, but others might not. The binding should still require them if
> they are required for device to work. Mark also made it clear recently:
> 
> https://lore.kernel.org/all/31ca0ede-012c-4849-bf25-d0492b116681@sirena.org.uk/
> https://lore.kernel.org/all/5cd6764c-9b04-42ea-932d-9f14aa465605@sirena.org.uk/
> https://lore.kernel.org/all/f6f02138-8ef9-4a33-9b51-0b7cd371230f@sirena.org.uk/


OK. Then there are a lot of bindings to fix. Seems odd to me but meh it's
not something I care about.

Note this means that we can't have trivial-device.yaml for instance.

Ah well, I guess views change or crystallise over time or just differed
in the first place.

Jonathan

> 
> Best regards,
> Krzysztof
> 

[PATCH v1 2/4] misc: adps990x: convert to OF

[Svyatoslav Ryhel]

Add ability to get essential values from device tree.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/misc/apds990x.c | 56 +++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/misc/apds990x.c b/drivers/misc/apds990x.c
index 0024503ea6db..c53ead5a575d 100644
--- a/drivers/misc/apds990x.c
+++ b/drivers/misc/apds990x.c
@@ -180,8 +180,8 @@ static const u16 arates_hz[] = {10, 5, 2, 1};
 static const u8 apersis[] = {1, 2, 4, 5};
 
 /* Regulators */
-static const char reg_vcc[] = "Vdd";
-static const char reg_vled[] = "Vled";
+static const char reg_vcc[] = "vdd";
+static const char reg_vled[] = "vled";
 
 static int apds990x_read_byte(struct apds990x_chip *chip, u8 reg, u8 *data)
 {
@@ -1048,9 +1048,38 @@ static struct attribute *sysfs_attrs_ctrl[] = {
 };
 
 static const struct attribute_group apds990x_attribute_group[] = {
-	{.attrs = sysfs_attrs_ctrl },
+	{ .attrs = sysfs_attrs_ctrl },
 };
 
+static int apds990x_of_probe(struct i2c_client *client,
+			     struct apds990x_chip *chip)
+{
+	struct apds990x_platform_data *pdata;
+	u32 ret, val;
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	ret = device_property_read_u32(&client->dev, "avago,pdrive", &val);
+	if (ret) {
+		dev_info(&client->dev, "pdrive property is missing: ret %d\n", ret);
+		return ret;
+	}
+	pdata->pdrive = val;
+
+	ret = device_property_read_u32(&client->dev, "avago,ppcount", &val);
+	if (ret) {
+		dev_info(&client->dev, "ppcount property is missing: ret %d\n", ret);
+		return ret;
+	}
+	pdata->ppcount = val;
+
+	chip->pdata = pdata;
+
+	return 0;
+}
+
 static int apds990x_probe(struct i2c_client *client)
 {
 	struct apds990x_chip *chip;
@@ -1065,13 +1094,10 @@ static int apds990x_probe(struct i2c_client *client)
 
 	init_waitqueue_head(&chip->wait);
 	mutex_init(&chip->mutex);
-	chip->pdata	= client->dev.platform_data;
 
-	if (chip->pdata == NULL) {
-		dev_err(&client->dev, "platform data is mandatory\n");
-		err = -EINVAL;
-		goto fail1;
-	}
+	chip->pdata = client->dev.platform_data;
+	if (!chip->pdata)
+		apds990x_of_probe(client, chip);
 
 	if (chip->pdata->cf.ga == 0) {
 		/* set uncovered sensor default parameters */
@@ -1160,8 +1186,7 @@ static int apds990x_probe(struct i2c_client *client)
 
 	err = request_threaded_irq(client->irq, NULL,
 				apds990x_irq,
-				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_LOW |
-				IRQF_ONESHOT,
+				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				"apds990x", chip);
 	if (err) {
 		dev_err(&client->dev, "could not get IRQ %d\n",
@@ -1252,11 +1277,16 @@ static int apds990x_runtime_resume(struct device *dev)
 
 #endif
 
+static const struct of_device_id apds990x_match_table[] = {
+	{ .compatible = "avago,apds990x" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, apds990x_match_table);
+
 static const struct i2c_device_id apds990x_id[] = {
 	{"apds990x", 0 },
 	{}
 };
-
 MODULE_DEVICE_TABLE(i2c, apds990x_id);
 
 static const struct dev_pm_ops apds990x_pm_ops = {
@@ -1270,12 +1300,12 @@ static struct i2c_driver apds990x_driver = {
 	.driver	 = {
 		.name	= "apds990x",
 		.pm	= &apds990x_pm_ops,
+		.of_match_table = apds990x_match_table,
 	},
 	.probe_new = apds990x_probe,
 	.remove	  = apds990x_remove,
 	.id_table = apds990x_id,
 };
-
 module_i2c_driver(apds990x_driver);
 
 MODULE_DESCRIPTION("APDS990X combined ALS and proximity sensor");
-- 
2.37.2

Re: [PATCH v1 2/4] misc: adps990x: convert to OF

[Jonathan Cameron]

On Wed,  8 Mar 2023 11:02:17 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> Add ability to get essential values from device tree.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

Hi,

Some comments inline.

> ---
>  drivers/misc/apds990x.c | 56 +++++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/misc/apds990x.c b/drivers/misc/apds990x.c
> index 0024503ea6db..c53ead5a575d 100644
> --- a/drivers/misc/apds990x.c
> +++ b/drivers/misc/apds990x.c
> @@ -180,8 +180,8 @@ static const u16 arates_hz[] = {10, 5, 2, 1};
>  static const u8 apersis[] = {1, 2, 4, 5};
>  
>  /* Regulators */
> -static const char reg_vcc[] = "Vdd";
> -static const char reg_vled[] = "Vled";
> +static const char reg_vcc[] = "vdd";
> +static const char reg_vled[] = "vled";

Doesn't this break existing users?  That's fine if we are dropping platform
data support, but this patch doesn't do that.

>  
>  static int apds990x_read_byte(struct apds990x_chip *chip, u8 reg, u8 *data)
>  {
> @@ -1048,9 +1048,38 @@ static struct attribute *sysfs_attrs_ctrl[] = {
>  };
>  
>  static const struct attribute_group apds990x_attribute_group[] = {
> -	{.attrs = sysfs_attrs_ctrl },
> +	{ .attrs = sysfs_attrs_ctrl },

As below - white space changes are fine, but not in the same patch as
functional stuff.  Just makes ti harder to read.

>  };
>  
> +static int apds990x_of_probe(struct i2c_client *client,
> +			     struct apds990x_chip *chip)
> +{
> +	struct apds990x_platform_data *pdata;
> +	u32 ret, val;
> +
> +	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_u32(&client->dev, "avago,pdrive", &val);
> +	if (ret) {
> +		dev_info(&client->dev, "pdrive property is missing: ret %d\n", ret);

Is this required to work?  If so dev_err_probe() is neater and make sure to handle the
error return at the caller.

> +		return ret;
> +	}
> +	pdata->pdrive = val;
> +
> +	ret = device_property_read_u32(&client->dev, "avago,ppcount", &val);
> +	if (ret) {
> +		dev_info(&client->dev, "ppcount property is missing: ret %d\n", ret);
> +		return ret;

As above.

> +	}
> +	pdata->ppcount = val;
> +
> +	chip->pdata = pdata;
> +
> +	return 0;
> +}
> +
>  static int apds990x_probe(struct i2c_client *client)
>  {
>  	struct apds990x_chip *chip;
> @@ -1065,13 +1094,10 @@ static int apds990x_probe(struct i2c_client *client)
>  
>  	init_waitqueue_head(&chip->wait);
>  	mutex_init(&chip->mutex);
> -	chip->pdata	= client->dev.platform_data;
>  
> -	if (chip->pdata == NULL) {
> -		dev_err(&client->dev, "platform data is mandatory\n");
> -		err = -EINVAL;
> -		goto fail1;
> -	}
> +	chip->pdata = client->dev.platform_data;

Are there known users of the platform data?  If not can we drop that whilst
doing this conversion?

> +	if (!chip->pdata)
> +		apds990x_of_probe(client, chip);

For a modern driver, don't make it of specific. There are other firmware
types that can use the of table you've added. Look for PRP0001 ACPI
bindings for example that use this.  So name this
apds990x_fw_probe() instead.  You have done all the stuff correctly
with the generic accessors so the actual function looks good to me.

Needs error handling as you can end up without pdata being set.


>  
>  	if (chip->pdata->cf.ga == 0) {
>  		/* set uncovered sensor default parameters */
> @@ -1160,8 +1186,7 @@ static int apds990x_probe(struct i2c_client *client)
>  
>  	err = request_threaded_irq(client->irq, NULL,
>  				apds990x_irq,
> -				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_LOW |
> -				IRQF_ONESHOT,
> +				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,

Ideally the irq type is a problem for device tree, not the driver
(that lets the DT deal with describing the affect of inverters etc in
the signal path).

That causes problems for old DT that assumes the driver is doing it.
I'd like to see a clear description of why this change is here in
the patch description (even better to pull it out to a separate patch
as not obviously connected to of conversion).


>  				"apds990x", chip);
>  	if (err) {
>  		dev_err(&client->dev, "could not get IRQ %d\n",
> @@ -1252,11 +1277,16 @@ static int apds990x_runtime_resume(struct device *dev)
>  
>  #endif
>  
> +static const struct of_device_id apds990x_match_table[] = {
> +	{ .compatible = "avago,apds990x" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, apds990x_match_table);
> +
>  static const struct i2c_device_id apds990x_id[] = {
>  	{"apds990x", 0 },
>  	{}
>  };
> -
>  MODULE_DEVICE_TABLE(i2c, apds990x_id);
>  
>  static const struct dev_pm_ops apds990x_pm_ops = {
> @@ -1270,12 +1300,12 @@ static struct i2c_driver apds990x_driver = {
>  	.driver	 = {
>  		.name	= "apds990x",
>  		.pm	= &apds990x_pm_ops,
> +		.of_match_table = apds990x_match_table,
>  	},
>  	.probe_new = apds990x_probe,
>  	.remove	  = apds990x_remove,
>  	.id_table = apds990x_id,
>  };
> -

Try to avoid unrelated white space changes in a patch that does something else.
Fine to tidy them all up in a separate patch though!

>  module_i2c_driver(apds990x_driver);
>  
>  MODULE_DESCRIPTION("APDS990X combined ALS and proximity sensor");

[PATCH v1 3/4] misc: apds990x: convert to IIO

[Svyatoslav Ryhel]

Convert old sysfs export to an IIO look.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/misc/apds990x.c | 794 ++++++++++++++++++++--------------------
 1 file changed, 403 insertions(+), 391 deletions(-)

diff --git a/drivers/misc/apds990x.c b/drivers/misc/apds990x.c
index c53ead5a575d..0352962d6d89 100644
--- a/drivers/misc/apds990x.c
+++ b/drivers/misc/apds990x.c
@@ -20,6 +20,9 @@
 #include <linux/slab.h>
 #include <linux/platform_data/apds990x.h>
 
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
 /* Register map */
 #define APDS990X_ENABLE	 0x00 /* Enable of states and interrupts */
 #define APDS990X_ATIME	 0x01 /* ALS ADC time  */
@@ -100,6 +103,21 @@
 
 #define APDS990X_LUX_OUTPUT_SCALE 10
 
+enum {
+	APDS990X_LUX_RANGE_ATTR = 1,
+	APDS990X_LUX_CALIB_FORMAT_ATTR,
+	APDS990X_LUX_CALIB_ATTR,
+	APDS990X_LUX_RATE_AVAIL_ATTR,
+	APDS990X_LUX_RATE_ATTR,
+	APDS990X_LUX_THRESH_ABOVE_ATTR,
+	APDS990X_LUX_THRESH_BELOW_ATTR,
+	APDS990X_PROX_SENSOR_RANGE_ATTR,
+	APDS990X_PROX_THRESH_ABOVE_VALUE_ATTR,
+	APDS990X_PROX_REPORTING_MODE_ATTR,
+	APDS990X_PROX_REPORTING_MODE_AVAIL_ATTR,
+	APDS990X_CHIP_ID_ATTR,
+};
+
 /* Reverse chip factors for threshold calculation */
 struct reverse_factors {
 	u32 afactor;
@@ -116,7 +134,7 @@ struct apds990x_chip {
 	struct regulator_bulk_data	regs[2];
 	wait_queue_head_t		wait;
 
-	int	prox_en;
+	bool	prox_en;
 	bool	prox_continuous_mode;
 	bool	lux_wait_fresh_res;
 
@@ -235,12 +253,8 @@ static int apds990x_write_word(struct apds990x_chip *chip, u8 reg, u16 data)
 
 static int apds990x_mode_on(struct apds990x_chip *chip)
 {
-	/* ALS is mandatory, proximity optional */
 	u8 reg = APDS990X_EN_AIEN | APDS990X_EN_PON | APDS990X_EN_AEN |
-		APDS990X_EN_WEN;
-
-	if (chip->prox_en)
-		reg |= APDS990X_EN_PIEN | APDS990X_EN_PEN;
+		APDS990X_EN_WEN | APDS990X_EN_PIEN | APDS990X_EN_PEN;
 
 	return apds990x_write_byte(chip, APDS990X_ENABLE, reg);
 }
@@ -473,7 +487,8 @@ static int apds990x_ack_int(struct apds990x_chip *chip, u8 mode)
 
 static irqreturn_t apds990x_irq(int irq, void *data)
 {
-	struct apds990x_chip *chip = data;
+	struct iio_dev *indio_dev = data;
+	struct apds990x_chip *chip = iio_priv(indio_dev);
 	u8 status;
 
 	apds990x_read_byte(chip, APDS990X_STATUS, &status);
@@ -498,12 +513,10 @@ static irqreturn_t apds990x_irq(int irq, void *data)
 				chip->lux = chip->lux_raw;
 				chip->lux_wait_fresh_res = false;
 				wake_up(&chip->wait);
-				sysfs_notify(&chip->client->dev.kobj,
-					NULL, "lux0_input");
 			}
 		}
 
-		if ((status & APDS990X_ST_PINT) && chip->prox_en) {
+		if (status & APDS990X_ST_PINT) {
 			u16 clr_ch;
 
 			apds990x_read_word(chip, APDS990X_CDATAL, &clr_ch);
@@ -525,8 +538,6 @@ static irqreturn_t apds990x_irq(int irq, void *data)
 				chip->prox_data = 0;
 			else if (!chip->prox_continuous_mode)
 				chip->prox_data = APDS_PROX_RANGE;
-			sysfs_notify(&chip->client->dev.kobj,
-				NULL, "prox0_raw");
 		}
 	}
 	mutex_unlock(&chip->mutex);
@@ -619,102 +630,67 @@ static int apds990x_chip_off(struct apds990x_chip *chip)
 	return 0;
 }
 
-static ssize_t apds990x_lux_show(struct device *dev,
-				 struct device_attribute *attr, char *buf)
-{
-	struct apds990x_chip *chip = dev_get_drvdata(dev);
-	ssize_t ret;
-	u32 result;
-	long timeout;
-
-	if (pm_runtime_suspended(dev))
-		return -EIO;
-
-	timeout = wait_event_interruptible_timeout(chip->wait,
-						!chip->lux_wait_fresh_res,
-						msecs_to_jiffies(APDS_TIMEOUT));
-	if (!timeout)
-		return -EIO;
-
-	mutex_lock(&chip->mutex);
-	result = (chip->lux * chip->lux_calib) / APDS_CALIB_SCALER;
-	if (result > (APDS_RANGE * APDS990X_LUX_OUTPUT_SCALE))
-		result = APDS_RANGE * APDS990X_LUX_OUTPUT_SCALE;
-
-	ret = sprintf(buf, "%d.%d\n",
-		result / APDS990X_LUX_OUTPUT_SCALE,
-		result % APDS990X_LUX_OUTPUT_SCALE);
-	mutex_unlock(&chip->mutex);
-	return ret;
-}
+static const char * const reporting_modes[] = { "trigger", "periodic" };
 
-static DEVICE_ATTR(lux0_input, S_IRUGO, apds990x_lux_show, NULL);
-
-static ssize_t apds990x_lux_range_show(struct device *dev,
-				 struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%u\n", APDS_RANGE);
-}
-
-static DEVICE_ATTR(lux0_sensor_range, S_IRUGO, apds990x_lux_range_show, NULL);
-
-static ssize_t apds990x_lux_calib_format_show(struct device *dev,
-				 struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%u\n", APDS_CALIB_SCALER);
-}
-
-static DEVICE_ATTR(lux0_calibscale_default, S_IRUGO,
-		apds990x_lux_calib_format_show, NULL);
-
-static ssize_t apds990x_lux_calib_show(struct device *dev,
-				 struct device_attribute *attr, char *buf)
+static ssize_t apds990x_lux_prox_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
 {
-	struct apds990x_chip *chip = dev_get_drvdata(dev);
-
-	return sprintf(buf, "%u\n", chip->lux_calib);
-}
-
-static ssize_t apds990x_lux_calib_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t len)
-{
-	struct apds990x_chip *chip = dev_get_drvdata(dev);
-	unsigned long value;
-	int ret;
-
-	ret = kstrtoul(buf, 0, &value);
-	if (ret)
-		return ret;
-
-	chip->lux_calib = value;
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct apds990x_chip *chip = iio_priv(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	int i, len = 0;
+
+	mutex_lock(&indio_dev->mlock);
+	switch ((u32)this_attr->address) {
+	case APDS990X_LUX_RANGE_ATTR:
+		len = sprintf(buf, "%u\n", APDS_RANGE);
+		break;
+	case APDS990X_LUX_CALIB_FORMAT_ATTR:
+		len = sprintf(buf, "%u\n", APDS_CALIB_SCALER);
+		break;
+	case APDS990X_LUX_CALIB_ATTR:
+		len = sprintf(buf, "%u\n", chip->lux_calib);
+		break;
+	case APDS990X_LUX_RATE_AVAIL_ATTR:
+		for (i = 0; i < ARRAY_SIZE(arates_hz); i++)
+			len += sprintf(buf + len, "%d ", arates_hz[i]);
+		len = sprintf(buf + len - 1, "\n");
+		break;
+	case APDS990X_LUX_RATE_ATTR:
+		len = sprintf(buf, "%d\n", chip->arate);
+		break;
+	case APDS990X_LUX_THRESH_ABOVE_ATTR:
+		len = sprintf(buf, "%d\n", chip->lux_thres_hi);
+		break;
+	case APDS990X_LUX_THRESH_BELOW_ATTR:
+		len = sprintf(buf, "%d\n", chip->lux_thres_lo);
+		break;
+	case APDS990X_PROX_SENSOR_RANGE_ATTR:
+		len = sprintf(buf, "%u\n", APDS_PROX_RANGE);
+		break;
+	case APDS990X_PROX_THRESH_ABOVE_VALUE_ATTR:
+		len = sprintf(buf, "%d\n", chip->prox_thres);
+		break;
+	case APDS990X_PROX_REPORTING_MODE_ATTR:
+		len = sprintf(buf, "%s\n",
+			      reporting_modes[!!chip->prox_continuous_mode]);
+		break;
+	case APDS990X_PROX_REPORTING_MODE_AVAIL_ATTR:
+		len = sprintf(buf, "%s %s\n",
+			      reporting_modes[0], reporting_modes[1]);
+		break;
+	case APDS990X_CHIP_ID_ATTR:
+		len = sprintf(buf, "%s %d\n", chip->chipname, chip->revision);
+		break;
+	default:
+		return -EINVAL;
+	}
 
+	mutex_unlock(&indio_dev->mlock);
 	return len;
 }
 
-static DEVICE_ATTR(lux0_calibscale, S_IRUGO | S_IWUSR, apds990x_lux_calib_show,
-		apds990x_lux_calib_store);
-
-static ssize_t apds990x_rate_avail(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	int i;
-	int pos = 0;
-
-	for (i = 0; i < ARRAY_SIZE(arates_hz); i++)
-		pos += sprintf(buf + pos, "%d ", arates_hz[i]);
-	sprintf(buf + pos - 1, "\n");
-	return pos;
-}
-
-static ssize_t apds990x_rate_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-
-	return sprintf(buf, "%d\n", chip->arate);
-}
-
 static int apds990x_set_arate(struct apds990x_chip *chip, int rate)
 {
 	int i;
@@ -740,154 +716,8 @@ static int apds990x_set_arate(struct apds990x_chip *chip, int rate)
 			(chip->prox_persistence << APDS990X_PPERS_SHIFT));
 }
 
-static ssize_t apds990x_rate_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t len)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-	unsigned long value;
-	int ret;
-
-	ret = kstrtoul(buf, 0, &value);
-	if (ret)
-		return ret;
-
-	mutex_lock(&chip->mutex);
-	ret = apds990x_set_arate(chip, value);
-	mutex_unlock(&chip->mutex);
-
-	if (ret < 0)
-		return ret;
-	return len;
-}
-
-static DEVICE_ATTR(lux0_rate_avail, S_IRUGO, apds990x_rate_avail, NULL);
-
-static DEVICE_ATTR(lux0_rate, S_IRUGO | S_IWUSR, apds990x_rate_show,
-						 apds990x_rate_store);
-
-static ssize_t apds990x_prox_show(struct device *dev,
-				 struct device_attribute *attr, char *buf)
-{
-	ssize_t ret;
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-
-	if (pm_runtime_suspended(dev) || !chip->prox_en)
-		return -EIO;
-
-	mutex_lock(&chip->mutex);
-	ret = sprintf(buf, "%d\n", chip->prox_data);
-	mutex_unlock(&chip->mutex);
-	return ret;
-}
-
-static DEVICE_ATTR(prox0_raw, S_IRUGO, apds990x_prox_show, NULL);
-
-static ssize_t apds990x_prox_range_show(struct device *dev,
-				 struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%u\n", APDS_PROX_RANGE);
-}
-
-static DEVICE_ATTR(prox0_sensor_range, S_IRUGO, apds990x_prox_range_show, NULL);
-
-static ssize_t apds990x_prox_enable_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-
-	return sprintf(buf, "%d\n", chip->prox_en);
-}
-
-static ssize_t apds990x_prox_enable_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t len)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-	unsigned long value;
-	int ret;
-
-	ret = kstrtoul(buf, 0, &value);
-	if (ret)
-		return ret;
-
-	mutex_lock(&chip->mutex);
-
-	if (!chip->prox_en)
-		chip->prox_data = 0;
-
-	if (value)
-		chip->prox_en++;
-	else if (chip->prox_en > 0)
-		chip->prox_en--;
-
-	if (!pm_runtime_suspended(dev))
-		apds990x_mode_on(chip);
-	mutex_unlock(&chip->mutex);
-	return len;
-}
-
-static DEVICE_ATTR(prox0_raw_en, S_IRUGO | S_IWUSR, apds990x_prox_enable_show,
-						   apds990x_prox_enable_store);
-
-static const char *reporting_modes[] = {"trigger", "periodic"};
-
-static ssize_t apds990x_prox_reporting_mode_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-
-	return sprintf(buf, "%s\n",
-		reporting_modes[!!chip->prox_continuous_mode]);
-}
-
-static ssize_t apds990x_prox_reporting_mode_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t len)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-	int ret;
-
-	ret = sysfs_match_string(reporting_modes, buf);
-	if (ret < 0)
-		return ret;
-
-	chip->prox_continuous_mode = ret;
-	return len;
-}
-
-static DEVICE_ATTR(prox0_reporting_mode, S_IRUGO | S_IWUSR,
-		apds990x_prox_reporting_mode_show,
-		apds990x_prox_reporting_mode_store);
-
-static ssize_t apds990x_prox_reporting_avail_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%s %s\n", reporting_modes[0], reporting_modes[1]);
-}
-
-static DEVICE_ATTR(prox0_reporting_mode_avail, S_IRUGO | S_IWUSR,
-		apds990x_prox_reporting_avail_show, NULL);
-
-
-static ssize_t apds990x_lux_thresh_above_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-
-	return sprintf(buf, "%d\n", chip->lux_thres_hi);
-}
-
-static ssize_t apds990x_lux_thresh_below_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-
-	return sprintf(buf, "%d\n", chip->lux_thres_lo);
-}
-
-static ssize_t apds990x_set_lux_thresh(struct apds990x_chip *chip, u32 *target,
-				const char *buf)
+static int apds990x_set_lux_thresh(struct apds990x_chip *chip, u32 *target,
+				   const char *buf)
 {
 	unsigned long thresh;
 	int ret;
@@ -908,98 +738,165 @@ static ssize_t apds990x_set_lux_thresh(struct apds990x_chip *chip, u32 *target,
 	if (!chip->lux_wait_fresh_res)
 		apds990x_refresh_athres(chip);
 	mutex_unlock(&chip->mutex);
-	return ret;
 
+	return ret;
 }
 
-static ssize_t apds990x_lux_thresh_above_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t len)
+static ssize_t apds990x_lux_prox_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t len)
 {
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-	int ret = apds990x_set_lux_thresh(chip, &chip->lux_thres_hi, buf);
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct apds990x_chip *chip = iio_priv(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	unsigned long value;
+	int ret;
 
-	if (ret < 0)
+	ret = kstrtoul(buf, 0, &value);
+	if (ret)
 		return ret;
-	return len;
-}
 
-static ssize_t apds990x_lux_thresh_below_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t len)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-	int ret = apds990x_set_lux_thresh(chip, &chip->lux_thres_lo, buf);
-
-	if (ret < 0)
-		return ret;
-	return len;
-}
+	mutex_lock(&indio_dev->mlock);
+	switch ((u32)this_attr->address) {
+	case APDS990X_LUX_CALIB_ATTR:
+		chip->lux_calib = value;
+		break;
+	case APDS990X_LUX_RATE_ATTR:
+		mutex_lock(&chip->mutex);
+		ret = apds990x_set_arate(chip, value);
+		mutex_unlock(&chip->mutex);
 
-static DEVICE_ATTR(lux0_thresh_above_value, S_IRUGO | S_IWUSR,
-		apds990x_lux_thresh_above_show,
-		apds990x_lux_thresh_above_store);
+		if (ret < 0)
+			return ret;
+		break;
+	case APDS990X_LUX_THRESH_ABOVE_ATTR:
+		ret = apds990x_set_lux_thresh(chip,
+					      &chip->lux_thres_hi, buf);
 
-static DEVICE_ATTR(lux0_thresh_below_value, S_IRUGO | S_IWUSR,
-		apds990x_lux_thresh_below_show,
-		apds990x_lux_thresh_below_store);
+		if (ret < 0)
+			return ret;
+		break;
+	case APDS990X_LUX_THRESH_BELOW_ATTR:
+		ret = apds990x_set_lux_thresh(chip,
+					      &chip->lux_thres_lo, buf);
 
-static ssize_t apds990x_prox_threshold_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
+		if (ret < 0)
+			return ret;
+		break;
+	case APDS990X_PROX_THRESH_ABOVE_VALUE_ATTR:
+		if (value > APDS_RANGE || value == 0 ||
+		    value < APDS_PROX_HYSTERESIS)
+			return -EINVAL;
 
-	return sprintf(buf, "%d\n", chip->prox_thres);
-}
+		mutex_lock(&chip->mutex);
 
-static ssize_t apds990x_prox_threshold_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t len)
-{
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-	unsigned long value;
-	int ret;
+		chip->prox_thres = value;
+		apds990x_force_p_refresh(chip);
 
-	ret = kstrtoul(buf, 0, &value);
-	if (ret)
-		return ret;
+		mutex_unlock(&chip->mutex);
+		break;
+	case APDS990X_PROX_REPORTING_MODE_ATTR:
+		ret = sysfs_match_string(reporting_modes, buf);
+		if (ret < 0)
+			return ret;
 
-	if ((value > APDS_RANGE) || (value == 0) ||
-		(value < APDS_PROX_HYSTERESIS))
+		chip->prox_continuous_mode = ret;
+		break;
+	default:
 		return -EINVAL;
+	}
 
-	mutex_lock(&chip->mutex);
-	chip->prox_thres = value;
-
-	apds990x_force_p_refresh(chip);
-	mutex_unlock(&chip->mutex);
+	mutex_unlock(&indio_dev->mlock);
 	return len;
 }
 
-static DEVICE_ATTR(prox0_thresh_above_value, S_IRUGO | S_IWUSR,
-		apds990x_prox_threshold_show,
-		apds990x_prox_threshold_store);
+/* ALS ATTRIBUTES */
+static IIO_DEVICE_ATTR(in_illuminance_range, 0444,
+			apds990x_lux_prox_show,
+			NULL,
+			APDS990X_LUX_RANGE_ATTR);
+
+static IIO_DEVICE_ATTR(in_illuminance_calib_format, 0444,
+			apds990x_lux_prox_show,
+			NULL,
+			APDS990X_LUX_CALIB_FORMAT_ATTR);
+
+static IIO_DEVICE_ATTR(in_illuminance_calibscale, 0644,
+			apds990x_lux_prox_show,
+			apds990x_lux_prox_store,
+			APDS990X_LUX_CALIB_ATTR);
+
+static IIO_DEVICE_ATTR(in_illuminance_rate_avail, 0444,
+			apds990x_lux_prox_show,
+			NULL,
+			APDS990X_LUX_RATE_AVAIL_ATTR);
+
+static IIO_DEVICE_ATTR(in_illuminance_rate, 0644,
+			apds990x_lux_prox_show,
+			apds990x_lux_prox_store,
+			APDS990X_LUX_RATE_ATTR);
+
+static IIO_DEVICE_ATTR(in_illuminance_thresh_above_value, 0644,
+			apds990x_lux_prox_show,
+			apds990x_lux_prox_store,
+			APDS990X_LUX_THRESH_ABOVE_ATTR);
+
+static IIO_DEVICE_ATTR(in_illuminance_thresh_below_value, 0644,
+			apds990x_lux_prox_show,
+			apds990x_lux_prox_store,
+			APDS990X_LUX_THRESH_BELOW_ATTR);
+
+/* PROX ATTRIBUTES */
+static IIO_DEVICE_ATTR(in_proximity_sensor_range, 0444,
+			apds990x_lux_prox_show,
+			NULL,
+			APDS990X_PROX_SENSOR_RANGE_ATTR);
+
+static IIO_DEVICE_ATTR(in_proximity_reporting_mode, 0644,
+			apds990x_lux_prox_show,
+			apds990x_lux_prox_store,
+			APDS990X_PROX_REPORTING_MODE_ATTR);
+
+static IIO_DEVICE_ATTR(in_proximity_reporting_mode_avail, 0644,
+			apds990x_lux_prox_show,
+			NULL,
+			APDS990X_PROX_REPORTING_MODE_AVAIL_ATTR);
+
+static IIO_DEVICE_ATTR(in_proximity_thresh_above_value, 0644,
+			apds990x_lux_prox_show,
+			apds990x_lux_prox_store,
+			APDS990X_PROX_THRESH_ABOVE_VALUE_ATTR);
+
+static IIO_DEVICE_ATTR(chip_id, 0444,
+			apds990x_lux_prox_show,
+			NULL,
+			APDS990X_CHIP_ID_ATTR);
+
+static struct attribute *apds990x_attributes[] = {
+	&iio_dev_attr_in_illuminance_calib_format.dev_attr.attr,
+	&iio_dev_attr_in_illuminance_range.dev_attr.attr,
+	&iio_dev_attr_in_illuminance_calibscale.dev_attr.attr,
+	&iio_dev_attr_in_illuminance_rate.dev_attr.attr,
+	&iio_dev_attr_in_illuminance_rate_avail.dev_attr.attr,
+	&iio_dev_attr_in_illuminance_thresh_above_value.dev_attr.attr,
+	&iio_dev_attr_in_illuminance_thresh_below_value.dev_attr.attr,
+	&iio_dev_attr_in_proximity_sensor_range.dev_attr.attr,
+	&iio_dev_attr_in_proximity_thresh_above_value.dev_attr.attr,
+	&iio_dev_attr_in_proximity_reporting_mode.dev_attr.attr,
+	&iio_dev_attr_in_proximity_reporting_mode_avail.dev_attr.attr,
+	&iio_dev_attr_chip_id.dev_attr.attr,
+	NULL
+};
 
-static ssize_t apds990x_power_state_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%d\n", !pm_runtime_suspended(dev));
-	return 0;
-}
+static const struct attribute_group apds990x_attribute_group = {
+	.attrs = apds990x_attributes,
+};
 
-static ssize_t apds990x_power_state_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t len)
+static void apds990x_power_state_switch(struct apds990x_chip *chip, bool state)
 {
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
-	unsigned long value;
-	int ret;
+	struct device *dev = &chip->client->dev;
 
-	ret = kstrtoul(buf, 0, &value);
-	if (ret)
-		return ret;
-
-	if (value) {
+	if (state) {
 		pm_runtime_get_sync(dev);
 		mutex_lock(&chip->mutex);
 		chip->lux_wait_fresh_res = true;
@@ -1010,46 +907,80 @@ static ssize_t apds990x_power_state_store(struct device *dev,
 		if (!pm_runtime_suspended(dev))
 			pm_runtime_put(dev);
 	}
-	return len;
 }
 
-static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
-		apds990x_power_state_show,
-		apds990x_power_state_store);
-
-static ssize_t apds990x_chip_id_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
+static int apds990x_lux_raw(struct apds990x_chip *chip)
 {
-	struct apds990x_chip *chip =  dev_get_drvdata(dev);
+	struct device *dev = &chip->client->dev;
+	int ret;
+	long timeout;
+
+	if (pm_runtime_suspended(dev))
+		return -EIO;
+
+	timeout = wait_event_interruptible_timeout(chip->wait,
+						   !chip->lux_wait_fresh_res,
+						   msecs_to_jiffies(APDS_TIMEOUT));
+	if (!timeout)
+		return -EIO;
+
+	mutex_lock(&chip->mutex);
+
+	ret = (chip->lux * chip->lux_calib) / APDS_CALIB_SCALER;
+	if (ret > (APDS_RANGE * APDS990X_LUX_OUTPUT_SCALE))
+		ret = APDS_RANGE * APDS990X_LUX_OUTPUT_SCALE;
 
-	return sprintf(buf, "%s %d\n", chip->chipname, chip->revision);
+	mutex_unlock(&chip->mutex);
+
+	return ret;
 }
 
-static DEVICE_ATTR(chip_id, S_IRUGO, apds990x_chip_id_show, NULL);
-
-static struct attribute *sysfs_attrs_ctrl[] = {
-	&dev_attr_lux0_calibscale.attr,
-	&dev_attr_lux0_calibscale_default.attr,
-	&dev_attr_lux0_input.attr,
-	&dev_attr_lux0_sensor_range.attr,
-	&dev_attr_lux0_rate.attr,
-	&dev_attr_lux0_rate_avail.attr,
-	&dev_attr_lux0_thresh_above_value.attr,
-	&dev_attr_lux0_thresh_below_value.attr,
-	&dev_attr_prox0_raw_en.attr,
-	&dev_attr_prox0_raw.attr,
-	&dev_attr_prox0_sensor_range.attr,
-	&dev_attr_prox0_thresh_above_value.attr,
-	&dev_attr_prox0_reporting_mode.attr,
-	&dev_attr_prox0_reporting_mode_avail.attr,
-	&dev_attr_chip_id.attr,
-	&dev_attr_power_state.attr,
-	NULL
-};
+static int apds990x_prox_raw(struct apds990x_chip *chip)
+{
+	struct device *dev = &chip->client->dev;
+	long timeout;
+	u16 clr_ch;
 
-static const struct attribute_group apds990x_attribute_group[] = {
-	{ .attrs = sysfs_attrs_ctrl },
-};
+	if (!chip->prox_en) {
+		chip->prox_data = 0;
+		return chip->prox_data;
+	}
+
+	if (pm_runtime_suspended(dev))
+		return -EIO;
+
+	timeout = wait_event_interruptible_timeout(chip->wait,
+						   !chip->lux_wait_fresh_res,
+						   msecs_to_jiffies(APDS_TIMEOUT));
+	if (!timeout)
+		return -EIO;
+
+	mutex_lock(&chip->mutex);
+
+	apds990x_read_word(chip, APDS990X_CDATAL, &clr_ch);
+	/*
+	 * If ALS channel is saturated at min gain,
+	 * proximity gives false posivite values.
+	 * Just ignore them.
+	 */
+	if (chip->again_meas == 0 &&
+	    clr_ch == chip->a_max_result)
+		chip->prox_data = 0;
+	else
+		apds990x_read_word(chip,
+				   APDS990X_PDATAL,
+				   &chip->prox_data);
+
+	apds990x_refresh_pthres(chip, chip->prox_data);
+	if (chip->prox_data < chip->prox_thres)
+		chip->prox_data = 0;
+	else if (!chip->prox_continuous_mode)
+		chip->prox_data = 1;
+
+	mutex_unlock(&chip->mutex);
+
+	return chip->prox_data;
+}
 
 static int apds990x_of_probe(struct i2c_client *client,
 			     struct apds990x_chip *chip)
@@ -1080,15 +1011,114 @@ static int apds990x_of_probe(struct i2c_client *client,
 	return 0;
 }
 
+static int apds990x_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int *val,
+			     int *val2, long mask)
+{
+	struct apds990x_chip *chip = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		apds990x_power_state_switch(chip, true);
+
+		switch (chan->type) {
+		case IIO_LIGHT:
+			*val = apds990x_lux_raw(chip);
+			break;
+		case IIO_PROXIMITY:
+			*val = apds990x_prox_raw(chip);
+			break;
+		default:
+			break;
+		}
+
+		apds990x_power_state_switch(chip, false);
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_LIGHT:
+		case IIO_PROXIMITY:
+		default:
+			*val = 1;
+			break;
+		}
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_ENABLE:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			*val = chip->prox_en;
+		default:
+			break;
+		}
+
+		return IIO_VAL_INT;
+	}
+	return -EINVAL;
+}
+
+static int apds990x_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int val,
+			      int val2, long mask)
+{
+	struct apds990x_chip *chip = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_ENABLE:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			mutex_lock(&chip->mutex);
+			chip->prox_en = val;
+			mutex_unlock(&chip->mutex);
+		default:
+			break;
+		}
+
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static const struct iio_chan_spec apds990x_channels[] = {
+	{
+		.type	= IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	},
+	{
+		.type	= IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_ENABLE),
+	},
+};
+
+static const struct iio_info apds990x_info = {
+	.attrs		= &apds990x_attribute_group,
+	.read_raw	= apds990x_read_raw,
+	.write_raw	= apds990x_write_raw,
+};
+
 static int apds990x_probe(struct i2c_client *client)
 {
 	struct apds990x_chip *chip;
+	struct iio_dev *indio_dev;
 	int err;
 
-	chip = kzalloc(sizeof *chip, GFP_KERNEL);
-	if (!chip)
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
+	if (!indio_dev)
 		return -ENOMEM;
 
+	indio_dev->info = &apds990x_info;
+	indio_dev->name = "apds990x";
+	indio_dev->channels = apds990x_channels;
+	indio_dev->num_channels = ARRAY_SIZE(apds990x_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	chip = iio_priv(indio_dev);
 	i2c_set_clientdata(client, chip);
 	chip->client  = client;
 
@@ -1140,17 +1170,17 @@ static int apds990x_probe(struct i2c_client *client)
 	chip->regs[0].supply = reg_vcc;
 	chip->regs[1].supply = reg_vled;
 
-	err = regulator_bulk_get(&client->dev,
-				 ARRAY_SIZE(chip->regs), chip->regs);
+	err = devm_regulator_bulk_get(&client->dev,
+				      ARRAY_SIZE(chip->regs), chip->regs);
 	if (err < 0) {
 		dev_err(&client->dev, "Cannot get regulators\n");
-		goto fail1;
+		return err;
 	}
 
 	err = regulator_bulk_enable(ARRAY_SIZE(chip->regs), chip->regs);
 	if (err < 0) {
 		dev_err(&client->dev, "Cannot enable regulators\n");
-		goto fail2;
+		return err;
 	}
 
 	usleep_range(APDS_STARTUP_DELAY, 2 * APDS_STARTUP_DELAY);
@@ -1158,7 +1188,7 @@ static int apds990x_probe(struct i2c_client *client)
 	err = apds990x_detect(chip);
 	if (err < 0) {
 		dev_err(&client->dev, "APDS990X not found\n");
-		goto fail3;
+		return err;
 	}
 
 	pm_runtime_set_active(&client->dev);
@@ -1173,39 +1203,29 @@ static int apds990x_probe(struct i2c_client *client)
 		err = chip->pdata->setup_resources();
 		if (err) {
 			err = -EINVAL;
-			goto fail3;
+			goto fail;
 		}
 	}
 
-	err = sysfs_create_group(&chip->client->dev.kobj,
-				apds990x_attribute_group);
-	if (err < 0) {
-		dev_err(&chip->client->dev, "Sysfs registration failed\n");
-		goto fail4;
-	}
-
-	err = request_threaded_irq(client->irq, NULL,
-				apds990x_irq,
-				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-				"apds990x", chip);
+	err = devm_request_threaded_irq(&client->dev, client->irq,
+					NULL, apds990x_irq,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"apds990x", indio_dev);
 	if (err) {
 		dev_err(&client->dev, "could not get IRQ %d\n",
 			client->irq);
-		goto fail5;
+		goto fail;
 	}
-	return err;
-fail5:
-	sysfs_remove_group(&chip->client->dev.kobj,
-			&apds990x_attribute_group[0]);
-fail4:
+
+	err = iio_device_register(indio_dev);
+	if (err)
+		goto fail;
+
+	return 0;
+fail:
 	if (chip->pdata && chip->pdata->release_resources)
 		chip->pdata->release_resources();
-fail3:
-	regulator_bulk_disable(ARRAY_SIZE(chip->regs), chip->regs);
-fail2:
-	regulator_bulk_free(ARRAY_SIZE(chip->regs), chip->regs);
-fail1:
-	kfree(chip);
+
 	return err;
 }
 
@@ -1213,10 +1233,6 @@ static void apds990x_remove(struct i2c_client *client)
 {
 	struct apds990x_chip *chip = i2c_get_clientdata(client);
 
-	free_irq(client->irq, chip);
-	sysfs_remove_group(&chip->client->dev.kobj,
-			apds990x_attribute_group);
-
 	if (chip->pdata && chip->pdata->release_resources)
 		chip->pdata->release_resources();
 
@@ -1225,10 +1241,6 @@ static void apds990x_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-
-	regulator_bulk_free(ARRAY_SIZE(chip->regs), chip->regs);
-
-	kfree(chip);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.37.2

Re: [PATCH v1 3/4] misc: apds990x: convert to IIO

[Jonathan Cameron]

On Wed,  8 Mar 2023 11:02:18 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> Convert old sysfs export to an IIO look.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

This needs ABI documentation for all the custom ABI.
Documentation/ABI/testing/sysfs-bus-iio-adps9900

Note that we generally reluctant to add custom ABI if we can possibly
avoid it because it can't be used by generic code.
We are open to adding new standard ABI, but for that we need to clearly understand
the current gap.

Things are more complex for moves from other subsystems because
of a need for backwards compatibility.  That does concern me here as
there may be users who are relying on the misc interface who will be completely
broken by that changing.  Do we have good reason to assume that won't
be a problem here?

Jonathan



> ---
>  drivers/misc/apds990x.c | 794 ++++++++++++++++++++--------------------
>  1 file changed, 403 insertions(+), 391 deletions(-)
> 
> diff --git a/drivers/misc/apds990x.c b/drivers/misc/apds990x.c
> index c53ead5a575d..0352962d6d89 100644
> --- a/drivers/misc/apds990x.c
> +++ b/drivers/misc/apds990x.c
> @@ -20,6 +20,9 @@
>  #include <linux/slab.h>
>  #include <linux/platform_data/apds990x.h>
>  
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
>  /* Register map */
>  #define APDS990X_ENABLE	 0x00 /* Enable of states and interrupts */
>  #define APDS990X_ATIME	 0x01 /* ALS ADC time  */
> @@ -100,6 +103,21 @@
>  
>  #define APDS990X_LUX_OUTPUT_SCALE 10
>  
> +enum {
> +	APDS990X_LUX_RANGE_ATTR = 1,
> +	APDS990X_LUX_CALIB_FORMAT_ATTR,
> +	APDS990X_LUX_CALIB_ATTR,
> +	APDS990X_LUX_RATE_AVAIL_ATTR,
> +	APDS990X_LUX_RATE_ATTR,
> +	APDS990X_LUX_THRESH_ABOVE_ATTR,
> +	APDS990X_LUX_THRESH_BELOW_ATTR,
> +	APDS990X_PROX_SENSOR_RANGE_ATTR,
> +	APDS990X_PROX_THRESH_ABOVE_VALUE_ATTR,
> +	APDS990X_PROX_REPORTING_MODE_ATTR,
> +	APDS990X_PROX_REPORTING_MODE_AVAIL_ATTR,
> +	APDS990X_CHIP_ID_ATTR,
See below.  I'm not keen on the approach of one attr callback.
There is very little shared code, so I'd rather just see
individual minimal callbacks.

> +};
> +
>  /* Reverse chip factors for threshold calculation */
>  struct reverse_factors {
>  	u32 afactor;
> @@ -116,7 +134,7 @@ struct apds990x_chip {
>  	struct regulator_bulk_data	regs[2];
>  	wait_queue_head_t		wait;
>  
> -	int	prox_en;
> +	bool	prox_en;

This change seems unrelated to the interface change, so I'd prefer to
see it as a separate patch.  However, I'm not keen on channel enable being
used for a proximity sensor.  Userspace generally only deals with specific
channel enables for functions that count over time - so things like step
counters on IMUs.  For proximity channels the fact someone is reading it
is usually the 'enable'.  If the enable is slow, then runtime_pm autosuspend
type approaches can be used to disable it only if no one reads it for a while.

>  	bool	prox_continuous_mode;
>  	bool	lux_wait_fresh_res;
>  
> @@ -235,12 +253,8 @@ static int apds990x_write_word(struct apds990x_chip *chip, u8 reg, u16 data)
>  
>  static int apds990x_mode_on(struct apds990x_chip *chip)

Not part of your patch  but mode_on is not (to me) clear naming
for what this function is doing.  If it's turning the device on
then apds990x_enable() or adps990x_xxx_enable() with description of what
is being enabled would be clearer.

>  {
> -	/* ALS is mandatory, proximity optional */
>  	u8 reg = APDS990X_EN_AIEN | APDS990X_EN_PON | APDS990X_EN_AEN |
> -		APDS990X_EN_WEN;
> -
> -	if (chip->prox_en)
> -		reg |= APDS990X_EN_PIEN | APDS990X_EN_PEN;
> +		APDS990X_EN_WEN | APDS990X_EN_PIEN | APDS990X_EN_PEN;
>  
>  	return apds990x_write_byte(chip, APDS990X_ENABLE, reg);
>  }

...


> +static const char * const reporting_modes[] = { "trigger", "periodic" };

This sort of operating mode switch is effectively useless to generic userspace.
There are too many ways this is described on datasheets and different ways
devices implement these modes (some devices have lots of different modes) to
be able to design a useful generic userspace interface.

What we try to do is to make that decision automatically based on the userspace
interfaces used. Now I haven't looked in detail, so I'm basing the following
no an educated guess as to what these mean.

Normally a triggered mode is the state used when doing slow polled reads
(sysfs reads via read_raw()).  A periodic mode is that one that should be used
either when events are enabled (threshold detection etc) or when we are using
the buffered modes that IIO supports (access via a character device)

Given there is usually a clean mapping to how it is being used, we don't
expose the details of what the device 'mode' is.
The same applies to things like low power modes that affect latency of
readings.

> +static ssize_t apds990x_lux_prox_show(struct device *dev,

This is clearly doing a lot more than showing lux so is not a good name
for the function.
As a side note, lux is the unit not a thing to measure, so it should be
illuminance if we were talking about the thing measured in lux. 

> +				      struct device_attribute *attr,
> +				      char *buf)
>  {
> -	struct apds990x_chip *chip = dev_get_drvdata(dev);
> -
> -	return sprintf(buf, "%u\n", chip->lux_calib);
> -}
> -
> -static ssize_t apds990x_lux_calib_store(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buf, size_t len)
> -{
> -	struct apds990x_chip *chip = dev_get_drvdata(dev);
> -	unsigned long value;
> -	int ret;
> -
> -	ret = kstrtoul(buf, 0, &value);
> -	if (ret)
> -		return ret;
> -
> -	chip->lux_calib = value;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct apds990x_chip *chip = iio_priv(indio_dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	int i, len = 0;
> +
> +	mutex_lock(&indio_dev->mlock);
> +	switch ((u32)this_attr->address) {
> +	case APDS990X_LUX_RANGE_ATTR:
> +		len = sprintf(buf, "%u\n", APDS_RANGE);

If we ignore the fact this is a lot of custom ABI which is not a good
thing...

I don't see a good reason for having this as one big hydra of a function.
Just split it up for the individual attrs.  There is very very little shared
in most of these cases. Also there is no need to take the lock in some of
them.

> +		break;
> +	case APDS990X_LUX_CALIB_FORMAT_ATTR:
> +		len = sprintf(buf, "%u\n", APDS_CALIB_SCALER);
> +		break;
> +	case APDS990X_LUX_CALIB_ATTR:
> +		len = sprintf(buf, "%u\n", chip->lux_calib);

Calibration is one area where we do allow more custom ABI because it
can be very device specific.  So this needs documentation and should look
as close as possible to other sensors with calibration controls.

> +		break;
> +	case APDS990X_LUX_RATE_AVAIL_ATTR:
> +		for (i = 0; i < ARRAY_SIZE(arates_hz); i++)
> +			len += sprintf(buf + len, "%d ", arates_hz[i]);

It's not called rate in the IIO ABI.  sampling_frequency is what
you should use and that has read_avail() to provide a list of
available sampling frequencies.

> +		len = sprintf(buf + len - 1, "\n");
> +		break;
> +	case APDS990X_LUX_RATE_ATTR:
> +		len = sprintf(buf, "%d\n", chip->arate);
> +		break;
> +	case APDS990X_LUX_THRESH_ABOVE_ATTR:
> +		len = sprintf(buf, "%d\n", chip->lux_thres_hi);

A threshold detector should map to the IIO events interface that
has standard ways to present threshold detections to userspace.

> +		break;
> +	case APDS990X_LUX_THRESH_BELOW_ATTR:
> +		len = sprintf(buf, "%d\n", chip->lux_thres_lo);
> +		break;
> +	case APDS990X_PROX_SENSOR_RANGE_ATTR:
> +		len = sprintf(buf, "%u\n", APDS_PROX_RANGE);

There is standard ABI for range though it's not heavily used because
the scaling of the raw value is usually much more useful.

> +		break;
> +	case APDS990X_PROX_THRESH_ABOVE_VALUE_ATTR:
> +		len = sprintf(buf, "%d\n", chip->prox_thres);
> +		break;
> +	case APDS990X_PROX_REPORTING_MODE_ATTR:
> +		len = sprintf(buf, "%s\n",
> +			      reporting_modes[!!chip->prox_continuous_mode]);

As mentioned above, mode type attributes are normally a very bad idea
because generic code will never touch them.  And for light sensors most
users are going to be using generic code.

> +		break;
> +	case APDS990X_PROX_REPORTING_MODE_AVAIL_ATTR:
> +		len = sprintf(buf, "%s %s\n",
> +			      reporting_modes[0], reporting_modes[1]);
> +		break;
> +	case APDS990X_CHIP_ID_ATTR:
> +		len = sprintf(buf, "%s %d\n", chip->chipname, chip->revision);

The chip name should be in the main iio_dev->name
It is rarely useful to know chip revision at runtime as generic code doesn't
know what to do with it.  So if worth reporting we normally do that via
a print to the log at probe. If there is a good reason it's needed here then
we can discuss that once you've provided ABI docs.

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
> +	mutex_unlock(&indio_dev->mlock);
>  	return len;
>  }
>  

...

> +static int apds990x_set_lux_thresh(struct apds990x_chip *chip, u32 *target,
> +				   const char *buf)
>  {
>  	unsigned long thresh;
>  	int ret;
> @@ -908,98 +738,165 @@ static ssize_t apds990x_set_lux_thresh(struct apds990x_chip *chip, u32 *target,
>  	if (!chip->lux_wait_fresh_res)
>  		apds990x_refresh_athres(chip);
>  	mutex_unlock(&chip->mutex);
> -	return ret;
>  
> +	return ret;

Make sure to scrub your patches for non related changes. They add noise
when we are trying to understand the real and complex parts of this patch.

>  }
>  

> -static ssize_t apds990x_lux_thresh_below_store(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buf, size_t len)
> -{
> -	struct apds990x_chip *chip =  dev_get_drvdata(dev);
> -	int ret = apds990x_set_lux_thresh(chip, &chip->lux_thres_lo, buf);
> -
> -	if (ret < 0)
> -		return ret;
> -	return len;
> -}
> +	mutex_lock(&indio_dev->mlock);

An IIO driver should never be taking mlock directly.
It should only be accessed via iio_device_claim_direct_mode() etc
but I can't see why that is relevant here.

mlock is an internal subsystem implementation detail that a driver
shouldn't be aware of.

> +	switch ((u32)this_attr->address) {
> +	case APDS990X_LUX_CALIB_ATTR:
> +		chip->lux_calib = value;
> +		break;
> +	case APDS990X_LUX_RATE_ATTR:
> +		mutex_lock(&chip->mutex);
> +		ret = apds990x_set_arate(chip, value);
> +		mutex_unlock(&chip->mutex);
>  
> -static DEVICE_ATTR(lux0_thresh_above_value, S_IRUGO | S_IWUSR,
> -		apds990x_lux_thresh_above_show,
> -		apds990x_lux_thresh_above_store);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case APDS990X_LUX_THRESH_ABOVE_ATTR:
> +		ret = apds990x_set_lux_thresh(chip,
> +					      &chip->lux_thres_hi, buf);
>  
> -static DEVICE_ATTR(lux0_thresh_below_value, S_IRUGO | S_IWUSR,
> -		apds990x_lux_thresh_below_show,
> -		apds990x_lux_thresh_below_store);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case APDS990X_LUX_THRESH_BELOW_ATTR:
> +		ret = apds990x_set_lux_thresh(chip,
> +					      &chip->lux_thres_lo, buf);
>  
> -static ssize_t apds990x_prox_threshold_show(struct device *dev,
> -				   struct device_attribute *attr, char *buf)
> -{
> -	struct apds990x_chip *chip =  dev_get_drvdata(dev);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case APDS990X_PROX_THRESH_ABOVE_VALUE_ATTR:
> +		if (value > APDS_RANGE || value == 0 ||
> +		    value < APDS_PROX_HYSTERESIS)
> +			return -EINVAL;
>  
> -	return sprintf(buf, "%d\n", chip->prox_thres);
> -}
> +		mutex_lock(&chip->mutex);
>  
> -static ssize_t apds990x_prox_threshold_store(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buf, size_t len)
> -{
> -	struct apds990x_chip *chip =  dev_get_drvdata(dev);
> -	unsigned long value;
> -	int ret;
> +		chip->prox_thres = value;
> +		apds990x_force_p_refresh(chip);
>  
> -	ret = kstrtoul(buf, 0, &value);
> -	if (ret)
> -		return ret;
> +		mutex_unlock(&chip->mutex);
> +		break;
> +	case APDS990X_PROX_REPORTING_MODE_ATTR:
> +		ret = sysfs_match_string(reporting_modes, buf);
> +		if (ret < 0)
> +			return ret;
>  
> -	if ((value > APDS_RANGE) || (value == 0) ||
> -		(value < APDS_PROX_HYSTERESIS))
> +		chip->prox_continuous_mode = ret;
> +		break;
> +	default:
>  		return -EINVAL;
> +	}
>  
> -	mutex_lock(&chip->mutex);
> -	chip->prox_thres = value;
> -
> -	apds990x_force_p_refresh(chip);
> -	mutex_unlock(&chip->mutex);
> +	mutex_unlock(&indio_dev->mlock);
>  	return len;
>  }
>  


> -static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
> -		apds990x_power_state_show,
> -		apds990x_power_state_store);
> -
> -static ssize_t apds990x_chip_id_show(struct device *dev,
> -				   struct device_attribute *attr, char *buf)
> +static int apds990x_lux_raw(struct apds990x_chip *chip)
>  {
> -	struct apds990x_chip *chip =  dev_get_drvdata(dev);
> +	struct device *dev = &chip->client->dev;
> +	int ret;
> +	long timeout;
> +
> +	if (pm_runtime_suspended(dev))

If it's suspended you should be waking it up. Not relying on some
state having been configured somewhere else.

I think that it should be one in the call paths for this anyway
so this check is unnecessary.  Hard to tell though with the diff
in this patch.

> +		return -EIO;
> +
> +	timeout = wait_event_interruptible_timeout(chip->wait,
> +						   !chip->lux_wait_fresh_res,
> +						   msecs_to_jiffies(APDS_TIMEOUT));
> +	if (!timeout)
> +		return -EIO;
> +
> +	mutex_lock(&chip->mutex);
> +
> +	ret = (chip->lux * chip->lux_calib) / APDS_CALIB_SCALER;
> +	if (ret > (APDS_RANGE * APDS990X_LUX_OUTPUT_SCALE))
> +		ret = APDS_RANGE * APDS990X_LUX_OUTPUT_SCALE;
>  
> -	return sprintf(buf, "%s %d\n", chip->chipname, chip->revision);
> +	mutex_unlock(&chip->mutex);
> +
> +	return ret;
>  }
>  
...

>  
>  static int apds990x_of_probe(struct i2c_client *client,
>  			     struct apds990x_chip *chip)
> @@ -1080,15 +1011,114 @@ static int apds990x_of_probe(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static int apds990x_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	struct apds990x_chip *chip = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		apds990x_power_state_switch(chip, true);
> +
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			*val = apds990x_lux_raw(chip);
> +			break;
> +		case IIO_PROXIMITY:
> +			*val = apds990x_prox_raw(chip);
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		apds990x_power_state_switch(chip, false);
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +		case IIO_PROXIMITY:
> +		default:
> +			*val = 1;
If scale is one, then the values being provided are in the correct units.
Hence you don't need to provide it.

However note that makes the INFO_PROCESSED not INFO_RAW for the sensor
measuring illumaninance at least. It's trickier for the proximity sensor
as they tend to not have well defined scales. Hence fine to leave that
as _RAW without the scale value being provided.

> +			break;
> +		}
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_ENABLE:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			*val = chip->prox_en;

Mentioned above, but this is almost never a good idea.  Someone reading
proximity should trigger this state transition.

If it is event connected, then add that support to the driver.


> +		default:
> +			break;
> +		}
> +
> +		return IIO_VAL_INT;
> +	}
> +	return -EINVAL;
> +}
> +

>  static int apds990x_probe(struct i2c_client *client)
>  {
>  	struct apds990x_chip *chip;
> +	struct iio_dev *indio_dev;
>  	int err;
>  
> -	chip = kzalloc(sizeof *chip, GFP_KERNEL);
> -	if (!chip)
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> +	if (!indio_dev)
>  		return -ENOMEM;
>  
> +	indio_dev->info = &apds990x_info;
> +	indio_dev->name = "apds990x";

No wild cards in this.  Ideally it should be the name of the actual part
used but if not, choose one supported part.

> +	indio_dev->channels = apds990x_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(apds990x_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	chip = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, chip);
>  	chip->client  = client;
>  
> @@ -1140,17 +1170,17 @@ static int apds990x_probe(struct i2c_client *client)
>  	chip->regs[0].supply = reg_vcc;
>  	chip->regs[1].supply = reg_vled;
>  
> -	err = regulator_bulk_get(&client->dev,
> -				 ARRAY_SIZE(chip->regs), chip->regs);
> +	err = devm_regulator_bulk_get(&client->dev,
> +				      ARRAY_SIZE(chip->regs), chip->regs);

Unrelated to converting the driver to IIO. Please pull out as a precursort
patch.

>  	if (err < 0) {
>  		dev_err(&client->dev, "Cannot get regulators\n");
> -		goto fail1;
> +		return err;
>  	}
>  
>  	err = regulator_bulk_enable(ARRAY_SIZE(chip->regs), chip->regs);
>  	if (err < 0) {
>  		dev_err(&client->dev, "Cannot enable regulators\n");
> -		goto fail2;
> +		return err;
>  	}
>  
>  	usleep_range(APDS_STARTUP_DELAY, 2 * APDS_STARTUP_DELAY);
> @@ -1158,7 +1188,7 @@ static int apds990x_probe(struct i2c_client *client)
>  	err = apds990x_detect(chip);
>  	if (err < 0) {
>  		dev_err(&client->dev, "APDS990X not found\n");
> -		goto fail3;
> +		return err;
>  	}
>  
>  	pm_runtime_set_active(&client->dev);
> @@ -1173,39 +1203,29 @@ static int apds990x_probe(struct i2c_client *client)
>  		err = chip->pdata->setup_resources();
>  		if (err) {
>  			err = -EINVAL;
> -			goto fail3;
> +			goto fail;
>  		}
>  	}
>  
> -	err = sysfs_create_group(&chip->client->dev.kobj,
> -				apds990x_attribute_group);
> -	if (err < 0) {
> -		dev_err(&chip->client->dev, "Sysfs registration failed\n");
> -		goto fail4;
> -	}
> -
> -	err = request_threaded_irq(client->irq, NULL,
> -				apds990x_irq,
> -				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -				"apds990x", chip);
> +	err = devm_request_threaded_irq(&client->dev, client->irq,
> +					NULL, apds990x_irq,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					"apds990x", indio_dev);
>  	if (err) {
>  		dev_err(&client->dev, "could not get IRQ %d\n",
>  			client->irq);
> -		goto fail5;
> +		goto fail;
>  	}
> -	return err;
> -fail5:
> -	sysfs_remove_group(&chip->client->dev.kobj,
> -			&apds990x_attribute_group[0]);
> -fail4:
> +
> +	err = iio_device_register(indio_dev);
Mix and match of devm_ and normally error handling / unregister paths
is a bad idea as it is hard to reason about as the remove() order
is scrambled compared to probe().  We have devm_add_action_or_reset()
to help with that by ensuring everything occurs in the 'obvious'
unwind order which is reverse of the order used to set things up.

I'm sure I missed a lot in reading this patch, so as mentioned for patch 4
please include the full code.  You can ask git not to detect renames
for v2 and that will mean we can see all the code easily and help
with review.  If not, just pasting the code in to a reply is fine
too.

Thanks,

Jonathan
 
> +	if (err)
> +		goto fail;
> +
> +	return 0;
> +fail:
>  	if (chip->pdata && chip->pdata->release_resources)
>  		chip->pdata->release_resources();
> -fail3:
> -	regulator_bulk_disable(ARRAY_SIZE(chip->regs), chip->regs);
> -fail2:
> -	regulator_bulk_free(ARRAY_SIZE(chip->regs), chip->regs);
> -fail1:
> -	kfree(chip);
> +
>  	return err;
>  }
>  
> @@ -1213,10 +1233,6 @@ static void apds990x_remove(struct i2c_client *client)
>  {
>  	struct apds990x_chip *chip = i2c_get_clientdata(client);
>  
> -	free_irq(client->irq, chip);
> -	sysfs_remove_group(&chip->client->dev.kobj,
> -			apds990x_attribute_group);
> -
>  	if (chip->pdata && chip->pdata->release_resources)
>  		chip->pdata->release_resources();
>  
> @@ -1225,10 +1241,6 @@ static void apds990x_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -
> -	regulator_bulk_free(ARRAY_SIZE(chip->regs), chip->regs);
> -
> -	kfree(chip);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP

[PATCH v1 4/4] iio: light: move apds990x into proper place

[Svyatoslav Ryhel]

Since now apds990x supports IIO, it should be moved here from
misc folder.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/iio/light/Kconfig              | 10 ++++++++++
 drivers/iio/light/Makefile             |  1 +
 drivers/{misc => iio/light}/apds990x.c |  0
 drivers/misc/Kconfig                   | 10 ----------
 drivers/misc/Makefile                  |  1 -
 5 files changed, 11 insertions(+), 11 deletions(-)
 rename drivers/{misc => iio/light}/apds990x.c (100%)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 0d4447df7200..49c17eb72c73 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -73,6 +73,16 @@ config APDS9300
 	  To compile this driver as a module, choose M here: the
 	  module will be called apds9300.
 
+config APDS990X
+	tristate "APDS990X combined als and proximity sensors"
+	depends on I2C
+	help
+	   Say Y here if you want to build a driver for Avago APDS990x
+	   combined ambient light and proximity sensor chip.
+
+	   To compile this driver as a module, choose M here: the
+	   module will be called apds990x. If unsure, say N here.
+
 config APDS9960
 	tristate "Avago APDS9960 gesture/RGB/ALS/proximity sensor"
 	select REGMAP_I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 6f23817fae6f..f1ff7934318b 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ADUX1020)		+= adux1020.o
 obj-$(CONFIG_AL3010)		+= al3010.o
 obj-$(CONFIG_AL3320A)		+= al3320a.o
 obj-$(CONFIG_APDS9300)		+= apds9300.o
+obj-$(CONFIG_APDS990X)		+= apds990x.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o
 obj-$(CONFIG_AS73211)		+= as73211.o
 obj-$(CONFIG_BH1750)		+= bh1750.o
diff --git a/drivers/misc/apds990x.c b/drivers/iio/light/apds990x.c
similarity index 100%
rename from drivers/misc/apds990x.c
rename to drivers/iio/light/apds990x.c
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 9947b7892bd5..2856b6c57ca0 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -359,16 +359,6 @@ config SENSORS_BH1770
 	   To compile this driver as a module, choose M here: the
 	   module will be called bh1770glc. If unsure, say N here.
 
-config SENSORS_APDS990X
-	 tristate "APDS990X combined als and proximity sensors"
-	 depends on I2C
-	help
-	   Say Y here if you want to build a driver for Avago APDS990x
-	   combined ambient light and proximity sensor chip.
-
-	   To compile this driver as a module, choose M here: the
-	   module will be called apds990x. If unsure, say N here.
-
 config HMC6352
 	tristate "Honeywell HMC6352 compass"
 	depends on I2C
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 87b54a4a4422..3e3e510cb315 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -18,7 +18,6 @@ obj-$(CONFIG_PHANTOM)		+= phantom.o
 obj-$(CONFIG_QCOM_COINCELL)	+= qcom-coincell.o
 obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
 obj-$(CONFIG_SENSORS_BH1770)	+= bh1770glc.o
-obj-$(CONFIG_SENSORS_APDS990X)	+= apds990x.o
 obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
 obj-$(CONFIG_KGDB_TESTS)	+= kgdbts.o
 obj-$(CONFIG_SGI_XP)		+= sgi-xp/
-- 
2.37.2

Re: [PATCH v1 4/4] iio: light: move apds990x into proper place

[Jonathan Cameron]

On Wed,  8 Mar 2023 11:02:19 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> Since now apds990x supports IIO, it should be moved here from
> misc folder.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

Hi Svyatoslav,

Could you do me a favour and reply to this thread with a copy of the
.c file that is being moved.

We will want to treat this in a similar fashion to as driver graduating
from staging and do a full review of whether it is compliant with IIO ABI
and general code style etc.  That's easier to do if everyone can
see the code in an email for reviewing!

Thanks,

Jonathan


> ---
>  drivers/iio/light/Kconfig              | 10 ++++++++++
>  drivers/iio/light/Makefile             |  1 +
>  drivers/{misc => iio/light}/apds990x.c |  0
>  drivers/misc/Kconfig                   | 10 ----------
>  drivers/misc/Makefile                  |  1 -
>  5 files changed, 11 insertions(+), 11 deletions(-)
>  rename drivers/{misc => iio/light}/apds990x.c (100%)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 0d4447df7200..49c17eb72c73 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -73,6 +73,16 @@ config APDS9300
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called apds9300.
>  
> +config APDS990X
> +	tristate "APDS990X combined als and proximity sensors"
> +	depends on I2C
> +	help
> +	   Say Y here if you want to build a driver for Avago APDS990x
> +	   combined ambient light and proximity sensor chip.
> +
> +	   To compile this driver as a module, choose M here: the
> +	   module will be called apds990x. If unsure, say N here.
> +
>  config APDS9960
>  	tristate "Avago APDS9960 gesture/RGB/ALS/proximity sensor"
>  	select REGMAP_I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 6f23817fae6f..f1ff7934318b 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_ADUX1020)		+= adux1020.o
>  obj-$(CONFIG_AL3010)		+= al3010.o
>  obj-$(CONFIG_AL3320A)		+= al3320a.o
>  obj-$(CONFIG_APDS9300)		+= apds9300.o
> +obj-$(CONFIG_APDS990X)		+= apds990x.o
>  obj-$(CONFIG_APDS9960)		+= apds9960.o
>  obj-$(CONFIG_AS73211)		+= as73211.o
>  obj-$(CONFIG_BH1750)		+= bh1750.o
> diff --git a/drivers/misc/apds990x.c b/drivers/iio/light/apds990x.c
> similarity index 100%
> rename from drivers/misc/apds990x.c
> rename to drivers/iio/light/apds990x.c
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 9947b7892bd5..2856b6c57ca0 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -359,16 +359,6 @@ config SENSORS_BH1770
>  	   To compile this driver as a module, choose M here: the
>  	   module will be called bh1770glc. If unsure, say N here.
>  
> -config SENSORS_APDS990X
> -	 tristate "APDS990X combined als and proximity sensors"
> -	 depends on I2C
> -	help
> -	   Say Y here if you want to build a driver for Avago APDS990x
> -	   combined ambient light and proximity sensor chip.
> -
> -	   To compile this driver as a module, choose M here: the
> -	   module will be called apds990x. If unsure, say N here.
> -
>  config HMC6352
>  	tristate "Honeywell HMC6352 compass"
>  	depends on I2C
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 87b54a4a4422..3e3e510cb315 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -18,7 +18,6 @@ obj-$(CONFIG_PHANTOM)		+= phantom.o
>  obj-$(CONFIG_QCOM_COINCELL)	+= qcom-coincell.o
>  obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
>  obj-$(CONFIG_SENSORS_BH1770)	+= bh1770glc.o
> -obj-$(CONFIG_SENSORS_APDS990X)	+= apds990x.o
>  obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
>  obj-$(CONFIG_KGDB_TESTS)	+= kgdbts.o
>  obj-$(CONFIG_SGI_XP)		+= sgi-xp/

Re: [PATCH v1 0/4] Update APDS990x ALS to support IIO

[Greg Kroah-Hartman]

On Wed, Mar 08, 2023 at 11:02:15AM +0200, Svyatoslav Ryhel wrote:
> Add apds990x binding scheme, convert it to get basic data from
> dts and use common IIO API. Since it works with IIO now, move from
> /misc to /iio.
> 
> Svyatoslav Ryhel (4):
>   dt-bindings: iio: light: add apds990x binding
>   misc: adps990x: convert to OF
>   misc: apds990x: convert to IIO
>   iio: light: move apds990x into proper place
> 
>  .../bindings/iio/light/avago,apds990x.yaml    |  76 ++
>  drivers/iio/light/Kconfig                     |  10 +
>  drivers/iio/light/Makefile                    |   1 +
>  drivers/{misc => iio/light}/apds990x.c        | 802 +++++++++---------
>  drivers/misc/Kconfig                          |  10 -
>  drivers/misc/Makefile                         |   1 -
>  6 files changed, 509 insertions(+), 391 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml
>  rename drivers/{misc => iio/light}/apds990x.c (67%)
> 

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>