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

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

[Svyatoslav Ryhel]

AL3000a is an illuminance sensor found in ASUS TF101 tablet.

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                   | 214 ++++++++++++++++++
 5 files changed, 240 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iio/light/al3000a.c

-- 
2.43.0

[PATCH v1 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 re-use 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..6db4dfd5aa6c 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,al3010
+      - dynaimage,al3000a
 
   reg:
     maxItems: 1
-- 
2.43.0

[PATCH v1 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 hardware configuration.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/iio/light/Kconfig   |  10 ++
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/al3000a.c | 214 ++++++++++++++++++++++++++++++++++++
 3 files changed, 225 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..9e1f2ac6a933
--- /dev/null
+++ b/drivers/iio/light/al3000a.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AL3000a - Dyna Image Ambient Light Sensor
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.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
+
+/*
+ * This are pre-calculated lux values based on possible output
+ * of sensor (range 0x00 - 0x3F)
+ */
+static const u32 lux_table[64] = {
+	1, 1, 1, 2, 2, 2, 3, 4, 4, 5, 6, 7, 9, 11, 13, 16,
+	19, 22, 27, 32, 39, 46, 56, 67, 80, 96, 116, 139,
+	167, 200, 240, 289, 347, 416, 499, 600, 720, 864,
+	1037, 1245, 1495, 1795, 2155, 2587, 3105, 3728,	4475,
+	5373, 6450, 7743, 9296, 11160, 13397, 16084, 19309,
+	23180, 27828, 33408, 40107, 48148, 57803, 69393,
+	83306, 100000
+};
+
+struct al3000a_data {
+	struct i2c_client *client;
+	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 = &data->client->dev;
+	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 = i2c_smbus_write_byte_data(data->client, 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 = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM,
+					AL3000A_CONFIG_RESET);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(data->client, 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;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = i2c_smbus_read_byte_data(data->client,
+					       AL3000A_REG_DATA);
+		if (ret < 0)
+			return ret;
+
+		*val = lux_table[ret & 0x3F];
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 1;
+
+		return IIO_VAL_INT;
+	default:
+		break;
+	}
+
+	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 iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+
+	data->vdd_supply = devm_regulator_get(&client->dev, "vdd");
+	if (IS_ERR(data->vdd_supply))
+		return dev_err_probe(&client->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(&client->dev, ret,
+				     "failed to init ALS\n");
+
+	ret = devm_add_action_or_reset(&client->dev, al3000a_set_pwr_off,
+				       data);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to add action\n");
+
+	return devm_iio_device_register(&client->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

[PATCH v1 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

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

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 08:46:56AM +0200, Svyatoslav Ryhel wrote:
> AL3000a is a simple I2C-based ambient light sensor, which is
> closely related to AL3010 and AL3320a, but has significantly
> different hardware configuration.

(Note, the part of the below comments are applicable to your other series)

...

> +/*
> + * AL3000a - Dyna Image Ambient Light Sensor
> + */

Can be on a single line.

...

> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>

> +#include <linux/of.h>

No of*.h in the new code, please.

> +#include <linux/regulator/consumer.h>

Too small headers to be included. You use much more.


> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

...

> +/*
> + * This are pre-calculated lux values based on possible output
> + * of sensor (range 0x00 - 0x3F)
> + */

types.h

> +static const u32 lux_table[64] = {

I think you don't need 64 to be there, but okay, I understand the intention.

> +	1, 1, 1, 2, 2, 2, 3, 4, 4, 5, 6, 7, 9, 11, 13, 16,

For the better readability and maintenance put pow-of-2 amount of values per
line, like 8, and add the respective comment:

	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, 80, 96, 116, 139,
> +	167, 200, 240, 289, 347, 416, 499, 600, 720, 864,
> +	1037, 1245, 1495, 1795, 2155, 2587, 3105, 3728,	4475,
> +	5373, 6450, 7743, 9296, 11160, 13397, 16084, 19309,
> +	23180, 27828, 33408, 40107, 48148, 57803, 69393,
> +	83306, 100000

Leave trailing comma, it's not a terminated list generally speaking
(in the future it might grow).

> +};

...

> +struct al3000a_data {
> +	struct i2c_client *client;

	struct regmap *map;

will suffice, I believe, see below.


> +	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),

> +	}

Leave trailing comma

> +};

...

> +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr)
> +{
> +	struct device *dev = &data->client->dev;
> +	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;

With struct regmap *map in mind, the struct device *dev can be derived using
the respective API.

> +		}
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, val);

Why not using regmap I²C APIs?

> +	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 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;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:

> +		ret = i2c_smbus_read_byte_data(data->client,
> +					       AL3000A_REG_DATA);

It may be a single line. There is a lot of room.

> +		if (ret < 0)
> +			return ret;
> +
> +		*val = lux_table[ret & 0x3F];

I believe you want to define the size of that table and use it here.
Also this needs a comment to explain the meaning of the ret >= 64 and
when it may happen.

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

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

Return directly from the default case.

> +}

...

> +static int al3000a_probe(struct i2c_client *client)
> +{
> +	struct al3000a_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;

	struct device *dev = &client->dev;

will make the below lines shorter and easier to read.


> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	data->vdd_supply = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(data->vdd_supply))
> +		return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply),
> +				     "failed to get vdd regulator\n");

err.h

> +	indio_dev->info = &al3000a_info;
> +	indio_dev->name = AL3000A_DRV_NAME;
> +	indio_dev->channels = al3000a_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);

array_size.h

> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = al3000a_init(data);
> +	if (ret < 0)

> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to init ALS\n");

Single line.

> +	ret = devm_add_action_or_reset(&client->dev, al3000a_set_pwr_off,
> +				       data);

Ditto.

device.h

> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to add action\n");
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}

...

> +static const struct of_device_id al3000a_of_match[] = {

mod_devicetable.h

> +	{ .compatible = "dynaimage,al3000a" },
> +	{ /* sentinel */ }
> +};

...

> +static struct i2c_driver al3000a_driver = {
> +	.driver = {
> +		.name = AL3000A_DRV_NAME,
> +		.of_match_table = al3000a_of_match,

> +		.pm = pm_sleep_ptr(&al3000a_pm_ops),

pm.h

> +	},
> +	.probe = al3000a_probe,
> +};

-- 
With Best Regards,
Andy Shevchenko

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

[Svyatoslav Ryhel]

ср, 12 лют. 2025 р. о 16:28 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> пише:
>
> On Wed, Feb 12, 2025 at 08:46:56AM +0200, Svyatoslav Ryhel wrote:
> > AL3000a is a simple I2C-based ambient light sensor, which is
> > closely related to AL3010 and AL3320a, but has significantly
> > different hardware configuration.
>
> (Note, the part of the below comments are applicable to your other series)
>
> ...
>
> > +/*
> > + * AL3000a - Dyna Image Ambient Light Sensor
> > + */
>
> Can be on a single line.
>

Patch checking script did not catch this as warning or style issue. If
such commenting is discouraged than please add this to patch checking
script. Comments are stripped on compilation anyway, they do not
affect size.

> ...
>
> > +#include <linux/bitfield.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
>
> > +#include <linux/of.h>
>
> No of*.h in the new code, please.
>
> > +#include <linux/regulator/consumer.h>
>
> Too small headers to be included. You use much more.
>

Is there a check which determines the amount of headers I must include
and which headers are mandatory to be included and which are forbidden
to inclusion. Maybe at least a list? Thanks

>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
>
> ...
>
> > +/*
> > + * This are pre-calculated lux values based on possible output
> > + * of sensor (range 0x00 - 0x3F)
> > + */
>
> types.h
>
> > +static const u32 lux_table[64] = {
>
> I think you don't need 64 to be there, but okay, I understand the intention.
>
> > +     1, 1, 1, 2, 2, 2, 3, 4, 4, 5, 6, 7, 9, 11, 13, 16,
>
> For the better readability and maintenance put pow-of-2 amount of values per
> line, like 8, and add the respective comment:
>
>         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, 80, 96, 116, 139,
> > +     167, 200, 240, 289, 347, 416, 499, 600, 720, 864,
> > +     1037, 1245, 1495, 1795, 2155, 2587, 3105, 3728, 4475,
> > +     5373, 6450, 7743, 9296, 11160, 13397, 16084, 19309,
> > +     23180, 27828, 33408, 40107, 48148, 57803, 69393,
> > +     83306, 100000
>
> Leave trailing comma, it's not a terminated list generally speaking
> (in the future it might grow).

No, this list will not grow since the bit field seems to be 0x3f
(datasheet is not available, code is adaptation of downstream driver).

> > +};
>
> ...
>
> > +struct al3000a_data {
> > +     struct i2c_client *client;
>
>         struct regmap *map;
>
> will suffice, I believe, see below.
>
>
> > +     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),
>
> > +     }
>
> Leave trailing comma
>
> > +};
>
> ...
>
> > +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr)
> > +{
> > +     struct device *dev = &data->client->dev;
> > +     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;
>
> With struct regmap *map in mind, the struct device *dev can be derived using
> the respective API.
>
> > +             }
> > +     }
> > +
> > +     ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, val);
>
> Why not using regmap I涎 APIs?
>

This adaptation was written quite a long time ago, patch check did not
complained about using of i2c smbus. Is use of regmap mandatory now?
Is it somewhere specified? Thanks

I am not a full time linux contributor and may not be familiar with
the recent rules.

> > +     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 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;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
>
> > +             ret = i2c_smbus_read_byte_data(data->client,
> > +                                            AL3000A_REG_DATA);
>
> It may be a single line. There is a lot of room.
>
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             *val = lux_table[ret & 0x3F];
>
> I believe you want to define the size of that table and use it here.
> Also this needs a comment to explain the meaning of the ret >= 64 and
> when it may happen.
>
> > +             return IIO_VAL_INT;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             *val = 1;
> > +
> > +             return IIO_VAL_INT;
>
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return -EINVAL;
>
> Return directly from the default case.
>
> > +}
>
> ...
>
> > +static int al3000a_probe(struct i2c_client *client)
> > +{
> > +     struct al3000a_data *data;
> > +     struct iio_dev *indio_dev;
> > +     int ret;
>
>         struct device *dev = &client->dev;
>
> will make the below lines shorter and easier to read.
>
>
> > +     indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     data = iio_priv(indio_dev);
> > +     i2c_set_clientdata(client, indio_dev);
> > +     data->client = client;
> > +
> > +     data->vdd_supply = devm_regulator_get(&client->dev, "vdd");
> > +     if (IS_ERR(data->vdd_supply))
> > +             return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply),
> > +                                  "failed to get vdd regulator\n");
>
> err.h
>
> > +     indio_dev->info = &al3000a_info;
> > +     indio_dev->name = AL3000A_DRV_NAME;
> > +     indio_dev->channels = al3000a_channels;
> > +     indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);
>
> array_size.h
>
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +     ret = al3000a_init(data);
> > +     if (ret < 0)
>
> > +             return dev_err_probe(&client->dev, ret,
> > +                                  "failed to init ALS\n");
>
> Single line.
>
> > +     ret = devm_add_action_or_reset(&client->dev, al3000a_set_pwr_off,
> > +                                    data);
>
> Ditto.
>
> device.h
>
> > +     if (ret < 0)
> > +             return dev_err_probe(&client->dev, ret,
> > +                                  "failed to add action\n");
> > +
> > +     return devm_iio_device_register(&client->dev, indio_dev);
> > +}
>
> ...
>
> > +static const struct of_device_id al3000a_of_match[] = {
>
> mod_devicetable.h
>
> > +     { .compatible = "dynaimage,al3000a" },
> > +     { /* sentinel */ }
> > +};
>
> ...
>
> > +static struct i2c_driver al3000a_driver = {
> > +     .driver = {
> > +             .name = AL3000A_DRV_NAME,
> > +             .of_match_table = al3000a_of_match,
>
> > +             .pm = pm_sleep_ptr(&al3000a_pm_ops),
>
> pm.h
>
> > +     },
> > +     .probe = al3000a_probe,
> > +};
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Everything else is valid, thank you. I will add fixes and try to
switch to regmap.

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

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 05:20:04PM +0200, Svyatoslav Ryhel wrote:
> ср, 12 лют. 2025 р. о 16:28 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> пише:
> > On Wed, Feb 12, 2025 at 08:46:56AM +0200, Svyatoslav Ryhel wrote:

...

> > > +/*
> > > + * AL3000a - Dyna Image Ambient Light Sensor
> > > + */
> >
> > Can be on a single line.
> 
> Patch checking script did not catch this as warning or style issue. If
> such commenting is discouraged than please add this to patch checking
> script. Comments are stripped on compilation anyway, they do not
> affect size.

First of all, it uses verb 'can' for a reason (it's not anyhow big deal).
Second, not all stuff should be documented or scripted, we called it
a "common sense". The common sense rule in the code is: "Do not introduce
lines that are not needed or do not add a value". I see these 3 LoCs can
be replaced without any downsides to 1 LoC and make it even more readable,
less consumable of the resources, and more informative as opening the
first page in the editor will give me more code than mostly unrelated
comments.

...

> > > +#include <linux/bitfield.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> >
> > > +#include <linux/of.h>
> >
> > No of*.h in the new code, please.
> >
> > > +#include <linux/regulator/consumer.h>
> >
> > Too small headers to be included. You use much more.
> 
> Is there a check which determines the amount of headers I must include
> and which headers are mandatory to be included and which are forbidden
> to inclusion. Maybe at least a list? Thanks

No check, there is IWYU principle.

https://include-what-you-use.org/

The tool is not (yet?) suitable for the Linux kernel project and Jonathan,
who is the maintainer of IIO code, had even tried it for real some time ago.

> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>

...

> > > +static const u32 lux_table[64] = {
> >
> > I think you don't need 64 to be there, but okay, I understand the intention.
> >
> > > +     1, 1, 1, 2, 2, 2, 3, 4, 4, 5, 6, 7, 9, 11, 13, 16,
> >
> > For the better readability and maintenance put pow-of-2 amount of values per
> > line, like 8, and add the respective comment:
> >
> >         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, 80, 96, 116, 139,
> > > +     167, 200, 240, 289, 347, 416, 499, 600, 720, 864,
> > > +     1037, 1245, 1495, 1795, 2155, 2587, 3105, 3728, 4475,
> > > +     5373, 6450, 7743, 9296, 11160, 13397, 16084, 19309,
> > > +     23180, 27828, 33408, 40107, 48148, 57803, 69393,
> > > +     83306, 100000
> >
> > Leave trailing comma, it's not a terminated list generally speaking
> > (in the future it might grow).
> 
> No, this list will not grow since the bit field seems to be 0x3f
> (datasheet is not available, code is adaptation of downstream driver).

You never know. Sometimes driver is getting reused to support other compatible
HW. Telling you from the experience.

> > > +};

...

> > > +     ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, val);
> >
> > Why not using regmap I涎 APIs?
> 
> This adaptation was written quite a long time ago, patch check did not
> complained about using of i2c smbus. Is use of regmap mandatory now?
> Is it somewhere specified? Thanks

It adds a value to the code (in particular debugfs for free and
many nice helper APIs). It's recommended and many maintainers would like
to have it. It's rare that some of the generic framework or library committed
into the kernel just left to become rotten there.

> I am not a full time linux contributor and may not be familiar with
> the recent rules.

Many are not the rules so far, but recommendations and/or preferences by
certain maintainer(s).

-- 
With Best Regards,
Andy Shevchenko

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

[Svyatoslav Ryhel]

ср, 12 лют. 2025 р. о 18:10 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> пише:
>
> On Wed, Feb 12, 2025 at 05:20:04PM +0200, Svyatoslav Ryhel wrote:
> > ср, 12 лют. 2025 р. о 16:28 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> пише:
> > > On Wed, Feb 12, 2025 at 08:46:56AM +0200, Svyatoslav Ryhel wrote:
>
> ...
>
> > > > +/*
> > > > + * AL3000a - Dyna Image Ambient Light Sensor
> > > > + */
> > >
> > > Can be on a single line.
> >
> > Patch checking script did not catch this as warning or style issue. If
> > such commenting is discouraged than please add this to patch checking
> > script. Comments are stripped on compilation anyway, they do not
> > affect size.
>
> First of all, it uses verb 'can' for a reason (it's not anyhow big deal).
> Second, not all stuff should be documented or scripted, we called it
> a "common sense". The common sense rule in the code is: "Do not introduce
> lines that are not needed or do not add a value". I see these 3 LoCs can
> be replaced without any downsides to 1 LoC and make it even more readable,
> less consumable of the resources, and more informative as opening the
> first page in the editor will give me more code than mostly unrelated
> comments.
>
> ...
>
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/module.h>
> > >
> > > > +#include <linux/of.h>
> > >
> > > No of*.h in the new code, please.
> > >
> > > > +#include <linux/regulator/consumer.h>
> > >
> > > Too small headers to be included. You use much more.
> >
> > Is there a check which determines the amount of headers I must include
> > and which headers are mandatory to be included and which are forbidden
> > to inclusion. Maybe at least a list? Thanks
>
> No check, there is IWYU principle.
>
> https://include-what-you-use.org/
>
> The tool is not (yet?) suitable for the Linux kernel project and Jonathan,
> who is the maintainer of IIO code, had even tried it for real some time ago.
>

So it is not adopted by the Linux kernel. Lets return to this once it
will be adopted.

> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/sysfs.h>
>
> ...
>
> > > > +static const u32 lux_table[64] = {
> > >
> > > I think you don't need 64 to be there, but okay, I understand the intention.
> > >
> > > > +     1, 1, 1, 2, 2, 2, 3, 4, 4, 5, 6, 7, 9, 11, 13, 16,
> > >
> > > For the better readability and maintenance put pow-of-2 amount of values per
> > > line, like 8, and add the respective comment:
> > >
> > >         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, 80, 96, 116, 139,
> > > > +     167, 200, 240, 289, 347, 416, 499, 600, 720, 864,
> > > > +     1037, 1245, 1495, 1795, 2155, 2587, 3105, 3728, 4475,
> > > > +     5373, 6450, 7743, 9296, 11160, 13397, 16084, 19309,
> > > > +     23180, 27828, 33408, 40107, 48148, 57803, 69393,
> > > > +     83306, 100000
> > >
> > > Leave trailing comma, it's not a terminated list generally speaking
> > > (in the future it might grow).
> >
> > No, this list will not grow since the bit field seems to be 0x3f
> > (datasheet is not available, code is adaptation of downstream driver).
>
> You never know. Sometimes driver is getting reused to support other compatible
> HW. Telling you from the experience.
>
> > > > +};
>
> ...
>
> > > > +     ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, val);
> > >
> > > Why not using regmap I涎 APIs?
> >
> > This adaptation was written quite a long time ago, patch check did not
> > complained about using of i2c smbus. Is use of regmap mandatory now?
> > Is it somewhere specified? Thanks
>
> It adds a value to the code (in particular debugfs for free and
> many nice helper APIs). It's recommended and many maintainers would like
> to have it. It's rare that some of the generic framework or library committed
> into the kernel just left to become rotten there.
>
> > I am not a full time linux contributor and may not be familiar with
> > the recent rules.
>
> Many are not the rules so far, but recommendations and/or preferences by
> certain maintainer(s).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

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

[Svyatoslav Ryhel]

ср, 12 лют. 2025 р. о 18:10 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> пише:
>
> On Wed, Feb 12, 2025 at 05:20:04PM +0200, Svyatoslav Ryhel wrote:
> > ср, 12 лют. 2025 р. о 16:28 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> пише:
> > > On Wed, Feb 12, 2025 at 08:46:56AM +0200, Svyatoslav Ryhel wrote:
>
> ...
>
> > > > +/*
> > > > + * AL3000a - Dyna Image Ambient Light Sensor
> > > > + */
> > >
> > > Can be on a single line.
> >
> > Patch checking script did not catch this as warning or style issue. If
> > such commenting is discouraged than please add this to patch checking
> > script. Comments are stripped on compilation anyway, they do not
> > affect size.
>
> First of all, it uses verb 'can' for a reason (it's not anyhow big deal).
> Second, not all stuff should be documented or scripted, we called it
> a "common sense". The common sense rule in the code is: "Do not introduce
> lines that are not needed or do not add a value". I see these 3 LoCs can
> be replaced without any downsides to 1 LoC and make it even more readable,
> less consumable of the resources, and more informative as opening the
> first page in the editor will give me more code than mostly unrelated
> comments.
>
> ...
>
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/module.h>
> > >
> > > > +#include <linux/of.h>
> > >
> > > No of*.h in the new code, please.
> > >
> > > > +#include <linux/regulator/consumer.h>
> > >
> > > Too small headers to be included. You use much more.
> >
> > Is there a check which determines the amount of headers I must include
> > and which headers are mandatory to be included and which are forbidden
> > to inclusion. Maybe at least a list? Thanks
>
> No check, there is IWYU principle.
>
> https://include-what-you-use.org/
>
> The tool is not (yet?) suitable for the Linux kernel project and Jonathan,
> who is the maintainer of IIO code, had even tried it for real some time ago.
>
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/sysfs.h>
>
> ...
>
> > > > +static const u32 lux_table[64] = {
> > >
> > > I think you don't need 64 to be there, but okay, I understand the intention.
> > >
> > > > +     1, 1, 1, 2, 2, 2, 3, 4, 4, 5, 6, 7, 9, 11, 13, 16,
> > >
> > > For the better readability and maintenance put pow-of-2 amount of values per
> > > line, like 8, and add the respective comment:
> > >
> > >         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, 80, 96, 116, 139,
> > > > +     167, 200, 240, 289, 347, 416, 499, 600, 720, 864,
> > > > +     1037, 1245, 1495, 1795, 2155, 2587, 3105, 3728, 4475,
> > > > +     5373, 6450, 7743, 9296, 11160, 13397, 16084, 19309,
> > > > +     23180, 27828, 33408, 40107, 48148, 57803, 69393,
> > > > +     83306, 100000
> > >
> > > Leave trailing comma, it's not a terminated list generally speaking
> > > (in the future it might grow).
> >
> > No, this list will not grow since the bit field seems to be 0x3f
> > (datasheet is not available, code is adaptation of downstream driver).
>
> You never know. Sometimes driver is getting reused to support other compatible
> HW. Telling you from the experience.
>
> > > > +};
>
> ...
>
> > > > +     ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, val);
> > >
> > > Why not using regmap I涎 APIs?
> >
> > This adaptation was written quite a long time ago, patch check did not
> > complained about using of i2c smbus. Is use of regmap mandatory now?
> > Is it somewhere specified? Thanks
>
> It adds a value to the code (in particular debugfs for free and
> many nice helper APIs). It's recommended and many maintainers would like
> to have it. It's rare that some of the generic framework or library committed
> into the kernel just left to become rotten there.
>
> > I am not a full time linux contributor and may not be familiar with
> > the recent rules.
>
> Many are not the rules so far, but recommendations and/or preferences by
> certain maintainer(s).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

I will apply all your suggestions. Thank you.

Regards other patch series, may you please contain all advice inside
patch series since it is quite hard to track between them. For future
patches, I will try to avoid listed issues. Thank you.

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

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 06:36:37PM +0200, Svyatoslav Ryhel wrote:
> ср, 12 лют. 2025 р. о 18:10 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> пише:
> > On Wed, Feb 12, 2025 at 05:20:04PM +0200, Svyatoslav Ryhel wrote:
> > > ср, 12 лют. 2025 р. о 16:28 Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> пише:
> > > > On Wed, Feb 12, 2025 at 08:46:56AM +0200, Svyatoslav Ryhel wrote:

...

> > > > > +#include <linux/i2c.h>
> > > > > +#include <linux/module.h>
> > > >
> > > > > +#include <linux/of.h>
> > > >
> > > > No of*.h in the new code, please.
> > > >
> > > > > +#include <linux/regulator/consumer.h>
> > > >
> > > > Too small headers to be included. You use much more.
> > >
> > > Is there a check which determines the amount of headers I must include
> > > and which headers are mandatory to be included and which are forbidden
> > > to inclusion. Maybe at least a list? Thanks
> >
> > No check, there is IWYU principle.
> >
> > https://include-what-you-use.org/
> >
> > The tool is not (yet?) suitable for the Linux kernel project and Jonathan,
> > who is the maintainer of IIO code, had even tried it for real some time ago.
> 
> So it is not adopted by the Linux kernel.
> Lets return to this once it will be adopted.

I understand you want to push your way, but here is the thing. This is a
community of people and review is not something that comes for free. People,
who are reviewing a code, want to make sure the code follows not only
documented style, etc., but also common sense and the future maintenance.
When a contributor comes and drops something into Linux Kernel project
it adds a lot of work on maintainers' shoulders and other contributors
who may be in progress of solving the other tasks. I specifically sent
you a link where the tool and the principle is _explained_. So, it's not
about the tool, it's about the whole project to become better and easier
to maintain. You are a new guy in the development as you stated, so,
please try to see how this all works.

Of course, the last word here is Jonathan's as he is the maintainer of IIO,
but I truly believe he will suggest you to follow my advice and not otherwise.

> > > > > +#include <linux/iio/iio.h>
> > > > > +#include <linux/iio/sysfs.h>

...

I assume the non-commented parts you are satisfied with and they will be
addressed as suggested. Thank you!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 07:28:01PM +0200, Svyatoslav Ryhel wrote:
> ср, 12 лют. 2025 р. о 18:10 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> пише:

...

> I will apply all your suggestions. Thank you.
> 
> Regards other patch series, may you please contain all advice inside
> patch series since it is quite hard to track between them. For future
> patches, I will try to avoid listed issues. Thank you.

Sure, it was just a hint. I will check the other series as well when
I have time and motivation.

-- 
With Best Regards,
Andy Shevchenko

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

On Wed, Feb 12, 2025 at 08:46:55AM +0200, Svyatoslav Ryhel wrote:
> AL3000a is an ambient light sensor quite closely related to
> exising AL3010 and can re-use exising schema for AL3010.

Quite close you say, but the driver is entirely different it seems. How
closely related is the hardware itself?

> 
> 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..6db4dfd5aa6c 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,al3010
> +      - dynaimage,al3000a
>  
>    reg:
>      maxItems: 1
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Svyatoslav Ryhel]

ср, 12 лют. 2025 р. о 21:20 Conor Dooley <conor@kernel.org> пише:
>
> On Wed, Feb 12, 2025 at 08:46:55AM +0200, Svyatoslav Ryhel wrote:
> > AL3000a is an ambient light sensor quite closely related to
> > exising AL3010 and can re-use exising schema for AL3010.
>
> Quite close you say, but the driver is entirely different it seems. How
> closely related is the hardware itself?
>

Well, I can simply duplicate al3010 or al3320a schema if re-using
schema is not allowed. AL3000a has no available datasheet online.
Downstream code for al3000a and al3010 seems to have same principles,
apart from light measurements.

> >
> > 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..6db4dfd5aa6c 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,al3010
> > +      - dynaimage,al3000a
> >
> >    reg:
> >      maxItems: 1
> > --
> > 2.43.0
> >

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

[Krzysztof Kozlowski]

On Wed, Feb 12, 2025 at 08:46:55AM +0200, Svyatoslav Ryhel wrote:
> AL3000a is an ambient light sensor quite closely related to
> exising AL3010 and can re-use 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..6db4dfd5aa6c 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,al3010
> +      - dynaimage,al3000a

If this stays here, keep alphabetical order.

Best regards,
Krzysztof

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

[Svyatoslav Ryhel]

чт, 13 лют. 2025 р. о 11:11 Krzysztof Kozlowski <krzk@kernel.org> пише:
>
> On Wed, Feb 12, 2025 at 08:46:55AM +0200, Svyatoslav Ryhel wrote:
> > AL3000a is an ambient light sensor quite closely related to
> > exising AL3010 and can re-use 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..6db4dfd5aa6c 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,al3010
> > +      - dynaimage,al3000a
>
> If this stays here, keep alphabetical order.
>

Acknowledged, thank you.

> Best regards,
> Krzysztof
>

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2187 bytes --]

On Wed, Feb 12, 2025 at 09:39:06PM +0200, Svyatoslav Ryhel wrote:
> ср, 12 лют. 2025 р. о 21:20 Conor Dooley <conor@kernel.org> пише:
> >
> > On Wed, Feb 12, 2025 at 08:46:55AM +0200, Svyatoslav Ryhel wrote:
> > > AL3000a is an ambient light sensor quite closely related to
> > > exising AL3010 and can re-use exising schema for AL3010.
> >
> > Quite close you say, but the driver is entirely different it seems. How
> > closely related is the hardware itself?
> >
> 
> Well, I can simply duplicate al3010 or al3320a schema if re-using
> schema is not allowed. AL3000a has no available datasheet online.
> Downstream code for al3000a and al3010 seems to have same principles,
> apart from light measurements.

It's probably more of a question as to why you're duplicating the driver
for them, rather than telling you not to put both bindings together.
That said, information on what's actually different is helpful in the
binding, to explain why you're not using a fallback compatible etc.

> 
> > >
> > > 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..6db4dfd5aa6c 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,al3010
> > > +      - dynaimage,al3000a
> > >
> > >    reg:
> > >      maxItems: 1
> > > --
> > > 2.43.0
> > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Svyatoslav Ryhel]

чт, 13 лют. 2025 р. о 22:15 Conor Dooley <conor@kernel.org> пише:
>
> On Wed, Feb 12, 2025 at 09:39:06PM +0200, Svyatoslav Ryhel wrote:
> > ср, 12 лют. 2025 р. о 21:20 Conor Dooley <conor@kernel.org> пише:
> > >
> > > On Wed, Feb 12, 2025 at 08:46:55AM +0200, Svyatoslav Ryhel wrote:
> > > > AL3000a is an ambient light sensor quite closely related to
> > > > exising AL3010 and can re-use exising schema for AL3010.
> > >
> > > Quite close you say, but the driver is entirely different it seems. How
> > > closely related is the hardware itself?
> > >
> >
> > Well, I can simply duplicate al3010 or al3320a schema if re-using
> > schema is not allowed. AL3000a has no available datasheet online.
> > Downstream code for al3000a and al3010 seems to have same principles,
> > apart from light measurements.
>
> It's probably more of a question as to why you're duplicating the driver
> for them, rather than telling you not to put both bindings together.
> That said, information on what's actually different is helpful in the
> binding, to explain why you're not using a fallback compatible etc.
>

Quoting writing-bindings.rst:
DON'T refer to Linux or "device driver" in bindings. Bindings should
be based on what the hardware has, not what an OS and driver currently
support.

From all available data, hw configuration of al3000a closely matches
al3010 and seems to be part of same sensor lineup. It is not
prohibited to add new compatibles to existing schema. Schema does not
take in account way of processing data generated by sensor and this is
the main difference between al3000a and 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..6db4dfd5aa6c 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,al3010
> > > > +      - dynaimage,al3000a
> > > >
> > > >    reg:
> > > >      maxItems: 1
> > > > --
> > > > 2.43.0
> > > >

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

On Fri, Feb 14, 2025 at 08:21:03AM +0200, Svyatoslav Ryhel wrote:
> чт, 13 лют. 2025 р. о 22:15 Conor Dooley <conor@kernel.org> пише:
> >
> > On Wed, Feb 12, 2025 at 09:39:06PM +0200, Svyatoslav Ryhel wrote:
> > > ср, 12 лют. 2025 р. о 21:20 Conor Dooley <conor@kernel.org> пише:
> > > >
> > > > On Wed, Feb 12, 2025 at 08:46:55AM +0200, Svyatoslav Ryhel wrote:
> > > > > AL3000a is an ambient light sensor quite closely related to
> > > > > exising AL3010 and can re-use exising schema for AL3010.
> > > >
> > > > Quite close you say, but the driver is entirely different it seems. How
> > > > closely related is the hardware itself?
> > > >
> > >
> > > Well, I can simply duplicate al3010 or al3320a schema if re-using
> > > schema is not allowed. AL3000a has no available datasheet online.
> > > Downstream code for al3000a and al3010 seems to have same principles,
> > > apart from light measurements.
> >
> > It's probably more of a question as to why you're duplicating the driver
> > for them, rather than telling you not to put both bindings together.
> > That said, information on what's actually different is helpful in the
> > binding, to explain why you're not using a fallback compatible etc.
> >
> 
> Quoting writing-bindings.rst:
> DON'T refer to Linux or "device driver" in bindings. Bindings should
> be based on what the hardware has, not what an OS and driver currently
> support.

No need to quite that back at me, I'm the one usually attempting to
enforce these things. I just expect more information about the
similiarties/differences when you're content splitting into two drivers
but want to reuse the same binding.

> 
> From all available data, hw configuration of al3000a closely matches
> al3010 and seems to be part of same sensor lineup. It is not
> prohibited to add new compatibles to existing schema. Schema does not
> take in account way of processing data generated by sensor and this is
> the main difference between al3000a and al3010

Please mention this in your commit message.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]