drivers: sensor: Add driver for SB-TSI #60818

panikiel · 2023-07-26T09:27:12Z

Add a driver for the SB Temperature Sensor Interface. This is an I2C temperature sensor on AMD SoCs.

keith-zephyr · 2023-07-28T22:12:47Z

drivers/sensor/amd_sb_tsi/sb_tsi_emul.h

See my comments on #60833. Please convert this emulator to use the common backend support.

Done. tests/drivers/build_all/sensor/sensors.generic_test indeed runs my emulator (after manually setting build_only to false)

after manually setting build_only to false)

Yeah, the generic test is currently disabled upstream due to an unrelated driver acting up in CI. I'm working on addressing this.

drivers/sensor/amd_sb_tsi/sb_tsi.c

MaureenHelm · 2023-07-31T18:16:28Z

drivers/sensor/amd_sb_tsi/sb_tsi.h

+#define SB_TSI_TEMP_INT 0x01
+#define SB_TSI_TEMP_DEC 0x10
+#define SB_TSI_TEMP_DEC_SHIFT 5
+#define SB_TSI_TEMP_DEC_SCALE 8
+
+struct sb_tsi_data {
+	uint8_t sample_int;
+	uint8_t sample_dec;
+};
+
+struct sb_tsi_config {
+	struct i2c_dt_spec i2c;
+};


There's not enough here to justify a separate private header file; please move these definitions to the .c file

The defines are also used by the emulator, this header is included by sb_tsi.c and sb_tsi_emul.c.

struct sb_tsi_data and struct sb_tsi_config are only used by the C driver. Please move into the C source.

tristan-google · 2023-07-31T18:44:39Z

drivers/sensor/amd_sb_tsi/sb_tsi_emul.c

+	scaled_value = (int64_t)value << shift;
+	temp_eighths = scaled_value >> 28;
+	temp_eighths = CLAMP(temp_eighths, 0, 0x7ff);
+
+	data->reg[SB_TSI_TEMP_INT] = temp_eighths >> 3;
+	data->reg[SB_TSI_TEMP_DEC] = (temp_eighths & 0x7) << 5;


Continuing on from the other comment, what you want to do here is convert the fixed-point Celsius value passed in by the test to its corresponding register representation. In this case I'd unshift the supplied value and apply the appropriate scaling factor to convert to integer millicelsius. Then divide by 1/0.125 = 8 and clamp to get the register value.

Is this better?

tristan-google · 2023-07-31T18:44:44Z

drivers/sensor/amd_sb_tsi/sb_tsi_emul.c

+		return -ENOTSUP;
+	}
+
+	*lower = 0;


It looks like you're using these fields to pass around a raw register representation of the temperature rather than degrees Celsius in fixed-point format, which is the encoding the new sensors API as well as the backend emul API uses in place of the traditional struct sensor_val.

Based on what I could find this sensor runs a range of 0.125 C to 255.875 C. This will fit within the range of shift value 8 (Q9.23 in DSP notation), which is -256 to 255.9999998807907. The sensor's lower bound (0.125 C), for example, would be encoded as 0.125 * 2^31 >> 8 = 0x100000 (but I would just write the full expression instead of pasting the hex value for readability's sake: *lower = (int64_t)(0.125 * ((int64_t)INT32_MAX + 1)) >> *shift;). In this case, epsilon would be the same value because we go up in increments of 0.125 C. Epsilon is used as a tolerance when the generic test compares actual to expected to allow for rounding / precision errors. The value of *upper would be 0x7ff00000 (same math as above)

Let me know if you have any other questions, this can be hard to wrap one's head around at first. I found https://chummersone.github.io/qformat.html very useful. (For this case you'd plug in 32 as word size and 23 as the number of fractional bits)

Thank you very much for the explanation. I applied the changes, except for the lower bound being 0C, not 0.125C

keith-zephyr · 2023-08-08T21:38:38Z

drivers/sensor/amd_sb_tsi/sb_tsi.c

+	res = pm_device_runtime_get(dev);
+	if (res) {
+		return res;
+	}


I don't see other sensors using power management this way. There are few examples of sensors using the power management action to put the sensor into a low power mode. But what you've implemented here would prevent a power domain from going into suspend.
bmi323 example

@teburd - you mentioned defining a PM strategy for sensors here. What do you think?

I changed it to be like how it's done in bmi160 (as linked above).

keith-zephyr · 2023-08-14T17:01:21Z

drivers/sensor/amd_sb_tsi/sb_tsi_emul.c

+};
+
+struct sb_tsi_emul_cfg {
+};


Drivers and emulators don't have to provide const config data. You can just pass in NULL for the corresponding parameter.

keith-zephyr · 2023-08-14T17:01:59Z

drivers/sensor/amd_sb_tsi/sb_tsi_emul.c

+};
+
+#define SB_TSI_EMUL(n)								\
+	const struct sb_tsi_emul_cfg sb_tsi_emul_cfg_##n;			\


See above. const config info is not required.

keith-zephyr · 2023-08-14T17:02:11Z

drivers/sensor/amd_sb_tsi/sb_tsi_emul.c

+	const struct sb_tsi_emul_cfg sb_tsi_emul_cfg_##n;			\
+	struct sb_tsi_emul_data sb_tsi_emul_data_##n;				\
+	EMUL_DT_INST_DEFINE(n, sb_tsi_emul_init, &sb_tsi_emul_data_##n,		\
+			    &sb_tsi_emul_cfg_##n, &sb_tsi_emul_api_i2c,		\


Suggested change

&sb_tsi_emul_cfg_##n, &sb_tsi_emul_api_i2c, \

NULL, &sb_tsi_emul_api_i2c, \

keith-zephyr · 2023-08-14T17:04:32Z

drivers/sensor/amd_sb_tsi/sb_tsi.h

+#define SB_TSI_TEMP_INT 0x01
+#define SB_TSI_TEMP_DEC 0x10
+#define SB_TSI_TEMP_DEC_SHIFT 5
+#define SB_TSI_TEMP_DEC_SCALE 8
+
+struct sb_tsi_data {
+	uint8_t sample_int;
+	uint8_t sample_dec;
+};
+
+struct sb_tsi_config {
+	struct i2c_dt_spec i2c;
+};


struct sb_tsi_data and struct sb_tsi_config are only used by the C driver. Please move into the C source.

yperess · 2023-09-08T16:16:02Z

drivers/sensor/amd_sb_tsi/sb_tsi.c

+		return -EIO;
+	}
+
+	if (chan != SENSOR_CHAN_ALL && chan != SENSOR_CHAN_DIE_TEMP) {


I think this is the wrong channel, die temperature is usually the internal temperature of the sensor used for calibration. I think you want SENSOR_CHAN_AMBIENT_TEMP

yperess · 2023-09-08T16:16:58Z

drivers/sensor/amd_sb_tsi/sb_tsi.c

+{
+	struct sb_tsi_data *data = dev->data;
+
+	if (chan != SENSOR_CHAN_DIE_TEMP) {


Same here, use SENSOR_CHAN_AMBIENT_TEMP

panikiel · 2023-09-19T11:28:54Z

@yperess, could you please take another look?

Add a driver for the SB Temperature Sensor Interface. This is an I2C temperature sensor on AMD SoCs. Signed-off-by: Paweł Anikiel <[email protected]>

panikiel requested review from nashif, galak and MaureenHelm as code owners July 26, 2023 09:27

zephyrbot added area: Sensors Sensors area: Devicetree Binding PR modifies or adds a Device Tree binding labels Jul 26, 2023

zephyrbot requested review from avisconti, teburd and yperess July 26, 2023 09:27

zephyrbot assigned MaureenHelm Jul 26, 2023

panikiel force-pushed the sb-tsi branch from 8122d24 to 62ebdce Compare July 26, 2023 09:44

keith-zephyr requested changes Jul 28, 2023

View reviewed changes

panikiel force-pushed the sb-tsi branch from 62ebdce to bdec10c Compare July 31, 2023 15:22

keith-zephyr requested a review from tristan-google July 31, 2023 17:11

MaureenHelm requested changes Jul 31, 2023

View reviewed changes

tristan-google requested changes Jul 31, 2023

View reviewed changes

panikiel force-pushed the sb-tsi branch from bdec10c to c1509ad Compare August 1, 2023 11:27

str4t0m changed the title ~~Add driver for SB-TSI~~ drivers: sensor: Add driver for SB-TSI Aug 1, 2023

panikiel force-pushed the sb-tsi branch from c1509ad to 2472f26 Compare August 8, 2023 14:46

keith-zephyr requested changes Aug 8, 2023

View reviewed changes

panikiel force-pushed the sb-tsi branch from 2472f26 to 2afbdb7 Compare August 9, 2023 14:45

keith-zephyr reviewed Aug 14, 2023

View reviewed changes

panikiel force-pushed the sb-tsi branch from 2afbdb7 to 5504995 Compare August 16, 2023 09:34

MaureenHelm previously approved these changes Aug 16, 2023

View reviewed changes

keith-zephyr previously approved these changes Sep 6, 2023

View reviewed changes

panikiel dismissed stale reviews from keith-zephyr and MaureenHelm via 1750fd4 September 7, 2023 11:59

panikiel force-pushed the sb-tsi branch from 5504995 to 1750fd4 Compare September 7, 2023 11:59

yperess requested changes Sep 8, 2023

View reviewed changes

panikiel force-pushed the sb-tsi branch 2 times, most recently from a0b71dc to c929207 Compare September 13, 2023 08:40

panikiel force-pushed the sb-tsi branch from c929207 to caf6a38 Compare September 19, 2023 11:24

keith-zephyr previously approved these changes Sep 21, 2023

View reviewed changes

MaureenHelm previously approved these changes Sep 21, 2023

View reviewed changes

tristan-google self-requested a review September 21, 2023 18:08

tristan-google previously approved these changes Sep 21, 2023

View reviewed changes

teburd previously approved these changes Sep 22, 2023

View reviewed changes

drivers: sensor: Add driver for SB-TSI

bcac61f

Add a driver for the SB Temperature Sensor Interface. This is an I2C temperature sensor on AMD SoCs. Signed-off-by: Paweł Anikiel <[email protected]>

panikiel dismissed stale reviews from teburd, tristan-google, MaureenHelm, and keith-zephyr via bcac61f October 2, 2023 13:32

panikiel force-pushed the sb-tsi branch from caf6a38 to bcac61f Compare October 2, 2023 13:32

MaureenHelm approved these changes Oct 2, 2023

View reviewed changes

yperess approved these changes Oct 2, 2023

View reviewed changes

keith-zephyr approved these changes Oct 2, 2023

View reviewed changes

fabiobaltieri added this to the v3.6.0 milestone Oct 3, 2023

carlescufi merged commit 2f7cb40 into zephyrproject-rtos:main Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: sensor: Add driver for SB-TSI #60818

drivers: sensor: Add driver for SB-TSI #60818

panikiel commented Jul 26, 2023

keith-zephyr Jul 28, 2023

panikiel Jul 31, 2023

tristan-google Jul 31, 2023

MaureenHelm Jul 31, 2023

panikiel Aug 1, 2023 •

edited

Loading

keith-zephyr Aug 14, 2023

panikiel Aug 16, 2023

tristan-google Jul 31, 2023

panikiel Aug 1, 2023

tristan-google Jul 31, 2023

panikiel Aug 1, 2023

keith-zephyr Aug 8, 2023

panikiel Aug 9, 2023

keith-zephyr Aug 14, 2023

panikiel Aug 16, 2023

keith-zephyr Aug 14, 2023

panikiel Aug 16, 2023

keith-zephyr Aug 14, 2023

panikiel Aug 16, 2023

keith-zephyr Aug 14, 2023

yperess Sep 8, 2023

panikiel Sep 13, 2023

yperess Sep 8, 2023

panikiel Sep 13, 2023

panikiel commented Sep 19, 2023

	&sb_tsi_emul_cfg_##n, &sb_tsi_emul_api_i2c, \
	NULL, &sb_tsi_emul_api_i2c, \

drivers: sensor: Add driver for SB-TSI #60818

drivers: sensor: Add driver for SB-TSI #60818

Conversation

panikiel commented Jul 26, 2023

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

panikiel Aug 1, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panikiel commented Sep 19, 2023

panikiel Aug 1, 2023 •

edited

Loading