Re: [PATCH v16 6/6] soc: amd: Add support for AMD Pensando SoC Controller
From: Brad Larson
Date: Tue Sep 26 2023 - 16:05:58 EST
Hi Andy,
On Thu, Sep 21, 2023 at 18:19:57 +0300 Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> 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 ?
I'll add 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?
It's not a problem, the peripheral is PIO FIFO driven only.
>> + 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) ?
Yes, more compact for here and below.
>
>> + 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.
Perhaps if recommended, I don't see DEFINE_(FREE,UNLOCK,...) being used.
...
>> + /* 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.
I'll split this to be clear, yes return 0 for success.
...
>> + spi->chip_select = current_cs;
>
> spi_set_chipselect()
Yes, I'll change to inline function spi_set_chipselect(spi, 0, current_cs). The
second arg must be legacy as its unused.
...
>> +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?
>
Not a problem, the peripheral is PIO only using FIFOs.
>> + int ret;
>
>> + txbuf[0] = PENCTRL_SPI_CMD_REGRD;
>> + txbuf[1] = reg;
>> + txbuf[2] = 0;
>
> Can be assigned in the definition block
>
> u8 txbuf[] = { ... };
>
I'll change that here and below.
>> + 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?
I'll check/change.
>
>...
>
>> + spi->chip_select = 0;
>
> spi_set_chipselect()
Yes, spi_set_chipselect(spi, 0, 0);
...
>> + struct penctrl_device *penctrl =
>> + container_of(rcdev, struct penctrl_device, rcdev);
>
> One line?
I'll check/change.
...
>> + spi->chip_select = 0;
>
> spi_set_chipselect()
Yes, spi_set_chipselect(spi, 0, 0);
...
>> +static int penctrl_spi_probe(struct spi_device *spi)
>> +{
>> + int i, ret;
>> +
>> + /* Allocate driver data */
>> + penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
>
> devm_kzalloc() ?
Yes will change to 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: ?
Will use err_cleanup:
>> + for (i = 0; i < ARRAY_SIZE(penctrl_devices); i++) {
>
> while (i--) {
>
Yes, can change to while(), order doesn't matter.
>> + if (penctrl_devices[i].this_device)
>> + misc_deregister(&penctrl_devices[i]);
>> + }
>> + return ret;
>> +}
Regards,
Brad