| Message ID | 20170621102130.21024-2-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 21, 2017 at 12:21:22PM +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> > 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/serial_core.h | 12 ++++++++++++ > 4 files changed, 63 insertions(+) > create mode 100644 drivers/tty/serial/of.c > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index 5c8850f7a2a0..8baef5b95bed 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 This probably also needs a dependency on some device tree option, CONFIG_OF maybe? Sascha
On Wed, Jun 21, 2017 at 12:21:22PM +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> > 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/serial_core.h | 12 ++++++++++++ > 4 files changed, 63 insertions(+) > create mode 100644 drivers/tty/serial/of.c > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index 5c8850f7a2a0..8baef5b95bed 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 53c03e005132..0fee8f4e36cb 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..660a8cb09b44 > --- /dev/null > +++ b/drivers/tty/serial/of.c > @@ -0,0 +1,45 @@ > +#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, or a > + * negative error code. > + */ > +int of_get_rs485_mode(struct device_node *np, struct serial_rs485 *rs485conf) > +{ > + u32 rs485_delay[2]; > + int ret; > + > + ret = of_property_read_u32_array(np, "rs485-rts-delay" rs485_delay, 2); > + if (ret == -EINVAL) /* property does not exist */ > + return 1; As the de-facto standard for the drivers implementing the rs485 bindings is to make the properties optional, despite the documentation. Wouldn't it be better to make this property optional in the binding instead? This wouldn't unnecessarily break old device trees and 0/0 seems a sane default we can use when the property doesn't exist. Sascha
On Thu, Jun 22, 2017 at 08:10:07AM +0200, Sascha Hauer wrote: > On Wed, Jun 21, 2017 at 12:21:22PM +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> > > 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/serial_core.h | 12 ++++++++++++ > > 4 files changed, 63 insertions(+) > > create mode 100644 drivers/tty/serial/of.c > > > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > > index 5c8850f7a2a0..8baef5b95bed 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 > > This probably also needs a dependency on some device tree option, > CONFIG_OF maybe? Without CONFIG_OF of_property_read_u32_array returns -ENOSYS and of_property_read_bool returns false. So I think (but didn't test) there is no problem not depending on more stuff. But maybe do: if (!np) return 1; in of_get_rs485_mode? Best regards Uwe
Hello Sascha, On Thu, Jun 22, 2017 at 08:31:57AM +0200, Sascha Hauer wrote: > On Wed, Jun 21, 2017 at 12:21:22PM +0200, Uwe Kleine-König wrote: > > +/** > > + * 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, or a > > + * negative error code. > > + */ > > +int of_get_rs485_mode(struct device_node *np, struct serial_rs485 *rs485conf) > > +{ > > + u32 rs485_delay[2]; > > + int ret; > > + > > + ret = of_property_read_u32_array(np, "rs485-rts-delay" rs485_delay, 2); > > + if (ret == -EINVAL) /* property does not exist */ > > + return 1; > > As the de-facto standard for the drivers implementing the rs485 bindings > is to make the properties optional, despite the documentation. Wouldn't I must admit that I failed to mention in the commit log that being strict here was added by me even though I'm not the author. > it be better to make this property optional in the binding instead? This > wouldn't unnecessarily break old device trees and 0/0 seems a sane > default we can use when the property doesn't exist. Would be fine for me, too. It was Nicolas Ferre who specified this as being required in 0331bbf3c6fd9. Added people who discussed the patch back then (but without questioning rs485-rts-delay being mandatory). Best regards Uwe
On 22/06/2017 at 10:22, Uwe Kleine-König wrote: > Hello Sascha, > > On Thu, Jun 22, 2017 at 08:31:57AM +0200, Sascha Hauer wrote: >> On Wed, Jun 21, 2017 at 12:21:22PM +0200, Uwe Kleine-König wrote: >>> +/** >>> + * 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, or a >>> + * negative error code. >>> + */ >>> +int of_get_rs485_mode(struct device_node *np, struct serial_rs485 *rs485conf) >>> +{ >>> + u32 rs485_delay[2]; >>> + int ret; >>> + >>> + ret = of_property_read_u32_array(np, "rs485-rts-delay" rs485_delay, 2); >>> + if (ret == -EINVAL) /* property does not exist */ >>> + return 1; >> >> As the de-facto standard for the drivers implementing the rs485 bindings >> is to make the properties optional, despite the documentation. Wouldn't > > I must admit that I failed to mention in the commit log that being > strict here was added by me even though I'm not the author. > >> it be better to make this property optional in the binding instead? This >> wouldn't unnecessarily break old device trees and 0/0 seems a sane >> default we can use when the property doesn't exist. > > Would be fine for me, too. It was Nicolas Ferre who specified this as > being required in 0331bbf3c6fd9. Added people who discussed the patch > back then (but without questioning rs485-rts-delay being mandatory). Uwe, This would be find with me too. Indeed, the conversion to an optional option won't break the atmel driver. Thanks for your work on this topic. Best regards,
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig index 5c8850f7a2a0..8baef5b95bed 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 53c03e005132..0fee8f4e36cb 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..660a8cb09b44 --- /dev/null +++ b/drivers/tty/serial/of.c @@ -0,0 +1,45 @@ +#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, or a + * negative error code. + */ +int of_get_rs485_mode(struct device_node *np, struct serial_rs485 *rs485conf) +{ + u32 rs485_delay[2]; + int ret; + + ret = of_property_read_u32_array(np, "rs485-rts-delay" rs485_delay, 2); + if (ret == -EINVAL) /* property does not exist */ + return 1; + if (ret) /* property does not have a value or is smaller than two u32 */ + return ret; + + rs485conf->delay_rts_before_send = rs485_delay[0]; + rs485conf->delay_rts_after_send = rs485_delay[1]; + + /* + * 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 64d892f1e5cd..9a6055191d22 100644 --- a/include/linux/serial_core.h +++ b/include/linux/serial_core.h @@ -500,4 +500,16 @@ static inline int uart_handle_break(struct uart_port *port) (cflag) & CRTSCTS || \ !((cflag) & CLOCAL)) +/* + * Common device tree parsing helpers + */ +#ifdef CONFIG_OF_SERIAL +void of_get_rs485_mode(struct device_node *np, struct serial_rs485 *rs485conf); +#else +static inline void of_get_rs485_mode(struct device_node *np, + struct serial_rs485 *rs485conf) +{ +} +#endif + #endif /* LINUX_SERIAL_CORE_H */