Re: [PATCH v12 15/15] soc: amd: Add support for AMD Pensando SoC Controller

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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