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

[ChiaWei Wang]

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