| 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 |
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.
> 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
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.
> 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
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 --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)
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(-)