pinmux: some fixes after review

Author: dani-clo

cores/arduino/zephyrCommon.cpp

@@ -25,14 +25,17 @@
 /*
  * Pinctrl configuration structures for dynamic pin switching.
  *
- * Map deferred-init peripherals with their pinctrl configuration from devicetree.
+ * Map deferred-init peripherals and zephyr,console (that is not deferred) with their
+ * pinctrl configuration from devicetree.
  */
-#define PINCTRL_NODE_DEFERRED(node_id) DT_PROP_OR(node_id, zephyr_deferred_init, 0)
+#define NODE_SELECTED(node_id)                                                                     \
+	UTIL_OR(DT_PROP(node_id, zephyr_deferred_init),                                                \
+			DT_SAME_NODE(node_id, DT_CHOSEN(zephyr_console)))
 
-#define PINCTRL_DEFINE_IF_PRESENT(node_id)                                                         \
-	COND_CODE_1(PINCTRL_NODE_DEFERRED(node_id), (PINCTRL_DT_DEFINE(node_id);), ())
+#define PINCTRL_DEFINE_IF_SELECTED(node_id)                                                       \
+	COND_CODE_1(NODE_SELECTED(node_id), (PINCTRL_DT_DEFINE(node_id);), ())
 
-DT_FOREACH_STATUS_OKAY_NODE(PINCTRL_DEFINE_IF_PRESENT)
+DT_FOREACH_STATUS_OKAY_NODE(PINCTRL_DEFINE_IF_SELECTED)
 
 struct pinctrl_map_entry {
 	const struct device *dev;
@@ -41,7 +44,7 @@ struct pinctrl_map_entry {
 
 #define PINCTRL_MAP_ENTRY(node_id) {DEVICE_DT_GET(node_id), PINCTRL_DT_DEV_CONFIG_GET(node_id)},
 #define PINCTRL_MAP_ENTRY_IF_PRESENT(node_id)                                                      \
-	COND_CODE_1(PINCTRL_NODE_DEFERRED(node_id), (PINCTRL_MAP_ENTRY(node_id)), ())
+	COND_CODE_1(NODE_SELECTED(node_id), (PINCTRL_MAP_ENTRY(node_id)), ())
 
 static const struct pinctrl_map_entry pinctrl_map[] = {
 	DT_FOREACH_STATUS_OKAY_NODE(PINCTRL_MAP_ENTRY_IF_PRESENT){NULL, NULL},
@@ -59,89 +62,89 @@ static const struct pinctrl_dev_config *get_known_pcfg(const struct device *dev)
 		}
 	}
 
-	return NULL;
+	return nullptr;
 }
 
 /**
- * @brief Acquire a single pin of a peripheral to ARDUINO state.
+ * @brief Initialize the peripheral and acquire a single pin to ARDUINO state.
  *
  * Switches peripheral pins back to ARDUINO pinctrl state (alternate function),
  * typically after a temporary transition to GPIO mode.
  *
- * @param idx Index of the pin within the device's ARDUINO pinctrl state
  * @param dev Pointer to the peripheral device
+ * @param state_pin_idx Index of the pin within the device's ARDUINO pinctrl state
  * @return 0 on success, negative on error
  */
-int apply_dev_channel_pinctrl(size_t state_pin_idx, const struct device *dev) {
-
-	const pinctrl_soc_pin_t *soc_pin = NULL;
-	const struct pinctrl_state *state;
+int init_dev_apply_channel_pinctrl(const struct device *dev, size_t state_pin_idx) {
 
-	if (dev == NULL || dev->config == NULL) {
+	if (dev == nullptr) {
 		return -EINVAL;
 	}
 
+	if (!device_is_ready(dev)) {
+		// init device for first usage, if not ready
+		int err = device_init(dev);
+		if (err < 0) {
+			return err;
+		}
+	}
+
+	const struct pinctrl_state *state;
 	const struct pinctrl_dev_config *pcfg = get_known_pcfg(dev);
 
-	if (pcfg == NULL) {
+	if (pcfg == nullptr) {
+		/* Device not in DT mapping - add to pinctrl_map if needed */
 		return -ENOTSUP;
 	}
 
 	int err = pinctrl_lookup_state(pcfg, PINCTRL_STATE_ARDUINO, &state);
 	if (err < 0) {
-		return err;
+		return err; /* Fails if the state is not defined in pinctrl-names */
 	}
 
+	/* bounds check */
 	if (state_pin_idx >= state->pin_cnt) {
 		return -ERANGE;
 	}
 
-	soc_pin = &state->pins[state_pin_idx];
-
 	/*
-	 * On platforms where CONFIG_PINCTRL_STORE_REG is enabled (e.g. nRF),
-	 * the peripheral's PSEL register must be written via the device base
-	 * address stored in pcfg->reg.
-	 * On platforms without CONFIG_PINCTRL_STORE_REG (e.g. STM32) the reg
-	 * argument is ignored by their pinctrl driver, so passing PINCTRL_REG_NONE is safe.
+	 * On platforms without CONFIG_PINCTRL_STORE_REG (e.g. STM32) the pcfg->reg is not present but
+	 * the argument is ignored by their pinctrl driver, so passing PINCTRL_REG_NONE is safe.
 	 */
 #ifdef CONFIG_PINCTRL_STORE_REG
-	err = pinctrl_configure_pins(soc_pin, 1, pcfg->reg);
+	uintptr_t reg = pcfg->reg;
 #else
-	err = pinctrl_configure_pins(soc_pin, 1, PINCTRL_REG_NONE);
+	uintptr_t reg = PINCTRL_REG_NONE;
 #endif
-	if (err < 0) {
-		return err;
-	}
 
-	return 0;
+	return pinctrl_configure_pins(&state->pins[state_pin_idx], 1, reg);
 }
 
 /**
- * @brief Optimize peripheral transitions using pinctrl state switching.
+ * @brief Optimize peripheral transitions applying pinctrl state PINCTRL_STATE_DEFAULT.
  *
  * @param dev Target peripheral device to acquire pin for
- * @param target_state Desired pinctrl state (typically PINCTRL_STATE_DEFAULT or SLEEP)
  */
-int apply_dev_pinctrl(const struct device *dev, uint8_t target_state) {
+int init_dev_apply_pinctrl(const struct device *dev) {
 
-	if (dev == NULL || dev->config == NULL) {
+	if (dev == nullptr) {
 		return -EINVAL;
 	}
 
-	const struct pinctrl_dev_config *pcfg = get_known_pcfg(dev);
-	if (pcfg == NULL) {
-		/* Device not in DT mapping - add to get_known_pcfg() if needed */
-		return -ENOTSUP;
+	if (!device_is_ready(dev)) {
+		int ret = device_init(dev);
+		if (ret != 0 && ret != -EALREADY) {
+			return ret;
+		}
 	}
 
-	int ret = pinctrl_apply_state(pcfg, target_state);
-
-	if (ret < 0 && ret != -ENOENT) {
-		return ret;
+	const struct pinctrl_dev_config *pcfg = get_known_pcfg(dev);
+	if (pcfg == nullptr) {
+		/* Device not in DT mapping - add to pinctrl_map if needed */
+		return -ENOTSUP;
 	}
 
-	return 0;
+	return pinctrl_apply_state(pcfg, PINCTRL_STATE_DEFAULT);
 }
 
 static const struct gpio_dt_spec arduino_pins[] = {
@@ -624,14 +627,15 @@ void analogWrite(pin_size_t pinNumber, int value) {
 		return;
 	}
 
+	(void)init_dev_apply_channel_pinctrl(arduino_pwm[idx].dev,
+										 state_pin_index_from_spec_index(arduino_pwm, idx));
+
 	if (!pwm_is_ready_dt(&arduino_pwm[idx])) {
 		pinMode(pinNumber, OUTPUT);
 		digitalWrite(pinNumber, value > 127 ? HIGH : LOW);
 		return;
 	}
 
-	(void)apply_dev_channel_pinctrl(state_pin_index_from_spec_index(arduino_pwm, idx),
-									arduino_pwm[idx].dev);
 	value = CLAMP(value, 0, maxInput);
 
 	const uint32_t pulse = map64(value, 0, maxInput, 0, arduino_pwm[idx].period);
@@ -657,7 +661,7 @@ void analogWrite(enum dacPins dacName, int value) {
 
 	// TODO: add reverse map to find pin name from DAC* define
 	// In the meantime, consider A0 == DAC0
-	apply_dev_pinctrl(dac_dev, PINCTRL_STATE_DEFAULT);
+	(void)init_dev_apply_pinctrl(dac_dev);
 
 	ret = dac_channel_setup(dac_dev, &dac_ch_cfg[dacName]);
 	if (ret != 0) {
@@ -711,14 +715,6 @@ int analogRead(pin_size_t pinNumber) {
 		return -EINVAL;
 	}
 
-	if (!device_is_ready(arduino_adc[idx].dev)) {
-		// init device for first acquisition attempt, if not ready
-		err = device_init(arduino_adc[idx].dev);
-		if (err < 0) {
-			return err;
-		}
-	}
-
 	/*
 	 * ADC that is on MCU supported by Zephyr exists
 	 * only 16bit resolution, currently.
@@ -728,12 +724,13 @@ int analogRead(pin_size_t pinNumber) {
 	}
 
 	/*
-	 * Restore only this ADC pin to analog mode when transitioning from GPIO.
-	 * The pin is selected from the ADC device "arduino" pinctrl state.
-	 * Not checking the return value because the device might not have pinctrl (e.g. nRF SAADC).
+	 * Init the ADC device for the first acquisition and restore only the required pin to analog
+	 * mode when transitioning from GPIO. The pin is selected from the ADC device "arduino" pinctrl
+	 * state. Not checking the return value because the device might not have pinctrl (e.g. nRF
+	 * SAADC).
 	 */
-	(void)apply_dev_channel_pinctrl(state_pin_index_from_spec_index(arduino_adc, idx),
-									arduino_adc[idx].dev);
+	(void)init_dev_apply_channel_pinctrl(arduino_adc[idx].dev,
+										 state_pin_index_from_spec_index(arduino_adc, idx));
 
 	err = adc_channel_setup(arduino_adc[idx].dev, &arduino_adc[idx].channel_cfg);
 	if (err < 0) {

cores/arduino/zephyrInternal.h

@@ -15,7 +15,7 @@ extern "C" {
 void enableInterrupt(pin_size_t);
 void disableInterrupt(pin_size_t);
 
-int apply_dev_pinctrl(const struct device *dev, uint8_t target_state); 
+int init_dev_apply_pinctrl(const struct device *dev);
 
 #ifdef __cplusplus
 } // extern "C"

cores/arduino/zephyrSerial.cpp

@@ -5,10 +5,9 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
- #include "zephyrInternal.h"
- 
+#include "zephyrInternal.h"
+
 #include <zephyr/devicetree.h>
-#include <zephyr/drivers/pinctrl.h>
 #include <zephyr/drivers/uart.h>
 
 #include <api/HardwareSerial.h>
@@ -63,17 +62,10 @@ void arduino::ZephyrSerial::begin(unsigned long baud, uint16_t conf) {
 		.flow_ctrl = UART_CFG_FLOW_CTRL_NONE,
 	};
 
-	if (!device_is_ready(uart)) {
-		int ret = device_init(uart);
-		if (ret != 0 && ret != -EALREADY) {
-			return;
-		}
-	}
-
 	/* Re-apply DEFAULT pinctrl state so shared pins
-	 * are remuxed back to Serial after temporary GPIO/PWM usage.
+	 * are remuxed back to Serial after other peripherals have used them.
 	 */
-	(void)apply_dev_pinctrl(uart, PINCTRL_STATE_DEFAULT);
+	(void)init_dev_apply_pinctrl(uart);
 
 	uart_configure(uart, &config);
 	uart_irq_callback_user_data_set(uart, arduino::ZephyrSerial::IrqDispatch, this);