Re: [PATCH v12 15/15] soc: amd: Add support for AMD Pensando SoC Controller
From: Andy Shevchenko
Date: Thu Mar 23 2023 - 07:06:49 EST
On Thu, Mar 23, 2023 at 2:11 AM Brad Larson <blarson@xxxxxxx> wrote:
>
> The Pensando SoC controller is a SPI connected companion device
> that is present in all Pensando SoC board designs. The essential
> board management registers are accessed on chip select 0 with
> board mgmt IO support accessed using additional chip selects.
...
> +config AMD_PENSANDO_CTRL
> + tristate "AMD Pensando SoC Controller"
> + depends on SPI_MASTER=y
> + depends on (ARCH_PENSANDO && OF) || COMPILE_TEST
> + default y if ARCH_PENSANDO
default ARCH_PENSANDO
?
> + select REGMAP_SPI
> + select MFD_SYSCON
...
> +/*
> + * AMD Pensando SoC Controller
> + *
> + * Userspace interface and reset driver support for SPI connected Pensando SoC
> + * controller device. This device is present in all Pensando SoC designs and
> + * contains board control/status regsiters and management IO support.
registers ?
> + *
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + */
...
> +#include <linux/cdev.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
Seems semi-random. Are you sure you use this and not missing mod_devicetable.h?
> +#include <linux/reset-controller.h>
> +#include <linux/spi/spi.h>
...
> +struct penctrl_device {
> + struct spi_device *spi_dev;
> + struct reset_controller_dev rcdev;
Perhaps swapping these two might provide a better code generation.
> +};
...
> + struct spi_transfer t[2] = { 0 };
0 is not needed.
...
> + if (_IOC_DIR(cmd) & _IOC_READ)
> + ret = !access_ok((void __user *)arg, _IOC_SIZE(cmd));
> + else if (_IOC_DIR(cmd) & _IOC_WRITE)
> + ret = !access_ok((void __user *)arg, _IOC_SIZE(cmd));
Maybe you should create a temporary variable as
void __user *in = ... arg;
?
> + if (ret)
> + return -EFAULT;
...
> + /* Verify and prepare spi message */
SPI
> + size = _IOC_SIZE(cmd);
> + if ((size % sizeof(struct penctrl_spi_xfer)) != 0) {
' != 0' is redundant.
> + ret = -EINVAL;
> + goto done;
> + }
> + num_msgs = size / sizeof(struct penctrl_spi_xfer);
> + if (num_msgs == 0) {
> + ret = -EINVAL;
> + goto done;
> + }
Can be unified with a previous check as
if (size == 0 || size % ...)
> + msg = memdup_user((struct penctrl_spi_xfer __user *)arg, size);
> + if (!msg) {
> + ret = PTR_ERR(msg);
> + goto done;
> + }
...
> + if (copy_from_user((void *)(uintptr_t)tx_buf,
> + (void __user *)msg->tx_buf, msg->len)) {
Why are all these castings here?
> + ret = -EFAULT;
> + goto done;
> + }
...
> + if (copy_to_user((void __user *)msg->rx_buf,
> + (void *)(uintptr_t)rx_buf, msg->len))
> + ret = -EFAULT;
Ditto.
...
> + struct spi_transfer t[2] = { 0 };
0 is redundant.
...
> + struct spi_transfer t[1] = { 0 };
Ditto.
Why is this an array?
...
> + ret = spi_sync(spi_dev, &m);
> + return ret;
return spi_sync(...);
...
> + np = spi_dev->dev.parent->of_node;
> + ret = of_property_read_u32(np, "num-cs", &num_cs);
Why not simply device_property_read_u32()?
> + if (ret)
> + return dev_err_probe(&spi_dev->dev, ret,
> + "number of chip-selects not defined");
...
> + cdev = cdev_alloc();
> + if (!cdev) {
> + dev_err(&spi_dev->dev, "allocation of cdev failed");
> + ret = -ENOMEM;
ret = dev_err_probe(...);
> + goto cdev_failed;
> + }
...
> + ret = cdev_add(cdev, penctrl_devt, num_cs);
> + if (ret) {
> + dev_err(&spi_dev->dev, "register of cdev failed");
dev_err_probe() ?
> + goto cdev_delete;
> + }
...
> + penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
> + if (!penctrl) {
> + ret = -ENOMEM;
> + dev_err(&spi_dev->dev, "allocate driver data failed");
ret = dev_err_probe();
But we do not print memory allocation failure messages.
> + goto cdev_delete;
> + }
...
> + if (IS_ERR(dev)) {
> + ret = IS_ERR(dev);
> + dev_err(&spi_dev->dev, "error creating device\n");
ret = dev_err_probe();
> + goto cdev_delete;
> + }
> + dev_dbg(&spi_dev->dev, "created device major %u, minor %d\n",
> + MAJOR(penctrl_devt), cs);
> + }
...
> + spi_set_drvdata(spi_dev, penctrl);
Is it in use?
...
> + penctrl->rcdev.of_node = spi_dev->dev.of_node;
device_set_node();
...
> + ret = reset_controller_register(&penctrl->rcdev);
> + if (ret)
> + return dev_err_probe(&spi_dev->dev, ret,
> + "failed to register reset controller\n");
> + return ret;
return 0;
...
> + device_destroy(penctrl_class, penctrl_devt);
Are you sure this is the correct API?
> + return ret;
...
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
Sorted?
--
With Best Regards,
Andy Shevchenko