diff mbox series

[v2,03/14] serial: port: Introduce a common helper to read properties

Message ID 20240226142514.1485246-4-andriy.shevchenko@linux.intel.com
State Superseded
Headers show
Series serial: Add a helper to parse device properties and more | expand

Commit Message

Andy Shevchenko Feb. 26, 2024, 2:19 p.m. UTC
Several serial drivers want to read the same or similar set of
the port properties. Make a common helper for them.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/serial_port.c | 134 +++++++++++++++++++++++++++++++
 include/linux/serial_core.h      |   1 +
 2 files changed, 135 insertions(+)

Comments

Greg Kroah-Hartman March 2, 2024, 8:58 p.m. UTC | #1
On Mon, Feb 26, 2024 at 04:19:19PM +0200, Andy Shevchenko wrote:
> Several serial drivers want to read the same or similar set of
> the port properties. Make a common helper for them.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/serial_port.c | 134 +++++++++++++++++++++++++++++++
>  include/linux/serial_core.h      |   1 +
>  2 files changed, 135 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
> index 88975a4df306..ecc980e9dba6 100644
> --- a/drivers/tty/serial/serial_port.c
> +++ b/drivers/tty/serial/serial_port.c
> @@ -8,7 +8,10 @@
>  
>  #include <linux/device.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/property.h>
>  #include <linux/serial_core.h>
>  #include <linux/spinlock.h>
>  
> @@ -82,6 +85,137 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *port)
>  }
>  EXPORT_SYMBOL(uart_remove_one_port);
>  
> +/**
> + * uart_read_port_properties - read firmware properties of the given UART port

I like, but:

> + * @port: corresponding port
> + * @use_defaults: apply defaults (when %true) or validate the values (when %false)

Using random booleans in a function is horrid.  Every time you see the
function call, or want to call it, you need to go and look up what the
boolean is and means.

Make 2 public functions here, one that does it with use_defaults=true
and one =false and then have them both call this one static function,
that way the function names themselves are easy to read and understand
and maintain over time.

thanks,

greg k-h


> + *
> + * The following device properties are supported:
> + *   - clock-frequency (optional)
> + *   - fifo-size (optional)
> + *   - no-loopback-test (optional)
> + *   - reg-shift (defaults may apply)
> + *   - reg-offset (value may be validated)
> + *   - reg-io-width (defaults may apply or value may be validated)
> + *   - interrupts (OF only)
> + *   - serial [alias ID] (OF only)
> + *
> + * If the port->dev is of struct platform_device type the interrupt line
> + * will be retrieved via platform_get_irq() call against that device.
> + * Otherwise it will be assigned by fwnode_irq_get() call. In both cases
> + * the index 0 of the resource is used.
> + *
> + * The caller is responsible to initialize the following fields of the @port
> + *   ->dev (must be valid)
> + *   ->flags
> + *   ->mapbase
> + *   ->mapsize
> + *   ->regshift (if @use_defaults is false)
> + * before calling this function. Alternatively the above mentioned fields
> + * may be zeroed, in such case the only ones, that have associated properties
> + * found, will be set to the respective values.
> + *
> + * If no error happened, the ->irq, ->mapbase, ->mapsize will be altered.
> + * The ->iotype is always altered.
> + *
> + * When @use_defaults is true and the respective property is not found
> + * the following values will be applied:
> + *   ->regshift = 0
> + * In this case IRQ must be provided, otherwise an error will be returned.
> + *
> + * When @use_defaults is false and the respective property is found
> + * the following values will be validated:
> + *   - reg-io-width (->iotype)
> + *   - reg-offset (->mapsize against ->mapbase)
> + *
> + * Returns: 0 on success or negative errno on failure
> + */
> +int uart_read_port_properties(struct uart_port *port, bool use_defaults)
> +{
> +	struct device *dev = port->dev;
> +	u32 value;
> +	int ret;
> +
> +	/* Read optional UART functional clock frequency */
> +	device_property_read_u32(dev, "clock-frequency", &port->uartclk);
> +
> +	/* Read the registers alignment (default: 8-bit) */
> +	ret = device_property_read_u32(dev, "reg-shift", &value);
> +	if (ret)
> +		port->regshift = use_defaults ? 0 : port->regshift;
> +	else
> +		port->regshift = value;
> +
> +	/* Read the registers I/O access type (default: MMIO 8-bit) */
> +	ret = device_property_read_u32(dev, "reg-io-width", &value);
> +	if (ret) {
> +		port->iotype = UPIO_MEM;
> +	} else {
> +		switch (value) {
> +		case 1:
> +			port->iotype = UPIO_MEM;
> +			break;
> +		case 2:
> +			port->iotype = UPIO_MEM16;
> +			break;
> +		case 4:
> +			port->iotype = device_is_big_endian(dev) ? UPIO_MEM32BE : UPIO_MEM32;
> +			break;
> +		default:
> +			if (!use_defaults) {
> +				dev_err(dev, "Unsupported reg-io-width (%u)\n", value);
> +				return -EINVAL;
> +			}
> +			port->iotype = UPIO_UNKNOWN;
> +			break;
> +		}
> +	}
> +
> +	/* Read the address mapping base offset (default: no offset) */
> +	ret = device_property_read_u32(dev, "reg-offset", &value);
> +	if (ret)
> +		value = 0;
> +
> +	/* Check for shifted address mapping overflow */
> +	if (!use_defaults && port->mapsize < value) {
> +		dev_err(dev, "reg-offset %u exceeds region size %pa\n", value, &port->mapsize);
> +		return -EINVAL;
> +	}
> +
> +	port->mapbase += value;
> +	port->mapsize -= value;
> +
> +	/* Read optional FIFO size */
> +	device_property_read_u32(dev, "fifo-size", &port->fifosize);
> +
> +	if (device_property_read_bool(dev, "no-loopback-test"))
> +		port->flags |= UPF_SKIP_TEST;
> +
> +	/* Get index of serial line, if found in DT aliases */
> +	ret = of_alias_get_id(dev_of_node(dev), "serial");
> +	if (ret >= 0)
> +		port->line = ret;
> +
> +	if (dev_is_platform(dev))
> +		ret = platform_get_irq(to_platform_device(dev), 0);
> +	else
> +		ret = fwnode_irq_get(dev_fwnode(dev), 0);
> +	if (ret == -EPROBE_DEFER)
> +		return ret;
> +	if (ret > 0)
> +		port->irq = ret;
> +	else if (use_defaults)
> +		/* By default IRQ support is mandatory */
> +		return ret;
> +	else
> +		port->irq = 0;
> +
> +	port->flags |= UPF_SHARE_IRQ;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(uart_read_port_properties);

EXPORT_SYMBOL_GPL()?  I have to ask :)

thanks,

greg k-h
Andy Shevchenko March 4, 2024, 11:09 a.m. UTC | #2
On Sat, Mar 02, 2024 at 09:58:53PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Feb 26, 2024 at 04:19:19PM +0200, Andy Shevchenko wrote:

...

> > + * uart_read_port_properties - read firmware properties of the given UART port
> 
> I like, but:
> 
> > + * @port: corresponding port
> > + * @use_defaults: apply defaults (when %true) or validate the values (when %false)
> 
> Using random booleans in a function is horrid.  Every time you see the
> function call, or want to call it, you need to go and look up what the
> boolean is and means.
> 
> Make 2 public functions here, one that does it with use_defaults=true
> and one =false and then have them both call this one static function,
> that way the function names themselves are easy to read and understand
> and maintain over time.

Okay! I'll redo that.

...

> > +EXPORT_SYMBOL(uart_read_port_properties);
> 
> EXPORT_SYMBOL_GPL()?  I have to ask :)

No clue, the rest in this file is EXPORT_SYMBOL, but I admit I followed the
cargo cult. I'll check the modified code and see if I may use _GPL version.

Thank you for review!
diff mbox series

Patch

diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
index 88975a4df306..ecc980e9dba6 100644
--- a/drivers/tty/serial/serial_port.c
+++ b/drivers/tty/serial/serial_port.c
@@ -8,7 +8,10 @@ 
 
 #include <linux/device.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/property.h>
 #include <linux/serial_core.h>
 #include <linux/spinlock.h>
 
@@ -82,6 +85,137 @@  void uart_remove_one_port(struct uart_driver *drv, struct uart_port *port)
 }
 EXPORT_SYMBOL(uart_remove_one_port);
 
+/**
+ * uart_read_port_properties - read firmware properties of the given UART port
+ * @port: corresponding port
+ * @use_defaults: apply defaults (when %true) or validate the values (when %false)
+ *
+ * The following device properties are supported:
+ *   - clock-frequency (optional)
+ *   - fifo-size (optional)
+ *   - no-loopback-test (optional)
+ *   - reg-shift (defaults may apply)
+ *   - reg-offset (value may be validated)
+ *   - reg-io-width (defaults may apply or value may be validated)
+ *   - interrupts (OF only)
+ *   - serial [alias ID] (OF only)
+ *
+ * If the port->dev is of struct platform_device type the interrupt line
+ * will be retrieved via platform_get_irq() call against that device.
+ * Otherwise it will be assigned by fwnode_irq_get() call. In both cases
+ * the index 0 of the resource is used.
+ *
+ * The caller is responsible to initialize the following fields of the @port
+ *   ->dev (must be valid)
+ *   ->flags
+ *   ->mapbase
+ *   ->mapsize
+ *   ->regshift (if @use_defaults is false)
+ * before calling this function. Alternatively the above mentioned fields
+ * may be zeroed, in such case the only ones, that have associated properties
+ * found, will be set to the respective values.
+ *
+ * If no error happened, the ->irq, ->mapbase, ->mapsize will be altered.
+ * The ->iotype is always altered.
+ *
+ * When @use_defaults is true and the respective property is not found
+ * the following values will be applied:
+ *   ->regshift = 0
+ * In this case IRQ must be provided, otherwise an error will be returned.
+ *
+ * When @use_defaults is false and the respective property is found
+ * the following values will be validated:
+ *   - reg-io-width (->iotype)
+ *   - reg-offset (->mapsize against ->mapbase)
+ *
+ * Returns: 0 on success or negative errno on failure
+ */
+int uart_read_port_properties(struct uart_port *port, bool use_defaults)
+{
+	struct device *dev = port->dev;
+	u32 value;
+	int ret;
+
+	/* Read optional UART functional clock frequency */
+	device_property_read_u32(dev, "clock-frequency", &port->uartclk);
+
+	/* Read the registers alignment (default: 8-bit) */
+	ret = device_property_read_u32(dev, "reg-shift", &value);
+	if (ret)
+		port->regshift = use_defaults ? 0 : port->regshift;
+	else
+		port->regshift = value;
+
+	/* Read the registers I/O access type (default: MMIO 8-bit) */
+	ret = device_property_read_u32(dev, "reg-io-width", &value);
+	if (ret) {
+		port->iotype = UPIO_MEM;
+	} else {
+		switch (value) {
+		case 1:
+			port->iotype = UPIO_MEM;
+			break;
+		case 2:
+			port->iotype = UPIO_MEM16;
+			break;
+		case 4:
+			port->iotype = device_is_big_endian(dev) ? UPIO_MEM32BE : UPIO_MEM32;
+			break;
+		default:
+			if (!use_defaults) {
+				dev_err(dev, "Unsupported reg-io-width (%u)\n", value);
+				return -EINVAL;
+			}
+			port->iotype = UPIO_UNKNOWN;
+			break;
+		}
+	}
+
+	/* Read the address mapping base offset (default: no offset) */
+	ret = device_property_read_u32(dev, "reg-offset", &value);
+	if (ret)
+		value = 0;
+
+	/* Check for shifted address mapping overflow */
+	if (!use_defaults && port->mapsize < value) {
+		dev_err(dev, "reg-offset %u exceeds region size %pa\n", value, &port->mapsize);
+		return -EINVAL;
+	}
+
+	port->mapbase += value;
+	port->mapsize -= value;
+
+	/* Read optional FIFO size */
+	device_property_read_u32(dev, "fifo-size", &port->fifosize);
+
+	if (device_property_read_bool(dev, "no-loopback-test"))
+		port->flags |= UPF_SKIP_TEST;
+
+	/* Get index of serial line, if found in DT aliases */
+	ret = of_alias_get_id(dev_of_node(dev), "serial");
+	if (ret >= 0)
+		port->line = ret;
+
+	if (dev_is_platform(dev))
+		ret = platform_get_irq(to_platform_device(dev), 0);
+	else
+		ret = fwnode_irq_get(dev_fwnode(dev), 0);
+	if (ret == -EPROBE_DEFER)
+		return ret;
+	if (ret > 0)
+		port->irq = ret;
+	else if (use_defaults)
+		/* By default IRQ support is mandatory */
+		return ret;
+	else
+		port->irq = 0;
+
+	port->flags |= UPF_SHARE_IRQ;
+
+	return 0;
+}
+EXPORT_SYMBOL(uart_read_port_properties);
+
 static struct device_driver serial_port_driver = {
 	.name = "port",
 	.suppress_bind_attrs = true,
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index c2cf9484014c..3fc8683e7b53 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -962,6 +962,7 @@  int uart_register_driver(struct uart_driver *uart);
 void uart_unregister_driver(struct uart_driver *uart);
 int uart_add_one_port(struct uart_driver *reg, struct uart_port *port);
 void uart_remove_one_port(struct uart_driver *reg, struct uart_port *port);
+int uart_read_port_properties(struct uart_port *port, bool use_defaults);
 bool uart_match_port(const struct uart_port *port1,
 		const struct uart_port *port2);