Skip to content

Conversation

@gportay
Copy link
Contributor

@gportay gportay commented Oct 13, 2022

Dear Maintainers,

This is my first driver, and I wonder if you are interested in having it upstream.

The sensor has several working modes, and I have picked up the mode "one-time high resolution mode"; there are also five other modes that I can implement (including low resolution, continuous...). Do you want I implement all of them?

I have also a sample code, tell me if it needs to be part of the PR.

The code is based on the Bosch BME280 source code.

Kind Regards,
Gaël


This adds support for ROHM BH1750 the i2c light sensor.

Signed-off-by: Gaël PORTAY [email protected]

@zephyrbot zephyrbot added area: Sensors Sensors area: Devicetree Binding PR modifies or adds a Device Tree binding labels Oct 13, 2022
Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my first driver, and I wonder if you are interested in having it upstream.

Yes, thank you for the contribution!

The sensor has several working modes, and I have picked up the mode "one-time high resolution mode"; there are also five other modes that I can implement (including low resolution, continuous...). Do you want I implement all of them?

It's not required. If you want to add them, please do that in a follow up PR after this PR gets merged.

I have also a sample code, tell me if it needs to be part of the PR.

We are trying to reduce the number of driver-specific samples. If you're interested in helping create a generic light sensor sample similar to samples/sensor/accel_polling/ and samples/sensor/magn_polling/ and enabling the sample for this sensor, it would be a welcome addition.

The code is based on the Bosch BME280 source code.

If you've copied code from somewhere, please keep the original copyright and add your own copyright in a new line.

Please add this sensor tests/drivers/build_all/sensor to ensure the driver gets built in CI

@gportay gportay requested a review from nashif as a code owner October 14, 2022 00:51
@gportay gportay force-pushed the add-rohm-bh1750-sensor-driver branch 2 times, most recently from 984c7a2 to f454dff Compare October 14, 2022 16:27
Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, thank you.

Please review and address the failed twister checks.

@gportay gportay force-pushed the add-rohm-bh1750-sensor-driver branch from f454dff to 70da6e0 Compare October 14, 2022 19:19
Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review and address the failed twister checks.

This still needs to be addressed

@gportay gportay force-pushed the add-rohm-bh1750-sensor-driver branch from 93a713b to dd53f9f Compare October 18, 2022 17:46
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaureenHelm: as for 51269

You don't need floating point math for this

do you prefer the former solution with long long unsigned?

static int bh1750_channel_get(const struct device *dev,
			      enum sensor_channel chan,
			      struct sensor_value *val)
{
	struct bh1750_data *data = dev->data;
	long long unsigned tmp;

	switch (chan) {
	case SENSOR_CHAN_LIGHT:
		tmp = 1000000UL * data->raw_val / 1.2;
		val->val1 = tmp / 1000000;
		val->val2 = tmp % 1000000;
		break;
	default:
		return -EINVAL;
	}

	return 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaureenHelm do you want me to switch back to long long unsigned for that driver as well?

@gportay gportay force-pushed the add-rohm-bh1750-sensor-driver branch from dd53f9f to 08799fb Compare October 18, 2022 18:46
@gportay gportay force-pushed the add-rohm-bh1750-sensor-driver branch from 757455f to b1e2204 Compare December 13, 2022 02:45
This adds support for ROHM BH1750 the i2c light sensor.

Signed-off-by: Gaël PORTAY <[email protected]>
@gportay gportay force-pushed the add-rohm-bh1750-sensor-driver branch from b1e2204 to 2f26479 Compare February 3, 2023 07:57
@zephyrbot zephyrbot requested a review from yperess February 3, 2023 07:58
@gportay
Copy link
Contributor Author

gportay commented Feb 23, 2023

@MaureenHelm ping?

gportay added a commit to yidiot/esplorative-idiot that referenced this pull request Feb 24, 2023
gportay added a commit to yidiot/esperimentative-idiot that referenced this pull request Feb 25, 2023
gportay added a commit to yidiot/esperimentative-idiot that referenced this pull request Feb 25, 2023
Another driver is now upstream[1].

[1]: zephyrproject-rtos/zephyr#51268
@gportay
Copy link
Contributor Author

gportay commented Feb 25, 2023

I am closing the MR, since another driver for this hardware was merged in #43086.

@gportay gportay closed this Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Devicetree Binding PR modifies or adds a Device Tree binding area: Sensors Sensors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants