Re: [PATCH RFC SPI for-next 1/2] spi: microchip: pci1xxxx: Add driver for SPI controller of PCI1XXXX PCIe switch

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2850 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]