diff mbox series

[v3,03/21] spi: dw: Detach SPI device specific CR0 config method

Message ID 20201001222829.15977-4-Sergey.Semin@baikalelectronics.ru (mailing list archive)
State Superseded
Headers show
Series spi: dw: Add full Baikal-T1 SPI Controllers support | expand

Commit Message

Serge Semin Oct. 1, 2020, 10:28 p.m. UTC
Indeed there is no point in detecting the SPI peripheral device parameters
and initializing the CR0 register fields each time an SPI transfer is
executed. Instead let's define a dedicated CR0 chip-data member, which
will be initialized in accordance with the SPI device settings at the
moment of setting it up.

By doing so we'll finally make the SPI device chip_data serving as it's
supposed to - to preserve the SPI device specific DW SPI configuration.
See spi-fsl-dspi.c, spi-pl022.c, spi-pxa2xx.c drivers for example of the
way the chip data is utilized.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/spi/spi-dw-core.c | 43 +++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 13 deletions(-)

Comments

Andy Shevchenko Oct. 2, 2020, 10:22 a.m. UTC | #1
On Fri, Oct 02, 2020 at 01:28:11AM +0300, Serge Semin wrote:
> Indeed there is no point in detecting the SPI peripheral device parameters
> and initializing the CR0 register fields each time an SPI transfer is
> executed. Instead let's define a dedicated CR0 chip-data member, which
> will be initialized in accordance with the SPI device settings at the
> moment of setting it up.
> 
> By doing so we'll finally make the SPI device chip_data serving as it's
> supposed to - to preserve the SPI device specific DW SPI configuration.
> See spi-fsl-dspi.c, spi-pl022.c, spi-pxa2xx.c drivers for example of the
> way the chip data is utilized.

> +static void dw_spi_update_cr0(struct dw_spi *dws, struct spi_device *spi,
> +			      struct spi_transfer *transfer)

Yep, why not to place this in previous patch exactly here?

> +	/*
> +	 * Update CR0 data each time the setup callback is invoked since
> +	 * the device parameters could have been changed, for instance, by
> +	 * the MMC SPI driver or something else.
> +	 */
> +	chip->cr0 = dw_spi_get_cr0(dws, spi);

I would rather name it prepare or alike. 'get' assumes getting value or
something like that.
Serge Semin Oct. 2, 2020, 5:47 p.m. UTC | #2
On Fri, Oct 02, 2020 at 01:22:46PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 02, 2020 at 01:28:11AM +0300, Serge Semin wrote:
> > Indeed there is no point in detecting the SPI peripheral device parameters
> > and initializing the CR0 register fields each time an SPI transfer is
> > executed. Instead let's define a dedicated CR0 chip-data member, which
> > will be initialized in accordance with the SPI device settings at the
> > moment of setting it up.
> > 
> > By doing so we'll finally make the SPI device chip_data serving as it's
> > supposed to - to preserve the SPI device specific DW SPI configuration.
> > See spi-fsl-dspi.c, spi-pl022.c, spi-pxa2xx.c drivers for example of the
> > way the chip data is utilized.
> 
> > +static void dw_spi_update_cr0(struct dw_spi *dws, struct spi_device *spi,
> > +			      struct spi_transfer *transfer)
> 

> Yep, why not to place this in previous patch exactly here?

The previous patch is about introducing the DWC SSI capability. This one is
about splitting the functionality up.

> 
> > +	/*
> > +	 * Update CR0 data each time the setup callback is invoked since
> > +	 * the device parameters could have been changed, for instance, by
> > +	 * the MMC SPI driver or something else.
> > +	 */
> > +	chip->cr0 = dw_spi_get_cr0(dws, spi);
> 

> I would rather name it prepare or alike. 'get' assumes getting value or
> something like that.

This seems reasonable. What verb do you think would be better: prepare,
calc, assemble, construct, make, compute, collect, compose, form, compile, etc ?
Personally prepare or calc or assemble are the best candidates. What do you
think?

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Andy Shevchenko Oct. 2, 2020, 6:24 p.m. UTC | #3
On Fri, Oct 2, 2020 at 8:47 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Fri, Oct 02, 2020 at 01:22:46PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 02, 2020 at 01:28:11AM +0300, Serge Semin wrote:

> > > +   /*
> > > +    * Update CR0 data each time the setup callback is invoked since
> > > +    * the device parameters could have been changed, for instance, by
> > > +    * the MMC SPI driver or something else.
> > > +    */
> > > +   chip->cr0 = dw_spi_get_cr0(dws, spi);
> >
>
> > I would rather name it prepare or alike. 'get' assumes getting value or
> > something like that.
>
> This seems reasonable. What verb do you think would be better: prepare,
> calc, assemble, construct, make, compute, collect, compose, form, compile, etc ?
> Personally prepare or calc or assemble are the best candidates. What do you
> think?

prepare is good enough if you agree on it.
Serge Semin Oct. 2, 2020, 7:56 p.m. UTC | #4
On Fri, Oct 02, 2020 at 09:24:55PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 2, 2020 at 8:47 PM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> > On Fri, Oct 02, 2020 at 01:22:46PM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 02, 2020 at 01:28:11AM +0300, Serge Semin wrote:
> 
> > > > +   /*
> > > > +    * Update CR0 data each time the setup callback is invoked since
> > > > +    * the device parameters could have been changed, for instance, by
> > > > +    * the MMC SPI driver or something else.
> > > > +    */
> > > > +   chip->cr0 = dw_spi_get_cr0(dws, spi);
> > >
> >
> > > I would rather name it prepare or alike. 'get' assumes getting value or
> > > something like that.
> >
> > This seems reasonable. What verb do you think would be better: prepare,
> > calc, assemble, construct, make, compute, collect, compose, form, compile, etc ?
> > Personally prepare or calc or assemble are the best candidates. What do you
> > think?
> 

> prepare is good enough if you agree on it.

Ok. "prepare" it is then.

-Sergey

> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
diff mbox series

Patch

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index be16fdaf7ce0..6b89330708bc 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -27,6 +27,7 @@  struct chip_data {
 	u16 clk_div;		/* baud rate divider */
 	u32 speed_hz;		/* baud rate */
 
+	u32 cr0;
 	u32 rx_sample_dly;	/* RX sample delay */
 };
 
@@ -228,14 +229,9 @@  static irqreturn_t dw_spi_irq(int irq, void *dev_id)
 	return dws->transfer_handler(dws);
 }
 
-static void dw_spi_update_cr0(struct dw_spi *dws, struct spi_device *spi,
-			      struct spi_transfer *transfer)
+static u32 dw_spi_get_cr0(struct dw_spi *dws, struct spi_device *spi)
 {
-	struct chip_data *chip = spi_get_ctldata(spi);
-	u32 cr0;
-
-	/* CTRLR0[ 4/3: 0] Data Frame Size */
-	cr0 = (transfer->bits_per_word - 1);
+	u32 cr0 = 0;
 
 	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {
 		/* CTRLR0[ 5: 4] Frame Format */
@@ -251,9 +247,6 @@  static void dw_spi_update_cr0(struct dw_spi *dws, struct spi_device *spi,
 
 		/* CTRLR0[11] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << SPI_SRL_OFFSET;
-
-		/* CTRLR0[ 9:8] Transfer Mode */
-		cr0 |= chip->tmode << SPI_TMOD_OFFSET;
 	} else {
 		/* CTRLR0[ 7: 6] Frame Format */
 		cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET;
@@ -269,13 +262,29 @@  static void dw_spi_update_cr0(struct dw_spi *dws, struct spi_device *spi,
 		/* CTRLR0[13] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << DWC_SSI_CTRLR0_SRL_OFFSET;
 
-		/* CTRLR0[11:10] Transfer Mode */
-		cr0 |= chip->tmode << DWC_SSI_CTRLR0_TMOD_OFFSET;
-
 		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
 			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
 	}
 
+	return cr0;
+}
+
+static void dw_spi_update_cr0(struct dw_spi *dws, struct spi_device *spi,
+			      struct spi_transfer *transfer)
+{
+	struct chip_data *chip = spi_get_ctldata(spi);
+	u32 cr0 = chip->cr0;
+
+	/* CTRLR0[ 4/3: 0] Data Frame Size */
+	cr0 |= (transfer->bits_per_word - 1);
+
+	if (!(dws->caps & DW_SPI_CAP_DWC_SSI))
+		/* CTRLR0[ 9:8] Transfer Mode */
+		cr0 |= chip->tmode << SPI_TMOD_OFFSET;
+	else
+		/* CTRLR0[11:10] Transfer Mode */
+		cr0 |= chip->tmode << DWC_SSI_CTRLR0_TMOD_OFFSET;
+
 	dw_writel(dws, DW_SPI_CTRLR0, cr0);
 }
 
@@ -373,6 +382,7 @@  static void dw_spi_handle_err(struct spi_controller *master,
 /* This may be called twice for each spi dev */
 static int dw_spi_setup(struct spi_device *spi)
 {
+	struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
 	struct chip_data *chip;
 
 	/* Only alloc on first setup */
@@ -396,6 +406,13 @@  static int dw_spi_setup(struct spi_device *spi)
 							dws->max_freq);
 	}
 
+	/*
+	 * Update CR0 data each time the setup callback is invoked since
+	 * the device parameters could have been changed, for instance, by
+	 * the MMC SPI driver or something else.
+	 */
+	chip->cr0 = dw_spi_get_cr0(dws, spi);
+
 	chip->tmode = SPI_TMOD_TR;
 
 	return 0;