-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
drivers: sensor: tsl2540 #59330
drivers: sensor: tsl2540 #59330
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😊
74df510
to
4bdbd6f
Compare
5687c5a
to
5feddf4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
Please rebase and address the failed compliance check
drivers/sensor/tsl2540/tsl2540.c
Outdated
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
drivers/sensor/tsl2540/tsl2540.c
Outdated
uint16_t le16_buffer; | ||
|
||
ret = i2c_burst_read_dt( | ||
&((const struct tsl2540_config *)dev->config)->i2c_spec, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast shouldn't be needed
drivers/sensor/tsl2540/tsl2540.c
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document the magic numbers
dts/bindings/sensor/ams,tsl2540.yaml
Outdated
|
||
compatible: "ams,tsl2540" | ||
|
||
include: i2c-device.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include: i2c-device.yaml | |
include: [sensor-device.yaml, i2c-device.yaml] |
@@ -11,3 +11,4 @@ CONFIG_SPI=y | |||
CONFIG_W1=y | |||
CONFIG_SENSOR=y | |||
CONFIG_ICM42605_TRIGGER_NONE=y | |||
CONFIG_TSL2540=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessary
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please preserve alpha order
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please preserve alpha order
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please preserve alpha order
5feddf4
to
ddaa621
Compare
updated driver based on review and cleanup |
952e1d7
to
bb2018c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
drivers/sensor/tsl2540/Kconfig
Outdated
Integer value for a represenation with 5 decimal points. | ||
Typical value is 2.27205 from datasheet. | ||
Example: 1.2 would be 120000 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
drivers/sensor/tsl2540/Kconfig
Outdated
Integer value for a represenation with 5 decimal points. | ||
Typical value is 2.34305 from datasheet. | ||
Example: 1.2 would be 120000 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
ff3f2e9
to
5fe9b5a
Compare
Integer value for a represenation with 5 decimal points. | ||
Typical value is 2.27205 from datasheet. | ||
Example: 1.2 would be 120000 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite right. You need to justify in the description why the default was chosen, not what the default is.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite right. You need to justify in the description why the default was chosen, not what the default is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sorry. You have already specified it, sorry
5fe9b5a
to
20d5957
Compare
fix formatting issues |
df54a59
to
b2ee482
Compare
drivers/sensor/tsl2540/tsl2540.c
Outdated
|
||
k_sem_init(&data->sem, 1, K_SEM_MAX_LIMIT); | ||
|
||
if (!device_is_ready(cfg->i2c_spec.bus)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use i2c_is_ready_dt
Integer value for a represenation with 5 decimal points. | ||
Typical value is 2.27205 from datasheet. | ||
Example: 1.2 would be 120000 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite right. You need to justify in the description why the default was chosen, not what the default is.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
b2ee482
to
dea8903
Compare
Add the tsl2540 sensor to drivers.