On Wed, Sep 28, 2022 at 09:13:35AM +0530, Tharun Kumar P wrote:
> @@ -0,0 +1,378 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * PCI1xxxx SPI driver
> + * Copyright (C) 2022 Microchip Technology Inc.
> + * Authors: Tharun Kumar P <tharunkumar.pasumarthi@xxxxxxxxxxxxx>
> + * Kumaravel Thiagarajan <Kumaravel.Thiagarajan@xxxxxxxxxxxxx>
> + */
Please make the above a single C++ style comment block so things look
more intentional.
> +static void pci1xxxx_spi_set_cs(struct spi_device *spi, bool enable)
> +{
> + struct pci1xxxx_spi_internal *p = spi_controller_get_devdata(spi->controller);
> + struct pci1xxxx_spi *par = p->parent;
> + u32 regval;
> +
> + /* Set the DEV_SEL bits of the SPI_MST_CTL_REG */
> + regval = readl(par->reg_base + SPI_MST_CTL_REG_OFFSET(p->hw_inst));
> + if (enable) {
> + regval &= ~SPI_MST_CTL_DEVSEL_MASK;
> + regval |= (spi->chip_select << 25);
> + writel(regval,
> + par->reg_base + SPI_MST_CTL_REG_OFFSET(p->hw_inst));
> + }
> +}
This does nothing if the chip select is to be disabled, that's clearly
not going to work.
> +static int pci1xxxx_spi_transfer_one(struct spi_controller *spi_ctlr,
> + struct spi_device *spi, struct spi_transfer *xfer)
> +{
> + if (tx_buf) {
> + if (rx_buf) {
The driver should set SPI_CONTROLLER_MUST_TX since it needs to transmit
data in order to recieve.
> +static int pci1xxxx_spi_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> + u8 hw_inst_cnt, iter, start, only_sec_inst;
> + struct pci1xxxx_spi_internal *spi_sub_ptr;
> + struct device *dev = &pdev->dev;
> + struct pci1xxxx_spi *spi_bus;
> + struct spi_master *spi_host;
> + u32 regval;
> + int ret;
> +
> + hw_inst_cnt = ent->driver_data & 0x0f;
> + start = (ent->driver_data & 0xf0) >> 4;
> + (start == 1) ? (only_sec_inst = 1) : (only_sec_inst = 0);
Please write normal conditional statements to improve legibility.
> +
> + ret = devm_request_irq(&pdev->dev, spi_sub_ptr->irq,
> + pci1xxxx_spi_isr, PCI1XXXX_IRQ_FLAGS,
> + pci_name(pdev), spi_sub_ptr);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Unable to request irq : %d",
> + spi_sub_ptr->irq);
> + ret = -ENODEV;
> + goto error;
> + }
Are you sure the device is fully set up and ready for interrupts at this
point, and that the freeing of the driver will work fine with devm?
> + init_completion(&spi_sub_ptr->spi_xfer_done);
I note that the completion that the interrupt handler uses isn't
allocated yet for example...
> + /* Initialize Interrupts - SPI_INT */
> + regval = readl(spi_bus->reg_base +
> + SPI_MST_EVENT_MASK_REG_OFFSET(spi_sub_ptr->hw_inst));
> + regval &= ~SPI_INTR;
> + writel(regval, spi_bus->reg_base +
> + SPI_MST_EVENT_MASK_REG_OFFSET(spi_sub_ptr->hw_inst));
...and we do some interrupt mask management later.
Attachment:
signature.asc
Description: PGP signature
