Move ctx inside stmemsc #34389

avisconti · 2021-04-19T13:57:36Z

Every STMESC sensor driver needs to define a per-instance stmdev_ctx_t structure tha contains the platform specific bus read/write routines. This PR is moving this structure from data to config, in order to be able to fill it at compile time rather than runtime, with the benefit of being able to remove the bus_init() routines for I2C and SPI bus (so, removing the two files completely). THis was already suggested in #33506 by @mbolivar-nordic , and I turned the proposal down, but eventually I changed my mind and decided to implement it now. Sorry for this extra review...

This should become THE way for all the other ST sesnors using stmemsc module. I will provide the proper alignment in a separate PR once this will be reviewed.

EDIT:
In this PR I also put an additional commit unrelated to stmemsc, but belonging to same sensor used to test the changes (iis2iclx).
Tested using iis2iclx on x_nucleo_iks02a1 shield:

samples/shields/x_nucleo_iks02a1/standard (iis2iclx on shield (i2c) and on dil24 (spi))
samples/shields/x_nucleo_iks02a1/sensorhub (iis2iclx on shield (i2c and sensorhub) and on dil24 (spi))

pabigot

None of my concerns are blocking, but I do think at least a couple should be addressed, so not approving yet.

I hope somebody takes on global verification that unready device pointer obtained from devicetree macros don't reach the driver functions without being checked for being ready. That would be...bad.

pabigot · 2021-04-19T14:39:41Z

drivers/sensor/iis2iclx/iis2iclx.h

-	const char *irq_dev_name;
-	uint8_t irq_pin;
-	uint8_t irq_flags;
+	const struct gpio_dt_spec gpio_drdy;


You'd get less wasted padding if you moved the uint8_t member above the gpio_dt_spec member.

Are you suggesting to move uint8_t int_pin; before the gpio_dt_spec structure?

Yes, sure. Keeping the uint8_t all together should result in a better structure packing. THanks.

I think that I squashed this change with the wrong commit. I need to re-do it properly!

pabigot · 2021-04-19T14:43:27Z

drivers/sensor/iis2iclx/iis2iclx_trigger.c


-	if (gpio_add_callback(iis2iclx->gpio, &iis2iclx->gpio_cb) < 0) {
+	if (gpio_add_callback(cfg->gpio_drdy.port, &iis2iclx->gpio_cb) < 0) {


I don't remember what color we chose for this bikeshed; does anything in the driver infrastructure verify that the passed driver instance is ready? @galak @mnkp

I see it being checked in the clock control driver glue code, but not gpio.

I'm checking

if(device_is_ready(cfg->gpio_drdy.port))

at the beginning of the interrupt init function. Is this your concern? I'm not sure I got the point correctly.

OK, it's fine here, but in general requiring this sort of check at all call sites is error-prone. But maybe that's what people wanted. I'm not blocking on this.

does anything in the driver infrastructure verify that the passed driver instance is ready?

I don't think so. We check that the dev pointer passed to the function call is not NULL if user mode is enabled.

pabigot · 2021-04-19T14:45:36Z

drivers/sensor/iis2iclx/iis2iclx.c

@@ -659,12 +657,15 @@ static int iis2iclx_init(const struct device *dev)

 #define IIS2ICLX_CONFIG_SPI(inst)					\
 	{								\
+		.ctx.read_reg = (stmdev_read_ptr) stmemsc_spi_read,	\


This style technically works for C, but IMO it would be better as:

.ctx = { .read_reg = ..., ... },

Though the same concern applies to stmemsc_cfg,where it would be more tedious. So not blocking.

Something like that would be a good compromise? Seems more readable in fact...

#define IIS2ICLX_CONFIG_SPI(inst) \ { \ .ctx = { \ .read_reg = (stmdev_read_ptr) stmemsc_spi_read, \ .write_reg = (stmdev_write_ptr) stmemsc_spi_write, \ .handle = (void *)&iis2iclx_config_##inst.stmemsc_cfg, \ }, \ .stmemsc_cfg.spi = { \ .bus = DEVICE_DT_GET(DT_INST_BUS(inst)), \ .spi_cfg = \ SPI_CONFIG_DT_INST(inst, \ IIS2ICLX_SPI_OPERATION, \ 0), \ }, \ .odr = DT_INST_PROP(inst, odr), \ .range = DT_INST_PROP(inst, range), \ COND_CODE_1(DT_INST_NODE_HAS_PROP(inst, drdy_gpios), \ (IIS2ICLX_CFG_IRQ(inst)), ()) \ }

Yes, I like that better.

pabigot · 2021-04-19T14:48:30Z

drivers/sensor/iis2iclx/iis2iclx_trigger.c

@@ -238,7 +239,7 @@ int iis2iclx_init_interrupt(const struct device *dev)
 	}

 	/* enable interrupt on int1/int2 in pulse mode */
-	if (iis2iclx_int_notification_set(&iis2iclx->ctx,
+	if (iis2iclx_int_notification_set((stmdev_ctx_t *)&cfg->ctx,


Why do you need the cast here? &cfg->ctx is the right type already

It complains with a warning like this

In file included from /local/home/visconti/Projects/zephyrproject/zephyr/drivers/sensor/iis2iclx/iis2iclx.h:19,
from /local/home/visconti/Projects/zephyrproject/zephyr/drivers/sensor/iis2iclx/iis2iclx_trigger.c:18:
/local/home/visconti/Projects/zephyrproject/modules/hal/st/sensor/stmemsc/iis2iclx_STdC/driver/iis2iclx_reg.h:2605:53: note: expected 'stmdev_ctx_t *' {aka 'struct *'} but argument is of type 'const stmdev_ctx_t *' {aka 'const struct *'}

I guess this is because cfg is declared as

const struct iis2iclx_config *cfg

Isn't it? Is there a different way to avoid the warning?

You could fix the prototype to make the argument pointer-to-const. See also STMicroelectronics/STMems_Standard_C_drivers#121.

Casting away const is a MISRA violation, and generally a bad idea indicating a flawed API.

I don't want to change the prototype, because I want to keep the stmemsc module (the STdC driver in our terminology) same as it was released, without any change. So, I asked my colleague who is actually maintaining the STdC drivers to change it, but I'm not sure he want to do that even if it is correct what you said. Moreover I noticed that you already commented few days ago for the exactly same reason (or maybe similar, I don't recall) in that github repo.

OK, I hadn't noticed that function was from stmemc. In that case I guess you have to have the cast.

@pabigot
STMicroelectronics/STMems_Standard_C_drivers#121

Oh, sorry. It is the one that you just mentioned above... :D

drivers/sensor/iis2iclx/iis2iclx_trigger.c

mnkp · 2021-04-20T10:52:35Z

drivers/sensor/iis2iclx/iis2iclx_trigger.c


-	if (gpio_add_callback(iis2iclx->gpio, &iis2iclx->gpio_cb) < 0) {
+	if (gpio_add_callback(cfg->gpio_drdy.port, &iis2iclx->gpio_cb) < 0) {


does anything in the driver infrastructure verify that the passed driver instance is ready?

I don't think so. We check that the dev pointer passed to the function call is not NULL if user mode is enabled.

Use gpio_dt_spec structure and populate it using GPIO_DT_SPEC_GET macro. Signed-off-by: Armando Visconti <armando.visconti@st.com>

avisconti

github request me to write something here...

avisconti · 2021-04-20T11:52:19Z

drivers/sensor/iis2iclx/iis2iclx.h

-	const char *irq_dev_name;
-	uint8_t irq_pin;
-	uint8_t irq_flags;
+	const struct gpio_dt_spec gpio_drdy;


I think that I squashed this change with the wrong commit. I need to re-do it properly!

Move ctx structure from struct data to struct config, so that it can be filled at compile time and we could get rid of the bus init routines. Signed-off-by: Armando Visconti <armando.visconti@st.com>

avisconti · 2021-04-21T08:55:09Z

The CI failed with following error which seems to be unrelated to the PR itself:

fatal error: error writing to /tmp/cc3CNLA0.s: No space left on device

I'm just pushing again after changing hash of the commit (but w/o a real change to PR) just to re-trigger th CI.

avisconti added Enhancement Changes/Updates/Additions to existing features area: Drivers area: Sensors Sensors labels Apr 19, 2021

avisconti requested review from pabigot, MaureenHelm and mbolivar-nordic April 19, 2021 13:57

avisconti self-assigned this Apr 19, 2021

avisconti force-pushed the move-ctx-inside-stmemsc branch from fba10b2 to bbac22d Compare April 19, 2021 14:18

pabigot reviewed Apr 19, 2021

View reviewed changes

pabigot mentioned this pull request Apr 19, 2021

const correctness for platform write STMicroelectronics/STMems_Standard_C_drivers#121

Closed

avisconti force-pushed the move-ctx-inside-stmemsc branch from bbac22d to 0e64448 Compare April 20, 2021 08:49

mnkp reviewed Apr 20, 2021

View reviewed changes

avisconti force-pushed the move-ctx-inside-stmemsc branch from 0e64448 to eb6758d Compare April 20, 2021 11:37

mnkp approved these changes Apr 20, 2021

View reviewed changes

drivers/sensor: iis2iclx: Use DT helper for gpio_drdy

fd587ef

Use gpio_dt_spec structure and populate it using GPIO_DT_SPEC_GET macro. Signed-off-by: Armando Visconti <armando.visconti@st.com>

avisconti force-pushed the move-ctx-inside-stmemsc branch from eb6758d to 3936f38 Compare April 20, 2021 12:04

avisconti commented Apr 20, 2021

View reviewed changes

mbolivar-nordic approved these changes Apr 21, 2021

View reviewed changes

drivers/sensor: iis2iclx: move ctx structure inside config

5597dbf

Move ctx structure from struct data to struct config, so that it can be filled at compile time and we could get rid of the bus init routines. Signed-off-by: Armando Visconti <armando.visconti@st.com>

avisconti force-pushed the move-ctx-inside-stmemsc branch from 3936f38 to 5597dbf Compare April 21, 2021 08:51

pabigot approved these changes Apr 21, 2021

View reviewed changes

nashif merged commit 8717654 into zephyrproject-rtos:master Apr 22, 2021

avisconti deleted the move-ctx-inside-stmemsc branch April 23, 2021 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ctx inside stmemsc #34389

Move ctx inside stmemsc #34389

avisconti commented Apr 19, 2021 •

edited

Loading

pabigot left a comment

pabigot Apr 19, 2021

avisconti Apr 19, 2021

pabigot Apr 19, 2021

avisconti Apr 20, 2021

avisconti Apr 20, 2021

pabigot Apr 19, 2021

avisconti Apr 19, 2021

pabigot Apr 19, 2021

mnkp Apr 20, 2021

pabigot Apr 19, 2021

avisconti Apr 19, 2021

pabigot Apr 19, 2021

pabigot Apr 19, 2021

avisconti Apr 19, 2021

pabigot Apr 19, 2021

avisconti Apr 19, 2021

pabigot Apr 19, 2021

avisconti Apr 19, 2021

avisconti Apr 19, 2021

mnkp Apr 20, 2021

avisconti left a comment

avisconti Apr 20, 2021

avisconti commented Apr 21, 2021


		if (gpio_add_callback(iis2iclx->gpio, &iis2iclx->gpio_cb) < 0) {
		if (gpio_add_callback(cfg->gpio_drdy.port, &iis2iclx->gpio_cb) < 0) {

Move ctx inside stmemsc #34389

Move ctx inside stmemsc #34389

Conversation

avisconti commented Apr 19, 2021 • edited Loading

pabigot 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

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

avisconti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avisconti commented Apr 21, 2021

avisconti commented Apr 19, 2021 •

edited

Loading