Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Sep 2022 12:26:58 +0100
From:   Mark Brown <broonie@kernel.org>
To:     Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Cc:     linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
        UNGLinuxDriver@microchip.com
Subject: Re: [PATCH RFC SPI for-next 1/2] spi: microchip: pci1xxxx: Add
 driver for SPI controller of PCI1XXXX PCIe switch
Message-ID: <YzQvgmE9Hubx9mxN@sirena.org.uk>
References: <20220928034336.2939265-1-tharunkumar.pasumarthi@microchip.com>
 <20220928034336.2939265-2-tharunkumar.pasumarthi@microchip.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="2ZkGJ89x+m0DsCiW"
Content-Disposition: inline
In-Reply-To: <20220928034336.2939265-2-tharunkumar.pasumarthi@microchip.com>
Precedence: bulk


--2ZkGJ89x+m0DsCiW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--2ZkGJ89x+m0DsCiW
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmM0L4EACgkQJNaLcl1U
h9BPDAf9HNtpH+r1t/Et40XKvrwTJ27xRyncaDAtFkesUESXpS2bU9qln8TnSGuF
Co6VOt7kFzIe/RTRk0CqEFMg2bSW3Q84zEDK5rPEhllrVsVCeajj5dte9LonPUDr
DytVxW1M/SPUnX0VPyjymmLa1SgI92NtIsrrq0V+0GR+gjWa2X0iLIOfHDBHDJky
RGv1vhewmKRLtRoStzY/7LPUriPbeohccXBrQ6aSFwVFbJpb+Enx4oahthcybKjI
zsOh5Zgw+oY8qPqOd39vYk9tYbZhyCcqhAxu/lW1ZqRCgoiIBuhciR7pBXxTKqvV
CSTeLJufwGh0pbHIVHii0DfIOIMR6w==
=ZzTi
-----END PGP SIGNATURE-----

--2ZkGJ89x+m0DsCiW--