diff mbox

[v4,3/9] serial: Add common rs485 device tree parsing function

Message ID 20170718105948.21986-4-u.kleine-koenig@pengutronix.de
State New
Headers show

Commit Message

Uwe Kleine-König July 18, 2017, 10:59 a.m. UTC 
From: Sascha Hauer <s.hauer@pengutronix.de>

Several drivers have the same device tree parsing code. Create
a common helper function for it.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
[ukl: implement default <0 0> for rts-delay, unset unspecified flags]
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/Kconfig  |  4 ++++
 drivers/tty/serial/Makefile |  2 ++
 drivers/tty/serial/of.c     | 47 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/serial_core.h | 13 +++++++++++++
 4 files changed, 66 insertions(+)
 create mode 100644 drivers/tty/serial/of.c

Comments

Greg KH July 30, 2017, 2:49 p.m. UTC | #1 
On Tue, Jul 18, 2017 at 12:59:42PM +0200, Uwe Kleine-König wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Several drivers have the same device tree parsing code. Create
> a common helper function for it.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> [ukl: implement default <0 0> for rts-delay, unset unspecified flags]
> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  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?

> +/**
> + * 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.


> + */
> +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.

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?

thanks,

greg k-h
Uwe Kleine-König July 31, 2017, 7:42 a.m. UTC | #2 
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:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > Several drivers have the same device tree parsing code. Create
> > a common helper function for it.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > [ukl: implement default <0 0> for rts-delay, unset unspecified flags]
> > Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  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
 
> > +/**
> > + * 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.

> > + */
> > +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
diff mbox

Patch

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 1f096e2bb398..f7d7e4936940 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -14,6 +14,10 @@  config SERIAL_EARLYCON
 	  the console before standard serial driver is probed. The console is
 	  enabled when early_param is processed.
 
+config OF_SERIAL
+	depends on SERIAL_CORE
+	def_bool y
+
 source "drivers/tty/serial/8250/Kconfig"
 
 comment "Non-8250 serial port support"
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index fe88a75d9a59..a755faa25485 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -7,6 +7,8 @@  obj-$(CONFIG_SERIAL_CORE) += serial_core.o
 obj-$(CONFIG_SERIAL_EARLYCON) += earlycon.o
 obj-$(CONFIG_SERIAL_EARLYCON_ARM_SEMIHOST) += earlycon-arm-semihost.o
 
+obj-$(CONFIG_OF_SERIAL) += of.o
+
 # These Sparc drivers have to appear before others such as 8250
 # which share ttySx minor node space.  Otherwise console device
 # names change and other unplesantries.
diff --git a/drivers/tty/serial/of.c b/drivers/tty/serial/of.c
new file mode 100644
index 000000000000..2830c688bb5d
--- /dev/null
+++ b/drivers/tty/serial/of.c
@@ -0,0 +1,47 @@ 
+#include <linux/kernel.h>
+#include <linux/export.h>
+#include <linux/of.h>
+#include <linux/serial_core.h>
+
+/**
+ * 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.
+ */
+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;
+}
+#endif
+
 #endif /* LINUX_SERIAL_CORE_H */