[PATCH SPI for-next] spi: microchip: pci1xxxx: Fix minor bugs in spi-pci1xxxx driver

[PATCH SPI for-next] spi: microchip: pci1xxxx: Fix minor bugs in spi-pci1xxxx driver

[Tharun Kumar P]

Following bugs are fixed in this patch:
1. pci1xxxx_spi_resume API masks SPI interrupt bit which prohibits
firing of interrupt to the host at the end of the transaction after
suspend-resume. This patch unmasks this bit at resume.
2. In pci1xxxx_spi_transfer_one API, length of SPI transaction gets
cleared by unmasking length field. Set length of transaction after
unmasking length field.
3. Remove support for disabling chip select as hardware does not support
the same.

Fixes: 1cc0cbea7167 ("spi: microchip: pci1xxxx: Add driver for SPI controller of PCI1XXXX PCIe switch")
Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
---
 drivers/spi/spi-pci1xxxx.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-pci1xxxx.c b/drivers/spi/spi-pci1xxxx.c
index 1c5731641a04..9a044012aca7 100644
--- a/drivers/spi/spi-pci1xxxx.c
+++ b/drivers/spi/spi-pci1xxxx.c
@@ -58,7 +58,7 @@
 #define VENDOR_ID_MCHP 0x1055
 
 #define SPI_SUSPEND_CONFIG 0x101
-#define SPI_RESUME_CONFIG 0x303
+#define SPI_RESUME_CONFIG 0x203
 
 struct pci1xxxx_spi_internal {
 	u8 hw_inst;
@@ -114,16 +114,11 @@ static void pci1xxxx_spi_set_cs(struct spi_device *spi, bool enable)
 
 	/* 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) {
+	if (!enable) {
 		regval &= ~SPI_MST_CTL_DEVSEL_MASK;
 		regval |= (spi_get_chipselect(spi, 0) << 25);
 		writel(regval,
 		       par->reg_base + SPI_MST_CTL_REG_OFFSET(p->hw_inst));
-	} else {
-		regval &= ~(spi_get_chipselect(spi, 0) << 25);
-		writel(regval,
-		       par->reg_base + SPI_MST_CTL_REG_OFFSET(p->hw_inst));
-
 	}
 }
 
@@ -199,8 +194,9 @@ static int pci1xxxx_spi_transfer_one(struct spi_controller *spi_ctlr,
 			else
 				regval &= ~SPI_MST_CTL_MODE_SEL;
 
-			regval |= ((clkdiv << 5) | SPI_FORCE_CE | (len << 8));
+			regval |= ((clkdiv << 5) | SPI_FORCE_CE);
 			regval &= ~SPI_MST_CTL_CMD_LEN_MASK;
+			regval |= (len << 8);
 			writel(regval, par->reg_base +
 			       SPI_MST_CTL_REG_OFFSET(p->hw_inst));
 			regval = readl(par->reg_base +
-- 
2.25.1

Re: [PATCH SPI for-next] spi: microchip: pci1xxxx: Fix minor bugs in spi-pci1xxxx driver

[Mark Brown]

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

On Tue, Mar 28, 2023 at 11:12:12AM +0530, Tharun Kumar P wrote:
> Following bugs are fixed in this patch:
> 1. pci1xxxx_spi_resume API masks SPI interrupt bit which prohibits
> firing of interrupt to the host at the end of the transaction after
> suspend-resume. This patch unmasks this bit at resume.
> 2. In pci1xxxx_spi_transfer_one API, length of SPI transaction gets
> cleared by unmasking length field. Set length of transaction after
> unmasking length field.
> 3. Remove support for disabling chip select as hardware does not support
> the same.

As covered in submitting-patches.rst you should send one patch per
change, this makes things much easier to review.

>  drivers/spi/spi-pci1xxxx.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)

The subject says this is a patch for the microchip driver...

>  	/* 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) {
> +	if (!enable) {
>  		regval &= ~SPI_MST_CTL_DEVSEL_MASK;
>  		regval |= (spi_get_chipselect(spi, 0) << 25);
>  		writel(regval,
>  		       par->reg_base + SPI_MST_CTL_REG_OFFSET(p->hw_inst));
> -	} else {
> -		regval &= ~(spi_get_chipselect(spi, 0) << 25);
> -		writel(regval,
> -		       par->reg_base + SPI_MST_CTL_REG_OFFSET(p->hw_inst));
> -

I am unclear how chip select will ever be asserted with this change?
Now the value is only written if we are disabling.

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

Re: [PATCH SPI for-next] spi: microchip: pci1xxxx: Fix minor bugs in spi-pci1xxxx driver

[Tharunkumar.Pasumarthi]

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

Hi Mark,

Thanks for the comments. Please find my inline replies below:

> As covered in submitting-patches.rst you should send one patch per
> change, this makes things much easier to review.

Okay. I will submit these fixes as a new patchset.

> I am unclear how chip select will ever be asserted with this change?
> Now the value is only written if we are disabling.

In PCI1xxxx, there is a common bit in hardware to enable / disable chip
select lines. I will use this bit in pci1xxxx_spi_set_cs API in 
the upcoming version of patch. Currently, this bit is used within 
pci1xxxx_spi_transfer_one API.


Thanks,
Tharun Kumar P

[-- Attachment #2: Type: message/rfc822, Size: 5694 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 1484 bytes --]

On Tue, Mar 28, 2023 at 11:12:12AM +0530, Tharun Kumar P wrote:
> Following bugs are fixed in this patch:
> 1. pci1xxxx_spi_resume API masks SPI interrupt bit which prohibits
> firing of interrupt to the host at the end of the transaction after
> suspend-resume. This patch unmasks this bit at resume.
> 2. In pci1xxxx_spi_transfer_one API, length of SPI transaction gets
> cleared by unmasking length field. Set length of transaction after
> unmasking length field.
> 3. Remove support for disabling chip select as hardware does not support
> the same.

As covered in submitting-patches.rst you should send one patch per
change, this makes things much easier to review.

>  drivers/spi/spi-pci1xxxx.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)

The subject says this is a patch for the microchip driver...

>  	/* 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) {
> +	if (!enable) {
>  		regval &= ~SPI_MST_CTL_DEVSEL_MASK;
>  		regval |= (spi_get_chipselect(spi, 0) << 25);
>  		writel(regval,
>  		       par->reg_base + SPI_MST_CTL_REG_OFFSET(p->hw_inst));
> -	} else {
> -		regval &= ~(spi_get_chipselect(spi, 0) << 25);
> -		writel(regval,
> -		       par->reg_base + SPI_MST_CTL_REG_OFFSET(p->hw_inst));
> -

I am unclear how chip select will ever be asserted with this change?
Now the value is only written if we are disabling.

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