Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1432420 > unrolled thread

[PATCH v2 0/5] s3c64xx: consider the case of a disconnected CS line and some code rework

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/5] s3c64xx: consider the case of a disconnected CS line  and some code rework Andi Shyti <andi.shyti@samsung.com> - 2016-06-28 04:50 +0200
    [PATCH v2 3/5] spi: s3c64xx: do not configure the device twice Andi Shyti <andi.shyti@samsung.com> - 2016-06-28 04:50 +0200
    [PATCH v2 5/5] spi: s3c64xx: use unsigned type for fifo handling  variables Andi Shyti <andi.shyti@samsung.com> - 2016-06-28 04:50 +0200
      Re: [PATCH v2 5/5] spi: s3c64xx: use unsigned type for fifo handling  variables Mark Brown <broonie@kernel.org> - 2016-07-05 17:00 +0200
        Re: [PATCH v2 5/5] spi: s3c64xx: use unsigned type for fifo handling  variables Andi Shyti <andi@etezian.org> - 2016-07-05 17:10 +0200

#1432420 — [PATCH v2 0/5] s3c64xx: consider the case of a disconnected CS line and some code rework

Hi,

the main goal of the patchset is to support SPI cnnected device
without CS line link.

The first two patches make the s3c64xx driver to consider the
case of a disconnected CS line. This is done by adding a property
in the DTS ("no-cs-readback") which informs the device driver the
absence of a chip selection link.

The last three patches are just some code re-work and
beautification.

Changelog: V1 -> V2

 - the first version of the patchset was removing an 'if'
   statement on "!spi->chip_select" which was causing the SPI
   core to fail. In this case the drivers would have been able to
   set a chip_select = 0 in absence of a CS link. After a short
   discussion with Mark, this has been replaced, as described
   above, by a property in the DTS.

 - one more patch has been added which assigns to some variable
   the proper type

 - some typos fixed in the commit messages

Thanks,
Andi

Andi Shyti (5):
  spi: s3c64xx: group the CS signalling writes in a single function
  spi: s3c64xx: consider the case when the CS line is not connected
  spi: s3c64xx: do not configure the device twice
  spi: s3c64xx: simplify if statement in prepare_transfer function
  spi: s3c64xx: use unsigned type for fifo handling variables

 .../devicetree/bindings/spi/spi-samsung.txt        |   3 +
 drivers/spi/spi-s3c64xx.c                          | 114 +++++++++++----------
 include/linux/platform_data/spi-s3c64xx.h          |   1 +
 3 files changed, 65 insertions(+), 53 deletions(-)

-- 
2.8.1

[toc] | [next] | [standalone]

#1432422 — [PATCH v2 3/5] spi: s3c64xx: do not configure the device twice

At the start of the transfer, the spi_config function is called
twice, the first time when the 3c64xx_spi_prepare_message is
called and the second time with the s3c64xx_spi_transfer_one,
both called from the spi framework.

Remove the first call at the prepare message because in that
point we don't have the imformation about "bit per word" and
frequency.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 14269b0..c65a9e6 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -676,16 +676,6 @@ static int s3c64xx_spi_prepare_message(struct spi_master *master,
 	struct spi_device *spi = msg->spi;
 	struct s3c64xx_spi_csinfo *cs = spi->controller_data;
 
-	/* If Master's(controller) state differs from that needed by Slave */
-	if (sdd->cur_speed != spi->max_speed_hz
-			|| sdd->cur_mode != spi->mode
-			|| sdd->cur_bpw != spi->bits_per_word) {
-		sdd->cur_bpw = spi->bits_per_word;
-		sdd->cur_speed = spi->max_speed_hz;
-		sdd->cur_mode = spi->mode;
-		s3c64xx_spi_config(sdd);
-	}
-
 	/* Configure feedback delay */
 	writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);
 
@@ -712,6 +702,7 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 	if (bpw != sdd->cur_bpw || speed != sdd->cur_speed) {
 		sdd->cur_bpw = bpw;
 		sdd->cur_speed = speed;
+		sdd->cur_mode = spi->mode;
 		s3c64xx_spi_config(sdd);
 	}
 
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1432424 — [PATCH v2 5/5] spi: s3c64xx: use unsigned type for fifo handling variables

The 'quirks' variable cannot ever be negative, therefore use u8
instead of int. The 8 bit size is given from the fact that
currently the quirks variable has very few statuses.

The rx_lvl_offset and tx_st_done store shift values, so that u8
is a proper size.

fifo_lvl_mask stores a series of masks, to be in we will keep the
32 bit size.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 6d8486f..6c9503a 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -150,10 +150,10 @@ struct s3c64xx_spi_dma_data {
  * which is provided as driver data to the driver.
  */
 struct s3c64xx_spi_port_config {
-	int	fifo_lvl_mask[MAX_SPI_PORTS];
-	int	rx_lvl_offset;
-	int	tx_st_done;
-	int	quirks;
+	u32	fifo_lvl_mask[MAX_SPI_PORTS];
+	u8	rx_lvl_offset;
+	u8	tx_st_done;
+	u8	quirks;
 	bool	high_speed;
 	bool	clk_from_cmu;
 };
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1437098 — Re: [PATCH v2 5/5] spi: s3c64xx: use unsigned type for fifo handling variables

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jun 28, 2016 at 11:41:15AM +0900, Andi Shyti wrote:
> The 'quirks' variable cannot ever be negative, therefore use u8
> instead of int. The 8 bit size is given from the fact that
> currently the quirks variable has very few statuses.
> 
> The rx_lvl_offset and tx_st_done store shift values, so that u8
> is a proper size.
> 
> fifo_lvl_mask stores a series of masks, to be in we will keep the
> 32 bit size.

What's the advantage of these changes?  This feels like microptimisation
of something that shouldn't be a big performance issue and it's not
always the case that the compiler does something more sensible with
smaller or unsigned types.  I'm not saying don't do it, I'm just saying
that it's not clear to me what the win is.

[toc] | [prev] | [next] | [standalone]

#1437109 — Re: [PATCH v2 5/5] spi: s3c64xx: use unsigned type for fifo handling variables

Hi Mark,

> > The 'quirks' variable cannot ever be negative, therefore use u8
> > instead of int. The 8 bit size is given from the fact that
> > currently the quirks variable has very few statuses.
> > 
> > The rx_lvl_offset and tx_st_done store shift values, so that u8
> > is a proper size.
> > 
> > fifo_lvl_mask stores a series of masks, to be in we will keep the
> > 32 bit size.
> 
> What's the advantage of these changes?  This feels like microptimisation
> of something that shouldn't be a big performance issue and it's not
> always the case that the compiler does something more sensible with
> smaller or unsigned types.  I'm not saying don't do it, I'm just saying
> that it's not clear to me what the win is.

not much indeed. Just something that was a little bothering me
while I was reading through.

Please, feel free to drop it.

Thanks,
Andi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web