[PATCH 0/7] iio: light: veml6030: fix issues and add support for veml6035

[PATCH 0/7] 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,
and a fix to avoid a segmentation fault when reading the
in_illuminance_period_available attribute.

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>
---
Javier Carrasco (7):
      dt-bindings: iio: light: veml6030: rename to add manufacturer
      dt-bindings: iio: light: veml6030: add veml6035
      iio: light: veml6030: fix IIO device retrieval from embedded device
      iio: light: veml6030: make use of regmap_set_bits()
      iio: light: veml6030: update sensor resolution
      iio: light: veml6030: add set up delay after any power on sequence
      iio: light: veml6030: add support for veml6035

 .../light/{veml6030.yaml => vishay,veml6030.yaml}  |  42 ++-
 drivers/iio/light/veml6030.c                       | 323 ++++++++++++++++++---
 2 files changed, 319 insertions(+), 46 deletions(-)
---
base-commit: 5acd9952f95fb4b7da6d09a3be39195a80845eb6
change-id: 20240903-veml6035-7a91bc088c6f

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

[PATCH 1/7] dt-bindings: iio: light: veml6030: rename to add manufacturer

[Javier Carrasco]

Follow the common pattern manufacturer,devicename.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 .../bindings/iio/light/{veml6030.yaml => vishay,veml6030.yaml}          | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/light/veml6030.yaml b/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
similarity index 95%
rename from Documentation/devicetree/bindings/iio/light/veml6030.yaml
rename to Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
index fb19a2d7a849..7f4995557570 100644
--- a/Documentation/devicetree/bindings/iio/light/veml6030.yaml
+++ b/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0+
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/iio/light/veml6030.yaml#
+$id: http://devicetree.org/schemas/iio/light/vishay,veml6030.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: VEML6030 Ambient Light Sensor (ALS)

-- 
2.43.0

Re: [PATCH 1/7] dt-bindings: iio: light: veml6030: rename to add manufacturer

[Conor Dooley]

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

On Fri, Sep 13, 2024 at 03:18:56PM +0200, Javier Carrasco wrote:
> Follow the common pattern manufacturer,devicename.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

Re: [PATCH 1/7] dt-bindings: iio: light: veml6030: rename to add manufacturer

[Jonathan Cameron]

On Fri, 13 Sep 2024 17:53:31 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Fri, Sep 13, 2024 at 03:18:56PM +0200, Javier Carrasco wrote:
> > Follow the common pattern manufacturer,devicename.
> > 
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>  
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

Applied.

[PATCH 2/7] 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.

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 7f4995557570..f88e043d7ede 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:
@@ -45,6 +43,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

Re: [PATCH 2/7] dt-bindings: iio: light: veml6030: add veml6035

[Conor Dooley]

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

On Fri, Sep 13, 2024 at 03:18:57PM +0200, Javier Carrasco wrote:
> 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.
> 
> 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 7f4995557570..f88e043d7ede 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:
> @@ -45,6 +43,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

Ordinarily, I'd say that enforcing the reg properties isn't really
needed, but I think in this case the extra detail in the comments makes
it worth retaining.

Acked-by: Conor Dooley <conor.dooley@microchip.com>

> +
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - vishay,veml6035
> +    then:
> +      properties:
> +        reg:
> +          enum:
> +            - 0x29
> +
>  additionalProperties: false
>  
>  examples:
> 
> -- 
> 2.43.0
> 

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

[PATCH 3/7] iio: light: veml6030: fix IIO device retrieval from embedded device

[Javier Carrasco]

The dev pointer that is received as an argument in the
in_illuminance_period_available_show function references the device
embedded in the IIO device, not in the i2c client.

dev_to_iio_dev() must be used to accessthe right data. The current
implementation leads to a segmentation fault on every attempt to read
the attribute because indio_dev gets a NULL assignment.

This bug has been present since the first appearance of the driver,
apparently since the last version (V6) before getting applied. A
constant attribute was used until then, and the last modifications might
have not been tested again.

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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index 2e86d310952e..df2ba3078b91 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -99,9 +99,8 @@ static const char * const period_values[] = {
 static ssize_t in_illuminance_period_available_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
+	struct veml6030_data *data = iio_priv(dev_to_iio_dev(dev));
 	int ret, reg, x;
-	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
-	struct veml6030_data *data = iio_priv(indio_dev);
 
 	ret = regmap_read(data->regmap, VEML6030_REG_ALS_CONF, &reg);
 	if (ret) {

-- 
2.43.0

Re: [PATCH 3/7] iio: light: veml6030: fix IIO device retrieval from embedded device

[Jonathan Cameron]

On Fri, 13 Sep 2024 15:18:58 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The dev pointer that is received as an argument in the
> in_illuminance_period_available_show function references the device
> embedded in the IIO device, not in the i2c client.
> 
> dev_to_iio_dev() must be used to accessthe right data. The current
> implementation leads to a segmentation fault on every attempt to read
> the attribute because indio_dev gets a NULL assignment.
> 
> This bug has been present since the first appearance of the driver,
> apparently since the last version (V6) before getting applied. A
> constant attribute was used until then, and the last modifications might
> have not been tested again.
> 
> 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 to the fixes-togreg branch of iio.git.

Thanks,
Jonathan

> ---
>  drivers/iio/light/veml6030.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index 2e86d310952e..df2ba3078b91 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -99,9 +99,8 @@ static const char * const period_values[] = {
>  static ssize_t in_illuminance_period_available_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> +	struct veml6030_data *data = iio_priv(dev_to_iio_dev(dev));
>  	int ret, reg, x;
> -	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> -	struct veml6030_data *data = iio_priv(indio_dev);
>  
>  	ret = regmap_read(data->regmap, VEML6030_REG_ALS_CONF, &reg);
>  	if (ret) {
> 

[PATCH 4/7] iio: light: veml6030: make use of regmap_set_bits()

[Javier Carrasco]

Instead of using regmap_update_bits() and passing val = 1, use
regmap_set_bits().

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

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index df2ba3078b91..5d4c2e35b987 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -149,8 +149,8 @@ static int veml6030_als_pwr_on(struct veml6030_data *data)
 
 static int veml6030_als_shut_down(struct veml6030_data *data)
 {
-	return regmap_update_bits(data->regmap, VEML6030_REG_ALS_CONF,
-				 VEML6030_ALS_SD, 1);
+	return regmap_set_bits(data->regmap, VEML6030_REG_ALS_CONF,
+				 VEML6030_ALS_SD);
 }
 
 static void veml6030_als_shut_down_action(void *data)

-- 
2.43.0

Re: [PATCH 4/7] iio: light: veml6030: make use of regmap_set_bits()

[Jonathan Cameron]

On Fri, 13 Sep 2024 15:18:59 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> Instead of using regmap_update_bits() and passing val = 1, use
> regmap_set_bits().
> 
Applied but description tweaked as key here is that VEML6030_ALS_SD is 1.
If it contained other bits then this change would be wrong.


> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/iio/light/veml6030.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index df2ba3078b91..5d4c2e35b987 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -149,8 +149,8 @@ static int veml6030_als_pwr_on(struct veml6030_data *data)
>  
>  static int veml6030_als_shut_down(struct veml6030_data *data)
>  {
> -	return regmap_update_bits(data->regmap, VEML6030_REG_ALS_CONF,
> -				 VEML6030_ALS_SD, 1);
> +	return regmap_set_bits(data->regmap, VEML6030_REG_ALS_CONF,
> +				 VEML6030_ALS_SD);
>  }
>  
>  static void veml6030_als_shut_down_action(void *data)
> 

[PATCH 5/7] iio: light: veml6030: update 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.

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 5d4c2e35b987..d5add040d0b3 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -779,7 +779,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 5/7] iio: light: veml6030: update sensor resolution

[Jonathan Cameron]

On Fri, 13 Sep 2024 15:19:00 +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.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Interesting.  So does the datasheet say this was fixing an error, or
is there any chance there are different versions of the chip out there?

Also, should we treat this as a fix?  I think we probably should given
we don't really want stable kernels to have wrong data being reported.
If so, please reply with a fixes tag.

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 5d4c2e35b987..d5add040d0b3 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -779,7 +779,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;
> 

Re: [PATCH 5/7] iio: light: veml6030: update sensor resolution

[Javier Carrasco]

On 14/09/2024 16:57, Jonathan Cameron wrote:
> On Fri, 13 Sep 2024 15:19:00 +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.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> Interesting.  So does the datasheet say this was fixing an error, or
> is there any chance there are different versions of the chip out there?
> 
> Also, should we treat this as a fix?  I think we probably should given
> we don't really want stable kernels to have wrong data being reported.
> If so, please reply with a fixes tag.
> 
> Jonathan
>

According to the Product Information Notification (link in the cover
letter):

"Reason for Change: Adjusted resolution as this was wrongly stated in
the current datasheet."

"If resolution is defined in the particular application by the customer,
no changes in the system should be made. In the case resolution was
taken from the datasheet or app note, this has to be adjusted accordingly."

Which means that stable kernels are using the wrong resolution. I don't
know what IIO usually does in such cases, because a fix could
potentially make existing applications return "wrong data". If that is
alright, and applications are meant to be adjusted after the kernel
update, I have no problems to make this patch as a fix and add the
stable tag.

Best regards,
Javier Carrasco

>> ---
>>  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 5d4c2e35b987..d5add040d0b3 100644
>> --- a/drivers/iio/light/veml6030.c
>> +++ b/drivers/iio/light/veml6030.c
>> @@ -779,7 +779,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;
>>
> 

Re: [PATCH 5/7] iio: light: veml6030: update sensor resolution

[Jonathan Cameron]

On Sun, 15 Sep 2024 10:31:11 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> On 14/09/2024 16:57, Jonathan Cameron wrote:
> > On Fri, 13 Sep 2024 15:19:00 +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.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>  
> > Interesting.  So does the datasheet say this was fixing an error, or
> > is there any chance there are different versions of the chip out there?
> > 
> > Also, should we treat this as a fix?  I think we probably should given
> > we don't really want stable kernels to have wrong data being reported.
> > If so, please reply with a fixes tag.
> > 
> > Jonathan
> >  
> 
> According to the Product Information Notification (link in the cover
> letter):
> 
> "Reason for Change: Adjusted resolution as this was wrongly stated in
> the current datasheet."
> 
> "If resolution is defined in the particular application by the customer,
> no changes in the system should be made. In the case resolution was
> taken from the datasheet or app note, this has to be adjusted accordingly."
> 
> Which means that stable kernels are using the wrong resolution. I don't
> know what IIO usually does in such cases, because a fix could
> potentially make existing applications return "wrong data". If that is
> alright, and applications are meant to be adjusted after the kernel
> update, I have no problems to make this patch as a fix and add the
> stable tag.

It's unfortunate, but fixing a bug is a valid reason for ABI change
(which this is - sort of) so existing applications will need to be
fixed if anyone notices.

So please send this as a fix with appropriate tags and
that datasheet change log included in the patch description.

Thanks,

Jonathan

> 
> Best regards,
> Javier Carrasco
> 
> >> ---
> >>  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 5d4c2e35b987..d5add040d0b3 100644
> >> --- a/drivers/iio/light/veml6030.c
> >> +++ b/drivers/iio/light/veml6030.c
> >> @@ -779,7 +779,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 6/7] 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.

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 d5add040d0b3..2945cc1db599 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -143,8 +143,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 */
+	usleep_range(4000, 4002);
+
+	return 0;
 }
 
 static int veml6030_als_shut_down(struct veml6030_data *data)
@@ -766,9 +775,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 6/7] iio: light: veml6030: add set up delay after any power on sequence

[Jonathan Cameron]

On Fri, 13 Sep 2024 15:19:01 +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.
> 
> 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 d5add040d0b3..2945cc1db599 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -143,8 +143,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 */
> +	usleep_range(4000, 4002);
There is no useful point in such a narrow range. Given you are moving
it let's tidy that up as well.

Just use fsleep() which will use a much wider range but is good enough.

Jonathan

> +
> +	return 0;
>  }
>  
>  static int veml6030_als_shut_down(struct veml6030_data *data)
> @@ -766,9 +775,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 7/7] 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/veml6030.c | 300 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 273 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index 2945cc1db599..105f310c4954 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>
@@ -38,16 +44,33 @@
 #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 struct iio_info *info;
+	const struct iio_info *info_no_irq;
+	const char * const in_illuminance_scale_avail;
+	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;
@@ -55,6 +78,7 @@ struct veml6030_data {
 	int cur_resolution;
 	int cur_gain;
 	int cur_integration_time;
+	const struct veml603x_chip *chip;
 };
 
 /* Integration time available in seconds */
@@ -63,14 +87,25 @@ static IIO_CONST_ATTR(in_illuminance_integration_time_available,
 
 /*
  * 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 IIO_CONST_ATTR(in_illuminance_scale_available,
+static IIO_CONST_ATTR_NAMED(veml6030_in_illuminance_scale_available,
+			    in_illuminance_scale_available,
 				"0.125 0.25 1.0 2.0");
+static IIO_CONST_ATTR_NAMED(veml6035_in_illuminance_scale_available,
+			    in_illuminance_scale_available,
+				"0.125 0.25 0.5 1.0 2.0 4.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,
+	&iio_const_attr_veml6030_in_illuminance_scale_available.dev_attr.attr,
+	NULL
+};
+
+static struct attribute *veml6035_attributes[] = {
+	&iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
+	&iio_const_attr_veml6035_in_illuminance_scale_available.dev_attr.attr,
 	NULL
 };
 
@@ -78,6 +113,10 @@ static const struct attribute_group veml6030_attr_group = {
 	.attrs = veml6030_attributes,
 };
 
+static const struct attribute_group veml6035_attr_group = {
+	.attrs = veml6035_attributes,
+};
+
 /*
  * Persistence = 1/2/4/8 x integration time
  * Minimum time for which light readings must stay above configured
@@ -380,6 +419,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)
 {
@@ -410,19 +464,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,
@@ -462,6 +546,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)
 {
@@ -558,7 +688,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;
@@ -569,6 +699,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) {
@@ -580,7 +712,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;
 		}
@@ -691,12 +823,29 @@ 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,
+	.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 = &veml6035_attr_group,
+	.event_attrs = &veml6030_event_attr_group,
+};
+
 static const struct iio_info veml6030_info_no_irq = {
 	.read_raw  = veml6030_read_raw,
 	.write_raw = veml6030_write_raw,
 	.attrs = &veml6030_attr_group,
 };
 
+static const struct iio_info veml6035_info_no_irq = {
+	.read_raw  = veml6030_read_raw,
+	.write_raw = veml6030_write_raw,
+	.attrs = &veml6035_attr_group,
+};
+
 static irqreturn_t veml6030_event_handler(int irq, void *private)
 {
 	int ret, reg, evtdir;
@@ -791,6 +940,73 @@ 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) {
+		dev_err(&client->dev, "can't shutdown als %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF,
+			   VEML6035_SENS | VEML6035_CHAN_EN | VEML6030_ALS_SD);
+	if (ret) {
+		dev_err(&client->dev, "can't setup als configs %d\n", ret);
+		return ret;
+	}
+
+	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;
+	}
+
+	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;
+	}
+
+	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;
+	}
+
+	ret = veml6030_als_pwr_on(data);
+	if (ret) {
+		dev_err(&client->dev, "can't poweron als %d\n", ret);
+		return ret;
+	}
+
+	/* 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;
+	}
+
+	/* 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;
@@ -818,7 +1034,11 @@ static int veml6030_probe(struct i2c_client *client)
 	data->client = client;
 	data->regmap = regmap;
 
-	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;
@@ -827,18 +1047,18 @@ 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) {
 			dev_err(&client->dev,
 					"irq %d request failed\n", client->irq);
 			return ret;
 		}
-		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 = veml6030_hw_init(indio_dev);
+	ret = data->chip->hw_init(indio_dev);
 	if (ret < 0)
 		return ret;
 
@@ -879,14 +1099,40 @@ 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",
+	.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",
+	.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 7/7] iio: light: veml6030: add support for veml6035

[Jonathan Cameron]

On Fri, 13 Sep 2024 15:19:02 +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>
Mostly a request to first switch to using read_avail() and the relevant
bit masks instead of custom attributes.  That will require converting the
driver to that approach first, but looks straight forward.

> ---
>  drivers/iio/light/veml6030.c | 300 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 273 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index 2945cc1db599..105f310c4954 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>
> @@ -38,16 +44,33 @@
>  #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 struct iio_info *info;
> +	const struct iio_info *info_no_irq;
> +	const char * const in_illuminance_scale_avail;

For this, better with read_avail() provided and a pointer to an array of
values + a size element in here.  That way we can get rid of the
custom attribute handling.  Might end up as similar amount of code, but
will be simpler to read.

> +	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);
> +};

>  
>  /* Integration time available in seconds */
> @@ -63,14 +87,25 @@ static IIO_CONST_ATTR(in_illuminance_integration_time_available,
>  
>  /*
>   * 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 IIO_CONST_ATTR(in_illuminance_scale_available,
> +static IIO_CONST_ATTR_NAMED(veml6030_in_illuminance_scale_available,
> +			    in_illuminance_scale_available,
>  				"0.125 0.25 1.0 2.0");
> +static IIO_CONST_ATTR_NAMED(veml6035_in_illuminance_scale_available,
> +			    in_illuminance_scale_available,
> +				"0.125 0.25 0.5 1.0 2.0 4.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,
> +	&iio_const_attr_veml6030_in_illuminance_scale_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *veml6035_attributes[] = {
> +	&iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
> +	&iio_const_attr_veml6035_in_illuminance_scale_available.dev_attr.attr,

Using get_avail() etc would let you handle these as arrays of numbers rather than
strings + get rid of the need for any custom attributes. This should be
a very simple conversion so perhaps worth doing before adding the
new support.  Then you will have pointers to the value arrays + sizes
in your chip specific structures that just get looked up directly
by read_avail()


>  	NULL
>  };


>  
> +/*
> + * 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) {
> +		dev_err(&client->dev, "can't shutdown als %d\n", ret);
> +		return ret;

If this is only ever called from probe() (I think that's true?)
can use return dev_err_probe() for all these error cases.
Main advantage here being shorter simpler code.

> +	}
> +
> +	ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF,
> +			   VEML6035_SENS | VEML6035_CHAN_EN | VEML6030_ALS_SD);
> +	if (ret) {
> +		dev_err(&client->dev, "can't setup als configs %d\n", ret);
> +		return ret;
> +	}
> +
> +	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;
> +	}
> +
> +	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;
> +	}
> +
> +	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;
> +	}
> +
> +	ret = veml6030_als_pwr_on(data);
> +	if (ret) {
> +		dev_err(&client->dev, "can't poweron als %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* 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;

It's true of existing code, but I noticed it here.
Should we be powering down in this error path?

> +	}
> +
> +	/* Cache currently active measurement parameters */
> +	data->cur_gain = 5;
> +	data->cur_resolution = 1024;
> +	data->cur_integration_time = 3;
> +
> +	return 0;
> +}

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

[Javier Carrasco]

On 14/09/2024 18:03, Jonathan Cameron wrote:
> On Fri, 13 Sep 2024 15:19:02 +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>
> Mostly a request to first switch to using read_avail() and the relevant
> bit masks instead of custom attributes.  That will require converting the
> driver to that approach first, but looks straight forward.
> 
>> ---
>>  drivers/iio/light/veml6030.c | 300 +++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 273 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
>> index 2945cc1db599..105f310c4954 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>
>> @@ -38,16 +44,33 @@
>>  #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 struct iio_info *info;
>> +	const struct iio_info *info_no_irq;
>> +	const char * const in_illuminance_scale_avail;
> 
> For this, better with read_avail() provided and a pointer to an array of
> values + a size element in here.  That way we can get rid of the
> custom attribute handling.  Might end up as similar amount of code, but
> will be simpler to read.
> 
>> +	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);
>> +};
> 
>>  
>>  /* Integration time available in seconds */
>> @@ -63,14 +87,25 @@ static IIO_CONST_ATTR(in_illuminance_integration_time_available,
>>  
>>  /*
>>   * 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 IIO_CONST_ATTR(in_illuminance_scale_available,
>> +static IIO_CONST_ATTR_NAMED(veml6030_in_illuminance_scale_available,
>> +			    in_illuminance_scale_available,
>>  				"0.125 0.25 1.0 2.0");
>> +static IIO_CONST_ATTR_NAMED(veml6035_in_illuminance_scale_available,
>> +			    in_illuminance_scale_available,
>> +				"0.125 0.25 0.5 1.0 2.0 4.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,
>> +	&iio_const_attr_veml6030_in_illuminance_scale_available.dev_attr.attr,
>> +	NULL
>> +};
>> +
>> +static struct attribute *veml6035_attributes[] = {
>> +	&iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
>> +	&iio_const_attr_veml6035_in_illuminance_scale_available.dev_attr.attr,
> 
> Using get_avail() etc would let you handle these as arrays of numbers rather than
> strings + get rid of the need for any custom attributes. This should be
> a very simple conversion so perhaps worth doing before adding the
> new support.  Then you will have pointers to the value arrays + sizes
> in your chip specific structures that just get looked up directly
> by read_avail()
> 
> 

Hi Jonathan, thanks for your review.

I will update the update the driver to work that way.

>>  	NULL
>>  };
> 
> 
>>  
>> +/*
>> + * 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) {
>> +		dev_err(&client->dev, "can't shutdown als %d\n", ret);
>> +		return ret;
> 
> If this is only ever called from probe() (I think that's true?)
> can use return dev_err_probe() for all these error cases.
> Main advantage here being shorter simpler code.
> 

I know, I procrastinated a little bit and I left the dev_err() calls as
they are. But that's easy to update, so I will add a patch for it in v2.
>> +	}
>> +
>> +	ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF,
>> +			   VEML6035_SENS | VEML6035_CHAN_EN | VEML6030_ALS_SD);
>> +	if (ret) {
>> +		dev_err(&client->dev, "can't setup als configs %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	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;
>> +	}
>> +
>> +	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;
>> +	}
>> +
>> +	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;
>> +	}
>> +
>> +	ret = veml6030_als_pwr_on(data);
>> +	if (ret) {
>> +		dev_err(&client->dev, "can't poweron als %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* 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;
> 
> It's true of existing code, but I noticed it here.
> Should we be powering down in this error path?
> 

We could, because this is the only error path where the device is
powered on before the power off action gets registered. On the other
hand, could we not move the call to devm_add_action_or_reset() a few
lines up, so the action gets registered before calling hw_init()?

Powering off the device is just writing a bit, so it would not hurt in
the error paths where the device is already powered off. Then we would
not need an explicit call to power off the device in this error path.

>> +	}
>> +
>> +	/* Cache currently active measurement parameters */
>> +	data->cur_gain = 5;
>> +	data->cur_resolution = 1024;
>> +	data->cur_integration_time = 3;
>> +
>> +	return 0;
>> +}
> 


Best regards,
Javier Carrasco

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

[Jonathan Cameron]

...

> >>  
> >> +/*
> >> + * 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) {
> >> +		dev_err(&client->dev, "can't shutdown als %d\n", ret);
> >> +		return ret;  
> > 
> > If this is only ever called from probe() (I think that's true?)
> > can use return dev_err_probe() for all these error cases.
> > Main advantage here being shorter simpler code.
> >   
> 
> I know, I procrastinated a little bit and I left the dev_err() calls as
> they are. But that's easy to update, so I will add a patch for it in v2.
> >> +	}
> >> +
> >> +	ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF,
> >> +			   VEML6035_SENS | VEML6035_CHAN_EN | VEML6030_ALS_SD);
> >> +	if (ret) {
> >> +		dev_err(&client->dev, "can't setup als configs %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	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;
> >> +	}
> >> +
> >> +	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;
> >> +	}
> >> +
> >> +	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;
> >> +	}
> >> +
> >> +	ret = veml6030_als_pwr_on(data);
> >> +	if (ret) {
> >> +		dev_err(&client->dev, "can't poweron als %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	/* 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;  
> > 
> > It's true of existing code, but I noticed it here.
> > Should we be powering down in this error path?
> >   
> 
> We could, because this is the only error path where the device is
> powered on before the power off action gets registered. On the other
> hand, could we not move the call to devm_add_action_or_reset() a few
> lines up, so the action gets registered before calling hw_init()?

Move it in here so that you register the powerdown alongside the power
up. Doesn't matter that it is inside the hw_init() function so a little
less obvious.

Jonathan
> 
> Powering off the device is just writing a bit, so it would not hurt in
> the error paths where the device is already powered off. Then we would
> not need an explicit call to power off the device in this error path.
> 
> >> +	}
> >> +
> >> +	/* Cache currently active measurement parameters */
> >> +	data->cur_gain = 5;
> >> +	data->cur_resolution = 1024;
> >> +	data->cur_integration_time = 3;
> >> +
> >> +	return 0;
> >> +}  
> >   
> 
> 
> Best regards,
> Javier Carrasco