Re: [PATCH v16 6/6] soc: amd: Add support for AMD Pensando SoC Controller

[Andy Shevchenko]

On Thu, Sep 14, 2023 at 12:52 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.

...

> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spi/spi.h>

types.h ?

> +#include <linux/uaccess.h>

...

> +       struct penctrl_device *penctrl;

> +       u8 tx_buf[PENCTRL_MAX_MSG_LEN];
> +       u8 rx_buf[PENCTRL_MAX_MSG_LEN];

These are not DMA-safe, is this a problem?

> +       struct spi_transfer t[2] = {};
> +       struct penctrl_spi_xfer *msg;
> +       struct spi_device *spi;
> +       unsigned int num_msgs;
> +       struct spi_message m;
> +       u32 size;
> +       int ret;

...

> +       /* Verify and prepare SPI message */
> +       size = _IOC_SIZE(cmd);
> +       num_msgs = size / sizeof(struct penctrl_spi_xfer);

sizeof (*msg) ?

> +       if (num_msgs > 2 || size == 0 || size % sizeof(struct penctrl_spi_xfer)) {

Dito.

> +               ret = -EINVAL;
> +               goto out_unlock;
> +       }

...

> +       msg = memdup_user((struct penctrl_spi_xfer *)arg, size);
> +       if (IS_ERR(msg)) {
> +               ret = PTR_ERR(msg);
> +               goto out_unlock;
> +       }

Wondering if you can start using cleanup.h.

...

> +       /* Perform the transfer */
> +       mutex_lock(&spi_lock);
> +       ret = spi_sync(spi, &m);
> +       mutex_unlock(&spi_lock);

> +       if (ret || (num_msgs == 1))
> +               goto out_unlock;

Second conditional will return 0. Is it by design?
Since it's not so obvious I would split these conditionals.

...

> +       spi->chip_select = current_cs;

spi_set_chipselect()

...

> +static int penctrl_regs_read(struct penctrl_device *penctrl, u32 reg, u32 *val)
> +{
> +       struct spi_device *spi = penctrl->spi;
> +       struct spi_transfer t[2] = {};
> +       struct spi_message m;

> +       u8 txbuf[3];
> +       u8 rxbuf[1];

Not DMA-safe. Is it a problem?

> +       int ret;

> +       txbuf[0] = PENCTRL_SPI_CMD_REGRD;
> +       txbuf[1] = reg;
> +       txbuf[2] = 0;

Can be assigned in the definition block

       u8 txbuf[] = { ... };

> +       t[0].tx_buf = txbuf;
> +       t[0].len = sizeof(txbuf);

> +       rxbuf[0] = 0;

Ditto.

    u8 rxbuf[] = { 0 };

> +       t[1].rx_buf = rxbuf;
> +       t[1].len = sizeof(rxbuf);
> +
> +       spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t));
> +       ret = spi_sync(spi, &m);
> +       if (ret)
> +               return ret;
> +
> +       *val = rxbuf[0];
> +       return 0;
> +}

...

> +static int penctrl_regs_write(struct penctrl_device *penctrl, u32 reg, u32 val)
> +{
> +       struct spi_device *spi = penctrl->spi;
> +       struct spi_transfer t = {};
> +       struct spi_message m;
> +       u8 txbuf[4];

> +       txbuf[0] = PENCTRL_SPI_CMD_REGWR;
> +       txbuf[1] = reg;
> +       txbuf[2] = val;
> +       txbuf[3] = 0;

Can be assigned in the definition block.

> +       t.tx_buf = txbuf;
> +       t.len = sizeof(txbuf);
> +       spi_message_init_with_transfers(&m, &t, 1);
> +       return spi_sync(spi, &m);
> +}

...

> +       struct penctrl_device *penctrl =
> +               container_of(rcdev, struct penctrl_device, rcdev);

One line?

...

> +       spi->chip_select = 0;

spi_set_chipselect()

...

> +       struct penctrl_device *penctrl =
> +               container_of(rcdev, struct penctrl_device, rcdev);

One line?

...

> +       spi->chip_select = 0;

spi_set_chipselect()

...

> +static int penctrl_spi_probe(struct spi_device *spi)
> +{
> +       int i, ret;
> +
> +       /* Allocate driver data */
> +       penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);

devm_kzalloc() ?

> +       if (!penctrl)
> +               return -ENOMEM;
> +
> +       penctrl->spi = spi;
> +       mutex_init(&spi_lock);
> +
> +       for (i = 0; i < ARRAY_SIZE(penctrl_devices); i++) {
> +               ret = misc_register(&penctrl_devices[i]);
> +               if (ret) {
> +                       dev_err(&spi->dev, "Failed to register device %s\n",
> +                               penctrl_devices[i].name);
> +                       goto cleanup;
> +               }
> +       }
> +
> +       /* Register reset controller */
> +       penctrl->rcdev.dev = &spi->dev;
> +       penctrl->rcdev.ops = &penctrl_reset_ops;
> +       penctrl->rcdev.owner = THIS_MODULE;
> +       penctrl->rcdev.of_node = spi->dev.of_node;
> +       penctrl->rcdev.nr_resets = 1;
> +       device_set_node(penctrl->rcdev.dev, dev_fwnode(&spi->dev));
> +
> +       ret = reset_controller_register(&penctrl->rcdev);
> +       if (ret)
> +               return dev_err_probe(&spi->dev, ret,
> +                                    "failed to register reset controller\n");
> +       return 0;

> +cleanup:

err_cleanup: ?

> +       for (i = 0; i < ARRAY_SIZE(penctrl_devices); i++) {

  while (i--) {

> +               if (penctrl_devices[i].this_device)
> +                       misc_deregister(&penctrl_devices[i]);
> +       }
> +       return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko