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

quoted hunk

On Wed, 1 Sept 2021 at 06:22, Chia-Wei Wang [off-list ref] wrote:
Add driver support for the LPC UART routing control. Users can perform
As we discussed, remove the "LPC" part of the name.

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.

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.

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

+#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.

+static ssize_t aspeed_uart_routing_show(struct device *dev,
+                                       struct device_attribute *attr,
+                                       char *buf)
+{
+       struct aspeed_uart_routing *uart_routing = dev_get_drvdata(dev);
+       struct aspeed_uart_routing_selector *sel = to_routing_selector(attr);
+       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.

+               }
+       }
+
+       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,
+                                        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);
+       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.

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

+       if (!uart_routing) {
+               dev_err(dev, "cannot allocate memory\n");
I'd drop this error message.

+               return -ENOMEM;
+       }
+
+       uart_routing->map = syscon_node_to_regmap(dev->parent->of_node);
+       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.

Cheers,

Joel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help