[PATCH v2 0/3] iio: light: add al3000a als support

[PATCH v2 0/3] iio: light: add al3000a als support

[Svyatoslav Ryhel]

AL3000a is an illuminance sensor found in ASUS TF101 tablet.

---
Changes on switching from v1 to v2:
- sort compatible alphabetically in schema
- clarify commit descriptions
- convert to use regmap
- arrangle lux conversion table in rows of 8
- add more used headers
- improve code formatting 
---

Svyatoslav Ryhel (3):
  dt-bindings: iio: light: al3010: add al3000a support
  iio: light: Add support for AL3000a illuminance sensor
  ARM: tegra: tf101: Add al3000a illuminance sensor node

 .../bindings/iio/light/dynaimage,al3010.yaml  |   6 +-
 .../boot/dts/nvidia/tegra20-asus-tf101.dts    |  11 +
 drivers/iio/light/Kconfig                     |  10 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/al3000a.c                   | 221 ++++++++++++++++++
 5 files changed, 247 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iio/light/al3000a.c

-- 
2.43.0

[PATCH v2 1/3] dt-bindings: iio: light: al3010: add al3000a support

[Svyatoslav Ryhel]

AL3000a is an ambient light sensor quite closely related to
exising AL3010 and can reuse exising schema for AL3010.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../devicetree/bindings/iio/light/dynaimage,al3010.yaml     | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml b/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
index a3a979553e32..f1048c30e73e 100644
--- a/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
+++ b/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
@@ -4,14 +4,16 @@
 $id: http://devicetree.org/schemas/iio/light/dynaimage,al3010.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Dyna-Image AL3010 sensor
+title: Dyna-Image AL3000a/AL3010 sensor
 
 maintainers:
   - David Heidelberg <david@ixit.cz>
 
 properties:
   compatible:
-    const: dynaimage,al3010
+    enum:
+      - dynaimage,al3000a
+      - dynaimage,al3010
 
   reg:
     maxItems: 1
-- 
2.43.0

Re: [PATCH v2 1/3] dt-bindings: iio: light: al3010: add al3000a support

[Jonathan Cameron]

On Sat, 15 Feb 2025 12:31:57 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> AL3000a is an ambient light sensor quite closely related to
> exising AL3010 and can reuse exising schema for AL3010.
Hi,

For a binding like this, also explain how they are different enough that
we can't use a fallback compatible.

Thanks,

Jonathan

> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../devicetree/bindings/iio/light/dynaimage,al3010.yaml     | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml b/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
> index a3a979553e32..f1048c30e73e 100644
> --- a/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
> @@ -4,14 +4,16 @@
>  $id: http://devicetree.org/schemas/iio/light/dynaimage,al3010.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Dyna-Image AL3010 sensor
> +title: Dyna-Image AL3000a/AL3010 sensor
>  
>  maintainers:
>    - David Heidelberg <david@ixit.cz>
>  
>  properties:
>    compatible:
> -    const: dynaimage,al3010
> +    enum:
> +      - dynaimage,al3000a
> +      - dynaimage,al3010
>  
>    reg:
>      maxItems: 1

Re: [PATCH v2 1/3] dt-bindings: iio: light: al3010: add al3000a support

[Svyatoslav Ryhel]

нд, 16 лют. 2025 р. о 16:42 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Sat, 15 Feb 2025 12:31:57 +0200
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > AL3000a is an ambient light sensor quite closely related to
> > exising AL3010 and can reuse exising schema for AL3010.
> Hi,
>
> For a binding like this, also explain how they are different enough that
> we can't use a fallback compatible.
>
> Thanks,
>
> Jonathan
>

Fallback will cause use of inappropriate driver. As I have already
told, hardware configuration matches but data processing from sensor
does not.

> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  .../devicetree/bindings/iio/light/dynaimage,al3010.yaml     | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml b/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
> > index a3a979553e32..f1048c30e73e 100644
> > --- a/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
> > +++ b/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
> > @@ -4,14 +4,16 @@
> >  $id: http://devicetree.org/schemas/iio/light/dynaimage,al3010.yaml#
> >  $schema: http://devicetree.org/meta-schemas/core.yaml#
> >
> > -title: Dyna-Image AL3010 sensor
> > +title: Dyna-Image AL3000a/AL3010 sensor
> >
> >  maintainers:
> >    - David Heidelberg <david@ixit.cz>
> >
> >  properties:
> >    compatible:
> > -    const: dynaimage,al3010
> > +    enum:
> > +      - dynaimage,al3000a
> > +      - dynaimage,al3010
> >
> >    reg:
> >      maxItems: 1
>

Re: [PATCH v2 1/3] dt-bindings: iio: light: al3010: add al3000a support

[Jonathan Cameron]

On Sun, 16 Feb 2025 16:44:58 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> нд, 16 лют. 2025 р. о 16:42 Jonathan Cameron <jic23@kernel.org> пише:
> >
> > On Sat, 15 Feb 2025 12:31:57 +0200
> > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >  
> > > AL3000a is an ambient light sensor quite closely related to
> > > exising AL3010 and can reuse exising schema for AL3010.  
> > Hi,
> >
> > For a binding like this, also explain how they are different enough that
> > we can't use a fallback compatible.
> >
> > Thanks,
> >
> > Jonathan
> >  
> 
> Fallback will cause use of inappropriate driver. As I have already
> told, hardware configuration matches but data processing from sensor
> does not.
Absolutely understood.  The point is this patch description should
state exactly that. Say something like the register maps are incompatible.

Jonathan

> 
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  .../devicetree/bindings/iio/light/dynaimage,al3010.yaml     | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml b/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
> > > index a3a979553e32..f1048c30e73e 100644
> > > --- a/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/light/dynaimage,al3010.yaml
> > > @@ -4,14 +4,16 @@
> > >  $id: http://devicetree.org/schemas/iio/light/dynaimage,al3010.yaml#
> > >  $schema: http://devicetree.org/meta-schemas/core.yaml#
> > >
> > > -title: Dyna-Image AL3010 sensor
> > > +title: Dyna-Image AL3000a/AL3010 sensor
> > >
> > >  maintainers:
> > >    - David Heidelberg <david@ixit.cz>
> > >
> > >  properties:
> > >    compatible:
> > > -    const: dynaimage,al3010
> > > +    enum:
> > > +      - dynaimage,al3000a
> > > +      - dynaimage,al3010
> > >
> > >    reg:
> > >      maxItems: 1  
> >  
> 

[PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Svyatoslav Ryhel]

AL3000a is a simple I2C-based ambient light sensor, which is
closely related to AL3010 and AL3320a, but has significantly
different way of processing data generated by the sensor.

Tested-by: Robert Eckelmann <longnoserob@gmail.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/iio/light/Kconfig   |  10 ++
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/al3000a.c | 221 ++++++++++++++++++++++++++++++++++++
 3 files changed, 232 insertions(+)
 create mode 100644 drivers/iio/light/al3000a.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index e34e551eef3e..142f7f7ef0ec 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -43,6 +43,16 @@ config ADUX1020
 	 To compile this driver as a module, choose M here: the
 	 module will be called adux1020.
 
+config AL3000A
+	tristate "AL3000a ambient light sensor"
+	depends on I2C
+	help
+	  Say Y here if you want to build a driver for the Dyna Image AL3000a
+	  ambient light sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called al3000a.
+
 config AL3010
 	tristate "AL3010 ambient light sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 11a4041b918a..17030a4cc340 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -7,6 +7,7 @@
 obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
 obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
 obj-$(CONFIG_ADUX1020)		+= adux1020.o
+obj-$(CONFIG_AL3000A)		+= al3000a.o
 obj-$(CONFIG_AL3010)		+= al3010.o
 obj-$(CONFIG_AL3320A)		+= al3320a.o
 obj-$(CONFIG_APDS9300)		+= apds9300.o
diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
new file mode 100644
index 000000000000..58d4336dd081
--- /dev/null
+++ b/drivers/iio/light/al3000a.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define AL3000A_DRV_NAME		"al3000a"
+#define AL3000A_REG_SYSTEM		0x00
+#define AL3000A_REG_DATA		0x05
+
+#define AL3000A_CONFIG_ENABLE		0x00
+#define AL3000A_CONFIG_DISABLE		0x0b
+#define AL3000A_CONFIG_RESET		0x0f
+#define AL3000A_GAIN_MASK		GENMASK(5, 0)
+
+/*
+ * This are pre-calculated lux values based on possible output of sensor
+ * (range 0x00 - 0x3F)
+ */
+static const u32 lux_table[] = {
+	1, 1, 1, 2, 2, 2, 3, 4,					/* 0 - 7 */
+	4, 5, 6, 7, 9, 11, 13, 16,				/* 8 - 15 */
+	19, 22, 27, 32, 39, 46, 56, 67,				/* 16 - 23 */
+	80, 96, 116, 139, 167, 200, 240, 289,			/* 24 - 31 */
+	347, 416, 499, 600, 720, 864, 1037, 1245,		/* 32 - 39 */
+	1495, 1795, 2155, 2587, 3105, 3728, 4475, 5373,		/* 40 - 47 */
+	6450, 7743, 9296, 11160, 13397, 16084, 19309, 23180,	/* 48 - 55 */
+	27828, 33408, 40107, 48148, 57803, 69393, 83306, 100000 /* 56 - 63 */
+};
+
+static const struct regmap_config al3000a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AL3000A_REG_DATA,
+};
+
+struct al3000a_data {
+	struct regmap *regmap;
+	struct regulator *vdd_supply;
+};
+
+static const struct iio_chan_spec al3000a_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+static int al3000a_set_pwr(struct al3000a_data *data, bool pwr)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	u8 val = pwr ? AL3000A_CONFIG_ENABLE : AL3000A_CONFIG_DISABLE;
+	int ret;
+
+	if (pwr) {
+		ret = regulator_enable(data->vdd_supply);
+		if (ret < 0) {
+			dev_err(dev, "failed to enable vdd power supply\n");
+			return ret;
+		}
+	}
+
+	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, val);
+	if (ret < 0) {
+		dev_err(dev, "failed to write system register\n");
+		return ret;
+	}
+
+	if (!pwr) {
+		ret = regulator_disable(data->vdd_supply);
+		if (ret < 0) {
+			dev_err(dev, "failed to disable vdd power supply\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void al3000a_set_pwr_off(void *_data)
+{
+	struct al3000a_data *data = _data;
+
+	al3000a_set_pwr(data, false);
+}
+
+static int al3000a_init(struct al3000a_data *data)
+{
+	int ret;
+
+	ret = al3000a_set_pwr(data, true);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_RESET);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_ENABLE);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int al3000a_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	struct al3000a_data *data = iio_priv(indio_dev);
+	int ret, gain;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
+		if (ret < 0)
+			return ret;
+
+		*val = lux_table[gain & AL3000A_GAIN_MASK];
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 1;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info al3000a_info = {
+	.read_raw	= al3000a_read_raw,
+};
+
+static int al3000a_probe(struct i2c_client *client)
+{
+	struct al3000a_data *data;
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+
+	data->regmap = devm_regmap_init_i2c(client, &al3000a_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap),
+				     "cannot allocate regmap\n");
+
+	data->vdd_supply = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(data->vdd_supply))
+		return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
+				     "failed to get vdd regulator\n");
+
+	indio_dev->info = &al3000a_info;
+	indio_dev->name = AL3000A_DRV_NAME;
+	indio_dev->channels = al3000a_channels;
+	indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = al3000a_init(data);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to init ALS\n");
+
+	ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to add action\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int al3000a_suspend(struct device *dev)
+{
+	struct al3000a_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return al3000a_set_pwr(data, false);
+}
+
+static int al3000a_resume(struct device *dev)
+{
+	struct al3000a_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return al3000a_set_pwr(data, true);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(al3000a_pm_ops, al3000a_suspend, al3000a_resume);
+
+static const struct of_device_id al3000a_of_match[] = {
+	{ .compatible = "dynaimage,al3000a" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, al3000a_of_match);
+
+static struct i2c_driver al3000a_driver = {
+	.driver = {
+		.name = AL3000A_DRV_NAME,
+		.of_match_table = al3000a_of_match,
+		.pm = pm_sleep_ptr(&al3000a_pm_ops),
+	},
+	.probe = al3000a_probe,
+};
+module_i2c_driver(al3000a_driver);
+
+MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");
+MODULE_DESCRIPTION("al3000a Ambient Light Sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[David Heidelberg]

On 15/02/2025 11:31, Svyatoslav Ryhel wrote:
> AL3000a is a simple I2C-based ambient light sensor, which is
> closely related to AL3010 and AL3320a, but has significantly
> different way of processing data generated by the sensor.
> 
> Tested-by: Robert Eckelmann <longnoserob@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>   drivers/iio/light/Kconfig   |  10 ++
>   drivers/iio/light/Makefile  |   1 +
>   drivers/iio/light/al3000a.c | 221 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 232 insertions(+)
>   create mode 100644 drivers/iio/light/al3000a.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index e34e551eef3e..142f7f7ef0ec 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -43,6 +43,16 @@ config ADUX1020
>   	 To compile this driver as a module, choose M here: the
>   	 module will be called adux1020.
>   
> +config AL3000A
> +	tristate "AL3000a ambient light sensor"
> +	depends on I2C
> +	help
> +	  Say Y here if you want to build a driver for the Dyna Image AL3000a
> +	  ambient light sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called al3000a.
> +
>   config AL3010
>   	tristate "AL3010 ambient light sensor"
>   	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 11a4041b918a..17030a4cc340 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -7,6 +7,7 @@
>   obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
>   obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
>   obj-$(CONFIG_ADUX1020)		+= adux1020.o
> +obj-$(CONFIG_AL3000A)		+= al3000a.o
>   obj-$(CONFIG_AL3010)		+= al3010.o
>   obj-$(CONFIG_AL3320A)		+= al3320a.o
>   obj-$(CONFIG_APDS9300)		+= apds9300.o
> diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
> new file mode 100644
> index 000000000000..58d4336dd081
> --- /dev/null
> +++ b/drivers/iio/light/al3000a.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define AL3000A_DRV_NAME		"al3000a"
> +#define AL3000A_REG_SYSTEM		0x00
> +#define AL3000A_REG_DATA		0x05
> +
> +#define AL3000A_CONFIG_ENABLE		0x00
> +#define AL3000A_CONFIG_DISABLE		0x0b
> +#define AL3000A_CONFIG_RESET		0x0f
> +#define AL3000A_GAIN_MASK		GENMASK(5, 0)
> +
> +/*
> + * This are pre-calculated lux values based on possible output of sensor
> + * (range 0x00 - 0x3F)
> + */
> +static const u32 lux_table[] = {
> +	1, 1, 1, 2, 2, 2, 3, 4,					/* 0 - 7 */
> +	4, 5, 6, 7, 9, 11, 13, 16,				/* 8 - 15 */
> +	19, 22, 27, 32, 39, 46, 56, 67,				/* 16 - 23 */
> +	80, 96, 116, 139, 167, 200, 240, 289,			/* 24 - 31 */
> +	347, 416, 499, 600, 720, 864, 1037, 1245,		/* 32 - 39 */
> +	1495, 1795, 2155, 2587, 3105, 3728, 4475, 5373,		/* 40 - 47 */
> +	6450, 7743, 9296, 11160, 13397, 16084, 19309, 23180,	/* 48 - 55 */
> +	27828, 33408, 40107, 48148, 57803, 69393, 83306, 100000 /* 56 - 63 */
> +};
> +
> +static const struct regmap_config al3000a_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AL3000A_REG_DATA,
> +};
> +
> +struct al3000a_data {
> +	struct regmap *regmap;
> +	struct regulator *vdd_supply;
> +};
> +
> +static const struct iio_chan_spec al3000a_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};
> +
> +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	u8 val = pwr ? AL3000A_CONFIG_ENABLE : AL3000A_CONFIG_DISABLE;
> +	int ret;
> +
> +	if (pwr) {
> +		ret = regulator_enable(data->vdd_supply);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable vdd power supply\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to write system register\n");
> +		return ret;
> +	}
> +
> +	if (!pwr) {
> +		ret = regulator_disable(data->vdd_supply);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to disable vdd power supply\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void al3000a_set_pwr_off(void *_data)
> +{
> +	struct al3000a_data *data = _data;
> +
> +	al3000a_set_pwr(data, false);
> +}
> +
> +static int al3000a_init(struct al3000a_data *data)
> +{
> +	int ret;
> +
> +	ret = al3000a_set_pwr(data, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_RESET);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int al3000a_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct al3000a_data *data = iio_priv(indio_dev);
> +	int ret, gain;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = lux_table[gain & AL3000A_GAIN_MASK];

Why did you chosen to do post-processing in the RAW channel instead 
doing it in INFO_SCALE (same as al3010 does)?

Except this, LGTM.

Documentation and DT patch:

Reviewed-by: David Heidelberg <david@ixit.cz>
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 1;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info al3000a_info = {
> +	.read_raw	= al3000a_read_raw,
> +};
> +
> +static int al3000a_probe(struct i2c_client *client)
> +{
> +	struct al3000a_data *data;
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &al3000a_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "cannot allocate regmap\n");
> +
> +	data->vdd_supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(data->vdd_supply))
> +		return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
> +				     "failed to get vdd regulator\n");
> +
> +	indio_dev->info = &al3000a_info;
> +	indio_dev->name = AL3000A_DRV_NAME;
> +	indio_dev->channels = al3000a_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = al3000a_init(data);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to init ALS\n");
> +
> +	ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to add action\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static int al3000a_suspend(struct device *dev)
> +{
> +	struct al3000a_data *data = iio_priv(dev_get_drvdata(dev));
> +
> +	return al3000a_set_pwr(data, false);
> +}
> +
> +static int al3000a_resume(struct device *dev)
> +{
> +	struct al3000a_data *data = iio_priv(dev_get_drvdata(dev));
> +
> +	return al3000a_set_pwr(data, true);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(al3000a_pm_ops, al3000a_suspend, al3000a_resume);
> +
> +static const struct of_device_id al3000a_of_match[] = {
> +	{ .compatible = "dynaimage,al3000a" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, al3000a_of_match);
> +
> +static struct i2c_driver al3000a_driver = {
> +	.driver = {
> +		.name = AL3000A_DRV_NAME,
> +		.of_match_table = al3000a_of_match,
> +		.pm = pm_sleep_ptr(&al3000a_pm_ops),
> +	},
> +	.probe = al3000a_probe,
> +};
> +module_i2c_driver(al3000a_driver);
> +
> +MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("al3000a Ambient Light Sensor driver");
> +MODULE_LICENSE("GPL");

-- 
David Heidelberg

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Svyatoslav Ryhel]

сб, 15 лют. 2025 р. о 16:12 David Heidelberg <david@ixit.cz> пише:
>
>
>
> On 15/02/2025 11:31, Svyatoslav Ryhel wrote:
> > AL3000a is a simple I2C-based ambient light sensor, which is
> > closely related to AL3010 and AL3320a, but has significantly
> > different way of processing data generated by the sensor.
> >
> > Tested-by: Robert Eckelmann <longnoserob@gmail.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >   drivers/iio/light/Kconfig   |  10 ++
> >   drivers/iio/light/Makefile  |   1 +
> >   drivers/iio/light/al3000a.c | 221 ++++++++++++++++++++++++++++++++++++
> >   3 files changed, 232 insertions(+)
> >   create mode 100644 drivers/iio/light/al3000a.c
> >
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index e34e551eef3e..142f7f7ef0ec 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -43,6 +43,16 @@ config ADUX1020
> >        To compile this driver as a module, choose M here: the
> >        module will be called adux1020.
> >
> > +config AL3000A
> > +     tristate "AL3000a ambient light sensor"
> > +     depends on I2C
> > +     help
> > +       Say Y here if you want to build a driver for the Dyna Image AL3000a
> > +       ambient light sensor.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called al3000a.
> > +
> >   config AL3010
> >       tristate "AL3010 ambient light sensor"
> >       depends on I2C
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index 11a4041b918a..17030a4cc340 100644
> > --- a/drivers/iio/light/Makefile
> > +++ b/drivers/iio/light/Makefile
> > @@ -7,6 +7,7 @@
> >   obj-$(CONFIG_ACPI_ALS)              += acpi-als.o
> >   obj-$(CONFIG_ADJD_S311)             += adjd_s311.o
> >   obj-$(CONFIG_ADUX1020)              += adux1020.o
> > +obj-$(CONFIG_AL3000A)                += al3000a.o
> >   obj-$(CONFIG_AL3010)                += al3010.o
> >   obj-$(CONFIG_AL3320A)               += al3320a.o
> >   obj-$(CONFIG_APDS9300)              += apds9300.o
> > diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
> > new file mode 100644
> > index 000000000000..58d4336dd081
> > --- /dev/null
> > +++ b/drivers/iio/light/al3000a.c
> > @@ -0,0 +1,221 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/array_size.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/pm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/types.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#define AL3000A_DRV_NAME             "al3000a"
> > +#define AL3000A_REG_SYSTEM           0x00
> > +#define AL3000A_REG_DATA             0x05
> > +
> > +#define AL3000A_CONFIG_ENABLE                0x00
> > +#define AL3000A_CONFIG_DISABLE               0x0b
> > +#define AL3000A_CONFIG_RESET         0x0f
> > +#define AL3000A_GAIN_MASK            GENMASK(5, 0)
> > +
> > +/*
> > + * This are pre-calculated lux values based on possible output of sensor
> > + * (range 0x00 - 0x3F)
> > + */
> > +static const u32 lux_table[] = {
> > +     1, 1, 1, 2, 2, 2, 3, 4,                                 /* 0 - 7 */
> > +     4, 5, 6, 7, 9, 11, 13, 16,                              /* 8 - 15 */
> > +     19, 22, 27, 32, 39, 46, 56, 67,                         /* 16 - 23 */
> > +     80, 96, 116, 139, 167, 200, 240, 289,                   /* 24 - 31 */
> > +     347, 416, 499, 600, 720, 864, 1037, 1245,               /* 32 - 39 */
> > +     1495, 1795, 2155, 2587, 3105, 3728, 4475, 5373,         /* 40 - 47 */
> > +     6450, 7743, 9296, 11160, 13397, 16084, 19309, 23180,    /* 48 - 55 */
> > +     27828, 33408, 40107, 48148, 57803, 69393, 83306, 100000 /* 56 - 63 */
> > +};
> > +
> > +static const struct regmap_config al3000a_regmap_config = {
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .max_register = AL3000A_REG_DATA,
> > +};
> > +
> > +struct al3000a_data {
> > +     struct regmap *regmap;
> > +     struct regulator *vdd_supply;
> > +};
> > +
> > +static const struct iio_chan_spec al3000a_channels[] = {
> > +     {
> > +             .type = IIO_LIGHT,
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +                                   BIT(IIO_CHAN_INFO_SCALE),
> > +     },
> > +};
> > +
> > +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr)
> > +{
> > +     struct device *dev = regmap_get_device(data->regmap);
> > +     u8 val = pwr ? AL3000A_CONFIG_ENABLE : AL3000A_CONFIG_DISABLE;
> > +     int ret;
> > +
> > +     if (pwr) {
> > +             ret = regulator_enable(data->vdd_supply);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to enable vdd power supply\n");
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, val);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to write system register\n");
> > +             return ret;
> > +     }
> > +
> > +     if (!pwr) {
> > +             ret = regulator_disable(data->vdd_supply);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to disable vdd power supply\n");
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void al3000a_set_pwr_off(void *_data)
> > +{
> > +     struct al3000a_data *data = _data;
> > +
> > +     al3000a_set_pwr(data, false);
> > +}
> > +
> > +static int al3000a_init(struct al3000a_data *data)
> > +{
> > +     int ret;
> > +
> > +     ret = al3000a_set_pwr(data, true);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_RESET);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_ENABLE);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static int al3000a_read_raw(struct iio_dev *indio_dev,
> > +                         struct iio_chan_spec const *chan, int *val,
> > +                         int *val2, long mask)
> > +{
> > +     struct al3000a_data *data = iio_priv(indio_dev);
> > +     int ret, gain;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             *val = lux_table[gain & AL3000A_GAIN_MASK];
>
> Why did you chosen to do post-processing in the RAW channel instead
> doing it in INFO_SCALE (same as al3010 does)?
>

From my observation INFO_SCALE will just perform multiplication of RAW
to SCALE. In this case values which are read are not actual raw values
of illumination. Next is my assumption (since there is no datasheet),
but values obtained from register are similar to values from adc
thermal sensors, they need be converted via reference table to get
actual data.

> Except this, LGTM.
>
> Documentation and DT patch:
>
> Reviewed-by: David Heidelberg <david@ixit.cz>
> > +
> > +             return IIO_VAL_INT;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             *val = 1;
> > +
> > +             return IIO_VAL_INT;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +static const struct iio_info al3000a_info = {
> > +     .read_raw       = al3000a_read_raw,
> > +};
> > +
> > +static int al3000a_probe(struct i2c_client *client)
> > +{
> > +     struct al3000a_data *data;
> > +     struct device *dev = &client->dev;
> > +     struct iio_dev *indio_dev;
> > +     int ret;
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     data = iio_priv(indio_dev);
> > +     i2c_set_clientdata(client, indio_dev);
> > +
> > +     data->regmap = devm_regmap_init_i2c(client, &al3000a_regmap_config);
> > +     if (IS_ERR(data->regmap))
> > +             return dev_err_probe(dev, PTR_ERR(data->regmap),
> > +                                  "cannot allocate regmap\n");
> > +
> > +     data->vdd_supply = devm_regulator_get(dev, "vdd");
> > +     if (IS_ERR(data->vdd_supply))
> > +             return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
> > +                                  "failed to get vdd regulator\n");
> > +
> > +     indio_dev->info = &al3000a_info;
> > +     indio_dev->name = AL3000A_DRV_NAME;
> > +     indio_dev->channels = al3000a_channels;
> > +     indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +     ret = al3000a_init(data);
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret, "failed to init ALS\n");
> > +
> > +     ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret, "failed to add action\n");
> > +
> > +     return devm_iio_device_register(dev, indio_dev);
> > +}
> > +
> > +static int al3000a_suspend(struct device *dev)
> > +{
> > +     struct al3000a_data *data = iio_priv(dev_get_drvdata(dev));
> > +
> > +     return al3000a_set_pwr(data, false);
> > +}
> > +
> > +static int al3000a_resume(struct device *dev)
> > +{
> > +     struct al3000a_data *data = iio_priv(dev_get_drvdata(dev));
> > +
> > +     return al3000a_set_pwr(data, true);
> > +}
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(al3000a_pm_ops, al3000a_suspend, al3000a_resume);
> > +
> > +static const struct of_device_id al3000a_of_match[] = {
> > +     { .compatible = "dynaimage,al3000a" },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, al3000a_of_match);
> > +
> > +static struct i2c_driver al3000a_driver = {
> > +     .driver = {
> > +             .name = AL3000A_DRV_NAME,
> > +             .of_match_table = al3000a_of_match,
> > +             .pm = pm_sleep_ptr(&al3000a_pm_ops),
> > +     },
> > +     .probe = al3000a_probe,
> > +};
> > +module_i2c_driver(al3000a_driver);
> > +
> > +MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("al3000a Ambient Light Sensor driver");
> > +MODULE_LICENSE("GPL");
>
> --
> David Heidelberg
>

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Jonathan Cameron]

> > > +
> > > +static int al3000a_read_raw(struct iio_dev *indio_dev,
> > > +                         struct iio_chan_spec const *chan, int *val,
> > > +                         int *val2, long mask)
> > > +{
> > > +     struct al3000a_data *data = iio_priv(indio_dev);
> > > +     int ret, gain;
> > > +
> > > +     switch (mask) {
> > > +     case IIO_CHAN_INFO_RAW:
> > > +             ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             *val = lux_table[gain & AL3000A_GAIN_MASK];  
> >
> > Why did you chosen to do post-processing in the RAW channel instead
> > doing it in INFO_SCALE (same as al3010 does)?
> >  
> 
> From my observation INFO_SCALE will just perform multiplication of RAW
> to SCALE. In this case values which are read are not actual raw values
> of illumination. Next is my assumption (since there is no datasheet),
> but values obtained from register are similar to values from adc
> thermal sensors, they need be converted via reference table to get
> actual data.

Please add a comment somewhere here to say that we don't know the
relationship of these values to illuminance hence providing
_RAW and _SCALE would not be helpful.

> 
> > Except this, LGTM.
> >
> > Documentation and DT patch:
> >
> > Reviewed-by: David Heidelberg <david@ixit.cz>  
> > > +
> > > +             return IIO_VAL_INT;
> > > +     case IIO_CHAN_INFO_SCALE:
> > > +             *val = 1;
> > > +

Don't do this.  The above lack of known relationship has to be
expressed by not providing the _scale attribute.

> > > +             return IIO_VAL_INT;
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +}
> > > +
> > > +static const struct iio_info al3000a_info = {
> > > +     .read_raw       = al3000a_read_raw,
> > > +};

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Svyatoslav Ryhel]

нд, 16 лют. 2025 р. о 16:44 Jonathan Cameron <jic23@kernel.org> пише:
>
>
> > > > +
> > > > +static int al3000a_read_raw(struct iio_dev *indio_dev,
> > > > +                         struct iio_chan_spec const *chan, int *val,
> > > > +                         int *val2, long mask)
> > > > +{
> > > > +     struct al3000a_data *data = iio_priv(indio_dev);
> > > > +     int ret, gain;
> > > > +
> > > > +     switch (mask) {
> > > > +     case IIO_CHAN_INFO_RAW:
> > > > +             ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> > > > +             if (ret < 0)
> > > > +                     return ret;
> > > > +
> > > > +             *val = lux_table[gain & AL3000A_GAIN_MASK];
> > >
> > > Why did you chosen to do post-processing in the RAW channel instead
> > > doing it in INFO_SCALE (same as al3010 does)?
> > >
> >
> > From my observation INFO_SCALE will just perform multiplication of RAW
> > to SCALE. In this case values which are read are not actual raw values
> > of illumination. Next is my assumption (since there is no datasheet),
> > but values obtained from register are similar to values from adc
> > thermal sensors, they need be converted via reference table to get
> > actual data.
>
> Please add a comment somewhere here to say that we don't know the
> relationship of these values to illuminance hence providing
> _RAW and _SCALE would not be helpful.
>

We do know relationship of these values to illuminance thanks to
conversion table provided.

> >
> > > Except this, LGTM.
> > >
> > > Documentation and DT patch:
> > >
> > > Reviewed-by: David Heidelberg <david@ixit.cz>
> > > > +
> > > > +             return IIO_VAL_INT;
> > > > +     case IIO_CHAN_INFO_SCALE:
> > > > +             *val = 1;
> > > > +
>
> Don't do this.  The above lack of known relationship has to be
> expressed by not providing the _scale attribute.
>
> > > > +             return IIO_VAL_INT;
> > > > +     default:
> > > > +             return -EINVAL;
> > > > +     }
> > > > +}
> > > > +
> > > > +static const struct iio_info al3000a_info = {
> > > > +     .read_raw       = al3000a_read_raw,
> > > > +};
>
>

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Jonathan Cameron]

On Sun, 16 Feb 2025 16:47:52 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> нд, 16 лют. 2025 р. о 16:44 Jonathan Cameron <jic23@kernel.org> пише:
> >
> >  
> > > > > +
> > > > > +static int al3000a_read_raw(struct iio_dev *indio_dev,
> > > > > +                         struct iio_chan_spec const *chan, int *val,
> > > > > +                         int *val2, long mask)
> > > > > +{
> > > > > +     struct al3000a_data *data = iio_priv(indio_dev);
> > > > > +     int ret, gain;
> > > > > +
> > > > > +     switch (mask) {
> > > > > +     case IIO_CHAN_INFO_RAW:
> > > > > +             ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> > > > > +             if (ret < 0)
> > > > > +                     return ret;
> > > > > +
> > > > > +             *val = lux_table[gain & AL3000A_GAIN_MASK];  
> > > >
> > > > Why did you chosen to do post-processing in the RAW channel instead
> > > > doing it in INFO_SCALE (same as al3010 does)?
> > > >  
> > >
> > > From my observation INFO_SCALE will just perform multiplication of RAW
> > > to SCALE. In this case values which are read are not actual raw values
> > > of illumination. Next is my assumption (since there is no datasheet),
> > > but values obtained from register are similar to values from adc
> > > thermal sensors, they need be converted via reference table to get
> > > actual data.  
> >
> > Please add a comment somewhere here to say that we don't know the
> > relationship of these values to illuminance hence providing
> > _RAW and _SCALE would not be helpful.
> >  
> 
> We do know relationship of these values to illuminance thanks to
> conversion table provided.

Then convention is treat them not as IIO_LIGHT but as IIO_INTENSITY
which is unit free.  IIO_LIGHT is only for when we know the value
we are providing to userspace is illuminance and measured in Lux.

Jonathan


> 
> > >  
> > > > Except this, LGTM.
> > > >
> > > > Documentation and DT patch:
> > > >
> > > > Reviewed-by: David Heidelberg <david@ixit.cz>  
> > > > > +
> > > > > +             return IIO_VAL_INT;
> > > > > +     case IIO_CHAN_INFO_SCALE:
> > > > > +             *val = 1;
> > > > > +  
> >
> > Don't do this.  The above lack of known relationship has to be
> > expressed by not providing the _scale attribute.
> >  
> > > > > +             return IIO_VAL_INT;
> > > > > +     default:
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +}
> > > > > +
> > > > > +static const struct iio_info al3000a_info = {
> > > > > +     .read_raw       = al3000a_read_raw,
> > > > > +};  
> >
> >  

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Jonathan Cameron]

On Sat, 15 Feb 2025 12:31:58 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> AL3000a is a simple I2C-based ambient light sensor, which is
> closely related to AL3010 and AL3320a, but has significantly
> different way of processing data generated by the sensor.
> 
> Tested-by: Robert Eckelmann <longnoserob@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

Hi,

A few really minor comments inline.  A nice small driver :)

Jonathan


> diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
> new file mode 100644
> index 000000000000..58d4336dd081
> --- /dev/null
> +++ b/drivers/iio/light/al3000a.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
The iio/sysfs.h header is a bit of a legacy thing.  Ideally we will
eventually get rid of it.  In this driver, you are correctly not using
anything it provides so drop this include.

> +
> +static const struct iio_chan_spec al3000a_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
As below. I'm a little confused on units, but this may want to just
be BIT(IIO_CHAN_INFO_PROCESSED)
> +	},
> +};
> +
> +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	u8 val = pwr ? AL3000A_CONFIG_ENABLE : AL3000A_CONFIG_DISABLE;
> +	int ret;
> +
> +	if (pwr) {

The two flows are different enough I'd split this into power on and power
off functions.  Will be a few lines longer but slightly easier to read.

> +		ret = regulator_enable(data->vdd_supply);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable vdd power supply\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to write system register\n");
> +		return ret;
> +	}
> +
> +	if (!pwr) {
> +		ret = regulator_disable(data->vdd_supply);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to disable vdd power supply\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void al3000a_set_pwr_off(void *_data)
> +{
> +	struct al3000a_data *data = _data;
> +
> +	al3000a_set_pwr(data, false);
> +}
> +
> +static int al3000a_init(struct al3000a_data *data)
> +{
> +	int ret;
> +
> +	ret = al3000a_set_pwr(data, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_RESET);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_ENABLE);

regmap functions return <= 0 in all cases. So you can just do if (ret)
or in cases like this save a few lines with

	return regmap_write();

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int al3000a_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct al3000a_data *data = iio_priv(indio_dev);
> +	int ret, gain;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = lux_table[gain & AL3000A_GAIN_MASK];

I may have misinterpreted the other thread.  IS this value in lux?
If it is make this channel IIO_CHAN_INFO_PROCESSED instead.

> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 1;

Either way, default scale is 1 so this should not be expressed at all.

> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info al3000a_info = {
> +	.read_raw	= al3000a_read_raw,

I'd not put a tab in the middle.  Single space is fine.
This sort of alignment is a pain for long term maintenance anyway
as we get very noisy patches realigning things.
It makes even less sense with only one item!

> +};
> +
> +static int al3000a_probe(struct i2c_client *client)
> +{
> +	struct al3000a_data *data;
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &al3000a_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "cannot allocate regmap\n");
> +
> +	data->vdd_supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(data->vdd_supply))
> +		return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
> +				     "failed to get vdd regulator\n");
> +
> +	indio_dev->info = &al3000a_info;
> +	indio_dev->name = AL3000A_DRV_NAME;

As there is no actual reason why this name should necessarily match the
name of the driver I'd prefer to see the string expressed directly in both
places.  That avoids giving the wrong impression as the driver gains support
for additional devices and generally makes things slightly easier to review.

> +	indio_dev->channels = al3000a_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = al3000a_init(data);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to init ALS\n");
> +
> +	ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to add action\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(al3000a_pm_ops, al3000a_suspend, al3000a_resume);
> +
> +static const struct of_device_id al3000a_of_match[] = {
> +	{ .compatible = "dynaimage,al3000a" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, al3000a_of_match);

Also provide the i2c_device_id table as it's used for autoprobing of the
driver (long story for why we still need that!)

> +
> +static struct i2c_driver al3000a_driver = {
> +	.driver = {
> +		.name = AL3000A_DRV_NAME,
> +		.of_match_table = al3000a_of_match,
> +		.pm = pm_sleep_ptr(&al3000a_pm_ops),
> +	},
> +	.probe = al3000a_probe,
> +};
> +module_i2c_driver(al3000a_driver);
> +
> +MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("al3000a Ambient Light Sensor driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Svyatoslav Ryhel]

нд, 16 лют. 2025 р. о 16:54 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Sat, 15 Feb 2025 12:31:58 +0200
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > AL3000a is a simple I2C-based ambient light sensor, which is
> > closely related to AL3010 and AL3320a, but has significantly
> > different way of processing data generated by the sensor.
> >
> > Tested-by: Robert Eckelmann <longnoserob@gmail.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>
> Hi,
>
> A few really minor comments inline.  A nice small driver :)
>
> Jonathan
>
>
> > diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
> > new file mode 100644
> > index 000000000000..58d4336dd081
> > --- /dev/null
> > +++ b/drivers/iio/light/al3000a.c
> > @@ -0,0 +1,221 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/array_size.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/pm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/types.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> The iio/sysfs.h header is a bit of a legacy thing.  Ideally we will
> eventually get rid of it.  In this driver, you are correctly not using
> anything it provides so drop this include.
>
> > +
> > +static const struct iio_chan_spec al3000a_channels[] = {
> > +     {
> > +             .type = IIO_LIGHT,
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +                                   BIT(IIO_CHAN_INFO_SCALE),
> As below. I'm a little confused on units, but this may want to just
> be BIT(IIO_CHAN_INFO_PROCESSED)
> > +     },
> > +};
> > +
> > +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr)
> > +{
> > +     struct device *dev = regmap_get_device(data->regmap);
> > +     u8 val = pwr ? AL3000A_CONFIG_ENABLE : AL3000A_CONFIG_DISABLE;
> > +     int ret;
> > +
> > +     if (pwr) {
>
> The two flows are different enough I'd split this into power on and power
> off functions.  Will be a few lines longer but slightly easier to read.
>
> > +             ret = regulator_enable(data->vdd_supply);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to enable vdd power supply\n");
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, val);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to write system register\n");
> > +             return ret;
> > +     }
> > +
> > +     if (!pwr) {
> > +             ret = regulator_disable(data->vdd_supply);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to disable vdd power supply\n");
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void al3000a_set_pwr_off(void *_data)
> > +{
> > +     struct al3000a_data *data = _data;
> > +
> > +     al3000a_set_pwr(data, false);
> > +}
> > +
> > +static int al3000a_init(struct al3000a_data *data)
> > +{
> > +     int ret;
> > +
> > +     ret = al3000a_set_pwr(data, true);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_RESET);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_ENABLE);
>
> regmap functions return <= 0 in all cases. So you can just do if (ret)
> or in cases like this save a few lines with
>
>         return regmap_write();
>
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static int al3000a_read_raw(struct iio_dev *indio_dev,
> > +                         struct iio_chan_spec const *chan, int *val,
> > +                         int *val2, long mask)
> > +{
> > +     struct al3000a_data *data = iio_priv(indio_dev);
> > +     int ret, gain;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             *val = lux_table[gain & AL3000A_GAIN_MASK];
>
> I may have misinterpreted the other thread.  IS this value in lux?
> If it is make this channel IIO_CHAN_INFO_PROCESSED instead.
>

This is actually a really good hint, I will check if this works out
and if yes, then definitely will use it. Thank you.

> > +
> > +             return IIO_VAL_INT;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             *val = 1;
>
> Either way, default scale is 1 so this should not be expressed at all.
>
> > +
> > +             return IIO_VAL_INT;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +static const struct iio_info al3000a_info = {
> > +     .read_raw       = al3000a_read_raw,
>
> I'd not put a tab in the middle.  Single space is fine.
> This sort of alignment is a pain for long term maintenance anyway
> as we get very noisy patches realigning things.
> It makes even less sense with only one item!
>
> > +};
> > +
> > +static int al3000a_probe(struct i2c_client *client)
> > +{
> > +     struct al3000a_data *data;
> > +     struct device *dev = &client->dev;
> > +     struct iio_dev *indio_dev;
> > +     int ret;
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     data = iio_priv(indio_dev);
> > +     i2c_set_clientdata(client, indio_dev);
> > +
> > +     data->regmap = devm_regmap_init_i2c(client, &al3000a_regmap_config);
> > +     if (IS_ERR(data->regmap))
> > +             return dev_err_probe(dev, PTR_ERR(data->regmap),
> > +                                  "cannot allocate regmap\n");
> > +
> > +     data->vdd_supply = devm_regulator_get(dev, "vdd");
> > +     if (IS_ERR(data->vdd_supply))
> > +             return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
> > +                                  "failed to get vdd regulator\n");
> > +
> > +     indio_dev->info = &al3000a_info;
> > +     indio_dev->name = AL3000A_DRV_NAME;
>
> As there is no actual reason why this name should necessarily match the
> name of the driver I'd prefer to see the string expressed directly in both
> places.  That avoids giving the wrong impression as the driver gains support
> for additional devices and generally makes things slightly easier to review.
>
> > +     indio_dev->channels = al3000a_channels;
> > +     indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +     ret = al3000a_init(data);
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret, "failed to init ALS\n");
> > +
> > +     ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret, "failed to add action\n");
> > +
> > +     return devm_iio_device_register(dev, indio_dev);
> > +}
>
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(al3000a_pm_ops, al3000a_suspend, al3000a_resume);
> > +
> > +static const struct of_device_id al3000a_of_match[] = {
> > +     { .compatible = "dynaimage,al3000a" },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, al3000a_of_match);
>
> Also provide the i2c_device_id table as it's used for autoprobing of the
> driver (long story for why we still need that!)
>
> > +
> > +static struct i2c_driver al3000a_driver = {
> > +     .driver = {
> > +             .name = AL3000A_DRV_NAME,
> > +             .of_match_table = al3000a_of_match,
> > +             .pm = pm_sleep_ptr(&al3000a_pm_ops),
> > +     },
> > +     .probe = al3000a_probe,
> > +};
> > +module_i2c_driver(al3000a_driver);
> > +
> > +MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("al3000a Ambient Light Sensor driver");
> > +MODULE_LICENSE("GPL");
>

Everything else, acknowledged and accepted. Thank you.

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Jonathan Cameron]

Hi,

> > > +static int al3000a_read_raw(struct iio_dev *indio_dev,
> > > +                         struct iio_chan_spec const *chan, int *val,
> > > +                         int *val2, long mask)
> > > +{
> > > +     struct al3000a_data *data = iio_priv(indio_dev);
> > > +     int ret, gain;
> > > +
> > > +     switch (mask) {
> > > +     case IIO_CHAN_INFO_RAW:
> > > +             ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             *val = lux_table[gain & AL3000A_GAIN_MASK];  
> >
> > I may have misinterpreted the other thread.  IS this value in lux?
> > If it is make this channel IIO_CHAN_INFO_PROCESSED instead.
> >  
> 
> This is actually a really good hint, I will check if this works out
> and if yes, then definitely will use it. Thank you.

From your other reply it seems we have no idea of the correct scaling.
If that is the case, then channel type should be IIO_INTENSITY as
I assume we also have no idea if the light sensitivity curve is
matched to that required for illuminance (which approximates the
sensitivity of the human eye).  Various datasheets provide completely
garbage conversion formulas btw so even if we have data this can
be problematic. One recent sensor was using a green filter and
saying illuminance in lux was 1.2 * green which was assuming their
own definition of white light.

Jonathan

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Svyatoslav Ryhel]

пн, 17 лют. 2025 р. о 16:24 Jonathan Cameron <jic23@kernel.org> пише:
>
>
> Hi,
>
> > > > +static int al3000a_read_raw(struct iio_dev *indio_dev,
> > > > +                         struct iio_chan_spec const *chan, int *val,
> > > > +                         int *val2, long mask)
> > > > +{
> > > > +     struct al3000a_data *data = iio_priv(indio_dev);
> > > > +     int ret, gain;
> > > > +
> > > > +     switch (mask) {
> > > > +     case IIO_CHAN_INFO_RAW:
> > > > +             ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> > > > +             if (ret < 0)
> > > > +                     return ret;
> > > > +
> > > > +             *val = lux_table[gain & AL3000A_GAIN_MASK];
> > >
> > > I may have misinterpreted the other thread.  IS this value in lux?
> > > If it is make this channel IIO_CHAN_INFO_PROCESSED instead.
> > >
> >
> > This is actually a really good hint, I will check if this works out
> > and if yes, then definitely will use it. Thank you.
>
> From your other reply it seems we have no idea of the correct scaling.
> If that is the case, then channel type should be IIO_INTENSITY as
> I assume we also have no idea if the light sensitivity curve is
> matched to that required for illuminance (which approximates the
> sensitivity of the human eye). Various datasheets provide completely
> garbage conversion formulas btw so even if we have data this can
> be problematic. One recent sensor was using a green filter and
> saying illuminance in lux was 1.2 * green which was assuming their
> own definition of white light.
>
> Jonathan
>

Then why IIO_LIGHT exists at all? If you state that datasheets provide
garbage formulas and sensors cannot be trusted and all is around human
eye, then why IIO_LIGHT is still the case? I did not recall any
drivers for human eyes (thank god). Please be more consistent. Thank
you

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Jonathan Cameron]

On Mon, 17 Feb 2025 16:32:33 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> пн, 17 лют. 2025 р. о 16:24 Jonathan Cameron <jic23@kernel.org> пише:
> >
> >
> > Hi,
> >  
> > > > > +static int al3000a_read_raw(struct iio_dev *indio_dev,
> > > > > +                         struct iio_chan_spec const *chan, int *val,
> > > > > +                         int *val2, long mask)
> > > > > +{
> > > > > +     struct al3000a_data *data = iio_priv(indio_dev);
> > > > > +     int ret, gain;
> > > > > +
> > > > > +     switch (mask) {
> > > > > +     case IIO_CHAN_INFO_RAW:
> > > > > +             ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> > > > > +             if (ret < 0)
> > > > > +                     return ret;
> > > > > +
> > > > > +             *val = lux_table[gain & AL3000A_GAIN_MASK];  
> > > >
> > > > I may have misinterpreted the other thread.  IS this value in lux?
> > > > If it is make this channel IIO_CHAN_INFO_PROCESSED instead.
> > > >  
> > >
> > > This is actually a really good hint, I will check if this works out
> > > and if yes, then definitely will use it. Thank you.  
> >
> > From your other reply it seems we have no idea of the correct scaling.
> > If that is the case, then channel type should be IIO_INTENSITY as
> > I assume we also have no idea if the light sensitivity curve is
> > matched to that required for illuminance (which approximates the
> > sensitivity of the human eye). Various datasheets provide completely
> > garbage conversion formulas btw so even if we have data this can
> > be problematic. One recent sensor was using a green filter and
> > saying illuminance in lux was 1.2 * green which was assuming their
> > own definition of white light.
> >
> > Jonathan
> >  
> 
> Then why IIO_LIGHT exists at all? If you state that datasheets provide
> garbage formulas and sensors cannot be trusted and all is around human
> eye, then why IIO_LIGHT is still the case? I did not recall any
> drivers for human eyes (thank god). Please be more consistent. Thank

It exists because some sensors do this correctly, or at least a good
approximation to the standard sensitivity curves.  This is done two
ways.

1. Good light frequency filtering in front of the sensor to compensate
   for the difference in sensitivity between the measuring element
   and that the standard curves.  CIE1931 (there are a few other standards
   but they are close enough that we don't care).
   https://en.wikipedia.org/wiki/Illuminance
2. Multiple sensing elements. A common reason for this is to remove
   bit of infrared that we don't want. Often the calculation is a
   non linear combination of the various sensor outputs. Such a driver
   usually presents several IIO_INTENSITY channels and a calculated
   IIO_LIGHT channel.

In both cases the datasheet tends to include a comparison the the
CIE1931 etc standards. There will be small differences but that is
very different from taking a sensor that is only sensitive to green
and weighting it which is the example I gave above.

These sensors will compensate for the different sensivity
of the human eye to different wavelengths.  E.g. if you
think blue and green light LEDs have the same brightness then
the sensor will give close to the same output.

Anyhow, light sensors are a hole I have gone far too deep in over
the years. Key is some manufacturers provide insufficient information
or take the view it is a problem for the integrator of the sensor
to deal with. For those we do not pretend to know the answer and
use intensity channels instead.

Jonathan


> you
> 

Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Svyatoslav Ryhel]

сб, 22 лют. 2025 р. о 14:05 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Mon, 17 Feb 2025 16:32:33 +0200
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > пн, 17 лют. 2025 р. о 16:24 Jonathan Cameron <jic23@kernel.org> пише:
> > >
> > >
> > > Hi,
> > >
> > > > > > +static int al3000a_read_raw(struct iio_dev *indio_dev,
> > > > > > +                         struct iio_chan_spec const *chan, int *val,
> > > > > > +                         int *val2, long mask)
> > > > > > +{
> > > > > > +     struct al3000a_data *data = iio_priv(indio_dev);
> > > > > > +     int ret, gain;
> > > > > > +
> > > > > > +     switch (mask) {
> > > > > > +     case IIO_CHAN_INFO_RAW:
> > > > > > +             ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> > > > > > +             if (ret < 0)
> > > > > > +                     return ret;
> > > > > > +
> > > > > > +             *val = lux_table[gain & AL3000A_GAIN_MASK];
> > > > >
> > > > > I may have misinterpreted the other thread.  IS this value in lux?
> > > > > If it is make this channel IIO_CHAN_INFO_PROCESSED instead.
> > > > >
> > > >
> > > > This is actually a really good hint, I will check if this works out
> > > > and if yes, then definitely will use it. Thank you.
> > >
> > > From your other reply it seems we have no idea of the correct scaling.
> > > If that is the case, then channel type should be IIO_INTENSITY as
> > > I assume we also have no idea if the light sensitivity curve is
> > > matched to that required for illuminance (which approximates the
> > > sensitivity of the human eye). Various datasheets provide completely
> > > garbage conversion formulas btw so even if we have data this can
> > > be problematic. One recent sensor was using a green filter and
> > > saying illuminance in lux was 1.2 * green which was assuming their
> > > own definition of white light.
> > >
> > > Jonathan
> > >
> >
> > Then why IIO_LIGHT exists at all? If you state that datasheets provide
> > garbage formulas and sensors cannot be trusted and all is around human
> > eye, then why IIO_LIGHT is still the case? I did not recall any
> > drivers for human eyes (thank god). Please be more consistent. Thank
>
> It exists because some sensors do this correctly, or at least a good
> approximation to the standard sensitivity curves.  This is done two
> ways.
>
> 1. Good light frequency filtering in front of the sensor to compensate
>    for the difference in sensitivity between the measuring element
>    and that the standard curves.  CIE1931 (there are a few other standards
>    but they are close enough that we don't care).
>    https://en.wikipedia.org/wiki/Illuminance
> 2. Multiple sensing elements. A common reason for this is to remove
>    bit of infrared that we don't want. Often the calculation is a
>    non linear combination of the various sensor outputs. Such a driver
>    usually presents several IIO_INTENSITY channels and a calculated
>    IIO_LIGHT channel.
>
> In both cases the datasheet tends to include a comparison the the
> CIE1931 etc standards. There will be small differences but that is
> very different from taking a sensor that is only sensitive to green
> and weighting it which is the example I gave above.
>
> These sensors will compensate for the different sensivity
> of the human eye to different wavelengths.  E.g. if you
> think blue and green light LEDs have the same brightness then
> the sensor will give close to the same output.
>
> Anyhow, light sensors are a hole I have gone far too deep in over
> the years. Key is some manufacturers provide insufficient information
> or take the view it is a problem for the integrator of the sensor
> to deal with. For those we do not pretend to know the answer and
> use intensity channels instead.
>

This is quite an explicit explanation. Thank you.

> Jonathan
>
>
> > you
> >
>

[PATCH v2 3/3] ARM: tegra: tf101: Add al3000a illuminance sensor node

[Svyatoslav Ryhel]

Bind al3000a illuminance sensor found in ASUS TF101

Tested-by: Robert Eckelmann <longnoserob@gmail.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 arch/arm/boot/dts/nvidia/tegra20-asus-tf101.dts | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/nvidia/tegra20-asus-tf101.dts b/arch/arm/boot/dts/nvidia/tegra20-asus-tf101.dts
index e118809dc6d9..67764afeb013 100644
--- a/arch/arm/boot/dts/nvidia/tegra20-asus-tf101.dts
+++ b/arch/arm/boot/dts/nvidia/tegra20-asus-tf101.dts
@@ -1085,6 +1085,17 @@ smart-battery@b {
 				sbs,poll-retry-count = <10>;
 				power-supplies = <&mains>;
 			};
+
+			/* Dynaimage ambient light sensor */
+			light-sensor@1c {
+				compatible = "dynaimage,al3000a";
+				reg = <0x1c>;
+
+				interrupt-parent = <&gpio>;
+				interrupts = <TEGRA_GPIO(Z, 2) IRQ_TYPE_LEVEL_HIGH>;
+
+				vdd-supply = <&vdd_1v8_sys>;
+			};
 		};
 	};
 
-- 
2.43.0