[PATCH v1 0/2] Update APDS990x ALS to support device trees

[PATCH v1 0/2] Update APDS990x ALS to support device trees

[Svyatoslav Ryhel]

This patchset is the successor of a wider patch called
'Update APDS990x ALS to support IIO'. I have decided to divide it into
two separate patches to ease review and submitting. All suggestions
from the previous patchset were applied.

This part is dedicated to including device tree support into APDS990x.
APDS990x is a combined ambient light and proximity sensor. ALS and
proximity functionality are highly connected. From mandatory properties
required by device (apart of compatible and address on i2c line) are
two supplies, interrupt, pdrive (LED drive current) and ppcount (the
number of IR calibration pulses).

These patches are oriented ONLY on bringing support of device tree into
driver, pdrive and ppcount are mandatory and datasheet nor driver does
not provide any reference values.

Namings and compatible properties derive from the original driver name.

Svyatoslav Ryhel (2):
  dt-bindings: iio: light: add apds990x binding
  misc: adps990x: convert to OF

 .../bindings/iio/light/avago,apds990x.yaml    | 87 +++++++++++++++++++
 drivers/misc/apds990x.c                       | 55 ++++++++++--
 2 files changed, 135 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml

-- 
2.39.2

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

[Svyatoslav Ryhel]

Add dt-binding for apds990x ambient light/proximity sensor.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../bindings/iio/light/avago,apds990x.yaml    | 87 +++++++++++++++++++
 1 file changed, 87 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..89a061643de5
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml
@@ -0,0 +1,87 @@
+# 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 ambient light 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.
+  Datasheet at https://docs.broadcom.com/doc/AV02-2867EN
+
+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: 3
+    description:
+      The LED drive current is controlled by a regulated current
+      sink on the LDR pin. This feature eliminates the need to use
+      a current limiting resistor to control LED current. The LED
+      drive current can be configured for 12.5 mA (pdrive = 3),
+      25 mA (2), 50 mA (1) or 100 mA (0). For higher LED drive
+      requirements, an external P type transistor can be used to
+      control the LED current.
+
+  avago,ppcount:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 255
+    description:
+      The number of LED pulses can be programmed to a value of 1 to
+      255 pulses as needed. Increasing the number of LED pulses at a
+      given current will increase the sensor sensitivity. Sensitivity
+      grows by the square root of the number of pulses. Each pulse
+      has a 16 mS period.
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - 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 = <0>;
+            avago,ppcount = <3>;
+        };
+    };
+...
-- 
2.39.2

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

[Arnd Bergmann]

On Mon, Jul 31, 2023, at 13:02, Svyatoslav Ryhel wrote:

> +---
> +$id: http://devicetree.org/schemas/iio/light/avago,apds990x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Avago APDS990x ambient light and proximity sensor
> +

The APDS990x looks like a wildcard, which is not appropriate for
the "compatible" property in DT, and should be replaced with a list
of specific part numbers to which this applies.

    Arnd

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

[Svyatoslav Ryhel]

Add ability to use device tree bindings keeping existing setup.

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

diff --git a/drivers/misc/apds990x.c b/drivers/misc/apds990x.c
index 0024503ea6db..cf56d68c938a 100644
--- a/drivers/misc/apds990x.c
+++ b/drivers/misc/apds990x.c
@@ -1051,6 +1051,37 @@ static const struct attribute_group apds990x_attribute_group[] = {
 	{.attrs = sysfs_attrs_ctrl },
 };
 
+static int apds990x_fw_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)
+		return dev_err_probe(&client->dev, ret,
+				     "pdrive property is missing\n");
+	pdata->pdrive = val;
+
+	ret = device_property_read_u32(&client->dev, "avago,ppcount", &val);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "ppcount property is missing\n");
+	pdata->ppcount = val;
+
+	chip->pdata = pdata;
+
+	/* set regulator names which fit device tree entries */
+	chip->regs[0].supply = "vdd";
+	chip->regs[1].supply = "vled";
+
+	return 0;
+}
+
 static int apds990x_probe(struct i2c_client *client)
 {
 	struct apds990x_chip *chip;
@@ -1065,12 +1096,12 @@ 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) {
+		err = apds990x_fw_probe(client, chip);
+		if (err)
+			return err;
 	}
 
 	if (chip->pdata->cf.ga == 0) {
@@ -1111,8 +1142,11 @@ static int apds990x_probe(struct i2c_client *client)
 	chip->prox_persistence = APDS_DEFAULT_PROX_PERS;
 	chip->prox_continuous_mode = false;
 
-	chip->regs[0].supply = reg_vcc;
-	chip->regs[1].supply = reg_vled;
+	if (!chip->regs[0].supply)
+		chip->regs[0].supply = reg_vcc;
+
+	if (!chip->regs[1].supply)
+		chip->regs[1].supply = reg_vled;
 
 	err = regulator_bulk_get(&client->dev,
 				 ARRAY_SIZE(chip->regs), chip->regs);
@@ -1252,6 +1286,12 @@ 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 },
 	{}
@@ -1270,6 +1310,7 @@ 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,
-- 
2.39.2

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

[Arnd Bergmann]

On Mon, Jul 31, 2023, at 13:02, Svyatoslav Ryhel wrote:
> Add ability to use device tree bindings keeping existing setup.

I see that there are no more in-tree users of the old
apds990x_platform_data, so I think it would be best to completely
remove that codepath and merge that structure into struct
apds990x_chip, to simplify the probing and avoid the extra
allocation.

   Arnd

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

[Svyatoslav Ryhel]

31 липня 2023 р. 16:18:16 GMT+03:00, Arnd Bergmann <arnd@arndb.de> написав(-ла):
>On Mon, Jul 31, 2023, at 13:02, Svyatoslav Ryhel wrote:
>> Add ability to use device tree bindings keeping existing setup.
>
>I see that there are no more in-tree users of the old
>apds990x_platform_data, so I think it would be best to completely
>remove that codepath and merge that structure into struct
>apds990x_chip, to simplify the probing and avoid the extra
>allocation.

Thank you very much for your review, but is it mandatory to drop pdata in this particular patch set? To be honest this driver needs serious upgrades and refactoring, and I have no dedication to invest my time into refactoring it, moreover, I am not a maintainer of this driver, nor a full time kernel maintainer of any kind. I am doing what I am doing only because one of my devices uses this als but it is not something crucial.

Best regards,
Svyatoslav R.

>   Arnd

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

[Arnd Bergmann]

On Mon, Jul 31, 2023, at 16:58, Svyatoslav Ryhel wrote:
> 31 липня 2023 р. 16:18:16 GMT+03:00, Arnd Bergmann <arnd@arndb.de> написав(-ла):
>>On Mon, Jul 31, 2023, at 13:02, Svyatoslav Ryhel wrote:
>>> Add ability to use device tree bindings keeping existing setup.
>>
>>I see that there are no more in-tree users of the old
>>apds990x_platform_data, so I think it would be best to completely
>>remove that codepath and merge that structure into struct
>>apds990x_chip, to simplify the probing and avoid the extra
>>allocation.
>
> Thank you very much for your review, but is it mandatory to drop pdata 
> in this particular patch set? To be honest this driver needs serious 
> upgrades and refactoring, and I have no dedication to invest my time 
> into refactoring it, moreover, I am not a maintainer of this driver, 
> nor a full time kernel maintainer of any kind. I am doing what I am 
> doing only because one of my devices uses this als but it is not 
> something crucial.

We have a lot of drivers that are lacking the cleanup I'm asking
for, so I don't think I'd mandate it at this point, but I don't
actually expect the patch to be any more complicated in the end,
so just try it out.

I think at the minimum, please remove the include/platform_data
header and move the contents into the driver itself, I'd be fine
with that. If you can easily do further cleanup by dropping
the separate allocation and folding the apds990x_fw_probe()
function back into apds990x_probe(), please do that, just stop
at the point where you feel it gets too complicated.

    Arnd

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

[Jonathan Cameron]

On Mon, 31 Jul 2023 17:38:59 +0200
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Mon, Jul 31, 2023, at 16:58, Svyatoslav Ryhel wrote:
> > 31 липня 2023 р. 16:18:16 GMT+03:00, Arnd Bergmann <arnd@arndb.de> написав(-ла):  
> >>On Mon, Jul 31, 2023, at 13:02, Svyatoslav Ryhel wrote:  
> >>> Add ability to use device tree bindings keeping existing setup.  
> >>
> >>I see that there are no more in-tree users of the old
> >>apds990x_platform_data, so I think it would be best to completely
> >>remove that codepath and merge that structure into struct
> >>apds990x_chip, to simplify the probing and avoid the extra
> >>allocation.  
> >
> > Thank you very much for your review, but is it mandatory to drop pdata 
> > in this particular patch set? To be honest this driver needs serious 
> > upgrades and refactoring, and I have no dedication to invest my time 
> > into refactoring it, moreover, I am not a maintainer of this driver, 
> > nor a full time kernel maintainer of any kind. I am doing what I am 
> > doing only because one of my devices uses this als but it is not 
> > something crucial.  
> 
> We have a lot of drivers that are lacking the cleanup I'm asking
> for, so I don't think I'd mandate it at this point, but I don't
> actually expect the patch to be any more complicated in the end,
> so just try it out.
> 
> I think at the minimum, please remove the include/platform_data
> header and move the contents into the driver itself, I'd be fine
> with that. If you can easily do further cleanup by dropping
> the separate allocation and folding the apds990x_fw_probe()
> function back into apds990x_probe(), please do that, just stop
> at the point where you feel it gets too complicated.
> 

It's a long shot, but this looks pretty close in register map to
the apds9960 in IIO.

Maybe try adding the ID to that driver and cross your fingers?

There is some stuff going on around the register address / commands
that I haven't figured out but it looks similar for the byte access
path and that may be all the IIO driver is using.

If you are fine testing, it's possible someone else might do the
leg work (if me I'll emulate just enough to convince myself I didn't
break it too badly). Won't be high on my list, but maybe I'll get
a boring wet weekend sometime...

Jonathan

>     Arnd

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

[Svyatoslav Ryhel]

1 серпня 2023 р. 21:10:26 GMT+03:00, Jonathan Cameron <jic23@kernel.org> написав(-ла):
>On Mon, 31 Jul 2023 17:38:59 +0200
>"Arnd Bergmann" <arnd@arndb.de> wrote:
>
>> On Mon, Jul 31, 2023, at 16:58, Svyatoslav Ryhel wrote:
>> > 31 липня 2023 р. 16:18:16 GMT+03:00, Arnd Bergmann <arnd@arndb.de> написав(-ла):  
>> >>On Mon, Jul 31, 2023, at 13:02, Svyatoslav Ryhel wrote:  
>> >>> Add ability to use device tree bindings keeping existing setup.  
>> >>
>> >>I see that there are no more in-tree users of the old
>> >>apds990x_platform_data, so I think it would be best to completely
>> >>remove that codepath and merge that structure into struct
>> >>apds990x_chip, to simplify the probing and avoid the extra
>> >>allocation.  
>> >
>> > Thank you very much for your review, but is it mandatory to drop pdata 
>> > in this particular patch set? To be honest this driver needs serious 
>> > upgrades and refactoring, and I have no dedication to invest my time 
>> > into refactoring it, moreover, I am not a maintainer of this driver, 
>> > nor a full time kernel maintainer of any kind. I am doing what I am 
>> > doing only because one of my devices uses this als but it is not 
>> > something crucial.  
>> 
>> We have a lot of drivers that are lacking the cleanup I'm asking
>> for, so I don't think I'd mandate it at this point, but I don't
>> actually expect the patch to be any more complicated in the end,
>> so just try it out.
>> 
>> I think at the minimum, please remove the include/platform_data
>> header and move the contents into the driver itself, I'd be fine
>> with that. If you can easily do further cleanup by dropping
>> the separate allocation and folding the apds990x_fw_probe()
>> function back into apds990x_probe(), please do that, just stop
>> at the point where you feel it gets too complicated.
>> 
>
>It's a long shot, but this looks pretty close in register map to
>the apds9960 in IIO.
>
>Maybe try adding the ID to that driver and cross your fingers?

If you pay me for a broken phone or repair if smth goes wrong, sure, why not.

>There is some stuff going on around the register address / commands
>that I haven't figured out but it looks similar for the byte access
>path and that may be all the IIO driver is using.
>
>If you are fine testing, it's possible someone else might do the
>leg work (if me I'll emulate just enough to convince myself I didn't
>break it too badly). Won't be high on my list, but maybe I'll get
>a boring wet weekend sometime...
>
>Jonathan
>
>>     Arnd
>