From 960abcdabbde4ddf90ffad8d0dd97a171e7887ba Mon Sep 17 00:00:00 2001
From: Giovanni Di Sirio <gdisirio@gmail.com>
Date: Sat, 17 Jun 2023 10:21:21 +0000
Subject: [PATCH] Improved general drivers state machine handling.

git-svn-id: svn://svn.code.sf.net/p/chibios/svn/trunk@16291 27425a3e-05d8-49a3-a47f-9c15f0e5edd8
---
 os/xhal/codegen/hal_base_driver.xml | 71 +++++++++++++++++++++--------
 os/xhal/codegen/hal_spi.xml         |  3 +-
 os/xhal/include/hal.h               | 16 +++++--
 os/xhal/src/hal_base_driver.c       | 43 ++++++++++++-----
 os/xhal/src/hal_spi.c               |  5 ++
 5 files changed, 100 insertions(+), 38 deletions(-)

diff --git a/os/xhal/codegen/hal_base_driver.xml b/os/xhal/codegen/hal_base_driver.xml
index 074d9147a..e6f7cca72 100644
--- a/os/xhal/codegen/hal_base_driver.xml
+++ b/os/xhal/codegen/hal_base_driver.xml
@@ -116,7 +116,8 @@ drv_reg_remove(self);
               <brief>Low level driver stop.</brief>
               <notapi />
             </method>
-            <method name="drvConfigureX" shortname="configure" ctype="msg_t">
+            <method name="drvConfigureX" shortname="configure"
+              ctype="msg_t">
               <brief>Driver configure.</brief>
               <details><![CDATA[Applies a new configuration to the driver. The
                 configuration structure is architecture-dependent.]]>
@@ -126,7 +127,8 @@ drv_reg_remove(self);
                 depending on the driver implementation and current state.]]>
               </note>
               <param ctype="const void *" name="config" dir="in">
-                New driver configuration.
+                New driver
+                configuration.
               </param>
               <api />
             </method>
@@ -138,8 +140,23 @@ drv_reg_remove(self);
                 call the peripheral is physically initialized using a
                 default configuration, subsequent calls are ignored.]]>
               </details>
+              <note><![CDATA[The function can fail with error
+                @p HAL_RET_INV_STATE if called while the driver is already
+                being started or stopped. In case you need multiple threads
+                to perform start and stop operation on the driver then it is
+                suggested to lock/unlock the driver during such operations.]]></note>
               <return>The operation status.</return>
               <retval value="HAL_RET_SUCCESS">Operation successful.</retval>
+              <retval value="HAL_RET_INV_STATE"><![CDATA[If the driver was
+                in one of @p HAL_DRV_STATE_UNINIT, @p HAL_DRV_STATE_STARTING
+                or @p HAL_DRV_STATE_STOPPING states.]]></retval>
+              <retval value="HAL_RET_NO_RESOURCE">If a required resources
+                cannot be allocated.
+              </retval>
+              <retval value="HAL_RET_HW_BUSY">If a required HW resource is
+                already in use.
+              </retval>
+              <retval value="HAL_RET_HW_FAILURE">If an HW failure occurred.</retval>
               <api />
               <implementation><![CDATA[
 msg_t msg = HAL_RET_SUCCESS;
@@ -148,20 +165,28 @@ osalDbgCheck(self != NULL);
 
 osalSysLock();
 
-osalDbgAssert((self->state != HAL_DRV_STATE_UNINIT) &&
-              (self->state != HAL_DRV_STATE_STOPPING) &&
-              (self->state != HAL_DRV_STATE_STARTING),
-              "invalid state");
-
-if (self->state == HAL_DRV_STATE_STOP) {
+switch (self->state) {
+case HAL_DRV_STATE_UNINIT:
+  /* Falls through.*/
+case HAL_DRV_STATE_STARTING:
+  /* Falls through.*/
+case HAL_DRV_STATE_STOPPING:
+  msg = HAL_RET_INV_STATE;
+  break;
+case HAL_DRV_STATE_STOP:
   /* Physically starting the peripheral.*/
   msg = __drv_start(self);
   if (msg == HAL_RET_SUCCESS) {
     self->state = HAL_DRV_STATE_READY;
+
+    /* LLD is supposed to set a default configuration.*/
+    osalDbgAssert(self->config != NULL, "no configuration");
   }
   else {
     self->state = HAL_DRV_STATE_STOP;
-  }  
+  }
+default:
+  /* Any other state ignored, driver already started.*/
 }
 
 osalSysUnlock();
@@ -180,13 +205,13 @@ osalDbgCheck(self != NULL);
 
 osalSysLock();
 
-osalDbgAssert((self->state != HAL_DRV_STATE_UNINIT) &&
-              (self->state != HAL_DRV_STATE_STARTING),
-              "invalid state");
+osalDbgAssert(self->state != HAL_DRV_STATE_UNINIT, "invalid state");
 
-if (self->state != HAL_DRV_STATE_STOP) {
+if ((self->state != HAL_DRV_STATE_STOP) &&
+    (self->state != HAL_DRV_STATE_STARTING)) {
   __drv_stop(self);
-  self->state = HAL_DRV_STATE_STOP;
+  self->state  = HAL_DRV_STATE_STOP;
+  self->config = NULL;
 }
 
 osalSysUnlock();]]></implementation>
@@ -204,7 +229,8 @@ return self->state;]]></implementation>
             <method name="drvSetStateX" ctype="void">
               <brief>Driver state set.</brief>
               <param ctype="driver_state_t" name="state" dir="in">
-                New driver state.
+                New driver
+                state.
               </param>
               <api />
               <implementation><![CDATA[
@@ -319,7 +345,8 @@ return oopGetOwner(hal_base_driver_c, regent, rep);]]></implementation>
             This function is only available when HAL registry is enabled.
           </note>
           <param name="drvp" ctype="hal_base_driver_c *" dir="in">
-            Previously found driver.
+            Previously
+            found driver.
           </param>
           <return>A pointer to the next driver object.</return>
           <retval value="NULL">If there is no next driver.</retval>
@@ -346,8 +373,10 @@ return oopGetOwner(hal_base_driver_c, regent, rep);]]></implementation>
           </note>
           <param name="name" ctype="const char *" dir="in">Driver name.</param>
           <param name="msgp" ctype="msg_t *" dir="out">
-            Pointer to store the error code or @p NULL. Note that in case of
-            driver not found the returned code is @p HAL_RET_SUCCESS.
+            Pointer to store the
+            error code or @p NULL. Note that in case of
+            driver not found the
+            returned code is @p HAL_RET_SUCCESS.
           </param>
           <return>A reference to the driver.</return>
           <retval value="NULL">If an error occurred.</retval>
@@ -395,7 +424,8 @@ return drvp;]]></implementation>
             This function is only available when HAL registry is enabled.
           </note>
           <param name="drvp" ctype="hal_base_driver_c *" dir="both">
-            Pointer to the @p hal_base_driver_c instance to be inserted.
+            Pointer to
+            the @p hal_base_driver_c instance to be inserted.
           </param>
           <implementation><![CDATA[
 
@@ -407,7 +437,8 @@ hal_registry.prev       = &drvp->regent;]]></implementation>
         <function name="drv_reg_remove" ctype="void">
           <brief>Driver removal from the HAL registry.</brief>
           <param name="drvp" ctype="hal_base_driver_c *" dir="both">
-            Pointer to the @p hal_base_driver_c instance to be inserted.
+            Pointer to
+            the @p hal_base_driver_c instance to be inserted.
           </param>
           <implementation><![CDATA[
 
diff --git a/os/xhal/codegen/hal_spi.xml b/os/xhal/codegen/hal_spi.xml
index 2ef7bdea3..98a3b225d 100644
--- a/os/xhal/codegen/hal_spi.xml
+++ b/os/xhal/codegen/hal_spi.xml
@@ -851,7 +851,8 @@ spi_lld_stop(self);]]></implementation>
             <method shortname="configure">
               <implementation><![CDATA[
 
-self->config = config;]]></implementation>
+self->config = config;
+spi_lld_configure(self);]]></implementation>
             </method>
             <method shortname="setcb">
               <implementation><![CDATA[
diff --git a/os/xhal/include/hal.h b/os/xhal/include/hal.h
index d348e9459..17d577adb 100644
--- a/os/xhal/include/hal.h
+++ b/os/xhal/include/hal.h
@@ -91,32 +91,38 @@
  * @{
  */
 #define HAL_RET_SUCCESS         MSG_OK
+/**
+ * @brief   Operation requested during invalid driver state.
+ * @details This error is returned if the driver cannot accept the request
+ *          because its internal state.
+ */
+#define HAL_RET_INV_STATE       (msg_t)-16
 /**
  * @brief   Configuration error.
  * @details An error has been detected in the driver configuration structure.
  */
-#define HAL_RET_CONFIG_ERROR    (msg_t)-16
+#define HAL_RET_CONFIG_ERROR    (msg_t)-17
 /**
  * @brief   A required resource is not available.
  * @details One of the resources required for driver operations is not
  *          available.
  */
-#define HAL_RET_NO_RESOURCE     (msg_t)-17
+#define HAL_RET_NO_RESOURCE     (msg_t)-18
 /**
  * @brief   The peripheral is busy.
  * @details The peripheral is not available or taken by some other system
  *          actor.
  */
-#define HAL_RET_HW_BUSY         (msg_t)-18
+#define HAL_RET_HW_BUSY         (msg_t)-19
 /**
  * @brief   Peripheral failure.
  * @details Peripheral failed, for example HW timeouts.
  */
-#define HAL_RET_HW_FAILURE      (msg_t)-19
+#define HAL_RET_HW_FAILURE      (msg_t)-20
 /**
  * @brief   Unknown control code.
  */
-#define HAL_RET_UNKNOWN_CTL     (msg_t)-20
+#define HAL_RET_UNKNOWN_CTL     (msg_t)-21
 /** @} */
 
 /*===========================================================================*/
diff --git a/os/xhal/src/hal_base_driver.c b/os/xhal/src/hal_base_driver.c
index 3d273f03a..f9f3a767f 100644
--- a/os/xhal/src/hal_base_driver.c
+++ b/os/xhal/src/hal_base_driver.c
@@ -266,10 +266,21 @@ void __drv_dispose_impl(void *ip) {
  * @details     Starts driver operations, on the 1st call the peripheral is
  *              physically initialized using a default configuration,
  *              subsequent calls are ignored.
+ * @note        The function can fail with error @p HAL_RET_INV_STATE if called
+ *              while the driver is already being started or stopped. In case
+ *              you need multiple threads performing start and stop operation
+ *              on the driver then it is suggested to lock/unlock the driver
+ *              during such operations.
  *
  * @param[in,out] ip            Pointer to a @p hal_base_driver_c instance.
  * @return                      The operation status.
  * @retval HAL_RET_SUCCESS      Operation successful.
+ * @retval HAL_RET_INV_STATE    If the driver was in one of @p
+ *                              HAL_DRV_STATE_UNINIT, @p HAL_DRV_STATE_STARTING
+ *                              or @p HAL_DRV_STATE_STOPPING states.
+ * @retval HAL_RET_NO_RESOURCE  If a required resources cannot be allocated.
+ * @retval HAL_RET_HW_BUSY      If a required HW resource is already in use.
+ * @retval HAL_RET_HW_FAILURE   If an HW failure occurred.
  *
  * @api
  */
@@ -281,20 +292,28 @@ msg_t drvStart(void *ip) {
 
   osalSysLock();
 
-  osalDbgAssert((self->state != HAL_DRV_STATE_UNINIT) &&
-                (self->state != HAL_DRV_STATE_STOPPING) &&
-                (self->state != HAL_DRV_STATE_STARTING),
-                "invalid state");
-
-  if (self->state == HAL_DRV_STATE_STOP) {
+  switch (self->state) {
+  case HAL_DRV_STATE_UNINIT:
+    /* Falls through.*/
+  case HAL_DRV_STATE_STARTING:
+    /* Falls through.*/
+  case HAL_DRV_STATE_STOPPING:
+    msg = HAL_RET_INV_STATE;
+    break;
+  case HAL_DRV_STATE_STOP:
     /* Physically starting the peripheral.*/
     msg = __drv_start(self);
     if (msg == HAL_RET_SUCCESS) {
       self->state = HAL_DRV_STATE_READY;
+
+      /* LLD is supposed to set a default configuration.*/
+      osalDbgAssert(self->config != NULL, "no configuration");
     }
     else {
       self->state = HAL_DRV_STATE_STOP;
-    }  
+    }
+  default:
+    /* Any other state ignored, driver already started.*/
   }
 
   osalSysUnlock();
@@ -321,13 +340,13 @@ void drvStop(void *ip) {
 
   osalSysLock();
 
-  osalDbgAssert((self->state != HAL_DRV_STATE_UNINIT) &&
-                (self->state != HAL_DRV_STATE_STARTING),
-                "invalid state");
+  osalDbgAssert(self->state != HAL_DRV_STATE_UNINIT, "invalid state");
 
-  if (self->state != HAL_DRV_STATE_STOP) {
+  if ((self->state != HAL_DRV_STATE_STOP) &&
+      (self->state != HAL_DRV_STATE_STARTING)) {
     __drv_stop(self);
-    self->state = HAL_DRV_STATE_STOP;
+    self->state  = HAL_DRV_STATE_STOP;
+    self->config = NULL;
   }
 
   osalSysUnlock();
diff --git a/os/xhal/src/hal_spi.c b/os/xhal/src/hal_spi.c
index 964d7818b..46b123549 100644
--- a/os/xhal/src/hal_spi.c
+++ b/os/xhal/src/hal_spi.c
@@ -134,6 +134,8 @@ void __spi_dispose_impl(void *ip) {
  */
 msg_t __spi_start_impl(void *ip) {
   hal_spi_driver_c *self = (hal_spi_driver_c *)ip;
+
+  return spi_lld_start(self);
 }
 
 /**
@@ -146,6 +148,8 @@ msg_t __spi_start_impl(void *ip) {
  */
 void __spi_stop_impl(void *ip) {
   hal_spi_driver_c *self = (hal_spi_driver_c *)ip;
+
+  spi_lld_stop(self);
 }
 
 /**
@@ -161,6 +165,7 @@ msg_t __spi_configure_impl(void *ip, const void *config) {
   hal_spi_driver_c *self = (hal_spi_driver_c *)ip;
 
   self->config = config;
+  spi_lld_configure(self);
 }
 
 /**