[PATCH 1/2] soc: aspeed: Add LPC UART routing support

-----Original Message-----
From: Joel Stanley <joel@jms.id.au>
Sent: Wednesday, September 1, 2021 3:37 PM

On Wed, 1 Sept 2021 at 06:22, Chia-Wei Wang
[off-list ref] wrote:

quoted

Add driver support for the LPC UART routing control. Users can perform

As we discussed, remove the "LPC" part of the name.

quoted

runtime configuration of the RX muxes among the UART controllers and
the UART TXD/RXD IO pins. This is achieved through the exported sysfs

interface.

quoted

Signed-off-by: Chia-Wei Wang <redacted>

It would be good to have some example of how to use it, and the output from
sysfs.

You should also add a patch to document the sysfs files in Documentation/ABI.

Will add a new commit for the sysfs description.

quoted

+++ b/drivers/soc/aspeed/aspeed-lpc-uart-routing.c

@@ -0,0 +1,621 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2018 Google LLC
+ * Copyright (c) 2021 Aspeed Technology Inc.
+ */
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>

You can drop this one.

Revised as suggested.

quoted

+#include <linux/of_platform.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/platform_device.h>
+
+/* register offsets */
+#define HICR9  0x98
+#define HICRA  0x9c
+
+/* attributes options */
+#define UART_ROUTING_IO1       "io1"
+#define UART_ROUTING_IO2       "io2"
+#define UART_ROUTING_IO3       "io3"
+#define UART_ROUTING_IO4       "io4"
+#define UART_ROUTING_IO5       "io5"
+#define UART_ROUTING_IO6       "io6"
+#define UART_ROUTING_IO10      "io10"
+#define UART_ROUTING_UART1     "uart1"
+#define UART_ROUTING_UART2     "uart2"
+#define UART_ROUTING_UART3     "uart3"
+#define UART_ROUTING_UART4     "uart4"
+#define UART_ROUTING_UART5     "uart5"
+#define UART_ROUTING_UART6     "uart6"
+#define UART_ROUTING_UART10    "uart10"
+#define UART_ROUTING_RES       "reserved"
+
+struct aspeed_uart_routing {
+       struct regmap *map;
+       spinlock_t lock;
+       struct attribute_group const *attr_grp; };
+
+struct aspeed_uart_routing_selector {
+       struct device_attribute dev_attr;
+       uint32_t reg;
+       uint32_t mask;
+       uint32_t shift;

These can be u8.

Revised as suggested.

quoted

+static ssize_t aspeed_uart_routing_show(struct device *dev,
+                                       struct device_attribute

*attr,

quoted

+                                       char *buf) {
+       struct aspeed_uart_routing *uart_routing = dev_get_drvdata(dev);
+       struct aspeed_uart_routing_selector *sel =

to_routing_selector(attr);

quoted

+       int val, pos, len;
+
+       regmap_read(uart_routing->map, sel->reg, &val);
+       val = (val >> sel->shift) & sel->mask;
+
+       len = 0;
+       for (pos = 0; sel->options[pos] != NULL; ++pos) {
+               if (pos == val) {
+                       len += snprintf(buf + len, PAGE_SIZE - 1 - len,
+                                       "[%s] ", sel->options[pos]);
+               } else {
+                       len += snprintf(buf + len, PAGE_SIZE - 1 - len,
+                                       "%s ", sel->options[pos]);

The kernel prefers sysfs_emit and sysfs_emit_at insteading of using snprintf
directly.

Revised as suggested.

quoted

+               }
+       }
+
+       if (val >= pos) {
+               len += snprintf(buf + len, PAGE_SIZE - 1 - len,
+                               "[unknown(%d)]", val);
+       }
+
+       len += snprintf(buf + len, PAGE_SIZE - 1 - len, "\n");
+
+       return len;
+}
+
+static ssize_t aspeed_uart_routing_store(struct device *dev,
+                                        struct device_attribute

*attr,

quoted

+                                        const char *buf, size_t
+count) {
+       unsigned long flags;
+       struct aspeed_uart_routing *uart_routing = dev_get_drvdata(dev);
+       struct aspeed_uart_routing_selector *sel =

to_routing_selector(attr);

quoted

+       int val;
+
+       val = match_string(sel->options, -1, buf);
+       if (val < 0) {
+               dev_err(dev, "invalid value \"%s\"\n", buf);
+               return -EINVAL;
+       }
+
+       spin_lock_irqsave(&uart_routing->lock, flags);

I can't see why you would need a spinlock here. The regmap has it's own
locking so it will protect against concurrent updates to the registers.

You are right. Lock is not needed here. Will removed it as suggested.

quoted

+
+       regmap_update_bits(uart_routing->map, sel->reg,
+                       (sel->mask << sel->shift),
+                       (val & sel->mask) << sel->shift);
+
+       spin_unlock_irqrestore(&uart_routing->lock, flags);
+
+       return count;
+}
+
+static int aspeed_uart_routing_probe(struct platform_device *pdev) {
+       int rc;
+       struct device *dev = &pdev->dev;
+       struct aspeed_uart_routing *uart_routing;
+
+       uart_routing = devm_kzalloc(&pdev->dev,
+                                   sizeof(*uart_routing),
+                                   GFP_KERNEL);

You can reformat this file to have longer lines; the kernel is ok with up to 100
columsn these days.

quoted

+       if (!uart_routing) {
+               dev_err(dev, "cannot allocate memory\n");

I'd drop this error message.

Revised as suggested

quoted

+               return -ENOMEM;
+       }
+
+       uart_routing->map =

syscon_node_to_regmap(dev->parent->of_node);

quoted

+       if (IS_ERR(uart_routing->map)) {
+               dev_err(dev, "cannot get regmap\n");
+               return PTR_ERR(uart_routing->map);
+       }
+
+       uart_routing->attr_grp = of_device_get_match_data(dev);
+
+       spin_lock_init(&uart_routing->lock);

I don't think you need the lock at all.

Same as above.

Thanks for reviewing this.
The v2 patch will include the driver refactoring and additional documentation.

Chiawei