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

ChiaWei Wang chiawei_wang at aspeedtech.com
Wed Sep 1 02:43:30 PDT 2021


> -----Original Message-----
> From: Joel Stanley <joel at jms.id.au>
> Sent: Wednesday, September 1, 2021 3:37 PM
> 
> On Wed, 1 Sept 2021 at 06:22, Chia-Wei Wang
> <chiawei_wang at aspeedtech.com> 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 at aspeedtech.com>
> 
> 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


More information about the linux-arm-kernel mailing list