Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 23 Mar 2023 13:06:04 +0200 | Subject | Re: [PATCH v12 15/15] soc: amd: Add support for AMD Pensando SoC Controller |
| |
On Thu, Mar 23, 2023 at 2:11 AM Brad Larson <blarson@amd.com> 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
| |