On Sun, Jul 30, 2017 at 07:49:47AM -0700, Greg KH wrote:
On Tue, Jul 18, 2017 at 12:59:42PM +0200, Uwe Kleine-König wrote:
quoted
From: Sascha Hauer <redacted>
Several drivers have the same device tree parsing code. Create
a common helper function for it.
Signed-off-by: Sascha Hauer <redacted>
[ukl: implement default <0 0> for rts-delay, unset unspecified flags]
Acked-by: Nicolas Ferre <redacted>
Signed-off-by: Uwe Kleine-König <redacted>
---
drivers/tty/serial/Kconfig | 4 ++++
drivers/tty/serial/Makefile | 2 ++
drivers/tty/serial/of.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
Why is this a separate file, with a Kconfig option that is always
enabled? Why not just add the single function to the serial core?
Probably just a matter of taste. If you prefer I can put it into
drivers/tty/serial/serial_core.c
quoted
+/**
+ * of_get_rs485_mode() - Implement parsing rs485 properties
+ * @np: uart node
+ * @rs485conf: output parameter
+ *
+ * This function implements the device tree binding described in
+ * Documentation/devicetree/bindings/serial/rs485.txt.
+ *
+ * Return: 0 on success, 1 if the node doesn't contain rs485 stuff.
1 is not an error code :(
Return a proper error please.
Well, it's not an error. These properties are optional and if I return
(say) -ENOENT in this case, a caller looks as follows:
ret = of_get_rs485_mode(...);
if (ret == -ENOENT) {
/*
* hmm, does this mean there are no rs485 properties, or
* is this a different error that I should propagate
* because there is a problem with the device tree?
* I'm optimistic and assume the former.
* As of_get_rs485_mode already did everything
* necessary, there is no need to do anything else.
*/
} else if (ret < 0) {
return ret;
}
With the semantic I chose it is just:
ret = of_get_rs485_mode(...);
if (ret < 0)
return ret;
without any guessing.
quoted
+ */
+int of_get_rs485_mode(struct device_node *np, struct serial_rs485 *rs485conf)
+{
+ u32 rs485_delay[2];
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_OF) || !np)
+ return 1;
+
+ ret = of_property_read_u32_array(np, "rs485-rts-delay", rs485_delay, 2);
+ if (!ret) {
+ rs485conf->delay_rts_before_send = rs485_delay[0];
+ rs485conf->delay_rts_after_send = rs485_delay[1];
+ } else {
+ rs485conf->delay_rts_before_send = 0;
+ rs485conf->delay_rts_after_send = 0;
+ }
+
+ /*
+ * clear full-duplex and enabled flags to get to a defined state with
+ * the two following properties.
+ */
+ rs485conf->flags &= ~(SER_RS485_RX_DURING_TX | SER_RS485_ENABLED);
+
+ if (of_property_read_bool(np, "rs485-rx-during-tx"))
+ rs485conf->flags |= SER_RS485_RX_DURING_TX;
+
+ if (of_property_read_bool(np, "linux,rs485-enabled-at-boot-time"))
+ rs485conf->flags |= SER_RS485_ENABLED;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_rs485_mode);diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 1775500294bb..3a89e8925722 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -501,4 +501,17 @@ static inline int uart_handle_break(struct uart_port *port)
(cflag) & CRTSCTS || \
!((cflag) & CLOCAL))
+/*
+ * Common device tree parsing helpers
+ */
+#ifdef CONFIG_OF_SERIAL
+int of_get_rs485_mode(struct device_node *np, struct serial_rs485 *rs485conf);
+#else
+static inline int of_get_rs485_mode(struct device_node *np,
+ struct serial_rs485 *rs485conf)
+{
+ return 1;
Again, a real error.
OK, I could agree to return -ENOSYS here. I predict I get complaints
about this though and people will do:
ret = of_get_rs485_mode(...);
if (ret == -ENOSYS) {
/* ignore */
} else if (ret < 0) {
return ret;
}
which one might consider troublesome or even wrong. (See the same
discussions about 22c403676dbbb7c6f186099527af7f065498ef45 where I
wanted to keep -ENOSYS.)
And why not have a "generic" firmware function for this that defaults to
the of_ for that platform type if present? What's the odds that acpi
will need/want this soon anyway?
I don't know about ACPI and all systems I care about are using dt. So
I'd suggest we stick to dt and switch to "generic" if and when the need
arises?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html