[PATCH v2 00/10] iio: light: veml6030: fix issues and add support for veml6035

[PATCH v2 00/10] iio: light: veml6030: fix issues and add support for veml6035

[Javier Carrasco]

This series updates the driver for the veml6030 ALS and adds support for
the veml6035, which shares most of its functionality with the former.

The most relevant updates for the veml6030 are the resolution correction
to meet the datasheet update that took place with Rev 1.7, 28-Nov-2023,
a fix to avoid a segmentation fault when reading the
in_illuminance_period_available attribute, and the removal of the
processed value for the WHITE channel, as it uses the ALS resolution.

Vishay does not host the Product Information Notification where the
resolution correction was introduced, but it can still be found
online[1], and the corrected value is the one listed on the latest
version of the datasheet[2] (Rev. 1.7, 28-Nov-2023) and application
note[3] (Rev. 17-Jan-2024).

Link: https://www.farnell.com/datasheets/4379688.pdf [1]
Link: https://www.vishay.com/docs/84366/veml6030.pdf [2]
Link: https://www.vishay.com/docs/84367/designingveml6030.pdf [3]

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Changes in v2:
- Rebase to iio/testing, dropping applied patches [1/7], [4/7].
- Drop [3/7] (applied to iio/fixes-togreg).
- Add patch to use dev_err_probe() in probe error paths.
- Add patch to use read_avail() for available attributes.
- Add patches to use to support a regulator.
- Add patch to ensure that the device is powered off in error paths
  after powering it on.
- Add patch to drop processed values from the WHITE channel.
- Use fsleep() instead of usleep_range() in veml6030_als_pwr_on()
- Link to v1: https://lore.kernel.org/r/20240913-veml6035-v1-0-0b09c0c90418@gmail.com

---
Javier Carrasco (10):
      iio: light: veml6030: fix ALS sensor resolution
      iio: light: veml6030: add set up delay after any power on sequence
      iio: light: veml6030: use dev_err_probe()
      dt-bindings: iio: light: veml6030: add vdd-supply property
      iio: light: veml6030: add support for a regulator
      iio: light: veml6030: use read_avail() for available attributes
      iio: light: veml6030: drop processed info for white channel
      iio: light: veml6030: power off device in probe error paths
      dt-bindings: iio: light: veml6030: add veml6035
      iio: light: veml6030: add support for veml6035

 .../bindings/iio/light/vishay,veml6030.yaml        |  43 +-
 drivers/iio/light/Kconfig                          |   4 +-
 drivers/iio/light/veml6030.c                       | 453 ++++++++++++++++-----
 3 files changed, 391 insertions(+), 109 deletions(-)
---
base-commit: 8bea3878a1511bceadc2fbf284b00bcc5a2ef28d
change-id: 20240903-veml6035-7a91bc088c6f

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH v2 01/10] iio: light: veml6030: fix ALS sensor resolution

[Javier Carrasco]

The driver still uses the sensor resolution provided in the datasheet
until Rev. 1.6, 28-Apr-2022, which was updated with Rev 1.7,
28-Nov-2023. The original ambient light resolution has been updated from
0.0036 lx/ct to 0.0042 lx/ct, which is the value that can be found in
the current device datasheet.

Update the default resolution for IT = 100 ms and GAIN = 1/8 from the
original 4608 mlux/cnt to the current value from the "Resolution and
maximum detection range" table (Application Note 84367, page 5), 5376
mlux/cnt.

Cc: stable@vger.kernel.org
Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/veml6030.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index 4c436c5e0787..a3dfe56b7eec 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -780,7 +780,7 @@ static int veml6030_hw_init(struct iio_dev *indio_dev)
 
 	/* Cache currently active measurement parameters */
 	data->cur_gain = 3;
-	data->cur_resolution = 4608;
+	data->cur_resolution = 5376;
 	data->cur_integration_time = 3;
 
 	return ret;

-- 
2.43.0

Re: [PATCH v2 01/10] iio: light: veml6030: fix ALS sensor resolution

[Jonathan Cameron]

On Mon, 23 Sep 2024 00:17:49 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The driver still uses the sensor resolution provided in the datasheet
> until Rev. 1.6, 28-Apr-2022, which was updated with Rev 1.7,
> 28-Nov-2023. The original ambient light resolution has been updated from
> 0.0036 lx/ct to 0.0042 lx/ct, which is the value that can be found in
> the current device datasheet.
> 
> Update the default resolution for IT = 100 ms and GAIN = 1/8 from the
> original 4608 mlux/cnt to the current value from the "Resolution and
> maximum detection range" table (Application Note 84367, page 5), 5376
> mlux/cnt.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Applied this patch to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> ---
>  drivers/iio/light/veml6030.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index 4c436c5e0787..a3dfe56b7eec 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -780,7 +780,7 @@ static int veml6030_hw_init(struct iio_dev *indio_dev)
>  
>  	/* Cache currently active measurement parameters */
>  	data->cur_gain = 3;
> -	data->cur_resolution = 4608;
> +	data->cur_resolution = 5376;
>  	data->cur_integration_time = 3;
>  
>  	return ret;
> 

[PATCH v2 02/10] iio: light: veml6030: add set up delay after any power on sequence

[Javier Carrasco]

The veml6030 requires a delay of 4 ms after activating the sensor. That
is done correctly during the hw initialization, but it's missing after
resuming.

Move the delay to the power on function to make sure that it is always
observerd. When at it, use fsleep() instead of usleep_range() as such a
narrow range is not required.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/veml6030.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index a3dfe56b7eec..e412a22474e0 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -144,8 +144,17 @@ static const struct attribute_group veml6030_event_attr_group = {
 
 static int veml6030_als_pwr_on(struct veml6030_data *data)
 {
-	return regmap_clear_bits(data->regmap, VEML6030_REG_ALS_CONF,
+	int ret;
+
+	ret =  regmap_clear_bits(data->regmap, VEML6030_REG_ALS_CONF,
 				 VEML6030_ALS_SD);
+	if (ret)
+		return ret;
+
+	/* Wait 4 ms to let processor & oscillator start correctly */
+	fsleep(4000);
+
+	return 0;
 }
 
 static int veml6030_als_shut_down(struct veml6030_data *data)
@@ -767,9 +776,6 @@ static int veml6030_hw_init(struct iio_dev *indio_dev)
 		return ret;
 	}
 
-	/* Wait 4 ms to let processor & oscillator start correctly */
-	usleep_range(4000, 4002);
-
 	/* Clear stale interrupt status bits if any during start */
 	ret = regmap_read(data->regmap, VEML6030_REG_ALS_INT, &val);
 	if (ret < 0) {

-- 
2.43.0

Re: [PATCH v2 02/10] iio: light: veml6030: add set up delay after any power on sequence

[Jonathan Cameron]

On Mon, 23 Sep 2024 00:17:50 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The veml6030 requires a delay of 4 ms after activating the sensor. That
> is done correctly during the hw initialization, but it's missing after
> resuming.
> 
> Move the delay to the power on function to make sure that it is always
> observerd. When at it, use fsleep() instead of usleep_range() as such a
> narrow range is not required.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/iio/light/veml6030.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index a3dfe56b7eec..e412a22474e0 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -144,8 +144,17 @@ static const struct attribute_group veml6030_event_attr_group = {
>  
>  static int veml6030_als_pwr_on(struct veml6030_data *data)
>  {
> -	return regmap_clear_bits(data->regmap, VEML6030_REG_ALS_CONF,
> +	int ret;
> +
> +	ret =  regmap_clear_bits(data->regmap, VEML6030_REG_ALS_CONF,

sneaky double space on the line above. Should just be 1.

>  				 VEML6030_ALS_SD);
Which will require this be indented by 1 less as well and hence
an extra line of apparent change.

> +	if (ret)
> +		return ret;
> +
> +	/* Wait 4 ms to let processor & oscillator start correctly */
> +	fsleep(4000);
> +
> +	return 0;
>  }
>  
>  static int veml6030_als_shut_down(struct veml6030_data *data)
> @@ -767,9 +776,6 @@ static int veml6030_hw_init(struct iio_dev *indio_dev)
>  		return ret;
>  	}
>  
> -	/* Wait 4 ms to let processor & oscillator start correctly */
> -	usleep_range(4000, 4002);
> -
>  	/* Clear stale interrupt status bits if any during start */
>  	ret = regmap_read(data->regmap, VEML6030_REG_ALS_INT, &val);
>  	if (ret < 0) {
> 

[PATCH v2 03/10] iio: light: veml6030: use dev_err_probe()

[Javier Carrasco]

Use the more convenient dev_err_probe() to get rid of the dev_err() +
return sequence in the probe error paths.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/veml6030.c | 72 ++++++++++++++++++--------------------------
 1 file changed, 30 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index e412a22474e0..ccabd4c844e4 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -740,49 +740,39 @@ static int veml6030_hw_init(struct iio_dev *indio_dev)
 	struct i2c_client *client = data->client;
 
 	ret = veml6030_als_shut_down(data);
-	if (ret) {
-		dev_err(&client->dev, "can't shutdown als %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "can't shutdown als\n");
 
 	ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF, 0x1001);
-	if (ret) {
-		dev_err(&client->dev, "can't setup als configs %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "can't setup als configs\n");
 
 	ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_PSM,
 				 VEML6030_PSM | VEML6030_PSM_EN, 0x03);
-	if (ret) {
-		dev_err(&client->dev, "can't setup default PSM %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "can't setup default PSM\n");
 
 	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WH, 0xFFFF);
-	if (ret) {
-		dev_err(&client->dev, "can't setup high threshold %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "can't setup high threshold\n");
 
 	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WL, 0x0000);
-	if (ret) {
-		dev_err(&client->dev, "can't setup low threshold %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "can't setup low threshold\n");
 
 	ret = veml6030_als_pwr_on(data);
-	if (ret) {
-		dev_err(&client->dev, "can't poweron als %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "can't poweron als\n");
 
 	/* Clear stale interrupt status bits if any during start */
 	ret = regmap_read(data->regmap, VEML6030_REG_ALS_INT, &val);
-	if (ret < 0) {
-		dev_err(&client->dev,
-			"can't clear als interrupt status %d\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+				     "can't clear als interrupt status\n");
 
 	/* Cache currently active measurement parameters */
 	data->cur_gain = 3;
@@ -799,16 +789,14 @@ static int veml6030_probe(struct i2c_client *client)
 	struct iio_dev *indio_dev;
 	struct regmap *regmap;
 
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
-		dev_err(&client->dev, "i2c adapter doesn't support plain i2c\n");
-		return -EOPNOTSUPP;
-	}
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return dev_err_probe(&client->dev, -EOPNOTSUPP,
+				     "i2c adapter doesn't support plain i2c\n");
 
 	regmap = devm_regmap_init_i2c(client, &veml6030_regmap_config);
-	if (IS_ERR(regmap)) {
-		dev_err(&client->dev, "can't setup regmap\n");
-		return PTR_ERR(regmap);
-	}
+	if (IS_ERR(regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(regmap),
+				     "can't setup regmap\n");
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
@@ -829,11 +817,11 @@ static int veml6030_probe(struct i2c_client *client)
 						NULL, veml6030_event_handler,
 						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
 						"veml6030", indio_dev);
-		if (ret < 0) {
-			dev_err(&client->dev,
-					"irq %d request failed\n", client->irq);
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(&client->dev, ret,
+					     "irq %d request failed\n",
+					     client->irq);
+
 		indio_dev->info = &veml6030_info;
 	} else {
 		indio_dev->info = &veml6030_info_no_irq;

-- 
2.43.0

[PATCH v2 04/10] dt-bindings: iio: light: veml6030: add vdd-supply property

[Javier Carrasco]

Add vdd-supply to account for the sensor's power source.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml b/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
index 7f4995557570..2583b61c8357 100644
--- a/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
+++ b/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
@@ -41,6 +41,8 @@ properties:
       interrupt client node bindings.
     maxItems: 1
 
+    vdd-supply: true
+
 required:
   - compatible
   - reg
@@ -59,6 +61,7 @@ examples:
                 compatible = "vishay,veml6030";
                 reg = <0x10>;
                 interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
+                vdd-supply = <&vdd>;
         };
     };
 ...

-- 
2.43.0

Re: [PATCH v2 04/10] dt-bindings: iio: light: veml6030: add vdd-supply property

[Krzysztof Kozlowski]

On Mon, Sep 23, 2024 at 12:17:52AM +0200, Javier Carrasco wrote:
> Add vdd-supply to account for the sensor's power source.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v2 04/10] dt-bindings: iio: light: veml6030: add vdd-supply property

[Javier Carrasco]

On 24/09/2024 12:06, Krzysztof Kozlowski wrote:
> On Mon, Sep 23, 2024 at 12:17:52AM +0200, Javier Carrasco wrote:
>> Add vdd-supply to account for the sensor's power source.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml | 3 +++
>>  1 file changed, 3 insertions(+)
> 
> Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Best regards,
> Krzysztof
> 

I just realized that the indentation of vdd-supply is not right in the
version I sent, and dt_binding_check rightfully complains about that. I
will fix it for v3.

Best regards,
Javier Carrasco

[PATCH v2 05/10] iio: light: veml6030: add support for a regulator

[Javier Carrasco]

Use the device managed function from the regulator API to get and enable
a regulator powering the device.

Use "vdd" as the ID to account for the provided name in the datasheet.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/veml6030.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index ccabd4c844e4..89c98bfc5191 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -14,6 +14,7 @@
 #include <linux/regmap.h>
 #include <linux/interrupt.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
@@ -807,6 +808,11 @@ static int veml6030_probe(struct i2c_client *client)
 	data->client = client;
 	data->regmap = regmap;
 
+	ret = devm_regulator_get_enable(&client->dev, "vdd");
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to enable regulator\n");
+
 	indio_dev->name = "veml6030";
 	indio_dev->channels = veml6030_channels;
 	indio_dev->num_channels = ARRAY_SIZE(veml6030_channels);

-- 
2.43.0

[PATCH v2 06/10] iio: light: veml6030: use read_avail() for available attributes

[Javier Carrasco]

Drop custom attributes by using the standard read_avail() callback to
read scale and integration time. When at it, define these attributes as
available by all channels, as they affect the values of both the ALS and
the WHITE channel.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/veml6030.c | 64 +++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index 89c98bfc5191..a3190fab3add 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -58,25 +58,24 @@ struct veml6030_data {
 	int cur_integration_time;
 };
 
-/* Integration time available in seconds */
-static IIO_CONST_ATTR(in_illuminance_integration_time_available,
-				"0.025 0.05 0.1 0.2 0.4 0.8");
+static const int veml6030_it_times[][2] = {
+	{0, 25000},
+	{0, 50000},
+	{0, 100000},
+	{0, 200000},
+	{0, 400000},
+	{0, 800000},
+};
 
 /*
  * Scale is 1/gain. Value 0.125 is ALS gain x (1/8), 0.25 is
  * ALS gain x (1/4), 1.0 = ALS gain x 1 and 2.0 is ALS gain x 2.
  */
-static IIO_CONST_ATTR(in_illuminance_scale_available,
-				"0.125 0.25 1.0 2.0");
-
-static struct attribute *veml6030_attributes[] = {
-	&iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
-	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
-	NULL
-};
-
-static const struct attribute_group veml6030_attr_group = {
-	.attrs = veml6030_attributes,
+static const int veml6030_scale_vals[][2] = {
+	{0, 125000},
+	{0, 250000},
+	{1, 0},
+	{2, 0},
 };
 
 /*
@@ -197,9 +196,11 @@ static const struct iio_chan_spec veml6030_channels[] = {
 		.type = IIO_LIGHT,
 		.channel = CH_ALS,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-				BIT(IIO_CHAN_INFO_PROCESSED) |
-				BIT(IIO_CHAN_INFO_INT_TIME) |
-				BIT(IIO_CHAN_INFO_SCALE),
+				BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
+					       BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+						     BIT(IIO_CHAN_INFO_SCALE),
 		.event_spec = veml6030_event_spec,
 		.num_event_specs = ARRAY_SIZE(veml6030_event_spec),
 	},
@@ -210,6 +211,10 @@ static const struct iio_chan_spec veml6030_channels[] = {
 		.channel2 = IIO_MOD_LIGHT_BOTH,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 				BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
+					       BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+						     BIT(IIO_CHAN_INFO_SCALE),
 	},
 };
 
@@ -567,6 +572,27 @@ static int veml6030_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int veml6030_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*vals = (int *)&veml6030_it_times;
+		*length = 2 * ARRAY_SIZE(veml6030_it_times);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)&veml6030_scale_vals;
+		*length = 2 * ARRAY_SIZE(veml6030_scale_vals);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int veml6030_write_raw(struct iio_dev *indio_dev,
 				struct iio_chan_spec const *chan,
 				int val, int val2, long mask)
@@ -684,19 +710,19 @@ static int veml6030_write_interrupt_config(struct iio_dev *indio_dev,
 
 static const struct iio_info veml6030_info = {
 	.read_raw  = veml6030_read_raw,
+	.read_avail  = veml6030_read_avail,
 	.write_raw = veml6030_write_raw,
 	.read_event_value = veml6030_read_event_val,
 	.write_event_value	= veml6030_write_event_val,
 	.read_event_config = veml6030_read_interrupt_config,
 	.write_event_config	= veml6030_write_interrupt_config,
-	.attrs = &veml6030_attr_group,
 	.event_attrs = &veml6030_event_attr_group,
 };
 
 static const struct iio_info veml6030_info_no_irq = {
 	.read_raw  = veml6030_read_raw,
+	.read_avail  = veml6030_read_avail,
 	.write_raw = veml6030_write_raw,
-	.attrs = &veml6030_attr_group,
 };
 
 static irqreturn_t veml6030_event_handler(int irq, void *private)

-- 
2.43.0

Re: [PATCH v2 06/10] iio: light: veml6030: use read_avail() for available attributes

[Jonathan Cameron]

On Mon, 23 Sep 2024 00:17:54 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> Drop custom attributes by using the standard read_avail() callback to
> read scale and integration time. When at it, define these attributes as
> available by all channels, as they affect the values of both the ALS and
> the WHITE channel.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Hi Javier

Some comments inline

Thanks,

Jonathan

> ---
>  drivers/iio/light/veml6030.c | 64 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index 89c98bfc5191..a3190fab3add 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -58,25 +58,24 @@ struct veml6030_data {
>  	int cur_integration_time;
>  };
>  
> -/* Integration time available in seconds */
> -static IIO_CONST_ATTR(in_illuminance_integration_time_available,
> -				"0.025 0.05 0.1 0.2 0.4 0.8");
> +static const int veml6030_it_times[][2] = {
> +	{0, 25000},
Really minor but I'm trying to get IIO standardized on formatting for this
sort of array and I'd like not to introduce more instances of it
done without the extra spaces as it will just give more to clean up
at some point.

	{ 0, 25000 },
etc please.
> +	{0, 50000},
> +	{0, 100000},
> +	{0, 200000},
> +	{0, 400000},
> +	{0, 800000},
> +};
>  
>  /*
>   * Scale is 1/gain. Value 0.125 is ALS gain x (1/8), 0.25 is
>   * ALS gain x (1/4), 1.0 = ALS gain x 1 and 2.0 is ALS gain x 2.
>   */
> -static IIO_CONST_ATTR(in_illuminance_scale_available,
> -				"0.125 0.25 1.0 2.0");
> -
> -static struct attribute *veml6030_attributes[] = {
> -	&iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
> -	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group veml6030_attr_group = {
> -	.attrs = veml6030_attributes,
> +static const int veml6030_scale_vals[][2] = {
> +	{0, 125000},
> +	{0, 250000},
> +	{1, 0},
> +	{2, 0},

As above, add some spaces for minor readability improvement.

>  };
>  
>  /*
> @@ -197,9 +196,11 @@ static const struct iio_chan_spec veml6030_channels[] = {
>  		.type = IIO_LIGHT,
>  		.channel = CH_ALS,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -				BIT(IIO_CHAN_INFO_PROCESSED) |
> -				BIT(IIO_CHAN_INFO_INT_TIME) |
> -				BIT(IIO_CHAN_INFO_SCALE),
> +				BIT(IIO_CHAN_INFO_PROCESSED),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
> +					       BIT(IIO_CHAN_INFO_SCALE),
This bit is an ABI change and technically old code wasn't a bug, so
we don't really have a good enough reason to change it.  So Please
leave these as separate.

> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> +						     BIT(IIO_CHAN_INFO_SCALE),
That doesn't stop us sharing the available as that always was shared
in the attribute naming above.

>  		.event_spec = veml6030_event_spec,
>  		.num_event_specs = ARRAY_SIZE(veml6030_event_spec),
>  	},
> @@ -210,6 +211,10 @@ static const struct iio_chan_spec veml6030_channels[] = {
>  		.channel2 = IIO_MOD_LIGHT_BOTH,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  				BIT(IIO_CHAN_INFO_PROCESSED),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
> +					       BIT(IIO_CHAN_INFO_SCALE),
This confuses me.  Is it fixing a bug by effectively adding attributes for this
channel that were previously missing? If so we'll have to go with searpte
even though they are shared to avoid breaking the ABI for other channel.

> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> +						     BIT(IIO_CHAN_INFO_SCALE),
>  	},
>  };
>  

Re: [PATCH v2 06/10] iio: light: veml6030: use read_avail() for available attributes

[Javier Carrasco]

On 28/09/2024 18:19, Jonathan Cameron wrote:
> On Mon, 23 Sep 2024 00:17:54 +0200
> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> 
>> Drop custom attributes by using the standard read_avail() callback to
>> read scale and integration time. When at it, define these attributes as
>> available by all channels, as they affect the values of both the ALS and
>> the WHITE channel.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> Hi Javier
> 
> Some comments inline
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>  drivers/iio/light/veml6030.c | 64 +++++++++++++++++++++++++++++++-------------
>>  1 file changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
>> index 89c98bfc5191..a3190fab3add 100644
>> --- a/drivers/iio/light/veml6030.c
>> +++ b/drivers/iio/light/veml6030.c
>> @@ -58,25 +58,24 @@ struct veml6030_data {
>>  	int cur_integration_time;
>>  };
>>  
>> -/* Integration time available in seconds */
>> -static IIO_CONST_ATTR(in_illuminance_integration_time_available,
>> -				"0.025 0.05 0.1 0.2 0.4 0.8");
>> +static const int veml6030_it_times[][2] = {
>> +	{0, 25000},
> Really minor but I'm trying to get IIO standardized on formatting for this
> sort of array and I'd like not to introduce more instances of it
> done without the extra spaces as it will just give more to clean up
> at some point.
> 
> 	{ 0, 25000 },
> etc please.
>> +	{0, 50000},
>> +	{0, 100000},
>> +	{0, 200000},
>> +	{0, 400000},
>> +	{0, 800000},
>> +};
>>  
>>  /*
>>   * Scale is 1/gain. Value 0.125 is ALS gain x (1/8), 0.25 is
>>   * ALS gain x (1/4), 1.0 = ALS gain x 1 and 2.0 is ALS gain x 2.
>>   */
>> -static IIO_CONST_ATTR(in_illuminance_scale_available,
>> -				"0.125 0.25 1.0 2.0");
>> -
>> -static struct attribute *veml6030_attributes[] = {
>> -	&iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
>> -	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group veml6030_attr_group = {
>> -	.attrs = veml6030_attributes,
>> +static const int veml6030_scale_vals[][2] = {
>> +	{0, 125000},
>> +	{0, 250000},
>> +	{1, 0},
>> +	{2, 0},
> 
> As above, add some spaces for minor readability improvement.
> 
>>  };
>>  
>>  /*
>> @@ -197,9 +196,11 @@ static const struct iio_chan_spec veml6030_channels[] = {
>>  		.type = IIO_LIGHT,
>>  		.channel = CH_ALS,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> -				BIT(IIO_CHAN_INFO_PROCESSED) |
>> -				BIT(IIO_CHAN_INFO_INT_TIME) |
>> -				BIT(IIO_CHAN_INFO_SCALE),
>> +				BIT(IIO_CHAN_INFO_PROCESSED),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +					       BIT(IIO_CHAN_INFO_SCALE),
> This bit is an ABI change and technically old code wasn't a bug, so
> we don't really have a good enough reason to change it.  So Please
> leave these as separate.
> 
>> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +						     BIT(IIO_CHAN_INFO_SCALE),
> That doesn't stop us sharing the available as that always was shared
> in the attribute naming above.
> 
>>  		.event_spec = veml6030_event_spec,
>>  		.num_event_specs = ARRAY_SIZE(veml6030_event_spec),
>>  	},
>> @@ -210,6 +211,10 @@ static const struct iio_chan_spec veml6030_channels[] = {
>>  		.channel2 = IIO_MOD_LIGHT_BOTH,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  				BIT(IIO_CHAN_INFO_PROCESSED),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +					       BIT(IIO_CHAN_INFO_SCALE),
> This confuses me.  Is it fixing a bug by effectively adding attributes for this
> channel that were previously missing? If so we'll have to go with searpte
> even though they are shared to avoid breaking the ABI for other channel.
> 
>> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +						     BIT(IIO_CHAN_INFO_SCALE),
>>  	},
>>  };
>>  
> 
> 

This confused me as well, because even though the attributes where
defined as separate for the ALS channel, modifying their values affected
the values from the WHITE channel.

The integration time and the scale affect both channels, and therefore I
thought they should be shared attributes. But in that case, and to avoid
breaking the ABI for the other channel, I will make them separate even
though writing to one of them will change the value of the other as well.

Thanks and best regards,
Javier Carrasco

Re: [PATCH v2 06/10] iio: light: veml6030: use read_avail() for available attributes

[Jonathan Cameron]

On Sun, 29 Sep 2024 20:45:40 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> On 28/09/2024 18:19, Jonathan Cameron wrote:
> > On Mon, 23 Sep 2024 00:17:54 +0200
> > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> >   
> >> Drop custom attributes by using the standard read_avail() callback to
> >> read scale and integration time. When at it, define these attributes as
> >> available by all channels, as they affect the values of both the ALS and
> >> the WHITE channel.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>  
> > Hi Javier
> > 
> > Some comments inline
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> >> ---
> >>  drivers/iio/light/veml6030.c | 64 +++++++++++++++++++++++++++++++-------------
> >>  1 file changed, 45 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> >> index 89c98bfc5191..a3190fab3add 100644
> >> --- a/drivers/iio/light/veml6030.c
> >> +++ b/drivers/iio/light/veml6030.c
> >> @@ -58,25 +58,24 @@ struct veml6030_data {
> >>  	int cur_integration_time;
> >>  };
> >>  
> >> -/* Integration time available in seconds */
> >> -static IIO_CONST_ATTR(in_illuminance_integration_time_available,
> >> -				"0.025 0.05 0.1 0.2 0.4 0.8");
> >> +static const int veml6030_it_times[][2] = {
> >> +	{0, 25000},  
> > Really minor but I'm trying to get IIO standardized on formatting for this
> > sort of array and I'd like not to introduce more instances of it
> > done without the extra spaces as it will just give more to clean up
> > at some point.
> > 
> > 	{ 0, 25000 },
> > etc please.  
> >> +	{0, 50000},
> >> +	{0, 100000},
> >> +	{0, 200000},
> >> +	{0, 400000},
> >> +	{0, 800000},
> >> +};
> >>  
> >>  /*
> >>   * Scale is 1/gain. Value 0.125 is ALS gain x (1/8), 0.25 is
> >>   * ALS gain x (1/4), 1.0 = ALS gain x 1 and 2.0 is ALS gain x 2.
> >>   */
> >> -static IIO_CONST_ATTR(in_illuminance_scale_available,
> >> -				"0.125 0.25 1.0 2.0");
> >> -
> >> -static struct attribute *veml6030_attributes[] = {
> >> -	&iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
> >> -	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
> >> -	NULL
> >> -};
> >> -
> >> -static const struct attribute_group veml6030_attr_group = {
> >> -	.attrs = veml6030_attributes,
> >> +static const int veml6030_scale_vals[][2] = {
> >> +	{0, 125000},
> >> +	{0, 250000},
> >> +	{1, 0},
> >> +	{2, 0},  
> > 
> > As above, add some spaces for minor readability improvement.
> >   
> >>  };
> >>  
> >>  /*
> >> @@ -197,9 +196,11 @@ static const struct iio_chan_spec veml6030_channels[] = {
> >>  		.type = IIO_LIGHT,
> >>  		.channel = CH_ALS,
> >>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> -				BIT(IIO_CHAN_INFO_PROCESSED) |
> >> -				BIT(IIO_CHAN_INFO_INT_TIME) |
> >> -				BIT(IIO_CHAN_INFO_SCALE),
> >> +				BIT(IIO_CHAN_INFO_PROCESSED),
> >> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
> >> +					       BIT(IIO_CHAN_INFO_SCALE),  
> > This bit is an ABI change and technically old code wasn't a bug, so
> > we don't really have a good enough reason to change it.  So Please
> > leave these as separate.
> >   
> >> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> >> +						     BIT(IIO_CHAN_INFO_SCALE),  
> > That doesn't stop us sharing the available as that always was shared
> > in the attribute naming above.
> >   
> >>  		.event_spec = veml6030_event_spec,
> >>  		.num_event_specs = ARRAY_SIZE(veml6030_event_spec),
> >>  	},
> >> @@ -210,6 +211,10 @@ static const struct iio_chan_spec veml6030_channels[] = {
> >>  		.channel2 = IIO_MOD_LIGHT_BOTH,
> >>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >>  				BIT(IIO_CHAN_INFO_PROCESSED),
> >> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
> >> +					       BIT(IIO_CHAN_INFO_SCALE),  
> > This confuses me.  Is it fixing a bug by effectively adding attributes for this
> > channel that were previously missing? If so we'll have to go with searpte
> > even though they are shared to avoid breaking the ABI for other channel.
> >   
> >> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> >> +						     BIT(IIO_CHAN_INFO_SCALE),
> >>  	},
> >>  };
> >>    
> > 
> >   
> 
> This confused me as well, because even though the attributes where
> defined as separate for the ALS channel, modifying their values affected
> the values from the WHITE channel.
> 
> The integration time and the scale affect both channels, and therefore I
> thought they should be shared attributes. But in that case, and to avoid
> breaking the ABI for the other channel, I will make them separate even
> though writing to one of them will change the value of the other as well.
Yes.  The ABI fortunately always allows that sort of cross effect as we
have devices where similar controls affect some but not all channels on
a device, or different types of channel, but still not all channels.
Our fairly simple hierarchical sharing scheme never describes those.

Here we will have to take advantage of that being allowed even though
the driver should have had these shared in the first place :(

Jonathan

> 
> Thanks and best regards,
> Javier Carrasco
> 

[PATCH v2 07/10] iio: light: veml6030: drop processed info for white channel

[Javier Carrasco]

The resolution of the WHITE channel is not provided by the manufacturer,
neither in the datasheet nor in the application note (even their
proprietary application only processes the ALS channel, giving raw
values for WHITE).

The current implementation assumes that both resolutions are identical,
which is extremely unlikely, especially for photodiodes with different
spectral responses.

Drop the processed information as it is meaningless.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/veml6030.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index a3190fab3add..861bdf2edd4d 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -209,8 +209,7 @@ static const struct iio_chan_spec veml6030_channels[] = {
 		.channel = CH_WHITE,
 		.modified = 1,
 		.channel2 = IIO_MOD_LIGHT_BOTH,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-				BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
 					       BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
@@ -549,11 +548,6 @@ static int veml6030_read_raw(struct iio_dev *indio_dev,
 				dev_err(dev, "can't read white data %d\n", ret);
 				return ret;
 			}
-			if (mask == IIO_CHAN_INFO_PROCESSED) {
-				*val = (reg * data->cur_resolution) / 10000;
-				*val2 = (reg * data->cur_resolution) % 10000;
-				return IIO_VAL_INT_PLUS_MICRO;
-			}
 			*val = reg;
 			return IIO_VAL_INT;
 		default:

-- 
2.43.0

Re: [PATCH v2 07/10] iio: light: veml6030: drop processed info for white channel

[Jonathan Cameron]

On Mon, 23 Sep 2024 00:17:55 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The resolution of the WHITE channel is not provided by the manufacturer,
> neither in the datasheet nor in the application note (even their
> proprietary application only processes the ALS channel, giving raw
> values for WHITE).
> 
> The current implementation assumes that both resolutions are identical,
> which is extremely unlikely, especially for photodiodes with different
> spectral responses.
> 
> Drop the processed information as it is meaningless.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Just to note, this one is fine as it is fixing an ABI bug
so is a valid reason to change the ABI.

In theory though we should pull it out as a fix to add to stable
as well, but it's low risk that anyone is using the 'false'
channel so I don't think we care about backports for this one.

Thanks,

Jonathan

> ---
>  drivers/iio/light/veml6030.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index a3190fab3add..861bdf2edd4d 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -209,8 +209,7 @@ static const struct iio_chan_spec veml6030_channels[] = {
>  		.channel = CH_WHITE,
>  		.modified = 1,
>  		.channel2 = IIO_MOD_LIGHT_BOTH,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -				BIT(IIO_CHAN_INFO_PROCESSED),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
>  					       BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> @@ -549,11 +548,6 @@ static int veml6030_read_raw(struct iio_dev *indio_dev,
>  				dev_err(dev, "can't read white data %d\n", ret);
>  				return ret;
>  			}
> -			if (mask == IIO_CHAN_INFO_PROCESSED) {
> -				*val = (reg * data->cur_resolution) / 10000;
> -				*val2 = (reg * data->cur_resolution) % 10000;
> -				return IIO_VAL_INT_PLUS_MICRO;
> -			}
>  			*val = reg;
>  			return IIO_VAL_INT;
>  		default:
> 

[PATCH v2 08/10] iio: light: veml6030: power off device in probe error paths

[Javier Carrasco]

Move devm_add_action_or_reset() with a device shut down action above the
hardware initialization function to ensure that any error path after
powering on the device leads to a power off.

The power off action is carried out by setting the VEML6030_ALS_SD bit
of the VEML6030_REG_ALS_CONF, which is harmless in error paths were the
device is already off. On the other hand, making use of the registered
action in all error paths makes them more homogeneous by avoiding
special action depending on the current power state of the device.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/veml6030.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index 861bdf2edd4d..19c69bfad8cb 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -853,12 +853,12 @@ static int veml6030_probe(struct i2c_client *client)
 		indio_dev->info = &veml6030_info_no_irq;
 	}
 
-	ret = veml6030_hw_init(indio_dev);
+	ret = devm_add_action_or_reset(&client->dev,
+				       veml6030_als_shut_down_action, data);
 	if (ret < 0)
 		return ret;
 
-	ret = devm_add_action_or_reset(&client->dev,
-					veml6030_als_shut_down_action, data);
+	ret = veml6030_hw_init(indio_dev);
 	if (ret < 0)
 		return ret;
 

-- 
2.43.0

Re: [PATCH v2 08/10] iio: light: veml6030: power off device in probe error paths

[Jonathan Cameron]

On Mon, 23 Sep 2024 00:17:56 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> Move devm_add_action_or_reset() with a device shut down action above the
> hardware initialization function to ensure that any error path after
> powering on the device leads to a power off.
> 
> The power off action is carried out by setting the VEML6030_ALS_SD bit
> of the VEML6030_REG_ALS_CONF, which is harmless in error paths were the
> device is already off. On the other hand, making use of the registered
> action in all error paths makes them more homogeneous by avoiding
> special action depending on the current power state of the device.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
As per my very late reply, I'd move the devm_add_action_or_reset()
into the hw_init function so it's closely coupled with the thing
it is undoing rather than with a function that does lots of other
things.

You'll want to pass dev into the init function though so it is
easy to see what device the devm_ call is against.
Don't use the parent of the IIO dev as that's an implementation
detail!

Jonathan
> ---
>  drivers/iio/light/veml6030.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index 861bdf2edd4d..19c69bfad8cb 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -853,12 +853,12 @@ static int veml6030_probe(struct i2c_client *client)
>  		indio_dev->info = &veml6030_info_no_irq;
>  	}
>  
> -	ret = veml6030_hw_init(indio_dev);
> +	ret = devm_add_action_or_reset(&client->dev,
> +				       veml6030_als_shut_down_action, data);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = devm_add_action_or_reset(&client->dev,
> -					veml6030_als_shut_down_action, data);
> +	ret = veml6030_hw_init(indio_dev);
>  	if (ret < 0)
>  		return ret;
>  
> 

[PATCH v2 09/10] dt-bindings: iio: light: veml6030: add veml6035

[Javier Carrasco]

The veml6035 is a similar ambient light sensor to the veml6030, and
from the bindings point of view, it shares the same properties. Its
only difference in that respect is a different I2C address.

Estend the existing bindings to support the veml6035 ALS.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 .../bindings/iio/light/vishay,veml6030.yaml        | 40 +++++++++++++++++-----
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml b/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
index 2583b61c8357..69ca10cab09a 100644
--- a/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
+++ b/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
@@ -4,14 +4,14 @@
 $id: http://devicetree.org/schemas/iio/light/vishay,veml6030.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: VEML6030 Ambient Light Sensor (ALS)
+title: VEML6030 and VEML6035 Ambient Light Sensors (ALS)
 
 maintainers:
   - Rishi Gupta <gupt21@gmail.com>
 
 description: |
-  Bindings for the ambient light sensor veml6030 from Vishay
-  Semiconductors over an i2c interface.
+  Bindings for the ambient light sensors veml6030 and veml6035 from
+  Vishay Semiconductors over an i2c interface.
 
   Irrespective of whether interrupt is used or not, application
   can get the ALS and White channel reading from IIO raw interface.
@@ -19,20 +19,18 @@ description: |
   If the interrupts are used, application will receive an IIO event
   whenever configured threshold is crossed.
 
-  Specifications about the sensor can be found at:
+  Specifications about the sensors can be found at:
     https://www.vishay.com/docs/84366/veml6030.pdf
+    https://www.vishay.com/docs/84889/veml6035.pdf
 
 properties:
   compatible:
     enum:
       - vishay,veml6030
+      - vishay,veml6035
 
   reg:
-    description:
-      I2C address of the device.
-    enum:
-      - 0x10 # ADDR pin pulled down
-      - 0x48 # ADDR pin pulled up
+    maxItems: 1
 
   interrupts:
     description:
@@ -47,6 +45,30 @@ required:
   - compatible
   - reg
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            - vishay,veml6030
+    then:
+      properties:
+        reg:
+          enum:
+            - 0x10  # ADDR pin pulled down
+            - 0x48  # ADDR pin pulled up
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - vishay,veml6035
+    then:
+      properties:
+        reg:
+          enum:
+            - 0x29
+
 additionalProperties: false
 
 examples:

-- 
2.43.0

[PATCH v2 10/10] iio: light: veml6030: add support for veml6035

[Javier Carrasco]

The veml6035 is an ALS that shares most of its functionality with the
veml6030, which allows for some code recycling.

Some chip-specific properties differ and dedicated functions to get and
set the sensor gain as well as its initialization are required.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/Kconfig    |   4 +-
 drivers/iio/light/veml6030.c | 291 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 266 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 515ff46b5b82..171ccaecf5b6 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -669,12 +669,12 @@ config VCNL4035
 	  module will be called vcnl4035.
 
 config VEML6030
-	tristate "VEML6030 ambient light sensor"
+	tristate "VEML6030 and VEML6035 ambient light sensors"
 	select REGMAP_I2C
 	depends on I2C
 	help
 	  Say Y here if you want to build a driver for the Vishay VEML6030
-	  ambient light sensor (ALS).
+	  and VEML6035 ambient light sensors (ALS).
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called veml6030.
diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index 19c69bfad8cb..d5c09c148d22 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -1,13 +1,19 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * VEML6030 Ambient Light Sensor
+ * VEML6030 and VMEL6035 Ambient Light Sensors
  *
  * Copyright (c) 2019, Rishi Gupta <gupt21@gmail.com>
  *
+ * VEML6030:
  * Datasheet: https://www.vishay.com/docs/84366/veml6030.pdf
  * Appnote-84367: https://www.vishay.com/docs/84367/designingveml6030.pdf
+ *
+ * VEML6035:
+ * Datasheet: https://www.vishay.com/docs/84889/veml6035.pdf
+ * Appnote-84944: https://www.vishay.com/docs/84944/designingveml6035.pdf
  */
 
+#include <linux/bitfield.h>
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/err.h>
@@ -39,16 +45,34 @@
 #define VEML6030_ALS_INT_EN   BIT(1)
 #define VEML6030_ALS_SD       BIT(0)
 
+#define VEML6035_GAIN_M       GENMASK(12, 10)
+#define VEML6035_GAIN         BIT(10)
+#define VEML6035_DG           BIT(11)
+#define VEML6035_SENS         BIT(12)
+#define VEML6035_INT_CHAN     BIT(3)
+#define VEML6035_CHAN_EN      BIT(2)
+
+struct veml603x_chip {
+	const char *name;
+	const int(*scale_vals)[][2];
+	const int num_scale_vals;
+	const struct iio_info *info;
+	const struct iio_info *info_no_irq;
+	int (*hw_init)(struct iio_dev *indio_dev);
+	int (*set_als_gain)(struct iio_dev *indio_dev, int val, int val2);
+	int (*get_als_gain)(struct iio_dev *indio_dev, int *val, int *val2);
+};
+
 /*
  * The resolution depends on both gain and integration time. The
  * cur_resolution stores one of the resolution mentioned in the
  * table during startup and gets updated whenever integration time
  * or gain is changed.
  *
- * Table 'resolution and maximum detection range' in appnote 84367
+ * Table 'resolution and maximum detection range' in the appnotes
  * is visualized as a 2D array. The cur_gain stores index of gain
- * in this table (0-3) while the cur_integration_time holds index
- * of integration time (0-5).
+ * in this table (0-3 for VEML6030, 0-5 for VEML6035) while the
+ * cur_integration_time holds index of integration time (0-5).
  */
 struct veml6030_data {
 	struct i2c_client *client;
@@ -56,6 +80,7 @@ struct veml6030_data {
 	int cur_resolution;
 	int cur_gain;
 	int cur_integration_time;
+	const struct veml603x_chip *chip;
 };
 
 static const int veml6030_it_times[][2] = {
@@ -69,7 +94,8 @@ static const int veml6030_it_times[][2] = {
 
 /*
  * Scale is 1/gain. Value 0.125 is ALS gain x (1/8), 0.25 is
- * ALS gain x (1/4), 1.0 = ALS gain x 1 and 2.0 is ALS gain x 2.
+ * ALS gain x (1/4), 0.5 is ALS gain x (1/2), 1.0 is ALS gain x 1,
+ * 2.0 is ALS gain x2, and 4.0 is ALS gain x 4.
  */
 static const int veml6030_scale_vals[][2] = {
 	{0, 125000},
@@ -78,6 +104,15 @@ static const int veml6030_scale_vals[][2] = {
 	{2, 0},
 };
 
+static const int veml6035_scale_vals[][2] = {
+	{0, 125000},
+	{0, 250000},
+	{0, 500000},
+	{1, 0},
+	{2, 0},
+	{4, 0},
+};
+
 /*
  * Persistence = 1/2/4/8 x integration time
  * Minimum time for which light readings must stay above configured
@@ -386,6 +421,21 @@ static int veml6030_write_persistence(struct iio_dev *indio_dev,
 	return ret;
 }
 
+/*
+ * Cache currently set gain & update resolution. For every
+ * increase in the gain to next level, resolution is halved
+ * and vice-versa.
+ */
+static void veml6030_update_gain_res(struct veml6030_data *data, int gain_idx)
+{
+	if (data->cur_gain < gain_idx)
+		data->cur_resolution <<= gain_idx - data->cur_gain;
+	else if (data->cur_gain > gain_idx)
+		data->cur_resolution >>= data->cur_gain - gain_idx;
+
+	data->cur_gain = gain_idx;
+}
+
 static int veml6030_set_als_gain(struct iio_dev *indio_dev,
 						int val, int val2)
 {
@@ -416,19 +466,49 @@ static int veml6030_set_als_gain(struct iio_dev *indio_dev,
 		return ret;
 	}
 
-	/*
-	 * Cache currently set gain & update resolution. For every
-	 * increase in the gain to next level, resolution is halved
-	 * and vice-versa.
-	 */
-	if (data->cur_gain < gain_idx)
-		data->cur_resolution <<= gain_idx - data->cur_gain;
-	else if (data->cur_gain > gain_idx)
-		data->cur_resolution >>= data->cur_gain - gain_idx;
+	veml6030_update_gain_res(data, gain_idx);
 
-	data->cur_gain = gain_idx;
+	return 0;
+}
 
-	return ret;
+static int veml6035_set_als_gain(struct iio_dev *indio_dev, int val, int val2)
+{
+	int ret, new_gain, gain_idx;
+	struct veml6030_data *data = iio_priv(indio_dev);
+
+	if (val == 0 && val2 == 125000) {
+		new_gain = VEML6035_SENS;
+		gain_idx = 5;
+	} else if (val == 0 && val2 == 250000) {
+		new_gain = VEML6035_SENS | VEML6035_GAIN;
+		gain_idx = 4;
+	} else if (val == 0 && val2 == 500000) {
+		new_gain = VEML6035_SENS | VEML6035_GAIN |
+			VEML6035_DG;
+		gain_idx = 3;
+	} else if (val == 1 && val2 == 0) {
+		new_gain = 0x0000;
+		gain_idx = 2;
+	} else if (val == 2 && val2 == 0) {
+		new_gain = VEML6035_GAIN;
+		gain_idx = 1;
+	} else if (val == 4 && val2 == 0) {
+		new_gain = VEML6035_GAIN | VEML6035_DG;
+		gain_idx = 0;
+	} else {
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_CONF,
+				 VEML6035_GAIN_M, new_gain);
+	if (ret) {
+		dev_err(&data->client->dev, "can't set als gain %d\n", ret);
+		return ret;
+	}
+
+	veml6030_update_gain_res(data, gain_idx);
+
+	return 0;
 }
 
 static int veml6030_get_als_gain(struct iio_dev *indio_dev,
@@ -468,6 +548,52 @@ static int veml6030_get_als_gain(struct iio_dev *indio_dev,
 	return IIO_VAL_INT_PLUS_MICRO;
 }
 
+static int veml6035_get_als_gain(struct iio_dev *indio_dev, int *val, int *val2)
+{
+	int ret, reg;
+	struct veml6030_data *data = iio_priv(indio_dev);
+
+	ret = regmap_read(data->regmap, VEML6030_REG_ALS_CONF, &reg);
+	if (ret) {
+		dev_err(&data->client->dev,
+			"can't read als conf register %d\n", ret);
+		return ret;
+	}
+
+	switch (FIELD_GET(VEML6035_GAIN_M, reg)) {
+	case 0:
+		*val = 1;
+		*val2 = 0;
+		break;
+	case 1:
+	case 2:
+		*val = 2;
+		*val2 = 0;
+		break;
+	case 3:
+		*val = 4;
+		*val2 = 0;
+		break;
+	case 4:
+		*val = 0;
+		*val2 = 125000;
+		break;
+	case 5:
+	case 6:
+		*val = 0;
+		*val2 = 250000;
+		break;
+	case 7:
+		*val = 0;
+		*val2 = 500000;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
 static int veml6030_read_thresh(struct iio_dev *indio_dev,
 						int *val, int *val2, int dir)
 {
@@ -559,7 +685,7 @@ static int veml6030_read_raw(struct iio_dev *indio_dev,
 		return -EINVAL;
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type == IIO_LIGHT)
-			return veml6030_get_als_gain(indio_dev, val, val2);
+			return data->chip->get_als_gain(indio_dev, val, val2);
 		return -EINVAL;
 	default:
 		return -EINVAL;
@@ -571,6 +697,8 @@ static int veml6030_read_avail(struct iio_dev *indio_dev,
 			       const int **vals, int *type, int *length,
 			       long mask)
 {
+	struct veml6030_data *data = iio_priv(indio_dev);
+
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
 		*vals = (int *)&veml6030_it_times;
@@ -578,8 +706,8 @@ static int veml6030_read_avail(struct iio_dev *indio_dev,
 		*type = IIO_VAL_INT_PLUS_MICRO;
 		return IIO_AVAIL_LIST;
 	case IIO_CHAN_INFO_SCALE:
-		*vals = (int *)&veml6030_scale_vals;
-		*length = 2 * ARRAY_SIZE(veml6030_scale_vals);
+		*vals = (int *)*data->chip->scale_vals;
+		*length = 2 * data->chip->num_scale_vals;
 		*type = IIO_VAL_INT_PLUS_MICRO;
 		return IIO_AVAIL_LIST;
 	default:
@@ -591,6 +719,8 @@ static int veml6030_write_raw(struct iio_dev *indio_dev,
 				struct iio_chan_spec const *chan,
 				int val, int val2, long mask)
 {
+	struct veml6030_data *data = iio_priv(indio_dev);
+
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
 		switch (chan->type) {
@@ -602,7 +732,7 @@ static int veml6030_write_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		case IIO_LIGHT:
-			return veml6030_set_als_gain(indio_dev, val, val2);
+			return data->chip->set_als_gain(indio_dev, val, val2);
 		default:
 			return -EINVAL;
 		}
@@ -713,12 +843,28 @@ static const struct iio_info veml6030_info = {
 	.event_attrs = &veml6030_event_attr_group,
 };
 
+static const struct iio_info veml6035_info = {
+	.read_raw  = veml6030_read_raw,
+	.read_avail  = veml6030_read_avail,
+	.write_raw = veml6030_write_raw,
+	.read_event_value = veml6030_read_event_val,
+	.write_event_value = veml6030_write_event_val,
+	.read_event_config = veml6030_read_interrupt_config,
+	.write_event_config = veml6030_write_interrupt_config,
+	.event_attrs = &veml6030_event_attr_group,
+};
+
 static const struct iio_info veml6030_info_no_irq = {
 	.read_raw  = veml6030_read_raw,
 	.read_avail  = veml6030_read_avail,
 	.write_raw = veml6030_write_raw,
 };
 
+static const struct iio_info veml6035_info_no_irq = {
+	.read_raw  = veml6030_read_raw,
+	.write_raw = veml6030_write_raw,
+};
+
 static irqreturn_t veml6030_event_handler(int irq, void *private)
 {
 	int ret, reg, evtdir;
@@ -803,6 +949,63 @@ static int veml6030_hw_init(struct iio_dev *indio_dev)
 	return ret;
 }
 
+/*
+ * Set ALS gain to 1/8, integration time to 100 ms, ALS and WHITE
+ * channel enabled, ALS channel interrupt, PSM enabled,
+ * PSM_WAIT = 0.8 s, persistence to 1 x integration time and the
+ * threshold interrupt disabled by default. First shutdown the sensor,
+ * update registers and then power on the sensor.
+ */
+static int veml6035_hw_init(struct iio_dev *indio_dev)
+{
+	int ret, val;
+	struct veml6030_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+
+	ret = veml6030_als_shut_down(data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "can't shutdown als\n");
+
+	ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF,
+			   VEML6035_SENS | VEML6035_CHAN_EN | VEML6030_ALS_SD);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "can't setup als configs\n");
+
+	ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_PSM,
+				 VEML6030_PSM | VEML6030_PSM_EN, 0x03);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "can't setup default PSM\n");
+
+	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WH, 0xFFFF);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "can't setup high threshold\n");
+
+	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WL, 0x0000);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "can't setup low threshold\n");
+
+	ret = veml6030_als_pwr_on(data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "can't poweron als\n");
+
+	/* Clear stale interrupt status bits if any during start */
+	ret = regmap_read(data->regmap, VEML6030_REG_ALS_INT, &val);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+				     "can't clear als interrupt status\n");
+
+	/* Cache currently active measurement parameters */
+	data->cur_gain = 5;
+	data->cur_resolution = 1024;
+	data->cur_integration_time = 3;
+
+	return 0;
+}
+
 static int veml6030_probe(struct i2c_client *client)
 {
 	int ret;
@@ -833,7 +1036,11 @@ static int veml6030_probe(struct i2c_client *client)
 		return dev_err_probe(&client->dev, ret,
 				     "failed to enable regulator\n");
 
-	indio_dev->name = "veml6030";
+	data->chip = i2c_get_match_data(client);
+	if (!data->chip)
+		return -EINVAL;
+
+	indio_dev->name = data->chip->name;
 	indio_dev->channels = veml6030_channels;
 	indio_dev->num_channels = ARRAY_SIZE(veml6030_channels);
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -842,15 +1049,15 @@ static int veml6030_probe(struct i2c_client *client)
 		ret = devm_request_threaded_irq(&client->dev, client->irq,
 						NULL, veml6030_event_handler,
 						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-						"veml6030", indio_dev);
+						indio_dev->name, indio_dev);
 		if (ret < 0)
 			return dev_err_probe(&client->dev, ret,
 					     "irq %d request failed\n",
 					     client->irq);
 
-		indio_dev->info = &veml6030_info;
+		indio_dev->info = data->chip->info;
 	} else {
-		indio_dev->info = &veml6030_info_no_irq;
+		indio_dev->info = data->chip->info_no_irq;
 	}
 
 	ret = devm_add_action_or_reset(&client->dev,
@@ -858,7 +1065,7 @@ static int veml6030_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	ret = veml6030_hw_init(indio_dev);
+	ret = data->chip->hw_init(indio_dev);
 	if (ret < 0)
 		return ret;
 
@@ -894,14 +1101,44 @@ static int veml6030_runtime_resume(struct device *dev)
 static DEFINE_RUNTIME_DEV_PM_OPS(veml6030_pm_ops, veml6030_runtime_suspend,
 				 veml6030_runtime_resume, NULL);
 
+static const struct veml603x_chip veml6030_chip = {
+	.name = "veml6030",
+	.scale_vals = &veml6030_scale_vals,
+	.num_scale_vals = ARRAY_SIZE(veml6030_scale_vals),
+	.info = &veml6030_info,
+	.info_no_irq = &veml6030_info_no_irq,
+	.hw_init = veml6030_hw_init,
+	.set_als_gain = veml6030_set_als_gain,
+	.get_als_gain = veml6030_get_als_gain,
+};
+
+static const struct veml603x_chip veml6035_chip = {
+	.name = "veml6035",
+	.scale_vals = &veml6035_scale_vals,
+	.num_scale_vals = ARRAY_SIZE(veml6035_scale_vals),
+	.info = &veml6035_info,
+	.info_no_irq = &veml6035_info_no_irq,
+	.hw_init = veml6035_hw_init,
+	.set_als_gain = veml6035_set_als_gain,
+	.get_als_gain = veml6035_get_als_gain,
+};
+
 static const struct of_device_id veml6030_of_match[] = {
-	{ .compatible = "vishay,veml6030" },
+	{
+		.compatible = "vishay,veml6030",
+		.data = &veml6030_chip,
+	},
+	{
+		.compatible = "vishay,veml6035",
+		.data = &veml6035_chip,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, veml6030_of_match);
 
 static const struct i2c_device_id veml6030_id[] = {
-	{ "veml6030" },
+	{ "veml6030", (kernel_ulong_t)&veml6030_chip},
+	{ "veml6035", (kernel_ulong_t)&veml6035_chip},
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, veml6030_id);

-- 
2.43.0

Re: [PATCH v2 10/10] iio: light: veml6030: add support for veml6035

[Jonathan Cameron]

On Mon, 23 Sep 2024 00:17:58 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The veml6035 is an ALS that shares most of its functionality with the
> veml6030, which allows for some code recycling.
> 
> Some chip-specific properties differ and dedicated functions to get and
> set the sensor gain as well as its initialization are required.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Just a trivial formatting follow up comment inline.

Thanks,

Jonathan

> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index 19c69bfad8cb..d5c09c148d22 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -78,6 +104,15 @@ static const int veml6030_scale_vals[][2] = {
>  	{2, 0},
>  };
>  
> +static const int veml6035_scale_vals[][2] = {
> +	{0, 125000},
> +	{0, 250000},
> +	{0, 500000},
> +	{1, 0},
> +	{2, 0},
> +	{4, 0},

Similar to previous comments, add spaces after { and before }

> +};