| Message ID | 20201001222829.15977-4-Sergey.Semin@baikalelectronics.ru |
|---|---|
| State | Accepted |
| Commit | a3577bd8cba554f962b6af082eb43dde7fe7cd09 |
| Headers | show |
| Series | [v3,01/21] spi: dw: Use an explicit set_cs assignment | expand |
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. -- With Best Regards, Andy Shevchenko
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 > >
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;
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(-)