diff mbox series

[V3,5/9] spi: export spi core function spi_set_cs

Message ID 1555363834-32155-6-git-send-email-skomatineni@nvidia.com (mailing list archive)
State New, archived
Headers show
Series bug fixes and more features to Tegra SPI | expand

Commit Message

Sowjanya Komatineni April 15, 2019, 9:30 p.m. UTC
This patch exports spi_set_cs of the spi core to allow SPI masters
to use when gpio based chip select is needed.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/spi/spi.c       | 3 ++-
 include/linux/spi/spi.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Mark Brown April 19, 2019, 3:18 p.m. UTC | #1
On Mon, Apr 15, 2019 at 02:30:30PM -0700, Sowjanya Komatineni wrote:
> This patch exports spi_set_cs of the spi core to allow SPI masters
> to use when gpio based chip select is needed.

This isn't really what I meant when I said it'd be good to use the core
GPIO code - this function doesn't do a huge amount really and the usage
of it in your subsequent patch for the driver isn't exactly joined up
with the little it does (which is mainly swapping in the GPIO chip
select instead of the hardware chip select) isn't used in your driver
usage of this as far as I can see.  The bulk of the chip select handling
code in the core is actually in transfer_one_message() which your driver
doesn't use as it's got it's own implementation of that; I've not looked
in enough detail to figure out if it could use it.
Sowjanya Komatineni April 29, 2019, 10:02 p.m. UTC | #2
> On Mon, Apr 15, 2019 at 02:30:30PM -0700, Sowjanya Komatineni wrote:
> > This patch exports spi_set_cs of the spi core to allow SPI masters to 
> > use when gpio based chip select is needed.
>
> This isn't really what I meant when I said it'd be good to use the core GPIO code - this function doesn't do a huge amount really and the usage of it in your subsequent patch for the > driver isn't exactly joined up with the little it does (which is mainly swapping in the GPIO chip select instead of the hardware chip select) isn't used in your driver usage of this as far as I can see.  The bulk of the chip select handling code in the core is actually in transfer_one_message() which your driver doesn't use as it's got it's own implementation of that; I've not looked in enough detail to figure out if it could use it.
>
>
>
In SPI Tegra driver, we wanted to have GPIO based CS control when cs-gpios is specified in parallel to HW/SW CS. Having parallel GPIO based CS is to mimic some of the timing stuff that's needed for some spi devices by not actually using HW CS on platform but only for SPI HW design logic inside the chip.

Tegra spi driver don't use set_cs callback so looking into spi_set_cs from spi core implementation when cs-gpios property is used it exactly the same that is needed for GPIO control CS. So used this in V3. 

Can you please provide more details on what you are suggesting?
Do you prefer not to use SPI core spi_set_cs and gpio_set_values APIs and instead implement in tegra SPI driver using GPIO descriptors ?

Thanks
Sowjanya
Mark Brown May 6, 2019, 4:44 a.m. UTC | #3
On Mon, Apr 29, 2019 at 10:02:46PM +0000, Sowjanya Komatineni wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> > On Mon, Apr 15, 2019 at 02:30:30PM -0700, Sowjanya Komatineni wrote:
> > > This patch exports spi_set_cs of the spi core to allow SPI masters to 
> > > use when gpio based chip select is needed.

> > This isn't really what I meant when I said it'd be good to use the
> > core GPIO code - this function doesn't do a huge amount really and
> > the usage of it in your subsequent patch for the > driver isn't
> > exactly joined up with the little it does (which is mainly swapping
> > in the GPIO chip select instead of the hardware chip select) isn't
> > used in your driver usage of this as far as I can see.  The bulk of
> > the chip select handling code in the core is actually in
> > transfer_one_message() which your driver doesn't use as it's got
> > it's own implementation of that; I've not looked in enough detail to
> > figure out if it could use it.

> In SPI Tegra driver, we wanted to have GPIO based CS control when
> cs-gpios is specified in parallel to HW/SW CS. Having parallel GPIO
> based CS is to mimic some of the timing stuff that's needed for some
> spi devices by not actually using HW CS on platform but only for SPI
> HW design logic inside the chip.

> Tegra spi driver don't use set_cs callback so looking into spi_set_cs
> from spi core implementation when cs-gpios property is used it exactly
> the same that is needed for GPIO control CS. So used this in V3. 

> Can you please provide more details on what you are suggesting?
> Do you prefer not to use SPI core spi_set_cs and gpio_set_values APIs
> and instead implement in tegra SPI driver using GPIO descriptors ?

You're probably best open coding in the driver if there's value in using
the hardware chip select.
Sowjanya Komatineni May 10, 2019, 6:53 p.m. UTC | #4
> On Mon, Apr 29, 2019 at 10:02:46PM +0000, Sowjanya Komatineni wrote:
> 
> Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns.  Doing this makes your messages much easier to read and reply to.
> 
> > > On Mon, Apr 15, 2019 at 02:30:30PM -0700, Sowjanya Komatineni wrote:
> > > > This patch exports spi_set_cs of the spi core to allow SPI masters 
> > > > to use when gpio based chip select is needed.
>
> > > This isn't really what I meant when I said it'd be good to use the 
> > > core GPIO code - this function doesn't do a huge amount really and 
> > > the usage of it in your subsequent patch for the > driver isn't 
> > > exactly joined up with the little it does (which is mainly swapping 
> > > in the GPIO chip select instead of the hardware chip select) isn't 
> > > used in your driver usage of this as far as I can see.  The bulk of 
> > > the chip select handling code in the core is actually in
> > > transfer_one_message() which your driver doesn't use as it's got 
> > > it's own implementation of that; I've not looked in enough detail to 
> > > figure out if it could use it.
>
> > In SPI Tegra driver, we wanted to have GPIO based CS control when 
> > cs-gpios is specified in parallel to HW/SW CS. Having parallel GPIO 
> > based CS is to mimic some of the timing stuff that's needed for some 
> > spi devices by not actually using HW CS on platform but only for SPI 
> > HW design logic inside the chip.
>
> > Tegra spi driver don't use set_cs callback so looking into spi_set_cs 
> > from spi core implementation when cs-gpios property is used it exactly 
> > the same that is needed for GPIO control CS. So used this in V3.
>
> > Can you please provide more details on what you are suggesting?
> > Do you prefer not to use SPI core spi_set_cs and gpio_set_values APIs 
> > and instead implement in tegra SPI driver using GPIO descriptors ?
>
> You're probably best open coding in the driver if there's value in using the hardware chip select.

Sorry, Just to be clear on my understanding of your suggestion,

3 ways of CS control implementation is needed for Tegra SPI
- SW CS thru SPI Controller
- HW CS thru SPI Controller
- Direct GPIO based CS control 

Patch Series includes both HW CS and also direct GPIO based CS.

Regarding direct GPIO based CS, I understood you prefer to use GPIO descriptors.
I see SPI core set_cs API also uses GPIO descriptor for direct GPIO control of CS. 

Tegra SPI driver need parallel implementation of direct gpio based cs to hw/sw based
CS control thru SPI controller.
Since SPI core set_cs already has implementation using gpio descriptors, in V3 I am using
the same API.

Any concerns for using set_cs API from SPI core as it already does direct gpio based cs using
Descriptors?

Thanks
Sowjanya
Mark Brown May 12, 2019, 3:12 a.m. UTC | #5
On Fri, May 10, 2019 at 06:53:25PM +0000, Sowjanya Komatineni wrote:
> > On Mon, Apr 29, 2019 at 10:02:46PM +0000, Sowjanya Komatineni wrote:

To reiterate:

> > Please fix your mail client to word wrap within paragraphs at
> > something substantially less than 80 columns.  Doing this makes your
> > messages much easier to read and reply to.

> Any concerns for using set_cs API from SPI core as it already does direct gpio based cs using
> Descriptors?

Yes, that is precisely what I'm telling you not to do.  That function
doesn't do enough to make it worth exporting and the code that uses it
assumes it's managing the chip select entirely, adding a single other
use will make things fragile as it will mean that there's a chance
someone will change what the core is doing without taking into account
the different things that the Tegra code is doing.  Either let the core
handle chip select entirely or open code it.
diff mbox series

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 59b1e57cae74..fa70e595f17a 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -773,7 +773,7 @@  int spi_register_board_info(struct spi_board_info const *info, unsigned n)
 
 /*-------------------------------------------------------------------------*/
 
-static void spi_set_cs(struct spi_device *spi, bool enable)
+void spi_set_cs(struct spi_device *spi, bool enable)
 {
 	if (spi->mode & SPI_CS_HIGH)
 		enable = !enable;
@@ -801,6 +801,7 @@  static void spi_set_cs(struct spi_device *spi, bool enable)
 		spi->controller->set_cs(spi, !enable);
 	}
 }
+EXPORT_SYMBOL_GPL(spi_set_cs);
 
 #ifdef CONFIG_HAS_DMA
 int spi_map_buf(struct spi_controller *ctlr, struct device *dev,
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index fc4d21b4c2e4..c7ca95f26725 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -973,6 +973,7 @@  extern int spi_async(struct spi_device *spi, struct spi_message *message);
 extern int spi_async_locked(struct spi_device *spi,
 			    struct spi_message *message);
 extern int spi_slave_abort(struct spi_device *spi);
+extern void spi_set_cs(struct spi_device *spi, bool enable);
 
 static inline size_t
 spi_max_message_size(struct spi_device *spi)