lkml.org 
[lkml]   [2022]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC SPI for-next 1/2] spi: microchip: pci1xxxx: Add driver for SPI controller of PCI1XXXX PCIe switch
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@microchip.com>
> + * Kumaravel Thiagarajan <Kumaravel.Thiagarajan@microchip.com>
> + */

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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2022-09-28 13:27    [W:0.076 / U:0.536 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site