drivers: stm32: correct wrong clock_control_subsys_t cast in clock_control calls

[avolmat-st]

This PR correct incorrects (clock_control_subsys_t *) cast done when calling some clock_control_ apis in some stm32 drivers.

[mathieuchopstm]

How ironic. Makes you wonder whether casts should even be there in the first place...

[jfischer-no]

That is not a correct fix, explicit type conversion is the bug here, and we had too many of them last time.

diff --git a/drivers/usb/udc/udc_stm32.c b/drivers/usb/udc/udc_stm32.c
index 6d4f14af70c..fd067a77b57 100644
--- a/drivers/usb/udc/udc_stm32.c
+++ b/drivers/usb/udc/udc_stm32.c
@@ -1034,7 +1034,7 @@ static void priv_pcd_prepare(const struct device *dev)
 #endif /* USB_OTG_HS_EMB_PHY */
 }
 
-static const struct stm32_pclken pclken[] = STM32_DT_INST_CLOCKS(0);
+static struct stm32_pclken pclken[] = STM32_DT_INST_CLOCKS(0);
 
 static int priv_clock_enable(void)
 {
@@ -1108,14 +1108,13 @@ static int priv_clock_enable(void)
 #endif
 
        if (DT_INST_NUM_CLOCKS(0) > 1) {
-               if (clock_control_configure(clk, (clock_control_subsys_t *)&pclken[1],
-                                                                       NULL) != 0) {
+               if (clock_control_configure(clk, &pclken[1], NULL) != 0) {
                        LOG_ERR("Could not select USB domain clock");
                        return -EIO;
                }
        }
 
-       if (clock_control_on(clk, (clock_control_subsys_t *)&pclken[0]) != 0) {
+       if (clock_control_on(clk, &pclken[0]) != 0) {
                LOG_ERR("Unable to enable USB clock");
                return -EIO;
        }
@@ -1124,7 +1123,7 @@ static int priv_clock_enable(void)
                uint32_t usb_clock_rate;
 
                if (clock_control_get_rate(clk,
-                                          (clock_control_subsys_t *)&pclken[1],
+                                          &pclken[1],
                                           &usb_clock_rate) != 0) {
                        LOG_ERR("Failed to get USB domain clock rate");
                        return -EIO;
@@ -1195,7 +1194,7 @@ static int priv_clock_disable(void)
 {
        const struct device *clk = DEVICE_DT_GET(STM32_CLOCK_CONTROL_NODE);
 
-       if (clock_control_off(clk, (clock_control_subsys_t *)&pclken[0]) != 0) {
+       if (clock_control_off(clk, &pclken[0]) != 0) {
                LOG_ERR("Unable to disable USB clock");
                return -EIO;
        }

[mathieuchopstm]

Why should we remove const from pclken? Doing so unnecessarily increases the RAM footprint since the structure is never written to.

[avolmat-st]

@jfischer-no, the issue if we remove the (clock_control_subsys_t) is that we would be affecting a const to a non-const void * which will lead to a warning. Hence doing the (clock_control_subsys_t) cast.
It is very common to have the clock related information as const within the driver config structure since anyway those information are populated at build time by looking at the device-tree information.

Ex:

struct stm32_dcmipp_config {
        const struct stm32_pclken dcmipp_pclken;
        const struct stm32_pclken dcmipp_pclken_ker;
        irq_config_func_t irq_config;
        const struct pinctrl_dev_config *pctrl;
        const struct device *source_dev;
        const struct reset_dt_spec reset_dcmipp;
...

Unless I mistake, if we remove the whole cast, it would mean that we need to remove the const of the config structure as well.

[mathieuchopstm]

Yes, I understand that. My question may have been poorly worded, what I meant to say is "why should we remove const and move the pclken into RAM just to avoid a cast?"

[avolmat-st]

Sorry, my reply wasn't clear enough in fact. This was a reply targetting @jfischer-no about the proposal to remove const.

[avolmat-st]

@jfischer-no, could you clarify the point about my 2nd question ? As I wrote above, very very often (most of the case ?) the struct stm32_pclken is stored within the _config structure of the driver. As far as I understand, it is not only a matter of removing the const in front of the struct stm32_pclken within that structure but also removing the const in front of the whole _config struct otherwise this would still be a const pointer. (cf the example I have few comments above).

Regarding the fact that clock_control_subsys_t might be a nonsense, maybe yes, but shouldn't this be a treewide operation to do ?

[erwango]

And while we are on it, something like typedef void *clock_control_subsys_t; is nonsense.

@jfischer-no This question relates to the clock_control api definition, which is out of scope here.

So, as I can understand (but not made explicit despite the clarification questions), the complain here is only on cases where pclken is not in a struct and in which the proposed fix works.

Let's fix this case and move on.

[erwango]

Fixed now. @jfischer-no PTAL

[jfischer-no]

@jfischer-no, could you clarify the point about my 2nd question ? As I wrote above, very very often (most of the case ?) the struct stm32_pclken is stored within the _config structure of the driver.

Well it looks like it should be placed in driver's data instead. I guess in worst case the whole config would be placed in RAM (for example video_stm32_dcmi_config_0 is placed in RAM). However, I did not ask for these changes.

As far as I understand, it is not only a matter of removing the const in front of the struct stm32_pclken within that structure but also removing the const in front of the whole _config struct otherwise this would still be a const pointer. (cf the example I have few comments above).

I explicitly commented on the changes in drivers/usb/udc/udc_stm32.c. pclken is not a member of shim driver config. The "const pointer" does not do what you expect.

[erwango]

@jfischer-no Your comment has been addressed. Your fix has actually been taken in extenso, so if you want, you could claim co-authorship or sign-off, or if you have other comments on the scope of this PR, please do so, but can we move on, please ?

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[jfischer-no]

UDC changes LGTM. Perhaps someone has to move all the pclken to driver data as they are in no way placed in flash.