drivers: sensor: tsl2540 #59330

rtalbott-tmo · 2023-06-16T22:34:41Z

Add the tsl2540 sensor to drivers.

github-actions

Hello @rtalbott-tmo, and thank you very much for your first pull request to the Zephyr project!

A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊

MaureenHelm

Thank you for your contribution!

Please rebase and address the failed compliance check

MaureenHelm · 2023-08-10T20:20:18Z

drivers/sensor/tsl2540/tsl2540.c

+int tsl2540_reg_read(const struct device *dev, uint8_t reg, uint8_t *val)
+{
+	const struct tsl2540_config *cfg = dev->config;
+	int result;
+
+	result = i2c_reg_read_byte_dt(&cfg->i2c_spec, reg, val);
+
+	if (result < 0) {
+		return result;
+	}
+
+	return 0;
+}
+
+int tsl2540_reg_write(const struct device *dev, uint8_t reg, uint8_t val)
+{
+	const struct tsl2540_config *cfg = dev->config;
+	int result;
+
+	result = i2c_reg_write_byte_dt(&cfg->i2c_spec, reg, val);
+
+	if (result < 0) {
+		return result;
+	}
+
+	return 0;
+}


Wrappers like this are overkill unless the sensor supports multiple bus types (e.g., I2C and SPI). This sensor is I2C only, so please remove.

MaureenHelm · 2023-08-10T20:22:27Z

drivers/sensor/tsl2540/tsl2540.c

+		uint16_t le16_buffer;
+
+		ret = i2c_burst_read_dt(
+			     &((const struct tsl2540_config *)dev->config)->i2c_spec,


The cast shouldn't be needed

drivers/sensor/tsl2540/tsl2540.c

MaureenHelm · 2023-08-10T20:25:14Z

drivers/sensor/tsl2540/tsl2540.c

+	cpl = (data->integration_time + 1) * 2.81;
+	cpl *= data->again;
+
+	switch (chan) {
+	case SENSOR_CHAN_LIGHT:
+		sensor_value_from_double(val,
+					 data->count_vis / cpl * 53.0 * data->glass_attenuation);
+		break;
+	case SENSOR_CHAN_IR:
+		sensor_value_from_double(val,
+					 data->count_ir / cpl * 53.0 * data->glass_attenuation_ir);


Please document the magic numbers

MaureenHelm · 2023-08-10T20:41:48Z

dts/bindings/sensor/ams,tsl2540.yaml

+
+compatible: "ams,tsl2540"
+
+include: i2c-device.yaml


Suggested change

include: i2c-device.yaml

include: [sensor-device.yaml, i2c-device.yaml]

MaureenHelm · 2023-08-10T20:43:05Z

tests/drivers/build_all/sensor/prj.conf

@@ -11,3 +11,4 @@ CONFIG_SPI=y
 CONFIG_W1=y
 CONFIG_SENSOR=y
 CONFIG_ICM42605_TRIGGER_NONE=y
+CONFIG_TSL2540=y


This isn't necessary

MaureenHelm · 2023-08-10T20:43:46Z

tests/drivers/build_all/sensor/sensors_trigger_global.conf

@@ -33,6 +33,7 @@ CONFIG_LIS2DW12_TRIGGER_GLOBAL_THREAD=y
 CONFIG_LIS2MDL_TRIGGER_GLOBAL_THREAD=y
 CONFIG_LIS3MDL_TRIGGER_GLOBAL_THREAD=y
 CONFIG_LPS22HH_TRIGGER_GLOBAL_THREAD=y
+CONFIG_TSL2540_TRIGGER_GLOBAL_THREAD=y


Please preserve alpha order

MaureenHelm · 2023-08-10T20:43:50Z

tests/drivers/build_all/sensor/sensors_trigger_none.conf

@@ -33,6 +33,7 @@ CONFIG_LIS2DW12_TRIGGER_NONE=y
 CONFIG_LIS2MDL_TRIGGER_NONE=y
 CONFIG_LIS3MDL_TRIGGER_NONE=y
 CONFIG_LPS22HH_TRIGGER_NONE=y
+CONFIG_TSL2540_TRIGGER_NONE=y


Please preserve alpha order

MaureenHelm · 2023-08-10T20:43:55Z

tests/drivers/build_all/sensor/sensors_trigger_own.conf

@@ -31,6 +31,7 @@ CONFIG_LIS2DW12_TRIGGER_OWN_THREAD=y
 CONFIG_LIS2MDL_TRIGGER_OWN_THREAD=y
 CONFIG_LIS3MDL_TRIGGER_OWN_THREAD=y
 CONFIG_LPS22HH_TRIGGER_OWN_THREAD=y
+CONFIG_TSL2540_TRIGGER_OWN_THREAD=y


Please preserve alpha order

rtalbott-tmo · 2023-08-30T19:43:54Z

updated driver based on review and cleanup

avisconti

I wonder whether this parameter should be in Device Tree instead. Isn't it more a h/w configuration parameter? Moreover, when specified in DTS you can differentiate parameter values among multiple driver instance, which is maybe not applicable to this case, but I don't know.

Sorry, it is just brainstorming

avisconti · 2023-08-31T07:53:03Z

drivers/sensor/tsl2540/Kconfig

+	  Integer value for a represenation with 5 decimal points.
+	  Typical value is 2.27205 from datasheet.
+	  Example: 1.2 would be 120000
+


I wonder whether this parameter should be in Device Tree instead. Isn't it more a h/w configuration parameter? Moreover, when specified in DTS you can differentiate parameter values among multiple driver instance, which is maybe not applicable to this case, but I don't know.

Sorry, it is just brainstorming

I'm glad you brought it up as I was debating with myself about the correct implementation. My thought was that you would only generally only have once type of glass per device. But, I could see it change if placed on different sides.

I'll change it to use DTS instead of kconfig.

avisconti · 2023-08-31T07:53:14Z

drivers/sensor/tsl2540/Kconfig

+	  Integer value for a represenation with 5 decimal points.
+	  Typical value is 2.34305 from datasheet.
+	  Example: 1.2 would be 120000
+


avisconti · 2023-09-01T07:35:29Z

dts/bindings/sensor/ams,tsl2540.yaml

+      Integer value for a represenation with 5 decimal points.
+      Typical value is 2.27205 from datasheet.
+      Example: 1.2 would be 120000
+


That's ok. But I think you need to specify in the description wht that default has been selected. See https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#dt-bindings-default-rules

Rest is perfect. Thanks

This isn't quite right. You need to justify in the description why the default was chosen, not what the default is.

avisconti · 2023-09-01T07:35:52Z

dts/bindings/sensor/ams,tsl2540.yaml

+      Infa-red light attenuation.
+      Integer value for a represenation with 5 decimal points.
+      Typical value is 2.34305 from datasheet.
+      Example: 1.2 would be 120000


This isn't quite right. You need to justify in the description why the default was chosen, not what the default is.

avisconti

No, sorry. You have already specified it, sorry

rtalbott-tmo · 2023-09-01T15:22:53Z

fix formatting issues

drivers/sensor/tsl2540/tsl2540_trigger.c

drivers/sensor/tsl2540/tsl2540.h

drivers/sensor/tsl2540/tsl2540.c

MaureenHelm · 2023-09-08T15:23:30Z

drivers/sensor/tsl2540/tsl2540.c

+
+	k_sem_init(&data->sem, 1, K_SEM_MAX_LIMIT);
+
+	if (!device_is_ready(cfg->i2c_spec.bus)) {


Use i2c_is_ready_dt

MaureenHelm · 2023-09-08T15:26:44Z

dts/bindings/sensor/ams,tsl2540.yaml

+      Integer value for a represenation with 5 decimal points.
+      Typical value is 2.27205 from datasheet.
+      Example: 1.2 would be 120000
+


This isn't quite right. You need to justify in the description why the default was chosen, not what the default is.

MaureenHelm · 2023-09-08T15:27:16Z

dts/bindings/sensor/ams,tsl2540.yaml

+      Infa-red light attenuation.
+      Integer value for a represenation with 5 decimal points.
+      Typical value is 2.34305 from datasheet.
+      Example: 1.2 would be 120000


This isn't quite right. You need to justify in the description why the default was chosen, not what the default is.

Add the tsl2540 sensor to drivers. Signed-off-by: Rick Talbott <richard.talbott1@t-mobile.com>

rtalbott-tmo requested a review from MaureenHelm as a code owner June 16, 2023 22:34

zephyrbot added the area: Sensors Sensors label Jun 16, 2023

zephyrbot requested review from avisconti, teburd and yperess June 16, 2023 22:35

zephyrbot assigned MaureenHelm Jun 16, 2023

github-actions bot reviewed Jun 16, 2023

View reviewed changes

rtalbott-tmo force-pushed the drivers-tsl2540 branch 3 times, most recently from 74df510 to 4bdbd6f Compare June 27, 2023 21:36

rtalbott-tmo requested review from nashif, carlescufi, galak and alexanderwachter as code owners June 27, 2023 21:36

zephyrbot added the area: Devicetree Binding PR modifies or adds a Device Tree binding label Jun 27, 2023

rtalbott-tmo force-pushed the drivers-tsl2540 branch 2 times, most recently from 5687c5a to 5feddf4 Compare June 30, 2023 17:40

MaureenHelm requested changes Aug 10, 2023

View reviewed changes

rtalbott-tmo force-pushed the drivers-tsl2540 branch from 5feddf4 to ddaa621 Compare August 30, 2023 19:38

zephyrbot requested a review from tristan-google August 30, 2023 19:38

rtalbott-tmo requested a review from MaureenHelm August 30, 2023 19:44

rtalbott-tmo force-pushed the drivers-tsl2540 branch 2 times, most recently from 952e1d7 to bb2018c Compare August 30, 2023 20:39

avisconti reviewed Aug 31, 2023

View reviewed changes

rtalbott-tmo force-pushed the drivers-tsl2540 branch 3 times, most recently from ff3f2e9 to 5fe9b5a Compare August 31, 2023 19:33

rtalbott-tmo requested a review from avisconti August 31, 2023 19:44

avisconti requested changes Sep 1, 2023

View reviewed changes

avisconti previously approved these changes Sep 1, 2023

View reviewed changes

rtalbott-tmo dismissed avisconti’s stale review via 20d5957 September 1, 2023 15:21

rtalbott-tmo force-pushed the drivers-tsl2540 branch from 5fe9b5a to 20d5957 Compare September 1, 2023 15:21

rtalbott-tmo requested a review from avisconti September 1, 2023 15:23

avisconti previously approved these changes Sep 1, 2023

View reviewed changes

aasinclair reviewed Sep 8, 2023

View reviewed changes

drivers/sensor/tsl2540/tsl2540_trigger.c Outdated Show resolved Hide resolved

drivers/sensor/tsl2540/tsl2540.h Show resolved Hide resolved

drivers/sensor/tsl2540/tsl2540.c Outdated Show resolved Hide resolved

rtalbott-tmo dismissed avisconti’s stale review via daf6f2f September 8, 2023 14:37

rtalbott-tmo requested review from aasinclair and avisconti September 8, 2023 14:40

rtalbott-tmo force-pushed the drivers-tsl2540 branch 3 times, most recently from df54a59 to b2ee482 Compare September 8, 2023 15:26

MaureenHelm requested changes Sep 8, 2023

View reviewed changes

drivers: sensor: tsl2540

dea8903

Add the tsl2540 sensor to drivers. Signed-off-by: Rick Talbott <richard.talbott1@t-mobile.com>

rtalbott-tmo force-pushed the drivers-tsl2540 branch from b2ee482 to dea8903 Compare September 8, 2023 16:41

rtalbott-tmo requested a review from MaureenHelm September 8, 2023 16:43

MaureenHelm approved these changes Sep 11, 2023

View reviewed changes

avisconti approved these changes Sep 11, 2023

View reviewed changes

aasinclair approved these changes Sep 11, 2023

View reviewed changes

fabiobaltieri merged commit a07b79a into zephyrproject-rtos:main Sep 11, 2023
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: sensor: tsl2540 #59330

drivers: sensor: tsl2540 #59330

rtalbott-tmo commented Jun 16, 2023

github-actions bot left a comment

MaureenHelm left a comment

MaureenHelm Aug 10, 2023

MaureenHelm Aug 10, 2023

MaureenHelm Aug 10, 2023

MaureenHelm Aug 10, 2023

MaureenHelm Aug 10, 2023

MaureenHelm Aug 10, 2023

MaureenHelm Aug 10, 2023

MaureenHelm Aug 10, 2023

rtalbott-tmo commented Aug 30, 2023

avisconti left a comment

avisconti Aug 31, 2023

rtalbott-tmo Aug 31, 2023

avisconti Aug 31, 2023

avisconti Sep 1, 2023

MaureenHelm Sep 8, 2023

avisconti Sep 1, 2023

MaureenHelm Sep 8, 2023

avisconti left a comment

rtalbott-tmo commented Sep 1, 2023

MaureenHelm Sep 8, 2023

MaureenHelm Sep 8, 2023

MaureenHelm Sep 8, 2023

	include: i2c-device.yaml
	include: [sensor-device.yaml, i2c-device.yaml]


		k_sem_init(&data->sem, 1, K_SEM_MAX_LIMIT);

		if (!device_is_ready(cfg->i2c_spec.bus)) {

drivers: sensor: tsl2540 #59330

drivers: sensor: tsl2540 #59330

Conversation

rtalbott-tmo commented Jun 16, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

MaureenHelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtalbott-tmo commented Aug 30, 2023

avisconti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avisconti left a comment

Choose a reason for hiding this comment

rtalbott-tmo commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment