> -----Original Message-----
> From: Joel Stanley <joel@xxxxxxxxx>
> Sent: Wednesday, September 1, 2021 3:37 PM
>
> On Wed, 1 Sept 2021 at 06:22, Chia-Wei Wang
> <chiawei_wang@xxxxxxxxxxxxxx> 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 <chiawei_wang@xxxxxxxxxxxxxx>
>
> 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.
> > +++ 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.
>
> > +#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.
>
> > +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.
Revised as suggested.
>
> > + }
> > + }
> > +
> > + 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.
You are right. Lock is not needed here. Will removed it as suggested.
>
> > +
> > + 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.
Revised as suggested
>
> > + 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.
Same as above.
Thanks for reviewing this.
The v2 patch will include the driver refactoring and additional documentation.
Chiawei
