Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 21 Sep 2023 18:19:57 +0300 | Subject | Re: [PATCH v16 6/6] soc: amd: Add support for AMD Pensando SoC Controller |
| |
On Thu, Sep 14, 2023 at 12:52 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.
...
> +#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
| |